Re: [PATCH] sound/usb: Relax urb data alignment restriciton for HVR-950Q only
On Thu, Dec 10, 2009 at 8:49 PM, Devin Heitmueller wrote: > Hello John, > > On Thu, Dec 10, 2009 at 8:38 PM, John S Gruber wrote: >> I think you found something in the specification I haven't found. What did >> you >> see that indicated how to deal with equipment misbehaving in this way? > > I'm referring to section 2.3.2.3 of "Universal Serial Bus Device Class > Definition for Audio Data Formats" > >> I found the following list of USB ID's. Just double checking, but is >> the 850 enough like the >> 950 line for me to include it? >> >> case 72000: /* WinTV-HVR950q (Retail, IR, ATSC/QAM */ >> case 72001: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */ >> case 72211: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */ >> case 72221: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */ >> case 72231: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */ >> case 72241: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM and analog video */ >> case 72251: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */ >> --> case 72301: /* WinTV-HVR850 (Retail, IR, ATSC and analog video */ >> case 72500: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM */ > > Yes, the HVR-850 should be included in the list. > >> I'd add that being the beginner at making kernel changes your review >> is particularly >> helpful to me (and to my confidence). > > You are likely to receive a more thorough/critical review when you > send to the alsa-devel mailing list, as they have considerably more > expertise in this area than I do. > > Devin > > -- > Devin J. Heitmueller - Kernel Labs > http://www.kernellabs.com > I'll do that. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sound/usb: Relax urb data alignment restriciton for HVR-950Q only
Hello John, On Thu, Dec 10, 2009 at 8:38 PM, John S Gruber wrote: > I think you found something in the specification I haven't found. What did you > see that indicated how to deal with equipment misbehaving in this way? I'm referring to section 2.3.2.3 of "Universal Serial Bus Device Class Definition for Audio Data Formats" > I found the following list of USB ID's. Just double checking, but is > the 850 enough like the > 950 line for me to include it? > > case 72000: /* WinTV-HVR950q (Retail, IR, ATSC/QAM */ > case 72001: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */ > case 72211: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */ > case 72221: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */ > case 72231: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */ > case 72241: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM and analog video */ > case 72251: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */ > --> case 72301: /* WinTV-HVR850 (Retail, IR, ATSC and analog video */ > case 72500: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM */ Yes, the HVR-850 should be included in the list. > I'd add that being the beginner at making kernel changes your review > is particularly > helpful to me (and to my confidence). You are likely to receive a more thorough/critical review when you send to the alsa-devel mailing list, as they have considerably more expertise in this area than I do. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sound/usb: Relax urb data alignment restriciton for HVR-950Q only
> After reviewing the patch as well as the spec, your change looks > pretty reasonable (aside from the fact that you need the other USB > IDs). It seems pretty clear that the au0828 violates the spec, but > the spec does indicate how to handle that case, which is what your > code addresses. > I think you found something in the specification I haven't found. What did you see that indicated how to deal with equipment misbehaving in this way? I found the following list of USB ID's. Just double checking, but is the 850 enough like the 950 line for me to include it? case 72000: /* WinTV-HVR950q (Retail, IR, ATSC/QAM */ case 72001: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */ case 72211: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */ case 72221: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */ case 72231: /* WinTV-HVR950q (OEM, IR, ATSC/QAM and analog video */ case 72241: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM and analog video */ case 72251: /* WinTV-HVR950q (Retail, IR, ATSC/QAM and analog video */ --> case 72301: /* WinTV-HVR850 (Retail, IR, ATSC and analog video */ case 72500: /* WinTV-HVR950q (OEM, No IR, ATSC/QAM */ Thanks again, not only for your help with my change, but with all you did to provide support for the Hauppage equipment. I'd add that being the beginner at making kernel changes your review is particularly helpful to me (and to my confidence). John -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sound/usb: Relax urb data alignment restriciton for HVR-950Q only
On Fri, Dec 4, 2009 at 6:26 PM, Devin Heitmueller wrote: > On Fri, Dec 4, 2009 at 5:15 PM, 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. > > > Hello John, > > Thanks for taking the time to dig into this. I will try to review > your patch this weekend (in conjunction with the spec). > > It's worth noting that there are actually nine different USB IDs that > would need this change (see au0828-cards.c), so it might be nice to > see if we can figure out a way for the au0828 driver to tell the > usbaudio driver about the quirk without relying on embedding USB ids > in the usbaudio driver. Hi John, After reviewing the patch as well as the spec, your change looks pretty reasonable (aside from the fact that you need the other USB IDs). It seems pretty clear that the au0828 violates the spec, but the spec does indicate how to handle that case, which is what your code addresses. All that said, the driver in question is managed by the ALSA project. I would suggest posting a message with your patch on the alsa-devel mailing list, so that it can be merged (and in fairness, those guys are much better suited to review your patch). On a separate note, I've been doing some testing with the device in a low-latency application, and I think I've spotted a separate bug in the usbaudio driver that causes the audio capture to stall. I'll be sending some email myself to alsa-devel as soon as I can test a patch. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sound/usb: Relax urb data alignment restriciton for HVR-950Q only
On Fri, Dec 4, 2009 at 5:15 PM, 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. Hello John, Thanks for taking the time to dig into this. I will try to review your patch this weekend (in conjunction with the spec). It's worth noting that there are actually nine different USB IDs that would need this change (see au0828-cards.c), so it might be nice to see if we can figure out a way for the au0828 driver to tell the usbaudio driver about the quirk without relying on embedding USB ids in the usbaudio driver. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sound/usb: Relax urb data alignment restriciton for HVR-950Q 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.
modified: sound/usb/usbaudio.c
Signed-off-by: John S. Gruber
---
sound/usb/usbaudio.c | 45 +++--
1 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c
index 44b9cdc..64d9d3a 100644
--- a/sound/usb/usbaudio.c
+++ b/sound/usb/usbaudio.c
@@ -107,8 +107,9 @@ MODULE_PARM_DESC(ignore_ctl_error,
#define MAX_PACKS_HS (MAX_PACKS * 8) /* in high speed mode */
#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;
snd_pcm_format_t format;/* format type */
@@ -126,8 +127,9 @@ struct audioformat {
unsigned int rates; /* rate bitmasks */
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;
@@ -174,8 +176,11 @@ 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 remainder; /* extra bytes moved to buffer */
+ 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 */
@@ -342,9 +347,9 @@ 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;
@@ -353,31 +358,44 @@ static int retire_capture_urb(struct
snd_usb_substream *subs,
if (urb->iso_frame_desc[i].status) {
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 (!(subs->txfr_quirks & ALLOW_SUBSLOT_BOUNDARIES))
+ bytelen = (bytelen/stride)*stride;
+ if (bytelen%(runtime->sample_bits>>3) != 0) {
+ int oldbytelen = bytelen;
+ bytelen = ((bytelen/stride)*stride);
+ printk(KERN_DEBUG "Corrected urb data len. %d -> %d\n",
+ oldbytelen, bytelen);
+ }
/* update the current pointer */
spin_lock_irqsave(&subs->lock, flags);
- oldptr = subs->hwptr_done;
+ len = (bytelen + subs->remainder) / stride;
+ subs->remainder = (bytelen + subs->remainder) % stride;
+ oldbyteptr = subs->byteptr;
subs->hwptr_done += len;
if (subs->hwptr_done >= runtime->buffer_size)
subs->hwptr_done -= runtime->buffer_size;
subs->transfer_done += len;
if (subs->transfer_done >= runtime->period_size) {
subs->transfer_done -= runtime->period_size;
period_elapsed = 1;
}
+ subs->byteptr += bytelen;
+ if (subs->byteptr >= runtime->buffer_size*stride)
+ subs->byteptr -= runtime->buffer_size*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;
-
