Convert the SRC component to use the module interface

[gongjun-song]

Adopt module interface for src component.

Signed-off-by: Gongjun Song gongjun.song@intel.com

[gongjun-song]

The SRC module will have compatibility issues when implementing the module adapter on ipc3, so ipc3 retains the original implementation of comp_driver, and only implements the module adapter for ipc4.

Reasons for compatibility:
comp_driver executes the src_params function first, and then executes the dai_params function. The log is as follows ("ERROR gongjun: dai_verify_params()" is not a real error, just the log of the print position):

[    24094603.886316] (         129.270828) c0 src          2.9        src/audio/src/src.c:676  INFO src_params()
[    24094630.605065] (          26.718748) c0 src          2.9        src/audio/src/src.c:713  INFO src_params(), source_rate = 16000, sink_rate = 48000, format = 0
[    24094649.198815] (          18.593750) c0 src          2.9        src/audio/src/src.c:715  INFO src_params(), sourceb->channels = 2, sinkb->channels = 2, dev->frames = 48
[    24094794.042559] (         144.843750) c0 dai          2.14        src/ipc/ipc3/dai.c:110  INFO dai_data_config() dai type = 1 index = 0 dd 0x9e09b700
[    24094816.698808] (          22.656250) c0 dai          2.14    src/audio/dai-legacy.c:307  ERROR gongjun: dai_verify_params(): pcm rate parameter 48000, hardware rate 48000

If ipc3 is changed to module adapter, the dai_params function will be executed first, and then the src_params function will be executed (src_params is actually in the src_prepare function. "ERROR gongjun: dai_verify_params()" is not a real error, just the log of the print position).

[    15435984.647046] (          22.083332) c0 dai          2.14    src/audio/dai-legacy.c:306  ERROR gongjun: dai_verify_params(): pcm rate parameter 48000, hardware rate 48000
[    15436065.219959] (          80.572914) c0 dai          2.14    src/audio/dai-legacy.c:363  INFO dai_playback_params() dest_dev = 2 stream_id = 0 src_width = 4 dest_width = 4
[    15436085.428292] (          20.208332) c0 dai          2.14    src/audio/dai-legacy.c:369  INFO dai_playback_params() fifo 0x77010
[    15436124.178290] (          38.750000) c0 pipe         2.15  ....../pipeline-params.c:343  INFO pipe prepare
[    15436190.949121] (          66.770828) c0 src          2.9        src/audio/src/src.c:846  INFO src_prepare()
[    15436210.584537] (          19.635416) c0 src          2.9        src/audio/src/src.c:643  INFO src_params()
[    15436234.542869] (          23.958332) c0 src          2.9        src/audio/src/src.c:680  INFO src_params(), source_rate = 48000, sink_rate = 48000, format = 0

Explain why there is no error in the above module adapter log:
The log is to print the log when 48khz is selected, because if other rate is selected, an error will be reported at the position of dai_params, and the subsequent src_params cannot be executed, we unable to confirm the location of src_params when debugging . Like this:

[    72535328.159372] (          22.291666) c0 dai          2.14    src/audio/dai-legacy.c:308  ERROR dai_verify_params(): pcm rate parameter 16000 does not match hardware rate 48000
[    72535384.826036] (          56.666664) c0 dai          2.14    src/audio/dai-legacy.c:483  ERROR dai_params(): pcm params verification failed.
[    72535426.544784] (          41.718750) c0 pipe         2.15  ....../pipeline-params.c:273  ERROR pipeline_params(): ret = -22, host->comp.id = 8
[    72535467.794783] (          41.250000) c0 ipc                  src/ipc/ipc3/handler.c:305  ERROR ipc: pipe 2 comp 8 params failed -22
[    72535508.732281] (          40.937500) c0 pipe         2.15  ......./pipeline-graph.c:400  INFO pipe reset
[    72535646.805192] (         138.072906) c0 src          2.9      src/audio/component.c:66   INFO comp_set_state(), state already set to 1

[gongjun-song]

SRC's Ipc4 module adapter depends on the PR: thesofproject/rimage#116

[lgirdwood]

@singalsu pls review.

[lyakh]

It looks like it should be possible to reduce the size of this patch a lot, this would make reviewing it easier and reduce the probability of errors

[marc-hb]

It looks like it should be possible to reduce the size of this patch a lot, this would make reviewing it easier and reduce the probability of errors

Not just a review issue:

https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes

[gongjun-song]

It looks like it should be possible to reduce the size of this patch a lot, this would make reviewing it easier and reduce the probability of errors

Not just a review issue:

https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes

Thanks‘s for suggestion, I have changed code.

[marc-hb]

The compilation failures in https://github.com/thesofproject/sof/actions/runs/3523675882/jobs/5908116892 seem relevant

[gongjun-song]

The compilation failures in https://github.com/thesofproject/sof/actions/runs/3523675882/jobs/5908116892 seem relevant

Thanks, have fixed.

[singalsu]

SRC is very tricky, how has this been tested? The IPC3 SRC has been tested in testbench with tools/test/audio/src_test.m. But there's no IPC4 support in testbench.

[gongjun-song]

SRC is very tricky, how has this been tested? The IPC3 SRC has been tested in testbench with tools/test/audio/src_test.m. But there's no IPC4 support in testbench.

Due to commit 1dffde1, the testbench execution failed(program hangs), so I cannot use testbench to test the src of ipc3 (it seems like a bug, neither the version I wrote nor the default version of sof can work).

But whether it is ipc3 or ipc4, I use different rates to play audio, the sound is played normally, and there is no noise.
Test command:
$: aplay -t wav -r 16000 -Dplughw:0,0 ./audio_48k_10s_s16.wav
$: aplay -t wav -r 44100 -Dplughw:0,0 ./audio_48k_10s_s44.wav
$: aplay -t wav -r 48000 -Dplughw:0,0 ./audio_48k_10s_s48.wav

[lgirdwood]

@ranj063 can you take a look too, thanks :) Want to get all modules done for v2.5

[marc-hb]

Due to commit 1dffde1, the testbench execution failed(program hangs), so I cannot use testbench to test the src of ipc3 (it seems like a bug, neither the version I wrote nor the default version of sof can work).

https://github.com/thesofproject/sof/actions/runs/3565026602/jobs/5989687227 is green. Should it be red?

[gongjun-song]

Due to commit 1dffde1, the testbench execution failed(program hangs), so I cannot use testbench to test the src of ipc3 (it seems like a bug, neither the version I wrote nor the default version of sof can work).

https://github.com/thesofproject/sof/actions/runs/3565026602/jobs/5989687227 is green. Should it be red?

There is no problem with the test results of CI, I am not using the same script as the CI test.
CI uses ./scripts/host-testbench.sh, and I use tools/test/audio/src_test.sh for testing.

The script src_test.sh also test fails with default SOF on my machine, which is not caused by this PR.
In addition, the SRC ipc3 in this PR still maintains the original comp_driver processing method.

[singalsu]

SRC is very tricky, how has this been tested? The IPC3 SRC has been tested in testbench with tools/test/audio/src_test.m. But there's no IPC4 support in testbench.

Due to commit 1dffde1, the testbench execution failed(program hangs), so I cannot use testbench to test the src of ipc3 (it seems like a bug, neither the version I wrote nor the default version of sof can work).

But whether it is ipc3 or ipc4, I use different rates to play audio, the sound is played normally, and there is no noise. Test command: $: aplay -t wav -r 16000 -Dplughw:0,0 ./audio_48k_10s_s16.wav $: aplay -t wav -r 44100 -Dplughw:0,0 ./audio_48k_10s_s44.wav $: aplay -t wav -r 48000 -Dplughw:0,0 ./audio_48k_10s_s48.wav

The subjective listening can easily miss a small issue like -60 dB THD+N vs. -90 dB THD+N. The src_test.m modification to operate on a real device with nocodec SSP loopback would be a reliable quality test. Use aplay and arecord instead of testbench launch in the script. Could such enhance be taken to plans? Also enhancing testbench for IPC4 would be essential since we are dropping IPC3 from future releases.

[singalsu]

@gongjun-song I think I can fix the topology that src_test.m uses, increasing source and sink buffer should help the freezing 11.025 kHz rates to work. I'll try it today. Also testbench needs some max copy() count check and error to prevent freezes.

[lyakh]

Thanks for splitting your first version of this PR. Indeed, separating moving functions around to new locations into separate commits. I still think it could be done with less moving around. Your reason for moving functions is to keep only one major #if CONFIG_IPC_MAJOR_4 switch in the code, but you already have two of them. So, if you wanted to have one - yes, you have to move code around. But if you're fine with having multiple of them - you should be able to keep most of the current code in place. But that's less important, I guess. But buffer acquisition / releasing must be fixed

[singalsu]

@gongjun-song My (draft) fix for SRC freeze is #6688

[marc-hb]

There is no problem with the test results of CI, I am not using the same script as the CI test.
CI uses ./scripts/host-testbench.sh, and I use tools/test/audio/src_test.sh for testing.

If there is a script already written and available that finds bugs in a reasonably short time then it must be run in CI for every PR and catch regressions.

Whenever someone says "I'm testing something different from CI" then we have a serious test gap almost every time. It's a big red flag; absolutely not OK. Every product is only as good as the tests it's passing.

@singalsu can you please add src_test.sh to PR testing? Unless your #6668 already does something equivalent?

[singalsu]

Whenever someone says "I'm testing something different from CI" then we have a serious test gap almost every time. It's a big red flag; absolutely not OK. Every product is only as good as the tests it's passing.

@singalsu can you please add src_test.sh to PR testing? Unless your #6668 already does something equivalent?

Yep, would adding it to scripts/host-testbench.sh be OK? It would add about 1-2 minute of testing time as quick test.

[marc-hb]

Yep, would adding it to scripts/host-testbench.sh be OK?

It depends: is src_test.sh related / similar to host-testbench.sh? If not then it's better to keep them separate and append two lines at the bottom of .github/workflows/testbench.yml

By the way I had a look at src_test.sh and the very little it does looks silly: why can't octave print PASS/FAIL itself?

It would add about 1-2 minute of testing time as quick test.

This is a question only for the main developers = you: for how long are you ready to wait for test results?

[gongjun-song]

@gongjun-song My (draft) fix for SRC freeze is #6688

Thank you, based on the #6688, there is no problem running src_test.sh with SRC ipc3 of this PR, and all cases have passed.
For IPC4, @andrula-song will help write testbench for SRC in 2023-Q1.

@singalsu, in addition to the above testbench issue, could you please review the code to see if there are any other issues? thanks!

[lgirdwood]

@gongjun-song My (draft) fix for SRC freeze is #6688

Thank you, based on the #6688, there is no problem running src_test.sh with SRC ipc3 of this PR, and all cases have passed. For IPC4, @andrula-song will help write testbench for SRC in 2023-Q1.

@singalsu, in addition to the above testbench issue, could you please review the code to see if there are any other issues? thanks!

#6688 now merged - @singalsu @ranj063 pls review.

[lyakh]

LGTM now, just a couple of minor comments