[RFC 1/3] drm/i915: Rename intel_engine_cs.exec_id to uabi_id

[RFC 1/3] drm/i915: Rename intel_engine_cs.exec_id to uabi_id

[Chris Wilson]

We want to refer to the index of the engine consistently throughout the
userspace ABI. We already have such an index through the execbuffer
engine specifier, that needs to be able to refer to each engine
specifically.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c  |  8 ++++----
 drivers/gpu/drm/i915/i915_gem.c         |  2 +-
 drivers/gpu/drm/i915/intel_engine_cs.c  | 14 +++++++-------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  2 +-
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index f110ed6678e9..3cbc7507e4a2 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -1156,8 +1156,8 @@ static bool check_cmd(const struct intel_engine_cs *engine,
 				find_reg(engine, is_master, reg_addr);
 
 			if (!reg) {
-				DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (exec_id=%d)\n",
-						 reg_addr, *cmd, engine->exec_id);
+				DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (%s)\n",
+						 reg_addr, *cmd, engine->name);
 				return false;
 			}
 
@@ -1212,11 +1212,11 @@ static bool check_cmd(const struct intel_engine_cs *engine,
 				desc->bits[i].mask;
 
 			if (dword != desc->bits[i].expected) {
-				DRM_DEBUG_DRIVER("CMD: Rejected command 0x%08X for bitmask 0x%08X (exp=0x%08X act=0x%08X) (exec_id=%d)\n",
+				DRM_DEBUG_DRIVER("CMD: Rejected command 0x%08X for bitmask 0x%08X (exp=0x%08X act=0x%08X) (%s)\n",
 						 *cmd,
 						 desc->bits[i].mask,
 						 desc->bits[i].expected,
-						 dword, engine->exec_id);
+						 dword, engine->name);
 				return false;
 			}
 		}
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c2e5cb529b0f..15282eef5012 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3757,7 +3757,7 @@ __busy_set_if_active(const struct dma_fence *fence,
 	if (i915_gem_request_completed(rq))
 		return 0;
 
-	return flag(rq->engine->exec_id);
+	return flag(rq->engine->uabi_id);
 }
 
 static __always_inline unsigned int
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index bd91aa1f0deb..0f8b3312dad8 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -28,7 +28,7 @@
 
 static const struct engine_info {
 	const char *name;
-	unsigned int exec_id;
+	unsigned int uabi_id;
 	unsigned int hw_id;
 	u32 mmio_base;
 	unsigned irq_shift;
@@ -37,7 +37,7 @@ static const struct engine_info {
 } intel_engines[] = {
 	[RCS] = {
 		.name = "render ring",
-		.exec_id = I915_EXEC_RENDER,
+		.uabi_id = I915_EXEC_RENDER,
 		.hw_id = RCS_HW,
 		.mmio_base = RENDER_RING_BASE,
 		.irq_shift = GEN8_RCS_IRQ_SHIFT,
@@ -46,7 +46,7 @@ static const struct engine_info {
 	},
 	[BCS] = {
 		.name = "blitter ring",
-		.exec_id = I915_EXEC_BLT,
+		.uabi_id = I915_EXEC_BLT,
 		.hw_id = BCS_HW,
 		.mmio_base = BLT_RING_BASE,
 		.irq_shift = GEN8_BCS_IRQ_SHIFT,
@@ -55,7 +55,7 @@ static const struct engine_info {
 	},
 	[VCS] = {
 		.name = "bsd ring",
-		.exec_id = I915_EXEC_BSD,
+		.uabi_id = I915_EXEC_BSD,
 		.hw_id = VCS_HW,
 		.mmio_base = GEN6_BSD_RING_BASE,
 		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
@@ -64,7 +64,7 @@ static const struct engine_info {
 	},
 	[VCS2] = {
 		.name = "bsd2 ring",
-		.exec_id = I915_EXEC_BSD,
+		.uabi_id = I915_EXEC_BSD,
 		.hw_id = VCS2_HW,
 		.mmio_base = GEN8_BSD2_RING_BASE,
 		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
@@ -73,7 +73,7 @@ static const struct engine_info {
 	},
 	[VECS] = {
 		.name = "video enhancement ring",
-		.exec_id = I915_EXEC_VEBOX,
+		.uabi_id = I915_EXEC_VEBOX,
 		.hw_id = VECS_HW,
 		.mmio_base = VEBOX_RING_BASE,
 		.irq_shift = GEN8_VECS_IRQ_SHIFT,
@@ -97,7 +97,7 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 	engine->id = id;
 	engine->i915 = dev_priv;
 	engine->name = info->name;
-	engine->exec_id = info->exec_id;
+	engine->uabi_id = info->uabi_id;
 	engine->hw_id = engine->guc_id = info->hw_id;
 	engine->mmio_base = info->mmio_base;
 	engine->irq_shift = info->irq_shift;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 8db2c205111f..53880389e6c9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -183,7 +183,7 @@ struct intel_engine_cs {
 	struct drm_i915_private *i915;
 	const char	*name;
 	enum intel_engine_id id;
-	unsigned int exec_id;
+	unsigned int uabi_id;
 	unsigned int hw_id;
 	unsigned int guc_id;
 	u32		mmio_base;
-- 
2.11.0

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

[RFC 2/3] drm/i915: Add a routine to map from UABI id to engine

[Chris Wilson]

We have to mix a static UABI engine id with the potential for a varying
hw_id and layout, and so we need a way to map from the userspace id for
an engine to our internal pointers.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_engine_cs.c  | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 0f8b3312dad8..8a197f826d38 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1125,6 +1125,23 @@ void intel_engines_cleanup(struct drm_i915_private *i915)
 		engine->cleanup(engine);
 }
 
+struct intel_engine_cs *
+intel_engine_lookup(struct drm_i915_private *i915, u32 uabi_id)
+{
+	static const enum intel_engine_id uabi_map[] = {
+		[I915_EXEC_DEFAULT]	= RCS,
+		[I915_EXEC_RENDER]	= RCS,
+		[I915_EXEC_BLT]		= BCS,
+		[I915_EXEC_BSD]		= VCS,
+		[I915_EXEC_VEBOX]	= VECS,
+	};
+
+	if (uabi_id >= ARRAY_SIZE(uabi_map))
+		return NULL;
+
+	return i915->engine[uabi_map[uabi_id]];
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_engine.c"
 #endif
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 53880389e6c9..7f8366da5a4c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -671,6 +671,9 @@ static inline u32 *gen8_emit_pipe_control(u32 *batch, u32 flags, u32 offset)
 	return batch + 6;
 }
 
+struct intel_engine_cs *
+intel_engine_lookup(struct drm_i915_private *i915, u32 uabi_id);
+
 bool intel_engine_is_idle(struct intel_engine_cs *engine);
 bool intel_engines_are_idle(struct drm_i915_private *dev_priv);
 
-- 
2.11.0

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

[RFC 3/3] drm/i915: Finally assign BSD2 its own specific UABI identifier

[Chris Wilson]

3 years too late, but hopefully better late than never, start to
rectify the damage caused by commit a8ebba75b358 ("drm/i915: Use the
coarse ping-pong mechanism based on drm fd to dispatch the BSD command
on BDW GT3") and compounded by commit 8d360dffd6d8 ("drm/i915: Specify
bsd rings through exec flag") which did not allocate BSD2 its own
identifier but overloaded the existing BSD uabi id.

Worse, those patches *overrode* existing behaviour (i.e. were not
backwards or forwards compatible) which makes undoing the damage tricky.
As an ABI break seems unavoidable, make it so. The ramification is that
libva on a few bdw/skl boxes will lose access to the second BSD engine
until it is updated to request the second engine explicitly (which is
what libva preferred to do anyway!)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Zhipeng Gong <zhipeng.gong@intel.com>
Cc: Zhao Yakui <yakui.zhao@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch
---
 drivers/gpu/drm/i915/i915_drv.h            |  2 -
 drivers/gpu/drm/i915/i915_gem.c            |  2 -
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 79 +-----------------------------
 drivers/gpu/drm/i915/intel_engine_cs.c     |  3 +-
 include/uapi/drm/i915_drm.h                | 13 ++---
 5 files changed, 11 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 65e3666b9add..8119d74b6a2a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -540,8 +540,6 @@ struct drm_i915_file_private {
 		unsigned boosts;
 	} rps;
 
-	unsigned int bsd_engine;
-
 /* Client can have a maximum of 3 contexts banned before
  * it is denied of creating new contexts. As one context
  * ban needs 4 consecutive hangs, and more if there is
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 15282eef5012..c2a660c7538b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4873,8 +4873,6 @@ int i915_gem_open(struct drm_device *dev, struct drm_file *file)
 	spin_lock_init(&file_priv->mm.lock);
 	INIT_LIST_HEAD(&file_priv->mm.request_list);
 
-	file_priv->bsd_engine = -1;
-
 	ret = i915_gem_context_open(dev, file);
 	if (ret)
 		kfree(file_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 21c1478a6944..a24c3ce3a871 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1948,82 +1948,6 @@ eb_submit(struct i915_execbuffer *eb)
 	return 0;
 }
 
-/**
- * Find one BSD ring to dispatch the corresponding BSD command.
- * The engine index is returned.
- */
-static unsigned int
-gen8_dispatch_bsd_engine(struct drm_i915_private *dev_priv,
-			 struct drm_file *file)
-{
-	struct drm_i915_file_private *file_priv = file->driver_priv;
-
-	/* Check whether the file_priv has already selected one ring. */
-	if ((int)file_priv->bsd_engine < 0)
-		file_priv->bsd_engine = atomic_fetch_xor(1,
-			 &dev_priv->mm.bsd_engine_dispatch_index);
-
-	return file_priv->bsd_engine;
-}
-
-#define I915_USER_RINGS (4)
-
-static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
-	[I915_EXEC_DEFAULT]	= RCS,
-	[I915_EXEC_RENDER]	= RCS,
-	[I915_EXEC_BLT]		= BCS,
-	[I915_EXEC_BSD]		= VCS,
-	[I915_EXEC_VEBOX]	= VECS
-};
-
-static struct intel_engine_cs *
-eb_select_engine(struct drm_i915_private *dev_priv,
-		 struct drm_file *file,
-		 struct drm_i915_gem_execbuffer2 *args)
-{
-	unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK;
-	struct intel_engine_cs *engine;
-
-	if (user_ring_id > I915_USER_RINGS) {
-		DRM_DEBUG("execbuf with unknown ring: %u\n", user_ring_id);
-		return NULL;
-	}
-
-	if ((user_ring_id != I915_EXEC_BSD) &&
-	    ((args->flags & I915_EXEC_BSD_MASK) != 0)) {
-		DRM_DEBUG("execbuf with non bsd ring but with invalid "
-			  "bsd dispatch flags: %d\n", (int)(args->flags));
-		return NULL;
-	}
-
-	if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) {
-		unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
-
-		if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
-			bsd_idx = gen8_dispatch_bsd_engine(dev_priv, file);
-		} else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
-			   bsd_idx <= I915_EXEC_BSD_RING2) {
-			bsd_idx >>= I915_EXEC_BSD_SHIFT;
-			bsd_idx--;
-		} else {
-			DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
-				  bsd_idx);
-			return NULL;
-		}
-
-		engine = dev_priv->engine[_VCS(bsd_idx)];
-	} else {
-		engine = dev_priv->engine[user_ring_map[user_ring_id]];
-	}
-
-	if (!engine) {
-		DRM_DEBUG("execbuf with invalid ring: %u\n", user_ring_id);
-		return NULL;
-	}
-
-	return engine;
-}
-
 static struct dma_fence *
 dma_buf_get_fence(int fd, unsigned int flags)
 {
@@ -2107,7 +2031,8 @@ i915_gem_do_execbuffer(struct drm_device *dev,
 	if (args->flags & I915_EXEC_IS_PINNED)
 		eb.dispatch_flags |= I915_DISPATCH_PINNED;
 
-	eb.engine = eb_select_engine(eb.i915, file, args);
+	eb.engine = intel_engine_lookup(eb.i915,
+					args->flags & I915_EXEC_RING_MASK);
 	if (!eb.engine)
 		return -EINVAL;
 
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 8a197f826d38..c76a64483d64 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -64,7 +64,7 @@ static const struct engine_info {
 	},
 	[VCS2] = {
 		.name = "bsd2 ring",
-		.uabi_id = I915_EXEC_BSD,
+		.uabi_id = I915_EXEC_BSD2,
 		.hw_id = VCS2_HW,
 		.mmio_base = GEN8_BSD2_RING_BASE,
 		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
@@ -1134,6 +1134,7 @@ intel_engine_lookup(struct drm_i915_private *i915, u32 uabi_id)
 		[I915_EXEC_BLT]		= BCS,
 		[I915_EXEC_BSD]		= VCS,
 		[I915_EXEC_VEBOX]	= VECS,
+		[I915_EXEC_BSD2]	= VCS2,
 	};
 
 	if (uabi_id >= ARRAY_SIZE(uabi_map))
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index deab27c74480..72460826f818 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -870,12 +870,13 @@ struct drm_i915_gem_execbuffer2 {
 	__u32 num_cliprects;
 	/** This is a struct drm_clip_rect *cliprects */
 	__u64 cliprects_ptr;
-#define I915_EXEC_RING_MASK              (7<<0)
-#define I915_EXEC_DEFAULT                (0<<0)
-#define I915_EXEC_RENDER                 (1<<0)
-#define I915_EXEC_BSD                    (2<<0)
-#define I915_EXEC_BLT                    (3<<0)
-#define I915_EXEC_VEBOX                  (4<<0)
+#define I915_EXEC_RING_MASK		0x7
+#define I915_EXEC_DEFAULT		0
+#define I915_EXEC_RENDER		1
+#define I915_EXEC_BSD			2
+#define I915_EXEC_BLT			3
+#define I915_EXEC_VEBOX			4
+#define I915_EXEC_BSD2			5
 
 /* Used for switching the constants addressing mode on gen4+ RENDER ring.
  * Gen6+ only supports relative addressing to dynamic state (default) and
-- 
2.11.0

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

✓ Fi.CI.BAT: success for series starting with [RFC,1/3] drm/i915: Rename intel_engine_cs.exec_id to uabi_id

[Patchwork]

== Series Details ==

Series: series starting with [RFC,1/3] drm/i915: Rename intel_engine_cs.exec_id to uabi_id
URL   : https://patchwork.freedesktop.org/series/22088/
State : success

== Summary ==

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

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 461s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 462s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 584s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 534s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 593s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 502s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 506s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 435s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 437s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 448s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 509s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 497s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 486s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time: 594s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 485s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 598s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 488s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 522s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 460s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 548s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 430s

2f9f22b419350cafb06ba7e5342bc461fcb0afca drm-tip: 2017y-03m-29d-12h-52m-56s UTC integration manifest
18a8f8e drm/i915: Rename intel_engine_cs.exec_id to uabi_id

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4341/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC 1/3] drm/i915: Rename intel_engine_cs.exec_id to uabi_id

[Joonas Lahtinen]

On ke, 2017-03-29 at 14:58 +0100, Chris Wilson wrote:
> We want to refer to the index of the engine consistently throughout the
> userspace ABI. We already have such an index through the execbuffer
> engine specifier, that needs to be able to refer to each engine
> specifically.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Please re-spin this over Oscar's series (and R-b the remaining two
patches).

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx