[PATCH v1 06/11] ipa: rkisp1: Use the new ISP parameters abstraction
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Thu Jul 4 14:33:19 CEST 2024
Hi Laurent
only one comment on dpf which is the only one with a more convoluted
handling. The rest looks very nice!
On Thu, Jul 04, 2024 at 01:52:25AM GMT, Laurent Pinchart wrote:
> Use the new ISP parameters abstraction class RkISP1Params to access the
> ISP parameters in the IPA algorithms. The class replaces the pointer to
> the rkisp1_params_cfg structure passed to the algorithms' prepare()
> function, and is used to access individual parameters blocks.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/ipa/rkisp1/algorithms/agc.cpp | 46 +++++++++++------------
> src/ipa/rkisp1/algorithms/agc.h | 2 +-
> src/ipa/rkisp1/algorithms/awb.cpp | 56 ++++++++++++----------------
> src/ipa/rkisp1/algorithms/awb.h | 2 +-
> src/ipa/rkisp1/algorithms/blc.cpp | 18 ++++-----
> src/ipa/rkisp1/algorithms/blc.h | 3 +-
> src/ipa/rkisp1/algorithms/ccm.cpp | 14 ++-----
> src/ipa/rkisp1/algorithms/ccm.h | 4 +-
> src/ipa/rkisp1/algorithms/cproc.cpp | 13 +++----
> src/ipa/rkisp1/algorithms/cproc.h | 2 +-
> src/ipa/rkisp1/algorithms/dpcc.cpp | 9 ++---
> src/ipa/rkisp1/algorithms/dpcc.h | 2 +-
> src/ipa/rkisp1/algorithms/dpf.cpp | 28 +++++++-------
> src/ipa/rkisp1/algorithms/dpf.h | 2 +-
> src/ipa/rkisp1/algorithms/filter.cpp | 51 ++++++++++++-------------
> src/ipa/rkisp1/algorithms/filter.h | 2 +-
> src/ipa/rkisp1/algorithms/goc.cpp | 15 ++++----
> src/ipa/rkisp1/algorithms/goc.h | 2 +-
> src/ipa/rkisp1/algorithms/gsl.cpp | 20 ++++------
> src/ipa/rkisp1/algorithms/gsl.h | 2 +-
> src/ipa/rkisp1/algorithms/lsc.cpp | 27 ++++++--------
> src/ipa/rkisp1/algorithms/lsc.h | 4 +-
> src/ipa/rkisp1/module.h | 3 +-
> src/ipa/rkisp1/rkisp1.cpp | 12 ++----
> 24 files changed, 148 insertions(+), 191 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
[snip]
> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp
> index abf957288550..b3f4446631fc 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.cpp
> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp
> @@ -215,15 +215,26 @@ void Dpf::queueRequest(IPAContext &context,
> * \copydoc libcamera::ipa::Algorithm::prepare
> */
> void Dpf::prepare(IPAContext &context, const uint32_t frame,
> - IPAFrameContext &frameContext, rkisp1_params_cfg *params)
> + IPAFrameContext &frameContext, RkISP1Params *params)
> {
> + if (!frameContext.dpf.update && frame > 0)
> + return;
> +
> + RkISP1Params::State state = frameContext.dpf.denoise
> + ? RkISP1Params::State::Enable
> + : RkISP1Params::State::Disable;
> + auto config = params->block<Block::Dpf>(state);
> +
> if (frame == 0) {
> - params->others.dpf_config = config_;
> - params->others.dpf_strength_config = strengthConfig_;
> + auto strengthConfig = params->block<Block::DpfStrength>(state);
The dpf strength handler in the kernel ignores the enable state, and
strengths are configured only when frame == 0, so I would use Enable
here for clarity (even if it has not practical differences).
> + *strengthConfig = strengthConfig_;
> +
> + *config = config_;
This, however needs to be done everytime we enable the block. The
driver sees that state == enable and programs the dpf block with the
content of the config structure. If frame > 0 &&
frameContext.dpf.denoise == true, the kernel will program the block
with 0-initialized data if I read it right.
I would set *config = config_ for all frames if dpf.update &&
dpf.denoise ?
Thanks
j
>
> const auto &awb = context.configuration.awb;
> const auto &lsc = context.configuration.lsc;
> - auto &mode = params->others.dpf_config.gain.mode;
> +
> + auto &mode = config->gain.mode;
>
> /*
> * The DPF needs to take into account the total amount of
> @@ -241,15 +252,6 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,
> mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;
> else
> mode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;
> -
> - params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_DPF |
> - RKISP1_CIF_ISP_MODULE_DPF_STRENGTH;
> - }
> -
> - if (frameContext.dpf.update) {
> - params->module_en_update |= RKISP1_CIF_ISP_MODULE_DPF;
> - if (frameContext.dpf.denoise)
> - params->module_ens |= RKISP1_CIF_ISP_MODULE_DPF;
> }
> }
>
> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h
> index da0115baf8f1..2dd8cd362624 100644
> --- a/src/ipa/rkisp1/algorithms/dpf.h
> +++ b/src/ipa/rkisp1/algorithms/dpf.h
> @@ -27,7 +27,7 @@ public:
> const ControlList &controls) override;
> void prepare(IPAContext &context, const uint32_t frame,
> IPAFrameContext &frameContext,
> - rkisp1_params_cfg *params) override;
> + RkISP1Params *params) override;
>
> private:
> struct rkisp1_cif_isp_dpf_config config_;
> diff --git a/src/ipa/rkisp1/algorithms/filter.cpp b/src/ipa/rkisp1/algorithms/filter.cpp
> index 9752248a5965..242b781a10f6 100644
> --- a/src/ipa/rkisp1/algorithms/filter.cpp
> +++ b/src/ipa/rkisp1/algorithms/filter.cpp
> @@ -104,7 +104,7 @@ void Filter::queueRequest(IPAContext &context,
> */
> void Filter::prepare([[maybe_unused]] IPAContext &context,
> [[maybe_unused]] const uint32_t frame,
> - IPAFrameContext &frameContext, rkisp1_params_cfg *params)
> + IPAFrameContext &frameContext, RkISP1Params *params)
> {
> /* Check if the algorithm configuration has been updated. */
> if (!frameContext.filter.update)
> @@ -160,23 +160,24 @@ void Filter::prepare([[maybe_unused]] IPAContext &context,
>
> uint8_t denoise = frameContext.filter.denoise;
> uint8_t sharpness = frameContext.filter.sharpness;
> - auto &flt_config = params->others.flt_config;
>
> - flt_config.fac_sh0 = filt_fac_sh0[sharpness];
> - flt_config.fac_sh1 = filt_fac_sh1[sharpness];
> - flt_config.fac_mid = filt_fac_mid[sharpness];
> - flt_config.fac_bl0 = filt_fac_bl0[sharpness];
> - flt_config.fac_bl1 = filt_fac_bl1[sharpness];
> + auto config = params->block<Block::Flt>(RkISP1Params::State::Enable);
>
> - flt_config.lum_weight = kFiltLumWeightDefault;
> - flt_config.mode = kFiltModeDefault;
> - flt_config.thresh_sh0 = filt_thresh_sh0[denoise];
> - flt_config.thresh_sh1 = filt_thresh_sh1[denoise];
> - flt_config.thresh_bl0 = filt_thresh_bl0[denoise];
> - flt_config.thresh_bl1 = filt_thresh_bl1[denoise];
> - flt_config.grn_stage1 = stage1_select[denoise];
> - flt_config.chr_v_mode = filt_chr_v_mode[denoise];
> - flt_config.chr_h_mode = filt_chr_h_mode[denoise];
> + config->fac_sh0 = filt_fac_sh0[sharpness];
> + config->fac_sh1 = filt_fac_sh1[sharpness];
> + config->fac_mid = filt_fac_mid[sharpness];
> + config->fac_bl0 = filt_fac_bl0[sharpness];
> + config->fac_bl1 = filt_fac_bl1[sharpness];
> +
> + config->lum_weight = kFiltLumWeightDefault;
> + config->mode = kFiltModeDefault;
> + config->thresh_sh0 = filt_thresh_sh0[denoise];
> + config->thresh_sh1 = filt_thresh_sh1[denoise];
> + config->thresh_bl0 = filt_thresh_bl0[denoise];
> + config->thresh_bl1 = filt_thresh_bl1[denoise];
> + config->grn_stage1 = stage1_select[denoise];
> + config->chr_v_mode = filt_chr_v_mode[denoise];
> + config->chr_h_mode = filt_chr_h_mode[denoise];
>
> /*
> * Combined high denoising and high sharpening requires some
> @@ -186,27 +187,23 @@ void Filter::prepare([[maybe_unused]] IPAContext &context,
> */
> if (denoise == 9) {
> if (sharpness > 3)
> - flt_config.grn_stage1 = 2;
> + config->grn_stage1 = 2;
> } else if (denoise == 10) {
> if (sharpness > 5)
> - flt_config.grn_stage1 = 2;
> + config->grn_stage1 = 2;
> else if (sharpness > 3)
> - flt_config.grn_stage1 = 1;
> + config->grn_stage1 = 1;
> }
>
> if (denoise > 7) {
> if (sharpness > 7) {
> - flt_config.fac_bl0 /= 2;
> - flt_config.fac_bl1 /= 4;
> + config->fac_bl0 /= 2;
> + config->fac_bl1 /= 4;
> } else if (sharpness > 4) {
> - flt_config.fac_bl0 = flt_config.fac_bl0 * 3 / 4;
> - flt_config.fac_bl1 /= 2;
> + config->fac_bl0 = config->fac_bl0 * 3 / 4;
> + config->fac_bl1 /= 2;
> }
> }
> -
> - params->module_en_update |= RKISP1_CIF_ISP_MODULE_FLT;
> - params->module_ens |= RKISP1_CIF_ISP_MODULE_FLT;
> - params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_FLT;
> }
>
> REGISTER_IPA_ALGORITHM(Filter, "Filter")
> diff --git a/src/ipa/rkisp1/algorithms/filter.h b/src/ipa/rkisp1/algorithms/filter.h
> index d595811d455f..8f858e574fa2 100644
> --- a/src/ipa/rkisp1/algorithms/filter.h
> +++ b/src/ipa/rkisp1/algorithms/filter.h
> @@ -26,7 +26,7 @@ public:
> const ControlList &controls) override;
> void prepare(IPAContext &context, const uint32_t frame,
> IPAFrameContext &frameContext,
> - rkisp1_params_cfg *params) override;
> + RkISP1Params *params) override;
> };
>
> } /* namespace ipa::rkisp1::algorithms */
> diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp
> index a82cee3bbf61..9067bf34c581 100644
> --- a/src/ipa/rkisp1/algorithms/goc.cpp
> +++ b/src/ipa/rkisp1/algorithms/goc.cpp
> @@ -99,11 +99,14 @@ void GammaOutCorrection::queueRequest(IPAContext &context, const uint32_t frame,
> void GammaOutCorrection::prepare(IPAContext &context,
> [[maybe_unused]] const uint32_t frame,
> IPAFrameContext &frameContext,
> - rkisp1_params_cfg *params)
> + RkISP1Params *params)
> {
> ASSERT(context.hw->numGammaOutSamples ==
> RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);
>
> + if (!frameContext.goc.update)
> + return;
> +
> /*
> * The logarithmic segments as specified in the reference.
> * Plus an additional 0 to make the loop easier
> @@ -112,10 +115,9 @@ void GammaOutCorrection::prepare(IPAContext &context,
> 64, 64, 64, 64, 128, 128, 128, 128, 256,
> 256, 256, 512, 512, 512, 512, 512, 0
> };
> - __u16 *gamma_y = params->others.goc_config.gamma_y;
>
> - if (!frameContext.goc.update)
> - return;
> + auto config = params->block<Block::Goc>(RkISP1Params::State::Enable);
> + __u16 *gamma_y = config->gamma_y;
>
> unsigned x = 0;
> for (const auto [i, size] : utils::enumerate(segments)) {
> @@ -123,10 +125,7 @@ void GammaOutCorrection::prepare(IPAContext &context,
> x += size;
> }
>
> - params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;
> - params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;
> - params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;
> - params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;
> + config->mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;
> }
>
> /**
> diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h
> index 0e05d7ce4a01..bb2ddfc92375 100644
> --- a/src/ipa/rkisp1/algorithms/goc.h
> +++ b/src/ipa/rkisp1/algorithms/goc.h
> @@ -28,7 +28,7 @@ public:
> const ControlList &controls) override;
> void prepare(IPAContext &context, const uint32_t frame,
> IPAFrameContext &frameContext,
> - rkisp1_params_cfg *params) override;
> + RkISP1Params *params) override;
> void process(IPAContext &context, const uint32_t frame,
> IPAFrameContext &frameContext,
> const rkisp1_stat_buffer *stats,
> diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp
> index 9b056c6edd96..f1880f16cc5d 100644
> --- a/src/ipa/rkisp1/algorithms/gsl.cpp
> +++ b/src/ipa/rkisp1/algorithms/gsl.cpp
> @@ -119,24 +119,18 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,
> void GammaSensorLinearization::prepare([[maybe_unused]] IPAContext &context,
> const uint32_t frame,
> [[maybe_unused]] IPAFrameContext &frameContext,
> - rkisp1_params_cfg *params)
> + RkISP1Params *params)
> {
> if (frame > 0)
> return;
>
> - params->others.sdg_config.xa_pnts.gamma_dx0 = gammaDx_[0];
> - params->others.sdg_config.xa_pnts.gamma_dx1 = gammaDx_[1];
> + auto config = params->block<Block::Sdg>(RkISP1Params::State::Enable);
> + config->xa_pnts.gamma_dx0 = gammaDx_[0];
> + config->xa_pnts.gamma_dx1 = gammaDx_[1];
>
> - std::copy(curveYr_.begin(), curveYr_.end(),
> - params->others.sdg_config.curve_r.gamma_y);
> - std::copy(curveYg_.begin(), curveYg_.end(),
> - params->others.sdg_config.curve_g.gamma_y);
> - std::copy(curveYb_.begin(), curveYb_.end(),
> - params->others.sdg_config.curve_b.gamma_y);
> -
> - params->module_en_update |= RKISP1_CIF_ISP_MODULE_SDG;
> - params->module_ens |= RKISP1_CIF_ISP_MODULE_SDG;
> - params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_SDG;
> + std::copy(curveYr_.begin(), curveYr_.end(), config->curve_r.gamma_y);
> + std::copy(curveYg_.begin(), curveYg_.end(), config->curve_g.gamma_y);
> + std::copy(curveYb_.begin(), curveYb_.end(), config->curve_b.gamma_y);
> }
>
> REGISTER_IPA_ALGORITHM(GammaSensorLinearization, "GammaSensorLinearization")
> diff --git a/src/ipa/rkisp1/algorithms/gsl.h b/src/ipa/rkisp1/algorithms/gsl.h
> index c404105e6310..91cf6efa7925 100644
> --- a/src/ipa/rkisp1/algorithms/gsl.h
> +++ b/src/ipa/rkisp1/algorithms/gsl.h
> @@ -22,7 +22,7 @@ public:
> int init(IPAContext &context, const YamlObject &tuningData) override;
> void prepare(IPAContext &context, const uint32_t frame,
> IPAFrameContext &frameContext,
> - rkisp1_params_cfg *params) override;
> + RkISP1Params *params) override;
>
> private:
> uint32_t gammaDx_[2];
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index 161183fca352..81a50e5b4396 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -185,18 +185,12 @@ int LensShadingCorrection::configure(IPAContext &context,
> return 0;
> }
>
> -void LensShadingCorrection::setParameters(rkisp1_params_cfg *params)
> +void LensShadingCorrection::setParameters(rkisp1_cif_isp_lsc_config &config)
> {
> - struct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;
> -
> memcpy(config.x_grad_tbl, xGrad_, sizeof(config.x_grad_tbl));
> memcpy(config.y_grad_tbl, yGrad_, sizeof(config.y_grad_tbl));
> memcpy(config.x_size_tbl, xSizes_, sizeof(config.x_size_tbl));
> memcpy(config.y_size_tbl, ySizes_, sizeof(config.y_size_tbl));
> -
> - params->module_en_update |= RKISP1_CIF_ISP_MODULE_LSC;
> - params->module_ens |= RKISP1_CIF_ISP_MODULE_LSC;
> - params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_LSC;
> }
>
> void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config,
> @@ -248,10 +242,8 @@ void LensShadingCorrection::interpolateTable(rkisp1_cif_isp_lsc_config &config,
> void LensShadingCorrection::prepare(IPAContext &context,
> const uint32_t frame,
> [[maybe_unused]] IPAFrameContext &frameContext,
> - rkisp1_params_cfg *params)
> + RkISP1Params *params)
> {
> - struct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;
> -
> /*
> * If there is only one set, the configuration has already been done
> * for first frame.
> @@ -264,8 +256,10 @@ void LensShadingCorrection::prepare(IPAContext &context,
> * never be relevant.
> */
> if (sets_.size() == 1) {
> - setParameters(params);
> - copyTable(config, sets_.cbegin()->second);
> + auto config = params->block<Block::Lsc>(RkISP1Params::State::Enable);
> +
> + setParameters(*config);
> + copyTable(*config, sets_.cbegin()->second);
> return;
> }
>
> @@ -294,13 +288,14 @@ void LensShadingCorrection::prepare(IPAContext &context,
> (lastCt_.adjusted <= ct && ct <= lastCt_.original))
> return;
>
> - setParameters(params);
> + auto config = params->block<Block::Lsc>(RkISP1Params::State::Enable);
> + setParameters(*config);
>
> /*
> * The color temperature matches exactly one of the available LSC tables.
> */
> if (sets_.count(ct)) {
> - copyTable(config, sets_[ct]);
> + copyTable(*config, sets_[ct]);
> lastCt_ = { ct, ct };
> return;
> }
> @@ -319,7 +314,7 @@ void LensShadingCorrection::prepare(IPAContext &context,
> if (diff0 < threshold || diff1 < threshold) {
> const Components &set = diff0 < diff1 ? set0 : set1;
> LOG(RkISP1Lsc, Debug) << "using LSC table for " << set.ct;
> - copyTable(config, set);
> + copyTable(*config, set);
> lastCt_ = { ct, set.ct };
> return;
> }
> @@ -331,7 +326,7 @@ void LensShadingCorrection::prepare(IPAContext &context,
> LOG(RkISP1Lsc, Debug)
> << "ct is " << ct << ", interpolating between "
> << ct0 << " and " << ct1;
> - interpolateTable(config, set0, set1, ct);
> + interpolateTable(*config, set0, set1, ct);
> lastCt_ = { ct, ct };
> }
>
> diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h
> index 5baf592797a6..a9c7a230e0fc 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.h
> +++ b/src/ipa/rkisp1/algorithms/lsc.h
> @@ -25,7 +25,7 @@ public:
> int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> void prepare(IPAContext &context, const uint32_t frame,
> IPAFrameContext &frameContext,
> - rkisp1_params_cfg *params) override;
> + RkISP1Params *params) override;
>
> private:
> struct Components {
> @@ -36,7 +36,7 @@ private:
> std::vector<uint16_t> b;
> };
>
> - void setParameters(rkisp1_params_cfg *params);
> + void setParameters(rkisp1_cif_isp_lsc_config &config);
> void copyTable(rkisp1_cif_isp_lsc_config &config, const Components &set0);
> void interpolateTable(rkisp1_cif_isp_lsc_config &config,
> const Components &set0, const Components &set1,
> diff --git a/src/ipa/rkisp1/module.h b/src/ipa/rkisp1/module.h
> index 16c3e43e88df..69e9bc823720 100644
> --- a/src/ipa/rkisp1/module.h
> +++ b/src/ipa/rkisp1/module.h
> @@ -14,13 +14,14 @@
> #include <libipa/module.h>
>
> #include "ipa_context.h"
> +#include "params.h"
>
> namespace libcamera {
>
> namespace ipa::rkisp1 {
>
> using Module = ipa::Module<IPAContext, IPAFrameContext, IPACameraSensorInfo,
> - rkisp1_params_cfg, rkisp1_stat_buffer>;
> + RkISP1Params, rkisp1_stat_buffer>;
>
> } /* namespace ipa::rkisp1 */
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 84ffe6cf2777..1a89eabf10b4 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -31,6 +31,7 @@
> #include "algorithms/algorithm.h"
>
> #include "ipa_context.h"
> +#include "params.h"
>
> namespace libcamera {
>
> @@ -322,17 +323,12 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
> {
> IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>
> - rkisp1_params_cfg *params =
> - reinterpret_cast<rkisp1_params_cfg *>(
> - mappedBuffers_.at(bufferId).planes()[0].data());
> -
> - /* Prepare parameters buffer. */
> - memset(params, 0, sizeof(*params));
> + RkISP1Params params(paramFormat_, mappedBuffers_.at(bufferId).planes()[0]);
>
> for (auto const &algo : algorithms())
> - algo->prepare(context_, frame, frameContext, params);
> + algo->prepare(context_, frame, frameContext, ¶ms);
>
> - paramsBufferReady.emit(frame, sizeof(*params));
> + paramsBufferReady.emit(frame, params.size());
> }
>
> void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list