Hi Laurent, > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx] > Sent: Wednesday, August 08, 2012 5:32 PM > To: Bhupesh SHARMA > Cc: linux-usb@xxxxxxxxxxxxxxx; balbi@xxxxxx > Subject: Re: [PATCH V2 1/2] usb: gadget/uvc: Port UVC webcam gadget to > use videobuf2 framework > > 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. > */ Ok. Will add this comment in V3. > > > > - 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". Ok. > > > > > > > > > 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. Ok, but I have noticed that the underlying UDC will usually return all USB requests queued by the gadget when DISCONNECT happens. This means that no USB requests will be queued at UDC end when uvc_video_enable(0) is called. So if UDC saw a DISCONNECT from the Host, all the USB requests defined by the macro UVC_NUM_REQUESTS will become members of req_free list. So, in reality we don't have to dequeue any USB request on the UDC end, when uvc_video_enable(0) is called during DISCONNECT case (this is different from the case where user-space application itself working at the UVC gadget side is closed, in which case these requests will still be either queued to UDC or will be members of req_free list). In DISCONNECT from host, USB requests out of the UVC_NUM_REQUESTS count that were not already queued to the UDC are anyways not required to be dequeued and the rest have already been dequeued by the UDC itself. So, no USB requests need to be dequeued in this case. This is exactly what my patch does. Or am I missing something here? > 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. Yes, but that isn't a very neat solution. We are trying to dequeue USB requests from UDC when there are none queued there (not sure many UDC maintainers will agree to this). As the gadget saw the USB requests completed with ESHUTDOWN status, it has the sufficient information to make a correct decision on which requests to dequeue now. But, please feel free to let me know your ideas on the same :) Regards, Bhupesh > > > > 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
