[PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Thomas Pugliese]

Isochronous endpoints on devices with speed == USB_SPEED_WIRELESS can 
have a max packet size ranging from 1-3584 bytes.  Add a case to 
uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS.  Otherwise endpoints 
for those devices will fall to the default case which masks off any 
values > 2047.  This causes uvc_init_video to underestimate the 
bandwidth available and fail to find a suitable alt setting for high 
bandwidth video streams.

Signed-off-by: Thomas Pugliese <thomas.pugliese@gmail.com>
---
 drivers/media/usb/uvc/uvc_video.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 898c208..103cd4e 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1453,6 +1453,9 @@ static unsigned int uvc_endpoint_max_bpi(struct usb_device *dev,
 	case USB_SPEED_HIGH:
 		psize = usb_endpoint_maxp(&ep->desc);
 		return (psize & 0x07ff) * (1 + ((psize >> 11) & 3));
+	case USB_SPEED_WIRELESS:
+		psize = usb_endpoint_maxp(&ep->desc);
+		return psize;
 	default:
 		psize = usb_endpoint_maxp(&ep->desc);
 		return psize & 0x07ff;
-- 
1.8.3.2

Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Laurent Pinchart]

Hi Thomas,

Thank you for the patch.

On Friday 24 January 2014 15:17:28 Thomas Pugliese wrote:
> Isochronous endpoints on devices with speed == USB_SPEED_WIRELESS can
> have a max packet size ranging from 1-3584 bytes.  Add a case to
> uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS.  Otherwise endpoints
> for those devices will fall to the default case which masks off any
> values > 2047.  This causes uvc_init_video to underestimate the
> bandwidth available and fail to find a suitable alt setting for high
> bandwidth video streams.

I'm not too familiar with wireless USB, but shouldn't the value be multiplied 
by bMaxBurst from the endpoint companion descriptor ? Superspeed devices 
provide the multiplied value in their endpoint companion descriptor's 
wBytesPerInterval field, but there's no such field for wireless devices.

Out of curiosity, which device have you tested this with ?

> Signed-off-by: Thomas Pugliese <thomas.pugliese@gmail.com>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 898c208..103cd4e 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1453,6 +1453,9 @@ static unsigned int uvc_endpoint_max_bpi(struct
> usb_device *dev, case USB_SPEED_HIGH:
>  		psize = usb_endpoint_maxp(&ep->desc);
>  		return (psize & 0x07ff) * (1 + ((psize >> 11) & 3));
> +	case USB_SPEED_WIRELESS:
> +		psize = usb_endpoint_maxp(&ep->desc);
> +		return psize;
>  	default:
>  		psize = usb_endpoint_maxp(&ep->desc);
>  		return psize & 0x07ff;

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Thomas Pugliese]

On Mon, 27 Jan 2014, Laurent Pinchart wrote:

> Hi Thomas,
> 
> Thank you for the patch.
> 
> On Friday 24 January 2014 15:17:28 Thomas Pugliese wrote:
> > Isochronous endpoints on devices with speed == USB_SPEED_WIRELESS can
> > have a max packet size ranging from 1-3584 bytes.  Add a case to
> > uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS.  Otherwise endpoints
> > for those devices will fall to the default case which masks off any
> > values > 2047.  This causes uvc_init_video to underestimate the
> > bandwidth available and fail to find a suitable alt setting for high
> > bandwidth video streams.
> 
> I'm not too familiar with wireless USB, but shouldn't the value be multiplied 
> by bMaxBurst from the endpoint companion descriptor ? Superspeed devices 
> provide the multiplied value in their endpoint companion descriptor's 
> wBytesPerInterval field, but there's no such field for wireless devices.
> 

For wireless USB isochronous endpoints, the values in the endpoint 
descriptor are the logical interval and max packet size that the endpoint 
can support.  They are provided for backwards compatibility for just this 
type of situation.  You are correct that the actual endpoint 
characteristics are the bMaxBurst, wOverTheAirPacketSize, and 
bOverTheAirInterval values from the WUSB endpoint companion descriptor but 
only the host controller really needs to know about those details.  In 
fact, the values from the endpoint companion descriptor might actually 
over-estimate the bandwidth available since the device can set bMaxBurst 
to a higher value than necessary to allow for retries.

> Out of curiosity, which device have you tested this with ?

The device is a standard wired UVC webcam: Quanta CQEC2B (VID: 0x0408, 
PID: 0x9005).  It is connected to an Alereon Wireless USB bridge dev kit 
which allows it to operate as a WUSB device.

Thomas

> 
> > Signed-off-by: Thomas Pugliese <thomas.pugliese@gmail.com>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Laurent Pinchart]

Hi Thomas,

On Monday 27 January 2014 09:54:58 Thomas Pugliese wrote:
> On Mon, 27 Jan 2014, Laurent Pinchart wrote:
> > On Friday 24 January 2014 15:17:28 Thomas Pugliese wrote:
> > > Isochronous endpoints on devices with speed == USB_SPEED_WIRELESS can
> > > have a max packet size ranging from 1-3584 bytes.  Add a case to
> > > uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS.  Otherwise endpoints
> > > for those devices will fall to the default case which masks off any
> > > values > 2047.  This causes uvc_init_video to underestimate the
> > > bandwidth available and fail to find a suitable alt setting for high
> > > bandwidth video streams.
> > 
> > I'm not too familiar with wireless USB, but shouldn't the value be
> > multiplied by bMaxBurst from the endpoint companion descriptor ?
> > Superspeed devices provide the multiplied value in their endpoint
> > companion descriptor's wBytesPerInterval field, but there's no such field
> > for wireless devices.
>
> For wireless USB isochronous endpoints, the values in the endpoint
> descriptor are the logical interval and max packet size that the endpoint
> can support.  They are provided for backwards compatibility for just this
> type of situation.  You are correct that the actual endpoint
> characteristics are the bMaxBurst, wOverTheAirPacketSize, and
> bOverTheAirInterval values from the WUSB endpoint companion descriptor but
> only the host controller really needs to know about those details.  In
> fact, the values from the endpoint companion descriptor might actually
> over-estimate the bandwidth available since the device can set bMaxBurst
> to a higher value than necessary to allow for retries.

OK, I'll trust you on that :-)

I've taken the patch in my tree and will send a pull request for v3.15.

> > Out of curiosity, which device have you tested this with ?
> 
> The device is a standard wired UVC webcam: Quanta CQEC2B (VID: 0x0408,
> PID: 0x9005).  It is connected to an Alereon Wireless USB bridge dev kit
> which allows it to operate as a WUSB device.
> 
> Thomas
> 
> > > Signed-off-by: Thomas Pugliese <thomas.pugliese@gmail.com>
> > > ---
> > > 
> > >  drivers/media/usb/uvc/uvc_video.c | 3 +++
> > >  1 file changed, 3 insertions(+)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Thomas Pugliese]

On Mon, 27 Jan 2014, Laurent Pinchart wrote:

> Hi Thomas,
> 
> On Monday 27 January 2014 09:54:58 Thomas Pugliese wrote:
> > On Mon, 27 Jan 2014, Laurent Pinchart wrote:
> > > On Friday 24 January 2014 15:17:28 Thomas Pugliese wrote:
> > > > Isochronous endpoints on devices with speed == USB_SPEED_WIRELESS can
> > > > have a max packet size ranging from 1-3584 bytes.  Add a case to
> > > > uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS.  Otherwise endpoints
> > > > for those devices will fall to the default case which masks off any
> > > > values > 2047.  This causes uvc_init_video to underestimate the
> > > > bandwidth available and fail to find a suitable alt setting for high
> > > > bandwidth video streams.
> > > 
> > > I'm not too familiar with wireless USB, but shouldn't the value be
> > > multiplied by bMaxBurst from the endpoint companion descriptor ?
> > > Superspeed devices provide the multiplied value in their endpoint
> > > companion descriptor's wBytesPerInterval field, but there's no such field
> > > for wireless devices.
> >
> > For wireless USB isochronous endpoints, the values in the endpoint
> > descriptor are the logical interval and max packet size that the endpoint
> > can support.  They are provided for backwards compatibility for just this
> > type of situation.  You are correct that the actual endpoint
> > characteristics are the bMaxBurst, wOverTheAirPacketSize, and
> > bOverTheAirInterval values from the WUSB endpoint companion descriptor but
> > only the host controller really needs to know about those details.  In
> > fact, the values from the endpoint companion descriptor might actually
> > over-estimate the bandwidth available since the device can set bMaxBurst
> > to a higher value than necessary to allow for retries.
> 
> OK, I'll trust you on that :-)
> 
> I've taken the patch in my tree and will send a pull request for v3.15.
> 
> > > Out of curiosity, which device have you tested this with ?
> > 
> > The device is a standard wired UVC webcam: Quanta CQEC2B (VID: 0x0408,
> > PID: 0x9005).  It is connected to an Alereon Wireless USB bridge dev kit
> > which allows it to operate as a WUSB device.
> > 
> > Thomas
> > 
> > > > Signed-off-by: Thomas Pugliese <thomas.pugliese@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/media/usb/uvc/uvc_video.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

So it turns out that this change (commit 79af67e77f86404e77e65ad954bf) 
breaks wireless USB devices that were designed to work with Windows 
because Windows also does not differentiate between Wireless USB devices 
and USB 2.0 high speed devices.  This change should probably be reverted 
before it goes out in the 3.15 release.  Devices that are strictly WUSB 
spec compliant will not work with some max packet sizes but they never did 
anyway.

In order to support both compliant and non-compliant WUSB devices, 
uvc_endpoint_max_bpi should look at the endpoint companion descriptor but 
that descriptor is not readily available as it is for super speed devices 
so that patch will have to wait for another time.

Thanks,
Thomas

Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Laurent Pinchart]

Hi Thomas,

On Monday 14 April 2014 21:07:12 Thomas Pugliese wrote:
> On Mon, 27 Jan 2014, Laurent Pinchart wrote:
> > On Monday 27 January 2014 09:54:58 Thomas Pugliese wrote:
> > > On Mon, 27 Jan 2014, Laurent Pinchart wrote:
> > > > On Friday 24 January 2014 15:17:28 Thomas Pugliese wrote:
> > > > > Isochronous endpoints on devices with speed == USB_SPEED_WIRELESS
> > > > > can have a max packet size ranging from 1-3584 bytes. Add a case to
> > > > > uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS. Otherwise
> > > > > endpoints for those devices will fall to the default case which
> > > > > masks off any values > 2047. This causes uvc_init_video to
> > > > > underestimate the bandwidth available and fail to find a suitable
> > > > > alt setting for high bandwidth video streams.
> > > > 
> > > > I'm not too familiar with wireless USB, but shouldn't the value be
> > > > multiplied by bMaxBurst from the endpoint companion descriptor ?
> > > > Superspeed devices provide the multiplied value in their endpoint
> > > > companion descriptor's wBytesPerInterval field, but there's no such
> > > > field for wireless devices.
> > > 
> > > For wireless USB isochronous endpoints, the values in the endpoint
> > > descriptor are the logical interval and max packet size that the
> > > endpoint can support. They are provided for backwards compatibility for
> > > just this type of situation. You are correct that the actual endpoint
> > > characteristics are the bMaxBurst, wOverTheAirPacketSize, and
> > > bOverTheAirInterval values from the WUSB endpoint companion descriptor
> > > but only the host controller really needs to know about those details. 
> > > In fact, the values from the endpoint companion descriptor might
> > > actually over-estimate the bandwidth available since the device can set
> > > bMaxBurst to a higher value than necessary to allow for retries.
> > 
> > OK, I'll trust you on that :-)
> > 
> > I've taken the patch in my tree and will send a pull request for v3.15.
> > 
> > > > Out of curiosity, which device have you tested this with ?
> > > 
> > > The device is a standard wired UVC webcam: Quanta CQEC2B (VID: 0x0408,
> > > PID: 0x9005).  It is connected to an Alereon Wireless USB bridge dev kit
> > > which allows it to operate as a WUSB device.
> > > 
> > > Thomas
> > > 
> > > > > Signed-off-by: Thomas Pugliese <thomas.pugliese@gmail.com>
> > > > > ---
> > > > > 
> > > > >  drivers/media/usb/uvc/uvc_video.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> 
> So it turns out that this change (commit 79af67e77f86404e77e65ad954bf)
> breaks wireless USB devices that were designed to work with Windows
> because Windows also does not differentiate between Wireless USB devices
> and USB 2.0 high speed devices.  This change should probably be reverted
> before it goes out in the 3.15 release.  Devices that are strictly WUSB
> spec compliant will not work with some max packet sizes but they never did
> anyway.
> 
> In order to support both compliant and non-compliant WUSB devices,
> uvc_endpoint_max_bpi should look at the endpoint companion descriptor but
> that descriptor is not readily available as it is for super speed devices
> so that patch will have to wait for another time.

Could you please send me a proper revert patch with the above description in 
the commit message and CC Mauro Carvalho Chehab <m.chehab@samsung.com> ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Thomas Pugliese]

On Tue, 15 Apr 2014, Laurent Pinchart wrote:

> Hi Thomas,
> 
> 
> Could you please send me a proper revert patch with the above description in 
> the commit message and CC Mauro Carvalho Chehab <m.chehab@samsung.com> ?
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Hi Laurent, 
I can submit a patch to revert but I should make a correction first.  I 
had backported this change to an earlier kernel (2.6.39) which was before 
super speed support was added and the regression I described was based on 
that kernel.  It was actually the addition of super speed support that 
broke windows compatible devices.  My previous change fixed spec compliant 
devices but left windows compatible devices broken.

Basically, the timeline of changes is this:

1.  Prior to the addition of super speed support (commit 
6fd90db8df379e215): all WUSB devices were treated as HIGH_SPEED devices.  
This is how Windows works so Windows compatible devices would work.  For 
spec compliant WUSB devices, the max packet size would be incorrectly 
calculated which would result in high-bandwidth isoc streams being unable 
to find an alt setting that provided enough bandwidth.

2.  After super speed support: all WUSB devices fell through to the 
default case of uvc_endpoint_max_bpi which would mask off the upper bits 
of the max packet size.  This broke both WUSB spec compliant and non 
compliant devices because no endpoint with a large enough bpi would be 
found.

3.  After 79af67e77f86404e77e: Spec compliant devices are fixed but 
non-spec compliant (although Windows compatible) devices are broken.  
Basically, this is the opposite of how it worked prior to super speed 
support.

Given that, I can submit a patch to revert 79af67e77f86404e77e but that 
would go back to having all WUSB devices broken.  Alternatively, the 
change below will revert the behavior back to scenario 1 where Windows 
compatible devices work but strictly spec complaint devices may not.

I can send a proper patch for whichever scenario you prefer.

Thomas


diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 8d52baf..ed594d6 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1451,11 +1451,9 @@ static unsigned int uvc_endpoint_max_bpi(struct usb_device *dev,
 	case USB_SPEED_SUPER:
 		return ep->ss_ep_comp.wBytesPerInterval;
 	case USB_SPEED_HIGH:
-		psize = usb_endpoint_maxp(&ep->desc);
-		return (psize & 0x07ff) * (1 + ((psize >> 11) & 3));
 	case USB_SPEED_WIRELESS:
 		psize = usb_endpoint_maxp(&ep->desc);
-		return psize;
+		return (psize & 0x07ff) * (1 + ((psize >> 11) & 3));
 	default:
 		psize = usb_endpoint_maxp(&ep->desc);
 		return psize & 0x07ff;

Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Laurent Pinchart]

Hi Thomas,

(CC'ing the linux-usb mailing list)

On Tuesday 15 April 2014 16:45:28 Thomas Pugliese wrote:
> On Tue, 15 Apr 2014, Laurent Pinchart wrote:
> > Hi Thomas,
> > 
> > Could you please send me a proper revert patch with the above description
> > in the commit message and CC Mauro Carvalho Chehab <m.chehab@samsung.com>
> > ?
>
> Hi Laurent,
> I can submit a patch to revert but I should make a correction first.  I
> had backported this change to an earlier kernel (2.6.39) which was before
> super speed support was added and the regression I described was based on
> that kernel.  It was actually the addition of super speed support that
> broke windows compatible devices.  My previous change fixed spec compliant
> devices but left windows compatible devices broken.
> 
> Basically, the timeline of changes is this:
> 
> 1.  Prior to the addition of super speed support (commit
> 6fd90db8df379e215): all WUSB devices were treated as HIGH_SPEED devices.
> This is how Windows works so Windows compatible devices would work.  For
> spec compliant WUSB devices, the max packet size would be incorrectly
> calculated which would result in high-bandwidth isoc streams being unable
> to find an alt setting that provided enough bandwidth.
> 
> 2.  After super speed support: all WUSB devices fell through to the
> default case of uvc_endpoint_max_bpi which would mask off the upper bits
> of the max packet size.  This broke both WUSB spec compliant and non
> compliant devices because no endpoint with a large enough bpi would be
> found.
> 
> 3.  After 79af67e77f86404e77e: Spec compliant devices are fixed but
> non-spec compliant (although Windows compatible) devices are broken.
> Basically, this is the opposite of how it worked prior to super speed
> support.
> 
> Given that, I can submit a patch to revert 79af67e77f86404e77e but that
> would go back to having all WUSB devices broken.  Alternatively, the
> change below will revert the behavior back to scenario 1 where Windows
> compatible devices work but strictly spec complaint devices may not.
> 
> I can send a proper patch for whichever scenario you prefer.

Thank you for the explanation.

Reverting 79af67e77f86404e77e doesn't seem like a very good idea, given that 
all WUSB devices will be broken. We thus have two options:

- leaving the code as-is, with support for spec-compliant WUSB devices but not 
for microsoft-specific devices 

- applying the patch below, with support for microsoft-specific USB devices 
but not for spec-compliant devices

This isn't the first time this kind of situation occurs. Microsoft didn't 
support multiple configurations before Windows 8, making vendors come up with 
lots of "creative" MS-specific solutions. I consider those devices non USB 
compliant, and they should not be allowed to use the USB logo, but that would 
require a strong political move from the USB Implementers Forum which is more 
or less controlled by Microsoft... Welcome to the USB mafia.

Anyway, I have no experience with WUSB devices, so I don't know what's more 
common in the wild. What would you suggest ? Would there be a way to support 
both categories of devices ?

> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..ed594d6 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1451,11 +1451,9 @@ static unsigned int uvc_endpoint_max_bpi(struct
> usb_device *dev, case USB_SPEED_SUPER:
>  		return ep->ss_ep_comp.wBytesPerInterval;
>  	case USB_SPEED_HIGH:
> -		psize = usb_endpoint_maxp(&ep->desc);
> -		return (psize & 0x07ff) * (1 + ((psize >> 11) & 3));
>  	case USB_SPEED_WIRELESS:
>  		psize = usb_endpoint_maxp(&ep->desc);
> -		return psize;
> +		return (psize & 0x07ff) * (1 + ((psize >> 11) & 3));
>  	default:
>  		psize = usb_endpoint_maxp(&ep->desc);
>  		return psize & 0x07ff;

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Thomas Pugliese]

On Wed, 16 Apr 2014, Laurent Pinchart wrote:

> Hi Thomas,
> 
> (CC'ing the linux-usb mailing list)
> 
> On Tuesday 15 April 2014 16:45:28 Thomas Pugliese wrote:
> > On Tue, 15 Apr 2014, Laurent Pinchart wrote:
> > > Hi Thomas,
> > > 
> > > Could you please send me a proper revert patch with the above description
> > > in the commit message and CC Mauro Carvalho Chehab <m.chehab@samsung.com>
> > > ?
> >
> > Hi Laurent,
> > I can submit a patch to revert but I should make a correction first.  I
> > had backported this change to an earlier kernel (2.6.39) which was before
> > super speed support was added and the regression I described was based on
> > that kernel.  It was actually the addition of super speed support that
> > broke windows compatible devices.  My previous change fixed spec compliant
> > devices but left windows compatible devices broken.
> > 
> > Basically, the timeline of changes is this:
> > 
> > 1.  Prior to the addition of super speed support (commit
> > 6fd90db8df379e215): all WUSB devices were treated as HIGH_SPEED devices.
> > This is how Windows works so Windows compatible devices would work.  For
> > spec compliant WUSB devices, the max packet size would be incorrectly
> > calculated which would result in high-bandwidth isoc streams being unable
> > to find an alt setting that provided enough bandwidth.
> > 
> > 2.  After super speed support: all WUSB devices fell through to the
> > default case of uvc_endpoint_max_bpi which would mask off the upper bits
> > of the max packet size.  This broke both WUSB spec compliant and non
> > compliant devices because no endpoint with a large enough bpi would be
> > found.
> > 
> > 3.  After 79af67e77f86404e77e: Spec compliant devices are fixed but
> > non-spec compliant (although Windows compatible) devices are broken.
> > Basically, this is the opposite of how it worked prior to super speed
> > support.
> > 
> > Given that, I can submit a patch to revert 79af67e77f86404e77e but that
> > would go back to having all WUSB devices broken.  Alternatively, the
> > change below will revert the behavior back to scenario 1 where Windows
> > compatible devices work but strictly spec complaint devices may not.
> > 
> > I can send a proper patch for whichever scenario you prefer.
> 
> Thank you for the explanation.
> 
> Reverting 79af67e77f86404e77e doesn't seem like a very good idea, given that 
> all WUSB devices will be broken. We thus have two options:
> 
> - leaving the code as-is, with support for spec-compliant WUSB devices but not 
> for microsoft-specific devices 
> 
> - applying the patch below, with support for microsoft-specific USB devices 
> but not for spec-compliant devices
> 
> This isn't the first time this kind of situation occurs. Microsoft didn't 
> support multiple configurations before Windows 8, making vendors come up with 
> lots of "creative" MS-specific solutions. I consider those devices non USB 
> compliant, and they should not be allowed to use the USB logo, but that would 
> require a strong political move from the USB Implementers Forum which is more 
> or less controlled by Microsoft... Welcome to the USB mafia.
> 
> Anyway, I have no experience with WUSB devices, so I don't know what's more 
> common in the wild. What would you suggest ? 

I think that almost all current devices support the Windows/USB 2.0 format 
rather than stricty follow the WUSB spec.  Even the prototype device that 
I initially used to test UVC with Wireless USB has been updated to use the 
USB 2.0 format prior to shipping in order to remain compatible with 
Windows.  That being said, these devices are not very common at all in the 
consumer market.  They are mostly used in embedded/industrial settings so 
that may factor in as to which direction you want to go.

> Would there be a way to support 
> both categories of devices ?
> 

As you had mentioned previously, it should be possible to support both 
formats by ignoring the endpoint descriptor and looking at the bMaxBurst, 
bOverTheAirInterval and wOverTheAirPacketSize fields in the WUSB endpoint 
companion descriptor.  That is a more involved change to the UVC driver 
and also would require changes to USB core to store the WUSB endpoint 
companion descriptor in struct usb_host_endpoint similar to what is done 
for super speed devices.

Regards,
Thomas

Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Laurent Pinchart]

Hi Thomas,

On Wednesday 16 April 2014 12:29:22 Thomas Pugliese wrote:
> On Wed, 16 Apr 2014, Laurent Pinchart wrote:
> > Hi Thomas,
> > 
> > (CC'ing the linux-usb mailing list)
> > 
> > On Tuesday 15 April 2014 16:45:28 Thomas Pugliese wrote:
> > > On Tue, 15 Apr 2014, Laurent Pinchart wrote:
> > > > Hi Thomas,
> > > > 
> > > > Could you please send me a proper revert patch with the above
> > > > description in the commit message and CC Mauro Carvalho Chehab
> > > > <m.chehab@samsung.com> ?
> > > 
> > > Hi Laurent,
> > > I can submit a patch to revert but I should make a correction first.  I
> > > had backported this change to an earlier kernel (2.6.39) which was
> > > before super speed support was added and the regression I described was
> > > based on that kernel.  It was actually the addition of super speed
> > > support that broke windows compatible devices.  My previous change fixed
> > > spec compliant devices but left windows compatible devices broken.
> > > 
> > > Basically, the timeline of changes is this:
> > > 
> > > 1.  Prior to the addition of super speed support (commit
> > > 6fd90db8df379e215): all WUSB devices were treated as HIGH_SPEED devices.
> > > This is how Windows works so Windows compatible devices would work.  For
> > > spec compliant WUSB devices, the max packet size would be incorrectly
> > > calculated which would result in high-bandwidth isoc streams being
> > > unable to find an alt setting that provided enough bandwidth.
> > > 
> > > 2.  After super speed support: all WUSB devices fell through to the
> > > default case of uvc_endpoint_max_bpi which would mask off the upper bits
> > > of the max packet size.  This broke both WUSB spec compliant and non
> > > compliant devices because no endpoint with a large enough bpi would be
> > > found.
> > > 
> > > 3.  After 79af67e77f86404e77e: Spec compliant devices are fixed but
> > > non-spec compliant (although Windows compatible) devices are broken.
> > > Basically, this is the opposite of how it worked prior to super speed
> > > support.
> > > 
> > > Given that, I can submit a patch to revert 79af67e77f86404e77e but that
> > > would go back to having all WUSB devices broken.  Alternatively, the
> > > change below will revert the behavior back to scenario 1 where Windows
> > > compatible devices work but strictly spec complaint devices may not.
> > > 
> > > I can send a proper patch for whichever scenario you prefer.
> > 
> > Thank you for the explanation.
> > 
> > Reverting 79af67e77f86404e77e doesn't seem like a very good idea, given
> > that all WUSB devices will be broken. We thus have two options:
> > 
> > - leaving the code as-is, with support for spec-compliant WUSB devices but
> > not for microsoft-specific devices
> > 
> > - applying the patch below, with support for microsoft-specific USB
> > devices but not for spec-compliant devices
> > 
> > This isn't the first time this kind of situation occurs. Microsoft didn't
> > support multiple configurations before Windows 8, making vendors come up
> > with lots of "creative" MS-specific solutions. I consider those devices
> > non USB compliant, and they should not be allowed to use the USB logo,
> > but that would require a strong political move from the USB Implementers
> > Forum which is more or less controlled by Microsoft... Welcome to the USB
> > mafia.
> > 
> > Anyway, I have no experience with WUSB devices, so I don't know what's
> > more common in the wild. What would you suggest ?
> 
> I think that almost all current devices support the Windows/USB 2.0 format
> rather than stricty follow the WUSB spec.  Even the prototype device that
> I initially used to test UVC with Wireless USB has been updated to use the
> USB 2.0 format prior to shipping in order to remain compatible with
> Windows.  That being said, these devices are not very common at all in the
> consumer market.  They are mostly used in embedded/industrial settings so
> that may factor in as to which direction you want to go.
> 
> > Would there be a way to support
> > both categories of devices ?
> 
> As you had mentioned previously, it should be possible to support both
> formats by ignoring the endpoint descriptor and looking at the bMaxBurst,
> bOverTheAirInterval and wOverTheAirPacketSize fields in the WUSB endpoint
> companion descriptor.  That is a more involved change to the UVC driver
> and also would require changes to USB core to store the WUSB endpoint
> companion descriptor in struct usb_host_endpoint similar to what is done
> for super speed devices.

It's more complex indeed, but I believe it would be worth it. Any volunteer ? 
;-) In the meantime I'm fine with a patch that reverts to the previous 
behaviour. Please include the explanation of the problem in the commit 
message.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Thomas Pugliese]

On Thu, 17 Apr 2014, Laurent Pinchart wrote:

> Hi Thomas,
> 
> On Wednesday 16 April 2014 12:29:22 Thomas Pugliese wrote:
> > On Wed, 16 Apr 2014, Laurent Pinchart wrote:
> > > Hi Thomas,
> > > 
> > > (CC'ing the linux-usb mailing list)
> > > 
> > > On Tuesday 15 April 2014 16:45:28 Thomas Pugliese wrote:
> > > > On Tue, 15 Apr 2014, Laurent Pinchart wrote:
> > > > > Hi Thomas,
> > > > > 
> > > > > Could you please send me a proper revert patch with the above
> > > > > description in the commit message and CC Mauro Carvalho Chehab
> > > > > <m.chehab@samsung.com> ?
> > > > 
> > > > Hi Laurent,
> > > > I can submit a patch to revert but I should make a correction first.  I
> > > > had backported this change to an earlier kernel (2.6.39) which was
> > > > before super speed support was added and the regression I described was
> > > > based on that kernel.  It was actually the addition of super speed
> > > > support that broke windows compatible devices.  My previous change fixed
> > > > spec compliant devices but left windows compatible devices broken.
> > > > 
> > > > Basically, the timeline of changes is this:
> > > > 
> > > > 1.  Prior to the addition of super speed support (commit
> > > > 6fd90db8df379e215): all WUSB devices were treated as HIGH_SPEED devices.
> > > > This is how Windows works so Windows compatible devices would work.  For
> > > > spec compliant WUSB devices, the max packet size would be incorrectly
> > > > calculated which would result in high-bandwidth isoc streams being
> > > > unable to find an alt setting that provided enough bandwidth.
> > > > 
> > > > 2.  After super speed support: all WUSB devices fell through to the
> > > > default case of uvc_endpoint_max_bpi which would mask off the upper bits
> > > > of the max packet size.  This broke both WUSB spec compliant and non
> > > > compliant devices because no endpoint with a large enough bpi would be
> > > > found.
> > > > 
> > > > 3.  After 79af67e77f86404e77e: Spec compliant devices are fixed but
> > > > non-spec compliant (although Windows compatible) devices are broken.
> > > > Basically, this is the opposite of how it worked prior to super speed
> > > > support.
> > > > 
> > > > Given that, I can submit a patch to revert 79af67e77f86404e77e but that
> > > > would go back to having all WUSB devices broken.  Alternatively, the
> > > > change below will revert the behavior back to scenario 1 where Windows
> > > > compatible devices work but strictly spec complaint devices may not.
> > > > 
> > > > I can send a proper patch for whichever scenario you prefer.
> > > 
> > > Thank you for the explanation.
> > > 
> > > Reverting 79af67e77f86404e77e doesn't seem like a very good idea, given
> > > that all WUSB devices will be broken. We thus have two options:
> > > 
> > > - leaving the code as-is, with support for spec-compliant WUSB devices but
> > > not for microsoft-specific devices
> > > 
> > > - applying the patch below, with support for microsoft-specific USB
> > > devices but not for spec-compliant devices
> > > 
> > > This isn't the first time this kind of situation occurs. Microsoft didn't
> > > support multiple configurations before Windows 8, making vendors come up
> > > with lots of "creative" MS-specific solutions. I consider those devices
> > > non USB compliant, and they should not be allowed to use the USB logo,
> > > but that would require a strong political move from the USB Implementers
> > > Forum which is more or less controlled by Microsoft... Welcome to the USB
> > > mafia.
> > > 
> > > Anyway, I have no experience with WUSB devices, so I don't know what's
> > > more common in the wild. What would you suggest ?
> > 
> > I think that almost all current devices support the Windows/USB 2.0 format
> > rather than stricty follow the WUSB spec.  Even the prototype device that
> > I initially used to test UVC with Wireless USB has been updated to use the
> > USB 2.0 format prior to shipping in order to remain compatible with
> > Windows.  That being said, these devices are not very common at all in the
> > consumer market.  They are mostly used in embedded/industrial settings so
> > that may factor in as to which direction you want to go.
> > 
> > > Would there be a way to support
> > > both categories of devices ?
> > 
> > As you had mentioned previously, it should be possible to support both
> > formats by ignoring the endpoint descriptor and looking at the bMaxBurst,
> > bOverTheAirInterval and wOverTheAirPacketSize fields in the WUSB endpoint
> > companion descriptor.  That is a more involved change to the UVC driver
> > and also would require changes to USB core to store the WUSB endpoint
> > companion descriptor in struct usb_host_endpoint similar to what is done
> > for super speed devices.
> 
> It's more complex indeed, but I believe it would be worth it. Any volunteer ? 
> ;-) In the meantime I'm fine with a patch that reverts to the previous 
> behaviour. Please include the explanation of the problem in the commit 
> message.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

I may make an attempt at the more complete fix once I finish some of the 
other items in my queue.  

For clarification, would you like a patch that reverts to the pre-super 
speed behavior where windows-compatible devices work not but spec 
compliant devices will not (i.e. treat USB_SPEED_HIGH and 
USB_SPEED_WIRELESS the same)?

Regards,
Thomas

Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices

[Laurent Pinchart]

Hi Thomas,

On Thursday 17 April 2014 09:53:32 Thomas Pugliese wrote:
> On Thu, 17 Apr 2014, Laurent Pinchart wrote:
> > On Wednesday 16 April 2014 12:29:22 Thomas Pugliese wrote:

[snip]

> > > As you had mentioned previously, it should be possible to support both
> > > formats by ignoring the endpoint descriptor and looking at the
> > > bMaxBurst, bOverTheAirInterval and wOverTheAirPacketSize fields in the
> > > WUSB endpoint companion descriptor.  That is a more involved change to
> > > the UVC driver and also would require changes to USB core to store the
> > > WUSB endpoint companion descriptor in struct usb_host_endpoint similar
> > > to what is done for super speed devices.
> > 
> > It's more complex indeed, but I believe it would be worth it. Any
> > volunteer ? ;-) In the meantime I'm fine with a patch that reverts to the
> > previous behaviour. Please include the explanation of the problem in the
> > commit message.
> 
> I may make an attempt at the more complete fix once I finish some of the
> other items in my queue.
> 
> For clarification, would you like a patch that reverts to the pre-super
> speed behavior where windows-compatible devices work not but spec
> compliant devices will not (i.e. treat USB_SPEED_HIGH and
> USB_SPEED_WIRELESS the same)?

I'll trust your judgment on that, if you believe it would be better from a 
user point of view, please send a patch. Otherwise we can wait until you find 
time to work on a proper fix.

-- 
Regards,

Laurent Pinchart