Hello,

On Fri, Nov 26, 2021 at 05:14:31PM +0100, Ricardo Ribalda Delgado wrote:

Hello Hans

What if we make something like:

#define UVC_STREAM_FID_UNINITIALISED (UVC_STREAM_FID + 1)

and then use that define at the initialization and in decode_start() ?
I think it will be clearer than the current comparison.


Also you might want to wait to assign "stream->last_fid = fid;" until
line 1106, because otherwise the "Dropping payload" will be triggered
(I believe)

Thanks!

PS: You will get better response time if you email me at
ribalda@chromium.org , not much time recently for the personal email
:(

On Wed, Nov 24, 2021 at 11:49 AM Hans Verkuil wrote:

quoted

When you start streaming from uvc, then the first buffer will
have sequence number 0 and the second buffer has sequence number
2. Fix the logic to ensure proper monotonically increasing sequence
numbers.

The root cause is not setting last_fid when you start streaming
and a new fid is found for the first time.

I can confirm the issue with my device, but this short explanation
doesn't really tell me where the problem comes from. Could you elaborate
on this, maybe with a detailed sequence ?

quoted

This patch also changes the initial last_fid value from -1 to 0xff.
Since last_fid is unsigned, it is better to stick to unsigned values.

Maybe U8_MAX then ?

quoted

Signed-off-by: Hans Verkuil <redacted>
---
 drivers/media/usb/uvc/uvc_video.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 9f37eaf28ce7..8ba8d25e2c4a 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c

@@ -1055,7 +1055,10 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
         * that discontinuous sequence numbers always indicate lost frames.
         */
        if (stream->last_fid != fid) {
-               stream->sequence++;
+               if (stream->last_fid > UVC_STREAM_FID)
+                       stream->last_fid = fid;
+               else
+                       stream->sequence++;
                if (stream->sequence)
                        uvc_video_stats_update(stream);
        }

@@ -1080,7 +1083,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,

        /* Synchronize to the input stream by waiting for the FID bit to be
         * toggled when the the buffer state is not UVC_BUF_STATE_ACTIVE.
-        * stream->last_fid is initialized to -1, so the first isochronous
+        * stream->last_fid is initialized to 0xff, so the first isochronous
         * frame will always be in sync.
         *
         * If the device doesn't toggle the FID bit, invert stream->last_fid

@@ -1111,7 +1114,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
         * 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
+        * stream->last_fid is initialized to 0xff, so the first isochronous
         * frame will never trigger an end of frame detection.
         *
         * Empty buffers (bytesused == 0) don't trigger end of frame detection

@@ -1895,7 +1898,7 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
        int ret;

        stream->sequence = -1;
-       stream->last_fid = -1;
+       stream->last_fid = 0xff;
        stream->bulk.header_size = 0;
        stream->bulk.skip_payload = 0;
        stream->bulk.payload_size = 0;

-- 
Regards,

Laurent Pinchart