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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
> 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
