Re: [PATCH] drm/i915/ppgtt: Load address space after mi_set_context

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Sep 12, 2014 at 04:10:01PM +0100, Michel Thierry wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> 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 <ben@bwidawsk.net>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com> (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/i915/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 = ring->last_context;
>  	u32 hw_flags = 0;
>  	bool uninitialized = false;
> +	bool needs_pd_load_rcs = (INTEL_INFO(ring->dev)->gen < 8) && to->ppgtt;
> +	bool needs_pd_load_xcs = (ring != &dev_priv->ring[RCS]) && to->ppgtt;
>  	int ret, i;
>  
>  	if (from != NULL && ring == &dev_priv->ring[RCS]) {
> @@ -547,7 +549,11 @@ static int do_switch(struct intel_engine_cs *ring,
>  	 */
>  	from = 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 = 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_BIND);
>  	}
>  
> -	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 |= MI_RESTORE_INHIBIT;
> +		needs_pd_load_rcs = to->ppgtt && IS_GEN8(ring->dev);
> +	}
>  
>  	ret = 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 = to->ppgtt->switch_mm(to->ppgtt, ring);

Aren't we now loading both before _and_ after on <=gen7 (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 = 0; i < MAX_L3_SLICES; i++) {
>  		if (!(to->remap_slice & (1<<i)))
>  			continue;
> -- 
> 2.0.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC