Re: [PATCH v2 5/5] media: uvcvideo: clock: Do not run expensive code if not needed

[Ricardo Ribalda]

Hi Hans

On Tue, 12 May 2026 at 15:02, Hans de Goede <hansg@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 12-May-26 14:30, Ricardo Ribalda wrote:
> > We only save relevant samples into the circular buffer.
> >
> > If the data is very similar to the previous one, exit early.
> >
> > If the data is not going to be added, do not calculate the wall time.
> >
> > Suggested-by: Hans de Goede <hansg@xxxxxxxxxx>
> > Suggested-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
>
> 2 nits here, first nit, this really seems to be 2 different
> changes, IMHO it would be cleaner to have the (non trivial)
> early-exit on raw sof match patch as a separate patch from
> the one moving the uvc_video_get_time() call ?

ack
>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 20 ++++++++++++++------
> >  drivers/media/usb/uvc/uvcvideo.h  |  3 ++-
> >  2 files changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 8d0fd7003c62..ea8a76f57963 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -524,7 +524,7 @@ static void uvc_video_clock_add_sample(struct uvc_clock *clock,
> >
> >       spin_lock_irqsave(&clock->lock, flags);
> >
> > -     if (clock->count > 0 && clock->last_sof > sample->dev_sof) {
> > +     if (clock->count > 0 && clock->last_sof_processed > sample->dev_sof) {
> >               /*
> >                * Remove data from the circular buffer that is older than the
> >                * last SOF overflow. We only support one SOF overflow per
> > @@ -606,6 +606,12 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> >       sample.dev_sof = get_unaligned_le16(&data[header_size - 2]);
> >       sample.dev_stc = get_unaligned_le32(&data[header_size - 6]);
> >
> > +     /* If the sample sof is very similar to the previous one quit early. */
> > +     if (stream->clock.last_sof_raw == sample.dev_sof)
> > +             return;
> > +
> > +     stream->clock.last_sof_raw = sample.dev_sof;
> > +
> >       /*
> >        * STC (Source Time Clock) is the clock used by the camera. The UVC 1.5
> >        * standard states that it "must be captured when the first video data
> > @@ -644,8 +650,6 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> >       if (stream->dev->quirks & UVC_QUIRK_INVALID_DEVICE_SOF)
> >               sample.dev_sof = sample.host_sof;
> >
> > -     sample.host_time = uvc_video_get_time();
> > -
> >       /*
> >        * The UVC specification allows device implementations that can't obtain
> >        * the USB frame number to keep their own frame counters as long as they
> > @@ -682,19 +686,23 @@ uvc_video_clock_decode(struct uvc_streaming *stream, struct uvc_buffer *buf,
> >        * 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 (sof_diff(sample.dev_sof, stream->clock.last_sof) <=
> > +     if (sof_diff(sample.dev_sof, stream->clock.last_sof_processed) <=
> >           (UVC_MIN_HW_TIMESTAMP_DIFF / stream->clock.size))
> >               return;
> >
> > +     /* This is expensive, only do it if needed */
> > +     sample.host_time = uvc_video_get_time();
> > +
>
> I think it would be slightly cleaner to just move this call
> to inside uvc_video_clock_add_sample() ?

If you do not mind I prefer to keep it as is.

The other uvc_video_clock (add_sample, reset, init, cleanup) are
"content agnostic" and I would prefer to keep it that way.

Let me know if this is ok before I send v3.

Thanks!

>
> Regards,
>
> Hans
>
>
>
>
> >       uvc_video_clock_add_sample(&stream->clock, &sample);
> > -     stream->clock.last_sof = sample.dev_sof;
> > +     stream->clock.last_sof_processed = sample.dev_sof;
> >  }
> >
> >  static void uvc_video_clock_reset(struct uvc_clock *clock)
> >  {
> >       clock->head = 0;
> >       clock->count = 0;
> > -     clock->last_sof = -1;
> > +     clock->last_sof_processed = -1;
> > +     clock->last_sof_raw = -1;
> >       clock->last_sof_overflow = -1;
> >       clock->sof_offset = -1;
> >  }
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 0a0c01b2420f..7b8477e5a0ba 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -522,7 +522,8 @@ struct uvc_streaming {
> >               unsigned int size;
> >               unsigned int last_sof_overflow;
> >
> > -             u16 last_sof;
> > +             u16 last_sof_processed;
> > +             u16 last_sof_raw;
> >               u16 sof_offset;
> >
> >               u8 last_scr[6];
> >
>


-- 
Ricardo Ribalda