On 12/22/22 3:47 PM, Andi Shyti wrote:
Hi GG,
drivers/gpu/drm/i915/gt/intel_reset.c | 34
++++++++++++++++++++++-----
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
b/drivers/gpu/drm/i915/gt/intel_reset.c
index ffde89c5835a4..88dfc0c5316ff 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -268,6 +268,7 @@ static int ilk_do_reset(struct intel_gt *gt,
intel_engine_mask_t engine_mask,
static int gen6_hw_domain_reset(struct intel_gt *gt, u32
hw_domain_mask)
{
struct intel_uncore *uncore = gt->uncore;
+ int loops = 2;
int err;
/*
@@ -275,18 +276,39 @@ static int gen6_hw_domain_reset(struct
intel_gt *gt, u32 hw_domain_mask)
* for fifo space for the write or forcewake the chip for
* the read
*/
- intel_uncore_write_fw(uncore, GEN6_GDRST,
hw_domain_mask);
+ do {
+ intel_uncore_write_fw(uncore, GEN6_GDRST,
hw_domain_mask);
- /* Wait for the device to ack the reset requests */
- err = __intel_wait_for_register_fw(uncore,
- GEN6_GDRST,
hw_domain_mask, 0,
- 500, 0,
- NULL);
+ /*
+ * Wait for the device to ack the reset requests.
+ *
+ * On some platforms, e.g. Jasperlake, we see see
that the
+ * engine register state is not cleared until
shortly after
+ * GDRST reports completion, causing a failure as
we try
+ * to immediately resume while the internal state
is still
+ * in flux. If we immediately repeat the reset,
the second
+ * reset appears to serialise with the first, and
since
+ * it is a no-op, the registers should retain
their reset
+ * value. However, there is still a concern that
upon
+ * leaving the second reset, the internal engine
state
+ * is still in flux and not ready for resuming.
+ */
+ err = __intel_wait_for_register_fw(uncore,
GEN6_GDRST,
+
hw_domain_mask, 0,
+ 2000, 0,
+ NULL);
Andi, fast_timeout_us is increased from 500 to 2000, and if it fails, it
tries to reset it once more. How was this value of 2000 calculated?
No real reason, it's just an empiric choice to make the call a
bit more robust and suffer less from delayed feedback.
+ } while (err == 0 && --loops);
if (err)
GT_TRACE(gt,
"Wait for 0x%08x engines reset
failed\n",
hw_domain_mask);
Did GT_TRACE report an error in a situation where the problem was reported?
I guess so, in Jasperlake.
+ /*
+ * As we have observed that the engine state is still
volatile
+ * after GDRST is acked, impose a small delay to let
everything settle.
+ */
+ udelay(50);
udelay(50) affects all platforms that can call gen6_hw_domain_reset(), is
that intended?
Yes, that's intended as apparently we need to give it a bit more
time for the engines to recover from the reset. We are here in
atomic context and we need udelay to wait atomically, thus
udelay().
Hi Andi,
In scenarios/platforms where GSC Firmware is not used, reset through FLR
is not possible and this reset function is used.
Therefore if this problem cannot be avoided by other WAs and this method
is the only one, we might have to apply this patch as a temporal fix.
But we also ask the person who is in charge of this HW Platform
(Jasperlake or all of GEN11?)to analyze the problem and you need to sure
get proper WA guidance as a next step.
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
Br,
G.G.
Thank you,
Andi
