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

[Jacopo Mondi]

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 ?

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