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

[Takashi Iwai <tiwai@suse.de>]

On Sat, 09 Jan 2021 19:17:16 +0100,
Takashi Iwai wrote:
> 
> On Sat, 09 Jan 2021 18:46:01 +0100,
> Lauri Kasanen wrote:
> > 
> > On Sat, 09 Jan 2021 09:16:08 +0100
> > Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > > > > > +static const struct snd_pcm_hardware n64audio_pcm_hw = {
> > > > > > +	.info = (SNDRV_PCM_INFO_MMAP |
> > > > > > +		 SNDRV_PCM_INFO_MMAP_VALID |
> > > > > > +		 SNDRV_PCM_INFO_INTERLEAVED |
> > > > > > +		 SNDRV_PCM_INFO_BLOCK_TRANSFER),
> > > > > > +	.formats =          SNDRV_PCM_FMTBIT_S16_BE,
> > > > > > +	.rates =            SNDRV_PCM_RATE_8000_48000,
> > > > > > +	.rate_min =         8000,
> > > > > > +	.rate_max =         48000,
> > > > > > +	.channels_min =     2,
> > > > > > +	.channels_max =     2,
> > > > > > +	.buffer_bytes_max = 32768,
> > > > > > +	.period_bytes_min = 1024,
> > > > > > +	.period_bytes_max = 32768,
> > > > > > +	.periods_min =      1,
> > > > >
> > > > > periods_min=1 makes little sense for this driver.
> > > >
> > > > I have some questions about this.
> > > >
> > > > When I had periods_min = 128, OSS apps were broken. I mean simple ones,
> > > > open /dev/dsp, ioctl the format/rate/stereo, write data. They got an IO
> > > > error errno IIRC, and no clarifying error in dmesg.
> > > >
> > > > I tried following the error with printks, several levels deep. I gave
> > > > up when it got to the constraint resolving function, and there was no
> > > > good way to print and track which constraint failed, why, and who set
> > > > the constraint.
> > >
> > > Did you try to set up the hw constraint for the integer PERIODS like
> > > below at open?
> > >   snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS)
> > >
> > > Without this, it'd allow inconsistent buffer/period set up in your
> > > case.
> > 
> > No, not yet. But surely an inconsistent buffer size would still play
> > something, instead of immediately erroring out?
> 
> In some cases, it's possible.  PCM OSS translation has some special
> way depending on the period ("fragment" in OSS) setup...
> 
> > > > Only through blind guessing did I stumble upon periods_min.
> > >
> > > The periods_min usually defines the hardware/software limit of the
> > > interrupt transfer.
> > 
> > Why do you say periods_min=1 makes little sense? At 44.1 khz, that'd be
> > 172 interrupts per second, which is a lot but workable? There is no hw
> > limit against 172 irqs/s.
> 
> Well, it's not about the sample rate or the process speed.  You need
> to know what periods=1 means.  periods=1 is a VERY special usage.  No
> double buffering and the driver has to report the precise accurate
> position without period updates; i.e. it's almost for free-wheeling
> DMA transfer.  Hence periods_min=1 makes sense if the driver may
> behave like that.

And, after reading the patch again, I found another issue that is
relevant with this. 

The function transfers the data chunk of the period size is
implemented as:

static void n64audio_push(struct n64audio_t *priv, uint8_t irq)
{
	....
	memcpy(priv->ring_base, runtime->dma_area + priv->chan.nextpos, count);

	n64audio_write_reg(AI_ADDR_REG, priv->ring_base_dma);
	n64audio_write_reg(AI_LEN_REG, count);

	priv->chan.nextpos += count;
	priv->chan.nextpos %= priv->chan.bufsize;
	if (irq)
		priv->chan.pos = priv->chan.nextpos;

... and this is called from two places, the interrupt handler:

static irqreturn_t n64audio_isr(int irq, void *dev_id)
{
	....
	n64audio_push(priv, 1);
	snd_pcm_period_elapsed(priv->chan.substream);

... and the trigger START:

static int n64audio_pcm_trigger(struct snd_pcm_substream *substream,
				int cmd)
{
	switch (cmd) {
	case SNDRV_PCM_TRIGGER_START:
		n64audio_push(substream->pcm->private_data, 0);

Meanwhile the pointer callback returns the chan.pos:

static snd_pcm_uframes_t n64audio_pcm_pointer(struct snd_pcm_substream *substream)
{
	....
	return bytes_to_frames(substream->runtime,
			       priv->chan.pos);

When the start starts, it copies the full period size data, and moves
nextpos to period size while keeping pos 0.  And, at this moment, it's
even possible that no enough data has been filled for the period
size; this is practically a buffer underrun.
Usually PCM core can catch the buffer underrun by comparing the
current position reported by the pointer callback against the filled
size, but in this case, PCM core can't know of it because the driver
just tells the position 0.  This is one problem.

Then, at the first period IRQ, the next period is copied, then nextpos
becomes 2*period_size.  At this moment, pos = nextpos, hence it jumps
from 0 to 2*periodsize out of sudden.  It's quite confusing behavior
for applications.  That's the second problem.

I guess that both problems could be avoided if n64audio_pointer()
reports always nextpos instead of pos.  And, the driver may set
runtime->delay value to correct back for the period size; ideally this 
should correct to the actual playback position, but since we can't
read it, nothing but a constant max correction (= period size) is
applicable.


Takashi