Re: [PATCH 1/2] drm/i915: Split out !RCS legacy context switching

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 13, 2016 at 03:16:30PM +0100, Chris Wilson wrote:
> Having the !RCS legacy context switch threaded through the RCS switching
> code makes it much harder to follow and understand. In the next patch, I
> want to fix a bug handling the incomplete switch, this is made much
> simpler if we segregate the two paths now.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 74 +++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 453b655e86fc..c027240aacf3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -609,9 +609,9 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>  	return ret;
>  }
>  
> -static inline bool should_skip_switch(struct intel_engine_cs *engine,
> -				      struct intel_context *from,
> -				      struct intel_context *to)
> +static inline bool skip_rcs_switch(struct intel_engine_cs *engine,
> +				   struct intel_context *from,
> +				   struct intel_context *to)
>  {
>  	if (to->remap_slice)
>  		return false;
> @@ -626,15 +626,17 @@ static inline bool should_skip_switch(struct intel_engine_cs *engine,
>  static bool
>  needs_pd_load_pre(struct intel_engine_cs *engine, struct intel_context *to)
>  {
> -	struct drm_i915_private *dev_priv = engine->dev->dev_private;
> -
>  	if (!to->ppgtt)
>  		return false;
>  
> -	if (INTEL_INFO(engine->dev)->gen < 8)
> +	if (engine->last_context == to &&
> +	    !(intel_engine_flag(engine) & to->ppgtt->pd_dirty_rings))
> +		return false;
> +
> +	if (engine->id != RCS)
>  		return true;
>  
> -	if (engine != &dev_priv->engine[RCS])
> +	if (INTEL_INFO(engine->dev)->gen < 8)
>  		return true;
>  
>  	return false;
> @@ -661,32 +663,24 @@ needs_pd_load_post(struct intel_engine_cs *engine, struct intel_context *to,
>  	return false;
>  }
>  
> -static int do_switch(struct drm_i915_gem_request *req)
> +static int do_rcs_switch(struct drm_i915_gem_request *req)
>  {
>  	struct intel_context *to = req->ctx;
>  	struct intel_engine_cs *engine = req->engine;
> -	struct drm_i915_private *dev_priv = req->i915;
>  	struct intel_context *from = engine->last_context;
>  	u32 hw_flags = 0;
>  	bool uninitialized = false;
>  	int ret, i;
>  
> -	if (from != NULL && engine == &dev_priv->engine[RCS]) {
> -		BUG_ON(from->legacy_hw_ctx.rcs_state == NULL);
> -		BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state));
> -	}
> -
> -	if (should_skip_switch(engine, from, to))
> +	if (skip_rcs_switch(engine, from, to))
>  		return 0;
>  
>  	/* Trying to pin first makes error handling easier. */
> -	if (engine == &dev_priv->engine[RCS]) {
> -		ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state,
> -					    get_context_alignment(engine->dev),
> -					    0);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state,
> +				    get_context_alignment(engine->dev),
> +				    0);
> +	if (ret)
> +		return ret;
>  
>  	/*
>  	 * Pin can switch back to the default context if we end up calling into
> @@ -709,12 +703,6 @@ static int do_switch(struct drm_i915_gem_request *req)
>  		to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
>  	}
>  
> -	if (engine != &dev_priv->engine[RCS]) {
> -		if (from)
> -			i915_gem_context_unreference(from);
> -		goto done;
> -	}
> -
>  	/*
>  	 * Clear this page out of any CPU caches for coherent swap-in/out. Note
>  	 * that thanks to write = false in this call and us not setting any gpu
> @@ -802,7 +790,6 @@ static int do_switch(struct drm_i915_gem_request *req)
>  	uninitialized = !to->legacy_hw_ctx.initialized;
>  	to->legacy_hw_ctx.initialized = true;
>  
> -done:
>  	i915_gem_context_reference(to);
>  	engine->last_context = to;
>  
> @@ -817,8 +804,7 @@ done:
>  	return 0;
>  
>  unpin_out:
> -	if (engine->id == RCS)
> -		i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state);
> +	i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state);
>  	return ret;
>  }
>  
> @@ -843,17 +829,33 @@ int i915_switch_context(struct drm_i915_gem_request *req)
>  	WARN_ON(i915.enable_execlists);
>  	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>  
> -	if (req->ctx->legacy_hw_ctx.rcs_state == NULL) { /* We have the fake context */
> -		if (req->ctx != engine->last_context) {
> -			i915_gem_context_reference(req->ctx);
> +	if (engine->id != RCS ||
> +	    req->ctx->legacy_hw_ctx.rcs_state == NULL) {
> +		struct intel_context *to = req->ctx;
> +
> +		if (needs_pd_load_pre(engine, to)) {

Hm, I'd inline this condition now since it's a bit confusing if there's no
POST. Assuming I read code correctly it seems to boil down to to->ppgtt
(which reads a lot cleaner) for !RCS.

Either way (since this is just a bikeshed):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> +			int ret;
> +
> +			trace_switch_mm(engine, to);
> +			ret = to->ppgtt->switch_mm(to->ppgtt, req);
> +			if (ret)
> +				return ret;
> +
> +			/* Doing a PD load always reloads the page dirs */
> +			to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
> +		}
> +
> +		if (to != engine->last_context) {
> +			i915_gem_context_reference(to);
>  			if (engine->last_context)
>  				i915_gem_context_unreference(engine->last_context);
> -			engine->last_context = req->ctx;
> +			engine->last_context = to;
>  		}
> +
>  		return 0;
>  	}
>  
> -	return do_switch(req);
> +	return do_rcs_switch(req);
>  }
>  
>  static bool contexts_enabled(struct drm_device *dev)
> -- 
> 2.8.0.rc3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx