load topology for each device

[bardliao]

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

This should not impact the existing devices.

[bardliao]

SOFCI TEST

[ujfalusi]

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.

[bardliao]

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

[ujfalusi]

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

[bardliao]

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

[ujfalusi]

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

[bardliao]

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

[ujfalusi]

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?

[bardliao]

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.

[ujfalusi]

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

[bardliao]

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

[ujfalusi]

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?

[bardliao]

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.

[ujfalusi]

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

[bardliao]

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.

[ujfalusi]

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?

[bardliao]

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.

[ujfalusi]

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.

[bardliao]

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.

[ujfalusi]

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.

[bardliao]

Totally agree.

[ujfalusi]

have you thought about the documentation?

[bardliao]

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

[bardliao]

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

[bardliao]

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.

[ujfalusi]

Add TPLG_DEVICE_MAX at the end of the enum?

[bardliao]

Yes, it would be better.

[ujfalusi]

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?

[ujfalusi]

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.

[bardliao]

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.

[ujfalusi]

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?

[bardliao]

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.

[ujfalusi]

have you thought about the documentation?

[ujfalusi]

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

[bardliao]

I will change it to TPLG_DEVICE_PCH_DMIC, thanks

[ujfalusi]

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.

[bardliao]

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

[ujfalusi]

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

[ujfalusi]

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.

[ujfalusi]

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

[bardliao]

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.

[ujfalusi]

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

[bardliao]

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]

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

[ujfalusi]

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

[ujfalusi]

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

[bardliao]

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.

[ujfalusi]

is this needs to be resource managed?

[bardliao]

Fixed.

[ujfalusi]

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.

[bardliao]

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

[ranj063]

@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.

[ujfalusi]

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

[bardliao]

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

[ujfalusi]

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

[ranj063]

no magic numbers like 4 please

[ujfalusi]

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

[ujfalusi]

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

[bardliao]

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

[ranj063]

just one more typo @bardliao

[ujfalusi]

@bardliao, thank you, this is pretty cool, could not wait to see it in

[ujfalusi]

One nitpick:

if (tplg_cnt != 1)
	dev_info(scomp->dev, "loading function topology %d: %s\n", i, tplg_files[i]);
else
	dev_dbg(scomp->dev, "loading topology:%s\n", file);

In case of a single topology file we would print in debug level as we did before, but in case of using function topology fragment we print with info

[ujfalusi]

Or even better:

	if (tplg_cnt == 1)
		dev_dbg(scomp->dev, "loading topology:%s\n", file);
	else
		dev_info(scomp->dev, "Using function topologies instead %s\n", file);

	for (i = 0; i < tplg_cnt; i++) {
		if (tplg_cnt != 1)
			dev_info(scomp->dev, " loading funtion %d: %s\n", i, tplg_files[i]);

[ujfalusi]

i is not initialized at this point yet.

[ujfalusi]

and I would keep the original print of

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

[ujfalusi]

@bardliao, thank you, this looks really nice now, could not wait to see this deployed and in use!!

[ranj063]

@bardliao looks like there are some legit checkpatch errors. Can you please fix them?

[ranj063]

@ujfalusi can u please merge if you're good