Re: [PATCH 3/5] usb: gadget: uvc: increase worker prio to WQ_HIGHPRI

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Michael,

Thank you for the patch.

On Sun, Apr 03, 2022 at 01:39:12AM +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@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc.h       | 1 +
>  drivers/usb/gadget/function/uvc_v4l2.c  | 2 +-
>  drivers/usb/gadget/function/uvc_video.c | 9 +++++++--
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index c3607a32b98624..ab537acdae3184 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -86,6 +86,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 a2c78690c5c288..9b1488f7abd736 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 7f59a0c4740209..b1075e23a61010 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -269,7 +269,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
> @@ -469,7 +469,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;
>  }
> @@ -483,6 +483,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 stream specific work queue for asynchronous tasks. */

You can drop the "stream" here. The gadget driver handles a single
stream.

> +	video->async_wq = alloc_workqueue("uvcvideo", WQ_UNBOUND | WQ_HIGHPRI, 0);

Unless I'm mistaken, an unbound work queue means that multiple CPUs will
handle tasks in parallel. Is that safe ?

> +	if (!video->async_wq)
> +		return -EINVAL;

No need to destroy the work queue somewhere ?

> +
>  	video->uvc = uvc;
>  	video->fcc = V4L2_PIX_FMT_YUYV;
>  	video->bpp = 16;

-- 
Regards,

Laurent Pinchart