| Message ID | 20211123091451.67404-12-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jean-Michel, Thank you for the patch. On Tue, Nov 23, 2021 at 10:14:51AM +0100, Jean-Michel Hautbois wrote: > 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/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++- > src/ipa/rkisp1/algorithms/agc.h | 2 ++ > src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++ > src/ipa/rkisp1/ipa_context.h | 4 ++++ > src/ipa/rkisp1/rkisp1.cpp | 10 +++++++--- > 5 files changed, 36 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 0433813a..e5358ca3 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -61,7 +61,7 @@ Agc::Agc() > * \param[in] context The shared IPA context > * \param[in] configInfo The IPA configuration data > * > - * \return 0 > + * \return 0 on success or a negative error code otherwise > */ > int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > { > @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > context.frameContext.agc.gain = minAnalogueGain_; > context.frameContext.agc.exposure = 10ms / lineDuration_; > > + switch (context.configuration.hw.revision) { > + case RKISP1_V10: > + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; > + break; Blank line. Below too. > + case RKISP1_V12: > + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; > + break; > + default: > + LOG(RkISP1Agc, Error) > + << "Hardware revision " << context.configuration.hw.revision > + << " is currently not supported"; > + return -ENODEV; We already fail at init() time if the hardware revision is unknown, so I'd skip this. If you want to make it more full-proof, you could create an enum for the hw.revision field, the compiler will then warn if some values are not handled. > + } > + > return 0; > } > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > index ac95dea5..3ab3f347 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.cpp b/src/ipa/rkisp1/ipa_context.cpp > index 16fc248f..cdb952be 100644 > --- a/src/ipa/rkisp1/ipa_context.cpp > +++ b/src/ipa/rkisp1/ipa_context.cpp > @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 { > * \brief Maximum analogue gain supported with the configured sensor > */ > > +/** > + * \var IPASessionConfiguration::hw > + * \brief RkISP1 specific hardware information s/ specific/-specific/ > + * > + * \var IPASessionConfiguration::hw.revision > + * \brief RkISP1 revision reported * \brief Hardware revision of the ISP (RKISP1_V*) > + */ > + > /** > * \var IPAFrameContext::agc > * \brief Context for the Automatic Gain Control algorithm > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index 8e756fb1..316036c6 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -21,6 +21,10 @@ struct IPASessionConfiguration { > double minAnalogueGain; > double maxAnalogueGain; > } agc; > + > + struct { > + uint32_t revision; > + } hw; > }; > > struct IPAFrameContext { > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 59e868ff..a472d111 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -75,7 +75,7 @@ private: > uint32_t maxGain_; > > /* revision-specific data */ > - unsigned int hwAeMeanMax_; > + unsigned int hwRevision_; > unsigned int hwHistBinNMax_; > unsigned int hwGammaOutMaxSamples_; > unsigned int hwHistogramWeightGridsSize_; > @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) > /* \todo Add support for other revisions */ > switch (hwRevision) { > case RKISP1_V10: > - hwAeMeanMax_ = 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; > 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; > @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) > > LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision; > > + /* Cache the value to set it in configure. */ > + hwRevision_ = hwRevision; > + > camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel); > if (!camHelper_) { > LOG(IPARkISP1, Error) > @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > /* Clean context at configuration */ > context_ = {}; > > + /* Set the hardware revision for the algorithms. */ > + context_.configuration.hw.revision = hwRevision_; > + > /* > * When the AGC computes the new exposure values for a frame, it needs > * to know the limits for shutter speed and analogue gain.
Hi Laurent, On 23/11/2021 12:14, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Tue, Nov 23, 2021 at 10:14:51AM +0100, Jean-Michel Hautbois wrote: >> 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/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++- >> src/ipa/rkisp1/algorithms/agc.h | 2 ++ >> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++ >> src/ipa/rkisp1/ipa_context.h | 4 ++++ >> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++--- >> 5 files changed, 36 insertions(+), 4 deletions(-) >> >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp >> index 0433813a..e5358ca3 100644 >> --- a/src/ipa/rkisp1/algorithms/agc.cpp >> +++ b/src/ipa/rkisp1/algorithms/agc.cpp >> @@ -61,7 +61,7 @@ Agc::Agc() >> * \param[in] context The shared IPA context >> * \param[in] configInfo The IPA configuration data >> * >> - * \return 0 >> + * \return 0 on success or a negative error code otherwise >> */ >> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) >> { >> @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) >> context.frameContext.agc.gain = minAnalogueGain_; >> context.frameContext.agc.exposure = 10ms / lineDuration_; >> >> + switch (context.configuration.hw.revision) { >> + case RKISP1_V10: >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; >> + break; > > Blank line. Below too. > >> + case RKISP1_V12: >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; >> + break; >> + default: >> + LOG(RkISP1Agc, Error) >> + << "Hardware revision " << context.configuration.hw.revision >> + << " is currently not supported"; >> + return -ENODEV; > > We already fail at init() time if the hardware revision is unknown, so > I'd skip this. > > If you want to make it more full-proof, you could create an enum for the > hw.revision field, the compiler will then warn if some values are not > handled. > I hesitated on this one, and there is already the rkisp1_cif_isp_version enum and it sounded redundant. Or, I can change: struct { - uint32_t revision; + rkisp1_cif_isp_version revision; } hw; }; And make it use the enum. But init is called with an unsigned int. Should we change it already, move the revision into IPASettings and make it a rkisp1_cif_isp_version type too ? >> + } >> + >> return 0; >> } >> >> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h >> index ac95dea5..3ab3f347 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.cpp b/src/ipa/rkisp1/ipa_context.cpp >> index 16fc248f..cdb952be 100644 >> --- a/src/ipa/rkisp1/ipa_context.cpp >> +++ b/src/ipa/rkisp1/ipa_context.cpp >> @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 { >> * \brief Maximum analogue gain supported with the configured sensor >> */ >> >> +/** >> + * \var IPASessionConfiguration::hw >> + * \brief RkISP1 specific hardware information > > s/ specific/-specific/ > >> + * >> + * \var IPASessionConfiguration::hw.revision >> + * \brief RkISP1 revision reported > > * \brief Hardware revision of the ISP (RKISP1_V*) > >> + */ >> + >> /** >> * \var IPAFrameContext::agc >> * \brief Context for the Automatic Gain Control algorithm >> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h >> index 8e756fb1..316036c6 100644 >> --- a/src/ipa/rkisp1/ipa_context.h >> +++ b/src/ipa/rkisp1/ipa_context.h >> @@ -21,6 +21,10 @@ struct IPASessionConfiguration { >> double minAnalogueGain; >> double maxAnalogueGain; >> } agc; >> + >> + struct { >> + uint32_t revision; >> + } hw; >> }; >> >> struct IPAFrameContext { >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp >> index 59e868ff..a472d111 100644 >> --- a/src/ipa/rkisp1/rkisp1.cpp >> +++ b/src/ipa/rkisp1/rkisp1.cpp >> @@ -75,7 +75,7 @@ private: >> uint32_t maxGain_; >> >> /* revision-specific data */ >> - unsigned int hwAeMeanMax_; >> + unsigned int hwRevision_; >> unsigned int hwHistBinNMax_; >> unsigned int hwGammaOutMaxSamples_; >> unsigned int hwHistogramWeightGridsSize_; >> @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) >> /* \todo Add support for other revisions */ >> switch (hwRevision) { >> case RKISP1_V10: >> - hwAeMeanMax_ = 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; >> 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; >> @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) >> >> LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision; >> >> + /* Cache the value to set it in configure. */ >> + hwRevision_ = hwRevision; >> + >> camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel); >> if (!camHelper_) { >> LOG(IPARkISP1, Error) >> @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, >> /* Clean context at configuration */ >> context_ = {}; >> >> + /* Set the hardware revision for the algorithms. */ >> + context_.configuration.hw.revision = hwRevision_; >> + >> /* >> * When the AGC computes the new exposure values for a frame, it needs >> * to know the limits for shutter speed and analogue gain. >
Quoting Jean-Michel Hautbois (2021-11-23 13:53:08) > Hi Laurent, > > On 23/11/2021 12:14, Laurent Pinchart wrote: > > Hi Jean-Michel, > > > > Thank you for the patch. > > > > On Tue, Nov 23, 2021 at 10:14:51AM +0100, Jean-Michel Hautbois wrote: > >> 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/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++- > >> src/ipa/rkisp1/algorithms/agc.h | 2 ++ > >> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++ > >> src/ipa/rkisp1/ipa_context.h | 4 ++++ > >> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++--- > >> 5 files changed, 36 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > >> index 0433813a..e5358ca3 100644 > >> --- a/src/ipa/rkisp1/algorithms/agc.cpp > >> +++ b/src/ipa/rkisp1/algorithms/agc.cpp > >> @@ -61,7 +61,7 @@ Agc::Agc() > >> * \param[in] context The shared IPA context > >> * \param[in] configInfo The IPA configuration data > >> * > >> - * \return 0 > >> + * \return 0 on success or a negative error code otherwise > >> */ > >> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > >> { > >> @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > >> context.frameContext.agc.gain = minAnalogueGain_; > >> context.frameContext.agc.exposure = 10ms / lineDuration_; > >> > >> + switch (context.configuration.hw.revision) { > >> + case RKISP1_V10: > >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; > >> + break; > > > > Blank line. Below too. > > > >> + case RKISP1_V12: > >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; > >> + break; > >> + default: > >> + LOG(RkISP1Agc, Error) > >> + << "Hardware revision " << context.configuration.hw.revision > >> + << " is currently not supported"; > >> + return -ENODEV; > > > > We already fail at init() time if the hardware revision is unknown, so > > I'd skip this. > > > > If you want to make it more full-proof, you could create an enum for the > > hw.revision field, the compiler will then warn if some values are not > > handled. > > > > I hesitated on this one, and there is already the rkisp1_cif_isp_version > enum and it sounded redundant. Or, I can change: > struct { > - uint32_t revision; > + rkisp1_cif_isp_version revision; > } hw; > }; > > And make it use the enum. But init is called with an unsigned int. > Should we change it already, move the revision into IPASettings and make > it a rkisp1_cif_isp_version type too ? Isn't IPASettings 'libcamera generic' currently though? If so - we'll need to be able to extend this for pipeline specific settings... -- Kieran > > >> + } > >> + > >> return 0; > >> } > >> > >> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > >> index ac95dea5..3ab3f347 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.cpp b/src/ipa/rkisp1/ipa_context.cpp > >> index 16fc248f..cdb952be 100644 > >> --- a/src/ipa/rkisp1/ipa_context.cpp > >> +++ b/src/ipa/rkisp1/ipa_context.cpp > >> @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 { > >> * \brief Maximum analogue gain supported with the configured sensor > >> */ > >> > >> +/** > >> + * \var IPASessionConfiguration::hw > >> + * \brief RkISP1 specific hardware information > > > > s/ specific/-specific/ > > > >> + * > >> + * \var IPASessionConfiguration::hw.revision > >> + * \brief RkISP1 revision reported > > > > * \brief Hardware revision of the ISP (RKISP1_V*) > > > >> + */ > >> + > >> /** > >> * \var IPAFrameContext::agc > >> * \brief Context for the Automatic Gain Control algorithm > >> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > >> index 8e756fb1..316036c6 100644 > >> --- a/src/ipa/rkisp1/ipa_context.h > >> +++ b/src/ipa/rkisp1/ipa_context.h > >> @@ -21,6 +21,10 @@ struct IPASessionConfiguration { > >> double minAnalogueGain; > >> double maxAnalogueGain; > >> } agc; > >> + > >> + struct { > >> + uint32_t revision; > >> + } hw; > >> }; > >> > >> struct IPAFrameContext { > >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > >> index 59e868ff..a472d111 100644 > >> --- a/src/ipa/rkisp1/rkisp1.cpp > >> +++ b/src/ipa/rkisp1/rkisp1.cpp > >> @@ -75,7 +75,7 @@ private: > >> uint32_t maxGain_; > >> > >> /* revision-specific data */ > >> - unsigned int hwAeMeanMax_; > >> + unsigned int hwRevision_; > >> unsigned int hwHistBinNMax_; > >> unsigned int hwGammaOutMaxSamples_; > >> unsigned int hwHistogramWeightGridsSize_; > >> @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) > >> /* \todo Add support for other revisions */ > >> switch (hwRevision) { > >> case RKISP1_V10: > >> - hwAeMeanMax_ = 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; > >> 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; > >> @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) > >> > >> LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision; > >> > >> + /* Cache the value to set it in configure. */ > >> + hwRevision_ = hwRevision; > >> + > >> camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel); > >> if (!camHelper_) { > >> LOG(IPARkISP1, Error) > >> @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > >> /* Clean context at configuration */ > >> context_ = {}; > >> > >> + /* Set the hardware revision for the algorithms. */ > >> + context_.configuration.hw.revision = hwRevision_; > >> + > >> /* > >> * When the AGC computes the new exposure values for a frame, it needs > >> * to know the limits for shutter speed and analogue gain. > >
On Tue, Nov 23, 2021 at 02:04:02PM +0000, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2021-11-23 13:53:08) > > On 23/11/2021 12:14, Laurent Pinchart wrote: > > > On Tue, Nov 23, 2021 at 10:14:51AM +0100, Jean-Michel Hautbois wrote: > > >> 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/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++- > > >> src/ipa/rkisp1/algorithms/agc.h | 2 ++ > > >> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++ > > >> src/ipa/rkisp1/ipa_context.h | 4 ++++ > > >> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++--- > > >> 5 files changed, 36 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > >> index 0433813a..e5358ca3 100644 > > >> --- a/src/ipa/rkisp1/algorithms/agc.cpp > > >> +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > >> @@ -61,7 +61,7 @@ Agc::Agc() > > >> * \param[in] context The shared IPA context > > >> * \param[in] configInfo The IPA configuration data > > >> * > > >> - * \return 0 > > >> + * \return 0 on success or a negative error code otherwise > > >> */ > > >> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > >> { > > >> @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > >> context.frameContext.agc.gain = minAnalogueGain_; > > >> context.frameContext.agc.exposure = 10ms / lineDuration_; > > >> > > >> + switch (context.configuration.hw.revision) { > > >> + case RKISP1_V10: > > >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; > > >> + break; > > > > > > Blank line. Below too. > > > > > >> + case RKISP1_V12: > > >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; > > >> + break; > > >> + default: > > >> + LOG(RkISP1Agc, Error) > > >> + << "Hardware revision " << context.configuration.hw.revision > > >> + << " is currently not supported"; > > >> + return -ENODEV; > > > > > > We already fail at init() time if the hardware revision is unknown, so > > > I'd skip this. > > > > > > If you want to make it more full-proof, you could create an enum for the > > > hw.revision field, the compiler will then warn if some values are not > > > handled. > > > > I hesitated on this one, and there is already the rkisp1_cif_isp_version > > enum and it sounded redundant. Or, I can change: > > struct { > > - uint32_t revision; > > + rkisp1_cif_isp_version revision; > > } hw; > > }; Good point, we can use that. > > And make it use the enum. But init is called with an unsigned int. > > Should we change it already, move the revision into IPASettings and make > > it a rkisp1_cif_isp_version type too ? > > Isn't IPASettings 'libcamera generic' currently though? > > If so - we'll need to be able to extend this for pipeline specific > settings... It is generic, but a hardware revision field seems common enough to be included in a common structure, even if not used by all platforms. If we move the hw revision to IPASettings, then I'd make it a uint32_t there to match the hwrevision field from the MC API, and cast it to an enum in the RkISP1 IPA. > > >> + } > > >> + > > >> return 0; > > >> } > > >> > > >> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > > >> index ac95dea5..3ab3f347 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.cpp b/src/ipa/rkisp1/ipa_context.cpp > > >> index 16fc248f..cdb952be 100644 > > >> --- a/src/ipa/rkisp1/ipa_context.cpp > > >> +++ b/src/ipa/rkisp1/ipa_context.cpp > > >> @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 { > > >> * \brief Maximum analogue gain supported with the configured sensor > > >> */ > > >> > > >> +/** > > >> + * \var IPASessionConfiguration::hw > > >> + * \brief RkISP1 specific hardware information > > > > > > s/ specific/-specific/ > > > > > >> + * > > >> + * \var IPASessionConfiguration::hw.revision > > >> + * \brief RkISP1 revision reported > > > > > > * \brief Hardware revision of the ISP (RKISP1_V*) > > > > > >> + */ > > >> + > > >> /** > > >> * \var IPAFrameContext::agc > > >> * \brief Context for the Automatic Gain Control algorithm > > >> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > >> index 8e756fb1..316036c6 100644 > > >> --- a/src/ipa/rkisp1/ipa_context.h > > >> +++ b/src/ipa/rkisp1/ipa_context.h > > >> @@ -21,6 +21,10 @@ struct IPASessionConfiguration { > > >> double minAnalogueGain; > > >> double maxAnalogueGain; > > >> } agc; > > >> + > > >> + struct { > > >> + uint32_t revision; > > >> + } hw; > > >> }; > > >> > > >> struct IPAFrameContext { > > >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > >> index 59e868ff..a472d111 100644 > > >> --- a/src/ipa/rkisp1/rkisp1.cpp > > >> +++ b/src/ipa/rkisp1/rkisp1.cpp > > >> @@ -75,7 +75,7 @@ private: > > >> uint32_t maxGain_; > > >> > > >> /* revision-specific data */ > > >> - unsigned int hwAeMeanMax_; > > >> + unsigned int hwRevision_; > > >> unsigned int hwHistBinNMax_; > > >> unsigned int hwGammaOutMaxSamples_; > > >> unsigned int hwHistogramWeightGridsSize_; > > >> @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) > > >> /* \todo Add support for other revisions */ > > >> switch (hwRevision) { > > >> case RKISP1_V10: > > >> - hwAeMeanMax_ = 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; > > >> 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; > > >> @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) > > >> > > >> LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision; > > >> > > >> + /* Cache the value to set it in configure. */ > > >> + hwRevision_ = hwRevision; > > >> + > > >> camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel); > > >> if (!camHelper_) { > > >> LOG(IPARkISP1, Error) > > >> @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > > >> /* Clean context at configuration */ > > >> context_ = {}; > > >> > > >> + /* Set the hardware revision for the algorithms. */ > > >> + context_.configuration.hw.revision = hwRevision_; > > >> + > > >> /* > > >> * When the AGC computes the new exposure values for a frame, it needs > > >> * to know the limits for shutter speed and analogue gain.
On 23/11/2021 15:10, Laurent Pinchart wrote: > On Tue, Nov 23, 2021 at 02:04:02PM +0000, Kieran Bingham wrote: >> Quoting Jean-Michel Hautbois (2021-11-23 13:53:08) >>> On 23/11/2021 12:14, Laurent Pinchart wrote: >>>> On Tue, Nov 23, 2021 at 10:14:51AM +0100, Jean-Michel Hautbois wrote: >>>>> 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/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++- >>>>> src/ipa/rkisp1/algorithms/agc.h | 2 ++ >>>>> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++ >>>>> src/ipa/rkisp1/ipa_context.h | 4 ++++ >>>>> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++--- >>>>> 5 files changed, 36 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp >>>>> index 0433813a..e5358ca3 100644 >>>>> --- a/src/ipa/rkisp1/algorithms/agc.cpp >>>>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp >>>>> @@ -61,7 +61,7 @@ Agc::Agc() >>>>> * \param[in] context The shared IPA context >>>>> * \param[in] configInfo The IPA configuration data >>>>> * >>>>> - * \return 0 >>>>> + * \return 0 on success or a negative error code otherwise >>>>> */ >>>>> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) >>>>> { >>>>> @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) >>>>> context.frameContext.agc.gain = minAnalogueGain_; >>>>> context.frameContext.agc.exposure = 10ms / lineDuration_; >>>>> >>>>> + switch (context.configuration.hw.revision) { >>>>> + case RKISP1_V10: >>>>> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; >>>>> + break; >>>> >>>> Blank line. Below too. >>>> >>>>> + case RKISP1_V12: >>>>> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; >>>>> + break; >>>>> + default: >>>>> + LOG(RkISP1Agc, Error) >>>>> + << "Hardware revision " << context.configuration.hw.revision >>>>> + << " is currently not supported"; >>>>> + return -ENODEV; >>>> >>>> We already fail at init() time if the hardware revision is unknown, so >>>> I'd skip this. >>>> >>>> If you want to make it more full-proof, you could create an enum for the >>>> hw.revision field, the compiler will then warn if some values are not >>>> handled. >>> >>> I hesitated on this one, and there is already the rkisp1_cif_isp_version >>> enum and it sounded redundant. Or, I can change: >>> struct { >>> - uint32_t revision; >>> + rkisp1_cif_isp_version revision; >>> } hw; >>> }; > > Good point, we can use that. > >>> And make it use the enum. But init is called with an unsigned int. >>> Should we change it already, move the revision into IPASettings and make >>> it a rkisp1_cif_isp_version type too ? >> >> Isn't IPASettings 'libcamera generic' currently though? >> >> If so - we'll need to be able to extend this for pipeline specific >> settings... > > It is generic, but a hardware revision field seems common enough to be > included in a common structure, even if not used by all platforms. > > If we move the hw revision to IPASettings, then I'd make it a uint32_t > there to match the hwrevision field from the MC API, and cast it to an > enum in the RkISP1 IPA. > All right, then let's split the difference. I will use the existing enum in IPAContext and we'll add a revision field later in IPASettings. >>>>> + } >>>>> + >>>>> return 0; >>>>> } >>>>> >>>>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h >>>>> index ac95dea5..3ab3f347 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.cpp b/src/ipa/rkisp1/ipa_context.cpp >>>>> index 16fc248f..cdb952be 100644 >>>>> --- a/src/ipa/rkisp1/ipa_context.cpp >>>>> +++ b/src/ipa/rkisp1/ipa_context.cpp >>>>> @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 { >>>>> * \brief Maximum analogue gain supported with the configured sensor >>>>> */ >>>>> >>>>> +/** >>>>> + * \var IPASessionConfiguration::hw >>>>> + * \brief RkISP1 specific hardware information >>>> >>>> s/ specific/-specific/ >>>> >>>>> + * >>>>> + * \var IPASessionConfiguration::hw.revision >>>>> + * \brief RkISP1 revision reported >>>> >>>> * \brief Hardware revision of the ISP (RKISP1_V*) >>>> >>>>> + */ >>>>> + >>>>> /** >>>>> * \var IPAFrameContext::agc >>>>> * \brief Context for the Automatic Gain Control algorithm >>>>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h >>>>> index 8e756fb1..316036c6 100644 >>>>> --- a/src/ipa/rkisp1/ipa_context.h >>>>> +++ b/src/ipa/rkisp1/ipa_context.h >>>>> @@ -21,6 +21,10 @@ struct IPASessionConfiguration { >>>>> double minAnalogueGain; >>>>> double maxAnalogueGain; >>>>> } agc; >>>>> + >>>>> + struct { >>>>> + uint32_t revision; >>>>> + } hw; >>>>> }; >>>>> >>>>> struct IPAFrameContext { >>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp >>>>> index 59e868ff..a472d111 100644 >>>>> --- a/src/ipa/rkisp1/rkisp1.cpp >>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp >>>>> @@ -75,7 +75,7 @@ private: >>>>> uint32_t maxGain_; >>>>> >>>>> /* revision-specific data */ >>>>> - unsigned int hwAeMeanMax_; >>>>> + unsigned int hwRevision_; >>>>> unsigned int hwHistBinNMax_; >>>>> unsigned int hwGammaOutMaxSamples_; >>>>> unsigned int hwHistogramWeightGridsSize_; >>>>> @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) >>>>> /* \todo Add support for other revisions */ >>>>> switch (hwRevision) { >>>>> case RKISP1_V10: >>>>> - hwAeMeanMax_ = 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; >>>>> 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; >>>>> @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) >>>>> >>>>> LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision; >>>>> >>>>> + /* Cache the value to set it in configure. */ >>>>> + hwRevision_ = hwRevision; >>>>> + >>>>> camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel); >>>>> if (!camHelper_) { >>>>> LOG(IPARkISP1, Error) >>>>> @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, >>>>> /* Clean context at configuration */ >>>>> context_ = {}; >>>>> >>>>> + /* Set the hardware revision for the algorithms. */ >>>>> + context_.configuration.hw.revision = hwRevision_; >>>>> + >>>>> /* >>>>> * When the AGC computes the new exposure values for a frame, it needs >>>>> * to know the limits for shutter speed and analogue gain. >
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 0433813a..e5358ca3 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -61,7 +61,7 @@ Agc::Agc() * \param[in] context The shared IPA context * \param[in] configInfo The IPA configuration data * - * \return 0 + * \return 0 on success or a negative error code otherwise */ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) { @@ -79,6 +79,20 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) context.frameContext.agc.gain = minAnalogueGain_; context.frameContext.agc.exposure = 10ms / lineDuration_; + switch (context.configuration.hw.revision) { + case RKISP1_V10: + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; + break; + case RKISP1_V12: + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; + break; + default: + LOG(RkISP1Agc, Error) + << "Hardware revision " << context.configuration.hw.revision + << " is currently not supported"; + return -ENODEV; + } + return 0; } diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h index ac95dea5..3ab3f347 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.cpp b/src/ipa/rkisp1/ipa_context.cpp index 16fc248f..cdb952be 100644 --- a/src/ipa/rkisp1/ipa_context.cpp +++ b/src/ipa/rkisp1/ipa_context.cpp @@ -72,6 +72,14 @@ namespace libcamera::ipa::rkisp1 { * \brief Maximum analogue gain supported with the configured sensor */ +/** + * \var IPASessionConfiguration::hw + * \brief RkISP1 specific hardware information + * + * \var IPASessionConfiguration::hw.revision + * \brief RkISP1 revision reported + */ + /** * \var IPAFrameContext::agc * \brief Context for the Automatic Gain Control algorithm diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index 8e756fb1..316036c6 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -21,6 +21,10 @@ struct IPASessionConfiguration { double minAnalogueGain; double maxAnalogueGain; } agc; + + struct { + uint32_t revision; + } hw; }; struct IPAFrameContext { diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 59e868ff..a472d111 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -75,7 +75,7 @@ private: uint32_t maxGain_; /* revision-specific data */ - unsigned int hwAeMeanMax_; + unsigned int hwRevision_; unsigned int hwHistBinNMax_; unsigned int hwGammaOutMaxSamples_; unsigned int hwHistogramWeightGridsSize_; @@ -97,13 +97,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) /* \todo Add support for other revisions */ switch (hwRevision) { case RKISP1_V10: - hwAeMeanMax_ = 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; 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; @@ -117,6 +115,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision) LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision; + /* Cache the value to set it in configure. */ + hwRevision_ = hwRevision; + camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel); if (!camHelper_) { LOG(IPARkISP1, Error) @@ -184,6 +185,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, /* Clean context at configuration */ context_ = {}; + /* Set the hardware revision for the algorithms. */ + context_.configuration.hw.revision = hwRevision_; + /* * When the AGC computes the new exposure values for a frame, it needs * to know the limits for shutter speed and analogue gain.
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/rkisp1/algorithms/agc.cpp | 16 +++++++++++++++- src/ipa/rkisp1/algorithms/agc.h | 2 ++ src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++ src/ipa/rkisp1/ipa_context.h | 4 ++++ src/ipa/rkisp1/rkisp1.cpp | 10 +++++++--- 5 files changed, 36 insertions(+), 4 deletions(-)