[PATCH v2 0/5] media: uvcvideo: Fixes for hw timestamping

[PATCH v2 0/5] media: uvcvideo: Fixes for hw timestamping

[Ricardo Ribalda]

This series introduces fixes for the hardware timestamp calculations.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v2:
- Fix comments
- Add UCV_ prefix
- Improve commit messages
- Add "Do not run expensive code if not needed" patchset
- Link to v1: https://lore.kernel.org/r/20260323-uvc-hwtimestamp-v1-0-aa42e3865204@chromium.org

---
Ricardo Ribalda (5):
      media: uvcvideo: Fix dev_sof filtering in hw timestamp
      media: uvcvideo: Use hw timestaming if the clock buffer is full
      media: uvcvideo: Relax the constrains for interpolating the hw clock
      media: uvcvideo: Do not add clock samples with small sof delta
      media: uvcvideo: clock: Do not run expensive code if not needed

 drivers/media/usb/uvc/uvc_video.c | 77 +++++++++++++++++++++++++++++----------
 drivers/media/usb/uvc/uvcvideo.h  |  3 +-
 2 files changed, 59 insertions(+), 21 deletions(-)
---
base-commit: bc1ba628e37c93cf2abeb2c79716f49087f8a024
change-id: 20260309-uvc-hwtimestamp-f25dc27f5711

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v2 1/5] media: uvcvideo: Fix dev_sof filtering in hw timestamp

[Ricardo Ribalda]

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@vger.kernel.org
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Tested-by: Yunke Cao <yunkec@google.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 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 f6c8e3223796..cbf67c17a49a 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;
+
 	uvc_video_clock_add_sample(&stream->clock, &sample);
 	stream->clock.last_sof = sample.dev_sof;
 }

-- 
2.54.0.563.g4f69b47b94-goog

[PATCH v2 2/5] media: uvcvideo: Use hw timestaming if the clock buffer is full

[Ricardo Ribalda]

In some situations, even with a full clock buffer, it does not contain
250msec of data. This results in the driver jumping back from software
to hardware timestapsing creating a nasty artifact in the video.

If the clock buffer is full, use it to calculate the timestamp instead
of defaulting to software stamps, the reduced accuracy is less visible
than jumping from one timestamping mechanism to the other.

Fixes: 6243c83be6ee8 ("media: uvcvideo: Allow hw clock updates with buffers not full")
Cc: stable@vger.kernel.org
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Tested-by: Yunke Cao <yunkec@google.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index cbf67c17a49a..19a2880e0dc9 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -834,15 +834,22 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
 		y2 += 2048 << 16;
 
 	/*
-	 * Have at least 1/4 of a second of timestamps before we
-	 * try to do any calculation. Otherwise we do not have enough
-	 * precision. This value was determined by running Android CTS
-	 * on different devices.
+	 * If the buffer is not full, we want to gather at least 1/4th of
+	 * timestamps before using HW timestamping. We do this to avoid jitter
+	 * on the initial frames.
+	 *
+	 * If the buffer is full we would use it regardless of how much data
+	 * it represents. This could be solved with an infinite big circular
+	 * buffer, but RAM is expensive these days, specially the infinitely
+	 * big.
+	 *
+	 * The value of 1/4th of a second was determined by running Android's
+	 * CTS on different devices.
 	 *
 	 * dev_sof runs at 1KHz, and we have a fixed point precision of
 	 * 16 bits.
 	 */
-	if ((y2 - y1) < ((1000 / 4) << 16))
+	if (clock->size != clock->count && (y2 - y1) < ((1000 / 4) << 16))
 		goto done;
 
 	y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2

-- 
2.54.0.563.g4f69b47b94-goog

[PATCH v2 3/5] media: uvcvideo: Relax the constrains for interpolating the hw clock

[Ricardo Ribalda]

In the initial version we set the min value to 250msec. Looks like
100msec can also provide a good value.

Now that we are at it, add a macro to make it cleaner.

Fixes: 6243c83be6ee8 ("media: uvcvideo: Allow hw clock updates with buffers not full")
Cc: stable@vger.kernel.org
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Tested-by: Yunke Cao <yunkec@google.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 19a2880e0dc9..093186308eac 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -494,6 +494,13 @@ static int uvc_commit_video(struct uvc_streaming *stream,
  * Clocks and timestamps
  */
 
+/*
+ * The accuracy of the hardware timestamping depends on having enough data to
+ * interpolate between the different clock domains. This value is sof cycles,
+ * this is, milliseconds.
+ */
+#define UVC_MIN_HW_TIMESTAMP_DIFF 100
+
 static inline ktime_t uvc_video_get_time(void)
 {
 	if (uvc_clock_param == CLOCK_MONOTONIC)
@@ -843,13 +850,13 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
 	 * buffer, but RAM is expensive these days, specially the infinitely
 	 * big.
 	 *
-	 * The value of 1/4th of a second was determined by running Android's
-	 * CTS on different devices.
+	 * The value of UVC_MIN_HW_TIMESTAMP_DIFF was determined by running
+	 * Android's CTS on different devices.
 	 *
-	 * dev_sof runs at 1KHz, and we have a fixed point precision of
-	 * 16 bits.
+	 * y1 and y2 are dev_sof with a fixed point precision of 16 bits.
 	 */
-	if (clock->size != clock->count && (y2 - y1) < ((1000 / 4) << 16))
+	if (clock->size != clock->count &&
+	    (y2 - y1) < (UVC_MIN_HW_TIMESTAMP_DIFF << 16))
 		goto done;
 
 	y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2

-- 
2.54.0.563.g4f69b47b94-goog

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

[Ricardo Ribalda]

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@vger.kernel.org
Tested-by: Yunke Cao <yunkec@google.com>
Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 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 093186308eac..8d0fd7003c62 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) <=
+	    (UVC_MIN_HW_TIMESTAMP_DIFF / stream->clock.size))
 		return;
 
 	uvc_video_clock_add_sample(&stream->clock, &sample);

-- 
2.54.0.563.g4f69b47b94-goog

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

[Ricardo Ribalda]

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>
---
 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();
+
 	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];

-- 
2.54.0.563.g4f69b47b94-goog

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

[Ricardo Ribalda]

Hi Yunke

Could you also test this new patch?

Thanks!

On Tue, 12 May 2026 at 14:31, Ricardo Ribalda <ribalda@chromium.org> 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>
> ---
>  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();
> +
>         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];
>
> --
> 2.54.0.563.g4f69b47b94-goog
>


-- 
Ricardo Ribalda

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

[Hans de Goede]

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 ?

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

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

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@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.

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

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

[Hans de Goede]

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