[PATCH 0/3] ALSA: snd-usb: Regression fixes for 3.5

[PATCH 0/3] ALSA: snd-usb: Regression fixes for 3.5

[Daniel Mack]

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(-)

-- 
1.7.11.4

[PATCH 1/3] ALSA: snd-usb: restore delay information

[Daniel Mack]

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);
 }
 
-- 
1.7.11.4

[PATCH 2/3] ALSA: snd-usb: fix calls to next_packet_size

[Daniel Mack]

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;
-- 
1.7.11.4

[PATCH 3/3] ALSA: snd-usb: fix cross-interface streaming devices

[Daniel Mack]

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;
-- 
1.7.11.4

Re: [PATCH 3/3] ALSA: snd-usb: fix cross-interface streaming devices

[Daniel Mack]

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;
> 

Re: [PATCH 0/3] ALSA: snd-usb: Regression fixes for 3.5

[Takashi Iwai]

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
> 

Re: [PATCH 0/3] ALSA: snd-usb: Regression fixes for 3.5

[Daniel Mack]

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

Re: [PATCH 0/3] ALSA: snd-usb: Regression fixes for 3.5

[Felix Homann]

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.

Re: [PATCH 0/3] ALSA: snd-usb: Regression fixes for 3.5

[Takashi Iwai]

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