Copier: preparation work for convert copier to module interface by btian1 · Pull Request #7740 · thesofproject/sof

btian1 · 2023-06-05T08:16:34Z

No description provided.

ranj063 · 2023-06-15T00:15:05Z

will it all fit in one line?

ranj063 · 2023-06-15T00:17:27Z

can we move this inside case SOF_COMP_DAI: for the multiendpoint case?

will move once all CI passed.

ranj063 · 2023-06-15T00:18:06Z

for the module copier, will cd->endpoint_num ever be > 0?

it should be 0, also the type is NONE, neither host nor dai.

ranj063 · 2023-06-15T00:20:47Z

endpoint_num will always be 0 for module copier

and instead of checking for bsource_buffer, I'd check the cd->direction

ok, let's remove this part of code, since only endpoint copy need add this data.

ranj063 · 2023-06-15T00:24:02Z

this will likely run into compilation issues because this function is unused. Either you need to call this from copier_copy() or you need convert to module_interface in this patch

yes, right way is add all left code into this patch.

lyakh · 2023-06-16T07:25:52Z

I'm wondering why some .set_configuration() methods invalidate cache and others don't? If it's needed, maybe it should be done centrally in module_set_large_config()? @ranj063

lgirdwood

@btian1 can you check CI thanks.

ranj063 · 2023-07-03T17:35:04Z

@btian1 if you're going to do this, I think we can go further and remove do_enpoint_copy() altogether isnt it? Maybe we can do this ina follow up PR

I use last patch to remove all do_endpoint_copy, please have a check.

lyakh

mostly looks good to me, just one doubt about a potential change in behaviour and a couple of minor comments

lyakh · 2023-07-04T06:57:47Z

this is an unusual name for a "configuration" variable?

followed module_adapter_new, we can change use another patch to unify those usages.

ack, dst is usually used as a destination pointer. We use source or sink, so I would rename to sink_config if this is configuration data for a destination module. If its our local configuration then call it config.

ok, line 68 already have config named, for avoid confusing, I removed this variable.

lyakh · 2023-07-04T07:03:51Z

how about just md->cfg.init_data = spec and remove dst. Or even mod->priv.cfg.init_data = spec and remove both md and dst

In assembly level, there is no difference at all for both implementation.

Yes, dst is confusing name for me.

@lyakh @lgirdwood , use md->cfg.init_data = spec to remove dst usage, mod->priv.cfg.init_data = spec this is a little long, I prefer use md->cfg.init_data = spec.

In assembly level, there is no difference at all for both implementation.

Sure, but we aren't programming in assembly. There are many ways to write code in C to generate the same assembly output. You can name your variables a1, a2 etc. but that's not how we're programming

fully agree, please review the changes, and let me know if you still have further comments.

lyakh · 2023-07-04T07:12:16Z

what was wrong with the original line 637?

you are right, nothing changed, let me change back, don't remember why have this change.

lgirdwood · 2023-07-04T10:55:45Z

ack, dst is usually used as a destination pointer. We use source or sink, so I would rename to sink_config if this is configuration data for a destination module. If its our local configuration then call it config.

lgirdwood · 2023-07-04T10:57:53Z

Yes, dst is confusing name for me.

Set dev private as module interface, create new copier_init to meet module interface requirement, and change all copier_data related code. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Add new function to implement free and reset module interface. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Use module parameter instead of device parameters. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Use standard module set and get config interface in copier. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

Convert copier unbind to module interface. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

This will be used to separate multi-endpoint DAI copy from the module copier copy. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

In preparation for converting the copier to use the module interface, split up the multi-endpoint DAI copy and the module copier copy cases. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

It is only used in the multi-endpoint case, so use the multi_endpoint_buffer directly. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

This is 1ms function calling, remove one more layer to get better performance. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

lyakh

we can address the comments in a follow-up, and the problem with the .init_data is larger than just the copier

lyakh · 2023-07-07T10:13:45Z

 	 */
-	if (memcpy_s(&cd->config, sizeof(cd->config), copier, sizeof(*copier)) < 0)
-		goto error_cd;
+	if (memcpy_s(&cd->config, sizeof(cd->config), copier, sizeof(*copier)) < 0) {


actually it would be better to propagate the error:

ret = memcpy_s(&cd->config, sizeof(cd->config), copier, sizeof(*copier)); if (ret < 0) goto error;

Can be done in a follow up, unless you anyway submit an update to this PR

lyakh · 2023-07-07T10:15:30Z

-		comp_cl_err(&comp_copier, "pipeline %d is not existed", config->pipeline_id);
-		goto error_cd;
+		comp_err(dev, "pipeline %d is not existed", config->pipeline_id);
+		ret = -EPIPE;


I think EPIPE means "broken pipeline." Here it's more like ENODEV / ENOENT?

lyakh · 2023-07-07T10:31:38Z

+	comp_set_drvdata(dev, mod);
+	md = &mod->priv;
+	mod->dev = dev;
+	md->cfg.init_data = spec;


we should remove all these. spec points to the mailbox, data there will change, so storing a pointer to it isn't a good idea. The module adapter does that too and has to be fixed as well.

lyakh · 2023-07-07T11:57:32Z

-		break;
-	case SOF_COMP_DAI:
-		if (cd->endpoint_num == 1)
-			return dai_common_copy(cd->dd[0], dev, cd->converter);


this case is already handled somewhere, right?

lgirdwood

@ranj063 good for you ?

ranj063 · 2023-07-07T15:23:19Z

@ranj063 good for you ?

@lgirdwood yes already approved

btian1 changed the title ~~Copier modulerinterface~~ Copier: convert copier to module interface Jun 5, 2023

btian1 force-pushed the copier_modulerinterface branch from 91196fc to 222ca47 Compare June 5, 2023 13:00

ranj063 reviewed Jun 8, 2023

View reviewed changes

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

btian1 force-pushed the copier_modulerinterface branch 8 times, most recently from 87556ea to d5ead7c Compare June 14, 2023 03:39

lyakh reviewed Jun 14, 2023

View reviewed changes

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

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

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

ranj063 reviewed Jun 15, 2023

View reviewed changes

btian1 force-pushed the copier_modulerinterface branch 3 times, most recently from dfd81d5 to abf7ed6 Compare June 15, 2023 06:58

lyakh reviewed Jun 16, 2023

View reviewed changes

lyakh reviewed Jun 20, 2023

View reviewed changes

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

btian1 force-pushed the copier_modulerinterface branch 3 times, most recently from 0daed68 to 61bdee1 Compare June 30, 2023 02:24

btian1 changed the title ~~Copier: convert copier to module interface~~ Copier: preparation work for convert copier to module interface Jun 30, 2023

btian1 marked this pull request as ready for review June 30, 2023 02:30

btian1 requested review from RanderWang, abonislawski, jxstelter, lgirdwood, mmaka1, pblaszko and plbossart as code owners June 30, 2023 02:30

ranj063 approved these changes Jun 30, 2023

View reviewed changes

RanderWang approved these changes Jul 3, 2023

View reviewed changes

btian1 force-pushed the copier_modulerinterface branch 2 times, most recently from 0d06602 to 6a049b3 Compare July 3, 2023 13:23

lgirdwood reviewed Jul 3, 2023

View reviewed changes

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

ranj063 reviewed Jul 3, 2023

View reviewed changes

btian1 force-pushed the copier_modulerinterface branch from 6a049b3 to 16d2cfe Compare July 4, 2023 01:27

ranj063 mentioned this pull request Jul 4, 2023

Convert copier to use the module interface #7890

Merged

lyakh reviewed Jul 4, 2023

View reviewed changes

btian1 requested review from lgirdwood and lyakh July 4, 2023 08:41

btian1 force-pushed the copier_modulerinterface branch from 577de5c to 86019ec Compare July 4, 2023 08:47

lgirdwood reviewed Jul 4, 2023

View reviewed changes

btian1 added 5 commits July 5, 2023 09:38

copier: change copier new with module interface

09507a0

Set dev private as module interface, create new copier_init to meet module interface requirement, and change all copier_data related code. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

copier: change free and reset to module interface

b721aa8

Add new function to implement free and reset module interface. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

copier: change prepare and parameter to module interface

aef2a5a

Use module parameter instead of device parameters. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

copier: change set/get config to module interface

0fa89ee

Use standard module set and get config interface in copier. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

copier: convert unbind to module interface

6d52fe1

Convert copier unbind to module interface. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

btian1 force-pushed the copier_modulerinterface branch from 86019ec to e0f6e19 Compare July 5, 2023 01:43

btian1 requested a review from lgirdwood July 5, 2023 01:43

btian1 force-pushed the copier_modulerinterface branch from e0f6e19 to 5a1331a Compare July 6, 2023 06:12

ranj063 and others added 4 commits July 6, 2023 14:24

audio: copier: Add a helper function to copy to all sinks

703ba07

This will be used to separate multi-endpoint DAI copy from the module copier copy. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

audio: copier: Remove get_endpoint_buffer()

a18f062

It is only used in the multi-endpoint case, so use the multi_endpoint_buffer directly. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

copier: remove do_endpoint_copy function

5a1331a

This is 1ms function calling, remove one more layer to get better performance. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>

lyakh approved these changes Jul 7, 2023

View reviewed changes

lgirdwood approved these changes Jul 7, 2023

View reviewed changes

lgirdwood merged commit 8937929 into thesofproject:main Jul 7, 2023

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

btian1 commented Jun 5, 2023

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!

btian1 Jun 15, 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!

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!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ranj063 Jul 3, 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lyakh left a comment

btian1 Jun 15, 2023 •

edited

Loading

ranj063 Jul 3, 2023 •

edited

Loading