On 01/08/2019 11:45, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-08-01 11:18:25)From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> 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@xxxxxxxxx> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> --- 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
