All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5] drm/i915: unify first-stage engine struct setup
@ 2016-07-01 16:47 Tvrtko Ursulin
  2016-07-01 16:47 ` [PATCH 2/5] drm/i915: Prepare for engine init unification Tvrtko Ursulin
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Tvrtko Ursulin @ 2016-07-01 16:47 UTC (permalink / raw)
  To: Intel-gfx

From: Dave Gordon <david.s.gordon@intel.com>

intel_lrc.c has a table of "logical rings" (meaning engines), while
intel_ringbuffer.c has separately open-coded initialisation for each
engine. We can deduplicate this somewhat by using the same first-stage
engine-setup function for both modes.

So here we expose the function that transfers information from the
static table of (all) known engines to the dev_priv->engine array of
engines available on this device (adjusting the names along the way)
and then embed calls to it in both the LRC and the legacy-mode setup.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 40 +++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_ringbuffer.c | 40 +++++++++------------------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  5 +++++
 3 files changed, 41 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 339d8041075f..ed017f1a07a2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1994,8 +1994,9 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
 }
 
 static inline void
-logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
+logical_ring_default_irqs(struct intel_engine_cs *engine)
 {
+	unsigned shift = engine->irq_shift;
 	engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
 	engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
 	init_waitqueue_head(&engine->irq_queue);
@@ -2096,14 +2097,14 @@ static int logical_render_ring_init(struct intel_engine_cs *engine)
 	return ret;
 }
 
-static const struct logical_ring_info {
+static const struct engine_info {
 	const char *name;
 	unsigned exec_id;
 	unsigned guc_id;
 	u32 mmio_base;
 	unsigned irq_shift;
 	int (*init)(struct intel_engine_cs *engine);
-} logical_rings[] = {
+} intel_engines[] = {
 	[RCS] = {
 		.name = "render ring",
 		.exec_id = I915_EXEC_RENDER,
@@ -2146,20 +2147,31 @@ static const struct logical_ring_info {
 	},
 };
 
-static struct intel_engine_cs *
-logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
+struct intel_engine_cs *
+intel_engine_setup(struct drm_i915_private *dev_priv,
+		   enum intel_engine_id id)
 {
-	const struct logical_ring_info *info = &logical_rings[id];
+	const struct engine_info *info = &intel_engines[id];
 	struct intel_engine_cs *engine = &dev_priv->engine[id];
-	enum forcewake_domains fw_domains;
 
 	engine->id = id;
+	engine->i915 = dev_priv;
 	engine->name = info->name;
 	engine->exec_id = info->exec_id;
-	engine->guc_id = info->guc_id;
+	engine->hw_id = engine->guc_id = info->guc_id;
 	engine->mmio_base = info->mmio_base;
+	engine->irq_shift = info->irq_shift;
 
-	engine->i915 = dev_priv;
+	return engine;
+}
+
+static struct intel_engine_cs *
+logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
+{
+	struct intel_engine_cs *engine;
+	enum forcewake_domains fw_domains;
+
+	engine = intel_engine_setup(dev_priv, id);
 
 	/* Intentionally left blank. */
 	engine->buffer = NULL;
@@ -2189,7 +2201,7 @@ logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
 
 	logical_ring_init_platform_invariants(engine);
 	logical_ring_default_vfuncs(engine);
-	logical_ring_default_irqs(engine, info->irq_shift);
+	logical_ring_default_irqs(engine);
 
 	intel_engine_init_hangcheck(engine);
 	i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool);
@@ -2218,14 +2230,14 @@ int intel_logical_rings_init(struct drm_device *dev)
 	WARN_ON(INTEL_INFO(dev_priv)->ring_mask &
 		GENMASK(sizeof(mask) * BITS_PER_BYTE - 1, I915_NUM_ENGINES));
 
-	for (i = 0; i < ARRAY_SIZE(logical_rings); i++) {
+	for (i = 0; i < ARRAY_SIZE(intel_engines); i++) {
 		if (!HAS_ENGINE(dev_priv, i))
 			continue;
 
-		if (!logical_rings[i].init)
+		if (!intel_engines[i].init)
 			continue;
 
-		ret = logical_rings[i].init(logical_ring_setup(dev_priv, i));
+		ret = intel_engines[i].init(logical_ring_setup(dev_priv, i));
 		if (ret)
 			goto cleanup;
 
@@ -2233,7 +2245,7 @@ int intel_logical_rings_init(struct drm_device *dev)
 	}
 
 	/*
-	 * Catch failures to update logical_rings table when the new engines
+	 * Catch failures to update intel_engines table when the new engines
 	 * are added to the driver by a warning and disabling the forgotten
 	 * engines.
 	 */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 24cdc920f4b4..215424efe05c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -3036,15 +3036,11 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
 int intel_init_render_ring_buffer(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine = &dev_priv->engine[RCS];
+	struct intel_engine_cs *engine;
 	struct drm_i915_gem_object *obj;
 	int ret;
 
-	engine->name = "render ring";
-	engine->id = RCS;
-	engine->exec_id = I915_EXEC_RENDER;
-	engine->hw_id = 0;
-	engine->mmio_base = RENDER_RING_BASE;
+	engine = intel_engine_setup(dev_priv, RCS);
 
 	intel_ring_default_vfuncs(dev_priv, engine);
 
@@ -3117,17 +3113,13 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 int intel_init_bsd_ring_buffer(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine = &dev_priv->engine[VCS];
+	struct intel_engine_cs *engine;
 
-	engine->name = "bsd ring";
-	engine->id = VCS;
-	engine->exec_id = I915_EXEC_BSD;
-	engine->hw_id = 1;
+	engine = intel_engine_setup(dev_priv, VCS);
 
 	intel_ring_default_vfuncs(dev_priv, engine);
 
 	if (INTEL_GEN(dev_priv) >= 6) {
-		engine->mmio_base = GEN6_BSD_RING_BASE;
 		/* gen6 bsd needs a special wa for tail updates */
 		if (IS_GEN6(dev_priv))
 			engine->write_tail = gen6_bsd_ring_write_tail;
@@ -3155,13 +3147,9 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 int intel_init_bsd2_ring_buffer(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine = &dev_priv->engine[VCS2];
+	struct intel_engine_cs *engine;
 
-	engine->name = "bsd2 ring";
-	engine->id = VCS2;
-	engine->exec_id = I915_EXEC_BSD;
-	engine->hw_id = 4;
-	engine->mmio_base = GEN8_BSD2_RING_BASE;
+	engine = intel_engine_setup(dev_priv, VCS2);
 
 	intel_ring_default_vfuncs(dev_priv, engine);
 
@@ -3175,13 +3163,9 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
 int intel_init_blt_ring_buffer(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine = &dev_priv->engine[BCS];
+	struct intel_engine_cs *engine;
 
-	engine->name = "blitter ring";
-	engine->id = BCS;
-	engine->exec_id = I915_EXEC_BLT;
-	engine->hw_id = 2;
-	engine->mmio_base = BLT_RING_BASE;
+	engine = intel_engine_setup(dev_priv, BCS);
 
 	intel_ring_default_vfuncs(dev_priv, engine);
 
@@ -3198,13 +3182,9 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
 int intel_init_vebox_ring_buffer(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine = &dev_priv->engine[VECS];
+	struct intel_engine_cs *engine;
 
-	engine->name = "video enhancement ring";
-	engine->id = VECS;
-	engine->exec_id = I915_EXEC_VEBOX;
-	engine->hw_id = 3;
-	engine->mmio_base = VEBOX_RING_BASE;
+	engine = intel_engine_setup(dev_priv, VECS);
 
 	intel_ring_default_vfuncs(dev_priv, engine);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 113d5230a6de..1aeb00cba9e2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -145,6 +145,7 @@ struct intel_engine_cs {
 	unsigned int hw_id;
 	unsigned int guc_id; /* XXX same as hw_id? */
 	u32		mmio_base;
+	unsigned int irq_shift;
 	struct intel_ringbuffer *buffer;
 	struct list_head buffers;
 
@@ -335,6 +336,10 @@ struct intel_engine_cs {
 	u32 (*get_cmd_length_mask)(u32 cmd_header);
 };
 
+struct intel_engine_cs *
+intel_engine_setup(struct drm_i915_private *dev_priv,
+		   enum intel_engine_id id);
+
 static inline bool
 intel_engine_initialized(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] 14+ messages in thread
* [PATCH 2/5] drm/i915: Prepare for engine init unification
  2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
@ 2016-07-01 16:47 ` Tvrtko Ursulin
  2016-07-01 16:47 ` [PATCH 3/5] drm/i915: Unify engine init loop Tvrtko Ursulin
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Tvrtko Ursulin @ 2016-07-01 16:47 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Move the execlist engine setup to vfuncs so that the engine
init loop is clearly split into the mode agnostic and
specific steps.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 103 ++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ed017f1a07a2..abb165019af7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2020,6 +2020,46 @@ lrc_setup_hws(struct intel_engine_cs *engine,
 	return 0;
 }
 
+static void
+logical_ring_setup(struct intel_engine_cs *engine)
+{
+	struct drm_i915_private *dev_priv = engine->i915;
+	enum forcewake_domains fw_domains;
+
+	/* Intentionally left blank. */
+	engine->buffer = NULL;
+
+	fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
+						    RING_ELSP(engine),
+						    FW_REG_WRITE);
+
+	fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
+						     RING_CONTEXT_STATUS_PTR(engine),
+						     FW_REG_READ | FW_REG_WRITE);
+
+	fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
+						     RING_CONTEXT_STATUS_BUF_BASE(engine),
+						     FW_REG_READ);
+
+	engine->fw_domains = fw_domains;
+
+	INIT_LIST_HEAD(&engine->active_list);
+	INIT_LIST_HEAD(&engine->request_list);
+	INIT_LIST_HEAD(&engine->buffers);
+	INIT_LIST_HEAD(&engine->execlist_queue);
+	spin_lock_init(&engine->execlist_lock);
+
+	tasklet_init(&engine->irq_tasklet,
+		     intel_lrc_irq_handler, (unsigned long)engine);
+
+	logical_ring_init_platform_invariants(engine);
+	logical_ring_default_vfuncs(engine);
+	logical_ring_default_irqs(engine);
+
+	intel_engine_init_hangcheck(engine);
+	i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool);
+}
+
 static int
 logical_ring_init(struct intel_engine_cs *engine)
 {
@@ -2061,6 +2101,8 @@ static int logical_render_ring_init(struct intel_engine_cs *engine)
 	struct drm_i915_private *dev_priv = engine->i915;
 	int ret;
 
+	logical_ring_setup(engine);
+
 	if (HAS_L3_DPF(dev_priv))
 		engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
 
@@ -2097,6 +2139,13 @@ static int logical_render_ring_init(struct intel_engine_cs *engine)
 	return ret;
 }
 
+static int logical_xcs_ring_init(struct intel_engine_cs *engine)
+{
+	logical_ring_setup(engine);
+
+	return logical_ring_init(engine);
+}
+
 static const struct engine_info {
 	const char *name;
 	unsigned exec_id;
@@ -2119,7 +2168,7 @@ static const struct engine_info {
 		.guc_id = GUC_BLITTER_ENGINE,
 		.mmio_base = BLT_RING_BASE,
 		.irq_shift = GEN8_BCS_IRQ_SHIFT,
-		.init = logical_ring_init,
+		.init = logical_xcs_ring_init,
 	},
 	[VCS] = {
 		.name = "bsd ring",
@@ -2127,7 +2176,7 @@ static const struct engine_info {
 		.guc_id = GUC_VIDEO_ENGINE,
 		.mmio_base = GEN6_BSD_RING_BASE,
 		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
-		.init = logical_ring_init,
+		.init = logical_xcs_ring_init,
 	},
 	[VCS2] = {
 		.name = "bsd2 ring",
@@ -2135,7 +2184,7 @@ static const struct engine_info {
 		.guc_id = GUC_VIDEO_ENGINE2,
 		.mmio_base = GEN8_BSD2_RING_BASE,
 		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
-		.init = logical_ring_init,
+		.init = logical_xcs_ring_init,
 	},
 	[VECS] = {
 		.name = "video enhancement ring",
@@ -2143,7 +2192,7 @@ static const struct engine_info {
 		.guc_id = GUC_VIDEOENHANCE_ENGINE,
 		.mmio_base = VEBOX_RING_BASE,
 		.irq_shift = GEN8_VECS_IRQ_SHIFT,
-		.init = logical_ring_init,
+		.init = logical_xcs_ring_init,
 	},
 };
 
@@ -2165,50 +2214,6 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 	return engine;
 }
 
-static struct intel_engine_cs *
-logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
-{
-	struct intel_engine_cs *engine;
-	enum forcewake_domains fw_domains;
-
-	engine = intel_engine_setup(dev_priv, id);
-
-	/* Intentionally left blank. */
-	engine->buffer = NULL;
-
-	fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
-						    RING_ELSP(engine),
-						    FW_REG_WRITE);
-
-	fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
-						     RING_CONTEXT_STATUS_PTR(engine),
-						     FW_REG_READ | FW_REG_WRITE);
-
-	fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
-						     RING_CONTEXT_STATUS_BUF_BASE(engine),
-						     FW_REG_READ);
-
-	engine->fw_domains = fw_domains;
-
-	INIT_LIST_HEAD(&engine->active_list);
-	INIT_LIST_HEAD(&engine->request_list);
-	INIT_LIST_HEAD(&engine->buffers);
-	INIT_LIST_HEAD(&engine->execlist_queue);
-	spin_lock_init(&engine->execlist_lock);
-
-	tasklet_init(&engine->irq_tasklet,
-		     intel_lrc_irq_handler, (unsigned long)engine);
-
-	logical_ring_init_platform_invariants(engine);
-	logical_ring_default_vfuncs(engine);
-	logical_ring_default_irqs(engine);
-
-	intel_engine_init_hangcheck(engine);
-	i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool);
-
-	return engine;
-}
-
 /**
  * intel_logical_rings_init() - allocate, populate and init the Engine Command Streamers
  * @dev: DRM device.
@@ -2237,7 +2242,7 @@ int intel_logical_rings_init(struct drm_device *dev)
 		if (!intel_engines[i].init)
 			continue;
 
-		ret = intel_engines[i].init(logical_ring_setup(dev_priv, i));
+		ret = intel_engines[i].init(intel_engine_setup(dev_priv, i));
 		if (ret)
 			goto cleanup;
 
-- 
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] 14+ messages in thread
* [PATCH 3/5] drm/i915: Unify engine init loop
  2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
  2016-07-01 16:47 ` [PATCH 2/5] drm/i915: Prepare for engine init unification Tvrtko Ursulin
@ 2016-07-01 16:47 ` Tvrtko Ursulin
  2016-07-01 16:47 ` [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup Tvrtko Ursulin
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Tvrtko Ursulin @ 2016-07-01 16:47 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

With the unified common engine setup done, and the execlist engine
initialization loop clearly split into two phases, we can eliminate
the separate legacy engine initialization code.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  2 --
 drivers/gpu/drm/i915/i915_gem.c         | 51 +--------------------------------
 drivers/gpu/drm/i915/intel_lrc.c        | 25 ++++++++++------
 drivers/gpu/drm/i915/intel_lrc.h        |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 47 +++++++++++-------------------
 drivers/gpu/drm/i915/intel_ringbuffer.h | 10 +++----
 6 files changed, 39 insertions(+), 98 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 485ab1148181..c058cb8150be 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2030,7 +2030,6 @@ struct drm_i915_private {
 		int (*execbuf_submit)(struct i915_execbuffer_params *params,
 				      struct drm_i915_gem_execbuffer2 *args,
 				      struct list_head *vmas);
-		int (*init_engines)(struct drm_device *dev);
 		void (*cleanup_engine)(struct intel_engine_cs *engine);
 		void (*stop_engine)(struct intel_engine_cs *engine);
 	} gt;
@@ -3342,7 +3341,6 @@ static inline bool i915_stop_ring_allow_warn(struct drm_i915_private *dev_priv)
 void i915_gem_reset(struct drm_device *dev);
 bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
 int __must_check i915_gem_init(struct drm_device *dev);
-int i915_gem_init_engines(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_cleanup_engines(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1d9878258103..3b5c99833ddd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5029,53 +5029,6 @@ static void init_unused_rings(struct drm_device *dev)
 	}
 }
 
-int i915_gem_init_engines(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
-
-	ret = intel_init_render_ring_buffer(dev);
-	if (ret)
-		return ret;
-
-	if (HAS_BSD(dev)) {
-		ret = intel_init_bsd_ring_buffer(dev);
-		if (ret)
-			goto cleanup_render_ring;
-	}
-
-	if (HAS_BLT(dev)) {
-		ret = intel_init_blt_ring_buffer(dev);
-		if (ret)
-			goto cleanup_bsd_ring;
-	}
-
-	if (HAS_VEBOX(dev)) {
-		ret = intel_init_vebox_ring_buffer(dev);
-		if (ret)
-			goto cleanup_blt_ring;
-	}
-
-	if (HAS_BSD2(dev)) {
-		ret = intel_init_bsd2_ring_buffer(dev);
-		if (ret)
-			goto cleanup_vebox_ring;
-	}
-
-	return 0;
-
-cleanup_vebox_ring:
-	intel_cleanup_engine(&dev_priv->engine[VECS]);
-cleanup_blt_ring:
-	intel_cleanup_engine(&dev_priv->engine[BCS]);
-cleanup_bsd_ring:
-	intel_cleanup_engine(&dev_priv->engine[VCS]);
-cleanup_render_ring:
-	intel_cleanup_engine(&dev_priv->engine[RCS]);
-
-	return ret;
-}
-
 int
 i915_gem_init_hw(struct drm_device *dev)
 {
@@ -5157,12 +5110,10 @@ int i915_gem_init(struct drm_device *dev)
 
 	if (!i915.enable_execlists) {
 		dev_priv->gt.execbuf_submit = i915_gem_ringbuffer_submission;
-		dev_priv->gt.init_engines = i915_gem_init_engines;
 		dev_priv->gt.cleanup_engine = intel_cleanup_engine;
 		dev_priv->gt.stop_engine = intel_stop_engine;
 	} else {
 		dev_priv->gt.execbuf_submit = intel_execlists_submission;
-		dev_priv->gt.init_engines = intel_logical_rings_init;
 		dev_priv->gt.cleanup_engine = intel_logical_ring_cleanup;
 		dev_priv->gt.stop_engine = intel_logical_ring_stop;
 	}
@@ -5182,7 +5133,7 @@ int i915_gem_init(struct drm_device *dev)
 	if (ret)
 		goto out_unlock;
 
-	ret = dev_priv->gt.init_engines(dev);
+	ret = intel_engines_init(dev);
 	if (ret)
 		goto out_unlock;
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index abb165019af7..bf8f4a67c75b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2153,6 +2153,7 @@ static const struct engine_info {
 	u32 mmio_base;
 	unsigned irq_shift;
 	int (*init)(struct intel_engine_cs *engine);
+	int (*init_ringbuf)(struct intel_engine_cs *engine);
 } intel_engines[] = {
 	[RCS] = {
 		.name = "render ring",
@@ -2161,6 +2162,7 @@ static const struct engine_info {
 		.mmio_base = RENDER_RING_BASE,
 		.irq_shift = GEN8_RCS_IRQ_SHIFT,
 		.init = logical_render_ring_init,
+		.init_ringbuf = intel_init_render_ring_buffer,
 	},
 	[BCS] = {
 		.name = "blitter ring",
@@ -2169,6 +2171,7 @@ static const struct engine_info {
 		.mmio_base = BLT_RING_BASE,
 		.irq_shift = GEN8_BCS_IRQ_SHIFT,
 		.init = logical_xcs_ring_init,
+		.init_ringbuf = intel_init_blt_ring_buffer,
 	},
 	[VCS] = {
 		.name = "bsd ring",
@@ -2177,6 +2180,7 @@ static const struct engine_info {
 		.mmio_base = GEN6_BSD_RING_BASE,
 		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
 		.init = logical_xcs_ring_init,
+		.init_ringbuf = intel_init_bsd_ring_buffer,
 	},
 	[VCS2] = {
 		.name = "bsd2 ring",
@@ -2185,6 +2189,7 @@ static const struct engine_info {
 		.mmio_base = GEN8_BSD2_RING_BASE,
 		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
 		.init = logical_xcs_ring_init,
+		.init_ringbuf = intel_init_bsd2_ring_buffer,
 	},
 	[VECS] = {
 		.name = "video enhancement ring",
@@ -2193,6 +2198,7 @@ static const struct engine_info {
 		.mmio_base = VEBOX_RING_BASE,
 		.irq_shift = GEN8_VECS_IRQ_SHIFT,
 		.init = logical_xcs_ring_init,
+		.init_ringbuf = intel_init_vebox_ring_buffer,
 	},
 };
 
@@ -2215,20 +2221,16 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_logical_rings_init() - allocate, populate and init the Engine Command Streamers
+ * intel_engines_init() - allocate, populate and init the Engine Command Streamers
  * @dev: DRM device.
  *
- * This function inits the engines for an Execlists submission style (the
- * equivalent in the legacy ringbuffer submission world would be
- * i915_gem_init_engines). It does it only for those engines that are present in
- * the hardware.
- *
  * Return: non-zero if the initialization failed.
  */
-int intel_logical_rings_init(struct drm_device *dev)
+int intel_engines_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned int mask = 0;
+	int (*init)(struct intel_engine_cs *engine);
 	unsigned int i;
 	int ret;
 
@@ -2239,10 +2241,15 @@ int intel_logical_rings_init(struct drm_device *dev)
 		if (!HAS_ENGINE(dev_priv, i))
 			continue;
 
-		if (!intel_engines[i].init)
+		if (i915.enable_execlists)
+			init = intel_engines[i].init;
+		else
+			init = intel_engines[i].init_ringbuf;
+
+		if (!init)
 			continue;
 
-		ret = intel_engines[i].init(intel_engine_setup(dev_priv, i));
+		ret = init(intel_engine_setup(dev_priv, i));
 		if (ret)
 			goto cleanup;
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 2b8255c19dcc..aa8905c42de7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -67,7 +67,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
 void intel_logical_ring_stop(struct intel_engine_cs *engine);
 void intel_logical_ring_cleanup(struct intel_engine_cs *engine);
-int intel_logical_rings_init(struct drm_device *dev);
+int intel_engines_init(struct drm_device *dev);
 
 int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
 /**
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 215424efe05c..a00adc3438f3 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -3033,15 +3033,12 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
 	intel_ring_init_semaphores(dev_priv, engine);
 }
 
-int intel_init_render_ring_buffer(struct drm_device *dev)
+int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine;
+	struct drm_i915_private *dev_priv = engine->i915;
 	struct drm_i915_gem_object *obj;
 	int ret;
 
-	engine = intel_engine_setup(dev_priv, RCS);
-
 	intel_ring_default_vfuncs(dev_priv, engine);
 
 	if (INTEL_GEN(dev_priv) >= 8) {
@@ -3080,7 +3077,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 
 	/* Workaround batchbuffer to combat CS tlb bug. */
 	if (HAS_BROKEN_CS_TLB(dev_priv)) {
-		obj = i915_gem_object_create(dev, I830_WA_SIZE);
+		obj = i915_gem_object_create(dev_priv->dev, I830_WA_SIZE);
 		if (IS_ERR(obj)) {
 			DRM_ERROR("Failed to allocate batch bo\n");
 			return PTR_ERR(obj);
@@ -3097,7 +3094,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		engine->scratch.gtt_offset = i915_gem_obj_ggtt_offset(obj);
 	}
 
-	ret = intel_init_ring_buffer(dev, engine);
+	ret = intel_init_ring_buffer(dev_priv->dev, engine);
 	if (ret)
 		return ret;
 
@@ -3110,12 +3107,9 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 	return 0;
 }
 
-int intel_init_bsd_ring_buffer(struct drm_device *dev)
+int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine;
-
-	engine = intel_engine_setup(dev_priv, VCS);
+	struct drm_i915_private *dev_priv = engine->i915;
 
 	intel_ring_default_vfuncs(dev_priv, engine);
 
@@ -3138,18 +3132,15 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 			engine->irq_enable_mask = I915_BSD_USER_INTERRUPT;
 	}
 
-	return intel_init_ring_buffer(dev, engine);
+	return intel_init_ring_buffer(dev_priv->dev, engine);
 }
 
 /**
  * Initialize the second BSD ring (eg. Broadwell GT3, Skylake GT3)
  */
-int intel_init_bsd2_ring_buffer(struct drm_device *dev)
+int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine;
-
-	engine = intel_engine_setup(dev_priv, VCS2);
+	struct drm_i915_private *dev_priv = engine->i915;
 
 	intel_ring_default_vfuncs(dev_priv, engine);
 
@@ -3157,15 +3148,12 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
 	engine->irq_enable_mask =
 			GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT;
 
-	return intel_init_ring_buffer(dev, engine);
+	return intel_init_ring_buffer(dev_priv->dev, engine);
 }
 
-int intel_init_blt_ring_buffer(struct drm_device *dev)
+int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine;
-
-	engine = intel_engine_setup(dev_priv, BCS);
+	struct drm_i915_private *dev_priv = engine->i915;
 
 	intel_ring_default_vfuncs(dev_priv, engine);
 
@@ -3176,15 +3164,12 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
 	else
 		engine->irq_enable_mask = GT_BLT_USER_INTERRUPT;
 
-	return intel_init_ring_buffer(dev, engine);
+	return intel_init_ring_buffer(dev_priv->dev, engine);
 }
 
-int intel_init_vebox_ring_buffer(struct drm_device *dev)
+int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine;
-
-	engine = intel_engine_setup(dev_priv, VECS);
+	struct drm_i915_private *dev_priv = engine->i915;
 
 	intel_ring_default_vfuncs(dev_priv, engine);
 
@@ -3199,7 +3184,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
 		engine->irq_put = hsw_vebox_put_irq;
 	}
 
-	return intel_init_ring_buffer(dev, engine);
+	return intel_init_ring_buffer(dev_priv->dev, engine);
 }
 
 int
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 1aeb00cba9e2..a843164ab386 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -459,11 +459,11 @@ int intel_ring_invalidate_all_caches(struct drm_i915_gem_request *req);
 void intel_fini_pipe_control(struct intel_engine_cs *engine);
 int intel_init_pipe_control(struct intel_engine_cs *engine);
 
-int intel_init_render_ring_buffer(struct drm_device *dev);
-int intel_init_bsd_ring_buffer(struct drm_device *dev);
-int intel_init_bsd2_ring_buffer(struct drm_device *dev);
-int intel_init_blt_ring_buffer(struct drm_device *dev);
-int intel_init_vebox_ring_buffer(struct drm_device *dev);
+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);
 
 u64 intel_ring_get_active_head(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] 14+ messages in thread
* [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup
  2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
  2016-07-01 16:47 ` [PATCH 2/5] drm/i915: Prepare for engine init unification Tvrtko Ursulin
  2016-07-01 16:47 ` [PATCH 3/5] drm/i915: Unify engine init loop Tvrtko Ursulin
@ 2016-07-01 16:47 ` Tvrtko Ursulin
  2016-07-13 12:30   ` Daniel Vetter
  2016-07-01 16:47 ` [PATCH 5/5] drm/i915: Simplify intel_init_ring_buffer prototype Tvrtko Ursulin
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Tvrtko Ursulin @ 2016-07-01 16:47 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Use it for legacy engine initialization by adding a
intel_ring_default_irqs helper used by individual engines.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 36 +++++++++++++++------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index a00adc3438f3..964776bb181c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2394,8 +2394,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	memset(engine->semaphore.sync_seqno, 0,
 	       sizeof(engine->semaphore.sync_seqno));
 
-	init_waitqueue_head(&engine->irq_queue);
-
 	/* We may need to do things with the shrinker which
 	 * require us to immediately switch back to the default
 	 * context. This can cause a problem as pinning the
@@ -3033,6 +3031,13 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
 	intel_ring_init_semaphores(dev_priv, engine);
 }
 
+static void
+intel_ring_default_irqs(struct intel_engine_cs *engine)
+{
+	engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << engine->irq_shift;
+	init_waitqueue_head(&engine->irq_queue);
+}
+
 int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
@@ -3040,12 +3045,12 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
 	int ret;
 
 	intel_ring_default_vfuncs(dev_priv, engine);
+	intel_ring_default_irqs(engine);
 
 	if (INTEL_GEN(dev_priv) >= 8) {
 		engine->init_context = intel_rcs_ctx_init;
 		engine->add_request = gen8_render_add_request;
 		engine->flush = gen8_render_ring_flush;
-		engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
 		if (i915_semaphore_is_enabled(dev_priv))
 			engine->semaphore.signal = gen8_rcs_signal;
 	} else if (INTEL_GEN(dev_priv) >= 6) {
@@ -3053,14 +3058,12 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
 		engine->flush = gen7_render_ring_flush;
 		if (IS_GEN6(dev_priv))
 			engine->flush = gen6_render_ring_flush;
-		engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
 	} else if (IS_GEN5(dev_priv)) {
 		engine->add_request = pc_render_add_request;
 		engine->flush = gen4_render_ring_flush;
 		engine->get_seqno = pc_render_get_seqno;
 		engine->set_seqno = pc_render_set_seqno;
-		engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT |
-					GT_RENDER_PIPECTL_NOTIFY_INTERRUPT;
+		engine->irq_enable_mask |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT;
 	} else {
 		if (INTEL_GEN(dev_priv) < 4)
 			engine->flush = gen2_render_ring_flush;
@@ -3112,16 +3115,14 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
 	struct drm_i915_private *dev_priv = engine->i915;
 
 	intel_ring_default_vfuncs(dev_priv, engine);
+	intel_ring_default_irqs(engine);
 
 	if (INTEL_GEN(dev_priv) >= 6) {
 		/* gen6 bsd needs a special wa for tail updates */
 		if (IS_GEN6(dev_priv))
 			engine->write_tail = gen6_bsd_ring_write_tail;
 		engine->flush = gen6_bsd_ring_flush;
-		if (INTEL_GEN(dev_priv) >= 8)
-			engine->irq_enable_mask =
-				GT_RENDER_USER_INTERRUPT << GEN8_VCS1_IRQ_SHIFT;
-		else
+		if (INTEL_GEN(dev_priv) < 8)
 			engine->irq_enable_mask = GT_BSD_USER_INTERRUPT;
 	} else {
 		engine->mmio_base = BSD_RING_BASE;
@@ -3143,10 +3144,9 @@ 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);
+	intel_ring_default_irqs(engine);
 
 	engine->flush = gen6_bsd_ring_flush;
-	engine->irq_enable_mask =
-			GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT;
 
 	return intel_init_ring_buffer(dev_priv->dev, engine);
 }
@@ -3156,12 +3156,10 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
 	struct drm_i915_private *dev_priv = engine->i915;
 
 	intel_ring_default_vfuncs(dev_priv, engine);
+	intel_ring_default_irqs(engine);
 
 	engine->flush = gen6_ring_flush;
-	if (INTEL_GEN(dev_priv) >= 8)
-		engine->irq_enable_mask =
-			GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT;
-	else
+	if (INTEL_GEN(dev_priv) < 8)
 		engine->irq_enable_mask = GT_BLT_USER_INTERRUPT;
 
 	return intel_init_ring_buffer(dev_priv->dev, engine);
@@ -3172,13 +3170,11 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
 	struct drm_i915_private *dev_priv = engine->i915;
 
 	intel_ring_default_vfuncs(dev_priv, engine);
+	intel_ring_default_irqs(engine);
 
 	engine->flush = gen6_ring_flush;
 
-	if (INTEL_GEN(dev_priv) >= 8) {
-		engine->irq_enable_mask =
-			GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT;
-	} else {
+	if (INTEL_GEN(dev_priv) < 8) {
 		engine->irq_enable_mask = PM_VEBOX_USER_INTERRUPT;
 		engine->irq_get = hsw_vebox_get_irq;
 		engine->irq_put = hsw_vebox_put_irq;
-- 
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] 14+ messages in thread
* [PATCH 5/5] drm/i915: Simplify intel_init_ring_buffer prototype
  2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
                   ` (2 preceding siblings ...)
  2016-07-01 16:47 ` [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup Tvrtko Ursulin
@ 2016-07-01 16:47 ` Tvrtko Ursulin
  2016-07-13 12:30   ` Daniel Vetter
  2016-07-01 17:08 ` ✗ Ro.CI.BAT: warning for series starting with [1/5] drm/i915: unify first-stage engine struct setup Patchwork
  2016-07-13 12:23 ` [PATCH 1/5] " Daniel Vetter
  5 siblings, 1 reply; 14+ messages in thread
From: Tvrtko Ursulin @ 2016-07-01 16:47 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Engine contains dev_priv so need to pass it in.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 964776bb181c..760c6b53f063 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2376,21 +2376,19 @@ static void intel_ring_context_unpin(struct i915_gem_context *ctx,
 	i915_gem_context_unreference(ctx);
 }
 
-static int intel_init_ring_buffer(struct drm_device *dev,
-				  struct intel_engine_cs *engine)
+static int intel_init_ring_buffer(struct intel_engine_cs *engine)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_i915_private *dev_priv = engine->i915;
 	struct intel_ringbuffer *ringbuf;
 	int ret;
 
 	WARN_ON(engine->buffer);
 
-	engine->i915 = dev_priv;
 	INIT_LIST_HEAD(&engine->active_list);
 	INIT_LIST_HEAD(&engine->request_list);
 	INIT_LIST_HEAD(&engine->execlist_queue);
 	INIT_LIST_HEAD(&engine->buffers);
-	i915_gem_batch_pool_init(dev, &engine->batch_pool);
+	i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool);
 	memset(engine->semaphore.sync_seqno, 0,
 	       sizeof(engine->semaphore.sync_seqno));
 
@@ -3097,7 +3095,7 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
 		engine->scratch.gtt_offset = i915_gem_obj_ggtt_offset(obj);
 	}
 
-	ret = intel_init_ring_buffer(dev_priv->dev, engine);
+	ret = intel_init_ring_buffer(engine);
 	if (ret)
 		return ret;
 
@@ -3133,7 +3131,7 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
 			engine->irq_enable_mask = I915_BSD_USER_INTERRUPT;
 	}
 
-	return intel_init_ring_buffer(dev_priv->dev, engine);
+	return intel_init_ring_buffer(engine);
 }
 
 /**
@@ -3148,7 +3146,7 @@ int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
 
 	engine->flush = gen6_bsd_ring_flush;
 
-	return intel_init_ring_buffer(dev_priv->dev, engine);
+	return intel_init_ring_buffer(engine);
 }
 
 int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
@@ -3162,7 +3160,7 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
 	if (INTEL_GEN(dev_priv) < 8)
 		engine->irq_enable_mask = GT_BLT_USER_INTERRUPT;
 
-	return intel_init_ring_buffer(dev_priv->dev, engine);
+	return intel_init_ring_buffer(engine);
 }
 
 int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
@@ -3180,7 +3178,7 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
 		engine->irq_put = hsw_vebox_put_irq;
 	}
 
-	return intel_init_ring_buffer(dev_priv->dev, engine);
+	return intel_init_ring_buffer(engine);
 }
 
 int
-- 
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] 14+ messages in thread
* ✗ Ro.CI.BAT: warning for series starting with [1/5] drm/i915: unify first-stage engine struct setup
  2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
                   ` (3 preceding siblings ...)
  2016-07-01 16:47 ` [PATCH 5/5] drm/i915: Simplify intel_init_ring_buffer prototype Tvrtko Ursulin
@ 2016-07-01 17:08 ` Patchwork
  2016-07-13 12:23 ` [PATCH 1/5] " Daniel Vetter
  5 siblings, 0 replies; 14+ messages in thread
From: Patchwork @ 2016-07-01 17:08 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/5] drm/i915: unify first-stage engine struct setup
URL   : https://patchwork.freedesktop.org/series/9391/
State : warning

== Summary ==

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

Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-c:
                pass       -> SKIP       (fi-skl-i5-6260u)
        Subgroup suspend-read-crc-pipe-a:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
                skip       -> PASS       (fi-skl-i5-6260u)

fi-hsw-i7-4770k  total:229  pass:196  dwarn:0   dfail:0   fail:0   skip:33 
fi-kbl-qkkr      total:229  pass:160  dwarn:28  dfail:1   fail:0   skip:40 
fi-skl-i5-6260u  total:229  pass:203  dwarn:0   dfail:0   fail:0   skip:26 
fi-skl-i7-6700k  total:229  pass:190  dwarn:0   dfail:0   fail:0   skip:39 
ro-bdw-i5-5250u  total:229  pass:204  dwarn:3   dfail:1   fail:0   skip:21 
ro-bdw-i7-5557U  total:229  pass:204  dwarn:1   dfail:1   fail:0   skip:23 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:176  dwarn:0   dfail:1   fail:3   skip:49 
ro-byt-n2820     total:229  pass:180  dwarn:0   dfail:1   fail:3   skip:45 
ro-hsw-i3-4010u  total:229  pass:197  dwarn:0   dfail:1   fail:0   skip:31 
ro-hsw-i7-4770r  total:229  pass:197  dwarn:0   dfail:1   fail:0   skip:31 
ro-ilk-i7-620lm  total:229  pass:157  dwarn:0   dfail:1   fail:1   skip:70 
ro-ilk1-i5-650   total:224  pass:157  dwarn:0   dfail:1   fail:1   skip:65 
ro-ivb-i7-3770   total:229  pass:188  dwarn:0   dfail:1   fail:0   skip:40 
ro-skl3-i5-6260u total:229  pass:208  dwarn:1   dfail:1   fail:0   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1368/

a755d6c drm-intel-nightly: 2016y-07m-01d-13h-54m-24s UTC integration manifest
b43d558 drm/i915: Simplify intel_init_ring_buffer prototype
845bf70 drm/i915: Make more use of the shared engine irq setup
d9744b9 drm/i915: Unify engine init loop
2df5cf5 drm/i915: Prepare for engine init unification
b8131fa drm/i915: unify first-stage engine struct setup

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] drm/i915: unify first-stage engine struct setup
  2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
                   ` (4 preceding siblings ...)
  2016-07-01 17:08 ` ✗ Ro.CI.BAT: warning for series starting with [1/5] drm/i915: unify first-stage engine struct setup Patchwork
@ 2016-07-13 12:23 ` Daniel Vetter
  2016-07-13 13:16   ` Tvrtko Ursulin
  5 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2016-07-13 12:23 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel-gfx

On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
> From: Dave Gordon <david.s.gordon@intel.com>
> 
> intel_lrc.c has a table of "logical rings" (meaning engines), while
> intel_ringbuffer.c has separately open-coded initialisation for each
> engine. We can deduplicate this somewhat by using the same first-stage
> engine-setup function for both modes.
> 
> So here we expose the function that transfers information from the
> static table of (all) known engines to the dev_priv->engine array of
> engines available on this device (adjusting the names along the way)
> and then embed calls to it in both the LRC and the legacy-mode setup.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c        | 40 +++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 40 +++++++++------------------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  5 +++++
>  3 files changed, 41 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 339d8041075f..ed017f1a07a2 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1994,8 +1994,9 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
>  }
>  
>  static inline void
> -logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
> +logical_ring_default_irqs(struct intel_engine_cs *engine)
>  {
> +	unsigned shift = engine->irq_shift;
>  	engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
>  	engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
>  	init_waitqueue_head(&engine->irq_queue);
> @@ -2096,14 +2097,14 @@ static int logical_render_ring_init(struct intel_engine_cs *engine)
>  	return ret;
>  }
>  
> -static const struct logical_ring_info {
> +static const struct engine_info {
>  	const char *name;
>  	unsigned exec_id;
>  	unsigned guc_id;
>  	u32 mmio_base;
>  	unsigned irq_shift;
>  	int (*init)(struct intel_engine_cs *engine);
> -} logical_rings[] = {
> +} intel_engines[] = {
>  	[RCS] = {
>  		.name = "render ring",
>  		.exec_id = I915_EXEC_RENDER,
> @@ -2146,20 +2147,31 @@ static const struct logical_ring_info {
>  	},
>  };
>  
> -static struct intel_engine_cs *
> -logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
> +struct intel_engine_cs *
> +intel_engine_setup(struct drm_i915_private *dev_priv,
> +		   enum intel_engine_id id)

Kerneldoc for this would be nice. Also, we now have a mess between
intel_lrc.c and intel_ringbuffer.c. Extracting intel_engine.c with the
shared bits or something similar, plus cleanup up all the docs would be
awesome as a follow up.

With the kerneldoc added:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  {
> -	const struct logical_ring_info *info = &logical_rings[id];
> +	const struct engine_info *info = &intel_engines[id];
>  	struct intel_engine_cs *engine = &dev_priv->engine[id];
> -	enum forcewake_domains fw_domains;
>  
>  	engine->id = id;
> +	engine->i915 = dev_priv;
>  	engine->name = info->name;
>  	engine->exec_id = info->exec_id;
> -	engine->guc_id = info->guc_id;
> +	engine->hw_id = engine->guc_id = info->guc_id;

Optional bikeshed: s/info->guc_id/info->hw_id/ makes sense imo in the new
context. Or nuking engine->guc_id.

>  	engine->mmio_base = info->mmio_base;
> +	engine->irq_shift = info->irq_shift;
>  
> -	engine->i915 = dev_priv;
> +	return engine;
> +}
> +
> +static struct intel_engine_cs *
> +logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
> +{
> +	struct intel_engine_cs *engine;
> +	enum forcewake_domains fw_domains;
> +
> +	engine = intel_engine_setup(dev_priv, id);
>  
>  	/* Intentionally left blank. */
>  	engine->buffer = NULL;
> @@ -2189,7 +2201,7 @@ logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
>  
>  	logical_ring_init_platform_invariants(engine);
>  	logical_ring_default_vfuncs(engine);
> -	logical_ring_default_irqs(engine, info->irq_shift);
> +	logical_ring_default_irqs(engine);
>  
>  	intel_engine_init_hangcheck(engine);
>  	i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool);
> @@ -2218,14 +2230,14 @@ int intel_logical_rings_init(struct drm_device *dev)
>  	WARN_ON(INTEL_INFO(dev_priv)->ring_mask &
>  		GENMASK(sizeof(mask) * BITS_PER_BYTE - 1, I915_NUM_ENGINES));
>  
> -	for (i = 0; i < ARRAY_SIZE(logical_rings); i++) {
> +	for (i = 0; i < ARRAY_SIZE(intel_engines); i++) {
>  		if (!HAS_ENGINE(dev_priv, i))
>  			continue;
>  
> -		if (!logical_rings[i].init)
> +		if (!intel_engines[i].init)
>  			continue;
>  
> -		ret = logical_rings[i].init(logical_ring_setup(dev_priv, i));
> +		ret = intel_engines[i].init(logical_ring_setup(dev_priv, i));
>  		if (ret)
>  			goto cleanup;
>  
> @@ -2233,7 +2245,7 @@ int intel_logical_rings_init(struct drm_device *dev)
>  	}
>  
>  	/*
> -	 * Catch failures to update logical_rings table when the new engines
> +	 * Catch failures to update intel_engines table when the new engines
>  	 * are added to the driver by a warning and disabling the forgotten
>  	 * engines.
>  	 */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 24cdc920f4b4..215424efe05c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -3036,15 +3036,11 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
>  int intel_init_render_ring_buffer(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_engine_cs *engine = &dev_priv->engine[RCS];
> +	struct intel_engine_cs *engine;
>  	struct drm_i915_gem_object *obj;
>  	int ret;
>  
> -	engine->name = "render ring";
> -	engine->id = RCS;
> -	engine->exec_id = I915_EXEC_RENDER;
> -	engine->hw_id = 0;
> -	engine->mmio_base = RENDER_RING_BASE;
> +	engine = intel_engine_setup(dev_priv, RCS);
>  
>  	intel_ring_default_vfuncs(dev_priv, engine);
>  
> @@ -3117,17 +3113,13 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>  int intel_init_bsd_ring_buffer(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_engine_cs *engine = &dev_priv->engine[VCS];
> +	struct intel_engine_cs *engine;
>  
> -	engine->name = "bsd ring";
> -	engine->id = VCS;
> -	engine->exec_id = I915_EXEC_BSD;
> -	engine->hw_id = 1;
> +	engine = intel_engine_setup(dev_priv, VCS);
>  
>  	intel_ring_default_vfuncs(dev_priv, engine);
>  
>  	if (INTEL_GEN(dev_priv) >= 6) {
> -		engine->mmio_base = GEN6_BSD_RING_BASE;
>  		/* gen6 bsd needs a special wa for tail updates */
>  		if (IS_GEN6(dev_priv))
>  			engine->write_tail = gen6_bsd_ring_write_tail;
> @@ -3155,13 +3147,9 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
>  int intel_init_bsd2_ring_buffer(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_engine_cs *engine = &dev_priv->engine[VCS2];
> +	struct intel_engine_cs *engine;
>  
> -	engine->name = "bsd2 ring";
> -	engine->id = VCS2;
> -	engine->exec_id = I915_EXEC_BSD;
> -	engine->hw_id = 4;
> -	engine->mmio_base = GEN8_BSD2_RING_BASE;
> +	engine = intel_engine_setup(dev_priv, VCS2);
>  
>  	intel_ring_default_vfuncs(dev_priv, engine);
>  
> @@ -3175,13 +3163,9 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
>  int intel_init_blt_ring_buffer(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_engine_cs *engine = &dev_priv->engine[BCS];
> +	struct intel_engine_cs *engine;
>  
> -	engine->name = "blitter ring";
> -	engine->id = BCS;
> -	engine->exec_id = I915_EXEC_BLT;
> -	engine->hw_id = 2;
> -	engine->mmio_base = BLT_RING_BASE;
> +	engine = intel_engine_setup(dev_priv, BCS);
>  
>  	intel_ring_default_vfuncs(dev_priv, engine);
>  
> @@ -3198,13 +3182,9 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
>  int intel_init_vebox_ring_buffer(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_engine_cs *engine = &dev_priv->engine[VECS];
> +	struct intel_engine_cs *engine;
>  
> -	engine->name = "video enhancement ring";
> -	engine->id = VECS;
> -	engine->exec_id = I915_EXEC_VEBOX;
> -	engine->hw_id = 3;
> -	engine->mmio_base = VEBOX_RING_BASE;
> +	engine = intel_engine_setup(dev_priv, VECS);
>  
>  	intel_ring_default_vfuncs(dev_priv, engine);
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 113d5230a6de..1aeb00cba9e2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -145,6 +145,7 @@ struct intel_engine_cs {
>  	unsigned int hw_id;
>  	unsigned int guc_id; /* XXX same as hw_id? */
>  	u32		mmio_base;
> +	unsigned int irq_shift;
>  	struct intel_ringbuffer *buffer;
>  	struct list_head buffers;
>  
> @@ -335,6 +336,10 @@ struct intel_engine_cs {
>  	u32 (*get_cmd_length_mask)(u32 cmd_header);
>  };
>  
> +struct intel_engine_cs *
> +intel_engine_setup(struct drm_i915_private *dev_priv,
> +		   enum intel_engine_id id);
> +
>  static inline bool
>  intel_engine_initialized(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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup
  2016-07-01 16:47 ` [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup Tvrtko Ursulin
@ 2016-07-13 12:30   ` Daniel Vetter
  2016-07-13 13:19     ` Tvrtko Ursulin
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Vetter @ 2016-07-13 12:30 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel-gfx

On Fri, Jul 01, 2016 at 05:47:14PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Use it for legacy engine initialization by adding a
> intel_ring_default_irqs helper used by individual engines.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Not sure this is worth it. irq handling is fairly gen specific, and the
overlap between lrc and legacy rings is just gen8. For preceeding two
patches 2&3:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 36 +++++++++++++++------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index a00adc3438f3..964776bb181c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2394,8 +2394,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  	memset(engine->semaphore.sync_seqno, 0,
>  	       sizeof(engine->semaphore.sync_seqno));
>  
> -	init_waitqueue_head(&engine->irq_queue);
> -
>  	/* We may need to do things with the shrinker which
>  	 * require us to immediately switch back to the default
>  	 * context. This can cause a problem as pinning the
> @@ -3033,6 +3031,13 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
>  	intel_ring_init_semaphores(dev_priv, engine);
>  }
>  
> +static void
> +intel_ring_default_irqs(struct intel_engine_cs *engine)
> +{
> +	engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << engine->irq_shift;
> +	init_waitqueue_head(&engine->irq_queue);
> +}
> +
>  int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv = engine->i915;
> @@ -3040,12 +3045,12 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
>  	int ret;
>  
>  	intel_ring_default_vfuncs(dev_priv, engine);
> +	intel_ring_default_irqs(engine);
>  
>  	if (INTEL_GEN(dev_priv) >= 8) {
>  		engine->init_context = intel_rcs_ctx_init;
>  		engine->add_request = gen8_render_add_request;
>  		engine->flush = gen8_render_ring_flush;
> -		engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
>  		if (i915_semaphore_is_enabled(dev_priv))
>  			engine->semaphore.signal = gen8_rcs_signal;
>  	} else if (INTEL_GEN(dev_priv) >= 6) {
> @@ -3053,14 +3058,12 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
>  		engine->flush = gen7_render_ring_flush;
>  		if (IS_GEN6(dev_priv))
>  			engine->flush = gen6_render_ring_flush;
> -		engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
>  	} else if (IS_GEN5(dev_priv)) {
>  		engine->add_request = pc_render_add_request;
>  		engine->flush = gen4_render_ring_flush;
>  		engine->get_seqno = pc_render_get_seqno;
>  		engine->set_seqno = pc_render_set_seqno;
> -		engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT |
> -					GT_RENDER_PIPECTL_NOTIFY_INTERRUPT;
> +		engine->irq_enable_mask |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT;
>  	} else {
>  		if (INTEL_GEN(dev_priv) < 4)
>  			engine->flush = gen2_render_ring_flush;
> @@ -3112,16 +3115,14 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
>  	struct drm_i915_private *dev_priv = engine->i915;
>  
>  	intel_ring_default_vfuncs(dev_priv, engine);
> +	intel_ring_default_irqs(engine);
>  
>  	if (INTEL_GEN(dev_priv) >= 6) {
>  		/* gen6 bsd needs a special wa for tail updates */
>  		if (IS_GEN6(dev_priv))
>  			engine->write_tail = gen6_bsd_ring_write_tail;
>  		engine->flush = gen6_bsd_ring_flush;
> -		if (INTEL_GEN(dev_priv) >= 8)
> -			engine->irq_enable_mask =
> -				GT_RENDER_USER_INTERRUPT << GEN8_VCS1_IRQ_SHIFT;
> -		else
> +		if (INTEL_GEN(dev_priv) < 8)
>  			engine->irq_enable_mask = GT_BSD_USER_INTERRUPT;
>  	} else {
>  		engine->mmio_base = BSD_RING_BASE;
> @@ -3143,10 +3144,9 @@ 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);
> +	intel_ring_default_irqs(engine);
>  
>  	engine->flush = gen6_bsd_ring_flush;
> -	engine->irq_enable_mask =
> -			GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT;
>  
>  	return intel_init_ring_buffer(dev_priv->dev, engine);
>  }
> @@ -3156,12 +3156,10 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
>  	struct drm_i915_private *dev_priv = engine->i915;
>  
>  	intel_ring_default_vfuncs(dev_priv, engine);
> +	intel_ring_default_irqs(engine);
>  
>  	engine->flush = gen6_ring_flush;
> -	if (INTEL_GEN(dev_priv) >= 8)
> -		engine->irq_enable_mask =
> -			GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT;
> -	else
> +	if (INTEL_GEN(dev_priv) < 8)
>  		engine->irq_enable_mask = GT_BLT_USER_INTERRUPT;
>  
>  	return intel_init_ring_buffer(dev_priv->dev, engine);
> @@ -3172,13 +3170,11 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
>  	struct drm_i915_private *dev_priv = engine->i915;
>  
>  	intel_ring_default_vfuncs(dev_priv, engine);
> +	intel_ring_default_irqs(engine);
>  
>  	engine->flush = gen6_ring_flush;
>  
> -	if (INTEL_GEN(dev_priv) >= 8) {
> -		engine->irq_enable_mask =
> -			GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT;
> -	} else {
> +	if (INTEL_GEN(dev_priv) < 8) {
>  		engine->irq_enable_mask = PM_VEBOX_USER_INTERRUPT;
>  		engine->irq_get = hsw_vebox_get_irq;
>  		engine->irq_put = hsw_vebox_put_irq;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] drm/i915: Simplify intel_init_ring_buffer prototype
  2016-07-01 16:47 ` [PATCH 5/5] drm/i915: Simplify intel_init_ring_buffer prototype Tvrtko Ursulin
@ 2016-07-13 12:30   ` Daniel Vetter
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2016-07-13 12:30 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel-gfx

On Fri, Jul 01, 2016 at 05:47:15PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Engine contains dev_priv so need to pass it in.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 964776bb181c..760c6b53f063 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2376,21 +2376,19 @@ static void intel_ring_context_unpin(struct i915_gem_context *ctx,
>  	i915_gem_context_unreference(ctx);
>  }
>  
> -static int intel_init_ring_buffer(struct drm_device *dev,
> -				  struct intel_engine_cs *engine)
> +static int intel_init_ring_buffer(struct intel_engine_cs *engine)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_i915_private *dev_priv = engine->i915;
>  	struct intel_ringbuffer *ringbuf;
>  	int ret;
>  
>  	WARN_ON(engine->buffer);
>  
> -	engine->i915 = dev_priv;
>  	INIT_LIST_HEAD(&engine->active_list);
>  	INIT_LIST_HEAD(&engine->request_list);
>  	INIT_LIST_HEAD(&engine->execlist_queue);
>  	INIT_LIST_HEAD(&engine->buffers);
> -	i915_gem_batch_pool_init(dev, &engine->batch_pool);
> +	i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool);
>  	memset(engine->semaphore.sync_seqno, 0,
>  	       sizeof(engine->semaphore.sync_seqno));
>  
> @@ -3097,7 +3095,7 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
>  		engine->scratch.gtt_offset = i915_gem_obj_ggtt_offset(obj);
>  	}
>  
> -	ret = intel_init_ring_buffer(dev_priv->dev, engine);
> +	ret = intel_init_ring_buffer(engine);
>  	if (ret)
>  		return ret;
>  
> @@ -3133,7 +3131,7 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
>  			engine->irq_enable_mask = I915_BSD_USER_INTERRUPT;
>  	}
>  
> -	return intel_init_ring_buffer(dev_priv->dev, engine);
> +	return intel_init_ring_buffer(engine);
>  }
>  
>  /**
> @@ -3148,7 +3146,7 @@ int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
>  
>  	engine->flush = gen6_bsd_ring_flush;
>  
> -	return intel_init_ring_buffer(dev_priv->dev, engine);
> +	return intel_init_ring_buffer(engine);
>  }
>  
>  int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
> @@ -3162,7 +3160,7 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
>  	if (INTEL_GEN(dev_priv) < 8)
>  		engine->irq_enable_mask = GT_BLT_USER_INTERRUPT;
>  
> -	return intel_init_ring_buffer(dev_priv->dev, engine);
> +	return intel_init_ring_buffer(engine);
>  }
>  
>  int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
> @@ -3180,7 +3178,7 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
>  		engine->irq_put = hsw_vebox_put_irq;
>  	}
>  
> -	return intel_init_ring_buffer(dev_priv->dev, engine);
> +	return intel_init_ring_buffer(engine);
>  }
>  
>  int
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] drm/i915: unify first-stage engine struct setup
  2016-07-13 12:23 ` [PATCH 1/5] " Daniel Vetter
@ 2016-07-13 13:16   ` Tvrtko Ursulin
  2016-07-13 13:24     ` Tvrtko Ursulin
  2016-07-14 11:33     ` Dave Gordon
  0 siblings, 2 replies; 14+ messages in thread
From: Tvrtko Ursulin @ 2016-07-13 13:16 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Intel-gfx


On 13/07/16 13:23, Daniel Vetter wrote:
> On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
>> From: Dave Gordon <david.s.gordon@intel.com>
>>
>> intel_lrc.c has a table of "logical rings" (meaning engines), while
>> intel_ringbuffer.c has separately open-coded initialisation for each
>> engine. We can deduplicate this somewhat by using the same first-stage
>> engine-setup function for both modes.
>>
>> So here we expose the function that transfers information from the
>> static table of (all) known engines to the dev_priv->engine array of
>> engines available on this device (adjusting the names along the way)
>> and then embed calls to it in both the LRC and the legacy-mode setup.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_lrc.c        | 40 +++++++++++++++++++++------------
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 40 +++++++++------------------------
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  5 +++++
>>   3 files changed, 41 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 339d8041075f..ed017f1a07a2 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1994,8 +1994,9 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
>>   }
>>
>>   static inline void
>> -logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
>> +logical_ring_default_irqs(struct intel_engine_cs *engine)
>>   {
>> +	unsigned shift = engine->irq_shift;
>>   	engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
>>   	engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
>>   	init_waitqueue_head(&engine->irq_queue);
>> @@ -2096,14 +2097,14 @@ static int logical_render_ring_init(struct intel_engine_cs *engine)
>>   	return ret;
>>   }
>>
>> -static const struct logical_ring_info {
>> +static const struct engine_info {
>>   	const char *name;
>>   	unsigned exec_id;
>>   	unsigned guc_id;
>>   	u32 mmio_base;
>>   	unsigned irq_shift;
>>   	int (*init)(struct intel_engine_cs *engine);
>> -} logical_rings[] = {
>> +} intel_engines[] = {
>>   	[RCS] = {
>>   		.name = "render ring",
>>   		.exec_id = I915_EXEC_RENDER,
>> @@ -2146,20 +2147,31 @@ static const struct logical_ring_info {
>>   	},
>>   };
>>
>> -static struct intel_engine_cs *
>> -logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
>> +struct intel_engine_cs *
>> +intel_engine_setup(struct drm_i915_private *dev_priv,
>> +		   enum intel_engine_id id)
>
> Kerneldoc for this would be nice. Also, we now have a mess between
> intel_lrc.c and intel_ringbuffer.c. Extracting intel_engine.c with the

Yes that was already suggested by Chris and I am trybotting the 
additions today, again on his explicit request to progress this series.

> shared bits or something similar, plus cleanup up all the docs would be
> awesome as a follow up.

What do you mean "all the docs"? :D

>
> With the kerneldoc added:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks, will add.

>>   {
>> -	const struct logical_ring_info *info = &logical_rings[id];
>> +	const struct engine_info *info = &intel_engines[id];
>>   	struct intel_engine_cs *engine = &dev_priv->engine[id];
>> -	enum forcewake_domains fw_domains;
>>
>>   	engine->id = id;
>> +	engine->i915 = dev_priv;
>>   	engine->name = info->name;
>>   	engine->exec_id = info->exec_id;
>> -	engine->guc_id = info->guc_id;
>> +	engine->hw_id = engine->guc_id = info->guc_id;
>
> Optional bikeshed: s/info->guc_id/info->hw_id/ makes sense imo in the new
> context. Or nuking engine->guc_id.

Someone said we cannot be sure they will be the same in the future. So 
maybe just rename to hw_id for now.

Regards,

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup
  2016-07-13 12:30   ` Daniel Vetter
@ 2016-07-13 13:19     ` Tvrtko Ursulin
  0 siblings, 0 replies; 14+ messages in thread
From: Tvrtko Ursulin @ 2016-07-13 13:19 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Intel-gfx


On 13/07/16 13:30, Daniel Vetter wrote:
> On Fri, Jul 01, 2016 at 05:47:14PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Use it for legacy engine initialization by adding a
>> intel_ring_default_irqs helper used by individual engines.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Not sure this is worth it. irq handling is fairly gen specific, and the
> overlap between lrc and legacy rings is just gen8. For preceeding two

In the latest rebase it is:

  1 file changed, 5 insertions(+), 15 deletions(-)

So it does manage to use the shared static state significantly. So on 
the basis of that vs the rebase work dropping this one would cause, 
maybe change your mind? :)

> patches 2&3:

> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks!

Regards,

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] drm/i915: unify first-stage engine struct setup
  2016-07-13 13:16   ` Tvrtko Ursulin
@ 2016-07-13 13:24     ` Tvrtko Ursulin
  2016-07-14 14:29       ` Daniel Vetter
  2016-07-14 11:33     ` Dave Gordon
  1 sibling, 1 reply; 14+ messages in thread
From: Tvrtko Ursulin @ 2016-07-13 13:24 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Intel-gfx


On 13/07/16 14:16, Tvrtko Ursulin wrote:
>
> On 13/07/16 13:23, Daniel Vetter wrote:
>> On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
>>> From: Dave Gordon <david.s.gordon@intel.com>
>>>
>>> intel_lrc.c has a table of "logical rings" (meaning engines), while
>>> intel_ringbuffer.c has separately open-coded initialisation for each
>>> engine. We can deduplicate this somewhat by using the same first-stage
>>> engine-setup function for both modes.
>>>
>>> So here we expose the function that transfers information from the
>>> static table of (all) known engines to the dev_priv->engine array of
>>> engines available on this device (adjusting the names along the way)
>>> and then embed calls to it in both the LRC and the legacy-mode setup.
>>>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_lrc.c        | 40
>>> +++++++++++++++++++++------------
>>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 40
>>> +++++++++------------------------
>>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  5 +++++
>>>   3 files changed, 41 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
>>> b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 339d8041075f..ed017f1a07a2 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -1994,8 +1994,9 @@ logical_ring_default_vfuncs(struct
>>> intel_engine_cs *engine)
>>>   }
>>>
>>>   static inline void
>>> -logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned
>>> shift)
>>> +logical_ring_default_irqs(struct intel_engine_cs *engine)
>>>   {
>>> +    unsigned shift = engine->irq_shift;
>>>       engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
>>>       engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
>>>       init_waitqueue_head(&engine->irq_queue);
>>> @@ -2096,14 +2097,14 @@ static int logical_render_ring_init(struct
>>> intel_engine_cs *engine)
>>>       return ret;
>>>   }
>>>
>>> -static const struct logical_ring_info {
>>> +static const struct engine_info {
>>>       const char *name;
>>>       unsigned exec_id;
>>>       unsigned guc_id;
>>>       u32 mmio_base;
>>>       unsigned irq_shift;
>>>       int (*init)(struct intel_engine_cs *engine);
>>> -} logical_rings[] = {
>>> +} intel_engines[] = {
>>>       [RCS] = {
>>>           .name = "render ring",
>>>           .exec_id = I915_EXEC_RENDER,
>>> @@ -2146,20 +2147,31 @@ static const struct logical_ring_info {
>>>       },
>>>   };
>>>
>>> -static struct intel_engine_cs *
>>> -logical_ring_setup(struct drm_i915_private *dev_priv, enum
>>> intel_engine_id id)
>>> +struct intel_engine_cs *
>>> +intel_engine_setup(struct drm_i915_private *dev_priv,
>>> +           enum intel_engine_id id)
>>
>> Kerneldoc for this would be nice. Also, we now have a mess between
>> intel_lrc.c and intel_ringbuffer.c. Extracting intel_engine.c with the
>
> Yes that was already suggested by Chris and I am trybotting the
> additions today, again on his explicit request to progress this series.
>
>> shared bits or something similar, plus cleanup up all the docs would be
>> awesome as a follow up.
>
> What do you mean "all the docs"? :D
>
>>
>> With the kerneldoc added:
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Thanks, will add.

Actually the function becomes static later in the series, when the 
common stuff is moved into intel_engine_cs.c.

Can I keep the r-b without adding kernel doc for it then?

Regards,

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] drm/i915: unify first-stage engine struct setup
  2016-07-13 13:16   ` Tvrtko Ursulin
  2016-07-13 13:24     ` Tvrtko Ursulin
@ 2016-07-14 11:33     ` Dave Gordon
  1 sibling, 0 replies; 14+ messages in thread
From: Dave Gordon @ 2016-07-14 11:33 UTC (permalink / raw)
  To: intel-gfx

On 13/07/16 14:16, Tvrtko Ursulin wrote:
>
> On 13/07/16 13:23, Daniel Vetter wrote:
>> On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
>>> From: Dave Gordon <david.s.gordon@intel.com>
>>>
[snip]
>>>   {
>>> -    const struct logical_ring_info *info = &logical_rings[id];
>>> +    const struct engine_info *info = &intel_engines[id];
>>>       struct intel_engine_cs *engine = &dev_priv->engine[id];
>>> -    enum forcewake_domains fw_domains;
>>>
>>>       engine->id = id;
>>> +    engine->i915 = dev_priv;
>>>       engine->name = info->name;
>>>       engine->exec_id = info->exec_id;
>>> -    engine->guc_id = info->guc_id;
>>> +    engine->hw_id = engine->guc_id = info->guc_id;
>>
>> Optional bikeshed: s/info->guc_id/info->hw_id/ makes sense imo in the new
>> context. Or nuking engine->guc_id.
>
> Someone said we cannot be sure they will be the same in the future. So
> maybe just rename to hw_id for now.
>
> Regards,
> Tvrtko

The GuC firmware *could* use a completely different enumeration of the 
engines, but why would it? Since it's closely tied to the hardware, it 
*ought* to use the same naming scheme as the h/w. So I have no objection 
to getting rid of guc_id entirely, and changing existing uses to use 
hw_id instead.

OTOH it seems little benefit and would certainly involve more work to 
reverse. The firmware team might, after all, decide that they too want 
to decouple the logical-engine-numbers used for the KMD interface from 
whatever the hardware team decide is the best way to number engines -- 
which might after all change between generations or even different SKUs 
of the same device!

SO, I think the "best" version of that line is probably:

+	engine->hw_id = info->hw_id;
+
+	/* Current GuC f/w uses hw_id not driver id */
+	engine->guc_id = info->hw_id;

and we'll add "info->guc_id" back again if it ever becomes necessary.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] drm/i915: unify first-stage engine struct setup
  2016-07-13 13:24     ` Tvrtko Ursulin
@ 2016-07-14 14:29       ` Daniel Vetter
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2016-07-14 14:29 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: Intel-gfx

On Wed, Jul 13, 2016 at 02:24:06PM +0100, Tvrtko Ursulin wrote:
> 
> On 13/07/16 14:16, Tvrtko Ursulin wrote:
> > 
> > On 13/07/16 13:23, Daniel Vetter wrote:
> > > On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
> > > > From: Dave Gordon <david.s.gordon@intel.com>
> > > > 
> > > > intel_lrc.c has a table of "logical rings" (meaning engines), while
> > > > intel_ringbuffer.c has separately open-coded initialisation for each
> > > > engine. We can deduplicate this somewhat by using the same first-stage
> > > > engine-setup function for both modes.
> > > > 
> > > > So here we expose the function that transfers information from the
> > > > static table of (all) known engines to the dev_priv->engine array of
> > > > engines available on this device (adjusting the names along the way)
> > > > and then embed calls to it in both the LRC and the legacy-mode setup.
> > > > 
> > > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_lrc.c        | 40
> > > > +++++++++++++++++++++------------
> > > >   drivers/gpu/drm/i915/intel_ringbuffer.c | 40
> > > > +++++++++------------------------
> > > >   drivers/gpu/drm/i915/intel_ringbuffer.h |  5 +++++
> > > >   3 files changed, 41 insertions(+), 44 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > > > b/drivers/gpu/drm/i915/intel_lrc.c
> > > > index 339d8041075f..ed017f1a07a2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > > @@ -1994,8 +1994,9 @@ logical_ring_default_vfuncs(struct
> > > > intel_engine_cs *engine)
> > > >   }
> > > > 
> > > >   static inline void
> > > > -logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned
> > > > shift)
> > > > +logical_ring_default_irqs(struct intel_engine_cs *engine)
> > > >   {
> > > > +    unsigned shift = engine->irq_shift;
> > > >       engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
> > > >       engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
> > > >       init_waitqueue_head(&engine->irq_queue);
> > > > @@ -2096,14 +2097,14 @@ static int logical_render_ring_init(struct
> > > > intel_engine_cs *engine)
> > > >       return ret;
> > > >   }
> > > > 
> > > > -static const struct logical_ring_info {
> > > > +static const struct engine_info {
> > > >       const char *name;
> > > >       unsigned exec_id;
> > > >       unsigned guc_id;
> > > >       u32 mmio_base;
> > > >       unsigned irq_shift;
> > > >       int (*init)(struct intel_engine_cs *engine);
> > > > -} logical_rings[] = {
> > > > +} intel_engines[] = {
> > > >       [RCS] = {
> > > >           .name = "render ring",
> > > >           .exec_id = I915_EXEC_RENDER,
> > > > @@ -2146,20 +2147,31 @@ static const struct logical_ring_info {
> > > >       },
> > > >   };
> > > > 
> > > > -static struct intel_engine_cs *
> > > > -logical_ring_setup(struct drm_i915_private *dev_priv, enum
> > > > intel_engine_id id)
> > > > +struct intel_engine_cs *
> > > > +intel_engine_setup(struct drm_i915_private *dev_priv,
> > > > +           enum intel_engine_id id)
> > > 
> > > Kerneldoc for this would be nice. Also, we now have a mess between
> > > intel_lrc.c and intel_ringbuffer.c. Extracting intel_engine.c with the
> > 
> > Yes that was already suggested by Chris and I am trybotting the
> > additions today, again on his explicit request to progress this series.
> > 
> > > shared bits or something similar, plus cleanup up all the docs would be
> > > awesome as a follow up.
> > 
> > What do you mean "all the docs"? :D
> > 
> > > 
> > > With the kerneldoc added:
> > > 
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Thanks, will add.
> 
> Actually the function becomes static later in the series, when the common
> stuff is moved into intel_engine_cs.c.
> 
> Can I keep the r-b without adding kernel doc for it then?

Sure.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-07-14 14:29 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
2016-07-01 16:47 ` [PATCH 2/5] drm/i915: Prepare for engine init unification Tvrtko Ursulin
2016-07-01 16:47 ` [PATCH 3/5] drm/i915: Unify engine init loop Tvrtko Ursulin
2016-07-01 16:47 ` [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup Tvrtko Ursulin
2016-07-13 12:30   ` Daniel Vetter
2016-07-13 13:19     ` Tvrtko Ursulin
2016-07-01 16:47 ` [PATCH 5/5] drm/i915: Simplify intel_init_ring_buffer prototype Tvrtko Ursulin
2016-07-13 12:30   ` Daniel Vetter
2016-07-01 17:08 ` ✗ Ro.CI.BAT: warning for series starting with [1/5] drm/i915: unify first-stage engine struct setup Patchwork
2016-07-13 12:23 ` [PATCH 1/5] " Daniel Vetter
2016-07-13 13:16   ` Tvrtko Ursulin
2016-07-13 13:24     ` Tvrtko Ursulin
2016-07-14 14:29       ` Daniel Vetter
2016-07-14 11:33     ` Dave Gordon

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.