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

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

[Chris Wilson]

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.

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 | 35 +++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 70e9e40..cf35f01 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -351,7 +351,7 @@ mi_set_context(struct intel_ring_buffer *ring,
 	       struct i915_hw_context *new_context,
 	       u32 hw_flags)
 {
-	int ret;
+	int ret, len;
 
 	/* 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
@@ -364,7 +364,16 @@ mi_set_context(struct intel_ring_buffer *ring,
 			return ret;
 	}
 
-	ret = intel_ring_begin(ring, 6);
+	len = 4;
+	switch (INTEL_INFO(ring->dev)->gen) {
+	case 7:
+	case 5: len += 2;
+		break;
+	}
+	if (!new_context->is_initialized)
+		len += 2;
+
+	ret = intel_ring_begin(ring, len);
 	if (ret)
 		return ret;
 
@@ -376,9 +385,22 @@ mi_set_context(struct intel_ring_buffer *ring,
 	case 5:
 		intel_ring_emit(ring, MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
 		break;
-	default:
-		intel_ring_emit(ring, MI_NOOP);
-		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);
 	}
 
 	intel_ring_emit(ring, MI_NOOP);
@@ -398,9 +420,6 @@ mi_set_context(struct intel_ring_buffer *ring,
 	case 5:
 		intel_ring_emit(ring, MI_SUSPEND_FLUSH);
 		break;
-	default:
-		intel_ring_emit(ring, MI_NOOP);
-		break;
 	}
 
 	intel_ring_advance(ring);
-- 
1.8.4.rc1

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

[Ben Widawsky]

On Thu, Aug 08, 2013 at 02:37:35PM +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.
>
>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>

If you split this up in 2, the variable length for mi_set_context, and
the workaround - the variable length thing is:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

I'd like to dig a bit more at the workaround portion.

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 1/2] drm/i915: Eliminate unrequired dwords for MI_SET_CONTEXT

[Chris Wilson]

A later patch adds yet another workaround for MI_SET_CONTEXT, at which
point we start to end up with more NOOPs than actual command dwords
along the non-workaround paths. It is time that we made the MI_SET_CONTEXT
a variable length block and only emit the dwords we truly need.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 879bfa2..8a7b61e 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -348,7 +348,7 @@ mi_set_context(struct intel_ring_buffer *ring,
 	       struct i915_hw_context *new_context,
 	       u32 hw_flags)
 {
-	int ret;
+	int ret, len;
 
 	/* 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
@@ -361,7 +361,14 @@ mi_set_context(struct intel_ring_buffer *ring,
 			return ret;
 	}
 
-	ret = intel_ring_begin(ring, 6);
+	len = 4;
+	switch (INTEL_INFO(ring->dev)->gen) {
+	case 7:
+	case 5: len += 2;
+		break;
+	}
+
+	ret = intel_ring_begin(ring, len);
 	if (ret)
 		return ret;
 
@@ -373,9 +380,6 @@ mi_set_context(struct intel_ring_buffer *ring,
 	case 5:
 		intel_ring_emit(ring, MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
 		break;
-	default:
-		intel_ring_emit(ring, MI_NOOP);
-		break;
 	}
 
 	intel_ring_emit(ring, MI_NOOP);
@@ -395,9 +399,6 @@ mi_set_context(struct intel_ring_buffer *ring,
 	case 5:
 		intel_ring_emit(ring, MI_SUSPEND_FLUSH);
 		break;
-	default:
-		intel_ring_emit(ring, MI_NOOP);
-		break;
 	}
 
 	intel_ring_advance(ring);
-- 
1.8.4.rc1

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

[Chris Wilson]

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);
+	}
+
 	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

Re: [PATCH 1/2] drm/i915: Eliminate unrequired dwords for MI_SET_CONTEXT

[Daniel Vetter]

On Thu, Aug 08, 2013 at 08:00:25PM +0100, Chris Wilson wrote:
> A later patch adds yet another workaround for MI_SET_CONTEXT, at which
> point we start to end up with more NOOPs than actual command dwords
> along the non-workaround paths. It is time that we made the MI_SET_CONTEXT
> a variable length block and only emit the dwords we truly need.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Since this seems to be based on top of gen5 context support: Care to slap
an r-b and tested-by onto it so that I can merge the entire shebang?

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 879bfa2..8a7b61e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -348,7 +348,7 @@ mi_set_context(struct intel_ring_buffer *ring,
>  	       struct i915_hw_context *new_context,
>  	       u32 hw_flags)
>  {
> -	int ret;
> +	int ret, len;
>  
>  	/* 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
> @@ -361,7 +361,14 @@ mi_set_context(struct intel_ring_buffer *ring,
>  			return ret;
>  	}
>  
> -	ret = intel_ring_begin(ring, 6);
> +	len = 4;
> +	switch (INTEL_INFO(ring->dev)->gen) {
> +	case 7:
> +	case 5: len += 2;
> +		break;
> +	}
> +
> +	ret = intel_ring_begin(ring, len);
>  	if (ret)
>  		return ret;
>  
> @@ -373,9 +380,6 @@ mi_set_context(struct intel_ring_buffer *ring,
>  	case 5:
>  		intel_ring_emit(ring, MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
>  		break;
> -	default:
> -		intel_ring_emit(ring, MI_NOOP);
> -		break;
>  	}
>  
>  	intel_ring_emit(ring, MI_NOOP);
> @@ -395,9 +399,6 @@ mi_set_context(struct intel_ring_buffer *ring,
>  	case 5:
>  		intel_ring_emit(ring, MI_SUSPEND_FLUSH);
>  		break;
> -	default:
> -		intel_ring_emit(ring, MI_NOOP);
> -		break;
>  	}
>  
>  	intel_ring_advance(ring);
> -- 
> 1.8.4.rc1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/2] drm/i915: Eliminate unrequired dwords for MI_SET_CONTEXT

[Chris Wilson]

On Fri, Aug 09, 2013 at 11:41:26AM +0200, Daniel Vetter wrote:
> On Thu, Aug 08, 2013 at 08:00:25PM +0100, Chris Wilson wrote:
> > A later patch adds yet another workaround for MI_SET_CONTEXT, at which
> > point we start to end up with more NOOPs than actual command dwords
> > along the non-workaround paths. It is time that we made the MI_SET_CONTEXT
> > a variable length block and only emit the dwords we truly need.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Since this seems to be based on top of gen5 context support: Care to slap
> an r-b and tested-by onto it so that I can merge the entire shebang?

Sure, there was nothing outrageous in the enabling patches, and I can
confirm that in the end none of the symptoms I had were related to
context enabling (just the machine dying as it turns out).

So for the ilk ctx patches:

The i915_gem_obj_ggtt_pin() changes are ultimately bogus, that is
  drm/i915: Make ILK context objects more like the others
and
  drm/i915: Restore ILK powerctx pin attributes
cancel each other out and can quite happily die. What would be nice
would be to use
  i915_gem_object_ggtt_pin(ctx, 0, false, false);
instead.

All the other patches look good, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

And a Tested-by: Chris Wilson <chris@chris-wilson.co.uk> # X still works!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Ville Syrjälä]

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

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

[Ville Syrjälä]

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

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

-- 
Ville Syrjälä
Intel OTC

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

[Daniel Vetter]

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

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

[Abdiel Janulgue]

On Wednesday, August 21, 2013 11:12:36 PM Daniel Vetter wrote:
> 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.

The RS State Save/Restore bits on MI_SET_CONTEXT should probably be switched 
on only when we have the RS enabled. Basically it saves RS state (hw-bt 
images, gather image) and would not make sense to enable this bit without RS 
filling this data. I am not sure of the behaviour though when this bit is set 
and the RS switched off.

-Abdiel

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

[Ville Syrjälä]

On Thu, Aug 22, 2013 at 12:18:10PM +0300, Abdiel Janulgue wrote:
> On Wednesday, August 21, 2013 11:12:36 PM Daniel Vetter wrote:
> > 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.
> 
> The RS State Save/Restore bits on MI_SET_CONTEXT should probably be switched 
> on only when we have the RS enabled. Basically it saves RS state (hw-bt 
> images, gather image) and would not make sense to enable this bit without RS 
> filling this data. I am not sure of the behaviour though when this bit is set 
> and the RS switched off.

I guess we might want to defer it until the first RS batch is executed.
After that point we'd have to keep it enabled I think. Although I'm not
sure if earlier batches can already set up some RS state w/o actually 
turning RS on?

-- 
Ville Syrjälä
Intel OTC