[RFC PATCH] media: uvcvideo: handle urb completion in a work queue

[RFC PATCH] media: uvcvideo: handle urb completion in a work queue

[Mian Yousaf Kaukab]

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.

Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@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 */
+	struct workqueue_struct *wq = stream->urb_wq;
+
+	if (wq)
+		queue_work(wq, &uw->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 = &stream->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);
+	}
+
+	for (i = 0; i < UVC_URBS; ++i) {
+		urb = stream->uw[i].urb;
+		if (urb == NULL)
+			continue;
+
 		usb_free_urb(urb);
-		stream->urb[i] = NULL;
+		stream->uw[i].urb = NULL;
 	}
 
 	if (free_buffers)
@@ -1514,7 +1545,7 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
 		}
 
 		urb->dev = stream->dev->udev;
-		urb->context = stream;
+		urb->context = &stream->uw[i];
 		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
 				ep->desc.bEndpointAddress);
 #ifndef CONFIG_DMA_NONCOHERENT
@@ -1525,7 +1556,7 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
 #endif
 		urb->interval = ep->desc.bInterval;
 		urb->transfer_buffer = stream->urb_buffer[i];
-		urb->complete = uvc_video_complete;
+		urb->complete = uvc_urb_complete;
 		urb->number_of_packets = npackets;
 		urb->transfer_buffer_length = size;
 
@@ -1534,7 +1565,9 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
 			urb->iso_frame_desc[j].length = psize;
 		}
 
-		stream->urb[i] = urb;
+		stream->uw[i].urb = urb;
+		stream->uw[i].stream = stream;
+		INIT_WORK(&stream->uw[i].work, uvc_video_complete_work);
 	}
 
 	return 0;
@@ -1580,14 +1613,16 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
 		}
 
 		usb_fill_bulk_urb(urb, stream->dev->udev, pipe,
-			stream->urb_buffer[i], size, uvc_video_complete,
-			stream);
+			stream->urb_buffer[i], size, uvc_urb_complete,
+			&stream->uw[i]);
 #ifndef CONFIG_DMA_NONCOHERENT
 		urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
 		urb->transfer_dma = stream->urb_dma[i];
 #endif
 
-		stream->urb[i] = urb;
+		stream->uw[i].urb = urb;
+		stream->uw[i].stream = stream;
+		INIT_WORK(&stream->uw[i].work, uvc_video_complete_work);
 	}
 
 	return 0;
@@ -1676,9 +1711,15 @@ static int uvc_init_video(struct uvc_streaming *stream, gfp_t gfp_flags)
 	if (ret < 0)
 		return ret;
 
+	stream->urb_wq = alloc_ordered_workqueue(stream->dev->name, 0);
+	if (!stream->urb_wq) {
+		uvc_printk(KERN_ERR, "Workqueue allocation failed\n");
+		return -ENOMEM;
+	}
+
 	/* Submit the URBs. */
 	for (i = 0; i < UVC_URBS; ++i) {
-		ret = usb_submit_urb(stream->urb[i], gfp_flags);
+		ret = usb_submit_urb(stream->uw[i].urb, gfp_flags);
 		if (ret < 0) {
 			uvc_printk(KERN_ERR, "Failed to submit URB %u "
 					"(%d).\n", i, ret);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 816dd1a..e2c0617b 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -440,6 +440,12 @@ struct uvc_stats_stream {
 	unsigned int max_sof;		/* Maximum STC.SOF value */
 };
 
+struct uvc_urb_work {
+	struct urb *urb;
+	struct uvc_streaming *stream;
+	struct work_struct work;
+};
+
 struct uvc_streaming {
 	struct list_head list;
 	struct uvc_device *dev;
@@ -482,7 +488,8 @@ struct uvc_streaming {
 		__u32 max_payload_size;
 	} bulk;
 
-	struct urb *urb[UVC_URBS];
+	struct workqueue_struct *urb_wq;
+	struct uvc_urb_work uw[UVC_URBS];
 	char *urb_buffer[UVC_URBS];
 	dma_addr_t urb_dma[UVC_URBS];
 	unsigned int urb_size;
-- 
2.3.3

Re: [RFC PATCH] media: uvcvideo: handle urb completion in a work queue

[Laurent Pinchart]

Hello Mian Yousaf,

Thank you for the patch.

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.

> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@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().

> +	struct workqueue_struct *wq = stream->urb_wq;
> +
> +	if (wq)
> +		queue_work(wq, &uw->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 = &stream->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.

> +	}
> +
> +	for (i = 0; i < UVC_URBS; ++i) {
> +		urb = stream->uw[i].urb;
> +		if (urb == NULL)
> +			continue;
> +
>  		usb_free_urb(urb);
> -		stream->urb[i] = NULL;
> +		stream->uw[i].urb = NULL;
>  	}
> 
>  	if (free_buffers)
> @@ -1514,7 +1545,7 @@ static int uvc_init_video_isoc(struct uvc_streaming
> *stream, }
> 
>  		urb->dev = stream->dev->udev;
> -		urb->context = stream;
> +		urb->context = &stream->uw[i];
>  		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
>  				ep->desc.bEndpointAddress);
>  #ifndef CONFIG_DMA_NONCOHERENT
> @@ -1525,7 +1556,7 @@ static int uvc_init_video_isoc(struct uvc_streaming
> *stream, #endif
>  		urb->interval = ep->desc.bInterval;
>  		urb->transfer_buffer = stream->urb_buffer[i];
> -		urb->complete = uvc_video_complete;
> +		urb->complete = uvc_urb_complete;
>  		urb->number_of_packets = npackets;
>  		urb->transfer_buffer_length = size;
> 
> @@ -1534,7 +1565,9 @@ static int uvc_init_video_isoc(struct uvc_streaming
> *stream, urb->iso_frame_desc[j].length = psize;
>  		}
> 
> -		stream->urb[i] = urb;
> +		stream->uw[i].urb = urb;
> +		stream->uw[i].stream = stream;
> +		INIT_WORK(&stream->uw[i].work, uvc_video_complete_work);
>  	}
> 
>  	return 0;
> @@ -1580,14 +1613,16 @@ static int uvc_init_video_bulk(struct uvc_streaming
> *stream, }
> 
>  		usb_fill_bulk_urb(urb, stream->dev->udev, pipe,
> -			stream->urb_buffer[i], size, uvc_video_complete,
> -			stream);
> +			stream->urb_buffer[i], size, uvc_urb_complete,
> +			&stream->uw[i]);
>  #ifndef CONFIG_DMA_NONCOHERENT
>  		urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
>  		urb->transfer_dma = stream->urb_dma[i];
>  #endif
> 
> -		stream->urb[i] = urb;
> +		stream->uw[i].urb = urb;
> +		stream->uw[i].stream = stream;
> +		INIT_WORK(&stream->uw[i].work, uvc_video_complete_work);
>  	}
> 
>  	return 0;
> @@ -1676,9 +1711,15 @@ static int uvc_init_video(struct uvc_streaming
> *stream, gfp_t gfp_flags) if (ret < 0)
>  		return ret;
> 
> +	stream->urb_wq = alloc_ordered_workqueue(stream->dev->name, 0);
> +	if (!stream->urb_wq) {
> +		uvc_printk(KERN_ERR, "Workqueue allocation failed\n");
> +		return -ENOMEM;
> +	}
> +
>  	/* Submit the URBs. */
>  	for (i = 0; i < UVC_URBS; ++i) {
> -		ret = usb_submit_urb(stream->urb[i], gfp_flags);
> +		ret = usb_submit_urb(stream->uw[i].urb, gfp_flags);
>  		if (ret < 0) {
>  			uvc_printk(KERN_ERR, "Failed to submit URB %u "
>  					"(%d).\n", i, ret);
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 816dd1a..e2c0617b 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -440,6 +440,12 @@ struct uvc_stats_stream {
>  	unsigned int max_sof;		/* Maximum STC.SOF value */
>  };
> 
> +struct uvc_urb_work {
> +	struct urb *urb;
> +	struct uvc_streaming *stream;
> +	struct work_struct work;
> +};
> +
>  struct uvc_streaming {
>  	struct list_head list;
>  	struct uvc_device *dev;
> @@ -482,7 +488,8 @@ struct uvc_streaming {
>  		__u32 max_payload_size;
>  	} bulk;
> 
> -	struct urb *urb[UVC_URBS];
> +	struct workqueue_struct *urb_wq;
> +	struct uvc_urb_work uw[UVC_URBS];
>  	char *urb_buffer[UVC_URBS];
>  	dma_addr_t urb_dma[UVC_URBS];
>  	unsigned int urb_size;

-- 
Regards,

Laurent Pinchart

RE: [RFC PATCH] media: uvcvideo: handle urb completion in a work queue

[Kaukab, Yousaf]

Hi Laurent,

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Tuesday, September 1, 2015 2:45 PM
> To: Kaukab, Yousaf
> Cc: linux-media@vger.kernel.org; mchehab@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.kaukab@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, &uw->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 = &stream->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) {
> > +		urb = stream->uw[i].urb;
> > +		if (urb == NULL)
> > +			continue;
> > +
> >  		usb_free_urb(urb);
> > -		stream->urb[i] = NULL;
> > +		stream->uw[i].urb = NULL;
> >  	}
> >
> >  	if (free_buffers)
> > @@ -1514,7 +1545,7 @@ static int uvc_init_video_isoc(struct
> > uvc_streaming *stream, }
> >
> >  		urb->dev = stream->dev->udev;
> > -		urb->context = stream;
> > +		urb->context = &stream->uw[i];
> >  		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
> >  				ep-
> >desc.bEndpointAddress);
> >  #ifndef CONFIG_DMA_NONCOHERENT
> > @@ -1525,7 +1556,7 @@ static int uvc_init_video_isoc(struct
> > uvc_streaming *stream, #endif
> >  		urb->interval = ep->desc.bInterval;
> >  		urb->transfer_buffer = stream->urb_buffer[i];
> > -		urb->complete = uvc_video_complete;
> > +		urb->complete = uvc_urb_complete;
> >  		urb->number_of_packets = npackets;
> >  		urb->transfer_buffer_length = size;
> >
> > @@ -1534,7 +1565,9 @@ static int uvc_init_video_isoc(struct
> > uvc_streaming *stream, urb->iso_frame_desc[j].length = psize;
> >  		}
> >
> > -		stream->urb[i] = urb;
> > +		stream->uw[i].urb = urb;
> > +		stream->uw[i].stream = stream;
> > +		INIT_WORK(&stream->uw[i].work,
> uvc_video_complete_work);
> >  	}
> >
> >  	return 0;
> > @@ -1580,14 +1613,16 @@ static int uvc_init_video_bulk(struct
> > uvc_streaming *stream, }
> >
> >  		usb_fill_bulk_urb(urb, stream->dev->udev, pipe,
> > -			stream->urb_buffer[i], size,
> uvc_video_complete,
> > -			stream);
> > +			stream->urb_buffer[i], size,
> uvc_urb_complete,
> > +			&stream->uw[i]);
> >  #ifndef CONFIG_DMA_NONCOHERENT
> >  		urb->transfer_flags =
> URB_NO_TRANSFER_DMA_MAP;
> >  		urb->transfer_dma = stream->urb_dma[i];  #endif
> >
> > -		stream->urb[i] = urb;
> > +		stream->uw[i].urb = urb;
> > +		stream->uw[i].stream = stream;
> > +		INIT_WORK(&stream->uw[i].work,
> uvc_video_complete_work);
> >  	}
> >
> >  	return 0;
> > @@ -1676,9 +1711,15 @@ static int uvc_init_video(struct uvc_streaming
> > *stream, gfp_t gfp_flags) if (ret < 0)
> >  		return ret;
> >
> > +	stream->urb_wq = alloc_ordered_workqueue(stream->dev-
> >name, 0);
> > +	if (!stream->urb_wq) {
> > +		uvc_printk(KERN_ERR, "Workqueue allocation
> failed\n");
> > +		return -ENOMEM;
> > +	}
> > +
> >  	/* Submit the URBs. */
> >  	for (i = 0; i < UVC_URBS; ++i) {
> > -		ret = usb_submit_urb(stream->urb[i], gfp_flags);
> > +		ret = usb_submit_urb(stream->uw[i].urb,
> gfp_flags);
> >  		if (ret < 0) {
> >  			uvc_printk(KERN_ERR, "Failed to
> submit URB %u "
> >
> 	"(%d).\n", i, ret);
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h
> > b/drivers/media/usb/uvc/uvcvideo.h index 816dd1a..e2c0617b 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -440,6 +440,12 @@ struct uvc_stats_stream {
> >  	unsigned int max_sof;		/* Maximum
> STC.SOF value */
> >  };
> >
> > +struct uvc_urb_work {
> > +	struct urb *urb;
> > +	struct uvc_streaming *stream;
> > +	struct work_struct work;
> > +};
> > +
> >  struct uvc_streaming {
> >  	struct list_head list;
> >  	struct uvc_device *dev;
> > @@ -482,7 +488,8 @@ struct uvc_streaming {
> >  		__u32 max_payload_size;
> >  	} bulk;
> >
> > -	struct urb *urb[UVC_URBS];
> > +	struct workqueue_struct *urb_wq;
> > +	struct uvc_urb_work uw[UVC_URBS];
> >  	char *urb_buffer[UVC_URBS];
> >  	dma_addr_t urb_dma[UVC_URBS];
> >  	unsigned int urb_size;
> 
> --
> Regards,
> 
> Laurent Pinchart

BR,
Yousaf

Re: [RFC PATCH] media: uvcvideo: handle urb completion in a work queue

[Laurent Pinchart]

Hi Mian Yousaf,

(CC'ing linux-usb for tips regarding proper handling of URBs in work queues)

On Tuesday 01 September 2015 13:49:31 Kaukab, Yousaf wrote:
> On Tuesday, September 1, 2015 2:45 PM Laurent Pinchart wrote:
> > 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()?

Yes, I think that would work. I think it's especially important in 
uvc_video_decode_start(). For uvc_video_clock_decode() it might not matter (I 
won't mind if you investigate whether it's needed ;-)), but if you use the 
saved timestamp there, you should also save the USB frame number along with 
the timestamp as they must match.

> >> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@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()

Indeed, I've missed that.

There's still at least one race condition though. The URB completion handler 
uvc_video_complete() is now called from the work queue. It could thus race 
usb_kill_urb(), which will make resubmission of the URB with usb_submit_urb() 
return -EPERM. The driver will then print an error message to the kernel log 
that could worry the user unnecessarily.

I'm in general a bit wary regarding race conditions, and especially when a 
complex function that used to run synchronously is moved to a work queue. I'm 
wondering whether it wouldn't be better to use a lock, as contention would 
only occur at stream stop time.

Could you please double-check possible race conditions ? Keeping the work 
queue around for the whole duration of the device life time might also help 
simplifying the code, but I haven't investigated that.

Another idea that just came to my mind, wouldn't it be better to add URBs to a 
list in their synchronous completion handler and use a normal work queue ? If 
several URBs complete in a row we could possibly avoid some scheduling context 
switches.

> >> +	struct workqueue_struct *wq = stream->urb_wq;
> >> +
> >> +	if (wq)
> >> +		queue_work(wq, &uw->work);
> >> +}

[snip]

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

It's a matter of resources consumed by the work queue vs. the time spent to 
create it when starting the stream, as well as code complexity. 

> >> +	}
> >> +
> >> +	for (i = 0; i < UVC_URBS; ++i) {
> >> +		urb = stream->uw[i].urb;
> >> +		if (urb == NULL)
> >> +			continue;
> >> +
> >>  		usb_free_urb(urb);
> >> 
> >> -		stream->urb[i] = NULL;
> >> +		stream->uw[i].urb = NULL;
> >>  	}
> >>  	
> >>  	if (free_buffers)

-- 
Regards,

Laurent Pinchart