Convert copier to use the module interface by ranj063 · Pull Request #7890 · thesofproject/sof

ranj063 · 2023-07-04T05:03:09Z

Includes patches from #7740

lgirdwood

LGTM, just minor comments.

RanderWang

LGTM

Add the bind/unbind ops that can be optionally implemented by modules to perform module-specific actions when other modules are bound/unbound from them. These are particularly useful for modules such as the mixers, smart_amp, copier that can have multiple input/output pins. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Introduce a new struct module_endpoint_ops which contains all the ops that are relevant for endpoint devices (host copier and DAI copier) and add a new field for setting the endpoint_ops. These will be needed when the copier component is converted to use the module adapter interface. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

ranj063 · 2023-07-09T20:33:12Z

SOFCI TEST

lyakh · 2023-07-10T13:35:05Z

if I understand correctly, this call to buffer_set_params() is now practically dropped, was it not needed?

it isnt dropped, it is handled by ipc4_update_buffer_format() now

Add the dai endpoint ops to the module adapter in preparation for converting the copier module to use the module interface. Also, modify the module_adapter ops to handle the HOST/DAI components are a special case. In the case of module_adapter_prepare(), there's no need to compute the period_bytes for the HOST/DAI because these are computed by the copier module prepare op. In the case of copy, HOST/DAI components are handled separately as well as these components deal with the DMA buffers. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

The state check is redundant as the state will always be >= MODULE_INITIALIZED if the module exists. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

For IPC4, the component device state machine is handled by the comp_set_state() calls in the module adapter ops already. So, the module state checks are redundant. But, keep the state checks for IPC3 so as to not break the existing codec implementations for IPC3. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Define the module interface ops and the endpoint ops for the copier module to convert it to use the module interface. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

It will be needed by other modules. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Update the pipeline params based on the base config. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

It will be needed by other modules as well. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Fix the function name in the comment. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

The input stream params may not be updated by the source module. So update the stream params from the config. Also, update the source/sink ibuffer formats based on the base config audio format. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Use the helper functions to update the stream params and the source/sink buffer formats. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

btian1 · 2023-07-12T05:22:30Z

 	buffer_release(sink_c);

-	return ret;
+	sourceb = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list);


previous code only have sink buffer update, does not have source buffer update, just want to avoid some parameter missing caused issues? do we need add source update to each module?

for the volume component, the source buffer and sink buffer formats are exactly the same. So yes we should update both buffers. And should we do it for each module individually? Probably not, something we should work on unifying for all modules in IPC4 in a follow up PR

so, any audio processing module, like iir, fir they also need assume source and sink buffer have same format, right?

lyakh

minor comments / questions only

lyakh · 2023-07-12T15:04:04Z

+	if (ret < 0)
+		return ret;
+
+	comp_info(dev, "copier_prepare()");


can we keep it comp_dbg()?

I changed to info on purpose @lyakh mainly because prepare is a one time thing and it is useful to see which components are done getting pepared during trigger

lyakh · 2023-07-12T15:14:43Z


+#if CONFIG_IPC_MAJOR_4
+	ipc4_base_module_cfg_to_stream_params(&mod->priv.cfg.base_cfg, params);
+	ipc4_base_module_cfg_to_stream_params(&mod->priv.cfg.base_cfg, mod->stream_params);


for my understanding: why is this needed? Is this fixing a bug?

Yes, now that the copier is not using the comp_driver, its params wont get called until prepare. So the params will not propagated if we wont do this and components that do not use the module adapter yet will fail

does that mean that that before this commit copier.c is broken, which breaks bisection? Should this commit be merged with 6bcaf89 which comes before it?

lyakh · 2023-07-12T15:16:07Z

+	/* update the source/sink buffer formats. Sink rate will be modified below */
+	ipc4_update_buffer_format(source_c, &cd->ipc_config.base.audio_fmt);
+	ipc4_update_buffer_format(sink_c, &cd->ipc_config.base.audio_fmt);
+#endif


same here - what's the effect of these changes?

same explanation, copier doesnt set the source/sink buffer until prepare. So asrc_params( will fail without it

lgirdwood · 2023-07-12T15:54:09Z

SOFCI TEST

lgirdwood · 2023-07-12T15:54:57Z

@ranj063 pls merge after CAVS CI finishes, I've restarted it as it was pending for 1 day.

ranj063 · 2023-07-12T16:06:11Z

@ranj063 pls merge after CAVS CI finishes, I've restarted it as it was pending for 1 day.

@lgirdwood even though it shows as pending, if you click on the link, you can see the results. I have notified @fredoh9 @marc-hb about the problem

ranj063 requested review from RanderWang, abonislawski, dbaluta, jxstelter, lbetlej, lgirdwood, mmaka1, pblaszko and plbossart as code owners July 4, 2023 05:03

ranj063 marked this pull request as draft July 4, 2023 05:04

btian1 reviewed Jul 4, 2023

View reviewed changes

Comment thread src/audio/module_adapter/module_adapter.c Outdated

ranj063 force-pushed the pr7740_2 branch 2 times, most recently from 0e3f502 to 35fbe85 Compare July 5, 2023 01:44

RanderWang reviewed Jul 5, 2023

View reviewed changes

Comment thread src/include/sof/audio/module_adapter/module/module_interface.h Outdated

lgirdwood reviewed Jul 5, 2023

View reviewed changes

Comment thread src/audio/copier/copier.c Outdated

Comment thread src/audio/copier/copier.c Outdated

Comment thread src/audio/module_adapter/module_adapter.c Outdated

ranj063 force-pushed the pr7740_2 branch 4 times, most recently from 69de002 to 187b02d Compare July 6, 2023 00:19

RanderWang approved these changes Jul 6, 2023

View reviewed changes

ranj063 force-pushed the pr7740_2 branch 7 times, most recently from 54f27cd to ab2291e Compare July 7, 2023 19:53

ranj063 added 2 commits July 7, 2023 12:53

ranj063 force-pushed the pr7740_2 branch from ab2291e to 13d0e0f Compare July 7, 2023 20:12

ranj063 marked this pull request as ready for review July 9, 2023 18:52

ranj063 requested review from aborisovich and marcinszkudlinski as code owners July 9, 2023 18:52

ranj063 force-pushed the pr7740_2 branch from df7cee5 to e3a76de Compare July 9, 2023 22:12

btian1 reviewed Jul 10, 2023

View reviewed changes

Comment thread src/audio/module_adapter/module/generic.c Outdated

Comment thread src/audio/copier/copier.c Outdated

Comment thread src/audio/copier/copier.c Outdated

Comment thread src/audio/copier/copier_host.c Outdated

lyakh reviewed Jul 10, 2023

View reviewed changes

ranj063 added 10 commits July 10, 2023 16:51

audio: volume: Remove state check

a581267

The state check is redundant as the state will always be >= MODULE_INITIALIZED if the module exists. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

audio: copier: Convert module to use module interface

6bcaf89

Define the module interface ops and the endpoint ops for the copier module to convert it to use the module interface. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ipc4: base-config: Expose ipc4_base_module_cfg_to_stream_params()

d3055f9

It will be needed by other modules. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

audio: module_adapter: Update stream params

44ec642

Update the pipeline params based on the base config. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ipc4: Expose ipc4_update_buffer_format()

79d82aa

It will be needed by other modules as well. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

audio: asrc: Fix a typo

057c46f

Fix the function name in the comment. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

audio: volume: Simplify volume_params()

d6e4583

Use the helper functions to update the stream params and the source/sink buffer formats. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the pr7740_2 branch from 081171d to d6e4583 Compare July 10, 2023 23:56

btian1 approved these changes Jul 12, 2023

View reviewed changes

btian1 reviewed Jul 12, 2023

View reviewed changes

lgirdwood approved these changes Jul 12, 2023

View reviewed changes

kv2019i approved these changes Jul 12, 2023

View reviewed changes

lyakh approved these changes Jul 12, 2023

View reviewed changes

lgirdwood merged commit 385110c into thesofproject:main Jul 13, 2023

marc-hb mentioned this pull request Jul 14, 2023

module_interface.h: add forward struct comp_dev and timestamp_data #7945

Merged

btian1 mentioned this pull request Jul 14, 2023

module adapter and copier performance optimization #7909

Merged

btian1 mentioned this pull request Aug 11, 2023

[BUG]: [Multiple Capture - No-codec mode]: Observed dragging and metallic noise from data captured from cascaded (second level) copier #7074

Closed

Conversation

ranj063 commented Jul 4, 2023

Uh oh!

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Jul 9, 2023

Uh oh!

Uh oh!

Uh oh!

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!

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

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 Jul 12, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Jul 12, 2023

Uh oh!

lgirdwood commented Jul 12, 2023

Uh oh!

ranj063 commented Jul 12, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ranj063 Jul 12, 2023 •

edited

Loading