Re: [PATCH v2] drm/display: let a bridge configure sink audio from hw_params

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] audio: CPU DAI timeout during resume due to ASoC prepare order
- [Medium] audio: sink unconfigured if suspended in SETUP state
--

commit d257aa80aa39633c1d1299041c31c3df56ca93d7
Author: Jean-Francois Bobier <jean-francois.bobier@laposte.net>

drm/display: let a bridge configure sink audio from hw_params

This commit allows HDMI audio bridges to configure the sink audio path from
the ASoC hw_params callback instead of prepare. It adds tracking flags to
ensure the configuration isn't run twice when not needed, specifically to
address bridges that require sink clocks running before the CPU DAI starts.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_audio_helper.c b/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
> index 7d78b02c14462..d4281cee7c65e 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
> @@ -34,9 +34,47 @@ static int drm_connector_hdmi_audio_prepare(struct device *dev, void *data,
>  	const struct drm_connector_hdmi_audio_funcs *funcs =
>  		connector->hdmi_audio.funcs;
>  
> +	/*
> +	 * Only skip the specific prepare that hw_params already did the work
> +	 * for. ASoC also reaches .prepare with no .hw_params call ahead of it
> +	 * on XRUN recovery and on resume from suspend; prepare_done_in_hw_params
> +	 * is clear in both cases, so the call below still runs and configures
> +	 * the sink here instead of leaving it unconfigured.
> +	 */
> +	if (connector->hdmi_audio.prepare_early &&
> +	    connector->hdmi_audio.prepare_done_in_hw_params) {
> +		connector->hdmi_audio.prepare_done_in_hw_params = false;
> +		return 0;
> +	}

[Severity: High]
Does this state tracking properly account for the CPU DAI being prepared first
and timing out before this code is reached?

When the system suspends during active playback, on resume ALSA calls
snd_soc_pcm_dai_prepare() without a preceding .hw_params call. Since ASoC
iterates CPU DAIs before Codec DAIs, the CPU DAI's .prepare executes first
and waits for the sink's audio clock.

However, the clock is not yet running because this codec DAI .prepare has
not been reached. Consequently, the CPU DAI times out and aborts the loop,
leaving audio broken on resume.

[Severity: Medium]
Additionally, does this logic correctly handle a system suspend that occurs
while in the SETUP state?

If the system suspends between the ALSA .hw_params and .prepare ioctls,
hardware power and configuration are lost. On resume, ALSA calls .prepare to
recover.

Because prepare_done_in_hw_params is still true from the previous SETUP phase,
this function clears the flag and returns immediately. This leaves the HDMI
audio sink unconfigured and causes a silent audio failure.

[ ... ]

> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 2ee7c59329aae..749eab1f96303 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -2184,6 +2184,26 @@ struct drm_connector_hdmi_audio {
>  	 * The port in DT that is used for the Codec DAI.
>  	 */
>  	int dai_port;
> +
> +	/**
> +	 * @prepare_early:
> +	 *
> +	 * Call @funcs->prepare from the ASoC hw_params callback instead of
> +	 * from prepare. See &drm_bridge.hdmi_audio_prepare_early.
> +	 */
> +	bool prepare_early;
> +
> +	/**
> +	 * @prepare_done_in_hw_params:
> +	 *
> +	 * Private state for the @prepare_early helper implementation. Not to
> +	 * be set by drivers. Tracks whether hw_params already ran
> +	 * @funcs->prepare for the prepare call immediately ahead, so that
> +	 * call can be skipped without skipping any other prepare -- ASoC
> +	 * calls .prepare without a preceding .hw_params on XRUN recovery and
> +	 * on resume from suspend, and @funcs->prepare must still run then.
> +	 */
> +	bool prepare_done_in_hw_params;
>  };

[Severity: High]
This documentation notes that ASoC calls .prepare without .hw_params on
resume from suspend. Given that ASoC walks CPU DAIs before codec DAIs, will
the CPU DAI's .prepare time out waiting for a clock that this codec DAI has
not yet enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005161824.36445-1-jean-francois.bobier@laposte.net?part=1