[libcamera-devel] [PATCH v3 2/6] libcamera: camera_sensor: Validate driver support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 30 20:53:42 CET 2020
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 at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at 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 at 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;
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list