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@xxxxxxxxxxxxxxxxxx>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > Below comments addressed,
> >
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> >
<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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
