smart amp: added Maxim DSM audio component #3248

ryans-lee · 2020-07-28T21:25:10Z

This commit is about adding "Maxim DSM(Dynamic Speaker Management) component".
This component is written based on "Smart amplifier test component".
SOF audio component get variable length frame per approximately 1ms.
Because DSM requires fixed frame size(16 bit, 240 samples per channel), this component do internal buffering
to get desired input format.
This component originally works with DSM function library, but I removed it from this commit because it is not open source based and it has potential license issue. So it is currently working as bypass component.
This component tested on Volteer board with MAX98373 amp over I2S.

Please help to review.
Thank you in advance.

plbossart

License question below:

src/audio/dsm/sof_dsm.c

cujomalainey · 2020-07-28T22:35:41Z

I think you need to rebase your commit, you have conflicts and appear to be merging more than you intend

ryans-lee · 2020-07-29T22:24:22Z

I think you need to rebase your commit, you have conflicts and appear to be merging more than you intend

Thanks for the comment. I rebased my commit.

cujomalainey

general review comments as I am by no means an expert in DSM algorithms :)

src/audio/dsm/sof_dsm_inf.c

src/audio/dsm/sof_dsm.c

lgirdwood

Mostly minor stuff. @mmaka1 are we good to use the new trace UUID API here too ?

src/audio/dsm/sof_dsm.c

src/audio/dsm/sof_dsm_inf.c

src/audio/smart_amp_maxim.c

src/include/sof/audio/smart_amp_maxim.h

src/include/sof/audio/sof_dsm.h

ryans-lee · 2020-08-08T16:00:57Z

@plbossart @cujomalainey @lgirdwood I appreciate for your review.
I have updated changes to follow up your requests. Please help to review.
Thank you for your time.

src/audio/smart_amp/smart_amp.c

src/audio/smart_amp/smart_amp_generic.c

src/audio/smart_amp/smart_amp_maxim_dsm.c

ryans-lee · 2020-08-14T21:20:33Z

Added one more commit about 'memmove' implementation in lib/lib.c

lgirdwood

@ryans-lee can you make the memmove into it's own PR and add the comment about POSIX
@singalsu any comments ? This looks good to me.
@RDharageswari are there any Smart AMP dependencies that ned to be merged prior to this ?

lgirdwood · 2020-08-17T10:18:12Z

src/lib/lib.c

I'm assuming this allings with the POSIX version of memove ? If so, we should say so in the commit message.

@lgirdwood Let me create separate PR to add memmove function. Actually all memmove functions in this PR can be replaced by memcpy because it is copying higher address to lower address. memcpy cannot be used for the opposite case.
Let me switch memmove function to memcpy in this PR.

ryans-lee · 2020-08-18T23:57:44Z

I removed 'memmove' from this PR and updated the code again. Please help to review and approve.

lgirdwood · 2020-08-20T10:30:32Z

@cujomalainey any comments ?

cujomalainey · 2020-08-20T18:06:21Z

src/audio/smart_amp/smart_amp.c

no handling of the data here?

Parameter read/write function have not been implemented yet.
'SOF_CTRL_CMD_BINARY' covers general parameter update to the component and I'm not sure configuration by 'SOF_CTRL_CMD_ENUM' at this moment. Will add implementation if needed when I submit separate PR for the parameter read/write function. Thank you.

Ok, would you mind removing the ENUM since you have no usage? it looks like a bug there as it stands.

OK. Shall fix it.

cujomalainey · 2020-08-20T18:14:06Z

src/audio/smart_amp/smart_amp.c

should we return error on streams that are not S32LE so we don't have silent memory failures?

I took this code from the smart amp reference code smart_amp_test.c and I don't quite well understand about the reason why it is s32_le hardcoded. I confirmed feedback data is coming in s32_le but frame_fmt was still s16_le.
I suppose this issue is still there because smart_amp_test.c is still same.
Let me remove the hardcoded value later once frmae_fmt variable get the correct value.
@bkokoszx Could you please add comment here? Thanks for your help.

There were two reasons why S32_LE format is hardcoded here:

At the beginning there was requirement only for that format on feedback stream.

Our pipeline params propagation mechanism does not handle case with DSM topology (format parameter is not properly propagated to feedback buffer, so it was hardcoded at that moment (assuming that we only need s32_le)).

If we want to delete this hardcode, we should reopen @keyonjie PR with params negotiation #2788.

If the current platforms only support S32LE then i am fine with the hardcode but I recommend we open a tracking bug in case other formats such float wish to use this then we have a bug we can refer to

@bkokoszx Could you please help on this?

@ryans-lee can you put a check in here that returns error with some trace error output so this does not silenttly fail. You should keep the TODO info so that this can be addressed one the negotion is completed by @keyonjie .

@lgirdwood I've added a check and put 'com_err' message to notify the issue. I considered to return error in smart_amp_process, but it causes several errors and blocked testing for capture stream.

ryans-lee · 2020-08-24T21:00:58Z

Removed 'SOF_CTRL_CMD_ENUM' case.
@cujomalainey Please help to review. Thank you.

One additional modification - Removed topology change from this PR. Created separate PR3342 for the same thing.

RDharageswari · 2020-08-24T21:05:29Z

@lgirdwood : There are no dependencies for this patch

lgirdwood

I think we just need the one chnage and we are good ?

lgirdwood · 2020-08-27T11:10:06Z

src/audio/smart_amp/smart_amp.c

@ryans-lee can you put a check in here that returns error with some trace error output so this does not silenttly fail. You should keep the TODO info so that this can be addressed one the negotion is completed by @keyonjie .

Maxim DSM(Dynamic Speaker Management) audio component is added. This component takes stereo input + 4 channel IV feedback. Signed-off-by: Ryan Lee <ryans.lee@maximintegrated.com>

lgirdwood · 2020-08-28T12:37:06Z

Jenkins CI is kernel ROM status errors for ICL and CML.
@cujomalainey good for you too ? we can make any incremetal fixes as feature continues integration.

ryans-lee · 2020-08-31T15:19:16Z

@cujomalainey Would you please provide your comments?

cujomalainey · 2020-08-31T21:05:34Z

src/audio/smart_amp/smart_amp.c

+	 */
+	if (sad->feedback_buf->stream.frame_fmt != SOF_IPC_FRAME_S32_LE) {
+		sad->feedback_buf->stream.frame_fmt = SOF_IPC_FRAME_S32_LE;
+		comp_err(dev, "smart_amp_prepare(): S32_LE format is hardcoded as workaround");


no return error code?

@cujomalainey I considered to return error in smart_amp_process, but it causes several errors and blocks testing for capture stream. Hence I removed error return and added error message to move forward.

Got it, as long as userspace handles the change properly then Im ok with this.

lgirdwood · 2020-09-01T09:19:50Z

Jenkins CI showing ROM status errors on CML and ICL around PM flows that are unrelated.

ryans-lee · 2020-09-01T17:52:09Z

I appreciate for all your time and effort for the review.

cujomalainey · 2020-09-01T18:01:33Z

Thanks for the patience in the review

ryans-lee requested review from a team, bardliao, dbaluta, jajanusz, lbetlej, lgirdwood, libinyang, mmaka1, mrajwa, plbossart, ranj063, tlauda and xiulipan as code owners July 28, 2020 21:25

ryans-lee assigned RDharageswari Jul 28, 2020

ryans-lee changed the title ~~sof dsm audio component~~ smart Jul 28, 2020

ryans-lee changed the title ~~smart~~ smart amp: added Maxim DSM audio component Jul 28, 2020

plbossart reviewed Jul 28, 2020

View reviewed changes

src/audio/dsm/sof_dsm.c Outdated Show resolved Hide resolved

ryans-lee force-pushed the sof_dsm_audio_component branch from 6e0af57 to 5ee011a Compare July 29, 2020 22:21

cujomalainey suggested changes Jul 29, 2020

View reviewed changes

src/audio/dsm/sof_dsm_inf.c Outdated Show resolved Hide resolved

src/audio/dsm/sof_dsm.c Outdated Show resolved Hide resolved

ryans-lee unassigned RDharageswari Jul 29, 2020

ryans-lee force-pushed the sof_dsm_audio_component branch 2 times, most recently from 6f15b9c to 589439e Compare July 31, 2020 01:06

lgirdwood requested changes Aug 3, 2020

View reviewed changes

ryans-lee force-pushed the sof_dsm_audio_component branch from 589439e to e60e820 Compare August 8, 2020 15:49

lgirdwood requested changes Aug 10, 2020

View reviewed changes

ryans-lee force-pushed the sof_dsm_audio_component branch from e60e820 to 2de5785 Compare August 11, 2020 03:04

ryans-lee force-pushed the sof_dsm_audio_component branch 2 times, most recently from 9bd9b90 to 47b9e16 Compare August 14, 2020 21:15

ryans-lee mentioned this pull request Aug 15, 2020

smart amp: added Maxim DSM audio component with static library #3302

Closed

lgirdwood reviewed Aug 17, 2020

View reviewed changes

ryans-lee force-pushed the sof_dsm_audio_component branch from 47b9e16 to 10235e9 Compare August 17, 2020 23:42

lgirdwood approved these changes Aug 20, 2020

View reviewed changes

cujomalainey reviewed Aug 20, 2020

View reviewed changes

ryans-lee force-pushed the sof_dsm_audio_component branch from 10235e9 to 0b56909 Compare August 24, 2020 20:56

ryans-lee requested review from cujomalainey and xiulipan August 26, 2020 15:13

lgirdwood requested changes Aug 27, 2020

View reviewed changes

Added Maxim DSM audio component

6df8437

Maxim DSM(Dynamic Speaker Management) audio component is added. This component takes stereo input + 4 channel IV feedback. Signed-off-by: Ryan Lee <ryans.lee@maximintegrated.com>

ryans-lee force-pushed the sof_dsm_audio_component branch from 0b56909 to 6df8437 Compare August 27, 2020 17:03

ryans-lee requested review from lgirdwood and plbossart August 27, 2020 17:10

lgirdwood approved these changes Aug 28, 2020

View reviewed changes

cujomalainey reviewed Aug 31, 2020

View reviewed changes

cujomalainey approved these changes Aug 31, 2020

View reviewed changes

lgirdwood merged commit 26875c7 into thesofproject:master Sep 1, 2020

RDharageswari mentioned this pull request Sep 2, 2020

Pass uuid token to the smart_amp widget #3374

Merged

smart amp: added Maxim DSM audio component #3248

smart amp: added Maxim DSM audio component #3248

Conversation

ryans-lee commented Jul 28, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cujomalainey commented Jul 28, 2020

Uh oh!

ryans-lee commented Jul 29, 2020

Uh oh!

cujomalainey left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryans-lee commented Aug 8, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryans-lee commented Aug 14, 2020

Uh oh!

lgirdwood 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!

ryans-lee commented Aug 18, 2020

Uh oh!

lgirdwood commented Aug 20, 2020

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!

ryans-lee commented Aug 24, 2020

Uh oh!