[PATCH 0/4] Haswell modeset fixes

[PATCH 0/4] Haswell modeset fixes

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

After we solved the Haswell modeset hang, I started to see FIFO underruns. This
series addresses the problems I was able to spot and fix. I think these patches
may help solving the corruptions Linus reported.

Thanks,
Paulo

Paulo Zanoni (3):
  drm/i915: promote FIFO underruns to DRM_ERROR
  drm/i915: don't disable ERR_INT on the IRQ handler
  drm/i915: implement the Haswell mode set sequence workaround

Ville Syrjälä (1):
  drm/i915: Disable/enable planes as the first/last thing during modeset
    on HSW

 drivers/gpu/drm/i915/i915_irq.c      |  63 ++++++++--------------
 drivers/gpu/drm/i915/intel_display.c | 100 ++++++++++++++++++++++++++---------
 2 files changed, 98 insertions(+), 65 deletions(-)

-- 
1.8.3.1

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

[PATCH 1/4] drm/i915: promote FIFO underruns to DRM_ERROR

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Linus recently complained about screen corruption when coming out of
DPMS on his Haswell machine, and he also mentioned there were no error
messages on the log. I think I can reproduce this problem, and when it
happens I get a "FIFO underrun" message, but since it's just
DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
messages because reports containing "I'm getting a FIFO underrun on
pipe A" are way much better than "I'm getting a screen corruption".

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 44 ++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a42f30b..9ecc9d9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -263,8 +263,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));
 		}
 	}
 }
@@ -339,8 +339,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));
 		}
 	}
 }
@@ -1115,8 +1115,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 			 */
 			if (pipe_stats[pipe] & 0x8000ffff) {
 				if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS)
-					DRM_DEBUG_DRIVER("pipe %c underrun\n",
-							 pipe_name(pipe));
+					DRM_ERROR("pipe %c underrun\n",
+						  pipe_name(pipe));
 				I915_WRITE(reg, pipe_stats[pipe]);
 			}
 		}
@@ -1206,12 +1206,12 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 	if (pch_iir & SDE_TRANSA_FIFO_UNDER)
 		if (intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A,
 							  false))
-			DRM_DEBUG_DRIVER("PCH transcoder A FIFO underrun\n");
+			DRM_ERROR("PCH transcoder A FIFO underrun\n");
 
 	if (pch_iir & SDE_TRANSB_FIFO_UNDER)
 		if (intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_B,
 							  false))
-			DRM_DEBUG_DRIVER("PCH transcoder B FIFO underrun\n");
+			DRM_ERROR("PCH transcoder B FIFO underrun\n");
 }
 
 static void ivb_err_int_handler(struct drm_device *dev)
@@ -1224,15 +1224,15 @@ static void ivb_err_int_handler(struct drm_device *dev)
 
 	if (err_int & ERR_INT_FIFO_UNDERRUN_A)
 		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_A, false))
-			DRM_DEBUG_DRIVER("Pipe A FIFO underrun\n");
+			DRM_ERROR("Pipe A FIFO underrun\n");
 
 	if (err_int & ERR_INT_FIFO_UNDERRUN_B)
 		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_B, false))
-			DRM_DEBUG_DRIVER("Pipe B FIFO underrun\n");
+			DRM_ERROR("Pipe B FIFO underrun\n");
 
 	if (err_int & ERR_INT_FIFO_UNDERRUN_C)
 		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_C, false))
-			DRM_DEBUG_DRIVER("Pipe C FIFO underrun\n");
+			DRM_ERROR("Pipe C FIFO underrun\n");
 
 	I915_WRITE(GEN7_ERR_INT, err_int);
 }
@@ -1248,17 +1248,17 @@ static void cpt_serr_int_handler(struct drm_device *dev)
 	if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
 		if (intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A,
 							  false))
-			DRM_DEBUG_DRIVER("PCH transcoder A FIFO underrun\n");
+			DRM_ERROR("PCH transcoder A FIFO underrun\n");
 
 	if (serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN)
 		if (intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_B,
 							  false))
-			DRM_DEBUG_DRIVER("PCH transcoder B FIFO underrun\n");
+			DRM_ERROR("PCH transcoder B FIFO underrun\n");
 
 	if (serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN)
 		if (intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_C,
 							  false))
-			DRM_DEBUG_DRIVER("PCH transcoder C FIFO underrun\n");
+			DRM_ERROR("PCH transcoder C FIFO underrun\n");
 
 	I915_WRITE(SERR_INT, serr_int);
 }
@@ -1321,11 +1321,11 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	if (de_iir & DE_PIPEA_FIFO_UNDERRUN)
 		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_A, false))
-			DRM_DEBUG_DRIVER("Pipe A FIFO underrun\n");
+			DRM_ERROR("Pipe A FIFO underrun\n");
 
 	if (de_iir & DE_PIPEB_FIFO_UNDERRUN)
 		if (intel_set_cpu_fifo_underrun_reporting(dev, PIPE_B, false))
-			DRM_DEBUG_DRIVER("Pipe B FIFO underrun\n");
+			DRM_ERROR("Pipe B FIFO underrun\n");
 
 	if (de_iir & DE_PLANEA_FLIP_DONE) {
 		intel_prepare_page_flip(dev, 0);
@@ -2566,8 +2566,8 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
 			 */
 			if (pipe_stats[pipe] & 0x8000ffff) {
 				if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS)
-					DRM_DEBUG_DRIVER("pipe %c underrun\n",
-							 pipe_name(pipe));
+					DRM_ERROR("pipe %c underrun\n",
+						  pipe_name(pipe));
 				I915_WRITE(reg, pipe_stats[pipe]);
 			}
 		}
@@ -2737,8 +2737,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
 			/* Clear the PIPE*STAT regs before the IIR */
 			if (pipe_stats[pipe] & 0x8000ffff) {
 				if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS)
-					DRM_DEBUG_DRIVER("pipe %c underrun\n",
-							 pipe_name(pipe));
+					DRM_ERROR("pipe %c underrun\n",
+						  pipe_name(pipe));
 				I915_WRITE(reg, pipe_stats[pipe]);
 				irq_received = true;
 			}
@@ -2979,8 +2979,8 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 			 */
 			if (pipe_stats[pipe] & 0x8000ffff) {
 				if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS)
-					DRM_DEBUG_DRIVER("pipe %c underrun\n",
-							 pipe_name(pipe));
+					DRM_ERROR("pipe %c underrun\n",
+						  pipe_name(pipe));
 				I915_WRITE(reg, pipe_stats[pipe]);
 				irq_received = 1;
 			}
-- 
1.8.3.1

[PATCH 2/4] drm/i915: don't disable ERR_INT on the IRQ handler

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We currently disable the ERR_INT interrupts while running the IRQ
handler because we fear that if we do an unclaimed register access
from inside the IRQ handler we'll keep triggering the IRQ handler
forever.

The problem is that since we always disable the ERR_INT interrupts at
the IRQ handler, when we get a FIFO underrun we'll always print both
messages:
  - "uncleared fifo underrun on pipe A"
  - "Pipe A FIFO underrun"

Because the "was_enabled" variable from
ivybridge_set_fifo_underrun_reporting will always be false (since we
disable ERR int at the IRQ handler!).

Instead of actually fixing ivybridge_set_fifo_underrun_reporting,
let's just remove the "disable ERR_INT during the IRQ handler" code.
As far as we know we shouldn't really be triggering ERR_INT interrupts
from the IRQ handler, so if we ever get stuck in the endless loop of
interrupts we can git-bisect and revert (and we can even bisect and
revert this patch in case I'm just wrong). As a bonus, our IRQ handler
is now simpler and a few nanoseconds faster.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9ecc9d9..e9f6bc3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1394,7 +1394,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	u32 de_iir, gt_iir, de_ier, sde_ier = 0;
 	irqreturn_t ret = IRQ_NONE;
-	bool err_int_reenable = false;
 
 	atomic_inc(&dev_priv->irq_received);
 
@@ -1418,17 +1417,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 		POSTING_READ(SDEIER);
 	}
 
-	/* On Haswell, also mask ERR_INT because we don't want to risk
-	 * generating "unclaimed register" interrupts from inside the interrupt
-	 * handler. */
-	if (IS_HASWELL(dev)) {
-		spin_lock(&dev_priv->irq_lock);
-		err_int_reenable = ~dev_priv->irq_mask & DE_ERR_INT_IVB;
-		if (err_int_reenable)
-			ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
-		spin_unlock(&dev_priv->irq_lock);
-	}
-
 	gt_iir = I915_READ(GTIIR);
 	if (gt_iir) {
 		if (INTEL_INFO(dev)->gen >= 6)
@@ -1458,13 +1446,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 		}
 	}
 
-	if (err_int_reenable) {
-		spin_lock(&dev_priv->irq_lock);
-		if (ivb_can_enable_err_int(dev))
-			ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
-		spin_unlock(&dev_priv->irq_lock);
-	}
-
 	I915_WRITE(DEIER, de_ier);
 	POSTING_READ(DEIER);
 	if (!HAS_PCH_NOP(dev)) {
-- 
1.8.3.1

[PATCH 3/4] drm/i915: Disable/enable planes as the first/last thing during modeset on HSW

[Paulo Zanoni]

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

Refactor the plane enabling/disabling into helper functions and move
the calls to happen as the first thing during .crtc_disable, and the
last thing during .crtc_enable.

Those are the two clear points where we are sure that the pipe is
actually running regardless of the encoder type or hardware
generation.

v2: Made by Paulo:
  Remove the code touching everything but the Haswell functions. We
  need this change on Haswell right now since it fixes a FIFO underrun
  that we get on pipe A while we enable pipe B (see the workaround
  notes on the Haswell mode set sequence documentation). We can bring
  back the code to gens 2-7 later, once they're tested.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 69 +++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d55d041..8c3000d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3391,6 +3391,47 @@ static void hsw_disable_ips(struct intel_crtc *crtc)
 	intel_wait_for_vblank(dev, crtc->pipe);
 }
 
+static void haswell_crtc_enable_planes(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int pipe = intel_crtc->pipe;
+	int plane = intel_crtc->plane;
+
+	intel_enable_plane(dev_priv, plane, pipe);
+	intel_enable_planes(crtc);
+	intel_crtc_update_cursor(crtc, true);
+
+	hsw_enable_ips(intel_crtc);
+
+	mutex_lock(&dev->struct_mutex);
+	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
+}
+
+static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int pipe = intel_crtc->pipe;
+	int plane = intel_crtc->plane;
+
+	intel_crtc_wait_for_pending_flips(crtc);
+	drm_vblank_off(dev, pipe);
+
+	/* FBC must be disabled before disabling the plane on HSW. */
+	if (dev_priv->fbc.plane == plane)
+		intel_disable_fbc(dev);
+
+	hsw_disable_ips(intel_crtc);
+
+	intel_crtc_update_cursor(crtc, false);
+	intel_disable_planes(crtc);
+	intel_disable_plane(dev_priv, plane, pipe);
+}
+
 static void haswell_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -3398,7 +3439,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
-	int plane = intel_crtc->plane;
 
 	WARN_ON(!crtc->enabled);
 
@@ -3434,24 +3474,17 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe,
 			  intel_crtc->config.has_pch_encoder, false);
-	intel_enable_plane(dev_priv, plane, pipe);
-	intel_enable_planes(crtc);
-	intel_crtc_update_cursor(crtc, true);
-
-	hsw_enable_ips(intel_crtc);
 
 	if (intel_crtc->config.has_pch_encoder)
 		lpt_pch_enable(crtc);
 
-	mutex_lock(&dev->struct_mutex);
-	intel_update_fbc(dev);
-	mutex_unlock(&dev->struct_mutex);
-
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		encoder->enable(encoder);
 		intel_opregion_notify_encoder(encoder, true);
 	}
 
+	haswell_crtc_enable_planes(crtc);
+
 	/*
 	 * There seems to be a race in PCH platform hw (at least on some
 	 * outputs) where an enabled pipe still completes any pageflip right
@@ -3558,30 +3591,18 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
-	int plane = intel_crtc->plane;
 	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
 
 	if (!intel_crtc->active)
 		return;
 
+	haswell_crtc_disable_planes(crtc);
+
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		intel_opregion_notify_encoder(encoder, false);
 		encoder->disable(encoder);
 	}
 
-	intel_crtc_wait_for_pending_flips(crtc);
-	drm_vblank_off(dev, pipe);
-
-	/* FBC must be disabled before disabling the plane on HSW. */
-	if (dev_priv->fbc.plane == plane)
-		intel_disable_fbc(dev);
-
-	hsw_disable_ips(intel_crtc);
-
-	intel_crtc_update_cursor(crtc, false);
-	intel_disable_planes(crtc);
-	intel_disable_plane(dev_priv, plane, pipe);
-
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, false);
 	intel_disable_pipe(dev_priv, pipe);
