Em Sat, 5 Jun 2021 16:00:28 +0200
Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> escreveu:
> Em Sat, 5 Jun 2021 20:54:56 +0900
> Seongyong Park <euphoriccatface@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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.
The actual the logic should be more complex, as it needs to
dynamically adjust the delay based on how much frames were streamed
and how much time it was elapsed, but the basically idea is that
you would need to use:
usleep_range(min_delay_us, max_delay_us);
instead of:
schedule_timeout_interruptible(schedule_delay);
in order to tell the realtime clock about a dead line for
sleeping.
Thanks,
Mauro
