Add dummy Loadable smart amp support #8580

RanderWang · 2023-12-06T06:34:19Z

No description provided.

lyakh · 2023-12-06T11:06:48Z

lmdk/modules/smart_amp_test/smart_amp_test.c

+	int ret;
+
+	base_cfg = mod_data->cfg.init_data;
+	sad = &smart_amp_priv;


can and shall we add a check that this function isn't called when 1 instance of the module is already running?

Only one smart amp in topology and the pcm can't be used two times at the same time, so no need to check it.

Only one smart amp in topology and the pcm can't be used two times at the same time, so no need to check it.

If we so far don't have any topology with more than one smart amp, it doesn't mean, that we don't need to be able to correctly handle such topologies. SOF must be able to handle any valid topologies.

marcinszkudlinski · 2023-12-06T13:57:12Z

src/audio/module_adapter/module_adapter.c

@@ -1107,6 +1114,8 @@ static int module_adapter_sink_source_copy(struct comp_dev *dev)

 	comp_dbg(dev, "module_adapter_sink_source_copy(): start");

+	module_adapter_sink_src_update(dev, mod);


Why you want to get new set of sink/src pointers before each copy?
What is "dynamic multi-source issue"?

EDIT: it may affect overall performance as it will be executed every cycle for every single component

This commit does fix the CI issue reported for #8537 PR. Verified on draft PR #8576 with 1855581 commit included.

Here is an example use case that requires this commit: consider two pipelines, one with mixin and the other with mixout, mixin is connected to mixout. After pipelines started, .prepare() is called for mixin and mixout. So now mixout has num_of_sources set to 1. After some time driver creates third pipeline containing mixin, binds that mixin to mixout and starts this new pipeline. .prepare() will be called for this new mixin, however, .prepare() will not be called for mixout!!! And mixout's num_of_sources is still 1, which is now wrong. This is because mixout pipeline was already running. To call .prepare() for mixout, its pipeline should be paused, then reset and then started again. However, driver does not do this.

So this commit fixes the problem. However, you are right, it is excessive to update sinks/sources at every copy call for each module. It would be better to move the module_adapter_sink_src_update() to bind() and unbind() handlers.

sure, I agree to move it to bind & unbind. Actually I did it in bind & unbind at first. Let's change it.

for smart amp, the reference buffer from capture will attach to playback stream, so the source count will be changed. This is the issue

lgirdwood

Some of this PR is fixes and can merged now with commit messages, but we need reuse and a single dummy smart amp code base.
We should only have 1 code base for each module regardless of how it's used (built-in or module)

lgirdwood · 2023-12-06T14:21:52Z

lmdk/cmake/build.cmake

@@ -25,6 +25,7 @@ foreach(MODULE ${MODULES_LIST})
 		"${LMDK_BASE}/include"
 		"${RIMAGE_INCLUDE_DIR}"
 		"${SOF_BASE}/src/include/module"
+		"${SOF_BASE}/src/include"


Is this fix to be squashed ? (its missing a commit message)

src/audio/module_adapter/module/modules.c

lgirdwood · 2023-12-06T14:27:43Z

lmdk/modules/smart_amp_test/smart_amp_test.c

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


This needs to be in modules directory and we need to reuse code, we cant have 2 implementations of dummy smart amp. i.e. I would use Kconfig to select which functions to build in library mode and which to build in as I imagine most of the code will be the same.

lgirdwood · 2023-12-06T14:28:35Z

lmdk/modules/smart_amp_test/smart_amp_test.h

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


This file needs to be in the common modules directory alongside the dummy smart amp implementation.

lmdk/libraries/smart_amp_test/CMakeLists.txt

src/audio/module_adapter/module/modules.c

src/samples/audio/smart_amp_test_ipc4.c

src/audio/module_adapter/module/generic.c

marcinszkudlinski

don't remap source pointers from const *, accidental modification of source buffers may result in hard to track issues (even to cache incoherency if dpQueue is used)

src/samples/audio/smart_amp_test_ipc4.c

lgirdwood

need to keep all logging otherwise we will have difficulty with debug

src/samples/audio/smart_amp_test_ipc4.c

RanderWang · 2023-12-11T08:51:31Z

SOFCI TEST

lgirdwood

@marcinszkudlinski good for you ?

lyakh

We plan to enable two ways to build the smart amp test as a module: as in this PR, using the system services / LMDK approach, and as in #8180 . It should be possible to select in Kconfig which way the module is built. Some aspects of this work are common, like enabling tristate Kconfig options, and I find that some of them are better resolved in #8180. I propose to first merge at least those common commits and then merge these PRs in any order. Of course, the one, that gets merged as second, will have to be adjusted a bit

lyakh · 2023-12-13T07:30:36Z

src/audio/module_adapter/module/modules.c

+	.process = modules_process,
+#else
+	.process_raw_data = modules_raw_process,
+#endif


wouldn't 2873535 be a better solution?

please check this #8554 (review). And I think the process will be the best one, so I didn't add more interface

@RanderWang ok, we can use that work-around, but then you shouldn't need to touch modules.c ops at all - they're replaced by smart-amp's ops, aren't they?

@lyakh this modules.c is used by loadable module which use "struct module_interface interface" as ops and in this ops to invoke loadable module's ops, right ?

Build-in smart amp doesn't use this ops.

if you use the work-around, proposed by @softwarecki in softwarecki@26f4b47 then these ops won't be used by the smart-amp test when built as a loadable module

@lyakh Actually I first used this patch and we have a discussion #8580 (comment)

So we have 2 months until v2.9, lets refine this further as module support is still moving quickly. i.e. we can see how things work in practice with service API modules and Zephyr modules and see what solution for above supports both best.

lyakh · 2023-12-13T07:35:46Z

src/samples/audio/smart_amp_test_ipc4.c


-	ret = smart_amp_params(mod);


do you mean, that after #8612 is merged, the configuration will be handled at the module-adapter level? Then this PR should be marked as dependent on that one, and maybe removing (then redundant) configuration setting here should be a separate commit

lyakh · 2023-12-13T07:38:40Z

src/samples/audio/smart_amp_test_ipc4.c

 	int ret;
 	const struct ipc4_base_module_extended_cfg *base_cfg = mod_data->cfg.init_data;

-	comp_dbg(dev, "smart_amp_init()");
+	LOG_DBG("smart_amp_init()");


cannot you just declare domp_dbg() and friends as dummies themselves for modular builds?

No. The problem is that dev is not available for loadable module build. please check "struct processing_module". I spent much effort to make compiling pass.

@RanderWang sorry, don't understand. You replace comp_dbg() with LOG_DBG() and then redefine it to a NOP for loadable modules. So, why don't you just redefine the original comp_dbg() to a NOP? If you do, you won't care about dev

@lyakh I understand your idea. if I use comp_dbg(), it will be more ifdef in the code. So I don't prefer it

#ifdef MODULE stuct comp_dev dev = mode->dev; #endif ..... comp_dbg(dev, "....."); ````c

then define comp_dbg() to something, that keeps dev "used" and prevents compiler unused variable warnings?

this return to the first question dev is not available for loadable module

This should be resolved by using native Zephyr logging, but this is WIP.

this return to the first question dev is not available for loadable module

I mean something like

#define dev_dbg(dev, ...) do { (void)dev; } while (0)

The previous one wouldn't work probably because dev isn't available at all as a variable. So I think a simple

#define comp_dbg(...) do {} while (0)

should do. If for some reason that doesn't work, maybe

#define comp_dbg(dev, ...) do { int dev; (void)dev; } while (0)

would.

lyakh · 2023-12-13T07:39:56Z

src/samples/audio/smart_amp_test_ipc4.c

-	if (!sad->model_handler) {
-		ret = -ENOMEM;
-		goto sad_fail;
-	}


I think it was decided that the smart amp test doesn't really need and isn't using the data blob API, right? Can we make removing it a separate commit?

src/samples/audio/smart_amp_test_ipc4.c

lyakh · 2023-12-13T07:44:16Z

src/samples/audio/smart_amp_test_ipc4.c

@@ -358,8 +349,32 @@ static const struct module_interface smart_amp_test_interface = {
 	.free = smart_amp_free
 };

+#ifndef __MODULE_BUILD__


please have a look at 03aad36 and the first hunk in 9e6dd7e

OK, we need to avoid common names to avoid collision with Zephyr, can we rename to __SOF_MODULE_SERVICE_BUILD__ and this would indicate a SOF macro for modules using the services API. Not a blocker for this PR, can be incrementally updated.

sure, this is for a demo. We still need a lot of work to include loadable module building system

RanderWang · 2023-12-13T09:13:22Z

Alsabat test issue in CI test. debugging

keqiaozhang · 2023-12-14T05:16:50Z

SOFCI TEST

RanderWang · 2023-12-14T06:04:52Z

This PR depends on #8594

Loadable modules are linked, using a linked script, built by a cmake script. That linker script includes multiple existing linker script fragments. Each of those fragments defines 1 or more sections and respective PHDRs. However, some of those scripts, e.g. common_rodata_linker_script.txt and data_linker_script.txt add sections to the same rodata_phdr PHDR. This makes the linker allocate sections in that PHDR twice in the resulting output file: one copy is real and the other one is filled with zeros. Removing one of the PHDR definitions solves the problem and removes about 60KiB of empty space from the output file. Signed-off-by: Guennadi Liakhovetski <[email protected]>

Set correct source & sink parameters for module prepare function Signed-off-by: Guennadi Liakhovetski <[email protected]>

RanderWang · 2023-12-14T06:14:43Z

Test this PR with #8594

RanderWang · 2023-12-14T13:09:36Z

#8594 was merged , ready for test

kv2019i

Any blocking issues @lyakh @softwarecki ?

lgirdwood

I think we are good to go, as the landing for the initial service API module user. This will no doubt be improved and refined as we head towards v2.9, but this now get some example code that can be tested.

lgirdwood · 2023-12-14T21:44:28Z

src/audio/module_adapter/module/modules.c

+	.process = modules_process,
+#else
+	.process_raw_data = modules_raw_process,
+#endif


So we have 2 months until v2.9, lets refine this further as module support is still moving quickly. i.e. we can see how things work in practice with service API modules and Zephyr modules and see what solution for above supports both best.

lgirdwood · 2023-12-14T21:47:43Z

src/samples/audio/smart_amp_test_ipc4.c

 	int ret;
 	const struct ipc4_base_module_extended_cfg *base_cfg = mod_data->cfg.init_data;

-	comp_dbg(dev, "smart_amp_init()");
+	LOG_DBG("smart_amp_init()");


This should be resolved by using native Zephyr logging, but this is WIP.

lgirdwood · 2023-12-14T21:50:56Z

@lrudyX @wszypelt not sure if you test the dummy smart amp in CI today ? if not, good to merge ?

The module-adapter API has 3 processing modes: raw, stream and source-sink, and until now only one of them can be implemented by any module. However, the "modules" module, that loads loadable modules, has to implement all of them to be prepared to handle any loadable modules. This adds support for such modules. Signed-off-by: Guennadi Liakhovetski <[email protected]> Signed-off-by: Rander Wang <[email protected]>

Use process instead of process_audio_stream for pipeline 2.0 Signed-off-by: Rander Wang <[email protected]>

It will be built will __SOF_MODULE_SERVICE_BUILD__ enabled Signed-off-by: Rander Wang <[email protected]>

RanderWang · 2023-12-15T06:20:21Z

updated. The failure was caused by FDK library tests which uses different process function compared to sof loadable module. I have to reuse patch: 22c398c. I checked the QT internal state , all the tests passed, but why it still show error ?

lyakh

Sorry, commit number 4 in this series now looks like too intrusive to me. As it stands it seems to modify the current smart-amp testing module a lot and I don't quite understand why. As far as I understand we don't want to change this module's functionality, we just want to make it usable as a loadable module. I understand that with system services that means eliminating most calls to the base firmware. But I'd think, that this should be done by disabling that code using #ifdefs for this build, not by removing it, as requested e.g. in #8580 (comment) . Also comparing to 3163edf#diff-37cb5e9335e5fb23ba003096bb4c98cd7cbd91d550e280bbb06a188175974e6f which only adds a hook for loading and doesn't modify any code, this really looks very intrusive.

Clarified: the large diff in commit 4 is because that commit actually converts to the source-sink API. Such a conversion would be worthy a separate PR, but that isn't critical. And the removal of comp_dbg() in commit 5 is because dev isn't present in struct processing_module when built for modules...

wszypelt · 2023-12-15T09:47:33Z

@lgirdwood We have tests for dummy smart amp but only for built-in module. after re-test all tests on green

lgirdwood · 2023-12-15T09:48:22Z

@marcinszkudlinski @mwasko good to cherry-pick

plbossart · 2023-12-18T15:02:04Z

@lgirdwood TGLU_RVP_NOCODEC failed in the CI tests https://sof-ci.01.org/sofpr/PR8580/build1172/devicetest/index.html?model=TGLU_RVP_NOCODEC-ipc4&testcase=check-suspend-resume-with-capture

Not sure why it's marked as TIMEOUT and in the issue #8641 as a DSP panic?

@keqiaozhang are you sure about the bisect to this PR?

keqiaozhang · 2023-12-21T02:28:37Z

@keqiaozhang are you sure about the bisect to this PR?

@plbossart I confirmed it again this morning, issue #8641 cannot be reproduced after reverting these 2 commits: 68c6357 and 7331745.

RanderWang requested review from ranj063, jxstelter, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners December 6, 2023 06:34

RanderWang force-pushed the loadable_smart_amplifier branch from a60b216 to 19b0ed0 Compare December 6, 2023 07:28

lyakh reviewed Dec 6, 2023

View reviewed changes

marcinszkudlinski reviewed Dec 6, 2023

View reviewed changes

lgirdwood requested changes Dec 6, 2023

View reviewed changes

serhiy-katsyuba-intel mentioned this pull request Dec 6, 2023

ipc4: mixin/mixout: Switch to use sink/source API #8537

Merged

ranj063 reviewed Dec 6, 2023

View reviewed changes

lmdk/libraries/smart_amp_test/CMakeLists.txt Show resolved Hide resolved

RanderWang force-pushed the loadable_smart_amplifier branch from 19b0ed0 to 823e545 Compare December 7, 2023 12:06

RanderWang requested a review from pblaszko as a code owner December 7, 2023 12:06

RanderWang force-pushed the loadable_smart_amplifier branch from 823e545 to e6dda3a Compare December 7, 2023 12:19

marcinszkudlinski reviewed Dec 7, 2023

View reviewed changes

src/audio/module_adapter/module/modules.c Show resolved Hide resolved

src/audio/module_adapter/module/modules.c Outdated Show resolved Hide resolved

marcinszkudlinski requested changes Dec 7, 2023

View reviewed changes

marcinszkudlinski reviewed Dec 7, 2023

View reviewed changes

src/audio/module_adapter/module/generic.c Outdated Show resolved Hide resolved

src/audio/module_adapter/module/generic.c Outdated Show resolved Hide resolved

src/audio/module_adapter/module/generic.c Outdated Show resolved Hide resolved

RanderWang changed the title ~~Add Loadable smart amp support~~ [RFC] Add Loadable smart amp support Dec 7, 2023

RanderWang force-pushed the loadable_smart_amplifier branch 3 times, most recently from 1cc00c4 to 9eb1c3b Compare December 8, 2023 06:44

marcinszkudlinski requested changes Dec 8, 2023

View reviewed changes

src/samples/audio/smart_amp_test_ipc4.c Outdated Show resolved Hide resolved

src/samples/audio/smart_amp_test_ipc4.c Outdated Show resolved Hide resolved

src/samples/audio/smart_amp_test_ipc4.c Outdated Show resolved Hide resolved

lgirdwood requested changes Dec 8, 2023

View reviewed changes

src/samples/audio/smart_amp_test_ipc4.c Show resolved Hide resolved

src/samples/audio/smart_amp_test_ipc4.c Show resolved Hide resolved

RanderWang force-pushed the loadable_smart_amplifier branch 2 times, most recently from 5572498 to be878db Compare December 11, 2023 07:39

lgirdwood approved these changes Dec 11, 2023

View reviewed changes

lyakh reviewed Dec 13, 2023

View reviewed changes

RanderWang force-pushed the loadable_smart_amplifier branch from 3153147 to 3dbd49a Compare December 14, 2023 03:44

lyakh added 2 commits December 14, 2023 14:13

module: fix firmware compilation for loadable modules

5f3d492

Set correct source & sink parameters for module prepare function Signed-off-by: Guennadi Liakhovetski <[email protected]>

RanderWang force-pushed the loadable_smart_amplifier branch from 3dbd49a to 0e2194d Compare December 14, 2023 06:14

RanderWang force-pushed the loadable_smart_amplifier branch from 0e2194d to e8be072 Compare December 14, 2023 13:05

RanderWang changed the title ~~[RFC] Add Loadable smart amp support~~ Add dummy Loadable smart amp support Dec 14, 2023

kv2019i approved these changes Dec 14, 2023

View reviewed changes

lgirdwood approved these changes Dec 14, 2023

View reviewed changes

RanderWang force-pushed the loadable_smart_amplifier branch 2 times, most recently from 7637b37 to 1d05739 Compare December 15, 2023 03:54

lyakh and others added 3 commits December 15, 2023 13:08

dummy-smart-amp: use sink & source buffer

efff75d

Use process instead of process_audio_stream for pipeline 2.0 Signed-off-by: Rander Wang <[email protected]>

lmdk: add dummy loadable smart amp support

694aa9e

It will be built will __SOF_MODULE_SERVICE_BUILD__ enabled Signed-off-by: Rander Wang <[email protected]>

RanderWang force-pushed the loadable_smart_amplifier branch from 1d05739 to 694aa9e Compare December 15, 2023 05:09

lyakh previously requested changes Dec 15, 2023

View reviewed changes

lgirdwood merged commit 68c6357 into thesofproject:main Dec 15, 2023
37 of 39 checks passed

keqiaozhang mentioned this pull request Dec 18, 2023

[BUG] DSP panic when testing suspend/resume with audio on TGLU_RVP_NOCODEC #8641

Closed

		@@ -1107,6 +1114,8 @@ static int module_adapter_sink_source_copy(struct comp_dev *dev)

		comp_dbg(dev, "module_adapter_sink_source_copy(): start");

		module_adapter_sink_src_update(dev, mod);

Add dummy Loadable smart amp support #8580

Add dummy Loadable smart amp support #8580

Conversation

RanderWang commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinszkudlinski Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment • 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

marcinszkudlinski left a comment • edited Loading

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

RanderWang commented Dec 11, 2023

lgirdwood left a comment

Choose a reason for hiding this comment

lyakh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang Dec 13, 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

RanderWang Dec 14, 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

lyakh Dec 15, 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

RanderWang commented Dec 13, 2023

keqiaozhang commented Dec 14, 2023

RanderWang commented Dec 14, 2023

RanderWang commented Dec 14, 2023

RanderWang commented Dec 14, 2023

kv2019i left a comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Dec 14, 2023

RanderWang commented Dec 15, 2023

lyakh left a comment

Choose a reason for hiding this comment

wszypelt commented Dec 15, 2023

lgirdwood commented Dec 15, 2023

plbossart commented Dec 18, 2023

keqiaozhang commented Dec 21, 2023

marcinszkudlinski Dec 6, 2023 •

edited

Loading

RanderWang Dec 7, 2023 •

edited

Loading

lgirdwood left a comment •

edited

Loading

marcinszkudlinski left a comment •

edited

Loading

RanderWang Dec 13, 2023 •

edited

Loading

RanderWang Dec 14, 2023 •

edited

Loading

lyakh Dec 15, 2023 •

edited

Loading