Hi Bhupesh,
Thanks for the patch. It looks quite good, please see below for various small
comments.
On Friday 01 June 2012 15:08:57 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 MMAO IO method
s/MMAO/MMAP/
> 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
I don't think dequeued and queueing need capitals :-)
> 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.
>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@xxxxxx>
> ---
> drivers/usb/gadget/Kconfig | 1 +
> drivers/usb/gadget/uvc_queue.c | 524 ++++++++++---------------------------
> drivers/usb/gadget/uvc_queue.h | 25 +--
> drivers/usb/gadget/uvc_v4l2.c | 35 ++--
> drivers/usb/gadget/uvc_video.c | 17 +-
> 5 files changed, 184 insertions(+), 418 deletions(-)
[snip]
> diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c
> index 0cdf89d..907ece8 100644
> --- a/drivers/usb/gadget/uvc_queue.c
> +++ b/drivers/usb/gadget/uvc_queue.c
[snip]
This part is a bit difficult to review, as git tried too hard to create a
universal diff where your patch really replaces the code. I'll remove the -
lines to make the comments as readable as possible.
> +/*
> ---------------------------------------------------------------------------
> --
> + * videobuf2 queue operations
> */
> +
> +static int uvc_queue_setup(struct vb2_queue *vq, const struct v4l2_format
> *fmt,
> + unsigned int *nbuffers, unsigned int *nplanes,
> + unsigned int sizes[], void *alloc_ctxs[])
> {
> + struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> + struct uvc_video *video =
> + container_of(queue, struct uvc_video, queue);
No need for a line split.
>
> + if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)
> + *nbuffers = UVC_MAX_VIDEO_BUFFERS;
>
> + *nplanes = 1;
> +
> + sizes[0] = video->imagesize;
>
> return 0;
> }
>
> +static int uvc_buffer_prepare(struct vb2_buffer *vb)
> {
> + struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
> + struct uvc_buffer *buf = container_of(vb, struct uvc_buffer, buf);
>
> + if (vb->v4l2_buf.type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
> + vb2_get_plane_payload(vb, 0) > vb2_plane_size(vb, 0)) {
Please align vb2 with the vb-> on the previous line.
Have you by any chance found some inspiration in
drivers/media/video/uvc/uvc_queue.c ? :-) It would probably make sense to move
this check to vb2 core, but that's outside of the scope of this patch.
> + uvc_trace(UVC_TRACE_CAPTURE, "[E] Bytes used out of bounds.\n");
> + return -EINVAL;
> + }
>
> + if (unlikely(queue->flags & UVC_QUEUE_DISCONNECTED))
> + return -ENODEV;
>
> + buf->state = UVC_BUF_STATE_QUEUED;
> + buf->mem = vb2_plane_vaddr(vb, 0);
> + buf->length = vb2_plane_size(vb, 0);
> + if (vb->v4l2_buf.type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> + buf->bytesused = 0;
> + else
> + buf->bytesused = vb2_get_plane_payload(vb, 0);
The driver doesn't support the capture type at the moment so this might be a
bit overkill, but I think it's a good idea to support capture in the queue
imeplementation. I plan to try and merge the uvcvideo and uvcgadget queue
implementations at some point.
>
> + return 0;
> +}
>
> +static void uvc_buffer_queue(struct vb2_buffer *vb)
> +{
> + struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
> + struct uvc_buffer *buf = container_of(vb, struct uvc_buffer, buf);
> + unsigned long flags;
>
> + spin_lock_irqsave(&queue->irqlock, flags);
>
> + if (likely(!(queue->flags & UVC_QUEUE_DISCONNECTED))) {
> + list_add_tail(&buf->queue, &queue->irqqueue);
> + } else {
> + /* If the device is disconnected return the buffer to userspace
> + * directly. The next QBUF call will fail with -ENODEV.
> + */
> + buf->state = UVC_BUF_STATE_ERROR;
> + vb2_buffer_done(&buf->buf, VB2_BUF_STATE_ERROR);
> }
>
> + spin_unlock_irqrestore(&queue->irqlock, flags);
> }
>
> +static struct vb2_ops uvc_queue_qops = {
> + .queue_setup = uvc_queue_setup,
> + .buf_prepare = uvc_buffer_prepare,
> + .buf_queue = uvc_buffer_queue,
> +};
> +
> +static
> +void uvc_queue_init(struct uvc_video_queue *queue,
> + enum v4l2_buf_type type)
This can fit on two lines. Please align enum with struct.
> {
> + mutex_init(&queue->mutex);
> + spin_lock_init(&queue->irqlock);
> + INIT_LIST_HEAD(&queue->irqqueue);
Please add a blank line here.
> + queue->queue.type = type;
> + queue->queue.io_modes = VB2_MMAP | VB2_USERPTR;
> + queue->queue.drv_priv = queue;
> + queue->queue.buf_struct_size = sizeof(struct uvc_buffer);
> + queue->queue.ops = &uvc_queue_qops;
> + queue->queue.mem_ops = &vb2_vmalloc_memops;
> + vb2_queue_init(&queue->queue);
> }
[snip]
> /*
> + * Allocate the video buffers.
> */
> +static int uvc_alloc_buffers(struct uvc_video_queue *queue,
> + struct v4l2_requestbuffers *rb)
Please align struct with struct (same for the rest of the file).
> {
> + int ret;
>
> + /*
> + * we can support a max of UVC_MAX_VIDEO_BUFFERS video buffers
> + */
> + if (rb->count > UVC_MAX_VIDEO_BUFFERS)
> + rb->count = UVC_MAX_VIDEO_BUFFERS;
>
The check is already present in uvc_queue_setup(), you can remove it here.
> mutex_lock(&queue->mutex);
> + ret = vb2_reqbufs(&queue->queue, rb);
> + mutex_unlock(&queue->mutex);
>
> + return ret ? ret : rb->count;
> +}
[snip]
> @@ -481,10 +250,10 @@ static void uvc_queue_cancel(struct uvc_video_queue
> *queue, int disconnect) spin_lock_irqsave(&queue->irqlock, flags);
> while (!list_empty(&queue->irqqueue)) {
> buf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
> - queue);
> + queue);
No need to change indentation here.
> 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
> * conditions between uvc_queue_buffer and the disconnection event that
> @@ -516,26 +285,28 @@ 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 long flags;
> int ret = 0;
>
> mutex_lock(&queue->mutex);
> if (enable) {
> + ret = vb2_streamon(&queue->queue, queue->queue.type);
> + if (ret < 0)
> goto done;
> +
> queue->buf_used = 0;
> + queue->flags |= UVC_QUEUE_STREAMING;
I think UVC_QUEUE_STREAMING isn't used anymore.
> } else {
> + if (uvc_queue_streaming(queue)) {
The uvcvideo driver doesn't have this check. It thus returns -EINVAL if
VIDIOC_STREAMOFF is called on a stream that is already stopped. I'm not sure
what the right behaviour is, so let's keep the check here until we figure it
out.
> + ret = vb2_streamoff(&queue->queue, queue->queue.type);
> + if (ret < 0)
> + goto done;
> +
> + spin_lock_irqsave(&queue->irqlock, flags);
> + INIT_LIST_HEAD(&queue->irqqueue);
> + queue->flags &= ~UVC_QUEUE_STREAMING;
> + spin_unlock_irqrestore(&queue->irqlock, flags);
> + }
> }
>
> done:
> @@ -543,30 +314,29 @@ done:
> return ret;
> }
>
> -/* called with queue->irqlock held.. */
> +/* called with &queue_irqlock held.. */
> static struct uvc_buffer *
> uvc_queue_next_buffer(struct uvc_video_queue *queue, struct uvc_buffer
> *buf) {
> struct uvc_buffer *nextbuf;
>
> if ((queue->flags & UVC_QUEUE_DROP_INCOMPLETE) &&
> - buf->buf.length != buf->buf.bytesused) {
> + buf->length != buf->bytesused) {
Please keep the indentation.
> buf->state = UVC_BUF_STATE_QUEUED;
> - buf->buf.bytesused = 0;
> + vb2_set_plane_payload(&buf->buf, 0, 0);
> return buf;
> }
>
> list_del(&buf->queue);
> if (!list_empty(&queue->irqqueue))
> nextbuf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
> - queue);
> + queue);
Same here.
> else
> nextbuf = NULL;
>
> - buf->buf.sequence = queue->sequence++;
> - do_gettimeofday(&buf->buf.timestamp);
videobuf2 doesn't fill the sequence number or timestamp fields, so you either
need to keep this here or move it to the caller.
> + vb2_set_plane_payload(&buf->buf, 0, buf->bytesused);
> + vb2_buffer_done(&buf->buf, VB2_BUF_STATE_DONE);
>
> - wake_up(&buf->wait);
> return nextbuf;
> }
>
> @@ -576,7 +346,7 @@ static struct uvc_buffer *uvc_queue_head(struct
> uvc_video_queue *queue)
>
> if (!list_empty(&queue->irqqueue))
> buf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
> - queue);
> + queue);
Please keep the indentation.
> else
> queue->flags |= UVC_QUEUE_PAUSED;
>
[snip]
> diff --git a/drivers/usb/gadget/uvc_v4l2.c b/drivers/usb/gadget/uvc_v4l2.c
> index f6e083b..9c2b45b 100644
> --- a/drivers/usb/gadget/uvc_v4l2.c
> +++ b/drivers/usb/gadget/uvc_v4l2.c
> @@ -144,20 +144,23 @@ uvc_v4l2_release(struct file *file)
> struct uvc_device *uvc = video_get_drvdata(vdev);
> struct uvc_file_handle *handle = to_uvc_file_handle(file->private_data);
> struct uvc_video *video = handle->device;
> + int ret;
>
> uvc_function_disconnect(uvc);
>
> - uvc_video_enable(video, 0);
> - mutex_lock(&video->queue.mutex);
> - if (uvc_free_buffers(&video->queue) < 0)
> - printk(KERN_ERR "uvc_v4l2_release: Unable to free "
> - "buffers.\n");
> - mutex_unlock(&video->queue.mutex);
> + ret = uvc_video_enable(video, 0);
> + if (ret < 0) {
> + printk(KERN_ERR "uvc_v4l2_release: uvc video disable failed\n");
> + return ret;
> + }
This shouldn't prevent uvc_v4l2_release() from succeeding. In practive
uvc_video_enable(0) will never fail, so you can remove the error check.
> +
> + uvc_free_buffers(&video->queue);
>
> file->private_data = NULL;
> v4l2_fh_del(&handle->vfh);
> v4l2_fh_exit(&handle->vfh);
> kfree(handle);
> +
> return 0;
> }
[snip]
> diff --git a/drivers/usb/gadget/uvc_video.c b/drivers/usb/gadget/uvc_video.c
> index b0e53a8..195bbb6 100644
> --- a/drivers/usb/gadget/uvc_video.c
> +++ b/drivers/usb/gadget/uvc_video.c
[snip]
> @@ -161,6 +161,7 @@ static void
> uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
> {
> struct uvc_video *video = req->context;
> + struct uvc_video_queue *queue = &video->queue;
> struct uvc_buffer *buf;
> unsigned long flags;
> int ret;
> @@ -169,13 +170,15 @@ uvc_video_complete(struct usb_ep *ep, struct
> usb_request *req) case 0:
> break;
>
> - case -ESHUTDOWN:
> + case -ESHUTDOWN: /* disconnect from host. */
> printk(KERN_INFO "VS request cancelled.\n");
> + uvc_queue_cancel(queue, 1);
> goto requeue;
>
> default:
> printk(KERN_INFO "VS request completed with status %d.\n",
> req->status);
> + uvc_queue_cancel(queue, 0);
I wonder why there was no uvc_queue_cancel() here already, it makes me a bit
suspicious :-) Have you double-checked this ?
> goto requeue;
> }
--
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
