Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

load topology for each device #5209

Open
wants to merge 4 commits into
base: topic/sof-dev
Choose a base branch
from

Conversation

bardliao
Copy link
Collaborator

Get device information from dai links. load topology for each device.

This should not impact the existing devices.

@bardliao bardliao requested a review from ujfalusi October 18, 2024 13:32
@bardliao
Copy link
Collaborator Author

SOFCI TEST

@bardliao bardliao force-pushed the test-multi-topologies branch from 7eb5975 to ffadfbb Compare October 21, 2024 08:38
@bardliao bardliao force-pushed the test-multi-topologies branch from ffadfbb to 1deb725 Compare October 28, 2024 10:47
@bardliao bardliao force-pushed the test-multi-topologies branch from 1deb725 to 3264fc3 Compare November 13, 2024 05:44
sound/soc/sof/topology.c Show resolved Hide resolved
sound/soc/sof/topology.c Outdated Show resolved Hide resolved
sound/soc/sof/topology.c Outdated Show resolved Hide resolved
ret = request_firmware(&fw, tplg_files[i].file, scomp->dev);
if (ret < 0) {
if (i == 0) {
dev_dbg(scomp->dev,"Fail back to %s\n", tplg_name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it was a topology w/ DMIC then you have dropped the -2ch in a previous loop, you are not going to load the topology you supposed to be loading.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. I need to restore the original topology name.

tplg_name = strremove(tplg_name, "-2ch");
} else if (strstr(file, "-4ch")) {
tplg_device = "dmic-4ch";
tplg_name = strremove(tplg_name, "-4ch");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you dropping the 2ch/4ch from the original name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I would like to load the topology without dmic. For example, sof-hda-generic-2ch.tplg -> sof-hda-generic.tplg or similar.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the tplg_name is used only for loading the fallback, legacy, monolithic topology, in which case the fragments are not loaded. On a machine which have DMIC, you will load the topology w/o DMIC

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly, tplg_name topology will be also loaded if not all dai links are handled. For example, if the monolithic topology is sof-lnl-rt711-4ch, then sof-lnl-rt711 and sof-lnl-dmic-4ch-id5 will be loaded

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but if something fails in between and we need to fallback to the monolithic topology then at line 2641 you will have sof-lnl-rt711.tplg instead the correct monolithic sof-lnl-rt711-4ch.tplg, or am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but if something fails in between and we need to fallback to the monolithic topology then at line 2641 you will have sof-lnl-rt711.tplg instead the correct monolithic sof-lnl-rt711-4ch.tplg, or am I missing something?

Yeah, I will restore the tplg file name when it fails back to the monolithic topology, thanks.

if (strstr(file, "hda-generic")) {
tplg_device = "idisp";
} else {
tplg_device = "sdca-hdmi";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is HDMI audio using SDW link or HDMI audio using HDA link, but used with SDW machine?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the HDMI audio used with SDW codecs. I use different name for different codec interfaces is because that the existing topologies use different PCM id for different codec interfaces. For example, the HDMI PCM ids are 5,6,7 for SDW machines and 3,4,5 for HDA machines.

00-00: Jack Out (*) :  : playback 1
00-01: Jack In (*) :  : capture 1
00-02: Speaker (*) :  : playback 1
00-03: Amp feedback (*) :  : capture 1
00-04: Microphone (*) :  : capture 1
00-05: HDMI1 (*) :  : playback 1
00-06: HDMI2 (*) :  : playback 1
00-07: HDMI3 (*) :  : playback 1
00-31: Deepbuffer Jack Out (*) :  : playback 1
00-00: HDA Analog (*) :  : playback 1 : capture 1
00-03: HDMI1 (*) :  : playback 1
00-04: HDMI2 (*) :  : playback 1
00-05: HDMI3 (*) :  : playback 1
00-06: DMIC Raw (*) :  : capture 1
00-31: Deepbuffer HDA Analog (*) :  : playback 1

} else if (strstr(dai_link->name, "iDisp")) {
tplg_dev = TPLG_DEVICE_HDMI;
if (strstr(file, "hda-generic")) {
tplg_device = "idisp";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read the function right, this is not going to be used as the hda-generic will fall under load_default_tplg = true, the HDA dai_link->name is not handled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_default_tplg = true means there are still some remaining pcms that are not loaded in the sparated topologies. And we need to load the renamed topology at line 2620.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but can you load the idisp.tplg and the sof-hda-generic.tplg which also contains idisp definitions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but can you load the idisp.tplg and the sof-hda-generic.tplg which also contains idisp definitions?

No, so we need to make sure there is no idisp in sof-hda-generic.tplg.

"sdca-%damp", dai_link->num_cpus);
} else if (strstr(dai_link->name, "SmartMic")) {
tplg_dev = TPLG_DEVICE_SDW_MIC;
tplg_device = "sdca-mic";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and for the other checks: we need to update this match list every time a new component got introduced to products and we have new tplg fragment?
Is it going to scale?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to update this match list when a new dai links got introduced. Given that we didn't update dai links in our machine drivers. I think we will only update the match list occasionally.

tplg_files[i].device,
tplg_files[i].be_id);
dev_dbg(scomp->dev, "Requesting %d %s\n", i, tplg_files[i].file);
ret = request_firmware(&fw, tplg_files[i].file, scomp->dev);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the sof_create_ipc_file_profile() will check if the firmware and the topology file (the monolithic one!) is in place.
The means that you always need to have the monolithic tplg and the split components installed, otherwise the profile is rejected and if not fallback IPC version is available then we will fail.

I have a feeling that this split tplg handling has to touch the fw-file-profile.c as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I am thinking this open, too. My idea is that we don't touch the monolithic topology. I.e. the monolithic topology will coexist with the split topologies. So that sof_create_ipc_file_profile() will still valid.

"%s/sof-%s-%s-id%d.tplg",
sof_pdata->tplg_filename_prefix, platform,
tplg_files[i].device,
tplg_files[i].be_id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a clean documentation as commit message and comments to explain clearly the expected file naming and also an update to sof-docs must be done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you thought about the documentation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I almost forget the document. I will do it next week.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. @ujfalusi The sof-docs PR is thesofproject/sof-docs#507

@bardliao bardliao force-pushed the test-multi-topologies branch 2 times, most recently from 6259f1d to 78eed41 Compare November 20, 2024 06:55
@bardliao
Copy link
Collaborator Author

Topology PR: thesofproject/sof#9668. I would like to start with SDCA codecs only. SDCA is more urgent than others. If we can create dai links based on the supported SDCA functions, we don't need to use quirks for enabling/disabling specific dai links.

@bardliao bardliao force-pushed the test-multi-topologies branch 2 times, most recently from 95da27b to 9641e65 Compare November 22, 2024 08:14
@bardliao bardliao changed the title TEST: load topology for each device load topology for each device Nov 27, 2024
@bardliao bardliao marked this pull request as ready for review November 27, 2024 01:50
@bardliao bardliao requested a review from ranj063 as a code owner November 27, 2024 01:50
@bardliao bardliao force-pushed the test-multi-topologies branch from 9641e65 to 788e9bf Compare November 27, 2024 02:09
@@ -2489,7 +2489,6 @@ int snd_sof_load_topology(struct snd_soc_component *scomp, const char *file)
if (ret < 0) {
dev_err(scomp->dev, "error: tplg component load failed %d\n",
ret);
ret = -EINVAL;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can also drop the curly brackets

@@ -2464,15 +2465,180 @@ static const struct snd_soc_tplg_ops sof_dspless_tplg_ops = {
.bytes_ext_ops_count = ARRAY_SIZE(sof_dspless_bytes_ext_ops),
};

#define MAX_TPLG_NUM 5
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add TPLG_DEVICE_MAX at the end of the enum?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would be better.


static bool is_platform_support_separated_tplg(const char *platform)
{
if (!strcmp(platform, "mtl") || !strcmp(platform, "lnl") || !strcmp(platform, "ptl"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not going to scale... You are already missing ARL...
I would allow this for SOF_IPC4, I guess you handle the case when the topology files are not split, so it should just fallback?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you want to exclude the sof-hda on top of platform, right?
if !ipc4 || hda -> false ?
But this includes I2S machines also... Not sure what rule you want to make here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first thought is to only list the platforms that we already have the topology support. And "HDA" is one of the "platform" because we use sof-hda-generic.tplg for all platforms. Yeah, we can remove this check and fallback if the driver doesn't find the topology files.

} else if (strstr(dai_link->name, "SmartAmp")) {
tplg_dev = TPLG_DEVICE_SDW_AMP;
tplg_device = devm_kasprintf(sdev->dev, GFP_KERNEL,
"sdca-%damp", dai_link->num_cpus);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can there be multiple SmartAmp link? You only going to record the first one. The naming of the device is also interesting, it is postfixed with a number of CPU dais?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't support multiple SmartAmp link in the machine driver. And, yes, postfixing with a number of CPU dais is intentional. We need to set NUM_SDW_AMP_LINKS in the topology.

"%s/sof-%s-%s-id%d.tplg",
sof_pdata->tplg_filename_prefix, platform,
tplg_files[i].device,
tplg_files[i].be_id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you thought about the documentation?

TPLG_DEVICE_SDW_JACK,
TPLG_DEVICE_SDW_AMP,
TPLG_DEVICE_SDW_MIC,
TPLG_DEVICE_HOST_DMIC,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awkwardly named, I think you meant INTEL_DMIC (conencted to PCH-DMIC)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change it to TPLG_DEVICE_PCH_DMIC, thanks

tplg_dev = TPLG_DEVICE_HOST_DMIC;
} else if (strstr(dai_link->name, "iDisp")) {
tplg_dev = TPLG_DEVICE_HDMI;
tplg_device = "sdca-hdmi";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that you have excluded generic HDA, but things might fall through the cracks and picks this as HDMI where the PCM device id range is not 5,6,7. Which I suppose this fragment will provide.

How this naming going to scale with display audio serviced by SDW?

I'm not sure how this can be handled, but if we let HDA generic into \this logic, it is going to fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the be_id in the topology name, so that the PCM device id can be any number.

goto legacy_tplg;
}

dev_err(scomp->dev, "error: tplg request firmware %s failed err: %d\n",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use 'error:' in new prints.

return -ENOMEM;

dev_dbg(scomp->dev, "Requesting %d %s\n", i, tplg_files[i].file);
ret = request_firmware(&fw, tplg_files[i].file, scomp->dev);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

firmware_request_nowarn() to handle the fallback in case the fragmented tplg is not present, but what will happen if you load two fragment thn the third is missing and try to load the monolithic over it?

I'm sure that this is going to happen.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the original name is altered, the -2/4ch is dropped, so we are not going to load the right tplg in some cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

firmware_request_nowarn() to handle the fallback in case the fragmented tplg is not present, but what will happen if you load two fragment thn the third is missing and try to load the monolithic over it?

It will return error if the second or later topology is not present. My idea is that if we can't find the first fragmented topology, it means we don't have the required version of topologies, then we need to fall back to use the original topology. However, if we find the first fragmented topology, it means we use the new version of topology, and there should be something wrong if we don't find any required fragmented topology.

and the original name is altered, the -2/4ch is dropped, so we are not going to load the right tplg in some cases.

I will restore the file name in this case.


for (i = 0; i < tplg_num; i++) {
tplg_files[i].file = devm_kasprintf(sdev->dev, GFP_KERNEL,
"%s/sof-%s-%s-id%d.tplg",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another question: can a sof-sdca-jack.tplg work on all platforms, do we really need the mtl/lnl/ptl/arl/etc variants?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good questions. We set 'PLATFORM' in the topology, but we don't set any variables for SDW. So, I think we can remove the platform name for the SDW topologies. It will be easier to implement the topologies.

@bardliao bardliao force-pushed the test-multi-topologies branch 2 times, most recently from f5eb366 to ec01782 Compare December 16, 2024 02:51
@bardliao bardliao force-pushed the test-multi-topologies branch 2 times, most recently from f86b410 to 5a6f019 Compare January 16, 2025 09:02
@bardliao bardliao requested a review from lgirdwood January 16, 2025 09:16
@bardliao
Copy link
Collaborator Author

The sof-docs PR is thesofproject/sof-docs#507 and topology PR is thesofproject/sof#9668. @ujfalusi @ranj063 @lgirdwood Could you review? Thanks.

Copy link
Collaborator

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bardliao, I will continue on Monday, sorry for the delay.

};

/* The code is from https://stackoverflow.com/questions/47116974/remove-a-substring-from-a-string-in-c */
static char *strremove(char *str, const char *sub)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we want code from 'shady' places in kernel ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the second thought, the current PR only supports split topologies when all devices are covered by the split topologies. There is no needed to change the original tplg file name. And we don't need *strremove() in the commit.


dev_dbg(scomp->dev, "loading topology:%s\n", file);

ret = request_firmware(&fw, file, scomp->dev);
tplg_name = devm_kasprintf(sdev->dev, GFP_KERNEL, "%s", file);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needs to be resource managed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

if (sdev->pdata->ipc_type == SOF_IPC_TYPE_3)
goto legacy_tplg;

ret = sscanf(sof_pdata->tplg_filename, "sof-%3s-*.tplg", platform);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we must always have a monolithic topology present in filesystem? During probe we check for existence of the topology file in sof_test_topology_file() and we will fail if it is missing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we will create the monolithic topology. It is also for the old kernel that doesn't support split topologies.

Copy link
Collaborator

@ranj063 ranj063 Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bardliao I have a couple of fundamental problems with your solution.

  1. We start off as if there's only 1 topology file for each machine and then you come here in this function and decide thats not correct and then figure out what all sub-topologies to load. What does it mean "user will create topology for single device...". Its more like the user will create multiple generic mini topologies that are applicable across platforms isnt it?
  2. Secondly, this is very Soundwire-specific isnt it? Can we make it a bit more generic so that it applies to all machines, SDW, I2S, HDA etc? We can have a coarse-grained split based on DAI types i.e SDW/I2S/HDA + HDMI + DMIC and a fine-grained split for the first part based onthe disco table for the SDW bits. This way we can do this change in 2 steps, first the split by DAI types and then isolate the SDW disco-specific split as a second step so it makes it more intuitive for everyone to understand.

const struct firmware *fw;
bool load_default_tplg = false;
unsigned long tplg_mask = 0;
char platform[4];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"tgl-h" needs 6 char space...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the PLATFORM name is "tgl" in the topology.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but this is a generic code, you cannot assume that every vendor's every topology family will use three letters.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no magic numbers like 4 please

int snd_sof_load_topology(struct snd_soc_component *scomp, const char *file)
{
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
struct topology_file tplg_files[TPLG_DEVICE_MAX];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can ever have 1 instance providing a function? no two jack/amp/mic/dmic/hdmi ? only ever one from each?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think about karaoke devices with multiple jacks/mics for example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to have a topology for a function. Like we have 3 hdmi pcms, but we only have 1 hdmi topology.

if (ret != 1)
goto legacy_tplg;

for_each_card_prelinks(scomp->card, i, dai_link) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems very cool and potential big save in tplg combination maintainance!

One thing about this snd_sof_load_topology() logic is that how applicable this is to non-Intel SOF platforms? SDCA is in theory ok (same for all vendors), but concepts like "iDisp" and cards name lie "sof-hda-dsp" is already fairly vendor specific, while this function has been agnostic before. Should this be branched out via ops to allow for variation between vendors? I guess not all needs to be done immediately, but have you thought about this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first idea is that other vendors can add their link_name and card name to snd_sof_load_topology(). But yeah, using ops is more structured. Let me think more about it.

@bardliao bardliao force-pushed the test-multi-topologies branch 2 times, most recently from 52b69cf to 4abe5cb Compare February 10, 2025 14:25
sound/soc/sof/topology.c Show resolved Hide resolved
const struct firmware *fw;
bool load_default_tplg = false;
unsigned long tplg_mask = 0;
char platform[4];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no magic numbers like 4 please

if (sdev->pdata->ipc_type == SOF_IPC_TYPE_3)
goto legacy_tplg;

ret = sscanf(sof_pdata->tplg_filename, "sof-%3s-*.tplg", platform);
Copy link
Collaborator

@ranj063 ranj063 Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bardliao I have a couple of fundamental problems with your solution.

  1. We start off as if there's only 1 topology file for each machine and then you come here in this function and decide thats not correct and then figure out what all sub-topologies to load. What does it mean "user will create topology for single device...". Its more like the user will create multiple generic mini topologies that are applicable across platforms isnt it?
  2. Secondly, this is very Soundwire-specific isnt it? Can we make it a bit more generic so that it applies to all machines, SDW, I2S, HDA etc? We can have a coarse-grained split based on DAI types i.e SDW/I2S/HDA + HDMI + DMIC and a fine-grained split for the first part based onthe disco table for the SDW bits. This way we can do this change in 2 steps, first the split by DAI types and then isolate the SDW disco-specific split as a second step so it makes it more intuitive for everyone to understand.

@bardliao
Copy link
Collaborator Author

@ranj063

  1. I didn't say that 1 topology file for each machine is not correct. But loading sub-topologies can significantly reduce our effort to enable a new codec configuration. We added more and more quirks to decide what endpoints are used in the machine. And create topologies for every single audio configuration. Those are not needed if we could detect what dai links should be created and what sub-topologies should be loaded. And, yes, it is more like that user can create a topology just for one device type. Like speakers. And the topology is applicable across platforms.
  2. Yes, but I hope we can have this feature ASAP as there are more and more quirks/SDW topologies coming. So that I decide to focus on the SDW cards first.

@bardliao
Copy link
Collaborator Author

@ranj063 @kv2019i I will implement a card_tplg_ops. So that we can focus on the 'sof-soundwire' card, and it is easily to extend to other cards includes the non-Intel cards.

@ranj063
Copy link
Collaborator

ranj063 commented Feb 11, 2025

@ranj063 @kv2019i I will implement a card_tplg_ops. So that we can focus on the 'sof-soundwire' card, and it is easily to extend to other cards includes the non-Intel cards.

@bardliao sounds good. Do you want to convert this to draft until you have the other solution ready?

@bardliao bardliao marked this pull request as draft February 12, 2025 14:06
In preparation to handle tuples from multiple topologies, duplicate the
tuple string value by allocating memory for it.

Signed-off-by: Bard Liao <[email protected]>
No need to convert the return value of snd_soc_tplg_component_load().

Signed-off-by: Bard Liao <[email protected]>
@bardliao bardliao force-pushed the test-multi-topologies branch from 4abe5cb to 33565ab Compare February 13, 2025 11:49
@bardliao bardliao marked this pull request as ready for review February 13, 2025 11:50
@bardliao bardliao requested a review from lyakh as a code owner February 13, 2025 11:50
@bardliao bardliao force-pushed the test-multi-topologies branch from 33565ab to 30ebb6e Compare February 14, 2025 02:54
@bardliao bardliao force-pushed the test-multi-topologies branch from 30ebb6e to 951be49 Compare February 14, 2025 03:10

if (ret < 0) {
dev_err(scomp->dev, "tplg component load failed %d\n",
ret);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would fit in 1 line

tplg_mask |= BIT(tplg_dev);
switch (tplg_dev) {
case TPLG_DEVICE_PCH_DMIC:
(*tplg_files)[tplg_num] = kasprintf(GFP_KERNEL, "%s/sof-%s-%s-id%d.tplg",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you've allocated *tplg_files as just a single pointer, and here you're using it as a virtually endless (depending on the number of the cards) array. This seems like a memory corruption to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will not be endless. It is in the for_each_card_prelinks() loop which is (i) < (card)->num_links). So, the max number of tplg_num is num_links. And I allocate *tplg_files = kcalloc(scomp->card->num_links, sizeof(char *), GFP_KERNEL); before entering the loop.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bardliao oh, you're right, my oversight, sorry

sound/soc/sof/topology.c Outdated Show resolved Hide resolved
@lyakh lyakh self-requested a review February 14, 2025 09:49
lyakh
lyakh previously approved these changes Feb 14, 2025
A topology can be split into several sub-topologies. The
sub-topologies can be used across cards. In other words, we can create
multiple mini topologies and let driver select what it needs.
However, the topology depends on the cards. Add a get_tplg_files ops
allow cards have their own sub-topology file names.
The commit add an empty ops first. Will add the first ops in the follow
up commit.

Signed-off-by: Bard Liao <[email protected]>
Add get_sof_sdw_tplg_files ops to generate the topology file names for
the "sof-soundwire" card.

Signed-off-by: Bard Liao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants