[alsa-devel] [PATCH 0/2] Fix usb-audio delay calculation
Hi,
the following two patches are for fixing the wrong PCM delay reported from usb-audio driver. The first one is trivial, just to fix / optimize for the capture direction. The second one is a deeper fix for corner cases like playback pause.
Takashi
It doesn't make sense to calculate the delay for capture streams in the current implementation. It's always zero, so we should skip the computation in snd_usb_pcm_pointer() in the case of capture.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/pcm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index e919c2e..8e1d5e0 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -75,7 +75,8 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream return SNDRV_PCM_POS_XRUN; spin_lock(&subs->lock); hwptr_done = subs->hwptr_done; - substream->runtime->delay = snd_usb_pcm_delay(subs, + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + substream->runtime->delay = snd_usb_pcm_delay(subs, substream->runtime->rate); spin_unlock(&subs->lock); return hwptr_done / (substream->runtime->frame_bits >> 3);
On 11/23/2012 09:21 AM, Takashi Iwai wrote:
It doesn't make sense to calculate the delay for capture streams in the current implementation. It's always zero, so we should skip the computation in snd_usb_pcm_pointer() in the case of capture.
Shouldn't we add support for delay on the capture path instead? We could use the same frame counter to report the delay and resync when the urb is retired. -Pierre
At Mon, 26 Nov 2012 08:24:16 -0600, Pierre-Louis Bossart wrote:
On 11/23/2012 09:21 AM, Takashi Iwai wrote:
It doesn't make sense to calculate the delay for capture streams in the current implementation. It's always zero, so we should skip the computation in snd_usb_pcm_pointer() in the case of capture.
Shouldn't we add support for delay on the capture path instead? We could use the same frame counter to report the delay and resync when the urb is retired.
Sure, it must be possible like that way. Wouldn't you like to volunteer? ;)
Takashi
It doesn't make sense to calculate the delay for capture streams in the current implementation. It's always zero, so we should skip the computation in snd_usb_pcm_pointer() in the case of capture.
Shouldn't we add support for delay on the capture path instead? We could use the same frame counter to report the delay and resync when the urb is retired.
Sure, it must be possible like that way. Wouldn't you like to volunteer? ;)
Sounds like a good skunkworks for the end of the year...If we have both HDA and USB behaving correctly on delays, this would raise the bar for everyone.
Shouldn't we add support for delay on the capture path instead? We could use the same frame counter to report the delay and resync when the urb is retired.
Sure, it must be possible like that way. Wouldn't you like to volunteer? ;)
here's a suggestion. Since we don't submit multiple URBs as in the playback case, we can only track the delay using the frame counter for a single URB, and we reset the counters when the URB is retired. Did a couple of tests, seems to work ok. I couldn't figure out if the related patches are already in git, I didn't find them. Takashi, did you merge them already? -Pierre
At Mon, 10 Dec 2012 15:33:56 -0600, Pierre-Louis Bossart wrote:
Shouldn't we add support for delay on the capture path instead? We could use the same frame counter to report the delay and resync when the urb is retired.
Sure, it must be possible like that way. Wouldn't you like to volunteer? ;)
here's a suggestion. Since we don't submit multiple URBs as in the playback case, we can only track the delay using the frame counter for a single URB, and we reset the counters when the URB is retired. Did a couple of tests, seems to work ok.
Looks good, but I'd like to merge this after 3.8 merge window. It's no urgent fix, so better to have a long test for 3.9.
I couldn't figure out if the related patches are already in git, I didn't find them. Takashi, did you merge them already?
Yes, these have been already queued for 3.8. See for-next branch of sound git tree.
thanks,
Takashi
-Pierre
[2 usb_delay.patch <text/x-patch (7bit)>] diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index c659310..65dc53e 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -59,7 +59,12 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs,
/* Approximation based on number of samples per USB frame (ms), some truncation for 44.1 but the estimate is good enough */
- est_delay = subs->last_delay - (frame_diff * rate / 1000);
- est_delay = frame_diff * rate / 1000;
- if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
est_delay = subs->last_delay - est_delay;- else
est_delay = subs->last_delay + est_delay;- if (est_delay < 0) est_delay = 0; return est_delay;
@@ -1147,6 +1152,10 @@ static void retire_capture_urb(struct snd_usb_substream *subs, int i, period_elapsed = 0; unsigned long flags; unsigned char *cp;
int current_frame_number;
/* read frame number here, update pointer in critical section */
current_frame_number = usb_get_current_frame_number(subs->dev);
stride = runtime->frame_bits >> 3;
@@ -1180,6 +1189,13 @@ static void retire_capture_urb(struct snd_usb_substream *subs, subs->transfer_done -= runtime->period_size; period_elapsed = 1; }
/* capture delay is by construction limited to one URB, reset delays here */runtime->delay = subs->last_delay = 0;/* realign last_frame_number */subs->last_frame_number = current_frame_number;subs->last_frame_number &= 0xFF; /* keep 8 LSBs */- spin_unlock_irqrestore(&subs->lock, flags); /* copy a data chunk */ if (oldptr + bytes > runtime->buffer_size * stride) {
When a playback stream is paused, the stream isn't actually stopped, thus we still need to take care of the in-flight data amount for the delay calculation. Otherwise the value of subs->last_delay is no longer reliable and can give a bogus value after resuming from pause. This will result in "delay: estimated XX, actual YY" error messages.
Also, during pause after all in flight data are processed (i.e. last_delay = 0), we don't have to calculate the actual delay from the current frame. Give a short path in such a case.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/pcm.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 8e1d5e0..7c64b95 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -46,6 +46,9 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs, int frame_diff; int est_delay;
+ if (!subs->last_delay) + return 0; /* short path */ + current_frame_number = usb_get_current_frame_number(subs->dev); /* * HCD implementations use different widths, use lower 8 bits. @@ -1195,6 +1198,9 @@ static void retire_playback_urb(struct snd_usb_substream *subs, return;
spin_lock_irqsave(&subs->lock, flags); + if (!subs->last_delay) + goto out; /* short path */ + est_delay = snd_usb_pcm_delay(subs, runtime->rate); /* update delay with exact number of samples played */ if (processed > subs->last_delay) @@ -1212,6 +1218,15 @@ static void retire_playback_urb(struct snd_usb_substream *subs, snd_printk(KERN_DEBUG "delay: estimated %d, actual %d\n", est_delay, subs->last_delay);
+ if (!subs->running) { + /* update last_frame_number for delay counting here since + * prepare_playback_urb won't be called during pause + */ + subs->last_frame_number = + usb_get_current_frame_number(subs->dev) & 0xff; + } + + out: spin_unlock_irqrestore(&subs->lock, flags); }
@@ -1253,7 +1268,8 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea return 0; case SNDRV_PCM_TRIGGER_PAUSE_PUSH: subs->data_endpoint->prepare_data_urb = NULL; - subs->data_endpoint->retire_data_urb = NULL; + /* keep retire_data_urb for delay calculation */ + subs->data_endpoint->retire_data_urb = retire_playback_urb; subs->running = 0; return 0; }
participants (2)
-
Pierre-Louis Bossart -
Takashi Iwai