Re: [PATCH 4/4] ALSA: x86: Refactor PCM process engine

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 02/06/2017 09:54 AM, Takashi Iwai wrote:
> 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.
>
>
ok, i guess I need more coffee... David's code did not include the 
additional read and I wanted to check this was fine.
Thanks for the precision.