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

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

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 435 bytes --]

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.

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

[-- Attachment #1.2: drm-i915-disable-psmi-sleep-messages-on-all-rings-3.16.patch --]
[-- Type: text/x-patch, Size: 6140 bytes --]

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)
 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

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

[Luis Henriques]

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)
>