Hi Laurent,
On 5/2/2013 6:39 AM, Laurent Pinchart wrote:
Hi Bhupesh,
On Wednesday 01 May 2013 01:31:18 Bhupesh SHARMA wrote:
On 4/29/2013 4:23 PM, Laurent Pinchart wrote:
On Thursday 11 April 2013 15:04:04 Bhupesh Sharma wrote:
This patch adds the support for Bulk endpoint to be used as video
streamingendpoint, on basis of a module parameter.
By default, the gadget still supports Isochronous endpoint for video
streaming, but if the module parameter 'bulk_streaming_ep' is set to 1,
we can support Bulk endpoint as well, which is useful for UDC's which
don't support Isochronous endpoints.
The important difference between the two implementations is that,
alt-settings in a video streaming interface are supported only for
Isochronous endpoints as there are different alt-settings for
zero-bandwidth and full-bandwidth use-cases, but the same is not true for
Bulk endpoints, as they support only a single alt-setting.
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxx>
---
Note that to ease review and integration of this patch, I have rebased it
on Laurent's UVC gadget git tree available here (head uvc-gadget):
git://linuxtv.org/pinchartl/uvcvideo.git
This will allow the patch to be pulled into Felipe's repo in one go
after review and any subsequent rework (if required).
Thank you.
drivers/usb/gadget/f_uvc.c | 321 +++++++++++++++++++++++++--------
drivers/usb/gadget/uvc.h | 2 +
drivers/usb/gadget/uvc_v4l2.c | 17 ++-
drivers/usb/gadget/uvc_video.c | 13 ++-
4 files changed, 286 insertions(+), 67 deletions(-)
diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c
index 38dcedd..e5953eb 100644
--- a/drivers/usb/gadget/f_uvc.c
+++ b/drivers/usb/gadget/f_uvc.c
[snip]
@@ -317,45 +408,91 @@ uvc_function_set_alt(struct usb_function *f,
unsigned
interface, unsigned alt) if (interface != uvc->streaming_intf)
return -EINVAL;
The number of nested statements below is getting a bit large for my taste.
Before addressing that, I have some doubts regarding the logic below.
uvc_function_set_alt() will be once to select alt setting 0 in response to
SET_CONFIGURATION (see set_config() in composite.c) and once due to a
SET_INTERFACE request sent by the host UVC driver at initiazation time
(not at stream start timee). As SET_INTERFACE doesn't make much sense for
interfaces with bulk endpoints only (but is definitely valid), the
Windows UVC driver might not issue that extra SET_INTERFACE call. Even if
it does selecting alt setting 0 doesn't mean that we should start
streaming. I'm thus not sure if this is the best place to handle stream
start for bulk devices.
Sure. If you see my last patch on the UVC test application:
"[Test Application PATCH 1/2] UVC gadget: Add support for integration
with a video-capture device and other fixes"
you will notice that the STREAMON event passed to user space application
doesn't start the streaming on UVC gadget in case of Bulk endpoints.
Instead it is done at the time the UVC webcam gadget receives a
"UVC_VS_COMMIT_CONTROL" command from USB host.
This is in my opinion a shortcoming of the UVC specification, there's no
explicit indication sent by the host to the device that it should start
streaming on a bulk endpoint. I don't know how other devices handle that,
they might start streaming when they receive the first bulk request, and
stop after a timeout. The gadget framework would need to be extended to
inform drivers about those conditions (or at least about the first
condition, the second one could be detected in drivers).
Correct. I saw atleast one commercial webcam starting streaming on Bulk
video streaming endpoint, when it received the UVC_VS_COMMIT_CONTROL
command from USB host.
So, I used a similar approach and it works fine atleast in the use-case
I tested.
That sounds good to me. I'm pretty sure the logic below could be simplified
though. I could have a look at it in your v2.
Ok.
- /* TODO
- if (usb_endpoint_xfer_bulk(&uvc->desc.vs_ep))
- return alt ? -EINVAL : 0;
- */
+ if (!bulk_streaming_ep) {
+ switch (alt) {
+ case 0:
+ if (uvc->state != UVC_STATE_STREAMING)
+ return 0;
+
+ if (uvc->video.ep)
+ usb_ep_disable(uvc->video.ep);
+
+ memset(&v4l2_event, 0, sizeof(v4l2_event));
+ v4l2_event.type = UVC_EVENT_STREAMOFF;
+ v4l2_event_queue(uvc->vdev, &v4l2_event);
- switch (alt) {
- case 0:
- if (uvc->state != UVC_STATE_STREAMING)
+ uvc->state = UVC_STATE_CONNECTED;
return 0;
- if (uvc->video.ep)
- usb_ep_disable(uvc->video.ep);
+ case 1:
+ if (uvc->state != UVC_STATE_CONNECTED)
+ return 0;
- memset(&v4l2_event, 0, sizeof(v4l2_event));
- v4l2_event.type = UVC_EVENT_STREAMOFF;
- v4l2_event_queue(uvc->vdev, &v4l2_event);
+ if (uvc->video.ep) {
+ ret = config_ep_by_speed
+ (f->config->cdev->gadget,
+ &(uvc->func), uvc->video.ep);
+ if (ret)
+ return ret;
+ usb_ep_enable(uvc->video.ep);
+ }
- uvc->state = UVC_STATE_CONNECTED;
- return 0;
+ memset(&v4l2_event, 0, sizeof(v4l2_event));
+ v4l2_event.type = UVC_EVENT_STREAMON;
+ v4l2_event_queue(uvc->vdev, &v4l2_event);
+ return USB_GADGET_DELAYED_STATUS;
- case 1:
- if (uvc->state != UVC_STATE_CONNECTED)
+ default:
+ return -EINVAL;
+ }
+ } else {
+ switch (uvc->state) {
+ case UVC_STATE_CONNECTED:
+ if (!uvc->video.ep->driver_data) {
When can this happen ?
I will check this again and get back.
+ /*
+ * Enable the video streaming endpoint,
+ * but don't change the 'uvc->state'.
+ */
+ if (uvc->video.ep) {
+ ret = config_ep_by_speed
+ (f->config->cdev->gadget,
+ &(uvc->func), uvc->video.ep);
+ if (ret)
+ return ret;
+ ret = usb_ep_enable(uvc->video.ep);
+ if (ret)
+ return ret;
+
+ uvc->video.ep->driver_data = uvc;
+ }
+ } else {
+ memset(&v4l2_event, 0, sizeof(v4l2_event));
+ v4l2_event.type = UVC_EVENT_STREAMON;
+ v4l2_event_queue(uvc->vdev, &v4l2_event);
+
+ uvc->state = UVC_STATE_STREAMING;
+ }
return 0;
- if (uvc->video.ep) {
- ret = config_ep_by_speed(f->config->cdev->gadget,
- &(uvc->func), uvc->video.ep);
- if (ret)
- return ret;
- usb_ep_enable(uvc->video.ep);
- }
+ case UVC_STATE_STREAMING:
+ if (uvc->video.ep->driver_data) {
+ if (uvc->video.ep) {
+ ret = usb_ep_disable(uvc->video.ep);
+ if (ret)
+ return ret;
+ }
+ }
- memset(&v4l2_event, 0, sizeof(v4l2_event));
- v4l2_event.type = UVC_EVENT_STREAMON;
- v4l2_event_queue(uvc->vdev, &v4l2_event);
- return USB_GADGET_DELAYED_STATUS;
+ memset(&v4l2_event, 0, sizeof(v4l2_event));
+ v4l2_event.type = UVC_EVENT_STREAMOFF;
+ v4l2_event_queue(uvc->vdev, &v4l2_event);
- default:
- return -EINVAL;
+ uvc->state = UVC_STATE_CONNECTED;
+ return 0;
+
+ default:
+ return -EINVAL;
+ }
}
}
[snip]
@@ -594,11 +737,19 @@ uvc_function_bind(struct usb_configuration *c,
struct
usb_function *f)
INFO(cdev, "uvc_function_bind\n");
+ /* Use Bulk endpoint for video streaming?. */
+ uvc->video.bulk_streaming_ep = bulk_streaming_ep;
+
/* Sanity check the streaming endpoint module parameters.
*/
- streaming_interval = clamp(streaming_interval, 1U, 16U);
- streaming_maxpacket = clamp(streaming_maxpacket, 1U, 3072U);
- streaming_maxburst = min(streaming_maxburst, 15U);
+ if (!bulk_streaming_ep) {
+ streaming_interval = clamp(streaming_interval, 1U, 16U);
+ streaming_maxpacket = clamp(streaming_maxpacket, 1U, 3072U);
+ streaming_maxburst = min(streaming_maxburst, 15U);
+ } else {
+ streaming_maxpacket = clamp(streaming_maxpacket, 1U, 1024U);
+ streaming_maxburst = min(streaming_maxburst, 15U);
+ }
/* Fill in the FS/HS/SS Video Streaming specific descriptors from the
* module parameters.
@@ -617,19 +768,34 @@ uvc_function_bind(struct usb_configuration *c,
struct
usb_function *f) max_packet_size = streaming_maxpacket / 3;
}
Wouldn't the code below become much simpler if we merged the isoc and bulk
endpoint descriptors and just patched the endpoint type at runtime ?
Only patching the endpoint type will not help. There are various other
differences in ISOC and BULK endpoint properties that must be handled here:
- mult for USBSS: valid for ISOC endpoints, and not defined for BULK
endpoints.
- maxpkt size variations for different speeds.
Sure, those need to be handled as well. My point was that I think the code
would be simpler if you merged the isoc and bulk endpoint descriptors above,
and just set the fields that differ here. You would get rid of at least some
of the bulk_streaming_ep checks.
Ok. Let me write down what I understood from your mail (please correct
me if I am wrong):
There should be only one instance of video streaming endpoint (both for
Bulk and Isoc) and we patch the same as per the module parameters as and
where required, to set the following parameters:
- endpoint type.
- ep max pkt size.
- bInterval.
- SS parameters.
- etc.
- uvc_fs_streaming_ep.wMaxPacketSize = min(streaming_maxpacket, 1023U);
- uvc_fs_streaming_ep.bInterval = streaming_interval;
+ if (!bulk_streaming_ep) {
+ uvc_fs_streaming_ep.wMaxPacketSize = min(streaming_maxpacket,
+ 1023U);
+ uvc_fs_streaming_ep.bInterval = streaming_interval;
+
+ uvc_hs_streaming_ep.wMaxPacketSize = max_packet_size;
+ uvc_hs_streaming_ep.wMaxPacketSize |= ((max_packet_mult - 1)
+ << 11);
+ uvc_hs_streaming_ep.bInterval = streaming_interval;
+
+ uvc_ss_streaming_ep.wMaxPacketSize = max_packet_size;
+ uvc_ss_streaming_ep.bInterval = streaming_interval;
+ uvc_ss_streaming_comp.bmAttributes = max_packet_mult - 1;
+ uvc_ss_streaming_comp.bMaxBurst = streaming_maxburst;
+ uvc_ss_streaming_comp.wBytesPerInterval =
+ max_packet_size * max_packet_mult * streaming_maxburst;
+ } else {
+ uvc_fs_bulk_streaming_ep.wMaxPacketSize =
+ min(streaming_maxpacket, 64U);
- uvc_hs_streaming_ep.wMaxPacketSize = max_packet_size;
- uvc_hs_streaming_ep.wMaxPacketSize |= ((max_packet_mult - 1) << 11);
- uvc_hs_streaming_ep.bInterval = streaming_interval;
+ uvc_hs_bulk_streaming_ep.wMaxPacketSize =
+ min(streaming_maxpacket, 512U);
- uvc_ss_streaming_ep.wMaxPacketSize = max_packet_size;
- uvc_ss_streaming_ep.bInterval = streaming_interval;
- uvc_ss_streaming_comp.bmAttributes = max_packet_mult - 1;
- uvc_ss_streaming_comp.bMaxBurst = streaming_maxburst;
- uvc_ss_streaming_comp.wBytesPerInterval =
- max_packet_size * max_packet_mult * streaming_maxburst;
+ uvc_ss_bulk_streaming_ep.wMaxPacketSize = max_packet_size;
+ uvc_ss_streaming_comp.bMaxBurst = streaming_maxburst;
+ uvc_ss_streaming_comp.wBytesPerInterval =
+ max_packet_size * streaming_maxburst;
+ }
/* Allocate endpoints. */
ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
[snip]
diff --git a/drivers/usb/gadget/uvc_video.c
b/drivers/usb/gadget/uvc_video.c index 71e896d..87ac526 100644
--- a/drivers/usb/gadget/uvc_video.c
+++ b/drivers/usb/gadget/uvc_video.c
@@ -238,9 +238,13 @@ uvc_video_alloc_requests(struct uvc_video *video)
BUG_ON(video->req_size);
- req_size = video->ep->maxpacket
- * max_t(unsigned int, video->ep->maxburst, 1)
- * (video->ep->mult + 1);
+ if (!video->bulk_streaming_ep)
+ req_size = video->ep->maxpacket
+ * max_t(unsigned int, video->ep->maxburst, 1)
+ * (video->ep->mult + 1);
+ else
+ req_size = video->ep->maxpacket
+ * max_t(unsigned int, video->ep->maxburst, 1);
What is the value of video->ep->mult for bulk endpoints ? If it's always 0
there's no need to change the code here.
Actually USB3.0 specs do not define MULT parameter for Bulk endpoints,
so I am not sure if assuming the same to have a value of 0 here will be
in SYNC with the specs. That's why I differentiated the two paths in the
patch.
config_ep_by_speed() sets mult to 0 for all endpoints except isoc super speed.
I think it's thus safe to multiple by (video->ep->mult + 1) unconditionally.
Alternatively, you could do
+ req_size = video->ep->maxpacket
+ * max_t(unsigned int, video->ep->maxburst, 1);
+ if (!video->bulk_streaming_ep)
+ req_size *= video->ep->mult + 1;
To be honest, defining ep->mult parameter for Bulk endpoints seems
inconsistent to me in the very first place. So, I am not sure that
config_ep_by_speed() setting ep->mult = 0 for all endpoints except
ISOC endpoints is consistent with USB3.0 specs. It's like setting a
register to a particular value which is marked as RESERVED in the
data-sheet :)
So, I will prefer the alternative suggested by you.
Regards,
Bhupesh
for (i = 0; i < UVC_NUM_REQUESTS; ++i) {
video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
