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

[Frank Zhang]

On 4/20/26 17:00, Dmitry Baryshkov wrote:
> On Mon, Apr 20, 2026 at 02:11:28PM +0800, Frank Zhang wrote:
> > On 4/19/26 08:40, Dmitry Baryshkov wrote:
> > > On Sat, Apr 18, 2026 at 06:19:36PM +0800, Frank Zhang wrote:
> > > > 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")
> > > > Signed-off-by: Frank Zhang <rmxpzlb@xxxxxxxxx>
> > > >
> > > > ---
> > > > 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@xxxxxxxxx/
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > > > index d649a1cf07f5..7760527484c8 100644
> > > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > > > @@ -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?
> > >
> > > >    }
> > > >    static int dw_hdmi_qp_i2c_read(struct dw_hdmi_qp *hdmi,
> > > > --
> > > > 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.
>
> 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

> > I'd like to get the maintainers' opinion about adding such an interface.
> >
> > Thanks,
> > Frank Zhang