[PATCH 0/5] Wrap handling / seqno allocations

[PATCH 0/5] Wrap handling / seqno allocations

[Mika Kuoppala]

Hi all,

This patchset tries to address the seqno wrapping issues in driver. 
When seqnos wrap, all requests need to be retired and this can fail.
Thus seqno_allocation is needed. We pre allocate seqnos prior to
pushing stuff into rings. Patch from Chris also avoids allocation
of seqno's when gpu_idle() is called for retiring. Without this
we would recurse back into next_request_seqno().

Comments, flames and reviews are all welcome.

Chris Wilson (1):
  drm/i915: Wait upon the last request seqno, rather than a future
    seqno

Mika Kuoppala (4):
  drm/i915: fix debugfs seqno info print to use uint
  drm/i915: Add i915_gem_alloc_seqno
  drm/i915: Allocate seqno conditionally in i915_add_request
  drm/i915: don't return seqno from ring->add_request()

 drivers/gpu/drm/i915/i915_debugfs.c        |    2 +-
 drivers/gpu/drm/i915/i915_drv.h            |    1 +
 drivers/gpu/drm/i915/i915_gem.c            |  102 ++++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   29 +++-----
 drivers/gpu/drm/i915/intel_ringbuffer.c    |   18 ++---
 drivers/gpu/drm/i915/intel_ringbuffer.h    |    2 +-
 6 files changed, 108 insertions(+), 46 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] drm/i915: Wait upon the last request seqno, rather than a future seqno

[Mika Kuoppala]

From: Chris Wilson <chris@chris-wilson.co.uk>

In commit 69c2fc891343cb5217c866d10709343cff190bdc
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Jul 20 12:41:03 2012 +0100

    drm/i915: Remove the per-ring write list

the explicit flush was removed from i915_ring_idle(). However, we
continued to wait upon the next seqno which now did not correspond to
any request (except for the unusual condition of a failure to queue a
request after execbuffer) and so would wait indefinitely.

This also prevents nesting into next_seqno during i915_gpu_idle
when seqno wrap is about to happen.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 107f09b..7e17382 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2471,10 +2471,29 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
 
 static int i915_ring_idle(struct intel_ring_buffer *ring)
 {
-	if (list_empty(&ring->active_list))
+	u32 seqno;
+	int ret;
+
+	/* We need to add any requests required to flush the objects */
+	if (!list_empty(&ring->active_list)) {
+		seqno = list_entry(ring->active_list.prev,
+				   struct drm_i915_gem_object,
+				   ring_list)->last_read_seqno;
+
+		ret = i915_gem_check_olr(ring, seqno);
+		if (ret)
+			return ret;
+	}
+
+	/* Wait upon the last request to be completed */
+	if (list_empty(&ring->request_list))
 		return 0;
 
-	return i915_wait_seqno(ring, i915_gem_next_request_seqno(ring));
+	seqno = list_entry(ring->request_list.prev,
+			   struct drm_i915_gem_request,
+			   list)->seqno;
+
+	return i915_wait_seqno(ring, seqno);
 }
 
 int i915_gpu_idle(struct drm_device *dev)
-- 
1.7.9.5

[PATCH 2/5] drm/i915: fix debugfs seqno info print to use uint

[Mika Kuoppala]

seqno's are u32 so print accordingly

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index dde8b50..589fd84 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -387,7 +387,7 @@ static void i915_ring_seqno_info(struct seq_file *m,
 				 struct intel_ring_buffer *ring)
 {
 	if (ring->get_seqno) {
-		seq_printf(m, "Current sequence (%s): %d\n",
+		seq_printf(m, "Current sequence (%s): %u\n",
 			   ring->name, ring->get_seqno(ring, false));
 	}
 }
-- 
1.7.9.5

[PATCH 3/5] drm/i915: Add i915_gem_alloc_seqno

[Mika Kuoppala]

Hardware can't handle seqno wrapping so all previous requests need to
be retired before seqno can wrap. As this can fail, make seqno
allocation explicit by introducing i915_gem_alloc_seqno()

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_gem.c |   51 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f511fa2..598f3b8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1387,6 +1387,7 @@ i915_seqno_passed(uint32_t seq1, uint32_t seq2)
 	return (int32_t)(seq1 - seq2) >= 0;
 }
 
+int i915_gem_alloc_seqno(struct intel_ring_buffer *ring, u32 *seqno);
 u32 i915_gem_next_request_seqno(struct intel_ring_buffer *ring);
 
 int __must_check i915_gem_object_get_fence(struct drm_i915_gem_object *obj);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7e17382..a637d5d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1933,6 +1933,34 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 	WARN_ON(i915_verify_lists(dev));
 }
 
+static int
+i915_gem_handle_seqno_wrap(struct drm_device *dev)
+{
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
+	int ret;
+	int i;
+
+	/* The GPU can not handle its semaphore value wrapping,
+	   +	 * so every billion or so execbuffers, we need to stall
+	   * the GPU in order to reset the counters.
+	   */
+
+	ret = i915_gpu_idle(dev);
+	if (ret)
+		return ret;
+
+	i915_gem_retire_requests(dev);
+
+	for_each_ring(ring, dev_priv, i) {
+		int j;
+		for (j = 0; j < ARRAY_SIZE(ring->sync_seqno); j++)
+			BUG_ON(ring->sync_seqno[j]);
+	}
+
+	return 0;
+}
+
 static u32
 i915_gem_get_seqno(struct drm_device *dev)
 {
@@ -1956,6 +1984,29 @@ i915_gem_next_request_seqno(struct intel_ring_buffer *ring)
 }
 
 int
+i915_gem_alloc_seqno(struct intel_ring_buffer *ring, u32 *seqno)
+{
+	drm_i915_private_t *dev_priv = ring->dev->dev_private;
+	int ret;
+
+	/* reserve 0 for non-seqno */
+	if (unlikely(dev_priv->next_seqno == ~0)) {
+		ret = i915_gem_handle_seqno_wrap(ring->dev);
+		if (ret)
+			return ret;
+
+		dev_priv->next_seqno = 1;
+	}
+
+	dev_priv->next_seqno++;
+
+	if (seqno)
+		*seqno = i915_gem_next_request_seqno(ring);
+
+	return 0;
+}
+
+int
 i915_add_request(struct intel_ring_buffer *ring,
 		 struct drm_file *file,
 		 u32 *out_seqno)
-- 
1.7.9.5

[PATCH 4/5] drm/i915: Allocate seqno conditionally in i915_add_request

[Mika Kuoppala]

If seqno already exists, add request with that seqno. Otherwise
allocate new seqno for this request. Make i915_gem_check_olr use
already allocated seqnos in order to avoid nesting into
i915_gem_alloc_seqno during i915_gem_idle().

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |   28 ++++++++++++++++-----------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   29 ++++++++++------------------
 2 files changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a637d5d..858f14f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -981,7 +981,7 @@ i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno)
 
 	ret = 0;
 	if (seqno == ring->outstanding_lazy_request)
-		ret = i915_add_request(ring, NULL, NULL);
+		ret = i915_add_request(ring, NULL, &seqno);
 
 	return ret;
 }
@@ -1942,7 +1942,7 @@ i915_gem_handle_seqno_wrap(struct drm_device *dev)
 	int i;
 
 	/* The GPU can not handle its semaphore value wrapping,
-	   +	 * so every billion or so execbuffers, we need to stall
+	   * so every billion or so execbuffers, we need to stall
 	   * the GPU in order to reset the counters.
 	   */
 
@@ -1967,10 +1967,6 @@ i915_gem_get_seqno(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u32 seqno = dev_priv->next_seqno;
 
-	/* reserve 0 for non-seqno */
-	if (++dev_priv->next_seqno == 0)
-		dev_priv->next_seqno = 1;
-
 	return seqno;
 }
 
@@ -2009,7 +2005,7 @@ i915_gem_alloc_seqno(struct intel_ring_buffer *ring, u32 *seqno)
 int
 i915_add_request(struct intel_ring_buffer *ring,
 		 struct drm_file *file,
-		 u32 *out_seqno)
+		 u32 *req_seqno)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
 	struct drm_i915_gem_request *request;
@@ -2029,12 +2025,21 @@ i915_add_request(struct intel_ring_buffer *ring,
 	if (ret)
 		return ret;
 
+	if (req_seqno)
+		seqno = *req_seqno;
+	else
+		seqno = 0;
+
+	if (seqno == 0) {
+		ret = i915_gem_alloc_seqno(ring, &seqno);
+		if (ret)
+			return ret;
+	}
+
 	request = kmalloc(sizeof(*request), GFP_KERNEL);
 	if (request == NULL)
 		return -ENOMEM;
 
-	seqno = i915_gem_next_request_seqno(ring);
-
 	/* Record the position of the start of the request so that
 	 * should we detect the updated seqno part-way through the
 	 * GPU processing the request, we never over-estimate the
@@ -2083,8 +2088,9 @@ i915_add_request(struct intel_ring_buffer *ring,
 		}
 	}
 
-	if (out_seqno)
-		*out_seqno = seqno;
+	if (req_seqno)
+		*req_seqno = seqno;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3eea143..07582f9 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -750,13 +750,14 @@ i915_gem_execbuffer_move_to_active(struct list_head *objects,
 static void
 i915_gem_execbuffer_retire_commands(struct drm_device *dev,
 				    struct drm_file *file,
-				    struct intel_ring_buffer *ring)
+				    struct intel_ring_buffer *ring,
+				    u32 seqno)
 {
 	/* Unconditionally force add_request to emit a full flush. */
 	ring->gpu_caches_dirty = true;
 
 	/* Add a breadcrumb for the completion of the batch buffer */
-	(void)i915_add_request(ring, file, NULL);
+	(void)i915_add_request(ring, file, &seqno);
 }
 
 static int
@@ -910,6 +911,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (ret)
 		goto pre_mutex_err;
 
+	ret = i915_gem_alloc_seqno(ring, &seqno);
+	if (ret) {
+		mutex_unlock(&dev->struct_mutex);
+		goto pre_mutex_err;
+	}
+
 	if (dev_priv->mm.suspended) {
 		mutex_unlock(&dev->struct_mutex);
 		ret = -EBUSY;
@@ -987,22 +994,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (ret)
 		goto err;
 
-	seqno = i915_gem_next_request_seqno(ring);
-	for (i = 0; i < ARRAY_SIZE(ring->sync_seqno); i++) {
-		if (seqno < ring->sync_seqno[i]) {
-			/* The GPU can not handle its semaphore value wrapping,
-			 * so every billion or so execbuffers, we need to stall
-			 * the GPU in order to reset the counters.
-			 */
-			ret = i915_gpu_idle(dev);
-			if (ret)
-				goto err;
-			i915_gem_retire_requests(dev);
-
-			BUG_ON(ring->sync_seqno[i]);
-		}
-	}
-
 	ret = i915_switch_context(ring, file, ctx_id);
 	if (ret)
 		goto err;
@@ -1051,7 +1042,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	}
 
 	i915_gem_execbuffer_move_to_active(&objects, ring, seqno);
-	i915_gem_execbuffer_retire_commands(dev, file, ring);
+	i915_gem_execbuffer_retire_commands(dev, file, ring, seqno);
 
 err:
 	eb_destroy(eb);
-- 
1.7.9.5

Re: [PATCH 4/5] drm/i915: Allocate seqno conditionally in i915_add_request

[Chris Wilson]

On Thu, 22 Nov 2012 12:51:29 +0200, Mika Kuoppala <mika.kuoppala@intel.com> wrote:
> If seqno already exists, add request with that seqno. Otherwise
> allocate new seqno for this request. Make i915_gem_check_olr use
> already allocated seqnos in order to avoid nesting into
> i915_gem_alloc_seqno during i915_gem_idle().

I really don't like that mess in i915_add_request(). I was thinking more
alongthe lines of:

http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=bug55984&id=87cf35086182f9ce36bb8d85a0fe4cfc136cb3e2
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/5] drm/i915: Allocate seqno conditionally in i915_add_request

[Mika Kuoppala]

On Thu, 22 Nov 2012 11:13:04 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, 22 Nov 2012 12:51:29 +0200, Mika Kuoppala <mika.kuoppala@intel.com> wrote:
> > If seqno already exists, add request with that seqno. Otherwise
> > allocate new seqno for this request. Make i915_gem_check_olr use
> > already allocated seqnos in order to avoid nesting into
> > i915_gem_alloc_seqno during i915_gem_idle().
> 
> I really don't like that mess in i915_add_request(). I was thinking more
> alongthe lines of:
> 
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=bug55984&id=87cf35086182f9ce36bb8d85a0fe4cfc136cb3e2
> -Chris

Yours look much better. I got distracted by intel_ring_begin can't fail
in do_execbuffer.

With quick testing this also can survive hundreds of wraps.

Please send yours for review.

Thanks,
-Mika

> -- 
> 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 5/5] drm/i915: don't return seqno from ring->add_request()

[Mika Kuoppala]

Sequence numbers are preallocated so make them immutable
in ring->add_request().

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |    2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |   18 ++++++------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    2 +-
 3 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 858f14f..d95c34d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2047,7 +2047,7 @@ i915_add_request(struct intel_ring_buffer *ring,
 	 */
 	request_ring_position = intel_ring_get_tail(ring);
 
-	ret = ring->add_request(ring, &seqno);
+	ret = ring->add_request(ring, seqno);
 	if (ret) {
 		kfree(request);
 		return ret;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ecbc5c5..44aa76f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -577,7 +577,7 @@ update_mboxes(struct intel_ring_buffer *ring,
  */
 static int
 gen6_add_request(struct intel_ring_buffer *ring,
-		 u32 *seqno)
+		 u32 seqno)
 {
 	u32 mbox1_reg;
 	u32 mbox2_reg;
@@ -590,13 +590,11 @@ gen6_add_request(struct intel_ring_buffer *ring,
 	mbox1_reg = ring->signal_mbox[0];
 	mbox2_reg = ring->signal_mbox[1];
 
-	*seqno = i915_gem_next_request_seqno(ring);
-
-	update_mboxes(ring, *seqno, mbox1_reg);
-	update_mboxes(ring, *seqno, mbox2_reg);
+	update_mboxes(ring, seqno, mbox1_reg);
+	update_mboxes(ring, seqno, mbox2_reg);
 	intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
 	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
-	intel_ring_emit(ring, *seqno);
+	intel_ring_emit(ring, seqno);
 	intel_ring_emit(ring, MI_USER_INTERRUPT);
 	intel_ring_advance(ring);
 
@@ -654,9 +652,8 @@ do {									\
 
 static int
 pc_render_add_request(struct intel_ring_buffer *ring,
-		      u32 *result)
+		      u32 seqno)
 {
-	u32 seqno = i915_gem_next_request_seqno(ring);
 	struct pipe_control *pc = ring->private;
 	u32 scratch_addr = pc->gtt_offset + 128;
 	int ret;
@@ -700,7 +697,6 @@ pc_render_add_request(struct intel_ring_buffer *ring,
 	intel_ring_emit(ring, 0);
 	intel_ring_advance(ring);
 
-	*result = seqno;
 	return 0;
 }
 
@@ -889,9 +885,8 @@ bsd_ring_flush(struct intel_ring_buffer *ring,
 
 static int
 i9xx_add_request(struct intel_ring_buffer *ring,
-		 u32 *result)
+		 u32 seqno)
 {
-	u32 seqno;
 	int ret;
 
 	ret = intel_ring_begin(ring, 4);
@@ -906,7 +901,6 @@ i9xx_add_request(struct intel_ring_buffer *ring,
 	intel_ring_emit(ring, MI_USER_INTERRUPT);
 	intel_ring_advance(ring);
 
-	*result = seqno;
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 2ea7a31..66b9a3b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -71,7 +71,7 @@ struct  intel_ring_buffer {
 				  u32	invalidate_domains,
 				  u32	flush_domains);
 	int		(*add_request)(struct intel_ring_buffer *ring,
-				       u32 *seqno);
+				       u32 seqno);
 	/* Some chipsets are not quite as coherent as advertised and need
 	 * an expensive kick to force a true read of the up-to-date seqno.
 	 * However, the up-to-date seqno is not always required and the last
-- 
1.7.9.5