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

[Laurent Pinchart]

On Mon, Jan 04, 2021 at 02:08:28PM +0100, Niklas Söderlund wrote:
> 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";

Then we won't have the timestamp and other prefixes added to the lines
beside the first one :-S

> 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()
> >  {
> >  	/*

-- 
Regards,

Laurent Pinchart