Re: UVC Gadget Driver shows glitched frames with a Linux host

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Apr 14, 2023 at 02:03:02PM -0700, Avichal Rakesh wrote:
> 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.

But with isoc packets, it's ok to drop them, that's what the protocol is
for.  If you require loss-less data transfer, you can NOT use isoc
endpoints.

So this sounds like it is working correctly, but you need to figure out
why your device can't keep up on the data stream properly.

> 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.

So you need a faster processor?  :)

> 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(-)
> 
> diff --git a/drivers/usb/gadget/function/uvc.h
> b/drivers/usb/gadget/function/uvc.h
> index 100475b1363e..d2c837011546 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -80,7 +80,8 @@ struct uvc_request {
> struct uvc_video *video;
> struct sg_table sgt;
> u8 header[UVCG_REQUEST_HEADER_LEN];
> - struct uvc_buffer *last_buf;
> + struct uvc_buffer *uvc_buf;
> + bool is_last;
> };

The patch is corrupted and can't be applied by anyone, sorry :(

Please fix up your email client and submit it as a real patch?

thanks,

greg k-h