[Intel-gfx] [PATCH] drm/i915: Only try to stop engines after a failed reset

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> Currently we try to stop the engine by programming the ring registers to
> be disabled before we perform the reset. Sometimes, we see the context
> image also have invalid ring registers, which one presumes may be
> actually caused by us doing so. Lets risk not doing programming the
> ring to zero on the first attempt to avoid preserving that corruption
> into the context image, leaving the w/a in place for subsequent
> reset attempts.

Might lead to some failed attempts but the
sentiment that use finesse instead of biggest hammer
in arsenal is sane.

It makes me also ponder if we can fight against this
in other side of the fence. Doing a precursory check,
for debug builds, on (first) submit that the lrc is sane?
Using the lrc_regs_ok()?

Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>

-Mika

>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reset.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index 12e74decd7a2..dfced5be1042 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -119,8 +119,10 @@ static void gen3_stop_engine(struct intel_engine_cs *engine)
>  	struct drm_i915_private *dev_priv = engine->i915;
>  	const u32 base = engine->mmio_base;
>  
> +	GEM_TRACE("%s\n", engine->name);
> +
>  	if (intel_engine_stop_cs(engine))
> -		DRM_DEBUG_DRIVER("%s: timed out on STOP_RING\n", engine->name);
> +		GEM_TRACE("%s: timed out on STOP_RING\n", engine->name);
>  
>  	I915_WRITE_FW(RING_HEAD(base), I915_READ_FW(RING_TAIL(base)));
>  	POSTING_READ_FW(RING_HEAD(base)); /* paranoia */
> @@ -133,9 +135,9 @@ static void gen3_stop_engine(struct intel_engine_cs *engine)
>  	I915_WRITE_FW(RING_CTL(base), 0);
>  
>  	/* Check acts as a post */
> -	if (I915_READ_FW(RING_HEAD(base)) != 0)
> -		DRM_DEBUG_DRIVER("%s: ring head not parked\n",
> -				 engine->name);
> +	if (I915_READ_FW(RING_HEAD(base)))
> +		GEM_TRACE("%s: ring head [%x] not parked\n",
> +			  engine->name, I915_READ_FW(RING_HEAD(base)));
>  }
>  
>  static void i915_stop_engines(struct drm_i915_private *i915,
> @@ -579,7 +581,8 @@ int intel_gpu_reset(struct drm_i915_private *i915, unsigned int engine_mask)
>  		 *
>  		 * FIXME: Wa for more modern gens needs to be validated
>  		 */
> -		i915_stop_engines(i915, engine_mask);
> +		if (retry)
> +			i915_stop_engines(i915, engine_mask);
>  
>  		GEM_TRACE("engine_mask=%x\n", engine_mask);
>  		preempt_disable();
> -- 
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx