[{"id":36191,"web_url":"https://patchwork.libcamera.org/comment/36191/","msgid":"<39dc8b20-dfbe-41e2-8ea8-0ce9853f860b@ideasonboard.com>","date":"2025-10-10T15:00:23","subject":"Re: [PATCH v5 4/5] ipa: libipa: Introduce V4L2Params","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 07/10/2025 19:17, Jacopo Mondi wrote:\n> The existing RkISP1Params helper class allows the RkISP1 IPA to handle\n> both the extensible parameters format and the legacy fixed-size format.\n> \n> With the introduction of v4l2-isp.h 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 parameters buffer compatible with v4l2-isp.h.\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 <jacopo.mondi@ideasonboard.com>\n> Tested-by: Antoine Bouyer <antoine.bouyer@nxp.com>\n> ---\n>   src/ipa/libipa/meson.build     |   2 +\n>   src/ipa/libipa/v4l2_params.cpp | 254 +++++++++++++++++++++++++++++++++++++++++\n>   src/ipa/libipa/v4l2_params.h   | 142 +++++++++++++++++++++++\n>   src/ipa/rkisp1/params.cpp      |  93 +--------------\n>   src/ipa/rkisp1/params.h        | 175 ++++++++++++++++------------\n>   5 files changed, 501 insertions(+), 165 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\nThese two aren't alphabetical.>\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..278b5e4259be4798d0f719c8b1876ef654a14c75\n> --- /dev/null\n> +++ b/src/ipa/libipa/v4l2_params.cpp\n> @@ -0,0 +1,254 @@\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 an ISP configuration buffer compatible with\n> + * the generic V4L2 ISP format\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 ISP generic parameters. The\n> + * V4L2 ISP parameters define a serialization format for ISP parameters that\n> + * allows userspace to populate a buffer of configuration data by appending them\n> + * one after the other in a binary buffer.\n\nI think this could do with some revision. Perhaps something like:\n\nThe Linux kernel defines a generic buffer format for configuring ISP devices. The format describes a \nserialisation method for ISP parameters that allows userspace to populate a buffer of configuration \ndata by appending blocks to the buffer with common headers but device-specific contents one after \nthe other.\n\n> + *\n> + * The V4L2Params class implementes support for working with V4L2 ISP parameters\n\ns/implementes/implements. And maybe just \"...implements support for the V4L2 ISP parameters buffer \nformat and allows...\"> + * buffer and allows users to populate the ISP configuration blocks, represented\n> + * as 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\ns/the IPA module supports/supported by the IPA module. s/abstraction/abstractions\n\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 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::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<struct my_isp_awb_config_data> 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 shall not create a V4L2ParamsBlock manually but should\n> + * use 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> + * \\var V4L2ParamsBlock::data_\n> + * \\brief Memory area reserved for the ISP configuration block\n> + */\n> +\n> + /**\n> +  * \\class V4L2Params\n> +  * \\brief Helper class that represent an ISP configuration buffer\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> + * driver.\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 by providing a 'param_traits' type the helps the class associate\n\ns/type the helps/type that helps\n\n> + * a block type with the actual memory area that represents the ISP\n> + * configuration block.\n> + *\n> + * \\code{.cpp}\n> + *\n> + * // Define the supported ISP blocks\n> + * enum class myISPBlocks {\n> + *\tAgc,\n> + *\tAwb,\n> + *\t...\n> + * };\n> + *\n> + * // Maps the C++ enum type to the kernel enum type and concrete parameter type\n> + * template<myISPBlocks B>\n> + * struct block_type {\n> + * };\n> + *\n> + * template<>\n> + * struct block_type<myISPBlock::Agc> {\n> + *\tusing type = struct my_isp_kernel_config_type_agc;\n> + *\tstatic constexpr kernel_enum_type blockType = MY_ISP_TYPE_AGC;\n> + * };\n> + *\n> + * template<>\n> + * struct block_type<myISPBlock::Awb> {\n> + *\tusing type = struct my_isp_kernel_config_type_awb;\n> + *\tstatic constexpr kernel_enum_type blockType = MY_ISP_TYPE_AWB;\n> + * };\n> + *\n> + *\n> + * // Convenience type to associate a block id to the 'block_type' overload\n> + * struct params_traits {\n> + * \tusing id_type = myISPBlocks;\n> + * \ttemplate<id_type Id> using id_to_details = block_type<Id>;\n> + * };\n> + *\n> + * ...\n> + *\n> + * // Derive the V4L2Params class by providing params_traits\n> + * class MyISPParams : public V4L2Params<params_traits>\n> + * {\n> + * public:\n> + * \tMyISPParams::MyISPParams(Span<uint8_t> data)\n> + * \t\t: V4L2Params(data, kVersion)\n> + * \t{\n> + * \t}\n> + * };\n> + *\n> + * \\endcode\n> + *\n> + * Users of this class can then easily access an ISP configuration block as a\n> + * V4L2ParamsBlock instance.\n> + *\n> + * \\code{.cpp}\n> + *\n> + * MyISPParams params(data);\n> + *\n> + * auto awb = params.block<myISPBlocks::AWB>();\n> + * awb->gain00 = ...;\n> + * awb->gain01 = ...;\n> + * awb->gain10 = ...;\n> + * awb->gain11 = ...;\n> + * \\endcode\n> + */\n> +\n> +/**\n> + * \\fn V4L2Params::V4L2Params()\n> + * \\brief Construct a V4L2Params\n\ns/a/an instance of\n\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\nI think I would prefer to call this \"used()\"; to me \"size()\" should return either the maximum size \nof the flexible data member or else the size of the entire buffer.\n\n> + */\n> +\n> +/**\n> + * \\fn V4L2Params::block()\n> + * \\brief Retrieve the location of an ISP configuration block a returns it\n\nEither \"Retrieve the location of an ISP configuration block and return it\" or \"Retrieves the \nlocation of an ISP configuration block and returns it\".\n\n> + * \\return A V4L2ParamsBlock instance that points to the ISP configuration block\n> + */\n> +\n> +/**\n> + * \\fn V4L2Params::block(typename Traits::id_type type, unsigned int blockType, size_t blockSize)\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> + * 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<T>\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..bf98bb2aba88506e3ad304a995505deba8e0712a\n> --- /dev/null\n> +++ b/src/ipa/libipa/v4l2_params.h\n> @@ -0,0 +1,142 @@\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 <map>\n> +#include <stdint.h>\n> +#include <string.h>\n> +\n> +#include <linux/media/v4l2-isp.h>\n> +\n> +#include <libcamera/base/span.h>\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +template<typename T>\n> +class V4L2ParamsBlock\n> +{\n> +public:\n> +\tV4L2ParamsBlock(const Span<uint8_t> data)\n> +\t\t: data_(data)\n> +\t{\n> +\t}\n> +\n> +\tvirtual ~V4L2ParamsBlock() {}\n> +\n> +\tvirtual void setEnabled(bool enabled)\n> +\t{\n> +\t\tstruct v4l2_isp_params_block_header *header =\n> +\t\t\treinterpret_cast<struct v4l2_isp_params_block_header *>(data_.data());\n> +\n> +\t\theader->flags &= ~(V4L2_ISP_PARAMS_FL_BLOCK_ENABLE |\n> +\t\t\t\t   V4L2_ISP_PARAMS_FL_BLOCK_DISABLE);\n> +\t\theader->flags |= enabled ? V4L2_ISP_PARAMS_FL_BLOCK_ENABLE\n> +\t\t\t\t\t : V4L2_ISP_PARAMS_FL_BLOCK_DISABLE;\n> +\t}\n> +\n> +\tvirtual const T *operator->() const\n> +\t{\n> +\t\treturn reinterpret_cast<const T *>(data_.data());\n> +\t}\n> +\n> +\tvirtual T *operator->()\n> +\t{\n> +\t\treturn reinterpret_cast<T *>(data_.data());\n> +\t}\n> +\n> +\tvirtual const T &operator*() const\n> +\t{\n> +\t\treturn *reinterpret_cast<const T *>(data_.data());\n> +\t}\n> +\n> +\tvirtual T &operator*()\n> +\t{\n> +\t\treturn *reinterpret_cast<T *>(data_.data());\n> +\t}\n> +\n> +protected:\n> +\tSpan<uint8_t> data_;\n> +};\n> +\n> +template<typename Traits>\n> +class V4L2Params\n> +{\n> +public:\n> +\tV4L2Params(Span<uint8_t> data, unsigned int version)\n> +\t\t: data_(data)\n> +\t{\n> +\t\tstruct v4l2_isp_params_buffer *cfg =\n> +\t\t\treinterpret_cast<struct v4l2_isp_params_buffer *>(data_.data());\n> +\t\tcfg->data_size = 0;\n> +\t\tcfg->version = version;\n> +\t\tused_ = offsetof(struct v4l2_isp_params_buffer, data);\n> +\t}\n> +\n> +\tsize_t size() const { return used_; }\n> +\n> +\ttemplate<typename Traits::id_type Id>\n> +\tauto block()\n> +\t{\n> +\t\tusing Details = typename Traits::template id_to_details<Id>;\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> +\t\treturn V4L2ParamsBlock<Type>(data);\n\nWhat happens if the type ID isn't one of the ones defined for the IPA?\n\n> +\t}\n> +\n> +protected:\n> +\tSpan<uint8_t> block(typename Traits::id_type type,\n> +\t\t\t    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> +\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> +\t\t/* Make sure we don't run out of space. */\n> +\t\tif (blockSize > data_.size() - used_)\n> +\t\t\treturn {};\n\nThis seems a little risky to me, because we pass that empty Span to V4L2ParamsBlock in its \nconstructor, which simply stores it and returns. Shouldn't something error out in that chain of events?\n\n> +\n> +\t\t/* Allocate a new block, clear its memory, and initialize its header. */\n> +\t\tSpan<uint8_t> block = data_.subspan(used_, blockSize);\n> +\t\tused_ += blockSize;\n> +\n> +\t\tstruct v4l2_isp_params_buffer *cfg =\n> +\t\t\treinterpret_cast<struct v4l2_isp_params_buffer *>(data_.data());\n\nIs it worth just storing this pointer in the constructor perhaps?\n\n> +\t\tcfg->data_size += blockSize;\n> +\n> +\t\tmemset(block.data(), 0, block.size());\n> +\n> +\t\tstruct v4l2_isp_params_block_header *header =\n> +\t\t\treinterpret_cast<struct v4l2_isp_params_block_header *>(block.data());\n> +\t\theader->type = blockType;\n> +\t\theader->size = block.size();\n\nYou could avoid another cast here I think by doing this in the outer block() with something like...\n\nblock = V4L2ParamsBlock<Type>(data);\nblock->header.type = kernelId;\nblock->header.size = sizeof(Type);\n\nreturn block;\n\nA later edit: Although perhaps that doesn't make sense, as far as I know nothing actually guarantees \nthat the extensible params block header is called \"header\"\n\nThanks\nDan\n\n> +\n> +\t\t/* Update the cache. */\n> +\t\tblocks_[type] = block;\n> +\n> +\t\treturn block;\n> +\t}\n> +\n> +\tSpan<uint8_t> data_;\n> +\tsize_t used_;\n> +\n> +\tstd::map<typename Traits::id_type, Span<uint8_t>> 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 5edb36c91b87859d02c0a8b41efe977ff048def5..7040207c26557aa278050a1f7232cc6c380505b1 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> @@ -79,56 +79,6 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {\n>   \n>   } /* namespace */\n>   \n> -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> -\t\t\t\t\t     const Span<uint8_t> &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<struct rkisp1_ext_params_block_header *>(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<uint8_t> 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<struct rkisp1_ext_params_cfg *>(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> @@ -178,44 +128,7 @@ Span<uint8_t> 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<uint8_t> block = data_.subspan(used_, size);\n> -\tused_ += size;\n> -\n> -\tstruct rkisp1_ext_params_cfg *cfg =\n> -\t\treinterpret_cast<struct rkisp1_ext_params_cfg *>(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<struct rkisp1_ext_params_block_header *>(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 2e60528d102ec44a31417d4b146e74cace363efa..eddb37d5c000b6b1de1c698ad915f2b42da58b81 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 <map>\n> -#include <stdint.h>\n> -\n>   #include <linux/rkisp1-config.h>\n> +#include <linux/videodev2.h>\n>   \n> -#include <libcamera/base/class.h>\n> -#include <libcamera/base/span.h>\n> +#include <libipa/v4l2_params.h>\n>   \n>   namespace libcamera {\n>   \n> @@ -49,115 +46,143 @@ template<BlockType B>\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<BlockType::blockType> {\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(Wdr, wdr)\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> +RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr, WDR)\n> +\n> +struct params_traits {\n> +\tusing id_type = BlockType;\n> +\n> +\ttemplate<id_type Id>\n> +\tusing id_to_details = block_type<Id>;\n> +};\n>   \n>   } /* namespace details */\n>   \n> -class RkISP1Params;\n> +template<typename T>\n> +class RkISP1ParamsBlock;\n>   \n> -class RkISP1ParamsBlockBase\n> +class RkISP1Params : public V4L2Params<details::params_traits>\n>   {\n>   public:\n> -\tRkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> -\t\t\t      const Span<uint8_t> &data);\n> +\tstatic constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;\n> +\n> +\tRkISP1Params(uint32_t format, Span<uint8_t> 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> +\ttemplate<details::params_traits::id_type id>\n> +\tauto block()\n> +\t{\n> +\t\tusing Type = typename details::block_type<id>::type;\n>   \n> -\tSpan<uint8_t> data() const { return data_; }\n> +\t\treturn RkISP1ParamsBlock<Type>(this, id, block(id));\n> +\t}\n>   \n> -\tvoid setEnabled(bool enabled);\n> +\tuint32_t format() const { return format_; }\n> +\tvoid setBlockEnabled(BlockType type, bool enabled);\n>   \n>   private:\n> -\tLIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)\n> +\tSpan<uint8_t> block(BlockType type);\n>   \n> -\tRkISP1Params *params_;\n> -\tBlockType type_;\n> -\tSpan<uint8_t> header_;\n> -\tSpan<uint8_t> data_;\n> +\tuint32_t format_;\n>   };\n>   \n> -template<BlockType B>\n> -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> +template<typename T>\n> +class RkISP1ParamsBlock final : public V4L2ParamsBlock<T>\n>   {\n>   public:\n> -\tusing Type = typename details::block_type<B>::type;\n> -\n> -\tRkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data)\n> -\t\t: RkISP1ParamsBlockBase(params, B, data)\n> +\tRkISP1ParamsBlock(RkISP1Params *params, BlockType type,\n> +\t\t\t  const Span<uint8_t> data)\n> +\t\t: V4L2ParamsBlock<T>(data)\n>   \t{\n> +\t\tparams_ = params;\n> +\t\ttype_ = type;\n> +\n> +\t\t/*\n> +\t\t * cifData_ points to the actual configuration data\n> +\t\t * (struct rkisp1_cif_isp_*) which is not prefixed by any header,\n> +\t\t * for the legacy fixed format.\n> +\t\t */\n> +\t\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> +\t\t\tcifData_ = data;\n> +\t\telse\n> +\t\t\tcifData_ = data.subspan(sizeof(v4l2_isp_params_block_header));\n>   \t}\n>   \n> -\tconst Type *operator->() const\n> +\tvoid setEnabled(bool enabled) override\n>   \t{\n> -\t\treturn reinterpret_cast<const Type *>(data().data());\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<T>::setEnabled(enabled);\n>   \t}\n>   \n> -\tType *operator->()\n> +\t/*\n> +\t * Override the dereference operators to return a reference to the\n> +\t * actual configuration data (struct rkisp1_cif_isp_*) skipping the\n> +\t * 'v4l2_isp_params_block_header' header.\n> +\t */\n> +\n> +\tvirtual const T *operator->() const override\n>   \t{\n> -\t\treturn reinterpret_cast<Type *>(data().data());\n> +\t\treturn reinterpret_cast<const T *>(cifData_.data());\n>   \t}\n>   \n> -\tconst Type &operator*() const &\n> +\tvirtual T *operator->() override\n>   \t{\n> -\t\treturn *reinterpret_cast<const Type *>(data().data());\n> +\t\treturn reinterpret_cast<T *>(cifData_.data());\n>   \t}\n>   \n> -\tType &operator*() &\n> +\tvirtual const T &operator*() const override\n>   \t{\n> -\t\treturn *reinterpret_cast<Type *>(data().data());\n> +\t\treturn *reinterpret_cast<const T *>(cifData_.data());\n>   \t}\n> -};\n> -\n> -class RkISP1Params\n> -{\n> -public:\n> -\tRkISP1Params(uint32_t format, Span<uint8_t> data);\n>   \n> -\ttemplate<BlockType B>\n> -\tRkISP1ParamsBlock<B> block()\n> +\tvirtual T &operator*() override\n>   \t{\n> -\t\treturn RkISP1ParamsBlock<B>(this, block(B));\n> +\t\treturn *reinterpret_cast<T *>(cifData_.data());\n>   \t}\n>   \n> -\tuint32_t format() const { return format_; }\n> -\tsize_t size() const { return used_; }\n> -\n>   private:\n> -\tfriend class RkISP1ParamsBlockBase;\n> -\n> -\tSpan<uint8_t> block(BlockType type);\n> -\tvoid setBlockEnabled(BlockType type, bool enabled);\n> -\n> -\tuint32_t format_;\n> -\n> -\tSpan<uint8_t> data_;\n> -\tsize_t used_;\n> -\n> -\tstd::map<BlockType, Span<uint8_t>> blocks_;\n> +\tRkISP1Params *params_;\n> +\tBlockType type_;\n> +\tSpan<uint8_t> cifData_;\n>   };\n>   \n>   } /* namespace ipa::rkisp1 */\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 12B4ABF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Oct 2025 15:00:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2DCF36B60E;\n\tFri, 10 Oct 2025 17:00:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DEC946B599\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Oct 2025 17:00:26 +0200 (CEST)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 262991807\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Oct 2025 16:58:51 +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=\"iivk/8fW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760108331;\n\tbh=FzsKOohv/J6lkl+zokUytpqqcrhQ0UknJX8a/IlLOdQ=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=iivk/8fWb/WXRDZ3vLnqjbcb37kM5xly1K0I5IdFKyF7o+u6du2d9mlklxHKB5xzr\n\tRTs7TKxjH7bpHKoNrjwlFLyar/oiW2+78yGxxH925QftjU5wGtUbsAPTpXTXiU527N\n\tYnM+o1BXetAtZ50HMMyKpYi5SqFiCTzDJ9vKb7ho=","Message-ID":"<39dc8b20-dfbe-41e2-8ea8-0ce9853f860b@ideasonboard.com>","Date":"Fri, 10 Oct 2025 16:00:23 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 4/5] ipa: libipa: Introduce V4L2Params","To":"libcamera-devel@lists.libcamera.org","References":"<20251007-v4l2-params-v5-0-8db451a81398@ideasonboard.com>\n\t<20251007-v4l2-params-v5-4-8db451a81398@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally <dan.scally@ideasonboard.com>","In-Reply-To":"<20251007-v4l2-params-v5-4-8db451a81398@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36200,"web_url":"https://patchwork.libcamera.org/comment/36200/","msgid":"<d7re6dwxej4dnfup5moiaqnwqoyftcj4zwbpqq55zrprxm64ja@zpw6tik7fmug>","date":"2025-10-10T16:13:33","subject":"Re: [PATCH v5 4/5] 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 Dan\n\n   thanks for review\n\nOn Fri, Oct 10, 2025 at 04:00:23PM +0100, Dan Scally wrote:\n> Hi Jacopo\n>\n> On 07/10/2025 19:17, Jacopo Mondi wrote:\n> > The existing RkISP1Params helper class allows the RkISP1 IPA to handle\n> > both the extensible parameters format and the legacy fixed-size format.\n> >\n> > With the introduction of v4l2-isp.h 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 parameters buffer compatible with v4l2-isp.h.\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 <jacopo.mondi@ideasonboard.com>\n> > Tested-by: Antoine Bouyer <antoine.bouyer@nxp.com>\n> > ---\n> >   src/ipa/libipa/meson.build     |   2 +\n> >   src/ipa/libipa/v4l2_params.cpp | 254 +++++++++++++++++++++++++++++++++++++++++\n> >   src/ipa/libipa/v4l2_params.h   | 142 +++++++++++++++++++++++\n> >   src/ipa/rkisp1/params.cpp      |  93 +--------------\n> >   src/ipa/rkisp1/params.h        | 175 ++++++++++++++++------------\n> >   5 files changed, 501 insertions(+), 165 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>\n> These two aren't alphabetical.>\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..278b5e4259be4798d0f719c8b1876ef654a14c75\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/v4l2_params.cpp\n> > @@ -0,0 +1,254 @@\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 an ISP configuration buffer compatible with\n> > + * the generic V4L2 ISP format\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 ISP generic parameters. The\n> > + * V4L2 ISP parameters define a serialization format for ISP parameters that\n> > + * allows userspace to populate a buffer of configuration data by appending them\n> > + * one after the other in a binary buffer.\n>\n> I think this could do with some revision. Perhaps something like:\n>\n> The Linux kernel defines a generic buffer format for configuring ISP\n> devices. The format describes a serialisation method for ISP parameters that\n> allows userspace to populate a buffer of configuration data by appending\n> blocks to the buffer with common headers but device-specific contents one\n> after the other.\n>\nThanks! reads way better\n\n\n> > + *\n> > + * The V4L2Params class implementes support for working with V4L2 ISP parameters\n>\n> s/implementes/implements. And maybe just \"...implements support for the V4L2\n> ISP parameters buffer format and allows...\"> + * buffer and allows users to\n> populate the ISP configuration blocks, represented\n\nack\n\n> > + * as 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>\n> s/the IPA module supports/supported by the IPA module. s/abstraction/abstractions\n\nindeed\n\n>\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 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::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<struct my_isp_awb_config_data> 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 shall not create a V4L2ParamsBlock manually but should\n> > + * use 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> > + * \\var V4L2ParamsBlock::data_\n> > + * \\brief Memory area reserved for the ISP configuration block\n> > + */\n> > +\n> > + /**\n> > +  * \\class V4L2Params\n> > +  * \\brief Helper class that represent an ISP configuration buffer\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> > + * driver.\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 by providing a 'param_traits' type the helps the class associate\n>\n> s/type the helps/type that helps\n>\n> > + * a block type with the actual memory area that represents the ISP\n> > + * configuration block.\n> > + *\n> > + * \\code{.cpp}\n> > + *\n> > + * // Define the supported ISP blocks\n> > + * enum class myISPBlocks {\n> > + *\tAgc,\n> > + *\tAwb,\n> > + *\t...\n> > + * };\n> > + *\n> > + * // Maps the C++ enum type to the kernel enum type and concrete parameter type\n> > + * template<myISPBlocks B>\n> > + * struct block_type {\n> > + * };\n> > + *\n> > + * template<>\n> > + * struct block_type<myISPBlock::Agc> {\n> > + *\tusing type = struct my_isp_kernel_config_type_agc;\n> > + *\tstatic constexpr kernel_enum_type blockType = MY_ISP_TYPE_AGC;\n> > + * };\n> > + *\n> > + * template<>\n> > + * struct block_type<myISPBlock::Awb> {\n> > + *\tusing type = struct my_isp_kernel_config_type_awb;\n> > + *\tstatic constexpr kernel_enum_type blockType = MY_ISP_TYPE_AWB;\n> > + * };\n> > + *\n> > + *\n> > + * // Convenience type to associate a block id to the 'block_type' overload\n> > + * struct params_traits {\n> > + * \tusing id_type = myISPBlocks;\n> > + * \ttemplate<id_type Id> using id_to_details = block_type<Id>;\n> > + * };\n> > + *\n> > + * ...\n> > + *\n> > + * // Derive the V4L2Params class by providing params_traits\n> > + * class MyISPParams : public V4L2Params<params_traits>\n> > + * {\n> > + * public:\n> > + * \tMyISPParams::MyISPParams(Span<uint8_t> data)\n> > + * \t\t: V4L2Params(data, kVersion)\n> > + * \t{\n> > + * \t}\n> > + * };\n> > + *\n> > + * \\endcode\n> > + *\n> > + * Users of this class can then easily access an ISP configuration block as a\n> > + * V4L2ParamsBlock instance.\n> > + *\n> > + * \\code{.cpp}\n> > + *\n> > + * MyISPParams params(data);\n> > + *\n> > + * auto awb = params.block<myISPBlocks::AWB>();\n> > + * awb->gain00 = ...;\n> > + * awb->gain01 = ...;\n> > + * awb->gain10 = ...;\n> > + * awb->gain11 = ...;\n> > + * \\endcode\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Params::V4L2Params()\n> > + * \\brief Construct a V4L2Params\n>\n> s/a/an instance of\n>\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> I think I would prefer to call this \"used()\"; to me \"size()\" should return\n> either the maximum size of the flexible data member or else the size of the\n> entire buffer.\n\nor, as you suggested below bytesused() ?\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Params::block()\n> > + * \\brief Retrieve the location of an ISP configuration block a returns it\n>\n> Either \"Retrieve the location of an ISP configuration block and return it\"\n> or \"Retrieves the location of an ISP configuration block and returns it\".\n>\n\nI think we mostly use imperative ? I'll check\n\n> > + * \\return A V4L2ParamsBlock instance that points to the ISP configuration block\n> > + */\n> > +\n> > +/**\n> > + * \\fn V4L2Params::block(typename Traits::id_type type, unsigned int blockType, size_t blockSize)\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> > + * 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<T>\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..bf98bb2aba88506e3ad304a995505deba8e0712a\n> > --- /dev/null\n> > +++ b/src/ipa/libipa/v4l2_params.h\n> > @@ -0,0 +1,142 @@\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 <map>\n> > +#include <stdint.h>\n> > +#include <string.h>\n> > +\n> > +#include <linux/media/v4l2-isp.h>\n> > +\n> > +#include <libcamera/base/span.h>\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa {\n> > +\n> > +template<typename T>\n> > +class V4L2ParamsBlock\n> > +{\n> > +public:\n> > +\tV4L2ParamsBlock(const Span<uint8_t> data)\n> > +\t\t: data_(data)\n> > +\t{\n> > +\t}\n> > +\n> > +\tvirtual ~V4L2ParamsBlock() {}\n> > +\n> > +\tvirtual void setEnabled(bool enabled)\n> > +\t{\n> > +\t\tstruct v4l2_isp_params_block_header *header =\n> > +\t\t\treinterpret_cast<struct v4l2_isp_params_block_header *>(data_.data());\n> > +\n> > +\t\theader->flags &= ~(V4L2_ISP_PARAMS_FL_BLOCK_ENABLE |\n> > +\t\t\t\t   V4L2_ISP_PARAMS_FL_BLOCK_DISABLE);\n> > +\t\theader->flags |= enabled ? V4L2_ISP_PARAMS_FL_BLOCK_ENABLE\n> > +\t\t\t\t\t : V4L2_ISP_PARAMS_FL_BLOCK_DISABLE;\n> > +\t}\n> > +\n> > +\tvirtual const T *operator->() const\n> > +\t{\n> > +\t\treturn reinterpret_cast<const T *>(data_.data());\n> > +\t}\n> > +\n> > +\tvirtual T *operator->()\n> > +\t{\n> > +\t\treturn reinterpret_cast<T *>(data_.data());\n> > +\t}\n> > +\n> > +\tvirtual const T &operator*() const\n> > +\t{\n> > +\t\treturn *reinterpret_cast<const T *>(data_.data());\n> > +\t}\n> > +\n> > +\tvirtual T &operator*()\n> > +\t{\n> > +\t\treturn *reinterpret_cast<T *>(data_.data());\n> > +\t}\n> > +\n> > +protected:\n> > +\tSpan<uint8_t> data_;\n> > +};\n> > +\n> > +template<typename Traits>\n> > +class V4L2Params\n> > +{\n> > +public:\n> > +\tV4L2Params(Span<uint8_t> data, unsigned int version)\n> > +\t\t: data_(data)\n> > +\t{\n> > +\t\tstruct v4l2_isp_params_buffer *cfg =\n> > +\t\t\treinterpret_cast<struct v4l2_isp_params_buffer *>(data_.data());\n> > +\t\tcfg->data_size = 0;\n> > +\t\tcfg->version = version;\n> > +\t\tused_ = offsetof(struct v4l2_isp_params_buffer, data);\n> > +\t}\n> > +\n> > +\tsize_t size() const { return used_; }\n> > +\n> > +\ttemplate<typename Traits::id_type Id>\n> > +\tauto block()\n> > +\t{\n> > +\t\tusing Details = typename Traits::template id_to_details<Id>;\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> > +\t\treturn V4L2ParamsBlock<Type>(data);\n>\n>What happens if the type ID isn't one of the ones defined for the IPA?\n\nWhat 'type ID' ? :)\n\nAre you suggesting what happens in an IPA defines a valid value for\nthe template argument 'Traits::id_type Id' in the block ids\nenumeration, but doesn't define any 'struct block_type<ID>' ?\n\nYou get a compiler error I presume as the template generation fails\nbecause the compiler can't resolve id_to_details<Id>;\n\n>\n> > +\t}\n> > +\n> > +protected:\n> > +\tSpan<uint8_t> block(typename Traits::id_type type,\n> > +\t\t\t    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> > +\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> > +\t\t/* Make sure we don't run out of space. */\n> > +\t\tif (blockSize > data_.size() - used_)\n> > +\t\t\treturn {};\n>\n> This seems a little risky to me, because we pass that empty Span to\n> V4L2ParamsBlock in its constructor, which simply stores it and returns.\n> Shouldn't something error out in that chain of events?\n>\n\nWe should at least warn, at least\n\nThis should segfault as soon as some tries to access the\nV4L2ParamsBlock<> constructed with an empty span, though..\n\n(confirmed, it does)\n\nhow handle it more gracefully... I'm not sure. We could assert, it's\nequally harsh but it provides more context.\n\n\n> > +\n> > +\t\t/* Allocate a new block, clear its memory, and initialize its header. */\n> > +\t\tSpan<uint8_t> block = data_.subspan(used_, blockSize);\n> > +\t\tused_ += blockSize;\n> > +\n> > +\t\tstruct v4l2_isp_params_buffer *cfg =\n> > +\t\t\treinterpret_cast<struct v4l2_isp_params_buffer *>(data_.data());\n>\n> Is it worth just storing this pointer in the constructor perhaps?\n>\n\nAs we access it at construction time, probably yes\n\n> > +\t\tcfg->data_size += blockSize;\n> > +\n> > +\t\tmemset(block.data(), 0, block.size());\n> > +\n> > +\t\tstruct v4l2_isp_params_block_header *header =\n> > +\t\t\treinterpret_cast<struct v4l2_isp_params_block_header *>(block.data());\n> > +\t\theader->type = blockType;\n> > +\t\theader->size = block.size();\n>\n> You could avoid another cast here I think by doing this in the outer block() with something like...\n>\n> block = V4L2ParamsBlock<Type>(data);\n> block->header.type = kernelId;\n> block->header.size = sizeof(Type);\n>\n> return block;\n>\n\nIt should be possible. But the rkisp1 overload then would have to do\nthe same.. at the cost of a cast, I would prefer to return an\ninitialized block from here\n\n> A later edit: Although perhaps that doesn't make sense, as far as I know\n> nothing actually guarantees that the extensible params block header is\n> called \"header\"\n\nThat too, yes\n\nThanks\n  j\n\n>\n> Thanks\n> Dan\n>\n> > +\n> > +\t\t/* Update the cache. */\n> > +\t\tblocks_[type] = block;\n> > +\n> > +\t\treturn block;\n> > +\t}\n> > +\n> > +\tSpan<uint8_t> data_;\n> > +\tsize_t used_;\n> > +\n> > +\tstd::map<typename Traits::id_type, Span<uint8_t>> 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 5edb36c91b87859d02c0a8b41efe977ff048def5..7040207c26557aa278050a1f7232cc6c380505b1 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> > @@ -79,56 +79,6 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {\n> >   } /* namespace */\n> > -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> > -\t\t\t\t\t     const Span<uint8_t> &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<struct rkisp1_ext_params_block_header *>(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<uint8_t> 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<struct rkisp1_ext_params_cfg *>(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> > @@ -178,44 +128,7 @@ Span<uint8_t> 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<uint8_t> block = data_.subspan(used_, size);\n> > -\tused_ += size;\n> > -\n> > -\tstruct rkisp1_ext_params_cfg *cfg =\n> > -\t\treinterpret_cast<struct rkisp1_ext_params_cfg *>(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<struct rkisp1_ext_params_block_header *>(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 2e60528d102ec44a31417d4b146e74cace363efa..eddb37d5c000b6b1de1c698ad915f2b42da58b81 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 <map>\n> > -#include <stdint.h>\n> > -\n> >   #include <linux/rkisp1-config.h>\n> > +#include <linux/videodev2.h>\n> > -#include <libcamera/base/class.h>\n> > -#include <libcamera/base/span.h>\n> > +#include <libipa/v4l2_params.h>\n> >   namespace libcamera {\n> > @@ -49,115 +46,143 @@ template<BlockType B>\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<BlockType::blockType> {\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(Wdr, wdr)\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> > +RKISP1_DEFINE_BLOCK_TYPE(Wdr, wdr, WDR)\n> > +\n> > +struct params_traits {\n> > +\tusing id_type = BlockType;\n> > +\n> > +\ttemplate<id_type Id>\n> > +\tusing id_to_details = block_type<Id>;\n> > +};\n> >   } /* namespace details */\n> > -class RkISP1Params;\n> > +template<typename T>\n> > +class RkISP1ParamsBlock;\n> > -class RkISP1ParamsBlockBase\n> > +class RkISP1Params : public V4L2Params<details::params_traits>\n> >   {\n> >   public:\n> > -\tRkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> > -\t\t\t      const Span<uint8_t> &data);\n> > +\tstatic constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;\n> > +\n> > +\tRkISP1Params(uint32_t format, Span<uint8_t> 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> > +\ttemplate<details::params_traits::id_type id>\n> > +\tauto block()\n> > +\t{\n> > +\t\tusing Type = typename details::block_type<id>::type;\n> > -\tSpan<uint8_t> data() const { return data_; }\n> > +\t\treturn RkISP1ParamsBlock<Type>(this, id, block(id));\n> > +\t}\n> > -\tvoid setEnabled(bool enabled);\n> > +\tuint32_t format() const { return format_; }\n> > +\tvoid setBlockEnabled(BlockType type, bool enabled);\n> >   private:\n> > -\tLIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)\n> > +\tSpan<uint8_t> block(BlockType type);\n> > -\tRkISP1Params *params_;\n> > -\tBlockType type_;\n> > -\tSpan<uint8_t> header_;\n> > -\tSpan<uint8_t> data_;\n> > +\tuint32_t format_;\n> >   };\n> > -template<BlockType B>\n> > -class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> > +template<typename T>\n> > +class RkISP1ParamsBlock final : public V4L2ParamsBlock<T>\n> >   {\n> >   public:\n> > -\tusing Type = typename details::block_type<B>::type;\n> > -\n> > -\tRkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data)\n> > -\t\t: RkISP1ParamsBlockBase(params, B, data)\n> > +\tRkISP1ParamsBlock(RkISP1Params *params, BlockType type,\n> > +\t\t\t  const Span<uint8_t> data)\n> > +\t\t: V4L2ParamsBlock<T>(data)\n> >   \t{\n> > +\t\tparams_ = params;\n> > +\t\ttype_ = type;\n> > +\n> > +\t\t/*\n> > +\t\t * cifData_ points to the actual configuration data\n> > +\t\t * (struct rkisp1_cif_isp_*) which is not prefixed by any header,\n> > +\t\t * for the legacy fixed format.\n> > +\t\t */\n> > +\t\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> > +\t\t\tcifData_ = data;\n> > +\t\telse\n> > +\t\t\tcifData_ = data.subspan(sizeof(v4l2_isp_params_block_header));\n> >   \t}\n> > -\tconst Type *operator->() const\n> > +\tvoid setEnabled(bool enabled) override\n> >   \t{\n> > -\t\treturn reinterpret_cast<const Type *>(data().data());\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<T>::setEnabled(enabled);\n> >   \t}\n> > -\tType *operator->()\n> > +\t/*\n> > +\t * Override the dereference operators to return a reference to the\n> > +\t * actual configuration data (struct rkisp1_cif_isp_*) skipping the\n> > +\t * 'v4l2_isp_params_block_header' header.\n> > +\t */\n> > +\n> > +\tvirtual const T *operator->() const override\n> >   \t{\n> > -\t\treturn reinterpret_cast<Type *>(data().data());\n> > +\t\treturn reinterpret_cast<const T *>(cifData_.data());\n> >   \t}\n> > -\tconst Type &operator*() const &\n> > +\tvirtual T *operator->() override\n> >   \t{\n> > -\t\treturn *reinterpret_cast<const Type *>(data().data());\n> > +\t\treturn reinterpret_cast<T *>(cifData_.data());\n> >   \t}\n> > -\tType &operator*() &\n> > +\tvirtual const T &operator*() const override\n> >   \t{\n> > -\t\treturn *reinterpret_cast<Type *>(data().data());\n> > +\t\treturn *reinterpret_cast<const T *>(cifData_.data());\n> >   \t}\n> > -};\n> > -\n> > -class RkISP1Params\n> > -{\n> > -public:\n> > -\tRkISP1Params(uint32_t format, Span<uint8_t> data);\n> > -\ttemplate<BlockType B>\n> > -\tRkISP1ParamsBlock<B> block()\n> > +\tvirtual T &operator*() override\n> >   \t{\n> > -\t\treturn RkISP1ParamsBlock<B>(this, block(B));\n> > +\t\treturn *reinterpret_cast<T *>(cifData_.data());\n> >   \t}\n> > -\tuint32_t format() const { return format_; }\n> > -\tsize_t size() const { return used_; }\n> > -\n> >   private:\n> > -\tfriend class RkISP1ParamsBlockBase;\n> > -\n> > -\tSpan<uint8_t> block(BlockType type);\n> > -\tvoid setBlockEnabled(BlockType type, bool enabled);\n> > -\n> > -\tuint32_t format_;\n> > -\n> > -\tSpan<uint8_t> data_;\n> > -\tsize_t used_;\n> > -\n> > -\tstd::map<BlockType, Span<uint8_t>> blocks_;\n> > +\tRkISP1Params *params_;\n> > +\tBlockType type_;\n> > +\tSpan<uint8_t> cifData_;\n> >   };\n> >   } /* namespace ipa::rkisp1 */\n> >\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 BF8A6BE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Oct 2025 16:13:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F1B606B60E;\n\tFri, 10 Oct 2025 18:13:38 +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 61D426B599\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Oct 2025 18:13:37 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8C456EFE;\n\tFri, 10 Oct 2025 18:12:01 +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=\"DyAiwZqU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1760112721;\n\tbh=JqAv6oSRuCXKo7J7wUXri/Wg4+l8sR2Tvcapm6Bkr+A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DyAiwZqUq2YJPrDTV3pFs/hFbxI0vb/LckDO/PAGsOXxXMXqFYDCihcyDXsfeGEOm\n\tNAvEcKSCQ+y9sgo62fGs2Lhk1urf6tJQOQe5LlepzR1YfWg8t8mDNOdoSexyJWJ/JE\n\tCVi+uTgpMXZW1xw3+cxZAZmpcMd/vvsic7o4aMfc=","Date":"Fri, 10 Oct 2025 18:13:33 +0200","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Dan Scally <dan.scally@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 4/5] ipa: libipa: Introduce V4L2Params","Message-ID":"<d7re6dwxej4dnfup5moiaqnwqoyftcj4zwbpqq55zrprxm64ja@zpw6tik7fmug>","References":"<20251007-v4l2-params-v5-0-8db451a81398@ideasonboard.com>\n\t<20251007-v4l2-params-v5-4-8db451a81398@ideasonboard.com>\n\t<39dc8b20-dfbe-41e2-8ea8-0ce9853f860b@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<39dc8b20-dfbe-41e2-8ea8-0ce9853f860b@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]