Skip to content

Conversation

@RanderWang
Copy link
Collaborator

Adopt module interface for eq_fir.

Signed-off-by: Rander Wang rander.wang@intel.com

@RanderWang RanderWang force-pushed the eq_fir_module_interface branch 2 times, most recently from 331f261 to f031555 Compare August 26, 2022 05:44
@RanderWang RanderWang changed the title [NOT REWVIEW] eq_fir: use module interface [NOT REVIEW] eq_fir: use module interface Aug 26, 2022
@marc-hb marc-hb marked this pull request as draft August 26, 2022 07:02
@marc-hb marc-hb removed their request for review August 26, 2022 07:02
@marc-hb
Copy link
Collaborator

marc-hb commented Aug 26, 2022

Please use github drafts to reduce spam.

@RanderWang RanderWang force-pushed the eq_fir_module_interface branch from f031555 to 9ef85fa Compare August 26, 2022 08:00
@RanderWang RanderWang changed the title [NOT REVIEW] eq_fir: use module interface eq_fir: use module interface Aug 26, 2022
@RanderWang RanderWang marked this pull request as ready for review August 26, 2022 08:15
@RanderWang RanderWang force-pushed the eq_fir_module_interface branch from 9ef85fa to fea2d20 Compare August 30, 2022 01:46
@RanderWang
Copy link
Collaborator Author

update PR for comments, thanks.

@lgirdwood
Copy link
Member

@RanderWang some build errors in CI.

@RanderWang RanderWang force-pushed the eq_fir_module_interface branch from fea2d20 to 907f414 Compare August 31, 2022 06:31
@RanderWang
Copy link
Collaborator Author

@RanderWang some build errors in CI.

thanks, fixed

@lgirdwood
Copy link
Member

@ranj063 good for you ?

@ranj063
Copy link
Collaborator

ranj063 commented Aug 31, 2022

@ranj063 good for you ?

@RanderWang Could you please let me know how this was tested? There is no support for EQ FIR in any of our topologies, there is no unit tests for EQ FIR and neither does testbench have any support for it.

@RanderWang
Copy link
Collaborator Author

@ranj063 good for you ?

@RanderWang Could you please let me know how this was tested? There is no support for EQ FIR in any of our topologies, there is no unit tests for EQ FIR and neither does testbench have any support for it.

I don't have any method to test it. The original eq-iir test was designed by Seppo, but none for eq-fir. @singalsu do you know how to test eq-fir ?

@RanderWang
Copy link
Collaborator Author

@ranj063 good for you ?

@RanderWang Could you please let me know how this was tested? There is no support for EQ FIR in any of our topologies, there is no unit tests for EQ FIR and neither does testbench have any support for it.

thanks @singalsu I will add a unit test

@RanderWang RanderWang force-pushed the eq_fir_module_interface branch from 31fdc4a to f75fec1 Compare September 9, 2022 07:07
@lgirdwood
Copy link
Member

@RanderWang you may need to force push again as internal CI could be blocked.
@ranj063 what are the next steps, did you have a PR you wanted to merge first that could help this PR ?

@RanderWang
Copy link
Collaborator Author

@RanderWang you may need to force push again as internal CI could be blocked. @ranj063 what are the next steps, did you have a PR you wanted to merge first that could help this PR ?

@lgirdwood My PR can works with zephyr build now and it was caused by dma trace setting on my device. But I find a strange behavior. 07_05_TestKdDmicD0ix16000Hz24b32b2ch is failed on JSL. It can pass without my last commit: eq_fir interface changing although it doesn't use eq-fir. One possible reason is that the memory usage maybe increased by this commit which results to this issue on JSL. I am not sure and need time to check it.

If @ranj063 has some breakthrough, I will try it.

@ranj063
Copy link
Collaborator

ranj063 commented Sep 9, 2022

@RanderWang you may need to force push again as internal CI could be blocked. @ranj063 what are the next steps, did you have a PR you wanted to merge first that could help this PR ?

@lgirdwood I am working on the unit test infra for module_adapter and will have it ready by next week. Lets merge this PR now with the condition that @RanderWang will add the unit tests for eq fir once I have the infra ready

@ranj063
Copy link
Collaborator

ranj063 commented Sep 9, 2022

@RanderWang you may need to force push again as internal CI could be blocked. @ranj063 what are the next steps, did you have a PR you wanted to merge first that could help this PR ?

@lgirdwood My PR can works with zephyr build now and it was caused by dma trace setting on my device. But I find a strange behavior. 07_05_TestKdDmicD0ix16000Hz24b32b2ch is failed on JSL. It can pass without my last commit: eq_fir interface changing although it doesn't use eq-fir. One possible reason is that the memory usage maybe increased by this commit which results to this issue on JSL. I am not sure and need time to check it.

If @ranj063 has some breakthrough, I will try it.

@RanderWang if the test doesnt use EQ FIR, how will the memory usage increase without even setting up the component?

@RanderWang
Copy link
Collaborator Author

@RanderWang you may need to force push again as internal CI could be blocked. @ranj063 what are the next steps, did you have a PR you wanted to merge first that could help this PR ?

@lgirdwood My PR can works with zephyr build now and it was caused by dma trace setting on my device. But I find a strange behavior. 07_05_TestKdDmicD0ix16000Hz24b32b2ch is failed on JSL. It can pass without my last commit: eq_fir interface changing although it doesn't use eq-fir. One possible reason is that the memory usage maybe increased by this commit which results to this issue on JSL. I am not sure and need time to check it.
If @ranj063 has some breakthrough, I will try it.

@RanderWang if the test doesnt use EQ FIR, how will the memory usage increase without even setting up the component?

I meant code segment. Compiler may generate more binary and use more dsp memory.

@ranj063
Copy link
Collaborator

ranj063 commented Sep 13, 2022

I meant code segment. Compiler may generate more binary and use more dsp memory.

if EQ FIR is not used on JSL, why not unselect it from the default modules built into the FW?

@RanderWang RanderWang force-pushed the eq_fir_module_interface branch 3 times, most recently from 1410be7 to 181157b Compare September 13, 2022 02:47
@RanderWang
Copy link
Collaborator Author

SOFCI TEST

@RanderWang
Copy link
Collaborator Author

I meant code segment. Compiler may generate more binary and use more dsp memory.

if EQ FIR is not used on JSL, why not unselect it from the default modules built into the FW?

It is not used by CI test but may be used by JSL. And no easy way to just disable it on JSL

@RanderWang
Copy link
Collaborator Author

update. I found the heap buffer size is decreased significantly with this PR on JSL. I am debugging it.

@RanderWang
Copy link
Collaborator Author

@lgirdwood The failed test case on a JSL RVP is caused by abnormal heap buffer size. But heap buffer size is correct with my PR on a CI JSL RVP, so there is something wrong with the CI build for JSL. And the test is using XTOS build, not zephyr version which doesn't has such heap buffer. @keqiaozhang , do you know how does this test item build fw ?

@lgirdwood
Copy link
Member

@ranj063 any update on your work to help module API updates ?

Adopt module interface for eq_fir component.

Signed-off-by: Rander Wang <rander.wang@intel.com>
@RanderWang RanderWang force-pushed the eq_fir_module_interface branch from 181157b to 2a599bf Compare September 16, 2022 08:43
@RanderWang
Copy link
Collaborator Author

@lgirdwood @ranj063 @singalsu This patch passed the unit test #6308 which uses pass-through coef. I have to use pass-through coef since I cooperated Andrula but still can't find a valid input & ref output. Seppo will help to improve this unit test. For no-pass-through coef, no fw panic found although the result can't match ref

@lgirdwood
Copy link
Member

@RanderWang can you provide the test result for @singalsu to comment ?

@RanderWang
Copy link
Collaborator Author

@RanderWang can you provide the test result for @singalsu to comment ?
Sure. It is based on pass-through coef. Seppo will help to use other coef.

(helper.c:310) comp new 0x55960ab4f800U type 14 id 0.0
(generic.c:84) module_init() start
(eq_fir.c:324) eq_fir_init()
(generic.c:117) module_init() done
(ipc-helper.c:44) buffer new size 0x180 id 0.0 flags 0x0
(ipc-helper.c:44) buffer new size 0x180 id 0.0 flags 0x0
(eq_fir.c:476) eq_fir_prepare()
(eq_fir.c:184) eq_fir_init_coef(), response assign for 2 channels, 1 responses
(eq_fir.c:228) eq_fir_init_coef(), ch 0 is set to bypass
(eq_fir.c:228) eq_fir_init_coef(), ch 1 is set to bypass
(eq_fir.c:114) set_fir_func(), SOF_IPC_FRAME_S16_LE
(eq_fir.c:380) eq_fir_free()
ok 1 - test_audio_eq_fir
(helper.c:310) comp new 0x55960ab4f800U type 14 id 0.0
(generic.c:84) module_init() start
(eq_fir.c:324) eq_fir_init()
(generic.c:117) module_init() done
(ipc-helper.c:44) buffer new size 0x300 id 0.0 flags 0x0
(ipc-helper.c:44) buffer new size 0x300 id 0.0 flags 0x0
(eq_fir.c:476) eq_fir_prepare()
(eq_fir.c:184) eq_fir_init_coef(), response assign for 2 channels, 1 responses
(eq_fir.c:228) eq_fir_init_coef(), ch 0 is set to bypass
(eq_fir.c:228) eq_fir_init_coef(), ch 1 is set to bypass
(eq_fir.c:120) set_fir_func(), SOF_IPC_FRAME_S24_4LE
(eq_fir.c:380) eq_fir_free()
ok 2 - test_audio_eq_fir
(helper.c:310) comp new 0x55960ab4f800U type 14 id 0.0
(generic.c:84) module_init() start
(eq_fir.c:324) eq_fir_init()
(generic.c:117) module_init() done
(ipc-helper.c:44) buffer new size 0x300 id 0.0 flags 0x0
(ipc-helper.c:44) buffer new size 0x300 id 0.0 flags 0x0
(eq_fir.c:476) eq_fir_prepare()
(eq_fir.c:184) eq_fir_init_coef(), response assign for 2 channels, 1 responses
(eq_fir.c:228) eq_fir_init_coef(), ch 0 is set to bypass
(eq_fir.c:228) eq_fir_init_coef(), ch 1 is set to bypass
(eq_fir.c:126) set_fir_func(), SOF_IPC_FRAME_S32_LE
(eq_fir.c:380) eq_fir_free()
ok 3 - test_audio_eq_fir
# ok - tests

@RanderWang
Copy link
Collaborator Author

hi all, the #6308 was improved by Seppo and it works with this PR. I will adapt it to new module adapter test in my Q4 task list. Thanks!

@singalsu
Copy link
Collaborator

hi all, the #6308 was improved by Seppo and it works with this PR. I will adapt it to new module adapter test in my Q4 task list. Thanks!

This seems to work well. The test pass criteria was now made more demanding, +/- 1 LSB for 16 bit +/- 2 for 24 bit and +/- 4 for 32 bit vs. float reference.

@lgirdwood
Copy link
Member

@wszypelt @lrudyX could someone check the DMIC result. Not expecting a DMIC failure with this PR.

@RanderWang
Copy link
Collaborator Author

The failed test case on a JSL RVP is caused by abnormal heap buffer size. But heap buffer size is correct with my PR on a CI JSL RVP, so there is something wrong with the CI build for JSL. And the test is using XTOS build, not zephyr version which doesn't has such heap buffer

@lgirdwood The failed test case on a JSL RVP is caused by abnormal heap buffer size. But heap buffer size is correct with my PR on a another JSL RVP, so there is something wrong with the CI build for JSL. And the test is using XTOS build, not zephyr version which doesn't has such heap buffer

@lgirdwood
Copy link
Member

@wszypelt @lrudyX can you retire the older CAVS platforms from daily PR testing and use them for testing on the CAVS production branches only.
We are beginning to see some resource limitations as we add more features for module adapter, IPC4 etc that wont be used on the older HW and hence PR testing the main branch on older HW is starting to impact development for MTL.

@lgirdwood
Copy link
Member

@wszypelt pls let me know when teh older HW is retired and I will merge to keep CI green. Thanks.

@lgirdwood lgirdwood merged commit 72300fe into thesofproject:main Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants