[PATCH v3 2/5] ipa: rkisp1: Store hardware parameters in IPA context
Paul Elder
paul.elder at ideasonboard.com
Tue Feb 20 10:56:11 CET 2024
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 at ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder at 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
>
More information about the libcamera-devel
mailing list