| Message ID | 20240218164908.15921-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 528dc21b09cc4c0e202c09677bc742db5a5e484d |
| Headers | show |
| Series |
|
| Related | show |
On Sun, Feb 18, 2024 at 06:49:05PM +0200, Laurent Pinchart wrote: > Versions of the ISP differ in the processing blocks they include, as > well as in the implementation of some of those blocks. In particular, > they have different numbers of histogram bins oe AE statistics cells. > The algorithms take these differences into account by checking the ISP > version reported by the driver. > > These checks are currently scattered in multiple places. Centralize them > in the IPARkISP1::init() function, and store the version-dependent > hardware parameters in the IPA context, accessible by all algorithms. > > While at it, drop the IPASessionConfiguration::hw member that stores the > revision number, unused by the algorithms. It can be added back laer to > the IPAHwSettings structure if needed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 27 +++++++++-------------- > src/ipa/rkisp1/algorithms/agc.h | 3 --- > src/ipa/rkisp1/ipa_context.cpp | 30 +++++++++++++++++++------- > src/ipa/rkisp1/ipa_context.h | 12 +++++++---- > src/ipa/rkisp1/rkisp1.cpp | 36 +++++++++++++++---------------- > 5 files changed, 57 insertions(+), 51 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index f83a9ba1686a..da705b14754c 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -59,7 +59,7 @@ static constexpr double kEvGainTarget = 0.5; > static constexpr double kRelativeLuminanceTarget = 0.4; > > Agc::Agc() > - : frameCount_(0), numCells_(0), numHistBins_(0), filteredExposure_(0s) > + : frameCount_(0), filteredExposure_(0s) > { > supportsRaw_ = true; > } > @@ -81,19 +81,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure; > context.activeState.agc.autoEnabled = !context.configuration.raw; > > - /* > - * According to the RkISP1 documentation: > - * - versions < V12 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_V12) { > - numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; > - numHistBins_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10; > - } else { > - numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; > - numHistBins_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12; > - } > - > /* > * Define the measurement window for AGC as a centered rectangle > * covering 3/4 of the image width and height. > @@ -186,7 +173,10 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > /* Produce the luminance histogram. */ > params->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM; > /* Set an average weighted histogram. */ > - Span<uint8_t> weights{ params->meas.hst_config.hist_weight, numHistBins_ }; > + Span<uint8_t> weights{ > + params->meas.hst_config.hist_weight, > + context.hw->numHistogramBins > + }; > std::fill(weights.begin(), weights.end(), 1); > /* Step size can't be less than 3. */ > params->meas.hst_config.histogram_predivider = 4; > @@ -414,8 +404,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > const rkisp1_cif_isp_stat *params = &stats->params; > ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP); > > - Span<const uint8_t> ae{ params->ae.exp_mean, numCells_ }; > - Span<const uint32_t> hist{ params->hist.hist_bins, numHistBins_ }; > + Span<const uint8_t> ae{ params->ae.exp_mean, context.hw->numAeCells }; > + Span<const uint32_t> hist{ > + params->hist.hist_bins, > + context.hw->numHistogramBins > + }; > > double iqMean = measureBrightness(hist); > double iqMeanGain = kEvGainTarget * hist.size() / iqMean; > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > index ce8594f393ab..fb82a33fc1bf 100644 > --- a/src/ipa/rkisp1/algorithms/agc.h > +++ b/src/ipa/rkisp1/algorithms/agc.h > @@ -50,9 +50,6 @@ private: > > uint64_t frameCount_; > > - uint32_t numCells_; > - uint32_t numHistBins_; > - > utils::Duration filteredExposure_; > }; > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > index 9bbf368432fa..070834fa682d 100644 > --- a/src/ipa/rkisp1/ipa_context.cpp > +++ b/src/ipa/rkisp1/ipa_context.cpp > @@ -14,6 +14,25 @@ > > namespace libcamera::ipa::rkisp1 { > > +/** > + * \struct IPAHwSettings > + * \brief RkISP1 version-specific hardware parameters > + */ > + > +/** > + * \var IPAHwSettings::numAeCells > + * \brief Number of cells in the AE exposure means grid > + * > + * \var IPAHwSettings::numHistogramBins > + * \brief Number of bins in the histogram > + * > + * \var IPAHwSettings::numHistogramWeights > + * \brief Number of weights in the histogram grid > + * > + * \var IPAHwSettings::numGammaOutSamples > + * \brief Number of samples in the gamma out table > + */ > + > /** > * \struct IPASessionConfiguration > * \brief Session configuration for the IPA module > @@ -32,14 +51,6 @@ namespace libcamera::ipa::rkisp1 { > * \brief AGC measure window > */ > > -/** > - * \var IPASessionConfiguration::hw > - * \brief RkISP1-specific hardware information > - * > - * \var IPASessionConfiguration::hw.revision > - * \brief Hardware revision of the ISP > - */ > - > /** > * \var IPASessionConfiguration::awb > * \brief AWB parameters configuration of the IPA > @@ -337,6 +348,9 @@ namespace libcamera::ipa::rkisp1 { > * \struct IPAContext > * \brief Global IPA context data shared between all algorithms > * > + * \var IPAContext::hw > + * \brief RkISP1 version-specific hardware parameters > + * > * \var IPAContext::configuration > * \brief The IPA session configuration, immutable during the session > * > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index b9b2065328d6..10d8f38cad5d 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -20,6 +20,13 @@ namespace libcamera { > > namespace ipa::rkisp1 { > > +struct IPAHwSettings { > + unsigned int numAeCells; > + unsigned int numHistogramBins; > + unsigned int numHistogramWeights; > + unsigned int numGammaOutSamples; > +}; > + > struct IPASessionConfiguration { > struct { > struct rkisp1_cif_isp_window measureWindow; > @@ -45,10 +52,6 @@ struct IPASessionConfiguration { > Size size; > } sensor; > > - struct { > - rkisp1_cif_isp_version revision; > - } hw; > - > bool raw; > }; > > @@ -143,6 +146,7 @@ struct IPAFrameContext : public FrameContext { > }; > > struct IPAContext { > + const IPAHwSettings *hw; > IPASessionConfiguration configuration; > IPAActiveState activeState; > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 6544c925ba25..44401af8910d 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -81,12 +81,6 @@ private: > > ControlInfoMap sensorControls_; > > - /* revision-specific data */ > - rkisp1_cif_isp_version hwRevision_; > - unsigned int hwHistBinNMax_; > - unsigned int hwGammaOutMaxSamples_; > - unsigned int hwHistogramWeightGridsSize_; > - > /* Interface to the Camera Helper */ > std::unique_ptr<CameraSensorHelper> camHelper_; > > @@ -96,6 +90,20 @@ private: > > namespace { > > +const IPAHwSettings ipaHwSettingsV10{ > + RKISP1_CIF_ISP_AE_MEAN_MAX_V10, > + RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10, > + RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10, > + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10, > +}; > + > +const IPAHwSettings ipaHwSettingsV12{ > + RKISP1_CIF_ISP_AE_MEAN_MAX_V12, > + RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12, > + RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12, > + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12, > +}; > + > /* List of controls handled by the RkISP1 IPA */ > const ControlInfoMap::Map rkisp1Controls{ > { &controls::AeEnable, ControlInfo(false, true) }, > @@ -111,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{ > } /* namespace */ > > IPARkISP1::IPARkISP1() > - : context_({ {}, {}, { kMaxFrameContexts } }) > + : context_({ {}, {}, {}, { kMaxFrameContexts } }) > { > } > > @@ -128,14 +136,10 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > /* \todo Add support for other revisions */ > switch (hwRevision) { > case RKISP1_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; > + context_.hw = &ipaHwSettingsV10; > break; > case RKISP1_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; > + context_.hw = &ipaHwSettingsV12; > break; > default: > LOG(IPARkISP1, Error) > @@ -146,9 +150,6 @@ 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_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); > if (!camHelper_) { > LOG(IPARkISP1, Error) > @@ -232,9 +233,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > context_.activeState = {}; > context_.frameContexts.clear(); > > - /* Set the hardware revision for the algorithms. */ > - context_.configuration.hw.revision = hwRevision_; > - > const IPACameraSensorInfo &info = ipaConfig.sensorInfo; > const ControlInfo vBlank = sensorControls_.find(V4L2_CID_VBLANK)->second; > context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>(); > -- > Regards, > > Laurent Pinchart >
Am 18.02.24 um 17:49 schrieb Laurent Pinchart: > Versions of the ISP differ in the processing blocks they include, as > well as in the implementation of some of those blocks. In particular, > they have different numbers of histogram bins oe AE statistics cells. > The algorithms take these differences into account by checking the ISP > version reported by the driver. > > These checks are currently scattered in multiple places. Centralize them > in the IPARkISP1::init() function, and store the version-dependent > hardware parameters in the IPA context, accessible by all algorithms. > > While at it, drop the IPASessionConfiguration::hw member that stores the > revision number, unused by the algorithms. It can be added back laer to > the IPAHwSettings structure if needed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 27 +++++++++-------------- > src/ipa/rkisp1/algorithms/agc.h | 3 --- > src/ipa/rkisp1/ipa_context.cpp | 30 +++++++++++++++++++------- > src/ipa/rkisp1/ipa_context.h | 12 +++++++---- > src/ipa/rkisp1/rkisp1.cpp | 36 +++++++++++++++---------------- > 5 files changed, 57 insertions(+), 51 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index f83a9ba1686a..da705b14754c 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -59,7 +59,7 @@ static constexpr double kEvGainTarget = 0.5; > static constexpr double kRelativeLuminanceTarget = 0.4; > > Agc::Agc() > - : frameCount_(0), numCells_(0), numHistBins_(0), filteredExposure_(0s) > + : frameCount_(0), filteredExposure_(0s) > { > supportsRaw_ = true; > } > @@ -81,19 +81,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure; > context.activeState.agc.autoEnabled = !context.configuration.raw; > > - /* > - * According to the RkISP1 documentation: > - * - versions < V12 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_V12) { > - numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; > - numHistBins_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10; > - } else { > - numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; > - numHistBins_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12; > - } > - > /* > * Define the measurement window for AGC as a centered rectangle > * covering 3/4 of the image width and height. > @@ -186,7 +173,10 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, > /* Produce the luminance histogram. */ > params->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM; > /* Set an average weighted histogram. */ > - Span<uint8_t> weights{ params->meas.hst_config.hist_weight, numHistBins_ }; > + Span<uint8_t> weights{ > + params->meas.hst_config.hist_weight, > + context.hw->numHistogramBins > + }; > std::fill(weights.begin(), weights.end(), 1); > /* Step size can't be less than 3. */ > params->meas.hst_config.histogram_predivider = 4; > @@ -414,8 +404,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > const rkisp1_cif_isp_stat *params = &stats->params; > ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP); > > - Span<const uint8_t> ae{ params->ae.exp_mean, numCells_ }; > - Span<const uint32_t> hist{ params->hist.hist_bins, numHistBins_ }; > + Span<const uint8_t> ae{ params->ae.exp_mean, context.hw->numAeCells }; > + Span<const uint32_t> hist{ > + params->hist.hist_bins, > + context.hw->numHistogramBins > + }; > > double iqMean = measureBrightness(hist); > double iqMeanGain = kEvGainTarget * hist.size() / iqMean; > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > index ce8594f393ab..fb82a33fc1bf 100644 > --- a/src/ipa/rkisp1/algorithms/agc.h > +++ b/src/ipa/rkisp1/algorithms/agc.h > @@ -50,9 +50,6 @@ private: > > uint64_t frameCount_; > > - uint32_t numCells_; > - uint32_t numHistBins_; > - > utils::Duration filteredExposure_; > }; > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp > index 9bbf368432fa..070834fa682d 100644 > --- a/src/ipa/rkisp1/ipa_context.cpp > +++ b/src/ipa/rkisp1/ipa_context.cpp > @@ -14,6 +14,25 @@ > > namespace libcamera::ipa::rkisp1 { > > +/** > + * \struct IPAHwSettings > + * \brief RkISP1 version-specific hardware parameters > + */ > + > +/** > + * \var IPAHwSettings::numAeCells > + * \brief Number of cells in the AE exposure means grid > + * > + * \var IPAHwSettings::numHistogramBins > + * \brief Number of bins in the histogram > + * > + * \var IPAHwSettings::numHistogramWeights > + * \brief Number of weights in the histogram grid > + * > + * \var IPAHwSettings::numGammaOutSamples > + * \brief Number of samples in the gamma out table > + */ > + > /** > * \struct IPASessionConfiguration > * \brief Session configuration for the IPA module > @@ -32,14 +51,6 @@ namespace libcamera::ipa::rkisp1 { > * \brief AGC measure window > */ > > -/** > - * \var IPASessionConfiguration::hw > - * \brief RkISP1-specific hardware information > - * > - * \var IPASessionConfiguration::hw.revision > - * \brief Hardware revision of the ISP > - */ > - > /** > * \var IPASessionConfiguration::awb > * \brief AWB parameters configuration of the IPA > @@ -337,6 +348,9 @@ namespace libcamera::ipa::rkisp1 { > * \struct IPAContext > * \brief Global IPA context data shared between all algorithms > * > + * \var IPAContext::hw > + * \brief RkISP1 version-specific hardware parameters > + * > * \var IPAContext::configuration > * \brief The IPA session configuration, immutable during the session > * > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index b9b2065328d6..10d8f38cad5d 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -20,6 +20,13 @@ namespace libcamera { > > namespace ipa::rkisp1 { > > +struct IPAHwSettings { > + unsigned int numAeCells; > + unsigned int numHistogramBins; > + unsigned int numHistogramWeights; > + unsigned int numGammaOutSamples; > +}; > + > struct IPASessionConfiguration { > struct { > struct rkisp1_cif_isp_window measureWindow; > @@ -45,10 +52,6 @@ struct IPASessionConfiguration { > Size size; > } sensor; > > - struct { > - rkisp1_cif_isp_version revision; > - } hw; > - > bool raw; > }; > > @@ -143,6 +146,7 @@ struct IPAFrameContext : public FrameContext { > }; > > struct IPAContext { > + const IPAHwSettings *hw; > IPASessionConfiguration configuration; > IPAActiveState activeState; > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 6544c925ba25..44401af8910d 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -81,12 +81,6 @@ private: > > ControlInfoMap sensorControls_; > > - /* revision-specific data */ > - rkisp1_cif_isp_version hwRevision_; > - unsigned int hwHistBinNMax_; > - unsigned int hwGammaOutMaxSamples_; > - unsigned int hwHistogramWeightGridsSize_; > - > /* Interface to the Camera Helper */ > std::unique_ptr<CameraSensorHelper> camHelper_; > > @@ -96,6 +90,20 @@ private: > > namespace { > > +const IPAHwSettings ipaHwSettingsV10{ > + RKISP1_CIF_ISP_AE_MEAN_MAX_V10, > + RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10, > + RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10, > + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10, > +}; > + > +const IPAHwSettings ipaHwSettingsV12{ > + RKISP1_CIF_ISP_AE_MEAN_MAX_V12, > + RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12, > + RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12, > + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12, > +}; > + > /* List of controls handled by the RkISP1 IPA */ > const ControlInfoMap::Map rkisp1Controls{ > { &controls::AeEnable, ControlInfo(false, true) }, > @@ -111,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{ > } /* namespace */ > > IPARkISP1::IPARkISP1() > - : context_({ {}, {}, { kMaxFrameContexts } }) > + : context_({ {}, {}, {}, { kMaxFrameContexts } }) > { > } > > @@ -128,14 +136,10 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > /* \todo Add support for other revisions */ > switch (hwRevision) { > case RKISP1_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; > + context_.hw = &ipaHwSettingsV10; > break; > case RKISP1_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; > + context_.hw = &ipaHwSettingsV12; > break; > default: > LOG(IPARkISP1, Error) > @@ -146,9 +150,6 @@ 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_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); > if (!camHelper_) { > LOG(IPARkISP1, Error) > @@ -232,9 +233,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > context_.activeState = {}; > context_.frameContexts.clear(); > > - /* Set the hardware revision for the algorithms. */ > - context_.configuration.hw.revision = hwRevision_; > - > const IPACameraSensorInfo &info = ipaConfig.sensorInfo; > const ControlInfo vBlank = sensorControls_.find(V4L2_CID_VBLANK)->second; > context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>();
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index f83a9ba1686a..da705b14754c 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -59,7 +59,7 @@ static constexpr double kEvGainTarget = 0.5; static constexpr double kRelativeLuminanceTarget = 0.4; Agc::Agc() - : frameCount_(0), numCells_(0), numHistBins_(0), filteredExposure_(0s) + : frameCount_(0), filteredExposure_(0s) { supportsRaw_ = true; } @@ -81,19 +81,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure; context.activeState.agc.autoEnabled = !context.configuration.raw; - /* - * According to the RkISP1 documentation: - * - versions < V12 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_V12) { - numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10; - numHistBins_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10; - } else { - numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12; - numHistBins_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12; - } - /* * Define the measurement window for AGC as a centered rectangle * covering 3/4 of the image width and height. @@ -186,7 +173,10 @@ void Agc::prepare(IPAContext &context, const uint32_t frame, /* Produce the luminance histogram. */ params->meas.hst_config.mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM; /* Set an average weighted histogram. */ - Span<uint8_t> weights{ params->meas.hst_config.hist_weight, numHistBins_ }; + Span<uint8_t> weights{ + params->meas.hst_config.hist_weight, + context.hw->numHistogramBins + }; std::fill(weights.begin(), weights.end(), 1); /* Step size can't be less than 3. */ params->meas.hst_config.histogram_predivider = 4; @@ -414,8 +404,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, const rkisp1_cif_isp_stat *params = &stats->params; ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP); - Span<const uint8_t> ae{ params->ae.exp_mean, numCells_ }; - Span<const uint32_t> hist{ params->hist.hist_bins, numHistBins_ }; + Span<const uint8_t> ae{ params->ae.exp_mean, context.hw->numAeCells }; + Span<const uint32_t> hist{ + params->hist.hist_bins, + context.hw->numHistogramBins + }; double iqMean = measureBrightness(hist); double iqMeanGain = kEvGainTarget * hist.size() / iqMean; diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h index ce8594f393ab..fb82a33fc1bf 100644 --- a/src/ipa/rkisp1/algorithms/agc.h +++ b/src/ipa/rkisp1/algorithms/agc.h @@ -50,9 +50,6 @@ private: uint64_t frameCount_; - uint32_t numCells_; - uint32_t numHistBins_; - utils::Duration filteredExposure_; }; diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp index 9bbf368432fa..070834fa682d 100644 --- a/src/ipa/rkisp1/ipa_context.cpp +++ b/src/ipa/rkisp1/ipa_context.cpp @@ -14,6 +14,25 @@ namespace libcamera::ipa::rkisp1 { +/** + * \struct IPAHwSettings + * \brief RkISP1 version-specific hardware parameters + */ + +/** + * \var IPAHwSettings::numAeCells + * \brief Number of cells in the AE exposure means grid + * + * \var IPAHwSettings::numHistogramBins + * \brief Number of bins in the histogram + * + * \var IPAHwSettings::numHistogramWeights + * \brief Number of weights in the histogram grid + * + * \var IPAHwSettings::numGammaOutSamples + * \brief Number of samples in the gamma out table + */ + /** * \struct IPASessionConfiguration * \brief Session configuration for the IPA module @@ -32,14 +51,6 @@ namespace libcamera::ipa::rkisp1 { * \brief AGC measure window */ -/** - * \var IPASessionConfiguration::hw - * \brief RkISP1-specific hardware information - * - * \var IPASessionConfiguration::hw.revision - * \brief Hardware revision of the ISP - */ - /** * \var IPASessionConfiguration::awb * \brief AWB parameters configuration of the IPA @@ -337,6 +348,9 @@ namespace libcamera::ipa::rkisp1 { * \struct IPAContext * \brief Global IPA context data shared between all algorithms * + * \var IPAContext::hw + * \brief RkISP1 version-specific hardware parameters + * * \var IPAContext::configuration * \brief The IPA session configuration, immutable during the session * diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index b9b2065328d6..10d8f38cad5d 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -20,6 +20,13 @@ namespace libcamera { namespace ipa::rkisp1 { +struct IPAHwSettings { + unsigned int numAeCells; + unsigned int numHistogramBins; + unsigned int numHistogramWeights; + unsigned int numGammaOutSamples; +}; + struct IPASessionConfiguration { struct { struct rkisp1_cif_isp_window measureWindow; @@ -45,10 +52,6 @@ struct IPASessionConfiguration { Size size; } sensor; - struct { - rkisp1_cif_isp_version revision; - } hw; - bool raw; }; @@ -143,6 +146,7 @@ struct IPAFrameContext : public FrameContext { }; struct IPAContext { + const IPAHwSettings *hw; IPASessionConfiguration configuration; IPAActiveState activeState; diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 6544c925ba25..44401af8910d 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -81,12 +81,6 @@ private: ControlInfoMap sensorControls_; - /* revision-specific data */ - rkisp1_cif_isp_version hwRevision_; - unsigned int hwHistBinNMax_; - unsigned int hwGammaOutMaxSamples_; - unsigned int hwHistogramWeightGridsSize_; - /* Interface to the Camera Helper */ std::unique_ptr<CameraSensorHelper> camHelper_; @@ -96,6 +90,20 @@ private: namespace { +const IPAHwSettings ipaHwSettingsV10{ + RKISP1_CIF_ISP_AE_MEAN_MAX_V10, + RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10, + RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10, + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10, +}; + +const IPAHwSettings ipaHwSettingsV12{ + RKISP1_CIF_ISP_AE_MEAN_MAX_V12, + RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12, + RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12, + RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12, +}; + /* List of controls handled by the RkISP1 IPA */ const ControlInfoMap::Map rkisp1Controls{ { &controls::AeEnable, ControlInfo(false, true) }, @@ -111,7 +119,7 @@ const ControlInfoMap::Map rkisp1Controls{ } /* namespace */ IPARkISP1::IPARkISP1() - : context_({ {}, {}, { kMaxFrameContexts } }) + : context_({ {}, {}, {}, { kMaxFrameContexts } }) { } @@ -128,14 +136,10 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, /* \todo Add support for other revisions */ switch (hwRevision) { case RKISP1_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; + context_.hw = &ipaHwSettingsV10; break; case RKISP1_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; + context_.hw = &ipaHwSettingsV12; break; default: LOG(IPARkISP1, Error) @@ -146,9 +150,6 @@ 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_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); if (!camHelper_) { LOG(IPARkISP1, Error) @@ -232,9 +233,6 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, context_.activeState = {}; context_.frameContexts.clear(); - /* Set the hardware revision for the algorithms. */ - context_.configuration.hw.revision = hwRevision_; - const IPACameraSensorInfo &info = ipaConfig.sensorInfo; const ControlInfo vBlank = sensorControls_.find(V4L2_CID_VBLANK)->second; context_.configuration.sensor.defVBlank = vBlank.def().get<int32_t>();
Versions of the ISP differ in the processing blocks they include, as well as in the implementation of some of those blocks. In particular, they have different numbers of histogram bins oe AE statistics cells. The algorithms take these differences into account by checking the ISP version reported by the driver. These checks are currently scattered in multiple places. Centralize them in the IPARkISP1::init() function, and store the version-dependent hardware parameters in the IPA context, accessible by all algorithms. While at it, drop the IPASessionConfiguration::hw member that stores the revision number, unused by the algorithms. It can be added back laer to the IPAHwSettings structure if needed. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/rkisp1/algorithms/agc.cpp | 27 +++++++++-------------- src/ipa/rkisp1/algorithms/agc.h | 3 --- src/ipa/rkisp1/ipa_context.cpp | 30 +++++++++++++++++++------- src/ipa/rkisp1/ipa_context.h | 12 +++++++---- src/ipa/rkisp1/rkisp1.cpp | 36 +++++++++++++++---------------- 5 files changed, 57 insertions(+), 51 deletions(-)