Hi Laurent, > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx] > Sent: Monday, June 11, 2012 12:55 PM > To: Bhupesh SHARMA > Cc: linux-usb@xxxxxxxxxxxxxxx; balbi@xxxxxx; linux- > media@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 3/5] usb: gadget/uvc: Add super-speed support to > UVC webcam gadget > > Hi Bhupesh, > > (Dropping Greg from the CC list, I think he gets enough e-mails already > without our help :-)) Ofcourse :) > On Saturday 09 June 2012 13:39:34 Bhupesh SHARMA wrote: > > 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. > > I've already prepared incremental patches so I'll send them. Ok. > > 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. > > I will review 4/5 and 5/5 and ask you to post a new version. I'll send > incremental patches for 1/5 to 3/5 myself. Sure. > > > > 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> > > [snip] > > > > > +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=driver > s/us > > b/gadget/f_sourcesink.c > > Having another driver implementing the same doesn't automatically make > it > right ;-) > > I still think the maxpacket and mult values should be combined into a > single > parameter. There's a single way to split a given number of bytes into > maxpacket and mult values. Felipe (and others), any opinion on this ? Ok. I will ask Felipe and others to pitch in as well before we finalize our approach. > > > > > +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.. > > I'll keep this parameter separate. When maxburst is non-zero the > maxpacket > value must be a multiple of 1024. If the user-specified value is > incorrect the > driver should error out. > > I forgot to mention that S_IWUSR looks unsafe to me. If we allow > changing the > mackpacket, mult and maxburst values at runtime, the function could be > bound > after one of the values have been changed but not the others. > > [snip] > > > > > + > > > > +/* 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=driver > s/us > > b/gadget/f_sourcesink.c > > Why is that ? It looks like a bug to me. > Felipe, can you please provide the reason why usb_ep_autoconfig is usually always called for the lowest speed we support. > > > > @@ -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 :) > > This is the UVC function gadget, it's allowed to be itself. > Hmm.. Ok. > > > > + 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. > > Please see my disagreement to your above explanation :-) > > [snip] > > > > > 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.. > > We don't need different descriptors now, and there's no clear > indication that > we will need them in the very near future. Let's not add unneeded code > to the > kernel now, we'll split the descriptors later when/if needed. > Ok. Lets remove the copy and keep only one set. Regards, 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
