Skip to content

idc: fix cross-core component attribute access, prepare, and reset - #11334

Draft
lgirdwood wants to merge 4 commits into
thesofproject:mainfrom
lgirdwood:topic/idc-remote-fixes
Draft

lgirdwood wants to merge 4 commits into
thesofproject:mainfrom
lgirdwood:topic/idc-remote-fixes

Conversation

@lgirdwood

Copy link
Copy Markdown
Member

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):

  1. zephyr: idc: align the IDC payload to the cache line:

    • struct idc_payload lives 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 trigger EXCCAUSE 9 (LoadStoreAlignmentCause) when handlers read wider fields (uint32_t). Aligns struct idc_payload to DCACHE_LINE_SIZE.
  2. audio: component: copy coherent get_attribute payload with 32-bit stores:

    • The remote get_attribute handler previously passed the uncached coherent IDC payload directly to comp_get_attribute(), which memcpy()s struct 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, faulting EXCCAUSE 9. Copies via a local cacheable structure and 32-bit word stores (ipc4_coherent_copy32).
  3. audio: component: make comp_prepare_remote non-blocking:

    • Remote component prepare() on a DP module can exceed CONFIG_IDC_TIMEOUT_US during one-time table generation, stalling the initiator core IPC/EDF thread or timing out (-EAGAIN). Sends prepare IDC non-blocking; in-order p4wq execution on the target core preserves ordering before subsequent trigger (START) messages.
  4. audio: component: wait for remote component reset to complete:

    • comp_reset_remote() previously used IDC_BLOCKING (CONFIG_IDC_TIMEOUT_US = 15 ms), which can time out if the target core's DP thread is finishing an in-flight process() call during schedule_task_cancel() -> k_thread_join(), leaving the component un-reset and wedging teardown. Introduces IDC_BLOCKING_NO_TIMEOUT (K_FOREVER) for comp_reset_remote().

Copilot AI balanced review requested due to automatic review settings October 22, 2026 13:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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_payload to DCACHE_LINE_SIZE to prevent Xtensa alignment faults when reading payload fields as wider types.
  • Introduces IDC_BLOCKING_NO_TIMEOUT and implements a K_FOREVER wait 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

Comment thread src/idc/idc.c
Comment on lines +135 to +149
/*
* 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;
Comment thread src/idc/zephyr_idc.c
Comment on lines +192 to +198
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;
Comment on lines +246 to +259
* 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);
Comment thread src/idc/idc.c
if (ret < 0)
return ret;

ipc4_coherent_copy32(get_attr_payload->value, &local_cfg, sizeof(local_cfg));
Comment on lines +304 to +313
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 lgirdwood left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use a vectorized version here, there is a global call that can be reused.

Comment thread src/idc/idc.c

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
/*

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revisit after ffmpeg merge

@lgirdwood
lgirdwood marked this pull request as draft October 22, 2026 13:36
@intel-sofci

Copy link
Copy Markdown

PR 11334: test results

Run date: 2026-10-22 13:32 UTC

Tested commit: 035cf0927f428486419923dc223e9493d0812cc0

mtl pass rate lnl pass rate ptl pass rate wcl pass rate nvl pass rate

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>
@lgirdwood
lgirdwood force-pushed the topic/idc-remote-fixes branch from 035cf09 to 7411f40 Compare October 22, 2026 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants