Re: [PATCH V2 1/2] usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework

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

 



Hi Bhupesh,

On Tuesday 07 August 2012 23:07:56 Bhupesh SHARMA wrote:
> On Tuesday, August 07, 2012 8:23 PM Laurent Pinchart wrote:
> > On Tuesday 31 July 2012 06:24:51 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 two issues 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.
> > > 	
> > > 	- In case the underlying UDC has already returned -ESHUTDOWN
> > > 	
> > > 	  as status of a queued request, it means that the video
> > > 	  streaming endpoint is going to be disabled and hence the
> > > 	  underlying UDC will giveback all queued requests with a status
> > > 	  of -ESHUTDOWN. In such a case, the requests are not longer
> > > 	  queued at the UDC end and it doesn't make sense to dequeue
> > > 	  them again in uvc_video_enable(0).

[snip]

> > > diff --git a/drivers/usb/gadget/uvc_queue.c
> > 
> > b/drivers/usb/gadget/uvc_queue.c
> > 
> > > index 104ae9c..bf6621b 100644
> > > --- a/drivers/usb/gadget/uvc_queue.c
> > > +++ b/drivers/usb/gadget/uvc_queue.c

[snip]

> > > @@ -516,26 +276,34 @@ 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;
> > > -		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;
> > > +
> > > +		spin_lock_irqsave(&queue->irqlock, flags);
> > > +
> > > +		INIT_LIST_HEAD(&queue->irqqueue);
> > > 
> > > -		for (i = 0; i < queue->count; ++i)
> > > -			queue->buffer[i].state = UVC_BUF_STATE_IDLE;
> > > +		/*
> > > +		 * as the uvc queue is being disabled, clear any
> > > +		 * DISCONNECTED flag which was set earlier, so that the
> > > +		 * next qbuf from userspace does not fail with ENODEV.
> > > +		 */
> > 
> > Please start the comment with a capital letter, spell UVC in capital,
> > and fill the space up to 80 columns.
> 
> Sure.
> 
> > > +		if (queue->flags & UVC_QUEUE_DISCONNECTED)
> > > +			queue->flags &= ~UVC_QUEUE_DISCONNECTED;
> > 
> > You can clear the flag unconditionally. What about moving this to the
> > enable branch of the if ?
> 
> Ok for the first sentence of the comment.
> For the 2nd part, we have to do it here itself as if the application
> accessing this UVC webcam is closed and then started again (sending a
> set-alt(0) and set-alt(1)), we will not be able to queue any buffers on the
> UVC gadget in the 2nd run.
> 
> This is because we had set the UVC_QUEUE_DISCONNECTED, when the underlying
> UDC had returned a USB request with completion status -104 (ESHUTDOWN).
> 
> Any attempt to queue the buffers without clearing this flag will return
> -ENODEV error. And as uvc_queue_enable(1) is called when we STREAMON ioctl
> is called by the user-space, any qbuf calls before that will fail.

Right, I understand the problem, but I've got one issue with this.

The purpose of the UVC_QUEUE_DISCONNECTED flag is to prevent buffers from 
being queued when the device has been disconnected, as those buffers would 
never be marked as complete (no URB in flight, so no URB complete callback) 
and would cause applications to wait forever on VIDIOC_DQBUF (assuming they 
use the blocking mode).

The UVC host driver uses the same mechanism, and never clears the flag as a 
disconnected device will never reappear (a new device instance will instead be 
created if the device is replugged). However, the UVC gadget driver is 
different, as the device will not be destroyed when disconnected. The flag 
thus needs to be cleared.

Clearing the flag in uvc_queue_enable() means that applications will be able 
to queue buffers after VIDIOC_STREAMOFF, even if the device is disconnected. 
I'm not sure how to fix that though, we could leave this for later.

If you don't fix this issue now, please change the above comment to something 
like

/* 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_STREAMING;
> > > +		spin_unlock_irqrestore(&queue->irqlock, flags);
> > >  	}
> > >  
> > >  done:

[snip]

> > > diff --git a/drivers/usb/gadget/uvc_video.c
> > > b/drivers/usb/gadget/uvc_video.c
> > > index b0e53a8..d29a67d 100644
> > > --- a/drivers/usb/gadget/uvc_video.c
> > > +++ b/drivers/usb/gadget/uvc_video.c
> > 
> > [snip]
> > 
> > > @@ -332,6 +335,7 @@ uvc_video_enable(struct uvc_video *video, int
> > > enable)
> > >  {
> > >  	unsigned int i;
> > >  	int ret;
> > > +	struct uvc_video_queue *queue = &video->queue;

Please move this line above the "unsigned int i".

> > > 
> > >  	if (video->ep == NULL) {
> > >  	
> > >  		printk(KERN_INFO "Video enable failed, device is "
> > > 
> > > @@ -340,8 +344,13 @@ uvc_video_enable(struct uvc_video *video, int
> > > enable)
> > >  	}
> > >  	
> > >  	if (!enable) {
> > > -		for (i = 0; i < UVC_NUM_REQUESTS; ++i)
> > > -			usb_ep_dequeue(video->ep, video->req[i]);
> > > +		/*
> > > +		 * dequeue requests on underlying UDC only if
> > > +		 * -ESHUTDOWN was not asserted by UDC earlier
> > > +		 */
> > > +		if (!(queue->flags & UVC_QUEUE_DISCONNECTED))
> > > +			for (i = 0; i < UVC_NUM_REQUESTS; ++i)
> > > +				usb_ep_dequeue(video->ep, video->req[i]);
> > 
> > What happens if you omit this check ?
> 
> Actually the underlying UDCs clean-up their respective request list when
> they see a disconnect event from the Host.
> 
> This makes them giveback all USB requests queued by an gadget with a status
> of ESHUTDOWN, so if we try to dequeue the 'already' dequeued requests from
> the UDC, it throws warning/errors that it doesn't have these requests
> queued any longer with it.

OK. In that case, I'd like to fix the problem globally. When we run out of 
V4L2 buffers the requests will end up being stored in the req_free list by 
uvc_video_complete(). Those requests are not queued, and should thus not be 
dequeued.

Could you please split this change to another patch, and only dequeue requests 
that are not queued ? This would require proper locking.

Another option would be to turn the warning/error messages into debug messages 
(some drivers - namely MUSB - already outputs a debug message in the 
usb_ep_dequeue handler if the request is not queued). I wonder whether that 
wouldn't be a better solution.

> > >  		uvc_video_free_requests(video);
> > >  		uvc_queue_enable(&video->queue, 0);

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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux