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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ messages in thread
* [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup
  2016-07-06 10:52 Tvrtko Ursulin
@ 2016-07-06 10:52 ` Tvrtko Ursulin
  0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2016-07-06 10:52 UTC (permalink / raw)
  To: Intel-gfx

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

Use more of the shared engine setup data for legacy engine
initialization. This time to simplify the irq initialization
code.

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

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 91ca268565e6..6b24c1642d84 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2789,6 +2789,8 @@ static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv,
 static void intel_ring_init_irq(struct drm_i915_private *dev_priv,
 				struct intel_engine_cs *engine)
 {
+	engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << engine->irq_shift;
+
 	if (INTEL_GEN(dev_priv) >= 8) {
 		engine->irq_enable = gen8_irq_enable;
 		engine->irq_disable = gen8_irq_disable;
@@ -2842,7 +2844,6 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
 
 	intel_ring_default_vfuncs(dev_priv, engine);
 
-	engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
 	if (HAS_L3_DPF(dev_priv))
 		engine->irq_keep_mask = GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
 
@@ -2901,10 +2902,7 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
 		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;
@@ -2928,8 +2926,6 @@ int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
 	intel_ring_default_vfuncs(dev_priv, 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->drm, engine);
 }
@@ -2941,10 +2937,7 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
 	intel_ring_default_vfuncs(dev_priv, 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->drm, engine);
@@ -2958,10 +2951,7 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *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_enable = hsw_vebox_irq_enable;
 		engine->irq_disable = hsw_vebox_irq_disable;
-- 
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] 15+ messages in thread
end of thread, other threads:[~2016-07-14 14:29 UTC | newest]

Thread overview: 15+ 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
2016-07-06 10:52 Tvrtko Ursulin
2016-07-06 10:52 ` [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup Tvrtko Ursulin

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.