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>
>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
