Skip to content

Test: Cmocka: Audio: Fix eq_fir blob set IPC length violation#9391

Closed
singalsu wants to merge 5 commits intothesofproject:mainfrom
singalsu:cmocka_fix_eqfir_blob_length
Closed

Test: Cmocka: Audio: Fix eq_fir blob set IPC length violation#9391
singalsu wants to merge 5 commits intothesofproject:mainfrom
singalsu:cmocka_fix_eqfir_blob_length

Conversation

@singalsu
Copy link
Collaborator

No description provided.

The FIR coefficients payload and the binary control set IPC
header need to fit a single 384 byte data structure in
Cmocka environment that is not supporting message split to parts.

This patch replaces the FIR coefficients with a shorter version
while still preserving test coverage for saturation and
sample values difference. Due to coefficients blob change the
reference data also changes.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The filter is shortened to 100 taps to avoid issue with
improved coverage in Cmocka unit tests for IPC length
violations. The earlier filter version was way too long to
fit a single IPC message structure that Cmocka tests need.

The brute force symmetrical filter taps removal (boxcar window) changes
the response but it doesn't impact Cmocka test needs.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu singalsu marked this pull request as ready for review August 22, 2024 14:27
@singalsu singalsu requested a review from cujomalainey August 22, 2024 14:28
@cujomalainey
Copy link
Contributor

I'm going to cherry pick this underneath my size check as a test and see what happens in CI

Copy link
Contributor

@cujomalainey cujomalainey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this doesn't cover the testbench failure

Can you add a commit to fix that as well?

@cujomalainey
Copy link
Contributor

Also looks like imx8 is not covered by this patch either?

17/52 Testing: eq_fir_process                                                                                                                                                                                                                                                                                                                          
17/52 Test: eq_fir_process                                                                                                                                                                                                                                                                                                                             
Command: "/usr/local/google/home/cujomalainey/sof_workspace/sof/build_ut_defs/imx8_defconfig/test/cmocka/src/audio/eq_fir/eq_fir_process"                                                                                                                                                                                                              
Directory: /usr/local/google/home/cujomalainey/sof_workspace/sof/build_ut_defs/imx8_defconfig/test/cmocka/src/audio/eq_fir                                                                                                                                                                                                                             
"eq_fir_process" start time: Aug 22 12:44 PDT                                                                                                                                                                                                                                                                                                          

With some debug I can see the IPC has a size off 552 still

hdr: 116, proc: 552, IPC: 384 

@singalsu
Copy link
Collaborator Author

Also looks like imx8 is not covered by this patch either?

17/52 Testing: eq_fir_process                                                                                                                                                                                                                                                                                                                          
17/52 Test: eq_fir_process                                                                                                                                                                                                                                                                                                                             
Command: "/usr/local/google/home/cujomalainey/sof_workspace/sof/build_ut_defs/imx8_defconfig/test/cmocka/src/audio/eq_fir/eq_fir_process"                                                                                                                                                                                                              
Directory: /usr/local/google/home/cujomalainey/sof_workspace/sof/build_ut_defs/imx8_defconfig/test/cmocka/src/audio/eq_fir                                                                                                                                                                                                                             
"eq_fir_process" start time: Aug 22 12:44 PDT                                                                                                                                                                                                                                                                                                          

With some debug I can see the IPC has a size off 552 still

hdr: 116, proc: 552, IPC: 384 

That's strange. I checked with /scripts/run-mocks.sh with your #9374 that with this it passed. Could there be an incremental build problem with dependencies for imx? There's same FIR unit test configuration for every platform.

@singalsu
Copy link
Collaborator Author

Looks like this doesn't cover the testbench failure
Can you add a commit to fix that as well?

I'm not able to access that link. Is it possible to personal email copy of the test result to me?

Update, sorry for noise, I found a fresh CI result from #9374 testbench step, it must be that. It seems that the testbench also exceeds the IPC size. I think I will first do a workaround with other shorter config to unblock your work, and later replace the binary control set with multiple parts IPC. Testbench run needs the full component configurability.

The eq_fir_coef_loudness.m4 cannot be used in current testbench
version without exceeding the maximum single IPC message size
of 384. The shorter filter eq_fir_coef_mid.m4 does not exceed
the limit so this can be used instead.

This is a workaround to quickly unblock other work in SOF. The
testbench binary control set needs to implement the multi-part
IPC message send to allow test of all supported configurations.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu
Copy link
Collaborator Author

Fixed FIR testbench run, next problem is with multiband-DRC.

@singalsu singalsu marked this pull request as draft August 23, 2024 13:50
This patch removes multiband-DRC from testbench tested
components by this script. It is not possible to have a
meaningful test configuration with IPC message size limit
of 384 for binary control set.

The test can be restored after multi-part IPC is added to
the testbench. This quick workaround is needed to unblock
other development in SOF.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch removes TDFB from testbench tested components
by this script. It is not possible to have a meaningful
test configuration with IPC message size limit of 384 for
binary control set.

The test can be restored after multi-part IPC support is
added to the testbench. This quick workaround is needed
to unblock other development in SOF.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@singalsu
Copy link
Collaborator Author

The added commits disable test of multiband-DRC and TDFB from scripts/host-testbench.sh. I'm not fond of doing that myself. I'll make other PR that increases max. IPC message size for testbench and unit test build. That's a better workaround before I'm able to update multi-part IPC to testbench (not straigtforward because such is not handled in component new() IPC).

@singalsu
Copy link
Collaborator Author

Closing as abandoned approach to work around the issue.

@singalsu singalsu closed this Aug 27, 2024
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.

3 participants