All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] ring seqno wrap handling
@ 2012-12-04 13:11 Mika Kuoppala
  2012-12-04 13:12 ` [PATCH 1/6] drm/i915: Add debugfs entry to read/write next_seqno Mika Kuoppala
                   ` (6 more replies)
  0 siblings, 7 replies; 15+ messages in thread
From: Mika Kuoppala @ 2012-12-04 13:11 UTC (permalink / raw)
  To: intel-gfx

Hi,

As Ben suggested, explicitly syncing across wrap boundary
revealed yet another set of issues that gem_stress didn't find.

So here is a patchset which addresses the syncing problems
across wrap boundary. Chris helped me to carve out seqno handling
ugliness from my initial attempts to handle the ring wrapping.

As I can't break this anymore, i added 6/6.
--Mika

Mika Kuoppala (6):
  drm/i915: Add debugfs entry to read/write next_seqno
  drm/i915: Fix debugfs seqno info print to use uint
  drm/i915: Don't emit semaphore wait if wrap happened
  drm/i915: Split intel_ring_begin
  drm/i915: Add intel_ring_handle_seqno wrap
  drm/i915: Set initial seqno value close to wrap boundary

 drivers/gpu/drm/i915/i915_debugfs.c     |   88 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem.c         |    6 ++-
 drivers/gpu/drm/i915/intel_ringbuffer.c |   72 +++++++++++++++++++------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    2 +-
 4 files changed, 148 insertions(+), 20 deletions(-)

-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 15+ messages in thread
* [PATCH 1/6] drm/i915: Add debugfs entry to read/write next_seqno
  2012-12-04 13:11 [PATCH 0/6] ring seqno wrap handling Mika Kuoppala
@ 2012-12-04 13:12 ` Mika Kuoppala
  2012-12-04 13:12 ` [PATCH 2/6] drm/i915: Fix debugfs seqno info print to use uint Mika Kuoppala
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Mika Kuoppala @ 2012-12-04 13:12 UTC (permalink / raw)
  To: intel-gfx

This is needed for testing seqno wrapping. Be careful to not bump next_seqno
more than 0x7FFFFFFF at a time (between some handled requests) as
i915_seqno_passed() can't handle bigger difference in between.

v2: Address review comments from Chris Wilson.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index ce60506..18de201 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -843,6 +843,86 @@ static const struct file_operations i915_error_state_fops = {
 	.release = i915_error_state_release,
 };
 
+static ssize_t
+i915_next_seqno_read(struct file *filp,
+		 char __user *ubuf,
+		 size_t max,
+		 loff_t *ppos)
+{
+	struct drm_device *dev = filp->private_data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	char buf[80];
+	int len;
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	len = snprintf(buf, sizeof(buf),
+		       "next_seqno :  0x%x\n",
+		       dev_priv->next_seqno);
+
+	mutex_unlock(&dev->struct_mutex);
+
+	if (len > sizeof(buf))
+		len = sizeof(buf);
+
+	return simple_read_from_buffer(ubuf, max, ppos, buf, len);
+}
+
+static ssize_t
+i915_next_seqno_write(struct file *filp,
+		      const char __user *ubuf,
+		      size_t cnt,
+		      loff_t *ppos)
+{
+	struct drm_device *dev = filp->private_data;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+	char buf[20];
+	u32 val = 1;
+	int ret;
+
+	if (cnt > 0) {
+		if (cnt > sizeof(buf) - 1)
+			return -EINVAL;
+
+		if (copy_from_user(buf, ubuf, cnt))
+			return -EFAULT;
+		buf[cnt] = 0;
+
+		ret = kstrtouint(buf, 0, &val);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (val == 0)
+		return -EINVAL;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	if (i915_seqno_passed(val, dev_priv->next_seqno)) {
+		dev_priv->next_seqno = val;
+		DRM_DEBUG_DRIVER("Advancing seqno to %u\n", val);
+	} else {
+		ret = -EINVAL;
+	}
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return ret ?: cnt;
+}
+
+static const struct file_operations i915_next_seqno_fops = {
+	.owner = THIS_MODULE,
+	.open = simple_open,
+	.read = i915_next_seqno_read,
+	.write = i915_next_seqno_write,
+	.llseek = default_llseek,
+};
+
 static int i915_rstdby_delays(struct seq_file *m, void *unused)
 {
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
@@ -2107,6 +2187,12 @@ int i915_debugfs_init(struct drm_minor *minor)
 	if (ret)
 		return ret;
 
+	ret = i915_debugfs_create(minor->debugfs_root, minor,
+				 "i915_next_seqno",
+				 &i915_next_seqno_fops);
+	if (ret)
+		return ret;
+
 	return drm_debugfs_create_files(i915_debugfs_list,
 					I915_DEBUGFS_ENTRIES,
 					minor->debugfs_root, minor);
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 2/6] drm/i915: Fix debugfs seqno info print to use uint
  2012-12-04 13:11 [PATCH 0/6] ring seqno wrap handling Mika Kuoppala
  2012-12-04 13:12 ` [PATCH 1/6] drm/i915: Add debugfs entry to read/write next_seqno Mika Kuoppala
@ 2012-12-04 13:12 ` Mika Kuoppala
  2012-12-04 13:12 ` [PATCH 3/6] drm/i915: Don't emit semaphore wait if wrap happened Mika Kuoppala
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Mika Kuoppala @ 2012-12-04 13:12 UTC (permalink / raw)
  To: intel-gfx

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 18de201..e51b89d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -389,7 +389,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

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 3/6] drm/i915: Don't emit semaphore wait if wrap happened
  2012-12-04 13:11 [PATCH 0/6] ring seqno wrap handling Mika Kuoppala
  2012-12-04 13:12 ` [PATCH 1/6] drm/i915: Add debugfs entry to read/write next_seqno Mika Kuoppala
  2012-12-04 13:12 ` [PATCH 2/6] drm/i915: Fix debugfs seqno info print to use uint Mika Kuoppala
@ 2012-12-04 13:12 ` Mika Kuoppala
  2012-12-05 20:44   ` Paulo Zanoni
  2012-12-06 14:12   ` [PATCH] Detect wraparound using next-seqno rather than waiter->olr Chris Wilson
  2012-12-04 13:12 ` [PATCH 4/6] drm/i915: Split intel_ring_begin Mika Kuoppala
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 15+ messages in thread
From: Mika Kuoppala @ 2012-12-04 13:12 UTC (permalink / raw)
  To: intel-gfx

If wrap just happened we need to prevent emitting waits for
pre wrap values. Detect this and emit no-ops instead.

v2: Use olr > seqno to detect wrap instead of *seqno == 0
as suggested by Chris Wilson.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 36e1e13a..2305af5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -626,11 +626,22 @@ gen6_ring_sync(struct intel_ring_buffer *waiter,
 	if (ret)
 		return ret;
 
-	intel_ring_emit(waiter,
-			dw1 | signaller->semaphore_register[waiter->id]);
-	intel_ring_emit(waiter, seqno);
-	intel_ring_emit(waiter, 0);
-	intel_ring_emit(waiter, MI_NOOP);
+	BUG_ON(!waiter->outstanding_lazy_request);
+
+	/* If seqno wrap happened, omit the wait with no-ops */
+	if (likely(waiter->outstanding_lazy_request > seqno)) {
+		intel_ring_emit(waiter,
+				dw1 |
+				signaller->semaphore_register[waiter->id]);
+		intel_ring_emit(waiter, seqno);
+		intel_ring_emit(waiter, 0);
+		intel_ring_emit(waiter, MI_NOOP);
+	} else {
+		intel_ring_emit(waiter, MI_NOOP);
+		intel_ring_emit(waiter, MI_NOOP);
+		intel_ring_emit(waiter, MI_NOOP);
+		intel_ring_emit(waiter, MI_NOOP);
+	}
 	intel_ring_advance(waiter);
 
 	return 0;
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 4/6] drm/i915: Split intel_ring_begin
  2012-12-04 13:11 [PATCH 0/6] ring seqno wrap handling Mika Kuoppala
                   ` (2 preceding siblings ...)
  2012-12-04 13:12 ` [PATCH 3/6] drm/i915: Don't emit semaphore wait if wrap happened Mika Kuoppala
@ 2012-12-04 13:12 ` Mika Kuoppala
  2012-12-04 13:12 ` [PATCH 5/6] drm/i915: Add intel_ring_handle_seqno wrap Mika Kuoppala
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 15+ messages in thread
From: Mika Kuoppala @ 2012-12-04 13:12 UTC (permalink / raw)
  To: intel-gfx

In preparation for handling ring seqno wrapping, split
intel_ring_begin into helper part which doesn't allocate
seqno.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |   37 ++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2305af5..9342bb2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1374,11 +1374,31 @@ intel_ring_alloc_seqno(struct intel_ring_buffer *ring)
 	return i915_gem_get_seqno(ring->dev, &ring->outstanding_lazy_request);
 }
 
+static int __intel_ring_begin(struct intel_ring_buffer *ring,
+			      int bytes)
+{
+	int ret;
+
+	if (unlikely(ring->tail + bytes > ring->effective_size)) {
+		ret = intel_wrap_ring_buffer(ring);
+		if (unlikely(ret))
+			return ret;
+	}
+
+	if (unlikely(ring->space < bytes)) {
+		ret = ring_wait_for_space(ring, bytes);
+		if (unlikely(ret))
+			return ret;
+	}
+
+	ring->space -= bytes;
+	return 0;
+}
+
 int intel_ring_begin(struct intel_ring_buffer *ring,
 		     int num_dwords)
 {
 	drm_i915_private_t *dev_priv = ring->dev->dev_private;
-	int n = 4*num_dwords;
 	int ret;
 
 	ret = i915_gem_check_wedge(dev_priv, dev_priv->mm.interruptible);
@@ -1390,20 +1410,7 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
 	if (ret)
 		return ret;
 
-	if (unlikely(ring->tail + n > ring->effective_size)) {
-		ret = intel_wrap_ring_buffer(ring);
-		if (unlikely(ret))
-			return ret;
-	}
-
-	if (unlikely(ring->space < n)) {
-		ret = ring_wait_for_space(ring, n);
-		if (unlikely(ret))
-			return ret;
-	}
-
-	ring->space -= n;
-	return 0;
+	return __intel_ring_begin(ring, num_dwords * sizeof(uint32_t));
 }
 
 void intel_ring_advance(struct intel_ring_buffer *ring)
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 5/6] drm/i915: Add intel_ring_handle_seqno wrap
  2012-12-04 13:11 [PATCH 0/6] ring seqno wrap handling Mika Kuoppala
                   ` (3 preceding siblings ...)
  2012-12-04 13:12 ` [PATCH 4/6] drm/i915: Split intel_ring_begin Mika Kuoppala
@ 2012-12-04 13:12 ` Mika Kuoppala
  2012-12-04 13:12 ` [PATCH 6/6] drm/i915: Set initial seqno value close to wrap boundary Mika Kuoppala
  2012-12-04 14:05 ` [PATCH 0/6] ring seqno wrap handling Chris Wilson
  6 siblings, 0 replies; 15+ messages in thread
From: Mika Kuoppala @ 2012-12-04 13:12 UTC (permalink / raw)
  To: intel-gfx

If there are pre-wrap values in semaphore-mbox registers after wrap,
syncing against some after-wrap request will complete immediately.
Fix this by emitting ring commands to set mbox registers to zero
when the wrap happens.

v2: Use __intel_ring_begin to emit ring commands, from
Chris Wilson.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         |    4 ++++
 drivers/gpu/drm/i915/intel_ringbuffer.c |   20 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h |    2 +-
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index cb94144..02d3151 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1952,6 +1952,10 @@ i915_gem_handle_seqno_wrap(struct drm_device *dev)
 
 	i915_gem_retire_requests(dev);
 	for_each_ring(ring, dev_priv, i) {
+		ret = intel_ring_handle_seqno_wrap(ring);
+		if (ret)
+			return ret;
+
 		for (j = 0; j < ARRAY_SIZE(ring->sync_seqno); j++)
 			ring->sync_seqno[j] = 0;
 	}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 9342bb2..23587b2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1413,6 +1413,26 @@ int intel_ring_begin(struct intel_ring_buffer *ring,
 	return __intel_ring_begin(ring, num_dwords * sizeof(uint32_t));
 }
 
+int intel_ring_handle_seqno_wrap(struct intel_ring_buffer *ring)
+{
+	int ret;
+
+	BUG_ON(ring->outstanding_lazy_request);
+
+	if (INTEL_INFO(ring->dev)->gen < 6)
+		return 0;
+
+	ret = __intel_ring_begin(ring, 6 * sizeof(uint32_t));
+	if (ret)
+		return ret;
+
+	update_mboxes(ring, ring->signal_mbox[0]);
+	update_mboxes(ring, ring->signal_mbox[1]);
+	intel_ring_advance(ring);
+
+	return 0;
+}
+
 void intel_ring_advance(struct intel_ring_buffer *ring)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index d4b7416..b4a533e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -196,7 +196,7 @@ static inline void intel_ring_emit(struct intel_ring_buffer *ring,
 }
 void intel_ring_advance(struct intel_ring_buffer *ring);
 int __must_check intel_ring_idle(struct intel_ring_buffer *ring);
-
+int __must_check intel_ring_handle_seqno_wrap(struct intel_ring_buffer *ring);
 int intel_ring_flush_all_caches(struct intel_ring_buffer *ring);
 int intel_ring_invalidate_all_caches(struct intel_ring_buffer *ring);
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 6/6] drm/i915: Set initial seqno value close to wrap boundary
  2012-12-04 13:11 [PATCH 0/6] ring seqno wrap handling Mika Kuoppala
                   ` (4 preceding siblings ...)
  2012-12-04 13:12 ` [PATCH 5/6] drm/i915: Add intel_ring_handle_seqno wrap Mika Kuoppala
@ 2012-12-04 13:12 ` Mika Kuoppala
  2012-12-04 14:05 ` [PATCH 0/6] ring seqno wrap handling Chris Wilson
  6 siblings, 0 replies; 15+ messages in thread
From: Mika Kuoppala @ 2012-12-04 13:12 UTC (permalink / raw)
  To: intel-gfx

To gain confidence in the wrap handling, make it happen quite
soon after the boot.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 02d3151..70bb4e8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3947,7 +3947,7 @@ i915_gem_init_hw(struct drm_device *dev)
 			goto cleanup_bsd_ring;
 	}
 
-	dev_priv->next_seqno = 1;
+	dev_priv->next_seqno = (u32)-1 - 0x1000;
 
 	/*
 	 * XXX: There was some w/a described somewhere suggesting loading
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/6] ring seqno wrap handling
  2012-12-04 13:11 [PATCH 0/6] ring seqno wrap handling Mika Kuoppala
                   ` (5 preceding siblings ...)
  2012-12-04 13:12 ` [PATCH 6/6] drm/i915: Set initial seqno value close to wrap boundary Mika Kuoppala
@ 2012-12-04 14:05 ` Chris Wilson
  2012-12-04 22:03   ` Daniel Vetter
  6 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2012-12-04 14:05 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

On Tue,  4 Dec 2012 15:11:59 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> Hi,
> 
> As Ben suggested, explicitly syncing across wrap boundary
> revealed yet another set of issues that gem_stress didn't find.
> 
> So here is a patchset which addresses the syncing problems
> across wrap boundary. Chris helped me to carve out seqno handling
> ugliness from my initial attempts to handle the ring wrapping.
> 
> As I can't break this anymore, i added 6/6.
> --Mika
> 
> Mika Kuoppala (6):
>   drm/i915: Fix debugfs seqno info print to use uint
>   drm/i915: Don't emit semaphore wait if wrap happened
>   drm/i915: Split intel_ring_begin
>   drm/i915: Add intel_ring_handle_seqno wrap
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

>   drm/i915: Add debugfs entry to read/write next_seqno
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>

>   drm/i915: Set initial seqno value close to wrap boundary
*washes hands*
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 0/6] ring seqno wrap handling
  2012-12-04 14:05 ` [PATCH 0/6] ring seqno wrap handling Chris Wilson
@ 2012-12-04 22:03   ` Daniel Vetter
  0 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2012-12-04 22:03 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Tue, Dec 04, 2012 at 02:05:42PM +0000, Chris Wilson wrote:
> On Tue,  4 Dec 2012 15:11:59 +0200, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> > Hi,
> > 
> > As Ben suggested, explicitly syncing across wrap boundary
> > revealed yet another set of issues that gem_stress didn't find.
> > 
> > So here is a patchset which addresses the syncing problems
> > across wrap boundary. Chris helped me to carve out seqno handling
> > ugliness from my initial attempts to handle the ring wrapping.
> > 
> > As I can't break this anymore, i added 6/6.
> > --Mika
> > 
> > Mika Kuoppala (6):
> >   drm/i915: Fix debugfs seqno info print to use uint
> >   drm/i915: Don't emit semaphore wait if wrap happened
> >   drm/i915: Split intel_ring_begin
> >   drm/i915: Add intel_ring_handle_seqno wrap
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> >   drm/i915: Add debugfs entry to read/write next_seqno
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>

Merged them all, thanks for patches&review.
> 
> >   drm/i915: Set initial seqno value close to wrap boundary
> *washes hands*

I'm feeling stupid/lucky/bold/whatever, also merged.

/me hides

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] drm/i915: Don't emit semaphore wait if wrap happened
  2012-12-04 13:12 ` [PATCH 3/6] drm/i915: Don't emit semaphore wait if wrap happened Mika Kuoppala
@ 2012-12-05 20:44   ` Paulo Zanoni
  2012-12-06  8:51     ` Daniel Vetter
  2012-12-06 14:12   ` [PATCH] Detect wraparound using next-seqno rather than waiter->olr Chris Wilson
  1 sibling, 1 reply; 15+ messages in thread
From: Paulo Zanoni @ 2012-12-05 20:44 UTC (permalink / raw)
  To: Mika Kuoppala; +Cc: intel-gfx

Hi

2012/12/4 Mika Kuoppala <mika.kuoppala@linux.intel.com>:
> If wrap just happened we need to prevent emitting waits for
> pre wrap values. Detect this and emit no-ops instead.
>
> v2: Use olr > seqno to detect wrap instead of *seqno == 0
> as suggested by Chris Wilson.

This commit introduces a bug on Haswell. Now when I'm typing my
password on GDM the screen keeps doing wrong rendering. It "blinks
blue". After logging in I don't see more problems.

>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 36e1e13a..2305af5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -626,11 +626,22 @@ gen6_ring_sync(struct intel_ring_buffer *waiter,
>         if (ret)
>                 return ret;
>
> -       intel_ring_emit(waiter,
> -                       dw1 | signaller->semaphore_register[waiter->id]);
> -       intel_ring_emit(waiter, seqno);
> -       intel_ring_emit(waiter, 0);
> -       intel_ring_emit(waiter, MI_NOOP);
> +       BUG_ON(!waiter->outstanding_lazy_request);
> +
> +       /* If seqno wrap happened, omit the wait with no-ops */
> +       if (likely(waiter->outstanding_lazy_request > seqno)) {
> +               intel_ring_emit(waiter,
> +                               dw1 |
> +                               signaller->semaphore_register[waiter->id]);
> +               intel_ring_emit(waiter, seqno);
> +               intel_ring_emit(waiter, 0);
> +               intel_ring_emit(waiter, MI_NOOP);
> +       } else {
> +               intel_ring_emit(waiter, MI_NOOP);
> +               intel_ring_emit(waiter, MI_NOOP);
> +               intel_ring_emit(waiter, MI_NOOP);
> +               intel_ring_emit(waiter, MI_NOOP);
> +       }
>         intel_ring_advance(waiter);
>
>         return 0;
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] drm/i915: Don't emit semaphore wait if wrap happened
  2012-12-05 20:44   ` Paulo Zanoni
@ 2012-12-06  8:51     ` Daniel Vetter
  2012-12-06 11:41       ` Paulo Zanoni
  0 siblings, 1 reply; 15+ messages in thread
From: Daniel Vetter @ 2012-12-06  8:51 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx

On Wed, Dec 5, 2012 at 9:44 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2012/12/4 Mika Kuoppala <mika.kuoppala@linux.intel.com>:
>> If wrap just happened we need to prevent emitting waits for
>> pre wrap values. Detect this and emit no-ops instead.
>>
>> v2: Use olr > seqno to detect wrap instead of *seqno == 0
>> as suggested by Chris Wilson.
>
> This commit introduces a bug on Haswell. Now when I'm typing my
> password on GDM the screen keeps doing wrong rendering. It "blinks
> blue". After logging in I don't see more prodrm/i915: Set initial seqno value close to wrap boundaryblems.

Just now I've taken out "drm/i915: Set initial seqno value close to
wrap boundary" since QA complained that it regresses things. Does that
help for you, too?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] drm/i915: Don't emit semaphore wait if wrap happened
  2012-12-06  8:51     ` Daniel Vetter
@ 2012-12-06 11:41       ` Paulo Zanoni
  2012-12-06 12:14         ` Daniel Vetter
  0 siblings, 1 reply; 15+ messages in thread
From: Paulo Zanoni @ 2012-12-06 11:41 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

Hi

2012/12/6 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Dec 5, 2012 at 9:44 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> 2012/12/4 Mika Kuoppala <mika.kuoppala@linux.intel.com>:
>>> If wrap just happened we need to prevent emitting waits for
>>> pre wrap values. Detect this and emit no-ops instead.
>>>
>>> v2: Use olr > seqno to detect wrap instead of *seqno == 0
>>> as suggested by Chris Wilson.
>>
>> This commit introduces a bug on Haswell. Now when I'm typing my
>> password on GDM the screen keeps doing wrong rendering. It "blinks
>> blue". After logging in I don't see more prodrm/i915: Set initial seqno value close to wrap boundaryblems.
>
> Just now I've taken out "drm/i915: Set initial seqno value close to
> wrap boundary" since QA complained that it regresses things. Does that
> help for you, too?

It helps: besides the "wrong rendering at GDM screen" I was also
getting  GPU hangs (when starting X, when running dmesg, when alt+tab,
etc), and it seems with today's dinq I don't get the gpu hangs
anymore. I still get the "wrong rendering" problem and it goes away if
we revert the "Don't emit semaphore wait  if wrap happened".

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] drm/i915: Don't emit semaphore wait if wrap happened
  2012-12-06 11:41       ` Paulo Zanoni
@ 2012-12-06 12:14         ` Daniel Vetter
  2012-12-06 12:24           ` Chris Wilson
  0 siblings, 1 reply; 15+ messages in thread
From: Daniel Vetter @ 2012-12-06 12:14 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx

On Thu, Dec 6, 2012 at 12:41 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2012/12/6 Daniel Vetter <daniel@ffwll.ch>:
>> On Wed, Dec 5, 2012 at 9:44 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>> 2012/12/4 Mika Kuoppala <mika.kuoppala@linux.intel.com>:
>>>> If wrap just happened we need to prevent emitting waits for
>>>> pre wrap values. Detect this and emit no-ops instead.
>>>>
>>>> v2: Use olr > seqno to detect wrap instead of *seqno == 0
>>>> as suggested by Chris Wilson.
>>>
>>> This commit introduces a bug on Haswell. Now when I'm typing my
>>> password on GDM the screen keeps doing wrong rendering. It "blinks
>>> blue". After logging in I don't see more prodrm/i915: Set initial seqno value close to wrap boundaryblems.
>>
>> Just now I've taken out "drm/i915: Set initial seqno value close to
>> wrap boundary" since QA complained that it regresses things. Does that
>> help for you, too?
>
> It helps: besides the "wrong rendering at GDM screen" I was also
> getting  GPU hangs (when starting X, when running dmesg, when alt+tab,
> etc), and it seems with today's dinq I don't get the gpu hangs
> anymore. I still get the "wrong rendering" problem and it goes away if
> we revert the "Don't emit semaphore wait  if wrap happened".

Ok, looks like we have still some fish left to fry here. I've backed
out the 2nd patch, too. And I guess we need some more tests in i-g-t
to check semaphore correctness, we seem to have some serious gaps ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/6] drm/i915: Don't emit semaphore wait if wrap happened
  2012-12-06 12:14         ` Daniel Vetter
@ 2012-12-06 12:24           ` Chris Wilson
  0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2012-12-06 12:24 UTC (permalink / raw)
  To: Daniel Vetter, Paulo Zanoni; +Cc: intel-gfx

On Thu, 6 Dec 2012 13:14:09 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Dec 6, 2012 at 12:41 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > 2012/12/6 Daniel Vetter <daniel@ffwll.ch>:
> >> On Wed, Dec 5, 2012 at 9:44 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> >>> 2012/12/4 Mika Kuoppala <mika.kuoppala@linux.intel.com>:
> >>>> If wrap just happened we need to prevent emitting waits for
> >>>> pre wrap values. Detect this and emit no-ops instead.
> >>>>
> >>>> v2: Use olr > seqno to detect wrap instead of *seqno == 0
> >>>> as suggested by Chris Wilson.
> >>>
> >>> This commit introduces a bug on Haswell. Now when I'm typing my
> >>> password on GDM the screen keeps doing wrong rendering. It "blinks
> >>> blue". After logging in I don't see more prodrm/i915: Set initial seqno value close to wrap boundaryblems.
> >>
> >> Just now I've taken out "drm/i915: Set initial seqno value close to
> >> wrap boundary" since QA complained that it regresses things. Does that
> >> help for you, too?
> >
> > It helps: besides the "wrong rendering at GDM screen" I was also
> > getting  GPU hangs (when starting X, when running dmesg, when alt+tab,
> > etc), and it seems with today's dinq I don't get the gpu hangs
> > anymore. I still get the "wrong rendering" problem and it goes away if
> > we revert the "Don't emit semaphore wait  if wrap happened".
> 
> Ok, looks like we have still some fish left to fry here. I've backed
> out the 2nd patch, too. And I guess we need some more tests in i-g-t
> to check semaphore correctness, we seem to have some serious gaps ...

I wouldn't back that out too quickly as it seems that Paulo has some
debugging to do first. A few WARNs would be a good start...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 15+ messages in thread
* [PATCH] Detect wraparound using next-seqno rather than waiter->olr
  2012-12-04 13:12 ` [PATCH 3/6] drm/i915: Don't emit semaphore wait if wrap happened Mika Kuoppala
  2012-12-05 20:44   ` Paulo Zanoni
@ 2012-12-06 14:12   ` Chris Wilson
  1 sibling, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2012-12-06 14:12 UTC (permalink / raw)
  To: intel-gfx

As the olr may have been allocated long in advance for a much earlier
write to the ring, waiter->olr may genuinely be less than the semaphore
seqno. So use the global next_seqno for wraparound detection instead.
---
 drivers/gpu/drm/i915/intel_ringbuffer.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index d5d5177..29f9e2c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -595,6 +595,12 @@ gen6_add_request(struct intel_ring_buffer *ring)
 	return 0;
 }
 
+static inline u32 i915_gem_next_seqno(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	return dev_priv->next_seqno;
+}
+
 /**
  * intel_ring_sync - sync the waiter to the signaller on seqno
  *
@@ -628,7 +634,7 @@ gen6_ring_sync(struct intel_ring_buffer *waiter,
 	BUG_ON(!waiter->outstanding_lazy_request);
 
 	/* If seqno wrap happened, omit the wait with no-ops */
-	if (likely(waiter->outstanding_lazy_request > seqno)) {
+	if (likely(i915_gem_next_seqno(waiter->dev) > seqno)) {
 		intel_ring_emit(waiter,
 				dw1 |
 				signaller->semaphore_register[waiter->id]);
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-12-06 14:14 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-04 13:11 [PATCH 0/6] ring seqno wrap handling Mika Kuoppala
2012-12-04 13:12 ` [PATCH 1/6] drm/i915: Add debugfs entry to read/write next_seqno Mika Kuoppala
2012-12-04 13:12 ` [PATCH 2/6] drm/i915: Fix debugfs seqno info print to use uint Mika Kuoppala
2012-12-04 13:12 ` [PATCH 3/6] drm/i915: Don't emit semaphore wait if wrap happened Mika Kuoppala
2012-12-05 20:44   ` Paulo Zanoni
2012-12-06  8:51     ` Daniel Vetter
2012-12-06 11:41       ` Paulo Zanoni
2012-12-06 12:14         ` Daniel Vetter
2012-12-06 12:24           ` Chris Wilson
2012-12-06 14:12   ` [PATCH] Detect wraparound using next-seqno rather than waiter->olr Chris Wilson
2012-12-04 13:12 ` [PATCH 4/6] drm/i915: Split intel_ring_begin Mika Kuoppala
2012-12-04 13:12 ` [PATCH 5/6] drm/i915: Add intel_ring_handle_seqno wrap Mika Kuoppala
2012-12-04 13:12 ` [PATCH 6/6] drm/i915: Set initial seqno value close to wrap boundary Mika Kuoppala
2012-12-04 14:05 ` [PATCH 0/6] ring seqno wrap handling Chris Wilson
2012-12-04 22:03   ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.