drm/i915: Initialize seqno for VECS too

PATCH In review Ben Widawsky 3 participants 5 messages Aug 12, 2013 – Aug 13, 2013

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Ben Widawsky

author

2013-08-12 23:54 UTC #0 lore raw

PATCH 1/2 drm/i915: Initialize seqno for VECS too

Cc: Mika Kuoppala <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65387
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67198

Signed-off-by Ben Widawsky <[email protected]>

---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0

index 10c2aaa..665602f 100644
@@ -1592,6 +1592,8 @@ void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno)
 	if (INTEL_INFO(ring->dev)->gen >= 6) {
 		I915_WRITE(RING_SYNC_0(ring->mmio_base), 0);
 		I915_WRITE(RING_SYNC_1(ring->mmio_base), 0);
+		if (HAS_VEBOX(ring->dev))
+			I915_WRITE(RING_SYNC_2(ring->mmio_base), 0);
 	}
 
 	ring->set_seqno(ring, seqno);

Signature · 1 line

1.8.3.3

Ben Widawsky

author

2013-08-12 23:54 UTC #1 lore raw

PATCH 2/2 drm/i915: Get VECS semaphore info on error

Ideally we could use for_each_ring with the ring flags as I've done a couple times
(http://lists.freedesktop.org/archives/intel-gfx/2013-June/029450.html).
Until Daniel merges that patch though, we can just use this.

Cc Mika Kuoppala <[email protected]>

Signed-off-by Ben Widawsky <[email protected]>

---
 drivers/gpu/drm/i915/i915_gpu_error.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/gpu/drm/i915/i915_gpu_error.c +11 -0

index 60393cb..558e568 100644
@@ -243,6 +243,11 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m,
 		err_printf(m, "  SYNC_1: 0x%08x [last synced 0x%08x]\n",
 			   error->semaphore_mboxes[ring][1],
 			   error->semaphore_seqno[ring][1]);
+		if (HAS_VEBOX(dev)) {
+			err_printf(m, "  SYNC_2: 0x%08x [last synced 0x%08x]\n",
+				   error->semaphore_mboxes[ring][2],
+				   error->semaphore_seqno[ring][2]);
+		}
 	}
 	err_printf(m, "  seqno: 0x%08x\n", error->seqno[ring]);
 	err_printf(m, "  waiting: %s\n", yesno(error->waiting[ring]));

@@ -682,6 +687,12 @@ static void i915_record_ring_state(struct drm_device *dev,
 		error->semaphore_seqno[ring->id][1] = ring->sync_seqno[1];
 	}
 
+	if (HAS_VEBOX(dev)) {
+		error->semaphore_mboxes[ring->id][2] =
+			I915_READ(RING_SYNC_2(ring->mmio_base));
+		error->semaphore_seqno[ring->id][2] = ring->sync_seqno[2];
+	}
+
 	if (INTEL_INFO(dev)->gen >= 4) {
 		error->faddr[ring->id] = I915_READ(RING_DMA_FADD(ring->mmio_base));
 		error->ipeir[ring->id] = I915_READ(RING_IPEIR(ring->mmio_base));

Signature · 1 line

1.8.3.3

Daniel Vetter

Tested-by lu hua on v1 1/2

2013-08-13 07:02 UTC #2 lore raw

In reply to Ben Widawsky

Quoted 4 lines · Ben Widawsky

Cc: Mika Kuoppala <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65387
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67198
Signed-off-by Ben Widawsky <[email protected]>

Tested-by lu hua <[email protected]> (both bug reports)

Please pimp the commit message as discussion on irc (reply with just the text is ok). Strictly speaking we initialize the 3rd mbox for the 4th ring here (which ofc is the vebox one for all rings != vebox). And also please mention the effects of this bug, i.e. how it blows up.

Thanks, Daniel

Quoted 24 lines

---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0

index 10c2aaa..665602f 100644
@@ -1592,6 +1592,8 @@ void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno)
 	if (INTEL_INFO(ring->dev)->gen >= 6) {
 		I915_WRITE(RING_SYNC_0(ring->mmio_base), 0);
 		I915_WRITE(RING_SYNC_1(ring->mmio_base), 0);
+		if (HAS_VEBOX(ring->dev))
+			I915_WRITE(RING_SYNC_2(ring->mmio_base), 0);
 	}
 
 	ring->set_seqno(ring, seqno);

Signature · 6 lines

1.8.3.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Ben Widawsky

2013-08-13 20:55 UTC #3 lore raw

In reply to Ben Widawsky

Quoted 13 lines · Ben Widawsky

Cc: Mika Kuoppala <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65387
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67198
Signed-off-by Ben Widawsky <[email protected]>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0
index 10c2aaa..665602f 100644

drivers/gpu/drm/i915/intel_ringbuffer.c void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno) · line 1599

Quoted 8 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0

@@ -1592,6 +1592,8 @@ void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno)
 	if (INTEL_INFO(ring->dev)->gen >= 6) {
 		I915_WRITE(RING_SYNC_0(ring->mmio_base), 0);
 		I915_WRITE(RING_SYNC_1(ring->mmio_base), 0);
+		if (HAS_VEBOX(ring->dev))
+			I915_WRITE(RING_SYNC_2(ring->mmio_base), 0);
 	}
 
 	ring->set_seqno(ring, seqno);

We require n-1 mailboxes for proper semaphore synchronization. All semaphore synchronization code relies on proper values in these mailboxes. The fact that we failed to touch the vebox ring by itself was unlikely to be an issue since the HW should be initializing the values to 0. However the error framework for testing seqno wrap introduced by Mika, in addition to the hangcheck via seqno, and i915_error_first_batchbuffer() combined caused a nice explosion.

The problem is caused by seqno wrap because the wrap condition is not properly setup. The wrap code attempts to set the sync mailboxes all to 0, and then set the current seqno to one less than 0. In all cases, the vebox mailbox wasn't properly being initialized. This caused a wrap to not occur. When hangcheck kicks in with the bogus seqno values, the rest just doesn't work. It makes me wonder if we shouldn't consider a dumber version of hangcheck...

How we messed this up:
VECS support was written before the aforementioned other features. Upon VECS being rebased, these facts were missed.

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Daniel Vetter

2013-08-13 21:12 UTC #4 lore raw

Quoted 39 lines · Ben Widawsky

Quoted 13 lines · Ben Widawsky
Cc: Mika Kuoppala <[email protected]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65387
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67198
Signed-off-by Ben Widawsky <[email protected]>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0
index 10c2aaa..665602f 100644
drivers/gpu/drm/i915/intel_ringbuffer.c void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno) · line 1599
Quoted 8 lines
drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0
@@ -1592,6 +1592,8 @@ void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno)
 	if (INTEL_INFO(ring->dev)->gen >= 6) {
 		I915_WRITE(RING_SYNC_0(ring->mmio_base), 0);
 		I915_WRITE(RING_SYNC_1(ring->mmio_base), 0);
+		if (HAS_VEBOX(ring->dev))
+			I915_WRITE(RING_SYNC_2(ring->mmio_base), 0);
 	}
 
 	ring->set_seqno(ring, seqno);
We require n-1 mailboxes for proper semaphore synchronization. All semaphore synchronization code relies on proper values in these mailboxes. The fact that we failed to touch the vebox ring by itself was unlikely to be an issue since the HW should be initializing the values to 0. However the error framework for testing seqno wrap introduced by Mika, in addition to the hangcheck via seqno, and i915_error_first_batchbuffer() combined caused a nice explosion.
The problem is caused by seqno wrap because the wrap condition is not properly setup. The wrap code attempts to set the sync mailboxes all to 0, and then set the current seqno to one less than 0. In all cases, the vebox mailbox wasn't properly being initialized. This caused a wrap to not occur. When hangcheck kicks in with the bogus seqno values, the rest just doesn't work. It makes me wonder if we shouldn't consider a dumber version of hangcheck...

Quoted 3 lines

How we messed this up:
VECS support was written before the aforementioned other features. Upon VECS being rebased, these facts were missed.

Both patches applied, thanks.
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch