diff mbox

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

Message ID 1353581490-5822-5-git-send-email-mika.kuoppala@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mika Kuoppala Nov. 22, 2012, 10:51 a.m. UTC
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(-)

Comments

Chris Wilson Nov. 22, 2012, 11:13 a.m. UTC | #1
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
Mika Kuoppala Nov. 22, 2012, 11:59 a.m. UTC | #2
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
diff mbox

Patch

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);