ASoC: SOF: ipc4-topology: add copier module type support #3888

bardliao · 2022-09-28T11:30:20Z

The corresponding IPC4 module of snd_soc_dapm_buffer widget is module
-to-module copier.
The module-to-module copier is a buffer-like component with demuxing
capabilities.
Rename the host_token_list to common_copier_token_list since it will
be used by host copier and module-to-module copier.
The setup callback is almost the same as sof_ipc4_widget_setup_pcm except
the gtw_cfg data, and the free callback is exactly the same as
sof_ipc4_widget_free_comp_pcm. To reduce the duplication, the commit
reuses the setup and free callbacks.

The topology PR is thesofproject/sof#6329.

@RanderWang @aiChaoSONG @libinyang Could you take a look?

aiChaoSONG · 2022-09-29T02:10:27Z

Do we have firmware log here? what firmware are you used?

RanderWang · 2022-09-29T02:10:47Z

@bardliao As I suggested, the last four dword should be "000000, ffffffff, 0000000, 000000" // invalid node id for non-gtw

[  118.230600] Message payload: 00000000: 0000066f 00000180 00000180 00000001
[  118.230602] Message payload: 00000010: 0000bb80 00000020 ffffff10 00000001
[  118.230604] Message payload: 00000020: 00000000 00002002 0000bb80 00000020
[  118.230606] Message payload: 00000030: ffffff10 00000001 00000000 00002002
[  118.230608] Message payload: 00000040: 00000000 00000000 00000300 00000000

bardliao · 2022-09-30T12:04:26Z

@bardliao As I suggested, the last four dword should be "000000, ffffffff, 0000000, 000000" // invalid node id for non-gtw

[  118.230600] Message payload: 00000000: 0000066f 00000180 00000180 00000001
[  118.230602] Message payload: 00000010: 0000bb80 00000020 ffffff10 00000001
[  118.230604] Message payload: 00000020: 00000000 00002002 0000bb80 00000020
[  118.230606] Message payload: 00000030: ffffff10 00000001 00000000 00002002
[  118.230608] Message payload: 00000040: 00000000 00000000 00000300 00000000

Thanks @RanderWang I will try it.

bardliao · 2022-10-03T07:25:20Z

@bardliao As I suggested, the last four dword should be "000000, ffffffff, 0000000, 000000" // invalid node id for non-gtw

[  118.230600] Message payload: 00000000: 0000066f 00000180 00000180 00000001
[  118.230602] Message payload: 00000010: 0000bb80 00000020 ffffff10 00000001
[  118.230604] Message payload: 00000020: 00000000 00002002 0000bb80 00000020
[  118.230606] Message payload: 00000030: ffffff10 00000001 00000000 00002002
[  118.230608] Message payload: 00000040: 00000000 00000000 00000300 00000000

Thanks @RanderWang I will try it.

Thanks @RanderWang. Your suggestion works with SOF.

@plbossart @ranj063 @RanderWang I have an open on this PR. What dapm type should we use for the "module" type copier? It is snd_soc_dapm_buffer in this PR, but I am wondering is it the best one?

sound/soc/sof/ipc4-topology.c

aiChaoSONG · 2022-10-09T02:54:00Z

What dapm type should we use for the "module" type copier? It is snd_soc_dapm_buffer in this PR, but I am wondering is it the best one?

I think it should be snd_soc_dapm_effect, because in this circumstance, the copier works just like a demux, and I think its init and configure can be covered by Libin's PR: #3755. I am just wondering if we really want to have special code for module copier?

plbossart · 2022-10-10T14:09:58Z

What's wrong with:

	snd_soc_dapm_demux,			/* connects the input to one of multiple outputs */

That's exactly what a copier does, doesn't it?

bardliao · 2022-10-11T03:39:43Z

What dapm type should we use for the "module" type copier? It is snd_soc_dapm_buffer in this PR, but I am wondering is it the best one?

I think it should be snd_soc_dapm_effect, because in this circumstance, the copier works just like a demux, and I think its init and configure can be covered by Libin's PR: #3755. I am just wondering if we really want to have special code for module copier?

@aiChaoSONG We already support copier module for host and dai. We can reuse the existing code if we add its ops in tplg_ipc4_widget_ops. And IMHO, demux is not an effect.

What's wrong with:
	snd_soc_dapm_demux,			/* connects the input to one of multiple outputs */
That's exactly what a copier does, doesn't it?

@plbossart In this specific use case, yes. But copier can also simply do format conversion. And I would like to use the same sof_ipc_tplg_widget_ops for both use cases.

RanderWang · 2022-10-11T05:42:17Z

What's wrong with:
	snd_soc_dapm_demux,			/* connects the input to one of multiple outputs */
That's exactly what a copier does, doesn't it?

copier can accept 1 input and duplicate to max 4 outputs

ranj063 · 2022-10-14T02:19:42Z

What's wrong with:
	snd_soc_dapm_demux,			/* connects the input to one of multiple outputs */
That's exactly what a copier does, doesn't it?

@plbossart the demux we have in SOF need kcontrol data isnt it whereas the copier doesnt it. If we call this type of copier a demux, we'll need to differentiate between the SOF demux component and the copier with UUIDs. I suppose its not so bad but I somehow like the "buffer" type better

bardliao · 2022-10-14T03:05:20Z

We can't use snd_soc_dapm_demux as there is no such tplg dapm type defined for now.
https://github.com/torvalds/linux/blob/master/include/uapi/sound/asoc.h#L56
https://github.com/alsa-project/alsa-lib/blob/master/src/topology/dapm.c#L24
So I will go with snd_soc_dapm_buffer

RanderWang

LGTM

bardliao · 2022-10-28T06:02:28Z

@plbossart Could you review it? Thanks.

ujfalusi

@bardliao, this "copier module" thing is basically a different configuration of the copier module, right?
We have copier on the DAI side, we have copier on the AIF side and this is somewhere else, likely in the middle.

It is a bit confusing to me at least the fact that we have separate setup and free callbacks for the two other type of copiers (this patch does not adds copier type support, it adds a buffer type of the copier) while you are re-using the AIF copier for the buffer copier.

I think it would be cleaner to just add a new setup and free for the buffer type of copier?

I would probably extend the commit message a bit since this is not obvious a bit.

The "module copier" is as meaningless as it can get, imho. I won't block this PR, but I really feel that it can be done cleaner.

ranj063 · 2022-10-28T13:50:18Z

It is a bit confusing to me at least the fact that we have separate setup and free callbacks for the two other type of copiers (this patch does not adds copier type support, it adds a buffer type of the copier) while you are re-using the AIF copier for the buffer copier.

@ujfalusi I asked to merge the set/free for cmodule copier and host copier parts. There was just too much code duplication with separate callbacks

bardliao · 2022-10-31T05:47:02Z

@bardliao, this "copier module" thing is basically a different configuration of the copier module, right?

Yes

We have copier on the DAI side, we have copier on the AIF side and this is somewhere else, likely in the middle.

Yes

It is a bit confusing to me at least the fact that we have separate setup and free callbacks for the two other type of copiers (this patch does not adds copier type support, it adds a buffer type of the copier) while you are re-using the AIF copier for the buffer copier.

I think it would be cleaner to just add a new setup and free for the buffer type of copier?

DAI and host copiers have separate setup and free callbacks because the callbacks don't look so similar.
However, the setup callbacks of module and host copiers are almost the same except the gtw_cfg data.
And the free callbacks of module and host copiers are exactly the same.
I call it module type instead of buffer type is because I would like to be aligned with topology definition.
See https://github.com/thesofproject/sof/blob/main/tools/topology/topology2/include/components/copier.conf#L50
The copier types are

				"HDA"
				"host"
				"SSP"
				"ALH"
				"DMIC"
				"module"

snd_soc_dapm_buffer is a dapm widget type and I use it to map to the module copier.

I would probably extend the commit message a bit since this is not obvious a bit.

Sure, I will try to explain it more in the commit message.

The "module copier" is as meaningless as it can get, imho. I won't block this PR, but I really feel that it can be done cleaner.

ujfalusi · 2022-10-31T07:24:40Z

It is a bit confusing to me at least the fact that we have separate setup and free callbacks for the two other type of copiers (this patch does not adds copier type support, it adds a buffer type of the copier) while you are re-using the AIF copier for the buffer copier.
I think it would be cleaner to just add a new setup and free for the buffer type of copier?

DAI and host copiers have separate setup and free callbacks because the callbacks don't look so similar. However, the setup callbacks of module and host copiers are almost the same except the gtw_cfg data. And the free callbacks of module and host copiers are exactly the same. I call it module type instead of buffer type is because I would like to be aligned with topology definition. See https://github.com/thesofproject/sof/blob/main/tools/topology/topology2/include/components/copier.conf#L50 The copier types are
				"HDA"
				"host"
				"SSP"
				"ALH"
				"DMIC"
				"module"

But a horrible and meaningless name in firmware does no mean it is good, again imho.
Think about it: this is the copier module's module mode configuration. What?
We name the dai configuration of the copier module' as dai_copier instead of hda/ssp/alh/dmic_copier

Codewise I don't have issue with the PR, but I opt to not approve which is not to say that this should not be merged, but to say that I really not on the let's call the buffer (mux/mix/passthrough/conversion) mode of the copier module as module.

snd_soc_dapm_buffer is a dapm widget type and I use it to map to the module copier.

I would probably extend the commit message a bit since this is not obvious a bit.

Sure, I will try to explain it more in the commit message.

Thanks!

The "module copier" is as meaningless as it can get, imho. I won't block this PR, but I really feel that it can be done cleaner.

ranj063 · 2022-11-01T16:58:05Z

@bardliao copier.module doesnt bring out the essensce of what this copier does. I understand where you're coming from ie the tester interface that uses this name. But if you want to convince reviewers and make it clearer, why not call it copier.module_to_module.blah. That way you know that it is a copier that goes between 2 other modules instead of DAI/host copier that are endpoint copiers

plbossart

It seems that for this buffer handling sometimes we use AIF helpers and sometimes DAI tokens. This looks rather weird. It looks rather ad-hoc and could be clarified IMHO

plbossart · 2022-11-01T18:01:02Z

sound/soc/sof/ipc4-topology.c

why are we using a dai_token_list for something that is not a DAI?

This sounds bad to me, I would guess some tokens apply only for real DAIs and not for internal buffer handling?

why are we using a dai_token_list for something that is not a DAI?

This sounds bad to me, I would guess some tokens apply only for real DAIs and not for internal buffer handling?

Thanks @plbossart change it to host_token_list now.

why are we using a dai_token_list for something that is not a DAI?
This sounds bad to me, I would guess some tokens apply only for real DAIs and not for internal buffer handling?

Thanks @plbossart change it to host_token_list now.

And rename it to common_copier_token_list after chatting with Ranjani.

plbossart · 2022-11-01T18:05:34Z

sound/soc/sof/ipc4-topology.c

I am a bit lost here, we are trying to reuse the same setup_pcm for AIF and internal buffer handling. So do we need a dma_buffer_size here? what does this mean for an internal buffer. Should this test be moved to line 349 instead?

I am a bit lost here, we are trying to reuse the same setup_pcm for AIF and internal buffer handling. So do we need a dma_buffer_size here? what does this mean for an internal buffer. Should this test be moved to line 349 instead?

We don't need to get dma_buffer_size from topology. It should be always 0. But we need to allocate available_fmt->dma_buffer_size and it will be used in sof_ipc4_prepare_copier_module https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/ipc4-topology.c#L1358

you should capture this with a comment, this is useful information that explains the code structure.
this can be done with a fixup

The corresponding IPC4 module of snd_soc_dapm_buffer widget is module -to-module copier. The module-to-module copier is a buffer-like component with demuxing capabilities. Rename the host_token_list to common_copier_token_list since it will be used by host copier and module-to-module copier. The setup callback is almost the same as sof_ipc4_widget_setup_pcm except the gtw_cfg data, and the free callback is exactly the same as sof_ipc4_widget_free_comp_pcm. To reduce the duplication, the commit reuses the setup and free callbacks. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

plbossart · 2022-11-02T12:42:18Z

sound/soc/sof/ipc4-topology.c

you should capture this with a comment, this is useful information that explains the code structure.
this can be done with a fixup

ujfalusi

@bardliao, thank you for the updates, looks great!

plbossart · 2022-11-03T19:51:45Z

SOFCI TEST

plbossart · 2022-11-04T17:50:42Z

no regressions beyond known issues, merging

bardliao requested review from RanderWang, aiChaoSONG, libinyang, plbossart and ranj063 September 28, 2022 11:30

bardliao force-pushed the copier-module-type branch 3 times, most recently from 9b949ab to 381bc16 Compare October 3, 2022 09:48

ranj063 reviewed Oct 3, 2022

View reviewed changes

sound/soc/sof/ipc4-topology.c Outdated Show resolved Hide resolved

sound/soc/sof/ipc4-topology.c Outdated Show resolved Hide resolved

bardliao force-pushed the copier-module-type branch from 381bc16 to 99f3dd4 Compare October 14, 2022 01:46

bardliao force-pushed the copier-module-type branch from 99f3dd4 to bb134dc Compare October 14, 2022 03:01

bardliao changed the title ~~[TEST] ASoC: SOF: ipc4-topology: add copier module type support~~ ASoC: SOF: ipc4-topology: add copier module type support Oct 14, 2022

bardliao force-pushed the copier-module-type branch 2 times, most recently from 427e0c0 to abc9f38 Compare October 14, 2022 06:17

bardliao marked this pull request as ready for review October 14, 2022 06:32

bardliao requested review from ajitkupandey, dbaluta, kv2019i, lgirdwood and ujfalusi as code owners October 14, 2022 06:32

RanderWang previously approved these changes Oct 26, 2022

View reviewed changes

ranj063 previously approved these changes Oct 26, 2022

View reviewed changes

bardliao mentioned this pull request Oct 28, 2022

Topology2: add ssp multi stream capture thesofproject/sof#6329

Merged

ujfalusi reviewed Oct 28, 2022

View reviewed changes

bardliao dismissed stale reviews from ranj063 and RanderWang via 836d4dc October 31, 2022 06:21

bardliao force-pushed the copier-module-type branch from 7843ada to 836d4dc Compare October 31, 2022 06:21

ranj063 previously approved these changes Nov 1, 2022

View reviewed changes

RanderWang previously approved these changes Nov 1, 2022

View reviewed changes

plbossart requested changes Nov 1, 2022

View reviewed changes

bardliao dismissed stale reviews from RanderWang and ranj063 via e5171d9 November 2, 2022 01:52

bardliao force-pushed the copier-module-type branch from 836d4dc to e5171d9 Compare November 2, 2022 01:52

bardliao force-pushed the copier-module-type branch from e5171d9 to 305aff1 Compare November 2, 2022 05:32

RanderWang approved these changes Nov 2, 2022

View reviewed changes

plbossart approved these changes Nov 2, 2022

View reviewed changes

plbossart requested review from ranj063 and ujfalusi November 2, 2022 12:44

ranj063 approved these changes Nov 2, 2022

View reviewed changes

ujfalusi approved these changes Nov 3, 2022

View reviewed changes

plbossart mentioned this pull request Nov 3, 2022

ASoC: SOF: Don't read reply if its not used #3925

Merged

plbossart merged commit 69222fa into thesofproject:topic/sof-dev Nov 4, 2022

ASoC: SOF: ipc4-topology: add copier module type support #3888

ASoC: SOF: ipc4-topology: add copier module type support #3888

Uh oh!

Conversation

bardliao commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aiChaoSONG commented Sep 29, 2022

Uh oh!

RanderWang commented Sep 29, 2022

Uh oh!

bardliao commented Sep 30, 2022

Uh oh!

bardliao commented Oct 3, 2022

Uh oh!

Uh oh!

Uh oh!

aiChaoSONG commented Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Oct 10, 2022

Uh oh!

bardliao commented Oct 11, 2022

Uh oh!

RanderWang commented Oct 11, 2022

Uh oh!

ranj063 commented Oct 14, 2022

Uh oh!

bardliao commented Oct 14, 2022

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

bardliao commented Oct 28, 2022

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Oct 28, 2022

Uh oh!

bardliao commented Oct 31, 2022

Uh oh!

ujfalusi commented Oct 31, 2022

Uh oh!

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

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

bardliao Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

bardliao Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

plbossart Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

bardliao Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

plbossart Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

plbossart Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart commented Nov 3, 2022

Uh oh!

plbossart commented Nov 4, 2022

Uh oh!

Reviewers

Assignees

bardliao commented Sep 28, 2022 •

edited

Loading

aiChaoSONG commented Oct 9, 2022 •

edited

Loading

ranj063 commented Nov 1, 2022 •

edited

Loading