[PATCH 4/6] alsa-lib:pcm: fix the hw_ptr update until the boundary available.

[PATCH 4/6] alsa-lib:pcm: fix the hw_ptr update until the boundary available.

[sutar.mounesh]

From: mahendran.k <mahendran.kuppusamy@in.bosch.com>

For long time test case, the slave_hw_ptr will exceed the boundary
and wraparound the slave_hw_ptr. This slave boundary wraparound will
cause the rate->hw_ptr to wraparound irrespective of the rate->boundary
availability and due to that the available size goes wrong.

Hence, To get the correct available size,
- Its necessary to increment the rate->hw_ptr upto the rate->boundary
and then wraparound the rate->hw_ptr.
- While handling fraction part of slave period, rounded value will be
introduced by input_frames(). To eliminate rounding issue on rate->hw_ptr,
subtract last rounded value from rate->hw_ptr and add new rounded value
of present slave_hw_ptr fraction part to rate->hw_ptr.

Signed-off-by: mahendran.k <mahendran.kuppusamy@in.bosch.com>
Signed-off-by: Mounesh Sutar <mounesh_sutar@mentor.com>

diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index 1f830dd..5bee77c 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -50,7 +50,7 @@ typedef struct _snd_pcm_rate snd_pcm_rate_t;
 
 struct _snd_pcm_rate {
 	snd_pcm_generic_t gen;
-	snd_pcm_uframes_t appl_ptr, hw_ptr;
+	snd_pcm_uframes_t appl_ptr, hw_ptr, last_slave_hw_ptr;
 	snd_pcm_uframes_t last_commit_ptr;
 	snd_pcm_uframes_t orig_avail_min;
 	snd_pcm_sw_params_t sw_params;
@@ -563,14 +563,28 @@ static inline void snd_pcm_rate_sync_hwptr0(snd_pcm_t *pcm, snd_pcm_uframes_t sl
 {
 	snd_pcm_rate_t *rate = pcm->private_data;
 
+	snd_pcm_sframes_t slave_hw_ptr_diff = slave_hw_ptr - rate->last_slave_hw_ptr;
+	snd_pcm_sframes_t last_slave_hw_ptr_frac;
+
 	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
 		return;
-	/* FIXME: boundary overlap of slave hw_ptr isn't evaluated here!
-	 *        e.g. if slave rate is small... 
-	 */
-	rate->hw_ptr =
-		(slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size +
-		rate->ops.input_frames(rate->obj, slave_hw_ptr % rate->gen.slave->period_size);
+
+	if (slave_hw_ptr_diff < 0)
+		slave_hw_ptr_diff += rate->gen.slave->boundary; /* slave boundary wraparound */
+	else if (slave_hw_ptr_diff == 0)
+		return;
+	last_slave_hw_ptr_frac = rate->last_slave_hw_ptr % rate->gen.slave->period_size;
+	/* While handling fraction part fo slave period, rounded value will be introduced by input_frames().
+	 * To eliminate rounding issue on rate->hw_ptr, subtract last rounded value from rate->hw_ptr and
+	 * add new rounded value of present slave_hw_ptr fraction part to rate->hw_ptr. Hence,
+	 * rate->hw_ptr += [ (no. of updated slave periods * pcm rate period size) -
+	 * 				fractional part of last_slave_hw_ptr rounded value +
+	 * 				fractional part of updated slave hw ptr's rounded value ] */
+	rate->hw_ptr += (
+			(((last_slave_hw_ptr_frac + slave_hw_ptr_diff) / rate->gen.slave->period_size) * pcm->period_size) -
+			rate->ops.input_frames(rate->obj, last_slave_hw_ptr_frac) +
+			rate->ops.input_frames(rate->obj, (last_slave_hw_ptr_frac + slave_hw_ptr_diff) % rate->gen.slave->period_size));
+	rate->last_slave_hw_ptr = slave_hw_ptr;
 
 	rate->hw_ptr %= pcm->boundary;
 }
@@ -635,6 +649,7 @@ static int snd_pcm_rate_prepare(snd_pcm_t *pcm)
 		return err;
 	*pcm->hw.ptr = 0;
 	*pcm->appl.ptr = 0;
+	rate->last_slave_hw_ptr = 0;
 	err = snd_pcm_rate_init(pcm);
 	if (err < 0)
 		return err;
@@ -650,6 +665,7 @@ static int snd_pcm_rate_reset(snd_pcm_t *pcm)
 		return err;
 	*pcm->hw.ptr = 0;
 	*pcm->appl.ptr = 0;
+	rate->last_slave_hw_ptr = 0;
 	err = snd_pcm_rate_init(pcm);
 	if (err < 0)
 		return err;
-- 
1.7.9.5

Re: [PATCH 4/6] alsa-lib:pcm: fix the hw_ptr update until the boundary available.

[Takashi Iwai]

On Fri, 30 Dec 2016 07:29:27 +0100,
sutar.mounesh@gmail.com wrote:
> 
> From: mahendran.k <mahendran.kuppusamy@in.bosch.com>
> 
> For long time test case, the slave_hw_ptr will exceed the boundary
> and wraparound the slave_hw_ptr. This slave boundary wraparound will
> cause the rate->hw_ptr to wraparound irrespective of the rate->boundary
> availability and due to that the available size goes wrong.
> 
> Hence, To get the correct available size,
> - Its necessary to increment the rate->hw_ptr upto the rate->boundary
> and then wraparound the rate->hw_ptr.
> - While handling fraction part of slave period, rounded value will be
> introduced by input_frames(). To eliminate rounding issue on rate->hw_ptr,
> subtract last rounded value from rate->hw_ptr and add new rounded value
> of present slave_hw_ptr fraction part to rate->hw_ptr.
> 
> Signed-off-by: mahendran.k <mahendran.kuppusamy@in.bosch.com>
> Signed-off-by: Mounesh Sutar <mounesh_sutar@mentor.com>

Thanks, it's been a long-standing issue, indeed.

I applied this one (again) with a slight coding style fix now.
But...

> diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
> index 1f830dd..5bee77c 100644
> --- a/src/pcm/pcm_rate.c
> +++ b/src/pcm/pcm_rate.c
> @@ -50,7 +50,7 @@ typedef struct _snd_pcm_rate snd_pcm_rate_t;
>  
>  struct _snd_pcm_rate {
>  	snd_pcm_generic_t gen;
> -	snd_pcm_uframes_t appl_ptr, hw_ptr;
> +	snd_pcm_uframes_t appl_ptr, hw_ptr, last_slave_hw_ptr;
>  	snd_pcm_uframes_t last_commit_ptr;
>  	snd_pcm_uframes_t orig_avail_min;
>  	snd_pcm_sw_params_t sw_params;
> @@ -563,14 +563,28 @@ static inline void snd_pcm_rate_sync_hwptr0(snd_pcm_t *pcm, snd_pcm_uframes_t sl
>  {
>  	snd_pcm_rate_t *rate = pcm->private_data;
>  
> +	snd_pcm_sframes_t slave_hw_ptr_diff = slave_hw_ptr - rate->last_slave_hw_ptr;
> +	snd_pcm_sframes_t last_slave_hw_ptr_frac;
> +
>  	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
>  		return;
> -	/* FIXME: boundary overlap of slave hw_ptr isn't evaluated here!
> -	 *        e.g. if slave rate is small... 
> -	 */
> -	rate->hw_ptr =
> -		(slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size +
> -		rate->ops.input_frames(rate->obj, slave_hw_ptr % rate->gen.slave->period_size);
> +
> +	if (slave_hw_ptr_diff < 0)
> +		slave_hw_ptr_diff += rate->gen.slave->boundary; /* slave boundary wraparound */
> +	else if (slave_hw_ptr_diff == 0)
> +		return;
> +	last_slave_hw_ptr_frac = rate->last_slave_hw_ptr % rate->gen.slave->period_size;
> +	/* While handling fraction part fo slave period, rounded value will be introduced by input_frames().
> +	 * To eliminate rounding issue on rate->hw_ptr, subtract last rounded value from rate->hw_ptr and
> +	 * add new rounded value of present slave_hw_ptr fraction part to rate->hw_ptr. Hence,
> +	 * rate->hw_ptr += [ (no. of updated slave periods * pcm rate period size) -
> +	 * 				fractional part of last_slave_hw_ptr rounded value +
> +	 * 				fractional part of updated slave hw ptr's rounded value ] */
> +	rate->hw_ptr += (
> +			(((last_slave_hw_ptr_frac + slave_hw_ptr_diff) / rate->gen.slave->period_size) * pcm->period_size) -
> +			rate->ops.input_frames(rate->obj, last_slave_hw_ptr_frac) +
> +			rate->ops.input_frames(rate->obj, (last_slave_hw_ptr_frac + slave_hw_ptr_diff) % rate->gen.slave->period_size));
> +	rate->last_slave_hw_ptr = slave_hw_ptr;

.... this ended up with a substantial increase of computation.
Can we optimize, e.g. doing this conditionally only for the
overwrapping case?


thanks,

Takashi

Re: [PATCH 4/6] alsa-lib:pcm: fix the hw_ptr update until the boundary available.

[Mahendran Kuppusamy (RBEI/ECF3)]

Hello Takashi Iwan, Hello Sutar,

Sorry for the late response. 

Yes, Fractional calculation is required for the overwrapping case only. So, Please do optimization and go ahead of "doing this conditionally only for the overwrapping case".

Please let me know, Any changes required for me.

Thanks in advance.

Best regards,

Mahendran Kuppusamy
RBEI/ECF3  

Tel. +91 80 6136-4450 


-----Original Message-----
From: Takashi Iwai [mailto:tiwai@suse.de] 
Sent: Monday, January 02, 2017 7:29 PM
To: sutar.mounesh@gmail.com
Cc: alsa-devel@alsa-project.org; mounesh_sutar@mentor.com; Mahendran Kuppusamy (RBEI/ECF32) <Mahendran.Kuppusamy@in.bosch.com>
Subject: Re: [PATCH 4/6] alsa-lib:pcm: fix the hw_ptr update until the boundary available.

On Fri, 30 Dec 2016 07:29:27 +0100,
sutar.mounesh@gmail.com wrote:
> 
> From: mahendran.k <mahendran.kuppusamy@in.bosch.com>
> 
> For long time test case, the slave_hw_ptr will exceed the boundary and 
> wraparound the slave_hw_ptr. This slave boundary wraparound will cause 
> the rate->hw_ptr to wraparound irrespective of the rate->boundary 
> availability and due to that the available size goes wrong.
> 
> Hence, To get the correct available size,
> - Its necessary to increment the rate->hw_ptr upto the rate->boundary 
> and then wraparound the rate->hw_ptr.
> - While handling fraction part of slave period, rounded value will be 
> introduced by input_frames(). To eliminate rounding issue on 
> rate->hw_ptr, subtract last rounded value from rate->hw_ptr and add 
> new rounded value of present slave_hw_ptr fraction part to rate->hw_ptr.
> 
> Signed-off-by: mahendran.k <mahendran.kuppusamy@in.bosch.com>
> Signed-off-by: Mounesh Sutar <mounesh_sutar@mentor.com>

Thanks, it's been a long-standing issue, indeed.

I applied this one (again) with a slight coding style fix now.
But...

> diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 
> 1f830dd..5bee77c 100644
> --- a/src/pcm/pcm_rate.c
> +++ b/src/pcm/pcm_rate.c
> @@ -50,7 +50,7 @@ typedef struct _snd_pcm_rate snd_pcm_rate_t;
>  
>  struct _snd_pcm_rate {
>  	snd_pcm_generic_t gen;
> -	snd_pcm_uframes_t appl_ptr, hw_ptr;
> +	snd_pcm_uframes_t appl_ptr, hw_ptr, last_slave_hw_ptr;
>  	snd_pcm_uframes_t last_commit_ptr;
>  	snd_pcm_uframes_t orig_avail_min;
>  	snd_pcm_sw_params_t sw_params;
> @@ -563,14 +563,28 @@ static inline void 
> snd_pcm_rate_sync_hwptr0(snd_pcm_t *pcm, snd_pcm_uframes_t sl  {
>  	snd_pcm_rate_t *rate = pcm->private_data;
>  
> +	snd_pcm_sframes_t slave_hw_ptr_diff = slave_hw_ptr - rate->last_slave_hw_ptr;
> +	snd_pcm_sframes_t last_slave_hw_ptr_frac;
> +
>  	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
>  		return;
> -	/* FIXME: boundary overlap of slave hw_ptr isn't evaluated here!
> -	 *        e.g. if slave rate is small... 
> -	 */
> -	rate->hw_ptr =
> -		(slave_hw_ptr / rate->gen.slave->period_size) * pcm->period_size +
> -		rate->ops.input_frames(rate->obj, slave_hw_ptr % rate->gen.slave->period_size);
> +
> +	if (slave_hw_ptr_diff < 0)
> +		slave_hw_ptr_diff += rate->gen.slave->boundary; /* slave boundary wraparound */
> +	else if (slave_hw_ptr_diff == 0)
> +		return;
> +	last_slave_hw_ptr_frac = rate->last_slave_hw_ptr % rate->gen.slave->period_size;
> +	/* While handling fraction part fo slave period, rounded value will be introduced by input_frames().
> +	 * To eliminate rounding issue on rate->hw_ptr, subtract last rounded value from rate->hw_ptr and
> +	 * add new rounded value of present slave_hw_ptr fraction part to rate->hw_ptr. Hence,
> +	 * rate->hw_ptr += [ (no. of updated slave periods * pcm rate period size) -
> +	 * 				fractional part of last_slave_hw_ptr rounded value +
> +	 * 				fractional part of updated slave hw ptr's rounded value ] */
> +	rate->hw_ptr += (
> +			(((last_slave_hw_ptr_frac + slave_hw_ptr_diff) / rate->gen.slave->period_size) * pcm->period_size) -
> +			rate->ops.input_frames(rate->obj, last_slave_hw_ptr_frac) +
> +			rate->ops.input_frames(rate->obj, (last_slave_hw_ptr_frac + slave_hw_ptr_diff) % rate->gen.slave->period_size));
> +	rate->last_slave_hw_ptr = slave_hw_ptr;

.... this ended up with a substantial increase of computation.
Can we optimize, e.g. doing this conditionally only for the overwrapping case?


thanks,

Takashi