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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
