| Message ID | 20260105-softisp-agc-v1-1-77626505853a@mainlining.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Vasiliy, thank you for the fix. Vasiliy Doylov <nekocwd@mainlining.org> writes: > If camera sensor helper is not set, again10 will be 0. It should be set according to gainInfo (https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/simple/soft_simple.cpp#n201) in this case. Do you mean the case when gainInfo doesn't contain meaningful values? > It will prevent exposure change, because again will be clamped to againMin > and condition again > again10 will always be true. > Exposure decrease change will never occur. > Fixed by adding again > againMin check. > > Signed-off-by: Vasiliy Doylov <nekocwd@mainlining.org> > --- > src/ipa/simple/algorithms/agc.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index 189de770..2972844b 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -71,7 +71,8 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou > } > > if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { > - if (again > context.configuration.agc.again10) { > + if (again > context.configuration.agc.again10 && > + again > context.configuration.agc.againMin) { I think it would be better to ensure that again10 >= againMin (and perhaps also again10 <= againMax) when initialising the gain variables in soft_simple.cpp. > double next = again * kExpNumeratorDown / kExpDenominator; > if (again - next < context.configuration.agc.againMinStep) > again -= context.configuration.agc.againMinStep; > > --- > base-commit: 2861817f09c96a0660b88783eff159631e42030f > change-id: 20260105-softisp-agc-16d01cb3da49 > > Best regards,
Hi, Thank you for your patch. On 5-Jan-26 09:11, Vasiliy Doylov wrote: > If camera sensor helper is not set, again10 will be 0. > It will prevent exposure change, because again will be clamped to againMin > and condition again > again10 will always be true. > Exposure decrease change will never occur. > Fixed by adding again > againMin check. > > Signed-off-by: Vasiliy Doylov <nekocwd@mainlining.org> > --- > src/ipa/simple/algorithms/agc.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index 189de770..2972844b 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -71,7 +71,8 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou > } > > if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { > - if (again > context.configuration.agc.again10) { > + if (again > context.configuration.agc.again10 && > + again > context.configuration.agc.againMin) { both again10 and againMin get set by src/ipa/simple/soft_simple.cpp: IPASoftSimple::configure(), if there is no camera sensor helper we hit the following else block: } else { /* * The camera sensor gain (g) is usually not equal to the value written * into the gain register (x). But the way how the AGC algorithm changes * the gain value to make the total exposure closer to the optimum * assumes that g(x) is not too far from linear function. If the minimal * gain is 0, the g(x) is likely to be far from the linear, like * g(x) = a / (b * x + c). To avoid unexpected changes to the gain by * the AGC algorithm (abrupt near one edge, and very small near the * other) we limit the range of the gain values used. */ context_.configuration.agc.againMax = againMax; context_.configuration.agc.again10 = againDef; if (againMin) { context_.configuration.agc.againMin = againMin; } else { LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear"; context_.configuration.agc.againMin = std::min(100, againMin / 2 + againMax / 2); } context_.configuration.agc.againMinStep = 1.0; } So I guess you are hitting the case where againMin == 0 and then we come up with an artificial againMin value causing againDef < againMin and thus also again10 < againMin. I've always considered this whole overruling of againMin a bit weird, it feels to me that if a sensor driver advertises againMin = 0 that that then means that we should be able to actually use an again of 0 instead of avoiding it. Since you seem to have a sensor with againMin == 0, can you try what happens if you just remove the code to override againMin in this case ? Regards, Hans > double next = again * kExpNumeratorDown / kExpDenominator; > if (again - next < context.configuration.agc.againMinStep) > again -= context.configuration.agc.againMinStep; > > --- > base-commit: 2861817f09c96a0660b88783eff159631e42030f > change-id: 20260105-softisp-agc-16d01cb3da49 > > Best regards,
On 1/5/26 1:26 PM, johannes.goede@oss.qualcomm.com wrote: > Hi, > > Thank you for your patch. > > ... > > So I guess you are hitting the case where againMin == 0 My setup: Oneplus 6 (oneplus-enchilada) Rear wide camera imx376 (which has no cam helper) againMax = 480 againMin = 100 again10 is set to 0 (for some reason?) if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { // Overexposure detected if (again > context.configuration.agc.again10 /* will always be true, because again clamped to againMin at the end and again10(=0) < againMin(=100) */) { double next = again * kExpNumeratorDown / kExpDenominator; if (again - next < context.configuration.agc.againMinStep) again -= context.configuration.agc.againMinStep; else again = next; } else { int32_t next = exposure * kExpNumeratorDown / kExpDenominator; if (exposure - next < 1) exposure -= 1; else exposure = next; } } // Clamp to againMin(to 100) again = std::clamp(again, context.configuration.agc.againMin, context.configuration.agc.againMax);
Hi, On 5-Jan-26 11:44, Vasiliy Doylov wrote: > > On 1/5/26 1:26 PM, johannes.goede@oss.qualcomm.com wrote: >> Hi, >> >> Thank you for your patch. >> >> ... >> >> So I guess you are hitting the case where againMin == 0 > > My setup: > > Oneplus 6 (oneplus-enchilada) > > Rear wide camera imx376 (which has no cam helper) > > againMax = 480 > > againMin = 100 > > again10 is set to 0 (for some reason?) If there is no helper, then again10 gets set to againDef which comes from this lines in src/ipa/simple/soft_simple.cpp: int32_t againDef = gainInfo.def().get<int32_t>(); context_.configuration.agc.again10 = againDef; I guess that your imx376 driver may have a bug and is reporting a default value outside of the min max range. What is the output of: v4l2-ctl -d /dev/v4l-subdev# -l replacing # with the subdev no of the imx376, specifically what is the reported default value for the analogue_gain control there ? Regards, Hans p.s. I could not find an imx376 driver in the mainline kernel sources, so I guess you are using an out of tree driver ? > > > if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { // Overexposure detected > if (again > context.configuration.agc.again10 /* will always be true, because again clamped to againMin at the end and again10(=0) < againMin(=100) */) { > double next = again * kExpNumeratorDown / kExpDenominator; > if (again - next < context.configuration.agc.againMinStep) > again -= context.configuration.agc.againMinStep; > else > again = next; > } else { > int32_t next = exposure * kExpNumeratorDown / kExpDenominator; > if (exposure - next < 1) > exposure -= 1; > else > exposure = next; > } > } > > // Clamp to againMin(to 100) > again = std::clamp(again, context.configuration.agc.againMin, > context.configuration.agc.againMax); >
On 1/5/26 1:24 PM, Milan Zamazal wrote: > Hi Vasiliy, > > thank you for the fix. > > It should be set according to gainInfo > (https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/simple/soft_simple.cpp#n201) > in this case. Do you mean the case when gainInfo doesn't contain > meaningful values? Maybe something broken in gainInfo handling, because v4l2-ctl shows me analogue_gain 0x009e0903 (int) : min=0 max=480 step=1 default=0 value=100 flags=has-min-max But libcamera reports that min gain is 100 and max 480. I think we need to set againMin to 0 or calculate somehow again10 in src/ipa/simple/soft_simple.cpp:252 if (againMin) { context_.configuration.agc.againMin = againMin; } else { LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear"; context_.configuration.agc.againMin = std::min(100, againMin / 2 + againMax / 2); }
On 1/5/26 1:50 PM, johannes.goede@oss.qualcomm.com wrote: > Hi, > > On 5-Jan-26 11:44, Vasiliy Doylov wrote: >> On 1/5/26 1:26 PM, johannes.goede@oss.qualcomm.com wrote: >>> Hi, >>> >>> Thank you for your patch. >>> >>> ... >>> >>> So I guess you are hitting the case where againMin == 0 >> My setup: >> >> Oneplus 6 (oneplus-enchilada) >> >> Rear wide camera imx376 (which has no cam helper) >> >> againMax = 480 >> >> againMin = 100 >> >> again10 is set to 0 (for some reason?) > If there is no helper, then again10 gets set to againDef > which comes from this lines in src/ipa/simple/soft_simple.cpp: > > int32_t againDef = gainInfo.def().get<int32_t>(); > > context_.configuration.agc.again10 = againDef; > > I guess that your imx376 driver may have a bug and is > reporting a default value outside of the min max range. > > What is the output of: > > v4l2-ctl -d /dev/v4l-subdev# -l analogue_gain 0x009e0903 (int) : min=0 max=480 step=1 default=0 value=100 flags=has-min-max Than again10 is ok. againMin is shifted up here. So i think we need to also shift again10 src/ipa/simple/soft_simple.cpp:252 ``` if (againMin) { context_.configuration.agc.againMin = againMin; } else { LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear"; context_.configuration.agc.againMin = std::min(100, againMin / 2 + againMax / 2); } ``` > > > p.s. > > I could not find an imx376 driver in the mainline kernel > sources, so I guess you are using an out of tree driver ? Yep, it's on sdm845-mainline (close to mainline tree)
Removed that shift and i see no bad impact on result image. Was that shift an edge case for something?
Hi, On 5-Jan-26 11:54, Vasiliy Doylov wrote: > > On 1/5/26 1:50 PM, johannes.goede@oss.qualcomm.com wrote: >> Hi, >> >> On 5-Jan-26 11:44, Vasiliy Doylov wrote: >>> On 1/5/26 1:26 PM, johannes.goede@oss.qualcomm.com wrote: >>>> Hi, >>>> >>>> Thank you for your patch. >>>> >>>> ... >>>> >>>> So I guess you are hitting the case where againMin == 0 >>> My setup: >>> >>> Oneplus 6 (oneplus-enchilada) >>> >>> Rear wide camera imx376 (which has no cam helper) >>> >>> againMax = 480 >>> >>> againMin = 100 >>> >>> again10 is set to 0 (for some reason?) >> If there is no helper, then again10 gets set to againDef >> which comes from this lines in src/ipa/simple/soft_simple.cpp: >> >> int32_t againDef = gainInfo.def().get<int32_t>(); >> >> context_.configuration.agc.again10 = againDef; >> >> I guess that your imx376 driver may have a bug and is >> reporting a default value outside of the min max range. >> >> What is the output of: >> >> v4l2-ctl -d /dev/v4l-subdev# -l > analogue_gain 0x009e0903 (int) : min=0 max=480 step=1 default=0 value=100 flags=has-min-max > Than again10 is ok. againMin is shifted up here. So i think we need to also shift again10 > src/ipa/simple/soft_simple.cpp:252 > ``` > if (againMin) { > context_.configuration.agc.againMin = againMin; > } else { > LOG(IPASoft, Warning) > << "Minimum gain is zero, that can't be linear"; > context_.configuration.agc.againMin = > std::min(100, againMin / 2 + againMax / 2); > } > ``` Right, so you are indeed hitting the againMin == 0 path and then the code decides to override againMin. Which is what I expected. If I'm reading the sensor-helper class code correct then on most imx sensors the formula seems to be: again_float = 512.0 / (512.0 - again_ctrl_value). so the default of 0 makes sense and actually translates to a gain of 1.0. And the max gain of 400 leads to 512.0 / 112.0 = 4.57 which is about typical for a max sensor gain. So as I already suspected but I never found a way to test the above code block you quoted: if (againMin) { context_.configuration.agc.againMin = againMin; } else { LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear"; context_.configuration.agc.againMin = std::min(100, againMin / 2 + againMax / 2); } is non-sense and the fix is to replace this entire block with just: context_.configuration.agc.againMin = againMin; Regards, Hans
Hi, On 5-Jan-26 12:12, Vasiliy Doylov wrote: > Removed that shift and i see no bad impact on result image. > > Was that shift an edge case for something? See the reply which I just send, this code was inherited from the very first version of the softISP and it never really seemed right to me. I think we should just drop the code which changes the minimum gain when it is 0. Regards, Hans
Should i send v2 with this drop? On 1/5/26 2:15 PM, johannes.goede@oss.qualcomm.com wrote: > Hi, > > On 5-Jan-26 12:12, Vasiliy Doylov wrote: >> Removed that shift and i see no bad impact on result image. >> >> Was that shift an edge case for something? > See the reply which I just send, this code was inherited > from the very first version of the softISP and it never > really seemed right to me. I think we should just drop > the code which changes the minimum gain when it is 0. > > Regards, > > Hans >
Hi, On 5-Jan-26 12:15, Vasiliy Doylov wrote: > Should i send v2 with this drop? Yes please, you can use my email bit about typical imx analog gain ctrl-value to gain-factor formula and how gainMin == 0 is totally valid there for the commit message. Regards, Hans > > On 1/5/26 2:15 PM, johannes.goede@oss.qualcomm.com wrote: >> Hi, >> >> On 5-Jan-26 12:12, Vasiliy Doylov wrote: >>> Removed that shift and i see no bad impact on result image. >>> >>> Was that shift an edge case for something? >> See the reply which I just send, this code was inherited >> from the very first version of the softISP and it never >> really seemed right to me. I think we should just drop >> the code which changes the minimum gain when it is 0. >> >> Regards, >> >> Hans >>
diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index 189de770..2972844b 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -71,7 +71,8 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou } if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { - if (again > context.configuration.agc.again10) { + if (again > context.configuration.agc.again10 && + again > context.configuration.agc.againMin) { double next = again * kExpNumeratorDown / kExpDenominator; if (again - next < context.configuration.agc.againMinStep) again -= context.configuration.agc.againMinStep;
If camera sensor helper is not set, again10 will be 0. It will prevent exposure change, because again will be clamped to againMin and condition again > again10 will always be true. Exposure decrease change will never occur. Fixed by adding again > againMin check. Signed-off-by: Vasiliy Doylov <nekocwd@mainlining.org> --- src/ipa/simple/algorithms/agc.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) --- base-commit: 2861817f09c96a0660b88783eff159631e42030f change-id: 20260105-softisp-agc-16d01cb3da49 Best regards,