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

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

Em Mon, 7 Jun 2021 00:06:36 +0900
Seongyong Park <euphoriccatface@gmail.com> escreveu:

> `2021년 6월 6일 (일) 오후 8:00, Mauro Carvalho Chehab <mchehab@kernel.org>님이 작성:
> >
> >
> > 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  
> 
> For a few hours, I couldn't get the module to make precise timing.
> It would always be a few tenths of FPS higher than I set, regardless
> of how I construct the calculation.
> And then it hit me, maybe jiffies is not granular enough - and of
> course, resolution of jiffies turns out to be 100Hz :P

It is actually worse than that, as it depends on CONFIG_HZ, which is
set by the one who built the Kernel. So, on other machines, it could
be higher (like 1200) or lower (24 is one of the options on mips) ;-)

> e.g. If you set it 16FPS, the count of jiffies to sleep results 100 / 16 = 6
> The sleep interval ends up being 60ms, resulting in 16.666... FPS.

That's basically why I suggested you to count the time from the start
of streaming, instead of just waiting for a fixed delay every time.

> This discrepancy gets quite big if you set it to 64 FPS, resulting in
> a single jiffies count, i.e. 100Hz.
> (Though you will need to either skip some data, or set I2C baud rate
> beyond the sensor's spec
> in order to realistically even achieve 64 FPS, at least on an RPi Zero
> it seems.)

The issue is probably not RPi zero, but the low speeds of I2C bus,
and the speed of the sensor. 

> So, I basically changed the time source from `jiffies` to
> `ktime_to_us(ktime_get())`.
> The timing is definitely better now :)

This helps but it probably use a lot more CPU cycles than reading
jiffies.

> One last question please, before creating another patchset.
> >                 usleep_range(schedule_delay_us * 3/4, schedule_delay_us);  
> Is it okay to make it essentially 0 microseconds sleep here, by
> setting `schedule_delay_us` to 0?
> It's going to be like,
> ```
> if (current_us > end_us) {
>     end_us = current_us;
> }
> schedule_delay_us = end_us - current_us;
> ```

Not sure, but you could instead revert the logic, like:

if (current_us <= end_us)
	usleep_range();

Thanks,
Mauro