[libcamera-devel] [PATCH v2 1/5] libcamera: camera_sensor: Validate driver support

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Dec 28, 2020 at 05:55:56PM +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 those information to register Camera properties and to

s/those information/that information/

> 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 at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h |   1 +
>  src/libcamera/camera_sensor.cpp            | 106 +++++++++++++++++++--
>  2 files changed, 100 insertions(+), 7 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..71d7c862e69a 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,81 @@ 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)
> +			<< "Please consider upgrading the sensor driver";

Maybe "The sensor kernel driver needs to be fixed" ? Users may wonder
what to upgrade to with a "please upgrade" message. But maybe I worry
too much :-) Up to you.

> +		return -EINVAL;
> +	}
> +
> +	int err = 0;
> +	/*
> +	 * 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, Info)
> +			<< "Optional V4L2 controls not supported";
> +		err = -EINVAL;
> +	}
> +
> +	/*
> +	 * 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";
> +		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)
> +			<< "Please consider upgrading the sensor driver";

Same as above.

> +
> +	return 0;

I think we need to make this fatal fairly soon, and I wonder whether we
could do so already. What platforms would we break ?

> +}
> +
>  int CameraSensor::initProperties()
>  {
>  	/*
> @@ -278,21 +357,29 @@ int CameraSensor::initProperties()
>  		}
>  	} else {
>  		propertyValue = properties::CameraLocationFront;
> +		LOG(CameraSensor, Debug)
> +			<< "Location property defaulted to 'Front Camera'";

If we make the failures above fatal, we will be able to drop the 'else'
branch here and below, right ?

>  	}
>  	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";
>  
>  	Rectangle crop;
>  	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &crop);
> @@ -306,6 +393,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. */
> @@ -569,6 +659,8 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  		LOG(CameraSensor, Error)
>  			<< "Failed to construct camera sensor info: "
>  			<< "the camera sensor does not report the active area";
> +		LOG(CameraSensor, Error)
> +			<< "The IPA might not work correctly";

Do we need this message and the one below ? In those error paths
sensorInfo() returns an error, and the caller will fail. It's not that
the IPA may not work correctly, the whole camera configuration will fail
:-) I'm tempted to take the opposite approach: now that we validate that
the sensor driver provides the right API, we could have less verbose
messages here. I'd drop the "Failed to construct camera sensor info:"
prefix, turning this particular error message into

  		LOG(CameraSensor, Error) << "Failed to get active area";

and similarly below.

>  
>  		return ret;
>  	}
> @@ -580,6 +672,8 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  		LOG(CameraSensor, Error)
>  			<< "Failed to construct camera sensor info: "
>  			<< "the camera sensor does not report the crop rectangle";
> +		LOG(CameraSensor, Error)
> +			<< "The IPA might not work correctly";
>  		return ret;
>  	}
>  
> @@ -601,16 +695,14 @@ 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)
> +			<< "The IPA might not work correctly";
>  		return -EINVAL;
>  	}
>  

-- 
Regards,

Laurent Pinchart