Re: [RESEND v2 2/3] usb: gadget: uvc: increase worker prio to WQ_HIGHPRI

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 08, 2022 at 09:41:30PM +0300, Laurent Pinchart wrote:
Hi Michael,

Thank you for the patch.

On Wed, Jun 08, 2022 at 01:03:38PM +0200, Michael Grzeschik wrote:
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@xxxxxxxxxxxxxx>

---
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(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index d3feeeb50841b8..dcc5f057810973 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -891,10 +891,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
 	 * though the video device removal uevent. Allow some time for the
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 a9bb4553db847e..9a9101851bc1e8 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
@@ -478,7 +478,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;
 }
@@ -492,6 +492,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("uvcvideo", WQ_UNBOUND | WQ_HIGHPRI, 0);

Let's call it "uvcgadget" (or "uvc gadget", "uvc-gadget", ...) as
"uvcvideo" refers to the host side driver.

I'm still a bit worried about WQ_UNBOUND and the risk of running work
items in parallel on different CPUs. uvcg_video_pump() looks mostly
safe, as it protects video->req_free with a spinlock, and the buffer
queue with another spinlock. The req_int_count increment at the end of
the loop would be unsafe though.

I looked into this again. But am still a bit unsure.

Why exactly would req_int_count be unsafe?

I thought WQ_UNBOUND is just making sure, that the workqueue could be
scheduled on any CPU, independent of the calling CPU waking the WQ. The
function uvcg_video_pump would than be called. But would it then be
called in parallel on two CPU at once? I doubt that. So how should
touching req_int_count on the bottom of the function be unsafe?

If WQ_UNBOUND would mean, that it would be run on more than one CPU
at once, this should clearly be documented.

Could we get to the bottom of this and find out whether or not the work
items can be executed in parallel ?

Since the list handling is properly locked, this should be fine.

+	if (!video->async_wq)
+		return -EINVAL;
+
 	video->uvc = uvc;
 	video->fcc = V4L2_PIX_FMT_YUYV;
 	video->bpp = 16;

Thanks,
Michael

--
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux