Hi Bhupesh,
Thank you for the patch. Please see below for comments.
On Thursday 11 April 2013 15:04:04 Bhupesh Sharma wrote:
> This patch adds the support for Bulk endpoint to be used as video streaming
> endpoint, 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
> @@ -45,6 +45,11 @@ static unsigned int streaming_maxburst;
> module_param(streaming_maxburst, uint, S_IRUGO|S_IWUSR);
> MODULE_PARM_DESC(streaming_maxburst, "0 - 15 (ss only)");
>
> +static bool bulk_streaming_ep;
> +module_param(bulk_streaming_ep, bool, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(bulk_streaming_ep, "0 (Use ISOC video streaming ep) / "
> + "1 (Use BULK video streaming ep)");
> +
Do you think an endpoint type property with values equal to "isoc" or "bulk"
would be more explicit ?
> /* ------------------------------------------------------------------------
> * Function descriptors
> */
> @@ -135,6 +140,19 @@ static struct usb_interface_descriptor
> uvc_streaming_intf_alt0 __initdata = { .iInterface = 0,
> };
>
> +static struct usb_interface_descriptor uvc_bulk_streaming_intf_alt0
> +__initdata = {
> + .bLength = USB_DT_INTERFACE_SIZE,
> + .bDescriptorType = USB_DT_INTERFACE,
> + .bInterfaceNumber = UVC_INTF_VIDEO_STREAMING,
> + .bAlternateSetting = 0,
> + .bNumEndpoints = 1,
> + .bInterfaceClass = USB_CLASS_VIDEO,
> + .bInterfaceSubClass = UVC_SC_VIDEOSTREAMING,
> + .bInterfaceProtocol = 0x00,
> + .iInterface = 0,
> +};
> +
For consistency, could you please rename uvc_streaming_intf_alt* to
uvc_isoc_streaming_intf_alt* (in a separate preparation patch) ?
> static struct usb_interface_descriptor uvc_streaming_intf_alt1 __initdata =
> { .bLength = USB_DT_INTERFACE_SIZE,
> .bDescriptorType = USB_DT_INTERFACE,
> @@ -160,6 +178,18 @@ static struct usb_endpoint_descriptor
> uvc_fs_streaming_ep __initdata = { .bInterval = 0,
> };
>
> +static struct usb_endpoint_descriptor uvc_fs_bulk_streaming_ep __initdata =
> { + .bLength = USB_DT_ENDPOINT_SIZE,
> + .bDescriptorType = USB_DT_ENDPOINT,
> + .bEndpointAddress = USB_DIR_IN,
> + .bmAttributes = USB_ENDPOINT_XFER_BULK,
> + /* The wMaxPacketSize and bInterval values will be initialized from
> + * module parameters.
> + */
> + .wMaxPacketSize = 0,
> + .bInterval = 0,
You can remove these two lines (I've sent a patch to do so for the rest of the
structures).
> +};
> +
> static struct usb_endpoint_descriptor uvc_hs_streaming_ep __initdata = {
> .bLength = USB_DT_ENDPOINT_SIZE,
> .bDescriptorType = USB_DT_ENDPOINT,
> @@ -173,6 +203,18 @@ static struct usb_endpoint_descriptor
> uvc_hs_streaming_ep __initdata = { .bInterval = 0,
> };
>
> +static struct usb_endpoint_descriptor uvc_hs_bulk_streaming_ep __initdata =
> { + .bLength = USB_DT_ENDPOINT_SIZE,
> + .bDescriptorType = USB_DT_ENDPOINT,
> + .bEndpointAddress = USB_DIR_IN,
> + .bmAttributes = USB_ENDPOINT_XFER_BULK,
> + /* The wMaxPacketSize and bInterval values will be initialized from
> + * module parameters.
> + */
> + .wMaxPacketSize = 0,
> + .bInterval = 0,
You can remove these two lines as well.
> +};
> +
Can you please group the bulk streaming endpoint descriptors together (either
before or after the isoc streaming endpoint descriptors) ?
> static struct usb_endpoint_descriptor uvc_ss_streaming_ep __initdata = {
> .bLength = USB_DT_ENDPOINT_SIZE,
> .bDescriptorType = USB_DT_ENDPOINT,
> @@ -187,6 +229,19 @@ static struct usb_endpoint_descriptor
> uvc_ss_streaming_ep __initdata = { .bInterval = 0,
> };
>
> +static struct usb_endpoint_descriptor uvc_ss_bulk_streaming_ep __initdata =
> { + .bLength = USB_DT_ENDPOINT_SIZE,
> + .bDescriptorType = USB_DT_ENDPOINT,
> +
> + .bEndpointAddress = USB_DIR_IN,
> + .bmAttributes = USB_ENDPOINT_XFER_BULK,
> + /* The wMaxPacketSize and bInterval values will be initialized from
> + * module parameters.
> + */
> + .wMaxPacketSize = 0,
> + .bInterval = 0,
Those two lines can be removed as well.
> +};
> +
> static struct usb_ss_ep_comp_descriptor uvc_ss_streaming_comp __initdata =
> { .bLength = sizeof(uvc_ss_streaming_comp),
> .bDescriptorType = USB_DT_SS_ENDPOINT_COMP,
> @@ -196,18 +251,38 @@ static struct usb_ss_ep_comp_descriptor
> uvc_ss_streaming_comp __initdata = { .wBytesPerInterval =
> cpu_to_le16(1024),
> };
>
> +static struct usb_ss_ep_comp_descriptor uvc_ss_bulk_streaming_comp
> +__initdata = {
> + .bLength = sizeof(uvc_ss_bulk_streaming_comp),
> + .bDescriptorType = USB_DT_SS_ENDPOINT_COMP,
> + /* The following 3 values can be tweaked if necessary. */
> + .bMaxBurst = 0,
> + .bmAttributes = 0,
> + .wBytesPerInterval = cpu_to_le16(1024),
Please replace these four lines with
/* The bMaxBurst, bmAttributes and wBytesPerInterval values will be
* initialized from module parameters.
*/
> +};
> +
> static const struct usb_descriptor_header * const uvc_fs_streaming[] = {
> (struct usb_descriptor_header *) &uvc_streaming_intf_alt1,
> (struct usb_descriptor_header *) &uvc_fs_streaming_ep,
> NULL,
> };
>
> +static const struct usb_descriptor_header * const uvc_fs_bulk_streaming[] =
> { + (struct usb_descriptor_header *) &uvc_fs_bulk_streaming_ep,
> + NULL,
> +};
> +
> static const struct usb_descriptor_header * const uvc_hs_streaming[] = {
> (struct usb_descriptor_header *) &uvc_streaming_intf_alt1,
> (struct usb_descriptor_header *) &uvc_hs_streaming_ep,
> NULL,
> };
>
> +static const struct usb_descriptor_header * const uvc_hs_bulk_streaming[] =
> { + (struct usb_descriptor_header *) &uvc_hs_bulk_streaming_ep,
> + NULL,
> +};
> +
> static const struct usb_descriptor_header * const uvc_ss_streaming[] = {
> (struct usb_descriptor_header *) &uvc_streaming_intf_alt1,
> (struct usb_descriptor_header *) &uvc_ss_streaming_ep,
> @@ -215,6 +290,12 @@ static const struct usb_descriptor_header * const
> uvc_ss_streaming[] = { NULL,
> };
>
> +static const struct usb_descriptor_header * const uvc_ss_bulk_streaming[] =
> { + (struct usb_descriptor_header *) &uvc_ss_bulk_streaming_ep,
> + (struct usb_descriptor_header *) &uvc_ss_bulk_streaming_comp,
> + NULL,
> +};
> +
Can you please group the bulk streaming endpoint descriptors together here as
well ?
> /*
> --------------------------------------------------------------------------
> * Control requests
> */
> @@ -285,7 +366,17 @@ uvc_function_get_alt(struct usb_function *f, unsigned
> interface) else if (interface != uvc->streaming_intf)
> return -EINVAL;
> else
> - return uvc->state == UVC_STATE_STREAMING ? 1 : 0;
> + /*
> + * Alt settings in an interface are supported only for
> + * ISOC endpoints as there are different alt-settings for
> + * zero-bandwidth and full-bandwidth cases, but the same
> + * is not true for BULK endpoints, as they have a single
> + * alt-setting.
> + */
> + if (!bulk_streaming_ep)
> + return uvc->state == UVC_STATE_STREAMING ? 1 : 0;
> + else
> + return 0;
What about just
- else
- return uvc->state == UVC_STATE_STREAMING ? 1 : 0;
+ else if (bulk_streaming_ep)
+ /* Alternate settings are not supported for bulk endpoints. */
+ return 0;
+ else
+ return uvc->state == UVC_STATE_STREAMING ? 1 : 0;
> }
>
> static int
> @@ -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.
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).
> - /* 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 ?
> + /*
> + * 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;
> + }
> }
> }
>
> @@ -450,6 +587,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum
> usb_device_speed speed) const struct uvc_descriptor_header * const
> *uvc_streaming_cls;
> const struct usb_descriptor_header * const *uvc_streaming_std;
> const struct usb_descriptor_header * const *src;
> + struct usb_interface_descriptor *streaming_intf_alt0;
> struct usb_descriptor_header **dst;
> struct usb_descriptor_header **hdr;
> unsigned int control_size;
> @@ -492,12 +630,17 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum
> usb_device_speed speed) * uvc_{fs|hs}_streaming
> */
>
> + if (!bulk_streaming_ep)
Could you test uvc->bulk_streaming_ep instead of the module parameters (here
and in all the other locations) ? This would allow parsing the module
parameter string value at init time (see above) into a numerical value.
> + streaming_intf_alt0 = &uvc_streaming_intf_alt0;
> + else
> + streaming_intf_alt0 = &uvc_bulk_streaming_intf_alt0;
> +
> /* Count descriptors and compute their size. */
> control_size = 0;
> streaming_size = 0;
> bytes = uvc_iad.bLength + uvc_control_intf.bLength
> + uvc_control_ep.bLength + uvc_control_cs_ep.bLength
> - + uvc_streaming_intf_alt0.bLength;
> + + streaming_intf_alt0->bLength;
>
> if (speed == USB_SPEED_SUPER) {
> bytes += uvc_ss_control_comp.bLength;
> @@ -547,7 +690,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum
> usb_device_speed speed) UVC_COPY_DESCRIPTOR(mem, dst,
> &uvc_ss_control_comp);
>
> UVC_COPY_DESCRIPTOR(mem, dst, &uvc_control_cs_ep);
> - UVC_COPY_DESCRIPTOR(mem, dst, &uvc_streaming_intf_alt0);
> + UVC_COPY_DESCRIPTOR(mem, dst, streaming_intf_alt0);
>
> uvc_streaming_header = mem;
> UVC_COPY_DESCRIPTORS(mem, dst,
> @@ -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 ?
> - 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);
> @@ -640,13 +806,31 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f) uvc->control_ep = ep;
> ep->driver_data = uvc;
>
> - if (gadget_is_superspeed(c->cdev->gadget))
> - ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep,
> - &uvc_ss_streaming_comp);
> - else if (gadget_is_dualspeed(cdev->gadget))
> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_hs_streaming_ep);
> - else
> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep);
> + if (gadget_is_superspeed(c->cdev->gadget)) {
> + if (!bulk_streaming_ep)
> + ep = usb_ep_autoconfig_ss(cdev->gadget,
> + &uvc_ss_streaming_ep,
> + &uvc_ss_streaming_comp);
> + else
> + ep = usb_ep_autoconfig_ss(cdev->gadget,
> + &uvc_ss_bulk_streaming_ep,
> + &uvc_ss_bulk_streaming_comp);
> +
> + } else if (gadget_is_dualspeed(cdev->gadget)) {
> + if (!bulk_streaming_ep)
> + ep = usb_ep_autoconfig(cdev->gadget,
> + &uvc_hs_streaming_ep);
> + else
> + ep = usb_ep_autoconfig(cdev->gadget,
> + &uvc_hs_bulk_streaming_ep);
> + } else {
> + if (!bulk_streaming_ep)
> + ep = usb_ep_autoconfig(cdev->gadget,
> + &uvc_fs_streaming_ep);
> + else
> + ep = usb_ep_autoconfig(cdev->gadget,
> + &uvc_fs_bulk_streaming_ep);
> + }
>
> if (!ep) {
> INFO(cdev, "Unable to allocate streaming EP\n");
> @@ -655,9 +839,18 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f) uvc->video.ep = ep;
> ep->driver_data = uvc;
>
> - uvc_fs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> - uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> - uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> + if (!bulk_streaming_ep) {
> + uvc_fs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> + uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> + uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
> + } else {
> + uvc_fs_bulk_streaming_ep.bEndpointAddress =
> + uvc->video.ep->address;
> + uvc_hs_bulk_streaming_ep.bEndpointAddress =
> + uvc->video.ep->address;
> + uvc_ss_bulk_streaming_ep.bEndpointAddress =
> + uvc->video.ep->address;
> + }
>
> /* Allocate interface IDs. */
> if ((ret = usb_interface_id(c, f)) < 0)
> @@ -668,8 +861,12 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f)
>
> if ((ret = usb_interface_id(c, f)) < 0)
> goto error;
> - uvc_streaming_intf_alt0.bInterfaceNumber = ret;
> - uvc_streaming_intf_alt1.bInterfaceNumber = ret;
> + if (!bulk_streaming_ep) {
> + uvc_streaming_intf_alt0.bInterfaceNumber = ret;
> + uvc_streaming_intf_alt1.bInterfaceNumber = ret;
> + } else {
> + uvc_bulk_streaming_intf_alt0.bInterfaceNumber = ret;
> + }
> uvc->streaming_intf = ret;
>
> /* Copy descriptors */
> @@ -806,8 +1003,12 @@ uvc_bind_config(struct usb_configuration *c,
> uvc_control_intf.iInterface =
> uvc_en_us_strings[UVC_STRING_CONTROL_IDX].id;
> ret = uvc_en_us_strings[UVC_STRING_STREAMING_IDX].id;
> - uvc_streaming_intf_alt0.iInterface = ret;
> - uvc_streaming_intf_alt1.iInterface = ret;
> + if (!bulk_streaming_ep) {
> + uvc_streaming_intf_alt0.iInterface = ret;
> + uvc_streaming_intf_alt1.iInterface = ret;
> + } else {
> + uvc_bulk_streaming_intf_alt0.bInterfaceNumber = ret;
> + }
> }
>
> /* Register the function. */
> diff --git a/drivers/usb/gadget/uvc.h b/drivers/usb/gadget/uvc.h
> index 817e9e1..8756853 100644
> --- a/drivers/usb/gadget/uvc.h
> +++ b/drivers/usb/gadget/uvc.h
> @@ -133,6 +133,8 @@ struct uvc_video
>
> struct uvc_video_queue queue;
> unsigned int fid;
> +
> + bool bulk_streaming_ep;
If you agree with the comments above this should be moved to the uvc_device
structure. I would call it streaming_ep_type and use the USB_ENDPOINT_XFER_*
values.
> };
>
> enum uvc_state
> diff --git a/drivers/usb/gadget/uvc_v4l2.c b/drivers/usb/gadget/uvc_v4l2.c
> index ad48e81..c9656dc 100644
> --- a/drivers/usb/gadget/uvc_v4l2.c
> +++ b/drivers/usb/gadget/uvc_v4l2.c
> @@ -250,11 +250,20 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned int cmd,
> void *arg) return ret;
>
> /*
> - * Complete the alternate setting selection setup phase now that
> - * userspace is ready to provide video frames.
> + * Alt settings in an interface are supported only for ISOC
> + * endpoints as there are different alt-settings for
> + * zero-bandwidth and full-bandwidth cases, but the same is not
> + * true for BULK endpoints, as they have a single alt-setting.
> */
> - uvc_function_setup_continue(uvc);
> - uvc->state = UVC_STATE_STREAMING;
> + if (!video->bulk_streaming_ep) {
You can invert the check and return 0 directly. The indentation level below
would then be lowered.
> + /*
> + * Complete the alternate setting selection setup
> + * phase now that userspace is ready to provide video
> + * frames.
> + */
> + uvc_function_setup_continue(uvc);
> + uvc->state = UVC_STATE_STREAMING;
> + }
>
> return 0;
> }
> 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.
>
> for (i = 0; i < UVC_NUM_REQUESTS; ++i) {
> video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL);
> @@ -387,6 +391,9 @@ uvc_video_init(struct uvc_video *video)
> video->height = 240;
> video->imagesize = 320 * 240 * 2;
>
> + if (video->bulk_streaming_ep)
> + video->max_payload_size = video->imagesize;
> +
> /* Initialize the video buffers queue. */
> uvc_queue_init(&video->queue, V4L2_BUF_TYPE_VIDEO_OUTPUT);
> return 0;
--
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
