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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx