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

[Takashi Iwai <tiwai@suse.de>]

On Sun, 10 Jan 2021 18:41:46 +0100,
Lauri Kasanen wrote:
> 
> On Sun, 10 Jan 2021 18:22:50 +0100
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Sun, 10 Jan 2021 18:03:32 +0100,
> > Lauri Kasanen wrote:
> > > On Sun, 10 Jan 2021 11:24:22 +0100
> > > Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > > > At first there was no nextpos, and _pointer() always reported pos. This
> > > > > didn't work, the core played through the audio at chipmunk speed. So
> > > > > there must be more that I don't understand here.
> > > >
> > > > Try to set the periods_min=2 and the integer periods hw constraint at
> > > > first, and change the pointer callback to return nextpos.  Also, at
> > > > the push function, set runtime->delay = period_size as well.
> > >
> > > When I do all this, it still causes the chipmunk speed. Several seconds
> > > of audio gets played in 0.3s or so. Sorry if this is taking too much of
> > > your time, I'm a bit lost here at what the alsa core is expecting.
> > >
> > > Printks show the following repeats:
> > > start, period size 1024
> > > push, bool irq=0
> > > irq fired
> > > push, bool irq=1
> > > pointer at 8192
> > > stop
> >
> > Hm, is the above about the result with the pointer callback returning
> > pos, not nextpos?  If so,
> 
> It was returning nextpos, but the pointer printk was in bytes. 8192
> bytes = 2048 frames.

OK, then it must be right.

Then I suppose that the update of pos should be changed in a different
way; it should always point to the previous nextpos.  That is,
something like:

static void n64audio_push(struct n64audio_t *priv, uint8_t irq)
{
	....
	if (irq)
		priv->chan.pos = priv->chan.nextpos;
	priv->chan.nextpos += count;
	priv->chan.nextpos %= priv->chan.bufsize;

If we use nextpos as the position, it'll lead to the double steps at
the first IRQ handling without snd_pcm_period_elapsed() call (the
first step missed it), and this may confuse PCM core.


> > > start, period size 1024
> > > push, bool irq=0
> >
> > At this moment, nextpos is 1024, and it should take some time until
> >
> > > irq fired
> >
> > ... this IRQ is triggered; it must be the period time.
> > Was the reported timing as expected?
> 
> It's roughly correct, but timing is not very precise, as printk itself
> has heavy overhead for the 93 MHz cpu.

OK, that sounds good, at least.


thanks,

Takashi