drm/i915/pmu: Do not use colon characters in PMU names
diff mbox series

Message ID 20200110111126.28241-1-tvrtko.ursulin@linux.intel.com
State New
Headers show
Series
  • drm/i915/pmu: Do not use colon characters in PMU names
Related show

Commit Message

Tvrtko Ursulin Jan. 10, 2020, 11:11 a.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We use PCI device path in the registered PMU name in order to distinguish
between multiple GPUs. But since tools/perf reserves a special meaning to
the colon character we need to transliterate them to something else. We
choose a dash.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reported-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Fixes: 05488673a4d4 ("drm/i915/pmu: Support multiple GPUs")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_pmu.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Chris Wilson Jan. 10, 2020, 11:21 a.m. UTC | #1
Quoting Tvrtko Ursulin (2020-01-10 11:11:26)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> We use PCI device path in the registered PMU name in order to distinguish
> between multiple GPUs. But since tools/perf reserves a special meaning to
> the colon character we need to transliterate them to something else. We
> choose a dash.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reported-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
> Fixes: 05488673a4d4 ("drm/i915/pmu: Support multiple GPUs")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index f3ef6700a5f2..ecbd0e1f1a90 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -1117,12 +1117,22 @@ void i915_pmu_register(struct drm_i915_private *i915)
>         hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>         pmu->timer.function = i915_sample;
>  
> -       if (!is_igp(i915))
> +       if (!is_igp(i915)) {
>                 pmu->name = kasprintf(GFP_KERNEL,
>                                       "i915-%s",
>                                       dev_name(i915->drm.dev));
> -       else
> +               if (pmu->name) {

/* tools/perf reserves colons as special. */
strreplace(pmu->name, ':', '-');

I worry because the err_idx pointed to the '-'. We may have to use _
-Chris
Tvrtko Ursulin Jan. 10, 2020, 11:27 a.m. UTC | #2
On 10/01/2020 11:21, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-01-10 11:11:26)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> We use PCI device path in the registered PMU name in order to distinguish
>> between multiple GPUs. But since tools/perf reserves a special meaning to
>> the colon character we need to transliterate them to something else. We
>> choose a dash.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reported-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
>> Fixes: 05488673a4d4 ("drm/i915/pmu: Support multiple GPUs")
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Andi Kleen <ak@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_pmu.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>> index f3ef6700a5f2..ecbd0e1f1a90 100644
>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>> @@ -1117,12 +1117,22 @@ void i915_pmu_register(struct drm_i915_private *i915)
>>          hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>>          pmu->timer.function = i915_sample;
>>   
>> -       if (!is_igp(i915))
>> +       if (!is_igp(i915)) {
>>                  pmu->name = kasprintf(GFP_KERNEL,
>>                                        "i915-%s",
>>                                        dev_name(i915->drm.dev));
>> -       else
>> +               if (pmu->name) {
> 
> /* tools/perf reserves colons as special. */
> strreplace(pmu->name, ':', '-');

I didn't know this exists, thanks.

> I worry because the err_idx pointed to the '-'. We may have to use _

What is err_idx? But yes.. it would had served me well to test before 
sending. :) I just find identifiers with a mix of underscores and dashes 
so visually unappealing. :(

Regards,

Tvrtko
Chris Wilson Jan. 10, 2020, 11:32 a.m. UTC | #3
Quoting Tvrtko Ursulin (2020-01-10 11:27:55)
> 
> On 10/01/2020 11:21, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-01-10 11:11:26)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> We use PCI device path in the registered PMU name in order to distinguish
> >> between multiple GPUs. But since tools/perf reserves a special meaning to
> >> the colon character we need to transliterate them to something else. We
> >> choose a dash.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> Reported-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
> >> Fixes: 05488673a4d4 ("drm/i915/pmu: Support multiple GPUs")
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Andi Kleen <ak@linux.intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_pmu.c | 14 ++++++++++++--
> >>   1 file changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> >> index f3ef6700a5f2..ecbd0e1f1a90 100644
> >> --- a/drivers/gpu/drm/i915/i915_pmu.c
> >> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> >> @@ -1117,12 +1117,22 @@ void i915_pmu_register(struct drm_i915_private *i915)
> >>          hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> >>          pmu->timer.function = i915_sample;
> >>   
> >> -       if (!is_igp(i915))
> >> +       if (!is_igp(i915)) {
> >>                  pmu->name = kasprintf(GFP_KERNEL,
> >>                                        "i915-%s",
> >>                                        dev_name(i915->drm.dev));
> >> -       else
> >> +               if (pmu->name) {
> > 
> > /* tools/perf reserves colons as special. */
> > strreplace(pmu->name, ':', '-');
> 
> I didn't know this exists, thanks.
> 
> > I worry because the err_idx pointed to the '-'. We may have to use _
> 
> What is err_idx? But yes.. it would had served me well to test before 
> sending. :) I just find identifiers with a mix of underscores and dashes 
> so visually unappealing. :(

event syntax error: 'i915-0000:00:02.0/bcs0-busy/'
                         \___ parser error

The parser sets err_idx on the character it failed at, and the error
message includes it. So unless we lost whitespace in all the cutting and
pasting, that says it barfed at '-'
-Chris
Tvrtko Ursulin Jan. 10, 2020, 11:40 a.m. UTC | #4
On 10/01/2020 11:32, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-01-10 11:27:55)
>>
>> On 10/01/2020 11:21, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2020-01-10 11:11:26)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> We use PCI device path in the registered PMU name in order to distinguish
>>>> between multiple GPUs. But since tools/perf reserves a special meaning to
>>>> the colon character we need to transliterate them to something else. We
>>>> choose a dash.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Reported-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
>>>> Fixes: 05488673a4d4 ("drm/i915/pmu: Support multiple GPUs")
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_pmu.c | 14 ++++++++++++--
>>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>>>> index f3ef6700a5f2..ecbd0e1f1a90 100644
>>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>>> @@ -1117,12 +1117,22 @@ void i915_pmu_register(struct drm_i915_private *i915)
>>>>           hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>>>>           pmu->timer.function = i915_sample;
>>>>    
>>>> -       if (!is_igp(i915))
>>>> +       if (!is_igp(i915)) {
>>>>                   pmu->name = kasprintf(GFP_KERNEL,
>>>>                                         "i915-%s",
>>>>                                         dev_name(i915->drm.dev));
>>>> -       else
>>>> +               if (pmu->name) {
>>>
>>> /* tools/perf reserves colons as special. */
>>> strreplace(pmu->name, ':', '-');
>>
>> I didn't know this exists, thanks.
>>
>>> I worry because the err_idx pointed to the '-'. We may have to use _
>>
>> What is err_idx? But yes.. it would had served me well to test before
>> sending. :) I just find identifiers with a mix of underscores and dashes
>> so visually unappealing. :(
> 
> event syntax error: 'i915-0000:00:02.0/bcs0-busy/'
>                           \___ parser error
> 
> The parser sets err_idx on the character it failed at, and the error
> message includes it. So unless we lost whitespace in all the cutting and
> pasting, that says it barfed at '-'

Oh right, interesting that it has no problem with a dash in event name. 
In v2 full event string is:

   i915_0000_00_02.0/vcs0-busy/

A bit ugly but seems to work.

Regards,

Tvrtko

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index f3ef6700a5f2..ecbd0e1f1a90 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -1117,12 +1117,22 @@  void i915_pmu_register(struct drm_i915_private *i915)
 	hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	pmu->timer.function = i915_sample;
 
-	if (!is_igp(i915))
+	if (!is_igp(i915)) {
 		pmu->name = kasprintf(GFP_KERNEL,
 				      "i915-%s",
 				      dev_name(i915->drm.dev));
-	else
+		if (pmu->name) {
+			char *p;
+
+			/* tools/perf reserves colons as special. */
+			for (p = (char *)pmu->name; *p; p++) {
+				if (*p == ':')
+					*p = '-';
+			}
+		}
+	} else {
 		pmu->name = "i915";
+	}
 	if (!pmu->name)
 		goto err;