[PATCH 0/5] drm/i915: Some FIFO underrun detection improvements

[PATCH 0/5] drm/i915: Some FIFO underrun detection improvements

[ville.syrjala]

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

I was looking at FIFO underruns a bit and came up with a few improvements to
the FIFO underrun reporting code. Mainly this should lead to detecting FIFO
a bit more more reliably on gmch platforms.

I also promoted the IVB/CPT uncleared FIFO underrun messages to errors
since that's what we do everywhere else as well.

Ville Syrjälä (5):
  drm/i915: Check for FIFO underuns when disabling reporting on gmch
    platforms
  drm/i915: Check for FIFO underruns at the end of modeset on gmch
  drm/i915: Convert uncleared FIFO underrun message to errors
  drm/i915: Simplify the uncleared FIFO underrun detection
  drm/i915: Shuffle fifo underrun disable/enable points for gmch
    platforms

 drivers/gpu/drm/i915/i915_irq.c      | 67 +++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 80 insertions(+), 24 deletions(-)

-- 
1.8.5.5

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

[PATCH 1/5] drm/i915: Check for FIFO underuns when disabling reporting on gmch platforms

[ville.syrjala]

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

FIFO underruns don't generate an interrupt on gmch platforms, so we
should check whether there were any that we failed to notice when
we're disabling FIFO underrun reporting.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b10fbde..8bb564b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -266,16 +266,22 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
 	return true;
 }
 
-static void i9xx_clear_fifo_underrun(struct drm_device *dev, enum pipe pipe)
+static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
+					     enum pipe pipe, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 reg = PIPESTAT(pipe);
-	u32 pipestat = I915_READ(reg) & 0x7fff0000;
+	u32 pipestat = I915_READ(reg) & 0xffff0000;
 
 	assert_spin_locked(&dev_priv->irq_lock);
 
-	I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
-	POSTING_READ(reg);
+	if (enable) {
+		I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
+		POSTING_READ(reg);
+	} else {
+		if (pipestat & PIPE_FIFO_UNDERRUN_STATUS)
+			DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
+	}
 }
 
 static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
@@ -432,8 +438,8 @@ bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
 
 	intel_crtc->cpu_fifo_underrun_disabled = !enable;
 
-	if (enable && (INTEL_INFO(dev)->gen < 5 || IS_VALLEYVIEW(dev)))
-		i9xx_clear_fifo_underrun(dev, pipe);
+	if (INTEL_INFO(dev)->gen < 5 || IS_VALLEYVIEW(dev))
+		i9xx_set_fifo_underrun_reporting(dev, pipe, enable);
 	else if (IS_GEN5(dev) || IS_GEN6(dev))
 		ironlake_set_fifo_underrun_reporting(dev, pipe, enable);
 	else if (IS_GEN7(dev))
-- 
1.8.5.5

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

Re: [PATCH 1/5] drm/i915: Check for FIFO underuns when disabling reporting on gmch platforms

[Thomas Wood]

On 16 May 2014 17:40,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> FIFO underruns don't generate an interrupt on gmch platforms, so we
> should check whether there were any that we failed to notice when
> we're disabling FIFO underrun reporting.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


Reviewed-by: Thomas Wood <thomas.wood@intel.com>


> ---
>  drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b10fbde..8bb564b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -266,16 +266,22 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
>         return true;
>  }
>
> -static void i9xx_clear_fifo_underrun(struct drm_device *dev, enum pipe pipe)
> +static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
> +                                            enum pipe pipe, bool enable)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 reg = PIPESTAT(pipe);
> -       u32 pipestat = I915_READ(reg) & 0x7fff0000;
> +       u32 pipestat = I915_READ(reg) & 0xffff0000;
>
>         assert_spin_locked(&dev_priv->irq_lock);
>
> -       I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
> -       POSTING_READ(reg);
> +       if (enable) {
> +               I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
> +               POSTING_READ(reg);
> +       } else {
> +               if (pipestat & PIPE_FIFO_UNDERRUN_STATUS)
> +                       DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
> +       }
>  }
>
>  static void ironlake_set_fifo_underrun_reporting(struct drm_device *dev,
> @@ -432,8 +438,8 @@ bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
>
>         intel_crtc->cpu_fifo_underrun_disabled = !enable;
>
> -       if (enable && (INTEL_INFO(dev)->gen < 5 || IS_VALLEYVIEW(dev)))
> -               i9xx_clear_fifo_underrun(dev, pipe);
> +       if (INTEL_INFO(dev)->gen < 5 || IS_VALLEYVIEW(dev))
> +               i9xx_set_fifo_underrun_reporting(dev, pipe, enable);
>         else if (IS_GEN5(dev) || IS_GEN6(dev))
>                 ironlake_set_fifo_underrun_reporting(dev, pipe, enable);
>         else if (IS_GEN7(dev))
> --
> 1.8.5.5
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915: Check for FIFO underruns at the end of modeset on gmch

[ville.syrjala]

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

FIFO underruns don't generate interrupts on gmch platforms, so
if we want to know whether a modeset triggered FIFO underruns we
need to explicitly check for them.

As a modeset on one pipe could cause underruns on other pipes,
check for underruns on all pipes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  6 ++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8bb564b..fdce260 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -266,6 +266,34 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
 	return true;
 }
 
+void i9xx_check_fifo_underruns(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *crtc;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
+
+	for_each_intel_crtc(dev, crtc) {
+		u32 reg = PIPESTAT(crtc->pipe);
+		u32 pipestat;
+
+		if (crtc->cpu_fifo_underrun_disabled)
+			continue;
+
+		pipestat = I915_READ(reg) & 0xffff0000;
+		if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
+			continue;
+
+		I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
+		POSTING_READ(reg);
+
+		DRM_ERROR("pipe %c underrun\n", pipe_name(crtc->pipe));
+	}
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
+}
+
 static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
 					     enum pipe pipe, bool enable)
 {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0f8f9bc..1b5164c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4545,6 +4545,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_enable_planes(crtc);
 
 	drm_vblank_on(dev, pipe);
+
+	/* Underruns don't raise interrupts, so check manually. */
+	i9xx_check_fifo_underruns(dev);
 }
 
 static void i9xx_crtc_enable(struct drm_crtc *crtc)
@@ -4581,6 +4584,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_enable_planes(crtc);
 
 	drm_vblank_on(dev, pipe);
+
+	/* Underruns don't raise interrupts, so check manually. */
+	i9xx_check_fifo_underruns(dev);
 }
 
 static void i9xx_pfit_disable(struct intel_crtc *crtc)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 32a74e1..db0a74d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -671,6 +671,7 @@ void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
 void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
 int intel_get_crtc_scanline(struct intel_crtc *crtc);
+void i9xx_check_fifo_underruns(struct drm_device *dev);
 
 
 /* intel_crt.c */
-- 
1.8.5.5

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

Re: [PATCH 2/5] drm/i915: Check for FIFO underruns at the end of modeset on gmch

[Thomas Wood]

On 16 May 2014 17:40,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> FIFO underruns don't generate interrupts on gmch platforms, so
> if we want to know whether a modeset triggered FIFO underruns we
> need to explicitly check for them.
>
> As a modeset on one pipe could cause underruns on other pipes,
> check for underruns on all pipes.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


Reviewed-by: Thomas Wood <thomas.wood@intel.com>


> ---
>  drivers/gpu/drm/i915/i915_irq.c      | 28 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  6 ++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 35 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 8bb564b..fdce260 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -266,6 +266,34 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
>         return true;
>  }
>
> +void i9xx_check_fifo_underruns(struct drm_device *dev)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_crtc *crtc;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&dev_priv->irq_lock, flags);
> +
> +       for_each_intel_crtc(dev, crtc) {
> +               u32 reg = PIPESTAT(crtc->pipe);
> +               u32 pipestat;
> +
> +               if (crtc->cpu_fifo_underrun_disabled)
> +                       continue;
> +
> +               pipestat = I915_READ(reg) & 0xffff0000;
> +               if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
> +                       continue;
> +
> +               I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
> +               POSTING_READ(reg);
> +
> +               DRM_ERROR("pipe %c underrun\n", pipe_name(crtc->pipe));
> +       }
> +
> +       spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> +}
> +
>  static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
>                                              enum pipe pipe, bool enable)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0f8f9bc..1b5164c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4545,6 +4545,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>         intel_crtc_enable_planes(crtc);
>
>         drm_vblank_on(dev, pipe);
> +
> +       /* Underruns don't raise interrupts, so check manually. */
> +       i9xx_check_fifo_underruns(dev);
>  }
>
>  static void i9xx_crtc_enable(struct drm_crtc *crtc)
> @@ -4581,6 +4584,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>         intel_crtc_enable_planes(crtc);
>
>         drm_vblank_on(dev, pipe);
> +
> +       /* Underruns don't raise interrupts, so check manually. */
> +       i9xx_check_fifo_underruns(dev);
>  }
>
>  static void i9xx_pfit_disable(struct intel_crtc *crtc)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 32a74e1..db0a74d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -671,6 +671,7 @@ void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>  void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
>  void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
>  int intel_get_crtc_scanline(struct intel_crtc *crtc);
> +void i9xx_check_fifo_underruns(struct drm_device *dev);
>
>
>  /* intel_crt.c */
> --
> 1.8.5.5
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Convert uncleared FIFO underrun message to errors

[ville.syrjala]

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

Some platforms have a shared error interrupt, so if FIFO underrun
reporting gets disabled for one pipe/transcoder it gets disabled
for all pipes/transcoders.

When we disable FIFO underrun reporting we check whether the
interrupt was enabled or not. If it wasn't we might have missed
an underrun and we perform one last check right there. Currently
we print a debug message when an underrun is detect using this
mechanism. Promote the message to DRM_ERROR() to match the other
underrun error messages.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index fdce260..862964f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -344,8 +344,8 @@ static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
 
 		if (!was_enabled &&
 		    (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe))) {
-			DRM_DEBUG_KMS("uncleared fifo underrun on pipe %c\n",
-				      pipe_name(pipe));
+			DRM_ERROR("uncleared fifo underrun on pipe %c\n",
+				  pipe_name(pipe));
 		}
 	}
 }
@@ -429,8 +429,8 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 
 		if (!was_enabled &&
 		    (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
-			DRM_DEBUG_KMS("uncleared pch fifo underrun on pch transcoder %c\n",
-				      transcoder_name(pch_transcoder));
+			DRM_ERROR("uncleared pch fifo underrun on pch transcoder %c\n",
+				  transcoder_name(pch_transcoder));
 		}
 	}
 }
-- 
1.8.5.5

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

Re: [PATCH 3/5] drm/i915: Convert uncleared FIFO underrun message to errors

[Daniel Vetter]

On Fri, May 16, 2014 at 07:40:23PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Some platforms have a shared error interrupt, so if FIFO underrun
> reporting gets disabled for one pipe/transcoder it gets disabled
> for all pipes/transcoders.
> 
> When we disable FIFO underrun reporting we check whether the
> interrupt was enabled or not. If it wasn't we might have missed
> an underrun and we perform one last check right there. Currently
> we print a debug message when an underrun is detect using this
> mechanism. Promote the message to DRM_ERROR() to match the other
> underrun error messages.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index fdce260..862964f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -344,8 +344,8 @@ static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
>  
>  		if (!was_enabled &&
>  		    (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe))) {
> -			DRM_DEBUG_KMS("uncleared fifo underrun on pipe %c\n",
> -				      pipe_name(pipe));
> +			DRM_ERROR("uncleared fifo underrun on pipe %c\n",
> +				  pipe_name(pipe));
>  		}
>  	}
>  }
> @@ -429,8 +429,8 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
>  
>  		if (!was_enabled &&
>  		    (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
> -			DRM_DEBUG_KMS("uncleared pch fifo underrun on pch transcoder %c\n",
> -				      transcoder_name(pch_transcoder));
> +			DRM_ERROR("uncleared pch fifo underrun on pch transcoder %c\n",
> +				  transcoder_name(pch_transcoder));
>  		}
>  	}
>  }
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 4/5] drm/i915: Simplify the uncleared FIFO underrun detection

[ville.syrjala]

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

Checking whether the error interrupt was enabled or not isn't really
necessary when we check for uncleared FIFO underruns. If it was enabled
we'll race with the interrupt handler a bit, but that seems OK as we
still claim the interrupt.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 862964f..dd6e359 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -337,13 +337,9 @@ static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
 
 		ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
 	} else {
-		bool was_enabled = !(I915_READ(DEIMR) & DE_ERR_INT_IVB);
-
-		/* Change the state _after_ we've read out the current one. */
 		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
 
-		if (!was_enabled &&
-		    (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe))) {
+		if (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe)) {
 			DRM_ERROR("uncleared fifo underrun on pipe %c\n",
 				  pipe_name(pipe));
 		}
@@ -421,14 +417,9 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
 
 		ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
 	} else {
-		uint32_t tmp = I915_READ(SERR_INT);
-		bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT);
-
-		/* Change the state _after_ we've read out the current one. */
 		ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
 
-		if (!was_enabled &&
-		    (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
+		if (I915_READ(SERR_INT) & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) {
 			DRM_ERROR("uncleared pch fifo underrun on pch transcoder %c\n",
 				  transcoder_name(pch_transcoder));
 		}
-- 
1.8.5.5

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

Re: [PATCH 4/5] drm/i915: Simplify the uncleared FIFO underrun detection

[Thomas Wood]

On 16 May 2014 17:40,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Checking whether the error interrupt was enabled or not isn't really
> necessary when we check for uncleared FIFO underruns. If it was enabled
> we'll race with the interrupt handler a bit, but that seems OK as we
> still claim the interrupt.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


checkpatch.pl complains that there are lines over 80 characters, but otherwise:

Reviewed-by: Thomas Wood <thomas.wood@intel.com>


> ---
>  drivers/gpu/drm/i915/i915_irq.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 862964f..dd6e359 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -337,13 +337,9 @@ static void ivybridge_set_fifo_underrun_reporting(struct drm_device *dev,
>
>                 ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
>         } else {
> -               bool was_enabled = !(I915_READ(DEIMR) & DE_ERR_INT_IVB);
> -
> -               /* Change the state _after_ we've read out the current one. */
>                 ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>
> -               if (!was_enabled &&
> -                   (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe))) {
> +               if (I915_READ(GEN7_ERR_INT) & ERR_INT_FIFO_UNDERRUN(pipe)) {
>                         DRM_ERROR("uncleared fifo underrun on pipe %c\n",
>                                   pipe_name(pipe));
>                 }
> @@ -421,14 +417,9 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
>
>                 ibx_enable_display_interrupt(dev_priv, SDE_ERROR_CPT);
>         } else {
> -               uint32_t tmp = I915_READ(SERR_INT);
> -               bool was_enabled = !(I915_READ(SDEIMR) & SDE_ERROR_CPT);
> -
> -               /* Change the state _after_ we've read out the current one. */
>                 ibx_disable_display_interrupt(dev_priv, SDE_ERROR_CPT);
>
> -               if (!was_enabled &&
> -                   (tmp & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder))) {
> +               if (I915_READ(SERR_INT) & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) {
>                         DRM_ERROR("uncleared pch fifo underrun on pch transcoder %c\n",
>                                   transcoder_name(pch_transcoder));
>                 }
> --
> 1.8.5.5
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/5] drm/i915: Shuffle fifo underrun disable/enable points for gmch platforms

[ville.syrjala]

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

Gen2 reports FIFO underruns whenever no planes are enabled on the pipe.
So in order to avoid false positives we must enable the FIFO underrun
reporting only when at least one plane is enabled on the pipe. For
now just move the underrun reporting enable/disable points to the
other side of the plane enable/disable point. That doesn't cover cases
when we turn off all the planes for the pipe but leave the pipe running
on purpose, but it's better than the current situation.

On gen4+ we can actually move the underrun reporting enable/disable to
the opposite ends of the crtc enable/disable hooks. I suppose in theory
we could leave the underrun reporting enabled all the time, except on
VLV where PIPESTAT stops working when the display power well is down.
If we ever get around to unifying the PIPESTAT irq handling for all
gmch platforms, we should still follow the VLV route for other platforms.
It would also micro-optimize the irq handler a bit since we could then
skip the PIPESTAT reads for all disabled pipes.

Gen3 is still a mystery, but for now I'm going to assume it behaves
like gen4+.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1b5164c..7f61047 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4514,6 +4514,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_crtc->active = true;
 
+	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_pll_enable)
 			encoder->pre_pll_enable(encoder);
@@ -4537,7 +4539,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(intel_crtc);
-	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
@@ -4564,6 +4565,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	intel_crtc->active = true;
 
+	if (!IS_GEN2(dev))
+		intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
 			encoder->pre_enable(encoder);
@@ -4576,13 +4580,22 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(intel_crtc);
-	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
 
 	intel_crtc_enable_planes(crtc);
 
+	/*
+	 * Gen2 reports pipe underruns whenever all planes are disabled.
+	 * So don't enable underrun reporting before at least some planes
+	 * are enabled.
+	 * FIXME: Need to fix the logic to work when we turn off all planes
+	 * but leave the pipe running.
+	 */
+	if (IS_GEN2(dev))
+		intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
+
 	drm_vblank_on(dev, pipe);
 
 	/* Underruns don't raise interrupts, so check manually. */
@@ -4615,12 +4628,20 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
+	/*
+	 * Gen2 reports pipe underruns whenever all planes are disabled.
+	 * So diasble underrun reporting before all the planes get disabled.
+	 * FIXME: Need to fix the logic to work when we turn off all planes
+	 * but leave the pipe running.
+	 */
+	if (IS_GEN2(dev))
+		intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
+
 	intel_crtc_disable_planes(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->disable(encoder);
 
-	intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
 	intel_disable_pipe(dev_priv, pipe);
 
 	i9xx_pfit_disable(intel_crtc);
@@ -4638,6 +4659,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 			i9xx_disable_pll(dev_priv, pipe);
 	}
 
+	if (!IS_GEN2(dev))
+		intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
+
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
-- 
1.8.5.5

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

Re: [PATCH 5/5] drm/i915: Shuffle fifo underrun disable/enable points for gmch platforms

[Thomas Wood]

On 16 May 2014 17:40,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Gen2 reports FIFO underruns whenever no planes are enabled on the pipe.
> So in order to avoid false positives we must enable the FIFO underrun
> reporting only when at least one plane is enabled on the pipe. For
> now just move the underrun reporting enable/disable points to the
> other side of the plane enable/disable point. That doesn't cover cases
> when we turn off all the planes for the pipe but leave the pipe running
> on purpose, but it's better than the current situation.
>
> On gen4+ we can actually move the underrun reporting enable/disable to
> the opposite ends of the crtc enable/disable hooks. I suppose in theory
> we could leave the underrun reporting enabled all the time, except on
> VLV where PIPESTAT stops working when the display power well is down.
> If we ever get around to unifying the PIPESTAT irq handling for all
> gmch platforms, we should still follow the VLV route for other platforms.
> It would also micro-optimize the irq handler a bit since we could then
> skip the PIPESTAT reads for all disabled pipes.
>
> Gen3 is still a mystery, but for now I'm going to assume it behaves
> like gen4+.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


This doesn't apply to drm-intel-nightly, but looks fine in principle:

Reviewed-by: Thomas Wood <thomas.wood@intel.com>




> ---
>  drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1b5164c..7f61047 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4514,6 +4514,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>
>         intel_crtc->active = true;
>
> +       intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> +
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_pll_enable)
>                         encoder->pre_pll_enable(encoder);
> @@ -4537,7 +4539,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>
>         intel_update_watermarks(crtc);
>         intel_enable_pipe(intel_crtc);
> -       intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 encoder->enable(encoder);
> @@ -4564,6 +4565,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>
>         intel_crtc->active = true;
>
> +       if (!IS_GEN2(dev))
> +               intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> +
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_enable)
>                         encoder->pre_enable(encoder);
> @@ -4576,13 +4580,22 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>
>         intel_update_watermarks(crtc);
>         intel_enable_pipe(intel_crtc);
> -       intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 encoder->enable(encoder);
>
>         intel_crtc_enable_planes(crtc);
>
> +       /*
> +        * Gen2 reports pipe underruns whenever all planes are disabled.
> +        * So don't enable underrun reporting before at least some planes
> +        * are enabled.
> +        * FIXME: Need to fix the logic to work when we turn off all planes
> +        * but leave the pipe running.
> +        */
> +       if (IS_GEN2(dev))
> +               intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> +
>         drm_vblank_on(dev, pipe);
>
>         /* Underruns don't raise interrupts, so check manually. */
> @@ -4615,12 +4628,20 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>         if (!intel_crtc->active)
>                 return;
>
> +       /*
> +        * Gen2 reports pipe underruns whenever all planes are disabled.
> +        * So diasble underrun reporting before all the planes get disabled.
> +        * FIXME: Need to fix the logic to work when we turn off all planes
> +        * but leave the pipe running.
> +        */
> +       if (IS_GEN2(dev))
> +               intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
> +
>         intel_crtc_disable_planes(crtc);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 encoder->disable(encoder);
>
> -       intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
>         intel_disable_pipe(dev_priv, pipe);
>
>         i9xx_pfit_disable(intel_crtc);
> @@ -4638,6 +4659,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>                         i9xx_disable_pll(dev_priv, pipe);
>         }
>
> +       if (!IS_GEN2(dev))
> +               intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
> +
>         intel_crtc->active = false;
>         intel_update_watermarks(crtc);
>
> --
> 1.8.5.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Shuffle fifo underrun disable/enable points for gmch platforms

[Daniel Vetter]

On Fri, May 16, 2014 at 07:40:25PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Gen2 reports FIFO underruns whenever no planes are enabled on the pipe.
> So in order to avoid false positives we must enable the FIFO underrun
> reporting only when at least one plane is enabled on the pipe. For
> now just move the underrun reporting enable/disable points to the
> other side of the plane enable/disable point. That doesn't cover cases
> when we turn off all the planes for the pipe but leave the pipe running
> on purpose, but it's better than the current situation.
> 
> On gen4+ we can actually move the underrun reporting enable/disable to
> the opposite ends of the crtc enable/disable hooks. I suppose in theory
> we could leave the underrun reporting enabled all the time, except on
> VLV where PIPESTAT stops working when the display power well is down.
> If we ever get around to unifying the PIPESTAT irq handling for all
> gmch platforms, we should still follow the VLV route for other platforms.
> It would also micro-optimize the irq handler a bit since we could then
> skip the PIPESTAT reads for all disabled pipes.
> 
> Gen3 is still a mystery, but for now I'm going to assume it behaves
> like gen4+.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1b5164c..7f61047 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4514,6 +4514,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  
>  	intel_crtc->active = true;
>  
> +	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> +
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		if (encoder->pre_pll_enable)
>  			encoder->pre_pll_enable(encoder);
> @@ -4537,7 +4539,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  
>  	intel_update_watermarks(crtc);
>  	intel_enable_pipe(intel_crtc);
> -	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> @@ -4564,6 +4565,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  
>  	intel_crtc->active = true;
>  
> +	if (!IS_GEN2(dev))
> +		intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> +
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		if (encoder->pre_enable)
>  			encoder->pre_enable(encoder);
> @@ -4576,13 +4580,22 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  
>  	intel_update_watermarks(crtc);
>  	intel_enable_pipe(intel_crtc);
> -	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
>  
>  	intel_crtc_enable_planes(crtc);
>  
> +	/*
> +	 * Gen2 reports pipe underruns whenever all planes are disabled.
> +	 * So don't enable underrun reporting before at least some planes
> +	 * are enabled.
> +	 * FIXME: Need to fix the logic to work when we turn off all planes
> +	 * but leave the pipe running.
> +	 */
> +	if (IS_GEN2(dev))
> +		intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);

I guess we should do this as part of the nuclear pageflip code iff all
planes are off. But for now this looks good enough imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/i915: Shuffle fifo underrun disable/enable points for gmch platforms

[Daniel Vetter]

On Wed, May 21, 2014 at 04:52:31PM +0200, Daniel Vetter wrote:
> On Fri, May 16, 2014 at 07:40:25PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Gen2 reports FIFO underruns whenever no planes are enabled on the pipe.
> > So in order to avoid false positives we must enable the FIFO underrun
> > reporting only when at least one plane is enabled on the pipe. For
> > now just move the underrun reporting enable/disable points to the
> > other side of the plane enable/disable point. That doesn't cover cases
> > when we turn off all the planes for the pipe but leave the pipe running
> > on purpose, but it's better than the current situation.
> > 
> > On gen4+ we can actually move the underrun reporting enable/disable to
> > the opposite ends of the crtc enable/disable hooks. I suppose in theory
> > we could leave the underrun reporting enabled all the time, except on
> > VLV where PIPESTAT stops working when the display power well is down.
> > If we ever get around to unifying the PIPESTAT irq handling for all
> > gmch platforms, we should still follow the VLV route for other platforms.
> > It would also micro-optimize the irq handler a bit since we could then
> > skip the PIPESTAT reads for all disabled pipes.
> > 
> > Gen3 is still a mystery, but for now I'm going to assume it behaves
> > like gen4+.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++++++++++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1b5164c..7f61047 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4514,6 +4514,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	intel_crtc->active = true;
> >  
> > +	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> > +
> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> >  		if (encoder->pre_pll_enable)
> >  			encoder->pre_pll_enable(encoder);
> > @@ -4537,7 +4539,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	intel_update_watermarks(crtc);
> >  	intel_enable_pipe(intel_crtc);
> > -	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> >  
> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> >  		encoder->enable(encoder);
> > @@ -4564,6 +4565,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	intel_crtc->active = true;
> >  
> > +	if (!IS_GEN2(dev))
> > +		intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> > +
> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> >  		if (encoder->pre_enable)
> >  			encoder->pre_enable(encoder);
> > @@ -4576,13 +4580,22 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	intel_update_watermarks(crtc);
> >  	intel_enable_pipe(intel_crtc);
> > -	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> >  
> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> >  		encoder->enable(encoder);
> >  
> >  	intel_crtc_enable_planes(crtc);
> >  
> > +	/*
> > +	 * Gen2 reports pipe underruns whenever all planes are disabled.
> > +	 * So don't enable underrun reporting before at least some planes
> > +	 * are enabled.
> > +	 * FIXME: Need to fix the logic to work when we turn off all planes
> > +	 * but leave the pipe running.
> > +	 */
> > +	if (IS_GEN2(dev))
> > +		intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
> 
> I guess we should do this as part of the nuclear pageflip code iff all
> planes are off. But for now this looks good enough imo.

Forgotten to mention: All patches merged now, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch