Re: [PATCH 5/9] drm/i915: Rearrange switch_context to load the aliasing ppgtt on first use

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 19/04/16 07:49, Chris Wilson wrote:
> The code to switch_mm() is already handled by i915_switch_context(), the
> only difference required to setup the aliasing ppgtt is that we need to
> emit te switch_mm() on the first context, i.e. when transitioning from
> engine->last_context == NULL.

Explanation feels a bit short for the amount of change.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         |  1 -
>   drivers/gpu/drm/i915/i915_gem.c         | 28 ---------------------
>   drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++------------------------
>   drivers/gpu/drm/i915/i915_gem_gtt.c     | 13 ----------
>   drivers/gpu/drm/i915/i915_gem_gtt.h     |  1 -
>   5 files changed, 12 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4c55f480f60b..77becfd5a09d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3125,7 +3125,6 @@ int __must_check i915_gem_context_init(struct drm_device *dev);
>   void i915_gem_context_fini(struct drm_device *dev);
>   void i915_gem_context_reset(struct drm_device *dev);
>   int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> -int i915_gem_context_enable(struct drm_i915_gem_request *req);
>   void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
>   int i915_switch_context(struct drm_i915_gem_request *req);
>   struct intel_context *
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index fd9a36badb45..4057c0febccd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4245,34 +4245,6 @@ i915_gem_init_hw(struct drm_device *dev)
>   		}
>   	}
>
> -	/* Now it is safe to go back round and do everything else: */
> -	for_each_engine(engine, dev_priv) {
> -		struct drm_i915_gem_request *req;
> -
> -		req = i915_gem_request_alloc(engine, NULL);
> -		if (IS_ERR(req)) {
> -			ret = PTR_ERR(req);
> -			break;
> -		}
> -
> -		ret = i915_ppgtt_init_ring(req);
> -		if (ret)
> -			goto err_request;
> -
> -		ret = i915_gem_context_enable(req);
> -		if (ret)
> -			goto err_request;
> -
> -err_request:
> -		i915_add_request_no_flush(req);
> -		if (ret) {
> -			DRM_ERROR("Failed to enable %s, error=%d\n",
> -				  engine->name, ret);
> -			i915_gem_cleanup_engines(dev);
> -			break;
> -		}
> -	}
> -
>   out:
>   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>   	return ret;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 6870556a180b..cf84138de4ec 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -471,27 +471,6 @@ void i915_gem_context_fini(struct drm_device *dev)
>   	dev_priv->kernel_context = NULL;
>   }
>
> -int i915_gem_context_enable(struct drm_i915_gem_request *req)
> -{
> -	struct intel_engine_cs *engine = req->engine;
> -	int ret;
> -
> -	if (i915.enable_execlists) {
> -		if (engine->init_context == NULL)
> -			return 0;
> -
> -		ret = engine->init_context(req);
> -	} else
> -		ret = i915_switch_context(req);
> -
> -	if (ret) {
> -		DRM_ERROR("ring init context: %d\n", ret);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -

So default context is not switched to at driver load, or before the 
non-default context even, any more? And that is fine?

>   static int context_idr_cleanup(int id, void *p, void *data)
>   {
>   	struct intel_context *ctx = p;
> @@ -661,7 +640,7 @@ static bool
>   needs_pd_load_pre(struct intel_engine_cs *engine, struct intel_context *to)
>   {
>   	if (!to->ppgtt)
> -		return false;
> +		return engine->last_context == NULL;
>
>   	if (engine->last_context == to &&
>   	    !(intel_engine_flag(engine) & to->ppgtt->pd_dirty_rings))
> @@ -724,6 +703,7 @@ 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 i915_hw_ppgtt *ppgtt = to->ppgtt ?: req->i915->mm.aliasing_ppgtt;
>   	struct intel_context *from;
>   	u32 hw_flags;
>   	int ret, i;
> @@ -765,7 +745,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
>   		 * Register Immediate commands in Ring Buffer before submitting
>   		 * a context."*/
>   		trace_switch_mm(engine, to);
> -		ret = to->ppgtt->switch_mm(to->ppgtt, req);
> +		ret = ppgtt->switch_mm(ppgtt, req);
>   		if (ret)
>   			goto unpin_out;
>   	}
> @@ -776,8 +756,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
>   		 * space. This means we must enforce that a page table load
>   		 * occur when this occurs. */
>   		hw_flags = MI_RESTORE_INHIBIT;
> -	else if (to->ppgtt &&
> -		 intel_engine_flag(engine) & to->ppgtt->pd_dirty_rings)
> +	else if (ppgtt && intel_engine_flag(engine) & ppgtt->pd_dirty_rings)
>   		hw_flags = MI_FORCE_RESTORE;
>   	else
>   		hw_flags = 0;
> @@ -822,7 +801,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
>   	 */
>   	if (needs_pd_load_post(to, hw_flags)) {
>   		trace_switch_mm(engine, to);
> -		ret = to->ppgtt->switch_mm(to->ppgtt, req);
> +		ret = ppgtt->switch_mm(to->ppgtt, req);

to->ppgtt should be ppgtt I think.

>   		/* 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
> @@ -832,8 +811,8 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
>   			return ret;
>   	}
>
> -	if (to->ppgtt)
> -		to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
> +	if (ppgtt)
> +		ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
>
>   	for (i = 0; i < MAX_L3_SLICES; i++) {
>   		if (!(to->remap_slice & (1<<i)))
> @@ -887,15 +866,17 @@ int i915_switch_context(struct drm_i915_gem_request *req)
>   		struct intel_context *to = req->ctx;
>
>   		if (needs_pd_load_pre(engine, to)) {
> +			struct i915_hw_ppgtt *ppgtt;
>   			int ret;
>
> +			ppgtt = to->ppgtt ?: to_i915(req)->mm.aliasing_ppgtt;

Wrong base again.

> +
>   			trace_switch_mm(engine, to);
> -			ret = to->ppgtt->switch_mm(to->ppgtt, req);
> +			ret = ppgtt->switch_mm(ppgtt, req);
>   			if (ret)
>   				return ret;
>
> -			/* Doing a PD load always reloads the page dirs */
> -			to->ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
> +			ppgtt->pd_dirty_rings &= ~intel_engine_flag(engine);
>   		}
>
>   		if (to != engine->last_context) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 780e3ad3ca10..50bdba5cb6d2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2180,19 +2180,6 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
>   	return 0;
>   }
>
> -int i915_ppgtt_init_ring(struct drm_i915_gem_request *req)
> -{
> -	struct i915_hw_ppgtt *ppgtt = to_i915(req)->mm.aliasing_ppgtt;
> -
> -	if (i915.enable_execlists)
> -		return 0;
> -
> -	if (!ppgtt)
> -		return 0;
> -
> -	return ppgtt->switch_mm(ppgtt, req);
> -}
> -
>   struct i915_hw_ppgtt *
>   i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv)
>   {
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index d7dd3d8a8758..333a2fc62b43 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -519,7 +519,6 @@ void i915_ggtt_cleanup_hw(struct drm_device *dev);
>
>   int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt);
>   int i915_ppgtt_init_hw(struct drm_device *dev);
> -int i915_ppgtt_init_ring(struct drm_i915_gem_request *req);
>   void i915_ppgtt_release(struct kref *kref);
>   struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_device *dev,
>   					struct drm_i915_file_private *fpriv);
>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx