SAI: less hardcoded

[eurofun]

The imx sai driver currently uses a lot of hardcoded parameters and of 6 sais only 2 are defined in the firmware.
I made changes to have sai2 and 3 work with 8ch each (pb & capture). Tested on my custom imx8mp board, works fine.

I use the btsco codec driver with two dai links in a single simple-audio-card. The linux-imx 6.1 kernel has a problem with that
and I will post details about the problem and my workaround later on.

Is there any reason I can't grasp why not all sais are defined? Is it because that would use a little bit more memory? I'm good with sai2 and 3...

I don't have a NXP evkit so I can't test if the changes break something. One candidate for such behaviour
would be the fifo packing settings: to enable pack8/pack16 and adapt the 'fifo priming' to always write one full frame, but at least one fifo word. I'm not fully understanding why that priming is needed in the first place

Some of the commits are more questions (like on the sdma watermark level) and I'm aware that the comment style likely has to be adapted (used some // comments) among some other things - thus marked as draft.

[sofci]

Can one of the admins verify this patch?

reply test this please to run this test once

[paulstelian97]

Please check these comments. I did not provide a full review as of now, these changes definitely are interesting, but I'm not fully convinced of them yet.

[gkbldcig]

Can one of the admins verify this patch?

[paulstelian97]

The imx sai driver currently uses a lot of hardcoded parameters and of 6 sais only 2 are defined in the firmware. I made changes to have sai2 and 3 work with 8ch each (pb & capture). Tested on my custom imx8mp board, works fine.

The only reason we only defined two SAIs is because we only used those. Feel free to add more as you use them.

I don't have a NXP evkit so I can't test if the changes break something. One candidate for such behaviour would be the fifo packing settings: to enable pack8/pack16 and adapt the 'fifo priming' to always write one full frame, but at least one fifo word. I'm not fully understanding why that priming is needed in the first place

I don't recall exactly why that was added (it wasn't there in my first revision of the driver), but I do have a vague memory of it being done to resolve certain issues due to the timing on when the DMA and DAI drivers are started. If you don't feed those zero bytes the SAI may have a chance to underrun before the DMA is ever started and that can prevent playback from starting. See commit 07da0a8 (taken from "git blame") for more details on that. It is worth verifying if the SOF on Zephyr builds also have the behaviour where said priming is necessary; my guess is "yes" but I don't have the data to say that's right or not.

On the watermark question, again "git blame" points to why it became as it is today -- an experimentally found value. See commit 12fdd63 for more details.

I like that you are asking these questions, despite that meaning more work for us to get things right.

[eurofun]

Thanks for reminding me to use git blame, somehow didn't get around doing it here...
I was expecting something like that for the watermark level and the fifo priming. It works nicely for my dual 8ch setup so something must be right - but certainly a good place for further experiments, time permitting.

What is the easiest way to tell a fw build is using zephyr or xtos? After I updated the main branch today logger started to say
src/audio/dai-legacy.c:655 INFO dai_zephyr_config_prepare(), channel = 0
Is that a sure sign that a switch to zephyr happened?

The failing of checkpatch is a false-positive in my opinion unless forming the plural of SAI as SAIs is really wrong. Will work around that...

[paulstelian97]

What is the easiest way to tell a fw build is using zephyr or xtos? After I updated the main branch today logger started to say src/audio/dai-legacy.c:655 INFO dai_zephyr_config_prepare(), channel = 0 Is that a sure sign that a switch to zephyr happened?

I am unsure exactly how to predict whether it was Zephyr that we're using. The most visible way to see this on an already built firmware file is to see if there are any logs from the scheduler (the LL scheduler logs come from different files between Xtos and Zephyr)

As for firmware you're building yourself, it's pretty simple. If you use the ./scripts/xtensa_build_all.sh way, it's Xtos. If you use ./scripts/xtensa_build_zephyr.py, it's Zephyr. The output files go in different directories too dependent on which way to build it is used.

In particular your own log entry is not really relevant. That function is a poorly named/compatibility shim function, as opposed to being genuinely Zephyr specific. On i.MX we still have Xtos builds (Intel has mostly, if not even entirely, done away with them) however there are plans to do a full migration to Zephyr so that we align with Intel.

[dbaluta]

What is the easiest way to tell a fw build is using zephyr or xtos?

At runtime you can look inside /sys/kerne/debug/sof/cc_info

[dbaluta]

@eurofun thanks a lot for your changes! We will try to review them these days.

Care to share with us what is your audio scenario? and what hardware are you using?

[eurofun]

I will give SOF on Zephyr a try next week, thanks for the info!

@dbaluta The audio scenario is an ip audio gateway with 8 speaker amplifiers on one SAI and 4 codecs on the other SAI. I will check with my bosses how much further I can go into detail...
The hw are custom boards based on imx8mp smarc modules from avnet/msc (thus I'm limited to SAIs 2 and 3).

[lyakh]

Does this mean, that also debug level messages will be printed by default? That might be a rather significant run-time overhead

[paulstelian97]

This looks like another one of the changes that he's only using for testing as opposed to for the actual patch. I guess it's best to skip these things (I believe I made my point quite clear in the idea that this shouldn't be in the final code) when reviewing this pull request. But I agree that this change shouldn't be in the final code.

[eurofun]

I removed the dbg logging, added all SAIs, increased available 'runtime' heap blocks, modified / removed some comments.

In my first comment above I talked about the kernel having problems with two dai-links in one simple-audio-card: that disappeared once I gave both DAI_CONFIGs link_id 0.

[paulstelian97]

No more obvious issues I could find with your code. This still needs testing, both on NXP hardware and your own (I assume you have already done the second part).

[dbaluta]

@eurofun can you also share with us the kernel changes?

Will start CI with your changes tonight and get back with the results. So we appreciate your patience.

We definitely want to merge this changes but we want to make sure that we don't break existing usecases.

[dbaluta]

Not sure I understand this change. Is this a bugfix? We should add it in a separate patch with proper explanation.

[eurofun]

I added a separate commit. Or did you mean put it into a separate pull request?

[dbaluta]

@eurofun I run our CI for this PR and looks good. Can you please remove this out of draft.

[eurofun]

@dbaluta I removed it from draft, together with its kernel counterpart thesofproject/linux#4391 .

One thing I found is that the clocking is a bit weird: If aplay is started for example on hw:1,0 all clocks of hw:1,1 turn on and remain enabled until a firmware suspend or until sai_stop is called by starting and stopping aplay on hw:1,1. This is caused by 1754bf9 and I came up with a maybe somewhat better solution that involves a minor kernel change and an optional topology parameter clock_gated (backwards compatible). Should I put that in here, in another draft pull request later on or in an issue?

I did watch out for input/output errors by running this script (inspired by #3809) for half an hour, no issues.

#!/bin/bash

test_aplay()
{
    while true
    do
        cnt=$(( cnt + 1 ))
        echo -n "aplay : $cnt "
        time=$(( ( RANDOM % 10 )  + 1 ))
        aplay -Dhw:1,0 -f S16_LE -c 8 -r 48000 -d $time /dev/random
        time=$(( ( RANDOM % 10 )  + 1 ))
        sleep $time
    done
}

test_aplay1()
{
    while true
    do
        cnt=$(( cnt + 1 ))
        echo -n "aplay1: $cnt "
        time=$(( ( RANDOM % 10 )  + 1 ))
        aplay -Dhw:1,1 -f S16_LE -c 8 -r 48000 -d $time /dev/random
        time=$(( ( RANDOM % 10 )  + 1 ))
        sleep $time
    done
}

test_arecord()
{
    while true
    do
        cnt=$(( cnt + 1 ))
        echo -n "arec  : $cnt "
        time=$(( ( RANDOM % 10 )  + 1 ))
        arecord  -Dhw:1,0 -f S16_LE -c 8 -r 48000 -d $time /dev/null
        time=$(( ( RANDOM % 10 )  + 1 ))
        sleep $time
    done
}

test_arecord1()
{
    while true
    do
        cnt=$(( cnt + 1 ))
        echo -n "arec1 : $cnt "
        time=$(( ( RANDOM % 10 )  + 1 ))
        arecord  -Dhw:1,1 -f S16_LE -c 8 -r 48000 -d $time /dev/null
        time=$(( ( RANDOM % 10 )  + 1 ))
        sleep $time
    done
}

test_aplay &
test_aplay1 &
test_arecord &
test_arecord1

[lgirdwood]

@dbaluta @paulstelian97 ping ?

[dbaluta]

@LaurentiuM1234 can you have a look at this? I think we can merge this into main and have it for stable-v2.7 release.

[eurofun]

No worries @dbaluta, I'm patient (or at least trying to be).

I'm now using RI-2020.4-linux + hifi4_mscale_v1_0_2_prod_linux.tgz as my xtensa toolchain (latest version with xt-xcc, from there on only xt-clang):
this is why I removed the two for loop inline int declarations as they lead to a xt-xcc warning. And I removed the superfluous initialisation as suggested by lyakh). I adapted the HEAP_RUNTIME_SIZE to match what is in the nxp v2.6 stable branch as that seems ok as well. And last but not least another force push to get rid of a submodule change that I committed accidentally...

[LaurentiuM1234]

LGTM! Thanks for your contribution and sorry for the long wait :(. @dbaluta IMO this can be merged since it has been already passed through our internal CI.

[dbaluta]

@eurofun can you please rebase and push, it seems that we have some unrelated compilation issues.

[dbaluta]

@lgirdwood @dabekjakub I see some compilation errors related to virtual heaps. Can you please check?

[lgirdwood]

@lgirdwood @dabekjakub I see some compilation errors related to virtual heaps. Can you please check?

Yeah, sorry that was fixed late last night. It's on HEAD so not sure why the reabse did not get it here ?

[eurofun]

@dbaluta The checks seemingly were not re-run after my rebase onto main and the push?

[dbaluta]

SOFCI TEST

[lgirdwood]

@eurofun did you rebase on top of HEAD from today ? I can confirm I did this on another PR today and the force push fixed the GH action build failures releted to VMH.

[eurofun]

@lgirdwood Yes, last commit is 658cb65. I know nothing about this CI but it seems to me yesterdays run did not completely finish yet (I'm seeing e.g. "Internal Intel CI System/merge/build Expected — Waiting for status to be reported") so no new run can start? On the other hand I'm sure there is some timeout...

[dbaluta]

SOFCI TEST

[dbaluta]

@lgirdwood can you use your super powers and forcefully merge this.

[lgirdwood]

@lgirdwood can you use your super powers and forcefully merge this.

Something funny going on here, GH is showing all CI tests are running slowly. I will give it until lunch time and merge if its only vmh errors like last time.

[lgirdwood]

@lgirdwood can you use your super powers and forcefully merge this.

Something funny going on here, GH is showing all CI tests are running slowly. I will give it until lunch time and merge if its only vmh errors like last time.

Looks like normal service now resumed and all original failed tests passing. I'll wait a few more hours so we can get a all results in, not expecting any more issues.

[lgirdwood]

ok, jenkins/intel CI passed yesterday and rebase with VMH is passing GH actions. We can merge since Intel CI is still pending and does not test NXP code.

[dbaluta]

Thanks a lot @eurofun for the PR and for your patience.

[eurofun]

You're welcome @dbaluta! I'm seeing some problems (only) on Zephyr (crash if I start more than one aplay/arecord). Not sure if some other modifications I made are to blame or if it is a general problem. I will try a 'stock' setup next week.