[5/5] drm/i915/pmu: Support multiple GPUs
diff mbox series

Message ID 20190801141732.31335-5-tvrtko.ursulin@linux.intel.com
State New
Headers show
Series
  • [1/5] drm/i915/pmu: Make more struct i915_pmu centric
Related show

Commit Message

Tvrtko Ursulin Aug. 1, 2019, 2:17 p.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.

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)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
Is our GPU always "0000:00:02.0"? CI will tell me.
---
 drivers/gpu/drm/i915/i915_pmu.c | 27 +++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
 2 files changed, 29 insertions(+), 2 deletions(-)

Comments

Chris Wilson Aug. 1, 2019, 2:54 p.m. UTC | #1
Quoting Tvrtko Ursulin (2019-08-01 15:17:32)
> 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)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
> Is our GPU always "0000:00:02.0"? CI will tell me.

It always has been. (With a few additional 2.1 for Windows95 multihead
where each head had to be a unique device!)

One hopes that by now it is firmly ingrained that it will always be
kept to 00:02.0.

> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 27 +++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index e0e0180bca7c..9a404d85c4e9 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 pdev->bus->parent == NULL &&

pci_domain_nr(pdev->bus) == 0 ?

> +              pdev->bus->number == 0 &&
> +              PCI_SLOT(pdev->devfn) == 2 &&
> +              PCI_FUNC(pdev->devfn) == 0;

I am surprised there isn't already a convenience function. None that I
could find.

> +}
> +
>  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";

Makes sense, and quite a neat solution.

> +       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,9 @@ void i915_pmu_register(struct drm_i915_private *i915)
>  
>  err_unreg:
>         perf_pmu_unregister(&pmu->base);
> +err_name:
> +       if (!is_igp(i915->drm.pdev))
> +               kfree(pmu->name);
kfree_const(pmu->name);

>  err:
>         pmu->base.event_init = NULL;
>         free_event_attributes(pmu);
> @@ -1116,5 +1137,7 @@ void i915_pmu_unregister(struct drm_i915_private *i915)
>  
>         perf_pmu_unregister(&pmu->base);
>         pmu->base.event_init = NULL;
> +       if (!is_igp(i915->drm.pdev))
> +               kfree(pmu->name);

kfree_const(pmu->name);

Works for me, I wonder what PeterZ will say...
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Tvrtko Ursulin Aug. 1, 2019, 3:10 p.m. UTC | #2
On 01/08/2019 15:54, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-08-01 15:17:32)
>> 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)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>> Is our GPU always "0000:00:02.0"? CI will tell me.
> 
> It always has been. (With a few additional 2.1 for Windows95 multihead
> where each head had to be a unique device!)
> 
> One hopes that by now it is firmly ingrained that it will always be
> kept to 00:02.0.

Re-assuring, thanks! However still some tests to do before I am happy 
this is upstream worthy, not least CI.

> 
>> ---
>>   drivers/gpu/drm/i915/i915_pmu.c | 27 +++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/i915_pmu.h |  4 ++++
>>   2 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>> index e0e0180bca7c..9a404d85c4e9 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 pdev->bus->parent == NULL &&
> 
> pci_domain_nr(pdev->bus) == 0 ?

Aha, thanks!

> 
>> +              pdev->bus->number == 0 &&
>> +              PCI_SLOT(pdev->devfn) == 2 &&
>> +              PCI_FUNC(pdev->devfn) == 0;
> 
> I am surprised there isn't already a convenience function. None that I
> could find.
> 
>> +}
>> +
>>   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";
> 
> Makes sense, and quite a neat solution.
> 
>> +       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,9 @@ void i915_pmu_register(struct drm_i915_private *i915)
>>   
>>   err_unreg:
>>          perf_pmu_unregister(&pmu->base);
>> +err_name:
>> +       if (!is_igp(i915->drm.pdev))
>> +               kfree(pmu->name);
> kfree_const(pmu->name);
> 
>>   err:
>>          pmu->base.event_init = NULL;
>>          free_event_attributes(pmu);
>> @@ -1116,5 +1137,7 @@ void i915_pmu_unregister(struct drm_i915_private *i915)
>>   
>>          perf_pmu_unregister(&pmu->base);
>>          pmu->base.event_init = NULL;
>> +       if (!is_igp(i915->drm.pdev))
>> +               kfree(pmu->name);
> 
> kfree_const(pmu->name);
> 
> Works for me, I wonder what PeterZ will say...

In what sense?

> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks,

Tvrtko
Chris Wilson Aug. 1, 2019, 3:20 p.m. UTC | #3
Quoting Tvrtko Ursulin (2019-08-01 16:10:14)
> 
> On 01/08/2019 15:54, Chris Wilson wrote:
> > Works for me, I wonder what PeterZ will say...
> 
> In what sense?

Just wondering if he has a plan for hotpluggable pmu devices. I can
certainly imagine his surprise in the future when he finds an adhoc
scheme in a random driver.
-Chris
Tvrtko Ursulin Aug. 1, 2019, 3:31 p.m. UTC | #4
On 01/08/2019 16:20, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-08-01 16:10:14)
>>
>> On 01/08/2019 15:54, Chris Wilson wrote:
>>> Works for me, I wonder what PeterZ will say...
>>
>> In what sense?
> 
> Just wondering if he has a plan for hotpluggable pmu devices. I can
> certainly imagine his surprise in the future when he finds an adhoc
> scheme in a random driver.

There is time to run this past him. I'll send something out.

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 e0e0180bca7c..9a404d85c4e9 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 pdev->bus->parent == NULL &&
+	       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,9 @@  void i915_pmu_register(struct drm_i915_private *i915)
 
 err_unreg:
 	perf_pmu_unregister(&pmu->base);
+err_name:
+	if (!is_igp(i915->drm.pdev))
+		kfree(pmu->name);
 err:
 	pmu->base.event_init = NULL;
 	free_event_attributes(pmu);
@@ -1116,5 +1137,7 @@  void i915_pmu_unregister(struct drm_i915_private *i915)
 
 	perf_pmu_unregister(&pmu->base);
 	pmu->base.event_init = NULL;
+	if (!is_igp(i915->drm.pdev))
+		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..8d7a388dcdc7 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.
+	 */
+	char *name;
 	/**
 	 * @lock: Lock protecting enable mask and ref count handling.
 	 */