From cf97f4b20550aa2f2f774c89c777c3b6b255286e Mon Sep 17 00:00:00 2001 From: Rander Wang Date: Thu, 18 Jan 2024 12:56:29 +0800 Subject: [PATCH 1/5] Revert "ipc4: relax the IPC timeout checks and be nicer to other threads" When a stream is triggered to start, host kernel first sendis trigger start ipc message to fw and then start host dma for this stream. Ipc_wait_for_compound_msg is used to wait for all pipelines in the stream to be complete and need to be done fast since it blocks host to start hda dma. The reverted commit adds a 10 ms delay and results to host copier xrun warning for it can't get data from host dma. And this commit also makes a negative effect for stream_start_offset calculation which is for the difference between dai gateway and host gateway. We calculate it in host start function but host dma is started 10ms later, so there will be at least 10ms data error. This reverts commit 909a3277f1271d91314736fc1eaff35f362176d1. Signed-off-by: Rander Wang --- src/ipc/ipc4/handler.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/ipc/ipc4/handler.c b/src/ipc/ipc4/handler.c index e70fd4e278d0..f36504989e69 100644 --- a/src/ipc/ipc4/handler.c +++ b/src/ipc/ipc4/handler.c @@ -510,13 +510,12 @@ static void ipc_compound_msg_done(uint32_t msg_id, int error) } } -/* wait for IPCs to complete on other cores and be nice to any LL work */ static int ipc_wait_for_compound_msg(void) { - int try_count = 30; /* timeout out is 30 x 10ms so 300ms for IPC */ + int try_count = 30; while (atomic_read(&msg_data.delayed_reply)) { - k_sleep(Z_TIMEOUT_MS(10)); + k_sleep(Z_TIMEOUT_US(250)); if (!try_count--) { atomic_set(&msg_data.delayed_reply, 0); From 7ab90ffea13d8d44b6019de245c7b7be5a90bc8c Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Wed, 24 Jan 2024 21:03:01 +0200 Subject: [PATCH 2/5] module_adapter: fix double-free of base_cfg_ext data Fix handling of input_pins free. Module reset can be called multiple times, so move the resource release to module_adapter_free(). Fixes: 70460043844f ("module_adapter_ipc4: Save and pre-parse base_cfg_ext data") Signed-off-by: Kai Vehmanen --- src/audio/module_adapter/module_adapter.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/audio/module_adapter/module_adapter.c b/src/audio/module_adapter/module_adapter.c index cde176f2fadb..61d098c2e12f 100644 --- a/src/audio/module_adapter/module_adapter.c +++ b/src/audio/module_adapter/module_adapter.c @@ -1338,10 +1338,6 @@ int module_adapter_reset(struct comp_dev *dev) rfree(mod->stream_params); mod->stream_params = NULL; -#if CONFIG_IPC_MAJOR_4 - rfree(mod->priv.cfg.input_pins); -#endif - comp_dbg(dev, "module_adapter_reset(): done"); return comp_set_state(dev, COMP_TRIGGER_RESET); @@ -1371,6 +1367,10 @@ void module_adapter_free(struct comp_dev *dev) buffer_free(buffer); } +#if CONFIG_IPC_MAJOR_4 + rfree(mod->priv.cfg.input_pins); +#endif + rfree(mod); rfree(dev); } From a4359feb3f6b1dff02429de6d9b0edab04f3e757 Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Wed, 17 Jan 2024 22:51:09 +0200 Subject: [PATCH 3/5] idc: zephyr: fix race between IDC p4wq worker and new msg submission A race exists between submitting a new IDC message and completion of a previous message. The p4wq interface mandates k_p4wq_wait() must be called to claim back ownership of the work object. The SOF IDC code does not do this, but relies on the caller not to send multiple IDC messages to the same destination core. Add appropriate calls to k_p4wq_wait() to handle this issue. If caller wants to send an IDC in blocking mode, also call k_p4wq_wait() in blocking mode. If non-blocking, return -EBUSY immediately. The check for CPU status is moved earlier in the function. No pointing in waiting for p4wq semaphore if the target core is already powered down, so do the power state check as the first step. Signed-off-by: Kai Vehmanen (cherry picked from commit 342553cf91801ebe751c7613d247104155936eae) --- src/idc/zephyr_idc.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/src/idc/zephyr_idc.c b/src/idc/zephyr_idc.c index 887cff0eb375..5d73016958bf 100644 --- a/src/idc/zephyr_idc.c +++ b/src/idc/zephyr_idc.c @@ -121,6 +121,32 @@ int idc_send_msg(struct idc_msg *msg, uint32_t mode) int ret; int idc_send_memcpy_err __unused; + if (!cpu_is_core_enabled(target_cpu)) { + tr_err(&zephyr_idc_tr, "Core %u is down, cannot sent IDC message", target_cpu); + return -EACCES; + } + + /* + * Handler is NULL when work object has never been submitted. + * In all other cases, we must use k_p4wq_wait() before reuse + * of the object. + */ + if (work->handler) { + /* + * If new request is in blocking mode, we must call + * k_p4wq in blocking mode. This is workaround for + * k_p4wq_wait() interface. + */ + work->sync = (mode == IDC_BLOCKING); + + ret = k_p4wq_wait(work, K_USEC(IDC_TIMEOUT)); + if (ret < 0) { + tr_err(&zephyr_idc_tr, "idc_send_msg error %d, target core %u", + ret, target_cpu); + return ret; + } + } + idc_send_memcpy_err = memcpy_s(msg_cp, sizeof(*msg_cp), msg, sizeof(*msg)); assert(!idc_send_memcpy_err); /* Same priority as the IPC thread which is an EDF task and under Zephyr */ @@ -129,10 +155,6 @@ int idc_send_msg(struct idc_msg *msg, uint32_t mode) work->handler = idc_handler; work->sync = mode == IDC_BLOCKING; - if (!cpu_is_core_enabled(target_cpu)) { - tr_err(&zephyr_idc_tr, "Core %u is down, cannot sent IDC message", target_cpu); - return -EACCES; - } if (msg->payload) { idc_send_memcpy_err = memcpy_s(payload->data, sizeof(payload->data), msg->payload, msg->size); From d4ae4b9ef9cc8fd5b6843e5b24a51963d2d35230 Mon Sep 17 00:00:00 2001 From: Daniel Leung Date: Fri, 3 Nov 2023 11:57:30 -0700 Subject: [PATCH 4/5] zephyr: use k_smp_cpu_start/_resume for secondary core power up This changes the secondary core power up routine to use the newly introduced k_smp_cpu_start() and k_smp_cpu_resume(). This removes the need to mirror part of the SMP start up code from Zephyr, and no longer need to call into Zephyr private kernel code. West update includes : eefaeee061c8 kernel: smp: introduce k_smp_cpu_resume 042cb6ac4e00 soc: intel_adsp: enable DfTTS-based time stamping on ACE platforms 6a0b1da158a4 soc: intel_adsp: call framework callback function for restore e7217925c93e ace: use a 'switch' statement in pm_state_set() c99a604bbf2c ace: remove superfluous variable initialisation a0ac2faf9bde intel_adsp: ace: enable power domain 4204ca9bcb3f ace: fix DSP panic during startup (fixes c3a6274bf5e4) d4b0273ab0c4 cmake: sparse.template: add COMMAND_ERROR_IS_FATAL ca12fd13c6d3 xtensa: intel_adsp: fix a cache handling error 0ee1e28a2f5f xtensa: polish doxygen and add to missing doc 035c8d8ceb4b xtensa: remove sys_define_gpr_with_alias() a64eec6aaeec xtensa: remove XTENSA_ERR_NORET Signed-off-by: Daniel Leung Signed-off-by: Rander Wang --- west.yml | 2 +- zephyr/lib/cpu.c | 80 ++++++++++-------------------------------------- 2 files changed, 18 insertions(+), 64 deletions(-) diff --git a/west.yml b/west.yml index 610f7d3f1c7b..b615055fc3f7 100644 --- a/west.yml +++ b/west.yml @@ -43,7 +43,7 @@ manifest: - name: zephyr repo-path: zephyr - revision: d7af6f371034f31b9440b27c694b0be3c87491d3 + revision: 6a0b1da158a4f8bc5f2ca5058637dce26a38660e remote: zephyrproject # Import some projects listed in zephyr/west.yml@revision diff --git a/zephyr/lib/cpu.c b/zephyr/lib/cpu.c index 3dde19dc38bc..6a896255795a 100644 --- a/zephyr/lib/cpu.c +++ b/zephyr/lib/cpu.c @@ -19,6 +19,7 @@ /* Zephyr includes */ #include #include +#include #include #include @@ -27,37 +28,9 @@ extern K_KERNEL_STACK_ARRAY_DEFINE(z_interrupt_stacks, CONFIG_MP_MAX_NUM_CPUS, CONFIG_ISR_STACK_SIZE); -static atomic_t start_flag; -static atomic_t ready_flag; - -/* Zephyr kernel_internal.h interface */ -extern void smp_timer_init(void); - -static FUNC_NORETURN void secondary_init(void *arg) +static void secondary_init(void *arg) { - struct k_thread dummy_thread; - - /* - * This is an open-coded version of zephyr/kernel/smp.c - * smp_init_top(). We do this so that we can call SOF - * secondary_core_init() for each core. - */ - - atomic_set(&ready_flag, 1); - z_smp_thread_init(arg, &dummy_thread); - smp_timer_init(); - secondary_core_init(sof_get()); - -#ifdef CONFIG_THREAD_STACK_INFO - dummy_thread.stack_info.start = (uintptr_t)z_interrupt_stacks + - arch_curr_cpu()->id * Z_KERNEL_STACK_LEN(CONFIG_ISR_STACK_SIZE); - dummy_thread.stack_info.size = Z_KERNEL_STACK_LEN(CONFIG_ISR_STACK_SIZE); -#endif - - z_smp_thread_swap(); - - CODE_UNREACHABLE; /* LCOV_EXCL_LINE */ } #if CONFIG_ZEPHYR_NATIVE_DRIVERS @@ -113,7 +86,6 @@ void cpu_notify_state_exit(enum pm_state state) /* Notifying primary core that secondary core successfully exit the D3 * state and is back in the Idle thread. */ - atomic_set(&ready_flag, 1); return; } #endif @@ -142,26 +114,17 @@ int cpu_enable_core(int id) if (arch_cpu_active(id)) return 0; -#if ZEPHYR_VERSION(3, 0, 99) <= ZEPHYR_VERSION_CODE /* During kernel initialization, the next pm state is set to ACTIVE. By checking this * value, we determine if this is the first core boot, if not, we need to skip idle thread * initialization. By reinitializing the idle thread, we would overwrite the kernel structs * and the idle thread stack. */ - if (pm_state_next_get(id)->state == PM_STATE_ACTIVE) - z_init_cpu(id); -#endif - - atomic_clear(&start_flag); - atomic_clear(&ready_flag); - - arch_start_cpu(id, z_interrupt_stacks[id], CONFIG_ISR_STACK_SIZE, - secondary_init, &start_flag); - - while (!atomic_get(&ready_flag)) - k_busy_wait(100); + if (pm_state_next_get(id)->state == PM_STATE_ACTIVE) { + k_smp_cpu_start(id, secondary_init, NULL); + return 0; + } - atomic_set(&start_flag, 1); + k_smp_cpu_resume(id, secondary_init, NULL, true, false); return 0; } @@ -239,29 +202,20 @@ int cpu_enable_core(int id) int cpu_enable_secondary_core(int id) { - /* - * This is an open-coded version of zephyr/kernel/smp.c - * z_smp_start_cpu(). We do this, so we can use a customized - * secondary_init() for SOF. - */ - if (arch_cpu_active(id)) return 0; -#if ZEPHYR_VERSION(3, 0, 99) <= ZEPHYR_VERSION_CODE - z_init_cpu(id); -#endif - - atomic_clear(&start_flag); - atomic_clear(&ready_flag); - - arch_start_cpu(id, z_interrupt_stacks[id], CONFIG_ISR_STACK_SIZE, - secondary_init, &start_flag); - - while (!atomic_get(&ready_flag)) - k_busy_wait(100); + /* During kernel initialization, the next pm state is set to ACTIVE. By checking this + * value, we determine if this is the first core boot, if not, we need to skip idle thread + * initialization. By reinitializing the idle thread, we would overwrite the kernel structs + * and the idle thread stack. + */ + if (pm_state_next_get(id)->state == PM_STATE_ACTIVE) { + k_smp_cpu_start(id, secondary_init, NULL); + return 0; + } - atomic_set(&start_flag, 1); + k_smp_cpu_resume(id, secondary_init, NULL, true, false); return 0; } From 35423116b3c5abaa6a2b5ed8dc5f5d2fc48af74a Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Thu, 25 Jan 2024 13:54:38 +0200 Subject: [PATCH 5/5] [DNM][RD] west.yml: test Zephyr PR68110 Combo of 3 PRs in Zephyr. --- west.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/west.yml b/west.yml index b615055fc3f7..7f935f23a948 100644 --- a/west.yml +++ b/west.yml @@ -43,7 +43,7 @@ manifest: - name: zephyr repo-path: zephyr - revision: 6a0b1da158a4f8bc5f2ca5058637dce26a38660e + revision: pull/68110/head remote: zephyrproject # Import some projects listed in zephyr/west.yml@revision