Re: [PATCH 2/2] drm/i915/lrc: Prevent preemption when lite-restore is disabled

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Sep 10, 2015 at 06:37:17PM +0100, Arun Siluvery wrote:
> On 04/09/2015 12:59, Michel Thierry wrote:
> >When WaEnableForceRestoreInCtxtDescForVCS is required, it is only
> >safe to send new contexts if the last reported event is "active to
> >idle". Otherwise the same context can fully preempt itself because
> >lite-restore is disabled.
> >
> >Testcase: igt/gem_concurrent_blit
> >Reported-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_lrc.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index d8b605f..c3fca4b 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -277,10 +277,18 @@ u32 intel_execlists_ctx_id(struct drm_i915_gem_object *ctx_obj)
> >  	return lrca >> 12;
> >  }
> >
> >+static bool disable_lite_restore_wa(struct intel_engine_cs *ring)
> >+{
> >+	struct drm_device *dev = ring->dev;
> >+
> >+	return ((IS_SKYLAKE(dev) && INTEL_REVID(dev) <= SKL_REVID_B0) ||
> >+		(IS_BROXTON(dev) && INTEL_REVID(dev) == BXT_REVID_A0)) &&
> >+	       (ring->id == VCS || ring->id == VCS2);
> >+}
> >+
> >  uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
> >  				     struct intel_engine_cs *ring)
> >  {
> >-	struct drm_device *dev = ring->dev;
> >  	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
> >  	uint64_t desc;
> >  	uint64_t lrca = i915_gem_obj_ggtt_offset(ctx_obj) +
> >@@ -302,9 +310,7 @@ uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
> >
> >  	/* WaEnableForceRestoreInCtxtDescForVCS:skl */
> >  	/* WaEnableForceRestoreInCtxtDescForVCS:bxt */
> >-	if (((IS_SKYLAKE(dev) && INTEL_REVID(dev) <= SKL_REVID_B0) ||
> >-	     (IS_BROXTON(dev) && INTEL_REVID(dev) == BXT_REVID_A0)) &&
> >-	    (ring->id == VCS || ring->id == VCS2))
> >+	if (disable_lite_restore_wa(ring))
> >  		desc |= GEN8_CTX_FORCE_RESTORE;
> >
> >  	return desc;
> >@@ -495,7 +501,7 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring)
> >  	u32 status_pointer;
> >  	u8 read_pointer;
> >  	u8 write_pointer;
> >-	u32 status;
> >+	u32 status = 0;
> >  	u32 status_id;
> >  	u32 submit_contexts = 0;
> >
> >@@ -533,8 +539,14 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring)
> >  		}
> >  	}
> >
> >-	if (submit_contexts != 0)
> >+	if (disable_lite_restore_wa(ring)) {
> >+		/* Prevent a ctx to preempt itself */
> >+		if ((status & GEN8_CTX_STATUS_ACTIVE_IDLE) &&
> >+		    (submit_contexts != 0))
> >+			execlists_context_unqueue(ring);
> >+	} else if (submit_contexts != 0) {
> >  		execlists_context_unqueue(ring);
> >+	}
> >
> >  	spin_unlock(&ring->execlist_lock);
> >
> >
> 
> Need changes if we decide to drop SKL, otherwise also it looks good to me,
> Reviewed-by: Arun Siluvery <arun.siluvery@linux.intel.com>

Both patches applied to dinq, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx