-
Notifications
You must be signed in to change notification settings - Fork 349
LLEXT: don't reload upon resume #10028
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
Conversation
|
@lyakh, how does the kernel knows that the booted firmware saves the library context thus no need to re-load the libraries? Can we send flag(s) via FW_READY for example? Or the context restore happens after the firmware has sent the FW_READY? |
kv2019i
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blockers, some questions in line.
| return IPC4_BUSY; | ||
| } | ||
|
|
||
| ret = llext_manager_store_to_dram(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this play with full CONTEXT_SAVE feature? If full SRAM is saved, then this is redundant, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, need to add that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any update here ?
| @@ -0,0 +1,308 @@ | |||
| // SPDX-License-Identifier: BSD-3-Clause | |||
| // | |||
| // Copyright(c) 2025 Intel Corporation. All rights reserved. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We omit the "all rights reserved" nowadays
@ujfalusi if this comment is correct 5e6ae48#diff-4287fd67d00ed77ae6296c02fa3a29e7f113fe39a7c488846b9e38cdb46447ffR194 then yes, in the present form restoring module context takes place before sending "ready" to the host, so it should be possible to adjust that message if needed |
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff, in general could do with more inline comments.
| lib_manager_dram.ext_lib = *_ext_lib; | ||
| for (i = 0, n_lib = 0, n_mod = 0, n_llext = 0, n_sect = 0, n_sym = 0; | ||
| i < ARRAY_SIZE(_ext_lib->desc); i++) | ||
| if (_ext_lib->desc[i]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!) continue ? reduces the indentation.
| n_lib++; | ||
| n_mod += _ext_lib->desc[i]->n_mod; | ||
| for (k = 0; k < _ext_lib->desc[i]->n_mod; k++) | ||
| if (_ext_lib->desc[i]->mod[k].ebl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto if(!) continue
|
|
||
| /* Also flatten dependency lists */ | ||
| int ret = llext_relink_dependency(lib_manager_dram.llext, n_llext); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lgirdwood unfortunately yes, otherwise something (checkpatch?) will complain, that it wants to have a blank line between declarations and code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ignore it now - it can be removed, this stuff is caught in review now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ignore it now - it can be removed, this stuff is caught in review now.
@lgirdwood sorry, which review? Some tool is now complaining about these blank lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no I mean us human reviewers. I pattern match the if() {block} together without the new line. new line here makes it look like another unrelated block to the if condition.
|
thesofproject/linux#5430 is merged, this can now be tested, now waiting for zephyrproject-rtos/zephyr#90400 |
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, some opens but lets get the Zephyr part merged 1st.
| return IPC4_BUSY; | ||
| } | ||
|
|
||
| ret = llext_manager_store_to_dram(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any update here ?
|
zephyrproject-rtos/zephyr#90400 got merged, now this becomes mergeable too |
The authentication context, used by the library manager to check module signature, is fully reinitialised and released for each loaded module, no need to store it permanently. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
L3 heap allocates memory in DRAM. Usually this is done to preserve contents over DSP reset. This patch adds a method to do that. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lib_manager.h uses some types without including respective headers or declaring them. Fix two such omissions. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
If module registration fails, lib_manager_load_library() will jump to "cleanup" where it will attempt to check whether a module is an LLEXT but in such cases the "mod" pointer can be NULL, which will cause an exception. Check the error code before checking the module type. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
llext_get_section_info() shouldn't fail when called with a valid section number, but it's still cleaner to check its success and skip the section in case of an error. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
When failing llext_manager_link_single() should return a negative error code, not 0. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Updpate Zephyr to include 4de0c9abc0e SoC: Intel: ADSP: ACE30: add .imrdata to MMU definitions 5f4177b47c9 llext: add context save and restore d8195c05c78 llext: rename _llext_list to llext_list Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
b17e512 to
46afc5a
Compare
|
for the record. Having enabled partial restore shortens resume time as follows: before enabling i.e. 164ms, after: i.e. 76ms |
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One open.
| CONFIG_SYS_CLOCK_TICKS_PER_SEC=12000 | ||
|
|
||
| # Zephyr / power settings | ||
| CONFIG_ADSP_IMR_CONTEXT_SAVE=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a Kconfig to select partial context save ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a Kconfig to select partial context save ?
@lgirdwood currently we have the following configuration possibilities:
- no LLEXT - obviously LLEXT selective context saving isn't used
- LLEXT and full context saving - LLEXT selective context saving isn't used
- otherwise - if LLEXT is enabled and full context saving isn't enabled, then LLEXT selective context saving is used.
So you'd like to also be able to select
4. LLEXT but no context saving - LLEXT reloading when booting
I didn't add a Kconfig option for this because why would anyone want to disable this? It almost looks like a fix instead of a new feature - we have LLEXT modules in DRAM, reloading them instead of reusing sounds more like a bug than a viable option to me. But if needed, sure, we can make this configurable. Also, there's a Zephyr option that this depends on: CONFIG_LLEXT_EXPERIMENTAL - without it this will fail, but it might to fail completely, haven't checked that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lgirdwood with the new version one can disable CONFIG_LLEXT_EXPERIMENTAL and return to the old behaviour - reloading libraries on each resume
Currently when resuming from D3 we lose the complete LLEXT context, which forces SOF to reload all the libraries, which costs time. This isn't necessary, because the libraries are stored in DRAM, which preserves its contents across DSP reset. Instead of reloading save LLEXT context before power down and reload it when resuming. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
| CONFIG_HEAP_MEM_POOL_SIZE=8192 | ||
| CONFIG_LLEXT=y | ||
| CONFIG_LLEXT_STORAGE_WRITABLE=y | ||
| CONFIG_LLEXT_EXPERIMENTAL=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why call it experimental? Perhaps LLEXT_SAVE_RESTORE makes better sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ranj063 we don't, Zephyr does. This option has recently been added to Zephyr to support our partial LLEXT restore, but the way how we added it there isn't perfectly clean. It's hardly usable by other LLEXT users, it's almost SOF-specific, therefore it's been decided to mark it "experimental" until a proper universal solution replaces it.
|
@lyakh Have you checked the PM-with-audio fail in https://sof-ci.01.org/sofpr/PR10028/build13381/devicetest/index.html ? Other tests (that we have available now) seem good. |
|
The check-suspend-resume-with-playback-5.sh fail is suspicious, let's rerun the test. This PR should not have any impact, but the Zephyr update could bring in some new problem. https://sof-ci.01.org/sofpr/PR10028/build13381/devicetest/index.html?model=PTLH_RVP_NOCODEC&testcase=check-suspend-resume-with-playback-5 |
|
SOFCI TEST |
|
check-suspend-resume-with-playback-5.sh fail not related to this PR. It has been seen with baseline (albeit with low repro rate) and the rerun of tests today with this PR didn't hit the failure. Proceeding with merge. |
Resuming after D3 currently reloads all LLEXT modules / libraries, which wastes time and can be avoided by re-using data stored in DRAM. This PR adds that functionality. It depends on zephyrproject-rtos/zephyr#90400 and also needs the Linux driver to avoid re-sending modules. @ujfalusi I used a simple diff for testing: