[Intel-gfx] [PATCH v2] drm/i915: Re-enable GTT following a device reset

[Chris Wilson]

Quoting Ville Syrjälä (2017-09-06 13:13:05)
> On Wed, Sep 06, 2017 at 12:14:05PM +0100, Chris Wilson wrote:
> > Ville Syrjälä spotted that PGETBL_CTL was losing its enable bit upon a
> > reset. That was causing the display to show garbage on his 945gm. On my
> > i915gm the effect was far more severe; re-enabling the display following
> > the reset without PGETBL_CTL being enabled lead to an immediate hard
> > hang.
> > 
> > We do have a routine to re-enable PGETBL_CTL which is applicable to
> > gen2-4, although on gen4 it is documented that a graphics reset doesn't
> > alter the register (no such wording is given for gen3) and should be safe
> > to call to punch back in the enable bit. However, that leaves the question
> > of whether we need to completely re-initialise the register and the
> > rest of the GSM. For g33/pnv/gen4+, where we do have a configurable
> > page table, its contents do seem to be kept, and so we should be able to
> > recover without having to reinitialise the GTT from scratch (as prior to
> > g33, that register is configured by the BIOS and we leave alone except
> > for the enable bit).
> > 
> > This appears to have been broken by commit 5fbd0418eef2 ("drm/i915:
> > Re-enable GGTT earlier during resume on pre-gen6 platforms"), which
> > moved the intel_enable_gtt() from i915_gem_init_hw() (also used by
> > reset) to add it earlier during hw init and resume, missing the reset
> > path.
> > 
> > v2: Find the culprit, rearrange ggtt_enable to be before gem_init_hw to
> > match init/resume
> > 
> > Reported-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Fixes: 5fbd0418eef2 ("drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms")
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101852
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Daniel Vetter <daniel at ffwll.ch>
> > Reviewed-by: Daniel Vetter <daniel at ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index f10a078e3a55..ff70fc45ba7c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1892,9 +1892,15 @@ void i915_reset(struct drm_i915_private *i915, unsigned int flags)
> >  
> >       /*
> >        * Everything depends on having the GTT running, so we need to start
> > -      * there.  Fortunately we don't need to do this unless we reset the
> > -      * chip at a PCI level.
> > -      *
> > +      * there.
> > +      */
> > +     ret = i915_ggtt_enable_hw(i915);
> > +     if (ret) {
> > +             DRM_ERROR("Failed to re-enable GGTT following reset %d\n", ret);
> > +             goto error;
> > +     }
> 
> I do wonder a bit whether the hardware might object to the fact that
> we restore fences before the GGTT gets re-enabled. But I suppose it's
> possible there's no linkage between the two until someone actually
> accesses the GGTT...

That's a reasonable suggestion. The real challenge is finding a name for
each step ;)
-Chris