| Message ID | 20211123150423.125524-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 04:04:23PM +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> > --- > v3: - Use the rkisp1 enum for the HW revision > > --- > src/ipa/rkisp1/algorithms/agc.cpp | 12 +++++++++++- > src/ipa/rkisp1/algorithms/agc.h | 2 ++ > src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++ > src/ipa/rkisp1/ipa_context.h | 6 ++++++ > src/ipa/rkisp1/rkisp1.cpp | 10 +++++++--- > 5 files changed, 34 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 0433813a..16966b16 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 You can drop this. > */ > int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > { > @@ -79,6 +79,16 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > context.frameContext.agc.gain = minAnalogueGain_; > context.frameContext.agc.exposure = 10ms / lineDuration_; > > + /* > + * According to the RkISP1 documentation: > + * - versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries, I would have gone for < V12 here and in the condition below, given that the macro is named RKISP1_CIF_ISP_AE_MEAN_MAX_V12. > + * - versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries. > + */ > + if (context.configuration.hw.revision <= RKISP1_V11) > + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; > + else > + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; > + > 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..faa002d1 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 Hardware revision of the ISP (RKISP1_V*) I know I've proposed adding (RKISP1_V*), but that was before the enum. You can drop it now if you prefer. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + */ > + > /** > * \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..e526a0d3 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -8,6 +8,8 @@ > #ifndef __LIBCAMERA_RKISP1_IPA_CONTEXT_H__ > #define __LIBCAMERA_RKISP1_IPA_CONTEXT_H__ > > +#include <linux/rkisp1-config.h> > + > #include <libcamera/base/utils.h> > > namespace libcamera { > @@ -21,6 +23,10 @@ struct IPASessionConfiguration { > double minAnalogueGain; > double maxAnalogueGain; > } agc; > + > + struct { > + rkisp1_cif_isp_version revision; > + } hw; > }; > > struct IPAFrameContext { > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 59e868ff..99ccd596 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_; > + rkisp1_cif_isp_version 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_ = static_cast<rkisp1_cif_isp_version>(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 16:13, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Tue, Nov 23, 2021 at 04:04:23PM +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> >> --- >> v3: - Use the rkisp1 enum for the HW revision >> >> --- >> src/ipa/rkisp1/algorithms/agc.cpp | 12 +++++++++++- >> src/ipa/rkisp1/algorithms/agc.h | 2 ++ >> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++ >> src/ipa/rkisp1/ipa_context.h | 6 ++++++ >> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++--- >> 5 files changed, 34 insertions(+), 4 deletions(-) >> >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp >> index 0433813a..16966b16 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 > > You can drop this. > >> */ >> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) >> { >> @@ -79,6 +79,16 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) >> context.frameContext.agc.gain = minAnalogueGain_; >> context.frameContext.agc.exposure = 10ms / lineDuration_; >> >> + /* >> + * According to the RkISP1 documentation: >> + * - versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries, > > I would have gone for < V12 here and in the condition below, given that > the macro is named RKISP1_CIF_ISP_AE_MEAN_MAX_V12. > >> + * - versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries. >> + */ >> + if (context.configuration.hw.revision <= RKISP1_V11) >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; >> + else >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; >> + >> 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..faa002d1 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 Hardware revision of the ISP (RKISP1_V*) > > I know I've proposed adding (RKISP1_V*), but that was before the enum. > You can drop it now if you prefer. As we don't have its type here, it can help, I suppose, so I will keep it like this if you don't mind :-). > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + */ >> + >> /** >> * \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..e526a0d3 100644 >> --- a/src/ipa/rkisp1/ipa_context.h >> +++ b/src/ipa/rkisp1/ipa_context.h >> @@ -8,6 +8,8 @@ >> #ifndef __LIBCAMERA_RKISP1_IPA_CONTEXT_H__ >> #define __LIBCAMERA_RKISP1_IPA_CONTEXT_H__ >> >> +#include <linux/rkisp1-config.h> >> + >> #include <libcamera/base/utils.h> >> >> namespace libcamera { >> @@ -21,6 +23,10 @@ struct IPASessionConfiguration { >> double minAnalogueGain; >> double maxAnalogueGain; >> } agc; >> + >> + struct { >> + rkisp1_cif_isp_version revision; >> + } hw; >> }; >> >> struct IPAFrameContext { >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp >> index 59e868ff..99ccd596 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_; >> + rkisp1_cif_isp_version 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_ = static_cast<rkisp1_cif_isp_version>(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 Jean-Michel, On Tue, Nov 23, 2021 at 04:20:41PM +0100, Jean-Michel Hautbois wrote: > On 23/11/2021 16:13, Laurent Pinchart wrote: > > On Tue, Nov 23, 2021 at 04:04:23PM +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> > >> --- > >> v3: - Use the rkisp1 enum for the HW revision > >> > >> --- > >> src/ipa/rkisp1/algorithms/agc.cpp | 12 +++++++++++- > >> src/ipa/rkisp1/algorithms/agc.h | 2 ++ > >> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++ > >> src/ipa/rkisp1/ipa_context.h | 6 ++++++ > >> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++--- > >> 5 files changed, 34 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > >> index 0433813a..16966b16 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 > > > > You can drop this. > > > >> */ > >> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > >> { > >> @@ -79,6 +79,16 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > >> context.frameContext.agc.gain = minAnalogueGain_; > >> context.frameContext.agc.exposure = 10ms / lineDuration_; > >> > >> + /* > >> + * According to the RkISP1 documentation: > >> + * - versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries, > > > > I would have gone for < V12 here and in the condition below, given that > > the macro is named RKISP1_CIF_ISP_AE_MEAN_MAX_V12. > > > >> + * - versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries. > >> + */ > >> + if (context.configuration.hw.revision <= RKISP1_V11) > >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; > >> + else > >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; > >> + > >> 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..faa002d1 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 Hardware revision of the ISP (RKISP1_V*) > > > > I know I've proposed adding (RKISP1_V*), but that was before the enum. > > You can drop it now if you prefer. > > As we don't have its type here, it can help, I suppose, so I will keep > it like this if you don't mind :-). Up to you, but we do have the type, this documents the hw.revision field. Doxygen will show the type in the HTML output. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> + */ > >> + > >> /** > >> * \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..e526a0d3 100644 > >> --- a/src/ipa/rkisp1/ipa_context.h > >> +++ b/src/ipa/rkisp1/ipa_context.h > >> @@ -8,6 +8,8 @@ > >> #ifndef __LIBCAMERA_RKISP1_IPA_CONTEXT_H__ > >> #define __LIBCAMERA_RKISP1_IPA_CONTEXT_H__ > >> > >> +#include <linux/rkisp1-config.h> > >> + > >> #include <libcamera/base/utils.h> > >> > >> namespace libcamera { > >> @@ -21,6 +23,10 @@ struct IPASessionConfiguration { > >> double minAnalogueGain; > >> double maxAnalogueGain; > >> } agc; > >> + > >> + struct { > >> + rkisp1_cif_isp_version revision; > >> + } hw; > >> }; > >> > >> struct IPAFrameContext { > >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > >> index 59e868ff..99ccd596 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_; > >> + rkisp1_cif_isp_version 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_ = static_cast<rkisp1_cif_isp_version>(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 Laurent Pinchart (2021-11-23 15:27:32) > Hi Jean-Michel, > > On Tue, Nov 23, 2021 at 04:20:41PM +0100, Jean-Michel Hautbois wrote: > > On 23/11/2021 16:13, Laurent Pinchart wrote: > > > On Tue, Nov 23, 2021 at 04:04:23PM +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> > > >> --- > > >> v3: - Use the rkisp1 enum for the HW revision > > >> > > >> --- > > >> src/ipa/rkisp1/algorithms/agc.cpp | 12 +++++++++++- > > >> src/ipa/rkisp1/algorithms/agc.h | 2 ++ > > >> src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++ > > >> src/ipa/rkisp1/ipa_context.h | 6 ++++++ > > >> src/ipa/rkisp1/rkisp1.cpp | 10 +++++++--- > > >> 5 files changed, 34 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > >> index 0433813a..16966b16 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 > > > > > > You can drop this. > > > > > >> */ > > >> int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > >> { > > >> @@ -79,6 +79,16 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > >> context.frameContext.agc.gain = minAnalogueGain_; > > >> context.frameContext.agc.exposure = 10ms / lineDuration_; > > >> > > >> + /* > > >> + * According to the RkISP1 documentation: > > >> + * - versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries, > > > > > > I would have gone for < V12 here and in the condition below, given that > > > the macro is named RKISP1_CIF_ISP_AE_MEAN_MAX_V12. Seconded here. It looks odd with the extra version mention. Either way though, it's correct, and I'm glad we're pushing these algorithm specific configuration details down into the algos. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > >> + * - versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries. > > >> + */ > > >> + if (context.configuration.hw.revision <= RKISP1_V11) > > >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; > > >> + else > > >> + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; > > >> + > > >> 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..faa002d1 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 Hardware revision of the ISP (RKISP1_V*) > > > > > > I know I've proposed adding (RKISP1_V*), but that was before the enum. > > > You can drop it now if you prefer. > > > > As we don't have its type here, it can help, I suppose, so I will keep > > it like this if you don't mind :-). > > Up to you, but we do have the type, this documents the hw.revision > field. Doxygen will show the type in the HTML output. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > >> + */ > > >> + > > >> /** > > >> * \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..e526a0d3 100644 > > >> --- a/src/ipa/rkisp1/ipa_context.h > > >> +++ b/src/ipa/rkisp1/ipa_context.h > > >> @@ -8,6 +8,8 @@ > > >> #ifndef __LIBCAMERA_RKISP1_IPA_CONTEXT_H__ > > >> #define __LIBCAMERA_RKISP1_IPA_CONTEXT_H__ > > >> > > >> +#include <linux/rkisp1-config.h> > > >> + > > >> #include <libcamera/base/utils.h> > > >> > > >> namespace libcamera { > > >> @@ -21,6 +23,10 @@ struct IPASessionConfiguration { > > >> double minAnalogueGain; > > >> double maxAnalogueGain; > > >> } agc; > > >> + > > >> + struct { > > >> + rkisp1_cif_isp_version revision; > > >> + } hw; > > >> }; > > >> > > >> struct IPAFrameContext { > > >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > >> index 59e868ff..99ccd596 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_; > > >> + rkisp1_cif_isp_version 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_ = static_cast<rkisp1_cif_isp_version>(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. > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 0433813a..16966b16 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,16 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) context.frameContext.agc.gain = minAnalogueGain_; context.frameContext.agc.exposure = 10ms / lineDuration_; + /* + * According to the RkISP1 documentation: + * - versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries, + * - versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries. + */ + if (context.configuration.hw.revision <= RKISP1_V11) + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; + else + numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; + 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..faa002d1 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 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..e526a0d3 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -8,6 +8,8 @@ #ifndef __LIBCAMERA_RKISP1_IPA_CONTEXT_H__ #define __LIBCAMERA_RKISP1_IPA_CONTEXT_H__ +#include <linux/rkisp1-config.h> + #include <libcamera/base/utils.h> namespace libcamera { @@ -21,6 +23,10 @@ struct IPASessionConfiguration { double minAnalogueGain; double maxAnalogueGain; } agc; + + struct { + rkisp1_cif_isp_version revision; + } hw; }; struct IPAFrameContext { diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 59e868ff..99ccd596 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_; + rkisp1_cif_isp_version 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_ = static_cast<rkisp1_cif_isp_version>(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> --- v3: - Use the rkisp1 enum for the HW revision --- src/ipa/rkisp1/algorithms/agc.cpp | 12 +++++++++++- src/ipa/rkisp1/algorithms/agc.h | 2 ++ src/ipa/rkisp1/ipa_context.cpp | 8 ++++++++ src/ipa/rkisp1/ipa_context.h | 6 ++++++ src/ipa/rkisp1/rkisp1.cpp | 10 +++++++--- 5 files changed, 34 insertions(+), 4 deletions(-)