Debug module framework #9787

marcinszkudlinski · 2025-01-20T19:08:14Z

Tester module is a framework for a runtime testing
that need a special internal code i.e. provide an extra
load to CPU by running additional thread, etc.

The idea is to allow testing of special cases
using a standard "production" build. The module should
be compiled as a loadable module only, as it is not needed
in any real production case.

In order to keep 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

lyakh · 2025-01-21T08:12:40Z

src/audio/sink_source_utils.c

 EXPORT_SYMBOL(source_to_sink_copy);
+EXPORT_SYMBOL(source_drop_data);


please place each EXPORT_SYMBOL() under respective function

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

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

@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);

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

src/audio/module_adapter/module_adapter.c

lyakh · 2025-01-21T08:23:28Z

src/debug/tester/tester.c

+
+SOF_DEFINE_REG_UUID(tester);
+
+DECLARE_TR_CTX(tester_tr, SOF_UUID(tester_uuid), LOG_LEVEL_INFO);


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

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.

lyakh · 2025-01-21T08:25:47Z

src/debug/tester/tester.c

+
+	if (bs != sizeof(struct tester_init_config))
+	{
+		comp_err(dev, "tester_init(): Invalid config");


please drop all function names from logs

lyakh · 2025-01-21T08:28:42Z

src/debug/tester/tester.c

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


since this pattern is repeated multiple times, how about

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

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

src/debug/tester/tester.c

src/debug/tester/tester_dummy_test.c

zephyr/CMakeLists.txt

dbaluta · 2025-01-21T11:36:42Z

src/audio/sink_source_utils.c

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


I would say EINVAL here.

it is the same as in source_to_sink_copy - NO_SPACE error if somebody wants to copy/fill more than available space in destination

src/audio/sink_source_utils.c

app/boards/intel_adsp_ace15_mtpm.conf

src/audio/module_adapter/module_adapter.c

dbaluta · 2025-01-21T11:44:59Z

Do you also have a topology file for this? It is not yet clear to me how the component is created and used.

marcinszkudlinski · 2025-01-21T14:42:34Z

@dbaluta in internal CI test. No topology needed.

marcinszkudlinski · 2025-01-21T14:45:03Z

I think you can export this into a separate patch, as it is not related to this change.

it is, the symbol is needed now in a loadable module. Anyway, separate commit does not hurt

lgirdwood

@marcinszkudlinski what will the flow be to use the tester ? i.e. how will a developer enable tester and how will it be inserted into pipelines ?

src/debug/tester/tester.c

kv2019i

This seems like a cool idea! A few notes inline about how this is explained/named. I think having fake/test/synthetic modules in audio frameworks is a widely used and well understood concept, so I think the descriptions could be in simpler language.

I think the big design question is do we lumb all possible test modules in single one, or do separate modules per type of test (e.g. audio-error-injector could be separate from load-generation). If there's any chance of the latter, then I'd stop for a second to think about naming as "tester" is very generic.

No blockers in the end though.

kv2019i · 2025-01-21T19:10:19Z

app/boards/intel_adsp_ace15_mtpm.conf

@@ -11,6 +11,7 @@ CONFIG_COMP_CHAIN_DMA=y
 CONFIG_COMP_CROSSOVER=y


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.

@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.

app/configs/mtl/modules.conf

kv2019i · 2025-01-21T19:12:28Z

src/debug/tester/Kconfig

+	default n
+	depends on IPC_MAJOR_4
+	help
+	  Select for loadable module for testing


Given the very generic name, and a new type of module, I think this warrants a bit more text to describe (i.e. the text you have in the git commti).

And here "loadable" confuses me a bit. Any module can be loadable, so isn't this just a test module.

kv2019i · 2025-01-21T19:13:12Z

src/debug/tester/tester.c

+#include "tester_dummy_test.h"
+
+/**
+ * Tester module is a framework for a runtime testing that need a special internal code


"special internal code" -> "test code" ?

Add "sink_fill_with_silence" and "source_drop_data" tools for sink/source. The tools will respectively drop given amount of data from source or fill sink witj silence. To be used when a module doesn't want to process data at the moment, but still needs to consume data at input and provide something at output Signed-off-by: Marcin Szkudlinski <[email protected]>

module_adapter_set_state will be needed in loadable debug module framework Signed-off-by: Marcin Szkudlinski <[email protected]>

Tester module is a framework for a runtime testing that need a special internal code i.e. provide an extra load to CPU by running additional thread, etc. The idea is to allow testing of special cases using a standard "production" build. The module should be compiled as a loadable module only, as it is not needed in any real production case. In order to keep 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 Signed-off-by: Marcin Szkudlinski <[email protected]>

the debug module is intended to be a place for a test code for continuous integration testing, performed on a standard "production" build The test code, however, should not be loaded on any of the target platforms, therefore debug module is compiled as a LLEXT loadable Signed-off-by: Marcin Szkudlinski <[email protected]>

lyakh · 2025-01-24T07:34:53Z

src/debug/tester/tester.c

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


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

lyakh · 2025-01-24T07:36:56Z

uuid-registry.txt

@@ -54,6 +54,7 @@ e87bf747-e574-4362-bf2cc015d2859dd9 clkdrv_mt8196
 c2b00d27-ffbc-4150-a51a245c79c5e54b dai
 06711c94-d37d-4a76-b302bbf6944fdd2b dai_lib
 b809efaf-5681-42b1-9ed604bb012dd384 dcblock
+08aeb4ff-7f68-4c71-86c3c842b4262898 tester


these seem to be alphabetically sorted by the name, please move

marcinszkudlinski requested review from kv2019i, lyakh, iuliana-prodan, dbaluta, abonislawski, ranj063, jxstelter, lgirdwood, plbossart, mmaka1 and lbetlej as code owners January 20, 2025 19:08

dbaluta changed the title ~~Debug module farmework~~ Debug module framework Jan 21, 2025

lyakh reviewed Jan 21, 2025

View reviewed changes

dbaluta reviewed Jan 21, 2025

View reviewed changes

marcinszkudlinski force-pushed the debug_module_farmework branch 3 times, most recently from 4f74e6f to 06b7610 Compare January 21, 2025 16:51

lgirdwood reviewed Jan 21, 2025

View reviewed changes

src/debug/tester/tester.c Show resolved Hide resolved

kv2019i reviewed Jan 21, 2025

View reviewed changes

marcinszkudlinski added 4 commits January 23, 2025 12:13

mod: export module_adapter_set_state to lext

c27dcf4

module_adapter_set_state will be needed in loadable debug module framework Signed-off-by: Marcin Szkudlinski <[email protected]>

marcinszkudlinski force-pushed the debug_module_farmework branch from 06b7610 to a778a7a Compare January 23, 2025 11:18

lyakh reviewed Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debug module framework #9787

Debug module framework #9787

marcinszkudlinski commented Jan 20, 2025

lyakh Jan 21, 2025

lyakh Jan 22, 2025

marcinszkudlinski Jan 22, 2025

lyakh Jan 23, 2025

marcinszkudlinski Jan 23, 2025

lyakh Jan 21, 2025

marcinszkudlinski Jan 21, 2025

lyakh Jan 21, 2025

lyakh Jan 21, 2025

marcinszkudlinski Jan 21, 2025

dbaluta Jan 21, 2025

marcinszkudlinski Jan 21, 2025

dbaluta commented Jan 21, 2025

marcinszkudlinski commented Jan 21, 2025

marcinszkudlinski commented Jan 21, 2025

lgirdwood left a comment

kv2019i left a comment

kv2019i Jan 21, 2025

dbaluta Jan 22, 2025 •

edited

Loading

kv2019i Jan 21, 2025

kv2019i Jan 21, 2025

lyakh Jan 24, 2025

lyakh Jan 24, 2025

		EXPORT_SYMBOL(source_to_sink_copy);
		EXPORT_SYMBOL(source_drop_data);


		SOF_DEFINE_REG_UUID(tester);

		DECLARE_TR_CTX(tester_tr, SOF_UUID(tester_uuid), LOG_LEVEL_INFO);

		@@ -11,6 +11,7 @@ CONFIG_COMP_CHAIN_DMA=y
		CONFIG_COMP_CROSSOVER=y

Debug module framework #9787

Are you sure you want to change the base?

Debug module framework #9787

Conversation

marcinszkudlinski commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaluta commented Jan 21, 2025

marcinszkudlinski commented Jan 21, 2025

marcinszkudlinski commented Jan 21, 2025

lgirdwood left a comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaluta Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaluta Jan 22, 2025 •

edited

Loading