| Message ID | 20211119111654.68445-9-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@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 >
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@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@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 >>
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@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 ? > > Reviewed-by: Kieran Bingham <kieran.bingham@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;
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@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@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; >
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@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@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; > >
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@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@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;
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; + 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++) { 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; 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;
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@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(-)