[PATCH 1/2] drm/i915: Dumb down the semaphore logic

[PATCH 1/2] drm/i915: Dumb down the semaphore logic

[Ben Widawsky]

While I think the previous code is correct, it was hard to follow and
hard to debug. Since we already have a ring abstraction, might as well
use it to handle the semaphore updates and compares.

I don't expect this code to make semaphores better or worse, but you
never know...

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    3 +-
 drivers/gpu/drm/i915/i915_reg.h            |    7 +
 drivers/gpu/drm/i915/intel_ringbuffer.c    |  176 +++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |    7 +-
 4 files changed, 145 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 4934cf8..3693e83 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -784,7 +784,8 @@ i915_gem_execbuffer_sync_rings(struct drm_i915_gem_object *obj,
 	}
 
 	from->sync_seqno[idx] = seqno;
-	return intel_ring_sync(to, from, seqno - 1);
+
+	return to->sync_to(to, from, seqno - 1);
 }
 
 static int
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 542453f..f0b5287 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -194,6 +194,13 @@
 #define  MI_SEMAPHORE_UPDATE	    (1<<21)
 #define  MI_SEMAPHORE_COMPARE	    (1<<20)
 #define  MI_SEMAPHORE_REGISTER	    (1<<18)
+#define  MI_SEMAPHORE_SYNC_RV	    (2<<16)
+#define  MI_SEMAPHORE_SYNC_RB	    (0<<16)
+#define  MI_SEMAPHORE_SYNC_VR	    (0<<16)
+#define  MI_SEMAPHORE_SYNC_VB	    (2<<16)
+#define  MI_SEMAPHORE_SYNC_BR	    (2<<16)
+#define  MI_SEMAPHORE_SYNC_BV	    (0<<16)
+#define  MI_SEMAPHORE_SYNC_INVALID  (1<<0)
 /*
  * 3D instructions used by the kernel
  */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c30626e..8f902a1 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -315,79 +315,155 @@ static void render_ring_cleanup(struct intel_ring_buffer *ring)
 }
 
 static void
-update_semaphore(struct intel_ring_buffer *ring, int i, u32 seqno)
+update_mboxes(struct intel_ring_buffer *ring,
+	    u32 seqno,
+	    u32 mmio_offset)
 {
-	struct drm_device *dev = ring->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int id;
-
-	/*
-	 * cs -> 1 = vcs, 0 = bcs
-	 * vcs -> 1 = bcs, 0 = cs,
-	 * bcs -> 1 = cs, 0 = vcs.
-	 */
-	id = ring - dev_priv->ring;
-	id += 2 - i;
-	id %= 3;
-
-	intel_ring_emit(ring,
-			MI_SEMAPHORE_MBOX |
-			MI_SEMAPHORE_REGISTER |
-			MI_SEMAPHORE_UPDATE);
+	intel_ring_emit(ring, MI_SEMAPHORE_MBOX |
+			      MI_SEMAPHORE_GLOBAL_GTT |
+			      MI_SEMAPHORE_REGISTER |
+			      MI_SEMAPHORE_UPDATE);
 	intel_ring_emit(ring, seqno);
-	intel_ring_emit(ring,
-			RING_SYNC_0(dev_priv->ring[id].mmio_base) + 4*i);
+	intel_ring_emit(ring, mmio_offset);
 }
 
-static int
+/**
+ * gen6_add_request - Update the semaphore mailbox registers
+ * 
+ * @ring - ring that is adding a request
+ * @mbox1_reg - mailbox address for RCS or VCS ring
+ * @mbox2_reg - mailbox address for VCS or BCS ring
+ *
+ * Update the mailbox registers in the *other* rings with the current seqno.
+ * This acts like a signal in the canonical semaphore.
+ */
+static u32
 gen6_add_request(struct intel_ring_buffer *ring,
-		 u32 *result)
+		 u32 mbox1_reg,
+		 u32 mbox2_reg)
 {
 	u32 seqno;
 	int ret;
+	seqno = i915_gem_get_seqno(ring->dev);
 
 	ret = intel_ring_begin(ring, 10);
 	if (ret)
 		return ret;
 
-	seqno = i915_gem_get_seqno(ring->dev);
-	update_semaphore(ring, 0, seqno);
-	update_semaphore(ring, 1, seqno);
-
+	update_mboxes(ring, seqno, mbox1_reg);
+	update_mboxes(ring, seqno, mbox2_reg);
 	intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
 	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
 	intel_ring_emit(ring, seqno);
 	intel_ring_emit(ring, MI_USER_INTERRUPT);
 	intel_ring_advance(ring);
 
-	*result = seqno;
+	return seqno;
+}
+
+static int
+gen6_blt_add_request(struct intel_ring_buffer *ring,
+		     u32 *result)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	*result = gen6_add_request(ring,
+				   dev_priv->ring[RCS].mmio_base + 0x44,
+				   dev_priv->ring[VCS].mmio_base + 0x40);
 	return 0;
 }
 
-int
-intel_ring_sync(struct intel_ring_buffer *ring,
-		struct intel_ring_buffer *to,
+static int
+gen6_bsd_add_request(struct intel_ring_buffer *ring,
+		     u32 *result)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	*result = gen6_add_request(ring,
+				   dev_priv->ring[RCS].mmio_base + 0x40,
+				   dev_priv->ring[BCS].mmio_base + 0x44);
+	return 0;
+}
+
+static int
+gen6_render_add_request(struct intel_ring_buffer *ring,
+		        u32 *result)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	*result = gen6_add_request(ring,
+				   dev_priv->ring[VCS].mmio_base + 0x44,
+				   dev_priv->ring[BCS].mmio_base + 0x40);
+	return 0;
+}
+
+static int
+intel_ring_sync(struct intel_ring_buffer *waiter,
+		struct intel_ring_buffer *signaller,
+		u32 semaphore_register,
 		u32 seqno)
 {
 	int ret;
+	u32 temp = MI_SEMAPHORE_MBOX |
+		   MI_SEMAPHORE_GLOBAL_GTT | /* Not needed */
+		   MI_SEMAPHORE_COMPARE;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(waiter, 4);
 	if (ret)
 		return ret;
 
-	intel_ring_emit(ring,
-			MI_SEMAPHORE_MBOX |
-			MI_SEMAPHORE_REGISTER |
-			intel_ring_sync_index(ring, to) << 17 |
-			MI_SEMAPHORE_COMPARE);
-	intel_ring_emit(ring, seqno);
-	intel_ring_emit(ring, 0);
-	intel_ring_emit(ring, MI_NOOP);
-	intel_ring_advance(ring);
+	temp |= MI_SEMAPHORE_REGISTER;
+
+	intel_ring_emit(waiter, temp | semaphore_register);
+	intel_ring_emit(waiter, seqno);
+	intel_ring_emit(waiter, 0);
+	intel_ring_emit(waiter, MI_NOOP);
+	intel_ring_advance(waiter);
 
 	return 0;
 }
 
+/* VCS->RCS (RVSYNC) or BCS->RCS (RBSYNC) */
+int
+render_ring_sync_to(struct intel_ring_buffer *waiter,
+		    struct intel_ring_buffer *signaller,
+		    u32 seqno)
+{
+	WARN_ON(signaller->semaphore_register[RCS] == MI_SEMAPHORE_SYNC_INVALID);
+	return intel_ring_sync(waiter,
+			       signaller,
+			       signaller->semaphore_register[RCS],
+			       seqno);
+}
+
+/* RCS->VCS (VRSYNC) or BCS->VCS (VBSYNC) */
+int
+gen6_bsd_ring_sync_to(struct intel_ring_buffer *waiter,
+		      struct intel_ring_buffer *signaller,
+		      u32 seqno)
+{
+	WARN_ON(signaller->semaphore_register[VCS] == MI_SEMAPHORE_SYNC_INVALID);
+	return intel_ring_sync(waiter,
+			       signaller,
+			       signaller->semaphore_register[VCS],
+			       seqno);
+}
+
+/* RCS->BCS (BRSYNC) or VCS->BCS (BVSYNC) */
+int
+gen6_blt_ring_sync_to(struct intel_ring_buffer *waiter,
+		      struct intel_ring_buffer *signaller,
+		      u32 seqno)
+{
+	WARN_ON(signaller->semaphore_register[BCS] == MI_SEMAPHORE_SYNC_INVALID);
+	return intel_ring_sync(waiter,
+			       signaller,
+			       signaller->semaphore_register[BCS],
+			       seqno);
+}
+
+
+
 #define PIPE_CONTROL_FLUSH(ring__, addr__)					\
 do {									\
 	intel_ring_emit(ring__, GFX_OP_PIPE_CONTROL | PIPE_CONTROL_QW_WRITE |		\
@@ -1027,6 +1103,10 @@ static const struct intel_ring_buffer render_ring = {
 	.irq_put		= render_ring_put_irq,
 	.dispatch_execbuffer	= render_ring_dispatch_execbuffer,
        .cleanup			= render_ring_cleanup,
+	.sync_to		= render_ring_sync_to,
+	.semaphore_register	= {MI_SEMAPHORE_SYNC_INVALID,
+				   MI_SEMAPHORE_SYNC_RV,
+				   MI_SEMAPHORE_SYNC_RB},
 };
 
 /* ring buffer for bit-stream decoder */
@@ -1149,11 +1229,15 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 	.init			= init_ring_common,
 	.write_tail		= gen6_bsd_ring_write_tail,
 	.flush			= gen6_ring_flush,
-	.add_request		= gen6_add_request,
+	.add_request		= gen6_bsd_add_request,
 	.get_seqno		= ring_get_seqno,
 	.irq_get		= gen6_bsd_ring_get_irq,
 	.irq_put		= gen6_bsd_ring_put_irq,
 	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
+	.sync_to		= gen6_bsd_ring_sync_to,
+	.semaphore_register	= {MI_SEMAPHORE_SYNC_VR,
+				   MI_SEMAPHORE_SYNC_INVALID,
+				   MI_SEMAPHORE_SYNC_VB},
 };
 
 /* Blitter support (SandyBridge+) */
@@ -1279,12 +1363,16 @@ static const struct intel_ring_buffer gen6_blt_ring = {
        .init			= blt_ring_init,
        .write_tail		= ring_write_tail,
        .flush			= blt_ring_flush,
-       .add_request		= gen6_add_request,
+       .add_request		= gen6_blt_add_request,
        .get_seqno		= ring_get_seqno,
        .irq_get			= blt_ring_get_irq,
        .irq_put			= blt_ring_put_irq,
        .dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
        .cleanup			= blt_ring_cleanup,
+       .sync_to			= gen6_blt_ring_sync_to,
+       .semaphore_register	= {MI_SEMAPHORE_SYNC_BR,
+				   MI_SEMAPHORE_SYNC_BV,
+				   MI_SEMAPHORE_SYNC_INVALID},
 };
 
 int intel_init_render_ring_buffer(struct drm_device *dev)
@@ -1294,7 +1382,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 
 	*ring = render_ring;
 	if (INTEL_INFO(dev)->gen >= 6) {
-		ring->add_request = gen6_add_request;
+		ring->add_request = gen6_render_add_request;
 		ring->irq_get = gen6_render_ring_get_irq;
 		ring->irq_put = gen6_render_ring_put_irq;
 	} else if (IS_GEN5(dev)) {
@@ -1317,7 +1405,7 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 
 	*ring = render_ring;
 	if (INTEL_INFO(dev)->gen >= 6) {
-		ring->add_request = gen6_add_request;
+		ring->add_request = gen6_render_add_request;
 		ring->irq_get = gen6_render_ring_get_irq;
 		ring->irq_put = gen6_render_ring_put_irq;
 	} else if (IS_GEN5(dev)) {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 39ac2b6..98052fd 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -75,7 +75,11 @@ struct  intel_ring_buffer {
 	int		(*dispatch_execbuffer)(struct intel_ring_buffer *ring,
 					       u32 offset, u32 length);
 	void		(*cleanup)(struct intel_ring_buffer *ring);
+	int		(*sync_to)(struct intel_ring_buffer *ring,
+				   struct intel_ring_buffer *to,
+				   u32 seqno);
 
+	u32		semaphore_register[3];
 	/**
 	 * List of objects currently involved in rendering from the
 	 * ringbuffer.
@@ -180,9 +184,6 @@ static inline void intel_ring_emit(struct intel_ring_buffer *ring,
 void intel_ring_advance(struct intel_ring_buffer *ring);
 
 u32 intel_ring_get_seqno(struct intel_ring_buffer *ring);
-int intel_ring_sync(struct intel_ring_buffer *ring,
-		    struct intel_ring_buffer *to,
-		    u32 seqno);
 
 int intel_init_render_ring_buffer(struct drm_device *dev);
 int intel_init_bsd_ring_buffer(struct drm_device *dev);
-- 
1.7.6.1

[PATCH 2/2] drm/i915: tracepoints for semaphores

[Ben Widawsky]

The tracepoints give enough info to figure the updates and compares
(document terminology for signals and waits) and dependencies therein of
the semaphore mailboxes.

Here are arguments to perf to get interesting info (mostly copied from
Chris):
record -f -g -c 1 -e i915:intel_ringbuffer_add_request -e i915:intel_ringbuffer_consume_semaphore -a

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_trace.h       |   61 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.c |    4 ++
 2 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index d623fef..493a0d3 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -410,6 +410,67 @@ TRACE_EVENT(i915_reg_rw,
 		     (u32)(__entry->val >> 32))
 );
 
+#define SEMAPHORE_REG_NAME(x) (\
+	(x == RENDER_RING_BASE + 0x40) ? ("RVSYNC") : \
+	((x == RENDER_RING_BASE + 0x44) ? ("RBSYNC") : \
+	((x == GEN6_BSD_RING_BASE + 0x40) ? ("VRSYNC") : \
+	((x == GEN6_BSD_RING_BASE + 0x44) ? ("VBSYNC") : \
+	((x == BLT_RING_BASE + 0x40) ? ("BVSYNC") : \
+	((x == BLT_RING_BASE + 0x44) ? ("BRSYNC") : \
+	("UNKNOWN")))))))
+
+#define RING_NAME(x) (\
+	(x == 1) ? ("rcs") : \
+	((x == 2) ? ("vcs") : \
+	((x == 4) ? ("bcs") : \
+	("unk"))))
+
+TRACE_EVENT(intel_ringbuffer_add_request,
+	    TP_PROTO(struct intel_ring_buffer *ring,
+		     u32 seqno,
+		     u32 reg1,
+		     u32 reg2),
+	    TP_ARGS(ring, seqno, reg1, reg2),
+	    TP_STRUCT__entry(
+			     __field(int, id)
+			     __field(u32, seqno)
+			     __field(u32, reg1)
+			     __field(u32, reg2)
+			    ),
+	    TP_fast_assign(
+			   __entry->id = ring->id;
+			   __entry->seqno = seqno;
+			   __entry->reg1 = reg1;
+			   __entry->reg2 = reg2;
+			  ),
+	    TP_printk("ring = %s, seqno = %u, signal mbox 1 = %s, signal mbox 2 = %s",
+		      RING_NAME(__entry->id), __entry->seqno,
+		      SEMAPHORE_REG_NAME(__entry->reg1),
+		      SEMAPHORE_REG_NAME(__entry->reg2))
+);
+
+TRACE_EVENT(intel_ringbuffer_consume_semaphore,
+	    TP_PROTO(struct intel_ring_buffer *updater,
+		     struct intel_ring_buffer *comparer,
+		     u32 seqno),
+	    TP_ARGS(updater, comparer, seqno),
+	    TP_STRUCT__entry(
+			     __field(int, src)
+			     __field(int, dest)
+			     __field(u32, seqno)
+			    ),
+	    TP_fast_assign(
+			   __entry->src = updater->id;
+			   __entry->dest = comparer->id;
+			   __entry->seqno = seqno;
+			  ),
+	    /* It's not easy to macro the consuming mbox register */
+	    TP_printk("signaller = %s, waiter = %s, seqno = %u",
+		      RING_NAME(__entry->src),
+		      RING_NAME(__entry->dest),
+		      __entry->seqno)
+);
+
 #endif /* _I915_TRACE_H_ */
 
 /* This part must be outside protection */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8f902a1..f816070 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -358,6 +358,8 @@ gen6_add_request(struct intel_ring_buffer *ring,
 	intel_ring_emit(ring, MI_USER_INTERRUPT);
 	intel_ring_advance(ring);
 
+	trace_intel_ringbuffer_add_request(ring, seqno, mbox1_reg, mbox2_reg);
+
 	return seqno;
 }
 
@@ -420,6 +422,8 @@ intel_ring_sync(struct intel_ring_buffer *waiter,
 	intel_ring_emit(waiter, MI_NOOP);
 	intel_ring_advance(waiter);
 
+	trace_intel_ringbuffer_consume_semaphore(signaller, waiter, seqno);
+
 	return 0;
 }
 
-- 
1.7.6.1

Re: [PATCH 1/2] drm/i915: Dumb down the semaphore logic

[Andrew Lutomirski]

On Wed, Sep 7, 2011 at 4:12 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> While I think the previous code is correct, it was hard to follow and
> hard to debug. Since we already have a ring abstraction, might as well
> use it to handle the semaphore updates and compares.
>
> I don't expect this code to make semaphores better or worse, but you
> never know...
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    3 +-
>  drivers/gpu/drm/i915/i915_reg.h            |    7 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |  176 +++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |    7 +-
>  4 files changed, 145 insertions(+), 48 deletions(-)
>

Sadly, it still instantly crashes.

--Andy

Re: [PATCH 1/2] drm/i915: Dumb down the semaphore logic

[Ben Widawsky]

On Sep 7, 2011, at 5:30 PM, Andrew Lutomirski <luto@mit.edu> wrote:

> On Wed, Sep 7, 2011 at 4:12 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>> While I think the previous code is correct, it was hard to follow and
>> hard to debug. Since we already have a ring abstraction, might as well
>> use it to handle the semaphore updates and compares.
>> 
>> I don't expect this code to make semaphores better or worse, but you
>> never know...
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Eric Anholt <eric@anholt.net>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    3 +-
>>  drivers/gpu/drm/i915/i915_reg.h            |    7 +
>>  drivers/gpu/drm/i915/intel_ringbuffer.c    |  176 +++++++++++++++++++++-------
>>  drivers/gpu/drm/i915/intel_ringbuffer.h    |    7 +-
>>  4 files changed, 145 insertions(+), 48 deletions(-)
>> 
> 
> Sadly, it still instantly crashes.
> 
> --Andy

Remind me again... Does ssh still work?

Re: [PATCH 1/2] drm/i915: Dumb down the semaphore logic

[Andrew Lutomirski]

On Wed, Sep 7, 2011 at 6:19 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>
>
> On Sep 7, 2011, at 5:30 PM, Andrew Lutomirski <luto@mit.edu> wrote:
>
>> On Wed, Sep 7, 2011 at 4:12 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>>> While I think the previous code is correct, it was hard to follow and
>>> hard to debug. Since we already have a ring abstraction, might as well
>>> use it to handle the semaphore updates and compares.
>>>
>>> I don't expect this code to make semaphores better or worse, but you
>>> never know...
>>>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Eric Anholt <eric@anholt.net>
>>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>>> ---
>>>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    3 +-
>>>  drivers/gpu/drm/i915/i915_reg.h            |    7 +
>>>  drivers/gpu/drm/i915/intel_ringbuffer.c    |  176 +++++++++++++++++++++-------
>>>  drivers/gpu/drm/i915/intel_ringbuffer.h    |    7 +-
>>>  4 files changed, 145 insertions(+), 48 deletions(-)
>>>
>>
>> Sadly, it still instantly crashes.
>>
>> --Andy
>
> Remind me again... Does ssh still work?

I haven't tried, but I'd be surprised.  The *reset* button (the
hardware one that's attached to the motherboard) doesn't work.

--Andy

Re: [PATCH 1/2] drm/i915: Dumb down the semaphore logic

[Keith Packard]

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

On Wed,  7 Sep 2011 16:12:41 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:

> -update_semaphore(struct intel_ring_buffer *ring, int i, u32 seqno)
> +update_mboxes(struct intel_ring_buffer *ring,
> +	    u32 seqno,
> +	    u32 mmio_offset)

Yeah, definitely like this change; lots less magic here.

> -static int
> +/**
> + * gen6_add_request - Update the semaphore mailbox registers
> + * 
> + * @ring - ring that is adding a request
> + * @mbox1_reg - mailbox address for RCS or VCS ring
> + * @mbox2_reg - mailbox address for VCS or BCS ring
> + *
> + * Update the mailbox registers in the *other* rings with the current seqno.
> + * This acts like a signal in the canonical semaphore.
> + */
> +static u32
>  gen6_add_request(struct intel_ring_buffer *ring,
> -		 u32 *result)
> +		 u32 mbox1_reg,
> +		 u32 mbox2_reg)

I think you're losing the ability to return errors from here.

>  	u32 seqno;
>  	int ret;
> +	seqno = i915_gem_get_seqno(ring->dev);
>  
>  	ret = intel_ring_begin(ring, 10);
>  	if (ret)
>  		return ret;
>  
> -	seqno = i915_gem_get_seqno(ring->dev);


Why change the ordering of get_seqno relative to ring_begin here?

> +static int
> +gen6_blt_add_request(struct intel_ring_buffer *ring,
> +		     u32 *result)
> +{
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	*result = gen6_add_request(ring,
> +				   dev_priv->ring[RCS].mmio_base + 0x44,
> +				   dev_priv->ring[VCS].mmio_base + 0x40);
>  	return 0;

Why the magic constants? Can we have named values? And, note that this
function never returns an error value, which is definitely not a good plan.

> +	temp |= MI_SEMAPHORE_REGISTER;

temp is a constant, why is it being |='d here?

> +/* VCS->RCS (RVSYNC) or BCS->RCS (RBSYNC) */
> +int
> +render_ring_sync_to(struct intel_ring_buffer *waiter,
> +		    struct intel_ring_buffer *signaller,
> +		    u32 seqno)
> +{
> +	WARN_ON(signaller->semaphore_register[RCS] == MI_SEMAPHORE_SYNC_INVALID);
> +	return intel_ring_sync(waiter,
> +			       signaller,
> +			       signaller->semaphore_register[RCS],

Should you just pass the index instead of the register value itself?

Otherwise, this seems like a reasonable change to me.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

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

Re: [PATCH 1/2] drm/i915: Dumb down the semaphore logic

[Daniel Vetter]

On Wed, Sep 07, 2011 at 04:12:41PM -0700, Ben Widawsky wrote:
> While I think the previous code is correct, it was hard to follow and
> hard to debug. Since we already have a ring abstraction, might as well
> use it to handle the semaphore updates and compares.
> 
> I don't expect this code to make semaphores better or worse, but you
> never know...

I kinda start to like this ;-)

While you stare at this, two things I'm pondering:
- Would it make sense to also move the !semaphores ring_sync_to case into
  the ringbuffer abstraction?
- Can we have a basic testcase for the magic values (and semaphores in
  general), please? I'm thinking of submitting a batchbuffer with a dummy
  r/w render relocation (hide it e.g. in the val field of
  MI_FLUSH_DW/PIPE_CONTROL without setting the write post-sync-op), and
  then randomly moving around the buffer to another ring. That should
  decently exercise the semaphores code in the kernel and sync hw in the
  gpu without (hopefully) anything else interfering.

-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] tests: basic ring<->cpu and ring<->ring tests

[Daniel Vetter]

Using a dummy reloc that doesn't matter to trick the kernel into
synchroizing the rings.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
Hi Ben,

This is the test I've had in mind. Unfortunately this kills my snb machine
both with semaphores=0 and semaphores=1. The ring<->cpu sync tests that
employ the exact same batchbuffer commands work flawless.

-Daniel
 lib/intel_chipset.h          |    4 +
 tests/Makefile.am            |    2 +
 tests/gem_dummy_reloc_loop.c |  152 ++++++++++++++++++++++++++++++++++++++++++
 tests/gem_ring_sync_loop.c   |  138 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 296 insertions(+), 0 deletions(-)
 create mode 100644 tests/gem_dummy_reloc_loop.c
 create mode 100644 tests/gem_ring_sync_loop.c

diff --git a/lib/intel_chipset.h b/lib/intel_chipset.h
index a38f661..35edaf7 100755
--- a/lib/intel_chipset.h
+++ b/lib/intel_chipset.h
@@ -169,6 +169,10 @@
 #define HAS_BLT_RING(devid)	(IS_GEN6(devid) || \
 				 IS_GEN7(devid))
 
+#define HAS_BSD_RING(devid)	(IS_GEN5(devid) || \
+				 IS_GEN6(devid) || \
+				 IS_GEN7(devid))
+
 #define IS_BROADWATER(devid)	(devid == PCI_CHIP_I946_GZ || \
 				 devid == PCI_CHIP_I965_G_1 || \
 				 devid == PCI_CHIP_I965_Q || \
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 8c52454..46ec696 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -47,6 +47,8 @@ TESTS = getversion \
 	gem_storedw_loop_bsd \
 	gem_storedw_batches_loop \
 	gem_pipe_control_store_loop \
+	gem_dummy_reloc_loop \
+	gem_ring_sync_loop \
 	$(NULL)
 
 HANG = \
diff --git a/tests/gem_dummy_reloc_loop.c b/tests/gem_dummy_reloc_loop.c
new file mode 100644
index 0000000..e0b6803
--- /dev/null
+++ b/tests/gem_dummy_reloc_loop.c
@@ -0,0 +1,152 @@
+/*
+ * Copyright © 2011 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Daniel Vetter <daniel.vetter@ffwll.ch> (based on gem_storedw_*.c)
+ *
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <assert.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/time.h>
+#include "drm.h"
+#include "i915_drm.h"
+#include "drmtest.h"
+#include "intel_bufmgr.h"
+#include "intel_batchbuffer.h"
+#include "intel_gpu_tools.h"
+#include "i830_reg.h"
+
+static drm_intel_bufmgr *bufmgr;
+struct intel_batchbuffer *batch;
+static drm_intel_bo *target_buffer;
+
+/*
+ * Testcase: Basic check of ring<->cpu sync using a dummy reloc
+ */
+
+#define GFX_OP_PIPE_CONTROL	((0x3<<29)|(0x3<<27)|(0x2<<24)|2)
+#define   PIPE_CONTROL_WC_FLUSH	(1<<12)
+
+static void
+dummy_reloc_loop(int ring)
+{
+	int i;
+
+	for (i = 0; i < 0x100000; i++) {
+		if (ring == I915_EXEC_RENDER) {
+			BEGIN_BATCH(4);
+			OUT_BATCH(GFX_OP_PIPE_CONTROL | PIPE_CONTROL_WC_FLUSH);
+			OUT_BATCH(0);
+			OUT_RELOC(target_buffer, I915_GEM_DOMAIN_RENDER,
+					I915_GEM_DOMAIN_RENDER, 0);
+			OUT_BATCH(0);
+			ADVANCE_BATCH();
+		} else {
+			BEGIN_BATCH(4);
+			OUT_BATCH(MI_FLUSH_DW | 2);
+			OUT_BATCH(0); /* reserved */
+			OUT_RELOC(target_buffer, I915_GEM_DOMAIN_RENDER,
+					I915_GEM_DOMAIN_RENDER, 0);
+			OUT_BATCH(0);
+			ADVANCE_BATCH();
+		}
+		intel_batchbuffer_flush_on_ring(batch, ring);
+	}
+
+	drm_intel_bo_map(target_buffer, 0);
+	// map to force completion
+	drm_intel_bo_unmap(target_buffer);
+}
+
+int main(int argc, char **argv)
+{
+	int fd;
+	int devid;
+
+	if (argc != 1) {
+		fprintf(stderr, "usage: %s\n", argv[0]);
+		exit(-1);
+	}
+
+	fd = drm_open_any();
+	devid = intel_get_drm_devid(fd);
+	if (!HAS_BLT_RING(devid)) {
+		fprintf(stderr, "not (yet) implemented for pre-snb\n");
+		goto out;
+	}
+
+	bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
+	if (!bufmgr) {
+		fprintf(stderr, "failed to init libdrm\n");
+		exit(-1);
+	}
+	drm_intel_bufmgr_gem_enable_reuse(bufmgr);
+
+	batch = intel_batchbuffer_alloc(bufmgr, devid);
+	if (!batch) {
+		fprintf(stderr, "failed to create batch buffer\n");
+		exit(-1);
+	}
+
+	target_buffer = drm_intel_bo_alloc(bufmgr, "target bo", 4096, 4096);
+	if (!target_buffer) {
+		fprintf(stderr, "failed to alloc target buffer\n");
+		exit(-1);
+	}
+
+	fprintf(stderr, "running dummy loop on render\n");
+	dummy_reloc_loop(I915_EXEC_RENDER);
+	fprintf(stderr, "dummy loop run on render completed\n");
+
+	if (!HAS_BSD_RING(devid))
+		goto skip;
+
+	sleep(2);
+	fprintf(stderr, "running dummy loop on bsd\n");
+	dummy_reloc_loop(I915_EXEC_BSD);
+	fprintf(stderr, "dummy loop run on bsd completed\n");
+
+	if (!HAS_BLT_RING(devid))
+		goto skip;
+
+	sleep(2);
+	fprintf(stderr, "running dummy loop on blt\n");
+	dummy_reloc_loop(I915_EXEC_BLT);
+	fprintf(stderr, "dummy loop run on blt completed\n");
+
+skip:
+	drm_intel_bo_unreference(target_buffer);
+	intel_batchbuffer_free(batch);
+	drm_intel_bufmgr_destroy(bufmgr);
+
+out:
+	close(fd);
+
+	return 0;
+}
diff --git a/tests/gem_ring_sync_loop.c b/tests/gem_ring_sync_loop.c
new file mode 100644
index 0000000..7688a1d
--- /dev/null
+++ b/tests/gem_ring_sync_loop.c
@@ -0,0 +1,138 @@
+/*
+ * Copyright © 2011 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Daniel Vetter <daniel.vetter@ffwll.ch> (based on gem_storedw_*.c)
+ *
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <assert.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/time.h>
+#include "drm.h"
+#include "i915_drm.h"
+#include "drmtest.h"
+#include "intel_bufmgr.h"
+#include "intel_batchbuffer.h"
+#include "intel_gpu_tools.h"
+#include "i830_reg.h"
+
+static drm_intel_bufmgr *bufmgr;
+struct intel_batchbuffer *batch;
+static drm_intel_bo *target_buffer;
+
+/*
+ * Testcase: Basic check of ring<->ring sync using a dummy reloc
+ */
+
+#define GFX_OP_PIPE_CONTROL	((0x3<<29)|(0x3<<27)|(0x2<<24)|2)
+#define   PIPE_CONTROL_WC_FLUSH	(1<<12)
+
+static void
+store_dword_loop(int ring)
+{
+	int i;
+
+	srandom(0xdeadbeef);
+
+	for (i = 0; i < 0x100; i++) {
+		int ring = random() % 3;
+
+		if (ring == I915_EXEC_RENDER) {
+			BEGIN_BATCH(4);
+			OUT_BATCH(GFX_OP_PIPE_CONTROL | PIPE_CONTROL_WC_FLUSH);
+			OUT_BATCH(0);
+			OUT_RELOC(target_buffer, I915_GEM_DOMAIN_RENDER,
+					I915_GEM_DOMAIN_RENDER, 0);
+			OUT_BATCH(0);
+			ADVANCE_BATCH();
+		} else {
+			BEGIN_BATCH(4);
+			OUT_BATCH(MI_FLUSH_DW | 2);
+			OUT_BATCH(0); /* reserved */
+			OUT_RELOC(target_buffer, I915_GEM_DOMAIN_RENDER,
+					I915_GEM_DOMAIN_RENDER, 0);
+			OUT_BATCH(0);
+			ADVANCE_BATCH();
+		}
+		intel_batchbuffer_flush_on_ring(batch, ring);
+
+		drm_intel_bo_map(target_buffer, 0);
+		// map to force waiting on rendering
+		drm_intel_bo_unmap(target_buffer);
+	}
+}
+
+int main(int argc, char **argv)
+{
+	int fd;
+	int devid;
+
+	if (argc != 1) {
+		fprintf(stderr, "usage: %s\n", argv[0]);
+		exit(-1);
+	}
+
+	fd = drm_open_any();
+	devid = intel_get_drm_devid(fd);
+	if (!HAS_BLT_RING(devid)) {
+		fprintf(stderr, "inter ring check needs gen6+\n");
+		goto out;
+	}
+
+
+	bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
+	if (!bufmgr) {
+		fprintf(stderr, "failed to init libdrm\n");
+		exit(-1);
+	}
+	drm_intel_bufmgr_gem_enable_reuse(bufmgr);
+
+	batch = intel_batchbuffer_alloc(bufmgr, devid);
+	if (!batch) {
+		fprintf(stderr, "failed to create batch buffer\n");
+		exit(-1);
+	}
+
+	target_buffer = drm_intel_bo_alloc(bufmgr, "target bo", 4096, 4096);
+	if (!target_buffer) {
+		fprintf(stderr, "failed to alloc target buffer\n");
+		exit(-1);
+	}
+
+	store_dword_loop(I915_EXEC_RENDER);
+
+	drm_intel_bo_unreference(target_buffer);
+	intel_batchbuffer_free(batch);
+	drm_intel_bufmgr_destroy(bufmgr);
+
+out:
+	close(fd);
+
+	return 0;
+}
-- 
1.7.6

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

Re: [PATCH 1/2] drm/i915: Dumb down the semaphore logic

[Ben Widawsky]

On Thu, 8 Sep 2011 09:52:36 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Sep 07, 2011 at 04:12:41PM -0700, Ben Widawsky wrote:
> > While I think the previous code is correct, it was hard to follow
> > and hard to debug. Since we already have a ring abstraction, might
> > as well use it to handle the semaphore updates and compares.
> > 
> > I don't expect this code to make semaphores better or worse, but you
> > never know...
> 
> I kinda start to like this ;-)

As I said, I am biased. To me it is much simpler to understand, but it's
definitely more code, and more abstraction.

> 
> While you stare at this, two things I'm pondering:
> - Would it make sense to also move the !semaphores ring_sync_to case
> into the ringbuffer abstraction?

That is an excellent idea. I will work on it with the other
recommendations from Keith.

> - Can we have a basic testcase for the magic values (and semaphores in
>   general), please?

I really like what you've done with the test case after the fixes
discussed on IRC. I will update my commit message to mention it if you
manage to push that before I fix up the other things.

Ben