Add support for IPC4 loadable modules and smart_amp_test module #4096

aiChaoSONG · 2022-12-13T09:14:13Z

All IPC4 loadable module will require base module config + base module config extension style blob to be initialized. the IPC4 smart_amp_test module is implemented with the same style init blob. Add the support in the kernel for them.

Patches have been verified with IADK dummy smart amp loadable module, and SOF smart_amp_test module here: thesofproject/sof#6751

There are two styles IPC payload blob for SOF module instance initialization: - base module config + module specific extension - base module config + base module config extension The first one is commonly used by basefw modules, and the second one is commonly used by SOF loadable modules. The patch adds structures for base module config extension. Signed-off-by: Chao Song <chao.song@linux.intel.com>

This patch adds a new token to tell which one of the below module instance initialization blob style a widget is using: - base module config + module specific extension - base module config + base module config extension So that the kernel knows how to prepare IPC4 payload for the widget. Signed-off-by: Chao Song <chao.song@linux.intel.com>

The new base module config + base module config extension module initialization blob is commonly used by SOF loadable process modules, this patch adds support for those modules. Signed-off-by: Chao Song <chao.song@linux.intel.com>

For module uses base module config + base module config extension style init blob, the pin format may be different from base module config. This patch will set copier sink from the connected pin format. Signed-off-by: Chao Song <chao.song@linux.intel.com>

Some process modules may change the pipeline params, update the pipeline params from output format in the prepare ops for process modules. Currently, only channels are required to be updated. Signed-off-by: Chao Song <chao.song@linux.intel.com>

ranj063 · 2022-12-13T18:01:48Z

sound/soc/sof/ipc4-topology.h

+	u8 reserved[8];
+	u32 priv_param_len;
+	struct sof_ipc4_pin_format sink_pin_fmt[0];
+	struct sof_ipc4_pin_format source_pin_fmt[0];


use DECLARE_FLEX_ARRAY() ?

The init blob comes from topology bytes kcontrol, we will use this structure to decode the binary blob, we don't need to alloc anything, I don't think FLEX_ARRAY is good to be used for this case.

@aiChaoSONG I dont think it makes sense to have 2 flexible arrays in a struct. Please combine them into one, pin_fmts[]

indeed this is heavily frowned upon. I don't understand what this is about quite frankly.

@aiChaoSONG I dont think it makes sense to have 2 flexible arrays in a struct. Please combine them into one, pin_fmts[]

Actually there are three: struct sof_ipc4_pin_format sink_pin_fmt[0]; struct sof_ipc4_pin_format source_pin_fmt[0]; u8 priv[0]; Maybe the easiest way is just to use void *priv_data[0]; or u8 priv[0] directly.

This does not compute for me either.

struct sof_ipc4_pin_format sink_pin_fmt[0]; struct sof_ipc4_pin_format source_pin_fmt[0]; u8 priv[0];

does not make any sense at all. Where priv[0] is pointing to? They all point to the same offset as all of them have size of 0.
at best you have data[] at the end with a comment that from there you have num_sink_pin_fmts x sof_ipc4_pin_format followed by num_source_pin_fmts x sof_ipc4_pin_format then priv_param_len number of bytes of some data.

ranj063 · 2022-12-13T18:03:51Z

sound/soc/sof/sof-audio.h

+	 *
+	 * This flag is used to tell which initialization blob style a widget is using.
+	 */
+	int init_blob_style;


this is IPC4 specific isnt it? Does it need to go in struct snd_sof_widget? Can we find a better place?

snd_sof_widget is shared between IPC3 and IPC4, I don't think we have a IPC4 specific struct which is a good fit for this.

we can later add snd_sof_ipc3_widget/snd_sof_ipc4_widget if necessary.

How about struct sof_ipc4_process? We know that the base_fw modules already use module specific extension.

payload_with_output_fmt is already IPC4-specific @ranj063, looks like another poorly defined partitioning.

payload_with_output_fmt is already IPC4-specific @ranj063, looks like another poorly defined partitioning.

Maybe fix both of them?

payload_with_output_fmt is already IPC4-specific @ranj063, looks like another poorly defined partitioning.

@ujfalusi raised this concern about payload_with_output_fmt before. But we don't have ipc4 related structure at that time, so we put them here temporarily. I think we do need a IPC4 specific place to put ipc4 stuff. If this is only used by process module, struct sof_ipc4_process is good.

ranj063 · 2022-12-13T18:07:08Z

sound/soc/sof/ipc4-topology.c


 	memcpy(cfg, data, control_data->data->size);
+
+	/* Update pipeline params from output format */


does this conflict with @libinyang 's PR?

Yeah, we do the same thing to update pipeline params, we have synced, and I will merge some of his code in this PR, then we can close #4097

ranj063 · 2022-12-13T18:08:08Z

sound/soc/sof/ipc4-topology.h

 	struct sof_ipc4_msg msg;
 };

+/**


are we already using this type "base module config + module specific extension"?

yes, all bese fw module use this type, for example:
copier:

struct ipc4_copier_module_cfg { struct ipc4_base_module_cfg base; struct ipc4_audio_format out_fmt; uint32_t copier_feature_mask; struct ipc4_copier_gateway_cfg gtw_cfg; } __attribute__((packed, aligned(4)));

peakvol

struct ipc4_peak_volume_module_cfg { struct ipc4_base_module_cfg base_cfg; struct ipc4_peak_volume_config config[0]; } __packed __aligned(8);

module specific extension is optional, mixin/mixout don't have module specific extension.
https://github.com/thesofproject/sof/blob/73f6171bb3c8c5b1d0963883014508022445b997/src/audio/mixin_mixout/mixin_mixout.c#L157

@aiChaoSONG can youplease add this to the commit message saying that support for the first type is already implemented

ranj063 · 2022-12-13T18:12:04Z

sound/soc/sof/ipc4-topology.c

 	/*
-	 * Output format is optional for process modules.
-	 * Process modules setup the output format based on audio format tokens in topology.
+	 * Output format is only valid and optional for modules with base module config +


i'd switch around the optional and only valid bits. "Output format is optional and only valid ...." and the check below as well

ranj063 · 2022-12-13T18:14:25Z

sound/soc/sof/ipc4-topology.c

+					sizeof(struct sof_ipc4_pin_format);
+		if (data_size != size) {
+			dev_err(sdev->dev, "Insufficient data for widget %s initialization\n",
+				swidget->widget->name);


should this check be done on line 1660? What if there are no kcontrols for the widget?

what if there are more than one kcontrols for the widget?

ranj063 · 2022-12-13T18:15:24Z

sound/soc/sof/ipc4-topology.c

 	}
 }

+static const struct sof_ipc4_audio_format *


typo in commit message " For modules using..."

ranj063 · 2022-12-13T18:17:29Z

sound/soc/sof/ipc4-topology.c

+	if (swidget->init_blob_style == SOF_INIT_BLOB_BASE_WITH_MOD_EXT) {
+		struct sof_ipc4_base_module_cfg *base_config = swidget->private;
+
+		if (pin_type == SOF_PIN_TYPE_SOURCE && swidget->payload_with_output_fmt) {


when will this be true? If I see the call below, pin_type is always SOF_PIN_TYPE_SINK?

in updating pipeline params (process module prepare ops), we need to get the output pin format, and set the output format to pipeline param, the pin_type will be SOF_PIN_TYPE_SOURCE there.

ranj063 · 2022-12-13T18:22:22Z

sound/soc/sof/ipc4-topology.c

+	} else {
+		pin_start = base_ext->num_sink_pin_fmts;


why do we need the else? same as the comment above pin_type will not be different at all

the function supports to get any input/output pin format, in setting copier sink, we need to get the input format of the widget that connected to copier, in updating pipeline params, we need to get the output format of the widget, and update the format to pipeline params.

plbossart

we're quite far from a mergeable PR @aiChaoSONG, please see comments below.

plbossart · 2022-12-14T18:56:54Z

sound/soc/sof/ipc4-topology.h

+ * struct sof_ipc4_pin_format - widget's pin format
+ * @pin_index: pin index for the widget
+ * @ibs: input buffer size for input pin
+ * @obs: output buffer size for output pin


how do we know if the pin is input or output?

plbossart · 2022-12-14T18:57:39Z

sound/soc/sof/ipc4-topology.h


+/**
+ * struct sof_ipc4_pin_format - widget's pin format
+ * @pin_index: pin index for the widget


is the pin_index relative to a direction, i.e. you can have both pin_index=1 for input and pin_index=1 for output

plbossart · 2022-12-14T18:59:24Z

sound/soc/sof/ipc4-topology.h

+	u8 reserved[8];
+	u32 priv_param_len;
+	struct sof_ipc4_pin_format sink_pin_fmt[0];
+	struct sof_ipc4_pin_format source_pin_fmt[0];


indeed this is heavily frowned upon. I don't understand what this is about quite frankly.

plbossart · 2022-12-14T19:00:39Z

include/uapi/sound/sof/tokens.h

 #define SOF_TKN_COMP_SINK_PIN_BINDING_WNAME	413
 #define SOF_TKN_COMP_SRC_PIN_BINDING_WNAME	414
 #define SOF_TKN_COMP_PAYLOAD_WITH_OUTPUT_FMT	415
+#define SOF_TKN_COMP_INIT_BLOB_STYLE		416


what are the values for this token?

plbossart · 2022-12-14T19:01:33Z

sound/soc/sof/sof-audio.h


+/* Widget initialization blob style */
+#define SOF_INIT_BLOB_BASE_WITH_MOD_EXT	0
+#define SOF_INIT_BLOB_BASE_WITH_BASE_EXT	1


well the values should be in the tokens.h definition, no? This defines the UAPI, it should be clear.
I am afraid we have similar issues with the pin type. Gah.

plbossart · 2022-12-14T19:13:16Z

sound/soc/sof/ipc4-topology.c

 	}
 }

+static const struct sof_ipc4_audio_format *


commit message: I have idea what this means

"This patch will set copier sink from the connected pin format."

The connected pin format can only be used to set a sink pin format, no?

The already merged PR name the function as sof_ipc4_set_copier_sink_format to align with firmware naming, it is actually setting the copier source(output) pin format from the connected sink(input) pin format of the downstream widget.

I will update the commit message to: set copier source pin from connected sink pin format

plbossart · 2022-12-14T19:13:35Z

sound/soc/sof/ipc4-topology.c


+static const struct sof_ipc4_audio_format *
+sof_ipc4_get_widget_pin_format(struct snd_sof_widget *swidget, bool pin_type,
+			       int pin_id)


plbossart · 2022-12-14T19:14:53Z

sound/soc/sof/ipc4-topology.c

+
+		base_ext = (struct sof_ipc4_base_module_cfg_ext *)control_data->data->data;
+		break;
+	}


this is spaghetti code to me. Just can't figure out how safe/reliable this is.

plbossart · 2022-12-14T19:15:50Z

sound/soc/sof/ipc4-topology.c

+	}
+
+	for (i = pin_start; i < pin_end; i++) {
+		if (pin_fmt[i].pin_index == pin_id)


you've got to explain how the pins are ordered or numbered. This is super hard to review.

plbossart · 2022-12-14T19:19:53Z

sound/soc/sof/ipc4-topology.c

+	hw_param_interval(pipeline_params, SNDRV_PCM_HW_PARAM_CHANNELS)->min =
+		SOF_IPC4_AUDIO_FORMAT_CFG_CHANNELS_COUNT(out_fmt->fmt_cfg);
+	hw_param_interval(pipeline_params, SNDRV_PCM_HW_PARAM_CHANNELS)->max =
+		SOF_IPC4_AUDIO_FORMAT_CFG_CHANNELS_COUNT(out_fmt->fmt_cfg);


I don't understand what this is about.

a) does a pipeline have format parameters? If not, then the commit message is just wrong.
b) can avoid the repetition of hw_param_interval(pipeline_params, SNDRV_PCM_HW_PARAM_CHANNELS)-> and use a local variable to make the code more readable?

bardliao · 2022-12-15T00:57:24Z

sound/soc/sof/ipc4-topology.c

 		if (control_data->data->blob_type != SOF_IPC4_MOD_INIT_INSTANCE)
 			continue;
-
+		data_size = control_data->data->size;


What if data_size = 0? Or there is no SOF_IPC4_MOD_INIT_INSTANCE blob type in the kcontrol?

aiChaoSONG · 2023-01-10T06:55:09Z

a better alternative is #4121, close this one.

Chao Song added 5 commits December 8, 2022 17:20

ranj063 reviewed Dec 13, 2022

View reviewed changes

libinyang mentioned this pull request Dec 14, 2022

ASoC: SOF: ipc4-topology: update output hw_params in init audio fmt #4097

Closed

aiChaoSONG mentioned this pull request Dec 14, 2022

Topology2: add smart amp nocodec test topology thesofproject/sof#6799

Closed

plbossart requested changes Dec 14, 2022

View reviewed changes

bardliao reviewed Dec 15, 2022

View reviewed changes

aiChaoSONG mentioned this pull request Jan 3, 2023

IPC4 process module instance initialization support #4121

Closed

aiChaoSONG closed this Jan 10, 2023


		memcpy(cfg, data, control_data->data->size);

		/* Update pipeline params from output format */

Add support for IPC4 loadable modules and smart_amp_test module #4096

Add support for IPC4 loadable modules and smart_amp_test module #4096

Uh oh!

Conversation

aiChaoSONG commented Dec 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aiChaoSONG Dec 14, 2022 • 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!

ranj063 Dec 14, 2022 • 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!

libinyang Dec 15, 2022 • 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!

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!

aiChaoSONG Dec 14, 2022 • 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!

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!

plbossart 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!

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

aiChaoSONG Dec 14, 2022 •

edited

Loading

ranj063 Dec 14, 2022 •

edited

Loading

libinyang Dec 15, 2022 •

edited

Loading

aiChaoSONG Dec 14, 2022 •

edited

Loading

aiChaoSONG commented Jan 10, 2023 •

edited

Loading