[{"id":24804,"web_url":"https://patchwork.libcamera.org/comment/24804/","msgid":"","date":"2022-08-28T18:08:39","subject":"Re: [libcamera-devel] [PATCH v3.1 4/7] utils: ipc: Add support for\n\tFlags","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, Aug 26, 2022 at 11:54:53AM -0500, Paul Elder via libcamera-devel wrote:\n> Add Flags as a supported type in the IPA interface.\n> \n> It is used in mojom with the [flags] attribute. Any field or parameter\n> type E that is prefixed with the [flags] attribute will direct the code\n> generator to generate the type name \"Flags\" and appropriate\n> serialization/deserialization code for Flags instead of for E.\n> \n> It is usable and has been tested in struct members, function input and\n> output parameters, and Signal parameters. This does not add support for\n> returning Flags as direct return values.\n> \n> Additionally, the [scopedEnum] attribute can be used on enum\n> definitions, which will instruct the code generator to convert it to an\n> enum class instead of a raw enum.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v3.1:\n> - add documentation of [scopedEnum] and [flags] to core.mojom\n> - it's not the best place, but better somewhere than nowhere for now\n> \n> Changes in v3:\n> - change flags attribute from [Flags] to [flags]\n> - make it so that enum input parameters are passed directly (as opposed\n> to const references)\n> - to turn enum definitions in mojom to enum classes, use the\n> [scopedEnum] attribute (as opposed to the [Flags] attribute in v2)\n> - correspondingly, the function in the mojom generator python script\n> is separated out from IsFlags() into IsScoped()\n> - clean up IsFlags()\n> - simplify GetFullNameForElement() for flags\n> \n> Question for reviewers (in v2): Should we use a different attribute name to\n> specify that an enum should be an enum class?\n> \n> Answer (in v3): Yes; that attribute name is [scopedEnum]\n> \n> Changes in v2:\n> - Use mojom attribute to specify that a field should be Flags,\n> instead of using a magic type name format\n> ---\n> include/libcamera/ipa/core.mojom | 9 ++++++\n> include/libcamera/ipa/ipa_interface.h | 1 +\n> .../definition_functions.tmpl | 2 +-\n> .../libcamera_templates/proxy_functions.tmpl | 10 +++++--\n> .../libcamera_templates/serializer.tmpl | 4 +++\n> .../generators/mojom_libcamera_generator.py | 29 +++++++++++++++++--\n> 6 files changed, 48 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom\n> index 74f3339e..ef28ff2d 100644\n> --- a/include/libcamera/ipa/core.mojom\n> +++ b/include/libcamera/ipa/core.mojom\n> @@ -33,6 +33,15 @@ module libcamera;\n> * available for the type and there's no need to generate one\n> * - hasFd - struct fields or empty structs only\n> * - Designate that this field or empty struct contains a SharedFD\n> + * - scopedEnum - enum definitions\n> + * - Designate that this enum should be an enum class, as opposed to a pure\n> + * enum\n> + * - flags - struct fields or function parameters that are enums\n> + * - Designate that this enum type E should be Flags in the generated C++\n> + * code\n> + * - For example, if a struct field is defined as `[flags] ErrorFlag f;`\n> + * (where ErrorFlag is defined as an enum elsewhere in mojom), then the\n> + * generated code for this field will be `Flags f`\n> *\n> * Rules:\n> * - If the type is defined in a libcamera C++ header *and* a (de)serializer is\n> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h\n> index 50ca0e7b..8afcfe21 100644\n> --- a/include/libcamera/ipa/ipa_interface.h\n> +++ b/include/libcamera/ipa/ipa_interface.h\n> @@ -13,6 +13,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> diff --git a/utils/ipc/generators/libcamera_templates/definition_functions.tmpl b/utils/ipc/generators/libcamera_templates/definition_functions.tmpl\n> index 94bb4918..8b8509f3 100644\n> --- a/utils/ipc/generators/libcamera_templates/definition_functions.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/definition_functions.tmpl\n> @@ -9,7 +9,7 @@\n> # \\param enum Enum object whose definition is to be generated\n> #}\n> {%- macro define_enum(enum) -%}\n> -enum {{enum.mojom_name}} {\n> +enum{{\" class\" if enum|is_scoped}} {{enum.mojom_name}} {\n> {%- for field in enum.fields %}\n> \t{{field.mojom_name}} = {{field.numeric_value}},\n> {%- endfor %}\n> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> index cbcfb64a..2be65d43 100644\n> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> @@ -62,7 +62,9 @@\n> {%- else %}\n> \tstd::tie({{param.mojom_name}}Buf, std::ignore) =\n> {%- endif %}\n> -{%- if param|is_enum %}\n> +{%- if param|is_flags %}\n> +\t\tIPADataSerializer<{{param|name_full}}>::serialize({{param.mojom_name}}\n> +{%- elif param|is_enum %}\n> \t\tIPADataSerializer::serialize(static_cast({{param.mojom_name}})\n> {%- else %}\n> \t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}\n> @@ -105,7 +107,9 @@\n> #}\n> {%- macro deserialize_param(param, pointer, loop, buf, fds, iter, data_size) -%}\n> {{\"*\" if pointer}}{{param.mojom_name}} =\n> -{%- if param|is_enum %}\n> +{%- if param|is_flags %}\n> +IPADataSerializer<{{param|name_full}}>::deserialize(\n> +{%- elif param|is_enum %}\n> static_cast<{{param|name_full}}>(IPADataSerializer::deserialize(\n> {%- else %}\n> IPADataSerializer<{{param|name}}>::deserialize(\n> @@ -133,7 +137,7 @@ IPADataSerializer<{{param|name}}>::deserialize(\n> {%- if param|needs_control_serializer %}\n> \t&controlSerializer_\n> {%- endif -%}\n> -){{\")\" if param|is_enum}};\n> +){{\")\" if param|is_enum and not param|is_flags}};\n> {%- endmacro -%}\n> \n> \n> diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> index 77bae36f..eec75211 100644\n> --- a/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> @@ -34,6 +34,8 @@\n> \t\tstd::tie({{field.mojom_name}}, std::ignore) =\n> \t{%- if field|is_pod %}\n> \t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}});\n> +\t{%- elif field|is_flags %}\n> +\t\t\tIPADataSerializer<{{field|name_full}}>::serialize(data.{{field.mojom_name}});\n> \t{%- elif field|is_enum %}\n> \t\t\tIPADataSerializer::serialize(data.{{field.mojom_name}});\n> \t{%- endif %}\n> @@ -96,6 +98,8 @@\n> \t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}}\n> \t\t{%- if field|is_pod %}\n> \t\tret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}});\n> +\t\t{%- elif field|is_flags %}\n> +\t\tret.{{field.mojom_name}} = IPADataSerializer<{{field|name_full}}>::deserialize(m, m + {{field_size}});\n> \t\t{%- else %}\n> \t\tret.{{field.mojom_name}} = static_cast<{{field|name_full}}>(IPADataSerializer::deserialize(m, m + {{field_size}}));\n> \t\t{%- endif %}\n> diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py\n> index 753bfc73..d7fe2031 100644\n> --- a/utils/ipc/generators/mojom_libcamera_generator.py\n> +++ b/utils/ipc/generators/mojom_libcamera_generator.py\n> @@ -74,6 +74,8 @@ def GetDefaultValue(element):\n> return element.default\n> if type(element.kind) == mojom.Kind:\n> return '0'\n> + if IsFlags(element):\n> + return ''\n> if mojom.IsEnumKind(element.kind):\n> return f'static_cast<{element.kind.mojom_name}>(0)'\n> if isinstance(element.kind, mojom.Struct) and \\\n> @@ -184,7 +186,7 @@ def MethodParameters(method):\n> params = []\n> for param in method.parameters:\n> params.append('const %s %s%s' % (GetNameForElement(param),\n> - '&' if not IsPod(param) else '',\n> + '' if IsPod(param) or (IsEnum(param) and not IsFlags(param)) else '&',\n\nThe Flags class wraps an integer, it can be passed by value too.\n\nAll the rest looks good,\n\nReviewed-by: Laurent Pinchart \n\n> param.mojom_name))\n> for param in MethodParamOutputs(method):\n> params.append(f'{GetNameForElement(param)} *{param.mojom_name}')\n> @@ -220,9 +222,21 @@ def IsControls(element):\n> def IsEnum(element):\n> return mojom.IsEnumKind(element.kind)\n> \n> +def IsScoped(element):\n> + attributes = getattr(element, 'attributes', None)\n> + if not attributes:\n> + return False\n> + return 'scopedEnum' in attributes\n> +\n> def IsFd(element):\n> return mojom.IsStructKind(element.kind) and element.kind.mojom_name == \"SharedFD\"\n> \n> +def IsFlags(element):\n> + attributes = getattr(element, 'attributes', None)\n> + if not attributes:\n> + return False\n> + return 'flags' in attributes\n> +\n> def IsMap(element):\n> return mojom.IsMapKind(element.kind)\n> \n> @@ -251,9 +265,11 @@ def ByteWidthFromCppType(t):\n> raise Exception('invalid type')\n> return str(int(_bit_widths[key]) // 8)\n> \n> -\n> # Get the type name for a given element\n> def GetNameForElement(element):\n> + # Flags\n> + if IsFlags(element):\n> + return f'Flags<{GetFullNameForElement(element.kind)}>'\n> # structs\n> if (mojom.IsEnumKind(element) or\n> mojom.IsInterfaceKind(element) or\n> @@ -302,7 +318,8 @@ def GetNameForElement(element):\n> def GetFullNameForElement(element):\n> name = GetNameForElement(element)\n> namespace_str = ''\n> - if mojom.IsStructKind(element):\n> + if (mojom.IsStructKind(element) or\n> + mojom.IsEnumKind(element)):\n> namespace_str = element.module.mojom_namespace.replace('.', '::')\n> elif (hasattr(element, 'kind') and\n> (mojom.IsStructKind(element.kind) or\n> @@ -311,6 +328,10 @@ def GetFullNameForElement(element):\n> \n> if namespace_str == '':\n> return name\n> +\n> + if IsFlags(element):\n> + return GetNameForElement(element)\n> +\n> return f'{namespace_str}::{name}'\n> \n> def ValidateZeroLength(l, s, cap=True):\n> @@ -408,9 +429,11 @@ class Generator(generator.Generator):\n> 'is_controls': IsControls,\n> 'is_enum': IsEnum,\n> 'is_fd': IsFd,\n> + 'is_flags': IsFlags,\n> 'is_map': IsMap,\n> 'is_plain_struct': IsPlainStruct,\n> 'is_pod': IsPod,\n> + 'is_scoped': IsScoped,\n> 'is_str': IsStr,\n> 'method_input_has_fd': MethodInputHasFd,\n> 'method_output_has_fd': MethodOutputHasFd,","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 5BE5FC3272\n\tfor ;\n\tSun, 28 Aug 2022 18:08:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6425561FC0;\n\tSun, 28 Aug 2022 20:08:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B707E61FBD\n\tfor ;\n\tSun, 28 Aug 2022 20:08:47 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0929DE5;\n\tSun, 28 Aug 2022 20:08:46 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661710129;\n\tbh=KPpbsd8lr1tXGUT8Upz7W2ZM89t9CUA9gxdPaMN5hDA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=jP+qDW3g1up9oVdltVuTXBKwRwt4Cwf/tiFgVIXMPxu95gxv1rqwQNPTpI8SwICUe\n\tNphCfW0tMo8Ryjg7RjeO+Sxu4ynu95mSJHOwhj2nt27jsaikcrOCzktECh/zNB9zF3\n\t3k05DsPIQ3Cli7U5P7e0cau1nzpFapoeFmqi8p96NoVmdzMCed2VGf2rMo5GBc1q2G\n\twxDIQCAoGwg2IEcnvmqegUp3+7/TlOnUWmBTT2mYdyaelyMgtXd5yumZT2y2kld0tS\n\tA38qLA+2k1NirZLN3lfz8h30JEX/5C8QCjmjR19lQMWIBojcU0W0ByWCtogZ3bwqp4\n\tUqxpsAwUY9Vzg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661710127;\n\tbh=KPpbsd8lr1tXGUT8Upz7W2ZM89t9CUA9gxdPaMN5hDA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=spe1/LvnhHovV3oHzW6TOuKHPdnwTKTX4jdVOyr2k63bGgAcoibW5J4ZhqTwZ2DGA\n\tG2ug7gXZRcb/OoQ50iA7z7+Kz7DC07TNyJyPw5IcBx1YjuPxl7hncN9CnwTC178NPu\n\tLDNuFXYA/nQeunEqu5wWd1iex/tIcevIk0psUp3E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"spe1/Lvn\"; dkim-atps=neutral","Date":"Sun, 28 Aug 2022 21:08:39 +0300","To":"Paul Elder ","Message-ID":"","References":"<20220826154419.4099372-5-paul.elder@ideasonboard.com>\n\t<20220826165453.135113-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220826165453.135113-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3.1 4/7] utils: ipc: Add support for\n\tFlags","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]