diff mbox series

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

Message ID 20240403-uvc_request_length_by_interval-v3-3-4da7033dd488@pengutronix.de
State Superseded
Headers show
Series usb: gadget: uvc: effectively fill the udc isoc pipeline with available video buffers | expand

Commit Message

Michael Grzeschik July 26, 2024, 10:02 p.m. UTC
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(-)

Comments

Michael Grzeschik Aug. 13, 2024, 9:10 a.m. UTC | #1
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
diff mbox series

Patch

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__ */