| Message ID | 20260520084631.3440-1-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thanks for the patch, On 20/05/2026 09:44, David Plowman wrote: > Digital gain is used to adjust overall image exposure when the target > exposure cannot be achieved (for example, cannot go high enough). But > when the target exposure is lower than we can achieve, the digital > gain was being set to values less than unity, causing saturation > problems. > > The fix is simply to clamp the digital gain at the bottom to 1.0, > resulting in images that, while "too bright", saturate correctly. > I've tested this on the OV64A40 and can confirm this fixes the issue seen. I think it needs a fixes tag: Fixes: 17f9912cff (ipa: rpi: agc: Calculate digital gain in process()) Reviewed-by: Alen Karnil <alen.karnil@ideasonboard.com> Tested-by: Alen Karnil <alen.karnil@ideasonboard.com> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/rpi/controller/rpi/agc_channel.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp > index cf0e77bd..c9462eed 100644 > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp > @@ -941,8 +941,8 @@ void AgcChannel::divideUpExposure() > /* Finally work out the digital gain that we will need. */ > filtered_.totalExposureNoDG = analogueGain * exposureTime; > double digitalGain = filtered_.totalExposure / filtered_.totalExposureNoDG; > - /* Limit dg by what is allowed. */ > - digitalGain = std::min(digitalGain, config_.maxDigitalGain); > + /* Limit dg by what is allowed (and to 1.0 to avoid saturation issues). */ > + digitalGain = std::clamp(digitalGain, 1.0, config_.maxDigitalGain); > /* Update total exposure, in case the dg went down. */ > filtered_.totalExposure = filtered_.totalExposureNoDG * digitalGain; >
Hi Alen Thanks for the review, and sorry if there's been some duplicated work there. On Wed, 20 May 2026 at 11:47, Alen Karnil <alen.karnil@ideasonboard.com> wrote: > > Hi David, > > Thanks for the patch, > > On 20/05/2026 09:44, David Plowman wrote: > > Digital gain is used to adjust overall image exposure when the target > > exposure cannot be achieved (for example, cannot go high enough). But > > when the target exposure is lower than we can achieve, the digital > > gain was being set to values less than unity, causing saturation > > problems. > > > > The fix is simply to clamp the digital gain at the bottom to 1.0, > > resulting in images that, while "too bright", saturate correctly. > > > > I've tested this on the OV64A40 and can confirm this fixes the issue seen. > > I think it needs a fixes tag: > Fixes: 17f9912cff (ipa: rpi: agc: Calculate digital gain in process()) > > Reviewed-by: Alen Karnil <alen.karnil@ideasonboard.com> > Tested-by: Alen Karnil <alen.karnil@ideasonboard.com> I'm hoping all those tags can be added while merging the patch? Otherwise I can always send an update. Thanks! David > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/rpi/controller/rpi/agc_channel.cpp | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp > > index cf0e77bd..c9462eed 100644 > > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp > > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp > > @@ -941,8 +941,8 @@ void AgcChannel::divideUpExposure() > > /* Finally work out the digital gain that we will need. */ > > filtered_.totalExposureNoDG = analogueGain * exposureTime; > > double digitalGain = filtered_.totalExposure / filtered_.totalExposureNoDG; > > - /* Limit dg by what is allowed. */ > > - digitalGain = std::min(digitalGain, config_.maxDigitalGain); > > + /* Limit dg by what is allowed (and to 1.0 to avoid saturation issues). */ > > + digitalGain = std::clamp(digitalGain, 1.0, config_.maxDigitalGain); > > /* Update total exposure, in case the dg went down. */ > > filtered_.totalExposure = filtered_.totalExposureNoDG * digitalGain; > > >
Quoting David Plowman (2026-05-20 11:53:18) > Hi Alen > > Thanks for the review, and sorry if there's been some duplicated work there. No worries, it's all a team effort. > On Wed, 20 May 2026 at 11:47, Alen Karnil <alen.karnil@ideasonboard.com> wrote: > > > > Hi David, > > > > Thanks for the patch, > > > > On 20/05/2026 09:44, David Plowman wrote: > > > Digital gain is used to adjust overall image exposure when the target > > > exposure cannot be achieved (for example, cannot go high enough). But > > > when the target exposure is lower than we can achieve, the digital > > > gain was being set to values less than unity, causing saturation > > > problems. > > > > > > The fix is simply to clamp the digital gain at the bottom to 1.0, > > > resulting in images that, while "too bright", saturate correctly. > > > > > > > I've tested this on the OV64A40 and can confirm this fixes the issue seen. > > > > I think it needs a fixes tag: > > Fixes: 17f9912cff (ipa: rpi: agc: Calculate digital gain in process()) > > > > Reviewed-by: Alen Karnil <alen.karnil@ideasonboard.com> > > Tested-by: Alen Karnil <alen.karnil@ideasonboard.com> > > I'm hoping all those tags can be added while merging the patch? > Otherwise I can always send an update. Absolutely, I can handle that while applying. I think this one is now well tested which I've seen with my own eyes, and I believe this is accurate for all sensors: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> And I'll kick the ci for test and merge. Thanks all. -- Regards Kieran > > Thanks! > > David > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/ipa/rpi/controller/rpi/agc_channel.cpp | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp > > > index cf0e77bd..c9462eed 100644 > > > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp > > > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp > > > @@ -941,8 +941,8 @@ void AgcChannel::divideUpExposure() > > > /* Finally work out the digital gain that we will need. */ > > > filtered_.totalExposureNoDG = analogueGain * exposureTime; > > > double digitalGain = filtered_.totalExposure / filtered_.totalExposureNoDG; > > > - /* Limit dg by what is allowed. */ > > > - digitalGain = std::min(digitalGain, config_.maxDigitalGain); > > > + /* Limit dg by what is allowed (and to 1.0 to avoid saturation issues). */ > > > + digitalGain = std::clamp(digitalGain, 1.0, config_.maxDigitalGain); > > > /* Update total exposure, in case the dg went down. */ > > > filtered_.totalExposure = filtered_.totalExposureNoDG * digitalGain; > > > > >
diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp index cf0e77bd..c9462eed 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp @@ -941,8 +941,8 @@ void AgcChannel::divideUpExposure() /* Finally work out the digital gain that we will need. */ filtered_.totalExposureNoDG = analogueGain * exposureTime; double digitalGain = filtered_.totalExposure / filtered_.totalExposureNoDG; - /* Limit dg by what is allowed. */ - digitalGain = std::min(digitalGain, config_.maxDigitalGain); + /* Limit dg by what is allowed (and to 1.0 to avoid saturation issues). */ + digitalGain = std::clamp(digitalGain, 1.0, config_.maxDigitalGain); /* Update total exposure, in case the dg went down. */ filtered_.totalExposure = filtered_.totalExposureNoDG * digitalGain;
Digital gain is used to adjust overall image exposure when the target exposure cannot be achieved (for example, cannot go high enough). But when the target exposure is lower than we can achieve, the digital gain was being set to values less than unity, causing saturation problems. The fix is simply to clamp the digital gain at the bottom to 1.0, resulting in images that, while "too bright", saturate correctly. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/rpi/controller/rpi/agc_channel.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)