Re: [PATCH v12 22/23] drm/bridge: synopsys: dw-dp: Add audio support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] drm/bridge: dw-dp: unselected audio clock disabled before mux
  switch
- [Medium] drm/bridge: dw-dp: unadvertised sample_width limit breaks
  32-bit I2S formats
- [Medium] drm/bridge: dw-dp: checking params->cea.channels breaks
  non-PCM audio
- [Medium] drm/bridge: dw-dp: missing
  drm_connector_hdmi_audio_plugged_notify() calls
- [Low] drm/bridge: dw-dp: invalid hdmi_audio_dai_port in dw_dp_probe()
--

commit 20fac3cf1ff3b933dbf4d3b4b4b914b2c8a55eb1
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

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

This commit adds DisplayPort audio support to the Synopsys DesignWare
DisplayPort bridge driver using the DRM bridge audio callbacks.

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

[ ... ]

> @@ -1696,6 +1727,261 @@ static int dw_dp_link_enable(struct dw_dp *dp)
>  	return ret;
>  }

[ ... ]

> +static int __dw_dp_audio_enable(struct dw_dp *dp)
> +{
> +	u8 audio_data_in_en;
> +
> +	switch (dp->audio_channels) {
> +	case 1:
> +	case 2:
> +		audio_data_in_en = AUDIO_DATA_IN_EN_CHANNEL12;
> +		break;
> +	case 8:
> +		audio_data_in_en = AUDIO_DATA_IN_EN_CHANNEL12 |
> +				   AUDIO_DATA_IN_EN_CHANNEL34 |
> +				   AUDIO_DATA_IN_EN_CHANNEL56 |
> +				   AUDIO_DATA_IN_EN_CHANNEL78;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	regmap_update_bits(dp->regmap, DW_DP_AUD_CONFIG1,
> +			   AUDIO_DATA_IN_EN | NUM_CHANNELS | AUDIO_DATA_WIDTH |
> +			   AUDIO_INF_SELECT | HBR_MODE_ENABLE | AUDIO_MUTE,
> +			   FIELD_PREP(AUDIO_DATA_IN_EN, audio_data_in_en) |
> +			   FIELD_PREP(NUM_CHANNELS, dp->audio_channels - 1) |
> +			   FIELD_PREP(AUDIO_DATA_WIDTH, dp->audio_sample_width) |
> +			   FIELD_PREP(AUDIO_INF_SELECT, dp->audio_interface) |
> +			   FIELD_PREP(HBR_MODE_ENABLE, 0) |
> +			   FIELD_PREP(AUDIO_MUTE, dp->audio_muted));
> +
> +	/* Wait for inf switch */
> +	usleep_range(20, 40);

[ ... ]

> +static int dw_dp_audio_prepare(struct drm_bridge *bridge,
> +			       struct drm_connector *connector,
> +			       struct hdmi_codec_daifmt *daifmt,
> +			       struct hdmi_codec_params *params)
> +{
> +	struct dw_dp *dp = bridge_to_dp(bridge);
> +	u8 supported_audio_interfaces;
> +	enum dw_dp_audio_interface audio_interface;
> +	u32 cfg1;
> +	int ret;
> +
> +	guard(mutex)(&dp->audio_lock);
> +
> +	/*
> +	 * prepare might be called multiple times, so release the clocks
> +	 * from previous calls to keep the calls in balance.
> +	 */
> +	if (dp->audio_interface != DW_DP_AUDIO_UNUSED)
> +		__dw_dp_audio_disable(dp);
> +
> +	/* The hardware is limited to 1,2 or 8 channels */
> +	switch (params->cea.channels) {
> +	case 1:
> +	case 2:
> +	case 8:
> +		break;
> +	default:
> +		dev_err(dp->dev, "invalid audio channels %d\n", params->cea.channels);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Should dw_dp_audio_prepare() check and store params->channels instead of
params->cea.channels?

In hdmi_codec_fill_codec_params(), params->cea.channels is set to 0 when
IEC958_AES0_NONAUDIO is set in the IEC958 status bits (meaning "refer to
stream header" in the CTA-861 Audio InfoFrame), while params->channels holds
the actual stream channel count.  Using params->cea.channels causes
dw_dp_audio_prepare() to reject non-PCM streams with -EINVAL, and would also
program NUM_CHANNELS and AUDIO_DATA_IN_EN in DW_DP_AUD_CONFIG1 with the
InfoFrame field rather than the stream channel count.

> +
> +	if (params->sample_width < 16 || params->sample_width > 24) {
> +		dev_err(dp->dev, "invalid data sample width %d\n", params->sample_width);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Does rejecting sample_width > 24 here break 32-bit formats advertised on the
I2S DAI?

In dw_dp_probe(), bridge->hdmi_audio_i2s_formats is left unset, so
hdmi_codec_probe() falls back to its default I2S_FORMATS mask, which
includes SNDRV_PCM_FMTBIT_S32_LE and SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE
(used by ALSA's iec958 plugin to insert PCUV bits in I2S mode).
hdmi_codec_prepare() sets params->sample_width to
snd_pcm_format_width(runtime->format), which is 32 for both formats, causing
dw_dp_audio_prepare() to fail with -EINVAL after hw_params succeeds.
Should bridge->hdmi_audio_i2s_formats be restricted to 16-bit through 24-bit
formats?

> +
> +	switch (daifmt->fmt) {
> +	case HDMI_SPDIF:
> +		audio_interface = DW_DP_AUDIO_SPDIF;
> +		break;
> +	case HDMI_I2S:
> +		/*
> +		 * It is recommended to use SPDIF instead of I2S, since I2S mode requires
> +		 * manually inserting PCUV control bits from userspace and this is done
> +		 * automatically in hardware for SPDIF mode.
> +		 */
> +		audio_interface = DW_DP_AUDIO_I2S;
> +		break;
> +	default:
> +		dev_err(dp->dev, "invalid DAI format %d\n", daifmt->fmt);
> +		return -EINVAL;
> +	}
> +
> +	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;
> +	}
> +
> +	if (audio_interface == DW_DP_AUDIO_I2S)
> +		clk_disable_unprepare(dp->spdif_clk);
> +	else if (audio_interface == DW_DP_AUDIO_SPDIF)
> +		clk_disable_unprepare(dp->i2s_clk);
> +
> +	dp->audio_channels = params->cea.channels;
> +	dp->audio_channel_allocation = params->cea.channel_allocation;
> +	dp->audio_sample_width = params->sample_width;
> +	dp->audio_interface = audio_interface;
> +
> +	ret = __dw_dp_audio_enable(dp);

[Severity: Medium]
Does disabling the unselected audio clock before calling
__dw_dp_audio_enable() defeat keeping both clocks running across the
AUDIO_INF_SELECT mux switch?

Both dp->spdif_clk and dp->i2s_clk are enabled here and the unselected clock
is immediately disabled before any register access occurs.  The actual
AUDIO_INF_SELECT update in DW_DP_AUD_CONFIG1 and the 20-40 us interface
switch delay happen inside __dw_dp_audio_enable(), which is also called from
dw_dp_bridge_atomic_enable() after dw_dp_reset() resets AUDIO_INF_SELECT to
0 (I2S).  If the hardware mux requires both clocks to be active while
switching AUDIO_INF_SELECT, both clocks should be enabled around the
DW_DP_AUD_CONFIG1 update and usleep_range(20, 40) inside
__dw_dp_audio_enable().

[ ... ]

> @@ -1734,12 +2020,22 @@ 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);
> +			if (ret < 0)
> +				dev_err(dp->dev, "failed to restore audio: %d\n", ret);
> +		}
> +	}
>  }

[Severity: Medium]
Should the driver call drm_connector_hdmi_audio_plugged_notify() when the
DisplayPort link is enabled, disabled, or hotplugged?

The patch includes drm/display/drm_hdmi_audio_helper.h and sets
DRM_BRIDGE_OP_DP_AUDIO, but never calls
drm_connector_hdmi_audio_plugged_notify().  Because drm_bridge_connector
only calls drm_atomic_helper_connector_hdmi_hotplug() automatically when
DRM_BRIDGE_OP_HDMI is set, hdmi-codec's plugged_cb() is never invoked for
this bridge.  As a result, ASoC jack detection (SND_JACK_AVOUT) is never
signaled and the ALSA ELD control is not updated on hotplug before a PCM
stream is opened.

[ ... ]

> @@ -2368,12 +2673,21 @@ int dw_dp_probe(struct dw_dp *dp)
>  		return ret;
>  
>  	dp->vsc_sdp_nr = -1;
> +	dp->audio_interface = DW_DP_AUDIO_UNUSED;
> +	dp->audio_sdp_nr = -1;
>  
>  	bridge = &dp->bridge;
>  	bridge->of_node = dev->of_node;
> -	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
> +	bridge->ops = DRM_BRIDGE_OP_DP_AUDIO |
> +		      DRM_BRIDGE_OP_DETECT |
> +		      DRM_BRIDGE_OP_EDID |
> +		      DRM_BRIDGE_OP_HPD;
>  	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
>  	bridge->ycbcr_420_allowed = true;
> +	bridge->hdmi_audio_dev = dev;
> +	bridge->hdmi_audio_max_i2s_playback_channels = 8;
> +	bridge->hdmi_audio_dai_port = 1;
> +	bridge->hdmi_audio_spdif_playback = true;

[Severity: Low]
Should bridge->hdmi_audio_dai_port be set to -1 instead of 1?

In rockchip,dw-dp.yaml (and in dw_dp_probe() via
of_drm_get_bridge_by_endpoint(dev->of_node, 1, 0)), OF-graph port 1 is the
DisplayPort video output port, while audio DAIs are referenced via
#sound-dai-cells = <1> rather than an OF-graph audio port.  Per struct
drm_bridge documentation, hdmi_audio_dai_port should be -1 when an OF-graph
audio port is not used, otherwise drm_connector_hdmi_audio_get_dai_id() will
match the video output port.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-synopsys-dw-dp-improvements-v12-0-d42d3dc5bdd4@collabora.com?part=22