drm/bridge: it6505: DP audio support + shared-DAI hw_params fix
Subsystems (4)
cover drm/bridge: it6505: DP audio support + shared-DAI hw_params fix
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/[email protected]/
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
Signature · 1 line
2.55.0
PATCH 1/2 drm/bridge: it6505: Add audio support
From: Jiaxin Yu <[email protected]>
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]
--- drivers/gpu/drm/bridge/ite-it6505.c | 92 ++++++++++++++++++++++++----- 1 file changed, 76 insertions(+), 16 deletions(-)
drivers/gpu/drm/bridge/ite-it6505.c +76 -16
@@ -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);
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH 1/2] drm/bridge: it6505: Add audio support
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
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 <[email protected]>
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.
Quoted 4 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0
[ ... ]
drivers/gpu/drm/bridge/ite-it6505.c static void __maybe_unused it6505_delayed_audio(struct work_struct *work) · line 2968
Quoted 12 lines
drivers/gpu/drm/bridge/ite-it6505.c +1
-1@@ -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?
[ ... ]
drivers/gpu/drm/bridge/ite-it6505.c static void __maybe_unused it6505_audio_shutdown(struct device *dev, void *data) · line 3033
Quoted 8 lines
drivers/gpu/drm/bridge/ite-it6505.c +1
-1-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?
Quoted 17 lines
drivers/gpu/drm/bridge/ite-it6505.c +13
-4-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);
drivers/gpu/drm/bridge/ite-it6505.c static void __maybe_unused it6505_audio_shutdown(struct device *dev, void *data) · line 3057
Quoted 10 lines
drivers/gpu/drm/bridge/ite-it6505.c +10
-0+ +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?
Quoted 6 lines
drivers/gpu/drm/bridge/ite-it6505.c +6
-0+else +queue_delayed_work (system_wq, &it6505->delayed_audio, +msecs_to_jiffies (180 )); + +return 0 ; +}
[ ... ]
Quoted 9 lines
drivers/gpu/drm/bridge/ite-it6505.c +9
-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,
drivers/gpu/drm/bridge/ite-it6505.c static int __maybe_unused it6505_audio_hook_plugged_cb(struct device *dev, · line 3104
Quoted 10 lines
drivers/gpu/drm/bridge/ite-it6505.c +10
-0+ }; +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?
Quoted 4 lines
drivers/gpu/drm/bridge/ite-it6505.c +4
-0+DRM_DEV_DEBUG_DRIVER (dev,"bound to %s" ,HDMI_CODEC_DRV_NAME ); + +return 0 ; +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH 2/2 drm/bridge: it6505: Don't reject audio hw_params without an encoder
In reply to Daniel Golleit6505_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.
--- drivers/gpu/drm/bridge/ite-it6505.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/bridge/ite-it6505.c +1 -1
@@ -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",
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH 2/2] drm/bridge: it6505: Don't reject audio hw_params without an encoder
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…
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 <[email protected]>
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.
Quoted 4 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0
[ ... ]
drivers/gpu/drm/bridge/ite-it6505.c static int it6505_audio_setup_hw_params(struct it6505 *it6505, · line 2982
Quoted 6 lines
drivers/gpu/drm/bridge/ite-it6505.c +1
-1@@ -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?
Quoted 3 lines
drivers/gpu/drm/bridge/ite-it6505.c +0
-0if (params->cea.channels <=1 || params->cea.channels >8 ) { DRM_DEV_DEBUG_DRIVER(dev, "channel number: %d not support",
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2