[{"id":37236,"web_url":"https://patchwork.libcamera.org/comment/37236/","msgid":"<176528394977.890597.12318068283110699724@ping.linuxembedded.co.uk>","date":"2025-12-09T12:39:09","subject":"Re: [PATCH v6 4/5] ipa: libipa: Introduce V4L2Params","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2025-10-14 11:55:39)\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\nsome very tiny nits below but nothing that I would consider block this:\n\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> ---\n>  src/ipa/libipa/meson.build     |   2 +\n>  src/ipa/libipa/v4l2_params.cpp | 254 +++++++++++++++++++++++++++++++++++++++++\n>  src/ipa/libipa/v4l2_params.h   | 152 ++++++++++++++++++++++++\n>  src/ipa/rkisp1/params.cpp      |  93 +--------------\n>  src/ipa/rkisp1/params.h        | 175 ++++++++++++++++------------\n>  src/ipa/rkisp1/rkisp1.cpp      |   2 +-\n>  6 files changed, 512 insertions(+), 166 deletions(-)\n> \n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 660be94054fa98b714b6bc586039081e45a6b4bc..7202df869c2fe4d58586f72f0c3d7c9fb1472d23 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -17,6 +17,7 @@ libipa_headers = files([\n>      'lux.h',\n>      'module.h',\n>      'pwl.h',\n> +    'v4l2_params.h',\n>  ])\n>  \n>  libipa_sources = files([\n> @@ -36,6 +37,7 @@ libipa_sources = files([\n>      'lux.cpp',\n>      'module.cpp',\n>      'pwl.cpp',\n> +    'v4l2_params.cpp',\n>  ])\n>  \n>  libipa_includes = include_directories('..')\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..ea0cc85471f1289ecd227d9833ed20d5851d404e\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> + * The format describes a serialisation method for ISP parameters that allows\n> + * userspace to populate a buffer of configuration data by appending blocks to\n> + * the buffer with common headers but device-specific contents one after the\n> + * other.\n> + *\n> + * The V4L2Params class implements support the V4L2 ISP parameters buffer format\n> + * and allows users to populate the ISP configuration blocks, represented as\n> + * V4L2ParamBlock class instances.\n> + *\n> + * IPA implementations using this helpers should define an enumeration of ISP\n\nnit: Either \"these helpers\" or \"this helper\"\n\n> + * blocks supported by the IPA module and use a set of common abstraction to\n> + * help their derived implementation of V4L2Params translate the enumerated ISP\n> + * block identifier to the actual type of the configuration data as defined by\n> + * the 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'an ISP'\n\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\nallows its\n\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> + * //          u16 gain_ch00;\n> + * //          u16 gain_ch01;\n> + * //          u16 gain_ch10;\n> + * //          u16 gain_ch11;\n> + * //\n\nCan probably drop this blank line\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 that helps the class associate\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> + *     Agc,\n> + *     Awb,\n> + *     ...\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> + *     using type = struct my_isp_kernel_config_type_agc;\n> + *     static constexpr kernel_enum_type blockType = MY_ISP_TYPE_AGC;\n> + * };\n> + *\n> + * template<>\n> + * struct block_type<myISPBlock::Awb> {\n> + *     using type = struct my_isp_kernel_config_type_awb;\n> + *     static 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> + *     using id_type = myISPBlocks;\n> + *     template<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> + *     MyISPParams::MyISPParams(Span<uint8_t> data)\n> + *             : V4L2Params(data, kVersion)\n> + *     {\n> + *     }\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 an instance of 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::bytesused()\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 Retrieve the location of an ISP configuration block a return it\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..728305c5b7b859815bee6da62ce35c44dd1ec41a\n> --- /dev/null\n> +++ b/src/ipa/libipa/v4l2_params.h\n> @@ -0,0 +1,152 @@\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> +#include <libcamera/base/log.h>\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +LOG_DECLARE_CATEGORY(V4L2Params)\n> +\n> +template<typename T>\n> +class V4L2ParamsBlock\n> +{\n> +public:\n> +       V4L2ParamsBlock(const Span<uint8_t> data)\n> +               : data_(data)\n> +       {\n> +       }\n> +\n> +       virtual ~V4L2ParamsBlock() {}\n> +\n> +       virtual void setEnabled(bool enabled)\n> +       {\n> +               struct v4l2_isp_params_block_header *header =\n> +                       reinterpret_cast<struct v4l2_isp_params_block_header *>(data_.data());\n> +\n> +               header->flags &= ~(V4L2_ISP_PARAMS_FL_BLOCK_ENABLE |\n> +                                  V4L2_ISP_PARAMS_FL_BLOCK_DISABLE);\n> +               header->flags |= enabled ? V4L2_ISP_PARAMS_FL_BLOCK_ENABLE\n> +                                        : V4L2_ISP_PARAMS_FL_BLOCK_DISABLE;\n> +       }\n> +\n> +       virtual const T *operator->() const\n> +       {\n> +               return reinterpret_cast<const T *>(data_.data());\n> +       }\n> +\n> +       virtual T *operator->()\n> +       {\n> +               return reinterpret_cast<T *>(data_.data());\n> +       }\n> +\n> +       virtual const T &operator*() const\n> +       {\n> +               return *reinterpret_cast<const T *>(data_.data());\n> +       }\n> +\n> +       virtual T &operator*()\n> +       {\n> +               return *reinterpret_cast<T *>(data_.data());\n> +       }\n> +\n> +protected:\n> +       Span<uint8_t> data_;\n> +};\n> +\n> +template<typename Traits>\n> +class V4L2Params\n> +{\n> +public:\n> +       V4L2Params(Span<uint8_t> data, unsigned int version)\n> +               : data_(data)\n> +       {\n> +               struct v4l2_isp_params_buffer *params =\n> +                       reinterpret_cast<struct v4l2_isp_params_buffer *>(data_.data());\n> +               params->data_size = 0;\n> +               params->version = version;\n> +\n> +               used_ = offsetof(struct v4l2_isp_params_buffer, data);\n> +       }\n> +\n> +       size_t bytesused() const { return used_; }\n> +\n> +       template<typename Traits::id_type Id>\n> +       auto block()\n> +       {\n> +               using Details = typename Traits::template id_to_details<Id>;\n> +\n> +               using Type = typename Details::type;\n> +               constexpr auto kernelId = Details::blockType;\n> +\n> +               auto data = block(Id, kernelId, sizeof(Type));\n> +               return V4L2ParamsBlock<Type>(data);\n> +       }\n> +\n> +protected:\n> +       Span<uint8_t> block(typename Traits::id_type type,\n> +                           unsigned int blockType, size_t blockSize)\n> +       {\n> +               /*\n> +                * Look up the block in the cache first. If an algorithm\n> +                * requests the same block type twice, it should get the same\n> +                * block.\n> +                */\n> +               auto cacheIt = blocks_.find(type);\n> +               if (cacheIt != blocks_.end())\n> +                       return cacheIt->second;\n> +\n> +               /*\n> +                * Make sure we don't run out of space. Assert as otherwise\n> +                * we get a segfault as soon as someone tries to access the\n> +                * empty Span<> returned from here.\n> +                */\n> +               if (blockSize > data_.size() - used_) {\n> +                       LOG(Fatal) <<\n> +                               \"Parameters buffer out of space; potential version mismatch between driver and libcamera\";\n> +                       return {};\n> +               }\n> +\n> +               /* Allocate a new block, clear its memory, and initialize its header. */\n> +               Span<uint8_t> block = data_.subspan(used_, blockSize);\n> +               memset(block.data(), 0, block.size());\n> +\n> +               struct v4l2_isp_params_block_header *header =\n> +                       reinterpret_cast<struct v4l2_isp_params_block_header *>(block.data());\n> +               header->type = blockType;\n> +               header->size = block.size();\n> +\n> +               used_ += block.size();\n> +\n> +               reinterpret_cast<struct v4l2_isp_params_buffer *>\n> +                       (data_.data())->data_size += block.size();\n> +\n> +               /* Update the cache. */\n> +               blocks_[type] = block;\n> +\n> +               return block;\n> +       }\n> +\n> +       Span<uint8_t> data_;\n> +       size_t used_;\n> +\n> +       std::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)                        \\\n>         { BlockType::block, {                                                   \\\n>                 RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,                              \\\n> -               sizeof(struct rkisp1_cif_isp_##type##_config),                  \\\n> +               sizeof(struct rkisp1_ext_params_##type##_config),               \\\n>                 offsetof(struct rkisp1_params_cfg, category.type##_config),     \\\n>                 RKISP1_CIF_ISP_MODULE_##bit,                                    \\\n>         } }\n> @@ -49,7 +49,7 @@ struct BlockTypeInfo {\n>  #define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)                           \\\n>         { BlockType::block, {                                                   \\\n>                 RKISP1_EXT_PARAMS_BLOCK_TYPE_##id,                              \\\n> -               sizeof(struct rkisp1_cif_isp_##type##_config),                  \\\n> +               sizeof(struct rkisp1_ext_params_##type##_config),               \\\n>                 0, 0,                                                           \\\n>         } }\n>  \n> @@ -79,56 +79,6 @@ const std::map<BlockType, BlockTypeInfo> kBlockTypeInfo = {\n>  \n>  } /* namespace */\n>  \n> -RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> -                                            const Span<uint8_t> &data)\n> -       : params_(params), type_(type)\n> -{\n> -       if (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> -               header_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n> -               data_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n> -       } else {\n> -               data_ = data;\n> -       }\n> -}\n> -\n> -void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n> -{\n> -       /*\n> -        * For the legacy fixed format, blocks are enabled in the top-level\n> -        * header. Delegate to the RkISP1Params class.\n> -        */\n> -       if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> -               return params_->setBlockEnabled(type_, enabled);\n> -\n> -       /*\n> -        * For the extensible format, set the enable and disable flags in the\n> -        * block header directly.\n> -        */\n> -       struct rkisp1_ext_params_block_header *header =\n> -               reinterpret_cast<struct rkisp1_ext_params_block_header *>(header_.data());\n> -       header->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |\n> -                          RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE);\n> -       header->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE\n> -                                : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE;\n> -}\n> -\n> -RkISP1Params::RkISP1Params(uint32_t format, Span<uint8_t> data)\n> -       : format_(format), data_(data), used_(0)\n> -{\n> -       if (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> -               struct rkisp1_ext_params_cfg *cfg =\n> -                       reinterpret_cast<struct rkisp1_ext_params_cfg *>(data.data());\n> -\n> -               cfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n> -               cfg->data_size = 0;\n> -\n> -               used_ += offsetof(struct rkisp1_ext_params_cfg, data);\n> -       } else {\n> -               memset(data.data(), 0, data.size());\n> -               used_ = sizeof(struct rkisp1_params_cfg);\n> -       }\n> -}\n> -\n>  void RkISP1Params::setBlockEnabled(BlockType type, bool enabled)\n>  {\n>         const BlockTypeInfo &info = kBlockTypeInfo.at(type);\n> @@ -178,44 +128,7 @@ Span<uint8_t> RkISP1Params::block(BlockType type)\n>                 return data_.subspan(info.offset, info.size);\n>         }\n>  \n> -       /*\n> -        * For the extensible format, allocate memory for the block, including\n> -        * the header. Look up the block in the cache first. If an algorithm\n> -        * requests the same block type twice, it should get the same block.\n> -        */\n> -       auto cacheIt = blocks_.find(type);\n> -       if (cacheIt != blocks_.end())\n> -               return cacheIt->second;\n> -\n> -       /* Make sure we don't run out of space. */\n> -       size_t size = sizeof(struct rkisp1_ext_params_block_header)\n> -                   + ((info.size + 7) & ~7);\n> -       if (size > data_.size() - used_) {\n> -               LOG(RkISP1Params, Error)\n> -                       << \"Out of memory to allocate block type \"\n> -                       << utils::to_underlying(type);\n> -               return {};\n> -       }\n> -\n> -       /* Allocate a new block, clear its memory, and initialize its header. */\n> -       Span<uint8_t> block = data_.subspan(used_, size);\n> -       used_ += size;\n> -\n> -       struct rkisp1_ext_params_cfg *cfg =\n> -               reinterpret_cast<struct rkisp1_ext_params_cfg *>(data_.data());\n> -       cfg->data_size += size;\n> -\n> -       memset(block.data(), 0, block.size());\n> -\n> -       struct rkisp1_ext_params_block_header *header =\n> -               reinterpret_cast<struct rkisp1_ext_params_block_header *>(block.data());\n> -       header->type = info.type;\n> -       header->size = block.size();\n> -\n> -       /* Update the cache. */\n> -       blocks_[type] = block;\n> -\n> -       return block;\n> +       return V4L2Params::block(type, info.type, info.size);\n\nWell it's definitely nice to see lots of code factored out :D\n\n\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)               \\\n> +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct, id)           \\\n>  template<>                                                             \\\n>  struct block_type<BlockType::blockType> {                              \\\n>         using type = struct rkisp1_cif_isp_##blockStruct##_config;      \\\n> +       static constexpr rkisp1_ext_params_block_type blockType =       \\\n> +               RKISP1_EXT_PARAMS_BLOCK_TYPE_##id;                      \\\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> +       using id_type = BlockType;\n> +\n> +       template<id_type Id>\n> +       using 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> -       RkISP1ParamsBlockBase(RkISP1Params *params, BlockType type,\n> -                             const Span<uint8_t> &data);\n> +       static constexpr unsigned int kVersion = RKISP1_EXT_PARAM_BUFFER_V1;\n> +\n> +       RkISP1Params(uint32_t format, Span<uint8_t> data)\n> +               : V4L2Params(data, kVersion), format_(format)\n> +       {\n> +               if (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n> +                       memset(data.data(), 0, data.size());\n> +                       used_ = sizeof(struct rkisp1_params_cfg);\n> +               }\n> +       }\n> +\n> +       template<details::params_traits::id_type id>\n> +       auto block()\n> +       {\n> +               using Type = typename details::block_type<id>::type;\n>  \n> -       Span<uint8_t> data() const { return data_; }\n> +               return RkISP1ParamsBlock<Type>(this, id, block(id));\n> +       }\n>  \n> -       void setEnabled(bool enabled);\n> +       uint32_t format() const { return format_; }\n> +       void setBlockEnabled(BlockType type, bool enabled);\n>  \n>  private:\n> -       LIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase)\n> +       Span<uint8_t> block(BlockType type);\n>  \n> -       RkISP1Params *params_;\n> -       BlockType type_;\n> -       Span<uint8_t> header_;\n> -       Span<uint8_t> data_;\n> +       uint32_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> -       using Type = typename details::block_type<B>::type;\n> -\n> -       RkISP1ParamsBlock(RkISP1Params *params, const Span<uint8_t> &data)\n> -               : RkISP1ParamsBlockBase(params, B, data)\n> +       RkISP1ParamsBlock(RkISP1Params *params, BlockType type,\n> +                         const Span<uint8_t> data)\n> +               : V4L2ParamsBlock<T>(data)\n>         {\n> +               params_ = params;\n> +               type_ = type;\n> +\n> +               /*\n> +                * cifData_ points to the actual configuration data\n> +                * (struct rkisp1_cif_isp_*) which is not prefixed by any header,\n> +                * for the legacy fixed format.\n> +                */\n> +               if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> +                       cifData_ = data;\n> +               else\n> +                       cifData_ = data.subspan(sizeof(v4l2_isp_params_block_header));\n>         }\n>  \n> -       const Type *operator->() const\n> +       void setEnabled(bool enabled) override\n>         {\n> -               return reinterpret_cast<const Type *>(data().data());\n> +               /*\n> +                * For the legacy fixed format, blocks are enabled in the\n> +                * top-level header. Delegate to the RkISP1Params class.\n> +                */\n> +               if (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> +                       return params_->setBlockEnabled(type_, enabled);\n> +\n> +               return V4L2ParamsBlock<T>::setEnabled(enabled);\n>         }\n>  \n> -       Type *operator->()\n> +       /*\n> +        * Override the dereference operators to return a reference to the\n> +        * actual configuration data (struct rkisp1_cif_isp_*) skipping the\n> +        * 'v4l2_isp_params_block_header' header.\n> +        */\n> +\n> +       virtual const T *operator->() const override\n>         {\n> -               return reinterpret_cast<Type *>(data().data());\n> +               return reinterpret_cast<const T *>(cifData_.data());\n>         }\n>  \n> -       const Type &operator*() const &\n> +       virtual T *operator->() override\n>         {\n> -               return *reinterpret_cast<const Type *>(data().data());\n> +               return reinterpret_cast<T *>(cifData_.data());\n>         }\n>  \n> -       Type &operator*() &\n> +       virtual const T &operator*() const override\n>         {\n> -               return *reinterpret_cast<Type *>(data().data());\n> +               return *reinterpret_cast<const T *>(cifData_.data());\n>         }\n> -};\n> -\n> -class RkISP1Params\n> -{\n> -public:\n> -       RkISP1Params(uint32_t format, Span<uint8_t> data);\n>  \n> -       template<BlockType B>\n> -       RkISP1ParamsBlock<B> block()\n> +       virtual T &operator*() override\n>         {\n> -               return RkISP1ParamsBlock<B>(this, block(B));\n> +               return *reinterpret_cast<T *>(cifData_.data());\n>         }\n>  \n> -       uint32_t format() const { return format_; }\n> -       size_t size() const { return used_; }\n> -\n>  private:\n> -       friend class RkISP1ParamsBlockBase;\n> -\n> -       Span<uint8_t> block(BlockType type);\n> -       void setBlockEnabled(BlockType type, bool enabled);\n> -\n> -       uint32_t format_;\n> -\n> -       Span<uint8_t> data_;\n> -       size_t used_;\n> -\n> -       std::map<BlockType, Span<uint8_t>> blocks_;\n> +       RkISP1Params *params_;\n> +       BlockType type_;\n> +       Span<uint8_t> cifData_;\n>  };\n>  \n>  } /* namespace ipa::rkisp1 */\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index fa22bfc349043002345d275b11a60ac983e329d7..f5f7377b63d21af11473ccc4ab18ab45294743cb 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -349,7 +349,7 @@ void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId)\n>         for (auto const &algo : algorithms())\n>                 algo->prepare(context_, frame, frameContext, &params);\n>  \n> -       paramsComputed.emit(frame, params.size());\n> +       paramsComputed.emit(frame, params.bytesused());\n>  }\n>  \n>  void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,\n> \n> -- \n> 2.51.0\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 1F423C3257\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  9 Dec 2025 12:39:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5063B61408;\n\tTue,  9 Dec 2025 13:39:16 +0100 (CET)","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 82B06606D5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  9 Dec 2025 13:39:14 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D1CFA104D;\n\tTue,  9 Dec 2025 13:39:12 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SD4ATwsc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765283953;\n\tbh=p2VRhhrFtC+sEG1Cf5/BNC4HxZ4OfoCN0SCINaFmhtY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=SD4ATwscyKEXULXr5HvPc6dsFncTBsiqDQnKnhuzVT5GGY8sqtn2MDK7vaGyrZIw+\n\tuWp/AO/iVr8qSxgCNSRXmhdaLxPwsMYI80vttRH3vFXhdO7g7TXd6TyFCeYlmsMBe8\n\tloqOrKWZNQDx9DBpfrt1c9oxZizDz7pbf+lQx/ZI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251014-v4l2-params-v6-4-caf5fa69eb29@ideasonboard.com>","References":"<20251014-v4l2-params-v6-0-caf5fa69eb29@ideasonboard.com>\n\t<20251014-v4l2-params-v6-4-caf5fa69eb29@ideasonboard.com>","Subject":"Re: [PATCH v6 4/5] ipa: libipa: Introduce V4L2Params","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tAntoine Bouyer <antoine.bouyer@nxp.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 09 Dec 2025 12:39:09 +0000","Message-ID":"<176528394977.890597.12318068283110699724@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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>"}}]