[{"id":36193,"web_url":"https://patchwork.libcamera.org/comment/36193/","msgid":"","date":"2025-10-10T15:16:37","subject":"Re: [PATCH v5 5/5] ipa: mali-c55: Introduce MaliC55Params","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 07/10/2025 19:17, Jacopo Mondi wrote:\n> Implement MaliC55Params to derive from V4L2Params and use the new\n> helpers in the Mali C55 IPA algorithms implementation.\n> \n> Signed-off-by: Jacopo Mondi \n> Tested-by: Antoine Bouyer \n\nCouple of nits below, but with or without them changed:\n\nReviewed-by: Daniel Scally \n\n> ---\n> src/ipa/mali-c55/algorithms/agc.cpp | 87 +++++++++++++++----------------------\n> src/ipa/mali-c55/algorithms/agc.h | 14 +++---\n> src/ipa/mali-c55/algorithms/awb.cpp | 64 +++++++++++----------------\n> src/ipa/mali-c55/algorithms/awb.h | 10 ++---\n> src/ipa/mali-c55/algorithms/blc.cpp | 20 +++------\n> src/ipa/mali-c55/algorithms/blc.h | 3 +-\n> src/ipa/mali-c55/algorithms/lsc.cpp | 58 ++++++++++---------------\n> src/ipa/mali-c55/algorithms/lsc.h | 8 ++--\n> src/ipa/mali-c55/mali-c55.cpp | 20 +++------\n> src/ipa/mali-c55/module.h | 3 +-\n> src/ipa/mali-c55/params.h | 83 +++++++++++++++++++++++++++++++++++\n> 11 files changed, 197 insertions(+), 173 deletions(-)\n> \n> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n> index 78e7e07b2348a711e6261bac45e006f49a59513a..5d60cd8c9ff8a90e2e48e5d6939b538135b7ec3d 100644\n> --- a/src/ipa/mali-c55/algorithms/agc.cpp\n> +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n> @@ -241,8 +241,8 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame,\n> \t}\n> }\n> \n> -size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContext,\n> -\t\t\t mali_c55_params_block block)\n> +void Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContext,\n> +\t\t\t MaliC55Params *params)\n> {\n> \tIPAActiveState &activeState = context.activeState;\n> \tdouble gain;\n> @@ -252,52 +252,50 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex\n> \telse\n> \t\tgain = activeState.agc.manual.ispGain;\n> \n> -\tblock.header->type = MALI_C55_PARAM_BLOCK_DIGITAL_GAIN;\n> -\tblock.header->flags = 0;\n> -\tblock.header->size = sizeof(struct mali_c55_params_digital_gain);\n> +\tauto block = params->block();\n> +\tblock->gain = floatingToFixedPoint<5, 8, uint16_t, double>(gain);\n> \n> -\tblock.digital_gain->gain = floatingToFixedPoint<5, 8, uint16_t, double>(gain);\n> \tframeContext.agc.ispGain = gain;\n> -\n> -\treturn block.header->size;\n> }\n> \n> -size_t Agc::fillParamsBuffer(mali_c55_params_block block,\n> -\t\t\t enum mali_c55_param_block_type type)\n> +void Agc::fillParamsBuffer(MaliC55Params *params, enum MaliC55Blocks type)\n> {\n> -\tblock.header->type = type;\n> -\tblock.header->flags = 0;\n> -\tblock.header->size = sizeof(struct mali_c55_params_aexp_hist);\n> +\n> +\tassert(type == MaliC55Blocks::AexpHist || type == MaliC55Blocks::AexpIhist);\n> +\n> +\tauto block = type == MaliC55Blocks::AexpHist ?\n> +\t\t\tparams->block() :\n> +\t\t\tparams->block();\n> \n> \t/* Collect every 3rd pixel horizontally */\n> -\tblock.aexp_hist->skip_x = 1;\n> +\tblock->skip_x = 1;\n> \t/* Start from first column */\n> -\tblock.aexp_hist->offset_x = 0;\n> +\tblock->offset_x = 0;\n> \t/* Collect every pixel vertically */\n> -\tblock.aexp_hist->skip_y = 0;\n> +\tblock->skip_y = 0;\n> \t/* Start from the first row */\n> -\tblock.aexp_hist->offset_y = 0;\n> +\tblock->offset_y = 0;\n> \t/* 1x scaling (i.e. none) */\n> -\tblock.aexp_hist->scale_bottom = 0;\n> -\tblock.aexp_hist->scale_top = 0;\n> +\tblock->scale_bottom = 0;\n> +\tblock->scale_top = 0;\n> \t/* Collect all Bayer planes into 4 separate histograms */\n> -\tblock.aexp_hist->plane_mode = 1;\n> +\tblock->plane_mode = 1;\n> \t/* Tap the data immediately after the digital gain block */\n> -\tblock.aexp_hist->tap_point = MALI_C55_AEXP_HIST_TAP_FS;\n> -\n> -\treturn block.header->size;\n> +\tblock->tap_point = MALI_C55_AEXP_HIST_TAP_FS;\n> }\n> \n> -size_t Agc::fillWeightsArrayBuffer(mali_c55_params_block block,\n> -\t\t\t\t enum mali_c55_param_block_type type)\n> +void Agc::fillWeightsArrayBuffer(MaliC55Params *params, const enum MaliC55Blocks type)\n> {\n> -\tblock.header->type = type;\n> -\tblock.header->flags = 0;\n> -\tblock.header->size = sizeof(struct mali_c55_params_aexp_weights);\n> +\tassert(type == MaliC55Blocks::AexpHistWeights ||\n> +\t type == MaliC55Blocks::AexpIhistWeights);\n> +\n> +\tauto block = type == MaliC55Blocks::AexpHistWeights ?\n> +\t\t\tparams->block() :\n> +\t\t\tparams->block();\n> \n> \t/* We use every zone - a 15x15 grid */\n> -\tblock.aexp_weights->nodes_used_horiz = 15;\n> -\tblock.aexp_weights->nodes_used_vert = 15;\n> +\tblock->nodes_used_horiz = 15;\n> +\tblock->nodes_used_vert = 15;\n> \n> \t/*\n> \t * We uniformly weight the zones to 1 - this results in the collected\n> @@ -305,40 +303,25 @@ size_t Agc::fillWeightsArrayBuffer(mali_c55_params_block block,\n> \t * approximate colour channel averages for the image.\n> \t */\n> \tSpan weights{\n> -\t\tblock.aexp_weights->zone_weights,\n> +\t\tblock->zone_weights,\n> \t\tMALI_C55_MAX_ZONES\n> \t};\n> \tstd::fill(weights.begin(), weights.end(), 1);\n> -\n> -\treturn block.header->size;\n> }\n> \n> void Agc::prepare(IPAContext &context, const uint32_t frame,\n> -\t\t IPAFrameContext &frameContext, v4l2_isp_params_buffer *params)\n> +\t\t IPAFrameContext &frameContext, MaliC55Params *params)\n> {\n> -\tmali_c55_params_block block;\n> -\n> -\tblock.data = ¶ms->data[params->data_size];\n> -\tparams->data_size += fillGainParamBlock(context, frameContext, block);\n> +\tfillGainParamBlock(context, frameContext, params);\n> \n> \tif (frame > 0)\n> \t\treturn;\n> \n> -\tblock.data = ¶ms->data[params->data_size];\n> -\tparams->data_size += fillParamsBuffer(block,\n> -\t\t\t\t\t MALI_C55_PARAM_BLOCK_AEXP_HIST);\n> -\n> -\tblock.data = ¶ms->data[params->data_size];\n> -\tparams->data_size += fillWeightsArrayBuffer(block,\n> -\t\t\t\t\t\t MALI_C55_PARAM_BLOCK_AEXP_HIST_WEIGHTS);\n> -\n> -\tblock.data = ¶ms->data[params->data_size];\n> -\tparams->data_size += fillParamsBuffer(block,\n> -\t\t\t\t\t MALI_C55_PARAM_BLOCK_AEXP_IHIST);\n> +\tfillParamsBuffer(params, MaliC55Blocks::AexpHist);\n> +\tfillWeightsArrayBuffer(params, MaliC55Blocks::AexpHistWeights);\n> \n> -\tblock.data = ¶ms->data[params->data_size];\n> -\tparams->data_size += fillWeightsArrayBuffer(block,\n> -\t\t\t\t\t\t MALI_C55_PARAM_BLOCK_AEXP_IHIST_WEIGHTS);\n> +\tfillParamsBuffer(params, MaliC55Blocks::AexpIhist);\n> +\tfillWeightsArrayBuffer(params, MaliC55Blocks::AexpIhistWeights);\n> }\n> \n> double Agc::estimateLuminance(const double gain) const\n> diff --git a/src/ipa/mali-c55/algorithms/agc.h b/src/ipa/mali-c55/algorithms/agc.h\n> index 4325ef5a9b7dcef36107b64a65db993f194d4167..9684fff664bc67d287bb00f8dc88e238d8dd0cea 100644\n> --- a/src/ipa/mali-c55/algorithms/agc.h\n> +++ b/src/ipa/mali-c55/algorithms/agc.h\n> @@ -57,7 +57,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 v4l2_isp_params_buffer *params) override;\n> +\t\t MaliC55Params *params) override;\n> \tvoid process(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t const mali_c55_stats_buffer *stats,\n> @@ -65,13 +65,11 @@ public:\n> \n> private:\n> \tdouble estimateLuminance(const double gain) const override;\n> -\tsize_t fillGainParamBlock(IPAContext &context,\n> -\t\t\t\t IPAFrameContext &frameContext,\n> -\t\t\t\t mali_c55_params_block block);\n> -\tsize_t fillParamsBuffer(mali_c55_params_block block,\n> -\t\t\t\tenum mali_c55_param_block_type type);\n> -\tsize_t fillWeightsArrayBuffer(mali_c55_params_block block,\n> -\t\t\t\t enum mali_c55_param_block_type type);\n> +\tvoid fillGainParamBlock(IPAContext &context,\n> +\t\t\t\tIPAFrameContext &frameContext,\n> +\t\t\t\tMaliC55Params *params);\n> +\tvoid fillParamsBuffer(MaliC55Params *params, enum MaliC55Blocks type);\n> +\tvoid fillWeightsArrayBuffer(MaliC55Params *params, enum MaliC55Blocks type);\n> \n> \tAgcStatistics statistics_;\n> };\n> diff --git a/src/ipa/mali-c55/algorithms/awb.cpp b/src/ipa/mali-c55/algorithms/awb.cpp\n> index 694c0aaa9c6804bb58e380ba9c744f11c39224fe..964e810882a93cce02f991675d74bbf163d51e7c 100644\n> --- a/src/ipa/mali-c55/algorithms/awb.cpp\n> +++ b/src/ipa/mali-c55/algorithms/awb.cpp\n> @@ -43,13 +43,9 @@ int Awb::configure([[maybe_unused]] IPAContext &context,\n> \treturn 0;\n> }\n> \n> -size_t Awb::fillGainsParamBlock(mali_c55_params_block block, IPAContext &context,\n> +void Awb::fillGainsParamBlock(MaliC55Params *params, IPAContext &context,\n> \t\t\t\tIPAFrameContext &frameContext)\n> {\n> -\tblock.header->type = MALI_C55_PARAM_BLOCK_AWB_GAINS;\n> -\tblock.header->flags = 0;\n> -\tblock.header->size = sizeof(struct mali_c55_params_awb_gains);\n> -\n> \tdouble rGain = context.activeState.awb.rGain;\n> \tdouble bGain = context.activeState.awb.bGain;\n> \n> @@ -63,34 +59,32 @@ size_t Awb::fillGainsParamBlock(mali_c55_params_block block, IPAContext &context\n> \t * This holds true regardless of the bayer order of the input data, as\n> \t * the mapping is done internally in the ISP.\n> \t */\n> -\tblock.awb_gains->gain00 = floatingToFixedPoint<4, 8, uint16_t, double>(rGain);\n> -\tblock.awb_gains->gain01 = floatingToFixedPoint<4, 8, uint16_t, double>(1.0);\n> -\tblock.awb_gains->gain10 = floatingToFixedPoint<4, 8, uint16_t, double>(1.0);\n> -\tblock.awb_gains->gain11 = floatingToFixedPoint<4, 8, uint16_t, double>(bGain);\n> +\tauto block = params->block();\n> +\n> +\tblock->gain00 = floatingToFixedPoint<4, 8, uint16_t, double>(rGain);\n> +\tblock->gain01 = floatingToFixedPoint<4, 8, uint16_t, double>(1.0);\n> +\tblock->gain10 = floatingToFixedPoint<4, 8, uint16_t, double>(1.0);\n> +\tblock->gain11 = floatingToFixedPoint<4, 8, uint16_t, double>(bGain);\n> \n> \tframeContext.awb.rGain = rGain;\n> \tframeContext.awb.bGain = bGain;\n> -\n> -\treturn sizeof(struct mali_c55_params_awb_gains);\n> }\n> \n> -size_t Awb::fillConfigParamBlock(mali_c55_params_block block)\n> +void Awb::fillConfigParamBlock(MaliC55Params *params)\n> {\n> -\tblock.header->type = MALI_C55_PARAM_BLOCK_AWB_CONFIG;\n> -\tblock.header->flags = 0;\n> -\tblock.header->size = sizeof(struct mali_c55_params_awb_config);\n> +\tauto block = params->block();\n> \n> \t/* Tap the stats after the purple fringe block */\n> -\tblock.awb_config->tap_point = MALI_C55_AWB_STATS_TAP_PF;\n> +\tblock->tap_point = MALI_C55_AWB_STATS_TAP_PF;\n> \n> \t/* Get R/G and B/G ratios as statistics */\n> -\tblock.awb_config->stats_mode = MALI_C55_AWB_MODE_RGBG;\n> +\tblock->stats_mode = MALI_C55_AWB_MODE_RGBG;\n> \n> \t/* Default white level */\n> -\tblock.awb_config->white_level = 1023;\n> +\tblock->white_level = 1023;\n> \n> \t/* Default black level */\n> -\tblock.awb_config->black_level = 0;\n> +\tblock->black_level = 0;\n> \n> \t/*\n> \t * By default pixels are included who's colour ratios are bounded in a\n> @@ -104,40 +98,34 @@ size_t Awb::fillConfigParamBlock(mali_c55_params_block block)\n> \t *\n> \t * \\todo should these perhaps be tunable?\n> \t */\n> -\tblock.awb_config->cr_max = 511;\n> -\tblock.awb_config->cr_min = 64;\n> -\tblock.awb_config->cb_max = 511;\n> -\tblock.awb_config->cb_min = 64;\n> +\tblock->cr_max = 511;\n> +\tblock->cr_min = 64;\n> +\tblock->cb_max = 511;\n> +\tblock->cb_min = 64;\n> \n> \t/* We use the full 15x15 zoning scheme */\n> -\tblock.awb_config->nodes_used_horiz = 15;\n> -\tblock.awb_config->nodes_used_vert = 15;\n> +\tblock->nodes_used_horiz = 15;\n> +\tblock->nodes_used_vert = 15;\n> \n> \t/*\n> \t * We set the trimming boundaries equivalent to the main boundaries. In\n> \t * other words; no trimming.\n> \t */\n> -\tblock.awb_config->cr_high = 511;\n> -\tblock.awb_config->cr_low = 64;\n> -\tblock.awb_config->cb_high = 511;\n> -\tblock.awb_config->cb_low = 64;\n> -\n> -\treturn sizeof(struct mali_c55_params_awb_config);\n> +\tblock->cr_high = 511;\n> +\tblock->cr_low = 64;\n> +\tblock->cb_high = 511;\n> +\tblock->cb_low = 64;\n> }\n> \n> void Awb::prepare(IPAContext &context, const uint32_t frame,\n> -\t\t IPAFrameContext &frameContext, v4l2_isp_params_buffer *params)\n> +\t\t IPAFrameContext &frameContext, MaliC55Params *params)\n> {\n> -\tmali_c55_params_block block;\n> -\tblock.data = ¶ms->data[params->data_size];\n> -\n> -\tparams->data_size += fillGainsParamBlock(block, context, frameContext);\n> +\tfillGainsParamBlock(params, context, frameContext);\n> \n> \tif (frame > 0)\n> \t\treturn;\n> \n> -\tblock.data = ¶ms->data[params->data_size];\n> -\tparams->data_size += fillConfigParamBlock(block);\n> +\tfillConfigParamBlock(params);\n> }\n> \n> void Awb::process(IPAContext &context, const uint32_t frame,\n> diff --git a/src/ipa/mali-c55/algorithms/awb.h b/src/ipa/mali-c55/algorithms/awb.h\n> index 647525ff700e8281e3cce63e64ba56a91294bcc2..683a62af263a14d2f5d5b261448953ada6669b2f 100644\n> --- a/src/ipa/mali-c55/algorithms/awb.h\n> +++ b/src/ipa/mali-c55/algorithms/awb.h\n> @@ -22,17 +22,17 @@ public:\n> \t\t const IPACameraSensorInfo &configInfo) override;\n> \tvoid prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> -\t\t v4l2_isp_params_buffer *params) override;\n> +\t\t MaliC55Params *params) override;\n> \tvoid process(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t const mali_c55_stats_buffer *stats,\n> \t\t ControlList &metadata) override;\n> \n> private:\n> -\tsize_t fillGainsParamBlock(mali_c55_params_block block,\n> -\t\t\t\t IPAContext &context,\n> -\t\t\t\t IPAFrameContext &frameContext);\n> -\tsize_t fillConfigParamBlock(mali_c55_params_block block);\n> +\tvoid fillGainsParamBlock(MaliC55Params *params,\n> +\t\t\t\t IPAContext &context,\n> +\t\t\t\t IPAFrameContext &frameContext);\n> +\tvoid fillConfigParamBlock(MaliC55Params *params);\n> };\n> \n> } /* namespace ipa::mali_c55::algorithms */\n> diff --git a/src/ipa/mali-c55/algorithms/blc.cpp b/src/ipa/mali-c55/algorithms/blc.cpp\n> index 543ba96cb57ac0cca2b8f822180d8d8b42f21fc7..d099219c3e43ec96fa452ed13fa46ada2025edc9 100644\n> --- a/src/ipa/mali-c55/algorithms/blc.cpp\n> +++ b/src/ipa/mali-c55/algorithms/blc.cpp\n> @@ -85,27 +85,19 @@ int BlackLevelCorrection::configure(IPAContext &context,\n> void BlackLevelCorrection::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 v4l2_isp_params_buffer *params)\n> +\t\t\t\t MaliC55Params *params)\n> {\n> -\tmali_c55_params_block block;\n> -\tblock.data = ¶ms->data[params->data_size];\n> -\n> \tif (frame > 0)\n> \t\treturn;\n> \n> \tif (!tuningParameters_)\n> \t\treturn;\n> \n> -\tblock.header->type = MALI_C55_PARAM_BLOCK_SENSOR_OFFS;\n> -\tblock.header->flags = 0;\n> -\tblock.header->size = sizeof(mali_c55_params_sensor_off_preshading);\n> -\n> -\tblock.sensor_offs->chan00 = offset00;\n> -\tblock.sensor_offs->chan01 = offset01;\n> -\tblock.sensor_offs->chan10 = offset10;\n> -\tblock.sensor_offs->chan11 = offset11;\n> -\n> -\tparams->data_size += block.header->size;\n> +\tauto block = params->block();\n> +\tblock->chan00 = offset00;\n> +\tblock->chan01 = offset01;\n> +\tblock->chan10 = offset10;\n> +\tblock->chan11 = offset11;\n> }\n> \n> void BlackLevelCorrection::process([[maybe_unused]] IPAContext &context,\n> diff --git a/src/ipa/mali-c55/algorithms/blc.h b/src/ipa/mali-c55/algorithms/blc.h\n> index ee6c889ed89ac2230b231cb58a9fe4412c0ce164..fc5a7ea310cbdbcf271eb26c73b17243aea744cd 100644\n> --- a/src/ipa/mali-c55/algorithms/blc.h\n> +++ b/src/ipa/mali-c55/algorithms/blc.h\n> @@ -6,6 +6,7 @@\n> */\n> \n> #include \"algorithm.h\"\n> +#include \"params.h\"\n> \n> namespace libcamera {\n> \n> @@ -22,7 +23,7 @@ public:\n> \t\t const IPACameraSensorInfo &configInfo) override;\n> \tvoid prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> -\t\t v4l2_isp_params_buffer *params) override;\n> +\t\t MaliC55Params *params) override;\n> \tvoid process(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext,\n> \t\t const mali_c55_stats_buffer *stats,\n> diff --git a/src/ipa/mali-c55/algorithms/lsc.cpp b/src/ipa/mali-c55/algorithms/lsc.cpp\n> index cb915c5efd3c22952035f2b03ee659f293942ec9..5b042c757bc70fe4eb4a5aaa848266b3afce9100 100644\n> --- a/src/ipa/mali-c55/algorithms/lsc.cpp\n> +++ b/src/ipa/mali-c55/algorithms/lsc.cpp\n> @@ -108,41 +108,33 @@ int Lsc::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> \treturn 0;\n> }\n> \n> -size_t Lsc::fillConfigParamsBlock(mali_c55_params_block block) const\n> +void Lsc::fillConfigParamsBlock(MaliC55Params *params) const\n> {\n> -\tblock.header->type = MALI_C55_PARAM_MESH_SHADING_CONFIG;\n> -\tblock.header->flags = 0;\n> -\tblock.header->size = sizeof(struct mali_c55_params_mesh_shading_config);\n> +\tauto block = params->block();\n> \n> -\tblock.shading_config->mesh_show = false;\n> -\tblock.shading_config->mesh_scale = meshScale_;\n> -\tblock.shading_config->mesh_page_r = 0;\n> -\tblock.shading_config->mesh_page_g = 1;\n> -\tblock.shading_config->mesh_page_b = 2;\n> -\tblock.shading_config->mesh_width = meshSize_;\n> -\tblock.shading_config->mesh_height = meshSize_;\n> +\tblock->mesh_show = false;\n> +\tblock->mesh_scale = meshScale_;\n> +\tblock->mesh_page_r = 0;\n> +\tblock->mesh_page_g = 1;\n> +\tblock->mesh_page_b = 2;\n> +\tblock->mesh_width = meshSize_;\n> +\tblock->mesh_height = meshSize_;\n> \n> -\tstd::copy(mesh_.begin(), mesh_.end(), block.shading_config->mesh);\n> -\n> -\treturn block.header->size;\n> +\tstd::copy(mesh_.begin(), mesh_.end(), block->mesh);\n> }\n> \n> -size_t Lsc::fillSelectionParamsBlock(mali_c55_params_block block, uint8_t bank,\n> +void Lsc::fillSelectionParamsBlock(MaliC55Params *params, uint8_t bank,\n> \t\t\t\t uint8_t alpha) const\n> {\n> -\tblock.header->type = MALI_C55_PARAM_MESH_SHADING_SELECTION;\n> -\tblock.header->flags = 0;\n> -\tblock.header->size = sizeof(struct mali_c55_params_mesh_shading_selection);\n> -\n> -\tblock.shading_selection->mesh_alpha_bank_r = bank;\n> -\tblock.shading_selection->mesh_alpha_bank_g = bank;\n> -\tblock.shading_selection->mesh_alpha_bank_b = bank;\n> -\tblock.shading_selection->mesh_alpha_r = alpha;\n> -\tblock.shading_selection->mesh_alpha_g = alpha;\n> -\tblock.shading_selection->mesh_alpha_b = alpha;\n> -\tblock.shading_selection->mesh_strength = 0x1000; /* Otherwise known as 1.0 */\n> -\n> -\treturn block.header->size;\n> +\tauto block = params->block();\n> +\n> +\tblock->mesh_alpha_bank_r = bank;\n> +\tblock->mesh_alpha_bank_g = bank;\n> +\tblock->mesh_alpha_bank_b = bank;\n> +\tblock->mesh_alpha_r = alpha;\n> +\tblock->mesh_alpha_g = alpha;\n> +\tblock->mesh_alpha_b = alpha;\n> +\tblock->mesh_strength = 0x1000; /* Otherwise known as 1.0 */\n> }\n> \n> std::tuple Lsc::findBankAndAlpha(uint32_t ct) const\n> @@ -170,7 +162,7 @@ std::tuple Lsc::findBankAndAlpha(uint32_t ct) const\n> \n> void Lsc::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> -\t\t v4l2_isp_params_buffer *params)\n> +\t\t MaliC55Params *params)\n> {\n> \t/*\n> \t * For each frame we assess the colour temperature of the **last** frame\n> @@ -193,10 +185,7 @@ void Lsc::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\tstd::tie(bank, alpha) = findBankAndAlpha(temperatureK);\n> \t}\n> \n> -\tmali_c55_params_block block;\n> -\tblock.data = ¶ms->data[params->data_size];\n> -\n> -\tparams->data_size += fillSelectionParamsBlock(block, bank, alpha);\n> +\tfillSelectionParamsBlock(params, bank, alpha);\n> \n> \tif (frame > 0)\n> \t\treturn;\n> @@ -205,8 +194,7 @@ void Lsc::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t * If this is the first frame, we need to load the parsed coefficient\n> \t * tables from tuning data to the ISP.\n> \t */\n> -\tblock.data = ¶ms->data[params->data_size];\n> -\tparams->data_size += fillConfigParamsBlock(block);\n> +\tfillConfigParamsBlock(params);\n> }\n> \n> REGISTER_IPA_ALGORITHM(Lsc, \"Lsc\")\n> diff --git a/src/ipa/mali-c55/algorithms/lsc.h b/src/ipa/mali-c55/algorithms/lsc.h\n> index c287900502528ca82ab6c62d732b6ffc28ad8df6..e7092bc74a0b0301463167d16e1e888696547d10 100644\n> --- a/src/ipa/mali-c55/algorithms/lsc.h\n> +++ b/src/ipa/mali-c55/algorithms/lsc.h\n> @@ -23,15 +23,15 @@ 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 v4l2_isp_params_buffer *params) override;\n> +\t\t MaliC55Params *params) override;\n> private:\n> \tstatic constexpr unsigned int kRedOffset = 0;\n> \tstatic constexpr unsigned int kGreenOffset = 1024;\n> \tstatic constexpr unsigned int kBlueOffset = 2048;\n> \n> -\tsize_t fillConfigParamsBlock(mali_c55_params_block block) const;\n> -\tsize_t fillSelectionParamsBlock(mali_c55_params_block block,\n> -\t\t\t\t\tuint8_t bank, uint8_t alpha) const;\n> +\tvoid fillConfigParamsBlock(MaliC55Params *params) const;\n> +\tvoid fillSelectionParamsBlock(MaliC55Params *params,\n> +\t\t\t\t uint8_t bank, uint8_t alpha) const;\n> \tstd::tuple findBankAndAlpha(uint32_t ct) const;\n> \n> \tstd::vector mesh_ = std::vector(3072);\n> diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp\n> index 504d95222074a35b033949a25178f998738e5699..fe36d4445c3ac75abe48dbe2335fa1f3aded9609 100644\n> --- a/src/ipa/mali-c55/mali-c55.cpp\n> +++ b/src/ipa/mali-c55/mali-c55.cpp\n> @@ -28,6 +28,7 @@\n> #include \"libipa/camera_sensor_helper.h\"\n> \n> #include \"ipa_context.h\"\n> +#include \"params.h\"\n> \n> namespace libcamera {\n> \n> @@ -331,24 +332,13 @@ void IPAMaliC55::queueRequest(const uint32_t request, const ControlList &control\n> void IPAMaliC55::fillParams(unsigned int request,\n> \t\t\t [[maybe_unused]] uint32_t bufferId)\n> {\n> -\tstruct v4l2_isp_params_buffer *params;\n> \tIPAFrameContext &frameContext = context_.frameContexts.get(request);\n> +\tMaliC55Params params(buffers_.at(bufferId).planes()[0]);\n> \n> -\tparams = reinterpret_cast(\n> -\t\tbuffers_.at(bufferId).planes()[0].data());\n> -\tmemset(params, 0,\n> -\t buffers_.at(bufferId).planes()[0].size());\n> -\n> -\tparams->version = MALI_C55_PARAM_BUFFER_V1;\n> -\n> -\tfor (auto const &algo : algorithms()) {\n> -\t\talgo->prepare(context_, request, frameContext, params);\n> -\n> -\t\tASSERT(params->data_size <= MALI_C55_PARAMS_MAX_SIZE);\n> -\t}\n> +\tfor (auto const &algo : algorithms())\n> +\t\talgo->prepare(context_, request, frameContext, ¶ms);\n> \n> -\tsize_t bytesused = offsetof(struct v4l2_isp_params_buffer, data) + params->data_size;\n> -\tparamsComputed.emit(request, bytesused);\n> +\tparamsComputed.emit(request, params.size());\n\nI think this reinforces my view that size() should be called used() (or even maybe bytesused!)> }\n> \n> void IPAMaliC55::processStats(unsigned int request, unsigned int bufferId,\n> diff --git a/src/ipa/mali-c55/module.h b/src/ipa/mali-c55/module.h\n> index a8dcb20841a4ebd7bde22cae4ff1f8eef530d8bb..13b34eb2839530a3518340165e6ca895d0f6bcaf 100644\n> --- a/src/ipa/mali-c55/module.h\n> +++ b/src/ipa/mali-c55/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::mali_c55 {\n> \n> using Module = ipa::Module -\t\t\t v4l2_isp_params_buffer, mali_c55_stats_buffer>;\n> +\t\t\t MaliC55Params, mali_c55_stats_buffer>;\n> \n> } /* namespace ipa::mali_c55 */\n> \n> diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h\n> new file mode 100644\n> index 0000000000000000000000000000000000000000..bb26da19a8f1336eeed91989a6c13e7630c6e5f4\n> --- /dev/null\n> +++ b/src/ipa/mali-c55/params.h\n> @@ -0,0 +1,83 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board\n> + *\n> + * Mali C55 ISP Parameters\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::mali_c55 {\n> +\n> +enum class MaliC55Blocks {\n> +\tBls,\n> +\tAexpHist,\n> +\tAexpHistWeights,\n> +\tAexpIhist,\n> +\tAexpIhistWeights,\n> +\tDgain,\n> +\tAwbGains,\n> +\tAwbConfig,\n> +\tMeshShadingConfig,\n> +\tMeshShadingSel,\n> +};\n> +\n> +namespace details {\n> +\n> +template\n> +struct block_type {\n> +};\n> +\n> +#define MALI_C55_DEFINE_BLOCK_TYPE(id, cfgType, blkType)\t\t\\\n> +template<>\t\t\t\t\t\t\t\t\\\n> +struct block_type {\t\t\t\t\t\\\n> +\tusing type = struct mali_c55_params_##cfgType;\t\t\t\\\n> +\tstatic constexpr mali_c55_param_block_type blockType = \t\t\\\n> +\t\tmali_c55_param_block_type::MALI_C55_PARAM_##blkType;\t\\\n> +};\n\nI think dropping the semicolon here and having it on the ends of the lines below would be more in \nline with what I'd expect to see.\n\n> +\n> +MALI_C55_DEFINE_BLOCK_TYPE(Bls, sensor_off_preshading, BLOCK_SENSOR_OFFS)\n> +MALI_C55_DEFINE_BLOCK_TYPE(AexpHist, aexp_hist, BLOCK_AEXP_HIST)\n> +MALI_C55_DEFINE_BLOCK_TYPE(AexpHistWeights, aexp_weights,\n> +\t\t\t BLOCK_AEXP_HIST_WEIGHTS)\n> +MALI_C55_DEFINE_BLOCK_TYPE(AexpIhist, aexp_hist, BLOCK_AEXP_IHIST)\n> +MALI_C55_DEFINE_BLOCK_TYPE(AexpIhistWeights, aexp_weights,\n> +\t\t\t BLOCK_AEXP_IHIST_WEIGHTS)\n> +MALI_C55_DEFINE_BLOCK_TYPE(Dgain, digital_gain, BLOCK_DIGITAL_GAIN)\n> +MALI_C55_DEFINE_BLOCK_TYPE(AwbGains, awb_gains, BLOCK_AWB_GAINS)\n> +MALI_C55_DEFINE_BLOCK_TYPE(AwbConfig, awb_config, BLOCK_AWB_CONFIG)\n> +MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig, mesh_shading_config,\n> +\t\t\t MESH_SHADING_CONFIG)\n> +MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel, mesh_shading_selection,\n> +\t\t\t MESH_SHADING_SELECTION)\n\nPersonally I think it would be better to format these so that the fields for each entry are more \nvisibly associated:\n\nMALI_C55_DEFINE_BLOCK_TYPE(Bls,\t\t\tsensor_off_preshading,\tBLOCK_SENSOR_OFFS);\nMALI_C55_DEFINE_BLOCK_TYPE(AexpHist,\t\taexp_hist,\t\tBLOCK_AEXP_HIST);\nMALI_C55_DEFINE_BLOCK_TYPE(AexpHistWeights,\taexp_weights,\t\tBLOCK_AEXP_HIST_WEIGHTS);\nMALI_C55_DEFINE_BLOCK_TYPE(AexpIhist,\t\taexp_hist,\t\tBLOCK_AEXP_IHIST);\nMALI_C55_DEFINE_BLOCK_TYPE(AexpIhistWeights,\taexp_weights,\t\tBLOCK_AEXP_IHIST_WEIGHTS);\nMALI_C55_DEFINE_BLOCK_TYPE(Dgain,\t\tdigital_gain,\t\tBLOCK_DIGITAL_GAIN);\nMALI_C55_DEFINE_BLOCK_TYPE(AwbGains,\t\tawb_gains,\t\tBLOCK_AWB_GAINS);\nMALI_C55_DEFINE_BLOCK_TYPE(AwbConfig,\t\tawb_config,\t\tBLOCK_AWB_CONFIG);\nMALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig,\tmesh_shading_config,\tMESH_SHADING_CONFIG)\nMALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel,\tmesh_shading_selection,\tMESH_SHADING_SELECTION);\n\nIt's longer, but I think it's a much easier to read style for this kind of thing.\n\nThanks\nDan\n\n> +\n> +struct param_traits {\n> +\tusing id_type = MaliC55Blocks;\n> +\n> +\ttemplate\n> +\tusing id_to_details = block_type;\n> +};\n> +\n> +} /* namespace details */\n> +\n> +class MaliC55Params : public V4L2Params\n> +{\n> +public:\n> +\tstatic constexpr unsigned int kVersion = MALI_C55_PARAM_BUFFER_V1;\n> +\n> +\tMaliC55Params(Span data)\n> +\t\t: V4L2Params(data, kVersion)\n> +\t{\n> +\t}\n> +};\n> +\n> +} /* namespace ipa::mali_c55 */\n> +\n> +} /* namespace libcamera */\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 E2495BE080\n\tfor ;\n\tFri, 10 Oct 2025 15:16:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D42B6B611;\n\tFri, 10 Oct 2025 17:16:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BACF26B599\n\tfor ;\n\tFri, 10 Oct 2025 17:16:40 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 31EC51807\n\tfor ;\n\tFri, 10 Oct 2025 17:15:05 +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=\"uDHTDcuf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760109305;\n\tbh=98e+4nUX09o0+LzRuJ/zygXc0Gu0z/8MCVhS8Itnv+A=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=uDHTDcufhvtDLNHXB79vUVZVZGIrMpynUZ80fiUh6NYVCXZS+gE+taKd6cApO84e7\n\teWJM9EUWbXis9ggkIaYLyBldQQ3JjrKxJu39cWWJC2owx2/+AspINzkaFBMuJ1Ifry\n\tlfnk3t9ogD1Tr2QH5xe78+h2dxWC1axalaYL3azc=","Message-ID":"","Date":"Fri, 10 Oct 2025 16:16:37 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 5/5] ipa: mali-c55: Introduce MaliC55Params","To":"libcamera-devel@lists.libcamera.org","References":"<20251007-v4l2-params-v5-0-8db451a81398@ideasonboard.com>\n\t<20251007-v4l2-params-v5-5-8db451a81398@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","In-Reply-To":"<20251007-v4l2-params-v5-5-8db451a81398@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":36201,"web_url":"https://patchwork.libcamera.org/comment/36201/","msgid":"","date":"2025-10-10T16:17:17","subject":"Re: [PATCH v5 5/5] ipa: mali-c55: Introduce MaliC55Params","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dan\n\nOn Fri, Oct 10, 2025 at 04:16:37PM +0100, Dan Scally wrote:\n> Hi Jacopo\n>\n> On 07/10/2025 19:17, Jacopo Mondi wrote:\n> > Implement MaliC55Params to derive from V4L2Params and use the new\n> > helpers in the Mali C55 IPA algorithms implementation.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Tested-by: Antoine Bouyer \n>\n> Couple of nits below, but with or without them changed:\n>\n> Reviewed-by: Daniel Scally \n\nThanks\n\n>\n> > ---\n> > src/ipa/mali-c55/algorithms/agc.cpp | 87 +++++++++++++++----------------------\n> > src/ipa/mali-c55/algorithms/agc.h | 14 +++---\n\n[snip]\n\n> > --- a/src/ipa/mali-c55/mali-c55.cpp\n> > +++ b/src/ipa/mali-c55/mali-c55.cpp\n> > @@ -28,6 +28,7 @@\n> > #include \"libipa/camera_sensor_helper.h\"\n> > #include \"ipa_context.h\"\n> > +#include \"params.h\"\n> > namespace libcamera {\n> > @@ -331,24 +332,13 @@ void IPAMaliC55::queueRequest(const uint32_t request, const ControlList &control\n> > void IPAMaliC55::fillParams(unsigned int request,\n> > \t\t\t [[maybe_unused]] uint32_t bufferId)\n> > {\n> > -\tstruct v4l2_isp_params_buffer *params;\n> > \tIPAFrameContext &frameContext = context_.frameContexts.get(request);\n> > +\tMaliC55Params params(buffers_.at(bufferId).planes()[0]);\n> > -\tparams = reinterpret_cast(\n> > -\t\tbuffers_.at(bufferId).planes()[0].data());\n> > -\tmemset(params, 0,\n> > -\t buffers_.at(bufferId).planes()[0].size());\n> > -\n> > -\tparams->version = MALI_C55_PARAM_BUFFER_V1;\n> > -\n> > -\tfor (auto const &algo : algorithms()) {\n> > -\t\talgo->prepare(context_, request, frameContext, params);\n> > -\n> > -\t\tASSERT(params->data_size <= MALI_C55_PARAMS_MAX_SIZE);\n> > -\t}\n> > +\tfor (auto const &algo : algorithms())\n> > +\t\talgo->prepare(context_, request, frameContext, ¶ms);\n> > -\tsize_t bytesused = offsetof(struct v4l2_isp_params_buffer, data) + params->data_size;\n> > -\tparamsComputed.emit(request, bytesused);\n> > +\tparamsComputed.emit(request, params.size());\n>\n> I think this reinforces my view that size() should be called used() (or even maybe bytesused!)> }\n\nlet's use bytesused()\n\n> > void IPAMaliC55::processStats(unsigned int request, unsigned int bufferId,\n> > diff --git a/src/ipa/mali-c55/module.h b/src/ipa/mali-c55/module.h\n> > index a8dcb20841a4ebd7bde22cae4ff1f8eef530d8bb..13b34eb2839530a3518340165e6ca895d0f6bcaf 100644\n> > --- a/src/ipa/mali-c55/module.h\n> > +++ b/src/ipa/mali-c55/module.h\n> > @@ -14,13 +14,14 @@\n> > #include \n> > #include \"ipa_context.h\"\n> > +#include \"params.h\"\n> > namespace libcamera {\n> > namespace ipa::mali_c55 {\n> > using Module = ipa::Module > -\t\t\t v4l2_isp_params_buffer, mali_c55_stats_buffer>;\n> > +\t\t\t MaliC55Params, mali_c55_stats_buffer>;\n> > } /* namespace ipa::mali_c55 */\n> > diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h\n> > new file mode 100644\n> > index 0000000000000000000000000000000000000000..bb26da19a8f1336eeed91989a6c13e7630c6e5f4\n> > --- /dev/null\n> > +++ b/src/ipa/mali-c55/params.h\n> > @@ -0,0 +1,83 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2025, Ideas On Board\n> > + *\n> > + * Mali C55 ISP Parameters\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::mali_c55 {\n> > +\n> > +enum class MaliC55Blocks {\n> > +\tBls,\n> > +\tAexpHist,\n> > +\tAexpHistWeights,\n> > +\tAexpIhist,\n> > +\tAexpIhistWeights,\n> > +\tDgain,\n> > +\tAwbGains,\n> > +\tAwbConfig,\n> > +\tMeshShadingConfig,\n> > +\tMeshShadingSel,\n> > +};\n> > +\n> > +namespace details {\n> > +\n> > +template\n> > +struct block_type {\n> > +};\n> > +\n> > +#define MALI_C55_DEFINE_BLOCK_TYPE(id, cfgType, blkType)\t\t\\\n> > +template<>\t\t\t\t\t\t\t\t\\\n> > +struct block_type {\t\t\t\t\t\\\n> > +\tusing type = struct mali_c55_params_##cfgType;\t\t\t\\\n> > +\tstatic constexpr mali_c55_param_block_type blockType = \t\t\\\n> > +\t\tmali_c55_param_block_type::MALI_C55_PARAM_##blkType;\t\\\n> > +};\n>\n> I think dropping the semicolon here and having it on the ends of the lines\n> below would be more in line with what I'd expect to see.\n\nI'm not sure I got what you mean here :)\n\nThis ?\n\nstruct block_type {\t\t\t\t\t\\\n\tusing type = struct mali_c55_params_##cfgType;\t\t\t\\\n\tstatic constexpr mali_c55_param_block_type blockType = \t\t\\\n\t\tmali_c55_param_block_type::MALI_C55_PARAM_##blkType; }\n\n>\n> > +\n> > +MALI_C55_DEFINE_BLOCK_TYPE(Bls, sensor_off_preshading, BLOCK_SENSOR_OFFS)\n> > +MALI_C55_DEFINE_BLOCK_TYPE(AexpHist, aexp_hist, BLOCK_AEXP_HIST)\n> > +MALI_C55_DEFINE_BLOCK_TYPE(AexpHistWeights, aexp_weights,\n> > +\t\t\t BLOCK_AEXP_HIST_WEIGHTS)\n> > +MALI_C55_DEFINE_BLOCK_TYPE(AexpIhist, aexp_hist, BLOCK_AEXP_IHIST)\n> > +MALI_C55_DEFINE_BLOCK_TYPE(AexpIhistWeights, aexp_weights,\n> > +\t\t\t BLOCK_AEXP_IHIST_WEIGHTS)\n> > +MALI_C55_DEFINE_BLOCK_TYPE(Dgain, digital_gain, BLOCK_DIGITAL_GAIN)\n> > +MALI_C55_DEFINE_BLOCK_TYPE(AwbGains, awb_gains, BLOCK_AWB_GAINS)\n> > +MALI_C55_DEFINE_BLOCK_TYPE(AwbConfig, awb_config, BLOCK_AWB_CONFIG)\n> > +MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig, mesh_shading_config,\n> > +\t\t\t MESH_SHADING_CONFIG)\n> > +MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel, mesh_shading_selection,\n> > +\t\t\t MESH_SHADING_SELECTION)\n>\n> Personally I think it would be better to format these so that the fields for\n> each entry are more visibly associated:\n>\n> MALI_C55_DEFINE_BLOCK_TYPE(Bls,\t\t\tsensor_off_preshading,\tBLOCK_SENSOR_OFFS);\n> MALI_C55_DEFINE_BLOCK_TYPE(AexpHist,\t\taexp_hist,\t\tBLOCK_AEXP_HIST);\n> MALI_C55_DEFINE_BLOCK_TYPE(AexpHistWeights,\taexp_weights,\t\tBLOCK_AEXP_HIST_WEIGHTS);\n> MALI_C55_DEFINE_BLOCK_TYPE(AexpIhist,\t\taexp_hist,\t\tBLOCK_AEXP_IHIST);\n> MALI_C55_DEFINE_BLOCK_TYPE(AexpIhistWeights,\taexp_weights,\t\tBLOCK_AEXP_IHIST_WEIGHTS);\n> MALI_C55_DEFINE_BLOCK_TYPE(Dgain,\t\tdigital_gain,\t\tBLOCK_DIGITAL_GAIN);\n> MALI_C55_DEFINE_BLOCK_TYPE(AwbGains,\t\tawb_gains,\t\tBLOCK_AWB_GAINS);\n> MALI_C55_DEFINE_BLOCK_TYPE(AwbConfig,\t\tawb_config,\t\tBLOCK_AWB_CONFIG);\n> MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig,\tmesh_shading_config,\tMESH_SHADING_CONFIG)\n> MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel,\tmesh_shading_selection,\tMESH_SHADING_SELECTION);\n>\n> It's longer, but I think it's a much easier to read style for this kind of thing.\n\nMore readable, I agree. Let's see how loud checkstyle complains (I\nthink it already was complaining on this version).\n\nThanks\n j\n\n>\n> Thanks\n> Dan\n>\n> > +\n> > +struct param_traits {\n> > +\tusing id_type = MaliC55Blocks;\n> > +\n> > +\ttemplate\n> > +\tusing id_to_details = block_type;\n> > +};\n> > +\n> > +} /* namespace details */\n> > +\n> > +class MaliC55Params : public V4L2Params\n> > +{\n> > +public:\n> > +\tstatic constexpr unsigned int kVersion = MALI_C55_PARAM_BUFFER_V1;\n> > +\n> > +\tMaliC55Params(Span data)\n> > +\t\t: V4L2Params(data, kVersion)\n> > +\t{\n> > +\t}\n> > +};\n> > +\n> > +} /* namespace ipa::mali_c55 */\n> > +\n> > +} /* namespace libcamera */\n> >\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 E28B8BF415\n\tfor ;\n\tFri, 10 Oct 2025 16:17:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3A95D6B5C4;\n\tFri, 10 Oct 2025 18:17:23 +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 C8DBC6B599\n\tfor ;\n\tFri, 10 Oct 2025 18:17:21 +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 367B8EFE;\n\tFri, 10 Oct 2025 18:15:46 +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=\"qz23dLkr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760112946;\n\tbh=xdslm59zS1D51V5I0/nwd62WbQLoaKRvsuxCJUsk3RE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qz23dLkrK66BbLzmv6JeatM7s9hqcVwHIksQzgf3ydu02IoPij3x1TZbE/lE7iwU2\n\taZNSuBpM7eBycQQPQYqhmXOwzxRNp9NbyKESo0GtL7/OU1fYMzPxY24U8g+WvbJ6vb\n\tk4Qr719vTJAYtgKaDeVRwVfhOc0r/E8r0Y/XWmCg=","Date":"Fri, 10 Oct 2025 18:17:17 +0200","From":"Jacopo Mondi ","To":"Dan Scally ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 5/5] ipa: mali-c55: Introduce MaliC55Params","Message-ID":"","References":"<20251007-v4l2-params-v5-0-8db451a81398@ideasonboard.com>\n\t<20251007-v4l2-params-v5-5-8db451a81398@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":36202,"web_url":"https://patchwork.libcamera.org/comment/36202/","msgid":"","date":"2025-10-10T16:35:13","subject":"Re: [PATCH v5 5/5] ipa: mali-c55: Introduce MaliC55Params","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 10/10/2025 17:17, Jacopo Mondi wrote:\n> Hi Dan\n> \n> On Fri, Oct 10, 2025 at 04:16:37PM +0100, Dan Scally wrote:\n>> Hi Jacopo\n>>\n>> On 07/10/2025 19:17, Jacopo Mondi wrote:\n>>> Implement MaliC55Params to derive from V4L2Params and use the new\n>>> helpers in the Mali C55 IPA algorithms implementation.\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>> Tested-by: Antoine Bouyer \n>>\n>> Couple of nits below, but with or without them changed:\n>>\n>> Reviewed-by: Daniel Scally \n> \n> Thanks\n> \n>>\n>>> ---\n>>> src/ipa/mali-c55/algorithms/agc.cpp | 87 +++++++++++++++----------------------\n>>> src/ipa/mali-c55/algorithms/agc.h | 14 +++---\n> \n> [snip]\n> \n>>> --- a/src/ipa/mali-c55/mali-c55.cpp\n>>> +++ b/src/ipa/mali-c55/mali-c55.cpp\n>>> @@ -28,6 +28,7 @@\n>>> #include \"libipa/camera_sensor_helper.h\"\n>>> #include \"ipa_context.h\"\n>>> +#include \"params.h\"\n>>> namespace libcamera {\n>>> @@ -331,24 +332,13 @@ void IPAMaliC55::queueRequest(const uint32_t request, const ControlList &control\n>>> void IPAMaliC55::fillParams(unsigned int request,\n>>> \t\t\t [[maybe_unused]] uint32_t bufferId)\n>>> {\n>>> -\tstruct v4l2_isp_params_buffer *params;\n>>> \tIPAFrameContext &frameContext = context_.frameContexts.get(request);\n>>> +\tMaliC55Params params(buffers_.at(bufferId).planes()[0]);\n>>> -\tparams = reinterpret_cast(\n>>> -\t\tbuffers_.at(bufferId).planes()[0].data());\n>>> -\tmemset(params, 0,\n>>> -\t buffers_.at(bufferId).planes()[0].size());\n>>> -\n>>> -\tparams->version = MALI_C55_PARAM_BUFFER_V1;\n>>> -\n>>> -\tfor (auto const &algo : algorithms()) {\n>>> -\t\talgo->prepare(context_, request, frameContext, params);\n>>> -\n>>> -\t\tASSERT(params->data_size <= MALI_C55_PARAMS_MAX_SIZE);\n>>> -\t}\n>>> +\tfor (auto const &algo : algorithms())\n>>> +\t\talgo->prepare(context_, request, frameContext, ¶ms);\n>>> -\tsize_t bytesused = offsetof(struct v4l2_isp_params_buffer, data) + params->data_size;\n>>> -\tparamsComputed.emit(request, bytesused);\n>>> +\tparamsComputed.emit(request, params.size());\n>>\n>> I think this reinforces my view that size() should be called used() (or even maybe bytesused!)> }\n> \n> let's use bytesused()\n\nSounds good\n\n> \n>>> void IPAMaliC55::processStats(unsigned int request, unsigned int bufferId,\n>>> diff --git a/src/ipa/mali-c55/module.h b/src/ipa/mali-c55/module.h\n>>> index a8dcb20841a4ebd7bde22cae4ff1f8eef530d8bb..13b34eb2839530a3518340165e6ca895d0f6bcaf 100644\n>>> --- a/src/ipa/mali-c55/module.h\n>>> +++ b/src/ipa/mali-c55/module.h\n>>> @@ -14,13 +14,14 @@\n>>> #include \n>>> #include \"ipa_context.h\"\n>>> +#include \"params.h\"\n>>> namespace libcamera {\n>>> namespace ipa::mali_c55 {\n>>> using Module = ipa::Module>> -\t\t\t v4l2_isp_params_buffer, mali_c55_stats_buffer>;\n>>> +\t\t\t MaliC55Params, mali_c55_stats_buffer>;\n>>> } /* namespace ipa::mali_c55 */\n>>> diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h\n>>> new file mode 100644\n>>> index 0000000000000000000000000000000000000000..bb26da19a8f1336eeed91989a6c13e7630c6e5f4\n>>> --- /dev/null\n>>> +++ b/src/ipa/mali-c55/params.h\n>>> @@ -0,0 +1,83 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2025, Ideas On Board\n>>> + *\n>>> + * Mali C55 ISP Parameters\n>>> + */\n>>> +\n>>> +#pragma once\n>>> +\n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +namespace ipa::mali_c55 {\n>>> +\n>>> +enum class MaliC55Blocks {\n>>> +\tBls,\n>>> +\tAexpHist,\n>>> +\tAexpHistWeights,\n>>> +\tAexpIhist,\n>>> +\tAexpIhistWeights,\n>>> +\tDgain,\n>>> +\tAwbGains,\n>>> +\tAwbConfig,\n>>> +\tMeshShadingConfig,\n>>> +\tMeshShadingSel,\n>>> +};\n>>> +\n>>> +namespace details {\n>>> +\n>>> +template\n>>> +struct block_type {\n>>> +};\n>>> +\n>>> +#define MALI_C55_DEFINE_BLOCK_TYPE(id, cfgType, blkType)\t\t\\\n>>> +template<>\t\t\t\t\t\t\t\t\\\n>>> +struct block_type {\t\t\t\t\t\\\n>>> +\tusing type = struct mali_c55_params_##cfgType;\t\t\t\\\n>>> +\tstatic constexpr mali_c55_param_block_type blockType = \t\t\\\n>>> +\t\tmali_c55_param_block_type::MALI_C55_PARAM_##blkType;\t\\\n>>> +};\n>>\n>> I think dropping the semicolon here and having it on the ends of the lines\n>> below would be more in line with what I'd expect to see.\n> \n> I'm not sure I got what you mean here :)\n\nInstead of \"};\" at the end of the definition of the macro have \"}\" so that we do...\n\nMALI_C55_DEFINE_BLOCK_TYPE(Bls, sensor_off_preshading, BLOCK_SENSOR_OFFS);\n\n...instead of...\n\nMALI_C55_DEFINE_BLOCK_TYPE(Bls, sensor_off_preshading, BLOCK_SENSOR_OFFS)\n\nBut it doesn't really matter :)\n\n> \n> This ?\n> \n> struct block_type {\t\t\t\t\t\\\n> \tusing type = struct mali_c55_params_##cfgType;\t\t\t\\\n> \tstatic constexpr mali_c55_param_block_type blockType = \t\t\\\n> \t\tmali_c55_param_block_type::MALI_C55_PARAM_##blkType; }\n> \n>>\n>>> +\n>>> +MALI_C55_DEFINE_BLOCK_TYPE(Bls, sensor_off_preshading, BLOCK_SENSOR_OFFS)\n>>> +MALI_C55_DEFINE_BLOCK_TYPE(AexpHist, aexp_hist, BLOCK_AEXP_HIST)\n>>> +MALI_C55_DEFINE_BLOCK_TYPE(AexpHistWeights, aexp_weights,\n>>> +\t\t\t BLOCK_AEXP_HIST_WEIGHTS)\n>>> +MALI_C55_DEFINE_BLOCK_TYPE(AexpIhist, aexp_hist, BLOCK_AEXP_IHIST)\n>>> +MALI_C55_DEFINE_BLOCK_TYPE(AexpIhistWeights, aexp_weights,\n>>> +\t\t\t BLOCK_AEXP_IHIST_WEIGHTS)\n>>> +MALI_C55_DEFINE_BLOCK_TYPE(Dgain, digital_gain, BLOCK_DIGITAL_GAIN)\n>>> +MALI_C55_DEFINE_BLOCK_TYPE(AwbGains, awb_gains, BLOCK_AWB_GAINS)\n>>> +MALI_C55_DEFINE_BLOCK_TYPE(AwbConfig, awb_config, BLOCK_AWB_CONFIG)\n>>> +MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig, mesh_shading_config,\n>>> +\t\t\t MESH_SHADING_CONFIG)\n>>> +MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel, mesh_shading_selection,\n>>> +\t\t\t MESH_SHADING_SELECTION)\n>>\n>> Personally I think it would be better to format these so that the fields for\n>> each entry are more visibly associated:\n>>\n>> MALI_C55_DEFINE_BLOCK_TYPE(Bls,\t\t\tsensor_off_preshading,\tBLOCK_SENSOR_OFFS);\n>> MALI_C55_DEFINE_BLOCK_TYPE(AexpHist,\t\taexp_hist,\t\tBLOCK_AEXP_HIST);\n>> MALI_C55_DEFINE_BLOCK_TYPE(AexpHistWeights,\taexp_weights,\t\tBLOCK_AEXP_HIST_WEIGHTS);\n>> MALI_C55_DEFINE_BLOCK_TYPE(AexpIhist,\t\taexp_hist,\t\tBLOCK_AEXP_IHIST);\n>> MALI_C55_DEFINE_BLOCK_TYPE(AexpIhistWeights,\taexp_weights,\t\tBLOCK_AEXP_IHIST_WEIGHTS);\n>> MALI_C55_DEFINE_BLOCK_TYPE(Dgain,\t\tdigital_gain,\t\tBLOCK_DIGITAL_GAIN);\n>> MALI_C55_DEFINE_BLOCK_TYPE(AwbGains,\t\tawb_gains,\t\tBLOCK_AWB_GAINS);\n>> MALI_C55_DEFINE_BLOCK_TYPE(AwbConfig,\t\tawb_config,\t\tBLOCK_AWB_CONFIG);\n>> MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig,\tmesh_shading_config,\tMESH_SHADING_CONFIG)\n>> MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel,\tmesh_shading_selection,\tMESH_SHADING_SELECTION);\n>>\n>> It's longer, but I think it's a much easier to read style for this kind of thing.\n> \n> More readable, I agree. Let's see how loud checkstyle complains (I\n> think it already was complaining on this version).\n\nAck\n\n> \n> Thanks\n> j\n> \n>>\n>> Thanks\n>> Dan\n>>\n>>> +\n>>> +struct param_traits {\n>>> +\tusing id_type = MaliC55Blocks;\n>>> +\n>>> +\ttemplate\n>>> +\tusing id_to_details = block_type;\n>>> +};\n>>> +\n>>> +} /* namespace details */\n>>> +\n>>> +class MaliC55Params : public V4L2Params\n>>> +{\n>>> +public:\n>>> +\tstatic constexpr unsigned int kVersion = MALI_C55_PARAM_BUFFER_V1;\n>>> +\n>>> +\tMaliC55Params(Span data)\n>>> +\t\t: V4L2Params(data, kVersion)\n>>> +\t{\n>>> +\t}\n>>> +};\n>>> +\n>>> +} /* namespace ipa::mali_c55 */\n>>> +\n>>> +} /* namespace libcamera */\n>>>\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 BA4E8BE080\n\tfor ;\n\tFri, 10 Oct 2025 16:35:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 088316B5C4;\n\tFri, 10 Oct 2025 18:35:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A9A06B599\n\tfor ;\n\tFri, 10 Oct 2025 18:35:16 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8B836EFE;\n\tFri, 10 Oct 2025 18:33:40 +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=\"PucQ1Uvt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760114020;\n\tbh=tBLkdF1C7RNLfbwilhUNyD+E0bkHP9zbakst3Y6OV4w=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=PucQ1UvtVrU3AZMpyp2mkD0giq7G+AxHwa/AIhRqGlLwaVHsmnpgpW4kyTRZou28I\n\tooSAYrIMlibA6RBYQn+2gYsnhspoPYUHjf0LxOEqReUdXSJ6rrKWJIvVpscI2BVH3u\n\tEEoa3DLLfjsrKr+aL2gPyaNVyG3HcrexL9dben/Q=","Message-ID":"","Date":"Fri, 10 Oct 2025 17:35:13 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 5/5] ipa: mali-c55: Introduce MaliC55Params","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20251007-v4l2-params-v5-0-8db451a81398@ideasonboard.com>\n\t<20251007-v4l2-params-v5-5-8db451a81398@ideasonboard.com>\n\t\n\t","Content-Language":"en-US","From":"Dan Scally ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}}]