diff mbox series

[03/18] drm/i915/display13: Enhanced pipe underrun reporting

Message ID 20210128192413.1715802-4-matthew.d.roper@intel.com (mailing list archive)
State New, archived
Headers show
Series Preliminary Display13 support | expand

Commit Message

Matt Roper Jan. 28, 2021, 7:23 p.m. UTC 
Display13 brings enhanced underrun recovery:  the hardware can somewhat
mitigate underruns by using an interpolated replacement pixel (soft
underrun) or the previous pixel (hard underrun).  Furthermore, underruns
can now be caused downstream by the port, even if the pipe itself is
operating properly.  The interrupt register gives us extra bits to
determine hard/soft underruns and whether the underrun was caused by the
port, so let's pass the iir down to the underrun handler and print some
more descriptive errors on Display13 platforms.

The context of the underrun is also available via PIPE_STATUS, but since
we have the same information in the IIR we don't have a need to read
from there.  PIPE_STATUS might be useful in debugfs in the future
though.

Bspec: 50335
Bspec: 50366
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 .../drm/i915/display/intel_fifo_underrun.c    | 55 ++++++++++++++++++-
 drivers/gpu/drm/i915/i915_irq.c               | 14 ++++-
 drivers/gpu/drm/i915/i915_reg.h               |  7 +++
 3 files changed, 73 insertions(+), 3 deletions(-)

Comments

Lucas De Marchi Feb. 11, 2021, 12:31 a.m. UTC | #1 
On Thu, Jan 28, 2021 at 11:23:58AM -0800, Matt Roper wrote:
>Display13 brings enhanced underrun recovery:  the hardware can somewhat
>mitigate underruns by using an interpolated replacement pixel (soft
>underrun) or the previous pixel (hard underrun).  Furthermore, underruns
>can now be caused downstream by the port, even if the pipe itself is
>operating properly.  The interrupt register gives us extra bits to
>determine hard/soft underruns and whether the underrun was caused by the
>port, so let's pass the iir down to the underrun handler and print some
>more descriptive errors on Display13 platforms.
>
>The context of the underrun is also available via PIPE_STATUS, but since
>we have the same information in the IIR we don't have a need to read
>from there.  PIPE_STATUS might be useful in debugfs in the future
>though.

is this comment outdated? See below...

>
>Bspec: 50335
>Bspec: 50366
>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>---
> .../drm/i915/display/intel_fifo_underrun.c    | 55 ++++++++++++++++++-
> drivers/gpu/drm/i915/i915_irq.c               | 14 ++++-
> drivers/gpu/drm/i915/i915_reg.h               |  7 +++
> 3 files changed, 73 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
>index 813a4f7033e1..6c377f0fc1b3 100644
>--- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
>+++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
>@@ -359,6 +359,39 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
> 	return old;
> }
>
>+static u32
>+underrun_pipestat_mask(struct drm_i915_private *dev_priv)
>+{
>+	u32 mask = PIPE_FIFO_UNDERRUN_STATUS;
>+
>+	if (HAS_DISPLAY13(dev_priv))
>+		mask |= PIPE_STAT_SOFT_UNDERRUN_D13 |
>+			PIPE_STAT_HARD_UNDERRUN_D13 |
>+			PIPE_STAT_PORT_UNDERRUN_D13;
>+
>+	return mask;
>+}
>+
>+static const char *
>+pipe_underrun_reason(u32 pipestat_underruns)
>+{
>+	if (pipestat_underruns & PIPE_STAT_SOFT_UNDERRUN_D13)
>+		/*
>+		 * Hardware used replacement/interpolated pixels at
>+		 * underrun locations.
>+		 */
>+		return "soft";
>+	else if (pipestat_underruns & PIPE_STAT_HARD_UNDERRUN_D13)
>+		/*
>+		 * Hardware used previous pixel value at underrun
>+		 * locations.
>+		 */
>+		return "hard";
>+	else
>+		/* Old platform or no extra soft/hard bit set */
>+		return "FIFO";
>+}
>+
> /**
>  * intel_cpu_fifo_underrun_irq_handler - handle CPU fifo underrun interrupt
>  * @dev_priv: i915 device instance
>@@ -372,6 +405,7 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> 					 enum pipe pipe)
> {
> 	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
>+	u32 underruns = 0;
>
> 	/* We may be called too early in init, thanks BIOS! */
> 	if (crtc == NULL)
>@@ -382,10 +416,27 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> 	    crtc->cpu_fifo_underrun_disabled)
> 		return;
>
>+	/*
>+	 * On Display13, we can find out whether an underrun is soft/hard from
>+	 * either the iir or PIPE_STAT, but we can only determine if underruns
>+	 * were due to downstream port logic from PIPE_STAT.
>+	 */

so... we are actually reading PIPE_STAT somce we want to report if it's
from downstream port.

>+	underruns = intel_uncore_read(&dev_priv->uncore, ICL_PIPESTAT(pipe)) &
>+		underrun_pipestat_mask(dev_priv);
>+	intel_uncore_write(&dev_priv->uncore, ICL_PIPESTAT(pipe), underruns);

maybe I'm missing something, but this doesn't look right to me.  We
unconditionally read/write ICL_PIPESTAT(pipe), even if it's not
display13.  Also, the `underruns = 0` initialization is just being
overwritten here.

intel_cpu_fifo_underrun_irq_handler() is called by very old gens as
well.

Lucas De Marchi

>+
> 	if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false)) {
> 		trace_intel_cpu_fifo_underrun(dev_priv, pipe);
>-		drm_err(&dev_priv->drm, "CPU pipe %c FIFO underrun\n",
>-			pipe_name(pipe));
>+
>+		if (underruns & PIPE_STAT_PORT_UNDERRUN_D13)
>+			/* Underrun was caused downstream from the pipes */
>+			drm_err(&dev_priv->drm, "Port triggered a %s underrun on pipe %c\n",
>+				pipe_underrun_reason(underruns),
>+				pipe_name(pipe));
>+		else
>+			drm_err(&dev_priv->drm, "CPU pipe %c %s underrun\n",
>+				pipe_name(pipe),
>+				pipe_underrun_reason(underruns));
> 	}
>
> 	intel_fbc_handle_fifo_underrun_irq(dev_priv);
>diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>index 1bced71470a5..407b42706a14 100644
>--- a/drivers/gpu/drm/i915/i915_irq.c
>+++ b/drivers/gpu/drm/i915/i915_irq.c
>@@ -2389,6 +2389,18 @@ static void gen11_dsi_te_interrupt_handler(struct drm_i915_private *dev_priv,
> 	intel_uncore_write(&dev_priv->uncore, DSI_INTR_IDENT_REG(port), tmp);
> }
>
>+static u32
>+underrun_iir_mask(struct drm_i915_private *dev_priv)
>+{
>+	u32 mask = GEN8_PIPE_FIFO_UNDERRUN;
>+
>+	if (HAS_DISPLAY13(dev_priv))
>+		mask |= D13_PIPE_SOFT_UNDERRUN |
>+			D13_PIPE_HARD_UNDERRUN;
>+
>+	return mask;
>+}
>+
> static irqreturn_t
> gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> {
>@@ -2497,7 +2509,7 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
> 		if (iir & GEN8_PIPE_CDCLK_CRC_DONE)
> 			hsw_pipe_crc_irq_handler(dev_priv, pipe);
>
>-		if (iir & GEN8_PIPE_FIFO_UNDERRUN)
>+		if (iir & underrun_iir_mask(dev_priv))
> 			intel_cpu_fifo_underrun_irq_handler(dev_priv, pipe);
>
> 		fault_errors = iir & gen8_de_pipe_fault_mask(dev_priv);
>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>index 10fd0e3af2d4..a57593f7d7b1 100644
>--- a/drivers/gpu/drm/i915/i915_reg.h
>+++ b/drivers/gpu/drm/i915/i915_reg.h
>@@ -6039,14 +6039,18 @@ enum {
> #define   PIPECONF_DITHER_TYPE_ST2 (2 << 2)
> #define   PIPECONF_DITHER_TYPE_TEMP (3 << 2)
> #define _PIPEASTAT		0x70024
>+#define _PIPEASTAT_ICL		0x70058
> #define   PIPE_FIFO_UNDERRUN_STATUS		(1UL << 31)
> #define   SPRITE1_FLIP_DONE_INT_EN_VLV		(1UL << 30)
> #define   PIPE_CRC_ERROR_ENABLE			(1UL << 29)
> #define   PIPE_CRC_DONE_ENABLE			(1UL << 28)
>+#define   PIPE_STAT_SOFT_UNDERRUN_D13		(1UL << 28)
> #define   PERF_COUNTER2_INTERRUPT_EN		(1UL << 27)
> #define   PIPE_GMBUS_EVENT_ENABLE		(1UL << 27)
>+#define   PIPE_STAT_HARD_UNDERRUN_D13		(1UL << 27)
> #define   PLANE_FLIP_DONE_INT_EN_VLV		(1UL << 26)
> #define   PIPE_HOTPLUG_INTERRUPT_ENABLE		(1UL << 26)
>+#define   PIPE_STAT_PORT_UNDERRUN_D13		(1UL << 26)
> #define   PIPE_VSYNC_INTERRUPT_ENABLE		(1UL << 25)
> #define   PIPE_DISPLAY_LINE_COMPARE_ENABLE	(1UL << 24)
> #define   PIPE_DPST_EVENT_ENABLE		(1UL << 23)
>@@ -6111,6 +6115,7 @@ enum {
> #define PIPEFRAME(pipe)		_MMIO_PIPE2(pipe, _PIPEAFRAMEHIGH)
> #define PIPEFRAMEPIXEL(pipe)	_MMIO_PIPE2(pipe, _PIPEAFRAMEPIXEL)
> #define PIPESTAT(pipe)		_MMIO_PIPE2(pipe, _PIPEASTAT)
>+#define ICL_PIPESTAT(pipe)	_MMIO_PIPE2(pipe, _PIPEASTAT_ICL)
>
> #define  _PIPEAGCMAX           0x70010
> #define  _PIPEBGCMAX           0x71010
>@@ -7789,6 +7794,8 @@ enum {
> #define  GEN8_PIPE_FIFO_UNDERRUN	(1 << 31)
> #define  GEN8_PIPE_CDCLK_CRC_ERROR	(1 << 29)
> #define  GEN8_PIPE_CDCLK_CRC_DONE	(1 << 28)
>+#define  D13_PIPE_SOFT_UNDERRUN		(1 << 22)
>+#define  D13_PIPE_HARD_UNDERRUN		(1 << 21)
> #define  GEN8_PIPE_CURSOR_FAULT		(1 << 10)
> #define  GEN8_PIPE_SPRITE_FAULT		(1 << 9)
> #define  GEN8_PIPE_PRIMARY_FAULT	(1 << 8)
>-- 
>2.25.4
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Feb. 11, 2021, 12:25 p.m. UTC | #2 
On Thu, Jan 28, 2021 at 11:23:58AM -0800, Matt Roper wrote:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 10fd0e3af2d4..a57593f7d7b1 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6039,14 +6039,18 @@ enum {
>  #define   PIPECONF_DITHER_TYPE_ST2 (2 << 2)
>  #define   PIPECONF_DITHER_TYPE_TEMP (3 << 2)
>  #define _PIPEASTAT		0x70024
> +#define _PIPEASTAT_ICL		0x70058

PIPESTAT is a gmch thing. This is not that for sure.

>  #define   PIPE_FIFO_UNDERRUN_STATUS		(1UL << 31)
>  #define   SPRITE1_FLIP_DONE_INT_EN_VLV		(1UL << 30)
>  #define   PIPE_CRC_ERROR_ENABLE			(1UL << 29)
>  #define   PIPE_CRC_DONE_ENABLE			(1UL << 28)
> +#define   PIPE_STAT_SOFT_UNDERRUN_D13		(1UL << 28)
>  #define   PERF_COUNTER2_INTERRUPT_EN		(1UL << 27)
>  #define   PIPE_GMBUS_EVENT_ENABLE		(1UL << 27)
> +#define   PIPE_STAT_HARD_UNDERRUN_D13		(1UL << 27)
>  #define   PLANE_FLIP_DONE_INT_EN_VLV		(1UL << 26)
>  #define   PIPE_HOTPLUG_INTERRUPT_ENABLE		(1UL << 26)
> +#define   PIPE_STAT_PORT_UNDERRUN_D13		(1UL << 26)
>  #define   PIPE_VSYNC_INTERRUPT_ENABLE		(1UL << 25)
>  #define   PIPE_DISPLAY_LINE_COMPARE_ENABLE	(1UL << 24)
>  #define   PIPE_DPST_EVENT_ENABLE		(1UL << 23)
> @@ -6111,6 +6115,7 @@ enum {
>  #define PIPEFRAME(pipe)		_MMIO_PIPE2(pipe, _PIPEAFRAMEHIGH)
>  #define PIPEFRAMEPIXEL(pipe)	_MMIO_PIPE2(pipe, _PIPEAFRAMEPIXEL)
>  #define PIPESTAT(pipe)		_MMIO_PIPE2(pipe, _PIPEASTAT)
> +#define ICL_PIPESTAT(pipe)	_MMIO_PIPE2(pipe, _PIPEASTAT_ICL)
>  
>  #define  _PIPEAGCMAX           0x70010
>  #define  _PIPEBGCMAX           0x71010
> @@ -7789,6 +7794,8 @@ enum {
>  #define  GEN8_PIPE_FIFO_UNDERRUN	(1 << 31)
>  #define  GEN8_PIPE_CDCLK_CRC_ERROR	(1 << 29)
>  #define  GEN8_PIPE_CDCLK_CRC_DONE	(1 << 28)
> +#define  D13_PIPE_SOFT_UNDERRUN		(1 << 22)
> +#define  D13_PIPE_HARD_UNDERRUN		(1 << 21)
>  #define  GEN8_PIPE_CURSOR_FAULT		(1 << 10)
>  #define  GEN8_PIPE_SPRITE_FAULT		(1 << 9)
>  #define  GEN8_PIPE_PRIMARY_FAULT	(1 << 8)
> -- 
> 2.25.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
index 813a4f7033e1..6c377f0fc1b3 100644
--- a/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
+++ b/drivers/gpu/drm/i915/display/intel_fifo_underrun.c
@@ -359,6 +359,39 @@  bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
 	return old;
 }
 
+static u32
+underrun_pipestat_mask(struct drm_i915_private *dev_priv)
+{
+	u32 mask = PIPE_FIFO_UNDERRUN_STATUS;
+
+	if (HAS_DISPLAY13(dev_priv))
+		mask |= PIPE_STAT_SOFT_UNDERRUN_D13 |
+			PIPE_STAT_HARD_UNDERRUN_D13 |
+			PIPE_STAT_PORT_UNDERRUN_D13;
+
+	return mask;
+}
+
+static const char *
+pipe_underrun_reason(u32 pipestat_underruns)
+{
+	if (pipestat_underruns & PIPE_STAT_SOFT_UNDERRUN_D13)
+		/*
+		 * Hardware used replacement/interpolated pixels at
+		 * underrun locations.
+		 */
+		return "soft";
+	else if (pipestat_underruns & PIPE_STAT_HARD_UNDERRUN_D13)
+		/*
+		 * Hardware used previous pixel value at underrun
+		 * locations.
+		 */
+		return "hard";
+	else
+		/* Old platform or no extra soft/hard bit set */
+		return "FIFO";
+}
+
 /**
  * intel_cpu_fifo_underrun_irq_handler - handle CPU fifo underrun interrupt
  * @dev_priv: i915 device instance
@@ -372,6 +405,7 @@  void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 					 enum pipe pipe)
 {
 	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
+	u32 underruns = 0;
 
 	/* We may be called too early in init, thanks BIOS! */
 	if (crtc == NULL)
@@ -382,10 +416,27 @@  void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
 	    crtc->cpu_fifo_underrun_disabled)
 		return;
 
+	/*
+	 * On Display13, we can find out whether an underrun is soft/hard from
+	 * either the iir or PIPE_STAT, but we can only determine if underruns
+	 * were due to downstream port logic from PIPE_STAT.
+	 */
+	underruns = intel_uncore_read(&dev_priv->uncore, ICL_PIPESTAT(pipe)) &
+		underrun_pipestat_mask(dev_priv);
+	intel_uncore_write(&dev_priv->uncore, ICL_PIPESTAT(pipe), underruns);
+
 	if (intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false)) {
 		trace_intel_cpu_fifo_underrun(dev_priv, pipe);
-		drm_err(&dev_priv->drm, "CPU pipe %c FIFO underrun\n",
-			pipe_name(pipe));
+
+		if (underruns & PIPE_STAT_PORT_UNDERRUN_D13)
+			/* Underrun was caused downstream from the pipes */
+			drm_err(&dev_priv->drm, "Port triggered a %s underrun on pipe %c\n",
+				pipe_underrun_reason(underruns),
+				pipe_name(pipe));
+		else
+			drm_err(&dev_priv->drm, "CPU pipe %c %s underrun\n",
+				pipe_name(pipe),
+				pipe_underrun_reason(underruns));
 	}
 
 	intel_fbc_handle_fifo_underrun_irq(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1bced71470a5..407b42706a14 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2389,6 +2389,18 @@  static void gen11_dsi_te_interrupt_handler(struct drm_i915_private *dev_priv,
 	intel_uncore_write(&dev_priv->uncore, DSI_INTR_IDENT_REG(port), tmp);
 }
 
+static u32
+underrun_iir_mask(struct drm_i915_private *dev_priv)
+{
+	u32 mask = GEN8_PIPE_FIFO_UNDERRUN;
+
+	if (HAS_DISPLAY13(dev_priv))
+		mask |= D13_PIPE_SOFT_UNDERRUN |
+			D13_PIPE_HARD_UNDERRUN;
+
+	return mask;
+}
+
 static irqreturn_t
 gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 {
@@ -2497,7 +2509,7 @@  gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 		if (iir & GEN8_PIPE_CDCLK_CRC_DONE)
 			hsw_pipe_crc_irq_handler(dev_priv, pipe);
 
-		if (iir & GEN8_PIPE_FIFO_UNDERRUN)
+		if (iir & underrun_iir_mask(dev_priv))
 			intel_cpu_fifo_underrun_irq_handler(dev_priv, pipe);
 
 		fault_errors = iir & gen8_de_pipe_fault_mask(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 10fd0e3af2d4..a57593f7d7b1 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6039,14 +6039,18 @@  enum {
 #define   PIPECONF_DITHER_TYPE_ST2 (2 << 2)
 #define   PIPECONF_DITHER_TYPE_TEMP (3 << 2)
 #define _PIPEASTAT		0x70024
+#define _PIPEASTAT_ICL		0x70058
 #define   PIPE_FIFO_UNDERRUN_STATUS		(1UL << 31)
 #define   SPRITE1_FLIP_DONE_INT_EN_VLV		(1UL << 30)
 #define   PIPE_CRC_ERROR_ENABLE			(1UL << 29)
 #define   PIPE_CRC_DONE_ENABLE			(1UL << 28)
+#define   PIPE_STAT_SOFT_UNDERRUN_D13		(1UL << 28)
 #define   PERF_COUNTER2_INTERRUPT_EN		(1UL << 27)
 #define   PIPE_GMBUS_EVENT_ENABLE		(1UL << 27)
+#define   PIPE_STAT_HARD_UNDERRUN_D13		(1UL << 27)
 #define   PLANE_FLIP_DONE_INT_EN_VLV		(1UL << 26)
 #define   PIPE_HOTPLUG_INTERRUPT_ENABLE		(1UL << 26)
+#define   PIPE_STAT_PORT_UNDERRUN_D13		(1UL << 26)
 #define   PIPE_VSYNC_INTERRUPT_ENABLE		(1UL << 25)
 #define   PIPE_DISPLAY_LINE_COMPARE_ENABLE	(1UL << 24)
 #define   PIPE_DPST_EVENT_ENABLE		(1UL << 23)
@@ -6111,6 +6115,7 @@  enum {
 #define PIPEFRAME(pipe)		_MMIO_PIPE2(pipe, _PIPEAFRAMEHIGH)
 #define PIPEFRAMEPIXEL(pipe)	_MMIO_PIPE2(pipe, _PIPEAFRAMEPIXEL)
 #define PIPESTAT(pipe)		_MMIO_PIPE2(pipe, _PIPEASTAT)
+#define ICL_PIPESTAT(pipe)	_MMIO_PIPE2(pipe, _PIPEASTAT_ICL)
 
 #define  _PIPEAGCMAX           0x70010
 #define  _PIPEBGCMAX           0x71010
@@ -7789,6 +7794,8 @@  enum {
 #define  GEN8_PIPE_FIFO_UNDERRUN	(1 << 31)
 #define  GEN8_PIPE_CDCLK_CRC_ERROR	(1 << 29)
 #define  GEN8_PIPE_CDCLK_CRC_DONE	(1 << 28)
+#define  D13_PIPE_SOFT_UNDERRUN		(1 << 22)
+#define  D13_PIPE_HARD_UNDERRUN		(1 << 21)
 #define  GEN8_PIPE_CURSOR_FAULT		(1 << 10)
 #define  GEN8_PIPE_SPRITE_FAULT		(1 << 9)
 #define  GEN8_PIPE_PRIMARY_FAULT	(1 << 8)