Re: [Intel-gfx] [PATCH] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Fri, 12 May 2023 01:59:08 -0700, Tvrtko Ursulin wrote:
>

Hi Tvrtko,

> On 12/05/2023 02:53, Ashutosh Dixit wrote:
> > pmu_needs_timer() keeps the timer running even when GT is parked,
> > ostensibly to sample requested/actual frequencies. However
> > frequency_sample() has the following:
> >
> >	/* Report 0/0 (actual/requested) frequency while parked. */
> >	if (!intel_gt_pm_get_if_awake(gt))
> >		return;
> >
> > The above code prevents frequencies to be sampled while the GT is
> > parked. So we might as well turn off the sampling timer itself in this
> > case and save CPU cycles/power.
>
> The confusing situation seems to be the consequence of b66ecd0438bf
> ("drm/i915/pmu: Report frequency as zero while GPU is sleeping").
>
> Before that commit we were deliberately sampling the frequencies as GPU
> minimum during the parked periods and to do so leaving the timer running.
>
> But then some RPS changes exposed that approach as questionable (AFAIR
> software tracked state stopped being reset to min freq and so created
> wild PMU readings) and we went the route of reporting zero when parked.
>
> At which point running the timer stopped making sense, so really that
> commit should/could have made the change you now propose.
>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_pmu.c | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index 7ece883a7d956..8db1d681cf4ab 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -124,11 +124,14 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
> >		  ENGINE_SAMPLE_MASK;
> >		/*
> > -	 * When the GPU is idle per-engine counters do not need to be
> > -	 * running so clear those bits out.
> > +	 * When GPU is idle, frequency or per-engine counters do not need
> > +	 * to be running so clear those bits out.
> >	 */
> > -	if (!gpu_active)
> > -		enable &= ~ENGINE_SAMPLE_MASK;
> > +	if (!gpu_active) {
> > +		enable &= ~(config_mask(I915_PMU_ACTUAL_FREQUENCY) |
> > +			    config_mask(I915_PMU_REQUESTED_FREQUENCY) |
> > +			    ENGINE_SAMPLE_MASK);
> > +	}
> >	/*
> >	 * Also there is software busyness tracking available we do not
> >	 * need the timer for I915_SAMPLE_BUSY counter.
>
> LGTM.
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Or maybe it is possible to simplify since it looks there is no way to return true if gt is parked. So that could be:
>
> pmu_needs_timer(..)
> {
>	...
>
>	if (!gpu_active)
>		return false;
>
>	...
>	enable = pmu->enable;
>
>	...
>	enable &= config_mask(I915_PMU_ACTUAL_FREQUENCY) |
>		  config_mask(I915_PMU_REQUESTED_FREQUENCY) |
>		  ENGINE_SAMPLE_MASK;
>
>	...
>	if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
>		enable &= ~BIT(I915_SAMPLE_BUSY);
>
>	return enable;
> }
>
> Not sure it is any better, your call.

I have made this change in v2 of the patch submitted here:

https://patchwork.freedesktop.org/series/118225/

And I have retained your R-b since the patch is essentially a copy of your
code above.

The original version of the patch was here:

https://patchwork.freedesktop.org/series/117658/

Thanks.
--
Ashutosh