-- 
1.8.3.1

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

[PATCH 4/4] drm/i915: implement the Haswell mode set sequence workaround

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This workaround is described in the mode set sequence documentation.
When enabling planes for the second pipe, we need to wait for 2
vblanks on the first pipe. This should solve "a flash of screen
corruption if planes are enabled on second/third pipe during the time
that big FIFO mode is exiting". Watermarks are fun :)

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8c3000d..fc55570 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3432,6 +3432,34 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
 	intel_disable_plane(dev_priv, plane, pipe);
 }
 
+/*
+ * This implements the workaround described in the "notes" section of the mode
+ * set sequence documentation. When going from no pipes or single pipe to
+ * multiple pipes, and planes are enabled after the pipe, we need to wait at
+ * least 2 vblanks on the first pipe before enabling planes on the second pipe.
+ */
+static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct intel_crtc *crtc_it, *other_active_crtc = NULL;
+
+	/* We want to get the other_active_crtc only if there's only 1 other
+	 * active crtc. */
+	list_for_each_entry(crtc_it, &dev->mode_config.crtc_list, base.head) {
+		if (crtc_it->active && crtc_it != crtc) {
+			if (other_active_crtc)
+				return;
+			else
+				other_active_crtc = crtc_it;
+		}
+	}
+	if (!other_active_crtc)
+		return;
+
+	intel_wait_for_vblank(dev, other_active_crtc->pipe);
+	intel_wait_for_vblank(dev, other_active_crtc->pipe);
+}
+
 static void haswell_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -3483,6 +3511,9 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 		intel_opregion_notify_encoder(encoder, true);
 	}
 
+	/* If we change the relative order between pipe/planes enabling, we need
+	 * to change the workaround. */
+	haswell_mode_set_planes_workaround(intel_crtc);
 	haswell_crtc_enable_planes(crtc);
 
 	/*
-- 
1.8.3.1

Re: [PATCH 1/4] drm/i915: promote FIFO underruns to DRM_ERROR

[Chris Wilson]

On Thu, Sep 19, 2013 at 05:00:35PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Linus recently complained about screen corruption when coming out of
> DPMS on his Haswell machine, and he also mentioned there were no error
> messages on the log. I think I can reproduce this problem, and when it
> happens I get a "FIFO underrun" message, but since it's just
> DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
> messages because reports containing "I'm getting a FIFO underrun on
> pipe A" are way much better than "I'm getting a screen corruption".

Sadly FIFO underruns on quite a few platforms are expected and
unfixable. Others they are just expected. We reduced the error level
because they are too noisy, and we were not in a position where the
underrun was actionable. As we do not yet have the underrun feedback in
place, I do not think we are ready for the onslaught of errors.

At minimum, you should only set the error message for the platforms you
intend to fix.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] drm/i915: don't disable ERR_INT on the IRQ handler

[Chris Wilson]

On Thu, Sep 19, 2013 at 05:00:36PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We currently disable the ERR_INT interrupts while running the IRQ
> handler because we fear that if we do an unclaimed register access
> from inside the IRQ handler we'll keep triggering the IRQ handler
> forever.
> 
> The problem is that since we always disable the ERR_INT interrupts at
> the IRQ handler, when we get a FIFO underrun we'll always print both
> messages:
>   - "uncleared fifo underrun on pipe A"
>   - "Pipe A FIFO underrun"
> 
> Because the "was_enabled" variable from
> ivybridge_set_fifo_underrun_reporting will always be false (since we
> disable ERR int at the IRQ handler!).
> 
> Instead of actually fixing ivybridge_set_fifo_underrun_reporting,
> let's just remove the "disable ERR_INT during the IRQ handler" code.
> As far as we know we shouldn't really be triggering ERR_INT interrupts
> from the IRQ handler, so if we ever get stuck in the endless loop of
> interrupts we can git-bisect and revert (and we can even bisect and
> revert this patch in case I'm just wrong). As a bonus, our IRQ handler
> is now simpler and a few nanoseconds faster.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

One could argue that by unmasking the err interrupt we prevent bugs
creeping into the interrupt handler.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/4] drm/i915: promote FIFO underruns to DRM_ERROR

[Paulo Zanoni]

2013/9/19 Chris Wilson <chris@chris-wilson.co.uk>:
> On Thu, Sep 19, 2013 at 05:00:35PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Linus recently complained about screen corruption when coming out of
>> DPMS on his Haswell machine, and he also mentioned there were no error
>> messages on the log. I think I can reproduce this problem, and when it
>> happens I get a "FIFO underrun" message, but since it's just
>> DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
>> messages because reports containing "I'm getting a FIFO underrun on
>> pipe A" are way much better than "I'm getting a screen corruption".
>
> Sadly FIFO underruns on quite a few platforms are expected and
> unfixable. Others they are just expected. We reduced the error level
> because they are too noisy, and we were not in a position where the
> underrun was actionable. As we do not yet have the underrun feedback in
> place, I do not think we are ready for the onslaught of errors.
>
> At minimum, you should only set the error message for the platforms you
> intend to fix.

Hmmmm, yeah, I should have thought about this a little bit more. Sorry :(

At least on ILK/SNB/IVB/HSW we have the code to stop flooding dmesg
and the code to ignore the expected underruns, so I guess a patch
touching just these platforms would be ok?

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH 1/4] drm/i915: promote FIFO underruns to DRM_ERROR

[Chris Wilson]

On Thu, Sep 19, 2013 at 05:20:49PM -0300, Paulo Zanoni wrote:
> 2013/9/19 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Thu, Sep 19, 2013 at 05:00:35PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Linus recently complained about screen corruption when coming out of
> >> DPMS on his Haswell machine, and he also mentioned there were no error
> >> messages on the log. I think I can reproduce this problem, and when it
> >> happens I get a "FIFO underrun" message, but since it's just
> >> DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
> >> messages because reports containing "I'm getting a FIFO underrun on
> >> pipe A" are way much better than "I'm getting a screen corruption".
> >
> > Sadly FIFO underruns on quite a few platforms are expected and
> > unfixable. Others they are just expected. We reduced the error level
> > because they are too noisy, and we were not in a position where the
> > underrun was actionable. As we do not yet have the underrun feedback in
> > place, I do not think we are ready for the onslaught of errors.
> >
> > At minimum, you should only set the error message for the platforms you
> > intend to fix.
> 
> Hmmmm, yeah, I should have thought about this a little bit more. Sorry :(
> 
> At least on ILK/SNB/IVB/HSW we have the code to stop flooding dmesg
> and the code to ignore the expected underruns, so I guess a patch
> touching just these platforms would be ok?

No worries, Mr Underrun.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: implement the Haswell mode set sequence workaround

[Ville Syrjälä]

On Thu, Sep 19, 2013 at 05:00:38PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This workaround is described in the mode set sequence documentation.
> When enabling planes for the second pipe, we need to wait for 2
> vblanks on the first pipe. This should solve "a flash of screen
> corruption if planes are enabled on second/third pipe during the time
> that big FIFO mode is exiting". Watermarks are fun :)
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8c3000d..fc55570 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3432,6 +3432,34 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
>  	intel_disable_plane(dev_priv, plane, pipe);
>  }
>  
> +/*
> + * This implements the workaround described in the "notes" section of the mode
> + * set sequence documentation. When going from no pipes or single pipe to
> + * multiple pipes, and planes are enabled after the pipe, we need to wait at
> + * least 2 vblanks on the first pipe before enabling planes on the second pipe.
> + */
> +static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct intel_crtc *crtc_it, *other_active_crtc = NULL;
> +
> +	/* We want to get the other_active_crtc only if there's only 1 other
> +	 * active crtc. */
> +	list_for_each_entry(crtc_it, &dev->mode_config.crtc_list, base.head) {
> +		if (crtc_it->active && crtc_it != crtc) {
> +			if (other_active_crtc)
> +				return;
> +			else
> +				other_active_crtc = crtc_it;
> +		}

Just a small bikeshed to avoid a few level is indentation here.

Eg.
	if (!crtc_it->active || crtc_it == crtc)
		continue;

	if (other_active_crtc)
		return;

	other_active_crtc = crtc_it;

But I won't insist on it, so even w/o the change:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +	}
> +	if (!other_active_crtc)
> +		return;
> +
> +	intel_wait_for_vblank(dev, other_active_crtc->pipe);
> +	intel_wait_for_vblank(dev, other_active_crtc->pipe);
> +}
> +
>  static void haswell_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -3483,6 +3511,9 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  		intel_opregion_notify_encoder(encoder, true);
>  	}
>  
> +	/* If we change the relative order between pipe/planes enabling, we need
> +	 * to change the workaround. */
> +	haswell_mode_set_planes_workaround(intel_crtc);
>  	haswell_crtc_enable_planes(crtc);
>  
>  	/*
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/4] drm/i915: promote FIFO underruns to DRM_ERROR

[Ville Syrjälä]

On Thu, Sep 19, 2013 at 05:20:49PM -0300, Paulo Zanoni wrote:
> 2013/9/19 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Thu, Sep 19, 2013 at 05:00:35PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Linus recently complained about screen corruption when coming out of
> >> DPMS on his Haswell machine, and he also mentioned there were no error
> >> messages on the log. I think I can reproduce this problem, and when it
> >> happens I get a "FIFO underrun" message, but since it's just
> >> DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
> >> messages because reports containing "I'm getting a FIFO underrun on
> >> pipe A" are way much better than "I'm getting a screen corruption".
> >
> > Sadly FIFO underruns on quite a few platforms are expected and
> > unfixable. Others they are just expected. We reduced the error level
> > because they are too noisy, and we were not in a position where the
> > underrun was actionable. As we do not yet have the underrun feedback in
> > place, I do not think we are ready for the onslaught of errors.
> >
> > At minimum, you should only set the error message for the platforms you
> > intend to fix.
> 
> Hmmmm, yeah, I should have thought about this a little bit more. Sorry :(
> 
> At least on ILK/SNB/IVB/HSW we have the code to stop flooding dmesg
> and the code to ignore the expected underruns, so I guess a patch
> touching just these platforms would be ok?

I would limit it to HSW before my ILK/SNB/IVB conversion to HSW
watermark code gets in. The current code apparently produces somewhat
incorrect results since according to Imre it even fixes some kind of
rare hang on ILK.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/4] drm/i915: don't disable ERR_INT on the IRQ handler

[Daniel Vetter]

On Thu, Sep 19, 2013 at 09:18:24PM +0100, Chris Wilson wrote:
> On Thu, Sep 19, 2013 at 05:00:36PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > We currently disable the ERR_INT interrupts while running the IRQ
> > handler because we fear that if we do an unclaimed register access
> > from inside the IRQ handler we'll keep triggering the IRQ handler
> > forever.
> > 
> > The problem is that since we always disable the ERR_INT interrupts at
> > the IRQ handler, when we get a FIFO underrun we'll always print both
> > messages:
> >   - "uncleared fifo underrun on pipe A"
> >   - "Pipe A FIFO underrun"
> > 
> > Because the "was_enabled" variable from
> > ivybridge_set_fifo_underrun_reporting will always be false (since we
> > disable ERR int at the IRQ handler!).
> > 
> > Instead of actually fixing ivybridge_set_fifo_underrun_reporting,
> > let's just remove the "disable ERR_INT during the IRQ handler" code.
> > As far as we know we shouldn't really be triggering ERR_INT interrupts
> > from the IRQ handler, so if we ever get stuck in the endless loop of
> > interrupts we can git-bisect and revert (and we can even bisect and
> > revert this patch in case I'm just wrong). As a bonus, our IRQ handler
> > is now simpler and a few nanoseconds faster.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> One could argue that by unmasking the err interrupt we prevent bugs
> creeping into the interrupt handler.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

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

Re: [PATCH 1/4] drm/i915: promote FIFO underruns to DRM_ERROR

[Paulo Zanoni]

2013/9/20 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Sep 19, 2013 at 05:20:49PM -0300, Paulo Zanoni wrote:
>> 2013/9/19 Chris Wilson <chris@chris-wilson.co.uk>:
>> > On Thu, Sep 19, 2013 at 05:00:35PM -0300, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >>
>> >> Linus recently complained about screen corruption when coming out of
>> >> DPMS on his Haswell machine, and he also mentioned there were no error
>> >> messages on the log. I think I can reproduce this problem, and when it
>> >> happens I get a "FIFO underrun" message, but since it's just
>> >> DRM_DEBUG_DRIVER it's hard to notice. So promote underruns to error
>> >> messages because reports containing "I'm getting a FIFO underrun on
>> >> pipe A" are way much better than "I'm getting a screen corruption".
>> >
>> > Sadly FIFO underruns on quite a few platforms are expected and
>> > unfixable. Others they are just expected. We reduced the error level
>> > because they are too noisy, and we were not in a position where the
>> > underrun was actionable. As we do not yet have the underrun feedback in
>> > place, I do not think we are ready for the onslaught of errors.
>> >
>> > At minimum, you should only set the error message for the platforms you
>> > intend to fix.
>>
>> Hmmmm, yeah, I should have thought about this a little bit more. Sorry :(
>>
>> At least on ILK/SNB/IVB/HSW we have the code to stop flooding dmesg
>> and the code to ignore the expected underruns, so I guess a patch
>> touching just these platforms would be ok?
>
> I would limit it to HSW before my ILK/SNB/IVB conversion to HSW
> watermark code gets in. The current code apparently produces somewhat
> incorrect results since according to Imre it even fixes some kind of
> rare hang on ILK.

Oh, adding "if IS_HASWELL" checks to that code will make it so much uglier :(

I guess I'll wait your ILK/SNB/IVB watermark fixes first :)

>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni

[PATCH 4/4] drm/i915: implement the Haswell mode set sequence workaround

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This workaround is described in the mode set sequence documentation.
When enabling planes for the second pipe, we need to wait for 2
vblanks on the first pipe. This should solve "a flash of screen
corruption if planes are enabled on second/third pipe during the time
that big FIFO mode is exiting". Watermarks are fun :)

v2: Save indentation levels

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d8325dd..61621ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3441,6 +3441,35 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
 	intel_disable_plane(dev_priv, plane, pipe);
 }
 
+/*
+ * This implements the workaround described in the "notes" section of the mode
+ * set sequence documentation. When going from no pipes or single pipe to
+ * multiple pipes, and planes are enabled after the pipe, we need to wait at
+ * least 2 vblanks on the first pipe before enabling planes on the second pipe.
+ */
+static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct intel_crtc *crtc_it, *other_active_crtc = NULL;
+
+	/* We want to get the other_active_crtc only if there's only 1 other
+	 * active crtc. */
+	list_for_each_entry(crtc_it, &dev->mode_config.crtc_list, base.head) {
+		if (!crtc_it->active || crtc_it == crtc)
+			continue;
+
+		if (other_active_crtc)
+			return;
+
+		other_active_crtc = crtc_it;
+	}
+	if (!other_active_crtc)
+		return;
+
+	intel_wait_for_vblank(dev, other_active_crtc->pipe);
+	intel_wait_for_vblank(dev, other_active_crtc->pipe);
+}
+
 static void haswell_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -3492,6 +3521,9 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 		intel_opregion_notify_encoder(encoder, true);
 	}
 
+	/* If we change the relative order between pipe/planes enabling, we need
+	 * to change the workaround. */
+	haswell_mode_set_planes_workaround(intel_crtc);
 	haswell_crtc_enable_planes(crtc);
 
 	/*
-- 
1.8.3.1

Re: [PATCH 4/4] drm/i915: implement the Haswell mode set sequence workaround

[Daniel Vetter]

On Fri, Sep 20, 2013 at 04:21:19PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This workaround is described in the mode set sequence documentation.
> When enabling planes for the second pipe, we need to wait for 2
> vblanks on the first pipe. This should solve "a flash of screen
> corruption if planes are enabled on second/third pipe during the time
> that big FIFO mode is exiting". Watermarks are fun :)
> 
> v2: Save indentation levels
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Last two patches of this series merged to dinq.

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