Audio: template_comp: Add a new SOF template component by singalsu · Pull Request #9952 · thesofproject/sof

singalsu · 2025-04-03T16:40:56Z

No description provided.

tools/testbench/utils_ipc3.c

src/audio/template_comp/template_comp.c

lyakh · 2025-04-04T06:36:21Z

src/audio/template_comp/template_comp.c

+				*y = *(x + cd->channels_order[ch]);
+				y++;
+			}
+			x += cd->channels;


maybe in a follow-up - should we add template-generic.c and template-hifi5.c with different versions of these functions? Then also a template_comp.h header becomes justified, otherwise it isn't really needed?

I need to complete this task quickly for coming power-on and move on to other critical things, so I'd like to postpone the HiFi5 version. But sure it makes sense, and shows how to use the Xtensa version helper macros.

Also per my own experience the xt-clang compiler for HiFi5 is good, it's hard to beat the performance of good C code (e.g. hot code loops unrolled by four or eight) with hand written intrinsics. Intrinsics are good for saturated math with rounding that is not efficient with C but otherwise the gain can be low.

@singalsu I think we can move this functions now into a file name template-generic.c. It doesn't pollute the template_comp.c code and makes everything easier to read.

marcinszkudlinski · 2025-04-04T07:11:49Z

src/audio/template_comp/template_comp.c

+	return NULL;
+}
+
+static int template_comp_process(struct processing_module *mod,


I strongly suggest using sink/source in the template for new modules

I've converted it now to sink/source. But as not experienced with it, not sure if the best way for everything. Some things look now a bit more complex than before. Can you please check again this!

@marcinszkudlinski has your request been addressed correctly? Should your request for change be dropped?

singalsu · 2025-04-04T10:54:41Z

Into this version I fixed the rimage .toml for ACE platforms, it loads and runs now.

@marcinszkudlinski Thanks for suggestion, good idea! I'll update next this to sink/source api.

dbaluta · 2025-04-07T07:42:51Z

@singalsu we already have similar component src/audio/module_adapter/module/passthrough.c but I see your point with this one.

Looks good. I'm working on a presentation for EOSS/ZDS on how to add a new component to SOF and I could use this PR as an example.

Is this component working only with IPC4?

lgirdwood

The code and topology look fine (we need the kcontrol logic), but we need

comments everywhere ! This should be like a book on how to create a module, AI should be able to understand it.
Based on all the files changes in this PR, can we add a Readme.md file that lists all the setpc needed (based on the steps taken in this PR.

lgirdwood · 2025-04-07T14:00:31Z

src/audio/template_comp/template_comp.c

+ */
+SOF_DEFINE_REG_UUID(template_comp);
+
+DECLARE_TR_CTX(template_comp_tr, SOF_UUID(template_comp_uuid), LOG_LEVEL_INFO);


Can we comment everything ! e.g. what is it for, why needed, how to use etc etc.

lgirdwood · 2025-04-07T14:02:02Z

src/audio/template_comp/template_comp.toml

@@ -0,0 +1,21 @@
+#ifndef LOAD_TYPE


I would also put a lot cf comments in here too.

Unfortunately I don't know the meaning of these. We have a document start in https://github.com/thesofproject/sof-docs/pull/451/files but it was never merged to sof-docs where it should be described.

abonislawski

Definitely need more comments, this template seems quite complicated

abonislawski · 2025-04-08T06:57:19Z

src/audio/template_comp/template_comp.c

+	return 0;
+}
+
+__cold static int template_comp_free(struct processing_module *mod)


e.g. here a short comment on why __cold

dbaluta · 2025-04-08T14:06:04Z

src/audio/template_comp/template_comp.c

+	if (cd->enable) {
+		/* Process the data with the channels
+		 * swap example function
+		 */


can this comment just fit on a single line?

src/audio/template_comp/template_comp.c

dbaluta · 2025-04-08T14:09:27Z

@singalsu I left some comments. Would it also be possible to add IPC3 support?

singalsu · 2025-04-08T14:50:24Z

@singalsu I left some comments. Would it also be possible to add IPC3 support?

Thanks! Yes I could add (control handling is main difference), but I'm not able to test it. Also need a topology. Could it be another PR after this?

singalsu · 2025-04-08T14:52:08Z

src/audio/template_comp/template_comp.h

+struct template_comp_comp_data {
+	template_comp_func template_comp_func; /**< processing function */
+	int channels_order[PLATFORM_MAX_CHANNELS]; /**< channels ordering */
+	int source_format; /**< audio format for input */


I forgot to remove this, not used.

dbaluta · 2025-04-08T15:08:55Z

Thanks! Yes I could add (control handling is main difference), but I'm not able to test it. Also need a topology. Could it be another PR after this?

Ok, could be a separate PR. I can help test it.

singalsu · 2025-04-08T15:55:02Z

Thanks! Yes I could add (control handling is main difference), but I'm not able to test it. Also need a topology. Could it be another PR after this?

Ok, could be a separate PR. I can help test it.

Yep, testing help and topology for IPC3 platform would be great. For Intel builds we can't have IPC3 and sink/source api without tricky back-porting.

lgirdwood · 2025-04-08T17:54:36Z

Thanks! Yes I could add (control handling is main difference), but I'm not able to test it. Also need a topology. Could it be another PR after this?

Ok, could be a separate PR. I can help test it.

Yep, testing help and topology for IPC3 platform would be great. For Intel builds we can't have IPC3 and sink/source api without tricky back-porting.

@dbaluta @singalsu can we then split this into template-common.c, template-ipc4.c and template-ipc3.c where @singalsu can deal with IPC4 and @dbaluta can deal with IPC3.

lgirdwood · 2025-04-11T14:14:55Z

src/audio/template_comp/template-generic.c

+	struct template_comp_comp_data *cd = module_get_private_data(mod);
+	int16_t *x, *x_start, *x_end;
+	int16_t *y, *y_start, *y_end;
+	size_t size;
+	int x_size, y_size;
+	int source_samples_without_wrap;
+	int samples_without_wrap;
+	int samples = frames * cd->channels;
+	int bytes = frames * cd->frame_bytes;
+	int ret;
+	int ch;
+	int i;
+
+	ret = source_get_data(source, bytes, (void const **)&x, (void const **)&x_start, &size);
+	x_size = size >> 1; /* Bytes to number of s16 samples */
+	if (ret)
+		return ret;
+
+	ret = sink_get_buffer(sink, bytes, (void **)&y, (void **)&y_start, &size);
+	y_size = size >> 1; /* Bytes to number of s16 samples */
+	if (ret)
+		return ret;
+
+	x_end = x_start + x_size;
+	y_end = y_start + y_size;
+	while (samples) {
+		source_samples_without_wrap = x_end - x;
+		samples_without_wrap = y_end - y;
+		samples_without_wrap = MIN(samples_without_wrap, source_samples_without_wrap);
+		samples_without_wrap = MIN(samples_without_wrap, samples);


@singalsu can you have a comment for each step as this is a learning module. I'm good to merge if you can give a good commentry in the module and I can come back and add more context on the infra.

Yep, I added comments to things happening in processing function.

lyakh · 2025-04-28T07:07:04Z

src/audio/template_comp/template-generic.c

+			 * as defined in array channels_order[].
+			 */
+			for (ch = 0; ch < cd->channels; ch++) {
+				*y = *(x + cd->channels_order[ch]);


isn't it similar to channel mapping in some other components, would channel_map be a better name for it?

I'm changing it since you prefer it, though this is not the same. Channels map elsewhere is an enum describing the stream, e.g. stereo or 5.1.

Adding, yes you are right, there's in IPC4 copiers usage a channel map vector.

lyakh · 2025-04-28T07:08:59Z

src/audio/template_comp/template-generic.c

+	ret = source_get_data(source, bytes, (void const **)&x, (void const **)&x_start, &size);
+	x_size = size >> 1; /* Bytes to number of s16 samples */
+	if (ret)
+		return ret;


would be better to put if (ret) return ret; directly after line 50, before x_size calculation. Particularly since this is a template, so a copy-paste source

OK, though I tried to make more compact looking code this way.

lyakh · 2025-04-28T07:09:22Z

src/audio/template_comp/template-generic.c

+	ret = sink_get_buffer(sink, bytes, (void **)&y, (void **)&y_start, &size);
+	y_size = size >> 1; /* Bytes to number of s16 samples */
+	if (ret)
+		return ret;


ditto, and in other formats below too of course

Yep, fixed now.

lyakh · 2025-04-28T07:12:59Z

src/audio/template_comp/template-ipc3.c

+				comp_err(dev, "Illegal switch control num_elems = %d.",
+					 cdata->num_elems);
+				return -EINVAL;
+			}


also a nitpick, but since this is a template:

if (cdata->num_elems != 1) { comp_err(dev, "Illegal switch control num_elems = %d.", cdata->num_elems); return -EINVAL; } cd->enable = cdata->chanv[0].value; comp_info(dev, "Setting enable = %d.", cd->enable);

i.e. check and return in case invalid, then process with standard processing without an else

Yep, good idea.

lyakh · 2025-04-28T07:13:32Z

src/audio/template_comp/template-ipc3.c

+		} else {
+			comp_err(dev, "Illegal switch control index = %d.", cdata->index);
+			return -EINVAL;
+		}


ditto - you also save 2 levels of indentation

Yep, good finding. Note that I wanted to keep index != 0 to make it obvious these are numbers assigned for controls in topology. I trust the compiler to find the best instructions to handle it.

lyakh · 2025-04-28T07:15:41Z

src/audio/template_comp/template.c

+	frames = MIN(frames, sink_frames);
+	if (cd->enable) {
+		/* Process the data with the channels swap example function. */
+		ret = cd->template_comp_func(mod, source, sink, frames);


could do return cd->template_comp_func(...); and remove the else below and the ret variable

lyakh · 2025-04-28T07:17:20Z

src/audio/template_comp/template.c

+	enum sof_ipc_frame source_format;
+	int i;
+
+	comp_info(dev, "template_comp_prepare()");


we don't even make .prepare() functions __cold because they should run reasonably fast. Better make this comp_dbg()

lyakh · 2025-04-28T07:22:16Z

src/audio/template_comp/template.c

+{
+	struct template_comp_comp_data *cd = module_get_private_data(mod);
+
+	comp_info(mod->dev, "template_comp_reset()");


lyakh · 2025-04-28T07:25:15Z

src/audio/template_comp/template.c

+
+	assert_can_be_cold();
+
+	comp_info(mod->dev, "template_comp_free()");


even in __cold functions I'd make these comp_dbg() - sometimes these are useful, e.g. to see which component ID is which type, but IMHO if we do this in all modules - that would be too much. At least would be good to have them consistent. Maybe use comp_info() in all .init() and comp_dbg() in all others

OK but hesitating a bit with this -- can no more see the life cycle of component from normal trace.

maybe we should make a decision - how we log .init(), .free(), .set_config() and follow that consistently. Of course for debugging the more the better - if it all were captured perfectly with no negative effects on system performance, but unfortunately both happen: (1) if too much is logged, the logging subsystem will drop log entries automatically, and it can then drop important ones, and (2) performance can suffer

Yep, the init, free, set_config would be my preference as minimum. The components are not causing as much traces traffic as the other framework.

Yep, the init, free, set_config would be my preference as minimum. The components are not causing as much traces traffic as the other framework.

@singalsu I wouldn't be against this in principle, if we really weren't already hitting a margin, where adding just a couple more logs triggers Zephyr to drop some. E.g. looking at mtrace in https://sof-ci.01.org/sofpr/PR9952/build12397/devicetest/index.html?model=PTLH_RVP_NOCODEC&testcase=check-playback-all-formats which is good because it runs 1 stream at a time. A stream there begins with

[ 588.396178] <inf> ipc: ipc_cmd: rx : 0x44000000|0x3060004c

which is MOD_LARGE_CONFIG_SET. This doesn't yet belong to the actual streaming but the next IPC

[ 588.398488] <inf> ipc: ipc_cmd: rx : 0x11000005|0x0

is already GLB_CREATE_PIPELINE. The last log entry, that belongs to stream configuration before starting streaming is

[ 588.421185] <wrn> host_comp: host_get_copy_bytes_normal: comp:0 0x3 no bytes to copy, available samples: 0, free_samples: 192

which is less than 3ms later. In this time we generate 66 log entries. And - again - this is just one stream. If we do init-free-set_config for each component, in this case we have 7 components in 2 pipelines, would be 14 lines alone (.free() isn't called during construction, but might interfere if we run multiple streams). Of those 66 lines 22 lines are IPCs (I think we do need these, I find them very useful), 12 "pipe: pipeline_connect:" lines (also rather useful IMHO), 6 "buffer_new: buffer new size ..." lines (do we need these?)... So 14 lines more seems a rather significant addition. But you can of course make such a test PR and we'll see what mtrace logs will look like there

I prefer the simple human readable traces, I've unless desperate never tried to decode in my mind the ipx rx/tx hex commands. We also see those IPC hex dumps in kernel messages.

This patch contains all needed to add a new minimal SOF component to FW build for TGL and more recent platforms plus sof-testbench4 simulation. the component name is template_comp and it is easy to duplicate for new component development from scratch. The component supports one switch kcontrol. When enabled the component swaps or reverses the channels order if there are two or more channels. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch adds the template_comp widget class to topology2 and provides three topologies with word lengths 16/24/32 bits to test the component in playback and capture. The topologies are built to development directory with names sof-hda-benchmark-template_comp<16/24/32>.tplg. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

marcinszkudlinski

looks OK, just minor suggestion for comments regarding multiple inputs/outputs

marcinszkudlinski · 2025-04-29T07:23:48Z

src/audio/template_comp/template.c

+
+	comp_dbg(dev, "template_comp_prepare()");
+
+	if (num_of_sources != 1 || num_of_sinks != 1)


add a comment - there may be more than 1 input and/or output.

@singalsu can you update incrementally, will merge this.

Yep, I'll do another PR to address Marcin's additions.

@marcinszkudlinski See #9990

marcinszkudlinski · 2025-04-29T07:26:59Z

src/audio/template_comp/template.c

+{
+	struct template_comp_comp_data *cd =  module_get_private_data(mod);
+	struct comp_dev *dev = mod->dev;
+	struct sof_source *source = sources[0];


singalsu commented Apr 3, 2025

View reviewed changes

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

lyakh reviewed Apr 4, 2025

View reviewed changes

marcinszkudlinski requested changes Apr 4, 2025

View reviewed changes

singalsu force-pushed the add_template_component branch from 61de80a to 5f3c2c4 Compare April 4, 2025 10:52

lgirdwood reviewed Apr 7, 2025

View reviewed changes

abonislawski reviewed Apr 8, 2025

View reviewed changes

singalsu force-pushed the add_template_component branch from 5f3c2c4 to 19822f1 Compare April 8, 2025 13:37

singalsu requested a review from marcinszkudlinski April 8, 2025 13:39

dbaluta reviewed Apr 8, 2025

View reviewed changes

src/audio/template_comp/template_comp.c Outdated Show resolved Hide resolved

singalsu commented Apr 8, 2025

View reviewed changes

singalsu force-pushed the add_template_component branch 2 times, most recently from 9f24b39 to 99720e6 Compare April 10, 2025 17:01

singalsu marked this pull request as ready for review April 10, 2025 17:11

singalsu requested review from jsarha, kv2019i, lbetlej, mmaka1, plbossart and ranj063 as code owners April 10, 2025 17:11

singalsu requested review from dbaluta, lgirdwood and lyakh April 10, 2025 17:11

singalsu requested a review from abonislawski April 10, 2025 17:11

lgirdwood reviewed Apr 11, 2025

View reviewed changes

singalsu force-pushed the add_template_component branch from 99720e6 to c95ce87 Compare April 25, 2025 17:06

singalsu requested a review from lgirdwood April 25, 2025 17:31

lyakh reviewed Apr 28, 2025

View reviewed changes

singalsu added 2 commits April 28, 2025 18:04

singalsu force-pushed the add_template_component branch from c95ce87 to ef5b0ba Compare April 28, 2025 15:16

singalsu requested a review from lyakh April 28, 2025 15:17

lyakh approved these changes Apr 29, 2025

View reviewed changes

marcinszkudlinski approved these changes Apr 29, 2025

View reviewed changes

lgirdwood merged commit 4da9683 into thesofproject:main May 2, 2025
42 of 51 checks passed


		assert_can_be_cold();

		comp_info(mod->dev, "template_comp_free()");


		comp_dbg(dev, "template_comp_prepare()");

		if (num_of_sources != 1 \|\| num_of_sinks != 1)

Conversation

singalsu commented Apr 3, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singalsu Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singalsu commented Apr 4, 2025

Uh oh!

dbaluta commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abonislawski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dbaluta commented Apr 8, 2025

Uh oh!

singalsu commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbaluta commented Apr 8, 2025

Uh oh!

singalsu commented Apr 8, 2025

Uh oh!

lgirdwood commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

singalsu Apr 4, 2025 •

edited

Loading

dbaluta commented Apr 7, 2025 •

edited

Loading