[PATCH v2 0/3] usb: gadget: uvc: allocate requests based on frame interval length and buffersize

[PATCH v2 0/3] usb: gadget: uvc: allocate requests based on frame interval length and buffersize

[Michael Grzeschik]

This patch series is improving the size calculation and allocation of
the uvc requests. Using the selected frame duration of the stream it is
possible to calculate the number of requests based on the interval
length.

This is an preperation to precalculate the request length based
on the actual per frame size for compressed formats.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
Changes in v2:
- added header size into calculation of request size
- Link to v1: https://lore.kernel.org/r/20240403-uvc_request_length_by_interval-v1-0-9436c4716233@pengutronix.de

---
Michael Grzeschik (3):
      usb: gadget: function: uvc: set req_size once when the vb2 queue is calculated
      usb: gadget: uvc: add g_parm and s_parm for frame interval
      usb: gadget: uvc: set req_size and n_requests based on the frame interval

 drivers/usb/gadget/function/uvc.h       |  1 +
 drivers/usb/gadget/function/uvc_queue.c | 32 +++++++++++++++-----
 drivers/usb/gadget/function/uvc_v4l2.c  | 52 +++++++++++++++++++++++++++++++++
 drivers/usb/gadget/function/uvc_video.c | 17 ++---------
 4 files changed, 81 insertions(+), 21 deletions(-)
---
base-commit: 819984a0dd3606b7c46fe156cd56a0dc0d604788
change-id: 20240403-uvc_request_length_by_interval-a7efd587d963

Best regards,
-- 
Michael Grzeschik <m.grzeschik@pengutronix.de>

[PATCH v2 1/3] usb: gadget: function: uvc: set req_size once when the vb2 queue is calculated

[Michael Grzeschik]

The uvc gadget driver is calculating the req_size on every
video_enable/alloc_request and is based on the fixed configfs parameters
maxpacket, maxburst and mult.

As those parameters can not be changed once the gadget is started and
the same calculation is done already early on the
vb2_streamon/queue_setup path its save to remove one extra calculation
and reuse the calculation from uvc_queue_setup for the allocation step.

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

---
Link to v1: https://lore.kernel.org/r/20240404-uvc_request_calc_once-v1-1-fea7fd8f0496@pengutronix.de

v1 -> v2: -
---
 drivers/usb/gadget/function/uvc_queue.c |  2 ++
 drivers/usb/gadget/function/uvc_video.c | 15 ++-------------
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index 0aa3d7e1f3cc3..ce51643fc4639 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -63,6 +63,8 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 	 */
 	nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
 	nreq = clamp(nreq, 4U, 64U);
+
+	video->req_size = req_size;
 	video->uvc_num_requests = nreq;
 
 	return 0;
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index d41f5f31dadd5..95bb64e16f3da 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -496,7 +496,6 @@ uvc_video_free_requests(struct uvc_video *video)
 	INIT_LIST_HEAD(&video->ureqs);
 	INIT_LIST_HEAD(&video->req_free);
 	INIT_LIST_HEAD(&video->req_ready);
-	video->req_size = 0;
 	return 0;
 }
 
@@ -504,16 +503,9 @@ static int
 uvc_video_alloc_requests(struct uvc_video *video)
 {
 	struct uvc_request *ureq;
-	unsigned int req_size;
 	unsigned int i;
 	int ret = -ENOMEM;
 
-	BUG_ON(video->req_size);
-
-	req_size = video->ep->maxpacket
-		 * max_t(unsigned int, video->ep->maxburst, 1)
-		 * (video->ep->mult);
-
 	for (i = 0; i < video->uvc_num_requests; i++) {
 		ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL);
 		if (ureq == NULL)
@@ -523,7 +515,7 @@ uvc_video_alloc_requests(struct uvc_video *video)
 
 		list_add_tail(&ureq->list, &video->ureqs);
 
-		ureq->req_buffer = kmalloc(req_size, GFP_KERNEL);
+		ureq->req_buffer = kmalloc(video->req_size, GFP_KERNEL);
 		if (ureq->req_buffer == NULL)
 			goto error;
 
@@ -541,12 +533,10 @@ uvc_video_alloc_requests(struct uvc_video *video)
 		list_add_tail(&ureq->req->list, &video->req_free);
 		/* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
 		sg_alloc_table(&ureq->sgt,
-			       DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN,
+			       DIV_ROUND_UP(video->req_size - UVCG_REQUEST_HEADER_LEN,
 					    PAGE_SIZE) + 2, GFP_KERNEL);
 	}
 
-	video->req_size = req_size;
-
 	return 0;
 
 error:
@@ -699,7 +689,6 @@ uvcg_video_disable(struct uvc_video *video)
 	INIT_LIST_HEAD(&video->ureqs);
 	INIT_LIST_HEAD(&video->req_free);
 	INIT_LIST_HEAD(&video->req_ready);
-	video->req_size = 0;
 	spin_unlock_irqrestore(&video->req_lock, flags);
 
 	/*

-- 
2.39.2

[PATCH v2 2/3] usb: gadget: uvc: add g_parm and s_parm for frame interval

[Michael Grzeschik]

The uvc gadget driver is lacking the information which frame interval
was set by the host. We add this information by implementing the g_parm
and s_parm callbacks.

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

---
v1 -> v2: -
---
 drivers/usb/gadget/function/uvc.h      |  1 +
 drivers/usb/gadget/function/uvc_v4l2.c | 52 ++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index cb35687b11e7e..d153bd9e35e31 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -97,6 +97,7 @@ struct uvc_video {
 	unsigned int width;
 	unsigned int height;
 	unsigned int imagesize;
+	unsigned int interval;
 	struct mutex mutex;	/* protects frame parameters */
 
 	unsigned int uvc_num_requests;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index a024aecb76dc3..5b579ec1f5040 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -307,6 +307,56 @@ uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
 	return ret;
 }
 
+static int uvc_v4l2_g_parm(struct file *file, void *fh,
+			    struct v4l2_streamparm *parm)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct uvc_device *uvc = video_get_drvdata(vdev);
+	struct uvc_video *video = &uvc->video;
+	struct v4l2_fract timeperframe;
+
+	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	/* Return the actual frame period. */
+	timeperframe.numerator = video->interval;
+	timeperframe.denominator = 10000000;
+	v4l2_simplify_fraction(&timeperframe.numerator,
+		&timeperframe.denominator, 8, 333);
+
+	uvcg_dbg(&uvc->func, "Getting frame interval of %u/%u (%u)\n",
+		timeperframe.numerator, timeperframe.denominator,
+		video->interval);
+
+	parm->parm.output.timeperframe = timeperframe;
+	parm->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
+
+	return 0;
+}
+
+static int uvc_v4l2_s_parm(struct file *file, void *fh,
+			    struct v4l2_streamparm *parm)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct uvc_device *uvc = video_get_drvdata(vdev);
+	struct uvc_video *video = &uvc->video;
+	struct v4l2_fract timeperframe;
+
+	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	timeperframe = parm->parm.output.timeperframe;
+
+	video->interval = v4l2_fraction_to_interval(timeperframe.numerator,
+		timeperframe.denominator);
+
+	uvcg_dbg(&uvc->func, "Setting frame interval to %u/%u (%u)\n",
+		timeperframe.numerator, timeperframe.denominator,
+		video->interval);
+
+	return 0;
+}
+
 static int
 uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
 		struct v4l2_frmivalenum *fival)
@@ -577,6 +627,8 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
 	.vidioc_dqbuf = uvc_v4l2_dqbuf,
 	.vidioc_streamon = uvc_v4l2_streamon,
 	.vidioc_streamoff = uvc_v4l2_streamoff,
+	.vidioc_s_parm = uvc_v4l2_s_parm,
+	.vidioc_g_parm = uvc_v4l2_g_parm,
 	.vidioc_subscribe_event = uvc_v4l2_subscribe_event,
 	.vidioc_unsubscribe_event = uvc_v4l2_unsubscribe_event,
 	.vidioc_default = uvc_v4l2_ioctl_default,

-- 
2.39.2

[PATCH v2 3/3] usb: gadget: uvc: set req_size and n_requests based on the frame interval

[Michael Grzeschik]

With the information of the interval frame length it is now possible to
calculate the number of usb requests by the frame duration. Based on the
request size and the imagesize we calculate the actual size per request.
This has calculation has the benefit that the frame data is equally
distributed over all allocated requests.

We keep the current req_size calculation as a fallback, if the interval
callbacks did not set the interval property.

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

---
v1 -> v2: - add headersize per request into calculation
---
 drivers/usb/gadget/function/uvc_queue.c | 30 +++++++++++++++++++++++-------
 drivers/usb/gadget/function/uvc_video.c |  2 +-
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index ce51643fc4639..141e52e34c610 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -44,7 +44,7 @@ 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);
-	unsigned int req_size;
+	unsigned int req_size, max_req_size, header_size;
 	unsigned int nreq;
 
 	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)
@@ -54,15 +54,31 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 
 	sizes[0] = video->imagesize;
 
-	req_size = video->ep->maxpacket
+	nreq = DIV_ROUND_UP(video->interval, video->ep->desc->bInterval * 1250);
+
+	header_size = nreq * UVCG_REQUEST_HEADER_LEN;
+
+	req_size = DIV_ROUND_UP(video->imagesize + header_size, nreq);
+
+	max_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);
+	if (!req_size) {
+		req_size = max_req_size;
+
+		/* 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);
+	} else if (req_size > max_req_size) {
+		/* The prepared interval length and expected buffer size
+		 * is not possible to stream with the currently configured
+		 * isoc bandwidth
+		 */
+		return -EINVAL;
+	}
 
 	video->req_size = req_size;
 	video->uvc_num_requests = nreq;
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 95bb64e16f3da..d197c46e93fb4 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -304,7 +304,7 @@ static int uvcg_video_usb_req_queue(struct uvc_video *video,
 		 */
 		if (list_empty(&video->req_free) || ureq->last_buf ||
 			!(video->req_int_count %
-			DIV_ROUND_UP(video->uvc_num_requests, 4))) {
+			clamp(DIV_ROUND_UP(video->uvc_num_requests, 4), 4U, 16U))) {
 			video->req_int_count = 0;
 			req->no_interrupt = 0;
 		} else {

-- 
2.39.2

Re: [PATCH v2 2/3] usb: gadget: uvc: add g_parm and s_parm for frame interval

[Avichal Rakesh]

On 6/22/24 4:48 PM, Michael Grzeschik wrote:
> The uvc gadget driver is lacking the information which frame interval
> was set by the host. We add this information by implementing the g_parm
> and s_parm callbacks.

This change requires the userspace application (uvc-gagdet equivalent) 
to call s/g_parm when the FPS negotiations are finished. This is fine, 
but we should document that in the commit message here so implementers 
know that something needs to be done to take advantage of the change.

On a similar note, the reference uvc-gadget should also be updated to 
call the added functions (and apologies if you've already put up a 
patch for it, I was unable find one).

> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> v1 -> v2: -
> ---
>  drivers/usb/gadget/function/uvc.h      |  1 +
>  drivers/usb/gadget/function/uvc_v4l2.c | 52 ++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index cb35687b11e7e..d153bd9e35e31 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -97,6 +97,7 @@ struct uvc_video {
>  	unsigned int width;
>  	unsigned int height;
>  	unsigned int imagesize;
> +	unsigned int interval;
>  	struct mutex mutex;	/* protects frame parameters */
>  
>  	unsigned int uvc_num_requests;
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index a024aecb76dc3..5b579ec1f5040 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -307,6 +307,56 @@ uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
>  	return ret;
>  }
>  
> +static int uvc_v4l2_g_parm(struct file *file, void *fh,
> +			    struct v4l2_streamparm *parm)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct uvc_device *uvc = video_get_drvdata(vdev);
> +	struct uvc_video *video = &uvc->video;
> +	struct v4l2_fract timeperframe;
> +
> +	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	/* Return the actual frame period. */
> +	timeperframe.numerator = video->interval;
> +	timeperframe.denominator = 10000000;
> +	v4l2_simplify_fraction(&timeperframe.numerator,
> +		&timeperframe.denominator, 8, 333);
> +
> +	uvcg_dbg(&uvc->func, "Getting frame interval of %u/%u (%u)\n",
> +		timeperframe.numerator, timeperframe.denominator,
> +		video->interval);
> +
> +	parm->parm.output.timeperframe = timeperframe;
> +	parm->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
> +
> +	return 0;
> +}
> +
> +static int uvc_v4l2_s_parm(struct file *file, void *fh,
> +			    struct v4l2_streamparm *parm)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct uvc_device *uvc = video_get_drvdata(vdev);
> +	struct uvc_video *video = &uvc->video;
> +	struct v4l2_fract timeperframe;
> +
> +	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +		return -EINVAL;
> +
> +	timeperframe = parm->parm.output.timeperframe;
> +
> +	video->interval = v4l2_fraction_to_interval(timeperframe.numerator,
> +		timeperframe.denominator);
> +
> +	uvcg_dbg(&uvc->func, "Setting frame interval to %u/%u (%u)\n",
> +		timeperframe.numerator, timeperframe.denominator,
> +		video->interval);
> +
> +	return 0;
> +}
> +
>  static int
>  uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
>  		struct v4l2_frmivalenum *fival)
> @@ -577,6 +627,8 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>  	.vidioc_dqbuf = uvc_v4l2_dqbuf,
>  	.vidioc_streamon = uvc_v4l2_streamon,
>  	.vidioc_streamoff = uvc_v4l2_streamoff,
> +	.vidioc_s_parm = uvc_v4l2_s_parm,
> +	.vidioc_g_parm = uvc_v4l2_g_parm,
>  	.vidioc_subscribe_event = uvc_v4l2_subscribe_event,
>  	.vidioc_unsubscribe_event = uvc_v4l2_unsubscribe_event,
>  	.vidioc_default = uvc_v4l2_ioctl_default,
> 

Re: [PATCH v2 3/3] usb: gadget: uvc: set req_size and n_requests based on the frame interval

[Avichal Rakesh]

On 6/22/24 4:48 PM, Michael Grzeschik wrote:
> With the information of the interval frame length it is now possible to
> calculate the number of usb requests by the frame duration. Based on the
> request size and the imagesize we calculate the actual size per request.
> This has calculation has the benefit that the frame data is equally
> distributed over all allocated requests.
> 
> We keep the current req_size calculation as a fallback, if the interval
> callbacks did not set the interval property.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> v1 -> v2: - add headersize per request into calculation
> ---
>  drivers/usb/gadget/function/uvc_queue.c | 30 +++++++++++++++++++++++-------
>  drivers/usb/gadget/function/uvc_video.c |  2 +-
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index ce51643fc4639..141e52e34c610 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -44,7 +44,7 @@ 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);
> -	unsigned int req_size;
> +	unsigned int req_size, max_req_size, header_size;
>  	unsigned int nreq;
>  
>  	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)
> @@ -54,15 +54,31 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>  
>  	sizes[0] = video->imagesize;
>  
> -	req_size = video->ep->maxpacket
> +	nreq = DIV_ROUND_UP(video->interval, video->ep->desc->bInterval * 1250);

This seems problematic? I am not very well versed in the different USB speeds,
but IIRC fullspeed and highspeed enpoints have different bus intervals, and 
treat bInterval in different units (in frames for fs and in microframes for hs).

We likely need some speed specific logic when calculating nreq.

Assuming this logic is for >= hs, this allocates the exact number of 
usb_requests needed to stream a frame over to the host in one video 
frame interval. With the zero length backpressure still in place, this 
would mean that the actual video frame is sent over a period longer than
on video frame interval. I will try these patches locally, but if you
haven't already, please do check if you run into the problem you
brought up in https://lore.kernel.org/all/ZiWga5Kqno1ICv97@pengutronix.de/.
My guess is that the problem will show up here as well.

> +
> +	header_size = nreq * UVCG_REQUEST_HEADER_LEN;
> +
> +	req_size = DIV_ROUND_UP(video->imagesize + header_size, nreq);
> +
> +	max_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);
> +	if (!req_size) {
> +		req_size = max_req_size;
> +
> +		/* 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);
> +	} else if (req_size > max_req_size) {
> +		/* The prepared interval length and expected buffer size
> +		 * is not possible to stream with the currently configured
> +		 * isoc bandwidth
> +		 */
> +		return -EINVAL;
> +	}
>  
>  	video->req_size = req_size;
>  	video->uvc_num_requests = nreq;
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 95bb64e16f3da..d197c46e93fb4 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -304,7 +304,7 @@ static int uvcg_video_usb_req_queue(struct uvc_video *video,
>  		 */
>  		if (list_empty(&video->req_free) || ureq->last_buf ||
>  			!(video->req_int_count %
> -			DIV_ROUND_UP(video->uvc_num_requests, 4))) {
> +			clamp(DIV_ROUND_UP(video->uvc_num_requests, 4), 4U, 16U))) {
>  			video->req_int_count = 0;
>  			req->no_interrupt = 0;
>  		} else {
> 

Re: [PATCH v2 3/3] usb: gadget: uvc: set req_size and n_requests based on the frame interval

[Michael Grzeschik]

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

Hi Avichal,

On Wed, Jun 26, 2024 at 11:57:42AM -0700, Avichal Rakesh wrote:
>
>
>On 6/22/24 4:48 PM, Michael Grzeschik wrote:
>> With the information of the interval frame length it is now possible to
>> calculate the number of usb requests by the frame duration. Based on the
>> request size and the imagesize we calculate the actual size per request.
>> This has calculation has the benefit that the frame data is equally
>> distributed over all allocated requests.
>>
>> We keep the current req_size calculation as a fallback, if the interval
>> callbacks did not set the interval property.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>> ---
>> v1 -> v2: - add headersize per request into calculation
>> ---
>>  drivers/usb/gadget/function/uvc_queue.c | 30 +++++++++++++++++++++++-------
>>  drivers/usb/gadget/function/uvc_video.c |  2 +-
>>  2 files changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
>> index ce51643fc4639..141e52e34c610 100644
>> --- a/drivers/usb/gadget/function/uvc_queue.c
>> +++ b/drivers/usb/gadget/function/uvc_queue.c
>> @@ -44,7 +44,7 @@ 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);
>> -	unsigned int req_size;
>> +	unsigned int req_size, max_req_size, header_size;
>>  	unsigned int nreq;
>>
>>  	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)
>> @@ -54,15 +54,31 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>>
>>  	sizes[0] = video->imagesize;
>>
>> -	req_size = video->ep->maxpacket
>> +	nreq = DIV_ROUND_UP(video->interval, video->ep->desc->bInterval * 1250);
>
>This seems problematic? I am not very well versed in the different USB speeds,
>but IIRC fullspeed and highspeed enpoints have different bus intervals, and
>treat bInterval in different units (in frames for fs and in microframes for hs).
>
>We likely need some speed specific logic when calculating nreq.

Fair point! I did not think about that yet and will fix it in v3.

>Assuming this logic is for >= hs, this allocates the exact number of
>usb_requests needed to stream a frame over to the host in one video
>frame interval. With the zero length backpressure still in place, this
>would mean that the actual video frame is sent over a period longer than
>on video frame interval. I will try these patches locally, but if you
>haven't already, please do check if you run into the problem you
>brought up in https://lore.kernel.org/all/ZiWga5Kqno1ICv97@pengutronix.de/.
>My guess is that the problem will show up here as well.

Yes. With this current patchset there is not enough requests to keep
enqueueing requests fast enough since the interrupt handler will have to
wait for ready requests to show up while it uses the finishing requests
to fill zero length requests instead of giving them back to the free
pool. So just having the exact amount of requests for one frame interval
available is way to less.

I fixed that by creating at least four times the amount of available
requests. Just the way you already suggested in an earlier mail :) .

I also added an threshold that will only enqueue zero length requests
if the currently enqueued amount of requests is undercut.

However this is not enough to fulfill the requirements for the dwc3
gadget driver. We also have to ensure that the interrupt handler is not
running too long. To solve this I made additional changes. I sort them
this week and send a next version of it.

I hope you could review and test them soon.

Regards,
Michael

>> +
>> +	header_size = nreq * UVCG_REQUEST_HEADER_LEN;
>> +
>> +	req_size = DIV_ROUND_UP(video->imagesize + header_size, nreq);
>> +
>> +	max_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);
>> +	if (!req_size) {
>> +		req_size = max_req_size;
>> +
>> +		/* 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);
>> +	} else if (req_size > max_req_size) {
>> +		/* The prepared interval length and expected buffer size
>> +		 * is not possible to stream with the currently configured
>> +		 * isoc bandwidth
>> +		 */
>> +		return -EINVAL;
>> +	}
>>
>>  	video->req_size = req_size;
>>  	video->uvc_num_requests = nreq;
>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>> index 95bb64e16f3da..d197c46e93fb4 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -304,7 +304,7 @@ static int uvcg_video_usb_req_queue(struct uvc_video *video,
>>  		 */
>>  		if (list_empty(&video->req_free) || ureq->last_buf ||
>>  			!(video->req_int_count %
>> -			DIV_ROUND_UP(video->uvc_num_requests, 4))) {
>> +			clamp(DIV_ROUND_UP(video->uvc_num_requests, 4), 4U, 16U))) {
>>  			video->req_int_count = 0;
>>  			req->no_interrupt = 0;
>>  		} else {
>>
>

-- 
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 v2 2/3] usb: gadget: uvc: add g_parm and s_parm for frame interval

[Michael Grzeschik]

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

Hi Avichal,

On Wed, Jun 26, 2024 at 11:30:58AM -0700, Avichal Rakesh wrote:
>On 6/22/24 4:48 PM, Michael Grzeschik wrote:
>> The uvc gadget driver is lacking the information which frame interval
>> was set by the host. We add this information by implementing the g_parm
>> and s_parm callbacks.
>
>This change requires the userspace application (uvc-gagdet equivalent)
>to call s/g_parm when the FPS negotiations are finished. This is fine,
>but we should document that in the commit message here so implementers
>know that something needs to be done to take advantage of the change.

Fair point! I will do that for v3.

>On a similar note, the reference uvc-gadget should also be updated to
>call the added functions (and apologies if you've already put up a
>patch for it, I was unable find one).

Since I am only working with gstreamer with the uvcsink nowadays I
missed that. The internal v4l2sink does already do everything right. But
you are absolutely right. I will send an patch for uvc-gadget.

Regards,
Michael

>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>> ---
>> v1 -> v2: -
>> ---
>>  drivers/usb/gadget/function/uvc.h      |  1 +
>>  drivers/usb/gadget/function/uvc_v4l2.c | 52 ++++++++++++++++++++++++++++++++++
>>  2 files changed, 53 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
>> index cb35687b11e7e..d153bd9e35e31 100644
>> --- a/drivers/usb/gadget/function/uvc.h
>> +++ b/drivers/usb/gadget/function/uvc.h
>> @@ -97,6 +97,7 @@ struct uvc_video {
>>  	unsigned int width;
>>  	unsigned int height;
>>  	unsigned int imagesize;
>> +	unsigned int interval;
>>  	struct mutex mutex;	/* protects frame parameters */
>>
>>  	unsigned int uvc_num_requests;
>> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
>> index a024aecb76dc3..5b579ec1f5040 100644
>> --- a/drivers/usb/gadget/function/uvc_v4l2.c
>> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
>> @@ -307,6 +307,56 @@ uvc_v4l2_set_format(struct file *file, void *fh, struct v4l2_format *fmt)
>>  	return ret;
>>  }
>>
>> +static int uvc_v4l2_g_parm(struct file *file, void *fh,
>> +			    struct v4l2_streamparm *parm)
>> +{
>> +	struct video_device *vdev = video_devdata(file);
>> +	struct uvc_device *uvc = video_get_drvdata(vdev);
>> +	struct uvc_video *video = &uvc->video;
>> +	struct v4l2_fract timeperframe;
>> +
>> +	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> +		return -EINVAL;
>> +
>> +	/* Return the actual frame period. */
>> +	timeperframe.numerator = video->interval;
>> +	timeperframe.denominator = 10000000;
>> +	v4l2_simplify_fraction(&timeperframe.numerator,
>> +		&timeperframe.denominator, 8, 333);
>> +
>> +	uvcg_dbg(&uvc->func, "Getting frame interval of %u/%u (%u)\n",
>> +		timeperframe.numerator, timeperframe.denominator,
>> +		video->interval);
>> +
>> +	parm->parm.output.timeperframe = timeperframe;
>> +	parm->parm.output.capability = V4L2_CAP_TIMEPERFRAME;
>> +
>> +	return 0;
>> +}
>> +
>> +static int uvc_v4l2_s_parm(struct file *file, void *fh,
>> +			    struct v4l2_streamparm *parm)
>> +{
>> +	struct video_device *vdev = video_devdata(file);
>> +	struct uvc_device *uvc = video_get_drvdata(vdev);
>> +	struct uvc_video *video = &uvc->video;
>> +	struct v4l2_fract timeperframe;
>> +
>> +	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> +		return -EINVAL;
>> +
>> +	timeperframe = parm->parm.output.timeperframe;
>> +
>> +	video->interval = v4l2_fraction_to_interval(timeperframe.numerator,
>> +		timeperframe.denominator);
>> +
>> +	uvcg_dbg(&uvc->func, "Setting frame interval to %u/%u (%u)\n",
>> +		timeperframe.numerator, timeperframe.denominator,
>> +		video->interval);
>> +
>> +	return 0;
>> +}
>> +
>>  static int
>>  uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
>>  		struct v4l2_frmivalenum *fival)
>> @@ -577,6 +627,8 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>>  	.vidioc_dqbuf = uvc_v4l2_dqbuf,
>>  	.vidioc_streamon = uvc_v4l2_streamon,
>>  	.vidioc_streamoff = uvc_v4l2_streamoff,
>> +	.vidioc_s_parm = uvc_v4l2_s_parm,
>> +	.vidioc_g_parm = uvc_v4l2_g_parm,
>>  	.vidioc_subscribe_event = uvc_v4l2_subscribe_event,
>>  	.vidioc_unsubscribe_event = uvc_v4l2_unsubscribe_event,
>>  	.vidioc_default = uvc_v4l2_ioctl_default,
>>
>

-- 
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 --]