-
Notifications
You must be signed in to change notification settings - Fork 350
Part 2 of compressed offload support for IPC4 (no topology changes) #10534
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?
Conversation
The zephyr_library_import() is needed to compile the vorbis decoder support Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
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 compressed offload support plumbing for IPC4 by enabling Cadence codec modules, exposing supported codec info via fw_config, and improving EOS handling/notifications for compressed streams.
Changes:
- Enable Cadence module inclusion in multiple rimage TOML configs and add a ready-to-use
compr_overlay.conf. - Add IPC4 fw_config SOF-specific tuple(s) to report supported codec capabilities to the host.
- Implement EOS handling and a module notification path for compressed streams; reduce noisy “no bytes to copy” logging in FAST_MODE.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/rimage/config/wcl.toml.h | Conditionally includes Cadence module TOML for this platform config. |
| tools/rimage/config/ptl.toml.h | Conditionally includes Cadence module TOML for this platform config. |
| tools/rimage/config/nvl.toml.h | Conditionally includes Cadence module TOML for this platform config. |
| tools/rimage/config/mtl.toml.h | Conditionally includes Cadence module TOML for this platform config. |
| tools/rimage/config/lnl.toml.h | Conditionally includes Cadence module TOML for this platform config. |
| src/include/sof/audio/module_adapter/module/generic.h | Adds EOS state fields to module processing data. |
| src/include/ipc4/header.h | Adds a new magic value for compressed module event notifications. |
| src/include/ipc4/base_fw.h | Introduces IPC4_FW_SOF_INFO and SOF sub-parameter IDs. |
| src/audio/module_adapter/module/cadence_ipc4.c | Adds Vorbis config case + EOS notification emission and sink EOS propagation. |
| src/audio/module_adapter/module/cadence.c | Initializes and updates EOS state based on decoder output and expected EOS. |
| src/audio/module_adapter/CMakeLists.txt | Imports Vorbis decoder library when enabled. |
| src/audio/host-zephyr.c | Suppresses “no bytes to copy” logs in FAST_MODE and improves warning formatting. |
| src/audio/base_fw.c | Builds and publishes codec capability TLVs under fw_config. |
| app/compr_overlay.conf | Adds an overlay to enable Cadence codecs and point to libraries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case CADENCE_CODEC_AAC_DEC_ID: | ||
| return cadence_configure_aac_dec_params(mod); | ||
| case CADENCE_CODEC_VORBIS_DEC_ID: | ||
| /* No configuration needed fro Vorbis */ |
Copilot
AI
Feb 10, 2026
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.
Correct typo in comment: 'fro' → 'for'.
| /* No configuration needed fro Vorbis */ | |
| /* No configuration needed for Vorbis */ |
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.
Typo here.
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.
Fixing..
| CONFIG_CADENCE_CODEC=y | ||
| CONFIG_CADENCE_CODEC_MP3_DEC=y | ||
| CONFIG_CADENCE_CODEC_MP3_DEC_LIB="../cadence_libs/xa_mp3_dec.a" | ||
| CONFIG_CADENCE_CODEC_MP3_ENC=y | ||
| CONFIG_CADENCE_CODEC_MP3_ENC_LIB="../cadence_libs/xa_mp3_enc.a" | ||
| CONFIG_CADENCE_CODEC_AAC_DEC=y | ||
| CONFIG_CADENCE_CODEC_AAC_DEC_LIB="../cadence_libs/xa_aac_dec.a" | ||
| CONFIG_CADENCE_CODEC_VORBIS_DEC=y | ||
| CONFIG_CADENCE_CODEC_VORBIS_DEC_LIB="../cadence_libs/xa_vorbis_dec.a" |
Copilot
AI
Feb 10, 2026
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.
This overlay relies on relative library paths (e.g., ../cadence_libs/...) but doesn’t document the expected directory layout or where the paths are resolved from during builds. Add brief comments at the top of the file explaining the expected location of cadence_libs/ relative to the build/app directory, and how users should adjust these paths for their environment.
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.
OK, I'll add a comment to the compr_overlay.conf file as well.
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.
Only minor things, looks good and easy to review code.
| case CADENCE_CODEC_AAC_DEC_ID: | ||
| return cadence_configure_aac_dec_params(mod); | ||
| case CADENCE_CODEC_VORBIS_DEC_ID: | ||
| /* No configuration needed fro Vorbis */ |
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.
Typo here.
|
|
||
| struct sof_ipc4_codec_info_data { | ||
| uint32_t count; | ||
| uint32_t items[32]; |
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 32? Future-proofing or aligning to some existing spec/convention?
| msg_module_data->event_id = SOF_IPC4_NOTIFY_MODULE_EVENTID_COMPR_MAGIC_VAL; | ||
| msg_module_data->event_data_size = 0; | ||
|
|
||
| comp_err(dev, "[peter] Sending notification"); |
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 think this should have one peter less.
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.
...and shouldn't be an "error." Debug would be enough?
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.
[peter] usually is in warn level, I will drop this.
| uint32_t items[32]; | ||
| } __packed __aligned(4); | ||
|
|
||
| #define SET_CODEC_INFO_ITEM(codec, dir) (((codec) & 0xff) | (((dir) & 0xff) << 16)) |
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.
internally 16 bits would be enough, I presume 32 bits are used for compatibility with external standards?
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.
The codec ID (lower 16bit) is actually defined in compress_params.h to be stored as __u32, max is 0x11 atm, so 8bit should be fine with bit7 as direction, but if I recall the fw_config itself have 32bit alignment requirement, all other flags are also stored as u32, so we follow this through
Is this acceptable answer @lyakh, @kv2019i ?
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 reason we are trying to save here, why not just go for 31bits ID and 1 bit direction to start with ?
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.
Yes, that would make more sense I guess, need ot align the kernel PR as well.
| msg_module_data->event_id = SOF_IPC4_NOTIFY_MODULE_EVENTID_COMPR_MAGIC_VAL; | ||
| msg_module_data->event_data_size = 0; | ||
|
|
||
| comp_err(dev, "[peter] Sending notification"); |
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.
...and shouldn't be an "error." Debug would be enough?
The decoder for Vorbis does not need any configuration. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…yload the sof_info (param id 35) contains SOF specific information in a tuple based array. The first such info block is the codec_info (sub ID: 0) to hold information about the codecs supported fro compress offload. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
In FAST_MODE the host DMA is not paced with the period size and can copy more data once to do no copying for a other periods. In other words: in FAST_MODE the host is not running in stream mode, but in batch mode. Printing warnings in this does not make sense and just fills up the firmware log. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…ruct To facilitate the End Of Stream handling in processing modules add two flags: eos_reached: when set, the processing module reached EOS by processing all data. EOS can happen when the pipeline have expect_eos flag set, indicating that EOS is going to happen eos_notification_sent: if the module uses notification to host, this flag can be used make sure to not flood with IPCs Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
0xC0C0 magic value is to be used by compr module as module notification of drain completion. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…host When the pipeline has the expect_eos flag set and the cadence module produces 0 bytes during process is an indication that the EOS has been completed and the module drained all data from input to output. Send a notification about this to host and set EOS state on downstream module instances. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…enabled Include the cadence.toml if CONFIG_CADENCE_CODEC is enabled. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Co-developed-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
The needed libraries should be placed in sof/../cadence_libs/ directory if the overlay is used. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
22b3ab7 to
c28c64c
Compare
|
Changes since v1:
|
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. No new opens.
| uint32_t items[32]; | ||
| } __packed __aligned(4); | ||
|
|
||
| #define SET_CODEC_INFO_ITEM(codec, dir) (((codec) & 0xff) | (((dir) & 0xff) << 16)) |
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 reason we are trying to save here, why not just go for 31bits ID and 1 bit direction to start with ?
| #define SOF_IPC4_ENUM_CONTROL_PARAM_ID 201 | ||
| #define SOF_IPC4_BYTES_CONTROL_PARAM_ID 202 | ||
| #define SOF_IPC4_NOTIFY_MODULE_EVENTID_ALSA_MAGIC_VAL ((uint32_t)(0xA15A << 16)) | ||
| #define SOF_IPC4_NOTIFY_MODULE_EVENTID_COMPR_MAGIC_VAL ((uint32_t)(0xC0C0 << 16)) |
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.
What about this magic value?
@lgirdwood, @ranj063, @kv2019i
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.
fine by me.
| /* Minimum size of host buffer in ms */ | ||
| IPC4_FW_MIN_HOST_BUFFER_PERIODS = 33, | ||
| /* decoder/encoder codec information */ | ||
| IPC4_FW_SOF_INFO = 35, |
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 this ID is good than we need to reserve it...
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.
35 seems free. We need to reserve it.
softwarecki
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.
My two cents. Nice to see the EOS functionality starting to get actual use :)
| primary->r.type = SOF_IPC4_GLB_NOTIFICATION; | ||
| primary->r.rsp = SOF_IPC4_MESSAGE_DIR_MSG_REQUEST; | ||
| primary->r.msg_tgt = SOF_IPC4_MESSAGE_TARGET_FW_GEN_MSG; | ||
| msg = ipc_msg_w_ext_init(msg_proto.header, msg_proto.extension, |
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.
Looks like a good place to use ipc_notification_pool_get
| /* Minimum size of host buffer in ms */ | ||
| IPC4_FW_MIN_HOST_BUFFER_PERIODS = 33, | ||
| /* decoder/encoder codec information */ | ||
| IPC4_FW_SOF_INFO = 35, |
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.
35 seems free. We need to reserve it.
Finalizing the code part of the compr offload support for IPC4:
compr_overlay.confas an easy way to enable all currently supported codecs via cadence module and provide a standardized location for the libraries.