[Intel-gfx] [PATCH 2/2] drm/i915: Track the previous pinned context inside the request

[Chris Wilson]

On Wed, Apr 20, 2016 at 03:08:19PM +0100, Tvrtko Ursulin wrote:
> 
> On 19/04/16 13:59, Chris Wilson wrote:
> >As the contexts are accessed by the hardware until the switch is completed
> >to a new context, the hardware may still be writing to the context object
> >after the breadcrumb is visible. We must not unpin/unbind/prune that
> >object whilst still active and so we keep the previous context pinned until
> >the following request. If we move this tracking onto the request, we can
> >simplify the code and treat execlists/GuC dispatch identically.
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/i915_drv.h  | 11 +++++++++++
> >  drivers/gpu/drm/i915/i915_gem.c  |  8 ++++----
> >  drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++---------
> >  3 files changed, 23 insertions(+), 13 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index c59b2670cc36..be98e9643072 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -2302,6 +2302,17 @@ struct drm_i915_gem_request {
> >  	struct intel_context *ctx;
> >  	struct intel_ringbuffer *ringbuf;
> >
> >+	/**
> >+	 * Context related to the previous request.
> >+	 * As the contexts are accessed by the hardware until the switch is
> >+	 * completed to a new context, the hardware may still be writing
> >+	 * to the context object after the breadcrumb is visible. We must
> >+	 * not unpin/unbind/prune that object whilst still active and so
> >+	 * we keep the previous context pinned until the following (this)
> >+	 * request is retired.
> >+	 */
> >+	struct intel_context *previous_context;
> >+
> >  	/** Batch buffer related to this request if any (used for
> >  	    error state dump only) */
> >  	struct drm_i915_gem_object *batch_obj;
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 9b4854a17264..537aacfda3eb 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -1413,13 +1413,13 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request)
> >  	list_del_init(&request->list);
> >  	i915_gem_request_remove_from_client(request);
> >
> >-	if (request->ctx) {
> >+	if (request->previous_context) {
> >  		if (i915.enable_execlists)
> >-			intel_lr_context_unpin(request->ctx, request->engine);
> >-
> >-		i915_gem_context_unreference(request->ctx);
> >+			intel_lr_context_unpin(request->previous_context,
> >+					       request->engine);
> >  	}
> >
> >+	i915_gem_context_unreference(request->ctx);
> >  	i915_gem_request_unreference(request);
> >  }
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index ee4e9bb80042..06e013293ec6 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -590,7 +590,6 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
> >  	struct drm_i915_gem_request *cursor;
> >  	int num_elements = 0;
> >
> >-	intel_lr_context_pin(request->ctx, request->engine);
> 
> I really really think this must go in a separate, subsequent patch.
> 
> Both from the conceptual side, leaving this patch to just extend
> pinning, not limit it; and from the POV that there is a bug unless a
> patch like mine which I pasted in yesterday is inserted between them
> ("drm/i915: Store LRC hardware id in the context", note the summary
> is wrong, it is storing in requests not contexts so I have to rename
> it).
> 
> Otherwise execlists_check_remove_request when accessing
> head_req->ctx is use after free. And I can demonstrate that easily
> via gem-close-race. Put a
> WARN_ON(atomic_read(&head_req->ctx->ref.refcount) == 0); and see. :)

Oh, I don't have those racy accesses in my tree.
 
> What I think happens is that with two submission ports, we can get
> two context completions aggregated in an interrupt which comes after
> the seqno for both has been consumed by GEM and so LRCs unpinned.
> 
> But with your persistent ctx hw id patches, I think the course is
> fine to do this including the complete elimination of the execlist
> retired queue.
> 
> You can just drop the two chunks for the patch and I will follow up
> with two patches to finish it all off.

Or I could bring some more patches forward ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre