Skip to content

[BUG] DSP panic problem in ASRC component  #4068

Closed
#6904
Closed
[BUG] DSP panic problem in ASRC component #4068
#6904
Assignees
singalsu
Labels
ICLApplies to IceLake platformWHLApplies to WhiskeyLake platformbugSomething isn't working as expecteddiagnostic driverSlim driver / diagnostic driver issue
Milestone
v1.8

Description

@kkarask
kkarask
opened on Apr 21, 2021

Describe the bug
In tests 19_00_TestAsrcPlayback8000Hz32b32b2ch and 19_00_TestAsrcPlayback48000Hz16b16b2ch Panic notification occurred.

Topology

              +---------------------------------+
    +------+    | +---+  +----+  +---+  +-------+ |
    | Host |----->|Buf|->|Asrc|->|Buf|->|SSP Dai|---+
    +------+    | +---+  +----+  +---+  +-------+ | |
                +---------------------------------+ |
                                                    |
                +------------------+                |
    +------+    | +---+  +-------+ |                |
    | Host |<-----|Buf|<-|SSP Dai|<-----------------+
    +------+    | +---+  +-------+ |
                +------------------+

To Reproduce
Run tests with Diagnostic driver 19_00_TestAsrcPlayback8000Hz32b32b2ch and 19_00_TestAsrcPlayback48000Hz16b16b2ch.

Reproduction Rate
100%
Problem doesn't exist before dc0ab8a

Environment

  1. Branch name and commit hash of the 2 repositories: sof (firmware/topology) and linux (kernel driver).
  2. Name of the platform(s) on which the bug is observed.
    • Platforms:
      • WHL RVP
      • ICL RVP
  3. Test environment:
    • SOF: BRANCH: master HASH: b7ed915d911acda055bbb822800b76557e9fe7de

Screenshots or console output

2021_04_19_23_43_01.zip

Activity

  1. kkarask
    added
    bugSomething isn't working as expected
    on Apr 21, 2021
  2. zrombel
    added
    WHLApplies to WhiskeyLake platform
    on Apr 21, 2021
  3. kkarask
    changed the title [-][BUG] DSP panic problem[/-] [+][BUG] DSP panic problem in ASRC component [/+] on Apr 21, 2021
  4. zrombel
    added
    ICLApplies to IceLake platform
    on Apr 21, 2021
  5. lgirdwood
    assigned
    singalsu
    on Apr 21, 2021

  6. lgirdwood commented on Apr 21, 2021

    @lgirdwood
    lgirdwood
    on Apr 21, 2021
    Member

    @singalsu 100% reproducible so should be easy to find.

  7. lgirdwood
    added this to the v1.8 milestone on Apr 21, 2021

  8. iuliana-prodan commented on Apr 21, 2021

    @iuliana-prodan
    iuliana-prodan
    on Apr 21, 2021
    Contributor

    @singalsu 100% reproducible so should be easy to find.

    I can confirm that I also found this issue.
    Tried to add ASRC components in an imx pipeline who looked like: PCM0 <----> ASRC <----> volume <-----> SAI1 (wm8960).
    When trying to playback a wav, even with 48kHz it crashes in SOF, I only get the IPC reply in kernel
    (sof-audio-of 556e8000.dsp: error: hw params ipc failed for stream 0) and no logs in sof-logger.

  9. singalsu commented on Apr 22, 2021

    @singalsu
    singalsu
    on Apr 22, 2021
    Collaborator

    Thanks for reporting this! I will start checking tomorrow.

  10. singalsu commented on Apr 23, 2021

    @singalsu
    singalsu
    on Apr 23, 2021
    Collaborator

    Yep, confirm the panic happens with normal SOF linux driver controlled device, no need to set up diagnostic driver that eases the debugging for me.

    Edit: The stack dump shows it happens at dai_ts_config(). I'll find out more next week.

  11. singalsu commented on Apr 26, 2021

    @singalsu
    singalsu
    on Apr 26, 2021
    Collaborator

    This avoid the panic but the pipeline no more works (with SSP DAI and setting up timestamp for SSP uses this information). I wonder if dd->chan has got deprecated.

    diff --git a/src/audio/dai.c b/src/audio/dai.c
index 44c033671..07c80228f 100644
--- a/src/audio/dai.c
+++ b/src/audio/dai.c
@@ -1161,7 +1161,13 @@ static int dai_ts_config(struct comp_dev *dev)
        cfg->direction = dai->direction;
        cfg->index = dd->dai->index;
        cfg->dma_id = dd->dma->plat_data.id;
-       cfg->dma_chan_index = dd->chan->index;
+       if (dd->chan) {
+               cfg->dma_chan_index = dd->chan->index;
+       } else {
+               comp_err(dev, "dai_ts_config(), NULL chan");
+               return -EINVAL;
+       }
+
        cfg->dma_chan_count = dd->dma->plat_data.channels;
        if (!dd->dai->drv->ts_ops.ts_config)
                return -ENXIO;

    @lgirdwood Do you remember if this has been changed?

    Edit: Seems like dai_config_prepare() sets it. Maybe ASRC prepare happens before DAI has been prepared. Maybe ASRC should set up timestamps in first copy instead.

    Edit 2: A fix where ASRC sets up timestamping in trigger start seems to work. Testing...

  12. lgirdwood commented on Apr 26, 2021

    @lgirdwood
    lgirdwood
    on Apr 26, 2021
    Member

    @singalsu any asynchronous APIs should not assume a previous state. In this case the DAI API assumes that the timestamp is setup by prepare(). The DAI API needs to make this clear (where the timestamp and other async features APIs are ready) and survive any attempts to use the API prior to setup. i.e. we check for NULL

  13. iuliana-prodan commented on Apr 27, 2021

    @iuliana-prodan
    iuliana-prodan
    on Apr 27, 2021
    Contributor

    @singalsu I've tested i.MX topology with ASRC, with your patches (#4095), and I don't get the DSP panic, anymore.

    But I still have some questions regarding ASRC.
    It seems to me that I can only convert from 8000 11025 16000 22050 32000 44100 48000 to 48000 Hz.
    If I try a downsampling from 64000 to 48000 I get:

    asrc         1.1      src/audio/asrc/asrc.c:571  asrc_prepare()
asrc         1.1   ...../asrc/asrc_farrow.c:750  ERROR initialise_filter(), fs_in = 64000
asrc         1.1   ...../asrc/asrc_farrow.c:384  ERROR asrc_initialise(), failed filter initialise
asrc         1.1      src/audio/asrc/asrc.c:695  ERROR initialise_asrc(), error -5

    Which seems expected, by looking in asrc_farrow.c, in initialise_filter() and the asrc_conversion_ratios

    But, I'm wondering why do you, in Intel's topologies, like sof-apl-asrc-pcm512x.m4 use 192000 Hz as max rate - see here .
    Is this really converted to 48000 Hz? If yes, how?

    Thanks!

  14. singalsu commented on Apr 28, 2021

    @singalsu
    singalsu
    on Apr 28, 2021
    Collaborator

    @iuliana-prodan Nice that it fixes the issue for you. There's a minor document add need for that patch and then it should be good to go.
    PCM512x codec usually is slave to APL so we can use the sychronous SRC to convert from 192 to 48 kHz. If the codec is master the topology could use SRC and ASRC both in same pipeline. Though this could reveal new pipeline rules issues. This is actually a good case to add to our rules study we are doing with @juimonen . With current quality ASRC coefficients the result could be better with such SRC-ASRC system because the ASRC aliases more. ASRC alone might also be higher load.

  15. lgirdwood commented on Apr 28, 2021

    @lgirdwood
    lgirdwood
    on Apr 28, 2021
    Member

    @singalsu ping - will we see the fix be ready to merge today ?

  16. singalsu
    added a commit that references this issue on Apr 28, 2021

    Audio: ASRC: Move DAI timestamping setup request to trigger start

    53eafd4

  17. singalsu commented on Apr 28, 2021

    @singalsu
    singalsu
    on Apr 28, 2021
    Collaborator

    @lgirdwood I updated the PR, hope the doxygen part add is OK.

    @iuliana-prodan @kkarask There's another problem that I noticed with the I2S devel topology that my other related PR adds. The simultaneous playback and capture with ASRC pipelines into same DAI causes and I/O error for one direction with xrun recover failed error in trace. I'll add a new issue to track the add of the access filtering code to HW timestamps.

  18. iuliana-prodan commented on Apr 28, 2021

    @iuliana-prodan
    iuliana-prodan
    on Apr 28, 2021
    Contributor

    @iuliana-prodan Nice that it fixes the issue for you. There's a minor document add need for that patch and then it should be good to go.

    Is there a document about ASRC? Where?
    I only know about SRC - https://thesofproject.github.io/latest/algos/src/sample_rate_conversion.html

    PCM512x codec usually is slave to APL so we can use the sychronous SRC to convert from 192 to 48 kHz. If the codec is master the topology could use SRC and ASRC both in same pipeline. Though this could reveal new pipeline rules issues. This is actually a good case to add to our rules study we are doing with @juimonen . With current quality ASRC coefficients the result could be better with such SRC-ASRC system because the ASRC aliases more. ASRC alone might also be higher load.

    So, for APL, for example, you use ASRC for upsampling and SRC for downsampling?

    For imx8QM we use wm8960 codec as master.
    You're saying that we can use SRC and ASRC in the same pipeline, something like:
    PCM0 <----> SRC <----> ASRC <---->volume <-----> SAI1 (wm8960)?

  19. singalsu commented on Apr 28, 2021

    @singalsu
    singalsu
    on Apr 28, 2021
    Collaborator

    @iuliana-prodan The history of ASRC is different than SRC. This was a contribution from other team than Liam's and we got permission to publish source code (I did the "SOFifying" work) but not the document that wasn't totally suitable for this need. A from scratch written new document should be possible given some time. Add an enhancement issue for SOF Docs perhaps?

    If there's need for large rate changes, SRC should be better for the hard work. I assume the shipped rates conversions are where the ASRC algorithm performs well. I didn't change those from original. Also some of the Q-format choices inside may to restrict the highest rates or highest in/out ratios so those would need to be checked. For now the SRC-ASRC combination is the recommendation to cover all rates. Load and latency of SRC is low when it does simple 1/N or N fractions. But there will be likely new pipeline params walk issues when SRC and ASRC are used both. Need to test and file issues.

  20. iuliana-prodan commented on Apr 28, 2021

    @iuliana-prodan
    iuliana-prodan
    on Apr 28, 2021
    Contributor

    @singalsu @lgirdwood @zrombel I'm looking on Intel's topologies with SRC and ASRC and on the CI (here, for example).
    Where can I find the tests for:
    11_01_TestSrcSimple16000On48000Hz16b16b2ch or 17_06_TestMultiCoreCompSrcPlb48000Hz24b32b4ch?
    I can only see the logs from the CI.
    How/where can I find out the topology used, the test itself?

    Thanks!

  21. lgirdwood
    closed this as completedin 7522a7bon Apr 28, 2021

  22. singalsu commented on Apr 30, 2021

    @singalsu
    singalsu
    on Apr 30, 2021
    Collaborator

    @iuliana-prodan Those test case implementations are not open source. They use the diagnostic driver (open source under SOF) and I think they are not based on any .tplg but some generated IPC sequence. It would be useful to have ASRC tests in sof-test as well to test via normal topologies and driver too.

  23. wszypelt
    added
    diagnostic driverSlim driver / diagnostic driver issue
    on Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

ICLApplies to IceLake platformWHLApplies to WhiskeyLake platformbugSomething isn't working as expecteddiagnostic driverSlim driver / diagnostic driver issue

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions