| Message ID | 1375988426-4713-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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?
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) |
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(+)