[PATCH v4 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers

[PATCH v4 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers

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

It also precalculates the request length based on the actual per frame
size for compressed formats.

For this calculations to work it was needed to rework the request
queueing by moving the encoding to one extra thread (in this case we
chose the qbuf) context.

Next it was needed to move the actual request enqueueing to one extra
thread which is kept busy to fill the isoc queue in the udc.

As a final step the series is increasing the minimum amount of
v4l2 buffers to 4 and allocates at least the amount of usb_requests
to store them in the usb gadgte isoc pipeline.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
Changes in v4:
- fixed exit path in uvc_enqueue_buffer on loop break
- Link to v3: https://lore.kernel.org/r/20240403-uvc_request_length_by_interval-v3-0-4da7033dd488@pengutronix.de

Changes in v3:
- Added more patches necessary to properly rework the request queueing
- Link to v2: https://lore.kernel.org/r/20240403-uvc_request_length_by_interval-v2-0-12690f7a2eff@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 (10):
      usb: gadget: uvc: always set interrupt on zero length requests
      usb: gadget: uvc: only enqueue zero length requests in potential underrun
      usb: gadget: uvc: remove pump worker and enqueue all buffers per frame in qbuf
      usb: gadget: uvc: rework to enqueue in pump worker from encoded queue
      usb: gadget: uvc: remove uvc_video_ep_queue_initial_requests
      usb: gadget: 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
      usb: gadget: uvc: set req_length based on payload by nreqs instead of req_size
      usb: gadget: uvc: add min g_ctrl vidioc and set min buffs to 4

 drivers/usb/gadget/function/f_uvc.c     |   3 +-
 drivers/usb/gadget/function/uvc.h       |  14 +-
 drivers/usb/gadget/function/uvc_queue.c |  52 +++++--
 drivers/usb/gadget/function/uvc_queue.h |   1 +
 drivers/usb/gadget/function/uvc_v4l2.c  |  67 ++++++++-
 drivers/usb/gadget/function/uvc_video.c | 243 +++++++++++++-------------------
 drivers/usb/gadget/function/uvc_video.h |   1 +
 7 files changed, 221 insertions(+), 160 deletions(-)
---
base-commit: 38343be0bf9a7d7ef0d160da5f2db887a0e29b62
change-id: 20240403-uvc_request_length_by_interval-a7efd587d963

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

[PATCH v4 01/10] usb: gadget: uvc: always set interrupt on zero length requests

[Michael Grzeschik]

Since the uvc gadget is depending on the completion handler to
properly enqueue new data, we have to ensure that the requeue mechanism
is always working. To be safe we always create an interrupt
on zero length requests.

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

---
v3 -> v4: -
v1 -> v3: new patch
---
 drivers/usb/gadget/function/uvc_video.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index d41f5f31dadd5..03bff39cd4902 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -303,6 +303,7 @@ static int uvcg_video_usb_req_queue(struct uvc_video *video,
 		 *   between latency and interrupt load.
 		 */
 		if (list_empty(&video->req_free) || ureq->last_buf ||
+				!req->length ||
 			!(video->req_int_count %
 			DIV_ROUND_UP(video->uvc_num_requests, 4))) {
 			video->req_int_count = 0;

-- 
2.39.2

Re: [PATCH v4 01/10] usb: gadget: uvc: always set interrupt on zero length requests

[Laurent Pinchart]

Hi Michael,

Thank you for the patch.

On Tue, Aug 13, 2024 at 11:09:25AM +0200, Michael Grzeschik wrote:
> Since the uvc gadget is depending on the completion handler to
> properly enqueue new data, we have to ensure that the requeue mechanism
> is always working. To be safe we always create an interrupt
> on zero length requests.

What do you mean "to be safe" ? Either there's an issue, and then the
commit message should describe it, or there's no issue, and this isn't
needed.

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> v3 -> v4: -
> v1 -> v3: new patch
> ---
>  drivers/usb/gadget/function/uvc_video.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index d41f5f31dadd5..03bff39cd4902 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -303,6 +303,7 @@ static int uvcg_video_usb_req_queue(struct uvc_video *video,
>  		 *   between latency and interrupt load.
>  		 */
>  		if (list_empty(&video->req_free) || ureq->last_buf ||
> +				!req->length ||
>  			!(video->req_int_count %
>  			DIV_ROUND_UP(video->uvc_num_requests, 4))) {
>  			video->req_int_count = 0;

-- 
Regards,

Laurent Pinchart

[PATCH v4 02/10] usb: gadget: uvc: only enqueue zero length requests in potential underrun

[Michael Grzeschik]

The complete handler will at least be called after 16 requests have
completed, but will still handle all finisher requests. Since we have
to maintain a costant filling in the isoc queue we ensure this by
adding zero length requests.

By counting the amount enqueued requests we can ensure that the queue is
never underrun and only need to get active if the queue is running
critical. This patch is setting 32 as the critical level, which
is twice the request amount that is needed to create interrupts.

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

---
v3 -> v4: -
v1 -> v3: new patch
---
 drivers/usb/gadget/function/uvc.h       | 5 ++++-
 drivers/usb/gadget/function/uvc_video.c | 7 +++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index cb35687b11e7e..646f1c01c5101 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -91,6 +91,9 @@ struct uvc_video {
 	struct work_struct pump;
 	struct workqueue_struct *async_wq;
 
+	int enqueued;
+	int dequeued;
+
 	/* Frame parameters */
 	u8 bpp;
 	u32 fcc;
@@ -106,7 +109,7 @@ struct uvc_video {
 	unsigned int req_size;
 	struct list_head ureqs; /* all uvc_requests allocated by uvc_video */
 
-	/* USB requests that the video pump thread can encode into */
+	/* USB requests that the video qbuf thread can encode into */
 	struct list_head req_free;
 
 	/*
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 03bff39cd4902..d47ddd674f457 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -269,6 +269,8 @@ static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req)
 		}
 	}
 
+	video->enqueued++;
+
 	return ret;
 }
 
@@ -380,6 +382,7 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 	int ret = 0;
 
 	spin_lock_irqsave(&video->req_lock, flags);
+	video->dequeued++;
 	if (!video->is_enabled) {
 		/*
 		 * When is_enabled is false, uvcg_video_disable() ensures
@@ -467,6 +470,10 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 		 * happen.
 		 */
 		queue_work(video->async_wq, &video->pump);
+	} else if ((video->enqueued - video->dequeued) > 32) {
+		spin_unlock_irqrestore(&video->req_lock, flags);
+
+		return;
 	}
 	/*
 	 * Queue to the endpoint. The actual queueing to ep will

-- 
2.39.2

[PATCH v4 03/10] usb: gadget: uvc: remove pump worker and enqueue all buffers per frame in qbuf

[Michael Grzeschik]

Since we now have at least the amount of requests to fill every frame
into the isoc queue that is requested with the REQBUFS ioctl, we can
directly encode all incoming frames into the available requests.

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

---
v3 -> v4: fixed exit path in uvc_enqueue_buffer by exceptionally call cleanup code on break
v1 -> v3: new patch
---
 drivers/usb/gadget/function/f_uvc.c     |  4 ---
 drivers/usb/gadget/function/uvc.h       |  5 +---
 drivers/usb/gadget/function/uvc_queue.c |  3 ++
 drivers/usb/gadget/function/uvc_v4l2.c  |  3 --
 drivers/usb/gadget/function/uvc_video.c | 53 ++++++++-------------------------
 drivers/usb/gadget/function/uvc_video.h |  1 +
 6 files changed, 18 insertions(+), 51 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 40187b7112e79..aeaf355a86eb3 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -986,14 +986,10 @@ 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
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 646f1c01c5101..e252c3db73072 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -88,9 +88,6 @@ struct uvc_video {
 	struct uvc_device *uvc;
 	struct usb_ep *ep;
 
-	struct work_struct pump;
-	struct workqueue_struct *async_wq;
-
 	int enqueued;
 	int dequeued;
 
@@ -113,7 +110,7 @@ struct uvc_video {
 	struct list_head req_free;
 
 	/*
-	 * USB requests video pump thread has already encoded into. These are
+	 * USB requests video qbuf thread has already encoded into. These are
 	 * ready to be queued to the endpoint.
 	 */
 	struct list_head req_ready;
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index 0aa3d7e1f3cc3..7995dd3fef184 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -102,6 +102,7 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
 static void uvc_buffer_queue(struct vb2_buffer *vb)
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
+	struct uvc_video *video = container_of(queue, struct uvc_video, queue);
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct uvc_buffer *buf = container_of(vbuf, struct uvc_buffer, buf);
 	unsigned long flags;
@@ -120,6 +121,8 @@ static void uvc_buffer_queue(struct vb2_buffer *vb)
 	}
 
 	spin_unlock_irqrestore(&queue->irqlock, flags);
+
+	uvc_enqueue_buffer(video, buf);
 }
 
 static const struct vb2_ops uvc_queue_qops = {
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index a024aecb76dc3..4085f459c3c70 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -429,9 +429,6 @@ uvc_v4l2_qbuf(struct file *file, void *fh, struct v4l2_buffer *b)
 	if (ret < 0)
 		return ret;
 
-	if (uvc->state == UVC_STATE_STREAMING)
-		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 d47ddd674f457..5b0bf8069d48f 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -445,7 +445,7 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 	/*
 	 * Here we check whether any request is available in the ready
 	 * list. If it is, queue it to the ep and add the current
-	 * usb_request to the req_free list - for video_pump to fill in.
+	 * usb_request to the req_free list - for qbuf to fill in.
 	 * Otherwise, just use the current usb_request to queue a 0
 	 * length request to the ep. Since we always add to the req_free
 	 * list if we dequeue from the ready list, there will never
@@ -469,7 +469,6 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 		 * dequeue -> queue -> dequeue flow of uvc buffers will not
 		 * happen.
 		 */
-		queue_work(video->async_wq, &video->pump);
 	} else if ((video->enqueued - video->dequeued) > 32) {
 		spin_unlock_irqrestore(&video->req_lock, flags);
 
@@ -566,27 +565,15 @@ uvc_video_alloc_requests(struct uvc_video *video)
  * Video streaming
  */
 
-/*
- * uvcg_video_pump - Pump video data into the USB requests
- *
- * This function fills the available USB requests (listed in req_free) with
- * video data from the queued buffers.
- */
-static void uvcg_video_pump(struct work_struct *work)
+int uvc_enqueue_buffer(struct uvc_video *video, struct uvc_buffer *buf)
 {
-	struct uvc_video *video = container_of(work, struct uvc_video, pump);
 	struct uvc_video_queue *queue = &video->queue;
-	/* video->max_payload_size is only set when using bulk transfer */
-	bool is_bulk = video->max_payload_size;
 	struct usb_request *req = NULL;
-	struct uvc_buffer *buf;
+	bool is_bulk = video->max_payload_size;
 	unsigned long flags;
 	int ret = 0;
 
-	while (true) {
-		if (!video->ep->enabled)
-			return;
-
+	while (buf->state != UVC_BUF_STATE_DONE) {
 		/*
 		 * Check is_enabled and retrieve the first available USB
 		 * request, protected by the request lock.
@@ -594,7 +581,7 @@ static void uvcg_video_pump(struct work_struct *work)
 		spin_lock_irqsave(&video->req_lock, flags);
 		if (!video->is_enabled || list_empty(&video->req_free)) {
 			spin_unlock_irqrestore(&video->req_lock, flags);
-			return;
+			return -ENOENT;
 		}
 		req = list_first_entry(&video->req_free, struct usb_request,
 					list);
@@ -605,22 +592,8 @@ static void uvcg_video_pump(struct work_struct *work)
 		 * Retrieve the first available video buffer and fill the
 		 * request, protected by the video queue irqlock.
 		 */
-		spin_lock_irqsave(&queue->irqlock, flags);
-		buf = uvcg_queue_head(queue);
-		if (!buf) {
-			/*
-			 * Either the queue has been disconnected or no video buffer
-			 * available for bulk transfer. Either way, stop processing
-			 * further.
-			 */
-			spin_unlock_irqrestore(&queue->irqlock, flags);
-			break;
-		}
-
 		video->encode(req, video, buf);
 
-		spin_unlock_irqrestore(&queue->irqlock, flags);
-
 		spin_lock_irqsave(&video->req_lock, flags);
 		/* For bulk end points we queue from the worker thread
 		 * since we would preferably not want to wait on requests
@@ -633,15 +606,22 @@ static void uvcg_video_pump(struct work_struct *work)
 
 		if (ret < 0) {
 			uvcg_queue_cancel(queue, 0);
-			break;
+			goto cleanup;
 		}
 	}
+
+	return 0;
+
+cleanup:
+
 	spin_lock_irqsave(&video->req_lock, flags);
 	if (video->is_enabled)
 		list_add_tail(&req->list, &video->req_free);
 	else
 		uvc_video_free_request(req->context, video->ep);
 	spin_unlock_irqrestore(&video->req_lock, flags);
+
+	return 0;
 }
 
 /*
@@ -681,7 +661,6 @@ uvcg_video_disable(struct uvc_video *video)
 	}
 	spin_unlock_irqrestore(&video->req_lock, flags);
 
-	cancel_work_sync(&video->pump);
 	uvcg_queue_cancel(&video->queue, 0);
 
 	spin_lock_irqsave(&video->req_lock, flags);
@@ -775,12 +754,6 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
 	INIT_LIST_HEAD(&video->req_free);
 	INIT_LIST_HEAD(&video->req_ready);
 	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("uvcgadget", WQ_UNBOUND | WQ_HIGHPRI, 0);
-	if (!video->async_wq)
-		return -EINVAL;
 
 	video->uvc = uvc;
 	video->fcc = V4L2_PIX_FMT_YUYV;
diff --git a/drivers/usb/gadget/function/uvc_video.h b/drivers/usb/gadget/function/uvc_video.h
index 8ef6259741f13..2f30ebd05fefb 100644
--- a/drivers/usb/gadget/function/uvc_video.h
+++ b/drivers/usb/gadget/function/uvc_video.h
@@ -17,6 +17,7 @@ struct uvc_video;
 int uvcg_video_enable(struct uvc_video *video);
 int uvcg_video_disable(struct uvc_video *video);
 
+int uvc_enqueue_buffer(struct uvc_video *video, struct uvc_buffer *buf);
 int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc);
 
 #endif /* __UVC_VIDEO_H__ */

-- 
2.39.2

[PATCH v4 04/10] usb: gadget: uvc: rework to enqueue in pump worker from encoded queue

[Michael Grzeschik]

We install an kthread with pfifo prioroity that is iterating over all
prepared requests and keeps the isoc queue busy. It also watches the
theshold to enqueue zero length requests if needed. This way it will be
scheduled with the same priority as the interrupt handler.

But the interrupt handler will not be running into the time consuming
and eventually locking work of actually enqueueing the requests back
into its own pipeline. This work can now even can be scheduled on
another cpu.

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

---
v3 -> v4: -
v1 -> v3: new patch
---
 drivers/usb/gadget/function/f_uvc.c     |   3 +
 drivers/usb/gadget/function/uvc.h       |   3 +
 drivers/usb/gadget/function/uvc_v4l2.c  |   3 +
 drivers/usb/gadget/function/uvc_video.c | 118 ++++++++++++++++++++------------
 4 files changed, 84 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index aeaf355a86eb3..1609daf85a258 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -986,10 +986,13 @@ static void uvc_function_unbind(struct usb_configuration *c,
 {
 	struct usb_composite_dev *cdev = c->cdev;
 	struct uvc_device *uvc = to_uvc(f);
+	struct uvc_video *video = &uvc->video;
 	long wait_ret = 1;
 
 	uvcg_info(f, "%s()\n", __func__);
 
+	kthread_cancel_work_sync(&video->pump);
+
 	/*
 	 * 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
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index e252c3db73072..b3a5165ac70ec 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -88,6 +88,9 @@ struct uvc_video {
 	struct uvc_device *uvc;
 	struct usb_ep *ep;
 
+	struct kthread_worker	*kworker;
+	struct kthread_work	pump;
+
 	int enqueued;
 	int dequeued;
 
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 4085f459c3c70..de41519ce9aa0 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -429,6 +429,9 @@ uvc_v4l2_qbuf(struct file *file, void *fh, struct v4l2_buffer *b)
 	if (ret < 0)
 		return ret;
 
+	if (uvc->state == UVC_STATE_STREAMING)
+		kthread_queue_work(video->kworker, &video->pump);
+
 	return ret;
 }
 
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 5b0bf8069d48f..a51e95e3b717c 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -376,10 +376,7 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 	struct uvc_video *video = ureq->video;
 	struct uvc_video_queue *queue = &video->queue;
 	struct uvc_buffer *last_buf;
-	struct usb_request *to_queue = req;
 	unsigned long flags;
-	bool is_bulk = video->max_payload_size;
-	int ret = 0;
 
 	spin_lock_irqsave(&video->req_lock, flags);
 	video->dequeued++;
@@ -442,54 +439,78 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 		return;
 	}
 
+	list_add_tail(&req->list, &video->req_free);
+
+	spin_unlock_irqrestore(&video->req_lock, flags);
+
 	/*
-	 * Here we check whether any request is available in the ready
-	 * list. If it is, queue it to the ep and add the current
-	 * usb_request to the req_free list - for qbuf to fill in.
-	 * Otherwise, just use the current usb_request to queue a 0
-	 * length request to the ep. Since we always add to the req_free
-	 * list if we dequeue from the ready list, there will never
-	 * be a situation where the req_free list is completely out of
-	 * requests and cannot recover.
+	 * Queue work to the wq as well since it is possible that a
+	 * buffer may not have been completely encoded with the set of
+	 * in-flight usb requests for whih the complete callbacks are
+	 * firing.
+	 * In that case, if we do not queue work to the worker thread,
+	 * the buffer will never be marked as complete - and therefore
+	 * not be returned to userpsace. As a result,
+	 * dequeue -> queue -> dequeue flow of uvc buffers will not
+	 * happen.
 	 */
-	to_queue->length = 0;
-	if (!list_empty(&video->req_ready)) {
-		to_queue = list_first_entry(&video->req_ready,
-			struct usb_request, list);
-		list_del(&to_queue->list);
-		list_add_tail(&req->list, &video->req_free);
+	kthread_queue_work(video->kworker, &video->pump);
+}
+
+static void uvcg_video_pump(struct kthread_work *work)
+{
+	struct uvc_video *video = container_of(work, struct uvc_video, pump);
+	bool is_bulk = video->max_payload_size;
+	unsigned long flags;
+	struct usb_request *req;
+	int ret = 0;
+
+	while (true) {
+		if (!video->ep->enabled)
+			return;
+		spin_lock_irqsave(&video->req_lock, flags);
 		/*
-		 * Queue work to the wq as well since it is possible that a
-		 * buffer may not have been completely encoded with the set of
-		 * in-flight usb requests for whih the complete callbacks are
-		 * firing.
-		 * In that case, if we do not queue work to the worker thread,
-		 * the buffer will never be marked as complete - and therefore
-		 * not be returned to userpsace. As a result,
-		 * dequeue -> queue -> dequeue flow of uvc buffers will not
-		 * happen.
+		 * Here we check whether any request is available in the ready
+		 * list. If it is, queue it to the ep and add the current
+		 * usb_request to the req_free list - for video_pump to fill in.
+		 * Otherwise, just use the current usb_request to queue a 0
+		 * length request to the ep. Since we always add to the req_free
+		 * list if we dequeue from the ready list, there will never
+		 * be a situation where the req_free list is completely out of
+		 * requests and cannot recover.
 		 */
-	} else if ((video->enqueued - video->dequeued) > 32) {
-		spin_unlock_irqrestore(&video->req_lock, flags);
+		if (!list_empty(&video->req_ready)) {
+			req = list_first_entry(&video->req_ready,
+					       struct usb_request, list);
+		} else {
+			if (list_empty(&video->req_free) || (video->enqueued - video->dequeued) > 32) {
+				spin_unlock_irqrestore(&video->req_lock, flags);
+
+				return;
+			}
+			req = list_first_entry(&video->req_free, struct usb_request,
+					       list);
+			req->length = 0;
+		}
+		list_del(&req->list);
 
-		return;
-	}
-	/*
-	 * Queue to the endpoint. The actual queueing to ep will
-	 * only happen on one thread - the async_wq for bulk endpoints
-	 * and this thread for isoc endpoints.
-	 */
-	ret = uvcg_video_usb_req_queue(video, to_queue, !is_bulk);
-	if (ret < 0) {
 		/*
-		 * Endpoint error, but the stream is still enabled.
-		 * Put request back in req_free for it to be cleaned
-		 * up later.
+		 * Queue to the endpoint. The actual queueing to ep will
+		 * only happen on one thread - the async_wq for bulk endpoints
+		 * and this thread for isoc endpoints.
 		 */
-		list_add_tail(&to_queue->list, &video->req_free);
-	}
+		ret = uvcg_video_usb_req_queue(video, req, !is_bulk);
+		if (ret < 0) {
+			/*
+			 * Endpoint error, but the stream is still enabled.
+			 * Put request back in req_free for it to be cleaned
+			 * up later.
+			 */
+			list_add_tail(&req->list, &video->req_free);
+		}
 
-	spin_unlock_irqrestore(&video->req_lock, flags);
+		spin_unlock_irqrestore(&video->req_lock, flags);
+	}
 }
 
 static int
@@ -755,6 +776,17 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
 	INIT_LIST_HEAD(&video->req_ready);
 	spin_lock_init(&video->req_lock);
 
+	/* Allocate a work queue for asynchronous video pump handler. */
+	video->kworker = kthread_create_worker(0, "UVCG");
+	if (IS_ERR(video->kworker)) {
+		uvcg_err(&video->uvc->func, "failed to create message pump kworker\n");
+		return PTR_ERR(video->kworker);
+	}
+
+	kthread_init_work(&video->pump, uvcg_video_pump);
+
+	sched_set_fifo(video->kworker->task);
+
 	video->uvc = uvc;
 	video->fcc = V4L2_PIX_FMT_YUYV;
 	video->bpp = 16;

-- 
2.39.2

[PATCH v4 05/10] usb: gadget: uvc: remove uvc_video_ep_queue_initial_requests

[Michael Grzeschik]

By moving the buffer enqueing to an extra worker and not
enqueueing the each request by its corresponding complete
interrupt, we can remove the initial amount of zero length requests.

As soon as real data is available the isoc queue will be filled
as much as possible.

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

---
v3 -> v4: -
v1 -> v3: new patch
---
 drivers/usb/gadget/function/uvc_video.c | 46 ---------------------------------
 1 file changed, 46 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index a51e95e3b717c..259920ae36843 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -325,50 +325,6 @@ static int uvcg_video_usb_req_queue(struct uvc_video *video,
 	return 0;
 }
 
-/*
- * Must only be called from uvcg_video_enable - since after that we only want to
- * queue requests to the endpoint from the uvc_video_complete complete handler.
- * This function is needed in order to 'kick start' the flow of requests from
- * gadget driver to the usb controller.
- */
-static void uvc_video_ep_queue_initial_requests(struct uvc_video *video)
-{
-	struct usb_request *req = NULL;
-	unsigned long flags = 0;
-	unsigned int count = 0;
-	int ret = 0;
-
-	/*
-	 * We only queue half of the free list since we still want to have
-	 * some free usb_requests in the free list for the video_pump async_wq
-	 * thread to encode uvc buffers into. Otherwise we could get into a
-	 * situation where the free list does not have any usb requests to
-	 * encode into - we always end up queueing 0 length requests to the
-	 * end point.
-	 */
-	unsigned int half_list_size = video->uvc_num_requests / 2;
-
-	spin_lock_irqsave(&video->req_lock, flags);
-	/*
-	 * Take these requests off the free list and queue them all to the
-	 * endpoint. Since we queue 0 length requests with the req_lock held,
-	 * there isn't any 'data' race involved here with the complete handler.
-	 */
-	while (count < half_list_size) {
-		req = list_first_entry(&video->req_free, struct usb_request,
-					list);
-		list_del(&req->list);
-		req->length = 0;
-		ret = uvcg_video_ep_queue(video, req);
-		if (ret < 0) {
-			uvcg_queue_cancel(&video->queue, 0);
-			break;
-		}
-		count++;
-	}
-	spin_unlock_irqrestore(&video->req_lock, flags);
-}
-
 static void
 uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 {
@@ -760,8 +716,6 @@ int uvcg_video_enable(struct uvc_video *video)
 
 	video->req_int_count = 0;
 
-	uvc_video_ep_queue_initial_requests(video);
-
 	return ret;
 }
 

-- 
2.39.2

[PATCH v4 06/10] usb: gadget: 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>

---
v3 -> v4: -
v2 -> v3: -
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 7995dd3fef184..2414d78b031f4 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 259920ae36843..a6786beef91ad 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -480,7 +480,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;
 }
 
@@ -488,16 +487,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)
@@ -507,7 +499,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;
 
@@ -525,12 +517,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:
@@ -663,7 +653,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

Re: [PATCH v4 06/10] usb: gadget: uvc: set req_size once when the vb2 queue is calculated

[Laurent Pinchart]

On Tue, Aug 13, 2024 at 11:09:30AM +0200, Michael Grzeschik wrote:
> 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.

Avoiding double calculations is good, but then don't compute the value
in uvc_queue_setup(). That will also be called multiple times, and its
timing will be controlled by userspace. Move it to a better location.

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> v3 -> v4: -
> v2 -> v3: -
> 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 7995dd3fef184..2414d78b031f4 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 259920ae36843..a6786beef91ad 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -480,7 +480,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;
>  }
>  
> @@ -488,16 +487,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)
> @@ -507,7 +499,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;
>  
> @@ -525,12 +517,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:
> @@ -663,7 +653,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);
>  
>  	/*

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/10] usb: gadget: uvc: set req_size once when the vb2 queue is calculated

[Michael Grzeschik]

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

On Tue, Aug 13, 2024 at 12:41:10PM +0300, Laurent Pinchart wrote:
>On Tue, Aug 13, 2024 at 11:09:30AM +0200, Michael Grzeschik wrote:
>> 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.
>
>Avoiding double calculations is good, but then don't compute the value
>in uvc_queue_setup(). That will also be called multiple times, and its
>timing will be controlled by userspace. Move it to a better location.

I think I was unclear regarding the avoidance of double calculations.

Actually this is the right place to calculate this, since the
resulting req_size will be dependent on the expected imagesize
and the frameduration, we have to calculate it in every queue_setup.

Since this patch is an preperation for the next change, I will leave the
codebase as is but will add the details in the patch description.

Michael

>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>> ---
>> v3 -> v4: -
>> v2 -> v3: -
>> 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 7995dd3fef184..2414d78b031f4 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 259920ae36843..a6786beef91ad 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -480,7 +480,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;
>>  }
>>
>> @@ -488,16 +487,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)
>> @@ -507,7 +499,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;
>>
>> @@ -525,12 +517,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:
>> @@ -663,7 +653,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);
>>
>>  	/*
>
>-- 
>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 v4 07/10] 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>

---
v3 -> v4: -
v2 -> v3: -
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 b3a5165ac70ec..f6bc58fb02b84 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -100,6 +100,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 de41519ce9aa0..392fb400aad14 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

Re: [PATCH v4 07/10] usb: gadget: uvc: add g_parm and s_parm for frame interval

[Laurent Pinchart]

On Tue, Aug 13, 2024 at 11:09:31AM +0200, 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.

As I've said countless times, this kind of hack is not the right way to
pass information that the kernel has no use for between two userspace
components. Please stop butchering this driver.

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> v3 -> v4: -
> v2 -> v3: -
> 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 b3a5165ac70ec..f6bc58fb02b84 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -100,6 +100,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 de41519ce9aa0..392fb400aad14 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,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 07/10] usb: gadget: uvc: add g_parm and s_parm for frame interval

[Laurent Pinchart]

On Tue, Aug 13, 2024 at 12:22:55PM +0300, Laurent Pinchart wrote:
> On Tue, Aug 13, 2024 at 11:09:31AM +0200, 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.
> 
> As I've said countless times, this kind of hack is not the right way to
> pass information that the kernel has no use for between two userspace
> components. Please stop butchering this driver.

Reading further patches in the series I see that you would like to get
more precise bandwidth information from userspace. That is fine, but I
don't think s_parm is the right option. This is not a regular V4L2
driver, pass it the exat information it needs instead, through a
dedicated API that will provide all the needed data.

> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> > v3 -> v4: -
> > v2 -> v3: -
> > 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 b3a5165ac70ec..f6bc58fb02b84 100644
> > --- a/drivers/usb/gadget/function/uvc.h
> > +++ b/drivers/usb/gadget/function/uvc.h
> > @@ -100,6 +100,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 de41519ce9aa0..392fb400aad14 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,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 07/10] usb: gadget: uvc: add g_parm and s_parm for frame interval

[Michael Grzeschik]

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

On Tue, Aug 13, 2024 at 12:28:20PM +0300, Laurent Pinchart wrote:
>On Tue, Aug 13, 2024 at 12:22:55PM +0300, Laurent Pinchart wrote:
>> On Tue, Aug 13, 2024 at 11:09:31AM +0200, 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.
>>
>> As I've said countless times, this kind of hack is not the right way to
>> pass information that the kernel has no use for between two userspace
>> components. Please stop butchering this driver.
>
>Reading further patches in the series I see that you would like to get
>more precise bandwidth information from userspace. That is fine, but I
>don't think s_parm is the right option. This is not a regular V4L2
>driver, pass it the exat information it needs instead, through a
>dedicated API that will provide all the needed data.

We have an API, where we can handover the framerate. Why invent
something new. All we need to fix is also patch you uvc-gadget
implementation.

What other API do you suggest then? Come up with something super new and
special, only dedicated for UVC then?

Regards,
Michael

>> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> > ---
>> > v3 -> v4: -
>> > v2 -> v3: -
>> > 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 b3a5165ac70ec..f6bc58fb02b84 100644
>> > --- a/drivers/usb/gadget/function/uvc.h
>> > +++ b/drivers/usb/gadget/function/uvc.h
>> > @@ -100,6 +100,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 de41519ce9aa0..392fb400aad14 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,
>
>-- 
>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 --]

Re: [PATCH v4 07/10] usb: gadget: uvc: add g_parm and s_parm for frame interval

[Laurent Pinchart]

On Tue, Aug 13, 2024 at 12:27:42PM +0200, Michael Grzeschik wrote:
> On Tue, Aug 13, 2024 at 12:28:20PM +0300, Laurent Pinchart wrote:
> >On Tue, Aug 13, 2024 at 12:22:55PM +0300, Laurent Pinchart wrote:
> >> On Tue, Aug 13, 2024 at 11:09:31AM +0200, 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.
> >>
> >> As I've said countless times, this kind of hack is not the right way to
> >> pass information that the kernel has no use for between two userspace
> >> components. Please stop butchering this driver.
> >
> > Reading further patches in the series I see that you would like to get
> > more precise bandwidth information from userspace. That is fine, but I
> > don't think s_parm is the right option. This is not a regular V4L2
> > driver, pass it the exat information it needs instead, through a
> > dedicated API that will provide all the needed data.
> 
> We have an API, where we can handover the framerate. Why invent
> something new. All we need to fix is also patch you uvc-gadget
> implementation.
> 
> What other API do you suggest then? Come up with something super new and
> special, only dedicated for UVC then?

Yes, and that's what should have been done instead of adding support for
the V4L2 format-related ioctls.

We can't change the past, but I'm tired and sad to see the driver
evolving in a direction I don't think is right. I however don't have
much time to maintain it these days, and it's obviously not fair to
block progress, even if I believe that good progress would have taken an
entirely different direction.

I will submit a patch to remove myself from MAINTAINERS for this driver.
Come what may.

> >> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> >> > ---
> >> > v3 -> v4: -
> >> > v2 -> v3: -
> >> > 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 b3a5165ac70ec..f6bc58fb02b84 100644
> >> > --- a/drivers/usb/gadget/function/uvc.h
> >> > +++ b/drivers/usb/gadget/function/uvc.h
> >> > @@ -100,6 +100,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 de41519ce9aa0..392fb400aad14 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,

-- 
Regards,

Laurent Pinchart

[PATCH v4 08/10] 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 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>

---
v3 -> v4: -
v2 -> v3:
 - added the frame duration for full-speed devices into calculation
v1 -> v2:
 - add headersize per request into calculation
---
 drivers/usb/gadget/function/uvc_queue.c | 35 ++++++++++++++++++++++++++-------
 drivers/usb/gadget/function/uvc_video.c |  2 +-
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index 2414d78b031f4..ab04df0e4f360 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -44,7 +44,9 @@ 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;
+	struct usb_composite_dev *cdev = video->uvc->func.config->cdev;
+	unsigned int interval_duration = video->ep->desc->bInterval * 1250;
+	unsigned int req_size, max_req_size, header_size;
 	unsigned int nreq;
 
 	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)
@@ -54,15 +56,34 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 
 	sizes[0] = video->imagesize;
 
-	req_size = video->ep->maxpacket
+	if (cdev->gadget->speed < USB_SPEED_HIGH)
+		interval_duration = video->ep->desc->bInterval * 10000;
+
+	nreq = DIV_ROUND_UP(video->interval, interval_duration);
+
+	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 a6786beef91ad..fe19608b57720 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -307,7 +307,7 @@ static int uvcg_video_usb_req_queue(struct uvc_video *video,
 		if (list_empty(&video->req_free) || ureq->last_buf ||
 				!req->length ||
 			!(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

[PATCH v4 09/10] usb: gadget: uvc: set req_length based on payload by nreqs instead of req_size

[Michael Grzeschik]

For uncompressed formats it makes sense to fill the requests with its
maximum since the amount of requests and its size is calculated for
this exact amount. Compressed formats generate content depending amount
of data that is set in the vb2 buffer by the payload_size. When
streaming those formats it is even better to scatter that smaller
data over all requests.

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

---
v3 -> v4: -
v1 -> v3: new patch
---
 drivers/usb/gadget/function/uvc_queue.c |  9 ++++++++-
 drivers/usb/gadget/function/uvc_queue.h |  1 +
 drivers/usb/gadget/function/uvc_video.c | 13 ++++++-------
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index ab04df0e4f360..e33ce72325031 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -94,6 +94,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 static int uvc_buffer_prepare(struct vb2_buffer *vb)
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
+	struct uvc_video *video = container_of(queue, struct uvc_video, queue);
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct uvc_buffer *buf = container_of(vbuf, struct uvc_buffer, buf);
 
@@ -116,8 +117,14 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
 	buf->length = vb2_plane_size(vb, 0);
 	if (vb->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		buf->bytesused = 0;
-	else
+	else {
+		unsigned int nreq;
+		nreq = DIV_ROUND_UP(video->interval, video->ep->desc->bInterval * 1250);
 		buf->bytesused = vb2_get_plane_payload(vb, 0);
+		buf->req_payload_size =
+			  DIV_ROUND_UP(buf->bytesused +
+					(nreq * UVCG_REQUEST_HEADER_LEN), nreq);
+	}
 
 	return 0;
 }
diff --git a/drivers/usb/gadget/function/uvc_queue.h b/drivers/usb/gadget/function/uvc_queue.h
index 41f87b917f6bc..a7355442dd6cd 100644
--- a/drivers/usb/gadget/function/uvc_queue.h
+++ b/drivers/usb/gadget/function/uvc_queue.h
@@ -39,6 +39,7 @@ struct uvc_buffer {
 	unsigned int offset;
 	unsigned int length;
 	unsigned int bytesused;
+	unsigned int req_payload_size;
 };
 
 #define UVC_QUEUE_DISCONNECTED		(1 << 0)
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index fe19608b57720..a5cf4dbdc5d59 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -136,7 +136,7 @@ uvc_video_encode_isoc_sg(struct usb_request *req, struct uvc_video *video,
 	unsigned int pending = buf->bytesused - video->queue.buf_used;
 	struct uvc_request *ureq = req->context;
 	struct scatterlist *sg, *iter;
-	unsigned int len = video->req_size;
+	unsigned int len = buf->req_payload_size;
 	unsigned int sg_left, part = 0;
 	unsigned int i;
 	int header_len;
@@ -145,16 +145,15 @@ uvc_video_encode_isoc_sg(struct usb_request *req, struct uvc_video *video,
 	sg_init_table(sg, ureq->sgt.nents);
 
 	/* Init the header. */
-	header_len = uvc_video_encode_header(video, buf, ureq->header,
-				      video->req_size);
+	header_len = uvc_video_encode_header(video, buf, ureq->header, len);
 	sg_set_buf(sg, ureq->header, header_len);
 	len -= header_len;
 
 	if (pending <= len)
 		len = pending;
 
-	req->length = (len == pending) ?
-		len + header_len : video->req_size;
+	req->length = (len == pending) ? len + header_len :
+		buf->req_payload_size;
 
 	/* Init the pending sgs with payload */
 	sg = sg_next(sg);
@@ -202,7 +201,7 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
 {
 	void *mem = req->buf;
 	struct uvc_request *ureq = req->context;
-	int len = video->req_size;
+	int len = buf->req_payload_size;
 	int ret;
 
 	/* Add the header. */
@@ -214,7 +213,7 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
 	ret = uvc_video_encode_data(video, buf, mem, len);
 	len -= ret;
 
-	req->length = video->req_size - len;
+	req->length = buf->req_payload_size - len;
 
 	if (buf->bytesused == video->queue.buf_used ||
 			video->queue.flags & UVC_QUEUE_DROP_INCOMPLETE) {

-- 
2.39.2

[PATCH v4 10/10] usb: gadget: uvc: add min g_ctrl vidioc and set min buffs to 4

[Michael Grzeschik]

We increase the minimum amount of v4l2 buffers that will be possibly
enqueued into the hardware and allocate at least
UVCG_STREAMING_MIN_BUFFERS amount of requests. This way the driver has
also more requests available to prefill the isoc hardware with.

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

---
v3 -> v4: -
v1 -> v3: new patch
---
 drivers/usb/gadget/function/uvc.h       |  2 ++
 drivers/usb/gadget/function/uvc_queue.c |  3 ++-
 drivers/usb/gadget/function/uvc_v4l2.c  | 13 +++++++++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index f6bc58fb02b84..e0b1f78fdbc65 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -71,6 +71,8 @@ extern unsigned int uvc_gadget_trace_param;
 
 #define UVCG_REQUEST_HEADER_LEN			12
 
+#define UVCG_STREAMING_MIN_BUFFERS		4
+
 /* ------------------------------------------------------------------------
  * Structures
  */
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index e33ce72325031..157e7f7d49c7a 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -21,6 +21,7 @@
 #include <media/videobuf2-vmalloc.h>
 
 #include "uvc.h"
+#include "uvc_video.h"
 
 /* ------------------------------------------------------------------------
  * Video buffers queue management.
@@ -86,7 +87,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 	}
 
 	video->req_size = req_size;
-	video->uvc_num_requests = nreq;
+	video->uvc_num_requests = nreq * UVCG_STREAMING_MIN_BUFFERS;
 
 	return 0;
 }
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 392fb400aad14..f96074f2c2824 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -357,6 +357,18 @@ static int uvc_v4l2_s_parm(struct file *file, void *fh,
 	return 0;
 }
 
+static int uvc_g_ctrl(struct file *file, void *priv, struct v4l2_control *vc)
+{
+	int ret = -EINVAL;
+
+	if (vc->id == V4L2_CID_MIN_BUFFERS_FOR_OUTPUT) {
+		vc->value = UVCG_STREAMING_MIN_BUFFERS;
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static int
 uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
 		struct v4l2_frmivalenum *fival)
@@ -629,6 +641,7 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
 	.vidioc_streamoff = uvc_v4l2_streamoff,
 	.vidioc_s_parm = uvc_v4l2_s_parm,
 	.vidioc_g_parm = uvc_v4l2_g_parm,
+	.vidioc_g_ctrl = uvc_g_ctrl,
 	.vidioc_subscribe_event = uvc_v4l2_subscribe_event,
 	.vidioc_unsubscribe_event = uvc_v4l2_unsubscribe_event,
 	.vidioc_default = uvc_v4l2_ioctl_default,

-- 
2.39.2

Re: [PATCH v4 10/10] usb: gadget: uvc: add min g_ctrl vidioc and set min buffs to 4

[Laurent Pinchart]

On Tue, Aug 13, 2024 at 11:09:34AM +0200, Michael Grzeschik wrote:
> We increase the minimum amount of v4l2 buffers that will be possibly
> enqueued into the hardware and allocate at least
> UVCG_STREAMING_MIN_BUFFERS amount of requests. This way the driver has
> also more requests available to prefill the isoc hardware with.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> v3 -> v4: -
> v1 -> v3: new patch
> ---
>  drivers/usb/gadget/function/uvc.h       |  2 ++
>  drivers/usb/gadget/function/uvc_queue.c |  3 ++-
>  drivers/usb/gadget/function/uvc_v4l2.c  | 13 +++++++++++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
> index f6bc58fb02b84..e0b1f78fdbc65 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -71,6 +71,8 @@ extern unsigned int uvc_gadget_trace_param;
>  
>  #define UVCG_REQUEST_HEADER_LEN			12
>  
> +#define UVCG_STREAMING_MIN_BUFFERS		4
> +

4 is a large number that drastically increases latency.

>  /* ------------------------------------------------------------------------
>   * Structures
>   */
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index e33ce72325031..157e7f7d49c7a 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -21,6 +21,7 @@
>  #include <media/videobuf2-vmalloc.h>
>  
>  #include "uvc.h"
> +#include "uvc_video.h"
>  
>  /* ------------------------------------------------------------------------
>   * Video buffers queue management.
> @@ -86,7 +87,7 @@ static int uvc_queue_setup(struct vb2_queue *vq,
>  	}
>  
>  	video->req_size = req_size;
> -	video->uvc_num_requests = nreq;
> +	video->uvc_num_requests = nreq * UVCG_STREAMING_MIN_BUFFERS;
>  
>  	return 0;
>  }
> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
> index 392fb400aad14..f96074f2c2824 100644
> --- a/drivers/usb/gadget/function/uvc_v4l2.c
> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
> @@ -357,6 +357,18 @@ static int uvc_v4l2_s_parm(struct file *file, void *fh,
>  	return 0;
>  }
>  
> +static int uvc_g_ctrl(struct file *file, void *priv, struct v4l2_control *vc)
> +{
> +	int ret = -EINVAL;
> +
> +	if (vc->id == V4L2_CID_MIN_BUFFERS_FOR_OUTPUT) {
> +		vc->value = UVCG_STREAMING_MIN_BUFFERS;
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static int
>  uvc_v4l2_enum_frameintervals(struct file *file, void *fh,
>  		struct v4l2_frmivalenum *fival)
> @@ -629,6 +641,7 @@ const struct v4l2_ioctl_ops uvc_v4l2_ioctl_ops = {
>  	.vidioc_streamoff = uvc_v4l2_streamoff,
>  	.vidioc_s_parm = uvc_v4l2_s_parm,
>  	.vidioc_g_parm = uvc_v4l2_g_parm,
> +	.vidioc_g_ctrl = uvc_g_ctrl,
>  	.vidioc_subscribe_event = uvc_v4l2_subscribe_event,
>  	.vidioc_unsubscribe_event = uvc_v4l2_unsubscribe_event,
>  	.vidioc_default = uvc_v4l2_ioctl_default,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers

[Greg Kroah-Hartman]

On Tue, Aug 13, 2024 at 11:09:24AM +0200, Michael Grzeschik wrote:
> 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.
> 
> It also precalculates the request length based on the actual per frame
> size for compressed formats.
> 
> For this calculations to work it was needed to rework the request
> queueing by moving the encoding to one extra thread (in this case we
> chose the qbuf) context.
> 
> Next it was needed to move the actual request enqueueing to one extra
> thread which is kept busy to fill the isoc queue in the udc.
> 
> As a final step the series is increasing the minimum amount of
> v4l2 buffers to 4 and allocates at least the amount of usb_requests
> to store them in the usb gadgte isoc pipeline.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> Changes in v4:
> - fixed exit path in uvc_enqueue_buffer on loop break
> - Link to v3: https://lore.kernel.org/r/20240403-uvc_request_length_by_interval-v3-0-4da7033dd488@pengutronix.de

I just took v3 in my tree, should I drop them?

thanks,

greg k-h

Re: [PATCH v4 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers

[Greg Kroah-Hartman]

On Tue, Aug 13, 2024 at 11:56:09AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 13, 2024 at 11:09:24AM +0200, Michael Grzeschik wrote:
> > 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.
> > 
> > It also precalculates the request length based on the actual per frame
> > size for compressed formats.
> > 
> > For this calculations to work it was needed to rework the request
> > queueing by moving the encoding to one extra thread (in this case we
> > chose the qbuf) context.
> > 
> > Next it was needed to move the actual request enqueueing to one extra
> > thread which is kept busy to fill the isoc queue in the udc.
> > 
> > As a final step the series is increasing the minimum amount of
> > v4l2 buffers to 4 and allocates at least the amount of usb_requests
> > to store them in the usb gadgte isoc pipeline.
> > 
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > ---
> > Changes in v4:
> > - fixed exit path in uvc_enqueue_buffer on loop break
> > - Link to v3: https://lore.kernel.org/r/20240403-uvc_request_length_by_interval-v3-0-4da7033dd488@pengutronix.de
> 
> I just took v3 in my tree, should I drop them?

Oops, emails crossed.  I'll go drop these now...

greg k-h