From patchwork Fri Jul 24 21:05:44 2026 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Magdum X-Patchwork-Id: 27506 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 E6492BE080 for ; Fri, 24 Jul 2026 21:05:48 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id A1E1C67F22; Fri, 24 Jul 2026 23:05:48 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="IWP+VZ6c"; dkim-atps=neutral Received: from mail-wm1-x334.google.com (mail-wm1-x334.google.com [IPv6:2a00:1450:4864:20::334]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id B2F3D67F22 for ; Fri, 24 Jul 2026 23:05:47 +0200 (CEST) Received: by mail-wm1-x334.google.com with SMTP id 5b1f17b1804b1-4954afac04bso9687705e9.0 for ; Fri, 24 Jul 2026 14:05:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1784927147; x=1785531947; darn=lists.libcamera.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to:content-type; bh=1pm0zdejAZbdW766O665QqntJdFakEns0w0WWONWTzQ=; b=IWP+VZ6czoxEIYLMr5bM5+/i46ZWl1rQDMn1bDCAOtGIsSo5n8ePCj2ZTsWwyCVs7p X6VVh3PqerQA/FAEhbxJ0TSwUpvSnExWQhlGbfqtTQM16d88qhW0/LGz+48B0/Fups5y ZdN5LPetrREsCgr9Kl5vqyjEz0KzuCXHUuq5MXoIo7QC/meQ6IU+/htzK5iB+XLlfObX 8pbbKFGsCXf6zXYX8PlI7VACu7rpqgTu3cOUEUu1vJEKaXUGDtCHGPj9C1gc4Fzvpr3f 0qAsvtamKrg3lGB55/KmwNarRkW8ZaPzGLqnmlwK+qTsPH2AO55UTCg4fsozztoKHdjl sXKw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784927147; x=1785531947; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to:content-type; bh=1pm0zdejAZbdW766O665QqntJdFakEns0w0WWONWTzQ=; b=qB4PB0wE0UYjAF3MvrnYTgI6K15QCqPfs7iwfPnr5FguQHwahLX9wYm51utSZcYLzy /sNnwNmR1KLbesOFcgbvyLv4oPRAtM7nWlvJ55Vm7icfxAIavhHDLLsqE2AUPMKXfh/G X8I6CBnprvjrCP4xMkZzq7VQiThw5kcyt5w/WmEc/aoWw3xNuvaeMvcqAOyFZfFANDIJ 4gfFsoiXS5NOXl6f0G4XOuuQn6XR5BI6YtJNptqxHb5FrXrmZATyJxQnP8sxIlb3Noj2 f9bgGbbnEOJ1Ud9PbUgJSEJwz6i7t78dEFf2rBO3OgAb3KXTzQXKuT2ppb6HteIrIqCa fQkw== X-Gm-Message-State: AOJu0Ywx38v+AOLwOY3lA2XS1PlXe6yVH634sHane+sw2xaL6DwT0ODX jHBG5dzGBkdNDdm1o/78MOAE/lM6RsBLHdKl3QvRLui0WAPwBIDOlaX8PyTxQ02Y3mM= X-Gm-Gg: AR+sD131K+LcZ+pJDB325hLqfHG2BO4V00KLgLof8rclmrVJl6dM6Q9FMs7tF4Q5s/a rvuPAj2PEAk0LryfpJgAjr9QU1gXmEgn1WEWUZ6p+cQS8q6wf/tgLKww1wI8IYNe2qsGeP545Ln Am0eewo+zG0mbh3jfrXL7uas1+UBrDjnB5AWv4N4utVErMnThCMQ29zrx1sN7s0GKOy3wazDL5V iPARLUwiALJNdwmtoEwQhhwJ2awLscbRa9hFDJOXd9BTwsnq+i6wpps1hQEWfaa6/4RyQoxbE3j OlDUKXYvJKIierxZZm63g5JayrpoNJLX+SC5s88E8zlbu3tLdtpSEf9Ux7gNPA1+EZ03knrP747 gfFDUDZWQShYvSVOpi4YbjqXj9IW3dOyyCf4EAuqsnbuexwkOUnFWSNbkIeLBdX63SawxZ5DRkX WJZ9RbO6/JBR1jXY8vmZ/wvUiCuvnfMOYuJ6WjhkqCPSwQed73MqJI X-Received: by 2002:a05:600c:1387:b0:495:689a:b3ff with SMTP id 5b1f17b1804b1-496b56b212fmr122625e9.8.1784927147059; Fri, 24 Jul 2026 14:05:47 -0700 (PDT) Received: from magdum-System-Product-Name.vodafone.ultrahub ([2a02:810d:4b14:4600:4f39:30e9:8fd8:737c]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4957b816ef8sm64803155e9.1.2026.07.24.14.05.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 Jul 2026 14:05:46 -0700 (PDT) From: Magdum To: libcamera-devel@lists.libcamera.org Cc: Magdum Subject: [PATCH v2] libcamera: Serialize local control names in IPA format v3 Date: Fri, 24 Jul 2026 23:05:44 +0200 Message-ID: <20260724210544.8012-1-magdum.foss@gmail.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20260723174644.6580-2-magdum.foss@gmail.com> References: <20260723174644.6580-2-magdum.foss@gmail.com> MIME-Version: 1.0 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" Bump IPA controls serialization format to v3 and add name_len to ControlInfoMap entries. Serialize ontrol names for local (V4L2-style) id maps to preserve names across IPC. Extend serializer tests with V4L2-like name round-trip coverage and malformed payload rejection for oversized length, missing terminator, and too-long serialize input. Signed-off-by: Magdum --- include/libcamera/ipa/ipa_controls.h | 8 +- src/libcamera/control_serializer.cpp | 141 ++++++++++++++----- src/libcamera/ipa_controls.cpp | 10 +- test/serialization/control_serialization.cpp | 109 ++++++++++++++ 4 files changed, 224 insertions(+), 44 deletions(-) diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h index 6af962ff3..cf4bbe3cf 100644 --- a/include/libcamera/ipa/ipa_controls.h +++ b/include/libcamera/ipa/ipa_controls.h @@ -15,7 +15,7 @@ namespace libcamera { extern "C" { #endif -#define IPA_CONTROLS_FORMAT_VERSION 2 +#define IPA_CONTROLS_FORMAT_VERSION 3 enum ipa_controls_id_map_type { IPA_CONTROL_ID_MAP_CONTROLS, @@ -50,7 +50,11 @@ struct ipa_control_info_entry { uint32_t id; uint32_t type; uint8_t direction; - uint8_t padding[7]; + uint8_t padding[3]; + /* Length of the control name. */ + uint32_t name_len; + /* Offset of the control name in the values section. */ + uint32_t name_offset; struct ipa_control_value_entry min; struct ipa_control_value_entry max; struct ipa_control_value_entry def; diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index c0285cc6c..a420d4700 100644 --- a/src/libcamera/control_serializer.cpp +++ b/src/libcamera/control_serializer.cpp @@ -31,6 +31,33 @@ namespace libcamera { LOG_DEFINE_CATEGORY(Serializer) +namespace { + +constexpr uint32_t kMaxControlNameLength = 1024; + +enum ipa_controls_id_map_type idMapTypeFor(const ControlIdMap &idmap) +{ + if (&idmap == &controls::controls) + return IPA_CONTROL_ID_MAP_CONTROLS; + if (&idmap == &properties::properties) + return IPA_CONTROL_ID_MAP_PROPERTIES; + + return IPA_CONTROL_ID_MAP_V4L2; +} + +bool idMapRequiresLocalIds(enum ipa_controls_id_map_type idMapType) +{ + return idMapType == IPA_CONTROL_ID_MAP_V4L2; +} + +size_t serializedControlNameSize(const ControlId *id, + enum ipa_controls_id_map_type idMapType) +{ + return idMapRequiresLocalIds(idMapType) ? id->name().size() + 1 : 1; +} + +} /* namespace */ + /** * \class ControlSerializer * \brief Serializer and deserializer for control-related classes @@ -165,10 +192,14 @@ size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap) { size_t size = sizeof(struct ipa_controls_header) + infoMap.size() * sizeof(struct ipa_control_info_entry); + enum ipa_controls_id_map_type idMapType = idMapTypeFor(infoMap.idmap()); - for (const auto &ctrl : infoMap) + for (const auto &ctrl : infoMap) { size += binarySize(ctrl.second); + size += serializedControlNameSize(ctrl.first, idMapType); + } + return size; } @@ -183,8 +214,7 @@ size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap) */ size_t ControlSerializer::binarySize(const ControlList &list) { - size_t size = sizeof(struct ipa_controls_header) - + list.size() * sizeof(struct ipa_control_list_entry); + size_t size = sizeof(struct ipa_controls_header) + list.size() * sizeof(struct ipa_control_list_entry); for (const auto &ctrl : list) size += binarySize(ctrl.second); @@ -231,24 +261,21 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, return 0; } + enum ipa_controls_id_map_type idMapType = idMapTypeFor(infoMap.idmap()); + /* Compute entries and data required sizes. */ size_t entriesSize = infoMap.size() * sizeof(struct ipa_control_info_entry); size_t valuesSize = 0; - for (const auto &ctrl : infoMap) + for (const auto &ctrl : infoMap) { valuesSize += binarySize(ctrl.second); - - const ControlIdMap *idmap = &infoMap.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; + valuesSize += idMapRequiresLocalIds(idMapType) + ? ctrl.first->name().size() + : 0; + } /* Prepare the packet header. */ - struct ipa_controls_header hdr; + struct ipa_controls_header hdr = {}; hdr.version = IPA_CONTROLS_FORMAT_VERSION; hdr.handle = serial_; hdr.entries = infoMap.size(); @@ -267,21 +294,29 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, */ serial_ += 2; - /* - * Serialize all entries. - * \todo Serialize the control name too - */ + /* Serialize all entries. */ ByteStreamBuffer entries = buffer.carveOut(entriesSize); ByteStreamBuffer values = buffer.carveOut(valuesSize); + static const std::string emptyName; for (const auto &ctrl : infoMap) { const ControlId *id = ctrl.first; const ControlInfo &info = ctrl.second; + const std::string &name = idMapRequiresLocalIds(idMapType) + ? id->name() + : emptyName; - struct ipa_control_info_entry entry; + if (name.size() > kMaxControlNameLength) { + LOG(Serializer, Error) + << "Control name too long: " << name.size(); + return -EINVAL; + } + + struct ipa_control_info_entry entry = {}; entry.id = id->id(); entry.type = id->type(); entry.direction = static_cast(id->direction()); + entry.name_len = static_cast(name.size()); populateControlValueEntry(entry.min, info.min(), values.offset()); store(info.min(), values); @@ -292,6 +327,11 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, populateControlValueEntry(entry.def, info.def(), values.offset()); store(info.def(), values); + entry.name_offset = values.offset(); + values.write(Span( + reinterpret_cast(name.c_str()), + name.size())); + entries.write(&entry); } @@ -355,7 +395,7 @@ int ControlSerializer::serialize(const ControlList &list, valuesSize += binarySize(ctrl.second); /* Prepare the packet header. */ - struct ipa_controls_header hdr; + struct ipa_controls_header hdr = {}; hdr.version = IPA_CONTROLS_FORMAT_VERSION; hdr.handle = infoMapHandle; hdr.entries = list.size(); @@ -485,25 +525,6 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer & ControlType type = static_cast(entry->type); - /* If we're using a local id map, populate it. */ - if (localIdMap) { - ControlId::DirectionFlags flags{ - static_cast(entry->direction) - }; - - /** - * \todo Find a way to preserve the control name for - * debugging purpose. - */ - controlIds_.emplace_back(std::make_unique(entry->id, - "", "local", type, - flags)); - (*localIdMap)[entry->id] = controlIds_.back().get(); - } - - const ControlId *controlId = idMap->at(entry->id); - ASSERT(controlId); - const ipa_control_value_entry &min_entry = entry->min; const ipa_control_value_entry &max_entry = entry->max; const ipa_control_value_entry &def_entry = entry->def; @@ -538,6 +559,48 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer & loadControlValue(values, static_cast(def_entry.type), def_entry.is_array, def_entry.count); + /* + * Deserialize the null-terminated control name from the values + * section. Reject unreasonably long names to guard against + * malformed packets. + */ + if (entry->name_len > kMaxControlNameLength) { + LOG(Serializer, Error) + << "Control name too long: " << entry->name_len; + return {}; + } + + if (entry->name_offset != values.offset()) { + LOG(Serializer, Error) + << "Bad data, name offset mismatch (entry " + << i << ")"; + return {}; + } + + const auto *nameData = entry->name_len + ? values.read(entry->name_len) : nullptr; + if (entry->name_len && !nameData) { + LOG(Serializer, Error) << "Out of data reading control name"; + return {}; + } + + /* If we're using a local id map, populate it with the restored name. */ + if (localIdMap) { + std::string ctrlName(reinterpret_cast(nameData), + entry->name_len); + + ControlId::DirectionFlags flags{ + static_cast(entry->direction) + }; + + controlIds_.emplace_back(std::make_unique(entry->id, + ctrlName, "local", + type, flags)); + (*localIdMap)[entry->id] = controlIds_.back().get(); + } + + const ControlId *controlId = idMap->at(entry->id); + ASSERT(controlId); /* Create and store the ControlInfo. */ ctrls.emplace(controlId, ControlInfo(min, max, def)); diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp index 61af7433d..181317616 100644 --- a/src/libcamera/ipa_controls.cpp +++ b/src/libcamera/ipa_controls.cpp @@ -234,14 +234,18 @@ static_assert(sizeof(ipa_control_list_entry) == 20, * \var ipa_control_info_entry::id * The numerical ID of the control * \var ipa_control_info_entry::type - * The type of the control (defined by enum ControlType) - * info data (shall be a multiple of 8 bytes) + * The type of the control (defined by enum ControlType). + * The info data shall be a multiple of 8 bytes. * \var ipa_control_info_entry::direction * The directions in which the control is allowed to be sent. This is a flags * value, where 0x1 signifies input (as controls), and 0x2 signifies output (as * metadata). \sa ControlId::Direction * \var ipa_control_info_entry::padding * Padding bytes (shall be set to 0) + * \var ipa_control_info_entry::name_len + * Length of the control name + * \var ipa_control_info_entry::name_offset + * Offset of the control name in the values section * \var ipa_control_info_entry::min * The description of the serialized ControlValue (min) * \var ipa_control_info_entry::max @@ -250,7 +254,7 @@ static_assert(sizeof(ipa_control_list_entry) == 20, * The description of the serialized ControlValue (def) */ -static_assert(sizeof(ipa_control_info_entry) == 64, +static_assert(sizeof(ipa_control_info_entry) == 68, "Invalid ABI size change for struct ipa_control_info_entry"); } /* namespace libcamera */ diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp index 06c572b77..b1638db9f 100644 --- a/test/serialization/control_serialization.cpp +++ b/test/serialization/control_serialization.cpp @@ -11,6 +11,8 @@ #include #include +#include + #include "libcamera/internal/byte_stream_buffer.h" #include "libcamera/internal/control_serializer.h" @@ -169,6 +171,113 @@ protected: return TestFail; } + /* Build a local (V4L2-like) ControlInfoMap and verify name round-trip. */ + vector> v4l2ControlIds; + ControlIdMap v4l2IdMap; + constexpr uint32_t kV4L2TestControlId = 0x009a2001; + const string kV4L2ControlName = "V4L2_CID_TEST_GAIN"; + + v4l2ControlIds.emplace_back(std::make_unique( + kV4L2TestControlId, kV4L2ControlName, "v4l2", + ControlTypeInteger32, ControlId::Direction::In)); + v4l2IdMap.emplace(kV4L2TestControlId, v4l2ControlIds.back().get()); + + ControlInfoMap::Map v4l2Info; + v4l2Info.emplace(v4l2ControlIds.back().get(), + ControlInfo(ControlValue(int32_t{ 0 }), + ControlValue(int32_t{ 255 }), + ControlValue(int32_t{ 16 }))); + ControlInfoMap v4l2InfoMap(std::move(v4l2Info), v4l2IdMap); + + ControlSerializer v4l2Serializer(ControlSerializer::Role::Proxy); + ControlSerializer v4l2Deserializer(ControlSerializer::Role::Worker); + + size = v4l2Serializer.binarySize(v4l2InfoMap); + infoData.resize(size); + buffer = ByteStreamBuffer(infoData.data(), infoData.size()); + + ret = v4l2Serializer.serialize(v4l2InfoMap, buffer); + if (ret < 0 || buffer.overflow()) { + cerr << "Failed to serialize V4L2-like ControlInfoMap" << endl; + return TestFail; + } + + buffer = ByteStreamBuffer(const_cast(infoData.data()), + infoData.size()); + ControlInfoMap v4l2InfoMapDes = + v4l2Deserializer.deserialize(buffer); + if (v4l2InfoMapDes.empty()) { + cerr << "Failed to deserialize V4L2-like ControlInfoMap" << endl; + return TestFail; + } + + auto idIt = v4l2InfoMapDes.idmap().find(kV4L2TestControlId); + if (idIt == v4l2InfoMapDes.idmap().end()) { + cerr << "Deserialized V4L2-like id map misses test control" << endl; + return TestFail; + } + + if (idIt->second->name() != kV4L2ControlName) { + cerr << "Deserialized V4L2-like control name doesn't match" << endl; + return TestFail; + } + + /* Reject malformed packets with over-sized names. */ + vector badNameLenData = infoData; + auto *badNameLenHeader = + reinterpret_cast(badNameLenData.data()); + auto *badNameLenEntry = reinterpret_cast( + badNameLenData.data() + sizeof(*badNameLenHeader)); + badNameLenEntry->name_len = 2048; + + ControlSerializer badNameLenDeserializer(ControlSerializer::Role::Worker); + buffer = ByteStreamBuffer(const_cast(badNameLenData.data()), + badNameLenData.size()); + if (!badNameLenDeserializer.deserialize(buffer).empty()) { + cerr << "Oversized control name should be rejected" << endl; + return TestFail; + } + + /* Reject malformed packets with non-null-terminated names. */ + vector badTermData = infoData; + badTermData.back() = 'X'; + + ControlSerializer badTermDeserializer(ControlSerializer::Role::Worker); + buffer = ByteStreamBuffer(const_cast(badTermData.data()), + badTermData.size()); + if (!badTermDeserializer.deserialize(buffer).empty()) { + cerr << "Control name without null terminator should be rejected" << endl; + return TestFail; + } + + /* Reject too-long names at serialization time. */ + vector> longNameControlIds; + ControlIdMap longNameIdMap; + string longName(1025, 'n'); + + longNameControlIds.emplace_back(std::make_unique( + 0x009a2002, longName, "v4l2", ControlTypeInteger32, + ControlId::Direction::In)); + longNameIdMap.emplace(0x009a2002, longNameControlIds.back().get()); + + ControlInfoMap::Map longNameInfo; + longNameInfo.emplace(longNameControlIds.back().get(), + ControlInfo(ControlValue(int32_t{ 0 }), + ControlValue(int32_t{ 255 }), + ControlValue(int32_t{ 16 }))); + ControlInfoMap longNameInfoMap(std::move(longNameInfo), longNameIdMap); + + ControlSerializer longNameSerializer(ControlSerializer::Role::Proxy); + size = longNameSerializer.binarySize(longNameInfoMap); + infoData.resize(size); + buffer = ByteStreamBuffer(infoData.data(), infoData.size()); + + ret = longNameSerializer.serialize(longNameInfoMap, buffer); + if (ret != -EINVAL) { + cerr << "Too-long control name should fail serialization" << endl; + return TestFail; + } + return TestPass; } };