WIP POSTING_READ fix series

WIP POSTING_READ fix series

[Eric Anholt]

Here's what I've been talking about on IRC today.  Patch 3 appears to
make things work.  Patch 4 should work, but re-breaks things (hangs
but no dmesg complaints).  I want to understand why, but I'm running
out of theories.  Perhaps there's some period of time where a write
has been PCI write posted but has not yet appeared in the GT FIFO?  Or
perhaps there's some time after GT FIFO but before it's really handled
by hardware?  Note that I was also able to fix the problem without
this series by just POSTING_READing 4 times instead of 1.

[PATCH 1/4] drm/i915: Control gen6 ring interrupts through a single mask field.

[Eric Anholt]

The BSpec says that the GTIMR is not to be used for masking
interrupts.  Regardless, writing one register should be better than
writing two registers.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/i915/i915_irq.c         |   19 +++++++++++++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |   32 +++++++++---------------------
 2 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b79619a..0f8af27 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1782,21 +1782,30 @@ int ironlake_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(DEIER, display_mask | DE_PIPEA_VBLANK | DE_PIPEB_VBLANK);
 	POSTING_READ(DEIER);
 
-	dev_priv->gt_irq_mask = ~0;
-
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
-	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
 
-	if (IS_GEN6(dev))
+	if (IS_GEN6(dev)) {
 		render_irqs =
 			GT_USER_INTERRUPT |
 			GT_GEN6_BSD_USER_INTERRUPT |
 			GT_BLT_USER_INTERRUPT;
-	else
+
+		/* The GTIMR is not supposed to be used for command streamer
+		 * interrupt events.  They are masked out in the per-ring
+		 * interrupt masks.
+		 */
+		dev_priv->gt_irq_mask = ~render_irqs;
+	} else {
 		render_irqs =
 			GT_USER_INTERRUPT |
 			GT_PIPE_NOTIFY |
 			GT_BSD_USER_INTERRUPT;
+
+		dev_priv->gt_irq_mask = ~0;
+	}
+
+	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
+
 	I915_WRITE(GTIER, render_irqs);
 	POSTING_READ(GTIER);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 95c4b14..c6c4d23 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -621,7 +621,7 @@ ring_add_request(struct intel_ring_buffer *ring,
 }
 
 static bool
-gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
+gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 rflag)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -633,7 +633,7 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
 	if (ring->irq_refcount++ == 0) {
 		ring->irq_mask &= ~rflag;
 		I915_WRITE_IMR(ring, ring->irq_mask);
-		ironlake_enable_irq(dev_priv, gflag);
+		POSTING_READ(RING_IMR(ring->mmio_base));
 	}
 	spin_unlock(&ring->irq_lock);
 
@@ -641,7 +641,7 @@ gen6_ring_get_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
 }
 
 static void
-gen6_ring_put_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
+gen6_ring_put_irq(struct intel_ring_buffer *ring, u32 rflag)
 {
 	struct drm_device *dev = ring->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
@@ -650,7 +650,7 @@ gen6_ring_put_irq(struct intel_ring_buffer *ring, u32 gflag, u32 rflag)
 	if (--ring->irq_refcount == 0) {
 		ring->irq_mask |= rflag;
 		I915_WRITE_IMR(ring, ring->irq_mask);
-		ironlake_disable_irq(dev_priv, gflag);
+		POSTING_READ(RING_IMR(ring->mmio_base));
 	}
 	spin_unlock(&ring->irq_lock);
 }
@@ -1105,33 +1105,25 @@ gen6_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
 static bool
 gen6_render_ring_get_irq(struct intel_ring_buffer *ring)
 {
-	return gen6_ring_get_irq(ring,
-				 GT_USER_INTERRUPT,
-				 GEN6_RENDER_USER_INTERRUPT);
+	return gen6_ring_get_irq(ring, GEN6_RENDER_USER_INTERRUPT);
 }
 
 static void
 gen6_render_ring_put_irq(struct intel_ring_buffer *ring)
 {
-	return gen6_ring_put_irq(ring,
-				 GT_USER_INTERRUPT,
-				 GEN6_RENDER_USER_INTERRUPT);
+	return gen6_ring_put_irq(ring, GEN6_RENDER_USER_INTERRUPT);
 }
 
 static bool
 gen6_bsd_ring_get_irq(struct intel_ring_buffer *ring)
 {
-	return gen6_ring_get_irq(ring,
-				 GT_GEN6_BSD_USER_INTERRUPT,
-				 GEN6_BSD_USER_INTERRUPT);
+	return gen6_ring_get_irq(ring, GEN6_BSD_USER_INTERRUPT);
 }
 
 static void
 gen6_bsd_ring_put_irq(struct intel_ring_buffer *ring)
 {
-	return gen6_ring_put_irq(ring,
-				 GT_GEN6_BSD_USER_INTERRUPT,
-				 GEN6_BSD_USER_INTERRUPT);
+	return gen6_ring_put_irq(ring, GEN6_BSD_USER_INTERRUPT);
 }
 
 /* ring buffer for Video Codec for Gen6+ */
@@ -1155,17 +1147,13 @@ static const struct intel_ring_buffer gen6_bsd_ring = {
 static bool
 blt_ring_get_irq(struct intel_ring_buffer *ring)
 {
-	return gen6_ring_get_irq(ring,
-				 GT_BLT_USER_INTERRUPT,
-				 GEN6_BLITTER_USER_INTERRUPT);
+	return gen6_ring_get_irq(ring, GEN6_BLITTER_USER_INTERRUPT);
 }
 
 static void
 blt_ring_put_irq(struct intel_ring_buffer *ring)
 {
-	gen6_ring_put_irq(ring,
-			  GT_BLT_USER_INTERRUPT,
-			  GEN6_BLITTER_USER_INTERRUPT);
+	gen6_ring_put_irq(ring, GEN6_BLITTER_USER_INTERRUPT);
 }
 
 
-- 
1.7.5.1

[PATCH 2/4] drm/i915: Move the forcewake refcounting to a spinlock.

[Eric Anholt]

We have forcewake in the interrupt handler in the following commit, so
we have to use a spinlock instead of a mutex.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c |    3 +--
 drivers/gpu/drm/i915/i915_dma.c     |    1 +
 drivers/gpu/drm/i915/i915_drv.c     |   17 ++++++++++++-----
 drivers/gpu/drm/i915/i915_drv.h     |    3 ++-
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 51c2257..e62ee5d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1226,8 +1226,7 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	seq_printf(m, "forcewake count = %d\n",
-		   atomic_read(&dev_priv->forcewake_count));
+	seq_printf(m, "forcewake count = %d\n", dev_priv->forcewake_count);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 0239e99..2de4a74 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2063,6 +2063,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->irq_lock);
 	spin_lock_init(&dev_priv->error_lock);
 	spin_lock_init(&dev_priv->rps_lock);
+	spin_lock_init(&dev_priv->forcewake_lock);
 
 	if (IS_MOBILE(dev) || !IS_GEN2(dev))
 		dev_priv->num_pipe = 2;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0defd42..0c46bfc 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -319,11 +319,14 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
  */
 void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 {
-	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+	unsigned long flags;
 
-	/* Forcewake is atomic in case we get in here without the lock */
-	if (atomic_add_return(1, &dev_priv->forcewake_count) == 1)
+	spin_lock_irqsave(&dev_priv->forcewake_lock, flags);
+
+	if (dev_priv->forcewake_count++ == 0)
 		__gen6_gt_force_wake_get(dev_priv);
+
+	spin_unlock_irqrestore(&dev_priv->forcewake_lock, flags);
 }
 
 static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
@@ -337,10 +340,14 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
  */
 void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 {
-	WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+	unsigned long flags;
 
-	if (atomic_dec_and_test(&dev_priv->forcewake_count))
+	spin_lock_irqsave(&dev_priv->forcewake_lock, flags);
+
+	if (--dev_priv->forcewake_count == 0)
 		__gen6_gt_force_wake_put(dev_priv);
+
+	spin_unlock_irqrestore(&dev_priv->forcewake_lock, flags);
 }
 
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ee66035..56ef324 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -717,7 +717,8 @@ typedef struct drm_i915_private {
 
 	struct drm_property *broadcast_rgb_property;
 
-	atomic_t forcewake_count;
+	int forcewake_count;
+	spinlock_t forcewake_lock;
 } drm_i915_private_t;
 
 enum i915_cache_level {
-- 
1.7.5.1

[PATCH 3/4] drm/i915: Fix missed IRQs on gen6.

[Eric Anholt]

Our wait-on-interrupts path goes:

I915_WRITE(IMR, enabled_val)
POSTING_READ(IMR);
if (!i915_gem_seqno_passed())
   sleep();

However, the IMR write can still be queued in the GT FIFO even after
the POSTING_READ, since the GT FIFO isn't implementing PCI semantics.
Yay.  So, when we're doing a POSTING_READ, we get to check that the
FIFO is empty, and unfortunately to check that the FIFO is empty, it
appears we have to do the FORCEWAKE dance.

Previously, piglit copypixels-sync test had about a 0.5% chance of
triggering a missed IRQ (meaning I've never seen 500 successful runs
without a failure).  With this pair of patches, I'm at 5700 clean runs
as I type this.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/i915/i915_drv.c |   41 +++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_drv.h |    3 +-
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0c46bfc..eabf82c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -304,7 +304,10 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 		udelay(10);
 
 	I915_WRITE_NOTRACE(FORCEWAKE, 1);
-	POSTING_READ(FORCEWAKE);
+	/* This is a posting read, but the POSTING_READ macro calls
+	 * this function.
+	 */
+	(void)I915_READ_NOTRACE(FORCEWAKE);
 
 	count = 0;
 	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1) == 0)
@@ -332,7 +335,10 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
 static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE_NOTRACE(FORCEWAKE, 0);
-	POSTING_READ(FORCEWAKE);
+	/* This is a posting read, but the POSTING_READ macro calls
+	 * this function.
+	 */
+	(void)I915_READ_NOTRACE(FORCEWAKE);
 }
 
 /*
@@ -360,6 +366,37 @@ void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 	}
 }
 
+/* In PCI, writes are not guaranteed to reach the device until a read
+ * from the device has occurred.  The POSTING_READ macro is used to
+ * indicate when we are doing one of those reads to force writes to
+ * the device, so that we can for example test a value in some cached
+ * memory and go to sleep waiting for an interrupt
+ * (i915_wait_request()) or implement some timing requirement.
+ *
+ * However, as of gen6, a mere PCI write posting read doesn't mean
+ * that the writes have reached the device, as they get queued in a
+ * FIFO to hide the wake-from-rc6 latency, even if the chip wasn't in
+ * RC6 at the time.  For gen6, we also have to wait for the fifo to
+ * drain, which means force-waking the GPU so that we can read the
+ * fifo count.
+ */
+void
+intel_posting_read(struct drm_i915_private *dev_priv, uint32_t reg)
+{
+	int ret;
+
+	if (dev_priv->info->gen < 6) {
+		(void)I915_READ_NOTRACE(reg);
+		return;
+	}
+
+	gen6_gt_force_wake_get(dev_priv);
+	ret = wait_for(I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES) == 0x3f, 500);
+	gen6_gt_force_wake_put(dev_priv);
+
+	WARN_ON_ONCE(ret != 0);
+}
+
 static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 56ef324..a064044 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1366,6 +1366,7 @@ extern void intel_display_print_error_state(struct seq_file *m,
 void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
 void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv);
+void intel_posting_read(struct drm_i915_private *dev_priv, uint32_t reg);
 
 /* We give fast paths for the really cool registers */
 #define NEEDS_FORCE_WAKE(dev_priv, reg) \
@@ -1423,7 +1424,7 @@ __i915_write(64, q)
 #define I915_WRITE64(reg, val)	i915_write64(dev_priv, (reg), (val))
 #define I915_READ64(reg)	i915_read64(dev_priv, (reg))
 
-#define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
+#define POSTING_READ(reg)	intel_posting_read(dev_priv, reg)
 #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
 
 
-- 
1.7.5.1

[PATCH 4/4] drm/i915: Try removing the forcewake get/put around the fifo counting.

[Eric Anholt]

This reintroduces the IRQ misses, with 2 events in 1874 runs.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/i915/i915_drv.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index eabf82c..5444032 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -383,18 +383,24 @@ void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 void
 intel_posting_read(struct drm_i915_private *dev_priv, uint32_t reg)
 {
-	int ret;
+	u32 fifo;
+	int loop = 5000;
 
 	if (dev_priv->info->gen < 6) {
 		(void)I915_READ_NOTRACE(reg);
 		return;
 	}
 
-	gen6_gt_force_wake_get(dev_priv);
-	ret = wait_for(I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES) == 0x3f, 500);
-	gen6_gt_force_wake_put(dev_priv);
+	if (!I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES))
+		printk("it got 0\n");
+
+	fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
+	while (fifo != 0x3f && --loop) {
+		udelay(10);
+		fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
+	}
 
-	WARN_ON_ONCE(ret != 0);
+	WARN_ON_ONCE(loop == 0);
 }
 
 static int i915_drm_freeze(struct drm_device *dev)
-- 
1.7.5.1

Re: WIP POSTING_READ fix series

[Jesse Barnes]

On Fri,  3 Jun 2011 12:20:18 -0700
Eric Anholt <eric@anholt.net> wrote:

> Here's what I've been talking about on IRC today.  Patch 3 appears to
> make things work.  Patch 4 should work, but re-breaks things (hangs
> but no dmesg complaints).  I want to understand why, but I'm running
> out of theories.  Perhaps there's some period of time where a write
> has been PCI write posted but has not yet appeared in the GT FIFO?  Or
> perhaps there's some time after GT FIFO but before it's really handled
> by hardware?  Note that I was also able to fix the problem without
> this series by just POSTING_READing 4 times instead of 1.

The fact that patch 4 re-introduces the issue makes me think we still
have a timing issue separate from the FIFO depth or force wake
handling.  How long does the function take to complete in both cases?
Does simply delaying the posting_read to match the approximate force
wake get/put time also get rid of the missed interrupts?  If so, maybe
we're missing some other aspect of the interrupt masking dance instead?

-- 
Jesse Barnes, Intel Open Source Technology Center