Re: Weird default vblank value in ov5693

[Dan Scally <dan.scally@ideasonboard.com>]

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
>
>