All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register
@ 2016-10-03 12:52 Chris Wilson
  2016-10-03 12:52 ` [PATCH 2/8] drm/i915/execlists: Reinitialise context image after GPU hang Chris Wilson
                   ` (9 more replies)
  0 siblings, 10 replies; 22+ messages in thread
From: Chris Wilson @ 2016-10-03 12:52 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala

Since both legacy and execlists want to poopulate the RING_CTL register,
share the computation of the right bits for the ring->size. We can then
stop masking errors and explicitly forbid them during creation!

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 1 +
 drivers/gpu/drm/i915/intel_lrc.c        | 2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 5 ++---
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8d44cee710f0..acc767a52d8e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1605,6 +1605,7 @@ enum skl_disp_power_wells {
 #define RING_HEAD(base)		_MMIO((base)+0x34)
 #define RING_START(base)	_MMIO((base)+0x38)
 #define RING_CTL(base)		_MMIO((base)+0x3c)
+#define   RING_CTL_SIZE(size)	((size) - PAGE_SIZE) /* in bytes -> pages */
 #define RING_SYNC_0(base)	_MMIO((base)+0x40)
 #define RING_SYNC_1(base)	_MMIO((base)+0x44)
 #define RING_SYNC_2(base)	_MMIO((base)+0x48)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2d8eb2eb2b72..5ede272eb4d2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1946,7 +1946,7 @@ populate_lr_context(struct i915_gem_context *ctx,
 		       RING_START(engine->mmio_base), 0);
 	ASSIGN_CTX_REG(reg_state, CTX_RING_BUFFER_CONTROL,
 		       RING_CTL(engine->mmio_base),
-		       ((ring->size - PAGE_SIZE) & RING_NR_PAGES) | RING_VALID);
+		       RING_CTL_SIZE(ring->size) | RING_VALID);
 	ASSIGN_CTX_REG(reg_state, CTX_BB_HEAD_U,
 		       RING_BBADDR_UDW(engine->mmio_base), 0);
 	ASSIGN_CTX_REG(reg_state, CTX_BB_HEAD_L,
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 67ea9dd5921e..26aa4c5e268f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -585,9 +585,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
 	I915_WRITE_TAIL(engine, ring->tail);
 	(void)I915_READ_TAIL(engine);
 
-	I915_WRITE_CTL(engine,
-			((ring->size - PAGE_SIZE) & RING_NR_PAGES)
-			| RING_VALID);
+	I915_WRITE_CTL(engine, RING_CTL_SIZE(ring->size) | RING_VALID);
 
 	/* If the head is still not zero, the ring is dead */
 	if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
@@ -1951,6 +1949,7 @@ intel_engine_create_ring(struct intel_engine_cs *engine, int size)
 	struct i915_vma *vma;
 
 	GEM_BUG_ON(!is_power_of_2(size));
+	GEM_BUG_ON(size & ~RING_NR_PAGES);
 
 	ring = kzalloc(sizeof(*ring), GFP_KERNEL);
 	if (!ring)
-- 
2.9.3

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

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

* [PATCH 2/8] drm/i915/execlists: Reinitialise context image after GPU hang
  2016-10-03 12:52 [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
@ 2016-10-03 12:52 ` Chris Wilson
  2016-10-03 12:52 ` [PATCH 3/8] drm/i915/execlists: Move clearing submission count from reset to init Chris Wilson
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2016-10-03 12:52 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala

On Braswell, at least, we observe that the context image is written in
multiple phases. The first phase is to clear the register state, and
subsequently rewrite it. A GPU reset at the right moment can interrupt
the context update leaving it corrupt, and our update of the RING_HEAD
is not sufficient to restart the engine afterwards. To recover, we need
to reset the registers back to their original values. The context state
is lost. What we need is a better mechanism to serialise the reset with
pending flushes from the GPU.

Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 103 +++++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 5ede272eb4d2..0ea992ba2723 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -226,10 +226,16 @@ enum {
 /* Typical size of the average request (2 pipecontrols and a MI_BB) */
 #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
 
+#define WA_TAIL_DWORDS 2
+
 static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
 					    struct intel_engine_cs *engine);
 static int intel_lr_context_pin(struct i915_gem_context *ctx,
 				struct intel_engine_cs *engine);
+static void execlists_init_reg_state(u32 *reg_state,
+				     struct i915_gem_context *ctx,
+				     struct intel_engine_cs *engine,
+				     struct intel_ring *ring);
 
 /**
  * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists
@@ -707,7 +713,6 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
 {
 	struct intel_context *ce = &ctx->engine[engine->id];
 	void *vaddr;
-	u32 *lrc_reg_state;
 	int ret;
 
 	lockdep_assert_held(&ctx->i915->drm.struct_mutex);
@@ -726,17 +731,16 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
 		goto unpin_vma;
 	}
 
-	lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
-
 	ret = intel_ring_pin(ce->ring);
 	if (ret)
 		goto unpin_map;
 
 	intel_lr_context_descriptor_update(ctx, engine);
 
-	lrc_reg_state[CTX_RING_BUFFER_START+1] =
+	ce->lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
+	ce->lrc_reg_state[CTX_RING_BUFFER_START+1] =
 		i915_ggtt_offset(ce->ring->vma);
-	ce->lrc_reg_state = lrc_reg_state;
+
 	ce->state->obj->dirty = true;
 
 	/* Invalidate GuC TLB. */
@@ -1284,8 +1288,21 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 	struct execlist_port *port = engine->execlist_port;
 	struct intel_context *ce = &request->ctx->engine[engine->id];
 
+	/* We want a simple context + ring to execute the breadcrumb update.
+	 * We cannot rely on the context being intact across the GPU hang,
+	 * so clear it and rebuild just what we need for the breadcrumb.
+	 * All pending requests for this context will be zapped, and any
+	 * future request will be after userspace has had the opportunity
+	 * to recreate its own state.
+	 */
+	execlists_init_reg_state(ce->lrc_reg_state,
+				 request->ctx, engine, ce->ring);
+
 	/* Move the RING_HEAD onto the breadcrumb, past the hanging batch */
+	ce->lrc_reg_state[CTX_RING_BUFFER_START+1] =
+		i915_ggtt_offset(ce->ring->vma);
 	ce->lrc_reg_state[CTX_RING_HEAD+1] = request->postfix;
+
 	request->ring->head = request->postfix;
 	request->ring->last_retired_head = -1;
 	intel_ring_update_space(request->ring);
@@ -1305,6 +1322,9 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 	GEM_BUG_ON(request->ctx != port[0].request->ctx);
 	port[0].count = 0;
 	port[1].count = 0;
+
+	/* Reset WaIdleLiteRestore:bdw,skl as well */
+	request->tail = request->wa_tail - WA_TAIL_DWORDS * sizeof(u32);
 }
 
 static int intel_logical_ring_emit_pdps(struct drm_i915_gem_request *req)
@@ -1542,7 +1562,6 @@ static void bxt_a_seqno_barrier(struct intel_engine_cs *engine)
  * used as a workaround for not being allowed to do lite
  * restore with HEAD==TAIL (WaIdleLiteRestore).
  */
-#define WA_TAIL_DWORDS 2
 
 static int gen8_emit_request(struct drm_i915_gem_request *request)
 {
@@ -1889,38 +1908,13 @@ static u32 intel_lr_indirect_ctx_offset(struct intel_engine_cs *engine)
 	return indirect_ctx_offset;
 }
 
-static int
-populate_lr_context(struct i915_gem_context *ctx,
-		    struct drm_i915_gem_object *ctx_obj,
-		    struct intel_engine_cs *engine,
-		    struct intel_ring *ring)
+static void execlists_init_reg_state(u32 *reg_state,
+				     struct i915_gem_context *ctx,
+				     struct intel_engine_cs *engine,
+				     struct intel_ring *ring)
 {
-	struct drm_i915_private *dev_priv = ctx->i915;
-	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
-	void *vaddr;
-	u32 *reg_state;
-	int ret;
-
-	if (!ppgtt)
-		ppgtt = dev_priv->mm.aliasing_ppgtt;
-
-	ret = i915_gem_object_set_to_cpu_domain(ctx_obj, true);
-	if (ret) {
-		DRM_DEBUG_DRIVER("Could not set to CPU domain\n");
-		return ret;
-	}
-
-	vaddr = i915_gem_object_pin_map(ctx_obj, I915_MAP_WB);
-	if (IS_ERR(vaddr)) {
-		ret = PTR_ERR(vaddr);
-		DRM_DEBUG_DRIVER("Could not map object pages! (%d)\n", ret);
-		return ret;
-	}
-	ctx_obj->dirty = true;
-
-	/* The second page of the context object contains some fields which must
-	 * be set up prior to the first execution. */
-	reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
+	struct drm_i915_private *dev_priv = engine->i915;
+	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt ?: dev_priv->mm.aliasing_ppgtt;
 
 	/* A context is actually a big batch buffer with several MI_LOAD_REGISTER_IMM
 	 * commands followed by (reg, value) pairs. The values we are setting here are
@@ -1934,14 +1928,11 @@ populate_lr_context(struct i915_gem_context *ctx,
 		       _MASKED_BIT_ENABLE(CTX_CTRL_INHIBIT_SYN_CTX_SWITCH |
 					  CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
 					  (HAS_RESOURCE_STREAMER(dev_priv) ?
-					    CTX_CTRL_RS_CTX_ENABLE : 0)));
+					   CTX_CTRL_RS_CTX_ENABLE : 0)));
 	ASSIGN_CTX_REG(reg_state, CTX_RING_HEAD, RING_HEAD(engine->mmio_base),
 		       0);
 	ASSIGN_CTX_REG(reg_state, CTX_RING_TAIL, RING_TAIL(engine->mmio_base),
 		       0);
-	/* Ring buffer start address is not known until the buffer is pinned.
-	 * It is written to the context image in execlists_update_context()
-	 */
 	ASSIGN_CTX_REG(reg_state, CTX_RING_BUFFER_START,
 		       RING_START(engine->mmio_base), 0);
 	ASSIGN_CTX_REG(reg_state, CTX_RING_BUFFER_CONTROL,
@@ -2024,6 +2015,36 @@ populate_lr_context(struct i915_gem_context *ctx,
 		ASSIGN_CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
 			       make_rpcs(dev_priv));
 	}
+}
+
+static int
+populate_lr_context(struct i915_gem_context *ctx,
+		    struct drm_i915_gem_object *ctx_obj,
+		    struct intel_engine_cs *engine,
+		    struct intel_ring *ring)
+{
+	void *vaddr;
+	int ret;
+
+	ret = i915_gem_object_set_to_cpu_domain(ctx_obj, true);
+	if (ret) {
+		DRM_DEBUG_DRIVER("Could not set to CPU domain\n");
+		return ret;
+	}
+
+	vaddr = i915_gem_object_pin_map(ctx_obj, I915_MAP_WB);
+	if (IS_ERR(vaddr)) {
+		ret = PTR_ERR(vaddr);
+		DRM_DEBUG_DRIVER("Could not map object pages! (%d)\n", ret);
+		return ret;
+	}
+	ctx_obj->dirty = true;
+
+	/* The second page of the context object contains some fields which must
+	 * be set up prior to the first execution. */
+
+	execlists_init_reg_state(vaddr + LRC_STATE_PN * PAGE_SIZE,
+				 ctx, engine, ring);
 
 	i915_gem_object_unpin_map(ctx_obj);
 
-- 
2.9.3

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

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

* [PATCH 3/8] drm/i915/execlists: Move clearing submission count from reset to init
  2016-10-03 12:52 [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
  2016-10-03 12:52 ` [PATCH 2/8] drm/i915/execlists: Reinitialise context image after GPU hang Chris Wilson
@ 2016-10-03 12:52 ` Chris Wilson
  2016-10-03 13:07   ` Mika Kuoppala
  2016-10-03 12:52 ` [PATCH 4/8] drm/i915: Disable irqs across GPU reset Chris Wilson
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2016-10-03 12:52 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala

After a GPU reset, we want to replay our queue of requests. However, the
GPU reset clobbered the state and we only fixup the state for the guitly
request - and engines deemed innocent we try to leave untouched so that
we recover as completely as possible. However, we need to clear the sw
tracking of the ELSP ports even for innocent requests, so move the clear
to the common path of init_hw (from reset_hw).

Reported-by: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 0ea992ba2723..936f6f63f626 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1242,8 +1242,12 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
 
 	intel_engine_init_hangcheck(engine);
 
-	if (!execlists_elsp_idle(engine))
+	/* After a GPU reset, we may have requests to replay */
+	if (!execlists_elsp_idle(engine)) {
+		engine->execlist_port[0].count = 0;
+		engine->execlist_port[1].count = 0;
 		execlists_submit_ports(engine);
+	}
 
 	return 0;
 }
@@ -1318,10 +1322,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
 		memset(&port[1], 0, sizeof(port[1]));
 	}
 
-	/* CS is stopped, and we will resubmit both ports on resume */
 	GEM_BUG_ON(request->ctx != port[0].request->ctx);
-	port[0].count = 0;
-	port[1].count = 0;
 
 	/* Reset WaIdleLiteRestore:bdw,skl as well */
 	request->tail = request->wa_tail - WA_TAIL_DWORDS * sizeof(u32);
-- 
2.9.3

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

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

* [PATCH 4/8] drm/i915: Disable irqs across GPU reset
  2016-10-03 12:52 [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
  2016-10-03 12:52 ` [PATCH 2/8] drm/i915/execlists: Reinitialise context image after GPU hang Chris Wilson
  2016-10-03 12:52 ` [PATCH 3/8] drm/i915/execlists: Move clearing submission count from reset to init Chris Wilson
@ 2016-10-03 12:52 ` Chris Wilson
  2016-10-03 13:09   ` Mika Kuoppala
  2016-10-03 12:52 ` [PATCH 5/8] drm/i915: Double check hangcheck.seqno after reset Chris Wilson
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2016-10-03 12:52 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala

Whilst we reset the GPU, we want to prevent execlists from submitting
new work (which it does via an interrupt handler). To achieve this we
disable the irq (and drain the irq tasklet) around the reset. When we
enable it again afters, the interrupt queue should be empty and we can
reinitialise from a known state without fear of the tasklet running
concurrently.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c |  2 --
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 31b2b6300d8d..7ce498898217 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1728,6 +1728,21 @@ int i915_resume_switcheroo(struct drm_device *dev)
 	return i915_drm_resume(dev);
 }
 
+static void disable_engines_irq(struct drm_i915_private *dev_priv)
+{
+	struct intel_engine_cs *engine;
+
+	/* Ensure irq handler finishes, and not run again. */
+	disable_irq(dev_priv->drm.irq);
+	for_each_engine(engine, dev_priv)
+		tasklet_kill(&engine->irq_tasklet);
+}
+
+static void enable_engines_irq(struct drm_i915_private *dev_priv)
+{
+	enable_irq(dev_priv->drm.irq);
+}
+
 /**
  * i915_reset - reset chip after a hang
  * @dev: drm device to reset
@@ -1761,7 +1776,11 @@ void i915_reset(struct drm_i915_private *dev_priv)
 	error->reset_count++;
 
 	pr_notice("drm/i915: Resetting chip after gpu hang\n");
+
+	disable_engines_irq(dev_priv);
 	ret = intel_gpu_reset(dev_priv, ALL_ENGINES);
+	enable_engines_irq(dev_priv);
+
 	if (ret) {
 		if (ret != -ENODEV)
 			DRM_ERROR("Failed to reset chip: %i\n", ret);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1418c1c522cb..0cae8acdf906 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2581,8 +2581,6 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	struct i915_gem_context *incomplete_ctx;
 	bool ring_hung;
 
-	/* Ensure irq handler finishes, and not run again. */
-	tasklet_kill(&engine->irq_tasklet);
 	if (engine->irq_seqno_barrier)
 		engine->irq_seqno_barrier(engine);
 
-- 
2.9.3

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

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

* [PATCH 5/8] drm/i915: Double check hangcheck.seqno after reset
  2016-10-03 12:52 [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
                   ` (2 preceding siblings ...)
  2016-10-03 12:52 ` [PATCH 4/8] drm/i915: Disable irqs across GPU reset Chris Wilson
@ 2016-10-03 12:52 ` Chris Wilson
  2016-10-03 13:14   ` Mika Kuoppala
  2016-10-03 12:52 ` [PATCH 6/8] drm/i915: Show bounds of active request in the ring on GPU hang Chris Wilson
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2016-10-03 12:52 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala

Check that there was not a late recovery between us declaring the GPU
hung and processing the reset. If the GPU did recover by itself, let the
request remain on the active list and see if it hangs again!

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0cae8acdf906..a89a88922448 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2589,6 +2589,9 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 		return;
 
 	ring_hung = engine->hangcheck.score >= HANGCHECK_SCORE_RING_HUNG;
+	if (engine->hangcheck.seqno != intel_engine_get_seqno(engine))
+		ring_hung = false;
+
 	i915_set_reset_status(request->ctx, ring_hung);
 	if (!ring_hung)
 		return;
-- 
2.9.3

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

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

* [PATCH 6/8] drm/i915: Show bounds of active request in the ring on GPU hang
  2016-10-03 12:52 [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
                   ` (3 preceding siblings ...)
  2016-10-03 12:52 ` [PATCH 5/8] drm/i915: Double check hangcheck.seqno after reset Chris Wilson
@ 2016-10-03 12:52 ` Chris Wilson
  2016-10-04 11:56   ` Mika Kuoppala
  2016-10-03 12:52 ` [PATCH 7/8] drm/i915: Show RING registers through debugfs Chris Wilson
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2016-10-03 12:52 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala

Include the position of the active request in the ring, and display that
alongside the current RING registers (on a GPU hang).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       | 3 +++
 drivers/gpu/drm/i915/i915_gpu_error.c | 9 +++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 91ac283d733e..e48044b3a447 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -776,6 +776,9 @@ struct drm_i915_error_state {
 		struct i915_address_space *vm;
 		int num_requests;
 
+		/* position of active request inside the ring */
+		u32 rq_head, rq_post, rq_tail;
+
 		/* our own tracking of ring head and tail */
 		u32 cpu_ring_head;
 		u32 cpu_ring_tail;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 2bbab226a46c..8b85efbdfa04 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -262,8 +262,9 @@ static void error_print_engine(struct drm_i915_error_state_buf *m,
 {
 	err_printf(m, "%s command stream:\n", engine_str(ee->engine_id));
 	err_printf(m, "  START: 0x%08x\n", ee->start);
-	err_printf(m, "  HEAD:  0x%08x\n", ee->head);
-	err_printf(m, "  TAIL:  0x%08x\n", ee->tail);
+	err_printf(m, "  HEAD:  0x%08x\n [0x%08x]", ee->head, ee->rq_head);
+	err_printf(m, "  TAIL:  0x%08x [0x%08x, 0x%08x]\n",
+		   ee->tail, ee->rq_post, ee->rq_tail);
 	err_printf(m, "  CTL:   0x%08x\n", ee->ctl);
 	err_printf(m, "  MODE:  0x%08x\n", ee->mode);
 	err_printf(m, "  HWS:   0x%08x\n", ee->hws);
@@ -1230,6 +1231,10 @@ static void i915_gem_record_rings(struct drm_i915_private *dev_priv,
 			error->simulated |=
 				request->ctx->flags & CONTEXT_NO_ERROR_CAPTURE;
 
+			ee->rq_head = request->head;
+			ee->rq_post = request->postfix;
+			ee->rq_tail = request->tail;
+
 			ring = request->ring;
 			ee->cpu_ring_head = ring->head;
 			ee->cpu_ring_tail = ring->tail;
-- 
2.9.3

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

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

* [PATCH 7/8] drm/i915: Show RING registers through debugfs
  2016-10-03 12:52 [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
                   ` (4 preceding siblings ...)
  2016-10-03 12:52 ` [PATCH 6/8] drm/i915: Show bounds of active request in the ring on GPU hang Chris Wilson
@ 2016-10-03 12:52 ` Chris Wilson
  2016-10-04 12:35   ` Mika Kuoppala
  2016-10-03 12:52 ` [PATCH 8/8] drm/i915: Show waiters in i915_hangcheck_info Chris Wilson
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2016-10-03 12:52 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala

Knowing where the RINGs are pointing is extremely useful in diagnosing
if the engines are executing the ringbuffers you expect - and igt may be
suppressing the usual method of looking in the GPU error state.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c     | 230 +++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_engine_cs.c  |  30 +++++
 drivers/gpu/drm/i915/intel_ringbuffer.c |  16 ---
 drivers/gpu/drm/i915/intel_ringbuffer.h |   2 +
 4 files changed, 170 insertions(+), 108 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4fb9d829884e..9c95ce73f2aa 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -635,6 +635,23 @@ static int i915_gem_batch_pool_info(struct seq_file *m, void *data)
 	return 0;
 }
 
+static void print_request(struct seq_file *m,
+			  struct drm_i915_gem_request *rq,
+			  const char *prefix)
+{
+	struct pid *pid = rq->ctx->pid;
+	struct task_struct *task;
+
+	rcu_read_lock();
+	task = pid ? pid_task(pid, PIDTYPE_PID) : NULL;
+	seq_printf(m, "%s%x [%x:%x] @ %d: %s [%d]\n", prefix,
+		   rq->fence.seqno, rq->ctx->hw_id, rq->fence.seqno,
+		   jiffies_to_msecs(jiffies - rq->emitted_jiffies),
+		   task ? task->comm : "<unknown>",
+		   task ? task->pid : -1);
+	rcu_read_unlock();
+}
+
 static int i915_gem_request_info(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -658,19 +675,8 @@ static int i915_gem_request_info(struct seq_file *m, void *data)
 			continue;
 
 		seq_printf(m, "%s requests: %d\n", engine->name, count);
-		list_for_each_entry(req, &engine->request_list, link) {
-			struct pid *pid = req->ctx->pid;
-			struct task_struct *task;
-
-			rcu_read_lock();
-			task = pid ? pid_task(pid, PIDTYPE_PID) : NULL;
-			seq_printf(m, "    %x @ %d: %s [%d]\n",
-				   req->fence.seqno,
-				   (int) (jiffies - req->emitted_jiffies),
-				   task ? task->comm : "<unknown>",
-				   task ? task->pid : -1);
-			rcu_read_unlock();
-		}
+		list_for_each_entry(req, &engine->request_list, link)
+			print_request(m, req, "    ");
 
 		any++;
 	}
@@ -2036,84 +2042,6 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
 	return 0;
 }
 
-static int i915_execlists(struct seq_file *m, void *data)
-{
-	struct drm_i915_private *dev_priv = node_to_i915(m->private);
-	struct drm_device *dev = &dev_priv->drm;
-	struct intel_engine_cs *engine;
-	u32 status_pointer;
-	u8 read_pointer;
-	u8 write_pointer;
-	u32 status;
-	u32 ctx_id;
-	struct list_head *cursor;
-	int i, ret;
-
-	if (!i915.enable_execlists) {
-		seq_puts(m, "Logical Ring Contexts are disabled\n");
-		return 0;
-	}
-
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		return ret;
-
-	intel_runtime_pm_get(dev_priv);
-
-	for_each_engine(engine, dev_priv) {
-		struct drm_i915_gem_request *head_req = NULL;
-		int count = 0;
-
-		seq_printf(m, "%s\n", engine->name);
-
-		status = I915_READ(RING_EXECLIST_STATUS_LO(engine));
-		ctx_id = I915_READ(RING_EXECLIST_STATUS_HI(engine));
-		seq_printf(m, "\tExeclist status: 0x%08X, context: %u\n",
-			   status, ctx_id);
-
-		status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(engine));
-		seq_printf(m, "\tStatus pointer: 0x%08X\n", status_pointer);
-
-		read_pointer = GEN8_CSB_READ_PTR(status_pointer);
-		write_pointer = GEN8_CSB_WRITE_PTR(status_pointer);
-		if (read_pointer > write_pointer)
-			write_pointer += GEN8_CSB_ENTRIES;
-		seq_printf(m, "\tRead pointer: 0x%08X, write pointer 0x%08X\n",
-			   read_pointer, write_pointer);
-
-		for (i = 0; i < GEN8_CSB_ENTRIES; i++) {
-			status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(engine, i));
-			ctx_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(engine, i));
-
-			seq_printf(m, "\tStatus buffer %d: 0x%08X, context: %u\n",
-				   i, status, ctx_id);
-		}
-
-		spin_lock_bh(&engine->execlist_lock);
-		list_for_each(cursor, &engine->execlist_queue)
-			count++;
-		head_req = list_first_entry_or_null(&engine->execlist_queue,
-						    struct drm_i915_gem_request,
-						    execlist_link);
-		spin_unlock_bh(&engine->execlist_lock);
-
-		seq_printf(m, "\t%d requests in queue\n", count);
-		if (head_req) {
-			seq_printf(m, "\tHead request context: %u\n",
-				   head_req->ctx->hw_id);
-			seq_printf(m, "\tHead request tail: %u\n",
-				   head_req->tail);
-		}
-
-		seq_putc(m, '\n');
-	}
-
-	intel_runtime_pm_put(dev_priv);
-	mutex_unlock(&dev->struct_mutex);
-
-	return 0;
-}
-
 static const char *swizzle_string(unsigned swizzle)
 {
 	switch (swizzle) {
@@ -3135,6 +3063,124 @@ static int i915_display_info(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static int i915_engine_info(struct seq_file *m, void *unused)
+{
+	struct drm_i915_private *dev_priv = node_to_i915(m->private);
+	struct intel_engine_cs *engine;
+
+	for_each_engine(engine, dev_priv) {
+		struct intel_breadcrumbs *b = &engine->breadcrumbs;
+		struct drm_i915_gem_request *rq;
+		struct rb_node *rb;
+		u64 addr;
+		int count;
+
+		seq_printf(m, "%s\n", engine->name);
+		seq_printf(m, "\tcurrent seqno %x, last %x, hangcheck %x [score %d]\n",
+			   intel_engine_get_seqno(engine),
+			   engine->last_submitted_seqno,
+			   engine->hangcheck.seqno,
+			   engine->hangcheck.score);
+
+		rcu_read_lock();
+
+		count = 0;
+		list_for_each_entry(rq, &engine->request_list, link)
+			count++;
+		seq_printf(m, "\t%d submitted requests\n", count);
+
+		if (count > 0) {
+			rq = list_first_entry(&engine->request_list,
+					      struct drm_i915_gem_request, link);
+			print_request(m, rq, "\t\tfirst  ");
+
+			rq = list_last_entry(&engine->request_list,
+					     struct drm_i915_gem_request, link);
+			print_request(m, rq, "\t\tlast   ");
+		}
+
+		rq = i915_gem_find_active_request(engine);
+		if (rq) {
+			print_request(m, rq, "\t\tactive ");
+			seq_printf(m,
+				   "\t\t[head %04x, postfix %04x, tail %04x\n, batch 0x%08x_%08x]",
+				   rq->head, rq->postfix, rq->tail,
+				   rq->batch ? upper_32_bits(rq->batch->node.start) : ~0u,
+				   rq->batch ? lower_32_bits(rq->batch->node.start) : ~0u);
+		}
+
+		seq_printf(m, "\tRING_START: 0x%08x [0x%08x]\n",
+			   I915_READ(RING_START(engine->mmio_base)),
+			   rq ? i915_ggtt_offset(rq->ring->vma) : 0);
+		seq_printf(m, "\tRING_HEAD:  0x%04x [0x%04x]\n",
+			   I915_READ(RING_HEAD(engine->mmio_base)) & HEAD_ADDR,
+			   rq ? rq->ring->head : 0);
+		seq_printf(m, "\tRING_TAIL:  0x%04x [0x%04x]\n",
+			   I915_READ(RING_TAIL(engine->mmio_base)) & TAIL_ADDR,
+			   rq ? rq->ring->tail : 0);
+		seq_printf(m, "\tRING_CTL:   0x%08x [%s]\n",
+			   I915_READ(RING_CTL(engine->mmio_base)),
+			   I915_READ(RING_CTL(engine->mmio_base)) & (RING_WAIT | RING_WAIT_SEMAPHORE) ? "waiting" : "");
+
+		rcu_read_unlock();
+
+		addr = intel_engine_get_active_head(engine);
+		seq_printf(m, "\tACTHD:  0x%08x_%08x\n",
+			   upper_32_bits(addr), lower_32_bits(addr));
+		addr = intel_engine_get_last_batch_head(engine);
+		seq_printf(m, "\tBBADDR: 0x%08x_%08x\n",
+			   upper_32_bits(addr), lower_32_bits(addr));
+
+		if (i915.enable_execlists) {
+			u32 ptr, read, write;
+
+			seq_printf(m, "\tExeclist status: 0x%08x %08x\n",
+				   I915_READ(RING_EXECLIST_STATUS_LO(engine)),
+				   I915_READ(RING_EXECLIST_STATUS_HI(engine)));
+
+			ptr = I915_READ(RING_CONTEXT_STATUS_PTR(engine));
+			read = GEN8_CSB_READ_PTR(ptr);
+			write = GEN8_CSB_WRITE_PTR(ptr);
+			seq_printf(m, "\tExeclist CSB read %d, write %d\n",
+				   read, write);
+			if (read >= GEN8_CSB_ENTRIES)
+				read = 0;
+			if (write >= GEN8_CSB_ENTRIES)
+				write = 0;
+			if (read > write)
+				write += GEN8_CSB_ENTRIES;
+			while (read < write) {
+				unsigned int idx = ++read % GEN8_CSB_ENTRIES;
+
+				seq_printf(m, "\tExeclist CSB[%d]: 0x%08x, context: %d\n",
+					   idx,
+					   I915_READ(RING_CONTEXT_STATUS_BUF_LO(engine, idx)),
+					   I915_READ(RING_CONTEXT_STATUS_BUF_HI(engine, idx)));
+			}
+		} else if (INTEL_GEN(dev_priv) > 6) {
+			seq_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
+				   I915_READ(RING_PP_DIR_BASE(engine)));
+			seq_printf(m, "\tPP_DIR_BASE_READ: 0x%08x\n",
+				   I915_READ(RING_PP_DIR_BASE_READ(engine)));
+			seq_printf(m, "\tPP_DIR_DCLV: 0x%08x\n",
+				   I915_READ(RING_PP_DIR_DCLV(engine)));
+		}
+
+		spin_lock(&b->lock);
+		for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
+			struct intel_wait *w = container_of(rb, typeof(*w), node);
+
+			seq_printf(m, "\t%s [%d] waiting for %x\n",
+				   w->tsk->comm, w->tsk->pid, w->seqno);
+		}
+		spin_unlock(&b->lock);
+
+		seq_puts(m, "\n");
+	}
+
+	return 0;
+}
+
 static int i915_semaphore_status(struct seq_file *m, void *unused)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -4441,7 +4487,6 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
 	{"i915_context_status", i915_context_status, 0},
 	{"i915_dump_lrc", i915_dump_lrc, 0},
-	{"i915_execlists", i915_execlists, 0},
 	{"i915_forcewake_domains", i915_forcewake_domains, 0},
 	{"i915_swizzle_info", i915_swizzle_info, 0},
 	{"i915_ppgtt_info", i915_ppgtt_info, 0},
@@ -4453,6 +4498,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_power_domain_info", i915_power_domain_info, 0},
 	{"i915_dmc_info", i915_dmc_info, 0},
 	{"i915_display_info", i915_display_info, 0},
+	{"i915_engine_info", i915_engine_info, 0},
 	{"i915_semaphore_status", i915_semaphore_status, 0},
 	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index e405f1080296..d00ec805f93d 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -334,3 +334,33 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 	intel_engine_cleanup_cmd_parser(engine);
 	i915_gem_batch_pool_fini(&engine->batch_pool);
 }
+
+u64 intel_engine_get_active_head(struct intel_engine_cs *engine)
+{
+	struct drm_i915_private *dev_priv = engine->i915;
+	u64 acthd;
+
+	if (INTEL_GEN(dev_priv) >= 8)
+		acthd = I915_READ64_2x32(RING_ACTHD(engine->mmio_base),
+					 RING_ACTHD_UDW(engine->mmio_base));
+	else if (INTEL_GEN(dev_priv) >= 4)
+		acthd = I915_READ(RING_ACTHD(engine->mmio_base));
+	else
+		acthd = I915_READ(ACTHD);
+
+	return acthd;
+}
+
+u64 intel_engine_get_last_batch_head(struct intel_engine_cs *engine)
+{
+	struct drm_i915_private *dev_priv = engine->i915;
+	u64 bbaddr;
+
+	if (INTEL_GEN(dev_priv) >= 8)
+		bbaddr = I915_READ64_2x32(RING_BBADDR(engine->mmio_base),
+					  RING_BBADDR_UDW(engine->mmio_base));
+	else
+		bbaddr = I915_READ(RING_BBADDR(engine->mmio_base));
+
+	return bbaddr;
+}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 26aa4c5e268f..87f4a2666c4e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -405,22 +405,6 @@ gen8_render_ring_flush(struct drm_i915_gem_request *req, u32 mode)
 	return gen8_emit_pipe_control(req, flags, scratch_addr);
 }
 
