[Intel-gfx] [PATCH 4/4] drm/i915: Provide an assert for when we expect forcewake to be held

[Chris Wilson]

Quoting Mika Kuoppala (2017-10-09 11:05:40)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > Add assert_forcewakes_active() (the complementary function to
> > assert_forcewakes_inactive) that documents the requirement of a
> > function for its callers to be holding the forcewake ref (i.e. the
> > function is part of a sequence over which RC6 must be prevented).
> >
> > One such example is during ringbuffer reset, where RC6 must be held
> > across the whole reinitialisation sequence.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 11 ++++++++++-
> >  drivers/gpu/drm/i915/intel_uncore.c     | 12 ++++++++++++
> >  drivers/gpu/drm/i915/intel_uncore.h     |  2 ++
> >  3 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 05c08b0bc172..4285f09ff8b8 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -579,7 +579,16 @@ static int init_ring_common(struct intel_engine_cs *engine)
> >  static void reset_ring_common(struct intel_engine_cs *engine,
> >                             struct drm_i915_gem_request *request)
> >  {
> > -     /* Try to restore the logical GPU state to match the continuation
> > +     /*
> > +      * RC6 must be prevented until the reset is complete and the engine
> > +      * reinitialised. If it occurs in the middle of this sequence, the
> > +      * state written to/loaded from the power context is ill-defined (e.g.
> > +      * the PP_BASE_DIR may be lost).
> > +      */
> > +     assert_forcewakes_active(engine->i915, FORCEWAKE_ALL);
> > +
> > +     /*
> > +      * Try to restore the logical GPU state to match the continuation
> >        * of the request queue. If we skip the context/PD restore, then
> >        * the next request may try to execute assuming that its context
> >        * is valid and loaded on the GPU and so may try to access invalid
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index b3c3f94fc7e4..3d41667919dc 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -629,6 +629,18 @@ void assert_forcewakes_inactive(struct drm_i915_private *dev_priv)
> >       WARN_ON(dev_priv->uncore.fw_domains_active);
> >  }
> >  
> > +void assert_forcewakes_active(struct drm_i915_private *dev_priv,
> > +                           enum forcewake_domains fw_domains)
> > +{
> > +     if (!dev_priv->uncore.funcs.force_wake_get)
> > +             return;
> > +
> > +     assert_rpm_wakelock_held(dev_priv);
> > +
> > +     fw_domains &= dev_priv->uncore.fw_domains;
> > +     WARN_ON(fw_domains & ~dev_priv->uncore.fw_domains_active);
> > +}
> > +
> 
> Adding a debug telling about current domains seems not
> to add any value...atleast yet.

But will if it ever fires, so yes I had better add it.
s/WARN_ON/WARN(".../
-Chris