From patchwork Tue Sep 7 11:10:34 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 13713 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 E0D1CBE175 for ; Tue, 7 Sep 2021 11:09:57 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id ADB3569170; Tue, 7 Sep 2021 13:09:57 +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 12C7260251 for ; Tue, 7 Sep 2021 13:09:56 +0200 (CEST) Received: (Authenticated sender: jacopo@jmondi.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id DCFE360005; Tue, 7 Sep 2021 11:09:54 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org Date: Tue, 7 Sep 2021 13:10:34 +0200 Message-Id: <20210907111038.739104-2-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 1/5] libcamera: ipu3: Drop entityControls map 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" The IPA::configure() function has an IPAConfigInfo parameters which contains a map of numerical indexes to ControlInfoMap instances. This is a leftover of the old IPA protocol, where it was not possible to specify a rich interface as it is possible today and each entity ControlInfoMap was indexed by a numerical id and stored in a map. Now that the IPA interface allows to specify parameters by name, drop the map and send the sensor's control info map only. If we'll need more ControlInfoMap to be shared with the IPA, a new parameter can be added to IPAConfigInfo. Signed-off-by: Jacopo Mondi Reviewed-by: Umang Jain Reviewed-by: Paul Elder Reviewed-by: Laurent Pinchart Reviewed-by: Kieran Bingham --- include/libcamera/ipa/ipu3.mojom | 2 +- src/ipa/ipu3/ipu3.cpp | 6 +++--- src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom index d561c2244907..2045ce909a88 100644 --- a/include/libcamera/ipa/ipu3.mojom +++ b/include/libcamera/ipa/ipu3.mojom @@ -32,7 +32,7 @@ struct IPU3Action { struct IPAConfigInfo { libcamera.IPACameraSensorInfo sensorInfo; - map entityControls; + libcamera.ControlInfoMap sensorControls; libcamera.Size bdsOutputSize; libcamera.Size iif; }; diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index c925cf642611..2229bf4fc84e 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize) int IPAIPU3::configure(const IPAConfigInfo &configInfo) { - if (configInfo.entityControls.empty()) { - LOG(IPAIPU3, Error) << "No controls provided"; + if (configInfo.sensorControls.empty()) { + LOG(IPAIPU3, Error) << "No sensor controls provided"; return -ENODATA; } sensorInfo_ = configInfo.sensorInfo; - ctrls_ = configInfo.entityControls.at(0); + ctrls_ = configInfo.sensorControls; const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); if (itExp == ctrls_.end()) { diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index c287bf86e79a..92e869257e53 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) } ipa::ipu3::IPAConfigInfo configInfo; - configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls()); + configInfo.sensorControls = data->cio2_.sensor()->controls(); configInfo.sensorInfo = sensorInfo; configInfo.bdsOutputSize = config->imguConfig().bds; configInfo.iif = config->imguConfig().iif; From patchwork Tue Sep 7 11:10:35 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 13714 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 12DA5BE175 for ; Tue, 7 Sep 2021 11:10:00 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id D389869174; Tue, 7 Sep 2021 13:09:59 +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 6D67060252 for ; Tue, 7 Sep 2021 13:09:57 +0200 (CEST) Received: (Authenticated sender: jacopo@jmondi.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 3721D60009; Tue, 7 Sep 2021 11:09:56 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org Date: Tue, 7 Sep 2021 13:10:35 +0200 Message-Id: <20210907111038.739104-3-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 2/5] ipa: proxy_worker: Reset ControlSerializer on worker 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" When running the IPA in isolated mode, each side of the IPC boundary has an instance of the ControlSerializer class which is used to serializer/deserialize controls before transmitting them on the wire. The IPAProxyWorker, which creates and manages the process the IPA runs in, does not reset its ControlSerializer upon an IPA::configure() call, while the IPAProxy does, effectively creating a misalignment between the two sides of the fence. This obviously creates issues as one side of the IPC runs with a populated and possibly stale cache of ControlInfoMap references, while the other side gets reset every time a new configuration is applied to the Camera. Fix that by resetting the IPAProxyWorker ControlSerializer on an IPA configure() call. This change fixes an issue which is easily triggered by running two consecutive capture sessions with the IPA running in isolated mode: ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism") Signed-off-by: Jacopo Mondi Reviewed-by: Umang Jain Reviewed-by: Paul Elder Reviewed-by: Laurent Pinchart Reviewed-by: Kieran Bingham --- .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 4 ++++ 1 file changed, 4 insertions(+) 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 c54ecdb90a1a..c5e51532db53 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 @@ -79,6 +79,10 @@ public: {% for method in interface_main.methods %} case {{cmd_enum_name}}::{{method.mojom_name|cap}}: { + +{%- if method.mojom_name == "configure" %} + 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 %} {{param|name}} {{param.mojom_name}}; From patchwork Tue Sep 7 11:10:36 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 13715 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 60AA6BEFBE for ; Tue, 7 Sep 2021 11:10:00 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 0950C69177; Tue, 7 Sep 2021 13:10:00 +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 7A43A60251 for ; Tue, 7 Sep 2021 13:09:58 +0200 (CEST) Received: (Authenticated sender: jacopo@jmondi.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 925AB60002; Tue, 7 Sep 2021 11:09:57 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org Date: Tue, 7 Sep 2021 13:10:36 +0200 Message-Id: <20210907111038.739104-4-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 3/5] libcamera: control_serializer: Use the right idmap 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" When a ControlList is deserialized, the code searches for a valid ControlInfoMap in the local cache and use its id map to initialize the list. If no valid ControlInfoMap is found, as it's usually the case for lists transporting libcamera controls and properties, the globally defined controls::controls id map is used unconditionally. This breaks the deserialization of libcamera properties, for which a wrong idmap is used at construction time. As the serialization header now transports an id_map_type field, store the idmap type at serialization time, and re-use it at deserialization time to identify the correct id map. Also make the validation stricter by imposing to list of V4L2 controls to have an associated ControlInfoMap available, as there is no globally defined idmap for such controls. To be able to retrieve the idmap associated with a ControlList, add an accessor function to the ControlList class. It might be worth in future using a ControlInfoMap to initialize the deserialized ControlList to implement controls validation against their limit. As such validation is not implemented at the moment, maintain the current behaviour and initialize the control list with an idmap. Signed-off-by: Jacopo Mondi Reviewed-by: Paul Elder Reviewed-by: Kieran Bingham --- include/libcamera/controls.h | 1 + src/libcamera/control_serializer.cpp | 51 +++++++++++++++++++++++----- src/libcamera/controls.cpp | 8 +++++ 3 files changed, 51 insertions(+), 9 deletions(-) diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 6668e4bb1053..8e5bc8b70b94 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -407,6 +407,7 @@ public: void set(unsigned int id, const ControlValue &value); const ControlInfoMap *infoMap() const { return infoMap_; } + const ControlIdMap *idMap() const { return idmap_; } private: const ControlValue *find(unsigned int id) const; diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index 27360526f9eb..d42840cddecb 100644 --- a/src/libcamera/control_serializer.cpp +++ b/src/libcamera/control_serializer.cpp @@ -275,6 +275,15 @@ int ControlSerializer::serialize(const ControlList &list, infoMapHandle = 0; } + const ControlIdMap *idmap = list.idMap(); + enum ipa_controls_id_map_type idMapType; + if (idmap == &controls::controls) + idMapType = IPA_CONTROL_ID_MAP_CONTROLS; + else if (idmap == &properties::properties) + idMapType = IPA_CONTROL_ID_MAP_PROPERTIES; + else + idMapType = IPA_CONTROL_ID_MAP_V4L2; + size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry); size_t valuesSize = 0; for (const auto &ctrl : list) @@ -287,6 +296,7 @@ int ControlSerializer::serialize(const ControlList &list, hdr.entries = list.size(); hdr.size = sizeof(hdr) + entriesSize + valuesSize; hdr.data_offset = sizeof(hdr) + entriesSize; + hdr.id_map_type = idMapType; buffer.write(&hdr); @@ -496,13 +506,15 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer } /* - * Retrieve the ControlInfoMap associated with the ControlList based on - * its ID. The mapping between infoMap and ID is set up when serializing - * or deserializing ControlInfoMap. If no mapping is found (which is - * currently the case for ControlList related to libcamera controls), - * use the global control::control idmap. + * Retrieve the ControlIdMap associated with the ControlList. + * + * The idmap is either retrieved from the list's ControlInfoMap when + * a valid handle has been initialized at serialization time, or by + * using the header's id_map_type field for lists that refer to the + * globally defined libcamera controls and properties, for which no + * ControlInfoMap is available. */ - const ControlInfoMap *infoMap; + const ControlIdMap *idMap; if (hdr->handle) { auto iter = std::find_if(infoMapHandles_.begin(), infoMapHandles_.end(), [&](decltype(infoMapHandles_)::value_type &entry) { @@ -514,12 +526,33 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer return {}; } - infoMap = iter->first; + const ControlInfoMap *infoMap = iter->first; + idMap = &infoMap->idmap(); + ASSERT(idMap); } else { - infoMap = nullptr; + switch (hdr->id_map_type) { + case IPA_CONTROL_ID_MAP_CONTROLS: + idMap = &controls::controls; + break; + + case IPA_CONTROL_ID_MAP_PROPERTIES: + idMap = &properties::properties; + break; + + case IPA_CONTROL_ID_MAP_V4L2: + LOG(Serializer, Fatal) + << "A list of V4L2 controls requires an ControlInfoMap"; + return {}; + } } - ControlList ctrls(infoMap ? infoMap->idmap() : controls::controls); + /* + * \todo When available, initialize the list with the ControlInfoMap + * so that controls can be validated against their limits. + * Currently no validation is performed, so it's fine relying on the + * idmap only. + */ + ControlList ctrls(*idMap); for (unsigned int i = 0; i < hdr->entries; ++i) { const struct ipa_control_value_entry *entry = diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 5c05ba4a7cd0..96625edb1380 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -1038,6 +1038,14 @@ void ControlList::set(unsigned int id, const ControlValue &value) * associated ControlInfoMap, nullptr is returned in that case. */ +/** + * \fn ControlList::idMap() + * \brief Retrieve the ControlId map used to construct the ControlList + * \return The ControlId map used to construct the ControlList. ControlsList + * instances constructed with ControlList() have no associated idmap, nullptr is + * returned in that case. + */ + const ControlValue *ControlList::find(unsigned int id) const { const auto iter = controls_.find(id); From patchwork Tue Sep 7 11:10:37 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jacopo Mondi X-Patchwork-Id: 13716 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 7363FBE175 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 400EA69179; 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 A62366916C for ; Tue, 7 Sep 2021 13:09:59 +0200 (CEST) Received: (Authenticated sender: jacopo@jmondi.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id A199260009; Tue, 7 Sep 2021 11:09:58 +0000 (UTC) From: Jacopo Mondi To: libcamera-devel@lists.libcamera.org Date: Tue, 7 Sep 2021 13:10:37 +0200 Message-Id: <20210907111038.739104-5-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 4/5] libcamera: control_serializer: Serialize info::def() 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" The ControlInfo class was originally designed to only transport the control's minimum and maximum values which represent the control's valid limits. Later the default value of the control has been added to the ControlInfo class, but the control serializer implementation has not been updated accordingly. This causes issues in IPA modules making use of ControlInfo::def() as, when running in isolation, they would receive 0. Fix that by serializing and deserializing the additional ControlValue and update the protocol description accordingly. Signed-off-by: Jacopo Mondi Reviewed-by: Kieran Bingham Reviewed-by: Paul Elder Reviewed-by: Laurent Pinchart --- src/libcamera/control_serializer.cpp | 6 ++++-- src/libcamera/ipa_controls.cpp | 14 ++++++++------ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index d42840cddecb..f1245ea620ab 100644 --- a/src/libcamera/control_serializer.cpp +++ b/src/libcamera/control_serializer.cpp @@ -105,7 +105,7 @@ size_t ControlSerializer::binarySize(const ControlValue &value) size_t ControlSerializer::binarySize(const ControlInfo &info) { - return binarySize(info.min()) + binarySize(info.max()); + return binarySize(info.min()) + binarySize(info.max()) + binarySize(info.def()); } /** @@ -158,6 +158,7 @@ void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer) { store(info.min(), buffer); store(info.max(), buffer); + store(info.def(), buffer); } /** @@ -346,8 +347,9 @@ ControlInfo ControlSerializer::loadControlInfo(ControlType type, ControlValue min = loadControlValue(type, b); ControlValue max = loadControlValue(type, b); + ControlValue def = loadControlValue(type, b); - return ControlInfo(min, max); + return ControlInfo(min, max, def); } /** diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp index fb98cda30ede..c3489bbff646 100644 --- a/src/libcamera/ipa_controls.cpp +++ b/src/libcamera/ipa_controls.cpp @@ -108,17 +108,19 @@ * +-------------------------+ . * / | ... | | entry[n].offset * | +-------------------------+ <-----ยด - * Data | | minimum value (#n) | \ - * section | +-------------------------+ | Entry #n - * | | maximum value (#n) | / + * | | minimum value (#n) | \ + * Data | +-------------------------+ | + * section | | maximum value (#n) | | Entry #n + * | +-------------------------+ | + * | | default value (#n) | / * | +-------------------------+ * \ | ... | * +-------------------------+ * ~~~~ * - * The minimum and maximum value are stored in the platform's native data - * format. The ipa_control_info_entry::offset field stores the offset from the - * beginning of the data section to the info data. + * The minimum, maximum and default values are stored in the platform's native + * data format. The ipa_control_info_entry::offset field stores the offset from + * the beginning of the data section to the info data. * * Info data in the data section shall be stored in the same order as the * entries array, shall be aligned to a multiple of 8 bytes, and shall be 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 -%}