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

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 17/19] drm/i915: Track the previous pinned context inside the request
Date: Thu, 21 Apr 2016 10:35:23 +0300	[thread overview]
Message-ID: <1461224123.4381.23.camel@linux.intel.com> (raw)
In-Reply-To: <20160421072242.GF15733@nuc-i3427.alporthouse.com>

On to, 2016-04-21 at 08:22 +0100, Chris Wilson wrote:
> 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@chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Below comments addressed,
> > 
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > 
<SNIP>
> > > 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.

Might be worthy a note in the commit message. "Remove redundand check
for request->ctx being NULL."

Regards, Joonas

> -Chris
> 
-- 
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