[Intel-gfx] [RFC 4/4] drm/i915/pmu: Support multiple GPUs

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-08-01 11:18:25)
> From: Tvrtko Ursulin <tvrtko.ursulin at 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 at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko at 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