Hi Laurent,
> -Original Message-
> From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com]
> Sent: Tuesday, September 1, 2015 2:45 PM
> To: Kaukab, Yousaf
> Cc: linux-media@vger.kernel.org; mche...@osg.samsung.com
> Subject: Re: [RFC PATCH] media: uvcvideo: handle urb completion in a work
> queue
>
> Hello Mian Yousaf,
>
> Thank you for the patch.
Thank you for reviewing it!
>
> On Tuesday 01 September 2015 11:45:11 Mian Yousaf Kaukab wrote:
> > urb completion callback is executed in host controllers interrupt
> > context. To keep preempt disable time short, add an ordered work-
> > queue. Associate a work_struct with each urb and queue work using it
> > on urb completion.
> >
> > In uvc_uninit_video, usb_kill_urb and usb_free_urb are separated in
> > different loops so that workqueue can be destroyed without a lock.
>
> This will change the timing of the uvc_video_clock_decode() call. Have you
> double-checked that it won't cause any issue ? It will also increase the delay
> between end of frame reception and timestamp sampling in
> uvc_video_decode_start(), which I'd like to avoid.
Can this be fixed by saving the timestamp from uvc_video_get_ts() in
uvc_urb_complete() and use it in both uvc_video_decode_start() and
uvc_video_clock_decode()?
>
> > Signed-off-by: Mian Yousaf Kaukab <yousaf.kau...@intel.com>
> > ---
> > drivers/media/usb/uvc/uvc_video.c | 63
> > +---
> > drivers/media/usb/uvc/uvcvideo.h | 9 +-
> > 2 files changed, 60 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c index f839654..943dbd6 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1317,9 +1317,23 @@ static void uvc_video_encode_bulk(struct urb
> > *urb, struct uvc_streaming *stream, urb->transfer_buffer_length =
> > stream->urb_size - len;
> > }
> >
> > -static void uvc_video_complete(struct urb *urb)
> > +static void uvc_urb_complete(struct urb *urb)
> > {
> > - struct uvc_streaming *stream = urb->context;
> > + struct uvc_urb_work *uw = urb->context;
> > + struct uvc_streaming *stream = uw->stream;
> > + /* stream->urb_wq can be set to NULL without lock */
>
> That's sound racy. If stream->urb_wq can be set to NULL and the work queue
> destroyed by uvc_uninit_video() in parallel to the URB completion handler, the
> work queue could be destroyed between the if (wq) check and the call to
> queue_work().
>
steam->urb_wq is set to NULL after killing all urbs. There should be
no completion callback when its NULL. This is the reason for two for-
loops in uvc_uninit_video()
> > + struct workqueue_struct *wq = stream->urb_wq;
> > +
> > + if (wq)
> > + queue_work(wq, >work);
> > +}
> > +
> > +static void uvc_video_complete_work(struct work_struct *work) {
> > + struct uvc_urb_work *uw = container_of(work, struct
> uvc_urb_work,
> > +
> work);
> > + struct urb *urb = uw->urb;
> > + struct uvc_streaming *stream = uw->stream;
> > struct uvc_video_queue *queue = >queue;
> > struct uvc_buffer *buf = NULL;
> > unsigned long flags;
> > @@ -1445,17 +1459,34 @@ static void uvc_uninit_video(struct
> > uvc_streaming *stream, int free_buffers) {
> > struct urb *urb;
> > unsigned int i;
> > + struct workqueue_struct *wq;
> >
> > uvc_video_stats_stop(stream);
> >
> > + /* Kill all URB first so that urb_wq can be destroyed without a
> lock
> > +*/
> > for (i = 0; i < UVC_URBS; ++i) {
> > - urb = stream->urb[i];
> > + urb = stream->uw[i].urb;
> > if (urb == NULL)
> > continue;
> >
> > usb_kill_urb(urb);
> > + }
> > +
> > + if (stream->urb_wq) {
> > + wq = stream->urb_wq;
> > + /* Since all URBs are killed set urb_wq to NULL */
> > + stream->urb_wq = NULL;
> > + flush_workqueue(wq);
> > + destroy_workqueue(wq);
>
> Does the work queue really need to be destroyed every time the video stream
> is stopped ? It looks to me like we could initialize it when the driver is
> initialized
> and destroy it only when the device is disconnected.
>
Probably yes. But why keep it when it's not in use?
> > + }
> > +
> > + for (i = 0; i < UVC_URBS; ++i) {
> > +
