Convert the mixin module to use the module adapter #6836

ranj063 · 2022-12-19T04:47:53Z

No description provided.

lgirdwood · 2022-12-20T16:45:07Z

@ranj063 #6833 now merged, this will need a rebase.

lgirdwood · 2022-12-21T13:40:18Z

@ranj063 can you check internal CI thanks.

ranj063 · 2022-12-21T20:03:20Z

@ranj063 can you check internal CI thanks.

@lgirdwood looking into it. Looks like the gain quality test is failing.

ranj063 · 2022-12-22T20:45:57Z

SOFCI TEST

ranj063 · 2022-12-22T22:12:31Z

SOFCI TEST

ranj063 · 2022-12-23T00:33:49Z

SOFCI TEST

ranj063 · 2022-12-23T02:11:21Z

SOFCI TEST

ranj063 · 2022-12-23T04:21:25Z

SOFCI TEST

src/audio/module_adapter/module_adapter.c

src/audio/mixin_mixout/mixin_mixout.c

kv2019i

Nothing major, some minor comments, please see inline.

src/audio/module_adapter/module_adapter.c

src/include/sof/audio/module_adapter/module/generic.h

kv2019i · 2022-12-29T17:31:47Z

src/audio/module_adapter/module_adapter.c

 	size_t size = MAX(mod->deep_buff_bytes, mod->period_bytes);
 	uint32_t min_free_frames = UINT_MAX;
 	uint32_t num_input_buffers = 0;
+	uint32_t num_output_buffers = 0;


Not really specific to this PR, but the "copy()" starts to be very complex. I wonder if some of this logic could be moved to some less-often-run function. Could we even have a separate copy for the "simple_copy case"?

kv2019i · 2022-12-29T17:39:44Z

src/audio/mixin_mixout/mixin_mixout.c

+	base_module_cfg_to_stream_params(&mod->priv.cfg.base_cfg, params);
+
+	/* Buffers between mixins and mixouts are not used (mixin writes data directly to mixout
+	 * sink). But, anyway, let's setup these buffers properly just in case.


This feels quit out-of-place, but as this is existing comment (and code) moved as-is, not need to block because of this.

the comment could be a bit "kinder" though at least, stating some "better sounding" reason than "just in case," e.g. "for correctness," "for consistency" etc. But yes, it's just a verbatim copy of the current comment

kv2019i · 2022-12-29T17:56:37Z

src/audio/mixin_mixout/mixin_mixout.c


 	dev->ipc_config.frame_fmt = frame_fmt;
 	mod->simple_copy = true;
+	mod->skip_src_buffer_invalidate = true;


Minor thing, but the commit leaves a bit open why the modules need the ability to do this. This does add two code branches to a very common code path (the module process). I wonder if it's more efficient to just remove the extra writebacks from modules, and if there a few cases where this can't be done, pay the cost only for these cases.

@kv2019i I think we need to clean this up for sure to not allow modules to do the WB/invalidates. But for now, I want to leave it as is to reduce the amount of changes.

The cache work needs to go in the module core, however, agree we should do teh module conversion first and then do the cache work (when hopefully the Zephyr cache API is ready).

kv2019i · 2022-12-29T17:57:39Z

@wszypelt "System/merge/codecheck" error hit with this one as well..

lyakh · 2023-01-05T09:48:20Z

src/audio/module_adapter/module_adapter.c

I still don't like this. We're in .copy() i.e. in the pipeline task, actually performing streaming, right? These buffers can only be modified when modifying topology, i.e. either when connecting or when disconnecting pipelines. Those actions should also only be done in a pipeline task context. As long as we only allow connected pipelines on the same core, these tasks will run sequentially without preempting each other. So within one scheduling invocation these buffers remain consistent.
Only when we move to connecting different cores, then this will become a problem. But even then this will be a larger problem, requiring proper robust locking. And if we ever hit this condition, it means we were in an inconsistent state and it can bite us elsewhere. These checks seem only to reduce the probability of hitting a problem, making it more difficult to debug and fix. Really don't seem useful to me. Same below of course.

@lyakh I'm fully with you that this does not seem right in copy (we should have less frequent state change callbacks where module can react to changes in its configuration or the configuration of its immediate connections, and NOT do this check in each copy), but it's still a fact that currently this is hit in pause-resume tests (with sof-tests), so it's not like this is a corner case at this point.

oh i missed these 2. fixed now

@lyakh I'm fully with you that this does not seem right in copy (we should have less frequent state change callbacks where module can react to changes in its configuration or the configuration of its immediate connections, and NOT do this check in each copy), but it's still a fact that currently this is hit in pause-resume tests (with sof-tests), so it's not like this is a corner case at this point.

@kv2019i if it's so easy to hit, then we really need to investigate and fix these properly. Maybe someone could add a task to our planning ;-)

kv2019i

Thanks, this is cleaner than my #6909 .

The concern raised by Guennadi on whether copy()/process() has to have so much conditional code, remains on open, but not really specific to this PR. Given I can easily hit these conditions with sof-test, it would seem this requires a separate PR (if someone disagrees, please chime in).

kv2019i · 2023-01-05T16:21:57Z

src/audio/module_adapter/module_adapter.c

Yup, this is cleaner,

Add a helper function to set up the input/output buffers for the 1:1 and N:1 source:sink buffer configuration. Also, expand the module_adapter_copy() function to support the simple_copy flag with the single source and multiple sink buffer configuration. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Replace the comp_drv ops for the mixin component to use the module_adapter interface. The trigger and get_attribute ops are replaced with the module_adapter ops which do the exact same thing. The base_cfg field from struct mixin_data is removed as it is replaced with the base_cfg in struct module_config and all users have been updated. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

…back Some modules like the mixin/mixout do the invalidate/writebacks for the source/sink buffers themselves. So, add flags in struct processing_module to skip doing the same again in the module adapter code. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

In the simple_copy case, source/sink modules that aren't active are skipped when passing the buffers to the process callback. So, produce/consume from only the active buffers that were passed to the process callback as well. Reported-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

src/audio/mixin_mixout/mixin_mixout.c

lyakh · 2023-01-06T07:13:13Z

src/audio/mixin_mixout/mixin_mixout.c

+	base_module_cfg_to_stream_params(&mod->priv.cfg.base_cfg, params);
+
+	/* Buffers between mixins and mixouts are not used (mixin writes data directly to mixout
+	 * sink). But, anyway, let's setup these buffers properly just in case.


the comment could be a bit "kinder" though at least, stating some "better sounding" reason than "just in case," e.g. "for correctness," "for consistency" etc. But yes, it's just a verbatim copy of the current comment

lgirdwood · 2023-01-06T16:59:37Z

@ranj063 btw, what's module adapter plan for N:1 and M:N based processing optimisation ?

ranj063 · 2023-01-06T17:29:03Z

@ranj063 btw, what's module adapter plan for N:1 and M:N based processing optimisation ?

@lgirdwood N:1 is already supported by the module adapter. M:N is the only open. I dont have a line of sight for this one due to the lack of a module for this type. Maybe what we need to think of extending the passthrough module to support M:N confg instead of 1:1 so we can test this. WHat do you think?

lgirdwood · 2023-01-09T15:13:28Z

@ranj063 lets leave M:N then for later.

ranj063 force-pushed the fix/mixin_convert branch 2 times, most recently from bf63c12 to 5e8cb39 Compare December 19, 2022 05:00

ranj063 changed the title ~~Convert the mixin module to use the module adapter~~ [DO NOT REVIEW]Convert the mixin module to use the module adapter Dec 19, 2022

ranj063 force-pushed the fix/mixin_convert branch from 5e8cb39 to b48fb8b Compare December 19, 2022 05:16

ranj063 changed the title ~~[DO NOT REVIEW]Convert the mixin module to use the module adapter~~ Convert the mixin module to use the module adapter Dec 19, 2022

ranj063 marked this pull request as ready for review December 19, 2022 16:43

ranj063 requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners December 19, 2022 16:43

ranj063 mentioned this pull request Dec 19, 2022

Convert mixout to use the module adapter #6833

Merged

ranj063 force-pushed the fix/mixin_convert branch 7 times, most recently from d661254 to b3c89ef Compare December 20, 2022 16:23

serhiy-katsyuba-intel approved these changes Dec 20, 2022

View reviewed changes

ranj063 force-pushed the fix/mixin_convert branch 2 times, most recently from f9929bb to 16133e9 Compare December 20, 2022 18:41

lgirdwood approved these changes Dec 21, 2022

View reviewed changes

ranj063 force-pushed the fix/mixin_convert branch 3 times, most recently from ae5793d to 7b09c9e Compare December 21, 2022 19:04

ranj063 force-pushed the fix/mixin_convert branch from 7b09c9e to d99dc59 Compare December 21, 2022 20:11

ranj063 force-pushed the fix/mixin_convert branch from fc12b41 to cd789e4 Compare December 22, 2022 21:10

lyakh reviewed Dec 23, 2022

View reviewed changes

ranj063 force-pushed the fix/mixin_convert branch from cd789e4 to 15051d8 Compare December 23, 2022 16:20

kv2019i reviewed Dec 29, 2022

View reviewed changes

This was referenced Jan 4, 2023

mixin-mixout: handle congestion corner case without assert #6900

Merged

module_adapter: fix buffer consumption logic in simple_copy mode #6909

Closed

ranj063 force-pushed the fix/mixin_convert branch from 15051d8 to 2d9da91 Compare January 4, 2023 18:09

ranj063 requested review from kv2019i and lyakh January 4, 2023 18:12

kv2019i mentioned this pull request Jan 5, 2023

app: zephyr: add CONFIG_ASSERT debug overlay #6530

Merged

13 tasks

lyakh reviewed Jan 5, 2023

View reviewed changes

kv2019i approved these changes Jan 5, 2023

View reviewed changes

src/audio/module_adapter/module_adapter.c Outdated

Copy link

Collaborator

kv2019i Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this is cleaner,

ranj063 added 4 commits January 5, 2023 09:04

ranj063 force-pushed the fix/mixin_convert branch from 2d9da91 to 5414c49 Compare January 5, 2023 17:05

lyakh approved these changes Jan 6, 2023

View reviewed changes

lgirdwood approved these changes Jan 6, 2023

View reviewed changes

lgirdwood merged commit f8bdef3 into thesofproject:main Jan 6, 2023

ranj063 deleted the fix/mixin_convert branch January 6, 2023 17:29

Convert the mixin module to use the module adapter #6836

Convert the mixin module to use the module adapter #6836

Conversation

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

Uh oh!

Uh oh!

lgirdwood commented Dec 20, 2022

Uh oh!

lgirdwood commented Dec 21, 2022

Uh oh!

ranj063 commented Dec 21, 2022

Uh oh!

ranj063 commented Dec 22, 2022

Uh oh!

ranj063 commented Dec 22, 2022

Uh oh!

ranj063 commented Dec 23, 2022

Uh oh!

ranj063 commented Dec 23, 2022

Uh oh!

ranj063 commented Dec 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Dec 29, 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!

kv2019i left a comment

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!

lgirdwood commented Jan 6, 2023

Uh oh!

ranj063 commented Jan 6, 2023

Uh oh!

lgirdwood commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ranj063 commented Dec 19, 2022 •

edited

Loading