diff mbox series

usb: gadget: uvc: Improve error checking and tagging

Message ID 20240324-uvc-gadget-errorcheck-v1-1-5538c57bbeba@pengutronix.de
State New
Headers show
Series usb: gadget: uvc: Improve error checking and tagging | expand

Commit Message

Michael Grzeschik March 24, 2024, 11:32 p.m. UTC 
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.

As this is patch is adding latency to the enqueuing path of the frames
we make this errorcheck optional by adding an extra module parameter.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/f_uvc.c     |  4 ++
 drivers/usb/gadget/function/uvc.h       |  3 ++
 drivers/usb/gadget/function/uvc_video.c | 69 +++++++++++++++++++++++++++++----
 3 files changed, 68 insertions(+), 8 deletions(-)


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

Best regards,

Comments

Greg KH March 26, 2024, 9:32 a.m. UTC | #1 
On Mon, Mar 25, 2024 at 12:32:30AM +0100, Michael Grzeschik wrote:
> 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.
> 
> As this is patch is adding latency to the enqueuing path of the frames
> we make this errorcheck optional by adding an extra module parameter.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/f_uvc.c     |  4 ++
>  drivers/usb/gadget/function/uvc.h       |  3 ++
>  drivers/usb/gadget/function/uvc_video.c | 69 +++++++++++++++++++++++++++++----
>  3 files changed, 68 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> index 929666805bd23..6a7ca8ccaf360 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -33,6 +33,10 @@ unsigned int uvc_gadget_trace_param;
>  module_param_named(trace, uvc_gadget_trace_param, uint, 0644);
>  MODULE_PARM_DESC(trace, "Trace level bitmask");
>  
> +bool uvc_gadget_errorcheck_param = true;
> +module_param_named(errorcheck, uvc_gadget_errorcheck_param, bool, 0644);
> +MODULE_PARM_DESC(errorcheck, "Check and mark errors in the transfer of a frame");

I really really really really hate adding new module parameters as they
do not scale nor work properly for multiple devices and really, it's not
the 1990's anymore.

Any way to make this debugging thing part of a debugfs interface or
worst case, a sysfs entry instead?

Or why not just make it a tracing thing instead?

But wait, you are fixing a real issue here, why is it an option at all?
Shouldn't this always be the case and the driver should always recover
in this way?  Why would you not want this to be the default and only way
it operates?

thanks,

greg k-h
Michael Grzeschik April 2, 2024, 7:45 a.m. UTC | #2 
On Tue, Mar 26, 2024 at 10:32:48AM +0100, Greg Kroah-Hartman wrote:
>On Mon, Mar 25, 2024 at 12:32:30AM +0100, Michael Grzeschik wrote:
>> 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.
>>
>> As this is patch is adding latency to the enqueuing path of the frames
>> we make this errorcheck optional by adding an extra module parameter.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>>  drivers/usb/gadget/function/f_uvc.c     |  4 ++
>>  drivers/usb/gadget/function/uvc.h       |  3 ++
>>  drivers/usb/gadget/function/uvc_video.c | 69 +++++++++++++++++++++++++++++----
>>  3 files changed, 68 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
>> index 929666805bd23..6a7ca8ccaf360 100644
>> --- a/drivers/usb/gadget/function/f_uvc.c
>> +++ b/drivers/usb/gadget/function/f_uvc.c
>> @@ -33,6 +33,10 @@ unsigned int uvc_gadget_trace_param;
>>  module_param_named(trace, uvc_gadget_trace_param, uint, 0644);
>>  MODULE_PARM_DESC(trace, "Trace level bitmask");
>>
>> +bool uvc_gadget_errorcheck_param = true;
>> +module_param_named(errorcheck, uvc_gadget_errorcheck_param, bool, 0644);
>> +MODULE_PARM_DESC(errorcheck, "Check and mark errors in the transfer of a frame");
>
>I really really really really hate adding new module parameters as they
>do not scale nor work properly for multiple devices and really, it's not
>the 1990's anymore.
>
>Any way to make this debugging thing part of a debugfs interface or
>worst case, a sysfs entry instead?
>
>Or why not just make it a tracing thing instead?
>
>But wait, you are fixing a real issue here, why is it an option at all?
>Shouldn't this always be the case and the driver should always recover
>in this way?  Why would you not want this to be the default and only way
>it operates?

Yes and no. I added the thange to be optional, since it is changing
the amount of frames we will be able to transfer at once. However
I have another set of changes in the queue, that is also taking
the expected size and fps into the account of requst_size and
therefor the real amount of bandwidth. That will indirectly will have
the same result.

So this makes the option obsolete anyway. I will remove it in v2.

Thanks,
Michael
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 929666805bd23..6a7ca8ccaf360 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -33,6 +33,10 @@  unsigned int uvc_gadget_trace_param;
 module_param_named(trace, uvc_gadget_trace_param, uint, 0644);
 MODULE_PARM_DESC(trace, "Trace level bitmask");
 
+bool uvc_gadget_errorcheck_param = true;
+module_param_named(errorcheck, uvc_gadget_errorcheck_param, bool, 0644);
+MODULE_PARM_DESC(errorcheck, "Check and mark errors in the transfer of a frame");
+
 /* --------------------------------------------------------------------------
  * Function descriptors
  */
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index cb35687b11e7e..e21fea6784597 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -46,6 +46,7 @@  struct uvc_device;
 #define UVC_WARN_PROBE_DEF			1
 
 extern unsigned int uvc_gadget_trace_param;
+extern bool uvc_gadget_errorcheck_param;
 
 #define uvc_trace(flag, msg...) \
 	do { \
@@ -91,6 +92,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..8e9ec21b5154b 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,7 +59,8 @@  uvc_video_encode_header(struct uvc_video *video, struct uvc_buffer *buf,
 
 	data[0] = pos;
 
-	if (buf->bytesused - video->queue.buf_used <= len - pos)
+	if (!uvc_gadget_errorcheck_param &&
+	    (buf->bytesused - video->queue.buf_used <= len - pos))
 		data[1] |= UVC_STREAM_EOF;
 
 	return pos;
@@ -366,6 +364,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 +401,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 +444,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 +476,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 the just copy the previous
+	 * header without payload to the empty request and fixup the
+	 * tags with ERR/EOF to finish the frame.
+	 */
+	if (uvc_gadget_errorcheck_param && (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 (uvc_gadget_errorcheck_param && 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