[Intel-gfx] [PATCH] drm/i915/pmu: Report frequency as zero while GPU is sleeping

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-11-28 16:10:51)
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> We used to report the minimum possible frequency as both requested and
> active while GPU was in sleep state. This was a consequence of sampling
> the value from the "current frequency" field in our software tracking.
> 
> This was strictly speaking wrong, but given that until recently the
> current frequency in sleeping state used to be equal to minimum, it did
> not stand out sufficiently to be noticed as such.
> 
> After some recent changes have made the current frequency be reported
> as last active before GPU went to sleep, meaning both requested and active
> frequencies could end up being reported at their maximum values for the
> duration of the GPU idle state, it became much more obvious that this does
> not make sense.
> 
> To fix this we will now sample the frequency counters only when the GPU is
> awake. As a consequence reported frequencies could be reported as below
> the GPU reported minimum but that should be much less confusing that the
> current situation.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Hmm. 0/0 while off, that will be a bit of a shock.

Idea/codewise
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

I'll dogfood it a bit to see how much getting used to it will take :)

> ---
>  drivers/gpu/drm/i915/i915_pmu.c | 44 +++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 95e824a78d4d..b576a2be9f81 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -378,32 +378,32 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns)
>         struct i915_pmu *pmu = &i915->pmu;
>         struct intel_rps *rps = &gt->rps;
>  
> +       if (!(pmu->enable &
> +             (config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY) |
> +              config_enabled_mask(I915_PMU_REQUESTED_FREQUENCY))) ||
> +           !intel_gt_pm_get_if_awake(gt))
> +               return;

Fwiw, I'd put this as two if()s since the multiline flags makes it
harder to read. Any chance we could do a

if (!frequency_sample_enabled(pmu))
	return;

/* Report 0/0 (actual/requested) frequency while parked */
if (!intel_gt_pm_get_if_awake(gt))
	return;

-Chris