intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals
@ 2021-01-19  9:40 Chris Wilson
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 2/5] drm/i915/selftests: Prepare the selftests for engine resets with ring submission Chris Wilson
                   ` (7 more replies)
  0 siblings, 8 replies; 17+ messages in thread
From: Chris Wilson @ 2021-01-19  9:40 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson

CI reports that Baytail requires one more invalidate after CACHE_MODE
for it to be happy.

Fixes: ace44e13e577 ("drm/i915/gt: Clear CACHE_MODE prior to clearing residuals")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
---
 drivers/gpu/drm/i915/gt/gen7_renderclear.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/gen7_renderclear.c b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
index 39478712769f..8551e6de50e8 100644
--- a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
+++ b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
@@ -353,19 +353,21 @@ static void gen7_emit_pipeline_flush(struct batch_chunk *batch)
 
 static void gen7_emit_pipeline_invalidate(struct batch_chunk *batch)
 {
-	u32 *cs = batch_alloc_items(batch, 0, 8);
+	u32 *cs = batch_alloc_items(batch, 0, 10);
 
 	/* ivb: Stall before STATE_CACHE_INVALIDATE */
-	*cs++ = GFX_OP_PIPE_CONTROL(4);
+	*cs++ = GFX_OP_PIPE_CONTROL(5);
 	*cs++ = PIPE_CONTROL_STALL_AT_SCOREBOARD |
 		PIPE_CONTROL_CS_STALL;
 	*cs++ = 0;
 	*cs++ = 0;
+	*cs++ = 0;
 
-	*cs++ = GFX_OP_PIPE_CONTROL(4);
+	*cs++ = GFX_OP_PIPE_CONTROL(5);
 	*cs++ = PIPE_CONTROL_STATE_CACHE_INVALIDATE;
 	*cs++ = 0;
 	*cs++ = 0;
+	*cs++ = 0;
 
 	batch_advance(batch, cs);
 }
@@ -397,6 +399,7 @@ static void emit_batch(struct i915_vma * const vma,
 	batch_add(&cmds, 0xffff0000);
 	batch_add(&cmds, i915_mmio_reg_offset(CACHE_MODE_1));
 	batch_add(&cmds, 0xffff0000 | PIXEL_SUBSPAN_COLLECT_OPT_DISABLE);
+	gen7_emit_pipeline_invalidate(&cmds);
 	gen7_emit_pipeline_flush(&cmds);
 
 	/* Switch to the media pipeline and our base address */
-- 
2.20.1

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

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

* [Intel-gfx] [PATCH 2/5] drm/i915/selftests: Prepare the selftests for engine resets with ring submission
  2021-01-19  9:40 [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Chris Wilson
@ 2021-01-19  9:40 ` Chris Wilson
  2021-01-19 10:26   ` Mika Kuoppala
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 3/5] drm/i915/gt: Lift stop_ring() to reset_prepare Chris Wilson
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2021-01-19  9:40 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson

The engine resets selftests kick the tasklets, safe up until now as only
execlists supported engine resets.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 18 ++++++++++++++----
 drivers/gpu/drm/i915/gt/selftest_reset.c     | 11 ++++++++---
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
index 460c3e9542f4..463bb6a700c8 100644
--- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
+++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
@@ -704,6 +704,7 @@ static int __igt_reset_engine(struct intel_gt *gt, bool active)
 
 	for_each_engine(engine, gt, id) {
 		unsigned int reset_count, reset_engine_count;
+		unsigned long count;
 		IGT_TIMEOUT(end_time);
 
 		if (active && !intel_engine_can_store_dword(engine))
@@ -721,6 +722,7 @@ static int __igt_reset_engine(struct intel_gt *gt, bool active)
 
 		st_engine_heartbeat_disable(engine);
 		set_bit(I915_RESET_ENGINE + id, &gt->reset.flags);
+		count = 0;
 		do {
 			if (active) {
 				struct i915_request *rq;
@@ -770,9 +772,13 @@ static int __igt_reset_engine(struct intel_gt *gt, bool active)
 				err = -EINVAL;
 				break;
 			}
+
+			count++;
 		} while (time_before(jiffies, end_time));
 		clear_bit(I915_RESET_ENGINE + id, &gt->reset.flags);
 		st_engine_heartbeat_enable(engine);
+		pr_info("%s: Completed %lu %s resets\n",
+			engine->name, count, active ? "active" : "idle");
 
 		if (err)
 			break;
@@ -1623,7 +1629,8 @@ static int igt_reset_queue(void *arg)
 			prev = rq;
 			count++;
 		} while (time_before(jiffies, end_time));
-		pr_info("%s: Completed %d resets\n", engine->name, count);
+		pr_info("%s: Completed %d queued resets\n",
+			engine->name, count);
 
 		*h.batch = MI_BATCH_BUFFER_END;
 		intel_gt_chipset_flush(engine->gt);
@@ -1720,7 +1727,8 @@ static int __igt_atomic_reset_engine(struct intel_engine_cs *engine,
 	GEM_TRACE("i915_reset_engine(%s:%s) under %s\n",
 		  engine->name, mode, p->name);
 
-	tasklet_disable(t);
+	if (t->func)
+		tasklet_disable(t);
 	if (strcmp(p->name, "softirq"))
 		local_bh_disable();
 	p->critical_section_begin();
@@ -1730,8 +1738,10 @@ static int __igt_atomic_reset_engine(struct intel_engine_cs *engine,
 	p->critical_section_end();
 	if (strcmp(p->name, "softirq"))
 		local_bh_enable();
-	tasklet_enable(t);
-	tasklet_hi_schedule(t);
+	if (t->func) {
+		tasklet_enable(t);
+		tasklet_hi_schedule(t);
+	}
 
 	if (err)
 		pr_err("i915_reset_engine(%s:%s) failed under %s\n",
diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
index b7befcfbdcde..8784257ec808 100644
--- a/drivers/gpu/drm/i915/gt/selftest_reset.c
+++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
@@ -321,7 +321,10 @@ static int igt_atomic_engine_reset(void *arg)
 		goto out_unlock;
 
 	for_each_engine(engine, gt, id) {
-		tasklet_disable(&engine->execlists.tasklet);
+		struct tasklet_struct *t = &engine->execlists.tasklet;
+
+		if (t->func)
+			tasklet_disable(t);
 		intel_engine_pm_get(engine);
 
 		for (p = igt_atomic_phases; p->name; p++) {
@@ -345,8 +348,10 @@ static int igt_atomic_engine_reset(void *arg)
 		}
 
 		intel_engine_pm_put(engine);
-		tasklet_enable(&engine->execlists.tasklet);
-		tasklet_hi_schedule(&engine->execlists.tasklet);
+		if (t->func) {
+			tasklet_enable(t);
+			tasklet_hi_schedule(t);
+		}
 		if (err)
 			break;
 	}
-- 
2.20.1

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

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

* [Intel-gfx] [PATCH 3/5] drm/i915/gt: Lift stop_ring() to reset_prepare
  2021-01-19  9:40 [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Chris Wilson
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 2/5] drm/i915/selftests: Prepare the selftests for engine resets with ring submission Chris Wilson
@ 2021-01-19  9:40 ` Chris Wilson
  2021-01-19 10:33   ` Mika Kuoppala
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 4/5] drm/i915/gt: Pull ring submission resume under its caller forcewake Chris Wilson
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2021-01-19  9:40 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson

Push the sleeping stop_ring() out of the reset resume function to reset
prepare; we are not allowed to sleep in the former.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 .../gpu/drm/i915/gt/intel_ring_submission.c   | 97 +++++++------------
 1 file changed, 36 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
index 8d0964d2d597..44159595d909 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
@@ -157,21 +157,6 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
 	flush_cs_tlb(engine);
 }
 
-static bool stop_ring(struct intel_engine_cs *engine)
-{
-	intel_engine_stop_cs(engine);
-
-	ENGINE_WRITE(engine, RING_HEAD, ENGINE_READ(engine, RING_TAIL));
-
-	ENGINE_WRITE(engine, RING_HEAD, 0);
-	ENGINE_WRITE(engine, RING_TAIL, 0);
-
-	/* The ring must be empty before it is disabled */
-	ENGINE_WRITE(engine, RING_CTL, 0);
-
-	return (ENGINE_READ(engine, RING_HEAD) & HEAD_ADDR) == 0;
-}
-
 static struct i915_address_space *vm_alias(struct i915_address_space *vm)
 {
 	if (i915_is_ggtt(vm))
@@ -213,31 +198,6 @@ static int xcs_resume(struct intel_engine_cs *engine)
 
 	intel_uncore_forcewake_get(engine->uncore, FORCEWAKE_ALL);
 
-	/* WaClearRingBufHeadRegAtInit:ctg,elk */
-	if (!stop_ring(engine)) {
-		/* G45 ring initialization often fails to reset head to zero */
-		drm_dbg(&dev_priv->drm, "%s head not reset to zero "
-			"ctl %08x head %08x tail %08x start %08x\n",
-			engine->name,
-			ENGINE_READ(engine, RING_CTL),
-			ENGINE_READ(engine, RING_HEAD),
-			ENGINE_READ(engine, RING_TAIL),
-			ENGINE_READ(engine, RING_START));
-
-		if (!stop_ring(engine)) {
-			drm_err(&dev_priv->drm,
-				"failed to set %s head to zero "
-				"ctl %08x head %08x tail %08x start %08x\n",
-				engine->name,
-				ENGINE_READ(engine, RING_CTL),
-				ENGINE_READ(engine, RING_HEAD),
-				ENGINE_READ(engine, RING_TAIL),
-				ENGINE_READ(engine, RING_START));
-			ret = -EIO;
-			goto out;
-		}
-	}
-
 	if (HWS_NEEDS_PHYSICAL(dev_priv))
 		ring_setup_phys_status_page(engine);
 	else
@@ -339,11 +299,21 @@ static void xcs_sanitize(struct intel_engine_cs *engine)
 	clflush_cache_range(engine->status_page.addr, PAGE_SIZE);
 }
 
+static bool stop_ring(struct intel_engine_cs *engine)
+{
+	ENGINE_WRITE_FW(engine, RING_HEAD, ENGINE_READ_FW(engine, RING_TAIL));
+
+	ENGINE_WRITE_FW(engine, RING_HEAD, 0);
+	ENGINE_WRITE_FW(engine, RING_TAIL, 0);
+
+	/* The ring must be empty before it is disabled */
+	ENGINE_WRITE_FW(engine, RING_CTL, 0);
+
+	return (ENGINE_READ_FW(engine, RING_HEAD) & HEAD_ADDR) == 0;
+}
+
 static void reset_prepare(struct intel_engine_cs *engine)
 {
-	struct intel_uncore *uncore = engine->uncore;
-	const u32 base = engine->mmio_base;
-
 	/*
 	 * We stop engines, otherwise we might get failed reset and a
 	 * dead gpu (on elk). Also as modern gpu as kbl can suffer
@@ -355,30 +325,35 @@ static void reset_prepare(struct intel_engine_cs *engine)
 	 * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
 	 *
 	 * WaMediaResetMainRingCleanup:ctg,elk (presumably)
+	 * WaClearRingBufHeadRegAtInit:ctg,elk
 	 *
 	 * FIXME: Wa for more modern gens needs to be validated
 	 */
 	ENGINE_TRACE(engine, "\n");
+	intel_engine_stop_cs(engine);
 
-	if (intel_engine_stop_cs(engine))
-		ENGINE_TRACE(engine, "timed out on STOP_RING\n");
+	if (!stop_ring(engine)) {
+		/* G45 ring initialization often fails to reset head to zero */
+		drm_dbg(&engine->i915->drm,
+			"%s head not reset to zero "
+			"ctl %08x head %08x tail %08x start %08x\n",
+			engine->name,
+			ENGINE_READ_FW(engine, RING_CTL),
+			ENGINE_READ_FW(engine, RING_HEAD),
+			ENGINE_READ_FW(engine, RING_TAIL),
+			ENGINE_READ_FW(engine, RING_START));
+	}
 
-	intel_uncore_write_fw(uncore,
-			      RING_HEAD(base),
-			      intel_uncore_read_fw(uncore, RING_TAIL(base)));
-	intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */
-
-	intel_uncore_write_fw(uncore, RING_HEAD(base), 0);
-	intel_uncore_write_fw(uncore, RING_TAIL(base), 0);
-	intel_uncore_posting_read_fw(uncore, RING_TAIL(base));
-
-	/* The ring must be empty before it is disabled */
-	intel_uncore_write_fw(uncore, RING_CTL(base), 0);
-
-	/* Check acts as a post */
-	if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
-		ENGINE_TRACE(engine, "ring head [%x] not parked\n",
-			     intel_uncore_read_fw(uncore, RING_HEAD(base)));
+	if (!stop_ring(engine)) {
+		drm_err(&engine->i915->drm,
+			"failed to set %s head to zero "
+			"ctl %08x head %08x tail %08x start %08x\n",
+			engine->name,
+			ENGINE_READ_FW(engine, RING_CTL),
+			ENGINE_READ_FW(engine, RING_HEAD),
+			ENGINE_READ_FW(engine, RING_TAIL),
+			ENGINE_READ_FW(engine, RING_START));
+	}
 }
 
 static void reset_rewind(struct intel_engine_cs *engine, bool stalled)
-- 
2.20.1

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

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

* [Intel-gfx] [PATCH 4/5] drm/i915/gt: Pull ring submission resume under its caller forcewake
  2021-01-19  9:40 [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Chris Wilson
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 2/5] drm/i915/selftests: Prepare the selftests for engine resets with ring submission Chris Wilson
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 3/5] drm/i915/gt: Lift stop_ring() to reset_prepare Chris Wilson
@ 2021-01-19  9:40 ` Chris Wilson
  2021-01-19 10:55   ` Mika Kuoppala
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 5/5] drm/i915: Mark per-engine-reset as supported on gen7 Chris Wilson
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2021-01-19  9:40 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson

Take advantage of calling xcs_resume under a forcewake by using direct
mmio access. In particular, we can avoid the sleeping variants to allow
resume to be called from softirq context, required for engine resets.

v2: Keep the positing read at the start of resume as a guardian memory
barrier.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 .../gpu/drm/i915/gt/intel_ring_submission.c   | 93 +++++++++----------
 1 file changed, 42 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
index 44159595d909..bf2834424add 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
@@ -122,31 +122,27 @@ static void set_hwsp(struct intel_engine_cs *engine, u32 offset)
 		hwsp = RING_HWS_PGA(engine->mmio_base);
 	}
 
-	intel_uncore_write(engine->uncore, hwsp, offset);
-	intel_uncore_posting_read(engine->uncore, hwsp);
+	intel_uncore_write_fw(engine->uncore, hwsp, offset);
+	intel_uncore_posting_read_fw(engine->uncore, hwsp);
 }
 
 static void flush_cs_tlb(struct intel_engine_cs *engine)
 {
-	struct drm_i915_private *dev_priv = engine->i915;
-
-	if (!IS_GEN_RANGE(dev_priv, 6, 7))
+	if (!IS_GEN_RANGE(engine->i915, 6, 7))
 		return;
 
 	/* ring should be idle before issuing a sync flush*/
-	drm_WARN_ON(&dev_priv->drm,
-		    (ENGINE_READ(engine, RING_MI_MODE) & MODE_IDLE) == 0);
+	GEM_DEBUG_WARN_ON((ENGINE_READ(engine, RING_MI_MODE) & MODE_IDLE) == 0);
 
-	ENGINE_WRITE(engine, RING_INSTPM,
-		     _MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE |
-					INSTPM_SYNC_FLUSH));
-	if (intel_wait_for_register(engine->uncore,
-				    RING_INSTPM(engine->mmio_base),
-				    INSTPM_SYNC_FLUSH, 0,
-				    1000))
-		drm_err(&dev_priv->drm,
-			"%s: wait for SyncFlush to complete for TLB invalidation timed out\n",
-			engine->name);
+	ENGINE_WRITE_FW(engine, RING_INSTPM,
+			_MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE |
+					   INSTPM_SYNC_FLUSH));
+	if (__intel_wait_for_register_fw(engine->uncore,
+					 RING_INSTPM(engine->mmio_base),
+					 INSTPM_SYNC_FLUSH, 0,
+					 2000, 0, NULL))
+		ENGINE_TRACE(engine,
+			     "wait for SyncFlush to complete for TLB invalidation timed out\n");
 }
 
 static void ring_setup_status_page(struct intel_engine_cs *engine)
@@ -177,13 +173,13 @@ static void set_pp_dir(struct intel_engine_cs *engine)
 	if (!vm)
 		return;
 
-	ENGINE_WRITE(engine, RING_PP_DIR_DCLV, PP_DIR_DCLV_2G);
-	ENGINE_WRITE(engine, RING_PP_DIR_BASE, pp_dir(vm));
+	ENGINE_WRITE_FW(engine, RING_PP_DIR_DCLV, PP_DIR_DCLV_2G);
+	ENGINE_WRITE_FW(engine, RING_PP_DIR_BASE, pp_dir(vm));
 
 	if (INTEL_GEN(engine->i915) >= 7) {
-		ENGINE_WRITE(engine,
-			     RING_MODE_GEN7,
-			     _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
+		ENGINE_WRITE_FW(engine,
+				RING_MODE_GEN7,
+				_MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
 	}
 }
 
@@ -191,13 +187,10 @@ static int xcs_resume(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
 	struct intel_ring *ring = engine->legacy.ring;
-	int ret = 0;
 
 	ENGINE_TRACE(engine, "ring:{HEAD:%04x, TAIL:%04x}\n",
 		     ring->head, ring->tail);
 
-	intel_uncore_forcewake_get(engine->uncore, FORCEWAKE_ALL);
-
 	if (HWS_NEEDS_PHYSICAL(dev_priv))
 		ring_setup_phys_status_page(engine);
 	else
@@ -214,7 +207,7 @@ static int xcs_resume(struct intel_engine_cs *engine)
 	 * also enforces ordering), otherwise the hw might lose the new ring
 	 * register values.
 	 */
-	ENGINE_WRITE(engine, RING_START, i915_ggtt_offset(ring->vma));
+	ENGINE_WRITE_FW(engine, RING_START, i915_ggtt_offset(ring->vma));
 
 	/* Check that the ring offsets point within the ring! */
 	GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
@@ -224,46 +217,44 @@ static int xcs_resume(struct intel_engine_cs *engine)
 	set_pp_dir(engine);
 
 	/* First wake the ring up to an empty/idle ring */
-	ENGINE_WRITE(engine, RING_HEAD, ring->head);
-	ENGINE_WRITE(engine, RING_TAIL, ring->head);
+	ENGINE_WRITE_FW(engine, RING_HEAD, ring->head);
+	ENGINE_WRITE_FW(engine, RING_TAIL, ring->head);
 	ENGINE_POSTING_READ(engine, RING_TAIL);
 
-	ENGINE_WRITE(engine, RING_CTL, RING_CTL_SIZE(ring->size) | RING_VALID);
+	ENGINE_WRITE_FW(engine, RING_CTL,
+			RING_CTL_SIZE(ring->size) | RING_VALID);
 
 	/* If the head is still not zero, the ring is dead */
-	if (intel_wait_for_register(engine->uncore,
-				    RING_CTL(engine->mmio_base),
-				    RING_VALID, RING_VALID,
-				    50)) {
-		drm_err(&dev_priv->drm, "%s initialization failed "
-			  "ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
-			  engine->name,
-			  ENGINE_READ(engine, RING_CTL),
-			  ENGINE_READ(engine, RING_CTL) & RING_VALID,
-			  ENGINE_READ(engine, RING_HEAD), ring->head,
-			  ENGINE_READ(engine, RING_TAIL), ring->tail,
-			  ENGINE_READ(engine, RING_START),
-			  i915_ggtt_offset(ring->vma));
-		ret = -EIO;
-		goto out;
+	if (__intel_wait_for_register_fw(engine->uncore,
+					 RING_CTL(engine->mmio_base),
+					 RING_VALID, RING_VALID,
+					 5000, 0, NULL)) {
+		drm_err(&dev_priv->drm,
+			"%s initialization failed; "
+			"ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
+			engine->name,
+			ENGINE_READ(engine, RING_CTL),
+			ENGINE_READ(engine, RING_CTL) & RING_VALID,
+			ENGINE_READ(engine, RING_HEAD), ring->head,
+			ENGINE_READ(engine, RING_TAIL), ring->tail,
+			ENGINE_READ(engine, RING_START),
+			i915_ggtt_offset(ring->vma));
+		return -EIO;
 	}
 
 	if (INTEL_GEN(dev_priv) > 2)
-		ENGINE_WRITE(engine,
-			     RING_MI_MODE, _MASKED_BIT_DISABLE(STOP_RING));
+		ENGINE_WRITE_FW(engine,
+				RING_MI_MODE, _MASKED_BIT_DISABLE(STOP_RING));
 
 	/* Now awake, let it get started */
 	if (ring->tail != ring->head) {
-		ENGINE_WRITE(engine, RING_TAIL, ring->tail);
+		ENGINE_WRITE_FW(engine, RING_TAIL, ring->tail);
 		ENGINE_POSTING_READ(engine, RING_TAIL);
 	}
 
 	/* Papering over lost _interrupts_ immediately following the restart */
 	intel_engine_signal_breadcrumbs(engine);
-out:
-	intel_uncore_forcewake_put(engine->uncore, FORCEWAKE_ALL);
-
-	return ret;
+	return 0;
 }
 
 static void sanitize_hwsp(struct intel_engine_cs *engine)
-- 
2.20.1

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

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

* [Intel-gfx] [PATCH 5/5] drm/i915: Mark per-engine-reset as supported on gen7
  2021-01-19  9:40 [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Chris Wilson
                   ` (2 preceding siblings ...)
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 4/5] drm/i915/gt: Pull ring submission resume under its caller forcewake Chris Wilson
@ 2021-01-19  9:40 ` Chris Wilson
  2021-01-19 11:00   ` Mika Kuoppala
  2021-01-19 10:21 ` [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Abodunrin, Akeem G
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2021-01-19  9:40 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson

The benefit of only resetting a single engine is that we leave other
streams of userspace work intact across a hang; vital for process
isolation. We had wired up individual engine resets for gen6, but only
enabled it from gen8; now let's turn it on for the forgotten gen7. gen6
is still a mystery as how to unravel some global state that appears to
be reset along with an engine (in particular the ppgtt enabling in
GFX_MODE).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_pci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 39608381b4a4..020b5f561f07 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -455,6 +455,7 @@ static const struct intel_device_info snb_m_gt2_info = {
 	.has_llc = 1, \
 	.has_rc6 = 1, \
 	.has_rc6p = 1, \
+	.has_reset_engine = true, \
 	.has_rps = true, \
 	.dma_mask_size = 40, \
 	.ppgtt_type = INTEL_PPGTT_ALIASING, \
@@ -513,6 +514,7 @@ static const struct intel_device_info vlv_info = {
 	.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B),
 	.has_runtime_pm = 1,
 	.has_rc6 = 1,
+	.has_reset_engine = true,
 	.has_rps = true,
 	.display.has_gmch = 1,
 	.display.has_hotplug = 1,
@@ -571,8 +573,7 @@ static const struct intel_device_info hsw_gt3_info = {
 	.dma_mask_size = 39, \
 	.ppgtt_type = INTEL_PPGTT_FULL, \
 	.ppgtt_size = 48, \
-	.has_64bit_reloc = 1, \
-	.has_reset_engine = 1
+	.has_64bit_reloc = 1
 
 #define BDW_PLATFORM \
 	GEN8_FEATURES, \
-- 
2.20.1

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

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

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals
  2021-01-19  9:40 [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Chris Wilson
                   ` (3 preceding siblings ...)
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 5/5] drm/i915: Mark per-engine-reset as supported on gen7 Chris Wilson
@ 2021-01-19 10:21 ` Abodunrin, Akeem G
  2021-01-19 10:25 ` Mika Kuoppala
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Abodunrin, Akeem G @ 2021-01-19 10:21 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx



> -----Original Message-----
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Sent: Tuesday, January 19, 2021 1:41 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: mika.kuoppala@linux.intel.com; Chris Wilson <chris@chris-wilson.co.uk>;
> Abodunrin, Akeem G <akeem.g.abodunrin@intel.com>
> Subject: [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals
> 
> CI reports that Baytail requires one more invalidate after CACHE_MODE for it
> to be happy.
> 
> Fixes: ace44e13e577 ("drm/i915/gt: Clear CACHE_MODE prior to clearing
> residuals")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/gen7_renderclear.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> index 39478712769f..8551e6de50e8 100644
> --- a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> +++ b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> @@ -353,19 +353,21 @@ static void gen7_emit_pipeline_flush(struct
> batch_chunk *batch)
> 
>  static void gen7_emit_pipeline_invalidate(struct batch_chunk *batch)  {
> -	u32 *cs = batch_alloc_items(batch, 0, 8);
> +	u32 *cs = batch_alloc_items(batch, 0, 10);
> 
>  	/* ivb: Stall before STATE_CACHE_INVALIDATE */
> -	*cs++ = GFX_OP_PIPE_CONTROL(4);
> +	*cs++ = GFX_OP_PIPE_CONTROL(5);
>  	*cs++ = PIPE_CONTROL_STALL_AT_SCOREBOARD |
>  		PIPE_CONTROL_CS_STALL;
>  	*cs++ = 0;
>  	*cs++ = 0;
> +	*cs++ = 0;
> 
> -	*cs++ = GFX_OP_PIPE_CONTROL(4);
> +	*cs++ = GFX_OP_PIPE_CONTROL(5);
>  	*cs++ = PIPE_CONTROL_STATE_CACHE_INVALIDATE;
>  	*cs++ = 0;
>  	*cs++ = 0;
> +	*cs++ = 0;
> 
>  	batch_advance(batch, cs);
>  }
> @@ -397,6 +399,7 @@ static void emit_batch(struct i915_vma * const vma,
>  	batch_add(&cmds, 0xffff0000);
>  	batch_add(&cmds, i915_mmio_reg_offset(CACHE_MODE_1));
>  	batch_add(&cmds, 0xffff0000 |
> PIXEL_SUBSPAN_COLLECT_OPT_DISABLE);
> +	gen7_emit_pipeline_invalidate(&cmds);
>  	gen7_emit_pipeline_flush(&cmds);
> 
>  	/* Switch to the media pipeline and our base address */
> --
> 2.20.1

Reviewed-by: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>

Thank you!
~Akeem

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

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

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals
  2021-01-19  9:40 [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Chris Wilson
                   ` (4 preceding siblings ...)
  2021-01-19 10:21 ` [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Abodunrin, Akeem G
@ 2021-01-19 10:25 ` Mika Kuoppala
  2021-01-19 10:34   ` Chris Wilson
  2021-01-19 13:08 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/5] " Patchwork
  2021-01-19 13:41 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
  7 siblings, 1 reply; 17+ messages in thread
From: Mika Kuoppala @ 2021-01-19 10:25 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: Chris Wilson

Chris Wilson <chris@chris-wilson.co.uk> writes:

> CI reports that Baytail requires one more invalidate after CACHE_MODE
> for it to be happy.
>
> Fixes: ace44e13e577 ("drm/i915/gt: Clear CACHE_MODE prior to clearing residuals")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/gen7_renderclear.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/gen7_renderclear.c b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> index 39478712769f..8551e6de50e8 100644
> --- a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> +++ b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> @@ -353,19 +353,21 @@ static void gen7_emit_pipeline_flush(struct batch_chunk *batch)
>  
>  static void gen7_emit_pipeline_invalidate(struct batch_chunk *batch)
>  {
> -	u32 *cs = batch_alloc_items(batch, 0, 8);
> +	u32 *cs = batch_alloc_items(batch, 0, 10);
>  
>  	/* ivb: Stall before STATE_CACHE_INVALIDATE */
> -	*cs++ = GFX_OP_PIPE_CONTROL(4);
> +	*cs++ = GFX_OP_PIPE_CONTROL(5);
>  	*cs++ = PIPE_CONTROL_STALL_AT_SCOREBOARD |
>  		PIPE_CONTROL_CS_STALL;
>  	*cs++ = 0;
>  	*cs++ = 0;
> +	*cs++ = 0;

dw[5] seems to be only for gen8+. Does it make a difference?

-Mika

>  
> -	*cs++ = GFX_OP_PIPE_CONTROL(4);
> +	*cs++ = GFX_OP_PIPE_CONTROL(5);
>  	*cs++ = PIPE_CONTROL_STATE_CACHE_INVALIDATE;
>  	*cs++ = 0;
>  	*cs++ = 0;
> +	*cs++ = 0;
>  
>  	batch_advance(batch, cs);
>  }
> @@ -397,6 +399,7 @@ static void emit_batch(struct i915_vma * const vma,
>  	batch_add(&cmds, 0xffff0000);
>  	batch_add(&cmds, i915_mmio_reg_offset(CACHE_MODE_1));
>  	batch_add(&cmds, 0xffff0000 | PIXEL_SUBSPAN_COLLECT_OPT_DISABLE);
> +	gen7_emit_pipeline_invalidate(&cmds);
>  	gen7_emit_pipeline_flush(&cmds);
>  
>  	/* Switch to the media pipeline and our base address */
> -- 
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH 2/5] drm/i915/selftests: Prepare the selftests for engine resets with ring submission
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 2/5] drm/i915/selftests: Prepare the selftests for engine resets with ring submission Chris Wilson
@ 2021-01-19 10:26   ` Mika Kuoppala
  0 siblings, 0 replies; 17+ messages in thread
From: Mika Kuoppala @ 2021-01-19 10:26 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: Chris Wilson

Chris Wilson <chris@chris-wilson.co.uk> writes:

> The engine resets selftests kick the tasklets, safe up until now as only
> execlists supported engine resets.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 18 ++++++++++++++----
>  drivers/gpu/drm/i915/gt/selftest_reset.c     | 11 ++++++++---
>  2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> index 460c3e9542f4..463bb6a700c8 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> @@ -704,6 +704,7 @@ static int __igt_reset_engine(struct intel_gt *gt, bool active)
>  
>  	for_each_engine(engine, gt, id) {
>  		unsigned int reset_count, reset_engine_count;
> +		unsigned long count;
>  		IGT_TIMEOUT(end_time);
>  
>  		if (active && !intel_engine_can_store_dword(engine))
> @@ -721,6 +722,7 @@ static int __igt_reset_engine(struct intel_gt *gt, bool active)
>  
>  		st_engine_heartbeat_disable(engine);
>  		set_bit(I915_RESET_ENGINE + id, &gt->reset.flags);
> +		count = 0;
>  		do {
>  			if (active) {
>  				struct i915_request *rq;
> @@ -770,9 +772,13 @@ static int __igt_reset_engine(struct intel_gt *gt, bool active)
>  				err = -EINVAL;
>  				break;
>  			}
> +
> +			count++;
>  		} while (time_before(jiffies, end_time));
>  		clear_bit(I915_RESET_ENGINE + id, &gt->reset.flags);
>  		st_engine_heartbeat_enable(engine);
> +		pr_info("%s: Completed %lu %s resets\n",
> +			engine->name, count, active ? "active" : "idle");
>  
>  		if (err)
>  			break;
> @@ -1623,7 +1629,8 @@ static int igt_reset_queue(void *arg)
>  			prev = rq;
>  			count++;
>  		} while (time_before(jiffies, end_time));
> -		pr_info("%s: Completed %d resets\n", engine->name, count);
> +		pr_info("%s: Completed %d queued resets\n",
> +			engine->name, count);
>  
>  		*h.batch = MI_BATCH_BUFFER_END;
>  		intel_gt_chipset_flush(engine->gt);
> @@ -1720,7 +1727,8 @@ static int __igt_atomic_reset_engine(struct intel_engine_cs *engine,
>  	GEM_TRACE("i915_reset_engine(%s:%s) under %s\n",
>  		  engine->name, mode, p->name);
>  
> -	tasklet_disable(t);
> +	if (t->func)
> +		tasklet_disable(t);
>  	if (strcmp(p->name, "softirq"))
>  		local_bh_disable();
>  	p->critical_section_begin();
> @@ -1730,8 +1738,10 @@ static int __igt_atomic_reset_engine(struct intel_engine_cs *engine,
>  	p->critical_section_end();
>  	if (strcmp(p->name, "softirq"))
>  		local_bh_enable();
> -	tasklet_enable(t);
> -	tasklet_hi_schedule(t);
> +	if (t->func) {
> +		tasklet_enable(t);
> +		tasklet_hi_schedule(t);
> +	}
>  
>  	if (err)
>  		pr_err("i915_reset_engine(%s:%s) failed under %s\n",
> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
> index b7befcfbdcde..8784257ec808 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
> @@ -321,7 +321,10 @@ static int igt_atomic_engine_reset(void *arg)
>  		goto out_unlock;
>  
>  	for_each_engine(engine, gt, id) {
> -		tasklet_disable(&engine->execlists.tasklet);
> +		struct tasklet_struct *t = &engine->execlists.tasklet;
> +
> +		if (t->func)
> +			tasklet_disable(t);
>  		intel_engine_pm_get(engine);
>  
>  		for (p = igt_atomic_phases; p->name; p++) {
> @@ -345,8 +348,10 @@ static int igt_atomic_engine_reset(void *arg)
>  		}
>  
>  		intel_engine_pm_put(engine);
> -		tasklet_enable(&engine->execlists.tasklet);
> -		tasklet_hi_schedule(&engine->execlists.tasklet);
> +		if (t->func) {
> +			tasklet_enable(t);
> +			tasklet_hi_schedule(t);
> +		}
>  		if (err)
>  			break;
>  	}
> -- 
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH 3/5] drm/i915/gt: Lift stop_ring() to reset_prepare
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 3/5] drm/i915/gt: Lift stop_ring() to reset_prepare Chris Wilson
@ 2021-01-19 10:33   ` Mika Kuoppala
  2021-01-19 10:39     ` Chris Wilson
  0 siblings, 1 reply; 17+ messages in thread
From: Mika Kuoppala @ 2021-01-19 10:33 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: Chris Wilson

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Push the sleeping stop_ring() out of the reset resume function to reset
> prepare; we are not allowed to sleep in the former.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  .../gpu/drm/i915/gt/intel_ring_submission.c   | 97 +++++++------------
>  1 file changed, 36 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> index 8d0964d2d597..44159595d909 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> @@ -157,21 +157,6 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
>  	flush_cs_tlb(engine);
>  }
>  
> -static bool stop_ring(struct intel_engine_cs *engine)
> -{
> -	intel_engine_stop_cs(engine);
> -
> -	ENGINE_WRITE(engine, RING_HEAD, ENGINE_READ(engine, RING_TAIL));
> -
> -	ENGINE_WRITE(engine, RING_HEAD, 0);
> -	ENGINE_WRITE(engine, RING_TAIL, 0);
> -
> -	/* The ring must be empty before it is disabled */
> -	ENGINE_WRITE(engine, RING_CTL, 0);
> -
> -	return (ENGINE_READ(engine, RING_HEAD) & HEAD_ADDR) == 0;
> -}
> -
>  static struct i915_address_space *vm_alias(struct i915_address_space *vm)
>  {
>  	if (i915_is_ggtt(vm))
> @@ -213,31 +198,6 @@ static int xcs_resume(struct intel_engine_cs *engine)
>  
>  	intel_uncore_forcewake_get(engine->uncore, FORCEWAKE_ALL);
>  
> -	/* WaClearRingBufHeadRegAtInit:ctg,elk */
> -	if (!stop_ring(engine)) {
> -		/* G45 ring initialization often fails to reset head to zero */
> -		drm_dbg(&dev_priv->drm, "%s head not reset to zero "
> -			"ctl %08x head %08x tail %08x start %08x\n",
> -			engine->name,
> -			ENGINE_READ(engine, RING_CTL),
> -			ENGINE_READ(engine, RING_HEAD),
> -			ENGINE_READ(engine, RING_TAIL),
> -			ENGINE_READ(engine, RING_START));
> -
> -		if (!stop_ring(engine)) {
> -			drm_err(&dev_priv->drm,
> -				"failed to set %s head to zero "
> -				"ctl %08x head %08x tail %08x start %08x\n",
> -				engine->name,
> -				ENGINE_READ(engine, RING_CTL),
> -				ENGINE_READ(engine, RING_HEAD),
> -				ENGINE_READ(engine, RING_TAIL),
> -				ENGINE_READ(engine, RING_START));
> -			ret = -EIO;
> -			goto out;
> -		}
> -	}
> -
>  	if (HWS_NEEDS_PHYSICAL(dev_priv))
>  		ring_setup_phys_status_page(engine);
>  	else
> @@ -339,11 +299,21 @@ static void xcs_sanitize(struct intel_engine_cs *engine)
>  	clflush_cache_range(engine->status_page.addr, PAGE_SIZE);
>  }
>  
> +static bool stop_ring(struct intel_engine_cs *engine)
> +{
> +	ENGINE_WRITE_FW(engine, RING_HEAD, ENGINE_READ_FW(engine, RING_TAIL));
> +

Not related to this patch but this makes me wondering if the actual
disable should be at this point before zeroing as manipulating the
head again might kick the hardware forward.

As in this point the 'ring must be empty' is satisfied.

For this patch,

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> +	ENGINE_WRITE_FW(engine, RING_HEAD, 0);
> +	ENGINE_WRITE_FW(engine, RING_TAIL, 0);
> +
> +	/* The ring must be empty before it is disabled */
> +	ENGINE_WRITE_FW(engine, RING_CTL, 0);
> +
> +	return (ENGINE_READ_FW(engine, RING_HEAD) & HEAD_ADDR) == 0;
> +}
> +
>  static void reset_prepare(struct intel_engine_cs *engine)
>  {
> -	struct intel_uncore *uncore = engine->uncore;
> -	const u32 base = engine->mmio_base;
> -
>  	/*
>  	 * We stop engines, otherwise we might get failed reset and a
>  	 * dead gpu (on elk). Also as modern gpu as kbl can suffer
> @@ -355,30 +325,35 @@ static void reset_prepare(struct intel_engine_cs *engine)
>  	 * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES)
>  	 *
>  	 * WaMediaResetMainRingCleanup:ctg,elk (presumably)
> +	 * WaClearRingBufHeadRegAtInit:ctg,elk
>  	 *
>  	 * FIXME: Wa for more modern gens needs to be validated
>  	 */
>  	ENGINE_TRACE(engine, "\n");
> +	intel_engine_stop_cs(engine);
>  
> -	if (intel_engine_stop_cs(engine))
> -		ENGINE_TRACE(engine, "timed out on STOP_RING\n");
> +	if (!stop_ring(engine)) {
> +		/* G45 ring initialization often fails to reset head to zero */
> +		drm_dbg(&engine->i915->drm,
> +			"%s head not reset to zero "
> +			"ctl %08x head %08x tail %08x start %08x\n",
> +			engine->name,
> +			ENGINE_READ_FW(engine, RING_CTL),
> +			ENGINE_READ_FW(engine, RING_HEAD),
> +			ENGINE_READ_FW(engine, RING_TAIL),
> +			ENGINE_READ_FW(engine, RING_START));
> +	}
>  
> -	intel_uncore_write_fw(uncore,
> -			      RING_HEAD(base),
> -			      intel_uncore_read_fw(uncore, RING_TAIL(base)));
> -	intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */
> -
> -	intel_uncore_write_fw(uncore, RING_HEAD(base), 0);
> -	intel_uncore_write_fw(uncore, RING_TAIL(base), 0);
> -	intel_uncore_posting_read_fw(uncore, RING_TAIL(base));
> -
> -	/* The ring must be empty before it is disabled */
> -	intel_uncore_write_fw(uncore, RING_CTL(base), 0);
> -
> -	/* Check acts as a post */
> -	if (intel_uncore_read_fw(uncore, RING_HEAD(base)))
> -		ENGINE_TRACE(engine, "ring head [%x] not parked\n",
> -			     intel_uncore_read_fw(uncore, RING_HEAD(base)));
> +	if (!stop_ring(engine)) {
> +		drm_err(&engine->i915->drm,
> +			"failed to set %s head to zero "
> +			"ctl %08x head %08x tail %08x start %08x\n",
> +			engine->name,
> +			ENGINE_READ_FW(engine, RING_CTL),
> +			ENGINE_READ_FW(engine, RING_HEAD),
> +			ENGINE_READ_FW(engine, RING_TAIL),
> +			ENGINE_READ_FW(engine, RING_START));
> +	}
>  }
>  
>  static void reset_rewind(struct intel_engine_cs *engine, bool stalled)
> -- 
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals
  2021-01-19 10:25 ` Mika Kuoppala
@ 2021-01-19 10:34   ` Chris Wilson
  2021-01-19 10:59     ` Mika Kuoppala
  0 siblings, 1 reply; 17+ messages in thread
From: Chris Wilson @ 2021-01-19 10:34 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2021-01-19 10:25:14)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > CI reports that Baytail requires one more invalidate after CACHE_MODE
> > for it to be happy.
> >
> > Fixes: ace44e13e577 ("drm/i915/gt: Clear CACHE_MODE prior to clearing residuals")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/gen7_renderclear.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/gen7_renderclear.c b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> > index 39478712769f..8551e6de50e8 100644
> > --- a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> > +++ b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> > @@ -353,19 +353,21 @@ static void gen7_emit_pipeline_flush(struct batch_chunk *batch)
> >  
> >  static void gen7_emit_pipeline_invalidate(struct batch_chunk *batch)
> >  {
> > -     u32 *cs = batch_alloc_items(batch, 0, 8);
> > +     u32 *cs = batch_alloc_items(batch, 0, 10);
> >  
> >       /* ivb: Stall before STATE_CACHE_INVALIDATE */
> > -     *cs++ = GFX_OP_PIPE_CONTROL(4);
> > +     *cs++ = GFX_OP_PIPE_CONTROL(5);
> >       *cs++ = PIPE_CONTROL_STALL_AT_SCOREBOARD |
> >               PIPE_CONTROL_CS_STALL;
> >       *cs++ = 0;
> >       *cs++ = 0;
> > +     *cs++ = 0;
> 
> dw[5] seems to be only for gen8+. Does it make a difference?

Pipe-control has always supported a qword-write, so a packet-length of 4
or 5 on gen4-7. As I recall the debate for gen8+ was whether
pipe-control still supported only a dword-write, and so we went with
always using the qword-length.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH 3/5] drm/i915/gt: Lift stop_ring() to reset_prepare
  2021-01-19 10:33   ` Mika Kuoppala
@ 2021-01-19 10:39     ` Chris Wilson
  0 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2021-01-19 10:39 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2021-01-19 10:33:17)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Push the sleeping stop_ring() out of the reset resume function to reset
> > prepare; we are not allowed to sleep in the former.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  .../gpu/drm/i915/gt/intel_ring_submission.c   | 97 +++++++------------
> >  1 file changed, 36 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > index 8d0964d2d597..44159595d909 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > @@ -157,21 +157,6 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
> >       flush_cs_tlb(engine);
> >  }
> >  
> > -static bool stop_ring(struct intel_engine_cs *engine)
> > -{
> > -     intel_engine_stop_cs(engine);
> > -
> > -     ENGINE_WRITE(engine, RING_HEAD, ENGINE_READ(engine, RING_TAIL));
> > -
> > -     ENGINE_WRITE(engine, RING_HEAD, 0);
> > -     ENGINE_WRITE(engine, RING_TAIL, 0);
> > -
> > -     /* The ring must be empty before it is disabled */
> > -     ENGINE_WRITE(engine, RING_CTL, 0);
> > -
> > -     return (ENGINE_READ(engine, RING_HEAD) & HEAD_ADDR) == 0;
> > -}
> > -
> >  static struct i915_address_space *vm_alias(struct i915_address_space *vm)
> >  {
> >       if (i915_is_ggtt(vm))
> > @@ -213,31 +198,6 @@ static int xcs_resume(struct intel_engine_cs *engine)
> >  
> >       intel_uncore_forcewake_get(engine->uncore, FORCEWAKE_ALL);
> >  
> > -     /* WaClearRingBufHeadRegAtInit:ctg,elk */
> > -     if (!stop_ring(engine)) {
> > -             /* G45 ring initialization often fails to reset head to zero */
> > -             drm_dbg(&dev_priv->drm, "%s head not reset to zero "
> > -                     "ctl %08x head %08x tail %08x start %08x\n",
> > -                     engine->name,
> > -                     ENGINE_READ(engine, RING_CTL),
> > -                     ENGINE_READ(engine, RING_HEAD),
> > -                     ENGINE_READ(engine, RING_TAIL),
> > -                     ENGINE_READ(engine, RING_START));
> > -
> > -             if (!stop_ring(engine)) {
> > -                     drm_err(&dev_priv->drm,
> > -                             "failed to set %s head to zero "
> > -                             "ctl %08x head %08x tail %08x start %08x\n",
> > -                             engine->name,
> > -                             ENGINE_READ(engine, RING_CTL),
> > -                             ENGINE_READ(engine, RING_HEAD),
> > -                             ENGINE_READ(engine, RING_TAIL),
> > -                             ENGINE_READ(engine, RING_START));
> > -                     ret = -EIO;
> > -                     goto out;
> > -             }
> > -     }
> > -
> >       if (HWS_NEEDS_PHYSICAL(dev_priv))
> >               ring_setup_phys_status_page(engine);
> >       else
> > @@ -339,11 +299,21 @@ static void xcs_sanitize(struct intel_engine_cs *engine)
> >       clflush_cache_range(engine->status_page.addr, PAGE_SIZE);
> >  }
> >  
> > +static bool stop_ring(struct intel_engine_cs *engine)
> > +{
> > +     ENGINE_WRITE_FW(engine, RING_HEAD, ENGINE_READ_FW(engine, RING_TAIL));
> > +
> 
> Not related to this patch but this makes me wondering if the actual
> disable should be at this point before zeroing as manipulating the
> head again might kick the hardware forward.

We move HEAD to TAIL, with the intent of kicking the HW to the end and
"emptying the ring".

If we move HEAD to 0 first, it will start executing random stuff. And by
the time we set TAIL to 0, HEAD will have moved on.

So I think this is correct: set HEAD to TAIL to empty the ring.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH 4/5] drm/i915/gt: Pull ring submission resume under its caller forcewake
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 4/5] drm/i915/gt: Pull ring submission resume under its caller forcewake Chris Wilson
@ 2021-01-19 10:55   ` Mika Kuoppala
  0 siblings, 0 replies; 17+ messages in thread
From: Mika Kuoppala @ 2021-01-19 10:55 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: Chris Wilson

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Take advantage of calling xcs_resume under a forcewake by using direct
> mmio access. In particular, we can avoid the sleeping variants to allow
> resume to be called from softirq context, required for engine resets.
>
> v2: Keep the positing read at the start of resume as a guardian memory
> barrier.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  .../gpu/drm/i915/gt/intel_ring_submission.c   | 93 +++++++++----------
>  1 file changed, 42 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> index 44159595d909..bf2834424add 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> @@ -122,31 +122,27 @@ static void set_hwsp(struct intel_engine_cs *engine, u32 offset)
>  		hwsp = RING_HWS_PGA(engine->mmio_base);
>  	}
>  
> -	intel_uncore_write(engine->uncore, hwsp, offset);
> -	intel_uncore_posting_read(engine->uncore, hwsp);
> +	intel_uncore_write_fw(engine->uncore, hwsp, offset);
> +	intel_uncore_posting_read_fw(engine->uncore, hwsp);
>  }
>  
>  static void flush_cs_tlb(struct intel_engine_cs *engine)
>  {
> -	struct drm_i915_private *dev_priv = engine->i915;
> -
> -	if (!IS_GEN_RANGE(dev_priv, 6, 7))
> +	if (!IS_GEN_RANGE(engine->i915, 6, 7))
>  		return;
>  
>  	/* ring should be idle before issuing a sync flush*/
> -	drm_WARN_ON(&dev_priv->drm,
> -		    (ENGINE_READ(engine, RING_MI_MODE) & MODE_IDLE) == 0);
> +	GEM_DEBUG_WARN_ON((ENGINE_READ(engine, RING_MI_MODE) & MODE_IDLE) == 0);
>  
> -	ENGINE_WRITE(engine, RING_INSTPM,
> -		     _MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE |
> -					INSTPM_SYNC_FLUSH));
> -	if (intel_wait_for_register(engine->uncore,
> -				    RING_INSTPM(engine->mmio_base),
> -				    INSTPM_SYNC_FLUSH, 0,
> -				    1000))
> -		drm_err(&dev_priv->drm,
> -			"%s: wait for SyncFlush to complete for TLB invalidation timed out\n",
> -			engine->name);
> +	ENGINE_WRITE_FW(engine, RING_INSTPM,
> +			_MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE |
> +					   INSTPM_SYNC_FLUSH));
> +	if (__intel_wait_for_register_fw(engine->uncore,
> +					 RING_INSTPM(engine->mmio_base),
> +					 INSTPM_SYNC_FLUSH, 0,
> +					 2000, 0, NULL))
> +		ENGINE_TRACE(engine,
> +			     "wait for SyncFlush to complete for TLB invalidation timed out\n");
>  }
>  
>  static void ring_setup_status_page(struct intel_engine_cs *engine)
> @@ -177,13 +173,13 @@ static void set_pp_dir(struct intel_engine_cs *engine)
>  	if (!vm)
>  		return;
>  
> -	ENGINE_WRITE(engine, RING_PP_DIR_DCLV, PP_DIR_DCLV_2G);
> -	ENGINE_WRITE(engine, RING_PP_DIR_BASE, pp_dir(vm));
> +	ENGINE_WRITE_FW(engine, RING_PP_DIR_DCLV, PP_DIR_DCLV_2G);
> +	ENGINE_WRITE_FW(engine, RING_PP_DIR_BASE, pp_dir(vm));
>  
>  	if (INTEL_GEN(engine->i915) >= 7) {
> -		ENGINE_WRITE(engine,
> -			     RING_MODE_GEN7,
> -			     _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
> +		ENGINE_WRITE_FW(engine,
> +				RING_MODE_GEN7,
> +				_MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
>  	}
>  }
>  
> @@ -191,13 +187,10 @@ static int xcs_resume(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
>  	struct intel_ring *ring = engine->legacy.ring;
> -	int ret = 0;
>  
>  	ENGINE_TRACE(engine, "ring:{HEAD:%04x, TAIL:%04x}\n",
>  		     ring->head, ring->tail);
>  
> -	intel_uncore_forcewake_get(engine->uncore, FORCEWAKE_ALL);
> -
>  	if (HWS_NEEDS_PHYSICAL(dev_priv))
>  		ring_setup_phys_status_page(engine);
>  	else
> @@ -214,7 +207,7 @@ static int xcs_resume(struct intel_engine_cs *engine)
>  	 * also enforces ordering), otherwise the hw might lose the new ring
>  	 * register values.
>  	 */
> -	ENGINE_WRITE(engine, RING_START, i915_ggtt_offset(ring->vma));
> +	ENGINE_WRITE_FW(engine, RING_START, i915_ggtt_offset(ring->vma));
>  
>  	/* Check that the ring offsets point within the ring! */
>  	GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
> @@ -224,46 +217,44 @@ static int xcs_resume(struct intel_engine_cs *engine)
>  	set_pp_dir(engine);
>  
>  	/* First wake the ring up to an empty/idle ring */
> -	ENGINE_WRITE(engine, RING_HEAD, ring->head);
> -	ENGINE_WRITE(engine, RING_TAIL, ring->head);
> +	ENGINE_WRITE_FW(engine, RING_HEAD, ring->head);
> +	ENGINE_WRITE_FW(engine, RING_TAIL, ring->head);
>  	ENGINE_POSTING_READ(engine, RING_TAIL);
>  
> -	ENGINE_WRITE(engine, RING_CTL, RING_CTL_SIZE(ring->size) | RING_VALID);
> +	ENGINE_WRITE_FW(engine, RING_CTL,
> +			RING_CTL_SIZE(ring->size) | RING_VALID);
>  
>  	/* If the head is still not zero, the ring is dead */
> -	if (intel_wait_for_register(engine->uncore,
> -				    RING_CTL(engine->mmio_base),
> -				    RING_VALID, RING_VALID,
> -				    50)) {
> -		drm_err(&dev_priv->drm, "%s initialization failed "
> -			  "ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
> -			  engine->name,
> -			  ENGINE_READ(engine, RING_CTL),
> -			  ENGINE_READ(engine, RING_CTL) & RING_VALID,
> -			  ENGINE_READ(engine, RING_HEAD), ring->head,
> -			  ENGINE_READ(engine, RING_TAIL), ring->tail,
> -			  ENGINE_READ(engine, RING_START),
> -			  i915_ggtt_offset(ring->vma));
> -		ret = -EIO;
> -		goto out;
> +	if (__intel_wait_for_register_fw(engine->uncore,
> +					 RING_CTL(engine->mmio_base),
> +					 RING_VALID, RING_VALID,
> +					 5000, 0, NULL)) {
> +		drm_err(&dev_priv->drm,
> +			"%s initialization failed; "
> +			"ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
> +			engine->name,
> +			ENGINE_READ(engine, RING_CTL),
> +			ENGINE_READ(engine, RING_CTL) & RING_VALID,
> +			ENGINE_READ(engine, RING_HEAD), ring->head,
> +			ENGINE_READ(engine, RING_TAIL), ring->tail,
> +			ENGINE_READ(engine, RING_START),
> +			i915_ggtt_offset(ring->vma));
> +		return -EIO;
>  	}
>  
>  	if (INTEL_GEN(dev_priv) > 2)
> -		ENGINE_WRITE(engine,
> -			     RING_MI_MODE, _MASKED_BIT_DISABLE(STOP_RING));
> +		ENGINE_WRITE_FW(engine,
> +				RING_MI_MODE, _MASKED_BIT_DISABLE(STOP_RING));
>  
>  	/* Now awake, let it get started */
>  	if (ring->tail != ring->head) {
> -		ENGINE_WRITE(engine, RING_TAIL, ring->tail);
> +		ENGINE_WRITE_FW(engine, RING_TAIL, ring->tail);
>  		ENGINE_POSTING_READ(engine, RING_TAIL);
>  	}
>  
>  	/* Papering over lost _interrupts_ immediately following the restart */
>  	intel_engine_signal_breadcrumbs(engine);
> -out:
> -	intel_uncore_forcewake_put(engine->uncore, FORCEWAKE_ALL);
> -
> -	return ret;
> +	return 0;
>  }
>  
>  static void sanitize_hwsp(struct intel_engine_cs *engine)
> -- 
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals
  2021-01-19 10:34   ` Chris Wilson
@ 2021-01-19 10:59     ` Mika Kuoppala
  0 siblings, 0 replies; 17+ messages in thread
From: Mika Kuoppala @ 2021-01-19 10:59 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2021-01-19 10:25:14)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > CI reports that Baytail requires one more invalidate after CACHE_MODE
>> > for it to be happy.
>> >
>> > Fixes: ace44e13e577 ("drm/i915/gt: Clear CACHE_MODE prior to clearing residuals")
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > Cc: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/gt/gen7_renderclear.c | 9 ++++++---
>> >  1 file changed, 6 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/gen7_renderclear.c b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
>> > index 39478712769f..8551e6de50e8 100644
>> > --- a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
>> > +++ b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
>> > @@ -353,19 +353,21 @@ static void gen7_emit_pipeline_flush(struct batch_chunk *batch)
>> >  
>> >  static void gen7_emit_pipeline_invalidate(struct batch_chunk *batch)
>> >  {
>> > -     u32 *cs = batch_alloc_items(batch, 0, 8);
>> > +     u32 *cs = batch_alloc_items(batch, 0, 10);
>> >  
>> >       /* ivb: Stall before STATE_CACHE_INVALIDATE */
>> > -     *cs++ = GFX_OP_PIPE_CONTROL(4);
>> > +     *cs++ = GFX_OP_PIPE_CONTROL(5);
>> >       *cs++ = PIPE_CONTROL_STALL_AT_SCOREBOARD |
>> >               PIPE_CONTROL_CS_STALL;
>> >       *cs++ = 0;
>> >       *cs++ = 0;
>> > +     *cs++ = 0;
>> 
>> dw[5] seems to be only for gen8+. Does it make a difference?
>
> Pipe-control has always supported a qword-write, so a packet-length of 4
> or 5 on gen4-7. As I recall the debate for gen8+ was whether
> pipe-control still supported only a dword-write, and so we went with
> always using the qword-length.

Yes, there seem to ample evidence that 5 is fine.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

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

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

* Re: [Intel-gfx] [PATCH 5/5] drm/i915: Mark per-engine-reset as supported on gen7
  2021-01-19  9:40 ` [Intel-gfx] [PATCH 5/5] drm/i915: Mark per-engine-reset as supported on gen7 Chris Wilson
@ 2021-01-19 11:00   ` Mika Kuoppala
  2021-01-19 11:06     ` Chris Wilson
  0 siblings, 1 reply; 17+ messages in thread
From: Mika Kuoppala @ 2021-01-19 11:00 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: Chris Wilson

Chris Wilson <chris@chris-wilson.co.uk> writes:

> The benefit of only resetting a single engine is that we leave other
> streams of userspace work intact across a hang; vital for process
> isolation. We had wired up individual engine resets for gen6, but only
> enabled it from gen8; now let's turn it on for the forgotten gen7. gen6
> is still a mystery as how to unravel some global state that appears to
> be reset along with an engine (in particular the ppgtt enabling in
> GFX_MODE).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_pci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 39608381b4a4..020b5f561f07 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -455,6 +455,7 @@ static const struct intel_device_info snb_m_gt2_info = {
>  	.has_llc = 1, \
>  	.has_rc6 = 1, \
>  	.has_rc6p = 1, \
> +	.has_reset_engine = true, \
>  	.has_rps = true, \
>  	.dma_mask_size = 40, \
>  	.ppgtt_type = INTEL_PPGTT_ALIASING, \
> @@ -513,6 +514,7 @@ static const struct intel_device_info vlv_info = {
>  	.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B),
>  	.has_runtime_pm = 1,
>  	.has_rc6 = 1,
> +	.has_reset_engine = true,
>  	.has_rps = true,
>  	.display.has_gmch = 1,
>  	.display.has_hotplug = 1,
> @@ -571,8 +573,7 @@ static const struct intel_device_info hsw_gt3_info = {
>  	.dma_mask_size = 39, \
>  	.ppgtt_type = INTEL_PPGTT_FULL, \
>  	.ppgtt_size = 48, \
> -	.has_64bit_reloc = 1, \
> -	.has_reset_engine = 1

Oh we already did on hsw.

Acked-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> +	.has_64bit_reloc = 1
>  
>  #define BDW_PLATFORM \
>  	GEN8_FEATURES, \
> -- 
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH 5/5] drm/i915: Mark per-engine-reset as supported on gen7
  2021-01-19 11:00   ` Mika Kuoppala
@ 2021-01-19 11:06     ` Chris Wilson
  0 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2021-01-19 11:06 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2021-01-19 11:00:19)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > The benefit of only resetting a single engine is that we leave other
> > streams of userspace work intact across a hang; vital for process
> > isolation. We had wired up individual engine resets for gen6, but only
> > enabled it from gen8; now let's turn it on for the forgotten gen7. gen6
> > is still a mystery as how to unravel some global state that appears to
> > be reset along with an engine (in particular the ppgtt enabling in
> > GFX_MODE).
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_pci.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > index 39608381b4a4..020b5f561f07 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -455,6 +455,7 @@ static const struct intel_device_info snb_m_gt2_info = {
> >       .has_llc = 1, \
> >       .has_rc6 = 1, \
> >       .has_rc6p = 1, \
> > +     .has_reset_engine = true, \
> >       .has_rps = true, \
> >       .dma_mask_size = 40, \
> >       .ppgtt_type = INTEL_PPGTT_ALIASING, \
> > @@ -513,6 +514,7 @@ static const struct intel_device_info vlv_info = {
> >       .cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B),
> >       .has_runtime_pm = 1,
> >       .has_rc6 = 1,
> > +     .has_reset_engine = true,
> >       .has_rps = true,
> >       .display.has_gmch = 1,
> >       .display.has_hotplug = 1,
> > @@ -571,8 +573,7 @@ static const struct intel_device_info hsw_gt3_info = {
> >       .dma_mask_size = 39, \
> >       .ppgtt_type = INTEL_PPGTT_FULL, \
> >       .ppgtt_size = 48, \
> > -     .has_64bit_reloc = 1, \
> > -     .has_reset_engine = 1
> 
> Oh we already did on hsw.

Don't be fooled by diff, it gets very confused by i915_pci.c

This is actually GEN8_FEATURES.
 
> Acked-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

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

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

* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/5] drm/i915/gt: One more flush for Baytrail clear residuals
  2021-01-19  9:40 [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Chris Wilson
                   ` (5 preceding siblings ...)
  2021-01-19 10:25 ` Mika Kuoppala
@ 2021-01-19 13:08 ` Patchwork
  2021-01-19 13:41 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
  7 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2021-01-19 13:08 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/5] drm/i915/gt: One more flush for Baytrail clear residuals
URL   : https://patchwork.freedesktop.org/series/86034/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.
-
+drivers/gpu/drm/i915/gt/intel_reset.c:1328:5: warning: context imbalance in 'intel_gt_reset_trylock' - different lock contexts for basic block


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

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

* [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/5] drm/i915/gt: One more flush for Baytrail clear residuals
  2021-01-19  9:40 [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Chris Wilson
                   ` (6 preceding siblings ...)
  2021-01-19 13:08 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/5] " Patchwork
@ 2021-01-19 13:41 ` Patchwork
  7 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2021-01-19 13:41 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 6139 bytes --]

== Series Details ==

Series: series starting with [1/5] drm/i915/gt: One more flush for Baytrail clear residuals
URL   : https://patchwork.freedesktop.org/series/86034/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_9640 -> Patchwork_19404
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_19404 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_19404, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_19404:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_suspend@basic-s0:
    - fi-glk-dsi:         [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9640/fi-glk-dsi/igt@gem_exec_suspend@basic-s0.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-glk-dsi/igt@gem_exec_suspend@basic-s0.html

  * igt@i915_selftest@live@hangcheck:
    - fi-byt-j1900:       NOTRUN -> [DMESG-WARN][3]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-byt-j1900/igt@i915_selftest@live@hangcheck.html

  * igt@runner@aborted:
    - fi-tgl-u2:          NOTRUN -> [FAIL][4]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-tgl-u2/igt@runner@aborted.html

  
Known issues
------------

  Here are the changes found in Patchwork_19404 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@query-info:
    - fi-tgl-y:           NOTRUN -> [SKIP][5] ([fdo#109315] / [i915#2575])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-tgl-y/igt@amdgpu/amd_basic@query-info.html

  * igt@amdgpu/amd_basic@semaphore:
    - fi-bdw-5557u:       NOTRUN -> [SKIP][6] ([fdo#109271]) +22 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-bdw-5557u/igt@amdgpu/amd_basic@semaphore.html

  * igt@core_hotunplug@unbind-rebind:
    - fi-bdw-5557u:       NOTRUN -> [WARN][7] ([i915#2283])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-bdw-5557u/igt@core_hotunplug@unbind-rebind.html

  * igt@fbdev@read:
    - fi-tgl-y:           [PASS][8] -> [DMESG-WARN][9] ([i915#402]) +2 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9640/fi-tgl-y/igt@fbdev@read.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-tgl-y/igt@fbdev@read.html

  * igt@gem_huc_copy@huc-copy:
    - fi-byt-j1900:       NOTRUN -> [SKIP][10] ([fdo#109271]) +27 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-byt-j1900/igt@gem_huc_copy@huc-copy.html

  * igt@i915_selftest@live@execlists:
    - fi-bsw-n3050:       [PASS][11] -> [INCOMPLETE][12] ([i915#2940])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9640/fi-bsw-n3050/igt@i915_selftest@live@execlists.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-bsw-n3050/igt@i915_selftest@live@execlists.html

  * igt@i915_selftest@live@hangcheck:
    - fi-tgl-u2:          [PASS][13] -> [INCOMPLETE][14] ([i915#750])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9640/fi-tgl-u2/igt@i915_selftest@live@hangcheck.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-tgl-u2/igt@i915_selftest@live@hangcheck.html

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-byt-j1900:       NOTRUN -> [SKIP][15] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-byt-j1900/igt@kms_chamelium@hdmi-crc-fast.html

  * igt@runner@aborted:
    - fi-bsw-n3050:       NOTRUN -> [FAIL][16] ([i915#1436])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-bsw-n3050/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@prime_vgem@basic-fence-flip:
    - fi-tgl-y:           [DMESG-WARN][17] ([i915#402]) -> [PASS][18] +1 similar issue
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9640/fi-tgl-y/igt@prime_vgem@basic-fence-flip.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/fi-tgl-y/igt@prime_vgem@basic-fence-flip.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
  [i915#2283]: https://gitlab.freedesktop.org/drm/intel/issues/2283
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2940]: https://gitlab.freedesktop.org/drm/intel/issues/2940
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#750]: https://gitlab.freedesktop.org/drm/intel/issues/750


Participating hosts (42 -> 38)
------------------------------

  Additional (1): fi-byt-j1900 
  Missing    (5): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_9640 -> Patchwork_19404

  CI-20190529: 20190529
  CI_DRM_9640: e4a7b069614eee3e01353d8529d5cf8b924feeec @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5960: ace82fcd5f3623f8dde7c220a825873dc53dfae4 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_19404: 69e32e3d770453dfd5e68d026bffc3221ebbe0fe @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

69e32e3d7704 drm/i915: Mark per-engine-reset as supported on gen7
15ca8069079a drm/i915/gt: Pull ring submission resume under its caller forcewake
93f264342613 drm/i915/gt: Lift stop_ring() to reset_prepare
9a7efa8c5162 drm/i915/selftests: Prepare the selftests for engine resets with ring submission
6c4ba82f7405 drm/i915/gt: One more flush for Baytrail clear residuals

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_19404/index.html

[-- Attachment #1.2: Type: text/html, Size: 7310 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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

end of thread, other threads:[~2021-01-19 13:41 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-19  9:40 [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Chris Wilson
2021-01-19  9:40 ` [Intel-gfx] [PATCH 2/5] drm/i915/selftests: Prepare the selftests for engine resets with ring submission Chris Wilson
2021-01-19 10:26   ` Mika Kuoppala
2021-01-19  9:40 ` [Intel-gfx] [PATCH 3/5] drm/i915/gt: Lift stop_ring() to reset_prepare Chris Wilson
2021-01-19 10:33   ` Mika Kuoppala
2021-01-19 10:39     ` Chris Wilson
2021-01-19  9:40 ` [Intel-gfx] [PATCH 4/5] drm/i915/gt: Pull ring submission resume under its caller forcewake Chris Wilson
2021-01-19 10:55   ` Mika Kuoppala
2021-01-19  9:40 ` [Intel-gfx] [PATCH 5/5] drm/i915: Mark per-engine-reset as supported on gen7 Chris Wilson
2021-01-19 11:00   ` Mika Kuoppala
2021-01-19 11:06     ` Chris Wilson
2021-01-19 10:21 ` [Intel-gfx] [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals Abodunrin, Akeem G
2021-01-19 10:25 ` Mika Kuoppala
2021-01-19 10:34   ` Chris Wilson
2021-01-19 10:59     ` Mika Kuoppala
2021-01-19 13:08 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/5] " Patchwork
2021-01-19 13:41 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).