UVC Gadget Driver shows glitched frames with a Linux host

Hey all,

First off, I am very new to the kernel space, so apologies for any
newb mistakes. Please feel free to point them out!

I've been trying to get the UVC gadget driver to work on an Android
device and have been seeing partial/glitched frames when streaming to
a Linux (or Windows) host fairly frequently (once every 2-4s). The UVC
gadget driver shows no error/exception logs, and neither does the
host's UVC driver.

Enabling tracing for the UVC gadget driver and the host's UVC driver
shows that the gadget sees a missed ISOC transfer, and the host sees a
frame with suspiciously low packet count at the same time as the
glitched frame.

```
[691171.704583] usb 1-4: frame 9274 stats: 0/8/8 packets, 0/0/0 pts
(!early !initial), 7/8 scr, last pts/stc/sof 0/259279856/14353
[691171.704602] usb 1-4: Frame complete (EOF found)
[691171.732584] usb 1-4: frame 9275 stats: 0/4/4 packets, 0/0/0 pts
(!early !initial), 3/4 scr, last pts/stc/sof 0/261131744/14661
[691171.732621] usb 1-4: Frame complete (EOF found)
[691171.768578] usb 1-4: frame 9276 stats: 0/8/8 packets, 0/0/0 pts
(!early !initial), 7/8 scr, last pts/stc/sof 0/262525136/14894
[691171.768616] usb 1-4: Frame complete (EOF found)
```

For reference, I am streaming 640x480 MJPEG frames of a (mostly)
static scene, and every frame takes 7-8 packets (~22kB). So 4 packets
for a frame is definitely not normal. From the logs, it looks like the
host sees an EOF header for the video frame that had the ISOC failure
and tries to display the frame with a partial buffer resulting in the
glitched frame.

This can happen because the UVC gadget driver waits for the encode
loop to drop the video frame
(https://lore.kernel.org/all/20221018215044.765044-4-w36195@motorola.com/).
However, because video frames are encoded (to usb_requests)
asynchronously, it is possible that the video frame is completely
encoded before the ISOC transfer failure is reported. In this case,
the next frame would be dropped, but the frame with missed ISOC will
remain in queue.

This problem may be further exaggerated by the DWC3 controller driver
(which is what my device has) not setting the IMI flag when
no_interrupt flag is set
(https://lore.kernel.org/all/ced336c84434571340c07994e3667a0ee284fefe.1666735451.git.Thinh.Nguyen@synopsys.com/)?
UVC Gadget Driver sets the no_interrupt flag for ~3/4 of its queued
usb_request, so an ISOC failure may not immediately interrupt the UVC
gadget driver, leaving more time for the frame to finish encoding.

I couldn't find any concrete error handling rules in the UVC specs, so
I am not sure what the proper solution here is. To try out, I created
a patch (attached below) that dequeues all queued usb_requests from
the endpoint in case of an ISOC failure and clears the uvc buffer
queue. This eliminated the partial frames with no perceivable frame
drops.

So my questions here are:
1. Is this a known issue, and if so are there workarounds for it?
2. If the answer to above is "No", does the explanation and mitigation
seem reasonable?

Patch follows (mostly for illustration, I can formalize it if
needed!). It adds a new 'req_inflight' list to track queued
usb_requests that have not been given back to the gadget driver and
drops all the queued requests in case of an ISOC failure. The other
changes are for the extra bookkeeping required to handle dropping all
frames. I haven't been able to confirm it, but as far as I can tell
the issue exists at ToT as well.

---
drivers/usb/gadget/function/uvc.h | 6 ++-
drivers/usb/gadget/function/uvc_queue.c | 16 ++++--
drivers/usb/gadget/function/uvc_queue.h | 2 +-
drivers/usb/gadget/function/uvc_video.c | 71 +++++++++++++++++++------
4 files changed, 72 insertions(+), 23 deletions(-)

}
@@ -181,15 +180,15 @@ uvc_video_encode_isoc_sg(struct usb_request
*req, struct uvc_video *video,

req->length -= len;
video->queue.buf_used += req->length - header_len;
+ ureq->uvc_buf = buf;

- if (buf->bytesused == video->queue.buf_used || !buf->sg ||
- video->queue.flags & UVC_QUEUE_DROP_INCOMPLETE) {
+ if (buf->bytesused == video->queue.buf_used || !buf->sg) {
video->queue.buf_used = 0;
buf->state = UVC_BUF_STATE_DONE;
buf->offset = 0;
list_del(&buf->queue);
video->fid ^= UVC_STREAM_FID;
- ureq->last_buf = buf;
+ ureq->is_last = true;
}
}

@@ -212,14 +211,14 @@ uvc_video_encode_isoc(struct usb_request *req,
struct uvc_video *video,
len -= ret;

req->length = video->req_size - len;
+ ureq->uvc_buf = buf;

- if (buf->bytesused == video->queue.buf_used ||
- video->queue.flags & UVC_QUEUE_DROP_INCOMPLETE) {
+ if (buf->bytesused == video->queue.buf_used) {
video->queue.buf_used = 0;
buf->state = UVC_BUF_STATE_DONE;
list_del(&buf->queue);
video->fid ^= UVC_STREAM_FID;
- ureq->last_buf = buf;
+ ureq->is_last = true;
}
}

@@ -231,11 +230,13 @@ static int uvcg_video_ep_queue(struct uvc_video
*video, struct usb_request *req)
{
int ret;

+ list_add_tail(&req->list, &video->req_inflight);
ret = usb_ep_queue(video->ep, req, GFP_ATOMIC);
if (ret < 0) {
uvcg_err(&video->uvc->func, "Failed to queue request (%d).\n",
ret);

+ list_del(&req->list);
/* If the endpoint is disabled the descriptor may be NULL. */
if (video->ep->desc) {
/* Isochronous endpoints can't be halted. */
@@ -254,15 +255,46 @@ 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_device *uvc = video->uvc;
+ struct uvc_buffer *uvc_buf = ureq->uvc_buf;
+ struct usb_request *itr;
unsigned long flags;

+ spin_lock_irqsave(&queue->irqlock, flags);
+ list_del(&req->list);
+ spin_unlock_irqrestore(&queue->irqlock, flags);
+
switch (req->status) {
case 0:
break;

case -EXDEV:
uvcg_dbg(&video->uvc->func, "VS request missed xfer.\n");
- queue->flags |= UVC_QUEUE_DROP_INCOMPLETE;
+ spin_lock_irqsave(&queue->irqlock, flags);
+ list_for_each_entry(itr, &video->req_inflight, list) {
+ usb_ep_dequeue(ep, itr);
+ }
+
+ if (uvc_buf->state != UVC_BUF_STATE_DONE) {
+ video->fid = UVC_STREAM_FID;
+ } else {
+ queue->flags |= UVC_QUEUE_DROP_INCOMPLETE;
+ }
+
+ /*
+ * We cancel the entire queue because calling usb_ep_dequeue on any
pending request will
+ * cancel all queued requests (on some USB controllers). ISOC
transfers are "best effort"
+ * and a missed frame or two won't really be noticeable. Rather than
adding extra
+ * bookkeeping to try and find the precise set of frames that need to
be dropped, we drop
+ * the entire queue.
+ */
+ uvcg_queue_cancel_locked(queue, 0);
+ video->req_int_count = 0;
+ spin_unlock_irqrestore(&queue->irqlock, flags);
+ break;
+
+ case -ECONNRESET:
+ uvcg_dbg(&video->uvc->func, "VS request dqed.\n");
+ // Request dequeued using usb_ep_dequeue, no special handling required
break;

case -ESHUTDOWN: /* disconnect from host. */
@@ -277,10 +309,11 @@ uvc_video_complete(struct usb_ep *ep, struct
usb_request *req)
uvcg_queue_cancel(queue, 0);
}

- if (ureq->last_buf) {
- uvcg_complete_buffer(&video->queue, ureq->last_buf);
- ureq->last_buf = NULL;
+ if (ureq->is_last) {
+ uvcg_complete_buffer(&video->queue, ureq->uvc_buf);
}
+ ureq->is_last = false;
+ ureq->uvc_buf = NULL;

spin_lock_irqsave(&video->req_lock, flags);
list_add_tail(&req->list, &video->req_free);
@@ -315,6 +348,7 @@ uvc_video_free_requests(struct uvc_video *video)
}

INIT_LIST_HEAD(&video->req_free);
+ INIT_LIST_HEAD(&video->req_inflight);
video->req_size = 0;
return 0;
}
@@ -350,7 +384,8 @@ uvc_video_alloc_requests(struct uvc_video *video)
video->ureq[i].req->complete = uvc_video_complete;
video->ureq[i].req->context = &video->ureq[i];
video->ureq[i].video = video;
- video->ureq[i].last_buf = NULL;
+ video->ureq[i].uvc_buf = NULL;
+ video->ureq[i].is_last = false;

list_add_tail(&video->ureq[i].req->list, &video->req_free);
/* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */
@@ -385,6 +420,7 @@ static void uvcg_video_pump(struct work_struct *work)
struct usb_request *req = NULL;
struct uvc_buffer *buf;
unsigned long flags;
+ bool must_interrupt = false;
int ret;

while (video->ep->enabled) {
@@ -400,6 +436,7 @@ static void uvcg_video_pump(struct work_struct *work)
req = list_first_entry(&video->req_free, struct usb_request,
list);
list_del(&req->list);
+ must_interrupt = list_empty(&video->req_free);
spin_unlock_irqrestore(&video->req_lock, flags);

/*
@@ -420,14 +457,14 @@ static void uvcg_video_pump(struct work_struct *work)
* interrupt load to a quarter but also catches the corner
* cases, which needs to be handled.
*/
- if (list_empty(&video->req_free) ||
+ if (must_interrupt ||
buf->state == UVC_BUF_STATE_DONE ||
!(video->req_int_count %
DIV_ROUND_UP(video->uvc_num_requests, 4))) {
video->req_int_count = 0;
req->no_interrupt = 0;
} else {
- req->no_interrupt = 1;
+ req->no_interrupt = 0;
}

/* Queue the USB request */
@@ -507,6 +544,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
{
INIT_LIST_HEAD(&video->req_free);
+ INIT_LIST_HEAD(&video->req_inflight);
spin_lock_init(&video->req_lock);
INIT_WORK(&video->pump, uvcg_video_pump);

@@ -527,4 +565,3 @@ int uvcg_video_init(struct uvc_video *video,
struct uvc_device *uvc)
V4L2_BUF_TYPE_VIDEO_OUTPUT, &video->mutex);
return 0;
}
-
--


Message ID	CAMHf4WKbi6KBPQztj9FA4kPvESc1fVKrC8G73-cs6tTeQby9=w@mail.gmail.com
State	New
Headers	show Return-Path: <linux-usb-owner@vger.kernel.org> MIME-Version: 1.0 From: Avichal Rakesh <arakesh@google.com> Date: Fri, 14 Apr 2023 14:03:02 -0700 Message-ID: <CAMHf4WKbi6KBPQztj9FA4kPvESc1fVKrC8G73-cs6tTeQby9=w@mail.gmail.com> Subject: UVC Gadget Driver shows glitched frames with a Linux host To: laurent.pinchart@ideasonboard.com, dan.scally@ideasonboard.com, gregkh@linuxfoundation.org, Thinh.Nguyen@synopsys.com, linux-usb@vger.kernel.org Cc: Jayant Chowdhary <jchowdhary@google.com>, "Eino-Ville Talvala (Eddy)" <etalvala@google.com> Content-Type: text/plain; charset="UTF-8" Precedence: bulk
Series	UVC Gadget Driver shows glitched frames with a Linux host \| expand UVC Gadget Driver shows glitched frames with a Linux host

UVC Gadget Driver shows glitched frames with a Linux host

Commit Message

Comments

Patch