| Message ID | 20221123113644.1778-2-david.plowman@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thanks for fixing this. On Wed, 23 Nov 2022 at 11:36, David Plowman via libcamera-devel < libcamera-devel@lists.libcamera.org> wrote: > We no longer reset the HBLANK in init because we may not own the > camera and the operation may fail. Instead, we reset it when we set > the camera format so that all pipeline handlers, even ones that do not > control HBLANK themselves, will have a well-defined value. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Seems reasonable! Reviewed-By: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/camera_sensor.cpp | 53 ++++++++++++++++----------------- > 1 file changed, 26 insertions(+), 27 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp > b/src/libcamera/camera_sensor.cpp > index 572a313a..ec7bb0e7 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -176,32 +176,6 @@ int CameraSensor::init() > if (ret) > return ret; > > - /* > - * Set HBLANK to the minimum to start with a well-defined line > length, > - * allowing IPA modules that do not modify HBLANK to use the sensor > - * minimum line length in their calculations. > - * > - * At present, there is no way of knowing if a control is > read-only. > - * As a workaround, assume that if the minimum and maximum values > of > - * the V4L2_CID_HBLANK control are the same, it implies the control > - * is read-only. > - * > - * \todo The control API ought to have a flag to specify if a > control > - * is read-only which could be used below. > - */ > - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > - const int32_t hblankMin = hblank.min().get<int32_t>(); > - const int32_t hblankMax = hblank.max().get<int32_t>(); > - > - if (hblankMin != hblankMax) { > - ControlList ctrl(subdev_->controls()); > - > - ctrl.set(V4L2_CID_HBLANK, hblankMin); > - ret = subdev_->setControls(&ctrl); > - if (ret) > - return ret; > - } > - > return > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > } > > @@ -748,7 +722,32 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat > *format) > return ret; > > updateControlInfo(); > - return 0; > + > + /* > + * Set HBLANK to the minimum to start with a well-defined line > length, > + * allowing IPA modules that do not modify HBLANK to use the sensor > + * minimum line length in their calculations. > + * > + * At present, there is no way of knowing if a control is > read-only. > + * As a workaround, assume that if the minimum and maximum values > of > + * the V4L2_CID_HBLANK control are the same, it implies the control > + * is read-only. > + * > + * \todo The control API ought to have a flag to specify if a > control > + * is read-only which could be used below. > + */ > + const ControlInfo hblank = controls().at(V4L2_CID_HBLANK); > + const int32_t hblankMin = hblank.min().get<int32_t>(); > + const int32_t hblankMax = hblank.max().get<int32_t>(); > + > + if (hblankMin != hblankMax) { > + ControlList ctrl(subdev_->controls()); > + > + ctrl.set(V4L2_CID_HBLANK, hblankMin); > + ret = subdev_->setControls(&ctrl); > + } > + > + return ret; > } > > /** > -- > 2.30.2 > >
Hi David On Wed, Nov 23, 2022 at 11:36:44AM +0000, David Plowman via libcamera-devel wrote: > We no longer reset the HBLANK in init because we may not own the > camera and the operation may fail. Instead, we reset it when we set > the camera format so that all pipeline handlers, even ones that do not > control HBLANK themselves, will have a well-defined value. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/libcamera/camera_sensor.cpp | 53 ++++++++++++++++----------------- > 1 file changed, 26 insertions(+), 27 deletions(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 572a313a..ec7bb0e7 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -176,32 +176,6 @@ int CameraSensor::init() > if (ret) > return ret; > > - /* > - * Set HBLANK to the minimum to start with a well-defined line length, > - * allowing IPA modules that do not modify HBLANK to use the sensor > - * minimum line length in their calculations. > - * > - * At present, there is no way of knowing if a control is read-only. > - * As a workaround, assume that if the minimum and maximum values of > - * the V4L2_CID_HBLANK control are the same, it implies the control > - * is read-only. > - * > - * \todo The control API ought to have a flag to specify if a control > - * is read-only which could be used below. > - */ > - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > - const int32_t hblankMin = hblank.min().get<int32_t>(); > - const int32_t hblankMax = hblank.max().get<int32_t>(); > - > - if (hblankMin != hblankMax) { > - ControlList ctrl(subdev_->controls()); > - > - ctrl.set(V4L2_CID_HBLANK, hblankMin); > - ret = subdev_->setControls(&ctrl); > - if (ret) > - return ret; > - } > - > return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > } > > @@ -748,7 +722,32 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format) > return ret; > > updateControlInfo(); I would do that -before- updating the controls info. > - return 0; > + > + /* > + * Set HBLANK to the minimum to start with a well-defined line length, > + * allowing IPA modules that do not modify HBLANK to use the sensor > + * minimum line length in their calculations. > + * > + * At present, there is no way of knowing if a control is read-only. I know it was already like this, but can't we use const struct v4l2_query_ext_ctrl *V4L2Device::controlInfo(uint32_t id) const and inspect the flags ? > + * As a workaround, assume that if the minimum and maximum values of > + * the V4L2_CID_HBLANK control are the same, it implies the control > + * is read-only. > + * > + * \todo The control API ought to have a flag to specify if a control > + * is read-only which could be used below. > + */ > + const ControlInfo hblank = controls().at(V4L2_CID_HBLANK); > + const int32_t hblankMin = hblank.min().get<int32_t>(); > + const int32_t hblankMax = hblank.max().get<int32_t>(); > + > + if (hblankMin != hblankMax) { > + ControlList ctrl(subdev_->controls()); > + > + ctrl.set(V4L2_CID_HBLANK, hblankMin); > + ret = subdev_->setControls(&ctrl); > + } > + > + return ret; > } I could take this one on top of the flip series maybe, if you agree with the above changes ? Thanks j > > /** > -- > 2.30.2 >
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 572a313a..ec7bb0e7 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -176,32 +176,6 @@ int CameraSensor::init() if (ret) return ret; - /* - * Set HBLANK to the minimum to start with a well-defined line length, - * allowing IPA modules that do not modify HBLANK to use the sensor - * minimum line length in their calculations. - * - * At present, there is no way of knowing if a control is read-only. - * As a workaround, assume that if the minimum and maximum values of - * the V4L2_CID_HBLANK control are the same, it implies the control - * is read-only. - * - * \todo The control API ought to have a flag to specify if a control - * is read-only which could be used below. - */ - const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); - const int32_t hblankMin = hblank.min().get<int32_t>(); - const int32_t hblankMax = hblank.max().get<int32_t>(); - - if (hblankMin != hblankMax) { - ControlList ctrl(subdev_->controls()); - - ctrl.set(V4L2_CID_HBLANK, hblankMin); - ret = subdev_->setControls(&ctrl); - if (ret) - return ret; - } - return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); } @@ -748,7 +722,32 @@ int CameraSensor::setFormat(V4L2SubdeviceFormat *format) return ret; updateControlInfo(); - return 0; + + /* + * Set HBLANK to the minimum to start with a well-defined line length, + * allowing IPA modules that do not modify HBLANK to use the sensor + * minimum line length in their calculations. + * + * At present, there is no way of knowing if a control is read-only. + * As a workaround, assume that if the minimum and maximum values of + * the V4L2_CID_HBLANK control are the same, it implies the control + * is read-only. + * + * \todo The control API ought to have a flag to specify if a control + * is read-only which could be used below. + */ + const ControlInfo hblank = controls().at(V4L2_CID_HBLANK); + const int32_t hblankMin = hblank.min().get<int32_t>(); + const int32_t hblankMax = hblank.max().get<int32_t>(); + + if (hblankMin != hblankMax) { + ControlList ctrl(subdev_->controls()); + + ctrl.set(V4L2_CID_HBLANK, hblankMin); + ret = subdev_->setControls(&ctrl); + } + + return ret; } /**
We no longer reset the HBLANK in init because we may not own the camera and the operation may fail. Instead, we reset it when we set the camera format so that all pipeline handlers, even ones that do not control HBLANK themselves, will have a well-defined value. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/libcamera/camera_sensor.cpp | 53 ++++++++++++++++----------------- 1 file changed, 26 insertions(+), 27 deletions(-)