Reduce usage of the name 'ring' for engines et al

Reduce usage of the name 'ring' for engines et al

[Dave Gordon]

Chris Wilson is trying to convert 'ringbuffer' to 'ring', but at present
there's rather too much legacy code using 'ring' for various other things,
usually engines or engine-ids. This patchset converts some of them (but
not as yet the gpu_error or trace code).

Chris: what is your prefered name for a local holding an engine id?
'engine_id' is obvious, but seems overly long and clunky. Anything better?

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

[PATCH 1/3] drm/i915: rename macro parameter(ring) to (engine)

[Dave Gordon]

'ring' is an old deprecated term for a GPU engine. Here we make the
terminology more consistent by renaming the 'ring' parameter of lots of
macros that calculate addresses within the MMIO space of an engine.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 14 +++++++-------
 drivers/gpu/drm/i915/intel_lrc.h        | 16 ++++++++--------
 drivers/gpu/drm/i915/intel_ringbuffer.h | 24 ++++++++++++------------
 3 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8bfde75..559c9d7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -186,13 +186,13 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define  GEN9_GRDOM_GUC			(1 << 5)
 #define  GEN8_GRDOM_MEDIA2		(1 << 7)
 
-#define RING_PP_DIR_BASE(ring)		_MMIO((ring)->mmio_base+0x228)
-#define RING_PP_DIR_BASE_READ(ring)	_MMIO((ring)->mmio_base+0x518)
-#define RING_PP_DIR_DCLV(ring)		_MMIO((ring)->mmio_base+0x220)
+#define RING_PP_DIR_BASE(engine)	_MMIO((engine)->mmio_base+0x228)
+#define RING_PP_DIR_BASE_READ(engine)	_MMIO((engine)->mmio_base+0x518)
+#define RING_PP_DIR_DCLV(engine)	_MMIO((engine)->mmio_base+0x220)
 #define   PP_DIR_DCLV_2G		0xffffffff
 
-#define GEN8_RING_PDP_UDW(ring, n)	_MMIO((ring)->mmio_base+0x270 + (n) * 8 + 4)
-#define GEN8_RING_PDP_LDW(ring, n)	_MMIO((ring)->mmio_base+0x270 + (n) * 8)
+#define GEN8_RING_PDP_UDW(engine, n)	_MMIO((engine)->mmio_base+0x270 + (n) * 8 + 4)
+#define GEN8_RING_PDP_LDW(engine, n)	_MMIO((engine)->mmio_base+0x270 + (n) * 8)
 
 #define GEN8_R_PWR_CLK_STATE		_MMIO(0x20C8)
 #define   GEN8_RPCS_ENABLE		(1 << 31)
@@ -1647,7 +1647,7 @@ enum skl_disp_power_wells {
 #define   ARB_MODE_BWGTLB_DISABLE (1<<9)
 #define   ARB_MODE_SWIZZLE_BDW	(1<<1)
 #define RENDER_HWS_PGA_GEN7	_MMIO(0x04080)
-#define RING_FAULT_REG(ring)	_MMIO(0x4094 + 0x100*(ring)->id)
+#define RING_FAULT_REG(engine)	_MMIO(0x4094 + 0x100*(engine)->id)
 #define   RING_FAULT_GTTSEL_MASK (1<<11)
 #define   RING_FAULT_SRCID(x)	(((x) >> 3) & 0xff)
 #define   RING_FAULT_FAULT_TYPE(x) (((x) >> 1) & 0x3)
@@ -1842,7 +1842,7 @@ enum skl_disp_power_wells {
 
 #define GFX_MODE	_MMIO(0x2520)
 #define GFX_MODE_GEN7	_MMIO(0x229c)
-#define RING_MODE_GEN7(ring)	_MMIO((ring)->mmio_base+0x29c)
+#define RING_MODE_GEN7(engine)	_MMIO((engine)->mmio_base+0x29c)
 #define   GFX_RUN_LIST_ENABLE		(1<<15)
 #define   GFX_INTERRUPT_STEERING	(1<<14)
 #define   GFX_TLB_INVALIDATE_EXPLICIT	(1<<13)
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index aa3ac02..3828730 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -29,17 +29,17 @@
 #define GEN8_LR_CONTEXT_ALIGN 4096
 
 /* Execlists regs */
-#define RING_ELSP(ring)				_MMIO((ring)->mmio_base + 0x230)
-#define RING_EXECLIST_STATUS_LO(ring)		_MMIO((ring)->mmio_base + 0x234)
-#define RING_EXECLIST_STATUS_HI(ring)		_MMIO((ring)->mmio_base + 0x234 + 4)
-#define RING_CONTEXT_CONTROL(ring)		_MMIO((ring)->mmio_base + 0x244)
+#define RING_ELSP(engine)			_MMIO((engine)->mmio_base + 0x230)
+#define RING_EXECLIST_STATUS_LO(engine)		_MMIO((engine)->mmio_base + 0x234)
+#define RING_EXECLIST_STATUS_HI(engine)		_MMIO((engine)->mmio_base + 0x234 + 4)
+#define RING_CONTEXT_CONTROL(engine)		_MMIO((engine)->mmio_base + 0x244)
 #define	  CTX_CTRL_INHIBIT_SYN_CTX_SWITCH	(1 << 3)
 #define	  CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT	(1 << 0)
 #define   CTX_CTRL_RS_CTX_ENABLE                (1 << 1)
-#define RING_CONTEXT_STATUS_BUF_BASE(ring)	_MMIO((ring)->mmio_base + 0x370)
-#define RING_CONTEXT_STATUS_BUF_LO(ring, i)	_MMIO((ring)->mmio_base + 0x370 + (i) * 8)
-#define RING_CONTEXT_STATUS_BUF_HI(ring, i)	_MMIO((ring)->mmio_base + 0x370 + (i) * 8 + 4)
-#define RING_CONTEXT_STATUS_PTR(ring)		_MMIO((ring)->mmio_base + 0x3a0)
+#define RING_CONTEXT_STATUS_BUF_BASE(engine)	_MMIO((engine)->mmio_base + 0x370)
+#define RING_CONTEXT_STATUS_BUF_LO(engine, i)	_MMIO((engine)->mmio_base + 0x370 + (i) * 8)
+#define RING_CONTEXT_STATUS_BUF_HI(engine, i)	_MMIO((engine)->mmio_base + 0x370 + (i) * 8 + 4)
+#define RING_CONTEXT_STATUS_PTR(engine)		_MMIO((engine)->mmio_base + 0x3a0)
 
 /* The docs specify that the write pointer wraps around after 5h, "After status
  * is written out to the last available status QW at offset 5h, this pointer
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 05bab8b..4671fb8 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -31,23 +31,23 @@ struct  intel_hw_status_page {
 	struct		drm_i915_gem_object *obj;
 };
 
-#define I915_READ_TAIL(ring) I915_READ(RING_TAIL((ring)->mmio_base))
-#define I915_WRITE_TAIL(ring, val) I915_WRITE(RING_TAIL((ring)->mmio_base), val)
+#define I915_READ_TAIL(engine) I915_READ(RING_TAIL((engine)->mmio_base))
+#define I915_WRITE_TAIL(engine, val) I915_WRITE(RING_TAIL((engine)->mmio_base), val)
 
-#define I915_READ_START(ring) I915_READ(RING_START((ring)->mmio_base))
-#define I915_WRITE_START(ring, val) I915_WRITE(RING_START((ring)->mmio_base), val)
+#define I915_READ_START(engine) I915_READ(RING_START((engine)->mmio_base))
+#define I915_WRITE_START(engine, val) I915_WRITE(RING_START((engine)->mmio_base), val)
 
-#define I915_READ_HEAD(ring)  I915_READ(RING_HEAD((ring)->mmio_base))
-#define I915_WRITE_HEAD(ring, val) I915_WRITE(RING_HEAD((ring)->mmio_base), val)
+#define I915_READ_HEAD(engine)  I915_READ(RING_HEAD((engine)->mmio_base))
+#define I915_WRITE_HEAD(engine, val) I915_WRITE(RING_HEAD((engine)->mmio_base), val)
 
-#define I915_READ_CTL(ring) I915_READ(RING_CTL((ring)->mmio_base))
-#define I915_WRITE_CTL(ring, val) I915_WRITE(RING_CTL((ring)->mmio_base), val)
+#define I915_READ_CTL(engine) I915_READ(RING_CTL((engine)->mmio_base))
+#define I915_WRITE_CTL(engine, val) I915_WRITE(RING_CTL((engine)->mmio_base), val)
 
-#define I915_READ_IMR(ring) I915_READ(RING_IMR((ring)->mmio_base))
-#define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val)
+#define I915_READ_IMR(engine) I915_READ(RING_IMR((engine)->mmio_base))
+#define I915_WRITE_IMR(engine, val) I915_WRITE(RING_IMR((engine)->mmio_base), val)
 
-#define I915_READ_MODE(ring) I915_READ(RING_MI_MODE((ring)->mmio_base))
-#define I915_WRITE_MODE(ring, val) I915_WRITE(RING_MI_MODE((ring)->mmio_base), val)
+#define I915_READ_MODE(engine) I915_READ(RING_MI_MODE((engine)->mmio_base))
+#define I915_WRITE_MODE(engine, val) I915_WRITE(RING_MI_MODE((engine)->mmio_base), val)
 
 /* seqno size is actually only a uint32, but since we plan to use MI_FLUSH_DW to
  * do the writes, and that must have qw aligned offsets, simply pretend it's 8b.
-- 
1.9.1

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

Re: [PATCH 1/3] drm/i915: rename macro parameter(ring) to (engine)

[Chris Wilson]

On Wed, Jul 20, 2016 at 06:16:05PM +0100, Dave Gordon wrote:
> 'ring' is an old deprecated term for a GPU engine. Here we make the
> terminology more consistent by renaming the 'ring' parameter of lots of
> macros that calculate addresses within the MMIO space of an engine.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 14 +++++++-------
>  drivers/gpu/drm/i915/intel_lrc.h        | 16 ++++++++--------
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 24 ++++++++++++------------
>  3 files changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8bfde75..559c9d7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -186,13 +186,13 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define  GEN9_GRDOM_GUC			(1 << 5)
>  #define  GEN8_GRDOM_MEDIA2		(1 << 7)
>  
> -#define RING_PP_DIR_BASE(ring)		_MMIO((ring)->mmio_base+0x228)
> -#define RING_PP_DIR_BASE_READ(ring)	_MMIO((ring)->mmio_base+0x518)
> -#define RING_PP_DIR_DCLV(ring)		_MMIO((ring)->mmio_base+0x220)
> +#define RING_PP_DIR_BASE(engine)	_MMIO((engine)->mmio_base+0x228)
> +#define RING_PP_DIR_BASE_READ(engine)	_MMIO((engine)->mmio_base+0x518)
> +#define RING_PP_DIR_DCLV(engine)	_MMIO((engine)->mmio_base+0x220)
>  #define   PP_DIR_DCLV_2G		0xffffffff

Since these registers don't refer to the actual ring buffer, but the
state of the engine itself, I was thinking about updating the RING_
prefix as well. I didn't think it is worth it atm.
-Chris

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

[PATCH 2/3] drm/i915: rename 'ring' where it refers to an engine or engine_id

[Dave Gordon]

'ring' is an old deprecated term for a GPU engine. Chris Wilson wants to
use the name for what is currently known as an intel_ringbuffer, but it
will be dreadfully confusing if some rings are ringbuffers but other
rings are still engines. So this patch changes the names of a bunch of
parameters called 'ring' to either 'engine' or 'engine_id' according to
what they actually are.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_mocs.c       |  6 +++---
 drivers/gpu/drm/i915/intel_mocs.h       |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h | 12 ++++++------
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index 2280c32..bd46968 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -204,9 +204,9 @@ static bool get_mocs_settings(struct drm_i915_private *dev_priv,
 	return result;
 }
 
-static i915_reg_t mocs_register(enum intel_engine_id ring, int index)
+static i915_reg_t mocs_register(enum intel_engine_id engine_id, int index)
 {
-	switch (ring) {
+	switch (engine_id) {
 	case RCS:
 		return GEN9_GFX_MOCS(index);
 	case VCS:
@@ -218,7 +218,7 @@ static i915_reg_t mocs_register(enum intel_engine_id ring, int index)
 	case VCS2:
 		return GEN9_MFX1_MOCS(index);
 	default:
-		MISSING_CASE(ring);
+		MISSING_CASE(engine_id);
 		return INVALID_MMIO_REG;
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_mocs.h b/drivers/gpu/drm/i915/intel_mocs.h
index 4640299..a8bd9f7 100644
--- a/drivers/gpu/drm/i915/intel_mocs.h
+++ b/drivers/gpu/drm/i915/intel_mocs.h
@@ -54,6 +54,6 @@
 
 int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req);
 void intel_mocs_init_l3cc_table(struct drm_device *dev);
-int intel_mocs_init_engine(struct intel_engine_cs *ring);
+int intel_mocs_init_engine(struct intel_engine_cs *engine);
 
 #endif
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index b844e69..a6f7db2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1581,7 +1581,7 @@ static inline bool i915_gem_has_seqno_wrapped(struct drm_i915_private *dev_priv,
 }
 
 static void
-gen5_seqno_barrier(struct intel_engine_cs *ring)
+gen5_seqno_barrier(struct intel_engine_cs *engine)
 {
 	/* MI_STORE are internally buffered by the GPU and not flushed
 	 * either by MI_FLUSH or SyncFlush or any other combination of
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 4671fb8..0f80194 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -197,14 +197,14 @@ struct intel_engine_cs {
 
 	u32             irq_keep_mask; /* always keep these interrupts */
 	u32		irq_enable_mask; /* bitmask to enable ring interrupt */
-	void		(*irq_enable)(struct intel_engine_cs *ring);
-	void		(*irq_disable)(struct intel_engine_cs *ring);
+	void		(*irq_enable)(struct intel_engine_cs *engine);
+	void		(*irq_disable)(struct intel_engine_cs *engine);
 
-	int		(*init_hw)(struct intel_engine_cs *ring);
+	int		(*init_hw)(struct intel_engine_cs *engine);
 
 	int		(*init_context)(struct drm_i915_gem_request *req);
 
-	void		(*write_tail)(struct intel_engine_cs *ring,
+	void		(*write_tail)(struct intel_engine_cs *engine,
 				      u32 value);
 	int __must_check (*flush)(struct drm_i915_gem_request *req,
 				  u32	invalidate_domains,
@@ -216,14 +216,14 @@ struct intel_engine_cs {
 	 * seen value is good enough. Note that the seqno will always be
 	 * monotonic, even if not coherent.
 	 */
-	void		(*irq_seqno_barrier)(struct intel_engine_cs *ring);
+	void		(*irq_seqno_barrier)(struct intel_engine_cs *engine);
 	int		(*dispatch_execbuffer)(struct drm_i915_gem_request *req,
 					       u64 offset, u32 length,
 					       unsigned dispatch_flags);
 #define I915_DISPATCH_SECURE 0x1
 #define I915_DISPATCH_PINNED 0x2
 #define I915_DISPATCH_RS     0x4
-	void		(*cleanup)(struct intel_engine_cs *ring);
+	void		(*cleanup)(struct intel_engine_cs *engine);
 
 	/* GEN8 signal/wait table - never trust comments!
 	 *	  signal to	signal to    signal to   signal to      signal to
-- 
1.9.1

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

Re: [PATCH 2/3] drm/i915: rename 'ring' where it refers to an engine or engine_id

[Chris Wilson]

On Wed, Jul 20, 2016 at 06:16:06PM +0100, Dave Gordon wrote:
> 'ring' is an old deprecated term for a GPU engine. Chris Wilson wants to
> use the name for what is currently known as an intel_ringbuffer, but it
> will be dreadfully confusing if some rings are ringbuffers but other
> rings are still engines. So this patch changes the names of a bunch of
> parameters called 'ring' to either 'engine' or 'engine_id' according to
> what they actually are.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

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

[PATCH 3/3] drm/i915: rename & update eb_select_ring()

[Dave Gordon]

'ring' is an old deprecated term for a GPU engine, so we're trying to
phase out all such terminology. eb_select_ring() not only has 'ring'
(meaning engine) in its name, but it has an ugly calling convention
whereby it returns an errno and stores a pointer-to-engine indirectly
through an output parameter. As there is only one error it ever returns
(-EINVAL), we can make it return the pointer directly, and have the
caller pass back the error code -EINVAL if the pointer result is NULL.

Thus we can replace
-	ret = eb_select_ring(dev_priv, file, args, &engine);
-	if (ret)
-		return ret;
with
+	engine = eb_select_engine(dev_priv, file, args);
+	if (!engine)
+		return -EINVAL;
for increased clarity and maybe save a few cycles too.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 32 +++++++++++++++---------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6482ec2..f8d8ae3 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1380,24 +1380,24 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	[I915_EXEC_VEBOX]	= VECS
 };
 
-static int
-eb_select_ring(struct drm_i915_private *dev_priv,
-	       struct drm_file *file,
-	       struct drm_i915_gem_execbuffer2 *args,
-	       struct intel_engine_cs **ring)
+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 -EINVAL;
+		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 -EINVAL;
+		return NULL;
 	}
 
 	if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) {
@@ -1412,20 +1412,20 @@ static bool only_mappable_for_reloc(unsigned int flags)
 		} else {
 			DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
 				  bsd_idx);
-			return -EINVAL;
+			return NULL;
 		}
 
-		*ring = &dev_priv->engine[_VCS(bsd_idx)];
+		engine = &dev_priv->engine[_VCS(bsd_idx)];
 	} else {
-		*ring = &dev_priv->engine[user_ring_map[user_ring_id]];
+		engine = &dev_priv->engine[user_ring_map[user_ring_id]];
 	}
 
-	if (!intel_engine_initialized(*ring)) {
+	if (!intel_engine_initialized(engine)) {
 		DRM_DEBUG("execbuf with invalid ring: %u\n", user_ring_id);
-		return -EINVAL;
+		return NULL;
 	}
 
-	return 0;
+	return engine;
 }
 
 static int
@@ -1467,9 +1467,9 @@ static bool only_mappable_for_reloc(unsigned int flags)
 	if (args->flags & I915_EXEC_IS_PINNED)
 		dispatch_flags |= I915_DISPATCH_PINNED;
 
-	ret = eb_select_ring(dev_priv, file, args, &engine);
-	if (ret)
-		return ret;
+	engine = eb_select_engine(dev_priv, file, args);
+	if (!engine)
+		return -EINVAL;
 
 	if (args->buffer_count < 1) {
 		DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
-- 
1.9.1

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

Re: [PATCH 3/3] drm/i915: rename & update eb_select_ring()

[Chris Wilson]

On Wed, Jul 20, 2016 at 06:16:07PM +0100, Dave Gordon wrote:
> 'ring' is an old deprecated term for a GPU engine, so we're trying to
> phase out all such terminology. eb_select_ring() not only has 'ring'
> (meaning engine) in its name, but it has an ugly calling convention
> whereby it returns an errno and stores a pointer-to-engine indirectly
> through an output parameter. As there is only one error it ever returns
> (-EINVAL), we can make it return the pointer directly, and have the
> caller pass back the error code -EINVAL if the pointer result is NULL.
> 
> Thus we can replace
> -	ret = eb_select_ring(dev_priv, file, args, &engine);
> -	if (ret)
> -		return ret;
> with
> +	engine = eb_select_engine(dev_priv, file, args);
> +	if (!engine)
> +		return -EINVAL;
> for increased clarity and maybe save a few cycles too.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

Thanks. I feel foolish for missing that easy transform before.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

There's a bsd_ring buried beneath here as well...
-Chris

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

Re: [PATCH 3/3] drm/i915: rename & update eb_select_ring()

[Tvrtko Ursulin]

On 20/07/16 18:31, Chris Wilson wrote:
> On Wed, Jul 20, 2016 at 06:16:07PM +0100, Dave Gordon wrote:
>> 'ring' is an old deprecated term for a GPU engine, so we're trying to
>> phase out all such terminology. eb_select_ring() not only has 'ring'
>> (meaning engine) in its name, but it has an ugly calling convention
>> whereby it returns an errno and stores a pointer-to-engine indirectly
>> through an output parameter. As there is only one error it ever returns
>> (-EINVAL), we can make it return the pointer directly, and have the
>> caller pass back the error code -EINVAL if the pointer result is NULL.
>>
>> Thus we can replace
>> -	ret = eb_select_ring(dev_priv, file, args, &engine);
>> -	if (ret)
>> -		return ret;
>> with
>> +	engine = eb_select_engine(dev_priv, file, args);
>> +	if (!engine)
>> +		return -EINVAL;
>> for increased clarity and maybe save a few cycles too.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>
> Thanks. I feel foolish for missing that easy transform before.
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> There's a bsd_ring buried beneath here as well...

The ugly calling convention was my doing in:

   commit de1add360522c876c25ef2bbbbab1c94bdb509ab
   Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
   Date:   Fri Jan 15 15:12:50 2016 +0000

       drm/i915: Decouple execbuf uAPI from internal implementation

And the reason for it was to avoid growing the text size of 
i915_gem_do_execbuffer while at the same time extracting all the engine 
selection logic into a separate function. For some reason GCC most liked 
it like that. Or maybe I was only trying the ERR_PTR route, not the 
NULL/ptr return. Don't remember now.

Anyway, I don't mind, just providing reasoning for the "ugly" calling 
convention.

Regards,

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

Re: Reduce usage of the name 'ring' for engines et al

[Chris Wilson]

On Wed, Jul 20, 2016 at 06:16:04PM +0100, Dave Gordon wrote:
> Chris Wilson is trying to convert 'ringbuffer' to 'ring', but at present
> there's rather too much legacy code using 'ring' for various other things,
> usually engines or engine-ids. This patchset converts some of them (but
> not as yet the gpu_error or trace code).

For trace, I think we want to be a little more relaxed, just so we avoid
user visible changes without a strong justification.
 
> Chris: what is your prefered name for a local holding an engine id?
> 'engine_id' is obvious, but seems overly long and clunky. Anything better?

I was going along the lines of int idx = req->engine->id. Of course,
that is only clear in simple cases. In more complex stacks, engine_id
wins - but those are fortunately rare.
-chris

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

✓ Ro.CI.BAT: success for series starting with [1/3] drm/i915: rename macro parameter(ring) to (engine)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: rename macro parameter(ring) to (engine)
URL   : https://patchwork.freedesktop.org/series/10101/
State : success

== Summary ==

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


fi-hsw-i7-4770k  total:244  pass:216  dwarn:0   dfail:0   fail:8   skip:20 
fi-kbl-qkkr      total:244  pass:179  dwarn:27  dfail:1   fail:9   skip:28 
fi-skl-i5-6260u  total:244  pass:224  dwarn:0   dfail:0   fail:8   skip:12 
fi-skl-i7-6700k  total:244  pass:210  dwarn:0   dfail:0   fail:8   skip:26 
fi-snb-i7-2600   total:244  pass:196  dwarn:0   dfail:0   fail:8   skip:40 
ro-bdw-i5-5250u  total:244  pass:219  dwarn:4   dfail:0   fail:8   skip:13 
ro-bdw-i7-5600u  total:244  pass:204  dwarn:0   dfail:0   fail:8   skip:32 
ro-bsw-n3050     total:218  pass:173  dwarn:0   dfail:0   fail:2   skip:42 
ro-byt-n2820     total:244  pass:197  dwarn:0   dfail:0   fail:9   skip:38 
ro-hsw-i3-4010u  total:244  pass:212  dwarn:0   dfail:0   fail:8   skip:24 
ro-hsw-i7-4770r  total:244  pass:212  dwarn:0   dfail:0   fail:8   skip:24 
ro-ilk1-i5-650   total:239  pass:172  dwarn:0   dfail:0   fail:9   skip:58 
ro-ivb-i7-3770   total:244  pass:203  dwarn:0   dfail:0   fail:8   skip:33 
ro-skl3-i5-6260u total:244  pass:224  dwarn:0   dfail:0   fail:8   skip:12 
ro-snb-i7-2620M  total:244  pass:193  dwarn:0   dfail:0   fail:9   skip:42 
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1549/

621e9dc drm-intel-nightly: 2016y-07m-20d-19h-19m-37s UTC integration manifest
b261a98 drm/i915: rename & update eb_select_ring()
a062554 drm/i915: rename 'ring' where it refers to an engine or engine_id
24e57f2 drm/i915: rename macro parameter(ring) to (engine)

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

Re: ✓ Ro.CI.BAT: success for series starting with [1/3] drm/i915: rename macro parameter(ring) to (engine)

[Chris Wilson]

On Thu, Jul 21, 2016 at 05:49:32AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/3] drm/i915: rename macro parameter(ring) to (engine)
> URL   : https://patchwork.freedesktop.org/series/10101/
> State : success

Pick these up, just a few more stray rings left.
-Chris

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