thesofproject / linux

Linux kernel source tree
Other
91 stars 133 forks source link

ASoC: SOF: Intel: Add NHLT architecture sub-string to generic topology name #4984

Closed ujfalusi closed 6 months ago

ujfalusi commented 6 months ago

If the user requested that the NHLT from the topology should be used with IPC4 and we have detected DMICs then the hda-generic topology name needs to include additional sub-string to identify the correct file to be loaded which includes the correct NHLT blob.

With this PR users will only need to add

options snd_sof_intel_hda_common sof_use_tplg_nhlt=1

and the kernel will figure out the correct topology to be loaded (no need for options snd_sof_pci tplg_filename=sof-hda-generic-cavs25-4ch.tplg - TGL and 4 DMICs)

plbossart commented 6 months ago

@ujfalusi Why not just add the blobs back in the generic topology?

ujfalusi commented 6 months ago

@ujfalusi Why not just add the blobs back in the generic topology?

Because the CAV25 and ACE1 topologies for IPC4 are in the same directory:

$ ls -al tools/build_tools/topology/topology2/target/sof-ipc4-tplg | grep sof-hda-generic
 sof-hda-generic-2ch.tplg
 sof-hda-generic-4ch.tplg
 sof-hda-generic-ace1-2ch.tplg
 sof-hda-generic-ace1-4ch.tplg
 sof-hda-generic-ace1-idisp-2ch.tplg
 sof-hda-generic-ace1-idisp-4ch.tplg
 sof-hda-generic-cavs25-2ch.tplg
 sof-hda-generic-cavs25-4ch.tplg
 sof-hda-generic-cavs25-idisp-2ch.tplg
 sof-hda-generic-cavs25-idisp-4ch.tplg
 sof-hda-generic-idisp-2ch.tplg
 sof-hda-generic-idisp-4ch.tplg
 sof-hda-generic-idisp.tplg
 sof-hda-generic.tplg
plbossart commented 6 months ago

@ujfalusi Why not just add the blobs back in the generic topology?

Because the CAV25 and ACE1 topologies for IPC4 are in the same directory:

So we painted ourselves in a corner, then painted the door handle and the stairs. Gah.

ujfalusi commented 6 months ago

@ujfalusi Why not just add the blobs back in the generic topology?

Because the CAV25 and ACE1 topologies for IPC4 are in the same directory:

So we painted ourselves in a corner, then painted the door handle and the stairs. Gah.

We could have gone for a use 'random directory names' based on DMIC/SSP blob type and sort platforms under those, which would be like throwing all the paint all over the place. The anticipation was that these generic+NHLT topologies would not be ever needed to be used by users, we included them just for completeness and for us to be used as a reference in rootcausing issues.

But right, with the single directory we will have these NHLT arch sub-strings coming up as well, so yeah, this has not been foreseen.

ujfalusi commented 6 months ago

@plbossart, I agree, with separate directory this would be likely easier to tackle as we would have always include the NHLT into the Xch topologies and they are in different dir. I take the blame for this.

ranj063 commented 6 months ago

@ujfalusi Why not just add the blobs back in the generic topology?

Because the CAV25 and ACE1 topologies for IPC4 are in the same directory:

$ ls -al tools/build_tools/topology/topology2/target/sof-ipc4-tplg | grep sof-hda-generic
 sof-hda-generic-2ch.tplg
 sof-hda-generic-4ch.tplg
 sof-hda-generic-ace1-2ch.tplg
 sof-hda-generic-ace1-4ch.tplg
 sof-hda-generic-ace1-idisp-2ch.tplg
 sof-hda-generic-ace1-idisp-4ch.tplg
 sof-hda-generic-cavs25-2ch.tplg
 sof-hda-generic-cavs25-4ch.tplg
 sof-hda-generic-cavs25-idisp-2ch.tplg
 sof-hda-generic-cavs25-idisp-4ch.tplg
 sof-hda-generic-idisp-2ch.tplg
 sof-hda-generic-idisp-4ch.tplg
 sof-hda-generic-idisp.tplg
 sof-hda-generic.tplg

@ujfalusi I also see the same

Because the CAV25 and ACE1 topologies for IPC4 are in the same directory:

@ujfalusi @plbossart if we look at the kernel side tplg paths for ACE and CAVS, we have separate paths set but unfortunately the target directories in our tplg builds for sof-ace-tplg and sof-ipc4-tplg are exactly the same. Can we fix this?

ujfalusi commented 6 months ago

@ujfalusi @plbossart if we look at the kernel side tplg paths for ACE and CAVS, we have separate paths set but unfortunately the target directories in our tplg builds for sof-ace-tplg and sof-ipc4-tplg are exactly the same. Can we fix this?

They are all sof-ipc4-tplgs and we have separate path only for MTL as we needed a sof-bin release which installs all sof-ipc4-tplgs under the 'correct' directory. We keep the symlink in there to have backwards compatibility for older kernels.

I think 'random' directories are really bad, we could again move things as intel/sof-ipc4-tplg/cavs25 intel/sof-ipc4-tplg/ace1 intel/sof-ipc4-tplg/ace2 -> ace1

This has been proposed, but the decision was to go with the sof-ipc4-tplg folder for all.

plbossart commented 6 months ago

Maybe we're making things too complicated....

we could just include the MTL blobs in the default hda-generic topology file, and rely on the tplg_name option on cavs2.5 platforms to select a topology with the TGL blobs. the user/tester would be responsible for setting both sof_use_tplg_nhlt=1 and tplg_filename=sof-hda-generic-cavs25-4ch.tplg

It wouldn't be perfect but those cavs2.5+IPC4 platforms are for tests only.

IOW we only make the "sof_use_tplg_nhlt=1" simpler on MTL, since is the platform where we're going to start seeing a lot more of those NHLT variations.

ujfalusi commented 6 months ago

@plbossart, it would work, but what are we going to do when ace3,4,5,6,7,8,9 comes around and some will need new type of NHLT blob? This PR would future proof the kernel for this. When a new NHLT format comes around it is going to come as a new architecture, so it is three additional line change to add the new NHLT arch's name.

We can do another transition to a new tplg directory structure as hinted above:

intel/sof-ipc4-tplg/cavs25
intel/sof-ipc4-tplg/ace1
intel/sof-ipc4-tplg/ace2 -> ace1
intel/sof-ace-tplg -> sof-ipc4-tplg/ace1
intel/sof-ipc4-tplg/sof-hda-generic* -> cavs25/sof-hda-generic*
intel/sof-ipc4-tplg/sof-tgl|adl* -> cavs25/sof-tgl|adl*

for a transition period.

plbossart commented 6 months ago

@ujfalusi I am not following the proposal, is the new structure only for the HDaudio topologies or would it impact SoundWire/I2S on ACE?

For future platforms we could introduce a new layer as needed. The problem is really ACE1 as a product, and what to do with cAVS2.5 which is more of a development/test platform.

ujfalusi commented 6 months ago

@plbossart, this only affects devices using sof-hda-generic + DMIC, all other topologies are prefixed with platform names sof-tgl/adl/mtl/etc This PR would make it easier to use the NHLT from the hda-generic topologies if needed, but on the other hand if a user really needs this it is just one additional line to add. I just thought that why not make life easier if we can.

I'm fine closing this, users will likely not going to need it anymore with the DMIC 16bit handling fixes.

plbossart commented 6 months ago

Yeah, maybe doing nothing is just fine :-)

I mean, if we need users to set one kernel parameter, they can add a second related one as well.

ujfalusi commented 6 months ago

By popular vote I'm closing this PR.