[PATCH] uvc: fix sequence counting

[PATCH] uvc: fix sequence counting

[Hans Verkuil]

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.

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.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 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;
-- 
2.33.0

Re: [PATCH] uvc: fix sequence counting

[Ricardo Ribalda Delgado]

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 <hverkuil@xs4all.nl> wrote:
>
> 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.
>
> 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.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  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;
> --
> 2.33.0
>


--
Ricardo Ribalda

Re: [PATCH] uvc: fix sequence counting

[Laurent Pinchart]

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:
> >
> > 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 ?

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

> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  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