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 <[email protected]> > --- > 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. > 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. > + 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 ? > - 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. > + 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 ? > 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, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
