[SKIP SOF-TEST] native loadable up-down-mixer take 2 #8210

pjdobrowolski · 2023-09-18T05:04:37Z

This is second approach to native sof loadable modules. This time are used zephyr's linker scripts to create separate archive for modules and put them into specific for module sections. After that cmake files, using objcopy put code for up-down-mixer to udm.mod file which is being sign by rimage.

Full process of building module is automated and run by python building scripts:
xtensa-build-zephyr.py -u mtl -k ../keys/<used_key_for_project>

marcinszkudlinski · 2023-09-18T08:59:52Z

src/audio/up_down_mixer/up_down_mixer.c

@@ -463,6 +495,3 @@ static struct module_interface up_down_mixer_interface = {
 	.reset = up_down_mixer_reset,
 	.free = up_down_mixer_free
 };
-
-DECLARE_MODULE_ADAPTER(up_down_mixer_interface, up_down_mixer_comp_uuid, up_down_mixer_comp_tr);
-SOF_MODULE_INIT(up_down_mixer, sys_comp_module_up_down_mixer_interface_init);


I think we need to have ability to use module in 2 ways - as a loadable OR as a built-in
It may be a kConfig option

quick suggestion (may be incorrect syntax, just a rough example):

choice prompt "up down mixer module" config NO_UP_DOWN_MIXER bool "up down mixer not available" config BUILT_IN_UP_DOWN_MIXER bool "up down mixer as a bullt in module" config LOADABLE_IN_UP_DOWN_MIXER bool "up down mixer as a loadable module" endchoice

Linux Kconfig uses a tristate variable for modules
y = builtin
n = not built
m = module
We shoudl do the same to simplify our Kconfigs.

marcinszkudlinski · 2023-09-18T09:09:32Z

zephyr/CMakeLists.txt

@@ -497,30 +512,6 @@ zephyr_library_sources_ifdef(CONFIG_SAMPLE_KEYPHRASE
 	${SOF_SAMPLES_PATH}/audio/detect_test.c
 )

-if(CONFIG_IPC_MAJOR_3)


Conversion of a specific module to loadable should be in separate commit than the loadable module infrastructure

marcinszkudlinski · 2023-09-18T09:10:12Z

src/audio/up_down_mixer/up_down_mixer.c

+		.affinity_mask = 1,
+}
+};
+


marcinszkudlinski · 2023-09-18T09:10:29Z

src/include/sof/audio/module_adapter/library/loadable_processing_module.h

+ * \brief Type definition of the package entry point.
+ * \note internal purpose.
+ */
+typedef int (ModulePackageEntryPoint)(uint32_t costam);


english please ;)

marcinszkudlinski · 2023-09-18T09:10:56Z

src/include/sof/audio/module_adapter/library/loadable_processing_module.h

+    DEFINE_MODULE_PACKAGE_ENTRY_POINT(MODULE)
+
+/*
+ * #define DECLARE_LOADABLE_MODULE(MODULE, MODULE_FACTORY) \


why commeted out?

during development we decided to drop that header file.

marcinszkudlinski · 2023-09-18T10:58:38Z

src/audio/up_down_mixer/up_down_mixer.c

+struct sof_module_api_build_info udm_build_info __attribute__((section(".buildinfo"))) = {
+	ADSP_BUILD_INFO_FORMAT,
+	{
+		((0x3FF & 5)  << 20) |


prepare a macro for those shifts

marcinszkudlinski · 2023-09-18T11:09:14Z

zephyr/CMakeLists.txt

 # Create Trace realtive file paths
 sof_append_relative_path_definitions(modules_sof)

+
+# Compile up_down_mixer as loadable module
+sof_loadable_library(udm ${SOF_AUDIO_PATH}/up_down_mixer/up_down_mixer.toml)


more meaningful name (updwmix) instead of udm

marcinszkudlinski · 2023-09-18T11:39:17Z

src/audio/up_down_mixer/up_down_mixer.c

+	}
+};
+
+extern struct module_interface up_down_mixer_interface;


this forward really needed? Maybe move declaration of sof_module_api_build_info to the end of the file?

marcinszkudlinski · 2023-09-18T11:41:01Z

src/audio/up_down_mixer/up_down_mixer.c

+		.type = { .load_type = SOF_MAN_MOD_TYPE_MODULE,
+		.domain_ll = 1 },
+		.affinity_mask = 1,
+}


lyakh · 2023-09-18T13:15:20Z

src/include/sof/audio/module_adapter/library/loadable_processing_module.h

@@ -8,7 +8,8 @@
 #ifndef _LOADABLE_PROCESSING_MODULE_H_
 #define _LOADABLE_PROCESSING_MODULE_H_

-#include  <sof/audio/module_adapter/iadk/adsp_stddef.h>
+#include <sof/audio/module_adapter/iadk/adsp_stddef.h>
+#include <sof/audio/module_adapter/library/module_api_ver.h>


this doesn't look right. You haven't changed anything in the header, so there's no reason why it now need that additional header

marc-hb

Thanks but I'm pretty sure the plan for /lib/firmare/intel/sof/ is to use --use-platform-subdir like this:

sof/scripts/xtensa-build-zephyr.py --use-platform-subdir mtl


build-sof-staging 
|-- sof 
|   +-- mtl 
|       |-- community 
|       |   +-- sof-mtl.ri 	sha256=401e8360bd1a...
|       |-- sof-mtl.ldc 	sha256=516ad1320ec8...
|       |-- loadable_module1.ri
|       |-- loadable_module2.ri

|       +-- loadable_module3.ri

EDIT: maybe --use-platform-subdir should be ON by default?

plbossart · 2023-09-18T21:33:04Z

The 'base fw' and loadable modules can be signed with different keys, so we absolutely do not want them to land in the same directory - the loadable modules MUST be in a key-specific directory.

marc-hb · 2023-09-18T21:45:56Z

@plbossart you're referring to --key-type-subdir.

I was talking about --use-platform-subdir which avoids the ugly sof-{file}-lib.ri rename hack.

They're not mutually exclusive.

In any case we need a tiny bit of plain english documentation to be reviewed and approved first, we don't want people to have to search and read python or kernel code[*] to try and fail to guess the /lib/firmware/intel/sof/ structure.

This can be as simple as 1-2 pages with the output of a sample "tree" command with a few plain English words around it. Let's stop making the same mistake over and over again:

[*] from @plbossart https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/intel/pci-mtl.c#L39

pjdobrowolski · 2023-09-19T04:55:32Z

I appreciate your engagement but this is still a draft.

marc-hb · 2023-09-20T06:33:07Z

In any case we need a tiny bit of plain english documentation to be reviewed and approved first, we don't want people to have to search and read python or kernel code[*] to try and fail to guess the /lib/firmware/intel/sof/ structure.

Thank you @kv2019i !

intel_debug: add info for Intel firmware and tplg filenames sof-docs#475

So according to that sof-docs PR, the path for loadable modules is: sof-ipc4-lib/PLAT/community/UUID.bin

lgirdwood · 2023-09-20T14:53:47Z

src/include/sof/audio/module_adapter/library/loadable_processing_module.h

+/*!
+ * \def STR(s)
+ * intermediate macro for Stringification
+ * \note internal purpose only. Consider to use XSTR(s) for Stringification.
+ */
+#define STR(s) #s
+/*!
+ * \def XSTR(s)
+ * Stringification macro
+ */
+#define XSTR(s) STR(s)


need to be careful here as I would imagine these common macros should come from Zephyr.

lgirdwood · 2023-09-20T14:54:42Z

src/include/sof/audio/module_adapter/library/loadable_processing_module.h

+ * \note internal purpose.
+ */
+#define DEFINE_MODULE_PACKAGE_ENTRY_POINT(MODULE) \
+    volatile sof_module_api_build_info MODULE_PACKAGE_ENTRY_BUILD_INFO_NAME(MODULE) BUILD_INFO_SECTION = \


why does this need to be volatile ?

lgirdwood · 2023-09-20T14:56:45Z

src/audio/up_down_mixer/up_down_mixer.toml

@@ -0,0 +1,86 @@
+version = [3, 0]
+
+[adsp]


fwiw, @aiChaoSONG is looking at having an #include <platform.toml> for all the non module stuff. This will help simplify

lgirdwood · 2023-09-20T15:39:08Z

src/audio/up_down_mixer/up_down_mixer.c

@@ -463,6 +495,3 @@ static struct module_interface up_down_mixer_interface = {
 	.reset = up_down_mixer_reset,
 	.free = up_down_mixer_free
 };
-
-DECLARE_MODULE_ADAPTER(up_down_mixer_interface, up_down_mixer_comp_uuid, up_down_mixer_comp_tr);
-SOF_MODULE_INIT(up_down_mixer, sys_comp_module_up_down_mixer_interface_init);


Linux Kconfig uses a tristate variable for modules
y = builtin
n = not built
m = module
We shoudl do the same to simplify our Kconfigs.

Added toml file to enable building up down mixer as loadable module. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

Updated zephyr revision to the top of 006 branch Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

updated zephyr to mtl-006 updated rimage to mtl-006

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

pjdobrowolski requested review from marc-hb, abonislawski, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners September 18, 2023 05:04

pjdobrowolski requested review from lyakh, kv2019i, lgirdwood, RDharageswari, abonislawski and softwarecki and removed request for dbaluta, plbossart, lgirdwood, lbetlej, mmaka1, abonislawski, kv2019i and marc-hb September 18, 2023 05:04

pjdobrowolski changed the title ~~Sof pawelxd loadable udm~~ native loadable up-down-mixer take 2 Sep 18, 2023

pjdobrowolski closed this Sep 18, 2023

pjdobrowolski reopened this Sep 18, 2023

pjdobrowolski marked this pull request as draft September 18, 2023 07:27

marcinszkudlinski reviewed Sep 18, 2023

View reviewed changes

lyakh reviewed Sep 18, 2023

View reviewed changes

marc-hb requested changes Sep 18, 2023

View reviewed changes

marc-hb mentioned this pull request Sep 18, 2023

ASoC: SOF: Intel: pci-tgl: Change the default paths and firmware names thesofproject/linux#4382

Merged

kv2019i mentioned this pull request Sep 19, 2023

intel_debug: add info for Intel firmware and tplg filenames thesofproject/sof-docs#475

Merged

lgirdwood reviewed Sep 20, 2023

View reviewed changes

softwarecki and others added 4 commits September 25, 2023 09:13

Add support for building sof native loadable modules

527b5ac

Added toml file to enable building up down mixer as loadable module. Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

west: Update Zephyr revision

4493f0c

Updated zephyr revision to the top of 006 branch Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

scripts: xtensa-build_zephyr: Copy loadable modules to staging directory

a5d536e

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

west: update zephyr version

8663ea4

updated zephyr to mtl-006 updated rimage to mtl-006

pjdobrowolski force-pushed the sof_pawelxd_loadable_udm branch 4 times, most recently from c662f6d to 58c0bdf Compare September 28, 2023 06:44

marc-hb changed the title ~~native loadable up-down-mixer take 2~~ [SKIP SOF-TEST] native loadable up-down-mixer take 2 Sep 28, 2023

pjdobrowolski force-pushed the sof_pawelxd_loadable_udm branch from 58c0bdf to 223cb55 Compare September 28, 2023 06:59

Example of loadable up_down_mixer

69925ce

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>

pjdobrowolski force-pushed the sof_pawelxd_loadable_udm branch from 223cb55 to 69925ce Compare September 28, 2023 07:16

pjdobrowolski closed this Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SKIP SOF-TEST] native loadable up-down-mixer take 2 #8210

[SKIP SOF-TEST] native loadable up-down-mixer take 2 #8210

pjdobrowolski commented Sep 18, 2023

marcinszkudlinski Sep 18, 2023 •

edited

Loading

lgirdwood Sep 20, 2023

marcinszkudlinski Sep 18, 2023

marcinszkudlinski Sep 18, 2023

marcinszkudlinski Sep 18, 2023

marcinszkudlinski Sep 18, 2023

pjdobrowolski Sep 18, 2023

marcinszkudlinski Sep 18, 2023

marcinszkudlinski Sep 18, 2023

marcinszkudlinski Sep 18, 2023

marcinszkudlinski Sep 18, 2023

lyakh Sep 18, 2023

marc-hb left a comment •

edited

Loading

plbossart commented Sep 18, 2023

marc-hb commented Sep 18, 2023

pjdobrowolski commented Sep 19, 2023

marc-hb commented Sep 20, 2023

lgirdwood Sep 20, 2023

lgirdwood Sep 20, 2023

lgirdwood Sep 20, 2023

lgirdwood Sep 20, 2023

[SKIP SOF-TEST] native loadable up-down-mixer take 2 #8210

[SKIP SOF-TEST] native loadable up-down-mixer take 2 #8210

Conversation

pjdobrowolski commented Sep 18, 2023

marcinszkudlinski Sep 18, 2023 • edited Loading

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

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

plbossart commented Sep 18, 2023

marc-hb commented Sep 18, 2023

pjdobrowolski commented Sep 19, 2023

marc-hb commented Sep 20, 2023

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

marcinszkudlinski Sep 18, 2023 •

edited

Loading

marc-hb left a comment •

edited

Loading