| Message ID | 20260603140818.2558156-1-jai.luthra@ideasonboard.com |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series |
|
| Related | show |
Hi Jai, Quoting Jai Luthra (2026-06-03 15:08:18) > Use the difference between againMin + 1 and againMin as the againMinStep > value. This is better than a heuristic of 1% of the whole range, which > doesn't match the actual allowed values by the V4L2 control, especially > for sensors with an exponential gain model (that map the control values > to equal-sized dB increments). > > Without this change I saw a lot of oscillations with IMX678. While the > oscillations did go away after I narrowed the gain control to only map > to 0 - 30dB of analogue gain, and ignore the 30.3 - 72dB digital gain > range, I still think we should use the minimum step allowed by V4L2 > rather than a heuristic of 1%. This sounds good, but there's some existing work on the list that we should also consider here: Can you take a look at https://patchwork.libcamera.org/project/libcamera/list/?series=5915 and in particular https://patchwork.libcamera.org/patch/26662/ please ? I also imagine lots of this changing as part of our AGC rework for libipa - so if there's a quick win here already I'm fine merging it - but I think there could be bigger changes imminent too. -- Kieran > > Even for sensors with a linear gain model like IMX219, that allow > setting more than 100 values, this should make the AGC algorithm > smoother. > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com> > --- > src/ipa/simple/soft_simple.cpp | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index 629e1a32d..e463e38af 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -228,10 +228,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > context_.configuration.agc.againMin = camHelper_->gain(againMin); > context_.configuration.agc.againMax = camHelper_->gain(againMax); > context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0); > + double againNext = camHelper_->gain(std::min(againMin + 1, againMax)); > context_.configuration.agc.againMinStep = > - (context_.configuration.agc.againMax - > - context_.configuration.agc.againMin) / > - 100.0; > + againNext - context_.configuration.agc.againMin; > if (camHelper_->blackLevel().has_value()) { > /* > * The black level from camHelper_ is a 16 bit value, software ISP > -- > 2.54.0 >
Hi Kieran, Thanks for the quick reply. Quoting Kieran Bingham (2026-06-03 19:54:45) > Hi Jai, > > > Quoting Jai Luthra (2026-06-03 15:08:18) > > Use the difference between againMin + 1 and againMin as the againMinStep > > value. This is better than a heuristic of 1% of the whole range, which > > doesn't match the actual allowed values by the V4L2 control, especially > > for sensors with an exponential gain model (that map the control values > > to equal-sized dB increments). > > > > Without this change I saw a lot of oscillations with IMX678. While the > > oscillations did go away after I narrowed the gain control to only map > > to 0 - 30dB of analogue gain, and ignore the 30.3 - 72dB digital gain > > range, I still think we should use the minimum step allowed by V4L2 > > rather than a heuristic of 1%. > > This sounds good, but there's some existing work on the list that we > should also consider here: > > Can you take a look at > https://patchwork.libcamera.org/project/libcamera/list/?series=5915 and > in particular https://patchwork.libcamera.org/patch/26662/ please ? > I have that particular patch already in my tree (based on master). While it fixed the algorithm to shrink the steps to <1% when close to the target, the actual programmed values still get clipped to againMinStep (= 1%) without my patch. > I also imagine lots of this changing as part of our AGC rework for > libipa - so if there's a quick win here already I'm fine merging it - > but I think there could be bigger changes imminent too. > Oh good point. If this whole code path is going to be dropped in the new libipa based AGC, we can skip this patch. > -- > Kieran > Thanks, Jai > > > > > Even for sensors with a linear gain model like IMX219, that allow > > setting more than 100 values, this should make the AGC algorithm > > smoother. > > > > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com> > > --- > > src/ipa/simple/soft_simple.cpp | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > > index 629e1a32d..e463e38af 100644 > > --- a/src/ipa/simple/soft_simple.cpp > > +++ b/src/ipa/simple/soft_simple.cpp > > @@ -228,10 +228,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > > context_.configuration.agc.againMin = camHelper_->gain(againMin); > > context_.configuration.agc.againMax = camHelper_->gain(againMax); > > context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0); > > + double againNext = camHelper_->gain(std::min(againMin + 1, againMax)); > > context_.configuration.agc.againMinStep = > > - (context_.configuration.agc.againMax - > > - context_.configuration.agc.againMin) / > > - 100.0; > > + againNext - context_.configuration.agc.againMin; > > if (camHelper_->blackLevel().has_value()) { > > /* > > * The black level from camHelper_ is a 16 bit value, software ISP > > -- > > 2.54.0 > >
Hi 2026. 06. 04. 6:42 keltezéssel, Jai Luthra írta: > Hi Kieran, > > Thanks for the quick reply. > > Quoting Kieran Bingham (2026-06-03 19:54:45) >> Hi Jai, >> >> >> Quoting Jai Luthra (2026-06-03 15:08:18) >>> Use the difference between againMin + 1 and againMin as the againMinStep >>> value. This is better than a heuristic of 1% of the whole range, which >>> doesn't match the actual allowed values by the V4L2 control, especially >>> for sensors with an exponential gain model (that map the control values >>> to equal-sized dB increments). >>> >>> Without this change I saw a lot of oscillations with IMX678. While the >>> oscillations did go away after I narrowed the gain control to only map >>> to 0 - 30dB of analogue gain, and ignore the 30.3 - 72dB digital gain >>> range, I still think we should use the minimum step allowed by V4L2 >>> rather than a heuristic of 1%. >> >> This sounds good, but there's some existing work on the list that we >> should also consider here: >> >> Can you take a look at >> https://patchwork.libcamera.org/project/libcamera/list/?series=5915 and >> in particular https://patchwork.libcamera.org/patch/26662/ please ? >> > > I have that particular patch already in my tree (based on master). While it > fixed the algorithm to shrink the steps to <1% when close to the target, > the actual programmed values still get clipped to againMinStep (= 1%) > without my patch. > >> I also imagine lots of this changing as part of our AGC rework for >> libipa - so if there's a quick win here already I'm fine merging it - >> but I think there could be bigger changes imminent too. >> > > Oh good point. If this whole code path is going to be dropped in the new > libipa based AGC, we can skip this patch. I'm afraid it won't be, since there needs to be an algorithm for sensor helper-less scenarios, and the current mean luminance based AGC implementation most likely does not satisfactorily with just gain codes. > >> -- >> Kieran >> > > Thanks, > Jai > >> >>> >>> Even for sensors with a linear gain model like IMX219, that allow >>> setting more than 100 values, this should make the AGC algorithm >>> smoother. >>> >>> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com> >>> --- >>> src/ipa/simple/soft_simple.cpp | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >>> index 629e1a32d..e463e38af 100644 >>> --- a/src/ipa/simple/soft_simple.cpp >>> +++ b/src/ipa/simple/soft_simple.cpp >>> @@ -228,10 +228,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >>> context_.configuration.agc.againMin = camHelper_->gain(againMin); >>> context_.configuration.agc.againMax = camHelper_->gain(againMax); >>> context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0); >>> + double againNext = camHelper_->gain(std::min(againMin + 1, againMax)); >>> context_.configuration.agc.againMinStep = >>> - (context_.configuration.agc.againMax - >>> - context_.configuration.agc.againMin) / >>> - 100.0; >>> + againNext - context_.configuration.agc.againMin; I imagine `gain(againMax) - gain(max(againMax - 1, againMin))` would be a valid choice as well? Should the min/max of the two be used? If the mapping is not linear, the two might be different, which one would be a better option? >>> if (camHelper_->blackLevel().has_value()) { >>> /* >>> * The black level from camHelper_ is a 16 bit value, software ISP >>> -- >>> 2.54.0 >>>
Quoting Barnabás Pőcze (2026-06-04 06:39:19) > Hi > > 2026. 06. 04. 6:42 keltezéssel, Jai Luthra írta: > > Hi Kieran, > > > > Thanks for the quick reply. > > > > Quoting Kieran Bingham (2026-06-03 19:54:45) > >> Hi Jai, > >> > >> > >> Quoting Jai Luthra (2026-06-03 15:08:18) > >>> Use the difference between againMin + 1 and againMin as the againMinStep > >>> value. This is better than a heuristic of 1% of the whole range, which > >>> doesn't match the actual allowed values by the V4L2 control, especially > >>> for sensors with an exponential gain model (that map the control values > >>> to equal-sized dB increments). > >>> > >>> Without this change I saw a lot of oscillations with IMX678. While the > >>> oscillations did go away after I narrowed the gain control to only map > >>> to 0 - 30dB of analogue gain, and ignore the 30.3 - 72dB digital gain > >>> range, I still think we should use the minimum step allowed by V4L2 > >>> rather than a heuristic of 1%. > >> > >> This sounds good, but there's some existing work on the list that we > >> should also consider here: > >> > >> Can you take a look at > >> https://patchwork.libcamera.org/project/libcamera/list/?series=5915 and > >> in particular https://patchwork.libcamera.org/patch/26662/ please ? > >> > > > > I have that particular patch already in my tree (based on master). While it > > fixed the algorithm to shrink the steps to <1% when close to the target, > > the actual programmed values still get clipped to againMinStep (= 1%) > > without my patch. > > > >> I also imagine lots of this changing as part of our AGC rework for > >> libipa - so if there's a quick win here already I'm fine merging it - > >> but I think there could be bigger changes imminent too. > >> > > > > Oh good point. If this whole code path is going to be dropped in the new > > libipa based AGC, we can skip this patch. > > I'm afraid it won't be, since there needs to be an algorithm for sensor > helper-less scenarios, and the current mean luminance based AGC implementation > most likely does not satisfactorily with just gain codes. > In this instance though - we have a camera sensor helper, which means we should have a correct gain model! So I think we should try to fix things so that when we have a gain model we /can/ use AgcMeanLuminance. I know that's ongoing work and investigation though. -- Kieran > > > > >> -- > >> Kieran > >> > > > > Thanks, > > Jai > > > >> > >>> > >>> Even for sensors with a linear gain model like IMX219, that allow > >>> setting more than 100 values, this should make the AGC algorithm > >>> smoother. > >>> > >>> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com> > >>> --- > >>> src/ipa/simple/soft_simple.cpp | 5 ++--- > >>> 1 file changed, 2 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > >>> index 629e1a32d..e463e38af 100644 > >>> --- a/src/ipa/simple/soft_simple.cpp > >>> +++ b/src/ipa/simple/soft_simple.cpp > >>> @@ -228,10 +228,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > >>> context_.configuration.agc.againMin = camHelper_->gain(againMin); > >>> context_.configuration.agc.againMax = camHelper_->gain(againMax); > >>> context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0); > >>> + double againNext = camHelper_->gain(std::min(againMin + 1, againMax)); > >>> context_.configuration.agc.againMinStep = > >>> - (context_.configuration.agc.againMax - > >>> - context_.configuration.agc.againMin) / > >>> - 100.0; > >>> + againNext - context_.configuration.agc.againMin; > > I imagine `gain(againMax) - gain(max(againMax - 1, againMin))` would be a > valid choice as well? Should the min/max of the two be used? If the mapping > is not linear, the two might be different, which one would be a better option? > > > > >>> if (camHelper_->blackLevel().has_value()) { > >>> /* > >>> * The black level from camHelper_ is a 16 bit value, software ISP > >>> -- > >>> 2.54.0 > >>> >
Hi Barnabás, Quoting Barnabás Pőcze (2026-06-04 11:09:19) > Hi > > 2026. 06. 04. 6:42 keltezéssel, Jai Luthra írta: > > Hi Kieran, > > > > Thanks for the quick reply. > > > > Quoting Kieran Bingham (2026-06-03 19:54:45) > >> Hi Jai, > >> > >> > >> Quoting Jai Luthra (2026-06-03 15:08:18) > >>> Use the difference between againMin + 1 and againMin as the againMinStep > >>> value. This is better than a heuristic of 1% of the whole range, which > >>> doesn't match the actual allowed values by the V4L2 control, especially > >>> for sensors with an exponential gain model (that map the control values > >>> to equal-sized dB increments). > >>> > >>> Without this change I saw a lot of oscillations with IMX678. While the > >>> oscillations did go away after I narrowed the gain control to only map > >>> to 0 - 30dB of analogue gain, and ignore the 30.3 - 72dB digital gain > >>> range, I still think we should use the minimum step allowed by V4L2 > >>> rather than a heuristic of 1%. > >> > >> This sounds good, but there's some existing work on the list that we > >> should also consider here: > >> > >> Can you take a look at > >> https://patchwork.libcamera.org/project/libcamera/list/?series=5915 and > >> in particular https://patchwork.libcamera.org/patch/26662/ please ? > >> > > > > I have that particular patch already in my tree (based on master). While it > > fixed the algorithm to shrink the steps to <1% when close to the target, > > the actual programmed values still get clipped to againMinStep (= 1%) > > without my patch. > > > >> I also imagine lots of this changing as part of our AGC rework for > >> libipa - so if there's a quick win here already I'm fine merging it - > >> but I think there could be bigger changes imminent too. > >> > > > > Oh good point. If this whole code path is going to be dropped in the new > > libipa based AGC, we can skip this patch. > > I'm afraid it won't be, since there needs to be an algorithm for sensor > helper-less scenarios, and the current mean luminance based AGC implementation > most likely does not satisfactorily with just gain codes. > > > > > >> -- > >> Kieran > >> > > > > Thanks, > > Jai > > > >> > >>> > >>> Even for sensors with a linear gain model like IMX219, that allow > >>> setting more than 100 values, this should make the AGC algorithm > >>> smoother. > >>> > >>> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com> > >>> --- > >>> src/ipa/simple/soft_simple.cpp | 5 ++--- > >>> 1 file changed, 2 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > >>> index 629e1a32d..e463e38af 100644 > >>> --- a/src/ipa/simple/soft_simple.cpp > >>> +++ b/src/ipa/simple/soft_simple.cpp > >>> @@ -228,10 +228,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > >>> context_.configuration.agc.againMin = camHelper_->gain(againMin); > >>> context_.configuration.agc.againMax = camHelper_->gain(againMax); > >>> context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0); > >>> + double againNext = camHelper_->gain(std::min(againMin + 1, againMax)); > >>> context_.configuration.agc.againMinStep = > >>> - (context_.configuration.agc.againMax - > >>> - context_.configuration.agc.againMin) / > >>> - 100.0; > >>> + againNext - context_.configuration.agc.againMin; > > I imagine `gain(againMax) - gain(max(againMax - 1, againMin))` would be a > valid choice as well? Should the min/max of the two be used? If the mapping > is not linear, the two might be different, which one would be a better option? > For exponential mappings each step is a fixed multiplicative factor, so the values near max will have really big differences between them. Using gain = 10^(dB/20) for IMX678 (0.3dB step, 1.035x multiplication) this would mean againMinStep is: gain(againMin + 1) - gain(againMin) = 1.035 - 1 = 0.035x gain(againMax) - gain(againMax - 1) = 31.62 - 30.54 = 1.08x With 1% step size we had before this patch, againMinStep was: (31.62 - 1) / 100 = 0.3x It gets worse if we use 72dB as max (like I had originally in the driver with combined digital gain): gain(againMax) - gain(againMax - 1) = 3981 - 3846 = 135x (gain(againMax) - gain(againMin)) / 100 = (3981 - 1)/100 = 398x Gain jumps between 1 and 100x or 400x are noticable I would say ;-) TBH, subtracting two gain values doesn't make sense for the exponential case at all, instead we should use 1.035x here as the minimum multiplicative step. Or just use a better algorithm for sensors that have a known gain model (like IMX678). For unknown cases though, I think it is better to be safe and use the smallest jump size, given sensors or bad drivers might expose unreasonably high values to userspace. Even if this means the algorithm is wasting iterations close to the target, bigger jumps are still allowed, so it shouldn't be slow or noticeable to the end-user. Thanks, Jai > > > >>> if (camHelper_->blackLevel().has_value()) { > >>> /* > >>> * The black level from camHelper_ is a 16 bit value, software ISP > >>> -- > >>> 2.54.0 > >>> >
Quoting Kieran Bingham (2026-06-04 17:44:16) > Quoting Barnabás Pőcze (2026-06-04 06:39:19) > > Hi > > > > 2026. 06. 04. 6:42 keltezéssel, Jai Luthra írta: > > > Hi Kieran, > > > > > > Thanks for the quick reply. > > > > > > Quoting Kieran Bingham (2026-06-03 19:54:45) > > >> Hi Jai, > > >> > > >> > > >> Quoting Jai Luthra (2026-06-03 15:08:18) > > >>> Use the difference between againMin + 1 and againMin as the againMinStep > > >>> value. This is better than a heuristic of 1% of the whole range, which > > >>> doesn't match the actual allowed values by the V4L2 control, especially > > >>> for sensors with an exponential gain model (that map the control values > > >>> to equal-sized dB increments). > > >>> > > >>> Without this change I saw a lot of oscillations with IMX678. While the > > >>> oscillations did go away after I narrowed the gain control to only map > > >>> to 0 - 30dB of analogue gain, and ignore the 30.3 - 72dB digital gain > > >>> range, I still think we should use the minimum step allowed by V4L2 > > >>> rather than a heuristic of 1%. > > >> > > >> This sounds good, but there's some existing work on the list that we > > >> should also consider here: > > >> > > >> Can you take a look at > > >> https://patchwork.libcamera.org/project/libcamera/list/?series=5915 and > > >> in particular https://patchwork.libcamera.org/patch/26662/ please ? > > >> > > > > > > I have that particular patch already in my tree (based on master). While it > > > fixed the algorithm to shrink the steps to <1% when close to the target, > > > the actual programmed values still get clipped to againMinStep (= 1%) > > > without my patch. > > > > > >> I also imagine lots of this changing as part of our AGC rework for > > >> libipa - so if there's a quick win here already I'm fine merging it - > > >> but I think there could be bigger changes imminent too. > > >> > > > > > > Oh good point. If this whole code path is going to be dropped in the new > > > libipa based AGC, we can skip this patch. > > > > I'm afraid it won't be, since there needs to be an algorithm for sensor > > helper-less scenarios, and the current mean luminance based AGC implementation > > most likely does not satisfactorily with just gain codes. > > > > In this instance though - we have a camera sensor helper, which means we > should have a correct gain model! So I think we should try to fix things > so that when we have a gain model we /can/ use AgcMeanLuminance. > > I know that's ongoing work and investigation though. Ah you're right, for cases where we don't have a camHelper_ we anyway do: context_.configuration.agc.againMax = againMax; context_.configuration.agc.again10 = againDef; context_.configuration.agc.againMin = againMin; context_.configuration.agc.againMinStep = 1.0; And for the cases where we have model, I think using AgcMeanLuminance is the correct way forward. You can ignore this patch I think. Thanks, Jai > > -- > Kieran > > > > > > > > > >> -- > > >> Kieran > > >> > > > > > > Thanks, > > > Jai > > > > > >> > > >>> > > >>> Even for sensors with a linear gain model like IMX219, that allow > > >>> setting more than 100 values, this should make the AGC algorithm > > >>> smoother. > > >>> > > >>> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com> > > >>> --- > > >>> src/ipa/simple/soft_simple.cpp | 5 ++--- > > >>> 1 file changed, 2 insertions(+), 3 deletions(-) > > >>> > > >>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > > >>> index 629e1a32d..e463e38af 100644 > > >>> --- a/src/ipa/simple/soft_simple.cpp > > >>> +++ b/src/ipa/simple/soft_simple.cpp > > >>> @@ -228,10 +228,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > > >>> context_.configuration.agc.againMin = camHelper_->gain(againMin); > > >>> context_.configuration.agc.againMax = camHelper_->gain(againMax); > > >>> context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0); > > >>> + double againNext = camHelper_->gain(std::min(againMin + 1, againMax)); > > >>> context_.configuration.agc.againMinStep = > > >>> - (context_.configuration.agc.againMax - > > >>> - context_.configuration.agc.againMin) / > > >>> - 100.0; > > >>> + againNext - context_.configuration.agc.againMin; > > > > I imagine `gain(againMax) - gain(max(againMax - 1, againMin))` would be a > > valid choice as well? Should the min/max of the two be used? If the mapping > > is not linear, the two might be different, which one would be a better option? > > > > > > > > >>> if (camHelper_->blackLevel().has_value()) { > > >>> /* > > >>> * The black level from camHelper_ is a 16 bit value, software ISP > > >>> -- > > >>> 2.54.0 > > >>> > >
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index 629e1a32d..e463e38af 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -228,10 +228,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) context_.configuration.agc.againMin = camHelper_->gain(againMin); context_.configuration.agc.againMax = camHelper_->gain(againMax); context_.configuration.agc.again10 = std::max(context_.configuration.agc.againMin, 1.0); + double againNext = camHelper_->gain(std::min(againMin + 1, againMax)); context_.configuration.agc.againMinStep = - (context_.configuration.agc.againMax - - context_.configuration.agc.againMin) / - 100.0; + againNext - context_.configuration.agc.againMin; if (camHelper_->blackLevel().has_value()) { /* * The black level from camHelper_ is a 16 bit value, software ISP
Use the difference between againMin + 1 and againMin as the againMinStep value. This is better than a heuristic of 1% of the whole range, which doesn't match the actual allowed values by the V4L2 control, especially for sensors with an exponential gain model (that map the control values to equal-sized dB increments). Without this change I saw a lot of oscillations with IMX678. While the oscillations did go away after I narrowed the gain control to only map to 0 - 30dB of analogue gain, and ignore the 30.3 - 72dB digital gain range, I still think we should use the minimum step allowed by V4L2 rather than a heuristic of 1%. Even for sensors with a linear gain model like IMX219, that allow setting more than 100 values, this should make the AGC algorithm smoother. Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com> --- src/ipa/simple/soft_simple.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)