[{"id":32573,"web_url":"https://patchwork.libcamera.org/comment/32573/","msgid":"<173348994905.3135963.16363381439149852001@ping.linuxembedded.co.uk>","date":"2024-12-06T12:59:09","subject":"Re: [PATCH v3 3/4] libcamera: controls: Add support for querying\n\tdirection information","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2024-11-29 09:19:15)\n> Add support to ControlId for querying direction information. This allows\n> applications to query whether a ControlId is meant for being set in\n> controls or to be returned in metadata or both. This also has a side\n> effect of properly encoding this information, as previously it was only\n> mentioned losely and inconsistently in the control id definition.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v3:\n> - make direction parameter to ControlId and Control consutrctors\n> mandatory\n> - this entails expanding the control serializer/deserializer for\n> V4L2Device and ControlSerializer to properly handle the direction\n> - also the constructor parameters have to be reordered, compared to\n> the last vesrion at least\n> \n> Changes in v2:\n> - simplify code\n> ---\n> include/libcamera/controls.h | 19 ++++++++++--\n> include/libcamera/ipa/ipa_controls.h | 3 +-\n> src/libcamera/control_ids.cpp.in | 4 +--\n> src/libcamera/control_serializer.cpp | 11 ++++++-\n> src/libcamera/controls.cpp | 45 ++++++++++++++++++++++++++--\n> src/libcamera/v4l2_device.cpp | 10 ++++++-\n> 6 files changed, 82 insertions(+), 10 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 3cfe2de5973c..13bb914badbf 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -17,6 +17,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -235,14 +236,24 @@ private:\n> class ControlId\n> {\n> public:\n> + enum class Direction {\n> + In = (1 << 0),\n> + Out = (1 << 1),\n> + };\n\nI'm curious that I think the previous patch is using the flags that are\nonly just defined here ...\n\n> +\n> + using DirectionFlags = Flags;\n> +\n> ControlId(unsigned int id, const std::string &name, const std::string &vendor,\n> - ControlType type, std::size_t size = 0,\n> + ControlType type, DirectionFlags direction,\n> + std::size_t size = 0,\n> const std::map &enumStrMap = {});\n> \n> unsigned int id() const { return id_; }\n> const std::string &name() const { return name_; }\n> const std::string &vendor() const { return vendor_; }\n> ControlType type() const { return type_; }\n> + bool isInput() const { return !!(direction_ & Direction::In); }\n> + bool isOutput() const { return !!(direction_ & Direction::Out); }\n> bool isArray() const { return size_ > 0; }\n> std::size_t size() const { return size_; }\n> const std::map &enumerators() const { return reverseMap_; }\n> @@ -254,11 +265,14 @@ private:\n> std::string name_;\n> std::string vendor_;\n> ControlType type_;\n> + DirectionFlags direction_;\n> std::size_t size_;\n> std::map enumStrMap_;\n> std::map reverseMap_;\n> };\n> \n> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction)\n> +\n> static inline bool operator==(unsigned int lhs, const ControlId &rhs)\n> {\n> return lhs == rhs.id();\n> @@ -286,9 +300,10 @@ public:\n> using type = T;\n> \n> Control(unsigned int id, const char *name, const char *vendor,\n> + ControlId::DirectionFlags direction,\n> const std::map &enumStrMap = {})\n> : ControlId(id, name, vendor, details::control_type>::value,\n> - details::control_type>::size, enumStrMap)\n> + direction, details::control_type>::size, enumStrMap)\n> {\n> }\n> \n> diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> index 5fd13394fcef..980668c86bcc 100644\n> --- a/include/libcamera/ipa/ipa_controls.h\n> +++ b/include/libcamera/ipa/ipa_controls.h\n> @@ -46,7 +46,8 @@ struct ipa_control_info_entry {\n> uint32_t id;\n> uint32_t type;\n> uint32_t offset;\n> - uint32_t padding[1];\n> + uint8_t direction;\n> + uint8_t padding[3];\n\nWhat's the padding for here? Alignment ? or something else?\nShould we 'add more' while we're in an ABI break ? or is it needed at\nall?\n\n> };\n> \n> #ifdef __cplusplus\n> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> index afe9e2c96610..65668d486dbc 100644\n> --- a/src/libcamera/control_ids.cpp.in\n> +++ b/src/libcamera/control_ids.cpp.in\n> @@ -89,9 +89,9 @@ extern const std::map {{ctrl.name}}NameValueMap = {\n> { \"{{enum.name}}\", {{enum.name}} },\n> {%- endfor %}\n> };\n> -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.name}}NameValueMap);\n> +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}}, {{ctrl.name}}NameValueMap);\n> {% else -%}\n> -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\");\n> +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}});\n> {% endif -%}\n> {%- endfor %}\n> \n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 0a5e8220a0ff..0804c178f0d4 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -281,6 +281,9 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> entry.id = id->id();\n> entry.type = id->type();\n> entry.offset = values.offset();\n> + entry.direction =\n> + (id->isInput() ? static_cast(ControlId::Direction::In) : 0) |\n> + (id->isOutput() ? static_cast(ControlId::Direction::Out) : 0);\n\nIs this necessary ? Can't the raw value just be serialized? (oh or\nperhaps the raw value can't be accessed in which case it probably is\nneeded ... unless we add an id->direction(); which is probably better\nthan re-encoding the same thing ...)\n\n> entries.write(&entry);\n> \n> store(info, values);\n> @@ -493,12 +496,18 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \n> /* If we're using a local id map, populate it. */\n> if (localIdMap) {\n> + ControlId::DirectionFlags flags;\n> + if (entry->direction & static_cast(ControlId::Direction::In))\n> + flags |= ControlId::Direction::In;\n> + if (entry->direction & static_cast(ControlId::Direction::Out))\n> + flags |= ControlId::Direction::Out;\n\n\nand here - can't we just do a direct usage? or is there an issue with\nserialising 'Flags' ?\n\n> /**\n> * \\todo Find a way to preserve the control name for\n> * debugging purpose.\n> */\n> controlIds_.emplace_back(std::make_unique(entry->id,\n> - \"\", \"local\", type));\n> + \"\", \"local\", type,\n> + flags));\n> (*localIdMap)[entry->id] = controlIds_.back().get();\n> }\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 2efecf0fc52b..23b71fac8e84 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -396,15 +396,16 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> * \\param[in] name The control name\n> * \\param[in] vendor The vendor name\n> * \\param[in] type The control data type\n> + * \\param[in] direction The direction of the control, if it can be used in Controls or Metadata\n> * \\param[in] size The size of the array control, or 0 if scalar control\n> * \\param[in] enumStrMap The map from enum names to values (optional)\n> */\n> ControlId::ControlId(unsigned int id, const std::string &name,\n> const std::string &vendor, ControlType type,\n> - std::size_t size,\n> + DirectionFlags direction, std::size_t size,\n> const std::map &enumStrMap)\n> - : id_(id), name_(name), vendor_(vendor), type_(type), size_(size),\n> - enumStrMap_(enumStrMap)\n> + : id_(id), name_(name), vendor_(vendor), type_(type),\n> + direction_(direction), size_(size), enumStrMap_(enumStrMap)\n> {\n> for (const auto &pair : enumStrMap_)\n> reverseMap_[pair.second] = pair.first;\n> @@ -434,6 +435,26 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> * \\return The control data type\n> */\n> \n> +/**\n> + * \\fn bool ControlId::isInput() const\n> + * \\brief Determine if the control is available to be used as an input control\n> + *\n> + * Controls can be used either as input in controls, or as output in metadata.\n> + * This function checks if the control is allowed to be used as the former.\n> + *\n> + * \\return True if the control can be used as an input control, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn bool ControlId::isOutput() const\n> + * \\brief Determine if the control is available to be used in output metadata\n> + *\n> + * Controls can be used either as input in controls, or as output in metadata.\n> + * This function checks if the control is allowed to be used as the latter.\n> + *\n> + * \\return True if the control can be returned in output metadata, false otherwise\n> + */\n> +\n> /**\n> * \\fn bool ControlId::isArray() const\n> * \\brief Determine if the control is an array control\n> @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> * \\return True if \\a lhs.id() is equal to \\a rhs, false otherwise\n> */\n> \n> +/**\n> + * \\enum ControlId::Direction\n> + * \\brief The direction the control is capable of being passed from/to\n> + *\n> + * \\var ControlId::Direction::In\n> + * \\brief The control can be passed as input in controls\n> + *\n> + * \\var ControlId::Direction::Out\n> + * \\brief The control can be returned as output in metadata\n> + */\n> +\n> +/**\n> + * \\typedef ControlId::DirectionFlags\n> + * \\brief A wrapper for ControlId::Direction so that it can be used as flags\n> + */\n> +\n> /**\n> * \\class Control\n> * \\brief Describe a control and its intrinsic properties\n> @@ -504,6 +541,8 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> * \\param[in] id The control numerical ID\n> * \\param[in] name The control name\n> * \\param[in] vendor The vendor name\n> + * \\param[in] direction The direction of the control, if it can be used in\n> + * Controls or Metadata\n> * \\param[in] enumStrMap The map from enum names to values (optional)\n> *\n> * The control data type is automatically deduced from the template type T.\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 7d21cf15fec1..4634ccc392b6 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -520,7 +520,15 @@ std::unique_ptr V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &\n> const std::string name(static_cast(ctrl.name), len);\n> const ControlType type = v4l2CtrlType(ctrl.type);\n> \n> - return std::make_unique(ctrl.id, name, \"v4l2\", type);\n> + ControlId::DirectionFlags flags;\n\nI expect it's fine, but the above is uninitialised,\n\n> + if (ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)\n> + flags |= ControlId::Direction::Out;\n\nSo you shouldn't be 'or'ing in here, just assign directly.\n\nSame for the two below.\n\n> + else if (ctrl.flags & V4L2_CTRL_FLAG_WRITE_ONLY)\n> + flags |= ControlId::Direction::In;\n> + else\n> + flags |= ControlId::Direction::In | ControlId::Direction::Out;\n> +\n> + return std::make_unique(ctrl.id, name, \"v4l2\", type, flags);\n> }\n> \n> /**\n> -- \n> 2.39.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 89617BF415\n\tfor ;\n\tFri, 6 Dec 2024 12:59:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 70B696613E;\n\tFri, 6 Dec 2024 13:59:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0548618B3\n\tfor ;\n\tFri, 6 Dec 2024 13:59:11 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 21DF774C;\n\tFri, 6 Dec 2024 13:58:42 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PeMMPmeO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733489922;\n\tbh=GcLaB9aPU4t/FDTw7/mekZsLEOtTNdZjKmSId99tRM4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=PeMMPmeOZubdyvnHvaG0+BuyZIeXkoZsTqx4XLG/1SRm2l+NMnfuOIUo/ljoJ9/gv\n\tg9J0g28W7XfpGBqNdmMfQeuTkwGVYc97c9lFCD9CiAPDzoBoWvHbzfVFuWmtqP6zZ+\n\tSCNUV+wf7HIvkZtaQFhkTEzx42g3WMs82A5Gs42o=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241129091916.298359-4-paul.elder@ideasonboard.com>","References":"<20241129091916.298359-1-paul.elder@ideasonboard.com>\n\t<20241129091916.298359-4-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v3 3/4] libcamera: controls: Add support for querying\n\tdirection information","From":"Kieran Bingham ","Cc":"Paul Elder ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 06 Dec 2024 12:59:09 +0000","Message-ID":"<173348994905.3135963.16363381439149852001@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32575,"web_url":"https://patchwork.libcamera.org/comment/32575/","msgid":"<20241206131259.GF25902@pendragon.ideasonboard.com>","date":"2024-12-06T13:12:59","subject":"Re: [PATCH v3 3/4] libcamera: controls: Add support for querying\n\tdirection information","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Nov 29, 2024 at 06:19:15PM +0900, Paul Elder wrote:\n> Add support to ControlId for querying direction information. This allows\n> applications to query whether a ControlId is meant for being set in\n> controls or to be returned in metadata or both. This also has a side\n> effect of properly encoding this information, as previously it was only\n> mentioned losely and inconsistently in the control id definition.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v3:\n> - make direction parameter to ControlId and Control consutrctors\n> mandatory\n> - this entails expanding the control serializer/deserializer for\n> V4L2Device and ControlSerializer to properly handle the direction\n> - also the constructor parameters have to be reordered, compared to\n> the last vesrion at least\n> \n> Changes in v2:\n> - simplify code\n> ---\n> include/libcamera/controls.h | 19 ++++++++++--\n> include/libcamera/ipa/ipa_controls.h | 3 +-\n> src/libcamera/control_ids.cpp.in | 4 +--\n> src/libcamera/control_serializer.cpp | 11 ++++++-\n> src/libcamera/controls.cpp | 45 ++++++++++++++++++++++++++--\n> src/libcamera/v4l2_device.cpp | 10 ++++++-\n> 6 files changed, 82 insertions(+), 10 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 3cfe2de5973c..13bb914badbf 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -17,6 +17,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -235,14 +236,24 @@ private:\n> class ControlId\n> {\n> public:\n> +\tenum class Direction {\n> +\t\tIn = (1 << 0),\n> +\t\tOut = (1 << 1),\n> +\t};\n> +\n> +\tusing DirectionFlags = Flags;\n> +\n> \tControlId(unsigned int id, const std::string &name, const std::string &vendor,\n> -\t\t ControlType type, std::size_t size = 0,\n> +\t\t ControlType type, DirectionFlags direction,\n> +\t\t std::size_t size = 0,\n> \t\t const std::map &enumStrMap = {});\n> \n> \tunsigned int id() const { return id_; }\n> \tconst std::string &name() const { return name_; }\n> \tconst std::string &vendor() const { return vendor_; }\n> \tControlType type() const { return type_; }\n> +\tbool isInput() const { return !!(direction_ & Direction::In); }\n> +\tbool isOutput() const { return !!(direction_ & Direction::Out); }\n> \tbool isArray() const { return size_ > 0; }\n> \tstd::size_t size() const { return size_; }\n> \tconst std::map &enumerators() const { return reverseMap_; }\n> @@ -254,11 +265,14 @@ private:\n> \tstd::string name_;\n> \tstd::string vendor_;\n> \tControlType type_;\n> +\tDirectionFlags direction_;\n> \tstd::size_t size_;\n> \tstd::map enumStrMap_;\n> \tstd::map reverseMap_;\n> };\n> \n> +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction)\n> +\n> static inline bool operator==(unsigned int lhs, const ControlId &rhs)\n> {\n> \treturn lhs == rhs.id();\n> @@ -286,9 +300,10 @@ public:\n> \tusing type = T;\n> \n> \tControl(unsigned int id, const char *name, const char *vendor,\n> +\t\tControlId::DirectionFlags direction,\n> \t\tconst std::map &enumStrMap = {})\n> \t\t: ControlId(id, name, vendor, details::control_type>::value,\n> -\t\t\t details::control_type>::size, enumStrMap)\n> +\t\t\t direction, details::control_type>::size, enumStrMap)\n> \t{\n> \t}\n> \n> diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> index 5fd13394fcef..980668c86bcc 100644\n> --- a/include/libcamera/ipa/ipa_controls.h\n> +++ b/include/libcamera/ipa/ipa_controls.h\n> @@ -46,7 +46,8 @@ struct ipa_control_info_entry {\n> \tuint32_t id;\n> \tuint32_t type;\n> \tuint32_t offset;\n> -\tuint32_t padding[1];\n> +\tuint8_t direction;\n> +\tuint8_t padding[3];\n> };\n> \n> #ifdef __cplusplus\n> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> index afe9e2c96610..65668d486dbc 100644\n> --- a/src/libcamera/control_ids.cpp.in\n> +++ b/src/libcamera/control_ids.cpp.in\n> @@ -89,9 +89,9 @@ extern const std::map {{ctrl.name}}NameValueMap = {\n> \t{ \"{{enum.name}}\", {{enum.name}} },\n> {%- endfor %}\n> };\n> -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.name}}NameValueMap);\n> +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}}, {{ctrl.name}}NameValueMap);\n> {% else -%}\n> -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\");\n> +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}});\n> {% endif -%}\n> {%- endfor %}\n> \n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index 0a5e8220a0ff..0804c178f0d4 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -281,6 +281,9 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \t\tentry.id = id->id();\n> \t\tentry.type = id->type();\n> \t\tentry.offset = values.offset();\n> +\t\tentry.direction =\n> +\t\t\t(id->isInput() ? static_cast(ControlId::Direction::In) : 0) |\n> +\t\t\t(id->isOutput() ? static_cast(ControlId::Direction::Out) : 0);\n> \t\tentries.write(&entry);\n> \n> \t\tstore(info, values);\n> @@ -493,12 +496,18 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \n> \t\t/* If we're using a local id map, populate it. */\n> \t\tif (localIdMap) {\n> +\t\t\tControlId::DirectionFlags flags;\n> +\t\t\tif (entry->direction & static_cast(ControlId::Direction::In))\n> +\t\t\t\tflags |= ControlId::Direction::In;\n> +\t\t\tif (entry->direction & static_cast(ControlId::Direction::Out))\n> +\t\t\t\tflags |= ControlId::Direction::Out;\n> \t\t\t/**\n> \t\t\t * \\todo Find a way to preserve the control name for\n> \t\t\t * debugging purpose.\n> \t\t\t */\n> \t\t\tcontrolIds_.emplace_back(std::make_unique(entry->id,\n> -\t\t\t\t\t\t\t\t\t \"\", \"local\", type));\n> +\t\t\t\t\t\t\t\t\t \"\", \"local\", type,\n> +\t\t\t\t\t\t\t\t\t flags));\n> \t\t\t(*localIdMap)[entry->id] = controlIds_.back().get();\n> \t\t}\n> \n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 2efecf0fc52b..23b71fac8e84 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -396,15 +396,16 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> * \\param[in] name The control name\n> * \\param[in] vendor The vendor name\n> * \\param[in] type The control data type\n> + * \\param[in] direction The direction of the control, if it can be used in Controls or Metadata\n> * \\param[in] size The size of the array control, or 0 if scalar control\n> * \\param[in] enumStrMap The map from enum names to values (optional)\n> */\n> ControlId::ControlId(unsigned int id, const std::string &name,\n> \t\t const std::string &vendor, ControlType type,\n> -\t\t std::size_t size,\n> +\t\t DirectionFlags direction, std::size_t size,\n> \t\t const std::map &enumStrMap)\n> -\t: id_(id), name_(name), vendor_(vendor), type_(type), size_(size),\n> -\t enumStrMap_(enumStrMap)\n> +\t: id_(id), name_(name), vendor_(vendor), type_(type),\n> +\t direction_(direction), size_(size), enumStrMap_(enumStrMap)\n> {\n> \tfor (const auto &pair : enumStrMap_)\n> \t\treverseMap_[pair.second] = pair.first;\n> @@ -434,6 +435,26 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> * \\return The control data type\n> */\n> \n> +/**\n> + * \\fn bool ControlId::isInput() const\n> + * \\brief Determine if the control is available to be used as an input control\n> + *\n> + * Controls can be used either as input in controls, or as output in metadata.\n> + * This function checks if the control is allowed to be used as the former.\n> + *\n> + * \\return True if the control can be used as an input control, false otherwise\n> + */\n> +\n> +/**\n> + * \\fn bool ControlId::isOutput() const\n> + * \\brief Determine if the control is available to be used in output metadata\n> + *\n> + * Controls can be used either as input in controls, or as output in metadata.\n> + * This function checks if the control is allowed to be used as the latter.\n> + *\n> + * \\return True if the control can be returned in output metadata, false otherwise\n> + */\n> +\n> /**\n> * \\fn bool ControlId::isArray() const\n> * \\brief Determine if the control is an array control\n> @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> * \\return True if \\a lhs.id() is equal to \\a rhs, false otherwise\n> */\n> \n> +/**\n> + * \\enum ControlId::Direction\n> + * \\brief The direction the control is capable of being passed from/to\n> + *\n> + * \\var ControlId::Direction::In\n> + * \\brief The control can be passed as input in controls\n> + *\n> + * \\var ControlId::Direction::Out\n> + * \\brief The control can be returned as output in metadata\n> + */\n> +\n> +/**\n> + * \\typedef ControlId::DirectionFlags\n> + * \\brief A wrapper for ControlId::Direction so that it can be used as flags\n> + */\n> +\n> /**\n> * \\class Control\n> * \\brief Describe a control and its intrinsic properties\n> @@ -504,6 +541,8 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> * \\param[in] id The control numerical ID\n> * \\param[in] name The control name\n> * \\param[in] vendor The vendor name\n> + * \\param[in] direction The direction of the control, if it can be used in\n> + * Controls or Metadata\n> * \\param[in] enumStrMap The map from enum names to values (optional)\n> *\n> * The control data type is automatically deduced from the template type T.\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 7d21cf15fec1..4634ccc392b6 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -520,7 +520,15 @@ std::unique_ptr V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &\n> \tconst std::string name(static_cast(ctrl.name), len);\n> \tconst ControlType type = v4l2CtrlType(ctrl.type);\n> \n> -\treturn std::make_unique(ctrl.id, name, \"v4l2\", type);\n> +\tControlId::DirectionFlags flags;\n> +\tif (ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)\n> +\t\tflags |= ControlId::Direction::Out;\n> +\telse if (ctrl.flags & V4L2_CTRL_FLAG_WRITE_ONLY)\n> +\t\tflags |= ControlId::Direction::In;\n> +\telse\n> +\t\tflags |= ControlId::Direction::In | ControlId::Direction::Out;\n\nYou could replace |= with =. Not sure it makes a difference.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\treturn std::make_unique(ctrl.id, name, \"v4l2\", type, flags);\n> }\n> \n> /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1FFF7BE173\n\tfor ;\n\tFri, 6 Dec 2024 13:13:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28DEB66145;\n\tFri, 6 Dec 2024 14:13:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A37E618B3\n\tfor ;\n\tFri, 6 Dec 2024 14:13:13 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 87F72641;\n\tFri, 6 Dec 2024 14:12:43 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hWniiMaQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733490763;\n\tbh=0cwDhFp/kYUxLnd0ywIzpAbiHp41QD+yjAXxU14GQh8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hWniiMaQ9F6cPrtdeSxVsFz+M3X1N1H2fhS4YJKEHKxzqlNI4mosRAotdBBOVNtjh\n\tgidQeXDiL3lyorn1WvgGrTSDnLf5/NREXWU9jhfs66IJFMRLcsySExjEPUqB++FPeY\n\tIub53QBQpmmvlU+TJD14MdKBOU8VdlSZyZFRQ9dE=","Date":"Fri, 6 Dec 2024 15:12:59 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 3/4] libcamera: controls: Add support for querying\n\tdirection information","Message-ID":"<20241206131259.GF25902@pendragon.ideasonboard.com>","References":"<20241129091916.298359-1-paul.elder@ideasonboard.com>\n\t<20241129091916.298359-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241129091916.298359-4-paul.elder@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32577,"web_url":"https://patchwork.libcamera.org/comment/32577/","msgid":"<20241206131445.GG25902@pendragon.ideasonboard.com>","date":"2024-12-06T13:14:45","subject":"Re: [PATCH v3 3/4] libcamera: controls: Add support for querying\n\tdirection information","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Dec 06, 2024 at 12:59:09PM +0000, Kieran Bingham wrote:\n> Quoting Paul Elder (2024-11-29 09:19:15)\n> > Add support to ControlId for querying direction information. This allows\n> > applications to query whether a ControlId is meant for being set in\n> > controls or to be returned in metadata or both. This also has a side\n> > effect of properly encoding this information, as previously it was only\n> > mentioned losely and inconsistently in the control id definition.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v3:\n> > - make direction parameter to ControlId and Control consutrctors\n> > mandatory\n> > - this entails expanding the control serializer/deserializer for\n> > V4L2Device and ControlSerializer to properly handle the direction\n> > - also the constructor parameters have to be reordered, compared to\n> > the last vesrion at least\n> > \n> > Changes in v2:\n> > - simplify code\n> > ---\n> > include/libcamera/controls.h | 19 ++++++++++--\n> > include/libcamera/ipa/ipa_controls.h | 3 +-\n> > src/libcamera/control_ids.cpp.in | 4 +--\n> > src/libcamera/control_serializer.cpp | 11 ++++++-\n> > src/libcamera/controls.cpp | 45 ++++++++++++++++++++++++++--\n> > src/libcamera/v4l2_device.cpp | 10 ++++++-\n> > 6 files changed, 82 insertions(+), 10 deletions(-)\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 3cfe2de5973c..13bb914badbf 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -17,6 +17,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -235,14 +236,24 @@ private:\n> > class ControlId\n> > {\n> > public:\n> > + enum class Direction {\n> > + In = (1 << 0),\n> > + Out = (1 << 1),\n> > + };\n> \n> I'm curious that I think the previous patch is using the flags that are\n> only just defined here ...\n> \n> > +\n> > + using DirectionFlags = Flags;\n> > +\n> > ControlId(unsigned int id, const std::string &name, const std::string &vendor,\n> > - ControlType type, std::size_t size = 0,\n> > + ControlType type, DirectionFlags direction,\n> > + std::size_t size = 0,\n> > const std::map &enumStrMap = {});\n> > \n> > unsigned int id() const { return id_; }\n> > const std::string &name() const { return name_; }\n> > const std::string &vendor() const { return vendor_; }\n> > ControlType type() const { return type_; }\n> > + bool isInput() const { return !!(direction_ & Direction::In); }\n> > + bool isOutput() const { return !!(direction_ & Direction::Out); }\n> > bool isArray() const { return size_ > 0; }\n> > std::size_t size() const { return size_; }\n> > const std::map &enumerators() const { return reverseMap_; }\n> > @@ -254,11 +265,14 @@ private:\n> > std::string name_;\n> > std::string vendor_;\n> > ControlType type_;\n> > + DirectionFlags direction_;\n> > std::size_t size_;\n> > std::map enumStrMap_;\n> > std::map reverseMap_;\n> > };\n> > \n> > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction)\n> > +\n> > static inline bool operator==(unsigned int lhs, const ControlId &rhs)\n> > {\n> > return lhs == rhs.id();\n> > @@ -286,9 +300,10 @@ public:\n> > using type = T;\n> > \n> > Control(unsigned int id, const char *name, const char *vendor,\n> > + ControlId::DirectionFlags direction,\n> > const std::map &enumStrMap = {})\n> > : ControlId(id, name, vendor, details::control_type>::value,\n> > - details::control_type>::size, enumStrMap)\n> > + direction, details::control_type>::size, enumStrMap)\n> > {\n> > }\n> > \n> > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> > index 5fd13394fcef..980668c86bcc 100644\n> > --- a/include/libcamera/ipa/ipa_controls.h\n> > +++ b/include/libcamera/ipa/ipa_controls.h\n> > @@ -46,7 +46,8 @@ struct ipa_control_info_entry {\n> > uint32_t id;\n> > uint32_t type;\n> > uint32_t offset;\n> > - uint32_t padding[1];\n> > + uint8_t direction;\n> > + uint8_t padding[3];\n> \n> What's the padding for here? Alignment ? or something else?\n> Should we 'add more' while we're in an ABI break ? or is it needed at\n> all?\n> \n> > };\n> > \n> > #ifdef __cplusplus\n> > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> > index afe9e2c96610..65668d486dbc 100644\n> > --- a/src/libcamera/control_ids.cpp.in\n> > +++ b/src/libcamera/control_ids.cpp.in\n> > @@ -89,9 +89,9 @@ extern const std::map {{ctrl.name}}NameValueMap = {\n> > { \"{{enum.name}}\", {{enum.name}} },\n> > {%- endfor %}\n> > };\n> > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.name}}NameValueMap);\n> > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}}, {{ctrl.name}}NameValueMap);\n> > {% else -%}\n> > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\");\n> > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}});\n> > {% endif -%}\n> > {%- endfor %}\n> > \n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index 0a5e8220a0ff..0804c178f0d4 100644\n> > --- a/src/libcamera/control_serializer.cpp\n> > +++ b/src/libcamera/control_serializer.cpp\n> > @@ -281,6 +281,9 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > entry.id = id->id();\n> > entry.type = id->type();\n> > entry.offset = values.offset();\n> > + entry.direction =\n> > + (id->isInput() ? static_cast(ControlId::Direction::In) : 0) |\n> > + (id->isOutput() ? static_cast(ControlId::Direction::Out) : 0);\n> \n> Is this necessary ? Can't the raw value just be serialized? (oh or\n> perhaps the raw value can't be accessed in which case it probably is\n> needed ... unless we add an id->direction(); which is probably better\n> than re-encoding the same thing ...)\n\nA direction() function could indeed be useful.\n\n> > entries.write(&entry);\n> > \n> > store(info, values);\n> > @@ -493,12 +496,18 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > \n> > /* If we're using a local id map, populate it. */\n> > if (localIdMap) {\n> > + ControlId::DirectionFlags flags;\n> > + if (entry->direction & static_cast(ControlId::Direction::In))\n> > + flags |= ControlId::Direction::In;\n> > + if (entry->direction & static_cast(ControlId::Direction::Out))\n> > + flags |= ControlId::Direction::Out;\n> \n> \n> and here - can't we just do a direct usage? or is there an issue with\n> serialising 'Flags' ?\n> \n> > /**\n> > * \\todo Find a way to preserve the control name for\n> > * debugging purpose.\n> > */\n> > controlIds_.emplace_back(std::make_unique(entry->id,\n> > - \"\", \"local\", type));\n> > + \"\", \"local\", type,\n> > + flags));\n> > (*localIdMap)[entry->id] = controlIds_.back().get();\n> > }\n> > \n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 2efecf0fc52b..23b71fac8e84 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -396,15 +396,16 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> > * \\param[in] name The control name\n> > * \\param[in] vendor The vendor name\n> > * \\param[in] type The control data type\n> > + * \\param[in] direction The direction of the control, if it can be used in Controls or Metadata\n> > * \\param[in] size The size of the array control, or 0 if scalar control\n> > * \\param[in] enumStrMap The map from enum names to values (optional)\n> > */\n> > ControlId::ControlId(unsigned int id, const std::string &name,\n> > const std::string &vendor, ControlType type,\n> > - std::size_t size,\n> > + DirectionFlags direction, std::size_t size,\n> > const std::map &enumStrMap)\n> > - : id_(id), name_(name), vendor_(vendor), type_(type), size_(size),\n> > - enumStrMap_(enumStrMap)\n> > + : id_(id), name_(name), vendor_(vendor), type_(type),\n> > + direction_(direction), size_(size), enumStrMap_(enumStrMap)\n> > {\n> > for (const auto &pair : enumStrMap_)\n> > reverseMap_[pair.second] = pair.first;\n> > @@ -434,6 +435,26 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > * \\return The control data type\n> > */\n> > \n> > +/**\n> > + * \\fn bool ControlId::isInput() const\n> > + * \\brief Determine if the control is available to be used as an input control\n> > + *\n> > + * Controls can be used either as input in controls, or as output in metadata.\n> > + * This function checks if the control is allowed to be used as the former.\n> > + *\n> > + * \\return True if the control can be used as an input control, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn bool ControlId::isOutput() const\n> > + * \\brief Determine if the control is available to be used in output metadata\n> > + *\n> > + * Controls can be used either as input in controls, or as output in metadata.\n> > + * This function checks if the control is allowed to be used as the latter.\n> > + *\n> > + * \\return True if the control can be returned in output metadata, false otherwise\n> > + */\n> > +\n> > /**\n> > * \\fn bool ControlId::isArray() const\n> > * \\brief Determine if the control is an array control\n> > @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > * \\return True if \\a lhs.id() is equal to \\a rhs, false otherwise\n> > */\n> > \n> > +/**\n> > + * \\enum ControlId::Direction\n> > + * \\brief The direction the control is capable of being passed from/to\n> > + *\n> > + * \\var ControlId::Direction::In\n> > + * \\brief The control can be passed as input in controls\n> > + *\n> > + * \\var ControlId::Direction::Out\n> > + * \\brief The control can be returned as output in metadata\n> > + */\n> > +\n> > +/**\n> > + * \\typedef ControlId::DirectionFlags\n> > + * \\brief A wrapper for ControlId::Direction so that it can be used as flags\n> > + */\n> > +\n> > /**\n> > * \\class Control\n> > * \\brief Describe a control and its intrinsic properties\n> > @@ -504,6 +541,8 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > * \\param[in] id The control numerical ID\n> > * \\param[in] name The control name\n> > * \\param[in] vendor The vendor name\n> > + * \\param[in] direction The direction of the control, if it can be used in\n> > + * Controls or Metadata\n> > * \\param[in] enumStrMap The map from enum names to values (optional)\n> > *\n> > * The control data type is automatically deduced from the template type T.\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 7d21cf15fec1..4634ccc392b6 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -520,7 +520,15 @@ std::unique_ptr V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &\n> > const std::string name(static_cast(ctrl.name), len);\n> > const ControlType type = v4l2CtrlType(ctrl.type);\n> > \n> > - return std::make_unique(ctrl.id, name, \"v4l2\", type);\n> > + ControlId::DirectionFlags flags;\n> \n> I expect it's fine, but the above is uninitialised,\n\nThe Flags class has a default constructor that initializes the value to\n°.\n\n> > + if (ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)\n> > + flags |= ControlId::Direction::Out;\n> \n> So you shouldn't be 'or'ing in here, just assign directly.\n> \n> Same for the two below.\n> \n> > + else if (ctrl.flags & V4L2_CTRL_FLAG_WRITE_ONLY)\n> > + flags |= ControlId::Direction::In;\n> > + else\n> > + flags |= ControlId::Direction::In | ControlId::Direction::Out;\n> > +\n> > + return std::make_unique(ctrl.id, name, \"v4l2\", type, flags);\n> > }\n> > \n> > /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 14F53BE173\n\tfor ;\n\tFri, 6 Dec 2024 13:15:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBBB266151;\n\tFri, 6 Dec 2024 14:15:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67D1D618B3\n\tfor ;\n\tFri, 6 Dec 2024 14:14:58 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E6B16641;\n\tFri, 6 Dec 2024 14:14:28 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wMgP7u3z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733490869;\n\tbh=lEI193LkzORcfWpMPPB1Tk6kRnB8KnnbpyzhWmuKGdI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wMgP7u3z7pCEOzSXNR/zVzoFvEzZxnasU0lCJfhem7zcbPv5hi/nh5ASp5ufsLJPz\n\tK6nHX494euiUgOFs8lUarSRWedyqpjl9JlfYkMQi2PhSV6YE1n/0BZBRTS0nkJH3y6\n\tFd2FZH8LSPU2DVT3ubS0hvNJuEuZv8WjdTMnkO0w=","Date":"Fri, 6 Dec 2024 15:14:45 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 3/4] libcamera: controls: Add support for querying\n\tdirection information","Message-ID":"<20241206131445.GG25902@pendragon.ideasonboard.com>","References":"<20241129091916.298359-1-paul.elder@ideasonboard.com>\n\t<20241129091916.298359-4-paul.elder@ideasonboard.com>\n\t<173348994905.3135963.16363381439149852001@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<173348994905.3135963.16363381439149852001@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32634,"web_url":"https://patchwork.libcamera.org/comment/32634/","msgid":"","date":"2024-12-09T11:00:41","subject":"Re: [PATCH v3 3/4] libcamera: controls: Add support for querying\n\tdirection information","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Fri, Dec 06, 2024 at 12:59:09PM +0000, Kieran Bingham wrote:\n> Quoting Paul Elder (2024-11-29 09:19:15)\n> > Add support to ControlId for querying direction information. This allows\n> > applications to query whether a ControlId is meant for being set in\n> > controls or to be returned in metadata or both. This also has a side\n> > effect of properly encoding this information, as previously it was only\n> > mentioned losely and inconsistently in the control id definition.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v3:\n> > - make direction parameter to ControlId and Control consutrctors\n> > mandatory\n> > - this entails expanding the control serializer/deserializer for\n> > V4L2Device and ControlSerializer to properly handle the direction\n> > - also the constructor parameters have to be reordered, compared to\n> > the last vesrion at least\n> > \n> > Changes in v2:\n> > - simplify code\n> > ---\n> > include/libcamera/controls.h | 19 ++++++++++--\n> > include/libcamera/ipa/ipa_controls.h | 3 +-\n> > src/libcamera/control_ids.cpp.in | 4 +--\n> > src/libcamera/control_serializer.cpp | 11 ++++++-\n> > src/libcamera/controls.cpp | 45 ++++++++++++++++++++++++++--\n> > src/libcamera/v4l2_device.cpp | 10 ++++++-\n> > 6 files changed, 82 insertions(+), 10 deletions(-)\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 3cfe2de5973c..13bb914badbf 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -17,6 +17,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -235,14 +236,24 @@ private:\n> > class ControlId\n> > {\n> > public:\n> > + enum class Direction {\n> > + In = (1 << 0),\n> > + Out = (1 << 1),\n> > + };\n> \n> I'm curious that I think the previous patch is using the flags that are\n> only just defined here ...\n> \n> > +\n> > + using DirectionFlags = Flags;\n> > +\n> > ControlId(unsigned int id, const std::string &name, const std::string &vendor,\n> > - ControlType type, std::size_t size = 0,\n> > + ControlType type, DirectionFlags direction,\n> > + std::size_t size = 0,\n> > const std::map &enumStrMap = {});\n> > \n> > unsigned int id() const { return id_; }\n> > const std::string &name() const { return name_; }\n> > const std::string &vendor() const { return vendor_; }\n> > ControlType type() const { return type_; }\n> > + bool isInput() const { return !!(direction_ & Direction::In); }\n> > + bool isOutput() const { return !!(direction_ & Direction::Out); }\n> > bool isArray() const { return size_ > 0; }\n> > std::size_t size() const { return size_; }\n> > const std::map &enumerators() const { return reverseMap_; }\n> > @@ -254,11 +265,14 @@ private:\n> > std::string name_;\n> > std::string vendor_;\n> > ControlType type_;\n> > + DirectionFlags direction_;\n> > std::size_t size_;\n> > std::map enumStrMap_;\n> > std::map reverseMap_;\n> > };\n> > \n> > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction)\n> > +\n> > static inline bool operator==(unsigned int lhs, const ControlId &rhs)\n> > {\n> > return lhs == rhs.id();\n> > @@ -286,9 +300,10 @@ public:\n> > using type = T;\n> > \n> > Control(unsigned int id, const char *name, const char *vendor,\n> > + ControlId::DirectionFlags direction,\n> > const std::map &enumStrMap = {})\n> > : ControlId(id, name, vendor, details::control_type>::value,\n> > - details::control_type>::size, enumStrMap)\n> > + direction, details::control_type>::size, enumStrMap)\n> > {\n> > }\n> > \n> > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> > index 5fd13394fcef..980668c86bcc 100644\n> > --- a/include/libcamera/ipa/ipa_controls.h\n> > +++ b/include/libcamera/ipa/ipa_controls.h\n> > @@ -46,7 +46,8 @@ struct ipa_control_info_entry {\n> > uint32_t id;\n> > uint32_t type;\n> > uint32_t offset;\n> > - uint32_t padding[1];\n> > + uint8_t direction;\n> > + uint8_t padding[3];\n> \n> What's the padding for here? Alignment ? or something else?\n> Should we 'add more' while we're in an ABI break ? or is it needed at\n> all?\n\nI assumed it was for alignment.\n\n\nPaul\n\n> \n> > };\n> > \n> > #ifdef __cplusplus\n> > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> > index afe9e2c96610..65668d486dbc 100644\n> > --- a/src/libcamera/control_ids.cpp.in\n> > +++ b/src/libcamera/control_ids.cpp.in\n> > @@ -89,9 +89,9 @@ extern const std::map {{ctrl.name}}NameValueMap = {\n> > { \"{{enum.name}}\", {{enum.name}} },\n> > {%- endfor %}\n> > };\n> > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.name}}NameValueMap);\n> > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}}, {{ctrl.name}}NameValueMap);\n> > {% else -%}\n> > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\");\n> > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", \"{{vendor}}\", {{ctrl.direction}});\n> > {% endif -%}\n> > {%- endfor %}\n> > \n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index 0a5e8220a0ff..0804c178f0d4 100644\n> > --- a/src/libcamera/control_serializer.cpp\n> > +++ b/src/libcamera/control_serializer.cpp\n> > @@ -281,6 +281,9 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > entry.id = id->id();\n> > entry.type = id->type();\n> > entry.offset = values.offset();\n> > + entry.direction =\n> > + (id->isInput() ? static_cast(ControlId::Direction::In) : 0) |\n> > + (id->isOutput() ? static_cast(ControlId::Direction::Out) : 0);\n> \n> Is this necessary ? Can't the raw value just be serialized? (oh or\n> perhaps the raw value can't be accessed in which case it probably is\n> needed ... unless we add an id->direction(); which is probably better\n> than re-encoding the same thing ...)\n> \n> > entries.write(&entry);\n> > \n> > store(info, values);\n> > @@ -493,12 +496,18 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > \n> > /* If we're using a local id map, populate it. */\n> > if (localIdMap) {\n> > + ControlId::DirectionFlags flags;\n> > + if (entry->direction & static_cast(ControlId::Direction::In))\n> > + flags |= ControlId::Direction::In;\n> > + if (entry->direction & static_cast(ControlId::Direction::Out))\n> > + flags |= ControlId::Direction::Out;\n> \n> \n> and here - can't we just do a direct usage? or is there an issue with\n> serialising 'Flags' ?\n> \n> > /**\n> > * \\todo Find a way to preserve the control name for\n> > * debugging purpose.\n> > */\n> > controlIds_.emplace_back(std::make_unique(entry->id,\n> > - \"\", \"local\", type));\n> > + \"\", \"local\", type,\n> > + flags));\n> > (*localIdMap)[entry->id] = controlIds_.back().get();\n> > }\n> > \n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 2efecf0fc52b..23b71fac8e84 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -396,15 +396,16 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> > * \\param[in] name The control name\n> > * \\param[in] vendor The vendor name\n> > * \\param[in] type The control data type\n> > + * \\param[in] direction The direction of the control, if it can be used in Controls or Metadata\n> > * \\param[in] size The size of the array control, or 0 if scalar control\n> > * \\param[in] enumStrMap The map from enum names to values (optional)\n> > */\n> > ControlId::ControlId(unsigned int id, const std::string &name,\n> > const std::string &vendor, ControlType type,\n> > - std::size_t size,\n> > + DirectionFlags direction, std::size_t size,\n> > const std::map &enumStrMap)\n> > - : id_(id), name_(name), vendor_(vendor), type_(type), size_(size),\n> > - enumStrMap_(enumStrMap)\n> > + : id_(id), name_(name), vendor_(vendor), type_(type),\n> > + direction_(direction), size_(size), enumStrMap_(enumStrMap)\n> > {\n> > for (const auto &pair : enumStrMap_)\n> > reverseMap_[pair.second] = pair.first;\n> > @@ -434,6 +435,26 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > * \\return The control data type\n> > */\n> > \n> > +/**\n> > + * \\fn bool ControlId::isInput() const\n> > + * \\brief Determine if the control is available to be used as an input control\n> > + *\n> > + * Controls can be used either as input in controls, or as output in metadata.\n> > + * This function checks if the control is allowed to be used as the former.\n> > + *\n> > + * \\return True if the control can be used as an input control, false otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn bool ControlId::isOutput() const\n> > + * \\brief Determine if the control is available to be used in output metadata\n> > + *\n> > + * Controls can be used either as input in controls, or as output in metadata.\n> > + * This function checks if the control is allowed to be used as the latter.\n> > + *\n> > + * \\return True if the control can be returned in output metadata, false otherwise\n> > + */\n> > +\n> > /**\n> > * \\fn bool ControlId::isArray() const\n> > * \\brief Determine if the control is an array control\n> > @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > * \\return True if \\a lhs.id() is equal to \\a rhs, false otherwise\n> > */\n> > \n> > +/**\n> > + * \\enum ControlId::Direction\n> > + * \\brief The direction the control is capable of being passed from/to\n> > + *\n> > + * \\var ControlId::Direction::In\n> > + * \\brief The control can be passed as input in controls\n> > + *\n> > + * \\var ControlId::Direction::Out\n> > + * \\brief The control can be returned as output in metadata\n> > + */\n> > +\n> > +/**\n> > + * \\typedef ControlId::DirectionFlags\n> > + * \\brief A wrapper for ControlId::Direction so that it can be used as flags\n> > + */\n> > +\n> > /**\n> > * \\class Control\n> > * \\brief Describe a control and its intrinsic properties\n> > @@ -504,6 +541,8 @@ ControlId::ControlId(unsigned int id, const std::string &name,\n> > * \\param[in] id The control numerical ID\n> > * \\param[in] name The control name\n> > * \\param[in] vendor The vendor name\n> > + * \\param[in] direction The direction of the control, if it can be used in\n> > + * Controls or Metadata\n> > * \\param[in] enumStrMap The map from enum names to values (optional)\n> > *\n> > * The control data type is automatically deduced from the template type T.\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 7d21cf15fec1..4634ccc392b6 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -520,7 +520,15 @@ std::unique_ptr V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &\n> > const std::string name(static_cast(ctrl.name), len);\n> > const ControlType type = v4l2CtrlType(ctrl.type);\n> > \n> > - return std::make_unique(ctrl.id, name, \"v4l2\", type);\n> > + ControlId::DirectionFlags flags;\n> \n> I expect it's fine, but the above is uninitialised,\n> \n> > + if (ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY)\n> > + flags |= ControlId::Direction::Out;\n> \n> So you shouldn't be 'or'ing in here, just assign directly.\n> \n> Same for the two below.\n> \n> > + else if (ctrl.flags & V4L2_CTRL_FLAG_WRITE_ONLY)\n> > + flags |= ControlId::Direction::In;\n> > + else\n> > + flags |= ControlId::Direction::In | ControlId::Direction::Out;\n> > +\n> > + return std::make_unique(ctrl.id, name, \"v4l2\", type, flags);\n> > }\n> > \n> > /**\n> > -- \n> > 2.39.2\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4ACF8BE080\n\tfor ;\n\tMon, 9 Dec 2024 11:00:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E894166132;\n\tMon, 9 Dec 2024 12:00:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A29866132\n\tfor ;\n\tMon, 9 Dec 2024 12:00:51 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:a56c:f299:b6d:3bb8])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D6C5189A;\n\tMon, 9 Dec 2024 12:00:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hWqWm0XQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733742019;\n\tbh=9rcItPdEstPjt+j7U/2pTthPTULMo2VUHcliJDp+kMI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hWqWm0XQudFK+iWNnN5s6OwoDZxZobV5lahal0SQdpCd8xalyuGnMXw6GnoXHpWSK\n\tj8fJQpFewH/2qlK2dRc0xwejAZQGzAxlaQDdv7K97Tpu6xCvQXBJ1buaHPItlZUpJH\n\tlo51q629280DRctdNPVD3dsfgmIKUH9KrgFQfru8=","Date":"Mon, 9 Dec 2024 20:00:41 +0900","From":"Paul Elder ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 3/4] libcamera: controls: Add support for querying\n\tdirection information","Message-ID":"","References":"<20241129091916.298359-1-paul.elder@ideasonboard.com>\n\t<20241129091916.298359-4-paul.elder@ideasonboard.com>\n\t<173348994905.3135963.16363381439149852001@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<173348994905.3135963.16363381439149852001@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]