Skip to content

DRAM debug take 2 #9907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lgirdwood merged 2 commits into from
Mar 28, 2025
Merged

DRAM debug take 2 #9907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8679ca3
cmake/zephyr: decentralize src/debug/
lyakh Mar 18, 2025
e78f624
dram: add temporarily banned DRAM execution
lyakh Mar 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions posix/include/sof/lib/memory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__ */
16 changes: 13 additions & 3 deletions src/debug/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,18 @@ if(CONFIG_GDB_DEBUG)
add_subdirectory(gdb)
endif()

if(CONFIG_COMP_TESTER)
add_subdirectory(tester)
endif()
add_subdirectory(tester)

is_zephyr(it_is)
Copy link
Member

@lgirdwood lgirdwood Mar 20, 2025

Choose a reason for hiding this comment

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

I think @kv2019i has improved this check now.

Copy link
Member

@abonislawski abonislawski Mar 20, 2025

Choose a reason for hiding this comment

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

I think we could stay just with assert_can_be_cold to not overcomplicate dram things

Copy link
Collaborator Author

@lyakh lyakh Mar 20, 2025

Choose a reason for hiding this comment

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

@lgirdwood don't think so, we plan to do this globally but I don't think this has been changed yet, I still see this in "main"

Copy link
Collaborator Author

@lyakh lyakh Mar 21, 2025

Choose a reason for hiding this comment

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

I think we could stay just with assert_can_be_cold to not overcomplicate dram things

@abonislawski that part doesn't change. That's remains the only check that we add to __cold functions. But inside that function it now performs 2 checks: (1) for LL-context, and (2) for any "critical path" violations

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)

endif() # Zephyr
67 changes: 67 additions & 0 deletions src/debug/dram.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// SPDX-License-Identifier: BSD-3-Clause
//
// Copyright(c) 2025 Intel Corporation.

#include <rtos/kernel.h>
#include <rtos/symbol.h>
#include <zephyr/logging/log.h>
#include <zephyr/sys/__assert.h>

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)
Copy link
Member

@lgirdwood lgirdwood Mar 24, 2025

Choose a reason for hiding this comment

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

We really need to put all debug logic behind a dbg_ API prefix and a Kconfig to enable and disable.

Copy link
Collaborator Author

@lyakh lyakh Mar 26, 2025

Choose a reason for hiding this comment

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

@lgirdwood so far we don't seem to have such a consistent convention. If you grep the source tree for dbg_ you'll see a few internal uses and a few global macros in src/include/sof/debug/debug.h which don't seem to be used anywhere any more, so they can be removed. After that a consistent dbg_* namespace could be established. FWIW other files under src/debug/ don't use it either. One of the files there uses the debug_ prefix

Copy link
Member

@lgirdwood lgirdwood Mar 28, 2025

Choose a reason for hiding this comment

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

Lets make it consistent starting with this file. We really have to namespace APIs better.

{
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);
}
1 change: 1 addition & 0 deletions src/debug/gdb/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# SPDX-License-Identifier: BSD-3-Clause

# Common to Zephyr and XTOS
add_local_sources(sof
gdb.c
ringbuffer.c
Expand Down
4 changes: 3 additions & 1 deletion src/debug/tester/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
2 changes: 2 additions & 0 deletions src/idc/idc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
4 changes: 4 additions & 0 deletions src/ipc/ipc-zephyr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 9 additions & 0 deletions src/ipc/ipc4/handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Collaborator

@kv2019i kv2019i Mar 21, 2025

Choose a reason for hiding this comment

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

Would be an excellent place to add a comment why this is considered a hot-path.


/* trigger the component */
ret = pipeline_trigger(host->cd->pipeline, host->cd, cmd);
if (ret < 0) {
Expand Down
1 change: 1 addition & 0 deletions xtos/include/sof/lib/memory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__ */
10 changes: 1 addition & 9 deletions zephyr/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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/)
Expand Down Expand Up @@ -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
)
Expand Down
10 changes: 10 additions & 0 deletions zephyr/include/sof/lib/memory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Loading