[PATCH v3 0/4] drm/vc4: Add a load tracker

[PATCH v3 0/4] drm/vc4: Add a load tracker

[Paul Kocialkowski]

Hi,

Here is a third iteration of the VC4 load tracking series, which was
initially developed by Boris Brezillon and that I have now taken over.

This new version aggregates a standalone patch adding a debugfs entry,
which was updated since its last revision. A new preliminary patch is
also introduced, which fixes underrun reports when reconfiguring the
display pipeline.

A detailed changelog is available on each patch when applicable.

Cheers,

Paul

Boris Brezillon (2):
  drm/vc4: Report underrun errors
  drm/vc4: Add a load tracker to prevent HVS underflow errors

Paul Kocialkowski (2):
  drm/vc4: Wait for display list synchronization when completing commit
  drm/vc4: Add a debugfs entry to disable/enable the load tracker

 drivers/gpu/drm/vc4/vc4_debugfs.c |  10 +++
 drivers/gpu/drm/vc4/vc4_drv.c     |   1 +
 drivers/gpu/drm/vc4/vc4_drv.h     |  25 +++++++
 drivers/gpu/drm/vc4/vc4_hvs.c     | 104 ++++++++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_kms.c     | 118 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/vc4/vc4_plane.c   |  57 +++++++++++++++
 drivers/gpu/drm/vc4/vc4_regs.h    |  48 ++++--------
 7 files changed, 327 insertions(+), 36 deletions(-)

-- 
2.20.1

[PATCH v3 1/4] drm/vc4: Wait for display list synchronization when completing commit

[Paul Kocialkowski]

During an atomic commit, the HVS is configured with a display list
for the channel matching the associated CRTC. The Pixel Valve (CRTC)
and encoder are also configured for the new setup at that time.
While the Pixel Valve and encoder are reconfigured synchronously, the
HVS is only reconfigured after the display list address (DISPLIST) has
been updated to the current display list address (DISPLACTX), which is
the responsibility of the hardware.

The time frame during which the HVS is still running on its previous
configuration but the CRTC and encoder have been reconfigured already
can lead to a number of synchronization issues. They will eventually
cause errors reported on the FIFOs, such as underruns.

With underrun detection enabled (from Boris Brezillon's series), this
leads to unreliable underrun detection with random false positives.

To ensure a coherent state, wait for each enabled channel of the HVS
to synchronize its current display list address. This fixes the issue
of random underrun reporting on commits.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
 drivers/gpu/drm/vc4/vc4_hvs.c  | 17 +++++++++++++++++
 drivers/gpu/drm/vc4/vc4_kms.c  |  2 ++
 drivers/gpu/drm/vc4/vc4_regs.h |  2 ++
 4 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index c24b078f0593..955f157f5ad0 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -772,6 +772,7 @@ void vc4_irq_reset(struct drm_device *dev);
 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);
 
 /* 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 5d8c749c9749..1ba60b8e0c2d 100644
--- a/drivers/gpu/drm/vc4/vc4_hvs.c
+++ b/drivers/gpu/drm/vc4/vc4_hvs.c
@@ -166,6 +166,23 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
 	return 0;
 }
 
+void vc4_hvs_sync_dlist(struct drm_device *dev)
+{
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < SCALER_CHANNELS_COUNT; i++) {
+		if (!(HVS_READ(SCALER_DISPCTRLX(i)) & SCALER_DISPCTRLX_ENABLE))
+			continue;
+
+		ret = wait_for(HVS_READ(SCALER_DISPLACTX(i)) ==
+			       HVS_READ(SCALER_DISPLISTX(i)), 1000);
+		WARN(ret, "Timeout waiting for channel %d display list sync\n",
+		     i);
+	}
+}
+
 static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
 {
 	struct platform_device *pdev = to_platform_device(dev);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 0490edb192a1..2d66a2b57a91 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -155,6 +155,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 
 	drm_atomic_helper_commit_hw_done(state);
 
+	vc4_hvs_sync_dlist(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 931088014272..50c653309aec 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -212,6 +212,8 @@
 
 #define PV_HACT_ACT				0x30
 
+#define SCALER_CHANNELS_COUNT			3
+
 #define SCALER_DISPCTRL                         0x00000000
 /* Global register for clock gating the HVS */
 # define SCALER_DISPCTRL_ENABLE			BIT(31)
-- 
2.20.1

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

[Paul Kocialkowski]

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(-)

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)
 
-- 
2.20.1

[PATCH v3 3/4] drm/vc4: Add a load tracker to prevent HVS underflow errors

[Paul Kocialkowski]

From: Boris Brezillon <boris.brezillon@bootlin.com>

The HVS block is supposed to fill the pixelvalve FIFOs fast enough to
meet the requested framerate. The problem is, the HVS and memory bus
bandwidths are limited, and if we don't take these limitations into
account we might end up with HVS underflow errors.

This patch is trying to model the per-plane HVS and memory bus bandwidth
consumption and take a decision at atomic_check() time whether the
estimated load will fit in the HVS and membus budget.

Note that we take an extra margin on the memory bus consumption to let
the system run smoothly when other blocks are doing heavy use of the
memory bus. Same goes for the HVS limit, except the margin is smaller in
this case, since the HVS is not used by external components.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v3:
- Use an u32 instead of an unsigned long to store/calculate the load
- Fix a typo in a comment
- Fix HVS load calculation (use crtc_h/w instead of src_h/w)

Changes in v2:
- Remove an unused var in vc4_load_tracker_atomic_check()
---
 drivers/gpu/drm/vc4/vc4_drv.c   |   1 +
 drivers/gpu/drm/vc4/vc4_drv.h   |  11 ++++
 drivers/gpu/drm/vc4/vc4_kms.c   | 103 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/vc4/vc4_plane.c |  57 ++++++++++++++++++
 4 files changed, 171 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index f6f5cd80c04d..7195a0bcceb3 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -313,6 +313,7 @@ static void vc4_drm_unbind(struct device *dev)
 
 	drm_mode_config_cleanup(drm);
 
+	drm_atomic_private_obj_fini(&vc4->load_tracker);
 	drm_atomic_private_obj_fini(&vc4->ctm_manager);
 
 	drm_dev_put(drm);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 619fb8bec428..30cde58be737 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -207,6 +207,7 @@ struct vc4_dev {
 
 	struct drm_modeset_lock ctm_state_lock;
 	struct drm_private_obj ctm_manager;
+	struct drm_private_obj load_tracker;
 };
 
 static inline struct vc4_dev *
@@ -382,6 +383,16 @@ struct vc4_plane_state {
 	 * when async update is not possible.
 	 */
 	bool dlist_initialized;
+
+	/* Load of this plane on the HVS block. The load is expressed in HVS
+	 * cycles/sec.
+	 */
+	u64 hvs_load;
+
+	/* Memory bandwidth needed for this plane. This is expressed in
+	 * bytes/sec.
+	 */
+	u64 membus_load;
 };
 
 static inline struct vc4_plane_state *
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index a28e801aeae2..695ec793d429 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -34,6 +34,18 @@ static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
 	return container_of(priv, struct vc4_ctm_state, base);
 }
 
+struct vc4_load_tracker_state {
+	struct drm_private_state base;
+	u64 hvs_load;
+	u64 membus_load;
+};
+
+static struct vc4_load_tracker_state *
+to_vc4_load_tracker_state(struct drm_private_state *priv)
+{
+	return container_of(priv, struct vc4_load_tracker_state, base);
+}
+
 static struct vc4_ctm_state *vc4_get_ctm_state(struct drm_atomic_state *state,
 					       struct drm_private_obj *manager)
 {
@@ -391,6 +403,81 @@ vc4_ctm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 	return 0;
 }
 
+static int vc4_load_tracker_atomic_check(struct drm_atomic_state *state)
+{
+	struct drm_plane_state *old_plane_state, *new_plane_state;
+	struct vc4_dev *vc4 = to_vc4_dev(state->dev);
+	struct vc4_load_tracker_state *load_state;
+	struct drm_private_state *priv_state;
+	struct drm_plane *plane;
+	int i;
+
+	priv_state = drm_atomic_get_private_obj_state(state,
+						      &vc4->load_tracker);
+	if (IS_ERR(priv_state))
+		return PTR_ERR(priv_state);
+
+	load_state = to_vc4_load_tracker_state(priv_state);
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state,
+				       new_plane_state, i) {
+		struct vc4_plane_state *vc4_plane_state;
+
+		if (old_plane_state->fb && old_plane_state->crtc) {
+			vc4_plane_state = to_vc4_plane_state(old_plane_state);
+			load_state->membus_load -= vc4_plane_state->membus_load;
+			load_state->hvs_load -= vc4_plane_state->hvs_load;
+		}
+
+		if (new_plane_state->fb && new_plane_state->crtc) {
+			vc4_plane_state = to_vc4_plane_state(new_plane_state);
+			load_state->membus_load += vc4_plane_state->membus_load;
+			load_state->hvs_load += vc4_plane_state->hvs_load;
+		}
+	}
+
+	/* The absolute limit is 2Gbyte/sec, but let's take a margin to let
+	 * the system work when other blocks are accessing the memory.
+	 */
+	if (load_state->membus_load > SZ_1G + SZ_512M)
+		return -ENOSPC;
+
+	/* HVS clock is supposed to run @ 250Mhz, let's take a margin and
+	 * consider the maximum number of cycles is 240M.
+	 */
+	if (load_state->hvs_load > 240000000ULL)
+		return -ENOSPC;
+
+	return 0;
+}
+
+static struct drm_private_state *
+vc4_load_tracker_duplicate_state(struct drm_private_obj *obj)
+{
+	struct vc4_load_tracker_state *state;
+
+	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+	return &state->base;
+}
+
+static void vc4_load_tracker_destroy_state(struct drm_private_obj *obj,
+					   struct drm_private_state *state)
+{
+	struct vc4_load_tracker_state *load_state;
+
+	load_state = to_vc4_load_tracker_state(state);
+	kfree(load_state);
+}
+
+static const struct drm_private_state_funcs vc4_load_tracker_state_funcs = {
+	.atomic_duplicate_state = vc4_load_tracker_duplicate_state,
+	.atomic_destroy_state = vc4_load_tracker_destroy_state,
+};
+
 static int
 vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 {
@@ -400,7 +487,11 @@ vc4_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 	if (ret < 0)
 		return ret;
 
-	return drm_atomic_helper_check(dev, state);
+	ret = drm_atomic_helper_check(dev, state);
+	if (ret)
+		return ret;
+
+	return vc4_load_tracker_atomic_check(state);
 }
 
 static const struct drm_mode_config_funcs vc4_mode_funcs = {
@@ -413,6 +504,7 @@ int vc4_kms_load(struct drm_device *dev)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
 	struct vc4_ctm_state *ctm_state;
+	struct vc4_load_tracker_state *load_state;
 	int ret;
 
 	sema_init(&vc4->async_modeset, 1);
@@ -442,6 +534,15 @@ int vc4_kms_load(struct drm_device *dev)
 	drm_atomic_private_obj_init(dev, &vc4->ctm_manager, &ctm_state->base,
 				    &vc4_ctm_state_funcs);
 
+	load_state = kzalloc(sizeof(*load_state), GFP_KERNEL);
+	if (!load_state) {
+		drm_atomic_private_obj_fini(&vc4->ctm_manager);
+		return -ENOMEM;
+	}
+
+	drm_atomic_private_obj_init(dev, &vc4->load_tracker, &load_state->base,
+				    &vc4_load_tracker_state_funcs);
+
 	drm_mode_config_reset(dev);
 
 	drm_kms_helper_poll_init(dev);
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 2901ed0c5223..6c643953a6e2 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -488,6 +488,61 @@ static void vc4_write_scaling_parameters(struct drm_plane_state *state,
 	}
 }
 
+static void vc4_plane_calc_load(struct drm_plane_state *state)
+{
+	unsigned int hvs_load_shift, vrefresh, i;
+	struct drm_framebuffer *fb = state->fb;
+	struct vc4_plane_state *vc4_state;
+	struct drm_crtc_state *crtc_state;
+	unsigned int vscale_factor;
+
+	vc4_state = to_vc4_plane_state(state);
+	crtc_state = drm_atomic_get_existing_crtc_state(state->state,
+							state->crtc);
+	vrefresh = drm_mode_vrefresh(&crtc_state->adjusted_mode);
+
+	/* The HVS is able to process 2 pixels/cycle when scaling the source,
+	 * 4 pixels/cycle otherwise.
+	 * Alpha blending step seems to be pipelined and it's always operating
+	 * at 4 pixels/cycle, so the limiting aspect here seems to be the
+	 * scaler block.
+	 * HVS load is expressed in clk-cycles/sec (AKA Hz).
+	 */
+	if (vc4_state->x_scaling[0] != VC4_SCALING_NONE ||
+	    vc4_state->x_scaling[1] != VC4_SCALING_NONE ||
+	    vc4_state->y_scaling[0] != VC4_SCALING_NONE ||
+	    vc4_state->y_scaling[1] != VC4_SCALING_NONE)
+		hvs_load_shift = 1;
+	else
+		hvs_load_shift = 2;
+
+	vc4_state->membus_load = 0;
+	vc4_state->hvs_load = 0;
+	for (i = 0; i < fb->format->num_planes; i++) {
+		/* Even if the bandwidth/plane required for a single frame is
+		 *
+		 * vc4_state->src_w[i] * vc4_state->src_h[i] * cpp * vrefresh
+		 *
+		 * when downscaling, we have to read more pixels per line in
+		 * the time frame reserved for a single line, so the bandwidth
+		 * demand can be punctually higher. To account for that, we
+		 * calculate the down-scaling factor and multiply the plane
+		 * load by this number. We're likely over-estimating the read
+		 * demand, but that's better than under-estimating it.
+		 */
+		vscale_factor = DIV_ROUND_UP(vc4_state->src_h[i],
+					     vc4_state->crtc_h);
+		vc4_state->membus_load += vc4_state->src_w[i] *
+					  vc4_state->src_h[i] * vscale_factor *
+					  fb->format->cpp[i];
+		vc4_state->hvs_load += vc4_state->crtc_h * vc4_state->crtc_w;
+	}
+
+	vc4_state->hvs_load *= vrefresh;
+	vc4_state->hvs_load >>= hvs_load_shift;
+	vc4_state->membus_load *= vrefresh;
+}
+
 static int vc4_plane_allocate_lbm(struct drm_plane_state *state)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(state->plane->dev);
@@ -888,6 +943,8 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	 */
 	vc4_state->dlist_initialized = 1;
 
+	vc4_plane_calc_load(state);
+
 	return 0;
 }
 
-- 
2.20.1

[PATCH v3 4/4] drm/vc4: Add a debugfs entry to disable/enable the load tracker

[Paul Kocialkowski]

In order to test whether the load tracker is working as expected, we
need the ability to compare the commit result with the underrun
indication. With the load tracker always enabled, commits that are
expected to trigger an underrun are always rejected, so userspace
cannot get the actual underrun indication from the hardware.

Add a debugfs entry to disable/enable the load tracker, so that a DRM
commit expected to trigger an underrun can go through with the load
tracker disabled. The underrun indication is then available to
userspace and can be checked against the commit result with the load
tracker enabled.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes since v3 (of the standalone patch):
* Added a comment for the load_tracker_enabled field;
* Moved the ->load_tracker_enabled initialization in vc4_kms_load();
* Moved the if (vc4->load_tracker_enabled) check in
  vc4_load_tracker_atomic_check();
* Renamed the debugfs node to hvs_load_tracker to match hvs_underrun.

Changes since v2:
* Used the standard debugfs_create_bool instead of reinventing it.

Changes since v1:
* Moved all the debugfs-related functions and structure to vc4_debugfs.c.
---
 drivers/gpu/drm/vc4/vc4_debugfs.c | 9 +++++++++
 drivers/gpu/drm/vc4/vc4_drv.h     | 3 +++
 drivers/gpu/drm/vc4/vc4_kms.c     | 9 +++++++++
 3 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c
index 64021bcba041..59cdad89f844 100644
--- a/drivers/gpu/drm/vc4/vc4_debugfs.c
+++ b/drivers/gpu/drm/vc4/vc4_debugfs.c
@@ -36,6 +36,15 @@ static const struct drm_info_list vc4_debugfs_list[] = {
 int
 vc4_debugfs_init(struct drm_minor *minor)
 {
+	struct vc4_dev *vc4 = to_vc4_dev(minor->dev);
+	struct dentry *dentry;
+
+	dentry = debugfs_create_bool("hvs_load_tracker", S_IRUGO | S_IWUSR,
+				     minor->debugfs_root,
+				     &vc4->load_tracker_enabled);
+	if (!dentry)
+		return -ENOMEM;
+
 	return drm_debugfs_create_files(vc4_debugfs_list, VC4_DEBUGFS_ENTRIES,
 					minor->debugfs_root, minor);
 }
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 30cde58be737..4ba05c955787 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -195,6 +195,9 @@ struct vc4_dev {
 
 	int power_refcount;
 
+	/* Set to true when the load tracker is active. */
+	bool load_tracker_enabled;
+
 	/* Mutex controlling the power refcount. */
 	struct mutex power_lock;
 
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 695ec793d429..48f07f371e63 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -435,6 +435,10 @@ static int vc4_load_tracker_atomic_check(struct drm_atomic_state *state)
 		}
 	}
 
+	/* Don't check the load when the tracker is disabled. */
+	if (!vc4->load_tracker_enabled)
+		return 0;
+
 	/* The absolute limit is 2Gbyte/sec, but let's take a margin to let
 	 * the system work when other blocks are accessing the memory.
 	 */
@@ -507,6 +511,11 @@ int vc4_kms_load(struct drm_device *dev)
 	struct vc4_load_tracker_state *load_state;
 	int ret;
 
+	/* Start with the load tracker enabled. Can be disabled through the
+	 * debugfs load_tracker file.
+	 */
+	vc4->load_tracker_enabled = true;
+
 	sema_init(&vc4->async_modeset, 1);
 
 	/* Set support for vblank irq fast disable, before drm_vblank_init() */
-- 
2.20.1

Re: [PATCH v3 1/4] drm/vc4: Wait for display list synchronization when completing commit

[Daniel Vetter]

On Tue, Jan 8, 2019 at 3:51 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> During an atomic commit, the HVS is configured with a display list
> for the channel matching the associated CRTC. The Pixel Valve (CRTC)
> and encoder are also configured for the new setup at that time.
> While the Pixel Valve and encoder are reconfigured synchronously, the
> HVS is only reconfigured after the display list address (DISPLIST) has
> been updated to the current display list address (DISPLACTX), which is
> the responsibility of the hardware.
>
> The time frame during which the HVS is still running on its previous
> configuration but the CRTC and encoder have been reconfigured already
> can lead to a number of synchronization issues. They will eventually
> cause errors reported on the FIFOs, such as underruns.
>
> With underrun detection enabled (from Boris Brezillon's series), this
> leads to unreliable underrun detection with random false positives.
>
> To ensure a coherent state, wait for each enabled channel of the HVS
> to synchronize its current display list address. This fixes the issue
> of random underrun reporting on commits.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>  drivers/gpu/drm/vc4/vc4_hvs.c  | 17 +++++++++++++++++
>  drivers/gpu/drm/vc4/vc4_kms.c  |  2 ++
>  drivers/gpu/drm/vc4/vc4_regs.h |  2 ++
>  4 files changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index c24b078f0593..955f157f5ad0 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -772,6 +772,7 @@ void vc4_irq_reset(struct drm_device *dev);
>  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);
>
>  /* 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 5d8c749c9749..1ba60b8e0c2d 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -166,6 +166,23 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
>         return 0;
>  }
>
> +void vc4_hvs_sync_dlist(struct drm_device *dev)
> +{
> +       struct vc4_dev *vc4 = to_vc4_dev(dev);
> +       unsigned int i;
> +       int ret;
> +
> +       for (i = 0; i < SCALER_CHANNELS_COUNT; i++) {
> +               if (!(HVS_READ(SCALER_DISPCTRLX(i)) & SCALER_DISPCTRLX_ENABLE))
> +                       continue;
> +
> +               ret = wait_for(HVS_READ(SCALER_DISPLACTX(i)) ==
> +                              HVS_READ(SCALER_DISPLISTX(i)), 1000);
> +               WARN(ret, "Timeout waiting for channel %d display list sync\n",
> +                    i);
> +       }
> +}
> +
>  static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>  {
>         struct platform_device *pdev = to_platform_device(dev);
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 0490edb192a1..2d66a2b57a91 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -155,6 +155,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>
>         drm_atomic_helper_commit_hw_done(state);
>
> +       vc4_hvs_sync_dlist(dev);

From your description I'd have guessed you want this between when you
update the planes and the crtc, so somewhere between commit_planes()
and commit_modeset_enables(). At least I have no idea how waiting here
can prevent underruns, by this point there's no further hw programming
happening. Only exception is if you have an IOMMU which can fault, in
which case the cleanup_planes might remove the buffers prematurely.
But if that's the problem, then your semantics of the flip_done event
are wrong - when flip_done is signalled, the hw must have stopped
scanning out the old planes, since userspace expects to be able to
start overwriting/reusing them.
-Daniel

> +
>         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 931088014272..50c653309aec 100644
> --- a/drivers/gpu/drm/vc4/vc4_regs.h
> +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> @@ -212,6 +212,8 @@
>
>  #define PV_HACT_ACT                            0x30
>
> +#define SCALER_CHANNELS_COUNT                  3
> +
>  #define SCALER_DISPCTRL                         0x00000000
>  /* Global register for clock gating the HVS */
>  # define SCALER_DISPCTRL_ENABLE                        BIT(31)
> --
> 2.20.1
>


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

