[PATCH] drm/i915: fix SFC reset flow

* [PATCH] drm/i915: fix SFC reset flow
@ 2019-09-16 21:41 Daniele Ceraolo Spurio
  2019-09-16 22:41 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Daniele Ceraolo Spurio @ 2019-09-16 21:41 UTC (permalink / raw)
  To: intel-gfx; +Cc: Owen Zhang

Our assumption that the we can ask the HW to lock the SFC even if not
currently in use does not match the HW commitment. The expectation from
the HW is that SW will not try to lock the SFC if the engine is not
using it and if we do that the behavior is undefined; on ICL the HW
ends up to returning the ack and ignoring our lock request, but this is
not guaranteed and we shouldn't expect it going forward.

Reported-by: Owen Zhang <owen.zhang@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_reset.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index 8327220ac558..900958804bd5 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -352,13 +352,15 @@ static u32 gen11_lock_sfc(struct intel_engine_cs *engine)
 	}
 
 	/*
-	 * Tell the engine that a software reset is going to happen. The engine
-	 * will then try to force lock the SFC (if currently locked, it will
-	 * remain so until we tell the engine it is safe to unlock; if currently
-	 * unlocked, it will ignore this and all new lock requests). If SFC
-	 * ends up being locked to the engine we want to reset, we have to reset
-	 * it as well (we will unlock it once the reset sequence is completed).
+	 * If the engine is using a SFC, tell the engine that a software reset
+	 * is going to happen. The engine will then try to force lock the SFC.
+	 * If SFC ends up being locked to the engine we want to reset, we have
+	 * to reset it as well (we will unlock it once the reset sequence is
+	 * completed).
 	 */
+	if (!(intel_uncore_read_fw(uncore, sfc_usage) & sfc_usage_bit))
+		return 0;
+
 	rmw_set_fw(uncore, sfc_forced_lock, sfc_forced_lock_bit);
 
 	if (__intel_wait_for_register_fw(uncore,
@@ -366,10 +368,13 @@ static u32 gen11_lock_sfc(struct intel_engine_cs *engine)
 					 sfc_forced_lock_ack_bit,
 					 sfc_forced_lock_ack_bit,
 					 1000, 0, NULL)) {
-		DRM_DEBUG_DRIVER("Wait for SFC forced lock ack failed\n");
+		/* did we race the unlock? */
+		if (intel_uncore_read_fw(uncore, sfc_usage) & sfc_usage_bit)
+			DRM_ERROR("Wait for SFC forced lock ack failed\n");
 		return 0;
 	}
 
+	/* The HW could return the ack even if the sfc is not in use */
 	if (intel_uncore_read_fw(uncore, sfc_usage) & sfc_usage_bit)
 		return sfc_reset_bit;
 
@@ -382,6 +387,7 @@ static void gen11_unlock_sfc(struct intel_engine_cs *engine)
 	u8 vdbox_sfc_access = RUNTIME_INFO(engine->i915)->vdbox_sfc_access;
 	i915_reg_t sfc_forced_lock;
 	u32 sfc_forced_lock_bit;
+	u32 lock;
 
 	switch (engine->class) {
 	case VIDEO_DECODE_CLASS:
@@ -401,7 +407,10 @@ static void gen11_unlock_sfc(struct intel_engine_cs *engine)
 		return;
 	}
 
-	rmw_clear_fw(uncore, sfc_forced_lock, sfc_forced_lock_bit);
+	lock = intel_uncore_read_fw(uncore, sfc_forced_lock);
+	if (lock & sfc_forced_lock_bit)
+		intel_uncore_write_fw(uncore, sfc_forced_lock,
+				      lock & ~sfc_forced_lock_bit);
 }
 
 static int gen11_reset_engines(struct intel_gt *gt,
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread