[Intel-gfx] [PATCH] drm/i915/pmu: Clear the previous sample value when parking

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-23 06:56:02)
> 
> On 23/11/2017 00:06, Chris Wilson wrote:
> > When turning off the engines, and the pmu sampling, clear the previous
> > value as the current measurement should be 0.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_pmu.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index f1e932a3fb85..6f1316f5039a 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -135,9 +135,18 @@ static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
> >   
> >   void i915_pmu_gt_parked(struct drm_i915_private *i915)
> >   {
> > +     struct intel_engine_cs *engine;
> > +     enum intel_engine_id id;
> > +
> >       if (!i915->pmu.base.event_init)
> >               return;
> >   
> > +     for_each_engine(engine, i915, id) {
> > +             engine->pmu.sample[I915_SAMPLE_BUSY].prev = 0;
> > +             engine->pmu.sample[I915_SAMPLE_WAIT].prev = 0;
> > +             engine->pmu.sample[I915_SAMPLE_SEMA].prev = 0;
> > +     }
> > +
> >       spin_lock_irq(&i915->pmu.lock);
> >       /*
> >        * Signal sampling timer to stop if only engine events are enabled and
> > 
> 
> Yes, this is indeed required.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Mind if I change it to use a loop to I915_ENGINE_SAMPLE_MAX, and then 
> follow up with a patch which moves this define from uapi to i915_pmu.h? 
> Since during engine classes we realized "/* non-ABI */" enums do not 
> make sense in the uapi?

Sure. I also think you might want to take a look at how we might just
use the timer to finish the last incomplete sample and then turning it
off. As I started to get worried about the sampler running concurrently.
If we do something like that, we can then assume that we hold the rpm
wakeref for the sampler, as it will only be running while gt.awake.

So whilst something like this is required, this is only a step in the
right direction.
-Chris