drm/bridge: dw-hdmi-qp: Guard clear_audio_infoframe when PHY is down
Subsystems (5)
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().
---
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
@@ -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
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 setstmds_char_rateto 0 and disable the PHY.In this case,
dw_hdmi_qp_audio_disable()will calldrm_atomic_helper_connector_hdmi_clear_audio_infoframe()directly, accessing registers without checkingtmds_char_rate.Move
drm_atomic_helper_connector_hdmi_clear_audio_infoframe()inside the if (hdmi->tmds_char_rate) ofdw_hdmi_qp_audio_disable().Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")Signed-off-by Frank Zhang <[email protected]>---
Changes in v2:
- Movedrm_atomic_helper_connector_hdmi_clear_audio_infoframe()inside the if (hdmi->tmds_char_rate) ofdw_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
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
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 setstmds_char_rateto 0 and disable the PHY.In this case,
dw_hdmi_qp_audio_disable()will calldrm_atomic_helper_connector_hdmi_clear_audio_infoframe()directly, accessing registers without checkingtmds_char_rate.Move
drm_atomic_helper_connector_hdmi_clear_audio_infoframe()inside the if (hdmi->tmds_char_rate) ofdw_hdmi_qp_audio_disable().Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")Signed-off-by Frank Zhang <[email protected]>---
Changes in v2:
- Movedrm_atomic_helper_connector_hdmi_clear_audio_infoframe()inside the if (hdmi->tmds_char_rate) ofdw_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@@ -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
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 setstmds_char_rateto 0 and disable the PHY.In this case,
dw_hdmi_qp_audio_disable()will calldrm_atomic_helper_connector_hdmi_clear_audio_infoframe()directly, accessing registers without checkingtmds_char_rate.Move
drm_atomic_helper_connector_hdmi_clear_audio_infoframe()inside the if (hdmi->tmds_char_rate) ofdw_hdmi_qp_audio_disable().Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")Signed-off-by Frank Zhang <[email protected]>---
Changes in v2:
- Movedrm_atomic_helper_connector_hdmi_clear_audio_infoframe()inside the if (hdmi->tmds_char_rate) ofdw_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@@ -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.0Sorry, 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 indrm_hdmi_state_helper.cthat does basically the same asdrm_atomic_helper_connector_hdmi_clear_audio_infoframe(), but only clearing the software state without callingclear_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
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 setstmds_char_rateto 0 and disable the PHY.In this case,
dw_hdmi_qp_audio_disable()will calldrm_atomic_helper_connector_hdmi_clear_audio_infoframe()directly, accessing registers without checkingtmds_char_rate.Move
drm_atomic_helper_connector_hdmi_clear_audio_infoframe()inside the if (hdmi->tmds_char_rate) ofdw_hdmi_qp_audio_disable().Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")Signed-off-by Frank Zhang <[email protected]>---
Changes in v2:
- Movedrm_atomic_helper_connector_hdmi_clear_audio_infoframe()insidethe 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
-0drivers/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.0Sorry, 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 driverQuoted 5 lines
I think the better approach is to add a '
reset_audio_infoframe' interface indrm_hdmi_state_helper.cthat does basically the same asdrm_atomic_helper_connector_hdmi_clear_audio_infoframe(), but only clearing the software state without callingclear_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
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 setstmds_char_rateto 0 and disable the PHY.In this case,
dw_hdmi_qp_audio_disable()will calldrm_atomic_helper_connector_hdmi_clear_audio_infoframe()directly, accessing registers without checkingtmds_char_rate.Move
drm_atomic_helper_connector_hdmi_clear_audio_infoframe()inside the if (hdmi->tmds_char_rate) ofdw_hdmi_qp_audio_disable().Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")Signed-off-by Frank Zhang <[email protected]>---
Changes in v2:
- Movedrm_atomic_helper_connector_hdmi_clear_audio_infoframe()insidethe 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@@ -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.0Sorry, 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 driverQuoted 5 lines
I think the better approach is to add a '
reset_audio_infoframe' interface indrm_hdmi_state_helper.cthat does basically the same asdrm_atomic_helper_connector_hdmi_clear_audio_infoframe(), but only clearing the software state without callingclear_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
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 setstmds_char_rateto 0 and disable the PHY.In this case,
dw_hdmi_qp_audio_disable()will calldrm_atomic_helper_connector_hdmi_clear_audio_infoframe()directly, accessing registers without checkingtmds_char_rate.Move
drm_atomic_helper_connector_hdmi_clear_audio_infoframe()inside the if (hdmi->tmds_char_rate) ofdw_hdmi_qp_audio_disable().Fixes fd0141d1a8a2 ("drm/bridge: synopsys: Add audio support for dw-hdmi-qp")Signed-off-by Frank Zhang <[email protected]>---
Changes in v2:
- Movedrm_atomic_helper_connector_hdmi_clear_audio_infoframe()insidethe 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@@ -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.0Sorry, 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 driverQuoted 5 lines
I think the better approach is to add a '
reset_audio_infoframe' interface indrm_hdmi_state_helper.cthat does basically the same asdrm_atomic_helper_connector_hdmi_clear_audio_infoframe(), but only clearing the software state without callingclear_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-ENODEVbefore 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 checktmds_char_rate, call the new function only whentmds_char_rateis available.dw_hdmi_qp_bridge_write_audio_infoframe()should call the new function directly instead ofdw_hdmi_qp_bridge_clear_audio_infoframe(), without checkingtmds_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_infoframeguards register access by checkingtmds_char_rate, whilewrite_audio_infoframedoes not need to check it again. How about it?Thanks,
Frank ZhangQuoted 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