[PATCH 0/3] a few small cleanups in intel_ringbuffer.c

[PATCH 0/3] a few small cleanups in intel_ringbuffer.c

[Daniel Vetter]

Hi all,

While mucking around with gfdt to check whether the llc api feels good (it
does!) I've once again noticed a few things in intel_ringbuffer.c

These patches weed them out. Please review and consider merging for -next.

Thanks, Daniel

Daniel Vetter (3):
  drm/i915/ringbuffer: kill snb blt workaround
  drm/i915/ringbuffer: kill pipe_control->gtt_offset
  drm/i915/ringbuffer: embed pipe_control into struct ring_buffer

 drivers/gpu/drm/i915/intel_ringbuffer.c |  146 +++++--------------------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    9 ++-
 2 files changed, 27 insertions(+), 128 deletions(-)

-- 
1.7.4.2

[PATCH 1/3] drm/i915/ringbuffer: kill snb blt workaround

[Daniel Vetter]

This was just to facilitate product enablement with pre-production hw.
Allows us to kill quite a bit of cruft.

Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   83 +------------------------------
 1 files changed, 2 insertions(+), 81 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index f15d80f..afb2a34 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -887,9 +887,6 @@ void intel_cleanup_ring_buffer(struct intel_ring_buffer *ring)
 	drm_gem_object_unreference(&ring->obj->base);
 	ring->obj = NULL;
 
-	if (ring->cleanup)
-		ring->cleanup(ring);
-
 	cleanup_status_page(ring);
 }
 
@@ -1153,78 +1150,13 @@ blt_ring_put_irq(struct intel_ring_buffer *ring)
 }
 
 
-/* Workaround for some stepping of SNB,
- * each time when BLT engine ring tail moved,
- * the first command in the ring to be parsed
- * should be MI_BATCH_BUFFER_START
- */
-#define NEED_BLT_WORKAROUND(dev) \
-	(IS_GEN6(dev) && (dev->pdev->revision < 8))
-
-static inline struct drm_i915_gem_object *
-to_blt_workaround(struct intel_ring_buffer *ring)
-{
-	return ring->private;
-}
-
-static int blt_ring_init(struct intel_ring_buffer *ring)
-{
-	if (NEED_BLT_WORKAROUND(ring->dev)) {
-		struct drm_i915_gem_object *obj;
-		u32 *ptr;
-		int ret;
-
-		obj = i915_gem_alloc_object(ring->dev, 4096);
-		if (obj == NULL)
-			return -ENOMEM;
-
-		ret = i915_gem_object_pin(obj, 4096, true);
-		if (ret) {
-			drm_gem_object_unreference(&obj->base);
-			return ret;
-		}
-
-		ptr = kmap(obj->pages[0]);
-		*ptr++ = MI_BATCH_BUFFER_END;
-		*ptr++ = MI_NOOP;
-		kunmap(obj->pages[0]);
-
-		ret = i915_gem_object_set_to_gtt_domain(obj, false);
-		if (ret) {
-			i915_gem_object_unpin(obj);
-			drm_gem_object_unreference(&obj->base);
-			return ret;
-		}
-
-		ring->private = obj;
-	}
-
-	return init_ring_common(ring);
-}
-
-static int blt_ring_begin(struct intel_ring_buffer *ring,
-			  int num_dwords)
-{
-	if (ring->private) {
-		int ret = intel_ring_begin(ring, num_dwords+2);
-		if (ret)
-			return ret;
-
-		intel_ring_emit(ring, MI_BATCH_BUFFER_START);
-		intel_ring_emit(ring, to_blt_workaround(ring)->gtt_offset);
-
-		return 0;
-	} else
-		return intel_ring_begin(ring, 4);
-}
-
 static int blt_ring_flush(struct intel_ring_buffer *ring,
 			  u32 invalidate, u32 flush)
 {
 	uint32_t cmd;
 	int ret;
 
-	ret = blt_ring_begin(ring, 4);
+	ret = intel_ring_begin(ring, 4);
 	if (ret)
 		return ret;
 
@@ -1239,22 +1171,12 @@ static int blt_ring_flush(struct intel_ring_buffer *ring,
 	return 0;
 }
 
-static void blt_ring_cleanup(struct intel_ring_buffer *ring)
-{
-	if (!ring->private)
-		return;
-
-	i915_gem_object_unpin(ring->private);
-	drm_gem_object_unreference(ring->private);
-	ring->private = NULL;
-}
-
 static const struct intel_ring_buffer gen6_blt_ring = {
        .name			= "blt ring",
        .id			= RING_BLT,
        .mmio_base		= BLT_RING_BASE,
        .size			= 32 * PAGE_SIZE,
-       .init			= blt_ring_init,
+       .init			= init_ring_common,
        .write_tail		= ring_write_tail,
        .flush			= blt_ring_flush,
        .add_request		= gen6_add_request,
@@ -1262,7 +1184,6 @@ static const struct intel_ring_buffer gen6_blt_ring = {
        .irq_get			= blt_ring_get_irq,
        .irq_put			= blt_ring_put_irq,
        .dispatch_execbuffer	= gen6_ring_dispatch_execbuffer,
-       .cleanup			= blt_ring_cleanup,
 };
 
 int intel_init_render_ring_buffer(struct drm_device *dev)
-- 
1.7.4.2

Re: [PATCH 1/3] drm/i915/ringbuffer: kill snb blt workaround

[Daniel Vetter]

Bleh, I've fat-fingered the fdo address on the first submission, to
which Eric replied.
Pasting his response below:

On Wed, 27 Apr 2011 22:33:35 +0200, Daniel Vetter
<daniel.vetter@ffwll.ch> wrote:
> This was just to facilitate product enablement with pre-production hw.
> Allows us to kill quite a bit of cruft.
>
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Eric Anholt <eric@anholt.net>
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

[PATCH 2/3] drm/i915/ringbuffer: kill pipe_control->gtt_offset

[Daniel Vetter]

With the snb blt workaround gone, gen5 pipe_control is the only user of
ring->private. It's already tiny, but make it even smaller to prepare for
embedding. The added indirection will be killed in the next patch.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index afb2a34..c73410f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -213,7 +213,6 @@ static int init_ring_common(struct intel_ring_buffer *ring)
 struct pipe_control {
 	struct drm_i915_gem_object *obj;
 	volatile u32 *cpu_page;
-	u32 gtt_offset;
 };
 
 static int
@@ -243,7 +242,6 @@ init_pipe_control(struct intel_ring_buffer *ring)
 	if (ret)
 		goto err_unref;
 
-	pc->gtt_offset = obj->gtt_offset;
 	pc->cpu_page =  kmap(obj->pages[0]);
 	if (pc->cpu_page == NULL)
 		goto err_unpin;
@@ -400,7 +398,7 @@ pc_render_add_request(struct intel_ring_buffer *ring,
 	struct drm_device *dev = ring->dev;
 	u32 seqno = i915_gem_get_seqno(dev);
 	struct pipe_control *pc = ring->private;
-	u32 scratch_addr = pc->gtt_offset + 128;
+	u32 scratch_addr = pc->obj->gtt_offset + 128;
 	int ret;
 
 	/* For Ironlake, MI_USER_INTERRUPT was deprecated and apparently
@@ -417,7 +415,7 @@ pc_render_add_request(struct intel_ring_buffer *ring,
 
 	intel_ring_emit(ring, GFX_OP_PIPE_CONTROL | PIPE_CONTROL_QW_WRITE |
 			PIPE_CONTROL_WC_FLUSH | PIPE_CONTROL_TC_FLUSH);
-	intel_ring_emit(ring, pc->gtt_offset | PIPE_CONTROL_GLOBAL_GTT);
+	intel_ring_emit(ring, pc->obj->gtt_offset | PIPE_CONTROL_GLOBAL_GTT);
 	intel_ring_emit(ring, seqno);
 	intel_ring_emit(ring, 0);
 	PIPE_CONTROL_FLUSH(ring, scratch_addr);
@@ -434,7 +432,7 @@ pc_render_add_request(struct intel_ring_buffer *ring,
 	intel_ring_emit(ring, GFX_OP_PIPE_CONTROL | PIPE_CONTROL_QW_WRITE |
 			PIPE_CONTROL_WC_FLUSH | PIPE_CONTROL_TC_FLUSH |
 			PIPE_CONTROL_NOTIFY);
-	intel_ring_emit(ring, pc->gtt_offset | PIPE_CONTROL_GLOBAL_GTT);
+	intel_ring_emit(ring, pc->obj->gtt_offset | PIPE_CONTROL_GLOBAL_GTT);
 	intel_ring_emit(ring, seqno);
 	intel_ring_emit(ring, 0);
 	intel_ring_advance(ring);
-- 
1.7.4.2

[PATCH 3/3] drm/i915/ringbuffer: embed pipe_control into struct ring_buffer

[Daniel Vetter]

Pipecontrol is also required to implement gfdt flushing on gen6+.
And if we ever switch to pipecontrol based cache management in the
kernel, also required on gen4. I don't see the point in saving that
little bit of storge.

In the process make cleanup_pipe_control more robust and call it
unconditionally.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   55 +++++++++----------------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    9 +++--
 2 files changed, 22 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c73410f..0160508 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -210,24 +210,17 @@ static int init_ring_common(struct intel_ring_buffer *ring)
  * 965+ support PIPE_CONTROL commands, which provide finer grained control
  * over cache flushing.
  */
-struct pipe_control {
-	struct drm_i915_gem_object *obj;
-	volatile u32 *cpu_page;
-};
-
 static int
 init_pipe_control(struct intel_ring_buffer *ring)
 {
-	struct pipe_control *pc;
+	struct intel_pipe_control *pc;
 	struct drm_i915_gem_object *obj;
 	int ret;
 
-	if (ring->private)
-		return 0;
+	pc = &ring->pipe_control;
 
-	pc = kmalloc(sizeof(*pc), GFP_KERNEL);
-	if (!pc)
-		return -ENOMEM;
+	if (pc->cpu_page)
+		return 0;
 
 	obj = i915_gem_alloc_object(ring->dev, 4096);
 	if (obj == NULL) {
@@ -240,41 +233,34 @@ init_pipe_control(struct intel_ring_buffer *ring)
 
 	ret = i915_gem_object_pin(obj, 4096, true);
 	if (ret)
-		goto err_unref;
+		goto err;
 
 	pc->cpu_page =  kmap(obj->pages[0]);
 	if (pc->cpu_page == NULL)
 		goto err_unpin;
 
 	pc->obj = obj;
-	ring->private = pc;
 	return 0;
 
 err_unpin:
 	i915_gem_object_unpin(obj);
-err_unref:
-	drm_gem_object_unreference(&obj->base);
 err:
-	kfree(pc);
+	drm_gem_object_unreference(&obj->base);
 	return ret;
 }
 
 static void
 cleanup_pipe_control(struct intel_ring_buffer *ring)
 {
-	struct pipe_control *pc = ring->private;
+	struct intel_pipe_control *pc = &ring->pipe_control;
 	struct drm_i915_gem_object *obj;
 
-	if (!ring->private)
-		return;
-
-	obj = pc->obj;
-	kunmap(obj->pages[0]);
-	i915_gem_object_unpin(obj);
-	drm_gem_object_unreference(&obj->base);
-
-	kfree(pc);
-	ring->private = NULL;
+	if (pc->cpu_page)
+		kunmap(obj->pages[0]);
+	if (pc->obj) {
+		i915_gem_object_unpin(pc->obj);
+		drm_gem_object_unreference(&pc->obj->base);
+	}
 }
 
 static int init_render_ring(struct intel_ring_buffer *ring)
@@ -300,14 +286,6 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 	return ret;
 }
 
-static void render_ring_cleanup(struct intel_ring_buffer *ring)
-{
-	if (!ring->private)
-		return;
-
-	cleanup_pipe_control(ring);
-}
-
 static void
 update_semaphore(struct intel_ring_buffer *ring, int i, u32 seqno)
 {
@@ -397,7 +375,7 @@ pc_render_add_request(struct intel_ring_buffer *ring,
 {
 	struct drm_device *dev = ring->dev;
 	u32 seqno = i915_gem_get_seqno(dev);
-	struct pipe_control *pc = ring->private;
+	struct intel_pipe_control *pc = &ring->pipe_control;
 	u32 scratch_addr = pc->obj->gtt_offset + 128;
 	int ret;
 
@@ -472,8 +450,7 @@ ring_get_seqno(struct intel_ring_buffer *ring)
 static u32
 pc_render_get_seqno(struct intel_ring_buffer *ring)
 {
-	struct pipe_control *pc = ring->private;
-	return pc->cpu_page[0];
+	return ring->pipe_control.cpu_page[0];
 }
 
 static void
@@ -886,6 +863,7 @@ void intel_cleanup_ring_buffer(struct intel_ring_buffer *ring)
 	ring->obj = NULL;
 
 	cleanup_status_page(ring);
+	cleanup_pipe_control(ring);
 }
 
 static int intel_wrap_ring_buffer(struct intel_ring_buffer *ring)
@@ -999,7 +977,6 @@ static const struct intel_ring_buffer render_ring = {
 	.irq_get		= render_ring_get_irq,
 	.irq_put		= render_ring_put_irq,
 	.dispatch_execbuffer	= render_ring_dispatch_execbuffer,
-       .cleanup			= render_ring_cleanup,
 };
 
 /* ring buffer for bit-stream decoder */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 16cb125..6cddcfa 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -14,6 +14,11 @@ struct  intel_hw_status_page {
 	struct		drm_i915_gem_object *obj;
 };
 
+struct intel_pipe_control {
+	struct drm_i915_gem_object *obj;
+	volatile u32 *cpu_page;
+};
+
 #define I915_RING_READ(reg) i915_gt_read(dev_priv, reg)
 #define I915_RING_WRITE(reg, val) i915_gt_write(dev_priv, reg, val)
 
@@ -54,6 +59,7 @@ struct  intel_ring_buffer {
 	int		size;
 	int		effective_size;
 	struct intel_hw_status_page status_page;
+	struct intel_pipe_control pipe_control;
 
 	spinlock_t	irq_lock;
 	u32		irq_refcount;
@@ -77,7 +83,6 @@ struct  intel_ring_buffer {
 	u32		(*get_seqno)(struct intel_ring_buffer *ring);
 	int		(*dispatch_execbuffer)(struct intel_ring_buffer *ring,
 					       u32 offset, u32 length);
-	void		(*cleanup)(struct intel_ring_buffer *ring);
 
 	/**
 	 * List of objects currently involved in rendering from the
@@ -113,8 +118,6 @@ struct  intel_ring_buffer {
 
 	wait_queue_head_t irq_queue;
 	drm_local_map_t map;
-
-	void *private;
 };
 
 static inline u32
-- 
1.7.4.2

Re: [PATCH 3/3] drm/i915/ringbuffer: embed pipe_control into struct ring_buffer

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 674 bytes --]

On Wed, 27 Apr 2011 22:46:06 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Pipecontrol is also required to implement gfdt flushing on gen6+.
> And if we ever switch to pipecontrol based cache management in the
> kernel, also required on gen4. I don't see the point in saving that
> little bit of storge.
> 
> In the process make cleanup_pipe_control more robust and call it
> unconditionally.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Nice.  I thought about this when reviewing the first, then decided I
didn't care enough.   But if you've already cleaned it up, great!

these two are:

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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