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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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