[Intel-gfx] [CI 14/25] drm/i915: Manually unwind after a failed request allocation

[Dave Gordon]

On 28/04/16 09:56, 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.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at 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 dd628eae0592..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;

If you move the begin() into the mechanism-specific code, you should 
logically move the above assignment with it; after all, the eventual 
add_request will itself indirect back to submission-mechanism-dependent 
code which may (will!) have to add different amounts of code to the ring 
according to the operation of the different mechanisms.

Or if you prefer, the above constant could mean "how much the common 
submission-independent code will need for submission", and each 
submission mechanism could then *add* its own maximum requirement before 
the begin() call.

.Dave.

> -	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, along with any reserved space.
> -		 */
> -		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 68e50b221f63..492f9c671c6f 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2339,7 +2339,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);
>   }
>
>   static int wait_for_space(struct drm_i915_gem_request *req, int bytes)
>