[alsa-devel] [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 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
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