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

Takashi Iwai tiwai at suse.de
Fri Dec 25 14:31:44 CET 2009

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

Could you rebase it to the latest sound git tree

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.



More information about the Alsa-devel mailing list