src: codec_adapter: Add process passthrough path #3766

johnylin76 · 2021-01-20T16:03:33Z

If the passthrough flag is enabled, on process() the data in the input
buffer should be directly copied to the output buffer.

We should determine how and when to set the passthrough flag.

src/audio/codec_adapter/codec/generic.c

dbaluta · 2021-01-20T16:42:16Z

src/audio/codec_adapter/codec/generic.c

@@ -261,7 +277,12 @@ int codec_process(struct comp_dev *dev)
 		return -EPERM;
 	}

-	ret = codec->ops->process(dev);
+	// TODO: how to set cd->passthrough flag?
+	if (cd->passthrough) {


via topology?

Yea, but we have no example of enum or on/off controls I think is why it's left as a todo

@juimonen any example so thi scan be added into topology and code here ?

we have examples of enums in tdfb, @johnylin76 please us those

cujomalainey

I realized that we may need to add a "passthrough_possible" flag. for codecs that more "encodecs/decoders" (e.g. MP3 decoder). We could do a simple source_hwparams == sink_hwparams as a check

lgirdwood

I assume the use case here is runtime switching into and out of passthrough mode ?

lgirdwood · 2021-01-22T15:20:47Z

src/audio/codec_adapter/codec/generic.c

@@ -261,7 +277,12 @@ int codec_process(struct comp_dev *dev)
 		return -EPERM;
 	}

-	ret = codec->ops->process(dev);
+	// TODO: how to set cd->passthrough flag?
+	if (cd->passthrough) {


@juimonen any example so thi scan be added into topology and code here ?

dbaluta · 2021-01-22T15:49:43Z

I assume the use case here is runtime switching into and out of passthrough mode ?

OK, makes sense. If the switching is a runtime I think the best way is to forget about doing this via topology file and use a kcontrol widget associated with codec adapter component.

cujomalainey · 2021-01-22T16:51:28Z

I assume the use case here is runtime switching into and out of passthrough mode ?

OK, makes sense. If the switching is a runtime I think the best way is to forget about doing this via topology file and use a kcontrol widget associated with codec adapter component.

Yes, that is the plan is to do this via runtime controls, not topology. So as long as the process is not doing any any sort of format conversion (i.e. sink_hw_params == source_hw_params) we can shut of the processing easily from userspace regardless of whether the codec added a passthrough mode

johnylin76 · 2021-01-25T07:39:40Z

I assume the use case here is runtime switching into and out of passthrough mode ?

OK, makes sense. If the switching is a runtime I think the best way is to forget about doing this via topology file and use a kcontrol widget associated with codec adapter component.

Yes, that is the plan is to do this via runtime controls, not topology. So as long as the process is not doing any any sort of format conversion (i.e. sink_hw_params == source_hw_params) we can shut of the processing easily from userspace regardless of whether the codec added a passthrough mode

Agree to the concept "passthrough_possible". However I suspect current codec adapter is still specified to sample processing (not including encoder/decoder). For example, ca_config only stores channels, one sample_width, and one sample_rate, and cpd provides no information but data. (I am surprised that there is no format (S16, S24,...) information for codec.) So I think that the encoder/decoder might be probably not considered by codec adapter?

"passthrough" flag should be a runtime control in generic layer (like ca_config) rather than in codec layer. However runtime config now only supports params in codec layer. I think we should create a ca_runtime_config, or preserve an id of codec_param for it?

lgirdwood · 2021-02-05T17:00:36Z

@plbossart @johnylin76 @cujomalainey @dbaluta this actually gave me an idea when we are pass a nonfree processing component UUID in a topology (where there are no nonfree componentes in the FW) we can assign it to this passthrough (and emit obvious FW / kernel warnings). Topology will still load, user will get sound albeit without the 3rd party processing.

cujomalainey · 2021-02-05T17:18:03Z

That would actually optimize our fallback mechanism for closed source processing when using the open source builds. Excellent idea. So we should default the adapter to Y in this case?

lgirdwood · 2021-02-09T17:27:53Z

That would actually optimize our fallback mechanism for closed source processing when using the open source builds. Excellent idea. So we should default the adapter to Y in this case?

Probably yes (on CAVS platforms). It's not huge. @mrajwa any inputs here ?

mrajwa · 2021-02-09T19:11:53Z

So I think that the encoder/decoder might be probably not considered by codec adapter?
However I suspect current codec adapter is still specified to sample processing (not including encoder/decoder)

codec_adapter, as the name suggest doesn't perform any encoding/deciding by itself. So if you need some additional, not already present variables struct codec_data and in particular its *private member is the place to go.

Probably yes (on CAVS platforms). It's not huge. @mrajwa any inputs here ?

I am OK with the passthru default setting in the topology which can be overloaded by RUNTIME params.

cujomalainey · 2021-02-09T19:20:03Z

I think the idea here is to go past params. E.g. if topology asks for codec adapter with cadence core but firmware doesnt have cadence core but does have codec adapter, then adapter would install in passthrough mode just to keep audio working.

mrajwa · 2021-02-09T19:40:31Z

I think the idea here is to go past params. E.g. if topology asks for codec adapter with cadence core but firmware doesnt have cadence core but does have codec adapter, then adapter would install in passthrough mode just to keep audio working.

Hmm but if FW has codec_adapter on some pipeline then it should be tied with some processing lib right? What's the point of having "dummy" adapter with no lib bounded? I am also not sure about "keep audio working" part, can you describe this situation?

cujomalainey · 2021-02-09T19:51:08Z

Sure, hypothetical situation. Chromeos launches device with processing lib. Due to licensing restrictions, final sof firmware build is only part of private chromeos builds. Therefore, the public (chromium) builds need a different firmware without the processing lib. Rather than also duplicating the topology to also remove the processing lib from the topology, this would allow the same topology to be used but the codec_adapter would just default to a passthrough since the processing lib was not part of the firmware image.

In other words, users will be able to grab private topologies and reuse them with public/dev SOF builds.

dbaluta · 2021-02-10T07:22:18Z

@cujomalainey ideally we should separate somehow sof firmware binary from processing libraries binaries, in the same way Linux kernel separates kernel Image from modules. Or in the space way, user space binaries use shared libraries.

As it is now, sof-.ri needs all binaries stitched together, so we are stuck with different .ri binaries to solve the licensing restrictions. I think the proposed solution for pass through is a reasonable compromise.

cujomalainey · 2021-02-10T07:51:51Z

I agree, dynamic libs would be an excellent solution but it is not something we have unfortunately. It would also make my job easier

dbaluta · 2021-02-10T14:12:03Z

Will try to attack this dynamic libs approach with Google Summer of Code project :).

lgirdwood · 2021-02-10T14:45:22Z

Will try to attack this dynamic libs approach with Google Summer of Code project :).

Good to hear - The simplest method given the GSoC time constraints is to statically map a library into a known memory offset i.e.

We "reserve" region(s) of heap for dynamic text, data and bss
load library via IPC request - kernel passes in the library and copy to reserved region (this needs to be cache managed too).
Add IPC to unload library.
This should be the fastest and integrate with current, build and linker scripts.

The optimal solution is to do the runtime linking and adress remapping (like Linux does), but this will take longer.

cujomalainey · 2021-02-10T23:15:29Z

Random thought, could we possibly design it in a way to thin provision the code regions? I.e.

Build firmware that when completely loaded uses > system memory constraint but has the expectation that topology will only trigger certain modules to be loaded and the subset will be less than system memory.

I am thinking for the chrome case. Say platform X has device A with processing lib J, and Device B with processing lib K. A firmware with both lib J and K would exceed system memory limits. Since they all use the same system image, currently each firmware would have to be built independently and have unique paths. But with with this mechanism there would be one firmware image and the topology would trigger only loading the proper modules for the device.

Or does this simply fall under runtime linking?

dbaluta · 2021-02-11T07:12:59Z

@cujomalainey I would expect most of the decisions should be taken at runtime linking. I am yet to study this.
Although, I find that deciding, statically, which lib to use when creating the topology is not of our best ideas. In topology the user should only provide codec_adapter component type (e.g Cadence, Dummy, waves) and then the actual library which will be loaded should be decided at runtime.

How do I see things:

create topology with codec adapter component
Linux kernel boots, loads the pipeline but it doesn't need to load the library yet
later, when someone decides to use the compress card is the moment when the library is loaded.
- e.g cplay -c 1 -d 0 -I MP3 sample.mp3
- when cplay stops library is unloaded
later, we can use the compress card with other library
- e.g cplay -c 1 -d 0 -I AAC sample.aac

So, we should be able to run all sorts of algorithms from dynamically loaded libraries and the constraint here would be that the memory needed in order to run this to be at least equal with the size of largest library binary.

This will be a very interesting feature to have.

Also, right now all the modules + sof firmware are compiled under the same sof-.ri this is a very big limitation both because of licensing and of the size of sof-.ri.

@cujomalainey I understand your question. I am yet to process the suggestions from @lgirdwood

Later edit: I understand now @lgirdwood suggestion and it makes sense.

cujomalainey · 2021-02-11T19:33:35Z

Hmm, that makes for a different usage than for processing libs which will not need swapping out. This may need a meeting and design proposal.

If the passthrough flag is enabled, on process() the data in the input buffer should be directly copied to the output buffer. The passthrough flag is implemented to be set/get by topology enum control. Signed-off-by: Pin-chih Lin <johnylin@google.com>

johnylin76 · 2021-03-02T11:42:58Z

I have implemented set/get passthrough flag by enum control.
However I met an dmesg error on loading topology of SOF testing on Delbin

2021-03-02T09:55:40.185038Z DEBUG kernel: [    6.377578] sof-audio-pci 0000:00:1f.3: tplg: load control type 2 name : CA Setup Config5
2021-03-02T09:55:40.185038Z DEBUG kernel: [    6.377579] sof-audio-pci 0000:00:1f.3: tplg: load kcontrol index 23 chans 0
2021-03-02T09:55:40.185039Z DEBUG kernel: [    6.377580] sof-audio-pci 0000:00:1f.3: tplg: load control type 2 name : CA Runtime Params5
2021-03-02T09:55:40.185041Z DEBUG kernel: [    6.377582] sof-audio-pci 0000:00:1f.3: tplg: load kcontrol index 23 chans 0
2021-03-02T09:55:40.185041Z DEBUG kernel: [    6.377584] sof-audio-pci 0000:00:1f.3: tplg: load control type 3 name : CA PassThrough5
2021-03-02T09:55:40.185042Z DEBUG kernel: [    6.377586] sof-audio-pci 0000:00:1f.3: tplg: load kcontrol index 23 chans 1 comp_id 23
2021-03-02T09:55:40.185043Z DEBUG kernel: [    6.377587] sof-audio-pci 0000:00:1f.3: tplg: ready widget id 23 pipe 5 type 33 name : CODEC_ADAPTER5.0 stream none
2021-03-02T09:55:40.185044Z ERR kernel: [    6.377589] sof-audio-pci 0000:00:1f.3: error: no scontrol for widget CODEC_ADAPTER5.0
2021-03-02T09:55:40.185046Z ERR kernel: [    6.377592] sof-audio-pci 0000:00:1f.3: error: process loading failed
2021-03-02T09:55:40.185047Z ERR kernel: [    6.377595] sof-audio-pci 0000:00:1f.3: error: DSP failed to add widget id 0 type 33 name : CODEC_ADAPTER5.0 stream none reply 0
2021-03-02T09:55:40.185048Z ERR kernel: [    6.377597] sof-audio-pci 0000:00:1f.3: ASoC: failed to load widget CODEC_ADAPTER5.0
2021-03-02T09:55:40.185048Z ERR kernel: [    6.377599] sof-audio-pci 0000:00:1f.3: ASoC: topology: could not load header: -22

According to error message "no scontrol for widget" from the source code, it reports error because there is no private data for enum control?
https://lxr.missinglinkelectronics.com/linux/sound/soc/sof/topology.c#L1795

Does it mean we need the definition of private data for enum control just like byte control?

dbaluta · 2021-03-02T11:58:14Z

src/audio/codec_adapter/codec/generic.c

+		ret = process_passthrough(dev);
+	} else {
+		ret = codec->ops->process(dev);
+	}


no need for curly brackets here.

johnylin76 · 2021-03-03T07:24:11Z

Hi @singalsu and @juimonen ,
Do you have any debug direction to provide? Do we need private data for enum control, or is there wrong usage in my current implementation?
Thanks a lot.

I have implemented set/get passthrough flag by enum control.
However I met an dmesg error on loading topology of SOF testing on Delbin

2021-03-02T09:55:40.185038Z DEBUG kernel: [    6.377578] sof-audio-pci 0000:00:1f.3: tplg: load control type 2 name : CA Setup Config5
2021-03-02T09:55:40.185038Z DEBUG kernel: [    6.377579] sof-audio-pci 0000:00:1f.3: tplg: load kcontrol index 23 chans 0
2021-03-02T09:55:40.185039Z DEBUG kernel: [    6.377580] sof-audio-pci 0000:00:1f.3: tplg: load control type 2 name : CA Runtime Params5
2021-03-02T09:55:40.185041Z DEBUG kernel: [    6.377582] sof-audio-pci 0000:00:1f.3: tplg: load kcontrol index 23 chans 0
2021-03-02T09:55:40.185041Z DEBUG kernel: [    6.377584] sof-audio-pci 0000:00:1f.3: tplg: load control type 3 name : CA PassThrough5
2021-03-02T09:55:40.185042Z DEBUG kernel: [    6.377586] sof-audio-pci 0000:00:1f.3: tplg: load kcontrol index 23 chans 1 comp_id 23
2021-03-02T09:55:40.185043Z DEBUG kernel: [    6.377587] sof-audio-pci 0000:00:1f.3: tplg: ready widget id 23 pipe 5 type 33 name : CODEC_ADAPTER5.0 stream none
2021-03-02T09:55:40.185044Z ERR kernel: [    6.377589] sof-audio-pci 0000:00:1f.3: error: no scontrol for widget CODEC_ADAPTER5.0
2021-03-02T09:55:40.185046Z ERR kernel: [    6.377592] sof-audio-pci 0000:00:1f.3: error: process loading failed
2021-03-02T09:55:40.185047Z ERR kernel: [    6.377595] sof-audio-pci 0000:00:1f.3: error: DSP failed to add widget id 0 type 33 name : CODEC_ADAPTER5.0 stream none reply 0
2021-03-02T09:55:40.185048Z ERR kernel: [    6.377597] sof-audio-pci 0000:00:1f.3: ASoC: failed to load widget CODEC_ADAPTER5.0
2021-03-02T09:55:40.185048Z ERR kernel: [    6.377599] sof-audio-pci 0000:00:1f.3: ASoC: topology: could not load header: -22

According to error message "no scontrol for widget" from the source code, it reports error because there is no private data for enum control?
https://lxr.missinglinkelectronics.com/linux/sound/soc/sof/topology.c#L1795

Does it mean we need the definition of private data for enum control just like byte control?

dbaluta · 2021-03-03T08:09:58Z

@johnylin76 do you have a separate m4 topology for your usecase?

juimonen · 2021-03-03T10:01:48Z

@johnylin76 do you have this in your kernel? thesofproject/linux#2423, you need to have it for multiple different types of control in 1 widget...

johnylin76 · 2021-03-04T06:21:11Z

@johnylin76 do you have this in your kernel? thesofproject/linux#2423, you need to have it for multiple different types of control in 1 widget...

I didn't have that, and I think this is probably the root cause. Will give it a try.
Thanks a lot.

johnylin76 · 2021-03-04T06:29:31Z

@johnylin76 do you have a separate m4 topology for your usecase?

Do you mean the top-level topology like sof-tgl-*.m4? Yes I have but I didn't upload to this PR because it is just for testing purpose.

dbaluta · 2021-03-04T08:08:16Z

Do you mean the top-level topology like sof-tgl-*.m4? Yes I have but I didn't upload to this PR because it is just for testing purpose.
@johnylin76 Yes. I wanted to have a look at it. But I think the fix is as @juimonen said.

lgirdwood · 2021-03-05T12:14:33Z

@juimonen do we have an ETA for thesofproject/linux#2423 landing upstream ?

paulstelian97 · 2021-03-29T08:57:49Z

Please resubmit with "main" as PR base branch.

johnylin76 requested a review from cujomalainey January 20, 2021 16:03

dbaluta reviewed Jan 20, 2021

View reviewed changes

src/audio/codec_adapter/codec/generic.c Show resolved Hide resolved

dbaluta reviewed Jan 20, 2021

View reviewed changes

cujomalainey reviewed Jan 21, 2021

View reviewed changes

lgirdwood reviewed Jan 22, 2021

View reviewed changes

lgirdwood added the codec Issues related to a 3rd party codec HW or driver label Feb 24, 2021

lgirdwood added this to the v1.8 milestone Feb 24, 2021

johnylin76 force-pushed the passthrough branch from 1dd910f to 485c939 Compare March 2, 2021 11:29

dbaluta reviewed Mar 2, 2021

View reviewed changes

johnylin76 requested a review from juimonen March 3, 2021 07:24

johnylin76 requested a review from singalsu March 3, 2021 07:24

dbaluta mentioned this pull request Mar 26, 2021

RFC: Add final support for compress audio #3959

Closed

lgirdwood closed this Mar 28, 2021

lgirdwood deleted the branch master March 28, 2021 13:44

johnylin76 deleted the passthrough branch June 30, 2022 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: codec_adapter: Add process passthrough path #3766

src: codec_adapter: Add process passthrough path #3766

johnylin76 commented Jan 20, 2021

dbaluta Jan 20, 2021

cujomalainey Jan 20, 2021

lgirdwood Jan 22, 2021

cujomalainey Mar 2, 2021

cujomalainey left a comment •

edited

Loading

lgirdwood left a comment

lgirdwood Jan 22, 2021

dbaluta commented Jan 22, 2021

cujomalainey commented Jan 22, 2021

johnylin76 commented Jan 25, 2021

lgirdwood commented Feb 5, 2021

cujomalainey commented Feb 5, 2021

lgirdwood commented Feb 9, 2021

mrajwa commented Feb 9, 2021 •

edited

Loading

cujomalainey commented Feb 9, 2021

mrajwa commented Feb 9, 2021

cujomalainey commented Feb 9, 2021

dbaluta commented Feb 10, 2021 •

edited

Loading

cujomalainey commented Feb 10, 2021

dbaluta commented Feb 10, 2021

lgirdwood commented Feb 10, 2021

cujomalainey commented Feb 10, 2021 •

edited

Loading

dbaluta commented Feb 11, 2021 •

edited

Loading

cujomalainey commented Feb 11, 2021

johnylin76 commented Mar 2, 2021

dbaluta Mar 2, 2021

johnylin76 commented Mar 3, 2021

dbaluta commented Mar 3, 2021

juimonen commented Mar 3, 2021

johnylin76 commented Mar 4, 2021

johnylin76 commented Mar 4, 2021

dbaluta commented Mar 4, 2021

lgirdwood commented Mar 5, 2021

paulstelian97 commented Mar 29, 2021

src: codec_adapter: Add process passthrough path #3766

src: codec_adapter: Add process passthrough path #3766

Conversation

johnylin76 commented Jan 20, 2021

dbaluta Jan 20, 2021

Choose a reason for hiding this comment

cujomalainey Jan 20, 2021

Choose a reason for hiding this comment

lgirdwood Jan 22, 2021

Choose a reason for hiding this comment

cujomalainey Mar 2, 2021

Choose a reason for hiding this comment

cujomalainey left a comment • edited Loading

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

lgirdwood Jan 22, 2021

Choose a reason for hiding this comment

dbaluta commented Jan 22, 2021

cujomalainey commented Jan 22, 2021

johnylin76 commented Jan 25, 2021

lgirdwood commented Feb 5, 2021

cujomalainey commented Feb 5, 2021

lgirdwood commented Feb 9, 2021

mrajwa commented Feb 9, 2021 • edited Loading

cujomalainey commented Feb 9, 2021

mrajwa commented Feb 9, 2021

cujomalainey commented Feb 9, 2021

dbaluta commented Feb 10, 2021 • edited Loading

cujomalainey commented Feb 10, 2021

dbaluta commented Feb 10, 2021

lgirdwood commented Feb 10, 2021

cujomalainey commented Feb 10, 2021 • edited Loading

dbaluta commented Feb 11, 2021 • edited Loading

cujomalainey commented Feb 11, 2021

johnylin76 commented Mar 2, 2021

dbaluta Mar 2, 2021

Choose a reason for hiding this comment

johnylin76 commented Mar 3, 2021

dbaluta commented Mar 3, 2021

juimonen commented Mar 3, 2021

johnylin76 commented Mar 4, 2021

johnylin76 commented Mar 4, 2021

dbaluta commented Mar 4, 2021

lgirdwood commented Mar 5, 2021

paulstelian97 commented Mar 29, 2021

cujomalainey left a comment •

edited

Loading

mrajwa commented Feb 9, 2021 •

edited

Loading

dbaluta commented Feb 10, 2021 •

edited

Loading

cujomalainey commented Feb 10, 2021 •

edited

Loading

dbaluta commented Feb 11, 2021 •

edited

Loading