[alsa-devel] [PATCH 0/3] ALSA: snd-usb: Regression fixes for 3.5
Here are 3 more regression fixes for snd-usb in 3.5. It's annoying they haven't been found earlier, but at least we're now back to the streaming quality we had before the streaming refactoring.
Anyone with a USB device, please give it a quick shot at least.
Thanks, Daniel
Daniel Mack (3): ALSA: snd-usb: restore delay information ALSA: snd-usb: fix calls to next_packet_size ALSA: snd-usb: fix cross-interface streaming devices
sound/usb/endpoint.c | 13 +------------ sound/usb/endpoint.h | 1 + sound/usb/pcm.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 49 insertions(+), 16 deletions(-)
Parts of commit 294c4fb8 ("ALSA: usb: refine delay information with USB frame counter") were unfortunately lost during the refactoring of the snd-usb driver in 3.5.
This patch adds them back, restoring the correct delay information behaviour.
Signed-off-by: Daniel Mack zonque@gmail.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: stable@kernel.org [3.5+] --- sound/usb/pcm.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 1546577..5ceb8f1 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -1091,7 +1091,16 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, subs->hwptr_done += bytes; if (subs->hwptr_done >= runtime->buffer_size * stride) subs->hwptr_done -= runtime->buffer_size * stride; + + /* update delay with exact number of samples queued */ + runtime->delay = subs->last_delay; runtime->delay += frames; + subs->last_delay = runtime->delay; + + /* realign last_frame_number */ + subs->last_frame_number = usb_get_current_frame_number(subs->dev); + subs->last_frame_number &= 0xFF; /* keep 8 LSBs */ + spin_unlock_irqrestore(&subs->lock, flags); urb->transfer_buffer_length = bytes; if (period_elapsed) @@ -1109,12 +1118,26 @@ static void retire_playback_urb(struct snd_usb_substream *subs, struct snd_pcm_runtime *runtime = subs->pcm_substream->runtime; int stride = runtime->frame_bits >> 3; int processed = urb->transfer_buffer_length / stride; + int est_delay;
spin_lock_irqsave(&subs->lock, flags); - if (processed > runtime->delay) - runtime->delay = 0; + est_delay = snd_usb_pcm_delay(subs, runtime->rate); + /* update delay with exact number of samples played */ + if (processed > subs->last_delay) + subs->last_delay = 0; else - runtime->delay -= processed; + subs->last_delay -= processed; + runtime->delay = subs->last_delay; + + /* + * Report when delay estimate is off by more than 2ms. + * The error should be lower than 2ms since the estimate relies + * on two reads of a counter updated every ms. + */ + if (abs(est_delay - subs->last_delay) * 1000 > runtime->rate * 2) + snd_printk(KERN_DEBUG "delay: estimated %d, actual %d\n", + est_delay, subs->last_delay); + spin_unlock_irqrestore(&subs->lock, flags); }
In order to support devices with implicit feedback streaming models, packet sizes are now stored with each individual urb, and the PCM handling code which fills the buffers purely relies on the size fields now.
However, calling snd_usb_audio_next_packet_size() for all possible packets in an URB at once, prior to letting the PCM code do its job does in fact not lead to the same behaviour than what the old code did: The PCM code will break its loop once a period boundary is reached, consequently using up less packets that it really could.
As snd_usb_audio_next_packet_size() implements a feedback mechanism to the endpoints phase accumulator, the number of calls to that function matters, and when called too often, the data rate runs out of bounds.
Fix this by making the next_packet function public, and call it from the PCM code as before if the packet data sizes are not defined.
Signed-off-by: Daniel Mack zonque@gmail.com Cc: stable@kernel.org [v3.5+] --- sound/usb/endpoint.c | 13 +------------ sound/usb/endpoint.h | 1 + sound/usb/pcm.c | 7 ++++++- 3 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index b896c55..d6e2bb4 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -141,7 +141,7 @@ int snd_usb_endpoint_implict_feedback_sink(struct snd_usb_endpoint *ep) * * For implicit feedback, next_packet_size() is unused. */ -static int next_packet_size(struct snd_usb_endpoint *ep) +int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep) { unsigned long flags; int ret; @@ -177,15 +177,6 @@ static void retire_inbound_urb(struct snd_usb_endpoint *ep, ep->retire_data_urb(ep->data_subs, urb); }
-static void prepare_outbound_urb_sizes(struct snd_usb_endpoint *ep, - struct snd_urb_ctx *ctx) -{ - int i; - - for (i = 0; i < ctx->packets; ++i) - ctx->packet_size[i] = next_packet_size(ep); -} - /* * Prepare a PLAYBACK urb for submission to the bus. */ @@ -370,7 +361,6 @@ static void snd_complete_urb(struct urb *urb) goto exit_clear; }
- prepare_outbound_urb_sizes(ep, ctx); prepare_outbound_urb(ep, ctx); } else { retire_inbound_urb(ep, ctx); @@ -857,7 +847,6 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, int can_sleep) goto __error;
if (usb_pipeout(ep->pipe)) { - prepare_outbound_urb_sizes(ep, urb->context); prepare_outbound_urb(ep, urb->context); } else { prepare_inbound_urb(ep, urb->context); diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h index a8e60c1..cbbbdf2 100644 --- a/sound/usb/endpoint.h +++ b/sound/usb/endpoint.h @@ -21,6 +21,7 @@ int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep); void snd_usb_endpoint_free(struct list_head *head);
int snd_usb_endpoint_implict_feedback_sink(struct snd_usb_endpoint *ep); +int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep);
void snd_usb_handle_sync_urb(struct snd_usb_endpoint *ep, struct snd_usb_endpoint *sender, diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 5ceb8f1..e80b668 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -1029,6 +1029,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, struct urb *urb) { struct snd_pcm_runtime *runtime = subs->pcm_substream->runtime; + struct snd_usb_endpoint *ep = subs->data_endpoint; struct snd_urb_ctx *ctx = urb->context; unsigned int counts, frames, bytes; int i, stride, period_elapsed = 0; @@ -1040,7 +1041,11 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, urb->number_of_packets = 0; spin_lock_irqsave(&subs->lock, flags); for (i = 0; i < ctx->packets; i++) { - counts = ctx->packet_size[i]; + if (ctx->packet_size[i]) + counts = ctx->packet_size[i]; + else + counts = snd_usb_endpoint_next_packet_size(ep); + /* set up descriptor */ urb->iso_frame_desc[i].offset = frames * stride; urb->iso_frame_desc[i].length = counts * stride;
Commit 68e67f40b ("ALSA: snd-usb: move calls to usb_set_interface") saved us some unnecessary calls to snd_usb_set_interface() but ignored the fact that there is at least one device out there which operates on two endpoint in different interfaces simultaniously.
Take care for this by catching the case where data and sync endpoints are located on different interfaces and calling snd_usb_set_interface() between the start of the two endpoints.
Signed-off-by: Daniel Mack zonque@gmail.com Reported-by: Robert M. Albrecht linux@romal.de --- sound/usb/pcm.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index e80b668..fd5e982 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -236,6 +236,21 @@ static int start_endpoints(struct snd_usb_substream *subs, int can_sleep) !test_and_set_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags)) { struct snd_usb_endpoint *ep = subs->sync_endpoint;
+ if (subs->data_endpoint->iface != subs->sync_endpoint->iface || + subs->data_endpoint->alt_idx != subs->sync_endpoint->alt_idx) { + err = usb_set_interface(subs->dev, + subs->sync_endpoint->iface, + subs->sync_endpoint->alt_idx); + if (err < 0) { + snd_printk(KERN_ERR + "%d:%d:%d: cannot set interface (%d)\n", + subs->dev->devnum, + subs->sync_endpoint->iface, + subs->sync_endpoint->alt_idx, err); + return -EIO; + } + } + snd_printdd(KERN_DEBUG "Starting sync EP @%p\n", ep);
ep->sync_slave = subs->data_endpoint;
On 30.08.2012 18:52, Daniel Mack wrote:
Commit 68e67f40b ("ALSA: snd-usb: move calls to usb_set_interface") saved us some unnecessary calls to snd_usb_set_interface() but ignored the fact that there is at least one device out there which operates on two endpoint in different interfaces simultaniously.
Take care for this by catching the case where data and sync endpoints are located on different interfaces and calling snd_usb_set_interface() between the start of the two endpoints.
Signed-off-by: Daniel Mack zonque@gmail.com Reported-by: Robert M. Albrecht linux@romal.de
Cc: stable@kernel.org [v3.5+]
sound/usb/pcm.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index e80b668..fd5e982 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -236,6 +236,21 @@ static int start_endpoints(struct snd_usb_substream *subs, int can_sleep) !test_and_set_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags)) { struct snd_usb_endpoint *ep = subs->sync_endpoint;
if (subs->data_endpoint->iface != subs->sync_endpoint->iface ||
subs->data_endpoint->alt_idx != subs->sync_endpoint->alt_idx) {
err = usb_set_interface(subs->dev,
subs->sync_endpoint->iface,
subs->sync_endpoint->alt_idx);
if (err < 0) {
snd_printk(KERN_ERR
"%d:%d:%d: cannot set interface (%d)\n",
subs->dev->devnum,
subs->sync_endpoint->iface,
subs->sync_endpoint->alt_idx, err);
return -EIO;
}
}
snd_printdd(KERN_DEBUG "Starting sync EP @%p\n", ep);
ep->sync_slave = subs->data_endpoint;
At Thu, 30 Aug 2012 18:52:28 +0200, Daniel Mack wrote:
Here are 3 more regression fixes for snd-usb in 3.5. It's annoying they haven't been found earlier, but at least we're now back to the streaming quality we had before the streaming refactoring.
Anyone with a USB device, please give it a quick shot at least.
I have no device right now during traveling, so ping me if you got a positive report or you tested by yourself. Then I'll merge and send the pull request in the next week.
thanks,
Takashi
Thanks, Daniel
Daniel Mack (3): ALSA: snd-usb: restore delay information ALSA: snd-usb: fix calls to next_packet_size ALSA: snd-usb: fix cross-interface streaming devices
sound/usb/endpoint.c | 13 +------------ sound/usb/endpoint.h | 1 + sound/usb/pcm.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 49 insertions(+), 16 deletions(-)
-- 1.7.11.4
On 31.08.2012 18:26, Takashi Iwai wrote:
At Thu, 30 Aug 2012 18:52:28 +0200, Daniel Mack wrote:
Here are 3 more regression fixes for snd-usb in 3.5. It's annoying they haven't been found earlier, but at least we're now back to the streaming quality we had before the streaming refactoring.
Anyone with a USB device, please give it a quick shot at least.
I have no device right now during traveling, so ping me if you got a positive report or you tested by yourself. Then I'll merge and send the pull request in the next week.
Well, I tested them extensively after I experienced the regressions when stress-testing my devices under PureData with very low latencies. I'm very sure all three fixes are correct. If you agree, just apply them. But we can as well wait until next week. Maybe people find more time for testing over the weekend :)
Thanks, Daniel
Hi,
Am 31.08.2012 18:30 schrieb "Daniel Mack" zonque@gmail.com:
But we can as well wait until next week. Maybe people find more time for testing over the weekend :)
I won't be able to test this patch set before Wednesday.
At Fri, 31 Aug 2012 18:30:03 +0200, Daniel Mack wrote:
On 31.08.2012 18:26, Takashi Iwai wrote:
At Thu, 30 Aug 2012 18:52:28 +0200, Daniel Mack wrote:
Here are 3 more regression fixes for snd-usb in 3.5. It's annoying they haven't been found earlier, but at least we're now back to the streaming quality we had before the streaming refactoring.
Anyone with a USB device, please give it a quick shot at least.
I have no device right now during traveling, so ping me if you got a positive report or you tested by yourself. Then I'll merge and send the pull request in the next week.
Well, I tested them extensively after I experienced the regressions when stress-testing my devices under PureData with very low latencies. I'm very sure all three fixes are correct. If you agree, just apply them. But we can as well wait until next week. Maybe people find more time for testing over the weekend :)
FYI, I already merged these into sound git tree as it worked well with my devices. I'm going to send a pull request including these fixes.
Takashi
participants (3)
-
Daniel Mack
-
Felix Homann
-
Takashi Iwai