Received-SPF: pass (google.com: domain of linux-kernel+bounces-132580-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Michael Grzeschik <m.grzeschik@pengutronix.de>
Date: Fri, 05 Apr 2024 09:44:56 +0200
Subject: [PATCH v2] usb: gadget: uvc: Improve error checking and tagging
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Message-Id: <20240324-uvc-gadget-errorcheck-v2-1-f141b13ade0e@pengutronix.de>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, 
 Daniel Scally <dan.scally@ideasonboard.com>, 
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, 
 Michael Grzeschik <m.grzeschik@pengutronix.de>

Right now after one transfer was completed with EXDEV the currently
encoded frame will get the UVC_STREAM_ERR tag attached. Since the
complete and encode path are handling separate requests from different
threads, there is no direct correspondence between the missed transfer
of one request and the currently encoded request which might already
belong to an completely different frame.

When queueing requests into the hardware by calling ep_queue the
underlying ringbuffer of the usb driver will be filled. However when
one of these requests will have some issue while transfer the hardware
will trigger an interrupt but will continue transferring the pending
requests in the ringbuffer. This interrupt-latency will make it
impossible to react in time to tag the fully enqueued frame with the
UVC_STREAM_ERR in the header.

This patch is also addressing this particular issue by delaying the
transmit of the EOF/ERR tagged header by waiting for the last enqueued
buffer of the frame to be completed. This way it is possible to react to
send the EOF/ERR tag depending on the whole frame transfer status.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
Changes in v2:
- removed unnecessary uvc_gadget_errorcheck_param module parameter
- Link to v1: https://lore.kernel.org/r/20240324-uvc-gadget-errorcheck-v1-1-5538c57bbeba@pengutronix.de
---
 drivers/usb/gadget/function/uvc.h       |  2 +
 drivers/usb/gadget/function/uvc_video.c | 69 ++++++++++++++++++++++++++++-----
 2 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index cb35687b11e7e..210c9b5e6a5ee 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -91,6 +91,8 @@ struct uvc_video {
 	struct work_struct pump;
 	struct workqueue_struct *async_wq;
 
+	struct usb_request *last_req;
+
 	/* Frame parameters */
 	u8 bpp;
 	u32 fcc;
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index d41f5f31dadd5..2bfeea94131a9 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -35,9 +35,6 @@ uvc_video_encode_header(struct uvc_video *video, struct uvc_buffer *buf,
 
 	data[1] = UVC_STREAM_EOH | video->fid;
 
-	if (video->queue.flags & UVC_QUEUE_DROP_INCOMPLETE)
-		data[1] |= UVC_STREAM_ERR;
-
 	if (video->queue.buf_used == 0 && ts.tv_sec) {
 		/* dwClockFrequency is 48 MHz */
 		u32 pts = ((u64)ts.tv_sec * USEC_PER_SEC + ts.tv_nsec / NSEC_PER_USEC) * 48;
@@ -62,9 +59,6 @@ uvc_video_encode_header(struct uvc_video *video, struct uvc_buffer *buf,
 
 	data[0] = pos;
 
-	if (buf->bytesused - video->queue.buf_used <= len - pos)
-		data[1] |= UVC_STREAM_EOF;
-
 	return pos;
 }
 
@@ -366,6 +360,32 @@ static void uvc_video_ep_queue_initial_requests(struct uvc_video *video)
 	spin_unlock_irqrestore(&video->req_lock, flags);
 }
 
+static void
+uvc_video_fixup_header(struct usb_request *next, struct usb_request *done,
+		       bool error)
+{
+	struct uvc_request *next_ureq = next->context;
+	struct uvc_request *done_ureq = done->context;
+	struct uvc_video *video = next_ureq->video;
+	struct uvc_video_queue *queue = &video->queue;
+
+	u8 header = UVC_STREAM_EOF;
+
+	if (error)
+		header |= UVC_STREAM_ERR;
+
+	if (queue->use_sg) {
+		memcpy(next_ureq->header, done_ureq->header,
+				UVCG_REQUEST_HEADER_LEN);
+
+		((u8 *)next_ureq->header)[1] |= header;
+	} else {
+		memcpy(next->buf, done->buf, UVCG_REQUEST_HEADER_LEN);
+
+		((u8 *)next->buf)[1] |= header;
+	}
+}
+
 static void
 uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 {
@@ -377,6 +397,7 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 	unsigned long flags;
 	bool is_bulk = video->max_payload_size;
 	int ret = 0;
+	bool error = false;
 
 	spin_lock_irqsave(&video->req_lock, flags);
 	if (!video->is_enabled) {
@@ -419,6 +440,8 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 
 	if (last_buf) {
 		spin_lock_irqsave(&queue->irqlock, flags);
+		if (queue->flags & UVC_QUEUE_DROP_INCOMPLETE)
+			error = true;
 		uvcg_complete_buffer(queue, last_buf);
 		spin_unlock_irqrestore(&queue->irqlock, flags);
 	}
@@ -449,11 +472,37 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 	 * requests and cannot recover.
 	 */
 	to_queue->length = 0;
+
+	/* If the last request was queued just copy the previous
+	 * header without payload to the empty request and fixup the
+	 * tags with ERR/EOF to finish the frame.
+	 */
+	if (video->last_req == req) {
+		uvc_video_fixup_header(to_queue, req, error);
+		to_queue->length = UVCG_REQUEST_HEADER_LEN;
+		video->last_req = NULL;
+	}
+
 	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);
+		struct usb_request *next_req =
+				list_first_entry(&video->req_ready,
+						 struct usb_request, list);
+		struct uvc_request *next_ureq = next_req->context;
+
+		/* If the last request of the frame will be queued, we delay
+		 * the enqueueing of every next request. This way it is
+		 * possible to react to send the EOF/ERR tag depending
+		 * on the whole frame transfer status.
+		 */
+		if (!video->last_req && !to_queue->length) {
+			if (next_ureq->last_buf)
+				video->last_req = next_req;
+
+			to_queue = next_req;
+			list_del(&to_queue->list);
+			list_add_tail(&req->list, &video->req_free);
+		}
+
 		/*
 		 * Queue work to the wq as well since it is possible that a
 		 * buffer may not have been completely encoded with the set of

---
base-commit: 3295f1b866bfbcabd625511968e8a5c541f9ab32
change-id: 20240324-uvc-gadget-errorcheck-41b817aff44d

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