Re: [PATCH v3 1/4] drm/vc4: Wait for display list synchronization when completing commit

[Paul Kocialkowski]

Hi Daniel,

On Tue, 2019-01-08 at 19:21 +0100, Daniel Vetter wrote:
> On Tue, Jan 8, 2019 at 3:51 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > During an atomic commit, the HVS is configured with a display list
> > for the channel matching the associated CRTC. The Pixel Valve (CRTC)
> > and encoder are also configured for the new setup at that time.
> > While the Pixel Valve and encoder are reconfigured synchronously, the
> > HVS is only reconfigured after the display list address (DISPLIST) has
> > been updated to the current display list address (DISPLACTX), which is
> > the responsibility of the hardware.
> > 
> > The time frame during which the HVS is still running on its previous
> > configuration but the CRTC and encoder have been reconfigured already
> > can lead to a number of synchronization issues. They will eventually
> > cause errors reported on the FIFOs, such as underruns.
> > 
> > With underrun detection enabled (from Boris Brezillon's series), this
> > leads to unreliable underrun detection with random false positives.
> > 
> > To ensure a coherent state, wait for each enabled channel of the HVS
> > to synchronize its current display list address. This fixes the issue
> > of random underrun reporting on commits.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
> >  drivers/gpu/drm/vc4/vc4_hvs.c  | 17 +++++++++++++++++
> >  drivers/gpu/drm/vc4/vc4_kms.c  |  2 ++
> >  drivers/gpu/drm/vc4/vc4_regs.h |  2 ++
> >  4 files changed, 22 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index c24b078f0593..955f157f5ad0 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -772,6 +772,7 @@ void vc4_irq_reset(struct drm_device *dev);
> >  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);
> > 
> >  /* 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 5d8c749c9749..1ba60b8e0c2d 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> > @@ -166,6 +166,23 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
> >         return 0;
> >  }
> > 
> > +void vc4_hvs_sync_dlist(struct drm_device *dev)
> > +{
> > +       struct vc4_dev *vc4 = to_vc4_dev(dev);
> > +       unsigned int i;
> > +       int ret;
> > +
> > +       for (i = 0; i < SCALER_CHANNELS_COUNT; i++) {
> > +               if (!(HVS_READ(SCALER_DISPCTRLX(i)) & SCALER_DISPCTRLX_ENABLE))
> > +                       continue;
> > +
> > +               ret = wait_for(HVS_READ(SCALER_DISPLACTX(i)) ==
> > +                              HVS_READ(SCALER_DISPLISTX(i)), 1000);
> > +               WARN(ret, "Timeout waiting for channel %d display list sync\n",
> > +                    i);
> > +       }
> > +}
> > +
> >  static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
> >  {
> >         struct platform_device *pdev = to_platform_device(dev);
> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index 0490edb192a1..2d66a2b57a91 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -155,6 +155,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
> > 
> >         drm_atomic_helper_commit_hw_done(state);
> > 
> > +       vc4_hvs_sync_dlist(dev);
> 
> From your description I'd have guessed you want this between when you
> update the planes and the crtc, so somewhere between commit_planes()
> and commit_modeset_enables(). At least I have no idea how waiting here
> can prevent underruns, by this point there's no further hw programming
> happening.

One thing that I did not mention is that the display list (that
configures the planes) is only set at crtc_enable time (and taken into
account by the hardware later).

However, even calling vc4_hvs_sync_dlist right at the end of
crtc_enable doesn't do either (the old display list just sticks). It
only seems to work after the HDMI encoder enable step and I don't know
any good reason why.

I didn't find any description of when that dlist sync mechanism is
supposed to take place and what particular event triggers it. Perhaps
it is triggered by a signal originating from the encoder? If anyone has
insight on the hardware, feel free to shed some light here :)

Cheers and thanks for the review,

Paul

> Only exception is if you have an IOMMU which can fault, in
> which case the cleanup_planes might remove the buffers prematurely.
> But if that's the problem, then your semantics of the flip_done event
> are wrong - when flip_done is signalled, the hw must have stopped
> scanning out the old planes, since userspace expects to be able to
> start overwriting/reusing them.
> -Daniel
> 
> > +
> >         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 931088014272..50c653309aec 100644
> > --- a/drivers/gpu/drm/vc4/vc4_regs.h
> > +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> > @@ -212,6 +212,8 @@
> > 
> >  #define PV_HACT_ACT                            0x30
> > 
> > +#define SCALER_CHANNELS_COUNT                  3
> > +
> >  #define SCALER_DISPCTRL                         0x00000000
> >  /* Global register for clock gating the HVS */
> >  # define SCALER_DISPCTRL_ENABLE                        BIT(31)
> > --
> > 2.20.1
> > 
> 
> 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v3 1/4] drm/vc4: Wait for display list synchronization when completing commit

[Daniel Vetter]

On Wed, Jan 09, 2019 at 05:52:20PM +0100, Paul Kocialkowski wrote:
> Hi Daniel,
> 
> On Tue, 2019-01-08 at 19:21 +0100, Daniel Vetter wrote:
> > On Tue, Jan 8, 2019 at 3:51 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > During an atomic commit, the HVS is configured with a display list
> > > for the channel matching the associated CRTC. The Pixel Valve (CRTC)
> > > and encoder are also configured for the new setup at that time.
> > > While the Pixel Valve and encoder are reconfigured synchronously, the
> > > HVS is only reconfigured after the display list address (DISPLIST) has
> > > been updated to the current display list address (DISPLACTX), which is
> > > the responsibility of the hardware.
> > > 
> > > The time frame during which the HVS is still running on its previous
> > > configuration but the CRTC and encoder have been reconfigured already
> > > can lead to a number of synchronization issues. They will eventually
> > > cause errors reported on the FIFOs, such as underruns.
> > > 
> > > With underrun detection enabled (from Boris Brezillon's series), this
> > > leads to unreliable underrun detection with random false positives.
> > > 
> > > To ensure a coherent state, wait for each enabled channel of the HVS
> > > to synchronize its current display list address. This fixes the issue
> > > of random underrun reporting on commits.
> > > 
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > ---
> > >  drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
> > >  drivers/gpu/drm/vc4/vc4_hvs.c  | 17 +++++++++++++++++
> > >  drivers/gpu/drm/vc4/vc4_kms.c  |  2 ++
> > >  drivers/gpu/drm/vc4/vc4_regs.h |  2 ++
> > >  4 files changed, 22 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > > index c24b078f0593..955f157f5ad0 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > > @@ -772,6 +772,7 @@ void vc4_irq_reset(struct drm_device *dev);
> > >  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);
> > > 
> > >  /* 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 5d8c749c9749..1ba60b8e0c2d 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> > > @@ -166,6 +166,23 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
> > >         return 0;
> > >  }
> > > 
> > > +void vc4_hvs_sync_dlist(struct drm_device *dev)
> > > +{
> > > +       struct vc4_dev *vc4 = to_vc4_dev(dev);
> > > +       unsigned int i;
> > > +       int ret;
> > > +
> > > +       for (i = 0; i < SCALER_CHANNELS_COUNT; i++) {
> > > +               if (!(HVS_READ(SCALER_DISPCTRLX(i)) & SCALER_DISPCTRLX_ENABLE))
> > > +                       continue;
> > > +
> > > +               ret = wait_for(HVS_READ(SCALER_DISPLACTX(i)) ==
> > > +                              HVS_READ(SCALER_DISPLISTX(i)), 1000);
> > > +               WARN(ret, "Timeout waiting for channel %d display list sync\n",
> > > +                    i);
> > > +       }
> > > +}
> > > +
> > >  static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
> > >  {
> > >         struct platform_device *pdev = to_platform_device(dev);
> > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > > index 0490edb192a1..2d66a2b57a91 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > > @@ -155,6 +155,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
> > > 
> > >         drm_atomic_helper_commit_hw_done(state);
> > > 
> > > +       vc4_hvs_sync_dlist(dev);
> > 
> > From your description I'd have guessed you want this between when you
> > update the planes and the crtc, so somewhere between commit_planes()
> > and commit_modeset_enables(). At least I have no idea how waiting here
> > can prevent underruns, by this point there's no further hw programming
> > happening.
> 
> One thing that I did not mention is that the display list (that
> configures the planes) is only set at crtc_enable time (and taken into
> account by the hardware later).
> 
> However, even calling vc4_hvs_sync_dlist right at the end of
> crtc_enable doesn't do either (the old display list just sticks). It
> only seems to work after the HDMI encoder enable step and I don't know
> any good reason why.
> 
> I didn't find any description of when that dlist sync mechanism is
> supposed to take place and what particular event triggers it. Perhaps
> it is triggered by a signal originating from the encoder? If anyone has
> insight on the hardware, feel free to shed some light here :)

Maybe my concern wasn't clear: I have no idea why you need this exactly
and how your hw works. Only thing I meant to highlight is that since all
you're doing is wait a bit, then the only reason I can come up with why
that wait does anything is cleanup_planes() later on. And if that's the
case, then you also need to sufficiently delay the flip_done signalling to
userspace (i.e. sending out the crtc_state->event vblank event).

But I'm really not understanding what the hw does and how your patch here
helps at all. It just looked really strange from a atomic kms pov.
-Daniel

> 
> Cheers and thanks for the review,
> 
> Paul
> 
> > Only exception is if you have an IOMMU which can fault, in
> > which case the cleanup_planes might remove the buffers prematurely.
> > But if that's the problem, then your semantics of the flip_done event
> > are wrong - when flip_done is signalled, the hw must have stopped
> > scanning out the old planes, since userspace expects to be able to
> > start overwriting/reusing them.
> > -Daniel
> > 
> > > +
> > >         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 931088014272..50c653309aec 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_regs.h
> > > +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> > > @@ -212,6 +212,8 @@
> > > 
> > >  #define PV_HACT_ACT                            0x30
> > > 
> > > +#define SCALER_CHANNELS_COUNT                  3
> > > +
> > >  #define SCALER_DISPCTRL                         0x00000000
> > >  /* Global register for clock gating the HVS */
> > >  # define SCALER_DISPCTRL_ENABLE                        BIT(31)
> > > --
> > > 2.20.1
> > > 
> > 
> > 
> -- 
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 1/4] drm/vc4: Wait for display list synchronization when completing commit

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3568 bytes --]

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

> During an atomic commit, the HVS is configured with a display list
> for the channel matching the associated CRTC. The Pixel Valve (CRTC)
> and encoder are also configured for the new setup at that time.
> While the Pixel Valve and encoder are reconfigured synchronously, the
> HVS is only reconfigured after the display list address (DISPLIST) has
> been updated to the current display list address (DISPLACTX), which is
> the responsibility of the hardware.
>
> The time frame during which the HVS is still running on its previous
> configuration but the CRTC and encoder have been reconfigured already
> can lead to a number of synchronization issues. They will eventually
> cause errors reported on the FIFOs, such as underruns.
>
> With underrun detection enabled (from Boris Brezillon's series), this
> leads to unreliable underrun detection with random false positives.
>
> To ensure a coherent state, wait for each enabled channel of the HVS
> to synchronize its current display list address. This fixes the issue
> of random underrun reporting on commits.
>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>  drivers/gpu/drm/vc4/vc4_hvs.c  | 17 +++++++++++++++++
>  drivers/gpu/drm/vc4/vc4_kms.c  |  2 ++
>  drivers/gpu/drm/vc4/vc4_regs.h |  2 ++
>  4 files changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index c24b078f0593..955f157f5ad0 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -772,6 +772,7 @@ void vc4_irq_reset(struct drm_device *dev);
>  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);
>  
>  /* 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 5d8c749c9749..1ba60b8e0c2d 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -166,6 +166,23 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
>  	return 0;
>  }
>  
> +void vc4_hvs_sync_dlist(struct drm_device *dev)
> +{
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < SCALER_CHANNELS_COUNT; i++) {
> +		if (!(HVS_READ(SCALER_DISPCTRLX(i)) & SCALER_DISPCTRLX_ENABLE))
> +			continue;
> +
> +		ret = wait_for(HVS_READ(SCALER_DISPLACTX(i)) ==
> +			       HVS_READ(SCALER_DISPLISTX(i)), 1000);
> +		WARN(ret, "Timeout waiting for channel %d display list sync\n",
> +		     i);
> +	}
> +}
> +
>  static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 0490edb192a1..2d66a2b57a91 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -155,6 +155,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>  
>  	drm_atomic_helper_commit_hw_done(state);
>  
> +	vc4_hvs_sync_dlist(dev);
> +
>  	drm_atomic_helper_wait_for_flip_done(dev, state);

wait_for_flip_done should already be waiting for DISPLACTX to match our
crtc's display list, though, right (see vc4_crtc_handle_page_flip())?
This seems like a no-op to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 12762 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Paul Kocialkowski]

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, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH v3 1/4] drm/vc4: Wait for display list synchronization when completing commit

[Paul Kocialkowski]

Hi,

On Wed, 2019-01-23 at 10:34 -0800, Eric Anholt wrote:
> Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:
> 
> > During an atomic commit, the HVS is configured with a display list
> > for the channel matching the associated CRTC. The Pixel Valve (CRTC)
> > and encoder are also configured for the new setup at that time.
> > While the Pixel Valve and encoder are reconfigured synchronously, the
> > HVS is only reconfigured after the display list address (DISPLIST) has
> > been updated to the current display list address (DISPLACTX), which is
> > the responsibility of the hardware.
> > 
> > The time frame during which the HVS is still running on its previous
> > configuration but the CRTC and encoder have been reconfigured already
> > can lead to a number of synchronization issues. They will eventually
> > cause errors reported on the FIFOs, such as underruns.
> > 
> > With underrun detection enabled (from Boris Brezillon's series), this
> > leads to unreliable underrun detection with random false positives.
> > 
> > To ensure a coherent state, wait for each enabled channel of the HVS
> > to synchronize its current display list address. This fixes the issue
> > of random underrun reporting on commits.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
> >  drivers/gpu/drm/vc4/vc4_hvs.c  | 17 +++++++++++++++++
> >  drivers/gpu/drm/vc4/vc4_kms.c  |  2 ++
> >  drivers/gpu/drm/vc4/vc4_regs.h |  2 ++
> >  4 files changed, 22 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> > index c24b078f0593..955f157f5ad0 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> > @@ -772,6 +772,7 @@ void vc4_irq_reset(struct drm_device *dev);
> >  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);
> >  
> >  /* 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 5d8c749c9749..1ba60b8e0c2d 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> > @@ -166,6 +166,23 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
> >  	return 0;
> >  }
> >  
> > +void vc4_hvs_sync_dlist(struct drm_device *dev)
> > +{
> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < SCALER_CHANNELS_COUNT; i++) {
> > +		if (!(HVS_READ(SCALER_DISPCTRLX(i)) & SCALER_DISPCTRLX_ENABLE))
> > +			continue;
> > +
> > +		ret = wait_for(HVS_READ(SCALER_DISPLACTX(i)) ==
> > +			       HVS_READ(SCALER_DISPLISTX(i)), 1000);
> > +		WARN(ret, "Timeout waiting for channel %d display list sync\n",
> > +		     i);
> > +	}
> > +}
> > +
> >  static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index 0490edb192a1..2d66a2b57a91 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -155,6 +155,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
> >  
> >  	drm_atomic_helper_commit_hw_done(state);
> >  
> > +	vc4_hvs_sync_dlist(dev);
> > +
> >  	drm_atomic_helper_wait_for_flip_done(dev, state);
> 
> wait_for_flip_done should already be waiting for DISPLACTX to match our
> crtc's display list, though, right (see vc4_crtc_handle_page_flip())?
> This seems like a no-op to me.

Right, at this stage it's pretty much a no-op. It does make a
difference when bringing-in vc4_hvs_unmask_underrun, because this
vc4_hvs_sync_dlist call comes before it.

When the display lists are not yet synchronized (and differ), an
underrun occurs so it will be reported although it's not related to the
new display list configuration. Waiting for the display lists to sync
before unmasking the interrupt prevents that.

I think waiting for the page flip to unmask the underrun interrupt
would be too late because the HVS should have finished processing the
new dlist by then (but maybe I'm mistaken about that), so we would miss
the underrun indication.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2867 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3 1/4] drm/vc4: Wait for display list synchronization when completing commit

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 4942 bytes --]

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

> Hi,
>
> On Wed, 2019-01-23 at 10:34 -0800, Eric Anholt wrote:
>> Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:
>> 
>> > During an atomic commit, the HVS is configured with a display list
>> > for the channel matching the associated CRTC. The Pixel Valve (CRTC)
>> > and encoder are also configured for the new setup at that time.
>> > While the Pixel Valve and encoder are reconfigured synchronously, the
>> > HVS is only reconfigured after the display list address (DISPLIST) has
>> > been updated to the current display list address (DISPLACTX), which is
>> > the responsibility of the hardware.
>> > 
>> > The time frame during which the HVS is still running on its previous
>> > configuration but the CRTC and encoder have been reconfigured already
>> > can lead to a number of synchronization issues. They will eventually
>> > cause errors reported on the FIFOs, such as underruns.
>> > 
>> > With underrun detection enabled (from Boris Brezillon's series), this
>> > leads to unreliable underrun detection with random false positives.
>> > 
>> > To ensure a coherent state, wait for each enabled channel of the HVS
>> > to synchronize its current display list address. This fixes the issue
>> > of random underrun reporting on commits.
>> > 
>> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
>> > ---
>> >  drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>> >  drivers/gpu/drm/vc4/vc4_hvs.c  | 17 +++++++++++++++++
>> >  drivers/gpu/drm/vc4/vc4_kms.c  |  2 ++
>> >  drivers/gpu/drm/vc4/vc4_regs.h |  2 ++
>> >  4 files changed, 22 insertions(+)
>> > 
>> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
>> > index c24b078f0593..955f157f5ad0 100644
>> > --- a/drivers/gpu/drm/vc4/vc4_drv.h
>> > +++ b/drivers/gpu/drm/vc4/vc4_drv.h
>> > @@ -772,6 +772,7 @@ void vc4_irq_reset(struct drm_device *dev);
>> >  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);
>> >  
>> >  /* 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 5d8c749c9749..1ba60b8e0c2d 100644
>> > --- a/drivers/gpu/drm/vc4/vc4_hvs.c
>> > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
>> > @@ -166,6 +166,23 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
>> >  	return 0;
>> >  }
>> >  
>> > +void vc4_hvs_sync_dlist(struct drm_device *dev)
>> > +{
>> > +	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> > +	unsigned int i;
>> > +	int ret;
>> > +
>> > +	for (i = 0; i < SCALER_CHANNELS_COUNT; i++) {
>> > +		if (!(HVS_READ(SCALER_DISPCTRLX(i)) & SCALER_DISPCTRLX_ENABLE))
>> > +			continue;
>> > +
>> > +		ret = wait_for(HVS_READ(SCALER_DISPLACTX(i)) ==
>> > +			       HVS_READ(SCALER_DISPLISTX(i)), 1000);
>> > +		WARN(ret, "Timeout waiting for channel %d display list sync\n",
>> > +		     i);
>> > +	}
>> > +}
>> > +
>> >  static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>> >  {
>> >  	struct platform_device *pdev = to_platform_device(dev);
>> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
>> > index 0490edb192a1..2d66a2b57a91 100644
>> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
>> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
>> > @@ -155,6 +155,8 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>> >  
>> >  	drm_atomic_helper_commit_hw_done(state);
>> >  
>> > +	vc4_hvs_sync_dlist(dev);
>> > +
>> >  	drm_atomic_helper_wait_for_flip_done(dev, state);
>> 
>> wait_for_flip_done should already be waiting for DISPLACTX to match our
>> crtc's display list, though, right (see vc4_crtc_handle_page_flip())?
>> This seems like a no-op to me.
>
> Right, at this stage it's pretty much a no-op. It does make a
> difference when bringing-in vc4_hvs_unmask_underrun, because this
> vc4_hvs_sync_dlist call comes before it.
>
> When the display lists are not yet synchronized (and differ), an
> underrun occurs so it will be reported although it's not related to the
> new display list configuration. Waiting for the display lists to sync
> before unmasking the interrupt prevents that.
>
> I think waiting for the page flip to unmask the underrun interrupt
> would be too late because the HVS should have finished processing the
> new dlist by then (but maybe I'm mistaken about that), so we would miss
> the underrun indication.

For load tracking I'm mainly concerned about making sure that we don't
have long-standing underruns -- if the first frame after a modeset is a
little wonky, you probably won't notice it.

Getting a clean modeset sequence would be really nice to have,
just... don't block on that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]