From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [REPOST] [PATCH] drm/i915/ppgtt: Load address space after mi_set_context Date: Thu, 12 Jun 2014 20:28:57 +0300 Message-ID: <20140612172857.GU27580@intel.com> References: <1402586752-22484-1-git-send-email-benjamin.widawsky@intel.com> <20140612161648.GT27580@intel.com> <20140612165550.GA9509@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 0C51C6E4A9 for ; Thu, 12 Jun 2014 10:29:02 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140612165550.GA9509@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ben Widawsky Cc: Intel GFX , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Thu, Jun 12, 2014 at 09:55:50AM -0700, Ben Widawsky wrote: > On Thu, Jun 12, 2014 at 07:16:48PM +0300, Ville Syrj=E4l=E4 wrote: > > On Thu, Jun 12, 2014 at 08:25:52AM -0700, Ben Widawsky wrote: > > > On GEN8 the PDPs are saved and restored with context, which means we > > > must set them after the context switch has occurred. If we do not do > > > this, we end up saving the new PDPs for the old context. > > > = > > > Example of a problem > > > LRI PDPs 1 > > > MI_SET_CONTEXT bar > > > LRI_PDPs 2 > > > MI_SET_CONTEXT foo // save PDPs 2 to bar's context > > > // load foos PDPs > > > LRI PDPs 1 > > > MI_SET_CONTEXT bar // save PDPs 1 to foo's context > > > = > > > It's all wacky. This should allow full PPGTT on Broadwell to work. > > = > > Hmm. I had this impression too but now I'm not sure. The PDPs are listed > > as being in the execlist context. Do we save/restore that in ring buffer > > mode too? IIRC on ivb/hsw it got skipped. > > = > > And if the PDPs are really part of the context we shouldn't need to > > load them at all I think, except when we skip the restore (unitialized > > or default context). Or am I missing something here? > = > Yes, I believe you are correct. However, I don't see any harm with the > existing solution either, Yeah just a bit wasteful to load twice. Should still work fine. > and I was hesitant to change stuff around, > since at the time I just needed to make it work for the 64b series. But > I think what you're saying is just adding an "&& !to->is_initialized" to > the USES_FULL_PPGTT check, right? I can try that. or '&& (hw_flags & MI_RESTORE_INHIBIT)' to catch both default and !initialized contexts. > = > > = > > And what about ivb/hsw? Doesn't this reordering risk the context restore > > accessing the wrong PPGTT. That's assuming the context restore can > > already chase some pointers. > = > This is not meant for IVB/HSW. Empirically we've found it doesn't > have the same behavior, as you said above. So that's definite an issue > which requires a patch rev. OK > = > > = > > > = > > > Signed-off-by: Ben Widawsky > > > --- > > > drivers/gpu/drm/i915/i915_gem_context.c | 24 ++++++++++++++++++------ > > > 1 file changed, 18 insertions(+), 6 deletions(-) > > > = > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/dr= m/i915/i915_gem_context.c > > > index 3ffe308..b2434e0 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > > @@ -623,13 +623,12 @@ static int do_switch(struct intel_engine_cs *ri= ng, > > > */ > > > from =3D ring->last_context; > > > = > > > - if (USES_FULL_PPGTT(ring->dev)) { > > > - ret =3D ppgtt->switch_mm(ppgtt, ring, false); > > > - if (ret) > > > - goto unpin_out; > > > - } > > > - > > > if (ring !=3D &dev_priv->ring[RCS]) { > > > + if (USES_FULL_PPGTT(ring->dev)) { > > > + ret =3D ppgtt->switch_mm(ppgtt, ring, false); > > > + if (ret) > > > + goto unpin_out; > > > + } > > > if (from) > > > i915_gem_context_unreference(from); > > > goto done; > > > @@ -660,6 +659,19 @@ static int do_switch(struct intel_engine_cs *rin= g, > > > if (ret) > > > goto unpin_out; > > > = > > > + if (USES_FULL_PPGTT(ring->dev)) { > > > + ret =3D ppgtt->switch_mm(ppgtt, ring, false); > > > + /* The hardware context switch is emitted, but we haven't > > > + * actually changed the state - so it's probably safe to bail > > > + * here. Still, let the user know something dangerous has > > > + * happened. > > > + */ > > > + if (ret) { > > > + DRM_ERROR("Failed to change address space on context switch\n"); > > > + goto unpin_out; > > > + } > > > + } > > > + > > > for (i =3D 0; i < MAX_L3_SLICES; i++) { > > > if (!(to->remap_slice & (1< > > continue; > > > -- = > > > 2.0.0 > > > = > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > = > > -- = > > Ville Syrj=E4l=E4 > > Intel OTC > = > -- = > Ben Widawsky, Intel Open Source Technology Center -- = Ville Syrj=E4l=E4 Intel OTC