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

[Ben Widawsky <ben@bwidawsk.net>]

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