[libcamera-devel] [PATCH 3/3] libcamera: camera_sensor: Check control availability from idmap

[Jacopo Mondi]

Hi Laurent

On Sun, Jan 31, 2021 at 08:17:22PM +0200, Laurent Pinchart wrote:
> The presence of mandatory and optional controls is checked in
> CameraSensor::validateSensorDriver() by trying to retrieve them. This
> cases an error message to be printed in the V4L2Device class if an

causes

> optional control isn't present, while this isn't an error.
>
> To fix this, use the control idmap reported by the V4L2Device to check
> for control availability. The function can now print the whole list of

controls

> missing controls, making debugging easier.

Niiiice!

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/camera_sensor.cpp | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 10713d3a0c29..85813befbf58 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -250,14 +250,18 @@ int CameraSensor::validateSensorDriver()
>  	 * Optional controls are used to register optional sensor properties. If
>  	 * not present, some values will be defaulted.
>  	 */
> -	const std::vector<uint32_t> optionalControls{
> +	static constexpr uint32_t optionalControls[] = {

Unrelated but welcome

>  		V4L2_CID_CAMERA_ORIENTATION,
>  		V4L2_CID_CAMERA_SENSOR_ROTATION,
>  	};
>
> -	ControlList ctrls = subdev_->getControls(optionalControls);
> -	if (ctrls.empty())
> -		LOG(CameraSensor, Debug) << "Optional V4L2 controls not supported";
> +	const ControlIdMap &controls = subdev_->controls().idmap();
> +	for (uint32_t ctrl : optionalControls) {
> +		if (!controls.count(ctrl))

Why going through the idmap ? Can't you call count() on the
ControlInfoMap returned by subdev_->controls() ?

> +			LOG(CameraSensor, Debug)
> +				<< "Optional V4L2 control " << utils::hex(ctrl)
> +				<< " not supported";
> +	}

I really hoped we could have printed the control name out.
The only way I can think of, as we can't create the V4L2ControlId from
the kernel interface that reports the control's name is going through
a macro that stringifies the V4L2_CID_... identifier.

Not for this patch though

>
>  	/*
>  	 * Make sure the required selection targets are supported.
> @@ -312,21 +316,28 @@ int CameraSensor::validateSensorDriver()
>  	 * For raw sensors, make sure the sensor driver supports the controls
>  	 * required by the CameraSensor class.
>  	 */
> -	const std::vector<uint32_t> mandatoryControls{
> +	static constexpr uint32_t mandatoryControls[] = {
>  		V4L2_CID_EXPOSURE,
>  		V4L2_CID_HBLANK,
>  		V4L2_CID_PIXEL_RATE,
>  	};
>
> -	ctrls = subdev_->getControls(mandatoryControls);
> -	if (ctrls.empty()) {
> -		LOG(CameraSensor, Error)
> -			<< "Mandatory V4L2 controls not available";
> +	err = 0;
> +	for (uint32_t ctrl : mandatoryControls) {
> +		if (!controls.count(ctrl)) {
> +			LOG(CameraSensor, Error)
> +				<< "Mandatory V4L2 control " << utils::hex(ctrl)
> +				<< " not available";
> +			err = -EINVAL;

Should you break here ?

> +		}
> +	}
> +
> +	if (err) {
>  		LOG(CameraSensor, Error)
>  			<< "The sensor kernel driver needs to be fixed";
>  		LOG(CameraSensor, Error)
>  			<< "See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information";
> -		return -EINVAL;
> +		return err;
>  	}
>
>  	return 0;
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel