Add support for IPC4 loadable modules and smart_amp_test module by aiChaoSONG · Pull Request #4096 · thesofproject/linux

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

+	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.
structsof_ipc4_pin_formatsink_pin_fmt[0]; structsof_ipc4_pin_formatsource_pin_fmt[0]; u8priv[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

+ *
+ * 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


 	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

 	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

 	/*
- * 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

+ 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

 	}
 }

+static const struct sof_ipc4_audio_format *


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

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

+	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

+	} 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

+ * 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


+/**
+ * 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

+	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

 #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


+/* 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

 	}
 }

+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


+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

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

+	}
+
+	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

+	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

 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.

Floating point instructions in userspace can crash some arm kernels built with clang/LLD 17.0.6: BUG: unsupported FP instruction in kernel mode FPEXC == 0xc0000780 Internal error: Oops - undefined instruction: 0 [thesofproject#1] ARM CPU: 0 PID: 196 Comm: vfp-reproducer Not tainted 6.10.0 thesofproject#1 Hardware name: BCM2835 PC is at vfp_support_entry+0xc8/0x2cc LR is at do_undefinstr+0xa8/0x250 pc : [<c0101d50>] lr : [<c010a80c>] psr: a0000013 sp : dc8d1f68 ip : 60000013 fp : bedea19c r10: ec532b17 r9 : 00000010 r8 : 0044766c r7 : c0000780 r6 : ec532b17 r5 : c1c13800 r4 : dc8d1fb0 r3 : c10072c4 r2 : c0101c88 r1 : ec532b17 r0 : 0044766c Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 00c5387d Table: 0251c008 DAC: 00000051 Register r0 information: non-paged memory Register r1 information: vmalloc memory Register r2 information: non-slab/vmalloc memory Register r3 information: non-slab/vmalloc memory Register r4 information: 2-page vmalloc region Register r5 information: slab kmalloc-cg-2k Register r6 information: vmalloc memory Register r7 information: non-slab/vmalloc memory Register r8 information: non-paged memory Register r9 information: zero-size pointer Register r10 information: vmalloc memory Register r11 information: non-paged memory Register r12 information: non-paged memory Process vfp-reproducer (pid: 196, stack limit = 0x61aaaf8b) Stack: (0xdc8d1f68 to 0xdc8d2000) 1f60: 0000081f b6f69300 0000000f c10073f4 c10072c4 dc8d1fb0 1f80: ec532b17 0c532b17 0044766c b6f9ccd8 00000000 c010a80c 00447670 60000010 1fa0: ffffffff c1c13800 00c5387d c0100f10 b6f68af8 00448fc0 00000000 bedea188 1fc0: bedea314 00000001 00448ebc b6f9d000 00447608 b6f9ccd8 00000000 bedea19c 1fe0: bede9198 bedea188 b6e1061c 0044766c 60000010 ffffffff 00000000 00000000 Call trace: [<c0101d50>] (vfp_support_entry) from [<c010a80c>] (do_undefinstr+0xa8/0x250) [<c010a80c>] (do_undefinstr) from [<c0100f10>] (__und_usr+0x70/0x80) Exception stack(0xdc8d1fb0 to 0xdc8d1ff8) 1fa0: b6f68af8 00448fc0 00000000 bedea188 1fc0: bedea314 00000001 00448ebc b6f9d000 00447608 b6f9ccd8 00000000 bedea19c 1fe0: bede9198 bedea188 b6e1061c 0044766c 60000010 ffffffff Code: 0a000061 e3877202 e594003c e3a0901 (eef16a10) ---[ end trace 0000000000000000 ]--- Kernel panic - not syncing: Fatal exception in interrupt ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]--- This is a minimal userspace reproducer on a Raspberry Pi Zero W: #include <stdio.h> #include <math.h> int main(void) { double v = 1.0; printf("%fn", NAN + *(volatile double *)&v); return 0; } Another way to consistently trigger the oops is: calvin@raspberry-pi-zero-w ~$ python -c "import json" The bug reproduces only when the kernel is built with DYNAMIC_DEBUG=n, because the pr_debug() calls act as barriers even when not activated. This is the output from the same kernel source built with the same compiler and DYNAMIC_DEBUG=y, where the userspace reproducer works as expected: VFP: bounce: trigger ec532b17 fpexc c0000780 VFP: emulate: INST=0xee377b06 SCR=0x00000000 VFP: bounce: trigger eef1fa10 fpexc c0000780 VFP: emulate: INST=0xeeb40b40 SCR=0x00000000 VFP: raising exceptions 30000000 calvin@raspberry-pi-zero-w ~$ ./vfp-reproducer nan Crudely grepping for vmsr/vmrs instructions in the otherwise nearly idential text for vfp_support_entry() makes the problem obvious: vmlinux.llvm.good [0xc0101cb8] <+48>: vmrs r7, fpexc vmlinux.llvm.good [0xc0101cd8] <+80>: vmsr fpexc, r0 vmlinux.llvm.good [0xc0101d20] <+152>: vmsr fpexc, r7 vmlinux.llvm.good [0xc0101d38] <+176>: vmrs r4, fpexc vmlinux.llvm.good [0xc0101d6c] <+228>: vmrs r0, fpscr vmlinux.llvm.good [0xc0101dc4] <+316>: vmsr fpexc, r0 vmlinux.llvm.good [0xc0101dc8] <+320>: vmrs r0, fpsid vmlinux.llvm.good [0xc0101dcc] <+324>: vmrs r6, fpscr vmlinux.llvm.good [0xc0101e10] <+392>: vmrs r10, fpinst vmlinux.llvm.good [0xc0101eb8] <+560>: vmrs r10, fpinst2 vmlinux.llvm.bad [0xc0101cb8] <+48>: vmrs r7, fpexc vmlinux.llvm.bad [0xc0101cd8] <+80>: vmsr fpexc, r0 vmlinux.llvm.bad [0xc0101d20] <+152>: vmsr fpexc, r7 vmlinux.llvm.bad [0xc0101d30] <+168>: vmrs r0, fpscr vmlinux.llvm.bad [0xc0101d50] <+200>: vmrs r6, fpscr <== BOOM! vmlinux.llvm.bad [0xc0101d6c] <+228>: vmsr fpexc, r0 vmlinux.llvm.bad [0xc0101d70] <+232>: vmrs r0, fpsid vmlinux.llvm.bad [0xc0101da4] <+284>: vmrs r10, fpinst vmlinux.llvm.bad [0xc0101df8] <+368>: vmrs r4, fpexc vmlinux.llvm.bad [0xc0101e5c] <+468>: vmrs r10, fpinst2 I think LLVM's reordering is valid as the code is currently written: the compiler doesn't know the instructions have side effects in hardware. Fix by using "asm volatile" in fmxr() and fmrx(), so they cannot be reordered with respect to each other. The original compiler now produces working kernels on my hardware with DYNAMIC_DEBUG=n. This is the relevant piece of the diff of the vfp_support_entry() text, from the original oopsing kernel to a working kernel with this patch: vmrs r0, fpscr tst r0, thesofproject#4096 bne 0xc0101d48 tst r0, #458752 beq 0xc0101ecc orr r7, r7, #536870912 ldr r0, [r4, #0x3c] mov r9, thesofproject#16 -vmrs r6, fpscr orr r9, r9, #251658240 add r0, r0, thesofproject#4 str r0, [r4, #0x3c] mvn r0, thesofproject#159 sub r0, r0, #-1207959552 and r0, r7, r0 vmsr fpexc, r0 vmrs r0, fpsid +vmrs r6, fpscr and r0, r0, #983040 cmp r0, #65536 bne 0xc0101d88 Fixes: 4708fb0 ("ARM: vfp: Reimplement VFP exception entry in C code") Signed-off-by: Calvin Owens <calvin@wbinvd.org> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

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 */

Uh oh!

Conversation

aiChaoSONG commented Dec 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

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

aiChaoSONGDec 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

Uh oh!

Choose a reason for hiding this comment

aiChaoSONGDec 14, 2022•
edited
Loading

ranj063Dec 14, 2022•
edited
Loading

libinyangDec 15, 2022•
edited
Loading

aiChaoSONGDec 14, 2022•
edited
Loading

aiChaoSONG commented Jan 10, 2023•
edited
Loading