[alsa-devel] [PATCH - usb/usbaudio.c 1/1] sound/usb: Relax urb data align. restriction for HVR-950Q and HVR-850 only
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... */
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
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); }
[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;
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
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.
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
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)
[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,
participants (5)
-
Clemens Ladisch -
Devin Heitmueller -
John S Gruber -
johnsgruber＠gmail.com
-
Takashi Iwai