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@xxxxxxxxxx/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
