[libcamera-devel] [PATCH v1 8/8] ipa: rkisp1: agc: Configure the number of cells used by HW
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Nov 19 15:47:12 CET 2021
Quoting Jean-Michel Hautbois (2021-11-19 14:41:16)
> 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) ?
We'd have to be careful about how we clean the IPAContext too.
Currently it gets reset/cleared during configure...
It might make sense to be able to keep the algorithm specific
requirements closely associated with it's algorithm though.
>
> >
> >>> 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;
> >
More information about the libcamera-devel
mailing list