[alsa-devel] intel 8x0 ALSA driver in virtualbox - choppy sound

[Takashi Iwai]

At Sat, 14 Aug 2010 09:53:38 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Fri, 13 Aug 2010, Takashi Iwai wrote:
> 
> >> http://www.alsa-project.org/main/index.php/XRUN_Debug is a good start to
> >> see if the virtual machine does receive interrupts in the right time.
> >
> > Actually, there is no real interrupt, but just the virtual one,
> > thus it can be never accurate.  So, rather the question is how the
> > driver is tolerant for sloppy IRQ updates.
> >
> > Usually, when the period size is large and the buffer have many
> > periods (not two), the delayed update works more stably.
> 
> I know. The current position checking in pcm_lib.c should handle 
> delayed updates, but it does not handle well the multiple elapsed() calls 
> merged to one time (double interrupt acknowledges). In some cases,
> the code things that buffer_size boundary were crossed, thus the hw_ptr 
> position update fails.
> 
> I wanted to force the driver to be responsible to call the elapsed() 
> calls at the right time. But it's true that the interrupt controller and 
> other operating system contrainsts and behaviour might influence the 
> interrupt processing which is more visible in the virtual environment.
> 
> Thinking more about it, we need probably another quick check against 
> different timing source to check properly for the buffer_size crossing.
> 
> What about this patch (the hw_base gets updates in in_interrupt only when 
> delta for next jiffies is bigger than (buffer jiffies / 2)). It should 
> handle all double acks from the interrupt callbacks:

Looks good.  We need some tests, though.
Maybe we can put some randomness in dummy driver...


thanks,

Takashi


> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 85f1c6b..dfd9b76 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -278,6 +278,7 @@ struct snd_pcm_runtime {
>   	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
>   	snd_pcm_uframes_t hw_ptr_interrupt; /* Position at interrupt time */
>   	unsigned long hw_ptr_jiffies;	/* Time when hw_ptr is updated */
> +	unsigned long hw_ptr_buffer_jiffies; /* buffer time in jiffies */
>   	snd_pcm_sframes_t delay;	/* extra delay; typically FIFO size */
> 
>   	/* -- HW params -- */
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index e23e0e7..a1707cc 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -334,11 +334,15 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
>   		/* delta = "expected next hw_ptr" for in_interrupt != 0 */
>   		delta = runtime->hw_ptr_interrupt + runtime->period_size;
>   		if (delta > new_hw_ptr) {
> -			hw_base += runtime->buffer_size;
> -			if (hw_base >= runtime->boundary)
> -				hw_base = 0;
> -			new_hw_ptr = hw_base + pos;
> -			goto __delta;
> +			/* check for double acknowledged interrupts */
> +			hdelta = jiffies - runtime->hw_ptr_jiffies;
> +			if (hdelta > runtime->hw_ptr_buffer_jiffies/2) {
> +				hw_base += runtime->buffer_size;
> +				if (hw_base >= runtime->boundary)
> +					hw_base = 0;
> +				new_hw_ptr = hw_base + pos;
> +				goto __delta;
> +			}
>   		}
>   	}
>   	/* new_hw_ptr might be lower than old_hw_ptr in case when */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index a3b2a64..b5dae26 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -864,6 +864,8 @@ static void snd_pcm_post_start(struct snd_pcm_substream *substream, int state)
>   	struct snd_pcm_runtime *runtime = substream->runtime;
>   	snd_pcm_trigger_tstamp(substream);
>   	runtime->hw_ptr_jiffies = jiffies;
> +	runtime->hw_ptr_buffer_jiffies = (runtime->buffer_size * HZ) / 
> +							    runtime->rate;
>   	runtime->status->state = state;
>   	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
>   	    runtime->silence_size > 0)
> 
> -----
> Jaroslav Kysela <perex at perex.cz>
> Linux Kernel Sound Maintainer
> ALSA Project, Red Hat, Inc.
>