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@xxxxxxxxxxxxxxx
>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
>> ---
>>  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;
>>  }
>>
>