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

[Jacopo Mondi]

Hi Niklas,

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

Printing out without timestamp break the logs symmetry, it renders the
output really confusing to look at.

Printing longer lines is not convenient, as it makes the log
unreadable on smaller screen.

I prefer to keep what I have here if possible.

> >
> > > With this fixed,
> > >
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > >

Thanks
   j

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