Skip to content

[BUG][WHL][SSP] Channel swapped with trigger pause/stop/start on capture pipline.  #3525

Closed
Closed
[BUG][WHL][SSP] Channel swapped with trigger pause/stop/start on capture pipline. #3525
Assignees
keyonjiebkokoszx
Labels
I2SApplies to I2S bus for codec connectionP1Blocker bugs or important featuresWHLApplies to WhiskeyLake platformbugSomething isn't working as expected

Description

@jdrzaszcz
jdrzaszcz
opened on Oct 15, 2020

Describe the bug
When running simultaneous playback and capture on external loopback (same SSP) with pause, stop and start operation are done on capture stream there are swapped channel after trigger start on captured wave file.

This commit: 242be34 fixed swapped channels with pause, stop and start operation on capture stream with dsm component on TGL platform, but problem is still occurs specially on Frame formats where:
SAMPLE_SIZE=24 bits
CONTAINER_SIZE = 32 bits.

WHL results:
image

Example wave:
image

Topology

                pipe_plb
                +--------------------+
+----------+    | +------+   +-----+ |   +----+
| Host plb |----->|Buffer|-->| Dai |---->|SSPx|
+----------+    | +------+   +-----+ |   +----+
                +--------------------+    |  |
                pipe_cap                  |  |
                +--------------------+    |  |
+----------+    | +------+   +-----+ |   +----+
| Host cap |<-----|Buffer|<--| Dai |---->|SSPx|
+----------+    | +------+   +-----+ |   +----+
                +--------------------+

To Reproduce
Slim Driver python test:
test_20_ptp_cap_stream_stop_start_operations.py with --playback_iterations>=10

Reproduction Rate
On with Frame format 24/32 - 80%
On other Frame formats - 20%

Expected behavior
Channels shouldn't be swapped on recorded files.

Impact
What impact does this issue have on your progress (e.g., annoyance, showstopper)

Environment
I checked it on WHL platform.
SOF master, commit: 8a5059a

Example logs
For WHL:
02_20_TestPtpCaptureStreamStopStartOperation48000Hz32b32b4ch.zip

Activity

  1. jdrzaszcz
    added
    bugSomething isn't working as expected
    on Oct 15, 2020

  2. bkokoszx commented on Oct 15, 2020

    @bkokoszx
    bkokoszx
    on Oct 15, 2020
    Collaborator

    @lgirdwood @keyonjie
    It looks like we still have some problem with ssp_empty_rx_fifo() function.
    With following logs:

    diff --git a/src/drivers/intel/ssp/ssp.c b/src/drivers/intel/ssp/ssp.c
index cf103da73..168ec5c0a 100644
--- a/src/drivers/intel/ssp/ssp.c
+++ b/src/drivers/intel/ssp/ssp.c
@@ -86,6 +86,11 @@ static void ssp_empty_rx_fifo(struct dai *dai)
        } while ((ssp_read(dai, SSSR) & SSSR_BSY) &&
                 platform_timer_get(timer) < deadline);

+       uint32_t entries = SSCR3_RFL_VAL(ssp_read(dai, SSCR3));
+       dai_info(dai, "ssp_empty_rx_fifo(), entries %d", entries);
+       dai_info(dai, "ssp_empty_rx_fifo(), SSSR_RNE %d", (ssp_read(dai, SSSR) & SSSR_RNE) >> 3);
+       dai_info(dai, "ssp_empty_rx_fifo(), SSSR_BSY %d", (ssp_read(dai, SSSR) & SSSR_BSY) >> 4);
+
        /* clear interrupt */
        ssp_update_bits(dai, SSSR, SSSR_ROR, SSSR_ROR);
 }

    I've got:

    [  9691533.567708] (       93.723961) c0 ssp-dai      1.0   /drivers/intel/ssp/ssp.c:90   ssp_empty_rx_fifo(), entries 1
[  9691537.656250] (        4.088542) c0 ssp-dai      1.0   /drivers/intel/ssp/ssp.c:91   ssp_empty_rx_fifo(), SSSR_RNE 1
[  9691541.770833] (        4.114583) c0 ssp-dai      1.0   /drivers/intel/ssp/ssp.c:92   ssp_empty_rx_fifo(), SSSR_BSY 1

    It looks like there is still some entries in rx fifo. I've tried also to increase deadline, but it does not help. I've started to wonder whether we use BSY bit correctly here (BSY means that SSP is transmitting or receiving - in above case we still have playback working - may BSY == 1 refer to playback?).

    I've tried also just to remove ssp_empty_rx_fifo() function:

    diff --git a/src/drivers/intel/ssp/ssp.c b/src/drivers/intel/ssp/ssp.c
index cf103da73..f8e576a5f 100644
--- a/src/drivers/intel/ssp/ssp.c
+++ b/src/drivers/intel/ssp/ssp.c
@@ -735,7 +735,7 @@ static void ssp_stop(struct dai *dai, int direction)
            ssp->state[SOF_IPC_STREAM_CAPTURE] != COMP_STATE_PREPARE) {
                ssp_update_bits(dai, SSCR1, SSCR1_RSRE, 0);
                ssp_update_bits(dai, SSRSA, SSRSA_RXEN, 0);
-               ssp_empty_rx_fifo(dai);
+               // ssp_empty_rx_fifo(dai);
                ssp->state[SOF_IPC_STREAM_CAPTURE] = COMP_STATE_PREPARE;
                dai_info(dai, "ssp_stop(), RX stop");
        }

    With above difference I've got 10/10 passes on format: 02_20_TestPtpCaptureStreamStopStartOperation48000Hz24b32b8ch.

  3. lgirdwood commented on Oct 15, 2020

    @lgirdwood
    lgirdwood
    on Oct 15, 2020
    Member

    @jdrzaszcz can you extend RX fifo empty timeout ? It sounds like a stale sample....

  4. keyonjie commented on Oct 16, 2020

    @keyonjie
    keyonjie
    on Oct 16, 2020
    Contributor

    Thanks for reporting this @jdrzaszcz . Looks we need more experiment here, @bkokoszx found good point, I will find some time to work on this, @singalsu reported similar one on BYT #3520, so we need mimic fixes for BYT and maybe BDW also.

  5. jdrzaszcz commented on Oct 16, 2020

    @jdrzaszcz
    jdrzaszcz
    on Oct 16, 2020
    Author

    @lgirdwood I tried to increase SSP_RX_FLUSH_TIMEOUT to 1000 and it dose not help.
    @bkokoszx also tried to increase this value and when I talked with him he confirmed that it did not help to fix this issue.

  6. keyonjie commented on Oct 16, 2020

    @keyonjie
    keyonjie
    on Oct 16, 2020
    Contributor

    @jdrzaszcz @bkokoszx can you confirm that you are using SSP external Tx -> Rx connection, not the SSC1.LBM for nocodec tplg?

    These are facts:

    1. if playback is active while capture is stopped, SSSR_BSY = 1.
    2. SSSR_RNE is consistent with SSCR3.RFL, they are either both 0, or both non-0.

    I observe that with SSC1.LBM set, although I stop the Rx with clearing SSSR_BSY and SSRSA_RXEN (via stopping arecord), the SSSR_RNE and SSCR3.RFL keeping changing while the playback is running. Wondering if this is observed with non-SSC1.LBM mode. @jdrzaszcz can you try it with this change and catch FW log:

    diff --git a/src/drivers/intel/ssp/ssp.c b/src/drivers/intel/ssp/ssp.c
index cf103da73..b574ce1a9 100644
--- a/src/drivers/intel/ssp/ssp.c
+++ b/src/drivers/intel/ssp/ssp.c
@@ -72,6 +72,11 @@ static void ssp_empty_rx_fifo(struct dai *dai)
                clock_ms_to_ticks(PLATFORM_DEFAULT_CLOCK, 1) *
                SSP_RX_FLUSH_TIMEOUT / 1000;
 
+       uint32_t entries = SSCR3_RFL_VAL(ssp_read(dai, SSCR3));
+       dai_info(dai, "ssp_empty_rx_fifo(), entries %d", entries);
+       dai_info(dai, "ssp_empty_rx_fifo(), deadline: %d", (uint32_t)deadline);
+       dai_info(dai, "ssp_empty_rx_fifo(), SSCR2_RSAFD %d", (ssp_read(dai, SSCR2) & BIT(9)) >> 9);
+
        /*
         * To make sure all the RX FIFO entries are read out for the flushing,
         * we need to wait until the SSSR_RNE is cleared after the SSP link is
@@ -80,12 +85,23 @@ static void ssp_empty_rx_fifo(struct dai *dai)
         * run with the SSP RX and lead to samples mismatched issue.
         */
        do {
+               dai_info(dai, "ssp_empty_rx_fifo(), time %d", (uint32_t)platform_timer_get(timer));
+               entries = SSCR3_RFL_VAL(ssp_read(dai, SSCR3));
+               dai_info(dai, "ssp_empty_rx_fifo(), entries %d", entries);
+               dai_info(dai, "ssp_empty_rx_fifo(), SSSR_RNE %d", (ssp_read(dai, SSSR) & SSSR_RNE) >> 3);
+               dai_info(dai, "ssp_empty_rx_fifo(), SSSR_BSY %d", (ssp_read(dai, SSSR) & SSSR_BSY) >> 4);
+
                while (ssp_read(dai, SSSR) & SSSR_RNE)
                        /* read to empty the fifo */
                        ssp_read(dai, SSDR);
        } while ((ssp_read(dai, SSSR) & SSSR_BSY) &&
                 platform_timer_get(timer) < deadline);
 
+       entries = SSCR3_RFL_VAL(ssp_read(dai, SSCR3));
+       dai_info(dai, "ssp_empty_rx_fifo(), entries %d", entries);
+       dai_info(dai, "ssp_empty_rx_fifo(), SSSR_RNE %d", (ssp_read(dai, SSSR) & SSSR_RNE) >> 3);
+       dai_info(dai, "ssp_empty_rx_fifo(), SSSR_BSY %d", (ssp_read(dai, SSSR) & SSSR_BSY) >> 4);
+
        /* clear interrupt */
        ssp_update_bits(dai, SSSR, SSSR_ROR, SSSR_ROR);
 }

    I also did experiment to clear and reset Rx slot for capture stop/start iterate, but it doesn't help for LBM, not sure it help for your case @jdrzaszcz :

    @@ -710,6 +726,10 @@ static void ssp_start(struct dai *dai, int direction)
                ssp_update_bits(dai, SSCR1, SSCR1_TSRE, SSCR1_TSRE);
                ssp_update_bits(dai, SSTSA, SSTSA_TXEN, SSTSA_TXEN);
        } else {
+               /* mode, re-enable RX slots to resume the FIFO shifter */
+                ssp_update_bits(dai, SSRSA, SSRSA_SSRSA(0xff),
+                               SSRSA_SSRSA(ssp->config.ssp.rx_slots));
                ssp_update_bits(dai, SSCR1, SSCR1_RSRE, SSCR1_RSRE);
                ssp_update_bits(dai, SSRSA, SSRSA_RXEN, SSRSA_RXEN);
        }
@@ -735,6 +755,11 @@ static void ssp_stop(struct dai *dai, int direction)
            ssp->state[SOF_IPC_STREAM_CAPTURE] != COMP_STATE_PREPARE) {
                ssp_update_bits(dai, SSCR1, SSCR1_RSRE, 0);
                ssp_update_bits(dai, SSRSA, SSRSA_RXEN, 0);
+
+               /* disable RX slots to stop the FIFO shifter */
+               ssp_update_bits(dai, SSRSA, SSRSA_SSRSA(0xff), 0);
+
                ssp_empty_rx_fifo(dai);

  7. jdrzaszcz commented on Oct 16, 2020

    @jdrzaszcz
    jdrzaszcz
    on Oct 16, 2020
    Author

    @keyonjie I checked 2 audio formats on TGL with your second patch on internal loopback mode and the channels weren't swapped. I have to do more tests and let you know later.
    Unfortunately, I couldn't find platform with working external ssp loopback. When I will have such a platform I let you know about tests results.

  8. jdrzaszcz commented on Oct 19, 2020

    @jdrzaszcz
    jdrzaszcz
    on Oct 19, 2020
    Author

    @keyonjie I checked trigger pause/stop/start on capture pipeline (internal loopback mode and --playback-iteration=10) with clearing and resetting Rx slot for capture stop/start iterate on WHL and TGL and channels wasn't swapped channels.
    On external loopback mode I performed test on WHL platform and the issue is reproducible, but reproduction is smaller than on internal loopback mode.
    Logs for external loopback with @keyonjie additional info logs:
    dma_trace_1.log

  9. keyonjie commented on Oct 19, 2020

    @keyonjie
    keyonjie
    on Oct 19, 2020
    Contributor

    @keyonjie I checked trigger pause/stop/start on capture pipeline (--playback-iteration=10) with clearing and resetting Rx slot for capture stop/start iterate on WHL and TGL and channels wasn't swapped channels.
    On external loopback mode I performed test on WHL platform and the issue is reproducible, but reproduction is smaller than on internal loopback mode.
    Logs for external loopback with @keyonjie additional info logs:
    dma_trace_1.log

    Thanks for the experiments, @jdrzaszcz can you help to try if clearing/resetting Rx slot on your WHL external loopback mode also?

  10. keyonjie commented on Oct 19, 2020

    @keyonjie
    keyonjie
    on Oct 19, 2020
    Contributor

    @jdrzaszcz from the log you attached for external loopback, there should be no reproduction as the SSSR_RNE is always 0 after the first round of ssp_read().

  11. jdrzaszcz commented on Oct 19, 2020

    @jdrzaszcz
    jdrzaszcz
    on Oct 19, 2020
    Author

    @jdrzaszcz from the log you attached for external loopback, there should be no reproduction as the SSSR_RNE is always 0 after the first round of ssp_read().

    Data from the test, which FW logs I attached:
    2020_10_19_08_20_42.zip

    In file TestPtpCaptureStreamStopStartOperation16000Hz24b32b2ch.test[iter0]_2020_10_19_08_20_43_tplg0_iter1.wavcon.wav you can see that after ~ 3s that channels are swapped.
    image

  12. bkokoszx commented on Oct 19, 2020

    @bkokoszx
    bkokoszx
    on Oct 19, 2020
    Collaborator

    I also did experiment to clear and reset Rx slot for capture stop/start iterate, but it doesn't help for LBM, not sure it help for your case @jdrzaszcz :

    @Keyon, do you mean that on internal loopback you still observe channel swapping or SSSR_RNE and SSCR3.RFL are still keeping changing?
    Because clearing SSRSA_SSRSA also helps me in internal case.

  13. keyonjie commented on Oct 19, 2020

    @keyonjie
    keyonjie
    on Oct 19, 2020
    Contributor

    @bkokoszx @jdrzaszcz my experiment with change that doing the flush at ssp_start() and remove the ssp_stop() one fixes all issues observed, @lgirdwood what do you think about this?

  14. keyonjie commented on Oct 19, 2020

    @keyonjie
    keyonjie
    on Oct 19, 2020
    Contributor

    I also did experiment to clear and reset Rx slot for capture stop/start iterate, but it doesn't help for LBM, not sure it help for your case @jdrzaszcz :

    @Keyon, do you mean that on internal loopback you still observe channel swapping or SSSR_RNE and SSCR3.RFL are still keeping changing?
    Because clearing SSRSA_SSRSA also helps me in internal case.

    Yes, if I run more stress test on it.

  15. jdrzaszcz commented on Oct 19, 2020

    @jdrzaszcz
    jdrzaszcz
    on Oct 19, 2020
    Author

    @keyonjie On external loopback mode (test: for trigger pause/stop/start on capture pipeline, --playback-iteration=10) there were no swapped channels with your patch to clear and reset Rx slot.

  16. 8 remaining items

  17. plbossart commented on Oct 20, 2020

    @plbossart
    plbossart
    on Oct 20, 2020
    Member

    @lgirdwood I dropped this solution and implemented a new one, which basically disables SSP port when starting another direction transmission with one direction is already run, please help to review.

    That doesn't seem right? If you disable the SSP before starting playback while capture is already running, surely the capture is impacted?

    We don't want to solve a problem by creating another one, do we?

  18. jdrzaszcz commented on Oct 21, 2020

    @jdrzaszcz
    jdrzaszcz
    on Oct 21, 2020
    Author

    @keyonjie @lgirdwood On external loopback, channel are also swapped. I verified it on WHL RVP with last SOF master commit: 7270988.
    Examples:
    image

    image

  19. bkokoszx
    self-assigned this
    on Oct 27, 2020
  20. mengdonglin
    added
    I2SApplies to I2S bus for codec connection
    TGLApplies to Tiger Lake
    WHLApplies to WhiskeyLake platform
    on Nov 3, 2020

  21. keyonjie commented on Nov 6, 2020

    @keyonjie
    keyonjie
    on Nov 6, 2020
    Contributor

    Hi @jdrzaszcz @bkokoszx as you may know, on the TGP platforms we have extra issues need to be fixed, can you help to split the issues?

  22. jdrzaszcz
    removed
    TGLApplies to Tiger Lake
    on Nov 6, 2020
  23. keyonjie
    changed the title [-][BUG][SSP] Channel swapped with trigger pause/stop/start on capture pipline. [/-] [+][BUG][WHL][SSP] Channel swapped with trigger pause/stop/start on capture pipline. [/+] on Nov 9, 2020
  24. keyonjie
    removed this from the v1.6 milestone on Nov 10, 2020
  25. keyonjie
    added a commit that references this issue on Nov 12, 2020

    ssp: refine the RX FIFO flushing logic

    ec405b1
  26. lgirdwood
    closed this as completedin 60dfaa7on Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

I2SApplies to I2S bus for codec connectionP1Blocker bugs or important featuresWHLApplies to WhiskeyLake platformbugSomething isn't working as expected

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions