| Message ID | 20190801101825.2784-5-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PMU update for more than one card | expand |
Quoting Tvrtko Ursulin (2019-08-01 11:18:25) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > With discrete graphics system can have both integrated and discrete GPU > handled by i915. > > Currently we use a fixed name ("i915") when registering as the uncore PMU > provider which stops working in this case. > > Add an unique instance number in form of "i915-<instance>" to each > registered PMU to support this. First instance keeps the old name to > preserve compatibility with existing userspace. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > --- > One problem is how to tie cars and PMUs. > > Should we append something better than the opaque instance (which > depends on probing order)? Like some bus slot id? But then we have to > say goodbye to preserving the "legacy" name "i915". > > I couldn't find that perf supports anything for these scenarios. > > Or maybe we can add a new file (sysfs?), which would be correctly under > the bus hierarchy, and would export the name of the PMU instance to > use? We definitely need to add some means of uniquely identifying i915 into sysfs, and for us to link from bus/pci/devices/ It seems odd that this is not a solved problem :| (Which means I've probably missed how it is meant to be done.) Storing a pmu-name inside sysfs seems reasonable, and postpones the problem of persistent device names for i915 until later :) (Fwiw, how about using i915-${pci_addr}, or even just ${pci_addr}? I expect that perf will grow multiple lookup paths if more hotpluggable devices start supporting perf eventss) > --- > drivers/gpu/drm/i915/i915_pmu.c | 22 ++++++++++++++++++++-- > drivers/gpu/drm/i915/i915_pmu.h | 8 ++++++++ > 2 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > index 7c48fcf0cccb..105246ae5160 100644 > --- a/drivers/gpu/drm/i915/i915_pmu.c > +++ b/drivers/gpu/drm/i915/i915_pmu.c > @@ -1053,6 +1053,8 @@ static void i915_pmu_unregister_cpuhp_state(struct i915_pmu *pmu) > cpuhp_remove_multi_state(cpuhp_slot); > } > > +static atomic_t i915_pmu_instance = ATOMIC_INIT(0); > + > void i915_pmu_register(struct drm_i915_private *i915) > { > struct i915_pmu *pmu = &i915->pmu; > @@ -1083,9 +1085,17 @@ void i915_pmu_register(struct drm_i915_private *i915) > hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > pmu->timer.function = i915_sample; > > - ret = perf_pmu_register(&pmu->base, "i915", -1); > + pmu->instance = atomic_inc_return(&i915_pmu_instance) - 1; > + if (pmu->instance) > + pmu->name = kasprintf(GFP_KERNEL, "i915-%u", pmu->instance); > + else > + pmu->name = "i915"; > + if (!pmu->name) > + goto err_instance; > + > + ret = perf_pmu_register(&pmu->base, pmu->name, -1); Iirc, the name is basically only used to construct the perf sysfs, and the tools extract the event names and tags from the sysfs. So I wonder if we could add a symlink... -Chris
On 01/08/2019 11:45, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-08-01 11:18:25) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> With discrete graphics system can have both integrated and discrete GPU >> handled by i915. >> >> Currently we use a fixed name ("i915") when registering as the uncore PMU >> provider which stops working in this case. >> >> Add an unique instance number in form of "i915-<instance>" to each >> registered PMU to support this. First instance keeps the old name to >> preserve compatibility with existing userspace. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> --- >> One problem is how to tie cars and PMUs. >> >> Should we append something better than the opaque instance (which >> depends on probing order)? Like some bus slot id? But then we have to >> say goodbye to preserving the "legacy" name "i915". >> >> I couldn't find that perf supports anything for these scenarios. >> >> Or maybe we can add a new file (sysfs?), which would be correctly under >> the bus hierarchy, and would export the name of the PMU instance to >> use? > > We definitely need to add some means of uniquely identifying i915 into > sysfs, and for us to link from bus/pci/devices/ It seems odd that this > is not a solved problem :| (Which means I've probably missed how it is > meant to be done.) I don't think there is the "how it is meant to be done" part. For instance even unplug/unload is not handled by the core. Which reminds me that I wanted to add a module_get/put somewhere in i915_pmu.c to at least same myself from doing i915_reload with intel_gpu_top in another window.. :) > Storing a pmu-name inside sysfs seems reasonable, and postpones the > problem of persistent device names for i915 until later :) > (Fwiw, how about using i915-${pci_addr}, or even just ${pci_addr}? I > expect that perf will grow multiple lookup paths if more hotpluggable > devices start supporting perf eventss) Yes pci address looks like a good candidate, but how important is legacy compatibility? Becuase I don't think we have ability to add a symlink at /sys/devices/i915 -> /sys/devices/i915-${pci}. Can we break intel_gpu_top or other tools which are only looking at /sys/devices/i915/events? > >> --- >> drivers/gpu/drm/i915/i915_pmu.c | 22 ++++++++++++++++++++-- >> drivers/gpu/drm/i915/i915_pmu.h | 8 ++++++++ >> 2 files changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c >> index 7c48fcf0cccb..105246ae5160 100644 >> --- a/drivers/gpu/drm/i915/i915_pmu.c >> +++ b/drivers/gpu/drm/i915/i915_pmu.c >> @@ -1053,6 +1053,8 @@ static void i915_pmu_unregister_cpuhp_state(struct i915_pmu *pmu) >> cpuhp_remove_multi_state(cpuhp_slot); >> } >> >> +static atomic_t i915_pmu_instance = ATOMIC_INIT(0); >> + >> void i915_pmu_register(struct drm_i915_private *i915) >> { >> struct i915_pmu *pmu = &i915->pmu; >> @@ -1083,9 +1085,17 @@ void i915_pmu_register(struct drm_i915_private *i915) >> hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); >> pmu->timer.function = i915_sample; >> >> - ret = perf_pmu_register(&pmu->base, "i915", -1); >> + pmu->instance = atomic_inc_return(&i915_pmu_instance) - 1; >> + if (pmu->instance) >> + pmu->name = kasprintf(GFP_KERNEL, "i915-%u", pmu->instance); >> + else >> + pmu->name = "i915"; >> + if (!pmu->name) >> + goto err_instance; >> + >> + ret = perf_pmu_register(&pmu->base, pmu->name, -1); > > Iirc, the name is basically only used to construct the perf sysfs, and > the tools extract the event names and tags from the sysfs. So I wonder > if we could add a symlink... Oh ok symlinks are here.. yeah, I thought we can't. Can we? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index 7c48fcf0cccb..105246ae5160 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -1053,6 +1053,8 @@ static void i915_pmu_unregister_cpuhp_state(struct i915_pmu *pmu) cpuhp_remove_multi_state(cpuhp_slot); } +static atomic_t i915_pmu_instance = ATOMIC_INIT(0); + void i915_pmu_register(struct drm_i915_private *i915) { struct i915_pmu *pmu = &i915->pmu; @@ -1083,9 +1085,17 @@ void i915_pmu_register(struct drm_i915_private *i915) hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); pmu->timer.function = i915_sample; - ret = perf_pmu_register(&pmu->base, "i915", -1); + pmu->instance = atomic_inc_return(&i915_pmu_instance) - 1; + if (pmu->instance) + pmu->name = kasprintf(GFP_KERNEL, "i915-%u", pmu->instance); + else + pmu->name = "i915"; + if (!pmu->name) + goto err_instance; + + ret = perf_pmu_register(&pmu->base, pmu->name, -1); if (ret) - goto err; + goto err_name; ret = i915_pmu_register_cpuhp_state(pmu); if (ret) @@ -1095,6 +1105,11 @@ void i915_pmu_register(struct drm_i915_private *i915) err_unreg: perf_pmu_unregister(&pmu->base); +err_name: + if (pmu->instance) + kfree(pmu->name); +err_instance: + atomic_dec(&i915_pmu_instance); err: pmu->base.event_init = NULL; free_event_attributes(pmu); @@ -1116,5 +1131,8 @@ void i915_pmu_unregister(struct drm_i915_private *i915) perf_pmu_unregister(&pmu->base); pmu->base.event_init = NULL; + atomic_dec(&i915_pmu_instance); + if (pmu->instance) + kfree(pmu->name); free_event_attributes(pmu); } diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h index 4fc4f2478301..5289d4fc82b5 100644 --- a/drivers/gpu/drm/i915/i915_pmu.h +++ b/drivers/gpu/drm/i915/i915_pmu.h @@ -46,6 +46,14 @@ struct i915_pmu { * @base: PMU base. */ struct pmu base; + /** + * @instance: PMU instance number when multiple devices. + */ + unsigned int instance; + /** + * @name: Name as registered with perf core. + */ + char *name; /** * @lock: Lock protecting enable mask and ref count handling. */