[PATCH] drm/i915: Don't rmw PIPESTAT enable bits

[PATCH] drm/i915: Don't rmw PIPESTAT enable bits

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

i830 seems to occasionally forget the PIPESTAT enable bits when
we read the register. These aren't the only registers on i830 that
have problems with RMW, as reading the double buffered plane
registers returns the latched value rather than the last written
value. So something similar is perhaps going on with PIPESTAT.

This corruption results on vblank interrupts occasionally turning off
on their own, which leads to vblank timeouts and generally a stuck
display subsystem.

So let's not RMW the pipestat enable bits, and instead use the cached
copy we have around.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            |   2 +
 drivers/gpu/drm/i915/i915_irq.c            | 135 ++++++++++++-----------------
 drivers/gpu/drm/i915/intel_fifo_underrun.c |  14 +--
 3 files changed, 66 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 28ad5dadbb18..3b4dd410cad1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3307,6 +3307,8 @@ static inline bool intel_vgpu_active(struct drm_i915_private *dev_priv)
 	return dev_priv->vgpu.active;
 }
 
+u32 i915_pipestat_enable_mask(struct drm_i915_private *dev_priv,
+			      enum pipe pipe);
 void
 i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 		     u32 status_mask);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 003a92857102..7c4e1a1faed7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -567,62 +567,16 @@ void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
 	POSTING_READ(SDEIMR);
 }
 
-static void
-__i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
-		       u32 enable_mask, u32 status_mask)
+u32 i915_pipestat_enable_mask(struct drm_i915_private *dev_priv,
+			      enum pipe pipe)
 {
-	i915_reg_t reg = PIPESTAT(pipe);
-	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
-
-	lockdep_assert_held(&dev_priv->irq_lock);
-	WARN_ON(!intel_irqs_enabled(dev_priv));
-
-	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
-		      status_mask & ~PIPESTAT_INT_STATUS_MASK,
-		      "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
-		      pipe_name(pipe), enable_mask, status_mask))
-		return;
-
-	if ((pipestat & enable_mask) == enable_mask)
-		return;
-
-	dev_priv->pipestat_irq_mask[pipe] |= status_mask;
-
-	/* Enable the interrupt, clear any pending status */
-	pipestat |= enable_mask | status_mask;
-	I915_WRITE(reg, pipestat);
-	POSTING_READ(reg);
-}
-
-static void
-__i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
-		        u32 enable_mask, u32 status_mask)
-{
-	i915_reg_t reg = PIPESTAT(pipe);
-	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
+	u32 status_mask = dev_priv->pipestat_irq_mask[pipe];
+	u32 enable_mask = status_mask << 16;
 
 	lockdep_assert_held(&dev_priv->irq_lock);
-	WARN_ON(!intel_irqs_enabled(dev_priv));
-
-	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
-		      status_mask & ~PIPESTAT_INT_STATUS_MASK,
-		      "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
-		      pipe_name(pipe), enable_mask, status_mask))
-		return;
-
-	if ((pipestat & enable_mask) == 0)
-		return;
-
-	dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
-
-	pipestat &= ~enable_mask;
-	I915_WRITE(reg, pipestat);
-	POSTING_READ(reg);
-}
 
-static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32 status_mask)
-{
-	u32 enable_mask = status_mask << 16;
+	if (INTEL_GEN(dev_priv) < 5)
+		goto out;
 
 	/*
 	 * On pipe A we don't support the PSR interrupt yet,
@@ -645,35 +599,59 @@ static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32 status_mask)
 	if (status_mask & SPRITE1_FLIP_DONE_INT_STATUS_VLV)
 		enable_mask |= SPRITE1_FLIP_DONE_INT_EN_VLV;
 
+out:
+	WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
+		  status_mask & ~PIPESTAT_INT_STATUS_MASK,
+		  "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
+		  pipe_name(pipe), enable_mask, status_mask);
+
 	return enable_mask;
 }
 
-void
-i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
-		     u32 status_mask)
+void i915_enable_pipestat(struct drm_i915_private *dev_priv,
+			  enum pipe pipe, u32 status_mask)
 {
+	i915_reg_t reg = PIPESTAT(pipe);
 	u32 enable_mask;
 
-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		enable_mask = vlv_get_pipestat_enable_mask(&dev_priv->drm,
-							   status_mask);
-	else
-		enable_mask = status_mask << 16;
-	__i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
+	WARN_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK,
+		  "pipe %c: status_mask=0x%x\n",
+		  pipe_name(pipe), status_mask);
+
+	lockdep_assert_held(&dev_priv->irq_lock);
+	WARN_ON(!intel_irqs_enabled(dev_priv));
+
+	if ((dev_priv->pipestat_irq_mask[pipe] & status_mask) == status_mask)
+		return;
+
+	dev_priv->pipestat_irq_mask[pipe] |= status_mask;
+	enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
+
+	I915_WRITE(reg, enable_mask | status_mask);
+	POSTING_READ(reg);
 }
 
-void
-i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
-		      u32 status_mask)
+void i915_disable_pipestat(struct drm_i915_private *dev_priv,
+			   enum pipe pipe, u32 status_mask)
 {
+	i915_reg_t reg = PIPESTAT(pipe);
 	u32 enable_mask;
 
-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		enable_mask = vlv_get_pipestat_enable_mask(&dev_priv->drm,
-							   status_mask);
-	else
-		enable_mask = status_mask << 16;
-	__i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
+	WARN_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK,
+		  "pipe %c: status_mask=0x%x\n",
+		  pipe_name(pipe), status_mask);
+
+	lockdep_assert_held(&dev_priv->irq_lock);
+	WARN_ON(!intel_irqs_enabled(dev_priv));
+
+	if ((dev_priv->pipestat_irq_mask[pipe] & status_mask) == 0)
+		return;
+
+	dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
+	enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
+
+	I915_WRITE(reg, enable_mask | status_mask);
+	POSTING_READ(reg);
 }
 
 /**
@@ -1769,7 +1747,7 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
 
 	for_each_pipe(dev_priv, pipe) {
 		i915_reg_t reg;
-		u32 mask, iir_bit = 0;
+		u32 status_mask, enable_mask, iir_bit = 0;
 
 		/*
 		 * PIPESTAT bits get signalled even when the interrupt is
@@ -1780,7 +1758,7 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
 		 */
 
 		/* fifo underruns are filterered in the underrun handler. */
-		mask = PIPE_FIFO_UNDERRUN_STATUS;
+		status_mask = PIPE_FIFO_UNDERRUN_STATUS;
 
 		switch (pipe) {
 		case PIPE_A:
@@ -1794,21 +1772,20 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
 			break;
 		}
 		if (iir & iir_bit)
-			mask |= dev_priv->pipestat_irq_mask[pipe];
+			status_mask |= dev_priv->pipestat_irq_mask[pipe];
 
-		if (!mask)
+		if (!status_mask)
 			continue;
 
 		reg = PIPESTAT(pipe);
-		mask |= PIPESTAT_INT_ENABLE_MASK;
-		pipe_stats[pipe] = I915_READ(reg) & mask;
+		pipe_stats[pipe] = I915_READ(reg) & status_mask;
+		enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
 
 		/*
 		 * Clear the PIPE*STAT regs before the IIR
 		 */
-		if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
-					PIPESTAT_INT_STATUS_MASK))
-			I915_WRITE(reg, pipe_stats[pipe]);
+		if (pipe_stats[pipe])
+			I915_WRITE(reg, enable_mask | pipe_stats[pipe]);
 	}
 	spin_unlock(&dev_priv->irq_lock);
 }
diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
index 04689600e337..77c123cc8817 100644
--- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
@@ -88,14 +88,15 @@ static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	i915_reg_t reg = PIPESTAT(crtc->pipe);
-	u32 pipestat = I915_READ(reg) & 0xffff0000;
+	u32 enable_mask;
 
 	lockdep_assert_held(&dev_priv->irq_lock);
 
-	if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
+	if ((I915_READ(reg) & PIPE_FIFO_UNDERRUN_STATUS) == 0)
 		return;
 
-	I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
+	enable_mask = i915_pipestat_enable_mask(dev_priv, crtc->pipe);
+	I915_WRITE(reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
 	POSTING_READ(reg);
 
 	trace_intel_cpu_fifo_underrun(dev_priv, crtc->pipe);
@@ -108,15 +109,16 @@ static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	i915_reg_t reg = PIPESTAT(pipe);
-	u32 pipestat = I915_READ(reg) & 0xffff0000;
 
 	lockdep_assert_held(&dev_priv->irq_lock);
 
 	if (enable) {
-		I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
+		u32 enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
+
+		I915_WRITE(reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
 		POSTING_READ(reg);
 	} else {
-		if (old && pipestat & PIPE_FIFO_UNDERRUN_STATUS)
+		if (old && I915_READ(reg) & PIPE_FIFO_UNDERRUN_STATUS)
 			DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
 	}
 }
-- 
2.13.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Don't rmw PIPESTAT enable bits

[Patchwork]

== Series Details ==

Series: drm/i915: Don't rmw PIPESTAT enable bits
URL   : https://patchwork.freedesktop.org/series/30367/
State : success

== Summary ==

Series 30367v1 drm/i915: Don't rmw PIPESTAT enable bits
https://patchwork.freedesktop.org/api/1.0/series/30367/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-snb-2600) fdo#100215
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                skip       -> PASS       (fi-skl-x1585l)

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:443s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:455s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:381s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:533s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:271s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:501s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:504s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:495s
fi-cfl-s         total:289  pass:223  dwarn:34  dfail:0   fail:0   skip:32  time:554s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:447s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:592s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:426s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:407s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:441s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:486s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:467s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:490s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:577s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:584s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:550s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:462s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:519s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:503s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:464s
fi-skl-x1585l    total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:476s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:570s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:1   skip:39  time:420s

46e05d756fe32271bca4ac3ef1b3f1f6006fcc0c drm-tip: 2017y-09m-14d-14h-46m-01s UTC integration manifest
ae8aa6262034 drm/i915: Don't rmw PIPESTAT enable bits

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5702/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't rmw PIPESTAT enable bits

[Chris Wilson]

Quoting Ville Syrjala (2017-09-14 16:17:31)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> i830 seems to occasionally forget the PIPESTAT enable bits when
> we read the register. These aren't the only registers on i830 that
> have problems with RMW, as reading the double buffered plane
> registers returns the latched value rather than the last written
> value. So something similar is perhaps going on with PIPESTAT.
> 
> This corruption results on vblank interrupts occasionally turning off
> on their own, which leads to vblank timeouts and generally a stuck
> display subsystem.
> 
> So let's not RMW the pipestat enable bits, and instead use the cached
> copy we have around.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Well it didn't make my 845g any worse. Still has

[  245.683349] [drm:pipe_config_err] *ERROR* mismatch in base.adjusted_mode.flags (2) (expected 0, found 2)
[  245.683382] [drm:pipe_config_err] *ERROR* mismatch in base.adjusted_mode.flags (8) (expected 0, found 8)
[  245.683427] pipe state doesn't match!

if you are interested.

How occasional and which test in particular hits the issue?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for drm/i915: Don't rmw PIPESTAT enable bits

[Patchwork]

== Series Details ==

Series: drm/i915: Don't rmw PIPESTAT enable bits
URL   : https://patchwork.freedesktop.org/series/30367/
State : failure

== Summary ==

Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252 +1
Test kms_plane:
        Subgroup plane-panning-bottom-right-suspend-pipe-C-planes:
                skip       -> PASS       (shard-hsw)
Test kms_rmfb:
        Subgroup close-fd:
                skip       -> PASS       (shard-hsw)
Test kms_universal_plane:
        Subgroup universal-plane-pipe-B-functional:
                skip       -> PASS       (shard-hsw)
Test kms_cursor_crc:
        Subgroup cursor-256x256-dpms:
                skip       -> PASS       (shard-hsw)
Test kms_flip:
        Subgroup flip-vs-fences-interruptible:
                pass       -> FAIL       (shard-hsw)

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-hsw        total:2313 pass:1243 dwarn:0   dfail:0   fail:15  skip:1055 time:9384s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5702/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't rmw PIPESTAT enable bits

[Ville Syrjälä]

On Thu, Sep 14, 2017 at 09:37:37PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2017-09-14 16:17:31)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > i830 seems to occasionally forget the PIPESTAT enable bits when
> > we read the register. These aren't the only registers on i830 that
> > have problems with RMW, as reading the double buffered plane
> > registers returns the latched value rather than the last written
> > value. So something similar is perhaps going on with PIPESTAT.
> > 
> > This corruption results on vblank interrupts occasionally turning off
> > on their own, which leads to vblank timeouts and generally a stuck
> > display subsystem.
> > 
> > So let's not RMW the pipestat enable bits, and instead use the cached
> > copy we have around.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Well it didn't make my 845g any worse. Still has
> 
> [  245.683349] [drm:pipe_config_err] *ERROR* mismatch in base.adjusted_mode.flags (2) (expected 0, found 2)
> [  245.683382] [drm:pipe_config_err] *ERROR* mismatch in base.adjusted_mode.flags (8) (expected 0, found 8)
> [  245.683427] pipe state doesn't match!
> 
> if you are interested.

That's a bit odd. Even if the mode originally doesn't have any
hsync/vsync flags intel_modeset_pipe_config() should give it some.
So I can't immediately see how we could manage to get there with
expected==0.

> 
> How occasional and which test in particular hits the issue?

It's been a few months but IIRC I was just running xonotic when I
hit the problem. It was pretty good at hitting it. Prior to the fix
I had to rewrite PIPESTAT several times to make it through the
the-big-keybench demo. I don't recall anymore if I hit it with
anything else.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't rmw PIPESTAT enable bits

[Imre Deak]

On Thu, Sep 14, 2017 at 06:17:31PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> i830 seems to occasionally forget the PIPESTAT enable bits when
> we read the register. These aren't the only registers on i830 that
> have problems with RMW, as reading the double buffered plane
> registers returns the latched value rather than the last written
> value. So something similar is perhaps going on with PIPESTAT.
> 
> This corruption results on vblank interrupts occasionally turning off
> on their own, which leads to vblank timeouts and generally a stuck
> display subsystem.
> 
> So let's not RMW the pipestat enable bits, and instead use the cached
> copy we have around.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |   2 +
>  drivers/gpu/drm/i915/i915_irq.c            | 135 ++++++++++++-----------------
>  drivers/gpu/drm/i915/intel_fifo_underrun.c |  14 +--
>  3 files changed, 66 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 28ad5dadbb18..3b4dd410cad1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3307,6 +3307,8 @@ static inline bool intel_vgpu_active(struct drm_i915_private *dev_priv)
>  	return dev_priv->vgpu.active;
>  }
>  
> +u32 i915_pipestat_enable_mask(struct drm_i915_private *dev_priv,
> +			      enum pipe pipe);
>  void
>  i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>  		     u32 status_mask);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 003a92857102..7c4e1a1faed7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -567,62 +567,16 @@ void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
>  	POSTING_READ(SDEIMR);
>  }
>  
> -static void
> -__i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> -		       u32 enable_mask, u32 status_mask)
> +u32 i915_pipestat_enable_mask(struct drm_i915_private *dev_priv,
> +			      enum pipe pipe)
>  {
> -	i915_reg_t reg = PIPESTAT(pipe);
> -	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> -
> -	lockdep_assert_held(&dev_priv->irq_lock);
> -	WARN_ON(!intel_irqs_enabled(dev_priv));
> -
> -	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> -		      status_mask & ~PIPESTAT_INT_STATUS_MASK,
> -		      "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
> -		      pipe_name(pipe), enable_mask, status_mask))
> -		return;
> -
> -	if ((pipestat & enable_mask) == enable_mask)
> -		return;
> -
> -	dev_priv->pipestat_irq_mask[pipe] |= status_mask;
> -
> -	/* Enable the interrupt, clear any pending status */
> -	pipestat |= enable_mask | status_mask;
> -	I915_WRITE(reg, pipestat);
> -	POSTING_READ(reg);
> -}
> -
> -static void
> -__i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> -		        u32 enable_mask, u32 status_mask)
> -{
> -	i915_reg_t reg = PIPESTAT(pipe);
> -	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> +	u32 status_mask = dev_priv->pipestat_irq_mask[pipe];
> +	u32 enable_mask = status_mask << 16;
>  
>  	lockdep_assert_held(&dev_priv->irq_lock);
> -	WARN_ON(!intel_irqs_enabled(dev_priv));
> -
> -	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> -		      status_mask & ~PIPESTAT_INT_STATUS_MASK,
> -		      "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
> -		      pipe_name(pipe), enable_mask, status_mask))
> -		return;
> -
> -	if ((pipestat & enable_mask) == 0)
> -		return;
> -
> -	dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> -
> -	pipestat &= ~enable_mask;
> -	I915_WRITE(reg, pipestat);
> -	POSTING_READ(reg);
> -}
>  
> -static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32 status_mask)
> -{
> -	u32 enable_mask = status_mask << 16;
> +	if (INTEL_GEN(dev_priv) < 5)
> +		goto out;
>  
>  	/*
>  	 * On pipe A we don't support the PSR interrupt yet,
> @@ -645,35 +599,59 @@ static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32 status_mask)
>  	if (status_mask & SPRITE1_FLIP_DONE_INT_STATUS_VLV)
>  		enable_mask |= SPRITE1_FLIP_DONE_INT_EN_VLV;
>  
> +out:
> +	WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> +		  status_mask & ~PIPESTAT_INT_STATUS_MASK,
> +		  "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
> +		  pipe_name(pipe), enable_mask, status_mask);
> +
>  	return enable_mask;
>  }
>  
> -void
> -i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> -		     u32 status_mask)
> +void i915_enable_pipestat(struct drm_i915_private *dev_priv,
> +			  enum pipe pipe, u32 status_mask)
>  {
> +	i915_reg_t reg = PIPESTAT(pipe);
>  	u32 enable_mask;
>  
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		enable_mask = vlv_get_pipestat_enable_mask(&dev_priv->drm,
> -							   status_mask);
> -	else
> -		enable_mask = status_mask << 16;
> -	__i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
> +	WARN_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK,
> +		  "pipe %c: status_mask=0x%x\n",
> +		  pipe_name(pipe), status_mask);
> +
> +	lockdep_assert_held(&dev_priv->irq_lock);
> +	WARN_ON(!intel_irqs_enabled(dev_priv));
> +
> +	if ((dev_priv->pipestat_irq_mask[pipe] & status_mask) == status_mask)
> +		return;
> +
> +	dev_priv->pipestat_irq_mask[pipe] |= status_mask;
> +	enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
> +
> +	I915_WRITE(reg, enable_mask | status_mask);
> +	POSTING_READ(reg);
>  }
>  
> -void
> -i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> -		      u32 status_mask)
> +void i915_disable_pipestat(struct drm_i915_private *dev_priv,
> +			   enum pipe pipe, u32 status_mask)
>  {
> +	i915_reg_t reg = PIPESTAT(pipe);
>  	u32 enable_mask;
>  
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		enable_mask = vlv_get_pipestat_enable_mask(&dev_priv->drm,
> -							   status_mask);
> -	else
> -		enable_mask = status_mask << 16;
> -	__i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
> +	WARN_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK,
> +		  "pipe %c: status_mask=0x%x\n",
> +		  pipe_name(pipe), status_mask);
> +
> +	lockdep_assert_held(&dev_priv->irq_lock);
> +	WARN_ON(!intel_irqs_enabled(dev_priv));
> +
> +	if ((dev_priv->pipestat_irq_mask[pipe] & status_mask) == 0)
> +		return;
> +
> +	dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> +	enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
> +
> +	I915_WRITE(reg, enable_mask | status_mask);
> +	POSTING_READ(reg);
>  }
>  
>  /**
> @@ -1769,7 +1747,7 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
>  
>  	for_each_pipe(dev_priv, pipe) {
>  		i915_reg_t reg;
> -		u32 mask, iir_bit = 0;
> +		u32 status_mask, enable_mask, iir_bit = 0;
>  
>  		/*
>  		 * PIPESTAT bits get signalled even when the interrupt is
> @@ -1780,7 +1758,7 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
>  		 */
>  
>  		/* fifo underruns are filterered in the underrun handler. */
> -		mask = PIPE_FIFO_UNDERRUN_STATUS;
> +		status_mask = PIPE_FIFO_UNDERRUN_STATUS;
>  
>  		switch (pipe) {
>  		case PIPE_A:
> @@ -1794,21 +1772,20 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
>  			break;
>  		}
>  		if (iir & iir_bit)
> -			mask |= dev_priv->pipestat_irq_mask[pipe];
> +			status_mask |= dev_priv->pipestat_irq_mask[pipe];
>  
> -		if (!mask)
> +		if (!status_mask)
>  			continue;

Not introduced here, but the above could be removed.
The patch looks ok:

Reviewed-by: Imre Deak <imre.deak@intel.com>

>  
>  		reg = PIPESTAT(pipe);
> -		mask |= PIPESTAT_INT_ENABLE_MASK;
> -		pipe_stats[pipe] = I915_READ(reg) & mask;
> +		pipe_stats[pipe] = I915_READ(reg) & status_mask;
> +		enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
>  
>  		/*
>  		 * Clear the PIPE*STAT regs before the IIR
>  		 */
> -		if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
> -					PIPESTAT_INT_STATUS_MASK))
> -			I915_WRITE(reg, pipe_stats[pipe]);
> +		if (pipe_stats[pipe])
> +			I915_WRITE(reg, enable_mask | pipe_stats[pipe]);
>  	}
>  	spin_unlock(&dev_priv->irq_lock);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> index 04689600e337..77c123cc8817 100644
> --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> @@ -88,14 +88,15 @@ static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	i915_reg_t reg = PIPESTAT(crtc->pipe);
> -	u32 pipestat = I915_READ(reg) & 0xffff0000;
> +	u32 enable_mask;
>  
>  	lockdep_assert_held(&dev_priv->irq_lock);
>  
> -	if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
> +	if ((I915_READ(reg) & PIPE_FIFO_UNDERRUN_STATUS) == 0)
>  		return;
>  
> -	I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
> +	enable_mask = i915_pipestat_enable_mask(dev_priv, crtc->pipe);
> +	I915_WRITE(reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
>  	POSTING_READ(reg);
>  
>  	trace_intel_cpu_fifo_underrun(dev_priv, crtc->pipe);
> @@ -108,15 +109,16 @@ static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	i915_reg_t reg = PIPESTAT(pipe);
> -	u32 pipestat = I915_READ(reg) & 0xffff0000;
>  
>  	lockdep_assert_held(&dev_priv->irq_lock);
>  
>  	if (enable) {
> -		I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
> +		u32 enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
> +
> +		I915_WRITE(reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
>  		POSTING_READ(reg);
>  	} else {
> -		if (old && pipestat & PIPE_FIFO_UNDERRUN_STATUS)
> +		if (old && I915_READ(reg) & PIPE_FIFO_UNDERRUN_STATUS)
>  			DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
>  	}
>  }
> -- 
> 2.13.5
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't rmw PIPESTAT enable bits

[Ville Syrjälä]

On Mon, Sep 25, 2017 at 01:33:18PM +0300, Imre Deak wrote:
> On Thu, Sep 14, 2017 at 06:17:31PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > i830 seems to occasionally forget the PIPESTAT enable bits when
> > we read the register. These aren't the only registers on i830 that
> > have problems with RMW, as reading the double buffered plane
> > registers returns the latched value rather than the last written
> > value. So something similar is perhaps going on with PIPESTAT.
> > 
> > This corruption results on vblank interrupts occasionally turning off
> > on their own, which leads to vblank timeouts and generally a stuck
> > display subsystem.
> > 
> > So let's not RMW the pipestat enable bits, and instead use the cached
> > copy we have around.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h            |   2 +
> >  drivers/gpu/drm/i915/i915_irq.c            | 135 ++++++++++++-----------------
> >  drivers/gpu/drm/i915/intel_fifo_underrun.c |  14 +--
> >  3 files changed, 66 insertions(+), 85 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 28ad5dadbb18..3b4dd410cad1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3307,6 +3307,8 @@ static inline bool intel_vgpu_active(struct drm_i915_private *dev_priv)
> >  	return dev_priv->vgpu.active;
> >  }
> >  
> > +u32 i915_pipestat_enable_mask(struct drm_i915_private *dev_priv,
> > +			      enum pipe pipe);
> >  void
> >  i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> >  		     u32 status_mask);
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 003a92857102..7c4e1a1faed7 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -567,62 +567,16 @@ void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
> >  	POSTING_READ(SDEIMR);
> >  }
> >  
> > -static void
> > -__i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> > -		       u32 enable_mask, u32 status_mask)
> > +u32 i915_pipestat_enable_mask(struct drm_i915_private *dev_priv,
> > +			      enum pipe pipe)
> >  {
> > -	i915_reg_t reg = PIPESTAT(pipe);
> > -	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> > -
> > -	lockdep_assert_held(&dev_priv->irq_lock);
> > -	WARN_ON(!intel_irqs_enabled(dev_priv));
> > -
> > -	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> > -		      status_mask & ~PIPESTAT_INT_STATUS_MASK,
> > -		      "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
> > -		      pipe_name(pipe), enable_mask, status_mask))
> > -		return;
> > -
> > -	if ((pipestat & enable_mask) == enable_mask)
> > -		return;
> > -
> > -	dev_priv->pipestat_irq_mask[pipe] |= status_mask;
> > -
> > -	/* Enable the interrupt, clear any pending status */
> > -	pipestat |= enable_mask | status_mask;
> > -	I915_WRITE(reg, pipestat);
> > -	POSTING_READ(reg);
> > -}
> > -
> > -static void
> > -__i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> > -		        u32 enable_mask, u32 status_mask)
> > -{
> > -	i915_reg_t reg = PIPESTAT(pipe);
> > -	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> > +	u32 status_mask = dev_priv->pipestat_irq_mask[pipe];
> > +	u32 enable_mask = status_mask << 16;
> >  
> >  	lockdep_assert_held(&dev_priv->irq_lock);
> > -	WARN_ON(!intel_irqs_enabled(dev_priv));
> > -
> > -	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> > -		      status_mask & ~PIPESTAT_INT_STATUS_MASK,
> > -		      "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
> > -		      pipe_name(pipe), enable_mask, status_mask))
> > -		return;
> > -
> > -	if ((pipestat & enable_mask) == 0)
> > -		return;
> > -
> > -	dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> > -
> > -	pipestat &= ~enable_mask;
> > -	I915_WRITE(reg, pipestat);
> > -	POSTING_READ(reg);
> > -}
> >  
> > -static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32 status_mask)
> > -{
> > -	u32 enable_mask = status_mask << 16;
> > +	if (INTEL_GEN(dev_priv) < 5)
> > +		goto out;
> >  
> >  	/*
> >  	 * On pipe A we don't support the PSR interrupt yet,
> > @@ -645,35 +599,59 @@ static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32 status_mask)
> >  	if (status_mask & SPRITE1_FLIP_DONE_INT_STATUS_VLV)
> >  		enable_mask |= SPRITE1_FLIP_DONE_INT_EN_VLV;
> >  
> > +out:
> > +	WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> > +		  status_mask & ~PIPESTAT_INT_STATUS_MASK,
> > +		  "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
> > +		  pipe_name(pipe), enable_mask, status_mask);
> > +
> >  	return enable_mask;
> >  }
> >  
> > -void
> > -i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> > -		     u32 status_mask)
> > +void i915_enable_pipestat(struct drm_i915_private *dev_priv,
> > +			  enum pipe pipe, u32 status_mask)
> >  {
> > +	i915_reg_t reg = PIPESTAT(pipe);
> >  	u32 enable_mask;
> >  
> > -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > -		enable_mask = vlv_get_pipestat_enable_mask(&dev_priv->drm,
> > -							   status_mask);
> > -	else
> > -		enable_mask = status_mask << 16;
> > -	__i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
> > +	WARN_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK,
> > +		  "pipe %c: status_mask=0x%x\n",
> > +		  pipe_name(pipe), status_mask);
> > +
> > +	lockdep_assert_held(&dev_priv->irq_lock);
> > +	WARN_ON(!intel_irqs_enabled(dev_priv));
> > +
> > +	if ((dev_priv->pipestat_irq_mask[pipe] & status_mask) == status_mask)
> > +		return;
> > +
> > +	dev_priv->pipestat_irq_mask[pipe] |= status_mask;
> > +	enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
> > +
> > +	I915_WRITE(reg, enable_mask | status_mask);
> > +	POSTING_READ(reg);
> >  }
> >  
> > -void
> > -i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> > -		      u32 status_mask)
> > +void i915_disable_pipestat(struct drm_i915_private *dev_priv,
> > +			   enum pipe pipe, u32 status_mask)
> >  {
> > +	i915_reg_t reg = PIPESTAT(pipe);
> >  	u32 enable_mask;
> >  
> > -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > -		enable_mask = vlv_get_pipestat_enable_mask(&dev_priv->drm,
> > -							   status_mask);
> > -	else
> > -		enable_mask = status_mask << 16;
> > -	__i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
> > +	WARN_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK,
> > +		  "pipe %c: status_mask=0x%x\n",
> > +		  pipe_name(pipe), status_mask);
> > +
> > +	lockdep_assert_held(&dev_priv->irq_lock);
> > +	WARN_ON(!intel_irqs_enabled(dev_priv));
> > +
> > +	if ((dev_priv->pipestat_irq_mask[pipe] & status_mask) == 0)
> > +		return;
> > +
> > +	dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> > +	enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
> > +
> > +	I915_WRITE(reg, enable_mask | status_mask);
> > +	POSTING_READ(reg);
> >  }
> >  
> >  /**
> > @@ -1769,7 +1747,7 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> >  
> >  	for_each_pipe(dev_priv, pipe) {
> >  		i915_reg_t reg;
> > -		u32 mask, iir_bit = 0;
> > +		u32 status_mask, enable_mask, iir_bit = 0;
> >  
> >  		/*
> >  		 * PIPESTAT bits get signalled even when the interrupt is
> > @@ -1780,7 +1758,7 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> >  		 */
> >  
> >  		/* fifo underruns are filterered in the underrun handler. */
> > -		mask = PIPE_FIFO_UNDERRUN_STATUS;
> > +		status_mask = PIPE_FIFO_UNDERRUN_STATUS;
> >  
> >  		switch (pipe) {
> >  		case PIPE_A:
> > @@ -1794,21 +1772,20 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> >  			break;
> >  		}
> >  		if (iir & iir_bit)
> > -			mask |= dev_priv->pipestat_irq_mask[pipe];
> > +			status_mask |= dev_priv->pipestat_irq_mask[pipe];
> >  
> > -		if (!mask)
> > +		if (!status_mask)
> >  			continue;
> 
> Not introduced here, but the above could be removed.

Indeed. I guess a separate patch would be cleaner.

> The patch looks ok:
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Thanks. Pushed to dinq.

> 
> >  
> >  		reg = PIPESTAT(pipe);
> > -		mask |= PIPESTAT_INT_ENABLE_MASK;
> > -		pipe_stats[pipe] = I915_READ(reg) & mask;
> > +		pipe_stats[pipe] = I915_READ(reg) & status_mask;
> > +		enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
> >  
> >  		/*
> >  		 * Clear the PIPE*STAT regs before the IIR
> >  		 */
> > -		if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
> > -					PIPESTAT_INT_STATUS_MASK))
> > -			I915_WRITE(reg, pipe_stats[pipe]);
> > +		if (pipe_stats[pipe])
> > +			I915_WRITE(reg, enable_mask | pipe_stats[pipe]);
> >  	}
> >  	spin_unlock(&dev_priv->irq_lock);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > index 04689600e337..77c123cc8817 100644
> > --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > @@ -88,14 +88,15 @@ static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	i915_reg_t reg = PIPESTAT(crtc->pipe);
> > -	u32 pipestat = I915_READ(reg) & 0xffff0000;
> > +	u32 enable_mask;
> >  
> >  	lockdep_assert_held(&dev_priv->irq_lock);
> >  
> > -	if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
> > +	if ((I915_READ(reg) & PIPE_FIFO_UNDERRUN_STATUS) == 0)
> >  		return;
> >  
> > -	I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
> > +	enable_mask = i915_pipestat_enable_mask(dev_priv, crtc->pipe);
> > +	I915_WRITE(reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
> >  	POSTING_READ(reg);
> >  
> >  	trace_intel_cpu_fifo_underrun(dev_priv, crtc->pipe);
> > @@ -108,15 +109,16 @@ static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	i915_reg_t reg = PIPESTAT(pipe);
> > -	u32 pipestat = I915_READ(reg) & 0xffff0000;
> >  
> >  	lockdep_assert_held(&dev_priv->irq_lock);
> >  
> >  	if (enable) {
> > -		I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
> > +		u32 enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
> > +
> > +		I915_WRITE(reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
> >  		POSTING_READ(reg);
> >  	} else {
> > -		if (old && pipestat & PIPE_FIFO_UNDERRUN_STATUS)
> > +		if (old && I915_READ(reg) & PIPE_FIFO_UNDERRUN_STATUS)
> >  			DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
> >  	}
> >  }
> > -- 
> > 2.13.5
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't rmw PIPESTAT enable bits

[Ville Syrjälä]

On Fri, Sep 15, 2017 at 01:03:36PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 14, 2017 at 09:37:37PM +0100, Chris Wilson wrote:
> > Quoting Ville Syrjala (2017-09-14 16:17:31)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > i830 seems to occasionally forget the PIPESTAT enable bits when
> > > we read the register. These aren't the only registers on i830 that
> > > have problems with RMW, as reading the double buffered plane
> > > registers returns the latched value rather than the last written
> > > value. So something similar is perhaps going on with PIPESTAT.
> > > 
> > > This corruption results on vblank interrupts occasionally turning off
> > > on their own, which leads to vblank timeouts and generally a stuck
> > > display subsystem.
> > > 
> > > So let's not RMW the pipestat enable bits, and instead use the cached
> > > copy we have around.
> > > 
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Well it didn't make my 845g any worse. Still has
> > 
> > [  245.683349] [drm:pipe_config_err] *ERROR* mismatch in base.adjusted_mode.flags (2) (expected 0, found 2)
> > [  245.683382] [drm:pipe_config_err] *ERROR* mismatch in base.adjusted_mode.flags (8) (expected 0, found 8)
> > [  245.683427] pipe state doesn't match!
> > 
> > if you are interested.
> 
> That's a bit odd. Even if the mode originally doesn't have any
> hsync/vsync flags intel_modeset_pipe_config() should give it some.
> So I can't immediately see how we could manage to get there with
> expected==0.

Hmm. Does you 845g have DVO encoders? That could potentially explain if
it ends up using intel_crtc_mode_get(). That one fails to read out the
sync flags correctly, and it has other issues as well.

https://patchwork.freedesktop.org/series/5183/ but not sure it applies
cleanly anymore.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't rmw PIPESTAT enable bits

[Chris Wilson]

Quoting Ville Syrjälä (2017-09-25 15:05:51)
> On Fri, Sep 15, 2017 at 01:03:36PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 14, 2017 at 09:37:37PM +0100, Chris Wilson wrote:
> > > Quoting Ville Syrjala (2017-09-14 16:17:31)
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > i830 seems to occasionally forget the PIPESTAT enable bits when
> > > > we read the register. These aren't the only registers on i830 that
> > > > have problems with RMW, as reading the double buffered plane
> > > > registers returns the latched value rather than the last written
> > > > value. So something similar is perhaps going on with PIPESTAT.
> > > > 
> > > > This corruption results on vblank interrupts occasionally turning off
> > > > on their own, which leads to vblank timeouts and generally a stuck
> > > > display subsystem.
> > > > 
> > > > So let's not RMW the pipestat enable bits, and instead use the cached
> > > > copy we have around.
> > > > 
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Well it didn't make my 845g any worse. Still has
> > > 
> > > [  245.683349] [drm:pipe_config_err] *ERROR* mismatch in base.adjusted_mode.flags (2) (expected 0, found 2)
> > > [  245.683382] [drm:pipe_config_err] *ERROR* mismatch in base.adjusted_mode.flags (8) (expected 0, found 8)
> > > [  245.683427] pipe state doesn't match!
> > > 
> > > if you are interested.
> > 
> > That's a bit odd. Even if the mode originally doesn't have any
> > hsync/vsync flags intel_modeset_pipe_config() should give it some.
> > So I can't immediately see how we could manage to get there with
> > expected==0.
> 
> Hmm. Does you 845g have DVO encoders? That could potentially explain if
> it ends up using intel_crtc_mode_get(). That one fails to read out the
> sync flags correctly, and it has other issues as well.

CRTC info
---------
CRTC 31: pipe: A, active=yes, (size=1024x768), dither=no, bpp=24
	fb: 39, pos: 0x0, size: 1024x768
	encoder 34: type: DVO C, connectors:
		connector 35: type: LVDS-1, status: connected, mode:
		id 0:"1024x768" freq 60 clock 65000 hdisp 1024 hss 1048 hse 1184 htot 1344 vdisp 768 vss 771 vse 777 vtot 806 type 0x8 flags 0x0
	cursor visible? no, position (0, 0), size 0x0, addr 0x00000000
	No scalers available on this platform
	--Plane id 27: type=PRI, crtc_pos=   0x   0, crtc_size=1024x 768, src_pos=0.0000x0.0000, src_size=1024.0000x768.0000, format=XR24 little-endian (0x34325258), rotation=0 (0x00000001)
	--Plane id 29: type=CUR, crtc_pos=   0x   0, crtc_size=   0x   0, src_pos=0.0000x0.0000, src_size=0.0000x0.0000, format=N/A, rotation=0 (0x00000001)
	underrun reporting: cpu=yes pch=yes 

Connector info
--------------
connector 35: type LVDS-1, status: connected
	name: 
	physical dimensions: 0x0mm
	subpixel order: Horizontal RGB
	CEA rev: 0
	modes:
		id 37:"1024x768" freq 60 clock 65000 hdisp 1024 hss 1048 hse 1184 htot 1344 vdisp 768 vss 771 vse 777 vtot 806 type 0x8 flags 0x0
connector 32: type VGA-1, status: unknown
	modes:

So yes, it appears LVDS is attached via DVO.

> https://patchwork.freedesktop.org/series/5183/ but not sure it applies
> cleanly anymore.

It has two chances...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx