audio: igo_nr: added igo_nr component #4017

fuyuntsuo · 2021-04-09T03:38:43Z

Added intelliGo noise reduction wrapper codes and topology.

The proprietary static library shall be released by intelliGo via private channel upon request.

src/audio/igo_nr/igo_nr.c

src/include/kernel/abi.h

src/include/sof/audio/igo_nr/igo_nr_comp.h

tools/topology/platform/intel/intel-generic-dmic-kwd.m4

src/audio/Kconfig

src/audio/igo_nr/igo_nr.c

cujomalainey · 2021-04-09T15:51:54Z

src/audio/igo_nr/igo_nr.c

please show me where the wrapper would reject, i don't seen anything in the params or prepare callbacks that would reject s24/s32/float or other channel counts or rates

cujomalainey · 2021-04-09T15:53:43Z

src/audio/igo_nr/igo_nr.c

yes, that is fine, but SOF can try and open the component with an invalid format/channels/rate, even if your topology specifies certain config, do not assume that the next configuration engineer will make the same wise decisions. The code should defend against invalid configs.

src/include/sof/audio/igo_nr/igo_nr_comp.h

tools/topology/platform/intel/intel-generic-dmic-kwd.m4

src/audio/Kconfig

src/audio/igo_nr/igo_nr.c

src/include/sof/audio/igo_nr/igo_nr_comp.h

cujomalainey

getting better, but you are still missing your checks in either prepare or params to verify the current hardware params are valid for your capabilities.

src/audio/igo_nr/CMakeLists.txt

src/audio/igo_nr/igo_nr.c

fuyuntsuo · 2021-04-13T09:45:43Z

getting better, but you are still missing your checks in either prepare or params to verify the current hardware params are valid for your capabilities.

Inside set_capture_func of igo_nr_prepare, we will check the sample rate and frame format.

iuliana-prodan · 2021-04-13T14:07:40Z

getting better, but you are still missing your checks in either prepare or params to verify the current hardware params are valid for your capabilities.

Inside set_capture_func of igo_nr_prepare, we will check the sample rate and frame format.

Maybe I'm missing something, but I don't see where do you check for sample rate?
In set_capture_func you filter the frame format.
In igo_nr_lib_init, called from igo_nr_prepare, you just set the number of channels to 1 and the sample rate to 16000.
I don't see any validation.

cujomalainey

component looks good, Intel please verify you are ok with topology changes. Thanks for keeping with this.

fuyuntsuo · 2021-04-29T03:53:17Z

component looks good, Intel please verify you are ok with topology changes. Thanks for keeping with this.

@cujomalainey thanks! Just updated the copyright.

cujomalainey · 2021-04-29T16:06:04Z

I would ideally like to get one more review, @ranj063 mind doing a review?

fuyuntsuo · 2021-05-03T06:17:11Z

Seems like the reviewer has not responded yet. Is there anything I can do?

I just check the CI messages again and fix most of the complains. (The remaining complains have been discussed before.)

Please let me know if there is any PR blocker other than this one.

lgirdwood · 2021-05-03T09:57:49Z

@fuyuntsuo I will ping @ranj063 to take a look.
@kkarask @wszypelt @zrombel looks like a whole WHL series failure in CI, but this is unrelated. Good to go ?

ranj063

I would ideally like to get one more review, @ranj063 mind doing a review?

@cujomalainey working on it now.

ranj063 · 2021-05-03T19:32:22Z

src/audio/igo_nr/igo_nr.c

NULL assignemtn not necessary

With this NULL assignment, the memory allocations afterwards can share the same error handler.

That is, when if (!cd) holds (L243:igo_nr.c), then it goes to the last and rfree(NULL) becomes nop.

sorry not getting this point. Is (!cd) is true, that mean cd is NULL anyway

@fuyuntsuo none of the error paths before your alloc for cd jump to err so this NULL doesn't make a difference

Lets fix this incrementally.

Get it @ranj063 @cujomalainey. This issue has been resolved in #4152.

src/audio/igo_nr/igo_nr.c

ranj063 · 2021-05-03T19:39:55Z

src/audio/igo_nr/igo_nr.c

looks like we dont need ret at all. just remoe it and return 0?

the ret was missing. I add it back. thanks!

sorry not getting it. I meant just return 0 here and remove the declaration for "ret". You're not using it at all other than initializing it to 0

