Re: [PATCH v7 18/18] drm/bridge: synopsys: dw-dp: Add audio support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Multichannel audio channel allocation is hardcoded to 0 (stereo), breaking surround sound routing.
- [High] Audio InfoFrame can steal SDP slot 0, which is hardcoded for Video Stream Configuration (VSC), corrupting the VSC packet if ALSA prepares audio before the display is enabled.
- [Medium] Unconditional attempt to enable both I2S and SPDIF clocks causes valid audio configurations to fail if the unused clock cannot be enabled.
- [Medium] The audio mute state (AUDIO_MUTE) is lost across DisplayPort controller resets during atomic_disable/atomic_enable cycles.
--

--- Patch [18]: [PATCH v7 18/18] drm/bridge: synopsys: dw-dp: Add audio support ---
commit 5e1e4e31e16d403e05ea9b2201464ef6e1a35fc1
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Add audio support

Implement audio support for the Synopsys DesignWare DisplayPort controller.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index c0026bd4c7b53..3b73e822478d7 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -1679,6 +1708,247 @@ static int dw_dp_link_enable(struct dw_dp *dp)
>  	return ret;
>  }
>  
> +static int dw_dp_audio_infoframe_send(struct dw_dp *dp)
> +{
> +	struct hdmi_audio_infoframe frame;
> +	struct dw_dp_sdp sdp;
> +	int ret;
> +
> +	ret = hdmi_audio_infoframe_init(&frame);

[Severity: High]
The hdmi_audio_infoframe_init() function initializes the frame to zero, which
implicitly sets the channel allocation to 0 (stereo).

Since dw_dp_audio_prepare() receives the channel allocation in
params->cea.channel_allocation but does not save it, will multi-channel audio
(like 5.1 or 7.1) be transmitted incorrectly as stereo, potentially causing
the AV receiver to drop or improperly route channels?

> +	if (ret < 0)
> +		return ret;
> +
> +	frame.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
> +	frame.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
> +	frame.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
> +	frame.channels = dp->audio_channels;

[ ... ]

> +static int __dw_dp_audio_enable(struct dw_dp *dp)
> +{
> +	u8 audio_data_in_en;
> +
> +	switch (dp->audio_channels) {

[ ... ]

> +	}
> +
> +	regmap_update_bits(dp->regmap, DW_DP_AUD_CONFIG1,
> +			   AUDIO_DATA_IN_EN | NUM_CHANNELS | AUDIO_DATA_WIDTH |
> +			   AUDIO_INF_SELECT | HBR_MODE_ENABLE,

[Severity: Medium]
The dw_dp_audio_mute_stream() function sets the AUDIO_MUTE bit in hardware,
but it does not save this state in the software context.

If the display is disabled and re-enabled (causing a hardware reset),
__dw_dp_audio_enable() restores DW_DP_AUD_CONFIG1 here but does not include
AUDIO_MUTE in its update mask.

Will an intentionally muted audio stream suddenly become unmuted after a
display power cycle?

> +			   FIELD_PREP(AUDIO_DATA_IN_EN, audio_data_in_en) |
> +			   FIELD_PREP(NUM_CHANNELS, dp->audio_channels - 1) |

[ ... ]

> +static int dw_dp_audio_prepare(struct drm_bridge *bridge,
> +			       struct drm_connector *connector,
> +			       struct hdmi_codec_daifmt *daifmt,
> +			       struct hdmi_codec_params *params)
> +{

[ ... ]

> +	regmap_read(dp->regmap, DW_DP_CONFIG_REG1, &cfg1);
> +	supported_audio_interfaces = FIELD_GET(AUDIO_SELECT, cfg1);
> +
> +	if (supported_audio_interfaces != DW_DP_AUDIO_I2S_AND_SPDIF &&
> +	    supported_audio_interfaces != audio_interface) {
> +		dev_err(dp->dev, "unsupported DAI %d\n", daifmt->fmt);
> +		return -EINVAL;
> +	}
> +
> +	ret = clk_prepare_enable(dp->spdif_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(dp->i2s_clk);
> +	if (ret) {
> +		clk_disable_unprepare(dp->spdif_clk);
> +		return ret;
> +	}

[Severity: Medium]
This code attempts to enable both the spdif_clk and i2s_clk unconditionally,
returning an error if either fails.

If a platform has one of these clocks defined in the device tree but it
temporarily or permanently fails to enable, will this cause the entire audio
preparation to fail even if the user only requested the other (working) audio
interface?

> +
> +	if (audio_interface == DW_DP_AUDIO_I2S)
> +		clk_disable_unprepare(dp->spdif_clk);

[ ... ]

> @@ -1720,6 +1990,14 @@ static void dw_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  		dev_err(dp->dev, "failed to enable video: %d\n", ret);
>  		return;
>  	}
> +
> +	scoped_guard(mutex, &dp->audio_lock) {
> +		if (dp->audio_interface != DW_DP_AUDIO_UNUSED) {
> +			ret = __dw_dp_audio_enable(dp);

[Severity: High]
If userspace prepares audio while the display is disabled, dw_dp_send_sdp()
might allocate the first free slot (slot 0) for the Audio InfoFrame.

Earlier in this function, dw_dp_bridge_atomic_enable() hardcodes a reservation
for slot 0 for the Video Stream Configuration (VSC) packet:

	scoped_guard(mutex, &dp->sdp_lock)
		set_bit(0, dp->sdp_reg_bank);

Does calling __dw_dp_audio_enable() here then clear slot 0 (wiping out the VSC
packet) and reallocate it to audio since audio previously held slot 0?

> +			if (ret < 0)
> +				dev_err(dp->dev, "failed to restore audio: %d\n", ret);
> +		}
> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-synopsys-dw-dp-improvements-v7-0-b7640fa8cf48@collabora.com?part=18