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

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [CI 14/25] drm/i915: Manually unwind after a failed request allocation
Date: Thu, 28 Apr 2016 15:55:03 +0100	[thread overview]
Message-ID: <20160428145503.GA8055@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <572221A8.1000600@intel.com>

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@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>
> >Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx