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

[Takashi Iwai <tiwai@suse.de>]

On Wed, 15 May 2019 08:26:35 +0200,
<vanitha.channaiah@in.bosch.com> wrote:
> 
> From: Vanitha Channaiah <vanitha.channaiah@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 <vanitha.channaiah@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