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

[RESEND 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

[RESEND 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>
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

[RESEND 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 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);
+	if (!video->async_wq)
+		return -EINVAL;
+
 	video->uvc = uvc;
 	video->fcc = V4L2_PIX_FMT_YUYV;
 	video->bpp = 16;
-- 
2.30.2

[RESEND 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: [RESEND v2 1/3] usb: gadget: uvc: calculate the number of request depending on framesize

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Wed, Jun 08, 2022 at 01:03:37PM +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>
> 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);

No need for parentheses.

> +
> +	/* We divide by two, to increase the chance to run
> +	 * into fewer requests for smaller framesizes.
> +	 */

Could you please change the comment style to the more standard

	/*
	 * We divide by two, to increase the chance to run into fewer requests
	 * for smaller framesizes.
	 */

(with the text reflowed to 80 columns) ?

I'm however now sure where the division by 2 come from.

Furthermore, as far as I understand, the reason why the number of
requests was increased for superspeed devices (by you ;-)) was to avoid
underruns at higher speeds and keep the queue full. This is less of a
concern at lower speeds. Is there any drawback in increasing it
regardless of the speed ? Increased latency comes to mind, but is it a
problem in practice ?

> +	nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
> +	nreq = clamp(nreq, 4U, 64U);
> +	video->uvc_num_requests = nreq;
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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@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 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.

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

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

-- 
Regards,

Laurent Pinchart

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

[Michael Grzeschik]

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

Hi Laurent,

On Wed, Jun 08, 2022 at 09:06:55PM +0300, Laurent Pinchart wrote:
>On Wed, Jun 08, 2022 at 01:03:37PM +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>
>> 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);
>
>No need for parentheses.
>
>> +
>> +	/* We divide by two, to increase the chance to run
>> +	 * into fewer requests for smaller framesizes.
>> +	 */
>
>Could you please change the comment style to the more standard
>
>	/*
>	 * We divide by two, to increase the chance to run into fewer requests
>	 * for smaller framesizes.
>	 */
>
>(with the text reflowed to 80 columns) ?

These two things have to be fixed in a seperate patch.
Greg seems to have taken it already, as is.

>I'm however now sure where the division by 2 come from.
>
>Furthermore, as far as I understand, the reason why the number of
>requests was increased for superspeed devices (by you ;-)) was to avoid
>underruns at higher speeds and keep the queue full. This is less of a
>concern at lower speeds. Is there any drawback in increasing it
>regardless of the speed ? Increased latency comes to mind, but is it a
>problem in practice ?

This is a good question. Lets think through the case, where we set
the number of requests to an high number, just to be safe.

I think that for lower bandwidth USB you will probably not send high
resolution video. Especially if they are uncompressed. So having an
extra amount of requests available, they will probably unnecessary keep
more data than one frame in memory.

For the high bandwidth usb case we would by default also keep an extra
amount of memory available, especially if the burst and mult is set in
the upper values.

We would have to think of an resonable value for both cases.

This is why I think depending on the framesize is an good compromise.

>> +	nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
>> +	nreq = clamp(nreq, 4U, 64U);
>> +	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 --]

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

[Michael Grzeschik]

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

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@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 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.

Good Idea. I will fix this.

>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 didn't think about that. I will have to check for that.

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

Do you have an Idea to check for that?

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

-- 
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] fixup! usb: gadget: uvc: calculate the number of request depending on framesize

[Michael Grzeschik]

---
In Patch "c5d337a3: usb: gadget: uvc: Fix comment blocks style" the overall
comment style was fixed to use a consistent format.

Laurent suggested to fix the comment format and to drop the extra
parantheses around video->ep->mult in this patch. But it was already
taken without those changes.

Greg, could you fix this up in usb-next?

Thanks,
Michael
---
 drivers/usb/gadget/function/uvc_queue.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index ec500ee499eed1..267474225ee409 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -56,9 +56,10 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 
 	req_size = video->ep->maxpacket
 		 * max_t(unsigned int, video->ep->maxburst, 1)
-		 * (video->ep->mult);
+		 * video->ep->mult;
 
-	/* We divide by two, to increase the chance to run
+	/*
+	 * 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);
-- 
2.30.2

Re: [PATCH] fixup! usb: gadget: uvc: calculate the number of request depending on framesize

[Greg KH]

On Mon, Jun 13, 2022 at 08:58:44AM +0200, Michael Grzeschik wrote:
> ---
> In Patch "c5d337a3: usb: gadget: uvc: Fix comment blocks style" the overall
> comment style was fixed to use a consistent format.
> 
> Laurent suggested to fix the comment format and to drop the extra
> parantheses around video->ep->mult in this patch. But it was already
> taken without those changes.
> 
> Greg, could you fix this up in usb-next?

I need a real patch that I can apply, this is not that format :(

[PATCH] usb: gadget: uvc: Style fixes in uvc_queue

[Michael Grzeschik]

In Patch "c5d337a3: usb: gadget: uvc: Fix comment blocks style" the
overall comment style was fixed to use a consistent format.

This was missed in the Patch "87d76b5f: usb: gadget: uvc: calculate the
number of request depending on framesize". In this patch, it also missed
to remove the extra parentheses around video->ep->mult. We fix it now.

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

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index ec500ee499eed1..267474225ee409 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -56,9 +56,10 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 
 	req_size = video->ep->maxpacket
 		 * max_t(unsigned int, video->ep->maxburst, 1)
-		 * (video->ep->mult);
+		 * video->ep->mult;
 
-	/* We divide by two, to increase the chance to run
+	/*
+	 * 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);
-- 
2.30.2

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

[Michael Grzeschik]

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

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

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

[Lucas Stach]

Am Mittwoch, dem 20.07.2022 um 00:52 +0200 schrieb Michael Grzeschik:
> 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@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 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.

All workqueues (including the system_wq, that is used by schedule_work)
can execute multiple workitems at the same time. The max_active
parameter provided to alloc_workqueue() is what regulates concurrency,
WQ_UNBOUND has nothing to do with this, expect that it provides a
different maximum of the possible concurrency.

If the code works fine as-is, then this change should make no
difference. Without looking into the details, I think the singlethread
assumption here is satisfied by the video pump being a single work
item, so if it is already queued it will not be queued again, so there
is nothing to execute in parallel.

Regards,
Lucas