[libcamera-devel] [PATCH] libcamera: camera_sensor: only access V4L_CID_HBLANK if existing

[Jacopo Mondi]

Hi Alain

On Mon, Nov 13, 2023 at 11:08:51AM +0100, Alain Volmat via libcamera-devel wrote:
> Correct a crash in CameraSensor::init when trying to set the
> V4L2_CID_HBLANK control on sensor not implementing this control.
> The HBLANK sensor not being mandatory for non-RAW sensors, it can
> happen that the sensor does not expose this control.
> Perform check against availability of the control prior to usage in
> order to avoid the crash.
>
> Signed-off-by: Alain Volmat <alain.volmat at foss.st.com>
> ---
>  src/libcamera/camera_sensor.cpp | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index d9261672..0ef78d9c 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -197,17 +197,19 @@ int CameraSensor::init()
>  	 * \todo The control API ought to have a flag to specify if a control
>  	 * is read-only which could be used below.
>  	 */

Actually the above comment suggest that there's no way to know if a
control is RO, while I see

	if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) &&
	    vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY))

in other parts of the class.

This is unrelated to this change though

> -	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;
> +	if (ctrls.infoMap()->find(V4L2_CID_HBLANK) != ctrls.infoMap()->end()) {

Most YUV/RGB sensor drivers I know of do not support HBLANK, so I
would have expected this issue to be hit earlier than this. Possibly
libcamera is not very populare with YUV/RGB sensor :)

> +		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;
> +		}

The patch looks good to me
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

>  	}
>
>  	return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);
> --
> 2.25.1
>