[libcamera-devel] [PATCH v2 1/5] libcamera: camera_sensor: Validate driver support
Jacopo Mondi
jacopo at jmondi.org
Wed Dec 30 11:16:07 CET 2020
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 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.
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
More information about the libcamera-devel
mailing list