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

From: Hans Verkuil
Date: Mon Apr 22 2024 - 08:17:16 EST


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,