Message ID | 20220907215818.2670097-1-m.grzeschik@pengutronix.de |
---|---|
State | New |
Headers | show |
Series | [v4] usb: gadget: uvc: increase worker prio to WQ_HIGHPRI | expand |
Hi Michael, On Wed, Sep 07, 2022 at 11:58:18PM +0200, Michael Grzeschik wrote: > This patch is changing the simple workqueue in the gadget driver to be > allocated as async_wq with a higher priority. The pump worker, that is > filling the usb requests, will have a higher priority and will not be > scheduled away so often while the video stream is handled. This will > lead to fewer streaming underruns. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> ... > 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; I am commenting here because this is the most recent change but after this showed up in -next as commit 9b91a6523078 ("usb: gadget: uvc: increase worker prio to WQ_HIGHPRI"), I see the following warning/error when building s390 allmodconfig: In file included from ../include/linux/string.h:253, from ../include/linux/bitmap.h:11, from ../include/linux/cpumask.h:12, from ../include/linux/smp.h:13, from ../include/linux/lockdep.h:14, from ../include/linux/rcupdate.h:29, from ../include/linux/rculist.h:11, from ../include/linux/pid.h:5, from ../include/linux/sched.h:14, from ../include/linux/ratelimit.h:6, from ../include/linux/dev_printk.h:16, from ../include/linux/device.h:15, from ../drivers/usb/gadget/function/f_uvc.c:9: In function ‘fortify_memset_chk’, inlined from ‘uvc_register_video’ at ../drivers/usb/gadget/function/f_uvc.c:424:2: ../include/linux/fortify-string.h:301:25: error: call to ‘__write_overflow_field’ declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning] 301 | __write_overflow_field(p_size_field, size); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This commit did not directly cause this, it just made the issue more obvious. In commit e4ce9ed835bc ("usb: gadget: uvc: ensure the vdev is unset"), also authored by you, the size parameter appears to be wrong? It is using the size of 'struct uvc_video', instead of the size of 'struct video_device'. It appears to be pure luck that everything worked up until this point, as those two types had the same size (1400 bytes) before this change but now 'struct uvc_video' is 1408 bytes, meaning there is now an overwrite. Any reason this is not the fix? Cheers, Nathan diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index e6948cf8def3..836601227155 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -421,7 +421,7 @@ uvc_register_video(struct uvc_device *uvc) int ret; /* TODO reference counting. */ - memset(&uvc->vdev, 0, sizeof(uvc->video)); + memset(&uvc->vdev, 0, sizeof(uvc->vdev)); uvc->vdev.v4l2_dev = &uvc->v4l2_dev; uvc->vdev.v4l2_dev->dev = &cdev->gadget->dev; uvc->vdev.fops = &uvc_v4l2_fops;
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index f4f6cf75930beb..09961f4ca981da 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 511f106f984375..d6dbf9b763b2e1 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;
This patch is changing the simple workqueue in the gadget driver to be allocated as async_wq with a higher priority. The pump worker, that is filling the usb requests, will have a higher priority and will not be scheduled away so often while the video stream is handled. This will lead to fewer streaming underruns. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v3 -> v4: - improved commit message to be more clear 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(-)