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

[Tvrtko Ursulin]

On 01/08/2019 11:45, Chris Wilson wrote:
> 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.)

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