[PATCH v3] usb: gadget: uvc: clean up comments and styling in video_pump

From: Avichal Rakesh <arakesh@google.com>
To: dan.scally@ideasonboard.com, laurent.pinchart@ideasonboard.com
Cc: thinh.nguyen@synopsys.com, arakesh@google.com,
	etalvala@google.com, gregkh@linuxfoundation.org,
	jchowdhary@google.com, linux-kernel@vger.kernel.org,
	linux-usb@vger.kernel.org
Subject: [PATCH v3] usb: gadget: uvc: clean up comments and styling in video_pump
Date: Fri,  2 Jun 2023 15:04:55 -0700	[thread overview]
Message-ID: <20230602220455.313801-1-arakesh@google.com> (raw)
In-Reply-To: <20230602211602.3b7rfa252wliiszp@synopsys.com>

This patch elaborates on some of the edge cases handled by
video_pump around setting no_interrupt flag, and brings the
code style in line with rest of the file.

Link: https://lore.kernel.org/20230602151916.GH26944@pendragon.ideasonboard.com/
Signed-off-by: Avichal Rakesh <arakesh@google.com>
---
Changelog:
v2:
  - Updated commit message to make it clear that userspace application is not
    required to match the ISOC rate.
  - Styling and comment revision based on review
v3:
  - Rebased on to Greg's usb-next where v1 had already merged
  - Updated commit message to match the actual changes after rebase.


 drivers/usb/gadget/function/uvc_video.c | 38 ++++++++++++++++---------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index e81865978299..91af3b1ef0d4 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -382,13 +382,13 @@ static void uvcg_video_pump(struct work_struct *work)
 {
 	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;
 	unsigned long flags;
+	bool buf_done;
 	int ret;
-	bool buf_int;
-	/* video->max_payload_size is only set when using bulk transfer */
-	bool is_bulk = video->max_payload_size;

 	while (video->ep->enabled) {
 		/*
@@ -414,20 +414,19 @@ static void uvcg_video_pump(struct work_struct *work)

 		if (buf != NULL) {
 			video->encode(req, video, buf);
-			/* Always interrupt for the last request of a video buffer */
-			buf_int = buf->state == UVC_BUF_STATE_DONE;
+			buf_done = buf->state == UVC_BUF_STATE_DONE;
 		} else if (!(queue->flags & UVC_QUEUE_DISCONNECTED) && !is_bulk) {
 			/*
 			 * No video buffer available; the queue is still connected and
-			 * we're traferring over ISOC. Queue a 0 length request to
+			 * we're transferring over ISOC. Queue a 0 length request to
 			 * prevent missed ISOC transfers.
 			 */
 			req->length = 0;
-			buf_int = false;
+			buf_done = false;
 		} else {
 			/*
-			 * Either queue has been disconnected or no video buffer
-			 * available to bulk transfer. Either way, stop processing
+			 * 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);
@@ -435,11 +434,24 @@ static void uvcg_video_pump(struct work_struct *work)
 		}

 		/*
-		 * With usb3 we have more requests. This will decrease the
-		 * interrupt load to a quarter but also catches the corner
-		 * cases, which needs to be handled.
+		 * With USB3 handling more requests at a higher speed, we can't
+		 * afford to generate an interrupt for every request. Decide to
+		 * interrupt:
+		 *
+		 * - When no more requests are available in the free queue, as
+		 *   this may be our last chance to refill the endpoint's
+		 *   request queue.
+		 *
+		 * - When this is request is the last request for the video
+		 *   buffer, as we want to start sending the next video buffer
+		 *   ASAP in case it doesn't get started already in the next
+		 *   iteration of this loop.
+		 *
+		 * - Four times over the length of the requests queue (as
+		 *   indicated by video->uvc_num_requests), as a trade-off
+		 *   between latency and interrupt load.
 		 */
-		if (list_empty(&video->req_free) || buf_int ||
+		if (list_empty(&video->req_free) || buf_done ||
 		    !(video->req_int_count %
 		       DIV_ROUND_UP(video->uvc_num_requests, 4))) {
 			video->req_int_count = 0;
--
2.41.0.rc0.172.g3f132b7071-goog

