From patchwork Tue Sep 7 11:10:38 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 13717 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id A907ABEFBE for ; Tue, 7 Sep 2021 11:10:02 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 6C6386917D; Tue, 7 Sep 2021 13:10:02 +0200 (CEST) Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 519BF6916C for ; Tue, 7 Sep 2021 13:10:00 +0200 (CEST) Received: (Authenticated sender: jacopo@jmondi.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id D1A0C60005; Tue, 7 Sep 2021 11:09:59 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org Date: Tue, 7 Sep 2021 13:10:38 +0200 Message-Id: <20210907111038.739104-6-jacopo@jmondi.org> X-Mailer: git-send-email 2.32.0 In-Reply-To: <20210907111038.739104-1-jacopo@jmondi.org> References: <20210907111038.739104-1-jacopo@jmondi.org> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 5/5] libcamera: control_serializer: Separate the handles space X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Two instances of the ControlSerializer class are in use at the IPC boundaries, one in the Proxy class that serializes data from the pipeline handler to the IPA, and one in the ProxyWorker which serializes data in the opposite direction. Each instance operates autonomously, without any centralized point of control, and each one assigns a numerical handle to each ControlInfoMap it serializes. This creates a risk of potential collision on the handle values, as both instances will use the same numerical space and are not aware of what handles has been already used by the instance "on the other side". To fix that, partition the handles numerical space by initializing the control serializer with a seed (even or odd) and increment the handle number by 2, to avoid any collision risk. While this is temporary and rather hacky solution, it solves an issue with isolated IPA modules without too much complexity added. Signed-off-by: Jacopo Mondi Reviewed-by: Paul Elder --- .../libcamera/internal/control_serializer.h | 8 ++- src/libcamera/control_serializer.cpp | 60 +++++++++++++++++-- test/serialization/control_serialization.cpp | 4 +- .../ipa_data_serializer_test.cpp | 26 ++++---- .../module_ipa_proxy.cpp.tmpl | 6 +- .../module_ipa_proxy.h.tmpl | 2 +- .../module_ipa_proxy_worker.cpp.tmpl | 14 +++-- .../libcamera_templates/proxy_functions.tmpl | 4 +- 8 files changed, 97 insertions(+), 27 deletions(-) diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h index 8a66be324138..f96c6f8443d1 100644 --- a/include/libcamera/internal/control_serializer.h +++ b/include/libcamera/internal/control_serializer.h @@ -20,7 +20,12 @@ class ByteStreamBuffer; class ControlSerializer { public: - ControlSerializer(); + enum class Seeds { + EVEN = 0, + ODD + }; + + ControlSerializer(Seeds seed); void reset(); @@ -47,6 +52,7 @@ private: ControlInfo loadControlInfo(ControlType type, ByteStreamBuffer &buffer); unsigned int serial_; + enum Seeds seed_; std::vector> controlIds_; std::vector> controlIdMaps_; std::map infoMaps_; diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index f1245ea620ab..a60e665af5ae 100644 --- a/src/libcamera/control_serializer.cpp +++ b/src/libcamera/control_serializer.cpp @@ -62,6 +62,14 @@ LOG_DEFINE_CATEGORY(Serializer) * corresponding ControlInfoMap handle in the binary data, and when * deserializing to retrieve the corresponding ControlInfoMap. * + * As a ControlSerializer instance is used at both sides of the IPC boundary, + * and the two instances operate without a shared point of control, there is a + * potential risk of collision of the numerical handles assigned to each + * serialized ControlInfoMap. For this reason the control serializer is + * initialized with an even/odd seed and the handle is incremented by 2, so that + * instances initialized with a different seed operates on a separate numerical + * space, avoiding any collision risk. + * * In order to perform those tasks, the serializer keeps an internal state that * needs to be properly populated. This mechanism requires the ControlInfoMap * corresponding to a ControlList to have been serialized or deserialized @@ -77,9 +85,42 @@ LOG_DEFINE_CATEGORY(Serializer) * proceed with care to avoid stale references. */ -ControlSerializer::ControlSerializer() - : serial_(0) +/** + * \enum ControlSerializer::Seeds + * \brief Define the seeds to initialize the ControlSerializer handle numerical + * space with + * + * \var ControlSerializer::Seeds::EVEN + * \brief The control serializer uses even numerical handles + * + * \var ControlSerializer::Seeds::ODD + * \brief The control serializer uses odd numerical handles + */ + +/** + * \brief Construct a new ControlSerializer and initialize its seed + * \param[in] seed The serializer's handle seed + */ +ControlSerializer::ControlSerializer(Seeds seed) + : seed_(seed) { + /* + * Initialize the handle numerical space using the seed value. + * + * Instances initialized with a different seed will use a different + * numerical handle space, avoiding any collision risk when, in example, + * two instances of the ControlSerializer class are used at the IPC + * boundaries. + * + * Start from 1 as '0' is a special value used as handle place holder + * when serializing lists that do not have a ControlInfoMap associated + * (in example list of libcamera controls::controls). + * + * \todo This is a temporary hack and should probably be better + * engineered, but for the time being it avoid collisions on the handle + * value when using IPC. + */ + serial_ = seed_ == Seeds::EVEN ? 2 : 1; } /** @@ -90,7 +131,7 @@ ControlSerializer::ControlSerializer() */ void ControlSerializer::reset() { - serial_ = 0; + serial_ = seed_ == Seeds::EVEN ? 2 : 1; infoMapHandles_.clear(); infoMaps_.clear(); @@ -200,10 +241,10 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, else idMapType = IPA_CONTROL_ID_MAP_V4L2; - /* Prepare the packet header, assign a handle to the ControlInfoMap. */ + /* Prepare the packet header. */ struct ipa_controls_header hdr; hdr.version = IPA_CONTROLS_FORMAT_VERSION; - hdr.handle = ++serial_; + hdr.handle = serial_; hdr.entries = infoMap.size(); hdr.size = sizeof(hdr) + entriesSize + valuesSize; hdr.data_offset = sizeof(hdr) + entriesSize; @@ -211,6 +252,15 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, buffer.write(&hdr); + /* + * Increment the handle for the ControlInfoMap by 2 keep the handles + * numerical space partitioned between instances initialized with a + * different seed. + * + * \sa ControlSerializer::Seeds + */ + serial_ += 2; + /* * Serialize all entries. * \todo Serialize the control name too diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp index 5ac9c4ede5f9..89c8ba59de24 100644 --- a/test/serialization/control_serialization.cpp +++ b/test/serialization/control_serialization.cpp @@ -30,8 +30,8 @@ protected: int run() override { - ControlSerializer serializer; - ControlSerializer deserializer; + ControlSerializer serializer(ControlSerializer::Seeds::EVEN); + ControlSerializer deserializer(ControlSerializer::Seeds::ODD); std::vector infoData; std::vector listData; diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp index eca19a6642e1..a1ce8e3767bc 100644 --- a/test/serialization/ipa_data_serializer_test.cpp +++ b/test/serialization/ipa_data_serializer_test.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -162,23 +163,24 @@ private: int testControls() { - ControlSerializer cs; + std::unique_ptr cs = + std::make_unique(ControlSerializer::Seeds::EVEN); const ControlInfoMap &infoMap = camera_->controls(); ControlList list = generateControlList(infoMap); std::vector infoMapBuf; std::tie(infoMapBuf, std::ignore) = - IPADataSerializer::serialize(infoMap, &cs); + IPADataSerializer::serialize(infoMap, cs.get()); std::vector listBuf; std::tie(listBuf, std::ignore) = - IPADataSerializer::serialize(list, &cs); + IPADataSerializer::serialize(list, cs.get()); const ControlInfoMap infoMapOut = - IPADataSerializer::deserialize(infoMapBuf, &cs); + IPADataSerializer::deserialize(infoMapBuf, cs.get()); - ControlList listOut = IPADataSerializer::deserialize(listBuf, &cs); + ControlList listOut = IPADataSerializer::deserialize(listBuf, cs.get()); if (!SerializationTest::equals(infoMap, infoMapOut)) { cerr << "Deserialized map doesn't match original" << endl; @@ -195,7 +197,8 @@ private: int testVector() { - ControlSerializer cs; + std::unique_ptr cs = + std::make_unique(ControlSerializer::Seeds::EVEN); /* * We don't test FileDescriptor serdes because it dup()s, so we @@ -257,7 +260,7 @@ private: if (testVectorSerdes(vecString) != TestPass) return TestFail; - if (testVectorSerdes(vecControlInfoMap, &cs) != TestPass) + if (testVectorSerdes(vecControlInfoMap, cs.get()) != TestPass) return TestFail; return TestPass; @@ -265,7 +268,8 @@ private: int testMap() { - ControlSerializer cs; + std::unique_ptr cs = + std::make_unique(ControlSerializer::Seeds::EVEN); /* * Realistically, only string and integral keys. @@ -302,13 +306,13 @@ private: if (testMapSerdes(mapStrStr) != TestPass) return TestFail; - if (testMapSerdes(mapUintCIM, &cs) != TestPass) + if (testMapSerdes(mapUintCIM, cs.get()) != TestPass) return TestFail; - if (testMapSerdes(mapIntCIM, &cs) != TestPass) + if (testMapSerdes(mapIntCIM, cs.get()) != TestPass) return TestFail; - if (testMapSerdes(mapStrCIM, &cs) != TestPass) + if (testMapSerdes(mapStrCIM, cs.get()) != TestPass) return TestFail; if (testMapSerdes(mapUintBVec) != TestPass) diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl index aab1fffbcaf0..7e28c2ed129a 100644 --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl @@ -52,6 +52,8 @@ namespace {{ns}} { << "initializing {{module_name}} proxy: loading IPA from " << ipam->path(); + controlSerializer_ = new ControlSerializer(ControlSerializer::Seeds::EVEN); + if (isolate_) { const std::string proxyWorkerPath = resolvePath("{{module_name}}_ipa_proxy"); if (proxyWorkerPath.empty()) { @@ -95,6 +97,8 @@ namespace {{ns}} { {{proxy_name}}::~{{proxy_name}}() { + delete controlSerializer_; + if (isolate_) { IPCMessage::Header header = { static_cast({{cmd_enum_name}}::Exit), seq_++ }; @@ -185,7 +189,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) {{proxy_funcs.func_sig(proxy_name, method, "IPC")}} { {%- if method.mojom_name == "configure" %} - controlSerializer_.reset(); + controlSerializer_->reset(); {%- endif %} {%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != "void" %} {%- set cmd = cmd_enum_name + "::" + method.mojom_name|cap %} diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl index 1979e68ff74d..fdbf25418963 100644 --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl @@ -118,7 +118,7 @@ private: std::unique_ptr ipc_; - ControlSerializer controlSerializer_; + ControlSerializer *controlSerializer_; {# \todo Move this to IPCPipe #} uint32_t seq_; diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl index c5e51532db53..5829059b8e93 100644 --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl @@ -53,9 +53,15 @@ class {{proxy_worker_name}} { public: {{proxy_worker_name}}() - : ipa_(nullptr), exit_(false) {} + : ipa_(nullptr), exit_(false) + { + controlSerializer_ = new ControlSerializer(ControlSerializer::Seeds::ODD); + } - ~{{proxy_worker_name}}() {} + ~{{proxy_worker_name}}() + { + delete controlSerializer_; + } void readyRead() { @@ -81,7 +87,7 @@ public: case {{cmd_enum_name}}::{{method.mojom_name|cap}}: { {%- if method.mojom_name == "configure" %} - controlSerializer_.reset(); + controlSerializer_->reset(); {%- endif %} {{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}} {% for param in method|method_param_outputs %} @@ -180,7 +186,7 @@ private: {{interface_name}} *ipa_; IPCUnixSocket socket_; - ControlSerializer controlSerializer_; + ControlSerializer *controlSerializer_; bool exit_; }; diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl index ebcd2aaaafae..d78e5c53566f 100644 --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl @@ -60,7 +60,7 @@ std::tie({{param.mojom_name}}Buf, std::ignore) = {%- endif %} IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}} -{{- ", &controlSerializer_" if param|needs_control_serializer -}} +{{- ", controlSerializer_" if param|needs_control_serializer -}} ); {%- endfor %} @@ -119,7 +119,7 @@ {%- endif -%} {{- "," if param|needs_control_serializer}} {%- if param|needs_control_serializer %} - &controlSerializer_ + controlSerializer_ {%- endif -%} ); {%- endmacro -%}