public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/panel/boe-tv101wum-nl6: set MIPI_DSI_MODE_LPM after sending panel disable cmds
@ 2026-04-21 15:31 Brady Norander
  2026-04-22 22:22 ` Claude review: " Claude Code Review Bot
  2026-04-22 22:22 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Brady Norander @ 2026-04-21 15:31 UTC (permalink / raw)
  To: Neil Armstrong, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Brady Norander

A recent change to Mediatek drm driver enabled HS mode support. With that
change, my MT8183-based krane sku176 chromebook display's colors would look darker
and desaturated after turning the display off and back on. Looking at other panel
drivers, it seems common to enable HS mode to send the disable commands and to
disable HS mode afterwards. However, the boe-tv101wum-nl6 driver would never
disable HS mode, leading to this issue. Update the behavior of boe_panel_disable
to match those other panel drivers. As a side note, I did also test removing the
line which enables HS mode during panel_disable. This worked fine for my device,
but just in case that doesn't work for other devices, I chose to keep that and
instead disable HS mode after sending the panel disable commands.

Signed-off-by: Brady Norander <brady.norander@mainlining.org>

diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
index d5fe105bdbdd..f69b5bd776c0 100644
--- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
+++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
@@ -1326,6 +1326,8 @@ static int boe_panel_disable(struct drm_panel *panel)
 
 	mipi_dsi_msleep(&ctx, 150);
 
+	boe->dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
 	return ctx.accum_err;
 }
 
-- 
2.52.0

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: drm/panel/boe-tv101wum-nl6: set MIPI_DSI_MODE_LPM after sending panel disable cmds
  2026-04-21 15:31 [PATCH] drm/panel/boe-tv101wum-nl6: set MIPI_DSI_MODE_LPM after sending panel disable cmds Brady Norander
@ 2026-04-22 22:22 ` Claude Code Review Bot
  2026-04-22 22:22 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 22:22 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/panel/boe-tv101wum-nl6: set MIPI_DSI_MODE_LPM after sending panel disable cmds
Author: Brady Norander <brady.norander@mainlining.org>
Patches: 1
Reviewed: 2026-04-23T08:22:29.769041

---

This is a single-patch fix for a real visual regression on MT8183 krane Chromebook hardware. The commit message clearly explains the root cause: a recent Mediatek DRM change enabled HS mode support, and the boe-tv101wum-nl6 panel driver was clearing `MIPI_DSI_MODE_LPM` in `boe_panel_disable()` to send disable commands in HS mode but never restoring it. This left the DSI link in HS mode permanently after the first disable cycle, causing color issues on subsequent enable.

The fix is minimal, correct, and follows established patterns in other panel drivers. The code change is a single line addition that restores `MIPI_DSI_MODE_LPM` after the disable commands have been sent and the sleep delay has elapsed. This is a clean, low-risk bugfix.

**Verdict: Looks good. This should be straightforward to accept.**

One minor nit: the patch lacks a `Fixes:` tag. It would be helpful to include one referencing the Mediatek DRM commit that exposed this issue, or at least the original commit that introduced the asymmetric LPM clearing in `boe_panel_disable()`. This would help stable backport triage.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: drm/panel/boe-tv101wum-nl6: set MIPI_DSI_MODE_LPM after sending panel disable cmds
  2026-04-21 15:31 [PATCH] drm/panel/boe-tv101wum-nl6: set MIPI_DSI_MODE_LPM after sending panel disable cmds Brady Norander
  2026-04-22 22:22 ` Claude review: " Claude Code Review Bot
@ 2026-04-22 22:22 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-04-22 22:22 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: Good.** The change is straightforward and correct:

```c
boe->dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;   // line 1322: clear LPM (switch to HS)

mipi_dsi_dcs_set_display_off_multi(&ctx);        // line 1324: send disable in HS
mipi_dsi_dcs_enter_sleep_mode_multi(&ctx);        // line 1325: send sleep in HS

mipi_dsi_msleep(&ctx, 150);                      // line 1327: wait for panel

boe->dsi->mode_flags |= MIPI_DSI_MODE_LPM;      // line 1329: restore LPM  <-- NEW
```

The flag is restored with `|=` rather than `=`, which is correct — it only sets the `MIPI_DSI_MODE_LPM` bit without disturbing other flags. Since only `MIPI_DSI_MODE_LPM` was cleared earlier (line 1322), this perfectly reverses the earlier modification.

**Pattern consistency:** The clear-before/restore-after pattern is used in other panel drivers (e.g., `panel-asus-z00t-tm5p5-n35596.c` lines 174/180). Interestingly, some other panel drivers like `panel-ebbg-ft8719.c` and `panel-boe-td4320.c` do NOT restore LPM after their off sequence, so this is not universally consistent — but restoring it is clearly the safer approach and matches the initial state set at probe time (`dsi->mode_flags = desc->mode_flags` at line 1758, where all descriptors include `MIPI_DSI_MODE_LPM`).

**Placement:** Restoring LPM after the `mipi_dsi_msleep()` is the right choice — all DSI commands have been fully sent before the flag is changed back.

**Impact scope:** This driver covers multiple panel variants (BOE TV101WUM-NL6, TV105WUM-NW0, AUO KD101N80-45NA, etc.). The `boe_panel_disable()` function is shared across all of them, so this fix applies to all. The submitter tested on krane sku176; other devices should also benefit since the asymmetry was a latent bug.

**Nits:**
- **Missing `Fixes:` tag.** This would be very helpful for stable tree backport consideration. It should reference either the Mediatek HS mode commit that exposed this, or the original commit that introduced the `mode_flags &= ~MIPI_DSI_MODE_LPM` without a corresponding restore.
- **Commit subject line** is a bit long (76 chars after the prefix). Minor — well within tolerance.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-04-22 22:22 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-21 15:31 [PATCH] drm/panel/boe-tv101wum-nl6: set MIPI_DSI_MODE_LPM after sending panel disable cmds Brady Norander
2026-04-22 22:22 ` Claude review: " Claude Code Review Bot
2026-04-22 22:22 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox