On Mon, Apr 29, 2013 at 10:26:49PM +0200, Laurent Pinchart wrote:
> Hi Michael,
>
> 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.
> 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.
Thanks,
Michael Grzeschik
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
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
