On Tue, Jun 8, 2021 at 8:25 AM Seongyong Park <euphoriccatface@xxxxxxxxx> wrote:
>
> MLX90640 should ideally be working without a frame skip.
> In short, if a frame is skipped, then half of a frame loses correction
> information, having no way to retrieve its original compensation.
>
> This patch improves the timing in three ways:
>
> 1) Replaced schedule_timeout_interruptible() to usleep_range()
> The former "only ensures that it will sleep for at least
> schedule_delay (if not interrupted)", as pointed out by mchehab.
> As a result, the frame rate could lag behind than the actual capability
> of the hardware
> (Raspberry Pi would show a few Hz slower than set value)
>
> 2) Calculation based on us, not jiffies
> Jiffies usually has resolution of 100Hz, and possibly even cruder.
> MLX90640 can go up to 64Hz frame rate, which does not make sense to
> calculate the interval with aforementioned resolution.
>
> 3) Interval calculation based on the last frame's end time
> Using the start time of the current frame will probably make tiny bit
> of drift every time. This made more sense when I didn't realize 1),
> but it still makes sense without adding virtually any complexity,
> so this stays in.
>
> Signed-off-by: Seongyong Park <euphoriccatface@xxxxxxxxx>
> ---
> drivers/media/i2c/video-i2c.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 0465832a4..64ba96329 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -443,14 +443,15 @@ static void buffer_queue(struct vb2_buffer *vb)
> 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);
> + u32 delay = mult_frac(1000000UL, data->frame_interval.numerator,
> + data->frame_interval.denominator);
> + s64 end_us = ktime_to_us(ktime_get());
>
> set_freezable();
>
> do {
> - unsigned long start_jiffies = jiffies;
> struct video_i2c_buffer *vid_cap_buf = NULL;
> + s64 current_us;
> int schedule_delay;
>
> try_to_freeze();
> @@ -477,12 +478,14 @@ 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_timeout_interruptible(schedule_delay);
> + end_us += delay;
> + current_us = ktime_to_us(ktime_get());
> + if (current_us < end_us) {
> + schedule_delay = end_us - current_us;
> + usleep_range(schedule_delay * 3/4, schedule_delay);
Looks fine to me.. not sure I like the 3/4 but the only way to do it
without a floating point of 0.75 which
is evil and would kick off a compiler warning anyway.
Acked-by: Matt Ranostay <matt.ranostay@xxxxxxxxxxxx>
> + } else {
> + end_us = current_us;
> + }
> } while (!kthread_should_stop());
>
> return 0;
> --
> 2.31.1
>
