[PATCH 0/4] FIFO underrun reporting v2

[PATCH 0/4] FIFO underrun reporting v2

[Paulo Zanoni]

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

Hi

Here's a new series that tries to report FIFO underruns. The new implementation
is completely different from the old one, due to the reviews I received. Now we
don't just "ignore" the FIFO underrun interrupts when we receive them, we
effectively disable the interrupts (the downsides of this approach are
documented in the commit message of patch 2).

We will still see at most one of each error FIFO underrun message per mode set,
so this is not really going to flood dmesg. I tested this series on ILK, SNB and
HSW, and I am only seeing FIFO underruns on ILK when using 2 monitors
(LVDS+HDMI). I'll work on a patch to fix this message later (at least now we
know we have problems!).

It seems not everybody is in favor of adding the "poison" error message, so the
"print poison interrupts" patch comes _after_ the "print FIFO underruns". We can
discard the patch if we conclude we don't want it. I haven't seen the poison
message on any of my tests.

Thanks,
Paulo

Paulo Zanoni (4):
  drm/i915: also disable south interrupts when handling them
  drm/i915: report Gen5+ CPU and PCH FIFO underruns
  drm/i915: print Gen5+ CPU/PCH poison interrupts
  drm/i915: remove "inline" keyword from ironlake_disable_display_irq

 drivers/gpu/drm/i915/i915_irq.c      |  345 ++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h      |   15 +-
 drivers/gpu/drm/i915/intel_display.c |   17 +-
 drivers/gpu/drm/i915/intel_drv.h     |   10 +
 4 files changed, 371 insertions(+), 16 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] drm/i915: also disable south interrupts when handling them

[Paulo Zanoni]

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

>From the docs:

  "IIR can queue up to two interrupt events. When the IIR is cleared,
  it will set itself again after one clock if a second event was
  stored."

  "Only the rising edge of the PCH Display interrupt will cause the
  North Display IIR (DEIIR) PCH Display Interrupt even bit to be set,
  so all PCH Display Interrupts, including back to back interrupts,
  must be cleared before a new PCH Display interrupt can cause DEIIR
  to be set".

The current code works fine because we don't get many interrupts, but
if we enable the PCH FIFO underrun interrupts we'll start getting so
many interrupts that at some point new PCH interrupts won't cause
DEIIR to be set.

The initial implementation I tried was to turn the code that checks
SDEIIR into a loop, but we can still get interrupts even after the
loop is done (and before the irq handler finishes), so we have to
either disable the interrupts or mask them. In the end I concluded
that just disabling the PCH interrupts is enough, you don't even need
the loop, so this is what this patch implements. I've tested it and it
passes the 2 "PCH FIFO underrun interrupt storms" I can reproduce:
the "ironlake_crtc_disable" case and the "wrong watermarks" case.

In other words, here's how to reproduce the problem fixed by this
patch:
  1 - Enable PCH FIFO underrun interrupts (SERR_INT on SNB+)
  2 - Boot the machine
  3 - While booting we'll get tons of PCH FIFO underrun interrupts
  4 - Plug a new monitor
  5 - Run xrandr, notice it won't detect the new monitor
  6 - Read SDEIIR and notice it's not 0 while DEIIR is 0

Q: Can't we just clear DEIIR before SDEIIR?
A: It doesn't work. SDEIIR has to be completely cleared (including the
interrupts stored on its back queue) before it can flip DEIIR's bit to
1 again, and even while you're clearing it you'll be getting more and
more interrupts.

Q: Why does it work by just disabling+enabling the south interrupts?
A: Because when we re-enable them, if there's something on the SDEIIR
register (maybe an interrupt stored on the queue), the re-enabling
will make DEIIR's bit flip to 1, and since we'll already have
interrupts enabled we'll get another interrupt, then run our irq
handler again to process the "back" interrupts.

v2: Even bigger commit message, added code comments.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 29037e0..7531095 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -701,7 +701,7 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-	u32 de_iir, gt_iir, de_ier, pm_iir;
+	u32 de_iir, gt_iir, de_ier, pm_iir, sde_ier;
 	irqreturn_t ret = IRQ_NONE;
 	int i;
 
@@ -711,6 +711,15 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 	de_ier = I915_READ(DEIER);
 	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
 
+	/* Disable south interrupts. We'll only write to SDEIIR once, so further
+	 * interrupts will will be stored on its back queue, and then we'll be
+	 * able to process them after we restore SDEIER (as soon as we restore
+	 * it, we'll get an interrupt if SDEIIR still has something to process
+	 * due to its back queue). */
+	sde_ier = I915_READ(SDEIER);
+	I915_WRITE(SDEIER, 0);
+	POSTING_READ(SDEIER);
+
 	gt_iir = I915_READ(GTIIR);
 	if (gt_iir) {
 		snb_gt_irq_handler(dev, dev_priv, gt_iir);
@@ -759,6 +768,8 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 
 	I915_WRITE(DEIER, de_ier);
 	POSTING_READ(DEIER);
+	I915_WRITE(SDEIER, sde_ier);
+	POSTING_READ(SDEIER);
 
 	return ret;
 }
@@ -778,7 +789,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	struct drm_device *dev = (struct drm_device *) arg;
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	int ret = IRQ_NONE;
-	u32 de_iir, gt_iir, de_ier, pm_iir;
+	u32 de_iir, gt_iir, de_ier, pm_iir, sde_ier;
 
 	atomic_inc(&dev_priv->irq_received);
 
@@ -787,6 +798,15 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
 	POSTING_READ(DEIER);
 
+	/* Disable south interrupts. We'll only write to SDEIIR once, so further
+	 * interrupts will will be stored on its back queue, and then we'll be
+	 * able to process them after we restore SDEIER (as soon as we restore
+	 * it, we'll get an interrupt if SDEIIR still has something to process
+	 * due to its back queue). */
+	sde_ier = I915_READ(SDEIER);
+	I915_WRITE(SDEIER, 0);
+	POSTING_READ(SDEIER);
+
 	de_iir = I915_READ(DEIIR);
 	gt_iir = I915_READ(GTIIR);
 	pm_iir = I915_READ(GEN6_PMIIR);
@@ -849,6 +869,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 done:
 	I915_WRITE(DEIER, de_ier);
 	POSTING_READ(DEIER);
+	I915_WRITE(SDEIER, sde_ier);
+	POSTING_READ(SDEIER);
 
 	return ret;
 }
-- 
1.7.10.4

[PATCH 2/4] drm/i915: report Gen5+ CPU and PCH FIFO underruns

[Paulo Zanoni]

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

In this commit we enable both CPU and PCH FIFO underrun reporting and
start reporting them. We follow a few rules:
  - after we receive one of these errors, we mask the interrupt, so
    we won't get an "interrupt storm" and we also won't flood dmesg;
  - at each mode set we enable the interrupts again, so we'll see each
    message at most once per mode set;
  - in the specific places where we need to ignore the errors, we
    completely mask the interrupts.

The downside of this patch is that since we're completely disabling
(masking) the interrupts instead of just not printing error messages,
we will mask more than just what we want on IVB/HSW CPU interrupts
(due to GEN7_ERR_INT) and on CPT/PPT/LPT PCHs (due to SERR_INT). So
when we decide to mask PCH FIFO underruns for pipe A on CPT, we'll
also be masking PCH FIFO underruns for pipe B, because both are
reported by SERR_INT which has to be either completely enabled or
completely disabled (in othe words, there's no way to disable/enable
specific bits of GEN7_ERR_INT and SERR_INT).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  307 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_reg.h      |   13 +-
 drivers/gpu/drm/i915/intel_display.c |   17 +-
 drivers/gpu/drm/i915/intel_drv.h     |   10 ++
 4 files changed, 334 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7531095..d0f9c47 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -57,6 +57,208 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 	}
 }
 
+static bool disable_gen7_err_int(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc;
+	enum pipe pipe;
+
+	for_each_pipe(pipe) {
+		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+
+		if (crtc->disable_cpu_fifo_underrun)
+			return true;
+	}
+
+	return false;
+}
+
+static bool disable_serr_int(struct drm_i915_private *dev_priv)
+{
+	enum pipe pipe;
+	struct intel_crtc *crtc;
+
+	for_each_pipe(pipe) {
+		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+
+		if (crtc->disable_pch_fifo_underrun)
+			return true;
+	}
+
+	return false;
+}
+
+static void ironlake_report_fifo_underrun(struct drm_device *dev,
+					  enum pipe pipe, bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t bit = (pipe == PIPE_A) ? DE_PIPEA_FIFO_UNDERRUN :
+					  DE_PIPEB_FIFO_UNDERRUN;
+
+	if (enable)
+		ironlake_enable_display_irq(dev_priv, bit);
+	else
+		ironlake_disable_display_irq(dev_priv, bit);
+}
+
+static void ivybridge_report_fifo_underrun(struct drm_device *dev, bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (enable) {
+		if (disable_gen7_err_int(dev_priv))
+			return;
+
+		I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN_A |
+					 ERR_INT_FIFO_UNDERRUN_B |
+					 ERR_INT_FIFO_UNDERRUN_C);
+
+		ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
+	} else {
+		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
+	}
+}
+
+static void ibx_report_fifo_underrun(struct intel_crtc *crtc, bool enable)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	uint32_t bit = (crtc->pipe == PIPE_A) ? SDE_TRANSA_FIFO_UNDER :
+						SDE_TRANSB_FIFO_UNDER;
+
+	if (enable)
+		I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~bit);
+	else
+		I915_WRITE(SDEIMR, I915_READ(SDEIMR) | bit);
+
+	POSTING_READ(SDEIMR);
+}
+
+static void cpt_report_fifo_underrun(struct drm_device *dev,
+				     enum transcoder pch_transcoder,
+				     bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (enable) {
+		if (disable_serr_int(dev_priv))
+			return;
+
+		I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
+				     SERR_INT_TRANS_B_FIFO_UNDERRUN |
+				     SERR_INT_TRANS_C_FIFO_UNDERRUN);
+
+		I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~SDE_ERROR_CPT);
+	} else {
+		I915_WRITE(SDEIMR, I915_READ(SDEIMR) | SDE_ERROR_CPT);
+	}
+
+	POSTING_READ(SDEIMR);
+}
+
+/**
+ * intel_report_cpu_fifo_underrun - enable/disable FIFO underrun messages
+ * @dev: drm device
+ * @pipe: pipe
+ * @enable: true if we want to report FIFO underrun errors, false otherwise
+ *
+ * This function makes us disable or report CPU fifo underruns for a specific
+ * pipe. Notice that on some Gens (e.g. IVB, HSW), disabling FIFO underrun
+ * reporting for one pipe may also disable all the other CPU error interruts for
+ * the other pipes, due to the fact that there's just one interrupt mask/enable
+ * bit for all the pipes.
+ *
+ * Returns the previous state of underrun reporting.
+ */
+bool intel_report_cpu_fifo_underrun(struct drm_device *dev, enum pipe pipe,
+				    bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	unsigned long flags;
+	bool ret;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
+
+	ret = !intel_crtc->disable_cpu_fifo_underrun;
+
+	if (enable == ret)
+		goto done;
+
+	intel_crtc->disable_cpu_fifo_underrun = !enable;
+
+	if (IS_GEN5(dev) || IS_GEN6(dev))
+		ironlake_report_fifo_underrun(dev, pipe, enable);
+	else if (IS_GEN7(dev))
+		ivybridge_report_fifo_underrun(dev, enable);
+
+done:
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
+	return ret;
+}
+
+/**
+ * intel_report_pch_fifo_underrun - enable/disable FIFO underrun messages
+ * @dev: drm device
+ * @pch_transcoder: the PCH transcoder (same as pipe on IVB and older)
+ * @enable: true if we want to report FIFO underrun errors, false otherwise
+ *
+ * This function makes us disable or report PCH fifo underruns for a specific
+ * PCH transcoder. Notice that on some PCHs (e.g. CPT/PPT), disabling FIFO
+ * underrun reporting for one transcoder may also disable all the other PCH
+ * error interruts for the other transcoders, due to the fact that there's just
+ * one interrupt mask/enable bit for all the transcoders.
+ *
+ * Returns the previous state of underrun reporting.
+ */
+bool intel_report_pch_fifo_underrun(struct drm_device *dev,
+				    enum transcoder pch_transcoder,
+				    bool enable)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum pipe p;
+	struct drm_crtc *crtc;
+	struct intel_crtc *intel_crtc;
+	unsigned long flags;
+	bool ret;
+
+	if (HAS_PCH_LPT(dev)) {
+		crtc = NULL;
+		for_each_pipe(p) {
+			struct drm_crtc *c = dev_priv->pipe_to_crtc_mapping[p];
+			if (intel_pipe_has_type(c, INTEL_OUTPUT_ANALOG)) {
+				crtc = c;
+				break;
+			}
+		}
+		if (!crtc) {
+			DRM_ERROR("PCH FIFO underrun, but no CRTC using the PCH found\n");
+			return false;
+		}
+	} else {
+		crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder];
+	}
+	intel_crtc = to_intel_crtc(crtc);
+
+	spin_lock_irqsave(&dev_priv->irq_lock, flags);
+
+	ret = !intel_crtc->disable_pch_fifo_underrun;
+
+	if (enable == ret)
+		goto done;
+
+	intel_crtc->disable_pch_fifo_underrun = !enable;
+
+	if (HAS_PCH_IBX(dev))
+		ibx_report_fifo_underrun(intel_crtc, enable);
+	else
+		cpt_report_fifo_underrun(dev, pch_transcoder, enable);
+
+done:
+	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
+	return ret;
+}
+
 void
 i915_enable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask)
 {
@@ -659,14 +861,57 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 	if (pch_iir & (SDE_TRANSB_CRC_ERR | SDE_TRANSA_CRC_ERR))
 		DRM_DEBUG_DRIVER("PCH transcoder CRC error interrupt\n");
 
-	if (pch_iir & SDE_TRANSB_FIFO_UNDER)
-		DRM_DEBUG_DRIVER("PCH transcoder B underrun interrupt\n");
 	if (pch_iir & SDE_TRANSA_FIFO_UNDER)
-		DRM_DEBUG_DRIVER("PCH transcoder A underrun interrupt\n");
+		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false))
+			DRM_ERROR("PCH transcoder A FIFO underrun\n");
+
+	if (pch_iir & SDE_TRANSB_FIFO_UNDER)
+		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_B, false))
+			DRM_ERROR("PCH transcoder B FIFO underrun\n");
 }
 
+static void err_int_handler(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 err_int = I915_READ(GEN7_ERR_INT);
+
+	if (err_int & ERR_INT_FIFO_UNDERRUN_A)
+		if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
+			DRM_ERROR("Pipe A FIFO underrun\n");
+
+	if (err_int & ERR_INT_FIFO_UNDERRUN_B)
+		if (intel_report_cpu_fifo_underrun(dev, PIPE_B, false))
+			DRM_ERROR("Pipe B FIFO underrun\n");
+
+	if (err_int & ERR_INT_FIFO_UNDERRUN_C)
+		if (intel_report_cpu_fifo_underrun(dev, PIPE_C, false))
+			DRM_ERROR("Pipe C FIFO underrun\n");
+
+	I915_WRITE(GEN7_ERR_INT, err_int);
+}
+
+static void serr_int_handler(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 serr_int = I915_READ(SERR_INT);
+
+	if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
+		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false))
+			DRM_ERROR("PCH transcoder A FIFO underrun\n");
+
+	if (serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN)
+		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_B, false))
+			DRM_ERROR("PCH transcoder B FIFO underrun\n");
+
+	if (serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN)
+		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_C, false))
+			DRM_ERROR("PCH transcoder C FIFO underrun\n");
+
+	I915_WRITE(SERR_INT, serr_int);
+}
 static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 {
+
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	int pipe;
 
@@ -695,6 +940,9 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 			DRM_DEBUG_DRIVER("  pipe %c FDI IIR: 0x%08x\n",
 					 pipe_name(pipe),
 					 I915_READ(FDI_RX_IIR(pipe)));
+
+	if (pch_iir & SDE_ERROR_CPT)
+		serr_int_handler(dev);
 }
 
 static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
@@ -707,6 +955,14 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 
 	atomic_inc(&dev_priv->irq_received);
 
+	/* We get interrupts on unclaimed registers, so check for this before we
+	 * do any I915_{READ,WRITE}. */
+	if (IS_HASWELL(dev) &&
+	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
+		DRM_ERROR("Unclaimed register before interrupt\n");
+		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
+	}
+
 	/* disable master interrupt before clearing iir  */
 	de_ier = I915_READ(DEIER);
 	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
@@ -720,6 +976,12 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 	I915_WRITE(SDEIER, 0);
 	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))
+		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
+
 	gt_iir = I915_READ(GTIIR);
 	if (gt_iir) {
 		snb_gt_irq_handler(dev, dev_priv, gt_iir);
@@ -729,6 +991,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 
 	de_iir = I915_READ(DEIIR);
 	if (de_iir) {
+		if (de_iir & DE_ERR_INT_IVB)
+			err_int_handler(dev);
+
 		if (de_iir & DE_AUX_CHANNEL_A_IVB)
 			dp_aux_irq_handler(dev);
 
@@ -766,6 +1031,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
 		ret = IRQ_HANDLED;
 	}
 
+	if (IS_HASWELL(dev) && !disable_gen7_err_int(dev_priv))
+		ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
+
 	I915_WRITE(DEIER, de_ier);
 	POSTING_READ(DEIER);
 	I915_WRITE(SDEIER, sde_ier);
@@ -833,6 +1101,14 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	if (de_iir & DE_PIPEB_VBLANK)
 		drm_handle_vblank(dev, 1);
 
+	if (de_iir & DE_PIPEA_FIFO_UNDERRUN)
+		if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
+			DRM_ERROR("Pipe A FIFO underrun\n");
+
+	if (de_iir & DE_PIPEB_FIFO_UNDERRUN)
+		if (intel_report_cpu_fifo_underrun(dev, PIPE_B, false))
+			DRM_ERROR("Pipe B FIFO underrun\n");
+
 	if (de_iir & DE_PLANEA_FLIP_DONE) {
 		intel_prepare_page_flip(dev, 0);
 		intel_finish_page_flip_plane(dev, 0);
@@ -1966,14 +2242,20 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	u32 mask;
 
-	if (HAS_PCH_IBX(dev))
+	if (HAS_PCH_IBX(dev)) {
 		mask = SDE_HOTPLUG_MASK |
 		       SDE_GMBUS |
-		       SDE_AUX_MASK;
-	else
+		       SDE_AUX_MASK |
+		       SDE_TRANSB_FIFO_UNDER |
+		       SDE_TRANSA_FIFO_UNDER;
+	} else {
 		mask = SDE_HOTPLUG_MASK_CPT |
 		       SDE_GMBUS_CPT |
-		       SDE_AUX_MASK_CPT;
+		       SDE_AUX_MASK_CPT |
+		       SDE_ERROR_CPT;
+
+		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
+	}
 
 	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
 	I915_WRITE(SDEIMR, ~mask);
@@ -1989,7 +2271,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 	/* enable kind of interrupts always enabled */
 	u32 display_mask = DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
 			   DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
-			   DE_AUX_CHANNEL_A;
+			   DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
+			   DE_PIPEA_FIFO_UNDERRUN;
 	u32 render_irqs;
 
 	dev_priv->irq_mask = ~display_mask;
@@ -2039,12 +2322,14 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 		DE_PLANEC_FLIP_DONE_IVB |
 		DE_PLANEB_FLIP_DONE_IVB |
 		DE_PLANEA_FLIP_DONE_IVB |
-		DE_AUX_CHANNEL_A_IVB;
+		DE_AUX_CHANNEL_A_IVB |
+		DE_ERR_INT_IVB;
 	u32 render_irqs;
 
 	dev_priv->irq_mask = ~display_mask;
 
 	/* should always can generate irq */
+	I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 	I915_WRITE(DEIIR, I915_READ(DEIIR));
 	I915_WRITE(DEIMR, dev_priv->irq_mask);
 	I915_WRITE(DEIER,
@@ -2195,6 +2480,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(DEIMR, 0xffffffff);
 	I915_WRITE(DEIER, 0x0);
 	I915_WRITE(DEIIR, I915_READ(DEIIR));
+	if (IS_GEN7(dev))
+		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
 	I915_WRITE(GTIMR, 0xffffffff);
 	I915_WRITE(GTIER, 0x0);
@@ -2203,6 +2490,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	I915_WRITE(SDEIMR, 0xffffffff);
 	I915_WRITE(SDEIER, 0x0);
 	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
+	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
+		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
 }
 
 static void i8xx_irq_preinstall(struct drm_device * dev)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cd226c2..4d27320 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -520,7 +520,10 @@
 
 #define ERROR_GEN6	0x040a0
 #define GEN7_ERR_INT	0x44040
-#define   ERR_INT_MMIO_UNCLAIMED (1<<13)
+#define   ERR_INT_MMIO_UNCLAIMED	(1<<13)
+#define   ERR_INT_FIFO_UNDERRUN_C	(1<<6)
+#define   ERR_INT_FIFO_UNDERRUN_B	(1<<3)
+#define   ERR_INT_FIFO_UNDERRUN_A	(1<<0)
 
 #define FPGA_DBG		0x42300
 #define   FPGA_DBG_RM_NOCLAIM	(1<<31)
@@ -3390,7 +3393,7 @@
 #define DE_PIPEA_FIFO_UNDERRUN  (1 << 0)
 
 /* More Ivybridge lolz */
-#define DE_ERR_DEBUG_IVB		(1<<30)
+#define DE_ERR_INT_IVB			(1<<30)
 #define DE_GSE_IVB			(1<<29)
 #define DE_PCH_EVENT_IVB		(1<<28)
 #define DE_DP_A_HOTPLUG_IVB		(1<<27)
@@ -3540,6 +3543,7 @@
 				 SDE_PORTC_HOTPLUG_CPT |	\
 				 SDE_PORTB_HOTPLUG_CPT)
 #define SDE_GMBUS_CPT		(1 << 17)
+#define SDE_ERROR_CPT		(1 << 16)
 #define SDE_AUDIO_CP_REQ_C_CPT	(1 << 10)
 #define SDE_AUDIO_CP_CHG_C_CPT	(1 << 9)
 #define SDE_FDI_RXC_CPT		(1 << 8)
@@ -3564,6 +3568,11 @@
 #define SDEIIR  0xc4008
 #define SDEIER  0xc400c
 
+#define SERR_INT			0xc4040
+#define  SERR_INT_TRANS_C_FIFO_UNDERRUN	(1<<6)
+#define  SERR_INT_TRANS_B_FIFO_UNDERRUN	(1<<3)
+#define  SERR_INT_TRANS_A_FIFO_UNDERRUN	(1<<0)
+
 /* digital port hotplug */
 #define PCH_PORT_HOTPLUG        0xc4030		/* SHOTPLUG_CTL */
 #define PORTD_HOTPLUG_ENABLE            (1 << 20)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9b0cd86..7152f35 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -41,7 +41,6 @@
 #include <drm/drm_crtc_helper.h>
 #include <linux/dma_remapping.h>
 
-bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
 
@@ -3305,6 +3304,10 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->active = true;
+
+	intel_report_cpu_fifo_underrun(dev, pipe, true);
+	intel_report_pch_fifo_underrun(dev, pipe, true);
+
 	intel_update_watermarks(dev);
 
 	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
@@ -3397,10 +3400,15 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->active = true;
-	intel_update_watermarks(dev);
 
 	is_pch_port = haswell_crtc_driving_pch(crtc);
 
+	intel_report_cpu_fifo_underrun(dev, pipe, true);
+	if (is_pch_port)
+		intel_report_pch_fifo_underrun(dev, TRANSCODER_A, true);
+
+	intel_update_watermarks(dev);
+
 	if (is_pch_port)
 		dev_priv->display.fdi_link_train(crtc);
 
@@ -3484,6 +3492,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	if (dev_priv->cfb_plane == plane)
 		intel_disable_fbc(dev);
 
+	intel_report_pch_fifo_underrun(dev, pipe, false);
 	intel_disable_pipe(dev_priv, pipe);
 
 	/* Disable PF */
@@ -3497,6 +3506,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	ironlake_fdi_disable(crtc);
 
 	ironlake_disable_pch_transcoder(dev_priv, pipe);
+	intel_report_pch_fifo_underrun(dev, pipe, true);
 
 	if (HAS_PCH_CPT(dev)) {
 		/* disable TRANS_DP_CTL */
@@ -3566,6 +3576,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	if (dev_priv->cfb_plane == plane)
 		intel_disable_fbc(dev);
 
+	if (is_pch_port)
+		intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false);
 	intel_disable_pipe(dev_priv, pipe);
 
 	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
@@ -3582,6 +3594,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 
 	if (is_pch_port) {
 		lpt_disable_pch_transcoder(dev_priv);
+		intel_report_pch_fifo_underrun(dev, TRANSCODER_A, true);
 		intel_ddi_fdi_disable(crtc);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 010e998..aa8f948 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -238,6 +238,9 @@ struct intel_crtc {
 
 	/* reset counter value when the last flip was submitted */
 	unsigned int reset_counter;
+
+	bool disable_cpu_fifo_underrun;
+	bool disable_pch_fifo_underrun;
 };
 
 struct intel_plane {
@@ -442,6 +445,7 @@ int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
 extern void intel_attach_force_audio_property(struct drm_connector *connector);
 extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
 
+extern bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
 extern void intel_crt_init(struct drm_device *dev);
 extern void intel_hdmi_init(struct drm_device *dev,
 			    int hdmi_reg, enum port port);
@@ -697,5 +701,11 @@ intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
 extern void intel_ddi_fdi_disable(struct drm_crtc *crtc);
 
 extern void intel_display_handle_reset(struct drm_device *dev);
+extern bool intel_report_cpu_fifo_underrun(struct drm_device *dev,
+					   enum pipe pipe,
+					   bool enable);
+extern bool intel_report_pch_fifo_underrun(struct drm_device *dev,
+					   enum transcoder pch_transcoder,
+					   bool enable);
 
 #endif /* __INTEL_DRV_H__ */
-- 
1.7.10.4

[PATCH 3/4] drm/i915: print Gen5+ CPU/PCH poison interrupts

[Paulo Zanoni]

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

This is bad news and shouldn't be happening.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c |   14 ++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h |    2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d0f9c47..98a57ed 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -875,6 +875,9 @@ static void err_int_handler(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 err_int = I915_READ(GEN7_ERR_INT);
 
+	if (err_int & ERR_INT_POISON)
+		DRM_ERROR("Poison interrupt\n");
+
 	if (err_int & ERR_INT_FIFO_UNDERRUN_A)
 		if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
 			DRM_ERROR("Pipe A FIFO underrun\n");
@@ -895,6 +898,9 @@ static void serr_int_handler(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 serr_int = I915_READ(SERR_INT);
 
+	if (serr_int & SERR_INT_POISON)
+		DRM_ERROR("PCH poison interrupt\n");
+
 	if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
 		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false))
 			DRM_ERROR("PCH transcoder A FIFO underrun\n");
@@ -1101,6 +1107,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
 	if (de_iir & DE_PIPEB_VBLANK)
 		drm_handle_vblank(dev, 1);
 
+	if (de_iir & DE_POISON)
+		DRM_ERROR("Poison interrupt\n");
+
 	if (de_iir & DE_PIPEA_FIFO_UNDERRUN)
 		if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
 			DRM_ERROR("Pipe A FIFO underrun\n");
@@ -2247,7 +2256,8 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 		       SDE_GMBUS |
 		       SDE_AUX_MASK |
 		       SDE_TRANSB_FIFO_UNDER |
-		       SDE_TRANSA_FIFO_UNDER;
+		       SDE_TRANSA_FIFO_UNDER |
+		       SDE_POISON;
 	} else {
 		mask = SDE_HOTPLUG_MASK_CPT |
 		       SDE_GMBUS_CPT |
@@ -2272,7 +2282,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 	u32 display_mask = DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
 			   DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
 			   DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
-			   DE_PIPEA_FIFO_UNDERRUN;
+			   DE_PIPEA_FIFO_UNDERRUN | DE_POISON;
 	u32 render_irqs;
 
 	dev_priv->irq_mask = ~display_mask;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4d27320..62cfcea 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -520,6 +520,7 @@
 
 #define ERROR_GEN6	0x040a0
 #define GEN7_ERR_INT	0x44040
+#define   ERR_INT_POISON		(1<<31)
 #define   ERR_INT_MMIO_UNCLAIMED	(1<<13)
 #define   ERR_INT_FIFO_UNDERRUN_C	(1<<6)
 #define   ERR_INT_FIFO_UNDERRUN_B	(1<<3)
@@ -3569,6 +3570,7 @@
 #define SDEIER  0xc400c
 
 #define SERR_INT			0xc4040
+#define  SERR_INT_POISON		(1<<31)
 #define  SERR_INT_TRANS_C_FIFO_UNDERRUN	(1<<6)
 #define  SERR_INT_TRANS_B_FIFO_UNDERRUN	(1<<3)
 #define  SERR_INT_TRANS_A_FIFO_UNDERRUN	(1<<0)
-- 
1.7.10.4

[PATCH 4/4] drm/i915: remove "inline" keyword from ironlake_disable_display_irq

[Paulo Zanoni]

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

 - It's a static function
 - I just added a few more users to it
 - Its sister ironlake_enable_display_irq is not marked as inline
 - The compiler will still inline if it thinks it should do

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 98a57ed..f5213d8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -47,7 +47,7 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 	}
 }
 
-static inline void
+static void
 ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
 {
 	if ((dev_priv->irq_mask & mask) != mask) {
-- 
1.7.10.4

Re: [PATCH 0/4] FIFO underrun reporting v2

[Ville Syrjälä]

On Fri, Feb 22, 2013 at 05:05:27PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Hi
> 
> Here's a new series that tries to report FIFO underruns. The new implementation
> is completely different from the old one, due to the reviews I received. Now we
> don't just "ignore" the FIFO underrun interrupts when we receive them, we
> effectively disable the interrupts (the downsides of this approach are
> documented in the commit message of patch 2).
> 
> We will still see at most one of each error FIFO underrun message per mode set,
> so this is not really going to flood dmesg. I tested this series on ILK, SNB and
> HSW, and I am only seeing FIFO underruns on ILK when using 2 monitors
> (LVDS+HDMI). I'll work on a patch to fix this message later (at least now we
> know we have problems!).

If you want another way to get FIFO underruns, take an IVB machine, grab
my plane clipping patches and my glplane app, run the app w/ something
like 1080p resolution and hit 't'. When the sprite is at a magic
position on the screen and the downscaling ratio is at the maximum you
should see underruns.

I'm not sure if we're misprogramming the watermarks or something. I know
we're not checking the display core clock against the pixel rate
limitations, but I did a quick manual calculation and it gave me a
limit of ~152 MHz for 2x downscaled 32bpp sprite. The pixel clock of the
mode I'm using is 148.5 MHz, so in theory it should be safe.

At first I was only seeing pipe underruns, but now I'm also getting PCH
underruns. Not sure why the PCH underruns didn't get triggered
initially.

BTW I noticed that with your patches the ERR_INT triggers twice on underrun,
but the first time disables the underrun reporting so nothing gets printed
for the second occurance. I didn't read your patches with enough detail
yet to know if that's expected or not. I suppose one spurious triggering
of ERR_INT is not a big deal.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/4] drm/i915: also disable south interrupts when handling them

[Daniel Vetter]

On Fri, Feb 22, 2013 at 05:05:28PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> From the docs:
> 
>   "IIR can queue up to two interrupt events. When the IIR is cleared,
>   it will set itself again after one clock if a second event was
>   stored."
> 
>   "Only the rising edge of the PCH Display interrupt will cause the
>   North Display IIR (DEIIR) PCH Display Interrupt even bit to be set,
>   so all PCH Display Interrupts, including back to back interrupts,
>   must be cleared before a new PCH Display interrupt can cause DEIIR
>   to be set".
> 
> The current code works fine because we don't get many interrupts, but
> if we enable the PCH FIFO underrun interrupts we'll start getting so
> many interrupts that at some point new PCH interrupts won't cause
> DEIIR to be set.
> 
> The initial implementation I tried was to turn the code that checks
> SDEIIR into a loop, but we can still get interrupts even after the
> loop is done (and before the irq handler finishes), so we have to
> either disable the interrupts or mask them. In the end I concluded
> that just disabling the PCH interrupts is enough, you don't even need
> the loop, so this is what this patch implements. I've tested it and it
> passes the 2 "PCH FIFO underrun interrupt storms" I can reproduce:
> the "ironlake_crtc_disable" case and the "wrong watermarks" case.
> 
> In other words, here's how to reproduce the problem fixed by this
> patch:
>   1 - Enable PCH FIFO underrun interrupts (SERR_INT on SNB+)
>   2 - Boot the machine
>   3 - While booting we'll get tons of PCH FIFO underrun interrupts
>   4 - Plug a new monitor
>   5 - Run xrandr, notice it won't detect the new monitor
>   6 - Read SDEIIR and notice it's not 0 while DEIIR is 0
> 
> Q: Can't we just clear DEIIR before SDEIIR?
> A: It doesn't work. SDEIIR has to be completely cleared (including the
> interrupts stored on its back queue) before it can flip DEIIR's bit to
> 1 again, and even while you're clearing it you'll be getting more and
> more interrupts.
> 
> Q: Why does it work by just disabling+enabling the south interrupts?
> A: Because when we re-enable them, if there's something on the SDEIIR
> register (maybe an interrupt stored on the queue), the re-enabling
> will make DEIIR's bit flip to 1, and since we'll already have
> interrupts enabled we'll get another interrupt, then run our irq
> handler again to process the "back" interrupts.
> 
> v2: Even bigger commit message, added code comments.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Since this seems to fix the dp aux irq timeout regression I've merged this
to -fixes. Also volunteered Imre for a review, I'll add that if it pops up
in the next few days.

Big thanks to Paulo&Imre for tracking this down.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: remove "inline" keyword from ironlake_disable_display_irq

[Daniel Vetter]

On Fri, Feb 22, 2013 at 05:05:31PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
>  - It's a static function
>  - I just added a few more users to it
>  - Its sister ironlake_enable_display_irq is not marked as inline
>  - The compiler will still inline if it thinks it should do
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Queued for -next, thanks for the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_irq.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 98a57ed..f5213d8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -47,7 +47,7 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>  	}
>  }
>  
> -static inline void
> +static void
>  ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>  {
>  	if ((dev_priv->irq_mask & mask) != mask) {
> -- 
> 1.7.10.4
> 
> _______________________________________________
> 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

Re: [PATCH 2/4] drm/i915: report Gen5+ CPU and PCH FIFO underruns

[Daniel Vetter]

On Fri, Feb 22, 2013 at 05:05:29PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> In this commit we enable both CPU and PCH FIFO underrun reporting and
> start reporting them. We follow a few rules:
>   - after we receive one of these errors, we mask the interrupt, so
>     we won't get an "interrupt storm" and we also won't flood dmesg;
>   - at each mode set we enable the interrupts again, so we'll see each
>     message at most once per mode set;
>   - in the specific places where we need to ignore the errors, we
>     completely mask the interrupts.
> 
> The downside of this patch is that since we're completely disabling
> (masking) the interrupts instead of just not printing error messages,
> we will mask more than just what we want on IVB/HSW CPU interrupts
> (due to GEN7_ERR_INT) and on CPT/PPT/LPT PCHs (due to SERR_INT). So
> when we decide to mask PCH FIFO underruns for pipe A on CPT, we'll
> also be masking PCH FIFO underruns for pipe B, because both are
> reported by SERR_INT which has to be either completely enabled or
> completely disabled (in othe words, there's no way to disable/enable
> specific bits of GEN7_ERR_INT and SERR_INT).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Ok, sorry for the long delay in reviewing this (I've somehow hoped that
someone else would do it, but meh ...).

I'm happy with the logic in your patch here, but it took me a while to
piece together the control flow and what exactly is done where. I think
that can be improved by renaming a few functions to make it clearer what
exactly they're doing. Suggestions below.

The only functional comment is about the hsw err irq blocking, at least to
me it looks not required.

Yours, Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c      |  307 +++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h      |   13 +-
>  drivers/gpu/drm/i915/intel_display.c |   17 +-
>  drivers/gpu/drm/i915/intel_drv.h     |   10 ++
>  4 files changed, 334 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 7531095..d0f9c47 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -57,6 +57,208 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>  	}
>  }
>  
> +static bool disable_gen7_err_int(struct drm_i915_private *dev_priv)

It took me a while to figure out that this just checks whether underrun
reporting is disabled on any crtc and whether we hence need to disable it
completely. Usually I presume that a function call verb_something actually
does what the verb say, e.g. disable_foo I expect that the function
actually disables foo.

Since I can't come up with a concise name for these two functions here
(have_pipe_with_disabled_underrun_reporting is the best I could do ...)
maybe just inline it into their respective (only) callsite and add a
comment before the loop saying what it does? Maybe like

	bool can_enable_underrun_reporting = true;

	/* No per-crtc underrun irq disable bit, only a global one. */
	for_each_pipe {
		if (crtc->underrun_reporting_disabled)
			can_enable_underrun_reporting = false;
	}

	if (enable && can_enable_underrun_reporting)
		/* enable */
	else
		/* disable */
> +{
> +	struct intel_crtc *crtc;
> +	enum pipe pipe;
> +
> +	for_each_pipe(pipe) {
> +		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> +
> +		if (crtc->disable_cpu_fifo_underrun)

Same here, I'd go with cpu_fifo_underrun_disabled to make it clear that
it's a state tracking, not a request/action variable.

> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool disable_serr_int(struct drm_i915_private *dev_priv)
> +{
> +	enum pipe pipe;
> +	struct intel_crtc *crtc;
> +
> +	for_each_pipe(pipe) {
> +		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> +
> +		if (crtc->disable_pch_fifo_underrun)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static void ironlake_report_fifo_underrun(struct drm_device *dev,
> +					  enum pipe pipe, bool enable)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t bit = (pipe == PIPE_A) ? DE_PIPEA_FIFO_UNDERRUN :
> +					  DE_PIPEB_FIFO_UNDERRUN;
> +
> +	if (enable)
> +		ironlake_enable_display_irq(dev_priv, bit);
> +	else
> +		ironlake_disable_display_irq(dev_priv, bit);
> +}
> +
> +static void ivybridge_report_fifo_underrun(struct drm_device *dev, bool enable)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (enable) {
> +		if (disable_gen7_err_int(dev_priv))
> +			return;
> +
> +		I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN_A |
> +					 ERR_INT_FIFO_UNDERRUN_B |
> +					 ERR_INT_FIFO_UNDERRUN_C);
> +
> +		ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
> +	} else {
> +		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
> +	}
> +}
> +
> +static void ibx_report_fifo_underrun(struct intel_crtc *crtc, bool enable)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t bit = (crtc->pipe == PIPE_A) ? SDE_TRANSA_FIFO_UNDER :
> +						SDE_TRANSB_FIFO_UNDER;
> +
> +	if (enable)
> +		I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~bit);
> +	else
> +		I915_WRITE(SDEIMR, I915_READ(SDEIMR) | bit);
> +
> +	POSTING_READ(SDEIMR);
> +}
> +
> +static void cpt_report_fifo_underrun(struct drm_device *dev,
> +				     enum transcoder pch_transcoder,
> +				     bool enable)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (enable) {
> +		if (disable_serr_int(dev_priv))
> +			return;
> +
> +		I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
> +				     SERR_INT_TRANS_B_FIFO_UNDERRUN |
> +				     SERR_INT_TRANS_C_FIFO_UNDERRUN);
> +
> +		I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~SDE_ERROR_CPT);
> +	} else {
> +		I915_WRITE(SDEIMR, I915_READ(SDEIMR) | SDE_ERROR_CPT);
> +	}
> +
> +	POSTING_READ(SDEIMR);
> +}
> +
> +/**
> + * intel_report_cpu_fifo_underrun - enable/disable FIFO underrun messages
> + * @dev: drm device
> + * @pipe: pipe
> + * @enable: true if we want to report FIFO underrun errors, false otherwise
> + *
> + * This function makes us disable or report CPU fifo underruns for a specific

"disable or enable" (instead of report) is imo clearer, see below.

> + * pipe. Notice that on some Gens (e.g. IVB, HSW), disabling FIFO underrun
> + * reporting for one pipe may also disable all the other CPU error interruts for
> + * the other pipes, due to the fact that there's just one interrupt mask/enable
> + * bit for all the pipes.
> + *
> + * Returns the previous state of underrun reporting.
> + */
> +bool intel_report_cpu_fifo_underrun(struct drm_device *dev, enum pipe pipe,
> +				    bool enable)

Again I've tripped up about the report verb, presuming that this function
reports underruns. But it only updates the underrun reporting state, so
maybe intel_*_fifo_underrun_reporting?

> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	unsigned long flags;
> +	bool ret;
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> +
> +	ret = !intel_crtc->disable_cpu_fifo_underrun;
> +
> +	if (enable == ret)
> +		goto done;
> +
> +	intel_crtc->disable_cpu_fifo_underrun = !enable;
> +
> +	if (IS_GEN5(dev) || IS_GEN6(dev))
> +		ironlake_report_fifo_underrun(dev, pipe, enable);
> +	else if (IS_GEN7(dev))
> +		ivybridge_report_fifo_underrun(dev, enable);
> +
> +done:
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> +	return ret;
> +}
> +
> +/**
> + * intel_report_pch_fifo_underrun - enable/disable FIFO underrun messages
> + * @dev: drm device
> + * @pch_transcoder: the PCH transcoder (same as pipe on IVB and older)
> + * @enable: true if we want to report FIFO underrun errors, false otherwise
> + *
> + * This function makes us disable or report PCH fifo underruns for a specific
> + * PCH transcoder. Notice that on some PCHs (e.g. CPT/PPT), disabling FIFO
> + * underrun reporting for one transcoder may also disable all the other PCH
> + * error interruts for the other transcoders, due to the fact that there's just
> + * one interrupt mask/enable bit for all the transcoders.
> + *
> + * Returns the previous state of underrun reporting.
> + */
> +bool intel_report_pch_fifo_underrun(struct drm_device *dev,
> +				    enum transcoder pch_transcoder,
> +				    bool enable)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum pipe p;
> +	struct drm_crtc *crtc;
> +	struct intel_crtc *intel_crtc;
> +	unsigned long flags;
> +	bool ret;
> +
> +	if (HAS_PCH_LPT(dev)) {
> +		crtc = NULL;
> +		for_each_pipe(p) {
> +			struct drm_crtc *c = dev_priv->pipe_to_crtc_mapping[p];
> +			if (intel_pipe_has_type(c, INTEL_OUTPUT_ANALOG)) {
> +				crtc = c;
> +				break;
> +			}
> +		}
> +		if (!crtc) {
> +			DRM_ERROR("PCH FIFO underrun, but no CRTC using the PCH found\n");
> +			return false;
> +		}
> +	} else {
> +		crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder];
> +	}
> +	intel_crtc = to_intel_crtc(crtc);
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, flags);
> +
> +	ret = !intel_crtc->disable_pch_fifo_underrun;
> +
> +	if (enable == ret)
> +		goto done;
> +
> +	intel_crtc->disable_pch_fifo_underrun = !enable;
> +
> +	if (HAS_PCH_IBX(dev))
> +		ibx_report_fifo_underrun(intel_crtc, enable);
> +	else
> +		cpt_report_fifo_underrun(dev, pch_transcoder, enable);
> +
> +done:
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> +	return ret;
> +}
> +
>  void
>  i915_enable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask)
>  {
> @@ -659,14 +861,57 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
>  	if (pch_iir & (SDE_TRANSB_CRC_ERR | SDE_TRANSA_CRC_ERR))
>  		DRM_DEBUG_DRIVER("PCH transcoder CRC error interrupt\n");
>  
> -	if (pch_iir & SDE_TRANSB_FIFO_UNDER)
> -		DRM_DEBUG_DRIVER("PCH transcoder B underrun interrupt\n");
>  	if (pch_iir & SDE_TRANSA_FIFO_UNDER)
> -		DRM_DEBUG_DRIVER("PCH transcoder A underrun interrupt\n");
> +		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false))
> +			DRM_ERROR("PCH transcoder A FIFO underrun\n");

It sounds like we have outstanding issues with underruns still, but I'd
like to merge your patch sooner than later (it looks like the kind with
massive rebase pain). So keep things at DRM_DEBUG_DRIVER meanwhile, until
we've fixed the know ones?

> +
> +	if (pch_iir & SDE_TRANSB_FIFO_UNDER)
> +		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_B, false))
> +			DRM_ERROR("PCH transcoder B FIFO underrun\n");
>  }
>  
> +static void err_int_handler(struct drm_device *dev)

snb_ prefix should be added imo.

> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 err_int = I915_READ(GEN7_ERR_INT);
> +
> +	if (err_int & ERR_INT_FIFO_UNDERRUN_A)
> +		if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
> +			DRM_ERROR("Pipe A FIFO underrun\n");
> +
> +	if (err_int & ERR_INT_FIFO_UNDERRUN_B)
> +		if (intel_report_cpu_fifo_underrun(dev, PIPE_B, false))
> +			DRM_ERROR("Pipe B FIFO underrun\n");
> +
> +	if (err_int & ERR_INT_FIFO_UNDERRUN_C)
> +		if (intel_report_cpu_fifo_underrun(dev, PIPE_C, false))
> +			DRM_ERROR("Pipe C FIFO underrun\n");
> +
> +	I915_WRITE(GEN7_ERR_INT, err_int);
> +}
> +
> +static void serr_int_handler(struct drm_device *dev)

Same here for an cpt_ prefix.

> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 serr_int = I915_READ(SERR_INT);
> +
> +	if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
> +		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false))
> +			DRM_ERROR("PCH transcoder A FIFO underrun\n");
> +
> +	if (serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN)
> +		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_B, false))
> +			DRM_ERROR("PCH transcoder B FIFO underrun\n");
> +
> +	if (serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN)
> +		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_C, false))
> +			DRM_ERROR("PCH transcoder C FIFO underrun\n");
> +
> +	I915_WRITE(SERR_INT, serr_int);
> +}
>  static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>  {
> +
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	int pipe;
>  
> @@ -695,6 +940,9 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>  			DRM_DEBUG_DRIVER("  pipe %c FDI IIR: 0x%08x\n",
>  					 pipe_name(pipe),
>  					 I915_READ(FDI_RX_IIR(pipe)));
> +
> +	if (pch_iir & SDE_ERROR_CPT)
> +		serr_int_handler(dev);
>  }
>  
>  static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
> @@ -707,6 +955,14 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  
>  	atomic_inc(&dev_priv->irq_received);
>  
> +	/* We get interrupts on unclaimed registers, so check for this before we
> +	 * do any I915_{READ,WRITE}. */
> +	if (IS_HASWELL(dev) &&
> +	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
> +		DRM_ERROR("Unclaimed register before interrupt\n");
> +		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> +	}
> +
>  	/* disable master interrupt before clearing iir  */
>  	de_ier = I915_READ(DEIER);
>  	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> @@ -720,6 +976,12 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  	I915_WRITE(SDEIER, 0);
>  	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))
> +		ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);

The above two chunks look like they've escaped from your unclaimed
register series. Do we still need those?

Imo if we manage to create an unclaimed register interrupt in the irq
handler, we deserve it. And the linux irq subsystem already ensures that a
given irq handler can't run in parallel (if another irq fires while it's
running, an immediate 2nd run is enqueued).

> +
>  	gt_iir = I915_READ(GTIIR);
>  	if (gt_iir) {
>  		snb_gt_irq_handler(dev, dev_priv, gt_iir);
> @@ -729,6 +991,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  
>  	de_iir = I915_READ(DEIIR);
>  	if (de_iir) {
> +		if (de_iir & DE_ERR_INT_IVB)
> +			err_int_handler(dev);
> +
>  		if (de_iir & DE_AUX_CHANNEL_A_IVB)
>  			dp_aux_irq_handler(dev);
>  
> @@ -766,6 +1031,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  		ret = IRQ_HANDLED;
>  	}
>  
> +	if (IS_HASWELL(dev) && !disable_gen7_err_int(dev_priv))
> +		ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
> +
>  	I915_WRITE(DEIER, de_ier);
>  	POSTING_READ(DEIER);
>  	I915_WRITE(SDEIER, sde_ier);
> @@ -833,6 +1101,14 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	if (de_iir & DE_PIPEB_VBLANK)
>  		drm_handle_vblank(dev, 1);
>  
> +	if (de_iir & DE_PIPEA_FIFO_UNDERRUN)
> +		if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
> +			DRM_ERROR("Pipe A FIFO underrun\n");
> +
> +	if (de_iir & DE_PIPEB_FIFO_UNDERRUN)
> +		if (intel_report_cpu_fifo_underrun(dev, PIPE_B, false))
> +			DRM_ERROR("Pipe B FIFO underrun\n");
> +
>  	if (de_iir & DE_PLANEA_FLIP_DONE) {
>  		intel_prepare_page_flip(dev, 0);
>  		intel_finish_page_flip_plane(dev, 0);
> @@ -1966,14 +2242,20 @@ static void ibx_irq_postinstall(struct drm_device *dev)
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	u32 mask;
>  
> -	if (HAS_PCH_IBX(dev))
> +	if (HAS_PCH_IBX(dev)) {
>  		mask = SDE_HOTPLUG_MASK |
>  		       SDE_GMBUS |
> -		       SDE_AUX_MASK;
> -	else
> +		       SDE_AUX_MASK |
> +		       SDE_TRANSB_FIFO_UNDER |
> +		       SDE_TRANSA_FIFO_UNDER;
> +	} else {
>  		mask = SDE_HOTPLUG_MASK_CPT |
>  		       SDE_GMBUS_CPT |
> -		       SDE_AUX_MASK_CPT;
> +		       SDE_AUX_MASK_CPT |
> +		       SDE_ERROR_CPT;
> +
> +		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
> +	}
>  
>  	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
>  	I915_WRITE(SDEIMR, ~mask);
> @@ -1989,7 +2271,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  	/* enable kind of interrupts always enabled */
>  	u32 display_mask = DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
>  			   DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
> -			   DE_AUX_CHANNEL_A;
> +			   DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
> +			   DE_PIPEA_FIFO_UNDERRUN;
>  	u32 render_irqs;
>  
>  	dev_priv->irq_mask = ~display_mask;
> @@ -2039,12 +2322,14 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>  		DE_PLANEC_FLIP_DONE_IVB |
>  		DE_PLANEB_FLIP_DONE_IVB |
>  		DE_PLANEA_FLIP_DONE_IVB |
> -		DE_AUX_CHANNEL_A_IVB;
> +		DE_AUX_CHANNEL_A_IVB |
> +		DE_ERR_INT_IVB;
>  	u32 render_irqs;
>  
>  	dev_priv->irq_mask = ~display_mask;
>  
>  	/* should always can generate irq */
> +	I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>  	I915_WRITE(DEIIR, I915_READ(DEIIR));
>  	I915_WRITE(DEIMR, dev_priv->irq_mask);
>  	I915_WRITE(DEIER,
> @@ -2195,6 +2480,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  	I915_WRITE(DEIMR, 0xffffffff);
>  	I915_WRITE(DEIER, 0x0);
>  	I915_WRITE(DEIIR, I915_READ(DEIIR));
> +	if (IS_GEN7(dev))
> +		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>  
>  	I915_WRITE(GTIMR, 0xffffffff);
>  	I915_WRITE(GTIER, 0x0);
> @@ -2203,6 +2490,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  	I915_WRITE(SDEIMR, 0xffffffff);
>  	I915_WRITE(SDEIER, 0x0);
>  	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
> +	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
> +		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
>  }
>  
>  static void i8xx_irq_preinstall(struct drm_device * dev)
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cd226c2..4d27320 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -520,7 +520,10 @@
>  
>  #define ERROR_GEN6	0x040a0
>  #define GEN7_ERR_INT	0x44040
> -#define   ERR_INT_MMIO_UNCLAIMED (1<<13)
> +#define   ERR_INT_MMIO_UNCLAIMED	(1<<13)
> +#define   ERR_INT_FIFO_UNDERRUN_C	(1<<6)
> +#define   ERR_INT_FIFO_UNDERRUN_B	(1<<3)
> +#define   ERR_INT_FIFO_UNDERRUN_A	(1<<0)
>  
>  #define FPGA_DBG		0x42300
>  #define   FPGA_DBG_RM_NOCLAIM	(1<<31)
> @@ -3390,7 +3393,7 @@
>  #define DE_PIPEA_FIFO_UNDERRUN  (1 << 0)
>  
>  /* More Ivybridge lolz */
> -#define DE_ERR_DEBUG_IVB		(1<<30)
> +#define DE_ERR_INT_IVB			(1<<30)
>  #define DE_GSE_IVB			(1<<29)
>  #define DE_PCH_EVENT_IVB		(1<<28)
>  #define DE_DP_A_HOTPLUG_IVB		(1<<27)
> @@ -3540,6 +3543,7 @@
>  				 SDE_PORTC_HOTPLUG_CPT |	\
>  				 SDE_PORTB_HOTPLUG_CPT)
>  #define SDE_GMBUS_CPT		(1 << 17)
> +#define SDE_ERROR_CPT		(1 << 16)
>  #define SDE_AUDIO_CP_REQ_C_CPT	(1 << 10)
>  #define SDE_AUDIO_CP_CHG_C_CPT	(1 << 9)
>  #define SDE_FDI_RXC_CPT		(1 << 8)
> @@ -3564,6 +3568,11 @@
>  #define SDEIIR  0xc4008
>  #define SDEIER  0xc400c
>  
> +#define SERR_INT			0xc4040
> +#define  SERR_INT_TRANS_C_FIFO_UNDERRUN	(1<<6)
> +#define  SERR_INT_TRANS_B_FIFO_UNDERRUN	(1<<3)
> +#define  SERR_INT_TRANS_A_FIFO_UNDERRUN	(1<<0)
> +
>  /* digital port hotplug */
>  #define PCH_PORT_HOTPLUG        0xc4030		/* SHOTPLUG_CTL */
>  #define PORTD_HOTPLUG_ENABLE            (1 << 20)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9b0cd86..7152f35 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -41,7 +41,6 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <linux/dma_remapping.h>
>  
> -bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>  
> @@ -3305,6 +3304,10 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  		return;
>  
>  	intel_crtc->active = true;
> +
> +	intel_report_cpu_fifo_underrun(dev, pipe, true);
> +	intel_report_pch_fifo_underrun(dev, pipe, true);
> +
>  	intel_update_watermarks(dev);
>  
>  	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> @@ -3397,10 +3400,15 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  		return;
>  
>  	intel_crtc->active = true;
> -	intel_update_watermarks(dev);
>  
>  	is_pch_port = haswell_crtc_driving_pch(crtc);
>  
> +	intel_report_cpu_fifo_underrun(dev, pipe, true);
> +	if (is_pch_port)
> +		intel_report_pch_fifo_underrun(dev, TRANSCODER_A, true);
> +
> +	intel_update_watermarks(dev);
> +
>  	if (is_pch_port)
>  		dev_priv->display.fdi_link_train(crtc);
>  
> @@ -3484,6 +3492,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	if (dev_priv->cfb_plane == plane)
>  		intel_disable_fbc(dev);
>  
> +	intel_report_pch_fifo_underrun(dev, pipe, false);
>  	intel_disable_pipe(dev_priv, pipe);
>  
>  	/* Disable PF */
> @@ -3497,6 +3506,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	ironlake_fdi_disable(crtc);
>  
>  	ironlake_disable_pch_transcoder(dev_priv, pipe);
> +	intel_report_pch_fifo_underrun(dev, pipe, true);
>  
>  	if (HAS_PCH_CPT(dev)) {
>  		/* disable TRANS_DP_CTL */
> @@ -3566,6 +3576,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	if (dev_priv->cfb_plane == plane)
>  		intel_disable_fbc(dev);
>  
> +	if (is_pch_port)
> +		intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false);
>  	intel_disable_pipe(dev_priv, pipe);
>  
>  	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
> @@ -3582,6 +3594,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  
>  	if (is_pch_port) {
>  		lpt_disable_pch_transcoder(dev_priv);
> +		intel_report_pch_fifo_underrun(dev, TRANSCODER_A, true);
>  		intel_ddi_fdi_disable(crtc);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 010e998..aa8f948 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -238,6 +238,9 @@ struct intel_crtc {
>  
>  	/* reset counter value when the last flip was submitted */
>  	unsigned int reset_counter;
> +
> +	bool disable_cpu_fifo_underrun;
> +	bool disable_pch_fifo_underrun;
>  };
>  
>  struct intel_plane {
> @@ -442,6 +445,7 @@ int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
>  extern void intel_attach_force_audio_property(struct drm_connector *connector);
>  extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
>  
> +extern bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
>  extern void intel_crt_init(struct drm_device *dev);
>  extern void intel_hdmi_init(struct drm_device *dev,
>  			    int hdmi_reg, enum port port);
> @@ -697,5 +701,11 @@ intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
>  extern void intel_ddi_fdi_disable(struct drm_crtc *crtc);
>  
>  extern void intel_display_handle_reset(struct drm_device *dev);
> +extern bool intel_report_cpu_fifo_underrun(struct drm_device *dev,
> +					   enum pipe pipe,
> +					   bool enable);
> +extern bool intel_report_pch_fifo_underrun(struct drm_device *dev,
> +					   enum transcoder pch_transcoder,
> +					   bool enable);
>  
>  #endif /* __INTEL_DRV_H__ */
> -- 
> 1.7.10.4
> 
> _______________________________________________
> 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

Re: [PATCH 3/4] drm/i915: print Gen5+ CPU/PCH poison interrupts

[Daniel Vetter]

On Fri, Feb 22, 2013 at 05:05:30PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This is bad news and shouldn't be happening.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

No bikesheds here and I like this. So when you resend the underrun
reporting patch, please also resend this one here, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c |   14 ++++++++++++--
>  drivers/gpu/drm/i915/i915_reg.h |    2 ++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d0f9c47..98a57ed 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -875,6 +875,9 @@ static void err_int_handler(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 err_int = I915_READ(GEN7_ERR_INT);
>  
> +	if (err_int & ERR_INT_POISON)
> +		DRM_ERROR("Poison interrupt\n");
> +
>  	if (err_int & ERR_INT_FIFO_UNDERRUN_A)
>  		if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
>  			DRM_ERROR("Pipe A FIFO underrun\n");
> @@ -895,6 +898,9 @@ static void serr_int_handler(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 serr_int = I915_READ(SERR_INT);
>  
> +	if (serr_int & SERR_INT_POISON)
> +		DRM_ERROR("PCH poison interrupt\n");
> +
>  	if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
>  		if (intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false))
>  			DRM_ERROR("PCH transcoder A FIFO underrun\n");
> @@ -1101,6 +1107,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	if (de_iir & DE_PIPEB_VBLANK)
>  		drm_handle_vblank(dev, 1);
>  
> +	if (de_iir & DE_POISON)
> +		DRM_ERROR("Poison interrupt\n");
> +
>  	if (de_iir & DE_PIPEA_FIFO_UNDERRUN)
>  		if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
>  			DRM_ERROR("Pipe A FIFO underrun\n");
> @@ -2247,7 +2256,8 @@ static void ibx_irq_postinstall(struct drm_device *dev)
>  		       SDE_GMBUS |
>  		       SDE_AUX_MASK |
>  		       SDE_TRANSB_FIFO_UNDER |
> -		       SDE_TRANSA_FIFO_UNDER;
> +		       SDE_TRANSA_FIFO_UNDER |
> +		       SDE_POISON;
>  	} else {
>  		mask = SDE_HOTPLUG_MASK_CPT |
>  		       SDE_GMBUS_CPT |
> @@ -2272,7 +2282,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  	u32 display_mask = DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
>  			   DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
>  			   DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
> -			   DE_PIPEA_FIFO_UNDERRUN;
> +			   DE_PIPEA_FIFO_UNDERRUN | DE_POISON;
>  	u32 render_irqs;
>  
>  	dev_priv->irq_mask = ~display_mask;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 4d27320..62cfcea 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -520,6 +520,7 @@
>  
>  #define ERROR_GEN6	0x040a0
>  #define GEN7_ERR_INT	0x44040
> +#define   ERR_INT_POISON		(1<<31)
>  #define   ERR_INT_MMIO_UNCLAIMED	(1<<13)
>  #define   ERR_INT_FIFO_UNDERRUN_C	(1<<6)
>  #define   ERR_INT_FIFO_UNDERRUN_B	(1<<3)
> @@ -3569,6 +3570,7 @@
>  #define SDEIER  0xc400c
>  
>  #define SERR_INT			0xc4040
> +#define  SERR_INT_POISON		(1<<31)
>  #define  SERR_INT_TRANS_C_FIFO_UNDERRUN	(1<<6)
>  #define  SERR_INT_TRANS_B_FIFO_UNDERRUN	(1<<3)
>  #define  SERR_INT_TRANS_A_FIFO_UNDERRUN	(1<<0)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> 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

Re: [PATCH 2/4] drm/i915: report Gen5+ CPU and PCH FIFO underruns

[Daniel Vetter]

On Fri, Feb 22, 2013 at 9:05 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 010e998..aa8f948 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -238,6 +238,9 @@ struct intel_crtc {
>
>         /* reset counter value when the last flip was submitted */
>         unsigned int reset_counter;
> +
> +       bool disable_cpu_fifo_underrun;
> +       bool disable_pch_fifo_underrun;
>  };

Forgot one: This needs a comment saying that the underrun stuff is
protected by dev_priv->irq_lock. I kinda like massive locks which
protect all kinds of things less, so I'd personally lean towards a new
dev_priv->underrun_reporting_lock. But that's now a real bikeshed, so
pls ignore ;-) Especially since it really fits well with the already
existing users of the irq_lock.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/4] drm/i915: report Gen5+ CPU and PCH FIFO underruns

[Paulo Zanoni]

Hi

2013/3/28 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Feb 22, 2013 at 05:05:29PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> In this commit we enable both CPU and PCH FIFO underrun reporting and
>> start reporting them. We follow a few rules:
>>   - after we receive one of these errors, we mask the interrupt, so
>>     we won't get an "interrupt storm" and we also won't flood dmesg;
>>   - at each mode set we enable the interrupts again, so we'll see each
>>     message at most once per mode set;
>>   - in the specific places where we need to ignore the errors, we
>>     completely mask the interrupts.
>>
>> The downside of this patch is that since we're completely disabling
>> (masking) the interrupts instead of just not printing error messages,
>> we will mask more than just what we want on IVB/HSW CPU interrupts
>> (due to GEN7_ERR_INT) and on CPT/PPT/LPT PCHs (due to SERR_INT). So
>> when we decide to mask PCH FIFO underruns for pipe A on CPT, we'll
>> also be masking PCH FIFO underruns for pipe B, because both are
>> reported by SERR_INT which has to be either completely enabled or
>> completely disabled (in othe words, there's no way to disable/enable
>> specific bits of GEN7_ERR_INT and SERR_INT).
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Ok, sorry for the long delay in reviewing this (I've somehow hoped that
> someone else would do it, but meh ...).
>
> I'm happy with the logic in your patch here, but it took me a while to
> piece together the control flow and what exactly is done where. I think
> that can be improved by renaming a few functions to make it clearer what
> exactly they're doing. Suggestions below.

Yeah, looks like the names were not really good. See below.


>
> The only functional comment is about the hsw err irq blocking, at least to
> me it looks not required.
>
> Yours, Daniel
>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c      |  307 +++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/i915_reg.h      |   13 +-
>>  drivers/gpu/drm/i915/intel_display.c |   17 +-
>>  drivers/gpu/drm/i915/intel_drv.h     |   10 ++
>>  4 files changed, 334 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 7531095..d0f9c47 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -57,6 +57,208 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>>       }
>>  }
>>
>> +static bool disable_gen7_err_int(struct drm_i915_private *dev_priv)
>
> It took me a while to figure out that this just checks whether underrun
> reporting is disabled on any crtc and whether we hence need to disable it
> completely. Usually I presume that a function call verb_something actually
> does what the verb say, e.g. disable_foo I expect that the function
> actually disables foo.
>
> Since I can't come up with a concise name for these two functions here
> (have_pipe_with_disabled_underrun_reporting is the best I could do ...)
> maybe just inline it into their respective (only) callsite and add a
> comment before the loop saying what it does? Maybe like
>
>         bool can_enable_underrun_reporting = true;
>
>         /* No per-crtc underrun irq disable bit, only a global one. */
>         for_each_pipe {
>                 if (crtc->underrun_reporting_disabled)
>                         can_enable_underrun_reporting = false;
>         }
>
>         if (enable && can_enable_underrun_reporting)
>                 /* enable */
>         else
>                 /* disable */

I renamed disable_gen7_err_int() to ivb_can_enable_err_int() (and
inverted its behavior, of course). Same thing with
cpt_can_enable_serr_int(). I hope it's understandable now :)

>> +{
>> +     struct intel_crtc *crtc;
>> +     enum pipe pipe;
>> +
>> +     for_each_pipe(pipe) {
>> +             crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>> +
>> +             if (crtc->disable_cpu_fifo_underrun)
>
> Same here, I'd go with cpu_fifo_underrun_disabled to make it clear that
> it's a state tracking, not a request/action variable.

Done.

>
>> +                     return true;
>> +     }
>> +
>> +     return false;
>> +}
>> +
>> +static bool disable_serr_int(struct drm_i915_private *dev_priv)
>> +{
>> +     enum pipe pipe;
>> +     struct intel_crtc *crtc;
>> +
>> +     for_each_pipe(pipe) {
>> +             crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>> +
>> +             if (crtc->disable_pch_fifo_underrun)
>> +                     return true;
>> +     }
>> +
>> +     return false;
>> +}
>> +
>> +static void ironlake_report_fifo_underrun(struct drm_device *dev,
>> +                                       enum pipe pipe, bool enable)
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     uint32_t bit = (pipe == PIPE_A) ? DE_PIPEA_FIFO_UNDERRUN :
>> +                                       DE_PIPEB_FIFO_UNDERRUN;
>> +
>> +     if (enable)
>> +             ironlake_enable_display_irq(dev_priv, bit);
>> +     else
>> +             ironlake_disable_display_irq(dev_priv, bit);
>> +}
>> +
>> +static void ivybridge_report_fifo_underrun(struct drm_device *dev, bool enable)
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     if (enable) {
>> +             if (disable_gen7_err_int(dev_priv))
>> +                     return;
>> +
>> +             I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN_A |
>> +                                      ERR_INT_FIFO_UNDERRUN_B |
>> +                                      ERR_INT_FIFO_UNDERRUN_C);
>> +
>> +             ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> +     } else {
>> +             ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> +     }
>> +}
>> +
>> +static void ibx_report_fifo_underrun(struct intel_crtc *crtc, bool enable)
>> +{
>> +     struct drm_device *dev = crtc->base.dev;
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     uint32_t bit = (crtc->pipe == PIPE_A) ? SDE_TRANSA_FIFO_UNDER :
>> +                                             SDE_TRANSB_FIFO_UNDER;
>> +
>> +     if (enable)
>> +             I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~bit);
>> +     else
>> +             I915_WRITE(SDEIMR, I915_READ(SDEIMR) | bit);
>> +
>> +     POSTING_READ(SDEIMR);
>> +}
>> +
>> +static void cpt_report_fifo_underrun(struct drm_device *dev,
>> +                                  enum transcoder pch_transcoder,
>> +                                  bool enable)
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     if (enable) {
>> +             if (disable_serr_int(dev_priv))
>> +                     return;
>> +
>> +             I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
>> +                                  SERR_INT_TRANS_B_FIFO_UNDERRUN |
>> +                                  SERR_INT_TRANS_C_FIFO_UNDERRUN);
>> +
>> +             I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~SDE_ERROR_CPT);
>> +     } else {
>> +             I915_WRITE(SDEIMR, I915_READ(SDEIMR) | SDE_ERROR_CPT);
>> +     }
>> +
>> +     POSTING_READ(SDEIMR);
>> +}
>> +
>> +/**
>> + * intel_report_cpu_fifo_underrun - enable/disable FIFO underrun messages
>> + * @dev: drm device
>> + * @pipe: pipe
>> + * @enable: true if we want to report FIFO underrun errors, false otherwise
>> + *
>> + * This function makes us disable or report CPU fifo underruns for a specific
>
> "disable or enable" (instead of report) is imo clearer, see below.

Done.

>
>> + * pipe. Notice that on some Gens (e.g. IVB, HSW), disabling FIFO underrun
>> + * reporting for one pipe may also disable all the other CPU error interruts for
>> + * the other pipes, due to the fact that there's just one interrupt mask/enable
>> + * bit for all the pipes.
>> + *
>> + * Returns the previous state of underrun reporting.
>> + */
>> +bool intel_report_cpu_fifo_underrun(struct drm_device *dev, enum pipe pipe,
>> +                                 bool enable)
>
> Again I've tripped up about the report verb, presuming that this function
> reports underruns. But it only updates the underrun reporting state, so
> maybe intel_*_fifo_underrun_reporting?

Changed to intel_set_{cpu,pch}_fifo_underrun_reporting.

>
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>> +     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +     unsigned long flags;
>> +     bool ret;
>> +
>> +     spin_lock_irqsave(&dev_priv->irq_lock, flags);
>> +
>> +     ret = !intel_crtc->disable_cpu_fifo_underrun;
>> +
>> +     if (enable == ret)
>> +             goto done;
>> +
>> +     intel_crtc->disable_cpu_fifo_underrun = !enable;
>> +
>> +     if (IS_GEN5(dev) || IS_GEN6(dev))
>> +             ironlake_report_fifo_underrun(dev, pipe, enable);
>> +     else if (IS_GEN7(dev))
>> +             ivybridge_report_fifo_underrun(dev, enable);
>> +
>> +done:
>> +     spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>> +     return ret;
>> +}
>> +
>> +/**
>> + * intel_report_pch_fifo_underrun - enable/disable FIFO underrun messages
>> + * @dev: drm device
>> + * @pch_transcoder: the PCH transcoder (same as pipe on IVB and older)
>> + * @enable: true if we want to report FIFO underrun errors, false otherwise
>> + *
>> + * This function makes us disable or report PCH fifo underruns for a specific
>> + * PCH transcoder. Notice that on some PCHs (e.g. CPT/PPT), disabling FIFO
>> + * underrun reporting for one transcoder may also disable all the other PCH
>> + * error interruts for the other transcoders, due to the fact that there's just
>> + * one interrupt mask/enable bit for all the transcoders.
>> + *
>> + * Returns the previous state of underrun reporting.
>> + */
>> +bool intel_report_pch_fifo_underrun(struct drm_device *dev,
>> +                                 enum transcoder pch_transcoder,
>> +                                 bool enable)
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     enum pipe p;
>> +     struct drm_crtc *crtc;
>> +     struct intel_crtc *intel_crtc;
>> +     unsigned long flags;
>> +     bool ret;
>> +
>> +     if (HAS_PCH_LPT(dev)) {
>> +             crtc = NULL;
>> +             for_each_pipe(p) {
>> +                     struct drm_crtc *c = dev_priv->pipe_to_crtc_mapping[p];
>> +                     if (intel_pipe_has_type(c, INTEL_OUTPUT_ANALOG)) {
>> +                             crtc = c;
>> +                             break;
>> +                     }
>> +             }
>> +             if (!crtc) {
>> +                     DRM_ERROR("PCH FIFO underrun, but no CRTC using the PCH found\n");
>> +                     return false;
>> +             }
>> +     } else {
>> +             crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder];
>> +     }
>> +     intel_crtc = to_intel_crtc(crtc);
>> +
>> +     spin_lock_irqsave(&dev_priv->irq_lock, flags);
>> +
>> +     ret = !intel_crtc->disable_pch_fifo_underrun;
>> +
>> +     if (enable == ret)
>> +             goto done;
>> +
>> +     intel_crtc->disable_pch_fifo_underrun = !enable;
>> +
>> +     if (HAS_PCH_IBX(dev))
>> +             ibx_report_fifo_underrun(intel_crtc, enable);
>> +     else
>> +             cpt_report_fifo_underrun(dev, pch_transcoder, enable);
>> +
>> +done:
>> +     spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>> +     return ret;
>> +}
>> +
>>  void
>>  i915_enable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask)
>>  {
>> @@ -659,14 +861,57 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
>>       if (pch_iir & (SDE_TRANSB_CRC_ERR | SDE_TRANSA_CRC_ERR))
>>               DRM_DEBUG_DRIVER("PCH transcoder CRC error interrupt\n");
>>
>> -     if (pch_iir & SDE_TRANSB_FIFO_UNDER)
>> -             DRM_DEBUG_DRIVER("PCH transcoder B underrun interrupt\n");
>>       if (pch_iir & SDE_TRANSA_FIFO_UNDER)
>> -             DRM_DEBUG_DRIVER("PCH transcoder A underrun interrupt\n");
>> +             if (intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false))
>> +                     DRM_ERROR("PCH transcoder A FIFO underrun\n");
>
> It sounds like we have outstanding issues with underruns still, but I'd
> like to merge your patch sooner than later (it looks like the kind with
> massive rebase pain). So keep things at DRM_DEBUG_DRIVER meanwhile, until
> we've fixed the know ones?

Done. Let's see what happens when we ship this :)

>
>> +
>> +     if (pch_iir & SDE_TRANSB_FIFO_UNDER)
>> +             if (intel_report_pch_fifo_underrun(dev, TRANSCODER_B, false))
>> +                     DRM_ERROR("PCH transcoder B FIFO underrun\n");
>>  }
>>
>> +static void err_int_handler(struct drm_device *dev)
>
> snb_ prefix should be added imo.

Done. It's actually ivb_.

>
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     u32 err_int = I915_READ(GEN7_ERR_INT);
>> +
>> +     if (err_int & ERR_INT_FIFO_UNDERRUN_A)
>> +             if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
>> +                     DRM_ERROR("Pipe A FIFO underrun\n");
>> +
>> +     if (err_int & ERR_INT_FIFO_UNDERRUN_B)
>> +             if (intel_report_cpu_fifo_underrun(dev, PIPE_B, false))
>> +                     DRM_ERROR("Pipe B FIFO underrun\n");
>> +
>> +     if (err_int & ERR_INT_FIFO_UNDERRUN_C)
>> +             if (intel_report_cpu_fifo_underrun(dev, PIPE_C, false))
>> +                     DRM_ERROR("Pipe C FIFO underrun\n");
>> +
>> +     I915_WRITE(GEN7_ERR_INT, err_int);
>> +}
>> +
>> +static void serr_int_handler(struct drm_device *dev)
>
> Same here for an cpt_ prefix.

Done.

>
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +     u32 serr_int = I915_READ(SERR_INT);
>> +
>> +     if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
>> +             if (intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false))
>> +                     DRM_ERROR("PCH transcoder A FIFO underrun\n");
>> +
>> +     if (serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN)
>> +             if (intel_report_pch_fifo_underrun(dev, TRANSCODER_B, false))
>> +                     DRM_ERROR("PCH transcoder B FIFO underrun\n");
>> +
>> +     if (serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN)
>> +             if (intel_report_pch_fifo_underrun(dev, TRANSCODER_C, false))
>> +                     DRM_ERROR("PCH transcoder C FIFO underrun\n");
>> +
>> +     I915_WRITE(SERR_INT, serr_int);
>> +}
>>  static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>>  {
>> +
>>       drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>>       int pipe;
>>
>> @@ -695,6 +940,9 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>>                       DRM_DEBUG_DRIVER("  pipe %c FDI IIR: 0x%08x\n",
>>                                        pipe_name(pipe),
>>                                        I915_READ(FDI_RX_IIR(pipe)));
>> +
>> +     if (pch_iir & SDE_ERROR_CPT)
>> +             serr_int_handler(dev);
>>  }
>>
>>  static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>> @@ -707,6 +955,14 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>
>>       atomic_inc(&dev_priv->irq_received);
>>
>> +     /* We get interrupts on unclaimed registers, so check for this before we
>> +      * do any I915_{READ,WRITE}. */
>> +     if (IS_HASWELL(dev) &&
>> +         (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
>> +             DRM_ERROR("Unclaimed register before interrupt\n");
>> +             I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>> +     }
>> +
>>       /* disable master interrupt before clearing iir  */
>>       de_ier = I915_READ(DEIER);
>>       I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
>> @@ -720,6 +976,12 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>       I915_WRITE(SDEIER, 0);
>>       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))
>> +             ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>
> The above two chunks look like they've escaped from your unclaimed
> register series. Do we still need those?

Yes. We're enabling GEN7_ERR_INT on this patch, so now whenever we
poke an unclaimed register we'll get an interrupt. The good thing is
that this shouldn't be happening with our default configs :)

>
> Imo if we manage to create an unclaimed register interrupt in the irq
> handler, we deserve it. And the linux irq subsystem already ensures that a
> given irq handler can't run in parallel (if another irq fires while it's
> running, an immediate 2nd run is enqueued).

As far as I remember, depending on the case we may keep running irq
handlers forever. Also, we'll still print error messages about
unclaimed registers due to the FPGA_DBG register, so it's not like
errors will go unnoticed :)

I also added the comment suggested on your other email. Thanks for the review :)
<snip>

>
>> +
>>       gt_iir = I915_READ(GTIIR);
>>       if (gt_iir) {
>>               snb_gt_irq_handler(dev, dev_priv, gt_iir);
>> @@ -729,6 +991,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>
>>       de_iir = I915_READ(DEIIR);
>>       if (de_iir) {
>> +             if (de_iir & DE_ERR_INT_IVB)
>> +                     err_int_handler(dev);
>> +
>>               if (de_iir & DE_AUX_CHANNEL_A_IVB)
>>                       dp_aux_irq_handler(dev);
>>
>> @@ -766,6 +1031,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>               ret = IRQ_HANDLED;
>>       }
>>
>> +     if (IS_HASWELL(dev) && !disable_gen7_err_int(dev_priv))
>> +             ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> +
>>       I915_WRITE(DEIER, de_ier);
>>       POSTING_READ(DEIER);
>>       I915_WRITE(SDEIER, sde_ier);
>> @@ -833,6 +1101,14 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>>       if (de_iir & DE_PIPEB_VBLANK)
>>               drm_handle_vblank(dev, 1);
>>
>> +     if (de_iir & DE_PIPEA_FIFO_UNDERRUN)
>> +             if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
>> +                     DRM_ERROR("Pipe A FIFO underrun\n");
>> +
>> +     if (de_iir & DE_PIPEB_FIFO_UNDERRUN)
>> +             if (intel_report_cpu_fifo_underrun(dev, PIPE_B, false))
>> +                     DRM_ERROR("Pipe B FIFO underrun\n");
>> +
>>       if (de_iir & DE_PLANEA_FLIP_DONE) {
>>               intel_prepare_page_flip(dev, 0);
>>               intel_finish_page_flip_plane(dev, 0);
>> @@ -1966,14 +2242,20 @@ static void ibx_irq_postinstall(struct drm_device *dev)
>>       drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>>       u32 mask;
>>
>> -     if (HAS_PCH_IBX(dev))
>> +     if (HAS_PCH_IBX(dev)) {
>>               mask = SDE_HOTPLUG_MASK |
>>                      SDE_GMBUS |
>> -                    SDE_AUX_MASK;
>> -     else
>> +                    SDE_AUX_MASK |
>> +                    SDE_TRANSB_FIFO_UNDER |
>> +                    SDE_TRANSA_FIFO_UNDER;
>> +     } else {
>>               mask = SDE_HOTPLUG_MASK_CPT |
>>                      SDE_GMBUS_CPT |
>> -                    SDE_AUX_MASK_CPT;
>> +                    SDE_AUX_MASK_CPT |
>> +                    SDE_ERROR_CPT;
>> +
>> +             I915_WRITE(SERR_INT, I915_READ(SERR_INT));
>> +     }
>>
>>       I915_WRITE(SDEIIR, I915_READ(SDEIIR));
>>       I915_WRITE(SDEIMR, ~mask);
>> @@ -1989,7 +2271,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>>       /* enable kind of interrupts always enabled */
>>       u32 display_mask = DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
>>                          DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
>> -                        DE_AUX_CHANNEL_A;
>> +                        DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
>> +                        DE_PIPEA_FIFO_UNDERRUN;
>>       u32 render_irqs;
>>
>>       dev_priv->irq_mask = ~display_mask;
>> @@ -2039,12 +2322,14 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>>               DE_PLANEC_FLIP_DONE_IVB |
>>               DE_PLANEB_FLIP_DONE_IVB |
>>               DE_PLANEA_FLIP_DONE_IVB |
>> -             DE_AUX_CHANNEL_A_IVB;
>> +             DE_AUX_CHANNEL_A_IVB |
>> +             DE_ERR_INT_IVB;
>>       u32 render_irqs;
>>
>>       dev_priv->irq_mask = ~display_mask;
>>
>>       /* should always can generate irq */
>> +     I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>>       I915_WRITE(DEIIR, I915_READ(DEIIR));
>>       I915_WRITE(DEIMR, dev_priv->irq_mask);
>>       I915_WRITE(DEIER,
>> @@ -2195,6 +2480,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>>       I915_WRITE(DEIMR, 0xffffffff);
>>       I915_WRITE(DEIER, 0x0);
>>       I915_WRITE(DEIIR, I915_READ(DEIIR));
>> +     if (IS_GEN7(dev))
>> +             I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>>
>>       I915_WRITE(GTIMR, 0xffffffff);
>>       I915_WRITE(GTIER, 0x0);
>> @@ -2203,6 +2490,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>>       I915_WRITE(SDEIMR, 0xffffffff);
>>       I915_WRITE(SDEIER, 0x0);
>>       I915_WRITE(SDEIIR, I915_READ(SDEIIR));
>> +     if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
>> +             I915_WRITE(SERR_INT, I915_READ(SERR_INT));
>>  }
>>
>>  static void i8xx_irq_preinstall(struct drm_device * dev)
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index cd226c2..4d27320 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -520,7 +520,10 @@
>>
>>  #define ERROR_GEN6   0x040a0
>>  #define GEN7_ERR_INT 0x44040
>> -#define   ERR_INT_MMIO_UNCLAIMED (1<<13)
>> +#define   ERR_INT_MMIO_UNCLAIMED     (1<<13)
>> +#define   ERR_INT_FIFO_UNDERRUN_C    (1<<6)
>> +#define   ERR_INT_FIFO_UNDERRUN_B    (1<<3)
>> +#define   ERR_INT_FIFO_UNDERRUN_A    (1<<0)
>>
>>  #define FPGA_DBG             0x42300
>>  #define   FPGA_DBG_RM_NOCLAIM        (1<<31)
>> @@ -3390,7 +3393,7 @@
>>  #define DE_PIPEA_FIFO_UNDERRUN  (1 << 0)
>>
>>  /* More Ivybridge lolz */
>> -#define DE_ERR_DEBUG_IVB             (1<<30)
>> +#define DE_ERR_INT_IVB                       (1<<30)
>>  #define DE_GSE_IVB                   (1<<29)
>>  #define DE_PCH_EVENT_IVB             (1<<28)
>>  #define DE_DP_A_HOTPLUG_IVB          (1<<27)
>> @@ -3540,6 +3543,7 @@
>>                                SDE_PORTC_HOTPLUG_CPT |        \
>>                                SDE_PORTB_HOTPLUG_CPT)
>>  #define SDE_GMBUS_CPT                (1 << 17)
>> +#define SDE_ERROR_CPT                (1 << 16)
>>  #define SDE_AUDIO_CP_REQ_C_CPT       (1 << 10)
>>  #define SDE_AUDIO_CP_CHG_C_CPT       (1 << 9)
>>  #define SDE_FDI_RXC_CPT              (1 << 8)
>> @@ -3564,6 +3568,11 @@
>>  #define SDEIIR  0xc4008
>>  #define SDEIER  0xc400c
>>
>> +#define SERR_INT                     0xc4040
>> +#define  SERR_INT_TRANS_C_FIFO_UNDERRUN      (1<<6)
>> +#define  SERR_INT_TRANS_B_FIFO_UNDERRUN      (1<<3)
>> +#define  SERR_INT_TRANS_A_FIFO_UNDERRUN      (1<<0)
>> +
>>  /* digital port hotplug */
>>  #define PCH_PORT_HOTPLUG        0xc4030              /* SHOTPLUG_CTL */
>>  #define PORTD_HOTPLUG_ENABLE            (1 << 20)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 9b0cd86..7152f35 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -41,7 +41,6 @@
>>  #include <drm/drm_crtc_helper.h>
>>  #include <linux/dma_remapping.h>
>>
>> -bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
>>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>>
>> @@ -3305,6 +3304,10 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>>               return;
>>
>>       intel_crtc->active = true;
>> +
>> +     intel_report_cpu_fifo_underrun(dev, pipe, true);
>> +     intel_report_pch_fifo_underrun(dev, pipe, true);
>> +
>>       intel_update_watermarks(dev);
>>
>>       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
>> @@ -3397,10 +3400,15 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>>               return;
>>
>>       intel_crtc->active = true;
>> -     intel_update_watermarks(dev);
>>
>>       is_pch_port = haswell_crtc_driving_pch(crtc);
>>
>> +     intel_report_cpu_fifo_underrun(dev, pipe, true);
>> +     if (is_pch_port)
>> +             intel_report_pch_fifo_underrun(dev, TRANSCODER_A, true);
>> +
>> +     intel_update_watermarks(dev);
>> +
>>       if (is_pch_port)
>>               dev_priv->display.fdi_link_train(crtc);
>>
>> @@ -3484,6 +3492,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>>       if (dev_priv->cfb_plane == plane)
>>               intel_disable_fbc(dev);
>>
>> +     intel_report_pch_fifo_underrun(dev, pipe, false);
>>       intel_disable_pipe(dev_priv, pipe);
>>
>>       /* Disable PF */
>> @@ -3497,6 +3506,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>>       ironlake_fdi_disable(crtc);
>>
>>       ironlake_disable_pch_transcoder(dev_priv, pipe);
>> +     intel_report_pch_fifo_underrun(dev, pipe, true);
>>
>>       if (HAS_PCH_CPT(dev)) {
>>               /* disable TRANS_DP_CTL */
>> @@ -3566,6 +3576,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>>       if (dev_priv->cfb_plane == plane)
>>               intel_disable_fbc(dev);
>>
>> +     if (is_pch_port)
>> +             intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false);
>>       intel_disable_pipe(dev_priv, pipe);
>>
>>       intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
>> @@ -3582,6 +3594,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>>
>>       if (is_pch_port) {
>>               lpt_disable_pch_transcoder(dev_priv);
>> +             intel_report_pch_fifo_underrun(dev, TRANSCODER_A, true);
>>               intel_ddi_fdi_disable(crtc);
>>       }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 010e998..aa8f948 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -238,6 +238,9 @@ struct intel_crtc {
>>
>>       /* reset counter value when the last flip was submitted */
>>       unsigned int reset_counter;
>> +
>> +     bool disable_cpu_fifo_underrun;
>> +     bool disable_pch_fifo_underrun;
>>  };
>>
>>  struct intel_plane {
>> @@ -442,6 +445,7 @@ int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
>>  extern void intel_attach_force_audio_property(struct drm_connector *connector);
>>  extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
>>
>> +extern bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
>>  extern void intel_crt_init(struct drm_device *dev);
>>  extern void intel_hdmi_init(struct drm_device *dev,
>>                           int hdmi_reg, enum port port);
>> @@ -697,5 +701,11 @@ intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
>>  extern void intel_ddi_fdi_disable(struct drm_crtc *crtc);
>>
>>  extern void intel_display_handle_reset(struct drm_device *dev);
>> +extern bool intel_report_cpu_fifo_underrun(struct drm_device *dev,
>> +                                        enum pipe pipe,
>> +                                        bool enable);
>> +extern bool intel_report_pch_fifo_underrun(struct drm_device *dev,
>> +                                        enum transcoder pch_transcoder,
>> +                                        bool enable);
>>
>>  #endif /* __INTEL_DRV_H__ */
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> 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



--
Paulo Zanoni

Re: [PATCH 1/4] drm/i915: also disable south interrupts when handling them

[Daniel Vetter]

On Tue, Mar 5, 2013 at 8:08 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Feb 22, 2013 at 05:05:28PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> From the docs:
>>
>>   "IIR can queue up to two interrupt events. When the IIR is cleared,
>>   it will set itself again after one clock if a second event was
>>   stored."
>>
>>   "Only the rising edge of the PCH Display interrupt will cause the
>>   North Display IIR (DEIIR) PCH Display Interrupt even bit to be set,
>>   so all PCH Display Interrupts, including back to back interrupts,
>>   must be cleared before a new PCH Display interrupt can cause DEIIR
>>   to be set".
>>
>> The current code works fine because we don't get many interrupts, but
>> if we enable the PCH FIFO underrun interrupts we'll start getting so
>> many interrupts that at some point new PCH interrupts won't cause
>> DEIIR to be set.
>>
>> The initial implementation I tried was to turn the code that checks
>> SDEIIR into a loop, but we can still get interrupts even after the
>> loop is done (and before the irq handler finishes), so we have to
>> either disable the interrupts or mask them. In the end I concluded
>> that just disabling the PCH interrupts is enough, you don't even need
>> the loop, so this is what this patch implements. I've tested it and it
>> passes the 2 "PCH FIFO underrun interrupt storms" I can reproduce:
>> the "ironlake_crtc_disable" case and the "wrong watermarks" case.
>>
>> In other words, here's how to reproduce the problem fixed by this
>> patch:
>>   1 - Enable PCH FIFO underrun interrupts (SERR_INT on SNB+)
>>   2 - Boot the machine
>>   3 - While booting we'll get tons of PCH FIFO underrun interrupts
>>   4 - Plug a new monitor
>>   5 - Run xrandr, notice it won't detect the new monitor
>>   6 - Read SDEIIR and notice it's not 0 while DEIIR is 0
>>
>> Q: Can't we just clear DEIIR before SDEIIR?
>> A: It doesn't work. SDEIIR has to be completely cleared (including the
>> interrupts stored on its back queue) before it can flip DEIIR's bit to
>> 1 again, and even while you're clearing it you'll be getting more and
>> more interrupts.
>>
>> Q: Why does it work by just disabling+enabling the south interrupts?
>> A: Because when we re-enable them, if there's something on the SDEIIR
>> register (maybe an interrupt stored on the queue), the re-enabling
>> will make DEIIR's bit flip to 1, and since we'll already have
>> interrupts enabled we'll get another interrupt, then run our irq
>> handler again to process the "back" interrupts.
>>
>> v2: Even bigger commit message, added code comments.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Since this seems to fix the dp aux irq timeout regression I've merged this
> to -fixes. Also volunteered Imre for a review, I'll add that if it pops up
> in the next few days.
>
> Big thanks to Paulo&Imre for tracking this down.

Digging up zombies ... So reading through Oscar's execlist patches
I've had a new idea for fixing our various missed irq issues. The new
trick seems to work for ring interrupts, but I couldn't yet test the
pch interrupts.

Do you still remember on which platform we get tons of fifo underruns
on the pch when doing modesets? I've tried hdmi on my ivb here for a
start, but that didn't work out - no pch irq storm, not even a single
underrun :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/i915: also disable south interrupts when handling them

[Paulo Zanoni]

2014-05-23 5:13 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Mar 5, 2013 at 8:08 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Fri, Feb 22, 2013 at 05:05:28PM -0300, Paulo Zanoni wrote:
>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>
>>> From the docs:
>>>
>>>   "IIR can queue up to two interrupt events. When the IIR is cleared,
>>>   it will set itself again after one clock if a second event was
>>>   stored."
>>>
>>>   "Only the rising edge of the PCH Display interrupt will cause the
>>>   North Display IIR (DEIIR) PCH Display Interrupt even bit to be set,
>>>   so all PCH Display Interrupts, including back to back interrupts,
>>>   must be cleared before a new PCH Display interrupt can cause DEIIR
>>>   to be set".
>>>
>>> The current code works fine because we don't get many interrupts, but
>>> if we enable the PCH FIFO underrun interrupts we'll start getting so
>>> many interrupts that at some point new PCH interrupts won't cause
>>> DEIIR to be set.
>>>
>>> The initial implementation I tried was to turn the code that checks
>>> SDEIIR into a loop, but we can still get interrupts even after the
>>> loop is done (and before the irq handler finishes), so we have to
>>> either disable the interrupts or mask them. In the end I concluded
>>> that just disabling the PCH interrupts is enough, you don't even need
>>> the loop, so this is what this patch implements. I've tested it and it
>>> passes the 2 "PCH FIFO underrun interrupt storms" I can reproduce:
>>> the "ironlake_crtc_disable" case and the "wrong watermarks" case.
>>>
>>> In other words, here's how to reproduce the problem fixed by this
>>> patch:
>>>   1 - Enable PCH FIFO underrun interrupts (SERR_INT on SNB+)
>>>   2 - Boot the machine
>>>   3 - While booting we'll get tons of PCH FIFO underrun interrupts
>>>   4 - Plug a new monitor
>>>   5 - Run xrandr, notice it won't detect the new monitor
>>>   6 - Read SDEIIR and notice it's not 0 while DEIIR is 0
>>>
>>> Q: Can't we just clear DEIIR before SDEIIR?
>>> A: It doesn't work. SDEIIR has to be completely cleared (including the
>>> interrupts stored on its back queue) before it can flip DEIIR's bit to
>>> 1 again, and even while you're clearing it you'll be getting more and
>>> more interrupts.
>>>
>>> Q: Why does it work by just disabling+enabling the south interrupts?
>>> A: Because when we re-enable them, if there's something on the SDEIIR
>>> register (maybe an interrupt stored on the queue), the re-enabling
>>> will make DEIIR's bit flip to 1, and since we'll already have
>>> interrupts enabled we'll get another interrupt, then run our irq
>>> handler again to process the "back" interrupts.
>>>
>>> v2: Even bigger commit message, added code comments.
>>>
>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Since this seems to fix the dp aux irq timeout regression I've merged this
>> to -fixes. Also volunteered Imre for a review, I'll add that if it pops up
>> in the next few days.
>>
>> Big thanks to Paulo&Imre for tracking this down.
>
> Digging up zombies ... So reading through Oscar's execlist patches
> I've had a new idea for fixing our various missed irq issues. The new
> trick seems to work for ring interrupts, but I couldn't yet test the
> pch interrupts.
>
> Do you still remember on which platform we get tons of fifo underruns
> on the pch when doing modesets? I've tried hdmi on my ivb here for a
> start, but that didn't work out - no pch irq storm, not even a single
> underrun :(

I think I tested all my patches on both SNB and HSW, could reproduce
the problem on both. And I usually have 2 monitors connected on my
machines.

But if you want to get the interrupt storn, you have to revert all the
FIFO underrun detection code, revert the patch above, and just add
code that tries to enable/check/clear the underrun bits in case we get
it.

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



-- 
Paulo Zanoni

Re: [PATCH 1/4] drm/i915: also disable south interrupts when handling them

[Daniel Vetter]

On Fri, May 23, 2014 at 08:25:59AM -0300, Paulo Zanoni wrote:
> 2014-05-23 5:13 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Tue, Mar 5, 2013 at 8:08 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> On Fri, Feb 22, 2013 at 05:05:28PM -0300, Paulo Zanoni wrote:
> >>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>>
> >>> From the docs:
> >>>
> >>>   "IIR can queue up to two interrupt events. When the IIR is cleared,
> >>>   it will set itself again after one clock if a second event was
> >>>   stored."
> >>>
> >>>   "Only the rising edge of the PCH Display interrupt will cause the
> >>>   North Display IIR (DEIIR) PCH Display Interrupt even bit to be set,
> >>>   so all PCH Display Interrupts, including back to back interrupts,
> >>>   must be cleared before a new PCH Display interrupt can cause DEIIR
> >>>   to be set".
> >>>
> >>> The current code works fine because we don't get many interrupts, but
> >>> if we enable the PCH FIFO underrun interrupts we'll start getting so
> >>> many interrupts that at some point new PCH interrupts won't cause
> >>> DEIIR to be set.
> >>>
> >>> The initial implementation I tried was to turn the code that checks
> >>> SDEIIR into a loop, but we can still get interrupts even after the
> >>> loop is done (and before the irq handler finishes), so we have to
> >>> either disable the interrupts or mask them. In the end I concluded
> >>> that just disabling the PCH interrupts is enough, you don't even need
> >>> the loop, so this is what this patch implements. I've tested it and it
> >>> passes the 2 "PCH FIFO underrun interrupt storms" I can reproduce:
> >>> the "ironlake_crtc_disable" case and the "wrong watermarks" case.
> >>>
> >>> In other words, here's how to reproduce the problem fixed by this
> >>> patch:
> >>>   1 - Enable PCH FIFO underrun interrupts (SERR_INT on SNB+)
> >>>   2 - Boot the machine
> >>>   3 - While booting we'll get tons of PCH FIFO underrun interrupts
> >>>   4 - Plug a new monitor
> >>>   5 - Run xrandr, notice it won't detect the new monitor
> >>>   6 - Read SDEIIR and notice it's not 0 while DEIIR is 0
> >>>
> >>> Q: Can't we just clear DEIIR before SDEIIR?
> >>> A: It doesn't work. SDEIIR has to be completely cleared (including the
> >>> interrupts stored on its back queue) before it can flip DEIIR's bit to
> >>> 1 again, and even while you're clearing it you'll be getting more and
> >>> more interrupts.
> >>>
> >>> Q: Why does it work by just disabling+enabling the south interrupts?
> >>> A: Because when we re-enable them, if there's something on the SDEIIR
> >>> register (maybe an interrupt stored on the queue), the re-enabling
> >>> will make DEIIR's bit flip to 1, and since we'll already have
> >>> interrupts enabled we'll get another interrupt, then run our irq
> >>> handler again to process the "back" interrupts.
> >>>
> >>> v2: Even bigger commit message, added code comments.
> >>>
> >>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Since this seems to fix the dp aux irq timeout regression I've merged this
> >> to -fixes. Also volunteered Imre for a review, I'll add that if it pops up
> >> in the next few days.
> >>
> >> Big thanks to Paulo&Imre for tracking this down.
> >
> > Digging up zombies ... So reading through Oscar's execlist patches
> > I've had a new idea for fixing our various missed irq issues. The new
> > trick seems to work for ring interrupts, but I couldn't yet test the
> > pch interrupts.
> >
> > Do you still remember on which platform we get tons of fifo underruns
> > on the pch when doing modesets? I've tried hdmi on my ivb here for a
> > start, but that didn't work out - no pch irq storm, not even a single
> > underrun :(
> 
> I think I tested all my patches on both SNB and HSW, could reproduce
> the problem on both. And I usually have 2 monitors connected on my
> machines.
> 
> But if you want to get the interrupt storn, you have to revert all the
> FIFO underrun detection code, revert the patch above, and just add
> code that tries to enable/check/clear the underrun bits in case we get
> it.

Well I've hacked it to never disable fifo underruns and still didn't get
a storm somehow. But then I've decided that my patch was bogus anyway, so
I think I'll leave it at that. I've tried on hsw and ivb with multi-pipe
configs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch