Re: [PATCH 7/9] drm/i915: Move the magical deferred context allocation into the request

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 19/04/16 07:49, Chris Wilson wrote:
> We can hide more details of execlists from higher level code by removing
> the explicit call to create an execlist context into its first use.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |  8 --------
>   drivers/gpu/drm/i915/intel_lrc.c           | 21 ++++++++++++++-------
>   drivers/gpu/drm/i915/intel_lrc.h           |  2 --
>   3 files changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 0d210c383487..3da5978ac0f7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1080,14 +1080,6 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
>   		return ERR_PTR(-EIO);
>   	}
>
> -	if (i915.enable_execlists && !ctx->engine[engine->id].state) {
> -		int ret = intel_lr_context_deferred_alloc(ctx, engine);
> -		if (ret) {
> -			DRM_DEBUG("Could not create LRC %u: %d\n", ctx_id, ret);
> -			return ERR_PTR(ret);
> -		}
> -	}
> -
>   	return ctx;
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index c104015813d3..b0d20af38574 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -227,6 +227,8 @@ enum {
>   #define GEN8_CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT	0x17
>   #define GEN9_CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT	0x26
>
> +static int execlists_context_deferred_alloc(struct intel_context *ctx,
> +					    struct intel_engine_cs *engine);
>   static int intel_lr_context_pin(struct intel_context *ctx,
>   				struct intel_engine_cs *engine);
>
> @@ -672,8 +674,6 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>   	struct intel_engine_cs *engine = request->engine;
>   	int ret;
>
> -	request->ringbuf = request->ctx->engine[engine->id].ringbuf;
> -
>   	if (i915.enable_guc_submission) {
>   		/*
>   		 * Check that the GuC has space for the request before
> @@ -687,6 +687,14 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>   			return ret;
>   	}
>
> +	if (request->ctx->engine[engine->id].state == NULL) {
> +		ret = execlists_context_deferred_alloc(request->ctx, engine);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	request->ringbuf = request->ctx->engine[engine->id].ringbuf;
> +
>   	ret = intel_lr_context_pin(request->ctx, engine);
>   	if (ret)
>   		return ret;
> @@ -2091,7 +2099,7 @@ logical_ring_init(struct intel_engine_cs *engine)
>   	if (ret)
>   		goto error;
>
> -	ret = intel_lr_context_deferred_alloc(dctx, engine);
> +	ret = execlists_context_deferred_alloc(dctx, engine);
>   	if (ret)
>   		goto error;
>
> @@ -2541,7 +2549,7 @@ uint32_t intel_lr_context_size(struct intel_engine_cs *engine)
>   }
>
>   /**
> - * intel_lr_context_deferred_alloc() - create the LRC specific bits of a context
> + * execlists_context_deferred_alloc() - create the LRC specific bits of a context
>    * @ctx: LR context to create.
>    * @ring: engine to be used with the context.
>    *
> @@ -2553,9 +2561,8 @@ uint32_t intel_lr_context_size(struct intel_engine_cs *engine)
>    *
>    * Return: non-zero on error.
>    */
> -
> -int intel_lr_context_deferred_alloc(struct intel_context *ctx,
> -				    struct intel_engine_cs *engine)
> +static int execlists_context_deferred_alloc(struct intel_context *ctx,
> +					    struct intel_engine_cs *engine)
>   {
>   	struct drm_i915_gem_object *ctx_obj;
>   	uint32_t context_size;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index b17ab79333aa..8bea937973f6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -102,8 +102,6 @@ static inline void intel_logical_ring_emit_reg(struct intel_ringbuffer *ringbuf,
>
>   void intel_lr_context_free(struct intel_context *ctx);
>   uint32_t intel_lr_context_size(struct intel_engine_cs *engine);
> -int intel_lr_context_deferred_alloc(struct intel_context *ctx,
> -				    struct intel_engine_cs *engine);
>   void intel_lr_context_unpin(struct intel_context *ctx,
>   			    struct intel_engine_cs *engine);

This one looks fine to me. (And very desirable to improve the layering 
like this.)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

Regards,

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