Hello
On 29/01/2024 12:24, Hans de Goede wrote:
Hi Jacopo,
On 1/29/24 13:05, Jacopo Mondi wrote:
Hi Hans
On Mon, Jan 29, 2024 at 11:30:33AM +0100, Hans de Goede wrote:
Hi Dan,
On 1/29/24 10:53, Dan Scally wrote:
Morning Hans
On 28/01/2024 20:58, Hans de Goede wrote:
Hi All,
While adding vblank ctrl support to the ov2680 driver I was looking
at the vblank ctrl code in the ov5693 and I noticed something
which I believe is weird / undesirable.
When setting a new mode the ov5693 driver does not keep the current
vts value (adjusting vblank to keep vts and thus the framerare and
exposure unchanged).
Instead it calculates a new vts value, resetting the framerate
to 30 fps; or 60 fps for smaller resolutions and then sets
vblank to the new (vts - new_mode->height) and adjusts
the exposure control-range to fit within the new vts, potentially
also changing the exposure value.
This behavior of the ov5693 code means that switching resolution
also changes the framerate and the exposure value which seems
undesirable.
I think I did it that way because I was hitting problems when changing the framesize exceeded the current VTS and it seemed cleaner to just reset it to a known situation. Really though the only thing it would affect would be the framerate; that would have to reduce if the VTS increased but exposure could stay the same (though the maximum would change). So probably it ought to work like:
* if we change from a larger to a smaller framesize then we can just increase blanking to keep the same VTS and that should be fine
* if we're going from a smaller to a larger framesize that fits within the currently configured VTS with minimum blanking, we can just reduce the blanking to keep the same VTS and that should be fine
* if we're going from a smaller to a larger framesize that exceeds the currently configured VTS we drop blanking to a minimum so that the new framerate is as close to the old one as it can be
Does that sound like more reasonable behaviour? If so, shall I patch it?
This sounds more or like what I had in mind (keep VTS unchanged if possible),
so I have been looking more into this yesterday evening and
implementing this is a bit tricky (*).
Combining this with your last point of "that the new framerate is as
close to the old one as it can be" I think I prefer a more KISS
approach.
IMHO the best thing (principle of least surprise) would be to
on a set_fmt pad-op reset things to a default fps of say 30,
as Jacopo's doc patches suggest. My reasons for suggesting
this approach is:
a) This is easier to implement and thus less likely to have bugs
b) It leads to consistent behavior where as your suggested try to
keep current vts approach leads to sometimes vts being kept, but
other times when going from smaller to higher resolutions vts
changing which will lead to hard to debug problems if userspace
relies on vts being kept.
For the ov5693 driver this would mean dropping __ov5693_calc_vts()
replacing it with a DEFAULT_VTS define of:
ALIGN_DOWN(OV5693_PIXEL_RATE / OV5693_FIXED_PPL / 30, 2)
(does vts need to be a multiple of 2? We don't enforce that
in the vblank control)
Alternatively, we can reset blankings to their minimum. This is
'predictable' but the end result (in example, possible higher fps)
might surprise applications. Please note the same reasoning applies
when using a vblank value that gives a known FPS as an application
running at 5fps might get 30fps after a set_fmt.
The difference between the two approaches (min-blank vs.
known-fps-blank) is that sensors provide different FPS at different
resolutions, with full resolution modes sometime being limited to 5
fps or less, making difficult to define what the "standard fps" is for
all configurations.
Good point about not all sensors being able to do 30 fps
at their highest resolution.
I'm not a fan of min-vblank as vblank default value since
with low-height resolutions this will significantly limit
the maximum exposure time.
OTOH I do believe that we want a simple default for vblank which gets
set on every set_mode call to keep things KISS.
How about something like this: (based on your doc patch):
"""
The vblank control default value is reset so that the sensor runs
at 30 fps. Except when 30 fps cannot be achieved, in that case
the vblank control default value is reset to the control's minimum.
After adjusting the range, the vblank control's value must be set to its
new default value for consistent behavior after applying a new frame size.
"""
In later code meant to achieve the same thing I've fallen back on the minimum vblank if it can't
reach some pre-defined value (though I chose 15fps IIRC) - so that seems fine to me.
What about enum/get/set frame_interval vs set_mode vs
vblank ctrl ? Options:
a) Sensor drivers MUST not implement enum/get/set frame_interval?
Ideally they shouldn't, for RAW sensors at least.
For YUV/RGB sensors instead the high-level parameters used by
frame_interval might be ok as some of those sensors might not even
allow you to control blankings explicitly.
Whenever the hardware allows that, blankings should always be
preferred over frame_interval imho.
b) enum/get/set frame_interval only enum/get/set the default
frame_interval set by set_mode (e.g. fixed 30 fps).
Any vblank changes made after the set_mode are not reflected
by get_frame_interval and set_frame_interval only influences
the next set_mode call, not the current mode ? Or maybe
only allow set_frame_interval when not streaming and then
have it set vblank to match the interval like it would
have done when called before the last set_mode call ?
c) enum/get/set frame_interval are a second way to control
hts (lets not go there just here for completeness sake)
My own preference here would be to go with a) .
Mine as well, but as said for YUV/RGB sensors it might not even be
possible to control blankings explicitly. In this case
set_frame_interval can be used but if the driver registers the vblank
control the newly computed blanking value (in response to a
set_frame_interval) should be reflected there ?
IMHO if the driver registers the vblank control then it *must not*
implement enum/get/set frame_interval . Trying to support both at
the same time is just going to cause pain.
Agreed
Assuming a driver implementing vblank also implements hblank
and pixelrate controls (we can make that mandatory I guess?)
then there is no need for enum/get/set frame_interval, since
userspace can then fully query / control the framerate
through those controls + the frame size.
Regards,
Hans
