On Wed, Feb 07, 2024 at 01:04:48PM +0000, Kieran Bingham wrote:
>Hi Michael,
>
>Quoting Michael Grzeschik (2024-02-06 21:30:17)
>> On Tue, Feb 06, 2024 at 03:23:17AM +0000, Thinh Nguyen wrote:
>> >On Tue, Feb 06, 2024, Michael Grzeschik wrote:
>> >> Hi Thinh
>> >>
>> >> I found some strange situation while streaming via uvc-gadget to some
>> >> usb host. It happens when some requests are missed due to higher load on
>> >> the gadget machine. In some cases some requests will reach the host
>> >> twice. In my special case, I added the following changes [1] for the
>> >> host and gadget side.
>> >
>> >Does this only happen to some specific hosts?
>>
>> >Are all the data of the duplicate requests matching or just some bits of
>> >the transfer? Were you able to confirm from some usb analyzer/sniffer
>> >that the data out the wire is actually duplicate?
>>
>> Turns out, this duplicates are just misinterpretations.
>
>I'm glad there's no deeper issue to worry about here.
>
>> The linux uvcvideo driver will parse the uvc header payload twice. (If
>> the FID was incremented inbetween). This led to those double misleading
>> outputs. Although this means that there is a bug in
>> uvc_video_stats_decode for incrementing the error count.
>
>Do you plan/are you able to submit a patch to fix this?  Hopefully that
>would prevent anyone else following the same rabbit-hole.

Sure. For now I came up with this changes. This leaves the errorcounts
in a sane range. However with this compiled kernel I saw some issues
with the host machine to freeze up. I will just ensure that this is not
caused by this particular patch.

Regards,
Michael

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 40eafe43d1888..b582698be7f00 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1094,6 +1094,29 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
                         uvc_video_stats_update(stream);
         }

+       /*
+        * Mark the buffer as done if we're at the beginning of a new frame.
+        * End of frame detection is better implemented by checking the EOF
+        * bit (FID bit toggling is delayed by one frame compared to the EOF
+        * bit), but some devices don't set the bit at end of frame (and the
+        * last payload can be lost anyway). We thus must check if the FID has
+        * been toggled.
+        *
+        * stream->last_fid is initialized to -1, so the first isochronous
+        * frame will never trigger an end of frame detection.
+        *
+        * Empty buffers (bytesused == 0) don't trigger end of frame detection
+        * as it doesn't make sense to return an empty buffer. This also
+        * avoids detecting end of frame conditions at FID toggling if the
+        * previous payload had the EOF bit set.
+        */
+       if (fid != stream->last_fid && buf->bytesused != 0) {
+               uvc_dbg(stream->dev, FRAME,
+                       "Frame complete (FID bit toggled)\n");
+               buf->state = UVC_BUF_STATE_READY;
+               return -EAGAIN;
+       }
+
         uvc_video_clock_decode(stream, buf, data, len);
         uvc_video_stats_decode(stream, data, len);

@@ -1140,29 +1163,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
                 buf->state = UVC_BUF_STATE_ACTIVE;
         }

-       /*
-        * Mark the buffer as done if we're at the beginning of a new frame.
-        * End of frame detection is better implemented by checking the EOF
-        * bit (FID bit toggling is delayed by one frame compared to the EOF
-        * bit), but some devices don't set the bit at end of frame (and the
-        * last payload can be lost anyway). We thus must check if the FID has
-        * been toggled.
-        *
-        * stream->last_fid is initialized to -1, so the first isochronous
-        * frame will never trigger an end of frame detection.
-        *
-        * Empty buffers (bytesused == 0) don't trigger end of frame detection
-        * as it doesn't make sense to return an empty buffer. This also
-        * avoids detecting end of frame conditions at FID toggling if the
-        * previous payload had the EOF bit set.
-        */
-       if (fid != stream->last_fid && buf->bytesused != 0) {
-               uvc_dbg(stream->dev, FRAME,
-                       "Frame complete (FID bit toggled)\n");
-               buf->state = UVC_BUF_STATE_READY;
-               return -EAGAIN;
-       }
-
         stream->last_fid = fid;

         return data[0];



-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |