[PATCH 3/3] echoaudio: Address bugs in the interrupt handling

[Mark Hills]

On Wed, 17 Jun 2020, Giuliano Pochini wrote:

> On Tue, 16 Jun 2020 14:17:43 +0100
> Mark Hills <mark at xwax.org> wrote:
> 
> > +	counter = le32_to_cpu(*pipe->dma_counter);
> > +
> > +	period = bytes_to_frames(runtime, counter)
> > +		/ runtime->period_size;
> > +	last_period = bytes_to_frames(runtime, pipe->last_counter)
> > +		/ runtime->period_size;
> > +
> > +	if (period == last_period)
> > +		return 0;  /* don't do any work yet */
> > +
> > +	step = counter - pipe->last_counter;
> 
> Uhmm.. no, when the dma_counter wraps around the comparison gives wrong 
> result, unless 4G is divisible by the buffer size.

Agree.

> Please consider this patch (to apply after yours). It computes the new 
> period using pipe->position before and after adding the amount of bytes 
> tranferred since the previous period. Briefly tested - It seems ok.
> 
> 
> Signed-off-by: Giuliano Pochini <pochini at shiny.it>
> 
> --- sound/pci/echoaudio/echoaudio.c__orig	2020-06-16 23:36:02.818601055 +0200
> +++ sound/pci/echoaudio/echoaudio.c	2020-06-16 23:52:46.593325066 +0200
> @@ -1781,7 +1781,7 @@
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	struct audiopipe *pipe = runtime->private_data;
> -	unsigned counter, step, period, last_period;
> +	u32 counter, step, period, last_period, pipe_position;
>  	size_t buffer_bytes;
>  
>  	if (pipe->state != PIPE_STATE_STARTED)
> @@ -1789,24 +1789,22 @@
>  
>  	counter = le32_to_cpu(*pipe->dma_counter);
>  
> -	period = bytes_to_frames(runtime, counter)
> +	step = counter - pipe->last_counter;
> +	pipe_position = pipe->position + step;	/* bytes */
> +	buffer_bytes = frames_to_bytes(runtime, runtime->buffer_size);
> +	while (pipe_position >= buffer_bytes)
> +		pipe_position -= buffer_bytes;
> +
> +	period = bytes_to_frames(runtime, pipe_position)
>  		/ runtime->period_size;
> -	last_period = bytes_to_frames(runtime, pipe->last_counter)
> +	last_period = bytes_to_frames(runtime, pipe->position)
>  		/ runtime->period_size;
>  
>  	if (period == last_period)
>  		return 0;  /* don't do any work yet */
>  
> -	step = counter - pipe->last_counter;
> +	pipe->position = pipe_position;
>  	pipe->last_counter = counter;
> -
> -	pipe->position += step;  /* bytes */
> -
> -	buffer_bytes = frames_to_bytes(runtime, runtime->buffer_size);
> -
> -	while (pipe->position >= buffer_bytes)
> -		pipe->position -= buffer_bytes;
> -
>  	return 1;

I think this risks returning to a case where it concludes nothing advances 
if the counter advances by a whole buffer?

You might be able to do the comparison before wrapping pipe_position, but 
hopefully you'll consider my patch in reply to Takashi has more clarity.

-- 
Mark