| Message ID | 20161130075923.15205-2-jiada_wang@mentor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 30 Nov 2016 08:59:21 +0100, Jiada Wang wrote: > > From: Andreas Pape <apape@de.adit-jv.com> > > since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected packetsize is always limited to Please use a form with 12 chars SHA ID plus the commit subject, e.g. 1234567890ab ("blah blah...") > nominal + 25%. It was discovered, that some devices have a much higher jitter > in used packetsizes than 25% which would result in BABBLE condition and dropping of packets. > A better solution is so assume the jitter to be the nominal packetsize: > -one nearly empty packet followed by a almost double sized one. The increase of the max frequency is supposedly OK. A remaining question is whether this should be included in stable kernel. It fixes in one side, but it's quite untested in another side. Maybe we queue this for 4.10, and later on notify to stable maintainer once when it's confirmed to work and be harmless. thanks, Takashi > > Signed-off-by: Andreas Pape <apape@de.adit-jv.com> > Signed-off-by: Jiada Wang <jiada_wang@mentor.com> > --- > sound/usb/endpoint.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > index c470251..2f592dd 100644 > --- a/sound/usb/endpoint.c > +++ b/sound/usb/endpoint.c > @@ -632,8 +632,8 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep, > ep->stride = frame_bits >> 3; > ep->silence_value = pcm_format == SNDRV_PCM_FORMAT_U8 ? 0x80 : 0; > > - /* assume max. frequency is 25% higher than nominal */ > - ep->freqmax = ep->freqn + (ep->freqn >> 2); > + /* assume max. frequency is double than nominal */ > + ep->freqmax = ep->freqn * 2; > /* Round up freqmax to nearest integer in order to calculate maximum > * packet size, which must represent a whole number of frames. > * This is accomplished by adding 0x0.ffff before converting the > -- > 2.9.3 > >
Hello Takashi On 11/30/2016 05:54 PM, Takashi Iwai wrote: > On Wed, 30 Nov 2016 08:59:21 +0100, > Jiada Wang wrote: >> >> From: Andreas Pape <apape@de.adit-jv.com> >> >> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected packetsize is always limited to > > Please use a form with 12 chars SHA ID plus the commit subject, e.g. > 1234567890ab ("blah blah...") I will update changelog as you have suggested in v2. > >> nominal + 25%. It was discovered, that some devices have a much higher jitter >> in used packetsizes than 25% which would result in BABBLE condition and dropping of packets. >> A better solution is so assume the jitter to be the nominal packetsize: >> -one nearly empty packet followed by a almost double sized one. > > The increase of the max frequency is supposedly OK. > A remaining question is whether this should be included in stable > kernel. It fixes in one side, but it's quite untested in another > side. Maybe we queue this for 4.10, and later on notify to stable > maintainer once when it's confirmed to work and be harmless. > > this makes sense to me Thanks, Jiada > thanks, > > Takashi > >> >> Signed-off-by: Andreas Pape <apape@de.adit-jv.com> >> Signed-off-by: Jiada Wang <jiada_wang@mentor.com> >> --- >> sound/usb/endpoint.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c >> index c470251..2f592dd 100644 >> --- a/sound/usb/endpoint.c >> +++ b/sound/usb/endpoint.c >> @@ -632,8 +632,8 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep, >> ep->stride = frame_bits >> 3; >> ep->silence_value = pcm_format == SNDRV_PCM_FORMAT_U8 ? 0x80 : 0; >> >> - /* assume max. frequency is 25% higher than nominal */ >> - ep->freqmax = ep->freqn + (ep->freqn >> 2); >> + /* assume max. frequency is double than nominal */ >> + ep->freqmax = ep->freqn * 2; >> /* Round up freqmax to nearest integer in order to calculate maximum >> * packet size, which must represent a whole number of frames. >> * This is accomplished by adding 0x0.ffff before converting the >> -- >> 2.9.3 >> >>
Jiada Wang wrote: > since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected packetsize is always limited to > nominal + 25%. It was discovered, that some devices Which devices? > have a much higher jitter in used packetsizes than 25% How high? (Please note that the USB specification restricts the jitter to at most one frame in consecutive packets.) > which would result in BABBLE condition and dropping of packets. > A better solution is so assume the jitter to be the nominal packetsize This solution is better for this one particular device, but how does it affect normal devices, or the Scarlett 2i4 on EHCI affected? Regards, Clemens
On Thu, 01 Dec 2016 08:41:17 +0100, Clemens Ladisch wrote: > > Jiada Wang wrote: > > since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected packetsize is always limited to > > nominal + 25%. It was discovered, that some devices > > Which devices? > > > have a much higher jitter in used packetsizes than 25% > > How high? (Please note that the USB specification restricts the jitter > to at most one frame in consecutive packets.) > > > which would result in BABBLE condition and dropping of packets. > > A better solution is so assume the jitter to be the nominal packetsize > > This solution is better for this one particular device, but how does it > affect normal devices, or the Scarlett 2i4 on EHCI affected? Actually, which value does this affected device in ep->maxpacksize? In the commit mentioned above, we changed the logic to take +25% frequency as the basis, and it my *reduce* if ep->maxpacksize is lower than that. OTOH, if ep->maxpacksize is sane, we can rely on it rather than the implicit +25% frequency. That said, maybe we can check ep->maxpacksize whether it fits within the expected range, then adapt it, or take +25% freq as fallback? thanks, Takashi
Takashi Iwai wrote: > Clemens Ladisch wrote: >> Jiada Wang wrote: >>> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected packetsize is always limited to >>> nominal + 25%. It was discovered, that some devices >> >> Which devices? >> >>> have a much higher jitter in used packetsizes than 25% >> >> How high? (Please note that the USB specification restricts the jitter >> to at most one frame in consecutive packets.) >> >>> which would result in BABBLE condition and dropping of packets. >>> A better solution is so assume the jitter to be the nominal packetsize >> >> This solution is better for this one particular device, but how does it >> affect normal devices, or the Scarlett 2i4 on EHCI affected? > > Actually, which value does this affected device in ep->maxpacksize? > In the commit mentioned above, we changed the logic to take +25% > frequency as the basis, and it my *reduce* if ep->maxpacksize is lower > than that. > > OTOH, if ep->maxpacksize is sane, we can rely on it rather than the > implicit +25% frequency. That said, maybe we can check > ep->maxpacksize whether it fits within the expected range, then adapt > it, or take +25% freq as fallback? You are describing how the current code behaves. The +25% limit _is_ what the code takes as the expected range. I'm wondering if that unknown device just declares a wrong interval value. Regards, Clemens
On Thu, 01 Dec 2016 12:16:47 +0100, Clemens Ladisch wrote: > > Takashi Iwai wrote: > > Clemens Ladisch wrote: > >> Jiada Wang wrote: > >>> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected packetsize is always limited to > >>> nominal + 25%. It was discovered, that some devices > >> > >> Which devices? > >> > >>> have a much higher jitter in used packetsizes than 25% > >> > >> How high? (Please note that the USB specification restricts the jitter > >> to at most one frame in consecutive packets.) > >> > >>> which would result in BABBLE condition and dropping of packets. > >>> A better solution is so assume the jitter to be the nominal packetsize > >> > >> This solution is better for this one particular device, but how does it > >> affect normal devices, or the Scarlett 2i4 on EHCI affected? > > > > Actually, which value does this affected device in ep->maxpacksize? > > In the commit mentioned above, we changed the logic to take +25% > > frequency as the basis, and it my *reduce* if ep->maxpacksize is lower > > than that. > > > > OTOH, if ep->maxpacksize is sane, we can rely on it rather than the > > implicit +25% frequency. That said, maybe we can check > > ep->maxpacksize whether it fits within the expected range, then adapt > > it, or take +25% freq as fallback? > > You are describing how the current code behaves. The +25% limit _is_ > what the code takes as the expected range. Well, the question is what is the "sane" range. +25% doesn't fit for some devices. If maxpacksize fits without +100% as this patch suggests, can we rely on it instead? Takashi > > > I'm wondering if that unknown device just declares a wrong interval value. > > > Regards, > Clemens >
Hello Clemens On 11/30/2016 11:41 PM, Clemens Ladisch wrote: > Jiada Wang wrote: >> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected packetsize is always limited to >> nominal + 25%. It was discovered, that some devices > Which devices? It was a LG nexus > >> have a much higher jitter in used packetsizes than 25% > How high? (Please note that the USB specification restricts the jitter > to at most one frame in consecutive packets.) the nominal packet size was somewhere around 176bytes +25% would result in max expected packets to be ~220bytes We observed some packets exceeding this size (256byte) which caused the babble and dropping of that packets. Thanks, Jiada >> which would result in BABBLE condition and dropping of packets. >> A better solution is so assume the jitter to be the nominal packetsize > This solution is better for this one particular device, but how does it > affect normal devices, or the Scarlett 2i4 on EHCI affected? > > > Regards, > Clemens
Takashi Iwai wrote: > Clemens Ladisch wrote: >> Takashi Iwai wrote: >>> [...] >>> In the commit mentioned above, we changed the logic to take +25% >>> frequency as the basis, and it my *reduce* if ep->maxpacksize is lower >>> than that. >>> >>> OTOH, if ep->maxpacksize is sane, we can rely on it rather than the >>> implicit +25% frequency. That said, maybe we can check >>> ep->maxpacksize whether it fits within the expected range, then adapt >>> it, or take +25% freq as fallback? >> >> You are describing how the current code behaves. The +25% limit _is_ >> what the code takes as the expected range. > > Well, the question is what is the "sane" range. +25% doesn't fit for > some devices. The USB audio specification _requires_ that there is as little jitter as possible. It's no surprise that some device violates the specification. But we don't know what the actual error is; whether we could adjust the packet size for this particular device only, or increase the limit for all devices, or use a completely different workaround. > If maxpacksize fits without +100% as this patch suggests, can we rely > on it instead? The packet size affect the following computations, like the number of packets per URB. I don't know how bad the effects would be. Regards, Clemens
Jiada Wang wrote: > On 11/30/2016 11:41 PM, Clemens Ladisch wrote: >> Jiada Wang wrote: >>> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected packetsize is always limited to >>> nominal + 25%. It was discovered, that some devices >> >> Which devices? > > It was a LG nexus So it was the Android audio accessory mode. >>> have a much higher jitter in used packetsizes than 25% >> >> How high? > > the nominal packet size was somewhere around 176bytes > +25% would result in max expected packets to be ~220bytes > We observed some packets exceeding this size (256byte) 256 bytes per USB frame would correspond to 64 kHz, instead of the nominal 44.1 kHz. The audio accessory sample format is fixed, and that mode is no longer developed, so increasing the limit to +50% would be sufficient to work around this problem. I don't know if this is a bug in Google's generic AOA code, or if LG did some changes; I have not heard any other report so far ... Regards, Clemens
Hello Clemens On 12/01/2016 04:15 AM, Clemens Ladisch wrote: > Jiada Wang wrote: >> On 11/30/2016 11:41 PM, Clemens Ladisch wrote: >>> Jiada Wang wrote: >>>> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected packetsize is always limited to >>>> nominal + 25%. It was discovered, that some devices >>> Which devices? >> It was a LG nexus > So it was the Android audio accessory mode. > >>>> have a much higher jitter in used packetsizes than 25% >>> How high? >> the nominal packet size was somewhere around 176bytes >> +25% would result in max expected packets to be ~220bytes >> We observed some packets exceeding this size (256byte) > 256 bytes per USB frame would correspond to 64 kHz, instead of the > nominal 44.1 kHz. > > The audio accessory sample format is fixed, and that mode is no longer > developed, so increasing the limit to +50% would be sufficient to work > around this problem. > > I don't know if this is a bug in Google's generic AOA code, or if LG did > some changes; I have not heard any other report so far ... > We also reproduced the same issue with following android devices, * Samsung S4, Android 4.2.2, model GT-I9505 * Sony Xperia Z, Android 4.2.2, model number CC6603 * LGE Nexus 4, Android 4.2.2 so it most likely not a LG specific issue. I agree to increase to +50% would be sufficient to avoid this issue. Thanks, Jiada > Regards, > Clemens
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index c470251..2f592dd 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -632,8 +632,8 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep, ep->stride = frame_bits >> 3; ep->silence_value = pcm_format == SNDRV_PCM_FORMAT_U8 ? 0x80 : 0; - /* assume max. frequency is 25% higher than nominal */ - ep->freqmax = ep->freqn + (ep->freqn >> 2); + /* assume max. frequency is double than nominal */ + ep->freqmax = ep->freqn * 2; /* Round up freqmax to nearest integer in order to calculate maximum * packet size, which must represent a whole number of frames. * This is accomplished by adding 0x0.ffff before converting the