Semaphore cleanups

Semaphore cleanups

[Ben Widawsky]

FWIW, I thought my original patch was the easiest to read, but this certainly
removes the most magic. These had a ton of input from Daniel Vetter, and Chris
Wilson. The goal is only to make the code more readable, and easier to veryify/debug.

 drivers/gpu/drm/i915/i915_debugfs.c        |   10 ++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 +-
 drivers/gpu/drm/i915/i915_reg.h            |   24 +++++++++-
 drivers/gpu/drm/i915/i915_trace.h          |   61 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.c    |   66 +++++++++++++++------------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |   57 +++++++++++++-----------
 6 files changed, 157 insertions(+), 65 deletions(-)

[PATCH 1/5] drm/i915: clean up ring id/semaphore sync index magic

[Ben Widawsky]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

The calculation of the semaphore sync register index is obfuscated
by some pointer calculation to get at the ring id (RCS, VCS and BCS).
Now we already have a ring->id field, but that contains a flag value
usefull for ORing together multiple rings - that's what the flushing
code as the user of this field needs.

So change the meaning of ring->id to be the real id, add a tiny
helper for those that actually want a flag and use ring->id in the
semaphore index calculations.

Also extract the inverse function intel_sync_index_to_ring_id and
move it right next to intel_ring_sync_index.

v2: Simplify RING_SYNC macro as suggested by Chris Wilson.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |   10 +++---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 +-
 drivers/gpu/drm/i915/i915_reg.h            |    3 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c    |   30 +++++++----------
 drivers/gpu/drm/i915/intel_ringbuffer.h    |   48 +++++++++++++++++++--------
 5 files changed, 54 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3c395a5..b4f3ecb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -647,8 +647,8 @@ static int i915_ringbuffer_info(struct seq_file *m, void *data)
 	seq_printf(m, "  Active :  %08x\n", intel_ring_get_active_head(ring));
 	seq_printf(m, "  NOPID :   %08x\n", I915_READ_NOPID(ring));
 	if (IS_GEN6(dev)) {
-		seq_printf(m, "  Sync 0 :   %08x\n", I915_READ_SYNC_0(ring));
-		seq_printf(m, "  Sync 1 :   %08x\n", I915_READ_SYNC_1(ring));
+		seq_printf(m, "  Sync 0 :   %08x\n", I915_READ_SYNC(ring, 0));
+		seq_printf(m, "  Sync 1 :   %08x\n", I915_READ_SYNC(ring, 1));
 	}
 	seq_printf(m, "  Control : %08x\n", I915_READ_CTL(ring));
 	seq_printf(m, "  Start :   %08x\n", I915_READ_START(ring));
@@ -659,9 +659,9 @@ static int i915_ringbuffer_info(struct seq_file *m, void *data)
 static const char *ring_str(int ring)
 {
 	switch (ring) {
-	case RING_RENDER: return " render";
-	case RING_BSD: return " bsd";
-	case RING_BLT: return " blt";
+	case RCS: return " render";
+	case VCS: return " bsd";
+	case BCS: return " blt";
 	default: return "";
 	}
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 4934cf8..f2e1af7 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -202,9 +202,9 @@ i915_gem_object_set_to_gpu_domain(struct drm_i915_gem_object *obj,
 	cd->invalidate_domains |= invalidate_domains;
 	cd->flush_domains |= flush_domains;
 	if (flush_domains & I915_GEM_GPU_DOMAINS)
-		cd->flush_rings |= obj->ring->id;
+		cd->flush_rings |= intel_ring_flag(obj->ring);
 	if (invalidate_domains & I915_GEM_GPU_DOMAINS)
-		cd->flush_rings |= ring->id;
+		cd->flush_rings |= intel_ring_flag(ring);
 }
 
 struct eb_objects {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 542453f..f1cc55a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -294,8 +294,7 @@
 #define RING_HEAD(base)		((base)+0x34)
 #define RING_START(base)	((base)+0x38)
 #define RING_CTL(base)		((base)+0x3c)
-#define RING_SYNC_0(base)	((base)+0x40)
-#define RING_SYNC_1(base)	((base)+0x44)
+#define RING_SYNC(base, idx)	((base)+0x40 + 4*(idx))
 #define RING_MAX_IDLE(base)	((base)+0x54)
 #define RING_HWS_PGA(base)	((base)+0x80)
 #define RING_HWS_PGA_GEN6(base)	((base)+0x2080)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c30626e..9be0f11 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -319,24 +319,18 @@ update_semaphore(struct intel_ring_buffer *ring, int i, u32 seqno)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int id;
+	struct intel_ring_buffer *other;
+	u32 reg;
 
-	/*
-	 * 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;
+	other = &dev_priv->ring[intel_sync_index_to_ring_id(ring, i)];
+	reg = RING_SYNC(other->mmio_base, i);
 
 	intel_ring_emit(ring,
 			MI_SEMAPHORE_MBOX |
 			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, reg);
 }
 
 static int
@@ -565,13 +559,13 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring)
 	 */
 	if (IS_GEN7(dev)) {
 		switch (ring->id) {
-		case RING_RENDER:
+		case RCS:
 			mmio = RENDER_HWS_PGA_GEN7;
 			break;
-		case RING_BLT:
+		case BCS:
 			mmio = BLT_HWS_PGA_GEN7;
 			break;
-		case RING_BSD:
+		case VCS:
 			mmio = BSD_HWS_PGA_GEN7;
 			break;
 		}
@@ -1015,7 +1009,7 @@ void intel_ring_advance(struct intel_ring_buffer *ring)
 
 static const struct intel_ring_buffer render_ring = {
 	.name			= "render ring",
-	.id			= RING_RENDER,
+	.id			= RCS,
 	.mmio_base		= RENDER_RING_BASE,
 	.size			= 32 * PAGE_SIZE,
 	.init			= init_render_ring,
@@ -1033,7 +1027,7 @@ static const struct intel_ring_buffer render_ring = {
 
 static const struct intel_ring_buffer bsd_ring = {
 	.name                   = "bsd ring",
-	.id			= RING_BSD,
+	.id			= VCS,
 	.mmio_base		= BSD_RING_BASE,
 	.size			= 32 * PAGE_SIZE,
 	.init			= init_ring_common,
@@ -1143,7 +1137,7 @@ gen6_bsd_ring_put_irq(struct intel_ring_buffer *ring)
 /* ring buffer for Video Codec for Gen6+ */
 static const struct intel_ring_buffer gen6_bsd_ring = {
 	.name			= "gen6 bsd ring",
-	.id			= RING_BSD,
+	.id			= VCS,
 	.mmio_base		= GEN6_BSD_RING_BASE,
 	.size			= 32 * PAGE_SIZE,
 	.init			= init_ring_common,
@@ -1273,7 +1267,7 @@ static void blt_ring_cleanup(struct intel_ring_buffer *ring)
 
 static const struct intel_ring_buffer gen6_blt_ring = {
        .name			= "blt ring",
-       .id			= RING_BLT,
+       .id			= BCS,
        .mmio_base		= BLT_RING_BASE,
        .size			= 32 * PAGE_SIZE,
        .init			= blt_ring_init,
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 39ac2b6..0029978 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -1,13 +1,6 @@
 #ifndef _INTEL_RINGBUFFER_H_
 #define _INTEL_RINGBUFFER_H_
 
-enum {
-    RCS = 0x0,
-    VCS,
-    BCS,
-    I915_NUM_RINGS,
-};
-
 struct  intel_hw_status_page {
 	u32	__iomem	*page_addr;
 	unsigned int	gfx_addr;
@@ -30,16 +23,19 @@ struct  intel_hw_status_page {
 #define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val)
 
 #define I915_READ_NOPID(ring) I915_READ(RING_NOPID((ring)->mmio_base))
-#define I915_READ_SYNC_0(ring) I915_READ(RING_SYNC_0((ring)->mmio_base))
-#define I915_READ_SYNC_1(ring) I915_READ(RING_SYNC_1((ring)->mmio_base))
+#define I915_READ_SYNC(ring, idx) I915_READ(RING_SYNC((ring)->mmio_base, (idx)))
 
 struct  intel_ring_buffer {
 	const char	*name;
-	enum intel_ring_id {
-		RING_RENDER = 0x1,
-		RING_BSD = 0x2,
-		RING_BLT = 0x4,
+	enum {
+	    RCS = 0x0,
+	    VCS,
+	    BCS,
 	} id;
+#define I915_NUM_RINGS (BCS+1)
+#define RING_RENDER (1<<RCS)
+#define RING_BSD (1<<VCS)
+#define RING_BLT (1<<BLT)
 	u32		mmio_base;
 	void		__iomem *virtual_start;
 	struct		drm_device *dev;
@@ -114,6 +110,12 @@ struct  intel_ring_buffer {
 	void *private;
 };
 
+static inline unsigned
+intel_ring_flag(struct intel_ring_buffer *ring)
+{
+	return (1 << ring->id);
+}
+
 static inline u32
 intel_ring_sync_index(struct intel_ring_buffer *ring,
 		      struct intel_ring_buffer *other)
@@ -126,13 +128,31 @@ intel_ring_sync_index(struct intel_ring_buffer *ring,
 	 * bcs -> 0 = cs, 1 = vcs.
 	 */
 
-	idx = (other - ring) - 1;
+	idx = (other->id - ring->id) - 1;
 	if (idx < 0)
 		idx += I915_NUM_RINGS;
 
 	return idx;
 }
 
+static inline int
+intel_sync_index_to_ring_id(struct intel_ring_buffer *ring,
+			 int other)
+{
+	int id;
+
+	/*
+	 * cs -> 1 = vcs, 0 = bcs
+	 * vcs -> 1 = bcs, 0 = cs,
+	 * bcs -> 1 = cs, 0 = vcs.
+	 */
+	id = ring->id;
+	id += 2 - other;
+	id %= 3;
+
+	return id;
+}
+
 static inline u32
 intel_read_status_page(struct intel_ring_buffer *ring,
 		       int reg)
-- 
1.7.6.1

[PATCH 2/5] drm/i915: semaphore wait cleanup

[Ben Widawsky]

Add a per ring lookup table, and #defines to clarify semaphore waits.

Rename ring variables in sync function to clearly show correct names based on
canonical semaphore naming.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_reg.h         |   15 +++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.c |   28 +++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    2 ++
 3 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f1cc55a..fd087b6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -189,11 +189,26 @@
 #define   MI_BATCH_NON_SECURE	(1)
 #define   MI_BATCH_NON_SECURE_I965 (1<<8)
 #define MI_BATCH_BUFFER_START	MI_INSTR(0x31, 0)
+
+/* Semaphore mailboxes are a bit tricky to understand based on names.
+ * There are NUM_RINGS-1 mailboxes per ring, and they are written to by
+ * the other ring(s). When a ring wishes to wait, it does so by
+ * comparing the value in it's own mailbox register, and SW must program
+ * which mailbox to compare against.
+ */
 #define MI_SEMAPHORE_MBOX	MI_INSTR(0x16, 1) /* gen6+ */
 #define  MI_SEMAPHORE_GLOBAL_GTT    (1<<22)
 #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 9be0f11..2aac49e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -359,25 +359,25 @@ gen6_add_request(struct intel_ring_buffer *ring,
 }
 
 int
-intel_ring_sync(struct intel_ring_buffer *ring,
-		struct intel_ring_buffer *to,
+intel_ring_sync(struct intel_ring_buffer *waiter,
+		struct intel_ring_buffer *signaller,
 		u32 seqno)
 {
 	int ret;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(waiter, 4);
 	if (ret)
 		return ret;
 
-	intel_ring_emit(ring,
+	intel_ring_emit(waiter,
 			MI_SEMAPHORE_MBOX |
 			MI_SEMAPHORE_REGISTER |
-			intel_ring_sync_index(ring, to) << 17 |
+			signaller->semaphore_register[waiter->id] |
 			MI_SEMAPHORE_COMPARE);
-	intel_ring_emit(ring, seqno);
-	intel_ring_emit(ring, 0);
-	intel_ring_emit(ring, MI_NOOP);
-	intel_ring_advance(ring);
+	intel_ring_emit(waiter, seqno);
+	intel_ring_emit(waiter, 0);
+	intel_ring_emit(waiter, MI_NOOP);
+	intel_ring_advance(waiter);
 
 	return 0;
 }
@@ -1021,6 +1021,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,
+	.semaphore_register     = {MI_SEMAPHORE_SYNC_INVALID,
+				   MI_SEMAPHORE_SYNC_RV,
+				   MI_SEMAPHORE_SYNC_RB},
+
 };
 
 /* ring buffer for bit-stream decoder */
@@ -1148,6 +1152,9 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 	.irq_get		= gen6_bsd_ring_get_irq,
 	.irq_put		= gen6_bsd_ring_put_irq,
 	.dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
+	.semaphore_register     = {MI_SEMAPHORE_SYNC_VR,
+				   MI_SEMAPHORE_SYNC_INVALID,
+				   MI_SEMAPHORE_SYNC_VB},
 };
 
 /* Blitter support (SandyBridge+) */
@@ -1279,6 +1286,9 @@ static const struct intel_ring_buffer gen6_blt_ring = {
        .irq_put			= blt_ring_put_irq,
        .dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
        .cleanup			= blt_ring_cleanup,
+	.semaphore_register     = {MI_SEMAPHORE_SYNC_BR,
+				   MI_SEMAPHORE_SYNC_BV,
+				   MI_SEMAPHORE_SYNC_INVALID},
 };
 
 int intel_init_render_ring_buffer(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 0029978..13167c6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -72,6 +72,8 @@ struct  intel_ring_buffer {
 					       u32 offset, u32 length);
 	void		(*cleanup)(struct intel_ring_buffer *ring);
 
+	u32             semaphore_register[3]; /*our mbox written by others */
+
 	/**
 	 * List of objects currently involved in rendering from the
 	 * ringbuffer.
-- 
1.7.6.1

[PATCH 3/5] drm/i915: semaphore signal cleanup

[Ben Widawsky]

Create another lookup table and #defines again to clarify ring signalling.

Cut out previous black (yet clever) magic.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_reg.h         |    6 ++++++
 drivers/gpu/drm/i915/intel_ringbuffer.c |    9 ++++-----
 drivers/gpu/drm/i915/intel_ringbuffer.h |   19 +------------------
 3 files changed, 11 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fd087b6..dfee8f2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -310,6 +310,12 @@
 #define RING_START(base)	((base)+0x38)
 #define RING_CTL(base)		((base)+0x3c)
 #define RING_SYNC(base, idx)	((base)+0x40 + 4*(idx))
+#define GEN6_RVSYNC (RING_SYNC(RENDER_RING_BASE, 0))
+#define GEN6_RBSYNC (RING_SYNC(RENDER_RING_BASE, 1))
+#define GEN6_VRSYNC (RING_SYNC(GEN6_BSD_RING_BASE, 1))
+#define GEN6_VBSYNC (RING_SYNC(GEN6_BSD_RING_BASE, 0))
+#define GEN6_BRSYNC (RING_SYNC(BLT_RING_BASE, 0))
+#define GEN6_BVSYNC (RING_SYNC(BLT_RING_BASE, 1))
 #define RING_MAX_IDLE(base)	((base)+0x54)
 #define RING_HWS_PGA(base)	((base)+0x80)
 #define RING_HWS_PGA_GEN6(base)	((base)+0x2080)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2aac49e..cbf255e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -317,13 +317,9 @@ static void render_ring_cleanup(struct intel_ring_buffer *ring)
 static void
 update_semaphore(struct intel_ring_buffer *ring, int i, u32 seqno)
 {
-	struct drm_device *dev = ring->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_ring_buffer *other;
 	u32 reg;
 
-	other = &dev_priv->ring[intel_sync_index_to_ring_id(ring, i)];
-	reg = RING_SYNC(other->mmio_base, i);
+	reg = ring->signal_mbox[i];
 
 	intel_ring_emit(ring,
 			MI_SEMAPHORE_MBOX |
@@ -1024,6 +1020,7 @@ static const struct intel_ring_buffer render_ring = {
 	.semaphore_register     = {MI_SEMAPHORE_SYNC_INVALID,
 				   MI_SEMAPHORE_SYNC_RV,
 				   MI_SEMAPHORE_SYNC_RB},
+	.signal_mbox            = {GEN6_VRSYNC, GEN6_BRSYNC},
 
 };
 
@@ -1155,6 +1152,7 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 	.semaphore_register     = {MI_SEMAPHORE_SYNC_VR,
 				   MI_SEMAPHORE_SYNC_INVALID,
 				   MI_SEMAPHORE_SYNC_VB},
+	.signal_mbox            = {GEN6_RVSYNC, GEN6_BVSYNC},
 };
 
 /* Blitter support (SandyBridge+) */
@@ -1289,6 +1287,7 @@ static const struct intel_ring_buffer gen6_blt_ring = {
 	.semaphore_register     = {MI_SEMAPHORE_SYNC_BR,
 				   MI_SEMAPHORE_SYNC_BV,
 				   MI_SEMAPHORE_SYNC_INVALID},
+	.signal_mbox            = {GEN6_RBSYNC, GEN6_VBSYNC},
 };
 
 int intel_init_render_ring_buffer(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 13167c6..61790c8 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -73,6 +73,7 @@ struct  intel_ring_buffer {
 	void		(*cleanup)(struct intel_ring_buffer *ring);
 
 	u32             semaphore_register[3]; /*our mbox written by others */
+	u32             signal_mbox[2]; /* mboxes this ring signals to */
 
 	/**
 	 * List of objects currently involved in rendering from the
@@ -137,24 +138,6 @@ intel_ring_sync_index(struct intel_ring_buffer *ring,
 	return idx;
 }
 
-static inline int
-intel_sync_index_to_ring_id(struct intel_ring_buffer *ring,
-			 int other)
-{
-	int id;
-
-	/*
-	 * cs -> 1 = vcs, 0 = bcs
-	 * vcs -> 1 = bcs, 0 = cs,
-	 * bcs -> 1 = cs, 0 = vcs.
-	 */
-	id = ring->id;
-	id += 2 - other;
-	id %= 3;
-
-	return id;
-}
-
 static inline u32
 intel_read_status_page(struct intel_ring_buffer *ring,
 		       int reg)
-- 
1.7.6.1

[PATCH 4/5] drm/i915: more semaphore cleanup

[Ben Widawsky]

This turns the black magic into brown magic.

It's arguable whether or not this is more readable than the existing
code. It does add a nice assertion, fewer lines of actual code, and some
nice comments - as well as sticking to semantics now used in the
ringbuffer code.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_ringbuffer.h |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 61790c8..e4516e4 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -119,23 +119,21 @@ intel_ring_flag(struct intel_ring_buffer *ring)
 	return (1 << ring->id);
 }
 
+/* intel_ring_sync_index - return the index (0-n, where n is total rings - 1)
+ * of the ring we're synchronizing to.
+ *
+ * @signaller - ring which last updated the mailbox
+ * @waiter - ring waiting on seqno in mailbox
+ */
 static inline u32
-intel_ring_sync_index(struct intel_ring_buffer *ring,
-		      struct intel_ring_buffer *other)
+intel_ring_sync_index(struct intel_ring_buffer *signaller,
+		      struct intel_ring_buffer *waiter)
 {
-	int idx;
-
-	/*
-	 * cs -> 0 = vcs, 1 = bcs
-	 * vcs -> 0 = bcs, 1 = cs,
-	 * bcs -> 0 = cs, 1 = vcs.
+	/* This bit of magic is based on the fact that there are 3 rings, and
+	 * the value of bit 17 is a unique way to identify the mailbox
 	 */
-
-	idx = (other->id - ring->id) - 1;
-	if (idx < 0)
-		idx += I915_NUM_RINGS;
-
-	return idx;
+	BUG_ON(signaller->semaphore_register[waiter->id] >> 17 > 1);
+	return (signaller->semaphore_register[waiter->id] >> 17);
 }
 
 static inline u32
-- 
1.7.6.1

[PATCH 5/5] 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 |    5 +++
 2 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index d623fef..47d55fd 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 = %d, signal mbox 2 = %d",
+		      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 cbf255e..6bf1dd9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -351,6 +351,9 @@ gen6_add_request(struct intel_ring_buffer *ring,
 	intel_ring_advance(ring);
 
 	*result = seqno;
+
+	trace_intel_ringbuffer_add_request(ring, seqno, ring->signal_mbox[0],
+					   ring->signal_mbox[1]);
 	return 0;
 }
 
@@ -375,6 +378,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/5] drm/i915: clean up ring id/semaphore sync index magic

[Ben Widawsky]

On Thu, 15 Sep 2011 19:08:57 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> The calculation of the semaphore sync register index is obfuscated
> by some pointer calculation to get at the ring id (RCS, VCS and BCS).
> Now we already have a ring->id field, but that contains a flag value
> usefull for ORing together multiple rings - that's what the flushing
> code as the user of this field needs.
> 
> So change the meaning of ring->id to be the real id, add a tiny
> helper for those that actually want a flag and use ring->id in the
> semaphore index calculations.
> 
> Also extract the inverse function intel_sync_index_to_ring_id and
> move it right next to intel_ring_sync_index.
> 
> v2: Simplify RING_SYNC macro as suggested by Chris Wilson.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Crap, I forgot:
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Re: [PATCH 4/5] drm/i915: more semaphore cleanup

[Zou, Nanhai]

>>-----Original Message-----
>>From: intel-gfx-bounces+nanhai.zou=intel.com@lists.freedesktop.org
>>[mailto:intel-gfx-bounces+nanhai.zou=intel.com@lists.freedesktop.org] On
>>Behalf Of Ben Widawsky
>>Sent: 2011年9月16日 10:09
>>To: intel-gfx@lists.freedesktop.org
>>Cc: Daniel Vetter; Ben Widawsky
>>Subject: [Intel-gfx] [PATCH 4/5] drm/i915: more semaphore cleanup
>>
>>This turns the black magic into brown magic.
>>
>>It's arguable whether or not this is more readable than the existing
>>code. It does add a nice assertion, fewer lines of actual code, and some
>>nice comments - as well as sticking to semantics now used in the
>>ringbuffer code.
>>
>>Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>>---
>> drivers/gpu/drm/i915/intel_ringbuffer.h |   26 ++++++++++++--------------
>> 1 files changed, 12 insertions(+), 14 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
>>b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>index 61790c8..e4516e4 100644
>>--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>>+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>@@ -119,23 +119,21 @@ intel_ring_flag(struct intel_ring_buffer *ring)
>> 	return (1 << ring->id);
>> }
>>
>>+/* intel_ring_sync_index - return the index (0-n, where n is total rings -
>>1)
>>+ * of the ring we're synchronizing to.
>>+ *
>>+ * @signaller - ring which last updated the mailbox
>>+ * @waiter - ring waiting on seqno in mailbox
>>+ */
>> static inline u32
>>-intel_ring_sync_index(struct intel_ring_buffer *ring,
>>-		      struct intel_ring_buffer *other)
>>+intel_ring_sync_index(struct intel_ring_buffer *signaller,
>>+		      struct intel_ring_buffer *waiter)
>> {
>>-	int idx;
>>-
>>-	/*
>>-	 * cs -> 0 = vcs, 1 = bcs
>>-	 * vcs -> 0 = bcs, 1 = cs,
>>-	 * bcs -> 0 = cs, 1 = vcs.
>>+	/* This bit of magic is based on the fact that there are 3 rings, and
>>+	 * the value of bit 17 is a unique way to identify the mailbox
>> 	 */
>>-
>>-	idx = (other->id - ring->id) - 1;
>>-	if (idx < 0)
>>-		idx += I915_NUM_RINGS;
>>-
>>-	return idx;
>>+	BUG_ON(signaller->semaphore_register[waiter->id] >> 17 > 1);
>>+	return (signaller->semaphore_register[waiter->id] >> 17);
>> }
>>
>> static inline u32
>>--
>>1.7.6.1

Can we get rid of all those magic calculations by simple lookup table?

Consider there will be more rings in future hardware.
E.g. video enhancement ring.

Thanks
Zou Nanhai

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

Re: Semaphore cleanups

[Daniel Vetter]

On Thu, Sep 15, 2011 at 07:08:56PM -0700, Ben Widawsky wrote:
> FWIW, I thought my original patch was the easiest to read, but this certainly
> removes the most magic. These had a ton of input from Daniel Vetter, and Chris
> Wilson. The goal is only to make the code more readable, and easier to veryify/debug.

With the bloat gone:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> for patches 2-4

But in then end I think it boils down to having a bunch of magic values in
a table or having a bunch or magic calculations in a few tiny functions.
So after the ring->id cleanup (which imo is independant cleanup, albeit just a
tiny improvement) I don't care with what approach we'll be going. And imo
engineering for future products isn't a good idea either - we just don't
know what they'll look like exactly. In the end it's likely just yet
another function pointer with completely different code, and having less
chipset specific magic constant in struct intel_ring_buffer might even
help.

Now for the trace patch, I definitely want that. But that has a tiny
review comment ;-)
-Daniel

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

Re: [PATCH 5/5] drm/i915: tracepoints for semaphores

[Daniel Vetter]

On Thu, Sep 15, 2011 at 07:09:01PM -0700, Ben Widawsky wrote:
> 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 |    5 +++
>  2 files changed, 66 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index d623fef..47d55fd 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"))))

With the ring->id cleanup, that's now wrong. Also, the other trace events
only report the numeric ring id, so for consistency, I think you can just
drop this.

> +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 = %d, signal mbox 2 = %d",
> +		      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 cbf255e..6bf1dd9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -351,6 +351,9 @@ gen6_add_request(struct intel_ring_buffer *ring,
>  	intel_ring_advance(ring);
>  
>  	*result = seqno;
> +
> +	trace_intel_ringbuffer_add_request(ring, seqno, ring->signal_mbox[0],
> +					   ring->signal_mbox[1]);
>  	return 0;
>  }

We already have trace_i915_gem_request_add in i915_add_request which is
essentially giving out the same information (well, minus the hopefully
correct singal_mbox reg addresses). I think we can drop this one.

> @@ -375,6 +378,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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 5/5] drm/i915: tracepoints for semaphores

[Chris Wilson]

On Fri, 16 Sep 2011 09:58:53 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> We already have trace_i915_gem_request_add in i915_add_request which is
> essentially giving out the same information (well, minus the hopefully
> correct singal_mbox reg addresses). I think we can drop this one.

I thought that as well, but then realised that a complete set of ring
tracepoints would be useful independent of the request/object
tracepoints.

A couple of tracepoints on the gen6 paths is insufficients. ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/5] drm/i915: tracepoints for semaphores

[Daniel Vetter]

On Fri, Sep 16, 2011 at 09:37:41AM +0100, Chris Wilson wrote:
> On Fri, 16 Sep 2011 09:58:53 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > We already have trace_i915_gem_request_add in i915_add_request which is
> > essentially giving out the same information (well, minus the hopefully
> > correct singal_mbox reg addresses). I think we can drop this one.
> 
> I thought that as well, but then realised that a complete set of ring
> tracepoints would be useful independent of the request/object
> tracepoints.
> 
> A couple of tracepoints on the gen6 paths is insufficients. ;-)

Well, I've quickly reviewed them and I think we're mostly covered. Many of
the tracepoints are called from generic code i915_gem.c right before/after
calling into the chipset-specific ringbuffer functions.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 5/5] drm/i915: tracepoints for semaphores

[Chris Wilson]

On Fri, 16 Sep 2011 11:07:29 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Sep 16, 2011 at 09:37:41AM +0100, Chris Wilson wrote:
> > On Fri, 16 Sep 2011 09:58:53 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > We already have trace_i915_gem_request_add in i915_add_request which is
> > > essentially giving out the same information (well, minus the hopefully
> > > correct singal_mbox reg addresses). I think we can drop this one.
> > 
> > I thought that as well, but then realised that a complete set of ring
> > tracepoints would be useful independent of the request/object
> > tracepoints.
> > 
> > A couple of tracepoints on the gen6 paths is insufficients. ;-)
> 
> Well, I've quickly reviewed them and I think we're mostly covered. Many of
> the tracepoints are called from generic code i915_gem.c right before/after
> calling into the chipset-specific ringbuffer functions.

It's just the difference of being able to watch only i915:intel_ring* to
reduce the amount of noise if that is all you are interested in. And you
don't have to remember the relationship between a particular
i915_gem_request and its effect upon the ring.
-Chris 

-- 
Chris Wilson, Intel Open Source Technology Centre