mbox series

[v7,0/3] Add HDMI audio on the rk3588 SoC

Message ID 20250217215641.372723-1-detlev.casanova@collabora.com (mailing list archive)
Headers show
Series Add HDMI audio on the rk3588 SoC | expand

Message

Detlev Casanova Feb. 17, 2025, 9:47 p.m. UTC
To support HDMI audio on the rk3588 based devices, the generic HDMI
Codec framework is used in the dw-hdmi-qp DRM bridge driver.

The implementation is mainly based on the downstream driver, ported to the
generic HDMI Codec framework [1] recently merged in the master branch.
The parameters computation has been kept as is and the data stored in the
dw_hdmi_qp struct as been cleaned up.

The table for the N values has been edited to reflect N recommended values
as well as CTS recommended values.

The downstream kernel also implements a machine driver for HDMI audio but
it is doing exactly what the simple-audio-card driver does, so use that
instead in the RK3588 SoC device tree.

This adds HDMI audio support for both HDMI TX ports.

*** Dependencies ***

Based on Linus' master branch, but also needs Cristian's dts patches for HDMI1
support [2], which depends on Heiko's patchset for
phy-rockchip-samsung-hdptx [3]. Patches will apply without [3], but HDMI will
not work (at all).

[1]: https://lore.kernel.org/all/20241224-drm-bridge-hdmi-connector-v10-0-dc89577cd438@linaro.org
[2]: https://lore.kernel.org/linux-rockchip/20241211-rk3588-hdmi1-v2-0-02cdca22ff68@collabora.com
[3]: https://lore.kernel.org/lkml/20241206103401.1780416-3-heiko@sntech.de/

Changes since v6:
 - Fix arguments alignement (checkpatch --strict warnings)
 - Add hdmi1 audio support too
 - Move hdmi0_sound node under hdmi0_out_con

Changes since v5:
 - Simplify audio math computation for N
 - Move hdmi0-sound node up with other address-less nodes

Changes since v4:
 - Moved hdmi0_audio node the rk3588-base.dtsi
 - Enable hdmi0_audio in rk3588-rock-5b.dts

Changes since v3:
 - Renamed function to start with dw_hdmi_qp

Changes since v2:
 - Also clear the audio infoframe
 - Write AUDI_CONTENTS0 to its default value in case it gets overwritten.
 - Store tmds_char_rate in the dw_hdmi_qp struct in atomic_enable
 - Clear tmds_char_rate in atomic_disable and only write registers when
   tmds_char_rate is not 0.
 - Do not use connector_state duplicates

Changes since v1:
 - Remove useless audio_mutex (was used downstream for multiple drivers access
   to audio functions)
 - Let hdmi_codec build and setup audio infoframes
 - Only access audio registers when connector is connected
 - Rebased on master branch

Detlev Casanova (2):
  arm64: dts: rockchip: Add HDMI audio outputs for rk3588 SoC
  arm64: dts: rockchip: Enable HDMI audio outputs for Rock 5B

Sugar Zhang (1):
  drm/bridge: synopsys: Add audio support for dw-hdmi-qp

 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi |  17 +
 .../arm64/boot/dts/rockchip/rk3588-extra.dtsi |  17 +
 .../boot/dts/rockchip/rk3588-rock-5b.dts      |  16 +
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c  | 489 ++++++++++++++++++
 4 files changed, 539 insertions(+)

Comments

Piotr Oniszczuk Feb. 20, 2025, 11:16 a.m. UTC | #1
> Wiadomość napisana przez Detlev Casanova <detlev.casanova@collabora.com> w dniu 17 lut 2025, o godz. 22:47:
> 
> To support HDMI audio on the rk3588 based devices, the generic HDMI
> Codec framework is used in the dw-hdmi-qp DRM bridge driver.
> 
> The implementation is mainly based on the downstream driver, ported to the
> generic HDMI Codec framework [1] recently merged in the master branch.
> The parameters computation has been kept as is and the data stored in the
> dw_hdmi_qp struct as been cleaned up.
> 
> The table for the N values has been edited to reflect N recommended values
> as well as CTS recommended values.
> 
> The downstream kernel also implements a machine driver for HDMI audio but
> it is doing exactly what the simple-audio-card driver does, so use that
> instead in the RK3588 SoC device tree.
> 
> This adds HDMI audio support for both HDMI TX ports.
> 
> *** Dependencies ***
> 
> Based on Linus' master branch, but also needs Cristian's dts patches for HDMI1
> support [2], which depends on Heiko's patchset for
> phy-rockchip-samsung-hdptx [3]. Patches will apply without [3], but HDMI will
> not work (at all).
> 
> [1]: https://lore.kernel.org/all/20241224-drm-bridge-hdmi-connector-v10-0-dc89577cd438@linaro.org
> [2]: https://lore.kernel.org/linux-rockchip/20241211-rk3588-hdmi1-v2-0-02cdca22ff68@collabora.com
> [3]: https://lore.kernel.org/lkml/20241206103401.1780416-3-heiko@sntech.de/
> 
> Changes since v6:
> - Fix arguments alignement (checkpatch --strict warnings)
> - Add hdmi1 audio support too
> - Move hdmi0_sound node under hdmi0_out_con
> 
> Changes since v5:
> - Simplify audio math computation for N
> - Move hdmi0-sound node up with other address-less nodes
> 
> Changes since v4:
> - Moved hdmi0_audio node the rk3588-base.dtsi
> - Enable hdmi0_audio in rk3588-rock-5b.dts
> 
> Changes since v3:
> - Renamed function to start with dw_hdmi_qp
> 
> Changes since v2:
> - Also clear the audio infoframe
> - Write AUDI_CONTENTS0 to its default value in case it gets overwritten.
> - Store tmds_char_rate in the dw_hdmi_qp struct in atomic_enable
> - Clear tmds_char_rate in atomic_disable and only write registers when
>   tmds_char_rate is not 0.
> - Do not use connector_state duplicates
> 
> Changes since v1:
> - Remove useless audio_mutex (was used downstream for multiple drivers access
>   to audio functions)
> - Let hdmi_codec build and setup audio infoframes
> - Only access audio registers when connector is connected
> - Rebased on master branch
> 
> Detlev Casanova (2):
>  arm64: dts: rockchip: Add HDMI audio outputs for rk3588 SoC
>  arm64: dts: rockchip: Enable HDMI audio outputs for Rock 5B
> 
> Sugar Zhang (1):
>  drm/bridge: synopsys: Add audio support for dw-hdmi-qp
> 
> arch/arm64/boot/dts/rockchip/rk3588-base.dtsi |  17 +
> .../arm64/boot/dts/rockchip/rk3588-extra.dtsi |  17 +
> .../boot/dts/rockchip/rk3588-rock-5b.dts      |  16 +
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c  | 489 ++++++++++++++++++
> 4 files changed, 539 insertions(+)
> 
> -- 
> 2.48.1
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip


Detelv,

Just curious of your opinion:

I’m on 6.14-rc3 and using https://gitlab.collabora.com/cristicc/linux-next/-/commits/rk3588-hdmi-bridge + this v7 series.

On mine rock5b all works nicely but - at boot time - i’m getting some oops in kernel like this: https://gist.github.com/warpme/e1668acbef7817e5d2a88a6840328722

