[PATCH 2/5] ipa: rkisp1: agc: Add digital gain

[Kieran Bingham]

Quoting Umang Jain (2024-04-12 08:59:01)
> Hi Paul,
> 
> On 05/04/24 8:17 pm, Paul Elder wrote:
> > Add support to the rkisp1 AGC to set digital gain.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> >   src/ipa/rkisp1/algorithms/agc.cpp | 5 +++++
> >   src/ipa/rkisp1/ipa_context.h      | 3 +++
> >   src/ipa/rkisp1/rkisp1.cpp         | 2 ++
> >   3 files changed, 10 insertions(+)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index fd47ba4e..7220f00a 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -10,6 +10,7 @@
> >   #include <algorithm>
> >   #include <chrono>
> >   #include <cmath>
> > +#include <tuple>
> 
> Is this required? Not sure by reading this particular patch
> 
> Looks good to me otherwise. With this addressed,
> 
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> 
> >   
> >   #include <libcamera/base/log.h>
> >   #include <libcamera/base/utils.h>
> > @@ -152,8 +153,10 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >       context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain;
> >       context.activeState.agc.automatic.exposure =
> >               10ms / context.configuration.sensor.lineDuration;
> > +     context.activeState.agc.automatic.dgain = 1;
> >       context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;
> >       context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> > +     context.activeState.agc.manual.dgain = 1;
> >       context.activeState.agc.autoEnabled = !context.configuration.raw;
> >   
> >       /*
> > @@ -237,6 +240,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
> >       if (frameContext.agc.autoEnabled) {
> >               frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
> >               frameContext.agc.gain = context.activeState.agc.automatic.gain;
> > +             frameContext.agc.dgain = context.activeState.agc.automatic.dgain;
> >       }
> >   
> >       /* \todo Remove this when we can set the below with controls */
> > @@ -380,6 +384,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >       /* Update the estimated exposure and gain. */
> >       activeState.agc.automatic.exposure = shutterTime / context.configuration.sensor.lineDuration;
> >       activeState.agc.automatic.gain = aGain;
> > +     activeState.agc.automatic.dgain = dGain;
> >   
> >       fillMetadata(context, frameContext, metadata);
> >   }
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 256b75eb..a70c7eb3 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -61,10 +61,12 @@ struct IPAActiveState {
> >               struct {
> >                       uint32_t exposure;
> >                       double gain;
> > +                     double dgain;
> >               } manual;
> >               struct {
> >                       uint32_t exposure;
> >                       double gain;
> > +                     double dgain;
> >               } automatic;
> >   
> >               bool autoEnabled;
> > @@ -110,6 +112,7 @@ struct IPAFrameContext : public FrameContext {
> >       struct {
> >               uint32_t exposure;
> >               double gain;
> > +             double dgain;
> >               bool autoEnabled;
> >       } agc;
> >   
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index b0bbcd8c..d66dfdd7 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -446,10 +446,12 @@ void IPARkISP1::setControls(unsigned int frame)
> >       IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> >       uint32_t exposure = frameContext.agc.exposure;
> >       uint32_t gain = camHelper_->gainCode(frameContext.agc.gain);
> > +     uint32_t dgain = camHelper_->gainCode(frameContext.agc.dgain);
> >   
> >       ControlList ctrls(sensorControls_);
> >       ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> >       ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> > +     ctrls.set(V4L2_CID_DIGITAL_GAIN, static_cast<int32_t>(dgain));

Not all sensors will have a digital gain, and digital gain models can be
different to analogue gain models so they would require a separate
helper, and can not use gainCode().

I assume we can handle some digital gain on the RKISP1 right? So this
should be being applied through there - not the sensor.

--
Kieran


> >   
> >       setSensorControls.emit(frame, ctrls);
> >   }
>