Re: [PATCH 3.16] drm/i915: Disable PSMI sleep messages on all rings around context switches

[Luis Henriques <luis.henriques@canonical.com>]

On Wed, Dec 30, 2015 at 10:31:17PM +0000, Ben Hutchings wrote:
> This fix from 3.19 (commit 2c550183476d) has so far only been applied
> to 3.18.y, but the bug appears to have been introduced in 3.14 or
> earlier. �There is a positive result of testing on 3.16-ckt here:
> https://bugs.debian.org/777231
> 
> For 3.16-ckt I only made context changes (see attached patch). �I gave
> up trying to backport to 3.14.
> 
> Ben.
>

Thanks Ben, I'll queue for the next 3.16 release.

Cheers,
--
Lu�s


> -- 
> Ben Hutchings
> This sentence contradicts itself - no actually it doesn't.

> From d2317cde33256e6e0f302af8ed8a888e35921255 Mon Sep 17 00:00:00 2001
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Tue, 16 Dec 2014 10:02:27 +0000
> Subject: [PATCH] drm/i915: Disable PSMI sleep messages on all rings around
>  context switches
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> commit 2c550183476dfa25641309ae9a28d30feed14379 upstream.
> 
> There exists a current workaround to prevent a hang on context switch
> should the ring go to sleep in the middle of the restore,
> WaProgramMiArbOnOffAroundMiSetContext (applicable to all gen7+). In
> spite of disabling arbitration (which prevents the ring from powering
> down during the critical section) we were still hitting hangs that had
> the hallmarks of the known erratum. That is we are still seeing hangs
> "on the last instruction in the context restore". By comparing -nightly
> (broken) with requests (working), we were able to deduce that it was the
> semaphore LRI cross-talk that reproduced the original failure. The key
> was that requests implemented deferred semaphore signalling, and
> disabling that, i.e. emitting the semaphore signal to every other ring
> after every batch restored the frequent hang.  Explicitly disabling PSMI
> sleep on the RCS ring was insufficient, all the rings had to be awake to
> prevent the hangs. Fortunately, we can reduce the wakelock to the
> MI_SET_CONTEXT operation itself, and so should be able to limit the extra
> power implications.
> 
> Since the MI_ARB_ON_OFF workaround is listed for all gen7 and above
> products, we should apply this extra hammer for all of the same
> platforms despite so far that we have only been able to reproduce the
> hang on certain ivb and hsw models. The last question is whether we want
> to always use the extra hammer or only when we know semaphores are in
> operation. At the moment, we only use LRI on non-RCS rings for
> semaphores, but that may change in the future with the possibility of
> reintroducing this bug under subtle conditions.
> 
> v2: Make it explicit that the PSMI LRI are an extension to the original
> workaround for the other rings.
> v3: Bikeshedding variable names and whitespacing
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80660
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83677
> Cc: Simon Farnsworth <simon@farnz.org.uk>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: Peter Fr�hberger <fritsch@xbmc.org>
> Reviewed-by: Daniel Vetter <daniel@ffwll.ch>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> [bwh: Backported to 3.16: adjust context]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 48 +++++++++++++++++++++++++++------
>  drivers/gpu/drm/i915/i915_reg.h         |  2 ++
>  2 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a5ddf3b..14f9264 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -545,7 +545,12 @@ mi_set_context(struct intel_engine_cs *ring,
>  	       struct intel_context *new_context,
>  	       u32 hw_flags)
>  {
> -	int ret;
> +	const int num_rings =
> +		/* Use an extended w/a on ivb+ if signalling from other rings */
> +		i915_semaphore_is_enabled(ring->dev) ?
> +		hweight32(INTEL_INFO(ring->dev)->ring_mask) - 1 :
> +		0;
> +	int len, i, ret;
>  
>  	/* w/a: If Flush TLB Invalidation Mode is enabled, driver must do a TLB
>  	 * invalidation prior to MI_SET_CONTEXT. On GEN6 we don't set the value
> @@ -558,15 +563,31 @@ mi_set_context(struct intel_engine_cs *ring,
>  			return ret;
>  	}
>  
> -	ret = intel_ring_begin(ring, 6);
> +
> +	len = 4;
> +	if (INTEL_INFO(ring->dev)->gen >= 7)
> +		len += 2 + (num_rings ? 4*num_rings + 2 : 0);
> +
> +	ret = intel_ring_begin(ring, len);
>  	if (ret)
>  		return ret;
>  
>  	/* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
> -	if (INTEL_INFO(ring->dev)->gen >= 7)
> +	if (INTEL_INFO(ring->dev)->gen >= 7) {
>  		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> -	else
> -		intel_ring_emit(ring, MI_NOOP);
> +		if (num_rings) {
> +			struct intel_engine_cs *signaller;
> +
> +			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
> +			for_each_ring(signaller, to_i915(ring->dev), i) {
> +				if (signaller == ring)
> +					continue;
> +
> +				intel_ring_emit(ring, RING_PSMI_CTL(signaller->mmio_base));
> +				intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
> +			}
> +		}
> +	}
>  
>  	intel_ring_emit(ring, MI_NOOP);
>  	intel_ring_emit(ring, MI_SET_CONTEXT);
> @@ -581,10 +602,21 @@ mi_set_context(struct intel_engine_cs *ring,
>  	 */
>  	intel_ring_emit(ring, MI_NOOP);
>  
> -	if (INTEL_INFO(ring->dev)->gen >= 7)
> +	if (INTEL_INFO(ring->dev)->gen >= 7) {
> +		if (num_rings) {
> +			struct intel_engine_cs *signaller;
> +
> +			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
> +			for_each_ring(signaller, to_i915(ring->dev), i) {
> +				if (signaller == ring)
> +					continue;
> +
> +				intel_ring_emit(ring, RING_PSMI_CTL(signaller->mmio_base));
> +				intel_ring_emit(ring, _MASKED_BIT_DISABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
> +			}
> +		}
>  		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_ENABLE);
> -	else
> -		intel_ring_emit(ring, MI_NOOP);
> +	}
>  
>  	intel_ring_advance(ring);
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index fa0ec5a..8196408 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -978,6 +978,7 @@ enum punit_power_well {
>  #define GEN6_VERSYNC	(RING_SYNC_1(VEBOX_RING_BASE))
>  #define GEN6_VEVSYNC	(RING_SYNC_2(VEBOX_RING_BASE))
>  #define GEN6_NOSYNC 0
> +#define RING_PSMI_CTL(base)	((base)+0x50)
>  #define RING_MAX_IDLE(base)	((base)+0x54)
>  #define RING_HWS_PGA(base)	((base)+0x80)
>  #define RING_HWS_PGA_GEN6(base)	((base)+0x2080)
> @@ -1301,6 +1302,7 @@ enum punit_power_well {
>  #define   GEN6_BLITTER_FBC_NOTIFY			(1<<3)
>  
>  #define GEN6_RC_SLEEP_PSMI_CONTROL	0x2050
> +#define   GEN6_PSMI_SLEEP_MSG_DISABLE	(1 << 0)
>  #define   GEN8_RC_SEMA_IDLE_MSG_DISABLE	(1 << 12)
>  #define   GEN8_FF_DOP_CLOCK_GATE_DISABLE	(1<<10)
>