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

[PATCH 0/5] 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 (5):
  usb: gadget: uvc: reset bytesused on queue cancel
  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
  usb: gadget: uvc: stop the pump on more conditions

 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 | 13 +++++++++----
 4 files changed, 23 insertions(+), 10 deletions(-)

-- 
2.30.2

[PATCH 1/5] usb: gadget: uvc: reset bytesused on queue cancel

[Michael Grzeschik]

On uvcg_queue_cancel the buf_used counter has to be reset. Since the
encode function uses the variable to decide if the encoded data has
reached the end of frame. Intermediate calls of uvcg_queue_cancel can
therefor lead to miscalculations in the encode functions, if buf_used
was not properly reset.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_queue.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index d852ac9e47e72c..cfa0ac4adb04d5 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -256,6 +256,8 @@ void uvcg_queue_cancel(struct uvc_video_queue *queue, int disconnect)
 	struct uvc_buffer *buf;
 	unsigned long flags;
 
+	queue->buf_used = 0;
+
 	spin_lock_irqsave(&queue->irqlock, flags);
 	while (!list_empty(&queue->irqqueue)) {
 		buf = list_first_entry(&queue->irqqueue, struct uvc_buffer,
-- 
2.30.2

Re: [PATCH 1/5] usb: gadget: uvc: reset bytesused on queue cancel

[Sergey Shtylyov]

Hello!

On 4/3/22 2:39 AM, Michael Grzeschik wrote:

> On uvcg_queue_cancel the buf_used counter has to be reset. Since the
> encode function uses the variable to decide if the encoded data has
> reached the end of frame. Intermediate calls of uvcg_queue_cancel can
> therefor lead to miscalculations in the encode functions, if buf_used

   Therefore?

> was not properly reset.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
[...]

MBR, Sergey

Re: [PATCH 1/5] usb: gadget: uvc: reset bytesused on queue cancel

[Dan Vacura]

Hi Michael,

Looks like we found the same issue, I submitted the same change the
other week here:
https://lore.kernel.org/all/20220331184024.23918-1-w36195@motorola.com/

One difference is you had the reset outside of the queue lock. I figured
to keep it within the lock since we can get a cancel while the pump
worker is running and this can negate the reset. Do you agree?

Thanks,

Dan

On Tue, Apr 05, 2022 at 11:43:16AM +0300, Sergey Shtylyov wrote:
> Hello!
> 
> On 4/3/22 2:39 AM, Michael Grzeschik wrote:
> 
> > On uvcg_queue_cancel the buf_used counter has to be reset. Since the
> > encode function uses the variable to decide if the encoded data has
> > reached the end of frame. Intermediate calls of uvcg_queue_cancel can
> > therefor lead to miscalculations in the encode functions, if buf_used
> 
>    Therefore?
> 
> > was not properly reset.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> [...]
> 
> MBR, Sergey

Re: [PATCH 1/5] usb: gadget: uvc: reset bytesused on queue cancel

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

Hi Dan,


On Tue, Apr 05, 2022 at 10:01:34AM -0500, Dan Vacura wrote:
>Looks like we found the same issue, I submitted the same change the
>other week here:
>https://lore.kernel.org/all/20220331184024.23918-1-w36195@motorola.com/
>
>One difference is you had the reset outside of the queue lock. I figured
>to keep it within the lock since we can get a cancel while the pump
>worker is running and this can negate the reset. Do you agree?

Yes! Your patch is to favour and mine can be dropped from this series.

Thanks,
Michael

>
>On Tue, Apr 05, 2022 at 11:43:16AM +0300, Sergey Shtylyov wrote:
>> Hello!
>>
>> On 4/3/22 2:39 AM, Michael Grzeschik wrote:
>>
>> > On uvcg_queue_cancel the buf_used counter has to be reset. Since the
>> > encode function uses the variable to decide if the encoded data has
>> > reached the end of frame. Intermediate calls of uvcg_queue_cancel can
>> > therefor lead to miscalculations in the encode functions, if buf_used
>>
>>    Therefore?
>>
>> > was not properly reset.
>> >
>> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> [...]
>>
>> MBR, Sergey
>

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/5] 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.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_queue.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index cfa0ac4adb04d5..2a091cf07981e1 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,14 @@ 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);
+
+	nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
+	nreq = min_t(unsigned int, nreq, 64);
+	nreq = max_t(unsigned int, nreq, 4);
+	video->uvc_num_requests = nreq;
 
 	return 0;
 }
-- 
2.30.2

Re: [PATCH 2/5] usb: gadget: uvc: calculate the number of request depending on framesize

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Sun, Apr 03, 2022 at 01:39:11AM +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.

What are typical values you get for the number of requests in your use
cases with this change ? Could you mention them in the commit message ?

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc_queue.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index cfa0ac4adb04d5..2a091cf07981e1 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,14 @@ 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);
> +
> +	nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);

Where does the division by 2 come from ?

> +	nreq = min_t(unsigned int, nreq, 64);
> +	nreq = max_t(unsigned int, nreq, 4);

You can use clamp():

	video->uvc_num_requests = clamp(nreq, 4U, 64U);

> +	video->uvc_num_requests = nreq;
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] usb: gadget: uvc: calculate the number of request depending on framesize

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 3089 bytes --]

Hi Laurent,

On Tue, Apr 19, 2022 at 11:46:44PM +0300, Laurent Pinchart wrote:
>Thank you for the patch.
>
>On Sun, Apr 03, 2022 at 01:39:11AM +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.
>
>What are typical values you get for the number of requests in your use
>cases with this change ?

With this patch, for a 4k Video stream I get usually sizes of 3127808
bytes and 64 requests. With 1080p sizes is 800768 bytes and with this
patch I get 56 request.

>Could you mention them in the commit message ?

Yes, I will add them in the comment of v2.

>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>>  drivers/usb/gadget/function/uvc_queue.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>> index cfa0ac4adb04d5..2a091cf07981e1 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,14 @@ 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);
>> +
>> +	nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
>
>Where does the division by 2 come from ?

That is a good question. I think that I used it as a tool to
come into the range inbetween 4 and 64 requests for 1080p video
streaming. Since the framesizes where more likely to run into
the 64 requests to be allocated.

>> +	nreq = min_t(unsigned int, nreq, 64);
>> +	nreq = max_t(unsigned int, nreq, 4);
>
>You can use clamp():
>
>	video->uvc_num_requests = clamp(nreq, 4U, 64U);

Thanks! I fixed that for v2.

>> +	video->uvc_num_requests = nreq;
>>
>>  	return 0;
>>  }

Regards,
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/5] 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>
---
 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. */
+	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

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

[Laurent Pinchart]

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

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

[Dan Vacura]

Hi Michael,

Thanks for this change it improves the performance with the DWC3
controller on QCOM chips in an Android 5.10 kernel. I haven't tested the
scatter/gather path, so memcpy was used here via
uvc_video_encode_isoc(). I was able to get around 30% improvement (fps
on host side). I did modify the alloc to only set the WQ_HIGHPRI flag.

On Tue, Apr 19, 2022 at 11:46:57PM +0300, Laurent Pinchart wrote:
> 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>
> > +	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 ?

I found that with the WQ_UNBOUND flag I didn't see any performance
improvement to the baseline, perhaps related to cpu caching or
scheduling delays. I didn't notice any stability problems or concurrent
execution. Do you see any benefit to keeping the WQ_UNBOUND flag?

> 
> > +	if (!video->async_wq)
> > +		return -EINVAL;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Thanks,

Dan

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

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

Hi Dan,
Hi Laurent,

On Fri, Apr 29, 2022 at 01:51:48PM -0500, Dan Vacura wrote:
>Thanks for this change it improves the performance with the DWC3
>controller on QCOM chips in an Android 5.10 kernel. I haven't tested the
>scatter/gather path, so memcpy was used here via
>uvc_video_encode_isoc(). I was able to get around 30% improvement (fps
>on host side). I did modify the alloc to only set the WQ_HIGHPRI flag.
>
>On Tue, Apr 19, 2022 at 11:46:57PM +0300, Laurent Pinchart wrote:
>> 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>
>> > +	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 ?
>
>I found that with the WQ_UNBOUND flag I didn't see any performance
>improvement to the baseline, perhaps related to cpu caching or
>scheduling delays. I didn't notice any stability problems or concurrent
>execution. Do you see any benefit to keeping the WQ_UNBOUND flag?

I actually copied this from drivers/media/usb/uvc/uvc_driver.c ,
which is also allocating the workqueue with WQ_UNBOUND.

Look into drivers/media/usb/uvc/uvc_driver.c + 486

	stream->async_wq = alloc_workqueue("uvcvideo", WQ_UNBOUND | WQ_HIGHPRI,

In my tests, continous streaming did not trigger any errors. In fact if
this would be unsafe, the issue would probably trigger early, numerous
and obvious on multicore cpus.

However, some users seem to have seen recent issues on unplugging the
cable while streaming. I have to check if this could be related.

>> > +	if (!video->async_wq)
>> > +		return -EINVAL;
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>
>Thanks,
>
>Dan
>

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

Hi Dan,

On Fri, Apr 29, 2022 at 10:01:37PM +0200, Michael Grzeschik wrote:
>On Fri, Apr 29, 2022 at 01:51:48PM -0500, Dan Vacura wrote:
>>Thanks for this change it improves the performance with the DWC3
>>controller on QCOM chips in an Android 5.10 kernel. I haven't tested the
>>scatter/gather path, so memcpy was used here via
>>uvc_video_encode_isoc(). I was able to get around 30% improvement (fps
>>on host side). I did modify the alloc to only set the WQ_HIGHPRI flag.

I missed to ask you to try the WQ_CPU_INTENSIVE flag. It would be
interesting if you can see further improvement.

Regards,
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dan Vacura]

Hi Michael,

On Mon, May 02, 2022 at 11:00:03AM +0200, Michael Grzeschik wrote:
> Hi Dan,
> 
> On Fri, Apr 29, 2022 at 10:01:37PM +0200, Michael Grzeschik wrote:
> > On Fri, Apr 29, 2022 at 01:51:48PM -0500, Dan Vacura wrote:
> > > Thanks for this change it improves the performance with the DWC3
> > > controller on QCOM chips in an Android 5.10 kernel. I haven't tested the
> > > scatter/gather path, so memcpy was used here via
> > > uvc_video_encode_isoc(). I was able to get around 30% improvement (fps
> > > on host side). I did modify the alloc to only set the WQ_HIGHPRI flag.
> 
> I missed to ask you to try the WQ_CPU_INTENSIVE flag. It would be
> interesting if you can see further improvement.

I had some time to test this flag and I couldn't find any discernible
difference with it set or not.

Regards,

Dan

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

[Laurent Pinchart]

Hi Michael,

On Fri, Apr 29, 2022 at 10:01:37PM +0200, Michael Grzeschik wrote:
> Hi Dan,
> Hi Laurent,
> 
> On Fri, Apr 29, 2022 at 01:51:48PM -0500, Dan Vacura wrote:
> > Thanks for this change it improves the performance with the DWC3
> > controller on QCOM chips in an Android 5.10 kernel. I haven't tested the
> > scatter/gather path, so memcpy was used here via
> > uvc_video_encode_isoc(). I was able to get around 30% improvement (fps
> > on host side). I did modify the alloc to only set the WQ_HIGHPRI flag.
> >
> > On Tue, Apr 19, 2022 at 11:46:57PM +0300, Laurent Pinchart wrote:
> >> 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>
> >> > +	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 ?
> >
> > I found that with the WQ_UNBOUND flag I didn't see any performance
> > improvement to the baseline, perhaps related to cpu caching or
> > scheduling delays. I didn't notice any stability problems or concurrent
> > execution. Do you see any benefit to keeping the WQ_UNBOUND flag?
> 
> I actually copied this from drivers/media/usb/uvc/uvc_driver.c ,
> which is also allocating the workqueue with WQ_UNBOUND.
> 
> Look into drivers/media/usb/uvc/uvc_driver.c + 486
> 
> 	stream->async_wq = alloc_workqueue("uvcvideo", WQ_UNBOUND | WQ_HIGHPRI,

Just for the record, as a newer version of this patch has been merged,
the host-side uvcvideo driver is specifically made to handle multiple
work items in parallel. Each work item will essentially perform one or
multiple memcpy operations, with the size and offset calculated by the
code that dispatches the work items.

As Lucas separately commented, the UVC gadget driver has a single
work_struct, so there can't be any concurrency. We seem to be safe for
now.

> In my tests, continous streaming did not trigger any errors. In fact if
> this would be unsafe, the issue would probably trigger early, numerous
> and obvious on multicore cpus.
> 
> However, some users seem to have seen recent issues on unplugging the
> cable while streaming. I have to check if this could be related.
> 
> >> > +	if (!video->async_wq)
> >> > +		return -EINVAL;

-- 
Regards,

Laurent Pinchart

[PATCH 4/5] 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>
---
 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 b1075e23a61010..8b3116d48d2bd8 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -258,7 +258,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 4/5] usb: gadget: uvc: call uvc uvcg_warn on completed status instead of uvcg_info

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Sun, Apr 03, 2022 at 01:39:13AM +0200, Michael Grzeschik wrote:
> 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>

> ---
>  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 b1075e23a61010..8b3116d48d2bd8 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -258,7 +258,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);

-- 
Regards,

Laurent Pinchart

[PATCH 5/5] usb: gadget: uvc: stop the pump on more conditions

[Michael Grzeschik]

While looping in the pump, there are more conditions to stop handling
requests. The streamoff event that will disable the endpoint and
the vb2_queue is called early. We add the variables into account.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 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 8b3116d48d2bd8..b1d7083d07846e 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -368,7 +368,7 @@ static void uvcg_video_pump(struct work_struct *work)
 	unsigned long flags;
 	int ret;
 
-	while (video->ep->enabled) {
+	while (video->ep->enabled || queue->queue.streaming || video->uvc->streamon) {
 		/* Retrieve the first available USB request, protected by the
 		 * request lock.
 		 */
-- 
2.30.2

Re: [PATCH 5/5] usb: gadget: uvc: stop the pump on more conditions

[kernel test robot]

Hi Michael,

I love your patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v5.18-rc1 next-20220404]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Michael-Grzeschik/usb-gadget-uvc-fixes-and-improvements/20220404-165031
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arm64-randconfig-r016-20220404 (https://download.01.org/0day-ci/archive/20220404/202204041903.wSoTM3yH-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/3577e91e5a0a9a94ee3d4b22240e7b143c31133c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Michael-Grzeschik/usb-gadget-uvc-fixes-and-improvements/20220404-165031
        git checkout 3577e91e5a0a9a94ee3d4b22240e7b143c31133c
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/usb/gadget/function/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/usb/gadget/function/uvc_video.c: In function 'uvcg_video_pump':
>> drivers/usb/gadget/function/uvc_video.c:371:74: error: 'struct uvc_device' has no member named 'streamon'
     371 |         while (video->ep->enabled || queue->queue.streaming || video->uvc->streamon) {
         |                                                                          ^~


vim +371 drivers/usb/gadget/function/uvc_video.c

   351	
   352	/* --------------------------------------------------------------------------
   353	 * Video streaming
   354	 */
   355	
   356	/*
   357	 * uvcg_video_pump - Pump video data into the USB requests
   358	 *
   359	 * This function fills the available USB requests (listed in req_free) with
   360	 * video data from the queued buffers.
   361	 */
   362	static void uvcg_video_pump(struct work_struct *work)
   363	{
   364		struct uvc_video *video = container_of(work, struct uvc_video, pump);
   365		struct uvc_video_queue *queue = &video->queue;
   366		struct usb_request *req = NULL;
   367		struct uvc_buffer *buf;
   368		unsigned long flags;
   369		int ret;
   370	
 > 371		while (video->ep->enabled || queue->queue.streaming || video->uvc->streamon) {
   372			/* Retrieve the first available USB request, protected by the
   373			 * request lock.
   374			 */
   375			spin_lock_irqsave(&video->req_lock, flags);
   376			if (list_empty(&video->req_free)) {
   377				spin_unlock_irqrestore(&video->req_lock, flags);
   378				return;
   379			}
   380			req = list_first_entry(&video->req_free, struct usb_request,
   381						list);
   382			list_del(&req->list);
   383			spin_unlock_irqrestore(&video->req_lock, flags);
   384	
   385			/* Retrieve the first available video buffer and fill the
   386			 * request, protected by the video queue irqlock.
   387			 */
   388			spin_lock_irqsave(&queue->irqlock, flags);
   389			buf = uvcg_queue_head(queue);
   390			if (buf == NULL) {
   391				spin_unlock_irqrestore(&queue->irqlock, flags);
   392				break;
   393			}
   394	
   395			video->encode(req, video, buf);
   396	
   397			/* With usb3 we have more requests. This will decrease the
   398			 * interrupt load to a quarter but also catches the corner
   399			 * cases, which needs to be handled */
   400			if (list_empty(&video->req_free) ||
   401			    buf->state == UVC_BUF_STATE_DONE ||
   402			    !(video->req_int_count %
   403			       DIV_ROUND_UP(video->uvc_num_requests, 4))) {
   404				video->req_int_count = 0;
   405				req->no_interrupt = 0;
   406			} else {
   407				req->no_interrupt = 1;
   408			}
   409	
   410			/* Queue the USB request */
   411			ret = uvcg_video_ep_queue(video, req);
   412			spin_unlock_irqrestore(&queue->irqlock, flags);
   413	
   414			if (ret < 0) {
   415				uvcg_queue_cancel(queue, 0);
   416				break;
   417			}
   418			video->req_int_count++;
   419		}
   420	
   421		if (!req)
   422			return;
   423	
   424		spin_lock_irqsave(&video->req_lock, flags);
   425		list_add_tail(&req->list, &video->req_free);
   426		spin_unlock_irqrestore(&video->req_lock, flags);
   427		return;
   428	}
   429	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 5/5] usb: gadget: uvc: stop the pump on more conditions

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]

I think we can skip this patch for now, since it is depending on this series:

https://lore.kernel.org/linux-usb/20220315143356.3919911-1-m.grzeschik@pengutronix.de/

The other Patches of this series have no dependencies.

Michael

On Sun, Apr 03, 2022 at 01:39:14AM +0200, Michael Grzeschik wrote:
>While looping in the pump, there are more conditions to stop handling
>requests. The streamoff event that will disable the endpoint and
>the vb2_queue is called early. We add the variables into account.
>
>Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>---
> 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 8b3116d48d2bd8..b1d7083d07846e 100644
>--- a/drivers/usb/gadget/function/uvc_video.c
>+++ b/drivers/usb/gadget/function/uvc_video.c
>@@ -368,7 +368,7 @@ static void uvcg_video_pump(struct work_struct *work)
> 	unsigned long flags;
> 	int ret;
>
>-	while (video->ep->enabled) {
>+	while (video->ep->enabled || queue->queue.streaming || video->uvc->streamon) {
> 		/* Retrieve the first available USB request, protected by the
> 		 * request lock.
> 		 */
>-- 
>2.30.2
>
>
>

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] usb: gadget: uvc: stop the pump on more conditions

[Greg KH]

On Mon, Apr 04, 2022 at 03:07:58PM +0200, Michael Grzeschik wrote:
> I think we can skip this patch for now, since it is depending on this series:
> 
> https://lore.kernel.org/linux-usb/20220315143356.3919911-1-m.grzeschik@pengutronix.de/
> 
> The other Patches of this series have no dependencies.

Can you please fix up and resend then?  Our tools want to apply the
whole series at once, for obvious reasons.

thanks,

greg k-h