Use sof sdw as default sdw machine #5417
Conversation
bardliao
requested review from
dbaluta,
kv2019i,
lgirdwood,
plbossart,
ranj063 and
ujfalusi
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR updates the default SoundWire machine driver configuration to use "sof_sdw" when no matching link ADR is found in the ACPI table. Key changes include new endpoint discovery functions in the Intel HDA driver, updates to symbol exports and cast assignments for link structures, and modifications to ACPI endpoint definitions across multiple files (including header changes to support a new "name_prefix" field).
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sound/soc/sof/intel/hda.c | Adds new helper functions and creates a default SDW machine driver with updated cast assignments. |
| sound/soc/sof/amd/acp-common.c | Updates the assignment of mach_params.links with an explicit cast. |
| sound/soc/sdw_utils/soc_sdw_utils.c | Updates codec info structures with new "name_prefix" attributes and makes asoc_sdw_get_dai_type public. |
| Various ACPI match files | Removes const qualifiers from endpoint data and updates symbol exports. |
| include/sound/soc_sdw_utils.h & include/sound/soc-acpi.h | Updates structure definitions to support mutable link pointers and the new "name_prefix" field. |
Comments suppressed due to low confidence (1)
include/sound/soc-acpi.h:84
- The update from a const pointer to a mutable pointer for the 'links' field in struct snd_soc_acpi_mach_params should be reviewed and documented to ensure it is consistent with the intended immutability or mutability of endpoint data across the codebase.
struct snd_soc_acpi_link_adr *links;
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
The explicit cast from a const pointer to a mutable pointer for 'mach->mach_params.links' suggests an underlying type mismatch. Consider updating the structure definitions to align the types, which would avoid the need for the cast.
include/sound/soc_sdw_utils.h
Outdated
There was a problem hiding this comment.
A new field 'name_prefix' has been added to asoc_sdw_codec_info without accompanying documentation. Please add a descriptive comment to clarify its intended purpose and usage.
bardliao
force-pushed
the
use_sof_sdw_as_default_sdw_machine
branch
from
63ea027 to
7c63353
Compare
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Only minor questions. @charleskeepax good for you ?
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
if (!) continue would help with indentation.
bardliao
force-pushed
the
use_sof_sdw_as_default_sdw_machine
branch
2 times, most recently
from
3ca3ebc to
630629d
Compare
|
@lgirdwood comments added |
There was a problem hiding this comment.
Do we actually need to remove the const on all of these hard coded entries? At least from the description where we are only creating a match if one doesn't exist that doesn't imply we would be changing an existing hard coded entry?
There was a problem hiding this comment.
The problem is that I don't know how to convert a const struct to a non-const struct. For example, I will get below compile error if I don't remove the const of cs42l43_endpoints and try to force cast the type when set the .endpoints.
sound/soc/intel/common/soc-acpi-intel-arl-match.c:200:38: error: conversion to non-scalar type requested
200 | .endpoints = (struct snd_soc_acpi_endpoint)cs42l43_endpoints,
| ^~~~~~~~~~~~~~~~~~~~~
It would be great if you could provide me some advice. I am more than happy if we don't need to remove the const on all of those hard coded entries.
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
Is this right? I feel like num_peripherals is the number of devices but links are the soundwire links in usage? Is the naming slightly confusing or are we allocating the right sizes here?
There was a problem hiding this comment.
My idea was that as we don't know the link numbers at this moment. Allocating the number of devices can guarantee that we allocate enough space for the links array. But, I use a loop to get the number of the links now. Thanks for pointing it out.
bardliao
force-pushed
the
use_sof_sdw_as_default_sdw_machine
branch
from
630629d to
133b10f
Compare
|
SOFCI TEST |
sound/soc/sof/intel/hda.c
Outdated
|
|
||
| adr_dev[index].endpoints = | ||
| devm_kzalloc(dev, codec_info_list[i].dai_num * | ||
| sizeof(struct snd_soc_acpi_endpoint), GFP_KERNEL); |
There was a problem hiding this comment.
Is the problem here? If instead of assigning the endpoint pointer in adr_dev directly what if you made a local non-const pointer, then once you have filled in the endpoints you can then assign that to the const pointer in the adr_dev structure.
There was a problem hiding this comment.
No, the problem is
struct snd_soc_acpi_adr_device {
const u64 adr;
const u8 num_endpoints;
const struct snd_soc_acpi_endpoint *endpoints;
const char *name_prefix;
};and
struct snd_soc_acpi_link_adr {
const u32 mask;
const u32 num_adr;
const struct snd_soc_acpi_adr_device *adr_d;
};I will get assignment of read-only member error even if the adr_device and the link_adr themselves are not const.
There was a problem hiding this comment.
Pretty sure the problem is there, this seems to build, although I haven't actually runtime tested it:
charleskeepax@ce250fb
https://github.com/charleskeepax/linux/commits/sof/fix/
And feel free squash that in where ever it goes.
There was a problem hiding this comment.
Oh, yes. Just make required variables non const and keep snd_soc_acpi_endpoint and snd_soc_acpi_adr_device const. Thanks for the advice. I will revise my PR, thanks.
bardliao
force-pushed
the
use_sof_sdw_as_default_sdw_machine
branch
from
133b10f to
06ad996
Compare
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
Rather than doing this which depends on the links being ordered, would it be reasonable to do something like:
link_index = link_mask & (BIT(peripherals->array[i]->bus->link_id) - 1);
Pretty sure that works.
There was a problem hiding this comment.
But the link_index may exceed the array size if the link_id is not continued or it doesn't start from 0. Like link 2 and 3 are used.
There was a problem hiding this comment.
Apologies it seems I forgot an hweight in my comment there:
link_index = hweight(link_mask & (BIT(peripherals->array[i]->bus->link_id) - 1));
For example a system using links 1,3 would look like:
link_index = hweight(link_mask & (BIT(link_id) - 1))) = hweight(0xA & (0x2 - 1)) = hweight(0xA & 0x1) = 0
link_index = hweight(link_mask & (BIT(link_id) - 1))) = hweight(0xA & (0x8 - 1)) = hweight(0xA & 0x3) = 1
There was a problem hiding this comment.
It basically counts the number of used links below the current link to get the link_index.
There was a problem hiding this comment.
Wow, that is clever. Thanks for the suggestion.
What in my mind is that new match tables will use a generic topology file which doesn't have to prefect match to the audio configuration. And the function topologies will be used. And yes, our goal is to use function topologies.
Added 7faf4cf to fix it. Missing function topology was not an error that's why we used |
|
@bardliao Cool, I'll be able to test tomorrow but I see the message change, it'll be interesting to see what the failure case looks like now in the log. on
We have a load of PTL matches that we're considering upstreaming as we already have some entries that will match on just the codec. I'm debating with myself whether these should have the functional topology callback for the reasons I mentioned - if there is a specific match entry then shouldn't the system use it in preference to the generic support? |
To me the benefit of using functional topology is to reduce the effort of creating new monolithic topologies. It is totally fine if the matches don't have the functional topology callback. |
|
Hi @bardliao , So I tried that branch, continuing from where we were:
So it didn't find a match table entry, said it was going to use functional topologies, couldn't find the file that I'd sabotaged and then It did fall back to using the monolithic topology that should only be used as a dummy to get the "lnl" platform and pass a file existence check. This got us wondering.... if someone upgraded the kernel to a version that did this, but didn't upgrade SOF, what if the monolithic topology it chose didn't actually fail... and the answer is that it look like it succeeds, the codec and amp drivers appeared to initialise properly, we get a sound card that includes all the controls we'd expect, wireplumber has a Lunar Lake audio controller with a stereo sink that you can play to but there are no catastrophic error messages in the log. Wireplumber gets headset events and appears to switch between speakers and headphones. Is there a way of not using the dummy monolithic topology? Replace the tlpg filename with the platform name string, bypass the filename check if it's only three characters and if the functional topology load fails bail out without using the dummy file? -Simon |
I don't think it will break the existing devices if they upgrade the kernel without upgrading SOF. All the existing working devices have their own ACPI match table. So, the driver will find the specific match. And regarding the dummy topology, if an acpi mach with
Yes, I will try to do it. |
bardliao
force-pushed
the
use_sof_sdw_as_default_sdw_machine
branch
from
7faf4cf to
67c965d
Compare
|
@simontrimmer PR updated. |
|
Hi @bardliao, This product has a match table entry in the kernel and a monolithic topology file on the filesystem, but because of
It'll try to use functional topologies first, which aren't on this filesystem
but does seem to then fallback to the monolithic topology as the sound card does all hook up, but it doesn't print the topology that was used (intel/sof-ace-tplg/sof-arl-cs42l43-l0-cs35l56-l23.tplg) Did we have to add functional topology callbacks to all the existing match entries? It makes sense to use it going forward but I'm wondering how much confusion this is going to cause |
Hmm, the topology name is printed with debug level.
I added functional topology callbacks to the multi-function codecs match entries. Currently the sof_sdw_get_tplg_files callback only supports multi-function codecs. |
|
I'm just weighing up continuing changing the level of debug messages vs removing the callback from matches (there are around 15 the moment in scope for the Cirrus codecs and amps). The logic is ideally if there is a match it should use the monolithic topology specified in the match entry as this maintains the behaviour of products that have already been tested with customers and it's extremely unlikely that we'd remove topology files from sof-bin as that would break compatibility with older kernels. On the downside that does mean I have a load of matches and topologies to make and upstream for PTL products. I'm extremely keen to not add matches for the future Cirrus devices and any that sneak in before end user products go to market we'd back out so that they only used functional topologies. |
I am totally fine if you want to remove the get_function_tplg_files callback from the acpi mach table. I added them just because I think they should work.
That is my goal, too. I am tired of adding new matches and new topologies. :) |
sound/soc/sof/intel/hda.c
Outdated
|
|
||
| return NULL; | ||
| /* The function topology needs the tplg file name to get the platform name for PCH DMIC */ | ||
| /* FIXME: find another way to get the platform name */ |
There was a problem hiding this comment.
can we update struct sof_intel_dsp_desc with the platform name?
There was a problem hiding this comment.
Good suggestion will update.
|
HI @bardliao ,
I think increasing this to an info level is worthwhile; I've been trying out the changes on some different products that have different bios features and encountered one with bios mic mute. This product drops the SDCA function associated with the SmartMic so our old friend ID numbering comes up again:
The code then (silently) falls back into the monolithic topology and you get errors beginning with
So the error message would actually be quite helpful to understand what really happened
I'm also trying to find a good reason why we shouldn't remove this match entirely: Which I think would channel all of our PTL products down the functional topology support (except the one ptl_cs42l43_l2_cs35l56x6_l13 that you'd added without the callback, maybe remove that earlier one too). We'd likely still have to make PTL matches and monolithic topologies for products that require a backport, but it would certainly reduce the amount of product support patches we'd have to make. I'm just debating approaches with the other developers now... |
may be used It may confuse people if "Topology file:" is printed but the topology file is not really used. We keep the print if the topology file is missing thus user can know what topology is missed in the file system. And increase the log level to info for the actual topology that is used. So that user can get which topology is used. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Currently, we use predefined snd_soc_acpi_link_adr tables to match the link adr from ACPI table to select the machine driver and the topology. However, with the mechanism, we need to create the snd_soc_acpi_link_adr table for each audio config. The sof_sdw machine driver is used by almost all Intel platforms with SOF and we can load required topology file dynamically today. In other words, we can use sof_sdw machine driver as the default machine driver for Intel SOF SoundWire codecs and no need to create snd_soc_acpi_link_adr table for every new audio configs. To achieve it, we need to drop the const for some members and edit the link adr and acpi adr data to match the data from the ACPI table. Suggested-by: Charles Keepax <ckeepax@opensource.cirrus.com> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Currently, the codec name_prefix of Intel SoundWire machine driver is from the ACPI match table. We can have it in the asoc_sdw_codec_info struct as a default name_prefix of a codec if there is no corresponding audio config found in the ACPI match table. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
The sof_sdw_get_tplg_files function is a callback of snd_soc_acpi_mach. Export it to allow other modules to use it. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
asoc_sdw_get_dai_type() is quite useful to convert SDCA function types to SDW DAI types. It can be used by other drivers. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
The platform name will be used construct the topology name. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
If there is no SoundWire machine matches the existing acpi match table, get the required machine data from the acpi table and construct the link adrs and endpoints. Pass the data to the default Intel SoundWire machine driver. And we don't need to add new item to the acpi match table in common cases. We will construct a dummy topology name. The dummy topology is just used to extract the platform name for function topology and should not be used. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
topoplogy Function topology must be used if the ACPI match table is created based on the reported ACPI table because the monolithic topology is not picked for the audio configuration. Escalate the log and ask the user to download the latest topologies. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
bardliao
force-pushed
the
use_sof_sdw_as_default_sdw_machine
branch
from
67c965d to
698c629
Compare
Increased the log level.
Ah, another missing id. I will submit a PR to add the topology.
This can only be removed after this PR is merged. Otherwise, the driver can't find any match that matches the config and SDW machine driver will not be used.
This match will be selected when the cs42l43 is on SDW link 3 and no preceding match matches. Note that the more specific match should be added in the front of the table so it will be selected.
|
|
Hi @bardliao , Yes removing the lone cs42l43 bus match would be after this chain, at the moment at least some of the PTL products I'm tracking would match it and wouldn't instantiate the amplifiers in various arrangements, whereas it it is removed they would all just use the new method. Doing it at the point of a new CPU generation has it's attraction. |
|
Hi @bardliao , |
9 participants
Use sof_sdw as default Intel SoundWire machine driver when there is no link adr matches in the acpi match table.