[PATCH 0/2] drm/bridge: it6505: DP audio support + shared-DAI hw_params fix

[PATCH 0/2] drm/bridge: it6505: DP audio support + shared-DAI hw_params fix

[Daniel Golle]

This revives Jiaxin Yu's stalled "drm/bridge: it6505: Add audio support"
v3 [1], rebased onto current mainline, plus the fix needed to make it work
on boards where the it6505 DP output shares an I2S DAI with another codec.

Patch 1 wires up the previously-unused it6505 audio helpers via
hdmi_codec_ops, which unblocks the mt8186-mt6366 sound card that
references it6505 as the I2S3 codec.

Patch 2 stops it6505_audio_setup_hw_params() from returning -ENODEV when
no DP display is attached, which would otherwise tear down the shared
backend and break playback on the co-located codec (the speaker).

Tested on a MediaTek MT8186 (google,steelix) Chromebook: speaker and
built-in mic work. Actual HDMI/DP audio output could not be tested as the
sample exposes no usable DP-audio port.

[1] https://lore.kernel.org/all/20230730180803.22570-4-jiaxin.yu@mediatek.com/
Daniel Golle (1):
  drm/bridge: it6505: Don't reject audio hw_params without an encoder

Jiaxin Yu (1):
  drm/bridge: it6505: Add audio support

 drivers/gpu/drm/bridge/ite-it6505.c | 94 +++++++++++++++++++++++------
 1 file changed, 77 insertions(+), 17 deletions(-)


base-commit: 0718283ab28bc3907e10b61a6b4be6fefa1cbb2f
prerequisite-patch-id: 0000000000000000000000000000000000000000
-- 
2.55.0

[PATCH 1/2] drm/bridge: it6505: Add audio support

[Daniel Golle]

From: Jiaxin Yu <jiaxin.yu@mediatek.com>

Add audio support for it6505 by bridging to the hdmi-codec, registering
an "hdmi-audio-codec" platform device from probe. The it6505's audio
setup/shutdown helpers were merged earlier as unused code; wire them up
via hdmi_codec_ops so the DAI actually appears, which unblocks the
mt8186-mt6366 sound card that references it6505 as the I2S3 codec.

Some DP-to-HDMI dongles get into a bad state if InfoFrame is sent
without audio data, so it6505's audio is only enabled once the stream
is unmuted.

[djg: respin of Jiaxin Yu's v3; rebased onto current tree; the driver
 gained it6505->dev so drop the &client->dev usages; hdmi_codec_ops no
 longer has .trigger, so enable/disable via .mute_stream instead; drop
 now-redundant __maybe_unused on the wired-up helpers]

Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
Link: https://lore.kernel.org/all/20230730180803.22570-4-jiaxin.yu@mediatek.com/
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 92 ++++++++++++++++++++++++-----
 1 file changed, 76 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 8ecb43611dba..698ae01b3a47 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2320,19 +2320,12 @@ static void it6505_stop_link_train(struct it6505 *it6505)
 
 static void it6505_link_train_ok(struct it6505 *it6505)
 {
-	struct device *dev = it6505->dev;
-
 	it6505->link_state = LINK_OK;
 	/* disalbe mute enable avi info frame */
 	it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_VID_MUTE, 0x00);
 	it6505_set_bits(it6505, REG_INFOFRAME_CTRL,
 			EN_VID_CTRL_PKT, EN_VID_CTRL_PKT);
 
-	if (it6505_audio_input(it6505)) {
-		DRM_DEV_DEBUG_DRIVER(dev, "Enable audio!");
-		it6505_enable_audio(it6505);
-	}
-
 	if (it6505->hdcp_desired)
 		it6505_start_hdcp(it6505);
 }
@@ -2960,7 +2953,7 @@ static void it6505_remove_notifier_module(struct it6505 *it6505)
 	}
 }
 
-static void __maybe_unused it6505_delayed_audio(struct work_struct *work)
+static void it6505_delayed_audio(struct work_struct *work)
 {
 	struct it6505 *it6505 = container_of(work, struct it6505,
 					     delayed_audio.work);
@@ -2974,9 +2967,9 @@ static void __maybe_unused it6505_delayed_audio(struct work_struct *work)
 		it6505_enable_audio(it6505);
 }
 
-static int __maybe_unused it6505_audio_setup_hw_params(struct it6505 *it6505,
-						       struct hdmi_codec_params
-						       *params)
+static int it6505_audio_setup_hw_params(struct it6505 *it6505,
+					struct hdmi_codec_params
+					*params)
 {
 	struct device *dev = it6505->dev;
 	int i = 0;
@@ -3031,7 +3024,7 @@ static int __maybe_unused it6505_audio_setup_hw_params(struct it6505 *it6505,
 	return 0;
 }
 
-static void __maybe_unused it6505_audio_shutdown(struct device *dev, void *data)
+static void it6505_audio_shutdown(struct device *dev, void *data)
 {
 	struct it6505 *it6505 = dev_get_drvdata(dev);
 
@@ -3039,10 +3032,40 @@ static void __maybe_unused it6505_audio_shutdown(struct device *dev, void *data)
 		it6505_disable_audio(it6505);
 }
 
-static int __maybe_unused it6505_audio_hook_plugged_cb(struct device *dev,
-						       void *data,
-						       hdmi_codec_plugged_cb fn,
-						       struct device *codec_dev)
+static int it6505_audio_hw_params(struct device *dev, void *data,
+				  struct hdmi_codec_daifmt *daifmt,
+				  struct hdmi_codec_params *params)
+{
+	struct it6505 *it6505 = dev_get_drvdata(dev);
+
+	return it6505_audio_setup_hw_params(it6505, params);
+}
+
+static int it6505_audio_mute(struct device *dev, void *data,
+			     bool enable, int direction)
+{
+	struct it6505 *it6505 = dev_get_drvdata(dev);
+
+	DRM_DEV_DEBUG_DRIVER(dev, "mute: %d", enable);
+
+	/*
+	 * Some DP-to-HDMI dongles get into a bad state if the InfoFrame is
+	 * sent without audio data, so only enable it6505's audio once the
+	 * stream is unmuted (i.e. actually playing).
+	 */
+	if (enable)
+		cancel_delayed_work(&it6505->delayed_audio);
+	else
+		queue_delayed_work(system_wq, &it6505->delayed_audio,
+				   msecs_to_jiffies(180));
+
+	return 0;
+}
+
+static int it6505_audio_hook_plugged_cb(struct device *dev,
+					void *data,
+					hdmi_codec_plugged_cb fn,
+					struct device *codec_dev)
 {
 	struct it6505 *it6505 = data;
 
@@ -3053,6 +3076,37 @@ static int __maybe_unused it6505_audio_hook_plugged_cb(struct device *dev,
 	return 0;
 }
 
+static const struct hdmi_codec_ops it6505_audio_codec_ops = {
+	.hw_params = it6505_audio_hw_params,
+	.mute_stream = it6505_audio_mute,
+	.audio_shutdown = it6505_audio_shutdown,
+	.hook_plugged_cb = it6505_audio_hook_plugged_cb,
+};
+
+static int it6505_register_audio_driver(struct device *dev)
+{
+	struct it6505 *it6505 = dev_get_drvdata(dev);
+	struct hdmi_codec_pdata codec_data = {
+		.ops = &it6505_audio_codec_ops,
+		.max_i2s_channels = 8,
+		.i2s = 1,
+		.no_capture_mute = 1,
+		.data = it6505,
+	};
+	struct platform_device *pdev;
+
+	pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME,
+					     PLATFORM_DEVID_AUTO, &codec_data,
+					     sizeof(codec_data));
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	INIT_DELAYED_WORK(&it6505->delayed_audio, it6505_delayed_audio);
+	DRM_DEV_DEBUG_DRIVER(dev, "bound to %s", HDMI_CODEC_DRV_NAME);
+
+	return 0;
+}
+
 static inline struct it6505 *bridge_to_it6505(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct it6505, bridge);
@@ -3612,6 +3666,12 @@ static int it6505_i2c_probe(struct i2c_client *client)
 		return err;
 	}
 
+	err = it6505_register_audio_driver(dev);
+	if (err < 0) {
+		dev_err(dev, "Failed to register audio driver: %d", err);
+		return err;
+	}
+
 	INIT_WORK(&it6505->link_works, it6505_link_training_work);
 	INIT_WORK(&it6505->hdcp_wait_ksv_list, it6505_hdcp_wait_ksv_list);
 	INIT_DELAYED_WORK(&it6505->hdcp_work, it6505_hdcp_work);
-- 
2.55.0

[PATCH 2/2] drm/bridge: it6505: Don't reject audio hw_params without an encoder

[Daniel Golle]

it6505_audio_setup_hw_params() returns -ENODEV when the bridge is not
attached to a DRM encoder. Now that it6505 registers an hdmi-audio-codec,
this callback runs whenever a stream is configured on the I2S DAI it is
wired to, including when that DAI is shared with another codec. On
mt8186-corsola the speaker amplifier (rt1019) and it6505 share I2S3, so
when the it6505 DP output has no display attached (bridge.encoder is
NULL) the -ENODEV propagates up through dpcm_be_dai_hw_params() and tears
down the whole backend, breaking speaker playback.

A codec must not fail hw_params on a shared DAI just because its own sink
is absent; the actual output is already gated by it6505->powered. Return
0 so the shared link can be configured.

Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 698ae01b3a47..e88960c82a23 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2979,7 +2979,7 @@ static int it6505_audio_setup_hw_params(struct it6505 *it6505,
 			     params->cea.channels);
 
 	if (!it6505->bridge.encoder)
-		return -ENODEV;
+		return 0;
 
 	if (params->cea.channels <= 1 || params->cea.channels > 8) {
 		DRM_DEV_DEBUG_DRIVER(dev, "channel number: %d not support",
-- 
2.55.0

Re: [PATCH 1/2] drm/bridge: it6505: Add audio support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] Resource leak and Use-After-Free of hdmi-codec platform device
- [High] Missing synchronous cancellation of `delayed_audio` on driver removal causes Use-After-Free
- [High] Pending `delayed_audio` work is not cancelled during ALSA audio shutdown
- [High] `delayed_audio` workqueue is initialized after device registration
- [High] Concurrent I2C operations in audio enable/disable lack synchronization
- [Medium] Audio workaround logic fails to disable audio when muted

Pre-existing issues:
- [High] Pre-existing missing synchronous cancellation of HDCP work on driver removal
--

--- Patch [1]: [PATCH 1/2] drm/bridge: it6505: Add audio support ---
commit 645bfa307d3295ef683ceb3cf17d99b78c9a13a0
Author: Jiaxin Yu <jiaxin.yu@mediatek.com>

drm/bridge: it6505: Add audio support

Add audio support for it6505 by bridging to the hdmi-codec, registering
an "hdmi-audio-codec" platform device from probe. The it6505's audio
setup/shutdown helpers were merged earlier as unused code; wire them up
via hdmi_codec_ops so the DAI actually appears.

Link: https://lore.kernel.org/all/20230730180803.22570-4-jiaxin.yu@mediatek.com/

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 8ecb43611dba8..698ae01b3a474 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
[ ... ]
> @@ -2960,7 +2953,7 @@ static void it6505_remove_notifier_module(struct it6505 *it6505)
>  	}
>  }
>  
> -static void __maybe_unused it6505_delayed_audio(struct work_struct *work)
> +static void it6505_delayed_audio(struct work_struct *work)
>  {
>  	struct it6505 *it6505 = container_of(work, struct it6505,
>  					     delayed_audio.work);
> @@ -2974,9 +2967,9 @@ static void __maybe_unused it6505_delayed_audio(struct work_struct *work)
>  		it6505_enable_audio(it6505);
>  }

[Severity: High]
With the wiring up of delayed_audio and the hdmi_codec_ops,
it6505_enable_audio() and it6505_disable_audio() can now execute concurrently
from the system workqueue, ALSA process context, and threaded IRQs.

Since both functions perform multi-step unprotected hardware configuration,
could their I2C operations interleave and corrupt the audio configuration
registers? Is a mutex needed to protect these sequences?

[ ... ]
> -static void __maybe_unused it6505_audio_shutdown(struct device *dev, void *data)
> +static void it6505_audio_shutdown(struct device *dev, void *data)
>  {
>  	struct it6505 *it6505 = dev_get_drvdata(dev);
>  
> @@ -3039,10 +3032,40 @@ static void __maybe_unused it6505_audio_shutdown(struct device *dev, void *data)
>  		it6505_disable_audio(it6505);
>  }

[Severity: High]
Does this leave pending delayed work active?

If a user starts the audio stream and closes it within 180ms, the delayed work
might still execute after this shutdown completes, which would re-enable the
audio hardware on a closed stream.

Should this call cancel_delayed_work_sync(&it6505->delayed_audio) before
disabling the audio?

> -static int __maybe_unused it6505_audio_hook_plugged_cb(struct device *dev,
> -						       void *data,
> -						       hdmi_codec_plugged_cb fn,
> -						       struct device *codec_dev)
> +static int it6505_audio_hw_params(struct device *dev, void *data,
> +				  struct hdmi_codec_daifmt *daifmt,
> +				  struct hdmi_codec_params *params)
> +{
> +	struct it6505 *it6505 = dev_get_drvdata(dev);
> +
> +	return it6505_audio_setup_hw_params(it6505, params);
> +}
> +
> +static int it6505_audio_mute(struct device *dev, void *data,
> +			     bool enable, int direction)
> +{
> +	struct it6505 *it6505 = dev_get_drvdata(dev);
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "mute: %d", enable);
> +
> +	/*
> +	 * Some DP-to-HDMI dongles get into a bad state if the InfoFrame is
> +	 * sent without audio data, so only enable it6505's audio once the
> +	 * stream is unmuted (i.e. actually playing).
> +	 */
> +	if (enable)
> +		cancel_delayed_work(&it6505->delayed_audio);

[Severity: Medium]
Will this fail to disable audio when the stream is muted after being unmuted
for more than 180ms?

If enable is true (which typically means muting in ALSA semantics), and the
delayed work has already executed, canceling the work will do nothing.

Should this actively call it6505_disable_audio() if the stream is being muted?

Also, could using the non-synchronous cancel_delayed_work() introduce a race
condition if the worker is concurrently executing?

> +	else
> +		queue_delayed_work(system_wq, &it6505->delayed_audio,
> +				   msecs_to_jiffies(180));
> +
> +	return 0;
> +}
[ ... ]
> +static int it6505_register_audio_driver(struct device *dev)
> +{
> +	struct it6505 *it6505 = dev_get_drvdata(dev);
> +	struct hdmi_codec_pdata codec_data = {
> +		.ops = &it6505_audio_codec_ops,
> +		.max_i2s_channels = 8,
> +		.i2s = 1,
> +		.no_capture_mute = 1,
> +		.data = it6505,
> +	};
> +	struct platform_device *pdev;
> +
> +	pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME,
> +					     PLATFORM_DEVID_AUTO, &codec_data,
> +					     sizeof(codec_data));
> +	if (IS_ERR(pdev))
> +		return PTR_ERR(pdev);
> +
> +	INIT_DELAYED_WORK(&it6505->delayed_audio, it6505_delayed_audio);

[Severity: High]
Could this result in an uninitialized work structure being used?

If the ALSA driver binds and invokes the mute_stream callback immediately after
platform_device_register_data() returns, queue_delayed_work() could be called
before INIT_DELAYED_WORK() is executed.

[Severity: High]
Does this code leak the platform device on driver unbind?

The returned pdev is discarded, meaning platform_device_unregister() is never
called during driver teardown.

Since the platform device's .data pointer references the struct it6505 allocated
by the I2C driver, could this also lead to a use-after-free when the ALSA
subsystem interacts with the codec after the I2C driver is unbound?

[Severity: High]
When the I2C driver is removed, the newly added delayed_audio work does not
appear to be cancelled anywhere.

Since struct it6505 is freed by devres on unbind, could a pending delayed work
execute on the freed memory and cause a use-after-free?

[Severity: High]
This is a pre-existing issue, but similar to the delayed_audio work, the I2C
driver's remove path appears to be missing synchronous cancellation for
hdcp_work and hdcp_wait_ksv_list.

Could this also lead to a use-after-free if the module is removed while HDCP
authentication is ongoing?

> +	DRM_DEV_DEBUG_DRIVER(dev, "bound to %s", HDMI_CODEC_DRV_NAME);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784393979.git.daniel@makrotopia.org?part=1

Re: [PATCH 2/2] drm/bridge: it6505: Don't reject audio hw_params without an encoder

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized `channel_count` due to the early return can lead to an out-of-bounds stack array access in `it6505_enable_audio_infoframe()` if a hotplug race condition occurs.
- [Medium] Returning early when `bridge.encoder` is NULL bypasses audio parameter caching, causing DP audio to use stale or incorrect hardware parameters when a display is later hotplugged.
--

--- Patch [2]: [PATCH 2/2] drm/bridge: it6505: Don't reject audio hw_params without an encoder ---
commit 716c566c9be6893ad8cd2b80d06ce9aa0a5b67df
Author: Daniel Golle <daniel@makrotopia.org>

drm/bridge: it6505: Don't reject audio hw_params without an encoder

it6505_audio_setup_hw_params() returns -ENODEV when the bridge is not
attached to a DRM encoder. Now that it6505 registers an hdmi-audio-codec,
this callback runs whenever a stream is configured on the I2S DAI it is
wired to, including when that DAI is shared with another codec.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 698ae01b3a474..e88960c82a239 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c

[ ... ]

> @@ -2979,7 +2979,7 @@ static int it6505_audio_setup_hw_params(struct it6505 *it6505,
>  			     params->cea.channels);
>  
>  	if (!it6505->bridge.encoder)
> -		return -ENODEV;
> +		return 0;

[Severity: High]
Can this early return lead to an out-of-bounds stack array access during a
hotplug race condition?

If the early return prevents it6505->audio.channel_count from being
initialized, it will remain 0. If the stream is unmuted and delayed_audio is
queued, a subsequent DP hotplug within 180ms could unblock it6505_irq_hpd().

If delayed_audio fires before it6505_irq_hpd() executes
it6505_variable_config() to set channel_count to 2, it will call
it6505_enable_audio_infoframe() with channel_count=0:

it6505_enable_audio_infoframe() {
        ...
        it6505_write(it6505, REG_AUD_INFOFRAM_DB3,
                     audio_info_ca[it6505->audio.channel_count - 1]);
        ...
}

Would this result in reading audio_info_ca[-1] from the stack?


[Severity: Medium]
Will returning early here bypass the audio parameter caching?

If an ALSA stream on a shared DAI is played with non-default parameters
while DP is disconnected, it6505_audio_setup_hw_params() will return 0 and
skip updating the it6505->audio state.

When a DP display is later hotplugged, could it6505_irq_audio_fifo_error()
auto-enable audio using stale or default parameters instead of the active
stream's parameters?

>  
>  	if (params->cea.channels <= 1 || params->cea.channels > 8) {
>  		DRM_DEV_DEBUG_DRIVER(dev, "channel number: %d not support",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784393979.git.daniel@makrotopia.org?part=2