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

[Chris Wilson]

On Thu, Apr 28, 2016 at 03:43:52PM +0100, Dave Gordon wrote:
> 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.

That's where we are going with this. We need 336 bytes for legacy and 128
bytes for execlists (today). Moving this from a common to an engine
specific value makes sense, especially as we are looking to make this more
dynamic in the future.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre