[alsa-devel] [PATCH 4/4] ALSA: x86: Refactor PCM process engine

[Takashi Iwai]

On Mon, 06 Feb 2017 16:46:53 +0100,
Pierre-Louis Bossart wrote:
> 
> Looks nice, with one comment below:
> 
> 
> > +/* process a bd, advance to the next */
> > +static void had_advance_ringbuf(struct snd_pcm_substream *substream,
> > +				struct snd_intelhad *intelhaddata)
> > +{
> > +	int num_periods = substream->runtime->periods;
> > +
> > +	/* reprogram the next buffer */
> > +	had_prog_bd(substream, intelhaddata);
> > +
> > +	/* proceed to next */
> > +	intelhaddata->pcmbuf_head++;
> > +	intelhaddata->pcmbuf_head %= num_periods;
> > +}
> > +
> > +/* process the current BD(s);
> > + * returns the current PCM buffer byte position, or -EPIPE for underrun.
> > + */
> > +static int had_process_ringbuf(struct snd_pcm_substream *substream,
> > +			       struct snd_intelhad *intelhaddata)
> > +{
> > +	int len, processed;
> > +	unsigned long flags;
> > +
> > +	processed = 0;
> > +	spin_lock_irqsave(&intelhaddata->had_spinlock, flags);
> > +	for (;;) {
> > +		/* get the remaining bytes on the buffer */
> > +		had_read_register(intelhaddata,
> > +				  AUD_BUF_LEN(intelhaddata->bd_head),
> > +				  &len);
> > +		if (len < 0 || len > intelhaddata->period_bytes) {
> > +			dev_dbg(intelhaddata->dev, "Invalid buf length %d\n",
> > +				len);
> > +			len = -EPIPE;
> > +			goto out;
> > +		}
> > +
> > +		if (len > 0) /* OK, this is the current buffer */
> > +			break;
> > +
> > +		/* len=0 => already empty, check the next buffer */
> > +		if (++processed >= intelhaddata->num_bds) {
> > +			len = -EPIPE; /* all empty? - report underrun */
> > +			goto out;
> > +		}
> > +		had_advance_ringbuf(substream, intelhaddata);
> > +	}
> > +
> > +	len = intelhaddata->period_bytes - len;
> > +	len += intelhaddata->period_bytes * intelhaddata->pcmbuf_head;
> I don't know if this code is completely correct (and I had similar
> concerns with David's).
> If the len==0, then the new buffer descriptor will be used in the next
> iteration. If the register is read immediately, there is a risk that
> the DMA position has not moved and len then becomes
> intelhaddata->period_bytes, but the last line will increase the number
> of bytes by a period. I think there should be a test here to handle
> this corner case.

That's OK.  When len=0, the loop goes to the next buffer -- i.e.
pcm_buf is also increased.  Then it reads len=period_bytes and quits
the loop.  Now len is re-calculated as
  len = period_bytes - len;
	--> len = 0
  len += period_bytes * pcmbuf_head;
	--> len = new head position in bytes

which is exactly the expected position.


thanks,

Takashi