From d0dd6cc22fad2becd937bd4162131f39a3940651 Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Sun, 26 Jul 2026 20:37:46 +0100 Subject: [PATCH 1/4] zephyr: idc: align the IDC payload to the cache line 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 --- zephyr/include/rtos/idc.h | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/zephyr/include/rtos/idc.h b/zephyr/include/rtos/idc.h index 3e30250344e2..209cefcd6d9c 100644 --- a/zephyr/include/rtos/idc.h +++ b/zephyr/include/rtos/idc.h @@ -149,10 +149,20 @@ /** \brief IDC free function flags */ #define IDC_FREE_IRQ_ONLY BIT(0) /**< disable only irqs */ -/** \brief IDC message payload. */ +/** + * \brief IDC message payload. + * + * Placed in coherent (SHARED_DATA) memory and shared across cores. The struct + * is only a byte array, so without an explicit alignment the linker may drop + * it (and every per-core slot) on an odd address. Handlers that read the + * payload as wider types -- e.g. the cross-core get_attribute path reads a + * uint32 'type' field -- then take an EXCCAUSE 9 (load/store alignment) fault. + * Align to the cache line, which is the size this payload is already built + * around (IDC_MAX_PAYLOAD_SIZE == DCACHE_LINE_SIZE * 2). + */ struct idc_payload { uint8_t data[IDC_MAX_PAYLOAD_SIZE]; -}; +} __aligned(DCACHE_LINE_SIZE); /** \brief IDC message. */ struct idc_msg { From 44f3a854fd6b2ab85fea662f51fd683c3392f870 Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Sun, 26 Jul 2026 20:37:46 +0100 Subject: [PATCH 2/4] audio: component: copy coherent get_attribute payload with 32-bit stores 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 --- src/idc/idc.c | 19 ++++++++++++++++- src/include/sof/audio/component_ext.h | 30 +++++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/idc/idc.c b/src/idc/idc.c index 8fdecb8ba173..6bf9ab6b01fd 100644 --- a/src/idc/idc.c +++ b/src/idc/idc.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -121,6 +122,8 @@ static int idc_get_attribute(uint32_t comp_id) struct ipc_comp_dev *ipc_dev; struct idc_payload *idc_payload; struct get_attribute_remote_payload *get_attr_payload; + struct ipc4_base_module_cfg local_cfg = { 0 }; + int ret; ipc_dev = ipc_get_comp_by_id(ipc_get(), comp_id); if (!ipc_dev) @@ -129,7 +132,21 @@ static int idc_get_attribute(uint32_t comp_id) idc_payload = idc_payload_get(*idc_get(), cpu_get_id()); get_attr_payload = (struct get_attribute_remote_payload *)idc_payload; - 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 + * 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; } /** diff --git a/src/include/sof/audio/component_ext.h b/src/include/sof/audio/component_ext.h index 74562203864e..c6030be2229d 100644 --- a/src/include/sof/audio/component_ext.h +++ b/src/include/sof/audio/component_ext.h @@ -275,6 +275,29 @@ struct get_attribute_remote_payload { void *value; /**< Pointer to attribute value storage */ }; +/* + * Copy to/from a coherent (uncached) buffer using aligned 32-bit word accesses. + * + * A whole-struct memcpy() straight into (or out of) the uncached alias faults + * on Xtensa with EXCCAUSE 9: the compiler lowers the struct copy to wide/HiFi + * accesses (e.g. AE_L32X2 / 64-bit loads) that the uncached window does not + * support. The IDC status word (a single aligned uint32_t) already round-trips + * through this same coherent region without faulting, so restricting the copy + * to volatile 32-bit word transactions is safe. @bytes must be a multiple of 4 + * and both pointers 32-bit aligned (guaranteed here: ipc4_base_module_cfg is + * aligned(4) and rzalloc() returns >=4-byte-aligned memory). + */ +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]; +} + /** * \brief Retrieve component attribute from a remote core (IPC4). * \param[in,out] dev Component device. @@ -305,9 +328,12 @@ static inline int comp_ipc4_get_attribute_remote(struct comp_dev *dev, uint32_t ret = idc_send_msg(&msg, IDC_BLOCKING); + /* base_cfg is coherent (uncached) -- read it back with 32-bit word + * accesses, not a struct memcpy that would fault on the uncached alias. + */ if (ret == 0) - memcpy_s(value, sizeof(struct ipc4_base_module_cfg), - base_cfg, sizeof(struct ipc4_base_module_cfg)); + ipc4_coherent_copy32(value, base_cfg, + sizeof(struct ipc4_base_module_cfg)); rfree(base_cfg); return ret; From 5875cbba840e586cdd1125bb064cb40aeb61c50c Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Sun, 26 Jul 2026 20:37:46 +0100 Subject: [PATCH 3/4] audio: component: make comp_prepare_remote non-blocking 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 --- src/include/sof/audio/component_ext.h | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/include/sof/audio/component_ext.h b/src/include/sof/audio/component_ext.h index c6030be2229d..2acf0fd552f2 100644 --- a/src/include/sof/audio/component_ext.h +++ b/src/include/sof/audio/component_ext.h @@ -242,7 +242,21 @@ static inline int comp_prepare_remote(struct comp_dev *dev) struct idc_msg msg = { IDC_MSG_PREPARE, IDC_MSG_PREPARE_EXT(dev->ipc_config.id), dev->ipc_config.core, }; - return idc_send_msg(&msg, IDC_BLOCKING); + /* + * 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); } /** From 7411f409bd5a54cf2364eeda91d045df5e437fa0 Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Tue, 28 Jul 2026 18:43:58 +0100 Subject: [PATCH 4/4] audio: component: wait for remote component reset to complete 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 --- posix/include/rtos/idc.h | 3 +++ src/idc/zephyr_idc.c | 9 ++++++++- src/include/sof/audio/component_ext.h | 17 ++++++++++++++++- zephyr/include/rtos/idc.h | 9 +++++++++ 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/posix/include/rtos/idc.h b/posix/include/rtos/idc.h index 3f4c48bf0f51..e7ff4ae442c7 100644 --- a/posix/include/rtos/idc.h +++ b/posix/include/rtos/idc.h @@ -28,6 +28,9 @@ /** \brief IDC send non-blocking flag. */ #define IDC_NON_BLOCKING 1 +/** \brief IDC send blocking flag, wait for completion without timing out. */ +#define IDC_BLOCKING_NO_TIMEOUT 4 + /** \brief IDC send core power up flag. */ #define IDC_POWER_UP 2 diff --git a/src/idc/zephyr_idc.c b/src/idc/zephyr_idc.c index 70b73cab97df..e08e26176107 100644 --- a/src/idc/zephyr_idc.c +++ b/src/idc/zephyr_idc.c @@ -157,7 +157,7 @@ int idc_send_msg(struct idc_msg *msg, uint32_t mode) work->priority = CONFIG_EDF_THREAD_PRIORITY; work->deadline = 0; work->handler = idc_handler; - work->sync = mode == IDC_BLOCKING; + work->sync = mode == IDC_BLOCKING || mode == IDC_BLOCKING_NO_TIMEOUT; if (msg->payload) { idc_send_memcpy_err = memcpy_s(payload->data, sizeof(payload->data), @@ -189,6 +189,13 @@ int idc_send_msg(struct idc_msg *msg, uint32_t mode) /* message was sent and executed successfully, get status code */ ret = idc_msg_status_get(msg->core); break; + 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; case IDC_POWER_UP: case IDC_NON_BLOCKING: default: diff --git a/src/include/sof/audio/component_ext.h b/src/include/sof/audio/component_ext.h index 2acf0fd552f2..ca42fc728f56 100644 --- a/src/include/sof/audio/component_ext.h +++ b/src/include/sof/audio/component_ext.h @@ -405,7 +405,22 @@ static inline int comp_reset_remote(struct comp_dev *dev) struct idc_msg msg = { IDC_MSG_RESET, IDC_MSG_RESET_EXT(dev->ipc_config.id), dev->ipc_config.core, }; - return idc_send_msg(&msg, IDC_BLOCKING); + /* + * Blocking, and waiting for actual completion (no timeout): the caller + * (pipeline_reset -> host) depends on the reset being finished before it + * frees the component and powers down the remote core. A DP decoder's + * reset runs schedule_task_cancel() -> k_thread_join() on that core, which + * only returns once the DP thread finishes its in-flight process() call. + * That call is bounded but can be long during codec cold-start (a deferred + * avcodec_open2 table build, ~1.4 s for aac, or the first decode after it), + * exceeding CONFIG_IDC_TIMEOUT_US. A plain IDC_BLOCKING would give up at + * that timeout (-EAGAIN) while the reset is still running remotely, leaving + * the component un-reset; pipeline_reset() then fails and the subsequent + * free is refused (invalid state) / the core cannot be disabled, wedging + * every teardown until reboot. A normal end-of-stream teardown joins an + * idle DP thread and returns immediately, so this only waits when it must. + */ + return idc_send_msg(&msg, IDC_BLOCKING_NO_TIMEOUT); } /** diff --git a/zephyr/include/rtos/idc.h b/zephyr/include/rtos/idc.h index 209cefcd6d9c..04b9b75b14f5 100644 --- a/zephyr/include/rtos/idc.h +++ b/zephyr/include/rtos/idc.h @@ -28,6 +28,15 @@ /** \brief IDC send non-blocking flag. */ #define IDC_NON_BLOCKING 1 +/** \brief IDC send blocking flag, wait for completion without timing out. + * Like IDC_BLOCKING but waits (K_FOREVER) for the handler to finish rather + * than only CONFIG_IDC_TIMEOUT_US. For remote ops whose completion the + * caller depends on and which are bounded but can exceed the IDC timeout + * (e.g. a DP component reset that joins a decoder thread mid codec + * cold-start). + */ +#define IDC_BLOCKING_NO_TIMEOUT 4 + /** \brief IDC send core power up flag. */ #define IDC_POWER_UP 2