| Message ID | 20251114005428.90024-19-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch! Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com> Quoting Kieran Bingham (2025-11-14 00:54:22) > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8 > format, a range of 0.0 to (very nearly) 32.0. > > Convert usage to the new UQ5_8 FixedPoint Quantised type which will > support the conversion, clamping and quantisation so that the metadata > and debug prints can now report the effective gain applied instead of > the potentially inaccurate float. > > As the UQ5_8 type already clamps values, remove the explicit clamping. > This removes the clamping to a minimum of 1.0 gain, so we rely on > calculateNewEv to provide a valid gain. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/mali-c55/algorithms/agc.cpp | 20 ++++++++++---------- > src/ipa/mali-c55/ipa_context.h | 6 +++--- > 2 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp > index f60fddac3f04..3f10b237f581 100644 > --- a/src/ipa/mali-c55/algorithms/agc.cpp > +++ b/src/ipa/mali-c55/algorithms/agc.cpp > @@ -38,8 +38,8 @@ static constexpr unsigned int kNumHistogramBins = 256; > * format, a range of 0.0 to (very nearly) 32.0. We clamp from 1.0 to the actual > * max value which is 8191 * 2^-8. > */ > -static constexpr double kMinDigitalGain = 1.0; > -static constexpr double kMaxDigitalGain = 31.99609375; > +static constexpr float kMinDigitalGain = 1.0; > +static constexpr float kMaxDigitalGain = UQ5_8::TraitsType::max; > > uint32_t AgcStatistics::decodeBinValue(uint16_t binVal) > { > @@ -236,7 +236,7 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame, > agc.manual.ispGain = *digitalGain; > > LOG(MaliC55Agc, Debug) > - << "Digital gain set to " << agc.manual.ispGain > + << "Digital gain set to " << agc.manual.ispGain.value() > << " on request sequence " << frame; > } > } > @@ -245,7 +245,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex > mali_c55_params_block block) > { > IPAActiveState &activeState = context.activeState; > - double gain; > + UQ5_8 gain; > > if (activeState.agc.autoEnabled) > gain = activeState.agc.automatic.ispGain; > @@ -256,7 +256,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex > block.header->flags = MALI_C55_PARAM_BLOCK_FL_NONE; > block.header->size = sizeof(struct mali_c55_params_digital_gain); > > - block.digital_gain->gain = floatingToFixedPoint<5, 8, uint16_t, double>(gain); > + block.digital_gain->gain = gain.quantized(); > frameContext.agc.ispGain = gain; > > return block.header->size; > @@ -376,7 +376,7 @@ void Agc::process(IPAContext &context, > */ > uint32_t exposure = frameContext.agc.exposure; > double analogueGain = frameContext.agc.sensorGain; > - double digitalGain = frameContext.agc.ispGain; > + double digitalGain = frameContext.agc.ispGain.value(); > double totalGain = analogueGain * digitalGain; > utils::Duration currentShutter = exposure * configuration.sensor.lineDuration; > utils::Duration effectiveExposureValue = currentShutter * totalGain; > @@ -388,19 +388,19 @@ void Agc::process(IPAContext &context, > activeState.agc.exposureMode, statistics_.yHist, > effectiveExposureValue); > > - dGain = std::clamp(dGain, kMinDigitalGain, kMaxDigitalGain); > + UQ5_8 dGainQ = static_cast<float>(dGain); > > LOG(MaliC55Agc, Debug) > << "Divided up shutter, analogue gain and digital gain are " > - << shutterTime << ", " << aGain << " and " << dGain; > + << shutterTime << ", " << aGain << " and " << dGainQ.value(); > > activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration; > activeState.agc.automatic.sensorGain = aGain; > - activeState.agc.automatic.ispGain = dGain; > + activeState.agc.automatic.ispGain = dGainQ; > > metadata.set(controls::ExposureTime, currentShutter.get<std::micro>()); > metadata.set(controls::AnalogueGain, frameContext.agc.sensorGain); > - metadata.set(controls::DigitalGain, frameContext.agc.ispGain); > + metadata.set(controls::DigitalGain, frameContext.agc.ispGain.value()); > metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK); > } > > diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h > index 86d060a731cb..2db3fdf9e5cf 100644 > --- a/src/ipa/mali-c55/ipa_context.h > +++ b/src/ipa/mali-c55/ipa_context.h > @@ -41,12 +41,12 @@ struct IPAActiveState { > struct { > uint32_t exposure; > double sensorGain; > - double ispGain; > + UQ5_8 ispGain; > } automatic; > struct { > uint32_t exposure; > double sensorGain; > - double ispGain; > + UQ5_8 ispGain; > } manual; > bool autoEnabled; > uint32_t constraintMode; > @@ -64,7 +64,7 @@ struct IPAFrameContext : public FrameContext { > struct { > uint32_t exposure; > double sensorGain; > - double ispGain; > + UQ5_8 ispGain; > } agc; > > struct { > -- > 2.51.1 >
Hi Kieran, Thank you for the patch. On Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote: > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8 > format, a range of 0.0 to (very nearly) 32.0. > > Convert usage to the new UQ5_8 FixedPoint Quantised type which will > support the conversion, clamping and quantisation so that the metadata > and debug prints can now report the effective gain applied instead of > the potentially inaccurate float. > > As the UQ5_8 type already clamps values, remove the explicit clamping. > This removes the clamping to a minimum of 1.0 gain, so we rely on > calculateNewEv to provide a valid gain. Is that guaranteed ? The function documentation doesn't indicate it, and it seems that the constraint is not enforced at least in the fixed exposure time and gain code path. > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/mali-c55/algorithms/agc.cpp | 20 ++++++++++---------- > src/ipa/mali-c55/ipa_context.h | 6 +++--- > 2 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp > index f60fddac3f04..3f10b237f581 100644 > --- a/src/ipa/mali-c55/algorithms/agc.cpp > +++ b/src/ipa/mali-c55/algorithms/agc.cpp > @@ -38,8 +38,8 @@ static constexpr unsigned int kNumHistogramBins = 256; > * format, a range of 0.0 to (very nearly) 32.0. We clamp from 1.0 to the actual > * max value which is 8191 * 2^-8. > */ > -static constexpr double kMinDigitalGain = 1.0; > -static constexpr double kMaxDigitalGain = 31.99609375; > +static constexpr float kMinDigitalGain = 1.0; > +static constexpr float kMaxDigitalGain = UQ5_8::TraitsType::max; > > uint32_t AgcStatistics::decodeBinValue(uint16_t binVal) > { > @@ -236,7 +236,7 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame, > agc.manual.ispGain = *digitalGain; > > LOG(MaliC55Agc, Debug) > - << "Digital gain set to " << agc.manual.ispGain > + << "Digital gain set to " << agc.manual.ispGain.value() > << " on request sequence " << frame; > } > } > @@ -245,7 +245,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex > mali_c55_params_block block) > { > IPAActiveState &activeState = context.activeState; > - double gain; > + UQ5_8 gain; > > if (activeState.agc.autoEnabled) > gain = activeState.agc.automatic.ispGain; > @@ -256,7 +256,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex > block.header->flags = MALI_C55_PARAM_BLOCK_FL_NONE; > block.header->size = sizeof(struct mali_c55_params_digital_gain); > > - block.digital_gain->gain = floatingToFixedPoint<5, 8, uint16_t, double>(gain); > + block.digital_gain->gain = gain.quantized(); > frameContext.agc.ispGain = gain; > > return block.header->size; > @@ -376,7 +376,7 @@ void Agc::process(IPAContext &context, > */ > uint32_t exposure = frameContext.agc.exposure; > double analogueGain = frameContext.agc.sensorGain; > - double digitalGain = frameContext.agc.ispGain; > + double digitalGain = frameContext.agc.ispGain.value(); > double totalGain = analogueGain * digitalGain; > utils::Duration currentShutter = exposure * configuration.sensor.lineDuration; > utils::Duration effectiveExposureValue = currentShutter * totalGain; > @@ -388,19 +388,19 @@ void Agc::process(IPAContext &context, > activeState.agc.exposureMode, statistics_.yHist, > effectiveExposureValue); > > - dGain = std::clamp(dGain, kMinDigitalGain, kMaxDigitalGain); > + UQ5_8 dGainQ = static_cast<float>(dGain); You don't name quantizzed variables with a 'Q' suffix elsewhere, I wouldn't do it here either. > > LOG(MaliC55Agc, Debug) > << "Divided up shutter, analogue gain and digital gain are " > - << shutterTime << ", " << aGain << " and " << dGain; > + << shutterTime << ", " << aGain << " and " << dGainQ.value(); Looking forward to operator<<() :-) > > activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration; > activeState.agc.automatic.sensorGain = aGain; > - activeState.agc.automatic.ispGain = dGain; > + activeState.agc.automatic.ispGain = dGainQ; > > metadata.set(controls::ExposureTime, currentShutter.get<std::micro>()); > metadata.set(controls::AnalogueGain, frameContext.agc.sensorGain); > - metadata.set(controls::DigitalGain, frameContext.agc.ispGain); > + metadata.set(controls::DigitalGain, frameContext.agc.ispGain.value()); > metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK); > } > > diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h > index 86d060a731cb..2db3fdf9e5cf 100644 > --- a/src/ipa/mali-c55/ipa_context.h > +++ b/src/ipa/mali-c55/ipa_context.h > @@ -41,12 +41,12 @@ struct IPAActiveState { > struct { > uint32_t exposure; > double sensorGain; > - double ispGain; > + UQ5_8 ispGain; > } automatic; > struct { > uint32_t exposure; > double sensorGain; > - double ispGain; > + UQ5_8 ispGain; > } manual; > bool autoEnabled; > uint32_t constraintMode; > @@ -64,7 +64,7 @@ struct IPAFrameContext : public FrameContext { > struct { > uint32_t exposure; > double sensorGain; > - double ispGain; > + UQ5_8 ispGain; > } agc; > > struct { >
Quoting Laurent Pinchart (2025-11-19 04:30:07) > Hi Kieran, > > Thank you for the patch. > > On Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote: > > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8 > > format, a range of 0.0 to (very nearly) 32.0. > > > > Convert usage to the new UQ5_8 FixedPoint Quantised type which will > > support the conversion, clamping and quantisation so that the metadata > > and debug prints can now report the effective gain applied instead of > > the potentially inaccurate float. > > > > As the UQ5_8 type already clamps values, remove the explicit clamping. > > This removes the clamping to a minimum of 1.0 gain, so we rely on > > calculateNewEv to provide a valid gain. > > Is that guaranteed ? The function documentation doesn't indicate it, and > it seems that the constraint is not enforced at least in the fixed > exposure time and gain code path. Guaranteed, no Expected, yes. That's why I've stated it here. I haven't dwelled on this because if calculateNewEv() asks for a digital gain less than 1 ... I could imagine that it might really want that. Or if it asks for it - it could/should anticipate that it might get applied. Stefan, would you expect/desire/anticipate digital gains less than 1.0 ? or should we restrict them either here or in the calculateNewEv() call? > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/ipa/mali-c55/algorithms/agc.cpp | 20 ++++++++++---------- > > src/ipa/mali-c55/ipa_context.h | 6 +++--- > > 2 files changed, 13 insertions(+), 13 deletions(-) > > > > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp > > index f60fddac3f04..3f10b237f581 100644 > > --- a/src/ipa/mali-c55/algorithms/agc.cpp > > +++ b/src/ipa/mali-c55/algorithms/agc.cpp > > @@ -38,8 +38,8 @@ static constexpr unsigned int kNumHistogramBins = 256; > > * format, a range of 0.0 to (very nearly) 32.0. We clamp from 1.0 to the actual > > * max value which is 8191 * 2^-8. > > */ > > -static constexpr double kMinDigitalGain = 1.0; > > -static constexpr double kMaxDigitalGain = 31.99609375; > > +static constexpr float kMinDigitalGain = 1.0; > > +static constexpr float kMaxDigitalGain = UQ5_8::TraitsType::max; > > > > uint32_t AgcStatistics::decodeBinValue(uint16_t binVal) > > { > > @@ -236,7 +236,7 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame, > > agc.manual.ispGain = *digitalGain; > > > > LOG(MaliC55Agc, Debug) > > - << "Digital gain set to " << agc.manual.ispGain > > + << "Digital gain set to " << agc.manual.ispGain.value() > > << " on request sequence " << frame; > > } > > } > > @@ -245,7 +245,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex > > mali_c55_params_block block) > > { > > IPAActiveState &activeState = context.activeState; > > - double gain; > > + UQ5_8 gain; > > > > if (activeState.agc.autoEnabled) > > gain = activeState.agc.automatic.ispGain; > > @@ -256,7 +256,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex > > block.header->flags = MALI_C55_PARAM_BLOCK_FL_NONE; > > block.header->size = sizeof(struct mali_c55_params_digital_gain); > > > > - block.digital_gain->gain = floatingToFixedPoint<5, 8, uint16_t, double>(gain); > > + block.digital_gain->gain = gain.quantized(); > > frameContext.agc.ispGain = gain; > > > > return block.header->size; > > @@ -376,7 +376,7 @@ void Agc::process(IPAContext &context, > > */ > > uint32_t exposure = frameContext.agc.exposure; > > double analogueGain = frameContext.agc.sensorGain; > > - double digitalGain = frameContext.agc.ispGain; > > + double digitalGain = frameContext.agc.ispGain.value(); > > double totalGain = analogueGain * digitalGain; > > utils::Duration currentShutter = exposure * configuration.sensor.lineDuration; > > utils::Duration effectiveExposureValue = currentShutter * totalGain; > > @@ -388,19 +388,19 @@ void Agc::process(IPAContext &context, > > activeState.agc.exposureMode, statistics_.yHist, > > effectiveExposureValue); > > > > - dGain = std::clamp(dGain, kMinDigitalGain, kMaxDigitalGain); > > + UQ5_8 dGainQ = static_cast<float>(dGain); > > You don't name quantizzed variables with a 'Q' suffix elsewhere, I > wouldn't do it here either. What do you recommend? I can't call it dGain - that's already the floating point digital gain from calculateNewEv(). quantizedDigitalGain gets a bit long ;-) qDGain? or digitalGain along side dGain ? I think we need to convey that /this/ variable is the quantized one. > > > > > LOG(MaliC55Agc, Debug) > > << "Divided up shutter, analogue gain and digital gain are " > > - << shutterTime << ", " << aGain << " and " << dGain; > > + << shutterTime << ", " << aGain << " and " << dGainQ.value(); > > Looking forward to operator<<() :-) > > > > > activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration; > > activeState.agc.automatic.sensorGain = aGain; > > - activeState.agc.automatic.ispGain = dGain; > > + activeState.agc.automatic.ispGain = dGainQ; > > > > metadata.set(controls::ExposureTime, currentShutter.get<std::micro>()); > > metadata.set(controls::AnalogueGain, frameContext.agc.sensorGain); > > - metadata.set(controls::DigitalGain, frameContext.agc.ispGain); > > + metadata.set(controls::DigitalGain, frameContext.agc.ispGain.value()); > > metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK); > > } > > > > diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h > > index 86d060a731cb..2db3fdf9e5cf 100644 > > --- a/src/ipa/mali-c55/ipa_context.h > > +++ b/src/ipa/mali-c55/ipa_context.h > > @@ -41,12 +41,12 @@ struct IPAActiveState { > > struct { > > uint32_t exposure; > > double sensorGain; > > - double ispGain; > > + UQ5_8 ispGain; > > } automatic; > > struct { > > uint32_t exposure; > > double sensorGain; > > - double ispGain; > > + UQ5_8 ispGain; > > } manual; > > bool autoEnabled; > > uint32_t constraintMode; > > @@ -64,7 +64,7 @@ struct IPAFrameContext : public FrameContext { > > struct { > > uint32_t exposure; > > double sensorGain; > > - double ispGain; > > + UQ5_8 ispGain; > > } agc; > > > > struct { > > > > -- > Regards, > > Laurent Pinchart
Hi Kieran, Quoting Kieran Bingham (2025-11-19 16:51:02) > Quoting Laurent Pinchart (2025-11-19 04:30:07) > > Hi Kieran, > > > > Thank you for the patch. > > > > On Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote: > > > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8 > > > format, a range of 0.0 to (very nearly) 32.0. > > > > > > Convert usage to the new UQ5_8 FixedPoint Quantised type which will > > > support the conversion, clamping and quantisation so that the metadata > > > and debug prints can now report the effective gain applied instead of > > > the potentially inaccurate float. > > > > > > As the UQ5_8 type already clamps values, remove the explicit clamping. > > > This removes the clamping to a minimum of 1.0 gain, so we rely on > > > calculateNewEv to provide a valid gain. > > > > Is that guaranteed ? The function documentation doesn't indicate it, and > > it seems that the constraint is not enforced at least in the fixed > > exposure time and gain code path. > > Guaranteed, no > Expected, yes. > > That's why I've stated it here. > > I haven't dwelled on this because if calculateNewEv() asks for a digital > gain less than 1 ... I could imagine that it might really want that. > > Or if it asks for it - it could/should anticipate that it might get > applied. > > > Stefan, would you expect/desire/anticipate digital gains less than 1.0 ? > or should we restrict them either here or in the calculateNewEv() call? That is hard to tell. I can't really come up with a reasonable use case. The issue would arise if the scene is extremely bright and the minimum exposure time is too high to create a properly exposed image. I've never seen this in practice. If we then apply a digital gain < 1 all the saturated pixels will end up grey, which is also most likely not what we want. So my tendency would be to clamp at 1.0. Now regarding calculateNewEv()/splitExposure(): If you see that as a building block to build all kinds of IPAs, I would expect it to split the exposure within the available constraints and potentially return a digital gain < 1. The IPA would then be responsible to clamp to 1. If we treat it as the "libcamera IPA base class" we can move the clamping in there as we currently don't see a reasonable use case for a digital gain < 1.0. Does that make sense? Best regards, Stefan > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > src/ipa/mali-c55/algorithms/agc.cpp | 20 ++++++++++---------- > > > src/ipa/mali-c55/ipa_context.h | 6 +++--- > > > 2 files changed, 13 insertions(+), 13 deletions(-) > > > > > > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp > > > index f60fddac3f04..3f10b237f581 100644 > > > --- a/src/ipa/mali-c55/algorithms/agc.cpp > > > +++ b/src/ipa/mali-c55/algorithms/agc.cpp > > > @@ -38,8 +38,8 @@ static constexpr unsigned int kNumHistogramBins = 256; > > > * format, a range of 0.0 to (very nearly) 32.0. We clamp from 1.0 to the actual > > > * max value which is 8191 * 2^-8. > > > */ > > > -static constexpr double kMinDigitalGain = 1.0; > > > -static constexpr double kMaxDigitalGain = 31.99609375; > > > +static constexpr float kMinDigitalGain = 1.0; > > > +static constexpr float kMaxDigitalGain = UQ5_8::TraitsType::max; > > > > > > uint32_t AgcStatistics::decodeBinValue(uint16_t binVal) > > > { > > > @@ -236,7 +236,7 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame, > > > agc.manual.ispGain = *digitalGain; > > > > > > LOG(MaliC55Agc, Debug) > > > - << "Digital gain set to " << agc.manual.ispGain > > > + << "Digital gain set to " << agc.manual.ispGain.value() > > > << " on request sequence " << frame; > > > } > > > } > > > @@ -245,7 +245,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex > > > mali_c55_params_block block) > > > { > > > IPAActiveState &activeState = context.activeState; > > > - double gain; > > > + UQ5_8 gain; > > > > > > if (activeState.agc.autoEnabled) > > > gain = activeState.agc.automatic.ispGain; > > > @@ -256,7 +256,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex > > > block.header->flags = MALI_C55_PARAM_BLOCK_FL_NONE; > > > block.header->size = sizeof(struct mali_c55_params_digital_gain); > > > > > > - block.digital_gain->gain = floatingToFixedPoint<5, 8, uint16_t, double>(gain); > > > + block.digital_gain->gain = gain.quantized(); > > > frameContext.agc.ispGain = gain; > > > > > > return block.header->size; > > > @@ -376,7 +376,7 @@ void Agc::process(IPAContext &context, > > > */ > > > uint32_t exposure = frameContext.agc.exposure; > > > double analogueGain = frameContext.agc.sensorGain; > > > - double digitalGain = frameContext.agc.ispGain; > > > + double digitalGain = frameContext.agc.ispGain.value(); > > > double totalGain = analogueGain * digitalGain; > > > utils::Duration currentShutter = exposure * configuration.sensor.lineDuration; > > > utils::Duration effectiveExposureValue = currentShutter * totalGain; > > > @@ -388,19 +388,19 @@ void Agc::process(IPAContext &context, > > > activeState.agc.exposureMode, statistics_.yHist, > > > effectiveExposureValue); > > > > > > - dGain = std::clamp(dGain, kMinDigitalGain, kMaxDigitalGain); > > > + UQ5_8 dGainQ = static_cast<float>(dGain); > > > > You don't name quantizzed variables with a 'Q' suffix elsewhere, I > > wouldn't do it here either. > > What do you recommend? > > I can't call it dGain - that's already the floating point digital gain > from calculateNewEv(). > > quantizedDigitalGain gets a bit long ;-) > qDGain? > > or digitalGain along side dGain ? I think we need to convey that /this/ > variable is the quantized one. > > > > > > > > > > LOG(MaliC55Agc, Debug) > > > << "Divided up shutter, analogue gain and digital gain are " > > > - << shutterTime << ", " << aGain << " and " << dGain; > > > + << shutterTime << ", " << aGain << " and " << dGainQ.value(); > > > > Looking forward to operator<<() :-) > > > > > > > > activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration; > > > activeState.agc.automatic.sensorGain = aGain; > > > - activeState.agc.automatic.ispGain = dGain; > > > + activeState.agc.automatic.ispGain = dGainQ; > > > > > > metadata.set(controls::ExposureTime, currentShutter.get<std::micro>()); > > > metadata.set(controls::AnalogueGain, frameContext.agc.sensorGain); > > > - metadata.set(controls::DigitalGain, frameContext.agc.ispGain); > > > + metadata.set(controls::DigitalGain, frameContext.agc.ispGain.value()); > > > metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK); > > > } > > > > > > diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h > > > index 86d060a731cb..2db3fdf9e5cf 100644 > > > --- a/src/ipa/mali-c55/ipa_context.h > > > +++ b/src/ipa/mali-c55/ipa_context.h > > > @@ -41,12 +41,12 @@ struct IPAActiveState { > > > struct { > > > uint32_t exposure; > > > double sensorGain; > > > - double ispGain; > > > + UQ5_8 ispGain; > > > } automatic; > > > struct { > > > uint32_t exposure; > > > double sensorGain; > > > - double ispGain; > > > + UQ5_8 ispGain; > > > } manual; > > > bool autoEnabled; > > > uint32_t constraintMode; > > > @@ -64,7 +64,7 @@ struct IPAFrameContext : public FrameContext { > > > struct { > > > uint32_t exposure; > > > double sensorGain; > > > - double ispGain; > > > + UQ5_8 ispGain; > > > } agc; > > > > > > struct { > > > > > > > -- > > Regards, > > > > Laurent Pinchart
Quoting Stefan Klug (2025-11-19 16:09:35) > Hi Kieran, > > Quoting Kieran Bingham (2025-11-19 16:51:02) > > Quoting Laurent Pinchart (2025-11-19 04:30:07) > > > Hi Kieran, > > > > > > Thank you for the patch. > > > > > > On Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote: > > > > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8 > > > > format, a range of 0.0 to (very nearly) 32.0. > > > > > > > > Convert usage to the new UQ5_8 FixedPoint Quantised type which will > > > > support the conversion, clamping and quantisation so that the metadata > > > > and debug prints can now report the effective gain applied instead of > > > > the potentially inaccurate float. > > > > > > > > As the UQ5_8 type already clamps values, remove the explicit clamping. > > > > This removes the clamping to a minimum of 1.0 gain, so we rely on > > > > calculateNewEv to provide a valid gain. > > > > > > Is that guaranteed ? The function documentation doesn't indicate it, and > > > it seems that the constraint is not enforced at least in the fixed > > > exposure time and gain code path. > > > > Guaranteed, no > > Expected, yes. > > > > That's why I've stated it here. > > > > I haven't dwelled on this because if calculateNewEv() asks for a digital > > gain less than 1 ... I could imagine that it might really want that. > > > > Or if it asks for it - it could/should anticipate that it might get > > applied. > > > > > > Stefan, would you expect/desire/anticipate digital gains less than 1.0 ? > > or should we restrict them either here or in the calculateNewEv() call? > > That is hard to tell. I can't really come up with a reasonable use case. > The issue would arise if the scene is extremely bright and the minimum > exposure time is too high to create a properly exposed image. I've never > seen this in practice. If we then apply a digital gain < 1 all the > saturated pixels will end up grey, which is also most likely not what we > want. So my tendency would be to clamp at 1.0. > > Now regarding calculateNewEv()/splitExposure(): If you see that as a > building block to build all kinds of IPAs, I would expect it to split > the exposure within the available constraints and potentially return a > digital gain < 1. The IPA would then be responsible to clamp to 1. If we > treat it as the "libcamera IPA base class" we can move the clamping in > there as we currently don't see a reasonable use case for a digital gain > < 1.0. Does that make sense? If a digital gain of < 1.0 doesn't make sense, then I don't think every IPA who uses calculateNewEv//splitExposure should do the same clamping, and we should make that helper guarantee it will always return gains > 1.0. -- Kieran > Best regards, > Stefan
Quoting Kieran Bingham (2025-11-19 17:32:26) > Quoting Stefan Klug (2025-11-19 16:09:35) > > Hi Kieran, > > > > Quoting Kieran Bingham (2025-11-19 16:51:02) > > > Quoting Laurent Pinchart (2025-11-19 04:30:07) > > > > Hi Kieran, > > > > > > > > Thank you for the patch. > > > > > > > > On Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote: > > > > > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8 > > > > > format, a range of 0.0 to (very nearly) 32.0. > > > > > > > > > > Convert usage to the new UQ5_8 FixedPoint Quantised type which will > > > > > support the conversion, clamping and quantisation so that the metadata > > > > > and debug prints can now report the effective gain applied instead of > > > > > the potentially inaccurate float. > > > > > > > > > > As the UQ5_8 type already clamps values, remove the explicit clamping. > > > > > This removes the clamping to a minimum of 1.0 gain, so we rely on > > > > > calculateNewEv to provide a valid gain. > > > > > > > > Is that guaranteed ? The function documentation doesn't indicate it, and > > > > it seems that the constraint is not enforced at least in the fixed > > > > exposure time and gain code path. > > > > > > Guaranteed, no > > > Expected, yes. > > > > > > That's why I've stated it here. > > > > > > I haven't dwelled on this because if calculateNewEv() asks for a digital > > > gain less than 1 ... I could imagine that it might really want that. > > > > > > Or if it asks for it - it could/should anticipate that it might get > > > applied. > > > > > > > > > Stefan, would you expect/desire/anticipate digital gains less than 1.0 ? > > > or should we restrict them either here or in the calculateNewEv() call? > > > > That is hard to tell. I can't really come up with a reasonable use case. > > The issue would arise if the scene is extremely bright and the minimum > > exposure time is too high to create a properly exposed image. I've never > > seen this in practice. If we then apply a digital gain < 1 all the > > saturated pixels will end up grey, which is also most likely not what we > > want. So my tendency would be to clamp at 1.0. > > > > Now regarding calculateNewEv()/splitExposure(): If you see that as a > > building block to build all kinds of IPAs, I would expect it to split > > the exposure within the available constraints and potentially return a > > digital gain < 1. The IPA would then be responsible to clamp to 1. If we > > treat it as the "libcamera IPA base class" we can move the clamping in > > there as we currently don't see a reasonable use case for a digital gain > > < 1.0. Does that make sense? > > If a digital gain of < 1.0 doesn't make sense, then I don't think every > IPA who uses calculateNewEv//splitExposure should do the same clamping, > and we should make that helper guarantee it will always return gains > > 1.0. Well, just because I can't come up with a use case right now doesn't mean it doesn't make sense :-). But I'm fine with clamping in there... Cheers, Stefan > > -- > Kieran > > > Best regards, > > Stefan
On Wed, Nov 19, 2025 at 05:35:26PM +0100, Stefan Klug wrote: > Quoting Kieran Bingham (2025-11-19 17:32:26) > > Quoting Stefan Klug (2025-11-19 16:09:35) > > > Quoting Kieran Bingham (2025-11-19 16:51:02) > > > > Quoting Laurent Pinchart (2025-11-19 04:30:07) > > > > > On Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote: > > > > > > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8 > > > > > > format, a range of 0.0 to (very nearly) 32.0. > > > > > > > > > > > > Convert usage to the new UQ5_8 FixedPoint Quantised type which will > > > > > > support the conversion, clamping and quantisation so that the metadata > > > > > > and debug prints can now report the effective gain applied instead of > > > > > > the potentially inaccurate float. > > > > > > > > > > > > As the UQ5_8 type already clamps values, remove the explicit clamping. > > > > > > This removes the clamping to a minimum of 1.0 gain, so we rely on > > > > > > calculateNewEv to provide a valid gain. > > > > > > > > > > Is that guaranteed ? The function documentation doesn't indicate it, and > > > > > it seems that the constraint is not enforced at least in the fixed > > > > > exposure time and gain code path. > > > > > > > > Guaranteed, no > > > > Expected, yes. If it's expected but not guaranteed then this patch may introduce a regression. > > > > That's why I've stated it here. > > > > > > > > I haven't dwelled on this because if calculateNewEv() asks for a digital > > > > gain less than 1 ... I could imagine that it might really want that. > > > > > > > > Or if it asks for it - it could/should anticipate that it might get > > > > applied. > > > > > > > > > > > > Stefan, would you expect/desire/anticipate digital gains less than 1.0 ? > > > > or should we restrict them either here or in the calculateNewEv() call? > > > > > > That is hard to tell. I can't really come up with a reasonable use case. > > > The issue would arise if the scene is extremely bright and the minimum > > > exposure time is too high to create a properly exposed image. I've never > > > seen this in practice. If we then apply a digital gain < 1 all the > > > saturated pixels will end up grey, which is also most likely not what we > > > want. So my tendency would be to clamp at 1.0. Most hardware won't allow applying a digital gain smaller than 1, and even if it was possible, it would be quite useless due to saturation. > > > Now regarding calculateNewEv()/splitExposure(): If you see that as a > > > building block to build all kinds of IPAs, I would expect it to split > > > the exposure within the available constraints and potentially return a > > > digital gain < 1. The IPA would then be responsible to clamp to 1. If we > > > treat it as the "libcamera IPA base class" we can move the clamping in > > > there as we currently don't see a reasonable use case for a digital gain > > > < 1.0. Does that make sense? > > > > If a digital gain of < 1.0 doesn't make sense, then I don't think every > > IPA who uses calculateNewEv//splitExposure should do the same clamping, > > and we should make that helper guarantee it will always return gains > > > 1.0. > > Well, just because I can't come up with a use case right now doesn't > mean it doesn't make sense :-). But I'm fine with clamping in there... I agree with Kieran that duplicating clamping is not ideal. This being said, we do duplicate code between AGC implementations already, and I think Jacopo is working on fixing that by factoring out code to a common helper in libipa. We can clamp in calculateNewEv(), or somewhere else, depending on how we define calculateNewEv(). As long as things are well designed (as in giving calculateNewEv() a clear and meaningful purpose) and clearly documented, I don't mind much.
diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp index f60fddac3f04..3f10b237f581 100644 --- a/src/ipa/mali-c55/algorithms/agc.cpp +++ b/src/ipa/mali-c55/algorithms/agc.cpp @@ -38,8 +38,8 @@ static constexpr unsigned int kNumHistogramBins = 256; * format, a range of 0.0 to (very nearly) 32.0. We clamp from 1.0 to the actual * max value which is 8191 * 2^-8. */ -static constexpr double kMinDigitalGain = 1.0; -static constexpr double kMaxDigitalGain = 31.99609375; +static constexpr float kMinDigitalGain = 1.0; +static constexpr float kMaxDigitalGain = UQ5_8::TraitsType::max; uint32_t AgcStatistics::decodeBinValue(uint16_t binVal) { @@ -236,7 +236,7 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame, agc.manual.ispGain = *digitalGain; LOG(MaliC55Agc, Debug) - << "Digital gain set to " << agc.manual.ispGain + << "Digital gain set to " << agc.manual.ispGain.value() << " on request sequence " << frame; } } @@ -245,7 +245,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex mali_c55_params_block block) { IPAActiveState &activeState = context.activeState; - double gain; + UQ5_8 gain; if (activeState.agc.autoEnabled) gain = activeState.agc.automatic.ispGain; @@ -256,7 +256,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex block.header->flags = MALI_C55_PARAM_BLOCK_FL_NONE; block.header->size = sizeof(struct mali_c55_params_digital_gain); - block.digital_gain->gain = floatingToFixedPoint<5, 8, uint16_t, double>(gain); + block.digital_gain->gain = gain.quantized(); frameContext.agc.ispGain = gain; return block.header->size; @@ -376,7 +376,7 @@ void Agc::process(IPAContext &context, */ uint32_t exposure = frameContext.agc.exposure; double analogueGain = frameContext.agc.sensorGain; - double digitalGain = frameContext.agc.ispGain; + double digitalGain = frameContext.agc.ispGain.value(); double totalGain = analogueGain * digitalGain; utils::Duration currentShutter = exposure * configuration.sensor.lineDuration; utils::Duration effectiveExposureValue = currentShutter * totalGain; @@ -388,19 +388,19 @@ void Agc::process(IPAContext &context, activeState.agc.exposureMode, statistics_.yHist, effectiveExposureValue); - dGain = std::clamp(dGain, kMinDigitalGain, kMaxDigitalGain); + UQ5_8 dGainQ = static_cast<float>(dGain); LOG(MaliC55Agc, Debug) << "Divided up shutter, analogue gain and digital gain are " - << shutterTime << ", " << aGain << " and " << dGain; + << shutterTime << ", " << aGain << " and " << dGainQ.value(); activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration; activeState.agc.automatic.sensorGain = aGain; - activeState.agc.automatic.ispGain = dGain; + activeState.agc.automatic.ispGain = dGainQ; metadata.set(controls::ExposureTime, currentShutter.get<std::micro>()); metadata.set(controls::AnalogueGain, frameContext.agc.sensorGain); - metadata.set(controls::DigitalGain, frameContext.agc.ispGain); + metadata.set(controls::DigitalGain, frameContext.agc.ispGain.value()); metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK); } diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h index 86d060a731cb..2db3fdf9e5cf 100644 --- a/src/ipa/mali-c55/ipa_context.h +++ b/src/ipa/mali-c55/ipa_context.h @@ -41,12 +41,12 @@ struct IPAActiveState { struct { uint32_t exposure; double sensorGain; - double ispGain; + UQ5_8 ispGain; } automatic; struct { uint32_t exposure; double sensorGain; - double ispGain; + UQ5_8 ispGain; } manual; bool autoEnabled; uint32_t constraintMode; @@ -64,7 +64,7 @@ struct IPAFrameContext : public FrameContext { struct { uint32_t exposure; double sensorGain; - double ispGain; + UQ5_8 ispGain; } agc; struct {
The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8 format, a range of 0.0 to (very nearly) 32.0. Convert usage to the new UQ5_8 FixedPoint Quantised type which will support the conversion, clamping and quantisation so that the metadata and debug prints can now report the effective gain applied instead of the potentially inaccurate float. As the UQ5_8 type already clamps values, remove the explicit clamping. This removes the clamping to a minimum of 1.0 gain, so we rely on calculateNewEv to provide a valid gain. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/mali-c55/algorithms/agc.cpp | 20 ++++++++++---------- src/ipa/mali-c55/ipa_context.h | 6 +++--- 2 files changed, 13 insertions(+), 13 deletions(-)