[Intel-gfx] [PATCH 4/4] drm/i915: Park before resetting the submission backend

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-04-05 12:54:38)
> 
> 
> On 4/5/2018 4:32 PM, Chris Wilson wrote:
> > As different backends may have different park/unpark callbacks, we
> > should only ever switch backends (reset_default_submission on wedge
> > recovery, or on enabling the guc) while parked.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c             | 11 +++++++++++
> >   drivers/gpu/drm/i915/intel_engine_cs.c      |  3 +++
> >   drivers/gpu/drm/i915/intel_guc_submission.c |  1 +
> >   3 files changed, 15 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index e148db310ea6..e2880de2fc7e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3380,6 +3380,17 @@ bool i915_gem_unset_wedged(struct drm_i915_private *i915)
> >       i915_retire_requests(i915);
> >       GEM_BUG_ON(i915->gt.active_requests);
> >   
> > +     /*
> > +      * Park before disengaging the old submit mechanism as different
> > +      * backends may have different park/unpack callbacks.
> > +      *
> > +      * We are idle; the idle-worker will be queued, but we need to run
> > +      * it now. As we already hold the struct mutex, we can get park
> > +      * the GPU right away, letting the lazy worker see that we are
> > +      * already active again by the time it acquires the mutex.
> > +      */
> > +     i915_gem_park(i915);
> I think we should be calling this before gem_unset_wedged in i915_reset.

We can't, we aren't yet conclusively idle at that point. This is the
earliest in that sequence where we know we are.

> With GuC, hitting the GEM_BUG_ON(awake) in guc_submission_enable.

Hmm. We definitely need to keep park/unpark balanced. Brute force would
be to force idle at that point. Or we do if (awake) engine->unpark on
the takeover.

> Also idle_work can execute just before reset so GEM_BUG_ON(!awake) in 
> gem_park can be hit I think.

True, we could just ignore the whole wait-for-idle if already !gt.awake.
-Chris