Message ID | 1460123698-16832-3-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, Apr 08, 2016 at 02:54:56PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko@ursulin.net> > > In GuC mode submitting requests is only putting them into the > GuC queue with the actual submission to hardware following at > some future point. This makes the per engine last context > tracking insufficient for closing the premature context > unpin race. > > Instead we need to make requests track and pin the previous > context on the same engine, and only unpin them when they > themselves are retired. This will ensure contexts remain pinned > in all cases until the are fully saved by the GPU. > > v2: Only track contexts. > > Signed-off-by: Tvrtko Ursulin <tvrtko@ursulin.net> > Cc: Alex Dai <yu.dai@intel.com> > Cc: Dave Gordon <david.s.gordon@intel.com> > Cc: Nick Hoath <nicholas.hoath@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 +++- > drivers/gpu/drm/i915/i915_gem.c | 5 +++++ > drivers/gpu/drm/i915/intel_lrc.c | 17 +++++++++++++++++ > 3 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index fa199b7e51b5..216b2297ae3e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2283,6 +2283,9 @@ struct drm_i915_gem_request { > struct intel_context *ctx; > struct intel_ringbuffer *ringbuf; > > + /** Context preceding this one on the same engine. Can be NULL. */ > + struct intel_context *prev_ctx; > + > /** Batch buffer related to this request if any (used for > error state dump only) */ > struct drm_i915_gem_object *batch_obj; > @@ -2318,7 +2321,6 @@ struct drm_i915_gem_request { > > /** Execlists no. of times this request has been sent to the ELSP */ > int elsp_submitted; > - > }; > > struct drm_i915_gem_request * __must_check > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index c1df696f9002..118911ce7231 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1445,6 +1445,11 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request) > */ > request->ringbuf->last_retired_head = request->postfix; > > + if (request->prev_ctx) { > + intel_lr_context_unpin(request->prev_ctx, request->engine); > + request->prev_ctx = NULL; > + } > + > __i915_gem_request_release(request); > } > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 31445aa3429b..3d08dac0a9b0 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -798,6 +798,23 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request) > if (intel_engine_stopped(engine)) > return 0; > > + /* > + * Pin the previous context on this engine to ensure it is not > + * prematurely unpinned in GuC mode. > + * Previous context will be unpinned when this request is retired, > + * ensuring the GPU has switched out from the previous context and into > + * a new context at that point. > + */ > + if (engine->last_context) { > + if (engine->last_context != request->i915->kernel_context) { > + intel_lr_context_pin(engine->last_context, engine); > + request->prev_ctx = engine->last_context; > + } > + } > + > + /* > + * Track and pin the last context submitted on an engine. > + */ > if (engine->last_context != request->ctx) { > if (engine->last_context) > intel_lr_context_unpin(engine->last_context, engine); We can use pinned_context to reduce the code complexity, not make it worse. If you first apply the execlists default context fix, adding pinned_context is a reduction is code. -Chris
On 08/04/16 15:37, Chris Wilson wrote: > On Fri, Apr 08, 2016 at 02:54:56PM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko@ursulin.net> >> >> In GuC mode submitting requests is only putting them into the >> GuC queue with the actual submission to hardware following at >> some future point. This makes the per engine last context >> tracking insufficient for closing the premature context >> unpin race. >> >> Instead we need to make requests track and pin the previous >> context on the same engine, and only unpin them when they >> themselves are retired. This will ensure contexts remain pinned >> in all cases until the are fully saved by the GPU. >> >> v2: Only track contexts. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko@ursulin.net> >> Cc: Alex Dai <yu.dai@intel.com> >> Cc: Dave Gordon <david.s.gordon@intel.com> >> Cc: Nick Hoath <nicholas.hoath@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 4 +++- >> drivers/gpu/drm/i915/i915_gem.c | 5 +++++ >> drivers/gpu/drm/i915/intel_lrc.c | 17 +++++++++++++++++ >> 3 files changed, 25 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index fa199b7e51b5..216b2297ae3e 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2283,6 +2283,9 @@ struct drm_i915_gem_request { >> struct intel_context *ctx; >> struct intel_ringbuffer *ringbuf; >> >> + /** Context preceding this one on the same engine. Can be NULL. */ >> + struct intel_context *prev_ctx; >> + >> /** Batch buffer related to this request if any (used for >> error state dump only) */ >> struct drm_i915_gem_object *batch_obj; >> @@ -2318,7 +2321,6 @@ struct drm_i915_gem_request { >> >> /** Execlists no. of times this request has been sent to the ELSP */ >> int elsp_submitted; >> - >> }; >> >> struct drm_i915_gem_request * __must_check >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index c1df696f9002..118911ce7231 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -1445,6 +1445,11 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request) >> */ >> request->ringbuf->last_retired_head = request->postfix; >> >> + if (request->prev_ctx) { >> + intel_lr_context_unpin(request->prev_ctx, request->engine); >> + request->prev_ctx = NULL; >> + } >> + >> __i915_gem_request_release(request); >> } >> >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> index 31445aa3429b..3d08dac0a9b0 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -798,6 +798,23 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request) >> if (intel_engine_stopped(engine)) >> return 0; >> >> + /* >> + * Pin the previous context on this engine to ensure it is not >> + * prematurely unpinned in GuC mode. >> + * Previous context will be unpinned when this request is retired, >> + * ensuring the GPU has switched out from the previous context and into >> + * a new context at that point. >> + */ >> + if (engine->last_context) { >> + if (engine->last_context != request->i915->kernel_context) { >> + intel_lr_context_pin(engine->last_context, engine); >> + request->prev_ctx = engine->last_context; >> + } >> + } >> + >> + /* >> + * Track and pin the last context submitted on an engine. >> + */ >> if (engine->last_context != request->ctx) { >> if (engine->last_context) >> intel_lr_context_unpin(engine->last_context, engine); > > We can use pinned_context to reduce the code complexity, not make it > worse. > > If you first apply the execlists default context fix, adding > pinned_context is a reduction is code. Okay I'll pull that patch in this series. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index fa199b7e51b5..216b2297ae3e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2283,6 +2283,9 @@ struct drm_i915_gem_request { struct intel_context *ctx; struct intel_ringbuffer *ringbuf; + /** Context preceding this one on the same engine. Can be NULL. */ + struct intel_context *prev_ctx; + /** Batch buffer related to this request if any (used for error state dump only) */ struct drm_i915_gem_object *batch_obj; @@ -2318,7 +2321,6 @@ struct drm_i915_gem_request { /** Execlists no. of times this request has been sent to the ELSP */ int elsp_submitted; - }; struct drm_i915_gem_request * __must_check diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c1df696f9002..118911ce7231 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1445,6 +1445,11 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request) */ request->ringbuf->last_retired_head = request->postfix; + if (request->prev_ctx) { + intel_lr_context_unpin(request->prev_ctx, request->engine); + request->prev_ctx = NULL; + } + __i915_gem_request_release(request); } diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 31445aa3429b..3d08dac0a9b0 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -798,6 +798,23 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request) if (intel_engine_stopped(engine)) return 0; + /* + * Pin the previous context on this engine to ensure it is not + * prematurely unpinned in GuC mode. + * Previous context will be unpinned when this request is retired, + * ensuring the GPU has switched out from the previous context and into + * a new context at that point. + */ + if (engine->last_context) { + if (engine->last_context != request->i915->kernel_context) { + intel_lr_context_pin(engine->last_context, engine); + request->prev_ctx = engine->last_context; + } + } + + /* + * Track and pin the last context submitted on an engine. + */ if (engine->last_context != request->ctx) { if (engine->last_context) intel_lr_context_unpin(engine->last_context, engine);