thesofproject / sof

Sound Open Firmware
Other
560 stars 318 forks source link

[BUG] Did mixer ever work with module_adapter on IPC3? #7326

Closed paulstelian97 closed 1 year ago

paulstelian97 commented 1 year ago

Describe the bug

Whenever I'm trying to play back two streams on the mixer interface, I'm getting an error that prevents the second stream from loading (unable to install hardware params on the kernel side, some state issues on firmware side).

I have looked through the code and as it looks, there is no way it could possibly work. Since Intel uses IPC4 (which has the mixin/mixout code instead) this might have likely flown off the radar. On the imx_stable_v2.4 branch this doesn't happen but that is before the mixer was converted to the module interface. The detail is that mixer requests simple_copy, which module_adapter cannot do if there are multiple inputs. But then that specific issue is hidden by the fact that module_adapter_prepare returns early if the component is already active (which prevents the mixer from returning PPL_STATUS_PATH_STOP, which allows the DAI to receive a second prepare command when already active which is wrong).

(edit: just noticed that simple_copy only fails if BOTH multiple inputs and multiple outputs are there; the failure is still there though)

To Reproduce Just do regular playback on both streams. The commands below can be run in any order; the one that runs the second fails.

aplay -Dhw:1,0 bruno.wav
aplay -Dhw:1,1 bruno.wav

Reproduction Rate Consistent, 10/10.

Expected behavior I would expect that the mixer... actually worked? Yeah.

Impact Important regression.

Environment 1) Branch name and commit hash of the 2 repositories: sof (firmware/topology) and linux (kernel driver).

Screenshots or console output

Firmware error:

[    15453112.354698] (        4072.499756) c0 pipe         3.16  ....../pipeline-params.c:244  INFO pipe params dir 0 frame_fmt 2 buffer_fmt 0 rate 48000
[    15453228.344277] (         115.989578) c0 pipe         3.16  ....../pipeline-params.c:247  INFO pipe params stream_tag 0 channels 2 sample_valid_bytes 4 sample_container_bytes 4
[    15455665.062930] (        2436.718750) c0 dai          1.2     src/audio/dai-legacy.c:658  INFO dai_prepare()    
[    15455790.062925] (         124.999992) c0 dai          1.2     src/audio/dai-legacy.c:608  INFO dai_config_prepare(): Component is in active state.
[    15455948.812919] (         158.750000) c0 dai          1.2      src/audio/component.c:123  ERROR comp_set_state(): wrong state = 5, COMP_TRIGGER_PREPARE
[    15456096.417080] (         147.604156) c0 pipe         3.16  ....../pipeline-params.c:375  ERROR pipeline_prepare(): ret = -22, dev->comp.id = 12
[    15456218.604575] (         122.187492) c0 ipc                  src/ipc/ipc3/handler.c:325  ERROR ipc: pipe 3 comp 12 prepare failed -22
[    15457846.156594] (        1627.552002) c0 mixer        1.0   ......./module/generic.c:274  ERROR module_reset() error 1: module specific reset() failed for comp 0

Userspace:

root@imx8mpevk:/opt/ltp/testcases/bin# aplay -Dhw:0,0 -f S24_LE -c 2 -r 48000 -t raw /dev/urandom 2> output.txt &
[2] 4179
root@imx8mpevk:/opt/ltp/testcases/bin# aplay -Dhw:0,1 -f S24_LE -c 2 -r 48000 -t raw /dev/urandom
Playing raw data '/dev/urandom' :
 Signed 24 bit Little Endian, Rate 48000 Hz, Stereo
aplay: set_params:1416: Unable to install hw params:
ACCESS:  RW_INTERLEAVED
FORMAT:  S24_LE
SUBFORMAT:  STD
SAMPLE_BITS: 32
FRAME_BITS: 64
CHANNELS: 2
RATE: 48000
PERIOD_TIME: (42666 42667)
PERIOD_SIZE: 2048
PERIOD_BYTES: 16384
PERIODS: 4
BUFFER_TIME: (170666 170667)
BUFFER_SIZE: 8192
BUFFER_BYTES: 65536
TICK_TIME: 0
dbaluta commented 1 year ago

@paulstelian97 thanks for the report.

I think integration of MIXER with module_adapter is completely broken. I wonder if there are Intel users of MIXER with IPC3.

Cc: @lgirdwood @lyakh @ranj063

We have this code in module_adapter.c:

int module_adapter_prepare(struct comp_dev *dev)

          /*
»        * check if the component is already active. This could happen in the case of mixer when
»        * one of the sources is already active
»        */
»       if (dev->state == COMP_STATE_ACTIVE)
»       »       return 0;

     ret = module_prepare(mod);
»       if (ret) {
»       »       if (ret == PPL_STATUS_PATH_STOP)
»       »       »       return ret;

When playing second stream dev->state will already be active and this function will return 0, but IPC3 expects PPL_STATUS_PATH_STOP.

So, quick fix would be:

@@ -181,7 +181,11 @@ int module_adapter_prepare(struct comp_dev *dev)

        if (ret == COMP_STATUS_STATE_ALREADY_SET) {
                comp_warn(dev, "module_adapter_prepare(): module has already been prepared");
+#if CONFIG_IPC_MAJOR_4
+               return 0;
+#else
                return PPL_STATUS_PATH_STOP;
+#endif
        }

But I think that there might be issues as even when we call prepare for second Mixer stream we might want to get into module_prepare for some initialization. And let module_prepare() return COMP_STATUS_STATE_ALREADY_SET

plbossart commented 1 year ago

I am positive that the mixer did work when we tried the multi-capture parts in https://github.com/thesofproject/sof/pull/6131 But that was eons ago, and since Intel doesn't test IPC3 any longer it's possible that there's a coverage gap indeed.

paulstelian97 commented 1 year ago

I don't think the mixer was using module_adapter back then, hell it still doesn't use it in our imx-stable-v2.4 branch.

lgirdwood commented 1 year ago

Ack, it did work but we are no longer testing IPC3 code on main branch - we testing IPC3 on teh stable v2.2 branch. Focus for Intel is now testing IPC4 on our available DUTs. We cant test both IPC3/4 as the DUTs were taking a reliability hit so we needed to lower the burden on them to make then last longer (the DUTs are developer boards not and designed for 24/7 cloud CI usage).

paulstelian97 commented 1 year ago

7357 + #7420 fix this.