[v3,2/4] drm/vc4: Report underrun errors

Submitted by Paul Kocialkowski on Jan. 8, 2019, 2:50 p.m.

Details

Message ID 20190108145056.2276-3-paul.kocialkowski@bootlin.com
State New
Headers show
Series "drm/vc4: Add a load tracker" ( rev: 2 ) in DRI devel

Not browsing as part of any series.

Commit Message

Paul Kocialkowski Jan. 8, 2019, 2:50 p.m.
From: Boris Brezillon <boris.brezillon@bootlin.com>

The DRM framework provides a generic way to report underrun errors.
Let's implement the necessary hooks to support it in the VC4 driver.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v3:
- Generic underrun report function has been dropped, adjust the
  code accordingly

Changes in v2:
- New patch
---
 drivers/gpu/drm/vc4/vc4_debugfs.c |  1 +
 drivers/gpu/drm/vc4/vc4_drv.h     | 10 ++++
 drivers/gpu/drm/vc4/vc4_hvs.c     | 87 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_kms.c     |  4 ++
 drivers/gpu/drm/vc4/vc4_regs.h    | 46 ++++------------
 5 files changed, 113 insertions(+), 35 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
index 7a0003de71ab..64021bcba041 100644
--- a/drivers/gpu/drm/vc4/vc4_debugfs.c
+++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
@@ -23,6 +23,7 @@  static const struct drm_info_list vc4_debugfs_list[] = {
 	{"vec_regs", vc4_vec_debugfs_regs, 0},
 	{"txp_regs", vc4_txp_debugfs_regs, 0},
 	{"hvs_regs", vc4_hvs_debugfs_regs, 0},
+	{"hvs_underrun",  vc4_hvs_debugfs_underrun, 0},
 	{"crtc0_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)0},
 	{"crtc1_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)1},
 	{"crtc2_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)2},
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 955f157f5ad0..619fb8bec428 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -184,6 +184,13 @@  struct vc4_dev {
 	/* Bitmask of the current bin_alloc used for overflow memory. */
 	uint32_t bin_alloc_overflow;
 
+	/* Incremented when an underrun error happened after an atomic commit.
+	 * This is particularly useful to detect when a specific modeset is too
+	 * demanding in term of memory or HVS bandwidth which is hard to guess
+	 * at atomic check time.
+	 */
+	atomic_t underrun;
+
 	struct work_struct overflow_mem_work;
 
 	int power_refcount;
@@ -773,6 +780,9 @@  extern struct platform_driver vc4_hvs_driver;
 void vc4_hvs_dump_state(struct drm_device *dev);
 int vc4_hvs_debugfs_regs(struct seq_file *m, void *unused);
 void vc4_hvs_sync_dlist(struct drm_device *dev);
+int vc4_hvs_debugfs_underrun(struct seq_file *m, void *unused);
+void vc4_hvs_unmask_underrun(struct drm_device *dev);
+void vc4_hvs_mask_underrun(struct drm_device *dev);
 
 /* vc4_kms.c */
 int vc4_kms_load(struct drm_device *dev);
diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
index 1ba60b8e0c2d..3095fad2ff8b 100644
--- a/drivers/gpu/drm/vc4/vc4_hvs.c
+++ b/drivers/gpu/drm/vc4/vc4_hvs.c
@@ -22,6 +22,7 @@ 
  * each CRTC.
  */
 
+#include <drm/drm_atomic_helper.h>
 #include <linux/component.h>
 #include "vc4_drv.h"
 #include "vc4_regs.h"
@@ -102,6 +103,18 @@  int vc4_hvs_debugfs_regs(struct seq_file *m, void *unused)
 
 	return 0;
 }
+
+int vc4_hvs_debugfs_underrun(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	struct drm_printer p = drm_seq_file_printer(m);
+
+	drm_printf(&p, "%d\n", atomic_read(&vc4->underrun));
+
+	return 0;
+}
 #endif
 
 /* The filter kernel is composed of dwords each containing 3 9-bit
@@ -183,6 +196,62 @@  void vc4_hvs_sync_dlist(struct drm_device *dev)
 	}
 }
 
+void vc4_hvs_mask_underrun(struct drm_device *dev)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
+
+	dispctrl &= ~(SCALER_DISPCTRL_DSPEISLUR(0) |
+		      SCALER_DISPCTRL_DSPEISLUR(1) |
+		      SCALER_DISPCTRL_DSPEISLUR(2));
+
+	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
+}
+
+void vc4_hvs_unmask_underrun(struct drm_device *dev)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
+
+	dispctrl |= SCALER_DISPCTRL_DSPEISLUR(0) |
+		    SCALER_DISPCTRL_DSPEISLUR(1) |
+		    SCALER_DISPCTRL_DSPEISLUR(2);
+
+	HVS_WRITE(SCALER_DISPSTAT,
+		  SCALER_DISPSTAT_EUFLOW(0) |
+		  SCALER_DISPSTAT_EUFLOW(1) |
+		  SCALER_DISPSTAT_EUFLOW(2));
+	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
+}
+
+static void vc4_hvs_report_underrun(struct drm_device *dev)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+
+	atomic_inc(&vc4->underrun);
+	DRM_DEV_ERROR(dev->dev, "HVS underrun\n");
+}
+
+static irqreturn_t vc4_hvs_irq_handler(int irq, void *data)
+{
+	struct drm_device *dev = data;
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	u32 status;
+
+	status = HVS_READ(SCALER_DISPSTAT);
+
+	if (status &
+	    (SCALER_DISPSTAT_EUFLOW(0) | SCALER_DISPSTAT_EUFLOW(1) |
+	     SCALER_DISPSTAT_EUFLOW(2))) {
+		vc4_hvs_mask_underrun(dev);
+		vc4_hvs_report_underrun(dev);
+	}
+
+	HVS_WRITE(SCALER_DISPSTAT, status);
+
+	return status ? IRQ_HANDLED : IRQ_NONE;
+}
+
 static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -236,15 +305,33 @@  static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
 	dispctrl = HVS_READ(SCALER_DISPCTRL);
 
 	dispctrl |= SCALER_DISPCTRL_ENABLE;
+	dispctrl |= SCALER_DISPCTRL_DSPEISLUR(0) | SCALER_DISPCTRL_DISPEIRQ(0) |
+		    SCALER_DISPCTRL_DSPEISLUR(1) | SCALER_DISPCTRL_DISPEIRQ(1) |
+		    SCALER_DISPCTRL_DSPEISLUR(2) | SCALER_DISPCTRL_DISPEIRQ(2);
 
 	/* Set DSP3 (PV1) to use HVS channel 2, which would otherwise
 	 * be unused.
 	 */
 	dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK;
+	dispctrl &= ~(SCALER_DISPCTRL_DMAEIRQ |
+		      SCALER_DISPCTRL_SLVWREIRQ |
+		      SCALER_DISPCTRL_SLVRDEIRQ |
+		      SCALER_DISPCTRL_DSPEIEOF(0) |
+		      SCALER_DISPCTRL_DSPEIEOF(1) |
+		      SCALER_DISPCTRL_DSPEIEOF(2) |
+		      SCALER_DISPCTRL_DSPEIEOLN(0) |
+		      SCALER_DISPCTRL_DSPEIEOLN(1) |
+		      SCALER_DISPCTRL_DSPEIEOLN(2) |
+		      SCALER_DISPCTRL_SCLEIRQ);
 	dispctrl |= VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);
 
 	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
 
+	ret = devm_request_irq(dev, platform_get_irq(pdev, 0),
+			       vc4_hvs_irq_handler, 0, "vc4 hvs", drm);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 2d66a2b57a91..a28e801aeae2 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -139,6 +139,8 @@  vc4_atomic_complete_commit(struct drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
 
+	vc4_hvs_mask_underrun(dev);
+
 	drm_atomic_helper_wait_for_fences(dev, state, false);
 
 	drm_atomic_helper_wait_for_dependencies(state);
@@ -157,6 +159,8 @@  vc4_atomic_complete_commit(struct drm_atomic_state *state)
 
 	vc4_hvs_sync_dlist(dev);
 
+	vc4_hvs_unmask_underrun(dev);
+
 	drm_atomic_helper_wait_for_flip_done(dev, state);
 
 	drm_atomic_helper_cleanup_planes(dev, state);
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
index 50c653309aec..7e2692e9a83e 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -217,8 +217,6 @@ 
 #define SCALER_DISPCTRL                         0x00000000
 /* Global register for clock gating the HVS */
 # define SCALER_DISPCTRL_ENABLE			BIT(31)
-# define SCALER_DISPCTRL_DSP2EISLUR		BIT(15)
-# define SCALER_DISPCTRL_DSP1EISLUR		BIT(14)
 # define SCALER_DISPCTRL_DSP3_MUX_MASK		VC4_MASK(19, 18)
 # define SCALER_DISPCTRL_DSP3_MUX_SHIFT		18
 
@@ -226,45 +224,25 @@ 
  * SCALER_DISPSTAT_IRQDISP0.  Note that short frame contributions are
  * always enabled.
  */
-# define SCALER_DISPCTRL_DSP0EISLUR		BIT(13)
-# define SCALER_DISPCTRL_DSP2EIEOLN		BIT(12)
-# define SCALER_DISPCTRL_DSP2EIEOF		BIT(11)
-# define SCALER_DISPCTRL_DSP1EIEOLN		BIT(10)
-# define SCALER_DISPCTRL_DSP1EIEOF		BIT(9)
+# define SCALER_DISPCTRL_DSPEISLUR(x)		BIT(13 + (x))
 /* Enables Display 0 end-of-line-N contribution to
  * SCALER_DISPSTAT_IRQDISP0
  */
-# define SCALER_DISPCTRL_DSP0EIEOLN		BIT(8)
+# define SCALER_DISPCTRL_DSPEIEOLN(x)		BIT(8 + ((x) * 2))
 /* Enables Display 0 EOF contribution to SCALER_DISPSTAT_IRQDISP0 */
-# define SCALER_DISPCTRL_DSP0EIEOF		BIT(7)
+# define SCALER_DISPCTRL_DSPEIEOF(x)		BIT(7 + ((x) * 2))
 
 # define SCALER_DISPCTRL_SLVRDEIRQ		BIT(6)
 # define SCALER_DISPCTRL_SLVWREIRQ		BIT(5)
 # define SCALER_DISPCTRL_DMAEIRQ		BIT(4)
-# define SCALER_DISPCTRL_DISP2EIRQ		BIT(3)
-# define SCALER_DISPCTRL_DISP1EIRQ		BIT(2)
 /* Enables interrupt generation on the enabled EOF/EOLN/EISLUR
  * bits and short frames..
  */
-# define SCALER_DISPCTRL_DISP0EIRQ		BIT(1)
+# define SCALER_DISPCTRL_DISPEIRQ(x)		BIT(1 + (x))
 /* Enables interrupt generation on scaler profiler interrupt. */
 # define SCALER_DISPCTRL_SCLEIRQ		BIT(0)
 
 #define SCALER_DISPSTAT                         0x00000004
-# define SCALER_DISPSTAT_COBLOW2		BIT(29)
-# define SCALER_DISPSTAT_EOLN2			BIT(28)
-# define SCALER_DISPSTAT_ESFRAME2		BIT(27)
-# define SCALER_DISPSTAT_ESLINE2		BIT(26)
-# define SCALER_DISPSTAT_EUFLOW2		BIT(25)
-# define SCALER_DISPSTAT_EOF2			BIT(24)
-
-# define SCALER_DISPSTAT_COBLOW1		BIT(21)
-# define SCALER_DISPSTAT_EOLN1			BIT(20)
-# define SCALER_DISPSTAT_ESFRAME1		BIT(19)
-# define SCALER_DISPSTAT_ESLINE1		BIT(18)
-# define SCALER_DISPSTAT_EUFLOW1		BIT(17)
-# define SCALER_DISPSTAT_EOF1			BIT(16)
-
 # define SCALER_DISPSTAT_RESP_MASK		VC4_MASK(15, 14)
 # define SCALER_DISPSTAT_RESP_SHIFT		14
 # define SCALER_DISPSTAT_RESP_OKAY		0
@@ -272,23 +250,23 @@ 
 # define SCALER_DISPSTAT_RESP_SLVERR		2
 # define SCALER_DISPSTAT_RESP_DECERR		3
 
-# define SCALER_DISPSTAT_COBLOW0		BIT(13)
+# define SCALER_DISPSTAT_COBLOW(x)		BIT(5 + (((x) + 1) * 8))
 /* Set when the DISPEOLN line is done compositing. */
-# define SCALER_DISPSTAT_EOLN0			BIT(12)
+# define SCALER_DISPSTAT_EOLN(x)		BIT(4 + (((x) + 1) * 8))
 /* Set when VSTART is seen but there are still pixels in the current
  * output line.
  */
-# define SCALER_DISPSTAT_ESFRAME0		BIT(11)
+# define SCALER_DISPSTAT_ESFRAME(x)		BIT(3 + (((x) + 1) * 8))
 /* Set when HSTART is seen but there are still pixels in the current
  * output line.
  */
-# define SCALER_DISPSTAT_ESLINE0		BIT(10)
+# define SCALER_DISPSTAT_ESLINE(x)		BIT(2 + (((x) + 1) * 8))
 /* Set when the the downstream tries to read from the display FIFO
  * while it's empty.
  */
-# define SCALER_DISPSTAT_EUFLOW0		BIT(9)
+# define SCALER_DISPSTAT_EUFLOW(x)		BIT(1 + (((x) + 1) * 8))
 /* Set when the display mode changes from RUN to EOF */
-# define SCALER_DISPSTAT_EOF0			BIT(8)
+# define SCALER_DISPSTAT_EOF(x)			BIT(((x) + 1) * 8)
 
 /* Set on AXI invalid DMA ID error. */
 # define SCALER_DISPSTAT_DMA_ERROR		BIT(7)
@@ -300,12 +278,10 @@ 
  * SCALER_DISPSTAT_RESP_ERROR is not SCALER_DISPSTAT_RESP_OKAY.
  */
 # define SCALER_DISPSTAT_IRQDMA			BIT(4)
-# define SCALER_DISPSTAT_IRQDISP2		BIT(3)
-# define SCALER_DISPSTAT_IRQDISP1		BIT(2)
 /* Set when any of the EOF/EOLN/ESFRAME/ESLINE bits are set and their
  * corresponding interrupt bit is enabled in DISPCTRL.
  */
-# define SCALER_DISPSTAT_IRQDISP0		BIT(1)
+# define SCALER_DISPSTAT_IRQDISP(x)		BIT(1 + (x))
 /* On read, the profiler interrupt.  On write, clear *all* interrupt bits. */
 # define SCALER_DISPSTAT_IRQSCL			BIT(0)
 

Comments

Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:

> From: Boris Brezillon <boris.brezillon@bootlin.com>
>
> The DRM framework provides a generic way to report underrun errors.
> Let's implement the necessary hooks to support it in the VC4 driver.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> Changes in v3:
> - Generic underrun report function has been dropped, adjust the
>   code accordingly

Update commit message for DRM framework not providing a generic way any
more?

>
> Changes in v2:
> - New patch
> ---
>  drivers/gpu/drm/vc4/vc4_debugfs.c |  1 +
>  drivers/gpu/drm/vc4/vc4_drv.h     | 10 ++++
>  drivers/gpu/drm/vc4/vc4_hvs.c     | 87 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/vc4/vc4_kms.c     |  4 ++
>  drivers/gpu/drm/vc4/vc4_regs.h    | 46 ++++------------
>  5 files changed, 113 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
> index 7a0003de71ab..64021bcba041 100644
> --- a/drivers/gpu/drm/vc4/vc4_debugfs.c
> +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
> @@ -23,6 +23,7 @@ static const struct drm_info_list vc4_debugfs_list[] = {
>  	{"vec_regs", vc4_vec_debugfs_regs, 0},
>  	{"txp_regs", vc4_txp_debugfs_regs, 0},
>  	{"hvs_regs", vc4_hvs_debugfs_regs, 0},
> +	{"hvs_underrun",  vc4_hvs_debugfs_underrun, 0},
>  	{"crtc0_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)0},
>  	{"crtc1_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)1},
>  	{"crtc2_regs", vc4_crtc_debugfs_regs, 0, (void *)(uintptr_t)2},
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 955f157f5ad0..619fb8bec428 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -184,6 +184,13 @@ struct vc4_dev {
>  	/* Bitmask of the current bin_alloc used for overflow memory. */
>  	uint32_t bin_alloc_overflow;
>  
> +	/* Incremented when an underrun error happened after an atomic commit.
> +	 * This is particularly useful to detect when a specific modeset is too
> +	 * demanding in term of memory or HVS bandwidth which is hard to guess
> +	 * at atomic check time.
> +	 */
> +	atomic_t underrun;
> +
>  	struct work_struct overflow_mem_work;
>  
>  	int power_refcount;
> @@ -773,6 +780,9 @@ extern struct platform_driver vc4_hvs_driver;
>  void vc4_hvs_dump_state(struct drm_device *dev);
>  int vc4_hvs_debugfs_regs(struct seq_file *m, void *unused);
>  void vc4_hvs_sync_dlist(struct drm_device *dev);
> +int vc4_hvs_debugfs_underrun(struct seq_file *m, void *unused);
> +void vc4_hvs_unmask_underrun(struct drm_device *dev);
> +void vc4_hvs_mask_underrun(struct drm_device *dev);
>  
>  /* vc4_kms.c */
>  int vc4_kms_load(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index 1ba60b8e0c2d..3095fad2ff8b 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -22,6 +22,7 @@
>   * each CRTC.
>   */
>  
> +#include <drm/drm_atomic_helper.h>
>  #include <linux/component.h>
>  #include "vc4_drv.h"
>  #include "vc4_regs.h"
> @@ -102,6 +103,18 @@ int vc4_hvs_debugfs_regs(struct seq_file *m, void *unused)
>  
>  	return 0;
>  }
> +
> +int vc4_hvs_debugfs_underrun(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	struct drm_printer p = drm_seq_file_printer(m);
> +
> +	drm_printf(&p, "%d\n", atomic_read(&vc4->underrun));
> +
> +	return 0;
> +}
>  #endif
>  
>  /* The filter kernel is composed of dwords each containing 3 9-bit
> @@ -183,6 +196,62 @@ void vc4_hvs_sync_dlist(struct drm_device *dev)
>  	}
>  }
>  
> +void vc4_hvs_mask_underrun(struct drm_device *dev)
> +{
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
> +
> +	dispctrl &= ~(SCALER_DISPCTRL_DSPEISLUR(0) |
> +		      SCALER_DISPCTRL_DSPEISLUR(1) |
> +		      SCALER_DISPCTRL_DSPEISLUR(2));
> +
> +	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
> +}
> +
> +void vc4_hvs_unmask_underrun(struct drm_device *dev)
> +{
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
> +
> +	dispctrl |= SCALER_DISPCTRL_DSPEISLUR(0) |
> +		    SCALER_DISPCTRL_DSPEISLUR(1) |
> +		    SCALER_DISPCTRL_DSPEISLUR(2);
> +
> +	HVS_WRITE(SCALER_DISPSTAT,
> +		  SCALER_DISPSTAT_EUFLOW(0) |
> +		  SCALER_DISPSTAT_EUFLOW(1) |
> +		  SCALER_DISPSTAT_EUFLOW(2));
> +	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
> +}
> +
> +static void vc4_hvs_report_underrun(struct drm_device *dev)
> +{
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +
> +	atomic_inc(&vc4->underrun);
> +	DRM_DEV_ERROR(dev->dev, "HVS underrun\n");
> +}
> +
> +static irqreturn_t vc4_hvs_irq_handler(int irq, void *data)
> +{
> +	struct drm_device *dev = data;
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	u32 status;
> +
> +	status = HVS_READ(SCALER_DISPSTAT);
> +
> +	if (status &
> +	    (SCALER_DISPSTAT_EUFLOW(0) | SCALER_DISPSTAT_EUFLOW(1) |
> +	     SCALER_DISPSTAT_EUFLOW(2))) {
> +		vc4_hvs_mask_underrun(dev);
> +		vc4_hvs_report_underrun(dev);
> +	}
> +
> +	HVS_WRITE(SCALER_DISPSTAT, status);
> +
> +	return status ? IRQ_HANDLED : IRQ_NONE;
> +}

So, if UFLOW is set then we incremented the counter and disabled the
interrupt, otherwise we acked this specific interrupt and return?  Given
that a short-line error (the other potential cause of EISLUR) would be
likely to persist, we should probably either vc4_hvs_mask_underrun() in
that case too, or only return IRQ_HANDLED for the case we actually
handled.

> +
>  static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -236,15 +305,33 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>  	dispctrl = HVS_READ(SCALER_DISPCTRL);
>  
>  	dispctrl |= SCALER_DISPCTRL_ENABLE;
> +	dispctrl |= SCALER_DISPCTRL_DSPEISLUR(0) | SCALER_DISPCTRL_DISPEIRQ(0) |
> +		    SCALER_DISPCTRL_DSPEISLUR(1) | SCALER_DISPCTRL_DISPEIRQ(1) |
> +		    SCALER_DISPCTRL_DSPEISLUR(2) | SCALER_DISPCTRL_DISPEIRQ(2);
>  
>  	/* Set DSP3 (PV1) to use HVS channel 2, which would otherwise
>  	 * be unused.
>  	 */
>  	dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK;
> +	dispctrl &= ~(SCALER_DISPCTRL_DMAEIRQ |
> +		      SCALER_DISPCTRL_SLVWREIRQ |
> +		      SCALER_DISPCTRL_SLVRDEIRQ |
> +		      SCALER_DISPCTRL_DSPEIEOF(0) |
> +		      SCALER_DISPCTRL_DSPEIEOF(1) |
> +		      SCALER_DISPCTRL_DSPEIEOF(2) |
> +		      SCALER_DISPCTRL_DSPEIEOLN(0) |
> +		      SCALER_DISPCTRL_DSPEIEOLN(1) |
> +		      SCALER_DISPCTRL_DSPEIEOLN(2) |
> +		      SCALER_DISPCTRL_SCLEIRQ);
>  	dispctrl |= VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);

future work: At some point, we should stop inheriting old dispctrl setup
and just initialize it on our own (so we get priority flags even if the
firmware didn't set them up for us)

> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 2d66a2b57a91..a28e801aeae2 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -139,6 +139,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>  	struct drm_device *dev = state->dev;
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
>  
> +	vc4_hvs_mask_underrun(dev);
> +
>  	drm_atomic_helper_wait_for_fences(dev, state, false);
>  
>  	drm_atomic_helper_wait_for_dependencies(state);
> @@ -157,6 +159,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>  
>  	vc4_hvs_sync_dlist(dev);
>  
> +	vc4_hvs_unmask_underrun(dev);
> +
>  	drm_atomic_helper_wait_for_flip_done(dev, state);
>  
>  	drm_atomic_helper_cleanup_planes(dev, state);
> diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
> index 50c653309aec..7e2692e9a83e 100644
> --- a/drivers/gpu/drm/vc4/vc4_regs.h
> +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> @@ -217,8 +217,6 @@
>  #define SCALER_DISPCTRL                         0x00000000
>  /* Global register for clock gating the HVS */
>  # define SCALER_DISPCTRL_ENABLE			BIT(31)
> -# define SCALER_DISPCTRL_DSP2EISLUR		BIT(15)
> -# define SCALER_DISPCTRL_DSP1EISLUR		BIT(14)
>  # define SCALER_DISPCTRL_DSP3_MUX_MASK		VC4_MASK(19, 18)
>  # define SCALER_DISPCTRL_DSP3_MUX_SHIFT		18
>  
> @@ -226,45 +224,25 @@
>   * SCALER_DISPSTAT_IRQDISP0.  Note that short frame contributions are
>   * always enabled.
>   */
> -# define SCALER_DISPCTRL_DSP0EISLUR		BIT(13)
> -# define SCALER_DISPCTRL_DSP2EIEOLN		BIT(12)
> -# define SCALER_DISPCTRL_DSP2EIEOF		BIT(11)
> -# define SCALER_DISPCTRL_DSP1EIEOLN		BIT(10)
> -# define SCALER_DISPCTRL_DSP1EIEOF		BIT(9)
> +# define SCALER_DISPCTRL_DSPEISLUR(x)		BIT(13 + (x))
>  /* Enables Display 0 end-of-line-N contribution to
>   * SCALER_DISPSTAT_IRQDISP0
>   */
> -# define SCALER_DISPCTRL_DSP0EIEOLN		BIT(8)
> +# define SCALER_DISPCTRL_DSPEIEOLN(x)		BIT(8 + ((x) * 2))
>  /* Enables Display 0 EOF contribution to SCALER_DISPSTAT_IRQDISP0 */
> -# define SCALER_DISPCTRL_DSP0EIEOF		BIT(7)
> +# define SCALER_DISPCTRL_DSPEIEOF(x)		BIT(7 + ((x) * 2))
>  
>  # define SCALER_DISPCTRL_SLVRDEIRQ		BIT(6)
>  # define SCALER_DISPCTRL_SLVWREIRQ		BIT(5)
>  # define SCALER_DISPCTRL_DMAEIRQ		BIT(4)
> -# define SCALER_DISPCTRL_DISP2EIRQ		BIT(3)
> -# define SCALER_DISPCTRL_DISP1EIRQ		BIT(2)
>  /* Enables interrupt generation on the enabled EOF/EOLN/EISLUR
>   * bits and short frames..
>   */
> -# define SCALER_DISPCTRL_DISP0EIRQ		BIT(1)
> +# define SCALER_DISPCTRL_DISPEIRQ(x)		BIT(1 + (x))
>  /* Enables interrupt generation on scaler profiler interrupt. */
>  # define SCALER_DISPCTRL_SCLEIRQ		BIT(0)
>  
>  #define SCALER_DISPSTAT                         0x00000004
> -# define SCALER_DISPSTAT_COBLOW2		BIT(29)
> -# define SCALER_DISPSTAT_EOLN2			BIT(28)
> -# define SCALER_DISPSTAT_ESFRAME2		BIT(27)
> -# define SCALER_DISPSTAT_ESLINE2		BIT(26)
> -# define SCALER_DISPSTAT_EUFLOW2		BIT(25)
> -# define SCALER_DISPSTAT_EOF2			BIT(24)
> -
> -# define SCALER_DISPSTAT_COBLOW1		BIT(21)
> -# define SCALER_DISPSTAT_EOLN1			BIT(20)
> -# define SCALER_DISPSTAT_ESFRAME1		BIT(19)
> -# define SCALER_DISPSTAT_ESLINE1		BIT(18)
> -# define SCALER_DISPSTAT_EUFLOW1		BIT(17)
> -# define SCALER_DISPSTAT_EOF1			BIT(16)
> -
>  # define SCALER_DISPSTAT_RESP_MASK		VC4_MASK(15, 14)
>  # define SCALER_DISPSTAT_RESP_SHIFT		14
>  # define SCALER_DISPSTAT_RESP_OKAY		0
> @@ -272,23 +250,23 @@
>  # define SCALER_DISPSTAT_RESP_SLVERR		2
>  # define SCALER_DISPSTAT_RESP_DECERR		3
>  
> -# define SCALER_DISPSTAT_COBLOW0		BIT(13)
> +# define SCALER_DISPSTAT_COBLOW(x)		BIT(5 + (((x) + 1) * 8))

These are weird to me -- why are we doing 5 + (x + 1) * 8, instead of 13
+ x * 8?  The bottom 8 bits don't seem to be related to the 3 sets in
the top 24.

>  /* Set when the DISPEOLN line is done compositing. */
> -# define SCALER_DISPSTAT_EOLN0			BIT(12)
> +# define SCALER_DISPSTAT_EOLN(x)		BIT(4 + (((x) + 1) * 8))
>  /* Set when VSTART is seen but there are still pixels in the current
>   * output line.
>   */
> -# define SCALER_DISPSTAT_ESFRAME0		BIT(11)
> +# define SCALER_DISPSTAT_ESFRAME(x)		BIT(3 + (((x) + 1) * 8))
>  /* Set when HSTART is seen but there are still pixels in the current
>   * output line.
>   */
> -# define SCALER_DISPSTAT_ESLINE0		BIT(10)
> +# define SCALER_DISPSTAT_ESLINE(x)		BIT(2 + (((x) + 1) * 8))
>  /* Set when the the downstream tries to read from the display FIFO
>   * while it's empty.
>   */
> -# define SCALER_DISPSTAT_EUFLOW0		BIT(9)
> +# define SCALER_DISPSTAT_EUFLOW(x)		BIT(1 + (((x) + 1) * 8))
>  /* Set when the display mode changes from RUN to EOF */
> -# define SCALER_DISPSTAT_EOF0			BIT(8)
> +# define SCALER_DISPSTAT_EOF(x)			BIT(((x) + 1) * 8)
>  
>  /* Set on AXI invalid DMA ID error. */
>  # define SCALER_DISPSTAT_DMA_ERROR		BIT(7)
> @@ -300,12 +278,10 @@
>   * SCALER_DISPSTAT_RESP_ERROR is not SCALER_DISPSTAT_RESP_OKAY.
>   */
>  # define SCALER_DISPSTAT_IRQDMA			BIT(4)
> -# define SCALER_DISPSTAT_IRQDISP2		BIT(3)
> -# define SCALER_DISPSTAT_IRQDISP1		BIT(2)
>  /* Set when any of the EOF/EOLN/ESFRAME/ESLINE bits are set and their
>   * corresponding interrupt bit is enabled in DISPCTRL.
>   */
> -# define SCALER_DISPSTAT_IRQDISP0		BIT(1)
> +# define SCALER_DISPSTAT_IRQDISP(x)		BIT(1 + (x))
>  /* On read, the profiler interrupt.  On write, clear *all* interrupt bits. */
>  # define SCALER_DISPSTAT_IRQSCL			BIT(0)
>  
> -- 
> 2.20.1
Hi Eric,

On Wed, 2019-01-23 at 10:47 -0800, Eric Anholt wrote:
> Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:
> 
> > From: Boris Brezillon <boris.brezillon@bootlin.com>
> > 
> > The DRM framework provides a generic way to report underrun errors.
> > Let's implement the necessary hooks to support it in the VC4 driver.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> > Changes in v3:
> > - Generic underrun report function has been dropped, adjust the
> >   code accordingly
> 
> Update commit message for DRM framework not providing a generic way any
> more?

Woops, sorry I missed that and left the commit inconsistent with the
changelog, indeed.

[...]

> > +void vc4_hvs_mask_underrun(struct drm_device *dev)
> > +{
> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> > +	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
> > +
> > +	dispctrl &= ~(SCALER_DISPCTRL_DSPEISLUR(0) |
> > +		      SCALER_DISPCTRL_DSPEISLUR(1) |
> > +		      SCALER_DISPCTRL_DSPEISLUR(2));
> > +
> > +	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
> > +}
> > +
> > +void vc4_hvs_unmask_underrun(struct drm_device *dev)
> > +{
> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> > +	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
> > +
> > +	dispctrl |= SCALER_DISPCTRL_DSPEISLUR(0) |
> > +		    SCALER_DISPCTRL_DSPEISLUR(1) |
> > +		    SCALER_DISPCTRL_DSPEISLUR(2);
> > +
> > +	HVS_WRITE(SCALER_DISPSTAT,
> > +		  SCALER_DISPSTAT_EUFLOW(0) |
> > +		  SCALER_DISPSTAT_EUFLOW(1) |
> > +		  SCALER_DISPSTAT_EUFLOW(2));
> > +	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
> > +}
> > +
> > +static void vc4_hvs_report_underrun(struct drm_device *dev)
> > +{
> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> > +
> > +	atomic_inc(&vc4->underrun);
> > +	DRM_DEV_ERROR(dev->dev, "HVS underrun\n");
> > +}
> > +
> > +static irqreturn_t vc4_hvs_irq_handler(int irq, void *data)
> > +{
> > +	struct drm_device *dev = data;
> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> > +	u32 status;
> > +
> > +	status = HVS_READ(SCALER_DISPSTAT);
> > +
> > +	if (status &
> > +	    (SCALER_DISPSTAT_EUFLOW(0) | SCALER_DISPSTAT_EUFLOW(1) |
> > +	     SCALER_DISPSTAT_EUFLOW(2))) {
> > +		vc4_hvs_mask_underrun(dev);
> > +		vc4_hvs_report_underrun(dev);
> > +	}
> > +
> > +	HVS_WRITE(SCALER_DISPSTAT, status);
> > +
> > +	return status ? IRQ_HANDLED : IRQ_NONE;
> > +}
> 
> So, if UFLOW is set then we incremented the counter and disabled the
> interrupt, otherwise we acked this specific interrupt and return?  Given
> that a short-line error (the other potential cause of EISLUR) would be
> likely to persist, we should probably either vc4_hvs_mask_underrun() in
> that case too, or only return IRQ_HANDLED for the case we actually
> handled.

I see, there is definitely an inconsistency here. I don't think we
should be disabling the interrupt if we get a short line indication,
just in case the interrupt gets triggered later for a legitimate
underrun (before the next commit).

So I think we should just totally ignore the short line status bit for
the IRQ return (although it certainly doesn't hurt to clear it as
well). What do you think?

> > +
> >  static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> > @@ -236,15 +305,33 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
> >  	dispctrl = HVS_READ(SCALER_DISPCTRL);
> >  
> >  	dispctrl |= SCALER_DISPCTRL_ENABLE;
> > +	dispctrl |= SCALER_DISPCTRL_DSPEISLUR(0) | SCALER_DISPCTRL_DISPEIRQ(0) |
> > +		    SCALER_DISPCTRL_DSPEISLUR(1) | SCALER_DISPCTRL_DISPEIRQ(1) |
> > +		    SCALER_DISPCTRL_DSPEISLUR(2) | SCALER_DISPCTRL_DISPEIRQ(2);
> >  
> >  	/* Set DSP3 (PV1) to use HVS channel 2, which would otherwise
> >  	 * be unused.
> >  	 */
> >  	dispctrl &= ~SCALER_DISPCTRL_DSP3_MUX_MASK;
> > +	dispctrl &= ~(SCALER_DISPCTRL_DMAEIRQ |
> > +		      SCALER_DISPCTRL_SLVWREIRQ |
> > +		      SCALER_DISPCTRL_SLVRDEIRQ |
> > +		      SCALER_DISPCTRL_DSPEIEOF(0) |
> > +		      SCALER_DISPCTRL_DSPEIEOF(1) |
> > +		      SCALER_DISPCTRL_DSPEIEOF(2) |
> > +		      SCALER_DISPCTRL_DSPEIEOLN(0) |
> > +		      SCALER_DISPCTRL_DSPEIEOLN(1) |
> > +		      SCALER_DISPCTRL_DSPEIEOLN(2) |
> > +		      SCALER_DISPCTRL_SCLEIRQ);
> >  	dispctrl |= VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);
> 
> future work: At some point, we should stop inheriting old dispctrl setup
> and just initialize it on our own (so we get priority flags even if the
> firmware didn't set them up for us)

Sounds good, I'm taking a note of that for crafting a patch in that
direction.

> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index 2d66a2b57a91..a28e801aeae2 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -139,6 +139,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
> >  	struct drm_device *dev = state->dev;
> >  	struct vc4_dev *vc4 = to_vc4_dev(dev);
> >  
> > +	vc4_hvs_mask_underrun(dev);
> > +
> >  	drm_atomic_helper_wait_for_fences(dev, state, false);
> >  
> >  	drm_atomic_helper_wait_for_dependencies(state);
> > @@ -157,6 +159,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
> >  
> >  	vc4_hvs_sync_dlist(dev);
> >  
> > +	vc4_hvs_unmask_underrun(dev);
> > +
> >  	drm_atomic_helper_wait_for_flip_done(dev, state);
> >  
> >  	drm_atomic_helper_cleanup_planes(dev, state);
> > diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
> > index 50c653309aec..7e2692e9a83e 100644
> > --- a/drivers/gpu/drm/vc4/vc4_regs.h
> > +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> > @@ -217,8 +217,6 @@
> >  #define SCALER_DISPCTRL                         0x00000000
> >  /* Global register for clock gating the HVS */
> >  # define SCALER_DISPCTRL_ENABLE			BIT(31)
> > -# define SCALER_DISPCTRL_DSP2EISLUR		BIT(15)
> > -# define SCALER_DISPCTRL_DSP1EISLUR		BIT(14)
> >  # define SCALER_DISPCTRL_DSP3_MUX_MASK		VC4_MASK(19, 18)
> >  # define SCALER_DISPCTRL_DSP3_MUX_SHIFT		18
> >  
> > @@ -226,45 +224,25 @@
> >   * SCALER_DISPSTAT_IRQDISP0.  Note that short frame contributions are
> >   * always enabled.
> >   */
> > -# define SCALER_DISPCTRL_DSP0EISLUR		BIT(13)
> > -# define SCALER_DISPCTRL_DSP2EIEOLN		BIT(12)
> > -# define SCALER_DISPCTRL_DSP2EIEOF		BIT(11)
> > -# define SCALER_DISPCTRL_DSP1EIEOLN		BIT(10)
> > -# define SCALER_DISPCTRL_DSP1EIEOF		BIT(9)
> > +# define SCALER_DISPCTRL_DSPEISLUR(x)		BIT(13 + (x))
> >  /* Enables Display 0 end-of-line-N contribution to
> >   * SCALER_DISPSTAT_IRQDISP0
> >   */
> > -# define SCALER_DISPCTRL_DSP0EIEOLN		BIT(8)
> > +# define SCALER_DISPCTRL_DSPEIEOLN(x)		BIT(8 + ((x) * 2))
> >  /* Enables Display 0 EOF contribution to SCALER_DISPSTAT_IRQDISP0 */
> > -# define SCALER_DISPCTRL_DSP0EIEOF		BIT(7)
> > +# define SCALER_DISPCTRL_DSPEIEOF(x)		BIT(7 + ((x) * 2))
> >  
> >  # define SCALER_DISPCTRL_SLVRDEIRQ		BIT(6)
> >  # define SCALER_DISPCTRL_SLVWREIRQ		BIT(5)
> >  # define SCALER_DISPCTRL_DMAEIRQ		BIT(4)
> > -# define SCALER_DISPCTRL_DISP2EIRQ		BIT(3)
> > -# define SCALER_DISPCTRL_DISP1EIRQ		BIT(2)
> >  /* Enables interrupt generation on the enabled EOF/EOLN/EISLUR
> >   * bits and short frames..
> >   */
> > -# define SCALER_DISPCTRL_DISP0EIRQ		BIT(1)
> > +# define SCALER_DISPCTRL_DISPEIRQ(x)		BIT(1 + (x))
> >  /* Enables interrupt generation on scaler profiler interrupt. */
> >  # define SCALER_DISPCTRL_SCLEIRQ		BIT(0)
> >  
> >  #define SCALER_DISPSTAT                         0x00000004
> > -# define SCALER_DISPSTAT_COBLOW2		BIT(29)
> > -# define SCALER_DISPSTAT_EOLN2			BIT(28)
> > -# define SCALER_DISPSTAT_ESFRAME2		BIT(27)
> > -# define SCALER_DISPSTAT_ESLINE2		BIT(26)
> > -# define SCALER_DISPSTAT_EUFLOW2		BIT(25)
> > -# define SCALER_DISPSTAT_EOF2			BIT(24)
> > -
> > -# define SCALER_DISPSTAT_COBLOW1		BIT(21)
> > -# define SCALER_DISPSTAT_EOLN1			BIT(20)
> > -# define SCALER_DISPSTAT_ESFRAME1		BIT(19)
> > -# define SCALER_DISPSTAT_ESLINE1		BIT(18)
> > -# define SCALER_DISPSTAT_EUFLOW1		BIT(17)
> > -# define SCALER_DISPSTAT_EOF1			BIT(16)
> > -
> >  # define SCALER_DISPSTAT_RESP_MASK		VC4_MASK(15, 14)
> >  # define SCALER_DISPSTAT_RESP_SHIFT		14
> >  # define SCALER_DISPSTAT_RESP_OKAY		0
> > @@ -272,23 +250,23 @@
> >  # define SCALER_DISPSTAT_RESP_SLVERR		2
> >  # define SCALER_DISPSTAT_RESP_DECERR		3
> >  
> > -# define SCALER_DISPSTAT_COBLOW0		BIT(13)
> > +# define SCALER_DISPSTAT_COBLOW(x)		BIT(5 + (((x) + 1) * 8))
> 
> These are weird to me -- why are we doing 5 + (x + 1) * 8, instead of 13
> + x * 8?  The bottom 8 bits don't seem to be related to the 3 sets in
> the top 24.

The reasoning behind it is to think in terms of "bit offset within the
byte for display x" + "number of bytes to the byte for display x" which
I feel comes somewhat naturally when reading the datasheet (the bits
for each display start byte-aligned).

But without the overall structure in mind, I agree it's a bit
disturbing and it's a more complex expression than what you suggested
either way, so I'll use your suggestion in the next revision.

Cheers and thanks for the review,

Paul

> >  /* Set when the DISPEOLN line is done compositing. */
> > -# define SCALER_DISPSTAT_EOLN0			BIT(12)
> > +# define SCALER_DISPSTAT_EOLN(x)		BIT(4 + (((x) + 1) * 8))
> >  /* Set when VSTART is seen but there are still pixels in the current
> >   * output line.
> >   */
> > -# define SCALER_DISPSTAT_ESFRAME0		BIT(11)
> > +# define SCALER_DISPSTAT_ESFRAME(x)		BIT(3 + (((x) + 1) * 8))
> >  /* Set when HSTART is seen but there are still pixels in the current
> >   * output line.
> >   */
> > -# define SCALER_DISPSTAT_ESLINE0		BIT(10)
> > +# define SCALER_DISPSTAT_ESLINE(x)		BIT(2 + (((x) + 1) * 8))
> >  /* Set when the the downstream tries to read from the display FIFO
> >   * while it's empty.
> >   */
> > -# define SCALER_DISPSTAT_EUFLOW0		BIT(9)
> > +# define SCALER_DISPSTAT_EUFLOW(x)		BIT(1 + (((x) + 1) * 8))
> >  /* Set when the display mode changes from RUN to EOF */
> > -# define SCALER_DISPSTAT_EOF0			BIT(8)
> > +# define SCALER_DISPSTAT_EOF(x)			BIT(((x) + 1) * 8)
> >  
> >  /* Set on AXI invalid DMA ID error. */
> >  # define SCALER_DISPSTAT_DMA_ERROR		BIT(7)
> > @@ -300,12 +278,10 @@
> >   * SCALER_DISPSTAT_RESP_ERROR is not SCALER_DISPSTAT_RESP_OKAY.
> >   */
> >  # define SCALER_DISPSTAT_IRQDMA			BIT(4)
> > -# define SCALER_DISPSTAT_IRQDISP2		BIT(3)
> > -# define SCALER_DISPSTAT_IRQDISP1		BIT(2)
> >  /* Set when any of the EOF/EOLN/ESFRAME/ESLINE bits are set and their
> >   * corresponding interrupt bit is enabled in DISPCTRL.
> >   */
> > -# define SCALER_DISPSTAT_IRQDISP0		BIT(1)
> > +# define SCALER_DISPSTAT_IRQDISP(x)		BIT(1 + (x))
> >  /* On read, the profiler interrupt.  On write, clear *all* interrupt bits. */
> >  # define SCALER_DISPSTAT_IRQSCL			BIT(0)
> >  
> > -- 
> > 2.20.1
Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:

> Hi Eric,
>
> On Wed, 2019-01-23 at 10:47 -0800, Eric Anholt wrote:
>> Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:
>> > +void vc4_hvs_mask_underrun(struct drm_device *dev)
>> > +{
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
>> > +
>> > +	dispctrl &= ~(SCALER_DISPCTRL_DSPEISLUR(0) |
>> > +		      SCALER_DISPCTRL_DSPEISLUR(1) |
>> > +		      SCALER_DISPCTRL_DSPEISLUR(2));
>> > +
>> > +	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
>> > +}
>> > +
>> > +void vc4_hvs_unmask_underrun(struct drm_device *dev)
>> > +{
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +	u32 dispctrl = HVS_READ(SCALER_DISPCTRL);
>> > +
>> > +	dispctrl |= SCALER_DISPCTRL_DSPEISLUR(0) |
>> > +		    SCALER_DISPCTRL_DSPEISLUR(1) |
>> > +		    SCALER_DISPCTRL_DSPEISLUR(2);
>> > +
>> > +	HVS_WRITE(SCALER_DISPSTAT,
>> > +		  SCALER_DISPSTAT_EUFLOW(0) |
>> > +		  SCALER_DISPSTAT_EUFLOW(1) |
>> > +		  SCALER_DISPSTAT_EUFLOW(2));
>> > +	HVS_WRITE(SCALER_DISPCTRL, dispctrl);
>> > +}
>> > +
>> > +static void vc4_hvs_report_underrun(struct drm_device *dev)
>> > +{
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +
>> > +	atomic_inc(&vc4->underrun);
>> > +	DRM_DEV_ERROR(dev->dev, "HVS underrun\n");
>> > +}
>> > +
>> > +static irqreturn_t vc4_hvs_irq_handler(int irq, void *data)
>> > +{
>> > +	struct drm_device *dev = data;
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +	u32 status;
>> > +
>> > +	status = HVS_READ(SCALER_DISPSTAT);
>> > +
>> > +	if (status &
>> > +	    (SCALER_DISPSTAT_EUFLOW(0) | SCALER_DISPSTAT_EUFLOW(1) |
>> > +	     SCALER_DISPSTAT_EUFLOW(2))) {
>> > +		vc4_hvs_mask_underrun(dev);
>> > +		vc4_hvs_report_underrun(dev);
>> > +	}
>> > +
>> > +	HVS_WRITE(SCALER_DISPSTAT, status);
>> > +
>> > +	return status ? IRQ_HANDLED : IRQ_NONE;
>> > +}
>> 
>> So, if UFLOW is set then we incremented the counter and disabled the
>> interrupt, otherwise we acked this specific interrupt and return?  Given
>> that a short-line error (the other potential cause of EISLUR) would be
>> likely to persist, we should probably either vc4_hvs_mask_underrun() in
>> that case too, or only return IRQ_HANDLED for the case we actually
>> handled.
>
> I see, there is definitely an inconsistency here. I don't think we
> should be disabling the interrupt if we get a short line indication,
> just in case the interrupt gets triggered later for a legitimate
> underrun (before the next commit).
>
> So I think we should just totally ignore the short line status bit for
> the IRQ return (although it certainly doesn't hurt to clear it as
> well). What do you think?

You just have to make sure that you return UNHANDLED for short line, so
an IRQ storm doesn't take down the machine.