From patchwork Mon Nov 17 08:08:14 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Paul Elder X-Patchwork-Id: 25076 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 29D0EC3241 for ; Mon, 17 Nov 2025 08:08:47 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 9FCB260AA2; Mon, 17 Nov 2025 09:08:46 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="ot0GckDo"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 377ED606A0 for ; Mon, 17 Nov 2025 09:08:43 +0100 (CET) Received: from neptunite.hamster-moth.ts.net (unknown [IPv6:2404:7a81:160:2100:e0cd:14aa:3681:9fc1]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 8AF01142; Mon, 17 Nov 2025 09:06:38 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1763366800; bh=UmWQJhKI+5RhGQWCwZlNQbrNN9Ui3ZoMj6xwAGhvwFk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=ot0GckDo/zrHUvtxOG0KKkSZ31c7A0YGLpAF8VDFIrhj65HYnR7wHCFpqJdntNk34 qTF91g7Si+TAut0Br18enZ6ae8vx2/iHyDWH1McsS9/XwntFFbuphNs9Iv4yGviwUo 8OjATuePiOsDuyQzwly99UWY3TKl8MXf1zhxQn5E= From: Paul Elder To: libcamera-devel@lists.libcamera.org Cc: Paul Elder , =?utf-8?q?Barnab=C3=A1s_P?= =?utf-8?b?xZFjemU=?= , Daniel Scally Subject: [PATCH v6 1/2] libcamera: control_serializer: Add array info to serialized ControlValue Date: Mon, 17 Nov 2025 17:08:14 +0900 Message-ID: <20251117080818.3009835-2-paul.elder@ideasonboard.com> X-Mailer: git-send-email 2.47.2 In-Reply-To: <20251117080818.3009835-1-paul.elder@ideasonboard.com> References: <20251117080818.3009835-1-paul.elder@ideasonboard.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" Array controls (eg. ColourCorrectionMatrix, FrameDurationLimits, ColourGains) are serialized properly by the ControlSerializer, but are not deserialized properly. This is because their arrayness and size are not considered during deserialization. Fix this by adding arrayness and size to the serialized form of all ControlValues. This is achieved by fully serializing the min/max/def ControlValue's metadata associated with each ControlInfo entry in the ControlInfoMap. While at it, clean up the serialization format of ControlValues and ControlLists: - ControlValue's id is only used by ControlList, so add a new struct for ControlList entries to contain it, and remove id from ControlValue - Remove offset from ControlInfo's entry, as it is no longer needed, since the serialized data of a ControlInfo has now been converted to simply three serialized ControlValues - Remove the type from the serialized data of ControlValue, as it is already in the metadata entry The issue regarding array controls was not noticed before because the default value of the ControlInfo of other array controls had been set to scalar values similar to how min/max are set, and ColourCorrectionMatrix was the first control to properly define a non-scalar default value. Bug: https://bugs.libcamera.org/show_bug.cgi?id=285 Signed-off-by: Paul Elder Tested-by: Barnabás Pőcze # rkisp1 Reviewed-by: Daniel Scally --- Changes in v6: - move ipa_control_value_entry into ipa_control_info_entry - update documentation of the serialized format Changes in v5: - re/move reserved fields in ipa_controls.h - remote default parameters from loadControlValue - fix binarySize of ControlValue Changes in v4: - remove id from ipa_control_value_entry, as it is only used by ControlList - move id into a new struct ipa_control_list_entry - remove offset from ipa_control_info_entry, as it is no longer used - since ControlInfo is no longer serialized, and instead it is three ControlValues serialized individually - remove type from the serialized data portion of ControlValue, as the information is in the metadata entries - error-check the offsets when deserializing ControlInfoMap Changes in v3: - instead of adding an extra header to store isArray and numElements like in v2, just reuse struct ipa_control_value_entry, and add these entries to the serialized form of ControlInfoMap to store information for each of the three ControlValues that make of min and max and def in ControlInfoMap Changes in v2: - make it so that the *serialized form* of ControlValue includes arrayness and size instead - Compared to v1, this ties the arrayness and size information to ControlValue instead of ControlId, which as a side effect allows us to support scalar and array min/max values, not just def values. This also gives us support for variable-length arrays --- .../libcamera/internal/control_serializer.h | 8 +- include/libcamera/ipa/ipa_controls.h | 16 +- src/libcamera/control_serializer.cpp | 106 +++++++---- src/libcamera/ipa_controls.cpp | 178 ++++++++++-------- 4 files changed, 182 insertions(+), 126 deletions(-) diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h index 8a63ae44a13e..307ecba572fc 100644 --- a/include/libcamera/internal/control_serializer.h +++ b/include/libcamera/internal/control_serializer.h @@ -47,9 +47,13 @@ private: static void store(const ControlValue &value, ByteStreamBuffer &buffer); static void store(const ControlInfo &info, ByteStreamBuffer &buffer); + void populateControlValueEntry(struct ipa_control_value_entry &entry, + const ControlValue &value, + uint32_t offset); + ControlValue loadControlValue(ByteStreamBuffer &buffer, - bool isArray = false, unsigned int count = 1); - ControlInfo loadControlInfo(ByteStreamBuffer &buffer); + ControlType type, + bool isArray, unsigned int count); unsigned int serial_; unsigned int serialSeed_; diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h index 980668c86bcc..6af962ff325e 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 1 +#define IPA_CONTROLS_FORMAT_VERSION 2 enum ipa_controls_id_map_type { IPA_CONTROL_ID_MAP_CONTROLS, @@ -34,20 +34,26 @@ struct ipa_controls_header { }; struct ipa_control_value_entry { - uint32_t id; uint8_t type; uint8_t is_array; uint16_t count; uint32_t offset; - uint32_t padding[1]; + uint32_t reserved[2]; +}; + +struct ipa_control_list_entry { + uint32_t id; + struct ipa_control_value_entry value; }; struct ipa_control_info_entry { uint32_t id; uint32_t type; - uint32_t offset; uint8_t direction; - uint8_t padding[3]; + uint8_t padding[7]; + struct ipa_control_value_entry min; + struct ipa_control_value_entry max; + struct ipa_control_value_entry def; }; #ifdef __cplusplus diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index 050f8512bd52..843e2772f848 100644 --- a/src/libcamera/control_serializer.cpp +++ b/src/libcamera/control_serializer.cpp @@ -144,7 +144,7 @@ void ControlSerializer::reset() size_t ControlSerializer::binarySize(const ControlValue &value) { - return sizeof(ControlType) + value.data().size_bytes(); + return value.data().size_bytes(); } size_t ControlSerializer::binarySize(const ControlInfo &info) @@ -164,7 +164,8 @@ size_t ControlSerializer::binarySize(const ControlInfo &info) size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap) { size_t size = sizeof(struct ipa_controls_header) - + infoMap.size() * sizeof(struct ipa_control_info_entry); + + infoMap.size() * (sizeof(struct ipa_control_info_entry) + + 3 * sizeof(struct ipa_control_value_entry)); for (const auto &ctrl : infoMap) size += binarySize(ctrl.second); @@ -184,7 +185,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_value_entry); + + list.size() * sizeof(struct ipa_control_list_entry); for (const auto &ctrl : list) size += binarySize(ctrl.second); @@ -195,16 +196,17 @@ size_t ControlSerializer::binarySize(const ControlList &list) void ControlSerializer::store(const ControlValue &value, ByteStreamBuffer &buffer) { - const ControlType type = value.type(); - buffer.write(&type); buffer.write(value.data()); } -void ControlSerializer::store(const ControlInfo &info, ByteStreamBuffer &buffer) +void ControlSerializer::populateControlValueEntry(struct ipa_control_value_entry &entry, + const ControlValue &value, + uint32_t offset) { - store(info.min(), buffer); - store(info.max(), buffer); - store(info.def(), buffer); + entry.type = value.type(); + entry.is_array = value.isArray(); + entry.count = value.numElements(); + entry.offset = offset; } /** @@ -232,7 +234,8 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, /* Compute entries and data required sizes. */ size_t entriesSize = infoMap.size() - * sizeof(struct ipa_control_info_entry); + * (sizeof(struct ipa_control_info_entry) + + 3 * sizeof(struct ipa_control_value_entry)); size_t valuesSize = 0; for (const auto &ctrl : infoMap) valuesSize += binarySize(ctrl.second); @@ -280,11 +283,18 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, struct ipa_control_info_entry entry; entry.id = id->id(); entry.type = id->type(); - entry.offset = values.offset(); entry.direction = static_cast(id->direction()); - entries.write(&entry); - store(info, values); + populateControlValueEntry(entry.min, info.min(), values.offset()); + store(info.min(), values); + + populateControlValueEntry(entry.max, info.max(), values.offset()); + store(info.max(), values); + + populateControlValueEntry(entry.def, info.def(), values.offset()); + store(info.def(), values); + + entries.write(&entry); } if (buffer.overflow()) @@ -341,7 +351,7 @@ int ControlSerializer::serialize(const ControlList &list, else idMapType = IPA_CONTROL_ID_MAP_V4L2; - size_t entriesSize = list.size() * sizeof(struct ipa_control_value_entry); + size_t entriesSize = list.size() * sizeof(struct ipa_control_list_entry); size_t valuesSize = 0; for (const auto &ctrl : list) valuesSize += binarySize(ctrl.second); @@ -365,12 +375,9 @@ int ControlSerializer::serialize(const ControlList &list, unsigned int id = ctrl.first; const ControlValue &value = ctrl.second; - struct ipa_control_value_entry entry; + struct ipa_control_list_entry entry; entry.id = id; - entry.type = value.type(); - entry.is_array = value.isArray(); - entry.count = value.numElements(); - entry.offset = values.offset(); + populateControlValueEntry(entry.value, value, values.offset()); entries.write(&entry); store(value, values); @@ -383,12 +390,10 @@ int ControlSerializer::serialize(const ControlList &list, } ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer, + ControlType type, bool isArray, unsigned int count) { - ControlType type; - buffer.read(&type); - ControlValue value; value.reserve(type, isArray, count); @@ -397,15 +402,6 @@ ControlValue ControlSerializer::loadControlValue(ByteStreamBuffer &buffer, return value; } -ControlInfo ControlSerializer::loadControlInfo(ByteStreamBuffer &b) -{ - ControlValue min = loadControlValue(b); - ControlValue max = loadControlValue(b); - ControlValue def = loadControlValue(b); - - return ControlInfo(min, max, def); -} - /** * \fn template T ControlSerializer::deserialize(ByteStreamBuffer &buffer) * \brief Deserialize an object from a binary buffer @@ -483,8 +479,7 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer & ControlInfoMap::Map ctrls; for (unsigned int i = 0; i < hdr->entries; ++i) { - const struct ipa_control_info_entry *entry = - entries.read(); + const auto *entry = entries.read(); if (!entry) { LOG(Serializer, Error) << "Out of data"; return {}; @@ -511,15 +506,43 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer & const ControlId *controlId = idMap->at(entry->id); ASSERT(controlId); - if (entry->offset != values.offset()) { + 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; + + if (min_entry.offset != values.offset()) { LOG(Serializer, Error) - << "Bad data, entry offset mismatch (entry " + << "Bad data, entry offset mismatch (min entry " << i << ")"; return {}; } + ControlValue min = + loadControlValue(values, static_cast(min_entry.type), + min_entry.is_array, min_entry.count); + + if (max_entry.offset != values.offset()) { + LOG(Serializer, Error) + << "Bad data, entry offset mismatch (max entry " + << i << ")"; + return {}; + } + ControlValue max = + loadControlValue(values, static_cast(max_entry.type), + max_entry.is_array, max_entry.count); + + if (def_entry.offset != values.offset()) { + LOG(Serializer, Error) + << "Bad data, entry offset mismatch (def entry " + << i << ")"; + return {}; + } + ControlValue def = + loadControlValue(values, static_cast(def_entry.type), + def_entry.is_array, def_entry.count); + /* Create and store the ControlInfo. */ - ctrls.emplace(controlId, loadControlInfo(values)); + ctrls.emplace(controlId, ControlInfo(min, max, def)); } /* @@ -618,12 +641,12 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer ControlList ctrls(*idMap); for (unsigned int i = 0; i < hdr->entries; ++i) { - const struct ipa_control_value_entry *entry = - entries.read(); - if (!entry) { + auto *list_entry = entries.read(); + if (!list_entry) { LOG(Serializer, Error) << "Out of data"; return {}; } + const ipa_control_value_entry *entry = &list_entry->value; if (entry->offset != values.offset()) { LOG(Serializer, Error) @@ -632,8 +655,9 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer return {}; } - ctrls.set(entry->id, - loadControlValue(values, entry->is_array, entry->count)); + ctrls.set(list_entry->id, + loadControlValue(values, static_cast(entry->type), + entry->is_array, entry->count)); } return ctrls; diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp index 12d92ebe894d..61af7433da17 100644 --- a/src/libcamera/ipa_controls.cpp +++ b/src/libcamera/ipa_controls.cpp @@ -26,28 +26,28 @@ * The following diagram describes the layout of the ControlList packet. * * ~~~~ - * +-------------------------+ . . - * Header / | ipa_controls_header | | | - * | | | | | - * \ | | | | - * +-------------------------+ | | - * / | ipa_control_value_entry | | hdr.data_offset | - * | | #0 | | | - * Control | +-------------------------+ | | - * value | | ... | | | - * entries | +-------------------------+ | | - * | | ipa_control_value_entry | | hdr.size | - * \ | #hdr.entries - 1 | | | - * +-------------------------+ | | - * | empty space (optional) | | | - * +-------------------------+ <--´ . | - * / | ... | | entry[n].offset | - * Data | | ... | | | - * section | | value data for entry #n | <-----´ | - * \ | ... | | - * +-------------------------+ | - * | empty space (optional) | | - * +-------------------------+ <-------------------------´ + * +-------------------------+ . . + * Header / | ipa_controls_header | | | + * | | | | | + * \ | | | | + * +-------------------------+ | | + * / | ipa_control_list_entry | | hdr.data_offset | + * | | #0 | | | + * Control | +-------------------------+ | | + * value | | ... | | | + * entries | +-------------------------+ | | + * | | ipa_control_list_entry | | hdr.size | + * \ | #hdr.entries - 1 | | | + * +-------------------------+ | | + * | empty space (optional) | | | + * +-------------------------+ <--´ . | + * / | ... | | entry[n].value.offset | + * Data | | ... | | | + * section | | value data for entry #n | <-----´ | + * \ | ... | | + * +-------------------------+ | + * | empty space (optional) | | + * +-------------------------+ <-----------------------------´ * ~~~~ * * The packet header contains the size of the packet, the number of entries, and @@ -56,12 +56,14 @@ * offset ipa_controls_header::data_offset from the beginning of the packet, and * shall be aligned to a multiple of 8 bytes. * - * Entries are described by the ipa_control_value_entry structure. They contain - * the numerical ID of the control, its type, and the number of control values. + * Entries are described by the ipa_control_list_entry structure. They contain + * the numerical ID of the control and an ipa_control_value_entry structure, + * which contains the type and the number of control values. * - * The control values are stored in the data section in the platform's native - * format. The ipa_control_value_entry::offset field stores the offset from the - * beginning of the data section to the values. + * The control values are stored (as ipa_control_list_entry) in the data + * section in the platform's native format. The ipa_control_value_entry::offset + * field stores the offset from the beginning of the data section to the + * values. * * All control values in the data section shall be stored in the same order as * the respective control entries, shall be aligned to a multiple of 8 bytes, @@ -74,59 +76,65 @@ * The following diagram describes the layout of the ControlInfoMap packet. * * ~~~~ - * +-------------------------+ . . - * Header / | ipa_controls_header | | | - * | | | | | - * \ | | | | - * +-------------------------+ | | - * / | ipa_control_info_entry | | hdr.data_offset | - * | | #0 | | | - * Control | +-------------------------+ | | - * info | | ... | | | - * entries | +-------------------------+ | | - * | | ipa_control_info_entry | | hdr.size | - * \ | #hdr.entries - 1 | | | - * +-------------------------+ | | - * | empty space (optional) | | | - * +-------------------------+ <--´ . | - * / | ... | | entry[n].offset | - * Data | | ... | | | - * section | | info data for entry #n | <-----´ | - * \ | ... | | - * +-------------------------+ | - * | empty space (optional) | | - * +-------------------------+ <-------------------------´ + * +------------------------------+ . . + * Header / | ipa_controls_header | | | + * | | | | | + * \ | | | | + * +------------------------------+ | | + * / | ipa_control_info_entry | | hdr.data_offset | + * | | #0 | | | + * Control | +------------------------------+ | | + * info | | ... | | | + * entries | +------------------------------+ | | + * | | ipa_control_info_entry | | | + * \ | #hdr.entries - 1 | | | + * +------------------------------+ | | + * | empty space (optional) | | | + * +------------------------------+ <--´ . . . | + * / | ... | | entry[n].min.offset | + * | | ... | | | | | + * Data | | ... | | | entry[n].max.offset | + * section | | min value data for entry #n | <-----´ | | | + * | | max value data for entry #n | <-------´ | entry[n].def.offset | + * | | def value data for entry #n | <---------´ | + * | | ... | | + * \ | ... | | + * +------------------------------+ | + * | empty space (optional) | | + * +------------------------------+ <-------------------------------´ * ~~~~ * * The packet header is identical to the ControlList packet header. * * Entries are described by the ipa_control_info_entry structure. They contain - * the numerical ID and type of the control. The control info data is stored - * in the data section as described by the following diagram. + * the numerical ID, direction (in/out) of the control, and three + * ipa_control_value_entry structures for the min, max, and def ControlValues + * that make up the ControlInfo. * - * ~~~~ - * +-------------------------+ . - * / | ... | | entry[n].offset - * | +-------------------------+ <-----´ - * | | minimum value (#n) | \ - * Data | +-------------------------+ | - * section | | maximum value (#n) | | Entry #n - * | +-------------------------+ | - * | | default value (#n) | / - * | +-------------------------+ - * \ | ... | - * +-------------------------+ - * ~~~~ + * The control info has no associated data in the data section; + * instead the three control values for min, max, and def are stored in the data section + * + * + * The control values are stored (as ipa_control_list_entry) in the data + * section in the platform's native format. The ipa_control_value_entry::offset + * field stores the offset from the beginning of the data section to the + * values. * - * 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. + * ipa_control_value_entry structures contain the relevant + * ControlValue information for the ControlInfo's min, max, and def + * respectively, and their associated data is stored in the data section. * - * 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 - * contiguous in memory. + * The control info has no associated data in the data section. Instead the + * minimum, maximum, and default control values of the control info are stored + * n the data section in the platform's native data format. The + * ipa_control_value_entry::offset field stores the offset from the beginning + * of the data section to the control value data. * - * As for the ControlList packet, empty spaces may be present between the end of + * All control values in the data section shall be stored in the same order as + * the respective control info entries, in the order of min, max, def, and shall be + * aligned to a multiple of 8 bytes, and shall be contiguous in memory. + * + * As with the ControlList packet, empty spaces may be present between the end of * the entries array and the data section, and after the data section. They * shall be ignored when parsing the packet. */ @@ -192,8 +200,6 @@ static_assert(sizeof(ipa_controls_header) == 32, /** * \struct ipa_control_value_entry * \brief Description of a serialized ControlValue entry - * \var ipa_control_value_entry::id - * The numerical ID of the control * \var ipa_control_value_entry::type * The type of the control (defined by enum ControlType) * \var ipa_control_value_entry::is_array @@ -203,13 +209,25 @@ static_assert(sizeof(ipa_controls_header) == 32, * \var ipa_control_value_entry::offset * The offset in bytes from the beginning of the data section to the control * value data (shall be a multiple of 8 bytes). - * \var ipa_control_value_entry::padding - * Padding bytes (shall be set to 0) + * \var ipa_control_value_entry::reserved + * Reserved for future extensions */ static_assert(sizeof(ipa_control_value_entry) == 16, "Invalid ABI size change for struct ipa_control_value_entry"); +/** + * \struct ipa_control_list_entry + * \brief Description of a serialized ControlList entry + * \var ipa_control_list_entry::id + * The numerical ID of the control + * \var ipa_control_list_entry::value + * The description of the serialized ControlValue + */ + +static_assert(sizeof(ipa_control_list_entry) == 20, + "Invalid ABI size change for struct ipa_control_list_entry"); + /** * \struct ipa_control_info_entry * \brief Description of a serialized ControlInfo entry @@ -217,8 +235,6 @@ static_assert(sizeof(ipa_control_value_entry) == 16, * The numerical ID of the control * \var ipa_control_info_entry::type * The type of the control (defined by enum ControlType) - * \var ipa_control_info_entry::offset - * The offset in bytes from the beginning of the data section to the control * 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 @@ -226,9 +242,15 @@ static_assert(sizeof(ipa_control_value_entry) == 16, * metadata). \sa ControlId::Direction * \var ipa_control_info_entry::padding * Padding bytes (shall be set to 0) + * \var ipa_control_info_entry::min + * The description of the serialized ControlValue (min) + * \var ipa_control_info_entry::max + * The description of the serialized ControlValue (max) + * \var ipa_control_info_entry::def + * The description of the serialized ControlValue (def) */ -static_assert(sizeof(ipa_control_info_entry) == 16, +static_assert(sizeof(ipa_control_info_entry) == 64, "Invalid ABI size change for struct ipa_control_info_entry"); } /* namespace libcamera */ From patchwork Mon Nov 17 08:08:15 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Paul Elder X-Patchwork-Id: 25077 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 2FCCEBD80A for ; Mon, 17 Nov 2025 08:08:49 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 7865560A8A; Mon, 17 Nov 2025 09:08:47 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="Unco73E+"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 10579606A0 for ; Mon, 17 Nov 2025 09:08:45 +0100 (CET) Received: from neptunite.hamster-moth.ts.net (unknown [IPv6:2404:7a81:160:2100:e0cd:14aa:3681:9fc1]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id B4287FE; Mon, 17 Nov 2025 09:06:40 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1763366802; bh=SuAZT90gXGAqeM+eDrIqrsD8KQTAdFC2T5zm6f2kcgA=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Unco73E+R39sPegcyC7IFLFL85OiYl4JJIx+MQ/FQKWRIgH8ksjgpk7P9WFkgzCdI RvZhXbw5Gtcoe5xW61/H8DpeJ0BqvkXxu/PW0GI3H7fJE13EoB3y2slNGbPoPWdczd FGFK3q/6uopUckjYEPnuWGO6NLV+IT7Pwae4DJBc= From: Paul Elder To: libcamera-devel@lists.libcamera.org Cc: Paul Elder , =?utf-8?q?Barnab=C3=A1s_P?= =?utf-8?b?xZFjemU=?= , Naushir Patuck Subject: [PATCH v6 2/2] ipa: ipu3, mali-c55, rkisp1, rpi: Fix reporting non-scalar controls Date: Mon, 17 Nov 2025 17:08:15 +0900 Message-ID: <20251117080818.3009835-3-paul.elder@ideasonboard.com> X-Mailer: git-send-email 2.47.2 In-Reply-To: <20251117080818.3009835-1-paul.elder@ideasonboard.com> References: <20251117080818.3009835-1-paul.elder@ideasonboard.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" The ControlInfos of non-scalar controls that are reported in controls() must have non-scalar default values for controls that have a defined size. This is because applications should be able to directly set the default value from a ControlInfo to the control. Currently this is relevant to the following controls: - ColourGains - ColourCorrectionMatrix - FrameDurationLimits - AfWindows Fix the scalarness of these controls where relevant. Signed-off-by: Paul Elder Tested-by: Barnabás Pőcze # rkisp1 Reviewed-by: Naushir Patuck --- No change in v6 Changes in v5: - add AfWindows - create Span arrays on-the-fly Changes in v4: - improve commit message No change in v3 No change in v2 --- src/ipa/ipu3/ipu3.cpp | 4 ++-- src/ipa/mali-c55/mali-c55.cpp | 4 +++- src/ipa/rkisp1/algorithms/awb.cpp | 4 +++- src/ipa/rkisp1/rkisp1.cpp | 3 ++- src/ipa/rpi/common/ipa_base.cpp | 7 +++++-- 5 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 1cae08bf255f..b926f579a9a3 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -280,10 +281,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, uint64_t frameSize = lineLength * frameHeights[i]; frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); } - controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0], frameDurations[1], - frameDurations[2]); + Span{ { frameDurations[2], frameDurations[2] } }); controls.merge(context_.ctrlMap); *ipaControls = ControlInfoMap(std::move(controls), controls::controls); diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp index 7d45e7310aec..4eaedabb47b8 100644 --- a/src/ipa/mali-c55/mali-c55.cpp +++ b/src/ipa/mali-c55/mali-c55.cpp @@ -5,6 +5,7 @@ * Mali-C55 ISP image processing algorithms */ +#include #include #include #include @@ -14,6 +15,7 @@ #include #include +#include #include #include @@ -236,7 +238,7 @@ void IPAMaliC55::updateControls(const IPACameraSensorInfo &sensorInfo, ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0], frameDurations[1], - frameDurations[2]); + Span{ { frameDurations[2], frameDurations[2] } }); /* * Compute exposure time limits from the V4L2_CID_EXPOSURE control diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 399fb51be414..e8da7974a1d6 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -91,7 +91,9 @@ int Awb::init(IPAContext &context, const YamlObject &tuningData) kMaxColourTemperature, kDefaultColourTemperature); cmap[&controls::AwbEnable] = ControlInfo(false, true); - cmap[&controls::ColourGains] = ControlInfo(0.0f, 3.996f, 1.0f); + + cmap[&controls::ColourGains] = ControlInfo(0.0f, 3.996f, + Span{ { 1.0f, 1.0f } }); if (!tuningData.contains("algorithm")) LOG(RkISP1Awb, Info) << "No AWB algorithm specified." diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index fa22bfc34904..61d3d1f6f96b 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -439,7 +439,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, /* \todo Move this (and other agc-related controls) to agc */ context_.ctrlMap[&controls::FrameDurationLimits] = - ControlInfo(frameDurations[0], frameDurations[1], frameDurations[2]); + ControlInfo(frameDurations[0], frameDurations[1], + ControlValue(Span{ { frameDurations[2], frameDurations[2] } })); ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end()); *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index 8dfe35cc3267..14aba4500ae4 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -7,6 +7,7 @@ #include "ipa_base.h" +#include #include #include @@ -105,7 +106,8 @@ const ControlInfoMap::Map ipaAfControls{ { &controls::AfRange, ControlInfo(controls::AfRangeValues) }, { &controls::AfSpeed, ControlInfo(controls::AfSpeedValues) }, { &controls::AfMetering, ControlInfo(controls::AfMeteringValues) }, - { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, + { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), + Span{ { Rectangle{} } }) }, { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, { &controls::AfPause, ControlInfo(controls::AfPauseValues) }, { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) } @@ -246,7 +248,8 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa ctrlMap[&controls::FrameDurationLimits] = ControlInfo(static_cast(mode_.minFrameDuration.get()), static_cast(mode_.maxFrameDuration.get()), - static_cast(defaultMinFrameDuration.get())); + Span{ { static_cast(defaultMinFrameDuration.get()), + static_cast(defaultMinFrameDuration.get()) } }); ctrlMap[&controls::AnalogueGain] = ControlInfo(static_cast(mode_.minAnalogueGain),