Part 2 of compressed offload support for IPC4 (no topology changes)#10534
Part 2 of compressed offload support for IPC4 (no topology changes)#10534lgirdwood merged 12 commits intothesofproject:mainfrom
Conversation
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
Correct typo in comment: 'fro' → 'for'.
| /* No configuration needed fro Vorbis */ | |
| /* No configuration needed for Vorbis */ |
| 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" |
There was a problem hiding this comment.
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.
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.
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 */ |
|
|
||
| struct sof_ipc4_codec_info_data { | ||
| uint32_t count; | ||
| uint32_t items[32]; |
There was a problem hiding this comment.
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.
I think this should have one peter less.
There was a problem hiding this comment.
...and shouldn't be an "error." Debug would be enough?
There was a problem hiding this comment.
[peter] usually is in warn level, I will drop this.
src/audio/base_fw.c
Outdated
| 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.
internally 16 bits would be enough, I presume 32 bits are used for compatibility with external standards?
There was a problem hiding this comment.
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.
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.
Yes, that would make more sense I guess, need ot align the kernel PR as well.
There was a problem hiding this comment.
compress direction have 3 values:
enum snd_compr_direction {
SND_COMPRESS_PLAYBACK = 0,
SND_COMPRESS_CAPTURE,
SND_COMPRESS_ACCEL
};I guess we could use bit 30-31, that would allow one more value for future proofing.
I'll go with something which is bit more compact:
BIT 0-7 (8 bits): codec_id
BIT 8-11 (4 bits): dir
and we leave BIT 12-31 for future use if needed.
| 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.
...and shouldn't be an "error." Debug would be enough?
22b3ab7 to
c28c64c
Compare
|
Changes since v1:
|
src/audio/base_fw.c
Outdated
| 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.
any reason we are trying to save here, why not just go for 31bits ID and 1 bit direction to start with ?
| /* 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.
If this ID is good than we need to reserve it...
There was a problem hiding this comment.
35 seems free. We need to reserve it.
There was a problem hiding this comment.
Yes, can we initiate the reservation process?
This will reduce the collision surface for future in both ways.
softwarecki
left a comment
There was a problem hiding this comment.
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.
Looks like a good place to use ipc_notification_pool_get
There was a problem hiding this comment.
Would a followup patch do?
All module messages are using this mode and it will be better to update them with a single PR.
We can most likely also add helper to src/ipc/ipc4/notification.c for these at the same round?
| /* 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.
35 seems free. We need to reserve it.
The zephyr_library_import() is needed to compile the vorbis decoder support Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
setup_cfg can be in theory undefined when no module_data is provided, initialize it to NULL and check for NULL pointer before accessing to it's member. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
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>
Convert TGL platform to modular toml configuration to align with more modern architectures. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Convert TGL-H platform to modular toml configuration to align with more modern architectures. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…module if 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>
c28c64c to
f3f018b
Compare
|
Changes since v2:
|
| { | ||
| char sof_config_data[SOF_CONFIG_SIZE_MAX] = { 0 }; | ||
| struct sof_tlv *sof_config_tuple = (struct sof_tlv *)sof_config_data; | ||
| uint32_t sof_config_size; |
| uint32_t sof_config_size; | ||
|
|
||
| get_codec_info(&sof_config_tuple); | ||
| sof_config_size = (uint32_t)((char *)sof_config_tuple - sof_config_data); |
There was a problem hiding this comment.
you could just type-cast sof_config_tuple to uintptr_t to avoid double-casting
|
|
||
| get_codec_info(&sof_config_tuple); | ||
| sof_config_size = (uint32_t)((char *)sof_config_tuple - sof_config_data); | ||
| if (sof_config_size == 0) |
|
Lets deal with any of the minor requests in the next update as this one is passing CI and there is an update fro FLAC coming. |
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.kernel PR: thesofproject/linux#5647