-
Notifications
You must be signed in to change notification settings - Fork 350
ipc4: ace: Add dynamic log level control via IPC with runtime filtering #10536
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
base: main
Are you sure you want to change the base?
ipc4: ace: Add dynamic log level control via IPC with runtime filtering #10536
Conversation
This commit will be replaced with actual zephyr update after necessary changes are merged. Signed-off-by: Tomasz Leman <[email protected]>
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.
Pull request overview
Adds IPC4-driven runtime log level control so the host can adjust firmware logging verbosity dynamically, and enables Zephyr runtime log filtering on Intel ADSP ACE boards with a quiet default.
Changes:
- Parse
logs_maskfromIPC4_ENABLE_LOGSand derive a Zephyr log level used when enabling the backend. - Enable
CONFIG_LOG_RUNTIME_FILTERINGand setCONFIG_LOG_RUNTIME_DEFAULT_LEVEL=1(ERROR) across multiple Intel ADSP ACE board configs. - Point the Zephyr west manifest to an upstream PR branch providing
CONFIG_LOG_RUNTIME_DEFAULT_LEVEL.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| west.yml | Switch Zephyr revision to the upstream PR providing CONFIG_LOG_RUNTIME_DEFAULT_LEVEL. |
| src/ipc/ipc4/logging.c | Compute log level from IPC4 logs_mask and pass it to log_backend_enable(). |
| app/debug_overlay.conf | Sets runtime filtering to n in the debug overlay config. |
| app/boards/intel_adsp_ace40_nvls.conf | Enables runtime filtering and sets default runtime level to ERROR. |
| app/boards/intel_adsp_ace40_nvl.conf | Enables runtime filtering and sets default runtime level to ERROR. |
| app/boards/intel_adsp_ace30_wcl.conf | Enables runtime filtering and sets default runtime level to ERROR. |
| app/boards/intel_adsp_ace30_ptl.conf | Enables runtime filtering and sets default runtime level to ERROR. |
| app/boards/intel_adsp_ace20_lnl.conf | Enables runtime filtering and sets default runtime level to ERROR. |
| app/boards/intel_adsp_ace15_mtpm.conf | Enables runtime filtering and sets default runtime level to ERROR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Enable backend if not already active */ | ||
| if (!log_backend_is_active(log_backend)) | ||
| log_backend_enable(log_backend, mtrace_log_hook, CONFIG_SOF_LOG_LEVEL); | ||
| log_backend_enable(log_backend, mtrace_log_hook, log_level); |
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.
Does it make sense to enable backend for LOG_LEVEL_NONE?
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.
First of all, this doesn't seem to be illegal (pointless, but not illegal).
This IPC properly implemented using what Zephyr logger offers will allow, for example, to silence all logs for a specified backend and then set them to a higher level only for a specified source. And in such a scenario, enabling with level set to NONE makes perfect sense.
|
@kv2019i what does Linux support for this IPC look like? Presumably it needs to be implemented? Is it fine to immediately switch the default log filtering state? |
| CONFIG_LOG_FLUSH_SLEEP_US=5000 | ||
| CONFIG_WINSTREAM_CONSOLE=n | ||
| CONFIG_LOG_RUNTIME_FILTERING=y | ||
| CONFIG_LOG_RUNTIME_DEFAULT_LEVEL=1 |
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.
Either I don't understand something or something isn't working. This means, that only ERR level logs should appear in mtrace, right? But if you check this PR logs it's still the usual INFO. If indeed this is trying to set log level to ERR, is this a good idea? We need INF level logs in CI
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.
At this moment, according to my knowledge, everything works as I intended. The default log filtering level is set to ERROR at startup. This doesn't change the log level that gets compiled in, it still remains INFO. The fact that CI assumes that logs will appear in the memory window without sending IPC which is supposed to enable them, I've known for some time (I once wanted to disable logger backend autostart). The FW used in CI is built with the app/debug_overlay.conf and there runtime filtering is disabled to maintain the existing behavior expected by CI.
Implement dynamic log level selection based on the logs_mask field from the IPC4_ENABLE_LOGS command, allowing the host to control firmware logging verbosity at runtime. Previously, logging was enabled with a fixed compile-time level (CONFIG_SOF_LOG_LEVEL), preventing dynamic control. This change parses the logs_mask[0] bits 0-4 to determine the requested log level and passes it to the backend. Mask bit mapping to Zephyr log levels: - bit 0 (critical/error) -> LOG_LEVEL_ERR (1) - bit 1 (high/warning) -> LOG_LEVEL_WRN (2) - bit 2 (medium) -> LOG_LEVEL_INF (3) - bit 3 (low/info) -> LOG_LEVEL_INF (3) - bit 4 (verbose/debug) -> LOG_LEVEL_DBG (4) The highest set bit determines the maximum log level. This allows the host to request ERROR-only logging for quiet operation, or DEBUG for verbose troubleshooting, without firmware recompilation. Additionally, log_backend_enable() is now always called (it is idempotent and safe for reconfiguration), removing the previous check that prevented level changes on already-active backends. A TODO comment documents that the mask handling should be improved with proper macros or structs in the future for better code maintainability, but the current implementation is sufficient. Signed-off-by: Tomasz Leman <[email protected]>
21ebebb to
e55ea21
Compare
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.
Good new feature, but we need to figure out how to introduce without breaking existing systems. Some comments inline.
| else if (mask & BIT(1)) | ||
| log_level = LOG_LEVEL_WRN; | ||
| else if (mask & BIT(0)) | ||
| log_level = LOG_LEVEL_ERR; |
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 need to think about how to introduce this. Current shipping Linux kernels do not set log_mask and we cannot retroactively change shipped kernels (update of FW must not break user systems with older kernel), so this would effectively break mtrace for all existing systems as log level is forced to LOG_LEVEL_NONE. I think this needs to be a a build option (so we can keep the old behaviour in Linux overlay) and/or the default needs to be LOG_LEVEL_INF (like it is now). Looping in our kernel folks @bardliao @ujfalusi @ranj063
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 does FW that doesn't unnecessarily spam with logs mean breaking the user's system?
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.
@tmleman I wasn't claiming that. Just saying this effectively breaks mtrace for existing systems. By default logs are disabled (no enable sent by host), but we have documented flow to set a kernel module parameter. If set, kernel will enable mtrace (no log mask) and mtrace is expected to work (also in release firmware files). As I understand this change, after this PR, there's no way to get mtrace working with current Linux kernel and to get FW logs out, one needs a new kernel that can set the log_mask to some non-zero value. Or did I miss something?
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.
Its probably best to have a Kconfig=level for Linux based OS to boot with current level and then have kernel tune after FW boot if required. This can be a debugFS file @ujfalusi
Other OSes/targets can set the level as needed in their Kconfigs.
Enable CONFIG_LOG_RUNTIME_FILTERING and set CONFIG_LOG_RUNTIME_DEFAULT_LEVEL to 1 (ERROR) for all Intel ADSP ACE platforms. This provides quiet firmware boot with minimal logging overhead while maintaining full DEBUG compile-time capability for dynamic log level control via IPC. Platforms configured: - intel_adsp_ace15_mtpm - intel_adsp_ace20_lnl - intel_adsp_ace30_ptl - intel_adsp_ace30_wcl - intel_adsp_ace40_nvl - intel_adsp_ace40_nvls With these settings: - Firmware boots showing only ERROR level logs (quiet startup) - Host can dynamically increase verbosity to INFO or DEBUG via IPC4_ENABLE_LOGS without recompilation - All log macros (LOG_ERR, LOG_INF, etc.) remain compiled in Runtime filtering is explicitly disabled in debug_overlay.conf to maintain current debug build behavior where logs are not filtered at runtime. This works in conjunction with the IPC4 logging handler changes that parse logs_mask to set the appropriate runtime filter level. Signed-off-by: Tomasz Leman <[email protected]>
Export log_dynamic_drc symbol when CONFIG_LOG_RUNTIME_FILTERING is enabled to fix LLEXT module linking failure. When runtime filtering is enabled, LOG_MODULE_REGISTER() creates both log_const_<module> and log_dynamic_<module> structures. LLEXT modules need to link against these symbols at load time. Previously, only log_const_drc was exported, causing the LLEXT linker to fail with: <wrn> llext: llext_link_plt: PLT: cannot find idx 46 name log_dynamic_drc <err> llext: do_llext_load: Failed to link, ret -2 <err> lib_manager: llext_manager_link_single: linking failed: -2 This prevented the DRC module from loading when built as LLEXT with runtime log filtering enabled. The fix conditionally exports log_dynamic_drc when runtime filtering is configured, allowing the LLEXT module to resolve all required logging symbols. Signed-off-by: Tomasz Leman <[email protected]>
e55ea21 to
4ac3119
Compare
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.
I assume its not possible to block critical error messages ? i.e. on oops we would still get the stack trace, register dump alongside any error ?
| else if (mask & BIT(1)) | ||
| log_level = LOG_LEVEL_WRN; | ||
| else if (mask & BIT(0)) | ||
| log_level = LOG_LEVEL_ERR; |
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.
Its probably best to have a Kconfig=level for Linux based OS to boot with current level and then have kernel tune after FW boot if required. This can be a debugFS file @ujfalusi
Other OSes/targets can set the level as needed in their Kconfigs.
@lgirdwood I checked this. If the log level is set to NONE then no ERROR will appear in the logs. There will also be no logs if the backend is inactive (which this IPC can do). If the host passes such settings in IPC then it seems this is correct behavior. |
|
|
||
| # Logging options | ||
| # Disable runtime filtering to maintain expected behavior in SOF CI. | ||
| CONFIG_LOG_RUNTIME_FILTERING=n |
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.
@lyakh This explains why we have logs in CI results.
This does mean that for builds that don't use overlay, it's impossible to enable mtrace logs unless you rebuild the kernel, or this feature is added to Linux driver.
I mean, this can be ok, this is a developer/debug feature after all, but personally I'd like to maintain current behaviour for Linux SOF builds. We can change the default for some future platform (PTL+x ) where we can ensure anyone running Linux on platform PTL+x, will have recent enough Linux kernel version (or otherwise PTL+x would not work). This is our normal procedure to implement FW changes that require host side change. But alas, this is a debug feature.
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.
It seems reasonable to choose some future platform to change the default behavior, is NVL still fine for such change?
ok, so we need to be aware of this the, its probably worth mentioning in the Kconfig or API comments so it doesn't catch anyone out. |
This PR adds support for dynamic firmware log level control through the IPC4_ENABLE_LOGS command, allowing the host to adjust logging verbosity at runtime without firmware recompilation.
Dependencies:
This PR depends on Zephyr upstream PR that adds
CONFIG_LOG_RUNTIME_DEFAULT_LEVEL:Changes Overview:
IPC4 logging handler enhancement - Parse logs_mask field from IPC4_ENABLE_LOGS to determine requested log level and pass it to the backend, enabling dynamic control (ERROR/WARNING/INFO/DEBUG) based on host request
Platform configuration updates - Enable runtime filtering with ERROR default level for all Intel ADSP ACE platforms (MTL/LNL/PTL/WCL/NVL/NVLS), providing quiet boot while maintaining full debug capability