[libcamera-devel] [PATCH v2 1/1] libcamera: camera_sensor: Reset HBLANK when setting the format

[Jacopo Mondi]

Hi David

On Wed, Nov 23, 2022 at 11:36:44AM +0000, David Plowman via libcamera-devel wrote:
> We no longer reset the HBLANK in init because we may not own the
> camera and the operation may fail. Instead, we reset it when we set
> the camera format so that all pipeline handlers, even ones that do not
> control HBLANK themselves, will have a well-defined value.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/libcamera/camera_sensor.cpp | 53 ++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 572a313a..ec7bb0e7 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -176,32 +176,6 @@ int CameraSensor::init()
>  	if (ret)
>  		return ret;
>
> -	/*
> -	 * 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.
> -	 */
> -	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>();
> -
> -	if (hblankMin != hblankMax) {
> -		ControlList ctrl(subdev_->controls());
> -
> -		ctrl.set(V4L2_CID_HBLANK, hblankMin);
> -		ret = subdev_->setControls(&ctrl);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);
>  }
>
> @@ -748,7 +722,32 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
>  		return ret;
>
>  	updateControlInfo();

I would do that -before- updating the controls info.

> -	return 0;
> +
> +	/*
> +	 * 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.

I know it was already like this, but can't we use

const struct v4l2_query_ext_ctrl *V4L2Device::controlInfo(uint32_t id) const

and inspect the flags ?

> +	 * 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.
> +	 */
> +	const ControlInfo hblank = controls().at(V4L2_CID_HBLANK);
> +	const int32_t hblankMin = hblank.min().get<int32_t>();
> +	const int32_t hblankMax = hblank.max().get<int32_t>();
> +
> +	if (hblankMin != hblankMax) {
> +		ControlList ctrl(subdev_->controls());
> +
> +		ctrl.set(V4L2_CID_HBLANK, hblankMin);
> +		ret = subdev_->setControls(&ctrl);
> +	}
> +
> +	return ret;
>  }

I could take this one on top of the flip series maybe, if you agree
with the above changes ?

Thanks
  j

>
>  /**
> --
> 2.30.2
>