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

[Jacopo Mondi]

Hi Laurent,

On Mon, Feb 01, 2021 at 10:45:02PM +0200, Laurent Pinchart wrote:
> 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.
>

Right, sorry, I've overlooked this

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

That would be very nice and would make the controls framework get past
the <ControlId> <unsigned int> duality.

> > >
> > >  	/*
> > >  	 * 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.
>

Correct

Please add
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

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