On Tue, 07 Feb 2017 17:38:00 +0100,
Takashi Iwai wrote:
>
> At the interrupt handler, we usually call snd_pcm_period_elapsed() to
> inform the PCM core to proceed the hwptr. However, the hwptr update
> might have been already processed by the explicit call of PCM pointer
> callback via another thread. If both happen concurrently, the call of
> snd_pcm_period_elapsed() might be wrong.
>
> Here is the fix for this slightly possible race: had_process_ringbuf()
> returns the number of processed BDs, and the irq handler calls
> snd_pcm_period_elapsed() only when it's not zero.
Actually this looks like a wrong assumption. The call of
snd_pcm_period_elapsed() is still OK even after other thread already
updating hw_ptr, and it rather must call. The PCM timer notification
is triggered only in snd_pcm_period_elapsed(), thus without its call,
the PCM timer won't be updated properly. I drop this one.
Takashi
>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
> sound/x86/intel_hdmi_audio.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c
> index 15147fec1a7e..a0c9a4e0c95d 100644
> --- a/sound/x86/intel_hdmi_audio.c
> +++ b/sound/x86/intel_hdmi_audio.c
> @@ -899,10 +899,13 @@ static void had_advance_ringbuf(struct snd_pcm_substream *substream,
> }
>
> /* process the current BD(s);
> - * returns the current PCM buffer byte position, or -EPIPE for underrun.
> + * returns the number of processed BDs, zero if no BD was processed,
> + * or -EPIPE for underrun.
> + * When @pos_ret is non-NULL, the current PCM buffer byte position is stored.
> */
> static int had_process_ringbuf(struct snd_pcm_substream *substream,
> - struct snd_intelhad *intelhaddata)
> + struct snd_intelhad *intelhaddata,
> + int *pos_ret)
> {
> int len, processed;
> unsigned long flags;
> @@ -917,7 +920,7 @@ static int had_process_ringbuf(struct snd_pcm_substream *substream,
> if (len < 0 || len > intelhaddata->period_bytes) {
> dev_dbg(intelhaddata->dev, "Invalid buf length %d\n",
> len);
> - len = -EPIPE;
> + processed = -EPIPE;
> goto out;
> }
>
> @@ -926,23 +929,27 @@ static int had_process_ringbuf(struct snd_pcm_substream *substream,
>
> /* len=0 => already empty, check the next buffer */
> if (++processed >= intelhaddata->num_bds) {
> - len = -EPIPE; /* all empty? - report underrun */
> + processed = -EPIPE; /* all empty? - report underrun */
> goto out;
> }
> had_advance_ringbuf(substream, intelhaddata);
> }
>
> - len = intelhaddata->period_bytes - len;
> - len += intelhaddata->period_bytes * intelhaddata->pcmbuf_head;
> + if (pos_ret) {
> + len = intelhaddata->period_bytes - len;
> + len += intelhaddata->period_bytes * intelhaddata->pcmbuf_head;
> + *pos_ret = len;
> + }
> out:
> spin_unlock_irqrestore(&intelhaddata->had_spinlock, flags);
> - return len;
> + return processed;
> }
>
> /* called from irq handler */
> static void had_process_buffer_done(struct snd_intelhad *intelhaddata)
> {
> struct snd_pcm_substream *substream;
> + int processed;
>
> if (!intelhaddata->connected)
> return; /* disconnected? - bail out */
> @@ -952,9 +959,10 @@ static void had_process_buffer_done(struct snd_intelhad *intelhaddata)
> return; /* no stream? - bail out */
>
> /* process or stop the stream */
> - if (had_process_ringbuf(substream, intelhaddata) < 0)
> + processed = had_process_ringbuf(substream, intelhaddata, NULL);
> + if (processed < 0)
> snd_pcm_stop_xrun(substream);
> - else
> + else if (processed > 0)
> snd_pcm_period_elapsed(substream);
>
> had_substream_put(intelhaddata);
> @@ -1254,8 +1262,7 @@ static snd_pcm_uframes_t had_pcm_pointer(struct snd_pcm_substream *substream)
> if (!intelhaddata->connected)
> return SNDRV_PCM_POS_XRUN;
>
> - len = had_process_ringbuf(substream, intelhaddata);
> - if (len < 0)
> + if (had_process_ringbuf(substream, intelhaddata, &len) < 0)
> return SNDRV_PCM_POS_XRUN;
> return bytes_to_frames(substream->runtime, len);
> }
> --
> 2.11.0
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@xxxxxxxxxxxxxxxx
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
