Preparation for introducing the SOF ALSA plugin #8132

ranj063 · 2023-08-30T23:46:26Z

The SOF ALSA plugin will be used for running SOF topology pipelines to run on the host for development/debugging purposes.
The changes in this PR include preparatory patches for:

Building shared libraries for SOF modules
Posix headers for host build
Addition of missing platform definitions for library build

btian1 · 2023-08-31T01:38:23Z

posix/include/rtos/alloc.h

@@ -0,0 +1,172 @@
+/* SPDX-License-Identifier: BSD-3-Clause


not able to include those headers? duplicate is not a good option.

@btian1 this is for host build for testbench/tplg parser/plugin etc where zephyr/xtos doesnt feature

so you mean plugin build does not have xtos/zephyr included?

we do already use SOF headers for library (testbench, cmocka) builds, cannot we also use them for this? Really should avoid duplicating - a sure path to breakage and confusion...

But which set? I think this approach has merits as well. I think we have three options:

XTOS build with CONFIG_LIBRARY (use sof/xtos/rtos)

Zephyr build with posix as arch (use sof/zephyr/rtos)

Native library build (use sof/posix/rtos)

I think (3) involves code duplication, but for a pure library build (that can be linked to any Linux application), I think this approach is cleanest. Especially if we longterm remove the the XTOS build. Using Zephyr posix build is one option, but I don't think that is really ideal for the library use (like an ALSA plugin, or exposing audio modules via some other framerwork like LV2).

This is the correct direction - its not perfect today but we still need to abstract some RTOS calls today for host usage (and this is this abstraction). Cmocka should also use this abstraction.

Long term our abstractions should continue and shrink so that we have a minimal abstraction surface on top of POSIX (if any).

I would move list.h and other generic (non RTOS api) level headers to a sof/lib (but this could be another patch).

If these files are strictly identical then they should be copied at build time. See commit e0d8078 / #7244 for an example of a .h file copied at build time.

If this is a copy/paste/diverge then the copies should at least refer to each other to keep divergence damage somewhat under control.

Also, there's probably no need to duplicate all the doxygen, is it? Just point to the main copy instead. I don't think we publish ALL these doxygens do we?

@lgirdwood Right, but we don't need XTOS arch code if plugin uses its own rtos implementation. sof/posix/rtos should be independent of sof/src/arch . cmocka uses it, but it was done before we had a rtos abstraction.

not hugely important, but calling these "posix headers" is a bit confusing - don't think all these headers are POSIX

lyakh · 2023-08-31T08:09:35Z

tools/testbench/common_test.c

 		printf("debug: %s", message);
 }

 /* enable trace in testbench */
 void tb_enable_trace(bool enable)
 {
-	test_bench_trace = enable;
+	host_trace_level = enable;


confused by this commit: (1) where host_trace_level is defined (apparently, it is defined somewhere, since this all works, or maybe it doesn't need to be, and (2) doesn't the above mean, that we set host_trace_level to either LOG_LEVEL_ERROR or to 0, which isn't even a valid error level?

debug colides easily and test_bench_trace tightly coupled any user code to testbench.

@lgirdwood sorry, I mean it's a wrong assignment IMHO. Before test_bench_trace was effectively a boolean variable - it only took values of 0 and 1. Now host_trace_level is a printing verbosity level taking a value between LOG_LEVEL_CRITICAL (1) and LOG_LEVEL_VERBOSE (4). So this assignment should be fixed.

@ranj063 can you update testbench to take a log level instead of a bool on the command line which can be passed above. Thanks!

src/platform/library/lib/trace.c

lyakh · 2023-08-31T08:16:13Z

posix/include/rtos/alloc.h

@@ -0,0 +1,172 @@
+/* SPDX-License-Identifier: BSD-3-Clause


we do already use SOF headers for library (testbench, cmocka) builds, cannot we also use them for this? Really should avoid duplicating - a sure path to breakage and confusion...

lyakh · 2023-08-31T08:25:17Z

src/audio/Kconfig

@@ -684,4 +684,11 @@ config WRAP_ACTUAL_POSITION
 	  It is not necessary that on wrap, the buffer position would be zero.At wrap,
 	  in some cases based on the period size, the frame may not exactly be at the
 	  end of the buffer and roll over for some bytes from the beginning of the buffer.
+
+config COMP_MODULES_SO


we might soon need a similar option to build SOF modules for loading to the DSP, can we maybe add "HOST" somewhere to this option to distinguish between the two?

Or LIBRARY something, that's what this is in the end, right?

We can call them host modules

kv2019i

Looks pretty good. One bigger issue is the arch layer. If we decide to go with a top-level rtos abstraction (i.e. library is an alternative to a XTOS or Zephyr build), I don't see a point in using the arch layer. We can proceed to merge this version and remove it later, but I'd like to get a consensus before we proceed. Zephyr posix build is of course one option, but not sure whether it works as a plugin library (probably less work to have a native library build for Linux).

kv2019i · 2023-08-31T11:31:25Z

posix/include/rtos/alloc.h

@@ -0,0 +1,172 @@
+/* SPDX-License-Identifier: BSD-3-Clause


But which set? I think this approach has merits as well. I think we have three options:

XTOS build with CONFIG_LIBRARY (use sof/xtos/rtos)

Zephyr build with posix as arch (use sof/zephyr/rtos)

Native library build (use sof/posix/rtos)

I think (3) involves code duplication, but for a pure library build (that can be linked to any Linux application), I think this approach is cleanest. Especially if we longterm remove the the XTOS build. Using Zephyr posix build is one option, but I don't think that is really ideal for the library use (like an ALSA plugin, or exposing audio modules via some other framerwork like LV2).

kv2019i · 2023-08-31T11:32:44Z

posix/include/rtos/atomic.h

+
+static inline void atomic_init(atomic_t *a, int32_t value)
+{
+	arch_atomic_init(a, value);


I don't think this arch_foo() layer makes sense. If we pull the rtos layer out from XTOS (or Zephyr), then we don't need any dependency to a separate arch layer. I.e. this should use POSIX primitives (or Linux extensions) directly.

@kv2019i but the arch layer used here would use the gcc atomic builtins as defined in the host arch atomic.h. Isnt that good enough?

@ranj063 This surely works and to be clear, I'm ok to merge this as an intermedia step, but I want to get aligned conceptually that there is no need to depend on the XTOS arch layer.

kv2019i · 2023-08-31T11:33:57Z

src/arch/host/include/arch/debug/panic.h

@@ -5,7 +5,7 @@
 * Author: Tomasz Lauda <[email protected]>
 */

-#ifdef __XTOS_RTOS_PANIC_H__
+#ifdef __POSIX_RTOS_PANIC_H__


Hmm, continuing with previous, @lrgirdwo @ranj063 , I don't see a point in arch layer if we go for indepdent "rtos" implementation ("XTOS", "Zephyr" and "posix").

We are stuck a bit today whilst xtos is still being used, i.e. difficult to extract until more xtos related code can be removed (and this depends on progress with Zephyr integration for all platforms).

src/platform/library/include/platform/lib/memory.h

kv2019i · 2023-08-31T11:36:15Z

src/audio/Kconfig

@@ -684,4 +684,11 @@ config WRAP_ACTUAL_POSITION
 	  It is not necessary that on wrap, the buffer position would be zero.At wrap,
 	  in some cases based on the period size, the frame may not exactly be at the
 	  end of the buffer and roll over for some bytes from the beginning of the buffer.
+
+config COMP_MODULES_SO


Or LIBRARY something, that's what this is in the end, right?

marc-hb

I don't think "host" is a good name for what is really just SIMULation.

tl;dr: "host" = best avoided. Use "simulation" instead.

Simulation is the term used by Zephyr, XTensa and the testbench:

"Host" is already used for something different: Host is where the Linux / Windows driver run:

https://thesofproject.github.io/latest/introduction/index.html

Of course "host" is typically the same system for many developers because why would you inconveniently use two different systems when the same workstation can do everything. Yet it absolutely does not have to be the same system because these two "hosts" are conceptually two totally different things. The testbench and the fuzzer have many "driver-free" users. Conversely, production "hosts" use drivers but never any testbench or fuzzer. For Yocto users these two "hosts" are two different systems too.

Using the same word "host" for everything just because some people use the same workstation for everything maintains a bad confusion.

Making things even worse, "host" and "target" are also widely misunderstood toolchain concepts:
https://mesonbuild.com/Cross-compilation.html

In a nutshell, the typical mistake assumes that the terms build, host and target refer to some fixed positions whereas they're actually relative to where the current compiler is running. Think of host as a child of the current compiler and target as an optional grand-child. Compilers don't change their terminology when they're creating another compiler.

marc-hb · 2023-08-31T14:45:16Z

posix/include/rtos/alloc.h

@@ -0,0 +1,172 @@
+/* SPDX-License-Identifier: BSD-3-Clause


If these files are strictly identical then they should be copied at build time. See commit e0d8078 / #7244 for an example of a .h file copied at build time.

If this is a copy/paste/diverge then the copies should at least refer to each other to keep divergence damage somewhat under control.

Also, there's probably no need to duplicate all the doxygen, is it? Just point to the main copy instead. I don't think we publish ALL these doxygens do we?

lgirdwood · 2023-08-31T19:46:22Z

I don't think "host" is a good name for what is really just SIMULation.

tl;dr: "host" = best avoided. Use "simulation" instead.

Simulation is the term used by Zephyr and XTensa:

@marc-hb Not following why you think this is a simulation ? This is an ALSA plugin that can run SOF pipeline topologies and modules in userspace.

marc-hb · 2023-08-31T22:23:34Z

@marc-hb Not following why you think this is a simulation ? This is an ALSA plugin that can run SOF pipeline topologies and modules in userspace.

Because it does not run on the DSP like all the other simulations I listed above.

ranj063 · 2023-08-31T23:49:20Z

@marc-hb Not following why you think this is a simulation ? This is an ALSA plugin that can run SOF pipeline topologies and modules in userspace.

Because it does not run on the DSP like all the other simulations I listed above.

This is not sumlation @marc-hb and host matches exactly with your quoted definition. These modules can be used to run pipelines on the host just like the linux/windows driver.

Host based mailbox needs to be fixed in another place. It's here now until rtos partitioning is done. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

Add maths directory for host. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

lgirdwood · 2023-09-01T08:32:52Z

@marc-hb Not following why you think this is a simulation ? This is an ALSA plugin that can run SOF pipeline topologies and modules in userspace.

Because it does not run on the DSP like all the other simulations I listed above.

@marc-hb You are probably thinking of the POSIX Zephyr mode where SOF runs under Zephyr as a POSIX application (it could be considered a simulation) that we use for fuzzing. This plugin is a userspace executable that runs generic SOF pipeline and module code.

marc-hb · 2023-09-02T00:06:49Z

@marc-hb You are probably thinking of ...

There are different cases and I'm thinking "host" suits none of them.

In some places in the code we need a name for "not running on DSP". But negations don't make good names; I don't think anyone wants something like CONFIG_NOT_ON_THE_DSP.

So instead we tend to see things like HOST_MODULES, host_trace_level and arch/host/include. But which host!?! There are many different hosts, so "host" is very confusing.

=> Specific "hosts" should be named with their specific name, e.g.: fuzzer, testbench, zephyr POSIX, ALSA plugin, etc. Leave the generic and vague "host" name only for Linux/Windows drivers where there's no firmware code running and where it was used first.

=> CONFIG_NOT_ON_THE_DSP should be called CONFIG_SIMULATION because it's the term already used everywhere, see all the examples above.

lyakh · 2023-09-04T10:18:51Z

CONFIG_NOT_ON_THE_DSP should be called CONFIG_SIMULATION because it's the term already used everywhere, see all the examples above.

@marc-hb I disagree too. All the above examples are ok with "simulation" because they are simulations. Whereas actually running in a user configuration "on the host," being part of the audio processing pipeline and performing useful work - that isn't a simulation any more. Historically maybe "host" used to mean "native" to the "build host." I.e. not cross-compiled, but now it certainly doesn't have to be the case. E.g. in case of ARM if we build ALSA plugins, we'll build them to run on the ARM "host" and we might well cross-compile them. So in case of ARM we have 3 architectures: build / test / simulate, DSP, and OS "host." Then, yes, we could use "simulation" for the test system, like the CI, which is typically x86*, the DSP can be Xtensa or ARM, and the "OS host" can be x86* or ARM.

kv2019i · 2023-09-04T12:23:35Z

src/include/sof/trace/trace.h

+#include <sys/time.h>
+
+/* trace level used on host configurations */
+extern int host_trace_level;


I don't think we need to get caught up with the naming (so not a showstopper for me), but as a option, we could call this "extern int sof_library_trace_level".

I think this is logical. We have a build target to build SOF a library, not an application, and when built this way, certain extra interfaces are exposed to do things normally done internally by "SOF-the-application". Like setting trace levels.

kv2019i · 2023-09-04T12:26:19Z

src/audio/Kconfig

@@ -684,4 +684,12 @@ config WRAP_ACTUAL_POSITION
 	  It is not necessary that on wrap, the buffer position would be zero.At wrap,
 	  in some cases based on the period size, the frame may not exactly be at the
 	  end of the buffer and roll over for some bytes from the beginning of the buffer.
+
+config COMP_HOST_MODULES_SO
+	bool "Build modules as shared objects"


COMP_MODULE_SHARED_LIBRARY_BUILD

"Build SOF modules as shared libraries"

updated now @kv2019i

marc-hb · 2023-09-05T18:40:20Z

Whereas actually running in a user configuration "on the host," being part of the audio processing pipeline and performing useful work - that isn't a simulation any more.

You could call it "hybrid emulation" if you want to make a difference. Note the difference between emulation and simulation depends on whom you ask. Either term makes is clear that the code does not run on the DSP.

"Host" is clearly not on the DSP either but it makes you wonder "which host(s)?" which may or may not be relevant. Super confusing.

E.g. in case of ARM if we build ALSA plugins, we'll build them to run on the ARM "host" and we might well cross-compile them. So in case of ARM we have 3 architectures: build / test / simulate, DSP, and OS "host." Then, yes, we could use "simulation" for the test system, like the CI, which is typically x86*, the DSP can be Xtensa or ARM, and the "OS host" can be x86* or ARM.

Toolchains matter of course but I'm not sure architecture examples really help clarifying... Anyway good demonstration that the word "host" is now void of any meaning, thanks this is my main point! I suspect "host" is appealing because it's.... short.

btian1

@ranj063 , still have code style problems, please check.

RanderWang

LGTM

lyakh · 2023-09-06T06:23:14Z

src/include/sof/trace/trace.h

+	(void)id_1;						\
+	(void)id_2;						\
+	struct timeval tv;					\
+	char *msg = "(%s:%d) " format;				\


Why not put both these variables under the if? In practical terms the difference isn't huge - also this way their scope is limited to the "pseudo-loop," but having them under the if would be more logical

lyakh · 2023-09-06T06:26:58Z

tools/testbench/common_test.c

 		printf("debug: %s", message);
 }

 /* enable trace in testbench */
 void tb_enable_trace(bool enable)
 {
-	test_bench_trace = enable;
+	host_trace_level = enable;


@lgirdwood sorry, I mean it's a wrong assignment IMHO. Before test_bench_trace was effectively a boolean variable - it only took values of 0 and 1. Now host_trace_level is a printing verbosity level taking a value between LOG_LEVEL_CRITICAL (1) and LOG_LEVEL_VERBOSE (4). So this assignment should be fixed.

lyakh · 2023-09-06T06:34:21Z

posix/include/rtos/alloc.h

@@ -0,0 +1,172 @@
+/* SPDX-License-Identifier: BSD-3-Clause


not hugely important, but calling these "posix headers" is a bit confusing - don't think all these headers are POSIX

kv2019i

I'm good to merge. We probably need to refactor rtos layer more, but this is easier done in a follow-up. Having this merged, allows more people to contribute and this doesn't interfere with other builds of SOF, so I don't see much to gain by keeping this in review for much logner.

lyakh · 2023-09-08T06:15:17Z

I'm good to merge. We probably need to refactor rtos layer more, but this is easier done in a follow-up. Having this merged, allows more people to contribute and this doesn't interfere with other builds of SOF, so I don't see much to gain by keeping this in review for much logner.

@kv2019i I think #8132 (comment) is a bug and should be fixed

Host build should not depend on testbench symbols. Also add timestamp to the trace log. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

Add the posix headers for use with the SOF ALSA plugin. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

And add the POSIX prefix. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

Add missing platform definitions for CPU freq, DAI definitions etc. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

Fix the base_fw.h file to add the missing preprocessor directive. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

Add a new kconfig option to build the shared library modules that can be used to run the pipelines on the host with the testbench or the ALSA plugin. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

lyakh · 2023-09-14T11:03:04Z

posix/include/sof/lib/mailbox.h

+	int dsp_read_err __unused;
+
+	dcache_invalidate_region((__sparse_force void __sparse_cache *)(MAILBOX_DSPBOX_BASE +
+									offset), bytes);


do we really need all these dcache calls? None of the hosts are cache-incoherent, are they?

the show-stopper comment has been addressed

ranj063 requested review from akloniex, singalsu, jxstelter, RanderWang, marcinszkudlinski, pblaszko, lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners August 30, 2023 23:46

ranj063 force-pushed the plugin_5 branch from a181a94 to ba19c35 Compare August 31, 2023 00:50

ranj063 requested a review from marc-hb as a code owner August 31, 2023 00:50

ranj063 force-pushed the plugin_5 branch from ba19c35 to 35e7f85 Compare August 31, 2023 00:59

btian1 reviewed Aug 31, 2023

View reviewed changes

lyakh reviewed Aug 31, 2023

View reviewed changes

kv2019i reviewed Aug 31, 2023

View reviewed changes

marc-hb requested review from andyross and cujomalainey August 31, 2023 14:36

marc-hb reviewed Aug 31, 2023

View reviewed changes

ranj063 force-pushed the plugin_5 branch from 35e7f85 to 0735f71 Compare August 31, 2023 23:46

lrgirdwo added 2 commits August 31, 2023 16:54

xtos: mailbox: use memcpy for host mailbox_write

6206a16

Host based mailbox needs to be fixed in another place. It's here now until rtos partitioning is done. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

host: cmake: make sure we build maths support for host config

b6a8a18

Add maths directory for host. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

ranj063 force-pushed the plugin_5 branch from 0735f71 to 65efabd Compare August 31, 2023 23:56

kv2019i reviewed Sep 4, 2023

View reviewed changes

ranj063 force-pushed the plugin_5 branch from 65efabd to c4ac220 Compare September 5, 2023 17:33

ranj063 requested review from lyakh and kv2019i September 5, 2023 17:33

btian1 approved these changes Sep 6, 2023

View reviewed changes

RanderWang approved these changes Sep 6, 2023

View reviewed changes

lyakh previously requested changes Sep 6, 2023

View reviewed changes

kv2019i approved these changes Sep 7, 2023

View reviewed changes

lgirdwood approved these changes Sep 8, 2023

View reviewed changes

ranj063 force-pushed the plugin_5 branch from c4ac220 to 84a70de Compare September 13, 2023 15:48

lrgirdwo added 6 commits September 13, 2023 08:58

host: trace: decouple the testbench from host build

c9f06f7

Host build should not depend on testbench symbols. Also add timestamp to the trace log. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

posix: Add posix headers

82bf72f

Add the posix headers for use with the SOF ALSA plugin. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

host: remove XTOS prefix from headers

8faa78c

And add the POSIX prefix. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

library: Add missing definitions

23678fd

Add missing platform definitions for CPU freq, DAI definitions etc. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

ipc4: Add preprocessor directive

28c8db9

Fix the base_fw.h file to add the missing preprocessor directive. Signed-off-by: Liam Girdwood <[email protected]> Signed-off-by: Ranjani Sridharan <[email protected]>

ranj063 force-pushed the plugin_5 branch from 84a70de to f0bb9f3 Compare September 13, 2023 15:59

lyakh reviewed Sep 14, 2023

View reviewed changes

lgirdwood merged commit b720f1a into thesofproject:main Sep 14, 2023

ranj063 deleted the plugin_5 branch September 18, 2023 18:28

marc-hb mentioned this pull request Mar 1, 2024

audio/CMakeLists.txt: move testbench/plugin to new, separate file #8892

Open

Preparation for introducing the SOF ALSA plugin #8132

Preparation for introducing the SOF ALSA plugin #8132

Conversation

ranj063 commented Aug 30, 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

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

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

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Aug 31, 2023

marc-hb commented Aug 31, 2023

ranj063 commented Aug 31, 2023

lgirdwood commented Sep 1, 2023

marc-hb commented Sep 2, 2023 • edited Loading

lyakh commented Sep 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb commented Sep 5, 2023

btian1 left a comment

Choose a reason for hiding this comment

RanderWang 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

kv2019i left a comment

Choose a reason for hiding this comment

lyakh commented Sep 8, 2023

Choose a reason for hiding this comment

ranj063 commented Aug 30, 2023 •

edited

Loading

marc-hb left a comment •

edited

Loading

marc-hb commented Sep 2, 2023 •

edited

Loading