| Message ID | 20210813143241.3528968-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Of course the title prefix of this should be libcamera: camera_sensor: -- Kieran On 13/08/2021 15:32, Kieran Bingham wrote: > The V4L2 controls which are optional, recommended, and mandatory report > errors if they are not found by CameraSensor::validateSensorDriver(). > > These errors report hex values for the controls, which can easily lead > to confusion and incorrect assumption about which control needs to be > investigated. > > This can be seen occuring already in issues such as [0] and is generally > an unfriendly result to report to users in a warning or error message. > > Adapt the tables of controls to store both the id and name of the > controls to report a user friendly message that can ease diagnosis of > any CameraSensor validation issues. > > [0] https://github.com/raspberrypi/linux/issues/4500 > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > I'm aware of course of the checkstyle changes suggested on V4L2_CID, but > I thought it was better to keep those macros 'scoped' here, and indented > accordingly. > --- > src/libcamera/camera_sensor.cpp | 45 ++++++++++++++++++++------------- > 1 file changed, 27 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 7a3864150c89..1409da1b071c 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -168,19 +168,26 @@ int CameraSensor::validateSensorDriver() > { > int err = 0; > > + struct v4l2_cid { > + const uint32_t id; > + const char *name; > + }; > + > + #define V4L2_CID(x) { x, #x } > + > /* > * Optional controls are used to register optional sensor properties. If > * not present, some values will be defaulted. > */ > - static constexpr uint32_t optionalControls[] = { > - V4L2_CID_CAMERA_SENSOR_ROTATION, > + static constexpr v4l2_cid optionalControls[] = { > + V4L2_CID(V4L2_CID_CAMERA_SENSOR_ROTATION), > }; > > const ControlIdMap &controls = subdev_->controls().idmap(); > - for (uint32_t ctrl : optionalControls) { > - if (!controls.count(ctrl)) > + for (auto &ctrl : optionalControls) { > + if (!controls.count(ctrl.id)) > LOG(CameraSensor, Debug) > - << "Optional V4L2 control " << utils::hex(ctrl) > + << "Optional V4L2 control " << ctrl.name > << " not supported"; > } > > @@ -188,14 +195,14 @@ int CameraSensor::validateSensorDriver() > * Recommended controls are similar to optional controls, but will > * become mandatory in the near future. Be loud if they're missing. > */ > - static constexpr uint32_t recommendedControls[] = { > - V4L2_CID_CAMERA_ORIENTATION, > + static constexpr v4l2_cid recommendedControls[] = { > + V4L2_CID(V4L2_CID_CAMERA_ORIENTATION), > }; > > - for (uint32_t ctrl : recommendedControls) { > - if (!controls.count(ctrl)) { > + for (auto &ctrl : recommendedControls) { > + if (!controls.count(ctrl.id)) { > LOG(CameraSensor, Warning) > - << "Recommended V4L2 control " << utils::hex(ctrl) > + << "Recommended V4L2 control " << ctrl.name > << " not supported"; > err = -EINVAL; > } > @@ -259,18 +266,20 @@ int CameraSensor::validateSensorDriver() > * For raw sensors, make sure the sensor driver supports the controls > * required by the CameraSensor class. > */ > - static constexpr uint32_t mandatoryControls[] = { > - V4L2_CID_EXPOSURE, > - V4L2_CID_HBLANK, > - V4L2_CID_PIXEL_RATE, > - V4L2_CID_VBLANK, > + static constexpr v4l2_cid mandatoryControls[] = { > + V4L2_CID(V4L2_CID_EXPOSURE), > + V4L2_CID(V4L2_CID_HBLANK), > + V4L2_CID(V4L2_CID_PIXEL_RATE), > + V4L2_CID(V4L2_CID_VBLANK), > }; > > + #undef V4L2_CID > + > err = 0; > - for (uint32_t ctrl : mandatoryControls) { > - if (!controls.count(ctrl)) { > + for (auto &ctrl : mandatoryControls) { > + if (!controls.count(ctrl.id)) { > LOG(CameraSensor, Error) > - << "Mandatory V4L2 control " << utils::hex(ctrl) > + << "Mandatory V4L2 control " << ctrl.name > << " not available"; > err = -EINVAL; > } >
Hi Kieran, Thank you for the patch. On Fri, Aug 13, 2021 at 03:32:41PM +0100, Kieran Bingham wrote: > The V4L2 controls which are optional, recommended, and mandatory report > errors if they are not found by CameraSensor::validateSensorDriver(). > > These errors report hex values for the controls, which can easily lead > to confusion and incorrect assumption about which control needs to be > investigated. > > This can be seen occuring already in issues such as [0] and is generally > an unfriendly result to report to users in a warning or error message. > > Adapt the tables of controls to store both the id and name of the > controls to report a user friendly message that can ease diagnosis of > any CameraSensor validation issues. > > [0] https://github.com/raspberrypi/linux/issues/4500 > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I'm not fond of this ad-hoc fix, as we would need similar code in other locations then. I'd rather try to revive the patch that creates Control instances for V4L2 controls. > --- > I'm aware of course of the checkstyle changes suggested on V4L2_CID, but > I thought it was better to keep those macros 'scoped' here, and indented > accordingly. > --- > src/libcamera/camera_sensor.cpp | 45 ++++++++++++++++++++------------- > 1 file changed, 27 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 7a3864150c89..1409da1b071c 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -168,19 +168,26 @@ int CameraSensor::validateSensorDriver() > { > int err = 0; > > + struct v4l2_cid { > + const uint32_t id; > + const char *name; > + }; > + > + #define V4L2_CID(x) { x, #x } > + > /* > * Optional controls are used to register optional sensor properties. If > * not present, some values will be defaulted. > */ > - static constexpr uint32_t optionalControls[] = { > - V4L2_CID_CAMERA_SENSOR_ROTATION, > + static constexpr v4l2_cid optionalControls[] = { > + V4L2_CID(V4L2_CID_CAMERA_SENSOR_ROTATION), > }; > > const ControlIdMap &controls = subdev_->controls().idmap(); > - for (uint32_t ctrl : optionalControls) { > - if (!controls.count(ctrl)) > + for (auto &ctrl : optionalControls) { > + if (!controls.count(ctrl.id)) > LOG(CameraSensor, Debug) > - << "Optional V4L2 control " << utils::hex(ctrl) > + << "Optional V4L2 control " << ctrl.name > << " not supported"; > } > > @@ -188,14 +195,14 @@ int CameraSensor::validateSensorDriver() > * Recommended controls are similar to optional controls, but will > * become mandatory in the near future. Be loud if they're missing. > */ > - static constexpr uint32_t recommendedControls[] = { > - V4L2_CID_CAMERA_ORIENTATION, > + static constexpr v4l2_cid recommendedControls[] = { > + V4L2_CID(V4L2_CID_CAMERA_ORIENTATION), > }; > > - for (uint32_t ctrl : recommendedControls) { > - if (!controls.count(ctrl)) { > + for (auto &ctrl : recommendedControls) { > + if (!controls.count(ctrl.id)) { > LOG(CameraSensor, Warning) > - << "Recommended V4L2 control " << utils::hex(ctrl) > + << "Recommended V4L2 control " << ctrl.name > << " not supported"; > err = -EINVAL; > } > @@ -259,18 +266,20 @@ int CameraSensor::validateSensorDriver() > * For raw sensors, make sure the sensor driver supports the controls > * required by the CameraSensor class. > */ > - static constexpr uint32_t mandatoryControls[] = { > - V4L2_CID_EXPOSURE, > - V4L2_CID_HBLANK, > - V4L2_CID_PIXEL_RATE, > - V4L2_CID_VBLANK, > + static constexpr v4l2_cid mandatoryControls[] = { > + V4L2_CID(V4L2_CID_EXPOSURE), > + V4L2_CID(V4L2_CID_HBLANK), > + V4L2_CID(V4L2_CID_PIXEL_RATE), > + V4L2_CID(V4L2_CID_VBLANK), > }; > > + #undef V4L2_CID > + > err = 0; > - for (uint32_t ctrl : mandatoryControls) { > - if (!controls.count(ctrl)) { > + for (auto &ctrl : mandatoryControls) { > + if (!controls.count(ctrl.id)) { > LOG(CameraSensor, Error) > - << "Mandatory V4L2 control " << utils::hex(ctrl) > + << "Mandatory V4L2 control " << ctrl.name > << " not available"; > err = -EINVAL; > }
On 13/08/2021 15:45, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Aug 13, 2021 at 03:32:41PM +0100, Kieran Bingham wrote: >> The V4L2 controls which are optional, recommended, and mandatory report >> errors if they are not found by CameraSensor::validateSensorDriver(). >> >> These errors report hex values for the controls, which can easily lead >> to confusion and incorrect assumption about which control needs to be >> investigated. >> >> This can be seen occuring already in issues such as [0] and is generally >> an unfriendly result to report to users in a warning or error message. >> >> Adapt the tables of controls to store both the id and name of the >> controls to report a user friendly message that can ease diagnosis of >> any CameraSensor validation issues. >> >> [0] https://github.com/raspberrypi/linux/issues/4500 >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > I'm not fond of this ad-hoc fix, as we would need similar code in other > locations then. I'd rather try to revive the patch that creates Control > instances for V4L2 controls. I posted something like this ... many months ago, and it didtn' get integrated for probably similar push back. Yet - here we are - still lacking a /very simple/ key feature. Leaving the debug^WError logs giving people hex codes to decipher. How about I add a \todo Remove this when V4L2 controsl print their names natively ;-) I want /something/ You're blocked/ not keen on your V4L2 Control implementation because it does things you don't like, so I have no idea when you'll merge that. -- Kieran > >> --- >> I'm aware of course of the checkstyle changes suggested on V4L2_CID, but >> I thought it was better to keep those macros 'scoped' here, and indented >> accordingly. >> --- >> src/libcamera/camera_sensor.cpp | 45 ++++++++++++++++++++------------- >> 1 file changed, 27 insertions(+), 18 deletions(-) >> >> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp >> index 7a3864150c89..1409da1b071c 100644 >> --- a/src/libcamera/camera_sensor.cpp >> +++ b/src/libcamera/camera_sensor.cpp >> @@ -168,19 +168,26 @@ int CameraSensor::validateSensorDriver() >> { >> int err = 0; >> >> + struct v4l2_cid { >> + const uint32_t id; >> + const char *name; >> + }; >> + >> + #define V4L2_CID(x) { x, #x } >> + >> /* >> * Optional controls are used to register optional sensor properties. If >> * not present, some values will be defaulted. >> */ >> - static constexpr uint32_t optionalControls[] = { >> - V4L2_CID_CAMERA_SENSOR_ROTATION, >> + static constexpr v4l2_cid optionalControls[] = { >> + V4L2_CID(V4L2_CID_CAMERA_SENSOR_ROTATION), >> }; >> >> const ControlIdMap &controls = subdev_->controls().idmap(); >> - for (uint32_t ctrl : optionalControls) { >> - if (!controls.count(ctrl)) >> + for (auto &ctrl : optionalControls) { >> + if (!controls.count(ctrl.id)) >> LOG(CameraSensor, Debug) >> - << "Optional V4L2 control " << utils::hex(ctrl) >> + << "Optional V4L2 control " << ctrl.name >> << " not supported"; >> } >> >> @@ -188,14 +195,14 @@ int CameraSensor::validateSensorDriver() >> * Recommended controls are similar to optional controls, but will >> * become mandatory in the near future. Be loud if they're missing. >> */ >> - static constexpr uint32_t recommendedControls[] = { >> - V4L2_CID_CAMERA_ORIENTATION, >> + static constexpr v4l2_cid recommendedControls[] = { >> + V4L2_CID(V4L2_CID_CAMERA_ORIENTATION), >> }; >> >> - for (uint32_t ctrl : recommendedControls) { >> - if (!controls.count(ctrl)) { >> + for (auto &ctrl : recommendedControls) { >> + if (!controls.count(ctrl.id)) { >> LOG(CameraSensor, Warning) >> - << "Recommended V4L2 control " << utils::hex(ctrl) >> + << "Recommended V4L2 control " << ctrl.name >> << " not supported"; >> err = -EINVAL; >> } >> @@ -259,18 +266,20 @@ int CameraSensor::validateSensorDriver() >> * For raw sensors, make sure the sensor driver supports the controls >> * required by the CameraSensor class. >> */ >> - static constexpr uint32_t mandatoryControls[] = { >> - V4L2_CID_EXPOSURE, >> - V4L2_CID_HBLANK, >> - V4L2_CID_PIXEL_RATE, >> - V4L2_CID_VBLANK, >> + static constexpr v4l2_cid mandatoryControls[] = { >> + V4L2_CID(V4L2_CID_EXPOSURE), >> + V4L2_CID(V4L2_CID_HBLANK), >> + V4L2_CID(V4L2_CID_PIXEL_RATE), >> + V4L2_CID(V4L2_CID_VBLANK), >> }; >> >> + #undef V4L2_CID >> + >> err = 0; >> - for (uint32_t ctrl : mandatoryControls) { >> - if (!controls.count(ctrl)) { >> + for (auto &ctrl : mandatoryControls) { >> + if (!controls.count(ctrl.id)) { >> LOG(CameraSensor, Error) >> - << "Mandatory V4L2 control " << utils::hex(ctrl) >> + << "Mandatory V4L2 control " << ctrl.name >> << " not available"; >> err = -EINVAL; >> } >
Hi Kieran, On Fri, Aug 13, 2021 at 03:47:36PM +0100, Kieran Bingham wrote: > On 13/08/2021 15:45, Laurent Pinchart wrote: > > On Fri, Aug 13, 2021 at 03:32:41PM +0100, Kieran Bingham wrote: > >> The V4L2 controls which are optional, recommended, and mandatory report > >> errors if they are not found by CameraSensor::validateSensorDriver(). > >> > >> These errors report hex values for the controls, which can easily lead > >> to confusion and incorrect assumption about which control needs to be > >> investigated. > >> > >> This can be seen occuring already in issues such as [0] and is generally > >> an unfriendly result to report to users in a warning or error message. > >> > >> Adapt the tables of controls to store both the id and name of the > >> controls to report a user friendly message that can ease diagnosis of > >> any CameraSensor validation issues. > >> > >> [0] https://github.com/raspberrypi/linux/issues/4500 > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > I'm not fond of this ad-hoc fix, as we would need similar code in other > > locations then. I'd rather try to revive the patch that creates Control > > instances for V4L2 controls. > > I posted something like this ... many months ago, and it didtn' get > integrated for probably similar push back. > > Yet - here we are - still lacking a /very simple/ key feature. > > Leaving the debug^WError logs giving people hex codes to decipher. > > How about I add a \todo Remove this when V4L2 controsl print their names > natively ;-) > > I want /something/ > > You're blocked/ not keen on your V4L2 Control implementation because it > does things you don't like, so I have no idea when you'll merge that. I've rebased that patch series, and I think I like it even less than before :-S I'd rather try to move away from V4L2 in IPA modules if possible, handling V4L2 in the pipeline handlers only. In any case, as that series isn't about the get merged, let's try to find a solution for the problem at hand. What would you think about moving control names to v4l2_device.cpp, with a static function to lookup the name based on the ID ? > >> --- > >> I'm aware of course of the checkstyle changes suggested on V4L2_CID, but > >> I thought it was better to keep those macros 'scoped' here, and indented > >> accordingly. > >> --- > >> src/libcamera/camera_sensor.cpp | 45 ++++++++++++++++++++------------- > >> 1 file changed, 27 insertions(+), 18 deletions(-) > >> > >> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > >> index 7a3864150c89..1409da1b071c 100644 > >> --- a/src/libcamera/camera_sensor.cpp > >> +++ b/src/libcamera/camera_sensor.cpp > >> @@ -168,19 +168,26 @@ int CameraSensor::validateSensorDriver() > >> { > >> int err = 0; > >> > >> + struct v4l2_cid { > >> + const uint32_t id; > >> + const char *name; > >> + }; > >> + > >> + #define V4L2_CID(x) { x, #x } > >> + > >> /* > >> * Optional controls are used to register optional sensor properties. If > >> * not present, some values will be defaulted. > >> */ > >> - static constexpr uint32_t optionalControls[] = { > >> - V4L2_CID_CAMERA_SENSOR_ROTATION, > >> + static constexpr v4l2_cid optionalControls[] = { > >> + V4L2_CID(V4L2_CID_CAMERA_SENSOR_ROTATION), > >> }; > >> > >> const ControlIdMap &controls = subdev_->controls().idmap(); > >> - for (uint32_t ctrl : optionalControls) { > >> - if (!controls.count(ctrl)) > >> + for (auto &ctrl : optionalControls) { > >> + if (!controls.count(ctrl.id)) > >> LOG(CameraSensor, Debug) > >> - << "Optional V4L2 control " << utils::hex(ctrl) > >> + << "Optional V4L2 control " << ctrl.name > >> << " not supported"; > >> } > >> > >> @@ -188,14 +195,14 @@ int CameraSensor::validateSensorDriver() > >> * Recommended controls are similar to optional controls, but will > >> * become mandatory in the near future. Be loud if they're missing. > >> */ > >> - static constexpr uint32_t recommendedControls[] = { > >> - V4L2_CID_CAMERA_ORIENTATION, > >> + static constexpr v4l2_cid recommendedControls[] = { > >> + V4L2_CID(V4L2_CID_CAMERA_ORIENTATION), > >> }; > >> > >> - for (uint32_t ctrl : recommendedControls) { > >> - if (!controls.count(ctrl)) { > >> + for (auto &ctrl : recommendedControls) { > >> + if (!controls.count(ctrl.id)) { > >> LOG(CameraSensor, Warning) > >> - << "Recommended V4L2 control " << utils::hex(ctrl) > >> + << "Recommended V4L2 control " << ctrl.name > >> << " not supported"; > >> err = -EINVAL; > >> } > >> @@ -259,18 +266,20 @@ int CameraSensor::validateSensorDriver() > >> * For raw sensors, make sure the sensor driver supports the controls > >> * required by the CameraSensor class. > >> */ > >> - static constexpr uint32_t mandatoryControls[] = { > >> - V4L2_CID_EXPOSURE, > >> - V4L2_CID_HBLANK, > >> - V4L2_CID_PIXEL_RATE, > >> - V4L2_CID_VBLANK, > >> + static constexpr v4l2_cid mandatoryControls[] = { > >> + V4L2_CID(V4L2_CID_EXPOSURE), > >> + V4L2_CID(V4L2_CID_HBLANK), > >> + V4L2_CID(V4L2_CID_PIXEL_RATE), > >> + V4L2_CID(V4L2_CID_VBLANK), > >> }; > >> > >> + #undef V4L2_CID > >> + > >> err = 0; > >> - for (uint32_t ctrl : mandatoryControls) { > >> - if (!controls.count(ctrl)) { > >> + for (auto &ctrl : mandatoryControls) { > >> + if (!controls.count(ctrl.id)) { > >> LOG(CameraSensor, Error) > >> - << "Mandatory V4L2 control " << utils::hex(ctrl) > >> + << "Mandatory V4L2 control " << ctrl.name > >> << " not available"; > >> err = -EINVAL; > >> }
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 7a3864150c89..1409da1b071c 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -168,19 +168,26 @@ int CameraSensor::validateSensorDriver() { int err = 0; + struct v4l2_cid { + const uint32_t id; + const char *name; + }; + + #define V4L2_CID(x) { x, #x } + /* * Optional controls are used to register optional sensor properties. If * not present, some values will be defaulted. */ - static constexpr uint32_t optionalControls[] = { - V4L2_CID_CAMERA_SENSOR_ROTATION, + static constexpr v4l2_cid optionalControls[] = { + V4L2_CID(V4L2_CID_CAMERA_SENSOR_ROTATION), }; const ControlIdMap &controls = subdev_->controls().idmap(); - for (uint32_t ctrl : optionalControls) { - if (!controls.count(ctrl)) + for (auto &ctrl : optionalControls) { + if (!controls.count(ctrl.id)) LOG(CameraSensor, Debug) - << "Optional V4L2 control " << utils::hex(ctrl) + << "Optional V4L2 control " << ctrl.name << " not supported"; } @@ -188,14 +195,14 @@ int CameraSensor::validateSensorDriver() * Recommended controls are similar to optional controls, but will * become mandatory in the near future. Be loud if they're missing. */ - static constexpr uint32_t recommendedControls[] = { - V4L2_CID_CAMERA_ORIENTATION, + static constexpr v4l2_cid recommendedControls[] = { + V4L2_CID(V4L2_CID_CAMERA_ORIENTATION), }; - for (uint32_t ctrl : recommendedControls) { - if (!controls.count(ctrl)) { + for (auto &ctrl : recommendedControls) { + if (!controls.count(ctrl.id)) { LOG(CameraSensor, Warning) - << "Recommended V4L2 control " << utils::hex(ctrl) + << "Recommended V4L2 control " << ctrl.name << " not supported"; err = -EINVAL; } @@ -259,18 +266,20 @@ int CameraSensor::validateSensorDriver() * For raw sensors, make sure the sensor driver supports the controls * required by the CameraSensor class. */ - static constexpr uint32_t mandatoryControls[] = { - V4L2_CID_EXPOSURE, - V4L2_CID_HBLANK, - V4L2_CID_PIXEL_RATE, - V4L2_CID_VBLANK, + static constexpr v4l2_cid mandatoryControls[] = { + V4L2_CID(V4L2_CID_EXPOSURE), + V4L2_CID(V4L2_CID_HBLANK), + V4L2_CID(V4L2_CID_PIXEL_RATE), + V4L2_CID(V4L2_CID_VBLANK), }; + #undef V4L2_CID + err = 0; - for (uint32_t ctrl : mandatoryControls) { - if (!controls.count(ctrl)) { + for (auto &ctrl : mandatoryControls) { + if (!controls.count(ctrl.id)) { LOG(CameraSensor, Error) - << "Mandatory V4L2 control " << utils::hex(ctrl) + << "Mandatory V4L2 control " << ctrl.name << " not available"; err = -EINVAL; }
The V4L2 controls which are optional, recommended, and mandatory report errors if they are not found by CameraSensor::validateSensorDriver(). These errors report hex values for the controls, which can easily lead to confusion and incorrect assumption about which control needs to be investigated. This can be seen occuring already in issues such as [0] and is generally an unfriendly result to report to users in a warning or error message. Adapt the tables of controls to store both the id and name of the controls to report a user friendly message that can ease diagnosis of any CameraSensor validation issues. [0] https://github.com/raspberrypi/linux/issues/4500 Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- I'm aware of course of the checkstyle changes suggested on V4L2_CID, but I thought it was better to keep those macros 'scoped' here, and indented accordingly. --- src/libcamera/camera_sensor.cpp | 45 ++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 18 deletions(-)