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

[Kieran Bingham]

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.


>                 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
;-)


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;
> -- 
> 2.32.0
>