ASoC: SOF: topology: add support to smart amplifier by keyonjie · Pull Request #1971 · thesofproject/linux

keyonjie · 2020-04-01T07:36:08Z

Add smart amplifier component support, which is designed as another new
type of process component and used for Dynamic Speaker Management.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

keyonjie · 2020-04-01T07:36:48Z

The corresponding FW part PR is here:
thesofproject/sof#1896

plbossart · 2020-04-01T15:31:53Z

I think this is a dangerous path. the 'smart-amp' is by no means generic and the code is not available, it's 3rd-party IP and it should be classified as such. This is not a full-blown process maintained in the SOF tree. At the very least it should be an ID that belongs in the 3rd party range

@lgirdwood what's your take on this?

keyonjie · 2020-04-01T16:29:39Z

@plbossart Thanks for reviewing.

I am not sure if your "dangerous" means it could break the amplifier with bad algorithm, or adding more and more new component ID is not the correct path, but let me try to add some information about what we have been doing. Per my understanding, the FW team has been using the model for 3rd-party IP development like this:

create a dummy(or simple) algorithm plus the component shell, integrate them to the whole SOF stack.
provide standard APIs for integrating 3rd-party algorithms, e.g.
a. the detect_func() in detector component for KWD, where samples and size are provided and detected or not result is expected from the return
b. the smart_amp_process() in smart_amp component, where the playback input and the feedback input samples are provided, the output samples are expected to be written to the sink buffer.
Our release will focus on the "generic" framework for the feature only, we are implement these dummy sample algorithm, creating topology files, trying to validate and make sure the sequence run as in the realword scenarios.

@bkokoszx @mrajwa please correct me if I have written something wrong.

ranj063 · 2020-04-01T16:38:05Z

I think this is a dangerous path. the 'smart-amp' is by no means generic and the code is not available, it's 3rd-party IP and it should be classified as such. This is not a full-blown process maintained in the SOF tree. At the very least it should be an ID that belongs in the 3rd party range
@lgirdwood what's your take on this?

@plbossart I agree that this isnt a scalable solution for supporting 3rd party modules. Do we have a process for adding new modules?

plbossart · 2020-04-01T18:43:33Z

create a dummy(or simple) algorithm plus the component shell, integrate them to the whole SOF stack.

That makes it impossible to know if the component is implemented with the dummy test version or the real one... Something is missing here really.

keyonjie · 2020-04-02T01:43:18Z

create a dummy(or simple) algorithm plus the component shell, integrate them to the whole SOF stack.
That makes it impossible to know if the component is implemented with the dummy test version or the real one... Something is missing here really.

By default we are using dummy test version in upstream. We plan to add config item for 3rd-party version selection.

kv2019i · 2020-04-02T09:03:09Z

@keyonjie How does this relate to:
https://github.com/orgs/thesofproject/teams/sof-developers/discussions/32
... you started this already three weeks back. But here in #1971 we are still adding a component type in the old way. Is this an intermediate thing, or is the proposal to add smart amp as a "well known" processing type?

keyonjie · 2020-04-02T09:56:24Z

@keyonjie How does this relate to:
https://github.com/orgs/thesofproject/teams/sof-developers/discussions/32
... you started this already three weeks back. But here in #1971 we are still adding a component type in the old way. Is this an intermediate thing, or is the proposal to add smart amp as a "well known" processing type?

Yes they have relationship, the discussion/32 is something like framework refinement, but I had no time to do that yet, it need effort on both FW and driver side together.

kv2019i

Smart amp seems to be generic enough. The is remaining work to identify the implementations with UUID, but that applies to all generic processing components, not just this one.

plbossart · 2020-04-03T16:38:32Z

I am personally not comfortable with merging this
a) internal releases already took it without reviews, so there's no urgency in topic/sof-dev
b) we don't know what the next steps are
c) it's again a piecemeal approach with incremental patches and no design document.

Call me a grumpy old person if you want, but it's a recurring pattern really. There was plenty of time to do the right thing and we didn't.

keyonjie · 2020-04-07T02:34:25Z

I am personally not comfortable with merging this
a) internal releases already took it without reviews, so there's no urgency in topic/sof-dev
b) we don't know what the next steps are
c) it's again a piecemeal approach with incremental patches and no design document.
Call me a grumpy old person if you want, but it's a recurring pattern really. There was plenty of time to do the right thing and we didn't.

Hi @plbossart There is discussion and extra work to implement the UUID in the driver side, we need to align on that before we can refine the generic processing components.

lgirdwood · 2020-04-16T11:24:02Z

@plbossart @keyonjie @kv2019i set to MINOR 15 like FW

lgirdwood · 2020-04-16T14:49:22Z

@plbossart @keyonjie @kv2019i set to MINOR 15 like FW

NAK 15, now at 16. @keyonjie please use MINOR 16.

keyonjie · 2020-04-17T02:22:20Z

@plbossart @keyonjie @kv2019i set to MINOR 15 like FW
NAK 15, now at 16. @keyonjie please use MINOR 16.

Thanks for the information @lgirdwood .

So should we add explicit ABI bump for this change or the MINOR 16 is for update with several other changes? I am not seeing the 16 bumping in the FW corresponding PR thesofproject/sof#1896.

keyonjie · 2020-04-17T02:48:17Z

@lgirdwood @plbossart @kv2019i just checked the ABI MINOR in dirver side is already 16.

kv2019i · 2020-04-20T10:45:08Z

Update on this PR: the proposal is to use same approach as with EQ, KPB and others for Smart Amp.
The more generic solution to describe and control "effect" type components, is not ready yet (work on this continues in thesofproject/sof#769 and https://github.com/orgs/thesofproject/teams/sof-developers/discussions/32).

plbossart

let's hope this is the last one we add this way - but I wouldn't hold my breath.

Add smart amplifier component support, which is designed as another new type of process component and used for speaker protection algorithm integration. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

RanderWang · 2020-04-22T08:21:52Z

 if (!strcmp(codec_dai->component->name, MAX_98373_DEV0_NAME)) {
 /* DEV0 tdm slot configuration */
- snd_soc_dai_set_tdm_slot(codec_dai, 0x30, 3, 8, 16);
+ snd_soc_dai_set_tdm_slot(codec_dai, 0x3, 3, 8, 32);


unify code style to 0x3, 0x3, 8, 32 ? tx_mask and rx_mask should be in the same style.

sorry, I just pushed wrong local change recently, this is for debug only so should not go to here :)

removed. @RanderWang

kv2019i · 2020-04-22T10:24:25Z

@plbossart wrote:

let's hope this is the last one we add this way - but I wouldn't hold my breath.

And you were right of course. Although this was so quick that you could have even held your breath for real while waiting for this one ;D
#2030

More seriously, I think it is now pretty evident that we need to invest in the infra in this area. I.e. time to proceed with https://github.com/orgs/thesofproject/teams/sof-developers/discussions/32

kv2019i

Looks good.

keyonjie · 2020-04-22T13:00:51Z

Thanks @plbossart @kv2019i , yes, that's true, I will put effort to the UUID implementation on driver side after freed from the release.

plbossart · 2020-04-24T22:13:09Z

@keyonjie and @sebcarlucci can you please chat and figure out which one of these two components goes in first and what ABI level this is.

@lgirdwood FYI this is a mess

keyonjie · 2020-04-26T01:25:01Z

@keyonjie and @sebcarlucci can you please chat and figure out which one of these two components goes in first and what ABI level this is.

@sebcarlucci is it OK for you to let this smart amplifier one go first, it has been hold for quite long time and it is actually waited from the product.

For the ABI version, I think @lgirdwood and @kv2019i has aligned to use Minor 16 for it? @kv2019i has initiated an discussion about ABI refinement in https://github.com/orgs/thesofproject/teams/sof-developers/discussions/35, but I am not sure will that impact this PR actually.

@lgirdwood FYI this is a mess

sebcarlucci · 2020-04-26T02:02:33Z

@sebcarlucci is it OK for you to let this smart amplifier one go first, it has been hold for quite long time and it is actually waited from the product.

@keyonjie Sounds good to me

keyonjie · 2020-04-26T02:59:03Z

@sebcarlucci is it OK for you to let this smart amplifier one go first, it has been hold for quite long time and it is actually waited from the product.
@keyonjie Sounds good to me

Thanks.

kv2019i · 2020-04-27T10:14:14Z

There were errors on one machine in CI: https://sof-ci.01.org/linuxpr/PR1971/build3638/devicetest/
... but given the commit contents, I can't see how it could be related.

So basicly, I could merge this, but I'm a bit puzzled I see no approvals for the FW change thesofproject/sof#1896 .. what's up with that?

kv2019i · 2020-05-07T12:26:45Z

The FW change is still missing reviews, not merging this before it's at least approved.

keyonjie · 2020-05-09T02:23:13Z

@kv2019i the FW one is approved now: thesofproject/sof#1896

Let's merge it?

kv2019i · 2020-05-12T10:45:45Z

FW patch approved, merging this as well.

keyonjie requested review from bardliao, juimonen, ranj063 and singalsu as code owners April 1, 2020 07:36

mengdonglin added the Smart Amp Smart Amp or DSM (Dynamic Speaker Management) label Apr 1, 2020

This was referenced Apr 2, 2020
[BUG][TGL]Two channels both output sound at the same time during speaker test on aplay0,0(speaker). #1977
Closed
[BUG][TGL]Warning message "snd_hda_codec_hdmi ehdaudio0D2: ASoC: can't open DAI intel-hdmi-hifi2: -16" occurred during multipipline test. #1978
Closed

keyonjie force-pushed the topic/smart-amp-process branch from b5067bf to 0f1ca2aCompare April 3, 2020 08:50

kv2019i previously approved these changes Apr 3, 2020
View reviewed changes

plbossart added the ABI involves ABI changes, need extra attention label Apr 13, 2020

keyonjie mentioned this pull request Apr 16, 2020
topology: add ipc for smart amplifier component thesofproject/sof#1896
Merged

lyakh previously approved these changes Apr 17, 2020
View reviewed changes

plbossart previously approved these changes Apr 20, 2020
View reviewed changes

plbossart mentioned this pull request Apr 21, 2020
ASoC: SOF: topology: Add support for Crossover #2030
Closed

keyonjie dismissed plbossart’s stale review via 0bd4c1cApril 22, 2020 02:02

keyonjie dismissed stale reviews from lyakh and kv2019i via 0bd4c1cApril 22, 2020 02:02

keyonjie force-pushed the topic/smart-amp-process branch from 0f1ca2a to 0bd4c1cCompare April 22, 2020 02:02

keyonjie requested review from dbaluta and lgirdwood as code owners April 22, 2020 02:02

ASoC: SOF: topology: add support to smart amplifier
7357efe
Add smart amplifier component support, which is designed as another new type of process component and used for speaker protection algorithm integration. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

RanderWang reviewed Apr 22, 2020
View reviewed changes

keyonjie force-pushed the topic/smart-amp-process branch from 0bd4c1c to 7357efeCompare April 22, 2020 08:40

plbossart approved these changes Apr 22, 2020
View reviewed changes

kv2019i approved these changes Apr 22, 2020
View reviewed changes

bkokoszx mentioned this pull request Apr 27, 2020
[BUG][TGL][DSM] old/wrong data at the beginning of released stream thesofproject/sof#2883
Closed

kv2019i merged commit a2c2695 into thesofproject:topic/sof-devMay 12, 2020

keyonjie mentioned this pull request May 18, 2020
smart_amp: add smart amplifier test component thesofproject/sof#2897
Merged

Uh oh!

Conversation

keyonjie commented Apr 1, 2020

Uh oh!

keyonjie commented Apr 1, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Apr 1, 2020

Uh oh!

keyonjie commented Apr 1, 2020

Uh oh!

ranj063 commented Apr 1, 2020

Uh oh!

plbossart commented Apr 1, 2020

Uh oh!

keyonjie commented Apr 2, 2020

Uh oh!

kv2019i commented Apr 2, 2020

Uh oh!

keyonjie commented Apr 2, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart commented Apr 3, 2020

Uh oh!

keyonjie commented Apr 7, 2020

Uh oh!

lgirdwood commented Apr 16, 2020

Uh oh!

lgirdwood commented Apr 16, 2020

Uh oh!

keyonjie commented Apr 17, 2020

Uh oh!

keyonjie commented Apr 17, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kv2019i commented Apr 20, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

RanderWangApr 22, 2020

Choose a reason for hiding this comment

Uh oh!

keyonjieApr 22, 2020

Choose a reason for hiding this comment

Uh oh!

keyonjieApr 22, 2020

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Apr 22, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

keyonjie commented Apr 22, 2020

Uh oh!

plbossart commented Apr 24, 2020

Uh oh!

keyonjie commented Apr 26, 2020

Uh oh!

sebcarlucci commented Apr 26, 2020

Uh oh!

keyonjie commented Apr 26, 2020

Uh oh!

kv2019i commented Apr 27, 2020

Uh oh!

kv2019i commented May 7, 2020

Uh oh!

keyonjie commented May 9, 2020

Uh oh!

kv2019i commented May 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

keyonjie commented Apr 1, 2020•
edited
Loading

keyonjie commented Apr 17, 2020•
edited
Loading