I’m not sure where issue is but it looks to me like kind of interference between analog audio and hdmi audio as commenting analog audio in dts ( https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts?h=v6.14-rc3#n24 ) stops kernel from oops’ing….

 rock5b dts i’m using is like this: https://gist.github.com/warpme/a8a32afd4a05d4b7f25d2808984b05ac
Detlev Casanova Feb. 20, 2025, 5:03 p.m. UTC | #2
Hi Piotr,

On Thursday, 20 February 2025 06:16:20 EST Piotr Oniszczuk wrote:
> > Wiadomość napisana przez Detlev Casanova <detlev.casanova@collabora.com> w
> > dniu 17 lut 2025, o godz. 22:47:
> > 
> > To support HDMI audio on the rk3588 based devices, the generic HDMI
> > Codec framework is used in the dw-hdmi-qp DRM bridge driver.
> > 
> > The implementation is mainly based on the downstream driver, ported to the
> > generic HDMI Codec framework [1] recently merged in the master branch.
> > The parameters computation has been kept as is and the data stored in the
> > dw_hdmi_qp struct as been cleaned up.
> > 
> > The table for the N values has been edited to reflect N recommended values
> > as well as CTS recommended values.
> > 
> > The downstream kernel also implements a machine driver for HDMI audio but
> > it is doing exactly what the simple-audio-card driver does, so use that
> > instead in the RK3588 SoC device tree.
> > 
> > This adds HDMI audio support for both HDMI TX ports.
> > 
> > *** Dependencies ***
> > 
> > Based on Linus' master branch, but also needs Cristian's dts patches for
> > HDMI1 support [2], which depends on Heiko's patchset for
> > phy-rockchip-samsung-hdptx [3]. Patches will apply without [3], but HDMI
> > will not work (at all).
> > 
> > [1]:
> > https://lore.kernel.org/all/20241224-drm-bridge-hdmi-connector-v10-0-dc89
> > 577cd438@linaro.org [2]:
> > https://lore.kernel.org/linux-rockchip/20241211-rk3588-hdmi1-v2-0-02cdca2
> > 2ff68@collabora.com [3]:
> > https://lore.kernel.org/lkml/20241206103401.1780416-3-heiko@sntech.de/
> > 
> > Changes since v6:
> > - Fix arguments alignement (checkpatch --strict warnings)
> > - Add hdmi1 audio support too
> > - Move hdmi0_sound node under hdmi0_out_con
> > 
> > Changes since v5:
> > - Simplify audio math computation for N
> > - Move hdmi0-sound node up with other address-less nodes
> > 
> > Changes since v4:
> > - Moved hdmi0_audio node the rk3588-base.dtsi
> > - Enable hdmi0_audio in rk3588-rock-5b.dts
> > 
> > Changes since v3:
> > - Renamed function to start with dw_hdmi_qp
> > 
> > Changes since v2:
> > - Also clear the audio infoframe
> > - Write AUDI_CONTENTS0 to its default value in case it gets overwritten.
> > - Store tmds_char_rate in the dw_hdmi_qp struct in atomic_enable
> > - Clear tmds_char_rate in atomic_disable and only write registers when
> > 
> >   tmds_char_rate is not 0.
> > 
> > - Do not use connector_state duplicates
> > 
> > Changes since v1:
> > - Remove useless audio_mutex (was used downstream for multiple drivers
> > access> 
> >   to audio functions)
> > 
> > - Let hdmi_codec build and setup audio infoframes
> > - Only access audio registers when connector is connected
> > - Rebased on master branch
> > 
> > Detlev Casanova (2):
> >  arm64: dts: rockchip: Add HDMI audio outputs for rk3588 SoC
> >  arm64: dts: rockchip: Enable HDMI audio outputs for Rock 5B
> > 
> > Sugar Zhang (1):
> >  drm/bridge: synopsys: Add audio support for dw-hdmi-qp
> > 
> > arch/arm64/boot/dts/rockchip/rk3588-base.dtsi |  17 +
> > .../arm64/boot/dts/rockchip/rk3588-extra.dtsi |  17 +
> > .../boot/dts/rockchip/rk3588-rock-5b.dts      |  16 +
> > drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c  | 489 ++++++++++++++++++
> > 4 files changed, 539 insertions(+)
> 
> Detelv,
> 
> Just curious of your opinion:
> 
> I’m on 6.14-rc3 and using
> https://gitlab.collabora.com/cristicc/linux-next/-/commits/rk3588-hdmi-brid
> ge + this v7 series.

Do you have the branch available somewhere ?

> On mine rock5b all works nicely but - at boot time - i’m getting some oops
> in kernel like this:
> https://gist.github.com/warpme/e1668acbef7817e5d2a88a6840328722

I did notice that at one point but it disappeard after a rebase on the the 
latest master so I didn't look further into that.
Could it be related to 2518a0e1b878042f9afa45ae063e544a16efc1a3 "ASoC: simple-
card: use __free(device_node) for device node" ?

I'm not exactly sure how __free(device_node) works though, but reverting that 
commit could fix the issue.

> I’m not sure where issue is but it looks to me like kind of interference
> between analog audio and hdmi audio as commenting analog audio in dts (
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/
> arm64/boot/dts/rockchip/rk3588-rock-5b.dts?h=v6.14-rc3#n24 ) stops kernel
> from oops’ing….

I cannot reproduce anymore, so if you have a branch/config that you use, I 
could try, looking into that.
I don't see any relevant commits that would change this behavior on master 
since v6.14-rc3, so I'm not sure what is going on.

>  rock5b dts i’m using is like this:
> https://gist.github.com/warpme/a8a32afd4a05d4b7f25d2808984b05ac

Regards,
Detlev.
Piotr Oniszczuk Feb. 20, 2025, 6:31 p.m. UTC | #3
> Wiadomość napisana przez Detlev Casanova <detlev.casanova@collabora.com> w dniu 20 lut 2025, o godz. 18:03:
> 
> Hi Piotr,
> 
> On Thursday, 20 February 2025 06:16:20 EST Piotr Oniszczuk wrote:
>> 
>> 
>> Detelv,
>> 
>> Just curious of your opinion:
>> 
>> I’m on 6.14-rc3 and using
>> https://gitlab.collabora.com/cristicc/linux-next/-/commits/rk3588-hdmi-brid
>> ge + this v7 series.
> 
> Do you have the branch available somewhere ?

My tests are on my MiniMyth2 distro. 
My build system is vanilla upstream+patches style.  
Kernel is mainline 6.14-rc3 kernel with applied series of patches:

PATCHFILES += 1001-math.h-add-DIV_ROUND_UP_NO_OVERFLOW.patch
PATCHFILES += 1002-clk-divider-Fix-divisor-masking-on-64-bit-platforms.patch
PATCHFILES += 1003-clk-composite-replace-open-coded-abs_diff.patch
# hdmi video support
PATCHFILES += 1010-FROM-ML-phy-phy-rockchip-samsung-hdptx-Don-t-use-dt-.patch
PATCHFILES += 1011-FROM-UPSTREAM-drm-rockchip-Don-t-change-hdmi-referen.patch
PATCHFILES += 1012-FROM-UPSTREAM-drm-rockchip-vop2-Drop-unnecessary-if_.patch
PATCHFILES += 1013-FROM-UPSTREAM-drm-rockchip-vop2-Improve-display-mode.patch
PATCHFILES += 1014-WIP-drm-rockchip-vop2-Improve-display-modes-handling.patch
PATCHFILES += 1015-drm-bridge-dw-hdmi-Sync-comments-with-actual-bus-for.patch
PATCHFILES += 1016-drm-bridge-connector-Sync-supported_formats-with-com.patch
PATCHFILES += 1017-drm-connector-hdmi-Evaluate-limited-range-after-comp.patch
PATCHFILES += 1018-drm-connector-hdmi-Add-support-for-YUV420-format-ver.patch
PATCHFILES += 1019-drm-connector-hdmi-Improve-debug-message-for-support.patch
PATCHFILES += 1020-drm-connector-hdmi-Use-YUV420-output-format-as-an-RG.patch
PATCHFILES += 1021-phy-Add-HDMI-configuration-options.patch
PATCHFILES += 1022-phy-hdmi-Add-color-depth-configuration.patch
PATCHFILES += 1023-phy-rockchip-samsung-hdptx-Fix-clock-ratio-setup.patch
PATCHFILES += 1024-phy-rockchip-samsung-hdptx-Drop-unused-lcpll_config.patch
PATCHFILES += 1025-phy-rockchip-samsung-hdptx-Setup-TMDS-char-rate-via-.patch
PATCHFILES += 1026-phy-rockchip-samsung-hdptx-Add-high-color-depth-mana.patch
PATCHFILES += 1027-phy-rockchip-samsung-hdptx-Cleanup-internal-rate-han.patch
PATCHFILES += 1028-phy-rockchip-samsung-hdptx-Avoid-Hz-hHz-unit-convers.patch
PATCHFILES += 1029-TEST-phy-rockchip-samsung-hdptx-Add-verbose-logging.patch
PATCHFILES += 1030-WIP-drm-bridge-Add-detect_ctx-hook.patch
PATCHFILES += 1031-WIP-drm-bridge-connector-Switch-from-detect-to-detec.patch
PATCHFILES += 1032-WIP-drm-bridge-dw-hdmi-qp-Add-high-TMDS-clock-ratio-.patch
PATCHFILES += 1033-WIP-drm-rockchip-vop2-Add-high-color-depth-support.patch
PATCHFILES += 1034-WIP-drm-rockchip-vop2-Add-YUV420-support.patch
PATCHFILES += 1035-WIP-drm-rockchip-dw_hdmi_qp-Make-use-of-phy_configur.patch
PATCHFILES += 1036-WIP-drm-rockchip-dw_hdmi_qp-Add-10bpc-and-YUV420-out.patch
PATCHFILES += 1037-WIP-drm-bridge-dw-hdmi-qp-Enable-10bpc-and-YUV420.patch
# hdmi audio support
PATCHFILES += 1040-drm-bridge-synopsys-add-audio-support-for-dw-hdmi-qp-v7.patch
# cec support
PATCHFILES += 1045-drm-bridge-synopsys-add-cec-support.patch
# var additions
PATCHFILES += 1060-net-ethernet-add-yt6801-gige-pcie-controller.patch
PATCHFILES += 1061-net-ethernet-yt6801-gige-pcie-silence-debug-msgs.patch
PATCHFILES += 1062-WIP-iommu-rockchip-add-flush_iotlb_all-ops.patch
PATCHFILES += 1063-media-rockchip-add-rkvdec2-driver.patch
PATCHFILES += 1064-media-rkvdec2-add-iommu-support-v3.patch
PATCHFILES += 1065-wip-add-hevc-support.patch
PATCHFILES += 1066-wip-hevc-add-ref-frames-support.patch
# dtsi additions
PATCHFILES += 1070-arm64-dtsi-rk3588s-add-vop2-clock-resets.patch
PATCHFILES += 1071-arm64-dtsi-rockchip-3588s-add-hdmi-bridge.patch
PATCHFILES += 1072-arm64-dtsi-rockchip-3588-hdmi-add-audio-support.patch
PATCHFILES += 1074-arm64-dtsi-rockchip-add-rkvdec2-video-vecoder-on-rk3588.patch
PATCHFILES += 1077-arm64-dtsi-rkvdec2-add-iommu-support-v3.patch
PATCHFILES += 1078-arm64-dtsi-rockchip-rk356x-add-rkvdec2-video-decoder-nodes.patch
# dts patches
PATCHFILES += 1080-arm64-dts-rockchip-rk3588s-rock5a-dts-improvements.patch
PATCHFILES += 1081-arm64-dts-rockchip-rk3588-rock5b-dts-improvements.patch
PATCHFILES += 1082-arm64-dts-rockchip-rk3588s-rock5c-dts-improvements.patch
PATCHFILES += 1083-arm64-dts-rockchip-rk3588-rock5itx-dts-improvements.patch
PATCHFILES += 1084-arm64-dts-rockchip-rk3588s-opi5-dts-improvements.patch
PATCHFILES += 1085-arm64-dts-rockchip-rk3588-opi5plus-dts-improvements.patch
PATCHFILES += 1086-arm64-dts-rockchip-rk3588s-add-opi5pro-dts.patch
PATCHFILES += 1087-arm64-dts-rockchip-rk3588s-add-nanopi-m6-dts.patch
PATCHFILES += 1088-arm64-dts-rockchip-rk3588s-nanopc-r6s-dts-improvements.patch
PATCHFILES += 1089-arm64-dts-rockchip-rk3588-nanopc-t6-dtsi-improvements.patch
PATCHFILES += 1090-arm64-dts-rockchip-rk3588-add-rock5t-dt.patch

 patches are from: https://github.com/warpme/minimyth2/tree/master/script/kernel/linux-6.14/files

Kernel config is: https://github.com/warpme/minimyth2/blob/master/script/kernel/linux-6.14/files/linux-6.14-arm64-armv8.config

Above patching is effectively: 

- https://gitlab.collabora.com/cristicc/linux-next/-/commits/rk3588-hdmi-bridge
- Yours hdmi audio v7
. added cec support
- added rkvdec2 (including hevc)
- some dts enablements for above


> 
>> On mine rock5b all works nicely but - at boot time - i’m getting some oops
>> in kernel like this:
>> https://gist.github.com/warpme/e1668acbef7817e5d2a88a6840328722
> 
> I did notice that at one point but it disappeard after a rebase on the the 
> latest master so I didn't look further into that.

Indeed - i.e. i don’t have these oops on rk3588 based orange5plus.
Also 6.12 kernel is clean.
But i have them reproducible on rock5b (and also e.g. on rock5t)
  
> Could it be related to 2518a0e1b878042f9afa45ae063e544a16efc1a3 "ASoC: simple-
> card: use __free(device_node) for device node" ?
> 

I tried with 2518a0e1b878042f9afa45ae063e544a16efc1a3 revered and this NOT helps.
With reverted above commit, dmesg is: https://gist.github.com/warpme/dbfe813583e4660a02b74315f193e768
Piotr Oniszczuk Feb. 21, 2025, 11:42 a.m. UTC | #4
Small data point: on rock5b switching in dts analog audio: from audio-graph-card to simple-audio-card (so dts is: https://gist.github.com/warpme/349b27e49bc6f617ef1041047e75adab ) makes kernel oops go away with analog audio still working…

so maybe issue is in audio-graph-card code (or its dts fragments)?
Detlev Casanova Feb. 21, 2025, 2:43 p.m. UTC | #5
On Friday, 21 February 2025 06:42:31 EST Piotr Oniszczuk wrote:
> Small data point: on rock5b switching in dts analog audio: from
> audio-graph-card to simple-audio-card (so dts is:
> https://gist.github.com/warpme/349b27e49bc6f617ef1041047e75adab ) makes
> kernel oops go away with analog audio still working…
> 
> so maybe issue is in audio-graph-card code (or its dts fragments)?

I can reproduce with your config (I guess that removing the driver for analog 
audio hides the issue)

I'm really feel like simple_util_clean_reference(card) in simple_probe() 
errors path should not be called anymore, since
https://lore.kernel.org/all/87zfk4o5l2.wl-kuninori.morimoto.gx@renesas.com/

I'm adding Kuninori Morimoto in the to list of this thread for extra input 
(See thread at https://lore.kernel.org/all/B8EF5196-55FB-44EC-B93C-E327C791225B@gmail.com/) as they made that change.
Especially those commits:
      ASoC: audio-graph-card2: use __free(device_node) for device node
      ASoC: audio-graph-card: use __free(device_node) for device node
      ASoC: simple-card: use __free(device_node) for device node

Detlev.
Kuninori Morimoto Feb. 25, 2025, 1:03 a.m. UTC | #6
Hi Detlev

Thank you for Cc:ing to me.
Sorry for my late response. Monday was holiday in Japan.

> I'm really feel like simple_util_clean_reference(card) in simple_probe() 
> errors path should not be called anymore, since
(snip)
> I'm adding Kuninori Morimoto in the to list of this thread for extra input
(snip)
> Especially those commits:
>       ASoC: audio-graph-card2: use __free(device_node) for device node
>       ASoC: audio-graph-card: use __free(device_node) for device node
>       ASoC: simple-card: use __free(device_node) for device node

I got same report from our test team. I'm now tring to solve it.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Kuninori Morimoto Feb. 25, 2025, 6:16 a.m. UTC | #7
Hi Detlev, again

> > Especially those commits:
> >       ASoC: audio-graph-card2: use __free(device_node) for device node
> >       ASoC: audio-graph-card: use __free(device_node) for device node
> >       ASoC: simple-card: use __free(device_node) for device node
> 
> I got same report from our test team. I'm now tring to solve it.

Unfortunately, I can't reproduce the issue on my environment,
but I guess I found the root cause. Does attached patch can solve
your issue ?
I will officially post the patch to ML, but want to confirm it before it.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
From 3995478ae84240b24964a458ecbefe9fb6dc2272 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Date: Tue, 25 Feb 2025 09:54:47 +0900
Subject: [PATCH] ASoC: simple-card-utils: Don't use __free(device_node) at
 graph_util_parse_dai()

commit 419d1918105e ("ASoC: simple-card-utils: use __free(device_node) for
device node") uses __free(device_node) for dlc->of_node, but we need to
keep it while driver is in use.

Don't use __free(device_node) in graph_util_parse_dai().

Fixes: 419d1918105e ("ASoC: simple-card-utils: use __free(device_node) for device node")
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/generic/simple-card-utils.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index 51e0e434514d..79fdd57a470c 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -1102,6 +1102,7 @@ static int graph_get_dai_id(struct device_node *ep)
 int graph_util_parse_dai(struct simple_util_priv *priv, struct device_node *ep,
 			 struct snd_soc_dai_link_component *dlc, int *is_single_link)
 {
+	struct device_node *node;
 	struct device *dev = simple_priv_to_dev(priv);
 	struct of_phandle_args args = {};
 	struct snd_soc_dai *dai;
@@ -1110,7 +1111,7 @@ int graph_util_parse_dai(struct simple_util_priv *priv, struct device_node *ep,
 	if (!ep)
 		return 0;
 
-	struct device_node *node __free(device_node) = of_graph_get_port_parent(ep);
+	node = of_graph_get_port_parent(ep);
 
 	/*
 	 * Try to find from DAI node
@@ -1153,8 +1154,10 @@ int graph_util_parse_dai(struct simple_util_priv *priv, struct device_node *ep,
 	 *    if he unbinded CPU or Codec.
 	 */
 	ret = snd_soc_get_dlc(&args, dlc);
-	if (ret < 0)
+	if (ret < 0) {
+		of_node_put(node);
 		goto end;
+	}
 
 parse_dai_end:
 	if (is_single_link)
Detlev Casanova Feb. 25, 2025, 2:58 p.m. UTC | #8
Hi Morimoto-san,

On Tuesday, 25 February 2025 01:16:50 EST Kuninori Morimoto wrote:
> Hi Detlev, again
> 
> > > Especially those commits:
> > >       ASoC: audio-graph-card2: use __free(device_node) for device node
> > >       ASoC: audio-graph-card: use __free(device_node) for device node
> > >       ASoC: simple-card: use __free(device_node) for device node
> > 
> > I got same report from our test team. I'm now tring to solve it.
> 
> Unfortunately, I can't reproduce the issue on my environment,
> but I guess I found the root cause. Does attached patch can solve
> your issue ?

From what I see, the error is not present anymore on linux 6.14-rc4. I tried 
reverting your patch "ASoC: simple-card-utils.c: add missing dlc->of_node" 
(dabbd325b25edb5cdd99c94391817202dd54b651) and the error reappears.

On 6.14-rc3, any of your patches (dabbd325b25e, or the one you attached here) 
will fix the issue and on 6.14-rc4, there is already a patch that fixes the 
issue.

Also, since dabbd325b25e, the node indeed should be kept while the driver is 
used. So even though the issue reported here is fixed by another patch, both 
are likely needed.

That being said, I'm not sure I completely understand why that extra line fixes 
the issue. Is the __free() attribute smart enough to know that the pointer has 
been copied and not free it at the end of scope ?

> I will officially post the patch to ML, but want to confirm it before it.
> 
> Thank you for your help !!

Thank you for looking into this too :)

Detlev.
Kuninori Morimoto Feb. 26, 2025, 1:14 a.m. UTC | #9
Hi Detlev

Thank you for the testing.

> I got same report from our test team. I'm now tring to solve it.

It seems our test team and your team are encounter same issues by different
way, and it seems it happen if some kind of deffer issue happen (?).

> That being said, I'm not sure I completely understand why that extra line fixes 
> the issue. Is the __free() attribute smart enough to know that the pointer has 
> been copied and not free it at the end of scope ?

__free(device_node) attribute will automatically calls of_node_put() when it
lost the scope. By below patch, graph_util_parse_dai() will use
__free(device_node) and thus dlc->of_node counter was not increased.

	419d1918105e5d9926ab02f1f834bb416dc76f65
	("ASoC: simple-card-utils: use __free(device_node) for device node")

In such situation, if some kind of deffer issue happened, xxx-card probe()
will failed and calls simple_util_clean_reference(). It calls of_node_put()
for each dlc->of_node, but above one was already called of_node_put() via
__free(device_node). So you encounter the error.

I'm not 100% sure why dabbd325b2 commit removed the issue, but I think
it is just luck, I think.

Thank you for your test !!
I will post the patch with below tag if our test team could confirm it.

	Tested-by: Detlev Casanova <detlev.casanova@collabora.com>

Best regards
---
Kuninori Morimoto
Heiko Stuebner Feb. 27, 2025, 10:55 a.m. UTC | #10
On Mon, 17 Feb 2025 16:47:39 -0500, Detlev Casanova wrote:
> To support HDMI audio on the rk3588 based devices, the generic HDMI
> Codec framework is used in the dw-hdmi-qp DRM bridge driver.
> 
> The implementation is mainly based on the downstream driver, ported to the
> generic HDMI Codec framework [1] recently merged in the master branch.
> The parameters computation has been kept as is and the data stored in the
> dw_hdmi_qp struct as been cleaned up.
> 
> [...]

Applied, thanks!

[1/3] drm/bridge: synopsys: Add audio support for dw-hdmi-qp
      commit: fd0141d1a8a2a26675ee88df75615c05a55044de

Best regards,
Heiko Stuebner Feb. 27, 2025, 1:37 p.m. UTC | #11
On Mon, 17 Feb 2025 16:47:39 -0500, Detlev Casanova wrote:
> To support HDMI audio on the rk3588 based devices, the generic HDMI
> Codec framework is used in the dw-hdmi-qp DRM bridge driver.
> 
> The implementation is mainly based on the downstream driver, ported to the
> generic HDMI Codec framework [1] recently merged in the master branch.
> The parameters computation has been kept as is and the data stored in the
> dw_hdmi_qp struct as been cleaned up.
> 
> [...]

Applied, thanks!

[2/3] arm64: dts: rockchip: Add HDMI audio outputs for rk3588 SoC
      commit: b8c6c136971c0e9750eec89f367529b2854d3a3c
[3/3] arm64: dts: rockchip: Enable HDMI audio outputs for Rock 5B
      commit: 97aa62ed1e970bf8aa9f57e87c946a95fa3d5bef

Best regards,
Piotr Oniszczuk March 1, 2025, 9:11 a.m. UTC | #12
> Wiadomość napisana przez Detlev Casanova <detlev.casanova@collabora.com> w dniu 25 lut 2025, o godz. 15:58:
> 
> From what I see, the error is not present anymore on linux 6.14-rc4. I tried 
> reverting your patch "ASoC: simple-card-utils.c: add missing dlc->of_node" 
> (dabbd325b25edb5cdd99c94391817202dd54b651) and the error reappears.

Guys,

Just FYI:

On 6.14-rc4 without 0001-ASoC-simple-card-utils-Don-t-use-__free-device_node-.patch - i still have oops like this: https://gist.github.com/warpme/ed75c05d3b68f995d429dbd9097005ba
They are happening not every boot - but still happening.

However applying 0001-ASoC-simple-card-utils-Don-t-use-__free-device_node-.patch (with some adaptations as it not applies cleanly on 6.140rc4) - dmesg becomes clean (10 boots; all ok)
Heiko Stuebner March 1, 2025, 6:27 p.m. UTC | #13
Am Samstag, 1. März 2025, 10:11:54 MEZ schrieb Piotr Oniszczuk:
> 
> > Wiadomość napisana przez Detlev Casanova <detlev.casanova@collabora.com> w dniu 25 lut 2025, o godz. 15:58:
> > 
> > From what I see, the error is not present anymore on linux 6.14-rc4. I tried 
> > reverting your patch "ASoC: simple-card-utils.c: add missing dlc->of_node" 
> > (dabbd325b25edb5cdd99c94391817202dd54b651) and the error reappears.
> 
> Guys,
> 
> Just FYI:
> 
> On 6.14-rc4 without 0001-ASoC-simple-card-utils-Don-t-use-__free-device_node-.patch - i still have oops like this: https://gist.github.com/warpme/ed75c05d3b68f995d429dbd9097005ba
> They are happening not every boot - but still happening.
> 
> However applying 0001-ASoC-simple-card-utils-Don-t-use-__free-device_node-.patch (with some adaptations as it not applies cleanly on 6.140rc4) - dmesg becomes clean (10 boots; all ok)

that patch was submitted yesterday [0], so hopefully will make its
way into 6.14-rc next week or so.



[0] https://lore.kernel.org/all/87eczisyhh.wl-kuninori.morimoto.gx@renesas.com/T/#me866307a928c2d592a2ba883867f028c5c8b9b40