[libcamera-devel] [PATCH v1 8/8] ipa: rkisp1: agc: Configure the number of cells used by HW

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Fri, Nov 19, 2021 at 12:35:22PM +0100, Jean-Michel Hautbois wrote:
> On 19/11/2021 12:29, Kieran Bingham wrote:
> > Quoting Jean-Michel Hautbois (2021-11-19 11:16:54)
> >> The ISP can use 25 or 81 cells depending on its revision. Remove the
> >> cached value in IPARkISP1 and use IPASessionConfiguration to store it
> >> and pass it to AGC.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >> ---
> >>   src/ipa/ipu3/ipa_context.cpp      | 3 +++
> >>   src/ipa/rkisp1/algorithms/agc.cpp | 4 +++-
> >>   src/ipa/rkisp1/algorithms/agc.h   | 2 ++
> >>   src/ipa/rkisp1/ipa_context.h      | 1 +
> >>   src/ipa/rkisp1/rkisp1.cpp         | 5 ++---
> >>   5 files changed, 11 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp
> >> index 99caf9ad..71a8619b 100644
> >> --- a/src/ipa/ipu3/ipa_context.cpp
> >> +++ b/src/ipa/ipu3/ipa_context.cpp
> >> @@ -84,6 +84,9 @@ namespace libcamera::ipa::ipu3 {
> >>    *
> >>    * \var IPASessionConfiguration::grid.maxAnalogueGain
> >>    * \brief Maximum analogue gain supported with the configured sensor
> >> + *
> >> + * \var IPASessionConfiguration::agc.numCells
> >> + * \brief Number of cells used by the ISP to store the Y means
> >>    */
> >>   
> >>   /**
> >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> >> index 2b7c6fda..471cf584 100644
> >> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> >> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> >> @@ -82,6 +82,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >>          context.frameContext.agc.gain = minAnalogueGain_;
> >>          context.frameContext.agc.exposure = 10ms / lineDuration_;
> >>   
> >> +       numCells_ = context.configuration.agc.numCells;
> > 
> > Does the AGC need to store this? or can it always access the context
> > when it needs it?
> > 
> >> +
> >>          return 0;
> >>   }
> >>   
> >> @@ -195,7 +197,7 @@ double Agc::computeInitialY(const rkisp1_cif_isp_ae_stat *ae, double currentYGai
> >>          double ySum = 0.0;
> >>          unsigned int num = 0;
> >>   
> >> -       for (unsigned int aeCell = 0; aeCell < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; aeCell++) {
> >> +       for (unsigned int aeCell = 0; aeCell < numCells_; aeCell++) {
> > 
> > Ah - ok that answers that - this function isn't being given the context.
> > So this is fine for me.
> 
> You got me ! Yes, I will remove the cached value when AWB will be 
> introduced, as I will then pass the context to the function to get the 
> awb gains applied ;-).
> 
> >>                  ySum += ae->exp_mean[aeCell] * currentYGain;
> >>                  num++;
> >>          }
> >> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> >> index 934a455e..f4293035 100644
> >> --- a/src/ipa/rkisp1/algorithms/agc.h
> >> +++ b/src/ipa/rkisp1/algorithms/agc.h
> >> @@ -45,6 +45,8 @@ private:
> >>          double minAnalogueGain_;
> >>          double maxAnalogueGain_;
> >>   
> >> +       uint32_t numCells_;
> >> +
> >>          utils::Duration filteredExposure_;
> >>          utils::Duration currentExposure_;
> >>   };
> >> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> >> index ca48ffc5..e8e1dc47 100644
> >> --- a/src/ipa/rkisp1/ipa_context.h
> >> +++ b/src/ipa/rkisp1/ipa_context.h
> >> @@ -24,6 +24,7 @@ struct IPASessionConfiguration {
> >>                  utils::Duration maxShutterSpeed;
> >>                  double minAnalogueGain;
> >>                  double maxAnalogueGain;
> >> +               uint32_t numCells;
> >>          } agc;
> >>   };
> >>   
> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >> index b1bfb8b6..cebf2c65 100644
> >> --- a/src/ipa/rkisp1/rkisp1.cpp
> >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >> @@ -75,7 +75,6 @@ private:
> >>          uint32_t maxGain_;
> >>   
> >>          /* revision-specific data */
> >> -       unsigned int hwAeMeanMax_;
> >>          unsigned int hwHistBinNMax_;
> >>          unsigned int hwGammaOutMaxSamples_;
> >>          unsigned int hwHistogramWeightGridsSize_;
> >> @@ -98,13 +97,13 @@ int IPARkISP1::init([[maybe_unused]] const IPASettings &settings,
> >>          /* \todo Add support for other revisions */
> >>          switch (hwRevision) {
> >>          case RKISP1_V10:
> >> -               hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> >> +               context_.configuration.agc.numCells = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> >>                  hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
> >>                  hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
> >>                  hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
> > 
> > I suspect each of those are going to move into the context too... the
> > algorithms shouldn't have to dig into the top level class. But .. later
> > ;-)
> 
> Each algorithm will remove the cached value which won't be needed anymore.

Would it make sense to store the hardware revision in the context, and
compute (and cache) the derived values such as numCells in the
algorithms ?

> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> >>                  break;
> >>          case RKISP1_V12:
> >> -               hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> >> +               context_.configuration.agc.numCells = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> >>                  hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
> >>                  hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
> >>                  hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;

-- 
Regards,

Laurent Pinchart