thesofproject / sof

Sound Open Firmware
Other
529 stars 307 forks source link

[BUG][MTL] SSP File is completely silent 24in32bit #8086

Closed wszypelt closed 1 year ago

wszypelt commented 1 year ago

Describe the bug Issue was created after merging 87856a73d77b49bea4db8e75298414b26fccf95a Request for revert and repair

Issue with I2S with config 24in32bit ERROR:SSP File is completely silent (5s_i2sssp_1_24000_4_24_32_buffers_1_1_1_1.wav). Average samples amplitude: 0.25%.

To Reproduce pytest -s "tests/ace/fw_113_ssp/test_02_ssp_internal_loop.py::TestSspInternalLoop::test_113_02_simple_internal_loopback[24000Hz_24in32bit_4ch-ssp_1_to_ssp_1]"

Reproduction Rate 100%

Expected behavior File not silent

Impact all I2S tests with config 24in32bit

Environment 1) * SOF: 87856a73d77b49bea4db8e75298414b26fccf95a

Topology

        """
        OBJECTIVE:
        SSP (I2S mode) internal loopback test.

        TEST STEPS:
        1. Load FW.
        2. Create topology with audio format and SSP instance as in title:
            - Host Output Copier->I2S TX Copier (internal loopback enabled)
            - I2S RX Copier->Host Input Copier (internal loopback enabled)
        3. Run pipelines and streams (5 seconds).
        4. Verify amount of unhandled notification - proper value is 0.
        5. Perform SNR comparison verification method of played and
            recorded file and verify it meets quality criteria.

        PASS CRITERIA:
        All steps passed successfully

        PIPELINES TOPOLOGY::
        +--------------------------------+
        | COPIER_HOST < -- COPIER_SSP-+  |
        |                             |  |
        |                             |  |
        | COPIER_HOST -- > COPIER_SSP-+  |
        +--------------------- ----------+
        """

Screenshots or console output image

Logs: i2s_1_24000_4_24_32_buffers_1_1_1_1.zip grabbed_fw_logs_0.txt

mengdonglin commented 1 year ago

@RanderWang Can you help give a check?

wszypelt commented 1 year ago

@mengdonglin @RanderWang CI is not working properly because of this issue, please revert, then fix the errors

wszypelt commented 1 year ago

after the revert, the error does not occur

RanderWang commented 1 year ago

@lgirdwood @marcinszkudlinski @kv2019i @wszypelt I made great effort and finally found the root reason.

The pipelines are showed in the following graph. All the 4 copiers use 24/32 MSB sample type.

        PIPELINES TOPOLOGY::
        +--------------------------------+
        | COPIER_HOST < -- COPIER_SSP-+  |
        |                             |  |
        |                             |  |
        | COPIER_HOST -- > COPIER_SSP-+  |
        +--------------------- ----------+

The original code choose conversion function : convert_s32_to_s24, copy_stream, copy_stream, convert_s24_to_s32. (this conversion is incorrect for single playback and only work for loop back test but it was not detected by CI test) My PR change it to : copy_stream, copy_stream, copy_stream, copy_stream. This works on Linux tests but is failed with the CI test. Why ?

l did many tests then found it was caused by SSP setting

SOF: ffffff10 c1d0077f d0700004 00000000 02200000 00000003 00000003 00004002 00000000 07070000 00000020 00110001 00000fff

CI test: 00000000 81D00F37 C0700004 00000000 02990000 00000103 00000103 00004002 00000000 07070F00 00000020 00000000 00000000

SOF use 32bits frame width for SSP port but CI use 25 bit frame width with 1 dummy bits. So if converting stream with copy_stream function the MSB 8bits will be clamped by CI blob, but it works with SOF blob. The original code is very lucky to workaround this CI setting.

SOF use 32bits frame width which can deal with different formats and this makes Linux kernel driver easy to set I2S codec to use 32bits format for all cases. How to deal with this CI blob setting ? The real usage on Windows, Linux and chrome don't use this blob, do we need to hack FW to support it ?

// hack in FW
if ( gateway == ssp && 24bit MSB && it is playback)  // we may need to detect this is called by python test or normal Linux....
        use S32_to_S24 conversion function
else if (gateway == ssp && 24bit MSB && it is capture)
        use S24_to_S32 conversion function
lgirdwood commented 1 year ago

@wszypelt @mwasko fyi, need to align SSP blobs with CI. Can CI use upstream blobs ? i.e. we upstream good blobs and align on BKC.

kv2019i commented 1 year ago

Great investigation @RanderWang ! So it sounds like the CI copier configuration should use "32 bit, 24 valid, LSB" for the DAI copiers and then the 25bit SSP blob is correct. Now the copier config and SSP blob mismatch. I assume this passes with reference firmware as it ignores requested copier config, and override the value. But this isn't really good, host should send correct copier configuration.

mwasko commented 1 year ago

@dnikodem FYI

mwasko commented 1 year ago

@wszypelt @mwasko fyi, need to align SSP blobs with CI. Can CI use upstream blobs ? i.e. we upstream good blobs and align on BKC.

@lgirdwood after internal discussion we decided that both SSP blob configurations need to be supported - there are production use-cases. However, to support 25bit SSP blob (24 bit + 1 dummy) the Copier format config must be set correctly (LSB bit) in CI tests.

@wszypelt is coordinating CI tests update.

wszypelt commented 1 year ago

@RanderWang @lgirdwood We changed the tests and I checked them on FW with changes from https://github.com/thesofproject/sof/pull/7959, The tests pass correctly :) However, I have a lot of problems to make changes to the Internal Intel CI, I don't know how long it will take, in this case I will withdraw the 24in32bit I2S test from the Internal Intel CI, and when we deal with the service, I will add the corrected one. I believe that you can already merge the changes that were in the above pull request (7959).

RanderWang commented 1 year ago

@wszypelt thanks very much !

mengdonglin commented 1 year ago

Closing as PR #8151 has been merged. @RanderWang @wszypelt