| Message ID | 20200110111126.28241-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pmu: Do not use colon characters in PMU names | expand |
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
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
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
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
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;