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

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

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