[PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only

[PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only

[John S Gruber]

Addressing audio quality problem.

In sound/usb/usbaudio.c, for the Hauppage HVR-950Q only, change
retire_capture_urb to copy the entire byte stream while still counting
entire audio frames. urbs unaligned on channel sample boundaries are
still truncated to the next lowest stride (audio slot) size to try to
retain channel alignment in cases of data loss over usb.

With the HVR950Q the left and right channel samples can be split between
two different urbs. Throwing away extra channel samples causes a sound
quality problem for stereo streams as the left and right channels are
swapped repeatedly.

BugLink: https://bugs.launchpad.net/ubuntu/+bug/495745

Signed-off-by: John S. Gruber <JohnSGruber@gmail.com>
---
 usb/usbaudio.c |   53 ++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/usb/usbaudio.c b/usb/usbaudio.c
index b074a59..d450b23 100644
--- a/usb/usbaudio.c
+++ b/usb/usbaudio.c
@@ -108,6 +108,7 @@ MODULE_PARM_DESC(ignore_ctl_error,
 #define MAX_URBS	8
 #define SYNC_URBS	4	/* always four urbs for sync */
 #define MAX_QUEUE	24	/* try not to exceed this queue length, in ms */
+#define ALLOW_SUBSLOT_BOUNDARIES 0x01	/* quirk */

 struct audioformat {
 	struct list_head list;
@@ -127,6 +128,7 @@ struct audioformat {
 	unsigned int rate_min, rate_max;	/* min/max rates */
 	unsigned int nr_rates;		/* number of rate table entries */
 	unsigned int *rate_table;	/* rate table */
+	unsigned int txfr_quirks;	/* transfer quirks */
 };

 struct snd_usb_substream;
@@ -175,6 +177,8 @@ struct snd_usb_substream {
 	unsigned int running: 1;	/* running status */

 	unsigned int hwptr_done;			/* processed frame position in the buffer */
+	unsigned int byteptr;		/* position, in bytes, of next move */
+	unsigned int txfr_quirks;		/* substream transfer quirks */
 	unsigned int transfer_done;		/* processed frames since last period update */
 	unsigned long active_mask;	/* bitmask of active urbs */
 	unsigned long unlink_mask;	/* bitmask of unlinked urbs */
@@ -343,7 +347,7 @@ static int retire_capture_urb(struct
snd_usb_substream *subs,
 	unsigned long flags;
 	unsigned char *cp;
 	int i;
-	unsigned int stride, len, oldptr;
+	unsigned int stride, len, bytelen, oldbyteptr;
 	int period_elapsed = 0;

 	stride = runtime->frame_bits >> 3;
@@ -354,29 +358,39 @@ static int retire_capture_urb(struct
snd_usb_substream *subs,
 			snd_printd(KERN_ERR "frame %d active: %d\n", i,
urb->iso_frame_desc[i].status);
 			// continue;
 		}
-		len = urb->iso_frame_desc[i].actual_length / stride;
-		if (! len)
+		bytelen = (urb->iso_frame_desc[i].actual_length);
+		if (!bytelen)
 			continue;
+		if (bytelen%(runtime->sample_bits>>3) != 0) {
+			int oldbytelen = bytelen;
+			bytelen = ((bytelen/stride)*stride);
+			snd_printd(KERN_ERR "Corrected urb data len. %d->%d\n",
+				oldbytelen, bytelen);
+		}
+		if (!(subs->txfr_quirks & ALLOW_SUBSLOT_BOUNDARIES))
+			bytelen = (bytelen/stride)*stride;
 		/* update the current pointer */
 		spin_lock_irqsave(&subs->lock, flags);
-		oldptr = subs->hwptr_done;
-		subs->hwptr_done += len;
-		if (subs->hwptr_done >= runtime->buffer_size)
-			subs->hwptr_done -= runtime->buffer_size;
+		len = (bytelen + (subs->byteptr % stride)) / stride;
 		subs->transfer_done += len;
 		if (subs->transfer_done >= runtime->period_size) {
 			subs->transfer_done -= runtime->period_size;
 			period_elapsed = 1;
 		}
+		oldbyteptr = subs->byteptr;
+		subs->byteptr += bytelen;
+		if (subs->byteptr >= runtime->buffer_size*stride)
+			subs->byteptr -= runtime->buffer_size*stride;
+		subs->hwptr_done = subs->byteptr/stride;
 		spin_unlock_irqrestore(&subs->lock, flags);
 		/* copy a data chunk */
-		if (oldptr + len > runtime->buffer_size) {
-			unsigned int cnt = runtime->buffer_size - oldptr;
-			unsigned int blen = cnt * stride;
-			memcpy(runtime->dma_area + oldptr * stride, cp, blen);
-			memcpy(runtime->dma_area, cp + blen, len * stride - blen);
+		if ((oldbyteptr +  bytelen) > (runtime->buffer_size*stride)) {
+			unsigned int blen;
+			blen = (runtime->buffer_size*stride) - oldbyteptr;
+			memcpy(runtime->dma_area + oldbyteptr, cp, blen);
+			memcpy(runtime->dma_area, cp + blen,  bytelen - blen);
 		} else {
-			memcpy(runtime->dma_area + oldptr * stride, cp, len * stride);
+			memcpy(runtime->dma_area + oldbyteptr, cp, bytelen);
 		}
 	}
 	if (period_elapsed)
@@ -1363,6 +1377,7 @@ static int set_format(struct snd_usb_substream
*subs, struct audioformat *fmt)
 	subs->syncpipe = subs->syncinterval = 0;
 	subs->maxpacksize = fmt->maxpacksize;
 	subs->fill_max = 0;
+	subs->txfr_quirks = fmt->txfr_quirks;

 	/* we need a sync pipe in async OUT or adaptive IN mode */
 	/* check the number of EP, since some devices have broken
@@ -1532,6 +1547,7 @@ static int snd_usb_pcm_prepare(struct
snd_pcm_substream *substream)
 	/* reset the pointer */
 	subs->hwptr_done = 0;
 	subs->transfer_done = 0;
+	subs->byteptr = 0;
 	subs->phase = 0;
 	runtime->delay = 0;

@@ -2776,6 +2792,7 @@ static int parse_audio_endpoints(struct
snd_usb_audio *chip, int iface_no)
 			fp->maxpacksize = (((fp->maxpacksize >> 11) & 3) + 1)
 					* (fp->maxpacksize & 0x7ff);
 		fp->attributes = csep ? csep[3] : 0;
+		fp->txfr_quirks = 0;

 		/* some quirks for attributes here */

@@ -2804,6 +2821,16 @@ static int parse_audio_endpoints(struct
snd_usb_audio *chip, int iface_no)
 			else
 				fp->ep_attr |= EP_ATTR_SYNC;
 			break;
+		case USB_ID(0x2040, 0x7200): /* Hauppage hvr950Q */
+		case USB_ID(0x2040, 0x7221): /* Hauppage hvr950Q */
+		case USB_ID(0x2040, 0x7222): /* Hauppage hvr950Q */
+		case USB_ID(0x2040, 0x7223): /* Hauppage hvr950Q */
+		case USB_ID(0x2040, 0x7224): /* Hauppage hvr950Q */
+		case USB_ID(0x2040, 0x7225): /* Hauppage hvr950Q */
+		case USB_ID(0x2040, 0x7230): /* Hauppage hvr850 */
+		case USB_ID(0x2040, 0x7250): /* Hauppage hvr950Q */
+			fp->txfr_quirks |= ALLOW_SUBSLOT_BOUNDARIES;
+			break;
 		}

 		/* ok, let's parse further... */
-- 
1.6.3.3

I'm new to changing the kernel.

Normally the driver should transfer only whole audio frames,
disregarding fragments, in order to keep channel and sample
synchronization in the face of data loss on the bus. That's how the
current driver works.

The patch relaxes this (only) for devices with the quirk.

An earlier version of this patch was posted recently to the
linux-media list. No one there suggested a way to make the au0828 chip
in this equipment behave better. They kindly forwarded me to this
mailing list for review and consideration of the patch.

To test the patch with a non-quirk device run it on a busy or
problematic bus and listen for audio switching channels. To test with
a HVR-950Q use composite input and disconnect either the left or right
channel. Without the patch the audio will alternate between channels
quickly, most easily heard with headphones or earphones, producing a
flutter sound, With the patch the remaining test audio channel appears
only on one channel and is steady.

Re: [PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only

[Takashi Iwai]

At Sun, 13 Dec 2009 19:51:01 -0500,
John S Gruber wrote:
> 
> Addressing audio quality problem.
> 
> In sound/usb/usbaudio.c, for the Hauppage HVR-950Q only, change
> retire_capture_urb to copy the entire byte stream while still counting
> entire audio frames. urbs unaligned on channel sample boundaries are
> still truncated to the next lowest stride (audio slot) size to try to
> retain channel alignment in cases of data loss over usb.
> 
> With the HVR950Q the left and right channel samples can be split between
> two different urbs. Throwing away extra channel samples causes a sound
> quality problem for stereo streams as the left and right channels are
> swapped repeatedly.

Through a quick glance, the patch looks good and safe to apply.

Just a few comments below.

> diff --git a/usb/usbaudio.c b/usb/usbaudio.c
> index b074a59..d450b23 100644
> --- a/usb/usbaudio.c
> +++ b/usb/usbaudio.c
> @@ -108,6 +108,7 @@ MODULE_PARM_DESC(ignore_ctl_error,
>  #define MAX_URBS	8
>  #define SYNC_URBS	4	/* always four urbs for sync */
>  #define MAX_QUEUE	24	/* try not to exceed this queue length, in ms */
> +#define ALLOW_SUBSLOT_BOUNDARIES 0x01	/* quirk */

If it's just a boolean flag, we don't need to use bits.
Simply use a char flag or a bit-field flag instead of bit-and operation.


>  struct audioformat {
>  	struct list_head list;
> @@ -127,6 +128,7 @@ struct audioformat {
>  	unsigned int rate_min, rate_max;	/* min/max rates */
>  	unsigned int nr_rates;		/* number of rate table entries */
>  	unsigned int *rate_table;	/* rate table */
> +	unsigned int txfr_quirks;	/* transfer quirks */

I'm not sure whether this flag should belong to audiofmt.
Isn't it rather controller-wide?  If so, it can be better fit to another,
more global struct.

The rest are just trivial coding-style issues.  For example...

> @@ -354,29 +358,39 @@ static int retire_capture_urb(struct
> snd_usb_substream *subs,
>  			snd_printd(KERN_ERR "frame %d active: %d\n", i,
> urb->iso_frame_desc[i].status);
>  			// continue;
>  		}
> -		len = urb->iso_frame_desc[i].actual_length / stride;
> -		if (! len)
> +		bytelen = (urb->iso_frame_desc[i].actual_length);

No need of parentheses.

> +		if (!bytelen)
>  			continue;
> +		if (bytelen%(runtime->sample_bits>>3) != 0) {

Please put spaces around operators.

> +			int oldbytelen = bytelen;
> +			bytelen = ((bytelen/stride)*stride);

Ditto.  Try to run scripts/checkpatch.pl once and fix the complains
there.

> +			snd_printd(KERN_ERR "Corrected urb data len. %d->%d\n",
> +				oldbytelen, bytelen);

Hm, how often would it be printed?  If it's really verbose, use
snd_printdd() instead.


thanks,

Takashi

Re: [PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only

[Clemens Ladisch]

John S Gruber wrote:
> In sound/usb/usbaudio.c, for the Hauppage HVR-950Q only, change
> retire_capture_urb to copy the entire byte stream while still counting
> entire audio frames.

Now we have two pointers, one for frames and one for bytes.  Since the
first can be computed from the second, it doesn't make much sense to
keep both around.

The patch below modifies the driver to use a byte-based buffer pointer.
Please rewrite your patch to apply on top on that patch.

> urbs unaligned on channel sample boundaries are
> still truncated to the next lowest stride (audio slot) size to try to
> retain channel alignment in cases of data loss over usb.

USB packets are checksummed, so in case of data corruption, the entire
packets would be dropped by the controller, so you'll never get partial
samples.

With your device, one lost packet could potentially corrupt the entire
following stream.  There is no good error handling strategy for that.


Best regards,
Clemens

==========

sound: usb-audio: make buffer pointer based on bytes instead on frames

Since there are devices that do not align the size of their data packets
to frame boundaries, the driver needs to be able to keep track of
partial frames.  This patch prepares for support for such devices by
changing the hwptr_done variable from a frame counter to a byte counter.

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>

--- linux/sound/usb/usbaudio.c
+++ linux/sound/usb/usbaudio.c
@@ -173,7 +173,7 @@ struct snd_usb_substream {
 
 	unsigned int running: 1;	/* running status */
 
-	unsigned int hwptr_done;			/* processed frame position in the buffer */
+	unsigned int hwptr_done;	/* processed byte position in the buffer */
 	unsigned int transfer_done;		/* processed frames since last period update */
 	unsigned long active_mask;	/* bitmask of active urbs */
 	unsigned long unlink_mask;	/* bitmask of unlinked urbs */
@@ -342,7 +342,7 @@ static int retire_capture_urb(struct snd
 	unsigned long flags;
 	unsigned char *cp;
 	int i;
-	unsigned int stride, len, oldptr;
+	unsigned int stride, frames, bytes, oldptr;
 	int period_elapsed = 0;
 
 	stride = runtime->frame_bits >> 3;
@@ -353,29 +353,28 @@ static int retire_capture_urb(struct snd
 			snd_printd(KERN_ERR "frame %d active: %d\n", i, urb->iso_frame_desc[i].status);
 			// continue;
 		}
-		len = urb->iso_frame_desc[i].actual_length / stride;
-		if (! len)
-			continue;
+		frames = urb->iso_frame_desc[i].actual_length / stride;
+		bytes = frames * stride;
 		/* update the current pointer */
 		spin_lock_irqsave(&subs->lock, flags);
 		oldptr = subs->hwptr_done;
-		subs->hwptr_done += len;
-		if (subs->hwptr_done >= runtime->buffer_size)
-			subs->hwptr_done -= runtime->buffer_size;
-		subs->transfer_done += len;
+		subs->hwptr_done += bytes;
+		if (subs->hwptr_done >= runtime->buffer_size * stride)
+			subs->hwptr_done -= runtime->buffer_size * stride;
+		subs->transfer_done += frames;
 		if (subs->transfer_done >= runtime->period_size) {
 			subs->transfer_done -= runtime->period_size;
 			period_elapsed = 1;
 		}
 		spin_unlock_irqrestore(&subs->lock, flags);
 		/* copy a data chunk */
-		if (oldptr + len > runtime->buffer_size) {
-			unsigned int cnt = runtime->buffer_size - oldptr;
-			unsigned int blen = cnt * stride;
-			memcpy(runtime->dma_area + oldptr * stride, cp, blen);
-			memcpy(runtime->dma_area, cp + blen, len * stride - blen);
+		if (oldptr + bytes > runtime->buffer_size * stride) {
+			unsigned int bytes1 =
+					runtime->buffer_size * stride - oldptr;
+			memcpy(runtime->dma_area + oldptr, cp, bytes1);
+			memcpy(runtime->dma_area, cp + bytes1, bytes - bytes1);
 		} else {
-			memcpy(runtime->dma_area + oldptr * stride, cp, len * stride);
+			memcpy(runtime->dma_area + oldptr, cp, bytes);
 		}
 	}
 	if (period_elapsed)
@@ -562,24 +561,24 @@ static int prepare_playback_urb(struct s
 				struct snd_pcm_runtime *runtime,
 				struct urb *urb)
 {
-	int i, stride, offs;
-	unsigned int counts;
+	int i, stride;
+	unsigned int counts, frames, bytes;
 	unsigned long flags;
 	int period_elapsed = 0;
 	struct snd_urb_ctx *ctx = urb->context;
 
 	stride = runtime->frame_bits >> 3;
 
-	offs = 0;
+	frames = 0;
 	urb->dev = ctx->subs->dev; /* we need to set this at each time */
 	urb->number_of_packets = 0;
 	spin_lock_irqsave(&subs->lock, flags);
 	for (i = 0; i < ctx->packets; i++) {
 		counts = snd_usb_audio_next_packet_size(subs);
 		/* set up descriptor */
-		urb->iso_frame_desc[i].offset = offs * stride;
+		urb->iso_frame_desc[i].offset = frames * stride;
 		urb->iso_frame_desc[i].length = counts * stride;
-		offs += counts;
+		frames += counts;
 		urb->number_of_packets++;
 		subs->transfer_done += counts;
 		if (subs->transfer_done >= runtime->period_size) {
@@ -589,7 +588,7 @@ static int prepare_playback_urb(struct s
 				if (subs->transfer_done > 0) {
 					/* FIXME: fill-max mode is not
 					 * supported yet */
-					offs -= subs->transfer_done;
+					frames -= subs->transfer_done;
 					counts -= subs->transfer_done;
 					urb->iso_frame_desc[i].length =
 						counts * stride;
@@ -599,7 +598,7 @@ static int prepare_playback_urb(struct s
 				if (i < ctx->packets) {
 					/* add a transfer delimiter */
 					urb->iso_frame_desc[i].offset =
-						offs * stride;
+						frames * stride;
 					urb->iso_frame_desc[i].length = 0;
 					urb->number_of_packets++;
 				}
@@ -609,26 +608,25 @@ static int prepare_playback_urb(struct s
 		if (period_elapsed) /* finish at the period boundary */
 			break;
 	}
-	if (subs->hwptr_done + offs > runtime->buffer_size) {
+	bytes = frames * stride;
+	if (subs->hwptr_done + bytes > runtime->buffer_size * stride) {
 		/* err, the transferred area goes over buffer boundary. */
-		unsigned int len = runtime->buffer_size - subs->hwptr_done;
+		unsigned int bytes1 =
+			runtime->buffer_size * stride - subs->hwptr_done;
 		memcpy(urb->transfer_buffer,
-		       runtime->dma_area + subs->hwptr_done * stride,
-		       len * stride);
-		memcpy(urb->transfer_buffer + len * stride,
-		       runtime->dma_area,
-		       (offs - len) * stride);
+		       runtime->dma_area + subs->hwptr_done, bytes1);
+		memcpy(urb->transfer_buffer + bytes1,
+		       runtime->dma_area, bytes - bytes1);
 	} else {
 		memcpy(urb->transfer_buffer,
-		       runtime->dma_area + subs->hwptr_done * stride,
-		       offs * stride);
+		       runtime->dma_area + subs->hwptr_done, bytes);
 	}
-	subs->hwptr_done += offs;
-	if (subs->hwptr_done >= runtime->buffer_size)
-		subs->hwptr_done -= runtime->buffer_size;
-	runtime->delay += offs;
+	subs->hwptr_done += bytes;
+	if (subs->hwptr_done >= runtime->buffer_size * stride)
+		subs->hwptr_done -= runtime->buffer_size * stride;
+	runtime->delay += frames;
 	spin_unlock_irqrestore(&subs->lock, flags);
-	urb->transfer_buffer_length = offs * stride;
+	urb->transfer_buffer_length = bytes;
 	if (period_elapsed)
 		snd_pcm_period_elapsed(subs->pcm_substream);
 	return 0;
@@ -901,18 +899,18 @@ static int wait_clear_urbs(struct snd_us
 
 
 /*
- * return the current pcm pointer.  just return the hwptr_done value.
+ * return the current pcm pointer.  just based on the hwptr_done value.
  */
 static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream)
 {
 	struct snd_usb_substream *subs;
-	snd_pcm_uframes_t hwptr_done;
+	unsigned int hwptr_done;
 	
 	subs = (struct snd_usb_substream *)substream->runtime->private_data;
 	spin_lock(&subs->lock);
 	hwptr_done = subs->hwptr_done;
 	spin_unlock(&subs->lock);
-	return hwptr_done;
+	return hwptr_done / (substream->runtime->frame_bits >> 3);
 }

Re: [PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only

[John S Gruber]

[PATCH 1/2] sound: usb-audio: relax urb data align. restriction
HVR-950Q and HVR-850 only

Addressing audio quality problem.

In sound/usb/usbaudio.c, for the Hauppage HVR-950Q and HVR-850 only, change
retire_capture_urb to allow transfers on audio sub-slot boundaries rather
than audio slots boundaries.

With these devices the left and right channel samples can be split between
two different urbs. Throwing away extra channel samples causes a sound
quality problem for stereo streams as the left and right channels are
swapped repeatedly, perhaps many times per second.

Urbs unaligned on sub-slot boundaries are still truncated to the next
lowest stride (audio slot) to retain synchronization on samples even
though left/right channel synchronization may be lost in this case.

Detect the quirk using a case statement in snd_usb_audio_probe.

BugLink: https://bugs.launchpad.net/ubuntu/+bug/495745

Signed-off-by: John S. Gruber <JohnSGruber@gmail.com>
---
Clemens:

Thank you for your patch. It certainly makes my patch much more straightforward.

>USB packets are checksummed, so in case of data corruption, the entire
>packets would be dropped by the controller, so you'll never get partial
>samples.

>With your device, one lost packet could potentially corrupt the entire
>following stream.  There is no good error handling strategy for that.

You are certainly correct that a dropped packet corrupting the entire
stream. The misaligned packets occur about 1/40 - 1/80 packets, so
that's the odds that a dropped frame will cause the left and right channels
to swap. Much better than 12-24 channel swaps per second, though.

WRT checksum errors, during my last test I saw that the urb's with an odd
byte length coincided with the debugging message produced a few lines up:

Dec 21 22:52:54 gruber-laptop kernel: [178695.608453] ALSA
usbaudio.c:361: frame 0 active: -71
Dec 21 22:52:54 gruber-laptop kernel: [178695.608470] ALSA
usbaudio.c:372: Corrected urb data len. 191->188
Dec 21 22:53:25 gruber-laptop kernel: [178726.396561] ALSA
usbaudio.c:361: frame 0 active: -71
Dec 21 22:53:25 gruber-laptop kernel: [178726.396576] ALSA
usbaudio.c:372: Corrected urb data len. 191->188

While every odd byte length coincided with a -71 packet status (EPROTO?),
there also were -71 packet status messages appearing alone. At other times
there were uncorrelated -18 (-EXDEV?) messages too.

In usb/usbaudio.c retire_capture_urb() I note that the continue statement
that would drop these packets right after the snd_printd() is commented out.

Takashi:

>> +#define ALLOW_SUBSLOT_BOUNDARIES 0x01        /* quirk */

>If it's just a boolean flag, we don't need to use bits.
>Simply use a char flag or a bit-field flag instead of bit-and operation.

>>  struct audioformat {
>>       struct list_head list;
>> @@ -127,6 +128,7 @@ struct audioformat {
>>       unsigned int rate_min, rate_max;        /* min/max rates */
>>       unsigned int nr_rates;          /* number of rate table entries */
>>       unsigned int *rate_table;       /* rate table */
>> +     unsigned int txfr_quirks;       /* transfer quirks */

>I'm not sure whether this flag should belong to audiofmt.
>Isn't it rather controller-wide?  If so, it can be better fit to another,
>more global struct.

I've switched to using bit-field flags instead and moved the format
occurrance of the flag to the more general snd_usb_audio structure.
The flag is copied to the substream structure as an optimization,
of course.

>The rest are just trivial coding-style issues.  For example...

I'm sorry about the style problems. After reworking the patch I've looked again
for style problems and hope that I've found and fixed everything, but I'm new
at Linux kernel changes. I've run the patches through a recent version
checkpatch.pl
and I've tried to check them more thoroughly by hand, too.

>> +                     snd_printd(KERN_ERR "Corrected urb data len. %d->%d\n",
>> +                             oldbytelen, bytelen);

>Hm, how often would it be printed?  If it's really verbose, use
>snd_printdd() instead.

Although the debugging message is produced rather rarely with my device,
I don't know for sure that it won't occur more often on the 850 or other
950Q types. I've changed the snd_printd() to a snd_printdd() at
your suggestion.

*********

I don't know whether a case statement or a quirk in usbquirk.h is more
maintainable. I've added a second patch in case usbquirk.h is preferable. It
uses an unusual positive return code to indicate to the probe code that
processing should continue as normal.

Please review or ignore the second patch as deemed best.

The first patch follows immediately. The prerequisite is Clemens'
patch sound: "usb-audio:
make buffer pointer based on bytes instead on frames".

Thanks for your time and help.

John
---
 usb/usbaudio.c |   28 ++++++++++++++++++++++++++--
 usb/usbaudio.h |    1 +
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/usb/usbaudio.c b/usb/usbaudio.c
index 6e93cb5..8618be2 100644
--- a/usb/usbaudio.c
+++ b/usb/usbaudio.c
@@ -170,6 +170,7 @@ struct snd_usb_substream {
 	unsigned int curpacksize;	/* current packet size in bytes (for capture) */
 	unsigned int curframesize;	/* current packet size in frames (for capture) */
 	unsigned int fill_max: 1;	/* fill max packet size always */
+	unsigned int txfr_quirk:1;	/* allow sub-frame alignment */
 	unsigned int fmt_type;		/* USB audio format type (1-3) */

 	unsigned int running: 1;	/* running status */
@@ -354,8 +355,16 @@ static int retire_capture_urb(struct
snd_usb_substream *subs,
 			snd_printd(KERN_ERR "frame %d active: %d\n", i,
urb->iso_frame_desc[i].status);
 			// continue;
 		}
-		frames = urb->iso_frame_desc[i].actual_length / stride;
-		bytes = frames * stride;
+		bytes = urb->iso_frame_desc[i].actual_length;
+		frames = bytes / stride;
+		if (!subs->txfr_quirk)
+			bytes = frames * stride;
+		if (bytes % (runtime->sample_bits >> 3) != 0) {
+			int oldbytes = bytes;
+			bytes = frames * stride;
+			snd_printdd(KERN_ERR "Corrected urb data len. %d->%d\n",
+							oldbytes, bytes);
+		}
 		/* update the current pointer */
 		spin_lock_irqsave(&subs->lock, flags);
 		oldptr = subs->hwptr_done;
@@ -2225,6 +2234,7 @@ static void init_substream(struct snd_usb_stream
*as, int stream, struct audiofo
 	subs->stream = as;
 	subs->direction = stream;
 	subs->dev = as->chip->dev;
+	subs->txfr_quirk = as->chip->txfr_quirk;
 	if (snd_usb_get_speed(subs->dev) == USB_SPEED_FULL) {
 		subs->ops = audio_urb_ops[stream];
 	} else {
@@ -3659,6 +3669,20 @@ static void *snd_usb_audio_probe(struct usb_device *dev,
 		}
 	}

+	switch (chip->usb_id) {
+	case USB_ID(0x2040, 0x7200): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7221): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7222): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7223): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7224): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7225): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7230): /* Hauppage hvr850 */
+	case USB_ID(0x2040, 0x7250): /* Hauppage hvr950Q */
+		chip->txfr_quirk = 1;
+		break;
+	default:
+		chip->txfr_quirk = 0;
+		}
 	err = 1; /* continue */
 	if (quirk && quirk->ifnum != QUIRK_NO_INTERFACE) {
 		/* need some special handlings */
diff --git a/usb/usbaudio.h b/usb/usbaudio.h
index 40ba811..f48742d 100644
--- a/usb/usbaudio.h
+++ b/usb/usbaudio.h
@@ -125,6 +125,7 @@ struct snd_usb_audio {
 	struct snd_card *card;
 	u32 usb_id;
 	int shutdown;
+	unsigned int txfr_quirk:1; /* Subframe boundaries on transfers */
 	int num_interfaces;
 	int num_suspended_intf;

-- 
1.6.3.3

Re: [PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only

[John S Gruber]

[PATCH 2/2] sound: usb-audio: relax urb data align. restriction
HVR-950Q and HVR-850 only

Detect the HVR-950Q HVR-850 urb data alignment quirk using usbquirk.h
rather than using a case statement in snd_usb_audio_probe.

Signed-off-by: John S. Gruber <JohnSGruber@gmail.com>
---
 usb/usbaudio.c  |   30 +++++++-------
 usb/usbaudio.h  |    1 +
 usb/usbquirks.h |  114 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+), 15 deletions(-)

diff --git a/usb/usbaudio.c b/usb/usbaudio.c
index 8618be2..1f93a47 100644
--- a/usb/usbaudio.c
+++ b/usb/usbaudio.c
@@ -3240,6 +3240,18 @@ static int ignore_interface_quirk(struct
snd_usb_audio *chip,
 	return 0;
 }

+/*
+ * Allow alignment on audio sub-slot (channel samples) rather than
+ * on audio slots (audio frames)
+ */
+static int create_align_transfer_quirk(struct snd_usb_audio *chip,
+				  struct usb_interface *iface,
+				  const struct snd_usb_audio_quirk *quirk)
+{
+	chip->txfr_quirk = 1;
+	return 1;	/* Continue with creating streams and mixer */
+}
+

 /*
  * boot quirks
@@ -3415,7 +3427,8 @@ static int snd_usb_create_quirk(struct
snd_usb_audio *chip,
 		[QUIRK_AUDIO_FIXED_ENDPOINT] = create_fixed_stream_quirk,
 		[QUIRK_AUDIO_EDIROL_UA1000] = create_ua1000_quirk,
 		[QUIRK_AUDIO_EDIROL_UA101] = create_ua101_quirk,
-		[QUIRK_AUDIO_EDIROL_UAXX] = create_uaxx_quirk
+		[QUIRK_AUDIO_EDIROL_UAXX] = create_uaxx_quirk,
+		[QUIRK_AUDIO_ALIGN_TRANSFER] = create_align_transfer_quirk
 	};

 	if (quirk->type < QUIRK_TYPE_COUNT) {
@@ -3669,20 +3682,7 @@ static void *snd_usb_audio_probe(struct usb_device *dev,
 		}
 	}

-	switch (chip->usb_id) {
-	case USB_ID(0x2040, 0x7200): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7221): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7222): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7223): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7224): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7225): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7230): /* Hauppage hvr850 */
-	case USB_ID(0x2040, 0x7250): /* Hauppage hvr950Q */
-		chip->txfr_quirk = 1;
-		break;
-	default:
-		chip->txfr_quirk = 0;
-		}
+	chip->txfr_quirk = 0;
 	err = 1; /* continue */
 	if (quirk && quirk->ifnum != QUIRK_NO_INTERFACE) {
 		/* need some special handlings */
diff --git a/usb/usbaudio.h b/usb/usbaudio.h
index f48742d..6aa26d1 100644
--- a/usb/usbaudio.h
+++ b/usb/usbaudio.h
@@ -162,6 +162,7 @@ enum quirk_type {
 	QUIRK_AUDIO_EDIROL_UA1000,
 	QUIRK_AUDIO_EDIROL_UA101,
 	QUIRK_AUDIO_EDIROL_UAXX,
+	QUIRK_AUDIO_ALIGN_TRANSFER,

 	QUIRK_TYPE_COUNT
 };
diff --git a/usb/usbquirks.h b/usb/usbquirks.h
index a892bda..e58f959 100644
--- a/usb/usbquirks.h
+++ b/usb/usbquirks.h
@@ -2105,6 +2105,120 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 	}
 },

+/* Hauppauge HVR-950Q and HVR-850 */
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7200),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7201),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7202),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7203),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7204),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7205),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7250),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7230),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-850",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+
 {
 	/*
 	 * Some USB MIDI devices don't have an audio control interface,
-- 
1.6.3.3

Re: [PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only

[Takashi Iwai]

At Wed, 23 Dec 2009 23:32:56 -0500,
John S Gruber wrote:
> 
> [PATCH 1/2] sound: usb-audio: relax urb data align. restriction
> HVR-950Q and HVR-850 only
> 
> Addressing audio quality problem.
> 
> In sound/usb/usbaudio.c, for the Hauppage HVR-950Q and HVR-850 only, change
> retire_capture_urb to allow transfers on audio sub-slot boundaries rather
> than audio slots boundaries.
> 
> With these devices the left and right channel samples can be split between
> two different urbs. Throwing away extra channel samples causes a sound
> quality problem for stereo streams as the left and right channels are
> swapped repeatedly, perhaps many times per second.
> 
> Urbs unaligned on sub-slot boundaries are still truncated to the next
> lowest stride (audio slot) to retain synchronization on samples even
> though left/right channel synchronization may be lost in this case.
> 
> Detect the quirk using a case statement in snd_usb_audio_probe.
> 
> BugLink: https://bugs.launchpad.net/ubuntu/+bug/495745
> 
> Signed-off-by: John S. Gruber <JohnSGruber@gmail.com>
> ---
> Clemens:
> 
> Thank you for your patch. It certainly makes my patch much more straightforward.
> 
> >USB packets are checksummed, so in case of data corruption, the entire
> >packets would be dropped by the controller, so you'll never get partial
> >samples.
> 
> >With your device, one lost packet could potentially corrupt the entire
> >following stream.  There is no good error handling strategy for that.
> 
> You are certainly correct that a dropped packet corrupting the entire
> stream. The misaligned packets occur about 1/40 - 1/80 packets, so
> that's the odds that a dropped frame will cause the left and right channels
> to swap. Much better than 12-24 channel swaps per second, though.
> 
> WRT checksum errors, during my last test I saw that the urb's with an odd
> byte length coincided with the debugging message produced a few lines up:
> 
> Dec 21 22:52:54 gruber-laptop kernel: [178695.608453] ALSA
> usbaudio.c:361: frame 0 active: -71
> Dec 21 22:52:54 gruber-laptop kernel: [178695.608470] ALSA
> usbaudio.c:372: Corrected urb data len. 191->188
> Dec 21 22:53:25 gruber-laptop kernel: [178726.396561] ALSA
> usbaudio.c:361: frame 0 active: -71
> Dec 21 22:53:25 gruber-laptop kernel: [178726.396576] ALSA
> usbaudio.c:372: Corrected urb data len. 191->188
> 
> While every odd byte length coincided with a -71 packet status (EPROTO?),
> there also were -71 packet status messages appearing alone. At other times
> there were uncorrelated -18 (-EXDEV?) messages too.
> 
> In usb/usbaudio.c retire_capture_urb() I note that the continue statement
> that would drop these packets right after the snd_printd() is commented out.
> 
> Takashi:
> 
> >> +#define ALLOW_SUBSLOT_BOUNDARIES 0x01        /* quirk */
> 
> >If it's just a boolean flag, we don't need to use bits.
> >Simply use a char flag or a bit-field flag instead of bit-and operation.
> 
> >>  struct audioformat {
> >>       struct list_head list;
> >> @@ -127,6 +128,7 @@ struct audioformat {
> >>       unsigned int rate_min, rate_max;        /* min/max rates */
> >>       unsigned int nr_rates;          /* number of rate table entries */
> >>       unsigned int *rate_table;       /* rate table */
> >> +     unsigned int txfr_quirks;       /* transfer quirks */
> 
> >I'm not sure whether this flag should belong to audiofmt.
> >Isn't it rather controller-wide?  If so, it can be better fit to another,
> >more global struct.
> 
> I've switched to using bit-field flags instead and moved the format
> occurrance of the flag to the more general snd_usb_audio structure.
> The flag is copied to the substream structure as an optimization,
> of course.
> 
> >The rest are just trivial coding-style issues.  For example...
> 
> I'm sorry about the style problems. After reworking the patch I've looked again
> for style problems and hope that I've found and fixed everything, but I'm new
> at Linux kernel changes. I've run the patches through a recent version
> checkpatch.pl
> and I've tried to check them more thoroughly by hand, too.
> 
> >> +                     snd_printd(KERN_ERR "Corrected urb data len. %d->%d\n",
> >> +                             oldbytelen, bytelen);
> 
> >Hm, how often would it be printed?  If it's really verbose, use
> >snd_printdd() instead.
> 
> Although the debugging message is produced rather rarely with my device,
> I don't know for sure that it won't occur more often on the 850 or other
> 950Q types. I've changed the snd_printd() to a snd_printdd() at
> your suggestion.
> 
> *********
> 
> I don't know whether a case statement or a quirk in usbquirk.h is more
> maintainable. I've added a second patch in case usbquirk.h is preferable. It
> uses an unusual positive return code to indicate to the probe code that
> processing should continue as normal.
> 
> Please review or ignore the second patch as deemed best.
> 
> The first patch follows immediately. The prerequisite is Clemens'
> patch sound: "usb-audio:
> make buffer pointer based on bytes instead on frames".

Thanks for your work.  It all looks good to me now, but it couldn't be
applied as is.  I guess your patch isn't based on the very latest
version.

Could you rebase it to the latest sound git tree
    git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git

Also, the embedded patch in your post was broken, maybe due to MUA.
If it's difficult to fix your MUA, just use an attachment.


thanks,

Takashi

[PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only

[johnsgruber]

Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git c1e3acc1131be209a27676d8f363cbabcf9e0c76

Refine frame counting

Sent with git send-email to avoid MUA line folding.

[PATCH 1/3] sound: usb-audio: make buffer pointer based on bytes instead on frames

[johnsgruber]

From: Clemens Ladisch <clemens@ladisch.de>

Since there are devices that do not align the size of their data packets
to frame boundaries, the driver needs to be able to keep track of
partial frames.  This patch prepares for support for such devices by
changing the hwptr_done variable from a frame counter to a byte counter.

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
---
 sound/usb/usbaudio.c |   76 ++++++++++++++++++++++++-------------------------
 1 files changed, 37 insertions(+), 39 deletions(-)

diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c
index 31b63ea..1ba8f41 100644
--- a/sound/usb/usbaudio.c
+++ b/sound/usb/usbaudio.c
@@ -173,7 +173,7 @@ struct snd_usb_substream {
 
 	unsigned int running: 1;	/* running status */
 
-	unsigned int hwptr_done;			/* processed frame position in the buffer */
+	unsigned int hwptr_done;	/* processed byte position in the buffer */
 	unsigned int transfer_done;		/* processed frames since last period update */
 	unsigned long active_mask;	/* bitmask of active urbs */
 	unsigned long unlink_mask;	/* bitmask of unlinked urbs */
@@ -342,7 +342,7 @@ static int retire_capture_urb(struct snd_usb_substream *subs,
 	unsigned long flags;
 	unsigned char *cp;
 	int i;
-	unsigned int stride, len, oldptr;
+	unsigned int stride, frames, bytes, oldptr;
 	int period_elapsed = 0;
 
 	stride = runtime->frame_bits >> 3;
@@ -353,29 +353,28 @@ static int retire_capture_urb(struct snd_usb_substream *subs,
 			snd_printd(KERN_ERR "frame %d active: %d\n", i, urb->iso_frame_desc[i].status);
 			// continue;
 		}
-		len = urb->iso_frame_desc[i].actual_length / stride;
-		if (! len)
-			continue;
+		frames = urb->iso_frame_desc[i].actual_length / stride;
+		bytes = frames * stride;
 		/* update the current pointer */
 		spin_lock_irqsave(&subs->lock, flags);
 		oldptr = subs->hwptr_done;
-		subs->hwptr_done += len;
-		if (subs->hwptr_done >= runtime->buffer_size)
-			subs->hwptr_done -= runtime->buffer_size;
-		subs->transfer_done += len;
+		subs->hwptr_done += bytes;
+		if (subs->hwptr_done >= runtime->buffer_size * stride)
+			subs->hwptr_done -= runtime->buffer_size * stride;
+		subs->transfer_done += frames;
 		if (subs->transfer_done >= runtime->period_size) {
 			subs->transfer_done -= runtime->period_size;
 			period_elapsed = 1;
 		}
 		spin_unlock_irqrestore(&subs->lock, flags);
 		/* copy a data chunk */
-		if (oldptr + len > runtime->buffer_size) {
-			unsigned int cnt = runtime->buffer_size - oldptr;
-			unsigned int blen = cnt * stride;
-			memcpy(runtime->dma_area + oldptr * stride, cp, blen);
-			memcpy(runtime->dma_area, cp + blen, len * stride - blen);
+		if (oldptr + bytes > runtime->buffer_size * stride) {
+			unsigned int bytes1 =
+					runtime->buffer_size * stride - oldptr;
+			memcpy(runtime->dma_area + oldptr, cp, bytes1);
+			memcpy(runtime->dma_area, cp + bytes1, bytes - bytes1);
 		} else {
-			memcpy(runtime->dma_area + oldptr * stride, cp, len * stride);
+			memcpy(runtime->dma_area + oldptr, cp, bytes);
 		}
 	}
 	if (period_elapsed)
@@ -562,24 +561,24 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
 				struct snd_pcm_runtime *runtime,
 				struct urb *urb)
 {
-	int i, stride, offs;
-	unsigned int counts;
+	int i, stride;
+	unsigned int counts, frames, bytes;
 	unsigned long flags;
 	int period_elapsed = 0;
 	struct snd_urb_ctx *ctx = urb->context;
 
 	stride = runtime->frame_bits >> 3;
 
-	offs = 0;
+	frames = 0;
 	urb->dev = ctx->subs->dev; /* we need to set this at each time */
 	urb->number_of_packets = 0;
 	spin_lock_irqsave(&subs->lock, flags);
 	for (i = 0; i < ctx->packets; i++) {
 		counts = snd_usb_audio_next_packet_size(subs);
 		/* set up descriptor */
-		urb->iso_frame_desc[i].offset = offs * stride;
+		urb->iso_frame_desc[i].offset = frames * stride;
 		urb->iso_frame_desc[i].length = counts * stride;
-		offs += counts;
+		frames += counts;
 		urb->number_of_packets++;
 		subs->transfer_done += counts;
 		if (subs->transfer_done >= runtime->period_size) {
@@ -589,7 +588,7 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
 				if (subs->transfer_done > 0) {
 					/* FIXME: fill-max mode is not
 					 * supported yet */
-					offs -= subs->transfer_done;
+					frames -= subs->transfer_done;
 					counts -= subs->transfer_done;
 					urb->iso_frame_desc[i].length =
 						counts * stride;
@@ -599,7 +598,7 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
 				if (i < ctx->packets) {
 					/* add a transfer delimiter */
 					urb->iso_frame_desc[i].offset =
-						offs * stride;
+						frames * stride;
 					urb->iso_frame_desc[i].length = 0;
 					urb->number_of_packets++;
 				}
@@ -609,26 +608,25 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
 		if (period_elapsed) /* finish at the period boundary */
 			break;
 	}
-	if (subs->hwptr_done + offs > runtime->buffer_size) {
+	bytes = frames * stride;
+	if (subs->hwptr_done + bytes > runtime->buffer_size * stride) {
 		/* err, the transferred area goes over buffer boundary. */
-		unsigned int len = runtime->buffer_size - subs->hwptr_done;
+		unsigned int bytes1 =
+			runtime->buffer_size * stride - subs->hwptr_done;
 		memcpy(urb->transfer_buffer,
-		       runtime->dma_area + subs->hwptr_done * stride,
-		       len * stride);
-		memcpy(urb->transfer_buffer + len * stride,
-		       runtime->dma_area,
-		       (offs - len) * stride);
+		       runtime->dma_area + subs->hwptr_done, bytes1);
+		memcpy(urb->transfer_buffer + bytes1,
+		       runtime->dma_area, bytes - bytes1);
 	} else {
 		memcpy(urb->transfer_buffer,
-		       runtime->dma_area + subs->hwptr_done * stride,
-		       offs * stride);
+		       runtime->dma_area + subs->hwptr_done, bytes);
 	}
-	subs->hwptr_done += offs;
-	if (subs->hwptr_done >= runtime->buffer_size)
-		subs->hwptr_done -= runtime->buffer_size;
-	runtime->delay += offs;
+	subs->hwptr_done += bytes;
+	if (subs->hwptr_done >= runtime->buffer_size * stride)
+		subs->hwptr_done -= runtime->buffer_size * stride;
+	runtime->delay += frames;
 	spin_unlock_irqrestore(&subs->lock, flags);
-	urb->transfer_buffer_length = offs * stride;
+	urb->transfer_buffer_length = bytes;
 	if (period_elapsed)
 		snd_pcm_period_elapsed(subs->pcm_substream);
 	return 0;
@@ -901,18 +899,18 @@ static int wait_clear_urbs(struct snd_usb_substream *subs)
 
 
 /*
- * return the current pcm pointer.  just return the hwptr_done value.
+ * return the current pcm pointer.  just based on the hwptr_done value.
  */
 static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream)
 {
 	struct snd_usb_substream *subs;
-	snd_pcm_uframes_t hwptr_done;
+	unsigned int hwptr_done;
 	
 	subs = (struct snd_usb_substream *)substream->runtime->private_data;
 	spin_lock(&subs->lock);
 	hwptr_done = subs->hwptr_done;
 	spin_unlock(&subs->lock);
-	return hwptr_done;
+	return hwptr_done / (substream->runtime->frame_bits >> 3);
 }
 
 
-- 
1.6.3.3

[PATCH 2/3] sound: usb-audio: relax urb data align. restriction HVR-950Q and HVR-850 only

[johnsgruber]

From: John S. Gruber <JohnSGruber@gmail.com>

Addressing audio quality problem.

In sound/usb/usbaudio.c, for the Hauppage HVR-950Q and HVR-850 only, change
retire_capture_urb to allow transfers on audio sub-slot boundaries rather
than audio slots boundaries.

With these devices the left and right channel samples can be split between
two different urbs. Throwing away extra channel samples causes a sound
quality problem for stereo streams as the left and right channels are
swapped repeatedly, perhaps many times per second.

Urbs unaligned on sub-slot boundaries are still truncated to the next
lowest stride (audio slot) to retain synchronization on samples even
though left/right channel synchronization may be lost in this case.

Detect the quirk using a case statement in snd_usb_audio_probe.

BugLink: https://bugs.launchpad.net/ubuntu/+bug/495745

Signed-off-by: John S. Gruber <JohnSGruber@gmail.com>
---
 sound/usb/usbaudio.c |   29 +++++++++++++++++++++++++++--
 sound/usb/usbaudio.h |    1 +
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c
index 1ba8f41..be6f017 100644
--- a/sound/usb/usbaudio.c
+++ b/sound/usb/usbaudio.c
@@ -169,6 +169,7 @@ struct snd_usb_substream {
 	unsigned int curpacksize;	/* current packet size in bytes (for capture) */
 	unsigned int curframesize;	/* current packet size in frames (for capture) */
 	unsigned int fill_max: 1;	/* fill max packet size always */
+	unsigned int txfr_quirk:1;	/* allow sub-frame alignment */
 	unsigned int fmt_type;		/* USB audio format type (1-3) */
 
 	unsigned int running: 1;	/* running status */
@@ -353,14 +354,23 @@ static int retire_capture_urb(struct snd_usb_substream *subs,
 			snd_printd(KERN_ERR "frame %d active: %d\n", i, urb->iso_frame_desc[i].status);
 			// continue;
 		}
-		frames = urb->iso_frame_desc[i].actual_length / stride;
-		bytes = frames * stride;
+		bytes = urb->iso_frame_desc[i].actual_length;
+		frames = bytes / stride;
+		if (!subs->txfr_quirk)
+			bytes = frames * stride;
+		if (bytes % (runtime->sample_bits >> 3) != 0) {
+			int oldbytes = bytes;
+			bytes = frames * stride;
+			snd_printdd(KERN_ERR "Corrected urb data len. %d->%d\n",
+							oldbytes, bytes);
+		}
 		/* update the current pointer */
 		spin_lock_irqsave(&subs->lock, flags);
 		oldptr = subs->hwptr_done;
 		subs->hwptr_done += bytes;
 		if (subs->hwptr_done >= runtime->buffer_size * stride)
 			subs->hwptr_done -= runtime->buffer_size * stride;
+		frames = (bytes + (oldptr % stride)) / stride;
 		subs->transfer_done += frames;
 		if (subs->transfer_done >= runtime->period_size) {
 			subs->transfer_done -= runtime->period_size;
@@ -2189,6 +2199,7 @@ static void init_substream(struct snd_usb_stream *as, int stream, struct audiofo
 	subs->stream = as;
 	subs->direction = stream;
 	subs->dev = as->chip->dev;
+	subs->txfr_quirk = as->chip->txfr_quirk;
 	if (snd_usb_get_speed(subs->dev) == USB_SPEED_FULL) {
 		subs->ops = audio_urb_ops[stream];
 	} else {
@@ -3569,6 +3580,20 @@ static void *snd_usb_audio_probe(struct usb_device *dev,
 		}
 	}
 
+	switch (chip->usb_id) {
+	case USB_ID(0x2040, 0x7200): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7221): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7222): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7223): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7224): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7225): /* Hauppage hvr950Q */
+	case USB_ID(0x2040, 0x7230): /* Hauppage hvr850 */
+	case USB_ID(0x2040, 0x7250): /* Hauppage hvr950Q */
+		chip->txfr_quirk = 1;
+		break;
+	default:
+		chip->txfr_quirk = 0;
+		}
 	err = 1; /* continue */
 	if (quirk && quirk->ifnum != QUIRK_NO_INTERFACE) {
 		/* need some special handlings */
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index 9826337..228da06 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -125,6 +125,7 @@ struct snd_usb_audio {
 	struct snd_card *card;
 	u32 usb_id;
 	int shutdown;
+	unsigned int txfr_quirk:1; /* Subframe boundaries on transfers */
 	int num_interfaces;
 	int num_suspended_intf;
 
-- 
1.6.3.3

[PATCH 3/3] sound: usb-audio: relax urb data align. restriction HVR-950Q and HVR-850 only

[johnsgruber]

From: John S. Gruber <JohnSGruber@gmail.com>

Detect the HVR-950Q HVR-850 urb data alignment quirk using usbquirk.h
rather than using a case statement in snd_usb_audio_probe.

Signed-off-by: John S. Gruber <JohnSGruber@gmail.com>
---
 sound/usb/usbaudio.c  |   30 ++++++------
 sound/usb/usbaudio.h  |    1 +
 sound/usb/usbquirks.h |  114 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+), 15 deletions(-)

diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c
index be6f017..0cfff95 100644
--- a/sound/usb/usbaudio.c
+++ b/sound/usb/usbaudio.c
@@ -3152,6 +3152,18 @@ static int ignore_interface_quirk(struct snd_usb_audio *chip,
 	return 0;
 }
 
+/*
+ * Allow alignment on audio sub-slot (channel samples) rather than
+ * on audio slots (audio frames)
+ */
+static int create_align_transfer_quirk(struct snd_usb_audio *chip,
+				  struct usb_interface *iface,
+				  const struct snd_usb_audio_quirk *quirk)
+{
+	chip->txfr_quirk = 1;
+	return 1;	/* Continue with creating streams and mixer */
+}
+
 
 /*
  * boot quirks
@@ -3326,7 +3338,8 @@ static int snd_usb_create_quirk(struct snd_usb_audio *chip,
 		[QUIRK_AUDIO_STANDARD_INTERFACE] = create_standard_audio_quirk,
 		[QUIRK_AUDIO_FIXED_ENDPOINT] = create_fixed_stream_quirk,
 		[QUIRK_AUDIO_EDIROL_UA1000] = create_ua1000_quirk,
-		[QUIRK_AUDIO_EDIROL_UAXX] = create_uaxx_quirk
+		[QUIRK_AUDIO_EDIROL_UAXX] = create_uaxx_quirk,
+		[QUIRK_AUDIO_ALIGN_TRANSFER] = create_align_transfer_quirk
 	};
 
 	if (quirk->type < QUIRK_TYPE_COUNT) {
@@ -3580,20 +3593,7 @@ static void *snd_usb_audio_probe(struct usb_device *dev,
 		}
 	}
 
-	switch (chip->usb_id) {
-	case USB_ID(0x2040, 0x7200): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7221): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7222): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7223): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7224): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7225): /* Hauppage hvr950Q */
-	case USB_ID(0x2040, 0x7230): /* Hauppage hvr850 */
-	case USB_ID(0x2040, 0x7250): /* Hauppage hvr950Q */
-		chip->txfr_quirk = 1;
-		break;
-	default:
-		chip->txfr_quirk = 0;
-		}
+	chip->txfr_quirk = 0;
 	err = 1; /* continue */
 	if (quirk && quirk->ifnum != QUIRK_NO_INTERFACE) {
 		/* need some special handlings */
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index 228da06..9c88f60 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -161,6 +161,7 @@ enum quirk_type {
 	QUIRK_AUDIO_FIXED_ENDPOINT,
 	QUIRK_AUDIO_EDIROL_UA1000,
 	QUIRK_AUDIO_EDIROL_UAXX,
+	QUIRK_AUDIO_ALIGN_TRANSFER,
 
 	QUIRK_TYPE_COUNT
 };
diff --git a/sound/usb/usbquirks.h b/sound/usb/usbquirks.h
index bd6706c..65bbd22 100644
--- a/sound/usb/usbquirks.h
+++ b/sound/usb/usbquirks.h
@@ -2074,6 +2074,120 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 	}
 },
 
+/* Hauppauge HVR-950Q and HVR-850 */
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7200),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7201),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7202),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7203),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7204),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7205),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7250),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-950Q",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+{
+	USB_DEVICE_VENDOR_SPEC(0x2040, 0x7230),
+	.match_flags = USB_DEVICE_ID_MATCH_DEVICE |
+		       USB_DEVICE_ID_MATCH_INT_CLASS |
+		       USB_DEVICE_ID_MATCH_INT_SUBCLASS,
+	.bInterfaceClass = USB_CLASS_AUDIO,
+	.bInterfaceSubClass = USB_SUBCLASS_AUDIO_CONTROL,
+	.driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+		.vendor_name = "Hauppauge",
+		.product_name = "HVR-850",
+		.ifnum = QUIRK_ANY_INTERFACE,
+		.type = QUIRK_AUDIO_ALIGN_TRANSFER,
+	}
+},
+
 {
 	/*
 	 * Some USB MIDI devices don't have an audio control interface,
-- 
1.6.3.3

Re: [PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only

[Takashi Iwai]

At Sun, 27 Dec 2009 12:19:56 -0500,
johnsgruber@gmail.com wrote:
> 
> Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git c1e3acc1131be209a27676d8f363cbabcf9e0c76
> 
> Refine frame counting
> 
> Sent with git send-email to avoid MUA line folding.

Thanks.  Applied all three patches now (with a minor fix for a compile
warning).


Takashi

Re: [PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only

[Devin Heitmueller]

On Mon, Dec 28, 2009 at 6:34 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Sun, 27 Dec 2009 12:19:56 -0500,
> johnsgruber@gmail.com wrote:
>>
>> Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git c1e3acc1131be209a27676d8f363cbabcf9e0c76
>>
>> Refine frame counting
>>
>> Sent with git send-email to avoid MUA line folding.
>
> Thanks.  Applied all three patches now (with a minor fix for a compile
> warning).

John,

Thanks for taking the time to work through the issues and getting this
into the mainline.  I'm sure it will help out users quite a bit.

Cheers,

Devin Heitmueller
(the resident HVR-950q maintainer)

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com