From 8679ca3323c8eb3feac780954a715cd7cb39f810 Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Tue, 18 Mar 2025 16:43:48 +0100 Subject: [PATCH 1/2] cmake/zephyr: decentralize src/debug/ Adding all source files in a single, giant zephyr/CMakeLists.txt is inconvenient and does not scale. Link: https://github.com/thesofproject/sof/issues/8260 Signed-off-by: Guennadi Liakhovetski --- src/debug/CMakeLists.txt | 14 +++++++++++--- src/debug/gdb/CMakeLists.txt | 1 + src/debug/tester/CMakeLists.txt | 4 +++- zephyr/CMakeLists.txt | 10 +--------- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/debug/CMakeLists.txt b/src/debug/CMakeLists.txt index 60521cfa2cb0..cf460504f2f7 100644 --- a/src/debug/CMakeLists.txt +++ b/src/debug/CMakeLists.txt @@ -4,8 +4,16 @@ if(CONFIG_GDB_DEBUG) add_subdirectory(gdb) endif() -if(CONFIG_COMP_TESTER) - add_subdirectory(tester) -endif() +add_subdirectory(tester) + +is_zephyr(it_is) +if(it_is) ### Zephyr ### + +add_subdirectory(debug_stream) +add_subdirectory(telemetry) + +else() ### Not Zephyr ### add_local_sources(sof panic.c) + +endif() # Zephyr diff --git a/src/debug/gdb/CMakeLists.txt b/src/debug/gdb/CMakeLists.txt index 3cd0fe5f2d2b..4ca1af254b05 100644 --- a/src/debug/gdb/CMakeLists.txt +++ b/src/debug/gdb/CMakeLists.txt @@ -1,5 +1,6 @@ # SPDX-License-Identifier: BSD-3-Clause +# Common to Zephyr and XTOS add_local_sources(sof gdb.c ringbuffer.c diff --git a/src/debug/tester/CMakeLists.txt b/src/debug/tester/CMakeLists.txt index aa146c24d597..2220c5b773b8 100644 --- a/src/debug/tester/CMakeLists.txt +++ b/src/debug/tester/CMakeLists.txt @@ -22,6 +22,8 @@ if(it_is) ### Zephyr ### else() ### XTOS ### - add_local_sources(sof ${base_files}) + if(CONFIG_COMP_TESTER) + add_local_sources(sof ${base_files}) + endif() endif() diff --git a/zephyr/CMakeLists.txt b/zephyr/CMakeLists.txt index 7a3c92f9ce64..1d030866a5db 100644 --- a/zephyr/CMakeLists.txt +++ b/zephyr/CMakeLists.txt @@ -139,7 +139,6 @@ set(SOF_AUDIO_MODULES_PATH "${SOF_SRC_PATH}/audio/module_adapter/module") set(SOF_SAMPLES_PATH "${SOF_SRC_PATH}/samples") set(SOF_LIB_PATH "${SOF_SRC_PATH}/lib") set(SOF_DRIVERS_PATH "${SOF_SRC_PATH}/drivers") -set(SOF_DEBUG_PATH "${SOF_SRC_PATH}/debug") set(SOF_TRACE_PATH "${SOF_SRC_PATH}/trace") set(RIMAGE_TOP ${sof_top_dir}/tools/rimage) @@ -204,9 +203,7 @@ macro(sof_list_append_ifdef feature_toggle list) endmacro() add_subdirectory(../src/audio/ audio_unused_install/) -add_subdirectory(../src/debug/debug_stream/ debug_stream_unused_install/) -add_subdirectory(../src/debug/telemetry/ telemetry_unused_install/) -add_subdirectory(../src/debug/tester/ debug_tester_unused_install/) +add_subdirectory(../src/debug/ debug_unused_install/) add_subdirectory(../src/init/ init_unused_install/) add_subdirectory(../src/ipc/ ipc_unused_install/) add_subdirectory(../src/math/ math_unused_install/) @@ -909,11 +906,6 @@ zephyr_library_sources_ifdef(CONFIG_AMS ${SOF_LIB_PATH}/ams.c ) -zephyr_library_sources_ifdef(CONFIG_GDB_DEBUG - ${SOF_DEBUG_PATH}/gdb/gdb.c - ${SOF_DEBUG_PATH}/gdb/ringbuffer.c -) - zephyr_library_sources_ifdef(CONFIG_DW_DMA ${SOF_DRIVERS_PATH}/dw/dma.c ) From e78f6241d1ee752182cb22e129d31ea8670e0445 Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Tue, 18 Mar 2025 14:48:59 +0200 Subject: [PATCH 2/2] dram: add temporarily banned DRAM execution DRAM execution is banned in LL task context, but there are also time intervals during which performance is important. Moreover, sometimes such critical periods aren't certain until a later time. E.g. when processing IPCs, only some of them are performance critical. And those time-critical IPCs should be handled with maximum performance from IPC-entry till IPC-exit, not only while handling that specific IPC. Consider this pseudo-code: void ipc_cmd() { common_pre_processing(); switch (cmd) { case NON_CRITICAL_CMD: handle_non_critical(); break; case CRITICAL_CMD: handle_critical(); } common_post_processing(); } In this case ipc_cmd(), common_pre_processing(), handle_critical() and common_post_processing() cannot be executed in DRAM, while handle_non_critical() can be executed in DRAM. If we place start-critical-debugging and stop-critical-debugging to cover all of ipc_cmd(), we'll be forced to also place handle_non_critical() in SRAM. OTOH if we only surround handle_critical() with those markers, we will miss all the common code. To support such cases we use 3 markers: mem_hot_path_start_watching() mem_hot_path_stop_watching() mem_hot_path_confirm() Then we place start- and stop-watching in the beginning and end of ipc_cmd(), and mem_hot_path_confirm() under the CRITICAL_CMD case. Then if we made common_pre_processing() or common_post_processing() __cold, the fact of them being called while watching will be recorded. Then mem_hot_path_confirm() will set a confirmation flag. Then when watching is stopped, we check if the flag was raised and a cold function was called, in which case we issue an error. Signed-off-by: Guennadi Liakhovetski --- posix/include/sof/lib/memory.h | 1 + src/debug/CMakeLists.txt | 2 + src/debug/dram.c | 67 +++++++++++++++++++++++++++++++++ src/idc/idc.c | 2 + src/ipc/ipc-zephyr.c | 4 ++ src/ipc/ipc4/handler.c | 9 +++++ xtos/include/sof/lib/memory.h | 1 + zephyr/include/sof/lib/memory.h | 10 +++++ 8 files changed, 96 insertions(+) create mode 100644 src/debug/dram.c diff --git a/posix/include/sof/lib/memory.h b/posix/include/sof/lib/memory.h index 219ebf2bc76c..8f191a872df3 100644 --- a/posix/include/sof/lib/memory.h +++ b/posix/include/sof/lib/memory.h @@ -19,5 +19,6 @@ #endif #define assert_can_be_cold() do {} while (0) +#define mem_hot_path_confirm() do {} while (0) #endif /* __SOF_LIB_MEMORY_H__ */ diff --git a/src/debug/CMakeLists.txt b/src/debug/CMakeLists.txt index cf460504f2f7..3025cb8919f5 100644 --- a/src/debug/CMakeLists.txt +++ b/src/debug/CMakeLists.txt @@ -12,6 +12,8 @@ if(it_is) ### Zephyr ### add_subdirectory(debug_stream) add_subdirectory(telemetry) +zephyr_library_sources_ifdef(CONFIG_COLD_STORE_EXECUTE_DEBUG dram.c) + else() ### Not Zephyr ### add_local_sources(sof panic.c) diff --git a/src/debug/dram.c b/src/debug/dram.c new file mode 100644 index 000000000000..7fe14ca1d926 --- /dev/null +++ b/src/debug/dram.c @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: BSD-3-Clause +// +// Copyright(c) 2025 Intel Corporation. + +#include +#include +#include +#include + +LOG_MODULE_REGISTER(dram_debug, CONFIG_SOF_LOG_LEVEL); + +static struct k_spinlock hot_path_lock; +static unsigned int hot_path_depth; +static const char *cold_path_fn; +static bool hot_path_confirmed; + +void mem_cold_path_enter(const char *fn) +{ + k_spinlock_key_t key = k_spin_lock(&hot_path_lock); + + cold_path_fn = fn; + + k_spin_unlock(&hot_path_lock, key); +} +EXPORT_SYMBOL(mem_cold_path_enter); + +void mem_hot_path_start_watching(void) +{ + k_spinlock_key_t key = k_spin_lock(&hot_path_lock); + + if (!hot_path_depth++) { + cold_path_fn = NULL; + hot_path_confirmed = false; + } + + k_spin_unlock(&hot_path_lock, key); +} + +void mem_hot_path_confirm(void) +{ + k_spinlock_key_t key = k_spin_lock(&hot_path_lock); + + hot_path_confirmed = true; + + k_spin_unlock(&hot_path_lock, key); +} + +void mem_hot_path_stop_watching(void) +{ + bool underrun, error; + k_spinlock_key_t key = k_spin_lock(&hot_path_lock); + + if (hot_path_depth) { + underrun = false; + hot_path_depth--; + error = hot_path_confirmed && cold_path_fn; + } else { + error = underrun = true; + } + + k_spin_unlock(&hot_path_lock, key); + + if (underrun) + LOG_ERR("Hot path depth underrun!"); + else + __ASSERT(!error, "Cold function %s() has run while on hot path!", cold_path_fn); +} diff --git a/src/idc/idc.c b/src/idc/idc.c index 7fe0988788ea..3154b8d5fa15 100644 --- a/src/idc/idc.c +++ b/src/idc/idc.c @@ -425,7 +425,9 @@ void idc_cmd(struct idc_msg *msg) notifier_notify_remote(); break; case iTS(IDC_MSG_IPC): + mem_hot_path_start_watching(); idc_ipc(); + mem_hot_path_stop_watching(); break; #if CONFIG_IPC_MAJOR_4 case iTS(IDC_MSG_BIND): diff --git a/src/ipc/ipc-zephyr.c b/src/ipc/ipc-zephyr.c index fc2eac10166c..b71ff94f32bc 100644 --- a/src/ipc/ipc-zephyr.c +++ b/src/ipc/ipc-zephyr.c @@ -217,11 +217,15 @@ enum task_state ipc_platform_do_cmd(struct ipc *ipc) { struct ipc_cmd_hdr *hdr; + mem_hot_path_start_watching(); + hdr = ipc_compact_read_msg(); /* perform command */ ipc_cmd(hdr); + mem_hot_path_stop_watching(); + if (ipc->task_mask & IPC_TASK_POWERDOWN || ipc_get()->pm_prepare_D3) { #if defined(CONFIG_PM) diff --git a/src/ipc/ipc4/handler.c b/src/ipc/ipc4/handler.c index dbffb96e415d..625b3b309c57 100644 --- a/src/ipc/ipc4/handler.c +++ b/src/ipc/ipc4/handler.c @@ -477,6 +477,15 @@ int ipc4_pipeline_trigger(struct ipc_comp_dev *ppl_icd, uint32_t cmd, bool *dela return IPC4_INVALID_REQUEST; } + /* + * We're handling a pipeline-trigger event, this means that we're in a + * performance-critical context. Set a marker, so that if any cold code, + * calling assert_can_be_cold() is called on this flow between the + * mem_hot_path_start_watching() - mem_hot_path_stop_watching() + * brackets, the latter will generate an error / trigger a panic. + */ + mem_hot_path_confirm(); + /* trigger the component */ ret = pipeline_trigger(host->cd->pipeline, host->cd, cmd); if (ret < 0) { diff --git a/xtos/include/sof/lib/memory.h b/xtos/include/sof/lib/memory.h index 219ebf2bc76c..8f191a872df3 100644 --- a/xtos/include/sof/lib/memory.h +++ b/xtos/include/sof/lib/memory.h @@ -19,5 +19,6 @@ #endif #define assert_can_be_cold() do {} while (0) +#define mem_hot_path_confirm() do {} while (0) #endif /* __SOF_LIB_MEMORY_H__ */ diff --git a/zephyr/include/sof/lib/memory.h b/zephyr/include/sof/lib/memory.h index bd0ebc6e61b8..16c2e686420e 100644 --- a/zephyr/include/sof/lib/memory.h +++ b/zephyr/include/sof/lib/memory.h @@ -25,12 +25,22 @@ bool ll_sch_is_current(void); #define ll_sch_is_current() false #endif +void mem_hot_path_start_watching(void); +void mem_hot_path_stop_watching(void); +void mem_hot_path_confirm(void); +void mem_cold_path_enter(const char *fn); + static inline void __assert_can_be_cold(const char *fn) { __ASSERT(!ll_sch_is_current(), "%s() called from an LL thread!", fn); + mem_cold_path_enter(fn); } #define assert_can_be_cold() __assert_can_be_cold(__func__) #else +#define mem_hot_path_start_watching() do {} while (0) +#define mem_hot_path_stop_watching() do {} while (0) +#define mem_hot_path_confirm() do {} while (0) +#define mem_cold_path_enter() do {} while (0) #define assert_can_be_cold() do {} while (0) #endif