[PATCH 1/3] drm/i915: i915_gem_object_sync must handle NULL

[PATCH 1/3] drm/i915: i915_gem_object_sync must handle NULL

[Ben Widawsky]

When I extracted the synchronization code for implementing semaphorified
pageflips (74f5f6e0), I neglected the non pipelined case which also
calls this code. The modesetting code wants to make sure the object has
finished rendering to the frame before configuring the scanout (ie.
non-pipelined case).

As a result of a follow on discussion on IRC, I've decided to add a
comment about the function itself which received much inspiration from
Chris as well. So really, this patch was ghost-written by Chris :).

Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1bfb0d2..9fcdc9a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1953,6 +1953,18 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
+/**
+ * i915_gem_object_sync - sync an object to a ring.
+ *
+ * @obj: object which may be in use on another ring.
+ * @to: ring we wish to use the object on. May be NULL.
+ *
+ * This code is meant to abstract object synchronization with the GPU.
+ * Calling with NULL implies synchronizing the object with the CPU
+ * rather than a particular GPU ring.
+ *
+ * Returns 0 if successful, else propagates up the lower layer error.
+ */
 int
 i915_gem_object_sync(struct drm_i915_gem_object *obj,
 		     struct intel_ring_buffer *to)
@@ -1964,7 +1976,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
 	if (from == NULL || to == from)
 		return 0;
 
-	if (!i915_semaphore_is_enabled(obj->base.dev))
+	if (to == NULL || !i915_semaphore_is_enabled(obj->base.dev))
 		return i915_gem_object_wait_rendering(obj);
 
 	idx = intel_ring_sync_index(from, to);
-- 
1.7.10

[PATCH 2/3] drm/i915: fix for when semaphore updates fail

[Ben Widawsky]

This fixes a long standing issue where emitting the semaphore updates
may have failed, but we've already updated our internal data structure.

Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9fcdc9a..0115b12 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2001,10 +2001,12 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
 		seqno = request->seqno;
 	}
 
-	from->sync_seqno[idx] = seqno;
 
-	return to->sync_to(to, from, seqno - 1);
+	ret = to->sync_to(to, from, seqno - 1);
+	if (!ret)
+		from->sync_seqno[idx] = seqno;
 
+	return ret;
 }
 
 static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
-- 
1.7.10

[PATCH 3/3] drm/i915: hide (seqno-1) in ringbuffer code

[Ben Widawsky]

Waiting for seqno-1 in our object synchronization code is an
implementation detail given how we've decided to do the waits within the
rest of our code.

Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |    2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |    6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0115b12..71934dd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2002,7 +2002,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
 	}
 
 
-	ret = to->sync_to(to, from, seqno - 1);
+	ret = to->sync_to(to, from, seqno);
 	if (!ret)
 		from->sync_seqno[idx] = seqno;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index dfdb613..467b331 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -482,6 +482,12 @@ intel_ring_sync(struct intel_ring_buffer *waiter,
 		  MI_SEMAPHORE_COMPARE |
 		  MI_SEMAPHORE_REGISTER;
 
+	/* Throughout all of the GEM code, seqno passed implies our current
+	 * seqno is >= the last seqno executed. However for hardware the
+	 * comparison is strictly greater than.
+	 */
+	seqno -= 1;
+
 	ret = intel_ring_begin(waiter, 4);
 	if (ret)
 		return ret;
-- 
1.7.10

Re: [PATCH 1/3] drm/i915: i915_gem_object_sync must handle NULL

[Chris Wilson]

On Wed, 11 Apr 2012 11:18:19 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> When I extracted the synchronization code for implementing semaphorified
> pageflips (74f5f6e0), I neglected the non pipelined case which also
> calls this code. The modesetting code wants to make sure the object has
> finished rendering to the frame before configuring the scanout (ie.
> non-pipelined case).
> 
> As a result of a follow on discussion on IRC, I've decided to add a
> comment about the function itself which received much inspiration from
> Chris as well. So really, this patch was ghost-written by Chris :).
> 
> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/3] drm/i915: fix for when semaphore updates fail

[Chris Wilson]

On Wed, 11 Apr 2012 11:18:20 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> This fixes a long standing issue where emitting the semaphore updates
> may have failed, but we've already updated our internal data structure.
> 
> Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>

I would have written it as ret == 0, but since I didn't write this,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Fortunately the impact of the bug would have been render corruption and
not a GPU hang.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: hide (seqno-1) in ringbuffer code

[Chris Wilson]

On Wed, 11 Apr 2012 11:18:21 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Waiting for seqno-1 in our object synchronization code is an
> implementation detail given how we've decided to do the waits within the
> rest of our code.
> 
> Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>

Thanks for the comment, I had forgotten about that hw detail.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: i915_gem_object_sync must handle NULL

[Daniel Vetter]

On Wed, Apr 11, 2012 at 07:41:04PM +0100, Chris Wilson wrote:
> On Wed, 11 Apr 2012 11:18:19 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > When I extracted the synchronization code for implementing semaphorified
> > pageflips (74f5f6e0), I neglected the non pipelined case which also
> > calls this code. The modesetting code wants to make sure the object has
> > finished rendering to the frame before configuring the scanout (ie.
> > non-pipelined case).
> > 
> > As a result of a follow on discussion on IRC, I've decided to add a
> > comment about the function itself which received much inspiration from
> > Chris as well. So really, this patch was ghost-written by Chris :).
> > 
> > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
I've picked up all 3 patches for -next, thanks for them.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48