RE: [PATCH 3/5] usb: gadget/uvc: Add super-speed support to UVC webcam gadget

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

Thanks for your review comments.
Please find my comments inline:

> As Felipe has already applied the patch to his public tree, I'll send
> incremental cleanup patches. Here's my review nonetheless, with a
> question
> which I'd like to know your opinion about to write the cleanup patches.

Not to worry, I can send incremental patches.
Anyways I need to ensure 4/5 and 5/5 patches of the series also cleanly apply
on Felipe's tree as he was facing issues while applying these patches.

Please review 4/5 and 5/5 patches of this patch-set as well and then I can
re-circulate patches for re-review and incorporation in gadget-next.

> On Friday 01 June 2012 15:08:56 Bhupesh Sharma wrote:
> > This patch adds super-speed support to UVC webcam gadget.
> >
> > Also in this patch:
> >     - We add the configurability to pass bInterval, bMaxBurst, mult
> >       factors for video streaming endpoint (ISOC IN) through module
> >       parameters.
> >
> >     - We use config_ep_by_speed helper routine to configure video
> >       streaming endpoint.
> >
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxx>
> > ---
> >  drivers/usb/gadget/f_uvc.c  |  241
> +++++++++++++++++++++++++++++++++++-----
> >  drivers/usb/gadget/f_uvc.h  |    8 +-
> >  drivers/usb/gadget/uvc.h    |    4 +-
> >  drivers/usb/gadget/webcam.c |   29 +++++-
> >  4 files changed, 247 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c
> > index dd7d7a9..2a8bf06 100644
> > --- a/drivers/usb/gadget/f_uvc.c
> > +++ b/drivers/usb/gadget/f_uvc.c
> > @@ -29,6 +29,25 @@
> >
> >  unsigned int uvc_gadget_trace_param;
> >
> > +/*------------------------------------------------------------------
> -------
> > */ +
> > +/* module parameters specific to the Video streaming endpoint */
> > +static unsigned streaming_interval = 1;
> > +module_param(streaming_interval, uint, S_IRUGO|S_IWUSR);
> > +MODULE_PARM_DESC(streaming_interval, "1 - 16");
> > +
> > +static unsigned streaming_maxpacket = 1024;
>
> unsigned int please.

Ok.

> > +module_param(streaming_maxpacket, uint, S_IRUGO|S_IWUSR);
> > +MODULE_PARM_DESC(streaming_maxpacket, "0 - 1023 (fs), 0 - 1024
> (hs/ss)");
> > +
> > +static unsigned streaming_mult;
> > +module_param(streaming_mult, uint, S_IRUGO|S_IWUSR);
> > +MODULE_PARM_DESC(streaming_mult, "0 - 2 (hs/ss only)");
>
> I'd rather like to integrate this into the streaming_maxpacket
> parameter, and
> compute the multiplier at runtime. This shouldn't be difficult for high
> speed,
> the multiplier for max packet sizes between 1 and 1024 is 1, between
> 1025 and
> 2048 is 2 and between 2049 and 3072 is 3.

There is a specific purpose for keeping these three module parameters separate,
with xHCI hosts and USB3 device-side controllers still in stabilization phase, it
is easy for a person switching from USB2.0 to USB3.0 to understand these module
parameters as the major difference points in respect to ISOC IN endpoints.

A similar methodology is already used in the reference USB gadget "zero" (see here [1])
and I have tried to keep the same methodology here as well.

[1] http://git.kernel.org/?p=linux/kernel/git/balbi/usb.git;a=blob;f=drivers/usb/gadget/f_sourcesink.c

> > +static unsigned streaming_maxburst;
> > +module_param(streaming_maxburst, uint, S_IRUGO|S_IWUSR);
> > +MODULE_PARM_DESC(streaming_maxburst, "0 - 15 (ss only)");
>
> Do you think maxburst could also be integrated into the
> streaming_maxpacket
> parameter ? Put it another way, can we computer the multiplier and the
> burst
> value from a single maximum number of bytes per service interval, or do
> they
> have to be specified independently ? If using more than one burst, the
> wMaxPacketSize value must be 1024 for HS. Only multiples of 1024 higher
> than
> 1024 can thus be achieved through different multipler/burst settings.

Pls see above..

> > +
> >  /*
> > ---------------------------------------------------------------------
> -----
> > * Function descriptors
> >   */
> > @@ -84,7 +103,7 @@ static struct usb_interface_descriptor
> uvc_control_intf
> > __initdata = { .iInterface          = 0,
> >  };
> >
> > -static struct usb_endpoint_descriptor uvc_control_ep __initdata = {
> > +static struct usb_endpoint_descriptor uvc_fs_control_ep __initdata =
> {
> >     .bLength                = USB_DT_ENDPOINT_SIZE,
> >     .bDescriptorType        = USB_DT_ENDPOINT,
> >     .bEndpointAddress       = USB_DIR_IN,
> > @@ -124,7 +143,7 @@ static struct usb_interface_descriptor
> > uvc_streaming_intf_alt1 __initdata = { .iInterface          = 0,
> >  };
> >
> > -static struct usb_endpoint_descriptor uvc_streaming_ep = {
> > +static struct usb_endpoint_descriptor uvc_fs_streaming_ep = {
> >     .bLength                = USB_DT_ENDPOINT_SIZE,
> >     .bDescriptorType        = USB_DT_ENDPOINT,
> >     .bEndpointAddress       = USB_DIR_IN,
> > @@ -133,15 +152,72 @@ static struct usb_endpoint_descriptor
> uvc_streaming_ep
> > = { .bInterval              = 1,
> >  };
> >
> > +static struct usb_endpoint_descriptor uvc_hs_streaming_ep = {
> > +   .bLength                = USB_DT_ENDPOINT_SIZE,
> > +   .bDescriptorType        = USB_DT_ENDPOINT,
> > +   .bEndpointAddress       = USB_DIR_IN,
> > +   .bmAttributes           = USB_ENDPOINT_XFER_ISOC,
> > +   .wMaxPacketSize         = cpu_to_le16(1024),
> > +   .bInterval              = 1,
>
> wMaxPacketSize and bInterval are now initialized from module
> parameters, so
> I'd leave it to zero and add a comment about it.

Ok.

> > +};
>
> Please keep the indentation style consistent with the rest of the file.

Ok.

> > +
> > +/* super speed support */
> > +static struct usb_endpoint_descriptor uvc_ss_control_ep __initdata =
> {
> > +   .bLength =              USB_DT_ENDPOINT_SIZE,
> > +   .bDescriptorType =      USB_DT_ENDPOINT,
> > +
> > +   .bEndpointAddress =     USB_DIR_IN,
> > +   .bmAttributes =         USB_ENDPOINT_XFER_INT,
> > +   .wMaxPacketSize =       cpu_to_le16(STATUS_BYTECOUNT),
> > +   .bInterval =            8,
> > +};
>
> The FS/HS/SS control endpoint descriptors are identical, there's no
> need to
> define separate descriptors. You also don't set the SS endpoint number
> to the
> FS endpoint number. As you don't call usb_ep_autoconfig() on the SS
> endpoint,
> I doubt this will work in SS. Have you tested SS support ?

Yes. I have tested the SS support. It works fine :)
usb_ep_autoconfig is usually always called for the lowest speed we support.
See other gadget drivers for reference:

[2] http://git.kernel.org/?p=linux/kernel/git/balbi/usb.git;a=blob;f=drivers/usb/gadget/f_sourcesink.c

> > +
> > +static struct usb_ss_ep_comp_descriptor uvc_ss_control_comp
> __initdata = {
> > +   .bLength =              sizeof uvc_ss_control_comp,
> > +   .bDescriptorType =      USB_DT_SS_ENDPOINT_COMP,
> > +
> > +   /* the following 3 values can be tweaked if necessary */
> > +   /* .bMaxBurst =         0, */
> > +   /* .bmAttributes =      0, */
> > +   .wBytesPerInterval =    cpu_to_le16(STATUS_BYTECOUNT),
> > +};
> > +
> > +static struct usb_endpoint_descriptor uvc_ss_streaming_ep __initdata
> = {
> > +   .bLength =              USB_DT_ENDPOINT_SIZE,
> > +   .bDescriptorType =      USB_DT_ENDPOINT,
> > +
> > +   .bEndpointAddress =     USB_DIR_IN,
> > +   .bmAttributes =         USB_ENDPOINT_XFER_ISOC,
> > +   .wMaxPacketSize =       cpu_to_le16(1024),
> > +   .bInterval =            4,
> > +};
> > +
> > +static struct usb_ss_ep_comp_descriptor uvc_ss_streaming_comp = {
> > +   .bLength =              sizeof uvc_ss_streaming_comp,
>
> The kernel coding style uses sizeof(uvc_ss_streaming_comp).

Ok.

> > +   .bDescriptorType =      USB_DT_SS_ENDPOINT_COMP,
> > +
> > +   /* the following 3 values can be tweaked if necessary */
> > +   .bMaxBurst =            0,
> > +   .bmAttributes = 0,
>
> The two values are commented in uvc_ss_control_comp but not here.

Oops. Will correct in patch respin.

> > +   .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_streaming_ep,
> > +   (struct usb_descriptor_header *) &uvc_fs_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_streaming_ep,
> > +   (struct usb_descriptor_header *) &uvc_hs_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,
> > +   (struct usb_descriptor_header *) &uvc_ss_streaming_comp,
> >     NULL,
> >  };
>
> Those structures are missing __initdata

Ok.

> >
> > @@ -217,6 +293,7 @@ uvc_function_set_alt(struct usb_function *f,
> unsigned
> > interface, unsigned alt) struct uvc_device *uvc = to_uvc(f);
> >     struct v4l2_event v4l2_event;
> >     struct uvc_event *uvc_event = (void *)&v4l2_event.u.data;
> > +   int ret;
> >
> >     INFO(f->config->cdev, "uvc_function_set_alt(%u, %u)\n",
> interface, alt);
> >
> > @@ -264,7 +341,10 @@ uvc_function_set_alt(struct usb_function *f,
> unsigned
> > interface, unsigned alt) return 0;
> >
> >             if (uvc->video.ep) {
> > -                   uvc->video.ep->desc = &uvc_streaming_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);
> >             }
> >
> > @@ -370,9 +450,11 @@ uvc_copy_descriptors(struct uvc_device *uvc,
> enum
> > usb_device_speed speed) {
> >     struct uvc_input_header_descriptor *uvc_streaming_header;
> >     struct uvc_header_descriptor *uvc_control_header;
> > +   const struct uvc_descriptor_header * const *uvc_control_desc;
> >     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;
> > +   static struct usb_endpoint_descriptor *uvc_control_ep;
>
> This doesn't need to be static.

Oops. Yes, you are right.

> >     struct usb_descriptor_header **dst;
> >     struct usb_descriptor_header **hdr;
> >     unsigned int control_size;
> > @@ -381,10 +463,29 @@ uvc_copy_descriptors(struct uvc_device *uvc,
> enum
> > usb_device_speed speed) unsigned int bytes;
> >     void *mem;
> >
> > -   uvc_streaming_cls = (speed == USB_SPEED_FULL)
> > -                     ? uvc->desc.fs_streaming : uvc->desc.hs_streaming;
> > -   uvc_streaming_std = (speed == USB_SPEED_FULL)
> > -                     ? uvc_fs_streaming : uvc_hs_streaming;
> > +   switch (speed) {
> > +   case USB_SPEED_SUPER:
> > +           uvc_control_desc = uvc->desc.ss_control;
> > +           uvc_streaming_cls = uvc->desc.ss_streaming;
> > +           uvc_streaming_std = uvc_ss_streaming;
> > +           uvc_control_ep = &uvc_ss_control_ep;
> > +           break;
> > +
> > +   case USB_SPEED_HIGH:
> > +           uvc_control_desc = uvc->desc.fs_control;
> > +           uvc_streaming_cls = uvc->desc.hs_streaming;
> > +           uvc_streaming_std = uvc_hs_streaming;
> > +           uvc_control_ep = &uvc_fs_control_ep;
> > +           break;
> > +
> > +   case USB_SPEED_FULL:
> > +   default:
> > +           uvc_control_desc = uvc->desc.fs_control;
> > +           uvc_streaming_cls = uvc->desc.fs_streaming;
> > +           uvc_streaming_std = uvc_fs_streaming;
> > +           uvc_control_ep = &uvc_fs_control_ep;
> > +           break;
> > +   }
> >
> >     /* Descriptors layout
> >      *
>
> The comment should be updated with the uvc_ss_control_comp descriptor.

Sure.

> > @@ -402,16 +503,24 @@ uvc_copy_descriptors(struct uvc_device *uvc,
> enum
> > usb_device_speed speed) control_size = 0;
> >     streaming_size = 0;
> >     bytes = uvc_iad.bLength + uvc_control_intf.bLength
> > -         + uvc_control_ep.bLength + uvc_control_cs_ep.bLength
> > +         + uvc_control_ep->bLength + uvc_control_cs_ep.bLength
> >           + uvc_streaming_intf_alt0.bLength;
> > -   n_desc = 5;
> >
> > -   for (src = (const struct usb_descriptor_header**)uvc-
> >desc.control; *src;
> > ++src) { +  if (speed == USB_SPEED_SUPER) {
> > +           bytes += uvc_ss_control_comp.bLength;
> > +           n_desc = 6;
> > +   } else {
> > +           n_desc = 5;
> > +   }
> > +
> > +   for (src = (const struct usb_descriptor_header
> **)uvc_control_desc;
> > +                   *src; ++src) {
> >             control_size += (*src)->bLength;
> >             bytes += (*src)->bLength;
> >             n_desc++;
> >     }
> > -   for (src = (const struct
> usb_descriptor_header**)uvc_streaming_cls; *src;
> > ++src) { +  for (src = (const struct usb_descriptor_header
> > **)uvc_streaming_cls; +                     *src; ++src) {
> >             streaming_size += (*src)->bLength;
> >             bytes += (*src)->bLength;
> >             n_desc++;
> > @@ -435,12 +544,15 @@ uvc_copy_descriptors(struct uvc_device *uvc,
> enum
> > usb_device_speed speed)
> >
> >     uvc_control_header = mem;
> >     UVC_COPY_DESCRIPTORS(mem, dst,
> > -           (const struct usb_descriptor_header**)uvc->desc.control);
> > +           (const struct usb_descriptor_header **)uvc_control_desc);
> >     uvc_control_header->wTotalLength = cpu_to_le16(control_size);
> >     uvc_control_header->bInCollection = 1;
> >     uvc_control_header->baInterfaceNr[0] = uvc->streaming_intf;
> >
> > -   UVC_COPY_DESCRIPTOR(mem, dst, &uvc_control_ep);
> > +   UVC_COPY_DESCRIPTOR(mem, dst, uvc_control_ep);
> > +   if (speed == USB_SPEED_SUPER)
> > +           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);
> >
> > @@ -448,7 +560,8 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum
> > usb_device_speed speed) UVC_COPY_DESCRIPTORS(mem, dst,
> >             (const struct usb_descriptor_header**)uvc_streaming_cls);
> >     uvc_streaming_header->wTotalLength = cpu_to_le16(streaming_size);
> > -   uvc_streaming_header->bEndpointAddress =
> > uvc_streaming_ep.bEndpointAddress;
> > +   uvc_streaming_header->bEndpointAddress =
> > +           uvc_fs_streaming_ep.bEndpointAddress;
> >
> >     UVC_COPY_DESCRIPTORS(mem, dst, uvc_streaming_std);
> >
> > @@ -484,6 +597,7 @@ uvc_function_unbind(struct usb_configuration *c,
> struct
> > usb_function *f)
> >
> >     kfree(f->descriptors);
> >     kfree(f->hs_descriptors);
> > +   kfree(f->ss_descriptors);
> >
> >     kfree(uvc);
> >  }
> > @@ -498,8 +612,26 @@ uvc_function_bind(struct usb_configuration *c,
> struct
> > usb_function *f)
> >
> >     INFO(cdev, "uvc_function_bind\n");
> >
> > +   /* sanity check the streaming endpoint module parameters */
> > +   if (streaming_interval < 1)
> > +           streaming_interval = 1;
> > +   if (streaming_interval > 16)
> > +           streaming_interval = 16;
>
> You can use clamp() instead (although one might argue that it's less
> readable).

Again, I am trying to be as close as possible to "zero" gadget :)

> > +   if (streaming_mult > 2)
> > +           streaming_mult = 2;
> > +   if (streaming_maxburst > 15)
> > +           streaming_maxburst = 15;
> > +
> > +   /*
> > +    * fill in the FS video streaming specific descriptors from the
> > +    * module parameters
> > +    */
> > +   uvc_fs_streaming_ep.wMaxPacketSize = streaming_maxpacket > 1023 ?
> > +                                           1023 : streaming_maxpacket;
> > +   uvc_fs_streaming_ep.bInterval = streaming_interval;
> > +
> >     /* Allocate endpoints. */
> > -   ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
> > +   ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_control_ep);
> >     if (!ep) {
> >             INFO(cdev, "Unable to allocate control EP\n");
> >             goto error;
> > @@ -507,7 +639,7 @@ uvc_function_bind(struct usb_configuration *c,
> struct
> > usb_function *f) uvc->control_ep = ep;
> >     ep->driver_data = uvc;
> >
> > -   ep = usb_ep_autoconfig(cdev->gadget, &uvc_streaming_ep);
> > +   ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep);
>
> This will select an endpoint suitable for FS, but there's no guarantee
> that
> the endpoint will be suitable for FS and HS maximum packet sizes. I
> think
> calling usb_ep_autoconf(_ss) on the endpoint for the highest supported
> speed
> should fix the problem.

Please see explanation of the same given above.

> >     if (!ep) {
> >             INFO(cdev, "Unable to allocate streaming EP\n");
> >             goto error;
> > @@ -528,9 +660,52 @@ uvc_function_bind(struct usb_configuration *c,
> struct
> > usb_function *f) uvc_streaming_intf_alt1.bInterfaceNumber = ret;
> >     uvc->streaming_intf = ret;
> >
> > -   /* Copy descriptors. */
> > +   /* sanity check the streaming endpoint module parameters */
> > +   if (streaming_maxpacket > 1024)
> > +           streaming_maxpacket = 1024;
>
> This should be moved above, with the other sanity checks.

Again, I am trying to be as close as possible to "zero" gadget :)

> > +
> > +   /* Copy descriptors for FS. */
> >     f->descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
> > -   f->hs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_HIGH);
> > +
> > +   /* support high speed hardware */
> > +   if (gadget_is_dualspeed(cdev->gadget)) {
> > +           /*
> > +            * Fill in the HS descriptors from the module parameters
> for the
> > +            * Video Streaming endpoint.
> > +            * NOTE: We assume that the user knows what they are doing
> and
> > +            * won't give parameters that their UDC doesn't support.
> > +            */
> > +           uvc_hs_streaming_ep.wMaxPacketSize = streaming_maxpacket;
> > +           uvc_hs_streaming_ep.wMaxPacketSize |= streaming_mult << 11;
> > +           uvc_hs_streaming_ep.bInterval = streaming_interval;
> > +           uvc_hs_streaming_ep.bEndpointAddress =
> > +                           uvc_fs_streaming_ep.bEndpointAddress;
> > +
> > +           /* Copy descriptors. */
> > +           f->hs_descriptors = uvc_copy_descriptors(uvc,
> USB_SPEED_HIGH);
> > +   }
> > +
> > +   /* support super speed hardware */
> > +   if (gadget_is_superspeed(c->cdev->gadget)) {
> > +           /*
> > +            * Fill in the SS descriptors from the module parameters
> for the
> > +            * Video Streaming endpoint.
> > +            * NOTE: We assume that the user knows what they are doing
> and
> > +            * won't give parameters that their UDC doesn't support.
> > +            */
> > +           uvc_ss_streaming_ep.wMaxPacketSize = streaming_maxpacket;
> > +           uvc_ss_streaming_ep.bInterval = streaming_interval;
> > +           uvc_ss_streaming_comp.bmAttributes = streaming_mult;
> > +           uvc_ss_streaming_comp.bMaxBurst = streaming_maxburst;
> > +           uvc_ss_streaming_comp.wBytesPerInterval =
> > +                   streaming_maxpacket * (streaming_mult + 1) *
> > +                   (streaming_maxburst + 1);
> > +           uvc_ss_streaming_ep.bEndpointAddress =
> > +                           uvc_fs_streaming_ep.bEndpointAddress;
> > +
> > +           /* Copy descriptors. */
> > +           f->ss_descriptors = uvc_copy_descriptors(uvc,
> USB_SPEED_SUPER);
> > +   }
> >
> >     /* Preallocate control endpoint request. */
> >     uvc->control_req = usb_ep_alloc_request(cdev->gadget->ep0,
> GFP_KERNEL);
> > @@ -585,9 +760,11 @@ error:
> >   */
> >  int __init
> >  uvc_bind_config(struct usb_configuration *c,
> > -           const struct uvc_descriptor_header * const *control,
> > +           const struct uvc_descriptor_header * const *fs_control,
> > +           const struct uvc_descriptor_header * const *ss_control,
> >             const struct uvc_descriptor_header * const *fs_streaming,
> > -           const struct uvc_descriptor_header * const *hs_streaming)
> > +           const struct uvc_descriptor_header * const *hs_streaming,
> > +           const struct uvc_descriptor_header * const *ss_streaming)
> >  {
> >     struct uvc_device *uvc;
> >     int ret = 0;
> > @@ -605,21 +782,31 @@ uvc_bind_config(struct usb_configuration *c,
> >     uvc->state = UVC_STATE_DISCONNECTED;
> >
> >     /* Validate the descriptors. */
> > -   if (control == NULL || control[0] == NULL ||
> > -       control[0]->bDescriptorSubType != UVC_VC_HEADER)
> > +   if (fs_control == NULL || fs_control[0] == NULL ||
> > +           fs_control[0]->bDescriptorSubType != UVC_VC_HEADER)
> > +           goto error;
> > +
> > +   if (ss_control == NULL || ss_control[0] == NULL ||
> > +           ss_control[0]->bDescriptorSubType != UVC_VC_HEADER)
> >             goto error;
> >
> >     if (fs_streaming == NULL || fs_streaming[0] == NULL ||
> > -       fs_streaming[0]->bDescriptorSubType != UVC_VS_INPUT_HEADER)
> > +           fs_streaming[0]->bDescriptorSubType != UVC_VS_INPUT_HEADER)
> >             goto error;
>
> Please keep the indentation consistent. This change is useless and just
> makes
> the driver coding style inconsistent.

Ok.

> >
> >     if (hs_streaming == NULL || hs_streaming[0] == NULL ||
> > -       hs_streaming[0]->bDescriptorSubType != UVC_VS_INPUT_HEADER)
> > +           hs_streaming[0]->bDescriptorSubType != UVC_VS_INPUT_HEADER)
> > +           goto error;
> > +
> > +   if (ss_streaming == NULL || ss_streaming[0] == NULL ||
> > +           ss_streaming[0]->bDescriptorSubType != UVC_VS_INPUT_HEADER)
> >             goto error;
> >
> > -   uvc->desc.control = control;
> > +   uvc->desc.fs_control = fs_control;
> > +   uvc->desc.ss_control = ss_control;
> >     uvc->desc.fs_streaming = fs_streaming;
> >     uvc->desc.hs_streaming = hs_streaming;
> > +   uvc->desc.ss_streaming = ss_streaming;
> >
> >     /* maybe allocate device-global string IDs, and patch descriptors
> */
> >     if (uvc_en_us_strings[UVC_STRING_ASSOCIATION_IDX].id == 0) {
> > diff --git a/drivers/usb/gadget/f_uvc.h b/drivers/usb/gadget/f_uvc.h
> > index abf8329..c3d258d 100644
> > --- a/drivers/usb/gadget/f_uvc.h
> > +++ b/drivers/usb/gadget/f_uvc.h
> > @@ -17,9 +17,11 @@
> >  #include <linux/usb/video.h>
> >
> >  extern int uvc_bind_config(struct usb_configuration *c,
> > -                      const struct uvc_descriptor_header * const
> *control,
> > -                      const struct uvc_descriptor_header * const
> *fs_streaming,
> > -                      const struct uvc_descriptor_header * const
> *hs_streaming);
> > +              const struct uvc_descriptor_header * const *fs_control,
> > +              const struct uvc_descriptor_header * const *hs_control,
>
> You're calling the parameter hs_control here and ss_control in the
> implementation.

Ok.

> > +              const struct uvc_descriptor_header * const
> *fs_streaming,
> > +              const struct uvc_descriptor_header * const
> *hs_streaming,
> > +              const struct uvc_descriptor_header * const
> *ss_streaming);
> >
> >  #endif /* _F_UVC_H_ */
> >
> > diff --git a/drivers/usb/gadget/uvc.h b/drivers/usb/gadget/uvc.h
> > index bc78c60..d78ea25 100644
> > --- a/drivers/usb/gadget/uvc.h
> > +++ b/drivers/usb/gadget/uvc.h
> > @@ -153,9 +153,11 @@ struct uvc_device
> >
> >     /* Descriptors */
> >     struct {
> > -           const struct uvc_descriptor_header * const *control;
> > +           const struct uvc_descriptor_header * const *fs_control;
> > +           const struct uvc_descriptor_header * const *ss_control;
> >             const struct uvc_descriptor_header * const *fs_streaming;
> >             const struct uvc_descriptor_header * const *hs_streaming;
> > +           const struct uvc_descriptor_header * const *ss_streaming;
> >     } desc;
> >
> >     unsigned int control_intf;
> > diff --git a/drivers/usb/gadget/webcam.c
> b/drivers/usb/gadget/webcam.c
> > index 668fe12..120e134 100644
> > --- a/drivers/usb/gadget/webcam.c
> > +++ b/drivers/usb/gadget/webcam.c
> > @@ -272,7 +272,15 @@ static const struct
> uvc_color_matching_descriptor
> > uvc_color_matching = { .bMatrixCoefficients = 4,
> >  };
> >
> > -static const struct uvc_descriptor_header * const uvc_control_cls[]
> = {
> > +static const struct uvc_descriptor_header * const
> uvc_fs_control_cls[] = {
> > +   (const struct uvc_descriptor_header *) &uvc_control_header,
> > +   (const struct uvc_descriptor_header *) &uvc_camera_terminal,
> > +   (const struct uvc_descriptor_header *) &uvc_processing,
> > +   (const struct uvc_descriptor_header *) &uvc_output_terminal,
> > +   NULL,
> > +};
> > +
> > +static const struct uvc_descriptor_header * const
> uvc_ss_control_cls[] = {
> >     (const struct uvc_descriptor_header *) &uvc_control_header,
> >     (const struct uvc_descriptor_header *) &uvc_camera_terminal,
> >     (const struct uvc_descriptor_header *) &uvc_processing,
>
> uvc_fs_control_cls and uvc_ss_controls_cls are identical and const, we
> don't
> need two copies.

Hmm. Actually the UVC specification draft committee has not met since the
last 5 yrs. The last updated specification is dated July, 2005. I am
not sure if they will meet again to add some errata for USB3.0 keeping
in mind the new concept of sequence numbers and burst window added in USB3.0
I have kept these placeholders only for that purpose.

If you suggest, I can remove these or better still add a comment here..

> > @@ -304,6 +312,18 @@ static const struct uvc_descriptor_header *
> const
> > uvc_hs_streaming_cls[] = { NULL,
> >  };
> >
> > +static const struct uvc_descriptor_header * const
> uvc_ss_streaming_cls[] =
> > { + (const struct uvc_descriptor_header *) &uvc_input_header,
> > +   (const struct uvc_descriptor_header *) &uvc_format_yuv,
> > +   (const struct uvc_descriptor_header *) &uvc_frame_yuv_360p,
> > +   (const struct uvc_descriptor_header *) &uvc_frame_yuv_720p,
> > +   (const struct uvc_descriptor_header *) &uvc_format_mjpg,
> > +   (const struct uvc_descriptor_header *) &uvc_frame_mjpg_360p,
> > +   (const struct uvc_descriptor_header *) &uvc_frame_mjpg_720p,
> > +   (const struct uvc_descriptor_header *) &uvc_color_matching,
> > +   NULL,
> > +};
> > +
> >  /*
> > ---------------------------------------------------------------------
> -----
> > * USB configuration
> >   */
> > @@ -311,8 +331,9 @@ static const struct uvc_descriptor_header * const
> > uvc_hs_streaming_cls[] = { static int __init
> >  webcam_config_bind(struct usb_configuration *c)
> >  {
> > -   return uvc_bind_config(c, uvc_control_cls, uvc_fs_streaming_cls,
> > -                          uvc_hs_streaming_cls);
> > +   return uvc_bind_config(c, uvc_fs_control_cls, uvc_ss_control_cls,
> > +           uvc_fs_streaming_cls, uvc_hs_streaming_cls,
> > +           uvc_ss_streaming_cls);
> >  }
> >
> >  static struct usb_configuration webcam_config_driver = {
> > @@ -373,7 +394,7 @@ static struct usb_composite_driver webcam_driver
> = {
> >     .name           = "g_webcam",
> >     .dev            = &webcam_device_descriptor,
> >     .strings        = webcam_device_strings,
> > -   .max_speed      = USB_SPEED_HIGH,
> > +   .max_speed      = USB_SPEED_SUPER,
> >     .unbind         = webcam_unbind,
> >  };
>
> --

Thanks,
Bhupesh
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux