[RFC,v1,09/17] ipa: mali-c55: Remove `DigitalGain` support
diff mbox series

Message ID 20260703153819.1088752-10-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: libipa: agc rework
Related show

Commit Message

Barnabás Pőcze July 3, 2026, 3:38 p.m. UTC 
The mali-c55 agc algorithm still mainly uses the `AeEnable` control,
but the interaction of that control and `AeEnable` is not well defined,
and not other platform supports setting it. So remove it for now until
things are properly defined.

Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/262
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/mali-c55/algorithms/agc.cpp | 57 ++---------------------------
 src/ipa/mali-c55/algorithms/agc.h   |  3 --
 src/ipa/mali-c55/ipa_context.h      |  3 --
 3 files changed, 4 insertions(+), 59 deletions(-)

Comments

Kieran Bingham July 4, 2026, 5:27 a.m. UTC | #1 
Quoting Barnabás Pőcze (2026-07-03 16:38:11)
> The mali-c55 agc algorithm still mainly uses the `AeEnable` control,
> but the interaction of that control and `AeEnable` is not well defined,
> and not other platform supports setting it. So remove it for now until
> things are properly defined.

Is this message accurate with the $SUBJECT? On first read these sound
like different things. Lets see what the patch says...


Aha, ok so it was 'that control' == DigitalGain.

