Hi Laurent, > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx] > Sent: Wednesday, March 27, 2013 11:25 PM > To: Bhupesh SHARMA > Cc: linux-usb@xxxxxxxxxxxxxxx; balbi@xxxxxx; bhupesh.linux@xxxxxxxxx; > mgr@xxxxxxxxxxxxxx; gang.chen@xxxxxxxxxxx; tipecaml@xxxxxxxxx > Subject: Re: [PATCH V4 1/2] usb: gadget/uvc: Port UVC webcam gadget to > use videobuf2 framework > > 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.sharma@xxxxxx> > > --- > > 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] > > -- All comments accepted. I will send out V5 shortly. Regards, Bhupesh -- 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
