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

[Jean-Michel Hautbois]

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.

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