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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 21, 2013 at 06:31:07PM +0300, Ville Syrjälä wrote:
> On Wed, Aug 21, 2013 at 04:43:33PM +0300, Ville Syrjälä wrote:
> > 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>
> 
> I just noticed that on HSW these bits control the resource streamer
> state save/restore. The spec says we should always set the RS
> restore bit if we set the RS save bit. So maybe we need some
> !IS_HASWELL checks in there...

Looks like we're lucky since we don't have RS support yet ;-) Can you
please poke Abdiel about this so we make sure to test/check for this?
Cc'ing him.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch