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

10761 diff mbox series

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
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            | 106 +++++++++++++++++++--
 2 files changed, 100 insertions(+), 7 deletions(-)

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@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@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;
>  	}
>

Hi Laurent,

On Wed, Dec 30, 2020 at 11:51:46AM +0200, Laurent Pinchart wrote:
> 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@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@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.

Wouldn't the user wonder what to fix with "driver needs to be fixed" :)

Anyway, I can easily change the message

>
> > +		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 ?
>

At the moment Soraka for sure until three sensor patches I have out
won't be backported. RPi should be fine, Scarlet I have not checked
tbh.

I agree failing early is the most efficient way to have those sensor
drivers fixed

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

Depends if we want to make what I named "optionalControls" mandatory.
In this case we will break most platforms, as none (afaict) provides
the required information in the firmward (being OF for RPi or ACPI for
Soraka)

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

depends on the caller implementation :)

> 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.

Makes sense, we are verbose enough during validation
I'll make these shorter.

>
> >
> >  		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