[libcamera-devel] [PATCH v4 2/6] libcamera: camera_sensor: Validate driver support

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2020-12-31 00:05:59 +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 validating the sensor driver requirements in a
> single function
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  1 +
>  src/libcamera/camera_sensor.cpp            | 84 ++++++++++++++++++++++
>  2 files changed, 85 insertions(+)
> 
> 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..048fcd6a1fae 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";

Here and below I think this should be reworked to a single LOG() call.  
When we go more multithreading there is a possibility other threads may 
inject other LOG() calls between the ones here which may be confusing, 
how about?

LOG(CameraSensor, Error)
        << "Mandatory V4L2 controls not available" << std::endl
        << "The sensor kernel driver needs to be fixed" << std::endl
        << "See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information";

With this fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Optional controls are used to register optional sensor properties. If
> +	 * not present, some values will be defaulted.
> +	 */
> +	const std::vector<uint32_t> optionalControls{
> +		V4L2_CID_CAMERA_ORIENTATION,
> +		V4L2_CID_CAMERA_SENSOR_ROTATION,
> +	};
> +
> +	ctrls = subdev_->getControls(optionalControls);
> +	if (ctrls.empty())
> +		LOG(CameraSensor, Debug) << "Optional V4L2 controls not supported";
> +
> +	/*
> +	 * 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.
> +	 *
> +	 * \todo Make support for selection targets mandatory as soon as all
> +	 * test platforms have been updated.
> +	 */
> +	int err = 0;
> +	Rectangle rect;
> +	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);
> +	if (ret) {
> +		LOG(CameraSensor, Warning)
> +			<< "Failed to retrieve the readable pixel array size";
> +		err = -EINVAL;
> +	}
> +
> +	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
> +	if (ret) {
> +		LOG(CameraSensor, Warning)
> +			<< "Failed to retrieve the active pixel array size";
> +		err = -EINVAL;
> +	}
> +
> +	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);
> +	if (ret) {
> +		LOG(CameraSensor, Warning)
> +			<< "Failed to retrieve the sensor crop rectangle";
> +		err = -EINVAL;
> +	}
> +
> +	if (err) {
> +		LOG(CameraSensor, Warning)
> +			<< "The sensor kernel driver needs to be fixed";
> +		LOG(CameraSensor, Warning)
> +			<< "See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information";
> +	}
> +
> +	return 0;
> +}
> +
>  int CameraSensor::initProperties()
>  {
>  	/*
> -- 
> 2.29.2
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund