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

[Jacopo Mondi]

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