Re: [PATCH 4/4] media: uvcvideo: Do not add clock samples with small sof delta

[Ricardo Ribalda]

Hi Hans

On Mon, 11 May 2026 at 18:07, Hans de Goede <hansg@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 23-Mar-26 14:10, Ricardo Ribalda wrote:
> > Some UVC 1.1 cameras running in fast isochronous mode tend to spam the
> > USB host with a lot of empty packets. These packets contain clock
> > information and are added to the clock buffer but do not add any
> > accuracy to the calculation. In fact, it is quite the opposite, in our
> > calculations, only the first and the last timestamp is used, and we only
> > have 32 slots.
> >
> > Ignore the samples that will produce less than MIN_HW_TIMESTAMP_DIFF
> > data.
> >
> > Fixes: 141270bd95d4 ("media: uvcvideo: Refactor clock circular buffer")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index dcbc0941ffe6..e1a4e84d6841 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -544,6 +544,19 @@ static void uvc_video_clock_add_sample(struct uvc_clock *clock,
> >       spin_unlock_irqrestore(&clock->lock, flags);
> >  }
> >
> > +static inline u16 sof_diff(u16 a, u16 b)
> > +{
> > +     u32 aux;
> > +
> > +     a &= 2047;
> > +     b &= 2047;
> > +     if (a >= b)
> > +             return a - b;
> > +
> > +     aux = a + 2048;
> > +     return (u16)(aux - b);
> > +}
> > +
> >  static void
> >  uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> >                      const u8 *data, int len)
> > @@ -664,12 +677,13 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> >       sample.dev_sof = (sample.dev_sof + stream->clock.sof_offset) & 2047;
> >
> >       /*
> > -      * To limit the amount of data, drop SCRs with an SOF identical to the
> > +      * To limit the amount of data, drop SCRs with an SOF similar to the
> >        * previous one. This filtering is also needed to support UVC 1.5, where
> >        * all the data packets of the same frame contains the same SOF. In that
> >        * case only the first one will match the host_sof.
> >        */
> > -     if (sample.dev_sof == stream->clock.last_sof)
> > +     if (sof_diff(sample.dev_sof, stream->clock.last_sof) <=
> > +         (MIN_HW_TIMESTAMP_DIFF / stream->clock.size))
> >               return;
>
> If I understand things correctly then uvc_video_clock_update() uses
> first->host_time + some correction time. But you might end up not
> storing a sample for the very first isochronous USB packet of a frame
> because of this new check.  Which means that the first->host_time used
> as a starting point for the timestamp just has become inaccurate ?

In UVC 1.5 All the ISOC packets have the same dev_sof and dev_stc.
So this check will avoid adding a whole frame into the timestamp
circular buffer when running at more than 320 Hz (1/(0.1/32))

In UVC 1.1 all ISOC packets have the same dev_stc but different dev_sof.
This check will avoid adding some of those packets into the circular
buffer, but the accuracy will not be lost. We will use the data from
the neighbour packets (even from previous frames) to recover the sof.

The biggest winner for this patch is UVC 1.1, which will have much
more accurate timestamps, because the distance between the first and
last will be bigger (as in uvc1.5)
>
> Regards,
>
> Hans
>
>


-- 
Ricardo Ribalda