Tools: Testbench: xt-testbench #6513

singalsu · 2022-11-01T12:29:23Z

Now ready. see commit texts for description.

tools/testbench/makefile-xcc

src/arch/host/CMakeLists.txt

src/arch/host/include/arch/debug/backtrace.h

src/platform/library/schedule/ll_schedule.c

btian1 · 2022-11-04T02:24:56Z

one more question is: where is the entry for this testbench?

lgirdwood · 2022-11-08T13:37:58Z

one more question is: where is the entry for this testbench?

src/tools/testbench

src/audio/asrc/asrc.c

src/arch/host/include/arch/debug/backtrace.h

src/audio/module_adapter/CMakeLists.txt

tools/testbench/include/alsa/sound/uapi/asoc.h

tools/testbench/makefile-xcc

tools/testbench/CMakeLists.txt

tools/tplg_parser/tokens.c

tools/test/audio/comp_run.sh

scripts/rebuild-xt-testbench.sh

src/arch/host/CMakeLists.txt

src/arch/host/include/arch/debug/backtrace.h

tools/test/audio/comp_run.sh

src/audio/module_adapter/CMakeLists.txt

singalsu · 2023-03-08T08:19:34Z

Thanks @marc-hb the file DAI may be missing some DAI operation that fuzzer expects.

lyakh

Do I understand it correctly, that we currently can run testbench tests on the host multi-threaded and after this PR we won't be able to do that any more? Cannot we keep both modes?

src/include/sof/schedule/ll_schedule.h

src/include/sof/audio/component.h

tools/testbench/topology.c

tools/tplg_parser/tokens.c

src/arch/host/include/arch/atomic.h

singalsu · 2023-03-08T10:19:56Z

Do I understand it correctly, that we currently can run testbench tests on the host multi-threaded and after this PR we won't be able to do that any more? Cannot we keep both modes?

Yep, this is what it does... @lgirdwood can we restore the multi-threading with your work for alsa plugin so we could have both?

My idea is that I would get the improvements for topology parser and IPC simulation to xt-testbench from the plugin work. So this would be an intermediate step. Or wait to have both somehow.

sys-pt1s · 2023-04-06T10:19:28Z

Can one of the admins verify this patch?

lyakh · 2023-04-13T07:28:30Z

tools/testbench/testbench.c

+#if defined __XCC__
+	printf("Total execution cycles: %lu\n\n", (unsigned long)delta);
+#else
+	printf("Total execution time: %lu us, %.2f x realtime\n\n",


shouldn't double be %lf?

Double is overkill for this, I'll change the equation to float. The table https://cplusplus.com/reference/cstdio/printf/ suggests that L would be used for long double, while l seems to be for integers unless I'm misunderstanding it.

singalsu

I've now addressed review feedback, thanks for the comments!

singalsu · 2023-04-14T16:31:58Z

tools/tplg_parser/include/tplg_parser/topology.h

@@ -86,7 +86,8 @@ struct tplg_context {
 	ptr = (struct snd_soc_tplg_hdr *)(ctx->tplg_base + ctx->tplg_offset);	\
 	if (ptr->size != sizeof(*ptr)) {					\
 		printf("%s %d hdr size mismatch 0x%x:0x%lx at offset %ld\n",	\
-				__func__, __LINE__, ptr->size, sizeof(*ptr),	\
+				__func__, __LINE__, ptr->size,			\
+				(unsigned long)sizeof(*ptr),			\


Changing 0x%lx to 0x%zx seems be OK for both gcc and xcc, I'll update the patch with it.

singalsu · 2023-04-14T16:33:52Z

tools/testbench/testbench.c

@@ -637,7 +651,11 @@ static int pipline_test(struct testbench_prm *tp)
 				dp_count, err);
 			break;
 		}
+#if defined __XCC__


Yep, I've added functions to get time and cycles. The patch now also computes MCPS with file read/write consumption excluded.

singalsu · 2023-04-14T16:43:47Z

tools/testbench/testbench.c

-	printf("Total execution time: %zu us, %.2f x realtime\n\n",
-	       delta, (double)((double)n_out / tp->channels_out / tp->fs_out) * 1000000 / delta);
+#if defined __XCC__
+	printf("Total execution cycles: %lu\n\n", (unsigned long)delta);


Thanks, yes that works!

singalsu · 2023-04-14T16:51:35Z

tools/testbench/testbench.c

+#if defined __XCC__
+	printf("Total execution cycles: %lu\n\n", (unsigned long)delta);
+#else
+	printf("Total execution time: %lu us, %.2f x realtime\n\n",


Double is overkill for this, I'll change the equation to float. The table https://cplusplus.com/reference/cstdio/printf/ suggests that L would be used for long double, while l seems to be for integers unless I'm misunderstanding it.

singalsu · 2023-04-14T17:08:00Z

Now run of e.g. >> process_test('volume', 32, 32, 48000, 0, 'xt-run'); gives test result for MCPS

Input sample (frame) count: 326400 (163200)
Output sample (frame) count: 326400 (163200)
Total execution cycles: 18810961
File component cycles: 11272232
Pipeline cycles: 7538729
Pipeline MCPS: 2.22

marc-hb · 2023-04-14T17:32:34Z

tools/testbench/testbench.c

@@ -602,10 +626,10 @@ static int pipline_test(struct testbench_prm *tp)
 	struct tplg_context ctx;
 	struct timespec ts;
 	struct timespec td0, td1;
+	uint64_t delta_t;


long long for consistency?

Thanks, yes changed now!

marc-hb

Caught one $TPLGFN typo, otherwise shell script changes LGTM

marc-hb · 2023-04-17T23:24:54Z

tools/test/audio/comp_run.sh

 # Use topology from component test topologies
 INFMT=s${BITS_IN}le
 OUTFMT=s${BITS_OUT}le
 TPLGFN=test-${DIRECTION}-ssp5-mclk-0-I2S-${COMP}-${INFMT}-${OUTFMT}-48k-24576k-codec.tplg
 TPLG=${TPLG_DIR}/${TPLGFN}
+[ -f "$TPLG" ] || {
+    echo
+    echo "Error: topology $TPLGFN does not exist."


typo

Suggested change

echo "Error: topology $TPLGFN does not exist."

echo "Error: topology $TPLG does not exist."

Fixed, the previous print was without path. Better this way.

marc-hb · 2023-04-17T23:25:28Z

tools/test/audio/comp_run.sh

+    XTRUN_CMD=$XTENSA_PATH/$XTRUN
+    if $VALGRIND; then
+	>&2 printf "WARNING: Ignoring VALGRIND with xt-run\n"
+	VALGRIND=false


Same, I have no idea what's wrong with this indent. My emacs bash mode indents this way.

Mix of tabs and spaces again?

tools/test/audio/comp_run.sh

marc-hb · 2023-04-17T23:26:29Z

scripts/rebuild-testbench.sh

+    test -n "${XTENSA_TOOLS_VERSION}" ||
+	die "Illegal platform $BUILD_PLATFORM, no XTENSA_TOOLS_VERSION found.\n"
+    test -n "${XTENSA_CORE}" ||
+	die "Illegal platform $BUILD_PLATFORM, no XTENSA_CORE found.\n"


Sorry, I have no idea what's wrong with indent. Help?

Spaces on one line and tabs on the next feels fishy, even for shell scripts.

It's unfortunate that we have some files with tabs and other files with spaces in the same project but I agree 100% with @paulstelian97 : let's please not cross the line where we have a mix of tabs and spaces in the same file!

So @singalsu please find the shortcut in your editor that lets you quickly switch between tabs and spaces like the rest of us :-)

PS: I can't resist sorry: this sort of time-consuming discussion is exactly why tabs suck. Off-topic.

The file is already without my patch a mix of tabs and spaces. I can convert them all to spaces then.

M-x untabify is now done.

I checked and there are only 5 tabs right now. Feel free to convert them to spaces or to leave them alone, as long as you don't add any it's fine by me.

tools/testbench/testbench.c

paulstelian97

Modulo that one comment this looks good (and I can live with that one too).

btian1 · 2023-04-26T08:14:31Z

tools/testbench/common_test.c

+void tb_getcycles(uint64_t *cycles)
+{
+#if defined __XCC__
+	*cycles = XT_RSR_CCOUNT();


it may be a int32 usage, no need int64.
https://elixir.bootlin.com/zephyr/v1.7.0-rc2/source/drivers/timer/xtensa_sys_timer.c#L41

int32 should be enough, max should be set to: 400,000?

I'd keep this as 64 bits. With 400 MHz 32 bit counter overflows at 10.7s. Other Xtensas can be over 1 GHz clocked and we can support also other processor simulators too in the future.

cycle should be a wrap data, once it go to max, then will go back to zero, so there is another issue with the code,
please refer below:

if (cycles1 > cycles0) diff = cycles1 - cycles0; else diff = UINT32_MAX - cycles0 + cycles1;

it is not a always increase data.

so there is another issue with the code,

What was the first issue?

cycle should be a wrap data, once it go to max, then will go back to zero,

Yes and that's OK because unsigned integers (unlike signed integers) are guaranteed to wrap around by the C standard

import numpy as np np.uint32(5) - np.uint32(0xffffffff) 6

https://www.acepace.net/integerQuiz/

btian1 · 2023-04-26T08:17:30Z

tools/testbench/include/testbench/file.h

@@ -29,14 +31,15 @@ enum file_format {

 /* file component state */
 struct file_state {
-	char *fn;
+	uint64_t cycles_count;


int32 should be ok, since it is delta between one module. no way to exceed int32

Yes, it's true a single file copy should not consume over 32 bit worth of cycles but having this 64 bits avoid type casts. The overhead from this is small and it makes this more future proof, so I'd keep it as is. Unless you or others really want me to change this to smaller size.

As far as I understand this code will only ever run on a GHz 64bits CPU so I don't see what difference 32 bits would make.

Mixing signed and unsigned is fraught with peril and signed overflow is undefined so unsigned is clearly better here.

kv2019i

Looks great @singalsu !

This patch avoids the build errors. Most of the issues are from different types for formatted printing in gcc vs. xt-xcc. The "__attribute__ ((fallthrough));" is not supported in xt-xcc. The xtensa C library does not have clock_gettime() so it is only left out from build. The cycles count and MCPS is printed instead. The include of dlfcn.h is not needed since the testbench no more has dynamic libraries. Structs within structs need to be initialized to zero in xt-xcc with multiple brackets. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch adds to rebuild-testbench option -x <platform> that can be used to build testbench for xt-run execution. The enhanced script reuses native testbench build but with CC, LD, LDFLAGS, etc. defines to use the xt-xcc compiler for build. Currently TGL (HiFi3) is the only supported platform. More will be added later. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch adds to process_test() sixth argument to run the tests with xt-run environment with argument set to 'xt-run' or 'xt-run --turbo'. The set and print of LD_LIBRARY_PATH is no more needed with static testbench version. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

No script changes, just unify shell script style to be with indents with spaces instead of both tabs and spaces. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu · 2023-05-04T12:39:25Z

I just rebased this PR and added a commit with only tabs to spaces changes to comp_run.sh.

kv2019i · 2023-05-04T15:07:41Z

Style checkpatch warning relates to long string literals -> this is ok

One unrelated fail in https://sof-ci.01.org/sofpr/PR6513/build6991/devicetest/index.html

Proceeding with merge.

btian1 self-requested a review November 2, 2022 01:34