Re: [PATCH 1/4] media: uvcvideo: Fix dev_sof filtering in hw timestamp

[Ricardo Ribalda]

On Mon, 11 May 2026 at 17:46, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Mon, Mar 23, 2026 at 01:10:28PM +0000, Ricardo Ribalda wrote:
> > To avoid filling the clock circular buffer with duplicated data we only
> > add it if the new value sof is different than the last added sof.
> >
> > The issue is that we compare the unprocess sof with the processed sof.
> > If there is a sof_offset, or UVC_QUIRK_INVALID_DEVICE_SOF is enabled,
> > the comparison will not work as expected.
> >
> > This patch moves the comparison to the right place.
> >
> > 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 | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 40c76c051da2..6786ca38fe5e 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -583,16 +583,7 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> >       if (!has_scr)
> >               return;
> >
> > -     /*
> > -      * To limit the amount of data, drop SCRs with an SOF identical 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.
> > -      */
> >       sample.dev_sof = get_unaligned_le16(&data[header_size - 2]);
> > -     if (sample.dev_sof == stream->clock.last_sof)
> > -             return;
> > -
> >       sample.dev_stc = get_unaligned_le32(&data[header_size - 6]);
> >
> >       /*
> > @@ -664,6 +655,16 @@ 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
> > +      * 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)
> > +             return;
> > +
>
> We will now uncondtionally call some potentially more expensive
> operations, in particular usb_get_current_frame_number(). Wouldn't it be
> better to store the unprocessed SOF in the sample in addition to the
> processed SOF, to allow early comparison ?

Works for me. But I'd rather do it as an optimization 5/5

I would like to have an early equality comparison against the
unprocessed_sof. And then a similarity check as in 4/5 with the
processed_sof
>
> >       uvc_video_clock_add_sample(&stream->clock, &sample);
> >       stream->clock.last_sof = sample.dev_sof;
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda