| Message ID | 20210131181722.5410-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent 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@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() ? > + 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 > > /* > * 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 ? > + } > + } > + > + 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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;
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@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@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
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[] = { 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)) + LOG(CameraSensor, Debug) + << "Optional V4L2 control " << utils::hex(ctrl) + << " not supported"; + } /* * 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; + } + } + + 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;
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 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 missing controls, making debugging easier. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/camera_sensor.cpp | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)