On Wed, 17 Jun 2020, Giuliano Pochini wrote:
> On Tue, 16 Jun 2020 14:17:43 +0100
> Mark Hills <mark@xxxxxxxx> 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@xxxxxxxx>
>
> --- 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
