LLEXT multiple modules #9090

lyakh · 2024-04-30T13:49:43Z

This converts two more modules: mixin-mixout and eq_iir to llext modules. This has an added complexity of mixin-mixout containing two module-adapter drivers. This PR currently contains #9013 with an expectation of that being merged soon, after which the first 7 commits will be dropped from this one.

lgirdwood

Just minor optimization questions.

lgirdwood · 2024-05-03T09:51:30Z

src/audio/eq_iir/eq_iir.c

+#if CONFIG_COMP_IIR_MODULE
+/* modular: llext dynamic link */
+
+static const struct module_interface *loadable_eq_iir_main(void *mod_cfg,
+							   void *parent_ppl,
+							   void **mod_ptr)
+{
+	return &eq_iir_interface;
+}
+
+static const struct sof_man_module_manifest eq_iir_manifest __section(".module") __used = {
+	.module = {
+		.name = "EQIIR",
+		.uuid = {0xE6, 0xC0, 0x50, 0x51, 0xF9, 0x27, 0xC8, 0x4E,
+			 0x83, 0x51, 0xC7, 0x05, 0xB6, 0x42, 0xD1, 0x2F},
+		.entry_point = (uint32_t)loadable_eq_iir_main,
+		.type = {
+			.load_type = SOF_MAN_MOD_TYPE_LLEXT,
+			.domain_ll = 1,
+		},
+		.affinity_mask = 1,
+	}
+};
+
+static const struct sof_module_api_build_info buildinfo __section(".mod_buildinfo") __used = {
+	.format = SOF_MODULE_API_BUILD_INFO_FORMAT,
+	.api_version_number.full = SOF_MODULE_API_CURRENT_VERSION,
+};
+
+#endif


I think all of this can be expressed as a single macro i.e.

SOF_DECLARE_MODULE(name, interface, uuid);

lgirdwood · 2024-05-03T09:53:42Z

src/audio/eq_iir/llext/CMakeLists.txt

@@ -0,0 +1,99 @@
+# Copyright (c) 2024 Intel Corporation.


I'm assuming this file will be shrunk/replaced when Zephyr llext cmake helpers are used ?

lgirdwood · 2024-05-06T15:58:59Z

src/audio/eq_iir/llext/CMakeLists.txt

+add_llext_command(TARGET ${MODULE}
+	POST_BUILD
+	COMMAND ${SOF_BASE}scripts/llext_link_helper.py
+		--text-addr="0xa06ea000" -f ${proc_in_file} ${CMAKE_C_COMPILER} --


where does this address come from ? Does it need to come from platform / DT or Kconfig ?

@lgirdwood right, now this is a good question, let's talk about this. These address values come originally from the original loadable module concept, whereby we build modules for a fixed address in memory - I think that holds for system services too, probably for IADK as well. Having those addresses helps - it makes linking at run time easier, internal address references are already correct, but it does mean that we need to maintain those addresses somehow somewhere. The original one, that is now used for the smart-amp was exactly the address used in some old examples. Now for mixin-mixout and eq-iir I picked up some addresses "similar" to smart-amp but far enough from it... But you're right, we need to come up with a system of how to assign these addresses or maybe to aim to get rid of them completely and to perform full run-time linking / relocation instead

At the very least don't copy/paste/diverge it in every module. Define it in one place and ta-da: you can paste your explanation above in that place.

yeah, we can have a comment today and have a per platform memory address for this in the Zephyr llext core as next steps, probably best to align with @teburd

I had some ideas around this by reusing the devicetree flash partitions or something like it, which is also used by mcuboot to designate memory regions for images.

E.g. imagine a set of partitions described for fixed location modules/applications akin to this dts:
https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/nxp/frdm_k64f/frdm_k64f.dts#L229

I think @dapperlo would also be interested (also using parts of llext) zephyrproject-rtos/zephyr#72258

Interested indeed. I need to ramp on the context for this use-case, but there is a desire for fixed address extensions and the load-time benefits that comes from it.

Nodes in the dts sounds reasonable, routing loaded .llext blobs could then use a label/attribute in blob header to associate.

yeah, we can have a comment today and have a per platform memory address for this in the Zephyr llext core as next steps, probably best to align with @teburd

It isn't just per platform, it's also per module. This is the starting address of this module on this platform. So, we could have a per-platform address range for loadable modules and then per-module offsets defined in modules themselves? But that would mean some co-ordination in terms of sufficiently large offsets. I'm not sure how large is the usable address range e.g. on MTL? If it's large enough we could just dedicate an MB per module?

Alternatively: we already have TOMLs per platform that include all modules, so I'm wondering if we could add in all those locations like

sof/tools/rimage/config/mtl.toml.h

Line 29 in 374d2d6

#ifdef CONFIG_COMP_COPIER

#define ${MODULE}_base 0x... statements (e.g. #define smart_amp_test_base 0xa06ca000) but I'm not sure we'd be able to pull those out into cmake.

E.g. imagine a set of partitions described for fixed location modules/applications akin to this dts:
https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/nxp/frdm_k64f/frdm_k64f.dts#L229

@teburd yes, looks good, with a slight inconvenience that we now have on MTL something like 28 modules. Maybe a separate .dts, included into the main one

I have no issue with toml either but I’ll need help in understanding how we piece this together. Do we need to link the extensions to be placed appropriately with a custom linker script per extension slot? Sure seems like it to me at first glance but please do correct me if wrong.

@teburd no, I tried very hard and so far have succeeded in avoiding custom linker scripts. I "only" provide section addresses to the linker on the command line. For that I only take a single base address as input and calculate all further addresses automatically.

kv2019i

No blockers really, a few comments inline.

kv2019i · 2024-05-07T11:14:29Z

src/include/sof/common.h

+#ifndef typeof
+#define typeof  __typeof__
+#endif
+


Don't we have a Zephyr solution for this? We should be shrinking, not growing the sof/common.h.

we could just replace all typeof() with __typeof__(), I haven't found anything global in Zephyr, they have two "local" definitions in arch/arm/include/cortex_m/cmse.h and in drivers/usb/device/usb_dc_rpi_pico.c

@kv2019i I think I'll do that - replace that patch with a switch to __typeof__() globally in SOF

kv2019i · 2024-05-07T11:16:13Z

src/samples/audio/smart_amp_test_llext/CMakeLists.txt

-		-o ${MODULE}_pre.so $<TARGET_OBJECTS:${MODULE}>
-	COMMAND ${COPY_CMD}
-	COMMAND_EXPAND_LISTS
+		--text-addr="0xa06ca000" -f ${proc_in_file} ${CMAKE_C_COMPILER} --


This magic address does pop out given this is no way platform specific file.

marc-hb

The CMake files are 95% copy/paste/diverge! CMake sucks but it has functions and macros. Define a new sof_add_llext() function and problem solved.

marc-hb · 2024-05-07T22:06:24Z

src/samples/audio/smart_amp_test_llext/CMakeLists.txt

-project(smart_amp_test)
-
-SET_PROPERTY(GLOBAL PROPERTY TARGET_SUPPORTS_SHARED_LIBS TRUE)
+cmake_path(SET SOF_BASE NORMALIZE ${CMAKE_CURRENT_SOURCE_DIR}/../../../..)


The quadruple .. is quite ugly, how about ${APPLICATION_SOURCE_DIR}/.. instead?

marc-hb · 2024-05-07T22:11:56Z

src/audio/eq_iir/llext/CMakeLists.txt

+	# extract UUID value - drop the 'uuid = ' part of the assignment line
+	string(REGEX REPLACE "^[ \t]*uuid *= \"([0-9A-F\-]*)\"" "\\1" uuid ${line})
+	file(APPEND ${ZEPHYR_BINARY_DIR}/${MODULE}_llext/llext.uuid "${uuid}\n")
+endforeach()


This is fairly complex regex, please don't copy/paste/diverge it in every module. Make this a macro or function in zephyr/CMakeLists.txt

Warning: CMake function can't "return" anything: they can only set "pseudo-return" variables. Find a very simple macro example in ef563eb

marc-hb · 2024-05-07T22:13:34Z

src/audio/eq_iir/llext/CMakeLists.txt

+	COMMAND ${CMAKE_C_COMPILER} -E ${CMAKE_CURRENT_LIST_DIR}/llext.toml.h -P -DREM=
+		-I${SOF_BASE} -I${SOF_BASE}src
+		-imacros ../include/generated/autoconf.h
+		-o rimage_config.toml


This one does not return anything so it is trivial to make it a shared function in zephyr/CMakeLists.txt

marc-hb · 2024-05-07T22:15:37Z

src/audio/eq_iir/llext/CMakeLists.txt

+add_llext_command(TARGET ${MODULE}
+	POST_BUILD
+	COMMAND ${SOF_BASE}scripts/llext_link_helper.py
+		--text-addr="0xa06ea000" -f ${proc_in_file} ${CMAKE_C_COMPILER} --


At the very least don't copy/paste/diverge it in every module. Define it in one place and ta-da: you can paste your explanation above in that place.

marc-hb

suffix confusion
mass CMake duplication

marc-hb · 2024-05-07T23:21:45Z

scripts/xtensa-build-zephyr.py

-			llext_input = entry_path / (llext_base + '.so')
-			llext_output = entry_path / llext_file
+			llext_input = entry_path / (llext_base + '.llext')
+			llext_output = entry_path / (llext_file + '.ri')


Err... no, please don't go from .llext to .ri and then BACK to .llext where each step is a different file type; this is incredibly confusing!

One suggestion:

.llext because that's what comes from Zephyr so you don't have a choice?

.ri0 because that's the first rimage stage. Similar to zephyr_pre0

Either .ri or .llext.ri.

@marc-hb yeah, .llext -> something else -> .llext is ugly, I agree. There was a wish to have .llext as the final suffix, but now with the use of Zephyr cmake .llext comes out of those functions. Maybe yes - .llext.ri would be a good option for the final output

marc-hb · 2024-05-09T19:49:06Z

I think this PR should be split in (at least) two PRs:

switch to add_llext_target() and other refactoring of the single smart_amp module
Additions of new modules based on 1.

When compiled with '-std=c99' or newer 'typeof' is unavailable and '__typeof__' should be used instead [1]. [1] https://gcc.gnu.org/onlinedocs/gcc/Alternate-Keywords.html Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Add 3 macros to simplify LLEXT module implementation and use them in mixin-mixout and eq_iir. Only two of them can be used in smart_amp_test_ipc4.c so far, because it's also used for LMDK-style module building. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Zephyr now provides a convenient cmake API for LLEXT modules, update SOF to use it by defining common cmake functions. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

west.configuration.get() already returns a string, no need to call 'str()' on it again. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Convert mixin-mixout to a loadable llext module. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Build eq-iir as a loadable llext module. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Export additional symbols, needed for modules like eq_iir and mixin-mixout. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

If module_adapter_new() fails in lib_manager_module_create(), lib_manager_free_module() must be called to free allocated resources. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Some components like mixin-mixout implement multiple modules internally. They should be handled by LLEXT as a single ELF object, including for use-counting. But at the SOF level they implement multiple module adapter drivers. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh · 2024-05-10T09:16:48Z

I think this PR should be split in (at least) two PRs:

1. switch to add_llext_target() and other refactoring of the single smart_amp module

2. Additions of new modules based on 1.

@marc-hb individual commits like mixin-mixout and eq-iir have become much smaller now (and don't say you didn't ask for that), so, maybe 1 PR would be acceptable

lyakh · 2024-05-10T11:50:08Z

CI: a single alsabat failure on LNL shouldn't be related https://sof-ci.01.org/sofpr/PR9090/build4606/devicetest/index.html?model=LNLM_SDW_AIOC&testcase=check-alsabat-headset-playback-997
Checkpatch complains about existing code patterns in SOF container_of() implementation, a multi-component UUID definition and an opening curly brace on a new line. There's also one trivial typo, nothing exciting https://github.com/thesofproject/sof/actions/runs/9030070153/job/24813619935?pr=9090

lgirdwood

This is good to go, the remaining opens are all second order and can be resolved incrementally. The important thing here is to get this code to the CI devs for integration into test/build/sign services.

kv2019i

My comments addressed, refresh +1

marc-hb · 2024-05-15T03:58:23Z

Looks like this was merged broken:

https://github.com/thesofproject/sof/actions/runs/9030070149/job/24813622867
https://github.com/thesofproject/sof/actions/runs/9088127947

/opt/toolchains/zephyr-sdk-0.16.4/aarch64-zephyr-elf/bin/../lib/gcc/aarch64-zephyr-elf/12.2.0/../../../../aarch64-zephyr-elf/bin/ld.bfd:
modules/sof/libmodules_sof.a(numbers.c.obj):(._llext_const_symbol.static.__udivdi3_sym_+0x8): undefined reference to `__udivdi3'
collect2: error: ld returned 1 exit status

cc: @LaurentiuM1234

lyakh · 2024-05-16T15:25:30Z

Looks like this was merged broken:

https://github.com/thesofproject/sof/actions/runs/9030070149/job/24813622867 https://github.com/thesofproject/sof/actions/runs/9088127947
/opt/toolchains/zephyr-sdk-0.16.4/aarch64-zephyr-elf/bin/../lib/gcc/aarch64-zephyr-elf/12.2.0/../../../../aarch64-zephyr-elf/bin/ld.bfd:
modules/sof/libmodules_sof.a(numbers.c.obj):(._llext_const_symbol.static.__udivdi3_sym_+0x8): undefined reference to `__udivdi3'
collect2: error: ld returned 1 exit status
cc: @LaurentiuM1234

@marc-hb @LaurentiuM1234 #9130

lyakh force-pushed the llext-multi branch 2 times, most recently from 752cbf2 to 7f78163 Compare May 2, 2024 12:50

lgirdwood reviewed May 3, 2024

View reviewed changes

lyakh force-pushed the llext-multi branch from 7f78163 to 20c2479 Compare May 3, 2024 14:52

lgirdwood reviewed May 6, 2024

View reviewed changes

lyakh force-pushed the llext-multi branch from 20c2479 to 3e1fbd3 Compare May 7, 2024 10:54

lyakh changed the title ~~[PREVIEW] LLEXT multiple modules~~ LLEXT multiple modules May 7, 2024

lyakh marked this pull request as ready for review May 7, 2024 10:55

lyakh requested review from marc-hb, singalsu, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners May 7, 2024 10:55

kv2019i reviewed May 7, 2024

View reviewed changes

lyakh force-pushed the llext-multi branch from 3e1fbd3 to f1afa7a Compare May 7, 2024 13:37

marc-hb mentioned this pull request May 7, 2024

llext: install modules appropriately for testing #9013

Merged

marc-hb reviewed May 7, 2024

View reviewed changes

marc-hb requested changes May 7, 2024

View reviewed changes

lyakh added 9 commits May 10, 2024 10:32

llext: update to use add_llext_target()

0283582

Zephyr now provides a convenient cmake API for LLEXT modules, update SOF to use it by defining common cmake functions. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

build: remove a redundant call to 'str()'

8b9803c

west.configuration.get() already returns a string, no need to call 'str()' on it again. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

mixin-mixout: make modular

d2cfe1c

Convert mixin-mixout to a loadable llext module. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

eq-iir: convert to a loadable module

df5392b

Build eq-iir as a loadable llext module. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

llext: add missing symbol exports

e8bb27d

Export additional symbols, needed for modules like eq_iir and mixin-mixout. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lib_manager: fix a memory leak

51740df

If module_adapter_new() fails in lib_manager_module_create(), lib_manager_free_module() must be called to free allocated resources. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the llext-multi branch from f1afa7a to cb5693f Compare May 10, 2024 09:15

lyakh requested a review from marc-hb May 13, 2024 08:12

lgirdwood approved these changes May 14, 2024

View reviewed changes

kv2019i approved these changes May 14, 2024

View reviewed changes

lgirdwood merged commit 01228bb into thesofproject:main May 14, 2024
43 of 47 checks passed

lyakh deleted the llext-multi branch May 14, 2024 14:58

kv2019i mentioned this pull request May 16, 2024

[BUG] build error undefined reference to `__udivdi3 in mimx93_evk_a55 build #9129

Closed

lyakh mentioned this pull request Jun 5, 2024

Zephyr: fix build breakage with Zephyr "main" #9199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LLEXT multiple modules #9090

LLEXT multiple modules #9090

lyakh commented Apr 30, 2024

lgirdwood left a comment

lgirdwood May 3, 2024

lgirdwood May 3, 2024

lgirdwood May 6, 2024

lyakh May 7, 2024

marc-hb May 7, 2024

lgirdwood May 8, 2024

teburd May 9, 2024 •

edited

Loading

dapperlo May 9, 2024

lyakh May 10, 2024

lyakh May 10, 2024

teburd May 10, 2024

lyakh May 13, 2024

kv2019i left a comment

kv2019i May 7, 2024

lyakh May 7, 2024

lyakh May 7, 2024

kv2019i May 7, 2024

marc-hb left a comment

marc-hb May 7, 2024

marc-hb May 7, 2024

marc-hb May 7, 2024

marc-hb May 7, 2024

marc-hb left a comment

marc-hb May 7, 2024 •

edited

Loading

lyakh May 8, 2024

marc-hb commented May 9, 2024

lyakh commented May 10, 2024

lyakh commented May 10, 2024

lgirdwood left a comment

kv2019i left a comment

marc-hb commented May 15, 2024 •

edited

Loading

lyakh commented May 16, 2024

LLEXT multiple modules #9090

LLEXT multiple modules #9090

Conversation

lyakh commented Apr 30, 2024

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb left a comment

Choose a reason for hiding this comment

marc-hb May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb commented May 9, 2024

lyakh commented May 10, 2024

lyakh commented May 10, 2024

lgirdwood left a comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

marc-hb commented May 15, 2024 • edited Loading

lyakh commented May 16, 2024

teburd May 9, 2024 •

edited

Loading

marc-hb May 7, 2024 •

edited

Loading

marc-hb commented May 15, 2024 •

edited

Loading