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

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

Em Sun, 6 Jun 2021 16:20:53 +0900
Seongyong Park <euphoriccatface@gmail.com> escreveu:

> 2021년 6월 5일 (토) 오후 11:53, Mauro Carvalho Chehab <mchehab@kernel.org>님이 작성:
> > 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  
> 
> Okay, I have tried `usleep_range()` instead, and it indeed shows
> improvement in the frame rate.
> Now it's basically the same as before my patch, except for
> `jiffies_to_usecs()` and then `usleep_range()`.
> 
> ...
>          int schedule_delay;
> +        uint64_t schedule_delay_us;
> 
>         try_to_freeze();
> ...
>         if (time_after(jiffies, start_jiffies + delay))
>             schedule_delay = delay;
> 
> -        schedule_timeout_interruptible(schedule_delay);
> +        schedule_delay_us = jiffies_to_usecs(schedule_delay);
> +        usleep_range(schedule_delay_us * 3/4, schedule_delay_us);
>      } while (!kthread_should_stop());
> 
>      return 0;
> ...
> 
> I decided to keep the `if (...) schedule_delay = delay;` part.

Yeah, you would need something like that.

> The concern was that my RPi Zero was having quite a bit of constant
> drift, like showing 3FPS when set to 4FPS, 6FPS when 8FPS, 10FPS when
> 16FPS, and so on.
> Now that I've confirmed the timing's good enough (usually ~0.5 FPS
> faster than the frame rate given), there's no need for me to bother
> anymore.

In other to avoid the drift, the logic needs to calculate the delay based
on something like this (pseudo-C) code:

	start_jiffies = jiffies;
	frame_count = 0;
	i = 0;
	do {
		i++;
		delay = jiffies - (start_jiffies * i * <interval>);
...
	}

The actual logic should probably avoid multiplying stuff there, as this
could go sideways easily due to overflows.

Perhaps something like this would work better, keeping a more precise
average fps rate:

	next_jiffies = jiffies + delay;
	do {
...
		schedule_delay_us = jiffies_to_usecs(next_jiffies - jiffies);
		usleep_range(schedule_delay_us * 3/4, schedule_delay_us);		...
		next_jiffies += delay;
	}

> 
> I'll send another patchset if it doesn't look too bad.
> 
> Thank you very much.
> Seongyong Park



Thanks,
Mauro