From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 2/2] drm/i915: Prevent loading of uninitialized context garbage Date: Thu, 22 Aug 2013 12:32:32 +0300 Message-ID: <20130822093232.GB2954@intel.com> References: <20130808171205.GB12344@bwidawsk.net> <20130821153107.GG7159@intel.com> <20130821211236.GF26909@phenom.ffwll.local> <2025042.9isxVR1UVE@abj-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 1D7D8E6562 for ; Thu, 22 Aug 2013 02:32:40 -0700 (PDT) Content-Disposition: inline In-Reply-To: <2025042.9isxVR1UVE@abj-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Abdiel Janulgue Cc: intel-gfx@lists.freedesktop.org, Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org 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=E4l=E4 wrote: > > > On Wed, Aug 21, 2013 at 04:43:33PM +0300, Ville Syrj=E4l=E4 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 re= al > > > > > 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_CONT= EXT > > > > > command sequence. > > > > > = > > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=3D64073 > > > > > Signed-off-by: Chris Wilson > > > > > Cc: Ben Widawsky > > > > > --- > > > > > = > > > > > 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 +=3D 2; > > > > > = > > > > > break; > > > > > = > > > > > } > > > > > = > > > > > + if (!new_context->is_initialized) > > > > > + len +=3D 2; > > > > > = > > > > > ret =3D intel_ring_begin(ring, len); > > > > > if (ret) > > > > > = > > > > > @@ -382,6 +384,22 @@ mi_set_context(struct intel_ring_buffer *rin= g, > > > > > = > > > > > 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 store= d in > > > > CCID to control the subsequent switch to the same context. So does = that > > > > actually mean that 'effective.restore_ext =3D CCID.restore_ext | > > > > MI_SET_CONTEXT.restore_ext'? > > > > = > > > > Oh, but BSpec also says that both bits must be set when RS2 power s= tate > > > > 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=E4l=E4 > > > = > > > 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 switc= hed = > 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=E4l=E4 Intel OTC