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

[Niklas Söderlund]

On 2021-01-04 15:12:23 +0200, Laurent Pinchart wrote:
> 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

True, but is that really a problem? Are not all three lines part of the 
same "message"? In a perfect world I would rather extend our LOG() 
implementation to support multiline messages by only printing the 
timestamp+prefixes for the first line and then indent the rest with 
whitespaces to make it clear that some lines belongs together.

I do not feel strongly about this so if I'm in minority feel free to 
keep my R-b while keeping the multiple LOG() calls.

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

-- 
Regards,
Niklas Söderlund