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/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/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 74562203864e..ca42fc728f56 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); } /** @@ -275,6 +289,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 +342,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; @@ -365,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 3e30250344e2..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 @@ -149,10 +158,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 {