[RFC,4/4] drm/i915/pmu: Support multiple GPUs
diff mbox series

Message ID 20190801101825.2784-5-tvrtko.ursulin@linux.intel.com
State New
Headers show
Series
  • PMU update for more than one card
Related show

Commit Message

Tvrtko Ursulin Aug. 1, 2019, 10:18 a.m. UTC
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.

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@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@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?
---
 drivers/gpu/drm/i915/i915_pmu.c | 22 ++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_pmu.h |  8 ++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

Comments

Chris Wilson Aug. 1, 2019, 10:45 a.m. UTC | #1
Quoting Tvrtko Ursulin (2019-08-01 11:18:25)
> 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.
> 
> 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@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@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
Tvrtko Ursulin Aug. 1, 2019, 12:03 p.m. UTC | #2
On 01/08/2019 11:45, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-08-01 11:18:25)
>> 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.
>>
>> 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@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@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

Patch
diff mbox series

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);
 	if (ret)
-		goto err;
+		goto err_name;
 
 	ret = i915_pmu_register_cpuhp_state(pmu);
 	if (ret)
@@ -1095,6 +1105,11 @@  void i915_pmu_register(struct drm_i915_private *i915)
 
 err_unreg:
 	perf_pmu_unregister(&pmu->base);
+err_name:
+	if (pmu->instance)
+		kfree(pmu->name);
+err_instance:
+	atomic_dec(&i915_pmu_instance);
 err:
 	pmu->base.event_init = NULL;
 	free_event_attributes(pmu);
@@ -1116,5 +1131,8 @@  void i915_pmu_unregister(struct drm_i915_private *i915)
 
 	perf_pmu_unregister(&pmu->base);
 	pmu->base.event_init = NULL;
+	atomic_dec(&i915_pmu_instance);
+	if (pmu->instance)
+		kfree(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..5289d4fc82b5 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -46,6 +46,14 @@  struct i915_pmu {
 	 * @base: PMU base.
 	 */
 	struct pmu base;
+	/**
+	 * @instance: PMU instance number when multiple devices.
+	 */
+	unsigned int instance;
+	/**
+	 * @name: Name as registered with perf core.
+	 */
+	char *name;
 	/**
 	 * @lock: Lock protecting enable mask and ref count handling.
 	 */