ASoC/SoundWire: parse SDCA functions to filter the RT712-VB configuration #4995

plbossart · 2024-05-14T21:57:40Z

Somewhat convoluted code to skip machine driver entries that require RT712 VB hardware.

plbossart · 2024-05-15T02:49:06Z

@shumingfan @bardliao here's an example of what we could do to filter the required topology.

On a MTL device with RT713, I can detect the VA version.
I don't have a device with RT712 VB but presumably the SmartMic function is the main difference.

plbossart · 2024-05-15T16:39:25Z

@bardliao @shumingfan I can't figure out if the RT712 and RT713 are identical or not, and if they have the same number of endpoints.

For now I get this error when I align RT712 and RT713...

[ 144.018284] sof_sdw sof_sdw: 1 is too many endpoints for codec: 0x713

plbossart · 2024-05-15T17:59:05Z

@shumingfan can you test this with RT712 VA and VB? The topologies required are generated with thesofproject/sof#9125

bardliao · 2024-05-16T01:13:10Z

@bardliao @shumingfan I can't figure out if the RT712 and RT713 are identical or not, and if they have the same number of endpoints.

For now I get this error when I align RT712 and RT713...

[ 144.018284] sof_sdw sof_sdw: 1 is too many endpoints for codec: 0x713

rt712 and rt713 are not identical. rt712 supports amp function while rt713 doesn't.

bardliao · 2024-05-16T01:30:29Z

sound/soc/sdca/sdca_device.c

Maybe >= 0x1000? As far as I know, the function type definition changed from sdca 0.6 to 1.0.

@bardliao @shumingfan I could see this value in the ASL file:
Package (2) {"mipi-sdw-sdca-interface-revision", 0x0801}, // Integer: SDCA 0.8r01

Can I use it as an indicator for VB, or just keep the > 0x0604 that was used for VA?

@plbossart Yes, the SDCA version of VB should be version 0x0801 or above.

shumingfan · 2024-05-16T08:32:35Z

@shumingfan can you test this with RT712 VA and VB? The topologies required are generated with thesofproject/sof#9125

@plbossart I try to test RT712VA and VB on MTL RVP with different ASL files.
rt712-l0-disco-table-test.asl.txt
rt712-vb-l0-disco-table-test.asl.txt
This PR could find out the VA and VB, then load different tplg.

ujfalusi

@plbossart, we should not lock SDCA to be only usable with ACPI, that sounds wrong.
I find some other parts confusing as well, probably documentation can help address those.

sound/soc/sdca/Kconfig

sound/soc/sdca/sdca_functions.c

sound/soc/codecs/Kconfig

sound/soc/intel/common/soc-acpi-intel-mtl-match.c

sound/soc/sof/intel/hda.c

plbossart · 2024-05-16T13:01:24Z

Of course we have a side effect with MTL AIOC having a bad DSDT

[    6.321319] kernel: acpi device:2e: found invalid SDCA function AF01 type 4 ADR 1, skipped
[    6.321847] kernel: acpi device:35: found invalid SDCA function AF02 type 2 ADR 2, skipped

Gah. what a mess.

sound/soc/sdca/sdca_device.c

plbossart · 2024-05-16T20:02:11Z

https://sof-ci.01.org/linuxpr/PR4995/build3008/devicetest/index.html?model=MTLP_SDW_AIOC&testcase=verify-kernel-boot-log is a case of bad configuration I am afraid on ba-mtlp-sdw-aioc-02: the device is detected as HDaudio and we use it for SoundWire tests??

@ssavati can you please double-check this device? This is clearly wrong:

May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: snd_sof_intel_hda_generic:hda_init_caps: sof-audio-pci-intel-mtl 0000:00:1f.3: skipping SoundWire, no links enabled
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: snd_sof_intel_hda:hda_codec_probe: sof-audio-pci-intel-mtl 0000:00:1f.3: HDA codec #2 probed OK: response: 8086281d
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: snd_sof_intel_hda:request_codec_module: hdaudio ehdaudio0D2: loading codec module: hdaudio:v8086281Dr00100000a01
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: hda codecs found, mask 4
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: using HDA machine driver skl_hda_dsp_generic now
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: snd_intel_dspcfg:intel_nhlt_get_dmic_geo: sof-audio-pci-intel-mtl 0000:00:1f.3: found 1 format definitions
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: snd_intel_dspcfg:intel_nhlt_get_dmic_geo: sof-audio-pci-intel-mtl 0000:00:1f.3: max channels found 2
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: snd_intel_dspcfg:intel_nhlt_get_dmic_geo: sof-audio-pci-intel-mtl 0000:00:1f.3: Array with 2 dmics
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: snd_intel_dspcfg:intel_nhlt_get_dmic_geo: sof-audio-pci-intel-mtl 0000:00:1f.3: dmic number 2 max_ch 2
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: DMICs detected in NHLT tables: 2
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: snd_sof:sof_select_ipc_and_paths: sof-audio-pci-intel-mtl 0000:00:1f.3: Module parameter used, changed fw path to intel/sof-ipc4/mtl/community
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: snd_sof:sof_select_ipc_and_paths: sof-audio-pci-intel-mtl 0000:00:1f.3: Module parameter used, changed tplg path to intel/sof-ipc4-tplg
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: snd_sof:sof_select_ipc_and_paths: sof-audio-pci-intel-mtl 0000:00:1f.3: Module parameter used, changed tplg name to sof-mtl-rt711-l0-rt1316-l23-rt714-l1.tplg
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: Firmware paths/files for ipc type 1:
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: sof-audio-pci-intel-mtl 0000:00:1f.3:  Firmware file:     intel/sof-ipc4/mtl/community/sof-mtl.ri
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: sof-audio-pci-intel-mtl 0000:00:1f.3:  Firmware lib path: intel/sof-ipc4-lib/mtl
May 16 18:08:04 ba-mtlp-sdw-aioc-02 kernel: sof-audio-pci-intel-mtl 0000:00:1f.3:  Topology file:     intel/sof-ipc4-tplg/sof-mtl-rt711-l0-rt1316-l23-rt714-l1.tplg

ssavati · 2024-05-17T04:46:18Z

@plbossart I have checked the BIOS configuration and other settings , everything is fine. Same device without this PR works fine. I think there is regression in this PR.

ujfalusi

@plbossart, It looks much cleaner, I have few suggestions around the acpi handling to make the code more generic.

include/linux/soundwire/sdw.h

sound/soc/sdca/sdca_functions.c

drivers/soundwire/slave.c

include/sound/soc-acpi.h

sound/soc/intel/common/soc-acpi-intel-mtl-match.c

ujfalusi · 2024-05-17T10:05:23Z

include/sound/soc-acpi.h

probably it is going to be too much overload, but the functionality of this new callback is is_machine_match()

sound/soc/sdca/sdca_device.c

plbossart · 2024-05-17T21:51:24Z

@plbossart I have checked the BIOS configuration and other settings , everything is fine. Same device without this PR works fine. I think there is regression in this PR.

Thanks for checking @ssavati there was a one-line Kconfig change that caused the problem by completely disabling SoundWire with the default kconfig scripts. Fixed now on my side.

plbossart · 2024-05-17T22:25:31Z

New version with a cleaner parsing of the functions with the backwards-compatible re-ordering for early devices

[  131.499758] soundwire sdw:0:0:025d:0713:01: found SDCA function UAJ (type 6)
[  131.499761] soundwire sdw:0:0:025d:0713:01: found SDCA function HID (type 10)
[  131.500270] soundwire sdw:0:1:025d:1316:01: found SDCA function SmartAmp (type 1)
[  131.500804] soundwire sdw:0:2:025d:1316:01: found SDCA function SmartAmp (type 1)

@shumingfan please re-check if I did something wrong, it's Friday afternoon and it's been a long week. Thanks!

marc-hb · 2024-05-17T23:06:37Z

https://github.com/thesofproject/linux/actions/runs/9134952643/job/25121470472?pr=4995

sound/soc/sof/amd/acp-common.c: In function ‘amd_sof_sdw_machine_select’:
sound/soc/sof/amd/acp-common.c:148:86: error: ‘struct sdw_amd_ctx’ has no member named ‘ids’
  148 |                                                                         acp_data->sdw->ids,
      |                                                                                      ^~
sound/soc/sof/amd/acp-common.c:149:86: error: ‘struct sdw_amd_ctx’ has no member named ‘num_slaves’
  149 |                                                                         acp_data->sdw->num_slaves))
      |                                                                                      ^~
sound/soc/sof/amd/acp-common.c:147:38: error: too many arguments to function ‘snd_soc_acpi_sdw_link_slaves_found’
  147 |                                 if (!snd_soc_acpi_sdw_link_slaves_found(sdev->dev, link,

bardliao

LGTM

ujfalusi

@plbossart, thank you for the updates, looks good with the exception of the mixed slave->dev / dev use for prints.

sound/soc/sdca/sdca_functions.c

ujfalusi · 2024-05-21T08:59:47Z

sound/soc/sdca/sdca_functions.c

does it needs to be dual license?

include/sound/soc-acpi.h

This structure is used to copy information from the 'sdw_slave' structures, it's better to create a flexible array of 'sdw_slave' pointers and directly access the information. This will also help access additional information stored in the 'sdw_slave' structure, such as an SDCA context. This patch does not add new functionality, it only modified how the information is retrieved. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add new module for SDCA (SoundWire Device Class for Audio) support. For now just add a parser to identify the SDCA revision and the function mask. Note that the SDCA definitions and related MIPI DisCo properties are defined only for ACPI platforms and extracted with _DSD helpers. There is currently no support for Device Tree in the specification, the 'depends on ACPI' reflects this design limitation. This might change in a future revision of the specification but for SDCA 1.0 ACPI is the only supported type of platform firmware. The SDCA library is defined with static inline fallbacks, which will allow for unconditional addition of SDCA support in common parts of the code. For more technical details, the SDCA specification is available for public downloads at https://www.mipi.org/mipi-sdca-v1-0-download Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Use SDCA helpers to get the information and store it. When platforms are not based on ACPI the helpers do absolutely nothing. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add a generic match function for quirks, chances are we are going to have lots of those... Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The existing machine_quirk() returns a pointer to a soc_acpi_mach structure. For SoundWire/SDCA support, we need a slightly different functionality where a quirk function either validates or NACKs an initial selection, based on additional firmware/DMI information. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add a filter to skip the RT172 VB configuration if a SmartMic Function is not found in the SDCA descriptors. If the ACPI information is incorrect this can only be quirked further with DMI information. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Use the existing machine_quirk() callback to select an alternate topology for RT712-VB devices. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

In theory the dailinks are created based on the number of endpoints reported in ACPI match tables, so it should harmless to add a new dailink: RT712 VA would not use it since it has only 2 endpoints. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart

the definition of a function_mask was a bad idea...

plbossart · 2024-05-21T19:15:28Z

include/sound/sdca.h

+ */
+struct sdca_device_data {
+	u32 interface_revision;
+	u32 function_mask;


Note to self: this function_mask assumed that there is only ONE type of function type per codec. The spec says otherwise, it's permitted to have multiple instances of the same function type, except for HID. That means we need an array of (ADR, function_type) pairs.

plbossart · 2024-05-21T23:23:00Z

closing for now, WIP alternate solution with extensions for functions

plbossart requested review from bardliao, dbaluta, kv2019i, lgirdwood, ranj063 and ujfalusi as code owners May 14, 2024 21:57

plbossart force-pushed the sdw/rt712-quirks branch 2 times, most recently from dc306f5 to 7b12bc0 Compare May 15, 2024 02:47

plbossart changed the title ~~[HACK] ASoC: Intel: mtl: add rt722 VB quirk~~ ASoC/SoundWire: parse SDCA functions to filter the RT712-VB configuration May 15, 2024

plbossart force-pushed the sdw/rt712-quirks branch from 7b12bc0 to 5644b4b Compare May 15, 2024 16:38

plbossart force-pushed the sdw/rt712-quirks branch from 5644b4b to 667a5af Compare May 15, 2024 17:53

bardliao reviewed May 16, 2024

View reviewed changes

ujfalusi requested changes May 16, 2024

View reviewed changes

plbossart force-pushed the sdw/rt712-quirks branch from 667a5af to 4e2d7f6 Compare May 16, 2024 17:44

plbossart commented May 16, 2024

View reviewed changes

sound/soc/sdca/sdca_device.c Outdated Show resolved Hide resolved

ujfalusi reviewed May 17, 2024

View reviewed changes

ujfalusi self-requested a review May 17, 2024 09:23

ujfalusi reviewed May 17, 2024

View reviewed changes

plbossart mentioned this pull request May 17, 2024

soundwire: sdca: parse function type and controls #4930

Closed

plbossart force-pushed the sdw/rt712-quirks branch from 4e2d7f6 to ff658f4 Compare May 17, 2024 22:26

plbossart force-pushed the sdw/rt712-quirks branch 2 times, most recently from e07bb4d to 00d3933 Compare May 20, 2024 22:24

plbossart requested review from bardliao, shumingfan and ujfalusi May 20, 2024 22:24

bardliao previously approved these changes May 21, 2024

View reviewed changes

ujfalusi reviewed May 21, 2024

View reviewed changes

plbossart added 8 commits May 21, 2024 09:06

soundwire: slave: parse SDCA function mask and revision

591b2de

Use SDCA helpers to get the information and store it. When platforms are not based on ACPI the helpers do absolutely nothing. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SDCA: add quirk function for RT712_VB match

00ac11f

Add a generic match function for quirks, chances are we are going to have lots of those... Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: Intel: hda: use machine_quirk() for SoundWire

b4e8dda

Use the existing machine_quirk() callback to select an alternate topology for RT712-VB devices. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart dismissed bardliao’s stale review via a037045 May 21, 2024 14:42

plbossart force-pushed the sdw/rt712-quirks branch from 00d3933 to a037045 Compare May 21, 2024 14:42

plbossart commented May 21, 2024

View reviewed changes

plbossart closed this May 29, 2024

plbossart mentioned this pull request May 29, 2024

ASoC/SoundWire: add support for RT712VB quirk #5028

Closed

ASoC/SoundWire: parse SDCA functions to filter the RT712-VB configuration #4995

ASoC/SoundWire: parse SDCA functions to filter the RT712-VB configuration #4995

Uh oh!

Conversation

plbossart commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented May 15, 2024

Uh oh!

plbossart commented May 15, 2024

Uh oh!

plbossart commented May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bardliao commented May 16, 2024

Uh oh!

bardliao May 16, 2024

Choose a reason for hiding this comment

Uh oh!

plbossart May 16, 2024

Choose a reason for hiding this comment

Uh oh!

shumingfan May 20, 2024

Choose a reason for hiding this comment

Uh oh!

shumingfan commented May 16, 2024

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart commented May 16, 2024

Uh oh!

Uh oh!

plbossart commented May 16, 2024

Uh oh!

ssavati commented May 17, 2024

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ujfalusi May 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plbossart commented May 17, 2024

Uh oh!

plbossart commented May 17, 2024

Uh oh!

marc-hb commented May 17, 2024

Uh oh!

bardliao left a comment

Choose a reason for hiding this comment

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ujfalusi May 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart May 21, 2024

Choose a reason for hiding this comment

Uh oh!

plbossart commented May 21, 2024

Uh oh!

plbossart commented May 14, 2024 •

edited

Loading

plbossart commented May 15, 2024 •

edited

Loading