[{"id":30306,"web_url":"https://patchwork.libcamera.org/comment/30306/","msgid":"","date":"2024-07-04T09:58:10","subject":"Re: [PATCH v1 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 01:52:24AM +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> src/ipa/rkisp1/meson.build | 1 +\n> src/ipa/rkisp1/params.cpp | 169 +++++++++++++++++++++++++++++++++++++\n> src/ipa/rkisp1/params.h | 140 ++++++++++++++++++++++++++++++\n> 3 files changed, 310 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..ac25ade1c8c4\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/params.cpp\n> @@ -0,0 +1,169 @@\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> +namespace libcamera {\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> +\t__u32 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> +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> +Span RkISP1Params::block(Block type, State state)\n> +{\n> +\tauto infoIt = kBlockTypeInfo.find(type);\n> +\tif (infoIt == kBlockTypeInfo.end())\n> +\t\treturn {};\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\treturn {};\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> +\t\tcfg->module_ens = state == State::Enable ? info.enableBit : 0;\n\nI guess I'm missing something here. But shouldn't these only modify the\nrelevant bit, as the cfg structure is shared among all algorithms? \n\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.\n> +\t */\n> +\n> +\t/* Look up the block in the cache first. Caching helps providing the\n> +\t * same behaviour for the legacy and extensible formats when block() is\n> +\t * called multiple times for the same block, which simplifies the\n> +\t * implementation of algorithms and avoids hard to debug issues.\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\treturn {};\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->enable = state == State::Enable\n> +\t\t ? RKISP1_EXT_PARAMS_BLOCK_ENABLE\n> +\t\t : RKISP1_EXT_PARAMS_BLOCK_DISABLE;\n> +\theader->size = block.size();\n> +\n> +\t/* Skip the block header to get the data. */\n> +\tblock = block.subspan(sizeof(*header));\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..ddd081de7894\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/params.h\n> @@ -0,0 +1,140 @@\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> +#include \n> +\n> +#include \n> +\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> +template\n> +class RkISP1ParamsBlock\n> +{\n> +public:\n> +\tusing Type = typename details::block_type::type;\n> +\n> +\tRkISP1ParamsBlock(const Span &data)\n> +\t\t: data_(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> +private:\n> +\tSpan data_;\n> +};\n> +\n> +class RkISP1Params\n> +{\n> +public:\n> +\tenum class State {\n> +\t\tDisable = 1,\n> +\t\tEnable = 1,\n> +\t};\n> +\n> +\tRkISP1Params(uint32_t format, Span data);\n> +\n> +\ttemplate\n> +\tRkISP1ParamsBlock block(State state)\n\nYou mentioned that on irc already. To me it would feel more natural to\ndo: \n\nblock = params->block();\nblock.setEnabled(true/false);\nblock->someconfig...\n\nBut I didn't check the implications that would have on the\nimplementation :-)\n\nOtherwise I like that concept.\n\nRegards,\nStefan\n\n\n> +\t{\n> +\t\treturn RkISP1ParamsBlock(block(B, state));\n> +\t}\n> +\n> +\tsize_t size() const { return used_; }\n> +\n> +private:\n> +\tSpan block(Block type, State state);\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 9140ABD87C\n\tfor ;\n\tThu, 4 Jul 2024 09:58:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE0E962E23;\n\tThu, 4 Jul 2024 11:58:13 +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 B21EE619C4\n\tfor ;\n\tThu, 4 Jul 2024 11:58:12 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c01a:815:781:30fb])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DE0E0502;\n\tThu, 4 Jul 2024 11:57:43 +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=\"iwmtGyrH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720087064;\n\tbh=5UURSzOleoXk1aZsnhgaM7wwCRrWCYAOW72mRZ1PD4Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iwmtGyrHuden6lEEY+PvYBeVgjlMmEACJJCbcoOOw7QTbe5MrFORq6m5JQGE9hLUh\n\tiZxv4wibUtp3ZCjGhZkQsDMGaqOPaWcQ5gZ+WcwJQClo0bQ30/UVjyBgcV0h5CkjXQ\n\tQDJ03Gyc/u+sP+V/dmEpwbc11EZDIznrf8UxLeNs=","Date":"Thu, 4 Jul 2024 11:58:10 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ,\n\tPaul Elder ","Subject":"Re: [PATCH v1 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","Message-ID":"","References":"<20240703225230.3530-1-laurent.pinchart@ideasonboard.com>\n\t<20240703225230.3530-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240703225230.3530-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":30309,"web_url":"https://patchwork.libcamera.org/comment/30309/","msgid":"<20240704100456.GB10099@pendragon.ideasonboard.com>","date":"2024-07-04T10:04:56","subject":"Re: [PATCH v1 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 Thu, Jul 04, 2024 at 11:58:10AM +0200, Stefan Klug wrote:\n> Hi Laurent,\n> \n> Thank you for the patch.\n> \n> On Thu, Jul 04, 2024 at 01:52:24AM +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> > src/ipa/rkisp1/meson.build | 1 +\n> > src/ipa/rkisp1/params.cpp | 169 +++++++++++++++++++++++++++++++++++++\n> > src/ipa/rkisp1/params.h | 140 ++++++++++++++++++++++++++++++\n> > 3 files changed, 310 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..ac25ade1c8c4\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/params.cpp\n> > @@ -0,0 +1,169 @@\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> > +namespace libcamera {\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> > +\t__u32 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> > +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> > +Span RkISP1Params::block(Block type, State state)\n> > +{\n> > +\tauto infoIt = kBlockTypeInfo.find(type);\n> > +\tif (infoIt == kBlockTypeInfo.end())\n> > +\t\treturn {};\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\treturn {};\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> > +\t\tcfg->module_ens = state == State::Enable ? info.enableBit : 0;\n> \n> I guess I'm missing something here. But shouldn't these only modify the\n> relevant bit, as the cfg structure is shared among all algorithms? \n\nHow did I miss that... This clearly needs to be |=.\n\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.\n> > +\t */\n> > +\n> > +\t/* Look up the block in the cache first. Caching helps providing the\n> > +\t * same behaviour for the legacy and extensible formats when block() is\n> > +\t * called multiple times for the same block, which simplifies the\n> > +\t * implementation of algorithms and avoids hard to debug issues.\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\treturn {};\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->enable = state == State::Enable\n> > +\t\t ? RKISP1_EXT_PARAMS_BLOCK_ENABLE\n> > +\t\t : RKISP1_EXT_PARAMS_BLOCK_DISABLE;\n> > +\theader->size = block.size();\n> > +\n> > +\t/* Skip the block header to get the data. */\n> > +\tblock = block.subspan(sizeof(*header));\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..ddd081de7894\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/params.h\n> > @@ -0,0 +1,140 @@\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> > +#include \n> > +\n> > +#include \n> > +\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> > +template\n> > +class RkISP1ParamsBlock\n> > +{\n> > +public:\n> > +\tusing Type = typename details::block_type::type;\n> > +\n> > +\tRkISP1ParamsBlock(const Span &data)\n> > +\t\t: data_(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> > +private:\n> > +\tSpan data_;\n> > +};\n> > +\n> > +class RkISP1Params\n> > +{\n> > +public:\n> > +\tenum class State {\n> > +\t\tDisable = 1,\n> > +\t\tEnable = 1,\n> > +\t};\n> > +\n> > +\tRkISP1Params(uint32_t format, Span data);\n> > +\n> > +\ttemplate\n> > +\tRkISP1ParamsBlock block(State state)\n> \n> You mentioned that on irc already. To me it would feel more natural to\n> do: \n> \n> block = params->block();\n> block.setEnabled(true/false);\n> block->someconfig...\n\nI've considered that too. I'll give it a try. Would you object this\nbeing changed on top of the series ?\n\n> But I didn't check the implications that would have on the\n> implementation :-)\n> \n> Otherwise I like that concept.\n> \n> > +\t{\n> > +\t\treturn RkISP1ParamsBlock(block(B, state));\n> > +\t}\n> > +\n> > +\tsize_t size() const { return used_; }\n> > +\n> > +private:\n> > +\tSpan block(Block type, State state);\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 147A0BEFBE\n\tfor ;\n\tThu, 4 Jul 2024 10:05:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B229162E24;\n\tThu, 4 Jul 2024 12:05:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 53CAE62E17\n\tfor ;\n\tThu, 4 Jul 2024 12:05:17 +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 9144E502;\n\tThu, 4 Jul 2024 12:04:48 +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=\"EndaGznU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720087488;\n\tbh=itxhZLgfyD8KAHAanJpHT2DPR6v+WRs0LrWUJGNdeoM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EndaGznUWGR8uNUCPixzBpvNUc1BVeNVNYUltuKZri7CU3Q4tS7Pacpa3naIL14PJ\n\tcbV5kVPzsSDQ7SAgmz8esXnWPsPFp43CGiggouJznuaNU21aEMvWEbDMeCHHXS7dkJ\n\tQ2T4AlhgINDOqWSk56JasIkGkSpITOdaKQdgvYkQ=","Date":"Thu, 4 Jul 2024 13:04:56 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ,\n\tPaul Elder ","Subject":"Re: [PATCH v1 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","Message-ID":"<20240704100456.GB10099@pendragon.ideasonboard.com>","References":"<20240703225230.3530-1-laurent.pinchart@ideasonboard.com>\n\t<20240703225230.3530-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":30326,"web_url":"https://patchwork.libcamera.org/comment/30326/","msgid":"","date":"2024-07-04T11:50:25","subject":"Re: [PATCH v1 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Thu, Jul 04, 2024 at 01:52:24AM 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> ---\n> src/ipa/rkisp1/meson.build | 1 +\n> src/ipa/rkisp1/params.cpp | 169 +++++++++++++++++++++++++++++++++++++\n> src/ipa/rkisp1/params.h | 140 ++++++++++++++++++++++++++++++\n> 3 files changed, 310 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..ac25ade1c8c4\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/params.cpp\n> @@ -0,0 +1,169 @@\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> +namespace libcamera {\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> +\t__u32 enableBit;\n\nWhy __u32 and not just unsigned int or uint32_t ?\n\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> +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> +Span RkISP1Params::block(Block type, State state)\n> +{\n> +\tauto infoIt = kBlockTypeInfo.find(type);\n> +\tif (infoIt == kBlockTypeInfo.end())\n> +\t\treturn {};\n\nAs this can only be caused by a development errors (right?) should\nthis be made Fatal ?\n\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\treturn {};\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> +\t\tcfg->module_ens = state == State::Enable ? info.enableBit : 0;\n> +\n> +\t\treturn data_.subspan(info.offset, info.size);\n\nclever\n\n> +\t}\n> +\n> +\t/*\n> +\t * For the extensible format, allocate memory for the block, including\n> +\t * the header.\n> +\t */\n> +\n> +\t/* Look up the block in the cache first. Caching helps providing the\n\n /*\n\t * Look up the block in the cache first. Caching helps providing the\n\n> +\t * same behaviour for the legacy and extensible formats when block() is\n\nI'm not sure this is very much related to legacy vs extensible or just\nto the fact when an algorithm references a block, it wants to get the\nsame piece of memory every time\n\n> +\t * called multiple times for the same block, which simplifies the\n> +\t * implementation of algorithms and avoids hard to debug issues.\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\nblocks are 8-bytes aligned, do you need the + 7.. ?\n\n> +\tif (size > data_.size() - used_)\n> +\t\treturn {};\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\nIsn't it better to memset the whole data block to 0 at construction\ntime ?\n\n> +\n> +\tstruct rkisp1_ext_params_block_header *header =\n> +\t\treinterpret_cast(block.data());\n> +\theader->type = info.type;\n> +\theader->enable = state == State::Enable\n> +\t\t ? RKISP1_EXT_PARAMS_BLOCK_ENABLE\n> +\t\t : RKISP1_EXT_PARAMS_BLOCK_DISABLE;\n> +\theader->size = block.size();\n> +\n> +\t/* Skip the block header to get the data. */\n> +\tblock = block.subspan(sizeof(*header));\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..ddd081de7894\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/params.h\n> @@ -0,0 +1,140 @@\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\nDo you need optional ?\n\n> +#include \n> +\n> +#include \n> +\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> +template\n> +class RkISP1ParamsBlock\n> +{\n> +public:\n> +\tusing Type = typename details::block_type::type;\n\nclever!\n\nI have a gut feeling we could generate the templates specializations\nof RkISP1ParamsBlock<> without going through details::block_type<> but\nthis is surely nice\n\n> +\n> +\tRkISP1ParamsBlock(const Span &data)\n> +\t\t: data_(data)\n> +\t{\n\nis it necessary to expliticlty delete copy and move constructors ?\n\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> +private:\n> +\tSpan data_;\n> +};\n> +\n> +class RkISP1Params\n> +{\n> +public:\n> +\tenum class State {\n> +\t\tDisable = 1,\n\nDisable = 0,\n\nor simply drop the initializers\n\nvery nice and elegant!\n\n> +\t\tEnable = 1,\n> +\t};\n> +\n> +\tRkISP1Params(uint32_t format, Span data);\n> +\n> +\ttemplate\n> +\tRkISP1ParamsBlock block(State state)\n> +\t{\n> +\t\treturn RkISP1ParamsBlock(block(B, state));\n> +\t}\n> +\n> +\tsize_t size() const { return used_; }\n> +\n> +private:\n> +\tSpan block(Block type, State state);\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 90DFDBD87C\n\tfor ;\n\tThu, 4 Jul 2024 11:50:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38B3362E22;\n\tThu, 4 Jul 2024 13:50:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28933619C8\n\tfor ;\n\tThu, 4 Jul 2024 13:50:28 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1FB67502;\n\tThu, 4 Jul 2024 13:49:59 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Wz/JHAHp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720093799;\n\tbh=iRsr4r/VzqRujLN4HcnkcXfn1wL4PI+lR9Bidd9RSOY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Wz/JHAHpjW2bbitAHSlvz51pdiOBwExiG7yjf3AbLWLJOV3jnCAJwpyYDfmqNF4WN\n\tmRlBgZLCyWwlF+hQaVR4xtxQiaFqB+MbtCevErR5iPmUn9lLUxEl8veU3IeEo+giim\n\tQ+lshNFF4CmiLSKv9kSUk5fYhFbDbmfK3kFa8eK8=","Date":"Thu, 4 Jul 2024 13:50:25 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ,\n\tPaul Elder ","Subject":"Re: [PATCH v1 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","Message-ID":"","References":"<20240703225230.3530-1-laurent.pinchart@ideasonboard.com>\n\t<20240703225230.3530-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240703225230.3530-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":30329,"web_url":"https://patchwork.libcamera.org/comment/30329/","msgid":"<20240704132409.GD6230@pendragon.ideasonboard.com>","date":"2024-07-04T13:24:09","subject":"Re: [PATCH v1 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 Thu, Jul 04, 2024 at 01:50:25PM +0200, Jacopo Mondi wrote:\n> Hi Laurent\n> \n> On Thu, Jul 04, 2024 at 01:52:24AM 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> > ---\n> > src/ipa/rkisp1/meson.build | 1 +\n> > src/ipa/rkisp1/params.cpp | 169 +++++++++++++++++++++++++++++++++++++\n> > src/ipa/rkisp1/params.h | 140 ++++++++++++++++++++++++++++++\n> > 3 files changed, 310 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..ac25ade1c8c4\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/params.cpp\n> > @@ -0,0 +1,169 @@\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> > +namespace libcamera {\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> > +\t__u32 enableBit;\n> \n> Why __u32 and not just unsigned int or uint32_t ?\n\nBecause it's a bit for the kernel API. It doesn't matter much though,\nI'll switch back to uint32_t.\n\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> > +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> > +Span RkISP1Params::block(Block type, State state)\n> > +{\n> > +\tauto infoIt = kBlockTypeInfo.find(type);\n> > +\tif (infoIt == kBlockTypeInfo.end())\n> > +\t\treturn {};\n> \n> As this can only be caused by a development errors (right?) should\n> this be made Fatal ?\n\nI can do that.\n\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\treturn {};\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> > +\t\tcfg->module_ens = state == State::Enable ? info.enableBit : 0;\n> > +\n> > +\t\treturn data_.subspan(info.offset, info.size);\n> \n> clever\n\nThanks :-)\n\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * For the extensible format, allocate memory for the block, including\n> > +\t * the header.\n> > +\t */\n> > +\n> > +\t/* Look up the block in the cache first. Caching helps providing the\n> \n> /*\n> \t * Look up the block in the cache first. Caching helps providing the\n> \n> > +\t * same behaviour for the legacy and extensible formats when block() is\n> \n> I'm not sure this is very much related to legacy vs extensible or just\n> to the fact when an algorithm references a block, it wants to get the\n> same piece of memory every time\n\nFor the legacy format that's what happens, so I wanted the same to be\ntrue for the extensible format. I'll reword the message.\n\n> > +\t * called multiple times for the same block, which simplifies the\n> > +\t * implementation of algorithms and avoids hard to debug issues.\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> \n> blocks are 8-bytes aligned, do you need the + 7.. ?\n\nThe inner data structures (e.g. rkisp1_cif_isp_bls_config) are not\nmarked as aligned in the UAPI.\n\n> > +\tif (size > data_.size() - used_)\n> > +\t\treturn {};\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> Isn't it better to memset the whole data block to 0 at construction\n> time ?\n\nThat's more costly, given that most of the time only a small number of\nblocks will be included.\n\n> > +\n> > +\tstruct rkisp1_ext_params_block_header *header =\n> > +\t\treinterpret_cast(block.data());\n> > +\theader->type = info.type;\n> > +\theader->enable = state == State::Enable\n> > +\t\t ? RKISP1_EXT_PARAMS_BLOCK_ENABLE\n> > +\t\t : RKISP1_EXT_PARAMS_BLOCK_DISABLE;\n> > +\theader->size = block.size();\n> > +\n> > +\t/* Skip the block header to get the data. */\n> > +\tblock = block.subspan(sizeof(*header));\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..ddd081de7894\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/params.h\n> > @@ -0,0 +1,140 @@\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> Do you need optional ?\n\nI used to :-) Probably not anymore.\n\n> > +#include \n> > +\n> > +#include \n> > +\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> > +template\n> > +class RkISP1ParamsBlock\n> > +{\n> > +public:\n> > +\tusing Type = typename details::block_type::type;\n> \n> clever!\n> \n> I have a gut feeling we could generate the templates specializations\n> of RkISP1ParamsBlock<> without going through details::block_type<> but\n> this is surely nice\n> \n> > +\n> > +\tRkISP1ParamsBlock(const Span &data)\n> > +\t\t: data_(data)\n> > +\t{\n> \n> is it necessary to expliticlty delete copy and move constructors ?\n\nGiven the current implementation copying would hurt, but I can try\ndisabling them.\n\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> > +private:\n> > +\tSpan data_;\n> > +};\n> > +\n> > +class RkISP1Params\n> > +{\n> > +public:\n> > +\tenum class State {\n> > +\t\tDisable = 1,\n> \n> Disable = 0,\n> \n> or simply drop the initializers\n> \n> very nice and elegant!\n> \n> > +\t\tEnable = 1,\n> > +\t};\n> > +\n> > +\tRkISP1Params(uint32_t format, Span data);\n> > +\n> > +\ttemplate\n> > +\tRkISP1ParamsBlock block(State state)\n> > +\t{\n> > +\t\treturn RkISP1ParamsBlock(block(B, state));\n> > +\t}\n> > +\n> > +\tsize_t size() const { return used_; }\n> > +\n> > +private:\n> > +\tSpan block(Block type, State state);\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 24E30BD87C\n\tfor ;\n\tThu, 4 Jul 2024 13:24:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A518962E22;\n\tThu, 4 Jul 2024 15:24:32 +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 5AB61619C8\n\tfor ;\n\tThu, 4 Jul 2024 15:24:31 +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 535EE63D;\n\tThu, 4 Jul 2024 15:24:02 +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=\"czkXSxcD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1720099442;\n\tbh=PwRWrZdA/uMj5kI2Sk97s4ZV0b6NYGgLsj/VtE2hkpU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=czkXSxcD6Gq3KvAcu6ppF1JlWtGH6vgEAoo9gVJWl0Zt3boOtC3S3blRboIXk6A+Y\n\tRK2NuknDmfjgMNcpXgbWjz+wxTyoLfbPTdxf2bNSXVje3fKvkrZyQSiQlCuGV66d2r\n\t2K6zMqp58dWSE1PXe6DECGAKdYWBI+QCHoLoC6SY=","Date":"Thu, 4 Jul 2024 16:24:09 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v1 05/11] ipa: rkisp1: Add ISP parameters abstraction\n\tclass","Message-ID":"<20240704132409.GD6230@pendragon.ideasonboard.com>","References":"<20240703225230.3530-1-laurent.pinchart@ideasonboard.com>\n\t<20240703225230.3530-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\" "}}]