Hi Laurent, Thanks for your review. > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx] > Sent: Wednesday, August 01, 2012 6:40 PM > To: Bhupesh SHARMA > Cc: linux-usb@xxxxxxxxxxxxxxx; balbi@xxxxxx > Subject: Re: [PATCH V2 2/2] usb: gadget/uvc: Add support for > 'USB_GADGET_DELAYED_STATUS' response for a set_intf(alt-set 1) command > > Hi Bhupesh, > > Thanks for the patch. Just a couple of small comments. > > On Tuesday 31 July 2012 06:24:52 Bhupesh Sharma wrote: > > This patch adds the support in UVC webcam gadget design for providing > > USB_GADGET_DELAYED_STATUS in response to a set_interface(alt setting > 1) > > command issue by the Host. > > > > The current UVC webcam gadget design generates a STREAMON event > > corresponding to a set_interface(alt setting 1) command from the > Host. > > This STREAMON event will eventually be routed to a real V4L2 device. > > > > To start video streaming, it may be required to perform some register > > writes to a camera sensor device over slow external busses like I2C > or SPI. > > So, it makes sense to ensure that we delay the STATUS stage of the > > set_interface(alt setting 1) command. > > > > Otherwise, a lot of ISOC IN tokens sent by the Host will be replied > to > > by zero-length packets by the webcam device. On certain Hosts this > may even > > lead to ISOC URBs been cancelled from the Host side. > > > > So, as soon as we finish doing all the "streaming" related stuff on > the > > real V4L2 device, we call a STREAMON ioctl on the UVC side and from > here we > > call the 'usb_composite_setup_continue' function to complete the > status > > stage of the set_interface(alt setting 1) command. > > > > Further, we need to ensure that we queue no video buffers on the UVC > > webcam gadget, until we de-queue a video buffer from the V4L2 device. > > So, the application should call the STREAMON on UVC side only when it > has > > dequeued sufficient buffers from the V4L2 side and queued them to the > > UVC gadget. > > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxx> > > --- > > drivers/usb/gadget/f_uvc.c | 16 +++++++++++----- > > drivers/usb/gadget/uvc.h | 1 + > > drivers/usb/gadget/uvc_v4l2.c | 15 ++++++++++++++- > > 3 files changed, 26 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c > > index 2a8bf06..22f7570 100644 > > --- a/drivers/usb/gadget/f_uvc.c > > +++ b/drivers/usb/gadget/f_uvc.c > > @@ -272,6 +272,13 @@ uvc_function_setup(struct usb_function *f, const > struct > > usb_ctrlrequest *ctrl) return 0; > > } > > > > +void uvc_function_setup_continue(struct uvc_device *uvc) > > +{ > > + struct usb_composite_dev *cdev = uvc->func.config->cdev; > > + > > + usb_composite_setup_continue(cdev); > > +} > > + > > static int > > uvc_function_get_alt(struct usb_function *f, unsigned interface) > > { > > @@ -334,7 +341,8 @@ uvc_function_set_alt(struct usb_function *f, > unsigned > > interface, unsigned alt) v4l2_event_queue(uvc->vdev, &v4l2_event); > > > > uvc->state = UVC_STATE_CONNECTED; > > - break; > > + > > No need for two blank lines here. Oops. Ok. > > + return 0; > > > > case 1: > > if (uvc->state != UVC_STATE_CONNECTED) > > @@ -352,14 +360,12 @@ uvc_function_set_alt(struct usb_function *f, > unsigned > > interface, unsigned alt) v4l2_event.type = UVC_EVENT_STREAMON; > > v4l2_event_queue(uvc->vdev, &v4l2_event); > > > > - uvc->state = UVC_STATE_STREAMING; > > - break; > > + > > Same here. Ok. > > + return USB_GADGET_DELAYED_STATUS; > > > > default: > > return -EINVAL; > > } > > - > > - return 0; > > } > > > > static void > > diff --git a/drivers/usb/gadget/uvc.h b/drivers/usb/gadget/uvc.h > > index 93b0c11..9391993 100644 > > --- a/drivers/usb/gadget/uvc.h > > +++ b/drivers/usb/gadget/uvc.h > > @@ -190,6 +190,7 @@ struct uvc_file_handle > > * Functions > > */ > > > > +extern void uvc_function_setup_continue(struct uvc_device *uvc); > > extern void uvc_endpoint_stream(struct uvc_device *dev); > > > > extern void uvc_function_connect(struct uvc_device *uvc); > > diff --git a/drivers/usb/gadget/uvc_v4l2.c > b/drivers/usb/gadget/uvc_v4l2.c > > index 134bfe5..b47e0f7 100644 > > --- a/drivers/usb/gadget/uvc_v4l2.c > > +++ b/drivers/usb/gadget/uvc_v4l2.c > > @@ -245,7 +245,20 @@ uvc_v4l2_do_ioctl(struct file *file, unsigned > int cmd, > > void *arg) if (*type != video->queue.queue.type) > > return -EINVAL; > > > > - return uvc_video_enable(video, 1); > > + /* enable uvc video. */ > > Please start comments with a capital letter and spell UVC in uppercase. :) Regarding the first letter as a capital letter, I don't find any such guideline in the Documentation Style :), though I agree on spelling UVC in capitals. > > + ret = uvc_video_enable(video, 1); > > + if (ret < 0) > > + return ret; > > + > > + /* > > + * since the real video device has now started streaming > > + * its safe now to complete the status phase of the > > + * set_interface (alt setting 1) > > + */ > > I would change this comment, as not all use cases of the UVC gadget > driver > involve a real video capture device. What about something like > > "Complete the alternate setting selection setup phase now that > userspace is > ready to provide video frames." Not really, even your dummy UVC test application provides the video frames but it doesn't need to wait for data coming from a real video source. This change is required only for real video sources which are placed over slow busses like I2C/SPI and may take time to start streaming (and hence outputting 1st useful video frame). Dummy sources can always keep videobuffers queued at the UVC side (as the data is anyways fake) at the very start, so they can immediately return the status stage of set-alt(1). This is different from the real video sources as there you will have to wait for a real videobuffer filled with data before queuing the same at the UVC end and then completing the status phase of the set-alt(1) So, in my view the part "userspace is ready to provide video frames" doesn't explain the exact use case. Pls feel free to disagree though :) Regards, Bhupesh > > + uvc_function_setup_continue(uvc); > > + uvc->state = UVC_STATE_STREAMING; > > + > > + return 0; > > } > > > > case VIDIOC_STREAMOFF: > -- > Regards, > > Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
