Hi Bhupesh,
Thanks for the patch. Please see below for a couple of small comments.
On Thursday 21 March 2013 13:56:08 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 one issue 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.
>
> Signed-off-by: Bhupesh Sharma <bhupesh.sha...@st.com>
> ---
> drivers/usb/gadget/Kconfig | 1 +
> drivers/usb/gadget/uvc_queue.c | 533 ++++++++++++-------------------------
> drivers/usb/gadget/uvc_queue.h | 25 +--
> drivers/usb/gadget/uvc_v4l2.c | 37 +--
> drivers/usb/gadget/uvc_video.c | 17 +-
> 5 files changed, 188 insertions(+), 425 deletions(-)
[snip]
> diff --git a/drivers/usb/gadget/uvc_queue.c b/drivers/usb/gadget/uvc_queue.c
> index 104ae9c..3f5eeae 100644
> --- a/drivers/usb/gadget/uvc_queue.c
> +++ b/drivers/usb/gadget/uvc_queue.c
[snip]
> +static int uvc_queue_init(struct uvc_video_queue *queue,
> + enum v4l2_buf_type type)
> {
> + int ret;
>
> + 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;
> + ret = vb2_queue_init(&queue->queue);
> + if (ret)
> + return ret;
>
> + mutex_init(&queue->mutex);
> + spin_lock_init(&queue->irqlock);
> + INIT_LIST_HEAD(&queue->irqqueue);
I think you should initialize queue->flags to 0 here.
>
> + return 0;
> }
[snip]
> +static int uvc_queue_buffer(struct uvc_video_queue *queue,
> + struct v4l2_buffer *buf)
> {
> + int ret;
> +
> + mutex_lock(&queue->mutex);
> + ret = vb2_qbuf(&queue->queue, buf);
> + ret = (queue->flags & UVC_QUEUE_PAUSED) != 0;
> + queue->flags &= ~UVC_QUEUE_PAUSED;
Access to the queue flags need to be protected by the irqlock spinlock here
(in addition to the queue mutex).
> + mutex_unlock(&queue->mutex);
>
> + return ret;
> }
[snip]
> @@ -516,26 +283,32 @@ 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;
Shouldn't you keep this line ?
> - 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;
>
> - for (i = 0; i < queue->count; ++i)
> - queue->buffer[i].state = UVC_BUF_STATE_IDLE;
> + spin_lock_irqsave(&queue->irqlock, flags);
> + INIT_LIST_HEAD(&queue->irqqueue);
>
> - queue->flags &= ~UVC_QUEUE_STREAMING;
> + /*
> + * FIXME: We need to clear the DISCONNECTED flag to ensure that
> + * applications will be able to queue buffers for the next
> + * streaming run. However, clearing it here doesn't guarantee
> + * that the device will be reconnected in the meantime.
> + */
> + queue->flags &= ~UVC_QUEUE_DISCONNECTED;
> + spin_unlock_irqrestore(&queue->irqlock, flags);
> }
>
> done:
[snip]
> diff --git a/drivers/usb/gadget/uvc_queue.h b/drivers/usb/gadget/uvc_queue.h
> index 1812a8e..47ad0b8 100644
> --- a/drivers/usb/gadget/uvc_queue.h
> +++ b/drivers/usb/gadget/uvc_queue.h
[snip]
> @@ -25,14 +26,13 @@ enum uvc_buffer_state {
> };
>
> struct uvc_buffer {
> - unsigned long vma_use_count;
> - struct list_head stream;
> -
> - /* Touched by interrupt handler. */
> - struct v4l2_buffer buf;
> + struct vb2_buffer buf;
> struct list_head queue;
> - wait_queue_head_t wait;
> +
> enum uvc_buffer_state state;
> + void *mem;
> + unsigned int length;
> + unsigned int bytesused;
> };
>
> #define UVC_QUEUE_STREAMING (1 << 0)
You can remove the UVC_QUEUE_STREAMIN flag, it's not used anymore.
> @@ -41,26 +41,21 @@ struct uvc_buffer {
> #define UVC_QUEUE_PAUSED (1 << 3)
>
> struct uvc_video_queue {
> - enum v4l2_buf_type type;
> + struct vb2_queue queue;
> + struct mutex mutex; /* Protects queue */
>
> - void *mem;
> unsigned int flags;
> __u32 sequence;
>
> - unsigned int count;
> - unsigned int buf_size;
> unsigned int buf_used;
> - struct uvc_buffer buffer[UVC_MAX_VIDEO_BUFFERS];
> - struct mutex mutex; /* protects buffers and mainqueue */
> - spinlock_t irqlock; /* protects irqqueue */
>
> - struct list_head mainqueue;
> + spinlock_t irqlock; /* Protects irqqueue */
The comment is incorrect, it should be "Protects flags and irqqueue" (it's my
fault, but let's fix it :-)).
> struct list_head irqqueue;
> };
>
> static inline int uvc_queue_streaming(struct uvc_video_queue *queue)
> {
> - return queue->flags & UVC_QUEUE_STREAMING;
> + return vb2_is_streaming(&queue->queue);
> }
>
> #endif /* __KERNEL__ */
[snip]
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
