Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices
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.che...@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
Re: [PATCH] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices
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.che...@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 -- 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] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices
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.che...@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; -- 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] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices
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.pugli...@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 -- 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] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices
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.pugli...@gmail.com --- drivers/media/usb/uvc/uvc_video.c | 3 +++ 1 file changed, 3 insertions(+) -- Regards, Laurent Pinchart -- 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] uvc: update uvc_endpoint_max_bpi to handle USB_SPEED_WIRELESS devices
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.pugli...@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 -- 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
