[libcamera-devel] [PATCH v1 8/8] ipa: rkisp1: agc: Configure the number of cells used by HW
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Nov 19 17:05:54 CET 2021
On Fri, Nov 19, 2021 at 02:47:12PM +0000, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-11-19 14:41:16)
> > On 19/11/2021 15:00, Laurent Pinchart wrote:
> > > 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.
That was my idea, if all (or most) of the hw*_ fields currently stored
in IPARkISP1 are specific to a single algorithm, they can be computed
from the revision in the algorithms and stored there.
> > >>> 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
More information about the libcamera-devel
mailing list