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

[Chris Wilson]

Quoting Michal Wajdeczko (2018-04-06 14:39:28)
> On Fri, 06 Apr 2018 14:25:48 +0200, Chris Wilson  
> <chris at chris-wilson.co.uk> wrote:
> 
> > 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.
> >> With GuC, hitting the GEM_BUG_ON(awake) in guc_submission_enable.
> >
> > Right, we really do need to restore guc submission before restarting. So
> > how can we fit in something like
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c  
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 9650a7b10c5f..95fa30d9aec6 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3132,6 +3132,9 @@ void i915_gem_reset(struct drm_i915_private  
> > *dev_priv)
> >        i915_retire_requests(dev_priv);
> > +       if (USES_GUC_SUBMISSION(dev_priv))
> > +               (void)intel_guc_submission_enable(guc);
> > +
> >         for_each_engine(engine, dev_priv, id) {
> >                 struct i915_gem_context *ctx;
> > ?
> 
> In series [1] I was trying to fix symmetry in calls to uc_init_hw/fini_hw
> where we are enabling/disabling GuC submission. In particular, patch [2]
> fixes reset path.

Tbh, I first misread that as i915_gem_fini(). Hmm, but we still should
be re-enabling guc prior to submitting our first requests here in
i915_gem_reset() prior to calling i915_gem_init_hw(). We want to start
from the repopulated request lists, so sticking the current init_hw() is
overkill.

I guess I have this sketch in mind:

i915_reset:

	i915_gem_reset_prepare();
	i915_gem_disable_hw(); // placeholder!

	i915_gem_fini_hw(); // revert to default state

	intel_gpu_reset();

	i915_gem_init_hw(); // just the HW setup

	i915_gem_reset();

	i915_gem_enable_hw(); // tell the HW to run
	i915_gem_reset_finish();

That i915_gem_reset() sticks out like a sore thumb. I think that's what
we need to break up and rearrange. Hmm, there's some of that in the RPS
series. Looks like we're too early to try and do this neatly.

> So maybe we should try to merge* both series ?

Hmm, looking over that series doesn't patch 5 hit the GEM_BUG_ON removed
in patch 6? Otherwise, merging the first 5 with Sagar's blessing would
be on the cards as they look sane otherwise. Maybe start with the first
4 then?
-Chris