[PATCH/RFC 17/32] libcamera: camera_sensor: Test for read-only HBLANK with READ_ONLY flag

[Laurent Pinchart]

On Mon, Mar 04, 2024 at 06:41:03PM +0100, Jacopo Mondi wrote:
> Hi Laurent
> 
> On Fri, Mar 01, 2024 at 11:21:06PM +0200, Laurent Pinchart wrote:
> > The CameraSensor class tests if the sensor HBLANK control is read-only
> > by comparing the minimum and maximum values, and documents this as being
> > a workaround for the lack of a read-only control flag in V4L2. This is
> > incorrect, as the V4L2 API provides such a flag. Use it to replace the
> > workaround.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/sensor/camera_sensor.cpp | 27 +++++++-------------------
> >  1 file changed, 7 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
> > index 86ad9a85371c..402025566544 100644
> > --- a/src/libcamera/sensor/camera_sensor.cpp
> > +++ b/src/libcamera/sensor/camera_sensor.cpp
> > @@ -188,28 +188,15 @@ int CameraSensor::init()
> >  	 * Set HBLANK to the minimum to start with a well-defined line length,
> >  	 * allowing IPA modules that do not modify HBLANK to use the sensor
> >  	 * minimum line length in their calculations.
> > -	 *
> > -	 * At present, there is no way of knowing if a control is read-only.
> > -	 * As a workaround, assume that if the minimum and maximum values of
> > -	 * the V4L2_CID_HBLANK control are the same, it implies the control
> > -	 * is read-only.
> > -	 *
> > -	 * \todo The control API ought to have a flag to specify if a control
> > -	 * is read-only which could be used below.
> >  	 */
> 
> Weird, I'm sure V4L2_CTRL_FLAG_READ_ONLY was there in 2022 when this
> has been introduced. Maybe the API we had to check flags was (or is)
> not the best one and we decided to compare values ?

It puzzled me too, I really don't recall why we didn't use it. Naush,
does it ring a bell ?

> > -	if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) {
> > -		const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);
> > -		const int32_t hblankMin = hblank.min().get<int32_t>();
> > -		const int32_t hblankMax = hblank.max().get<int32_t>();
> > +	const struct v4l2_query_ext_ctrl *hblankInfo = subdev_->controlInfo(V4L2_CID_HBLANK);
> > +	if (hblankInfo && !(hblankInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) {
> > +		ControlList ctrl(subdev_->controls());
> 
> This could be fast-tracked too!
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> 
> > -		if (hblankMin != hblankMax) {
> > -			ControlList ctrl(subdev_->controls());
> > -
> > -			ctrl.set(V4L2_CID_HBLANK, hblankMin);
> > -			ret = subdev_->setControls(&ctrl);
> > -			if (ret)
> > -				return ret;
> > -		}
> > +		ctrl.set(V4L2_CID_HBLANK, static_cast<int32_t>(hblankInfo->minimum));
> > +		ret = subdev_->setControls(&ctrl);
> > +		if (ret)
> > +			return ret;
> >  	}
> >
> >  	return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);

-- 
Regards,

Laurent Pinchart