| Message ID | 20190801141732.31335-5-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915/pmu: Make more struct i915_pmu centric | expand |
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
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
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
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
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. */