Re: [PATCH 5/6 v2] sound: Add n64 driver

[Lauri Kasanen <cand@gmx.com>]

On Mon, 11 Jan 2021 16:25:08 +0100
Takashi Iwai <tiwai@suse.de> wrote:

> On Mon, 11 Jan 2021 13:02:22 +0100,
> Lauri Kasanen wrote:
> >
> > On Mon, 11 Jan 2021 11:11:39 +0100
> > Takashi Iwai <tiwai@suse.de> wrote:
> > > Oh yes, at the last IRQ, the push should be avoided.
> > > I guess that the code order should be changed to the following way:
> > >
> > >   1. advance the position for a period size
> > >   2. call snd_pcm_period_elapsed()
> > >   3. check if the stream is still running
> > >   4. copy the next chunk and update nextpos
> >
> > This order gives correct pointer advancing etc, but now it's hitting a
> > new problem: the pcm core is reusing the buffer from under the audio
> > card. It's writing new data to the area that is currently being read by
> > DMA.
>
> Could you elaborate?  I still don't get what's going on there.

The audio plays partially out of order. At the point when the IRQ
fires, the data for the next period is correct, but it becomes
incorrect during playing it.

This is clear because if I memcpy it in the isr, like in the first
patch, the playback is correct. When copied to a private buffer, the
pcm core can't overwrite it when the card is reading it.

However the playback speed is correct, and when I print out the events,
the pointer advances correctly, one period at a time, and there are no
continuous stops and restarts.

> > I assume the core expects DMA to be instant, but in this card's case
> > it's ondemand, reading bytes as needed.
>
> No, PCM core doesn't expect it.  The only expectation is that the data
> from the current pointer to the next period boundary will be
> transferred until the next period-elapsed call.

Curious. This problem shouldn't exist then if everything is correct.
I'm thankful for your help in solving these; I haven't done alsa
hacking before.

In the current code, everything should be in sync with how you wrote.

period size 1026 frames
irq fires for the first time, pos becomes 1026*4=4104 bytes
snd_pcm_period_elapsed gets called, pointer returns 4104/4 frames
stream is running, so we call push()
push() starts playing at 4104 bytes into alsa's dma buffer

So the driver is playing the second period, and the core shouldn't be
writing into it as the pointer points to the start of that period.

> > By restoring the memcpy buffer, I get good audio with this new order
> > (sans occasional crackling due to the memcpy taking too long).
>
> The overwriting problem has been already present in the original patch
> version as I already argued.  The driver copies the full next period
> to be updated, but this is never guaranteed to have been filled by the
> application.  Maybe this didn't surface obviously with the original
> version because it essentially gives one more period available on the
> buffer -- i.e. it might be a matter of number of periods.

Yes, this startup issue was present, but I believe it's different to
the current issue. I could be wrong though.

- Lauri