public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v4] drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down
@ 2026-05-12 10:31 Frank Zhang
  2026-05-16  3:45 ` Claude review: " Claude Code Review Bot
  2026-05-16  3:45 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Frank Zhang @ 2026-05-12 10:31 UTC (permalink / raw)
  To: andrzej.hajda, neil.armstrong, rfoss, maarten.lankhorst, mripard,
	tzimmermann, airlied, simona
  Cc: detlev.casanova, cristian.ciocaltea, daniels, dmitry.baryshkov,
	heiko, Laurent.pinchart, jonas, jernej.skrabec, dri-devel,
	linux-kernel, stable

The following panic was observed during system reboot:

Kernel panic - not syncing: Asynchronous SError Interrupt
CPU: 6 UID: 1000 PID: 2348 Comm: pipewire ... 7.0.5+ #4 PREEMPT(full)
Call trace:
 ...
 regmap_update_bits_base+0x70/0xa8
 dw_hdmi_qp_bridge_clear_audio_infoframe+0x3c/0x58 [dw_hdmi_qp]
 drm_bridge_connector_clear_audio_infoframe+0x2c/0x48 [drm_display_helper]
 ...
 dw_hdmi_qp_audio_disable+0x28/0xa8 [dw_hdmi_qp]
 drm_bridge_connector_audio_shutdown+0x38/0x68 [drm_display_helper]
 drm_connector_hdmi_audio_shutdown+0x28/0x40 [drm_display_helper]
 hdmi_codec_shutdown+0x60/0x90 [snd_soc_hdmi_codec]
 ...
 snd_pcm_release_substream+0xcc/0x120 [snd_pcm]
 snd_pcm_release+0x4c/0xc0 [snd_pcm]
 ...

The root cause is pipewire tries to close the HDMI audio device after
atomic_disable(), which sets tmds_char_rate to 0 and disables the PHY.

In this case, dw_hdmi_qp_audio_disable() will call
dw_hdmi_qp_bridge_clear_audio_infoframe(), accessing register without
checking tmds_char_rate.

Add a tmds_char_rate guard in dw_hdmi_qp_bridge_clear_audio_infoframe().
Decouple write_audio_infoframe from clear_audio_infoframe to avoid the
redundant check in the write path.
Add PKTSCHED_AMD_TX_EN to the clear mask to keep the enable/disable
balance.

Fixes: fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")
Cc: stable@vger.kernel.org
Signed-off-by: Frank Zhang <rmxpzlb@gmail.com>

---
Changes in v2:
- Move drm_atomic_helper_connector_hdmi_clear_audio_infoframe() inside
  the if (hdmi->tmds_char_rate) of dw_hdmi_qp_audio_disable().
- Link to v1: https://lore.kernel.org/all/20260416093150.13853-1-rmxpzlb@gmail.com/

Changes in v3:
- Add a tmds_char_rate guard in clear_audio_infoframe path.
- Decouple write_audio_infoframe from clear_audio_infoframe.
- Balance the PKTSCHED_AMD_TX_EN bit enable/disable.
- Link to v2: https://lore.kernel.org/all/20260418101936.7731-1-rmxpzlb@gmail.com/

Changes in v4:
- Update panic stack on 7.0.5
- Link to v3: https://lore.kernel.org/all/20260423081514.15444-1-rmxpzlb@gmail.com/
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index d649a1cf07f5..1c18f8650fcd 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -886,11 +886,11 @@ static int dw_hdmi_qp_bridge_clear_audio_infoframe(struct drm_bridge *bridge)
 {
 	struct dw_hdmi_qp *hdmi = bridge->driver_private;
 
-	dw_hdmi_qp_mod(hdmi, 0,
-		       PKTSCHED_ACR_TX_EN |
-		       PKTSCHED_AUDS_TX_EN |
-		       PKTSCHED_AUDI_TX_EN,
-		       PKTSCHED_PKT_EN);
+	if (hdmi->tmds_char_rate)
+		dw_hdmi_qp_mod(hdmi, 0,
+			       PKTSCHED_ACR_TX_EN | PKTSCHED_AMD_TX_EN |
+			       PKTSCHED_AUDS_TX_EN | PKTSCHED_AUDI_TX_EN,
+			       PKTSCHED_PKT_EN);
 
 	return 0;
 }
@@ -989,7 +989,10 @@ static int dw_hdmi_qp_bridge_write_audio_infoframe(struct drm_bridge *bridge,
 {
 	struct dw_hdmi_qp *hdmi = bridge->driver_private;
 
-	dw_hdmi_qp_bridge_clear_audio_infoframe(bridge);
+	dw_hdmi_qp_mod(hdmi, 0,
+		       PKTSCHED_ACR_TX_EN | PKTSCHED_AMD_TX_EN |
+		       PKTSCHED_AUDS_TX_EN | PKTSCHED_AUDI_TX_EN,
+		       PKTSCHED_PKT_EN);
 
 	/*
 	 * AUDI_CONTENTS0: { RSV, HB2, HB1, RSV }
-- 
2.54.0


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

* Claude review: drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down
  2026-05-12 10:31 [PATCH v4] drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down Frank Zhang
@ 2026-05-16  3:45 ` Claude Code Review Bot
  2026-05-16  3:45 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  3:45 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down
Author: Frank Zhang <rmxpzlb@gmail.com>
Patches: 1
Reviewed: 2026-05-16T13:45:35.779160

---

This is a single-patch bug fix for a kernel panic (Asynchronous SError Interrupt) triggered when pipewire closes an HDMI audio device after the display pipeline has already been disabled. The race is: `atomic_disable()` sets `tmds_char_rate = 0` and powers down the PHY, then pipewire's PCM release path calls `dw_hdmi_qp_bridge_clear_audio_infoframe()` which performs a regmap write to hardware that is no longer clocked, causing an SError.

The fix is reasonable and addresses a real crash. The approach of guarding `clear_audio_infoframe` with a `tmds_char_rate` check follows the existing pattern already used in `dw_hdmi_qp_audio_enable()` (line 472) and `dw_hdmi_qp_audio_disable()` (line 531). The decoupling of write from clear avoids adding the guard in the write path where it would be unnecessary (write is only called when the PHY is active).

One concern: the other `clear_*_infoframe` functions (`clear_avi_infoframe`, `clear_hdmi_infoframe`, `clear_hdr_drm_infoframe`, `clear_spd_infoframe`) do **not** have the same `tmds_char_rate` guard. If any of those can also be called after PHY teardown, they would have the same bug. This patch only fixes the audio path because that's where the crash was observed, but it may be worth considering whether the same race exists for other infoframes. That said, for a targeted stable-backport fix, limiting scope to the observed crash is pragmatic.

**Verdict: The patch looks correct for the stated problem. Recommend accepting with minor comments.**

---

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down
  2026-05-12 10:31 [PATCH v4] drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down Frank Zhang
  2026-05-16  3:45 ` Claude review: " Claude Code Review Bot
@ 2026-05-16  3:45 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  3:45 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Commit message:** Well-written. Includes the panic trace, clear root cause analysis, and describes the three logical changes (guard, decouple, balance AMD bit). The Fixes tag and Cc: stable are appropriate.

**Change 1 — Guard in `clear_audio_infoframe` (lines 889-893):**

```c
if (hdmi->tmds_char_rate)
	dw_hdmi_qp_mod(hdmi, 0,
		       PKTSCHED_ACR_TX_EN | PKTSCHED_AMD_TX_EN |
		       PKTSCHED_AUDS_TX_EN | PKTSCHED_AUDI_TX_EN,
		       PKTSCHED_PKT_EN);
```

This is the core fix. When `tmds_char_rate == 0`, the PHY is down and register access is unsafe, so we skip it. The function still returns 0, which is fine — the framework doesn't act on error returns from `clear_infoframe` callbacks, and there's nothing useful to do when the PHY is already off.

No locking concern: `tmds_char_rate` is set to 0 in `atomic_disable` and the audio shutdown path is the only late caller, so reading it without a lock is acceptable for this guard-check pattern (a stale non-zero read would just do a harmless register write before the PHY goes down).

**Change 2 — Decouple `write_audio_infoframe` from `clear_audio_infoframe` (lines 992-995):**

```c
dw_hdmi_qp_mod(hdmi, 0,
	       PKTSCHED_ACR_TX_EN | PKTSCHED_AMD_TX_EN |
	       PKTSCHED_AUDS_TX_EN | PKTSCHED_AUDI_TX_EN,
	       PKTSCHED_PKT_EN);
```

Previously `write_audio_infoframe` called `clear_audio_infoframe` to disable packets before re-programming them. Now it inlines the `dw_hdmi_qp_mod` call directly, avoiding the `tmds_char_rate` guard that would be incorrect in the write path (where the PHY **is** up). This is the right design — the write path is only called when audio is being configured, so the PHY must be active. All other `write_*_infoframe` functions (AVI, HDMI, HDR_DRM, SPD) also call their corresponding clear functions directly without a guard, so this is consistent in spirit even though the implementation is now inlined rather than calling through.

**Change 3 — Adding `PKTSCHED_AMD_TX_EN` to the clear mask:**

The original `clear_audio_infoframe` cleared `ACR`, `AUDS`, and `AUDI` but not `AMD`. However, `write_audio_infoframe` enables all four (`ACR`, `AUDI`, `AMD` at line 1014, plus `AUDS` at line 1019). The clear path was missing the `AMD` disable, meaning repeated write/clear cycles would leave `PKTSCHED_AMD_TX_EN` set. Adding it to both the clear function and the inline clear in write restores proper enable/disable balance. This is a genuine secondary bug fix.

**Minor observations:**

1. **Consistency with other clear functions:** As noted above, `clear_avi_infoframe`, `clear_hdmi_infoframe`, `clear_hdr_drm_infoframe`, and `clear_spd_infoframe` don't guard on `tmds_char_rate`. If those can ever be called after PHY shutdown (e.g., through framework infoframe management during connector teardown), they would have the same crash. The author might want to investigate whether follow-up patches are needed there, though this is out of scope for the immediate crash fix.

2. **Race window:** There's a theoretical race where `atomic_disable` sets `tmds_char_rate = 0` and then disables the PHY, and between those two operations another CPU could read `tmds_char_rate != 0` and proceed with the register write. However, `tmds_char_rate` is set to 0 **before** `phy.ops->disable()` at lines 792-794, so the window is actually safe — the PHY is still up when the rate is zeroed. The truly dangerous case (PHY already down, rate not yet zeroed) cannot happen with the current ordering.

**Overall: Clean, well-scoped fix. The three changes are logically coherent and the commit message explains the rationale clearly. Recommend acceptance.**

---
Generated by Claude Code Patch Reviewer

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

end of thread, other threads:[~2026-05-16  3:45 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-12 10:31 [PATCH v4] drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down Frank Zhang
2026-05-16  3:45 ` Claude review: " Claude Code Review Bot
2026-05-16  3:45 ` 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