Tools: Testbench: Add IPC4 support #9483

singalsu · 2024-09-17T17:37:14Z

This PR replaces #9025. The IPC3 and IPC4 testbench versions can be now build from same sources by selecting CONFIG_IPC_MAJOR_3=y or CONFIG_IPC_MAJOR_4=n.

The IPC4 testbench version can use e.g. topologies sof-hda-benchmark-comp<16/24/32>.tplg from tools/build_tools/topology/topology2/development.

The IPC3 testbench version is similar as before but cleaned up a bit.

tools/testbench/CMakeLists.txt

tools/testbench/topology_ipc4.c

lgirdwood

Looks good except for the variable array build fix patch.

src/ipc/ipc4/handler.c

src/include/ipc4/header.h

src/ipc/ipc4/handler.c

tools/testbench/common_test.c

tools/testbench/common_test_ipc4.c

cujomalainey · 2024-09-18T20:13:19Z

tools/testbench/common_test_ipc4.c

+		return -EINVAL;
+	}
+
+	/* TODO 44.1 kHz like rates */


make a bug for this, orphaned todos in code are useless

Ah, that's something I have not yet been able to test, 44.1 kHz capability is very useful so it won't be orphaned. Also seems that copier in/out formats parsing is not complete, only 1st format is working (S32_LE) so even basic S16_LE fails with the topologies I used.

This and next one remain still opens in the version of today. I will address them later when I find suitable topologies to use and have found fix for copier formats discovery.

cujomalainey · 2024-09-18T20:14:05Z

tools/testbench/common_test_ipc4.c

+	tp->config[0].format = tp->frame_fmt;
+	tp->period_size = 2 * krate;
+
+	/* TODO used for what? determine from topology and enabled pipelines if need */


@lgirdwood do you know what this is for?

It's a feature from SOF plugin, I'm not sure I need this. Maybe need if I with testbench -pA,B,C,.. command line switch select pipelines those are not connected to 1st PCM. I need to test with other topologies, e.g. sof-hda-generic that has more pipelines.

@ranj063 do we still need this as an open or can we remove the comment/fix ?

@singalsu with the plugin, the pipelines are determined based on just the PCM ID for the device that we want to start playback/capture on. I suppose this is not needed for the testbench because we only have 1 pipeline in your test topology and you want to set up all the widgets anyway.

Thanks for explanation! I'll keep it for now for possible more complex scenarios. But I will need to update the PCM ID based on the command line specified pipelines.

@singalsu maybe add that explanation from @ranj063 as a comment here?

cujomalainey

approve for google owned code, unresolved comments are nit

tools/testbench/README.md

singalsu · 2024-09-20T08:34:40Z

Some pause/resume fails with https://sof-ci.01.org/sofpr/PR9483/build8235/devicetest/index.html and https://sof-ci.01.org/sofpr/PR9483/build8233/devicetest/index.html . Unlikely related. But I'll still fix that typo and add to README.md how to run debugger (ddd) with xtensa xt-gdb target.

kv2019i

Excellent stuff @singalsu ! Only very minor comments, but if you do a update, please take a look. You can keep the one valgrind find in the series, given it's only one commit in the series, others seem to be strictly related to enabling IPC4 testbench.

kv2019i · 2024-09-20T09:28:00Z

src/ipc/ipc4/handler.c

@@ -1615,7 +1615,7 @@ void ipc_cmd(struct ipc_cmd_hdr *_hdr)
 	/* FW sends an ipc message to host if request bit is clear */
 	if (in->primary.r.rsp == SOF_IPC4_MESSAGE_DIR_MSG_REQUEST) {
 		struct ipc *ipc = ipc_get();
-		struct ipc4_message_reply reply;
+		struct ipc4_message_reply reply = {{0}};


This kind of stuff could have been in a separate PR btw.

I can take it to separate if this review pass takes longer than few days.

kv2019i · 2024-09-20T09:33:03Z

tools/testbench/topology_ipc4.c

@@ -0,0 +1,1304 @@
+// SPDX-License-Identifier: BSD-3-Clause
+//
+// Copyright(c) 2018-2024 Intel Corporation. All rights reserved.


kv2019i · 2024-09-20T09:34:15Z

tools/testbench/topology_ipc4.c

+	memcpy(msg, module_init, sizeof(*module_init));
+	memcpy(msg + sizeof(*module_init), comp_info->ipc_payload, comp_info->ipc_size);
+
+	// TODO


Ah, it's from time when I didn't have IPC simulation, just remove these.

kv2019i · 2024-09-20T09:34:51Z

tools/testbench/topology_ipc4.c

+	msg.primary.r.instance_id = pipe_info->instance_id;
+	msg.primary.r.ppl_mem_size = pipe_info->mem_usage;
+
+	// TODO


Ditto, TODO what? And why C++ comments?

Same, I'm lazy to type, and // is easy to search :)

kv2019i · 2024-09-20T09:36:12Z

tools/testbench/topology_ipc4.c

+out:
+	/* free all data */
+
+	// TODO: Check this


Hmm, maybe a bit more verbose if we leave this to codebase as a TODO..?

I think this is remaining from some old code change try and I've forgot to clean it up. It's clear that ctx->tplg_base should be freed. It was was also missing from above load widget.

kv2019i · 2024-09-20T09:37:17Z

tools/testbench/README.md

+ * Much faster than real-time execution in native build, e.g. x86 on
+   Linux for efficient validation usage.
+ * With xtensa DSP build offers cycles accurate simulated environment
+   execution. And also with options for similation speed vs. model


typo: similation

Warnings are errors in testbench build. This change avoids error: sof/src/ipc/ipc4/helper.c:761:19: error: unused function 'process_dma_index' [-Werror,-Wunused-function] Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This addition allows to load file components for DAI and host copier replacement for testbench. DC blocker is added as process component to allow testing of this component as example. Support for other processing components will need a new solution to pass the UUIDs over IPC messages instead of the 8-bit index value. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Components initialize in testbench needs these headers. Adding these allows load of to the components when found in topology. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

The crossover_init_output_pins() function is changed to static. This change avoids build error: sof/src/audio/crossover/crossover_ipc4.c:32:5: error: no previous prototype for ‘crossover_init_output_pins’ [-Werror=missing-prototypes] Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

The build of testbench IPC4 with xt-clang fails with this error: sof/src/audio/tdfb/tdfb_ipc4.c:31:37: error: field 'module_data' with variable sized type 'struct sof_ipc4_notify_module_data' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] sof/src/audio/tdfb/tdfb_ipc4.c:32:38: error: field 'control_msg' with variable sized type 'struct sof_ipc4_control_msg_payload' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] The issue is fixed with use of struct pointers to IPC message msg_module_data and msg_payload to prepare the message template. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

The remove of variables in1 and out1 in peakvolume HiFi4 function vol_s16_to_s16() avoids testbench IPC4 build error: sof/src/audio/volume/volume_hifi4_with_peakvol.c:406:10: error: unused variable 'in1' [-Werror,-Wunused-variable] sof/src/audio/volume/volume_hifi4_with_peakvol.c:407:10: error: unused variable 'out1' [-Werror,-Wunused-variable] Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This change prepares to add IPC4 support to testbench. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Prefix macros with TB and add TESTBENCH to headers single time include control macros. Especially ifndef _TRACE_H is in risk to conflict with possible other headers. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

lyakh · 2024-09-24T06:40:58Z

tools/testbench/common_test_ipc4.c

+
+	list_for_item_safe(item, _item, &tp->pcm_list) {
+		pcm_info = container_of(item, struct tplg_pcm_info, item);
+		list_item_del(&pcm_info->item);


I think you can just do list_item_del(item) but the above is essentially the same

lyakh · 2024-09-24T06:42:09Z

tools/testbench/common_test.c

+	schedule_free(0);
+	schedulers = arch_schedulers_get();
+	list_for_item_safe(slist, _slist, &(*schedulers)->list) {
+		sch = container_of(slist, struct schedule_data, list);


same here actually - should this be removed from the list?

lyakh · 2024-09-24T06:42:58Z

tools/testbench/common_test_ipc3.c

+	int err;
+
+	/* remove the components for this pipeline */
+	list_for_item_safe(clist, temp, &sof_get()->ipc->comp_list) {


delete list item?

This list item can't be deleted yet.

lyakh · 2024-09-24T06:44:40Z

tools/testbench/common_test_ipc3.c

+	struct file_comp_data *fcd;
+
+	/* set the test limits for this pipeline */
+	list_for_item_safe(clist, temp, &sof_get()->ipc->comp_list) {


here no list items are removed or moved or added, so it doesn't need to be "safe?"

The helper functions are moved from testbench.c to common_test.c and common_test_ipc3.c as preparation to add IPC4 support. The file components are looked up in test setup to arrays to ease finding them in test run and control ending the test. The parse string for command getopt() is fixed to match the supported options. The testbench parameter struct is changed to dynamically allocated and zeroed by calloc(). It also avoids issues found with valgrind about uninitialized. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This change avoids a segfault. Topologies may contain non-supported PCMs such as HDMI and if pipelines are not restricted with -p A,B,C,.. option file might get set up without filename. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch adds topology parsing and common functions versions for IPC4. Due to dai_get_init_delay_ms() implementation in IPC4 build the file component is changed internally to copier to provide the DAI data struct. The change is common for both IPC3 and IPC4 though copier is not usually used with IPC3 systems. Since it works the same solution is used. The file state retrieve is changed because the file component data is placed deeper into the structures. Due to IPC4 scheduling of pipelines the file component is added a timeout. A file component sets timeout status if there has been three copy operations with no data to process. The timeout and EOF are used to end cleanly the test run. The library_defconfig still has CONFIG_IPC_MAJOR_4=n. The add of build type select to scripts/rebuild-testbench.sh is further work. Also the IPC4 testbench in this state is not well usable with only one component supported as process component and without byte control set up algorithms. Test run with DC blocker is possible this way: tools/testbench/build_testbench/install/bin/testbench -r 48000 -R 48000 -c 2 -n 2 -b S32_LE -p 1,2 -t tools/build_tools/topology/topology2/development/ sof-hda-benchmark-dcblock32.tplg -i in.raw -o out.raw Also sof-hda-benchmark-gain32.tplg can be run. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

The utils is a better description of the purpose of the functions in these files. There's no change to functionality. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This text add helps with various usages for testbench. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu · 2024-09-24T11:58:47Z

tools/testbench/common_test.c

+	schedule_free(0);
+	schedulers = arch_schedulers_get();
+	list_for_item_safe(slist, _slist, &(*schedulers)->list) {
+		sch = container_of(slist, struct schedule_data, list);


singalsu · 2024-09-24T12:02:17Z

tools/testbench/file.c

+	} else {
+		fprintf(stderr, "error: no filename set\n");
+		goto error;
+	}


Note for reviewers, this is a new commit and change that I added after I saw that I could still segfault this.

singalsu · 2024-09-24T12:04:46Z

tools/testbench/common_test_ipc4.c

+			ret = tb_set_up_widget(tp, current_comp_info);
+			if (ret < 0)
+				return ret;
+		}


I tried this change for this and next function but I got problems with run of sof-hda-benchmark-dcblock32.tplg with all four pipelines.

singalsu · 2024-09-24T12:12:48Z

tools/testbench/common_test_ipc4.c

+		if (route_info->source != current_comp_info)
+			continue;
+
+		/* Widgets will be freed when the pipeline is deleted, so just unbind modules */


I prefer to leave these as is and copy later from sof plugin new versions if these can be improved. Right now the logic of these doesn't feel clear to me.

singalsu · 2024-09-24T12:17:17Z

@lyakh @kv2019i Thanks for review, I hope I have now addressed or at least tried all your suggestions for improvements, so proposing a new round now. This versions passes my ipc3 and ipc4 check runs. I submit after this a PR that allows load other components than now (host, dai, pga, dcblock) with UUID based component load.

lyakh

the QB failure needs to be checked

singalsu · 2024-09-25T08:02:16Z

the QB failure needs to be checked

Yep, updownmixer on PTL FPGA fails, " pipe: pipeline_copy: pipe:0 0x0 pipeline_copy(): ret = -61, start->comp.id = 65540, dir = 1", in https://sof-ci.01.org/sof-pr-viewer/#/build/PR9483/build14242452 . I'll retry.

singalsu · 2024-09-25T08:02:28Z

SOFCI TEST

singalsu · 2024-09-26T06:56:04Z

Some pause/resume issues in https://sof-ci.01.org/sofpr/PR9483/build8324/devicetest/index.html but unlikely related to this.

src/audio/tdfb/tdfb_ipc4.c

singalsu mentioned this pull request Sep 17, 2024

[WIP][Do no review] Tools: Testbech: Add IPC4 support #9025

Closed

singalsu force-pushed the testbench_ipc4_take2 branch from 63ee596 to b76d285 Compare September 18, 2024 06:49

singalsu commented Sep 18, 2024

View reviewed changes

tools/testbench/CMakeLists.txt Outdated Show resolved Hide resolved

singalsu marked this pull request as ready for review September 18, 2024 07:00

singalsu requested review from akloniex, ranj063, a team, RanderWang, marcinszkudlinski, pblaszko, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners September 18, 2024 07:00

singalsu commented Sep 18, 2024

View reviewed changes

tools/testbench/topology_ipc4.c Outdated Show resolved Hide resolved

tools/testbench/topology_ipc4.c Outdated Show resolved Hide resolved

tools/testbench/topology_ipc4.c Outdated Show resolved Hide resolved

singalsu force-pushed the testbench_ipc4_take2 branch from b76d285 to e84305a Compare September 18, 2024 09:59

singalsu requested a review from lyakh September 18, 2024 10:04

lgirdwood reviewed Sep 18, 2024

View reviewed changes

src/ipc/ipc4/handler.c Outdated Show resolved Hide resolved

src/include/ipc4/header.h Outdated Show resolved Hide resolved

singalsu marked this pull request as draft September 18, 2024 16:45

cujomalainey reviewed Sep 18, 2024

View reviewed changes

singalsu force-pushed the testbench_ipc4_take2 branch from e84305a to 520519f Compare September 19, 2024 10:33

cujomalainey approved these changes Sep 19, 2024

View reviewed changes

singalsu marked this pull request as ready for review September 19, 2024 16:11

singalsu requested a review from lgirdwood September 19, 2024 16:12

singalsu force-pushed the testbench_ipc4_take2 branch from ed9f44b to 69c30da Compare September 19, 2024 16:16

singalsu commented Sep 19, 2024

View reviewed changes

tools/testbench/README.md Outdated Show resolved Hide resolved

kv2019i reviewed Sep 20, 2024

View reviewed changes

singalsu added 8 commits September 23, 2024 11:43

Audio: Component: Add module headers to component.h for testbench

3c3ebc4

Components initialize in testbench needs these headers. Adding these allows load of to the components when found in topology. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Tools: Testbench: Rename files with IPC3

e3f13a8

This change prepares to add IPC4 support to testbench. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Tools: Testbench: Clean up header files

c7b090a

Prefix macros with TB and add TESTBENCH to headers single time include control macros. Especially ifndef _TRACE_H is in risk to conflict with possible other headers. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu force-pushed the testbench_ipc4_take2 branch 2 times, most recently from 43f347b to d7af1af Compare September 23, 2024 16:56

lyakh reviewed Sep 24, 2024

View reviewed changes

singalsu added 5 commits September 24, 2024 14:45

Tools: Testbench: Rename common_test C source files to utils

088893e

The utils is a better description of the purpose of the functions in these files. There's no change to functionality. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Tools: Testbench: Add README.md text

2a0284f

This text add helps with various usages for testbench. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu force-pushed the testbench_ipc4_take2 branch from d7af1af to 2a0284f Compare September 24, 2024 11:49

singalsu commented Sep 24, 2024

View reviewed changes

singalsu requested review from lyakh and kv2019i September 24, 2024 12:14

lyakh approved these changes Sep 25, 2024

View reviewed changes

lgirdwood reviewed Sep 26, 2024

View reviewed changes

src/audio/tdfb/tdfb_ipc4.c Show resolved Hide resolved

kv2019i approved these changes Sep 27, 2024

View reviewed changes

lgirdwood approved these changes Oct 1, 2024

View reviewed changes

lgirdwood merged commit e5a2746 into thesofproject:main Oct 1, 2024
44 of 47 checks passed

Tools: Testbench: Add IPC4 support #9483

Tools: Testbench: Add IPC4 support #9483

Conversation

singalsu commented Sep 17, 2024 • edited Loading

lgirdwood left a 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

cujomalainey left a comment

Choose a reason for hiding this comment

singalsu commented Sep 20, 2024

kv2019i left a 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

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

singalsu commented Sep 24, 2024

lyakh left a comment

Choose a reason for hiding this comment

singalsu commented Sep 25, 2024

singalsu commented Sep 25, 2024

singalsu commented Sep 26, 2024

singalsu commented Sep 17, 2024 •

edited

Loading