| Message ID | 20260324123214.1762198-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2026-03-24 12:32:13) > This behaviour should be removed for the following reasons: > > * a `ControlList` is an unordered container, an index is largely meaningless; > * most callers either ignore the return value, or blindly return it, likely > contradicting their own documentation (e.g. `CameraSensorLegacy::setFormat()`), > and the error is already very visibly logged; > * an error index of 0 is indistinguishable from success with this approach. > > Instead, return `-EIO` instead of the index. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > Documentation/guides/pipeline-handler.rst | 6 ++---- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +- > src/libcamera/pipeline/vimc/vimc.cpp | 2 +- > src/libcamera/sensor/camera_sensor.cpp | 15 +-------------- > src/libcamera/v4l2_device.cpp | 14 ++++++-------- > 5 files changed, 11 insertions(+), 28 deletions(-) > > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst > index 083cdb745..b33d8b857 100644 > --- a/Documentation/guides/pipeline-handler.rst > +++ b/Documentation/guides/pipeline-handler.rst > @@ -1076,7 +1076,7 @@ device: > ret = data->video_->setControls(&controls); > if (ret) { > LOG(VIVID, Error) << "Failed to set controls: " << ret; > - return ret < 0 ? ret : -EINVAL; > + return ret; > } > > These controls configure VIVID to use a default test pattern, and enable all > @@ -1285,10 +1285,8 @@ before being set. > << " to " << ctrl.second.toString(); > > int ret = data->video_->setControls(&controls); > - if (ret) { > + if (ret) > LOG(VIVID, Error) << "Failed to set controls: " << ret; > - return ret < 0 ? ret : -EINVAL; > - } > > return ret; > } > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 3bd51733d..3435a7604 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -439,7 +439,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, const ControlList & > int ret = data->video_->setControls(&controls); > if (ret) { > LOG(UVC, Error) << "Failed to set controls: " << ret; > - return ret < 0 ? ret : -EINVAL; > + return ret; > } > > return ret; > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > index 08b100752..716f90818 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -434,7 +434,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) > int ret = data->sensor_->setControls(&controls); > if (ret) { > LOG(VIMC, Error) << "Failed to set controls: " << ret; > - return ret < 0 ? ret : -EINVAL; > + return ret; > } > > return ret; > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp > index 05390d1e1..602518afc 100644 > --- a/src/libcamera/sensor/camera_sensor.cpp > +++ b/src/libcamera/sensor/camera_sensor.cpp > @@ -379,21 +379,8 @@ int CameraSensor::setEmbeddedDataEnabled(bool enable) > * ctrls entry. The control identifiers are defined by the V4L2 specification > * (V4L2_CID_*). > * > - * If any control in \a ctrls is not supported by the device, is disabled (i.e. > - * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other > - * error occurs during validation of the requested controls, no control is > - * written and this function returns -EINVAL. > - * > - * If an error occurs while writing the controls, the index of the first > - * control that couldn't be written is returned. All controls below that index > - * are written and their values are updated in \a ctrls, while all other > - * controls are not written and their values are not changed. > - * > - * \sa V4L2Device::setControls() > - * > * \return 0 on success or an error code otherwise > - * \retval -EINVAL One of the control is not supported or not accessible > - * \retval i The index of the control that failed > + * \sa V4L2Device::setControls() > */ > > /** > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index b49d73b1c..53bd7865a 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -285,18 +285,16 @@ ControlList V4L2Device::getControls(Span<const uint32_t> ids, const V4L2Request > * \param[in] request An optional request > * > * This function writes the value of all controls contained in \a ctrls, and > - * stores the values actually applied to the device in the corresponding > - * \a ctrls entry. > + * updates \a ctrls with the values actually applied to the device. > * > * If any control in \a ctrls is not supported by the device, is disabled (i.e. > * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, if any other error > * occurs during validation of the requested controls, no control is written and > * this function returns -EINVAL. > * > - * If an error occurs while writing the controls, the index of the first > - * control that couldn't be written is returned. All controls below that index > - * are written and their values are updated in \a ctrls, while all other > - * controls are not written and their values are not changed. > + * If an error occurs while writing the controls, -EIO is returned. All controls > + * that were successfully written have their values are updated in \a ctrls, Something isn't right in the grammar here. Perhaps this meant: s/values are/are/g ? If an error occurs while writing the controls, -EIO is returned. All controls that were successfully written have their values updated in \a ctrls, while all other controls are not written and their values are not changed. I guess it's hard to know if a value was or wasn't written, as there's no way to know if the value was changed, or just stayed the same... But aside from that ... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + * while all other controls are not written and their values are not changed. > * > * If \a request is set, the controls will be applied to that request. If the > * device doesn't support requests, -EACCESS will be returned. If \a request is > @@ -304,8 +302,8 @@ ControlList V4L2Device::getControls(Span<const uint32_t> ids, const V4L2Request > * > * \return 0 on success or an error code otherwise > * \retval -EINVAL One of the controls is not supported or not accessible > + * \retval -EIO One or more controls were rejected by the device > * \retval -EACCESS The device does not support requests > - * \retval i The index of the control that failed > */ > int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request) > { > @@ -419,7 +417,7 @@ int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request) > << ": " << strerror(-ret); > > v4l2Ctrls.resize(errorIdx); > - ret = errorIdx; > + ret = -EIO; > } > > updateControls(ctrls, v4l2Ctrls); > -- > 2.53.0 >
diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst index 083cdb745..b33d8b857 100644 --- a/Documentation/guides/pipeline-handler.rst +++ b/Documentation/guides/pipeline-handler.rst @@ -1076,7 +1076,7 @@ device: ret = data->video_->setControls(&controls); if (ret) { LOG(VIVID, Error) << "Failed to set controls: " << ret; - return ret < 0 ? ret : -EINVAL; + return ret; } These controls configure VIVID to use a default test pattern, and enable all @@ -1285,10 +1285,8 @@ before being set. << " to " << ctrl.second.toString(); int ret = data->video_->setControls(&controls); - if (ret) { + if (ret) LOG(VIVID, Error) << "Failed to set controls: " << ret; - return ret < 0 ? ret : -EINVAL; - } return ret; } diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 3bd51733d..3435a7604 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -439,7 +439,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, const ControlList & int ret = data->video_->setControls(&controls); if (ret) { LOG(UVC, Error) << "Failed to set controls: " << ret; - return ret < 0 ? ret : -EINVAL; + return ret; } return ret; diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp index 08b100752..716f90818 100644 --- a/src/libcamera/pipeline/vimc/vimc.cpp +++ b/src/libcamera/pipeline/vimc/vimc.cpp @@ -434,7 +434,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) int ret = data->sensor_->setControls(&controls); if (ret) { LOG(VIMC, Error) << "Failed to set controls: " << ret; - return ret < 0 ? ret : -EINVAL; + return ret; } return ret; diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp index 05390d1e1..602518afc 100644 --- a/src/libcamera/sensor/camera_sensor.cpp +++ b/src/libcamera/sensor/camera_sensor.cpp @@ -379,21 +379,8 @@ int CameraSensor::setEmbeddedDataEnabled(bool enable) * ctrls entry. The control identifiers are defined by the V4L2 specification * (V4L2_CID_*). * - * If any control in \a ctrls is not supported by the device, is disabled (i.e. - * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other - * error occurs during validation of the requested controls, no control is - * written and this function returns -EINVAL. - * - * If an error occurs while writing the controls, the index of the first - * control that couldn't be written is returned. All controls below that index - * are written and their values are updated in \a ctrls, while all other - * controls are not written and their values are not changed. - * - * \sa V4L2Device::setControls() - * * \return 0 on success or an error code otherwise - * \retval -EINVAL One of the control is not supported or not accessible - * \retval i The index of the control that failed + * \sa V4L2Device::setControls() */ /** diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index b49d73b1c..53bd7865a 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -285,18 +285,16 @@ ControlList V4L2Device::getControls(Span<const uint32_t> ids, const V4L2Request * \param[in] request An optional request * * This function writes the value of all controls contained in \a ctrls, and - * stores the values actually applied to the device in the corresponding - * \a ctrls entry. + * updates \a ctrls with the values actually applied to the device. * * If any control in \a ctrls is not supported by the device, is disabled (i.e. * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, if any other error * occurs during validation of the requested controls, no control is written and * this function returns -EINVAL. * - * If an error occurs while writing the controls, the index of the first - * control that couldn't be written is returned. All controls below that index - * are written and their values are updated in \a ctrls, while all other - * controls are not written and their values are not changed. + * If an error occurs while writing the controls, -EIO is returned. All controls + * that were successfully written have their values are updated in \a ctrls, + * while all other controls are not written and their values are not changed. * * If \a request is set, the controls will be applied to that request. If the * device doesn't support requests, -EACCESS will be returned. If \a request is @@ -304,8 +302,8 @@ ControlList V4L2Device::getControls(Span<const uint32_t> ids, const V4L2Request * * \return 0 on success or an error code otherwise * \retval -EINVAL One of the controls is not supported or not accessible + * \retval -EIO One or more controls were rejected by the device * \retval -EACCESS The device does not support requests - * \retval i The index of the control that failed */ int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request) { @@ -419,7 +417,7 @@ int V4L2Device::setControls(ControlList *ctrls, const V4L2Request *request) << ": " << strerror(-ret); v4l2Ctrls.resize(errorIdx); - ret = errorIdx; + ret = -EIO; } updateControls(ctrls, v4l2Ctrls);
This behaviour should be removed for the following reasons: * a `ControlList` is an unordered container, an index is largely meaningless; * most callers either ignore the return value, or blindly return it, likely contradicting their own documentation (e.g. `CameraSensorLegacy::setFormat()`), and the error is already very visibly logged; * an error index of 0 is indistinguishable from success with this approach. Instead, return `-EIO` instead of the index. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- Documentation/guides/pipeline-handler.rst | 6 ++---- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +- src/libcamera/pipeline/vimc/vimc.cpp | 2 +- src/libcamera/sensor/camera_sensor.cpp | 15 +-------------- src/libcamera/v4l2_device.cpp | 14 ++++++-------- 5 files changed, 11 insertions(+), 28 deletions(-)