Repository navigation
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open issues
This relies on the sender enforcingget_attr_payload->type == COMP_ATTR_BASE_CONFIG, but the… · New UsingK_FOREVERcan permanently stall the caller if the remote core wedges, the work item never… · New Switchingcomp_prepare_remote()toIDC_NON_BLOCKINGmeans the caller no longer observes the… · Newipc4_coherent_copy32()is currently introduced incomponent_ext.h(audio/component layer), but… · Newipc4_coherent_copy32()silently truncates ifbytesis not a multiple of 4 (words = bytes / 4)… · New
What changed in this PR
Fixes cross-core IDC and remote component lifecycle issues on secondary cores by addressing alignment faults, uncached/coherent memcpy faults, and blocking behavior that can stall or time out during heavy DP cold-start/reset.
Changes:
- Aligns
struct idc_payloadtoDCACHE_LINE_SIZEto prevent Xtensa alignment faults when reading payload fields as wider types. - Introduces
IDC_BLOCKING_NO_TIMEOUTand implements aK_FOREVERwait path for reset completion. - Reworks remote component prepare to be non-blocking and adds 32-bit word copy helper for coherent (uncached) attribute payload transfers.
| File | Description |
|---|---|
| zephyr/include/rtos/idc.h | Adds IDC_BLOCKING_NO_TIMEOUT and aligns IDC payload to cache line to avoid alignment faults. |
| src/include/sof/audio/component_ext.h | Makes remote prepare non-blocking; adds coherent 32-bit copy helper; uses it for IPC4 get_attribute. |
| src/idc/zephyr_idc.c | Implements IDC_BLOCKING_NO_TIMEOUT behavior using k_p4wq_wait(..., K_FOREVER). |
| src/idc/idc.c | Updates remote get_attribute handler to use local cacheable struct + 32-bit stores into coherent buffer. |
| posix/include/rtos/idc.h | Defines IDC_BLOCKING_NO_TIMEOUT for parity with Zephyr header. |
Review effort: Lite
| /* | ||
| * Let the module fill a local (cacheable) copy, then publish it to the | ||
| * caller's coherent buffer with aligned 32-bit stores. Passing the | ||
| * coherent buffer straight to the module makes it memcpy() a whole | ||
| * struct into the uncached alias, which faults (EXCCAUSE 9) on Xtensa. | ||
| * Only COMP_ATTR_BASE_CONFIG is supported remotely (enforced by the | ||
| * sender), so local_cfg is always the right size. | ||
| */ | ||
| ret = comp_get_attribute(ipc_dev->cd, get_attr_payload->type, &local_cfg); | ||
| if (ret < 0) | ||
| return ret; | ||
|
|
||
| ipc4_coherent_copy32(get_attr_payload->value, &local_cfg, sizeof(local_cfg)); | ||
|
|
||
| return ret; |
| case IDC_BLOCKING_NO_TIMEOUT: | ||
| /* wait for completion; the handler is bounded but may exceed | ||
| * CONFIG_IDC_TIMEOUT_US (see IDC_BLOCKING_NO_TIMEOUT) */ | ||
| ret = k_p4wq_wait(work, K_FOREVER); | ||
| if (!ret) | ||
| ret = idc_msg_status_get(msg->core); | ||
| break; |
| * Non-blocking: a remote component's prepare may be expensive (e.g. a DP | ||
| * decoder's one-time codec open building large trig tables -- measured | ||
| * ~580 ms for mp3 on a secondary core). Waiting for it here stalls the | ||
| * initiator core's IPC/EDF thread for that whole time, and with the | ||
| * default CONFIG_IDC_TIMEOUT_US the k_p4wq_wait() would instead time out | ||
| * mid-prepare (-EAGAIN) and race the pipeline teardown into a crash. | ||
| * | ||
| * Ordering is preserved without the wait: each core drains its IDC work | ||
| * through a single in-order p4wq worker, so this prepare completes before | ||
| * the subsequent cross-core trigger (START) that is queued behind it. For | ||
| * a compress stream the START IPC also arrives only after the app's first | ||
| * write, giving the remote prepare time to finish off the critical path. | ||
| */ | ||
| return idc_send_msg(&msg, IDC_NON_BLOCKING); |
| if (ret < 0) | ||
| return ret; | ||
|
|
||
| ipc4_coherent_copy32(get_attr_payload->value, &local_cfg, sizeof(local_cfg)); |
| static inline void ipc4_coherent_copy32(void *dst, const void *src, size_t bytes) | ||
| { | ||
| volatile uint32_t *d = dst; | ||
| const volatile uint32_t *s = src; | ||
| size_t words = bytes / sizeof(uint32_t); | ||
| size_t i; | ||
|
|
||
| for (i = 0; i < words; i++) | ||
| d[i] = s[i]; | ||
| } |
lgirdwood
left a comment
There was a problem hiding this comment.
Revisit IDC update afer single core is upstream. Needs to be generic.
| size_t words = bytes / sizeof(uint32_t); | ||
| size_t i; | ||
|
|
||
| for (i = 0; i < words; i++) |
There was a problem hiding this comment.
we can use a vectorized version here, there is a global call that can be reused.
|
|
||
| return comp_get_attribute(ipc_dev->cd, get_attr_payload->type, get_attr_payload->value); | ||
| /* | ||
| * Let the module fill a local (cacheable) copy, then publish it to the |
There was a problem hiding this comment.
I suspect the copy is fixing a race we have somewhere else - revisit after ffmpeg only parts are merged.
| IDC_MSG_PREPARE_EXT(dev->ipc_config.id), dev->ipc_config.core, }; | ||
|
|
||
| return idc_send_msg(&msg, IDC_BLOCKING); | ||
| /* |
There was a problem hiding this comment.
revisit after ffmpeg merge
PR 11334: test resultsRun date: 2026-10-22 13:32 UTC Tested commit: 035cf0927f428486419923dc223e9493d0812cc0 |
struct idc_payload lives in coherent (SHARED_DATA) memory and is shared across cores, but it is only a byte array with no explicit alignment, so the linker was free to place it (and every per-core slot) on an odd address. Handlers that read the payload as a wider type -- e.g. the cross-core get_attribute path, which reads a uint32 type field -- then take an EXCCAUSE 9 (load/store alignment) fault when a component runs on a secondary core (ptl/ace30). Align the payload to the cache line. That is the granularity the payload is already built around (IDC_MAX_PAYLOAD_SIZE == DCACHE_LINE_SIZE * 2) and it guarantees every per-core slot starts word-aligned. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The remote (cross-core) get_attribute handler passed the caller coherent (uncached) IDC buffer straight to comp_get_attribute(), which memcpy()s a whole ipc4_base_module_cfg into it. On Xtensa the compiler lowers that struct copy to wide accesses (AE_L32X2 / 64-bit) that the uncached window does not support, faulting EXCCAUSE 9 -- so binding a component that lives on a secondary core (e.g. ffmpeg_dec offloaded to core 1 on ptl/ace30) crashed. Have the handler fill a local cacheable copy and publish it to the coherent buffer with aligned 32-bit word stores (ipc4_coherent_copy32); do the same for the read-back on the initiator side. Only COMP_ATTR_BASE_CONFIG is supported remotely, so the fixed-size local copy is always correct. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
A remote component prepare() can be expensive: a DP decoder one-time codec open builds large trig tables -- measured ~580 ms for mp3 on a secondary core. Waiting for it stalls the initiator core IPC/EDF thread for that whole time, and with the default CONFIG_IDC_TIMEOUT_US the blocking k_p4wq_wait() instead times out mid-prepare (-EAGAIN) and races pipeline teardown into a crash. Send the prepare IDC non-blocking. Ordering is preserved without the wait: each core drains its IDC work through a single in-order p4wq worker, so the prepare completes before the cross-core trigger (START) queued behind it. For a compress stream the START IPC also arrives only after the app first write, giving the remote prepare time to finish off the critical path. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
comp_reset_remote() sent the cross-core IDC_MSG_RESET with IDC_BLOCKING, which only waits CONFIG_IDC_TIMEOUT_US (15 ms) before returning -EAGAIN. A data-processing (DP) module reset runs schedule_task_cancel() -> k_thread_join() on the target core and cannot return until the DP thread finishes its in-flight process() call. That call is bounded but can be long during a codec cold-start (a deferred avcodec_open2 table build, up to ~1.4 s for aac, or the first decode after it), far exceeding the IDC timeout. When it timed out the component was left un-reset (not COMP_STATE_READY), so pipeline_reset() failed and the host-side free was then refused with an invalid-state error and the remote core could not be disabled - every teardown wedged until reboot. The caller (pipeline_reset -> host) depends on the reset actually being finished before it frees the component and powers down the remote core, so the reset must stay synchronous. Add IDC_BLOCKING_NO_TIMEOUT, which is IDC_BLOCKING but waits K_FOREVER for the handler to complete, and use it for comp_reset_remote(). A normal end-of-stream teardown joins an idle DP thread and returns immediately, so this only waits when it must. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
035cf09 to
7411f40
Compare


This series fixes four cross-core IDC and remote component lifecycle issues encountered when offloading data-processing (DP) modules with heavy cold-start initialization to secondary cores (e.g., on PTL / ACE 3.0):
zephyr: idc: align the IDC payload to the cache line:struct idc_payloadlives in coherent (SHARED_DATA) memory shared across cores, but previously had no alignment attribute, allowing the linker to place per-core slots on unaligned addresses and triggerEXCCAUSE 9(LoadStoreAlignmentCause) when handlers read wider fields (uint32_t). Alignsstruct idc_payloadtoDCACHE_LINE_SIZE.audio: component: copy coherent get_attribute payload with 32-bit stores:get_attributehandler previously passed the uncached coherent IDC payload directly tocomp_get_attribute(), whichmemcpy()sstruct ipc4_base_module_cfg. On Xtensa, the compiler lowers that struct copy to wide 64-bit loads/stores (AE_L32X2) unsupported by the uncached window, faultingEXCCAUSE 9. Copies via a local cacheable structure and 32-bit word stores (ipc4_coherent_copy32).audio: component: make comp_prepare_remote non-blocking:prepare()on a DP module can exceedCONFIG_IDC_TIMEOUT_USduring one-time table generation, stalling the initiator core IPC/EDF thread or timing out (-EAGAIN). SendsprepareIDC non-blocking; in-orderp4wqexecution on the target core preserves ordering before subsequenttrigger (START)messages.audio: component: wait for remote component reset to complete:comp_reset_remote()previously usedIDC_BLOCKING(CONFIG_IDC_TIMEOUT_US= 15 ms), which can time out if the target core's DP thread is finishing an in-flightprocess()call duringschedule_task_cancel() -> k_thread_join(), leaving the component un-reset and wedging teardown. IntroducesIDC_BLOCKING_NO_TIMEOUT(K_FOREVER) forcomp_reset_remote().