Weird default vblank value in ov5693

Weird default vblank value in ov5693

[Hans de Goede]

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.

The vblank and hblank control values changes on setting a mode
is unavoidable but the framerate and exposure value changing
at the same time seems undesirable.

Note that this also halves the max supported exposure value
when going to a lower-res mode even when userspace never
touches the vblank control.

Regards,

Hans

Re: Weird default vblank value in ov5693

[Jacopo Mondi]

Hi Hans

On Sun, Jan 28, 2024 at 09:58:41PM +0100, 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.
>
> The vblank and hblank control values changes on setting a mode
> is unavoidable but the framerate and exposure value changing
> at the same time seems undesirable.
>
> Note that this also halves the max supported exposure value
> when going to a lower-res mode even when userspace never
> touches the vblank control.
>

Would you be interested in reviewing
https://patchwork.kernel.org/project/linux-media/list/?series=697777&state=%2A&archive=both

and discuss there what the desired behaviour should be ?


> Regards,
>
> Hans
>
>
>
>
>

Re: Weird default vblank value in ov5693

[Hans de Goede]

Hi,

On 1/29/24 09:58, Jacopo Mondi wrote:
> Hi Hans
> 
> On Sun, Jan 28, 2024 at 09:58:41PM +0100, 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.
>>
>> The vblank and hblank control values changes on setting a mode
>> is unavoidable but the framerate and exposure value changing
>> at the same time seems undesirable.
>>
>> Note that this also halves the max supported exposure value
>> when going to a lower-res mode even when userspace never
>> touches the vblank control.
>>
> 
> Would you be interested in reviewing
> https://patchwork.kernel.org/project/linux-media/list/?series=697777&state=%2A&archive=both
> 
> and discuss there what the desired behaviour should be ?

I'm definitely interested in discussing this and in
helping with reviewing the documentation patches,
good timing wrt writing those patches :)

but Dan already replied in this thread, so lets
keep the discussion in this thread for now ?

Also I think this will probably make a good agenda
item for the libcamera workshop coming Friday ?

Hopefully we can make some decisions about this there
and then I can help with reviewing revised documentation
patches after that.

Regards,

Hans

Re: Weird default vblank value in ov5693

[Kieran Bingham]

Quoting Hans de Goede (2024-01-29 10:43:33)
> Hi,
> > 
> > Would you be interested in reviewing
> > https://patchwork.kernel.org/project/linux-media/list/?series=697777&state=%2A&archive=both

> Documentation: media: camera_sensor: Document blankings handling

> > 
> > and discuss there what the desired behaviour should be ?
> 
> I'm definitely interested in discussing this and in
> helping with reviewing the documentation patches,
> good timing wrt writing those patches :)
> 
> but Dan already replied in this thread, so lets
> keep the discussion in this thread for now ?
> 
> Also I think this will probably make a good agenda
> item for the libcamera workshop coming Friday ?

Added to the existing Sensor Modes topic that was added by Nicolas

--
Kieran

Re: Weird default vblank value in ov5693

[Dan Scally]

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?


Thanks

Dan

>
> The vblank and hblank control values changes on setting a mode
> is unavoidable but the framerate and exposure value changing
> at the same time seems undesirable.
>
> Note that this also halves the max supported exposure value
> when going to a lower-res mode even when userspace never
> touches the vblank control.
>
> Regards,
>
> Hans
>
>
>
>
>

Re: Weird default vblank value in ov5693

[Hans de Goede]

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)

Regards,

Hans


p.s.

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?
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) .




*) As Jacopo's doc patches mention the vblank range needs to be
updated when changing the mode. Which means calling
__v4l2_ctrl_modify_range() now this will clamp vblank to the new
range, potentially changing it leading to a __v4l2_ctrl_s_ctrl()
call under the hood.

We need to do this __v4l2_ctrl_modify_range() before actually
calling __v4l2_ctrl_s_ctrl() to set the new vblank value
(the new value calculated to keep vts the same). Otherwise
the new value may be out of range and we must not directly
poke the v4l2-ctrl internals to set a new in range value before
calling __v4l2_ctrl_modify_range(). So this lead to multiple
control-change events being emitted to userspace. But this
is unavoidable even with more KISS approaches.

Also when vts changes we also need to ensure that the exposure
range is corrected. Theoretically it is possible for vblank
to stay unchanged (e.g. changed from minimum vblank to minimum
vblank) so we cannot rely on s_ctrl to update the exposure range.

Note updating the exposure range twice is not a big deal since
__v4l2_ctrl_modify_range() checks if things actually change
and otherwise it is a no-op.






> 
> 
> Thanks
> 
> Dan
> 
>>
>> The vblank and hblank control values changes on setting a mode
>> is unavoidable but the framerate and exposure value changing
>> at the same time seems undesirable.
>>
>> Note that this also halves the max supported exposure value
>> when going to a lower-res mode even when userspace never
>> touches the vblank control.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>
> 

Re: Re: Weird default vblank value in ov5693

[Jacopo Mondi]

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.

>
> Regards,
>
> Hans
>
>
> p.s.
>
> 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 ?

>
>
> *) As Jacopo's doc patches mention the vblank range needs to be
> updated when changing the mode. Which means calling
> __v4l2_ctrl_modify_range() now this will clamp vblank to the new
> range, potentially changing it leading to a __v4l2_ctrl_s_ctrl()
> call under the hood.
>
> We need to do this __v4l2_ctrl_modify_range() before actually
> calling __v4l2_ctrl_s_ctrl() to set the new vblank value
> (the new value calculated to keep vts the same). Otherwise
> the new value may be out of range and we must not directly
> poke the v4l2-ctrl internals to set a new in range value before
> calling __v4l2_ctrl_modify_range(). So this lead to multiple
> control-change events being emitted to userspace. But this
> is unavoidable even with more KISS approaches.
>
> Also when vts changes we also need to ensure that the exposure
> range is corrected. Theoretically it is possible for vblank
> to stay unchanged (e.g. changed from minimum vblank to minimum
> vblank) so we cannot rely on s_ctrl to update the exposure range.
>
> Note updating the exposure range twice is not a big deal since
> __v4l2_ctrl_modify_range() checks if things actually change
> and otherwise it is a no-op.
>
>
>
>
>
>
> >
> >
> > Thanks
> >
> > Dan
> >
> >>
> >> The vblank and hblank control values changes on setting a mode
> >> is unavoidable but the framerate and exposure value changing
> >> at the same time seems undesirable.
> >>
> >> Note that this also halves the max supported exposure value
> >> when going to a lower-res mode even when userspace never
> >> touches the vblank control.
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >>
> >>
> >>
> >
>

Re: Weird default vblank value in ov5693

[Hans de Goede]

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

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

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

Re: Weird default vblank value in ov5693

[Dan Scally]

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

Re: Re: Weird default vblank value in ov5693

[Jacopo Mondi]

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

True that. The same could be said for controls initialization though,
where I suspect every driver does a different thing and there's no
standard behaviour

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

Maybe we're getting too concerned on this, as if an application sets a
new mode, it's likely setting new blankings and exposure as well..

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

Well, a RO vblank wouldn't hurt :)

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

This I agree with!

> Regards,
>
> Hans
>
>

Re: Weird default vblank value in ov5693

[Hans de Goede]

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

> Maybe we're getting too concerned on this, as if an application sets a
> new mode, it's likely setting new blankings and exposure as well..

ATM libcamera sets vblank to whatever default the sensor driver
advertises and not all pipelines change it after that, so IMHO we
need to have a somewhat sane default (and we probably want
libcamera pipelines to do a bit better, esp. with increasing
vblank to allow higher exposure in low light conditions).


> 
>>>> 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.
> 
> Well, a RO vblank wouldn't hurt :)

Yes and I guess that for sensors where we cannot explicitly
control vblank we would need a RO vblank to satisfy libcamera's
sensor requirements.

Regards,

Hans

Re: Weird default vblank value in ov5693

[Sakari Ailus]

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.

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.

> 
> > Maybe we're getting too concerned on this, as if an application sets a
> > new mode, it's likely setting new blankings and exposure as well..
> 
> ATM libcamera sets vblank to whatever default the sensor driver
> advertises and not all pipelines change it after that, so IMHO we
> need to have a somewhat sane default (and we probably want
> libcamera pipelines to do a bit better, esp. with increasing
> vblank to allow higher exposure in low light conditions).

It should be easy to calculate the right value, given the necessary
information. This is related to the needs of improving the sensor APIs for
register list based drivers.

> 
> 
> > 
> >>>> 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.
> > 
> > Well, a RO vblank wouldn't hurt :)
> 
> Yes and I guess that for sensors where we cannot explicitly
> control vblank we would need a RO vblank to satisfy libcamera's
> sensor requirements.

Vertical blanking is virtually always controllable, horizontal blanking not
nearly as often.

-- 
Regards,

Sakari Ailus

Re: Weird default vblank value in ov5693

[Hans de Goede]

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.

Where as for userspace implementations which already set vblank to
their own value the default does not matter.

>>> Maybe we're getting too concerned on this, as if an application sets a
>>> new mode, it's likely setting new blankings and exposure as well..
>>
>> ATM libcamera sets vblank to whatever default the sensor driver
>> advertises and not all pipelines change it after that, so IMHO we
>> need to have a somewhat sane default (and we probably want
>> libcamera pipelines to do a bit better, esp. with increasing
>> vblank to allow higher exposure in low light conditions).
> 
> It should be easy to calculate the right value, given the necessary
> information. This is related to the needs of improving the sensor APIs for
> register list based drivers.

IMHO it is important to where possible not change the behavior
of register list based drivers when adding improvements like
vblank control. As I said above these often use a fixed vts
for modes, the idea is to set a default vblank value so that
the vts does not change compared to before the introduction
of the vblank control.

This is just about the default value after a set_mode pad-op
call. Vblank aware userspace will likely override that anyway.

Regards,

Hans

Re: Weird default vblank value in ov5693

[Sakari Ailus]

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.

> 
> Where as for userspace implementations which already set vblank to
> their own value the default does not matter.
> 
> >>> Maybe we're getting too concerned on this, as if an application sets a
> >>> new mode, it's likely setting new blankings and exposure as well..
> >>
> >> ATM libcamera sets vblank to whatever default the sensor driver
> >> advertises and not all pipelines change it after that, so IMHO we
> >> need to have a somewhat sane default (and we probably want
> >> libcamera pipelines to do a bit better, esp. with increasing
> >> vblank to allow higher exposure in low light conditions).
> > 
> > It should be easy to calculate the right value, given the necessary
> > information. This is related to the needs of improving the sensor APIs for
> > register list based drivers.
> 
> IMHO it is important to where possible not change the behavior
> of register list based drivers when adding improvements like
> vblank control. As I said above these often use a fixed vts
> for modes, the idea is to set a default vblank value so that
> the vts does not change compared to before the introduction
> of the vblank control.

I agree. There are probably less than 20 such drivers, mostly old
OmniVision sensor drivers.

> 
> This is just about the default value after a set_mode pad-op
> call. Vblank aware userspace will likely override that anyway.

-- 
Regards,

Sakari Ailus

Re: Weird default vblank value in ov5693

[Hans de Goede]

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