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 <ch...@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuopp...@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursu...@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahti...@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
