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

[Hans de Goede <hansg@kernel.org>]

Hi,

On 12-May-26 15:15, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Tue, 12 May 2026 at 15:02, Hans de Goede <hansg@kernel.org> 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@kernel.org>
>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>
>> 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.

I've no strong preference either way, if you prefer to keep it
here that is fine.

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];
>>>
>>
> 
>