Skip to contentSkip to conversation

drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down

PATCH Superseded Frank Zhang 3 participants 7 messages Apr 18 – Apr 22

Subsystems (5)

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down

The following panic was observed during system reboot:

Kernel panic - not syncing: Asynchronous SError Interrupt
CPU: 7 UID: 1000 PID: 2637 Comm: pipewire ... 6.19.10-300.fc44.aarch64
Call trace:
 ...
 regmap_update_bits_base+0x5c/0x90
 dw_hdmi_qp_bridge_clear_infoframe+0xb0/0x120 [dw_hdmi_qp]
 drm_bridge_connector_clear_infoframe+0x28/0x48 [drm_display_helper]
 ...
 dw_hdmi_qp_audio_disable+0x24/0xb8 [dw_hdmi_qp]
 drm_bridge_connector_audio_shutdown+0x30/0x60 [drm_display_helper]
 drm_connector_hdmi_audio_shutdown+0x24/0x38 [drm_display_helper]
 hdmi_codec_shutdown+0x60/0x90 [snd_soc_hdmi_codec]
 ...
 snd_pcm_release_substream.part.0+0x44/0xd8 [snd_pcm]
 snd_pcm_release+0x60/0xe8 [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 disable the PHY.

In this case, dw_hdmi_qp_audio_disable() will call drm_atomic_helper_connector_hdmi_clear_audio_infoframe() directly, accessing registers without checking tmds_char_rate.

Move drm_atomic_helper_connector_hdmi_clear_audio_infoframe() inside the if (hdmi->tmds_char_rate) of dw_hdmi_qp_audio_disable().

Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")

---
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/[email protected]/

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +3 -3
index d649a1cf07f5..7760527484c8 100644
@@ -526,10 +526,10 @@ static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge,
 {
 	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
 
-	drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
-
-	if (hdmi->tmds_char_rate)
+	if (hdmi->tmds_char_rate) {
+		drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
 		dw_hdmi_qp_audio_disable_regs(hdmi);
+	}
 }
 
 static int dw_hdmi_qp_i2c_read(struct dw_hdmi_qp *hdmi,
Signature · 1 line
2.53.0
#1 lore raw
Quoted 43 lines · Frank Zhang

The following panic was observed during system reboot:

Kernel panic - not syncing: Asynchronous SError Interrupt
CPU: 7 UID: 1000 PID: 2637 Comm: pipewire ... 6.19.10-300.fc44.aarch64
Call trace:
 ...
 regmap_update_bits_base+0x5c/0x90
 dw_hdmi_qp_bridge_clear_infoframe+0xb0/0x120 [dw_hdmi_qp]
 drm_bridge_connector_clear_infoframe+0x28/0x48 [drm_display_helper]
 ...
 dw_hdmi_qp_audio_disable+0x24/0xb8 [dw_hdmi_qp]
 drm_bridge_connector_audio_shutdown+0x30/0x60 [drm_display_helper]
 drm_connector_hdmi_audio_shutdown+0x24/0x38 [drm_display_helper]
 hdmi_codec_shutdown+0x60/0x90 [snd_soc_hdmi_codec]
 ...
 snd_pcm_release_substream.part.0+0x44/0xd8 [snd_pcm]
 snd_pcm_release+0x60/0xe8 [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 disable the PHY.

In this case, dw_hdmi_qp_audio_disable() will call drm_atomic_helper_connector_hdmi_clear_audio_infoframe() directly, accessing registers without checking tmds_char_rate.

Move drm_atomic_helper_connector_hdmi_clear_audio_infoframe() inside the if (hdmi->tmds_char_rate) of dw_hdmi_qp_audio_disable().

Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")

---
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/[email protected]/

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -0
index d649a1cf07f5..7760527484c8 100644

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge, · line 532

Quoted 10 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +3 -3
@@ -526,10 +526,10 @@ static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge,
 {
 	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
 
-	drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
-
-	if (hdmi->tmds_char_rate)
+	if (hdmi->tmds_char_rate) {
+		drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
 		dw_hdmi_qp_audio_disable_regs(hdmi);
+	}

Will audio and audio infoframe remain disabled after consequetive atomic_enable() call?

Quoted 6 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -0
 }
 
 static int dw_hdmi_qp_i2c_read(struct dw_hdmi_qp *hdmi,
Signature · 1 line
2.53.0
Signature · 2 lines
With best wishes
Dmitry
#2 lore raw
Quoted 47 lines · Dmitry Baryshkov
Quoted 46 lines · Frank Zhang

The following panic was observed during system reboot:

Kernel panic - not syncing: Asynchronous SError Interrupt
CPU: 7 UID: 1000 PID: 2637 Comm: pipewire ... 6.19.10-300.fc44.aarch64
Call trace:
  ...
  regmap_update_bits_base+0x5c/0x90
  dw_hdmi_qp_bridge_clear_infoframe+0xb0/0x120 [dw_hdmi_qp]
  drm_bridge_connector_clear_infoframe+0x28/0x48 [drm_display_helper]
  ...
  dw_hdmi_qp_audio_disable+0x24/0xb8 [dw_hdmi_qp]
  drm_bridge_connector_audio_shutdown+0x30/0x60 [drm_display_helper]
  drm_connector_hdmi_audio_shutdown+0x24/0x38 [drm_display_helper]
  hdmi_codec_shutdown+0x60/0x90 [snd_soc_hdmi_codec]
  ...
  snd_pcm_release_substream.part.0+0x44/0xd8 [snd_pcm]
  snd_pcm_release+0x60/0xe8 [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 disable the PHY.

In this case, dw_hdmi_qp_audio_disable() will call drm_atomic_helper_connector_hdmi_clear_audio_infoframe() directly, accessing registers without checking tmds_char_rate.

Move drm_atomic_helper_connector_hdmi_clear_audio_infoframe() inside the if (hdmi->tmds_char_rate) of dw_hdmi_qp_audio_disable().

Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")

---
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/[email protected]/

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -0
index d649a1cf07f5..7760527484c8 100644
@@ -526,10 +526,10 @@ static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge,
  {
  	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
 
Quoted 18 lines

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge, · line 532

Quoted 7 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +3 -3
-	drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
-
-	if (hdmi->tmds_char_rate)
+	if (hdmi->tmds_char_rate) {
+		drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
  		dw_hdmi_qp_audio_disable_regs(hdmi);
+	}

Will audio and audio infoframe remain disabled after consequetive atomic_enable() call?

Quoted 6 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -0
  }
 
  static int dw_hdmi_qp_i2c_read(struct dw_hdmi_qp *hdmi,
Signature · 1 line
2.53.0

Sorry, I missed clearing the audio infoframe when the PHY is down. The next atomic_enable() will write the stale audio infoframe. My mistake.

To clear the stale audio infoframe, dw_hdmi_qp_audio_disable() can handle it in the else branch directly, but this seems like a layering violation for a bridge driver

I think the better approach is to add a 'reset_audio_infoframe' interface in drm_hdmi_state_helper.c that does basically the same as drm_atomic_helper_connector_hdmi_clear_audio_infoframe(), but only clearing the software state without calling clear_infoframe(). It's also a bit odd since it would only be used by dw-hdmi-qp.

I'd like to get the maintainers' opinion about adding such an interface.

Thanks,
Frank Zhang

#3 lore raw
Quoted 72 lines · Frank Zhang
Quoted 48 lines · Dmitry Baryshkov
Quoted 47 lines · Frank Zhang

The following panic was observed during system reboot:

Kernel panic - not syncing: Asynchronous SError Interrupt
CPU: 7 UID: 1000 PID: 2637 Comm: pipewire ... 6.19.10-300.fc44.aarch64
Call trace:
  ...
  regmap_update_bits_base+0x5c/0x90
  dw_hdmi_qp_bridge_clear_infoframe+0xb0/0x120 [dw_hdmi_qp]
  drm_bridge_connector_clear_infoframe+0x28/0x48 [drm_display_helper]
  ...
  dw_hdmi_qp_audio_disable+0x24/0xb8 [dw_hdmi_qp]
  drm_bridge_connector_audio_shutdown+0x30/0x60 [drm_display_helper]
  drm_connector_hdmi_audio_shutdown+0x24/0x38 [drm_display_helper]
  hdmi_codec_shutdown+0x60/0x90 [snd_soc_hdmi_codec]
  ...
  snd_pcm_release_substream.part.0+0x44/0xd8 [snd_pcm]
  snd_pcm_release+0x60/0xe8 [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 disable the PHY.

In this case, dw_hdmi_qp_audio_disable() will call drm_atomic_helper_connector_hdmi_clear_audio_infoframe() directly, accessing registers without checking tmds_char_rate.

Move drm_atomic_helper_connector_hdmi_clear_audio_infoframe() inside the if (hdmi->tmds_char_rate) of dw_hdmi_qp_audio_disable().

Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")

---
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/[email protected]/

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -2
index d649a1cf07f5..7760527484c8 100644
@@ -526,10 +526,10 @@ static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge,
  {
  	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
-	drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
-
Quoted 15 lines

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge, · line 532

Quoted 5 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +3 -1
-	if (hdmi->tmds_char_rate)
+	if (hdmi->tmds_char_rate) {
+		drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
  		dw_hdmi_qp_audio_disable_regs(hdmi);
+	}

Will audio and audio infoframe remain disabled after consequetive atomic_enable() call?

Quoted 5 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -0
  }
  static int dw_hdmi_qp_i2c_read(struct dw_hdmi_qp *hdmi,
Signature · 1 line
2.53.0

Sorry, I missed clearing the audio infoframe when the PHY is down. The next atomic_enable() will write the stale audio infoframe. My mistake.

To clear the stale audio infoframe, dw_hdmi_qp_audio_disable() can handle it in the else branch directly, but this seems like a layering violation for a bridge driver

Quoted 5 lines

I think the better approach is to add a 'reset_audio_infoframe' interface in drm_hdmi_state_helper.c that does basically the same as drm_atomic_helper_connector_hdmi_clear_audio_infoframe(), but only clearing the software state without calling clear_infoframe(). It's also a bit odd since it would only be used by dw-hdmi-qp.

That sounds too fine-grained and it also will not work straight ahead...

Just for my understanding, let's consider the opposite situation: the user tries to start audio playback before setting the mode. How is it handled in the driver?

Quoted 6 lines

I'd like to get the maintainers' opinion about adding such an interface.

Thanks,
Frank Zhang

Signature · 2 lines
With best wishes
Dmitry
#4 lore raw

Hi Dmitry,

Quoted 75 lines · Dmitry Baryshkov
Quoted 69 lines · Frank Zhang
Quoted 39 lines · Dmitry Baryshkov
Quoted 38 lines · Frank Zhang

The following panic was observed during system reboot:

Kernel panic - not syncing: Asynchronous SError Interrupt
CPU: 7 UID: 1000 PID: 2637 Comm: pipewire ... 6.19.10-300.fc44.aarch64
Call trace:
   ...
   regmap_update_bits_base+0x5c/0x90
   dw_hdmi_qp_bridge_clear_infoframe+0xb0/0x120 [dw_hdmi_qp]
   drm_bridge_connector_clear_infoframe+0x28/0x48 [drm_display_helper]
   ...
   dw_hdmi_qp_audio_disable+0x24/0xb8 [dw_hdmi_qp]
   drm_bridge_connector_audio_shutdown+0x30/0x60 [drm_display_helper]
   drm_connector_hdmi_audio_shutdown+0x24/0x38 [drm_display_helper]
   hdmi_codec_shutdown+0x60/0x90 [snd_soc_hdmi_codec]
   ...
   snd_pcm_release_substream.part.0+0x44/0xd8 [snd_pcm]
   snd_pcm_release+0x60/0xe8 [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 disable the PHY.

In this case, dw_hdmi_qp_audio_disable() will call drm_atomic_helper_connector_hdmi_clear_audio_infoframe() directly, accessing registers without checking tmds_char_rate.

Move drm_atomic_helper_connector_hdmi_clear_audio_infoframe() inside the if (hdmi->tmds_char_rate) of dw_hdmi_qp_audio_disable().

Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")

---
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/[email protected]/

Quoted 22 lines
Quoted 5 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -0
index d649a1cf07f5..7760527484c8 100644

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge, · line 531

Quoted 9 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +3 -3
@@ -526,10 +526,10 @@ static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge,
   {
   	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
-	drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
-
-	if (hdmi->tmds_char_rate)
+	if (hdmi->tmds_char_rate) {
+		drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
   		dw_hdmi_qp_audio_disable_regs(hdmi);
+	}

Will audio and audio infoframe remain disabled after consequetive atomic_enable() call?

Quoted 5 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -0
   }
   static int dw_hdmi_qp_i2c_read(struct dw_hdmi_qp *hdmi,
Signature · 1 line
2.53.0

Sorry, I missed clearing the audio infoframe when the PHY is down. The next atomic_enable() will write the stale audio infoframe. My mistake.

To clear the stale audio infoframe, dw_hdmi_qp_audio_disable() can handle it in the else branch directly, but this seems like a layering violation for a bridge driver

Quoted 5 lines

I think the better approach is to add a 'reset_audio_infoframe' interface in drm_hdmi_state_helper.c that does basically the same as drm_atomic_helper_connector_hdmi_clear_audio_infoframe(), but only clearing the software state without calling clear_infoframe(). It's also a bit odd since it would only be used by dw-hdmi-qp.

Quoted 5 lines

That sounds too fine-grained and it also will not work straight ahead...

Just for my understanding, let's consider the opposite situation: the user tries to start audio playback before setting the mode. How is it handled in the driver?

Currently, when setting audio params, the value of tmds_char_rate is needed to compute some clocks value.

So if the user starts audio before any mode is set, we just return -ENODEV.

I sent a tentative fix to just return 0 some time ago:
https://lore.kernel.org/all/[email protected]/

Detlev.

Quoted 5 lines
Quoted 5 lines

I'd like to get the maintainers' opinion about adding such an interface.

Thanks,
Frank Zhang

#5 lore raw
In reply to Dmitry Baryshkov
Quoted 79 lines · Dmitry Baryshkov
Quoted 72 lines · Frank Zhang
Quoted 48 lines · Dmitry Baryshkov
Quoted 47 lines · Frank Zhang

The following panic was observed during system reboot:

Kernel panic - not syncing: Asynchronous SError Interrupt
CPU: 7 UID: 1000 PID: 2637 Comm: pipewire ... 6.19.10-300.fc44.aarch64
Call trace:
   ...
   regmap_update_bits_base+0x5c/0x90
   dw_hdmi_qp_bridge_clear_infoframe+0xb0/0x120 [dw_hdmi_qp]
   drm_bridge_connector_clear_infoframe+0x28/0x48 [drm_display_helper]
   ...
   dw_hdmi_qp_audio_disable+0x24/0xb8 [dw_hdmi_qp]
   drm_bridge_connector_audio_shutdown+0x30/0x60 [drm_display_helper]
   drm_connector_hdmi_audio_shutdown+0x24/0x38 [drm_display_helper]
   hdmi_codec_shutdown+0x60/0x90 [snd_soc_hdmi_codec]
   ...
   snd_pcm_release_substream.part.0+0x44/0xd8 [snd_pcm]
   snd_pcm_release+0x60/0xe8 [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 disable the PHY.

In this case, dw_hdmi_qp_audio_disable() will call drm_atomic_helper_connector_hdmi_clear_audio_infoframe() directly, accessing registers without checking tmds_char_rate.

Move drm_atomic_helper_connector_hdmi_clear_audio_infoframe() inside the if (hdmi->tmds_char_rate) of dw_hdmi_qp_audio_disable().

Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")

---
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/[email protected]/

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -2
index d649a1cf07f5..7760527484c8 100644
@@ -526,10 +526,10 @@ static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge,
   {
   	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
-	drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
-
Quoted 15 lines

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge, · line 532

Quoted 5 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +3 -1
-	if (hdmi->tmds_char_rate)
+	if (hdmi->tmds_char_rate) {
+		drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
   		dw_hdmi_qp_audio_disable_regs(hdmi);
+	}

Will audio and audio infoframe remain disabled after consequetive atomic_enable() call?

Quoted 5 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -0
   }
   static int dw_hdmi_qp_i2c_read(struct dw_hdmi_qp *hdmi,
Signature · 1 line
2.53.0

Sorry, I missed clearing the audio infoframe when the PHY is down. The next atomic_enable() will write the stale audio infoframe. My mistake.

To clear the stale audio infoframe, dw_hdmi_qp_audio_disable() can handle it in the else branch directly, but this seems like a layering violation for a bridge driver

Quoted 5 lines

I think the better approach is to add a 'reset_audio_infoframe' interface in drm_hdmi_state_helper.c that does basically the same as drm_atomic_helper_connector_hdmi_clear_audio_infoframe(), but only clearing the software state without calling clear_infoframe(). It's also a bit odd since it would only be used by dw-hdmi-qp.

Quoted 6 lines

That sounds too fine-grained and it also will not work straight ahead...

Just for my understanding, let's consider the opposite situation: the user tries to start audio playback before setting the mode. How is it handled in the driver?

audio_prepare() will return -ENODEV before the mode is set.

I admit modifying dw-hdmi-qp is more reasonable. I have a new approach:

dw-hdmi-qp should have an internal function to clear the audio infoframe related registers.
dw_hdmi_qp_bridge_clear_audio_infoframe() should check tmds_char_rate, call the new function only when tmds_char_rate is available. dw_hdmi_qp_bridge_write_audio_infoframe() should call the new function directly instead of dw_hdmi_qp_bridge_clear_audio_infoframe(), without checking tmds_char_rate. dw_hdmi_qp_audio_disable() doesn't need any modification.

With this approach, clear_audio_infoframe guards register access by checking tmds_char_rate, while write_audio_infoframe does not need to check it again. How about it?

Thanks,
Frank Zhang

Quoted 7 lines
Quoted 6 lines

I'd like to get the maintainers' opinion about adding such an interface.

Thanks,
Frank Zhang

#6 lore raw
Quoted 91 lines · Frank Zhang
Quoted 79 lines · Dmitry Baryshkov
Quoted 72 lines · Frank Zhang
Quoted 48 lines · Dmitry Baryshkov
Quoted 47 lines · Frank Zhang

The following panic was observed during system reboot:

Kernel panic - not syncing: Asynchronous SError Interrupt
CPU: 7 UID: 1000 PID: 2637 Comm: pipewire ... 6.19.10-300.fc44.aarch64
Call trace:
   ...
   regmap_update_bits_base+0x5c/0x90
   dw_hdmi_qp_bridge_clear_infoframe+0xb0/0x120 [dw_hdmi_qp]
   drm_bridge_connector_clear_infoframe+0x28/0x48 [drm_display_helper]
   ...
   dw_hdmi_qp_audio_disable+0x24/0xb8 [dw_hdmi_qp]
   drm_bridge_connector_audio_shutdown+0x30/0x60 [drm_display_helper]
   drm_connector_hdmi_audio_shutdown+0x24/0x38 [drm_display_helper]
   hdmi_codec_shutdown+0x60/0x90 [snd_soc_hdmi_codec]
   ...
   snd_pcm_release_substream.part.0+0x44/0xd8 [snd_pcm]
   snd_pcm_release+0x60/0xe8 [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 disable the PHY.

In this case, dw_hdmi_qp_audio_disable() will call drm_atomic_helper_connector_hdmi_clear_audio_infoframe() directly, accessing registers without checking tmds_char_rate.

Move drm_atomic_helper_connector_hdmi_clear_audio_infoframe() inside the if (hdmi->tmds_char_rate) of dw_hdmi_qp_audio_disable().

Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")

---
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/[email protected]/

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -2
index d649a1cf07f5..7760527484c8 100644
@@ -526,10 +526,10 @@ static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge,
   {
   	struct dw_hdmi_qp *hdmi = dw_hdmi_qp_from_bridge(bridge);
-	drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
-
Quoted 15 lines

drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c static void dw_hdmi_qp_audio_disable(struct drm_bridge *bridge, · line 532

Quoted 5 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +3 -1
-	if (hdmi->tmds_char_rate)
+	if (hdmi->tmds_char_rate) {
+		drm_atomic_helper_connector_hdmi_clear_audio_infoframe(connector);
   		dw_hdmi_qp_audio_disable_regs(hdmi);
+	}

Will audio and audio infoframe remain disabled after consequetive atomic_enable() call?

Quoted 5 lines
drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c +0 -0
   }
   static int dw_hdmi_qp_i2c_read(struct dw_hdmi_qp *hdmi,
Signature · 1 line
2.53.0

Sorry, I missed clearing the audio infoframe when the PHY is down. The next atomic_enable() will write the stale audio infoframe. My mistake.

To clear the stale audio infoframe, dw_hdmi_qp_audio_disable() can handle it in the else branch directly, but this seems like a layering violation for a bridge driver

Quoted 5 lines

I think the better approach is to add a 'reset_audio_infoframe' interface in drm_hdmi_state_helper.c that does basically the same as drm_atomic_helper_connector_hdmi_clear_audio_infoframe(), but only clearing the software state without calling clear_infoframe(). It's also a bit odd since it would only be used by dw-hdmi-qp.

Quoted 6 lines

That sounds too fine-grained and it also will not work straight ahead...

Just for my understanding, let's consider the opposite situation: the user tries to start audio playback before setting the mode. How is it handled in the driver?

audio_prepare() will return -ENODEV before the mode is set.

I admit modifying dw-hdmi-qp is more reasonable. I have a new approach:

Quoted 8 lines

dw-hdmi-qp should have an internal function to clear the audio infoframe related registers.
dw_hdmi_qp_bridge_clear_audio_infoframe() should check tmds_char_rate, call the new function only when tmds_char_rate is available. dw_hdmi_qp_bridge_write_audio_infoframe() should call the new function directly instead of dw_hdmi_qp_bridge_clear_audio_infoframe(), without checking tmds_char_rate.
dw_hdmi_qp_audio_disable() doesn't need any modification.

Assuming that all audio-related registers are cleared when you perform PHY disable / enable cycle, this looks good to me.

Quoted 16 lines

With this approach, clear_audio_infoframe guards register access by checking tmds_char_rate, while write_audio_infoframe does not need to check it again. How about it?

Thanks,
Frank Zhang

Quoted 7 lines
Quoted 6 lines

I'd like to get the maintainers' opinion about adding such an interface.

Thanks,
Frank Zhang

Signature · 2 lines
With best wishes
Dmitry