Yes you are right. Return 0 here is just fine. Thanks!

src/audio/igo_nr/igo_nr.c

tools/topology/platform/intel/intel-generic-dmic-kwd.m4

ranj063 · 2021-05-03T19:52:54Z

@ranj063 we also need dynamic pipelines merged for IGO support (to support switching between IGO and passthrough capture using the same DMIC).

@lgirdwood the dynamic pipeline PR is merged to sof-dev. Switching just the pipelines for IGO to dynamic is also supprted in topology. But like we discussed, the dynamic pipelines patches are not upstream yet and also not part of the 5.4 kernel.

cujomalainey

@ranj063 thanks for catching that, I really need to get cppcheck into the CI for those kind of errors.

src/audio/igo_nr/igo_nr.c

This commit added IGO_NR component into sof-tgl-max98373-rt5682.m4 as DMIC capture PCM99 pipeline. Signed-off-by: fy.tsuo <fy.tsuo@intelli-go.com>

fuyuntsuo · 2021-05-04T07:11:21Z

Despite the changes above, I also moved a struct (L32:igo_nr.c) and a macro (L30:igo_nr.c) from igo_nr.h to igo_nr.c because they are intended to be called inside igo_nr.c only.

ranj063 · 2021-05-04T15:25:39Z

src/audio/igo_nr/igo_nr.c

return err so you dont lose the original error?

Done. Thanks!

ranj063 · 2021-05-04T15:28:13Z

src/audio/igo_nr/igo_nr.c

remove newline

ranj063 · 2021-05-04T15:30:11Z

src/audio/igo_nr/igo_nr.c

sorry not getting it. I meant just return 0 here and remove the declaration for "ret". You're not using it at all other than initializing it to 0

ranj063 · 2021-05-04T17:47:18Z

src/audio/igo_nr/igo_nr.c

remove new line

But I get this warning in post-commit check. Or you are saying the new line after comp_info?

@fuyuntsuo this can be fixed incrementally as I'm going to tag v1.8-rc1 today. The newline needs to go after the variable declarations i.e. after line 601 and before 602.

Thanks! This issue has been resolved in #4152.

ranj063 · 2021-05-04T17:49:43Z

Despite the changes above, I also moved a struct (L32:igo_nr.c) and a macro (L30:igo_nr.c) from igo_nr.h to igo_nr.c because they are intended to be called inside igo_nr.c only.

Thanks for the update @fuyuntsuo. Just a few more minor comments

This commit consists of files for adding IGO_NR component in arbitrary topology. The proprietary static library shall be released by intelliGo via private channel upon request. Signed-off-by: fy.tsuo <fy.tsuo@intelli-go.com>

fuyuntsuo · 2021-05-05T04:44:13Z

Despite the changes above, I also moved a struct (L32:igo_nr.c) and a macro (L30:igo_nr.c) from igo_nr.h to igo_nr.c because they are intended to be called inside igo_nr.c only.

Thanks for the update @fuyuntsuo. Just a few more minor comments

NP. Thanks for reviewing the PR as well. All the issues have been addressed. Please review again. Thanks!

lgirdwood · 2021-05-05T11:40:05Z

src/audio/igo_nr/igo_nr.c

Lets fix this incrementally.

lgirdwood · 2021-05-05T11:44:09Z

src/audio/igo_nr/igo_nr.c

Lets remove the enum since ALSA will map the switch as an ON/OFF type control. @fuyuntsuo this can be done as an update.

lgirdwood · 2021-05-05T11:48:18Z

src/audio/igo_nr/igo_nr.c

@fuyuntsuo this can be fixed incrementally as I'm going to tag v1.8-rc1 today. The newline needs to go after the variable declarations i.e. after line 601 and before 602.

lgirdwood · 2021-05-05T11:49:13Z

@fuyuntsuo please fix the comments above in a new PR - we can merge this fix PR for v1.8-rc2

fuyuntsuo · 2021-05-06T07:33:39Z

@fuyuntsuo please fix the comments above in a new PR - we can merge this fix PR for v1.8-rc2

No problem. We will address the remaining issues in the new PR. Thanks!

fuyuntsuo · 2021-05-07T03:31:32Z

@fuyuntsuo please fix the comments above in a new PR - we can merge this fix PR for v1.8-rc2

No problem. We will address the remaining issues in the new PR. Thanks!

The new PR is #4152.

fuyuntsuo requested review from a team, dbaluta, lbetlej, lgirdwood, mmaka1, plbossart and ranj063 as code owners April 9, 2021 03:38

fuyuntsuo requested a review from cujomalainey April 9, 2021 03:40

cujomalainey suggested changes Apr 9, 2021

View reviewed changes

dbaluta reviewed Apr 9, 2021

View reviewed changes

src/audio/Kconfig Outdated Show resolved Hide resolved

fuyuntsuo force-pushed the main branch from 9afcdbc to 0f4db3c Compare April 9, 2021 09:43

fuyuntsuo requested review from cujomalainey and dbaluta April 9, 2021 15:01

fuyuntsuo added the TGL Applies to Tiger Lake label Apr 9, 2021

cujomalainey suggested changes Apr 9, 2021

View reviewed changes

fuyuntsuo removed the TGL Applies to Tiger Lake label Apr 10, 2021

lgirdwood requested changes Apr 11, 2021

View reviewed changes

fuyuntsuo force-pushed the main branch 2 times, most recently from 5dea5af to b018cac Compare April 12, 2021 13:52

fuyuntsuo requested a review from cujomalainey April 12, 2021 13:54

cujomalainey suggested changes Apr 12, 2021

View reviewed changes

src/audio/igo_nr/CMakeLists.txt Outdated Show resolved Hide resolved

src/audio/igo_nr/igo_nr.c Outdated Show resolved Hide resolved

fuyuntsuo force-pushed the main branch from b018cac to 70be3e8 Compare April 13, 2021 08:22

fuyuntsuo requested review from cujomalainey, iuliana-prodan, lgirdwood and paulstelian97 April 13, 2021 08:22

iuliana-prodan reviewed Apr 13, 2021

View reviewed changes

src/audio/igo_nr/igo_nr.c Outdated Show resolved Hide resolved

fuyuntsuo force-pushed the main branch from 70be3e8 to 848468a Compare April 13, 2021 09:46

fuyuntsuo requested review from cujomalainey and marc-hb April 29, 2021 01:32

cujomalainey approved these changes Apr 29, 2021

View reviewed changes

fuyuntsuo force-pushed the main branch from 9f15a87 to d6a63e9 Compare April 29, 2021 03:51

juimonen mentioned this pull request Apr 29, 2021

enable multiple type controls for single widget thesofproject/linux#2423

Merged

lgirdwood approved these changes Apr 29, 2021

View reviewed changes

fuyuntsuo force-pushed the main branch from d6a63e9 to 038d284 Compare May 3, 2021 06:03

fuyuntsuo force-pushed the main branch from 038d284 to 8eb1fd3 Compare May 3, 2021 06:22

ranj063 reviewed May 3, 2021

View reviewed changes

cujomalainey reviewed May 3, 2021

View reviewed changes

src/audio/igo_nr/igo_nr.c Outdated Show resolved Hide resolved

topology: Added IGO_NR in sof-tgl-max98373-rt5682 topology.

8f51096

This commit added IGO_NR component into sof-tgl-max98373-rt5682.m4 as DMIC capture PCM99 pipeline. Signed-off-by: fy.tsuo <fy.tsuo@intelli-go.com>

fuyuntsuo force-pushed the main branch from 8eb1fd3 to 06d8c29 Compare May 4, 2021 07:08

ranj063 reviewed May 4, 2021

View reviewed changes

fuyuntsuo force-pushed the main branch from 06d8c29 to 64d5007 Compare May 5, 2021 04:42

lgirdwood approved these changes May 5, 2021

View reviewed changes

lgirdwood merged commit 37d97d4 into thesofproject:main May 5, 2021

fuyuntsuo mentioned this pull request May 7, 2021

audio: fix igo_nr wrapper code issues. #4152

Merged

audio: igo_nr: added igo_nr component #4017

audio: igo_nr: added igo_nr component #4017

Uh oh!

Conversation

fuyuntsuo commented Apr 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fuyuntsuo commented Apr 13, 2021

Uh oh!

iuliana-prodan commented Apr 13, 2021

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

fuyuntsuo commented Apr 29, 2021

Uh oh!

cujomalainey commented Apr 29, 2021

Uh oh!

fuyuntsuo commented May 3, 2021

Uh oh!

lgirdwood commented May 3, 2021

Uh oh!

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

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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