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.

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

Jump to bottom

LLEXT: build libraries #9702

Merged
merged 14 commits into from
Jan 13, 2025
Merged

LLEXT: build libraries #9702

merged 14 commits into from
Jan 13, 2025

Conversation

lyakh
Copy link
Collaborator

@lyakh lyakh commented Dec 2, 2024 •
edited
Loading

Extends #9688 with library build support. Needs zephyrproject-rtos/zephyr#82443

@lyakh lyakh marked this pull request as ready for review December 9, 2024 14:11
@lyakh lyakh added the DNM Do Not Merge tag label Dec 9, 2024
@lyakh
Copy link
Collaborator Author

lyakh commented Dec 9, 2024 •
edited
Loading

Can be reviewed but shouldn't be merged yet because of a zephyrproject-rtos/zephyr#82735 Zephyr dependency

lgirdwood
lgirdwood reviewed Dec 9, 2024
View reviewed changes
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.

LGTM - let get the Zephyr part merged first.

@lyakh
Copy link
Collaborator Author

lyakh commented Dec 9, 2024

one more change that has to be done before merging: actually only PTL should be converted to library style so far, convert it back to draft to really avoid accidental merge

@lyakh lyakh marked this pull request as draft December 9, 2024 16:20
@lyakh lyakh force-pushed the llext-lib branch 6 times, most recently from 41a829e to 5d5e5df Compare December 16, 2024 15:46
@lyakh lyakh force-pushed the llext-lib branch 2 times, most recently from 35eebb0 to 8afc3f3 Compare December 17, 2024 15:55
@lyakh lyakh marked this pull request as ready for review December 17, 2024 15:55
@lyakh lyakh removed the DNM Do Not Merge tag label Dec 17, 2024
@kv2019i kv2019i changed the title LLEXT: build libraries LLEXT: build libraries (include Zephyr west.yaml update) Dec 18, 2024
kv2019i
kv2019i reviewed Dec 18, 2024
View reviewed changes
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.

Code looks good, but I'm a bit uncertain what (if any) changes expect in the build with this. I did some test builds locally and I see no change... expected?

app/boards/intel_adsp_ace30_ptl.conf Outdated
@@ -36,6 +36,10 @@ CONFIG_ZEPHYR_NATIVE_DRIVERS=y
# SOF / loadable modules
CONFIG_INTEL_MODULES=y
CONFIG_LIBRARY_MANAGER=y
CONFIG_LIBRARY_BASE_ADDRESS=0xa0688000
CONFIG_LIBRARY_MANAGER=y
Copy link
Collaborator

Choose a reason for hiding this comment

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

CONFIG_LIBRARY_MANAGER^2 ? Also on L38

app/boards/intel_adsp_ace30_ptl.conf Outdated
CONFIG_LIBRARY_BASE_ADDRESS=0xa0688000
CONFIG_LIBRARY_MANAGER=y
CONFIG_LIBRARY_BUILD_LIB=y
CONFIG_LIBRARY_DEFAULT_MODULAR=y
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, is this series missing something still? I don't see ptl build changed with this PR, should it? I see ace30_ptl.conf doesn't have LLEXT set, so this CONFIG_LIBRARY_DEFAULT_MODULAR=y setting won't have any impact. Is this intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kv2019i no, it isn't intentional, thanks for catching! Let me fix

lyakh added 3 commits January 3, 2025 17:33
@lyakh
llext: add support for library building
9d90547 
Build libraries of modules, as specified in their cmake files.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
@lyakh
rimage: (cosmetic) no need to use "const" in sizeof()
62334e4 
Size of constant and writable objects of the same type is the same,
drop the "const" qualifier from sizeof() type specification.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
@lyakh
rimage: (cosmetic) fix misnamed variables
f368063 
Fix typos, using pin-configuration references when parsing module
configuration. Also clarify some magic constants.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
@lyakh
Copy link
Collaborator Author

lyakh commented Jan 6, 2025 •
edited
Loading

zephyrproject-rtos/zephyr#81360 seems to be needed for this, without it a lock up or a crash is observed when destroying pipeline threads. With that fix and with thesofproject/linux#5270 this PR enables building of "openmodules" libraries when enabled in Kconfig

lgirdwood
lgirdwood reviewed Jan 6, 2025
View reviewed changes
tools/rimage/src/manifest.c Show resolved Hide resolved
tools/rimage/src/manifest.c Outdated Show resolved Hide resolved
tools/rimage/src/manifest.c Show resolved Hide resolved
@lgirdwood
Copy link
Member

zephyrproject-rtos/zephyr#81360 seems to be needed for this, without it a lock up or a crash is observed when destroying pipeline threads. With that fix and with thesofproject/linux#5270 this PR enables building of "openmodules" libraries when enabled in Kconfig

Btw, I assume the log strings are part of the module (i.e. string memory is owned by module and we pass string pointer) but the formatted data is owned by the log infra, hence we should log_flush_and_sync() (or similar API) before we unload.

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 6, 2025

zephyrproject-rtos/zephyr#81360 seems to be needed for this, without it a lock up or a crash is observed when destroying pipeline threads. With that fix and with thesofproject/linux#5270 this PR enables building of "openmodules" libraries when enabled in Kconfig

Btw, I assume the log strings are part of the module (i.e. string memory is owned by module and we pass string pointer) but the formatted data is owned by the log infra, hence we should log_flush_and_sync() (or similar API) before we unload.

@lgirdwood this is already done by LLEXT in Zephyr core, PR81360 just changes the way how it is done

@lyakh lyakh mentioned this pull request Jan 7, 2025
Open
lyakh added 2 commits January 7, 2025 12:42
@lyakh
rimage: add a convenience function
476eb12 
Add a function to search for a TOML configuration entry by module
name.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
@lyakh
rimage: count modules and files
06f34ad 
When building libraries we know how many input files we use, but we
also need to count how many modules each file contains. Add
respective calculations.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
@lyakh lyakh force-pushed the llext-lib branch 2 times, most recently from 8955c21 to ad9e827 Compare January 8, 2025 07:16
@lyakh
Copy link
Collaborator Author

lyakh commented Jan 8, 2025

Dropping the GitHub out-of-storage space fix here, moving it to a separate #9766 , let's merge that one before this.

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 8, 2025

The CI seems to be misbehaving: https://sof-ci.01.org/sofpr/PR9702/build10065/devicetest/index.html, https://sof-ci.01.org/sofpr/PR9702/build10063/devicetest/index.html and https://sof-ci.01.org/sofpr/PR9702/build10064/devicetest/index.html seem to have appeared way too early.

@lgirdwood
Copy link
Member

@lyakh can you check CI , thanks!

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 10, 2025

@lyakh can you check CI , thanks!

LNL https://sof-ci.01.org/sofpr/PR9702/build10112/devicetest/index.html didn't run at all. Relaunching

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 10, 2025

SOFCI TEST

lyakh added 3 commits January 13, 2025 10:55
@lyakh
rimage: library: copy module configurations individually
f7d904e 
Files on rimage command line don't have to follow in the same order
as modules in TOML. Moreover, the TOML configuration is global, it
contains all modules, enabled for this platform, whereas when
building libraries we need to select only configurations for used
modules. This patch finds and copies each such configuration
individually.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
@lyakh
rimage: library: calculate hashes correctly
0d664c1 
Module hash sums are per-file, when building libraries we have to
walk files, not modules, and copy hashes to all modules in each file.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
@lyakh
ptl: temporarily revert to monolithic build
d172408 
Instead of dropping this in the commit that enabled this option, we
make a separate commit to make it simple to revert it once the kernel
driver and CI testing are updated to use installed module libraries.

Signed-off-by: Guennadi Liakhovetski <[email protected]>
@lyakh
Copy link
Collaborator Author

lyakh commented Jan 13, 2025

CI: jenkins tests https://sof-ci.01.org/sofpr/PR9702/build10148/devicetest/index.html and https://sof-ci.01.org/sofpr/PR9702/build10149/devicetest/index.html "failed" because one device out of 3 wasn't tested in each of them. The two tested devices in both sets are "green"

@lyakh
Copy link
Collaborator Author

lyakh commented Jan 13, 2025

as it stands, this PR won't break anything, because building libraries is disabled in this configuration. But to actually enable it it needs zephyrproject-rtos/zephyr#81360 and thesofproject/linux#5270

@lgirdwood lgirdwood merged commit dbdfe32 into thesofproject:main Jan 13, 2025
46 of 51 checks passed
@lyakh lyakh deleted the llext-lib branch January 14, 2025 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kv2019i kv2019i kv2019i approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@iuliana-prodan iuliana-prodan Awaiting requested review from iuliana-prodan iuliana-prodan is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@abonislawski abonislawski Awaiting requested review from abonislawski abonislawski is a code owner

@singalsu singalsu Awaiting requested review from singalsu singalsu is a code owner

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lyakh @kv2019i @lgirdwood