All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] Classify the engines in class + instance
@ 2017-04-05  9:30 Oscar Mateo
  2017-04-05  9:30 ` [PATCH 1/5] drm/i915: " Oscar Mateo
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Oscar Mateo @ 2017-04-05  9:30 UTC (permalink / raw)
  To: intel-gfx

This refactoring helps simplify a few things here and there.

Daniele Ceraolo Spurio (2):
  drm/i915: Classify the engines in class + instance
  drm/i915: Use the engine class to get the context size

Oscar Mateo (3):
  drm/i915: Use the same vfunc for BSD2 ring init
  drm/i915: Generate the engine name based on the instance number
  drm/i915: Split the engine info table in two levels, using class +
    instance

 drivers/gpu/drm/i915/intel_engine_cs.c       | 90 +++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_lrc.c             | 34 +++++++----
 drivers/gpu/drm/i915/intel_lrc.h             |  7 ++-
 drivers/gpu/drm/i915/intel_ringbuffer.c      | 14 -----
 drivers/gpu/drm/i915/intel_ringbuffer.h      | 15 ++++-
 drivers/gpu/drm/i915/selftests/mock_engine.c |  2 +-
 6 files changed, 104 insertions(+), 58 deletions(-)

-- 
1.9.1

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/5] drm/i915: Classify the engines in class + instance
  2017-04-05  9:30 [PATCH 0/5] Classify the engines in class + instance Oscar Mateo
@ 2017-04-05  9:30 ` Oscar Mateo
  2017-04-06 17:45   ` Tvrtko Ursulin
  2017-04-05  9:30 ` [PATCH 2/5] drm/i915: Use the same vfunc for BSD2 ring init Oscar Mateo
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Oscar Mateo @ 2017-04-05  9:30 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

In such a way that vcs and vcs2 are just two different instances (0 and 1)
of the same engine class (VIDEO_DECODE_CLASS).

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>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c  | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h | 10 ++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 854e8e0..b3d575a 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -30,6 +30,8 @@
 	const char *name;
 	unsigned int exec_id;
 	unsigned int hw_id;
+	enum intel_engine_class class;
+	u32 instance;
 	u32 mmio_base;
 	unsigned irq_shift;
 	int (*init_legacy)(struct intel_engine_cs *engine);
@@ -39,6 +41,8 @@
 		.name = "rcs",
 		.hw_id = RCS_HW,
 		.exec_id = I915_EXEC_RENDER,
+		.class = RENDER_CLASS,
+		.instance = 0,
 		.mmio_base = RENDER_RING_BASE,
 		.irq_shift = GEN8_RCS_IRQ_SHIFT,
 		.init_execlists = logical_render_ring_init,
@@ -48,6 +52,8 @@
 		.name = "bcs",
 		.hw_id = BCS_HW,
 		.exec_id = I915_EXEC_BLT,
+		.class = COPY_ENGINE_CLASS,
+		.instance = 0,
 		.mmio_base = BLT_RING_BASE,
 		.irq_shift = GEN8_BCS_IRQ_SHIFT,
 		.init_execlists = logical_xcs_ring_init,
@@ -57,6 +63,8 @@
 		.name = "vcs",
 		.hw_id = VCS_HW,
 		.exec_id = I915_EXEC_BSD,
+		.class = VIDEO_DECODE_CLASS,
+		.instance = 0,
 		.mmio_base = GEN6_BSD_RING_BASE,
 		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
 		.init_execlists = logical_xcs_ring_init,
@@ -66,6 +74,8 @@
 		.name = "vcs2",
 		.hw_id = VCS2_HW,
 		.exec_id = I915_EXEC_BSD,
+		.class = VIDEO_DECODE_CLASS,
+		.instance = 1,
 		.mmio_base = GEN8_BSD2_RING_BASE,
 		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
 		.init_execlists = logical_xcs_ring_init,
@@ -75,6 +85,8 @@
 		.name = "vecs",
 		.hw_id = VECS_HW,
 		.exec_id = I915_EXEC_VEBOX,
+		.class = VIDEO_ENHANCEMENT_CLASS,
+		.instance = 0,
 		.mmio_base = VEBOX_RING_BASE,
 		.irq_shift = GEN8_VECS_IRQ_SHIFT,
 		.init_execlists = logical_xcs_ring_init,
@@ -101,6 +113,8 @@
 	engine->hw_id = engine->guc_id = info->hw_id;
 	engine->mmio_base = info->mmio_base;
 	engine->irq_shift = info->irq_shift;
+	engine->class = info->class;
+	engine->instance = info->instance;
 
 	/* Nothing to do here, execute in order of dependencies */
 	engine->schedule = NULL;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index cbe61d3..4ab590b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -193,6 +193,16 @@ struct intel_engine_cs {
 	enum intel_engine_id id;
 	unsigned int exec_id;
 	unsigned int hw_id;
+
+	enum intel_engine_class {
+		RENDER_CLASS = 0,
+		VIDEO_DECODE_CLASS = 1,
+		VIDEO_ENHANCEMENT_CLASS = 2,
+		COPY_ENGINE_CLASS = 3,
+		OTHER_CLASS = 4
+	} class;
+	u8 instance;
+
 	unsigned int guc_id;
 	u32		mmio_base;
 	unsigned int irq_shift;
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/5] drm/i915: Use the same vfunc for BSD2 ring init
  2017-04-05  9:30 [PATCH 0/5] Classify the engines in class + instance Oscar Mateo
  2017-04-05  9:30 ` [PATCH 1/5] drm/i915: " Oscar Mateo
@ 2017-04-05  9:30 ` Oscar Mateo
  2017-04-06 17:48   ` Tvrtko Ursulin
  2017-04-05  9:30 ` [PATCH 3/5] drm/i915: Generate the engine name based on the instance number Oscar Mateo
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Oscar Mateo @ 2017-04-05  9:30 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi

If we needed to do something different for the init functions, we could
always look at the instance number to make the distinction.But, in any
case, the two functions are virtually identical already (please notice
that BSD2_RING is only used from gen8 onwards).

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  |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 14 --------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  1 -
 3 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index b3d575a..abc0a9c 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -79,7 +79,7 @@
 		.mmio_base = GEN8_BSD2_RING_BASE,
 		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
 		.init_execlists = logical_xcs_ring_init,
-		.init_legacy = intel_init_bsd2_ring_buffer,
+		.init_legacy = intel_init_bsd_ring_buffer,
 	},
 	[VECS] = {
 		.name = "vecs",
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c98acc2..81eee42 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2175,20 +2175,6 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
 	return intel_init_ring_buffer(engine);
 }
 
-/**
- * Initialize the second BSD ring (eg. Broadwell GT3, Skylake GT3)
- */
-int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
-{
-	struct drm_i915_private *dev_priv = engine->i915;
-
-	intel_ring_default_vfuncs(dev_priv, engine);
-
-	engine->emit_flush = gen6_bsd_ring_flush;
-
-	return intel_init_ring_buffer(engine);
-}
-
 int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 4ab590b..5c1a27f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -561,7 +561,6 @@ int intel_ring_pin(struct intel_ring *ring,
 
 int intel_init_render_ring_buffer(struct intel_engine_cs *engine);
 int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine);
-int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine);
 int intel_init_blt_ring_buffer(struct intel_engine_cs *engine);
 int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine);
 
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/5] drm/i915: Generate the engine name based on the instance number
  2017-04-05  9:30 [PATCH 0/5] Classify the engines in class + instance Oscar Mateo
  2017-04-05  9:30 ` [PATCH 1/5] drm/i915: " Oscar Mateo
  2017-04-05  9:30 ` [PATCH 2/5] drm/i915: Use the same vfunc for BSD2 ring init Oscar Mateo
@ 2017-04-05  9:30 ` Oscar Mateo
  2017-04-06 18:02   ` Tvrtko Ursulin
  2017-04-05  9:30 ` [PATCH 4/5] drm/i915: Split the engine info table in two levels, using class + instance Oscar Mateo
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Oscar Mateo @ 2017-04-05  9:30 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi

Not really needed, but makes the next change a little bit more compact.

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       | 8 ++++++--
 drivers/gpu/drm/i915/intel_ringbuffer.h      | 4 +++-
 drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +-
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index abc0a9c..530f822 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -71,7 +71,7 @@
 		.init_legacy = intel_init_bsd_ring_buffer,
 	},
 	[VCS2] = {
-		.name = "vcs2",
+		.name = "vcs",
 		.hw_id = VCS2_HW,
 		.exec_id = I915_EXEC_BSD,
 		.class = VIDEO_DECODE_CLASS,
@@ -100,6 +100,7 @@
 {
 	const struct engine_info *info = &intel_engines[id];
 	struct intel_engine_cs *engine;
+	char instance[3] = "";
 
 	GEM_BUG_ON(dev_priv->engine[id]);
 	engine = kzalloc(sizeof(*engine), GFP_KERNEL);
@@ -108,7 +109,10 @@
 
 	engine->id = id;
 	engine->i915 = dev_priv;
-	engine->name = info->name;
+	/* For historical reasons the engines are called: name, name2... */
+	if (info->instance)
+		snprintf(instance, sizeof(instance), "%u", info->instance + 1);
+	snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, instance);
 	engine->exec_id = info->exec_id;
 	engine->hw_id = engine->guc_id = info->hw_id;
 	engine->mmio_base = info->mmio_base;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 5c1a27f..d617049 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -187,9 +187,11 @@ enum intel_engine_id {
 	VECS
 };
 
+#define INTEL_ENGINE_CS_MAX_NAME 8
+
 struct intel_engine_cs {
 	struct drm_i915_private *i915;
-	const char	*name;
+	char name[INTEL_ENGINE_CS_MAX_NAME];
 	enum intel_engine_id id;
 	unsigned int exec_id;
 	unsigned int hw_id;
diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c
index b89050e..4a1ffca 100644
--- a/drivers/gpu/drm/i915/selftests/mock_engine.c
+++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
@@ -140,7 +140,7 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
 
 	/* minimal engine setup for requests */
 	engine->base.i915 = i915;
-	engine->base.name = name;
+	strncpy(engine->base.name, name, sizeof(engine->base.name) - 1);
 	engine->base.id = id++;
 	engine->base.status_page.page_addr = (void *)(engine + 1);
 
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 4/5] drm/i915: Split the engine info table in two levels, using class + instance
  2017-04-05  9:30 [PATCH 0/5] Classify the engines in class + instance Oscar Mateo
                   ` (2 preceding siblings ...)
  2017-04-05  9:30 ` [PATCH 3/5] drm/i915: Generate the engine name based on the instance number Oscar Mateo
@ 2017-04-05  9:30 ` Oscar Mateo
  2017-04-06 18:09   ` Tvrtko Ursulin
  2017-04-05  9:30 ` [PATCH 5/5] drm/i915: Use the engine class to get the context size Oscar Mateo
  2017-04-05 16:58 ` ✓ Fi.CI.BAT: success for Classify the engines in class + instance Patchwork
  5 siblings, 1 reply; 17+ messages in thread
From: Oscar Mateo @ 2017-04-05  9:30 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi

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 | 72 +++++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 530f822..691689c 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -26,71 +26,83 @@
 #include "intel_ringbuffer.h"
 #include "intel_lrc.h"
 
-static const struct engine_info {
+struct engine_class_info {
 	const char *name;
 	unsigned int exec_id;
+	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",
+		.exec_id = I915_EXEC_RENDER,
+		.init_execlists = logical_render_ring_init,
+		.init_legacy = intel_init_render_ring_buffer,
+	},
+	[COPY_ENGINE_CLASS] = {
+		.name = "bcs",
+		.exec_id = I915_EXEC_BLT,
+		.init_execlists = logical_xcs_ring_init,
+		.init_legacy = intel_init_blt_ring_buffer,
+	},
+	[VIDEO_DECODE_CLASS] = {
+		.name = "vcs",
+		.exec_id = I915_EXEC_BSD,
+		.init_execlists = logical_xcs_ring_init,
+		.init_legacy = intel_init_bsd_ring_buffer,
+	},
+	[VIDEO_ENHANCEMENT_CLASS] = {
+		.name = "vecs",
+		.exec_id = I915_EXEC_VEBOX,
+		.init_execlists = logical_xcs_ring_init,
+		.init_legacy = intel_init_vebox_ring_buffer,
+	},
+};
+
+struct engine_info {
 	unsigned int hw_id;
 	enum intel_engine_class class;
 	u32 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,
 		.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,
 		.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,
 		.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,
 		.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,
 		.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 +111,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;
 	char instance[3] = "";
 
@@ -112,8 +126,8 @@
 	/* For historical reasons the engines are called: name, name2... */
 	if (info->instance)
 		snprintf(instance, sizeof(instance), "%u", info->instance + 1);
-	snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, instance);
-	engine->exec_id = info->exec_id;
+	snprintf(engine->name, sizeof(engine->name), "%s%s", class_info->name, instance);
+	engine->exec_id = class_info->exec_id;
 	engine->hw_id = engine->guc_id = info->hw_id;
 	engine->mmio_base = info->mmio_base;
 	engine->irq_shift = info->irq_shift;
@@ -193,12 +207,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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 5/5] drm/i915: Use the engine class to get the context size
  2017-04-05  9:30 [PATCH 0/5] Classify the engines in class + instance Oscar Mateo
                   ` (3 preceding siblings ...)
  2017-04-05  9:30 ` [PATCH 4/5] drm/i915: Split the engine info table in two levels, using class + instance Oscar Mateo
@ 2017-04-05  9:30 ` Oscar Mateo
  2017-04-06 18:19   ` Tvrtko Ursulin
  2017-04-05 16:58 ` ✓ Fi.CI.BAT: success for Classify the engines in class + instance Patchwork
  5 siblings, 1 reply; 17+ messages in thread
From: Oscar Mateo @ 2017-04-05  9:30 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Technically speaking, the context size is per engine class, not per
instance.

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>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 34 ++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_lrc.h |  7 ++++++-
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 0dc1cc4..6b1fc4a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1908,8 +1908,10 @@ static void execlists_init_reg_state(u32 *regs,
 }
 
 /**
- * intel_lr_context_size() - return the size of the context for an engine
- * @engine: which engine to find the context size for
+ * intel_lr_class_context_size() - return the size of the context for a given
+ * engine class
+ * @dev_priv: i915 device private
+ * @class: which engine class to find the context size for
  *
  * Each engine may require a different amount of space for a context image,
  * so when allocating (or copying) an image, this function can be used to
@@ -1921,25 +1923,33 @@ static void execlists_init_reg_state(u32 *regs,
  * in LRC mode, but does not include the "shared data page" used with
  * GuC submission. The caller should account for this if using the GuC.
  */
-uint32_t intel_lr_context_size(struct intel_engine_cs *engine)
+uint32_t intel_lr_class_context_size(struct drm_i915_private *dev_priv,
+				     enum intel_engine_class class)
 {
 	int ret = 0;
 
-	WARN_ON(INTEL_GEN(engine->i915) < 8);
+	WARN_ON(INTEL_GEN(dev_priv) < 8);
 
-	switch (engine->id) {
-	case RCS:
-		if (INTEL_GEN(engine->i915) >= 9)
+	switch (class) {
+	case RENDER_CLASS:
+		switch (INTEL_GEN(dev_priv)) {
+		default:
+			DRM_ERROR("Unknown context size for GEN\n");
+		case 9:
 			ret = GEN9_LR_CONTEXT_RENDER_SIZE;
-		else
+			break;
+		case 8:
 			ret = GEN8_LR_CONTEXT_RENDER_SIZE;
+			break;
+		}
 		break;
-	case VCS:
-	case BCS:
-	case VECS:
-	case VCS2:
+	case VIDEO_DECODE_CLASS:
+	case VIDEO_ENHANCEMENT_CLASS:
+	case COPY_ENGINE_CLASS:
 		ret = GEN8_LR_CONTEXT_OTHER_SIZE;
 		break;
+	default:
+		MISSING_CASE(class);
 	}
 
 	return ret;
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index e8015e7..b3a4331 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -78,7 +78,12 @@ enum {
 struct drm_i915_private;
 struct i915_gem_context;
 
-uint32_t intel_lr_context_size(struct intel_engine_cs *engine);
+uint32_t intel_lr_class_context_size(struct drm_i915_private *dev_priv,
+				     enum intel_engine_class class);
+static inline uint32_t intel_lr_context_size(struct intel_engine_cs *engine)
+{
+	return intel_lr_class_context_size(engine->i915, engine->class);
+}
 
 void intel_lr_context_resume(struct drm_i915_private *dev_priv);
 uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* ✓ Fi.CI.BAT: success for Classify the engines in class + instance
  2017-04-05  9:30 [PATCH 0/5] Classify the engines in class + instance Oscar Mateo
                   ` (4 preceding siblings ...)
  2017-04-05  9:30 ` [PATCH 5/5] drm/i915: Use the engine class to get the context size Oscar Mateo
@ 2017-04-05 16:58 ` Patchwork
  5 siblings, 0 replies; 17+ messages in thread
From: Patchwork @ 2017-04-05 16:58 UTC (permalink / raw)
  To: Oscar Mateo; +Cc: intel-gfx

== Series Details ==

Series: Classify the engines in class + instance
URL   : https://patchwork.freedesktop.org/series/22535/
State : success

== Summary ==

Series 22535v1 Classify the engines in class + instance
https://patchwork.freedesktop.org/api/1.0/series/22535/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 432s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 425s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time: 574s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 543s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time: 479s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 484s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 412s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 406s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 422s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 488s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 465s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 455s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 575s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 450s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 573s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 459s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 491s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 436s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 529s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 411s
fi-bxt-j4205 failed to collect. IGT log at Patchwork_4413/fi-bxt-j4205/igt.log

39a0f48c8bc7c528cc705016dafa08a9dedfd36b drm-tip: 2017y-04m-05d-13h-22m-47s UTC integration manifest
c328081 drm/i915: Use the engine class to get the context size
dab2e3f drm/i915: Split the engine info table in two levels, using class + instance
f3c78f4 drm/i915: Generate the engine name based on the instance number
14f39bb drm/i915: Use the same vfunc for BSD2 ring init
50e8315 drm/i915: Classify the engines in class + instance

== Logs ==

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/5] drm/i915: Use the same vfunc for BSD2 ring init
  2017-04-06 17:48   ` Tvrtko Ursulin
@ 2017-04-06 11:14     ` Oscar Mateo
  0 siblings, 0 replies; 17+ messages in thread
From: Oscar Mateo @ 2017-04-06 11:14 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi



On 04/06/2017 10:48 AM, Tvrtko Ursulin wrote:
>
> On 05/04/2017 10:30, Oscar Mateo wrote:
>> If we needed to do something different for the init functions, we could
>> always look at the instance number to make the distinction.But, in any
>> case, the two functions are virtually identical already (please notice
>> that BSD2_RING is only used from gen8 onwards).
>>
>> 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  |  2 +-
>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 14 --------------
>>  drivers/gpu/drm/i915/intel_ringbuffer.h |  1 -
>>  3 files changed, 1 insertion(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
>> b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index b3d575a..abc0a9c 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -79,7 +79,7 @@
>>          .mmio_base = GEN8_BSD2_RING_BASE,
>>          .irq_shift = GEN8_VCS2_IRQ_SHIFT,
>>          .init_execlists = logical_xcs_ring_init,
>> -        .init_legacy = intel_init_bsd2_ring_buffer,
>> +        .init_legacy = intel_init_bsd_ring_buffer,
>>      },
>>      [VECS] = {
>>          .name = "vecs",
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c 
>> b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index c98acc2..81eee42 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -2175,20 +2175,6 @@ int intel_init_bsd_ring_buffer(struct 
>> intel_engine_cs *engine)
>>      return intel_init_ring_buffer(engine);
>>  }
>>
>> -/**
>> - * Initialize the second BSD ring (eg. Broadwell GT3, Skylake GT3)
>> - */
>> -int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
>> -{
>> -    struct drm_i915_private *dev_priv = engine->i915;
>> -
>> -    intel_ring_default_vfuncs(dev_priv, engine);
>> -
>> -    engine->emit_flush = gen6_bsd_ring_flush;
>> -
>> -    return intel_init_ring_buffer(engine);
>> -}
>> -
>>  int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
>>  {
>>      struct drm_i915_private *dev_priv = engine->i915;
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 4ab590b..5c1a27f 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -561,7 +561,6 @@ int intel_ring_pin(struct intel_ring *ring,
>>
>>  int intel_init_render_ring_buffer(struct intel_engine_cs *engine);
>>  int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine);
>> -int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine);
>>  int intel_init_blt_ring_buffer(struct intel_engine_cs *engine);
>>  int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine);
>>
>>
>
> Job is not done until there is nothing left to remove! :)
>
> Doesn't depend on anything so could have been first in the series, or 
> separate, but doesn't matter.
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Regards,
>
> Tvrtko

Every program can be simplified by at least one line of code, and every 
program contains at least one bug; so every program can be simplified to 
one line of code that doesn't work :)

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] drm/i915: Generate the engine name based on the instance number
  2017-04-06 18:02   ` Tvrtko Ursulin
@ 2017-04-06 11:22     ` Oscar Mateo
  2017-04-06 18:37       ` Tvrtko Ursulin
  2017-04-06 18:10     ` Chris Wilson
  1 sibling, 1 reply; 17+ messages in thread
From: Oscar Mateo @ 2017-04-06 11:22 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi



On 04/06/2017 11:02 AM, Tvrtko Ursulin wrote:
>
> On 05/04/2017 10:30, Oscar Mateo wrote:
>> Not really needed, but makes the next change a little bit more compact.
>>
>> 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       | 8 ++++++--
>>  drivers/gpu/drm/i915/intel_ringbuffer.h      | 4 +++-
>>  drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +-
>>  3 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
>> b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index abc0a9c..530f822 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -71,7 +71,7 @@
>>          .init_legacy = intel_init_bsd_ring_buffer,
>>      },
>>      [VCS2] = {
>> -        .name = "vcs2",
>> +        .name = "vcs",
>
> Rename the field to class_name perhaps?
>

In the following patch, when I move .name to the class table, it becomes 
much more obvious what it refers to, but I can change it to class_name 
if you feel strongly about it (or change it here and then back to name 
in the next patch?).

>>          .hw_id = VCS2_HW,
>>          .exec_id = I915_EXEC_BSD,
>>          .class = VIDEO_DECODE_CLASS,
>> @@ -100,6 +100,7 @@
>>  {
>>      const struct engine_info *info = &intel_engines[id];
>>      struct intel_engine_cs *engine;
>> +    char instance[3] = "";
>>
>>      GEM_BUG_ON(dev_priv->engine[id]);
>>      engine = kzalloc(sizeof(*engine), GFP_KERNEL);
>> @@ -108,7 +109,10 @@
>>
>>      engine->id = id;
>>      engine->i915 = dev_priv;
>> -    engine->name = info->name;
>> +    /* For historical reasons the engines are called: name, name2... */
>> +    if (info->instance)
>> +        snprintf(instance, sizeof(instance), "%u", info->instance + 1);
>> +    snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, 
>> instance);
>
> Since Chris has recently renamed all the engines, I'd say who cares 
> about the numbering scheme. Just drop it for simplicity.
>

Oooohh!
Can I also use zero-based numbering, like in the Bspec? (so xcs0, xcs1 
... xcsN?)

>>      engine->exec_id = info->exec_id;
>>      engine->hw_id = engine->guc_id = info->hw_id;
>>      engine->mmio_base = info->mmio_base;
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 5c1a27f..d617049 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -187,9 +187,11 @@ enum intel_engine_id {
>>      VECS
>>  };
>>
>> +#define INTEL_ENGINE_CS_MAX_NAME 8
>> +
>>  struct intel_engine_cs {
>>      struct drm_i915_private *i915;
>> -    const char    *name;
>> +    char name[INTEL_ENGINE_CS_MAX_NAME];
>>      enum intel_engine_id id;
>>      unsigned int exec_id;
>>      unsigned int hw_id;
>> diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c 
>> b/drivers/gpu/drm/i915/selftests/mock_engine.c
>> index b89050e..4a1ffca 100644
>> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c
>> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
>> @@ -140,7 +140,7 @@ struct intel_engine_cs *mock_engine(struct 
>> drm_i915_private *i915,
>>
>>      /* minimal engine setup for requests */
>>      engine->base.i915 = i915;
>> -    engine->base.name = name;
>> +    strncpy(engine->base.name, name, sizeof(engine->base.name) - 1);
>
> Can this miss to null-terminate? Or it relies on the mock_engine being 
> kzalloced?
>

It relies on the kzalloc, but I can add a \0 at the end for extra security?

>>      engine->base.id = id++;
>>      engine->base.status_page.page_addr = (void *)(engine + 1);
>>
>>
>
> Regards,
>
> Tvrtko

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] drm/i915: Classify the engines in class + instance
  2017-04-05  9:30 ` [PATCH 1/5] drm/i915: " Oscar Mateo
@ 2017-04-06 17:45   ` Tvrtko Ursulin
  0 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-04-06 17:45 UTC (permalink / raw)
  To: Oscar Mateo, intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi


On 05/04/2017 10:30, Oscar Mateo wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
> In such a way that vcs and vcs2 are just two different instances (0 and 1)
> of the same engine class (VIDEO_DECODE_CLASS).
>
> 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>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c  | 14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 10 ++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 854e8e0..b3d575a 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -30,6 +30,8 @@
>  	const char *name;
>  	unsigned int exec_id;
>  	unsigned int hw_id;
> +	enum intel_engine_class class;
> +	u32 instance;

Normal type I think, it's not anything hardware related to need explicit 
width. Or maybe make it u8 if it saves any space?

>  	u32 mmio_base;
>  	unsigned irq_shift;
>  	int (*init_legacy)(struct intel_engine_cs *engine);
> @@ -39,6 +41,8 @@
>  		.name = "rcs",
>  		.hw_id = RCS_HW,
>  		.exec_id = I915_EXEC_RENDER,
> +		.class = RENDER_CLASS,
> +		.instance = 0,
>  		.mmio_base = RENDER_RING_BASE,
>  		.irq_shift = GEN8_RCS_IRQ_SHIFT,
>  		.init_execlists = logical_render_ring_init,
> @@ -48,6 +52,8 @@
>  		.name = "bcs",
>  		.hw_id = BCS_HW,
>  		.exec_id = I915_EXEC_BLT,
> +		.class = COPY_ENGINE_CLASS,
> +		.instance = 0,
>  		.mmio_base = BLT_RING_BASE,
>  		.irq_shift = GEN8_BCS_IRQ_SHIFT,
>  		.init_execlists = logical_xcs_ring_init,
> @@ -57,6 +63,8 @@
>  		.name = "vcs",
>  		.hw_id = VCS_HW,
>  		.exec_id = I915_EXEC_BSD,
> +		.class = VIDEO_DECODE_CLASS,
> +		.instance = 0,
>  		.mmio_base = GEN6_BSD_RING_BASE,
>  		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
>  		.init_execlists = logical_xcs_ring_init,
> @@ -66,6 +74,8 @@
>  		.name = "vcs2",
>  		.hw_id = VCS2_HW,
>  		.exec_id = I915_EXEC_BSD,
> +		.class = VIDEO_DECODE_CLASS,
> +		.instance = 1,
>  		.mmio_base = GEN8_BSD2_RING_BASE,
>  		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
>  		.init_execlists = logical_xcs_ring_init,
> @@ -75,6 +85,8 @@
>  		.name = "vecs",
>  		.hw_id = VECS_HW,
>  		.exec_id = I915_EXEC_VEBOX,
> +		.class = VIDEO_ENHANCEMENT_CLASS,
> +		.instance = 0,
>  		.mmio_base = VEBOX_RING_BASE,
>  		.irq_shift = GEN8_VECS_IRQ_SHIFT,
>  		.init_execlists = logical_xcs_ring_init,
> @@ -101,6 +113,8 @@
>  	engine->hw_id = engine->guc_id = info->hw_id;
>  	engine->mmio_base = info->mmio_base;
>  	engine->irq_shift = info->irq_shift;
> +	engine->class = info->class;
> +	engine->instance = info->instance;
>
>  	/* Nothing to do here, execute in order of dependencies */
>  	engine->schedule = NULL;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index cbe61d3..4ab590b 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -193,6 +193,16 @@ struct intel_engine_cs {
>  	enum intel_engine_id id;
>  	unsigned int exec_id;
>  	unsigned int hw_id;
> +
> +	enum intel_engine_class {
> +		RENDER_CLASS = 0,
> +		VIDEO_DECODE_CLASS = 1,
> +		VIDEO_ENHANCEMENT_CLASS = 2,
> +		COPY_ENGINE_CLASS = 3,
> +		OTHER_CLASS = 4
> +	} class;
> +	u8 instance;

Oh it is already u8 here! :)

> +
>  	unsigned int guc_id;
>  	u32		mmio_base;
>  	unsigned int irq_shift;
>

Looks OK to me. With the types aligned:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/5] drm/i915: Use the same vfunc for BSD2 ring init
  2017-04-05  9:30 ` [PATCH 2/5] drm/i915: Use the same vfunc for BSD2 ring init Oscar Mateo
@ 2017-04-06 17:48   ` Tvrtko Ursulin
  2017-04-06 11:14     ` Oscar Mateo
  0 siblings, 1 reply; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-04-06 17:48 UTC (permalink / raw)
  To: Oscar Mateo, intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi


On 05/04/2017 10:30, Oscar Mateo wrote:
> If we needed to do something different for the init functions, we could
> always look at the instance number to make the distinction.But, in any
> case, the two functions are virtually identical already (please notice
> that BSD2_RING is only used from gen8 onwards).
>
> 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  |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 14 --------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  1 -
>  3 files changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index b3d575a..abc0a9c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -79,7 +79,7 @@
>  		.mmio_base = GEN8_BSD2_RING_BASE,
>  		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
>  		.init_execlists = logical_xcs_ring_init,
> -		.init_legacy = intel_init_bsd2_ring_buffer,
> +		.init_legacy = intel_init_bsd_ring_buffer,
>  	},
>  	[VECS] = {
>  		.name = "vecs",
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c98acc2..81eee42 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2175,20 +2175,6 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
>  	return intel_init_ring_buffer(engine);
>  }
>
> -/**
> - * Initialize the second BSD ring (eg. Broadwell GT3, Skylake GT3)
> - */
> -int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
> -{
> -	struct drm_i915_private *dev_priv = engine->i915;
> -
> -	intel_ring_default_vfuncs(dev_priv, engine);
> -
> -	engine->emit_flush = gen6_bsd_ring_flush;
> -
> -	return intel_init_ring_buffer(engine);
> -}
> -
>  int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 4ab590b..5c1a27f 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -561,7 +561,6 @@ int intel_ring_pin(struct intel_ring *ring,
>
>  int intel_init_render_ring_buffer(struct intel_engine_cs *engine);
>  int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine);
> -int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine);
>  int intel_init_blt_ring_buffer(struct intel_engine_cs *engine);
>  int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine);
>
>

Job is not done until there is nothing left to remove! :)

Doesn't depend on anything so could have been first in the series, or 
separate, but doesn't matter.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] drm/i915: Generate the engine name based on the instance number
  2017-04-05  9:30 ` [PATCH 3/5] drm/i915: Generate the engine name based on the instance number Oscar Mateo
@ 2017-04-06 18:02   ` Tvrtko Ursulin
  2017-04-06 11:22     ` Oscar Mateo
  2017-04-06 18:10     ` Chris Wilson
  0 siblings, 2 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-04-06 18:02 UTC (permalink / raw)
  To: Oscar Mateo, intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi


On 05/04/2017 10:30, Oscar Mateo wrote:
> Not really needed, but makes the next change a little bit more compact.
>
> 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       | 8 ++++++--
>  drivers/gpu/drm/i915/intel_ringbuffer.h      | 4 +++-
>  drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +-
>  3 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index abc0a9c..530f822 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -71,7 +71,7 @@
>  		.init_legacy = intel_init_bsd_ring_buffer,
>  	},
>  	[VCS2] = {
> -		.name = "vcs2",
> +		.name = "vcs",

Rename the field to class_name perhaps?

>  		.hw_id = VCS2_HW,
>  		.exec_id = I915_EXEC_BSD,
>  		.class = VIDEO_DECODE_CLASS,
> @@ -100,6 +100,7 @@
>  {
>  	const struct engine_info *info = &intel_engines[id];
>  	struct intel_engine_cs *engine;
> +	char instance[3] = "";
>
>  	GEM_BUG_ON(dev_priv->engine[id]);
>  	engine = kzalloc(sizeof(*engine), GFP_KERNEL);
> @@ -108,7 +109,10 @@
>
>  	engine->id = id;
>  	engine->i915 = dev_priv;
> -	engine->name = info->name;
> +	/* For historical reasons the engines are called: name, name2... */
> +	if (info->instance)
> +		snprintf(instance, sizeof(instance), "%u", info->instance + 1);
> +	snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, instance);

Since Chris has recently renamed all the engines, I'd say who cares 
about the numbering scheme. Just drop it for simplicity.

>  	engine->exec_id = info->exec_id;
>  	engine->hw_id = engine->guc_id = info->hw_id;
>  	engine->mmio_base = info->mmio_base;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 5c1a27f..d617049 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -187,9 +187,11 @@ enum intel_engine_id {
>  	VECS
>  };
>
> +#define INTEL_ENGINE_CS_MAX_NAME 8
> +
>  struct intel_engine_cs {
>  	struct drm_i915_private *i915;
> -	const char	*name;
> +	char name[INTEL_ENGINE_CS_MAX_NAME];
>  	enum intel_engine_id id;
>  	unsigned int exec_id;
>  	unsigned int hw_id;
> diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c
> index b89050e..4a1ffca 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
> @@ -140,7 +140,7 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
>
>  	/* minimal engine setup for requests */
>  	engine->base.i915 = i915;
> -	engine->base.name = name;
> +	strncpy(engine->base.name, name, sizeof(engine->base.name) - 1);

Can this miss to null-terminate? Or it relies on the mock_engine being 
kzalloced?

>  	engine->base.id = id++;
>  	engine->base.status_page.page_addr = (void *)(engine + 1);
>
>

Regards,

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/5] drm/i915: Split the engine info table in two levels, using class + instance
  2017-04-05  9:30 ` [PATCH 4/5] drm/i915: Split the engine info table in two levels, using class + instance Oscar Mateo
@ 2017-04-06 18:09   ` Tvrtko Ursulin
  0 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-04-06 18:09 UTC (permalink / raw)
  To: Oscar Mateo, intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi


On 05/04/2017 10:30, Oscar Mateo wrote:

Commit message missing.

> 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 | 72 +++++++++++++++++++++-------------
>  1 file changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 530f822..691689c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -26,71 +26,83 @@
>  #include "intel_ringbuffer.h"
>  #include "intel_lrc.h"
>
> -static const struct engine_info {
> +struct engine_class_info {
>  	const char *name;
>  	unsigned int exec_id;
> +	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",
> +		.exec_id = I915_EXEC_RENDER,
> +		.init_execlists = logical_render_ring_init,
> +		.init_legacy = intel_init_render_ring_buffer,
> +	},
> +	[COPY_ENGINE_CLASS] = {
> +		.name = "bcs",
> +		.exec_id = I915_EXEC_BLT,
> +		.init_execlists = logical_xcs_ring_init,
> +		.init_legacy = intel_init_blt_ring_buffer,
> +	},
> +	[VIDEO_DECODE_CLASS] = {
> +		.name = "vcs",
> +		.exec_id = I915_EXEC_BSD,
> +		.init_execlists = logical_xcs_ring_init,
> +		.init_legacy = intel_init_bsd_ring_buffer,
> +	},
> +	[VIDEO_ENHANCEMENT_CLASS] = {
> +		.name = "vecs",
> +		.exec_id = I915_EXEC_VEBOX,
> +		.init_execlists = logical_xcs_ring_init,
> +		.init_legacy = intel_init_vebox_ring_buffer,
> +	},
> +};
> +
> +struct engine_info {
>  	unsigned int hw_id;
>  	enum intel_engine_class class;
>  	u32 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,
>  		.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,
>  		.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,
>  		.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,
>  		.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,
>  		.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 +111,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;
>  	char instance[3] = "";
>
> @@ -112,8 +126,8 @@
>  	/* For historical reasons the engines are called: name, name2... */
>  	if (info->instance)
>  		snprintf(instance, sizeof(instance), "%u", info->instance + 1);
> -	snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, instance);
> -	engine->exec_id = info->exec_id;
> +	snprintf(engine->name, sizeof(engine->name), "%s%s", class_info->name, instance);
> +	engine->exec_id = class_info->exec_id;
>  	engine->hw_id = engine->guc_id = info->hw_id;
>  	engine->mmio_base = info->mmio_base;
>  	engine->irq_shift = info->irq_shift;
> @@ -193,12 +207,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;
>

Looks OK to me but it will have to be rebased a bit if you accept my 
comments on the previous patches.

Regards,

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] drm/i915: Generate the engine name based on the instance number
  2017-04-06 18:02   ` Tvrtko Ursulin
  2017-04-06 11:22     ` Oscar Mateo
@ 2017-04-06 18:10     ` Chris Wilson
  1 sibling, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2017-04-06 18:10 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx, Paulo Zanoni, Rodrigo Vivi

On Thu, Apr 06, 2017 at 07:02:10PM +0100, Tvrtko Ursulin wrote:
> 
> On 05/04/2017 10:30, Oscar Mateo wrote:
> >@@ -100,6 +100,7 @@
> > {
> > 	const struct engine_info *info = &intel_engines[id];
> > 	struct intel_engine_cs *engine;
> >+	char instance[3] = "";
> >
> > 	GEM_BUG_ON(dev_priv->engine[id]);
> > 	engine = kzalloc(sizeof(*engine), GFP_KERNEL);
> >@@ -108,7 +109,10 @@
> >
> > 	engine->id = id;
> > 	engine->i915 = dev_priv;
> >-	engine->name = info->name;
> >+	/* For historical reasons the engines are called: name, name2... */
> >+	if (info->instance)
> >+		snprintf(instance, sizeof(instance), "%u", info->instance + 1);
> >+	snprintf(engine->name, sizeof(engine->name), "%s%s", info->name, instance);
> 
> Since Chris has recently renamed all the engines, I'd say who cares
> about the numbering scheme. Just drop it for simplicity.

Seconded. class0 is ok, or if you are being fancy only use the classN
format if there are multiples in the class. Though kiss says just use
classN and be done with it.
-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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 5/5] drm/i915: Use the engine class to get the context size
  2017-04-05  9:30 ` [PATCH 5/5] drm/i915: Use the engine class to get the context size Oscar Mateo
@ 2017-04-06 18:19   ` Tvrtko Ursulin
  0 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-04-06 18:19 UTC (permalink / raw)
  To: Oscar Mateo, intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi


On 05/04/2017 10:30, Oscar Mateo wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
> Technically speaking, the context size is per engine class, not per
> instance.

It is very nice to have the code match the documentation!

> 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>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 34 ++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_lrc.h |  7 ++++++-
>  2 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0dc1cc4..6b1fc4a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1908,8 +1908,10 @@ static void execlists_init_reg_state(u32 *regs,
>  }
>
>  /**
> - * intel_lr_context_size() - return the size of the context for an engine
> - * @engine: which engine to find the context size for
> + * intel_lr_class_context_size() - return the size of the context for a given
> + * engine class
> + * @dev_priv: i915 device private
> + * @class: which engine class to find the context size for
>   *
>   * Each engine may require a different amount of space for a context image,
>   * so when allocating (or copying) an image, this function can be used to
> @@ -1921,25 +1923,33 @@ static void execlists_init_reg_state(u32 *regs,
>   * in LRC mode, but does not include the "shared data page" used with
>   * GuC submission. The caller should account for this if using the GuC.
>   */
> -uint32_t intel_lr_context_size(struct intel_engine_cs *engine)
> +uint32_t intel_lr_class_context_size(struct drm_i915_private *dev_priv,
> +				     enum intel_engine_class class)
>  {
>  	int ret = 0;
>
> -	WARN_ON(INTEL_GEN(engine->i915) < 8);
> +	WARN_ON(INTEL_GEN(dev_priv) < 8);
>
> -	switch (engine->id) {
> -	case RCS:
> -		if (INTEL_GEN(engine->i915) >= 9)
> +	switch (class) {
> +	case RENDER_CLASS:
> +		switch (INTEL_GEN(dev_priv)) {
> +		default:
> +			DRM_ERROR("Unknown context size for GEN\n");

MISSING_CASE I think, otherwise it is too easy to miss in the noise. And 
it is MISSING_CASE for the class below which looks appropriate to me for 
this layer.

> +		case 9:
>  			ret = GEN9_LR_CONTEXT_RENDER_SIZE;
> -		else
> +			break;
> +		case 8:
>  			ret = GEN8_LR_CONTEXT_RENDER_SIZE;
> +			break;
> +		}
>  		break;
> -	case VCS:
> -	case BCS:
> -	case VECS:
> -	case VCS2:
> +	case VIDEO_DECODE_CLASS:
> +	case VIDEO_ENHANCEMENT_CLASS:
> +	case COPY_ENGINE_CLASS:
>  		ret = GEN8_LR_CONTEXT_OTHER_SIZE;
>  		break;
> +	default:
> +		MISSING_CASE(class);
>  	}
>
>  	return ret;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index e8015e7..b3a4331 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -78,7 +78,12 @@ enum {
>  struct drm_i915_private;
>  struct i915_gem_context;
>
> -uint32_t intel_lr_context_size(struct intel_engine_cs *engine);
> +uint32_t intel_lr_class_context_size(struct drm_i915_private *dev_priv,
> +				     enum intel_engine_class class);
> +static inline uint32_t intel_lr_context_size(struct intel_engine_cs *engine)
> +{
> +	return intel_lr_class_context_size(engine->i915, engine->class);
> +}
>
>  void intel_lr_context_resume(struct drm_i915_private *dev_priv);
>  uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
>

Otherwise looks fine to me. I also really hope to be able to use the 
class/instance cleanup in scope of the better VCS load balancing which 
is currently being looked at.

Regards,

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] drm/i915: Generate the engine name based on the instance number
  2017-04-06 11:22     ` Oscar Mateo
@ 2017-04-06 18:37       ` Tvrtko Ursulin
  2017-04-06 18:44         ` Chris Wilson
  0 siblings, 1 reply; 17+ messages in thread
From: Tvrtko Ursulin @ 2017-04-06 18:37 UTC (permalink / raw)
  To: Oscar Mateo, intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi


On 06/04/2017 12:22, Oscar Mateo wrote:
> On 04/06/2017 11:02 AM, Tvrtko Ursulin wrote:
>> On 05/04/2017 10:30, Oscar Mateo wrote:
>>> Not really needed, but makes the next change a little bit more compact.
>>>
>>> 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       | 8 ++++++--
>>>  drivers/gpu/drm/i915/intel_ringbuffer.h      | 4 +++-
>>>  drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +-
>>>  3 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
>>> b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> index abc0a9c..530f822 100644
>>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>>> @@ -71,7 +71,7 @@
>>>          .init_legacy = intel_init_bsd_ring_buffer,
>>>      },
>>>      [VCS2] = {
>>> -        .name = "vcs2",
>>> +        .name = "vcs",
>>
>> Rename the field to class_name perhaps?
>>
>
> In the following patch, when I move .name to the class table, it becomes
> much more obvious what it refers to, but I can change it to class_name
> if you feel strongly about it (or change it here and then back to name
> in the next patch?).

No it's fine like it is, just a consequence of not looking-ahead enough.

>
>>>          .hw_id = VCS2_HW,
>>>          .exec_id = I915_EXEC_BSD,
>>>          .class = VIDEO_DECODE_CLASS,
>>> @@ -100,6 +100,7 @@
>>>  {
>>>      const struct engine_info *info = &intel_engines[id];
>>>      struct intel_engine_cs *engine;
>>> +    char instance[3] = "";
>>>
>>>      GEM_BUG_ON(dev_priv->engine[id]);
>>>      engine = kzalloc(sizeof(*engine), GFP_KERNEL);
>>> @@ -108,7 +109,10 @@
>>>
>>>      engine->id = id;
>>>      engine->i915 = dev_priv;
>>> -    engine->name = info->name;
>>> +    /* For historical reasons the engines are called: name, name2... */
>>> +    if (info->instance)
>>> +        snprintf(instance, sizeof(instance), "%u", info->instance + 1);
>>> +    snprintf(engine->name, sizeof(engine->name), "%s%s", info->name,
>>> instance);
>>
>> Since Chris has recently renamed all the engines, I'd say who cares
>> about the numbering scheme. Just drop it for simplicity.
>>
>
> Oooohh!
> Can I also use zero-based numbering, like in the Bspec? (so xcs0, xcs1
> ... xcsN?)

Sounds like the verdict is yes.

>
>>>      engine->exec_id = info->exec_id;
>>>      engine->hw_id = engine->guc_id = info->hw_id;
>>>      engine->mmio_base = info->mmio_base;
>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> index 5c1a27f..d617049 100644
>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>>> @@ -187,9 +187,11 @@ enum intel_engine_id {
>>>      VECS
>>>  };
>>>
>>> +#define INTEL_ENGINE_CS_MAX_NAME 8
>>> +
>>>  struct intel_engine_cs {
>>>      struct drm_i915_private *i915;
>>> -    const char    *name;
>>> +    char name[INTEL_ENGINE_CS_MAX_NAME];
>>>      enum intel_engine_id id;
>>>      unsigned int exec_id;
>>>      unsigned int hw_id;
>>> diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c
>>> b/drivers/gpu/drm/i915/selftests/mock_engine.c
>>> index b89050e..4a1ffca 100644
>>> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c
>>> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
>>> @@ -140,7 +140,7 @@ struct intel_engine_cs *mock_engine(struct
>>> drm_i915_private *i915,
>>>
>>>      /* minimal engine setup for requests */
>>>      engine->base.i915 = i915;
>>> -    engine->base.name = name;
>>> +    strncpy(engine->base.name, name, sizeof(engine->base.name) - 1);
>>
>> Can this miss to null-terminate? Or it relies on the mock_engine being
>> kzalloced?
>>
>
> It relies on the kzalloc, but I can add a \0 at the end for extra security?

Not sure at the moment without checking in detail how much mock_engine 
already depends on being zeroed. But I guess null-terminating it at the 
end wouldn't harm. Or maybe snprintf to eliminate the dilemma if it 
creates neater code?

Regards,

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] drm/i915: Generate the engine name based on the instance number
  2017-04-06 18:37       ` Tvrtko Ursulin
@ 2017-04-06 18:44         ` Chris Wilson
  0 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2017-04-06 18:44 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx, Paulo Zanoni, Rodrigo Vivi

On Thu, Apr 06, 2017 at 07:37:20PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/04/2017 12:22, Oscar Mateo wrote:
> >On 04/06/2017 11:02 AM, Tvrtko Ursulin wrote:
> >>On 05/04/2017 10:30, Oscar Mateo wrote:
> >>>Not really needed, but makes the next change a little bit more compact.
> >>>
> >>>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       | 8 ++++++--
> >>> drivers/gpu/drm/i915/intel_ringbuffer.h      | 4 +++-
> >>> drivers/gpu/drm/i915/selftests/mock_engine.c | 2 +-
> >>> 3 files changed, 10 insertions(+), 4 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> >>>b/drivers/gpu/drm/i915/intel_engine_cs.c
> >>>index abc0a9c..530f822 100644
> >>>--- a/drivers/gpu/drm/i915/intel_engine_cs.c
> >>>+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> >>>@@ -71,7 +71,7 @@
> >>>         .init_legacy = intel_init_bsd_ring_buffer,
> >>>     },
> >>>     [VCS2] = {
> >>>-        .name = "vcs2",
> >>>+        .name = "vcs",
> >>
> >>Rename the field to class_name perhaps?
> >>
> >
> >In the following patch, when I move .name to the class table, it becomes
> >much more obvious what it refers to, but I can change it to class_name
> >if you feel strongly about it (or change it here and then back to name
> >in the next patch?).
> 
> No it's fine like it is, just a consequence of not looking-ahead enough.
> 
> >
> >>>         .hw_id = VCS2_HW,
> >>>         .exec_id = I915_EXEC_BSD,
> >>>         .class = VIDEO_DECODE_CLASS,
> >>>@@ -100,6 +100,7 @@
> >>> {
> >>>     const struct engine_info *info = &intel_engines[id];
> >>>     struct intel_engine_cs *engine;
> >>>+    char instance[3] = "";
> >>>
> >>>     GEM_BUG_ON(dev_priv->engine[id]);
> >>>     engine = kzalloc(sizeof(*engine), GFP_KERNEL);
> >>>@@ -108,7 +109,10 @@
> >>>
> >>>     engine->id = id;
> >>>     engine->i915 = dev_priv;
> >>>-    engine->name = info->name;
> >>>+    /* For historical reasons the engines are called: name, name2... */
> >>>+    if (info->instance)
> >>>+        snprintf(instance, sizeof(instance), "%u", info->instance + 1);
> >>>+    snprintf(engine->name, sizeof(engine->name), "%s%s", info->name,
> >>>instance);
> >>
> >>Since Chris has recently renamed all the engines, I'd say who cares
> >>about the numbering scheme. Just drop it for simplicity.
> >>
> >
> >Oooohh!
> >Can I also use zero-based numbering, like in the Bspec? (so xcs0, xcs1
> >... xcsN?)
> 
> Sounds like the verdict is yes.
> 
> >
> >>>     engine->exec_id = info->exec_id;
> >>>     engine->hw_id = engine->guc_id = info->hw_id;
> >>>     engine->mmio_base = info->mmio_base;
> >>>diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>index 5c1a27f..d617049 100644
> >>>--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >>>@@ -187,9 +187,11 @@ enum intel_engine_id {
> >>>     VECS
> >>> };
> >>>
> >>>+#define INTEL_ENGINE_CS_MAX_NAME 8
> >>>+
> >>> struct intel_engine_cs {
> >>>     struct drm_i915_private *i915;
> >>>-    const char    *name;
> >>>+    char name[INTEL_ENGINE_CS_MAX_NAME];
> >>>     enum intel_engine_id id;
> >>>     unsigned int exec_id;
> >>>     unsigned int hw_id;
> >>>diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c
> >>>b/drivers/gpu/drm/i915/selftests/mock_engine.c
> >>>index b89050e..4a1ffca 100644
> >>>--- a/drivers/gpu/drm/i915/selftests/mock_engine.c
> >>>+++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
> >>>@@ -140,7 +140,7 @@ struct intel_engine_cs *mock_engine(struct
> >>>drm_i915_private *i915,
> >>>
> >>>     /* minimal engine setup for requests */
> >>>     engine->base.i915 = i915;
> >>>-    engine->base.name = name;
> >>>+    strncpy(engine->base.name, name, sizeof(engine->base.name) - 1);
> >>
> >>Can this miss to null-terminate? Or it relies on the mock_engine being
> >>kzalloced?
> >>
> >
> >It relies on the kzalloc, but I can add a \0 at the end for extra security?
> 
> Not sure at the moment without checking in detail how much
> mock_engine already depends on being zeroed.

It does depend on the kzalloc.

> But I guess
> null-terminating it at the end wouldn't harm. Or maybe snprintf to
> eliminate the dilemma if it creates neater code?

Indeed, I want my mockN! We may want multiple instances of the mock
engine class in the future.
-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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2017-04-06 18:44 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-05  9:30 [PATCH 0/5] Classify the engines in class + instance Oscar Mateo
2017-04-05  9:30 ` [PATCH 1/5] drm/i915: " Oscar Mateo
2017-04-06 17:45   ` Tvrtko Ursulin
2017-04-05  9:30 ` [PATCH 2/5] drm/i915: Use the same vfunc for BSD2 ring init Oscar Mateo
2017-04-06 17:48   ` Tvrtko Ursulin
2017-04-06 11:14     ` Oscar Mateo
2017-04-05  9:30 ` [PATCH 3/5] drm/i915: Generate the engine name based on the instance number Oscar Mateo
2017-04-06 18:02   ` Tvrtko Ursulin
2017-04-06 11:22     ` Oscar Mateo
2017-04-06 18:37       ` Tvrtko Ursulin
2017-04-06 18:44         ` Chris Wilson
2017-04-06 18:10     ` Chris Wilson
2017-04-05  9:30 ` [PATCH 4/5] drm/i915: Split the engine info table in two levels, using class + instance Oscar Mateo
2017-04-06 18:09   ` Tvrtko Ursulin
2017-04-05  9:30 ` [PATCH 5/5] drm/i915: Use the engine class to get the context size Oscar Mateo
2017-04-06 18:19   ` Tvrtko Ursulin
2017-04-05 16:58 ` ✓ Fi.CI.BAT: success for Classify the engines in class + instance Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.