[PATCH 1/5] drm/i915: s/init()/init_hw()/ in intel_engine_cs

[PATCH 1/5] drm/i915: s/init()/init_hw()/ in intel_engine_cs

[Daniel Vetter]

This is (mostly, some exceptions that need fixing) the hw setup
function which starts the ring. And not the function which allocates
all the resources.

Make this clear by giving it a better name.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 14 +++++++-------
 drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  2 +-
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index e588376227ea..5e14316c80d0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1389,8 +1389,8 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
 	if (ret)
 		return ret;
 
-	if (ring->init) {
-		ret = ring->init(ring);
+	if (ring->init_hw) {
+		ret = ring->init_hw(ring);
 		if (ret)
 			return ret;
 	}
@@ -1415,7 +1415,7 @@ static int logical_render_ring_init(struct drm_device *dev)
 	if (HAS_L3_DPF(dev))
 		ring->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
 
-	ring->init = gen8_init_render_ring;
+	ring->init_hw = gen8_init_render_ring;
 	ring->init_context = intel_logical_ring_workarounds_emit;
 	ring->cleanup = intel_fini_pipe_control;
 	ring->get_seqno = gen8_get_seqno;
@@ -1442,7 +1442,7 @@ static int logical_bsd_ring_init(struct drm_device *dev)
 	ring->irq_keep_mask =
 		GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS1_IRQ_SHIFT;
 
-	ring->init = gen8_init_common_ring;
+	ring->init_hw = gen8_init_common_ring;
 	ring->get_seqno = gen8_get_seqno;
 	ring->set_seqno = gen8_set_seqno;
 	ring->emit_request = gen8_emit_request;
@@ -1467,7 +1467,7 @@ static int logical_bsd2_ring_init(struct drm_device *dev)
 	ring->irq_keep_mask =
 		GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS2_IRQ_SHIFT;
 
-	ring->init = gen8_init_common_ring;
+	ring->init_hw = gen8_init_common_ring;
 	ring->get_seqno = gen8_get_seqno;
 	ring->set_seqno = gen8_set_seqno;
 	ring->emit_request = gen8_emit_request;
@@ -1492,7 +1492,7 @@ static int logical_blt_ring_init(struct drm_device *dev)
 	ring->irq_keep_mask =
 		GT_CONTEXT_SWITCH_INTERRUPT << GEN8_BCS_IRQ_SHIFT;
 
-	ring->init = gen8_init_common_ring;
+	ring->init_hw = gen8_init_common_ring;
 	ring->get_seqno = gen8_get_seqno;
 	ring->set_seqno = gen8_set_seqno;
 	ring->emit_request = gen8_emit_request;
@@ -1517,7 +1517,7 @@ static int logical_vebox_ring_init(struct drm_device *dev)
 	ring->irq_keep_mask =
 		GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VECS_IRQ_SHIFT;
 
-	ring->init = gen8_init_common_ring;
+	ring->init_hw = gen8_init_common_ring;
 	ring->get_seqno = gen8_get_seqno;
 	ring->set_seqno = gen8_set_seqno;
 	ring->emit_request = gen8_emit_request;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 1d01b51ff058..367a715a044c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1842,7 +1842,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	if (ret)
 		goto error;
 
-	ret = ring->init(ring);
+	ret = ring->init_hw(ring);
 	if (ret)
 		goto error;
 
@@ -2419,7 +2419,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->dispatch_execbuffer = i830_dispatch_execbuffer;
 	else
 		ring->dispatch_execbuffer = i915_dispatch_execbuffer;
-	ring->init = init_render_ring;
+	ring->init_hw = init_render_ring;
 	ring->cleanup = render_ring_cleanup;
 
 	/* Workaround batchbuffer to combat CS tlb bug. */
@@ -2512,7 +2512,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 		}
 		ring->dispatch_execbuffer = i965_dispatch_execbuffer;
 	}
-	ring->init = init_ring_common;
+	ring->init_hw = init_ring_common;
 
 	return intel_init_ring_buffer(dev, ring);
 }
@@ -2551,7 +2551,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
 		ring->semaphore.signal = gen8_xcs_signal;
 		GEN8_RING_SEMAPHORE_INIT;
 	}
-	ring->init = init_ring_common;
+	ring->init_hw = init_ring_common;
 
 	return intel_init_ring_buffer(dev, ring);
 }
@@ -2608,7 +2608,7 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
 			ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
 		}
 	}
-	ring->init = init_ring_common;
+	ring->init_hw = init_ring_common;
 
 	return intel_init_ring_buffer(dev, ring);
 }
@@ -2659,7 +2659,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
 			ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
 		}
 	}
-	ring->init = init_ring_common;
+	ring->init_hw = init_ring_common;
 
 	return intel_init_ring_buffer(dev, ring);
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index fe426cff598b..5033cd0d0580 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -146,7 +146,7 @@ struct  intel_engine_cs {
 	bool __must_check (*irq_get)(struct intel_engine_cs *ring);
 	void		(*irq_put)(struct intel_engine_cs *ring);
 
-	int		(*init)(struct intel_engine_cs *ring);
+	int		(*init_hw)(struct intel_engine_cs *ring);
 
 	int		(*init_context)(struct intel_engine_cs *ring,
 					struct intel_context *ctx);
-- 
1.9.3

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

[PATCH 2/5] drm/i915: s/intel_workarouns_ring/intel_render_workarounds/

[Daniel Vetter]

Since it's not for the rings but engine, and its specifically for
render state and workarounds.

Note that there's a massive s/ring/engine/ required all over the
driver, but that's really not part of this patch here. So I've leaft
the paramter names as-is.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 5e14316c80d0..25a2c2b45c6e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1165,7 +1165,7 @@ static int gen8_init_render_ring(struct intel_engine_cs *ring)
 
 	I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING));
 
-	return init_workarounds_ring(ring);
+	return init_render_workarounds(ring);
 }
 
 static int gen8_emit_bb_start(struct intel_ringbuffer *ringbuf,
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 367a715a044c..24af1e33a314 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -803,7 +803,7 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
 	return 0;
 }
 
-int init_workarounds_ring(struct intel_engine_cs *ring)
+int init_render_workarounds(struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -876,7 +876,7 @@ static int init_render_ring(struct intel_engine_cs *ring)
 	if (HAS_L3_DPF(dev))
 		I915_WRITE_IMR(ring, ~GT_PARITY_ERROR(dev));
 
-	return init_workarounds_ring(ring);
+	return init_render_workarounds(ring);
 }
 
 static void render_ring_cleanup(struct intel_engine_cs *ring)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 5033cd0d0580..f5cfab0d162b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -429,7 +429,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev);
 u64 intel_ring_get_active_head(struct intel_engine_cs *ring);
 void intel_ring_setup_status_page(struct intel_engine_cs *ring);
 
-int init_workarounds_ring(struct intel_engine_cs *ring);
+int init_render_workarounds(struct intel_engine_cs *ring);
 
 static inline u32 intel_ring_get_tail(struct intel_ringbuffer *ringbuf)
 {
-- 
1.9.3

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

[PATCH 3/5] drm/i915: Move intel_init_pipe_control out of engine->init_hw

[Daniel Vetter]

With this all the ->init_hw hooks really only set up hw state needed
to start the ring, all the software state setup and memory/buffer
allocations happen beforehand.

v2: We need to call intel_init_pipe_control after the ring init since
otherwise engine->dev is NULL and it falls over. Currently that's
now after the hw ring is enabled but a) we'll be fine as long as no
one submits a batch b) this will change soon.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 12 +++++++-----
 drivers/gpu/drm/i915/intel_ringbuffer.c | 18 +++++++++++-------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 25a2c2b45c6e..3540816c536e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1159,10 +1159,6 @@ static int gen8_init_render_ring(struct intel_engine_cs *ring)
 	 */
 	I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(ASYNC_FLIP_PERF_DISABLE));
 
-	ret = intel_init_pipe_control(ring);
-	if (ret)
-		return ret;
-
 	I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING));
 
 	return init_render_workarounds(ring);
@@ -1404,6 +1400,7 @@ static int logical_render_ring_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *ring = &dev_priv->ring[RCS];
+	int ret;
 
 	ring->name = "render ring";
 	ring->id = RCS;
@@ -1426,7 +1423,12 @@ static int logical_render_ring_init(struct drm_device *dev)
 	ring->irq_put = gen8_logical_ring_put_irq;
 	ring->emit_bb_start = gen8_emit_bb_start;
 
-	return logical_ring_init(dev, ring);
+	ring->dev = dev;
+	ret = logical_ring_init(dev, ring);
+	if (ret)
+		return ret;
+
+	return intel_init_pipe_control(ring);
 }
 
 static int logical_bsd_ring_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 24af1e33a314..2e34db8a791d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -854,12 +854,6 @@ static int init_render_ring(struct intel_engine_cs *ring)
 			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) |
 			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
 
-	if (INTEL_INFO(dev)->gen >= 5) {
-		ret = intel_init_pipe_control(ring);
-		if (ret)
-			return ret;
-	}
-
 	if (IS_GEN6(dev)) {
 		/* From the Sandybridge PRM, volume 1 part 3, page 24:
 		 * "If this bit is set, STCunit will have LRA as replacement
@@ -2441,7 +2435,17 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 		ring->scratch.gtt_offset = i915_gem_obj_ggtt_offset(obj);
 	}
 
-	return intel_init_ring_buffer(dev, ring);
+	ret = intel_init_ring_buffer(dev, ring);
+	if (ret)
+		return ret;
+
+	if (INTEL_INFO(dev)->gen >= 5) {
+		ret = intel_init_pipe_control(ring);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 int intel_init_bsd_ring_buffer(struct drm_device *dev)
-- 
1.9.3

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

[PATCH 4/5] drm/i915: Only init engines once

[Daniel Vetter]

We can do this.

And now there's finally the clean split between software setup and
hardware setup I kinda wanted since multi-ring support was merged
aeons ago. It only took almost 5 years.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         | 13 ++++++++++---
 drivers/gpu/drm/i915/intel_lrc.c        |  6 ------
 drivers/gpu/drm/i915/intel_ringbuffer.c |  4 ----
 3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f028589c431d..99de39a8dea1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4801,6 +4801,7 @@ int
 i915_gem_init_hw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_engine_cs *ring;
 	int ret, i;
 
 	if (INTEL_INFO(dev)->gen < 6 && !intel_enable_gtt())
@@ -4827,9 +4828,11 @@ i915_gem_init_hw(struct drm_device *dev)
 
 	i915_gem_init_swizzling(dev);
 
-	ret = dev_priv->gt.init_rings(dev);
-	if (ret)
-		return ret;
+	for_each_ring(ring, dev_priv, i) {
+		ret = ring->init_hw(ring);
+		if (ret)
+			return ret;
+	}
 
 	for (i = 0; i < NUM_L3_SLICES(dev); i++)
 		i915_gem_l3_remap(&dev_priv->ring[RCS], i);
@@ -4902,6 +4905,10 @@ int i915_gem_init(struct drm_device *dev)
 		return ret;
 	}
 
+	ret = dev_priv->gt.init_rings(dev);
+	if (ret)
+		return ret;
+
 	ret = i915_gem_init_hw(dev);
 	if (ret == -EIO) {
 		/* Allow ring initialisation to fail by marking the GPU as
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3540816c536e..392e9de361d9 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1385,12 +1385,6 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
 	if (ret)
 		return ret;
 
-	if (ring->init_hw) {
-		ret = ring->init_hw(ring);
-		if (ret)
-			return ret;
-	}
-
 	ret = intel_lr_context_deferred_create(ring->default_context, ring);
 
 	return ret;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2e34db8a791d..6bb900778c38 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1836,10 +1836,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	if (ret)
 		goto error;
 
-	ret = ring->init_hw(ring);
-	if (ret)
-		goto error;
-
 	return 0;
 
 error:
-- 
1.9.3

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

[PATCH 5/5] drm/i915: Flatten engine init control flow

[Daniel Vetter]

Now that sanity prevails and we have the clean split between software
init and starting the engines we can drop all the "have we allocate
this struct already?" nonsense.

Execlist code could benefit quite a bit more still, but that's for
another patch.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c  | 14 +++++-----
 drivers/gpu/drm/i915/intel_lrc.c        |  3 +--
 drivers/gpu/drm/i915/intel_ringbuffer.c | 45 ++++++++++++++++-----------------
 3 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 22c992a78ac6..6e9eac4b1757 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -716,13 +716,13 @@ int i915_cmd_parser_init_ring(struct intel_engine_cs *ring)
 	BUG_ON(!validate_cmds_sorted(ring, cmd_tables, cmd_table_count));
 	BUG_ON(!validate_regs_sorted(ring));
 
-	if (hash_empty(ring->cmd_hash)) {
-		ret = init_hash_table(ring, cmd_tables, cmd_table_count);
-		if (ret) {
-			DRM_ERROR("CMD: cmd_parser_init failed!\n");
-			fini_hash_table(ring);
-			return ret;
-		}
+	WARN_ON(!hash_empty(ring->cmd_hash));
+
+	ret = init_hash_table(ring, cmd_tables, cmd_table_count);
+	if (ret) {
+		DRM_ERROR("CMD: cmd_parser_init failed!\n");
+		fini_hash_table(ring);
+		return ret;
 	}
 
 	ring->needs_cmd_parser = true;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 392e9de361d9..7df97daed07d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1831,8 +1831,7 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 	int ret;
 
 	WARN_ON(ctx->legacy_hw_ctx.rcs_state != NULL);
-	if (ctx->engine[ring->id].state)
-		return 0;
+	WARN_ON(ctx->engine[ring->id].state);
 
 	context_size = round_up(get_lr_context_size(ring), 4096);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 6bb900778c38..41967280cc1a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -624,8 +624,7 @@ intel_init_pipe_control(struct intel_engine_cs *ring)
 {
 	int ret;
 
-	if (ring->scratch.obj)
-		return 0;
+	WARN_ON(ring->scratch.obj);
 
 	ring->scratch.obj = i915_gem_alloc_object(ring->dev, 4096);
 	if (ring->scratch.obj == NULL) {
@@ -1776,15 +1775,15 @@ int intel_alloc_ringbuffer_obj(struct drm_device *dev,
 static int intel_init_ring_buffer(struct drm_device *dev,
 				  struct intel_engine_cs *ring)
 {
-	struct intel_ringbuffer *ringbuf = ring->buffer;
+	struct intel_ringbuffer *ringbuf;
 	int ret;
 
-	if (ringbuf == NULL) {
-		ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL);
-		if (!ringbuf)
-			return -ENOMEM;
-		ring->buffer = ringbuf;
-	}
+	WARN_ON(ring->buffer);
+
+	ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL);
+	if (!ringbuf)
+		return -ENOMEM;
+	ring->buffer = ringbuf;
 
 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
@@ -1807,21 +1806,21 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 			goto error;
 	}
 
-	if (ringbuf->obj == NULL) {
-		ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
-		if (ret) {
-			DRM_ERROR("Failed to allocate ringbuffer %s: %d\n",
-					ring->name, ret);
-			goto error;
-		}
+	WARN_ON(ringbuf->obj);
 
-		ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
-		if (ret) {
-			DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n",
-					ring->name, ret);
-			intel_destroy_ringbuffer_obj(ringbuf);
-			goto error;
-		}
+	ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
+	if (ret) {
+		DRM_ERROR("Failed to allocate ringbuffer %s: %d\n",
+				ring->name, ret);
+		goto error;
+	}
+
+	ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
+	if (ret) {
+		DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n",
+				ring->name, ret);
+		intel_destroy_ringbuffer_obj(ringbuf);
+		goto error;
 	}
 
 	/* Workaround an erratum on the i830 which causes a hang if
-- 
1.9.3

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

Re: [PATCH 1/5] drm/i915: s/init()/init_hw()/ in intel_engine_cs

[Chris Wilson]

On Thu, Nov 20, 2014 at 12:33:04AM +0100, Daniel Vetter wrote:
> This is (mostly, some exceptions that need fixing) the hw setup
> function which starts the ring. And not the function which allocates
> all the resources.
> 
> Make this clear by giving it a better name.

->resume() because it should be only called in the resume paths after
init! And we should have a ->susped() as well.

(Not keen on the init_hw we have spread out.)

See

http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem.c#n4464
http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem.c#n4394
http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem.c#n4572

for the distinction
-Chris

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

Re: [PATCH 2/5] drm/i915: s/intel_workarouns_ring/intel_render_workarounds/

[Chris Wilson]

On Thu, Nov 20, 2014 at 12:33:05AM +0100, Daniel Vetter wrote:
> Since it's not for the rings but engine, and its specifically for
> render state and workarounds.

Nope. It is for initialising the context.

setup_workarounds() would be for constructing the table to be applied
once. The current function is still snafu by adding to the workarounds
every time we reinitialize the default context.
-Chris

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

Re: [PATCH 4/5] drm/i915: Only init engines once

[Chris Wilson]

On Thu, Nov 20, 2014 at 12:33:07AM +0100, Daniel Vetter wrote:
> We can do this.
> 
> And now there's finally the clean split between software setup and
> hardware setup I kinda wanted since multi-ring support was merged
> aeons ago. It only took almost 5 years.

Can I humbly suggest:

http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem.c#n4464
http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem.c#n4394
-Chris

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

Re: [PATCH 5/5] drm/i915: Flatten engine init control flow

[Chris Wilson]

On Thu, Nov 20, 2014 at 12:33:08AM +0100, Daniel Vetter wrote:
> Now that sanity prevails and we have the clean split between software
> init and starting the engines we can drop all the "have we allocate
> this struct already?" nonsense.

Actually, I was hoping for something completely different when you sad
flattening the init...

Compare with
http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/intel_ringbuffer.c#n1983

There is a lot of duplicated code in the ringbuffer setup, and the code
movement here wants to be split differently so that the sequence is
identical to execlists. (The only difference is that we only use one
ring with legacy, rather than one ring in each context [per-engine].)
Making sure the logic is foolproof for both is esssential.
-Chris

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

[PATCH] drm/i915: Move init_unused_rings to gem_init_hw

[Daniel Vetter]

We need to do that every time we resume the rings, not just at load.
I've overlooked this in my untangling of the ring init code.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 99de39a8dea1..23e17735a6a9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4741,14 +4741,6 @@ int i915_gem_init_rings(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	/*
-	 * At least 830 can leave some of the unused rings
-	 * "active" (ie. head != tail) after resume which
-	 * will prevent c3 entry. Makes sure all unused rings
-	 * are totally idle.
-	 */
-	init_unused_rings(dev);
-
 	ret = intel_init_render_ring_buffer(dev);
 	if (ret)
 		return ret;
@@ -4828,6 +4820,14 @@ i915_gem_init_hw(struct drm_device *dev)
 
 	i915_gem_init_swizzling(dev);
 
+	/*
+	 * At least 830 can leave some of the unused rings
+	 * "active" (ie. head != tail) after resume which
+	 * will prevent c3 entry. Makes sure all unused rings
+	 * are totally idle.
+	 */
+	init_unused_rings(dev);
+
 	for_each_ring(ring, dev_priv, i) {
 		ret = ring->init_hw(ring);
 		if (ret)
-- 
1.9.3

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

Re: [PATCH 1/5] drm/i915: s/init()/init_hw()/ in intel_engine_cs

[Daniel Vetter]

On Thu, Nov 20, 2014 at 08:03:29AM +0000, Chris Wilson wrote:
> On Thu, Nov 20, 2014 at 12:33:04AM +0100, Daniel Vetter wrote:
> > This is (mostly, some exceptions that need fixing) the hw setup
> > function which starts the ring. And not the function which allocates
> > all the resources.
> > 
> > Make this clear by giving it a better name.
> 
> ->resume() because it should be only called in the resume paths after
> init! And we should have a ->susped() as well.
> 
> (Not keen on the init_hw we have spread out.)

Well that's what the bunch is currently called - there's lots more init_hw
functions. I agree it's confusing but I'm not terribly offended really.
And fixing that up would be a much larger and more invasive patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: s/intel_workarouns_ring/intel_render_workarounds/

[Daniel Vetter]

On Thu, Nov 20, 2014 at 08:05:27AM +0000, Chris Wilson wrote:
> On Thu, Nov 20, 2014 at 12:33:05AM +0100, Daniel Vetter wrote:
> > Since it's not for the rings but engine, and its specifically for
> > render state and workarounds.
> 
> Nope. It is for initialising the context.
> 
> setup_workarounds() would be for constructing the table to be applied
> once. The current function is still snafu by adding to the workarounds
> every time we reinitialize the default context.

Well we also have a ring->init_context which does the other part. I agree
it's a snafu, but I've figured this one here will get us a tiny step
closer. Well, I'll drop it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: s/init()/init_hw()/ in intel_engine_cs

[Chris Wilson]

On Thu, Nov 20, 2014 at 10:11:29AM +0100, Daniel Vetter wrote:
> On Thu, Nov 20, 2014 at 08:03:29AM +0000, Chris Wilson wrote:
> > On Thu, Nov 20, 2014 at 12:33:04AM +0100, Daniel Vetter wrote:
> > > This is (mostly, some exceptions that need fixing) the hw setup
> > > function which starts the ring. And not the function which allocates
> > > all the resources.
> > > 
> > > Make this clear by giving it a better name.
> > 
> > ->resume() because it should be only called in the resume paths after
> > init! And we should have a ->susped() as well.
> > 
> > (Not keen on the init_hw we have spread out.)
> 
> Well that's what the bunch is currently called - there's lots more init_hw
> functions. I agree it's confusing but I'm not terribly offended really.
> And fixing that up would be a much larger and more invasive patch.

One piece at a time makes it a much smaller patch...
-Chris

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

Re: [PATCH 5/5] drm/i915: Flatten engine init control flow

[Daniel Vetter]

On Thu, Nov 20, 2014 at 08:10:27AM +0000, Chris Wilson wrote:
> On Thu, Nov 20, 2014 at 12:33:08AM +0100, Daniel Vetter wrote:
> > Now that sanity prevails and we have the clean split between software
> > init and starting the engines we can drop all the "have we allocate
> > this struct already?" nonsense.
> 
> Actually, I was hoping for something completely different when you sad
> flattening the init...

I presume you've hoped for more ;-) Essentially this is just the legacy
ringbuffer blueprint, applying to execlist left as an exercise for the
reader. And it's just a very small patch.

> Compare with
> http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/intel_ringbuffer.c#n1983
> 
> There is a lot of duplicated code in the ringbuffer setup, and the code
> movement here wants to be split differently so that the sequence is
> identical to execlists. (The only difference is that we only use one
> ring with legacy, rather than one ring in each context [per-engine].)
> Making sure the logic is foolproof for both is esssential.

Imo we first need to beat execlist into shape (with small patches, not by
rewriting it all). Once stuff settles we can take another look at whether
unification makes sense, but for now I want a big piece of insulation
between execlist and stuff we actually depend on. Or at least stuff Dave
might rip my head off over.

So for now I still want to embrace the duplication.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Move init_unused_rings to gem_init_hw

[Dave Gordon]

On 20/11/14 08:45, Daniel Vetter wrote:
> We need to do that every time we resume the rings, not just at load.
> I've overlooked this in my untangling of the ring init code.

Hi Daniel,

another thing that needs untangling in the general maze of init code is
the initialisation of the active and request lists -- Thomas Daniel's
complaint about 11/28 of the s/seqno/request/ patchset was essentially
because John was adding more lists that appear to be redundantly
initialised in multiple places. Please see my followup at
http://lists.freedesktop.org/archives/intel-gfx/2014-November/055856.html

As it looks like you're getting rid of intel_render_ring_init_dri(), if
we could also resolve whether init_ring_lists() is also now redundant,
that would mean there were no duplicated list initialisations :)

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

Re: [PATCH] drm/i915: Move init_unused_rings to gem_init_hw

[Daniel Vetter]

On Fri, Nov 21, 2014 at 07:01:54PM +0000, Dave Gordon wrote:
> On 20/11/14 08:45, Daniel Vetter wrote:
> > We need to do that every time we resume the rings, not just at load.
> > I've overlooked this in my untangling of the ring init code.
> 
> Hi Daniel,
> 
> another thing that needs untangling in the general maze of init code is
> the initialisation of the active and request lists -- Thomas Daniel's
> complaint about 11/28 of the s/seqno/request/ patchset was essentially
> because John was adding more lists that appear to be redundantly
> initialised in multiple places. Please see my followup at
> http://lists.freedesktop.org/archives/intel-gfx/2014-November/055856.html
> 
> As it looks like you're getting rid of intel_render_ring_init_dri(), if
> we could also resolve whether init_ring_lists() is also now redundant,
> that would mean there were no duplicated list initialisations :)

Actually I wanted to feature my little series here in your thread as one
step closer to untangling this stuff too, so ... care to review (except
for the one patch that is dropped already)?

Wrt the lists I think we should first untangle the execlit/request story,
since I expect that a few of them need to be moved to different structs.
After that it should be a bit clearer what needs to be moved where. And
then there's also the golden context and render wa init code which also
needs to be shuffled a bit (similar to this series).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: s/init()/init_hw()/ in intel_engine_cs

[Dave Gordon]

On 19/11/14 23:33, Daniel Vetter wrote:
> This is (mostly, some exceptions that need fixing) the hw setup
> function which starts the ring. And not the function which allocates
> all the resources.
> 
> Make this clear by giving it a better name.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_lrc.c        | 14 +++++++-------
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  2 +-
>  3 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index e588376227ea..5e14316c80d0 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1389,8 +1389,8 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
>  	if (ret)
>  		return ret;
>  
> -	if (ring->init) {
> -		ret = ring->init(ring);
> +	if (ring->init_hw) {
> +		ret = ring->init_hw(ring);
>  		if (ret)
>  			return ret;
>  	}
> @@ -1415,7 +1415,7 @@ static int logical_render_ring_init(struct drm_device *dev)
>  	if (HAS_L3_DPF(dev))
>  		ring->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
>  
> -	ring->init = gen8_init_render_ring;
> +	ring->init_hw = gen8_init_render_ring;
>  	ring->init_context = intel_logical_ring_workarounds_emit;
>  	ring->cleanup = intel_fini_pipe_control;
>  	ring->get_seqno = gen8_get_seqno;
> @@ -1442,7 +1442,7 @@ static int logical_bsd_ring_init(struct drm_device *dev)
>  	ring->irq_keep_mask =
>  		GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS1_IRQ_SHIFT;
>  
> -	ring->init = gen8_init_common_ring;
> +	ring->init_hw = gen8_init_common_ring;
>  	ring->get_seqno = gen8_get_seqno;
>  	ring->set_seqno = gen8_set_seqno;
>  	ring->emit_request = gen8_emit_request;
> @@ -1467,7 +1467,7 @@ static int logical_bsd2_ring_init(struct drm_device *dev)
>  	ring->irq_keep_mask =
>  		GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS2_IRQ_SHIFT;
>  
> -	ring->init = gen8_init_common_ring;
> +	ring->init_hw = gen8_init_common_ring;
>  	ring->get_seqno = gen8_get_seqno;
>  	ring->set_seqno = gen8_set_seqno;
>  	ring->emit_request = gen8_emit_request;
> @@ -1492,7 +1492,7 @@ static int logical_blt_ring_init(struct drm_device *dev)
>  	ring->irq_keep_mask =
>  		GT_CONTEXT_SWITCH_INTERRUPT << GEN8_BCS_IRQ_SHIFT;
>  
> -	ring->init = gen8_init_common_ring;
> +	ring->init_hw = gen8_init_common_ring;
>  	ring->get_seqno = gen8_get_seqno;
>  	ring->set_seqno = gen8_set_seqno;
>  	ring->emit_request = gen8_emit_request;
> @@ -1517,7 +1517,7 @@ static int logical_vebox_ring_init(struct drm_device *dev)
>  	ring->irq_keep_mask =
>  		GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VECS_IRQ_SHIFT;
>  
> -	ring->init = gen8_init_common_ring;
> +	ring->init_hw = gen8_init_common_ring;
>  	ring->get_seqno = gen8_get_seqno;
>  	ring->set_seqno = gen8_set_seqno;
>  	ring->emit_request = gen8_emit_request;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 1d01b51ff058..367a715a044c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1842,7 +1842,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  	if (ret)
>  		goto error;
>  
> -	ret = ring->init(ring);
> +	ret = ring->init_hw(ring);
>  	if (ret)
>  		goto error;
>  
> @@ -2419,7 +2419,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>  		ring->dispatch_execbuffer = i830_dispatch_execbuffer;
>  	else
>  		ring->dispatch_execbuffer = i915_dispatch_execbuffer;
> -	ring->init = init_render_ring;
> +	ring->init_hw = init_render_ring;
>  	ring->cleanup = render_ring_cleanup;
>  
>  	/* Workaround batchbuffer to combat CS tlb bug. */
> @@ -2512,7 +2512,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
>  		}
>  		ring->dispatch_execbuffer = i965_dispatch_execbuffer;
>  	}
> -	ring->init = init_ring_common;
> +	ring->init_hw = init_ring_common;
>  
>  	return intel_init_ring_buffer(dev, ring);
>  }
> @@ -2551,7 +2551,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
>  		ring->semaphore.signal = gen8_xcs_signal;
>  		GEN8_RING_SEMAPHORE_INIT;
>  	}
> -	ring->init = init_ring_common;
> +	ring->init_hw = init_ring_common;
>  
>  	return intel_init_ring_buffer(dev, ring);
>  }
> @@ -2608,7 +2608,7 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
>  			ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
>  		}
>  	}
> -	ring->init = init_ring_common;
> +	ring->init_hw = init_ring_common;
>  
>  	return intel_init_ring_buffer(dev, ring);
>  }
> @@ -2659,7 +2659,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
>  			ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
>  		}
>  	}
> -	ring->init = init_ring_common;
> +	ring->init_hw = init_ring_common;
>  
>  	return intel_init_ring_buffer(dev, ring);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index fe426cff598b..5033cd0d0580 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -146,7 +146,7 @@ struct  intel_engine_cs {
>  	bool __must_check (*irq_get)(struct intel_engine_cs *ring);
>  	void		(*irq_put)(struct intel_engine_cs *ring);
>  
> -	int		(*init)(struct intel_engine_cs *ring);
> +	int		(*init_hw)(struct intel_engine_cs *ring);
>  
>  	int		(*init_context)(struct intel_engine_cs *ring,
>  					struct intel_context *ctx);
> 

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

Re: [PATCH 2/5] drm/i915: s/intel_workarouns_ring/intel_render_workarounds/

[Dave Gordon]

On 19/11/14 23:33, Daniel Vetter wrote:
> Since it's not for the rings but engine, and its specifically for
> render state and workarounds.
> 
> Note that there's a massive s/ring/engine/ required all over the
> driver, but that's really not part of this patch here. So I've leaft
> the paramter names as-is.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---

Code is fine, but the commit message contains several typos.
s/leaft/left/ and s/paramter/parameter/ don't really matter,
but the one in the subject line is a bit more important.
It should be "s/init_workarounds_ring/init_render_workarounds/",
with no mention of "intel_" !

Once that's corrected, then

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

.Dave.

>  drivers/gpu/drm/i915/intel_lrc.c        | 2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 5e14316c80d0..25a2c2b45c6e 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1165,7 +1165,7 @@ static int gen8_init_render_ring(struct intel_engine_cs *ring)
>  
>  	I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING));
>  
> -	return init_workarounds_ring(ring);
> +	return init_render_workarounds(ring);
>  }
>  
>  static int gen8_emit_bb_start(struct intel_ringbuffer *ringbuf,
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 367a715a044c..24af1e33a314 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -803,7 +803,7 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
>  	return 0;
>  }
>  
> -int init_workarounds_ring(struct intel_engine_cs *ring)
> +int init_render_workarounds(struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = ring->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -876,7 +876,7 @@ static int init_render_ring(struct intel_engine_cs *ring)
>  	if (HAS_L3_DPF(dev))
>  		I915_WRITE_IMR(ring, ~GT_PARITY_ERROR(dev));
>  
> -	return init_workarounds_ring(ring);
> +	return init_render_workarounds(ring);
>  }
>  
>  static void render_ring_cleanup(struct intel_engine_cs *ring)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 5033cd0d0580..f5cfab0d162b 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -429,7 +429,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev);
>  u64 intel_ring_get_active_head(struct intel_engine_cs *ring);
>  void intel_ring_setup_status_page(struct intel_engine_cs *ring);
>  
> -int init_workarounds_ring(struct intel_engine_cs *ring);
> +int init_render_workarounds(struct intel_engine_cs *ring);
>  
>  static inline u32 intel_ring_get_tail(struct intel_ringbuffer *ringbuf)
>  {
> 

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

Re: [PATCH 3/5] drm/i915: Move intel_init_pipe_control out of engine->init_hw

[Dave Gordon]

On 19/11/14 23:33, Daniel Vetter wrote:
> With this all the ->init_hw hooks really only set up hw state needed
> to start the ring, all the software state setup and memory/buffer
> allocations happen beforehand.
> 
> v2: We need to call intel_init_pipe_control after the ring init since
> otherwise engine->dev is NULL and it falls over. Currently that's
> now after the hw ring is enabled but a) we'll be fine as long as no
> one submits a batch b) this will change soon.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

I was going to say that it would be nice to move the init_hw() out of
intel_init_ring_buffer() and logical_ring_init() so that they actually
do what their names say, but I see that's forthcoming, so

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

.Dave.

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

Re: [PATCH 4/5] drm/i915: Only init engines once

[Dave Gordon]

On 19/11/14 23:33, Daniel Vetter wrote:
> We can do this.
> 
> And now there's finally the clean split between software setup and
> hardware setup I kinda wanted since multi-ring support was merged
> aeons ago. It only took almost 5 years.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 13 ++++++++++---
>  drivers/gpu/drm/i915/intel_lrc.c        |  6 ------
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  4 ----
>  3 files changed, 10 insertions(+), 13 deletions(-)

Yep, that's what I was hoping for :)

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

(Mind you, I like Chris's suggestions too, especially converting all the
historical /ring/ to the more accurate /engine/).

> Can I humbly suggest:
>
>
http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem.c#n4464
>
http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem.c#n4394
> -Chris




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

Re: [PATCH 2/5] drm/i915: s/intel_workarouns_ring/intel_render_workarounds/

[Daniel Vetter]

On Thu, Nov 27, 2014 at 03:07:28PM +0000, Dave Gordon wrote:
> On 19/11/14 23:33, Daniel Vetter wrote:
> > Since it's not for the rings but engine, and its specifically for
> > render state and workarounds.
> > 
> > Note that there's a massive s/ring/engine/ required all over the
> > driver, but that's really not part of this patch here. So I've leaft
> > the paramter names as-is.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> 
> Code is fine, but the commit message contains several typos.
> s/leaft/left/ and s/paramter/parameter/ don't really matter,
> but the one in the subject line is a bit more important.
> It should be "s/init_workarounds_ring/init_render_workarounds/",
> with no mention of "intel_" !
> 
> Once that's corrected, then
> 
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

Well I've decided to drop this one since like Chris mentioned there's a
bit a mess here with different callbacks doing different parts of the ring
ctx init. And it kinda doesn't make sense to bikeshed names while we still
have a bigger confusion.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Only init engines once

[Daniel Vetter]

On Fri, Nov 28, 2014 at 12:02:40PM +0000, Dave Gordon wrote:
> On 19/11/14 23:33, Daniel Vetter wrote:
> > We can do this.
> > 
> > And now there's finally the clean split between software setup and
> > hardware setup I kinda wanted since multi-ring support was merged
> > aeons ago. It only took almost 5 years.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c         | 13 ++++++++++---
> >  drivers/gpu/drm/i915/intel_lrc.c        |  6 ------
> >  drivers/gpu/drm/i915/intel_ringbuffer.c |  4 ----
> >  3 files changed, 10 insertions(+), 13 deletions(-)
> 
> Yep, that's what I was hoping for :)
> 
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

Thanks for the review, I've merged the first 3 patches from here.

> (Mind you, I like Chris's suggestions too, especially converting all the
> historical /ring/ to the more accurate /engine/).

Yup, we're not going to run out of polish patches for a long time in GEM.
I actually chatted about this with Jon Ewins and he brought up the
possibility that someone from his team would dig into this full-time.
Otherwise it'll take ages to get the accumulated cruft of the past few
years out and document the tribal knowledge.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Only init engines once

[Chris Wilson]

On Fri, Nov 28, 2014 at 06:56:24PM +0100, Daniel Vetter wrote:
> On Fri, Nov 28, 2014 at 12:02:40PM +0000, Dave Gordon wrote:
> > On 19/11/14 23:33, Daniel Vetter wrote:
> > > We can do this.
> > > 
> > > And now there's finally the clean split between software setup and
> > > hardware setup I kinda wanted since multi-ring support was merged
> > > aeons ago. It only took almost 5 years.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c         | 13 ++++++++++---
> > >  drivers/gpu/drm/i915/intel_lrc.c        |  6 ------
> > >  drivers/gpu/drm/i915/intel_ringbuffer.c |  4 ----
> > >  3 files changed, 10 insertions(+), 13 deletions(-)
> > 
> > Yep, that's what I was hoping for :)
> > 
> > Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
> 
> Thanks for the review, I've merged the first 3 patches from here.
> 
> > (Mind you, I like Chris's suggestions too, especially converting all the
> > historical /ring/ to the more accurate /engine/).
> 
> Yup, we're not going to run out of polish patches for a long time in GEM.
> I actually chatted about this with Jon Ewins and he brought up the
> possibility that someone from his team would dig into this full-time.
> Otherwise it'll take ages to get the accumulated cruft of the past few
> years out and document the tribal knowledge.

I spent 8 hours doing so one day only to get completely ignored.
-Chris

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

Re: [PATCH 5/5] drm/i915: Flatten engine init control flow

[Dave Gordon]

On 20/11/14 09:19, Daniel Vetter wrote:
> On Thu, Nov 20, 2014 at 08:10:27AM +0000, Chris Wilson wrote:
>> On Thu, Nov 20, 2014 at 12:33:08AM +0100, Daniel Vetter wrote:
>>> Now that sanity prevails and we have the clean split between software
>>> init and starting the engines we can drop all the "have we allocate
>>> this struct already?" nonsense.
>>
>> Actually, I was hoping for something completely different when you sad
>> flattening the init...
> 
> I presume you've hoped for more ;-) Essentially this is just the legacy
> ringbuffer blueprint, applying to execlist left as an exercise for the
> reader. And it's just a very small patch.
> 
>> Compare with
>> http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/intel_ringbuffer.c#n1983
>>
>> There is a lot of duplicated code in the ringbuffer setup, and the code
>> movement here wants to be split differently so that the sequence is
>> identical to execlists. (The only difference is that we only use one
>> ring with legacy, rather than one ring in each context [per-engine].)
>> Making sure the logic is foolproof for both is esssential.
> 
> Imo we first need to beat execlist into shape (with small patches, not by
> rewriting it all). Once stuff settles we can take another look at whether
> unification makes sense, but for now I want a big piece of insulation
> between execlist and stuff we actually depend on. Or at least stuff Dave
> might rip my head off over.
> 
> So for now I still want to embrace the duplication.
> -Daniel

As a matter of policy: while we have the duplication and the split
between legacy ringbuffer and lrc versions, should developers try to
submit corresponding changes to both paths together (e.g. in the same
patchset). Or is it OK to submit improvements only to one of the two
paths, causing them to diverge and hence possibly making deduplication
more difficult?

Anyway, this change looks OK in itself, so subject to the "exercise for
the reader" approach being acceptable (on which point I defer to your
and others judgement), then:

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

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

Re: [PATCH 5/5] drm/i915: Flatten engine init control flow

[Daniel Vetter]

On Mon, Dec 01, 2014 at 04:11:15PM +0000, Dave Gordon wrote:
> On 20/11/14 09:19, Daniel Vetter wrote:
> > On Thu, Nov 20, 2014 at 08:10:27AM +0000, Chris Wilson wrote:
> >> On Thu, Nov 20, 2014 at 12:33:08AM +0100, Daniel Vetter wrote:
> >>> Now that sanity prevails and we have the clean split between software
> >>> init and starting the engines we can drop all the "have we allocate
> >>> this struct already?" nonsense.
> >>
> >> Actually, I was hoping for something completely different when you sad
> >> flattening the init...
> > 
> > I presume you've hoped for more ;-) Essentially this is just the legacy
> > ringbuffer blueprint, applying to execlist left as an exercise for the
> > reader. And it's just a very small patch.
> > 
> >> Compare with
> >> http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/intel_ringbuffer.c#n1983
> >>
> >> There is a lot of duplicated code in the ringbuffer setup, and the code
> >> movement here wants to be split differently so that the sequence is
> >> identical to execlists. (The only difference is that we only use one
> >> ring with legacy, rather than one ring in each context [per-engine].)
> >> Making sure the logic is foolproof for both is esssential.
> > 
> > Imo we first need to beat execlist into shape (with small patches, not by
> > rewriting it all). Once stuff settles we can take another look at whether
> > unification makes sense, but for now I want a big piece of insulation
> > between execlist and stuff we actually depend on. Or at least stuff Dave
> > might rip my head off over.
> > 
> > So for now I still want to embrace the duplication.
> > -Daniel
> 
> As a matter of policy: while we have the duplication and the split
> between legacy ringbuffer and lrc versions, should developers try to
> submit corresponding changes to both paths together (e.g. in the same
> patchset). Or is it OK to submit improvements only to one of the two
> paths, causing them to diverge and hence possibly making deduplication
> more difficult?

Yeah in this case I've figured I'll cause more trouble than benefit when I
trample all over intel_lrc.c. Also, I don't have a machine for testing
execlist ;-)
> 
> Anyway, this change looks OK in itself, so subject to the "exercise for
> the reader" approach being acceptable (on which point I defer to your
> and others judgement), then:
> 
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

Queued for -next, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Move init_unused_rings to gem_init_hw

[Dave Gordon]

On 21/11/14 20:27, Daniel Vetter wrote:
> On Fri, Nov 21, 2014 at 07:01:54PM +0000, Dave Gordon wrote:
>> On 20/11/14 08:45, Daniel Vetter wrote:
>>> We need to do that every time we resume the rings, not just at load.
>>> I've overlooked this in my untangling of the ring init code.
>>
>> Hi Daniel,
>>
>> another thing that needs untangling in the general maze of init code is
>> the initialisation of the active and request lists -- Thomas Daniel's
>> complaint about 11/28 of the s/seqno/request/ patchset was essentially
>> because John was adding more lists that appear to be redundantly
>> initialised in multiple places. Please see my followup at
>> http://lists.freedesktop.org/archives/intel-gfx/2014-November/055856.html
>>
>> As it looks like you're getting rid of intel_render_ring_init_dri(), if
>> we could also resolve whether init_ring_lists() is also now redundant,
>> that would mean there were no duplicated list initialisations :)
> 
> Actually I wanted to feature my little series here in your thread as one
> step closer to untangling this stuff too, so ... care to review (except
> for the one patch that is dropped already)?
> 
> Wrt the lists I think we should first untangle the execlit/request story,
> since I expect that a few of them need to be moved to different structs.
> After that it should be a bit clearer what needs to be moved where. And
> then there's also the golden context and render wa init code which also
> needs to be shuffled a bit (similar to this series).
> -Daniel

Right, this patch looks perfectly sensible :)

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

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