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

Ricardo Ribalda posted 1 patch 2 weeks ago
drivers/media/test-drivers/vivid/vivid-kthread-cap.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
[PATCH] media: vivid: Enforce a monotonic sequence number
Posted by Ricardo Ribalda 2 weeks ago
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@chromium.org>
---
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;

---
base-commit: 836e2548524d2dfcb5acaf3be78f203b6b4bde6f
change-id: 20240417-vivid-seq-d0eb044a085f

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>
Re: [PATCH] media: vivid: Enforce a monotonic sequence number
Posted by Hans Verkuil 1 week, 2 days ago
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@chromium.org>
> ---
> 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,