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

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

On Wed, 24 May 2023 02:12:31 -0700, Andrzej Hajda wrote:
>

Hi Andrzej,

> On 23.05.2023 17:19, 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.
> >
> > v2: Instead of turning freq bits off, return false, since no counters will
> >      run after this change when GT is parked (Tvrtko)
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_pmu.c | 12 +++++-------
> >   1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index a814583e19fd7..b47d890d4ada1 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -144,6 +144,10 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
> >	struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
> >	u32 enable;
> >   +	/* When GPU is idle, at present no counters need to run */
> > +	if (!gpu_active)
> > +		return false;
> > +
>
> What is then purpose of calling pmu_needs_timer with 2nd arg false?
> Why not just replace all occurrences of pmu_needs_timer(.., false) with
> false? And remove the 2nd argument.

OK, this didn't seem unreasonable so I went ahead and made this change in
Patch v3. Copying Tvrtko too in case he prefers v2 for any reason. Please
review.

Thanks.
--
Ashutosh


>
>
>
> >	/*
> >	 * Only some counters need the sampling timer.
> >	 *
> > @@ -157,17 +161,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
> >	 */
> >	enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
> >   -	/*
> > -	 * When the GPU is idle per-engine counters do not need to be
> > -	 * running so clear those bits out.
> > -	 */
> > -	if (!gpu_active)
> > -		enable &= ~ENGINE_SAMPLE_MASK;
> >	/*
> >	 * Also there is software busyness tracking available we do not
> >	 * need the timer for I915_SAMPLE_BUSY counter.
> >	 */
> > -	else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
> > +	if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
> >		enable &= ~BIT(I915_SAMPLE_BUSY);
> >		/*
>