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@xxxxxxxxxxxxxxxxxx> > > Cc: Ben Widawsky <ben@xxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
