On Wed, Jan 20, 2016 at 02:06:43PM +0000, Tvrtko Ursulin wrote: > > On 20/01/16 13:55, Chris Wilson wrote: > >On Wed, Jan 20, 2016 at 01:40:57PM +0000, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin <[email protected]> > >> > >>In GuC mode LRC pinning lifetime depends exclusively on the > >>request liftime. Since that is terminated by the seqno update > >>that opens up a race condition between GPU finishing writing > >>out the context image and the driver unpinning the LRC. > >> > >>To extend the LRC lifetime we will employ a similar approach > >>to what legacy ringbuffer submission does. > >> > >>We will start tracking the last submitted context per engine > >>and keep it pinned until it is replaced by another one. > >> > >>Note that the driver unload path is a bit fragile and could > >>benefit greatly from efforts to unify the legacy and exec > >>list submission code paths. > >> > >>At the moment i915_gem_context_fini has special casing for the > >>two which are potentialy not needed, and also depends on > >>i915_gem_cleanup_ringbuffer running before itself. > >> > >>Signed-off-by: Tvrtko Ursulin <[email protected]> > >>Issue: VIZ-4277 > >>Cc: Chris Wilson <[email protected]> > >>Cc: Nick Hoath <[email protected]> > >>--- > >>I cannot test this with GuC but it passes BAT with execlists > >>and some real world smoke tests. > >>--- > >> drivers/gpu/drm/i915/i915_gem_context.c | 4 +++- > >> drivers/gpu/drm/i915/intel_lrc.c | 7 +++++++ > >> 2 files changed, 10 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > >>b/drivers/gpu/drm/i915/i915_gem_context.c > >>index c25083c78ba7..0b419e165836 100644 > >>--- a/drivers/gpu/drm/i915/i915_gem_context.c > >>+++ b/drivers/gpu/drm/i915/i915_gem_context.c > >>@@ -438,7 +438,9 @@ void i915_gem_context_fini(struct drm_device *dev) > >> for (i = 0; i < I915_NUM_RINGS; i++) { > >> struct intel_engine_cs *ring = &dev_priv->ring[i]; > >> > >>- if (ring->last_context) > >>+ if (ring->last_context && i915.enable_execlists) > >>+ intel_lr_context_unpin(ring->last_context, ring); > >>+ else if (ring->last_context) > >> i915_gem_context_unreference(ring->last_context); > >> > >> ring->default_context = NULL; > >>diff --git a/drivers/gpu/drm/i915/intel_lrc.c > >>b/drivers/gpu/drm/i915/intel_lrc.c > >>index 5c3f57fed916..b8a7e126d6d2 100644 > >>--- a/drivers/gpu/drm/i915/intel_lrc.c > >>+++ b/drivers/gpu/drm/i915/intel_lrc.c > >>@@ -918,6 +918,7 @@ int intel_execlists_submission(struct > >>i915_execbuffer_params *params, > >> struct intel_engine_cs *ring = params->ring; > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> struct intel_ringbuffer *ringbuf = > >> params->ctx->engine[ring->id].ringbuf; > >>+ struct intel_context *ctx = params->request->ctx; > >> u64 exec_start; > >> int instp_mode; > >> u32 instp_mask; > >>@@ -982,6 +983,12 @@ int intel_execlists_submission(struct > >>i915_execbuffer_params *params, > >> > >> trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags); > >> > >>+ if (ring->last_context && ring->last_context != ctx) { > >>+ intel_lr_context_unpin(ring->last_context, ring); > >>+ intel_lr_context_pin(ctx, ring); > >>+ ring->last_context = ctx; > >>+ } > > > >I think this is the wrong location and should be part of submitting the > >context inside the engine (because intel_execlists_submission should not > >as it is entirely duplicating the common GEM batch submision code and > >the unique part is engine->add_request()). > > So into engine->emit_request you are saying? That works just as well > AFAICS, just making sure I understood correctly.
Oh, yeah you haven't got the unify add_request/emit_request patch... So gen8_emit_request(), but considering the other patches in flight, intel_logical_ring_advance_and_submit() will be the ultimate location. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
