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

[Chris Wilson]

On Thu, Apr 21, 2016 at 10:07:50AM +0300, Joonas Lahtinen wrote:
> On ke, 2016-04-20 at 19:42 +0100, 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. We can generalise the tracking we already do via
> > the engine->last_context and move it to the request so that it works
> > equally for execlists and GuC.
> > 
> > v2: Drop the execlists double pin as that exposes a race inside the lrc
> > irq handler as it tries to access the context after it may be retired.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> 
> Below comments addressed,
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  | 11 +++++++++++
> >  drivers/gpu/drm/i915/i915_gem.c  |  8 ++++----
> >  drivers/gpu/drm/i915/intel_lrc.c | 14 ++++++++------
> >  3 files changed, 23 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index a26a026ef8e2..515b8badce61 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 eaf1d13c943f..dbfc38f91f7d 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) {
> 
> Could be if (i915.enable_execlists && request->previous_context) now.

I didn't do this because I intend (been trying!) to remove the execlists
special case. So this will be

if (request->previous_context)
	request->engine->unpin_context(request->engine,
				       request->previous_context);

Though the plan I actually have is to move the context over to activity
tracking so that it finally behaves like legacy. And one step closer to
removing the explicit ordering requirement between contexts.

> >  		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);
> 
> Obviously some previous patch changed it so that request->ctx is never
> NULL at this point, as no more testing is done.

Since requests.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre