[Intel-gfx] [PATCH v2 2/2] drm/i915/g4x: Improve gpu reset reliability

[Chris Wilson]

On Thu, May 18, 2017 at 05:28:41PM +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. This patch makes
> reset succesful on hangs induced by chained batches (the igt ones).
> Note that if the hang is inside a shader, it is possible
> that our attempts to stop the ring achieves anything.
> 
> v2: zero ctl,head,tail also. bug ref. use driver debugs (Chris)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100942
> Testcase: igt/gem_busy/*-hang
> Testcase: igt/gem_ringfill/hang-*

Maybe add # elk to these to indicate the problem isn't quite that
widespread!

> 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 | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 7eaaf2225e1a..43da84be0321 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1427,6 +1427,35 @@ 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 u32 base = engine->mmio_base;
> +		const i915_reg_t mode = RING_MI_MODE(base);
> +
> +		I915_WRITE_FW(mode, _MASKED_BIT_ENABLE(STOP_RING));
> +		if (intel_wait_for_register_fw(dev_priv,
> +					       mode,
> +					       MODE_IDLE,
> +					       MODE_IDLE,
> +					       500))
> +			DRM_DEBUG_DRIVER("%s: timed out on STOP_RING\n",
> +					 engine->name);
> +
> +		I915_WRITE_FW(RING_CTL(base), 0);
> +		I915_WRITE_FW(RING_HEAD(base), 0);
> +		I915_WRITE_FW(RING_TAIL(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);
> +	}
> +}
> +
>  static bool i915_reset_complete(struct pci_dev *pdev)
>  {
>  	u8 gdrst;
> @@ -1472,6 +1501,12 @@ static int g4x_do_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
>  	I915_WRITE(VDECCLK_GATE_D, I915_READ(VDECCLK_GATE_D) | VCP_UNIT_CLOCK_GATE_DISABLE);
>  	POSTING_READ(VDECCLK_GATE_D);
>  
> +	/* We stop engines, otherwise we might get failed reset and a
> +	 * dead gpu (on elk).
> +	 */
> +	/* WaMediaResetMainRingCleanup:ctg,elk (supposedly) */

Join this into a single comment block, s/supposedly/presumably/

Just a small concern we have some duplication of stop_ring() here, but I
don't have a better suggestion (along the lines of export intel_stop_ring,
gen3_engine_stop_ring, so far looks more confusing than helpful). As you
have tested with DRM_ERROR to be sure that fear about this simply
timing out for our spinning batches, it looks good to me.

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

-- 
Chris Wilson, Intel Open Source Technology Centre