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

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

[Tvrtko Ursulin]

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.
>   	 */
> 

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

[Tvrtko Ursulin]

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