Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loadable libraries and smart-amp-test as an example #8180

Merged
merged 9 commits into from
Mar 8, 2024

Conversation

lyakh
Copy link
Collaborator

@lyakh lyakh commented Sep 8, 2023

This implements support for loadable libraries in SOF and adds the necessary interface to the smart-amp-test to be built and used as such a module. Still WiP

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a lot of patches here are ready to be sent as seperate PRs.
Additionally, we need to use the same module build and linking methodolody as Linux i.e. .o file instead of shared libraries.

Comment on lines 39 to 46
"${SOF_BASE}/zephyr/include"
"${SOF_BASE}/xtos/include"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xtos ?

Copy link
Collaborator Author

@lyakh lyakh Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood yes, needed for compiler_attributes.h, although might be possible to get rid of it - builds without it at least as a module too

@@ -0,0 +1,23 @@
cmake_minimum_required(VERSION 3.20)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should all go in the module directory for smart amp, same for each module.

@@ -0,0 +1,80 @@
version = [3, 0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should really run the pre-processor on the toml files like we do with linker scripts. @kv2019i fyi.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood @aiChaoSONG Hmm, given we always have one platform and one algo bit, not sure we need to go that far. But for sure this is one option for thesofproject/rimage#176 One benefit is that we'll have cpp on all platforms, so this would cover the portability part.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so upgrading to use cpp here would also be easy enough to do and also allow us to pick UUIDs and memory addreses and sizes from headers (instead of coding them more than once).

Comment on lines 3 to 26
[adsp]
name = "mtl"
image_size = "0x2C0000" # (22) bank * 128KB
alias_mask = "0xE0000000"

[[adsp.mem_zone]]
type = "ROM"
base = "0x1FF80000"
size = "0x400"
[[adsp.mem_zone]]
type = "IMR"
base = "0xA104A000"
size = "0x2000"
[[adsp.mem_zone]]
type = "SRAM"
base = "0xa00f0000"
size = "0x100000"

[[adsp.mem_alias]]
type = "uncached"
base = "0x40000000"
[[adsp.mem_alias]]
type = "cached"
base = "0xA0000000"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can the #include <platform.tml> for above

Comment on lines 27 to 57

[cse]
partition_name = "ADSP"
[[cse.entry]]
name = "ADSP.man"
offset = "0x5c"
length = "0x464"
[[cse.entry]]
name = "ADSP.met"
offset = "0x4c0"
length = "0x70"
[[cse.entry]]
name = "ADSP"
offset = "0x540"
length = "0x0" # calculated by rimage

[css]

[signed_pkg]
name = "ADSP"
partition_usage = "0x23"
[[signed_pkg.module]]
name = "ADSP.met"

[adsp_file]
[[adsp_file.comp]]
base_offset = "0x2000"

[fw_desc.header]
name = "ADSPFW"
load_offset = "0x40000"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can the #include <vendor.tml>

@@ -30,9 +30,6 @@ void *native_system_agent_start(uint32_t *sys_service,
native_sys_agent.log_handle = log_handle;

void *system_agent_p = &native_sys_agent;
uint32_t **sys_service_p = &sys_service;

*sys_service_p = (uint32_t *)(&native_sys_agent.system_service);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seperate fix ?

@@ -222,8 +222,7 @@ int module_prepare(struct processing_module *mod,
* as it has been applied during the procedure - it is safe to
* free it.
*/
if (md->cfg.data)
rfree(md->cfg.data);
rfree(md->cfg.data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seperate patch

@@ -782,7 +782,7 @@ static int ipc4_process_glb_message(struct ipc4_message_request *ipc4)

static int ipc4_init_module_instance(struct ipc4_message_request *ipc4)
{
struct ipc4_module_init_instance module_init = {};
struct ipc4_module_init_instance module_init;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seperate fix.

Comment on lines 76 to 78
"-Wl,-Ttext=0xa06ca000"
"-Wl,-Tdata=0xa06cb000"
"-Wl,--section-start=.rodata=0xa06cb100"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic - are we copied here ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a comment there - need a script to calculate these. The approach is rather simple: .text is fixed (per module? to make them unique?) the .data should then be page-aligned. Haven't been able to achieve that in a better way so far

@@ -39,6 +39,8 @@ struct smart_amp_data {
uint32_t out_channels;
};

static int keep_bss;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a comment.

@@ -65,7 +74,9 @@ static int smart_amp_init(struct processing_module *mod)

if (base_cfg->base_cfg_ext.nb_input_pins != SMART_AMP_NUM_IN_PINS ||
base_cfg->base_cfg_ext.nb_output_pins != SMART_AMP_NUM_OUT_PINS) {
comp_err(dev, "smart_amp_init(): Invalid pin configuration");
comp_err(dev, "smart_amp_init(): Invalid pin configuration: %d %d",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep changes to bare minimum needed in any module. Chnages to log messages should be another PR.

@lyakh lyakh force-pushed the ldlib-wip branch 2 times, most recently from 2526537 to 2527242 Compare September 29, 2023 12:37
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we have some seperate fixes and patches in the PR and it seems like they could upstream now as separate PRs.
@pblaszko pls see if any of @lyakh makefile changes will help your PR to reuse headers.

Comment on lines 307 to 310
/*
* Module unloading isn't implemented yet, it has to be done from a
* dedicated IPC, not when stopping streams.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Module unloading come from ref counting users based on running pipelines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, forgot to update the comment, fixing, thanks

Comment on lines 74 to 116
/* At this point module resources are allocated and it is moved to L2 memory. */
module_entry_point = lib_manager_allocate_module(dev->drv, config, src_cfg);
if (module_entry_point == 0) {
comp_err(dev, "modules_init(), lib_manager_allocate_module() failed!");
return -EINVAL;
}
md->module_entry_point = module_entry_point;
comp_info(mod->dev, "modules_init() start");

uint32_t module_id = IPC4_MOD_ID(mod->dev->ipc_config.id);
uint32_t instance_id = IPC4_INST_ID(mod->dev->ipc_config.id);
uint32_t log_handle = (uint32_t) mod->dev->drv->tctx;
/* Connect loadable module interfaces with module adapter entity. */
/* Check if native Zephyr lib is loaded */
struct sof_man_fw_desc *desc;

desc = lib_manager_get_library_module_desc(module_id);
if (!desc) {
comp_err(dev, "modules_init(): Failed to load manifest");
return -ENOMEM;
}
struct sof_man_module *module_entry =
(struct sof_man_module *)((char *)desc + SOF_MAN_MODULE_OFFSET(0));
if (!md->module_adapter) {
byte_array_t mod_cfg;

struct sof_module_api_build_info *mod_buildinfo =
(struct sof_module_api_build_info *)
(module_entry->segment[SOF_MAN_SEGMENT_TEXT].v_base_addr);
mod_cfg.data = (uint8_t *)md->cfg.init_data;
/* Intel modules expects DW size here */
mod_cfg.size = md->cfg.size >> 2;
md->private = mod;

void *mod_adp;
struct comp_ipc_config *config = &(mod->dev->ipc_config);

/* Check if module is FDK*/
if (mod_buildinfo->api_version_number.fields.major < SOF_MODULE_API_MAJOR_VERSION) {
mod_adp = system_agent_start(md->module_entry_point, module_id,
instance_id, 0, log_handle, &mod_cfg);
} else {
/* If not start agent for sof loadable */
mod->is_native_sof = true;
mod_adp = native_system_agent_start(mod->sys_service, md->module_entry_point,
module_id, instance_id, 0, log_handle,
&mod_cfg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you go into more detail what is being modified here.

@lyakh lyakh force-pushed the ldlib-wip branch 4 times, most recently from c413d69 to 003e828 Compare October 4, 2023 15:26
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See its been updated to newer Zephyr. One other thing we need is to have the module specific infra in the module directory, I dont want a duplication of stuff in module/ and lmdk/

@lyakh lyakh force-pushed the ldlib-wip branch 2 times, most recently from 333d97d to 49ed952 Compare November 21, 2023 14:05
@lyakh lyakh force-pushed the ldlib-wip branch 3 times, most recently from 021cc8d to 4efcce6 Compare December 5, 2023 16:55
@lyakh lyakh changed the title [WIP] loadable libraries and smart-amp-test as an example [WIP][DNM] loadable libraries and smart-amp-test as an example Dec 5, 2023
@lyakh
Copy link
Collaborator Author

lyakh commented Dec 5, 2023

Not all of it should be merged (e.g. no need for Zephyr PR and we need to decide whether we set the smart-amp sample as module hard), but it's now close to the final version, I think, I'd say, almost no hacks.

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 17, 2024

From zephyrproject-rtos/zephyr#67431 (comment)

That code was introduced in zephyrproject-rtos/zephyr@aee2d1a as the only example usage of tristate in the Zephyr code. There is no actual need to do that, but I did not want to remove it either, so I just moved that around.

This is typical: "later cleanup" = "never".

Also no clear reason for 17 commits in a single PR
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

All while we all already have severe crashes: #8747

The opposite of "Continuous" integration.

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 18, 2024

I'm good to go providing the IADK comment from @jxstelter is resolved alongside any CI issues are resolved. We can come back and align with Zephyr cmake once its merged.

@lgirdwood yes: #8180 (comment)

@lyakh lyakh mentioned this pull request Jan 18, 2024
@marc-hb
Copy link
Collaborator

marc-hb commented Jan 19, 2024

The review of the llext+CMake work in Zephyr is going extremely well, multiple approvals including from the maintainer:

@lyakh please download it and give it at least a try before merging this.

At the very least we don't want this PR #8180 to immediately introduce a (logical) conflict with this Zephyr PR 67431 about to be merged.

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 22, 2024

The review of the llext+CMake work in Zephyr is going extremely well, multiple approvals including from the maintainer:

* [cmake: add llext compilation module zephyrproject-rtos/zephyr#67431](https://github.com/zephyrproject-rtos/zephyr/pull/67431)

@lyakh please download it and give it at least a try before merging this.

At the very least we don't want this PR #8180 to immediately introduce a (logical) conflict with this Zephyr PR 67431 about to be merged.

@marc-hb that PR is now merged, but it doesn't help us here a whole lot. It doesn't implement address calculations for us. So at best we can use the new add_llext_target() for the first stage - compiling sources and generating an intermediate binary, but then we anyway need to add a custom target to rewrite addresses. So I still think this can be done as a second step after this PR is merged

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 22, 2024

but then we anyway need to add a custom target to rewrite addresses.

Thanks for looking at it! Any idea how we can solve this in the longer term? (= never?). I remember you said it's SOF-specific but SOF is one of the very first llext users so it's still a pity. @pillo79 any idea, even vague?

One thing this PR seems to miss compared to the way Zephyr does it is CFLAGS management...

@pillo79
Copy link

pillo79 commented Jan 23, 2024

@marc-hb I talked about this briefly yesterday with @lyakh - I am preparing a tiny followup to that PR with a custom post processing step. That should help!


CONFIG_LLEXT=y
CONFIG_LLEXT_STORAGE_WRITABLE=y
CONFIG_MODULES=y
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have 3 different approaches to loading modules, I wouldn't use such generic name. It cause misunderstanding.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pjdobrowolski it isn't a name that we choose, it's hard-coded in kconfig tools. We want to use tristate in module Kconfig and set it to "m" to build as a module. And for that you need CONFIG_MODULES. And I'd propose to use tristate for any modules - whichever implementation they use, but that's up to respective authors / maintainers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modules:
The Symbol instance for the modules symbol. Currently hardcoded to
MODULES, which is backwards compatible. Kconfiglib will warn if
'option modules' is set on some other symbol. Tell me if you need proper
'option modules' support.

Can't we add another specific symbol for LLEXT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tell me if you need proper 'option modules' support.

Please source quotations. This one comes from here:
https://github.com/ulfalizer/Kconfiglib/blame/061e71f7d78cb0/kconfiglib.py#L671

This comment is 7 years old. Zephyr modules are months old and still in the making. I guess this text should be updated.

The reason @teburd and @lyakh could implement Zephyr modules without making any kconfiglib.py change is because kconfiglib.py has stuck to bug-for-bug compatibility with the original Kconfig implementation in C. https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html
I suspect the kconfiglib.py test suite covered modules even when Zephyr didn't support them yet.

Diverging kconfiglib.py away from the original Linux kernel implementation could break the kconfiglib.py test suite and make them slightly incompatible with each other, require slightly different documentations, disrupt other Zephyr users besides SOF,...A potentially huge amount of hassle for a small naming gain.

BTW kconfiglib.py is also used outside Zephyr.

I agree calling one of SOF's "3 different approaches" with a generic "CONFIG_MODULES" sucks, but Zephyr seems to have only one approach so it has no reason to disambiguate. This is hopefully just an implementation detail that most users will not pay much attention to: Kconfig has a user interface that shows the help text.

Note we have "HOST" abbreviated to "HST" and register sets called DfPMCCH. This is not an excuse not to care about names but it put things in a bit of perspective :-)

In any case this sounds like a Zephyr topic, not an SOF topic?

Copy link
Collaborator Author

@lyakh lyakh Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context, @marc-hb

Tell me if you need proper 'option modules' support.

Please source quotations. This one comes from here: https://github.com/ulfalizer/Kconfiglib/blame/061e71f7d78cb0/kconfiglib.py#L671

This comment is 7 years old. Zephyr modules are months old and still in the making. I guess this text should be updated.

Just to clarify a bit - it isn't just about the text (documentation), it's in the kconfiglib implementation. And yes, your text below actually alludes to that too.

[snip]

I agree calling one of SOF's "3 different approaches" with a generic "CONFIG_MODULES" sucks, but Zephyr seems to have only one approach so it has no reason to disambiguate. This is hopefully just an implementation detail that most users will not pay much attention to: Kconfig has a user interface that shows the help text.

This isn't an option for one of the 3 different approaches. Other approaches are welcome and encouraged to use it too! IMHO it's a simple and natural way to switch a module between a built-in and modular build, so, any Kconfig options, that want this functionality, are free to use this! E.g. if we modularise our Maths libraries. As for which of the "three approaches" is used - I'd propose to use other means to make those decisions, if we ever have modules, that can be built with more than one option. E.g. indeed with this PR the smart-amp-test module would be buildable in three ways - built-in, as a module using system services or as an LLEXT. Currently The "y" Kconfig option selects whether it's built-in, selecting "m" build it as an LLEXT immediately during the base-firmware build, and to build it as a system-services module you run a separate cmake command, which ignores this Kconfig option completely, so you can build such a module regardless of this Kconfig value.

Copy link
Collaborator

@marc-hb marc-hb Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lyakh . I didn't expect system-services modules to ignore the Kconfig of the base firmware (sounds quite risky... even autoconf.h?) but if they do then this entire discussion seems indeed irrelevant.

# We need to calculate ELF section addresses of an LLEXT module and use them to
# run the linker. We could just use Python to calculate addresses and pass them
# back to cmake to have it call the linker. However, there doesn't seem to be a
# portable way to do that. Therefore we pass the linker path and all the command
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we do it in linker scripts?

Add support for loadable modules, built for dynamic linking with
Zephyr's LLEXT API.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lib-manager is module-adapter specific, it doesn't need the component
API and shouldn't use struct comp_driver in function arguments.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Module adapter drivers can be loaded and unloaded, using the Zephyr
loadable extension API. Add its context to struct struct module_data.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Pointers that we store in a global array for each loaded library,
aren't really firmware manifest descriptor pointers, they're pointers
to storage buffers, where entire libraries are stored. A
SOF_MAN_ELF_TEXT_OFFSET offset has to be added to that address to get
to the manifest descriptor.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Regroup code in modules_init() to move declarations and assignments
closer to where they're needed.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lib_manager_allocate_module() returns a pointer as an integer, use
uintptr_t for it.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The first part modules_new() is only needed when a new module is
registered, the rest is needed every time a module is instantiated.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
While modules are in use, no need to unload and re-load them,
re-initialising audio interfaces is enough. With llext modules this
uses the LLEXT reference counting to identify when a module should be
unloaded.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Convert the smart-amp-test in its IPC4 version to a loadable LLEXT
module. Use an overlay configuration to select between monolithic and
modular builds.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
@lyakh
Copy link
Collaborator Author

lyakh commented Mar 6, 2024

@pjdobrowolski have we answered all your concerns? If so please consider removing your request for change

@pjdobrowolski pjdobrowolski dismissed their stale review March 7, 2024 08:45

changes done.

@lyakh
Copy link
Collaborator Author

lyakh commented Mar 7, 2024

Now that we upgraded to a Zephyr version with CONFIG_MODULES support, this is mergeable (again). CI failures: https://sof-ci.01.org/sofpr/PR8180/build3168/devicetest/index.html?model=MTLP_RVP_NOCODEC&testcase=multiple-pause-resume-50 I think is known and unrelated (@kv2019i @marc-hb am I right?), https://sof-ci.01.org/sofpr/PR8180/build3169/devicetest/index.html is suspend-resume failures, style - one warning is a moved code line, another one is __used vs. __attribute__((used)) - this one could be addressed I think, Zephyr does have a __used macro, SOF should be able to use it too.

@lyakh lyakh requested a review from softwarecki March 7, 2024 09:11
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments, but given this has been in the works for a long time and touches a lot of code, I'd lean to merge this as-is and address minor issues in follow-up PRs. @softwarecki I think you'll need to rebase yours after this, but probably easier for you than this big set, so if ok, let's go with this first.

"$$(${SOF_BASE}scripts/calc_elf_addresses.py" -f lib${MODULE}.so -t "A06CA000)"
-shared -fPIC
-o lib${MODULE}_llext.so $<TARGET_OBJECTS:${MODULE}>
COMMAND ${CMAKE_STRIP} -R .xt.* -o lib${MODULE}_out.so lib${MODULE}_llext.so
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh ? To me using same extension as upstream Zephyr seems like a good alignment

@kv2019i kv2019i merged commit 980b1c6 into thesofproject:main Mar 8, 2024
35 of 44 checks passed
@lyakh lyakh deleted the ldlib-wip branch March 8, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants