Skip to content

library-manager: use Kconfig for INTEL_MODULES #9936

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.

Sign up for GitHub

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

Merged
lgirdwood merged 1 commit into from
Apr 4, 2025
Merged

library-manager: use Kconfig for INTEL_MODULES #9936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/audio/module_adapter/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,20 @@ if(zephyr) ### Zephyr ###
zephyr_library_import(xa_mp3_enc ${CONFIG_CADENCE_CODEC_MP3_ENC_LIB})
endif()

zephyr_include_directories_ifdef(CONFIG_INTEL_MODULES
zephyr_include_directories_ifdef(CONFIG_LIBRARY_MANAGER
Copy link
Member

@lgirdwood lgirdwood Apr 2, 2025

Choose a reason for hiding this comment

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

Would this not be something like the original CONFIG ? Dont we want to preserve the original Kconfig to select IADK API ?

Copy link
Collaborator Author

@lyakh lyakh Apr 2, 2025

Choose a reason for hiding this comment

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

@lgirdwood not sure I understand correctly. Currently my understanding is, that CONFIG_LIBRARY_MANAGER is needed always when using the library manager, i.e. for both LLEXT and IADK. While CONFIG_INTEL_MODULES is only needed for IADK. So, here I change it to the former to make LLEXT usable without the latter enabled.

Copy link
Member

@abonislawski abonislawski Apr 3, 2025

Choose a reason for hiding this comment

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

So we need sof/audio/module_adapter/iadk/ directory just with CONFIG_LIBRARY_MANAGER=y?
looks like it should be included only with CONFIG_INTEL_MODULES=y

Copy link
Collaborator Author

@lyakh lyakh Apr 4, 2025

Choose a reason for hiding this comment

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

@abonislawski unfortunately yes. Obviously, I tried building SOF with LLEXT modules with CONFIG_INTEL_MODULES=n and then I got errors about headers not being found. When I removed IADK headers, I got undefined types for some structures. We can work more on this to decouple non-IADK use-cases from IADK headers, but in this first step I didn't go all the way to do that, it would involve for me making less trivial changes to code that I cannot test locally

${SOF_SRC_PATH}/include/sof/audio/module_adapter/iadk/
${SOF_SRC_PATH}/include/sof/audio/module_adapter/library/
)

zephyr_library_sources_ifdef(CONFIG_LIBRARY_MANAGER
library/native_system_agent.c
)

zephyr_library_sources_ifdef(CONFIG_INTEL_MODULES
module/modules.c
iadk/module_initial_settings_concrete.cpp
iadk/iadk_module_adapter.cpp
iadk/system_agent.cpp
library/native_system_agent.c
library/native_system_service.c
)

Expand Down
1 change: 0 additions & 1 deletion src/include/sof/audio/module_adapter/library/native_system_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#define NATIVE_SYSTEM_SERVICE_H

#include <stdint.h>
#include <adsp_stddef.h>

#ifdef __cplusplus
extern "C" {
Expand Down
14 changes: 4 additions & 10 deletions src/library_manager/lib_manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,6 @@ const struct sof_man_module *lib_manager_get_module_manifest(const uint32_t modu
SOF_MAN_MODULE_OFFSET(entry_index));
}

#if CONFIG_INTEL_MODULES
/*
* \brief Load module code, allocate its instance and create a module adapter component.
* \param[in] drv - component driver pointer.
Expand Down Expand Up @@ -584,7 +583,9 @@ static void lib_manager_prepare_module_adapter(struct comp_driver *drv, const st
drv->ops.dai_ts_start = module_adapter_ts_start_op;
drv->ops.dai_ts_stop = module_adapter_ts_stop_op;
drv->ops.dai_ts_get = module_adapter_ts_get_op;
#if CONFIG_INTEL_MODULES
drv->adapter_ops = &processing_module_adapter_interface;
Copy link
Collaborator

@softwarecki softwarecki Mar 31, 2025

Choose a reason for hiding this comment

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

You can't just not assign a adapter_ops pointer because it will end in a firmware crash when trying to load the iadk library.

Copy link
Collaborator Author

@lyakh lyakh Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@softwarecki IADK libraries cannot be loaded with firmware built with CONFIG_INTEL_MODULES=n, so no crash expected

Copy link
Collaborator

@softwarecki softwarecki Apr 2, 2025

Choose a reason for hiding this comment

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

@lyakh Attempting to load a iadk module in firmware built with CONFIG_INTEL_MODULES=n results in a crash in function lib_manager_module_create on call module_adapter_new.

[    0.000173] <err> os: print_fatal_exception:  ** CPU 0 EXCCAUSE 13 (load/store PIF data error)
[    0.000173] <err> os: print_fatal_exception:  **  PC 0xa00632c7 VADDR 0xa00303c4
[    0.000173] <err> os: print_fatal_exception:  **  PS 0x60d20
[    0.000173] <err> os: print_fatal_exception:  **    (INTLEVEL:0 EXCM: 0 UM:1 RING:0 WOE:1 OWB:13 CALLINC:2)
[    0.000173] <err> os: xtensa_dump_stack:  **  A0 0xa104a5b0  SP 0xa00a4660  A2 0xa00f51c4  A3 0xa00a46a4
[    0.000173] <err> os: xtensa_dump_stack:  **  A4 (nil)  A5 0xa00f51c0  A6 0x400f5100  A7 0xa00a4660
[    0.000173] <err> os: xtensa_dump_stack:  **  A8 0xa00631ec  A9 (nil) A10 (nil) A11 0xa00f51c4
[    0.000173] <err> os: xtensa_dump_stack:  ** A12 0x60 A13 0x400f53a0 A14 0xa0026098 A15 0x1
[    0.000173] <err> os: xtensa_dump_stack:  ** LBEG 0xa006daaa LEND 0xa006dae0 LCOUNT (nil)
[    0.000173] <err> os: xtensa_dump_stack:  ** SAR 0x12
[    0.000173] <err> os: xtensa_dump_stack:  **  THREADPTR 0x1

Copy link
Collaborator Author

@lyakh lyakh Apr 3, 2025

Choose a reason for hiding this comment

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

@softwarecki aha, this is interesting! We'd expect an error response instead of a crash. Let's try to fix this, thanks!

Copy link
Collaborator Author

@lyakh lyakh Apr 3, 2025

Choose a reason for hiding this comment

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

@lyakh Attempting to load a iadk module in firmware built with CONFIG_INTEL_MODULES=n results in a crash in function lib_manager_module_create on call module_adapter_new.

@softwarecki should be fixed in the updated version

#endif
}

int lib_manager_register_module(const uint32_t component_id)
Expand Down Expand Up @@ -649,7 +650,8 @@ int lib_manager_register_module(const uint32_t component_id)
build_info->format);

/* Check if module is IADK */
if (build_info->format == IADK_MODULE_API_BUILD_INFO_FORMAT &&
if (IS_ENABLED(CONFIG_INTEL_MODULES) &&
build_info->format == IADK_MODULE_API_BUILD_INFO_FORMAT &&
build_info->api_version_number.full == IADK_MODULE_API_CURRENT_VERSION) {
/* Use module_adapter functions */
drv->ops.create = module_adapter_new;
Expand Down Expand Up @@ -679,14 +681,6 @@ int lib_manager_register_module(const uint32_t component_id)
return ret;
}

#else /* CONFIG_INTEL_MODULES */
int lib_manager_register_module(const uint32_t component_id)
{
tr_err(&lib_manager_tr, "Dynamic module loading is not supported");
return -ENOTSUP;
}
#endif /* CONFIG_INTEL_MODULES */

static int lib_manager_dma_buffer_alloc(struct lib_manager_dma_ext *dma_ext,
uint32_t size)
{
Expand Down
Loading