[PATCH] drm/i915: Force HW context restore on resume.

[PATCH] drm/i915: Force HW context restore on resume.

[Rodrigo Vivi]

Using aliasing ppgtt in some cases like playing video the GPU might hang
because HW context was not in a reliable state.
When we resume we switch to default context and when we resume we can
force a restore if default is really there and object is bound.

Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: U. Artie Eoff <ullysses.a.eoff@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 180 +++++++++++++++++---------------
 1 file changed, 94 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index e4c57a3..0a8a07a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -97,6 +97,91 @@
 #define GEN6_CONTEXT_ALIGN (64<<10)
 #define GEN7_CONTEXT_ALIGN 4096
 
+static inline int
+mi_set_context(struct intel_engine_cs *ring,
+	       struct intel_context *new_context,
+	       u32 hw_flags)
+{
+	u32 flags = hw_flags | MI_MM_SPACE_GTT;
+	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
+	 * explicitly, so we rely on the value at ring init, stored in
+	 * itlb_before_ctx_switch.
+	 */
+	if (IS_GEN6(ring->dev)) {
+		ret = ring->flush(ring, I915_GEM_GPU_DOMAINS, 0);
+		if (ret)
+			return ret;
+	}
+
+	/* These flags are for resource streamer on HSW+ */
+	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
+		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
+
+
+	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) {
+		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
+		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);
+	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->legacy_hw_ctx.rcs_state) |
+			flags);
+	/*
+	 * w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
+	 * WaMiSetContext_Hang:snb,ivb,vlv
+	 */
+	intel_ring_emit(ring, MI_NOOP);
+
+	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);
+	}
+
+	intel_ring_advance(ring);
+
+	return ret;
+}
+
 static size_t get_context_alignment(struct drm_device *dev)
 {
 	if (IS_GEN6(dev))
@@ -412,6 +497,7 @@ void i915_gem_context_fini(struct drm_device *dev)
 int i915_gem_context_enable(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *ring;
+	struct intel_context *lctx = dev_priv->ring[RCS].last_context;
 	int ret, i;
 
 	BUG_ON(!dev_priv->ring[RCS].default_context);
@@ -429,12 +515,19 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
 			}
 		}
 
-	} else
+	} else {
 		for_each_ring(ring, dev_priv, i) {
 			ret = i915_switch_context(ring, ring->default_context);
 			if (ret)
 				return ret;
 		}
+		/* Force default HW Context restore on Resume */
+		if (lctx == dev_priv->ring[RCS].default_context &&
+		    i915_gem_obj_ggtt_bound(lctx->legacy_hw_ctx.rcs_state)) {
+			mi_set_context(&dev_priv->ring[RCS], lctx,
+				       MI_FORCE_RESTORE | MI_SAVE_EXT_STATE_EN);
+		}
+	}
 
 	return 0;
 }
@@ -486,91 +579,6 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 	return ctx;
 }
 
-static inline int
-mi_set_context(struct intel_engine_cs *ring,
-	       struct intel_context *new_context,
-	       u32 hw_flags)
-{
-	u32 flags = hw_flags | MI_MM_SPACE_GTT;
-	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
-	 * explicitly, so we rely on the value at ring init, stored in
-	 * itlb_before_ctx_switch.
-	 */
-	if (IS_GEN6(ring->dev)) {
-		ret = ring->flush(ring, I915_GEM_GPU_DOMAINS, 0);
-		if (ret)
-			return ret;
-	}
-
-	/* These flags are for resource streamer on HSW+ */
-	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
-		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
-
-
-	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) {
-		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
-		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);
-	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->legacy_hw_ctx.rcs_state) |
-			flags);
-	/*
-	 * w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
-	 * WaMiSetContext_Hang:snb,ivb,vlv
-	 */
-	intel_ring_emit(ring, MI_NOOP);
-
-	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);
-	}
-
-	intel_ring_advance(ring);
-
-	return ret;
-}
-
 static inline bool should_skip_switch(struct intel_engine_cs *ring,
 				      struct intel_context *from,
 				      struct intel_context *to)
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Force HW context restore on resume.

[Eoff, Ullysses A]

> -----Original Message-----
> From: Vivi, Rodrigo
> Sent: Friday, April 10, 2015 12:50 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Vivi, Rodrigo; Ben Widawsky; Eoff, Ullysses A
> Subject: [PATCH] drm/i915: Force HW context restore on resume.
> 
> Using aliasing ppgtt in some cases like playing video the GPU might hang
> because HW context was not in a reliable state.
> When we resume we switch to default context and when we resume we can
> force a restore if default is really there and object is bound.
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: U. Artie Eoff <ullysses.a.eoff@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Tested-by: U. Artie Eoff <ullysses.a.eoff@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 180 +++++++++++++++++---------------
>  1 file changed, 94 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index e4c57a3..0a8a07a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -97,6 +97,91 @@
>  #define GEN6_CONTEXT_ALIGN (64<<10)
>  #define GEN7_CONTEXT_ALIGN 4096
> 
> +static inline int
> +mi_set_context(struct intel_engine_cs *ring,
> +	       struct intel_context *new_context,
> +	       u32 hw_flags)
> +{
> +	u32 flags = hw_flags | MI_MM_SPACE_GTT;
> +	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
> +	 * explicitly, so we rely on the value at ring init, stored in
> +	 * itlb_before_ctx_switch.
> +	 */
> +	if (IS_GEN6(ring->dev)) {
> +		ret = ring->flush(ring, I915_GEM_GPU_DOMAINS, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* These flags are for resource streamer on HSW+ */
> +	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> +		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> +
> +
> +	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) {
> +		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> +		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);
> +	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->legacy_hw_ctx.rcs_state) |
> +			flags);
> +	/*
> +	 * w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
> +	 * WaMiSetContext_Hang:snb,ivb,vlv
> +	 */
> +	intel_ring_emit(ring, MI_NOOP);
> +
> +	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);
> +	}
> +
> +	intel_ring_advance(ring);
> +
> +	return ret;
> +}
> +
>  static size_t get_context_alignment(struct drm_device *dev)
>  {
>  	if (IS_GEN6(dev))
> @@ -412,6 +497,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>  int i915_gem_context_enable(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *ring;
> +	struct intel_context *lctx = dev_priv->ring[RCS].last_context;
>  	int ret, i;
> 
>  	BUG_ON(!dev_priv->ring[RCS].default_context);
> @@ -429,12 +515,19 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
>  			}
>  		}
> 
> -	} else
> +	} else {
>  		for_each_ring(ring, dev_priv, i) {
>  			ret = i915_switch_context(ring, ring->default_context);
>  			if (ret)
>  				return ret;
>  		}
> +		/* Force default HW Context restore on Resume */
> +		if (lctx == dev_priv->ring[RCS].default_context &&
> +		    i915_gem_obj_ggtt_bound(lctx->legacy_hw_ctx.rcs_state)) {
> +			mi_set_context(&dev_priv->ring[RCS], lctx,
> +				       MI_FORCE_RESTORE | MI_SAVE_EXT_STATE_EN);
> +		}
> +	}
> 
>  	return 0;
>  }
> @@ -486,91 +579,6 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
>  	return ctx;
>  }
> 
> -static inline int
> -mi_set_context(struct intel_engine_cs *ring,
> -	       struct intel_context *new_context,
> -	       u32 hw_flags)
> -{
> -	u32 flags = hw_flags | MI_MM_SPACE_GTT;
> -	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
> -	 * explicitly, so we rely on the value at ring init, stored in
> -	 * itlb_before_ctx_switch.
> -	 */
> -	if (IS_GEN6(ring->dev)) {
> -		ret = ring->flush(ring, I915_GEM_GPU_DOMAINS, 0);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	/* These flags are for resource streamer on HSW+ */
> -	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> -		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> -
> -
> -	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) {
> -		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> -		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);
> -	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->legacy_hw_ctx.rcs_state) |
> -			flags);
> -	/*
> -	 * w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
> -	 * WaMiSetContext_Hang:snb,ivb,vlv
> -	 */
> -	intel_ring_emit(ring, MI_NOOP);
> -
> -	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);
> -	}
> -
> -	intel_ring_advance(ring);
> -
> -	return ret;
> -}
> -
>  static inline bool should_skip_switch(struct intel_engine_cs *ring,
>  				      struct intel_context *from,
>  				      struct intel_context *to)
> --
> 2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Force HW context restore on resume.

[Ben Widawsky]

On Fri, Apr 10, 2015 at 12:50:17PM -0700, Rodrigo Vivi wrote:
> Using aliasing ppgtt in some cases like playing video the GPU might hang
> because HW context was not in a reliable state.
> When we resume we switch to default context and when we resume we can
> force a restore if default is really there and object is bound.
> 

"Empirically, the state of the GPU context on thaw does not match the state at
freeze. As a result, clients which depend on the default context (pre-ppgtt:
ddx, and libva) will resume emitting commands with an unknown state. Generally,
this should not be a problem as the GPU clients are expected to always emit all
state they're depending on. However, it seems that either clients do not do
this, or, the state is so fubar on gen8+ thaw that it doesn't matter. The
solution presented here is to force a context switch to the context that we were
last using at freeze. The force is required because the LRCA might be [*should
be*] be the same across suspend resume."

> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: U. Artie Eoff <ullysses.a.eoff@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Debugged-by?: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 180 +++++++++++++++++---------------
>  1 file changed, 94 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index e4c57a3..0a8a07a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -97,6 +97,91 @@
>  #define GEN6_CONTEXT_ALIGN (64<<10)
>  #define GEN7_CONTEXT_ALIGN 4096
>  
> +static inline int
> +mi_set_context(struct intel_engine_cs *ring,
> +	       struct intel_context *new_context,
> +	       u32 hw_flags)
> +{
> +	u32 flags = hw_flags | MI_MM_SPACE_GTT;
> +	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
> +	 * explicitly, so we rely on the value at ring init, stored in
> +	 * itlb_before_ctx_switch.
> +	 */
> +	if (IS_GEN6(ring->dev)) {
> +		ret = ring->flush(ring, I915_GEM_GPU_DOMAINS, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* These flags are for resource streamer on HSW+ */
> +	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> +		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> +
> +
> +	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) {
> +		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> +		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);
> +	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->legacy_hw_ctx.rcs_state) |
> +			flags);
> +	/*
> +	 * w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
> +	 * WaMiSetContext_Hang:snb,ivb,vlv
> +	 */
> +	intel_ring_emit(ring, MI_NOOP);
> +
> +	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);
> +	}
> +
> +	intel_ring_advance(ring);
> +
> +	return ret;
> +}
> +
>  static size_t get_context_alignment(struct drm_device *dev)
>  {
>  	if (IS_GEN6(dev))
> @@ -412,6 +497,7 @@ void i915_gem_context_fini(struct drm_device *dev)
>  int i915_gem_context_enable(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *ring;
> +	struct intel_context *lctx = dev_priv->ring[RCS].last_context;
>  	int ret, i;
>  
>  	BUG_ON(!dev_priv->ring[RCS].default_context);
> @@ -429,12 +515,19 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
>  			}
>  		}
>  
> -	} else
> +	} else {
>  		for_each_ring(ring, dev_priv, i) {
>  			ret = i915_switch_context(ring, ring->default_context);
>  			if (ret)
>  				return ret;
>  		}
> +		/* Force default HW Context restore on Resume */
> +		if (lctx == dev_priv->ring[RCS].default_context &&
> +		    i915_gem_obj_ggtt_bound(lctx->legacy_hw_ctx.rcs_state)) {
> +			mi_set_context(&dev_priv->ring[RCS], lctx,
> +				       MI_FORCE_RESTORE | MI_SAVE_EXT_STATE_EN);
> +		}
> +	}

I still think you should be handling the case when last is not equal to default
if/when idle fails on suspend. Also, I believe you do not want
MI_SAVE_EXT_STATE_EN. I also believe we shouldn't need this with full ppgtt.

