| Message ID | 1441100711-24519-1-git-send-email-yousaf.kaukab@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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;
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(-)