> 
> Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/262
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/mali-c55/algorithms/agc.cpp | 57 ++---------------------------
>  src/ipa/mali-c55/algorithms/agc.h   |  3 --
>  src/ipa/mali-c55/ipa_context.h      |  3 --
>  3 files changed, 4 insertions(+), 59 deletions(-)
> 
> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
> index 6e091603f9..8d104e01f2 100644
> --- a/src/ipa/mali-c55/algorithms/agc.cpp
> +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> @@ -16,7 +16,6 @@
>  #include <libcamera/property_ids.h>
>  
>  #include "libipa/colours.h"
> -#include "libipa/fixedpoint.h"
>  
>  namespace libcamera {
>  
> @@ -33,14 +32,6 @@ LOG_DEFINE_CATEGORY(MaliC55Agc)
>   */
>  static constexpr unsigned int kNumHistogramBins = 256;
>  
> -/*
> - * 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. We clamp from 1.0 to the actual
> - * max value which is 8191 * 2^-8.
> - */
> -static constexpr float kMinDigitalGain = 1.0;
> -static constexpr float kMaxDigitalGain = UQ<5, 8>::TraitsType::max;
> -

I'm a bit wary, as we'll want DigitalGain when Stefan's AGC work ends up
in here as we use DG to give smoother AGC... but I'd be ok removing it
at the moment if it helps with the cleanup to get there.

So a tentative. Dan/Stefan, I assume there's no objections here?

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


>  uint32_t AgcStatistics::decodeBinValue(uint16_t binVal)
>  {
>         int exponent = (binVal & 0xf000) >> 12;
> @@ -137,8 +128,6 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
>                 return ret;
>  
>         context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true);
> -       context.ctrlMap[&controls::DigitalGain] = ControlInfo(
> -               kMinDigitalGain, kMaxDigitalGain, kMinDigitalGain);
>         context.ctrlMap.merge(agc_.controls());
>  
>         return 0;
> @@ -158,10 +147,8 @@ int Agc::configure(IPAContext &context,
>         context.activeState.agc.autoEnabled = true;
>         context.activeState.agc.automatic.sensorGain = context.configuration.agc.minAnalogueGain;
>         context.activeState.agc.automatic.exposure = context.configuration.agc.defaultExposure;
> -       context.activeState.agc.automatic.ispGain = kMinDigitalGain;
>         context.activeState.agc.manual.sensorGain = context.configuration.agc.minAnalogueGain;
>         context.activeState.agc.manual.exposure = context.configuration.agc.defaultExposure;
> -       context.activeState.agc.manual.ispGain = kMinDigitalGain;
>         context.activeState.agc.constraintMode = agc_.constraintModes().begin()->first;
>         context.activeState.agc.exposureMode = agc_.exposureModeHelpers().begin()->first;
>  
> @@ -226,32 +213,6 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame,
>                         << "Analogue gain set to " << agc.manual.sensorGain
>                         << " on request sequence " << frame;
>         }
> -
> -       const auto &digitalGain = controls.get(controls::DigitalGain);
> -       if (digitalGain) {
> -               agc.manual.ispGain = *digitalGain;
> -
> -               LOG(MaliC55Agc, Debug)
> -                       << "Digital gain set to " << agc.manual.ispGain
> -                       << " on request sequence " << frame;
> -       }
> -}
> -
> -void Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContext,
> -                            MaliC55Params *params)
> -{
> -       IPAActiveState &activeState = context.activeState;
> -       UQ<5, 8> gain;
> -
> -       if (activeState.agc.autoEnabled)
> -               gain = activeState.agc.automatic.ispGain;
> -       else
> -               gain = activeState.agc.manual.ispGain;
> -
> -       auto block = params->block<MaliC55Blocks::Dgain>();
> -       block->gain = gain.quantized();
> -
> -       frameContext.agc.ispGain = gain;
>  }
>  
>  void Agc::fillParamsBuffer(MaliC55Params *params, enum MaliC55Blocks type)
> @@ -304,11 +265,9 @@ void Agc::fillWeightsArrayBuffer(MaliC55Params *params, const enum MaliC55Blocks
>         std::fill(weights.begin(), weights.end(), 1);
>  }
>  
> -void Agc::prepare(IPAContext &context, const uint32_t frame,
> -                 IPAFrameContext &frameContext, MaliC55Params *params)
> +void Agc::prepare([[maybe_unused]] IPAContext &context, const uint32_t frame,
> +                 [[maybe_unused]]  IPAFrameContext &frameContext, MaliC55Params *params)
>  {
> -       fillGainParamBlock(context, frameContext, params);
> -
>         if (frame > 0)
>                 return;
>  
> @@ -370,10 +329,8 @@ void Agc::process(IPAContext &context,
>          */
>         uint32_t exposure = frameContext.agc.exposure;
>         double analogueGain = frameContext.agc.sensorGain;
> -       double digitalGain = frameContext.agc.ispGain.value();
> -       double totalGain = analogueGain * digitalGain;
>         utils::Duration currentShutter = exposure * configuration.sensor.lineDuration;
> -       utils::Duration effectiveExposureValue = currentShutter * totalGain;
> +       utils::Duration effectiveExposureValue = currentShutter * analogueGain;
>         AgcTraits agcTraits(statistics_);
>  
>  
> @@ -384,21 +341,15 @@ void Agc::process(IPAContext &context,
>                                     activeState.agc.exposureMode, statistics_.yHist,
>                                     effectiveExposureValue, agcTraits);
>  
> -       UQ<5, 8> dGainQ = std::clamp(static_cast<float>(dGain),
> -                                    kMinDigitalGain,
> -                                    kMaxDigitalGain);
> -
>         LOG(MaliC55Agc, Debug)
>                 << "Divided up shutter, analogue gain and digital gain are "
> -               << shutterTime << ", " << aGain << " and " << dGainQ;
> +               << shutterTime << ", " << aGain << " and " << dGain;
>  
>         activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
>         activeState.agc.automatic.sensorGain = aGain;
> -       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.value());
>         metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK);
>  }
>  
> diff --git a/src/ipa/mali-c55/algorithms/agc.h b/src/ipa/mali-c55/algorithms/agc.h
> index e36378a2ac..ec7fe28c17 100644
> --- a/src/ipa/mali-c55/algorithms/agc.h
> +++ b/src/ipa/mali-c55/algorithms/agc.h
> @@ -64,9 +64,6 @@ public:
>                      ControlList &metadata) override;
>  
>  private:
> -       void fillGainParamBlock(IPAContext &context,
> -                               IPAFrameContext &frameContext,
> -                               MaliC55Params *params);
>         void fillParamsBuffer(MaliC55Params *params, enum MaliC55Blocks type);
>         void fillWeightsArrayBuffer(MaliC55Params *params, enum MaliC55Blocks type);
>  
> diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h
> index ac4b837738..075d6f66ef 100644
> --- a/src/ipa/mali-c55/ipa_context.h
> +++ b/src/ipa/mali-c55/ipa_context.h
> @@ -41,12 +41,10 @@ struct IPAActiveState {
>                 struct {
>                         uint32_t exposure;
>                         double sensorGain;
> -                       UQ<5, 8> ispGain;
>                 } automatic;
>                 struct {
>                         uint32_t exposure;
>                         double sensorGain;
> -                       UQ<5, 8> ispGain;
>                 } manual;
>                 bool autoEnabled;
>                 uint32_t constraintMode;
> @@ -64,7 +62,6 @@ struct IPAFrameContext : public FrameContext {
>         struct {
>                 uint32_t exposure;
>                 double sensorGain;
> -               UQ<5, 8> ispGain;
>         } agc;
>  
>         struct {
> -- 
> 2.54.0
>
Jacopo Mondi July 16, 2026, 4:26 p.m. UTC | #2 
Hi Barnabas

On Sat, Jul 04, 2026 at 06:27:30AM +0100, Kieran Bingham wrote:
> Quoting Barnabás Pőcze (2026-07-03 16:38:11)
> > The mali-c55 agc algorithm still mainly uses the `AeEnable` control,
> > but the interaction of that control and `AeEnable` is not well defined,
> > and not other platform supports setting it. So remove it for now until
> > things are properly defined.
>
> Is this message accurate with the $SUBJECT? On first read these sound
> like different things. Lets see what the patch says...
>
>
> Aha, ok so it was 'that control' == DigitalGain.
>
> >
> > Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/262
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> >  src/ipa/mali-c55/algorithms/agc.cpp | 57 ++---------------------------
> >  src/ipa/mali-c55/algorithms/agc.h   |  3 --
> >  src/ipa/mali-c55/ipa_context.h      |  3 --
> >  3 files changed, 4 insertions(+), 59 deletions(-)
> >
> > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
> > index 6e091603f9..8d104e01f2 100644
> > --- a/src/ipa/mali-c55/algorithms/agc.cpp
> > +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> > @@ -16,7 +16,6 @@
> >  #include <libcamera/property_ids.h>
> >
> >  #include "libipa/colours.h"
> > -#include "libipa/fixedpoint.h"
> >
> >  namespace libcamera {
> >
> > @@ -33,14 +32,6 @@ LOG_DEFINE_CATEGORY(MaliC55Agc)
> >   */
> >  static constexpr unsigned int kNumHistogramBins = 256;
> >
> > -/*
> > - * 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. We clamp from 1.0 to the actual
> > - * max value which is 8191 * 2^-8.
> > - */
> > -static constexpr float kMinDigitalGain = 1.0;
> > -static constexpr float kMaxDigitalGain = UQ<5, 8>::TraitsType::max;
> > -
>
> I'm a bit wary, as we'll want DigitalGain when Stefan's AGC work ends up
> in here as we use DG to give smoother AGC... but I'd be ok removing it
> at the moment if it helps with the cleanup to get there.

I agree with Kieran here.

Porting mali to the libipa algorithms will solve
https://gitlab.freedesktop.org/camera/libcamera/-/work_items/262

but do you think support for digital gain can be kept in the mali agc
after the porting has completed ?

>
> So a tentative. Dan/Stefan, I assume there's no objections here?
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>
> >  uint32_t AgcStatistics::decodeBinValue(uint16_t binVal)
> >  {
> >         int exponent = (binVal & 0xf000) >> 12;
> > @@ -137,8 +128,6 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
> >                 return ret;
> >
> >         context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true);
> > -       context.ctrlMap[&controls::DigitalGain] = ControlInfo(
> > -               kMinDigitalGain, kMaxDigitalGain, kMinDigitalGain);
> >         context.ctrlMap.merge(agc_.controls());
> >
> >         return 0;
> > @@ -158,10 +147,8 @@ int Agc::configure(IPAContext &context,
> >         context.activeState.agc.autoEnabled = true;
> >         context.activeState.agc.automatic.sensorGain = context.configuration.agc.minAnalogueGain;
> >         context.activeState.agc.automatic.exposure = context.configuration.agc.defaultExposure;
> > -       context.activeState.agc.automatic.ispGain = kMinDigitalGain;
> >         context.activeState.agc.manual.sensorGain = context.configuration.agc.minAnalogueGain;
> >         context.activeState.agc.manual.exposure = context.configuration.agc.defaultExposure;
> > -       context.activeState.agc.manual.ispGain = kMinDigitalGain;
> >         context.activeState.agc.constraintMode = agc_.constraintModes().begin()->first;
> >         context.activeState.agc.exposureMode = agc_.exposureModeHelpers().begin()->first;
> >
> > @@ -226,32 +213,6 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame,
> >                         << "Analogue gain set to " << agc.manual.sensorGain
> >                         << " on request sequence " << frame;
> >         }
> > -
> > -       const auto &digitalGain = controls.get(controls::DigitalGain);
> > -       if (digitalGain) {
> > -               agc.manual.ispGain = *digitalGain;
> > -
> > -               LOG(MaliC55Agc, Debug)
> > -                       << "Digital gain set to " << agc.manual.ispGain
> > -                       << " on request sequence " << frame;
> > -       }
> > -}
> > -
> > -void Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContext,
> > -                            MaliC55Params *params)
> > -{
> > -       IPAActiveState &activeState = context.activeState;
> > -       UQ<5, 8> gain;
> > -
> > -       if (activeState.agc.autoEnabled)
> > -               gain = activeState.agc.automatic.ispGain;
> > -       else
> > -               gain = activeState.agc.manual.ispGain;
> > -
> > -       auto block = params->block<MaliC55Blocks::Dgain>();
> > -       block->gain = gain.quantized();
> > -
> > -       frameContext.agc.ispGain = gain;
> >  }
> >
> >  void Agc::fillParamsBuffer(MaliC55Params *params, enum MaliC55Blocks type)
> > @@ -304,11 +265,9 @@ void Agc::fillWeightsArrayBuffer(MaliC55Params *params, const enum MaliC55Blocks
> >         std::fill(weights.begin(), weights.end(), 1);
> >  }
> >
> > -void Agc::prepare(IPAContext &context, const uint32_t frame,
> > -                 IPAFrameContext &frameContext, MaliC55Params *params)
> > +void Agc::prepare([[maybe_unused]] IPAContext &context, const uint32_t frame,
> > +                 [[maybe_unused]]  IPAFrameContext &frameContext, MaliC55Params *params)
> >  {
> > -       fillGainParamBlock(context, frameContext, params);
> > -
> >         if (frame > 0)
> >                 return;
> >
> > @@ -370,10 +329,8 @@ void Agc::process(IPAContext &context,
> >          */
> >         uint32_t exposure = frameContext.agc.exposure;
> >         double analogueGain = frameContext.agc.sensorGain;
> > -       double digitalGain = frameContext.agc.ispGain.value();
> > -       double totalGain = analogueGain * digitalGain;
> >         utils::Duration currentShutter = exposure * configuration.sensor.lineDuration;
> > -       utils::Duration effectiveExposureValue = currentShutter * totalGain;
> > +       utils::Duration effectiveExposureValue = currentShutter * analogueGain;
> >         AgcTraits agcTraits(statistics_);
> >
> >
> > @@ -384,21 +341,15 @@ void Agc::process(IPAContext &context,
> >                                     activeState.agc.exposureMode, statistics_.yHist,
> >                                     effectiveExposureValue, agcTraits);
> >
> > -       UQ<5, 8> dGainQ = std::clamp(static_cast<float>(dGain),
> > -                                    kMinDigitalGain,
> > -                                    kMaxDigitalGain);
> > -
> >         LOG(MaliC55Agc, Debug)
> >                 << "Divided up shutter, analogue gain and digital gain are "
> > -               << shutterTime << ", " << aGain << " and " << dGainQ;
> > +               << shutterTime << ", " << aGain << " and " << dGain;
> >
> >         activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
> >         activeState.agc.automatic.sensorGain = aGain;
> > -       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.value());
> >         metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK);
> >  }
> >
> > diff --git a/src/ipa/mali-c55/algorithms/agc.h b/src/ipa/mali-c55/algorithms/agc.h
> > index e36378a2ac..ec7fe28c17 100644
> > --- a/src/ipa/mali-c55/algorithms/agc.h
> > +++ b/src/ipa/mali-c55/algorithms/agc.h
> > @@ -64,9 +64,6 @@ public:
> >                      ControlList &metadata) override;
> >
> >  private:
> > -       void fillGainParamBlock(IPAContext &context,
> > -                               IPAFrameContext &frameContext,
> > -                               MaliC55Params *params);
> >         void fillParamsBuffer(MaliC55Params *params, enum MaliC55Blocks type);
> >         void fillWeightsArrayBuffer(MaliC55Params *params, enum MaliC55Blocks type);
> >
> > diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h
> > index ac4b837738..075d6f66ef 100644
> > --- a/src/ipa/mali-c55/ipa_context.h
> > +++ b/src/ipa/mali-c55/ipa_context.h
> > @@ -41,12 +41,10 @@ struct IPAActiveState {
> >                 struct {
> >                         uint32_t exposure;
> >                         double sensorGain;
> > -                       UQ<5, 8> ispGain;
> >                 } automatic;
> >                 struct {
> >                         uint32_t exposure;
> >                         double sensorGain;
> > -                       UQ<5, 8> ispGain;
> >                 } manual;
> >                 bool autoEnabled;
> >                 uint32_t constraintMode;
> > @@ -64,7 +62,6 @@ struct IPAFrameContext : public FrameContext {
> >         struct {
> >                 uint32_t exposure;
> >                 double sensorGain;
> > -               UQ<5, 8> ispGain;
> >         } agc;
> >
> >         struct {
> > --
> > 2.54.0
> >
Barnabás Pőcze July 16, 2026, 4:42 p.m. UTC | #3 
2026. 07. 16. 18:26 keltezéssel, Jacopo Mondi írta:
> Hi Barnabas
> 
> On Sat, Jul 04, 2026 at 06:27:30AM +0100, Kieran Bingham wrote:
>> Quoting Barnabás Pőcze (2026-07-03 16:38:11)
>>> The mali-c55 agc algorithm still mainly uses the `AeEnable` control,
>>> but the interaction of that control and `AeEnable` is not well defined,
>>> and not other platform supports setting it. So remove it for now until
>>> things are properly defined.
>>
>> Is this message accurate with the $SUBJECT? On first read these sound
>> like different things. Lets see what the patch says...
>>
>>
>> Aha, ok so it was 'that control' == DigitalGain.
>>
>>>
>>> Link: https://gitlab.freedesktop.org/camera/libcamera/-/work_items/262
>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>> ---
>>>   src/ipa/mali-c55/algorithms/agc.cpp | 57 ++---------------------------
>>>   src/ipa/mali-c55/algorithms/agc.h   |  3 --
>>>   src/ipa/mali-c55/ipa_context.h      |  3 --
>>>   3 files changed, 4 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
>>> index 6e091603f9..8d104e01f2 100644
>>> --- a/src/ipa/mali-c55/algorithms/agc.cpp
>>> +++ b/src/ipa/mali-c55/algorithms/agc.cpp
>>> @@ -16,7 +16,6 @@
>>>   #include <libcamera/property_ids.h>
>>>
>>>   #include "libipa/colours.h"
>>> -#include "libipa/fixedpoint.h"
>>>
>>>   namespace libcamera {
>>>
>>> @@ -33,14 +32,6 @@ LOG_DEFINE_CATEGORY(MaliC55Agc)
>>>    */
>>>   static constexpr unsigned int kNumHistogramBins = 256;
>>>
>>> -/*
>>> - * 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. We clamp from 1.0 to the actual
>>> - * max value which is 8191 * 2^-8.
>>> - */
>>> -static constexpr float kMinDigitalGain = 1.0;
>>> -static constexpr float kMaxDigitalGain = UQ<5, 8>::TraitsType::max;
>>> -
>>
>> I'm a bit wary, as we'll want DigitalGain when Stefan's AGC work ends up
>> in here as we use DG to give smoother AGC... but I'd be ok removing it
>> at the moment if it helps with the cleanup to get there.
> 
> I agree with Kieran here.
> 
> Porting mali to the libipa algorithms will solve
> https://gitlab.freedesktop.org/camera/libcamera/-/work_items/262
> 
> but do you think support for digital gain can be kept in the mali agc
> after the porting has completed ?

I don't know. I think there is a significant issue, namely that the
interaction wrt. AeEnable is not well defined. The mali-c55 parts
use it to control digital gain as well, but this is wholly undocumented.
And with this deduplication work AeEnable handling will disappear,
so what to do? Still check AeEnable here to control digital gain?

I feel like the cleanest solution is to remove DigitalGain support
for the time being, until something like DigitalGainMode is added.
And of course I suppose the rpi parts need to be considered as well
(as far as I can see it is only reported in metadata there, so
  maybe the option to set it could be removed from mali-c55 as
  a compromise for the time being).


> 
>>
>> So a tentative. Dan/Stefan, I assume there's no objections here?
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>>
>>>   uint32_t AgcStatistics::decodeBinValue(uint16_t binVal)
>>>   {
>>>          int exponent = (binVal & 0xf000) >> 12;
>>> @@ -137,8 +128,6 @@ int Agc::init(IPAContext &context, const ValueNode &tuningData)
>>>                  return ret;
>>>
>>>          context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true);
>>> -       context.ctrlMap[&controls::DigitalGain] = ControlInfo(
>>> -               kMinDigitalGain, kMaxDigitalGain, kMinDigitalGain);
>>>          context.ctrlMap.merge(agc_.controls());
>>>
>>>          return 0;
>>> @@ -158,10 +147,8 @@ int Agc::configure(IPAContext &context,
>>>          context.activeState.agc.autoEnabled = true;
>>>          context.activeState.agc.automatic.sensorGain = context.configuration.agc.minAnalogueGain;
>>>          context.activeState.agc.automatic.exposure = context.configuration.agc.defaultExposure;
>>> -       context.activeState.agc.automatic.ispGain = kMinDigitalGain;
>>>          context.activeState.agc.manual.sensorGain = context.configuration.agc.minAnalogueGain;
>>>          context.activeState.agc.manual.exposure = context.configuration.agc.defaultExposure;
>>> -       context.activeState.agc.manual.ispGain = kMinDigitalGain;
>>>          context.activeState.agc.constraintMode = agc_.constraintModes().begin()->first;
>>>          context.activeState.agc.exposureMode = agc_.exposureModeHelpers().begin()->first;
>>>
>>> @@ -226,32 +213,6 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame,
>>>                          << "Analogue gain set to " << agc.manual.sensorGain
>>>                          << " on request sequence " << frame;
>>>          }
>>> -
>>> -       const auto &digitalGain = controls.get(controls::DigitalGain);
>>> -       if (digitalGain) {
>>> -               agc.manual.ispGain = *digitalGain;
>>> -
>>> -               LOG(MaliC55Agc, Debug)
>>> -                       << "Digital gain set to " << agc.manual.ispGain
>>> -                       << " on request sequence " << frame;
>>> -       }
>>> -}
>>> -
>>> -void Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContext,
>>> -                            MaliC55Params *params)
>>> -{
>>> -       IPAActiveState &activeState = context.activeState;
>>> -       UQ<5, 8> gain;
>>> -
>>> -       if (activeState.agc.autoEnabled)
>>> -               gain = activeState.agc.automatic.ispGain;
>>> -       else
>>> -               gain = activeState.agc.manual.ispGain;
>>> -
>>> -       auto block = params->block<MaliC55Blocks::Dgain>();
>>> -       block->gain = gain.quantized();
>>> -
>>> -       frameContext.agc.ispGain = gain;
>>>   }
>>>
>>>   void Agc::fillParamsBuffer(MaliC55Params *params, enum MaliC55Blocks type)
>>> @@ -304,11 +265,9 @@ void Agc::fillWeightsArrayBuffer(MaliC55Params *params, const enum MaliC55Blocks
>>>          std::fill(weights.begin(), weights.end(), 1);
>>>   }
>>>
>>> -void Agc::prepare(IPAContext &context, const uint32_t frame,
>>> -                 IPAFrameContext &frameContext, MaliC55Params *params)
>>> +void Agc::prepare([[maybe_unused]] IPAContext &context, const uint32_t frame,
>>> +                 [[maybe_unused]]  IPAFrameContext &frameContext, MaliC55Params *params)
>>>   {
>>> -       fillGainParamBlock(context, frameContext, params);
>>> -
>>>          if (frame > 0)
>>>                  return;
>>>
>>> @@ -370,10 +329,8 @@ void Agc::process(IPAContext &context,
>>>           */
>>>          uint32_t exposure = frameContext.agc.exposure;
>>>          double analogueGain = frameContext.agc.sensorGain;
>>> -       double digitalGain = frameContext.agc.ispGain.value();
>>> -       double totalGain = analogueGain * digitalGain;
>>>          utils::Duration currentShutter = exposure * configuration.sensor.lineDuration;
>>> -       utils::Duration effectiveExposureValue = currentShutter * totalGain;
>>> +       utils::Duration effectiveExposureValue = currentShutter * analogueGain;
>>>          AgcTraits agcTraits(statistics_);
>>>
>>>
>>> @@ -384,21 +341,15 @@ void Agc::process(IPAContext &context,
>>>                                      activeState.agc.exposureMode, statistics_.yHist,
>>>                                      effectiveExposureValue, agcTraits);
>>>
>>> -       UQ<5, 8> dGainQ = std::clamp(static_cast<float>(dGain),
>>> -                                    kMinDigitalGain,
>>> -                                    kMaxDigitalGain);
>>> -
>>>          LOG(MaliC55Agc, Debug)
>>>                  << "Divided up shutter, analogue gain and digital gain are "
>>> -               << shutterTime << ", " << aGain << " and " << dGainQ;
>>> +               << shutterTime << ", " << aGain << " and " << dGain;
>>>
>>>          activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
>>>          activeState.agc.automatic.sensorGain = aGain;
>>> -       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.value());
>>>          metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK);
>>>   }
>>>
>>> diff --git a/src/ipa/mali-c55/algorithms/agc.h b/src/ipa/mali-c55/algorithms/agc.h
>>> index e36378a2ac..ec7fe28c17 100644
>>> --- a/src/ipa/mali-c55/algorithms/agc.h
>>> +++ b/src/ipa/mali-c55/algorithms/agc.h
>>> @@ -64,9 +64,6 @@ public:
>>>                       ControlList &metadata) override;
>>>
>>>   private:
>>> -       void fillGainParamBlock(IPAContext &context,
>>> -                               IPAFrameContext &frameContext,
>>> -                               MaliC55Params *params);
>>>          void fillParamsBuffer(MaliC55Params *params, enum MaliC55Blocks type);
>>>          void fillWeightsArrayBuffer(MaliC55Params *params, enum MaliC55Blocks type);
>>>
>>> diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h
>>> index ac4b837738..075d6f66ef 100644
>>> --- a/src/ipa/mali-c55/ipa_context.h
>>> +++ b/src/ipa/mali-c55/ipa_context.h
>>> @@ -41,12 +41,10 @@ struct IPAActiveState {
>>>                  struct {
>>>                          uint32_t exposure;
>>>                          double sensorGain;
>>> -                       UQ<5, 8> ispGain;
>>>                  } automatic;
>>>                  struct {
>>>                          uint32_t exposure;
>>>                          double sensorGain;
>>> -                       UQ<5, 8> ispGain;
>>>                  } manual;
>>>                  bool autoEnabled;
>>>                  uint32_t constraintMode;
>>> @@ -64,7 +62,6 @@ struct IPAFrameContext : public FrameContext {
>>>          struct {
>>>                  uint32_t exposure;
>>>                  double sensorGain;
>>> -               UQ<5, 8> ispGain;
>>>          } agc;
>>>
>>>          struct {
>>> --
>>> 2.54.0
>>>

Patch
diff mbox series 

diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
index 6e091603f9..8d104e01f2 100644
--- a/src/ipa/mali-c55/algorithms/agc.cpp
+++ b/src/ipa/mali-c55/algorithms/agc.cpp
@@ -16,7 +16,6 @@ 
 #include <libcamera/property_ids.h>
 
 #include "libipa/colours.h"
-#include "libipa/fixedpoint.h"
 
 namespace libcamera {
 
@@ -33,14 +32,6 @@  LOG_DEFINE_CATEGORY(MaliC55Agc)
  */
 static constexpr unsigned int kNumHistogramBins = 256;
 
-/*
- * 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. We clamp from 1.0 to the actual
- * max value which is 8191 * 2^-8.
- */
-static constexpr float kMinDigitalGain = 1.0;
-static constexpr float kMaxDigitalGain = UQ<5, 8>::TraitsType::max;
-
 uint32_t AgcStatistics::decodeBinValue(uint16_t binVal)
 {
 	int exponent = (binVal & 0xf000) >> 12;
@@ -137,8 +128,6 @@  int Agc::init(IPAContext &context, const ValueNode &tuningData)
 		return ret;
 
 	context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true);
-	context.ctrlMap[&controls::DigitalGain] = ControlInfo(
-		kMinDigitalGain, kMaxDigitalGain, kMinDigitalGain);
 	context.ctrlMap.merge(agc_.controls());
 
 	return 0;
@@ -158,10 +147,8 @@  int Agc::configure(IPAContext &context,
 	context.activeState.agc.autoEnabled = true;
 	context.activeState.agc.automatic.sensorGain = context.configuration.agc.minAnalogueGain;
 	context.activeState.agc.automatic.exposure = context.configuration.agc.defaultExposure;
-	context.activeState.agc.automatic.ispGain = kMinDigitalGain;
 	context.activeState.agc.manual.sensorGain = context.configuration.agc.minAnalogueGain;
 	context.activeState.agc.manual.exposure = context.configuration.agc.defaultExposure;
-	context.activeState.agc.manual.ispGain = kMinDigitalGain;
 	context.activeState.agc.constraintMode = agc_.constraintModes().begin()->first;
 	context.activeState.agc.exposureMode = agc_.exposureModeHelpers().begin()->first;
 
@@ -226,32 +213,6 @@  void Agc::queueRequest(IPAContext &context, const uint32_t frame,
 			<< "Analogue gain set to " << agc.manual.sensorGain
 			<< " on request sequence " << frame;
 	}
-
-	const auto &digitalGain = controls.get(controls::DigitalGain);
-	if (digitalGain) {
-		agc.manual.ispGain = *digitalGain;
-
-		LOG(MaliC55Agc, Debug)
-			<< "Digital gain set to " << agc.manual.ispGain
-			<< " on request sequence " << frame;
-	}
-}
-
-void Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContext,
-			     MaliC55Params *params)
-{
-	IPAActiveState &activeState = context.activeState;
-	UQ<5, 8> gain;
-
-	if (activeState.agc.autoEnabled)
-		gain = activeState.agc.automatic.ispGain;
-	else
-		gain = activeState.agc.manual.ispGain;
-
-	auto block = params->block<MaliC55Blocks::Dgain>();
-	block->gain = gain.quantized();
-
-	frameContext.agc.ispGain = gain;
 }
 
 void Agc::fillParamsBuffer(MaliC55Params *params, enum MaliC55Blocks type)
@@ -304,11 +265,9 @@  void Agc::fillWeightsArrayBuffer(MaliC55Params *params, const enum MaliC55Blocks
 	std::fill(weights.begin(), weights.end(), 1);
 }
 
-void Agc::prepare(IPAContext &context, const uint32_t frame,
-		  IPAFrameContext &frameContext, MaliC55Params *params)
+void Agc::prepare([[maybe_unused]] IPAContext &context, const uint32_t frame,
+		  [[maybe_unused]]  IPAFrameContext &frameContext, MaliC55Params *params)
 {
-	fillGainParamBlock(context, frameContext, params);
-
 	if (frame > 0)
 		return;
 
@@ -370,10 +329,8 @@  void Agc::process(IPAContext &context,
 	 */
 	uint32_t exposure = frameContext.agc.exposure;
 	double analogueGain = frameContext.agc.sensorGain;
-	double digitalGain = frameContext.agc.ispGain.value();
-	double totalGain = analogueGain * digitalGain;
 	utils::Duration currentShutter = exposure * configuration.sensor.lineDuration;
-	utils::Duration effectiveExposureValue = currentShutter * totalGain;
+	utils::Duration effectiveExposureValue = currentShutter * analogueGain;
 	AgcTraits agcTraits(statistics_);
 
 
@@ -384,21 +341,15 @@  void Agc::process(IPAContext &context,
 				    activeState.agc.exposureMode, statistics_.yHist,
 				    effectiveExposureValue, agcTraits);
 
-	UQ<5, 8> dGainQ = std::clamp(static_cast<float>(dGain),
-				     kMinDigitalGain,
-				     kMaxDigitalGain);
-
 	LOG(MaliC55Agc, Debug)
 		<< "Divided up shutter, analogue gain and digital gain are "
-		<< shutterTime << ", " << aGain << " and " << dGainQ;
+		<< shutterTime << ", " << aGain << " and " << dGain;
 
 	activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;
 	activeState.agc.automatic.sensorGain = aGain;
-	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.value());
 	metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK);
 }
 
diff --git a/src/ipa/mali-c55/algorithms/agc.h b/src/ipa/mali-c55/algorithms/agc.h
index e36378a2ac..ec7fe28c17 100644
--- a/src/ipa/mali-c55/algorithms/agc.h
+++ b/src/ipa/mali-c55/algorithms/agc.h
@@ -64,9 +64,6 @@  public:
 		     ControlList &metadata) override;
 
 private:
-	void fillGainParamBlock(IPAContext &context,
-				IPAFrameContext &frameContext,
-				MaliC55Params *params);
 	void fillParamsBuffer(MaliC55Params *params, enum MaliC55Blocks type);
 	void fillWeightsArrayBuffer(MaliC55Params *params, enum MaliC55Blocks type);
 
diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h
index ac4b837738..075d6f66ef 100644
--- a/src/ipa/mali-c55/ipa_context.h
+++ b/src/ipa/mali-c55/ipa_context.h
@@ -41,12 +41,10 @@  struct IPAActiveState {
 		struct {
 			uint32_t exposure;
 			double sensorGain;
-			UQ<5, 8> ispGain;
 		} automatic;
 		struct {
 			uint32_t exposure;
 			double sensorGain;
-			UQ<5, 8> ispGain;
 		} manual;
 		bool autoEnabled;
 		uint32_t constraintMode;
@@ -64,7 +62,6 @@  struct IPAFrameContext : public FrameContext {
 	struct {
 		uint32_t exposure;
 		double sensorGain;
-		UQ<5, 8> ispGain;
 	} agc;
 
 	struct {