[{"id":30348,"web_url":"https://patchwork.libcamera.org/comment/30348/","msgid":"","date":"2024-07-05T12:06:17","subject":"Re: [PATCH v2 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Jul 04, 2024 at 07:20:29PM +0300, Laurent Pinchart wrote:\n> Individual algorithms of the rkisp1 IPA module access their\n> corresponding ISP parameters through the top-level structure\n> rkisp1_params_cfg. This will not work anymore with the new parameters\n> format. In order to ease the transition to the new format, abstract the\n> ISP parameters in a new RkISP1Params class that offers the same\n> interface regardless of the format.\n\nWow that's magical (also not too bad of template magic as I expected).\n\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v1:\n> \n> - Fix module enable and update fields for legacy format\n> - Log messages when block allocation fails\n> - Use uint32_t type for enableBit\n> - Reword comment explaining block caching\n> - Fix State::Disable enumerator value\n> - Disable copying of RkISP1ParamsBlock class\n> - Move the params enabled state handling to a setEnabled() function\n> ---\n> src/ipa/rkisp1/meson.build | 1 +\n> src/ipa/rkisp1/params.cpp | 217 +++++++++++++++++++++++++++++++++++++\n> src/ipa/rkisp1/params.h | 157 +++++++++++++++++++++++++++\n> 3 files changed, 375 insertions(+)\n> create mode 100644 src/ipa/rkisp1/params.cpp\n> create mode 100644 src/ipa/rkisp1/params.h\n> \n> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> index e8b266f1ccca..65eef5d69c32 100644\n> --- a/src/ipa/rkisp1/meson.build\n> +++ b/src/ipa/rkisp1/meson.build\n> @@ -7,6 +7,7 @@ ipa_name = 'ipa_rkisp1'\n> \n> rkisp1_ipa_sources = files([\n> 'ipa_context.cpp',\n> + 'params.cpp',\n> 'rkisp1.cpp',\n> 'utils.cpp',\n> ])\n> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n> new file mode 100644\n> index 000000000000..7155038deb18\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/params.cpp\n> @@ -0,0 +1,217 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * RkISP1 ISP Parameters\n> + */\n> +\n> +#include \"params.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Params)\n> +\n> +namespace ipa::rkisp1 {\n> +\n> +namespace {\n> +\n> +struct BlockTypeInfo {\n> +\tenum rkisp1_ext_params_block_type type;\n> +\tsize_t size;\n> +\tsize_t offset;\n> +\tuint32_t enableBit;\n> +};\n> +\n> +#define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)\t\t\t\\\n> +\t{ Block::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\toffsetof(struct rkisp1_params_cfg, category.type##_config),\t\\\n> +\t\tRKISP1_CIF_ISP_MODULE_##bit,\t\t\t\t\t\\\n> +\t} }\n> +\n> +#define RKISP1_BLOCK_TYPE_ENTRY_MEAS(block, id, type)\t\t\t\t\\\n> +\tRKISP1_BLOCK_TYPE_ENTRY(block, id##_MEAS, type, meas, id)\n> +\n> +#define RKISP1_BLOCK_TYPE_ENTRY_OTHERS(block, id, type)\t\t\t\t\\\n> +\tRKISP1_BLOCK_TYPE_ENTRY(block, id, type, others, id)\n> +\n> +#define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)\t\t\t\t\\\n> +\t{ Block::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\t0, 0,\t\t\t\t\t\t\t\t\\\n> +\t} }\n> +\n> +const std::map kBlockTypeInfo = {\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bls, BLS, bls),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpcc, DPCC, dpcc),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Sdg, SDG, sdg),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(AwbGain, AWB_GAIN, awb_gain),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Flt, FLT, flt),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bdm, BDM, bdm),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ctk, CTK, ctk),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Goc, GOC, goc),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpf, DPF, dpf),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(DpfStrength, DPF_STRENGTH, dpf_strength),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Cproc, CPROC, cproc),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ie, IE, ie),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Lsc, LSC, lsc),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Awb, AWB, awb_meas),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Hst, HST, hst),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Aec, AEC, aec),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Afc, AFC, afc),\n> +};\n> +\n> +} /* namespace */\n> +\n> +RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, Block type,\n> +\t\t\t\t\t const Span &data)\n> +\t: params_(params), type_(type)\n> +{\n> +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> +\t\theader_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n> +\t\tdata_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n> +\t} else {\n> +\t\tdata_ = data;\n> +\t}\n> +}\n> +\n> +void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n> +{\n> +\t/*\n> +\t * For the legacy fixed format, blocks are enabled in the top-level\n> +\t * header. Delegate to the RkISP1Params class.\n> +\t */\n> +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> +\t\treturn params_->setBlockEnabled(type_, enabled);\n> +\n> +\t/*\n> +\t * For the extensible format, set the enable field in the block header\n> +\t * directly.\n> +\t */\n> +\tstruct rkisp1_ext_params_block_header *header =\n> +\t\treinterpret_cast(header_.data());\n> +\theader->enable = enabled ? RKISP1_EXT_PARAMS_BLOCK_ENABLE\n> +\t\t : RKISP1_EXT_PARAMS_BLOCK_DISABLE;\n> +}\n> +\n> +RkISP1Params::RkISP1Params(uint32_t format, Span data)\n> +\t: format_(format), data_(data), used_(0)\n> +{\n> +\tif (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> +\t\tstruct rkisp1_ext_params_cfg *cfg =\n> +\t\t\treinterpret_cast(data.data());\n> +\n> +\t\tcfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n> +\t\tcfg->data_size = 0;\n> +\n> +\t\tused_ += offsetof(struct rkisp1_ext_params_cfg, data);\n> +\t} else {\n> +\t\tmemset(data.data(), 0, data.size());\n> +\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> +\t}\n> +}\n> +\n> +void RkISP1Params::setBlockEnabled(Block type, bool enabled)\n> +{\n> +\tconst BlockTypeInfo &info = kBlockTypeInfo.at(type);\n> +\n> +\tstruct rkisp1_params_cfg *cfg =\n> +\t\treinterpret_cast(data_.data());\n> +\tif (enabled)\n> +\t\tcfg->module_ens |= info.enableBit;\n> +\telse\n> +\t\tcfg->module_ens &= ~info.enableBit;\n> +}\n> +\n> +Span RkISP1Params::block(Block type)\n> +{\n> +\tauto infoIt = kBlockTypeInfo.find(type);\n> +\tif (infoIt == kBlockTypeInfo.end()) {\n> +\t\tLOG(RkISP1Params, Error)\n> +\t\t\t<< \"Invalid parameters block type \"\n> +\t\t\t<< utils::to_underlying(type);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tconst BlockTypeInfo &info = infoIt->second;\n> +\n> +\t/*\n> +\t * For the legacy format, return a block referencing the fixed location\n> +\t * of the data.\n> +\t */\n> +\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n> +\t\t/*\n> +\t\t * Blocks available in extended parameters only have an offset\n\ns/extended/fixed/ ?\n\nReviewed-by: Paul Elder \n\n> +\t\t * of 0. Return nullptr in that case.\n> +\t\t */\n> +\t\tif (info.offset == 0) {\n> +\t\t\tLOG(RkISP1Params, Error)\n> +\t\t\t\t<< \"Block type \" << utils::to_underlying(type)\n> +\t\t\t\t<< \" unavailable in fixed parameters format\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tstruct rkisp1_params_cfg *cfg =\n> +\t\t\treinterpret_cast(data_.data());\n> +\n> +\t\tcfg->module_cfg_update |= info.enableBit;\n> +\t\tcfg->module_en_update |= info.enableBit;\n> +\n> +\t\treturn data_.subspan(info.offset, info.size);\n> +\t}\n> +\n> +\t/*\n> +\t * For the extensible format, allocate memory for the block, including\n> +\t * the header. Look up the block in the cache first. If an algorithm\n> +\t * requests the same block type twice, it should get the same block.\n> +\t */\n> +\tauto cacheIt = blocks_.find(type);\n> +\tif (cacheIt != blocks_.end())\n> +\t\treturn cacheIt->second;\n> +\n> +\t/* Make sure we don't run out of space. */\n> +\tsize_t size = sizeof(struct rkisp1_ext_params_block_header)\n> +\t\t + ((info.size + 7) & ~7);\n> +\tif (size > data_.size() - used_) {\n> +\t\tLOG(RkISP1Params, Error)\n> +\t\t\t<< \"Out of memory to allocate block type \"\n> +\t\t\t<< utils::to_underlying(type);\n> +\t\treturn {};\n> +\t}\n> +\n> +\t/* Allocate a new block, clear its memory, and initialize its header. */\n> +\tSpan block = data_.subspan(used_, size);\n> +\tused_ += size;\n> +\n> +\tstruct rkisp1_ext_params_cfg *cfg =\n> +\t\treinterpret_cast(data_.data());\n> +\tcfg->data_size += size;\n> +\n> +\tmemset(block.data(), 0, block.size());\n> +\n> +\tstruct rkisp1_ext_params_block_header *header =\n> +\t\treinterpret_cast(block.data());\n> +\theader->type = info.type;\n> +\theader->size = block.size();\n> +\n> +\t/* Update the cache. */\n> +\tblocks_[type] = block;\n> +\n> +\treturn block;\n> +}\n> +\n> +} /* namespace ipa::rkisp1 */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> new file mode 100644\n> index 000000000000..7a7bf07299b2\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/params.h\n> @@ -0,0 +1,157 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * RkISP1 ISP Parameters\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1 {\n> +\n> +enum class Block {\n> +\tBls,\n> +\tDpcc,\n> +\tSdg,\n> +\tAwbGain,\n> +\tFlt,\n> +\tBdm,\n> +\tCtk,\n> +\tGoc,\n> +\tDpf,\n> +\tDpfStrength,\n> +\tCproc,\n> +\tIe,\n> +\tLsc,\n> +\tAwb,\n> +\tHst,\n> +\tAec,\n> +\tAfc,\n> +};\n> +\n> +namespace details {\n> +\n> +template\n> +struct block_type {\n> +};\n> +\n> +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)\t\t\\\n> +template<>\t\t\t\t\t\t\t\t\\\n> +struct block_type {\t\t\t\t\t\\\n> +\tusing type = struct rkisp1_cif_isp_##blockStruct##_config;\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> +\n> +} /* namespace details */\n> +\n> +class RkISP1Params;\n> +\n> +class RkISP1ParamsBlockBase\n> +{\n> +public:\n> +\tRkISP1ParamsBlockBase(RkISP1Params *params, Block type,\n> +\t\t\t const Span &data);\n> +\n> +\tSpan data() const { return data_; }\n> +\n> +\tvoid setEnabled(bool enabled);\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase);\n> +\n> +\tRkISP1Params *params_;\n> +\tBlock type_;\n> +\tSpan header_;\n> +\tSpan data_;\n> +};\n> +\n> +template\n> +class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> +{\n> +public:\n> +\tusing Type = typename details::block_type::type;\n> +\n> +\tRkISP1ParamsBlock(RkISP1Params *params, const Span &data)\n> +\t\t: RkISP1ParamsBlockBase(params, B, data)\n> +\t{\n> +\t}\n> +\n> +\tconst Type *operator->() const\n> +\t{\n> +\t\treturn reinterpret_cast(data().data());\n> +\t}\n> +\n> +\tType *operator->()\n> +\t{\n> +\t\treturn reinterpret_cast(data().data());\n> +\t}\n> +\n> +\tconst Type &operator*() const &\n> +\t{\n> +\t\treturn *reinterpret_cast(data().data());\n> +\t}\n> +\n> +\tType &operator*() &\n> +\t{\n> +\t\treturn *reinterpret_cast(data().data());\n> +\t}\n> +};\n> +\n> +class RkISP1Params\n> +{\n> +public:\n> +\tRkISP1Params(uint32_t format, Span data);\n> +\n> +\ttemplate\n> +\tRkISP1ParamsBlock block()\n> +\t{\n> +\t\treturn RkISP1ParamsBlock(this, block(B));\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 block(Block type);\n> +\tvoid setBlockEnabled(Block type, bool enabled);\n> +\n> +\tuint32_t format_;\n> +\n> +\tSpan data_;\n> +\tsize_t used_;\n> +\n> +\tstd::map> blocks_;\n> +};\n> +\n> +} /* namespace ipa::rkisp1 */\n> +\n> +} /* namespace libcamera*/","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D8F90BEFBE\n\tfor ;\n\tFri, 5 Jul 2024 12:06:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7DAEE62E22;\n\tFri, 5 Jul 2024 14:06:26 +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 EBA85619BF\n\tfor ;\n\tFri, 5 Jul 2024 14:06:24 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1DA184CC;\n\tFri, 5 Jul 2024 14:05:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qsnSw37D\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720181155;\n\tbh=YXPVoAqm6BKxLvCepTqX1Jwuo40Fe6nnu4vEOcmChpA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qsnSw37Dmc3sc2gIxCXBhtB9+GM4g6ysA2UbhcC04yNsCN7kM/xt3UT6dcG84hHTA\n\tKFmMnaYD9wBhzrdKUJugtvTwiGIqV0yEZpI5eOprK25d9blK2Ix2SlSixAsJSt7wti\n\twcZFCaOhknyjCoVx7OViOhU7h8cy7/DueZBL/wd8=","Date":"Fri, 5 Jul 2024 21:06:17 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","Subject":"Re: [PATCH v2 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","Message-ID":"","References":"<20240704162035.15074-1-laurent.pinchart@ideasonboard.com>\n\t<20240704162035.15074-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240704162035.15074-6-laurent.pinchart@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30354,"web_url":"https://patchwork.libcamera.org/comment/30354/","msgid":"","date":"2024-07-05T13:24:12","subject":"Re: [PATCH v2 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the patch.\n\nOn Thu, Jul 04, 2024 at 07:20:29PM +0300, Laurent Pinchart wrote:\n> Individual algorithms of the rkisp1 IPA module access their\n> corresponding ISP parameters through the top-level structure\n> rkisp1_params_cfg. This will not work anymore with the new parameters\n> format. In order to ease the transition to the new format, abstract the\n> ISP parameters in a new RkISP1Params class that offers the same\n> interface regardless of the format.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v1:\n> \n> - Fix module enable and update fields for legacy format\n> - Log messages when block allocation fails\n> - Use uint32_t type for enableBit\n> - Reword comment explaining block caching\n> - Fix State::Disable enumerator value\n> - Disable copying of RkISP1ParamsBlock class\n> - Move the params enabled state handling to a setEnabled() function\n> ---\n> src/ipa/rkisp1/meson.build | 1 +\n> src/ipa/rkisp1/params.cpp | 217 +++++++++++++++++++++++++++++++++++++\n> src/ipa/rkisp1/params.h | 157 +++++++++++++++++++++++++++\n> 3 files changed, 375 insertions(+)\n> create mode 100644 src/ipa/rkisp1/params.cpp\n> create mode 100644 src/ipa/rkisp1/params.h\n> \n> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> index e8b266f1ccca..65eef5d69c32 100644\n> --- a/src/ipa/rkisp1/meson.build\n> +++ b/src/ipa/rkisp1/meson.build\n> @@ -7,6 +7,7 @@ ipa_name = 'ipa_rkisp1'\n> \n> rkisp1_ipa_sources = files([\n> 'ipa_context.cpp',\n> + 'params.cpp',\n> 'rkisp1.cpp',\n> 'utils.cpp',\n> ])\n> diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n> new file mode 100644\n> index 000000000000..7155038deb18\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/params.cpp\n> @@ -0,0 +1,217 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * RkISP1 ISP Parameters\n> + */\n> +\n> +#include \"params.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Params)\n> +\n> +namespace ipa::rkisp1 {\n> +\n> +namespace {\n> +\n> +struct BlockTypeInfo {\n> +\tenum rkisp1_ext_params_block_type type;\n> +\tsize_t size;\n> +\tsize_t offset;\n> +\tuint32_t enableBit;\n> +};\n> +\n> +#define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)\t\t\t\\\n> +\t{ Block::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\toffsetof(struct rkisp1_params_cfg, category.type##_config),\t\\\n> +\t\tRKISP1_CIF_ISP_MODULE_##bit,\t\t\t\t\t\\\n> +\t} }\n> +\n> +#define RKISP1_BLOCK_TYPE_ENTRY_MEAS(block, id, type)\t\t\t\t\\\n> +\tRKISP1_BLOCK_TYPE_ENTRY(block, id##_MEAS, type, meas, id)\n> +\n> +#define RKISP1_BLOCK_TYPE_ENTRY_OTHERS(block, id, type)\t\t\t\t\\\n> +\tRKISP1_BLOCK_TYPE_ENTRY(block, id, type, others, id)\n> +\n> +#define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)\t\t\t\t\\\n> +\t{ Block::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\t0, 0,\t\t\t\t\t\t\t\t\\\n> +\t} }\n> +\n> +const std::map kBlockTypeInfo = {\n\nThis took my brain a few cycles more. What about naming the enum\n\"BlockType\" instead of \"Block\". To me mapping a BlockType to a\nBlockTypeInfo feels more natural (the actual place where I stumbled is\nthe declaration of the block cache further down).\n\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bls, BLS, bls),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpcc, DPCC, dpcc),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Sdg, SDG, sdg),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(AwbGain, AWB_GAIN, awb_gain),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Flt, FLT, flt),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bdm, BDM, bdm),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ctk, CTK, ctk),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Goc, GOC, goc),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpf, DPF, dpf),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(DpfStrength, DPF_STRENGTH, dpf_strength),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Cproc, CPROC, cproc),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ie, IE, ie),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Lsc, LSC, lsc),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Awb, AWB, awb_meas),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Hst, HST, hst),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Aec, AEC, aec),\n> +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Afc, AFC, afc),\n> +};\n> +\n> +} /* namespace */\n> +\n> +RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, Block type,\n> +\t\t\t\t\t const Span &data)\n> +\t: params_(params), type_(type)\n> +{\n> +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> +\t\theader_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n> +\t\tdata_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n> +\t} else {\n> +\t\tdata_ = data;\n> +\t}\n> +}\n> +\n> +void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n> +{\n> +\t/*\n> +\t * For the legacy fixed format, blocks are enabled in the top-level\n> +\t * header. Delegate to the RkISP1Params class.\n> +\t */\n> +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> +\t\treturn params_->setBlockEnabled(type_, enabled);\n> +\n> +\t/*\n> +\t * For the extensible format, set the enable field in the block header\n> +\t * directly.\n> +\t */\n> +\tstruct rkisp1_ext_params_block_header *header =\n> +\t\treinterpret_cast(header_.data());\n> +\theader->enable = enabled ? RKISP1_EXT_PARAMS_BLOCK_ENABLE\n> +\t\t : RKISP1_EXT_PARAMS_BLOCK_DISABLE;\n> +}\n> +\n> +RkISP1Params::RkISP1Params(uint32_t format, Span data)\n> +\t: format_(format), data_(data), used_(0)\n> +{\n> +\tif (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> +\t\tstruct rkisp1_ext_params_cfg *cfg =\n> +\t\t\treinterpret_cast(data.data());\n> +\n> +\t\tcfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n> +\t\tcfg->data_size = 0;\n> +\n> +\t\tused_ += offsetof(struct rkisp1_ext_params_cfg, data);\n> +\t} else {\n> +\t\tmemset(data.data(), 0, data.size());\n> +\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> +\t}\n> +}\n> +\n> +void RkISP1Params::setBlockEnabled(Block type, bool enabled)\n> +{\n> +\tconst BlockTypeInfo &info = kBlockTypeInfo.at(type);\n> +\n> +\tstruct rkisp1_params_cfg *cfg =\n> +\t\treinterpret_cast(data_.data());\n> +\tif (enabled)\n> +\t\tcfg->module_ens |= info.enableBit;\n> +\telse\n> +\t\tcfg->module_ens &= ~info.enableBit;\n> +}\n> +\n> +Span RkISP1Params::block(Block type)\n> +{\n> +\tauto infoIt = kBlockTypeInfo.find(type);\n> +\tif (infoIt == kBlockTypeInfo.end()) {\n> +\t\tLOG(RkISP1Params, Error)\n> +\t\t\t<< \"Invalid parameters block type \"\n> +\t\t\t<< utils::to_underlying(type);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tconst BlockTypeInfo &info = infoIt->second;\n> +\n> +\t/*\n> +\t * For the legacy format, return a block referencing the fixed location\n> +\t * of the data.\n> +\t */\n> +\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n> +\t\t/*\n> +\t\t * Blocks available in extended parameters only have an offset\n> +\t\t * of 0. Return nullptr in that case.\n> +\t\t */\n> +\t\tif (info.offset == 0) {\n> +\t\t\tLOG(RkISP1Params, Error)\n> +\t\t\t\t<< \"Block type \" << utils::to_underlying(type)\n> +\t\t\t\t<< \" unavailable in fixed parameters format\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tstruct rkisp1_params_cfg *cfg =\n> +\t\t\treinterpret_cast(data_.data());\n> +\n> +\t\tcfg->module_cfg_update |= info.enableBit;\n> +\t\tcfg->module_en_update |= info.enableBit;\n> +\n> +\t\treturn data_.subspan(info.offset, info.size);\n> +\t}\n> +\n> +\t/*\n> +\t * For the extensible format, allocate memory for the block, including\n> +\t * the header. Look up the block in the cache first. If an algorithm\n> +\t * requests the same block type twice, it should get the same block.\n> +\t */\n> +\tauto cacheIt = blocks_.find(type);\n> +\tif (cacheIt != blocks_.end())\n> +\t\treturn cacheIt->second;\n> +\n> +\t/* Make sure we don't run out of space. */\n> +\tsize_t size = sizeof(struct rkisp1_ext_params_block_header)\n> +\t\t + ((info.size + 7) & ~7);\n> +\tif (size > data_.size() - used_) {\n> +\t\tLOG(RkISP1Params, Error)\n> +\t\t\t<< \"Out of memory to allocate block type \"\n> +\t\t\t<< utils::to_underlying(type);\n> +\t\treturn {};\n> +\t}\n> +\n> +\t/* Allocate a new block, clear its memory, and initialize its header. */\n> +\tSpan block = data_.subspan(used_, size);\n> +\tused_ += size;\n> +\n> +\tstruct rkisp1_ext_params_cfg *cfg =\n> +\t\treinterpret_cast(data_.data());\n> +\tcfg->data_size += size;\n> +\n> +\tmemset(block.data(), 0, block.size());\n> +\n> +\tstruct rkisp1_ext_params_block_header *header =\n> +\t\treinterpret_cast(block.data());\n> +\theader->type = info.type;\n> +\theader->size = block.size();\n> +\n> +\t/* Update the cache. */\n> +\tblocks_[type] = block;\n> +\n> +\treturn block;\n> +}\n> +\n> +} /* namespace ipa::rkisp1 */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> new file mode 100644\n> index 000000000000..7a7bf07299b2\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/params.h\n> @@ -0,0 +1,157 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * RkISP1 ISP Parameters\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1 {\n> +\n> +enum class Block {\n> +\tBls,\n> +\tDpcc,\n> +\tSdg,\n> +\tAwbGain,\n> +\tFlt,\n> +\tBdm,\n> +\tCtk,\n> +\tGoc,\n> +\tDpf,\n> +\tDpfStrength,\n> +\tCproc,\n> +\tIe,\n> +\tLsc,\n> +\tAwb,\n> +\tHst,\n> +\tAec,\n> +\tAfc,\n> +};\n> +\n> +namespace details {\n> +\n> +template\n> +struct block_type {\n> +};\n> +\n> +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)\t\t\\\n> +template<>\t\t\t\t\t\t\t\t\\\n> +struct block_type {\t\t\t\t\t\\\n> +\tusing type = struct rkisp1_cif_isp_##blockStruct##_config;\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> +\n> +} /* namespace details */\n> +\n> +class RkISP1Params;\n> +\n> +class RkISP1ParamsBlockBase\n> +{\n> +public:\n> +\tRkISP1ParamsBlockBase(RkISP1Params *params, Block type,\n> +\t\t\t const Span &data);\n> +\n> +\tSpan data() const { return data_; }\n> +\n> +\tvoid setEnabled(bool enabled);\n\nI like that.\n\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase);\n> +\n> +\tRkISP1Params *params_;\n> +\tBlock type_;\n> +\tSpan header_;\n> +\tSpan data_;\n> +};\n> +\n> +template\n> +class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> +{\n> +public:\n> +\tusing Type = typename details::block_type::type;\n> +\n> +\tRkISP1ParamsBlock(RkISP1Params *params, const Span &data)\n> +\t\t: RkISP1ParamsBlockBase(params, B, data)\n> +\t{\n> +\t}\n> +\n> +\tconst Type *operator->() const\n> +\t{\n> +\t\treturn reinterpret_cast(data().data());\n> +\t}\n> +\n> +\tType *operator->()\n> +\t{\n> +\t\treturn reinterpret_cast(data().data());\n> +\t}\n> +\n> +\tconst Type &operator*() const &\n> +\t{\n> +\t\treturn *reinterpret_cast(data().data());\n> +\t}\n> +\n> +\tType &operator*() &\n> +\t{\n> +\t\treturn *reinterpret_cast(data().data());\n> +\t}\n> +};\n> +\n> +class RkISP1Params\n> +{\n> +public:\n> +\tRkISP1Params(uint32_t format, Span data);\n> +\n> +\ttemplate\n> +\tRkISP1ParamsBlock block()\n> +\t{\n> +\t\treturn RkISP1ParamsBlock(this, block(B));\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 block(Block type);\n> +\tvoid setBlockEnabled(Block type, bool enabled);\n> +\n> +\tuint32_t format_;\n> +\n> +\tSpan data_;\n> +\tsize_t used_;\n> +\n> +\tstd::map> blocks_;\n\nThis could become\nstd::map> blocks_;\n\n\nWhichever you prefer.\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n\n> +};\n> +\n> +} /* namespace ipa::rkisp1 */\n> +\n> +} /* namespace libcamera*/\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CD1E9BEFBE\n\tfor ;\n\tFri, 5 Jul 2024 13:24:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8BBF62E22;\n\tFri, 5 Jul 2024 15:24:16 +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 B8AA0619BF\n\tfor ;\n\tFri, 5 Jul 2024 15:24:15 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:60b6:33a3:3a20:6030])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A1CE6541;\n\tFri, 5 Jul 2024 15:23:45 +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=\"cgDB08Lo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720185825;\n\tbh=yMLplrOCQX3qin3dZOynp0gqNwFpovnsoODWZEcBPeY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cgDB08LoLdkt03jLL5qmmcNXUZFX9Ou6wjOD21ni3ub2B+yEZMPPIH3HP1w93Z3NH\n\tQ+MZckgUWEE2BedRSs80//8MS261KUAKtj1Okz8yu2dspa/WeFhTNiLhrmVhOvSKWW\n\tz+Gfk4E1sniDFjDXGYoPbdUrS6gEz8YEpQincr6I=","Date":"Fri, 5 Jul 2024 15:24:12 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ,\n\tPaul Elder ","Subject":"Re: [PATCH v2 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","Message-ID":"","References":"<20240704162035.15074-1-laurent.pinchart@ideasonboard.com>\n\t<20240704162035.15074-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240704162035.15074-6-laurent.pinchart@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30359,"web_url":"https://patchwork.libcamera.org/comment/30359/","msgid":"<20240705134436.GA6489@pendragon.ideasonboard.com>","date":"2024-07-05T13:44:36","subject":"Re: [PATCH v2 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jul 05, 2024 at 09:06:17PM +0900, Paul Elder wrote:\n> On Thu, Jul 04, 2024 at 07:20:29PM +0300, Laurent Pinchart wrote:\n> > Individual algorithms of the rkisp1 IPA module access their\n> > corresponding ISP parameters through the top-level structure\n> > rkisp1_params_cfg. This will not work anymore with the new parameters\n> > format. In order to ease the transition to the new format, abstract the\n> > ISP parameters in a new RkISP1Params class that offers the same\n> > interface regardless of the format.\n> \n> Wow that's magical (also not too bad of template magic as I expected).\n\nSometimes it's white magic :-)\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v1:\n> > \n> > - Fix module enable and update fields for legacy format\n> > - Log messages when block allocation fails\n> > - Use uint32_t type for enableBit\n> > - Reword comment explaining block caching\n> > - Fix State::Disable enumerator value\n> > - Disable copying of RkISP1ParamsBlock class\n> > - Move the params enabled state handling to a setEnabled() function\n> > ---\n> > src/ipa/rkisp1/meson.build | 1 +\n> > src/ipa/rkisp1/params.cpp | 217 +++++++++++++++++++++++++++++++++++++\n> > src/ipa/rkisp1/params.h | 157 +++++++++++++++++++++++++++\n> > 3 files changed, 375 insertions(+)\n> > create mode 100644 src/ipa/rkisp1/params.cpp\n> > create mode 100644 src/ipa/rkisp1/params.h\n> > \n> > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> > index e8b266f1ccca..65eef5d69c32 100644\n> > --- a/src/ipa/rkisp1/meson.build\n> > +++ b/src/ipa/rkisp1/meson.build\n> > @@ -7,6 +7,7 @@ ipa_name = 'ipa_rkisp1'\n> > \n> > rkisp1_ipa_sources = files([\n> > 'ipa_context.cpp',\n> > + 'params.cpp',\n> > 'rkisp1.cpp',\n> > 'utils.cpp',\n> > ])\n> > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n> > new file mode 100644\n> > index 000000000000..7155038deb18\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/params.cpp\n> > @@ -0,0 +1,217 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board\n> > + *\n> > + * RkISP1 ISP Parameters\n> > + */\n> > +\n> > +#include \"params.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(RkISP1Params)\n> > +\n> > +namespace ipa::rkisp1 {\n> > +\n> > +namespace {\n> > +\n> > +struct BlockTypeInfo {\n> > +\tenum rkisp1_ext_params_block_type type;\n> > +\tsize_t size;\n> > +\tsize_t offset;\n> > +\tuint32_t enableBit;\n> > +};\n> > +\n> > +#define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)\t\t\t\\\n> > +\t{ Block::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\toffsetof(struct rkisp1_params_cfg, category.type##_config),\t\\\n> > +\t\tRKISP1_CIF_ISP_MODULE_##bit,\t\t\t\t\t\\\n> > +\t} }\n> > +\n> > +#define RKISP1_BLOCK_TYPE_ENTRY_MEAS(block, id, type)\t\t\t\t\\\n> > +\tRKISP1_BLOCK_TYPE_ENTRY(block, id##_MEAS, type, meas, id)\n> > +\n> > +#define RKISP1_BLOCK_TYPE_ENTRY_OTHERS(block, id, type)\t\t\t\t\\\n> > +\tRKISP1_BLOCK_TYPE_ENTRY(block, id, type, others, id)\n> > +\n> > +#define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)\t\t\t\t\\\n> > +\t{ Block::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\t0, 0,\t\t\t\t\t\t\t\t\\\n> > +\t} }\n> > +\n> > +const std::map kBlockTypeInfo = {\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bls, BLS, bls),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpcc, DPCC, dpcc),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Sdg, SDG, sdg),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(AwbGain, AWB_GAIN, awb_gain),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Flt, FLT, flt),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bdm, BDM, bdm),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ctk, CTK, ctk),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Goc, GOC, goc),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpf, DPF, dpf),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(DpfStrength, DPF_STRENGTH, dpf_strength),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Cproc, CPROC, cproc),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ie, IE, ie),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Lsc, LSC, lsc),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Awb, AWB, awb_meas),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Hst, HST, hst),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Aec, AEC, aec),\n> > +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Afc, AFC, afc),\n> > +};\n> > +\n> > +} /* namespace */\n> > +\n> > +RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, Block type,\n> > +\t\t\t\t\t const Span &data)\n> > +\t: params_(params), type_(type)\n> > +{\n> > +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > +\t\theader_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n> > +\t\tdata_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n> > +\t} else {\n> > +\t\tdata_ = data;\n> > +\t}\n> > +}\n> > +\n> > +void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n> > +{\n> > +\t/*\n> > +\t * For the legacy fixed format, blocks are enabled in the top-level\n> > +\t * header. Delegate to the RkISP1Params class.\n> > +\t */\n> > +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> > +\t\treturn params_->setBlockEnabled(type_, enabled);\n> > +\n> > +\t/*\n> > +\t * For the extensible format, set the enable field in the block header\n> > +\t * directly.\n> > +\t */\n> > +\tstruct rkisp1_ext_params_block_header *header =\n> > +\t\treinterpret_cast(header_.data());\n> > +\theader->enable = enabled ? RKISP1_EXT_PARAMS_BLOCK_ENABLE\n> > +\t\t : RKISP1_EXT_PARAMS_BLOCK_DISABLE;\n> > +}\n> > +\n> > +RkISP1Params::RkISP1Params(uint32_t format, Span data)\n> > +\t: format_(format), data_(data), used_(0)\n> > +{\n> > +\tif (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > +\t\tstruct rkisp1_ext_params_cfg *cfg =\n> > +\t\t\treinterpret_cast(data.data());\n> > +\n> > +\t\tcfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n> > +\t\tcfg->data_size = 0;\n> > +\n> > +\t\tused_ += offsetof(struct rkisp1_ext_params_cfg, data);\n> > +\t} else {\n> > +\t\tmemset(data.data(), 0, data.size());\n> > +\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> > +\t}\n> > +}\n> > +\n> > +void RkISP1Params::setBlockEnabled(Block type, bool enabled)\n> > +{\n> > +\tconst BlockTypeInfo &info = kBlockTypeInfo.at(type);\n> > +\n> > +\tstruct rkisp1_params_cfg *cfg =\n> > +\t\treinterpret_cast(data_.data());\n> > +\tif (enabled)\n> > +\t\tcfg->module_ens |= info.enableBit;\n> > +\telse\n> > +\t\tcfg->module_ens &= ~info.enableBit;\n> > +}\n> > +\n> > +Span RkISP1Params::block(Block type)\n> > +{\n> > +\tauto infoIt = kBlockTypeInfo.find(type);\n> > +\tif (infoIt == kBlockTypeInfo.end()) {\n> > +\t\tLOG(RkISP1Params, Error)\n> > +\t\t\t<< \"Invalid parameters block type \"\n> > +\t\t\t<< utils::to_underlying(type);\n> > +\t\treturn {};\n> > +\t}\n> > +\n> > +\tconst BlockTypeInfo &info = infoIt->second;\n> > +\n> > +\t/*\n> > +\t * For the legacy format, return a block referencing the fixed location\n> > +\t * of the data.\n> > +\t */\n> > +\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n> > +\t\t/*\n> > +\t\t * Blocks available in extended parameters only have an offset\n> \n> s/extended/fixed/ ?\n\nNo, I meant extended here. The blocks that are only available with the\nextended API (e.g. companding) have an offset of 0, they're detected\nhere and we return an error as the fixed format doesn't support them.\n\n> Reviewed-by: Paul Elder \n> \n> > +\t\t * of 0. Return nullptr in that case.\n> > +\t\t */\n> > +\t\tif (info.offset == 0) {\n> > +\t\t\tLOG(RkISP1Params, Error)\n> > +\t\t\t\t<< \"Block type \" << utils::to_underlying(type)\n> > +\t\t\t\t<< \" unavailable in fixed parameters format\";\n> > +\t\t\treturn {};\n> > +\t\t}\n> > +\n> > +\t\tstruct rkisp1_params_cfg *cfg =\n> > +\t\t\treinterpret_cast(data_.data());\n> > +\n> > +\t\tcfg->module_cfg_update |= info.enableBit;\n> > +\t\tcfg->module_en_update |= info.enableBit;\n> > +\n> > +\t\treturn data_.subspan(info.offset, info.size);\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * For the extensible format, allocate memory for the block, including\n> > +\t * the header. Look up the block in the cache first. If an algorithm\n> > +\t * requests the same block type twice, it should get the same block.\n> > +\t */\n> > +\tauto cacheIt = blocks_.find(type);\n> > +\tif (cacheIt != blocks_.end())\n> > +\t\treturn cacheIt->second;\n> > +\n> > +\t/* Make sure we don't run out of space. */\n> > +\tsize_t size = sizeof(struct rkisp1_ext_params_block_header)\n> > +\t\t + ((info.size + 7) & ~7);\n> > +\tif (size > data_.size() - used_) {\n> > +\t\tLOG(RkISP1Params, Error)\n> > +\t\t\t<< \"Out of memory to allocate block type \"\n> > +\t\t\t<< utils::to_underlying(type);\n> > +\t\treturn {};\n> > +\t}\n> > +\n> > +\t/* Allocate a new block, clear its memory, and initialize its header. */\n> > +\tSpan block = data_.subspan(used_, size);\n> > +\tused_ += size;\n> > +\n> > +\tstruct rkisp1_ext_params_cfg *cfg =\n> > +\t\treinterpret_cast(data_.data());\n> > +\tcfg->data_size += size;\n> > +\n> > +\tmemset(block.data(), 0, block.size());\n> > +\n> > +\tstruct rkisp1_ext_params_block_header *header =\n> > +\t\treinterpret_cast(block.data());\n> > +\theader->type = info.type;\n> > +\theader->size = block.size();\n> > +\n> > +\t/* Update the cache. */\n> > +\tblocks_[type] = block;\n> > +\n> > +\treturn block;\n> > +}\n> > +\n> > +} /* namespace ipa::rkisp1 */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> > new file mode 100644\n> > index 000000000000..7a7bf07299b2\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/params.h\n> > @@ -0,0 +1,157 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board\n> > + *\n> > + * RkISP1 ISP Parameters\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1 {\n> > +\n> > +enum class Block {\n> > +\tBls,\n> > +\tDpcc,\n> > +\tSdg,\n> > +\tAwbGain,\n> > +\tFlt,\n> > +\tBdm,\n> > +\tCtk,\n> > +\tGoc,\n> > +\tDpf,\n> > +\tDpfStrength,\n> > +\tCproc,\n> > +\tIe,\n> > +\tLsc,\n> > +\tAwb,\n> > +\tHst,\n> > +\tAec,\n> > +\tAfc,\n> > +};\n> > +\n> > +namespace details {\n> > +\n> > +template\n> > +struct block_type {\n> > +};\n> > +\n> > +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)\t\t\\\n> > +template<>\t\t\t\t\t\t\t\t\\\n> > +struct block_type {\t\t\t\t\t\\\n> > +\tusing type = struct rkisp1_cif_isp_##blockStruct##_config;\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> > +\n> > +} /* namespace details */\n> > +\n> > +class RkISP1Params;\n> > +\n> > +class RkISP1ParamsBlockBase\n> > +{\n> > +public:\n> > +\tRkISP1ParamsBlockBase(RkISP1Params *params, Block type,\n> > +\t\t\t const Span &data);\n> > +\n> > +\tSpan data() const { return data_; }\n> > +\n> > +\tvoid setEnabled(bool enabled);\n> > +\n> > +private:\n> > +\tLIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase);\n> > +\n> > +\tRkISP1Params *params_;\n> > +\tBlock type_;\n> > +\tSpan header_;\n> > +\tSpan data_;\n> > +};\n> > +\n> > +template\n> > +class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> > +{\n> > +public:\n> > +\tusing Type = typename details::block_type::type;\n> > +\n> > +\tRkISP1ParamsBlock(RkISP1Params *params, const Span &data)\n> > +\t\t: RkISP1ParamsBlockBase(params, B, data)\n> > +\t{\n> > +\t}\n> > +\n> > +\tconst Type *operator->() const\n> > +\t{\n> > +\t\treturn reinterpret_cast(data().data());\n> > +\t}\n> > +\n> > +\tType *operator->()\n> > +\t{\n> > +\t\treturn reinterpret_cast(data().data());\n> > +\t}\n> > +\n> > +\tconst Type &operator*() const &\n> > +\t{\n> > +\t\treturn *reinterpret_cast(data().data());\n> > +\t}\n> > +\n> > +\tType &operator*() &\n> > +\t{\n> > +\t\treturn *reinterpret_cast(data().data());\n> > +\t}\n> > +};\n> > +\n> > +class RkISP1Params\n> > +{\n> > +public:\n> > +\tRkISP1Params(uint32_t format, Span data);\n> > +\n> > +\ttemplate\n> > +\tRkISP1ParamsBlock block()\n> > +\t{\n> > +\t\treturn RkISP1ParamsBlock(this, block(B));\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 block(Block type);\n> > +\tvoid setBlockEnabled(Block type, bool enabled);\n> > +\n> > +\tuint32_t format_;\n> > +\n> > +\tSpan data_;\n> > +\tsize_t used_;\n> > +\n> > +\tstd::map> blocks_;\n> > +};\n> > +\n> > +} /* namespace ipa::rkisp1 */\n> > +\n> > +} /* namespace libcamera*/","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 14C06BD87C\n\tfor ;\n\tFri, 5 Jul 2024 13:45:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC4C862E23;\n\tFri, 5 Jul 2024 15:45:00 +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 D6CBB619BF\n\tfor ;\n\tFri, 5 Jul 2024 15:44:58 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1BA69541;\n\tFri, 5 Jul 2024 15:44:29 +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=\"LNeMFYoH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720187069;\n\tbh=uqJ0GxIN26thiByxF4HYkmiY2Qplq3ASi1iGODwi2Kw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LNeMFYoH3Pz6HCrCVpOiMcDHVmcbuTt/dVUDxzZ8H7pRo+olRGSrqHcQf5vYHcxeL\n\t+2NhGooVLfSUp31djMiMaJNa1TV6a2SjN/+WNHmufCB/S4/t7UdylEYoS1RjUe9J90\n\tfmbi561/AJY5Y5dJtFvOR1IgHckFetilEh36g9Wg=","Date":"Fri, 5 Jul 2024 16:44:36 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","Subject":"Re: [PATCH v2 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","Message-ID":"<20240705134436.GA6489@pendragon.ideasonboard.com>","References":"<20240704162035.15074-1-laurent.pinchart@ideasonboard.com>\n\t<20240704162035.15074-6-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30384,"web_url":"https://patchwork.libcamera.org/comment/30384/","msgid":"","date":"2024-07-11T04:35:34","subject":"Re: [PATCH v2 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Fri, Jul 05, 2024 at 04:44:36PM +0300, Laurent Pinchart wrote:\n> On Fri, Jul 05, 2024 at 09:06:17PM +0900, Paul Elder wrote:\n> > On Thu, Jul 04, 2024 at 07:20:29PM +0300, Laurent Pinchart wrote:\n> > > Individual algorithms of the rkisp1 IPA module access their\n> > > corresponding ISP parameters through the top-level structure\n> > > rkisp1_params_cfg. This will not work anymore with the new parameters\n> > > format. In order to ease the transition to the new format, abstract the\n> > > ISP parameters in a new RkISP1Params class that offers the same\n> > > interface regardless of the format.\n> > \n> > Wow that's magical (also not too bad of template magic as I expected).\n> \n> Sometimes it's white magic :-)\n> \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > Changes since v1:\n> > > \n> > > - Fix module enable and update fields for legacy format\n> > > - Log messages when block allocation fails\n> > > - Use uint32_t type for enableBit\n> > > - Reword comment explaining block caching\n> > > - Fix State::Disable enumerator value\n> > > - Disable copying of RkISP1ParamsBlock class\n> > > - Move the params enabled state handling to a setEnabled() function\n> > > ---\n> > > src/ipa/rkisp1/meson.build | 1 +\n> > > src/ipa/rkisp1/params.cpp | 217 +++++++++++++++++++++++++++++++++++++\n> > > src/ipa/rkisp1/params.h | 157 +++++++++++++++++++++++++++\n> > > 3 files changed, 375 insertions(+)\n> > > create mode 100644 src/ipa/rkisp1/params.cpp\n> > > create mode 100644 src/ipa/rkisp1/params.h\n> > > \n> > > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> > > index e8b266f1ccca..65eef5d69c32 100644\n> > > --- a/src/ipa/rkisp1/meson.build\n> > > +++ b/src/ipa/rkisp1/meson.build\n> > > @@ -7,6 +7,7 @@ ipa_name = 'ipa_rkisp1'\n> > > \n> > > rkisp1_ipa_sources = files([\n> > > 'ipa_context.cpp',\n> > > + 'params.cpp',\n> > > 'rkisp1.cpp',\n> > > 'utils.cpp',\n> > > ])\n> > > diff --git a/src/ipa/rkisp1/params.cpp b/src/ipa/rkisp1/params.cpp\n> > > new file mode 100644\n> > > index 000000000000..7155038deb18\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/params.cpp\n> > > @@ -0,0 +1,217 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas On Board\n> > > + *\n> > > + * RkISP1 ISP Parameters\n> > > + */\n> > > +\n> > > +#include \"params.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(RkISP1Params)\n> > > +\n> > > +namespace ipa::rkisp1 {\n> > > +\n> > > +namespace {\n> > > +\n> > > +struct BlockTypeInfo {\n> > > +\tenum rkisp1_ext_params_block_type type;\n> > > +\tsize_t size;\n> > > +\tsize_t offset;\n> > > +\tuint32_t enableBit;\n> > > +};\n> > > +\n> > > +#define RKISP1_BLOCK_TYPE_ENTRY(block, id, type, category, bit)\t\t\t\\\n> > > +\t{ Block::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\toffsetof(struct rkisp1_params_cfg, category.type##_config),\t\\\n> > > +\t\tRKISP1_CIF_ISP_MODULE_##bit,\t\t\t\t\t\\\n> > > +\t} }\n> > > +\n> > > +#define RKISP1_BLOCK_TYPE_ENTRY_MEAS(block, id, type)\t\t\t\t\\\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY(block, id##_MEAS, type, meas, id)\n> > > +\n> > > +#define RKISP1_BLOCK_TYPE_ENTRY_OTHERS(block, id, type)\t\t\t\t\\\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY(block, id, type, others, id)\n> > > +\n> > > +#define RKISP1_BLOCK_TYPE_ENTRY_EXT(block, id, type)\t\t\t\t\\\n> > > +\t{ Block::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\t0, 0,\t\t\t\t\t\t\t\t\\\n> > > +\t} }\n> > > +\n> > > +const std::map kBlockTypeInfo = {\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bls, BLS, bls),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpcc, DPCC, dpcc),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Sdg, SDG, sdg),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(AwbGain, AWB_GAIN, awb_gain),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Flt, FLT, flt),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Bdm, BDM, bdm),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ctk, CTK, ctk),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Goc, GOC, goc),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Dpf, DPF, dpf),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(DpfStrength, DPF_STRENGTH, dpf_strength),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Cproc, CPROC, cproc),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Ie, IE, ie),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_OTHERS(Lsc, LSC, lsc),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Awb, AWB, awb_meas),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Hst, HST, hst),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Aec, AEC, aec),\n> > > +\tRKISP1_BLOCK_TYPE_ENTRY_MEAS(Afc, AFC, afc),\n> > > +};\n> > > +\n> > > +} /* namespace */\n> > > +\n> > > +RkISP1ParamsBlockBase::RkISP1ParamsBlockBase(RkISP1Params *params, Block type,\n> > > +\t\t\t\t\t const Span &data)\n> > > +\t: params_(params), type_(type)\n> > > +{\n> > > +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > > +\t\theader_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n> > > +\t\tdata_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n> > > +\t} else {\n> > > +\t\tdata_ = data;\n> > > +\t}\n> > > +}\n> > > +\n> > > +void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n> > > +{\n> > > +\t/*\n> > > +\t * For the legacy fixed format, blocks are enabled in the top-level\n> > > +\t * header. Delegate to the RkISP1Params class.\n> > > +\t */\n> > > +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> > > +\t\treturn params_->setBlockEnabled(type_, enabled);\n> > > +\n> > > +\t/*\n> > > +\t * For the extensible format, set the enable field in the block header\n> > > +\t * directly.\n> > > +\t */\n> > > +\tstruct rkisp1_ext_params_block_header *header =\n> > > +\t\treinterpret_cast(header_.data());\n> > > +\theader->enable = enabled ? RKISP1_EXT_PARAMS_BLOCK_ENABLE\n> > > +\t\t : RKISP1_EXT_PARAMS_BLOCK_DISABLE;\n> > > +}\n> > > +\n> > > +RkISP1Params::RkISP1Params(uint32_t format, Span data)\n> > > +\t: format_(format), data_(data), used_(0)\n> > > +{\n> > > +\tif (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > > +\t\tstruct rkisp1_ext_params_cfg *cfg =\n> > > +\t\t\treinterpret_cast(data.data());\n> > > +\n> > > +\t\tcfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n> > > +\t\tcfg->data_size = 0;\n> > > +\n> > > +\t\tused_ += offsetof(struct rkisp1_ext_params_cfg, data);\n> > > +\t} else {\n> > > +\t\tmemset(data.data(), 0, data.size());\n> > > +\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> > > +\t}\n> > > +}\n> > > +\n> > > +void RkISP1Params::setBlockEnabled(Block type, bool enabled)\n> > > +{\n> > > +\tconst BlockTypeInfo &info = kBlockTypeInfo.at(type);\n> > > +\n> > > +\tstruct rkisp1_params_cfg *cfg =\n> > > +\t\treinterpret_cast(data_.data());\n> > > +\tif (enabled)\n> > > +\t\tcfg->module_ens |= info.enableBit;\n> > > +\telse\n> > > +\t\tcfg->module_ens &= ~info.enableBit;\n> > > +}\n> > > +\n> > > +Span RkISP1Params::block(Block type)\n> > > +{\n> > > +\tauto infoIt = kBlockTypeInfo.find(type);\n> > > +\tif (infoIt == kBlockTypeInfo.end()) {\n> > > +\t\tLOG(RkISP1Params, Error)\n> > > +\t\t\t<< \"Invalid parameters block type \"\n> > > +\t\t\t<< utils::to_underlying(type);\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\tconst BlockTypeInfo &info = infoIt->second;\n> > > +\n> > > +\t/*\n> > > +\t * For the legacy format, return a block referencing the fixed location\n> > > +\t * of the data.\n> > > +\t */\n> > > +\tif (format_ == V4L2_META_FMT_RK_ISP1_PARAMS) {\n> > > +\t\t/*\n> > > +\t\t * Blocks available in extended parameters only have an offset\n> > \n> > s/extended/fixed/ ?\n> \n> No, I meant extended here. The blocks that are only available with the\n> extended API (e.g. companding) have an offset of 0, they're detected\n> here and we return an error as the fixed format doesn't support them.\n\nAh, in that case it should be \"Blocks available only in extended parameters\nhave...\"\n\n\nPaul\n\n> \n> > Reviewed-by: Paul Elder \n> > \n> > > +\t\t * of 0. Return nullptr in that case.\n> > > +\t\t */\n> > > +\t\tif (info.offset == 0) {\n> > > +\t\t\tLOG(RkISP1Params, Error)\n> > > +\t\t\t\t<< \"Block type \" << utils::to_underlying(type)\n> > > +\t\t\t\t<< \" unavailable in fixed parameters format\";\n> > > +\t\t\treturn {};\n> > > +\t\t}\n> > > +\n> > > +\t\tstruct rkisp1_params_cfg *cfg =\n> > > +\t\t\treinterpret_cast(data_.data());\n> > > +\n> > > +\t\tcfg->module_cfg_update |= info.enableBit;\n> > > +\t\tcfg->module_en_update |= info.enableBit;\n> > > +\n> > > +\t\treturn data_.subspan(info.offset, info.size);\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * For the extensible format, allocate memory for the block, including\n> > > +\t * the header. Look up the block in the cache first. If an algorithm\n> > > +\t * requests the same block type twice, it should get the same block.\n> > > +\t */\n> > > +\tauto cacheIt = blocks_.find(type);\n> > > +\tif (cacheIt != blocks_.end())\n> > > +\t\treturn cacheIt->second;\n> > > +\n> > > +\t/* Make sure we don't run out of space. */\n> > > +\tsize_t size = sizeof(struct rkisp1_ext_params_block_header)\n> > > +\t\t + ((info.size + 7) & ~7);\n> > > +\tif (size > data_.size() - used_) {\n> > > +\t\tLOG(RkISP1Params, Error)\n> > > +\t\t\t<< \"Out of memory to allocate block type \"\n> > > +\t\t\t<< utils::to_underlying(type);\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\t/* Allocate a new block, clear its memory, and initialize its header. */\n> > > +\tSpan block = data_.subspan(used_, size);\n> > > +\tused_ += size;\n> > > +\n> > > +\tstruct rkisp1_ext_params_cfg *cfg =\n> > > +\t\treinterpret_cast(data_.data());\n> > > +\tcfg->data_size += size;\n> > > +\n> > > +\tmemset(block.data(), 0, block.size());\n> > > +\n> > > +\tstruct rkisp1_ext_params_block_header *header =\n> > > +\t\treinterpret_cast(block.data());\n> > > +\theader->type = info.type;\n> > > +\theader->size = block.size();\n> > > +\n> > > +\t/* Update the cache. */\n> > > +\tblocks_[type] = block;\n> > > +\n> > > +\treturn block;\n> > > +}\n> > > +\n> > > +} /* namespace ipa::rkisp1 */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/rkisp1/params.h b/src/ipa/rkisp1/params.h\n> > > new file mode 100644\n> > > index 000000000000..7a7bf07299b2\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/params.h\n> > > @@ -0,0 +1,157 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas On Board\n> > > + *\n> > > + * RkISP1 ISP Parameters\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1 {\n> > > +\n> > > +enum class Block {\n> > > +\tBls,\n> > > +\tDpcc,\n> > > +\tSdg,\n> > > +\tAwbGain,\n> > > +\tFlt,\n> > > +\tBdm,\n> > > +\tCtk,\n> > > +\tGoc,\n> > > +\tDpf,\n> > > +\tDpfStrength,\n> > > +\tCproc,\n> > > +\tIe,\n> > > +\tLsc,\n> > > +\tAwb,\n> > > +\tHst,\n> > > +\tAec,\n> > > +\tAfc,\n> > > +};\n> > > +\n> > > +namespace details {\n> > > +\n> > > +template\n> > > +struct block_type {\n> > > +};\n> > > +\n> > > +#define RKISP1_DEFINE_BLOCK_TYPE(blockType, blockStruct)\t\t\\\n> > > +template<>\t\t\t\t\t\t\t\t\\\n> > > +struct block_type {\t\t\t\t\t\\\n> > > +\tusing type = struct rkisp1_cif_isp_##blockStruct##_config;\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> > > +\n> > > +} /* namespace details */\n> > > +\n> > > +class RkISP1Params;\n> > > +\n> > > +class RkISP1ParamsBlockBase\n> > > +{\n> > > +public:\n> > > +\tRkISP1ParamsBlockBase(RkISP1Params *params, Block type,\n> > > +\t\t\t const Span &data);\n> > > +\n> > > +\tSpan data() const { return data_; }\n> > > +\n> > > +\tvoid setEnabled(bool enabled);\n> > > +\n> > > +private:\n> > > +\tLIBCAMERA_DISABLE_COPY(RkISP1ParamsBlockBase);\n> > > +\n> > > +\tRkISP1Params *params_;\n> > > +\tBlock type_;\n> > > +\tSpan header_;\n> > > +\tSpan data_;\n> > > +};\n> > > +\n> > > +template\n> > > +class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> > > +{\n> > > +public:\n> > > +\tusing Type = typename details::block_type::type;\n> > > +\n> > > +\tRkISP1ParamsBlock(RkISP1Params *params, const Span &data)\n> > > +\t\t: RkISP1ParamsBlockBase(params, B, data)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\tconst Type *operator->() const\n> > > +\t{\n> > > +\t\treturn reinterpret_cast(data().data());\n> > > +\t}\n> > > +\n> > > +\tType *operator->()\n> > > +\t{\n> > > +\t\treturn reinterpret_cast(data().data());\n> > > +\t}\n> > > +\n> > > +\tconst Type &operator*() const &\n> > > +\t{\n> > > +\t\treturn *reinterpret_cast(data().data());\n> > > +\t}\n> > > +\n> > > +\tType &operator*() &\n> > > +\t{\n> > > +\t\treturn *reinterpret_cast(data().data());\n> > > +\t}\n> > > +};\n> > > +\n> > > +class RkISP1Params\n> > > +{\n> > > +public:\n> > > +\tRkISP1Params(uint32_t format, Span data);\n> > > +\n> > > +\ttemplate\n> > > +\tRkISP1ParamsBlock block()\n> > > +\t{\n> > > +\t\treturn RkISP1ParamsBlock(this, block(B));\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 block(Block type);\n> > > +\tvoid setBlockEnabled(Block type, bool enabled);\n> > > +\n> > > +\tuint32_t format_;\n> > > +\n> > > +\tSpan data_;\n> > > +\tsize_t used_;\n> > > +\n> > > +\tstd::map> blocks_;\n> > > +};\n> > > +\n> > > +} /* namespace ipa::rkisp1 */\n> > > +\n> > > +} /* namespace libcamera*/","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1B371BDB1C\n\tfor ;\n\tThu, 11 Jul 2024 04:35:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C35186336F;\n\tThu, 11 Jul 2024 06:35:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D20A619AB\n\tfor ;\n\tThu, 11 Jul 2024 06:35:43 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6DA1B8D0;\n\tThu, 11 Jul 2024 06:35:08 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PSnY3GJn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720672509;\n\tbh=hIHD/klHMqUgkqJaQvp9vYrS9qNFXPN488Lvv0JQ7Rw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PSnY3GJna6j8P6d17KtGZ0C3lRNxoPPxz5eE3cbeVBbkcbb9HqL3iaTGN3RgF4WU0\n\tbS/x19xMVucAUaBmVvIx+ybe8QhB6trOG9m3QL6WWY+OmlzsUx1JdspsalkH8VTbQt\n\tAIAd8vgu10BAiC4q2t77teHOpcGqLEaUbmZH5xxg=","Date":"Thu, 11 Jul 2024 13:35:34 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","Subject":"Re: [PATCH v2 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","Message-ID":"","References":"<20240704162035.15074-1-laurent.pinchart@ideasonboard.com>\n\t<20240704162035.15074-6-laurent.pinchart@ideasonboard.com>\n\t\n\t<20240705134436.GA6489@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240705134436.GA6489@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]