[libcamera-devel] [PATCH 3/3] libcamera: camera_sensor: Check control availability from idmap

[Laurent Pinchart]

Hi Jacopo,

On Mon, Feb 01, 2021 at 11:04:38AM +0100, Jacopo Mondi wrote:
> On Sun, Jan 31, 2021 at 08:17:22PM +0200, Laurent Pinchart wrote:
> > The presence of mandatory and optional controls is checked in
> > CameraSensor::validateSensorDriver() by trying to retrieve them. This
> > cases an error message to be printed in the V4L2Device class if an
> 
> causes
> 
> > optional control isn't present, while this isn't an error.
> >
> > To fix this, use the control idmap reported by the V4L2Device to check
> > for control availability. The function can now print the whole list of
> 
> controls
> 
> > missing controls, making debugging easier.
> 
> Niiiice!
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/camera_sensor.cpp | 31 +++++++++++++++++++++----------
> >  1 file changed, 21 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 10713d3a0c29..85813befbf58 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -250,14 +250,18 @@ int CameraSensor::validateSensorDriver()
> >  	 * Optional controls are used to register optional sensor properties. If
> >  	 * not present, some values will be defaulted.
> >  	 */
> > -	const std::vector<uint32_t> optionalControls{
> > +	static constexpr uint32_t optionalControls[] = {
> 
> Unrelated but welcome
> 
> >  		V4L2_CID_CAMERA_ORIENTATION,
> >  		V4L2_CID_CAMERA_SENSOR_ROTATION,
> >  	};
> >
> > -	ControlList ctrls = subdev_->getControls(optionalControls);
> > -	if (ctrls.empty())
> > -		LOG(CameraSensor, Debug) << "Optional V4L2 controls not supported";
> > +	const ControlIdMap &controls = subdev_->controls().idmap();
> > +	for (uint32_t ctrl : optionalControls) {
> > +		if (!controls.count(ctrl))
> 
> Why going through the idmap ? Can't you call count() on the
> ControlInfoMap returned by subdev_->controls() ?

ControlInfoMap is indexed by a ControlId *, while we have unsigned int
IDs here.

> > +			LOG(CameraSensor, Debug)
> > +				<< "Optional V4L2 control " << utils::hex(ctrl)
> > +				<< " not supported";
> > +	}
> 
> I really hoped we could have printed the control name out.
> The only way I can think of, as we can't create the V4L2ControlId from
> the kernel interface that reports the control's name is going through
> a macro that stringifies the V4L2_CID_... identifier.
> 
> Not for this patch though

I agree it would be useful. We could generate a list of Control<> for
all V4L2 controls, like we do for libcamera controls ;-) It's an idea
I've been toying with, but I'm still not sure if it's worth it.

> >
> >  	/*
> >  	 * Make sure the required selection targets are supported.
> > @@ -312,21 +316,28 @@ int CameraSensor::validateSensorDriver()
> >  	 * For raw sensors, make sure the sensor driver supports the controls
> >  	 * required by the CameraSensor class.
> >  	 */
> > -	const std::vector<uint32_t> mandatoryControls{
> > +	static constexpr uint32_t mandatoryControls[] = {
> >  		V4L2_CID_EXPOSURE,
> >  		V4L2_CID_HBLANK,
> >  		V4L2_CID_PIXEL_RATE,
> >  	};
> >
> > -	ctrls = subdev_->getControls(mandatoryControls);
> > -	if (ctrls.empty()) {
> > -		LOG(CameraSensor, Error)
> > -			<< "Mandatory V4L2 controls not available";
> > +	err = 0;
> > +	for (uint32_t ctrl : mandatoryControls) {
> > +		if (!controls.count(ctrl)) {
> > +			LOG(CameraSensor, Error)
> > +				<< "Mandatory V4L2 control " << utils::hex(ctrl)
> > +				<< " not available";
> > +			err = -EINVAL;
> 
> Should you break here ?

I could, but I think printing all the missing controls is useful,
instead of only printing the first one. Otherwise you'd go to the kernel
driver, implement the missing control, try again, and only notice the
next one. OK, you could also read the documentation and get a full list
:-) But it's pretty much free to print them all.

> > +		}
> > +	}
> > +
> > +	if (err) {
> >  		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;
> > +		return err;
> >  	}
> >
> >  	return 0;

-- 
Regards,

Laurent Pinchart