From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915/ppgtt: Load address space after mi_set_context Date: Fri, 12 Sep 2014 18:35:10 +0300 Message-ID: <20140912153510.GC12416@intel.com> References: <1410534601-11523-1-git-send-email-michel.thierry@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 7056A6E740 for ; Fri, 12 Sep 2014 08:35:57 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1410534601-11523-1-git-send-email-michel.thierry@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Michel Thierry Cc: intel-gfx@lists.freedesktop.org, Ben Widawsky , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Fri, Sep 12, 2014 at 04:10:01PM +0100, Michel Thierry wrote: > From: Ben Widawsky > = > The simple explanation is, the docs say to do this for GEN8. Perhaps we > want to do this for GEN7 too, I am not certain. > = > PDPs are saved and restored with context. Contexts (without execlists) > only exist on the render ring. The docs say that PDPs are not power > context save/restored. I've learned that this actually means something > which SW doesn't care about. So pretend the statement doesn't exist. > For non RCS, nothing changes. > = > All this patch now does is change the ordering of LRI vs MI_SET_CONTEXT > for the initialization of the context. I do this because the docs say to > do it, and frankly, I cannot reason why it is necessary. I've thought > about it a lot, and tried, without success, to get a reason from design. > The answer I got more or less says, "gen7 is different than gen8." I've > given up, and am adding this little bit of code to make it in sync with > the docs. > = > v2: Completely rewritten commit message that addresses the requests > Ville made for v1 > Only load PDPs for initial context load (Ville) > = > v3: Rebased after ppgtt clean-up rules, and apply only for render > ring. This is needed to boot to desktop with full ppgtt in legacy mode > (without execlists). > = > Signed-off-by: Ben Widawsky > Signed-off-by: Michel Thierry (v3) > --- > drivers/gpu/drm/i915/i915_gem_context.c | 31 +++++++++++++++++++++++++++= ++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i9= 15/i915_gem_context.c > index a5221d8..faebbf3 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -522,6 +522,8 @@ static int do_switch(struct intel_engine_cs *ring, > struct intel_context *from =3D ring->last_context; > u32 hw_flags =3D 0; > bool uninitialized =3D false; > + bool needs_pd_load_rcs =3D (INTEL_INFO(ring->dev)->gen < 8) && to->ppgt= t; > + bool needs_pd_load_xcs =3D (ring !=3D &dev_priv->ring[RCS]) && to->ppgt= t; > int ret, i; > = > if (from !=3D NULL && ring =3D=3D &dev_priv->ring[RCS]) { > @@ -547,7 +549,11 @@ static int do_switch(struct intel_engine_cs *ring, > */ > from =3D ring->last_context; > = > - if (to->ppgtt) { > + if (needs_pd_load_rcs || needs_pd_load_xcs) { > + /* Older GENs and non render rings still want the load first, > + * "PP_DCLV followed by PP_DIR_BASE register through Load > + * Register Immediate commands in Ring Buffer before submitting > + * a context."*/ > ret =3D to->ppgtt->switch_mm(to->ppgtt, ring); > if (ret) > goto unpin_out; > @@ -577,13 +583,34 @@ static int do_switch(struct intel_engine_cs *ring, > vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, GLOBAL_BI= ND); > } > = > - if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) > + if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) { > hw_flags |=3D MI_RESTORE_INHIBIT; > + needs_pd_load_rcs =3D to->ppgtt && IS_GEN8(ring->dev); > + } > = > ret =3D mi_set_context(ring, to, hw_flags); > if (ret) > goto unpin_out; > = > + /* GEN8 does *not* require an explicit reload if the PDPs have been > + * setup, and we do not wish to move them. > + * > + * XXX: If we implemented page directory eviction code, this > + * optimization needs to be removed. > + */ The comment seems a bit misplaced. Would seem more appropriate around where we derive needs_pd_load_rcs. > + if (needs_pd_load_rcs) { > + ret =3D to->ppgtt->switch_mm(to->ppgtt, ring); Aren't we now loading both before _and_ after on <=3Dgen7 (except when uninitialized or default ctx is used)? Maybe the variables should be called needs_pd_load_pre and needs_pd_load_post or something? The current approach just seems somehow confusing to me. > + /* 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.3 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC