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.
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
