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

[Jean-Michel Hautbois]

Hi Laurent,

On 19/11/2021 15:00, Laurent Pinchart wrote:
> 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 ?

It is possible, but is there any interest in doing so (except that it 
makes it out from IPAContext) ?

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