Hi Laurent, Thanks for your review. > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx] > Sent: Tuesday, August 07, 2012 8:23 PM > To: Bhupesh SHARMA > Cc: linux-usb@xxxxxxxxxxxxxxx; balbi@xxxxxx > Subject: Re: [PATCH V2 1/2] usb: gadget/uvc: Port UVC webcam gadget to > use videobuf2 framework > > Hi Bhupesh, > > Thank you for the patch. > > On Tuesday 31 July 2012 06:24:51 Bhupesh Sharma wrote: > > This patch reworks the videobuffer management logic present in the > UVC > > webcam gadget and ports it to use the "more apt" videobuf2 framework > for > > video buffer management. > > > > To support routing video data captured from a real V4L2 video capture > > device with a "zero copy" operation on videobuffers (as they pass > from > > the V4L2 domain to UVC domain via a user-space application), we need > to > > support USER_PTR IO method at the UVC gadget side. > > > > So the V4L2 capture device driver can still continue to use MMAP IO > > method and now the user-space application can just pass a pointer to > the > > video buffers being dequeued from the V4L2 device side while queueing > them > > at the UVC gadget end. This ensures that we have a "zero-copy" design > as > > the videobuffers pass from the V4L2 capture device to the UVC gadget. > > > > Note that there will still be a need to apply UVC specific payload > > headers on top of each UVC payload data, which will still require a > copy > > operation to be performed in the 'encode' routines of the UVC gadget. > > > > This patch also addresses two issues found out while porting the UVC > > gadget to videobuf2 framework: > > - In case the usb requests queued by the gadget get completed > > with a status of -ESHUTDOWN (disconnected from host), the > > queue of videobuf2 should be cancelled to ensure that the > > application space daemon is not left in a state waiting for > > a vb2 to be successfully absorbed at the USB side. > > > > - In case the underlying UDC has already returned -ESHUTDOWN > > as status of a queued request, it means that the video > > streaming endpoint is going to be disabled and hence the > > underlying UDC will giveback all queued requests with a status > > of -ESHUTDOWN. In such a case, the requests are not longer > > queued at the UDC end and it doesn't make sense to dequeue > > them again in uvc_video_enable(0). > > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxx> > > --- > > drivers/usb/gadget/uvc_queue.c | 531 ++++++++++++------------------ > ------- > > drivers/usb/gadget/uvc_queue.h | 25 +-- > > drivers/usb/gadget/uvc_v4l2.c | 27 +-- > > drivers/usb/gadget/uvc_video.c | 28 ++- > > 4 files changed, 190 insertions(+), 421 deletions(-) > > Shouldn't you select VIDEOBUF2_VMALLOC in Kconfig ? That was done in v1 > but > you seem to have dropped it here. Oops. My Bad. Will add this in V3. > > diff --git a/drivers/usb/gadget/uvc_queue.c > b/drivers/usb/gadget/uvc_queue.c > > index 104ae9c..bf6621b 100644 > > --- a/drivers/usb/gadget/uvc_queue.c > > +++ b/drivers/usb/gadget/uvc_queue.c > > [snip] > > > @@ -484,9 +243,10 @@ static void uvc_queue_cancel(struct > uvc_video_queue > > *queue, int disconnect) queue); > > list_del(&buf->queue); > > buf->state = UVC_BUF_STATE_ERROR; > > - wake_up(&buf->wait); > > + vb2_buffer_done(&buf->buf, VB2_BUF_STATE_ERROR); > > } > > - /* This must be protected by the irqlock spinlock to avoid race > > + /* > > + * This must be protected by the irqlock spinlock to avoid race > > * conditions between uvc_queue_buffer and the disconnection > event that > > * could result in an interruptible wait in uvc_dequeue_buffer. > Do not > > * blindly replace this logic by checking for the > UVC_DEV_DISCONNECTED > > @@ -516,26 +276,34 @@ static void uvc_queue_cancel(struct > uvc_video_queue > > *queue, int disconnect) */ > > static int uvc_queue_enable(struct uvc_video_queue *queue, int > enable) > > { > > - unsigned int i; > > + unsigned long flags; > > int ret = 0; > > > > mutex_lock(&queue->mutex); > > if (enable) { > > - if (uvc_queue_streaming(queue)) { > > - ret = -EBUSY; > > + ret = vb2_streamon(&queue->queue, queue->queue.type); > > + if (ret < 0) > > goto done; > > - } > > - queue->sequence = 0; > > - queue->flags |= UVC_QUEUE_STREAMING; > > + > > queue->buf_used = 0; > > } else { > > - uvc_queue_cancel(queue, 0); > > - INIT_LIST_HEAD(&queue->mainqueue); > > + ret = vb2_streamoff(&queue->queue, queue->queue.type); > > + if (ret < 0) > > + goto done; > > + > > + spin_lock_irqsave(&queue->irqlock, flags); > > + > > + INIT_LIST_HEAD(&queue->irqqueue); > > > > - for (i = 0; i < queue->count; ++i) > > - queue->buffer[i].state = UVC_BUF_STATE_IDLE; > > + /* > > + * as the uvc queue is being disabled, clear any > > + * DISCONNECTED flag which was set earlier, so that the > > + * next qbuf from userspace does not fail with ENODEV. > > + */ > > Please start the comment with a capital letter, spell UVC in capital, > and fill > the space up to 80 columns. Sure. > > + if (queue->flags & UVC_QUEUE_DISCONNECTED) > > + queue->flags &= ~UVC_QUEUE_DISCONNECTED; > > You can clear the flag unconditionally. What about moving this to the > enable > branch of the if ? Ok for the first sentence of the comment. For the 2nd part, we have to do it here itself as if the application accessing this UVC webcam is closed and then started again (sending a set-alt(0) and set-alt(1)), we will not be able to queue any buffers on the UVC gadget in the 2nd run. This is because we had set the UVC_QUEUE_DISCONNECTED, when the underlying UDC had returned a USB request with completion status -104 (ESHUTDOWN). Any attempt to queue the buffers without clearing this flag will return -ENODEV error. And as uvc_queue_enable(1) is called when we STREAMON ioctl is called by the user-space, any qbuf calls before that will fail. > > > - queue->flags &= ~UVC_QUEUE_STREAMING; > > + spin_unlock_irqrestore(&queue->irqlock, flags); > > } > > > > done: > > [snip] > > > @@ -563,10 +331,18 @@ uvc_queue_next_buffer(struct uvc_video_queue > *queue, > > struct uvc_buffer *buf) else > > nextbuf = NULL; > > > > - buf->buf.sequence = queue->sequence++; > > - do_gettimeofday(&buf->buf.timestamp); > > + /* > > + * FIXME: with videobuf2, the sequence number or timestamp fields > > + * are valid only for video capture devices and the UVC gadget > > + * usually is a video output device. Keeping these until the > > + * specs are clear on this aspect. > > + */ > > Please fill the space up to 80 columns. Ok. > > + buf->buf.v4l2_buf.sequence = queue->sequence++; > > + do_gettimeofday(&buf->buf.v4l2_buf.timestamp); > > + > > + vb2_set_plane_payload(&buf->buf, 0, buf->bytesused); > > + vb2_buffer_done(&buf->buf, VB2_BUF_STATE_DONE); > > > > - wake_up(&buf->wait); > > return nextbuf; > > } > > > > [snip] > > > diff --git a/drivers/usb/gadget/uvc_video.c > b/drivers/usb/gadget/uvc_video.c > > index b0e53a8..d29a67d 100644 > > --- a/drivers/usb/gadget/uvc_video.c > > +++ b/drivers/usb/gadget/uvc_video.c > > [snip] > > > @@ -332,6 +335,7 @@ uvc_video_enable(struct uvc_video *video, int > enable) > > { > > unsigned int i; > > int ret; > > + struct uvc_video_queue *queue = &video->queue; > > > > if (video->ep == NULL) { > > printk(KERN_INFO "Video enable failed, device is " > > @@ -340,8 +344,13 @@ uvc_video_enable(struct uvc_video *video, int > enable) > > } > > > > if (!enable) { > > - for (i = 0; i < UVC_NUM_REQUESTS; ++i) > > - usb_ep_dequeue(video->ep, video->req[i]); > > + /* > > + * dequeue requests on underlying UDC only if > > + * -ESHUTDOWN was not asserted by UDC earlier > > + */ > > + if (!(queue->flags & UVC_QUEUE_DISCONNECTED)) > > + for (i = 0; i < UVC_NUM_REQUESTS; ++i) > > + usb_ep_dequeue(video->ep, video->req[i]); > > What happens if you omit this check ? Actually the underlying UDCs clean-up their respective request list when they see a disconnect event from the Host. This makes them giveback all USB requests queued by an gadget with a status of ESHUTDOWN, so if we try to dequeue the 'already' dequeued requests from the UDC, it throws warning/errors that it doesn't have these requests queued any longer with it. > > uvc_video_free_requests(video); > > uvc_queue_enable(&video->queue, 0); > > @@ -382,4 +391,3 @@ uvc_video_init(struct uvc_video *video) > > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
