[{"id":18645,"web_url":"https://patchwork.libcamera.org/comment/18645/","msgid":"","date":"2021-08-10T00:53:07","subject":"Re: [libcamera-devel] [PATCH v3 3/5] libcamera: controls: Use\n\tControlIdMap in deserialization","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Aug 09, 2021 at 05:23:06PM +0200, Jacopo Mondi wrote:\n> Introduce a new field in the controls serialization protocol to\n> allow discerning which ControlIdMap a ControlInfoMap refers to.\n> \n> The newly introduced IdMapType enumeration describes the possible\n> info maps:\n> - Either the globally available controls::controls and\n> properties::properties maps, which are valid across IPC boundaries\n> - A ControlIdMap created locally by the V4L2 device, which is not valid\n> across the IPC boundaries\n> \n> At de-serialization time the idMapType filed is inspected and\n> - If the idmap is a globally available one, there's no need to create\n> new ControlId instances when populating the de-serialized\n> ControlInfoMap. Use the globally available map to retrieve the\n> ControlId reference and use it\n> - If the idmap is a map only available locally, create a new ControlId\n> as it used to happen before this patch.\n> \n> As a direct consequence, this change allows us to perform lookup by\n> ControlId reference on de-serialized ControlIdMap that refers to the\n> libcamera defined controls::controls and properties::properties.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Paul Elder \n> ---\n> include/libcamera/ipa/ipa_controls.h | 9 +++-\n> src/libcamera/control_serializer.cpp | 65 +++++++++++++++++++++++-----\n> src/libcamera/ipa_controls.cpp | 29 +++++++++++++\n> 3 files changed, 90 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> index 6d3bf279c22d..f4bb7b77a974 100644\n> --- a/include/libcamera/ipa/ipa_controls.h\n> +++ b/include/libcamera/ipa/ipa_controls.h\n> @@ -15,13 +15,20 @@ extern \"C\" {\n> \n> #define IPA_CONTROLS_FORMAT_VERSION\t1\n> \n> +enum ipa_controls_id_map_type {\n> +\tIPA_CONTROL_ID_MAP_CONTROLS,\n> +\tIPA_CONTROL_ID_MAP_PROPERTIES,\n> +\tIPA_CONTROL_ID_MAP_V4L2,\n> +};\n> +\n> struct ipa_controls_header {\n> \tuint32_t version;\n> \tuint32_t handle;\n> \tuint32_t entries;\n> \tuint32_t size;\n> \tuint32_t data_offset;\n> -\tuint32_t reserved[3];\n> +\tipa_controls_id_map_type id_map_type;\n\nIn C you need to include the enum keyword.\n\n> +\tuint32_t reserved[2];\n> };\n> \n> struct ipa_control_value_entry {\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index df6ed93f477e..9127678f25f6 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -17,6 +17,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \"libcamera/internal/byte_stream_buffer.h\"\n> \n> @@ -188,6 +189,15 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \tfor (const auto &ctrl : infoMap)\n> \t\tvaluesSize += binarySize(ctrl.second);\n> \n> +\tconst ControlIdMap *idmap = &infoMap.idmap();\n> +\tenum ipa_controls_id_map_type idMapType;\n> +\tif (idmap == &controls::controls)\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> +\telse if (idmap == &properties::properties)\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> +\telse\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_V4L2;\n> +\n> \t/* Prepare the packet header, assign a handle to the ControlInfoMap. */\n> \tstruct ipa_controls_header hdr;\n> \thdr.version = IPA_CONTROLS_FORMAT_VERSION;\n> @@ -195,6 +205,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \thdr.entries = infoMap.size();\n> \thdr.size = sizeof(hdr) + entriesSize + valuesSize;\n> \thdr.data_offset = sizeof(hdr) + entriesSize;\n> +\thdr.id_map_type = idMapType;\n> \n> \tbuffer.write(&hdr);\n> \n> @@ -368,6 +379,33 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\treturn {};\n> \t}\n> \n> +\t/*\n> + * Use the ControlIdMap corresponding to the id map type. If the type\n> + * references a globally defined id map (such as controls::controls\n> + * or properties::properties), use it. Otherwise, create a local id map\n> + * that will be populated with dynamically created ControlId instances\n> + * when deserializing individual ControlInfoMap entries.\n\nCopied and pasted from e-mail, with spaces instead of tabs ? :-)\n\nReviewed-by: Laurent Pinchart \n\n> +\t */\n> +\tconst ControlIdMap *idMap = nullptr;\n> +\tControlIdMap *localIdMap = nullptr;\n> +\tswitch (hdr->id_map_type) {\n> +\tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> +\t\tidMap = &controls::controls;\n> +\t\tbreak;\n> +\tcase IPA_CONTROL_ID_MAP_PROPERTIES:\n> +\t\tidMap = &properties::properties;\n> +\t\tbreak;\n> +\tcase IPA_CONTROL_ID_MAP_V4L2:\n> +\t\tcontrolIdMaps_.emplace_back(std::make_unique());\n> +\t\tlocalIdMap = controlIdMaps_.back().get();\n> +\t\tidMap = localIdMap;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tLOG(Serializer, Error)\n> +\t\t\t<< \"Unknown id map type: \" << hdr->id_map_type;\n> +\t\treturn {};\n> +\t}\n> +\n> \tByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));\n> \tByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);\n> \n> @@ -377,9 +415,6 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t}\n> \n> \tControlInfoMap::Map ctrls;\n> -\tcontrolIdMaps_.emplace_back(std::make_unique());\n> -\tControlIdMap *localIdMap = controlIdMaps_.back().get();\n> -\n> \tfor (unsigned int i = 0; i < hdr->entries; ++i) {\n> \t\tconst struct ipa_control_info_entry *entry =\n> \t\t\tentries.read();\n> @@ -388,15 +423,21 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t\t\treturn {};\n> \t\t}\n> \n> -\t\t/* Create and cache the individual ControlId. */\n> \t\tControlType type = static_cast(entry->type);\n> -\t\t/**\n> -\t\t * \\todo Find a way to preserve the control name for debugging\n> -\t\t * purpose.\n> -\t\t */\n> -\t\tcontrolIds_.emplace_back(std::make_unique(entry->id, \"\", type));\n> -\t\tControlId *controlId = controlIds_.back().get();\n> -\t\t(*localIdMap)[entry->id] = controlId;\n> +\n> +\t\t/* If we're using a local id map, populate it. */\n> +\t\tif (localIdMap) {\n> +\t\t\t/**\n> +\t\t\t * \\todo Find a way to preserve the control name for\n> +\t\t\t * debugging purpose.\n> +\t\t\t */\n> +\t\t\tcontrolIds_.emplace_back(std::make_unique(entry->id,\n> +\t\t\t\t\t\t\t\t\t \"\", type));\n> +\t\t\t(*localIdMap)[entry->id] = controlIds_.back().get();\n> +\t\t}\n> +\n> +\t\tconst ControlId *controlId = idMap->at(entry->id);\n> +\t\tASSERT(controlId);\n> \n> \t\tif (entry->offset != values.offset()) {\n> \t\t\tLOG(Serializer, Error)\n> @@ -413,7 +454,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \t * Create the ControlInfoMap in the cache, and store the map to handle\n> \t * association.\n> \t */\n> -\tinfoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *localIdMap);\n> +\tinfoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);\n> \tControlInfoMap &map = infoMaps_[hdr->handle];\n> \tinfoMapHandles_[&map] = hdr->handle;\n> \n> diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp\n> index 8fd726513182..fb98cda30ede 100644\n> --- a/src/libcamera/ipa_controls.cpp\n> +++ b/src/libcamera/ipa_controls.cpp\n> @@ -134,6 +134,33 @@\n> * \\brief The current control serialization format version\n> */\n> \n> +/**\n> + * \\var ipa_controls_id_map_type\n> + * \\brief Enumerates the different control id map types\n> + *\n> + * Each ControlInfoMap and ControlList refers to a control id map that\n> + * associates the ControlId references to a numerical identifier.\n> + * During the serialization procedure the raw pointers to the ControlId\n> + * instances cannot be transported on the wire, hence their numerical id is\n> + * used to identify them in the serialized data buffer. At deserialization time\n> + * it is required to associate back to the numerical id the ControlId instance\n> + * it represents. This enumeration describes which ControlIdMap should be\n> + * used to perform such operation.\n> + *\n> + * \\var ipa_controls_id_map_type::IPA_CONTROL_ID_MAP_CONTROLS\n> + * \\brief The numerical control identifier are resolved to a ControlId * using\n> + * the global controls::controls id map\n> + * \\var ipa_controls_id_map_type::IPA_CONTROL_ID_MAP_PROPERTIES\n> + * \\brief The numerical control identifier are resolved to a ControlId * using\n> + * the global properties::properties id map\n> + * \\var ipa_controls_id_map_type::IPA_CONTROL_ID_MAP_V4L2\n> + * \\brief ControlId for V4L2 defined controls are created by the video device\n> + * that enumerates them, and are not available across the IPC boundaries. The\n> + * deserializer shall create new ControlId instances for them as well as store\n> + * them in a dedicated ControlIdMap. Only lookup by numerical id can be\n> + * performed on de-serialized ControlInfoMap that represents V4L2 controls.\n> + */\n> +\n> /**\n> * \\struct ipa_controls_header\n> * \\brief Serialized control packet header\n> @@ -149,6 +176,8 @@\n> * The total packet size in bytes\n> * \\var ipa_controls_header::data_offset\n> * Offset in bytes from the beginning of the packet of the data section start\n> + * \\var ipa_controls_header::id_map_type\n> + * The id map type as defined by the ipa_controls_id_map_type enumeration\n> * \\var ipa_controls_header::reserved\n> * Reserved for future extensions\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 1EB49BD87D\n\tfor ;\n\tTue, 10 Aug 2021 00:53:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95C6E68826;\n\tTue, 10 Aug 2021 02:53:11 +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 26062687EB\n\tfor ;\n\tTue, 10 Aug 2021 02:53:10 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8F021466;\n\tTue, 10 Aug 2021 02:53:09 +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=\"r9+oYffY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628556789;\n\tbh=wHTVwwA1rXuF4tKnEoL++6/X6YIVi7iktfD7WAd8DU8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r9+oYffY4WatHTXg8LdMLfyAPJmJuTCuoKnQrJbHXNjpTePjA0dzJLukoDpqKFpci\n\txQLmlRDbd22ltp6GccOsKOiPP8rerXEiojdiiEH+NfCLFG+wFSbQvRH3RPFlB+0D8V\n\tc7DbxbaI9UYCBvGyXpdHPmBilw/PgHK3yb8vyCRE=","Date":"Tue, 10 Aug 2021 03:53:07 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210809152308.31947-1-jacopo@jmondi.org>\n\t<20210809152308.31947-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210809152308.31947-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 3/5] libcamera: controls: Use\n\tControlIdMap in deserialization","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]