>  
>  	return 0;
>  }
> @@ -486,91 +579,6 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
>  	return ctx;
>  }
>  
> -static inline int
> -mi_set_context(struct intel_engine_cs *ring,
> -	       struct intel_context *new_context,
> -	       u32 hw_flags)
> -{
> -	u32 flags = hw_flags | MI_MM_SPACE_GTT;
> -	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
> -	 * explicitly, so we rely on the value at ring init, stored in
> -	 * itlb_before_ctx_switch.
> -	 */
> -	if (IS_GEN6(ring->dev)) {
> -		ret = ring->flush(ring, I915_GEM_GPU_DOMAINS, 0);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	/* These flags are for resource streamer on HSW+ */
> -	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> -		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> -
> -
> -	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) {
> -		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> -		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);
> -	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->legacy_hw_ctx.rcs_state) |
> -			flags);
> -	/*
> -	 * w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
> -	 * WaMiSetContext_Hang:snb,ivb,vlv
> -	 */
> -	intel_ring_emit(ring, MI_NOOP);
> -
> -	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);
> -	}
> -
> -	intel_ring_advance(ring);
> -
> -	return ret;
> -}
> -
>  static inline bool should_skip_switch(struct intel_engine_cs *ring,
>  				      struct intel_context *from,
>  				      struct intel_context *to)
> -- 
> 2.1.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Force HW context restore on resume.

[Chris Wilson]

On Fri, Apr 10, 2015 at 12:50:17PM -0700, Rodrigo Vivi wrote:
> Using aliasing ppgtt in some cases like playing video the GPU might hang
> because HW context was not in a reliable state.
> When we resume we switch to default context and when we resume we can
> force a restore if default is really there and object is bound.
> 
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: U. Artie Eoff <ullysses.a.eoff@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Bleh, you don't need to overload enable as the context switch should
happen naturally as part of the request initiation. All that you need to
do is mark the context as invalid on suspend. See the patch I sent for
#requests back in September.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Force HW context restore on resume.

[Ben Widawsky]

On Fri, Apr 10, 2015 at 09:37:04PM +0100, Chris Wilson wrote:
> On Fri, Apr 10, 2015 at 12:50:17PM -0700, Rodrigo Vivi wrote:
> > Using aliasing ppgtt in some cases like playing video the GPU might hang
> > because HW context was not in a reliable state.
> > When we resume we switch to default context and when we resume we can
> > force a restore if default is really there and object is bound.
> > 
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Cc: U. Artie Eoff <ullysses.a.eoff@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Bleh, you don't need to overload enable as the context switch should
> happen naturally as part of the request initiation. All that you need to
> do is mark the context as invalid on suspend. See the patch I sent for
> #requests back in September.
> -Chris
> 

That will occur even when using the global default context right out of resume?

> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Force HW context restore on resume.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6181
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -3              276/276              273/276
ILK                 -4              301/301              297/301
SNB                                  316/316              316/316
IVB                 -1              328/328              327/328
BYT                                  285/285              285/285
HSW                                  394/394              394/394
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt@gen3_render_linear_blits      FAIL(4)PASS(4)      FAIL(2)
 PNV  igt@gen3_render_tiledx_blits      FAIL(4)PASS(5)      FAIL(1)PASS(1)
 PNV  igt@gen3_render_tiledy_blits      FAIL(4)PASS(4)      FAIL(1)PASS(1)
*ILK  igt@drv_suspend@fence-restore-tiled2untiled      PASS(2)      NO_RESULT(1)
*ILK  igt@drv_suspend@fence-restore-untiled      PASS(2)      NO_RESULT(1)
*ILK  igt@drv_suspend@forcewake      PASS(2)      NO_RESULT(1)
*ILK  igt@gem_workarounds@suspend-resume      PASS(2)      NO_RESULT(1)
 IVB  igt@gem_pwrite_pread@uncached-copy-performance      DMESG_WARN(1)PASS(3)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:i915_hangcheck_elapsed[i915]]*ERROR*Hangcheck_timer_elapsed...blitter_ring_idle@Hangcheck timer elapsed... blitter ring idle
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx