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

[Pierre-Louis Bossart]

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.