[alsa-devel] [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.

[Channaiah Vanitha (RBEI/ECF3)]

Hello Takashi-san,

Can you please reply your feedback for below mail chain.

Best regards,
Vanitha Channaiah 
RBEI/ECF3

_____________________________________________
From: Channaiah Vanitha (RBEI/ECF3) 
Sent: Tuesday, June 18, 2019 4:44 AM
To: 'Takashi Iwai' <tiwai at suse.de>
Cc: alsa-devel at alsa-project.org; Wischer Timo (ADITG/ESS) <twischer at de.adit-jv.com>
Subject: RE: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.


Hello Takashi-san,

Firstly, very sorry for the late reply.

> The current condition was chosen because otherwise it'll cause 
> underrun errors.  If the round down is needed for avoiding errors, it 
> should be changed, yes.  Otherwise, it needs a careful evaluation.

> If buffer=2*period, the chance to slip the update is quite high unless you align the start.  And the instability with 2xperiod is the very reason we've added this hack at the beginning.

If this is the case, current condition would also solve under run errors for the condition buffer=2*period+1frame.
For, buffer=2*period+1frame, snd_pcm_wait() is waiting for more than a period and this could lead to xruns.
The chance to slip the condition is more in case of buffer=2*period-1frame. Because, after first write (one period write), avail is less than one period by 1 frame. It has to wait until next DMA interrupt. There is more chance of xruns.
In case of buffer=2*period, after first write (one period write) avail is still one period size and there is less chance of xruns.	
	
> In anyway, the description in the patch doesn't match with the change.  
> Please update it to fit with the actual change if we still need to 
> take this change inevitably.
	
For buffer>=2*period, round down of slave pointers and for buffer=2*period-1frame, round up of slave pointers will avoid xruns. 
Which otherwise causes snd_pcm_wait() to block for more than a period time and leads to xruns.

Best regards,
Vanitha Channaiah
RBEI/ECF3  

Tel. +91 80 6136-4436 



-----Original Message-----
From: Takashi Iwai <mailto:tiwai at suse.de> 
Sent: Friday, May 17, 2019 4:20 PM
To: Channaiah Vanitha (RBEI/ECF3) <mailto:Vanitha.Channaiah at in.bosch.com>
Cc: mailto:alsa-devel at alsa-project.org; Wischer Timo (ADITG/ESS) <mailto:twischer at de.adit-jv.com>
Subject: Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr pointer if buffer size is less than 2 period size.

On Thu, 16 May 2019 19:56:20 +0200,
Takashi Iwai wrote:
> 
> On Thu, 16 May 2019 19:40:35 +0200,
> Channaiah Vanitha (RBEI/ECF3) wrote:
> > 
> > Hello Takashi-san,
> >  
> > > It's still not clear to me why this change is made.
> > > The example mentioned in the above (period_size=96, 
> > > buffer_size=191) also
> > matches with the condition before the change, so there should be 
> > behavior change by the patch.
> > > IOW, your patch does nothing but modifying the condition to drop 
> > > the case
> > buffer_size == period_size * 2.  Why this condition can't
> > > (shouldn't) be a target of the round up?  That needs the 
> > > clarification in
> > the patch description.
> > 
> > In case of Buffer_size = 2 * period_size, round down of slave_hw_ptr 
> > was necessary which otherwise leads to Blocking of snd_pcm_wait() 
> > for longer time(i.e. more than 1n period)
> > 
> > An example of capture case is explained here:
> >  
> > Issue occurs in case of round up:      
> >  
> > - During the start, slave_hw_ptr = 128
> > - After round up: slave_app_ptr: 192 slave_hw_ptr: 128
> > - avail:0 app_ptr:0 hw_ptr:0
> > - snd_pcm_wait() locks
> > - new slave hw ptr updated to plugins: new_slave_hw_ptr = 192
> > - hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 128 = 64
> > - avail:64 app_ptr:0 hw_ptr:64
> > - snd_pcm_wait() still blocked ------------------à [issue occurs]
> > - new slave hw ptr updated to plugins: new_slave_hw_ptr = 288
> > - avail:160 app_ptr:0 hw_ptr:160(64+96)
> > - snd_pcm_wait() is released
> >  
> > In case of round down:
> >  
> > - During the start, slave_hw_ptr = 128
> > - After round up: slave_app_ptr:96 slave_hw_ptr:96
> > - avail:0 app_ptr:0 hw_ptr:0
> > - snd_pcm_wait() locks
> > - new slave hw ptr updated to plugins: new_slave_hw_ptr = 192
> > - hw_ptr = new_slave_hw_ptr - old_slave_hw_ptr = 192 - 96 = 96
> > - avail:96 app_ptr:0 hw_ptr:96
> > - snd_pcm_wait() is released------------------à [issue does not 
> > occurs]
> > - avail:160 app_ptr:0 hw_ptr:160(64+96)
> >  
> > Also, No other issue is introduced in case of playback scenario.
> 
> But the forced alignment has another drawback, namely it shifts the 
> streaming.  That is sometimes worse than the longer wakeup latency.
> You can't guess which behavior is preferred by user in the case of 
> "auto" policy.

Erm, scratch this, it makes further confusion, sorry.
But the below still applies:

> The current condition was chosen because otherwise it'll cause 
> underrun errors.  If the round down is needed for avoiding errors, it 
> should be changed, yes.  Otherwise, it needs a careful evaluation.

If buffer=2*period, the chance to slip the update is quite high unless you align the start.  And the instability with 2xperiod is the very reason we've added this hack at the beginning.


thanks,

Takashi

> In anyway, the description in the patch doesn't match with the change.  
> Please update it to fit with the actual change if we still need to 
> take this change inevitably.
> 
> 
> thanks,
> 
> Takashi
> 
> >  
> > Best regards,
> > Vanitha Channaiah
> > RBEI/ECF3
> >  
> > Tel. +91 80 6136-4436
> > 
> > -----Original Message-----
> > From: Takashi Iwai <mailto:tiwai at suse.de>
> > Sent: Wednesday, May 15, 2019 2:16 PM
> > To: Channaiah Vanitha (RBEI/ECF3) <mailto:Vanitha.Channaiah at in.bosch.com>
> > Cc: mailto:alsa-devel at alsa-project.org; Wischer Timo (ADITG/ESS) 
> > <mailto:twischer at de.adit-jv.com>
> > Subject: Re: [PATCH v2 4/6] pcm: direct: Round up of slave_app_ptr 
> > pointer if buffer size is less than 2 period size.
> >  
> > On Wed, 15 May 2019 08:26:35 +0200,
> > <mailto:vanitha.channaiah at in.bosch.com> wrote:
> > >
> > > From: Vanitha Channaiah <mailto:vanitha.channaiah at in.bosch.com>
> > >
> > > For buffer size less than two period size, the start position of 
> > > slave_app_ptr is rounded up in order to avoid xruns For e.g.:
> > > Considering below parameters and its values Period size = 96 
> > > Buffer size = 191 slave_appl_ptr = slave_hw_ptr = unaligned value
> > >
> > > Issue:
> > > - During the start of the stream, app_ptr = hw_ptr = 0
> > > - Application writes one period of data in the buffer i.e
> > >   app_ptr = 96, hw_ptr = 0
> > > - Now, the avail is just period-1 frames available.
> > >   avail = hw_ptr + buffer_size - app_ptr = 95
> > >   i.e. shortage of 1 frame space
> > > - so application is waiting for the 1frame space to be available.
> > > - slave_app_ptr and slave_hw_ptr would get updated to lower values
> > > - This could lead to under run to occur.
> > >
> > > Fix:
> > > If we round Up the slave_app_ptr pointer,
> > > - During the start of the stream, app_ptr = hw_ptr = 0
> > > - Application writes one period of data in the buffer i.e
> > >   app_ptr = 96, hw_ptr = 0
> > > - Round Up of slave_app_ptr pointer leads to below calculation:
> > > - slave_app_ptr rounded to 96
> > > - slave_app_ptr and slave_hw_ptr would get updated to larger value 
> > > nearing to 2 period size
> > > - avail = greater than period size.
> > > - Here, there is a lower chance of under run.
> > >
> > > Signed-off-by: Vanitha Channaiah <mailto:vanitha.channaiah at in.bosch.com>
> > > ---
> > >  src/pcm/pcm_direct.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index
> > > 54d9900..b56da85 100644
> > > --- a/src/pcm/pcm_direct.c
> > > +++ b/src/pcm/pcm_direct.c
> > > @@ -2043,10 +2043,12 @@ int
> > > snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t 
> > > *conf,
> > > 
> > >  void snd_pcm_direct_reset_slave_ptr(snd_pcm_t *pcm, 
> > > snd_pcm_direct_t
> > > *dmix)  {
> > > -
> > > +     /* For buffer size less than two period size, the start position
> > > +      * of slave app ptr is rounded up in order to avoid xruns
> > > +      */
> > >        if (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_ROUNDUP ||
> > >                (dmix->hw_ptr_alignment == SND_PCM_HW_PTR_ALIGNMENT_AUTO &&
> > > -             pcm->buffer_size <= pcm->period_size * 2))
> > > +             pcm->buffer_size < pcm->period_size * 2))
> > >                dmix->slave_appl_ptr =
> > >                        ((dmix->slave_appl_ptr + 
> > > dmix->slave_period_size - 1)
> > /
> > >                        dmix->slave_period_size) * 
> > > dmix->slave_period_size;
> >  
> > It's still not clear to me why this change is made.
> >  
> > The example mentioned in the above (period_size=96, buffer_size=191) 
> > also matches with the condition before the change, so there should 
> > be behavior change by the patch.
> >  
> > IOW, your patch does nothing but modifying the condition to drop the 
> > case buffer_size == period_size * 2.  Why this condition can't
> > (shouldn't) be a target of the round up?  That needs the 
> > clarification in the patch description.
> > 
> > thanks,
> >  
> > Takashi
> > 
> >