[PATCH v2 0/3] usb: gadget: uvc: fixes and improvements

[PATCH v2 0/3] usb: gadget: uvc: fixes and improvements

[Michael Grzeschik]

This series includes several patches to improve the overall usb uvc
gadget code. It includes some style changes and some serious fixes.

Michael Grzeschik (3):
  usb: gadget: uvc: calculate the number of request depending on
    framesize
  usb: gadget: uvc: increase worker prio to WQ_HIGHPRI
  usb: gadget: uvc: call uvc uvcg_warn on completed status instead of
    uvcg_info

 drivers/usb/gadget/function/f_uvc.c     |  4 ++++
 drivers/usb/gadget/function/uvc.h       |  1 +
 drivers/usb/gadget/function/uvc_queue.c | 17 ++++++++++++-----
 drivers/usb/gadget/function/uvc_v4l2.c  |  2 +-
 drivers/usb/gadget/function/uvc_video.c | 11 ++++++++---
 5 files changed, 26 insertions(+), 9 deletions(-)

-- 
2.30.2

[PATCH v2 1/3] usb: gadget: uvc: calculate the number of request depending on framesize

[Michael Grzeschik]

The current limitation of possible number of requests being handled is
dependent on the gadget speed. It makes more sense to depend on the
typical frame size when calculating the number of requests. This patch
is changing this and is using the previous limits as boundaries for
reasonable minimum and maximum number of requests.

For a 1080p jpeg encoded video stream with a maximum imagesize of
e.g. 800kB with a maxburst of 8 and an multiplier of 1 the resulting
number of requests is calculated to 49.

        800768         1
nreqs = ------ * -------------- ~= 49
          2      (1024 * 8 * 1)

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v1 -> v2: - using clamp instead of min/max
          - added calculation example to description
	  - commented the additional division by two in the code

 drivers/usb/gadget/function/uvc_queue.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index d25edc3d2174e1..eb9bd9d32cd056 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -44,7 +44,8 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
 	struct uvc_video *video = container_of(queue, struct uvc_video, queue);
-	struct usb_composite_dev *cdev = video->uvc->func.config->cdev;
+	unsigned int req_size;
+	unsigned int nreq;
 
 	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)
 		*nbuffers = UVC_MAX_VIDEO_BUFFERS;
@@ -53,10 +54,16 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 
 	sizes[0] = video->imagesize;
 
-	if (cdev->gadget->speed < USB_SPEED_SUPER)
-		video->uvc_num_requests = 4;
-	else
-		video->uvc_num_requests = 64;
+	req_size = video->ep->maxpacket
+		 * max_t(unsigned int, video->ep->maxburst, 1)
+		 * (video->ep->mult);
+
+	/* We divide by two, to increase the chance to run
+	 * into fewer requests for smaller framesizes.
+	 */
+	nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
+	nreq = clamp(nreq, 4U, 64U);
+	video->uvc_num_requests = nreq;
 
 	return 0;
 }
-- 
2.30.2

Re: [PATCH v2 1/3] usb: gadget: uvc: calculate the number of request depending on framesize

[Dan Vacura]

Hi Michael,

On Mon, May 30, 2022 at 12:38:46AM +0200, Michael Grzeschik wrote:
> The current limitation of possible number of requests being handled is
> dependent on the gadget speed. It makes more sense to depend on the
> typical frame size when calculating the number of requests. This patch
> is changing this and is using the previous limits as boundaries for
> reasonable minimum and maximum number of requests.
> 
> For a 1080p jpeg encoded video stream with a maximum imagesize of
> e.g. 800kB with a maxburst of 8 and an multiplier of 1 the resulting
> number of requests is calculated to 49.
> 
>         800768         1
> nreqs = ------ * -------------- ~= 49
>           2      (1024 * 8 * 1)
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Thanks for the patch. This improves the usb2 performance on our
android 5.10 kernel with dwc3 controller. Any reason to not cc stable
lines? This does help older releases as well.

Tested-by: Dan Vacura <w36195@motorola.com>

> 
> ---
> v1 -> v2: - using clamp instead of min/max
>           - added calculation example to description
> 	  - commented the additional division by two in the code
> 
>  drivers/usb/gadget/function/uvc_queue.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index d25edc3d2174e1..eb9bd9d32cd056 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -44,7 +44,8 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
>  	struct uvc_video *video = container_of(queue, struct uvc_video, queue);
> -	struct usb_composite_dev *cdev = video->uvc->func.config->cdev;
> +	unsigned int req_size;
> +	unsigned int nreq;
>  
>  	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)
>  		*nbuffers = UVC_MAX_VIDEO_BUFFERS;
> @@ -53,10 +54,16 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>  
>  	sizes[0] = video->imagesize;
>  
> -	if (cdev->gadget->speed < USB_SPEED_SUPER)
> -		video->uvc_num_requests = 4;
> -	else
> -		video->uvc_num_requests = 64;
> +	req_size = video->ep->maxpacket
> +		 * max_t(unsigned int, video->ep->maxburst, 1)
> +		 * (video->ep->mult);
> +
> +	/* We divide by two, to increase the chance to run
> +	 * into fewer requests for smaller framesizes.
> +	 */
> +	nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
> +	nreq = clamp(nreq, 4U, 64U);
> +	video->uvc_num_requests = nreq;
>  
>  	return 0;
>  }
> -- 
> 2.30.2
> 

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

[Michael Grzeschik]

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>

---
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 0334c8a414e9b9..02e0caecaada1a 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -891,9 +891,13 @@ 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;
 
 	uvcg_info(f, "%s()\n", __func__);
 
+	if (video->async_wq)
+		destroy_workqueue(video->async_wq);
+
 	device_remove_file(&uvc->vdev.dev, &dev_attr_function_name);
 	video_unregister_device(&uvc->vdev);
 	v4l2_device_unregister(&uvc->v4l2_dev);
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 9eec104b3666ad..2bc43ce20e25a5 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -87,6 +87,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 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);
+	if (!video->async_wq)
+		return -EINVAL;
+
 	video->uvc = uvc;
 	video->fcc = V4L2_PIX_FMT_YUYV;
 	video->bpp = 16;
-- 
2.30.2

[PATCH v2 3/3] usb: gadget: uvc: call uvc uvcg_warn on completed status instead of uvcg_info

[Michael Grzeschik]

Likewise to the uvcvideo hostside driver, this patch is changing the
usb_request message of an non zero completion handler call from dev_info
to dev_warn.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
v1 -> v2: - no changes

 drivers/usb/gadget/function/uvc_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 9a9101851bc1e8..1a3d39fd140f15 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -261,7 +261,7 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 		break;
 
 	default:
-		uvcg_info(&video->uvc->func,
+		uvcg_warn(&video->uvc->func,
 			  "VS request completed with status %d.\n",
 			  req->status);
 		uvcg_queue_cancel(queue, 0);
-- 
2.30.2

Re: [PATCH v2 0/3] usb: gadget: uvc: fixes and improvements

[Greg KH]

On Mon, May 30, 2022 at 12:38:45AM +0200, Michael Grzeschik wrote:
> This series includes several patches to improve the overall usb uvc
> gadget code. It includes some style changes and some serious fixes.
> 
> Michael Grzeschik (3):
>   usb: gadget: uvc: calculate the number of request depending on
>     framesize
>   usb: gadget: uvc: increase worker prio to WQ_HIGHPRI
>   usb: gadget: uvc: call uvc uvcg_warn on completed status instead of
>     uvcg_info
> 
>  drivers/usb/gadget/function/f_uvc.c     |  4 ++++
>  drivers/usb/gadget/function/uvc.h       |  1 +
>  drivers/usb/gadget/function/uvc_queue.c | 17 ++++++++++++-----
>  drivers/usb/gadget/function/uvc_v4l2.c  |  2 +-
>  drivers/usb/gadget/function/uvc_video.c | 11 ++++++++---
>  5 files changed, 26 insertions(+), 9 deletions(-)

This patch series fails to apply to my tree, can you please rebase and
resend?

thanks,

greg k-h

Re: [PATCH v2 0/3] usb: gadget: uvc: fixes and improvements

[Greg KH]

On Fri, Jun 10, 2022 at 11:51:23AM +0200, Greg KH wrote:
> On Mon, May 30, 2022 at 12:38:45AM +0200, Michael Grzeschik wrote:
> > This series includes several patches to improve the overall usb uvc
> > gadget code. It includes some style changes and some serious fixes.
> > 
> > Michael Grzeschik (3):
> >   usb: gadget: uvc: calculate the number of request depending on
> >     framesize
> >   usb: gadget: uvc: increase worker prio to WQ_HIGHPRI
> >   usb: gadget: uvc: call uvc uvcg_warn on completed status instead of
> >     uvcg_info
> > 
> >  drivers/usb/gadget/function/f_uvc.c     |  4 ++++
> >  drivers/usb/gadget/function/uvc.h       |  1 +
> >  drivers/usb/gadget/function/uvc_queue.c | 17 ++++++++++++-----
> >  drivers/usb/gadget/function/uvc_v4l2.c  |  2 +-
> >  drivers/usb/gadget/function/uvc_video.c | 11 ++++++++---
> >  5 files changed, 26 insertions(+), 9 deletions(-)
> 
> This patch series fails to apply to my tree, can you please rebase and
> resend?

Make that one patch failed to apply, I took the other two.

thanks,

greg k-h