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

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

[Jean-Francois Bobier]

drm_connector_hdmi_audio_ops implements .prepare and not .hw_params, so
every bridge using the HDMI audio helper configures its sink's audio path
from the codec DAI's prepare callback.

That is too late when the CPU side of the DAI link cannot start until the
sink's audio clock is already running. ASoC's snd_soc_pcm_dai_prepare()
walks CPU DAIs before codec DAIs and returns on the first error, so such
a CPU DAI fails and aborts the loop before the bridge's prepare is
reached -- the clock it was waiting for is enabled by a callback that the
failure prevents from running.

Qualcomm's LPASS is one such CPU side. On an sm8250 board with the
DisplayPort controller bound to the DISPLAY_PORT_RX AFE port, starting
playback gives:

  qcom-q6afe: AFE enable for port 0x6020 failed -110
  q6afe-dai: ASoC error (-110): at snd_soc_dai_prepare() on DISPLAY_PORT_RX_0

and an ftrace of the attempt shows msm_dp_audio_prepare() is never
called at all -- only msm_dp_audio_shutdown(), from the teardown path.

Add a .hw_params to the helper that runs the same configuration, and a
drm_bridge flag to select it. hw_params runs for every DAI in the link
before any of them is prepared, so the clock is up by the time the CPU
DAI starts its port. Both callbacks receive identical parameters, so the
configuration itself is unchanged.

The flag defaults to off: a bridge that does not set it keeps being
configured from prepare exactly as before, and the new hw_params returns
without touching it. Note that providing .hw_params at all means
hdmi_codec_hw_params() no longer returns early for these connectors, so
it now runs hdmi_codec_fill_codec_params() and the IEC958 channel status
fill before the (inert) callback. Both are computed into an on-stack
hdmi_codec_params that is then discarded, and the one shared value it
writes, daifmt->bit_fmt, is set by hdmi_codec_prepare() to the same value
immediately afterwards.

One more side effect worth disclosing: hdmi_codec_fill_codec_params()
also writes hcp->chmap_idx, which is not on-stack -- it persists on the
hdmi-codec device and is reused by the next call to
hdmi_codec_get_ch_alloc_table_idx(). hdmi_codec_prepare() recomputes it
immediately afterwards from the same channel count, so the value itself
does not go stale. But hdmi_codec_hw_params() now reaching this code at
all means that a channel count the sink's ELD does not advertise -- the
error path in hdmi_codec_get_ch_alloc_table_idx() -- is reported from
hw_params() instead of prepare(), for every bridge sharing
drm_connector_hdmi_audio_ops, not only the ones that set
hdmi_audio_prepare_early. That changes when a bad channel count is
reported, not whether it is, but it is a change to seven bridges this
patch has no other reason to touch, and worth a maintainer's opinion.

Signed-off-by: Jean-Francois Bobier <jean-francois.bobier@laposte.net>
---
 .../gpu/drm/display/drm_bridge_connector.c    |  3 +++
 .../gpu/drm/display/drm_hdmi_audio_helper.c   | 23 +++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_drm.c               |  8 +++++++
 include/drm/drm_bridge.h                      | 17 ++++++++++++++
 include/drm/drm_connector.h                   |  8 +++++++
 5 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 8b54069fa..51293181b 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -1048,6 +1048,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 						    bridge->hdmi_audio_dai_port);
 		if (ret)
 			return ERR_PTR(ret);
+
+		connector->hdmi_audio.prepare_early =
+			bridge->hdmi_audio_prepare_early;
 	}
 
 	if (bridge_connector->bridge_hdmi_cec &&
diff --git a/drivers/gpu/drm/display/drm_hdmi_audio_helper.c b/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
index 7d78b02c1..3ed5d73f1 100644
--- a/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
@@ -34,6 +34,28 @@ static int drm_connector_hdmi_audio_prepare(struct device *dev, void *data,
 	const struct drm_connector_hdmi_audio_funcs *funcs =
 		connector->hdmi_audio.funcs;
 
+	if (connector->hdmi_audio.prepare_early)
+		return 0;
+
+	return funcs->prepare(connector, fmt, hparms);
+}
+
+/*
+ * Only used by connectors that asked for it; everyone else keeps being
+ * configured from prepare() and this returns without doing anything. See
+ * &drm_bridge.hdmi_audio_prepare_early for why a connector would want it.
+ */
+static int drm_connector_hdmi_audio_hw_params(struct device *dev, void *data,
+					      struct hdmi_codec_daifmt *fmt,
+					      struct hdmi_codec_params *hparms)
+{
+	struct drm_connector *connector = data;
+	const struct drm_connector_hdmi_audio_funcs *funcs =
+		connector->hdmi_audio.funcs;
+
+	if (!connector->hdmi_audio.prepare_early)
+		return 0;
+
 	return funcs->prepare(connector, fmt, hparms);
 }
 
@@ -130,6 +152,7 @@ EXPORT_SYMBOL(drm_connector_hdmi_audio_plugged_notify);
 
 static const struct hdmi_codec_ops drm_connector_hdmi_audio_ops = {
 	.audio_startup = drm_connector_hdmi_audio_startup,
+	.hw_params = drm_connector_hdmi_audio_hw_params,
 	.prepare = drm_connector_hdmi_audio_prepare,
 	.audio_shutdown = drm_connector_hdmi_audio_shutdown,
 	.mute_stream = drm_connector_hdmi_audio_mute_stream,
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index c1e4147bf..52c90937f 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -327,6 +327,14 @@ int msm_dp_bridge_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
 		bridge->hdmi_audio_dev = &msm_dp_display->pdev->dev;
 		bridge->hdmi_audio_max_i2s_playback_channels = 8;
 		bridge->hdmi_audio_dai_port = -1;
+		/*
+		 * The LPASS/ADSP side of the DAI link will not start its AFE
+		 * port until this controller's audio clock is running, and
+		 * ASoC prepares CPU DAIs before codec DAIs, so configuring
+		 * from prepare() is too late -- the CPU DAI times out and
+		 * aborts the link before we are called at all.
+		 */
+		bridge->hdmi_audio_prepare_early = 1;
 	}
 
 	rc = devm_drm_bridge_add(dev->dev, bridge);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 58fff047f..e8fb944f3 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1128,6 +1128,23 @@ struct drm_bridge {
 	 */
 	unsigned int hdmi_audio_spdif_playback : 1;
 
+	/**
+	 * @hdmi_audio_prepare_early: configure the sink's audio path from the
+	 * ASoC hw_params callback rather than from prepare.
+	 *
+	 * Both callbacks receive identical parameters, so for most bridges it
+	 * makes no difference and the default (prepare) is right. It matters
+	 * when the CPU side of the DAI link cannot start until the sink's
+	 * audio clock is already running: ASoC walks CPU DAIs before codec
+	 * DAIs and stops at the first error, so a CPU DAI that fails waiting
+	 * for that clock aborts the loop before this bridge's prepare is ever
+	 * reached. Qualcomm's LPASS/ADSP is one such CPU side.
+	 *
+	 * Setting this moves the configuration to hw_params, which runs for
+	 * every DAI in the link before any of them is prepared.
+	 */
+	unsigned int hdmi_audio_prepare_early : 1;
+
 	/**
 	 * @hdmi_audio_dai_port: sound DAI port for either of
 	 * @DRM_BRIDGE_OP_HDMI_AUDIO and @DRM_BRIDGE_OP_DP_AUDIO, -1 if it is
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index a0cf0268d..ae3d1382f 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2027,6 +2027,14 @@ 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;
 };
 
 /*
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/display: skipping bridge prepare() breaks ALSA resume and XRUN recovery
--

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

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

This commit introduces a prepare_early flag to configure a sink's audio
path from the ASoC hw_params callback rather than from prepare. This is
done to support configurations where the CPU side of the DAI link cannot
start until the sink's audio clock is already running.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_audio_helper.c b/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
> index 7d78b02c14462..3ed5d73f17eb0 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
> @@ -34,6 +34,28 @@ static int drm_connector_hdmi_audio_prepare(struct device *dev, void *data,
>  	const struct drm_connector_hdmi_audio_funcs *funcs =
>  		connector->hdmi_audio.funcs;
>  
> +	if (connector->hdmi_audio.prepare_early)
> +		return 0;

[Severity: High]
Can skipping hardware initialization here break recovery from XRUNs or system
resume?

When ALSA resumes from system suspend or recovers from an XRUN, the core (or
userspace) calls snd_pcm_prepare() without calling .hw_params. Because
prepare_early is set, drm_connector_hdmi_audio_prepare() returns early and
skips funcs->prepare().

This means the hardware audio clocks and infoframe configurations are never
restored, and the subsequent CPU DAI .prepare can time out waiting for the
missing audio clock.

> +
> +	return funcs->prepare(connector, fmt, hparms);
> +}
> +
> +/*
> + * Only used by connectors that asked for it; everyone else keeps being
> + * configured from prepare() and this returns without doing anything. See
> + * &drm_bridge.hdmi_audio_prepare_early for why a connector would want it.
> + */
> +static int drm_connector_hdmi_audio_hw_params(struct device *dev, void *data,
> +					      struct hdmi_codec_daifmt *fmt,
> +					      struct hdmi_codec_params *hparms)
> +{
> +	struct drm_connector *connector = data;
> +	const struct drm_connector_hdmi_audio_funcs *funcs =
> +		connector->hdmi_audio.funcs;
> +
> +	if (!connector->hdmi_audio.prepare_early)
> +		return 0;
> +
> +	return funcs->prepare(connector, fmt, hparms);

[Severity: High]
Will shifting the hardware setup exclusively to .hw_params leave the hardware
disabled during recovery?

Because ALSA does not invoke .hw_params during XRUN or resume recovery,
shifting the configuration entirely here means the stream may become completely
unusable after an XRUN or system resume. This would require the stream to be
fully closed and reopened to recover.

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

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

[Jean-Francois Bobier]

drm_connector_hdmi_audio_ops implements .prepare and not .hw_params, so
every bridge using the HDMI audio helper configures its sink's audio path
from the codec DAI's prepare callback.

That is too late when the CPU side of the DAI link cannot start until the
sink's audio clock is already running. ASoC's snd_soc_pcm_dai_prepare()
walks CPU DAIs before codec DAIs and returns on the first error, so such
a CPU DAI fails and aborts the loop before the bridge's prepare is
reached -- the clock it was waiting for is enabled by a callback that the
failure prevents from running.

Qualcomm's LPASS is one such CPU side. On an sm8250 board with the
DisplayPort controller bound to the DISPLAY_PORT_RX AFE port, starting
playback gives:

  qcom-q6afe: AFE enable for port 0x6020 failed -110
  q6afe-dai: ASoC error (-110): at snd_soc_dai_prepare() on DISPLAY_PORT_RX_0

and an ftrace of the attempt shows msm_dp_audio_prepare() is never
called at all -- only msm_dp_audio_shutdown(), from the teardown path.

Add a .hw_params to the helper that runs the same configuration, and a
drm_bridge flag to select it. hw_params runs for every DAI in the link
before any of them is prepared, so the clock is up by the time the CPU
DAI starts its port. Both callbacks receive identical parameters, so the
configuration itself is unchanged.

The flag defaults to off: a bridge that does not set it keeps being
configured from prepare exactly as before. For one that does, prepare
skips calling the sink's configuration again only for the specific call
hw_params already did it for -- tracked per-connector, consumed on use --
because ASoC reaches .prepare with no .hw_params ahead of it on XRUN
recovery and on resume from suspend, and the sink still needs configuring
then. Skipping unconditionally would have left it silently unconfigured
after either.

Note that providing .hw_params at all means hdmi_codec_hw_params() no
longer returns early for these connectors, so it now runs
hdmi_codec_fill_codec_params() and the IEC958 channel status fill before
the (inert, when nothing needs doing) callback. Both are computed into an
on-stack hdmi_codec_params that is then discarded, and the one shared
value written, daifmt->bit_fmt, is set by hdmi_codec_prepare() to the
same value immediately afterwards.

One more side effect worth disclosing: hdmi_codec_fill_codec_params()
also writes hcp->chmap_idx, which is not on-stack -- it persists on the
hdmi-codec device and is reused by the next call to
hdmi_codec_get_ch_alloc_table_idx(). hdmi_codec_prepare() recomputes it
immediately afterwards from the same channel count, so the value itself
does not go stale. But hdmi_codec_hw_params() now reaching this code at
all means that a channel count the sink's ELD does not advertise is
reported from hw_params() instead of prepare(), for every bridge sharing
drm_connector_hdmi_audio_ops, not only the ones that set
hdmi_audio_prepare_early. That changes when a bad channel count is
reported, not whether it is, but it is a change to seven bridges this
patch has no other reason to touch, and worth a maintainer's opinion.

Signed-off-by: Jean-Francois Bobier <jean-francois.bobier@laposte.net>
---
Changes in v2:
- v1 skipped prepare() unconditionally whenever prepare_early was set.
  Checked against ALSA core (sound/core/pcm_native.c): .prepare is
  reachable from SNDRV_PCM_STATE_XRUN and SNDRV_PCM_STATE_SUSPENDED with
  no .hw_params call in between -- snd_pcm_pre_prepare() explicitly
  allows both, and neither XRUN recovery nor resume renegotiates hw
  params. v1's unconditional skip meant the sink's audio path was never
  reconfigured after either, which a full suspend in particular needs
  since the DP controller's power domain drops and the configuration
  .prepare would have applied is gone. Fixed by tracking, per connector,
  whether hw_params already did the work for the specific prepare call
  immediately ahead of it, consumed on use -- any prepare that isn't
  immediately preceded by a matching hw_params now runs the real
  configuration, same as every bridge not using this flag at all.
- Caught by this list's automated review on v1, not by hand.

 .../gpu/drm/display/drm_bridge_connector.c    |  3 ++
 .../gpu/drm/display/drm_hdmi_audio_helper.c   | 39 +++++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_drm.c               |  8 ++++
 include/drm/drm_bridge.h                      | 17 ++++++++
 include/drm/drm_connector.h                   | 20 ++++++++++
 5 files changed, 87 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 81f3e26f6..731ba4b2a 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -1107,6 +1107,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 						    bridge->hdmi_audio_dai_port);
 		if (ret)
 			return ERR_PTR(ret);
+
+		connector->hdmi_audio.prepare_early =
+			bridge->hdmi_audio_prepare_early;
 	}
 
 	if (bridge_connector->bridge_hdmi_cec &&
diff --git a/drivers/gpu/drm/display/drm_hdmi_audio_helper.c b/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
index 7d78b02c1..d4281cee7 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;
+	}
+
 	return funcs->prepare(connector, fmt, hparms);
 }
 
+/*
+ * Only used by connectors that asked for it; everyone else keeps being
+ * configured from prepare() and this returns without doing anything. See
+ * &drm_bridge.hdmi_audio_prepare_early for why a connector would want it.
+ */
+static int drm_connector_hdmi_audio_hw_params(struct device *dev, void *data,
+					      struct hdmi_codec_daifmt *fmt,
+					      struct hdmi_codec_params *hparms)
+{
+	struct drm_connector *connector = data;
+	const struct drm_connector_hdmi_audio_funcs *funcs =
+		connector->hdmi_audio.funcs;
+	int ret;
+
+	if (!connector->hdmi_audio.prepare_early)
+		return 0;
+
+	ret = funcs->prepare(connector, fmt, hparms);
+	if (ret)
+		return ret;
+
+	connector->hdmi_audio.prepare_done_in_hw_params = true;
+	return 0;
+}
+
 static void drm_connector_hdmi_audio_shutdown(struct device *dev, void *data)
 {
 	struct drm_connector *connector = data;
@@ -130,6 +168,7 @@ EXPORT_SYMBOL(drm_connector_hdmi_audio_plugged_notify);
 
 static const struct hdmi_codec_ops drm_connector_hdmi_audio_ops = {
 	.audio_startup = drm_connector_hdmi_audio_startup,
+	.hw_params = drm_connector_hdmi_audio_hw_params,
 	.prepare = drm_connector_hdmi_audio_prepare,
 	.audio_shutdown = drm_connector_hdmi_audio_shutdown,
 	.mute_stream = drm_connector_hdmi_audio_mute_stream,
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index c1e4147bf..52c90937f 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -327,6 +327,14 @@ int msm_dp_bridge_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
 		bridge->hdmi_audio_dev = &msm_dp_display->pdev->dev;
 		bridge->hdmi_audio_max_i2s_playback_channels = 8;
 		bridge->hdmi_audio_dai_port = -1;
+		/*
+		 * The LPASS/ADSP side of the DAI link will not start its AFE
+		 * port until this controller's audio clock is running, and
+		 * ASoC prepares CPU DAIs before codec DAIs, so configuring
+		 * from prepare() is too late -- the CPU DAI times out and
+		 * aborts the link before we are called at all.
+		 */
+		bridge->hdmi_audio_prepare_early = 1;
 	}
 
 	rc = devm_drm_bridge_add(dev->dev, bridge);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 1981d24a7..d208ba114 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -1177,6 +1177,23 @@ struct drm_bridge {
 	 */
 	unsigned int hdmi_audio_spdif_playback : 1;
 
+	/**
+	 * @hdmi_audio_prepare_early: configure the sink's audio path from the
+	 * ASoC hw_params callback rather than from prepare.
+	 *
+	 * Both callbacks receive identical parameters, so for most bridges it
+	 * makes no difference and the default (prepare) is right. It matters
+	 * when the CPU side of the DAI link cannot start until the sink's
+	 * audio clock is already running: ASoC walks CPU DAIs before codec
+	 * DAIs and stops at the first error, so a CPU DAI that fails waiting
+	 * for that clock aborts the loop before this bridge's prepare is ever
+	 * reached. Qualcomm's LPASS/ADSP is one such CPU side.
+	 *
+	 * Setting this moves the configuration to hw_params, which runs for
+	 * every DAI in the link before any of them is prepared.
+	 */
+	unsigned int hdmi_audio_prepare_early : 1;
+
 	/**
 	 * @hdmi_audio_dai_port: sound DAI port for either of
 	 * @DRM_BRIDGE_OP_HDMI_AUDIO and @DRM_BRIDGE_OP_DP_AUDIO, -1 if it is
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 2ee7c5932..749eab1f9 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;
 };
 
 /*
-- 
2.56.0

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

[sashiko-bot]

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

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

[Dmitry Baryshkov]

On Mon, Oct 05, 2026 at 06:18:24PM +0200, Jean-Francois Bobier wrote:
> drm_connector_hdmi_audio_ops implements .prepare and not .hw_params, so
> every bridge using the HDMI audio helper configures its sink's audio path
> from the codec DAI's prepare callback.
> 
> That is too late when the CPU side of the DAI link cannot start until the
> sink's audio clock is already running. ASoC's snd_soc_pcm_dai_prepare()
> walks CPU DAIs before codec DAIs and returns on the first error, so such
> a CPU DAI fails and aborts the loop before the bridge's prepare is
> reached -- the clock it was waiting for is enabled by a callback that the
> failure prevents from running.
> 
> Qualcomm's LPASS is one such CPU side. On an sm8250 board with the
> DisplayPort controller bound to the DISPLAY_PORT_RX AFE port, starting
> playback gives:
> 
>   qcom-q6afe: AFE enable for port 0x6020 failed -110
>   q6afe-dai: ASoC error (-110): at snd_soc_dai_prepare() on DISPLAY_PORT_RX_0
> 
> and an ftrace of the attempt shows msm_dp_audio_prepare() is never
> called at all -- only msm_dp_audio_shutdown(), from the teardown path.
> 
> Add a .hw_params to the helper that runs the same configuration, and a
> drm_bridge flag to select it. hw_params runs for every DAI in the link
> before any of them is prepared, so the clock is up by the time the CPU
> DAI starts its port. Both callbacks receive identical parameters, so the
> configuration itself is unchanged.

Similar changes were proposed several times already. The IEC958 status
bits are usually set by the userspace after the .hw_params callback has
been called. With your approach the hardware will never receive correct
status bits.

If my understanding is correct, a more apropriate fix is to make the
a6afe driver to emit AFE enables at the .trigger point instead (maybe
together with other commands) or to split the DP audio init, moving the
necessary bits from the dp_audio_prepare to the dp_audio_startup
callback.

> 
> The flag defaults to off: a bridge that does not set it keeps being
> configured from prepare exactly as before. For one that does, prepare
> skips calling the sink's configuration again only for the specific call
> hw_params already did it for -- tracked per-connector, consumed on use --
> because ASoC reaches .prepare with no .hw_params ahead of it on XRUN
> recovery and on resume from suspend, and the sink still needs configuring
> then. Skipping unconditionally would have left it silently unconfigured
> after either.
> 
> Note that providing .hw_params at all means hdmi_codec_hw_params() no
> longer returns early for these connectors, so it now runs
> hdmi_codec_fill_codec_params() and the IEC958 channel status fill before
> the (inert, when nothing needs doing) callback. Both are computed into an
> on-stack hdmi_codec_params that is then discarded, and the one shared
> value written, daifmt->bit_fmt, is set by hdmi_codec_prepare() to the
> same value immediately afterwards.
> 
> One more side effect worth disclosing: hdmi_codec_fill_codec_params()
> also writes hcp->chmap_idx, which is not on-stack -- it persists on the
> hdmi-codec device and is reused by the next call to
> hdmi_codec_get_ch_alloc_table_idx(). hdmi_codec_prepare() recomputes it
> immediately afterwards from the same channel count, so the value itself
> does not go stale. But hdmi_codec_hw_params() now reaching this code at
> all means that a channel count the sink's ELD does not advertise is
> reported from hw_params() instead of prepare(), for every bridge sharing
> drm_connector_hdmi_audio_ops, not only the ones that set
> hdmi_audio_prepare_early. That changes when a bad channel count is
> reported, not whether it is, but it is a change to seven bridges this
> patch has no other reason to touch, and worth a maintainer's opinion.
> 
> Signed-off-by: Jean-Francois Bobier <jean-francois.bobier@laposte.net>

Please take a look at Documentation/process/submitting-patches.rst and
likely Documentation/process/coding-assistants.rst. For example, don't
send the new version of the patch as a response to the previous
revision.


-- 
With best wishes
Dmitry

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

[Jean-Francois Bobier]

On Mon, Oct 05, 2026 at 06:18:24PM +0200, Jean-Francois Bobier wrote:
[...]

 > Similar changes were proposed several times already. The IEC958 status
 > bits are usually set by the userspace after the .hw_params callback has
 > been called. With your approach the hardware will never receive correct
 > status bits.

That's a real concern in general, but I don't think it applies to the
case this patch is actually fixing. hdmi_codec_iec958_default_put()
is optional -- hcp->iec_status already has a sane default from
snd_pcm_create_iec958_consumer_default() at init, and both
hdmi_codec_hw_params() and hdmi_codec_prepare() fill it from that same
baseline. Userspace only needs to override it for passthrough of
compressed formats, where IEC958_AES0_NONAUDIO has to be set correctly
before the stream opens.

For ordinary PCM playback -- which is the only thing DP/HDMI audio on
this board does -- nothing in the userspace stack ever touches that
control. I checked our actual UCM profile rather than assume: the HDMI
device there is a plain mixer enable and PlaybackChannels 2, nothing
IEC958-related. So the ordering you're describing matters for
passthrough users, but doesn't affect the case I have hardware for.

 > If my understanding is correct, a more apropriate fix is to make the
 > a6afe driver to emit AFE enables at the .trigger point instead (maybe
 > together with other commands) or to split the DP audio init, moving the
 > necessary bits from the dp_audio_prepare to the dp_audio_startup
 > callback.

I looked at q6afe-dai.c before replying. It doesn't implement .trigger
at all today, for any of its port types -- HDMI/DP, SLIMBUS, MI2S, TDM,
WSA codec DMA, USB all go through the same q6afe_dai_prepare(). Adding
.trigger there to fix DP timing would change behavior for every one of
those, on hardware I have no way to test.

The startup/prepare split is narrower, but I don't think it's free
either: .startup/.shutdown are tied to device open/close, not to each
hw_params/prepare cycle, so I'm not sure it actually survives a
suspend/resume any better than what this patch does -- it may just
relocate the same question rather than close it. Possibly worth
someone who knows this driver better confirming either way.

Given that, I'd rather not drop this while something that actually
works waits on a rewrite of a shared CPU DAI driver that doesn't exist
yet. Happy to be told I'm wrong about either of the above if I'm
missing something.

 > Please take a look at Documentation/process/submitting-patches.rst and
 > likely Documentation/process/coding-assistants.rst. For example, don't
 > send the new version of the patch as a response to the previous
 > revision.

Noted on the threading -- that's on me, I'll send revisions as fresh
threads with a link to the prior version instead. I'm not used to 
mail-based workflows like this in my work :(

Please let me know what you think - the patch does work on my board and 
brought up DP audio 100% working, but I would understand if it's not 
worth upstreaming given architectural impacts. (The bot seems to have 
found other gotchas on my v2 patch already so I will hold off until it's 
confirmed it's worth upstreaming at all).

Jean-Francois

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

[Dmitry Baryshkov]

On Wed, Oct 07, 2026 at 10:46:06AM +0200, Jean-Francois Bobier wrote:
> On Mon, Oct 05, 2026 at 06:18:24PM +0200, Jean-Francois Bobier wrote:
> [...]
> 
> > Similar changes were proposed several times already. The IEC958 status
> > bits are usually set by the userspace after the .hw_params callback has
> > been called. With your approach the hardware will never receive correct
> > status bits.
> 
> That's a real concern in general, but I don't think it applies to the
> case this patch is actually fixing. hdmi_codec_iec958_default_put()
> is optional -- hcp->iec_status already has a sane default from
> snd_pcm_create_iec958_consumer_default() at init, and both
> hdmi_codec_hw_params() and hdmi_codec_prepare() fill it from that same
> baseline. Userspace only needs to override it for passthrough of
> compressed formats, where IEC958_AES0_NONAUDIO has to be set correctly
> before the stream opens.
> 
> For ordinary PCM playback -- which is the only thing DP/HDMI audio on
> this board does -- nothing in the userspace stack ever touches that
> control. I checked our actual UCM profile rather than assume: the HDMI
> device there is a plain mixer enable and PlaybackChannels 2, nothing
> IEC958-related. So the ordering you're describing matters for
> passthrough users, but doesn't affect the case I have hardware for.

That's the difference between the framework and a single driver. At a
framework level we'd like to ensure that it doesn't let wrong values to
be programmed and transmitted to the receiver. Moreover, I don't think
there is anything preventing AC3 passthrough from working on Qualcomm
hardware (and then it becomes an issue).

> 
> > If my understanding is correct, a more apropriate fix is to make the
> > a6afe driver to emit AFE enables at the .trigger point instead (maybe
> > together with other commands) or to split the DP audio init, moving the
> > necessary bits from the dp_audio_prepare to the dp_audio_startup
> > callback.
> 
> I looked at q6afe-dai.c before replying. It doesn't implement .trigger
> at all today, for any of its port types -- HDMI/DP, SLIMBUS, MI2S, TDM,
> WSA codec DMA, USB all go through the same q6afe_dai_prepare(). Adding
> .trigger there to fix DP timing would change behavior for every one of
> those, on hardware I have no way to test.

Srinivas, would you have any suggestions or recommendation?

> 
> The startup/prepare split is narrower, but I don't think it's free
> either: .startup/.shutdown are tied to device open/close, not to each
> hw_params/prepare cycle, so I'm not sure it actually survives a
> suspend/resume any better than what this patch does -- it may just
> relocate the same question rather than close it. Possibly worth
> someone who knows this driver better confirming either way.
> 
> Given that, I'd rather not drop this while something that actually
> works waits on a rewrite of a shared CPU DAI driver that doesn't exist
> yet. Happy to be told I'm wrong about either of the above if I'm
> missing something.
> 

[...]

> 
> Please let me know what you think - the patch does work on my board and
> brought up DP audio 100% working, but I would understand if it's not worth
> upstreaming given architectural impacts. (The bot seems to have found other
> gotchas on my v2 patch already so I will hold off until it's confirmed it's
> worth upstreaming at all).

I think the issue is worth tracking and fixing, but at a different
place. I've pinged my colleague, let's see if he will have any detailed
suggestions.

-- 
With best wishes
Dmitry