diff mbox

[0/5] ASoC: fsl_ssi: Fixing various channel slips and bad samples insertions

Message ID 56966324.8090409@invoxia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Arnaud Mouiche Jan. 13, 2016, 2:45 p.m. UTC
Le 12/01/2016 00:44, Caleb Crome a écrit :
> On Sat, Jan 9, 2016 at 3:02 AM, arnaud.mouiche@invoxia.com
> <arnaud.mouiche@invoxia.com> wrote:
>> Hello Caleb
>>
>> Le 09/01/2016 01:47, Caleb Crome a écrit :
>>> [...]
>>>
>>> Hello Arnaud,
>>>     I have finally gotten to test your patches, and I'm still having
>>> trouble with channel slips.
>>>
>>> I applied your v2 patch set, along with your changes for using a dummy
>>> codec.
>>>
>>> The full changes are here:
>>> https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
>>>
>>> This ignores most of my previous patches, and uses your code to bring
>>> up the SSI (without a codec) on a wandboard.
>>>
>>> I am using SSI3, and doing a hardware loopback between TX and RX.
>>>
>>> Here's what I run:
>>> ./atest -r 16000 -c 8 -p 2048   -D default play
>>>
>>> which plays continuously.
>>>
>>> and in another shell:
>>> ./atest -r 16000 -c 8 -p 2048  -D default -d 10 capture
>>>
>>> which captures for 10 seconds.
>>>
>>>
>>> The first time I run the capture command, it succeeds, no problem.
>>>> dbg: dev: 'default'
>>>> dbg: default: capture_start
>>>> dbg: start a 10 seconds duration timer
>>>> warn: First valid frame
>>>> warn:   3400 3401 3402 3403 3404 3405 3406 3407
>>>> dbg: end of tests
>>>> total number of sequence errors: 0
>>>> global tests exit status: OK
>>> But the second and all subsequent captures, it fails with channel slips:
>>>
>>>> dbg: dev: 'default'
>>>> dbg: default: capture_start
>>>> dbg: start a 10 seconds duration timer
>>>> err: invalid frame after 0 null frames
>>>> err:   d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0
>>>> err:   d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0
>>>> dbg: end of tests
>>>> total number of sequence errors: 430080
>>>> global tests exit status: OK
>>
>> Can you use -I option to get a little more log of the error
>> $ ./atest -r 16000 -c 8 -p 2048  -D default -d 10 -I 10 capture
>>
>> Just to know if the wrong frames comes from the previous record session,
>> meaning the RX fifo was not cleared correctly,
>> as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect.
>> (ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may be
>> the start of the new recording session)
> That does appear to be what's happening.  I read the SFCSR before and
> after you update the SOR register (SOR_RX_CLR), and in both cases the
> rx buffer is full on the second enablement.
>
> I can't seem to empty that buffer, either by using the SOR_RX_CLR or
> by reading the SRX0 register. This is another one of those cases where
> there you cannot modify the register (i.e. fifo or RX0) when RE is
> disabled.
>
> So, instead of clearing on enable, I'm clearing on shutdown and on
> enable.  I attempted using the SOR_RX_CLR, but it doesn't seem to work
> on the MX6.  Rather, I clear the fifo by reading all the words in a
> loop, just before RE is disabled.
so the imx6.sl SSI behaves differently from the imx6.solo SSI ... Or 
some things have changes between 4.3 and 4.4.

May be SOR_RX_CLR and/or RX0 registers can't be read on 'solo' because 
de the SRCR.RFEN0 [or1] are not enabled yet.

In fact, clearing the fifos at the end may still introduce a race since 
the RX path is still enabled when your read from fifo, and
Samples may still be received in the meantime...

Also, I see that the CCSR_SSI_SOR register is not in the regmap 
fsl_ssi_volatile_reg() list (line 140). May be there is an implication 
of some caching mechanism ?

Can you try to move the the fifo clearing part just before the write in 
CCSR_SSI_SIER with the following change in top of my patches ?
Also may be you can additionally add the RX0 / RX1 reading from clearing 
mechanism at the same place.

On my side, I will check the 4.4 tree on imx6sl

Regards,
Arnaud

                 /*
                  * Clear RX or TX FIFO to remove samples from the previous
                  * stream session which may be still present in the 
FIFO and
@@ -435,9 +438,6 @@ static void fsl_ssi_config(struct fsl_ssi_private 
*ssi_private, int enable,
                         regmap_update_bits(regs, CCSR_SSI_SOR,
                                 CCSR_SSI_SOR_TX_CLR, CCSR_SSI_SOR_TX_CLR);

                 }
-
-               regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, 
vals->srcr);
-               regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, 
vals->stcr);
                 regmap_update_bits(regs, CCSR_SSI_SIER, vals->sier, 
vals->sier);
         } else {
                 u32 sier;

Comments

Caleb Crome Jan. 13, 2016, 8:20 p.m. UTC | #1
On Wed, Jan 13, 2016 at 6:45 AM, arnaud.mouiche@invoxia.com
<arnaud.mouiche@invoxia.com> wrote:
>
>
> Le 12/01/2016 00:44, Caleb Crome a écrit :
>>
>> On Sat, Jan 9, 2016 at 3:02 AM, arnaud.mouiche@invoxia.com
>> <arnaud.mouiche@invoxia.com> wrote:
>>>
>>> Hello Caleb
>>>
>>> Le 09/01/2016 01:47, Caleb Crome a écrit :
>>>>
>>>> [...]
>>>>
>>>> Hello Arnaud,
>>>>     I have finally gotten to test your patches, and I'm still having
>>>> trouble with channel slips.
>>>>
>>>> I applied your v2 patch set, along with your changes for using a dummy
>>>> codec.
>>>>
>>>> The full changes are here:
>>>> https://github.com/ccrome/linux-caleb-dev/tree/v4.4-rc8-armv7-x3
>>>>
>>>> This ignores most of my previous patches, and uses your code to bring
>>>> up the SSI (without a codec) on a wandboard.
>>>>
>>>> I am using SSI3, and doing a hardware loopback between TX and RX.
>>>>
>>>> Here's what I run:
>>>> ./atest -r 16000 -c 8 -p 2048   -D default play
>>>>
>>>> which plays continuously.
>>>>
>>>> and in another shell:
>>>> ./atest -r 16000 -c 8 -p 2048  -D default -d 10 capture
>>>>
>>>> which captures for 10 seconds.
>>>>
>>>>
>>>> The first time I run the capture command, it succeeds, no problem.
>>>>>
>>>>> dbg: dev: 'default'
>>>>> dbg: default: capture_start
>>>>> dbg: start a 10 seconds duration timer
>>>>> warn: First valid frame
>>>>> warn:   3400 3401 3402 3403 3404 3405 3406 3407
>>>>> dbg: end of tests
>>>>> total number of sequence errors: 0
>>>>> global tests exit status: OK
>>>>
>>>> But the second and all subsequent captures, it fails with channel slips:
>>>>
>>>>> dbg: dev: 'default'
>>>>> dbg: default: capture_start
>>>>> dbg: start a 10 seconds duration timer
>>>>> err: invalid frame after 0 null frames
>>>>> err:   d2a1 d2a2 d2a3 d2a4 d2a5 d2a6 d2a7 d2c0
>>>>> err:   d2c1 d2c2 d2c3 d2c4 d2c5 d2c6 d2c7 78e0
>>>>> dbg: end of tests
>>>>> total number of sequence errors: 430080
>>>>> global tests exit status: OK
>>>
>>>
>>> Can you use -I option to get a little more log of the error
>>> $ ./atest -r 16000 -c 8 -p 2048  -D default -d 10 -I 10 capture
>>>
>>> Just to know if the wrong frames comes from the previous record session,
>>> meaning the RX fifo was not cleared correctly,
>>> as if the CCSR_SSI_SOR_RX_CLR bit is not working as we might expect.
>>> (ie. d2a1 .. d2c7 may come from the previous recording, and 78e0 .. may
>>> be
>>> the start of the new recording session)
>>
>> That does appear to be what's happening.  I read the SFCSR before and
>> after you update the SOR register (SOR_RX_CLR), and in both cases the
>> rx buffer is full on the second enablement.
>>
>> I can't seem to empty that buffer, either by using the SOR_RX_CLR or
>> by reading the SRX0 register. This is another one of those cases where
>> there you cannot modify the register (i.e. fifo or RX0) when RE is
>> disabled.
>>
>> So, instead of clearing on enable, I'm clearing on shutdown and on
>> enable.  I attempted using the SOR_RX_CLR, but it doesn't seem to work
>> on the MX6.  Rather, I clear the fifo by reading all the words in a
>> loop, just before RE is disabled.
>
> so the imx6.sl SSI behaves differently from the imx6.solo SSI ... Or some
> things have changes between 4.3 and 4.4.
>
> May be SOR_RX_CLR and/or RX0 registers can't be read on 'solo' because de
> the SRCR.RFEN0 [or1] are not enabled yet.
>
> In fact, clearing the fifos at the end may still introduce a race since the
> RX path is still enabled when your read from fifo, and
> Samples may still be received in the meantime...

You are correct about the race condition.  It seems like a catch 22:
you can't empty the fifo if SCR.RE is disabled, and if the SCR.RE is
enabled, new samples might be received at any time.  in fact, when
disabling RE, it does keep receiving until the end of the frame.

>
> Also, I see that the CCSR_SSI_SOR register is not in the regmap
> fsl_ssi_volatile_reg() list (line 140). May be there is an implication of
> some caching mechanism ?

Ah!  Yes.  When I add the SOR to the volatile list, then writing the
SOR does clear the FIFO.  That's one for the patch for sure :-)

Now at least RX get's cleared on RX start.

However, now I've got another issue:

FYI I'm running at 16 channels/16-bits/frame, so 256 bits/frame, with
the codec as master.  (codec 0 anyway).

I'm running atest play in one shell, and atest capture in another shell.

At low sample rates, it works quite reliably (a few hundred simulated
xruns with no problem), but TX slips at higher sample rates.

I'm running
    shell a # ./atest -r SAMPLERATE -c 16 -p 1024   -D default play
    shell b # ./atest -r SAMPLERATE -c 16 -p 1024   -D default -d 100
capture  -x200,100

8kHz:  No problems after 100 seconds (~300 capture xruns)
16kHz:  No problems
32kHz: tx slips after a few RX xruns (12) and never recovers.
48kHz:  tx starts properly (usually), but slips immediately upon RX start.

FYI, I'm watching TX on a scope, and can verify that it is in fact the
TX slipping.   I caught it once, and found that it was a duplicated
sample in the TX stream, which seems to say that the DMA is not
keeping up maybe?  Perhaps tweaking with watermarks will help that, or
perhaps dual-fifo will help.  Will try a couple things and get back to
you.

In the meantime, any idea how to check to see what speed the SSI
peripheral clock is running?  I just want to make sure it's going fast
enough.

here's a log of the failure at 32kHz:
---------------------------------------------------
<first verified on the scope that play is running without slips>
... 20 or 30 valid xruns...
ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred
warn: First valid frame
warn:   49a0 49a1 49a2 49a3 49a4 49a5 49a6 49a7 49a8 49a9 49aa 49ab
49ac 49ad 49ae 49af
warn: default: force capture xrun
warn: default: CT_W4_XRUN_END
warn: default: capture read failed: Broken pipe
ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred
warn: First valid frame
warn:   3a40 3a41 3a42 3a43 3a44 3a45 3a46 3a47 3a48 3a49 3a4a 3a4b
3a4c 3a4d 3a4e 3a4f
warn: default: force capture xrun
warn: default: CT_W4_XRUN_END
warn: default: capture read failed: Broken pipe
ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred
warn: First valid frame
warn:   3240 3241 3242 3243 3244 3245 3246 3247 3248 3249 324a 324b
324c 324d 324e 324f
warn: first invalid frame while expecting frame 0x0617
warn:   c2e0 c2e1 c2e2 c2e3 c2e4 c2e5 c2e6 c2e6 c2e7 c2e8 c2e9 c2ea
c2eb c2ec c2ed c2ee
warn:   c2ef c300 c301 c302 c303 c304 c305 c306 c307 c308 c309 c30a
c30b c30c c30d c30e
warn:   c30f c320 c321 c322 c323 c324 c325 c326 c327 c328 c329 c32a
c32b c32c c32d c32e
warn:   c32f c340 c341 c342 c343 c344 c345 c346 c347 c348 c349 c34a
c34b c34c c34d c34e
err:   c34f c360 c361 c362 c363 c364 c365 c366 c367 c368 c369 c36a
c36b c36c c36d c36e
err:   c36f c380 c381 c382 c383 c384 c385 c386 c387 c388 c389 c38a
c38b c38c c38d c38e
err:   c38f c3a0 c3a1 c3a2 c3a3 c3a4 c3a5 c3a6 c3a7 c3a8 c3a9 c3aa
c3ab c3ac c3ad c3ae
err:   c3af c3c0 c3c1 c3c2 c3c3 c3c4 c3c5 c3c6 c3c7 c3c8 c3c9 c3ca
c3cb c3cc c3cd c3ce
err:   c3cf c3e0 c3e1 c3e2 c3e3 c3e4 c3e5 c3e6 c3e7 c3e8 c3e9 c3ea
c3eb c3ec c3ed c3ee
err:   c3ef c400 c401 c402 c403 c404 c405 c406 c407 c408 c409 c40a
c40b c40c c40d c40e
err:   c40f c420 c421 c422 c423 c424 c425 c426 c427 c428 c429 c42a
c42b c42c c42d c42e
warn: default: force capture xrun
warn: default: CT_W4_XRUN_END

As you can see, this is caused by duplicated c2e6, which I guess is a
DMA underrun?

here's a log of the failure at 48kHz:
---------------------------------------------------
<first verified on the scope that play is running without slips>
caleb@arm:~/atest$ ./atest -r 48000 -c 16 -p 1024   -D default -d 100
capture  -x200,100
dbg: dev: 'default'
dbg: default: capture_start
dbg: default: will simulate xrun every 200 ms
dbg: start a 100 seconds duration timer
err: invalid frame after 0 null frames
err:   9b2f 9b40 9b41 9b42 9b43 9b44 9b45 9b46 9b47 9b48 9b49 9b4a
9b4b 9b4c 9b4d 9b4e
err:   9b4f 9b60 9b61 9b62 9b63 9b64 9b65 9b66 9b67 9b68 9b69 9b6a
9b6b 9b6c 9b6d 9b6e
warn: default: force capture xrun
warn: default: CT_W4_XRUN_END
warn: default: capture read failed: Broken pipe
ALSA lib pcm.c:7843:(snd_pcm_recover) overrun occurred
err: invalid frame after 0 null frames

Thanks,
-Caleb
diff mbox

Patch

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 0277097..bd163b2 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -420,6 +420,9 @@  static void fsl_ssi_config(struct fsl_ssi_private 
*ssi_private, int enable,
          * (online configuration)
          */
         if (enable) {
+
+               regmap_update_bits(regs, CCSR_SSI_SRCR, vals->srcr, 
vals->srcr);
+               regmap_update_bits(regs, CCSR_SSI_STCR, vals->stcr, 
vals->stcr);