[Intel-gfx] [PATCH 2/2] drm/i915/g4x: Fix unreliable gpu reset

[Chris Wilson]

On Thu, May 18, 2017 at 03:51:19PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > On Thu, May 18, 2017 at 03:34:22PM +0300, Mika Kuoppala wrote:
> >> ELK seems to very picky about the preconditions to reset.
> >> Evidence on Eaglelake (8086:2e12 (rev 03)) shows that it does
> >> not like if reset occurs when there is active ring.
> >> 
> >> Ville found out that there is workaround with name
> >> 'WaMediaResetMainRingCleanup' which suggests that we need to
> >> cleanup rings before resetting. It is unclear what cleanup
> >> exactly means but evidence shows that stopping the ring
> >> does have an effect on reset reliability.
> >> 
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100998
> >> Suggested-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >> Cc: Tomi Sarvela <tomi.p.sarvela at intel.com>
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_uncore.c | 26 ++++++++++++++++++++++++++
> >>  1 file changed, 26 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> >> index 7eaaf2225e1a..1d473cd1f8a4 100644
> >> --- a/drivers/gpu/drm/i915/intel_uncore.c
> >> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> >> @@ -1427,6 +1427,26 @@ int i915_reg_read_ioctl(struct drm_device *dev,
> >>  	return ret;
> >>  }
> >>  
> >> +static void gen3_stop_rings(struct drm_i915_private *dev_priv)
> >> +{
> >> +	struct intel_engine_cs *engine;
> >> +	enum intel_engine_id id;
> >> +
> >> +	for_each_engine(engine, dev_priv, id) {
> >> +		const i915_reg_t mode_r = RING_MI_MODE(engine->mmio_base);
> >> +
> >> +		I915_WRITE_FW(mode_r, _MASKED_BIT_ENABLE(STOP_RING));
> >
> > This will only stop between instructions on the CS (i.e. within the
> > ring) aiui . If we did hang in a shader, this is not going to achieve
> > very much.
> 
> The subject is too grandiose then. Should I add to the subject 'basic
> hangs'?
> 
> This seems to help get our hang tests through, bringing more coverage.
> I pondered adding ctl, head and tail writes to zero. It would not help
> with shades, but should not hurt either.
> 
> >
> >> +		if (intel_wait_for_register_fw(dev_priv,
> >> +					       mode_r,
> >> +					       MODE_IDLE,
> >> +					       MODE_IDLE,
> >> +					       1000)) {
> >> +			DRM_DEBUG("%s : timed out STOP_RING\n",
> >> +				  engine->name);
> >
> > If we make this a DRM_ERROR I expect it to fire through the hang tests
> > in BAT.
> 
> Why would stopping the ring be a problem with a clean chained batch?
> Well I try to see how it goes.

My understanding is that the STOP_RING only takes affect on the ring
itself, our chained batches stay away from the ring. At least that's the
problem I encountered when trying to use STOP_RING + SYNC_FLUSH for
seqno flushing.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre