Re: [Intel-gfx] [PATCH v4 5/5] drm/i915/pmu: Support multiple GPUs

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi guys,

Any feedback on the below?

On 06/09/2019 16:47, Tvrtko Ursulin wrote:
> 
> Peter, Thomas,
> 
> If you could spare a moment for some brainstorming on the topic of 
> uncore PMU and multiple providers it would be appreciated.
> 
> So from i915 we export some metrics as uncore PMU, which shows up under 
> /sys/devices/i915. Shortsightedness or what, we did not realize that one 
> day we could have more than one i915 device in a system which now 
> creates a problem, or at least raises a question on naming.
> 
> The patch below works around this by appending the PCI device name to 
> additional instances of i915 when it registers with perf_pmu_register.
> 
> Question is if there is a better solution, or if not, whether you are 
> aware of any plans to extend the perf core to better support this? Are 
> there any other uncore PMU providers in an identical situation?
> 
> Regards,
> 
> Tvrtko
> 
> On 01/08/2019 16:54, Tvrtko Ursulin wrote:
>> 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.
>>
>> To fix this we add the PCI device name string to non-integrated devices
>> handled by us. Integrated devices keep the legacy name preserving
>> backward compatibility.
>>
>> v2:
>>   * Detect IGP and keep legacy name. (Michal)
>>   * Use PCI device name as suffix. (Michal, Chris)
>>
>> v3:
>>   * Constify the name. (Chris)
>>   * Use pci_domain_nr. (Chris)
>>
>> v4:
>>   * Fix kfree_const usage. (Chris)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_pmu.c | 25 +++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
>>   2 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c 
>> b/drivers/gpu/drm/i915/i915_pmu.c
>> index e0e0180bca7c..e0fea227077e 100644
>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>> @@ -1053,6 +1053,15 @@ static void 
>> i915_pmu_unregister_cpuhp_state(struct i915_pmu *pmu)
>>       cpuhp_remove_multi_state(cpuhp_slot);
>>   }
>> +static bool is_igp(struct pci_dev *pdev)
>> +{
>> +    /* IGP is 0000:00:02.0 */
>> +    return pci_domain_nr(pdev->bus) == 0 &&
>> +           pdev->bus->number == 0 &&
>> +           PCI_SLOT(pdev->devfn) == 2 &&
>> +           PCI_FUNC(pdev->devfn) == 0;
>> +}
>> +
>>   void i915_pmu_register(struct drm_i915_private *i915)
>>   {
>>       struct i915_pmu *pmu = &i915->pmu;
>> @@ -1083,10 +1092,19 @@ 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);
>> -    if (ret)
>> +    if (!is_igp(i915->drm.pdev))
>> +        pmu->name = kasprintf(GFP_KERNEL,
>> +                      "i915-%s",
>> +                      dev_name(i915->drm.dev));
>> +    else
>> +        pmu->name = "i915";
>> +    if (!pmu->name)
>>           goto err;
>> +    ret = perf_pmu_register(&pmu->base, pmu->name, -1);
>> +    if (ret)
>> +        goto err_name;
>> +
>>       ret = i915_pmu_register_cpuhp_state(pmu);
>>       if (ret)
>>           goto err_unreg;
>> @@ -1095,6 +1113,8 @@ void i915_pmu_register(struct drm_i915_private 
>> *i915)
>>   err_unreg:
>>       perf_pmu_unregister(&pmu->base);
>> +err_name:
>> +    kfree_const(pmu->name);
>>   err:
>>       pmu->base.event_init = NULL;
>>       free_event_attributes(pmu);
>> @@ -1116,5 +1136,6 @@ void i915_pmu_unregister(struct drm_i915_private 
>> *i915)
>>       perf_pmu_unregister(&pmu->base);
>>       pmu->base.event_init = NULL;
>> +    kfree_const(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..ff24f3bb0102 100644
>> --- a/drivers/gpu/drm/i915/i915_pmu.h
>> +++ b/drivers/gpu/drm/i915/i915_pmu.h
>> @@ -46,6 +46,10 @@ struct i915_pmu {
>>        * @base: PMU base.
>>        */
>>       struct pmu base;
>> +    /**
>> +     * @name: Name as registered with perf core.
>> +     */
>> +    const char *name;
>>       /**
>>        * @lock: Lock protecting enable mask and ref count handling.
>>        */
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx