[PATCH v2] drm/omap: dsi: avoid sending bta sync all the time in writes

[PATCH v2] drm/omap: dsi: avoid sending bta sync all the time in writes

[akemnade]

From: Andreas Kemnade <andreas@kemnade.info>

Some chips need configuration commands to be sent first, before they can
send data. TC358762 for example needs PPI_LPTXTIMECNT configured
and PPI_STARTPPI set to 1 to be able to transmit anything. To be able to
configure such chips, do not send bta sync during writes if no acks are
requested. Instead just wait for the packet to be sent to avoid FIFO
overflows. There might be more to do about acks, but there seem to be
virtually no users of that flag.

This came to light when fiddling with the Epson Moverio BT-200 display
which consists of 2 TC358762 bridges with SPI funneled through
to the unknown display chip. With that patch the bridge can be accessed,
Reading back registers works, when the above-mentioned registers are set.

In Command-Mode update, there was a nop sent, apparently the most
relevant part wos the bta sync to acually force low power mode.

Video mode panel at OMAP4 (BT-200) and video mode at OMAP5 was tested.

Fixes: e70965386353e ("drm/omap: dsi: simplify write function")
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Tested-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> # droid4
---
This has now been also tested with command mode display on droid4.

Qouting: Ivo:  "you may add my Tested-by"
---
Changes in v2:
- fix commandmode update, need bta sync there
- do not wait on short packets
- Link to v1: https://patch.msgid.link/20260528-vm-upstr-v1-1-fb93ef8cbe47@kernel.org
---
 drivers/gpu/drm/omapdrm/dss/dsi.c | 55 +++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
index 27fe7bca9e2c..6391203f6d25 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -2194,28 +2194,42 @@ static int dsi_vc_send_null(struct dsi_data *dsi, int vc, int channel)
 static int dsi_vc_write_common(struct omap_dss_device *dssdev, int vc,
 			       const struct mipi_dsi_msg *msg)
 {
+	DECLARE_COMPLETION_ONSTACK(completion);
 	struct dsi_data *dsi = to_dsi_data(dssdev);
 	int r;
 
 	if (mipi_dsi_packet_format_is_short(msg->type))
-		r = dsi_vc_send_short(dsi, vc, msg);
+		return dsi_vc_send_short(dsi, vc, msg);
 	else
 		r = dsi_vc_send_long(dsi, vc, msg);
 
 	if (r < 0)
 		return r;
 
-	/*
-	 * TODO: we do not always have to do the BTA sync, for example
-	 * we can improve performance by setting the update window
-	 * information without sending BTA sync between the commands.
-	 * In that case we can return early.
-	 */
+	/* wait for IRQ for long packet transmission confirmation */
+	r = dsi_register_isr_vc(dsi, vc, dsi_completion_handler,
+			&completion, DSI_VC_IRQ_PACKET_SENT);
+	if (r)
+		return r;
 
-	r = dsi_vc_send_bta_sync(dssdev, vc);
-	if (r) {
-		DSSERR("bta sync failed\n");
+	if (wait_for_completion_timeout(&completion,
+			msecs_to_jiffies(500)) == 0)
+		r = -EIO;
+
+	dsi_unregister_isr_vc(dsi, vc, dsi_completion_handler,
+			      &completion, DSI_VC_IRQ_PACKET_SENT);
+
+	if (r)
 		return r;
+
+	/* TODO: find out if more needs to be done for MIPI_DIS_MSG_REQ_ACK */
+
+	if (msg->flags & MIPI_DSI_MSG_REQ_ACK) {
+		r = dsi_vc_send_bta_sync(dssdev, vc);
+		if (r) {
+			DSSERR("bta sync failed\n");
+			return r;
+		}
 	}
 
 	/* RX_FIFO_NOT_EMPTY */
@@ -3233,21 +3247,6 @@ static int _dsi_update(struct dsi_data *dsi)
 	return 0;
 }
 
-static int _dsi_send_nop(struct dsi_data *dsi, int vc, int channel)
-{
-	const u8 payload[] = { MIPI_DCS_NOP };
-	const struct mipi_dsi_msg msg = {
-		.channel = channel,
-		.type = MIPI_DSI_DCS_SHORT_WRITE,
-		.tx_len = 1,
-		.tx_buf = payload,
-	};
-
-	WARN_ON(!dsi_bus_is_locked(dsi));
-
-	return _omap_dsi_host_transfer(dsi, vc, &msg);
-}
-
 static int dsi_update_channel(struct omap_dss_device *dssdev, int vc)
 {
 	struct dsi_data *dsi = to_dsi_data(dssdev);
@@ -3268,13 +3267,13 @@ static int dsi_update_channel(struct omap_dss_device *dssdev, int vc)
 	DSSDBG("dsi_update_channel: %d", vc);
 
 	/*
-	 * Send NOP between the frames. If we don't send something here, the
+	 * Transition to LP here. If we don't send something here, the
 	 * updates stop working. This is probably related to DSI spec stating
 	 * that the DSI host should transition to LP at least once per frame.
 	 */
-	r = _dsi_send_nop(dsi, VC_CMD, dsi->dsidev->channel);
+	r = dsi_vc_send_bta_sync(dssdev, vc);
 	if (r < 0) {
-		DSSWARN("failed to send nop between frames: %d\n", r);
+		DSSWARN("failed to send bta sync between frames: %d\n", r);
 		goto err;
 	}
 

---
base-commit: e7ae89a0c97ce2b68b0983cd01eda67cf373517d
change-id: 20260528-vm-upstr-c8e7634ebf56

Best regards,
--  
Andreas Kemnade <akemnade@kernel.org>

Claude review: drm/omap: dsi: avoid sending bta sync all the time in writes

[Claude Code Review Bot]

Overall Series Review

Subject: drm/omap: dsi: avoid sending bta sync all the time in writes
Author: akemnade@kernel.org
Patches: 1
Reviewed: 2026-06-04T16:00:55.211314

---

This is a single-patch fix for the OMAP DSI driver, addressing a real hardware issue with bridge chips (TC358762) that cannot respond to BTA sync requests before they are fully configured. The approach is sound: stop unconditionally sending BTA sync on every write, and instead wait for the PACKET_SENT IRQ for long packets (to avoid FIFO overflows) and skip waiting entirely for short packets. The command-mode update path is fixed to use a bare BTA sync instead of a NOP (which would no longer trigger BTA sync after this change).

The patch is well-motivated, has been tested on both video mode (OMAP4 BT-200) and command mode (droid4), and carries a Tested-by. The overall design is correct, though there are a few issues worth noting.

**Verdict:** Mostly good. A couple of minor issues, one potential race concern.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/omap: dsi: avoid sending bta sync all the time in writes

[Claude Code Review Bot]

Patch Review

**Positive aspects:**

- The rationale is clear and the problem is well-explained: TC358762 needs configuration before it can transmit, so BTA sync during early writes fails.
- Waiting for `DSI_VC_IRQ_PACKET_SENT` on long packets is the right approach to prevent FIFO overflows without requiring a round-trip BTA.
- The `_dsi_send_nop` removal is correct — it was only called from `dsi_update_channel` and its real purpose was the BTA sync side-effect. Replacing it with a direct `dsi_vc_send_bta_sync()` is cleaner and more explicit.
- The conditional BTA based on `MIPI_DSI_MSG_REQ_ACK` is the right abstraction.

**Issues:**

1. **Typo in TODO comment** (line 2225 of patched file):
   ```c
   /* TODO: find out if more needs to be done for MIPI_DIS_MSG_REQ_ACK */
   ```
   Should be `MIPI_DSI_MSG_REQ_ACK` (not `MIPI_DIS_MSG_REQ_ACK`). This is a transposition of "DSI" → "DIS".

2. **Race between PACKET_SENT IRQ and ISR registration** (lines 2204–2213): `dsi_vc_send_long()` is called *before* `dsi_register_isr_vc()`. If the long packet completes extremely quickly (or the hardware is fast), the PACKET_SENT IRQ could fire before the ISR is registered, and the completion would never be signalled — resulting in a 500ms timeout and spurious `-EIO`. The existing pattern in `dsi_if_enable` (and the BTA sync code in `dsi_vc_send_bta_sync`) registers the ISR *before* triggering the hardware action. The correct order should be:
   ```c
   r = dsi_register_isr_vc(dsi, vc, dsi_completion_handler,
                            &completion, DSI_VC_IRQ_PACKET_SENT);
   if (r)
       return r;

   r = dsi_vc_send_long(dsi, vc, msg);
   if (r < 0) {
       dsi_unregister_isr_vc(dsi, vc, dsi_completion_handler,
                             &completion, DSI_VC_IRQ_PACKET_SENT);
       return r;
   }
   ```
   This is the same register-before-trigger pattern used in `dsi_vc_send_bta_sync()` at lines 2026–2036.

3. **Short packet early return skips RX_FIFO_NOT_EMPTY check** (line 2202): The early `return` for short packets bypasses the `RX_FIFO_NOT_EMPTY` check at lines 2236–2240. The old code ran that check for all writes. This is likely intentional (short packets without BTA shouldn't produce RX data), but it's worth confirming this is safe — if some panel/bridge does produce unexpected data in the RX FIFO after a short write, it would go undetected and could confuse subsequent reads.

4. **Minor: commit message typos** — "wos" should be "was", and "acually" should be "actually" in line 99 of the mbox:
   ```
   the most relevant part wos the bta sync to acually force low power mode.
   ```

---
Generated by Claude Code Patch Reviewer