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