-u64 intel_engine_get_active_head(struct intel_engine_cs *engine)
-{
-	struct drm_i915_private *dev_priv = engine->i915;
-	u64 acthd;
-
-	if (INTEL_GEN(dev_priv) >= 8)
-		acthd = I915_READ64_2x32(RING_ACTHD(engine->mmio_base),
-					 RING_ACTHD_UDW(engine->mmio_base));
-	else if (INTEL_GEN(dev_priv) >= 4)
-		acthd = I915_READ(RING_ACTHD(engine->mmio_base));
-	else
-		acthd = I915_READ(ACTHD);
-
-	return acthd;
-}
-
 static void ring_setup_phys_status_page(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 66553bdb0a47..498931f0b1f1 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -541,6 +541,8 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine);
 int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine);
 
 u64 intel_engine_get_active_head(struct intel_engine_cs *engine);
+u64 intel_engine_get_last_batch_head(struct intel_engine_cs *engine);
+
 static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine)
 {
 	return intel_read_status_page(engine, I915_GEM_HWS_INDEX);
-- 
2.9.3

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

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

* [PATCH 8/8] drm/i915: Show waiters in i915_hangcheck_info
  2016-10-03 12:52 [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
                   ` (5 preceding siblings ...)
  2016-10-03 12:52 ` [PATCH 7/8] drm/i915: Show RING registers through debugfs Chris Wilson
@ 2016-10-03 12:52 ` Chris Wilson
  2016-10-04 12:41   ` Mika Kuoppala
  2016-10-04 13:22   ` Mika Kuoppala
  2016-10-03 13:00 ` [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
                   ` (2 subsequent siblings)
  9 siblings, 2 replies; 22+ messages in thread
From: Chris Wilson @ 2016-10-03 12:52 UTC (permalink / raw)
  To: intel-gfx

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 9c95ce73f2aa..39b76c24c84f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1343,6 +1343,9 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
 		seq_printf(m, "Hangcheck inactive\n");
 
 	for_each_engine_id(engine, dev_priv, id) {
+		struct intel_breadcrumbs *b = &engine->breadcrumbs;
+		struct rb_node *rb;
+
 		seq_printf(m, "%s:\n", engine->name);
 		seq_printf(m, "\tseqno = %x [current %x, last %x]\n",
 			   engine->hangcheck.seqno,
@@ -1352,6 +1355,15 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
 			   yesno(intel_engine_has_waiter(engine)),
 			   yesno(test_bit(engine->id,
 					  &dev_priv->gpu_error.missed_irq_rings)));
+		spin_lock(&b->lock);
+		for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
+			struct intel_wait *w = container_of(rb, typeof(*w), node);
+
+			seq_printf(m, "\t%s [%d] waiting for %x\n",
+				   w->tsk->comm, w->tsk->pid, w->seqno);
+		}
+		spin_unlock(&b->lock);
+
 		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
 			   (long long)engine->hangcheck.acthd,
 			   (long long)acthd[id]);
-- 
2.9.3

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

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

* Re: [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register
  2016-10-03 12:52 [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
                   ` (6 preceding siblings ...)
  2016-10-03 12:52 ` [PATCH 8/8] drm/i915: Show waiters in i915_hangcheck_info Chris Wilson
@ 2016-10-03 13:00 ` Chris Wilson
  2016-10-04 12:51   ` Mika Kuoppala
  2016-10-03 13:01 ` Mika Kuoppala
  2016-10-03 14:49 ` ✗ Fi.CI.BAT: warning for series starting with [1/8] " Patchwork
  9 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2016-10-03 13:00 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala

On Mon, Oct 03, 2016 at 01:52:39PM +0100, Chris Wilson wrote:
> Since both legacy and execlists want to poopulate the RING_CTL register,
> share the computation of the right bits for the ring->size. We can then
> stop masking errors and explicitly forbid them during creation!
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 1 +
>  drivers/gpu/drm/i915/intel_lrc.c        | 2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 5 ++---
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8d44cee710f0..acc767a52d8e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1605,6 +1605,7 @@ enum skl_disp_power_wells {
>  #define RING_HEAD(base)		_MMIO((base)+0x34)
>  #define RING_START(base)	_MMIO((base)+0x38)
>  #define RING_CTL(base)		_MMIO((base)+0x3c)
> +#define   RING_CTL_SIZE(size)	((size) - PAGE_SIZE) /* in bytes -> pages */
>  #define RING_SYNC_0(base)	_MMIO((base)+0x40)
>  #define RING_SYNC_1(base)	_MMIO((base)+0x44)
>  #define RING_SYNC_2(base)	_MMIO((base)+0x48)
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 2d8eb2eb2b72..5ede272eb4d2 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1946,7 +1946,7 @@ populate_lr_context(struct i915_gem_context *ctx,
>  		       RING_START(engine->mmio_base), 0);
>  	ASSIGN_CTX_REG(reg_state, CTX_RING_BUFFER_CONTROL,
>  		       RING_CTL(engine->mmio_base),
> -		       ((ring->size - PAGE_SIZE) & RING_NR_PAGES) | RING_VALID);
> +		       RING_CTL_SIZE(ring->size) | RING_VALID);
>  	ASSIGN_CTX_REG(reg_state, CTX_BB_HEAD_U,
>  		       RING_BBADDR_UDW(engine->mmio_base), 0);
>  	ASSIGN_CTX_REG(reg_state, CTX_BB_HEAD_L,
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 67ea9dd5921e..26aa4c5e268f 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -585,9 +585,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
>  	I915_WRITE_TAIL(engine, ring->tail);
>  	(void)I915_READ_TAIL(engine);
>  
> -	I915_WRITE_CTL(engine,
> -			((ring->size - PAGE_SIZE) & RING_NR_PAGES)
> -			| RING_VALID);
> +	I915_WRITE_CTL(engine, RING_CTL_SIZE(ring->size) | RING_VALID);
>  
>  	/* If the head is still not zero, the ring is dead */
>  	if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
> @@ -1951,6 +1949,7 @@ intel_engine_create_ring(struct intel_engine_cs *engine, int size)
>  	struct i915_vma *vma;
>  
>  	GEM_BUG_ON(!is_power_of_2(size));
> +	GEM_BUG_ON(size & ~RING_NR_PAGES);

(size - PAGE_SIZE) & ~RING_NR_PAGES
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register
  2016-10-03 12:52 [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
                   ` (7 preceding siblings ...)
  2016-10-03 13:00 ` [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
@ 2016-10-03 13:01 ` Mika Kuoppala
  2016-10-03 14:49 ` ✗ Fi.CI.BAT: warning for series starting with [1/8] " Patchwork
  9 siblings, 0 replies; 22+ messages in thread
From: Mika Kuoppala @ 2016-10-03 13:01 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> Since both legacy and execlists want to poopulate the RING_CTL
> register,

A bit too graphic, lets switch to populate.

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

> share the computation of the right bits for the ring->size. We can then
> stop masking errors and explicitly forbid them during creation!
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 1 +
>  drivers/gpu/drm/i915/intel_lrc.c        | 2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 5 ++---
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8d44cee710f0..acc767a52d8e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1605,6 +1605,7 @@ enum skl_disp_power_wells {
>  #define RING_HEAD(base)		_MMIO((base)+0x34)
>  #define RING_START(base)	_MMIO((base)+0x38)
>  #define RING_CTL(base)		_MMIO((base)+0x3c)
> +#define   RING_CTL_SIZE(size)	((size) - PAGE_SIZE) /* in bytes -> pages */
>  #define RING_SYNC_0(base)	_MMIO((base)+0x40)
>  #define RING_SYNC_1(base)	_MMIO((base)+0x44)
>  #define RING_SYNC_2(base)	_MMIO((base)+0x48)
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 2d8eb2eb2b72..5ede272eb4d2 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1946,7 +1946,7 @@ populate_lr_context(struct i915_gem_context *ctx,
>  		       RING_START(engine->mmio_base), 0);
>  	ASSIGN_CTX_REG(reg_state, CTX_RING_BUFFER_CONTROL,
>  		       RING_CTL(engine->mmio_base),
> -		       ((ring->size - PAGE_SIZE) & RING_NR_PAGES) | RING_VALID);
> +		       RING_CTL_SIZE(ring->size) | RING_VALID);
>  	ASSIGN_CTX_REG(reg_state, CTX_BB_HEAD_U,
>  		       RING_BBADDR_UDW(engine->mmio_base), 0);
>  	ASSIGN_CTX_REG(reg_state, CTX_BB_HEAD_L,
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 67ea9dd5921e..26aa4c5e268f 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -585,9 +585,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
>  	I915_WRITE_TAIL(engine, ring->tail);
>  	(void)I915_READ_TAIL(engine);
>  
> -	I915_WRITE_CTL(engine,
> -			((ring->size - PAGE_SIZE) & RING_NR_PAGES)
> -			| RING_VALID);
> +	I915_WRITE_CTL(engine, RING_CTL_SIZE(ring->size) | RING_VALID);
>  
>  	/* If the head is still not zero, the ring is dead */
>  	if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
> @@ -1951,6 +1949,7 @@ intel_engine_create_ring(struct intel_engine_cs *engine, int size)
>  	struct i915_vma *vma;
>  
>  	GEM_BUG_ON(!is_power_of_2(size));
> +	GEM_BUG_ON(size & ~RING_NR_PAGES);
>  
>  	ring = kzalloc(sizeof(*ring), GFP_KERNEL);
>  	if (!ring)
> -- 
> 2.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 3/8] drm/i915/execlists: Move clearing submission count from reset to init
  2016-10-03 12:52 ` [PATCH 3/8] drm/i915/execlists: Move clearing submission count from reset to init Chris Wilson
@ 2016-10-03 13:07   ` Mika Kuoppala
  0 siblings, 0 replies; 22+ messages in thread
From: Mika Kuoppala @ 2016-10-03 13:07 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> After a GPU reset, we want to replay our queue of requests. However, the
> GPU reset clobbered the state and we only fixup the state for the
> guitly

s/guitly/guilty.

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

> request - and engines deemed innocent we try to leave untouched so that
> we recover as completely as possible. However, we need to clear the sw
> tracking of the ELSP ports even for innocent requests, so move the clear
> to the common path of init_hw (from reset_hw).
>
> Reported-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0ea992ba2723..936f6f63f626 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1242,8 +1242,12 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>  
>  	intel_engine_init_hangcheck(engine);
>  
> -	if (!execlists_elsp_idle(engine))
> +	/* After a GPU reset, we may have requests to replay */
> +	if (!execlists_elsp_idle(engine)) {
> +		engine->execlist_port[0].count = 0;
> +		engine->execlist_port[1].count = 0;
>  		execlists_submit_ports(engine);
> +	}
>  
>  	return 0;
>  }
> @@ -1318,10 +1322,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
>  		memset(&port[1], 0, sizeof(port[1]));
>  	}
>  
> -	/* CS is stopped, and we will resubmit both ports on resume */
>  	GEM_BUG_ON(request->ctx != port[0].request->ctx);
> -	port[0].count = 0;
> -	port[1].count = 0;
>  
>  	/* Reset WaIdleLiteRestore:bdw,skl as well */
>  	request->tail = request->wa_tail - WA_TAIL_DWORDS * sizeof(u32);
> -- 
> 2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 4/8] drm/i915: Disable irqs across GPU reset
  2016-10-03 12:52 ` [PATCH 4/8] drm/i915: Disable irqs across GPU reset Chris Wilson
@ 2016-10-03 13:09   ` Mika Kuoppala
  0 siblings, 0 replies; 22+ messages in thread
From: Mika Kuoppala @ 2016-10-03 13:09 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> Whilst we reset the GPU, we want to prevent execlists from submitting
> new work (which it does via an interrupt handler). To achieve this we
> disable the irq (and drain the irq tasklet) around the reset. When we
> enable it again afters, the interrupt queue should be empty and we can
> reinitialise from a known state without fear of the tasklet running
> concurrently.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>

Restamping

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

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem.c |  2 --
>  2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 31b2b6300d8d..7ce498898217 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1728,6 +1728,21 @@ int i915_resume_switcheroo(struct drm_device *dev)
>  	return i915_drm_resume(dev);
>  }
>  
> +static void disable_engines_irq(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_engine_cs *engine;
> +
> +	/* Ensure irq handler finishes, and not run again. */
> +	disable_irq(dev_priv->drm.irq);
> +	for_each_engine(engine, dev_priv)
> +		tasklet_kill(&engine->irq_tasklet);
> +}
> +
> +static void enable_engines_irq(struct drm_i915_private *dev_priv)
> +{
> +	enable_irq(dev_priv->drm.irq);
> +}
> +
>  /**
>   * i915_reset - reset chip after a hang
>   * @dev: drm device to reset
> @@ -1761,7 +1776,11 @@ void i915_reset(struct drm_i915_private *dev_priv)
>  	error->reset_count++;
>  
>  	pr_notice("drm/i915: Resetting chip after gpu hang\n");
> +
> +	disable_engines_irq(dev_priv);
>  	ret = intel_gpu_reset(dev_priv, ALL_ENGINES);
> +	enable_engines_irq(dev_priv);
> +
>  	if (ret) {
>  		if (ret != -ENODEV)
>  			DRM_ERROR("Failed to reset chip: %i\n", ret);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1418c1c522cb..0cae8acdf906 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2581,8 +2581,6 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  	struct i915_gem_context *incomplete_ctx;
>  	bool ring_hung;
>  
> -	/* Ensure irq handler finishes, and not run again. */
> -	tasklet_kill(&engine->irq_tasklet);
>  	if (engine->irq_seqno_barrier)
>  		engine->irq_seqno_barrier(engine);
>  
> -- 
> 2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 5/8] drm/i915: Double check hangcheck.seqno after reset
  2016-10-03 12:52 ` [PATCH 5/8] drm/i915: Double check hangcheck.seqno after reset Chris Wilson
@ 2016-10-03 13:14   ` Mika Kuoppala
  2016-10-03 14:01     ` Chris Wilson
  0 siblings, 1 reply; 22+ messages in thread
From: Mika Kuoppala @ 2016-10-03 13:14 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> Check that there was not a late recovery between us declaring the GPU
> hung and processing the reset. If the GPU did recover by itself, let the
> request remain on the active list and see if it hangs again!
>

Did you see this in action? Makes sense to recheck
after reset. I don't remember how TDR will deal with multiple
reset on the same engine but we should start tracking the seqno
that cause it and make sure we don't get stuck by replaying the same.

Do we check the banning on resubmission and/or do we trust that
the breadcrumb update always succeedes?

I envision that if we get multiple resets on same seqno, we
just write the breadcrumbs through cpu and move on. But let's
hope we don't need to and the gpu breadcrumps are always enough.

Regardless, it's improvement and should weed out false positives
on some hangs.

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

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0cae8acdf906..a89a88922448 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2589,6 +2589,9 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  		return;
>  
>  	ring_hung = engine->hangcheck.score >= HANGCHECK_SCORE_RING_HUNG;
> +	if (engine->hangcheck.seqno != intel_engine_get_seqno(engine))
> +		ring_hung = false;
> +
>  	i915_set_reset_status(request->ctx, ring_hung);
>  	if (!ring_hung)
>  		return;
> -- 
> 2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 5/8] drm/i915: Double check hangcheck.seqno after reset
  2016-10-03 13:14   ` Mika Kuoppala
@ 2016-10-03 14:01     ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2016-10-03 14:01 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

On Mon, Oct 03, 2016 at 04:14:39PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Check that there was not a late recovery between us declaring the GPU
> > hung and processing the reset. If the GPU did recover by itself, let the
> > request remain on the active list and see if it hangs again!
> >
> 
> Did you see this in action? Makes sense to recheck
> after reset. I don't remember how TDR will deal with multiple
> reset on the same engine but we should start tracking the seqno
> that cause it and make sure we don't get stuck by replaying the same.

Not observed. All my testing is with true hangs, not slow batches at
~hangcheck interval, i.e. nothing that would unstick between us
detecting the hang and sending the reset request.
 
> Do we check the banning on resubmission and/or do we trust that
> the breadcrumb update always succeedes?

Breadcrumb update has to succeed as we are not the only observers.
Issuing it from the GPU is the only way we can do the reset without
shooting ourselves in the foot with nasty races (and general explosions
from use-after-free).
 
> I envision that if we get multiple resets on same seqno, we
> just write the breadcrumbs through cpu and move on. But let's
> hope we don't need to and the gpu breadcrumps are always enough.

Can't do. The only time we can take over with the GPU is if we declare
the machine wedged and drain everything and all observers (including
external). That is quite impractical. :|
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* ✗ Fi.CI.BAT: warning for series starting with [1/8] drm/i915: Share the computation of ring size for RING_CTL register
  2016-10-03 12:52 [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
                   ` (8 preceding siblings ...)
  2016-10-03 13:01 ` Mika Kuoppala
@ 2016-10-03 14:49 ` Patchwork
  9 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2016-10-03 14:49 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/8] drm/i915: Share the computation of ring size for RING_CTL register
URL   : https://patchwork.freedesktop.org/series/13231/
State : warning

== Summary ==

Series 13231v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/13231/revisions/1/mbox/

Test drv_module_reload_basic:
                skip       -> PASS       (fi-skl-6770hq)
Test kms_cursor_legacy:
        Subgroup basic-flip-after-cursor-varying-size:
                pass       -> DMESG-WARN (fi-byt-n2820)
        Subgroup basic-flip-before-cursor-varying-size:
                pass       -> DMESG-WARN (fi-bsw-n3050)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:201  dwarn:1   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:244  pass:214  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:244  pass:212  dwarn:0   dfail:0   fail:1   skip:31 
fi-byt-n2820     total:244  pass:207  dwarn:1   dfail:0   fail:1   skip:35 
fi-hsw-4770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ilk-650       total:244  pass:182  dwarn:0   dfail:0   fail:2   skip:60 
fi-ivb-3520m     total:244  pass:219  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6700k     total:244  pass:219  dwarn:1   dfail:0   fail:0   skip:24 
fi-skl-6770hq    total:244  pass:228  dwarn:1   dfail:0   fail:1   skip:14 
fi-snb-2520m     total:244  pass:208  dwarn:0   dfail:0   fail:0   skip:36 
fi-snb-2600      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_2614/

71d126590e2fa6d65d93fe3586d55ddf9f6c39a6 drm-intel-nightly: 2016y-10m-03d-13h-22m-56s UTC integration manifest
eac1ec0 drm/i915: Show waiters in i915_hangcheck_info
32b82cc drm/i915: Show RING registers through debugfs
884a554 drm/i915: Show bounds of active request in the ring on GPU hang
cd5fc2b drm/i915: Double check hangcheck.seqno after reset
cd8a938 drm/i915: Disable irqs across GPU reset
51bba8d drm/i915/execlists: Move clearing submission count from reset to init
aeb2793 drm/i915/execlists: Reinitialise context image after GPU hang
fb86a24 drm/i915: Share the computation of ring size for RING_CTL register

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

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

* Re: [PATCH 6/8] drm/i915: Show bounds of active request in the ring on GPU hang
  2016-10-03 12:52 ` [PATCH 6/8] drm/i915: Show bounds of active request in the ring on GPU hang Chris Wilson
@ 2016-10-04 11:56   ` Mika Kuoppala
  0 siblings, 0 replies; 22+ messages in thread
From: Mika Kuoppala @ 2016-10-04 11:56 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> Include the position of the active request in the ring, and display that
> alongside the current RING registers (on a GPU hang).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

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

> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       | 3 +++
>  drivers/gpu/drm/i915/i915_gpu_error.c | 9 +++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 91ac283d733e..e48044b3a447 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -776,6 +776,9 @@ struct drm_i915_error_state {
>  		struct i915_address_space *vm;
>  		int num_requests;
>  
> +		/* position of active request inside the ring */
> +		u32 rq_head, rq_post, rq_tail;
> +
>  		/* our own tracking of ring head and tail */
>  		u32 cpu_ring_head;
>  		u32 cpu_ring_tail;
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 2bbab226a46c..8b85efbdfa04 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -262,8 +262,9 @@ static void error_print_engine(struct drm_i915_error_state_buf *m,
>  {
>  	err_printf(m, "%s command stream:\n", engine_str(ee->engine_id));
>  	err_printf(m, "  START: 0x%08x\n", ee->start);
> -	err_printf(m, "  HEAD:  0x%08x\n", ee->head);
> -	err_printf(m, "  TAIL:  0x%08x\n", ee->tail);
> +	err_printf(m, "  HEAD:  0x%08x\n [0x%08x]", ee->head, ee->rq_head);
> +	err_printf(m, "  TAIL:  0x%08x [0x%08x, 0x%08x]\n",
> +		   ee->tail, ee->rq_post, ee->rq_tail);
>  	err_printf(m, "  CTL:   0x%08x\n", ee->ctl);
>  	err_printf(m, "  MODE:  0x%08x\n", ee->mode);
>  	err_printf(m, "  HWS:   0x%08x\n", ee->hws);
> @@ -1230,6 +1231,10 @@ static void i915_gem_record_rings(struct drm_i915_private *dev_priv,
>  			error->simulated |=
>  				request->ctx->flags & CONTEXT_NO_ERROR_CAPTURE;
>  
> +			ee->rq_head = request->head;
> +			ee->rq_post = request->postfix;
> +			ee->rq_tail = request->tail;
> +
>  			ring = request->ring;
>  			ee->cpu_ring_head = ring->head;
>  			ee->cpu_ring_tail = ring->tail;
> -- 
> 2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 7/8] drm/i915: Show RING registers through debugfs
  2016-10-03 12:52 ` [PATCH 7/8] drm/i915: Show RING registers through debugfs Chris Wilson
@ 2016-10-04 12:35   ` Mika Kuoppala
  2016-10-04 13:11     ` Chris Wilson
  0 siblings, 1 reply; 22+ messages in thread
From: Mika Kuoppala @ 2016-10-04 12:35 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> Knowing where the RINGs are pointing is extremely useful in diagnosing
> if the engines are executing the ringbuffers you expect - and igt may be
> suppressing the usual method of looking in the GPU error state.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     | 230 +++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_engine_cs.c  |  30 +++++
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  16 ---
>  drivers/gpu/drm/i915/intel_ringbuffer.h |   2 +
>  4 files changed, 170 insertions(+), 108 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 4fb9d829884e..9c95ce73f2aa 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -635,6 +635,23 @@ static int i915_gem_batch_pool_info(struct seq_file *m, void *data)
>  	return 0;
>  }
>  
> +static void print_request(struct seq_file *m,
> +			  struct drm_i915_gem_request *rq,
> +			  const char *prefix)
> +{
> +	struct pid *pid = rq->ctx->pid;
> +	struct task_struct *task;
> +
> +	rcu_read_lock();
> +	task = pid ? pid_task(pid, PIDTYPE_PID) : NULL;
> +	seq_printf(m, "%s%x [%x:%x] @ %d: %s [%d]\n", prefix,
> +		   rq->fence.seqno, rq->ctx->hw_id, rq->fence.seqno,
> +		   jiffies_to_msecs(jiffies - rq->emitted_jiffies),
> +		   task ? task->comm : "<unknown>",
> +		   task ? task->pid : -1);
> +	rcu_read_unlock();
> +}
> +
>  static int i915_gem_request_info(struct seq_file *m, void *data)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -658,19 +675,8 @@ static int i915_gem_request_info(struct seq_file *m, void *data)
>  			continue;
>  
>  		seq_printf(m, "%s requests: %d\n", engine->name, count);
> -		list_for_each_entry(req, &engine->request_list, link) {
> -			struct pid *pid = req->ctx->pid;
> -			struct task_struct *task;
> -
> -			rcu_read_lock();
> -			task = pid ? pid_task(pid, PIDTYPE_PID) : NULL;
> -			seq_printf(m, "    %x @ %d: %s [%d]\n",
> -				   req->fence.seqno,
> -				   (int) (jiffies - req->emitted_jiffies),
> -				   task ? task->comm : "<unknown>",
> -				   task ? task->pid : -1);
> -			rcu_read_unlock();
> -		}
> +		list_for_each_entry(req, &engine->request_list, link)
> +			print_request(m, req, "    ");
>  
>  		any++;
>  	}
> @@ -2036,84 +2042,6 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
>  	return 0;
>  }
>  
> -static int i915_execlists(struct seq_file *m, void *data)
> -{
> -	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> -	struct drm_device *dev = &dev_priv->drm;
> -	struct intel_engine_cs *engine;
> -	u32 status_pointer;
> -	u8 read_pointer;
> -	u8 write_pointer;
> -	u32 status;
> -	u32 ctx_id;
> -	struct list_head *cursor;
> -	int i, ret;
> -
> -	if (!i915.enable_execlists) {
> -		seq_puts(m, "Logical Ring Contexts are disabled\n");
> -		return 0;
> -	}
> -
> -	ret = mutex_lock_interruptible(&dev->struct_mutex);
> -	if (ret)
> -		return ret;
> -
> -	intel_runtime_pm_get(dev_priv);
> -
> -	for_each_engine(engine, dev_priv) {
> -		struct drm_i915_gem_request *head_req = NULL;
> -		int count = 0;
> -
> -		seq_printf(m, "%s\n", engine->name);
> -
> -		status = I915_READ(RING_EXECLIST_STATUS_LO(engine));
> -		ctx_id = I915_READ(RING_EXECLIST_STATUS_HI(engine));
> -		seq_printf(m, "\tExeclist status: 0x%08X, context: %u\n",
> -			   status, ctx_id);
> -
> -		status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(engine));
> -		seq_printf(m, "\tStatus pointer: 0x%08X\n", status_pointer);
> -
> -		read_pointer = GEN8_CSB_READ_PTR(status_pointer);
> -		write_pointer = GEN8_CSB_WRITE_PTR(status_pointer);
> -		if (read_pointer > write_pointer)
> -			write_pointer += GEN8_CSB_ENTRIES;
> -		seq_printf(m, "\tRead pointer: 0x%08X, write pointer 0x%08X\n",
> -			   read_pointer, write_pointer);
> -
> -		for (i = 0; i < GEN8_CSB_ENTRIES; i++) {
> -			status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(engine, i));
> -			ctx_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(engine, i));
> -
> -			seq_printf(m, "\tStatus buffer %d: 0x%08X, context: %u\n",
> -				   i, status, ctx_id);
> -		}
> -
> -		spin_lock_bh(&engine->execlist_lock);
> -		list_for_each(cursor, &engine->execlist_queue)
> -			count++;
> -		head_req = list_first_entry_or_null(&engine->execlist_queue,
> -						    struct drm_i915_gem_request,
> -						    execlist_link);
> -		spin_unlock_bh(&engine->execlist_lock);
> -
> -		seq_printf(m, "\t%d requests in queue\n", count);
> -		if (head_req) {
> -			seq_printf(m, "\tHead request context: %u\n",
> -				   head_req->ctx->hw_id);
> -			seq_printf(m, "\tHead request tail: %u\n",
> -				   head_req->tail);
> -		}
> -
> -		seq_putc(m, '\n');
> -	}
> -
> -	intel_runtime_pm_put(dev_priv);
> -	mutex_unlock(&dev->struct_mutex);
> -
> -	return 0;
> -}
> -
>  static const char *swizzle_string(unsigned swizzle)
>  {
>  	switch (swizzle) {
> @@ -3135,6 +3063,124 @@ static int i915_display_info(struct seq_file *m, void *unused)
>  	return 0;
>  }
>  
> +static int i915_engine_info(struct seq_file *m, void *unused)
> +{
> +	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> +	struct intel_engine_cs *engine;
> +
> +	for_each_engine(engine, dev_priv) {
> +		struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +		struct drm_i915_gem_request *rq;
> +		struct rb_node *rb;
> +		u64 addr;
> +		int count;
> +
> +		seq_printf(m, "%s\n", engine->name);
> +		seq_printf(m, "\tcurrent seqno %x, last %x, hangcheck %x [score %d]\n",
> +			   intel_engine_get_seqno(engine),
> +			   engine->last_submitted_seqno,
> +			   engine->hangcheck.seqno,
> +			   engine->hangcheck.score);
> +
> +		rcu_read_lock();
> +
> +		count = 0;
> +		list_for_each_entry(rq, &engine->request_list, link)
> +			count++;
> +		seq_printf(m, "\t%d submitted requests\n", count);
> +
> +		if (count > 0) {
> +			rq = list_first_entry(&engine->request_list,
> +					      struct drm_i915_gem_request, link);
> +			print_request(m, rq, "\t\tfirst  ");
> +
> +			rq = list_last_entry(&engine->request_list,
> +					     struct drm_i915_gem_request, link);
> +			print_request(m, rq, "\t\tlast   ");
> +		}
> +
> +		rq = i915_gem_find_active_request(engine);
> +		if (rq) {
> +			print_request(m, rq, "\t\tactive ");
> +			seq_printf(m,
> +				   "\t\t[head %04x, postfix %04x, tail %04x\n, batch 0x%08x_%08x]",
> +				   rq->head, rq->postfix, rq->tail,
> +				   rq->batch ? upper_32_bits(rq->batch->node.start) : ~0u,
> +				   rq->batch ? lower_32_bits(rq->batch->node.start) : ~0u);
> +		}
> +
> +		seq_printf(m, "\tRING_START: 0x%08x [0x%08x]\n",
> +			   I915_READ(RING_START(engine->mmio_base)),
> +			   rq ? i915_ggtt_offset(rq->ring->vma) : 0);
> +		seq_printf(m, "\tRING_HEAD:  0x%04x [0x%04x]\n",
> +			   I915_READ(RING_HEAD(engine->mmio_base)) & HEAD_ADDR,
> +			   rq ? rq->ring->head : 0);
> +		seq_printf(m, "\tRING_TAIL:  0x%04x [0x%04x]\n",
> +			   I915_READ(RING_TAIL(engine->mmio_base)) & TAIL_ADDR,
> +			   rq ? rq->ring->tail : 0);
> +		seq_printf(m, "\tRING_CTL:   0x%08x [%s]\n",
> +			   I915_READ(RING_CTL(engine->mmio_base)),
> +			   I915_READ(RING_CTL(engine->mmio_base)) & (RING_WAIT | RING_WAIT_SEMAPHORE) ? "waiting" : "");
> +
> +		rcu_read_unlock();
> +
> +		addr = intel_engine_get_active_head(engine);
> +		seq_printf(m, "\tACTHD:  0x%08x_%08x\n",
> +			   upper_32_bits(addr), lower_32_bits(addr));
> +		addr = intel_engine_get_last_batch_head(engine);
> +		seq_printf(m, "\tBBADDR: 0x%08x_%08x\n",
> +			   upper_32_bits(addr), lower_32_bits(addr));
> +
> +		if (i915.enable_execlists) {
> +			u32 ptr, read, write;
>

Ok, in here I am thinking that the requests in ports would be intresting
also. But it seems impossible to sample the ports without taking
locks. And we don't want to go there.

One option would be to store only the req->seqno of each port away during
elsp write. Perhaps with the reset count number. That would allow
us to see what was the last hw kick and it's timeline wrt to reset
and the request list. That would have a small perf impact tho.

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

> +			seq_printf(m, "\tExeclist status: 0x%08x %08x\n",
> +				   I915_READ(RING_EXECLIST_STATUS_LO(engine)),
> +				   I915_READ(RING_EXECLIST_STATUS_HI(engine)));
> +
> +			ptr = I915_READ(RING_CONTEXT_STATUS_PTR(engine));
> +			read = GEN8_CSB_READ_PTR(ptr);
> +			write = GEN8_CSB_WRITE_PTR(ptr);
> +			seq_printf(m, "\tExeclist CSB read %d, write %d\n",
> +				   read, write);
> +			if (read >= GEN8_CSB_ENTRIES)
> +				read = 0;
> +			if (write >= GEN8_CSB_ENTRIES)
> +				write = 0;
> +			if (read > write)
> +				write += GEN8_CSB_ENTRIES;
> +			while (read < write) {
> +				unsigned int idx = ++read % GEN8_CSB_ENTRIES;
> +
> +				seq_printf(m, "\tExeclist CSB[%d]: 0x%08x, context: %d\n",
> +					   idx,
> +					   I915_READ(RING_CONTEXT_STATUS_BUF_LO(engine, idx)),
> +					   I915_READ(RING_CONTEXT_STATUS_BUF_HI(engine, idx)));
> +			}
> +		} else if (INTEL_GEN(dev_priv) > 6) {
> +			seq_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
> +				   I915_READ(RING_PP_DIR_BASE(engine)));
> +			seq_printf(m, "\tPP_DIR_BASE_READ: 0x%08x\n",
> +				   I915_READ(RING_PP_DIR_BASE_READ(engine)));
> +			seq_printf(m, "\tPP_DIR_DCLV: 0x%08x\n",
> +				   I915_READ(RING_PP_DIR_DCLV(engine)));
> +		}
> +
> +		spin_lock(&b->lock);
> +		for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
> +			struct intel_wait *w = container_of(rb, typeof(*w), node);
> +
> +			seq_printf(m, "\t%s [%d] waiting for %x\n",
> +				   w->tsk->comm, w->tsk->pid, w->seqno);
> +		}
> +		spin_unlock(&b->lock);
> +
> +		seq_puts(m, "\n");
> +	}
> +
> +	return 0;
> +}
> +
>  static int i915_semaphore_status(struct seq_file *m, void *unused)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -4441,7 +4487,6 @@ static const struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_gem_framebuffer", i915_gem_framebuffer_info, 0},
>  	{"i915_context_status", i915_context_status, 0},
>  	{"i915_dump_lrc", i915_dump_lrc, 0},
> -	{"i915_execlists", i915_execlists, 0},
>  	{"i915_forcewake_domains", i915_forcewake_domains, 0},
>  	{"i915_swizzle_info", i915_swizzle_info, 0},
>  	{"i915_ppgtt_info", i915_ppgtt_info, 0},
> @@ -4453,6 +4498,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_power_domain_info", i915_power_domain_info, 0},
>  	{"i915_dmc_info", i915_dmc_info, 0},
>  	{"i915_display_info", i915_display_info, 0},
> +	{"i915_engine_info", i915_engine_info, 0},
>  	{"i915_semaphore_status", i915_semaphore_status, 0},
>  	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
>  	{"i915_dp_mst_info", i915_dp_mst_info, 0},
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index e405f1080296..d00ec805f93d 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -334,3 +334,33 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
>  	intel_engine_cleanup_cmd_parser(engine);
>  	i915_gem_batch_pool_fini(&engine->batch_pool);
>  }
> +
> +u64 intel_engine_get_active_head(struct intel_engine_cs *engine)
> +{
> +	struct drm_i915_private *dev_priv = engine->i915;
> +	u64 acthd;
> +
> +	if (INTEL_GEN(dev_priv) >= 8)
> +		acthd = I915_READ64_2x32(RING_ACTHD(engine->mmio_base),
> +					 RING_ACTHD_UDW(engine->mmio_base));
> +	else if (INTEL_GEN(dev_priv) >= 4)
> +		acthd = I915_READ(RING_ACTHD(engine->mmio_base));
> +	else
> +		acthd = I915_READ(ACTHD);
> +
> +	return acthd;
> +}
> +
> +u64 intel_engine_get_last_batch_head(struct intel_engine_cs *engine)
> +{
> +	struct drm_i915_private *dev_priv = engine->i915;
> +	u64 bbaddr;
> +
> +	if (INTEL_GEN(dev_priv) >= 8)
> +		bbaddr = I915_READ64_2x32(RING_BBADDR(engine->mmio_base),
> +					  RING_BBADDR_UDW(engine->mmio_base));
> +	else
> +		bbaddr = I915_READ(RING_BBADDR(engine->mmio_base));
> +
> +	return bbaddr;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 26aa4c5e268f..87f4a2666c4e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -405,22 +405,6 @@ gen8_render_ring_flush(struct drm_i915_gem_request *req, u32 mode)
>  	return gen8_emit_pipe_control(req, flags, scratch_addr);
>  }
>  
> -u64 intel_engine_get_active_head(struct intel_engine_cs *engine)
> -{
> -	struct drm_i915_private *dev_priv = engine->i915;
> -	u64 acthd;
> -
> -	if (INTEL_GEN(dev_priv) >= 8)
> -		acthd = I915_READ64_2x32(RING_ACTHD(engine->mmio_base),
> -					 RING_ACTHD_UDW(engine->mmio_base));
> -	else if (INTEL_GEN(dev_priv) >= 4)
> -		acthd = I915_READ(RING_ACTHD(engine->mmio_base));
> -	else
> -		acthd = I915_READ(ACTHD);
> -
> -	return acthd;
> -}
> -
>  static void ring_setup_phys_status_page(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 66553bdb0a47..498931f0b1f1 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -541,6 +541,8 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine);
>  int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine);
>  
>  u64 intel_engine_get_active_head(struct intel_engine_cs *engine);
> +u64 intel_engine_get_last_batch_head(struct intel_engine_cs *engine);
> +
>  static inline u32 intel_engine_get_seqno(struct intel_engine_cs *engine)
>  {
>  	return intel_read_status_page(engine, I915_GEM_HWS_INDEX);
> -- 
> 2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 8/8] drm/i915: Show waiters in i915_hangcheck_info
  2016-10-03 12:52 ` [PATCH 8/8] drm/i915: Show waiters in i915_hangcheck_info Chris Wilson
@ 2016-10-04 12:41   ` Mika Kuoppala
  2016-10-04 13:07     ` Chris Wilson
  2016-10-04 13:22   ` Mika Kuoppala
  1 sibling, 1 reply; 22+ messages in thread
From: Mika Kuoppala @ 2016-10-04 12:41 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

The commit message is missing.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9c95ce73f2aa..39b76c24c84f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1343,6 +1343,9 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>  		seq_printf(m, "Hangcheck inactive\n");
>  
>  	for_each_engine_id(engine, dev_priv, id) {
> +		struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +		struct rb_node *rb;
> +
>  		seq_printf(m, "%s:\n", engine->name);
>  		seq_printf(m, "\tseqno = %x [current %x, last %x]\n",
>  			   engine->hangcheck.seqno,
> @@ -1352,6 +1355,15 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>  			   yesno(intel_engine_has_waiter(engine)),
>  			   yesno(test_bit(engine->id,
>  					  &dev_priv->gpu_error.missed_irq_rings)));
> +		spin_lock(&b->lock);
> +		for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {

The waiters are embeddded in requests? So we need the rcu_read_lock?

-Mika


> +			struct intel_wait *w = container_of(rb, typeof(*w), node);
> +
> +			seq_printf(m, "\t%s [%d] waiting for %x\n",
> +				   w->tsk->comm, w->tsk->pid, w->seqno);
> +		}
> +		spin_unlock(&b->lock);
> +
>  		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
>  			   (long long)engine->hangcheck.acthd,
>  			   (long long)acthd[id]);
> -- 
> 2.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register
  2016-10-03 13:00 ` [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
@ 2016-10-04 12:51   ` Mika Kuoppala
  0 siblings, 0 replies; 22+ messages in thread
From: Mika Kuoppala @ 2016-10-04 12:51 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> On Mon, Oct 03, 2016 at 01:52:39PM +0100, Chris Wilson wrote:
>> Since both legacy and execlists want to poopulate the RING_CTL register,
>> share the computation of the right bits for the ring->size. We can then
>> stop masking errors and explicitly forbid them during creation!
>> 
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h         | 1 +
>>  drivers/gpu/drm/i915/intel_lrc.c        | 2 +-
>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 5 ++---
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 8d44cee710f0..acc767a52d8e 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1605,6 +1605,7 @@ enum skl_disp_power_wells {
>>  #define RING_HEAD(base)		_MMIO((base)+0x34)
>>  #define RING_START(base)	_MMIO((base)+0x38)
>>  #define RING_CTL(base)		_MMIO((base)+0x3c)
>> +#define   RING_CTL_SIZE(size)	((size) - PAGE_SIZE) /* in bytes -> pages */
>>  #define RING_SYNC_0(base)	_MMIO((base)+0x40)
>>  #define RING_SYNC_1(base)	_MMIO((base)+0x44)
>>  #define RING_SYNC_2(base)	_MMIO((base)+0x48)
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 2d8eb2eb2b72..5ede272eb4d2 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1946,7 +1946,7 @@ populate_lr_context(struct i915_gem_context *ctx,
>>  		       RING_START(engine->mmio_base), 0);
>>  	ASSIGN_CTX_REG(reg_state, CTX_RING_BUFFER_CONTROL,
>>  		       RING_CTL(engine->mmio_base),
>> -		       ((ring->size - PAGE_SIZE) & RING_NR_PAGES) | RING_VALID);
>> +		       RING_CTL_SIZE(ring->size) | RING_VALID);
>>  	ASSIGN_CTX_REG(reg_state, CTX_BB_HEAD_U,
>>  		       RING_BBADDR_UDW(engine->mmio_base), 0);
>>  	ASSIGN_CTX_REG(reg_state, CTX_BB_HEAD_L,
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 67ea9dd5921e..26aa4c5e268f 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -585,9 +585,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
>>  	I915_WRITE_TAIL(engine, ring->tail);
>>  	(void)I915_READ_TAIL(engine);
>>  
>> -	I915_WRITE_CTL(engine,
>> -			((ring->size - PAGE_SIZE) & RING_NR_PAGES)
>> -			| RING_VALID);
>> +	I915_WRITE_CTL(engine, RING_CTL_SIZE(ring->size) | RING_VALID);
>>  
>>  	/* If the head is still not zero, the ring is dead */
>>  	if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
>> @@ -1951,6 +1949,7 @@ intel_engine_create_ring(struct intel_engine_cs *engine, int size)
>>  	struct i915_vma *vma;
>>  
>>  	GEM_BUG_ON(!is_power_of_2(size));
>> +	GEM_BUG_ON(size & ~RING_NR_PAGES);
>
> (size - PAGE_SIZE) & ~RING_NR_PAGES

Oops. Or even RING_CTL_SIZE(size) & ~RING_NR_PAGES.

-Mika

> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 8/8] drm/i915: Show waiters in i915_hangcheck_info
  2016-10-04 12:41   ` Mika Kuoppala
@ 2016-10-04 13:07     ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2016-10-04 13:07 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

On Tue, Oct 04, 2016 at 03:41:24PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> The commit message is missing.
> 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 9c95ce73f2aa..39b76c24c84f 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1343,6 +1343,9 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
> >  		seq_printf(m, "Hangcheck inactive\n");
> >  
> >  	for_each_engine_id(engine, dev_priv, id) {
> > +		struct intel_breadcrumbs *b = &engine->breadcrumbs;
> > +		struct rb_node *rb;
> > +
> >  		seq_printf(m, "%s:\n", engine->name);
> >  		seq_printf(m, "\tseqno = %x [current %x, last %x]\n",
> >  			   engine->hangcheck.seqno,
> > @@ -1352,6 +1355,15 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
> >  			   yesno(intel_engine_has_waiter(engine)),
> >  			   yesno(test_bit(engine->id,
> >  					  &dev_priv->gpu_error.missed_irq_rings)));
> > +		spin_lock(&b->lock);
> > +		for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
> 
> The waiters are embeddded in requests? So we need the rcu_read_lock?

spin_lock outweighs rcu_read_lock, but the waiters are in a separate
per-engine list guarded by the breadcrumbs.lock, we need the spin lock
here.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 7/8] drm/i915: Show RING registers through debugfs
  2016-10-04 12:35   ` Mika Kuoppala
@ 2016-10-04 13:11     ` Chris Wilson
  0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2016-10-04 13:11 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

On Tue, Oct 04, 2016 at 03:35:20PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> > +		rcu_read_unlock();
> > +
> > +		addr = intel_engine_get_active_head(engine);
> > +		seq_printf(m, "\tACTHD:  0x%08x_%08x\n",
> > +			   upper_32_bits(addr), lower_32_bits(addr));
> > +		addr = intel_engine_get_last_batch_head(engine);
> > +		seq_printf(m, "\tBBADDR: 0x%08x_%08x\n",
> > +			   upper_32_bits(addr), lower_32_bits(addr));
> > +
> > +		if (i915.enable_execlists) {
> > +			u32 ptr, read, write;
> >
> 
> Ok, in here I am thinking that the requests in ports would be intresting
> also. But it seems impossible to sample the ports without taking
> locks. And we don't want to go there.

It's even worse than that. execlist_port[] are entirely unlocked and
only updated by the sole owner. However... We can read them under the
rcu guarantee that the requests themselves will not disappear (with the
exception that the contents may be reallocated - but not explode!).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 8/8] drm/i915: Show waiters in i915_hangcheck_info
  2016-10-03 12:52 ` [PATCH 8/8] drm/i915: Show waiters in i915_hangcheck_info Chris Wilson
  2016-10-04 12:41   ` Mika Kuoppala
@ 2016-10-04 13:22   ` Mika Kuoppala
  1 sibling, 0 replies; 22+ messages in thread
From: Mika Kuoppala @ 2016-10-04 13:22 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

With commit message stating the obvious added,

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

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9c95ce73f2aa..39b76c24c84f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1343,6 +1343,9 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>  		seq_printf(m, "Hangcheck inactive\n");
>  
>  	for_each_engine_id(engine, dev_priv, id) {
> +		struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +		struct rb_node *rb;
> +
>  		seq_printf(m, "%s:\n", engine->name);
>  		seq_printf(m, "\tseqno = %x [current %x, last %x]\n",
>  			   engine->hangcheck.seqno,
> @@ -1352,6 +1355,15 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>  			   yesno(intel_engine_has_waiter(engine)),
>  			   yesno(test_bit(engine->id,
>  					  &dev_priv->gpu_error.missed_irq_rings)));
> +		spin_lock(&b->lock);
> +		for (rb = rb_first(&b->waiters); rb; rb = rb_next(rb)) {
> +			struct intel_wait *w = container_of(rb, typeof(*w), node);
> +
> +			seq_printf(m, "\t%s [%d] waiting for %x\n",
> +				   w->tsk->comm, w->tsk->pid, w->seqno);
> +		}
> +		spin_unlock(&b->lock);
> +
>  		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
>  			   (long long)engine->hangcheck.acthd,
>  			   (long long)acthd[id]);
> -- 
> 2.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

end of thread, other threads:[~2016-10-04 13:23 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-03 12:52 [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
2016-10-03 12:52 ` [PATCH 2/8] drm/i915/execlists: Reinitialise context image after GPU hang Chris Wilson
2016-10-03 12:52 ` [PATCH 3/8] drm/i915/execlists: Move clearing submission count from reset to init Chris Wilson
2016-10-03 13:07   ` Mika Kuoppala
2016-10-03 12:52 ` [PATCH 4/8] drm/i915: Disable irqs across GPU reset Chris Wilson
2016-10-03 13:09   ` Mika Kuoppala
2016-10-03 12:52 ` [PATCH 5/8] drm/i915: Double check hangcheck.seqno after reset Chris Wilson
2016-10-03 13:14   ` Mika Kuoppala
2016-10-03 14:01     ` Chris Wilson
2016-10-03 12:52 ` [PATCH 6/8] drm/i915: Show bounds of active request in the ring on GPU hang Chris Wilson
2016-10-04 11:56   ` Mika Kuoppala
2016-10-03 12:52 ` [PATCH 7/8] drm/i915: Show RING registers through debugfs Chris Wilson
2016-10-04 12:35   ` Mika Kuoppala
2016-10-04 13:11     ` Chris Wilson
2016-10-03 12:52 ` [PATCH 8/8] drm/i915: Show waiters in i915_hangcheck_info Chris Wilson
2016-10-04 12:41   ` Mika Kuoppala
2016-10-04 13:07     ` Chris Wilson
2016-10-04 13:22   ` Mika Kuoppala
2016-10-03 13:00 ` [PATCH 1/8] drm/i915: Share the computation of ring size for RING_CTL register Chris Wilson
2016-10-04 12:51   ` Mika Kuoppala
2016-10-03 13:01 ` Mika Kuoppala
2016-10-03 14:49 ` ✗ Fi.CI.BAT: warning for series starting with [1/8] " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.