Convert volume to use the new module interface #5766

ranj063 · 2022-05-03T20:16:40Z

This first 4 commits are from #5872. This PR will need to be rebased once that is merged.

dbaluta · 2022-05-11T07:25:18Z

src/audio/module_adapter/module_adapter.c

@ranj063 having a second thought about this I think we shouldn't use config->type but UUID as we agreed with @cujomalainey at some point.

We want to avoid adding new component types in the future.

Personally I want to delete that enum, i think its just confusing for newcomers.

Pointer *spec points directly to IPC mailbox address. In this code You assume, it maps to following structure:
struct ipc_config_process {
uint32_t size; /< size of bespoke data section in bytes */
uint32_t type; /< sof_ipc_process_type */
unsigned char *data;
};
I think this is not true for IPC4 protocol, where initial data are passed through IPC4 header extended, and payload starts with beginning of the configuration. The size variable will be wrong here.

@jxstelter right the module adapter needs to be adapter for IPC4. My next target is to convert the copier. I will handle this with that change

ranj063 · 2022-06-03T16:11:17Z

@singalsu @lgirdwood this PR is ready for review now

lgirdwood

How does the legacy part work ? i.e how is volume partitioned ?

ranj063 · 2022-06-07T21:40:21Z

How does the legacy part work ? i.e how is volume partitioned ?

@lgirdwood I created new functions for the API's needed for the new module interface API and then reused them within the ops for the legacy comp_drv ops. This only difference is that with the comp_drv interface we also need to allocated struct processing_module apart from comp_dev. But this structure is minimally used with no memory allocation for the input/putput buffers

lyakh · 2022-06-08T06:33:32Z

tools/tplg_parser/pga.c

do we do this everywhere? It would be better to use container_of() for these

no I switched back to the original definition. so not relevant anymore

lyakh · 2022-06-08T06:35:39Z

tools/tplg_parser/pga.c

memcpy(volume + 1,...)

or better still a convenience macro for UUID copying (since I assume the relative position is calculated the same and the size is const)

not sure if a macro will be helpful, Liam. The size is actually not constant since it varies based on type of component.

tools/tplg_parser/pga.c

src/audio/module_adapter/module_adapter.c

src/audio/module_adapter/module/volume/volume.c

lyakh · 2022-06-08T06:53:45Z

src/audio/module_adapter/module/volume/volume.c

wouldn't it be better to put legacy and new in two different files?

volume is the only exception for a module that will need to keep the legacy comp_drv. While it might a bit cleaner to separate them out, I think keeping them all in one place might make it easier to read.

src/audio/module_adapter/module_adapter.c

singalsu · 2022-06-08T18:15:32Z

src/audio/module_adapter/module_adapter.c

Why is there need to zero the buffer?

isnt it better to zero the input buffers? WHy leave stale data in?

is it possible that that stale data ends up being delivered to the user? And this one is on a hot path - executed for each copy, so indeed should be avoided

Clearing the buffer consumes unnecessary MCPS. Please fix this afterwards if not in this version.

src/audio/module_adapter/module/volume/volume.c

singalsu · 2022-06-08T19:03:06Z

src/audio/module_adapter/module/volume/volume_generic.c

VOL_S32_SAMPLES_TO_BYTES() ?

In preparation for comverting the volume component to use the module interface, add support for parsing the UUID from topology when parsing the PGA widget in testbench. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

lyakh · 2022-06-09T08:48:13Z

src/audio/module_adapter/module_adapter.c

do I see it correctly, that these source and sink are only used in the simple_copy case below? If so, please move these assignments there

@lyakh if I move it inside the if, I get an error with these being used uninitialized.

first of all I think it's only source that the compiler won't be able to figure out. So, yes, we'd have to initialise it to NULL - I think that's better than assigning irrelevant for the non-simple case values here to be overwritten there.

Optimize the copy() function for modules that have only 1 source/sink buffer and produce only period bytes every period. There is no need to allocate intermediate buffers to hold the processed samples. Also, pass the buffer stream pointers directly so that there no extra copies. Introduce a new field, simple_copy in struct processing_module that can be set by modules that do not need deep buffering and produce exactly period worth of data in every copy(). Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

singalsu

Looks OK, the issues those I commented were from the original code. Please make an improvements PR later for this, e.g. the bzero().

lgirdwood

Lets get this under daily testing now and fix the other comments incrementally.
@ranj063 can we also include in the followup PR a Kconfi change that uses the new volume module on say TGL/ADL and the old comp on the other platforms. This will give us more CI/test coverage and make sure we dont see any regressions in the legacy.

lgirdwood · 2022-06-09T15:51:25Z

@ranj063 looks like a lot of QB CI failures, I 'm not sure if this is recent.
@wszypelt seeing messages like this

16:03:56.720 ITERATION TestDmicBothFifoSimpleCapture16000hz16b16b2ch.test === 1654783436.720132
16:03:56.720 => TestDmicBothFifoSimpleCapture16000hz16b16b2ch.test
16:03:56.735   => Pre-iteration setup
16:03:56.735     => Tested Audio Format
16:03:56.735     => Create 16000Hz 2ch playback file
16:03:56.735     => Create 16000Hz 2ch playback file
16:03:56.735     => Verify DMIC:
16:03:57.563   => Exception in test.
16:03:57.563     Exception TypeError occurred at 'C:\\work\\tools\\pyexecnetcache\\cavs_scripts-py\\tests\\sof\\fw_05_dmic\\dmic_base.py':111
                 'NoneType' object is not subscriptable

Any clues what Nonetype errors mean ?
I will rerun CI to be sure.

lgirdwood · 2022-06-09T15:51:33Z

SOFCI TEST

ranj063 · 2022-06-09T16:08:42Z

Lets get this under daily testing now and fix the other comments incrementally. @ranj063 can we also include in the followup PR a Kconfi change that uses the new volume module on say TGL/ADL and the old comp on the other platforms. This will give us more CI/test coverage and make sure we dont see any regressions in the legacy.

@lgirdwood done. I left the legacy comp drv interface for everything up until CNL

Move the volume component inside the module_adapter folder and use the module interface API instead of the component driver interface. Also fix the volume cmocka tests to use the modified scale_vol function. Also, add kconfig option for keeping the legacy comp_drv interface for platforms that are limited in text region memory size, such as BYT/CHT/APL/JSL/ICL. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

marc-hb · 2022-06-10T21:01:14Z

In case a git bisect lands you on commit volume: use the new module interface, a make clean may help. It helped me! Kconfig problem.

--- build_apl_gcc_volbroken/generated/.config	2022-06-10 20:50:39.178214458 +0000
+++ build_apl_gcc/generated/.config	2022-06-10 20:58:12.840624754 +0000
@@ -81,6 +81,8 @@
 # Audio components
 #
 CONFIG_COMP_DAI=y
+CONFIG_COMP_DAI_GROUP=y
+CONFIG_COMP_BLOB=y
 CONFIG_COMP_SRC=y
 # CONFIG_COMP_SRC_STD is not set
 CONFIG_COMP_SRC_SMALL=y
@@ -114,8 +116,19 @@
 CONFIG_ASRC_SUPPORT_CONVERSION_48000_TO_32000=y
 CONFIG_ASRC_SUPPORT_CONVERSION_48000_TO_44100=y
 # CONFIG_COMP_TDFB is not set
-# CONFIG_COMP_MODULE_ADAPTER is not set
-# CONFIG_COMP_LEGACY_INTERFACE is not set
+CONFIG_COMP_MODULE_ADAPTER=y
+CONFIG_COMP_LEGACY_INTERFACE=y
+
+#
+# Processing modules
+#
+# CONFIG_CADENCE_CODEC is not set
+CONFIG_COMP_VOLUME=y
+# CONFIG_COMP_VOLUME_WINDOWS_FADE is not set
+CONFIG_COMP_VOLUME_LINEAR_RAMP=y
+# CONFIG_PASSTHROUGH_CODEC is not set
+# CONFIG_WAVES_CODEC is not set
+# CONFIG_DTS_CODEC is not set
 # CONFIG_COMP_IGO_NR is not set
 # CONFIG_COMP_RTNR is not set

marc-hb · 2022-06-15T00:24:00Z

100% reproducible regression / panic filed in

[BUG] DSP panic when setting volume after checking kmod load-unload after playback on BDW_WSB_RT286 #5911

ranj063 force-pushed the fix/volume_new_api branch 4 times, most recently from 5ac90a7 to 213fb98 Compare May 4, 2022 15:38

dbaluta reviewed May 11, 2022

View reviewed changes

ranj063 force-pushed the fix/volume_new_api branch 25 times, most recently from b351fa3 to 7c563ed Compare May 26, 2022 19:26

ranj063 force-pushed the fix/volume_new_api branch from 487b141 to eb61ba8 Compare June 3, 2022 16:10

ranj063 requested review from lyakh and singalsu June 6, 2022 17:44

lgirdwood reviewed Jun 7, 2022

View reviewed changes

lyakh reviewed Jun 8, 2022

View reviewed changes

singalsu reviewed Jun 8, 2022

View reviewed changes

testbench: add support for parsing UUID

aad1f9c

In preparation for comverting the volume component to use the module interface, add support for parsing the UUID from topology when parsing the PGA widget in testbench. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/volume_new_api branch from eb61ba8 to c7612f0 Compare June 9, 2022 04:10

lyakh reviewed Jun 9, 2022

View reviewed changes

ranj063 force-pushed the fix/volume_new_api branch from c7612f0 to af2a74e Compare June 9, 2022 13:10

ranj063 requested review from lyakh and singalsu June 9, 2022 13:10

ranj063 force-pushed the fix/volume_new_api branch from af2a74e to e67347e Compare June 9, 2022 13:18

singalsu approved these changes Jun 9, 2022

View reviewed changes

lgirdwood approved these changes Jun 9, 2022

View reviewed changes

ranj063 force-pushed the fix/volume_new_api branch from e67347e to 7790c86 Compare June 9, 2022 16:08

ranj063 force-pushed the fix/volume_new_api branch from 7790c86 to 482a30a Compare June 9, 2022 16:21

ranj063 force-pushed the fix/volume_new_api branch from 482a30a to 603cb88 Compare June 9, 2022 17:55

lgirdwood merged commit f7663de into thesofproject:main Jun 10, 2022

miRoox mentioned this pull request Jun 13, 2022

[BUG] DSP panic when setting volume after checking kmod load-unload after playback on BDW_WSB_RT286 #5911

Closed

marc-hb mentioned this pull request Jun 15, 2022

Convert the EQ IIR component to use the module interface #5926

Merged

Convert volume to use the new module interface #5766

Convert volume to use the new module interface #5766

Uh oh!

Conversation

ranj063 commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

ranj063 commented Jun 3, 2022

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Jun 7, 2022

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh Jun 9, 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!

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!

lyakh Jun 10, 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!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Jun 9, 2022

Uh oh!

lgirdwood commented Jun 9, 2022

Uh oh!

ranj063 commented Jun 9, 2022

Uh oh!

marc-hb commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ranj063 commented May 3, 2022 •

edited

Loading

lyakh Jun 9, 2022 •

edited

Loading

lyakh Jun 10, 2022 •

edited

Loading

marc-hb commented Jun 10, 2022 •

edited

Loading

marc-hb commented Jun 15, 2022 •

edited

Loading