Re: Weird default vblank value in ov5693

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 1/30/24 13:00, Sakari Ailus wrote:
> Hi Hans,
> 
> On Tue, Jan 30, 2024 at 11:29:20AM +0100, Hans de Goede wrote:
>> Hi Sakari,
>>
>> On 1/29/24 19:40, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Mon, Jan 29, 2024 at 06:18:08PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 1/29/24 15:45, Jacopo Mondi wrote:
>>>>> Hi Hans
>>>>>
>>>>> +dave, laurent and sakari
>>>>>
>>>>> On Mon, Jan 29, 2024 at 01:24:31PM +0100, Hans de Goede wrote:
>>>>>> Hi Jacopo,
>>>>>>
>>>>>> On 1/29/24 13:05, Jacopo Mondi wrote:
>>>>>>> Hi Hans
>>>>>>>
>>>>
>>>> <snip (getting too long)>
>>>>
>>>>>> 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.
>>>>>> """
>>>>>>
>>>>>
>>>>> Sorry but I'm not super excited about blessing 30fps as the
>>>>> preferred or suggested setting in the documentation. For some use
>>>>> cases 30fps might be extremely slow or extremely fast, if a sensor or
>>>>> a mode cannot achieve this we then suggest the minimum... not sure
>>>>> what's best. What's others opinion here ?
>>>>
>>>> I'm fine with loosing the 30 fps language. I was actually
>>>> already thinking about dropping specifying 30 fps myself.
>>>>
>>>> In the pending documentation patch you write:
>>>>
>>>> "The value used to initialize the vertical and horizontal blanking controls
>>>> should be selected in order to realize, in association with the driver default
>>>> format and default pixel rate, a reasonable frame rate output, usually one of
>>>> the standard 15, 30 or 60 frame per second."
>>>>
>>>> How about:
>>>>
>>>> "When a new frame size is applied on the subdevice, sensor drivers are required
>>>> to update the limits of their blankings controls.
>>>>
>>>> ... part about calling __v4l2_ctrl_modify_range()...
>>>>
>>>> The control's default value is adjusted to achieve a reasonable framerate
>>>> again and the control's value is set to the new default for consistent
>>>> behavior after applying a new frame size."
>>>>
>>>> ?
>>>>
>>>> This basically blesses the existing ov5693 driver's behavior :)
>>>
>>> What would be the purpose of this? Presumably the user space will set the
>>> vblank value based on its needs in any case, before starting streaming.
>>
>> As I mentioned currently libcamera's sensor class sets vblank to its
>> default value at initialization time and some pipelines simply leave
>> it there so having a somewhat sane default is important to not have
>> a very high fps / have low max exposure for modes with a low height.
>>
>>> It would require changing many that currently don't have this. Changing
>>> this could also adversely affect some user space software but presumably is
>>> unlikely to break it.
>>
>> This is mostly to have clear guidelines for when adding vblank support
>> to existing drivers without vblank support.
>>
>> Existing drivers often have a fixed vts value independend of the mode /
>> amount of cropping so that they always run at a fixed fps.
>>
>> Ideally we would not change the behavior of these drivers when adding
>> vblank control. Having these drivers pick a default vblank value
>> (when adjusting the range) so that the old fixed vts is again achieved
>> and then resetting vblank to that default value ensures that the behavior
>> of the driver does not change for userspace which does not touch vblank.
> 
> Agreed. The text should also say that, then, so new drivers wouldn't need
> to bother. The complexity of adding that is trivial only on entirely
> register list based drivers which we prefer to get rid of anyway,
> eventually.

So what should new drivers use as default vblank value? The min
supported value as in Jacopo's current
"[PATCH v2 1/2] documentation: media: camera_sensor: Document blankings handling"
patch ?

Regards,

Hans