diff mbox

[4/5] drm/i915: Split the engine info table in two levels, using class + instance

Message ID 1491490816-26965-5-git-send-email-oscar.mateo@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

oscar.mateo@intel.com April 6, 2017, 3 p.m. UTC
There are some properties that logically belong to the engine class, and some
that belong to the engine instance. Make it explicit.

v2: Commit message (Tvrtko)

v3:
  - Rebased
  - Exec/uabi id should be per instance (Chris)

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c | 71 +++++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 27 deletions(-)

Comments

Tvrtko Ursulin April 7, 2017, 8:14 a.m. UTC | #1
On 06/04/2017 16:00, Oscar Mateo wrote:
> There are some properties that logically belong to the engine class, and some
> that belong to the engine instance. Make it explicit.
>
> v2: Commit message (Tvrtko)
>
> v3:
>   - Rebased
>   - Exec/uabi id should be per instance (Chris)
>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c | 71 +++++++++++++++++++++-------------
>  1 file changed, 44 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index c6a73d0..6eab22d 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -26,71 +26,84 @@
>  #include "intel_ringbuffer.h"
>  #include "intel_lrc.h"
>
> -static const struct engine_info {
> +struct engine_class_info {
>  	const char *name;
> +	int (*init_legacy)(struct intel_engine_cs *engine);
> +	int (*init_execlists)(struct intel_engine_cs *engine);
> +};
> +
> +static const struct engine_class_info intel_engine_classes[] = {
> +	[RENDER_CLASS] = {
> +		.name = "rcs",
> +		.init_execlists = logical_render_ring_init,
> +		.init_legacy = intel_init_render_ring_buffer,
> +	},
> +	[COPY_ENGINE_CLASS] = {
> +		.name = "bcs",
> +		.init_execlists = logical_xcs_ring_init,
> +		.init_legacy = intel_init_blt_ring_buffer,
> +	},
> +	[VIDEO_DECODE_CLASS] = {
> +		.name = "vcs",
> +		.init_execlists = logical_xcs_ring_init,
> +		.init_legacy = intel_init_bsd_ring_buffer,
> +	},
> +	[VIDEO_ENHANCEMENT_CLASS] = {
> +		.name = "vecs",
> +		.init_execlists = logical_xcs_ring_init,
> +		.init_legacy = intel_init_vebox_ring_buffer,
> +	},
> +};
> +
> +struct engine_info {
>  	unsigned int exec_id;
>  	unsigned int hw_id;
>  	enum intel_engine_class class;
>  	u8 instance;
>  	u32 mmio_base;
>  	unsigned irq_shift;
> -	int (*init_legacy)(struct intel_engine_cs *engine);
> -	int (*init_execlists)(struct intel_engine_cs *engine);
> -} intel_engines[] = {
> +};
> +
> +static const struct engine_info intel_engines[] = {
>  	[RCS] = {
> -		.name = "rcs",
> -		.hw_id = RCS_HW,
>  		.exec_id = I915_EXEC_RENDER,
> +		.hw_id = RCS_HW,

One more bit of polish - avoid re-ordering the two fields here and below 
for a smaller diff. Or it is a deliberate choice to re-order?

Regards,

Tvrtko

>  		.class = RENDER_CLASS,
>  		.instance = 0,
>  		.mmio_base = RENDER_RING_BASE,
>  		.irq_shift = GEN8_RCS_IRQ_SHIFT,
> -		.init_execlists = logical_render_ring_init,
> -		.init_legacy = intel_init_render_ring_buffer,
>  	},
>  	[BCS] = {
> -		.name = "bcs",
> -		.hw_id = BCS_HW,
>  		.exec_id = I915_EXEC_BLT,
> +		.hw_id = BCS_HW,
>  		.class = COPY_ENGINE_CLASS,
>  		.instance = 0,
>  		.mmio_base = BLT_RING_BASE,
>  		.irq_shift = GEN8_BCS_IRQ_SHIFT,
> -		.init_execlists = logical_xcs_ring_init,
> -		.init_legacy = intel_init_blt_ring_buffer,
>  	},
>  	[VCS] = {
> -		.name = "vcs",
> -		.hw_id = VCS_HW,
>  		.exec_id = I915_EXEC_BSD,
> +		.hw_id = VCS_HW,
>  		.class = VIDEO_DECODE_CLASS,
>  		.instance = 0,
>  		.mmio_base = GEN6_BSD_RING_BASE,
>  		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
> -		.init_execlists = logical_xcs_ring_init,
> -		.init_legacy = intel_init_bsd_ring_buffer,
>  	},
>  	[VCS2] = {
> -		.name = "vcs",
> -		.hw_id = VCS2_HW,
>  		.exec_id = I915_EXEC_BSD,
> +		.hw_id = VCS2_HW,
>  		.class = VIDEO_DECODE_CLASS,
>  		.instance = 1,
>  		.mmio_base = GEN8_BSD2_RING_BASE,
>  		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
> -		.init_execlists = logical_xcs_ring_init,
> -		.init_legacy = intel_init_bsd_ring_buffer,
>  	},
>  	[VECS] = {
> -		.name = "vecs",
> -		.hw_id = VECS_HW,
>  		.exec_id = I915_EXEC_VEBOX,
> +		.hw_id = VECS_HW,
>  		.class = VIDEO_ENHANCEMENT_CLASS,
>  		.instance = 0,
>  		.mmio_base = VEBOX_RING_BASE,
>  		.irq_shift = GEN8_VECS_IRQ_SHIFT,
> -		.init_execlists = logical_xcs_ring_init,
> -		.init_legacy = intel_init_vebox_ring_buffer,
>  	},
>  };
>
> @@ -99,6 +112,8 @@
>  		   enum intel_engine_id id)
>  {
>  	const struct engine_info *info = &intel_engines[id];
> +	const struct engine_class_info *class_info =
> +				&intel_engine_classes[info->class];
>  	struct intel_engine_cs *engine;
>
>  	GEM_BUG_ON(dev_priv->engine[id]);
> @@ -109,7 +124,7 @@
>  	engine->id = id;
>  	engine->i915 = dev_priv;
>  	snprintf(engine->name, sizeof(engine->name), "%s%u",
> -				info->name, info->instance);
> +				class_info->name, info->instance);
>  	engine->exec_id = info->exec_id;
>  	engine->hw_id = engine->guc_id = info->hw_id;
>  	engine->mmio_base = info->mmio_base;
> @@ -190,12 +205,14 @@ int intel_engines_init(struct drm_i915_private *dev_priv)
>  	int err = 0;
>
>  	for_each_engine(engine, dev_priv, id) {
> +		const struct engine_class_info *class_info =
> +					&intel_engine_classes[engine->class];
>  		int (*init)(struct intel_engine_cs *engine);
>
>  		if (i915.enable_execlists)
> -			init = intel_engines[id].init_execlists;
> +			init = class_info->init_execlists;
>  		else
> -			init = intel_engines[id].init_legacy;
> +			init = class_info->init_legacy;
>  		if (!init) {
>  			kfree(engine);
>  			dev_priv->engine[id] = NULL;
>
Michal Wajdeczko April 7, 2017, 10:43 a.m. UTC | #2
On Thu, Apr 06, 2017 at 08:00:15AM -0700, Oscar Mateo wrote:
> There are some properties that logically belong to the engine class, and some
> that belong to the engine instance. Make it explicit.
> 
> v2: Commit message (Tvrtko)
> 
> v3:
>   - Rebased
>   - Exec/uabi id should be per instance (Chris)
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c | 71 +++++++++++++++++++++-------------
>  1 file changed, 44 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index c6a73d0..6eab22d 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -26,71 +26,84 @@
>  #include "intel_ringbuffer.h"
>  #include "intel_lrc.h"
>  
> -static const struct engine_info {
> +struct engine_class_info {
>  	const char *name;
> +	int (*init_legacy)(struct intel_engine_cs *engine);
> +	int (*init_execlists)(struct intel_engine_cs *engine);
> +};
> +
> +static const struct engine_class_info intel_engine_classes[] = {
> +	[RENDER_CLASS] = {
> +		.name = "rcs",
> +		.init_execlists = logical_render_ring_init,
> +		.init_legacy = intel_init_render_ring_buffer,
> +	},
> +	[COPY_ENGINE_CLASS] = {
> +		.name = "bcs",
> +		.init_execlists = logical_xcs_ring_init,
> +		.init_legacy = intel_init_blt_ring_buffer,
> +	},
> +	[VIDEO_DECODE_CLASS] = {
> +		.name = "vcs",
> +		.init_execlists = logical_xcs_ring_init,
> +		.init_legacy = intel_init_bsd_ring_buffer,
> +	},
> +	[VIDEO_ENHANCEMENT_CLASS] = {
> +		.name = "vecs",
> +		.init_execlists = logical_xcs_ring_init,
> +		.init_legacy = intel_init_vebox_ring_buffer,
> +	},
> +};
> +
> +struct engine_info {
>  	unsigned int exec_id;
>  	unsigned int hw_id;
>  	enum intel_engine_class class;
>  	u8 instance;
>  	u32 mmio_base;
>  	unsigned irq_shift;
> -	int (*init_legacy)(struct intel_engine_cs *engine);
> -	int (*init_execlists)(struct intel_engine_cs *engine);
> -} intel_engines[] = {
> +};
> +
> +static const struct engine_info intel_engines[] = {
>  	[RCS] = {
> -		.name = "rcs",
> -		.hw_id = RCS_HW,
>  		.exec_id = I915_EXEC_RENDER,
> +		.hw_id = RCS_HW,
>  		.class = RENDER_CLASS,
>  		.instance = 0,
>  		.mmio_base = RENDER_RING_BASE,
>  		.irq_shift = GEN8_RCS_IRQ_SHIFT,
> -		.init_execlists = logical_render_ring_init,
> -		.init_legacy = intel_init_render_ring_buffer,
>  	},
>  	[BCS] = {
> -		.name = "bcs",
> -		.hw_id = BCS_HW,
>  		.exec_id = I915_EXEC_BLT,
> +		.hw_id = BCS_HW,
>  		.class = COPY_ENGINE_CLASS,
>  		.instance = 0,
>  		.mmio_base = BLT_RING_BASE,
>  		.irq_shift = GEN8_BCS_IRQ_SHIFT,
> -		.init_execlists = logical_xcs_ring_init,
> -		.init_legacy = intel_init_blt_ring_buffer,
>  	},
>  	[VCS] = {
> -		.name = "vcs",
> -		.hw_id = VCS_HW,
>  		.exec_id = I915_EXEC_BSD,
> +		.hw_id = VCS_HW,
>  		.class = VIDEO_DECODE_CLASS,
>  		.instance = 0,
>  		.mmio_base = GEN6_BSD_RING_BASE,
>  		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
> -		.init_execlists = logical_xcs_ring_init,
> -		.init_legacy = intel_init_bsd_ring_buffer,
>  	},
>  	[VCS2] = {
> -		.name = "vcs",
> -		.hw_id = VCS2_HW,
>  		.exec_id = I915_EXEC_BSD,
> +		.hw_id = VCS2_HW,
>  		.class = VIDEO_DECODE_CLASS,
>  		.instance = 1,
>  		.mmio_base = GEN8_BSD2_RING_BASE,
>  		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
> -		.init_execlists = logical_xcs_ring_init,
> -		.init_legacy = intel_init_bsd_ring_buffer,
>  	},
>  	[VECS] = {
> -		.name = "vecs",
> -		.hw_id = VECS_HW,
>  		.exec_id = I915_EXEC_VEBOX,
> +		.hw_id = VECS_HW,
>  		.class = VIDEO_ENHANCEMENT_CLASS,
>  		.instance = 0,
>  		.mmio_base = VEBOX_RING_BASE,
>  		.irq_shift = GEN8_VECS_IRQ_SHIFT,
> -		.init_execlists = logical_xcs_ring_init,
> -		.init_legacy = intel_init_vebox_ring_buffer,
>  	},
>  };
>  
> @@ -99,6 +112,8 @@
>  		   enum intel_engine_id id)
>  {
>  	const struct engine_info *info = &intel_engines[id];
> +	const struct engine_class_info *class_info =
> +				&intel_engine_classes[info->class];

Hmm, maybe we should add some protection against out-of-bound access
to the class info array ? We can start with:

	GEM_BUG_ON(info->class > ARRAY_SIZE(intel_engine_classes));

-Michal

>  	struct intel_engine_cs *engine;
>  
>  	GEM_BUG_ON(dev_priv->engine[id]);
> @@ -109,7 +124,7 @@
>  	engine->id = id;
>  	engine->i915 = dev_priv;
>  	snprintf(engine->name, sizeof(engine->name), "%s%u",
> -				info->name, info->instance);
> +				class_info->name, info->instance);
>  	engine->exec_id = info->exec_id;
>  	engine->hw_id = engine->guc_id = info->hw_id;
>  	engine->mmio_base = info->mmio_base;
> @@ -190,12 +205,14 @@ int intel_engines_init(struct drm_i915_private *dev_priv)
>  	int err = 0;
>  
>  	for_each_engine(engine, dev_priv, id) {
> +		const struct engine_class_info *class_info =
> +					&intel_engine_classes[engine->class];
>  		int (*init)(struct intel_engine_cs *engine);
>  
>  		if (i915.enable_execlists)
> -			init = intel_engines[id].init_execlists;
> +			init = class_info->init_execlists;
>  		else
> -			init = intel_engines[id].init_legacy;
> +			init = class_info->init_legacy;
>  		if (!init) {
>  			kfree(engine);
>  			dev_priv->engine[id] = NULL;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index c6a73d0..6eab22d 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -26,71 +26,84 @@ 
 #include "intel_ringbuffer.h"
 #include "intel_lrc.h"
 
-static const struct engine_info {
+struct engine_class_info {
 	const char *name;
+	int (*init_legacy)(struct intel_engine_cs *engine);
+	int (*init_execlists)(struct intel_engine_cs *engine);
+};
+
+static const struct engine_class_info intel_engine_classes[] = {
+	[RENDER_CLASS] = {
+		.name = "rcs",
+		.init_execlists = logical_render_ring_init,
+		.init_legacy = intel_init_render_ring_buffer,
+	},
+	[COPY_ENGINE_CLASS] = {
+		.name = "bcs",
+		.init_execlists = logical_xcs_ring_init,
+		.init_legacy = intel_init_blt_ring_buffer,
+	},
+	[VIDEO_DECODE_CLASS] = {
+		.name = "vcs",
+		.init_execlists = logical_xcs_ring_init,
+		.init_legacy = intel_init_bsd_ring_buffer,
+	},
+	[VIDEO_ENHANCEMENT_CLASS] = {
+		.name = "vecs",
+		.init_execlists = logical_xcs_ring_init,
+		.init_legacy = intel_init_vebox_ring_buffer,
+	},
+};
+
+struct engine_info {
 	unsigned int exec_id;
 	unsigned int hw_id;
 	enum intel_engine_class class;
 	u8 instance;
 	u32 mmio_base;
 	unsigned irq_shift;
-	int (*init_legacy)(struct intel_engine_cs *engine);
-	int (*init_execlists)(struct intel_engine_cs *engine);
-} intel_engines[] = {
+};
+
+static const struct engine_info intel_engines[] = {
 	[RCS] = {
-		.name = "rcs",
-		.hw_id = RCS_HW,
 		.exec_id = I915_EXEC_RENDER,
+		.hw_id = RCS_HW,
 		.class = RENDER_CLASS,
 		.instance = 0,
 		.mmio_base = RENDER_RING_BASE,
 		.irq_shift = GEN8_RCS_IRQ_SHIFT,
-		.init_execlists = logical_render_ring_init,
-		.init_legacy = intel_init_render_ring_buffer,
 	},
 	[BCS] = {
-		.name = "bcs",
-		.hw_id = BCS_HW,
 		.exec_id = I915_EXEC_BLT,
+		.hw_id = BCS_HW,
 		.class = COPY_ENGINE_CLASS,
 		.instance = 0,
 		.mmio_base = BLT_RING_BASE,
 		.irq_shift = GEN8_BCS_IRQ_SHIFT,
-		.init_execlists = logical_xcs_ring_init,
-		.init_legacy = intel_init_blt_ring_buffer,
 	},
 	[VCS] = {
-		.name = "vcs",
-		.hw_id = VCS_HW,
 		.exec_id = I915_EXEC_BSD,
+		.hw_id = VCS_HW,
 		.class = VIDEO_DECODE_CLASS,
 		.instance = 0,
 		.mmio_base = GEN6_BSD_RING_BASE,
 		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
-		.init_execlists = logical_xcs_ring_init,
-		.init_legacy = intel_init_bsd_ring_buffer,
 	},
 	[VCS2] = {
-		.name = "vcs",
-		.hw_id = VCS2_HW,
 		.exec_id = I915_EXEC_BSD,
+		.hw_id = VCS2_HW,
 		.class = VIDEO_DECODE_CLASS,
 		.instance = 1,
 		.mmio_base = GEN8_BSD2_RING_BASE,
 		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
-		.init_execlists = logical_xcs_ring_init,
-		.init_legacy = intel_init_bsd_ring_buffer,
 	},
 	[VECS] = {
-		.name = "vecs",
-		.hw_id = VECS_HW,
 		.exec_id = I915_EXEC_VEBOX,
+		.hw_id = VECS_HW,
 		.class = VIDEO_ENHANCEMENT_CLASS,
 		.instance = 0,
 		.mmio_base = VEBOX_RING_BASE,
 		.irq_shift = GEN8_VECS_IRQ_SHIFT,
-		.init_execlists = logical_xcs_ring_init,
-		.init_legacy = intel_init_vebox_ring_buffer,
 	},
 };
 
@@ -99,6 +112,8 @@ 
 		   enum intel_engine_id id)
 {
 	const struct engine_info *info = &intel_engines[id];
+	const struct engine_class_info *class_info =
+				&intel_engine_classes[info->class];
 	struct intel_engine_cs *engine;
 
 	GEM_BUG_ON(dev_priv->engine[id]);
@@ -109,7 +124,7 @@ 
 	engine->id = id;
 	engine->i915 = dev_priv;
 	snprintf(engine->name, sizeof(engine->name), "%s%u",
-				info->name, info->instance);
+				class_info->name, info->instance);
 	engine->exec_id = info->exec_id;
 	engine->hw_id = engine->guc_id = info->hw_id;
 	engine->mmio_base = info->mmio_base;
@@ -190,12 +205,14 @@  int intel_engines_init(struct drm_i915_private *dev_priv)
 	int err = 0;
 
 	for_each_engine(engine, dev_priv, id) {
+		const struct engine_class_info *class_info =
+					&intel_engine_classes[engine->class];
 		int (*init)(struct intel_engine_cs *engine);
 
 		if (i915.enable_execlists)
-			init = intel_engines[id].init_execlists;
+			init = class_info->init_execlists;
 		else
-			init = intel_engines[id].init_legacy;
+			init = class_info->init_legacy;
 		if (!init) {
 			kfree(engine);
 			dev_priv->engine[id] = NULL;