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