Re: [PATCH 1/2] media: video-i2c: frame delay based on last frame's end time

[Mauro Carvalho Chehab <mchehab@kernel.org>]

Em Sat,  5 Jun 2021 20:54:56 +0900
Seongyong Park <euphoriccatface@gmail.com> escreveu:

> Current implementation calculates frame delay based on the time of
> start of the loop. This inevitably causes the loop delay to be
> slightly longer than the actual measurement period, thus skipping a frame
> every now and then.
> 
> However, MLX90640 should ideally be working without a frame skip.
> Each measurement step updates half of the pixels in the frame
> (every other pixel in default "chess mode", and every other row
> in "interleave mode"), while additional coefficient data (25th & 26th row)
> updates every step. The compensational coefficient data only corresponds
> with the pixels updated in the same step.
> 
> In short, if a frame is skipped, then half of a frame loses correction
> information and becomes garbage data.
> 
> Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
> ---
>  drivers/media/i2c/video-i2c.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 0465832a4..2ccb08335 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -445,14 +445,16 @@ static int video_i2c_thread_vid_cap(void *priv)
>  	struct video_i2c_data *data = priv;
>  	unsigned int delay = mult_frac(HZ, data->frame_interval.numerator,
>  				       data->frame_interval.denominator);
> +	unsigned long end_jiffies = jiffies;
>  
>  	set_freezable();
>  
>  	do {
> -		unsigned long start_jiffies = jiffies;
>  		struct video_i2c_buffer *vid_cap_buf = NULL;
>  		int schedule_delay;
>  
> +		end_jiffies += delay;
> +
>  		try_to_freeze();
>  
>  		spin_lock(&data->slock);
> @@ -477,10 +479,9 @@ static int video_i2c_thread_vid_cap(void *priv)
>  				VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
>  		}
>  
> -		schedule_delay = delay - (jiffies - start_jiffies);
> -
> -		if (time_after(jiffies, start_jiffies + delay))
> -			schedule_delay = delay;

> +		schedule_delay = end_jiffies - jiffies;
> +		while (schedule_delay <= 0)
> +			schedule_delay += delay;

Huh? Why do you need a loop for that? If you want to make it positive,
you could just do:

	if (schedule_delay < 0)
		schedule_delay = delay; /* or something else */

but this won't solve the issue, as this is basically equivalent
to the logic you removed.

>  
>  		schedule_timeout_interruptible(schedule_delay);

This is probably what's causing troubles, as this only ensures
that it will sleep for at least schedule_delay (if not
interrupted).

If you want to give a dead line to schedule, you should
likely be doing, instead:

		usleep_range(delay, delay + delta);

this would tell the realtime clock that there's a dead line of
(schedule_delay + delta) microseconds.

You'll need to change the delay to be in microseconds too,
e. g., I guess that something similar to this:


    static int video_i2c_thread_vid_cap(void *priv)
    {
        struct video_i2c_data *data = priv;
	u64 delay;

	delay = div64_u64(1000000ULL * data->frame_interval.numerator,
			  data->frame_interval.denominator);

	set_freezable();

        do {
...
		usleep_range(delay * 3 / 4, delay);
	} while (!kthread_should_stop());

	return 0;
    }

would give you what you want.

Regards,
Mauro