Re: [Intel-gfx] [PATCH] drm/i915/pmu: Check actual RC6 status

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Apr 01, 2021 at 10:38:11AM +0100, Tvrtko Ursulin wrote:
> 
> On 01/04/2021 10:19, Rodrigo Vivi wrote:
> > On Wed, Mar 31, 2021 at 11:18:50AM +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > RC6 support cannot be simply established by looking at the static device
> > > HAS_RC6() flag. There are cases which disable RC6 at driver load time so
> > > use the status of those check when deciding whether to enumerate the rc6
> > > counter.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Reported-by: Eero T Tamminen <eero.t.tamminen@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_pmu.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > > index 41651ac255fa..a75cd1db320b 100644
> > > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > > @@ -476,6 +476,8 @@ engine_event_status(struct intel_engine_cs *engine,
> > >   static int
> > >   config_status(struct drm_i915_private *i915, u64 config)
> > >   {
> > > +	struct intel_gt *gt = &i915->gt;
> > > +
> > >   	switch (config) {
> > >   	case I915_PMU_ACTUAL_FREQUENCY:
> > >   		if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> > > @@ -489,7 +491,7 @@ config_status(struct drm_i915_private *i915, u64 config)
> > >   	case I915_PMU_INTERRUPTS:
> > >   		break;
> > >   	case I915_PMU_RC6_RESIDENCY:
> > > -		if (!HAS_RC6(i915))
> > > +		if (!gt->rc6.supported)
> > 
> > Is this really going to remove any confusion?
> > Right now it is there but with residency 0, but after this change the event is
> > not there anymore so I wonder if we are not just changing to a different kind
> > of confusion on users.
> 
> I think it is possible to argue both ways.
> 
> 1)
> HAS_RC6 means hardware has RC6 so if we view PMU as very low level we can
> say always export it.
> 
> If i915 had to turn it off (rc6->supported == false) due firmware or GVT-g,
> then we could say reporting zero RC6 is accurate in that sense. Only the
> reason "why it is zero" is missing for PMU users.
> 
> 2)
> Or if we go with this patch we could say that presence of the PMU metric
> means RC6 is active and enabled, while absence means it is either not
> supported due platform (or firmware) or how the platform is getting used
> (GVT-g).
>

yeap, these 2 cases described well my mental conflict...

> So I think patch is a bit better. I don't see it is adding more confusion.

As I said on the other patch I have no strong position on which is better,
but if you and Eero feel that this works better for the current case,
let's do it...

> 
> > 
> > >   			return -ENODEV;
> > 
> > would a different return help somehow?
> 
> Like distinguishing between not theoretically possible to support on this
> GPU, versus not active? Perhaps.. suggest an errno? :)

ENODATA? or EIDRM?

But only if it helps somehow... otherwise don't bother and move with
this as is:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Regards,
> 
> Tvrtko
> 
> > 
> > >   		break;
> > >   	case I915_PMU_SOFTWARE_GT_AWAKE_TIME:
> > > -- 
> > > 2.27.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel