[Intel-gfx] [PATCH] drm/i915: Change context lifecycle
Yu Dai
yu.dai at intel.com
Tue Nov 24 17:11:12 PST 2015
On 11/24/2015 08:23 AM, Nick Hoath wrote:
> Use the first retired request on a new context to unpin
> the old context. This ensures that the hw context remains
> bound until it has been saved.
> Now that the context is pinned until later in the request/context
> lifecycle, it no longer needs to be pinned from context_queue to
> retire_requests.
> This is to solve a hang with GuC submission, and a theoretical
> issue with execlist submission.
>
> v2: Moved the new pin to cover GuC submission (Alex Dai)
> Moved the new unpin to request_retire to fix coverage leak
> v3: Added switch to default context if freeing a still pinned
> context just in case the hw was actually still using it
> v4: Unwrapped context unpin to allow calling without a request
>
> Signed-off-by: Nick Hoath <nicholas.hoath at intel.com>
> Issue: VIZ-4277
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: David Gordon <david.s.gordon at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Alex Dai <yu.dai at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem.c | 9 ++++-
> drivers/gpu/drm/i915/intel_lrc.c | 73 ++++++++++++++++++++++++++++++----------
> drivers/gpu/drm/i915/intel_lrc.h | 1 +
> 4 files changed, 65 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d5cf30b..4d2f44c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -889,6 +889,7 @@ struct intel_context {
> struct {
> struct drm_i915_gem_object *state;
> struct intel_ringbuffer *ringbuf;
> + bool unsaved;
> int pin_count;
> } engine[I915_NUM_RINGS];
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e955499..6fee473 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1354,6 +1354,14 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
> {
> trace_i915_gem_request_retire(request);
>
> + if (i915.enable_execlists) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&request->ring->execlist_lock, flags);
> + intel_lr_context_complete_check(request);
> + spin_unlock_irqrestore(&request->ring->execlist_lock, flags);
> + }
> +
> /* We know the GPU must have read the request to have
> * sent us the seqno + interrupt, so use the position
> * of tail of the request to update the last known position
> @@ -1384,7 +1392,6 @@ __i915_gem_request_retire__upto(struct drm_i915_gem_request *req)
> do {
> tmp = list_first_entry(&engine->request_list,
> typeof(*tmp), list);
> -
> i915_gem_request_retire(tmp);
> } while (tmp != req);
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 06180dc..a527c21 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -566,9 +566,6 @@ static int execlists_context_queue(struct drm_i915_gem_request *request)
> struct drm_i915_gem_request *cursor;
> int num_elements = 0;
>
> - if (request->ctx != ring->default_context)
> - intel_lr_context_pin(request);
> -
> i915_gem_request_reference(request);
>
> spin_lock_irq(&ring->execlist_lock);
> @@ -728,10 +725,16 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
> intel_logical_ring_advance(request->ringbuf);
>
> request->tail = request->ringbuf->tail;
> -
> if (intel_ring_stopped(ring))
> return;
>
> + if (request->ctx != ring->default_context) {
> + if (!request->ctx->engine[ring->id].unsaved) {
> + intel_lr_context_pin(request);
> + request->ctx->engine[ring->id].unsaved = true;
> + }
> + }
> +
> if (dev_priv->guc.execbuf_client)
> i915_guc_submit(dev_priv->guc.execbuf_client, request);
> else
> @@ -958,12 +961,6 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
> spin_unlock_irq(&ring->execlist_lock);
>
> list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
> - struct intel_context *ctx = req->ctx;
> - struct drm_i915_gem_object *ctx_obj =
> - ctx->engine[ring->id].state;
> -
> - if (ctx_obj && (ctx != ring->default_context))
> - intel_lr_context_unpin(req);
> list_del(&req->execlist_link);
> i915_gem_request_unreference(req);
> }
> @@ -1058,21 +1055,41 @@ reset_pin_count:
> return ret;
> }
>
> -void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
> +static void intel_lr_context_unpin_no_req(struct intel_engine_cs *ring,
> + struct intel_context *ctx)
> {
> - struct intel_engine_cs *ring = rq->ring;
> - struct drm_i915_gem_object *ctx_obj = rq->ctx->engine[ring->id].state;
> - struct intel_ringbuffer *ringbuf = rq->ringbuf;
> -
> + struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
> + struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
> if (ctx_obj) {
> WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
> - if (--rq->ctx->engine[ring->id].pin_count == 0) {
> + if (--ctx->engine[ring->id].pin_count == 0) {
> intel_unpin_ringbuffer_obj(ringbuf);
> i915_gem_object_ggtt_unpin(ctx_obj);
> }
> }
> }
>
> +void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
> +{
> + intel_lr_context_unpin_no_req(rq->ring, rq->ctx);
> +}
> +
> +void intel_lr_context_complete_check(struct drm_i915_gem_request *req)
> +{
> + struct intel_engine_cs *ring = req->ring;
> +
> + assert_spin_locked(&ring->execlist_lock);
> +
> + if (ring->last_context && ring->last_context != req->ctx &&
> + ring->last_context->engine[ring->id].unsaved) {
Context pin is done for every submission but unpin is done with
condition. I think the engine.pin_count will be out of sync.
> + intel_lr_context_unpin_no_req(
> + ring,
> + ring->last_context);
> + ring->last_context->engine[ring->id].unsaved = false;
> + }
> + ring->last_context = req->ctx;
> +}
> +
> static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
> {
> int ret, i;
> @@ -2378,7 +2395,7 @@ void intel_lr_context_free(struct intel_context *ctx)
> {
> int i;
>
> - for (i = 0; i < I915_NUM_RINGS; i++) {
> + for (i = 0; i < I915_NUM_RINGS; ++i) {
> struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
>
> if (ctx_obj) {
> @@ -2390,7 +2407,20 @@ void intel_lr_context_free(struct intel_context *ctx)
> intel_unpin_ringbuffer_obj(ringbuf);
> i915_gem_object_ggtt_unpin(ctx_obj);
> }
> - WARN_ON(ctx->engine[ring->id].pin_count);
> +
> + if (ctx->engine[ring->id].unsaved) {
> + int ret;
> + struct drm_i915_gem_request *req;
> +
> + ret = i915_gem_request_alloc(ring,
> + ring->default_context, &req);
> + if (!ret) {
> + i915_add_request(req);
> + i915_wait_request(req);
> + }
The will wait for all requests to be finished. Waiting for the head is
good enough. If no request in queue, then add one from default context.
Alex
> + }
> +
> + WARN_ON(ctx->engine[i].pin_count);
> intel_ringbuffer_free(ringbuf);
> drm_gem_object_unreference(&ctx_obj->base);
> }
> @@ -2554,5 +2584,12 @@ void intel_lr_context_reset(struct drm_device *dev,
>
> ringbuf->head = 0;
> ringbuf->tail = 0;
> +
> + if (ctx->engine[ring->id].unsaved) {
> + intel_lr_context_unpin_no_req(
> + ring,
> + ctx);
> + ctx->engine[ring->id].unsaved = false;
> + }
> }
> }
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 4e60d54..cbc42b8 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -86,6 +86,7 @@ void intel_lr_context_reset(struct drm_device *dev,
> struct intel_context *ctx);
> uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
> struct intel_engine_cs *ring);
> +void intel_lr_context_complete_check(struct drm_i915_gem_request *req);
>
> /* Execlists */
> int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists);
More information about the Intel-gfx
mailing list