Hi Michael, On Thursday 02 May 2013 00:27:59 Michael Grzeschik wrote: > On Mon, Apr 29, 2013 at 10:26:49PM +0200, Laurent Pinchart wrote: > > On Monday 29 April 2013 17:56:50 Michael Grzeschik wrote: > > > On Mon, Feb 11, 2013 at 08:42:37PM +0100, Laurent Pinchart wrote: > > > > On Monday 04 February 2013 11:58:27 Michael Grzeschik wrote: > > > > > On Thu, Jan 17, 2013 at 04:23:51PM +0530, 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 | 537 ++++++++++----------------- > > > > > > drivers/usb/gadget/uvc_queue.h | 25 +-- > > > > > > drivers/usb/gadget/uvc_v4l2.c | 27 +-- > > > > > > drivers/usb/gadget/uvc_video.c | 18 +- > > > > > > 5 files changed, 189 insertions(+), 419 deletions(-) > > > > > > > > [snip] > > > > > > > > > > diff --git a/drivers/usb/gadget/uvc_queue.c > > > > > > b/drivers/usb/gadget/uvc_queue.c index 104ae9c..bd20fab 100644 > > > > > > --- a/drivers/usb/gadget/uvc_queue.c > > > > > > +++ b/drivers/usb/gadget/uvc_queue.c > > > > > > > > [snip] > > > > > > > > > > -static int uvc_queue_waiton(struct uvc_buffer *buf, int > > > > > > nonblocking) > > > > > > +static int uvc_queue_buffer(struct uvc_video_queue *queue, > > > > > > + struct v4l2_buffer *buf) > > > > > > > > > > > > { > > > > > > > > > > > > - if (nonblocking) { > > > > > > - return (buf->state != UVC_BUF_STATE_QUEUED && > > > > > > - buf->state != UVC_BUF_STATE_ACTIVE) > > > > > > - ? 0 : -EAGAIN; > > > > > > - } > > > > > > + int ret; > > > > > > > > > > > > - return wait_event_interruptible(buf->wait, > > > > > > - buf->state != UVC_BUF_STATE_QUEUED && > > > > > > - buf->state != UVC_BUF_STATE_ACTIVE); > > > > > > + mutex_lock(&queue->mutex); > > > > > > + ret = vb2_qbuf(&queue->queue, buf); > > > > > > + mutex_unlock(&queue->mutex); > > > > > > + > > > > > > > > > > How is the UVC_QUEUE_PAUSED handling supposed to be handled here? > > > > > > > > > > I see that this patch lost this hunk from uvc_queue_buffer(): > > > > > ret |= (queue->flags & uvc_queue_paused) != 0; > > > > > queue->flags &= ~UVC_QUEUE_PAUSED; > > > > > > > > Good catch. This needs to be handled. The PAUSED flag needs to be set > > > > with the irqlock held, this could be done in uvc_buffer_queue(). The > > > > return value needs to be computed when setting the flag, so it would > > > > need to be stored in the queue structure and read here. Adding a new > > > > UVC_QUEUE_PUMP flag should do but will require taking the irqlock > > > > again to read and clear that flag here. A new field is another > > > > possible solution. > > > > > > the handling of UVC_QUEUE_PAUSED is currently implemented like this: > > > > > > ret = (queue->flags & UVC_QUEUE_PAUSED) != 0 > > > queue->flags &= ~UVC_QUEUE_PAUSED; > > > > > > which is wrong as it overwrites the return value of vb2_qbuf > > > which will lead to an stalled connection if the return value > > > would be negative and got overwritten to "1". > > > > Oops, indeed. > > > > > What do you mean with the extra flag UVC_QUEUE_PUMP. I would like > > > to implement a fixup patch, but need more information how to > > > correctly handle the extra return value here. > > > > Would the following patch fix the issue ? > > Yes, i came up with a similar patch. But i think there is more involved > for a proper flag handling among the different state transitions. There might be more, but I think the patch below is a good first fix. I'll hapilly review better fixes of course :-) Regarding the UVC_QUEUE_PUMP flag I was thinking about a flag that would be set in the vb2 buf_queue operation handler if the queue was empty, and read back in uvc_queue_buffer() to return the correct value to the caller in order to trigger the uvc_video_pump() call. > > From f2349730e362f25e4a9661d1121a09ac02027889 Mon Sep 17 00:00:00 2001 > > From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > Date: Mon, 29 Apr 2013 22:18:01 +0200 > > Subject: [PATCH] usb: gadget/uvc: Fix error handling in uvc_queue_buffer() > > > > The conversion to videobuf2 failed to check the return value of > > vb2_qbuf(). Fix it. > > > > Reported-by: Michael Grzeschik <mgr@xxxxxxxxxxxxxx> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > --- > > > > drivers/usb/gadget/uvc_queue.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/gadget/uvc_queue.c > > b/drivers/usb/gadget/uvc_queue.c index e617047..0bb5d50 100644 > > --- a/drivers/usb/gadget/uvc_queue.c > > +++ b/drivers/usb/gadget/uvc_queue.c > > @@ -193,12 +193,16 @@ static int uvc_queue_buffer(struct uvc_video_queue > > *queue,> > > mutex_lock(&queue->mutex); > > ret = vb2_qbuf(&queue->queue, buf); > > > > + if (ret < 0) > > + goto done; > > + > > > > spin_lock_irqsave(&queue->irqlock, flags); > > ret = (queue->flags & UVC_QUEUE_PAUSED) != 0; > > queue->flags &= ~UVC_QUEUE_PAUSED; > > spin_unlock_irqrestore(&queue->irqlock, flags); > > > > - mutex_unlock(&queue->mutex); > > > > +done: > > + mutex_unlock(&queue->mutex); > > > > return ret; > > > > } > > If you plan to queue that patch for the development branch uvcvideo/uvc- > gadget, please feel free to add: > > Tested-By: Michael Grzeschik <mgr@xxxxxxxxxxxxxx> > > That patch is also a candidate for 3.10 stable. Thank you. -- 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