LLEXT: fix failures and make DRC an LLEXT module by default on MTL #9116

lyakh · 2024-05-10T14:33:28Z

Build all the code, supporting LLEXT, modular
UPDATE: changed to only build DRC as LLEXT on MTL

lyakh · 2024-05-13T07:56:38Z

It has been suggested that the CI might just work if we switch any module to llext and build a deployable layout.

@marc-hb ok, no, a very clear indication that it unfortunately doesn't "just work:" https://sof-ci.01.org/sofpr/PR9116/build4627/devicetest/index.html?model=MTLP_RVP_NOCODEC&testcase=verify-kernel-boot-log so supposedly some work needs to be done...

lyakh · 2024-06-05T11:46:27Z

SOFCI TEST

marc-hb · 2024-06-05T22:21:38Z

https://github.com/thesofproject/sof/actions/runs/9387214836/job/25849589149?pr=9116

FAILED: zephyr/smart_amp_test_llext/smart_amp_test.llext D:/a/sof/sof/workspace/build-mtl/zephyr/smart_amp_test_llext/smart_amp_test.llext 
C:\Windows\system32\cmd.exe /C "cd /D D:\a\sof\sof\workspace\build-mtl\zephyr\smart_amp_test_llext && D:\a\sof\sof\zephyr-sdk-0.16.4_windows-x86_64\zephyr-sdk-0.16.4\xtensa-intel_ace15_mtpm_zephyr-elf\bin\xtensa-intel_ace15_mtpm_zephyr-elf-strip.exe  -R.xt.* D:/a/sof/sof/workspace/build-mtl/zephyr/smart_amp_test.llext.pkg_input -oD:/a/sof/sof/workspace/build-mtl/zephyr/smart_amp_test_llext/smart_amp_test.llext  && "C:\Program Files\CMake\bin\cmake.exe" -E true"
D:\a\sof\sof\zephyr-sdk-0.16.4_windows-x86_64\zephyr-sdk-0.16.4\xtensa-intel_ace15_mtpm_zephyr-elf\bin\xtensa-intel_ace15_mtpm_zephyr-elf-strip.exe: 'D:/a/sof/sof/workspace/build-mtl/zephyr/smart_amp_test.llext.pkg_input': No such file

https://sof-ci.01.org/sofpr/PR9116/build5189/build also failed.

lyakh · 2024-06-07T08:31:59Z

https://github.com/thesofproject/sof/actions/runs/9387214836/job/25849589149?pr=9116

FAILED: zephyr/smart_amp_test_llext/smart_amp_test.llext D:/a/sof/sof/workspace/build-mtl/zephyr/smart_amp_test_llext/smart_amp_test.llext 
C:\Windows\system32\cmd.exe /C "cd /D D:\a\sof\sof\workspace\build-mtl\zephyr\smart_amp_test_llext && D:\a\sof\sof\zephyr-sdk-0.16.4_windows-x86_64\zephyr-sdk-0.16.4\xtensa-intel_ace15_mtpm_zephyr-elf\bin\xtensa-intel_ace15_mtpm_zephyr-elf-strip.exe  -R.xt.* D:/a/sof/sof/workspace/build-mtl/zephyr/smart_amp_test.llext.pkg_input -oD:/a/sof/sof/workspace/build-mtl/zephyr/smart_amp_test_llext/smart_amp_test.llext  && "C:\Program Files\CMake\bin\cmake.exe" -E true"
D:\a\sof\sof\zephyr-sdk-0.16.4_windows-x86_64\zephyr-sdk-0.16.4\xtensa-intel_ace15_mtpm_zephyr-elf\bin\xtensa-intel_ace15_mtpm_zephyr-elf-strip.exe: 'D:/a/sof/sof/workspace/build-mtl/zephyr/smart_amp_test.llext.pkg_input': No such file

https://sof-ci.01.org/sofpr/PR9116/build5189/build also failed.

@marc-hb fixed that. Could you or @fredoh9 help check why that's the case? We'd need to compare intermediate results
EDIT: sorry, I meant to ask: why the windows / linux build comparison is failing now

marc-hb · 2024-06-07T15:41:53Z

https://github.com/thesofproject/sof/actions/runs/9418758155/job/25947397562?pr=9116

Files linux-build mtl/build-sof-staging/sof/intel/sof-ipc4-lib/mtl/community/smart_amp_test.llext and windows-build mtl/build-sof-staging/sof/intel/sof-ipc4-lib/mtl/community/smart_amp_test.llext differ

Do such files have debug symbols? If yes then they shouldn't be compared, not until they're stripped (TODO)

marc-hb · 2024-06-08T01:06:42Z

Do such files have debug symbols? If yes then they shouldn't be compared, not until they're stripped (TODO)

Actually, there's a better temporary solution: turn off modules when testing reproducible builds. Otherwise code in modules is not tested.

lyakh · 2024-06-12T08:47:31Z

Do such files have debug symbols? If yes then they shouldn't be compared, not until they're stripped (TODO)

@marc-hb makes sense, thanks! I'll try adding stripping .comment to Zephyr LLEXT cmake code

lyakh · 2024-06-12T09:03:13Z

Actually, there's a better temporary solution: turn off modules when testing reproducible builds. Otherwise code in modules is not tested.

@marc-hb not sure I understand - doesn't the failing test mean, that modules do get tested?

lyakh · 2024-06-18T08:38:45Z

@wszypelt QB stuck?

wszypelt · 2024-06-18T09:01:27Z

@lyakh Unfortunately, I'm trying to solve this issue because more PRs are stuck. As long as I manually added it to the queue, the results should be available within an hour

lgirdwood · 2024-06-25T15:27:19Z

@lyakh can you rebase and re-push. Thanks !

lyakh · 2024-06-26T06:24:42Z

@lyakh can you rebase and re-push. Thanks !

@lgirdwood It isn't just about rebasing: we're waiting for 2 things to happen: (1) QB support for LLEXT modules @wszypelt , and (2) a solution on how to resolve the failing Linux-Windows comparison @marc-hb #9116 (comment)

lgirdwood · 2024-06-26T13:56:33Z

@lyakh can you rebase and re-push. Thanks !

@lgirdwood It isn't just about rebasing: we're waiting for 2 things to happen: (1) QB support for LLEXT modules @wszypelt , and (2) a solution on how to resolve the failing Linux-Windows comparison @marc-hb #9116 (comment)

Ok, lets disable the Windows/Linux comparison here as we know the toolchain has some opens around building shared objects/libraries.

@wszypelt is there an ETA for when internal CI could support this build target ? Fwiw, @mwasko and I were discussing today. My preference would be to have this build option testable by all CIs for best coverage.

lyakh · 2024-06-26T15:17:26Z

Ok, lets disable the Windows/Linux comparison here as we know the toolchain has some opens around building shared objects/libraries.

@lgirdwood @marc-hb we could diff --exclude=*.llext "for now"

lyakh · 2024-07-12T06:24:29Z

created #9298 as a part of this PR, also addressing Marc's latest comment. We can either merge it all with this one, or split it (again) into two to make them a bit smaller

lyakh · 2024-07-12T07:04:00Z

@wszypelt QB seems stuck again...

marc-hb · 2024-07-12T21:59:17Z

We can either merge it all with this one, or split it (again) into two to make them a bit smaller

Thanks: it would be lovely to have a 1 or 2 days of daily test runs with #9298 alone for CI purposes. This is a pretty big PR and significant change!
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

kv2019i · 2024-07-15T12:21:18Z

src/audio/pipeline/pipeline-stream.c

+
+					/* Cannot go below 50000kcps */
+					if (data.kcps[i] > core_kcps - 50000)
+						data.kcps[i] = core_kcps - 50000;


@lyakh I think this has to be cleaned up. At least a magic number definition at start of the file and preferably a Kconfig as this is hardly a constant that covers all platforms.

I'm ok to do this as a follow-up, as this PR has so many moving parts.

kv2019i · 2024-07-15T12:24:15Z

@softwarecki @ujfalusi @marc-hb FYI, I think this is good to go now. Unless major issues raised today, I'll proceed and merge this today. This does have the upside that it extends coverage for loadable module flows in upstream testing.

abonislawski

@kv2019i please wait for @wszypelt windows full scope tests report to clarify if there is no regression with KCPS changes.

marc-hb · 2024-07-15T16:58:24Z

FYI, I think this is good to go now.

The fix for the shlex / rimage_cmd quoting issue is in the #9298 subset but not here yet.

Please wait for @wszypelt windows full scope tests report to clarify if there is no regression with KCPS changes.

@abonislawski can you please approve #9298 in the mean time? It seems to have no KCPS change.

Passing options one by one in .github/workflows/zephyr.yml was becoming unwieldy. Reduces Windows/Linux duplication. This also makes local testing easier; less typing and guess work. This should also help with thesofproject#9116. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

Under windows the Python interpreter has to be called explicitly. Without it an attempt to execute a Python script fails silently. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> Suggested-by: Marc Herbert <marc.herbert@intel.com>

When ipc4_get_drv() fails to find a driver, it might mean, that the driver needs to be linked dynamically. Printing an error in such a case wrongly fails CI testing. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Use consistent TABs and spaces in src/samples/audio/Kconfig Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Maximum instance count cannot be zero, they have to be supplied by respective modules. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Use maximum instance count from TOML when building a manifest. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

When pipelines are destroyed, component drivers' .reset() and .free() are called. If those drivers were loaded dynamically their memory is then unmapped. But logging takes place in a low priority task, so it is important that no logging is done from those methods. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

So far we cannot build identical LLEXT modules under Linux and Windows, build a monolithic firmware for this test. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Export additional symbols, required for building DRC as an LLEXT object. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Add support for LLEXT building to drc. Since multiband DRC calls functions from DRC, we cannot so far build it if DRC is configured as a module. In the future it should be possible to build both as modules and to export symbols between them. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Export missing symbols for modular DRC builds and select it as a module on MTL and LNL. DRC isn't built by default, so we cannot use CONFIG_LIBRARY_DEFAULT_MODULAR for it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

If a module contains 0 as its CPC value, the consumption calculation routine will assign a "safe" maximum value to keep the DSP running at the maximum clock rate. This works when constructing a pipeline, but when a pipeline is torn down, returning the maximum clock rate leads to the clock being reduced to a small value. Fix this by detecting such cases in pipeline termination code. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh · 2024-07-16T06:38:05Z

The fix for the shlex / rimage_cmd quoting issue is in the #9298 subset but not here yet.

@marc-hb that was a mishap, thanks for keeping an eye on it. Updated now. OTOH 9298 is missing the rimage change, that this PR is incorporating, I'll mark it a draft at least temporarily to prevent accidental merge

abonislawski · 2024-07-16T08:22:36Z

src/audio/pipeline/pipeline-stream.c

@@ -367,6 +375,9 @@ int pipeline_trigger(struct pipeline *p, struct comp_dev *host, int cmd)
 {
 	int ret;
 #if CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL
+/* FIXME: this must be a platform-specific parameter or a Kconfig option */
+#define DSP_MIN_KCPS 50000


There is no point in keeping KCPS feature with such limit because it means we cannot go lower than highest available clock which is not true because there are basic topologies which we can handle on 38.4.

So why is this change required for llext module?

@abonislawski what do you mean "lower than highest available clock?" Our highest available clock is hundreds of MHz, i.e. hundreds of thousands of KCPS, isn't it? So this is definitely lower than that.
This patch is needed not only for LLEXT, I think there's a bug in general in the current CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL implementation - see commit description. Using a "safe" value for modules with a 0 CPC isn't implemented correctly. After I've fixed it I started getting lock ups when destroying pipelines. E.g. I had cases when the system was starting with 20000 KCPS, then a pipeline was constructed with a 0-CPC module, so the KCPS went to the maximum. Then as the pipeline was destroyed I tried to return to 20000 KCPS and then I got IPC timeouts in the driver. Introducing a minimum of 50000 (or 40000 worked too) KCPS fixed the problem.

KCPS higher than 38400 will switch to our highest clock and DSP_MIN_KCPS is already higher than that. PRIMARY_CORE_BASE_CPS_USAGE is currently 20000 (higher than needed).
So the one thing is to fix 0CPC problem (which I thought was fixed with previous versions of this commit) but another thing is to introduce such limit like DSP_MIN_KCPS which I dont understand

done

abonislawski

Discussed this with @lyakh and we will unblock LLEXT now and fix KCPS in separate PR.
The only requirement for llext is to pass CPC value so please make sure it works or fix in the next PR.

cujomalainey · 2024-07-16T14:36:30Z

src/audio/drc/drc.c

+#include <module/module/llext.h>
+#include <rimage/sof/user/manifest.h>
+
+#define UUID_DRC 0xda, 0xe4, 0x6e, 0xb3, 0x6f, 0x00, 0xf9, 0x47, \


@andyross is this needed with your changes?

It's compatible (UUIDs aren't required to be registered), but yes, this should be added to the registry.

lyakh force-pushed the smatest-mtl branch 2 times, most recently from 46b48b1 to 0eeab10 Compare June 5, 2024 15:44

lyakh force-pushed the smatest-mtl branch 3 times, most recently from 18b1e37 to 660d60b Compare June 7, 2024 08:11

lyakh force-pushed the smatest-mtl branch from fc40d8e to b2f5001 Compare June 13, 2024 12:35

lyakh marked this pull request as ready for review June 13, 2024 12:35

lyakh requested review from marc-hb, singalsu, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners June 13, 2024 12:35

lyakh changed the title ~~[TEST ONLY] smart-amp-test: build as module on MTL~~ LLEXT: build everything as modules on MTL Jun 13, 2024

lyakh force-pushed the smatest-mtl branch 2 times, most recently from 2cb61a8 to 79c18cb Compare July 12, 2024 15:42

lyakh force-pushed the smatest-mtl branch from 79c18cb to f93f521 Compare July 15, 2024 08:27

kv2019i reviewed Jul 15, 2024

View reviewed changes

abonislawski requested changes Jul 15, 2024

View reviewed changes

lyakh added 11 commits July 16, 2024 08:27

llext: fix Windows builds

167c303

Under windows the Python interpreter has to be called explicitly. Without it an attempt to execute a Python script fails silently. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com> Suggested-by: Marc Herbert <marc.herbert@intel.com>

samples: (cosmetic) clean up Kconfig spacing

1f3b74f

Use consistent TABs and spaces in src/samples/audio/Kconfig Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

llext: add maximum instance count support

ebdab32

Maximum instance count cannot be zero, they have to be supplied by respective modules. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

rimage: don't overwrite maximum module instance counts

9cc1afb

Use maximum instance count from TOML when building a manifest. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

llext: disable when testing reproducible builds

6e84874

So far we cannot build identical LLEXT modules under Linux and Windows, build a monolithic firmware for this test. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

llext: export symbols, required for modular DRC

6249a3b

Export additional symbols, required for building DRC as an LLEXT object. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

drc: make modular on MTL and LNL

95517f8

Export missing symbols for modular DRC builds and select it as a module on MTL and LNL. DRC isn't built by default, so we cannot use CONFIG_LIBRARY_DEFAULT_MODULAR for it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the smatest-mtl branch from f93f521 to a529d61 Compare July 16, 2024 06:36

