Skip to content
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.

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

Debug module framework #9787

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 app/boards/intel_adsp_ace15_mtpm.conf
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ CONFIG_COMP_CHAIN_DMA=y
CONFIG_COMP_CROSSOVER=y
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had to read the commit a few times, then code, and then again the commit, to understand what this is about. I think what confuses is the generic "tester framework" language. I'd actually not use the term "framework" here. IOW, in the end you are adding an audio module that happens to be implemented in a way that allows for easy extension. It would be maybe easier to understand if you just write: "Tester is an audio module that can be configured via module IPC to perform various test functions."

I also wonder about the "tester" name. It is very generic. To add something specific (like load generation), a "loadgen" module would be more descriptive, but I guess you want to keep this generic on purpose.

Copy link
Collaborator

@dbaluta dbaluta Jan 22, 2025

Choose a reason for hiding this comment

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

@kv2019i agree with you. I'm still yet to go back and do re-read the code to understand what this is all about. The naming is unfortunate.

CONFIG_COMP_DRC=y
CONFIG_COMP_KPB=y
CONFIG_COMP_TESTER=m
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aa, right, so this is in separate commit, but the one change to mtl/modules.conf was left to a different commit?

CONFIG_COMP_SRC_IPC4_FULL_MATRIX=y
CONFIG_COMP_SRC_LITE=y
CONFIG_COMP_MFCC=y
Expand Down
1 change: 1 addition & 0 deletions app/boards/intel_adsp_ace20_lnl.conf
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ CONFIG_COMP_ARIA=y
CONFIG_COMP_CHAIN_DMA=y
CONFIG_COMP_DRC=m
CONFIG_COMP_KPB=y
CONFIG_COMP_TESTER=m
CONFIG_COMP_SRC_IPC4_FULL_MATRIX=y
CONFIG_COMP_UP_DOWN_MIXER=y
CONFIG_COMP_VOLUME_WINDOWS_FADE=y
Expand Down
1 change: 1 addition & 0 deletions app/boards/intel_adsp_ace30_ptl.conf
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ CONFIG_COMP_ARIA=y
CONFIG_COMP_CHAIN_DMA=y
CONFIG_COMP_DRC=y
CONFIG_COMP_KPB=y
CONFIG_COMP_TESTER=m
CONFIG_COMP_SRC_IPC4_FULL_MATRIX=y
CONFIG_COMP_UP_DOWN_MIXER=y
CONFIG_COMP_VOLUME_WINDOWS_FADE=y
Expand Down
1 change: 1 addition & 0 deletions app/configs/mtl/modules.conf
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ CONFIG_COMP_CROSSOVER=m
CONFIG_COMP_MULTIBAND_DRC=m
CONFIG_COMP_GOOGLE_CTC_AUDIO_PROCESSING=m
CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING=m
CONFIG_COMP_TESTER=m
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to have the common non-intel changes in one commit and these Intel board changes in a separate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@marcinszkudlinski Not a blocking thing, but this board file change is still in the same commit as the tester module implementation. So a comment would be nice if you just prefer not to do this.

2 changes: 2 additions & 0 deletions src/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,5 @@ rsource "library_manager/Kconfig"
rsource "debug/telemetry/Kconfig"

rsource "debug/debug_stream/Kconfig"

rsource "debug/tester/Kconfig"
1 change: 1 addition & 0 deletions src/audio/module_adapter/module_adapter_ipc4.c
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ int module_adapter_set_state(struct processing_module *mod, struct comp_dev *dev
{
return comp_set_state(dev, cmd);
}
EXPORT_SYMBOL(module_adapter_set_state);

int module_set_large_config(struct comp_dev *dev, uint32_t param_id, bool first_block,
bool last_block, uint32_t data_offset_size, const char *data)
Expand Down
60 changes: 60 additions & 0 deletions src/audio/sink_source_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,63 @@ int source_to_sink_copy(struct sof_source *source,
return 0;
}
EXPORT_SYMBOL(source_to_sink_copy);

int sink_fill_with_silence(struct sof_sink *sink, size_t size)
{
uint8_t *dst_ptr;
uint8_t *dst_begin;
uint8_t *dst_end;
size_t dst_size;
int ret;

if (!size)
return 0;
if (size > sink_get_free_size(sink))
return -ENOSPC;

ret = sink_get_buffer(sink, size, (void **)&dst_ptr, (void **)&dst_begin, &dst_size);
if (ret)
return ret;

dst_end = dst_begin + dst_size;
while (size) {
uint32_t dst_to_buf_overlap = (uintptr_t)dst_end - (uintptr_t)dst_ptr;
uint32_t to_fill = MIN(dst_to_buf_overlap, size);

ret = memset_s(dst_ptr, dst_to_buf_overlap, 0, to_fill);
assert(!ret);

size -= to_fill;
dst_ptr += to_fill;
if (to_fill == dst_to_buf_overlap)
dst_ptr = dst_begin;
}

sink_commit_buffer(sink, INT_MAX);
return 0;
}
EXPORT_SYMBOL(sink_fill_with_silence);

int source_drop_data(struct sof_source *source, size_t size)
{
uint8_t const *src_ptr;
uint8_t const *src_begin;
size_t src_size;
int ret;

if (!size)
return 0;
if (size > source_get_data_available(source))
return -EFBIG;

ret = source_get_data(source, size,
(void const **)&src_ptr,
(void const **)&src_begin,
&src_size);
if (ret)
return ret;

source_release_data(source, INT_MAX);
return 0;
}
EXPORT_SYMBOL(source_drop_data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

please place each EXPORT_SYMBOL() under respective function

Copy link
Collaborator

Choose a reason for hiding this comment

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

@marcinszkudlinski let's keep "unresolved" until actually resolved or at least explained why you the existing version is preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I see it has been moved, it is now at the end of source_drop_data function, isn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@marcinszkudlinski sorry for not explaining clearly. Every EXPORT_SYMBOL() should immediately follow the respective function. Like

void f1()
{
}
EXPORT_SYMBOL(f1);

void f2()
{
}
EXPORT_SYMBOL(f2);

etc. See https://github.com/thesofproject/sof/actions/runs/12891822335/job/35944645332?pr=9787 :

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
#87: FILE: src/audio/sink_source_utils.c:134:
+EXPORT_SYMBOL(sink_fill_with_silence);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confusing, because I fixed it and in the PR it is exactly as you wrote :)
...except one detail, I put the fix in wrong commit,
ok, will fix

4 changes: 4 additions & 0 deletions src/debug/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,8 @@ if(CONFIG_GDB_DEBUG)
add_subdirectory(gdb)
endif()

if(CONFIG_COMP_TESTER)
add_subdirectory(tester)
endif()

add_local_sources(sof panic.c)
2 changes: 2 additions & 0 deletions src/debug/tester/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
add_local_sources(tester.c)
add_local_sources(tester_dummy_test.c)
10 changes: 10 additions & 0 deletions src/debug/tester/Kconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# SPDX-License-Identifier: BSD-3-Clause

config COMP_TESTER
tristate "tester module"
default n
depends on IPC_MAJOR_4
help
Build a tester module. To be used in continuous
integration process for special test cases that
require a test code injected into the system itself
7 changes: 7 additions & 0 deletions src/debug/tester/llext/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Copyright (c) 2024 Intel Corporation.
# SPDX-License-Identifier: Apache-2.0

sof_llext_build("tester"
SOURCES ../tester.c
../tester_dummy_test.c
)
6 changes: 6 additions & 0 deletions src/debug/tester/llext/llext.toml.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#include <tools/rimage/config/platform.toml>
#define LOAD_TYPE "2"
#include "../tester.toml"

[module]
count = __COUNTER__
252 changes: 252 additions & 0 deletions src/debug/tester/tester.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,252 @@
//SPDX-License-Identifier: BSD-3-Clause
//
// Copyright(c) 2025 Intel Corporation. All rights reserved.
//
// Author: Marcin Szkudlinski <marcin.szkudlinski@linux.intel.com>

#include <stddef.h>
#include <stdint.h>
#include <stdbool.h>
#include <rtos/init.h>
#include <sof/lib/uuid.h>
#include <sof/audio/component.h>
#include <sof/audio/module_adapter/module/generic.h>
#include <sof/audio/sink_api.h>
#include <sof/audio/source_api.h>
#include <sof/audio/sink_source_utils.h>
#include "tester.h"
#include "tester_dummy_test.h"

/**
* Tester module is a framework for a runtime testing that need a special test code
* injected into the SOF system, i.e.
* - provide an extra load to CPU by running additional thread
* - execute some incorrect operations
*
* The idea is to allow using of special cases in testing like Continuous Integration or pre-release
* testing using a standard, production build.
*
* In order to have only one testing module (in meaning of a SOF module with separate UUID),
* a framework is introduced, where a test to be executed is selected by IPC parameter
*
* In production, module should be compiled as a loadable module, as it should not needed to be
* loaded and used on customers' boards.
* During developing, however, the module may be built in to keep debugging simpler
*
*/

LOG_MODULE_REGISTER(tester, CONFIG_SOF_LOG_LEVEL);

SOF_DEFINE_REG_UUID(tester);

DECLARE_TR_CTX(tester_tr, SOF_UUID(tester_uuid), LOG_LEVEL_INFO);
Copy link
Collaborator

Choose a reason for hiding this comment

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

since you only want this as a module, maybe add a check that CONFIG_COMP_TESTER_MODULE is defined somewhere:

#if !CONFIG_COMP_TESTER_MODULE
#error only modular
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want it to be modular only for production, but I still prefer to be able to compile it as built-in - it is easier to have one binary in case of debugging.


struct tester_init_config {
struct ipc4_base_module_cfg ipc4_cfg;
int32_t test_type;
} __attribute__((packed, aligned(4)));

static int tester_init(struct processing_module *mod)
{
struct module_data *md = &mod->priv;
struct comp_dev *dev = mod->dev;
struct module_config *cfg = &md->cfg;
size_t bs = cfg->size;
struct tester_module_data *cd = NULL;
int ret = 0;

if (bs != sizeof(struct tester_init_config)) {
comp_err(dev, "Invalid config");
return -EINVAL;
}

/* allocate ctx for test module in shared memory - to allow all non-standard operations
* without problems with cache
*/
cd = rzalloc(SOF_MEM_ZONE_SYS_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*cd));
if (!cd) {
comp_err(dev, "Out of memory");
return -ENOMEM;
}

struct tester_init_config *init_cfg = (struct tester_init_config *)cfg->init_data;

cd->test_case_type = init_cfg->test_type;
switch (cd->test_case_type) {
case TESTER_MODULE_CASE_DUMMY_TEST:
cd->tester_case_interface = &tester_interface_dummy_test;
break;

default:
comp_err(dev, "Invalid config, unknown test type");
rfree(cd);
return -EINVAL;
}

module_set_private_data(mod, cd);

if (cd->tester_case_interface->init)
ret = cd->tester_case_interface->init(mod, &cd->test_case_ctx);

if (ret) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: I'd move this under the if in line 88 above - ret can become non-zero only there.

module_set_private_data(mod, NULL);
rfree(cd);
}

return ret;
}

static int tester_prepare(struct processing_module *mod,
struct sof_source **sources, int num_of_sources,
struct sof_sink **sinks, int num_of_sinks)
{
struct tester_module_data *cd = module_get_private_data(mod);
int ret = 0;

if (cd->tester_case_interface->prepare)
ret = cd->tester_case_interface->prepare(cd->test_case_ctx, mod,
sources, num_of_sources,
sinks, num_of_sinks);
Copy link
Collaborator

Choose a reason for hiding this comment

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

since this pattern is repeated multiple times, how about

if (cd->tester_case_interface->x)
    return cd->tester_case_interface->x(...);

return 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

repeated, but not always - see "trigger" and "free". I believe this framework will evolve with needs etc. So I used same pattern everywhere


return ret;
}

int tester_set_configuration(struct processing_module *mod,
uint32_t config_id,
enum module_cfg_fragment_position pos, uint32_t data_offset_size,
const uint8_t *fragment, size_t fragment_size, uint8_t *response,
size_t response_size)
{
struct tester_module_data *cd = module_get_private_data(mod);
int ret = 0;

if (cd->tester_case_interface->set_configuration)
ret = cd->tester_case_interface->set_configuration(cd->test_case_ctx, mod,
config_id, pos, data_offset_size,
fragment, fragment_size,
response, response_size);

return ret;
}

static int tester_process(struct processing_module *mod,
struct sof_source **sources, int num_of_sources,
struct sof_sink **sinks, int num_of_sinks)
{
struct tester_module_data *cd = module_get_private_data(mod);
int ret = 0;
bool do_copy = true;

if (cd->tester_case_interface->process)
ret = cd->tester_case_interface->process(cd->test_case_ctx, mod,
sources, num_of_sources,
sinks, num_of_sinks, &do_copy);

if (!ret) {
size_t sink_free = sink_get_free_size(sinks[0]);
size_t source_avail = source_get_data_available(sources[0]);
size_t to_copy = MIN(sink_free, source_avail);

if (do_copy) {
/* copy data from input to output */
source_to_sink_copy(sources[0], sinks[0], true, to_copy);
} else {
/* drop data and generate silence */
source_drop_data(sources[0], to_copy);
sink_fill_with_silence(sinks[0], to_copy);
}
}

return ret;
}

static int tester_reset(struct processing_module *mod)
{
struct tester_module_data *cd = module_get_private_data(mod);
int ret = 0;

if (cd->tester_case_interface->reset)
ret = cd->tester_case_interface->reset(cd->test_case_ctx, mod);

return ret;
}

static int tester_free(struct processing_module *mod)
{
struct tester_module_data *cd = module_get_private_data(mod);
int ret = 0;

if (cd->tester_case_interface->free)
ret = cd->tester_case_interface->free(cd->test_case_ctx, mod);

rfree(cd);
module_set_private_data(mod, NULL);
return ret;
}

static int tester_bind(struct processing_module *mod, void *data)
{
struct tester_module_data *cd = module_get_private_data(mod);
int ret = 0;

if (cd->tester_case_interface->bind)
ret = cd->tester_case_interface->bind(cd->test_case_ctx, mod, data);

return ret;
}

static int tester_unbind(struct processing_module *mod, void *data)
{
struct tester_module_data *cd = module_get_private_data(mod);
int ret = 0;

if (cd->tester_case_interface->unbind)
ret = cd->tester_case_interface->unbind(cd->test_case_ctx, mod, data);

return ret;
}

static int tester_trigger(struct processing_module *mod, int cmd)
{
struct tester_module_data *cd = module_get_private_data(mod);
int ret = 0;

if (cd->tester_case_interface->trigger)
ret = cd->tester_case_interface->trigger(cd->test_case_ctx, mod, cmd);

if (!ret)
ret = module_adapter_set_state(mod, mod->dev, cmd);

return ret;
}

static const struct module_interface tester_interface = {
.init = tester_init,
.prepare = tester_prepare,
.set_configuration = tester_set_configuration,
.process = tester_process,
.reset = tester_reset,
.free = tester_free,
.bind = tester_bind,
.unbind = tester_unbind,
.trigger = tester_trigger
};

DECLARE_MODULE_ADAPTER(tester_interface, tester_uuid, tester_tr);
SOF_MODULE_INIT(tester, sys_comp_module_tester_interface_init);

#if CONFIG_COMP_TESTER_MODULE
/* modular: llext dynamic link */

#include <module/module/api_ver.h>
#include <module/module/llext.h>
#include <rimage/sof/user/manifest.h>

SOF_LLEXT_MOD_ENTRY(tester, &tester_interface);

static const struct sof_man_module_manifest mod_manifest __section(".module") __used =
SOF_LLEXT_MODULE_MANIFEST("TESTER", tester_llext_entry, 1, SOF_REG_UUID(tester), 40);

SOF_LLEXT_BUILDINFO;

#endif
Loading
Loading