[libcamera-devel] [Camera sensor drivers] Resetting V4L2 controls after a mode change

[Jacopo Mondi]

On Wed, Oct 19, 2022 at 08:58:01AM +0200, Jacopo Mondi via libcamera-devel wrote:
> Hi Sakari,
>
> On Wed, Oct 19, 2022 at 06:43:28AM +0000, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Tue, Oct 18, 2022 at 12:00:05PM +0200, Jacopo Mondi wrote:
> > > Hello,
> > >    I'm breaking out a discussion point from a review comment I
> > > received from Dave on the following patch
> > > https://lore.kernel.org/linux-media/20221005190613.394277-1-jacopo@jmondi.org/T/#m3308616247d646ebecad89c158a622de35f1cce3
> > > to collect opinions and possibily define what is the the expected behaviour
> > > for sensor drivers.
> > >
> > > The controls I'm here considering are H/VBLANK and EXPOSURE, but the
> > > question might generally apply to other controls as well.
> > >
> > > To summarize the issue:
> > >
> > > When a new mode is applied the blanking controls limits (min/max)
> > > have to be updated to reflect the new configuration.
> > >
> > > In example
> > >
> > >        max_hblank = AR0521_TOTAL_WIDTH_MAX - sensor->fmt.width;
> > >        __v4l2_ctrl_modify_range(sensor->ctrls.hblank,
> > >                                 sensor->ctrls.hblank->minimum,
> > >                                 max_hblank, sensor->ctrls.hblank->step,
> > >                                 sensor->ctrls.hblank->minimum);
> > >
> > >        max_vblank = AR0521_TOTAL_HEIGHT_MAX - sensor->fmt.height;
> > >        __v4l2_ctrl_modify_range(sensor->ctrls.vblank,
> > >                                 sensor->ctrls.vblank->minimum,
> > >                                 max_vblank, sensor->ctrls.vblank->step,
> > >                                 sensor->ctrls.vblank->minimum);
> > >
> > > The questions I would like to clarify, and possibily define a standard
> > > behaviour that userspace can relay on, are:
> > >
> > > 1) When a new format is applied to the sensor, should the control value
> > >    and its default value be updated as well to maintain the same (or as close
> > >    as possible) configuration as the initial one, or should they be left
> > >    unchanged (apart for clamping them in the new limits, which is
> > >    automatically done by the v4l2-controls framework) ?
> > >
> > > 2) Can userspace rely on the control default value (set at initialization
> > >    time and after a mode change) to represent a sensible configuration, in
> > >    example a tested and known-to-work framerate ?
> >
> > What exactly do you mean by a "sensible configuration"?
> >
>
> In the context of blankings, a tested and reasonable FPS (30FPS,
> 60FPS, not 26.45 FPS).
>
> > >
> > > There are two main arguments in favour or againt resetting controls
> > > when a new mode is applied. The obvious one in favour is that when a
> > > new mode is applied, userspace can rely on a default configuration
> > > that is reasonable and known to work and the control default value
> > > can be used a reference if the control is later modified during
> > > streaming. Also, not resetting controls can leave the sensor driver in
> > > an unspecified (but hopefully valid) state (see Laurent's example in [1])
> >
> > The control values should remain valid and this needs to be ensured by the
> > driver.
>
> Doesn't the v4l2-ctrl framework clamp the control value in the new
> ranges in __v4l2_ctrl_modify_range() ?
>
> >
> > E.g. the CCS driver just changes the range (as above) as it is oblivious to
> > modes.
> >
> > I don't object resetting the values to defaults (or changing defaults) in
> > mode based drivers *if* that's helpful for the user space. In general
> > controls and other sensor configurations should be independent but
> > obviously when it comes to e.g. the blanking controls, there is a
> > dependency.
> >
>
> The fact is that "I don't object" either, but I think we need
> guidelines here. I understand the issue here is what a "sensible
> configuration" mean. Please note this is now well defined neither at

I meant "not well defined", sorry, pointing it out as it changes the
meaning of the statement..

> initialization time when picking a default value to initialize the
> control with. But as I assume the default, for blankings at least,
> corresponds to some tested configuration, I think we should require
> sensor driver to reset the control to a value that realize the same
> configuration selected at initialization time.
>
> > >
> > > This mostly benefits 'generic' userspace which is not interested in
> > > modifying blankings, but also provide 'specialized' userspace a
> > > reference to compute a default configuration which is known to work.
> > >
> > > However, on the other hand, such a behaviour would prevent (or make
> > > harder) concurrent access to the camera from different applications.
> > > The most trivial example is a camera viewer that does not control
> > > blankings, which are side-configured by the user through another
> > > application (say, v4l2-ctl). In this case, when a mode is set on the
> > > sensor the blanking values are reset, overwriting the manually applied
> > > configuration. This is a common use case when developing, when
> > > a configuration is forced on the sensor to validate it, but it's a valid use
> > > case in general, as a mode configuration would then actually
> > > overwrite other parameters that might be considered "unrelated" to the
> > > output resolution and format.
> > >
> > > Once we establish an expected behaviour, should we try to document it
> > > and possibly enforce it in sensor drivers through reviews and
> > > hopefully by providing helpers ? Whatever the decision ends up being
> > > I think we want to standardize sensor drivers to behave the same.
> > >
> > > Thanks
> > >    j
> > >
> > > [1] https://lists.libcamera.org/pipermail/libcamera-devel/2022-October/034885.html
> > > " Consider a case with two modes, lores and hires. As
> > > sensors typically express horizontal blanking as a horizontal total
> > > size, the hblank control max value will be larger for lores than hires.
> > > If the sensor were to set the default hblank to a value valid for lores
> > > but not hires, switching from lores -> hires -> lores would change the
> > > effective hblank value even if userspace doesn't touch the hblank
> > > control at all."
> >
> > --
> > Kind regards,
> >
> > Sakari Ailus