[{"id":35729,"web_url":"https://patchwork.libcamera.org/comment/35729/","msgid":"","date":"2025-09-08T16:09:08","subject":"Re: [PATCH 3/4] ipa: libipa: Introduce V4L2Params","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 08. 29. 13:54 keltezéssel, Jacopo Mondi írta:\n> The existing RkISP1Params helper classes allows the RkISP1 to handle\n> V4L2 extensible parameters format and the legacy RkIPS1-specific\n> fixed-size parameters format.\n> \n> With the introduction of v4l2-params in the Linux kernel the part of\n> the RkISP1Params helper class that handles extensible parameters can\n> be generalized so that other IPA modules can use the same helpers\n> to populate a v4l2-params compatible buffer.\n> \n> Generalize the RkISP1Params class to a new libipa component named\n> V4L2Params and derive the existing RkISP1Params from it, leaving\n> in the RkISP1-specific implementation the handling of the legacy format.\n> \n> Deriving RkISP1Params from V4L2Params requires changing the size\n> associated to each block to include the size of v4l2_params_block_header\n> in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block()\n> implementation doesn't account for that as RkIS1Params::block()\n> implementation did.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/libipa/meson.build | 2 +\n> src/ipa/libipa/v4l2_params.cpp | 252 +++++++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/v4l2_params.h | 135 ++++++++++++++++++++++\n> src/ipa/rkisp1/params.cpp | 93 +--------------\n> src/ipa/rkisp1/params.h | 108 ++++++++----------\n> 5 files changed, 438 insertions(+), 152 deletions(-)\n> \n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -16,6 +16,7 @@ libipa_headers = files([\n> 'lsc_polynomial.h',\n> 'lux.h',\n> 'module.h',\n> + 'v4l2_params.h',\n> 'pwl.h',\n> ])\n> \n> @@ -35,6 +36,7 @@ libipa_sources = files([\n> 'lsc_polynomial.cpp',\n> 'lux.cpp',\n> 'module.cpp',\n> + 'v4l2_params.cpp',\n> 'pwl.cpp',\n> ])\n> \n> diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp\n> new file mode 100644\n> index 0000000000000000000000000000000000000000..674018065ace0e1b6b48b1630e556cef590d1e84\n> --- /dev/null\n> +++ b/src/ipa/libipa/v4l2_params.cpp\n> @@ -0,0 +1,252 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board\n> + *\n> + * V4L2 Parameters\n> + */\n> +\n> +#include \"v4l2_params.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\file v4l2_params.cpp\n> + * \\brief Helper class to populate a v4l2-params compatible parameters buffer\n> + *\n> + * The Linux kernel defines a generic buffer format for configuring ISP devices\n> + * through a set of parameters in the form of V4L2 extensible parameters. The\n> + * V4L2 extensible parameters define a serialization format for ISP parameters\n> + * that allows userspace to populate a buffer of configuration data by appending\n> + * them one after the other in a binary buffer.\n> + *\n> + * Each ISP driver compatible with the v4l2-params format will define its own\n> + * meta-output format identifier and defines the types of the configuration data\n> + * of each ISP block that usually match the registers layout.\n> + *\n> + * The V4L2Params class represent the V4L2 extensible parameters buffer and\n> + * allows users to populate the ISP configuration blocks, represented by the\n> + * V4L2ParamBlock class instances.\n> + *\n> + * IPA implementations using this helpers should define an enumeration of ISP\n> + * blocks the IPA module supports and use a set of common abstraction to help\n> + * their derived implementation of V4L2Params translate the enumerated ISP block\n> + * identifier to the actual type of the configuration data as defined by the\n> + * kernel interface.\n> + *\n> + * As an example of this see the RkISP1 and Mali-C55 implementations.\n> + */\n> +\n> +/**\n> + * \\class V4L2ParamsBlock\n> + * \\brief Helper class that represents a ISP configuration block\n> + *\n> + * Each ISP function is associated with a set of configuration parameters\n> + * defined by the kernel interface.\n> + *\n> + * This class represents an ISP block configuration entry. It is constructed\n> + * with a reference to the memory area where the block configuration will be\n> + * stored in the parameters buffer. The template parameter represents\n> + * the underlying kernel-defined ISP block configuration type and allow its\n> + * user to easily cast it to said type to populate and read the configuration\n> + * parameters.\n> + *\n> + * \\sa V4L2Params::block()\n> + */\n> +\n> +/**\n> + * \\fn V4L2ParamsBlock::V4L2ParamsBlock()\n> + * \\brief Construct a V4L2ParamsBlock with memory represented by \\a data\n> + * \\param[in] data A view on the memory area where the ISP block is located\n> + */\n> +\n> +/**\n> + * \\fn V4L2ParamsBlock::setEnabled()\n> + * \\brief Enable/disable an ISP configuration block\n> + * \\param[in] enabled The enable flag\n> + */\n> +\n> +/**\n> + * \\fn V4L2ParamsBlock::header()\n> + * \\brief Retrieve a reference to the header (struct v4l2_params_block_header)\n> + * \\return The block header\n> + */\n> +\n> +/**\n> + * \\fn V4L2ParamsBlock::data()\n> + * \\brief Retrieve a reference to block configuration data memory area\n> + * \\return The block data\n> + */\n> +\n> +/**\n> + * \\fn V4L2ParamsBlock::operator->()\n> + * \\brief Access the ISP configuration block casting it to the kernel-defined\n> + * ISP configuration type\n> + *\n> + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration\n> + * block type. This function allows users to easily cast a V4L2ParamsBlock to\n> + * the underlying kernel-defined type in order to easily populate or read\n> + * the ISP configuration data.\n> + *\n> + * \\code{.cpp}\n> + *\n> + * // The kernel header defines the ISP configuration types, in example\n> + * // struct my_isp_awb_config_data {\n> + * //\t\tu16 gain_ch00;\n> + * //\t\tu16 gain_ch01;\n> + * //\t\tu16 gain_ch10;\n> + * //\t\tu16 gain_ch11;\n> + * //\n> + * // }\n> + *\n> + * template<> V4L2ParamsBlock awbBlock\n> + *\n> + * awbBlock->gain_ch00 = ...;\n> + * awbBlock->gain_ch01 = ...;\n> + * awbBlock->gain_ch10 = ...;\n> + * awbBlock->gain_ch11 = ...;\n> + *\n> + * \\endcode\n> + *\n> + * Users of this class are not expected to create a V4L2ParamsBlock manually but\n> + * should rather use V4L2Params::block() to retrieve a reference to the memory\n> + * area used to construct a V4L2ParamsBlock in their overloaded\n> + * implementation of V4L2Params::block().\n> + */\n> +\n> +/**\n> + * \\fn V4L2ParamsBlock::operator->() const\n> + * \\copydoc V4L2ParamsBlock::operator->()\n> + */\n> +\n> +/**\n> + * \\fn V4L2ParamsBlock::operator*() const\n> + * \\copydoc V4L2ParamsBlock::operator->()\n> + */\n> +\n> +/**\n> + * \\fn V4L2ParamsBlock::operator*()\n> + * \\copydoc V4L2ParamsBlock::operator->()\n> + */\n> +\n> + /**\n> + * \\class V4L2Params\n> + * \\brief Helper class that represent an ISP configuration buffer\n> + *\n> + * ISP implementation compatible with v4l2-params define their ISP configuration\n> + * buffer types compatible with the struct v4l2_params_buffer type.\n> + *\n> + * This class represents an ISP configuration buffer. It is constructed\n> + * with a reference to the memory mapped buffer that will be queued to the ISP.\n> + *\n> + * This class is templated with the type of the enumeration of ISP blocks that\n> + * each IPA module is expected to support. IPA modules are expected to derive\n> + * this class and use the V4L2Params::block() function to retrieve the memory\n> + * area for each ISP configuration block and use it to construct a\n> + * V4L2ParamsBlock with it before returning it to the user.\n> + *\n> + * \\code{.cpp}\n> + *\n> + * enum class myISPBlocks {\n> + *\tAgc,\n> + *\tAwb,\n> + *\t...\n> + * };\n> + *\n> + * template\n> + * struct block_type {\n> + * };\n> + *\n> + * template<>\n> + * struct block_type {\n> + *\tusing type = struct my_isp_kernel_config_type_agc;\n> + * };\n> + *\n> + * template<>\n> + * struct block_type {\n> + *\tusing type = struct my_isp_kernel_config_type_awb;\n> + * };\n> + *\n> + * ...\n> + *\n> + * class MyISPParams : public V4L2Params\n> + * {\n> + * public:\n> + * \ttemplate\n> + * \tauto block()\n> + * \t{\n> + *\n> + * \t\t// Use the kernel defined configuration type as template\n> + * \t\t// argument to V4L2ParamsBlock.\n> + * \t\tusing Type = typename details::block_type::type;\n> + *\n> + * \t\t// Each IPA module should provide the information required\n> + * \t\t// to populate the block header\n> + *\n> + * \t\t...\n> + *\n> + * \t\tauto data = V4L2Params::block(B, blockType, blockSize);\n> + *\n> + * \t\treturn V4L2ParamsBlock(data);\n> + * \t}\n> + * };\n> + *\n> + * \\endcode\n> + *\n> + * As an example, see the RkISP1Params and MaliC55Params implementations.\n> + */\n> +\n> +/**\n> + * \\fn V4L2Params::V4L2Params()\n> + * \\brief Construct a V4L2Params\n> + * \\param[in] data Reference to the v4l2-buffer memory mapped area\n> + * \\param[in] version The ISP parameters version the implementation supports\n> + */\n> +\n> +/**\n> + * \\fn V4L2Params::size()\n> + * \\brief Retrieve the used size of the parameters buffer (in bytes)\n> + *\n> + * The parameters buffer size is mostly used to populate the v4l2_buffer\n> + * bytesused field before queueing the buffer to the ISP.\n> + *\n> + * \\return The number of bytes occupied by the ISP configuration parameters\n> + */\n> +\n> +/**\n> + * \\fn V4L2Params::block()\n> + * \\brief Populate an ISP configuration block a returns a reference to its\n> + * memory\n> + * \\param[in] type The ISP block identifier enumerated by the IPA module\n> + * \\param[in] blockType The kernel-defined ISP block identifier, used to\n> + * populate the block header\n> + * \\param[in] blockSize The ISP block size, used to populate the block header\n> + *\n> + *\n> + * Initialize the block header with \\a blockType and \\a blockSize and\n> + * returns a reference to the memory used to store an ISP configuration block.\n> + *\n> + * IPA modules that derive the V4L2Params class shall use this function to\n> + * retrieve the memory area that will be used to construct a V4L2ParamsBlock\n> + * before returning it to the caller.\n> + */\n> +\n> +/**\n> + * \\var V4L2Params::data_\n> + * \\brief The ISP parameters buffer memory\n> + */\n> +\n> +/**\n> + * \\var V4L2Params::used_\n> + * \\brief The number of bytes used in the parameters buffer\n> + */\n> +\n> +/**\n> + * \\var V4L2Params::blocks_\n> + * \\brief Cache of ISP configuration blocks\n> + */\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h\n> new file mode 100644\n> index 0000000000000000000000000000000000000000..5586096c7ee8a2d20877838564e8074e0fc3d1ce\n> --- /dev/null\n> +++ b/src/ipa/libipa/v4l2_params.h\n> @@ -0,0 +1,135 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board\n> + *\n> + * V4L2 Parameters\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +template\n> +class V4L2ParamsBlock\n> +{\n> +public:\n> +\tV4L2ParamsBlock(const Span &data)\n\nI think taking by value is fine, no need for the const ref.\n\n\n> +\t{\n> +\t\theader_ = data.subspan(0, sizeof(v4l2_params_block_header));\n> +\t\tdata_ = data.subspan(sizeof(v4l2_params_block_header));\n\nI'd probably do these in the member init list.\n\n\n> +\t}\n> +\n> +\tvoid setEnabled(bool enabled)\n> +\t{\n> +\t\tstruct v4l2_params_block_header *header =\n> +\t\t\treinterpret_cast(header_.data());\n> +\n> +\t\theader->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE |\n> +\t\t\t\t V4L2_PARAMS_FL_BLOCK_DISABLE);\n> +\t\theader->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE\n> +\t\t\t\t\t : V4L2_PARAMS_FL_BLOCK_DISABLE;\n> +\t}\n> +\n> +\tSpan header() const { return header_; }\n> +\tSpan data() const { return data_; }\n> +\n> +\tconst T *operator->() const\n> +\t{\n> +\t\treturn reinterpret_cast(data().data());\n> +\t}\n> +\n> +\tT *operator->()\n> +\t{\n> +\t\treturn reinterpret_cast(data().data());\n> +\t}\n> +\n> +\tconst T &operator*() const\n> +\t{\n> +\t\treturn *reinterpret_cast(data().data());\n> +\t}\n> +\n> +\tT &operator*()\n> +\t{\n> +\t\treturn *reinterpret_cast(data().data());\n> +\t}\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY(V4L2ParamsBlock)\n\nWhy disable? I would assume that it's OK to copy this?\n\n\n> +\n> +\tSpan header_;\n> +\tSpan data_;\n> +};\n> +\n> +template\n> +class V4L2Params\n> +{\n> +public:\n> +\tV4L2Params(Span data, unsigned int version)\n> +\t\t: data_(data)\n> +\t{\n> +\t\tstruct v4l2_params_buffer *cfg =\n> +\t\t\treinterpret_cast(data_.data());\n> +\t\tcfg->data_size = 0;\n> +\t\tcfg->version = version;\n> +\t\tused_ = offsetof(struct v4l2_params_buffer, data);\n> +\t}\n> +\n> +\tsize_t size() const { return used_; }\n> +\n> +protected:\n> +\tSpan block(T type, unsigned int blockType, size_t blockSize)\n\nThis can modified so that the derived classes need not implement any extra logic.\nOne option is to pass the `details::block_type` template as a template argument\nto V4L2Params and then use that to map the id to the type:\n\n template typename IdToType>\n class V4L2Params {\n\n template\n auto block() {\n using Type = typename IdToType::type;\n ...\n }\n\nbut I think it's simpler and more extensible to pass another type that holds\nall this compile-time information. It could look something like this:\n\ndiff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h\nindex 5586096c7..019f06509 100644\n--- a/src/ipa/libipa/v4l2_params.h\n+++ b/src/ipa/libipa/v4l2_params.h\n@@ -71,7 +71,7 @@ private:\n \tSpan data_;\n };\n \n-template\n+template\n class V4L2Params\n {\n public:\n@@ -87,8 +87,21 @@ public:\n \n \tsize_t size() const { return used_; }\n \n+\ttemplate\n+\tauto block()\n+\t{\n+\t\tusing Details = typename Traits::template id_to_details;\n+\n+\t\tusing Type = typename Details::type;\n+\t\tconstexpr auto kernelId = Details::blockType;\n+\n+\t\tauto data = block(Id, kernelId, sizeof(Type));\n+\n+\t\treturn V4L2ParamsBlock(data);\n+\t}\n+\n protected:\n-\tSpan block(T type, unsigned int blockType, size_t blockSize)\n+\tSpan block(typename Traits::id_type type, unsigned int blockType, size_t blockSize)\n \t{\n \t\t/*\n \t\t * Look up the block in the cache first. If an algorithm\n@@ -127,7 +140,7 @@ protected:\n \tSpan data_;\n \tsize_t used_;\n \n-\tstd::map> blocks_;\n+\tstd::map> blocks_;\n };\n \n } /* namespace ipa */\ndiff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h\nindex 1cc56dbad..975122999 100644\n--- a/src/ipa/mali-c55/params.h\n+++ b/src/ipa/mali-c55/params.h\n@@ -58,30 +58,24 @@ MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig, mesh_shading_config,\n MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel, mesh_shading_selection,\n \t\t\t MESH_SHADING_SELECTION)\n \n+struct params_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+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\t: V4L2Params(data, kVersion)\n \t{\n \t}\n-\n-\ttemplate\n-\tauto block()\n-\t{\n-\t\tusing Type = typename details::block_type::type;\n-\n-\t\tauto blockType = details::block_type::blockType;\n-\t\tsize_t blockSize = sizeof(Type);\n-\n-\t\tauto data = V4L2Params::block(B, blockType, blockSize);\n-\n-\t\treturn V4L2ParamsBlock(data);\n-\t}\n };\n \n } /* namespace ipa::mali_c55 */\ndiff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\nindex 7a2127664..824a5924f 100644\n--- a/src/ipa/rkisp1/params.h\n+++ b/src/ipa/rkisp1/params.h\n@@ -45,45 +45,54 @@ template\n struct block_type {\n };\n \n-#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)\t\t\\\n+#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id)\t\t\\\n template<>\t\t\t\t\t\t\t\t\\\n struct block_type {\t\t\t\t\\\n \tusing type = struct rkisp1_cif_isp_##blockStruct##_config;\t\\\n+\tstatic constexpr rkisp1_ext_params_block_type blockType =\t\\\n+\t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id;\t\t\t\\\n };\n \n-RKISP1_DEFINE_BLOCK_TYPE(Bls, bls)\n-RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc)\n-RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg)\n-RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain)\n-RKISP1_DEFINE_BLOCK_TYPE(Flt, flt)\n-RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm)\n-RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk)\n-RKISP1_DEFINE_BLOCK_TYPE(Goc, goc)\n-RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf)\n-RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength)\n-RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc)\n-RKISP1_DEFINE_BLOCK_TYPE(Ie, ie)\n-RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc)\n-RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas)\n-RKISP1_DEFINE_BLOCK_TYPE(Hst, hst)\n-RKISP1_DEFINE_BLOCK_TYPE(Aec, aec)\n-RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)\n-RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)\n-RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)\n-RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n+RKISP1_DEFINE_BLOCK_TYPE(Bls, bls, BLS)\n+RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc, DPCC)\n+RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg, SDG)\n+RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain, AWB_GAIN)\n+RKISP1_DEFINE_BLOCK_TYPE(Flt, flt, FLT)\n+RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm, BDM)\n+RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk, CTK)\n+RKISP1_DEFINE_BLOCK_TYPE(Goc, goc, GOC)\n+RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf, DPF)\n+RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength, DPF_STRENGTH)\n+RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc, CPROC)\n+RKISP1_DEFINE_BLOCK_TYPE(Ie, ie, IE)\n+RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc, LSC)\n+RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas, AWB_MEAS)\n+RKISP1_DEFINE_BLOCK_TYPE(Hst, hst, HST_MEAS)\n+RKISP1_DEFINE_BLOCK_TYPE(Aec, aec, AEC_MEAS)\n+RKISP1_DEFINE_BLOCK_TYPE(Afc, afc, AFC_MEAS)\n+RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls, COMPAND_BLS)\n+RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve, COMPAND_EXPAND)\n+RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve, COMPAND_COMPRESS)\n+\n+struct params_traits {\n+\tusing id_type = BlockType;\n+\n+\ttemplate\n+\tusing id_to_details = block_type;\n+};\n \n } /* namespace details */\n \n template\n class RkISP1ParamsBlock;\n \n-class RkISP1Params : public V4L2Params\n+class RkISP1Params : public V4L2Params\n {\n public:\n \tstatic constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;\n \n \tRkISP1Params(uint32_t format, Span data)\n-\t\t: V4L2Params(data, kVersion), format_(format)\n+\t\t: V4L2Params(data, kVersion), format_(format)\n \t{\n \t\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n \t\t\tmemset(data.data(), 0, data.size());\n\n\nAdmittedly I haven't tested it, and the rkisp1 parts are not there yet.\nBut I believe this direction could simplify the code and make it easier\nto implement for other parameter sets.\n\nFor rkisp1, I think as a first step `kBlockTypeInfo` should be moved completely\ninto `rkisp1::details::block_type` template specializations to do the mapping\nat compile time, similarly to how the proposed `MaliC55Params` does it.\n\nThen I believe one could realistically extend this `V4L2Params` to even support\nfixed-offset formats generically, if needed, but that might be an overkill.\n\n\n> +\t{\n> +\t\t/*\n> +\t\t * Look up the block in the cache first. If an algorithm\n> +\t\t * requests the same block type twice, it should get the same\n> +\t\t * block.\n> +\t\t */\n> +\t\tauto cacheIt = blocks_.find(type);\n> +\t\tif (cacheIt != blocks_.end())\n> +\t\t\treturn cacheIt->second;\n> +\n> +\t\t/* Make sure we don't run out of space. */\n> +\t\tif (blockSize > data_.size() - used_)\n> +\t\t\treturn {};\n> +\n> +\t\t/* Allocate a new block, clear its memory, and initialize its header. */\n> +\t\tSpan block = data_.subspan(used_, blockSize);\n> +\t\tused_ += blockSize;\n> +\n> +\t\tstruct v4l2_params_buffer *cfg =\n> +\t\t\treinterpret_cast(data_.data());\n> +\t\tcfg->data_size += blockSize;\n> +\n> +\t\tmemset(block.data(), 0, block.size());\n> +\n> +\t\tstruct v4l2_params_block_header *header =\n> +\t\t\treinterpret_cast(block.data());\n> +\t\theader->type = blockType;\n> +\t\theader->size = block.size();\n> +\n> +\t\t/* Update the cache. */\n> +\t\tblocks_[type] = block;\n> +\n> +\t\treturn block;\n> +\t}\n> +\n> +\tSpan data_;\n> +\tsize_t used_;\n> +\n> +\tstd::map> blocks_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n> index 4c0b051ce65da1686323ee9c66b82e12669a754d..2b692e1a1f199d6c118af0938e1aaecef6186a2c 100644\n> --- a/src/ipa/rkisp1/params.cpp\n> +++ b/src/ipa/rkisp1/params.cpp\n> @@ -35,7 +35,7 @@ struct BlockTypeInfo {\n> #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)\t\t\t\\\n> \t{ BlockType::block, {\t\t\t\t\t\t\t\\\n> \t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> -\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> +\t\tsizeof(struct rkisp1_ext_params_##type##_config),\t\t\\\n> \t\toffsetof(struct rkisp1_params_cfg, category.type##_config),\t\\\n> \t\tRKISP1_CIF_ISP_MODULE_##bit,\t\t\t\t\t\\\n> \t} }\n> @@ -49,7 +49,7 @@ struct BlockTypeInfo {\n> #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)\t\t\t\t\\\n> \t{ BlockType::block, {\t\t\t\t\t\t\t\\\n> \t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> -\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> +\t\tsizeof(struct rkisp1_ext_params_##type##_config),\t\t\\\n> \t\t0, 0,\t\t\t\t\t\t\t\t\\\n> \t} }\n> \n> @@ -78,56 +78,6 @@ const std::map kBlockTypeInfo = {\n> \n> } /* namespace */\n> \n> -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> -\t\t\t\t\t const Span &data)\n> -\t: params_(params), type_(type)\n> -{\n> -\tif (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> -\t\theader_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n> -\t\tdata_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n> -\t} else {\n> -\t\tdata_ = data;\n> -\t}\n> -}\n> -\n> -void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n> -{\n> -\t/*\n> -\t * For the legacy fixed format, blocks are enabled in the top-level\n> -\t * header. Delegate to the RkISP1Params class.\n> -\t */\n> -\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> -\t\treturn params_->setBlockEnabled(type_, enabled);\n> -\n> -\t/*\n> -\t * For the extensible format, set the enable and disable flags in the\n> -\t * block header directly.\n> -\t */\n> -\tstruct rkisp1_ext_params_block_header *header =\n> -\t\treinterpret_cast(header_.data());\n> -\theader->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |\n> -\t\t\t RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE);\n> -\theader->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE\n> -\t\t\t\t : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE;\n> -}\n> -\n> -RkISP1Params::RkISP1Params(uint32_t format, Span data)\n> -\t: format_(format), data_(data), used_(0)\n> -{\n> -\tif (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> -\t\tstruct rkisp1_ext_params_cfg *cfg =\n> -\t\t\treinterpret_cast(data.data());\n> -\n> -\t\tcfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n> -\t\tcfg->data_size = 0;\n> -\n> -\t\tused_ += offsetof(struct rkisp1_ext_params_cfg, data);\n> -\t} else {\n> -\t\tmemset(data.data(), 0, data.size());\n> -\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> -\t}\n> -}\n> -\n> void RkISP1Params::setBlockEnabled(BlockType type, bool enabled)\n> {\n> \tconst BlockTypeInfo &info = kBlockTypeInfo.at(type);\n> @@ -177,44 +127,7 @@ Span RkISP1Params::block(BlockType type)\n> \t\treturn data_.subspan(info.offset, info.size);\n> \t}\n> \n> -\t/*\n> -\t * For the extensible format, allocate memory for the block, including\n> -\t * the header. Look up the block in the cache first. If an algorithm\n> -\t * requests the same block type twice, it should get the same block.\n> -\t */\n> -\tauto cacheIt = blocks_.find(type);\n> -\tif (cacheIt != blocks_.end())\n> -\t\treturn cacheIt->second;\n> -\n> -\t/* Make sure we don't run out of space. */\n> -\tsize_t size = sizeof(struct rkisp1_ext_params_block_header)\n> -\t\t + ((info.size + 7) & ~7);\n> -\tif (size > data_.size() - used_) {\n> -\t\tLOG(RkISP1Params, Error)\n> -\t\t\t<< \"Out of memory to allocate block type \"\n> -\t\t\t<< utils::to_underlying(type);\n> -\t\treturn {};\n> -\t}\n> -\n> -\t/* Allocate a new block, clear its memory, and initialize its header. */\n> -\tSpan block = data_.subspan(used_, size);\n> -\tused_ += size;\n> -\n> -\tstruct rkisp1_ext_params_cfg *cfg =\n> -\t\treinterpret_cast(data_.data());\n> -\tcfg->data_size += size;\n> -\n> -\tmemset(block.data(), 0, block.size());\n> -\n> -\tstruct rkisp1_ext_params_block_header *header =\n> -\t\treinterpret_cast(block.data());\n> -\theader->type = info.type;\n> -\theader->size = block.size();\n> -\n> -\t/* Update the cache. */\n> -\tblocks_[type] = block;\n> -\n> -\treturn block;\n> +\treturn V4L2Params::block(type, info.type, info.size);\n> }\n> \n> } /* namespace ipa::rkisp1 */\n> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> index 40450e34497a3aa71b5b0cda2bf045a1cc0e012f..7a21276648162a127317c75cb01d1c0059b96ee1 100644\n> --- a/src/ipa/rkisp1/params.h\n> +++ b/src/ipa/rkisp1/params.h\n> @@ -7,13 +7,10 @@\n> \n> #pragma once\n> \n> -#include \n> -#include \n> -\n> #include \n> +#include \n> \n> -#include \n> -#include \n> +#include \n> \n> namespace libcamera {\n> \n> @@ -77,85 +74,72 @@ RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n> \n> } /* namespace details */\n> \n> -class RkISP1Params;\n> +template\n> +class RkISP1ParamsBlock;\n> \n> -class RkISP1ParamsBlockBase\n> +class RkISP1Params : public V4L2Params\n> {\n> public:\n> -\tRkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> -\t\t\t const Span &data);\n> -\n> -\tSpan data() const { return data_; }\n> -\n> -\tvoid setEnabled(bool enabled);\n> +\tstatic constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;\n> \n> -private:\n> -\tLIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)\n> -\n> -\tRkISP1Params *params_;\n> -\tBlockType type_;\n> -\tSpan header_;\n> -\tSpan data_;\n> -};\n> -\n> -template\n> -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> -{\n> -public:\n> -\tusing Type = typename details::block_type::type;\n> -\n> -\tRkISP1ParamsBlock(RkISP1Params *params, const Span &data)\n> -\t\t: RkISP1ParamsBlockBase(params, B, data)\n> +\tRkISP1Params(uint32_t format, Span data)\n> +\t\t: V4L2Params(data, kVersion), format_(format)\n> \t{\n> +\t\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n> +\t\t\tmemset(data.data(), 0, data.size());\n> +\t\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> +\t\t}\n> \t}\n> \n> -\tconst Type *operator->() const\n> +\ttemplate\n> +\tauto block()\n> \t{\n> -\t\treturn reinterpret_cast(data().data());\n> -\t}\n> +\t\tusing Type = typename details::block_type::type;\n> \n> -\tType *operator->()\n> -\t{\n> -\t\treturn reinterpret_cast(data().data());\n> +\t\treturn RkISP1ParamsBlock(this, B, block(B));\n> \t}\n> \n> -\tconst Type &operator*() const &\n> -\t{\n> -\t\treturn *reinterpret_cast(data().data());\n> -\t}\n> +\tuint32_t format() const { return format_; }\n> +\tvoid setBlockEnabled(BlockType type, bool enabled);\n> \n> -\tType &operator*() &\n> -\t{\n> -\t\treturn *reinterpret_cast(data().data());\n> -\t}\n> +private:\n> +\tSpan block(BlockType type);\n> +\n> +\tuint32_t format_;\n> };\n> \n> -class RkISP1Params\n> +template\n> +class RkISP1ParamsBlock : public V4L2ParamsBlock\n> {\n> public:\n> -\tRkISP1Params(uint32_t format, Span data);\n> -\n> -\ttemplate\n> -\tRkISP1ParamsBlock block()\n> +\tRkISP1ParamsBlock(RkISP1Params *params, BlockType type,\n> +\t\t\t const Span &data)\n> +\t\t: V4L2ParamsBlock(data)\n> \t{\n> -\t\treturn RkISP1ParamsBlock(this, block(B));\n> +\t\tparams_ = params;\n> +\t\ttype_ = type;\n> +\n> +\t\t/* Legacy param format has no header */\n> +\t\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> +\t\t\tdata_ = data;\n> \t}\n> \n> -\tuint32_t format() const { return format_; }\n> -\tsize_t size() const { return used_; }\n> +\tvoid setEnabled(bool enabled)\n> +\t{\n> +\t\t/*\n> +\t\t * For the legacy fixed format, blocks are enabled in the\n> +\t\t * top-level header. Delegate to the RkISP1Params class.\n> +\t\t */\n> +\t\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> +\t\t\treturn params_->setBlockEnabled(type_, enabled);\n> +\n> +\t\treturn V4L2ParamsBlock::setEnabled(enabled);\n> +\t}\n\nI have one concern regarding this design. An `RkISP1ParamsBlock` instance\nwill not work as its base type correctly in all cases. That is, calling the\nbase class `setEnabled()`:\n\n RkISP1ParamsBlock<...> *p = ...;\n\n p->V4L2ParamsBlock<...>::setEnabled(...);\n // or\n static_cast *>(p)->setEnabled(...);\n\nwill not work correctly if the format is the \"non-extensible\" one.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> private:\n> -\tfriend class RkISP1ParamsBlockBase;\n> -\n> -\tSpan block(BlockType type);\n> -\tvoid setBlockEnabled(BlockType type, bool enabled);\n> -\n> -\tuint32_t format_;\n> -\n> +\tRkISP1Params *params_;\n> +\tBlockType type_;\n> \tSpan data_;\n> -\tsize_t used_;\n> -\n> -\tstd::map> blocks_;\n> };\n> \n> } /* namespace ipa::rkisp1 */\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 E664BBDB13\n\tfor ;\n\tMon, 8 Sep 2025 16:09:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D65A46935A;\n\tMon, 8 Sep 2025 18:09:14 +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 500C56934E\n\tfor ;\n\tMon, 8 Sep 2025 18:09:12 +0200 (CEST)","from [192.168.33.12] (185.221.142.115.nat.pool.zt.hu\n\t[185.221.142.115])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 83958725;\n\tMon, 8 Sep 2025 18:07:59 +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=\"tsHLn4Ia\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1757347679;\n\tbh=rWouzbY6laS+07RCEeYxHUMtyktNj18MnN6K29UDsOM=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=tsHLn4IaVuIsXtf7HbeaKDSaM5geVVwfucsa+UIckMOw3bDih5XYUgbKDQx3Ssxgl\n\tr2J5SfHTYUNUORch+o1DcXhgRIwL/kymJRYZrv3HqsrHxc013/RRc7WsXSWAeoQ7bj\n\tQtkrzLE7NCmfLT5vCaAxRdTLljVnpXK5lbsNMc+I=","Message-ID":"","Date":"Mon, 8 Sep 2025 18:09:08 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 3/4] ipa: libipa: Introduce V4L2Params","To":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250829-v4l2-params-v1-0-340773fb69ff@ideasonboard.com>\n\t<20250829-v4l2-params-v1-3-340773fb69ff@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250829-v4l2-params-v1-3-340773fb69ff@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":35834,"web_url":"https://patchwork.libcamera.org/comment/35834/","msgid":"<2lytfujt57qxype7akx5tyxufwfgevztqj6nqjpdw34qhhg5vo@wdgvykvddjfc>","date":"2025-09-16T07:50:41","subject":"Re: [PATCH 3/4] ipa: libipa: Introduce V4L2Params","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n thanks for review\n\nOn Mon, Sep 08, 2025 at 06:09:08PM +0200, Barnabás Pőcze wrote:\n> Hi\n>\n> 2025. 08. 29. 13:54 keltezéssel, Jacopo Mondi írta:\n> > The existing RkISP1Params helper classes allows the RkISP1 to handle\n> > V4L2 extensible parameters format and the legacy RkIPS1-specific\n> > fixed-size parameters format.\n> >\n> > With the introduction of v4l2-params in the Linux kernel the part of\n> > the RkISP1Params helper class that handles extensible parameters can\n> > be generalized so that other IPA modules can use the same helpers\n> > to populate a v4l2-params compatible buffer.\n> >\n> > Generalize the RkISP1Params class to a new libipa component named\n> > V4L2Params and derive the existing RkISP1Params from it, leaving\n> > in the RkISP1-specific implementation the handling of the legacy format.\n> >\n> > Deriving RkISP1Params from V4L2Params requires changing the size\n> > associated to each block to include the size of v4l2_params_block_header\n> > in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block()\n> > implementation doesn't account for that as RkIS1Params::block()\n> > implementation did.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/libipa/meson.build | 2 +\n> > src/ipa/libipa/v4l2_params.cpp | 252 +++++++++++++++++++++++++++++++++++++++++\n> > src/ipa/libipa/v4l2_params.h | 135 ++++++++++++++++++++++\n> > src/ipa/rkisp1/params.cpp | 93 +--------------\n> > src/ipa/rkisp1/params.h | 108 ++++++++----------\n> > 5 files changed, 438 insertions(+), 152 deletions(-)\n> >\n> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644\n> > --- a/src/ipa/libipa/meson.build\n> > +++ b/src/ipa/libipa/meson.build\n> > @@ -16,6 +16,7 @@ libipa_headers = files([\n> > 'lsc_polynomial.h',\n> > 'lux.h',\n> > 'module.h',\n> > + 'v4l2_params.h',\n> > 'pwl.h',\n> > ])\n> > @@ -35,6 +36,7 @@ libipa_sources = files([\n> > 'lsc_polynomial.cpp',\n> > 'lux.cpp',\n> > 'module.cpp',\n> > + 'v4l2_params.cpp',\n> > 'pwl.cpp',\n> > ])\n> > diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp\n> > new file mode 100644\n> > index 0000000000000000000000000000000000000000..674018065ace0e1b6b48b1630e556cef590d1e84\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/v4l2_params.cpp\n> > @@ -0,0 +1,252 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2025, Ideas On Board\n> > + *\n> > + * V4L2 Parameters\n> > + */\n> > +\n> > +#include \"v4l2_params.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +\n> > +/**\n> > + * \\file v4l2_params.cpp\n> > + * \\brief Helper class to populate a v4l2-params compatible parameters buffer\n> > + *\n> > + * The Linux kernel defines a generic buffer format for configuring ISP devices\n> > + * through a set of parameters in the form of V4L2 extensible parameters. The\n> > + * V4L2 extensible parameters define a serialization format for ISP parameters\n> > + * that allows userspace to populate a buffer of configuration data by appending\n> > + * them one after the other in a binary buffer.\n> > + *\n> > + * Each ISP driver compatible with the v4l2-params format will define its own\n> > + * meta-output format identifier and defines the types of the configuration data\n> > + * of each ISP block that usually match the registers layout.\n> > + *\n> > + * The V4L2Params class represent the V4L2 extensible parameters buffer and\n> > + * allows users to populate the ISP configuration blocks, represented by the\n> > + * V4L2ParamBlock class instances.\n> > + *\n> > + * IPA implementations using this helpers should define an enumeration of ISP\n> > + * blocks the IPA module supports and use a set of common abstraction to help\n> > + * their derived implementation of V4L2Params translate the enumerated ISP block\n> > + * identifier to the actual type of the configuration data as defined by the\n> > + * kernel interface.\n> > + *\n> > + * As an example of this see the RkISP1 and Mali-C55 implementations.\n> > + */\n> > +\n> > +/**\n> > + * \\class V4L2ParamsBlock\n> > + * \\brief Helper class that represents a ISP configuration block\n> > + *\n> > + * Each ISP function is associated with a set of configuration parameters\n> > + * defined by the kernel interface.\n> > + *\n> > + * This class represents an ISP block configuration entry. It is constructed\n> > + * with a reference to the memory area where the block configuration will be\n> > + * stored in the parameters buffer. The template parameter represents\n> > + * the underlying kernel-defined ISP block configuration type and allow its\n> > + * user to easily cast it to said type to populate and read the configuration\n> > + * parameters.\n> > + *\n> > + * \\sa V4L2Params::block()\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2ParamsBlock::V4L2ParamsBlock()\n> > + * \\brief Construct a V4L2ParamsBlock with memory represented by \\a data\n> > + * \\param[in] data A view on the memory area where the ISP block is located\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2ParamsBlock::setEnabled()\n> > + * \\brief Enable/disable an ISP configuration block\n> > + * \\param[in] enabled The enable flag\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2ParamsBlock::header()\n> > + * \\brief Retrieve a reference to the header (struct v4l2_params_block_header)\n> > + * \\return The block header\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2ParamsBlock::data()\n> > + * \\brief Retrieve a reference to block configuration data memory area\n> > + * \\return The block data\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2ParamsBlock::operator->()\n> > + * \\brief Access the ISP configuration block casting it to the kernel-defined\n> > + * ISP configuration type\n> > + *\n> > + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration\n> > + * block type. This function allows users to easily cast a V4L2ParamsBlock to\n> > + * the underlying kernel-defined type in order to easily populate or read\n> > + * the ISP configuration data.\n> > + *\n> > + * \\code{.cpp}\n> > + *\n> > + * // The kernel header defines the ISP configuration types, in example\n> > + * // struct my_isp_awb_config_data {\n> > + * //\t\tu16 gain_ch00;\n> > + * //\t\tu16 gain_ch01;\n> > + * //\t\tu16 gain_ch10;\n> > + * //\t\tu16 gain_ch11;\n> > + * //\n> > + * // }\n> > + *\n> > + * template<> V4L2ParamsBlock awbBlock\n> > + *\n> > + * awbBlock->gain_ch00 = ...;\n> > + * awbBlock->gain_ch01 = ...;\n> > + * awbBlock->gain_ch10 = ...;\n> > + * awbBlock->gain_ch11 = ...;\n> > + *\n> > + * \\endcode\n> > + *\n> > + * Users of this class are not expected to create a V4L2ParamsBlock manually but\n> > + * should rather use V4L2Params::block() to retrieve a reference to the memory\n> > + * area used to construct a V4L2ParamsBlock in their overloaded\n> > + * implementation of V4L2Params::block().\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2ParamsBlock::operator->() const\n> > + * \\copydoc V4L2ParamsBlock::operator->()\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2ParamsBlock::operator*() const\n> > + * \\copydoc V4L2ParamsBlock::operator->()\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2ParamsBlock::operator*()\n> > + * \\copydoc V4L2ParamsBlock::operator->()\n> > + */\n> > +\n> > + /**\n> > + * \\class V4L2Params\n> > + * \\brief Helper class that represent an ISP configuration buffer\n> > + *\n> > + * ISP implementation compatible with v4l2-params define their ISP configuration\n> > + * buffer types compatible with the struct v4l2_params_buffer type.\n> > + *\n> > + * This class represents an ISP configuration buffer. It is constructed\n> > + * with a reference to the memory mapped buffer that will be queued to the ISP.\n> > + *\n> > + * This class is templated with the type of the enumeration of ISP blocks that\n> > + * each IPA module is expected to support. IPA modules are expected to derive\n> > + * this class and use the V4L2Params::block() function to retrieve the memory\n> > + * area for each ISP configuration block and use it to construct a\n> > + * V4L2ParamsBlock with it before returning it to the user.\n> > + *\n> > + * \\code{.cpp}\n> > + *\n> > + * enum class myISPBlocks {\n> > + *\tAgc,\n> > + *\tAwb,\n> > + *\t...\n> > + * };\n> > + *\n> > + * template\n> > + * struct block_type {\n> > + * };\n> > + *\n> > + * template<>\n> > + * struct block_type {\n> > + *\tusing type = struct my_isp_kernel_config_type_agc;\n> > + * };\n> > + *\n> > + * template<>\n> > + * struct block_type {\n> > + *\tusing type = struct my_isp_kernel_config_type_awb;\n> > + * };\n> > + *\n> > + * ...\n> > + *\n> > + * class MyISPParams : public V4L2Params\n> > + * {\n> > + * public:\n> > + * \ttemplate\n> > + * \tauto block()\n> > + * \t{\n> > + *\n> > + * \t\t// Use the kernel defined configuration type as template\n> > + * \t\t// argument to V4L2ParamsBlock.\n> > + * \t\tusing Type = typename details::block_type::type;\n> > + *\n> > + * \t\t// Each IPA module should provide the information required\n> > + * \t\t// to populate the block header\n> > + *\n> > + * \t\t...\n> > + *\n> > + * \t\tauto data = V4L2Params::block(B, blockType, blockSize);\n> > + *\n> > + * \t\treturn V4L2ParamsBlock(data);\n> > + * \t}\n> > + * };\n> > + *\n> > + * \\endcode\n> > + *\n> > + * As an example, see the RkISP1Params and MaliC55Params implementations.\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Params::V4L2Params()\n> > + * \\brief Construct a V4L2Params\n> > + * \\param[in] data Reference to the v4l2-buffer memory mapped area\n> > + * \\param[in] version The ISP parameters version the implementation supports\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Params::size()\n> > + * \\brief Retrieve the used size of the parameters buffer (in bytes)\n> > + *\n> > + * The parameters buffer size is mostly used to populate the v4l2_buffer\n> > + * bytesused field before queueing the buffer to the ISP.\n> > + *\n> > + * \\return The number of bytes occupied by the ISP configuration parameters\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Params::block()\n> > + * \\brief Populate an ISP configuration block a returns a reference to its\n> > + * memory\n> > + * \\param[in] type The ISP block identifier enumerated by the IPA module\n> > + * \\param[in] blockType The kernel-defined ISP block identifier, used to\n> > + * populate the block header\n> > + * \\param[in] blockSize The ISP block size, used to populate the block header\n> > + *\n> > + *\n> > + * Initialize the block header with \\a blockType and \\a blockSize and\n> > + * returns a reference to the memory used to store an ISP configuration block.\n> > + *\n> > + * IPA modules that derive the V4L2Params class shall use this function to\n> > + * retrieve the memory area that will be used to construct a V4L2ParamsBlock\n> > + * before returning it to the caller.\n> > + */\n> > +\n> > +/**\n> > + * \\var V4L2Params::data_\n> > + * \\brief The ISP parameters buffer memory\n> > + */\n> > +\n> > +/**\n> > + * \\var V4L2Params::used_\n> > + * \\brief The number of bytes used in the parameters buffer\n> > + */\n> > +\n> > +/**\n> > + * \\var V4L2Params::blocks_\n> > + * \\brief Cache of ISP configuration blocks\n> > + */\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h\n> > new file mode 100644\n> > index 0000000000000000000000000000000000000000..5586096c7ee8a2d20877838564e8074e0fc3d1ce\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/v4l2_params.h\n> > @@ -0,0 +1,135 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2025, Ideas On Board\n> > + *\n> > + * V4L2 Parameters\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +\n> > +template\n> > +class V4L2ParamsBlock\n> > +{\n> > +public:\n> > +\tV4L2ParamsBlock(const Span &data)\n>\n> I think taking by value is fine, no need for the const ref.\n>\n\nright indeed\n\n>\n> > +\t{\n> > +\t\theader_ = data.subspan(0, sizeof(v4l2_params_block_header));\n> > +\t\tdata_ = data.subspan(sizeof(v4l2_params_block_header));\n>\n> I'd probably do these in the member init list.\n>\n\nis this just a style preference or are there reasons I'm missing ?\n\n>\n> > +\t}\n> > +\n> > +\tvoid setEnabled(bool enabled)\n> > +\t{\n> > +\t\tstruct v4l2_params_block_header *header =\n> > +\t\t\treinterpret_cast(header_.data());\n> > +\n> > +\t\theader->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE |\n> > +\t\t\t\t V4L2_PARAMS_FL_BLOCK_DISABLE);\n> > +\t\theader->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE\n> > +\t\t\t\t\t : V4L2_PARAMS_FL_BLOCK_DISABLE;\n> > +\t}\n> > +\n> > +\tSpan header() const { return header_; }\n> > +\tSpan data() const { return data_; }\n> > +\n> > +\tconst T *operator->() const\n> > +\t{\n> > +\t\treturn reinterpret_cast(data().data());\n> > +\t}\n> > +\n> > +\tT *operator->()\n> > +\t{\n> > +\t\treturn reinterpret_cast(data().data());\n> > +\t}\n> > +\n> > +\tconst T &operator*() const\n> > +\t{\n> > +\t\treturn *reinterpret_cast(data().data());\n> > +\t}\n> > +\n> > +\tT &operator*()\n> > +\t{\n> > +\t\treturn *reinterpret_cast(data().data());\n> > +\t}\n> > +\n> > +private:\n> > +\tLIBCAMERA_DISABLE_COPY(V4L2ParamsBlock)\n>\n> Why disable? I would assume that it's OK to copy this?\n>\n\ntbh I think I just copied this from RkISP1Params.\n\nHowever, why would one copy an instance of V4L2ParamsBlock ?\n\n>\n> > +\n> > +\tSpan header_;\n> > +\tSpan data_;\n> > +};\n> > +\n> > +template\n> > +class V4L2Params\n> > +{\n> > +public:\n> > +\tV4L2Params(Span data, unsigned int version)\n> > +\t\t: data_(data)\n> > +\t{\n> > +\t\tstruct v4l2_params_buffer *cfg =\n> > +\t\t\treinterpret_cast(data_.data());\n> > +\t\tcfg->data_size = 0;\n> > +\t\tcfg->version = version;\n> > +\t\tused_ = offsetof(struct v4l2_params_buffer, data);\n> > +\t}\n> > +\n> > +\tsize_t size() const { return used_; }\n> > +\n> > +protected:\n> > +\tSpan block(T type, unsigned int blockType, size_t blockSize)\n>\n> This can modified so that the derived classes need not implement any extra logic.\n> One option is to pass the `details::block_type` template as a template argument\n> to V4L2Params and then use that to map the id to the type:\n>\n> template typename IdToType>\n> class V4L2Params {\n>\n> template\n> auto block() {\n> using Type = typename IdToType::type;\n> ...\n> }\n>\n> but I think it's simpler and more extensible to pass another type that holds\n> all this compile-time information. It could look something like this:\n>\n> diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h\n> index 5586096c7..019f06509 100644\n> --- a/src/ipa/libipa/v4l2_params.h\n> +++ b/src/ipa/libipa/v4l2_params.h\n> @@ -71,7 +71,7 @@ private:\n> \tSpan data_;\n> };\n> -template\n> +template\n> class V4L2Params\n> {\n> public:\n> @@ -87,8 +87,21 @@ public:\n> \tsize_t size() const { return used_; }\n> +\ttemplate\n> +\tauto block()\n> +\t{\n> +\t\tusing Details = typename Traits::template id_to_details;\n\nI can't grok 'typename Traits::template'\n\nbut it's ok, I'll trust you here\n\n> +\n> +\t\tusing Type = typename Details::type;\n> +\t\tconstexpr auto kernelId = Details::blockType;\n> +\n> +\t\tauto data = block(Id, kernelId, sizeof(Type));\n> +\n> +\t\treturn V4L2ParamsBlock(data);\n> +\t}\n> +\n> protected:\n> -\tSpan block(T type, unsigned int blockType, size_t blockSize)\n> +\tSpan block(typename Traits::id_type type, unsigned int blockType, size_t blockSize)\n> \t{\n> \t\t/*\n> \t\t * Look up the block in the cache first. If an algorithm\n> @@ -127,7 +140,7 @@ protected:\n> \tSpan data_;\n> \tsize_t used_;\n> -\tstd::map> blocks_;\n> +\tstd::map> blocks_;\n> };\n> } /* namespace ipa */\n> diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h\n> index 1cc56dbad..975122999 100644\n> --- a/src/ipa/mali-c55/params.h\n> +++ b/src/ipa/mali-c55/params.h\n> @@ -58,30 +58,24 @@ MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig, mesh_shading_config,\n> MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel, mesh_shading_selection,\n> \t\t\t MESH_SHADING_SELECTION)\n> +struct params_traits {\n> +\tusing id_type = MaliC55Blocks;\n> +\n> +\ttemplate\n> +\tusing id_to_details = block_type;\n> +};\n> +\n> } /* namespace details */\n> -class MaliC55Params : public V4L2Params\n> +class MaliC55Params : public V4L2Params\n> {\n> public:\n> \tstatic constexpr unsigned int kVersion = MALI_C55_PARAM_BUFFER_V1;\n> \tMaliC55Params(Span data)\n> -\t\t: V4L2Params(data, kVersion)\n> +\t\t: V4L2Params(data, kVersion)\n> \t{\n> \t}\n> -\n> -\ttemplate\n> -\tauto block()\n> -\t{\n> -\t\tusing Type = typename details::block_type::type;\n> -\n> -\t\tauto blockType = details::block_type::blockType;\n> -\t\tsize_t blockSize = sizeof(Type);\n> -\n> -\t\tauto data = V4L2Params::block(B, blockType, blockSize);\n> -\n> -\t\treturn V4L2ParamsBlock(data);\n> -\t}\n\nThanks!\n\n'struct param_traits' and the associated changes in V4L2Params allowed\nme to simplify the MaliC55 implementation as you suggested\n\n> };\n> } /* namespace ipa::mali_c55 */\n> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> index 7a2127664..824a5924f 100644\n> --- a/src/ipa/rkisp1/params.h\n> +++ b/src/ipa/rkisp1/params.h\n> @@ -45,45 +45,54 @@ template\n> struct block_type {\n> };\n> -#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)\t\t\\\n> +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id)\t\t\\\n> template<>\t\t\t\t\t\t\t\t\\\n> struct block_type {\t\t\t\t\\\n> \tusing type = struct rkisp1_cif_isp_##blockStruct##_config;\t\\\n> +\tstatic constexpr rkisp1_ext_params_block_type blockType =\t\\\n> +\t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id;\t\t\t\\\n> };\n> -RKISP1_DEFINE_BLOCK_TYPE(Bls, bls)\n> -RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc)\n> -RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg)\n> -RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain)\n> -RKISP1_DEFINE_BLOCK_TYPE(Flt, flt)\n> -RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm)\n> -RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk)\n> -RKISP1_DEFINE_BLOCK_TYPE(Goc, goc)\n> -RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf)\n> -RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength)\n> -RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc)\n> -RKISP1_DEFINE_BLOCK_TYPE(Ie, ie)\n> -RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc)\n> -RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas)\n> -RKISP1_DEFINE_BLOCK_TYPE(Hst, hst)\n> -RKISP1_DEFINE_BLOCK_TYPE(Aec, aec)\n> -RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)\n> -RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)\n> -RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)\n> -RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n> +RKISP1_DEFINE_BLOCK_TYPE(Bls, bls, BLS)\n> +RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc, DPCC)\n> +RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg, SDG)\n> +RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain, AWB_GAIN)\n> +RKISP1_DEFINE_BLOCK_TYPE(Flt, flt, FLT)\n> +RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm, BDM)\n> +RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk, CTK)\n> +RKISP1_DEFINE_BLOCK_TYPE(Goc, goc, GOC)\n> +RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf, DPF)\n> +RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength, DPF_STRENGTH)\n> +RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc, CPROC)\n> +RKISP1_DEFINE_BLOCK_TYPE(Ie, ie, IE)\n> +RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc, LSC)\n> +RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas, AWB_MEAS)\n> +RKISP1_DEFINE_BLOCK_TYPE(Hst, hst, HST_MEAS)\n> +RKISP1_DEFINE_BLOCK_TYPE(Aec, aec, AEC_MEAS)\n> +RKISP1_DEFINE_BLOCK_TYPE(Afc, afc, AFC_MEAS)\n> +RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls, COMPAND_BLS)\n> +RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve, COMPAND_EXPAND)\n> +RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve, COMPAND_COMPRESS)\n> +\n> +struct params_traits {\n> +\tusing id_type = BlockType;\n> +\n> +\ttemplate\n> +\tusing id_to_details = block_type;\n> +};\n> } /* namespace details */\n> template\n> class RkISP1ParamsBlock;\n> -class RkISP1Params : public V4L2Params\n> +class RkISP1Params : public V4L2Params\n> {\n> public:\n> \tstatic constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;\n> \tRkISP1Params(uint32_t format, Span data)\n> -\t\t: V4L2Params(data, kVersion), format_(format)\n> +\t\t: V4L2Params(data, kVersion), format_(format)\n> \t{\n> \t\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n> \t\t\tmemset(data.data(), 0, data.size());\n>\n>\n> Admittedly I haven't tested it, and the rkisp1 parts are not there yet.\n> But I believe this direction could simplify the code and make it easier\n> to implement for other parameter sets.\n\nPlease note that for RkISP1 I had to keep an override for\nRkISP1Params::block() as we need to support both legacy and extensible\n\n>\n> For rkisp1, I think as a first step `kBlockTypeInfo` should be moved completely\n> into `rkisp1::details::block_type` template specializations to do the mapping\n> at compile time, similarly to how the proposed `MaliC55Params` does it.\n\nyeah, I don't like that map too much. But, if I may use it as an\nexcuse, it was there already so we're not doing any worse.\n\nI could have removed it like it's done for Mali, but the 'enableBit'\nused by setEnabled() to support the legacy format made it more\ncomplicated getting rid of it (I've not invested much time in trying,\nadmittedly).\n\nAnyway, could easily be done on top ?\n\n>\n> Then I believe one could realistically extend this `V4L2Params` to even support\n> fixed-offset formats generically, if needed, but that might be an overkill.\n\nNot sure I got what you mean here,\n>\n>\n> > +\t{\n> > +\t\t/*\n> > +\t\t * Look up the block in the cache first. If an algorithm\n> > +\t\t * requests the same block type twice, it should get the same\n> > +\t\t * block.\n> > +\t\t */\n> > +\t\tauto cacheIt = blocks_.find(type);\n> > +\t\tif (cacheIt != blocks_.end())\n> > +\t\t\treturn cacheIt->second;\n> > +\n> > +\t\t/* Make sure we don't run out of space. */\n> > +\t\tif (blockSize > data_.size() - used_)\n> > +\t\t\treturn {};\n> > +\n> > +\t\t/* Allocate a new block, clear its memory, and initialize its header. */\n> > +\t\tSpan block = data_.subspan(used_, blockSize);\n> > +\t\tused_ += blockSize;\n> > +\n> > +\t\tstruct v4l2_params_buffer *cfg =\n> > +\t\t\treinterpret_cast(data_.data());\n> > +\t\tcfg->data_size += blockSize;\n> > +\n> > +\t\tmemset(block.data(), 0, block.size());\n> > +\n> > +\t\tstruct v4l2_params_block_header *header =\n> > +\t\t\treinterpret_cast(block.data());\n> > +\t\theader->type = blockType;\n> > +\t\theader->size = block.size();\n> > +\n> > +\t\t/* Update the cache. */\n> > +\t\tblocks_[type] = block;\n> > +\n> > +\t\treturn block;\n> > +\t}\n> > +\n> > +\tSpan data_;\n> > +\tsize_t used_;\n> > +\n> > +\tstd::map> blocks_;\n> > +};\n> > +\n> > +} /* namespace ipa */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n> > index 4c0b051ce65da1686323ee9c66b82e12669a754d..2b692e1a1f199d6c118af0938e1aaecef6186a2c 100644\n> > --- a/src/ipa/rkisp1/params.cpp\n> > +++ b/src/ipa/rkisp1/params.cpp\n> > @@ -35,7 +35,7 @@ struct BlockTypeInfo {\n> > #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)\t\t\t\\\n> > \t{ BlockType::block, {\t\t\t\t\t\t\t\\\n> > \t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> > -\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> > +\t\tsizeof(struct rkisp1_ext_params_##type##_config),\t\t\\\n> > \t\toffsetof(struct rkisp1_params_cfg, category.type##_config),\t\\\n> > \t\tRKISP1_CIF_ISP_MODULE_##bit,\t\t\t\t\t\\\n> > \t} }\n> > @@ -49,7 +49,7 @@ struct BlockTypeInfo {\n> > #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)\t\t\t\t\\\n> > \t{ BlockType::block, {\t\t\t\t\t\t\t\\\n> > \t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> > -\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> > +\t\tsizeof(struct rkisp1_ext_params_##type##_config),\t\t\\\n> > \t\t0, 0,\t\t\t\t\t\t\t\t\\\n> > \t} }\n> > @@ -78,56 +78,6 @@ const std::map kBlockTypeInfo = {\n> > } /* namespace */\n> > -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> > -\t\t\t\t\t const Span &data)\n> > -\t: params_(params), type_(type)\n> > -{\n> > -\tif (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > -\t\theader_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n> > -\t\tdata_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n> > -\t} else {\n> > -\t\tdata_ = data;\n> > -\t}\n> > -}\n> > -\n> > -void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n> > -{\n> > -\t/*\n> > -\t * For the legacy fixed format, blocks are enabled in the top-level\n> > -\t * header. Delegate to the RkISP1Params class.\n> > -\t */\n> > -\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> > -\t\treturn params_->setBlockEnabled(type_, enabled);\n> > -\n> > -\t/*\n> > -\t * For the extensible format, set the enable and disable flags in the\n> > -\t * block header directly.\n> > -\t */\n> > -\tstruct rkisp1_ext_params_block_header *header =\n> > -\t\treinterpret_cast(header_.data());\n> > -\theader->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |\n> > -\t\t\t RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE);\n> > -\theader->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE\n> > -\t\t\t\t : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE;\n> > -}\n> > -\n> > -RkISP1Params::RkISP1Params(uint32_t format, Span data)\n> > -\t: format_(format), data_(data), used_(0)\n> > -{\n> > -\tif (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > -\t\tstruct rkisp1_ext_params_cfg *cfg =\n> > -\t\t\treinterpret_cast(data.data());\n> > -\n> > -\t\tcfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n> > -\t\tcfg->data_size = 0;\n> > -\n> > -\t\tused_ += offsetof(struct rkisp1_ext_params_cfg, data);\n> > -\t} else {\n> > -\t\tmemset(data.data(), 0, data.size());\n> > -\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> > -\t}\n> > -}\n> > -\n> > void RkISP1Params::setBlockEnabled(BlockType type, bool enabled)\n> > {\n> > \tconst BlockTypeInfo &info = kBlockTypeInfo.at(type);\n> > @@ -177,44 +127,7 @@ Span RkISP1Params::block(BlockType type)\n> > \t\treturn data_.subspan(info.offset, info.size);\n> > \t}\n> > -\t/*\n> > -\t * For the extensible format, allocate memory for the block, including\n> > -\t * the header. Look up the block in the cache first. If an algorithm\n> > -\t * requests the same block type twice, it should get the same block.\n> > -\t */\n> > -\tauto cacheIt = blocks_.find(type);\n> > -\tif (cacheIt != blocks_.end())\n> > -\t\treturn cacheIt->second;\n> > -\n> > -\t/* Make sure we don't run out of space. */\n> > -\tsize_t size = sizeof(struct rkisp1_ext_params_block_header)\n> > -\t\t + ((info.size + 7) & ~7);\n> > -\tif (size > data_.size() - used_) {\n> > -\t\tLOG(RkISP1Params, Error)\n> > -\t\t\t<< \"Out of memory to allocate block type \"\n> > -\t\t\t<< utils::to_underlying(type);\n> > -\t\treturn {};\n> > -\t}\n> > -\n> > -\t/* Allocate a new block, clear its memory, and initialize its header. */\n> > -\tSpan block = data_.subspan(used_, size);\n> > -\tused_ += size;\n> > -\n> > -\tstruct rkisp1_ext_params_cfg *cfg =\n> > -\t\treinterpret_cast(data_.data());\n> > -\tcfg->data_size += size;\n> > -\n> > -\tmemset(block.data(), 0, block.size());\n> > -\n> > -\tstruct rkisp1_ext_params_block_header *header =\n> > -\t\treinterpret_cast(block.data());\n> > -\theader->type = info.type;\n> > -\theader->size = block.size();\n> > -\n> > -\t/* Update the cache. */\n> > -\tblocks_[type] = block;\n> > -\n> > -\treturn block;\n> > +\treturn V4L2Params::block(type, info.type, info.size);\n> > }\n> > } /* namespace ipa::rkisp1 */\n> > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> > index 40450e34497a3aa71b5b0cda2bf045a1cc0e012f..7a21276648162a127317c75cb01d1c0059b96ee1 100644\n> > --- a/src/ipa/rkisp1/params.h\n> > +++ b/src/ipa/rkisp1/params.h\n> > @@ -7,13 +7,10 @@\n> > #pragma once\n> > -#include \n> > -#include \n> > -\n> > #include \n> > +#include \n> > -#include \n> > -#include \n> > +#include \n> > namespace libcamera {\n> > @@ -77,85 +74,72 @@ RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n> > } /* namespace details */\n> > -class RkISP1Params;\n> > +template\n> > +class RkISP1ParamsBlock;\n> > -class RkISP1ParamsBlockBase\n> > +class RkISP1Params : public V4L2Params\n> > {\n> > public:\n> > -\tRkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> > -\t\t\t const Span &data);\n> > -\n> > -\tSpan data() const { return data_; }\n> > -\n> > -\tvoid setEnabled(bool enabled);\n> > +\tstatic constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;\n> > -private:\n> > -\tLIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)\n> > -\n> > -\tRkISP1Params *params_;\n> > -\tBlockType type_;\n> > -\tSpan header_;\n> > -\tSpan data_;\n> > -};\n> > -\n> > -template\n> > -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> > -{\n> > -public:\n> > -\tusing Type = typename details::block_type::type;\n> > -\n> > -\tRkISP1ParamsBlock(RkISP1Params *params, const Span &data)\n> > -\t\t: RkISP1ParamsBlockBase(params, B, data)\n> > +\tRkISP1Params(uint32_t format, Span data)\n> > +\t\t: V4L2Params(data, kVersion), format_(format)\n> > \t{\n> > +\t\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n> > +\t\t\tmemset(data.data(), 0, data.size());\n> > +\t\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> > +\t\t}\n> > \t}\n> > -\tconst Type *operator->() const\n> > +\ttemplate\n> > +\tauto block()\n> > \t{\n> > -\t\treturn reinterpret_cast(data().data());\n> > -\t}\n> > +\t\tusing Type = typename details::block_type::type;\n> > -\tType *operator->()\n> > -\t{\n> > -\t\treturn reinterpret_cast(data().data());\n> > +\t\treturn RkISP1ParamsBlock(this, B, block(B));\n> > \t}\n> > -\tconst Type &operator*() const &\n> > -\t{\n> > -\t\treturn *reinterpret_cast(data().data());\n> > -\t}\n> > +\tuint32_t format() const { return format_; }\n> > +\tvoid setBlockEnabled(BlockType type, bool enabled);\n> > -\tType &operator*() &\n> > -\t{\n> > -\t\treturn *reinterpret_cast(data().data());\n> > -\t}\n> > +private:\n> > +\tSpan block(BlockType type);\n> > +\n> > +\tuint32_t format_;\n> > };\n> > -class RkISP1Params\n> > +template\n> > +class RkISP1ParamsBlock : public V4L2ParamsBlock\n> > {\n> > public:\n> > -\tRkISP1Params(uint32_t format, Span data);\n> > -\n> > -\ttemplate\n> > -\tRkISP1ParamsBlock block()\n> > +\tRkISP1ParamsBlock(RkISP1Params *params, BlockType type,\n> > +\t\t\t const Span &data)\n> > +\t\t: V4L2ParamsBlock(data)\n> > \t{\n> > -\t\treturn RkISP1ParamsBlock(this, block(B));\n> > +\t\tparams_ = params;\n> > +\t\ttype_ = type;\n> > +\n> > +\t\t/* Legacy param format has no header */\n> > +\t\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> > +\t\t\tdata_ = data;\n> > \t}\n> > -\tuint32_t format() const { return format_; }\n> > -\tsize_t size() const { return used_; }\n> > +\tvoid setEnabled(bool enabled)\n> > +\t{\n> > +\t\t/*\n> > +\t\t * For the legacy fixed format, blocks are enabled in the\n> > +\t\t * top-level header. Delegate to the RkISP1Params class.\n> > +\t\t */\n> > +\t\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> > +\t\t\treturn params_->setBlockEnabled(type_, enabled);\n> > +\n> > +\t\treturn V4L2ParamsBlock::setEnabled(enabled);\n> > +\t}\n>\n> I have one concern regarding this design. An `RkISP1ParamsBlock` instance\n> will not work as its base type correctly in all cases. That is, calling the\n> base class `setEnabled()`:\n>\n> RkISP1ParamsBlock<...> *p = ...;\n>\n> p->V4L2ParamsBlock<...>::setEnabled(...);\n> // or\n> static_cast *>(p)->setEnabled(...);\n>\n> will not work correctly if the format is the \"non-extensible\" one.\n\nThe only user of this class is the RkISP1 IPA, why would it forcefully\nuse the base class implementation ?\n\nIf there's an easy way to avoid the above, I'll anyway happily consider\nit.\n\nThanks\n j\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n> > private:\n> > -\tfriend class RkISP1ParamsBlockBase;\n> > -\n> > -\tSpan block(BlockType type);\n> > -\tvoid setBlockEnabled(BlockType type, bool enabled);\n> > -\n> > -\tuint32_t format_;\n> > -\n> > +\tRkISP1Params *params_;\n> > +\tBlockType type_;\n> > \tSpan data_;\n> > -\tsize_t used_;\n> > -\n> > -\tstd::map> blocks_;\n> > };\n> > } /* namespace ipa::rkisp1 */\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 8945BBE173\n\tfor ;\n\tTue, 16 Sep 2025 07:50:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4AF766936F;\n\tTue, 16 Sep 2025 09:50:48 +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 8C64162C3B\n\tfor ;\n\tTue, 16 Sep 2025 09:50:45 +0200 (CEST)","from ideasonboard.com (mob-5-90-51-255.net.vodafone.it\n\t[5.90.51.255])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 31A84EC1;\n\tTue, 16 Sep 2025 09:49:27 +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=\"uHWSteR6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758008967;\n\tbh=W+Ii50xufuFZAAIkUe/evSEAwsAORXFxs9tIc859Ooc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uHWSteR6Y5u3AsW0AHdpBE7sqec/+1GbOllYpo4XxzUjTRuOvPYRu+otW4umJYKLV\n\tkVYvihSuJTC05t6Sfc+s0z8ETXt3tilbqDhDUwV+TjCkuLXNnEpqjto6YvC0Wun9V8\n\tL9yIljkDbRghUkKdM+v3RzWsaIUkx3SsAJVwGwGI=","Date":"Tue, 16 Sep 2025 09:50:41 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/4] ipa: libipa: Introduce V4L2Params","Message-ID":"<2lytfujt57qxype7akx5tyxufwfgevztqj6nqjpdw34qhhg5vo@wdgvykvddjfc>","References":"<20250829-v4l2-params-v1-0-340773fb69ff@ideasonboard.com>\n\t<20250829-v4l2-params-v1-3-340773fb69ff@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","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":35836,"web_url":"https://patchwork.libcamera.org/comment/35836/","msgid":"<484baec7-814f-43e7-84cf-75b9f8ed5a0a@ideasonboard.com>","date":"2025-09-16T10:48:27","subject":"Re: [PATCH 3/4] ipa: libipa: Introduce V4L2Params","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 09. 16. 9:50 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> thanks for review\n> \n> On Mon, Sep 08, 2025 at 06:09:08PM +0200, Barnabás Pőcze wrote:\n>> Hi\n>>\n>> 2025. 08. 29. 13:54 keltezéssel, Jacopo Mondi írta:\n>>> The existing RkISP1Params helper classes allows the RkISP1 to handle\n>>> V4L2 extensible parameters format and the legacy RkIPS1-specific\n>>> fixed-size parameters format.\n>>>\n>>> With the introduction of v4l2-params in the Linux kernel the part of\n>>> the RkISP1Params helper class that handles extensible parameters can\n>>> be generalized so that other IPA modules can use the same helpers\n>>> to populate a v4l2-params compatible buffer.\n>>>\n>>> Generalize the RkISP1Params class to a new libipa component named\n>>> V4L2Params and derive the existing RkISP1Params from it, leaving\n>>> in the RkISP1-specific implementation the handling of the legacy format.\n>>>\n>>> Deriving RkISP1Params from V4L2Params requires changing the size\n>>> associated to each block to include the size of v4l2_params_block_header\n>>> in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block()\n>>> implementation doesn't account for that as RkIS1Params::block()\n>>> implementation did.\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>> ---\n>>> src/ipa/libipa/meson.build | 2 +\n>>> src/ipa/libipa/v4l2_params.cpp | 252 +++++++++++++++++++++++++++++++++++++++++\n>>> src/ipa/libipa/v4l2_params.h | 135 ++++++++++++++++++++++\n>>> src/ipa/rkisp1/params.cpp | 93 +--------------\n>>> src/ipa/rkisp1/params.h | 108 ++++++++----------\n>>> 5 files changed, 438 insertions(+), 152 deletions(-)\n>>>\n>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>>> index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644\n>>> --- a/src/ipa/libipa/meson.build\n>>> +++ b/src/ipa/libipa/meson.build\n>>> @@ -16,6 +16,7 @@ libipa_headers = files([\n>>> 'lsc_polynomial.h',\n>>> 'lux.h',\n>>> 'module.h',\n>>> + 'v4l2_params.h',\n>>> 'pwl.h',\n>>> ])\n>>> @@ -35,6 +36,7 @@ libipa_sources = files([\n>>> 'lsc_polynomial.cpp',\n>>> 'lux.cpp',\n>>> 'module.cpp',\n>>> + 'v4l2_params.cpp',\n>>> 'pwl.cpp',\n>>> ])\n>>> diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp\n>>> new file mode 100644\n>>> index 0000000000000000000000000000000000000000..674018065ace0e1b6b48b1630e556cef590d1e84\n>>> --- /dev/null\n>>> +++ b/src/ipa/libipa/v4l2_params.cpp\n>>> @@ -0,0 +1,252 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2025, Ideas On Board\n>>> + *\n>>> + * V4L2 Parameters\n>>> + */\n>>> +\n>>> +#include \"v4l2_params.h\"\n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +namespace ipa {\n>>> +\n>>> +/**\n>>> + * \\file v4l2_params.cpp\n>>> + * \\brief Helper class to populate a v4l2-params compatible parameters buffer\n>>> + *\n>>> + * The Linux kernel defines a generic buffer format for configuring ISP devices\n>>> + * through a set of parameters in the form of V4L2 extensible parameters. The\n>>> + * V4L2 extensible parameters define a serialization format for ISP parameters\n>>> + * that allows userspace to populate a buffer of configuration data by appending\n>>> + * them one after the other in a binary buffer.\n>>> + *\n>>> + * Each ISP driver compatible with the v4l2-params format will define its own\n>>> + * meta-output format identifier and defines the types of the configuration data\n>>> + * of each ISP block that usually match the registers layout.\n>>> + *\n>>> + * The V4L2Params class represent the V4L2 extensible parameters buffer and\n>>> + * allows users to populate the ISP configuration blocks, represented by the\n>>> + * V4L2ParamBlock class instances.\n>>> + *\n>>> + * IPA implementations using this helpers should define an enumeration of ISP\n>>> + * blocks the IPA module supports and use a set of common abstraction to help\n>>> + * their derived implementation of V4L2Params translate the enumerated ISP block\n>>> + * identifier to the actual type of the configuration data as defined by the\n>>> + * kernel interface.\n>>> + *\n>>> + * As an example of this see the RkISP1 and Mali-C55 implementations.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\class V4L2ParamsBlock\n>>> + * \\brief Helper class that represents a ISP configuration block\n>>> + *\n>>> + * Each ISP function is associated with a set of configuration parameters\n>>> + * defined by the kernel interface.\n>>> + *\n>>> + * This class represents an ISP block configuration entry. It is constructed\n>>> + * with a reference to the memory area where the block configuration will be\n>>> + * stored in the parameters buffer. The template parameter represents\n>>> + * the underlying kernel-defined ISP block configuration type and allow its\n>>> + * user to easily cast it to said type to populate and read the configuration\n>>> + * parameters.\n>>> + *\n>>> + * \\sa V4L2Params::block()\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn V4L2ParamsBlock::V4L2ParamsBlock()\n>>> + * \\brief Construct a V4L2ParamsBlock with memory represented by \\a data\n>>> + * \\param[in] data A view on the memory area where the ISP block is located\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn V4L2ParamsBlock::setEnabled()\n>>> + * \\brief Enable/disable an ISP configuration block\n>>> + * \\param[in] enabled The enable flag\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn V4L2ParamsBlock::header()\n>>> + * \\brief Retrieve a reference to the header (struct v4l2_params_block_header)\n>>> + * \\return The block header\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn V4L2ParamsBlock::data()\n>>> + * \\brief Retrieve a reference to block configuration data memory area\n>>> + * \\return The block data\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn V4L2ParamsBlock::operator->()\n>>> + * \\brief Access the ISP configuration block casting it to the kernel-defined\n>>> + * ISP configuration type\n>>> + *\n>>> + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration\n>>> + * block type. This function allows users to easily cast a V4L2ParamsBlock to\n>>> + * the underlying kernel-defined type in order to easily populate or read\n>>> + * the ISP configuration data.\n>>> + *\n>>> + * \\code{.cpp}\n>>> + *\n>>> + * // The kernel header defines the ISP configuration types, in example\n>>> + * // struct my_isp_awb_config_data {\n>>> + * //\t\tu16 gain_ch00;\n>>> + * //\t\tu16 gain_ch01;\n>>> + * //\t\tu16 gain_ch10;\n>>> + * //\t\tu16 gain_ch11;\n>>> + * //\n>>> + * // }\n>>> + *\n>>> + * template<> V4L2ParamsBlock awbBlock\n>>> + *\n>>> + * awbBlock->gain_ch00 = ...;\n>>> + * awbBlock->gain_ch01 = ...;\n>>> + * awbBlock->gain_ch10 = ...;\n>>> + * awbBlock->gain_ch11 = ...;\n>>> + *\n>>> + * \\endcode\n>>> + *\n>>> + * Users of this class are not expected to create a V4L2ParamsBlock manually but\n>>> + * should rather use V4L2Params::block() to retrieve a reference to the memory\n>>> + * area used to construct a V4L2ParamsBlock in their overloaded\n>>> + * implementation of V4L2Params::block().\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn V4L2ParamsBlock::operator->() const\n>>> + * \\copydoc V4L2ParamsBlock::operator->()\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn V4L2ParamsBlock::operator*() const\n>>> + * \\copydoc V4L2ParamsBlock::operator->()\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn V4L2ParamsBlock::operator*()\n>>> + * \\copydoc V4L2ParamsBlock::operator->()\n>>> + */\n>>> +\n>>> + /**\n>>> + * \\class V4L2Params\n>>> + * \\brief Helper class that represent an ISP configuration buffer\n>>> + *\n>>> + * ISP implementation compatible with v4l2-params define their ISP configuration\n>>> + * buffer types compatible with the struct v4l2_params_buffer type.\n>>> + *\n>>> + * This class represents an ISP configuration buffer. It is constructed\n>>> + * with a reference to the memory mapped buffer that will be queued to the ISP.\n>>> + *\n>>> + * This class is templated with the type of the enumeration of ISP blocks that\n>>> + * each IPA module is expected to support. IPA modules are expected to derive\n>>> + * this class and use the V4L2Params::block() function to retrieve the memory\n>>> + * area for each ISP configuration block and use it to construct a\n>>> + * V4L2ParamsBlock with it before returning it to the user.\n>>> + *\n>>> + * \\code{.cpp}\n>>> + *\n>>> + * enum class myISPBlocks {\n>>> + *\tAgc,\n>>> + *\tAwb,\n>>> + *\t...\n>>> + * };\n>>> + *\n>>> + * template\n>>> + * struct block_type {\n>>> + * };\n>>> + *\n>>> + * template<>\n>>> + * struct block_type {\n>>> + *\tusing type = struct my_isp_kernel_config_type_agc;\n>>> + * };\n>>> + *\n>>> + * template<>\n>>> + * struct block_type {\n>>> + *\tusing type = struct my_isp_kernel_config_type_awb;\n>>> + * };\n>>> + *\n>>> + * ...\n>>> + *\n>>> + * class MyISPParams : public V4L2Params\n>>> + * {\n>>> + * public:\n>>> + * \ttemplate\n>>> + * \tauto block()\n>>> + * \t{\n>>> + *\n>>> + * \t\t// Use the kernel defined configuration type as template\n>>> + * \t\t// argument to V4L2ParamsBlock.\n>>> + * \t\tusing Type = typename details::block_type::type;\n>>> + *\n>>> + * \t\t// Each IPA module should provide the information required\n>>> + * \t\t// to populate the block header\n>>> + *\n>>> + * \t\t...\n>>> + *\n>>> + * \t\tauto data = V4L2Params::block(B, blockType, blockSize);\n>>> + *\n>>> + * \t\treturn V4L2ParamsBlock(data);\n>>> + * \t}\n>>> + * };\n>>> + *\n>>> + * \\endcode\n>>> + *\n>>> + * As an example, see the RkISP1Params and MaliC55Params implementations.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn V4L2Params::V4L2Params()\n>>> + * \\brief Construct a V4L2Params\n>>> + * \\param[in] data Reference to the v4l2-buffer memory mapped area\n>>> + * \\param[in] version The ISP parameters version the implementation supports\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn V4L2Params::size()\n>>> + * \\brief Retrieve the used size of the parameters buffer (in bytes)\n>>> + *\n>>> + * The parameters buffer size is mostly used to populate the v4l2_buffer\n>>> + * bytesused field before queueing the buffer to the ISP.\n>>> + *\n>>> + * \\return The number of bytes occupied by the ISP configuration parameters\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn V4L2Params::block()\n>>> + * \\brief Populate an ISP configuration block a returns a reference to its\n>>> + * memory\n>>> + * \\param[in] type The ISP block identifier enumerated by the IPA module\n>>> + * \\param[in] blockType The kernel-defined ISP block identifier, used to\n>>> + * populate the block header\n>>> + * \\param[in] blockSize The ISP block size, used to populate the block header\n>>> + *\n>>> + *\n>>> + * Initialize the block header with \\a blockType and \\a blockSize and\n>>> + * returns a reference to the memory used to store an ISP configuration block.\n>>> + *\n>>> + * IPA modules that derive the V4L2Params class shall use this function to\n>>> + * retrieve the memory area that will be used to construct a V4L2ParamsBlock\n>>> + * before returning it to the caller.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var V4L2Params::data_\n>>> + * \\brief The ISP parameters buffer memory\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var V4L2Params::used_\n>>> + * \\brief The number of bytes used in the parameters buffer\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\var V4L2Params::blocks_\n>>> + * \\brief Cache of ISP configuration blocks\n>>> + */\n>>> +\n>>> +} /* namespace ipa */\n>>> +\n>>> +} /* namespace libcamera */\n>>> diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h\n>>> new file mode 100644\n>>> index 0000000000000000000000000000000000000000..5586096c7ee8a2d20877838564e8074e0fc3d1ce\n>>> --- /dev/null\n>>> +++ b/src/ipa/libipa/v4l2_params.h\n>>> @@ -0,0 +1,135 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2025, Ideas On Board\n>>> + *\n>>> + * V4L2 Parameters\n>>> + */\n>>> +\n>>> +#pragma once\n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>> +\n>>> +#include \n>>> +#include \n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +namespace ipa {\n>>> +\n>>> +template\n>>> +class V4L2ParamsBlock\n>>> +{\n>>> +public:\n>>> +\tV4L2ParamsBlock(const Span &data)\n>>\n>> I think taking by value is fine, no need for the const ref.\n>>\n> \n> right indeed\n> \n>>\n>>> +\t{\n>>> +\t\theader_ = data.subspan(0, sizeof(v4l2_params_block_header));\n>>> +\t\tdata_ = data.subspan(sizeof(v4l2_params_block_header));\n>>\n>> I'd probably do these in the member init list.\n>>\n> \n> is this just a style preference or are there reasons I'm missing ?\n\nThere is a difference: one is an assignment, one is a constructor call;\nso I always try to use the member init list. Although the difference does\nnot really matter much here, so you could say it's a preference thing.\n\n\n> \n>>\n>>> +\t}\n>>> +\n>>> +\tvoid setEnabled(bool enabled)\n>>> +\t{\n>>> +\t\tstruct v4l2_params_block_header *header =\n>>> +\t\t\treinterpret_cast(header_.data());\n>>> +\n>>> +\t\theader->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE |\n>>> +\t\t\t\t V4L2_PARAMS_FL_BLOCK_DISABLE);\n>>> +\t\theader->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE\n>>> +\t\t\t\t\t : V4L2_PARAMS_FL_BLOCK_DISABLE;\n>>> +\t}\n>>> +\n>>> +\tSpan header() const { return header_; }\n>>> +\tSpan data() const { return data_; }\n>>> +\n>>> +\tconst T *operator->() const\n>>> +\t{\n>>> +\t\treturn reinterpret_cast(data().data());\n>>> +\t}\n>>> +\n>>> +\tT *operator->()\n>>> +\t{\n>>> +\t\treturn reinterpret_cast(data().data());\n>>> +\t}\n>>> +\n>>> +\tconst T &operator*() const\n>>> +\t{\n>>> +\t\treturn *reinterpret_cast(data().data());\n>>> +\t}\n>>> +\n>>> +\tT &operator*()\n>>> +\t{\n>>> +\t\treturn *reinterpret_cast(data().data());\n>>> +\t}\n>>> +\n>>> +private:\n>>> +\tLIBCAMERA_DISABLE_COPY(V4L2ParamsBlock)\n>>\n>> Why disable? I would assume that it's OK to copy this?\n>>\n> \n> tbh I think I just copied this from RkISP1Params.\n> \n> However, why would one copy an instance of V4L2ParamsBlock ?\n\nMy thinking was that it is a \"reference-like type\", so there is no reason to\nprevent copying since it works and might be useful in some cases (e.g. passing\nto functions, etc.).\n\n\n> \n>>\n>>> +\n>>> +\tSpan header_;\n>>> +\tSpan data_;\n>>> +};\n>>> +\n>>> +template\n>>> +class V4L2Params\n>>> +{\n>>> +public:\n>>> +\tV4L2Params(Span data, unsigned int version)\n>>> +\t\t: data_(data)\n>>> +\t{\n>>> +\t\tstruct v4l2_params_buffer *cfg =\n>>> +\t\t\treinterpret_cast(data_.data());\n>>> +\t\tcfg->data_size = 0;\n>>> +\t\tcfg->version = version;\n>>> +\t\tused_ = offsetof(struct v4l2_params_buffer, data);\n>>> +\t}\n>>> +\n>>> +\tsize_t size() const { return used_; }\n>>> +\n>>> +protected:\n>>> +\tSpan block(T type, unsigned int blockType, size_t blockSize)\n>>\n>> This can modified so that the derived classes need not implement any extra logic.\n>> One option is to pass the `details::block_type` template as a template argument\n>> to V4L2Params and then use that to map the id to the type:\n>>\n>> template typename IdToType>\n>> class V4L2Params {\n>>\n>> template\n>> auto block() {\n>> using Type = typename IdToType::type;\n>> ...\n>> }\n>>\n>> but I think it's simpler and more extensible to pass another type that holds\n>> all this compile-time information. It could look something like this:\n>>\n>> diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h\n>> index 5586096c7..019f06509 100644\n>> --- a/src/ipa/libipa/v4l2_params.h\n>> +++ b/src/ipa/libipa/v4l2_params.h\n>> @@ -71,7 +71,7 @@ private:\n>> \tSpan data_;\n>> };\n>> -template\n>> +template\n>> class V4L2Params\n>> {\n>> public:\n>> @@ -87,8 +87,21 @@ public:\n>> \tsize_t size() const { return used_; }\n>> +\ttemplate\n>> +\tauto block()\n>> +\t{\n>> +\t\tusing Details = typename Traits::template id_to_details;\n> \n> I can't grok 'typename Traits::template'\n> \n> but it's ok, I'll trust you here\n\nSee https://www.en.cppreference.com/w/cpp/language/dependent_name.html\nunder \"The template disambiguator for dependent names\". It just tells\nthe compiler that `id_to_details` is a template.\n\nThis is quite similar to having to use the `typename` keyword in some cases.\n(see \"The typename disambiguator for dependent names\" on the linked site)\n\n\n> \n>> +\n>> +\t\tusing Type = typename Details::type;\n>> +\t\tconstexpr auto kernelId = Details::blockType;\n>> +\n>> +\t\tauto data = block(Id, kernelId, sizeof(Type));\n>> +\n>> +\t\treturn V4L2ParamsBlock(data);\n>> +\t}\n>> +\n>> protected:\n>> -\tSpan block(T type, unsigned int blockType, size_t blockSize)\n>> +\tSpan block(typename Traits::id_type type, unsigned int blockType, size_t blockSize)\n>> \t{\n>> \t\t/*\n>> \t\t * Look up the block in the cache first. If an algorithm\n>> @@ -127,7 +140,7 @@ protected:\n>> \tSpan data_;\n>> \tsize_t used_;\n>> -\tstd::map> blocks_;\n>> +\tstd::map> blocks_;\n>> };\n>> } /* namespace ipa */\n>> diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h\n>> index 1cc56dbad..975122999 100644\n>> --- a/src/ipa/mali-c55/params.h\n>> +++ b/src/ipa/mali-c55/params.h\n>> @@ -58,30 +58,24 @@ MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig, mesh_shading_config,\n>> MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel, mesh_shading_selection,\n>> \t\t\t MESH_SHADING_SELECTION)\n>> +struct params_traits {\n>> +\tusing id_type = MaliC55Blocks;\n>> +\n>> +\ttemplate\n>> +\tusing id_to_details = block_type;\n>> +};\n>> +\n>> } /* namespace details */\n>> -class MaliC55Params : public V4L2Params\n>> +class MaliC55Params : public V4L2Params\n>> {\n>> public:\n>> \tstatic constexpr unsigned int kVersion = MALI_C55_PARAM_BUFFER_V1;\n>> \tMaliC55Params(Span data)\n>> -\t\t: V4L2Params(data, kVersion)\n>> +\t\t: V4L2Params(data, kVersion)\n>> \t{\n>> \t}\n>> -\n>> -\ttemplate\n>> -\tauto block()\n>> -\t{\n>> -\t\tusing Type = typename details::block_type::type;\n>> -\n>> -\t\tauto blockType = details::block_type::blockType;\n>> -\t\tsize_t blockSize = sizeof(Type);\n>> -\n>> -\t\tauto data = V4L2Params::block(B, blockType, blockSize);\n>> -\n>> -\t\treturn V4L2ParamsBlock(data);\n>> -\t}\n> \n> Thanks!\n> \n> 'struct param_traits' and the associated changes in V4L2Params allowed\n> me to simplify the MaliC55 implementation as you suggested\n> \n>> };\n>> } /* namespace ipa::mali_c55 */\n>> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n>> index 7a2127664..824a5924f 100644\n>> --- a/src/ipa/rkisp1/params.h\n>> +++ b/src/ipa/rkisp1/params.h\n>> @@ -45,45 +45,54 @@ template\n>> struct block_type {\n>> };\n>> -#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)\t\t\\\n>> +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id)\t\t\\\n>> template<>\t\t\t\t\t\t\t\t\\\n>> struct block_type {\t\t\t\t\\\n>> \tusing type = struct rkisp1_cif_isp_##blockStruct##_config;\t\\\n>> +\tstatic constexpr rkisp1_ext_params_block_type blockType =\t\\\n>> +\t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id;\t\t\t\\\n>> };\n>> -RKISP1_DEFINE_BLOCK_TYPE(Bls, bls)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg)\n>> -RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Flt, flt)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Goc, goc)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf)\n>> -RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Ie, ie)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Hst, hst)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Aec, aec)\n>> -RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)\n>> -RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)\n>> -RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)\n>> -RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Bls, bls, BLS)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc, DPCC)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg, SDG)\n>> +RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain, AWB_GAIN)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Flt, flt, FLT)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm, BDM)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk, CTK)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Goc, goc, GOC)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf, DPF)\n>> +RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength, DPF_STRENGTH)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc, CPROC)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Ie, ie, IE)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc, LSC)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas, AWB_MEAS)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Hst, hst, HST_MEAS)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Aec, aec, AEC_MEAS)\n>> +RKISP1_DEFINE_BLOCK_TYPE(Afc, afc, AFC_MEAS)\n>> +RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls, COMPAND_BLS)\n>> +RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve, COMPAND_EXPAND)\n>> +RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve, COMPAND_COMPRESS)\n>> +\n>> +struct params_traits {\n>> +\tusing id_type = BlockType;\n>> +\n>> +\ttemplate\n>> +\tusing id_to_details = block_type;\n>> +};\n>> } /* namespace details */\n>> template\n>> class RkISP1ParamsBlock;\n>> -class RkISP1Params : public V4L2Params\n>> +class RkISP1Params : public V4L2Params\n>> {\n>> public:\n>> \tstatic constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;\n>> \tRkISP1Params(uint32_t format, Span data)\n>> -\t\t: V4L2Params(data, kVersion), format_(format)\n>> +\t\t: V4L2Params(data, kVersion), format_(format)\n>> \t{\n>> \t\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n>> \t\t\tmemset(data.data(), 0, data.size());\n>>\n>>\n>> Admittedly I haven't tested it, and the rkisp1 parts are not there yet.\n>> But I believe this direction could simplify the code and make it easier\n>> to implement for other parameter sets.\n> \n> Please note that for RkISP1 I had to keep an override for\n> RkISP1Params::block() as we need to support both legacy and extensible\n> \n>>\n>> For rkisp1, I think as a first step `kBlockTypeInfo` should be moved completely\n>> into `rkisp1::details::block_type` template specializations to do the mapping\n>> at compile time, similarly to how the proposed `MaliC55Params` does it.\n> \n> yeah, I don't like that map too much. But, if I may use it as an\n> excuse, it was there already so we're not doing any worse.\n> \n> I could have removed it like it's done for Mali, but the 'enableBit'\n> used by setEnabled() to support the legacy format made it more\n> complicated getting rid of it (I've not invested much time in trying,\n> admittedly).\n> \n> Anyway, could easily be done on top ?\n\nYes.\n\n\n> \n>>\n>> Then I believe one could realistically extend this `V4L2Params` to even support\n>> fixed-offset formats generically, if needed, but that might be an overkill.\n> \n> Not sure I got what you mean here,\n\nWhat I meant is that it is probably possible to support \"legacy\" parameter formats\nlike the rkisp1 one in the base template. But if that's the only one that is\nrelevant for us, then it's very likely an overkill.\n\n\n>>\n>>\n>>> +\t{\n>>> +\t\t/*\n>>> +\t\t * Look up the block in the cache first. If an algorithm\n>>> +\t\t * requests the same block type twice, it should get the same\n>>> +\t\t * block.\n>>> +\t\t */\n>>> +\t\tauto cacheIt = blocks_.find(type);\n>>> +\t\tif (cacheIt != blocks_.end())\n>>> +\t\t\treturn cacheIt->second;\n>>> +\n>>> +\t\t/* Make sure we don't run out of space. */\n>>> +\t\tif (blockSize > data_.size() - used_)\n>>> +\t\t\treturn {};\n>>> +\n>>> +\t\t/* Allocate a new block, clear its memory, and initialize its header. */\n>>> +\t\tSpan block = data_.subspan(used_, blockSize);\n>>> +\t\tused_ += blockSize;\n>>> +\n>>> +\t\tstruct v4l2_params_buffer *cfg =\n>>> +\t\t\treinterpret_cast(data_.data());\n>>> +\t\tcfg->data_size += blockSize;\n>>> +\n>>> +\t\tmemset(block.data(), 0, block.size());\n>>> +\n>>> +\t\tstruct v4l2_params_block_header *header =\n>>> +\t\t\treinterpret_cast(block.data());\n>>> +\t\theader->type = blockType;\n>>> +\t\theader->size = block.size();\n>>> +\n>>> +\t\t/* Update the cache. */\n>>> +\t\tblocks_[type] = block;\n>>> +\n>>> +\t\treturn block;\n>>> +\t}\n>>> +\n>>> +\tSpan data_;\n>>> +\tsize_t used_;\n>>> +\n>>> +\tstd::map> blocks_;\n>>> +};\n>>> +\n>>> +} /* namespace ipa */\n>>> +\n>>> +} /* namespace libcamera */\n>>> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n>>> index 4c0b051ce65da1686323ee9c66b82e12669a754d..2b692e1a1f199d6c118af0938e1aaecef6186a2c 100644\n>>> --- a/src/ipa/rkisp1/params.cpp\n>>> +++ b/src/ipa/rkisp1/params.cpp\n>>> @@ -35,7 +35,7 @@ struct BlockTypeInfo {\n>>> #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)\t\t\t\\\n>>> \t{ BlockType::block, {\t\t\t\t\t\t\t\\\n>>> \t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n>>> -\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n>>> +\t\tsizeof(struct rkisp1_ext_params_##type##_config),\t\t\\\n>>> \t\toffsetof(struct rkisp1_params_cfg, category.type##_config),\t\\\n>>> \t\tRKISP1_CIF_ISP_MODULE_##bit,\t\t\t\t\t\\\n>>> \t} }\n>>> @@ -49,7 +49,7 @@ struct BlockTypeInfo {\n>>> #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)\t\t\t\t\\\n>>> \t{ BlockType::block, {\t\t\t\t\t\t\t\\\n>>> \t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n>>> -\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n>>> +\t\tsizeof(struct rkisp1_ext_params_##type##_config),\t\t\\\n>>> \t\t0, 0,\t\t\t\t\t\t\t\t\\\n>>> \t} }\n>>> @@ -78,56 +78,6 @@ const std::map kBlockTypeInfo = {\n>>> } /* namespace */\n>>> -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n>>> -\t\t\t\t\t const Span &data)\n>>> -\t: params_(params), type_(type)\n>>> -{\n>>> -\tif (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n>>> -\t\theader_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n>>> -\t\tdata_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n>>> -\t} else {\n>>> -\t\tdata_ = data;\n>>> -\t}\n>>> -}\n>>> -\n>>> -void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n>>> -{\n>>> -\t/*\n>>> -\t * For the legacy fixed format, blocks are enabled in the top-level\n>>> -\t * header. Delegate to the RkISP1Params class.\n>>> -\t */\n>>> -\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n>>> -\t\treturn params_->setBlockEnabled(type_, enabled);\n>>> -\n>>> -\t/*\n>>> -\t * For the extensible format, set the enable and disable flags in the\n>>> -\t * block header directly.\n>>> -\t */\n>>> -\tstruct rkisp1_ext_params_block_header *header =\n>>> -\t\treinterpret_cast(header_.data());\n>>> -\theader->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |\n>>> -\t\t\t RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE);\n>>> -\theader->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE\n>>> -\t\t\t\t : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE;\n>>> -}\n>>> -\n>>> -RkISP1Params::RkISP1Params(uint32_t format, Span data)\n>>> -\t: format_(format), data_(data), used_(0)\n>>> -{\n>>> -\tif (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n>>> -\t\tstruct rkisp1_ext_params_cfg *cfg =\n>>> -\t\t\treinterpret_cast(data.data());\n>>> -\n>>> -\t\tcfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n>>> -\t\tcfg->data_size = 0;\n>>> -\n>>> -\t\tused_ += offsetof(struct rkisp1_ext_params_cfg, data);\n>>> -\t} else {\n>>> -\t\tmemset(data.data(), 0, data.size());\n>>> -\t\tused_ = sizeof(struct rkisp1_params_cfg);\n>>> -\t}\n>>> -}\n>>> -\n>>> void RkISP1Params::setBlockEnabled(BlockType type, bool enabled)\n>>> {\n>>> \tconst BlockTypeInfo &info = kBlockTypeInfo.at(type);\n>>> @@ -177,44 +127,7 @@ Span RkISP1Params::block(BlockType type)\n>>> \t\treturn data_.subspan(info.offset, info.size);\n>>> \t}\n>>> -\t/*\n>>> -\t * For the extensible format, allocate memory for the block, including\n>>> -\t * the header. Look up the block in the cache first. If an algorithm\n>>> -\t * requests the same block type twice, it should get the same block.\n>>> -\t */\n>>> -\tauto cacheIt = blocks_.find(type);\n>>> -\tif (cacheIt != blocks_.end())\n>>> -\t\treturn cacheIt->second;\n>>> -\n>>> -\t/* Make sure we don't run out of space. */\n>>> -\tsize_t size = sizeof(struct rkisp1_ext_params_block_header)\n>>> -\t\t + ((info.size + 7) & ~7);\n>>> -\tif (size > data_.size() - used_) {\n>>> -\t\tLOG(RkISP1Params, Error)\n>>> -\t\t\t<< \"Out of memory to allocate block type \"\n>>> -\t\t\t<< utils::to_underlying(type);\n>>> -\t\treturn {};\n>>> -\t}\n>>> -\n>>> -\t/* Allocate a new block, clear its memory, and initialize its header. */\n>>> -\tSpan block = data_.subspan(used_, size);\n>>> -\tused_ += size;\n>>> -\n>>> -\tstruct rkisp1_ext_params_cfg *cfg =\n>>> -\t\treinterpret_cast(data_.data());\n>>> -\tcfg->data_size += size;\n>>> -\n>>> -\tmemset(block.data(), 0, block.size());\n>>> -\n>>> -\tstruct rkisp1_ext_params_block_header *header =\n>>> -\t\treinterpret_cast(block.data());\n>>> -\theader->type = info.type;\n>>> -\theader->size = block.size();\n>>> -\n>>> -\t/* Update the cache. */\n>>> -\tblocks_[type] = block;\n>>> -\n>>> -\treturn block;\n>>> +\treturn V4L2Params::block(type, info.type, info.size);\n>>> }\n>>> } /* namespace ipa::rkisp1 */\n>>> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n>>> index 40450e34497a3aa71b5b0cda2bf045a1cc0e012f..7a21276648162a127317c75cb01d1c0059b96ee1 100644\n>>> --- a/src/ipa/rkisp1/params.h\n>>> +++ b/src/ipa/rkisp1/params.h\n>>> @@ -7,13 +7,10 @@\n>>> #pragma once\n>>> -#include \n>>> -#include \n>>> -\n>>> #include \n>>> +#include \n>>> -#include \n>>> -#include \n>>> +#include \n>>> namespace libcamera {\n>>> @@ -77,85 +74,72 @@ RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n>>> } /* namespace details */\n>>> -class RkISP1Params;\n>>> +template\n>>> +class RkISP1ParamsBlock;\n>>> -class RkISP1ParamsBlockBase\n>>> +class RkISP1Params : public V4L2Params\n>>> {\n>>> public:\n>>> -\tRkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n>>> -\t\t\t const Span &data);\n>>> -\n>>> -\tSpan data() const { return data_; }\n>>> -\n>>> -\tvoid setEnabled(bool enabled);\n>>> +\tstatic constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;\n>>> -private:\n>>> -\tLIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)\n>>> -\n>>> -\tRkISP1Params *params_;\n>>> -\tBlockType type_;\n>>> -\tSpan header_;\n>>> -\tSpan data_;\n>>> -};\n>>> -\n>>> -template\n>>> -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n>>> -{\n>>> -public:\n>>> -\tusing Type = typename details::block_type::type;\n>>> -\n>>> -\tRkISP1ParamsBlock(RkISP1Params *params, const Span &data)\n>>> -\t\t: RkISP1ParamsBlockBase(params, B, data)\n>>> +\tRkISP1Params(uint32_t format, Span data)\n>>> +\t\t: V4L2Params(data, kVersion), format_(format)\n>>> \t{\n>>> +\t\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n>>> +\t\t\tmemset(data.data(), 0, data.size());\n>>> +\t\t\tused_ = sizeof(struct rkisp1_params_cfg);\n>>> +\t\t}\n>>> \t}\n>>> -\tconst Type *operator->() const\n>>> +\ttemplate\n>>> +\tauto block()\n>>> \t{\n>>> -\t\treturn reinterpret_cast(data().data());\n>>> -\t}\n>>> +\t\tusing Type = typename details::block_type::type;\n>>> -\tType *operator->()\n>>> -\t{\n>>> -\t\treturn reinterpret_cast(data().data());\n>>> +\t\treturn RkISP1ParamsBlock(this, B, block(B));\n>>> \t}\n>>> -\tconst Type &operator*() const &\n>>> -\t{\n>>> -\t\treturn *reinterpret_cast(data().data());\n>>> -\t}\n>>> +\tuint32_t format() const { return format_; }\n>>> +\tvoid setBlockEnabled(BlockType type, bool enabled);\n>>> -\tType &operator*() &\n>>> -\t{\n>>> -\t\treturn *reinterpret_cast(data().data());\n>>> -\t}\n>>> +private:\n>>> +\tSpan block(BlockType type);\n>>> +\n>>> +\tuint32_t format_;\n>>> };\n>>> -class RkISP1Params\n>>> +template\n>>> +class RkISP1ParamsBlock : public V4L2ParamsBlock\n>>> {\n>>> public:\n>>> -\tRkISP1Params(uint32_t format, Span data);\n>>> -\n>>> -\ttemplate\n>>> -\tRkISP1ParamsBlock block()\n>>> +\tRkISP1ParamsBlock(RkISP1Params *params, BlockType type,\n>>> +\t\t\t const Span &data)\n>>> +\t\t: V4L2ParamsBlock(data)\n>>> \t{\n>>> -\t\treturn RkISP1ParamsBlock(this, block(B));\n>>> +\t\tparams_ = params;\n>>> +\t\ttype_ = type;\n>>> +\n>>> +\t\t/* Legacy param format has no header */\n>>> +\t\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n>>> +\t\t\tdata_ = data;\n>>> \t}\n>>> -\tuint32_t format() const { return format_; }\n>>> -\tsize_t size() const { return used_; }\n>>> +\tvoid setEnabled(bool enabled)\n>>> +\t{\n>>> +\t\t/*\n>>> +\t\t * For the legacy fixed format, blocks are enabled in the\n>>> +\t\t * top-level header. Delegate to the RkISP1Params class.\n>>> +\t\t */\n>>> +\t\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n>>> +\t\t\treturn params_->setBlockEnabled(type_, enabled);\n>>> +\n>>> +\t\treturn V4L2ParamsBlock::setEnabled(enabled);\n>>> +\t}\n>>\n>> I have one concern regarding this design. An `RkISP1ParamsBlock` instance\n>> will not work as its base type correctly in all cases. That is, calling the\n>> base class `setEnabled()`:\n>>\n>> RkISP1ParamsBlock<...> *p = ...;\n>>\n>> p->V4L2ParamsBlock<...>::setEnabled(...);\n>> // or\n>> static_cast *>(p)->setEnabled(...);\n>>\n>> will not work correctly if the format is the \"non-extensible\" one.\n> \n> The only user of this class is the RkISP1 IPA, why would it forcefully\n> use the base class implementation ?\n\nIt probably wouldn't, I just wanted to make a note of this. Although I can\nimagine that with time some parts would want to operate on `V4L2ParamsBlock`\nobjects directly, in a generic fashion, and then this might be an issue.\n\n\n> \n> If there's an easy way to avoid the above, I'll anyway happily consider\n> it.\n\nSadly I don't think I have an easy solution here. I'll take a look again\nat the next version.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Thanks\n> j\n> \n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>>> private:\n>>> -\tfriend class RkISP1ParamsBlockBase;\n>>> -\n>>> -\tSpan block(BlockType type);\n>>> -\tvoid setBlockEnabled(BlockType type, bool enabled);\n>>> -\n>>> -\tuint32_t format_;\n>>> -\n>>> +\tRkISP1Params *params_;\n>>> +\tBlockType type_;\n>>> \tSpan data_;\n>>> -\tsize_t used_;\n>>> -\n>>> -\tstd::map> blocks_;\n>>> };\n>>> } /* namespace ipa::rkisp1 */\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 A334EBE173\n\tfor ;\n\tTue, 16 Sep 2025 10:48:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8231F6936F;\n\tTue, 16 Sep 2025 12:48:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 59B6D613A1\n\tfor ;\n\tTue, 16 Sep 2025 12:48:31 +0200 (CEST)","from [192.168.33.22] (185.221.142.115.nat.pool.zt.hu\n\t[185.221.142.115])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C7902E8A;\n\tTue, 16 Sep 2025 12:47:12 +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=\"iTnx6SLi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758019632;\n\tbh=b1lz8DLj+y0lZ4r1uS3yBJS+YmbDrPb0Zg6EIwi44yA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=iTnx6SLit8x/llSe3B/n6mBrNYgOhdOvdsiNSd2athgBg9bF3JSznjkoNeacuR04p\n\tqSyROG2HN1YV4kosk9QeU1Fb/NL/ylwRSZUgTs5ZcDNL3FIYkeCUnsAhUO3C5O7Rf0\n\tB84crOAwQgKGC2Zk3Eh1nIHaGyeAppVmKWcFQsZM=","Message-ID":"<484baec7-814f-43e7-84cf-75b9f8ed5a0a@ideasonboard.com>","Date":"Tue, 16 Sep 2025 12:48:27 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 3/4] ipa: libipa: Introduce V4L2Params","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250829-v4l2-params-v1-0-340773fb69ff@ideasonboard.com>\n\t<20250829-v4l2-params-v1-3-340773fb69ff@ideasonboard.com>\n\t\n\t<2lytfujt57qxype7akx5tyxufwfgevztqj6nqjpdw34qhhg5vo@wdgvykvddjfc>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<2lytfujt57qxype7akx5tyxufwfgevztqj6nqjpdw34qhhg5vo@wdgvykvddjfc>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":35841,"web_url":"https://patchwork.libcamera.org/comment/35841/","msgid":"","date":"2025-09-16T11:53:23","subject":"Re: [PATCH 3/4] ipa: libipa: Introduce V4L2Params","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Tue, Sep 16, 2025 at 12:48:27PM +0200, Barnabás Pőcze wrote:\n> Hi\n>\n>\n> 2025. 09. 16. 9:50 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> > thanks for review\n> >\n> > On Mon, Sep 08, 2025 at 06:09:08PM +0200, Barnabás Pőcze wrote:\n> > > Hi\n> > >\n> > > 2025. 08. 29. 13:54 keltezéssel, Jacopo Mondi írta:\n> > > > The existing RkISP1Params helper classes allows the RkISP1 to handle\n> > > > V4L2 extensible parameters format and the legacy RkIPS1-specific\n> > > > fixed-size parameters format.\n> > > >\n> > > > With the introduction of v4l2-params in the Linux kernel the part of\n> > > > the RkISP1Params helper class that handles extensible parameters can\n> > > > be generalized so that other IPA modules can use the same helpers\n> > > > to populate a v4l2-params compatible buffer.\n> > > >\n> > > > Generalize the RkISP1Params class to a new libipa component named\n> > > > V4L2Params and derive the existing RkISP1Params from it, leaving\n> > > > in the RkISP1-specific implementation the handling of the legacy format.\n> > > >\n> > > > Deriving RkISP1Params from V4L2Params requires changing the size\n> > > > associated to each block to include the size of v4l2_params_block_header\n> > > > in the ipa:rkisp1::kBlockTypeInfo map as the V4L2Params::block()\n> > > > implementation doesn't account for that as RkIS1Params::block()\n> > > > implementation did.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/ipa/libipa/meson.build | 2 +\n> > > > src/ipa/libipa/v4l2_params.cpp | 252 +++++++++++++++++++++++++++++++++++++++++\n> > > > src/ipa/libipa/v4l2_params.h | 135 ++++++++++++++++++++++\n> > > > src/ipa/rkisp1/params.cpp | 93 +--------------\n> > > > src/ipa/rkisp1/params.h | 108 ++++++++----------\n> > > > 5 files changed, 438 insertions(+), 152 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> > > > index 660be94054fa98b714b6bc586039081e45a6b4bc..4010739e710eb38aa6108eb8258c574a616bf3c0 100644\n> > > > --- a/src/ipa/libipa/meson.build\n> > > > +++ b/src/ipa/libipa/meson.build\n> > > > @@ -16,6 +16,7 @@ libipa_headers = files([\n> > > > 'lsc_polynomial.h',\n> > > > 'lux.h',\n> > > > 'module.h',\n> > > > + 'v4l2_params.h',\n> > > > 'pwl.h',\n> > > > ])\n> > > > @@ -35,6 +36,7 @@ libipa_sources = files([\n> > > > 'lsc_polynomial.cpp',\n> > > > 'lux.cpp',\n> > > > 'module.cpp',\n> > > > + 'v4l2_params.cpp',\n> > > > 'pwl.cpp',\n> > > > ])\n> > > > diff --git a/src/ipa/libipa/v4l2_params.cpp b/src/ipa/libipa/v4l2_params.cpp\n> > > > new file mode 100644\n> > > > index 0000000000000000000000000000000000000000..674018065ace0e1b6b48b1630e556cef590d1e84\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/v4l2_params.cpp\n> > > > @@ -0,0 +1,252 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2025, Ideas On Board\n> > > > + *\n> > > > + * V4L2 Parameters\n> > > > + */\n> > > > +\n> > > > +#include \"v4l2_params.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa {\n> > > > +\n> > > > +/**\n> > > > + * \\file v4l2_params.cpp\n> > > > + * \\brief Helper class to populate a v4l2-params compatible parameters buffer\n> > > > + *\n> > > > + * The Linux kernel defines a generic buffer format for configuring ISP devices\n> > > > + * through a set of parameters in the form of V4L2 extensible parameters. The\n> > > > + * V4L2 extensible parameters define a serialization format for ISP parameters\n> > > > + * that allows userspace to populate a buffer of configuration data by appending\n> > > > + * them one after the other in a binary buffer.\n> > > > + *\n> > > > + * Each ISP driver compatible with the v4l2-params format will define its own\n> > > > + * meta-output format identifier and defines the types of the configuration data\n> > > > + * of each ISP block that usually match the registers layout.\n> > > > + *\n> > > > + * The V4L2Params class represent the V4L2 extensible parameters buffer and\n> > > > + * allows users to populate the ISP configuration blocks, represented by the\n> > > > + * V4L2ParamBlock class instances.\n> > > > + *\n> > > > + * IPA implementations using this helpers should define an enumeration of ISP\n> > > > + * blocks the IPA module supports and use a set of common abstraction to help\n> > > > + * their derived implementation of V4L2Params translate the enumerated ISP block\n> > > > + * identifier to the actual type of the configuration data as defined by the\n> > > > + * kernel interface.\n> > > > + *\n> > > > + * As an example of this see the RkISP1 and Mali-C55 implementations.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\class V4L2ParamsBlock\n> > > > + * \\brief Helper class that represents a ISP configuration block\n> > > > + *\n> > > > + * Each ISP function is associated with a set of configuration parameters\n> > > > + * defined by the kernel interface.\n> > > > + *\n> > > > + * This class represents an ISP block configuration entry. It is constructed\n> > > > + * with a reference to the memory area where the block configuration will be\n> > > > + * stored in the parameters buffer. The template parameter represents\n> > > > + * the underlying kernel-defined ISP block configuration type and allow its\n> > > > + * user to easily cast it to said type to populate and read the configuration\n> > > > + * parameters.\n> > > > + *\n> > > > + * \\sa V4L2Params::block()\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2ParamsBlock::V4L2ParamsBlock()\n> > > > + * \\brief Construct a V4L2ParamsBlock with memory represented by \\a data\n> > > > + * \\param[in] data A view on the memory area where the ISP block is located\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2ParamsBlock::setEnabled()\n> > > > + * \\brief Enable/disable an ISP configuration block\n> > > > + * \\param[in] enabled The enable flag\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2ParamsBlock::header()\n> > > > + * \\brief Retrieve a reference to the header (struct v4l2_params_block_header)\n> > > > + * \\return The block header\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2ParamsBlock::data()\n> > > > + * \\brief Retrieve a reference to block configuration data memory area\n> > > > + * \\return The block data\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2ParamsBlock::operator->()\n> > > > + * \\brief Access the ISP configuration block casting it to the kernel-defined\n> > > > + * ISP configuration type\n> > > > + *\n> > > > + * The V4L2ParamsBlock is templated with the kernel defined ISP configuration\n> > > > + * block type. This function allows users to easily cast a V4L2ParamsBlock to\n> > > > + * the underlying kernel-defined type in order to easily populate or read\n> > > > + * the ISP configuration data.\n> > > > + *\n> > > > + * \\code{.cpp}\n> > > > + *\n> > > > + * // The kernel header defines the ISP configuration types, in example\n> > > > + * // struct my_isp_awb_config_data {\n> > > > + * //\t\tu16 gain_ch00;\n> > > > + * //\t\tu16 gain_ch01;\n> > > > + * //\t\tu16 gain_ch10;\n> > > > + * //\t\tu16 gain_ch11;\n> > > > + * //\n> > > > + * // }\n> > > > + *\n> > > > + * template<> V4L2ParamsBlock awbBlock\n> > > > + *\n> > > > + * awbBlock->gain_ch00 = ...;\n> > > > + * awbBlock->gain_ch01 = ...;\n> > > > + * awbBlock->gain_ch10 = ...;\n> > > > + * awbBlock->gain_ch11 = ...;\n> > > > + *\n> > > > + * \\endcode\n> > > > + *\n> > > > + * Users of this class are not expected to create a V4L2ParamsBlock manually but\n> > > > + * should rather use V4L2Params::block() to retrieve a reference to the memory\n> > > > + * area used to construct a V4L2ParamsBlock in their overloaded\n> > > > + * implementation of V4L2Params::block().\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2ParamsBlock::operator->() const\n> > > > + * \\copydoc V4L2ParamsBlock::operator->()\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2ParamsBlock::operator*() const\n> > > > + * \\copydoc V4L2ParamsBlock::operator->()\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2ParamsBlock::operator*()\n> > > > + * \\copydoc V4L2ParamsBlock::operator->()\n> > > > + */\n> > > > +\n> > > > + /**\n> > > > + * \\class V4L2Params\n> > > > + * \\brief Helper class that represent an ISP configuration buffer\n> > > > + *\n> > > > + * ISP implementation compatible with v4l2-params define their ISP configuration\n> > > > + * buffer types compatible with the struct v4l2_params_buffer type.\n> > > > + *\n> > > > + * This class represents an ISP configuration buffer. It is constructed\n> > > > + * with a reference to the memory mapped buffer that will be queued to the ISP.\n> > > > + *\n> > > > + * This class is templated with the type of the enumeration of ISP blocks that\n> > > > + * each IPA module is expected to support. IPA modules are expected to derive\n> > > > + * this class and use the V4L2Params::block() function to retrieve the memory\n> > > > + * area for each ISP configuration block and use it to construct a\n> > > > + * V4L2ParamsBlock with it before returning it to the user.\n> > > > + *\n> > > > + * \\code{.cpp}\n> > > > + *\n> > > > + * enum class myISPBlocks {\n> > > > + *\tAgc,\n> > > > + *\tAwb,\n> > > > + *\t...\n> > > > + * };\n> > > > + *\n> > > > + * template\n> > > > + * struct block_type {\n> > > > + * };\n> > > > + *\n> > > > + * template<>\n> > > > + * struct block_type {\n> > > > + *\tusing type = struct my_isp_kernel_config_type_agc;\n> > > > + * };\n> > > > + *\n> > > > + * template<>\n> > > > + * struct block_type {\n> > > > + *\tusing type = struct my_isp_kernel_config_type_awb;\n> > > > + * };\n> > > > + *\n> > > > + * ...\n> > > > + *\n> > > > + * class MyISPParams : public V4L2Params\n> > > > + * {\n> > > > + * public:\n> > > > + * \ttemplate\n> > > > + * \tauto block()\n> > > > + * \t{\n> > > > + *\n> > > > + * \t\t// Use the kernel defined configuration type as template\n> > > > + * \t\t// argument to V4L2ParamsBlock.\n> > > > + * \t\tusing Type = typename details::block_type::type;\n> > > > + *\n> > > > + * \t\t// Each IPA module should provide the information required\n> > > > + * \t\t// to populate the block header\n> > > > + *\n> > > > + * \t\t...\n> > > > + *\n> > > > + * \t\tauto data = V4L2Params::block(B, blockType, blockSize);\n> > > > + *\n> > > > + * \t\treturn V4L2ParamsBlock(data);\n> > > > + * \t}\n> > > > + * };\n> > > > + *\n> > > > + * \\endcode\n> > > > + *\n> > > > + * As an example, see the RkISP1Params and MaliC55Params implementations.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2Params::V4L2Params()\n> > > > + * \\brief Construct a V4L2Params\n> > > > + * \\param[in] data Reference to the v4l2-buffer memory mapped area\n> > > > + * \\param[in] version The ISP parameters version the implementation supports\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2Params::size()\n> > > > + * \\brief Retrieve the used size of the parameters buffer (in bytes)\n> > > > + *\n> > > > + * The parameters buffer size is mostly used to populate the v4l2_buffer\n> > > > + * bytesused field before queueing the buffer to the ISP.\n> > > > + *\n> > > > + * \\return The number of bytes occupied by the ISP configuration parameters\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn V4L2Params::block()\n> > > > + * \\brief Populate an ISP configuration block a returns a reference to its\n> > > > + * memory\n> > > > + * \\param[in] type The ISP block identifier enumerated by the IPA module\n> > > > + * \\param[in] blockType The kernel-defined ISP block identifier, used to\n> > > > + * populate the block header\n> > > > + * \\param[in] blockSize The ISP block size, used to populate the block header\n> > > > + *\n> > > > + *\n> > > > + * Initialize the block header with \\a blockType and \\a blockSize and\n> > > > + * returns a reference to the memory used to store an ISP configuration block.\n> > > > + *\n> > > > + * IPA modules that derive the V4L2Params class shall use this function to\n> > > > + * retrieve the memory area that will be used to construct a V4L2ParamsBlock\n> > > > + * before returning it to the caller.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var V4L2Params::data_\n> > > > + * \\brief The ISP parameters buffer memory\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var V4L2Params::used_\n> > > > + * \\brief The number of bytes used in the parameters buffer\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\var V4L2Params::blocks_\n> > > > + * \\brief Cache of ISP configuration blocks\n> > > > + */\n> > > > +\n> > > > +} /* namespace ipa */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h\n> > > > new file mode 100644\n> > > > index 0000000000000000000000000000000000000000..5586096c7ee8a2d20877838564e8074e0fc3d1ce\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/libipa/v4l2_params.h\n> > > > @@ -0,0 +1,135 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2025, Ideas On Board\n> > > > + *\n> > > > + * V4L2 Parameters\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa {\n> > > > +\n> > > > +template\n> > > > +class V4L2ParamsBlock\n> > > > +{\n> > > > +public:\n> > > > +\tV4L2ParamsBlock(const Span &data)\n> > >\n> > > I think taking by value is fine, no need for the const ref.\n> > >\n> >\n> > right indeed\n> >\n> > >\n> > > > +\t{\n> > > > +\t\theader_ = data.subspan(0, sizeof(v4l2_params_block_header));\n> > > > +\t\tdata_ = data.subspan(sizeof(v4l2_params_block_header));\n> > >\n> > > I'd probably do these in the member init list.\n> > >\n> >\n> > is this just a style preference or are there reasons I'm missing ?\n>\n> There is a difference: one is an assignment, one is a constructor call;\n> so I always try to use the member init list. Although the difference does\n> not really matter much here, so you could say it's a preference thing.\n>\n\nNo problem, I'll use the member init list\n\n>\n> >\n> > >\n> > > > +\t}\n> > > > +\n> > > > +\tvoid setEnabled(bool enabled)\n> > > > +\t{\n> > > > +\t\tstruct v4l2_params_block_header *header =\n> > > > +\t\t\treinterpret_cast(header_.data());\n> > > > +\n> > > > +\t\theader->flags &= ~(V4L2_PARAMS_FL_BLOCK_ENABLE |\n> > > > +\t\t\t\t V4L2_PARAMS_FL_BLOCK_DISABLE);\n> > > > +\t\theader->flags |= enabled ? V4L2_PARAMS_FL_BLOCK_ENABLE\n> > > > +\t\t\t\t\t : V4L2_PARAMS_FL_BLOCK_DISABLE;\n> > > > +\t}\n> > > > +\n> > > > +\tSpan header() const { return header_; }\n> > > > +\tSpan data() const { return data_; }\n> > > > +\n> > > > +\tconst T *operator->() const\n> > > > +\t{\n> > > > +\t\treturn reinterpret_cast(data().data());\n> > > > +\t}\n> > > > +\n> > > > +\tT *operator->()\n> > > > +\t{\n> > > > +\t\treturn reinterpret_cast(data().data());\n> > > > +\t}\n> > > > +\n> > > > +\tconst T &operator*() const\n> > > > +\t{\n> > > > +\t\treturn *reinterpret_cast(data().data());\n> > > > +\t}\n> > > > +\n> > > > +\tT &operator*()\n> > > > +\t{\n> > > > +\t\treturn *reinterpret_cast(data().data());\n> > > > +\t}\n> > > > +\n> > > > +private:\n> > > > +\tLIBCAMERA_DISABLE_COPY(V4L2ParamsBlock)\n> > >\n> > > Why disable? I would assume that it's OK to copy this?\n> > >\n> >\n> > tbh I think I just copied this from RkISP1Params.\n> >\n> > However, why would one copy an instance of V4L2ParamsBlock ?\n>\n> My thinking was that it is a \"reference-like type\", so there is no reason to\n> prevent copying since it works and might be useful in some cases (e.g. passing\n> to functions, etc.).\n>\n\nRight. I'll drop this\n\n>\n> >\n> > >\n> > > > +\n> > > > +\tSpan header_;\n> > > > +\tSpan data_;\n> > > > +};\n> > > > +\n> > > > +template\n> > > > +class V4L2Params\n> > > > +{\n> > > > +public:\n> > > > +\tV4L2Params(Span data, unsigned int version)\n> > > > +\t\t: data_(data)\n> > > > +\t{\n> > > > +\t\tstruct v4l2_params_buffer *cfg =\n> > > > +\t\t\treinterpret_cast(data_.data());\n> > > > +\t\tcfg->data_size = 0;\n> > > > +\t\tcfg->version = version;\n> > > > +\t\tused_ = offsetof(struct v4l2_params_buffer, data);\n> > > > +\t}\n> > > > +\n> > > > +\tsize_t size() const { return used_; }\n> > > > +\n> > > > +protected:\n> > > > +\tSpan block(T type, unsigned int blockType, size_t blockSize)\n> > >\n> > > This can modified so that the derived classes need not implement any extra logic.\n> > > One option is to pass the `details::block_type` template as a template argument\n> > > to V4L2Params and then use that to map the id to the type:\n> > >\n> > > template typename IdToType>\n> > > class V4L2Params {\n> > >\n> > > template\n> > > auto block() {\n> > > using Type = typename IdToType::type;\n> > > ...\n> > > }\n> > >\n> > > but I think it's simpler and more extensible to pass another type that holds\n> > > all this compile-time information. It could look something like this:\n> > >\n> > > diff --git a/src/ipa/libipa/v4l2_params.h b/src/ipa/libipa/v4l2_params.h\n> > > index 5586096c7..019f06509 100644\n> > > --- a/src/ipa/libipa/v4l2_params.h\n> > > +++ b/src/ipa/libipa/v4l2_params.h\n> > > @@ -71,7 +71,7 @@ private:\n> > > \tSpan data_;\n> > > };\n> > > -template\n> > > +template\n> > > class V4L2Params\n> > > {\n> > > public:\n> > > @@ -87,8 +87,21 @@ public:\n> > > \tsize_t size() const { return used_; }\n> > > +\ttemplate\n> > > +\tauto block()\n> > > +\t{\n> > > +\t\tusing Details = typename Traits::template id_to_details;\n> >\n> > I can't grok 'typename Traits::template'\n> >\n> > but it's ok, I'll trust you here\n>\n> See https://www.en.cppreference.com/w/cpp/language/dependent_name.html\n> under \"The template disambiguator for dependent names\". It just tells\n> the compiler that `id_to_details` is a template.\n>\n> This is quite similar to having to use the `typename` keyword in some cases.\n> (see \"The typename disambiguator for dependent names\" on the linked site)\n>\n\nThanks, I'll educate myself\n>\n> >\n> > > +\n> > > +\t\tusing Type = typename Details::type;\n> > > +\t\tconstexpr auto kernelId = Details::blockType;\n> > > +\n> > > +\t\tauto data = block(Id, kernelId, sizeof(Type));\n> > > +\n> > > +\t\treturn V4L2ParamsBlock(data);\n> > > +\t}\n> > > +\n> > > protected:\n> > > -\tSpan block(T type, unsigned int blockType, size_t blockSize)\n> > > +\tSpan block(typename Traits::id_type type, unsigned int blockType, size_t blockSize)\n> > > \t{\n> > > \t\t/*\n> > > \t\t * Look up the block in the cache first. If an algorithm\n> > > @@ -127,7 +140,7 @@ protected:\n> > > \tSpan data_;\n> > > \tsize_t used_;\n> > > -\tstd::map> blocks_;\n> > > +\tstd::map> blocks_;\n> > > };\n> > > } /* namespace ipa */\n> > > diff --git a/src/ipa/mali-c55/params.h b/src/ipa/mali-c55/params.h\n> > > index 1cc56dbad..975122999 100644\n> > > --- a/src/ipa/mali-c55/params.h\n> > > +++ b/src/ipa/mali-c55/params.h\n> > > @@ -58,30 +58,24 @@ MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingConfig, mesh_shading_config,\n> > > MALI_C55_DEFINE_BLOCK_TYPE(MeshShadingSel, mesh_shading_selection,\n> > > \t\t\t MESH_SHADING_SELECTION)\n> > > +struct params_traits {\n> > > +\tusing id_type = MaliC55Blocks;\n> > > +\n> > > +\ttemplate\n> > > +\tusing id_to_details = block_type;\n> > > +};\n> > > +\n> > > } /* namespace details */\n> > > -class MaliC55Params : public V4L2Params\n> > > +class MaliC55Params : public V4L2Params\n> > > {\n> > > public:\n> > > \tstatic constexpr unsigned int kVersion = MALI_C55_PARAM_BUFFER_V1;\n> > > \tMaliC55Params(Span data)\n> > > -\t\t: V4L2Params(data, kVersion)\n> > > +\t\t: V4L2Params(data, kVersion)\n> > > \t{\n> > > \t}\n> > > -\n> > > -\ttemplate\n> > > -\tauto block()\n> > > -\t{\n> > > -\t\tusing Type = typename details::block_type::type;\n> > > -\n> > > -\t\tauto blockType = details::block_type::blockType;\n> > > -\t\tsize_t blockSize = sizeof(Type);\n> > > -\n> > > -\t\tauto data = V4L2Params::block(B, blockType, blockSize);\n> > > -\n> > > -\t\treturn V4L2ParamsBlock(data);\n> > > -\t}\n> >\n> > Thanks!\n> >\n> > 'struct param_traits' and the associated changes in V4L2Params allowed\n> > me to simplify the MaliC55 implementation as you suggested\n> >\n> > > };\n> > > } /* namespace ipa::mali_c55 */\n> > > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> > > index 7a2127664..824a5924f 100644\n> > > --- a/src/ipa/rkisp1/params.h\n> > > +++ b/src/ipa/rkisp1/params.h\n> > > @@ -45,45 +45,54 @@ template\n> > > struct block_type {\n> > > };\n> > > -#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)\t\t\\\n> > > +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id)\t\t\\\n> > > template<>\t\t\t\t\t\t\t\t\\\n> > > struct block_type {\t\t\t\t\\\n> > > \tusing type = struct rkisp1_cif_isp_##blockStruct##_config;\t\\\n> > > +\tstatic constexpr rkisp1_ext_params_block_type blockType =\t\\\n> > > +\t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id;\t\t\t\\\n> > > };\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Bls, bls)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Flt, flt)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Goc, goc)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Ie, ie)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Hst, hst)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Aec, aec)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(Afc, afc)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve)\n> > > -RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Bls, bls, BLS)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Dpcc, dpcc, DPCC)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Sdg, sdg, SDG)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(AwbGain, awb_gain, AWB_GAIN)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Flt, flt, FLT)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Bdm, bdm, BDM)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Ctk, ctk, CTK)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Goc, goc, GOC)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Dpf, dpf, DPF)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(DpfStrength, dpf_strength, DPF_STRENGTH)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Cproc, cproc, CPROC)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Ie, ie, IE)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Lsc, lsc, LSC)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Awb, awb_meas, AWB_MEAS)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Hst, hst, HST_MEAS)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Aec, aec, AEC_MEAS)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(Afc, afc, AFC_MEAS)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(CompandBls, compand_bls, COMPAND_BLS)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(CompandExpand, compand_curve, COMPAND_EXPAND)\n> > > +RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve, COMPAND_COMPRESS)\n> > > +\n> > > +struct params_traits {\n> > > +\tusing id_type = BlockType;\n> > > +\n> > > +\ttemplate\n> > > +\tusing id_to_details = block_type;\n> > > +};\n> > > } /* namespace details */\n> > > template\n> > > class RkISP1ParamsBlock;\n> > > -class RkISP1Params : public V4L2Params\n> > > +class RkISP1Params : public V4L2Params\n> > > {\n> > > public:\n> > > \tstatic constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;\n> > > \tRkISP1Params(uint32_t format, Span data)\n> > > -\t\t: V4L2Params(data, kVersion), format_(format)\n> > > +\t\t: V4L2Params(data, kVersion), format_(format)\n> > > \t{\n> > > \t\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n> > > \t\t\tmemset(data.data(), 0, data.size());\n> > >\n> > >\n> > > Admittedly I haven't tested it, and the rkisp1 parts are not there yet.\n> > > But I believe this direction could simplify the code and make it easier\n> > > to implement for other parameter sets.\n> >\n> > Please note that for RkISP1 I had to keep an override for\n> > RkISP1Params::block() as we need to support both legacy and extensible\n> >\n> > >\n> > > For rkisp1, I think as a first step `kBlockTypeInfo` should be moved completely\n> > > into `rkisp1::details::block_type` template specializations to do the mapping\n> > > at compile time, similarly to how the proposed `MaliC55Params` does it.\n> >\n> > yeah, I don't like that map too much. But, if I may use it as an\n> > excuse, it was there already so we're not doing any worse.\n> >\n> > I could have removed it like it's done for Mali, but the 'enableBit'\n> > used by setEnabled() to support the legacy format made it more\n> > complicated getting rid of it (I've not invested much time in trying,\n> > admittedly).\n> >\n> > Anyway, could easily be done on top ?\n>\n> Yes.\n>\n>\n> >\n> > >\n> > > Then I believe one could realistically extend this `V4L2Params` to even support\n> > > fixed-offset formats generically, if needed, but that might be an overkill.\n> >\n> > Not sure I got what you mean here,\n>\n> What I meant is that it is probably possible to support \"legacy\" parameter formats\n> like the rkisp1 one in the base template. But if that's the only one that is\n> relevant for us, then it's very likely an overkill.\n>\n\nI don't expect other platforms we supports to have to support both\nformats at the moment, so I would leave this option out for the time\nbeing\n\n>\n> > >\n> > >\n> > > > +\t{\n> > > > +\t\t/*\n> > > > +\t\t * Look up the block in the cache first. If an algorithm\n> > > > +\t\t * requests the same block type twice, it should get the same\n> > > > +\t\t * block.\n> > > > +\t\t */\n> > > > +\t\tauto cacheIt = blocks_.find(type);\n> > > > +\t\tif (cacheIt != blocks_.end())\n> > > > +\t\t\treturn cacheIt->second;\n> > > > +\n> > > > +\t\t/* Make sure we don't run out of space. */\n> > > > +\t\tif (blockSize > data_.size() - used_)\n> > > > +\t\t\treturn {};\n> > > > +\n> > > > +\t\t/* Allocate a new block, clear its memory, and initialize its header. */\n> > > > +\t\tSpan block = data_.subspan(used_, blockSize);\n> > > > +\t\tused_ += blockSize;\n> > > > +\n> > > > +\t\tstruct v4l2_params_buffer *cfg =\n> > > > +\t\t\treinterpret_cast(data_.data());\n> > > > +\t\tcfg->data_size += blockSize;\n> > > > +\n> > > > +\t\tmemset(block.data(), 0, block.size());\n> > > > +\n> > > > +\t\tstruct v4l2_params_block_header *header =\n> > > > +\t\t\treinterpret_cast(block.data());\n> > > > +\t\theader->type = blockType;\n> > > > +\t\theader->size = block.size();\n> > > > +\n> > > > +\t\t/* Update the cache. */\n> > > > +\t\tblocks_[type] = block;\n> > > > +\n> > > > +\t\treturn block;\n> > > > +\t}\n> > > > +\n> > > > +\tSpan data_;\n> > > > +\tsize_t used_;\n> > > > +\n> > > > +\tstd::map> blocks_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace ipa */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n> > > > index 4c0b051ce65da1686323ee9c66b82e12669a754d..2b692e1a1f199d6c118af0938e1aaecef6186a2c 100644\n> > > > --- a/src/ipa/rkisp1/params.cpp\n> > > > +++ b/src/ipa/rkisp1/params.cpp\n> > > > @@ -35,7 +35,7 @@ struct BlockTypeInfo {\n> > > > #define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)\t\t\t\\\n> > > > \t{ BlockType::block, {\t\t\t\t\t\t\t\\\n> > > > \t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> > > > -\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> > > > +\t\tsizeof(struct rkisp1_ext_params_##type##_config),\t\t\\\n> > > > \t\toffsetof(struct rkisp1_params_cfg, category.type##_config),\t\\\n> > > > \t\tRKISP1_CIF_ISP_MODULE_##bit,\t\t\t\t\t\\\n> > > > \t} }\n> > > > @@ -49,7 +49,7 @@ struct BlockTypeInfo {\n> > > > #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)\t\t\t\t\\\n> > > > \t{ BlockType::block, {\t\t\t\t\t\t\t\\\n> > > > \t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> > > > -\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> > > > +\t\tsizeof(struct rkisp1_ext_params_##type##_config),\t\t\\\n> > > > \t\t0, 0,\t\t\t\t\t\t\t\t\\\n> > > > \t} }\n> > > > @@ -78,56 +78,6 @@ const std::map kBlockTypeInfo = {\n> > > > } /* namespace */\n> > > > -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> > > > -\t\t\t\t\t const Span &data)\n> > > > -\t: params_(params), type_(type)\n> > > > -{\n> > > > -\tif (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > > > -\t\theader_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n> > > > -\t\tdata_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n> > > > -\t} else {\n> > > > -\t\tdata_ = data;\n> > > > -\t}\n> > > > -}\n> > > > -\n> > > > -void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n> > > > -{\n> > > > -\t/*\n> > > > -\t * For the legacy fixed format, blocks are enabled in the top-level\n> > > > -\t * header. Delegate to the RkISP1Params class.\n> > > > -\t */\n> > > > -\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> > > > -\t\treturn params_->setBlockEnabled(type_, enabled);\n> > > > -\n> > > > -\t/*\n> > > > -\t * For the extensible format, set the enable and disable flags in the\n> > > > -\t * block header directly.\n> > > > -\t */\n> > > > -\tstruct rkisp1_ext_params_block_header *header =\n> > > > -\t\treinterpret_cast(header_.data());\n> > > > -\theader->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |\n> > > > -\t\t\t RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE);\n> > > > -\theader->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE\n> > > > -\t\t\t\t : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE;\n> > > > -}\n> > > > -\n> > > > -RkISP1Params::RkISP1Params(uint32_t format, Span data)\n> > > > -\t: format_(format), data_(data), used_(0)\n> > > > -{\n> > > > -\tif (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > > > -\t\tstruct rkisp1_ext_params_cfg *cfg =\n> > > > -\t\t\treinterpret_cast(data.data());\n> > > > -\n> > > > -\t\tcfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n> > > > -\t\tcfg->data_size = 0;\n> > > > -\n> > > > -\t\tused_ += offsetof(struct rkisp1_ext_params_cfg, data);\n> > > > -\t} else {\n> > > > -\t\tmemset(data.data(), 0, data.size());\n> > > > -\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> > > > -\t}\n> > > > -}\n> > > > -\n> > > > void RkISP1Params::setBlockEnabled(BlockType type, bool enabled)\n> > > > {\n> > > > \tconst BlockTypeInfo &info = kBlockTypeInfo.at(type);\n> > > > @@ -177,44 +127,7 @@ Span RkISP1Params::block(BlockType type)\n> > > > \t\treturn data_.subspan(info.offset, info.size);\n> > > > \t}\n> > > > -\t/*\n> > > > -\t * For the extensible format, allocate memory for the block, including\n> > > > -\t * the header. Look up the block in the cache first. If an algorithm\n> > > > -\t * requests the same block type twice, it should get the same block.\n> > > > -\t */\n> > > > -\tauto cacheIt = blocks_.find(type);\n> > > > -\tif (cacheIt != blocks_.end())\n> > > > -\t\treturn cacheIt->second;\n> > > > -\n> > > > -\t/* Make sure we don't run out of space. */\n> > > > -\tsize_t size = sizeof(struct rkisp1_ext_params_block_header)\n> > > > -\t\t + ((info.size + 7) & ~7);\n> > > > -\tif (size > data_.size() - used_) {\n> > > > -\t\tLOG(RkISP1Params, Error)\n> > > > -\t\t\t<< \"Out of memory to allocate block type \"\n> > > > -\t\t\t<< utils::to_underlying(type);\n> > > > -\t\treturn {};\n> > > > -\t}\n> > > > -\n> > > > -\t/* Allocate a new block, clear its memory, and initialize its header. */\n> > > > -\tSpan block = data_.subspan(used_, size);\n> > > > -\tused_ += size;\n> > > > -\n> > > > -\tstruct rkisp1_ext_params_cfg *cfg =\n> > > > -\t\treinterpret_cast(data_.data());\n> > > > -\tcfg->data_size += size;\n> > > > -\n> > > > -\tmemset(block.data(), 0, block.size());\n> > > > -\n> > > > -\tstruct rkisp1_ext_params_block_header *header =\n> > > > -\t\treinterpret_cast(block.data());\n> > > > -\theader->type = info.type;\n> > > > -\theader->size = block.size();\n> > > > -\n> > > > -\t/* Update the cache. */\n> > > > -\tblocks_[type] = block;\n> > > > -\n> > > > -\treturn block;\n> > > > +\treturn V4L2Params::block(type, info.type, info.size);\n> > > > }\n> > > > } /* namespace ipa::rkisp1 */\n> > > > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> > > > index 40450e34497a3aa71b5b0cda2bf045a1cc0e012f..7a21276648162a127317c75cb01d1c0059b96ee1 100644\n> > > > --- a/src/ipa/rkisp1/params.h\n> > > > +++ b/src/ipa/rkisp1/params.h\n> > > > @@ -7,13 +7,10 @@\n> > > > #pragma once\n> > > > -#include \n> > > > -#include \n> > > > -\n> > > > #include \n> > > > +#include \n> > > > -#include \n> > > > -#include \n> > > > +#include \n> > > > namespace libcamera {\n> > > > @@ -77,85 +74,72 @@ RKISP1_DEFINE_BLOCK_TYPE(CompandCompress, compand_curve)\n> > > > } /* namespace details */\n> > > > -class RkISP1Params;\n> > > > +template\n> > > > +class RkISP1ParamsBlock;\n> > > > -class RkISP1ParamsBlockBase\n> > > > +class RkISP1Params : public V4L2Params\n> > > > {\n> > > > public:\n> > > > -\tRkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> > > > -\t\t\t const Span &data);\n> > > > -\n> > > > -\tSpan data() const { return data_; }\n> > > > -\n> > > > -\tvoid setEnabled(bool enabled);\n> > > > +\tstatic constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;\n> > > > -private:\n> > > > -\tLIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)\n> > > > -\n> > > > -\tRkISP1Params *params_;\n> > > > -\tBlockType type_;\n> > > > -\tSpan header_;\n> > > > -\tSpan data_;\n> > > > -};\n> > > > -\n> > > > -template\n> > > > -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> > > > -{\n> > > > -public:\n> > > > -\tusing Type = typename details::block_type::type;\n> > > > -\n> > > > -\tRkISP1ParamsBlock(RkISP1Params *params, const Span &data)\n> > > > -\t\t: RkISP1ParamsBlockBase(params, B, data)\n> > > > +\tRkISP1Params(uint32_t format, Span data)\n> > > > +\t\t: V4L2Params(data, kVersion), format_(format)\n> > > > \t{\n> > > > +\t\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n> > > > +\t\t\tmemset(data.data(), 0, data.size());\n> > > > +\t\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> > > > +\t\t}\n> > > > \t}\n> > > > -\tconst Type *operator->() const\n> > > > +\ttemplate\n> > > > +\tauto block()\n> > > > \t{\n> > > > -\t\treturn reinterpret_cast(data().data());\n> > > > -\t}\n> > > > +\t\tusing Type = typename details::block_type::type;\n> > > > -\tType *operator->()\n> > > > -\t{\n> > > > -\t\treturn reinterpret_cast(data().data());\n> > > > +\t\treturn RkISP1ParamsBlock(this, B, block(B));\n> > > > \t}\n> > > > -\tconst Type &operator*() const &\n> > > > -\t{\n> > > > -\t\treturn *reinterpret_cast(data().data());\n> > > > -\t}\n> > > > +\tuint32_t format() const { return format_; }\n> > > > +\tvoid setBlockEnabled(BlockType type, bool enabled);\n> > > > -\tType &operator*() &\n> > > > -\t{\n> > > > -\t\treturn *reinterpret_cast(data().data());\n> > > > -\t}\n> > > > +private:\n> > > > +\tSpan block(BlockType type);\n> > > > +\n> > > > +\tuint32_t format_;\n> > > > };\n> > > > -class RkISP1Params\n> > > > +template\n> > > > +class RkISP1ParamsBlock : public V4L2ParamsBlock\n> > > > {\n> > > > public:\n> > > > -\tRkISP1Params(uint32_t format, Span data);\n> > > > -\n> > > > -\ttemplate\n> > > > -\tRkISP1ParamsBlock block()\n> > > > +\tRkISP1ParamsBlock(RkISP1Params *params, BlockType type,\n> > > > +\t\t\t const Span &data)\n> > > > +\t\t: V4L2ParamsBlock(data)\n> > > > \t{\n> > > > -\t\treturn RkISP1ParamsBlock(this, block(B));\n> > > > +\t\tparams_ = params;\n> > > > +\t\ttype_ = type;\n> > > > +\n> > > > +\t\t/* Legacy param format has no header */\n> > > > +\t\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> > > > +\t\t\tdata_ = data;\n> > > > \t}\n> > > > -\tuint32_t format() const { return format_; }\n> > > > -\tsize_t size() const { return used_; }\n> > > > +\tvoid setEnabled(bool enabled)\n> > > > +\t{\n> > > > +\t\t/*\n> > > > +\t\t * For the legacy fixed format, blocks are enabled in the\n> > > > +\t\t * top-level header. Delegate to the RkISP1Params class.\n> > > > +\t\t */\n> > > > +\t\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> > > > +\t\t\treturn params_->setBlockEnabled(type_, enabled);\n> > > > +\n> > > > +\t\treturn V4L2ParamsBlock::setEnabled(enabled);\n> > > > +\t}\n> > >\n> > > I have one concern regarding this design. An `RkISP1ParamsBlock` instance\n> > > will not work as its base type correctly in all cases. That is, calling the\n> > > base class `setEnabled()`:\n> > >\n> > > RkISP1ParamsBlock<...> *p = ...;\n> > >\n> > > p->V4L2ParamsBlock<...>::setEnabled(...);\n> > > // or\n> > > static_cast *>(p)->setEnabled(...);\n> > >\n> > > will not work correctly if the format is the \"non-extensible\" one.\n> >\n> > The only user of this class is the RkISP1 IPA, why would it forcefully\n> > use the base class implementation ?\n>\n> It probably wouldn't, I just wanted to make a note of this. Although I can\n> imagine that with time some parts would want to operate on `V4L2ParamsBlock`\n> objects directly, in a generic fashion, and then this might be an issue.\n>\n\nOnly for RkISP1 which supports both legacy and extensible, right ?\n\n>\n> >\n> > If there's an easy way to avoid the above, I'll anyway happily consider\n> > it.\n>\n> Sadly I don't think I have an easy solution here. I'll take a look again\n> at the next version.\n\nthanks, let's not be concerned with this for the moment.\n\nNext version out soon, thanks!\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n> >\n> > Thanks\n> > j\n> >\n> > >\n> > >\n> > > Regards,\n> > > Barnabás Pőcze\n> > >\n> > >\n> > > > private:\n> > > > -\tfriend class RkISP1ParamsBlockBase;\n> > > > -\n> > > > -\tSpan block(BlockType type);\n> > > > -\tvoid setBlockEnabled(BlockType type, bool enabled);\n> > > > -\n> > > > -\tuint32_t format_;\n> > > > -\n> > > > +\tRkISP1Params *params_;\n> > > > +\tBlockType type_;\n> > > > \tSpan data_;\n> > > > -\tsize_t used_;\n> > > > -\n> > > > -\tstd::map> blocks_;\n> > > > };\n> > > > } /* namespace ipa::rkisp1 */\n> > > >\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 8911FC328C\n\tfor ;\n\tTue, 16 Sep 2025 11:53:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 064D46936F;\n\tTue, 16 Sep 2025 13:53:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79903613A1\n\tfor ;\n\tTue, 16 Sep 2025 13:53:27 +0200 (CEST)","from ideasonboard.com (mob-5-90-51-255.net.vodafone.it\n\t[5.90.51.255])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7F9CCC6F;\n\tTue, 16 Sep 2025 13:52:08 +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=\"h1DMXn1q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758023529;\n\tbh=aTOl4AybGqpwQk+BLJ732p1Ecgtamw/YNFWhYuXdhT8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h1DMXn1qUooh4iRFHnhEJEiReniPFgkfCDsSvakUtzzQ0bi3sL6A25ylIjNe6aFzV\n\tbDFQL4HxkeHZHc61QX1VdfQ2o845nmd5CVBjj+Ul9HsNSTXVgOx4+cZow1X3uzRRSv\n\tXwMOCXvG7ow3NYRMThb/PkEQDFBAuNs44nmuRoiM=","Date":"Tue, 16 Sep 2025 13:53:23 +0200","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/4] ipa: libipa: Introduce V4L2Params","Message-ID":"","References":"<20250829-v4l2-params-v1-0-340773fb69ff@ideasonboard.com>\n\t<20250829-v4l2-params-v1-3-340773fb69ff@ideasonboard.com>\n\t\n\t<2lytfujt57qxype7akx5tyxufwfgevztqj6nqjpdw34qhhg5vo@wdgvykvddjfc>\n\t<484baec7-814f-43e7-84cf-75b9f8ed5a0a@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<484baec7-814f-43e7-84cf-75b9f8ed5a0a@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\" "}}]