Re: [PATCH 2/2] drm/i915: Prevent loading of uninitialized context garbage

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Aug 08, 2013 at 08:00:26PM +0100, Chris Wilson wrote:
> The extended state bits are stored in the LCA register and affect all
> updates to the LCA register - i.e. the state on the old context is saved
> when SAVE_EX_STATE_EN  is currently set in the old context address before
> the update, and the new context is restored when RESTORE_EX_STATE_EN is
> set in the new context address. This is irrespective of the
> RESTORE_INHIBIT flag in the MI_SET_CONTEXT.
> 
> Hence, upon initial loading the contents of the extended state is read
> from uninitialised data. To workaround this, on first load we do a dummy
> load without the mandatory RESTORE_EX_STATE_EN bit so that the real load
> causes us to initialise the extended state of the context before it is
> then loaded by the LCA update.
> 
> v2: Split out the introduction of the variable length MI_SET_CONTEXT
> command sequence.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=64073
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 8a7b61e..a57d49a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -367,6 +367,8 @@ mi_set_context(struct intel_ring_buffer *ring,
>  	case 5: len += 2;
>  		break;
>  	}
> +	if (!new_context->is_initialized)
> +		len += 2;
>  
>  	ret = intel_ring_begin(ring, len);
>  	if (ret)
> @@ -382,6 +384,22 @@ mi_set_context(struct intel_ring_buffer *ring,
>  		break;
>  	}
>  
> +	if (!new_context->is_initialized) {
> +		/* The GPU tries to restore the extended state irrespective
> +		 * of RestoreInhibit (since it is part of the LCA switch
> +		 * itself rather than the MI_SET_CONTEXT command).
> +		 * Since the initial contents may be garbage we do a dummy
> +		 * load first then set the mandatory flag for any future
> +		 * ring context switches.
> +		 */
> +		intel_ring_emit(ring, MI_SET_CONTEXT);
> +		intel_ring_emit(ring,
> +				i915_gem_obj_ggtt_offset(new_context->obj) |
> +				MI_MM_SPACE_GTT |
> +				MI_SAVE_EXT_STATE_EN |
> +				hw_flags);
> +	}

Hmm. Couldn't we just do this w/ one MI_SET_CONTEXT? Just drop the
MI_RESTORE_EXT_STATE_EN flag if the context is not initialized. The
MI_SAVE_EXT_STATE_EN will be saved in the CCID, so when we switch to
another context the extended state will be saved. And for the next
switch to this context we will set the MI_RESTORE_EXT_STATE_EN bit
in MI_SET_CONTEXT so it should get restored.

But I must admit BSpec is a bit confusing on the topic. It says the
restore bit affects the switch to the context specified in the
logical context address. I take that to mean that the effect of the
restore bit is immediate. But BSpec also says that the bit is stored in
CCID to control the subsequent switch to the same context. So does that
actually mean that 'effective.restore_ext = CCID.restore_ext |
MI_SET_CONTEXT.restore_ext'?

Oh, but BSpec also says that both bits must be set when RS2 power state
is enabled. I think that's the same as RC6, or is it? So I guess the
hardware might consult these bits when entering/leaving RC6. So I suppose
we really need to make sure both bits are always set in case we hit RC6.
So based on that reasoning the patch would seem correct.

I guess I'll give it an r-b regardless :)

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
>  	intel_ring_emit(ring, MI_NOOP);
>  	intel_ring_emit(ring, MI_SET_CONTEXT);
>  	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->obj) |
> -- 
> 1.8.4.rc1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC