All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers
@ 2024-07-26 22:02 Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 01/10] usb: gadget: uvc: always set interrupt on zero length requests Michael Grzeschik
                   ` (9 more replies)
  0 siblings, 10 replies; 12+ messages in thread
From: Michael Grzeschik @ 2024-07-26 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel, 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 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 | 236 +++++++++++++-------------------
 drivers/usb/gadget/function/uvc_video.h |   1 +
 7 files changed, 215 insertions(+), 159 deletions(-)
---
base-commit: 1722389b0d863056d78287a120a1d6cadb8d4f7b
change-id: 20240403-uvc_request_length_by_interval-a7efd587d963

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 01/10] usb: gadget: uvc: always set interrupt on zero length requests
  2024-07-26 22:02 [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers Michael Grzeschik
@ 2024-07-26 22:02 ` Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 02/10] usb: gadget: uvc: only enqueue zero length requests in potential underrun Michael Grzeschik
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Michael Grzeschik @ 2024-07-26 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel, 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>

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 02/10] usb: gadget: uvc: only enqueue zero length requests in potential underrun
  2024-07-26 22:02 [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 01/10] usb: gadget: uvc: always set interrupt on zero length requests Michael Grzeschik
@ 2024-07-26 22:02 ` Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 03/10] usb: gadget: uvc: remove pump worker and enqueue all buffers per frame in qbuf Michael Grzeschik
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Michael Grzeschik @ 2024-07-26 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel, 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>

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 03/10] usb: gadget: uvc: remove pump worker and enqueue all buffers per frame in qbuf
  2024-07-26 22:02 [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 01/10] usb: gadget: uvc: always set interrupt on zero length requests Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 02/10] usb: gadget: uvc: only enqueue zero length requests in potential underrun Michael Grzeschik
@ 2024-07-26 22:02 ` Michael Grzeschik
  2024-08-13  9:10   ` Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 04/10] usb: gadget: uvc: rework to enqueue in pump worker from encoded queue Michael Grzeschik
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 12+ messages in thread
From: Michael Grzeschik @ 2024-07-26 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel, 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>

---
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 | 46 +++++----------------------------
 drivers/usb/gadget/function/uvc_video.h |  1 +
 6 files changed, 12 insertions(+), 50 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..0bd49d4e106b1 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
@@ -642,6 +615,8 @@ static void uvcg_video_pump(struct work_struct *work)
 	else
 		uvc_video_free_request(req->context, video->ep);
 	spin_unlock_irqrestore(&video->req_lock, flags);
+
+	return 0;
 }
 
 /*
@@ -681,7 +656,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 +749,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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 04/10] usb: gadget: uvc: rework to enqueue in pump worker from encoded queue
  2024-07-26 22:02 [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers Michael Grzeschik
                   ` (2 preceding siblings ...)
  2024-07-26 22:02 ` [PATCH v3 03/10] usb: gadget: uvc: remove pump worker and enqueue all buffers per frame in qbuf Michael Grzeschik
@ 2024-07-26 22:02 ` Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 05/10] usb: gadget: uvc: remove uvc_video_ep_queue_initial_requests Michael Grzeschik
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Michael Grzeschik @ 2024-07-26 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel, 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>

---
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 0bd49d4e106b1..a0448f8e8f334 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
@@ -750,6 +771,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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 05/10] usb: gadget: uvc: remove uvc_video_ep_queue_initial_requests
  2024-07-26 22:02 [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers Michael Grzeschik
                   ` (3 preceding siblings ...)
  2024-07-26 22:02 ` [PATCH v3 04/10] usb: gadget: uvc: rework to enqueue in pump worker from encoded queue Michael Grzeschik
@ 2024-07-26 22:02 ` Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 06/10] usb: gadget: uvc: set req_size once when the vb2 queue is calculated Michael Grzeschik
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Michael Grzeschik @ 2024-07-26 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel, 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>

---
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 a0448f8e8f334..463777b5db6ff 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)
 {
@@ -755,8 +711,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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 06/10] usb: gadget: uvc: set req_size once when the vb2 queue is calculated
  2024-07-26 22:02 [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers Michael Grzeschik
                   ` (4 preceding siblings ...)
  2024-07-26 22:02 ` [PATCH v3 05/10] usb: gadget: uvc: remove uvc_video_ep_queue_initial_requests Michael Grzeschik
@ 2024-07-26 22:02 ` Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 07/10] usb: gadget: uvc: add g_parm and s_parm for frame interval Michael Grzeschik
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Michael Grzeschik @ 2024-07-26 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel, 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>

---
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 463777b5db6ff..9d3cfa96b1350 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:
@@ -658,7 +648,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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 07/10] usb: gadget: uvc: add g_parm and s_parm for frame interval
  2024-07-26 22:02 [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers Michael Grzeschik
                   ` (5 preceding siblings ...)
  2024-07-26 22:02 ` [PATCH v3 06/10] usb: gadget: uvc: set req_size once when the vb2 queue is calculated Michael Grzeschik
@ 2024-07-26 22:02 ` Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 08/10] usb: gadget: uvc: set req_size and n_requests based on the " Michael Grzeschik
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 12+ messages in thread
From: Michael Grzeschik @ 2024-07-26 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel, 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>

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 08/10] usb: gadget: uvc: set req_size and n_requests based on the frame interval
  2024-07-26 22:02 [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers Michael Grzeschik
                   ` (6 preceding siblings ...)
  2024-07-26 22:02 ` [PATCH v3 07/10] usb: gadget: uvc: add g_parm and s_parm for frame interval Michael Grzeschik
@ 2024-07-26 22:02 ` Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 09/10] usb: gadget: uvc: set req_length based on payload by nreqs instead of req_size Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 10/10] usb: gadget: uvc: add min g_ctrl vidioc and set min buffs to 4 Michael Grzeschik
  9 siblings, 0 replies; 12+ messages in thread
From: Michael Grzeschik @ 2024-07-26 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel, 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>

---
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 9d3cfa96b1350..fd2195f7153d9 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 09/10] usb: gadget: uvc: set req_length based on payload by nreqs instead of req_size
  2024-07-26 22:02 [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers Michael Grzeschik
                   ` (7 preceding siblings ...)
  2024-07-26 22:02 ` [PATCH v3 08/10] usb: gadget: uvc: set req_size and n_requests based on the " Michael Grzeschik
@ 2024-07-26 22:02 ` Michael Grzeschik
  2024-07-26 22:02 ` [PATCH v3 10/10] usb: gadget: uvc: add min g_ctrl vidioc and set min buffs to 4 Michael Grzeschik
  9 siblings, 0 replies; 12+ messages in thread
From: Michael Grzeschik @ 2024-07-26 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel, 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>

---
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 fd2195f7153d9..f6911f124be4b 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 10/10] usb: gadget: uvc: add min g_ctrl vidioc and set min buffs to 4
  2024-07-26 22:02 [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers Michael Grzeschik
                   ` (8 preceding siblings ...)
  2024-07-26 22:02 ` [PATCH v3 09/10] usb: gadget: uvc: set req_length based on payload by nreqs instead of req_size Michael Grzeschik
@ 2024-07-26 22:02 ` Michael Grzeschik
  9 siblings, 0 replies; 12+ messages in thread
From: Michael Grzeschik @ 2024-07-26 22:02 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel, 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>

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 03/10] usb: gadget: uvc: remove pump worker and enqueue all buffers per frame in qbuf
  2024-07-26 22:02 ` [PATCH v3 03/10] usb: gadget: uvc: remove pump worker and enqueue all buffers per frame in qbuf Michael Grzeschik
@ 2024-08-13  9:10   ` Michael Grzeschik
  0 siblings, 0 replies; 12+ messages in thread
From: Michael Grzeschik @ 2024-08-13  9:10 UTC (permalink / raw)
  To: Laurent Pinchart, Daniel Scally, Greg Kroah-Hartman,
	Avichal Rakesh
  Cc: linux-usb, linux-kernel

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

Hi Greg,

On Sat, Jul 27, 2024 at 12:02:38AM +0200, Michael Grzeschik wrote:
>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>
>
>---
>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 | 46 +++++----------------------------
> drivers/usb/gadget/function/uvc_video.h |  1 +
> 6 files changed, 12 insertions(+), 50 deletions(-)
>

This patch is introducing a bug that I have queued in the next series.

I just saw that you picked this series. Could you please drop it and
pick v4 instead? I will just send it out now.

https://lore.kernel.org/r/20240403-uvc_request_length_by_interval-v4-0-ca22f334226e@pengutronix.de

Besid the mentioned fix, the series is identical.

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2024-08-13  9:10 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-26 22:02 [PATCH v3 00/10] usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers Michael Grzeschik
2024-07-26 22:02 ` [PATCH v3 01/10] usb: gadget: uvc: always set interrupt on zero length requests Michael Grzeschik
2024-07-26 22:02 ` [PATCH v3 02/10] usb: gadget: uvc: only enqueue zero length requests in potential underrun Michael Grzeschik
2024-07-26 22:02 ` [PATCH v3 03/10] usb: gadget: uvc: remove pump worker and enqueue all buffers per frame in qbuf Michael Grzeschik
2024-08-13  9:10   ` Michael Grzeschik
2024-07-26 22:02 ` [PATCH v3 04/10] usb: gadget: uvc: rework to enqueue in pump worker from encoded queue Michael Grzeschik
2024-07-26 22:02 ` [PATCH v3 05/10] usb: gadget: uvc: remove uvc_video_ep_queue_initial_requests Michael Grzeschik
2024-07-26 22:02 ` [PATCH v3 06/10] usb: gadget: uvc: set req_size once when the vb2 queue is calculated Michael Grzeschik
2024-07-26 22:02 ` [PATCH v3 07/10] usb: gadget: uvc: add g_parm and s_parm for frame interval Michael Grzeschik
2024-07-26 22:02 ` [PATCH v3 08/10] usb: gadget: uvc: set req_size and n_requests based on the " Michael Grzeschik
2024-07-26 22:02 ` [PATCH v3 09/10] usb: gadget: uvc: set req_length based on payload by nreqs instead of req_size Michael Grzeschik
2024-07-26 22:02 ` [PATCH v3 10/10] usb: gadget: uvc: add min g_ctrl vidioc and set min buffs to 4 Michael Grzeschik

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.