[PATCH 1/1] usb: gadget/uvc: Add support for Bulk endpoint to be used as Video Streaming ep

[Bhupesh Sharma]

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-***@public.gmane.org>
---
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).

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)");
+
/* --------------------------------------------------------------------------
* 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,
+};
+
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,
+};
+
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,
+};
+
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,
+};
+
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),
+};
+
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,
+};
+
/* --------------------------------------------------------------------------
* 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;
}

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;

- /* 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) {
+ /*
+ * 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)
+ 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;
}

- 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;
};

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) {
+ /*
+ * 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);

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;

[Laurent Pinchart]

Hi Bhupesh,

Thank you for the patch. Please see below for comments.

Post by Bhupesh Sharma
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.
---
Note that to ease review and integration of this patch, I have rebased it
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.

Post by Bhupesh Sharma
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 ?

Post by Bhupesh Sharma
/* ------------------------------------------------------------------------
* 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) ?

Post by Bhupesh Sharma
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).

Post by Bhupesh Sharma
+};
+
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.

Post by Bhupesh Sharma
+};
+

Can you please group the bulk streaming endpoint descriptors together (either
before or after the isoc streaming endpoint descriptors) ?

Post by Bhupesh Sharma
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.

Post by Bhupesh Sharma
+};
+
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.
*/

Post by Bhupesh Sharma
+};
+
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 ?

Post by Bhupesh Sharma
/*
--------------------------------------------------------------------------
* 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;

Post by Bhupesh Sharma
}
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).

Post by Bhupesh Sharma
- /* TODO
- if (usb_endpoint_xfer_bulk(&uvc->desc.vs_ep))
- return alt ? -EINVAL : 0;
- */
+ if (!bulk_streaming_ep) {
+ switch (alt) {
+ 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) {
- if (uvc->state != UVC_STATE_STREAMING)
+ uvc->state = UVC_STATE_CONNECTED;
return 0;
- if (uvc->video.ep)
- usb_ep_disable(uvc->video.ep);
+ 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;
- if (uvc->state != UVC_STATE_CONNECTED)
+ return -EINVAL;
+ }
+ } else {
+ switch (uvc->state) {
+ if (!uvc->video.ep->driver_data) {

When can this happen ?

Post by Bhupesh Sharma
+ /*
+ * 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);
- }
+ 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);
- return -EINVAL;
+ uvc->state = UVC_STATE_CONNECTED;
+ return 0;
+
+ 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.

Post by Bhupesh Sharma
+ 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 ?

Post by Bhupesh Sharma
- 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.

Post by Bhupesh Sharma
};
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.

Post by Bhupesh Sharma
+ /*
+ * 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.

Post by Bhupesh Sharma
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;

[Bhupesh SHARMA]

Hi Laurent,

Thanks for your review comments.

Post by Laurent Pinchart
Hi Bhupesh,
Thank you for the patch. Please see below for comments.

Post by Bhupesh Sharma
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.
---
Note that to ease review and integration of this patch, I have rebased it
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.

Post by Bhupesh Sharma
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 ?

Yes. Explicit values seem better. So I will add this in V2.
Any value other than ISOC and BULK will be an flagged as an invalid
module argument.

Post by Laurent Pinchart

Post by Bhupesh Sharma
/* ------------------------------------------------------------------------
* 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) ?

Ok.

Post by Laurent Pinchart

Post by Bhupesh Sharma
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).

Ok.

Post by Laurent Pinchart

Post by Bhupesh Sharma
+};
+
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.

Ok.

Post by Laurent Pinchart

Post by Bhupesh Sharma
+};
+

Can you please group the bulk streaming endpoint descriptors together (either
before or after the isoc streaming endpoint descriptors) ?

Sure.

Post by Laurent Pinchart

Post by Bhupesh Sharma
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.

Ok.

Post by Laurent Pinchart

Post by Bhupesh Sharma
+};
+
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.
*/

Post by Bhupesh Sharma
+};
+

Ok.

Post by Laurent Pinchart

Post by Bhupesh Sharma
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 ?

Sure.

Post by Laurent Pinchart

Post by Bhupesh Sharma
/*
--------------------------------------------------------------------------
* 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;

Ok. Will add this in V2.

Post by Laurent Pinchart

Post by Bhupesh Sharma
}
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.

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.

Post by Laurent Pinchart
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.

Post by Laurent Pinchart

Post by Bhupesh Sharma
- /* TODO
- if (usb_endpoint_xfer_bulk(&uvc->desc.vs_ep))
- return alt ? -EINVAL : 0;
- */
+ if (!bulk_streaming_ep) {
+ switch (alt) {
+ 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) {
- if (uvc->state != UVC_STATE_STREAMING)
+ uvc->state = UVC_STATE_CONNECTED;
return 0;
- if (uvc->video.ep)
- usb_ep_disable(uvc->video.ep);
+ 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;
- if (uvc->state != UVC_STATE_CONNECTED)
+ return -EINVAL;
+ }
+ } else {
+ switch (uvc->state) {
+ if (!uvc->video.ep->driver_data) {

When can this happen ?

I will check this again and get back.

Post by Laurent Pinchart

Post by Bhupesh Sharma
+ /*
+ * 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);
- }
+ 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);
- return -EINVAL;
+ uvc->state = UVC_STATE_CONNECTED;
+ return 0;
+
+ 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.

Ok.

Post by Laurent Pinchart

Post by Bhupesh Sharma
+ 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 ?

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.

Post by Laurent Pinchart

Post by Bhupesh Sharma
- 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.

Ok.

Post by Laurent Pinchart

Post by Bhupesh Sharma
};
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.

Ok.

Post by Laurent Pinchart

Post by Bhupesh Sharma
+ /*
+ * 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.

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.

Post by Laurent Pinchart

Post by Bhupesh Sharma
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,
Bhupesh
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Laurent Pinchart]

Hi Bhupesh,

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
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.
---
Note that to ease review and integration of this patch, I have rebased it
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.

Post by Bhupesh Sharma
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]

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
@@ -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.

"[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.

Post by Laurent Pinchart
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.

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
- /* TODO
- if (usb_endpoint_xfer_bulk(&uvc->desc.vs_ep))
- return alt ? -EINVAL : 0;
- */
+ if (!bulk_streaming_ep) {
+ switch (alt) {
+ 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) {
- if (uvc->state != UVC_STATE_STREAMING)
+ uvc->state = UVC_STATE_CONNECTED;
return 0;
- if (uvc->video.ep)
- usb_ep_disable(uvc->video.ep);
+ 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;
- if (uvc->state != UVC_STATE_CONNECTED)
+ return -EINVAL;
+ }
+ } else {
+ switch (uvc->state) {
+ if (!uvc->video.ep->driver_data) {

When can this happen ?

I will check this again and get back.

Post by Laurent Pinchart

Post by Bhupesh Sharma
+ /*
+ * 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);
- }
+ 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);
- return -EINVAL;
+ uvc->state = UVC_STATE_CONNECTED;
+ return 0;
+
+ return -EINVAL;
+ }
}
}

[snip]

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
@@ -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
- 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.

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
- 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]

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
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;

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
for (i = 0; i < UVC_NUM_REQUESTS; ++i) {
video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL);

[Bhupesh SHARMA]

Hi Laurent,

Post by Laurent Pinchart
Hi Bhupesh,

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
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.
---
Note that to ease review and integration of this patch, I have rebased it
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.

Post by Bhupesh Sharma
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]

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
@@ -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.

"[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.

Post by Laurent Pinchart
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.

Post by Laurent Pinchart

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
- /* TODO
- if (usb_endpoint_xfer_bulk(&uvc->desc.vs_ep))
- return alt ? -EINVAL : 0;
- */
+ if (!bulk_streaming_ep) {
+ switch (alt) {
+ 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) {
- if (uvc->state != UVC_STATE_STREAMING)
+ uvc->state = UVC_STATE_CONNECTED;
return 0;
- if (uvc->video.ep)
- usb_ep_disable(uvc->video.ep);
+ 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;
- if (uvc->state != UVC_STATE_CONNECTED)
+ return -EINVAL;
+ }
+ } else {
+ switch (uvc->state) {
+ if (!uvc->video.ep->driver_data) {

When can this happen ?

I will check this again and get back.

Post by Laurent Pinchart

Post by Bhupesh Sharma
+ /*
+ * 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);
- }
+ 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);
- return -EINVAL;
+ uvc->state = UVC_STATE_CONNECTED;
+ return 0;
+
+ return -EINVAL;
+ }
}
}

[snip]

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
@@ -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
- 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.

Post by Laurent Pinchart

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
- 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]

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
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

Post by Laurent Pinchart

Post by Bhupesh SHARMA

Post by Laurent Pinchart

Post by Bhupesh Sharma
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-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html