Re: [PATCH] media: vivid: Enforce a monotonic sequence number

[Hans Verkuil]

On 17/04/2024 11:30, Ricardo Ribalda wrote:
> Make sure that the sequence number is at least one unit bigger than the
> previous one.
> 
> This solves situations where the sequence number calculation does not
> have enough accuracy and repeats a sequence number.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
> We have seen some examples where the sequence number has been
> duplicated. This should not happen.
> ---
>  drivers/media/test-drivers/vivid/vivid-kthread-cap.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vivid/vivid-kthread-cap.c b/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
> index 42048727d7ff..6375e6484a4b 100644
> --- a/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
> +++ b/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
> @@ -665,7 +665,7 @@ static int vivid_thread_vid_cap(void *data)
>  {
>  	struct vivid_dev *dev = data;
>  	u64 numerators_since_start;
> -	u64 buffers_since_start;
> +	u64 buffers_since_start = ~0;
>  	u64 next_jiffies_since_start;
>  	unsigned long jiffies_since_start;
>  	unsigned long cur_jiffies;
> @@ -691,6 +691,7 @@ static int vivid_thread_vid_cap(void *data)
>  	vivid_cap_update_frame_period(dev);
>  
>  	for (;;) {
> +		u64 tmp;
>  		try_to_freeze();
>  		if (kthread_should_stop())
>  			break;
> @@ -719,9 +720,10 @@ static int vivid_thread_vid_cap(void *data)
>  		/* Calculate the number of jiffies since we started streaming */
>  		jiffies_since_start = cur_jiffies - dev->jiffies_vid_cap;
>  		/* Get the number of buffers streamed since the start */
> -		buffers_since_start = (u64)jiffies_since_start * denominator +
> -				      (HZ * numerator) / 2;
> -		do_div(buffers_since_start, HZ * numerator);
> +		tmp = (u64)jiffies_since_start * denominator +
> +		      (HZ * numerator) / 2;
> +		do_div(tmp, HZ * numerator);
> +		buffers_since_start = max(tmp, buffers_since_start + 1);
>  
>  		/*
>  		 * After more than 0xf0000000 (rounded down to a multiple of
> @@ -746,7 +748,7 @@ static int vivid_thread_vid_cap(void *data)
>  		 * Calculate the number of 'numerators' streamed since we started,
>  		 * including the current buffer.
>  		 */
> -		numerators_since_start = ++buffers_since_start * numerator;
> +		numerators_since_start = (buffers_since_start + 1) * numerator;
>  
>  		/* And the number of jiffies since we started */
>  		jiffies_since_start = jiffies - dev->jiffies_vid_cap;
> 

Just FYI: this patch isn't right: it's fixing the symptom, not the cause.
I am working on a better patch for this. It's the same for the v4l2-compliance
patch.

So I'll reject these two patches in patchwork.

Regards,

	Hans

> ---
> base-commit: 836e2548524d2dfcb5acaf3be78f203b6b4bde6f
> change-id: 20240417-vivid-seq-d0eb044a085f
> 
> Best regards,