Re: [PATCH] drm/i915: Extract context switch skip and add pd load logic

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Michel Thierry <michel.thierry@intel.com> writes:

> From: Ben Widawsky <benjamin.widawsky@intel.com>
>
> This patch just breaks out the logic of context switch skip.
>
> It also adds pd load pre, and pd load post logic (for GEN8).
>

I dont think this patch just breaks out the logic but it changes
it. And the reasons remains a mystery.

Could you please add justification why the logic changes are
required?

> v2: Use new functions to replace the logic right away (Daniel)
> v3: Add missing pd load logic.
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com> (v2+)
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 48 +++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 70346b0..8474e2c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -569,6 +569,33 @@ mi_set_context(struct intel_engine_cs *ring,
>  	return ret;
>  }
>  
> +static inline bool should_skip_switch(struct intel_engine_cs *ring,
> +				      struct intel_context *from,
> +				      struct intel_context *to)
> +{
> +	if (from == to && !to->remap_slice)
> +		return true;
> +
> +	return false;
> +}
> +
> +static bool
> +needs_pd_load_pre(struct intel_engine_cs *ring, struct intel_context *to)
> +{
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +
> +	return ((INTEL_INFO(ring->dev)->gen < 8) ||
> +			(ring != &dev_priv->ring[RCS])) && to->ppgtt;
> +}
> +
> +static bool
> +needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to)
> +{
> +	return (!to->legacy_hw_ctx.initialized ||
> +			i915_gem_context_is_default(to)) &&
> +			to->ppgtt && IS_GEN8(ring->dev);
> +}
> +
>  static int do_switch(struct intel_engine_cs *ring,
>  		     struct intel_context *to)
>  {
> @@ -584,7 +611,7 @@ static int do_switch(struct intel_engine_cs *ring,
>  		BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state));
>  	}
>  
> -	if (from == to && !to->remap_slice)
> +	if (should_skip_switch(ring, from, to))
>  		return 0;
>  
>  	/* Trying to pin first makes error handling easier. */
> @@ -602,7 +629,11 @@ static int do_switch(struct intel_engine_cs *ring,
>  	 */
>  	from = ring->last_context;
>  
> -	if (to->ppgtt) {
> +	if (needs_pd_load_pre(ring, to)) {
> +		/* 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."*/
>  		trace_switch_mm(ring, to);
>  		ret = to->ppgtt->switch_mm(to->ppgtt, ring);
>  		if (ret)
> @@ -644,6 +675,19 @@ static int do_switch(struct intel_engine_cs *ring,
>  	if (ret)
>  		goto unpin_out;
>  
> +	if (needs_pd_load_post(ring, to)) {
> +		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;
> +		}
> +	}
> +

Can there be a context where both pd load pre and pd load post is
needed? If not, please consider adding WARN_ON which trigger is
when we emit switch_mm twice due to both being true.

Thanks,
-Mika

>  	for (i = 0; i < MAX_L3_SLICES; i++) {
>  		if (!(to->remap_slice & (1<<i)))
>  			continue;
> -- 
> 2.1.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx