From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zhi Wang Subject: Re: [PATCH v4] drm/i915/ppgtt: Load address space after mi_set_context Date: Tue, 23 Sep 2014 14:58:02 +0800 Message-ID: <542119FA.8090005@intel.com> References: <1410773387-24682-1-git-send-email-michel.thierry@intel.com> <54211877.5070401@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 259EA89104 for ; Tue, 23 Sep 2014 00:13:49 -0700 (PDT) In-Reply-To: <54211877.5070401@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@intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org Hi Michel: I'm doing a task which require LRIs to load root pointer around RCS context switch.My BDW CPU is C stepping. And what I found is MI_SET_CONTEXT instruction will save/restore PDP0/1/2/3. It's weired that as part of EXECLIST context, the "EXECLIST PPGTT base" context also appear in the dump generated by MI_SET_CONTEXT. And I found LRI after MI_SET_CONTEXT will make ring not idle: 0x8000[0] != 1. By the way, if MI_SET_CONTEXT is issued very frequently. I also found that will make ring not idle: INSTDONE register shows VFE and TSG not done. Do you met these issues? Thanks. Thanks, Zhi. > -------- Original Message -------- > Subject: [Intel-gfx] [PATCH v4] drm/i915/ppgtt: Load address space after > mi_set_context > Date: Mon, 15 Sep 2014 10:29:47 +0100 > From: Michel Thierry > To: > > 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). > > v4: Clean up the pre/post load logic (Ville). > > Signed-off-by: Ben Widawsky > Signed-off-by: Michel Thierry (v3-4) > --- > drivers/gpu/drm/i915/i915_gem_context.c | 32 > ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > index a5221d8..7b73b36 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -522,6 +522,9 @@ static int do_switch(struct intel_engine_cs *ring, > struct intel_context *from = ring->last_context; > u32 hw_flags = 0; > bool uninitialized = false; > + bool needs_pd_load_pre = ((INTEL_INFO(ring->dev)->gen < 8) || > + (ring != &dev_priv->ring[RCS])) && to->ppgtt; > + bool needs_pd_load_post = false; > int ret, i; > > if (from != NULL && ring == &dev_priv->ring[RCS]) { > @@ -547,7 +550,11 @@ static int do_switch(struct intel_engine_cs *ring, > */ > from = ring->last_context; > > - if (to->ppgtt) { > + if (needs_pd_load_pre) { > + /* 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 = to->ppgtt->switch_mm(to->ppgtt, ring); > if (ret) > goto unpin_out; > @@ -577,13 +584,34 @@ static int do_switch(struct intel_engine_cs *ring, > vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, > GLOBAL_BIND); > } > > - if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) > + /* 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. > + */ > + if (!to->legacy_hw_ctx.initialized || > i915_gem_context_is_default(to)) { > hw_flags |= MI_RESTORE_INHIBIT; > + needs_pd_load_post = to->ppgtt && IS_GEN8(ring->dev); > + } > > ret = mi_set_context(ring, to, hw_flags); > if (ret) > goto unpin_out; > > + if (needs_pd_load_post) { > + ret = to->ppgtt->switch_mm(to->ppgtt, ring); > + /* 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 = 0; i < MAX_L3_SLICES; i++) { > if (!(to->remap_slice & (1< continue;