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.
> >> - /* 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.
> >> - 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;
> >> for (i = 0; i < UVC_NUM_REQUESTS; ++i) {
> >>
> >> video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL);
> >>
--
Regards,
Laurent Pinchart
--
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
