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