topology: add ipc for smart amplifier component #1896

bkokoszx · 2019-10-03T09:04:03Z

Add SOF_COMP_SMART_AMP type for smart amp component.
Basic topology using smart amplifier component
could be as follows:

            +--------------------------------------------+
+------+    | +---+  +---------------+  +---+  +-------+ |
| Host |----->|Buf|->|   Smart Amp   |->|Buf|->|SSP Dai|----------+
+------+    | +---+  +---------------+  +---+  +-------+ |        |
            +----------------^---------------------------+        |
                             |                                    |
                           +---+                                  |
                           |Buf|                                  | 
                           +---+                                  |
                             ^                                    |
            +----------------|-----------------------------+      |
+------+    | +---+  +-------|---------+  +---+  +-------+ |      |
| Host |<-----|Buf|<-|      Demux      |<-|Buf|<-|SSP Dai|<-------+
+------+    | +---+  +-----------------+  +---+  +-------+ |
            +----------------------------------------------+

Basic topology contains Smart Amp component in playback pipeline and Demux
component in capture pipeline. There is a additional buffer (feedback)
between them in order to process "feedback".

Signed-off-by: Bartosz Kokoszko bartoszx.kokoszko@linux.intel.com

dbaluta · 2019-10-03T10:34:53Z

@bkokoszx drawing is very nice. Please include the PR description inside commit message. Otherwise, it will be lost into git PR history.

No one looks into closed PRs, every useful information should be placed inside git commit message.

juimonen · 2019-10-03T10:45:26Z

@bkokoszx so is the smart_amp and smart_amp_demux different components or same component with different parameters? Also do you have to possible topology (m4->alsa.conf) changes soewhere?

bkokoszx · 2019-10-03T11:44:30Z

@dbaluta
I've updated commit message.

@juimonen
Smart Amp and Smart Amp Demux are different components created by the same ipc struct (the same as we have mux and demux). I still have a doubts do we really need separate struct sof_ipc_comp_smart_amp for those components, we can
also handle them by e.g. struct sof_ipc_comp_process easily (@dbaluta @juimonen @lgirdwood what's your opinion?).
I do not have tplg file yet and to be honest I'm not an specialist at this - I will probably need some help :)

bkokoszx · 2019-10-14T09:54:58Z

@lgirdwood
Could you please refer to my last comment?

xiulipan · 2019-10-16T08:44:41Z

@bkokoszx @lgirdwood @plbossart Do we got a ABI version for this? As I will work on the kernel part now.

PS: what dapm comp would this smart amp mapped to? Are they sometype of mux and demux with some special config?

bkokoszx · 2019-10-16T14:56:18Z

@xiulipan
IMHO It could be separate component type, but I may be wrong (@lgirdwood @plbossart what do you think?).

plbossart · 2019-10-16T17:04:16Z

yes there should be something indicating those new devices are present, but more importantly the topology ABI needs to be updated so that we can check consistency between topology and firmware.

xiulipan · 2019-10-18T08:07:09Z

@bkokoszx @plbossart Do we also need to create some new dapm structure for this? I think we do not have a suitable kind of widget in topology for this new feature.

plbossart · 2019-10-18T11:56:16Z

@juimonen @ranj063 can you comment on the DAPM/topology question?
I would think there's no need for a new widget, the following definition should work

#define SND_SOC_TPLG_DAPM_EFFECT	18

xiulipan · 2019-10-21T08:57:44Z

@plbossart @juimonen @ranj063 I will start to work with the SND_SOC_TPLG_DAPM_EFFECT
@bkokoszx If we would like to use the SND_SOC_TPLG_DAPM_EFFECT, we will not use a new comp struct. Please check the workflow of the KPB, MUX and DEMUX.

bkokoszx · 2019-10-21T09:10:08Z

@xiulipan
Ok, so will you use the following ipc structure?

struct sof_ipc_comp_process {
	struct sof_ipc_comp comp;
	struct sof_ipc_comp_config config;
	uint32_t size;	/**< size of bespoke data section in bytes */
	uint32_t type;	/**< sof_ipc_effect_type */

	/* reserved for future use */
	uint32_t reserved[7];

	unsigned char data[0];
} __attribute__((packed));

xiulipan · 2019-10-21T09:13:21Z

@bkokoszx You can take the reference of kpb enable patch. https://github.com/thesofproject/sof/blob/master/src/audio/kpb.c

sof/src/audio/kpb.c

Lines 1356 to 1369 in 3d5d314

    
           struct comp_driver comp_kpb = { 
        
           	.type = SOF_COMP_KPB, 
        
           	.ops = { 
        
           		.new = kpb_new, 
        
           		.free = kpb_free, 
        
           		.cmd = kpb_cmd, 
        
           		.trigger = kpb_trigger, 
        
           		.copy = kpb_copy, 
        
           		.prepare = kpb_prepare, 
        
           		.reset = kpb_reset, 
        
           		.cache = kpb_cache, 
        
           		.params = NULL, 
        
           	}, 
        
           };

So the only specific variable for the smart_amp is uint32_t feedback_buf_id; ?

bkokoszx · 2019-10-21T09:16:42Z

@xiulipan
Thanks.

I think we can even ignore uint32_t feedback_buf_id. I could "find" that buffer without feedback_buf_id in e.g.smart_amp_prepare() by connected components types.

Today I will try to update smart amp template.

bkokoszx · 2019-10-21T10:20:57Z

@xiulipan
Smart Amp template using struct sof_ipc_comp_process (without uint32_t feedback_buf_id) is available here:
https://github.com/thesofproject/sof/tree/topic/smart-amp-comp-process

plbossart · 2019-10-21T11:38:59Z

src/include/ipc/topology.h

what does this index represent? The component ID of the buffer? the topology ID?
There are so many indices throughout topology, kernel and firmware, you need to be more explicit on what this is tied to.

Sorry @plbossart , I've just updated PR and commit messages.

I've talked with @xiulipan and we decided to use sof_ipc_comp_process struct
(like e.g. in KPB; it does not have feedback_buf_id variable) instead of sof_ipc_comp_smart_amp.

xiulipan · 2019-10-22T10:59:26Z

tplg PR: #1983
linux PR: thesofproject/linux#1343

Test with https://github.com/thesofproject/sof/commits/topic/smart-amp-comp-process
tplg can load, but could only play noise and record is not working.
not sure if I used wrong buffer size for the pipeline.

    0      2         PIPE 2.16    451028710.572917         9.062500 src/audio/pipeline.c:463    pipeline_prepare()
    0      2         HOST 2.8     451028721.406250        10.833333     src/audio/host.c:695    host_prepare()
    0      2       VOLUME         451028743.802083        22.395834 udio/volume/volume.c:589    volume_prepare()
    0      1         COMP         451028748.281250         4.479167 rc/audio/component.c:120    comp_set_sink_buffer() error: sink buffer size is not sufficient
    0      1       VOLUME         451028753.593750         5.312500 udio/volume/volume.c:624    volume_prepare() error: comp_set_sink_buffer() failed
    0      1         PIPE         451028759.270833         5.677083 src/audio/pipeline.c:470    pipeline_prepare() error: ret = -22,dev->comp.id = 8
    0      1          IPC         451028765.572917         6.302083    src/ipc/handler.c:305    ipc: pipe 2 comp 8 prepare failed -22

bkokoszx · 2019-10-22T13:01:45Z

@xiulipan
We've just checked similar tplg (with volume in playback and capture pipeline - as you illustrated here: #1983 (comment)) with slim driver and it works. There is no special requirement for buffer sizes (size is directly related to the pcm format and number of periods). All buffers you have created have the same size? Could you send full log?

xiulipan · 2019-10-23T03:45:28Z

@bkokoszx Please check the logs. I tried
arecord first and aplay later.
log.txt

zrombel · 2019-11-27T11:24:38Z

Due to merg of PR #2029 and CI bit depth conversion tests changes - rebase is needed.

lgirdwood · 2020-01-06T16:26:42Z

@bkokoszx any update ?

lbetlej · 2020-02-04T13:47:14Z

SOFCI TEST

bkokoszx · 2020-03-17T08:52:28Z

SOFCI TEST

lgirdwood

This could be the last new one, @plbossart @kv2019i do you have a cut off date for this due to kernel impact.

lgirdwood · 2020-04-01T15:21:01Z

src/include/ipc/topology.h

Component types are going away. Lets not add any new ones.

I just had the same remark on the kernel side, even if we kept these comp_type it should be identified as 3rd-party stuff

@plbossart as replied on the kernel side, we plan to add config item for the 3rd-party algorithm specific selection, to the component itself, we want to make it a generic one with dummy(simple enough) algorithm, so if someone contribute an better open source algorithm, we can create an item for it also (something like a N->1 mux selection in the config menu).

@plbossart what's your suggestion about that?

plbossart · 2020-04-13T11:38:42Z

Assuming we use this, what is the ABI level? Still ABI 15?

bkokoszx · 2020-04-14T15:13:42Z

@plbossart
I think so. I've rebased this PR.
@lgirdwood
What do you think?

lgirdwood · 2020-04-16T10:27:59Z

@plbossart
I think so. I've rebased this PR.
@lgirdwood
What do you think?

Yep, ABI is set at MINOR 15 now for FW but where ie the kernel PR ???.

keyonjie · 2020-04-16T10:38:12Z

@plbossart
I think so. I've rebased this PR.
@lgirdwood
What do you think?

Yep, ABI is set at MINOR 15 now for FW but where ie the kernel PR ???.

thesofproject/linux#1971

lgirdwood · 2020-04-16T14:50:56Z

@bkokoszx sorry, needs to be MINOR 16 now - please check if any changes needed.

Add SOF_COMP_SMART_AMP type for smart amp component. Basic topology using smart amplifier component could be as follows: +------------------------------------------+ +------+ | +---+ +-------------+ +---+ +-------+ | | Host |----->|Buf|->| Smart Amp |->|Buf|->|SSP Dai|-----------+ +------+ | +---+ +-------------+ +---+ +-------+ | | +-----------------^------------------------+ | | | +---+ | |Buf| | +---+ | ^ | +-----------------|--------------------------+ | +------+ | +---+ +--------|------+ +---+ +-------+ | | | Host |<-----|Buf|<-| Demux |<-|Buf|<-|SSP Dai|<--------+ +------+ | +---+ +---------------+ +---+ +-------+ | +--------------------------------------------+ Basic topology contains Smart Amp component in playback pipeline and Demux component in capture pipeline. There is a additional buffer (feedback) between them in order to process "feedback". Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>

bkokoszx · 2020-04-17T08:05:54Z

@lgirdwood
Changed to MINOR 16.

lgirdwood · 2020-04-19T19:20:02Z

@kv2019i kernel part ok ? if good we can merge on Monday ?

kv2019i · 2020-04-20T10:39:51Z

@lgirdwood wrote:

@kv2019i kernel part ok ? if good we can merge on Monday ?

It seems ok, we have @plbossart request-changes still on this PR, would be good to get a formal ack on this.

lgirdwood · 2020-05-07T12:45:25Z

@bkokoszx @keyonjie ping, any update ?

bkokoszx · 2020-05-07T13:02:57Z

@lgirdwood
If the ABI is correct, then I think we are up to date.

keyonjie

@lgirdwood @kv2019i looks good to me.

lgirdwood

@kv2019i we are good from this end now.

kv2019i · 2020-05-12T10:46:19Z

@kv2019i we are good from this end now.

Ack @lgirdwood . Kernel PR is now merged.

lgirdwood · 2020-05-13T13:16:52Z

SOFCI TEST

lgirdwood · 2020-05-13T13:17:24Z

Looks like CI needs rerun sine it was down when last update was pushed.

lgirdwood · 2020-05-18T10:52:07Z

@dbaluta @paulstelian97 @dianacretu what's the ETA for imx qemu fix, I'm still seeing the qemu boot failure for one of the imx platforms with this PR. This is unrelated since this wont impact boot.

paulstelian97 · 2020-05-18T10:59:37Z

@dbaluta @paulstelian97 @dianacretu what's the ETA for imx qemu fix, I'm still seeing the qemu boot failure for one of the imx platforms with this PR. This is unrelated since this wont impact boot.

No ETA for the moment but we'll think about it. I think the issue is that our platform only reports boot complete via the IPC and the qemu check doesn't look at the IPC. So there are two ways to go here. Either let Diana add a generic IPC implementation (which will be useful for the other platforms as well) or I implement trace points (allocate a shared memory space for trace point and panic information) and have her only rely on the trace point itself. I assume the second one is probably simpler but I want an internal discussion on this as well.

lgirdwood · 2020-05-18T14:17:53Z

@paulstelian97 thanks for the update, I can apply this now and Diana can resolve the qemu CI issues when she is ready.

bkokoszx requested a review from a team as a code owner October 3, 2019 09:04

bkokoszx force-pushed the topology-smart-amp-comp branch from aab6199 to b7c7292 Compare October 3, 2019 11:31

bkokoszx added the ABI ABI change involved label Oct 17, 2019

plbossart requested changes Oct 21, 2019

View reviewed changes

bkokoszx force-pushed the topology-smart-amp-comp branch from b7c7292 to 44cc3bb Compare October 21, 2019 11:51

bkokoszx mentioned this pull request Oct 21, 2019

[RFC]ASoC: SOF: topology: add support for smart amp component thesofproject/linux#1343

Closed

bkokoszx mentioned this pull request Oct 22, 2019

[RFC]topology: add smart amp for apl pcm512x #1983

Closed

bkokoszx force-pushed the topology-smart-amp-comp branch from 44cc3bb to 5e284c4 Compare February 25, 2020 14:01

bkokoszx mentioned this pull request Mar 4, 2020

[FEATURE] Allow components/buffers to XRUN #2478

Closed

lgirdwood reviewed Apr 1, 2020

View reviewed changes

bkokoszx force-pushed the topology-smart-amp-comp branch from b0aa325 to 1200bf4 Compare April 14, 2020 08:33

monocodus bot approved these changes Apr 14, 2020

View reviewed changes

bkokoszx force-pushed the topology-smart-amp-comp branch from 1200bf4 to 49c8b59 Compare April 17, 2020 08:03

kv2019i requested review from keyonjie, lgirdwood and plbossart April 20, 2020 17:00

bkokoszx changed the title ~~[RFC] topology: add ipc for smart amplifier component~~ topology: add ipc for smart amplifier component Apr 29, 2020

bkokoszx mentioned this pull request Apr 29, 2020

smart_amp: add smart amplifier test component #2897

Merged

keyonjie approved these changes May 7, 2020

View reviewed changes

lgirdwood approved these changes May 8, 2020

View reviewed changes

lgirdwood merged commit 8a4ec56 into thesofproject:master May 18, 2020

bkokoszx mentioned this pull request May 27, 2020

Commit: 6069afa8: topology: add smart amp component type #2720

Closed

topology: add ipc for smart amplifier component #1896

topology: add ipc for smart amplifier component #1896

Uh oh!

Conversation

bkokoszx commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbaluta commented Oct 3, 2019

Uh oh!

juimonen commented Oct 3, 2019

Uh oh!

bkokoszx commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkokoszx commented Oct 14, 2019

Uh oh!

xiulipan commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkokoszx commented Oct 16, 2019

Uh oh!

plbossart commented Oct 16, 2019

Uh oh!

xiulipan commented Oct 18, 2019

Uh oh!

plbossart commented Oct 18, 2019

Uh oh!

xiulipan commented Oct 21, 2019

Uh oh!

bkokoszx commented Oct 21, 2019

Uh oh!

xiulipan commented Oct 21, 2019

Uh oh!

bkokoszx commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkokoszx commented Oct 21, 2019

Uh oh!

plbossart Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

bkokoszx Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

xiulipan commented Oct 22, 2019

Uh oh!

bkokoszx commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiulipan commented Oct 23, 2019

Uh oh!

zrombel commented Nov 27, 2019 • edited by lgirdwood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Jan 6, 2020

Uh oh!

lbetlej commented Feb 4, 2020

Uh oh!

bkokoszx commented Mar 17, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

plbossart Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

keyonjie Apr 2, 2020 • 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!

plbossart commented Apr 13, 2020

Uh oh!

bkokoszx commented Apr 14, 2020

Uh oh!

lgirdwood commented Apr 16, 2020

Uh oh!

keyonjie commented Apr 16, 2020

bkokoszx commented Oct 3, 2019 •

edited

Loading

bkokoszx commented Oct 3, 2019 •

edited

Loading

xiulipan commented Oct 16, 2019 •

edited

Loading

bkokoszx commented Oct 21, 2019 •

edited

Loading

bkokoszx commented Oct 22, 2019 •

edited

Loading

zrombel commented Nov 27, 2019 •

edited by lgirdwood

Loading

keyonjie Apr 2, 2020 •

edited

Loading