ASoC: SOF: Intel: pci-tgl: Change the default paths and firmware names #4382

ujfalusi · 2023-05-26T08:28:07Z

The currently used paths and firmware name reflects the reference firmware convention:

default_fw_path: intel/avs/{platform_name}
default_lib_path: intel/avs-lib/{platform_name}
default_tplg_path: intel/avs-tplg
default_fw_filename: dsp_basefw.bin

The SOF supports building the firmware for cAVS2.5 platforms using IPC4 and it is the preferred IPC4 implementation to be used on these devices.

Change the paths and firmware names to reflect this:

default_fw_path: intel/sof-ipc4/{platform_name}
default_lib_path: intel/sof-ipc4-lib/{platform_name}
default_tplg_path: intel/sof-ipc4-tplg
default_fw_filename: sof-{platform_name}.ri

ujfalusi · 2023-05-26T08:29:01Z

This will certainly going to fail in cavs testing due to missing files, needs CI action.

plbossart

no objection on the change proper but we need a clearer message about firmware releases, not just build.

plbossart · 2023-05-26T13:56:10Z

sound/soc/sof/intel/pci-tgl.c

I think you should clarify the backwards-compatibility story and which versions of sof-bin this will be aligned with.
It wouldn't hurt to clarify why the change is not made for earlier cAVS versions

Also if TGL uses this 'sof-ipc4-tplg' convention, I am not sure why we have the 'sof-ace-tplg'. It's a bit inconsistent, so my vote is to change MTL as well.

I think you should clarify the backwards-compatibility story and which versions of sof-bin this will be aligned with. It wouldn't hurt to clarify why the change is not made for earlier cAVS versions

I think the commit message says that the SOF firmware supports building for cAVS2.5 platforms, that is the reason why older platforms remain unchanged.

Also if TGL uses this 'sof-ipc4-tplg' convention, I am not sure why we have the 'sof-ace-tplg'. It's a bit inconsistent, so my vote is to change MTL as well.

Right, but should that be included in this PR?

bardliao · 2023-05-29T01:32:10Z

Does it mean SOF IPC4 supports TGL officially?

marc-hb · 2023-05-29T20:16:20Z

This will certainly going to fail in cavs testing due to missing files, needs CI action.

The /lib/firmware/ directory structure should certainly not be part of any closed-source CI "secret". It should be documented in places (sof-docs, sof-bin, both, other?) that do not require grepping and making sense of the Linux kernel sources and it should be implemented in the same build script(s) used by CI and developers. For instance:

thesofproject/sof@762f63bf8b4b8c63 (zephyr)
thesofproject/sof@5f04af9f10cc (XTOS, stable-v2.2)

There are just examples of (trying to) align everyone on the same /lib/firmware/ layout; I don't know how it must be done in this particular case.

If this is not done then there will be very tedious, duplicated and error-prone file shuffling and last minute bugs in sof-bin releases.

mengdonglin · 2023-05-29T06:12:02Z

sound/soc/sof/intel/pci-tgl.c

EHL stays with v2.2 LTS branch. We don't test SOF+IPC4 on Elkhart Lake (EHL) or release SOF binaries for this platform. Maybe we should remove EHL support from SOF main branch?

It does not really matter, EHL is in the TGL family and if ever it will get IPC4 support in SOF (it can be built, right, it is just the signing) then it must follow the convention we have.

Or if we drop EHL entirely then sure, the driver can drop it, but do we want that?

mengdonglin · 2023-05-29T06:20:37Z

sound/soc/sof/intel/pci-tgl.c

Maybe we can set default_fw_path to "intel/sof-ipc4" for both TGL and TGL-H? They use different default_fw_filename , sof-tgl.ri and sof-tgl-h.ri now.

@ujfalusi @kv2019i @marc-hb @plbossart Now xtensa-build-zephyr uses cAVS naming style: thesofproject/sof@762f63bf8b4b8c63 if w/ parameter --fw-naming AVS

But the default value of --fw-naming is SOF. If we omit this parameter or set --fw-naming SOF, and we also don't set '--use-platform-subdir', the layout of SOF+IPC4 is

/lib/firmware/intel/sof └─community │ ├── sof-tgl.ri │ ├── sof-tgl-h.ri │ └── sof-mtl.ri ├── dbgkey │ ├── sof-tgl.ri │ ├── sof-tgl-h.ri │ └── sof-mtl.ri ├── sof-tgl.ri ├── sof-tgl-h.ri └── sof-mtl.ri

The layout would be similar to that of SOF+IPC3. Is it good enough? Do we want to change the output directory name from sof to sof-ipc4?

The parameter '--use-platform-subdir' was introduced to avoid FW name collision of using AVS naming style.

I don't want to have exceptions to a rule that we apply.
{vendor}/sof-ipc4/{platform}/sof-{platform}.ri
or
{vendor}/sof-ipc4/sof-{platform}.ri
but not a mix of naming and placement convention, that will be a huge burden and just confusion at the end.

mengdonglin · 2023-05-29T06:24:36Z

sound/soc/sof/intel/pci-tgl.c

I'm not sure if adding platform subfolder name to default_fw_filename is enough or necessary. Maybe DSP HiFi version or module library API version is more relevant here.

Does it mean SOF IPC4 supports TGL officially?

@bardliao I think @kv2019i previously shared that from SOF v2.7 certain cavs2.5 devices will use IPC4 by default and we'll define quirks in kernel driver for these platforms. But probably most cavs2.5 devices will stay with stable v2.2 for Linux distributions, because the effort to define topology2 for all cavs2.5 platforms would be huge.

I have made the change to align with MTL and LNL convention, which uses subdirs. The pattern is:
{vendor}/sof-ipc4/{platform}/sof-{platform}.ri
For the library path we need to have separate directories, I think the reason was to have similar pattern under sof-ipc4 and sof-ipc4-lib

The reason why we never changed this is that everyone had a bit better idea and we never settled on a final solution ;)

ujfalusi · 2023-08-09T11:53:19Z

Changes since v1:

rebased on topic/sof-dev

ujfalusi · 2023-08-14T12:22:43Z

Changes since v2:

rebased on topic/sof-dev

marc-hb · 2023-09-18T22:07:45Z

More of the same mess:

[SKIP SOF-TEST] native loadable up-down-mixer take 2 sof#8210 (review)

It [the /lib/firmware/intel/sof* structure] should be documented in places (sof-docs, sof-bin, both, other?) that do not require grepping and making sense of the Linux kernel sources and it should be implemented in the same build script(s) used by CI and developers.

Ping? Can we please gather around and discuss a 1-2 pages long document (including pictures!) before making code changes? This commit message could be start.

I'm all for more self-explanatory code and less "paperwork" but asking people writing scripts to search and (fail) to fully understand Linux kernel source goes really too far - and it has failed consistently.

marc-hb · 2023-09-20T19:03:14Z

Ping? Can we please gather around and discuss a 1-2 pages long document (including pictures!) before making code changes?

Started by @kv2019i , please review:

intel_debug: add info for Intel firmware and tplg filenames sof-docs#475

ujfalusi · 2023-09-21T11:19:52Z

Changes since v3:

rebased on topic/sof-dev

I would like to push this for v6.6 upstream, CAV2.5 binaries are not published in the sof-bin release so it is a change that we can still do. It will only bring some discomfort for developers when moving, but it is a simple thing to handle.

ujfalusi · 2023-09-21T13:21:40Z

Ping? Can we please gather around and discuss a 1-2 pages long document (including pictures!) before making code changes?

Started by @kv2019i , please review:
* [intel_debug: add info for Intel firmware and tplg filenames sof-docs#475](https://github.com/thesofproject/sof-docs/pull/475)

This is not related to that, this is for IPC4 support on CAV2.5, which is not official atm and we need to have correct paths before it get official (if ever).

We absolutely cannot use intel/avs/* for SOF IPC4 binaries

marc-hb · 2023-09-21T14:12:43Z

This is not related to that, this is for IPC4 support on CAV2.5,

Both are about /lib/firmware/intel/ and all documentation about it ("official" and not) should be in the same place.

UPDATE: sorry, I edited the comment, now fixed

marc-hb · 2023-09-21T16:51:02Z

Also, an sof-docs PR about /lib/firmware/intel/ is much more "inclusive" than a kernel PR. Not just for people writing test/deploy/release scripts but even for managers if ever needed. The sof-docs PR should either come first or be submitted at the same time.

CI and validation have already wasted an inordinate amount of time on this topic; let's learn from our mistakes.

kv2019i · 2023-09-21T16:54:55Z

@marc-hb wrote:

This is not related to that, this is for IPC4 support on CAV2.5,
Both are about /lib/firmware/intel/ and all documentation about it ("official" and not) should be in the same place.

I think @ujfalusi you probably should add a note that this patch does NOT change the default IPC version, and does NOT change any paths for the driver that are used by default. I.e. this patch only affects configurations where user has explicitly forced IPC4 to be used across all systems (ipc_type parameter). I.e. only impacts developers and CI systems in Intel.

So given above, I think we don't need to mention this yet in sof-docs at all. The sof-docs page should document what the kernel does by default. If and when we change the default, then that must be accompanied with a sof-docs PR, that's for sure.

ujfalusi · 2023-09-21T17:16:21Z

@marc-hb, we will document the TGL IPC4 support when there are binaries to be used by users, currently they are not available and TGL family is only supported with IPC3.
Documenting something which is not available is just confusing.

Yes, when we reach the point that some parts of TGL can move to IPC4 then we will need documentation since there will be machines that might missing the tplg2 files, so they will fall back to IPC3.

What this PR is doing is to fix the incorrect IPC4 path before it is too late, as I said CAV2.5 must not refer to the avs path for SOF as we can build IPC4 for it. The AVS path is reserved for the AVS firmware, right?

marc-hb · 2023-09-21T18:26:53Z

we will document the TGL IPC4 support when there are binaries to be used by users, currently they are not available and TGL family is only supported with IPC3.

This does not seem correct:
https://github.com/thesofproject/sof-bin/blob/main/v2.5.x/README.md (March 2023)

Documenting something which is not available is just confusing.

sof-docs is mostly targeted at audio developers and testers (end-users should not need any documentation at all) and the IPC3/IPC4 transition has been in progress for much longer than six months.

Documentation can also refer to the past and to the future, this happens all the time. As long as the markers and distinction with the present is clear (kernel git versions, different README if desired, etc.) then that's not a problem. In fact that's even one of the main superiorities of documentation: it can span multiple versions. In documentation you can have plain English explanations starting with "starting with version X,..." that would look really strange in source code and that you obviously cannot add to older source code.

If really needed then the corresponding sof-docs PR can also stay a draft for a while and be merged only when the corresponding source code is merged.

In the mean time sof-docs will provide a much better place to discuss and better reference than the Linux source code; even before anything is merged.

What's especially frustrating in this particular case is that all the time spent discussing /lib/firmware/intel/ structure in general (admittedly not just this PR) and dealing with broken scripts is orders of magnitude bigger than the effort to write down something like thesofproject/sof-docs#475 Thanks again @kv2019i, let's keep turning that page.

ujfalusi · 2023-09-21T19:11:08Z

@marc-hb, yes, 2.5 went out with documenting the wrong path for the IPC4 files since the kernel was like that.

We need to change the kernel now so we can move and document the paths for developers.
The fact is that TGL family cannot use the avs directories, it was a mistake to have them and they should not stay.
I guess the reason for the paths that we had the reference firmware as a vehicle for development in early days, but that is not an excuse.

I can create a draft sof-docs PR to document the CAV2.5 paths for IPC4 SOF, sure, now issue.

Don't get me wrong: I agree that both kernel and sof-bin should implement what the documentation say and that should be the golden rule.

plbossart · 2023-09-21T19:22:34Z

I guess the reason for the paths that we had the reference firmware as a vehicle for development in early days, but that is not an excuse.

It was not a mistake, it was intentional and agreed on. We did not want to upstream support for any firmware that was not PV-quality.

Sometime in early 2023 it was decided to stop testing with reference firmware, so clearly that initial direction does not work any longer. We still don't have a clear direction however on what to do with IPC4 on TGL, do we?

ujfalusi · 2023-09-22T05:32:11Z

I guess the reason for the paths that we had the reference firmware as a vehicle for development in early days, but that is not an excuse.

It was not a mistake, it was intentional and agreed on. We did not want to upstream support for any firmware that was not PV-quality.

Yes, we were developing the kernel with the reference firmware since SOF back then did not had usable IPC4 support and it took some time to get there.

Sometime in early 2023 it was decided to stop testing with reference firmware, so clearly that initial direction does not work any longer.

And way before we stopped testing the reference firmware we were only run tests with it for the sake of compatibility sanity checks. To make sure that the development of SOF and kernel is not going to a direction when it is no longer compatible with IPC4 protocol.

We still don't have a clear direction however on what to do with IPC4 on TGL, do we?

Right, we don't, but on the other hand SOF supports IPC4 for TGL, we all (developers) use IPC4 on TGL and most of us (developers) would like to see IPC4 on TGL in the hands (ears) of our users.

My point is that TGL works with SOF firmware in IPC3 and IPC4, it is not working with the reference firmware out of box as the topologies have SOF modules, we cannot probe the reference firmware without modifying topologies.

If the direction clears and we decide that SOF.IPC4 on TGL is in good shape for users and then we try to fix the paths, it is already late.
What we have at the moment is incorrect regardless of the direction that we are going to take, reference firmware is not usable without significant effort. That effort is not straight forward for anyone who is not familiar with tplg2.

ujfalusi · 2023-09-22T06:56:04Z

@marc-hb, thesofproject/sof-docs#476

plbossart

No objections on the code changes proper, but this cannot be merged as is.

sound/soc/sof/intel/pci-tgl.c

plbossart · 2023-09-22T14:26:45Z

btw we have a fantastic new test status where all cavs tests are shown as completed without errors but they all TIMEOUT...

see https://sof-ci.01.org/linuxpr/PR4382/build6743/devicetest/index.html

ujfalusi · 2023-09-25T10:55:05Z

SOFCI TEST

ujfalusi · 2023-09-25T10:56:27Z

btw we have a fantastic new test status where all cavs tests are shown as completed without errors but they all TIMEOUT...

see https://sof-ci.01.org/linuxpr/PR4382/build6743/devicetest/index.html

This is really odd as this PR passed in the past. CI is specifying the firmware via module parameter for sure, I'm not sure about the topology file.

ujfalusi · 2023-09-25T12:58:14Z

@plbossart, yes CI does this:

[    4.697885] kernel: snd_sof:sof_select_ipc_and_paths: sof-audio-pci-intel-tgl 0000:00:1f.3: Module parameter used, changed fw path to intel/sof-ipc4/tgl/dbgkey

[    4.698635] kernel: snd_sof:sof_test_firmware_file: sof-audio-pci-intel-tgl 0000:00:1f.3: Failed to open firmware file: intel/sof-ipc4/tgl/dbgkey/sof-tgl.ri
[    4.698661] kernel: snd_sof:sof_test_firmware_file: sof-audio-pci-intel-tgl 0000:00:1f.3: Failed to open firmware file: intel/sof-ipc4/tgl/dbgkey/sof-tgl.ri

[    4.705124] kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: Firmware paths/files for ipc type 1:
[    4.705128] kernel: sof-audio-pci-intel-tgl 0000:00:1f.3:  Firmware file:     intel/sof-ipc4/tgl/dbgkey/dsp_basefw.bin
[    4.705130] kernel: sof-audio-pci-intel-tgl 0000:00:1f.3:  Firmware lib path: intel/avs/tgl
[    4.705131] kernel: sof-audio-pci-intel-tgl 0000:00:1f.3:  Topology file:     intel/avs-tplg/sof-tgl-rt711-rt1308-4ch.tplg

it sets intel/sof-ipc4/tgl/dbgkey as firmware path and leave the firmware name, tplg path as default. Topologies are stored in intel/avs-tplg/
#4603 is able to figure this out and handle it.

The currently used paths and firmware name reflects the reference firmware convention: default_fw_path: intel/avs/{platform_name} default_lib_path: intel/avs-lib/{platform_name} default_tplg_path: intel/avs-tplg default_fw_filename: dsp_basefw.bin The SOF supports building the firmware for cAVS2.5 platforms using IPC4 and it is the preferred IPC4 implementation to be used on these devices. Change the paths and firmware names to reflect this: default_fw_path: intel/sof-ipc4/{platform_name} default_lib_path: intel/sof-ipc4-lib/{platform_name} default_tplg_path: intel/sof-ipc4-tplg default_fw_filename: sof-{platform_name}.ri Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ujfalusi · 2023-12-07T13:13:12Z

Changes since v4:

rebased and force pushed to see how CI handles this to prepare a plan to get this one in to clear the way for held back changes.

plbossart

Thanks @ujfalusi

ujfalusi requested review from ajitkupandey, bardliao, dbaluta, kuanhsuncheng, kv2019i, lgirdwood, plbossart, ranj063 and yaochunhung as code owners May 26, 2023 08:28

ujfalusi requested review from marc-hb and mengdonglin May 26, 2023 08:29

plbossart reviewed May 26, 2023

View reviewed changes

bardliao previously approved these changes May 29, 2023

View reviewed changes

mengdonglin reviewed May 30, 2023

View reviewed changes

ujfalusi force-pushed the peter/sof/pr/tgl_family_ipc4_paths_01 branch from 3783358 to c2c51f1 Compare August 9, 2023 11:53

ujfalusi requested a review from vijendarmukunda as a code owner August 9, 2023 11:53

ujfalusi dismissed bardliao’s stale review via 48878f1 August 14, 2023 12:22

ujfalusi force-pushed the peter/sof/pr/tgl_family_ipc4_paths_01 branch from c2c51f1 to 48878f1 Compare August 14, 2023 12:22

bardliao approved these changes Aug 15, 2023

View reviewed changes

ujfalusi changed the title ~~[RFC] ASoC: SOF: Intel: pci-tgl: Change the default paths and firmware names~~ ASoC: SOF: Intel: pci-tgl: Change the default paths and firmware names Sep 6, 2023

ujfalusi mentioned this pull request Sep 14, 2023

Rename the default topology folder for MTL and LNL from intel/sof-ace-tplg to intel/sof-ipc4-tlpg #4590

Closed

marc-hb mentioned this pull request Sep 18, 2023

[SKIP SOF-TEST] native loadable up-down-mixer take 2 thesofproject/sof#8210

Closed

ujfalusi force-pushed the peter/sof/pr/tgl_family_ipc4_paths_01 branch from 48878f1 to d3d218a Compare September 21, 2023 11:15

kv2019i approved these changes Sep 21, 2023

View reviewed changes

ujfalusi mentioned this pull request Sep 21, 2023

ASoC: SOF: IPC fallback and extended IPC file profile #4603

Closed

plbossart requested changes Sep 22, 2023

View reviewed changes

sound/soc/sof/intel/pci-tgl.c Outdated Show resolved Hide resolved

plbossart added the Unclear No agreement on problem statement and resolution label Nov 21, 2023

ujfalusi force-pushed the peter/sof/pr/tgl_family_ipc4_paths_01 branch from d3d218a to 7e7261a Compare December 7, 2023 13:11

ujfalusi mentioned this pull request Dec 18, 2023

xtensa-build-zephyr.py: Deployable build thesofproject/sof#8643

Merged

plbossart approved these changes Dec 20, 2023

View reviewed changes

ujfalusi removed the Unclear No agreement on problem statement and resolution label Dec 22, 2023

bardliao merged commit 66ee247 into thesofproject:topic/sof-dev Dec 27, 2023

kv2019i mentioned this pull request Jan 2, 2024

[FEATURE] move to platform specific HDA tplg files thesofproject/sof#8683

Closed

ujfalusi deleted the peter/sof/pr/tgl_family_ipc4_paths_01 branch December 13, 2024 07:50

ASoC: SOF: Intel: pci-tgl: Change the default paths and firmware names #4382

ASoC: SOF: Intel: pci-tgl: Change the default paths and firmware names #4382

Uh oh!

Conversation

ujfalusi commented May 26, 2023

Uh oh!

ujfalusi commented May 26, 2023

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliao commented May 29, 2023

Uh oh!

marc-hb commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented Aug 9, 2023

Uh oh!

ujfalusi commented Aug 14, 2023

Uh oh!

marc-hb commented Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Sep 20, 2023

Uh oh!

ujfalusi commented Sep 21, 2023

Uh oh!

ujfalusi commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Sep 21, 2023 • edited by kv2019i Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Sep 21, 2023

Uh oh!

kv2019i commented Sep 21, 2023

Uh oh!

ujfalusi commented Sep 21, 2023

Uh oh!

marc-hb commented Sep 21, 2023

Uh oh!

ujfalusi commented Sep 21, 2023

Uh oh!

plbossart commented Sep 21, 2023

Uh oh!

ujfalusi commented Sep 22, 2023

Uh oh!

ujfalusi commented Sep 22, 2023

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plbossart commented Sep 22, 2023

Uh oh!

ujfalusi commented Sep 25, 2023

Uh oh!

ujfalusi commented Sep 25, 2023

Uh oh!

ujfalusi commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

marc-hb commented May 29, 2023 •

edited

Loading

marc-hb commented Sep 18, 2023 •

edited

Loading

ujfalusi commented Sep 21, 2023 •

edited

Loading

marc-hb commented Sep 21, 2023 •

edited by kv2019i

Loading

ujfalusi commented Sep 25, 2023 •

edited

Loading