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

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

[Hans de Goede]

Hi,

On 23-Mar-26 14:10, Ricardo Ribalda wrote:
> 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
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans




> ---
>  drivers/media/usb/uvc/uvc_video.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 6786ca38fe5e..c7ebedb3450f 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -842,7 +842,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
>  	 * 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
> 

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

[Laurent Pinchart]

On Mon, Mar 23, 2026 at 01:10:29PM +0000, Ricardo Ribalda wrote:
> 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
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 6786ca38fe5e..c7ebedb3450f 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -842,7 +842,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
>  	 * 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;

This requires an update to the comment above.

>  
>  	y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] media: uvcvideo: Relax the constrains for interpolating the hw clock

[Hans de Goede]

Hi,

On 23-Mar-26 14:10, Ricardo Ribalda wrote:
> 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, refactor a bit the code to make it cleaner.
> 
> Fixes: 6243c83be6ee8 ("media: uvcvideo: Allow hw clock updates with buffers not full")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans



> ---
>  drivers/media/usb/uvc/uvc_video.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index c7ebedb3450f..dcbc0941ffe6 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 MIN_HW_TIMESTAMP_DIFF 100
> +
>  static inline ktime_t uvc_video_get_time(void)
>  {
>  	if (uvc_clock_param == CLOCK_MONOTONIC)
> @@ -834,15 +841,12 @@ 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.
> +	 * Check that we have enough data to do the interpolation.
>  	 *
> -	 * 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) < (MIN_HW_TIMESTAMP_DIFF << 16))
>  		goto done;
>  
>  	y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
> 

Re: [PATCH 3/4] media: uvcvideo: Relax the constrains for interpolating the hw clock

[Laurent Pinchart]

On Mon, Mar 23, 2026 at 01:10:30PM +0000, Ricardo Ribalda wrote:
> In the initial version we set the min value to 250msec. Looks like
> 100msec can also provide a good value.

I'd like to know where the value comes from and how it has been tested.

> Now that we are at it, refactor a bit the code to make it cleaner.

Do you mean using a macro ? You can mention that explicitly here.

> Fixes: 6243c83be6ee8 ("media: uvcvideo: Allow hw clock updates with buffers not full")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index c7ebedb3450f..dcbc0941ffe6 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 MIN_HW_TIMESTAMP_DIFF 100

UVC prefix.

> +
>  static inline ktime_t uvc_video_get_time(void)
>  {
>  	if (uvc_clock_param == CLOCK_MONOTONIC)
> @@ -834,15 +841,12 @@ 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.
> +	 * Check that we have enough data to do the interpolation.
>  	 *
> -	 * 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) < (MIN_HW_TIMESTAMP_DIFF << 16))
>  		goto done;
>  
>  	y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] media: uvcvideo: Relax the constrains for interpolating the hw clock

[Ricardo Ribalda]

Hi Laurent

On Mon, 11 May 2026 at 17:51, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Mar 23, 2026 at 01:10:30PM +0000, Ricardo Ribalda wrote:
> > In the initial version we set the min value to 250msec. Looks like
> > 100msec can also provide a good value.
>
> I'd like to know where the value comes from and how it has been tested.

I used the Android CTS framework for testing. It checks in multiple
places that the timestamps are stable.

>
> > Now that we are at it, refactor a bit the code to make it cleaner.
>
> Do you mean using a macro ? You can mention that explicitly here.
>
> > Fixes: 6243c83be6ee8 ("media: uvcvideo: Allow hw clock updates with buffers not full")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index c7ebedb3450f..dcbc0941ffe6 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 MIN_HW_TIMESTAMP_DIFF 100
>
> UVC prefix.
>
> > +
> >  static inline ktime_t uvc_video_get_time(void)
> >  {
> >       if (uvc_clock_param == CLOCK_MONOTONIC)
> > @@ -834,15 +841,12 @@ 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.
> > +      * Check that we have enough data to do the interpolation.
> >        *
> > -      * 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) < (MIN_HW_TIMESTAMP_DIFF << 16))
> >               goto done;
> >
> >       y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH 3/4] media: uvcvideo: Relax the constrains for interpolating the hw clock

[Laurent Pinchart]

On Mon, May 11, 2026 at 05:58:30PM +0200, Ricardo Ribalda wrote:
> On Mon, 11 May 2026 at 17:51, Laurent Pinchart wrote:
> > On Mon, Mar 23, 2026 at 01:10:30PM +0000, Ricardo Ribalda wrote:
> > > In the initial version we set the min value to 250msec. Looks like
> > > 100msec can also provide a good value.
> >
> > I'd like to know where the value comes from and how it has been tested.
> 
> I used the Android CTS framework for testing. It checks in multiple
> places that the timestamps are stable.

You deleted the information from the comment below, and didn't mention
anything in the commit message, so I was wondering if the situation has
changed. I'd like to retain the information somewhere.

> > > Now that we are at it, refactor a bit the code to make it cleaner.
> >
> > Do you mean using a macro ? You can mention that explicitly here.
> >
> > > Fixes: 6243c83be6ee8 ("media: uvcvideo: Allow hw clock updates with buffers not full")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_video.c | 18 +++++++++++-------
> > >  1 file changed, 11 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > index c7ebedb3450f..dcbc0941ffe6 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 MIN_HW_TIMESTAMP_DIFF 100
> >
> > UVC prefix.
> >
> > > +
> > >  static inline ktime_t uvc_video_get_time(void)
> > >  {
> > >       if (uvc_clock_param == CLOCK_MONOTONIC)
> > > @@ -834,15 +841,12 @@ 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.
> > > +      * Check that we have enough data to do the interpolation.
> > >        *
> > > -      * 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) < (MIN_HW_TIMESTAMP_DIFF << 16))
> > >               goto done;
> > >
> > >       y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] media: uvcvideo: Relax the constrains for interpolating the hw clock

[Ricardo Ribalda]

Hi Laurent

On Tue, 12 May 2026 at 14:38, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, May 11, 2026 at 05:58:30PM +0200, Ricardo Ribalda wrote:
> > On Mon, 11 May 2026 at 17:51, Laurent Pinchart wrote:
> > > On Mon, Mar 23, 2026 at 01:10:30PM +0000, Ricardo Ribalda wrote:
> > > > In the initial version we set the min value to 250msec. Looks like
> > > > 100msec can also provide a good value.
> > >
> > > I'd like to know where the value comes from and how it has been tested.
> >
> > I used the Android CTS framework for testing. It checks in multiple
> > places that the timestamps are stable.
>
> You deleted the information from the comment below, and didn't mention
> anything in the commit message, so I was wondering if the situation has
> changed. I'd like to retain the information somewhere.

I believe it should be fixed in v2. Looks like I have to send a v3
anyway :P so I will double check that it is still there.

Thanks!


>
> > > > Now that we are at it, refactor a bit the code to make it cleaner.
> > >
> > > Do you mean using a macro ? You can mention that explicitly here.
> > >
> > > > Fixes: 6243c83be6ee8 ("media: uvcvideo: Allow hw clock updates with buffers not full")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_video.c | 18 +++++++++++-------
> > > >  1 file changed, 11 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index c7ebedb3450f..dcbc0941ffe6 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 MIN_HW_TIMESTAMP_DIFF 100
> > >
> > > UVC prefix.
> > >
> > > > +
> > > >  static inline ktime_t uvc_video_get_time(void)
> > > >  {
> > > >       if (uvc_clock_param == CLOCK_MONOTONIC)
> > > > @@ -834,15 +841,12 @@ 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.
> > > > +      * Check that we have enough data to do the interpolation.
> > > >        *
> > > > -      * 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) < (MIN_HW_TIMESTAMP_DIFF << 16))
> > > >               goto done;
> > > >
> > > >       y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

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

[Hans de Goede]

Hi,

On 23-Mar-26 14:10, 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@vger.kernel.org
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans




> ---
>  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;
> +
>  	uvc_video_clock_add_sample(&stream->clock, &sample);
>  	stream->clock.last_sof = sample.dev_sof;
>  }
> 

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

[Laurent Pinchart]

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@vger.kernel.org
> 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 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 ?

>  	uvc_video_clock_add_sample(&stream->clock, &sample);
>  	stream->clock.last_sof = sample.dev_sof;
>  }
> 

-- 
Regards,

Laurent Pinchart

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@ideasonboard.com> 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@vger.kernel.org
> > 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 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

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

[Hans de Goede]

Hi,

On 11-May-26 17:46, Laurent Pinchart 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@vger.kernel.org
>> 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 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 ?

While reviewing 4/4 I just came to the same conclusion,
uvc_video_get_time() is also expensive and unnecessary unless we
actually end up doing the uvc_video_clock_add_sample().

Moving the uvc_video_get_time() to below the check is
a straight-forward change.

Regards,

Hans


> 
>>  	uvc_video_clock_add_sample(&stream->clock, &sample);
>>  	stream->clock.last_sof = sample.dev_sof;
>>  }
>>
> 

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

[Hans de Goede]

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@vger.kernel.org
> 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 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 ?

Regards,

Hans

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@kernel.org> 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@vger.kernel.org
> > 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 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

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

[Hans de Goede]

Hi,

On 11-May-26 18:50, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Mon, 11 May 2026 at 18:07, Hans de Goede <hansg@kernel.org> 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@vger.kernel.org
>>> 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 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)

I'm still trying to wrap my head about the whole concept of the hw
timestamps TBH.

Upon reading it a couple of times I now see that when exactly we
take samples is not important because the actual frame time in
STC units is stored in buf->pts and that is supposed to be our
starting point. And the rest is just used to calculate
a factor + offset.

At least that is what the big comment says but I'm confused by
the code which is supposed to implement:

 * SOF = (SOF2 - SOF1) / (STC2 - STC1) * PTS
 *     + (SOF1 * STC2 - SOF2 * STC1) / (STC2 - STC1)
 *
 * or
 *
 * SOF = ((SOF2 - SOF1) * PTS + SOF1 * STC2 - SOF2 * STC1) / (STC2 - STC1)   (1)

I think that the code tries to implement the second formula:

We've (with some checks removed):

        /* First step, PTS to SOF conversion. */
        delta_stc = buf->pts - (1UL << 31);
        x1 = first->dev_stc - delta_stc;
        x2 = last->dev_stc - delta_stc;

        y1 = (first->dev_sof + 2048) << 16;
        y2 = (last->dev_sof + 2048) << 16;
        if (y2 < y1)
                y2 += 2048 << 16;

        y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
          - (u64)y2 * (u64)x1;
        y = div_u64(y, x2 - x1);

        sof = y;

Simplifying this by removing all the range-shifting
and using sof1/sof2 instead of y1/y2 like in the comment
we end up with:

        x1 = first->dev_stc - buf->pts;
        x2 = last->dev_stc - buf->pts;

	sof1 = first->dev_sof;
	sof2 = last->dev_sof

	sof = ((sof2 - sof1) + sof1 * x2 - sof2 * x1) / (x2 - x1)

Now substitute stc1/stc2 for first->dev_stc / last->dev_stc
and just pts for buf->pts and expand x1 + x2 we get:

	sof = ((sof2 - sof1) + sof1 * (stc2 - pts) - sof2 * (stc1 - pts)) /
	      ((stc2 - pts) - (stc1 - pts))

We can simplify the divisor here by getting rid of the pts bit
since the 2 "- pts" parts negate each other:

	sof = ((sof2 - sof1) + sof1 * (stc2 - pts) - sof2 * (stc1 - pts)) /
	      (stc2 - stc1)

Now lets get rid of the () from expanding x1 / x2:

	sof = ((sof2 - sof1) + sof1 * stc2 - sof1 * pts - sof2 * stc1 + sof2 * pts)) /
	      (stc2 - stc1)

Shuffle bringing " * pts" parts to the front:

	sof = (sof2 * pts - sof1 * pts + (sof2 - sof1) + sof1 * stc2 - sof2 * stc1)) /
	      (stc2 - stc1)

Simplify:

	sof = ((sof2 - sof1) * pts + (sof2 - sof1) + sof1 * stc2 - sof2 * stc1) /
	      (stc2 - stc1)

Looks a lot like the comment except there is a + (sof2 - sof1) too much
in there ?

And some of the range shifting also feels wrong. As long as we're only
subtracting the range shifting is fine. But as soon as we start multiplying
variables in different shifted ranges the end result actually changes.

Especially weird here is that we range-shift by (1UL << 31) for calculating
delta_stc and then *multiply* (y2 - y1) by (1ULL << 31) I guess this is
to compensate for the (1ULL << 31) component of x1/x2 but the first->dev_stc
and pts parts of x1 where never multiplied by (1ULL << 31) so these
are still in their original *scale*. Either we should multiply all
parts to go to some other fixed scale and the sof value are both range-shifted
by 2048 as well as multiplied by 65536, which also seems wrong to me as
soon as we do sof1 * stc2 or sof2 * stc1

All in all this all feels like there are some issues lurking here and it
does not seem to match the comment at the top.

Regards,

Hans

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

[Ricardo Ribalda]

Hi Hans

(Hi Laurent :P)



On Mon, 11 May 2026 at 20:33, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi,
>
> On 11-May-26 18:50, Ricardo Ribalda wrote:
> > Hi Hans
> >
> > On Mon, 11 May 2026 at 18:07, Hans de Goede <hansg@kernel.org> 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@vger.kernel.org
> >>> 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 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)
>
> I'm still trying to wrap my head about the whole concept of the hw
> timestamps TBH.
>
> Upon reading it a couple of times I now see that when exactly we
> take samples is not important because the actual frame time in
> STC units is stored in buf->pts and that is supposed to be our
> starting point. And the rest is just used to calculate
> a factor + offset.
>
> At least that is what the big comment says but I'm confused by
> the code which is supposed to implement:
>
>  * SOF = (SOF2 - SOF1) / (STC2 - STC1) * PTS
>  *     + (SOF1 * STC2 - SOF2 * STC1) / (STC2 - STC1)
>  *
>  * or
>  *
>  * SOF = ((SOF2 - SOF1) * PTS + SOF1 * STC2 - SOF2 * STC1) / (STC2 - STC1)   (1)
>
> I think that the code tries to implement the second formula:
>
> We've (with some checks removed):
>
>         /* First step, PTS to SOF conversion. */
>         delta_stc = buf->pts - (1UL << 31);
>         x1 = first->dev_stc - delta_stc;
>         x2 = last->dev_stc - delta_stc;
>
>         y1 = (first->dev_sof + 2048) << 16;
>         y2 = (last->dev_sof + 2048) << 16;
>         if (y2 < y1)
>                 y2 += 2048 << 16;
>
>         y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
>           - (u64)y2 * (u64)x1;
>         y = div_u64(y, x2 - x1);
>
>         sof = y;
>
> Simplifying this by removing all the range-shifting
> and using sof1/sof2 instead of y1/y2 like in the comment
> we end up with:
>
>         x1 = first->dev_stc - buf->pts;
>         x2 = last->dev_stc - buf->pts;
>
>         sof1 = first->dev_sof;
>         sof2 = last->dev_sof
>
>         sof = ((sof2 - sof1) + sof1 * x2 - sof2 * x1) / (x2 - x1)
>
> Now substitute stc1/stc2 for first->dev_stc / last->dev_stc
> and just pts for buf->pts and expand x1 + x2 we get:
>
>         sof = ((sof2 - sof1) + sof1 * (stc2 - pts) - sof2 * (stc1 - pts)) /
>               ((stc2 - pts) - (stc1 - pts))



I think this is where your explanation goes slightly off:

x2 is actually stc2 - pts + (1UL << 31), and x1 is stc1 - pts + (1UL << 31).

Before you scream at me, look at the end of the mail! :P


>
> We can simplify the divisor here by getting rid of the pts bit
> since the 2 "- pts" parts negate each other:
>
>         sof = ((sof2 - sof1) + sof1 * (stc2 - pts) - sof2 * (stc1 - pts)) /
>               (stc2 - stc1)
>
> Now lets get rid of the () from expanding x1 / x2:
>
>         sof = ((sof2 - sof1) + sof1 * stc2 - sof1 * pts - sof2 * stc1 + sof2 * pts)) /
>               (stc2 - stc1)
>
> Shuffle bringing " * pts" parts to the front:
>
>         sof = (sof2 * pts - sof1 * pts + (sof2 - sof1) + sof1 * stc2 - sof2 * stc1)) /
>               (stc2 - stc1)
>
> Simplify:
>
>         sof = ((sof2 - sof1) * pts + (sof2 - sof1) + sof1 * stc2 - sof2 * stc1) /
>               (stc2 - stc1)
>
> Looks a lot like the comment except there is a + (sof2 - sof1) too much
> in there ?
>
> And some of the range shifting also feels wrong. As long as we're only
> subtracting the range shifting is fine. But as soon as we start multiplying
> variables in different shifted ranges the end result actually changes.
>
> Especially weird here is that we range-shift by (1UL << 31) for calculating
> delta_stc and then *multiply* (y2 - y1) by (1ULL << 31) I guess this is
> to compensate for the (1ULL << 31) component of x1/x2 but the first->dev_stc
> and pts parts of x1 where never multiplied by (1ULL << 31) so these
> are still in their original *scale*. Either we should multiply all
> parts to go to some other fixed scale and the sof value are both range-shifted
> by 2048 as well as multiplied by 65536, which also seems wrong to me as
> soon as we do sof1 * stc2 or sof2 * stc1
>
> All in all this all feels like there are some issues lurking here and it
> does not seem to match the comment at the top.
>
> Regards,
>
> Hans
>
>
>

Lets go back to the beggining:

SOF = ((SOF2 - SOF1) * PTS + SOF1 * STC2 - SOF2 * STC1) / (STC2 - STC1)

This is the formula for a straight line when you know two points. The
names are super ugly, lets use something we are more used to:

y = ((y2 - y1) * x + y1 * x2 - y2 * x1) / (x2 - x1) ;

Ok. how would this look if we want x to be exactly at (1 << 31) to
prevent unsigned underflow? ?

we just have to move things around:

delta = x - (1<<31);

new_x1 = x1 - delta = x1 - x + (1<<31)
new_x2 = x2 - delta = x2 - x + (1<<31)

We plug this in the formula and:

y = ((y2-y1) * (1 <<31) + y1 * new_x2 - y2*new_x1) /(new_x2-new_x1);
Which is exactly what we have.


Now lets look at the scaling:

         delta_stc = buf->pts - (1UL << 31);
         x1 = first->dev_stc - delta_stc;
         x2 = last->dev_stc - delta_stc;

X1 and X2 are NOT scaled

         y1 = (first->dev_sof + 2048) << 16;
         y2 = (last->dev_sof + 2048) << 16;
         if (y2 < y1)
                 y2 += 2048 << 16;

Y1 and Y2 is scaled 16 (ignore the +2048, the variable is mod(2048))

         y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
           - (u64)y2 * (u64)x1;
         y = div_u64(y, x2 - x1);

y = (SCALE16 - SCALE16)*K + SCALE16*SCALE1 - SCALE16*SCALE1; => Result
is SCALE16
y = div64(SCALE16, SCALE1) => Result is SCALE16

So it looks good to me. It is a painful code, but I think it is correct.

(Painful, but I would probably fail to do it better :P)

Regards



-- 
Ricardo Ribalda

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

[Hans de Goede]

HI,

On 11-May-26 23:36, Ricardo Ribalda wrote:
> Hi Hans
> 
> (Hi Laurent :P)
> 
> 
> 
> On Mon, 11 May 2026 at 20:33, Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi,
>>
>> On 11-May-26 18:50, Ricardo Ribalda wrote:
>>> Hi Hans
>>>
>>> On Mon, 11 May 2026 at 18:07, Hans de Goede <hansg@kernel.org> 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@vger.kernel.org
>>>>> 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 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)
>>
>> I'm still trying to wrap my head about the whole concept of the hw
>> timestamps TBH.
>>
>> Upon reading it a couple of times I now see that when exactly we
>> take samples is not important because the actual frame time in
>> STC units is stored in buf->pts and that is supposed to be our
>> starting point. And the rest is just used to calculate
>> a factor + offset.
>>
>> At least that is what the big comment says but I'm confused by
>> the code which is supposed to implement:
>>
>>  * SOF = (SOF2 - SOF1) / (STC2 - STC1) * PTS
>>  *     + (SOF1 * STC2 - SOF2 * STC1) / (STC2 - STC1)
>>  *
>>  * or
>>  *
>>  * SOF = ((SOF2 - SOF1) * PTS + SOF1 * STC2 - SOF2 * STC1) / (STC2 - STC1)   (1)
>>
>> I think that the code tries to implement the second formula:
>>
>> We've (with some checks removed):
>>
>>         /* First step, PTS to SOF conversion. */
>>         delta_stc = buf->pts - (1UL << 31);
>>         x1 = first->dev_stc - delta_stc;
>>         x2 = last->dev_stc - delta_stc;
>>
>>         y1 = (first->dev_sof + 2048) << 16;
>>         y2 = (last->dev_sof + 2048) << 16;
>>         if (y2 < y1)
>>                 y2 += 2048 << 16;
>>
>>         y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
>>           - (u64)y2 * (u64)x1;
>>         y = div_u64(y, x2 - x1);
>>
>>         sof = y;
>>
>> Simplifying this by removing all the range-shifting
>> and using sof1/sof2 instead of y1/y2 like in the comment
>> we end up with:
>>
>>         x1 = first->dev_stc - buf->pts;
>>         x2 = last->dev_stc - buf->pts;
>>
>>         sof1 = first->dev_sof;
>>         sof2 = last->dev_sof
>>
>>         sof = ((sof2 - sof1) + sof1 * x2 - sof2 * x1) / (x2 - x1)
>>
>> Now substitute stc1/stc2 for first->dev_stc / last->dev_stc
>> and just pts for buf->pts and expand x1 + x2 we get:
>>
>>         sof = ((sof2 - sof1) + sof1 * (stc2 - pts) - sof2 * (stc1 - pts)) /
>>               ((stc2 - pts) - (stc1 - pts))
> 
> 
> 
> I think this is where your explanation goes slightly off:
> 
> x2 is actually stc2 - pts + (1UL << 31), and x1 is stc1 - pts + (1UL << 31).
> 
> Before you scream at me, look at the end of the mail! :P
> 
> 
>>
>> We can simplify the divisor here by getting rid of the pts bit
>> since the 2 "- pts" parts negate each other:
>>
>>         sof = ((sof2 - sof1) + sof1 * (stc2 - pts) - sof2 * (stc1 - pts)) /
>>               (stc2 - stc1)
>>
>> Now lets get rid of the () from expanding x1 / x2:
>>
>>         sof = ((sof2 - sof1) + sof1 * stc2 - sof1 * pts - sof2 * stc1 + sof2 * pts)) /
>>               (stc2 - stc1)
>>
>> Shuffle bringing " * pts" parts to the front:
>>
>>         sof = (sof2 * pts - sof1 * pts + (sof2 - sof1) + sof1 * stc2 - sof2 * stc1)) /
>>               (stc2 - stc1)
>>
>> Simplify:
>>
>>         sof = ((sof2 - sof1) * pts + (sof2 - sof1) + sof1 * stc2 - sof2 * stc1) /
>>               (stc2 - stc1)
>>
>> Looks a lot like the comment except there is a + (sof2 - sof1) too much
>> in there ?
>>
>> And some of the range shifting also feels wrong. As long as we're only
>> subtracting the range shifting is fine. But as soon as we start multiplying
>> variables in different shifted ranges the end result actually changes.
>>
>> Especially weird here is that we range-shift by (1UL << 31) for calculating
>> delta_stc and then *multiply* (y2 - y1) by (1ULL << 31) I guess this is
>> to compensate for the (1ULL << 31) component of x1/x2 but the first->dev_stc
>> and pts parts of x1 where never multiplied by (1ULL << 31) so these
>> are still in their original *scale*. Either we should multiply all
>> parts to go to some other fixed scale and the sof value are both range-shifted
>> by 2048 as well as multiplied by 65536, which also seems wrong to me as
>> soon as we do sof1 * stc2 or sof2 * stc1
>>
>> All in all this all feels like there are some issues lurking here and it
>> does not seem to match the comment at the top.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
> 
> Lets go back to the beggining:
> 
> SOF = ((SOF2 - SOF1) * PTS + SOF1 * STC2 - SOF2 * STC1) / (STC2 - STC1)
> 
> This is the formula for a straight line when you know two points. The
> names are super ugly, lets use something we are more used to:
> 
> y = ((y2 - y1) * x + y1 * x2 - y2 * x1) / (x2 - x1) ;
> 
> Ok. how would this look if we want x to be exactly at (1 << 31) to
> prevent unsigned underflow? ?
> 
> we just have to move things around:
> 
> delta = x - (1<<31);
> 
> new_x1 = x1 - delta = x1 - x + (1<<31)
> new_x2 = x2 - delta = x2 - x + (1<<31)
> 
> We plug this in the formula and:
> 
> y = ((y2-y1) * (1 <<31) + y1 * new_x2 - y2*new_x1) /(new_x2-new_x1);
> Which is exactly what we have.
> 
> 
> Now lets look at the scaling:
> 
>          delta_stc = buf->pts - (1UL << 31);
>          x1 = first->dev_stc - delta_stc;
>          x2 = last->dev_stc - delta_stc;
> 
> X1 and X2 are NOT scaled
> 
>          y1 = (first->dev_sof + 2048) << 16;
>          y2 = (last->dev_sof + 2048) << 16;
>          if (y2 < y1)
>                  y2 += 2048 << 16;
> 
> Y1 and Y2 is scaled 16 (ignore the +2048, the variable is mod(2048))
> 
>          y = (u64)(y2 - y1) * (1ULL << 31) + (u64)y1 * (u64)x2
>            - (u64)y2 * (u64)x1;
>          y = div_u64(y, x2 - x1);
> 
> y = (SCALE16 - SCALE16)*K + SCALE16*SCALE1 - SCALE16*SCALE1; => Result
> is SCALE16
> y = div64(SCALE16, SCALE1) => Result is SCALE16
> 
> So it looks good to me. It is a painful code, but I think it is correct.

Ok, thank you for explaining this.

Regardless of the math which true me off, the actual sampling logic
is not that complicated.

Now that I realize that the exact sample moments do not matter because
buf->pts is our time-reference for the frame and not start->host_time
as my naive first reading of the code suggested this patch seems fine:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans