Re: [PATCH v6 14/25] drm/i915: Manually unwind after a failed request allocation

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ti, 2016-04-26 at 21:06 +0100, Chris Wilson wrote:
> In the next patches, we want to move the work out of freeing the request
> and into its retirement (so that we can free the request without
> requiring the struct_mutex). This means that we cannot rely on
> unreferencing the request to completely teardown the request any more
> and so we need to manually unwind the failed allocation. In doing so, we
> reorder the allocation in order to make the unwind simple (and ensure
> that we don't try to unwind a partial request that may have modified
> global state) and so we end up pushing the initial preallocation down
> into the engine request initialisation functions where we have the
> requisite control over the state of the request.
> 
> Moving the initial preallocation into the engine is less than ideal: it
> moves logic to handle a specific problem with request handling out of
> the common code. On the other hand, it does allow those backends
> significantly more flexibility in performing its allocations.
> 

Adding John as CC,

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 28 +++++++++-------------------
>  drivers/gpu/drm/i915/intel_lrc.c        | 16 ++++++++++++++--
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  2 +-
>  3 files changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0e27484bd28a..d7ff5e79182f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2766,15 +2766,6 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine,
>  	req->ctx  = ctx;
>  	i915_gem_context_reference(req->ctx);
>  
> -	if (i915.enable_execlists)
> -		ret = intel_logical_ring_alloc_request_extras(req);
> -	else
> -		ret = intel_ring_alloc_request_extras(req);
> -	if (ret) {
> -		i915_gem_context_unreference(req->ctx);
> -		goto err;
> -	}
> -
>  	/*
>  	 * Reserve space in the ring buffer for all the commands required to
>  	 * eventually emit this request. This is to guarantee that the
> @@ -2783,20 +2774,19 @@ __i915_gem_request_alloc(struct intel_engine_cs *engine,
>  	 * away, e.g. because a GPU scheduler has deferred it.
>  	 */
>  	req->reserved_space = MIN_SPACE_FOR_ADD_REQUEST;
> -	ret = intel_ring_begin(req, 0);
> -	if (ret) {
> -		/*
> -		 * At this point, the request is fully allocated even if not
> -		 * fully prepared. Thus it can be cleaned up using the proper
> -		 * free code.
> -		 */
> -		i915_gem_request_unreference(req);
> -		return ret;
> -	}
> +
> +	if (i915.enable_execlists)
> +		ret = intel_logical_ring_alloc_request_extras(req);
> +	else
> +		ret = intel_ring_alloc_request_extras(req);
> +	if (ret)
> +		goto err_ctx;
>  
>  	*req_out = req;
>  	return 0;
>  
> +err_ctx:
> +	i915_gem_context_unreference(ctx);
>  err:
>  	kmem_cache_free(dev_priv->requests, req);
>  	return ret;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 910044cf143e..01517dd7069b 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -698,7 +698,7 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
>  
>  int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request)
>  {
> -	int ret = 0;
> +	int ret;
>  
>  	request->ringbuf = request->ctx->engine[request->engine->id].ringbuf;
>  
> @@ -715,9 +715,21 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>  			return ret;
>  	}
>  
> -	if (request->ctx != request->i915->kernel_context)
> +	if (request->ctx != request->i915->kernel_context) {
>  		ret = intel_lr_context_pin(request->ctx, request->engine);
> +		if (ret)
> +			return ret;
> +	}
>  
> +	ret = intel_ring_begin(request, 0);
> +	if (ret)
> +		goto err_unpin;
> +
> +	return 0;
> +
> +err_unpin:
> +	if (request->ctx != request->i915->kernel_context)
> +		intel_lr_context_unpin(request->ctx, request->engine);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ba5946b9fa06..1193372f74fd 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2333,7 +2333,7 @@ int intel_engine_idle(struct intel_engine_cs *engine)
>  int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request)
>  {
>  	request->ringbuf = request->engine->buffer;
> -	return 0;
> +	return intel_ring_begin(request, 0);
>  }

The names of these two _extras functions with the added functionality
do not make sense.

Regards, Joonas

>  
>  static int wait_for_space(struct drm_i915_gem_request *req, int bytes)
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx