RE: [PATCH 4/5] usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

Thanks for your review and sorry for being late with my replies.
I have a lot on my plate these days :)

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx]
> Sent: Tuesday, June 19, 2012 4:19 AM
> To: Bhupesh SHARMA
> Cc: linux-usb@xxxxxxxxxxxxxxx; balbi@xxxxxx; linux-
> media@xxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 4/5] usb: gadget/uvc: Port UVC webcam gadget to use
> videobuf2 framework
>
> 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/

Oops. Ok.

> > 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 :-)

:)
Yes, it seems, I always end up writing them in Camel case.
Will correct this.

> > 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.

Ok.

> >
> > +   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.

Ok.

>
> 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.
>

Yes :)

> > +           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.

I am thinking now whether we really need to support UVC as a capture type
video device. The use cases that I can think of now, UVC always seems to be
a output device.

Any use-case where you think UVC can be a capture device?

> >
> > +   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.

Sure.

> >  {
> > +   mutex_init(&queue->mutex);
> > +   spin_lock_init(&queue->irqlock);
> > +   INIT_LIST_HEAD(&queue->irqqueue);
>
> Please add a blank line here.

Ok.

> > +   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).

Ok.

>
> >  {
> > +   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.

Ok.

>
> >     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.

Oops. Ok

> >             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.

Ok.

> > +                   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.

Ok.

>
> >             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.

Ok.

>
> >     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.

Yes I think these fields are only valid for video capture devices.
As my use-case was only an output UVC video device, I didn't add the same.

Please let me know your views on the same.

>
> > +   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.

Ok.

> >     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.

To be honest, I saw once the 'uvc_video_enable(0)' failing that's why I
added this check. I don't remember the exact instance of the failure, but
I can try to check again and then will come back on the same.

> > +
> > +   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;

Added only after burning my hands :)
In case the buffer was queued at the UVC gadget and the USB cable was disconnected
in the middle of frame transfer, I saw that the buffer was never dequeued with error
and the user-space application kept waiting for this buffer transfer to be completed.

Regards,
Bhupesh
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux