Audio: IGO_NR: Convert to module adapter by singalsu · Pull Request #8301 · thesofproject/sof

singalsu · 2023-10-10T16:47:55Z

This is first "working" draft IPC4 version, but processing with stub library is forced to 48 frames for 1 ms LL scheduling. It obviously can't be used with the real processing library. For 16 ms need to change this further to support DP mode in IPC4.

lgirdwood

Looks clean.

lyakh

that's a lot of rework!

btian1

that's definitely not one PR work, right? title is convert to module interface, inside PR, at least 3 tasks are done, do you want to split it to several PRs? for better review/merge/track?

singalsu · 2023-12-12T12:00:45Z

that's definitely not one PR work, right? title is convert to module interface, inside PR, at least 3 tasks are done, do you want to split it to several PRs? for better review/merge/track?

The problem is that the component didn't work even with IPC3. It's impossible to test without all the changes included. Since this is a closed component and not useful without licensing it, I'd like to pass this with small effort to make this testable again. The owner or licensors of this should do the extra polishing effort.

lgirdwood · 2023-12-18T16:39:58Z

that's definitely not one PR work, right? title is convert to module interface, inside PR, at least 3 tasks are done, do you want to split it to several PRs? for better review/merge/track?
The problem is that the component didn't work even with IPC3. It's impossible to test without all the changes included. Since this is a closed component and not useful without licensing it, I'd like to pass this with small effort to make this testable again. The owner or licensors of this should do the extra polishing effort.

Ack - the aim here is not to change functionality or improve performance, but to do the mechanical work to bring it to the module API. Once at module API, it can then be integrated by folks who work on this module.

lyakh · 2023-12-19T13:57:00Z

+	case SOF_IPC4_SWITCH_CONTROL_PARAM_ID:
+ comp_info(dev, "igo_nr_cmd_set_config(), SOF_IPC4_SWITCH_CONTROL_PARAM_ID");
+ return igo_nr_set_chan(mod, ctl);
+ return 0;


redundant line

Oops, wasn't fixed. Done now.

btian1 · 2023-12-20T07:07:40Z

+ ptr = (char *)ptr - cir_buf_size;
+
+	return ptr;
+}


static inline void *cir_buf_wrap(void *ptr, void *buf_addr, void *buf_end)
{
if (ptr >= buf_end)
ptr = (char *)buf_addr +
((char *)ptr - (char *)buf_end);
assert((intptr_t)ptr <= (intptr_t)buf_end); return ptr;
}
why not directly use this?

I don't see issues in what I proposed.

check https://github.com/thesofproject/sof/blob/main/src/include/sof/audio/audio_stream.h#L414
you don't need define a new one.

OK, I've changed and tested it now - pushing new version!

lgirdwood · 2023-12-20T10:18:48Z

@lyakh @btian1 we only need to focus review on the changes to use the module API. This PR is not about improving the existing IGO code as we need to minimize the change surface here to module API integration only. So yes, there are lots of ways the IGO code can be improved, but we need to get integration with module API done before any other code improvements. Same goes for all 3P modules integrations to module API.

singalsu · 2024-01-04T13:31:38Z

I rebased and fixed rimage related conflicts. This version sounds choppy in playback test (sof-hda-benchmark-igo_nr32-tgl.tplg).

singalsu · 2024-01-04T15:32:31Z

I rebased and fixed rimage related conflicts. This version sounds choppy in playback test (sof-hda-benchmark-igo_nr32-tgl.tplg).

Correction, the issue was in my test setup: DUT -> USB sound card -> PC. Hooking headphones directly to DUT fixed the choppy playback. Also I was able to fix the choppy sounding test setup afterwards. So, this version is OK.

Also note for testing. Since there is no DP scheduler for TGL, I'm not able to process 768 frame size (16 ms), so I've made this code change (not in PR) just for my own DUT test run:

diff --git a/src/include/sof/audio/igo_nr/igo_nr_comp.h b/src/include/sof/audio/igo_nr/igo_nr_comp.h
index 82bf9f2d3..b1ba5f0d1 100644
--- a/src/include/sof/audio/igo_nr/igo_nr_comp.h
+++ b/src/include/sof/audio/igo_nr/igo_nr_comp.h
@@ -13,7 +13,7 @@
#include <sof/audio/igo_nr/igo_lib.h>
#include <user/igo_nr.h>
-#define IGO_FRAME_SIZE (768)
+#define IGO_FRAME_SIZE (48)
#define IGO_NR_IN_BUF_LENGTH (IGO_FRAME_SIZE)
#define IGO_NR_OUT_BUF_LENGTH (IGO_FRAME_SIZE)

Need to address the DP rimage mode for MTL and example topology later. In TGL changing the period in topology to 16 ms has no impact since IPC4 assumes 1 ms for LL scheduling.

btian1

LGTM, only one open left.

btian1 · 2024-01-16T13:29:17Z

+ ptr = (char *)ptr - cir_buf_size;
+
+	return ptr;
+}


check https://github.com/thesofproject/sof/blob/main/src/include/sof/audio/audio_stream.h#L414
you don't need define a new one.

lgirdwood · 2024-01-17T13:20:22Z

@singalsu conflicts, probably since RTNR merged.

This patch converts this component to module adapter API. - New() becomes simpler init() - Params() is handled in module adapter, in init() "mod->verify_params_flags = BUFF_PARAMS_RATE;" replaces igo_nr_verify_params(). - Rates check in params() is placed to separate function igo_nr_check_params() to be called from prepare(). - cmd() handler is changed to module adapter client style with igo_nr_set_config() and igo_nr_get_config(). - igo_nr_process() and igo_nr_copy() are merged to module API style igo_nr_process() function. - igo_nr_prepare() sets source and sink align constraints for processing. - igo_nr_trigger() is removed, handled by module adapter. - Changes to support IPC4. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Function igo_nr_init() expects a nonzero value for rballoc() size so this can't be left zero. The value is just something for stub usage that felt suitable for an unknown. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This allows to build and load IGO NR stub version to testbench. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

The source sink API is needed for DP (data processing) scheduler that is needed for 16 ms schedule rate. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch enables to load the component in the changed platforms. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu requested a review from fuyuntsuo October 10, 2023 16:52

singalsu commented Oct 10, 2023
View reviewed changes

Comment threadsrc/audio/igo_nr/igo_nr.c

singalsu commented Oct 11, 2023
View reviewed changes

Comment threadsrc/audio/igo_nr/igo_nr.c

lgirdwood reviewed Oct 17, 2023
View reviewed changes

Comment threadsrc/audio/igo_nr/igo_nr.c

singalsu force-pushed the igonr_module_convert branch from 9918c61 to cd4e32aCompare October 27, 2023 16:36

lyakh reviewed Oct 30, 2023
View reviewed changes

Comment threadsrc/audio/igo_nr/igo_nr.c Outdated
Comment threadsrc/audio/igo_nr/igo_nr.c Outdated
Comment threadsrc/audio/igo_nr/igo_nr.c Outdated

singalsu force-pushed the igonr_module_convert branch from cd4e32a to 5d0fd84Compare November 1, 2023 16:30

singalsu marked this pull request as ready for review November 2, 2023 12:59

singalsu requested review from dbaluta, kv2019i, lbetlej, mmaka1, plbossart and ranj063 as code owners November 2, 2023 12:59

singalsu requested review from lgirdwood and lyakh November 2, 2023 13:01

lyakh reviewed Nov 3, 2023
View reviewed changes

Comment threadsrc/audio/igo_nr/igo_nr.c Outdated
Comment threadsrc/audio/igo_nr/igo_nr.c Outdated
Comment threadsrc/audio/igo_nr/igo_nr.c
Comment threadsrc/include/sof/audio/igo_nr/igo_nr_comp.h
Comment threadsrc/audio/igo_nr/igo_nr.c

btian1 reviewed Nov 6, 2023
View reviewed changes

Comment threadsrc/audio/igo_nr/igo_nr.c

singalsu force-pushed the igonr_module_convert branch from 5d0fd84 to 2af73d8Compare November 6, 2023 10:45

singalsu requested review from btian1 and lyakh November 6, 2023 10:45

singalsu force-pushed the igonr_module_convert branch from 2af73d8 to 2a18630Compare November 6, 2023 10:54

lyakh reviewed Nov 7, 2023
View reviewed changes

Comment threadsrc/audio/igo_nr/igo_nr.c
Comment threadsrc/include/sof/audio/igo_nr/igo_nr_comp.h
Comment threadsrc/audio/igo_nr/igo_nr.c Outdated

btian1 reviewed Nov 10, 2023
View reviewed changes

Comment threadsrc/audio/igo_nr/igo_nr.c
Comment threadsrc/audio/igo_nr/igo_nr.c
Comment threadsrc/audio/igo_nr/igo_nr_stub.c

kv2019i added this to the v2.9 milestone Nov 29, 2023

kv2019i mentioned this pull request Nov 29, 2023
[FEATURE] Update and remove passthrough definitions #4487
Closed

singalsu force-pushed the igonr_module_convert branch from 2a18630 to ffca82fCompare December 19, 2023 12:22

singalsu requested a review from lyakh December 19, 2023 12:22

singalsu requested a review from btian1 December 19, 2023 12:22

lyakh reviewed Dec 19, 2023
View reviewed changes

btian1 reviewed Dec 20, 2023
View reviewed changes

singalsu force-pushed the igonr_module_convert branch from ffca82f to 4b17596Compare January 4, 2024 13:27

singalsu force-pushed the igonr_module_convert branch from 4b17596 to 582836cCompare January 12, 2024 15:09

singalsu requested review from btian1 and lyakh January 12, 2024 15:10

singalsu force-pushed the igonr_module_convert branch from 582836c to 4150713Compare January 12, 2024 15:32

btian1 reviewed Jan 16, 2024
View reviewed changes

singalsu force-pushed the igonr_module_convert branch from 4150713 to 419e505Compare January 16, 2024 15:27

singalsu requested a review from btian1 January 16, 2024 15:30

btian1 approved these changes Jan 17, 2024
View reviewed changes

singalsu force-pushed the igonr_module_convert branch from 419e505 to 068240bCompare January 17, 2024 11:13

singalsu added 5 commits January 17, 2024 17:49

Tools: Testbench: Build IGO_NR with stub for testbench
37c18f5
This allows to build and load IGO NR stub version to testbench. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Audio: IGO_NR: Convert to source sink API
f5e3629
The source sink API is needed for DP (data processing) scheduler that is needed for 16 ms schedule rate. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

Tools: Rimage: Config: Add IGO NR component for TGL, MTL, LNL platforms
3685642
This patch enables to load the component in the changed platforms. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu force-pushed the igonr_module_convert branch from 068240b to 3685642Compare January 17, 2024 16:03

lgirdwood approved these changes Jan 18, 2024
View reviewed changes

lgirdwood merged commit 7b5475b into thesofproject:mainJan 18, 2024

Uh oh!

Conversation

singalsu commented Oct 10, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

btian1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

singalsu commented Dec 12, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Dec 18, 2023

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgirdwood commented Dec 20, 2023

Uh oh!

singalsu commented Jan 4, 2024

Uh oh!

singalsu commented Jan 4, 2024

Uh oh!

btian1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Jan 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

singalsu commented Oct 10, 2023•
edited
Loading

singalsu commented Dec 12, 2023•
edited
Loading