[{"id":30907,"web_url":"https://patchwork.libcamera.org/comment/30907/","msgid":"<46kishp2tsijvu37bpcjuelfrdhorrkf5orqbkco4xooddook2@xi6f4tudankc>","date":"2024-08-27T06:53:44","subject":"Re: [PATCH v4 4/9] ipa: rkisp1: Add ISP parameters abstraction class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\nOn Tue, Aug 27, 2024 at 04:40:38AM GMT, 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> Reviewed-by: Paul Elder \n> Reviewed-by: Stefan Klug \n> ---\n> Changes since v3:\n>\n> - Update to latest kernel API\n>\n\nI don't get it, what kernel API was the previous version based on ?\n\nI tested against your v6.11/merge branch which if I'm not mistaken has\nmedia-stage merged in.\n\n\n> Changes since v2:\n>\n> - Rename Block to BlockType\n> - Fix grammer in comment\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 | 219 +++++++++++++++++++++++++++++++++++++\n> src/ipa/rkisp1/params.h | 157 ++++++++++++++++++++++++++\n> 3 files changed, 377 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 160ef52dd52e..34844f1498f9 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..6bc6f89919fb\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/params.cpp\n> @@ -0,0 +1,219 @@\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{ BlockType::block, {\t\t\t\t\t\t\t\\\n> +\t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> +\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> +\t\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{ BlockType::block, {\t\t\t\t\t\t\t\\\n> +\t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> +\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> +\t\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, BlockType type,\n> +\t\t\t\t\t const Span &data)\n> +\t: params_(params), type_(type)\n> +{\n> +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> +\t\theader_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n> +\t\tdata_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n> +\t} else {\n> +\t\tdata_ = data;\n> +\t}\n> +}\n> +\n> +void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n> +{\n> +\t/*\n> +\t * For the legacy fixed format, blocks are enabled in the top-level\n> +\t * header. Delegate to the RkISP1Params class.\n> +\t */\n> +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> +\t\treturn params_->setBlockEnabled(type_, enabled);\n> +\n> +\t/*\n> +\t * For the extensible format, set the enable and disable flags in the\n> +\t * block header directly.\n> +\t */\n> +\tstruct rkisp1_ext_params_block_header *header =\n> +\t\treinterpret_cast(header_.data());\n> +\theader->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |\n> +\t\t\t RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE);\n> +\theader->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE\n> +\t\t\t\t : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE;\n> +}\n> +\n> +RkISP1Params::RkISP1Params(uint32_t format, Span data)\n> +\t: format_(format), data_(data), used_(0)\n> +{\n> +\tif (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> +\t\tstruct rkisp1_ext_params_cfg *cfg =\n> +\t\t\treinterpret_cast(data.data());\n> +\n> +\t\tcfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n> +\t\tcfg->data_size = 0;\n> +\n> +\t\tused_ += offsetof(struct rkisp1_ext_params_cfg, data);\n> +\t} else {\n> +\t\tmemset(data.data(), 0, data.size());\n> +\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> +\t}\n> +}\n> +\n> +void RkISP1Params::setBlockEnabled(BlockType type, bool enabled)\n> +{\n> +\tconst BlockTypeInfo &info = kBlockTypeInfo.at(type);\n> +\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(BlockType 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 only in extended parameters 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..28a781bc447c\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 BlockType {\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\\\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, BlockType 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> +\tBlockType type_;\n> +\tSpan header_;\n> +\tSpan data_;\n> +};\n> +\n> +template\n> +class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> +{\n> +public:\n> +\tusing Type = typename details::block_type::type;\n> +\n> +\tRkISP1ParamsBlock(RkISP1Params *params, const Span &data)\n> +\t\t: RkISP1ParamsBlockBase(params, B, data)\n> +\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(BlockType type);\n> +\tvoid setBlockEnabled(BlockType 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*/\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 D76A3C323E\n\tfor ;\n\tTue, 27 Aug 2024 06:53:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB44E6342D;\n\tTue, 27 Aug 2024 08:53:50 +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 7D59861E4F\n\tfor ;\n\tTue, 27 Aug 2024 08:53:48 +0200 (CEST)","from ideasonboard.com (mob-5-90-141-165.net.vodafone.it\n\t[5.90.141.165])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D97D3D5;\n\tTue, 27 Aug 2024 08:52:41 +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=\"cvd2nj9p\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724741561;\n\tbh=VAuobCCtJo2d9oNMVZPRcv4y9BNR1esQ2YZLWGbA59U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cvd2nj9pU5WBEuu7pIkZjtxiDHPWVHJoHrs7QY0tNYsOU6bK4JF124nB484J3r319\n\tyBhnLodH8sli8yK8tY/LniyNELDneSnxB66rn97huOPeH1/4S19aIo+Zmo9Ev548OU\n\tAgevby0A4GU+Sy6j4VwYqq71UDMScAjmpWqTE4qQ=","Date":"Tue, 27 Aug 2024 08:53:44 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 4/9] ipa: rkisp1: Add ISP parameters abstraction class","Message-ID":"<46kishp2tsijvu37bpcjuelfrdhorrkf5orqbkco4xooddook2@xi6f4tudankc>","References":"<20240827014044.24673-1-laurent.pinchart@ideasonboard.com>\n\t<20240827014044.24673-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240827014044.24673-5-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":30910,"web_url":"https://patchwork.libcamera.org/comment/30910/","msgid":"<20240827074339.GC23129@pendragon.ideasonboard.com>","date":"2024-08-27T07:43:39","subject":"Re: [PATCH v4 4/9] ipa: rkisp1: Add ISP parameters abstraction class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Aug 27, 2024 at 08:53:44AM +0200, Jacopo Mondi wrote:\n> Hi Laurent\n> On Tue, Aug 27, 2024 at 04:40:38AM GMT, 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> > Reviewed-by: Paul Elder \n> > Reviewed-by: Stefan Klug \n> > ---\n> > Changes since v3:\n> >\n> > - Update to latest kernel API\n> >\n> \n> I don't get it, what kernel API was the previous version based on ?\n\nThis should have been in the \"changes since v2\" section, sorry. I'm\nreferring to the usage of the .flags field, replacing .enabled.\n\n> I tested against your v6.11/merge branch which if I'm not mistaken has\n> media-stage merged in.\n> \n> > Changes since v2:\n> >\n> > - Rename Block to BlockType\n> > - Fix grammer in comment\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 | 219 +++++++++++++++++++++++++++++++++++++\n> > src/ipa/rkisp1/params.h | 157 ++++++++++++++++++++++++++\n> > 3 files changed, 377 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 160ef52dd52e..34844f1498f9 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..6bc6f89919fb\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/params.cpp\n> > @@ -0,0 +1,219 @@\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{ BlockType::block, {\t\t\t\t\t\t\t\\\n> > +\t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> > +\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> > +\t\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{ BlockType::block, {\t\t\t\t\t\t\t\\\n> > +\t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> > +\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> > +\t\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, BlockType type,\n> > +\t\t\t\t\t const Span &data)\n> > +\t: params_(params), type_(type)\n> > +{\n> > +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > +\t\theader_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n> > +\t\tdata_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n> > +\t} else {\n> > +\t\tdata_ = data;\n> > +\t}\n> > +}\n> > +\n> > +void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n> > +{\n> > +\t/*\n> > +\t * For the legacy fixed format, blocks are enabled in the top-level\n> > +\t * header. Delegate to the RkISP1Params class.\n> > +\t */\n> > +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> > +\t\treturn params_->setBlockEnabled(type_, enabled);\n> > +\n> > +\t/*\n> > +\t * For the extensible format, set the enable and disable flags in the\n> > +\t * block header directly.\n> > +\t */\n> > +\tstruct rkisp1_ext_params_block_header *header =\n> > +\t\treinterpret_cast(header_.data());\n> > +\theader->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |\n> > +\t\t\t RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE);\n> > +\theader->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE\n> > +\t\t\t\t : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE;\n> > +}\n> > +\n> > +RkISP1Params::RkISP1Params(uint32_t format, Span data)\n> > +\t: format_(format), data_(data), used_(0)\n> > +{\n> > +\tif (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > +\t\tstruct rkisp1_ext_params_cfg *cfg =\n> > +\t\t\treinterpret_cast(data.data());\n> > +\n> > +\t\tcfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n> > +\t\tcfg->data_size = 0;\n> > +\n> > +\t\tused_ += offsetof(struct rkisp1_ext_params_cfg, data);\n> > +\t} else {\n> > +\t\tmemset(data.data(), 0, data.size());\n> > +\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> > +\t}\n> > +}\n> > +\n> > +void RkISP1Params::setBlockEnabled(BlockType type, bool enabled)\n> > +{\n> > +\tconst BlockTypeInfo &info = kBlockTypeInfo.at(type);\n> > +\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(BlockType 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 only in extended parameters 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..28a781bc447c\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 BlockType {\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\\\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, BlockType 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> > +\tBlockType type_;\n> > +\tSpan header_;\n> > +\tSpan data_;\n> > +};\n> > +\n> > +template\n> > +class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> > +{\n> > +public:\n> > +\tusing Type = typename details::block_type::type;\n> > +\n> > +\tRkISP1ParamsBlock(RkISP1Params *params, const Span &data)\n> > +\t\t: RkISP1ParamsBlockBase(params, B, data)\n> > +\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(BlockType type);\n> > +\tvoid setBlockEnabled(BlockType 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 DF2D7C323E\n\tfor ;\n\tTue, 27 Aug 2024 07:43:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8E3DD63445;\n\tTue, 27 Aug 2024 09:43:44 +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 C851963441\n\tfor ;\n\tTue, 27 Aug 2024 09:43:42 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E62AF9FF;\n\tTue, 27 Aug 2024 09:42:35 +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=\"Wa9uw1uI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724744556;\n\tbh=yoYwaWQ/GK5uNnkj4Mt85gS9zoVz5nqrC5Ezqo99LTg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Wa9uw1uIn8Fvb70UQ8b/iMJwSXZRFvZUUz67pDx+l/lrDpQvDWqT7TJ7MFy+JKgFU\n\tCKcbM2DoRbMC/i7n3V5ub8HdmvlQ0ykUmABN/n0lgx2t5CGLpIl5FG0peocOWQsLk3\n\tVceNVOWUgoGSTZTL/n4o4aRXwa8GWvL66Oqc6+Vk=","Date":"Tue, 27 Aug 2024 10:43:39 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 4/9] ipa: rkisp1: Add ISP parameters abstraction class","Message-ID":"<20240827074339.GC23129@pendragon.ideasonboard.com>","References":"<20240827014044.24673-1-laurent.pinchart@ideasonboard.com>\n\t<20240827014044.24673-5-laurent.pinchart@ideasonboard.com>\n\t<46kishp2tsijvu37bpcjuelfrdhorrkf5orqbkco4xooddook2@xi6f4tudankc>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<46kishp2tsijvu37bpcjuelfrdhorrkf5orqbkco4xooddook2@xi6f4tudankc>","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":30911,"web_url":"https://patchwork.libcamera.org/comment/30911/","msgid":"","date":"2024-08-27T07:49:07","subject":"Re: [PATCH v4 4/9] ipa: rkisp1: Add ISP parameters abstraction class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Tue, Aug 27, 2024 at 10:43:39AM GMT, Laurent Pinchart wrote:\n> On Tue, Aug 27, 2024 at 08:53:44AM +0200, Jacopo Mondi wrote:\n> > Hi Laurent\n> > On Tue, Aug 27, 2024 at 04:40:38AM GMT, 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> > > Reviewed-by: Paul Elder \n> > > Reviewed-by: Stefan Klug \n> > > ---\n> > > Changes since v3:\n> > >\n> > > - Update to latest kernel API\n> > >\n> >\n> > I don't get it, what kernel API was the previous version based on ?\n>\n> This should have been in the \"changes since v2\" section, sorry. I'm\n> referring to the usage of the .flags field, replacing .enabled.\n>\n\nAh ok, I was scared I have been testing against an older kernel\nrevision ;)\n\n> > I tested against your v6.11/merge branch which if I'm not mistaken has\n> > media-stage merged in.\n> >\n> > > Changes since v2:\n> > >\n> > > - Rename Block to BlockType\n> > > - Fix grammer in comment\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 | 219 +++++++++++++++++++++++++++++++++++++\n> > > src/ipa/rkisp1/params.h | 157 ++++++++++++++++++++++++++\n> > > 3 files changed, 377 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 160ef52dd52e..34844f1498f9 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..6bc6f89919fb\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/params.cpp\n> > > @@ -0,0 +1,219 @@\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{ BlockType::block, {\t\t\t\t\t\t\t\\\n> > > +\t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> > > +\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> > > +\t\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{ BlockType::block, {\t\t\t\t\t\t\t\\\n> > > +\t\tRKISP1_EXT_PARAMS_BLOCK_TYPE_##id,\t\t\t\t\\\n> > > +\t\tsizeof(struct rkisp1_cif_isp_##type##_config),\t\t\t\\\n> > > +\t\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, BlockType type,\n> > > +\t\t\t\t\t const Span &data)\n> > > +\t: params_(params), type_(type)\n> > > +{\n> > > +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > > +\t\theader_ = data.subspan(0, sizeof(rkisp1_ext_params_block_header));\n> > > +\t\tdata_ = data.subspan(sizeof(rkisp1_ext_params_block_header));\n> > > +\t} else {\n> > > +\t\tdata_ = data;\n> > > +\t}\n> > > +}\n> > > +\n> > > +void RkISP1ParamsBlockBase::setEnabled(bool enabled)\n> > > +{\n> > > +\t/*\n> > > +\t * For the legacy fixed format, blocks are enabled in the top-level\n> > > +\t * header. Delegate to the RkISP1Params class.\n> > > +\t */\n> > > +\tif (params_->format() == V4L2_META_FMT_RK_ISP1_PARAMS)\n> > > +\t\treturn params_->setBlockEnabled(type_, enabled);\n> > > +\n> > > +\t/*\n> > > +\t * For the extensible format, set the enable and disable flags in the\n> > > +\t * block header directly.\n> > > +\t */\n> > > +\tstruct rkisp1_ext_params_block_header *header =\n> > > +\t\treinterpret_cast(header_.data());\n> > > +\theader->flags &= ~(RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |\n> > > +\t\t\t RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE);\n> > > +\theader->flags |= enabled ? RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE\n> > > +\t\t\t\t : RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE;\n> > > +}\n> > > +\n> > > +RkISP1Params::RkISP1Params(uint32_t format, Span data)\n> > > +\t: format_(format), data_(data), used_(0)\n> > > +{\n> > > +\tif (format_ == V4L2_META_FMT_RK_ISP1_EXT_PARAMS) {\n> > > +\t\tstruct rkisp1_ext_params_cfg *cfg =\n> > > +\t\t\treinterpret_cast(data.data());\n> > > +\n> > > +\t\tcfg->version = RKISP1_EXT_PARAM_BUFFER_V1;\n> > > +\t\tcfg->data_size = 0;\n> > > +\n> > > +\t\tused_ += offsetof(struct rkisp1_ext_params_cfg, data);\n> > > +\t} else {\n> > > +\t\tmemset(data.data(), 0, data.size());\n> > > +\t\tused_ = sizeof(struct rkisp1_params_cfg);\n> > > +\t}\n> > > +}\n> > > +\n> > > +void RkISP1Params::setBlockEnabled(BlockType type, bool enabled)\n> > > +{\n> > > +\tconst BlockTypeInfo &info = kBlockTypeInfo.at(type);\n> > > +\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(BlockType 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 only in extended parameters 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..28a781bc447c\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 BlockType {\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\\\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, BlockType 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> > > +\tBlockType type_;\n> > > +\tSpan header_;\n> > > +\tSpan data_;\n> > > +};\n> > > +\n> > > +template\n> > > +class RkISP1ParamsBlock : public RkISP1ParamsBlockBase\n> > > +{\n> > > +public:\n> > > +\tusing Type = typename details::block_type::type;\n> > > +\n> > > +\tRkISP1ParamsBlock(RkISP1Params *params, const Span &data)\n> > > +\t\t: RkISP1ParamsBlockBase(params, B, data)\n> > > +\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(BlockType type);\n> > > +\tvoid setBlockEnabled(BlockType 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*/\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 384C8C324C\n\tfor ;\n\tTue, 27 Aug 2024 07:49:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE3F063446;\n\tTue, 27 Aug 2024 09:49:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE1AA6343F\n\tfor ;\n\tTue, 27 Aug 2024 09:49:12 +0200 (CEST)","from ideasonboard.com (mob-5-90-141-165.net.vodafone.it\n\t[5.90.141.165])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C0909FF;\n\tTue, 27 Aug 2024 09:48:04 +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=\"TvoUTyKo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724744884;\n\tbh=GJ/X4oIhEIL0iHl1ksxZR6/ElA4Hi9ZxElLqcAm58JY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TvoUTyKo9rdy6DA66bdeJahAsn8eQUmYBNmyp6l2Zm4XiaoA8cAj3+I8m6m+291op\n\teYqEBnS3m+TQeoBEphTVr+4qtuKbUeAOcO+P1lq4tcRqkxwGznZaTdWqjan0TKPufm\n\tgzx2dDQNog4pFuDoSJteZh8yNrE8RUj/702nn57A=","Date":"Tue, 27 Aug 2024 09:49:07 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 4/9] ipa: rkisp1: Add ISP parameters abstraction class","Message-ID":"","References":"<20240827014044.24673-1-laurent.pinchart@ideasonboard.com>\n\t<20240827014044.24673-5-laurent.pinchart@ideasonboard.com>\n\t<46kishp2tsijvu37bpcjuelfrdhorrkf5orqbkco4xooddook2@xi6f4tudankc>\n\t<20240827074339.GC23129@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240827074339.GC23129@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\" "}}]