| Message ID | 20250923190657.21453-3-hansg@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hans, thank you for the patch. Hans de Goede <hansg@kernel.org> writes: > At the moment when the overall image brightness is considered too high > the AGC code will lower the gain all the way down to againMin before > considering lowering the exposure. > > What should happen instead is lower the gain no lower then its default s/then/than/ > ctrl value and after that lower the exposure instead of lowering > the gain. > > Otherwise there might be a heavily overexposed image (e.g. all white) > which then is made less white by a very low gain which is no good. Do I understand it correctly that the gain should never be set below the default value and bright images can then be fully handled by reducing exposure? > While at it also remove the weird limitation to only lower the gain > when exposure is set to the maximum. As long as the gain is higher > then the default gain, the gain should be lowered first. s/then/than/ > Signed-off-by: Hans de Goede <hansg@kernel.org> > --- > src/ipa/simple/algorithms/agc.cpp | 3 +-- > src/ipa/simple/ipa_context.h | 2 +- > src/ipa/simple/soft_simple.cpp | 3 +++ > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index c46bb0ebe..94961f9fe 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou > } > > if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { > - if (exposure == context.configuration.agc.exposureMax && > - again > context.configuration.agc.againMin) { > + if (again > context.configuration.agc.againDef) { > next = again * kExpNumeratorDown / kExpDenominator; > if (again - next < context.configuration.agc.againMinStep) > again -= context.configuration.agc.againMinStep; > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > index a471b80ae..ba525a881 100644 > --- a/src/ipa/simple/ipa_context.h > +++ b/src/ipa/simple/ipa_context.h > @@ -28,7 +28,7 @@ struct IPASessionConfiguration { > float gamma; > struct { > int32_t exposureMin, exposureMax; > - double againMin, againMax, againMinStep; > + double againMin, againMax, againDef, againMinStep; > utils::Duration lineDuration; > } agc; > struct { > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index e70439ee5..f0764ef46 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > > int32_t againMin = gainInfo.min().get<int32_t>(); > int32_t againMax = gainInfo.max().get<int32_t>(); > + int32_t againDef = gainInfo.def().get<int32_t>(); > > if (camHelper_) { > context_.configuration.agc.againMin = camHelper_->gain(againMin); > context_.configuration.agc.againMax = camHelper_->gain(againMax); > + context_.configuration.agc.againDef = camHelper_->gain(againDef); > context_.configuration.agc.againMinStep = > (context_.configuration.agc.againMax - > context_.configuration.agc.againMin) / > @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > * other) we limit the range of the gain values used. > */ > context_.configuration.agc.againMax = againMax; > + context_.configuration.agc.againDef = againDef; > if (againMin) { > context_.configuration.agc.againMin = againMin; > } else {
Hi Hans, Thank you for the patch! Quoting Hans de Goede (2025-09-23 20:06:54) > At the moment when the overall image brightness is considered too high > the AGC code will lower the gain all the way down to againMin before > considering lowering the exposure. > > What should happen instead is lower the gain no lower then its default > ctrl value and after that lower the exposure instead of lowering > the gain. I may be wrong, but what if the default gain is something like 2.0, wouldn't that mean the light levels received at maximum exposure would be multiplied by 2? While I agree that using the absolute minimum is probably a bad idea (as even if it goes < 1, we're still artificially modifying the data), I think we maybe should be able to reduce the analogue gain to 1.0 at a minimum before touching exposure, to ensure we're making the most of the exposure we have. Best wishes, Isaac > > Otherwise there might be a heavily overexposed image (e.g. all white) > which then is made less white by a very low gain which is no good. > > While at it also remove the weird limitation to only lower the gain > when exposure is set to the maximum. As long as the gain is higher > then the default gain, the gain should be lowered first. > > Signed-off-by: Hans de Goede <hansg@kernel.org> > --- > src/ipa/simple/algorithms/agc.cpp | 3 +-- > src/ipa/simple/ipa_context.h | 2 +- > src/ipa/simple/soft_simple.cpp | 3 +++ > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > index c46bb0ebe..94961f9fe 100644 > --- a/src/ipa/simple/algorithms/agc.cpp > +++ b/src/ipa/simple/algorithms/agc.cpp > @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou > } > > if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { > - if (exposure == context.configuration.agc.exposureMax && > - again > context.configuration.agc.againMin) { > + if (again > context.configuration.agc.againDef) { > next = again * kExpNumeratorDown / kExpDenominator; > if (again - next < context.configuration.agc.againMinStep) > again -= context.configuration.agc.againMinStep; > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > index a471b80ae..ba525a881 100644 > --- a/src/ipa/simple/ipa_context.h > +++ b/src/ipa/simple/ipa_context.h > @@ -28,7 +28,7 @@ struct IPASessionConfiguration { > float gamma; > struct { > int32_t exposureMin, exposureMax; > - double againMin, againMax, againMinStep; > + double againMin, againMax, againDef, againMinStep; > utils::Duration lineDuration; > } agc; > struct { > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index e70439ee5..f0764ef46 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > > int32_t againMin = gainInfo.min().get<int32_t>(); > int32_t againMax = gainInfo.max().get<int32_t>(); > + int32_t againDef = gainInfo.def().get<int32_t>(); > > if (camHelper_) { > context_.configuration.agc.againMin = camHelper_->gain(againMin); > context_.configuration.agc.againMax = camHelper_->gain(againMax); > + context_.configuration.agc.againDef = camHelper_->gain(againDef); > context_.configuration.agc.againMinStep = > (context_.configuration.agc.againMax - > context_.configuration.agc.againMin) / > @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > * other) we limit the range of the gain values used. > */ > context_.configuration.agc.againMax = againMax; > + context_.configuration.agc.againDef = againDef; > if (againMin) { > context_.configuration.agc.againMin = againMin; > } else { > -- > 2.51.0 >
Hi Milan, On 24-Sep-25 09:54, Milan Zamazal wrote: > Hi Hans, > > thank you for the patch. > > Hans de Goede <hansg@kernel.org> writes: > >> At the moment when the overall image brightness is considered too high >> the AGC code will lower the gain all the way down to againMin before >> considering lowering the exposure. >> >> What should happen instead is lower the gain no lower then its default > > s/then/than/ Ack. >> ctrl value and after that lower the exposure instead of lowering >> the gain. >> >> Otherwise there might be a heavily overexposed image (e.g. all white) >> which then is made less white by a very low gain which is no good. > > Do I understand it correctly that the gain should never be set below the > default value and bright images can then be fully handled by reducing exposure? Yes basically the pipeline for a single pixel looks like this: light-sensor -> gain -> adc Where the light-sensor gives of an analog signal between black-level and max. Now lets say initially there is a lens-cap covering the sensor, so AGC boosts exposure and gain to their max values and the room is brightly lit with daylight, so after removing the cap the image brightness is way too high. Now with the max exposure pretty much all pixels will be outputting their max analog signal. So changing the gain to < 1.0 (if supported) will result in the frames going from fully white to light gray to 50% gray at which point the overall brightness measures as ok, but we don't have a proper picture. Where as we lower the gain from max-gain to default gain which typically is ~1.0 at which point the resulting picture is still full white (heavily overexposed) and then start lowering the exposure will result in a proper picture. Actually thinking more about this, sensors where the min-gain advertised by the sensor-driver is less then 1.0 is probably why we have the weird code to come up with some alternative min-value when the v4l2-ctrl reports 0 as min value. E.g. many ov sensors have a linear gain where ctrl-value / 128 is the gain value. So setting the ctrl to 128 results in a gain of 1.0 . Most of these report a min-value of 128, as well as a default value of 128. But I'm pretty sure the hw will accept 0 just fine too and then set a gain to 0.0 (not really useful), or accept 64 for a gain of 0.5 . I think the magic min-gain == 0 handling is based on sensor drivers which do allow control of the full gain range (this was inherited from Andrey's work). So if we go with this change then we can also remove that weird magic handling of min-gain == 0. >> While at it also remove the weird limitation to only lower the gain >> when exposure is set to the maximum. As long as the gain is higher >> then the default gain, the gain should be lowered first. > > s/then/than/ Ack. Regards, Hans > >> Signed-off-by: Hans de Goede <hansg@kernel.org> >> --- >> src/ipa/simple/algorithms/agc.cpp | 3 +-- >> src/ipa/simple/ipa_context.h | 2 +- >> src/ipa/simple/soft_simple.cpp | 3 +++ >> 3 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >> index c46bb0ebe..94961f9fe 100644 >> --- a/src/ipa/simple/algorithms/agc.cpp >> +++ b/src/ipa/simple/algorithms/agc.cpp >> @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou >> } >> >> if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { >> - if (exposure == context.configuration.agc.exposureMax && >> - again > context.configuration.agc.againMin) { >> + if (again > context.configuration.agc.againDef) { >> next = again * kExpNumeratorDown / kExpDenominator; >> if (again - next < context.configuration.agc.againMinStep) >> again -= context.configuration.agc.againMinStep; >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >> index a471b80ae..ba525a881 100644 >> --- a/src/ipa/simple/ipa_context.h >> +++ b/src/ipa/simple/ipa_context.h >> @@ -28,7 +28,7 @@ struct IPASessionConfiguration { >> float gamma; >> struct { >> int32_t exposureMin, exposureMax; >> - double againMin, againMax, againMinStep; >> + double againMin, againMax, againDef, againMinStep; >> utils::Duration lineDuration; >> } agc; >> struct { >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index e70439ee5..f0764ef46 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >> >> int32_t againMin = gainInfo.min().get<int32_t>(); >> int32_t againMax = gainInfo.max().get<int32_t>(); >> + int32_t againDef = gainInfo.def().get<int32_t>(); >> >> if (camHelper_) { >> context_.configuration.agc.againMin = camHelper_->gain(againMin); >> context_.configuration.agc.againMax = camHelper_->gain(againMax); >> + context_.configuration.agc.againDef = camHelper_->gain(againDef); >> context_.configuration.agc.againMinStep = >> (context_.configuration.agc.againMax - >> context_.configuration.agc.againMin) / >> @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >> * other) we limit the range of the gain values used. >> */ >> context_.configuration.agc.againMax = againMax; >> + context_.configuration.agc.againDef = againDef; >> if (againMin) { >> context_.configuration.agc.againMin = againMin; >> } else { >
Hi Isaac, On 25-Sep-25 12:03, Isaac Scott wrote: > Hi Hans, > > Thank you for the patch! > > Quoting Hans de Goede (2025-09-23 20:06:54) >> At the moment when the overall image brightness is considered too high >> the AGC code will lower the gain all the way down to againMin before >> considering lowering the exposure. >> >> What should happen instead is lower the gain no lower then its default >> ctrl value and after that lower the exposure instead of lowering >> the gain. > > I may be wrong, but what if the default gain is something like 2.0, > wouldn't that mean the light levels received at maximum exposure would > be multiplied by 2? > > While I agree that using the absolute minimum is probably a bad idea (as > even if it goes < 1, we're still artificially modifying the data), I > think we maybe should be able to reduce the analogue gain to 1.0 at a > minimum before touching exposure, to ensure we're making the most of the > exposure we have. I agree 1.0 is the value we want to use. But if we don't have the sensor listed in the sensor-helper then we don't now what value corresponds to a gain of 1.0 and then using the default gain is a better option then using the min-gain, also see my longer reply to Milan. I do think that using 1.0 when we do have the helper is best. So I'll change that for the next version. Regards, Hans >> Otherwise there might be a heavily overexposed image (e.g. all white) >> which then is made less white by a very low gain which is no good. >> >> While at it also remove the weird limitation to only lower the gain >> when exposure is set to the maximum. As long as the gain is higher >> then the default gain, the gain should be lowered first. >> >> Signed-off-by: Hans de Goede <hansg@kernel.org> >> --- >> src/ipa/simple/algorithms/agc.cpp | 3 +-- >> src/ipa/simple/ipa_context.h | 2 +- >> src/ipa/simple/soft_simple.cpp | 3 +++ >> 3 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >> index c46bb0ebe..94961f9fe 100644 >> --- a/src/ipa/simple/algorithms/agc.cpp >> +++ b/src/ipa/simple/algorithms/agc.cpp >> @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou >> } >> >> if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { >> - if (exposure == context.configuration.agc.exposureMax && >> - again > context.configuration.agc.againMin) { >> + if (again > context.configuration.agc.againDef) { >> next = again * kExpNumeratorDown / kExpDenominator; >> if (again - next < context.configuration.agc.againMinStep) >> again -= context.configuration.agc.againMinStep; >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >> index a471b80ae..ba525a881 100644 >> --- a/src/ipa/simple/ipa_context.h >> +++ b/src/ipa/simple/ipa_context.h >> @@ -28,7 +28,7 @@ struct IPASessionConfiguration { >> float gamma; >> struct { >> int32_t exposureMin, exposureMax; >> - double againMin, againMax, againMinStep; >> + double againMin, againMax, againDef, againMinStep; >> utils::Duration lineDuration; >> } agc; >> struct { >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index e70439ee5..f0764ef46 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >> >> int32_t againMin = gainInfo.min().get<int32_t>(); >> int32_t againMax = gainInfo.max().get<int32_t>(); >> + int32_t againDef = gainInfo.def().get<int32_t>(); >> >> if (camHelper_) { >> context_.configuration.agc.againMin = camHelper_->gain(againMin); >> context_.configuration.agc.againMax = camHelper_->gain(againMax); >> + context_.configuration.agc.againDef = camHelper_->gain(againDef); >> context_.configuration.agc.againMinStep = >> (context_.configuration.agc.againMax - >> context_.configuration.agc.againMin) / >> @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >> * other) we limit the range of the gain values used. >> */ >> context_.configuration.agc.againMax = againMax; >> + context_.configuration.agc.againDef = againDef; >> if (againMin) { >> context_.configuration.agc.againMin = againMin; >> } else { >> -- >> 2.51.0 >>
Quoting Isaac Scott (2025-09-25 11:03:48) > Hi Hans, > > Thank you for the patch! > > Quoting Hans de Goede (2025-09-23 20:06:54) > > At the moment when the overall image brightness is considered too high > > the AGC code will lower the gain all the way down to againMin before > > considering lowering the exposure. > > > > What should happen instead is lower the gain no lower then its default > > ctrl value and after that lower the exposure instead of lowering > > the gain. Have you encountered sensors where againMin != default ? What does the sensor driver convey in these instances? (What would min be and what would default be?) > I may be wrong, but what if the default gain is something like 2.0, > wouldn't that mean the light levels received at maximum exposure would > be multiplied by 2? Indeed, I don't think we should ever go below 1.0x gain. But I think the issue here is that without a gain model - we don't know what a 1.0x gain is - so I anticipate we're 'hoping' that the default is always a gain code that corresponds to 1.0x. We probably don't have anything else to convey that here so I think this is actually correct given the context here. > While I agree that using the absolute minimum is probably a bad idea (as > even if it goes < 1, we're still artificially modifying the data), I > think we maybe should be able to reduce the analogue gain to 1.0 at a > minimum before touching exposure, to ensure we're making the most of the > exposure we have. I think it's likely rare to have a sensor with an analogue gain less than 1.0. I don't think I've seen one - has anyone else? > > Best wishes, > Isaac > > > > > Otherwise there might be a heavily overexposed image (e.g. all white) > > which then is made less white by a very low gain which is no good. > > > > While at it also remove the weird limitation to only lower the gain > > when exposure is set to the maximum. As long as the gain is higher > > then the default gain, the gain should be lowered first. Presumably the gain is only /increased/ after the exposure reaches it's maximum ? But I suspect this is fine too. > > > > Signed-off-by: Hans de Goede <hansg@kernel.org> > > --- > > src/ipa/simple/algorithms/agc.cpp | 3 +-- > > src/ipa/simple/ipa_context.h | 2 +- > > src/ipa/simple/soft_simple.cpp | 3 +++ > > 3 files changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > > index c46bb0ebe..94961f9fe 100644 > > --- a/src/ipa/simple/algorithms/agc.cpp > > +++ b/src/ipa/simple/algorithms/agc.cpp > > @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou > > } > > > > if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { > > - if (exposure == context.configuration.agc.exposureMax && > > - again > context.configuration.agc.againMin) { > > + if (again > context.configuration.agc.againDef) { > > next = again * kExpNumeratorDown / kExpDenominator; > > if (again - next < context.configuration.agc.againMinStep) > > again -= context.configuration.agc.againMinStep; > > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > > index a471b80ae..ba525a881 100644 > > --- a/src/ipa/simple/ipa_context.h > > +++ b/src/ipa/simple/ipa_context.h > > @@ -28,7 +28,7 @@ struct IPASessionConfiguration { > > float gamma; > > struct { > > int32_t exposureMin, exposureMax; > > - double againMin, againMax, againMinStep; > > + double againMin, againMax, againDef, againMinStep; > > utils::Duration lineDuration; > > } agc; > > struct { > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > > index e70439ee5..f0764ef46 100644 > > --- a/src/ipa/simple/soft_simple.cpp > > +++ b/src/ipa/simple/soft_simple.cpp > > @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > > > > int32_t againMin = gainInfo.min().get<int32_t>(); > > int32_t againMax = gainInfo.max().get<int32_t>(); > > + int32_t againDef = gainInfo.def().get<int32_t>(); > > > > if (camHelper_) { > > context_.configuration.agc.againMin = camHelper_->gain(againMin); > > context_.configuration.agc.againMax = camHelper_->gain(againMax); > > + context_.configuration.agc.againDef = camHelper_->gain(againDef); > > context_.configuration.agc.againMinStep = > > (context_.configuration.agc.againMax - > > context_.configuration.agc.againMin) / > > @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > > * other) we limit the range of the gain values used. > > */ > > context_.configuration.agc.againMax = againMax; > > + context_.configuration.agc.againDef = againDef; > > if (againMin) { > > context_.configuration.agc.againMin = againMin; > > } else { > > -- > > 2.51.0 > >
Hi, On 25-Sep-25 14:10, Kieran Bingham wrote: > Quoting Isaac Scott (2025-09-25 11:03:48) >> Hi Hans, >> >> Thank you for the patch! >> >> Quoting Hans de Goede (2025-09-23 20:06:54) >>> At the moment when the overall image brightness is considered too high >>> the AGC code will lower the gain all the way down to againMin before >>> considering lowering the exposure. >>> >>> What should happen instead is lower the gain no lower then its default >>> ctrl value and after that lower the exposure instead of lowering >>> the gain. > > Have you encountered sensors where againMin != default ? Yes for example for the ov2680 the V4L2_CID_ANALOGUE_GAIN range is 0 - 1023 and the default gain is 250. > What does the sensor driver convey in these instances? (What would min > be and what would default be?) minimum gain is the minimum value the register accepts without behaving unexpectedly. Default gain should correspond to 1.0 . > >> I may be wrong, but what if the default gain is something like 2.0, >> wouldn't that mean the light levels received at maximum exposure would >> be multiplied by 2? > > Indeed, I don't think we should ever go below 1.0x gain. But I think the > issue here is that without a gain model - we don't know what a 1.0x gain > is - so I anticipate we're 'hoping' that the default is always a gain > code that corresponds to 1.0x. Right that is the idea, that default-gain == 1.0 Note many sensor drivers indeed use min-gain == def-gain in which case this patch is a no-op. > We probably don't have anything else to convey that here so I think this > is actually correct given the context here. Right. >> While I agree that using the absolute minimum is probably a bad idea (as >> even if it goes < 1, we're still artificially modifying the data), I >> think we maybe should be able to reduce the analogue gain to 1.0 at a >> minimum before touching exposure, to ensure we're making the most of the >> exposure we have. > > I think it's likely rare to have a sensor with an analogue gain less > than 1.0. I don't think I've seen one - has anyone else? I think (need to check) 0 on the ov2680 is actually 0. Another example is the ov2740 which according to the sensor-helper code in libcamera has a gain formula of: gain = gain-ctrl-reg / 128 have a gain-ctrl range exposed on V4L2_CID_ANALOGUE_GAIN of 128 - 1983, which gets written directly to the register. According to the formula above this means 1.0 - 15.5 . But I wonder if the register won't accept lower values and then do a gain below 1.0. The 1.0 / 128 min is currently enforced by the driver. But that does not mean the hw won't accept lower values. Let me give this a test-run on an ov08x40 and get back to you. >>> Otherwise there might be a heavily overexposed image (e.g. all white) >>> which then is made less white by a very low gain which is no good. >>> >>> While at it also remove the weird limitation to only lower the gain >>> when exposure is set to the maximum. As long as the gain is higher >>> then the default gain, the gain should be lowered first. > > Presumably the gain is only /increased/ after the exposure reaches it's > maximum ? When done by AGC yes, but libcamera atm inherits whatever control values were set at libcamera startup which may use a gain > 1.0 while not being at exposure max. Regards, Hans >>> >>> Signed-off-by: Hans de Goede <hansg@kernel.org> >>> --- >>> src/ipa/simple/algorithms/agc.cpp | 3 +-- >>> src/ipa/simple/ipa_context.h | 2 +- >>> src/ipa/simple/soft_simple.cpp | 3 +++ >>> 3 files changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >>> index c46bb0ebe..94961f9fe 100644 >>> --- a/src/ipa/simple/algorithms/agc.cpp >>> +++ b/src/ipa/simple/algorithms/agc.cpp >>> @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou >>> } >>> >>> if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { >>> - if (exposure == context.configuration.agc.exposureMax && >>> - again > context.configuration.agc.againMin) { >>> + if (again > context.configuration.agc.againDef) { >>> next = again * kExpNumeratorDown / kExpDenominator; >>> if (again - next < context.configuration.agc.againMinStep) >>> again -= context.configuration.agc.againMinStep; >>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >>> index a471b80ae..ba525a881 100644 >>> --- a/src/ipa/simple/ipa_context.h >>> +++ b/src/ipa/simple/ipa_context.h >>> @@ -28,7 +28,7 @@ struct IPASessionConfiguration { >>> float gamma; >>> struct { >>> int32_t exposureMin, exposureMax; >>> - double againMin, againMax, againMinStep; >>> + double againMin, againMax, againDef, againMinStep; >>> utils::Duration lineDuration; >>> } agc; >>> struct { >>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >>> index e70439ee5..f0764ef46 100644 >>> --- a/src/ipa/simple/soft_simple.cpp >>> +++ b/src/ipa/simple/soft_simple.cpp >>> @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >>> >>> int32_t againMin = gainInfo.min().get<int32_t>(); >>> int32_t againMax = gainInfo.max().get<int32_t>(); >>> + int32_t againDef = gainInfo.def().get<int32_t>(); >>> >>> if (camHelper_) { >>> context_.configuration.agc.againMin = camHelper_->gain(againMin); >>> context_.configuration.agc.againMax = camHelper_->gain(againMax); >>> + context_.configuration.agc.againDef = camHelper_->gain(againDef); >>> context_.configuration.agc.againMinStep = >>> (context_.configuration.agc.againMax - >>> context_.configuration.agc.againMin) / >>> @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >>> * other) we limit the range of the gain values used. >>> */ >>> context_.configuration.agc.againMax = againMax; >>> + context_.configuration.agc.againDef = againDef; >>> if (againMin) { >>> context_.configuration.agc.againMin = againMin; >>> } else { >>> -- >>> 2.51.0 >>>
Hi, On 25-Sep-25 14:37, Hans de Goede wrote: > Hi, > > On 25-Sep-25 14:10, Kieran Bingham wrote: >> Quoting Isaac Scott (2025-09-25 11:03:48) >>> Hi Hans, >>> >>> Thank you for the patch! >>> >>> Quoting Hans de Goede (2025-09-23 20:06:54) >>>> At the moment when the overall image brightness is considered too high >>>> the AGC code will lower the gain all the way down to againMin before >>>> considering lowering the exposure. >>>> >>>> What should happen instead is lower the gain no lower then its default >>>> ctrl value and after that lower the exposure instead of lowering >>>> the gain. >> >> Have you encountered sensors where againMin != default ? > > Yes for example for the ov2680 the V4L2_CID_ANALOGUE_GAIN range is > 0 - 1023 and the default gain is 250. > >> What does the sensor driver convey in these instances? (What would min >> be and what would default be?) > > minimum gain is the minimum value the register accepts without behaving > unexpectedly. Default gain should correspond to 1.0 . > >> >>> I may be wrong, but what if the default gain is something like 2.0, >>> wouldn't that mean the light levels received at maximum exposure would >>> be multiplied by 2? >> >> Indeed, I don't think we should ever go below 1.0x gain. But I think the >> issue here is that without a gain model - we don't know what a 1.0x gain >> is - so I anticipate we're 'hoping' that the default is always a gain >> code that corresponds to 1.0x. > > Right that is the idea, that default-gain == 1.0 Note many sensor drivers > indeed use min-gain == def-gain in which case this patch is a no-op. > >> We probably don't have anything else to convey that here so I think this >> is actually correct given the context here. > > Right. > >>> While I agree that using the absolute minimum is probably a bad idea (as >>> even if it goes < 1, we're still artificially modifying the data), I >>> think we maybe should be able to reduce the analogue gain to 1.0 at a >>> minimum before touching exposure, to ensure we're making the most of the >>> exposure we have. >> >> I think it's likely rare to have a sensor with an analogue gain less >> than 1.0. I don't think I've seen one - has anyone else? > > I think (need to check) 0 on the ov2680 is actually 0. > > Another example is the ov2740 which according to the sensor-helper code in > libcamera has a gain formula of: > > gain = gain-ctrl-reg / 128 > > have a gain-ctrl range exposed on V4L2_CID_ANALOGUE_GAIN of 128 - 1983, > which gets written directly to the register. According to the formula above > this means 1.0 - 15.5 . But I wonder if the register won't accept lower > values and then do a gain below 1.0. The 1.0 / 128 min is currently enforced > by the driver. But that does not mean the hw won't accept lower values. > > Let me give this a test-run on an ov08x40 and get back to you. So I modified the ov08x40 driver to allow values less then 128 and gave those values a try. On the ov08x40 sensor gain values below 128 result in the gain going up (so above 1.0) rather then down. Regards, Hans >>>> Signed-off-by: Hans de Goede <hansg@kernel.org> >>>> --- >>>> src/ipa/simple/algorithms/agc.cpp | 3 +-- >>>> src/ipa/simple/ipa_context.h | 2 +- >>>> src/ipa/simple/soft_simple.cpp | 3 +++ >>>> 3 files changed, 5 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >>>> index c46bb0ebe..94961f9fe 100644 >>>> --- a/src/ipa/simple/algorithms/agc.cpp >>>> +++ b/src/ipa/simple/algorithms/agc.cpp >>>> @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou >>>> } >>>> >>>> if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { >>>> - if (exposure == context.configuration.agc.exposureMax && >>>> - again > context.configuration.agc.againMin) { >>>> + if (again > context.configuration.agc.againDef) { >>>> next = again * kExpNumeratorDown / kExpDenominator; >>>> if (again - next < context.configuration.agc.againMinStep) >>>> again -= context.configuration.agc.againMinStep; >>>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >>>> index a471b80ae..ba525a881 100644 >>>> --- a/src/ipa/simple/ipa_context.h >>>> +++ b/src/ipa/simple/ipa_context.h >>>> @@ -28,7 +28,7 @@ struct IPASessionConfiguration { >>>> float gamma; >>>> struct { >>>> int32_t exposureMin, exposureMax; >>>> - double againMin, againMax, againMinStep; >>>> + double againMin, againMax, againDef, againMinStep; >>>> utils::Duration lineDuration; >>>> } agc; >>>> struct { >>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >>>> index e70439ee5..f0764ef46 100644 >>>> --- a/src/ipa/simple/soft_simple.cpp >>>> +++ b/src/ipa/simple/soft_simple.cpp >>>> @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >>>> >>>> int32_t againMin = gainInfo.min().get<int32_t>(); >>>> int32_t againMax = gainInfo.max().get<int32_t>(); >>>> + int32_t againDef = gainInfo.def().get<int32_t>(); >>>> >>>> if (camHelper_) { >>>> context_.configuration.agc.againMin = camHelper_->gain(againMin); >>>> context_.configuration.agc.againMax = camHelper_->gain(againMax); >>>> + context_.configuration.agc.againDef = camHelper_->gain(againDef); >>>> context_.configuration.agc.againMinStep = >>>> (context_.configuration.agc.againMax - >>>> context_.configuration.agc.againMin) / >>>> @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >>>> * other) we limit the range of the gain values used. >>>> */ >>>> context_.configuration.agc.againMax = againMax; >>>> + context_.configuration.agc.againDef = againDef; >>>> if (againMin) { >>>> context_.configuration.agc.againMin = againMin; >>>> } else { >>>> -- >>>> 2.51.0 >>>> >
Hans de Goede <hansg@kernel.org> writes: > Hi Milan, > > On 24-Sep-25 09:54, Milan Zamazal wrote: >> Hi Hans, >> >> thank you for the patch. >> >> Hans de Goede <hansg@kernel.org> writes: >> >>> At the moment when the overall image brightness is considered too high >>> the AGC code will lower the gain all the way down to againMin before >>> considering lowering the exposure. >>> >>> What should happen instead is lower the gain no lower then its default >> >> s/then/than/ > > Ack. > >>> ctrl value and after that lower the exposure instead of lowering >>> the gain. >>> >>> Otherwise there might be a heavily overexposed image (e.g. all white) >>> which then is made less white by a very low gain which is no good. >> >> Do I understand it correctly that the gain should never be set below the >> default value and bright images can then be fully handled by reducing exposure? > > Yes basically the pipeline for a single pixel looks like this: > > light-sensor -> gain -> adc > > Where the light-sensor gives of an analog signal between > black-level and max. > > Now lets say initially there is a lens-cap covering the sensor, so AGC > boosts exposure and gain to their max values and the room is brightly lit > with daylight, so after removing the cap the image brightness is way too > high. > > Now with the max exposure pretty much all pixels will be outputting > their max analog signal. So changing the gain to < 1.0 (if supported) > will result in the frames going from fully white to light gray > to 50% gray at which point the overall brightness measures as ok, > but we don't have a proper picture. I assume this applies also to situations where exposure is at the minimum value but the image is so bright that a sensor with the default gain is still saturated, right? Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > Where as we lower the gain from max-gain to default gain which typically > is ~1.0 at which point the resulting picture is still full white > (heavily overexposed) and then start lowering the exposure will result > in a proper picture. > > Actually thinking more about this, sensors where the min-gain advertised > by the sensor-driver is less then 1.0 is probably why we have the weird > code to come up with some alternative min-value when the v4l2-ctrl reports > 0 as min value. E.g. many ov sensors have a linear gain where ctrl-value / 128 > is the gain value. So setting the ctrl to 128 results in a gain of 1.0 . > Most of these report a min-value of 128, as well as a default value of > 128. But I'm pretty sure the hw will accept 0 just fine too and then > set a gain to 0.0 (not really useful), or accept 64 for a gain of 0.5 . > > I think the magic min-gain == 0 handling is based on sensor drivers > which do allow control of the full gain range (this was inherited from > Andrey's work). So if we go with this change then we can also remove > that weird magic handling of min-gain == 0. > >>> While at it also remove the weird limitation to only lower the gain >>> when exposure is set to the maximum. As long as the gain is higher >>> then the default gain, the gain should be lowered first. >> >> s/then/than/ > > Ack. > > Regards, > > Hans > > > >> >>> Signed-off-by: Hans de Goede <hansg@kernel.org> >>> --- >>> src/ipa/simple/algorithms/agc.cpp | 3 +-- >>> src/ipa/simple/ipa_context.h | 2 +- >>> src/ipa/simple/soft_simple.cpp | 3 +++ >>> 3 files changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >>> index c46bb0ebe..94961f9fe 100644 >>> --- a/src/ipa/simple/algorithms/agc.cpp >>> +++ b/src/ipa/simple/algorithms/agc.cpp >>> @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou >>> } >>> >>> if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { >>> - if (exposure == context.configuration.agc.exposureMax && >>> - again > context.configuration.agc.againMin) { >>> + if (again > context.configuration.agc.againDef) { >>> next = again * kExpNumeratorDown / kExpDenominator; >>> if (again - next < context.configuration.agc.againMinStep) >>> again -= context.configuration.agc.againMinStep; >>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >>> index a471b80ae..ba525a881 100644 >>> --- a/src/ipa/simple/ipa_context.h >>> +++ b/src/ipa/simple/ipa_context.h >>> @@ -28,7 +28,7 @@ struct IPASessionConfiguration { >>> float gamma; >>> struct { >>> int32_t exposureMin, exposureMax; >>> - double againMin, againMax, againMinStep; >>> + double againMin, againMax, againDef, againMinStep; >>> utils::Duration lineDuration; >>> } agc; >>> struct { >>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >>> index e70439ee5..f0764ef46 100644 >>> --- a/src/ipa/simple/soft_simple.cpp >>> +++ b/src/ipa/simple/soft_simple.cpp >>> @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >>> >>> int32_t againMin = gainInfo.min().get<int32_t>(); >>> int32_t againMax = gainInfo.max().get<int32_t>(); >>> + int32_t againDef = gainInfo.def().get<int32_t>(); >>> >>> if (camHelper_) { >>> context_.configuration.agc.againMin = camHelper_->gain(againMin); >>> context_.configuration.agc.againMax = camHelper_->gain(againMax); >>> + context_.configuration.agc.againDef = camHelper_->gain(againDef); >>> context_.configuration.agc.againMinStep = >>> (context_.configuration.agc.againMax - >>> context_.configuration.agc.againMin) / >>> @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >>> * other) we limit the range of the gain values used. >>> */ >>> context_.configuration.agc.againMax = againMax; >>> + context_.configuration.agc.againDef = againDef; >>> if (againMin) { >>> context_.configuration.agc.againMin = againMin; >>> } else { >>
Quoting Hans de Goede (2025-09-25 14:37:42) > Hi, > > On 25-Sep-25 14:37, Hans de Goede wrote: > > Hi, > > > > On 25-Sep-25 14:10, Kieran Bingham wrote: > >> Quoting Isaac Scott (2025-09-25 11:03:48) > >>> Hi Hans, > >>> > >>> Thank you for the patch! > >>> > >>> Quoting Hans de Goede (2025-09-23 20:06:54) > >>>> At the moment when the overall image brightness is considered too high > >>>> the AGC code will lower the gain all the way down to againMin before > >>>> considering lowering the exposure. > >>>> > >>>> What should happen instead is lower the gain no lower then its default > >>>> ctrl value and after that lower the exposure instead of lowering > >>>> the gain. > >> > >> Have you encountered sensors where againMin != default ? > > > > Yes for example for the ov2680 the V4L2_CID_ANALOGUE_GAIN range is > > 0 - 1023 and the default gain is 250. > > > >> What does the sensor driver convey in these instances? (What would min > >> be and what would default be?) > > > > minimum gain is the minimum value the register accepts without behaving > > unexpectedly. Default gain should correspond to 1.0 . > > > >> > >>> I may be wrong, but what if the default gain is something like 2.0, > >>> wouldn't that mean the light levels received at maximum exposure would > >>> be multiplied by 2? > >> > >> Indeed, I don't think we should ever go below 1.0x gain. But I think the > >> issue here is that without a gain model - we don't know what a 1.0x gain > >> is - so I anticipate we're 'hoping' that the default is always a gain > >> code that corresponds to 1.0x. > > > > Right that is the idea, that default-gain == 1.0 Note many sensor drivers > > indeed use min-gain == def-gain in which case this patch is a no-op. > > > >> We probably don't have anything else to convey that here so I think this > >> is actually correct given the context here. > > > > Right. > > > >>> While I agree that using the absolute minimum is probably a bad idea (as > >>> even if it goes < 1, we're still artificially modifying the data), I > >>> think we maybe should be able to reduce the analogue gain to 1.0 at a > >>> minimum before touching exposure, to ensure we're making the most of the > >>> exposure we have. > >> > >> I think it's likely rare to have a sensor with an analogue gain less > >> than 1.0. I don't think I've seen one - has anyone else? > > > > I think (need to check) 0 on the ov2680 is actually 0. > > > > Another example is the ov2740 which according to the sensor-helper code in > > libcamera has a gain formula of: > > > > gain = gain-ctrl-reg / 128 > > > > have a gain-ctrl range exposed on V4L2_CID_ANALOGUE_GAIN of 128 - 1983, > > which gets written directly to the register. According to the formula above > > this means 1.0 - 15.5 . But I wonder if the register won't accept lower > > values and then do a gain below 1.0. The 1.0 / 128 min is currently enforced > > by the driver. But that does not mean the hw won't accept lower values. > > > > Let me give this a test-run on an ov08x40 and get back to you. > > So I modified the ov08x40 driver to allow values less then 128 and > gave those values a try. On the ov08x40 sensor gain values below 128 result > in the gain going up (so above 1.0) rather then down. Haha - Ok well at least it's the kernels job to protect us from that part! -- Kieran > > Regards, > > Hans > > > > > >>>> Signed-off-by: Hans de Goede <hansg@kernel.org> > >>>> --- > >>>> src/ipa/simple/algorithms/agc.cpp | 3 +-- > >>>> src/ipa/simple/ipa_context.h | 2 +- > >>>> src/ipa/simple/soft_simple.cpp | 3 +++ > >>>> 3 files changed, 5 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp > >>>> index c46bb0ebe..94961f9fe 100644 > >>>> --- a/src/ipa/simple/algorithms/agc.cpp > >>>> +++ b/src/ipa/simple/algorithms/agc.cpp > >>>> @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou > >>>> } > >>>> > >>>> if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { > >>>> - if (exposure == context.configuration.agc.exposureMax && > >>>> - again > context.configuration.agc.againMin) { > >>>> + if (again > context.configuration.agc.againDef) { > >>>> next = again * kExpNumeratorDown / kExpDenominator; > >>>> if (again - next < context.configuration.agc.againMinStep) > >>>> again -= context.configuration.agc.againMinStep; > >>>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > >>>> index a471b80ae..ba525a881 100644 > >>>> --- a/src/ipa/simple/ipa_context.h > >>>> +++ b/src/ipa/simple/ipa_context.h > >>>> @@ -28,7 +28,7 @@ struct IPASessionConfiguration { > >>>> float gamma; > >>>> struct { > >>>> int32_t exposureMin, exposureMax; > >>>> - double againMin, againMax, againMinStep; > >>>> + double againMin, againMax, againDef, againMinStep; > >>>> utils::Duration lineDuration; > >>>> } agc; > >>>> struct { > >>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > >>>> index e70439ee5..f0764ef46 100644 > >>>> --- a/src/ipa/simple/soft_simple.cpp > >>>> +++ b/src/ipa/simple/soft_simple.cpp > >>>> @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > >>>> > >>>> int32_t againMin = gainInfo.min().get<int32_t>(); > >>>> int32_t againMax = gainInfo.max().get<int32_t>(); > >>>> + int32_t againDef = gainInfo.def().get<int32_t>(); > >>>> > >>>> if (camHelper_) { > >>>> context_.configuration.agc.againMin = camHelper_->gain(againMin); > >>>> context_.configuration.agc.againMax = camHelper_->gain(againMax); > >>>> + context_.configuration.agc.againDef = camHelper_->gain(againDef); > >>>> context_.configuration.agc.againMinStep = > >>>> (context_.configuration.agc.againMax - > >>>> context_.configuration.agc.againMin) / > >>>> @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > >>>> * other) we limit the range of the gain values used. > >>>> */ > >>>> context_.configuration.agc.againMax = againMax; > >>>> + context_.configuration.agc.againDef = againDef; > >>>> if (againMin) { > >>>> context_.configuration.agc.againMin = againMin; > >>>> } else { > >>>> -- > >>>> 2.51.0 > >>>> > > >
Hi Milan, On 25-Sep-25 15:41, Milan Zamazal wrote: > Hans de Goede <hansg@kernel.org> writes: > >> Hi Milan, >> >> On 24-Sep-25 09:54, Milan Zamazal wrote: >>> Hi Hans, >>> >>> thank you for the patch. >>> >>> Hans de Goede <hansg@kernel.org> writes: >>> >>>> At the moment when the overall image brightness is considered too high >>>> the AGC code will lower the gain all the way down to againMin before >>>> considering lowering the exposure. >>>> >>>> What should happen instead is lower the gain no lower then its default >>> >>> s/then/than/ >> >> Ack. >> >>>> ctrl value and after that lower the exposure instead of lowering >>>> the gain. >>>> >>>> Otherwise there might be a heavily overexposed image (e.g. all white) >>>> which then is made less white by a very low gain which is no good. >>> >>> Do I understand it correctly that the gain should never be set below the >>> default value and bright images can then be fully handled by reducing exposure? >> >> Yes basically the pipeline for a single pixel looks like this: >> >> light-sensor -> gain -> adc >> >> Where the light-sensor gives of an analog signal between >> black-level and max. >> >> Now lets say initially there is a lens-cap covering the sensor, so AGC >> boosts exposure and gain to their max values and the room is brightly lit >> with daylight, so after removing the cap the image brightness is way too >> high. >> >> Now with the max exposure pretty much all pixels will be outputting >> their max analog signal. So changing the gain to < 1.0 (if supported) >> will result in the frames going from fully white to light gray >> to 50% gray at which point the overall brightness measures as ok, >> but we don't have a proper picture. > > I assume this applies also to situations where exposure is at the > minimum value but the image is so bright that a sensor with the default > gain is still saturated, right? That should never happen. For the lens shading correction I've pointed the camera directly at a bright LED flood light and then I can get the image to not be saturated with an exposure of ~250 / 4000, or in other words 1/16th of a frame time. So with an expected minimum exposure range of 0-511 where we can lower the exposure time to 1/512th of a frame which should be small enough for even an extremely bright image. Regards, Hans > >> Where as we lower the gain from max-gain to default gain which typically >> is ~1.0 at which point the resulting picture is still full white >> (heavily overexposed) and then start lowering the exposure will result >> in a proper picture. >> >> Actually thinking more about this, sensors where the min-gain advertised >> by the sensor-driver is less then 1.0 is probably why we have the weird >> code to come up with some alternative min-value when the v4l2-ctrl reports >> 0 as min value. E.g. many ov sensors have a linear gain where ctrl-value / 128 >> is the gain value. So setting the ctrl to 128 results in a gain of 1.0 . >> Most of these report a min-value of 128, as well as a default value of >> 128. But I'm pretty sure the hw will accept 0 just fine too and then >> set a gain to 0.0 (not really useful), or accept 64 for a gain of 0.5 . >> >> I think the magic min-gain == 0 handling is based on sensor drivers >> which do allow control of the full gain range (this was inherited from >> Andrey's work). So if we go with this change then we can also remove >> that weird magic handling of min-gain == 0. >> >>>> While at it also remove the weird limitation to only lower the gain >>>> when exposure is set to the maximum. As long as the gain is higher >>>> then the default gain, the gain should be lowered first. >>> >>> s/then/than/ >> >> Ack. >> >> Regards, >> >> Hans >> >> >> >>> >>>> Signed-off-by: Hans de Goede <hansg@kernel.org> >>>> --- >>>> src/ipa/simple/algorithms/agc.cpp | 3 +-- >>>> src/ipa/simple/ipa_context.h | 2 +- >>>> src/ipa/simple/soft_simple.cpp | 3 +++ >>>> 3 files changed, 5 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp >>>> index c46bb0ebe..94961f9fe 100644 >>>> --- a/src/ipa/simple/algorithms/agc.cpp >>>> +++ b/src/ipa/simple/algorithms/agc.cpp >>>> @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou >>>> } >>>> >>>> if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { >>>> - if (exposure == context.configuration.agc.exposureMax && >>>> - again > context.configuration.agc.againMin) { >>>> + if (again > context.configuration.agc.againDef) { >>>> next = again * kExpNumeratorDown / kExpDenominator; >>>> if (again - next < context.configuration.agc.againMinStep) >>>> again -= context.configuration.agc.againMinStep; >>>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >>>> index a471b80ae..ba525a881 100644 >>>> --- a/src/ipa/simple/ipa_context.h >>>> +++ b/src/ipa/simple/ipa_context.h >>>> @@ -28,7 +28,7 @@ struct IPASessionConfiguration { >>>> float gamma; >>>> struct { >>>> int32_t exposureMin, exposureMax; >>>> - double againMin, againMax, againMinStep; >>>> + double againMin, againMax, againDef, againMinStep; >>>> utils::Duration lineDuration; >>>> } agc; >>>> struct { >>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >>>> index e70439ee5..f0764ef46 100644 >>>> --- a/src/ipa/simple/soft_simple.cpp >>>> +++ b/src/ipa/simple/soft_simple.cpp >>>> @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >>>> >>>> int32_t againMin = gainInfo.min().get<int32_t>(); >>>> int32_t againMax = gainInfo.max().get<int32_t>(); >>>> + int32_t againDef = gainInfo.def().get<int32_t>(); >>>> >>>> if (camHelper_) { >>>> context_.configuration.agc.againMin = camHelper_->gain(againMin); >>>> context_.configuration.agc.againMax = camHelper_->gain(againMax); >>>> + context_.configuration.agc.againDef = camHelper_->gain(againDef); >>>> context_.configuration.agc.againMinStep = >>>> (context_.configuration.agc.againMax - >>>> context_.configuration.agc.againMin) / >>>> @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >>>> * other) we limit the range of the gain values used. >>>> */ >>>> context_.configuration.agc.againMax = againMax; >>>> + context_.configuration.agc.againDef = againDef; >>>> if (againMin) { >>>> context_.configuration.agc.againMin = againMin; >>>> } else { >>> >
diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index c46bb0ebe..94961f9fe 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou } if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { - if (exposure == context.configuration.agc.exposureMax && - again > context.configuration.agc.againMin) { + if (again > context.configuration.agc.againDef) { next = again * kExpNumeratorDown / kExpDenominator; if (again - next < context.configuration.agc.againMinStep) again -= context.configuration.agc.againMinStep; diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h index a471b80ae..ba525a881 100644 --- a/src/ipa/simple/ipa_context.h +++ b/src/ipa/simple/ipa_context.h @@ -28,7 +28,7 @@ struct IPASessionConfiguration { float gamma; struct { int32_t exposureMin, exposureMax; - double againMin, againMax, againMinStep; + double againMin, againMax, againDef, againMinStep; utils::Duration lineDuration; } agc; struct { diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index e70439ee5..f0764ef46 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) int32_t againMin = gainInfo.min().get<int32_t>(); int32_t againMax = gainInfo.max().get<int32_t>(); + int32_t againDef = gainInfo.def().get<int32_t>(); if (camHelper_) { context_.configuration.agc.againMin = camHelper_->gain(againMin); context_.configuration.agc.againMax = camHelper_->gain(againMax); + context_.configuration.agc.againDef = camHelper_->gain(againDef); context_.configuration.agc.againMinStep = (context_.configuration.agc.againMax - context_.configuration.agc.againMin) / @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) * other) we limit the range of the gain values used. */ context_.configuration.agc.againMax = againMax; + context_.configuration.agc.againDef = againDef; if (againMin) { context_.configuration.agc.againMin = againMin; } else {
At the moment when the overall image brightness is considered too high the AGC code will lower the gain all the way down to againMin before considering lowering the exposure. What should happen instead is lower the gain no lower then its default ctrl value and after that lower the exposure instead of lowering the gain. Otherwise there might be a heavily overexposed image (e.g. all white) which then is made less white by a very low gain which is no good. While at it also remove the weird limitation to only lower the gain when exposure is set to the maximum. As long as the gain is higher then the default gain, the gain should be lowered first. Signed-off-by: Hans de Goede <hansg@kernel.org> --- src/ipa/simple/algorithms/agc.cpp | 3 +-- src/ipa/simple/ipa_context.h | 2 +- src/ipa/simple/soft_simple.cpp | 3 +++ 3 files changed, 5 insertions(+), 3 deletions(-)