abonislawski requested changes Jul 16, 2024

View reviewed changes

abonislawski approved these changes Jul 16, 2024

View reviewed changes

kv2019i merged commit 3051607 into thesofproject:main Jul 16, 2024
45 of 49 checks passed

cujomalainey reviewed Jul 16, 2024

View reviewed changes

lyakh mentioned this pull request Jul 19, 2024

Avoid recent MTL regression #9314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LLEXT: fix failures and make DRC an LLEXT module by default on MTL #9116

LLEXT: fix failures and make DRC an LLEXT module by default on MTL #9116

lyakh commented May 10, 2024 •

edited

Loading

lyakh commented May 13, 2024

lyakh commented Jun 5, 2024

marc-hb commented Jun 5, 2024 •

edited

Loading

lyakh commented Jun 7, 2024 •

edited

Loading

marc-hb commented Jun 7, 2024

marc-hb commented Jun 8, 2024

lyakh commented Jun 12, 2024

lyakh commented Jun 12, 2024

lyakh commented Jun 18, 2024

wszypelt commented Jun 18, 2024

lgirdwood commented Jun 25, 2024

lyakh commented Jun 26, 2024

lgirdwood commented Jun 26, 2024

lyakh commented Jun 26, 2024

lyakh commented Jul 12, 2024

lyakh commented Jul 12, 2024

marc-hb commented Jul 12, 2024 •

edited

Loading

kv2019i Jul 15, 2024

kv2019i commented Jul 15, 2024

abonislawski left a comment

marc-hb commented Jul 15, 2024 •

edited

Loading

lyakh commented Jul 16, 2024

abonislawski Jul 16, 2024

lyakh Jul 16, 2024 •

edited

Loading

abonislawski Jul 16, 2024

abonislawski left a comment

cujomalainey Jul 16, 2024

andyross Jul 17, 2024

LLEXT: fix failures and make DRC an LLEXT module by default on MTL #9116

LLEXT: fix failures and make DRC an LLEXT module by default on MTL #9116

Conversation

lyakh commented May 10, 2024 • edited Loading

lyakh commented May 13, 2024

lyakh commented Jun 5, 2024

marc-hb commented Jun 5, 2024 • edited Loading

lyakh commented Jun 7, 2024 • edited Loading

marc-hb commented Jun 7, 2024

marc-hb commented Jun 8, 2024

lyakh commented Jun 12, 2024

lyakh commented Jun 12, 2024

lyakh commented Jun 18, 2024

wszypelt commented Jun 18, 2024

lgirdwood commented Jun 25, 2024

lyakh commented Jun 26, 2024

lgirdwood commented Jun 26, 2024

lyakh commented Jun 26, 2024

lyakh commented Jul 12, 2024

lyakh commented Jul 12, 2024

marc-hb commented Jul 12, 2024 • edited Loading

kv2019i Jul 15, 2024

Choose a reason for hiding this comment

kv2019i commented Jul 15, 2024

abonislawski left a comment

Choose a reason for hiding this comment

marc-hb commented Jul 15, 2024 • edited Loading

lyakh commented Jul 16, 2024

abonislawski Jul 16, 2024

Choose a reason for hiding this comment

lyakh Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

abonislawski Jul 16, 2024

Choose a reason for hiding this comment

abonislawski left a comment

Choose a reason for hiding this comment

cujomalainey Jul 16, 2024

Choose a reason for hiding this comment

andyross Jul 17, 2024

Choose a reason for hiding this comment

lyakh commented May 10, 2024 •

edited

Loading

marc-hb commented Jun 5, 2024 •

edited

Loading

lyakh commented Jun 7, 2024 •

edited

Loading

marc-hb commented Jul 12, 2024 •

edited

Loading

marc-hb commented Jul 15, 2024 •

edited

Loading

lyakh Jul 16, 2024 •

edited

Loading