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 19:16:48 +0300 Message-ID: <20140612161648.GT27580@intel.com> References: <1402586752-22484-1-git-send-email-benjamin.widawsky@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 149C66E939 for ; Thu, 12 Jun 2014 09:17:12 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1402586752-22484-1-git-send-email-benjamin.widawsky@intel.com> 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 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? 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. > = > 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/drm/i9= 15/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 *ring, > */ > 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 *ring, > 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