| Message ID | 20220720144641.3480432-1-m.grzeschik@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usb: gadget: uvc: increase worker prio to WQ_HIGHPRI | expand |
On Wed, Jul 20, 2022 at 04:46:41PM +0200, Michael Grzeschik wrote: > Likewise to the uvcvideo hostside driver, this patch is changing the "Likewise" implies a previous patch being mentioned, which I do not see here :( > simple workqueue to an async_wq with higher priority. This ensures that > the worker will not be scheduled away while the video stream is handled. How will this ensure that? What happens if yet-another higher priority task comes in? This feels like a race that will just never be won without fixing this properly. thanks, greg k-h
On Fri, Aug 19, 2022 at 10:33:32AM +0200, Greg KH wrote: >On Wed, Jul 20, 2022 at 04:46:41PM +0200, Michael Grzeschik wrote: >> Likewise to the uvcvideo hostside driver, this patch is changing the > >"Likewise" implies a previous patch being mentioned, which I do not see >here :( I am not referring another patch but the uvcvideo driver. drivers/media/usb/uvc/uvc_driver.c: 206 /* Allocate a stream specific work queue for asynchronous tasks. */ 207 stream->async_wq = alloc_workqueue("uvcvideo", WQ_UNBOUND | WQ_HIGHPRI, 208 0); I will add a clear reference in the commit message. >> simple workqueue to an async_wq with higher priority. This ensures that >> the worker will not be scheduled away while the video stream is handled. > >How will this ensure that? What happens if yet-another higher priority >task comes in? This feels like a race that will just never be won >without fixing this properly. There is no race between two functions calls. There is a race between userspace filling the gadget isoc driver with data while the hardware is moving it towards the kernel. To ensure no underrun, we increase the priority of the buffer filling thread. Okay. I will probably have to rephrase it to be clear. Thanks, Michael
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index 71669e0e4d0074..241b0de7b4aa52 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -897,10 +897,14 @@ static void uvc_function_unbind(struct usb_configuration *c, { struct usb_composite_dev *cdev = c->cdev; struct uvc_device *uvc = to_uvc(f); + struct uvc_video *video = &uvc->video; long wait_ret = 1; uvcg_info(f, "%s()\n", __func__); + if (video->async_wq) + destroy_workqueue(video->async_wq); + /* * If we know we're connected via v4l2, then there should be a cleanup * of the device from userspace either via UVC_EVENT_DISCONNECT or diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 58e383afdd4406..1a31e6c6a5ffb8 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -88,6 +88,7 @@ struct uvc_video { struct usb_ep *ep; struct work_struct pump; + struct workqueue_struct *async_wq; /* Frame parameters */ u8 bpp; diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c index fd8f73bb726dd1..fddc392b8ab95d 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -170,7 +170,7 @@ uvc_v4l2_qbuf(struct file *file, void *fh, struct v4l2_buffer *b) return ret; if (uvc->state == UVC_STATE_STREAMING) - schedule_work(&video->pump); + queue_work(video->async_wq, &video->pump); return ret; } diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index c00ce0e91f5d5c..bb037fcc90e69e 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -277,7 +277,7 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) spin_unlock_irqrestore(&video->req_lock, flags); if (uvc->state == UVC_STATE_STREAMING) - schedule_work(&video->pump); + queue_work(video->async_wq, &video->pump); } static int @@ -485,7 +485,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) video->req_int_count = 0; - schedule_work(&video->pump); + queue_work(video->async_wq, &video->pump); return ret; } @@ -499,6 +499,11 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) spin_lock_init(&video->req_lock); INIT_WORK(&video->pump, uvcg_video_pump); + /* Allocate a work queue for asynchronous video pump handler. */ + video->async_wq = alloc_workqueue("uvcgadget", WQ_UNBOUND | WQ_HIGHPRI, 0); + if (!video->async_wq) + return -EINVAL; + video->uvc = uvc; video->fcc = V4L2_PIX_FMT_YUYV; video->bpp = 16;
Likewise to the uvcvideo hostside driver, this patch is changing the simple workqueue to an async_wq with higher priority. This ensures that the worker will not be scheduled away while the video stream is handled. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v2 -> v3: - renamed workqueue to "uvcgadget" v1 -> v2: - added destroy_workqueue in uvc_function_unbind - reworded comment above allow_workqueue drivers/usb/gadget/function/f_uvc.c | 4 ++++ drivers/usb/gadget/function/uvc.h | 1 + drivers/usb/gadget/function/uvc_v4l2.c | 2 +- drivers/usb/gadget/function/uvc_video.c | 9 +++++++-- 4 files changed, 13 insertions(+), 3 deletions(-)