[PATCH v4 0/5] drm/vc4: dsi: Make failed reads visible and recoverable

[PATCH v4 0/5] drm/vc4: dsi: Make failed reads visible and recoverable

[Henry Zhang]

DSI reads on the Raspberry Pi 4 (vc4, DSI1) can fail in two ways that
the caller cannot handle:

- A peripheral that detected an error answers a read with an
  Acknowledge and Error Report. vc4 copies its two bytes into the
  caller's buffer and returns success, so the error bits look like
  register contents (patch 2). An HSTX timeout can be reported as a
  success as well (patch 1).

- After a read whose bus turnaround times out, every following DSI
  transfer times out as well, until the display is switched off and on
  (patch 5).

Patch 3 rate-limits the error interrupt messages, which patch 5 would
otherwise turn into an interrupt storm. Patch 4 serializes host
transfers against each other and against the bridge callbacks, so that
the recovery cannot reset the block under a transfer or while it is
being configured.

These patches do not prevent the timeouts. With video running, vc4
does not keep commands within vertical blanking, so a command can still
collide with active video and time out. That is a separate issue; this
series makes such failures visible to the caller and recoverable
without a modeset.

Tested on a Raspberry Pi 4 (DSI1, 2-lane video mode panel) with the
same changes backported to the Raspberry Pi 6.18 kernel:

- 5 minutes of generic long writes from 4 extra threads with video
  running: 45000 writes, 582 timed out; each timeout was followed by a
  reset and restore, and the following writes went through. The DSI
  interrupt rate stayed at 147/s.
- 50 display off/on cycles with 2 writer threads running (the case that
  locked up v2): no lock-up, no warning.
- 250 display off/on cycles without extra traffic: 245 panel inits
  succeeded; 5 failed because the data lanes did not reach LP-11 after
  power-up, which this series does not address.

Reads were tested with these patches plus a change, not part of this
series, that stops video while a command is sent: 2000 of 2000 reads
returned the expected data.

Compile-tested on drm-misc-next (arm64 defconfig, DRM_VC4=m, W=1), each
patch on its own.

Changes in v4:
- Patch 1: new, set the transfer result before completing the transfer
  on an HSTX timeout (found while reviewing v3).
- Patch 2: add Dave's Reviewed-by (unchanged from v2 1/3).
- Patch 5: keep the interrupt handler out with disable_irq() around the
  reset instead of synchronize_irq() before it: an error interrupt could
  still run during the reset (Sashiko review).
- Link to v3: https://lore.kernel.org/r/20261026191618.9430-1-henryzhang62@gmail.com

Changes in v3:
- v2 is not safe; please don't apply it. Its recovery keeps the block
  usable after a timeout, and a peripheral that leaves the data lanes in
  a bad state then raises the "data lane 0 sequence" error interrupt
  tens of thousands of times per second. Printing each one starved a
  Raspberry Pi 4 until it had to be reset.
- Patch 3 (new in v3): rate-limit the error interrupt messages.
- Patch 4 (was v2 3/3): moved before the recovery, and also taken in the
  bridge pre_enable, enable, disable and post_disable callbacks.
- Patch 5 (was v2 2/3): only recover while the block is enabled, and mask
  the interrupts while the block and its AFE are in reset.
- Link to v2: https://lore.kernel.org/r/20261019174930.7237-1-henryzhang62@gmail.com

Changes in v2:
- Patch 2: restore the registers in the same order as pre_enable, and
  call synchronize_irq() before the reset (Sashiko review)
- Patch 3: new, serialize host transfers (Sashiko review)
- Link to v1: https://lore.kernel.org/r/20261012190249.3167-1-henryzhang62@gmail.com

Henry Zhang (5):
  drm/vc4: dsi: Set the transfer result before completing the transfer
  drm/vc4: dsi: Don't return error reports as read data
  drm/vc4: dsi: Rate-limit error interrupt messages
  drm/vc4: dsi: Serialize host transfers
  drm/vc4: dsi: Recover from a timed-out transfer

 drivers/gpu/drm/vc4/vc4_dsi.c | 180 ++++++++++++++++++++++++++++++++--
 1 file changed, 170 insertions(+), 10 deletions(-)


base-commit: 650385a0fd0ea6475a17d39a20ec90470c7a4c67
-- 
2.47.3

[PATCH v4 1/5] drm/vc4: dsi: Set the transfer result before completing the transfer

[Henry Zhang]

On an HSTX timeout the interrupt handler completes the transfer and
only then sets xfer_result to -ETIMEDOUT. The waiter, which may run on
another CPU, can wake up and read xfer_result in between, still 0 from
the start of the transfer, and report the timed-out transfer as a
success.

Set xfer_result first. complete() orders the store before the wake-up.

Fixes: 4078f5757144 ("drm/vc4: Add DSI driver")
Signed-off-by: Henry Zhang <henryzhang62@gmail.com>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 5ed3c35ee..aa2b46fb0 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1526,8 +1526,8 @@ static irqreturn_t vc4_dsi_irq_handler(int irq, void *data)
 		complete(&dsi->xfer_completion);
 		ret = IRQ_HANDLED;
 	} else if (stat & DSI_PORT_BIT(INT_HSTX_TO)) {
-		complete(&dsi->xfer_completion);
 		dsi->xfer_result = -ETIMEDOUT;
+		complete(&dsi->xfer_completion);
 		ret = IRQ_HANDLED;
 	}
 
-- 
2.47.3

[PATCH v4 2/5] drm/vc4: dsi: Don't return error reports as read data

[Henry Zhang]

When a DSI peripheral detects an error, for example contention during
the bus turnaround, it may answer a read request with an Acknowledge
and Error Report short packet instead of the read response.

vc4_dsi_host_transfer() does not check the type of the received packet
(there is a FINISHME for it). It copies the two bytes of the error
report into the caller's buffer and returns success, so the caller
gets the error bits as if they were register contents. Packets
received with a checksum error, an uncorrectable ECC error or
incomplete are passed on the same way.

Return -EIO in these cases so that callers can tell a failed read from
data, and retry if they want to.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Henry Zhang <henryzhang62@gmail.com>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index aa2b46fb0..1eea60aec 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -39,6 +39,8 @@
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
 
+#include <video/mipi_display.h>
+
 #include "vc4_drv.h"
 #include "vc4_regs.h"
 
@@ -1299,6 +1301,24 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
 		u32 rxpkt1h = DSI_PORT_READ(RXPKT1H);
 		u8 *msg_rx = msg->rx_buf;
 
+		/*
+		 * A peripheral that detected an error, e.g. contention during
+		 * the bus turnaround, may answer with an Acknowledge and Error
+		 * Report instead of the read response. Don't hand that, or a
+		 * packet received with errors, to the caller as read data.
+		 * Bits 7:6 of the Data ID are the virtual channel.
+		 */
+		if ((rxpkt1h & (DSI_RXPKT1H_CRC_ERR | DSI_RXPKT1H_DET_ERR |
+				DSI_RXPKT1H_INCOMP_PKT)) ||
+		    (!(rxpkt1h & DSI_RXPKT1H_PKT_TYPE_LONG) &&
+		     (VC4_GET_FIELD(rxpkt1h, DSI_RXPKT1H_DT_LP_CMD) & 0x3f) ==
+		     MIPI_DSI_RX_ACKNOWLEDGE_AND_ERROR_REPORT)) {
+			drm_dbg_driver(drm, "DSI read rejected, RXPKT1H 0x%08x\n",
+				       rxpkt1h);
+			ret = -EIO;
+			goto reset_fifo_and_return;
+		}
+
 		if (rxpkt1h & DSI_RXPKT1H_PKT_TYPE_LONG) {
 			u32 rxlen = VC4_GET_FIELD(rxpkt1h,
 						  DSI_RXPKT1H_BC_PARAM);
@@ -1313,8 +1333,6 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
 			for (i = 0; i < msg->rx_len; i++)
 				msg_rx[i] = DSI_READ(DSI1_RXPKT_FIFO);
 		} else {
-			/* FINISHME: Handle AWER */
-
 			msg_rx[0] = VC4_GET_FIELD(rxpkt1h,
 						  DSI_RXPKT1H_SHORT_0);
 			if (msg->rx_len > 1) {
-- 
2.47.3

[PATCH v4 3/5] drm/vc4: dsi: Rate-limit error interrupt messages

[Henry Zhang]

The error interrupts (contention, sequence and timeout errors) are
always enabled and each one is logged with drm_err() from the interrupt
handler. When a peripheral leaves the data lanes in a bad state, the
same error is raised again as soon as it is acknowledged. On a
Raspberry Pi 4 a "data lane 0 sequence error" was seen at more than
50000 interrupts per second; printing every one of them, mostly to a
serial console, starved the rest of the system to the point that the
SD card driver timed out and the board had to be reset.

Use drm_err_ratelimited() so that the errors stay visible without
taking the system down.

Signed-off-by: Henry Zhang <henryzhang62@gmail.com>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 1eea60aec..be9caaf50 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1487,8 +1487,8 @@ static void dsi_handle_error(struct vc4_dsi *dsi,
 	if (!(stat & bit))
 		return;
 
-	drm_err(dsi->bridge.dev, "DSI%d: %s error\n", dsi->variant->port,
-		type);
+	drm_err_ratelimited(dsi->bridge.dev, "DSI%d: %s error\n",
+			    dsi->variant->port, type);
 	*ret = IRQ_HANDLED;
 }
 
-- 
2.47.3

[PATCH v4 4/5] drm/vc4: dsi: Serialize host transfers

[Henry Zhang]

vc4_dsi_host_transfer() fills the command FIFOs and the TXPKT registers
and waits on a single completion without any locking. Two callers
sending at the same time, for example a panel driver and a backlight
update, can overwrite each other's packets. Nothing keeps a transfer
from running while the bridge callbacks reprogram the block either,
for example while pre_enable sets up the PHY timings.

Add a mutex and hold it for the whole host transfer and in the bridge
pre_enable, enable, disable and post_disable callbacks. None of these
callbacks calls into the DSI device, so the lock is never taken
recursively.

Signed-off-by: Henry Zhang <henryzhang62@gmail.com>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index be9caaf50..3ee6e8e86 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -25,6 +25,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
 #include <linux/io.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
@@ -595,6 +596,11 @@ struct vc4_dsi {
 	 */
 	struct clk *pixel_clock;
 
+	/*
+	 * Serializes host transfers against each other and against the
+	 * bridge enable/disable paths, which reprogram the block.
+	 */
+	struct mutex transfer_lock;
 	struct completion xfer_completion;
 	int xfer_result;
 
@@ -805,6 +811,8 @@ static void vc4_dsi_bridge_disable(struct drm_bridge *bridge,
 	struct vc4_dsi *dsi = bridge_to_vc4_dsi(bridge);
 	u32 disp0_ctrl;
 
+	guard(mutex)(&dsi->transfer_lock);
+
 	disp0_ctrl = DSI_PORT_READ(DISP0_CTRL);
 	disp0_ctrl &= ~DSI_DISP0_ENABLE;
 	DSI_PORT_WRITE(DISP0_CTRL, disp0_ctrl);
@@ -816,6 +824,8 @@ static void vc4_dsi_bridge_post_disable(struct drm_bridge *bridge,
 	struct vc4_dsi *dsi = bridge_to_vc4_dsi(bridge);
 	struct device *dev = &dsi->pdev->dev;
 
+	guard(mutex)(&dsi->transfer_lock);
+
 	clk_disable_unprepare(dsi->pll_phy_clock);
 	clk_disable_unprepare(dsi->escape_clock);
 	clk_disable_unprepare(dsi->pixel_clock);
@@ -891,6 +901,8 @@ static void vc4_dsi_bridge_pre_enable(struct drm_bridge *bridge,
 	unsigned long phy_clock;
 	int ret;
 
+	guard(mutex)(&dsi->transfer_lock);
+
 	ret = pm_runtime_resume_and_get(dev);
 	if (ret) {
 		drm_err(bridge->dev, "Failed to runtime PM enable on DSI%d\n", dsi->variant->port);
@@ -1148,6 +1160,8 @@ static void vc4_dsi_bridge_enable(struct drm_bridge *bridge,
 	bool debug_dump_regs = false;
 	u32 disp0_ctrl;
 
+	guard(mutex)(&dsi->transfer_lock);
+
 	disp0_ctrl = DSI_PORT_READ(DISP0_CTRL);
 	disp0_ctrl |= DSI_DISP0_ENABLE;
 	DSI_PORT_WRITE(DISP0_CTRL, disp0_ctrl);
@@ -1181,6 +1195,8 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
 	bool is_long = mipi_dsi_packet_format_is_long(msg->type);
 	u32 cmd_fifo_len = 0, pix_fifo_len = 0;
 
+	guard(mutex)(&dsi->transfer_lock);
+
 	mipi_dsi_create_packet(&packet, msg);
 
 	pkth |= VC4_SET_FIELD(packet.header[0], DSI_TXPKT1H_BC_DT);
@@ -1807,6 +1823,7 @@ static int vc4_dsi_dev_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct vc4_dsi *dsi;
+	int ret;
 
 	dsi = devm_drm_bridge_alloc(&pdev->dev, struct vc4_dsi, bridge, &vc4_dsi_bridge_funcs);
 	if (IS_ERR(dsi))
@@ -1814,6 +1831,9 @@ static int vc4_dsi_dev_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, dsi);
 
 	dsi->pdev = pdev;
+	ret = devm_mutex_init(dev, &dsi->transfer_lock);
+	if (ret)
+		return ret;
 #ifdef CONFIG_OF
 	dsi->bridge.of_node = dev->of_node;
 #endif
-- 
2.47.3

[PATCH v4 5/5] drm/vc4: dsi: Recover from a timed-out transfer

[Henry Zhang]

When a transfer times out, vc4_dsi_host_transfer() disables the
command, resets the FIFOs and returns -ETIMEDOUT. That is not always
enough: after an LP receive timeout during a read (the peripheral did
not complete the bus turnaround), every following transfer times out
as well, with no interrupt and STAT reading back 0. Only a modeset,
which goes through post_disable/pre_enable, brings the block back.

Reset the DSI block and its analog front end after a timeout and restore
the configuration programmed in pre_enable, in the same order as
pre_enable: the block is ungated and the AFE released only after the
timings are programmed. Keep the interrupt handler out with
disable_irq() until the block is restored and INT_STAT is cleared, so
that neither a late completion interrupt of the timed-out transfer nor
an error interrupt raised during the reset can complete the next
transfer or run in the middle of the reset. Also mask the interrupts
until the AFE is out of reset: the error interrupts otherwise fire
continuously while it is held in reset. The timed-out message is not
resent, as it may already have been executed by the peripheral; the
error is returned and the caller decides whether to retry.

Only do this while the block is enabled. Between post_disable and
pre_enable the clocks are off and there is nothing to restore; a
transfer then times out as before. The transfer lock keeps the
recovery from running in the middle of pre_enable or post_disable.

This does not prevent the timeouts themselves. With video running,
vc4 does not keep commands within vertical blanking, and transfers can
still time out, sometimes several in a row after a reset. That is a
separate issue.

Signed-off-by: Henry Zhang <henryzhang62@gmail.com>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 136 ++++++++++++++++++++++++++++++++--
 1 file changed, 129 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 3ee6e8e86..6edda74e1 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -601,8 +601,11 @@ struct vc4_dsi {
 	 * bridge enable/disable paths, which reprogram the block.
 	 */
 	struct mutex transfer_lock;
+	/* The block is powered and configured. Protected by transfer_lock. */
+	bool enabled;
 	struct completion xfer_completion;
 	int xfer_result;
+	int irq;
 
 	struct debugfs_regset32 regset;
 };
@@ -826,6 +829,8 @@ static void vc4_dsi_bridge_post_disable(struct drm_bridge *bridge,
 
 	guard(mutex)(&dsi->transfer_lock);
 
+	dsi->enabled = false;
+
 	clk_disable_unprepare(dsi->pll_phy_clock);
 	clk_disable_unprepare(dsi->escape_clock);
 	clk_disable_unprepare(dsi->pixel_clock);
@@ -1151,6 +1156,8 @@ static void vc4_dsi_bridge_pre_enable(struct drm_bridge *bridge,
 		DSI_PORT_WRITE(DISP0_CTRL,
 			       DSI_DISP0_COMMAND_MODE);
 	}
+
+	dsi->enabled = true;
 }
 
 static void vc4_dsi_bridge_enable(struct drm_bridge *bridge,
@@ -1184,10 +1191,9 @@ static int vc4_dsi_bridge_attach(struct drm_bridge *bridge,
 				 &dsi->bridge, flags);
 }
 
-static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
-				     const struct mipi_dsi_msg *msg)
+static ssize_t vc4_dsi_transfer(struct vc4_dsi *dsi,
+				const struct mipi_dsi_msg *msg)
 {
-	struct vc4_dsi *dsi = host_to_dsi(host);
 	struct drm_device *drm = dsi->bridge.dev;
 	struct mipi_dsi_packet packet;
 	u32 pkth = 0, pktc = 0;
@@ -1195,8 +1201,6 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
 	bool is_long = mipi_dsi_packet_format_is_long(msg->type);
 	u32 cmd_fifo_len = 0, pix_fifo_len = 0;
 
-	guard(mutex)(&dsi->transfer_lock);
-
 	mipi_dsi_create_packet(&packet, msg);
 
 	pkth |= VC4_SET_FIELD(packet.header[0], DSI_TXPKT1H_BC_DT);
@@ -1374,6 +1378,120 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
 	return ret;
 }
 
+/*
+ * Reset the DSI block and its analog front end, then restore the
+ * configuration done in pre_enable. Resetting the FIFOs alone does not
+ * clear the state the block can be left in after a transfer timed out
+ * (e.g. on an LP receive timeout): every following transfer then times
+ * out as well, until the next modeset.
+ */
+static void vc4_dsi_reset_and_restore(struct vc4_dsi *dsi)
+{
+	u32 en = dsi->variant->port ? DSI1_CTRL_EN : DSI0_CTRL_CTRL0;
+	u32 ctrl = DSI_PORT_READ(CTRL);
+	u32 afec0 = DSI_PORT_READ(PHY_AFEC0);
+	u32 afec1 = DSI_PORT_READ(PHY_AFEC1);
+	u32 disp0 = DSI_PORT_READ(DISP0_CTRL) & ~DSI_DISP0_ENABLE;
+	u32 disp1 = DSI_PORT_READ(DISP1_CTRL);
+	u32 hstx_to = DSI_PORT_READ(HSTX_TO_CNT);
+	u32 lprx_to = DSI_PORT_READ(LPRX_TO_CNT);
+	u32 ta_to = DSI_PORT_READ(TA_TO_CNT);
+	u32 pr_to = DSI_PORT_READ(PR_TO_CNT);
+	u32 phyc = DSI_PORT_READ(PHYC);
+	u32 clt0 = DSI_PORT_READ(HS_CLT0);
+	u32 clt1 = DSI_PORT_READ(HS_CLT1);
+	u32 clt2 = DSI_PORT_READ(HS_CLT2);
+	u32 dlt3 = DSI_PORT_READ(HS_DLT3);
+	u32 dlt4 = DSI_PORT_READ(HS_DLT4);
+	u32 dlt5 = DSI_PORT_READ(HS_DLT5);
+	u32 dlt6 = DSI_PORT_READ(HS_DLT6);
+	u32 dlt7 = DSI_PORT_READ(HS_DLT7);
+
+	/*
+	 * Mask the interrupts while the block and its AFE are in reset: the
+	 * error interrupts would otherwise fire continuously.
+	 */
+	DSI_PORT_WRITE(INT_EN, 0);
+	DSI_PORT_WRITE(DISP0_CTRL, disp0);
+	DSI_PORT_WRITE(TXPKT1C, 0);
+	DSI_PORT_WRITE(CTRL, DSI_CTRL_SOFT_RESET_CFG |
+		       DSI_PORT_BIT(CTRL_RESET_FIFOS));
+	DSI_PORT_WRITE(PHY_AFEC0, afec0 | DSI_PORT_BIT(PHY_AFEC0_RESET));
+	/* AFEC reset hold time */
+	mdelay(1);
+
+	/*
+	 * Same order as pre_enable: the block stays gated and the AFE in
+	 * reset until the timings are programmed.
+	 */
+	DSI_PORT_WRITE(CTRL, ctrl & ~(DSI_CTRL_CAL_BYTE | en));
+	DSI_PORT_WRITE(STAT, DSI_PORT_READ(STAT));
+	DSI_PORT_WRITE(PHY_AFEC1, afec1);
+	DSI_PORT_WRITE(HS_CLT0, clt0);
+	DSI_PORT_WRITE(HS_CLT1, clt1);
+	DSI_PORT_WRITE(HS_CLT2, clt2);
+	DSI_PORT_WRITE(HS_DLT3, dlt3);
+	DSI_PORT_WRITE(HS_DLT4, dlt4);
+	DSI_PORT_WRITE(HS_DLT5, dlt5);
+	DSI_PORT_WRITE(HS_DLT6, dlt6);
+	DSI_PORT_WRITE(HS_DLT7, dlt7);
+	DSI_PORT_WRITE(PHYC, phyc);
+	DSI_PORT_WRITE(CTRL, ctrl & ~en);
+	DSI_PORT_WRITE(HSTX_TO_CNT, hstx_to);
+	DSI_PORT_WRITE(LPRX_TO_CNT, lprx_to);
+	DSI_PORT_WRITE(TA_TO_CNT, ta_to);
+	DSI_PORT_WRITE(PR_TO_CNT, pr_to);
+	DSI_PORT_WRITE(DISP1_CTRL, disp1);
+	/* Ungate the block. */
+	DSI_PORT_WRITE(CTRL, ctrl);
+	/* Bring AFE out of reset. */
+	DSI_PORT_WRITE(PHY_AFEC0, afec0 & ~DSI_PORT_BIT(PHY_AFEC0_RESET));
+	DSI_PORT_WRITE(DISP0_CTRL, disp0);
+
+	DSI_PORT_WRITE(INT_STAT, DSI_PORT_READ(INT_STAT));
+	DSI_PORT_WRITE(INT_EN, DSI_PORT_BIT(INTERRUPTS_ALWAYS_ENABLED));
+}
+
+static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
+				     const struct mipi_dsi_msg *msg)
+{
+	struct vc4_dsi *dsi = host_to_dsi(host);
+	u32 disp0_ctrl;
+	ssize_t ret;
+
+	guard(mutex)(&dsi->transfer_lock);
+
+	ret = vc4_dsi_transfer(dsi, msg);
+	/* Only a running block can be reset and restored. */
+	if (ret != -ETIMEDOUT || !dsi->enabled)
+		return ret;
+
+	/*
+	 * Leave the block usable for the next transfer. The message is not
+	 * resent: it may already have reached the peripheral, and only the
+	 * caller knows whether it is safe to repeat.
+	 */
+	disp0_ctrl = DSI_PORT_READ(DISP0_CTRL);
+	dev_warn(&dsi->pdev->dev,
+		 "transfer timed out (stat 0x%08x), resetting DSI\n",
+		 DSI_PORT_READ(STAT));
+	/*
+	 * Keep the interrupt handler out until the block is restored and
+	 * INT_STAT is cleared. The error interrupts are still enabled, and
+	 * a handler running during the reset could complete the next
+	 * transfer, or race with the register writes on DSI1 of the
+	 * BCM2835, where both go through the DMA workaround. disable_irq()
+	 * also waits for a late completion interrupt of the timed-out
+	 * transfer.
+	 */
+	disable_irq(dsi->irq);
+	vc4_dsi_reset_and_restore(dsi);
+	DSI_PORT_WRITE(DISP0_CTRL, disp0_ctrl);
+	enable_irq(dsi->irq);
+
+	return ret;
+}
+
 static const struct component_ops vc4_dsi_ops;
 static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
 			       struct mipi_dsi_device *device)
@@ -1743,14 +1861,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 	/* Clear any existing interrupt state. */
 	DSI_PORT_WRITE(INT_STAT, DSI_PORT_READ(INT_STAT));
 
+	dsi->irq = platform_get_irq(pdev, 0);
+	if (dsi->irq < 0)
+		return dsi->irq;
+
 	if (dsi->reg_dma_mem)
-		ret = devm_request_threaded_irq(dev, platform_get_irq(pdev, 0),
+		ret = devm_request_threaded_irq(dev, dsi->irq,
 						vc4_dsi_irq_defer_to_thread_handler,
 						vc4_dsi_irq_handler,
 						IRQF_ONESHOT,
 						"vc4 dsi", dsi);
 	else
-		ret = devm_request_irq(dev, platform_get_irq(pdev, 0),
+		ret = devm_request_irq(dev, dsi->irq,
 				       vc4_dsi_irq_handler, 0, "vc4 dsi", dsi);
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
-- 
2.47.3

Re: [PATCH v4 5/5] drm/vc4: dsi: Recover from a timed-out transfer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/vc4: dsi: race on reg_dma_mem before disable_irq() in vc4_dsi_host_transfer()
--

commit 748a80df1fc43dd3d1d753f275003873696caaaf
Author: Henry Zhang <henryzhang62@gmail.com>

drm/vc4: dsi: Recover from a timed-out transfer

This commit recovers the VC4 DSI block after a transfer timeout by resetting
the DSI controller and its analog front end, then restoring the hardware
configuration from pre_enable while holding the interrupt disabled.

> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 3ee6e8e86a147..6edda74e1117c 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c

[ ... ]

> @@ -1374,6 +1378,120 @@ static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
>  	return ret;
>  }
>  
> +/*
> + * Reset the DSI block and its analog front end, then restore the
> + * configuration done in pre_enable. Resetting the FIFOs alone does not
> + * clear the state the block can be left in after a transfer timed out
> + * (e.g. on an LP receive timeout): every following transfer then times
> + * out as well, until the next modeset.
> + */
> +static void vc4_dsi_reset_and_restore(struct vc4_dsi *dsi)
> +{
> +	u32 en = dsi->variant->port ? DSI1_CTRL_EN : DSI0_CTRL_CTRL0;
> +	u32 ctrl = DSI_PORT_READ(CTRL);

[ ... ]

> +	/* Ungate the block. */
> +	DSI_PORT_WRITE(CTRL, ctrl);
> +	/* Bring AFE out of reset. */
> +	DSI_PORT_WRITE(PHY_AFEC0, afec0 & ~DSI_PORT_BIT(PHY_AFEC0_RESET));
> +	DSI_PORT_WRITE(DISP0_CTRL, disp0);
> +
> +	DSI_PORT_WRITE(INT_STAT, DSI_PORT_READ(INT_STAT));
> +	DSI_PORT_WRITE(INT_EN, DSI_PORT_BIT(INTERRUPTS_ALWAYS_ENABLED));
> +}
> +
> +static ssize_t vc4_dsi_host_transfer(struct mipi_dsi_host *host,
> +				     const struct mipi_dsi_msg *msg)
> +{
> +	struct vc4_dsi *dsi = host_to_dsi(host);
> +	u32 disp0_ctrl;
> +	ssize_t ret;
> +
> +	guard(mutex)(&dsi->transfer_lock);
> +
> +	ret = vc4_dsi_transfer(dsi, msg);
> +	/* Only a running block can be reset and restored. */
> +	if (ret != -ETIMEDOUT || !dsi->enabled)
> +		return ret;
> +
> +	/*
> +	 * Leave the block usable for the next transfer. The message is not
> +	 * resent: it may already have reached the peripheral, and only the
> +	 * caller knows whether it is safe to repeat.
> +	 */
> +	disp0_ctrl = DSI_PORT_READ(DISP0_CTRL);
> +	dev_warn(&dsi->pdev->dev,
> +		 "transfer timed out (stat 0x%08x), resetting DSI\n",
> +		 DSI_PORT_READ(STAT));
> +	/*
> +	 * Keep the interrupt handler out until the block is restored and
> +	 * INT_STAT is cleared. The error interrupts are still enabled, and
> +	 * a handler running during the reset could complete the next
> +	 * transfer, or race with the register writes on DSI1 of the
> +	 * BCM2835, where both go through the DMA workaround. disable_irq()
> +	 * also waits for a late completion interrupt of the timed-out
> +	 * transfer.
> +	 */
> +	disable_irq(dsi->irq);
> +	vc4_dsi_reset_and_restore(dsi);
> +	DSI_PORT_WRITE(DISP0_CTRL, disp0_ctrl);
> +	enable_irq(dsi->irq);

[Severity: High]
Can a late or error interrupt race on dsi->reg_dma_mem in vc4_dsi_transfer()
before disable_irq(dsi->irq) is reached in vc4_dsi_host_transfer(),
corrupting DSI1_CTRL before vc4_dsi_reset_and_restore() saves and restores
it?

When wait_for_completion_timeout() times out in vc4_dsi_transfer(),
dsi->irq and INT_EN are still enabled while vc4_dsi_transfer() performs
five register writes before returning -ETIMEDOUT:

drivers/gpu/drm/vc4/vc4_dsi.c:vc4_dsi_transfer() {
	...
	DSI_PORT_WRITE(INT_EN, DSI_PORT_BIT(INTERRUPTS_ALWAYS_ENABLED));

	if (ret)
		goto reset_fifo_and_return;
	...
reset_fifo_and_return:
	drm_err(drm, "DSI transfer failed, resetting: %d\n", ret);

	DSI_PORT_WRITE(TXPKT1C, DSI_PORT_READ(TXPKT1C) & ~DSI_TXPKT1C_CMD_EN);
	udelay(1);
	DSI_PORT_WRITE(CTRL,
		       DSI_PORT_READ(CTRL) |
		       DSI_PORT_BIT(CTRL_RESET_FIFOS));

	DSI_PORT_WRITE(TXPKT1C, 0);
	DSI_PORT_WRITE(INT_EN, DSI_PORT_BIT(INTERRUPTS_ALWAYS_ENABLED));
	return ret;
}

On BCM2835 DSI1 (where broken_axi_workaround is enabled), DSI_PORT_WRITE()
calls dsi_dma_workaround_write(), which writes the value into the single
shared buffer *dsi->reg_dma_mem and submits a DMA transfer from
dsi->reg_dma_paddr:

drivers/gpu/drm/vc4/vc4_dsi.c:dsi_dma_workaround_write() {
	...
	*dsi->reg_dma_mem = val;

	tx = chan->device->device_prep_dma_memcpy(chan,
						  dsi->reg_paddr + offset,
						  dsi->reg_dma_paddr,
						  4, 0);
	...
}

If the threaded handler vc4_dsi_irq_handler() runs concurrently (for
example on an LPRX/TA timeout error interrupt or a late completion
interrupt) and calls DSI_PORT_WRITE(INT_STAT, stat), it overwrites
*dsi->reg_dma_mem = stat without any lock.

If that happens during the read-modify-write of CTRL in
reset_fifo_and_return, the DMA transfer can write stat into DSI1_CTRL.
When vc4_dsi_reset_and_restore() then runs under disable_irq(dsi->irq), it
snapshots the corrupted register via u32 ctrl = DSI_PORT_READ(CTRL) and
writes it back at the end of the reset, leaving DSI1_CTRL corrupted (for
example with DSI1_CTRL_EN cleared so the block remains gated).

Should disable_irq(dsi->irq) be called in vc4_dsi_transfer() before the
post-timeout register writes (or before reset_fifo_and_return)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028164539.3471833-1-henryzhang62@gmail.com?part=5

Re: [PATCH v4 5/5] drm/vc4: dsi: Recover from a timed-out transfer

[Henry Zhang]

On Wed, Oct 28, 2026 at 10:19 AM <sashiko-bot@kernel.org> wrote:
....
> [Severity: High]
> Can a late or error interrupt race on dsi->reg_dma_mem in vc4_dsi_transfer()
> before disable_irq(dsi->irq) is reached in vc4_dsi_host_transfer(),
> corrupting DSI1_CTRL before vc4_dsi_reset_and_restore() saves and restores
> it?
>
> When wait_for_completion_timeout() times out in vc4_dsi_transfer(),
> dsi->irq and INT_EN are still enabled while vc4_dsi_transfer() performs
> five register writes before returning -ETIMEDOUT:
>
> drivers/gpu/drm/vc4/vc4_dsi.c:vc4_dsi_transfer() {
>         ...
>         DSI_PORT_WRITE(INT_EN, DSI_PORT_BIT(INTERRUPTS_ALWAYS_ENABLED));
>
>         if (ret)
>                 goto reset_fifo_and_return;
>         ...
> reset_fifo_and_return:
>         drm_err(drm, "DSI transfer failed, resetting: %d\n", ret);
>
>         DSI_PORT_WRITE(TXPKT1C, DSI_PORT_READ(TXPKT1C) & ~DSI_TXPKT1C_CMD_EN);
>         udelay(1);
>         DSI_PORT_WRITE(CTRL,
>                        DSI_PORT_READ(CTRL) |
>                        DSI_PORT_BIT(CTRL_RESET_FIFOS));
>
>         DSI_PORT_WRITE(TXPKT1C, 0);
>         DSI_PORT_WRITE(INT_EN, DSI_PORT_BIT(INTERRUPTS_ALWAYS_ENABLED));
>         return ret;
> }
>
> On BCM2835 DSI1 (where broken_axi_workaround is enabled), DSI_PORT_WRITE()
> calls dsi_dma_workaround_write(), which writes the value into the single
> shared buffer *dsi->reg_dma_mem and submits a DMA transfer from
> dsi->reg_dma_paddr:
>
> drivers/gpu/drm/vc4/vc4_dsi.c:dsi_dma_workaround_write() {
>         ...
>         *dsi->reg_dma_mem = val;
>
>         tx = chan->device->device_prep_dma_memcpy(chan,
>                                                   dsi->reg_paddr + offset,
>                                                   dsi->reg_dma_paddr,
>                                                   4, 0);
>         ...
> }
>
> If the threaded handler vc4_dsi_irq_handler() runs concurrently (for
> example on an LPRX/TA timeout error interrupt or a late completion
> interrupt) and calls DSI_PORT_WRITE(INT_STAT, stat), it overwrites
> *dsi->reg_dma_mem = stat without any lock.
>
> If that happens during the read-modify-write of CTRL in
> reset_fifo_and_return, the DMA transfer can write stat into DSI1_CTRL.
> When vc4_dsi_reset_and_restore() then runs under disable_irq(dsi->irq), it
> snapshots the corrupted register via u32 ctrl = DSI_PORT_READ(CTRL) and
> writes it back at the end of the reset, leaving DSI1_CTRL corrupted (for
> example with DSI1_CTRL_EN cleared so the block remains gated).
>
> Should disable_irq(dsi->irq) be called in vc4_dsi_transfer() before the
> post-timeout register writes (or before reset_fifo_and_return)?

The race is real, but it is not introduced by this patch and not limited
to the timeout path: on BCM2835/6/7 DSI1 every DSI_PORT_WRITE() goes
through the single reg_dma_mem buffer without a lock, so the threaded
handler can corrupt any register write made from process context
(transfer setup, the bridge callbacks, the existing reset_fifo_and_return
path). Moving disable_irq() would only cover this one path.

I'm sending a separate fix that serializes dsi_dma_workaround_write()
with a mutex, so this series stays as it is.

Henry