[{"id":30327,"web_url":"https://patchwork.libcamera.org/comment/30327/","msgid":"","date":"2024-07-04T12:33:19","subject":"Re: [PATCH v1 06/11] ipa: rkisp1: Use the new ISP parameters\n\tabstraction","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\n only one comment on dpf which is the only one with a more convoluted\nhandling. The rest looks very nice!\n\nOn Thu, Jul 04, 2024 at 01:52:25AM GMT, Laurent Pinchart wrote:\n> Use the new ISP parameters abstraction class RkISP1Params to access the\n> ISP parameters in the IPA algorithms. The class replaces the pointer to\n> the rkisp1_params_cfg structure passed to the algorithms' prepare()\n> function, and is used to access individual parameters blocks.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 46 +++++++++++------------\n> src/ipa/rkisp1/algorithms/agc.h | 2 +-\n> src/ipa/rkisp1/algorithms/awb.cpp | 56 ++++++++++++----------------\n> src/ipa/rkisp1/algorithms/awb.h | 2 +-\n> src/ipa/rkisp1/algorithms/blc.cpp | 18 ++++-----\n> src/ipa/rkisp1/algorithms/blc.h | 3 +-\n> src/ipa/rkisp1/algorithms/ccm.cpp | 14 ++-----\n> src/ipa/rkisp1/algorithms/ccm.h | 4 +-\n> src/ipa/rkisp1/algorithms/cproc.cpp | 13 +++----\n> src/ipa/rkisp1/algorithms/cproc.h | 2 +-\n> src/ipa/rkisp1/algorithms/dpcc.cpp | 9 ++---\n> src/ipa/rkisp1/algorithms/dpcc.h | 2 +-\n> src/ipa/rkisp1/algorithms/dpf.cpp | 28 +++++++-------\n> src/ipa/rkisp1/algorithms/dpf.h | 2 +-\n> src/ipa/rkisp1/algorithms/filter.cpp | 51 ++++++++++++-------------\n> src/ipa/rkisp1/algorithms/filter.h | 2 +-\n> src/ipa/rkisp1/algorithms/goc.cpp | 15 ++++----\n> src/ipa/rkisp1/algorithms/goc.h | 2 +-\n> src/ipa/rkisp1/algorithms/gsl.cpp | 20 ++++------\n> src/ipa/rkisp1/algorithms/gsl.h | 2 +-\n> src/ipa/rkisp1/algorithms/lsc.cpp | 27 ++++++--------\n> src/ipa/rkisp1/algorithms/lsc.h | 4 +-\n> src/ipa/rkisp1/module.h | 3 +-\n> src/ipa/rkisp1/rkisp1.cpp | 12 ++----\n> 24 files changed, 148 insertions(+), 191 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n\n[snip]\n\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> index abf957288550..b3f4446631fc 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> @@ -215,15 +215,26 @@ void Dpf::queueRequest(IPAContext &context,\n> * \\copydoc libcamera::ipa::Algorithm::prepare\n> */\n> void Dpf::prepare(IPAContext &context, const uint32_t frame,\n> -\t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> +\t\t IPAFrameContext &frameContext, RkISP1Params *params)\n> {\n> +\tif (!frameContext.dpf.update && frame > 0)\n> +\t\treturn;\n> +\n> +\tRkISP1Params::State state = frameContext.dpf.denoise\n> +\t\t\t\t ? RkISP1Params::State::Enable\n> +\t\t\t\t : RkISP1Params::State::Disable;\n> +\tauto config = params->block(state);\n> +\n> \tif (frame == 0) {\n> -\t\tparams->others.dpf_config = config_;\n> -\t\tparams->others.dpf_strength_config = strengthConfig_;\n> +\t\tauto strengthConfig = params->block(state);\n\nThe dpf strength handler in the kernel ignores the enable state, and\nstrengths are configured only when frame == 0, so I would use Enable\nhere for clarity (even if it has not practical differences).\n\n> +\t\t*strengthConfig = strengthConfig_;\n> +\n> +\t\t*config = config_;\n\nThis, however needs to be done everytime we enable the block. The\ndriver sees that state == enable and programs the dpf block with the\ncontent of the config structure. If frame > 0 &&\nframeContext.dpf.denoise == true, the kernel will program the block\nwith 0-initialized data if I read it right.\n\nI would set *config = config_ for all frames if dpf.update &&\ndpf.denoise ?\n\nThanks\n j\n\n>\n> \t\tconst auto &awb = context.configuration.awb;\n> \t\tconst auto &lsc = context.configuration.lsc;\n> -\t\tauto &mode = params->others.dpf_config.gain.mode;\n> +\n> +\t\tauto &mode = config->gain.mode;\n>\n> \t\t/*\n> \t\t * The DPF needs to take into account the total amount of\n> @@ -241,15 +252,6 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,\n> \t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;\n> \t\telse\n> \t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;\n> -\n> -\t\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_DPF |\n> -\t\t\t\t\t RKISP1_CIF_ISP_MODULE_DPF_STRENGTH;\n> -\t}\n> -\n> -\tif (frameContext.dpf.update) {\n> -\t\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_DPF;\n> -\t\tif (frameContext.dpf.denoise)\n> -\t\t\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_DPF;\n> \t}\n> }\n>\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h\n> index da0115baf8f1..2dd8cd362624 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.h\n> +++ b/src/ipa/rkisp1/algorithms/dpf.h\n> @@ -27,7 +27,7 @@ public:\n> \t\t\t const ControlList &controls) override;\n> \tvoid prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> -\t\t rkisp1_params_cfg *params) override;\n> +\t\t RkISP1Params *params) override;\n>\n> private:\n> \tstruct rkisp1_cif_isp_dpf_config config_;\n> diff --git a/src/ipa/rkisp1/algorithms/filter.cpp b/src/ipa/rkisp1/algorithms/filter.cpp\n> index 9752248a5965..242b781a10f6 100644\n> --- a/src/ipa/rkisp1/algorithms/filter.cpp\n> +++ b/src/ipa/rkisp1/algorithms/filter.cpp\n> @@ -104,7 +104,7 @@ void Filter::queueRequest(IPAContext &context,\n> */\n> void Filter::prepare([[maybe_unused]] IPAContext &context,\n> \t\t [[maybe_unused]] const uint32_t frame,\n> -\t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> +\t\t IPAFrameContext &frameContext, RkISP1Params *params)\n> {\n> \t/* Check if the algorithm configuration has been updated. */\n> \tif (!frameContext.filter.update)\n> @@ -160,23 +160,24 @@ void Filter::prepare([[maybe_unused]] IPAContext &context,\n>\n> \tuint8_t denoise = frameContext.filter.denoise;\n> \tuint8_t sharpness = frameContext.filter.sharpness;\n> -\tauto &flt_config = params->others.flt_config;\n>\n> -\tflt_config.fac_sh0 = filt_fac_sh0[sharpness];\n> -\tflt_config.fac_sh1 = filt_fac_sh1[sharpness];\n> -\tflt_config.fac_mid = filt_fac_mid[sharpness];\n> -\tflt_config.fac_bl0 = filt_fac_bl0[sharpness];\n> -\tflt_config.fac_bl1 = filt_fac_bl1[sharpness];\n> +\tauto config = params->block(RkISP1Params::State::Enable);\n>\n> -\tflt_config.lum_weight = kFiltLumWeightDefault;\n> -\tflt_config.mode = kFiltModeDefault;\n> -\tflt_config.thresh_sh0 = filt_thresh_sh0[denoise];\n> -\tflt_config.thresh_sh1 = filt_thresh_sh1[denoise];\n> -\tflt_config.thresh_bl0 = filt_thresh_bl0[denoise];\n> -\tflt_config.thresh_bl1 = filt_thresh_bl1[denoise];\n> -\tflt_config.grn_stage1 = stage1_select[denoise];\n> -\tflt_config.chr_v_mode = filt_chr_v_mode[denoise];\n> -\tflt_config.chr_h_mode = filt_chr_h_mode[denoise];\n> +\tconfig->fac_sh0 = filt_fac_sh0[sharpness];\n> +\tconfig->fac_sh1 = filt_fac_sh1[sharpness];\n> +\tconfig->fac_mid = filt_fac_mid[sharpness];\n> +\tconfig->fac_bl0 = filt_fac_bl0[sharpness];\n> +\tconfig->fac_bl1 = filt_fac_bl1[sharpness];\n> +\n> +\tconfig->lum_weight = kFiltLumWeightDefault;\n> +\tconfig->mode = kFiltModeDefault;\n> +\tconfig->thresh_sh0 = filt_thresh_sh0[denoise];\n> +\tconfig->thresh_sh1 = filt_thresh_sh1[denoise];\n> +\tconfig->thresh_bl0 = filt_thresh_bl0[denoise];\n> +\tconfig->thresh_bl1 = filt_thresh_bl1[denoise];\n> +\tconfig->grn_stage1 = stage1_select[denoise];\n> +\tconfig->chr_v_mode = filt_chr_v_mode[denoise];\n> +\tconfig->chr_h_mode = filt_chr_h_mode[denoise];\n>\n> \t/*\n> \t * Combined high denoising and high sharpening requires some\n> @@ -186,27 +187,23 @@ void Filter::prepare([[maybe_unused]] IPAContext &context,\n> \t */\n> \tif (denoise == 9) {\n> \t\tif (sharpness > 3)\n> -\t\t\tflt_config.grn_stage1 = 2;\n> +\t\t\tconfig->grn_stage1 = 2;\n> \t} else if (denoise == 10) {\n> \t\tif (sharpness > 5)\n> -\t\t\tflt_config.grn_stage1 = 2;\n> +\t\t\tconfig->grn_stage1 = 2;\n> \t\telse if (sharpness > 3)\n> -\t\t\tflt_config.grn_stage1 = 1;\n> +\t\t\tconfig->grn_stage1 = 1;\n> \t}\n>\n> \tif (denoise > 7) {\n> \t\tif (sharpness > 7) {\n> -\t\t\tflt_config.fac_bl0 /= 2;\n> -\t\t\tflt_config.fac_bl1 /= 4;\n> +\t\t\tconfig->fac_bl0 /= 2;\n> +\t\t\tconfig->fac_bl1 /= 4;\n> \t\t} else if (sharpness > 4) {\n> -\t\t\tflt_config.fac_bl0 = flt_config.fac_bl0 * 3 / 4;\n> -\t\t\tflt_config.fac_bl1 /= 2;\n> +\t\t\tconfig->fac_bl0 = config->fac_bl0 * 3 / 4;\n> +\t\t\tconfig->fac_bl1 /= 2;\n> \t\t}\n> \t}\n> -\n> -\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_FLT;\n> -\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_FLT;\n> -\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_FLT;\n> }\n>\n> REGISTER_IPA_ALGORITHM(Filter, \"Filter\")\n> diff --git a/src/ipa/rkisp1/algorithms/filter.h b/src/ipa/rkisp1/algorithms/filter.h\n> index d595811d455f..8f858e574fa2 100644\n> --- a/src/ipa/rkisp1/algorithms/filter.h\n> +++ b/src/ipa/rkisp1/algorithms/filter.h\n> @@ -26,7 +26,7 @@ public:\n> \t\t\t const ControlList &controls) override;\n> \tvoid prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> -\t\t rkisp1_params_cfg *params) override;\n> +\t\t RkISP1Params *params) override;\n> };\n>\n> } /* namespace ipa::rkisp1::algorithms */\n> diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp\n> index a82cee3bbf61..9067bf34c581 100644\n> --- a/src/ipa/rkisp1/algorithms/goc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/goc.cpp\n> @@ -99,11 +99,14 @@ void GammaOutCorrection::queueRequest(IPAContext &context, const uint32_t frame,\n> void GammaOutCorrection::prepare(IPAContext &context,\n> \t\t\t\t [[maybe_unused]] const uint32_t frame,\n> \t\t\t\t IPAFrameContext &frameContext,\n> -\t\t\t\t rkisp1_params_cfg *params)\n> +\t\t\t\t RkISP1Params *params)\n> {\n> \tASSERT(context.hw->numGammaOutSamples ==\n> \t RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);\n>\n> +\tif (!frameContext.goc.update)\n> +\t\treturn;\n> +\n> \t/*\n> \t * The logarithmic segments as specified in the reference.\n> \t * Plus an additional 0 to make the loop easier\n> @@ -112,10 +115,9 @@ void GammaOutCorrection::prepare(IPAContext &context,\n> \t\t64, 64, 64, 64, 128, 128, 128, 128, 256,\n> \t\t256, 256, 512, 512, 512, 512, 512, 0\n> \t};\n> -\t__u16 *gamma_y = params->others.goc_config.gamma_y;\n>\n> -\tif (!frameContext.goc.update)\n> -\t\treturn;\n> +\tauto config = params->block(RkISP1Params::State::Enable);\n> +\t__u16 *gamma_y = config->gamma_y;\n>\n> \tunsigned x = 0;\n> \tfor (const auto [i, size] : utils::enumerate(segments)) {\n> @@ -123,10 +125,7 @@ void GammaOutCorrection::prepare(IPAContext &context,\n> \t\tx += size;\n> \t}\n>\n> -\tparams->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> -\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> -\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;\n> -\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;\n> +\tconfig->mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;\n> }\n>\n> /**\n> diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h\n> index 0e05d7ce4a01..bb2ddfc92375 100644\n> --- a/src/ipa/rkisp1/algorithms/goc.h\n> +++ b/src/ipa/rkisp1/algorithms/goc.h\n> @@ -28,7 +28,7 @@ public:\n> \t\t\t const ControlList &controls) override;\n> \tvoid prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> -\t\t rkisp1_params_cfg *params) override;\n> +\t\t RkISP1Params *params) override;\n> \tvoid process(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t const rkisp1_stat_buffer *stats,\n> diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp\n> index 9b056c6edd96..f1880f16cc5d 100644\n> --- a/src/ipa/rkisp1/algorithms/gsl.cpp\n> +++ b/src/ipa/rkisp1/algorithms/gsl.cpp\n> @@ -119,24 +119,18 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context,\n> void GammaSensorLinearization::prepare([[maybe_unused]] IPAContext &context,\n> \t\t\t\t const uint32_t frame,\n> \t\t\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> -\t\t\t\t rkisp1_params_cfg *params)\n> +\t\t\t\t RkISP1Params *params)\n> {\n> \tif (frame > 0)\n> \t\treturn;\n>\n> -\tparams->others.sdg_config.xa_pnts.gamma_dx0 = gammaDx_[0];\n> -\tparams->others.sdg_config.xa_pnts.gamma_dx1 = gammaDx_[1];\n> +\tauto config = params->block(RkISP1Params::State::Enable);\n> +\tconfig->xa_pnts.gamma_dx0 = gammaDx_[0];\n> +\tconfig->xa_pnts.gamma_dx1 = gammaDx_[1];\n>\n> -\tstd::copy(curveYr_.begin(), curveYr_.end(),\n> -\t\t params->others.sdg_config.curve_r.gamma_y);\n> -\tstd::copy(curveYg_.begin(), curveYg_.end(),\n> -\t\t params->others.sdg_config.curve_g.gamma_y);\n> -\tstd::copy(curveYb_.begin(), curveYb_.end(),\n> -\t\t params->others.sdg_config.curve_b.gamma_y);\n> -\n> -\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_SDG;\n> -\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_SDG;\n> -\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_SDG;\n> +\tstd::copy(curveYr_.begin(), curveYr_.end(), config->curve_r.gamma_y);\n> +\tstd::copy(curveYg_.begin(), curveYg_.end(), config->curve_g.gamma_y);\n> +\tstd::copy(curveYb_.begin(), curveYb_.end(), config->curve_b.gamma_y);\n> }\n>\n> REGISTER_IPA_ALGORITHM(GammaSensorLinearization, \"GammaSensorLinearization\")\n> diff --git a/src/ipa/rkisp1/algorithms/gsl.h b/src/ipa/rkisp1/algorithms/gsl.h\n> index c404105e6310..91cf6efa7925 100644\n> --- a/src/ipa/rkisp1/algorithms/gsl.h\n> +++ b/src/ipa/rkisp1/algorithms/gsl.h\n> @@ -22,7 +22,7 @@ public:\n> \tint init(IPAContext &context, const YamlObject &tuningData) override;\n> \tvoid prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> -\t\t rkisp1_params_cfg *params) override;\n> +\t\t RkISP1Params *params) override;\n>\n> private:\n> \tuint32_t gammaDx_[2];\n> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp\n> index 161183fca352..81a50e5b4396 100644\n> --- a/src/ipa/rkisp1/algorithms/lsc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp\n> @@ -185,18 +185,12 @@ int LensShadingCorrection::configure(IPAContext &context,\n> \treturn 0;\n> }\n>\n> -void LensShadingCorrection::setParameters(rkisp1_params_cfg *params)\n> +void LensShadingCorrection::setParameters(rkisp1_cif_isp_lsc_config &config)\n> {\n> -\tstruct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;\n> -\n> \tmemcpy(config.x_grad_tbl, xGrad_, sizeof(config.x_grad_tbl));\n> \tmemcpy(config.y_grad_tbl, yGrad_, sizeof(config.y_grad_tbl));\n> \tmemcpy(config.x_size_tbl, xSizes_, sizeof(config.x_size_tbl));\n> \tmemcpy(config.y_size_tbl, ySizes_, sizeof(config.y_size_tbl));\n> -\n> -\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_LSC;\n> -\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_LSC;\n> -\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_LSC;\n> }\n>\n> void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config,\n> @@ -248,10 +242,8 @@ void LensShadingCorrection::interpolateTable(rkisp1_cif_isp_lsc_config &config,\n> void LensShadingCorrection::prepare(IPAContext &context,\n> \t\t\t\t const uint32_t frame,\n> \t\t\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> -\t\t\t\t rkisp1_params_cfg *params)\n> +\t\t\t\t RkISP1Params *params)\n> {\n> -\tstruct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;\n> -\n> \t/*\n> \t * If there is only one set, the configuration has already been done\n> \t * for first frame.\n> @@ -264,8 +256,10 @@ void LensShadingCorrection::prepare(IPAContext &context,\n> \t * never be relevant.\n> \t */\n> \tif (sets_.size() == 1) {\n> -\t\tsetParameters(params);\n> -\t\tcopyTable(config, sets_.cbegin()->second);\n> +\t\tauto config = params->block(RkISP1Params::State::Enable);\n> +\n> +\t\tsetParameters(*config);\n> +\t\tcopyTable(*config, sets_.cbegin()->second);\n> \t\treturn;\n> \t}\n>\n> @@ -294,13 +288,14 @@ void LensShadingCorrection::prepare(IPAContext &context,\n> \t (lastCt_.adjusted <= ct && ct <= lastCt_.original))\n> \t\treturn;\n>\n> -\tsetParameters(params);\n> +\tauto config = params->block(RkISP1Params::State::Enable);\n> +\tsetParameters(*config);\n>\n> \t/*\n> \t * The color temperature matches exactly one of the available LSC tables.\n> \t */\n> \tif (sets_.count(ct)) {\n> -\t\tcopyTable(config, sets_[ct]);\n> +\t\tcopyTable(*config, sets_[ct]);\n> \t\tlastCt_ = { ct, ct };\n> \t\treturn;\n> \t}\n> @@ -319,7 +314,7 @@ void LensShadingCorrection::prepare(IPAContext &context,\n> \tif (diff0 < threshold || diff1 < threshold) {\n> \t\tconst Components &set = diff0 < diff1 ? set0 : set1;\n> \t\tLOG(RkISP1Lsc, Debug) << \"using LSC table for \" << set.ct;\n> -\t\tcopyTable(config, set);\n> +\t\tcopyTable(*config, set);\n> \t\tlastCt_ = { ct, set.ct };\n> \t\treturn;\n> \t}\n> @@ -331,7 +326,7 @@ void LensShadingCorrection::prepare(IPAContext &context,\n> \tLOG(RkISP1Lsc, Debug)\n> \t\t<< \"ct is \" << ct << \", interpolating between \"\n> \t\t<< ct0 << \" and \" << ct1;\n> -\tinterpolateTable(config, set0, set1, ct);\n> +\tinterpolateTable(*config, set0, set1, ct);\n> \tlastCt_ = { ct, ct };\n> }\n>\n> diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h\n> index 5baf592797a6..a9c7a230e0fc 100644\n> --- a/src/ipa/rkisp1/algorithms/lsc.h\n> +++ b/src/ipa/rkisp1/algorithms/lsc.h\n> @@ -25,7 +25,7 @@ public:\n> \tint configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;\n> \tvoid prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> -\t\t rkisp1_params_cfg *params) override;\n> +\t\t RkISP1Params *params) override;\n>\n> private:\n> \tstruct Components {\n> @@ -36,7 +36,7 @@ private:\n> \t\tstd::vector b;\n> \t};\n>\n> -\tvoid setParameters(rkisp1_params_cfg *params);\n> +\tvoid setParameters(rkisp1_cif_isp_lsc_config &config);\n> \tvoid copyTable(rkisp1_cif_isp_lsc_config &config, const Components &set0);\n> \tvoid interpolateTable(rkisp1_cif_isp_lsc_config &config,\n> \t\t\t const Components &set0, const Components &set1,\n> diff --git a/src/ipa/rkisp1/module.h b/src/ipa/rkisp1/module.h\n> index 16c3e43e88df..69e9bc823720 100644\n> --- a/src/ipa/rkisp1/module.h\n> +++ b/src/ipa/rkisp1/module.h\n> @@ -14,13 +14,14 @@\n> #include \n>\n> #include \"ipa_context.h\"\n> +#include \"params.h\"\n>\n> namespace libcamera {\n>\n> namespace ipa::rkisp1 {\n>\n> using Module = ipa::Module -\t\t\t rkisp1_params_cfg, rkisp1_stat_buffer>;\n> +\t\t\t RkISP1Params, rkisp1_stat_buffer>;\n>\n> } /* namespace ipa::rkisp1 */\n>\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 84ffe6cf2777..1a89eabf10b4 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -31,6 +31,7 @@\n> #include \"algorithms/algorithm.h\"\n>\n> #include \"ipa_context.h\"\n> +#include \"params.h\"\n>\n> namespace libcamera {\n>\n> @@ -322,17 +323,12 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> {\n> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>\n> -\trkisp1_params_cfg *params =\n> -\t\treinterpret_cast(\n> -\t\t\tmappedBuffers_.at(bufferId).planes()[0].data());\n> -\n> -\t/* Prepare parameters buffer. */\n> -\tmemset(params, 0, sizeof(*params));\n> +\tRkISP1Params params(paramFormat_, mappedBuffers_.at(bufferId).planes()[0]);\n>\n> \tfor (auto const &algo : algorithms())\n> -\t\talgo->prepare(context_, frame, frameContext, params);\n> +\t\talgo->prepare(context_, frame, frameContext, ¶ms);\n>\n> -\tparamsBufferReady.emit(frame, sizeof(*params));\n> +\tparamsBufferReady.emit(frame, params.size());\n> }\n>\n> void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A0C67BD87C\n\tfor ;\n\tThu, 4 Jul 2024 12:33:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5532362E22;\n\tThu, 4 Jul 2024 14:33:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F053619C8\n\tfor ;\n\tThu, 4 Jul 2024 14:33:23 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F22AC63D;\n\tThu, 4 Jul 2024 14:32:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ev8DE7m3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720096374;\n\tbh=umli5d4tYECpsqY8m37nkjO9tSlTauMFtjjow2GebzI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ev8DE7m3qQUK497iQxQw6SpTqX/ng8jHPg9JjQSpuMsK1jSM8AFZPq4n2k3rqLLwK\n\tiI0VFax/wHRcdHkPow72yP6O60bdiGlzz3JO/Meb7g203qS0pHqXCPwqazXYIeWbXT\n\tEkHfWhX2oMoMjlDGlwomAlTli+YQyhkNyizByF0A=","Date":"Thu, 4 Jul 2024 14:33:19 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ,\n\tPaul Elder ","Subject":"Re: [PATCH v1 06/11] ipa: rkisp1: Use the new ISP parameters\n\tabstraction","Message-ID":"","References":"<20240703225230.3530-1-laurent.pinchart@ideasonboard.com>\n\t<20240703225230.3530-7-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240703225230.3530-7-laurent.pinchart@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30332,"web_url":"https://patchwork.libcamera.org/comment/30332/","msgid":"<20240704133834.GE6230@pendragon.ideasonboard.com>","date":"2024-07-04T13:38:34","subject":"Re: [PATCH v1 06/11] ipa: rkisp1: Use the new ISP parameters\n\tabstraction","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jul 04, 2024 at 02:33:19PM +0200, Jacopo Mondi wrote:\n> Hi Laurent\n> \n> only one comment on dpf which is the only one with a more convoluted\n> handling. The rest looks very nice!\n> \n> On Thu, Jul 04, 2024 at 01:52:25AM GMT, Laurent Pinchart wrote:\n> > Use the new ISP parameters abstraction class RkISP1Params to access the\n> > ISP parameters in the IPA algorithms. The class replaces the pointer to\n> > the rkisp1_params_cfg structure passed to the algorithms' prepare()\n> > function, and is used to access individual parameters blocks.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 46 +++++++++++------------\n> > src/ipa/rkisp1/algorithms/agc.h | 2 +-\n> > src/ipa/rkisp1/algorithms/awb.cpp | 56 ++++++++++++----------------\n> > src/ipa/rkisp1/algorithms/awb.h | 2 +-\n> > src/ipa/rkisp1/algorithms/blc.cpp | 18 ++++-----\n> > src/ipa/rkisp1/algorithms/blc.h | 3 +-\n> > src/ipa/rkisp1/algorithms/ccm.cpp | 14 ++-----\n> > src/ipa/rkisp1/algorithms/ccm.h | 4 +-\n> > src/ipa/rkisp1/algorithms/cproc.cpp | 13 +++----\n> > src/ipa/rkisp1/algorithms/cproc.h | 2 +-\n> > src/ipa/rkisp1/algorithms/dpcc.cpp | 9 ++---\n> > src/ipa/rkisp1/algorithms/dpcc.h | 2 +-\n> > src/ipa/rkisp1/algorithms/dpf.cpp | 28 +++++++-------\n> > src/ipa/rkisp1/algorithms/dpf.h | 2 +-\n> > src/ipa/rkisp1/algorithms/filter.cpp | 51 ++++++++++++-------------\n> > src/ipa/rkisp1/algorithms/filter.h | 2 +-\n> > src/ipa/rkisp1/algorithms/goc.cpp | 15 ++++----\n> > src/ipa/rkisp1/algorithms/goc.h | 2 +-\n> > src/ipa/rkisp1/algorithms/gsl.cpp | 20 ++++------\n> > src/ipa/rkisp1/algorithms/gsl.h | 2 +-\n> > src/ipa/rkisp1/algorithms/lsc.cpp | 27 ++++++--------\n> > src/ipa/rkisp1/algorithms/lsc.h | 4 +-\n> > src/ipa/rkisp1/module.h | 3 +-\n> > src/ipa/rkisp1/rkisp1.cpp | 12 ++----\n> > 24 files changed, 148 insertions(+), 191 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> \n> [snip]\n> \n> > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > index abf957288550..b3f4446631fc 100644\n> > --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > @@ -215,15 +215,26 @@ void Dpf::queueRequest(IPAContext &context,\n> > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > */\n> > void Dpf::prepare(IPAContext &context, const uint32_t frame,\n> > -\t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> > +\t\t IPAFrameContext &frameContext, RkISP1Params *params)\n> > {\n> > +\tif (!frameContext.dpf.update && frame > 0)\n> > +\t\treturn;\n> > +\n> > +\tRkISP1Params::State state = frameContext.dpf.denoise\n> > +\t\t\t\t ? RkISP1Params::State::Enable\n> > +\t\t\t\t : RkISP1Params::State::Disable;\n> > +\tauto config = params->block(state);\n> > +\n> > \tif (frame == 0) {\n> > -\t\tparams->others.dpf_config = config_;\n> > -\t\tparams->others.dpf_strength_config = strengthConfig_;\n> > +\t\tauto strengthConfig = params->block(state);\n> \n> The dpf strength handler in the kernel ignores the enable state, and\n> strengths are configured only when frame == 0, so I would use Enable\n> here for clarity (even if it has not practical differences).\n\nOK.\n\n> > +\t\t*strengthConfig = strengthConfig_;\n> > +\n> > +\t\t*config = config_;\n> \n> This, however needs to be done everytime we enable the block. The\n> driver sees that state == enable and programs the dpf block with the\n> content of the config structure. If frame > 0 &&\n> frameContext.dpf.denoise == true, the kernel will program the block\n> with 0-initialized data if I read it right.\n> \n> I would set *config = config_ for all frames if dpf.update &&\n> dpf.denoise ?\n\nYes there's an issue indeed.\n\nThis leads to an annoying question: do we need the ability to\nenable/disable blocks without reconfiguring them ?\n\n> >\n> > \t\tconst auto &awb = context.configuration.awb;\n> > \t\tconst auto &lsc = context.configuration.lsc;\n> > -\t\tauto &mode = params->others.dpf_config.gain.mode;\n> > +\n> > +\t\tauto &mode = config->gain.mode;\n> >\n> > \t\t/*\n> > \t\t * The DPF needs to take into account the total amount of\n> > @@ -241,15 +252,6 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,\n> > \t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;\n> > \t\telse\n> > \t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;\n> > -\n> > -\t\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_DPF |\n> > -\t\t\t\t\t RKISP1_CIF_ISP_MODULE_DPF_STRENGTH;\n> > -\t}\n> > -\n> > -\tif (frameContext.dpf.update) {\n> > -\t\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_DPF;\n> > -\t\tif (frameContext.dpf.denoise)\n> > -\t\t\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_DPF;\n> > \t}\n> > }\n> >\n\n[snip]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3F344BEFBE\n\tfor ;\n\tThu, 4 Jul 2024 13:38:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B080162E23;\n\tThu, 4 Jul 2024 15:38:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79D65619C8\n\tfor ;\n\tThu, 4 Jul 2024 15:38:55 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A5C76502;\n\tThu, 4 Jul 2024 15:38:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qSbX3tQa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720100306;\n\tbh=r4vzhrIijSRVTHgRR7GZod9GQJKClg7L+S/PoK9sKw0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qSbX3tQaoy3aQrSoFIOc7liqcFAeFgSTUp6f9iwDiUeDIMxJhSDnY3+MtVt9rVsk2\n\tOBR6Ue9Ky4Gizkv+gI5RGdUu3ufs3gV/ClEY2LSn5399vx0tuBfDv+3u2KH3uw6exj\n\tgehnhhvHDuftlobo3AWXrxQIFFS8z3GcvIWgf/n4=","Date":"Thu, 4 Jul 2024 16:38:34 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v1 06/11] ipa: rkisp1: Use the new ISP parameters\n\tabstraction","Message-ID":"<20240704133834.GE6230@pendragon.ideasonboard.com>","References":"<20240703225230.3530-1-laurent.pinchart@ideasonboard.com>\n\t<20240703225230.3530-7-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30335,"web_url":"https://patchwork.libcamera.org/comment/30335/","msgid":"","date":"2024-07-04T14:14:19","subject":"Re: [PATCH v1 06/11] ipa: rkisp1: Use the new ISP parameters\n\tabstraction","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Thu, Jul 04, 2024 at 04:38:34PM GMT, Laurent Pinchart wrote:\n> On Thu, Jul 04, 2024 at 02:33:19PM +0200, Jacopo Mondi wrote:\n> > Hi Laurent\n> >\n> > only one comment on dpf which is the only one with a more convoluted\n> > handling. The rest looks very nice!\n> >\n> > On Thu, Jul 04, 2024 at 01:52:25AM GMT, Laurent Pinchart wrote:\n> > > Use the new ISP parameters abstraction class RkISP1Params to access the\n> > > ISP parameters in the IPA algorithms. The class replaces the pointer to\n> > > the rkisp1_params_cfg structure passed to the algorithms' prepare()\n> > > function, and is used to access individual parameters blocks.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 46 +++++++++++------------\n> > > src/ipa/rkisp1/algorithms/agc.h | 2 +-\n> > > src/ipa/rkisp1/algorithms/awb.cpp | 56 ++++++++++++----------------\n> > > src/ipa/rkisp1/algorithms/awb.h | 2 +-\n> > > src/ipa/rkisp1/algorithms/blc.cpp | 18 ++++-----\n> > > src/ipa/rkisp1/algorithms/blc.h | 3 +-\n> > > src/ipa/rkisp1/algorithms/ccm.cpp | 14 ++-----\n> > > src/ipa/rkisp1/algorithms/ccm.h | 4 +-\n> > > src/ipa/rkisp1/algorithms/cproc.cpp | 13 +++----\n> > > src/ipa/rkisp1/algorithms/cproc.h | 2 +-\n> > > src/ipa/rkisp1/algorithms/dpcc.cpp | 9 ++---\n> > > src/ipa/rkisp1/algorithms/dpcc.h | 2 +-\n> > > src/ipa/rkisp1/algorithms/dpf.cpp | 28 +++++++-------\n> > > src/ipa/rkisp1/algorithms/dpf.h | 2 +-\n> > > src/ipa/rkisp1/algorithms/filter.cpp | 51 ++++++++++++-------------\n> > > src/ipa/rkisp1/algorithms/filter.h | 2 +-\n> > > src/ipa/rkisp1/algorithms/goc.cpp | 15 ++++----\n> > > src/ipa/rkisp1/algorithms/goc.h | 2 +-\n> > > src/ipa/rkisp1/algorithms/gsl.cpp | 20 ++++------\n> > > src/ipa/rkisp1/algorithms/gsl.h | 2 +-\n> > > src/ipa/rkisp1/algorithms/lsc.cpp | 27 ++++++--------\n> > > src/ipa/rkisp1/algorithms/lsc.h | 4 +-\n> > > src/ipa/rkisp1/module.h | 3 +-\n> > > src/ipa/rkisp1/rkisp1.cpp | 12 ++----\n> > > 24 files changed, 148 insertions(+), 191 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> >\n> > [snip]\n> >\n> > > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > > index abf957288550..b3f4446631fc 100644\n> > > --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > > @@ -215,15 +215,26 @@ void Dpf::queueRequest(IPAContext &context,\n> > > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > */\n> > > void Dpf::prepare(IPAContext &context, const uint32_t frame,\n> > > -\t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> > > +\t\t IPAFrameContext &frameContext, RkISP1Params *params)\n> > > {\n> > > +\tif (!frameContext.dpf.update && frame > 0)\n> > > +\t\treturn;\n> > > +\n> > > +\tRkISP1Params::State state = frameContext.dpf.denoise\n> > > +\t\t\t\t ? RkISP1Params::State::Enable\n> > > +\t\t\t\t : RkISP1Params::State::Disable;\n> > > +\tauto config = params->block(state);\n> > > +\n> > > \tif (frame == 0) {\n> > > -\t\tparams->others.dpf_config = config_;\n> > > -\t\tparams->others.dpf_strength_config = strengthConfig_;\n> > > +\t\tauto strengthConfig = params->block(state);\n> >\n> > The dpf strength handler in the kernel ignores the enable state, and\n> > strengths are configured only when frame == 0, so I would use Enable\n> > here for clarity (even if it has not practical differences).\n>\n> OK.\n>\n> > > +\t\t*strengthConfig = strengthConfig_;\n> > > +\n> > > +\t\t*config = config_;\n> >\n> > This, however needs to be done everytime we enable the block. The\n> > driver sees that state == enable and programs the dpf block with the\n> > content of the config structure. If frame > 0 &&\n> > frameContext.dpf.denoise == true, the kernel will program the block\n> > with 0-initialized data if I read it right.\n> >\n> > I would set *config = config_ for all frames if dpf.update &&\n> > dpf.denoise ?\n>\n> Yes there's an issue indeed.\n>\n> This leads to an annoying question: do we need the ability to\n> enable/disable blocks without reconfiguring them ?\n>\n\nThis indeed is not possible with the current uAPI. However, if to do\nwe need to expand the number of enablment states\n\nenum rkisp1_ext_params_block_enable {\n\tRKISP1_EXT_PARAMS_BLOCK_DISABLE,\n\tRKISP1_EXT_PARAMS_BLOCK_ENABLE,\n};\n\nin example by adding an additional entry as\n\nenum rkisp1_ext_params_block_enable {\n\tRKISP1_EXT_PARAMS_BLOCK_DISABLE,\n\tRKISP1_EXT_PARAMS_BLOCK_ENABLE,\n RKISP1_EXT_PARAMS_BLOCK_ENABLE_NO_CONFIG,\n};\n\n\nwould could do so later without breaking the existing users\n\n> > >\n> > > \t\tconst auto &awb = context.configuration.awb;\n> > > \t\tconst auto &lsc = context.configuration.lsc;\n> > > -\t\tauto &mode = params->others.dpf_config.gain.mode;\n> > > +\n> > > +\t\tauto &mode = config->gain.mode;\n> > >\n> > > \t\t/*\n> > > \t\t * The DPF needs to take into account the total amount of\n> > > @@ -241,15 +252,6 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,\n> > > \t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;\n> > > \t\telse\n> > > \t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;\n> > > -\n> > > -\t\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_DPF |\n> > > -\t\t\t\t\t RKISP1_CIF_ISP_MODULE_DPF_STRENGTH;\n> > > -\t}\n> > > -\n> > > -\tif (frameContext.dpf.update) {\n> > > -\t\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_DPF;\n> > > -\t\tif (frameContext.dpf.denoise)\n> > > -\t\t\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_DPF;\n> > > \t}\n> > > }\n> > >\n>\n> [snip]\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8D470BEFBE\n\tfor ;\n\tThu, 4 Jul 2024 14:14:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3A88262E22;\n\tThu, 4 Jul 2024 16:14:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E62E619C8\n\tfor ;\n\tThu, 4 Jul 2024 16:14:23 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DB0B063D;\n\tThu, 4 Jul 2024 16:13:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wHgqGjay\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720102434;\n\tbh=0k/HRv/r8TjwBBY8eYi1yu+aph7OBgAbwYwVMHRG0IQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wHgqGjayNsteYPPTn2ViRYf3NfE0XtWtDb7ZUMK60ozJ3ncOgGLIaHYRYw7taxLh4\n\tUG3YC2abwN90WQFDF5BPdrT7F9Exygq7j0sFphI6+va9BQEeyJb+2jXcLmBicxasnd\n\tXs7PAokMKcCf3SkHrKjsiMwqW0Lc2rI8YGAQ7luc=","Date":"Thu, 4 Jul 2024 16:14:19 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v1 06/11] ipa: rkisp1: Use the new ISP parameters\n\tabstraction","Message-ID":"","References":"<20240703225230.3530-1-laurent.pinchart@ideasonboard.com>\n\t<20240703225230.3530-7-laurent.pinchart@ideasonboard.com>\n\t\n\t<20240704133834.GE6230@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240704133834.GE6230@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30336,"web_url":"https://patchwork.libcamera.org/comment/30336/","msgid":"<20240704144944.GC24184@pendragon.ideasonboard.com>","date":"2024-07-04T14:49:44","subject":"Re: [PATCH v1 06/11] ipa: rkisp1: Use the new ISP parameters\n\tabstraction","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jul 04, 2024 at 04:14:19PM +0200, Jacopo Mondi wrote:\n> On Thu, Jul 04, 2024 at 04:38:34PM GMT, Laurent Pinchart wrote:\n> > On Thu, Jul 04, 2024 at 02:33:19PM +0200, Jacopo Mondi wrote:\n> > > Hi Laurent\n> > >\n> > > only one comment on dpf which is the only one with a more convoluted\n> > > handling. The rest looks very nice!\n> > >\n> > > On Thu, Jul 04, 2024 at 01:52:25AM GMT, Laurent Pinchart wrote:\n> > > > Use the new ISP parameters abstraction class RkISP1Params to access the\n> > > > ISP parameters in the IPA algorithms. The class replaces the pointer to\n> > > > the rkisp1_params_cfg structure passed to the algorithms' prepare()\n> > > > function, and is used to access individual parameters blocks.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 46 +++++++++++------------\n> > > > src/ipa/rkisp1/algorithms/agc.h | 2 +-\n> > > > src/ipa/rkisp1/algorithms/awb.cpp | 56 ++++++++++++----------------\n> > > > src/ipa/rkisp1/algorithms/awb.h | 2 +-\n> > > > src/ipa/rkisp1/algorithms/blc.cpp | 18 ++++-----\n> > > > src/ipa/rkisp1/algorithms/blc.h | 3 +-\n> > > > src/ipa/rkisp1/algorithms/ccm.cpp | 14 ++-----\n> > > > src/ipa/rkisp1/algorithms/ccm.h | 4 +-\n> > > > src/ipa/rkisp1/algorithms/cproc.cpp | 13 +++----\n> > > > src/ipa/rkisp1/algorithms/cproc.h | 2 +-\n> > > > src/ipa/rkisp1/algorithms/dpcc.cpp | 9 ++---\n> > > > src/ipa/rkisp1/algorithms/dpcc.h | 2 +-\n> > > > src/ipa/rkisp1/algorithms/dpf.cpp | 28 +++++++-------\n> > > > src/ipa/rkisp1/algorithms/dpf.h | 2 +-\n> > > > src/ipa/rkisp1/algorithms/filter.cpp | 51 ++++++++++++-------------\n> > > > src/ipa/rkisp1/algorithms/filter.h | 2 +-\n> > > > src/ipa/rkisp1/algorithms/goc.cpp | 15 ++++----\n> > > > src/ipa/rkisp1/algorithms/goc.h | 2 +-\n> > > > src/ipa/rkisp1/algorithms/gsl.cpp | 20 ++++------\n> > > > src/ipa/rkisp1/algorithms/gsl.h | 2 +-\n> > > > src/ipa/rkisp1/algorithms/lsc.cpp | 27 ++++++--------\n> > > > src/ipa/rkisp1/algorithms/lsc.h | 4 +-\n> > > > src/ipa/rkisp1/module.h | 3 +-\n> > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++----\n> > > > 24 files changed, 148 insertions(+), 191 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > >\n> > > [snip]\n> > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > > > index abf957288550..b3f4446631fc 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > > > @@ -215,15 +215,26 @@ void Dpf::queueRequest(IPAContext &context,\n> > > > * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > */\n> > > > void Dpf::prepare(IPAContext &context, const uint32_t frame,\n> > > > -\t\t IPAFrameContext &frameContext, rkisp1_params_cfg *params)\n> > > > +\t\t IPAFrameContext &frameContext, RkISP1Params *params)\n> > > > {\n> > > > +\tif (!frameContext.dpf.update && frame > 0)\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tRkISP1Params::State state = frameContext.dpf.denoise\n> > > > +\t\t\t\t ? RkISP1Params::State::Enable\n> > > > +\t\t\t\t : RkISP1Params::State::Disable;\n> > > > +\tauto config = params->block(state);\n> > > > +\n> > > > \tif (frame == 0) {\n> > > > -\t\tparams->others.dpf_config = config_;\n> > > > -\t\tparams->others.dpf_strength_config = strengthConfig_;\n> > > > +\t\tauto strengthConfig = params->block(state);\n> > >\n> > > The dpf strength handler in the kernel ignores the enable state, and\n> > > strengths are configured only when frame == 0, so I would use Enable\n> > > here for clarity (even if it has not practical differences).\n> >\n> > OK.\n> >\n> > > > +\t\t*strengthConfig = strengthConfig_;\n> > > > +\n> > > > +\t\t*config = config_;\n> > >\n> > > This, however needs to be done everytime we enable the block. The\n> > > driver sees that state == enable and programs the dpf block with the\n> > > content of the config structure. If frame > 0 &&\n> > > frameContext.dpf.denoise == true, the kernel will program the block\n> > > with 0-initialized data if I read it right.\n> > >\n> > > I would set *config = config_ for all frames if dpf.update &&\n> > > dpf.denoise ?\n> >\n> > Yes there's an issue indeed.\n> >\n> > This leads to an annoying question: do we need the ability to\n> > enable/disable blocks without reconfiguring them ?\n> \n> This indeed is not possible with the current uAPI. However, if to do\n> we need to expand the number of enablment states\n> \n> enum rkisp1_ext_params_block_enable {\n> \tRKISP1_EXT_PARAMS_BLOCK_DISABLE,\n> \tRKISP1_EXT_PARAMS_BLOCK_ENABLE,\n> };\n> \n> in example by adding an additional entry as\n> \n> enum rkisp1_ext_params_block_enable {\n> \tRKISP1_EXT_PARAMS_BLOCK_DISABLE,\n> \tRKISP1_EXT_PARAMS_BLOCK_ENABLE,\n> \tRKISP1_EXT_PARAMS_BLOCK_ENABLE_NO_CONFIG,\n> };\n\nAnd we may need the ability to configure the block while keeping it\ndisabled :-/\n\n> would could do so later without breaking the existing users\n\nThat, or we could omit the data after the header. It would decouple\nenabling (set through the enable flag) and config (set through inclusion\nof the data after the header). This should also not break existing\nusers, although we may want to then drop the structures that wrap the\nheader and configuration data.\n\nIt may also be more troublesome for userspace to use, as code would need\nto know whether or not configuration data is needed at the time the\nblock is allocated. We would need to prototype first.\n\n> > > >\n> > > > \t\tconst auto &awb = context.configuration.awb;\n> > > > \t\tconst auto &lsc = context.configuration.lsc;\n> > > > -\t\tauto &mode = params->others.dpf_config.gain.mode;\n> > > > +\n> > > > +\t\tauto &mode = config->gain.mode;\n> > > >\n> > > > \t\t/*\n> > > > \t\t * The DPF needs to take into account the total amount of\n> > > > @@ -241,15 +252,6 @@ void Dpf::prepare(IPAContext &context, const uint32_t frame,\n> > > > \t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_LSC_GAINS;\n> > > > \t\telse\n> > > > \t\t\tmode = RKISP1_CIF_ISP_DPF_GAIN_USAGE_DISABLED;\n> > > > -\n> > > > -\t\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_DPF |\n> > > > -\t\t\t\t\t RKISP1_CIF_ISP_MODULE_DPF_STRENGTH;\n> > > > -\t}\n> > > > -\n> > > > -\tif (frameContext.dpf.update) {\n> > > > -\t\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_DPF;\n> > > > -\t\tif (frameContext.dpf.denoise)\n> > > > -\t\t\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_DPF;\n> > > > \t}\n> > > > }\n> > > >\n> >\n> > [snip]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9EB2BBD87C\n\tfor ;\n\tThu, 4 Jul 2024 14:50:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53B26619C8;\n\tThu, 4 Jul 2024 16:50:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BAF22619C8\n\tfor ;\n\tThu, 4 Jul 2024 16:50:06 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7B7C063D;\n\tThu, 4 Jul 2024 16:49:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JT56IMfj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720104577;\n\tbh=PGR5qyNa5wXsMe5sJawOIbfA/A4c3p/iSyn4SWw45ZM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JT56IMfjGGDIhCc9DYJSCz9jRmNkivWenztM43zZH4+WHulpmo7OWh1vsX7N3zRLx\n\t9dy6JViybltJrxxED5f+QR5kvSu8jDEVifLqkWgE71esq4QSARC5j0d9x9uFXsOvZ4\n\trupnv8cMlB4aE3ko3DdYUqBKqSboFyVzI43/4mmI=","Date":"Thu, 4 Jul 2024 17:49:44 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v1 06/11] ipa: rkisp1: Use the new ISP parameters\n\tabstraction","Message-ID":"<20240704144944.GC24184@pendragon.ideasonboard.com>","References":"<20240703225230.3530-1-laurent.pinchart@ideasonboard.com>\n\t<20240703225230.3530-7-laurent.pinchart@ideasonboard.com>\n\t\n\t<20240704133834.GE6230@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]