[libcamera-devel,v3,2/6] libcamera: camera_sensor: Validate driver support

10780 diff mbox series

The CameraSensor class requires the sensor driver to report
information through V4L2 controls and through the V4L2 selection API,
and uses that information to register Camera properties and to
construct CameraSensorInfo class instances to provide them to the IPA.

Currently, validation of the kernel support happens each time a
feature is requested, with slighly similar debug/error messages
output to the user in case a feature is not supported.

Rationalize this by:
- Validate the sensor driver requirements in a single function
- Expand the debug output when a property gets defaulted to a value
- Be more verbose when constructing CameraSensorInfo is not possible

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/internal/camera_sensor.h |   1 +
 src/libcamera/camera_sensor.cpp            | 117 ++++++++++++++++++---
 2 files changed, 102 insertions(+), 16 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Wed, Dec 30, 2020 at 07:01:16PM +0100, Jacopo Mondi wrote:
> The CameraSensor class requires the sensor driver to report
> information through V4L2 controls and through the V4L2 selection API,
> and uses that information to register Camera properties and to
> construct CameraSensorInfo class instances to provide them to the IPA.
> 
> Currently, validation of the kernel support happens each time a
> feature is requested, with slighly similar debug/error messages
> output to the user in case a feature is not supported.
> 
> Rationalize this by:
> - Validate the sensor driver requirements in a single function
> - Expand the debug output when a property gets defaulted to a value
> - Be more verbose when constructing CameraSensorInfo is not possible
> 
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h |   1 +
>  src/libcamera/camera_sensor.cpp            | 117 ++++++++++++++++++---
>  2 files changed, 102 insertions(+), 16 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index f80d836161a5..aee10aa6e3c7 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -69,6 +69,7 @@ protected:
>  
>  private:
>  	int generateId();
> +	int validateSensorDriver();
>  	int initProperties();
>  
>  	const MediaEntity *entity_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index e786821d4ba2..a1f1256bd6f4 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -207,6 +207,10 @@ int CameraSensor::init()
>  	 */
>  	resolution_ = sizes_.back();
>  
> +	ret = validateSensorDriver();
> +	if (ret)
> +		return ret;
> +
>  	ret = initProperties();
>  	if (ret)
>  		return ret;
> @@ -214,6 +218,86 @@ int CameraSensor::init()
>  	return 0;
>  }
>  
> +int CameraSensor::validateSensorDriver()
> +{
> +	/*
> +	 * Make sure the sensor driver supports the mandatory controls
> +	 * required by the CameraSensor class.
> +	 * - V4L2_CID_PIXEL_RATE is used to calculate the sensor timings
> +	 * - V4L2_CID_HBLANK is used to calculate the line length
> +	 */
> +	const std::vector<uint32_t> mandatoryControls{
> +		V4L2_CID_PIXEL_RATE,
> +		V4L2_CID_HBLANK,
> +	};
> +
> +	ControlList ctrls = subdev_->getControls(mandatoryControls);
> +	if (ctrls.empty()) {
> +		LOG(CameraSensor, Error)
> +			<< "Mandatory V4L2 controls not available";
> +		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;
> +	}
> +
> +	int err = 0;
> +	/*
> +	 * Optional controls are used to register optional sensor
> +	 * properties. If not present, some values will be defaulted.

You could wrap less agressively if desired :-)

> +	 */
> +	const std::vector<uint32_t> optionalControls{
> +		V4L2_CID_CAMERA_ORIENTATION,
> +		V4L2_CID_CAMERA_SENSOR_ROTATION,
> +	};
> +
> +	ctrls = subdev_->getControls(optionalControls);
> +	if (ctrls.empty()) {
> +		LOG(CameraSensor, Info)
> +			<< "Optional V4L2 controls not supported";
> +		err = -EINVAL;

It's valid for those controls not to be reported by the kernel, as
they're not always available in the firmware. I'd thus make this a debug
message, and not touch err, to void printing the messages at the end.

> +	}
> +
> +	/*
> +	 * Make sure the required selection targets are supported.
> +	 *
> +	 * Failures in reading any of the targets are not deemed to be fatal,
> +	 * but some properties and features, like constructing a
> +	 * CameraSensorInfo for the IPA module, won't be supported.
> +	 */
> +	Rectangle rect;
> +	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);
> +	if (ret) {
> +		LOG(CameraSensor, Info)
> +			<< "Failed to retrieve the readable pixel area size";

We write "pixel array" in the properties definitions, I'd do the same
here (and below as well).

> +		err = -EINVAL;
> +	}
> +
> +	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
> +	if (ret) {
> +		LOG(CameraSensor, Info)
> +			<< "Failed to retrieve the active pixel area size";
> +		err = -EINVAL;
> +	}
> +
> +	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);
> +	if (ret) {
> +		LOG(CameraSensor, Info)
> +			<< "Failed to retreive the sensor crop rectangle";
> +		err = -EINVAL;
> +	}
> +
> +	if (err) {
> +		LOG(CameraSensor, Info)
> +			<< "The sensor kernel driver needs to be fixed";
> +		LOG(CameraSensor, Info)
> +			<< "See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information";

How about making all these warnings already, so that the only thing
we'll need to do is add a return err here ?

> +	}
> +
> +	return 0;
> +}
> +
>  int CameraSensor::initProperties()
>  {
>  	/*
> @@ -278,21 +362,29 @@ int CameraSensor::initProperties()
>  		}
>  	} else {
>  		propertyValue = properties::CameraLocationFront;
> +		LOG(CameraSensor, Debug)
> +			<< "Location property defaulted to 'Front Camera'";
>  	}
>  	properties_.set(properties::Location, propertyValue);
>  
>  	/* Camera Rotation: default is 0 degrees. */
>  	const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> -	if (rotationControl != controls.end())
> +	if (rotationControl != controls.end()) {
>  		propertyValue = rotationControl->second.def().get<int32_t>();
> -	else
> +	} else {
>  		propertyValue = 0;
> +		LOG(CameraSensor, Debug)
> +			<< "Rotation property defaulted to 0 degrees";
> +	}
>  	properties_.set(properties::Rotation, propertyValue);
>  
>  	Rectangle bounds;
>  	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &bounds);
>  	if (!ret)
>  		properties_.set(properties::PixelArraySize, bounds.size());
> +	else
> +		LOG(CameraSensor, Debug)
> +			<< "PixelArraySize property not registered";

I think we'll drop those messages when we will consider this a fatal
error, but that doesn't mean we can't have them in the meantime.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	Rectangle crop;
>  	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &crop);
> @@ -306,6 +398,9 @@ int CameraSensor::initProperties()
>  		crop.x -= bounds.x;
>  		crop.y -= bounds.y;
>  		properties_.set(properties::PixelArrayActiveAreas, { crop });
> +	} else {
> +		LOG(CameraSensor, Debug)
> +			<< "PixelArrayActiveAreas property not registered";
>  	}
>  
>  	/* Color filter array pattern, register only for RAW sensors. */
> @@ -566,10 +661,7 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  	Rectangle rect;
>  	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
>  	if (ret) {
> -		LOG(CameraSensor, Error)
> -			<< "Failed to construct camera sensor info: "
> -			<< "the camera sensor does not report the active area";
> -
> +		LOG(CameraSensor, Error) << "Failed to get the active area";
>  		return ret;
>  	}
>  	info->activeAreaSize = { rect.width, rect.height };
> @@ -577,9 +669,7 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  	/* It's mandatory for the subdevice to report its crop rectangle. */
>  	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
>  	if (ret) {
> -		LOG(CameraSensor, Error)
> -			<< "Failed to construct camera sensor info: "
> -			<< "the camera sensor does not report the crop rectangle";
> +		LOG(CameraSensor, Error) << "Failed to get the crop rectangle";
>  		return ret;
>  	}
>  
> @@ -601,16 +691,11 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  	info->bitsPerPixel = format.bitsPerPixel();
>  	info->outputSize = format.size;
>  
> -	/*
> -	 * Retrieve the pixel rate and the line length through V4L2 controls.
> -	 * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is
> -	 * mandatory.
> -	 */
> +	/* Retrieve the pixel rate and the line length through V4L2 controls. */
>  	ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
>  						   V4L2_CID_HBLANK });
>  	if (ctrls.empty()) {
> -		LOG(CameraSensor, Error)
> -			<< "Failed to retrieve camera info controls";
> +		LOG(CameraSensor, Error) << "Failed to retrieve camera controls";
>  		return -EINVAL;
>  	}
>