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@xxxxxxxxx> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > > --- > > 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); > > /* >
