| Message ID | 20260630082309.804169-1-naush@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Tue, 30 Jun 2026 at 09:23, Naushir Patuck <naush@raspberrypi.com> wrote: > > Some sensor device drivers (e.g, VD66GY and VD56G3) have sensor auto- > exposure enabled by default. This interferes with the AGC algorithm > running in the IPA. > > By default, disable the sensor auto-exposure on startup so that the IPA > can control the sensor shutter time and gain. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Works perfectly with vd65g4 through camera_sensor_legacy, so should work with all the ST sensors. I haven't got a setup using camera_sensor_raw at present to test that path, but the code is identical to the legacy one. Thanks Dave > --- > src/libcamera/sensor/camera_sensor_legacy.cpp | 14 ++++++++++++++ > src/libcamera/sensor/camera_sensor_raw.cpp | 14 ++++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > index 6a683821f219..d86624dc4cc7 100644 > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > @@ -295,6 +295,20 @@ int CameraSensorLegacy::init() > return ret; > } > > + /* > + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to > + * handle that. > + */ > + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); > + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > + ControlList ctrl(subdev_->controls()); > + > + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); > + ret = subdev_->setControls(&ctrl); > + if (ret) > + return ret; > + } > + > return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > } > > diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > index 10eba0331fe8..1c5ee80281ad 100644 > --- a/src/libcamera/sensor/camera_sensor_raw.cpp > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > @@ -551,6 +551,20 @@ std::optional<int> CameraSensorRaw::init() > return ret; > } > > + /* > + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to > + * handle that. > + */ > + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); > + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > + ControlList ctrl(subdev_->controls()); > + > + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); > + ret = subdev_->setControls(&ctrl); > + if (ret) > + return ret; > + } > + > ret = applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > if (ret) > return { ret }; > -- > 2.53.0 >
Hi Naush, Well that was fast. Le 30/06/2026 à 12:40, Dave Stevenson a écrit : > Hi Naush > > On Tue, 30 Jun 2026 at 09:23, Naushir Patuck <naush@raspberrypi.com> wrote: >> >> Some sensor device drivers (e.g, VD66GY and VD56G3) have sensor auto- >> exposure enabled by default. This interferes with the AGC algorithm >> running in the IPA. >> >> By default, disable the sensor auto-exposure on startup so that the IPA >> can control the sensor shutter time and gain. >> >> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Works perfectly with vd65g4 through camera_sensor_legacy, so should > work with all the ST sensors. I haven't got a setup using > camera_sensor_raw at present to test that path, but the code is > identical to the legacy one. Tested successfully on my side too with all ST sensor having auto exposure enabled at boot time, i.e vd55g1, vd55g4, vd65g4, vd56g3 and vd66gy. Thanks a ton. Tested-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> All comments further down are minor, feel free to ignore them. > > Thanks > Dave > >> --- >> src/libcamera/sensor/camera_sensor_legacy.cpp | 14 ++++++++++++++ >> src/libcamera/sensor/camera_sensor_raw.cpp | 14 ++++++++++++++ >> 2 files changed, 28 insertions(+) >> >> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp >> index 6a683821f219..d86624dc4cc7 100644 >> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp >> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp >> @@ -295,6 +295,20 @@ int CameraSensorLegacy::init() >> return ret; >> } >> >> + /* >> + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to >> + * handle that. >> + */ >> + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); >> + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { If the control is available and happens to be read only, should we print a warning message stating that we can't disable auto exposure here ? Something alongside : if (exposureAuto) { if (exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY) LOG(CameraSensor, Warning) << "Can't disable auto exposure"; else [...] } In that case it may also be worth checking if the control is set to V4L2_EXPOSURE_AUTO to avoid printing the warning if it's set to V4L2_EXPOSURE_MANUAL already. >> + ControlList ctrl(subdev_->controls()); >> + >> + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); >> + ret = subdev_->setControls(&ctrl); >> + if (ret) >> + return ret; >> + } >> +>> return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); >> } >> >> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp >> index 10eba0331fe8..1c5ee80281ad 100644 >> --- a/src/libcamera/sensor/camera_sensor_raw.cpp >> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp >> @@ -551,6 +551,20 @@ std::optional<int> CameraSensorRaw::init() >> return ret; >> } >> >> + /* >> + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to >> + * handle that. >> + */ >> + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); >> + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { Ditto. >> + ControlList ctrl(subdev_->controls());>> + >> + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); >> + ret = subdev_->setControls(&ctrl); >> + if (ret) >> + return ret; >> + } >> + >> ret = applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); >> if (ret) >> return { ret }; >> -- >> 2.53.0 >>
On Tue, 30 Jun 2026 at 13:07, Benjamin Mugnier <benjamin.mugnier@foss.st.com> wrote: > > Hi Naush, > > Well that was fast. > > Le 30/06/2026 à 12:40, Dave Stevenson a écrit : > > Hi Naush > > > > On Tue, 30 Jun 2026 at 09:23, Naushir Patuck <naush@raspberrypi.com> wrote: > >> > >> Some sensor device drivers (e.g, VD66GY and VD56G3) have sensor auto- > >> exposure enabled by default. This interferes with the AGC algorithm > >> running in the IPA. > >> > >> By default, disable the sensor auto-exposure on startup so that the IPA > >> can control the sensor shutter time and gain. > >> > >> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > Works perfectly with vd65g4 through camera_sensor_legacy, so should > > work with all the ST sensors. I haven't got a setup using > > camera_sensor_raw at present to test that path, but the code is > > identical to the legacy one. > > Tested successfully on my side too with all ST sensor having auto > exposure enabled at boot time, i.e vd55g1, vd55g4, vd65g4, vd56g3 and > vd66gy. Thanks a ton. > > Tested-by: Benjamin Mugnier <benjamin.mugnier@foss.st.com> > > All comments further down are minor, feel free to ignore them. > > > > > Thanks > > Dave > > > >> --- > >> src/libcamera/sensor/camera_sensor_legacy.cpp | 14 ++++++++++++++ > >> src/libcamera/sensor/camera_sensor_raw.cpp | 14 ++++++++++++++ > >> 2 files changed, 28 insertions(+) > >> > >> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > >> index 6a683821f219..d86624dc4cc7 100644 > >> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > >> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > >> @@ -295,6 +295,20 @@ int CameraSensorLegacy::init() > >> return ret; > >> } > >> > >> + /* > >> + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to > >> + * handle that. > >> + */ > >> + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); > >> + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > If the control is available and happens to be read only, should we print > a warning message stating that we can't disable auto exposure here ? > Something alongside : > > if (exposureAuto) { > if (exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY) > LOG(CameraSensor, Warning) << "Can't disable auto exposure"; > else > [...] > } > > In that case it may also be worth checking if the control is set to > V4L2_EXPOSURE_AUTO to avoid printing the warning if it's set to > V4L2_EXPOSURE_MANUAL already. I can do this, but let me defer to Laurent to see if that's desirable. Naush > > >> + ControlList ctrl(subdev_->controls()); > >> + > >> + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); > >> + ret = subdev_->setControls(&ctrl); > >> + if (ret) > >> + return ret; > >> + } > >> +>> return > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > >> } > >> > >> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > >> index 10eba0331fe8..1c5ee80281ad 100644 > >> --- a/src/libcamera/sensor/camera_sensor_raw.cpp > >> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > >> @@ -551,6 +551,20 @@ std::optional<int> CameraSensorRaw::init() > >> return ret; > >> } > >> > >> + /* > >> + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to > >> + * handle that. > >> + */ > >> + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); > >> + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > Ditto. > > >> + ControlList ctrl(subdev_->controls());>> + > >> + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); > >> + ret = subdev_->setControls(&ctrl); > >> + if (ret) > >> + return ret; > >> + } > >> + > >> ret = applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > >> if (ret) > >> return { ret }; > >> -- > >> 2.53.0 > >> > > -- > Regards, > Benjamin >
Hi all, Would it be possible to merge this patch soon-ish (assuming folks are happy with it) please? Once this is merged, I'd like to make an RPi libcamera package release. Regards, Naush On Tue, 30 Jun 2026 at 09:23, Naushir Patuck <naush@raspberrypi.com> wrote: > > Some sensor device drivers (e.g, VD66GY and VD56G3) have sensor auto- > exposure enabled by default. This interferes with the AGC algorithm > running in the IPA. > > By default, disable the sensor auto-exposure on startup so that the IPA > can control the sensor shutter time and gain. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/sensor/camera_sensor_legacy.cpp | 14 ++++++++++++++ > src/libcamera/sensor/camera_sensor_raw.cpp | 14 ++++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > index 6a683821f219..d86624dc4cc7 100644 > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > @@ -295,6 +295,20 @@ int CameraSensorLegacy::init() > return ret; > } > > + /* > + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to > + * handle that. > + */ > + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); > + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > + ControlList ctrl(subdev_->controls()); > + > + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); > + ret = subdev_->setControls(&ctrl); > + if (ret) > + return ret; > + } > + > return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > } > > diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > index 10eba0331fe8..1c5ee80281ad 100644 > --- a/src/libcamera/sensor/camera_sensor_raw.cpp > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > @@ -551,6 +551,20 @@ std::optional<int> CameraSensorRaw::init() > return ret; > } > > + /* > + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to > + * handle that. > + */ > + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); > + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > + ControlList ctrl(subdev_->controls()); > + > + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); > + ret = subdev_->setControls(&ctrl); > + if (ret) > + return ret; > + } > + > ret = applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > if (ret) > return { ret }; > -- > 2.53.0 >
Quoting Naushir Patuck (2026-07-27 10:19:06) > Hi all, > > Would it be possible to merge this patch soon-ish (assuming folks are > happy with it) please? Once this is merged, I'd like to make an RPi > libcamera package release. > > Regards, > Naush > > On Tue, 30 Jun 2026 at 09:23, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > Some sensor device drivers (e.g, VD66GY and VD56G3) have sensor auto- > > exposure enabled by default. This interferes with the AGC algorithm > > running in the IPA. > > > > By default, disable the sensor auto-exposure on startup so that the IPA > > can control the sensor shutter time and gain. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/sensor/camera_sensor_legacy.cpp | 14 ++++++++++++++ > > src/libcamera/sensor/camera_sensor_raw.cpp | 14 ++++++++++++++ > > 2 files changed, 28 insertions(+) > > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > > index 6a683821f219..d86624dc4cc7 100644 > > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > > @@ -295,6 +295,20 @@ int CameraSensorLegacy::init() > > return ret; > > } > > > > + /* > > + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to > > + * handle that. > > + */ > > + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); > > + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > + ControlList ctrl(subdev_->controls()); > > + > > + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); > > + ret = subdev_->setControls(&ctrl); > > + if (ret) > > + return ret; > > + } > > + This patch has made me notice that we seem to 'change' the sensor at init time. I think that's when we construct a camera and therefore before we 'acquire' it ? I think that might be bad, as we will be trying to make modifications to a camera we don't control. So for example if a camera was already streaming through a separate channel - it could then be modified by a separate application enumerating cameras ? We might need to later refactor our camera sensor class interface so that it doesn't perform any actions on ::init() (which seems to be during 'match') and instead does so at configure time ? But that's a pre-existing issue, not caused by this patch. Hrm ... I was about to add a tag anyway, but now I'm also worried about how this will impact sensors where we /want/ AE to happen on the sensor. What happens with say an OV5640 (or any other YUV sensor) hooked up to the simplie pipeline handler here? Will this automatically disable it's on chip AE when it should be enabled? -- Kieran > > return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > } > > > > diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > > index 10eba0331fe8..1c5ee80281ad 100644 > > --- a/src/libcamera/sensor/camera_sensor_raw.cpp > > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > > @@ -551,6 +551,20 @@ std::optional<int> CameraSensorRaw::init() > > return ret; > > } > > > > + /* > > + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to > > + * handle that. > > + */ > > + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); > > + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > + ControlList ctrl(subdev_->controls()); > > + > > + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); > > + ret = subdev_->setControls(&ctrl); > > + if (ret) > > + return ret; > > + } > > + > > ret = applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > if (ret) > > return { ret }; > > -- > > 2.53.0 > >
Hi Kieran, On Mon, 27 Jul 2026 at 10:53, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Naushir Patuck (2026-07-27 10:19:06) > > Hi all, > > > > Would it be possible to merge this patch soon-ish (assuming folks are > > happy with it) please? Once this is merged, I'd like to make an RPi > > libcamera package release. > > > > Regards, > > Naush > > > > On Tue, 30 Jun 2026 at 09:23, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > Some sensor device drivers (e.g, VD66GY and VD56G3) have sensor auto- > > > exposure enabled by default. This interferes with the AGC algorithm > > > running in the IPA. > > > > > > By default, disable the sensor auto-exposure on startup so that the IPA > > > can control the sensor shutter time and gain. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/sensor/camera_sensor_legacy.cpp | 14 ++++++++++++++ > > > src/libcamera/sensor/camera_sensor_raw.cpp | 14 ++++++++++++++ > > > 2 files changed, 28 insertions(+) > > > > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > > > index 6a683821f219..d86624dc4cc7 100644 > > > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > > > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > > > @@ -295,6 +295,20 @@ int CameraSensorLegacy::init() > > > return ret; > > > } > > > > > > + /* > > > + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to > > > + * handle that. > > > + */ > > > + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); > > > + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > > + ControlList ctrl(subdev_->controls()); > > > + > > > + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); > > > + ret = subdev_->setControls(&ctrl); > > > + if (ret) > > > + return ret; > > > + } > > > + > > This patch has made me notice that we seem to 'change' the sensor at > init time. I think that's when we construct a camera and therefore > before we 'acquire' it ? > > I think that might be bad, as we will be trying to make modifications to > a camera we don't control. > > So for example if a camera was already streaming through a separate > channel - it could then be modified by a separate application > enumerating cameras ? > > We might need to later refactor our camera sensor class interface so > that it doesn't perform any actions on ::init() (which seems to be > during 'match') and instead does so at configure time ? > > But that's a pre-existing issue, not caused by this patch. > > Hrm ... I was about to add a tag anyway, but now I'm also worried about > how this will impact sensors where we /want/ AE to happen on the sensor. > > What happens with say an OV5640 (or any other YUV sensor) hooked up to > the simplie pipeline handler here? > > Will this automatically disable it's on chip AE when it should be > enabled? It definitely will! I was assuming that all sensors (including those driven from the SoftISP PH) used the IPA's AE algorithm, not the on-sensor algorithm. If this assumption is wrong, then this patch will indeed break things there, and we need to come up with an alternative way to make this happen for the above ST sensors. Naush > > -- > Kieran > > > > > > return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > > } > > > > > > diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > > > index 10eba0331fe8..1c5ee80281ad 100644 > > > --- a/src/libcamera/sensor/camera_sensor_raw.cpp > > > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > > > @@ -551,6 +551,20 @@ std::optional<int> CameraSensorRaw::init() > > > return ret; > > > } > > > > > > + /* > > > + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to > > > + * handle that. > > > + */ > > > + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); > > > + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > > + ControlList ctrl(subdev_->controls()); > > > + > > > + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); > > > + ret = subdev_->setControls(&ctrl); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > ret = applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > > if (ret) > > > return { ret }; > > > -- > > > 2.53.0 > > >
Hi 2026. 07. 27. 11:59 keltezéssel, Naushir Patuck írta: > Hi Kieran, > > On Mon, 27 Jul 2026 at 10:53, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: >> >> Quoting Naushir Patuck (2026-07-27 10:19:06) >>> Hi all, >>> >>> Would it be possible to merge this patch soon-ish (assuming folks are >>> happy with it) please? Once this is merged, I'd like to make an RPi >>> libcamera package release. >>> >>> Regards, >>> Naush >>> >>> On Tue, 30 Jun 2026 at 09:23, Naushir Patuck <naush@raspberrypi.com> wrote: >>>> >>>> Some sensor device drivers (e.g, VD66GY and VD56G3) have sensor auto- >>>> exposure enabled by default. This interferes with the AGC algorithm >>>> running in the IPA. >>>> >>>> By default, disable the sensor auto-exposure on startup so that the IPA >>>> can control the sensor shutter time and gain. >>>> >>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >>>> --- >>>> src/libcamera/sensor/camera_sensor_legacy.cpp | 14 ++++++++++++++ >>>> src/libcamera/sensor/camera_sensor_raw.cpp | 14 ++++++++++++++ >>>> 2 files changed, 28 insertions(+) >>>> >>>> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp >>>> index 6a683821f219..d86624dc4cc7 100644 >>>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp >>>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp >>>> @@ -295,6 +295,20 @@ int CameraSensorLegacy::init() >>>> return ret; >>>> } >>>> >>>> + /* >>>> + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to >>>> + * handle that. >>>> + */ >>>> + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); >>>> + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { >>>> + ControlList ctrl(subdev_->controls()); >>>> + >>>> + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); >>>> + ret = subdev_->setControls(&ctrl); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >> >> This patch has made me notice that we seem to 'change' the sensor at >> init time. I think that's when we construct a camera and therefore >> before we 'acquire' it ? >> >> I think that might be bad, as we will be trying to make modifications to >> a camera we don't control. >> >> So for example if a camera was already streaming through a separate >> channel - it could then be modified by a separate application >> enumerating cameras ? >> >> We might need to later refactor our camera sensor class interface so >> that it doesn't perform any actions on ::init() (which seems to be >> during 'match') and instead does so at configure time ? >> >> But that's a pre-existing issue, not caused by this patch. >> >> Hrm ... I was about to add a tag anyway, but now I'm also worried about >> how this will impact sensors where we /want/ AE to happen on the sensor. >> >> What happens with say an OV5640 (or any other YUV sensor) hooked up to >> the simplie pipeline handler here? >> >> Will this automatically disable it's on chip AE when it should be >> enabled? > > It definitely will! I was assuming that all sensors (including those > driven from the SoftISP PH) used the IPA's AE algorithm, not the > on-sensor algorithm. If this assumption is wrong, then this patch > will indeed break things there, and we need to come up with an > alternative way to make this happen for the above ST sensors. I think it makes a lot of sense to try to bring the sensors to a more or less "known" state during initialization, just like it is done with the hblank or test patterns. So doing the same with auto exposure seems reasonable to me. However, due to the lack of a "software" aegc algorithm, as mentioned, in some cases it is most likely desirable to use the sensor's capabilities. For example, with the "imx8-isi" or "simple" (without software isp) pipeline handlers. So it seems in addition to this change, there is a need for some kind of `enableAutoExposure(bool)` function so that pipeline handlers can opt in should they deem it useful. Regards, Barnabás Pőcze > > Naush > > >> >> -- >> Kieran >> >> >> >>>> return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); >>>> } >>>> >>>> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp >>>> index 10eba0331fe8..1c5ee80281ad 100644 >>>> --- a/src/libcamera/sensor/camera_sensor_raw.cpp >>>> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp >>>> @@ -551,6 +551,20 @@ std::optional<int> CameraSensorRaw::init() >>>> return ret; >>>> } >>>> >>>> + /* >>>> + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to >>>> + * handle that. >>>> + */ >>>> + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); >>>> + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { >>>> + ControlList ctrl(subdev_->controls()); >>>> + >>>> + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); >>>> + ret = subdev_->setControls(&ctrl); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> ret = applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); >>>> if (ret) >>>> return { ret }; >>>> -- >>>> 2.53.0 >>>>
diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp index 6a683821f219..d86624dc4cc7 100644 --- a/src/libcamera/sensor/camera_sensor_legacy.cpp +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp @@ -295,6 +295,20 @@ int CameraSensorLegacy::init() return ret; } + /* + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to + * handle that. + */ + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { + ControlList ctrl(subdev_->controls()); + + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); + ret = subdev_->setControls(&ctrl); + if (ret) + return ret; + } + return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); } diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp index 10eba0331fe8..1c5ee80281ad 100644 --- a/src/libcamera/sensor/camera_sensor_raw.cpp +++ b/src/libcamera/sensor/camera_sensor_raw.cpp @@ -551,6 +551,20 @@ std::optional<int> CameraSensorRaw::init() return ret; } + /* + * Ensure auto-exposure is disabled in the sensor as the IPAs ought to + * handle that. + */ + const struct v4l2_query_ext_ctrl *exposureAuto = subdev_->controlInfo(V4L2_CID_EXPOSURE_AUTO); + if (exposureAuto && !(exposureAuto->flags & V4L2_CTRL_FLAG_READ_ONLY)) { + ControlList ctrl(subdev_->controls()); + + ctrl.set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL); + ret = subdev_->setControls(&ctrl); + if (ret) + return ret; + } + ret = applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); if (ret) return { ret };
Some sensor device drivers (e.g, VD66GY and VD56G3) have sensor auto- exposure enabled by default. This interferes with the AGC algorithm running in the IPA. By default, disable the sensor auto-exposure on startup so that the IPA can control the sensor shutter time and gain. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/sensor/camera_sensor_legacy.cpp | 14 ++++++++++++++ src/libcamera/sensor/camera_sensor_raw.cpp | 14 ++++++++++++++ 2 files changed, 28 insertions(+)