[-,dmix,v2,1/1] pcm: dmix: Align slave_hw_ptr to slave period boundary

Commit Message

Timo Wischer Oct. 30, 2018, 10:39 a.m. UTC

From: Laxmi Devi <[e-mail]>

These changes are required due to the kernel commit 07b7acb51d283d8469696c906b91f1882696a4d4
("ASoC: rsnd: update pointer more accurate")

Issue is that snd_pcm_wait() goes back to waiting because the hw_ptr
is not period aligned. Therefore snd_pcm_wait() will block for a longer
time as required.

With these rcar driver changes the exact position of the dma is returned.
During snd_pcm_start they read hw_ptr as reference, and this hw_ptr
is now not period aligned, and is a little ahead over the period while it
is read. Therefore when the avail is calculated during snd_pcm_wait(),
it is missing the avail_min by a few frames.
Consider the below example:

Considering the application is trying to write 0x120 frames and the
period_size = 0x60, avail_min = 0x120 and buffersize = 0x360 :

rsnd_pointer=0x12c -> dma pointer at the end of second write during
snd_pcm_dmix_start().
Since another 0x120 buffer is available, application writes 0x120 and goes
to snd_pcm_wait().
It is woken up after 3 snd_pcm_period_elapsed() to see rsnd_pointer=0x248.
So hw_ptr = new_slave_hw_ptr - reference_slave_hw_ptr = 0x248 - 0x12c = 0x11c.
It needs 4 more frames to be able to write. And so it goes back to waiting.

But since 3 snd_pcm_period_elapsed(), 3 periods should be available and it
should have been able to write.
If rsnd_pointer during the start was 0x120 which is 3 periods
then 0x248 - 0x120 =  128 it could go on with write.

Signed-off-by: Laxmi Devi <[e-mail]>
---

> On 10/29/18 16:54, Takashi Iwai wrote:
> The problem is that aligning the start essentially imposes an
> artificial long latency, and changes the behavior out of sudden.

Now, we are only align the salve_hw_ptr which also solves our delay issue.
But this change should not increase the latency because the write position
is given by slave_appl_ptr.
Do you see any other drawbacks?

Best regards

Timo

Comments

Takashi Iwai Oct. 30, 2018, 10:49 a.m. UTC | #1

On Tue, 30 Oct 2018 11:39:38 +0100,
<[e-mail]> wrote:
> 
> From: Laxmi Devi <[e-mail]>
> 
> These changes are required due to the kernel commit 07b7acb51d283d8469696c906b91f1882696a4d4
> ("ASoC: rsnd: update pointer more accurate")
> 
> Issue is that snd_pcm_wait() goes back to waiting because the hw_ptr
> is not period aligned. Therefore snd_pcm_wait() will block for a longer
> time as required.
> 
> With these rcar driver changes the exact position of the dma is returned.
> During snd_pcm_start they read hw_ptr as reference, and this hw_ptr
> is now not period aligned, and is a little ahead over the period while it
> is read. Therefore when the avail is calculated during snd_pcm_wait(),
> it is missing the avail_min by a few frames.
> Consider the below example:
> 
> Considering the application is trying to write 0x120 frames and the
> period_size = 0x60, avail_min = 0x120 and buffersize = 0x360 :
> 
> rsnd_pointer=0x12c -> dma pointer at the end of second write during
> snd_pcm_dmix_start().
> Since another 0x120 buffer is available, application writes 0x120 and goes
> to snd_pcm_wait().
> It is woken up after 3 snd_pcm_period_elapsed() to see rsnd_pointer=0x248.
> So hw_ptr = new_slave_hw_ptr - reference_slave_hw_ptr = 0x248 - 0x12c = 0x11c.
> It needs 4 more frames to be able to write. And so it goes back to waiting.
> 
> But since 3 snd_pcm_period_elapsed(), 3 periods should be available and it
> should have been able to write.
> If rsnd_pointer during the start was 0x120 which is 3 periods
> then 0x248 - 0x120 =  128 it could go on with write.
> 
> Signed-off-by: Laxmi Devi <[e-mail]>
> ---
> 
> > On 10/29/18 16:54, Takashi Iwai wrote:
> > The problem is that aligning the start essentially imposes an
> > artificial long latency, and changes the behavior out of sudden.
> 
> Now, we are only align the salve_hw_ptr which also solves our delay issue.
> But this change should not increase the latency because the write position
> is given by slave_appl_ptr.
> Do you see any other drawbacks?

How does this work?  I thought hwptr is synced to the actual slave
value soon later in anyway?


thanks,

Takashi

> 
> Best regards
> 
> Timo
> 
> diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
> index a6a8f3a..eaa0b0f 100644
> --- a/src/pcm/pcm_dmix.c
> +++ b/src/pcm/pcm_dmix.c
> @@ -560,6 +560,8 @@ static int snd_pcm_dmix_hwsync(snd_pcm_t *pcm)
>  static void reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix)
>  {
>  	dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr;
> +	dmix->slave_hw_ptr = ((dmix->slave_hw_ptr / dmix->slave_period_size)
> +				* dmix->slave_period_size);
>  	if (pcm->buffer_size > pcm->period_size * 2)
>  		return;
>  	/* If we have too litte periods, better to align the start position
> -- 
> 2.7.4
>

Timo Wischer Oct. 30, 2018, 12:04 p.m. UTC | #2

Best regards
*Timo Wischer*
Engineering Software Base (ADITG/ESB)

Tel. [phone]
On 10/30/18 11:49, Takashi Iwai wrote:
> On Tue, 30 Oct 2018 11:39:38 +0100,
> <[e-mail]> wrote:
>> From: Laxmi Devi <[e-mail]>
>>
>> These changes are required due to the kernel commit 07b7acb51d283d8469696c906b91f1882696a4d4
>> ("ASoC: rsnd: update pointer more accurate")
>>
>> Issue is that snd_pcm_wait() goes back to waiting because the hw_ptr
>> is not period aligned. Therefore snd_pcm_wait() will block for a longer
>> time as required.
>>
>> With these rcar driver changes the exact position of the dma is returned.
>> During snd_pcm_start they read hw_ptr as reference, and this hw_ptr
>> is now not period aligned, and is a little ahead over the period while it
>> is read. Therefore when the avail is calculated during snd_pcm_wait(),
>> it is missing the avail_min by a few frames.
>> Consider the below example:
>>
>> Considering the application is trying to write 0x120 frames and the
>> period_size = 0x60, avail_min = 0x120 and buffersize = 0x360 :
>>
>> rsnd_pointer=0x12c -> dma pointer at the end of second write during
>> snd_pcm_dmix_start().
>> Since another 0x120 buffer is available, application writes 0x120 and goes
>> to snd_pcm_wait().
>> It is woken up after 3 snd_pcm_period_elapsed() to see rsnd_pointer=0x248.
>> So hw_ptr = new_slave_hw_ptr - reference_slave_hw_ptr = 0x248 - 0x12c = 0x11c.
>> It needs 4 more frames to be able to write. And so it goes back to waiting.
>>
>> But since 3 snd_pcm_period_elapsed(), 3 periods should be available and it
>> should have been able to write.
>> If rsnd_pointer during the start was 0x120 which is 3 periods
>> then 0x248 - 0x120 =  128 it could go on with write.
>>
>> Signed-off-by: Laxmi Devi <[e-mail]>
>> ---
>>
>>> On 10/29/18 16:54, Takashi Iwai wrote:
>>> The problem is that aligning the start essentially imposes an
>>> artificial long latency, and changes the behavior out of sudden.
>> Now, we are only align the salve_hw_ptr which also solves our delay issue.
>> But this change should not increase the latency because the write position
>> is given by slave_appl_ptr.
>> Do you see any other drawbacks?
> How does this work?  I thought hwptr is synced to the actual slave
> value soon later in anyway?

The slave_hw_ptr is initialized in reset_slave_ptr() and updated in 
snd_pcm_dmix_sync_ptr() with

dmix->slave_hw_ptr = *dmix->spcm->hw.ptr;

The hw_ptr will be incremented in snd_pcm_dmix_sync_ptr() by adding the 
diff between the last slave_hw_ptr and the current slave_hw_ptr. Due to 
the alignment of the slave_hw_ptr on startup (in reset_slave_ptr()) the 
offset  between the slave_hw_ptr and hw_ptr is a multiple of 
slave_period. The following slave_hw_ptr will be always x * period_size 
+ y where x is always incremented for each snd_pcm_period_elapsed() 
call. y can be sometimes 0 but also contain some frames due to the delay 
between the DMA interrupt till reading the current DMA position.

The diff between the salve_hw_ptr will be continuously added to hw_ptr. 
Sometimes the diff can be < period_size but anyway the hw_ptr follows 
the same interval as the salve_hw_ptr:

  * 1 * period_size + y1
  * 2 * period_size + y2
  * 3 * period_size + y3
  * ...

y can differ between the snd_pcm_dmix_sync_ptr() calls. But it is never 
<0. Therefore the application can always write one period if it writes 
all chunks in periods.

Best regards

Timo

>
>
> thanks,
>
> Takashi
>
>> Best regards
>>
>> Timo
>>
>> diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
>> index a6a8f3a..eaa0b0f 100644
>> --- a/src/pcm/pcm_dmix.c
>> +++ b/src/pcm/pcm_dmix.c
>> @@ -560,6 +560,8 @@ static int snd_pcm_dmix_hwsync(snd_pcm_t *pcm)
>>   static void reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix)
>>   {
>>   	dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr;
>> +	dmix->slave_hw_ptr = ((dmix->slave_hw_ptr / dmix->slave_period_size)
>> +				* dmix->slave_period_size);
>>   	if (pcm->buffer_size > pcm->period_size * 2)
>>   		return;
>>   	/* If we have too litte periods, better to align the start position
>> -- 
>> 2.7.4
>>

Takashi Iwai Oct. 30, 2018, 1:21 p.m. UTC | #3

On Tue, 30 Oct 2018 13:04:33 +0100,
Timo Wischer wrote:
> 
> Best regards
> Timo Wischer
> Engineering Software Base (ADITG/ESB)
> 
> Tel. [phone]
> On 10/30/18 11:49, Takashi Iwai wrote:
> 
>     On Tue, 30 Oct 2018 11:39:38 +0100,
>     <[e-mail]> wrote:
>     
>         From: Laxmi Devi <[e-mail]>
>         
>         These changes are required due to the kernel commit 07b7acb51d283d8469696c906b91f1882696a4d4
>         ("ASoC: rsnd: update pointer more accurate")
>         
>         Issue is that snd_pcm_wait() goes back to waiting because the hw_ptr
>         is not period aligned. Therefore snd_pcm_wait() will block for a longer
>         time as required.
>         
>         With these rcar driver changes the exact position of the dma is returned.
>         During snd_pcm_start they read hw_ptr as reference, and this hw_ptr
>         is now not period aligned, and is a little ahead over the period while it
>         is read. Therefore when the avail is calculated during snd_pcm_wait(),
>         it is missing the avail_min by a few frames.
>         Consider the below example:
>         
>         Considering the application is trying to write 0x120 frames and the
>         period_size = 0x60, avail_min = 0x120 and buffersize = 0x360 :
>         
>         rsnd_pointer=0x12c -> dma pointer at the end of second write during
>         snd_pcm_dmix_start().
>         Since another 0x120 buffer is available, application writes 0x120 and goes
>         to snd_pcm_wait().
>         It is woken up after 3 snd_pcm_period_elapsed() to see rsnd_pointer=0x248.
>         So hw_ptr = new_slave_hw_ptr - reference_slave_hw_ptr = 0x248 - 0x12c = 0x11c.
>         It needs 4 more frames to be able to write. And so it goes back to waiting.
>         
>         But since 3 snd_pcm_period_elapsed(), 3 periods should be available and it
>         should have been able to write.
>         If rsnd_pointer during the start was 0x120 which is 3 periods
>         then 0x248 - 0x120 =  128 it could go on with write.
>         
>         Signed-off-by: Laxmi Devi <[e-mail]>
>         ---
> 
>             On 10/29/18 16:54, Takashi Iwai wrote:
>             The problem is that aligning the start essentially imposes an
>             artificial long latency, and changes the behavior out of sudden.
>             
>         Now, we are only align the salve_hw_ptr which also solves our delay issue.
>         But this change should not increase the latency because the write position
>         is given by slave_appl_ptr.
>         Do you see any other drawbacks?
>         
>     How does this work?  I thought hwptr is synced to the actual slave
>     value soon later in anyway?
>     
> The slave_hw_ptr is initialized in reset_slave_ptr() and updated in
> snd_pcm_dmix_sync_ptr() with
> 
> dmix->slave_hw_ptr = *dmix->spcm->hw.ptr;
> 
> The hw_ptr will be incremented in snd_pcm_dmix_sync_ptr() by adding the diff
> between the last slave_hw_ptr and the current slave_hw_ptr. Due to the
> alignment of the slave_hw_ptr on startup (in reset_slave_ptr()) the offset 
> between the slave_hw_ptr and hw_ptr is a multiple of slave_period.

Yes, and is this correct?  Suppose you start a stream at the position
one sample before the next period (psize * N + (psize-1)).  Then
slave_hw_ptr is psize * N.  At the next moment, the dpcm->hw.ptr
reaches to psize * (N+1), and snd_pcm_dmix_sync_ptr() gets called.
Then slave_hw_ptr will be updated to psize * (N+1) although only one
sample has been processed?


> The
> following slave_hw_ptr will be always x * period_size + y where x is always
> incremented for each snd_pcm_period_elapsed() call. y can be sometimes 0 but
> also contain some frames due to the delay between the DMA interrupt till
> reading the current DMA position.
> 
> The diff between the salve_hw_ptr will be continuously added to hw_ptr.
> Sometimes the diff can be < period_size but anyway the hw_ptr follows the same
> interval as the salve_hw_ptr:
> 
>   * 1 * period_size + y1
>   * 2 * period_size + y2
>   * 3 * period_size + y3
>   * ...
> 
> y can differ between the snd_pcm_dmix_sync_ptr() calls. But it is never <0.
> Therefore the application can always write one period if it writes all chunks
> in periods.

The update timing is right, but what about the first samples?


thanks,

Takashi

> 
> Best regards
> 
> Timo
> 
>     thanks,
>     
>     Takashi
> 
>         Best regards
>         
>         Timo
>         
>         diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
>         index a6a8f3a..eaa0b0f 100644
>         --- a/src/pcm/pcm_dmix.c
>         +++ b/src/pcm/pcm_dmix.c
>         @@ -560,6 +560,8 @@ static int snd_pcm_dmix_hwsync(snd_pcm_t *pcm)
>          static void reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix)
>          {
>               dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr;
>         +     dmix->slave_hw_ptr = ((dmix->slave_hw_ptr / dmix->slave_period_size)
>         +                                                                       * dmix->slave_period_size);
>               if (pcm->buffer_size > pcm->period_size * 2)
>                                     return;
>               /* If we have too litte periods, better to align the start position
>         -- 
>         2.7.4
> 
>

Patch

diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c
index a6a8f3a..eaa0b0f 100644
--- a/src/pcm/pcm_dmix.c
+++ b/src/pcm/pcm_dmix.c
@@ -560,6 +560,8 @@  static int snd_pcm_dmix_hwsync(snd_pcm_t *pcm)
 static void reset_slave_ptr(snd_pcm_t *pcm, snd_pcm_direct_t *dmix)
 {
 	dmix->slave_appl_ptr = dmix->slave_hw_ptr = *dmix->spcm->hw.ptr;
+	dmix->slave_hw_ptr = ((dmix->slave_hw_ptr / dmix->slave_period_size)
+				* dmix->slave_period_size);
 	if (pcm->buffer_size > pcm->period_size * 2)
 		return;
 	/* If we have too litte periods, better to align the start position