[{"id":12956,"web_url":"https://patchwork.libcamera.org/comment/12956/","msgid":"<20201004155640.GD8774@pendragon.ideasonboard.com>","date":"2020-10-04T15:56:40","subject":"Re: [libcamera-devel] [PATCH v3 06/38] utils: ipc: add templates\n\tfor code generation for IPC mechanism","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, Oct 02, 2020 at 11:31:22PM +0900, Paul Elder wrote:\n> Add templates to mojo to generate code for the IPC mechanism. These\n> templates generate:\n> - module header\n> - module serializer\n> - IPA proxy cpp, header, and worker\n> \n> Given an input data definition mojom file for a pipeline.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v3:\n> - add support for namespaces\n> - fix enum assignment (used to have +1 for CMD applied to all enums)\n> - use readHeader, writeHeader, and eraseHeader as static class functions\n> of IPAIPCUnixSocket (in the proxy worker)\n> - add requirement that base controls *must* be defined in\n> libcamera::{pipeline_name}::Controls\n> \n> Changes in v2:\n> - mandate the main and callback interfaces, and init(), start(), stop()\n> and their parameters\n> - fix returning single pod value from IPC-called function\n> - add licenses\n> - improve auto-generated message\n> - other fixes related to serdes\n> ---\n> .../module_generated.h.tmpl | 106 ++++\n\nThis isn't a great name, as _generated tells how the header is produced,\nbut not what it actually is. How about module_ipa_interface.h.tmpl ?\n\n> .../module_ipa_proxy.cpp.tmpl | 233 +++++++++\n> .../module_ipa_proxy.h.tmpl | 117 +++++\n> .../module_ipa_proxy_worker.cpp.tmpl | 184 +++++++\n> .../module_serializer.h.tmpl | 44 ++\n\nAnd maybe module_ipa_serializer.h.tmpl for consistency ?\n\n> .../libcamera_templates/proxy_functions.tmpl | 185 +++++++\n> .../libcamera_templates/serializer.tmpl | 276 +++++++++++\n> .../generators/mojom_libcamera_generator.py | 461 ++++++++++++++++++\n> 8 files changed, 1606 insertions(+)\n> create mode 100644 utils/ipc/generators/libcamera_templates/module_generated.h.tmpl\n> create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> create mode 100644 utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl\n> create mode 100644 utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> create mode 100644 utils/ipc/generators/libcamera_templates/serializer.tmpl\n> create mode 100644 utils/ipc/generators/mojom_libcamera_generator.py\n> \n> diff --git a/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl\n> new file mode 100644\n> index 00000000..7dd48b0b\n> --- /dev/null\n> +++ b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl\n> @@ -0,0 +1,106 @@\n> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) {{ year }}, Google Inc.\n> + *\n> + * {{ module_name }}_generated.h - Image Processing Algorithm interface for {{ module_name }}\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#ifndef __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__\n> +#define __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__\n> +\n> +#include \n> +\n\nI think you can drop this blank line.\n\n> +#include \n> +\n> +{% if has_map %}#include {% endif %}\n> +{% if has_array %}#include {% endif %}\n> +\n> +namespace libcamera {\n> +{%- if has_namespace %}\n> +{% for ns in namespace %}\n> +namespace {{ns}} {\n\nThere's a mix of {{ var }} and {{var}}. I don't mind much which one is\nused, but I think we should be consistent.\n\n> +{% endfor %}\n> +{%- endif %}\n> +\n> +enum {{ cmd_enum_name }} {\n\nIt would be nice to make this an enum class, to avoid namespace clashes.\nAlternatively, we could enforce the usage of namespaces for all IPAs.\n\nenum class also brings the added benefit of catching improper usages,\nbut comes at a cost of requiring additional static_cast<> to convert\nfrom and to uint32_t.\n\n> +\tCMD_EXIT = 0,\n\nI'd drop the CMD_ prefix, especially if we use enum class, and use\nCamelCase for names instead of uppercase.\n\n> +{%- for method in interface_main.methods %}\n> +\tCMD_{{ method.mojom_name|upper }} = {{loop.index}},\n> +{%- endfor %}\n> +{%- set count = interface_main.methods|length -%}\n> +{%- for method in interface_cb.methods %}\n> +\tCMD_{{ method.mojom_name|upper }} = {{loop.index + count}},\n> +{%- endfor %}\n\nShould we split the enum in two, one for each interface ?\n\n> +};\n> +\n> +{% for enum in enums %}\n> +enum {{ enum.mojom_name }} {\n> +{%- for field in enum.fields %}\n> +\t{{ field.mojom_name }} = {{ field.numeric_value }},\n> +{%- endfor %}\n> +};\n> +{% endfor %}\n> +\n> +{%- for struct in structs_nonempty %}\n> +struct {{ struct.mojom_name }}\n> +{\n> +public:\n> +\t{{ struct.mojom_name }}() {%- if struct|has_default_fields %} :{% endif %}\n> +{%- for field in struct.fields|with_default_values -%}\n> +{{\" \" if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ \", \" if not loop.last }}\n> +{%- endfor %} {}\n\nThis generates really long lines. How about\n\n\t{{ struct.mojom_name }}() {%- if struct|has_default_fields %}\n\t\t:{% endif %}\n{%- for field in struct.fields|with_default_values -%}\n{{\" \" if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ \", \" if not loop.last }}\n{%- endfor %}\n\t{\n\t}\n\nWould be great to wrap the initializers line too, but I think that's\noverkill.\n\n> +\t~{{ struct.mojom_name }}() {}\n> +\n> +\t{{ struct.mojom_name }}(\n> +{%- for field in struct.fields -%}\n> +{{ field|name }} {{ field.mojom_name }}{{ \", \" if not loop.last }}\n> +{%- endfor -%}\n> +) :\n> +{%- for field in struct.fields -%}\n> +{{\" \" if loop.first}}{{ field.mojom_name}}_({{ field.mojom_name }}){{ \", \" if not loop.last }}\n> +{%- endfor %} {}\n\nSame here.\n\n> +{% for field in struct.fields %}\n> +\t{{ field|name }} {{ field.mojom_name }}_;\n> +{%- endfor %}\n> +};\n> +{% endfor %}\n> +\n> +{#-\n> +What to do about the #includes? Should we forward-declare, or\n> +require {{module_name}}.h to include the required headers?\n> +For now I'm going for the latter, coz this header essentially\n> +extends {{module_name}}.h.\n> +#}\n\nLooking at the existing {{module_name}}.h, there are a few enums and\nconstants that could be moved to {{module_name}}_generated.h, such as\nenum BufferMark or MaxLsGridSize in raspberrypi.h. What do you think ?\n\nThen we have libcamera::RPi::Controls, which I'm wondering how to handle\nbest. It's not strictly related to this series, but wouldn't they be\nbetter stored in the pipeline handler, and passed to the IPA ?\n\nFinally there's the odd beast such as\n\n#define VIMC_IPA_FIFO_PATH \"/tmp/libcamera_ipa_vimc_fifo\"\n\nwhich I also wonder if it would make sense to pass it to the IPA at init\ntime. {{module_name}}.h could then possibly be dropped.\n\n> +class {{ interface_name }} : public IPAInterface\n> +{\n> +public:\n> +\tvirtual ~{{interface_name}}() {};\n\nNo need for the trailing ;\n\n> +{% for method in interface_main.methods %}\n> +\tvirtual {{method|method_return_value}} {{method.mojom_name}}(\n> +{%- for param in method|method_parameters %}\n> +\t\t{{param}}{{- \",\" if not loop.last}}\n> +{%- endfor -%}\n> +) = 0;\n> +{% endfor %}\n> +\n> +{%- for method in interface_cb.methods %}\n> +\tSignal<\n> +{%- for param in method.parameters -%}\n> +\t\tconst {{param|name}}{{\" &\" if not param|is_pod}}\n\nShould the const be dropped if param|is_pod ? Same below.\n\n> +\t\t{{- \", \" if not loop.last }}\n> +{%- endfor -%}\n> +> {{method.mojom_name}};\n> +{% endfor -%}\n> +};\n> +\n> +{%- if has_namespace %}\n> +{% for ns in namespace|reverse %}\n> +} /* {{ns}} */\n> +{% endfor %}\n> +{%- endif %}\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__ */\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> new file mode 100644\n> index 00000000..f3eeeaaa\n> --- /dev/null\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> @@ -0,0 +1,233 @@\n> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> +{%- import \"proxy_functions.tmpl\" as proxy_funcs -%}\n> +\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) {{ year }}, Google Inc.\n> + *\n> + * ipa_proxy_{{ module_name }}.cpp - Image Processing Algorithm proxy for {{ module_name }}\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#include \n> +\n> +#include \n\nI think you can drop this, as it's included by {{module_name}}.h. Same\nbelow.\n\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/control_serializer.h\"\n> +#include \"libcamera/internal/ipa_ipc.h\"\n> +#include \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> +#include \"libcamera/internal/ipa_data_serializer.h\"\n> +#include \"libcamera/internal/ipa_module.h\"\n> +#include \"libcamera/internal/ipa_proxy.h\"\n> +#include \"libcamera/internal/ipc_unixsocket.h\"\n> +#include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/process.h\"\n> +#include \"libcamera/internal/thread.h\"\n> +\n> +#include \n\nShould this be moved above with the other libcamera/ipa/ headers ?\n\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(IPAProxy)\n> +\n> +{%- if has_namespace %}\n> +{% for ns in namespace %}\n> +namespace {{ns}} {\n> +{% endfor %}\n> +{%- endif %}\n> +\n> +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> +\t: IPAProxy(ipam), running_(false),\n> +\t isolate_(isolate)\n> +{\n> +\tLOG(IPAProxy, Debug)\n> +\t\t<< \"initializing dummy proxy: loading IPA from \"\n\nIt's not a dummy proxy anymore :-)\n\n> +\t\t<< ipam->path();\n> +\n> +\tif (isolate_) {\n> +\t\tconst std::string proxy_worker_path = resolvePath(\"ipa_proxy_{{module_name}}\");\n> +\t\tif (proxy_worker_path.empty()) {\n> +\t\t\tLOG(IPAProxy, Error)\n> +\t\t\t\t<< \"Failed to get proxy worker path\";\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tipc_ = std::make_unique(ipam->path().c_str(), proxy_worker_path.c_str());\n> +\t\tif (!ipc_->isValid()) {\n> +\t\t\tLOG(IPAProxy, Error) << \"Failed to create IPAIPC\";\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +{% if interface_cb.methods|length > 0 %}\n> +\t\tipc_->recvIPC.connect(this, &{{proxy_name}}::recvIPC);\n> +{% endif %}\n> +\n> +\t\tvalid_ = true;\n> +\t\treturn;\n> +\t}\n\nThere's very little code shared between the isolated and non-isolated\npaths. It could make sense to split this class in two. On the other\nhand, that would require the ability for the IPAManager to create the\nright class type, which may complicate things a bit, but maybe not much.\nFor instance we could have the two classes called {{proxy_name}}Thread\nand {{proxy_name}}IPC, and have a {{proxy_name}} class defined as\n\nclass {{proxy_name}}\n{\npublic:\n\tusing IPC = {{proxy_name}}IPC;\n\tusing Thread = {{proxy_name}}Thread;\n};\n\nThen, in IPAManager::createIPA(), you would have\n\n\tstd::unique_ptr

proxy;\n\tif (self_->isSignatureValid(m)))\n\t\tproxy = std::make_unique(m);\n\telse\n\t\tproxy = std::make_unique(m);\n\n> +\n> +\tif (!ipam->load())\n> +\t\treturn;\n> +\n> +\tIPAInterface *ipai = ipam->createInterface();\n> +\tif (!ipai) {\n> +\t\tLOG(IPAProxy, Error)\n> +\t\t\t<< \"Failed to create IPA context for \" << ipam->path();\n> +\t\treturn;\n> +\t}\n> +\n> +\tipa_ = std::unique_ptr<{{interface_name}}>(dynamic_cast<{{interface_name}} *>(ipai));\n> +\tproxy_.setIPA(ipa_.get());\n> +\n> +{% for method in interface_cb.methods %}\n> +\tipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);\n> +{%- endfor %}\n> +\n> +\tvalid_ = true;\n> +}\n> +\n> +{{proxy_name}}::~{{proxy_name}}()\n> +{\n> +\tif (isolate_)\n> +\t\tipc_->sendAsync(CMD_EXIT, {}, {});\n> +}\n> +\n> +{% if interface_cb.methods|length > 0 %}\n> +void {{proxy_name}}::recvIPC(std::vector &data, std::vector &fds)\n> +{\n> +\tuint32_t cmd = (data[0] & 0xff) |\n> +\t\t ((data[1] & 0xff) << 8) |\n> +\t\t ((data[2] & 0xff) << 16) |\n> +\t\t ((data[3] & 0xff) << 24);\n\nAs data is a vector of uint8_t, I think the & 0xff is unnecessary.\n\nShould the data length be validated ?\n\n> +\n> +\t/* Need to skip another 4 bytes for the sequence number. */\n> +\tstd::vector vec(data.begin() + 8, data.end());\n\nThis creates a copy, which isn't very nice :-/ I think the IPC class\ninterface would benefit from switching from std::vector to Span.\n\n> +\tswitch (cmd) {\n> +{%- for method in interface_cb.methods %}\n> +\tcase CMD_{{method.mojom_name|upper}}: {\n> +\t\t{{method.mojom_name}}IPC(vec, fds);\n> +\t\tbreak;\n> +\t}\n> +{%- endfor %}\n\nShould we log unknown commands ?\n\n> +\t}\n> +}\n> +{%- endif %}\n> +\n> +{% for method in interface_main.methods %}\n> +{{proxy_funcs.func_sig(proxy_name, method)}}\n> +{\n> +\tif (isolate_)\n> +\t\t{{\"return \" if method|method_return_value != \"void\"}}{{method.mojom_name}}IPC(\n> +{%- for param in method|method_param_names -%}\n> +\t\t{{param}}{{- \", \" if not loop.last}}\n> +{%- endfor -%}\n> +);\n> +\telse\n> +\t\t{{\"return \" if method|method_return_value != \"void\"}}{{method.mojom_name}}Thread(\n> +{%- for param in method|method_param_names -%}\n> +\t\t{{param}}{{- \", \" if not loop.last}}\n> +{%- endfor -%}\n> +);\n> +}\n> +\n> +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> +{\n> +{%- if method.mojom_name == \"init\" %}\n> +\t{{proxy_funcs.init_thread_body()}}\n> +{% elif method.mojom_name == \"start\" %}\n\nI think you're missing a - before elif, here and below.\n\n> +\t{{proxy_funcs.start_thread_body()}}\n> +{% elif method.mojom_name == \"stop\" %}\n> +\t{{proxy_funcs.stop_thread_body()}}\n\nInstead of using macros, would it make sense to move the code to the\nbase IPAProxy class ?\n\n> +{% elif not method|is_async %}\n> +\tipa_->{{method.mojom_name}}(\n> +\t{%- for param in method|method_param_names -%}\n> +\t\t{{param}}{{- \", \" if not loop.last}}\n> +\t{%- endfor -%}\n> +);\n> +{% elif method|is_async %}\n> +\tproxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,\n> +\t{%- for param in method|method_param_names -%}\n> +\t\t{{param}}{{- \", \" if not loop.last}}\n> +\t{%- endfor -%}\n> +);\n> +{%- endif %}\n> +}\n> +\n> +{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n> +{\n> +{%- set has_input = true if method|method_param_inputs|length > 0 %}\n> +{%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != \"void\" %}\n> +{% if has_input %}\n\nShouldn't it be {%- ?\n\n> +\tstd::vector _ipcInputBuf;\n> +{%- if method|method_input_has_fd %}\n> +\tstd::vector _ipcInputFds;\n> +{%- endif %}\n> +{%- endif %}\n> +{%- if has_output %}\n> +\tstd::vector _ipcOutputBuf;\n> +{%- if method|method_output_has_fd %}\n> +\tstd::vector _ipcOutputFds;\n> +{%- endif %}\n> +{%- endif %}\n> +\n> +{{proxy_funcs.serialize_call(method|method_param_inputs, '_ipcInputBuf', '_ipcInputFds', base_controls)}}\n> +\n> +{%- set input_buf = \"_ipcInputBuf\" if has_input else \"{}\" %}\n> +{%- set fds_buf = \"_ipcInputFds\" if method|method_input_has_fd else \"{}\" %}\n> +{%- set cmd = \"CMD_\" + method.mojom_name|upper %}\n> +{% if method|is_async %}\n> +\tint ret = ipc_->sendAsync({{cmd}}, {{input_buf}}, {{fds_buf}});\n> +{%- else %}\n> +\tint ret = ipc_->sendSync({{cmd}}, {{input_buf}}, {{fds_buf}}\n> +{{- \", &_ipcOutputBuf\" if has_output -}}\n> +{{- \", &_ipcOutputFds\" if has_output and method|method_output_has_fd -}}\n> +);\n> +{%- endif %}\n> +\tif (ret < 0) {\n> +\t\tLOG(IPAProxy, Error) << \"Failed to call {{method.mojom_name}}\";\n> +{%- if method|method_return_value != \"void\" %}\n> +\t\treturn static_cast<{{method|method_return_value}}>(ret);\n> +{%- else %}\n> +\t\treturn;\n> +{%- endif %}\n> +\t}\n> +\tLOG(IPAProxy, Debug) << \"Done calling {{method.mojom_name}}\";\n\nIs this message helpful for debugging ? I think we should work at some\npoint on a tracing mechanism instead of manually tracing calls.\n\n> +{% if method|method_return_value != \"void\" %}\n> +\treturn IPADataSerializer<{{method.response_parameters|first|name}}>::deserialize(_ipcOutputBuf, 0);\n> +{% elif method|method_param_outputs|length > 0 %}\n> +{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf', '_ipcOutputFds')}}\n> +{% endif -%}\n> +}\n> +\n> +{% endfor %}\n> +\n> +\n\nOne blank line should be enough.\n\n> +{% for method in interface_cb.methods %}\n> +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> +{\n> +\t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> +}\n> +\n> +void {{proxy_name}}::{{method.mojom_name}}IPC(\n> +\tstd::vector &data,\n> +\t[[maybe_unused]] std::vector &fds)\n> +{ \n> +{%- for param in method.parameters %}\n> +\t{{param|name}} {{param.mojom_name}};\n> +{%- endfor %}\n> +{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false)}}\n> +\t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> +}\n> +{% endfor %}\n> +\n> +{%- if has_namespace %}\n> +{% for ns in namespace|reverse %}\n> +} /* {{ns}} */\n> +{% endfor %}\n> +{%- endif %}\n> +} /* namespace libcamera */\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> new file mode 100644\n> index 00000000..7d2bfb6c\n> --- /dev/null\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> @@ -0,0 +1,117 @@\n> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> +{%- import \"proxy_functions.tmpl\" as proxy_funcs -%}\n> +\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) {{ year }}, Google Inc.\n> + *\n> + * ipa_proxy_{{ module_name }}.h - Image Processing Algorithm proxy for {{ module_name }}\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#ifndef __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__\n> +#define __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/control_serializer.h\"\n> +#include \"libcamera/internal/ipa_ipc.h\"\n> +#include \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> +#include \"libcamera/internal/ipa_proxy.h\"\n> +#include \"libcamera/internal/ipc_unixsocket.h\"\n> +#include \"libcamera/internal/thread.h\"\n> +\n> +namespace libcamera {\n> +{%- if has_namespace %}\n> +{% for ns in namespace %}\n> +namespace {{ns}} {\n> +{% endfor %}\n> +{%- endif %}\n> +\n> +class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n> +{\n> +public:\n> +\t{{proxy_name}}(IPAModule *ipam, bool isolate);\n> +\t~{{proxy_name}}();\n> +\n> +{% for method in interface_main.methods %}\n> +{{proxy_funcs.func_sig(proxy_name, method, \"\", false, true)|indent(8, true)}};\n> +{% endfor %}\n> +\n> +{%- for method in interface_cb.methods %}\n> +\tSignal<\n> +{%- for param in method.parameters -%}\n> +\t\tconst {{param|name}}{{\" &\" if not param|is_pod}}\n> +\t\t{{- \", \" if not loop.last }}\n> +{%- endfor -%}\n> +> {{method.mojom_name}};\n> +{% endfor %}\n> +\n> +private:\n> +\tvoid recvIPC(std::vector &data, std::vector &fds);\n> +\n> +{% for method in interface_main.methods %}\n> +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> +{{proxy_funcs.func_sig(proxy_name, method, \"IPC\", false)|indent(8, true)}};\n> +{% endfor %}\n> +{% for method in interface_cb.methods %}\n> +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> +\tvoid {{method.mojom_name}}IPC(\n> +\t\tstd::vector &data,\n> +\t\tstd::vector &fds);\n\nShould these be const ?\n\n> +{% endfor %}\n> +\n> +\t/* Helper class to invoke async functions in another thread. */\n> +\tclass ThreadProxy : public Object\n> +\t{\n> +\tpublic:\n> +\t\tvoid setIPA({{interface_name}} *ipa)\n> +\t\t{\n> +\t\t\tipa_ = ipa;\n> +\t\t}\n> +\n> +\t\tint start()\n> +\t\t{\n> +\t\t\treturn ipa_->start();\n> +\t\t}\n> +\n> +\t\tvoid stop()\n> +\t\t{\n> +\t\t\tipa_->stop();\n> +\t\t}\n> +{% for method in interface_main.methods %}\n> +{%- if method|is_async %}\n> +\t\t{{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n> +\t\t{\n> +\t\t\tipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> +\t\t}\n> +{%- endif %}\n> +{%- endfor %}\n\nI think this is mixing two concepts, which shouldn't cause any issue for\nnow, but may come bite us back later.\n\nAs you've noticed in the existing IPAProxyThread implementation, we call\nsome methods directly, and proxy other methods across threads. The idea\nis that anything called before start() is a direct call, and anything\nfrom start() until but not including stop() is indirect. The reason is\nthat before start() there's no thread running, so we can't make a\ncross-thread call.\n\nOne direct consequence of this architecture is that all calls before\nstart() are synchronous, and can thus return a value. Calls after\nstart() are asynchronous, but that's a separate design decision. We\ncould synchronize cross-thread calls if we wanted, we have chosen not to\nin order to avoid delays that could damage the real time performance of\nthe pipeline handler.\n\nWith IPC the situation is different, all calls are cross-process. Still,\nwe want to expose a similar behaviour to pipeline handlers, so calls\nbefore start() are synchronous, and further calls are asynchronous.\n\nI don't think we can easily enforce this through the IDL, but I believe\nit would make sense to document this requirements, so that authors will\nnot create an async method meant to be called before start(), or a\nsynchronous method after start().\n\n> +\n> +\tprivate:\n> +\t\t{{interface_name}} *ipa_;\n> +\t};\n> +\n> +\tbool running_;\n> +\tThread thread_;\n> +\tThreadProxy proxy_;\n> +\tstd::unique_ptr<{{interface_name}}> ipa_;\n> +\n> +\tconst bool isolate_;\n> +\n> +\tstd::unique_ptr ipc_;\n> +\n> +\tControlSerializer controlSerializer_;\n> +};\n> +\n> +{%- if has_namespace %}\n> +{% for ns in namespace|reverse %}\n> +} /* {{ns}} */\n> +{% endfor %}\n> +{%- endif %}\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__ */\n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> new file mode 100644\n> index 00000000..1647a9ca\n> --- /dev/null\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> @@ -0,0 +1,184 @@\n> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> +{%- import \"proxy_functions.tmpl\" as proxy_funcs -%}\n> +\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) {{ year }}, Google Inc.\n> + *\n> + * ipa_proxy_{{ module_name }}_worker.cpp - Image Processing Algorithm proxy worker for {{ module_name }}\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/camera_sensor.h\"\n> +#include \"libcamera/internal/control_serializer.h\"\n> +#include \"libcamera/internal/ipa_data_serializer.h\"\n> +#include \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> +#include \"libcamera/internal/ipa_module.h\"\n> +#include \"libcamera/internal/ipa_proxy.h\"\n> +#include \"libcamera/internal/ipc_unixsocket.h\"\n> +#include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/thread.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY({{proxy_name}}Worker)\n> +\n> +{%- if has_namespace %}\n> +{% for ns in namespace -%}\n> +using namespace {{ns}};\n> +{% endfor %}\n> +{%- endif %}\n> +\n> +struct CallData {\n> +\tIPCUnixSocket::Payload *response;\n> +\tbool done;\n> +};\n> +\n> +{{interface_name}} *ipa_;\n> +IPCUnixSocket socket_;\n> +std::map callData_;\n\ncallData_ doesn't seem to be used (and thus struct CallData shouldn't be\nneeded).\n\n> +\n> +ControlSerializer controlSerializer_;\n> +\n> +bool exit_ = false;\n\nShould you group all this, as well as the functions below, in a\n{{module_name}}ProxyWorker class ?\n\n> +\n> +void readyRead(IPCUnixSocket *socket)\n> +{\n> +\tIPCUnixSocket::Payload _message, _response;\n> +\tint _ret = socket->receive(&_message);\n> +\tif (_ret) {\n> +\t\tLOG({{proxy_name}}Worker, Error)\n> +\t\t\t<< \"Receive message failed\" << _ret;\n> +\t\treturn;\n> +\t}\n> +\n> +\tuint32_t _cmd, _seq;\n> +\tstd::tie(_cmd, _seq) = IPAIPCUnixSocket::readHeader(_message);\n> +\tIPAIPCUnixSocket::eraseHeader(_message);\n\nUsing Span would help avoiding copies.\n\n> +\n> +\tswitch (_cmd) {\n> +\tcase CMD_EXIT: {\n> +\t\texit_ = true;\n> +\t\tbreak;\n> +\t}\n> +\n> +{% for method in interface_main.methods %}\n> +\tcase CMD_{{method.mojom_name|upper}}: {\n> +\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_message.data', '_message.fds', false, true)|indent(8, true)}}\n> +{% for param in method|method_param_outputs %}\n> +\t\t{{param|name}} {{param.mojom_name}};\n> +{% endfor %}\n> +{%- if method|method_return_value != \"void\" %}\n> +\t\t{{method|method_return_value}} _callRet = ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> +{%- else %}\n> +\t\tipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}}\n> +{{- \", \" if method|method_param_outputs|params_comma_sep -}}\n> +{%- for param in method|method_param_outputs -%}\n> +&{{param.mojom_name}}{{\", \" if not loop.last}}\n> +{%- endfor -%}\n> +);\n> +{%- endif %}\n> +{% if not method|is_async %}\n> +\t\tIPAIPCUnixSocket::writeHeader(_response, _cmd, _seq);\n> +{%- if method|method_return_value != \"void\" %}\n> +\t\tstd::vector _callRetBuf;\n> +\t\tstd::tie(_callRetBuf, std::ignore) =\n> +\t\t\tIPADataSerializer<{{method|method_return_value}}>::serialize(_callRet);\n> +\t\t_response.data.insert(_response.data.end(), _callRetBuf.begin(), _callRetBuf.end());\n> +{%- else %}\n> +\t{{proxy_funcs.serialize_call(method|method_param_outputs, \"_response.data\", \"_response.fds\", base_controls)|indent(8, true)}}\n> +{%- endif %}\n> +\t\tint _ret = socket_.send(_response);\n> +\t\tif (_ret < 0) {\n> +\t\t\tLOG({{proxy_name}}Worker, Error)\n> +\t\t\t\t<< \"Reply to {{method.mojom_name}}() failed\" << _ret;\n> +\t\t}\n> +\t\tLOG({{proxy_name}}Worker, Debug) << \"Done replying to {{method.mojom_name}}()\";\n> +{%- endif %}\n> +\t\tbreak;\n> +\t}\n> +{% endfor %}\n\nThis isn't a huge deal, but the generated code my be a bit clearer if\neach case called a function.\n\n> +\t}\n> +}\n> +\n> +{% for method in interface_cb.methods %}\n> +{{proxy_funcs.func_sig(proxy_name, method, \"\", false)}}\n> +{\n> +\tIPCUnixSocket::Payload _message;\n> +\n> +\tIPAIPCUnixSocket::writeHeader(_message, CMD_{{method.mojom_name|upper}}, 0);\n> +\t{{proxy_funcs.serialize_call(method|method_param_inputs, \"_message.data\", \"_message.fds\", base_controls)}}\n> +\n> +\tsocket_.send(_message);\n> +\n> +\tLOG({{proxy_name}}Worker, Debug) << \"{{method.mojom_name}} done\";\n> +}\n> +{%- endfor %}\n\nYou can drop the -\n\n> +\n> +int main(int argc, char **argv)\n> +{\n> +\t/* Uncomment this for debugging. */\n> +#if 0\n> +\tstd::string logPath = \"/tmp/libcamera.worker.\" +\n> +\t\t\t std::to_string(getpid()) + \".log\";\n> +\tlogSetFile(logPath.c_str());\n> +#endif\n> +\n> +\tif (argc < 3) {\n> +\t\tLOG({{proxy_name}}Worker, Debug)\n\nMaybe s/Debug/Error/ ?\n\n> +\t\t\t<< \"Tried to start worker with no args\";\n> +\t\treturn EXIT_FAILURE;\n> +\t}\n> +\n> +\tint fd = std::stoi(argv[2]);\n> +\tLOG({{proxy_name}}Worker, Debug)\n\nAnd Info ?\n\n> +\t\t<< \"Starting worker for IPA module \" << argv[1]\n> +\t\t<< \" with IPC fd = \" << fd;\n> +\n> +\tstd::unique_ptr ipam = std::make_unique(argv[1]);\n> +\tif (!ipam->isValid() || !ipam->load()) {\n> +\t\tLOG({{proxy_name}}Worker, Error)\n> +\t\t\t<< \"IPAModule \" << argv[1] << \" should be valid but isn't\";\n\nMaybe just \" isn't valid\" ?\n\n> +\t\treturn EXIT_FAILURE;\n> +\t}\n> +\n> +\tif (socket_.bind(fd) < 0) {\n> +\t\tLOG({{proxy_name}}Worker, Error) << \"IPC socket binding failed\";\n> +\t\treturn EXIT_FAILURE;\n> +\t}\n> +\tsocket_.readyRead.connect(&readyRead);\n> +\n> +\tipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());\n> +\tif (!ipa_) {\n> +\t\tLOG({{proxy_name}}Worker, Error) << \"Failed to create IPA interface instance\";\n> +\t\treturn EXIT_FAILURE;\n> +\t}\n> +{% for method in interface_cb.methods %}\n> +\tipa_->{{method.mojom_name}}.connect(&{{method.mojom_name}});\n> +{%- endfor %}\n> +\n> +\tLOG({{proxy_name}}Worker, Debug) << \"Proxy worker successfully started\";\n> +\n> +\t/* \\todo upgrade listening loop */\n\nGood point. We should make an EventLoop class at some point.\n\n> +\tEventDispatcher *dispatcher = Thread::current()->eventDispatcher();\n> +\twhile (!exit_)\n> +\t\tdispatcher->processEvents();\n> +\n> +\tdelete ipa_;\n> +\tsocket_.close();\n> +\n> +\treturn 0;\n> +}\n> diff --git a/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl\n> new file mode 100644\n> index 00000000..37352c1f\n> --- /dev/null\n> +++ b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl\n> @@ -0,0 +1,44 @@\n> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> +{%- import \"serializer.tmpl\" as serializer -%}\n> +\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) {{ year }}, Google Inc.\n> + *\n> + * {{ module_name }}_serializer.h - Image Processing Algorithm data serializer for {{ module_name }}\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__\n> +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/control_serializer.h\"\n> +#include \"libcamera/internal/ipa_data_serializer.h\"\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(IPADataSerializer)\n> +{% for struct in structs_nonempty %}\n> +template<>\n> +class IPADataSerializer<{{ struct|name_full(namespace_str) }}>\n> +{\n> +public:\n> +{{- serializer.serializer(struct, base_controls, namespace_str) }}\n> +{%- if struct|has_fd %}\n> +{{ serializer.deserializer_fd(struct, namespace_str) }}\n> +{%- else %}\n> +{{ serializer.deserializer_no_fd(struct, namespace_str) }}\n> +{%- endif %}\n> +};\n> +{% endfor %}\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__ */\n\nI'll review proxy_functions.tmpl and serializer.tmpl separately, this is\ngrowing quite big.\n\n> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> new file mode 100644\n> index 00000000..e1f5a20c\n> --- /dev/null\n> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> @@ -0,0 +1,185 @@\n> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> +{#\n> + # \\brief Generate fuction prototype\n> + #\n> + # \\param class Class name\n> + # \\param method mojom Method object\n> + # \\param suffix Suffix to append to \\a method function name\n> + # \\param need_class_name True to generate class name with function\n> + # \\param override True to generate override tag after the function prototype\n> + #}\n> +{%- macro func_sig(class, method, suffix, need_class_name = true, override = false) -%}\n> +{{method|method_return_value}} {{ class + \"::\" if need_class_name }}{{method.mojom_name}}{{suffix}}(\n> +{%- for param in method|method_parameters %}\n> +\t{{param}}{{- \",\" if not loop.last}}\n> +{%- endfor -%}\n> +){{\" override\" if override}}\n> +{%- endmacro -%}\n> +\n> +{#\n> + # \\brief Generate function body for IPA init() function for thread\n> + #}\n> +{%- macro init_thread_body() -%}\n> +\tint ret = ipa_->init(settings);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tproxy_.moveToThread(&thread_);\n> +\n> +\treturn 0;\n> +{%- endmacro -%}\n> +\n> +{#\n> + # \\brief Generate function body for IPA start() function for thread\n> + #}\n> +{%- macro start_thread_body() -%}\n> +\trunning_ = true;\n> +\tthread_.start();\n> +\n> +\treturn proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);\n> +{%- endmacro -%}\n> +\n> +{#\n> + # \\brief Generate function body for IPA stop() function for thread\n> + #}\n> +{%- macro stop_thread_body() -%}\n> +\tif (!running_)\n> +\t\treturn;\n> +\n> +\trunning_ = false;\n> +\n> +\tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n> +\n> +\tthread_.exit();\n> +\tthread_.wait();\n> +{%- endmacro -%}\n> +\n> +\n> +{#\n> + # \\brief Serialize multiple objects into data buffer and fd vector\n> + #\n> + # Generate code to serialize multiple objects, as specified in \\a params\n> + # (which are the parameters to some function), into \\a buf data buffer and\n> + # \\a fds fd vector.\n> + # This code is meant to be used by the proxy, for serializing prior to IPC calls.\n> + #}\n> +{%- macro serialize_call(params, buf, fds, base_controls) %}\n> +{%- for param in params %}\n> +\tstd::vector {{param.mojom_name}}Buf;\n> +{%- if param|has_fd %}\n> +\tstd::vector {{param.mojom_name}}Fds;\n> +\tstd::tie({{param.mojom_name}}Buf, {{param.mojom_name}}Fds) =\n> +{%- else %}\n> +\tstd::tie({{param.mojom_name}}Buf, std::ignore) =\n> +{%- endif %}\n> +{%- if param|is_controls %}\n> +\t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}, {{param.mojom_name}}.infoMap() ? *{{param.mojom_name}}.infoMap() : {{base_controls}}\n> +{%- else %}\n> +\t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}\n> +{%- endif %}\n> +{{- \", &controlSerializer_\" if param|needs_control_serializer -}}\n> +);\n> +{%- endfor %}\n> +\n> +{%- if params|length > 1 %}\n> +{%- for param in params %}\n> +\tappendUInt({{buf}}, {{param.mojom_name}}Buf.size());\n> +{%- if param|has_fd %}\n> +\tappendUInt({{buf}}, {{param.mojom_name}}Fds.size());\n> +{%- endif %}\n> +{%- endfor %}\n> +{%- endif %}\n> +\n> +{%- for param in params %}\n> +\t{{buf}}.insert({{buf}}.end(), {{param.mojom_name}}Buf.begin(), {{param.mojom_name}}Buf.end());\n> +{%- endfor %}\n> +\n> +{%- for param in params %}\n> +{%- if param|has_fd %}\n> +\t{{fds}}.insert({{fds}}.end(), {{param.mojom_name}}Fds.begin(), {{param.mojom_name}}Fds.end());\n> +{%- endif %}\n> +{%- endfor %}\n> +{%- endmacro -%}\n> +\n> +\n> +{#\n> + # \\brief Deserialize a single object from data buffer and fd vector\n> + #\n> + # \\param pointer True deserializes the object into a dereferenced pointer\n> + #\n> + # Generate code to deserialize a single object, as specified in \\a param,\n> + # from \\a buf data buffer and \\a fds fd vector.\n> + # This code is meant to be used by macro deserialize_call.\n> + #}\n> +{%- macro deserialize_param(param, pointer, loop, buf, fds) -%}\n> +{{\"*\" if pointer}}{{param.mojom_name}} = IPADataSerializer<{{param|name}}>::deserialize(\n> +\t{{buf}}.begin() + {{param.mojom_name}}Start,\n> +{%- if loop.last %}\n> +\t{{buf}}.end()\n> +{%- else %}\n> +\t{{buf}}.begin() + {{param.mojom_name}}Start + {{param.mojom_name}}BufSize\n> +{%- endif -%}\n> +{{- \",\" if param|has_fd }}\n> +{%- if param|has_fd %}\n> +\t{{fds}}.begin() + {{param.mojom_name}}FdStart,\n> +{%- if loop.last %}\n> +\t{{fds}}.end()\n> +{%- else %}\n> +\t{{fds}}.begin() + {{param.mojom_name}}FdStart + {{param.mojom_name}}FdsSize\n> +{%- endif -%}\n> +{%- endif -%}\n> +{{- \",\" if param|needs_control_serializer }}\n> +{%- if param|needs_control_serializer %}\n> +\t&controlSerializer_\n> +{%- endif -%}\n> +);\n> +{%- endmacro -%}\n> +\n> +\n> +{#\n> + # \\brief Deserialize multiple objects from data buffer and fd vector\n> + #\n> + # \\param pointer True deserializes objects into pointers, and adds a null check.\n> + # \\param declare True declares the objects in addition to deserialization.\n> + #\n> + # Generate code to deserialize multiple objects, as specified in \\a params\n> + # (which are the parameters to some function), from \\a buf data buffer and\n> + # \\a fds fd vector.\n> + # This code is meant to be used by the proxy, for serializing prior to IPC calls.\n> + #}\n> +{%- macro deserialize_call(params, buf, fds, pointer = true, declare = false) -%}\n> +{% set ns = namespace(size_offset = 0) %}\n> +{%- if params|length > 1 %}\n> +{%- for param in params %}\n> +\t[[maybe_unused]] size_t {{param.mojom_name}}BufSize = readUInt({{buf}}, {{ns.size_offset}});\n> +\t{%- set ns.size_offset = ns.size_offset + 4 %}\n> +{%- if param|has_fd %}\n> +\t[[maybe_unused]] size_t {{param.mojom_name}}FdsSize = readUInt({{buf}}, {{ns.size_offset}});\n> +\t{%- set ns.size_offset = ns.size_offset + 4 %}\n> +{%- endif %}\n> +{%- endfor %}\n> +{%- endif %}\n> +{% for param in params %}\n> +{%- if loop.first %}\n> +\tsize_t {{param.mojom_name}}Start = {{ns.size_offset}};\n> +{%- else %}\n> +\tsize_t {{param.mojom_name}}Start = {{loop.previtem.mojom_name}}Start + {{loop.previtem.mojom_name}}BufSize;\n> +{%- endif %}\n> +{%- endfor %}\n> +{% for param in params|with_fds %}\n> +{%- if loop.first %}\n> +\tsize_t {{param.mojom_name}}FdStart = 0;\n> +{%- elif not loop.last %}\n> +\tsize_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;\n> +{%- endif %}\n> +{%- endfor %}\n> +{% for param in params %}\n> +\t{%- if pointer %}\n> +\tif ({{param.mojom_name}}) {\n> +{{deserialize_param(param, pointer, loop, buf, fds)|indent(16, True)}}\n> +\t}\n> +\t{%- else %}\n> +\t{{param|name + \" \" if declare}}{{deserialize_param(param, pointer, loop, buf, fds)|indent(8)}}\n> +\t{%- endif %}\n> +{% endfor %}\n> +{%- endmacro -%}\n> diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> new file mode 100644\n> index 00000000..3c071c4e\n> --- /dev/null\n> +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> @@ -0,0 +1,276 @@\n> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> +{# Turn this into a C macro? #}\n> +{#\n> + # \\brief Verify that there is enough bytes to deserialize\n> + #\n> + # Generate code that verifies that \\a size is not greater than \\a dataSize.\n> + # Otherwise log an error with \\a name and \\a typename.\n> + #}\n> +{%- macro check_data_size(size, dataSize, name, typename) %}\n> +\t\tif ({{size}} > {{dataSize}}) {\n> +\t\t\tLOG(IPADataSerializer, Error)\n> +\t\t\t\t<< \"Failed to deserialize {{name}}: not enough {{typename}}, expected \"\n> +\t\t\t\t<< ({{size}}) << \", got \" << ({{dataSize}});\n> +\t\t\treturn ret;\n> +\t\t}\n> +{%- endmacro %}\n> +\n> +\n> +{#\n> + # \\brief Serialize some field into return vector\n> + #\n> + # Generate code to serialize \\a field into ret_data, including size of the\n> + # field and fds (where appropriate). \\a base_controls indicates the\n> + # default ControlInfoMap in the event that the ControlList does not have one.\n> + # This code is meant to be used by the IPADataSerializer specialization.\n> + #}\n> +{%- macro serializer_field(field, base_controls, namespace, loop) %}\n> +{%- if field|is_pod or field|is_enum %}\n> +\t\tstd::vector {{field.mojom_name}};\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_enum %}\n> +\t\t\tIPADataSerializer::serialize(data.{{field.mojom_name}}_);\n> +\t{%- endif %}\n> +\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> +{%- elif field|is_fd %}\n> +\t\tstd::vector {{field.mojom_name}};\n> +\t\tstd::vector {{field.mojom_name}}Fds;\n> +\t\tstd::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =\n> +\t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);\n> +\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> +\t\tret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());\n> +{%- elif field|is_controls %}\n> +\t\tif (data.{{field.mojom_name}}_.size() > 0) {\n> +\t\t\tstd::vector {{field.mojom_name}};\n> +\t\t\tstd::tie({{field.mojom_name}}, std::ignore) =\n> +\t\t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_,\n> +\t\t\t\t\tdata.{{field.mojom_name}}_.infoMap() ? *data.{{field.mojom_name}}_.infoMap() : {{base_controls}},\n> +\t\t\t\t\tcs);\n> +\t\t\tappendUInt(ret_data, {{field.mojom_name}}.size());\n> +\t\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> +\t\t} else {\n> +\t\t\tappendUInt(ret_data, 0);\n> +\t\t}\n> +{%- elif field|is_plain_struct or field|is_array or field|is_map %}\n> +\t\tstd::vector {{field.mojom_name}};\n> +\t{%- if field|has_fd %}\n> +\t\tstd::vector {{field.mojom_name}}Fds;\n> +\t\tstd::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =\n> +\t{%- else %}\n> +\t\tstd::tie({{field.mojom_name}}, std::ignore) =\n> +\t{%- endif %}\n> +\t{%- if field|is_array or field|is_map %}\n> +\t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_, cs);\n> +\t{%- else %}\n> +\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::serialize(data.{{field.mojom_name}}_, cs);\n> +\t{%- endif %}\n> +\t\tappendUInt(ret_data, {{field.mojom_name}}.size());\n> +\t{%- if field|has_fd %}\n> +\t\tappendUInt(ret_data, {{field.mojom_name}}Fds.size());\n> +\t{%- endif %}\n> +\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> +\t{%- if field|has_fd %}\n> +\t\tret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());\n> +\t{%- endif %}\n> +{%- else %}\n> +\t\t/* Unknown serialization for {{field.mojom_name}}. */\n> +{%- endif %}\n> +{%- endmacro %}\n> +\n> +\n> +{#\n> + # \\brief Deserialize some field into return struct\n> + #\n> + # Generate code to deserialize \\a field into object ret.\n> + # This code is meant to be used by the IPADataSerializer specialization.\n> + #}\n> +{%- macro deserializer_field(field, namespace, loop) %}\n> +{% if field|is_pod or field|is_enum %}\n> +\t{%- set field_size = (field|bit_width|int / 8)|int %}\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}}_ = static_cast<{{field|name}}>(IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}));\n> +\t\t{%- else %}\n> +\t\tret.{{field.mojom_name}}_ = static_cast<{{field|name}}>(IPADataSerializer::deserialize(m, m + {{field_size}}));\n> +\t\t{%- endif %}\n> +\t{%- if not loop.last %}\n> +\t\tm += {{field_size}};\n> +\t\tdataSize -= {{field_size}};\n> +\t{%- endif %}\n> +{% elif field|is_fd %}\n> +\t{%- set field_size = 1 %}\n> +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}\n> +\t\tret.{{field.mojom_name}}_ = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs);\n> +\t{%- if not loop.last %}\n> +\t\tm += {{field_size}};\n> +\t\tdataSize -= {{field_size}};\n> +\t\tn += ret.{{field.mojom_name}}_.isValid() ? 1 : 0;\n> +\t\tfdsSize -= ret.{{field.mojom_name}}_.isValid() ? 1 : 0;\n> +\t{%- endif %}\n> +{% elif field|is_controls %}\n> +\t{%- set field_size = 4 %}\n> +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}\n> +\t\tsize_t {{field.mojom_name}}Size = readUInt(m);\n> +\t{%- set field_size = '4 + ' + field.mojom_name + 'Size' -%}\n> +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}\n> +\t\tif ({{field.mojom_name}}Size > 0)\n> +\t\t\tret.{{field.mojom_name}}_ =\n> +\t\t\t\tIPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);\n> +\t{%- if not loop.last %}\n> +\t\tm += {{field_size}};\n> +\t\tdataSize -= {{field_size}};\n> +\t{%- endif %}\n> +{% elif field|is_plain_struct or field|is_array or field|is_map%}\n> +\t{%- set field_size = 4 %}\n> +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}\n> +\t\tsize_t {{field.mojom_name}}Size = readUInt(m);\n> +\t{%- if field|has_fd %}\n> +\t{%- set field_size = 8 %}\n> +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'FdsSize', 'data') }}\n> +\t\tsize_t {{field.mojom_name}}FdsSize = readUInt(m + 4);\n> +\t\t{{- check_data_size(field.mojom_name + 'FdsSize', 'fdsSize', field.mojom_name, 'fds') }}\n> +\t{%- endif %}\n> +\t{%- set field_size = field|has_fd|choose('8 + ', '4 + ') + field.mojom_name + 'Size' -%}\n> +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}\n> +\t\tret.{{field.mojom_name}}_ =\n> +\t{%- if field|has_fd and (field|is_array or field|is_map) %}\n> +\t\t\tIPADataSerializer<{{field|name}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);\n> +\t{%- elif field|has_fd and (not (field|is_array or field|is_map)) %}\n> +\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);\n> +\t{%- elif (not field|has_fd) and (field|is_array or field|is_map) %}\n> +\t\t\tIPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);\n> +\t{%- else %}\n> +\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);\n> +\t{%- endif %}\n> +\t{%- if not loop.last %}\n> +\t\tm += {{field_size}};\n> +\t\tdataSize -= {{field_size}};\n> +\t{%- if field|has_fd %}\n> +\t\tn += {{field.mojom_name}}FdsSize;\n> +\t\tfdsSize -= {{field.mojom_name}}FdsSize;\n> +\t{%- endif %}\n> +\t{%- endif %}\n> +{% else %}\n> +\t\t/* Unknown deserialization for {{field.mojom_name}}. */\n> +{%- endif %}\n> +{%- endmacro %}\n> +\n> +\n> +{#\n> + # \\brief Serialize a struct\n> + #\n> + # Generate code for IPADataSerializer specialization, for serializing\n> + # \\a struct. \\a base_controls indicates the default ControlInfoMap\n> + # in the event that the ControlList does not have one.\n> + #}\n> +{%- macro serializer(struct, base_controls, namespace) %}\n> +\tstatic std::tuple, std::vector>\n> +\tserialize(const {{ struct|name_full(namespace) }} &data,\n> +{%- if struct|needs_control_serializer %}\n> +\t\t ControlSerializer *cs)\n> +{%- else %}\n> +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +{%- endif %}\n> +\t{\n> +\t\tstd::vector ret_data;\n> +{%- if struct|has_fd %}\n> +\t\tstd::vector ret_fds;\n> +{%- endif %}\n> +{%- for field in struct.fields %}\n> +{{serializer_field(field, base_controls, namespace, loop)}}\n> +{%- endfor %}\n> +{% if struct|has_fd %}\n> +\t\treturn {ret_data, ret_fds};\n> +{%- else %}\n> +\t\treturn {ret_data, {}};\n> +{%- endif %}\n> +\t}\n> +{%- endmacro %}\n> +\n> +\n> +{#\n> + # \\brief Deserialize a struct that has fds\n> + #\n> + # Generate code for IPADataSerializer specialization, for deserializing\n> + # \\a struct, in the case that \\a struct has file descriptors.\n> + #}\n> +{%- macro deserializer_fd(struct, namespace) %}\n> +\tstatic {{ struct|name_full(namespace) }}\n> +\tdeserialize(std::vector &data,\n> +\t\t std::vector &fds,\n> +{%- if struct|needs_control_serializer %}\n> +\t\t ControlSerializer *cs)\n> +{%- else %}\n> +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +{%- endif %}\n> +\t{\n> +\t\t{{struct|name_full(namespace)}} ret;\n> +\t\tstd::vector::iterator m = data.begin();\n> +\t\tstd::vector::iterator n = fds.begin();\n> +\n> +\t\tsize_t dataSize = data.size();\n> +\t\tsize_t fdsSize = fds.size();\n> +{%- for field in struct.fields -%}\n> +{{deserializer_field(field, namespace, loop)}}\n> +{%- endfor %}\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tstatic {{ struct|name_full(namespace) }}\n> +\tdeserialize(std::vector::iterator data_it1,\n> +\t\t std::vector::iterator data_it2,\n> +\t\t std::vector::iterator fds_it1,\n> +\t\t std::vector::iterator fds_it2,\n> +{%- if struct|needs_control_serializer %}\n> +\t\t ControlSerializer *cs)\n> +{%- else %}\n> +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +{%- endif %}\n> +\t{\n> +\t\tstd::vector data(data_it1, data_it2);\n> +\t\tstd::vector fds(fds_it1, fds_it2);\n> +\t\treturn IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, fds, cs);\n> +\t}\n> +{%- endmacro %}\n> +\n> +\n> +{#\n> + # \\brief Deserialize a struct that has no fds\n> + #\n> + # Generate code for IPADataSerializer specialization, for deserializing\n> + # \\a struct, in the case that \\a struct does not have file descriptors.\n> + #}\n> +{%- macro deserializer_no_fd(struct, namespace) %}\n> +\tstatic {{ struct|name_full(namespace) }}\n> +\tdeserialize(std::vector &data,\n> +{%- if struct|needs_control_serializer %}\n> +\t\t ControlSerializer *cs)\n> +{%- else %}\n> +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +{%- endif %}\n> +\t{\n> +\t\t{{struct|name_full(namespace)}} ret;\n> +\t\tstd::vector::iterator m = data.begin();\n> +\n> +\t\tsize_t dataSize = data.size();\n> +{%- for field in struct.fields -%}\n> +{{deserializer_field(field, namespace, loop)}}\n> +{%- endfor %}\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tstatic {{ struct|name_full(namespace) }}\n> +\tdeserialize(std::vector::iterator data_it1,\n> +\t\t std::vector::iterator data_it2,\n> +{%- if struct|needs_control_serializer %}\n> +\t\t ControlSerializer *cs)\n> +{%- else %}\n> +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> +{%- endif %}\n> +\t{\n> +\t\tstd::vector data(data_it1, data_it2);\n> +\t\treturn IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, cs);\n> +\t}\n> +{%- endmacro %}\n> diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py\n> new file mode 100644\n> index 00000000..a9269c82\n> --- /dev/null\n> +++ b/utils/ipc/generators/mojom_libcamera_generator.py\n> @@ -0,0 +1,461 @@\n> +'''Generates libcamera files from a mojom.Module.'''\n> +\n> +import argparse\n> +import ast\n> +import datetime\n> +import contextlib\n> +import os\n> +import re\n> +import shutil\n> +import sys\n> +import tempfile\n> +\n> +from jinja2 import contextfilter\n> +\n> +import mojom.fileutil as fileutil\n> +import mojom.generate.generator as generator\n> +import mojom.generate.module as mojom\n> +from mojom.generate.template_expander import UseJinja\n> +\n> +\n> +GENERATOR_PREFIX = 'libcamera'\n> +\n> +_kind_to_cpp_type = {\n> + mojom.BOOL: 'bool',\n> + mojom.INT8: 'int8_t',\n> + mojom.UINT8: 'uint8_t',\n> + mojom.INT16: 'int16_t',\n> + mojom.UINT16: 'uint16_t',\n> + mojom.INT32: 'int32_t',\n> + mojom.UINT32: 'uint32_t',\n> + mojom.FLOAT: 'float',\n> + mojom.INT64: 'int64_t',\n> + mojom.UINT64: 'uint64_t',\n> + mojom.DOUBLE: 'double',\n> +}\n> +\n> +_bit_widths = {\n> + mojom.BOOL: '8',\n> + mojom.INT8: '8',\n> + mojom.UINT8: '8',\n> + mojom.INT16: '16',\n> + mojom.UINT16: '16',\n> + mojom.INT32: '32',\n> + mojom.UINT32: '32',\n> + mojom.FLOAT: '32',\n> + mojom.INT64: '64',\n> + mojom.UINT64: '64',\n> + mojom.DOUBLE: '64',\n> +}\n> +\n> +def ModuleName(path):\n> + return path.split('/')[-1].split('.')[0]\n> +\n> +def ModuleClassName(module):\n> + s = re.sub(r'^IPA', '', module.interfaces[0].mojom_name)\n> + return re.sub(r'Interface$', '', s)\n> +\n> +def UpperCamelCase(name):\n> + return ''.join([x.capitalize() for x in generator.SplitCamelCase(name)])\n> +\n> +def CamelCase(name):\n> + uccc = UpperCamelCase(name)\n> + return uccc[0].lower() + uccc[1:]\n> +\n> +def ConstantStyle(name):\n> + return generator.ToUpperSnakeCase(name)\n> +\n> +def Choose(cond, t, f):\n> + return t if cond else f\n> +\n> +def CommaSep(l):\n> + return ', '.join([m for m in l])\n> +\n> +def ParamsCommaSep(l):\n> + return ', '.join([m.mojom_name for m in l])\n> +\n> +def GetDefaultValue(element):\n> + if element.default is not None:\n> + return element.default\n> + if type(element.kind) == mojom.Kind:\n> + return '0'\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> + element.kind.mojom_name == 'FileDescriptor'):\n> + return '-1'\n> + return ''\n> +\n> +def WithDefaultValues(element):\n> + return [x for x in element if HasDefaultValue(x)]\n> +\n> +def WithFds(element):\n> + return [x for x in element if HasFd(x)]\n> +\n> +def HasDefaultValue(element):\n> + return GetDefaultValue(element) != ''\n> +\n> +def HasDefaultFields(element):\n> + return True in [HasDefaultValue(x) for x in element.fields]\n> +\n> +def GetAllTypes(element):\n> + if mojom.IsArrayKind(element):\n> + return GetAllTypes(element.kind)\n> + if mojom.IsMapKind(element):\n> + return GetAllTypes(element.key_kind) + GetAllTypes(element.value_kind)\n> + if isinstance(element, mojom.Parameter):\n> + return GetAllTypes(element.kind)\n> + if mojom.IsEnumKind(element):\n> + return [element.mojom_name]\n> + if not mojom.IsStructKind(element):\n> + return [element.spec]\n> + if len(element.fields) == 0:\n> + return [element.mojom_name]\n> + ret = [GetAllTypes(x.kind) for x in element.fields]\n> + ret = [x for sublist in ret for x in sublist]\n> + return list(set(ret))\n> +\n> +def GetAllAttrs(element):\n> + if mojom.IsArrayKind(element):\n> + return GetAllAttrs(element.kind)\n> + if mojom.IsMapKind(element):\n> + return {**GetAllAttrs(element.key_kind), **GetAllAttrs(element.value_kind)}\n> + if isinstance(element, mojom.Parameter):\n> + return GetAllAttrs(element.kind)\n> + if mojom.IsEnumKind(element):\n> + return element.attributes if element.attributes is not None else {}\n> + if mojom.IsStructKind(element) and len(element.fields) == 0:\n> + return element.attributes if element.attributes is not None else {}\n> + if not mojom.IsStructKind(element):\n> + return {}\n> + attrs = [(x.attributes) for x in element.fields]\n> + ret = {}\n> + for d in attrs:\n> + if d is not None:\n> + ret = {**ret, **d}\n> + return ret\n> +\n> +def NeedsControlSerializer(element):\n> + types = GetAllTypes(element)\n> + return \"ControlList\" in types or \"ControlInfoMap\" in types\n> +\n> +def HasFd(element):\n> + attrs = GetAllAttrs(element)\n> + if isinstance(element, mojom.Kind):\n> + types = GetAllTypes(element)\n> + else:\n> + types = GetAllTypes(element.kind)\n> + return \"FileDescriptor\" in types or (attrs is not None and \"hasFd\" in attrs)\n> +\n> +def MethodInputHasFd(method):\n> + if len([x for x in method.parameters if HasFd(x)]) > 0:\n> + return True\n> + return False\n> +\n> +def MethodOutputHasFd(method):\n> + if (MethodReturnValue(method) != 'void' or\n> + method.response_parameters is None):\n> + return False\n> + if len([x for x in method.parameters if HasFd(x)]) > 0:\n> + return True\n> + return False\n> +\n> +def MethodParamInputs(method):\n> + return method.parameters\n> +\n> +def MethodParamOutputs(method):\n> + if (MethodReturnValue(method) != 'void' or\n> + method.response_parameters is None):\n> + return []\n> + return method.response_parameters\n> +\n> +def MethodParamNames(method):\n> + params = []\n> + for param in method.parameters:\n> + params.append(param.mojom_name)\n> + if MethodReturnValue(method) == 'void':\n> + if method.response_parameters is None:\n> + return params\n> + for param in method.response_parameters:\n> + params.append(param.mojom_name)\n> + return params\n> +\n> +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> + param.mojom_name))\n> + if MethodReturnValue(method) == 'void':\n> + if method.response_parameters is None:\n> + return params\n> + for param in method.response_parameters:\n> + params.append(f'{GetNameForElement(param)} *{param.mojom_name}')\n> + return params\n> +\n> +def MethodReturnValue(method):\n> + if method.response_parameters is None:\n> + return 'void'\n> + if len(method.response_parameters) == 1 and IsPod(method.response_parameters[0]):\n> + return GetNameForElement(method.response_parameters[0])\n> + return 'void'\n> +\n> +def IsAsync(method):\n> + # callbacks are always async\n> + if re.match(\"^IPA.*CallbackInterface$\", method.interface.mojom_name):\n> + return True\n> + elif re.match(\"^IPA.*Interface$\", method.interface.mojom_name):\n> + if method.attributes is None:\n> + return False\n> + elif 'async' in method.attributes and method.attributes['async']:\n> + return True\n> + return False\n> +\n> +def IsArray(element):\n> + return mojom.IsArrayKind(element.kind)\n> +\n> +def IsControls(element):\n> + return mojom.IsStructKind(element.kind) and (element.kind.mojom_name == \"ControlList\" or\n> + element.kind.mojom_name == \"ControlInfoMap\")\n> +\n> +def IsEnum(element):\n> + return mojom.IsEnumKind(element.kind)\n> +\n> +def IsFd(element):\n> + return mojom.IsStructKind(element.kind) and element.kind.mojom_name == \"FileDescriptor\"\n> +\n> +def IsMap(element):\n> + return mojom.IsMapKind(element.kind)\n> +\n> +def IsPlainStruct(element):\n> + return mojom.IsStructKind(element.kind) and not IsControls(element) and not IsFd(element)\n> +\n> +def IsPod(element):\n> + return element.kind in _kind_to_cpp_type\n> +\n> +def BitWidth(element):\n> + if element.kind in _bit_widths:\n> + return _bit_widths[element.kind]\n> + if mojom.IsEnumKind(element.kind):\n> + return '32'\n> + return ''\n> +\n> +def GetNameForElement(element):\n> + if (mojom.IsEnumKind(element) or\n> + mojom.IsInterfaceKind(element) or\n> + mojom.IsStructKind(element)):\n> + return element.mojom_name\n> + if (mojom.IsArrayKind(element)):\n> + elem_name = GetNameForElement(element.kind)\n> + return f'std::vector<{elem_name}>'\n> + if (mojom.IsMapKind(element)):\n> + key_name = GetNameForElement(element.key_kind)\n> + value_name = GetNameForElement(element.value_kind)\n> + return f'std::map<{key_name}, {value_name}>'\n> + if isinstance(element, (mojom.Field, mojom.Method, mojom.Parameter)):\n> + if (mojom.IsArrayKind(element.kind) or mojom.IsMapKind(element.kind)):\n> + return GetNameForElement(element.kind)\n> + if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):\n> + return _kind_to_cpp_type[element.kind]\n> + return element.kind.mojom_name\n> + if isinstance(element, mojom.EnumValue):\n> + return (GetNameForElement(element.enum) + '.' +\n> + ConstantStyle(element.name))\n> + if isinstance(element, (mojom.NamedValue,\n> + mojom.Constant,\n> + mojom.EnumField)):\n> + return ConstantStyle(element.name)\n> + if (hasattr(element, '__hash__') and element in _kind_to_cpp_type):\n> + return _kind_to_cpp_type[element]\n> + if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):\n> + return _kind_to_cpp_type[element.kind]\n> + if (hasattr(element, 'spec')):\n> + return element.spec.split(':')[-1]\n> + if (mojom.IsInterfaceRequestKind(element) or\n> + mojom.IsAssociatedKind(element) or\n> + mojom.IsPendingRemoteKind(element) or\n> + mojom.IsPendingReceiverKind(element) or\n> + mojom.IsUnionKind(element)):\n> + raise Exception('Unsupported element: %s' % element)\n> + raise Exception('Unexpected element: %s' % element)\n> +\n> +def GetFullNameForElement(element, namespace_str):\n> + name = GetNameForElement(element)\n> + if namespace_str == '':\n> + return name\n> + return f'{namespace_str}::{name}'\n> +\n> +def ValidateZeroLength(l, s, cap=True):\n> + if len(l) > 0:\n> + raise Exception(f'{s.capitalize() if cap else s} should be empty')\n> +\n> +def ValidateSingleLength(l, s, cap=True):\n> + if len(l) > 1:\n> + raise Exception(f'Only one {s} allowed')\n> + if len(l) < 1:\n> + raise Exception(f'{s.capitalize() if cap else s} is required')\n> +\n> +def ValidateInterfaces(interfaces):\n> + # Validate presence of main interface\n> + intf = [x for x in interfaces\n> + if re.match(\"^IPA.*Interface\", x.mojom_name) and\n> + not re.match(\"^IPA.*CallbackInterface\", x.mojom_name)]\n> + ValidateSingleLength(intf, 'main interface')\n> +\n> + # Validate presence of callback interface\n> + cb = [x for x in interfaces if re.match(\"^IPA.*CallbackInterface\", x.mojom_name)]\n> + ValidateSingleLength(cb, 'callback interface')\n> +\n> + # Validate required main interface functions\n> + intf = intf[0]\n> + f_init = [x for x in intf.methods if x.mojom_name == 'init']\n> + f_start = [x for x in intf.methods if x.mojom_name == 'start']\n> + f_stop = [x for x in intf.methods if x.mojom_name == 'stop']\n> +\n> + ValidateSingleLength(f_init, 'init()', False)\n> + ValidateSingleLength(f_start, 'start()', False)\n> + ValidateSingleLength(f_stop, 'stop()', False)\n> +\n> + f_init = f_init[0]\n> + f_start = f_start[0]\n> + f_stop = f_stop[0]\n> +\n> + # Validate parameters to init()\n> + ValidateSingleLength(f_init.parameters, 'input parameter to init()')\n> + ValidateSingleLength(f_init.response_parameters, 'output parameter from init()')\n> + if f_init.parameters[0].kind.mojom_name != 'IPASettings':\n> + raise Exception('init() must have single IPASettings input parameter')\n> + if f_init.response_parameters[0].kind.spec != 'i32':\n> + raise Exception('init() must have single int32 output parameter')\n> +\n> + # Validate parameters to start()\n> + ValidateZeroLength(f_start.parameters, 'input parameter to start()')\n> + ValidateSingleLength(f_start.response_parameters, 'output parameter from start()')\n> + if f_start.response_parameters[0].kind.spec != 'i32':\n> + raise Exception('start() must have single int32 output parameter')\n> +\n> + # Validate parameters to stop()\n> + ValidateZeroLength(f_stop.parameters, 'input parameter to stop()')\n> + ValidateZeroLength(f_stop.parameters, 'output parameter from stop()')\n> +\n> +class Generator(generator.Generator):\n> +\n> + @staticmethod\n> + def GetTemplatePrefix():\n> + return 'libcamera_templates'\n> +\n> + def GetFilters(self):\n> + libcamera_filters = {\n> + 'all_types': GetAllTypes,\n> + 'bit_width': BitWidth,\n> + 'choose': Choose,\n> + 'comma_sep': CommaSep,\n> + 'default_value': GetDefaultValue,\n> + 'has_default_fields': HasDefaultFields,\n> + 'has_fd': HasFd,\n> + 'is_async': IsAsync,\n> + 'is_array': IsArray,\n> + 'is_controls': IsControls,\n> + 'is_enum': IsEnum,\n> + 'is_fd': IsFd,\n> + 'is_map': IsMap,\n> + 'is_plain_struct': IsPlainStruct,\n> + 'is_pod': IsPod,\n> + 'method_input_has_fd': MethodInputHasFd,\n> + 'method_output_has_fd': MethodOutputHasFd,\n> + 'method_param_names': MethodParamNames,\n> + 'method_param_inputs': MethodParamInputs,\n> + 'method_param_outputs': MethodParamOutputs,\n> + 'method_parameters': MethodParameters,\n> + 'method_return_value': MethodReturnValue,\n> + 'name': GetNameForElement,\n> + 'name_full': GetFullNameForElement,\n> + 'needs_control_serializer': NeedsControlSerializer,\n> + 'params_comma_sep': ParamsCommaSep,\n> + 'with_default_values': WithDefaultValues,\n> + 'with_fds': WithFds,\n> + }\n> + return libcamera_filters\n> +\n> + def _GetJinjaExports(self):\n> + return {\n> + 'base_controls': '%s::Controls' % ModuleClassName(self.module),\n> + 'cmd_enum_name': '_%sCMD' % ModuleClassName(self.module),\n\ns/CMD/Command/ ?\n\n> + 'enums': self.module.enums,\n> + 'has_array': len([x for x in self.module.kinds.keys() if x[0] == 'a']) > 0,\n> + 'has_map': len([x for x in self.module.kinds.keys() if x[0] == 'm']) > 0,\n> + 'has_namespace': self.module.mojom_namespace != '',\n> + 'imports': self.module.imports,\n> + 'interface_cb': self.module.interfaces[1],\n> + 'interface_main': self.module.interfaces[0],\n> + 'interface_name': 'IPA%sInterface' % ModuleClassName(self.module),\n> + 'ipc_name': 'IPAIPCUnixSocket',\n> + 'kinds': self.module.kinds,\n> + 'module': self.module,\n> + 'module_name': ModuleName(self.module.path),\n> + 'module_class_name': ModuleClassName(self.module),\n> + 'namespace': self.module.mojom_namespace.split('.'),\n> + 'namespace_str': self.module.mojom_namespace.replace('.', '::') if\n> + self.module.mojom_namespace is not None else '',\n> + 'proxy_name': 'IPAProxy%s' % ModuleClassName(self.module),\n> + 'proxy_worker_name': 'IPAProxy%sWorker' % ModuleClassName(self.module),\n> + 'structs_nonempty': [x for x in self.module.structs if len(x.fields) > 0],\n> + 'year': datetime.datetime.now().year,\n> + }\n> +\n> +\n> + @UseJinja('module_generated.h.tmpl')\n> + def _GenerateDataHeader(self):\n> + return self._GetJinjaExports()\n> +\n> + @UseJinja('module_serializer.h.tmpl')\n> + def _GenerateSerializer(self):\n> + return self._GetJinjaExports()\n> +\n> + @UseJinja('module_ipa_proxy.cpp.tmpl')\n> + def _GenerateProxyCpp(self):\n> + return self._GetJinjaExports()\n> +\n> + @UseJinja('module_ipa_proxy.h.tmpl')\n> + def _GenerateProxyHeader(self):\n> + return self._GetJinjaExports()\n> +\n> + @UseJinja('module_ipa_proxy_worker.cpp.tmpl')\n> + def _GenerateProxyWorker(self):\n> + return self._GetJinjaExports()\n> +\n> + def GenerateFiles(self, unparsed_args):\n> + parser = argparse.ArgumentParser()\n> + parser.add_argument('--libcamera_generate_header', action='store_true')\n> + parser.add_argument('--libcamera_generate_serializer', action='store_true')\n> + parser.add_argument('--libcamera_generate_proxy_cpp', action='store_true')\n> + parser.add_argument('--libcamera_generate_proxy_h', action='store_true')\n> + parser.add_argument('--libcamera_generate_proxy_worker', action='store_true')\n> + parser.add_argument('--libcamera_output_path')\n> + args = parser.parse_args(unparsed_args)\n> +\n> + ValidateInterfaces(self.module.interfaces)\n> +\n> + fileutil.EnsureDirectoryExists(os.path.dirname(args.libcamera_output_path))\n> +\n> + module_name = ModuleName(self.module.path)\n> +\n> + if args.libcamera_generate_header:\n> + self.Write(self._GenerateDataHeader(),\n> + args.libcamera_output_path)\n> +\n> + if args.libcamera_generate_serializer:\n> + self.Write(self._GenerateSerializer(),\n> + args.libcamera_output_path)\n> +\n> + if args.libcamera_generate_proxy_cpp:\n> + self.Write(self._GenerateProxyCpp(),\n> + args.libcamera_output_path)\n> +\n> + if args.libcamera_generate_proxy_h:\n> + self.Write(self._GenerateProxyHeader(),\n> + args.libcamera_output_path)\n> +\n> + if args.libcamera_generate_proxy_worker:\n> + self.Write(self._GenerateProxyWorker(),\n> + args.libcamera_output_path)","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 313A0C3B5C\n\tfor ;\n\tSun, 4 Oct 2020 15:57:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 994376229F;\n\tSun, 4 Oct 2020 17:57:20 +0200 (CEST)","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 9F2286035B\n\tfor ;\n\tSun, 4 Oct 2020 17:57:19 +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 CF6B13B;\n\tSun, 4 Oct 2020 17:57:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"i5LkhckU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601827039;\n\tbh=1E6rWlwA76myI0gP2gP7L4Gb5aoHHp9mxVr29ypHepE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=i5LkhckULnzxUswX5j4n3byW0fT9Evj/UpTBVcWd0xJWrrTVHjSBpMdADA/NpK8Up\n\t0s1HNKpz1AyneROSviBKFae5SX6novzEOuGvkLj1UqGPavwRr/2YbMg8No1hnBfQw7\n\txCfannybGdcKRyNBhTrxRTwF0Qza/r4YzXV3KVg4=","Date":"Sun, 4 Oct 2020 18:56:40 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20201004155640.GD8774@pendragon.ideasonboard.com>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201002143154.468162-7-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 06/38] utils: ipc: add templates\n\tfor code generation for IPC mechanism","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12969,"web_url":"https://patchwork.libcamera.org/comment/12969/","msgid":"<20201005100857.GH45948@pyrite.rasen.tech>","date":"2020-10-05T10:08:57","subject":"Re: [libcamera-devel] [PATCH v3 06/38] utils: ipc: add templates\n\tfor code generation for IPC mechanism","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Sun, Oct 04, 2020 at 06:56:40PM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Fri, Oct 02, 2020 at 11:31:22PM +0900, Paul Elder wrote:\n> > Add templates to mojo to generate code for the IPC mechanism. These\n> > templates generate:\n> > - module header\n> > - module serializer\n> > - IPA proxy cpp, header, and worker\n> > \n> > Given an input data definition mojom file for a pipeline.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v3:\n> > - add support for namespaces\n> > - fix enum assignment (used to have +1 for CMD applied to all enums)\n> > - use readHeader, writeHeader, and eraseHeader as static class functions\n> > of IPAIPCUnixSocket (in the proxy worker)\n> > - add requirement that base controls *must* be defined in\n> > libcamera::{pipeline_name}::Controls\n> > \n> > Changes in v2:\n> > - mandate the main and callback interfaces, and init(), start(), stop()\n> > and their parameters\n> > - fix returning single pod value from IPC-called function\n> > - add licenses\n> > - improve auto-generated message\n> > - other fixes related to serdes\n> > ---\n> > .../module_generated.h.tmpl | 106 ++++\n> \n> This isn't a great name, as _generated tells how the header is produced,\n> but not what it actually is. How about module_ipa_interface.h.tmpl ?\n\nI named it like that because that's what the generated headers end up\ngetting called. There's the main header, like raspberrypi.h, and then\nthere's the generated one, like raspberrypi_generated.h.\n\nraspberrypi_ipa_interface.h ...?\n\n> > .../module_ipa_proxy.cpp.tmpl | 233 +++++++++\n> > .../module_ipa_proxy.h.tmpl | 117 +++++\n> > .../module_ipa_proxy_worker.cpp.tmpl | 184 +++++++\n> > .../module_serializer.h.tmpl | 44 ++\n> \n> And maybe module_ipa_serializer.h.tmpl for consistency ?\n\nThis becomes raspberrypi_serializer.h. raspberrypi_ipa_serializer.h ?\n\nOr just the template file name?\n\n> > .../libcamera_templates/proxy_functions.tmpl | 185 +++++++\n> > .../libcamera_templates/serializer.tmpl | 276 +++++++++++\n> > .../generators/mojom_libcamera_generator.py | 461 ++++++++++++++++++\n> > 8 files changed, 1606 insertions(+)\n> > create mode 100644 utils/ipc/generators/libcamera_templates/module_generated.h.tmpl\n> > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > create mode 100644 utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl\n> > create mode 100644 utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> > create mode 100644 utils/ipc/generators/libcamera_templates/serializer.tmpl\n> > create mode 100644 utils/ipc/generators/mojom_libcamera_generator.py\n> > \n> > diff --git a/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl\n> > new file mode 100644\n> > index 00000000..7dd48b0b\n> > --- /dev/null\n> > +++ b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl\n> > @@ -0,0 +1,106 @@\n> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) {{ year }}, Google Inc.\n> > + *\n> > + * {{ module_name }}_generated.h - Image Processing Algorithm interface for {{ module_name }}\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__\n> > +#define __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__\n> > +\n> > +#include \n> > +\n> \n> I think you can drop this blank line.\n> \n> > +#include \n> > +\n> > +{% if has_map %}#include {% endif %}\n> > +{% if has_array %}#include {% endif %}\n> > +\n> > +namespace libcamera {\n> > +{%- if has_namespace %}\n> > +{% for ns in namespace %}\n> > +namespace {{ns}} {\n> \n> There's a mix of {{ var }} and {{var}}. I don't mind much which one is\n> used, but I think we should be consistent.\n\nAh right, I forgot to fix that.\n\n> > +{% endfor %}\n> > +{%- endif %}\n> > +\n> > +enum {{ cmd_enum_name }} {\n> \n> It would be nice to make this an enum class, to avoid namespace clashes.\n> Alternatively, we could enforce the usage of namespaces for all IPAs.\n\nI made cmd_enum_name start with an underscore to avoid namespace\nclashes.\n\n'cmd_enum_name': '_%sCMD' % ModuleClassName(self.module),\n\nSo like enum _RPiCMD. I thought the underscore would be sufficient for\nanti name clashes.\n\n> enum class also brings the added benefit of catching improper usages,\n> but comes at a cost of requiring additional static_cast<> to convert\n> from and to uint32_t.\n\nI thought you could use enum as a type anyway? That's what I did with\nthe enums generated from the mojom file, and they all needed\nstatic_cast<>s. They're in the generated serializers:\n\nret.op_ = static_cast(IPADataSerializer::deserialize(m, m + 4));\n\n> > +\tCMD_EXIT = 0,\n> \n> I'd drop the CMD_ prefix, especially if we use enum class, and use\n> CamelCase for names instead of uppercase.\n\nThis cmd enum is for RPC, so I thought it was fine for just this enum to\nbe prefix with CMD_, just to make it clear in the generated code that\nthat's what this is for. UnixSocketTest does this too (that's where I\ngot it from).\n\n> > +{%- for method in interface_main.methods %}\n> > +\tCMD_{{ method.mojom_name|upper }} = {{loop.index}},\n> > +{%- endfor %}\n> > +{%- set count = interface_main.methods|length -%}\n> > +{%- for method in interface_cb.methods %}\n> > +\tCMD_{{ method.mojom_name|upper }} = {{loop.index + count}},\n> > +{%- endfor %}\n> \n> Should we split the enum in two, one for each interface ?\n\nEither way is fine, it doesn't matter much. That's why I put them\ntogether. The code generator loops over interface_{main,cb}.methods and\nnot the enum when it generates both sides of the RPC calls, so putting\nthem in different or the same enum makes literally no difference.\n\n> > +};\n> > +\n> > +{% for enum in enums %}\n> > +enum {{ enum.mojom_name }} {\n> > +{%- for field in enum.fields %}\n> > +\t{{ field.mojom_name }} = {{ field.numeric_value }},\n> > +{%- endfor %}\n> > +};\n> > +{% endfor %}\n> > +\n> > +{%- for struct in structs_nonempty %}\n> > +struct {{ struct.mojom_name }}\n> > +{\n> > +public:\n> > +\t{{ struct.mojom_name }}() {%- if struct|has_default_fields %} :{% endif %}\n> > +{%- for field in struct.fields|with_default_values -%}\n> > +{{\" \" if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ \", \" if not loop.last }}\n> > +{%- endfor %} {}\n> \n> This generates really long lines. How about\n> \n> \t{{ struct.mojom_name }}() {%- if struct|has_default_fields %}\n> \t\t:{% endif %}\n> {%- for field in struct.fields|with_default_values -%}\n> {{\" \" if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ \", \" if not loop.last }}\n> {%- endfor %}\n> \t{\n> \t}\n\nAh, right.\n\n> Would be great to wrap the initializers line too, but I think that's\n> overkill.\n> \n> > +\t~{{ struct.mojom_name }}() {}\n> > +\n> > +\t{{ struct.mojom_name }}(\n> > +{%- for field in struct.fields -%}\n> > +{{ field|name }} {{ field.mojom_name }}{{ \", \" if not loop.last }}\n> > +{%- endfor -%}\n> > +) :\n> > +{%- for field in struct.fields -%}\n> > +{{\" \" if loop.first}}{{ field.mojom_name}}_({{ field.mojom_name }}){{ \", \" if not loop.last }}\n> > +{%- endfor %} {}\n> \n> Same here.\n> \n> > +{% for field in struct.fields %}\n> > +\t{{ field|name }} {{ field.mojom_name }}_;\n> > +{%- endfor %}\n> > +};\n> > +{% endfor %}\n> > +\n> > +{#-\n> > +What to do about the #includes? Should we forward-declare, or\n> > +require {{module_name}}.h to include the required headers?\n> > +For now I'm going for the latter, coz this header essentially\n> > +extends {{module_name}}.h.\n> > +#}\n> \n> Looking at the existing {{module_name}}.h, there are a few enums and\n> constants that could be moved to {{module_name}}_generated.h, such as\n> enum BufferMark or MaxLsGridSize in raspberrypi.h. What do you think ?\n\nYeah enums I think are better to move to the mojom file. I did that for\nConfigParameters.\n\nmojo supports const (like if we had const uint32 MaxLsGridSize = 0x8000;)\nbut the code generator doesn't yet.\n\n> Then we have libcamera::RPi::Controls, which I'm wondering how to handle\n> best. It's not strictly related to this series, but wouldn't they be\n> better stored in the pipeline handler, and passed to the IPA ?\n\nI think it would be best if all ControlLists had a ControlInfoMap, then\nit wouldn't matter where libcamera::RPi::controls goes.\n\n> Finally there's the odd beast such as\n> \n> #define VIMC_IPA_FIFO_PATH \"/tmp/libcamera_ipa_vimc_fifo\"\n> \n> which I also wonder if it would make sense to pass it to the IPA at init\n> time. {{module_name}}.h could then possibly be dropped.\n\nOh you're thinking of dropping the main one entirely. Yeah that could\nwork too. I'm just worried if it'll be too restrictive. #defines could\nbe replaced with consts, so it's just the base ControlInfoMap that's an\nissue.\n\n> > +class {{ interface_name }} : public IPAInterface\n> > +{\n> > +public:\n> > +\tvirtual ~{{interface_name}}() {};\n> \n> No need for the trailing ;\n> \n> > +{% for method in interface_main.methods %}\n> > +\tvirtual {{method|method_return_value}} {{method.mojom_name}}(\n> > +{%- for param in method|method_parameters %}\n> > +\t\t{{param}}{{- \",\" if not loop.last}}\n> > +{%- endfor -%}\n> > +) = 0;\n> > +{% endfor %}\n> > +\n> > +{%- for method in interface_cb.methods %}\n> > +\tSignal<\n> > +{%- for param in method.parameters -%}\n> > +\t\tconst {{param|name}}{{\" &\" if not param|is_pod}}\n> \n> Should the const be dropped if param|is_pod ? Same below.\n\nAh right it's pass-by-value. Sure, let's drop it.\n\n> > +\t\t{{- \", \" if not loop.last }}\n> > +{%- endfor -%}\n> > +> {{method.mojom_name}};\n> > +{% endfor -%}\n> > +};\n> > +\n> > +{%- if has_namespace %}\n> > +{% for ns in namespace|reverse %}\n> > +} /* {{ns}} */\n> > +{% endfor %}\n> > +{%- endif %}\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__ */\n> > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > new file mode 100644\n> > index 00000000..f3eeeaaa\n> > --- /dev/null\n> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > @@ -0,0 +1,233 @@\n> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > +{%- import \"proxy_functions.tmpl\" as proxy_funcs -%}\n> > +\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) {{ year }}, Google Inc.\n> > + *\n> > + * ipa_proxy_{{ module_name }}.cpp - Image Processing Algorithm proxy for {{ module_name }}\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#include \n> > +\n> > +#include \n> \n> I think you can drop this, as it's included by {{module_name}}.h. Same\n> below.\n\nOh, okay.\n\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/control_serializer.h\"\n> > +#include \"libcamera/internal/ipa_ipc.h\"\n> > +#include \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> > +#include \"libcamera/internal/ipa_data_serializer.h\"\n> > +#include \"libcamera/internal/ipa_module.h\"\n> > +#include \"libcamera/internal/ipa_proxy.h\"\n> > +#include \"libcamera/internal/ipc_unixsocket.h\"\n> > +#include \"libcamera/internal/log.h\"\n> > +#include \"libcamera/internal/process.h\"\n> > +#include \"libcamera/internal/thread.h\"\n> > +\n> > +#include \n> \n> Should this be moved above with the other libcamera/ipa/ headers ?\n\nThis is the header that corresponds to this cpp file. I thought that\ngets special treatment and goes on its own at the end like here.\n\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(IPAProxy)\n> > +\n> > +{%- if has_namespace %}\n> > +{% for ns in namespace %}\n> > +namespace {{ns}} {\n> > +{% endfor %}\n> > +{%- endif %}\n> > +\n> > +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> > +\t: IPAProxy(ipam), running_(false),\n> > +\t isolate_(isolate)\n> > +{\n> > +\tLOG(IPAProxy, Debug)\n> > +\t\t<< \"initializing dummy proxy: loading IPA from \"\n> \n> It's not a dummy proxy anymore :-)\n\nOops :p\n\n> > +\t\t<< ipam->path();\n> > +\n> > +\tif (isolate_) {\n> > +\t\tconst std::string proxy_worker_path = resolvePath(\"ipa_proxy_{{module_name}}\");\n> > +\t\tif (proxy_worker_path.empty()) {\n> > +\t\t\tLOG(IPAProxy, Error)\n> > +\t\t\t\t<< \"Failed to get proxy worker path\";\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\n> > +\t\tipc_ = std::make_unique(ipam->path().c_str(), proxy_worker_path.c_str());\n> > +\t\tif (!ipc_->isValid()) {\n> > +\t\t\tLOG(IPAProxy, Error) << \"Failed to create IPAIPC\";\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\n> > +{% if interface_cb.methods|length > 0 %}\n> > +\t\tipc_->recvIPC.connect(this, &{{proxy_name}}::recvIPC);\n> > +{% endif %}\n> > +\n> > +\t\tvalid_ = true;\n> > +\t\treturn;\n> > +\t}\n> \n> There's very little code shared between the isolated and non-isolated\n> paths. It could make sense to split this class in two. On the other\n> hand, that would require the ability for the IPAManager to create the\n> right class type, which may complicate things a bit, but maybe not much.\n> For instance we could have the two classes called {{proxy_name}}Thread\n> and {{proxy_name}}IPC, and have a {{proxy_name}} class defined as\n> \n> class {{proxy_name}}\n> {\n> public:\n> \tusing IPC = {{proxy_name}}IPC;\n> \tusing Thread = {{proxy_name}}Thread;\n> };\n> \n> Then, in IPAManager::createIPA(), you would have\n> \n> \tstd::unique_ptr

proxy;\n> \tif (self_->isSignatureValid(m)))\n> \t\tproxy = std::make_unique(m);\n> \telse\n> \t\tproxy = std::make_unique(m);\n\nYeah maybe. The amount of generated code doesn't change though, it'll\njust be organized differently, and the only one that will notice the\ndifference is IPAManager as shown in this snippet.\n\n> > +\n> > +\tif (!ipam->load())\n> > +\t\treturn;\n> > +\n> > +\tIPAInterface *ipai = ipam->createInterface();\n> > +\tif (!ipai) {\n> > +\t\tLOG(IPAProxy, Error)\n> > +\t\t\t<< \"Failed to create IPA context for \" << ipam->path();\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tipa_ = std::unique_ptr<{{interface_name}}>(dynamic_cast<{{interface_name}} *>(ipai));\n> > +\tproxy_.setIPA(ipa_.get());\n> > +\n> > +{% for method in interface_cb.methods %}\n> > +\tipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);\n> > +{%- endfor %}\n> > +\n> > +\tvalid_ = true;\n> > +}\n> > +\n> > +{{proxy_name}}::~{{proxy_name}}()\n> > +{\n> > +\tif (isolate_)\n> > +\t\tipc_->sendAsync(CMD_EXIT, {}, {});\n> > +}\n> > +\n> > +{% if interface_cb.methods|length > 0 %}\n> > +void {{proxy_name}}::recvIPC(std::vector &data, std::vector &fds)\n> > +{\n> > +\tuint32_t cmd = (data[0] & 0xff) |\n> > +\t\t ((data[1] & 0xff) << 8) |\n> > +\t\t ((data[2] & 0xff) << 16) |\n> > +\t\t ((data[3] & 0xff) << 24);\n> \n> As data is a vector of uint8_t, I think the & 0xff is unnecessary.\n> \n> Should the data length be validated ?\n\ntbh I was thinking about it. I think cmd and vec are guaranteed, though.\nMaybe we should check it just in case.\n\n> > +\n> > +\t/* Need to skip another 4 bytes for the sequence number. */\n> > +\tstd::vector vec(data.begin() + 8, data.end());\n> \n> This creates a copy, which isn't very nice :-/ I think the IPC class\n> interface would benefit from switching from std::vector to Span.\n\nAh okay, I'll look into it.\n\n> > +\tswitch (cmd) {\n> > +{%- for method in interface_cb.methods %}\n> > +\tcase CMD_{{method.mojom_name|upper}}: {\n> > +\t\t{{method.mojom_name}}IPC(vec, fds);\n> > +\t\tbreak;\n> > +\t}\n> > +{%- endfor %}\n> \n> Should we log unknown commands ?\n\nOh maybe.\n\n> > +\t}\n> > +}\n> > +{%- endif %}\n> > +\n> > +{% for method in interface_main.methods %}\n> > +{{proxy_funcs.func_sig(proxy_name, method)}}\n> > +{\n> > +\tif (isolate_)\n> > +\t\t{{\"return \" if method|method_return_value != \"void\"}}{{method.mojom_name}}IPC(\n> > +{%- for param in method|method_param_names -%}\n> > +\t\t{{param}}{{- \", \" if not loop.last}}\n> > +{%- endfor -%}\n> > +);\n> > +\telse\n> > +\t\t{{\"return \" if method|method_return_value != \"void\"}}{{method.mojom_name}}Thread(\n> > +{%- for param in method|method_param_names -%}\n> > +\t\t{{param}}{{- \", \" if not loop.last}}\n> > +{%- endfor -%}\n> > +);\n> > +}\n> > +\n> > +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> > +{\n> > +{%- if method.mojom_name == \"init\" %}\n> > +\t{{proxy_funcs.init_thread_body()}}\n> > +{% elif method.mojom_name == \"start\" %}\n> \n> I think you're missing a - before elif, here and below.\n\nThis, I don't think so.\n\n> > +\t{{proxy_funcs.start_thread_body()}}\n> > +{% elif method.mojom_name == \"stop\" %}\n> > +\t{{proxy_funcs.stop_thread_body()}}\n> \n> Instead of using macros, would it make sense to move the code to the\n> base IPAProxy class ?\n\nMaybe...\n\n> > +{% elif not method|is_async %}\n> > +\tipa_->{{method.mojom_name}}(\n> > +\t{%- for param in method|method_param_names -%}\n> > +\t\t{{param}}{{- \", \" if not loop.last}}\n> > +\t{%- endfor -%}\n> > +);\n> > +{% elif method|is_async %}\n> > +\tproxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,\n> > +\t{%- for param in method|method_param_names -%}\n> > +\t\t{{param}}{{- \", \" if not loop.last}}\n> > +\t{%- endfor -%}\n> > +);\n> > +{%- endif %}\n> > +}\n> > +\n> > +{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n> > +{\n> > +{%- set has_input = true if method|method_param_inputs|length > 0 %}\n> > +{%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != \"void\" %}\n> > +{% if has_input %}\n> \n> Shouldn't it be {%- ?\n\nProbably... I don't remember if this specific one caused other problems.\n\n> > +\tstd::vector _ipcInputBuf;\n> > +{%- if method|method_input_has_fd %}\n> > +\tstd::vector _ipcInputFds;\n> > +{%- endif %}\n> > +{%- endif %}\n> > +{%- if has_output %}\n> > +\tstd::vector _ipcOutputBuf;\n> > +{%- if method|method_output_has_fd %}\n> > +\tstd::vector _ipcOutputFds;\n> > +{%- endif %}\n> > +{%- endif %}\n> > +\n> > +{{proxy_funcs.serialize_call(method|method_param_inputs, '_ipcInputBuf', '_ipcInputFds', base_controls)}}\n> > +\n> > +{%- set input_buf = \"_ipcInputBuf\" if has_input else \"{}\" %}\n> > +{%- set fds_buf = \"_ipcInputFds\" if method|method_input_has_fd else \"{}\" %}\n> > +{%- set cmd = \"CMD_\" + method.mojom_name|upper %}\n> > +{% if method|is_async %}\n> > +\tint ret = ipc_->sendAsync({{cmd}}, {{input_buf}}, {{fds_buf}});\n> > +{%- else %}\n> > +\tint ret = ipc_->sendSync({{cmd}}, {{input_buf}}, {{fds_buf}}\n> > +{{- \", &_ipcOutputBuf\" if has_output -}}\n> > +{{- \", &_ipcOutputFds\" if has_output and method|method_output_has_fd -}}\n> > +);\n> > +{%- endif %}\n> > +\tif (ret < 0) {\n> > +\t\tLOG(IPAProxy, Error) << \"Failed to call {{method.mojom_name}}\";\n> > +{%- if method|method_return_value != \"void\" %}\n> > +\t\treturn static_cast<{{method|method_return_value}}>(ret);\n> > +{%- else %}\n> > +\t\treturn;\n> > +{%- endif %}\n> > +\t}\n> > +\tLOG(IPAProxy, Debug) << \"Done calling {{method.mojom_name}}\";\n> \n> Is this message helpful for debugging ? I think we should work at some\n> point on a tracing mechanism instead of manually tracing calls.\n\nIt was while I wasn't sure the calls were going through. I think at this\npoint it's stable enough that we can remove it.\n\nYeah tracing would be nice :)\n\n> > +{% if method|method_return_value != \"void\" %}\n> > +\treturn IPADataSerializer<{{method.response_parameters|first|name}}>::deserialize(_ipcOutputBuf, 0);\n> > +{% elif method|method_param_outputs|length > 0 %}\n> > +{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf', '_ipcOutputFds')}}\n> > +{% endif -%}\n> > +}\n> > +\n> > +{% endfor %}\n> > +\n> > +\n> \n> One blank line should be enough.\n\nAh, right.\n\n> > +{% for method in interface_cb.methods %}\n> > +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> > +{\n> > +\t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> > +}\n> > +\n> > +void {{proxy_name}}::{{method.mojom_name}}IPC(\n> > +\tstd::vector &data,\n> > +\t[[maybe_unused]] std::vector &fds)\n> > +{ \n> > +{%- for param in method.parameters %}\n> > +\t{{param|name}} {{param.mojom_name}};\n> > +{%- endfor %}\n> > +{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false)}}\n> > +\t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> > +}\n> > +{% endfor %}\n> > +\n> > +{%- if has_namespace %}\n> > +{% for ns in namespace|reverse %}\n> > +} /* {{ns}} */\n> > +{% endfor %}\n> > +{%- endif %}\n> > +} /* namespace libcamera */\n> > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > new file mode 100644\n> > index 00000000..7d2bfb6c\n> > --- /dev/null\n> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > @@ -0,0 +1,117 @@\n> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > +{%- import \"proxy_functions.tmpl\" as proxy_funcs -%}\n> > +\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) {{ year }}, Google Inc.\n> > + *\n> > + * ipa_proxy_{{ module_name }}.h - Image Processing Algorithm proxy for {{ module_name }}\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__\n> > +#define __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/control_serializer.h\"\n> > +#include \"libcamera/internal/ipa_ipc.h\"\n> > +#include \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> > +#include \"libcamera/internal/ipa_proxy.h\"\n> > +#include \"libcamera/internal/ipc_unixsocket.h\"\n> > +#include \"libcamera/internal/thread.h\"\n> > +\n> > +namespace libcamera {\n> > +{%- if has_namespace %}\n> > +{% for ns in namespace %}\n> > +namespace {{ns}} {\n> > +{% endfor %}\n> > +{%- endif %}\n> > +\n> > +class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n> > +{\n> > +public:\n> > +\t{{proxy_name}}(IPAModule *ipam, bool isolate);\n> > +\t~{{proxy_name}}();\n> > +\n> > +{% for method in interface_main.methods %}\n> > +{{proxy_funcs.func_sig(proxy_name, method, \"\", false, true)|indent(8, true)}};\n> > +{% endfor %}\n> > +\n> > +{%- for method in interface_cb.methods %}\n> > +\tSignal<\n> > +{%- for param in method.parameters -%}\n> > +\t\tconst {{param|name}}{{\" &\" if not param|is_pod}}\n> > +\t\t{{- \", \" if not loop.last }}\n> > +{%- endfor -%}\n> > +> {{method.mojom_name}};\n> > +{% endfor %}\n> > +\n> > +private:\n> > +\tvoid recvIPC(std::vector &data, std::vector &fds);\n> > +\n> > +{% for method in interface_main.methods %}\n> > +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> > +{{proxy_funcs.func_sig(proxy_name, method, \"IPC\", false)|indent(8, true)}};\n> > +{% endfor %}\n> > +{% for method in interface_cb.methods %}\n> > +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> > +\tvoid {{method.mojom_name}}IPC(\n> > +\t\tstd::vector &data,\n> > +\t\tstd::vector &fds);\n> \n> Should these be const ?\n\nYeah.\n\n> > +{% endfor %}\n> > +\n> > +\t/* Helper class to invoke async functions in another thread. */\n> > +\tclass ThreadProxy : public Object\n> > +\t{\n> > +\tpublic:\n> > +\t\tvoid setIPA({{interface_name}} *ipa)\n> > +\t\t{\n> > +\t\t\tipa_ = ipa;\n> > +\t\t}\n> > +\n> > +\t\tint start()\n> > +\t\t{\n> > +\t\t\treturn ipa_->start();\n> > +\t\t}\n> > +\n> > +\t\tvoid stop()\n> > +\t\t{\n> > +\t\t\tipa_->stop();\n> > +\t\t}\n> > +{% for method in interface_main.methods %}\n> > +{%- if method|is_async %}\n> > +\t\t{{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n> > +\t\t{\n> > +\t\t\tipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> > +\t\t}\n> > +{%- endif %}\n> > +{%- endfor %}\n> \n> I think this is mixing two concepts, which shouldn't cause any issue for\n> now, but may come bite us back later.\n> \n> As you've noticed in the existing IPAProxyThread implementation, we call\n> some methods directly, and proxy other methods across threads. The idea\n> is that anything called before start() is a direct call, and anything\n> from start() until but not including stop() is indirect. The reason is\n> that before start() there's no thread running, so we can't make a\n> cross-thread call.\n\nAh, so sync vs async is different from non-thread call vs thread call...\n\n> One direct consequence of this architecture is that all calls before\n> start() are synchronous, and can thus return a value. Calls after\n> start() are asynchronous, but that's a separate design decision. We\n> could synchronize cross-thread calls if we wanted, we have chosen not to\n> in order to avoid delays that could damage the real time performance of\n> the pipeline handler.\n> \n> With IPC the situation is different, all calls are cross-process. Still,\n> we want to expose a similar behaviour to pipeline handlers, so calls\n> before start() are synchronous, and further calls are asynchronous.\n> \n> I don't think we can easily enforce this through the IDL, but I believe\n> it would make sense to document this requirements, so that authors will\n> not create an async method meant to be called before start(), or a\n> synchronous method after start().\n\nYeah, I think documentation should be sufficient.\n\n> > +\n> > +\tprivate:\n> > +\t\t{{interface_name}} *ipa_;\n> > +\t};\n> > +\n> > +\tbool running_;\n> > +\tThread thread_;\n> > +\tThreadProxy proxy_;\n> > +\tstd::unique_ptr<{{interface_name}}> ipa_;\n> > +\n> > +\tconst bool isolate_;\n> > +\n> > +\tstd::unique_ptr ipc_;\n> > +\n> > +\tControlSerializer controlSerializer_;\n> > +};\n> > +\n> > +{%- if has_namespace %}\n> > +{% for ns in namespace|reverse %}\n> > +} /* {{ns}} */\n> > +{% endfor %}\n> > +{%- endif %}\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__ */\n> > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > new file mode 100644\n> > index 00000000..1647a9ca\n> > --- /dev/null\n> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > @@ -0,0 +1,184 @@\n> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > +{%- import \"proxy_functions.tmpl\" as proxy_funcs -%}\n> > +\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) {{ year }}, Google Inc.\n> > + *\n> > + * ipa_proxy_{{ module_name }}_worker.cpp - Image Processing Algorithm proxy worker for {{ module_name }}\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/camera_sensor.h\"\n> > +#include \"libcamera/internal/control_serializer.h\"\n> > +#include \"libcamera/internal/ipa_data_serializer.h\"\n> > +#include \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> > +#include \"libcamera/internal/ipa_module.h\"\n> > +#include \"libcamera/internal/ipa_proxy.h\"\n> > +#include \"libcamera/internal/ipc_unixsocket.h\"\n> > +#include \"libcamera/internal/log.h\"\n> > +#include \"libcamera/internal/thread.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +LOG_DEFINE_CATEGORY({{proxy_name}}Worker)\n> > +\n> > +{%- if has_namespace %}\n> > +{% for ns in namespace -%}\n> > +using namespace {{ns}};\n> > +{% endfor %}\n> > +{%- endif %}\n> > +\n> > +struct CallData {\n> > +\tIPCUnixSocket::Payload *response;\n> > +\tbool done;\n> > +};\n> > +\n> > +{{interface_name}} *ipa_;\n> > +IPCUnixSocket socket_;\n> > +std::map callData_;\n> \n> callData_ doesn't seem to be used (and thus struct CallData shouldn't be\n> needed).\n\nOops, remnant of copying from UnixSocket.\n\n> > +\n> > +ControlSerializer controlSerializer_;\n> > +\n> > +bool exit_ = false;\n> \n> Should you group all this, as well as the functions below, in a\n> {{module_name}}ProxyWorker class ?\n\nI could. It's standalone so I didn't see much point in doing so though.\n\n> > +\n> > +void readyRead(IPCUnixSocket *socket)\n> > +{\n> > +\tIPCUnixSocket::Payload _message, _response;\n> > +\tint _ret = socket->receive(&_message);\n> > +\tif (_ret) {\n> > +\t\tLOG({{proxy_name}}Worker, Error)\n> > +\t\t\t<< \"Receive message failed\" << _ret;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tuint32_t _cmd, _seq;\n> > +\tstd::tie(_cmd, _seq) = IPAIPCUnixSocket::readHeader(_message);\n> > +\tIPAIPCUnixSocket::eraseHeader(_message);\n> \n> Using Span would help avoiding copies.\n\nAh...\n\n> > +\n> > +\tswitch (_cmd) {\n> > +\tcase CMD_EXIT: {\n> > +\t\texit_ = true;\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +{% for method in interface_main.methods %}\n> > +\tcase CMD_{{method.mojom_name|upper}}: {\n> > +\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_message.data', '_message.fds', false, true)|indent(8, true)}}\n> > +{% for param in method|method_param_outputs %}\n> > +\t\t{{param|name}} {{param.mojom_name}};\n> > +{% endfor %}\n> > +{%- if method|method_return_value != \"void\" %}\n> > +\t\t{{method|method_return_value}} _callRet = ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> > +{%- else %}\n> > +\t\tipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}}\n> > +{{- \", \" if method|method_param_outputs|params_comma_sep -}}\n> > +{%- for param in method|method_param_outputs -%}\n> > +&{{param.mojom_name}}{{\", \" if not loop.last}}\n> > +{%- endfor -%}\n> > +);\n> > +{%- endif %}\n> > +{% if not method|is_async %}\n> > +\t\tIPAIPCUnixSocket::writeHeader(_response, _cmd, _seq);\n> > +{%- if method|method_return_value != \"void\" %}\n> > +\t\tstd::vector _callRetBuf;\n> > +\t\tstd::tie(_callRetBuf, std::ignore) =\n> > +\t\t\tIPADataSerializer<{{method|method_return_value}}>::serialize(_callRet);\n> > +\t\t_response.data.insert(_response.data.end(), _callRetBuf.begin(), _callRetBuf.end());\n> > +{%- else %}\n> > +\t{{proxy_funcs.serialize_call(method|method_param_outputs, \"_response.data\", \"_response.fds\", base_controls)|indent(8, true)}}\n> > +{%- endif %}\n> > +\t\tint _ret = socket_.send(_response);\n> > +\t\tif (_ret < 0) {\n> > +\t\t\tLOG({{proxy_name}}Worker, Error)\n> > +\t\t\t\t<< \"Reply to {{method.mojom_name}}() failed\" << _ret;\n> > +\t\t}\n> > +\t\tLOG({{proxy_name}}Worker, Debug) << \"Done replying to {{method.mojom_name}}()\";\n> > +{%- endif %}\n> > +\t\tbreak;\n> > +\t}\n> > +{% endfor %}\n> \n> This isn't a huge deal, but the generated code my be a bit clearer if\n> each case called a function.\n\nIt does! ipa_->function(); :) It just comes with deserialization\nbeforehand and serialization afterward for synchronous calls. I think\nit's good enough...\n\n> > +\t}\n> > +}\n> > +\n> > +{% for method in interface_cb.methods %}\n> > +{{proxy_funcs.func_sig(proxy_name, method, \"\", false)}}\n> > +{\n> > +\tIPCUnixSocket::Payload _message;\n> > +\n> > +\tIPAIPCUnixSocket::writeHeader(_message, CMD_{{method.mojom_name|upper}}, 0);\n> > +\t{{proxy_funcs.serialize_call(method|method_param_inputs, \"_message.data\", \"_message.fds\", base_controls)}}\n> > +\n> > +\tsocket_.send(_message);\n> > +\n> > +\tLOG({{proxy_name}}Worker, Debug) << \"{{method.mojom_name}} done\";\n> > +}\n> > +{%- endfor %}\n> \n> You can drop the -\n\nYeah, this one needs to be dropped.\n\n> > +\n> > +int main(int argc, char **argv)\n> > +{\n> > +\t/* Uncomment this for debugging. */\n> > +#if 0\n> > +\tstd::string logPath = \"/tmp/libcamera.worker.\" +\n> > +\t\t\t std::to_string(getpid()) + \".log\";\n> > +\tlogSetFile(logPath.c_str());\n> > +#endif\n> > +\n> > +\tif (argc < 3) {\n> > +\t\tLOG({{proxy_name}}Worker, Debug)\n> \n> Maybe s/Debug/Error/ ?\n\nAh yes.\n\n> > +\t\t\t<< \"Tried to start worker with no args\";\n> > +\t\treturn EXIT_FAILURE;\n> > +\t}\n> > +\n> > +\tint fd = std::stoi(argv[2]);\n> > +\tLOG({{proxy_name}}Worker, Debug)\n> \n> And Info ?\n\nYeah that's better too.\n\n> > +\t\t<< \"Starting worker for IPA module \" << argv[1]\n> > +\t\t<< \" with IPC fd = \" << fd;\n> > +\n> > +\tstd::unique_ptr ipam = std::make_unique(argv[1]);\n> > +\tif (!ipam->isValid() || !ipam->load()) {\n> > +\t\tLOG({{proxy_name}}Worker, Error)\n> > +\t\t\t<< \"IPAModule \" << argv[1] << \" should be valid but isn't\";\n> \n> Maybe just \" isn't valid\" ?\n\nack\n\n> > +\t\treturn EXIT_FAILURE;\n> > +\t}\n> > +\n> > +\tif (socket_.bind(fd) < 0) {\n> > +\t\tLOG({{proxy_name}}Worker, Error) << \"IPC socket binding failed\";\n> > +\t\treturn EXIT_FAILURE;\n> > +\t}\n> > +\tsocket_.readyRead.connect(&readyRead);\n> > +\n> > +\tipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());\n> > +\tif (!ipa_) {\n> > +\t\tLOG({{proxy_name}}Worker, Error) << \"Failed to create IPA interface instance\";\n> > +\t\treturn EXIT_FAILURE;\n> > +\t}\n> > +{% for method in interface_cb.methods %}\n> > +\tipa_->{{method.mojom_name}}.connect(&{{method.mojom_name}});\n> > +{%- endfor %}\n> > +\n> > +\tLOG({{proxy_name}}Worker, Debug) << \"Proxy worker successfully started\";\n> > +\n> > +\t/* \\todo upgrade listening loop */\n> \n> Good point. We should make an EventLoop class at some point.\n\nI'm pretty sure I copied this from somewhere... I can't remember where.\n\n> > +\tEventDispatcher *dispatcher = Thread::current()->eventDispatcher();\n> > +\twhile (!exit_)\n> > +\t\tdispatcher->processEvents();\n> > +\n> > +\tdelete ipa_;\n> > +\tsocket_.close();\n> > +\n> > +\treturn 0;\n> > +}\n> > diff --git a/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl\n> > new file mode 100644\n> > index 00000000..37352c1f\n> > --- /dev/null\n> > +++ b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl\n> > @@ -0,0 +1,44 @@\n> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > +{%- import \"serializer.tmpl\" as serializer -%}\n> > +\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) {{ year }}, Google Inc.\n> > + *\n> > + * {{ module_name }}_serializer.h - Image Processing Algorithm data serializer for {{ module_name }}\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__\n> > +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/control_serializer.h\"\n> > +#include \"libcamera/internal/ipa_data_serializer.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DECLARE_CATEGORY(IPADataSerializer)\n> > +{% for struct in structs_nonempty %}\n> > +template<>\n> > +class IPADataSerializer<{{ struct|name_full(namespace_str) }}>\n> > +{\n> > +public:\n> > +{{- serializer.serializer(struct, base_controls, namespace_str) }}\n> > +{%- if struct|has_fd %}\n> > +{{ serializer.deserializer_fd(struct, namespace_str) }}\n> > +{%- else %}\n> > +{{ serializer.deserializer_no_fd(struct, namespace_str) }}\n> > +{%- endif %}\n> > +};\n> > +{% endfor %}\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__ */\n> \n> I'll review proxy_functions.tmpl and serializer.tmpl separately, this is\n> growing quite big.\n\nOkay. Thank you for the reviews so far.\n\n\nPaul\n\n> > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> > new file mode 100644\n> > index 00000000..e1f5a20c\n> > --- /dev/null\n> > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> > @@ -0,0 +1,185 @@\n> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > +{#\n> > + # \\brief Generate fuction prototype\n> > + #\n> > + # \\param class Class name\n> > + # \\param method mojom Method object\n> > + # \\param suffix Suffix to append to \\a method function name\n> > + # \\param need_class_name True to generate class name with function\n> > + # \\param override True to generate override tag after the function prototype\n> > + #}\n> > +{%- macro func_sig(class, method, suffix, need_class_name = true, override = false) -%}\n> > +{{method|method_return_value}} {{ class + \"::\" if need_class_name }}{{method.mojom_name}}{{suffix}}(\n> > +{%- for param in method|method_parameters %}\n> > +\t{{param}}{{- \",\" if not loop.last}}\n> > +{%- endfor -%}\n> > +){{\" override\" if override}}\n> > +{%- endmacro -%}\n> > +\n> > +{#\n> > + # \\brief Generate function body for IPA init() function for thread\n> > + #}\n> > +{%- macro init_thread_body() -%}\n> > +\tint ret = ipa_->init(settings);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tproxy_.moveToThread(&thread_);\n> > +\n> > +\treturn 0;\n> > +{%- endmacro -%}\n> > +\n> > +{#\n> > + # \\brief Generate function body for IPA start() function for thread\n> > + #}\n> > +{%- macro start_thread_body() -%}\n> > +\trunning_ = true;\n> > +\tthread_.start();\n> > +\n> > +\treturn proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);\n> > +{%- endmacro -%}\n> > +\n> > +{#\n> > + # \\brief Generate function body for IPA stop() function for thread\n> > + #}\n> > +{%- macro stop_thread_body() -%}\n> > +\tif (!running_)\n> > +\t\treturn;\n> > +\n> > +\trunning_ = false;\n> > +\n> > +\tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n> > +\n> > +\tthread_.exit();\n> > +\tthread_.wait();\n> > +{%- endmacro -%}\n> > +\n> > +\n> > +{#\n> > + # \\brief Serialize multiple objects into data buffer and fd vector\n> > + #\n> > + # Generate code to serialize multiple objects, as specified in \\a params\n> > + # (which are the parameters to some function), into \\a buf data buffer and\n> > + # \\a fds fd vector.\n> > + # This code is meant to be used by the proxy, for serializing prior to IPC calls.\n> > + #}\n> > +{%- macro serialize_call(params, buf, fds, base_controls) %}\n> > +{%- for param in params %}\n> > +\tstd::vector {{param.mojom_name}}Buf;\n> > +{%- if param|has_fd %}\n> > +\tstd::vector {{param.mojom_name}}Fds;\n> > +\tstd::tie({{param.mojom_name}}Buf, {{param.mojom_name}}Fds) =\n> > +{%- else %}\n> > +\tstd::tie({{param.mojom_name}}Buf, std::ignore) =\n> > +{%- endif %}\n> > +{%- if param|is_controls %}\n> > +\t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}, {{param.mojom_name}}.infoMap() ? *{{param.mojom_name}}.infoMap() : {{base_controls}}\n> > +{%- else %}\n> > +\t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}\n> > +{%- endif %}\n> > +{{- \", &controlSerializer_\" if param|needs_control_serializer -}}\n> > +);\n> > +{%- endfor %}\n> > +\n> > +{%- if params|length > 1 %}\n> > +{%- for param in params %}\n> > +\tappendUInt({{buf}}, {{param.mojom_name}}Buf.size());\n> > +{%- if param|has_fd %}\n> > +\tappendUInt({{buf}}, {{param.mojom_name}}Fds.size());\n> > +{%- endif %}\n> > +{%- endfor %}\n> > +{%- endif %}\n> > +\n> > +{%- for param in params %}\n> > +\t{{buf}}.insert({{buf}}.end(), {{param.mojom_name}}Buf.begin(), {{param.mojom_name}}Buf.end());\n> > +{%- endfor %}\n> > +\n> > +{%- for param in params %}\n> > +{%- if param|has_fd %}\n> > +\t{{fds}}.insert({{fds}}.end(), {{param.mojom_name}}Fds.begin(), {{param.mojom_name}}Fds.end());\n> > +{%- endif %}\n> > +{%- endfor %}\n> > +{%- endmacro -%}\n> > +\n> > +\n> > +{#\n> > + # \\brief Deserialize a single object from data buffer and fd vector\n> > + #\n> > + # \\param pointer True deserializes the object into a dereferenced pointer\n> > + #\n> > + # Generate code to deserialize a single object, as specified in \\a param,\n> > + # from \\a buf data buffer and \\a fds fd vector.\n> > + # This code is meant to be used by macro deserialize_call.\n> > + #}\n> > +{%- macro deserialize_param(param, pointer, loop, buf, fds) -%}\n> > +{{\"*\" if pointer}}{{param.mojom_name}} = IPADataSerializer<{{param|name}}>::deserialize(\n> > +\t{{buf}}.begin() + {{param.mojom_name}}Start,\n> > +{%- if loop.last %}\n> > +\t{{buf}}.end()\n> > +{%- else %}\n> > +\t{{buf}}.begin() + {{param.mojom_name}}Start + {{param.mojom_name}}BufSize\n> > +{%- endif -%}\n> > +{{- \",\" if param|has_fd }}\n> > +{%- if param|has_fd %}\n> > +\t{{fds}}.begin() + {{param.mojom_name}}FdStart,\n> > +{%- if loop.last %}\n> > +\t{{fds}}.end()\n> > +{%- else %}\n> > +\t{{fds}}.begin() + {{param.mojom_name}}FdStart + {{param.mojom_name}}FdsSize\n> > +{%- endif -%}\n> > +{%- endif -%}\n> > +{{- \",\" if param|needs_control_serializer }}\n> > +{%- if param|needs_control_serializer %}\n> > +\t&controlSerializer_\n> > +{%- endif -%}\n> > +);\n> > +{%- endmacro -%}\n> > +\n> > +\n> > +{#\n> > + # \\brief Deserialize multiple objects from data buffer and fd vector\n> > + #\n> > + # \\param pointer True deserializes objects into pointers, and adds a null check.\n> > + # \\param declare True declares the objects in addition to deserialization.\n> > + #\n> > + # Generate code to deserialize multiple objects, as specified in \\a params\n> > + # (which are the parameters to some function), from \\a buf data buffer and\n> > + # \\a fds fd vector.\n> > + # This code is meant to be used by the proxy, for serializing prior to IPC calls.\n> > + #}\n> > +{%- macro deserialize_call(params, buf, fds, pointer = true, declare = false) -%}\n> > +{% set ns = namespace(size_offset = 0) %}\n> > +{%- if params|length > 1 %}\n> > +{%- for param in params %}\n> > +\t[[maybe_unused]] size_t {{param.mojom_name}}BufSize = readUInt({{buf}}, {{ns.size_offset}});\n> > +\t{%- set ns.size_offset = ns.size_offset + 4 %}\n> > +{%- if param|has_fd %}\n> > +\t[[maybe_unused]] size_t {{param.mojom_name}}FdsSize = readUInt({{buf}}, {{ns.size_offset}});\n> > +\t{%- set ns.size_offset = ns.size_offset + 4 %}\n> > +{%- endif %}\n> > +{%- endfor %}\n> > +{%- endif %}\n> > +{% for param in params %}\n> > +{%- if loop.first %}\n> > +\tsize_t {{param.mojom_name}}Start = {{ns.size_offset}};\n> > +{%- else %}\n> > +\tsize_t {{param.mojom_name}}Start = {{loop.previtem.mojom_name}}Start + {{loop.previtem.mojom_name}}BufSize;\n> > +{%- endif %}\n> > +{%- endfor %}\n> > +{% for param in params|with_fds %}\n> > +{%- if loop.first %}\n> > +\tsize_t {{param.mojom_name}}FdStart = 0;\n> > +{%- elif not loop.last %}\n> > +\tsize_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;\n> > +{%- endif %}\n> > +{%- endfor %}\n> > +{% for param in params %}\n> > +\t{%- if pointer %}\n> > +\tif ({{param.mojom_name}}) {\n> > +{{deserialize_param(param, pointer, loop, buf, fds)|indent(16, True)}}\n> > +\t}\n> > +\t{%- else %}\n> > +\t{{param|name + \" \" if declare}}{{deserialize_param(param, pointer, loop, buf, fds)|indent(8)}}\n> > +\t{%- endif %}\n> > +{% endfor %}\n> > +{%- endmacro -%}\n> > diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> > new file mode 100644\n> > index 00000000..3c071c4e\n> > --- /dev/null\n> > +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> > @@ -0,0 +1,276 @@\n> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > +{# Turn this into a C macro? #}\n> > +{#\n> > + # \\brief Verify that there is enough bytes to deserialize\n> > + #\n> > + # Generate code that verifies that \\a size is not greater than \\a dataSize.\n> > + # Otherwise log an error with \\a name and \\a typename.\n> > + #}\n> > +{%- macro check_data_size(size, dataSize, name, typename) %}\n> > +\t\tif ({{size}} > {{dataSize}}) {\n> > +\t\t\tLOG(IPADataSerializer, Error)\n> > +\t\t\t\t<< \"Failed to deserialize {{name}}: not enough {{typename}}, expected \"\n> > +\t\t\t\t<< ({{size}}) << \", got \" << ({{dataSize}});\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +{%- endmacro %}\n> > +\n> > +\n> > +{#\n> > + # \\brief Serialize some field into return vector\n> > + #\n> > + # Generate code to serialize \\a field into ret_data, including size of the\n> > + # field and fds (where appropriate). \\a base_controls indicates the\n> > + # default ControlInfoMap in the event that the ControlList does not have one.\n> > + # This code is meant to be used by the IPADataSerializer specialization.\n> > + #}\n> > +{%- macro serializer_field(field, base_controls, namespace, loop) %}\n> > +{%- if field|is_pod or field|is_enum %}\n> > +\t\tstd::vector {{field.mojom_name}};\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_enum %}\n> > +\t\t\tIPADataSerializer::serialize(data.{{field.mojom_name}}_);\n> > +\t{%- endif %}\n> > +\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> > +{%- elif field|is_fd %}\n> > +\t\tstd::vector {{field.mojom_name}};\n> > +\t\tstd::vector {{field.mojom_name}}Fds;\n> > +\t\tstd::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =\n> > +\t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);\n> > +\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> > +\t\tret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());\n> > +{%- elif field|is_controls %}\n> > +\t\tif (data.{{field.mojom_name}}_.size() > 0) {\n> > +\t\t\tstd::vector {{field.mojom_name}};\n> > +\t\t\tstd::tie({{field.mojom_name}}, std::ignore) =\n> > +\t\t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_,\n> > +\t\t\t\t\tdata.{{field.mojom_name}}_.infoMap() ? *data.{{field.mojom_name}}_.infoMap() : {{base_controls}},\n> > +\t\t\t\t\tcs);\n> > +\t\t\tappendUInt(ret_data, {{field.mojom_name}}.size());\n> > +\t\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> > +\t\t} else {\n> > +\t\t\tappendUInt(ret_data, 0);\n> > +\t\t}\n> > +{%- elif field|is_plain_struct or field|is_array or field|is_map %}\n> > +\t\tstd::vector {{field.mojom_name}};\n> > +\t{%- if field|has_fd %}\n> > +\t\tstd::vector {{field.mojom_name}}Fds;\n> > +\t\tstd::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =\n> > +\t{%- else %}\n> > +\t\tstd::tie({{field.mojom_name}}, std::ignore) =\n> > +\t{%- endif %}\n> > +\t{%- if field|is_array or field|is_map %}\n> > +\t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_, cs);\n> > +\t{%- else %}\n> > +\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::serialize(data.{{field.mojom_name}}_, cs);\n> > +\t{%- endif %}\n> > +\t\tappendUInt(ret_data, {{field.mojom_name}}.size());\n> > +\t{%- if field|has_fd %}\n> > +\t\tappendUInt(ret_data, {{field.mojom_name}}Fds.size());\n> > +\t{%- endif %}\n> > +\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> > +\t{%- if field|has_fd %}\n> > +\t\tret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());\n> > +\t{%- endif %}\n> > +{%- else %}\n> > +\t\t/* Unknown serialization for {{field.mojom_name}}. */\n> > +{%- endif %}\n> > +{%- endmacro %}\n> > +\n> > +\n> > +{#\n> > + # \\brief Deserialize some field into return struct\n> > + #\n> > + # Generate code to deserialize \\a field into object ret.\n> > + # This code is meant to be used by the IPADataSerializer specialization.\n> > + #}\n> > +{%- macro deserializer_field(field, namespace, loop) %}\n> > +{% if field|is_pod or field|is_enum %}\n> > +\t{%- set field_size = (field|bit_width|int / 8)|int %}\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}}_ = static_cast<{{field|name}}>(IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}));\n> > +\t\t{%- else %}\n> > +\t\tret.{{field.mojom_name}}_ = static_cast<{{field|name}}>(IPADataSerializer::deserialize(m, m + {{field_size}}));\n> > +\t\t{%- endif %}\n> > +\t{%- if not loop.last %}\n> > +\t\tm += {{field_size}};\n> > +\t\tdataSize -= {{field_size}};\n> > +\t{%- endif %}\n> > +{% elif field|is_fd %}\n> > +\t{%- set field_size = 1 %}\n> > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}\n> > +\t\tret.{{field.mojom_name}}_ = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs);\n> > +\t{%- if not loop.last %}\n> > +\t\tm += {{field_size}};\n> > +\t\tdataSize -= {{field_size}};\n> > +\t\tn += ret.{{field.mojom_name}}_.isValid() ? 1 : 0;\n> > +\t\tfdsSize -= ret.{{field.mojom_name}}_.isValid() ? 1 : 0;\n> > +\t{%- endif %}\n> > +{% elif field|is_controls %}\n> > +\t{%- set field_size = 4 %}\n> > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}\n> > +\t\tsize_t {{field.mojom_name}}Size = readUInt(m);\n> > +\t{%- set field_size = '4 + ' + field.mojom_name + 'Size' -%}\n> > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}\n> > +\t\tif ({{field.mojom_name}}Size > 0)\n> > +\t\t\tret.{{field.mojom_name}}_ =\n> > +\t\t\t\tIPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);\n> > +\t{%- if not loop.last %}\n> > +\t\tm += {{field_size}};\n> > +\t\tdataSize -= {{field_size}};\n> > +\t{%- endif %}\n> > +{% elif field|is_plain_struct or field|is_array or field|is_map%}\n> > +\t{%- set field_size = 4 %}\n> > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}\n> > +\t\tsize_t {{field.mojom_name}}Size = readUInt(m);\n> > +\t{%- if field|has_fd %}\n> > +\t{%- set field_size = 8 %}\n> > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'FdsSize', 'data') }}\n> > +\t\tsize_t {{field.mojom_name}}FdsSize = readUInt(m + 4);\n> > +\t\t{{- check_data_size(field.mojom_name + 'FdsSize', 'fdsSize', field.mojom_name, 'fds') }}\n> > +\t{%- endif %}\n> > +\t{%- set field_size = field|has_fd|choose('8 + ', '4 + ') + field.mojom_name + 'Size' -%}\n> > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}\n> > +\t\tret.{{field.mojom_name}}_ =\n> > +\t{%- if field|has_fd and (field|is_array or field|is_map) %}\n> > +\t\t\tIPADataSerializer<{{field|name}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);\n> > +\t{%- elif field|has_fd and (not (field|is_array or field|is_map)) %}\n> > +\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);\n> > +\t{%- elif (not field|has_fd) and (field|is_array or field|is_map) %}\n> > +\t\t\tIPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);\n> > +\t{%- else %}\n> > +\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);\n> > +\t{%- endif %}\n> > +\t{%- if not loop.last %}\n> > +\t\tm += {{field_size}};\n> > +\t\tdataSize -= {{field_size}};\n> > +\t{%- if field|has_fd %}\n> > +\t\tn += {{field.mojom_name}}FdsSize;\n> > +\t\tfdsSize -= {{field.mojom_name}}FdsSize;\n> > +\t{%- endif %}\n> > +\t{%- endif %}\n> > +{% else %}\n> > +\t\t/* Unknown deserialization for {{field.mojom_name}}. */\n> > +{%- endif %}\n> > +{%- endmacro %}\n> > +\n> > +\n> > +{#\n> > + # \\brief Serialize a struct\n> > + #\n> > + # Generate code for IPADataSerializer specialization, for serializing\n> > + # \\a struct. \\a base_controls indicates the default ControlInfoMap\n> > + # in the event that the ControlList does not have one.\n> > + #}\n> > +{%- macro serializer(struct, base_controls, namespace) %}\n> > +\tstatic std::tuple, std::vector>\n> > +\tserialize(const {{ struct|name_full(namespace) }} &data,\n> > +{%- if struct|needs_control_serializer %}\n> > +\t\t ControlSerializer *cs)\n> > +{%- else %}\n> > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +{%- endif %}\n> > +\t{\n> > +\t\tstd::vector ret_data;\n> > +{%- if struct|has_fd %}\n> > +\t\tstd::vector ret_fds;\n> > +{%- endif %}\n> > +{%- for field in struct.fields %}\n> > +{{serializer_field(field, base_controls, namespace, loop)}}\n> > +{%- endfor %}\n> > +{% if struct|has_fd %}\n> > +\t\treturn {ret_data, ret_fds};\n> > +{%- else %}\n> > +\t\treturn {ret_data, {}};\n> > +{%- endif %}\n> > +\t}\n> > +{%- endmacro %}\n> > +\n> > +\n> > +{#\n> > + # \\brief Deserialize a struct that has fds\n> > + #\n> > + # Generate code for IPADataSerializer specialization, for deserializing\n> > + # \\a struct, in the case that \\a struct has file descriptors.\n> > + #}\n> > +{%- macro deserializer_fd(struct, namespace) %}\n> > +\tstatic {{ struct|name_full(namespace) }}\n> > +\tdeserialize(std::vector &data,\n> > +\t\t std::vector &fds,\n> > +{%- if struct|needs_control_serializer %}\n> > +\t\t ControlSerializer *cs)\n> > +{%- else %}\n> > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +{%- endif %}\n> > +\t{\n> > +\t\t{{struct|name_full(namespace)}} ret;\n> > +\t\tstd::vector::iterator m = data.begin();\n> > +\t\tstd::vector::iterator n = fds.begin();\n> > +\n> > +\t\tsize_t dataSize = data.size();\n> > +\t\tsize_t fdsSize = fds.size();\n> > +{%- for field in struct.fields -%}\n> > +{{deserializer_field(field, namespace, loop)}}\n> > +{%- endfor %}\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tstatic {{ struct|name_full(namespace) }}\n> > +\tdeserialize(std::vector::iterator data_it1,\n> > +\t\t std::vector::iterator data_it2,\n> > +\t\t std::vector::iterator fds_it1,\n> > +\t\t std::vector::iterator fds_it2,\n> > +{%- if struct|needs_control_serializer %}\n> > +\t\t ControlSerializer *cs)\n> > +{%- else %}\n> > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +{%- endif %}\n> > +\t{\n> > +\t\tstd::vector data(data_it1, data_it2);\n> > +\t\tstd::vector fds(fds_it1, fds_it2);\n> > +\t\treturn IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, fds, cs);\n> > +\t}\n> > +{%- endmacro %}\n> > +\n> > +\n> > +{#\n> > + # \\brief Deserialize a struct that has no fds\n> > + #\n> > + # Generate code for IPADataSerializer specialization, for deserializing\n> > + # \\a struct, in the case that \\a struct does not have file descriptors.\n> > + #}\n> > +{%- macro deserializer_no_fd(struct, namespace) %}\n> > +\tstatic {{ struct|name_full(namespace) }}\n> > +\tdeserialize(std::vector &data,\n> > +{%- if struct|needs_control_serializer %}\n> > +\t\t ControlSerializer *cs)\n> > +{%- else %}\n> > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +{%- endif %}\n> > +\t{\n> > +\t\t{{struct|name_full(namespace)}} ret;\n> > +\t\tstd::vector::iterator m = data.begin();\n> > +\n> > +\t\tsize_t dataSize = data.size();\n> > +{%- for field in struct.fields -%}\n> > +{{deserializer_field(field, namespace, loop)}}\n> > +{%- endfor %}\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tstatic {{ struct|name_full(namespace) }}\n> > +\tdeserialize(std::vector::iterator data_it1,\n> > +\t\t std::vector::iterator data_it2,\n> > +{%- if struct|needs_control_serializer %}\n> > +\t\t ControlSerializer *cs)\n> > +{%- else %}\n> > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > +{%- endif %}\n> > +\t{\n> > +\t\tstd::vector data(data_it1, data_it2);\n> > +\t\treturn IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, cs);\n> > +\t}\n> > +{%- endmacro %}\n> > diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py\n> > new file mode 100644\n> > index 00000000..a9269c82\n> > --- /dev/null\n> > +++ b/utils/ipc/generators/mojom_libcamera_generator.py\n> > @@ -0,0 +1,461 @@\n> > +'''Generates libcamera files from a mojom.Module.'''\n> > +\n> > +import argparse\n> > +import ast\n> > +import datetime\n> > +import contextlib\n> > +import os\n> > +import re\n> > +import shutil\n> > +import sys\n> > +import tempfile\n> > +\n> > +from jinja2 import contextfilter\n> > +\n> > +import mojom.fileutil as fileutil\n> > +import mojom.generate.generator as generator\n> > +import mojom.generate.module as mojom\n> > +from mojom.generate.template_expander import UseJinja\n> > +\n> > +\n> > +GENERATOR_PREFIX = 'libcamera'\n> > +\n> > +_kind_to_cpp_type = {\n> > + mojom.BOOL: 'bool',\n> > + mojom.INT8: 'int8_t',\n> > + mojom.UINT8: 'uint8_t',\n> > + mojom.INT16: 'int16_t',\n> > + mojom.UINT16: 'uint16_t',\n> > + mojom.INT32: 'int32_t',\n> > + mojom.UINT32: 'uint32_t',\n> > + mojom.FLOAT: 'float',\n> > + mojom.INT64: 'int64_t',\n> > + mojom.UINT64: 'uint64_t',\n> > + mojom.DOUBLE: 'double',\n> > +}\n> > +\n> > +_bit_widths = {\n> > + mojom.BOOL: '8',\n> > + mojom.INT8: '8',\n> > + mojom.UINT8: '8',\n> > + mojom.INT16: '16',\n> > + mojom.UINT16: '16',\n> > + mojom.INT32: '32',\n> > + mojom.UINT32: '32',\n> > + mojom.FLOAT: '32',\n> > + mojom.INT64: '64',\n> > + mojom.UINT64: '64',\n> > + mojom.DOUBLE: '64',\n> > +}\n> > +\n> > +def ModuleName(path):\n> > + return path.split('/')[-1].split('.')[0]\n> > +\n> > +def ModuleClassName(module):\n> > + s = re.sub(r'^IPA', '', module.interfaces[0].mojom_name)\n> > + return re.sub(r'Interface$', '', s)\n> > +\n> > +def UpperCamelCase(name):\n> > + return ''.join([x.capitalize() for x in generator.SplitCamelCase(name)])\n> > +\n> > +def CamelCase(name):\n> > + uccc = UpperCamelCase(name)\n> > + return uccc[0].lower() + uccc[1:]\n> > +\n> > +def ConstantStyle(name):\n> > + return generator.ToUpperSnakeCase(name)\n> > +\n> > +def Choose(cond, t, f):\n> > + return t if cond else f\n> > +\n> > +def CommaSep(l):\n> > + return ', '.join([m for m in l])\n> > +\n> > +def ParamsCommaSep(l):\n> > + return ', '.join([m.mojom_name for m in l])\n> > +\n> > +def GetDefaultValue(element):\n> > + if element.default is not None:\n> > + return element.default\n> > + if type(element.kind) == mojom.Kind:\n> > + return '0'\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> > + element.kind.mojom_name == 'FileDescriptor'):\n> > + return '-1'\n> > + return ''\n> > +\n> > +def WithDefaultValues(element):\n> > + return [x for x in element if HasDefaultValue(x)]\n> > +\n> > +def WithFds(element):\n> > + return [x for x in element if HasFd(x)]\n> > +\n> > +def HasDefaultValue(element):\n> > + return GetDefaultValue(element) != ''\n> > +\n> > +def HasDefaultFields(element):\n> > + return True in [HasDefaultValue(x) for x in element.fields]\n> > +\n> > +def GetAllTypes(element):\n> > + if mojom.IsArrayKind(element):\n> > + return GetAllTypes(element.kind)\n> > + if mojom.IsMapKind(element):\n> > + return GetAllTypes(element.key_kind) + GetAllTypes(element.value_kind)\n> > + if isinstance(element, mojom.Parameter):\n> > + return GetAllTypes(element.kind)\n> > + if mojom.IsEnumKind(element):\n> > + return [element.mojom_name]\n> > + if not mojom.IsStructKind(element):\n> > + return [element.spec]\n> > + if len(element.fields) == 0:\n> > + return [element.mojom_name]\n> > + ret = [GetAllTypes(x.kind) for x in element.fields]\n> > + ret = [x for sublist in ret for x in sublist]\n> > + return list(set(ret))\n> > +\n> > +def GetAllAttrs(element):\n> > + if mojom.IsArrayKind(element):\n> > + return GetAllAttrs(element.kind)\n> > + if mojom.IsMapKind(element):\n> > + return {**GetAllAttrs(element.key_kind), **GetAllAttrs(element.value_kind)}\n> > + if isinstance(element, mojom.Parameter):\n> > + return GetAllAttrs(element.kind)\n> > + if mojom.IsEnumKind(element):\n> > + return element.attributes if element.attributes is not None else {}\n> > + if mojom.IsStructKind(element) and len(element.fields) == 0:\n> > + return element.attributes if element.attributes is not None else {}\n> > + if not mojom.IsStructKind(element):\n> > + return {}\n> > + attrs = [(x.attributes) for x in element.fields]\n> > + ret = {}\n> > + for d in attrs:\n> > + if d is not None:\n> > + ret = {**ret, **d}\n> > + return ret\n> > +\n> > +def NeedsControlSerializer(element):\n> > + types = GetAllTypes(element)\n> > + return \"ControlList\" in types or \"ControlInfoMap\" in types\n> > +\n> > +def HasFd(element):\n> > + attrs = GetAllAttrs(element)\n> > + if isinstance(element, mojom.Kind):\n> > + types = GetAllTypes(element)\n> > + else:\n> > + types = GetAllTypes(element.kind)\n> > + return \"FileDescriptor\" in types or (attrs is not None and \"hasFd\" in attrs)\n> > +\n> > +def MethodInputHasFd(method):\n> > + if len([x for x in method.parameters if HasFd(x)]) > 0:\n> > + return True\n> > + return False\n> > +\n> > +def MethodOutputHasFd(method):\n> > + if (MethodReturnValue(method) != 'void' or\n> > + method.response_parameters is None):\n> > + return False\n> > + if len([x for x in method.parameters if HasFd(x)]) > 0:\n> > + return True\n> > + return False\n> > +\n> > +def MethodParamInputs(method):\n> > + return method.parameters\n> > +\n> > +def MethodParamOutputs(method):\n> > + if (MethodReturnValue(method) != 'void' or\n> > + method.response_parameters is None):\n> > + return []\n> > + return method.response_parameters\n> > +\n> > +def MethodParamNames(method):\n> > + params = []\n> > + for param in method.parameters:\n> > + params.append(param.mojom_name)\n> > + if MethodReturnValue(method) == 'void':\n> > + if method.response_parameters is None:\n> > + return params\n> > + for param in method.response_parameters:\n> > + params.append(param.mojom_name)\n> > + return params\n> > +\n> > +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> > + param.mojom_name))\n> > + if MethodReturnValue(method) == 'void':\n> > + if method.response_parameters is None:\n> > + return params\n> > + for param in method.response_parameters:\n> > + params.append(f'{GetNameForElement(param)} *{param.mojom_name}')\n> > + return params\n> > +\n> > +def MethodReturnValue(method):\n> > + if method.response_parameters is None:\n> > + return 'void'\n> > + if len(method.response_parameters) == 1 and IsPod(method.response_parameters[0]):\n> > + return GetNameForElement(method.response_parameters[0])\n> > + return 'void'\n> > +\n> > +def IsAsync(method):\n> > + # callbacks are always async\n> > + if re.match(\"^IPA.*CallbackInterface$\", method.interface.mojom_name):\n> > + return True\n> > + elif re.match(\"^IPA.*Interface$\", method.interface.mojom_name):\n> > + if method.attributes is None:\n> > + return False\n> > + elif 'async' in method.attributes and method.attributes['async']:\n> > + return True\n> > + return False\n> > +\n> > +def IsArray(element):\n> > + return mojom.IsArrayKind(element.kind)\n> > +\n> > +def IsControls(element):\n> > + return mojom.IsStructKind(element.kind) and (element.kind.mojom_name == \"ControlList\" or\n> > + element.kind.mojom_name == \"ControlInfoMap\")\n> > +\n> > +def IsEnum(element):\n> > + return mojom.IsEnumKind(element.kind)\n> > +\n> > +def IsFd(element):\n> > + return mojom.IsStructKind(element.kind) and element.kind.mojom_name == \"FileDescriptor\"\n> > +\n> > +def IsMap(element):\n> > + return mojom.IsMapKind(element.kind)\n> > +\n> > +def IsPlainStruct(element):\n> > + return mojom.IsStructKind(element.kind) and not IsControls(element) and not IsFd(element)\n> > +\n> > +def IsPod(element):\n> > + return element.kind in _kind_to_cpp_type\n> > +\n> > +def BitWidth(element):\n> > + if element.kind in _bit_widths:\n> > + return _bit_widths[element.kind]\n> > + if mojom.IsEnumKind(element.kind):\n> > + return '32'\n> > + return ''\n> > +\n> > +def GetNameForElement(element):\n> > + if (mojom.IsEnumKind(element) or\n> > + mojom.IsInterfaceKind(element) or\n> > + mojom.IsStructKind(element)):\n> > + return element.mojom_name\n> > + if (mojom.IsArrayKind(element)):\n> > + elem_name = GetNameForElement(element.kind)\n> > + return f'std::vector<{elem_name}>'\n> > + if (mojom.IsMapKind(element)):\n> > + key_name = GetNameForElement(element.key_kind)\n> > + value_name = GetNameForElement(element.value_kind)\n> > + return f'std::map<{key_name}, {value_name}>'\n> > + if isinstance(element, (mojom.Field, mojom.Method, mojom.Parameter)):\n> > + if (mojom.IsArrayKind(element.kind) or mojom.IsMapKind(element.kind)):\n> > + return GetNameForElement(element.kind)\n> > + if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):\n> > + return _kind_to_cpp_type[element.kind]\n> > + return element.kind.mojom_name\n> > + if isinstance(element, mojom.EnumValue):\n> > + return (GetNameForElement(element.enum) + '.' +\n> > + ConstantStyle(element.name))\n> > + if isinstance(element, (mojom.NamedValue,\n> > + mojom.Constant,\n> > + mojom.EnumField)):\n> > + return ConstantStyle(element.name)\n> > + if (hasattr(element, '__hash__') and element in _kind_to_cpp_type):\n> > + return _kind_to_cpp_type[element]\n> > + if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):\n> > + return _kind_to_cpp_type[element.kind]\n> > + if (hasattr(element, 'spec')):\n> > + return element.spec.split(':')[-1]\n> > + if (mojom.IsInterfaceRequestKind(element) or\n> > + mojom.IsAssociatedKind(element) or\n> > + mojom.IsPendingRemoteKind(element) or\n> > + mojom.IsPendingReceiverKind(element) or\n> > + mojom.IsUnionKind(element)):\n> > + raise Exception('Unsupported element: %s' % element)\n> > + raise Exception('Unexpected element: %s' % element)\n> > +\n> > +def GetFullNameForElement(element, namespace_str):\n> > + name = GetNameForElement(element)\n> > + if namespace_str == '':\n> > + return name\n> > + return f'{namespace_str}::{name}'\n> > +\n> > +def ValidateZeroLength(l, s, cap=True):\n> > + if len(l) > 0:\n> > + raise Exception(f'{s.capitalize() if cap else s} should be empty')\n> > +\n> > +def ValidateSingleLength(l, s, cap=True):\n> > + if len(l) > 1:\n> > + raise Exception(f'Only one {s} allowed')\n> > + if len(l) < 1:\n> > + raise Exception(f'{s.capitalize() if cap else s} is required')\n> > +\n> > +def ValidateInterfaces(interfaces):\n> > + # Validate presence of main interface\n> > + intf = [x for x in interfaces\n> > + if re.match(\"^IPA.*Interface\", x.mojom_name) and\n> > + not re.match(\"^IPA.*CallbackInterface\", x.mojom_name)]\n> > + ValidateSingleLength(intf, 'main interface')\n> > +\n> > + # Validate presence of callback interface\n> > + cb = [x for x in interfaces if re.match(\"^IPA.*CallbackInterface\", x.mojom_name)]\n> > + ValidateSingleLength(cb, 'callback interface')\n> > +\n> > + # Validate required main interface functions\n> > + intf = intf[0]\n> > + f_init = [x for x in intf.methods if x.mojom_name == 'init']\n> > + f_start = [x for x in intf.methods if x.mojom_name == 'start']\n> > + f_stop = [x for x in intf.methods if x.mojom_name == 'stop']\n> > +\n> > + ValidateSingleLength(f_init, 'init()', False)\n> > + ValidateSingleLength(f_start, 'start()', False)\n> > + ValidateSingleLength(f_stop, 'stop()', False)\n> > +\n> > + f_init = f_init[0]\n> > + f_start = f_start[0]\n> > + f_stop = f_stop[0]\n> > +\n> > + # Validate parameters to init()\n> > + ValidateSingleLength(f_init.parameters, 'input parameter to init()')\n> > + ValidateSingleLength(f_init.response_parameters, 'output parameter from init()')\n> > + if f_init.parameters[0].kind.mojom_name != 'IPASettings':\n> > + raise Exception('init() must have single IPASettings input parameter')\n> > + if f_init.response_parameters[0].kind.spec != 'i32':\n> > + raise Exception('init() must have single int32 output parameter')\n> > +\n> > + # Validate parameters to start()\n> > + ValidateZeroLength(f_start.parameters, 'input parameter to start()')\n> > + ValidateSingleLength(f_start.response_parameters, 'output parameter from start()')\n> > + if f_start.response_parameters[0].kind.spec != 'i32':\n> > + raise Exception('start() must have single int32 output parameter')\n> > +\n> > + # Validate parameters to stop()\n> > + ValidateZeroLength(f_stop.parameters, 'input parameter to stop()')\n> > + ValidateZeroLength(f_stop.parameters, 'output parameter from stop()')\n> > +\n> > +class Generator(generator.Generator):\n> > +\n> > + @staticmethod\n> > + def GetTemplatePrefix():\n> > + return 'libcamera_templates'\n> > +\n> > + def GetFilters(self):\n> > + libcamera_filters = {\n> > + 'all_types': GetAllTypes,\n> > + 'bit_width': BitWidth,\n> > + 'choose': Choose,\n> > + 'comma_sep': CommaSep,\n> > + 'default_value': GetDefaultValue,\n> > + 'has_default_fields': HasDefaultFields,\n> > + 'has_fd': HasFd,\n> > + 'is_async': IsAsync,\n> > + 'is_array': IsArray,\n> > + 'is_controls': IsControls,\n> > + 'is_enum': IsEnum,\n> > + 'is_fd': IsFd,\n> > + 'is_map': IsMap,\n> > + 'is_plain_struct': IsPlainStruct,\n> > + 'is_pod': IsPod,\n> > + 'method_input_has_fd': MethodInputHasFd,\n> > + 'method_output_has_fd': MethodOutputHasFd,\n> > + 'method_param_names': MethodParamNames,\n> > + 'method_param_inputs': MethodParamInputs,\n> > + 'method_param_outputs': MethodParamOutputs,\n> > + 'method_parameters': MethodParameters,\n> > + 'method_return_value': MethodReturnValue,\n> > + 'name': GetNameForElement,\n> > + 'name_full': GetFullNameForElement,\n> > + 'needs_control_serializer': NeedsControlSerializer,\n> > + 'params_comma_sep': ParamsCommaSep,\n> > + 'with_default_values': WithDefaultValues,\n> > + 'with_fds': WithFds,\n> > + }\n> > + return libcamera_filters\n> > +\n> > + def _GetJinjaExports(self):\n> > + return {\n> > + 'base_controls': '%s::Controls' % ModuleClassName(self.module),\n> > + 'cmd_enum_name': '_%sCMD' % ModuleClassName(self.module),\n> \n> s/CMD/Command/ ?\n> \n> > + 'enums': self.module.enums,\n> > + 'has_array': len([x for x in self.module.kinds.keys() if x[0] == 'a']) > 0,\n> > + 'has_map': len([x for x in self.module.kinds.keys() if x[0] == 'm']) > 0,\n> > + 'has_namespace': self.module.mojom_namespace != '',\n> > + 'imports': self.module.imports,\n> > + 'interface_cb': self.module.interfaces[1],\n> > + 'interface_main': self.module.interfaces[0],\n> > + 'interface_name': 'IPA%sInterface' % ModuleClassName(self.module),\n> > + 'ipc_name': 'IPAIPCUnixSocket',\n> > + 'kinds': self.module.kinds,\n> > + 'module': self.module,\n> > + 'module_name': ModuleName(self.module.path),\n> > + 'module_class_name': ModuleClassName(self.module),\n> > + 'namespace': self.module.mojom_namespace.split('.'),\n> > + 'namespace_str': self.module.mojom_namespace.replace('.', '::') if\n> > + self.module.mojom_namespace is not None else '',\n> > + 'proxy_name': 'IPAProxy%s' % ModuleClassName(self.module),\n> > + 'proxy_worker_name': 'IPAProxy%sWorker' % ModuleClassName(self.module),\n> > + 'structs_nonempty': [x for x in self.module.structs if len(x.fields) > 0],\n> > + 'year': datetime.datetime.now().year,\n> > + }\n> > +\n> > +\n> > + @UseJinja('module_generated.h.tmpl')\n> > + def _GenerateDataHeader(self):\n> > + return self._GetJinjaExports()\n> > +\n> > + @UseJinja('module_serializer.h.tmpl')\n> > + def _GenerateSerializer(self):\n> > + return self._GetJinjaExports()\n> > +\n> > + @UseJinja('module_ipa_proxy.cpp.tmpl')\n> > + def _GenerateProxyCpp(self):\n> > + return self._GetJinjaExports()\n> > +\n> > + @UseJinja('module_ipa_proxy.h.tmpl')\n> > + def _GenerateProxyHeader(self):\n> > + return self._GetJinjaExports()\n> > +\n> > + @UseJinja('module_ipa_proxy_worker.cpp.tmpl')\n> > + def _GenerateProxyWorker(self):\n> > + return self._GetJinjaExports()\n> > +\n> > + def GenerateFiles(self, unparsed_args):\n> > + parser = argparse.ArgumentParser()\n> > + parser.add_argument('--libcamera_generate_header', action='store_true')\n> > + parser.add_argument('--libcamera_generate_serializer', action='store_true')\n> > + parser.add_argument('--libcamera_generate_proxy_cpp', action='store_true')\n> > + parser.add_argument('--libcamera_generate_proxy_h', action='store_true')\n> > + parser.add_argument('--libcamera_generate_proxy_worker', action='store_true')\n> > + parser.add_argument('--libcamera_output_path')\n> > + args = parser.parse_args(unparsed_args)\n> > +\n> > + ValidateInterfaces(self.module.interfaces)\n> > +\n> > + fileutil.EnsureDirectoryExists(os.path.dirname(args.libcamera_output_path))\n> > +\n> > + module_name = ModuleName(self.module.path)\n> > +\n> > + if args.libcamera_generate_header:\n> > + self.Write(self._GenerateDataHeader(),\n> > + args.libcamera_output_path)\n> > +\n> > + if args.libcamera_generate_serializer:\n> > + self.Write(self._GenerateSerializer(),\n> > + args.libcamera_output_path)\n> > +\n> > + if args.libcamera_generate_proxy_cpp:\n> > + self.Write(self._GenerateProxyCpp(),\n> > + args.libcamera_output_path)\n> > +\n> > + if args.libcamera_generate_proxy_h:\n> > + self.Write(self._GenerateProxyHeader(),\n> > + args.libcamera_output_path)\n> > +\n> > + if args.libcamera_generate_proxy_worker:\n> > + self.Write(self._GenerateProxyWorker(),\n> > + args.libcamera_output_path)","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 5A104C3B5C\n\tfor ;\n\tMon, 5 Oct 2020 10:09:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C83F063BBF;\n\tMon, 5 Oct 2020 12:09:08 +0200 (CEST)","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 9282263B27\n\tfor ;\n\tMon, 5 Oct 2020 12:09:06 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 558283B;\n\tMon, 5 Oct 2020 12:09:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HPpQE20L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601892546;\n\tbh=HFqdm4PBmNmZlB7aKtSaOel9crl4zmE8JKB0g2Tp0aw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HPpQE20Lfn7Z/o8UIf6p1y5xCceZOJ6v0OoRfZHFjm9duUYCDKsah3lmeMKL4vw/1\n\toX6/9KU7JtiWzE8712i5zZb2tsdawHt7VgbCfLgkAgVZ0hinIavilZgj7KjTQxclXg\n\tLvtuNt8aUOIf5TuduO2yY5L27EXAfk9zU6HlPhC0=","Date":"Mon, 5 Oct 2020 19:08:57 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20201005100857.GH45948@pyrite.rasen.tech>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-7-paul.elder@ideasonboard.com>\n\t<20201004155640.GD8774@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201004155640.GD8774@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 06/38] utils: ipc: add templates\n\tfor code generation for IPC mechanism","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12999,"web_url":"https://patchwork.libcamera.org/comment/12999/","msgid":"<20201006002611.GE11021@pendragon.ideasonboard.com>","date":"2020-10-06T00:26:11","subject":"Re: [libcamera-devel] [PATCH v3 06/38] utils: ipc: add templates\n\tfor code generation for IPC mechanism","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Mon, Oct 05, 2020 at 07:08:57PM +0900, paul.elder@ideasonboard.com wrote:\n> On Sun, Oct 04, 2020 at 06:56:40PM +0300, Laurent Pinchart wrote:\n> > On Fri, Oct 02, 2020 at 11:31:22PM +0900, Paul Elder wrote:\n> > > Add templates to mojo to generate code for the IPC mechanism. These\n> > > templates generate:\n> > > - module header\n> > > - module serializer\n> > > - IPA proxy cpp, header, and worker\n> > > \n> > > Given an input data definition mojom file for a pipeline.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > \n> > > ---\n> > > Changes in v3:\n> > > - add support for namespaces\n> > > - fix enum assignment (used to have +1 for CMD applied to all enums)\n> > > - use readHeader, writeHeader, and eraseHeader as static class functions\n> > > of IPAIPCUnixSocket (in the proxy worker)\n> > > - add requirement that base controls *must* be defined in\n> > > libcamera::{pipeline_name}::Controls\n> > > \n> > > Changes in v2:\n> > > - mandate the main and callback interfaces, and init(), start(), stop()\n> > > and their parameters\n> > > - fix returning single pod value from IPC-called function\n> > > - add licenses\n> > > - improve auto-generated message\n> > > - other fixes related to serdes\n> > > ---\n> > > .../module_generated.h.tmpl | 106 ++++\n> > \n> > This isn't a great name, as _generated tells how the header is produced,\n> > but not what it actually is. How about module_ipa_interface.h.tmpl ?\n> \n> I named it like that because that's what the generated headers end up\n> getting called. There's the main header, like raspberrypi.h, and then\n> there's the generated one, like raspberrypi_generated.h.\n> \n> raspberrypi_ipa_interface.h ...?\n\nYes, I meant that the generated header could be called\nraspberrypi_ipa_interface.h (name subject to bikeshedding, the point is\nto not have a _generated suffix). It would actually, I think, be useful\nto name the generated header with the same base name as the .mojom file.\nThat way, from a pipeline handler and IPA author point of view, writing\n.mojom and including .h would be more intuitive than including\n_generated.h. A _generated suffix wouldn't cause the world to do of\ncourse, but making it more intuitive helps keeping the whole experience\nsmoother. That's a design rule I've learnt from Qt, if a user who hasn't\nread detailed documentation writes code intuitively and gets it right,\nthen your API design is good.\n\n> > > .../module_ipa_proxy.cpp.tmpl | 233 +++++++++\n> > > .../module_ipa_proxy.h.tmpl | 117 +++++\n> > > .../module_ipa_proxy_worker.cpp.tmpl | 184 +++++++\n> > > .../module_serializer.h.tmpl | 44 ++\n> > \n> > And maybe module_ipa_serializer.h.tmpl for consistency ?\n> \n> This becomes raspberrypi_serializer.h. raspberrypi_ipa_serializer.h ?\n> \n> Or just the template file name?\n\nBoth the template and generated files, yes.\n\n> > > .../libcamera_templates/proxy_functions.tmpl | 185 +++++++\n> > > .../libcamera_templates/serializer.tmpl | 276 +++++++++++\n> > > .../generators/mojom_libcamera_generator.py | 461 ++++++++++++++++++\n> > > 8 files changed, 1606 insertions(+)\n> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_generated.h.tmpl\n> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > > create mode 100644 utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl\n> > > create mode 100644 utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> > > create mode 100644 utils/ipc/generators/libcamera_templates/serializer.tmpl\n> > > create mode 100644 utils/ipc/generators/mojom_libcamera_generator.py\n> > > \n> > > diff --git a/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl\n> > > new file mode 100644\n> > > index 00000000..7dd48b0b\n> > > --- /dev/null\n> > > +++ b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl\n> > > @@ -0,0 +1,106 @@\n> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) {{ year }}, Google Inc.\n> > > + *\n> > > + * {{ module_name }}_generated.h - Image Processing Algorithm interface for {{ module_name }}\n> > > + *\n> > > + * This file is auto-generated. Do not edit.\n> > > + */\n> > > +\n> > > +#ifndef __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__\n> > > +#define __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__\n> > > +\n> > > +#include \n> > > +\n> > \n> > I think you can drop this blank line.\n> > \n> > > +#include \n> > > +\n> > > +{% if has_map %}#include {% endif %}\n> > > +{% if has_array %}#include {% endif %}\n> > > +\n> > > +namespace libcamera {\n> > > +{%- if has_namespace %}\n> > > +{% for ns in namespace %}\n> > > +namespace {{ns}} {\n> > \n> > There's a mix of {{ var }} and {{var}}. I don't mind much which one is\n> > used, but I think we should be consistent.\n> \n> Ah right, I forgot to fix that.\n> \n> > > +{% endfor %}\n> > > +{%- endif %}\n> > > +\n> > > +enum {{ cmd_enum_name }} {\n> > \n> > It would be nice to make this an enum class, to avoid namespace clashes.\n> > Alternatively, we could enforce the usage of namespaces for all IPAs.\n> \n> I made cmd_enum_name start with an underscore to avoid namespace\n> clashes.\n> \n> 'cmd_enum_name': '_%sCMD' % ModuleClassName(self.module),\n> \n> So like enum _RPiCMD. I thought the underscore would be sufficient for\n> anti name clashes.\n\nI meant namespace clashes for the enumerated values.\n\nenum Foo {\n\tEnumValue,\n};\n\nenum Bar {\n\tEnumValue,\n};\n\nwould make\n\n\tuint32_t val = EnumValue;\n\nambiguous, while with enum class you always have to specify the Foo:: or\nBar:: prefix.\n\n> > enum class also brings the added benefit of catching improper usages,\n> > but comes at a cost of requiring additional static_cast<> to convert\n> > from and to uint32_t.\n> \n> I thought you could use enum as a type anyway? That's what I did with\n> the enums generated from the mojom file, and they all needed\n> static_cast<>s. They're in the generated serializers:\n> \n> ret.op_ = static_cast(IPADataSerializer::deserialize(m, m + 4));\n\nBoth unscoped enums (i.e. non-class) and scoped enums (enum class) are\ntypes, so this good is good. The two main differences is that enumerated\nvalues always have to be specified with the type specified for an enum\nclass (Foo::Value), and implicit casts between integers and enum class\nare not allowed (you always need an explicit cast).\n\n\"Each enumerator becomes a named constant of the enumeration's type\n(that is, name), which is contained within the scope of the enumeration,\nand can be accessed using scope resolution operator. There are no\nimplicit conversions from the values of a scoped enumerator to integral\ntypes, although static_cast may be used to obtain the numeric value of\nthe enumerator.\"\n\n(https://en.cppreference.com/w/cpp/language/enum)\n\nScoped enums thus bring additional safety, but always require explicit\ncasts. For the generated serialization code that's not an issue at all\nas it's just a matter of getting templates right. For enums that define\nbitfields meant to be combined, that gets more difficult:\n\nenum class Foo {\n\tValue0 = (1 << 0),\n\tValue1 = (1 << 1),\n};\n\nvoid foo(Foo f);\n\n...\n\nfoo(Value0);\t\t\t/* OK */\nfoo(Value0 | Value1);\t\t/* Error */\n\n> > > +\tCMD_EXIT = 0,\n> > \n> > I'd drop the CMD_ prefix, especially if we use enum class, and use\n> > CamelCase for names instead of uppercase.\n> \n> This cmd enum is for RPC, so I thought it was fine for just this enum to\n> be prefix with CMD_, just to make it clear in the generated code that\n> that's what this is for. UnixSocketTest does this too (that's where I\n> got it from).\n\nThe current implementation is temporary code, so you can depart from it\n:-) No big deal though as this only matters in generated code, but if we\ngo for enum class and have to write _RPiCMD::CMD_EXIT, I think\n_RPiCMD::EXIT would be better.\n\nOn this topic, would it be difficult to go for CamelCase for types\n(_RPiCmd, CmdExit / Exit, ...) ? It's generated code, but if one has to\nread it for any reason, keeping the coding style consistent would be\nnice.\n\n> > > +{%- for method in interface_main.methods %}\n> > > +\tCMD_{{ method.mojom_name|upper }} = {{loop.index}},\n> > > +{%- endfor %}\n> > > +{%- set count = interface_main.methods|length -%}\n> > > +{%- for method in interface_cb.methods %}\n> > > +\tCMD_{{ method.mojom_name|upper }} = {{loop.index + count}},\n> > > +{%- endfor %}\n> > \n> > Should we split the enum in two, one for each interface ?\n> \n> Either way is fine, it doesn't matter much. That's why I put them\n> together. The code generator loops over interface_{main,cb}.methods and\n> not the enum when it generates both sides of the RPC calls, so putting\n> them in different or the same enum makes literally no difference.\n\nYou're right, shouldn't be a big deal.\n\n> > > +};\n> > > +\n> > > +{% for enum in enums %}\n> > > +enum {{ enum.mojom_name }} {\n> > > +{%- for field in enum.fields %}\n> > > +\t{{ field.mojom_name }} = {{ field.numeric_value }},\n> > > +{%- endfor %}\n> > > +};\n> > > +{% endfor %}\n> > > +\n> > > +{%- for struct in structs_nonempty %}\n> > > +struct {{ struct.mojom_name }}\n> > > +{\n> > > +public:\n> > > +\t{{ struct.mojom_name }}() {%- if struct|has_default_fields %} :{% endif %}\n> > > +{%- for field in struct.fields|with_default_values -%}\n> > > +{{\" \" if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ \", \" if not loop.last }}\n> > > +{%- endfor %} {}\n> > \n> > This generates really long lines. How about\n> > \n> > \t{{ struct.mojom_name }}() {%- if struct|has_default_fields %}\n> > \t\t:{% endif %}\n> > {%- for field in struct.fields|with_default_values -%}\n> > {{\" \" if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ \", \" if not loop.last }}\n> > {%- endfor %}\n> > \t{\n> > \t}\n> \n> Ah, right.\n> \n> > Would be great to wrap the initializers line too, but I think that's\n> > overkill.\n> > \n> > > +\t~{{ struct.mojom_name }}() {}\n> > > +\n> > > +\t{{ struct.mojom_name }}(\n> > > +{%- for field in struct.fields -%}\n> > > +{{ field|name }} {{ field.mojom_name }}{{ \", \" if not loop.last }}\n> > > +{%- endfor -%}\n> > > +) :\n> > > +{%- for field in struct.fields -%}\n> > > +{{\" \" if loop.first}}{{ field.mojom_name}}_({{ field.mojom_name }}){{ \", \" if not loop.last }}\n> > > +{%- endfor %} {}\n> > \n> > Same here.\n> > \n> > > +{% for field in struct.fields %}\n> > > +\t{{ field|name }} {{ field.mojom_name }}_;\n> > > +{%- endfor %}\n> > > +};\n> > > +{% endfor %}\n> > > +\n> > > +{#-\n> > > +What to do about the #includes? Should we forward-declare, or\n> > > +require {{module_name}}.h to include the required headers?\n> > > +For now I'm going for the latter, coz this header essentially\n> > > +extends {{module_name}}.h.\n> > > +#}\n> > \n> > Looking at the existing {{module_name}}.h, there are a few enums and\n> > constants that could be moved to {{module_name}}_generated.h, such as\n> > enum BufferMark or MaxLsGridSize in raspberrypi.h. What do you think ?\n> \n> Yeah enums I think are better to move to the mojom file. I did that for\n> ConfigParameters.\n> \n> mojo supports const (like if we had const uint32 MaxLsGridSize = 0x8000;)\n> but the code generator doesn't yet.\n> \n> > Then we have libcamera::RPi::Controls, which I'm wondering how to handle\n> > best. It's not strictly related to this series, but wouldn't they be\n> > better stored in the pipeline handler, and passed to the IPA ?\n> \n> I think it would be best if all ControlLists had a ControlInfoMap, then\n> it wouldn't matter where libcamera::RPi::controls goes.\n\nI think we should move in a direction that doesn't declare controls in a\nshared header. It could be done on top though.\n\n> > Finally there's the odd beast such as\n> > \n> > #define VIMC_IPA_FIFO_PATH \"/tmp/libcamera_ipa_vimc_fifo\"\n> > \n> > which I also wonder if it would make sense to pass it to the IPA at init\n> > time. {{module_name}}.h could then possibly be dropped.\n> \n> Oh you're thinking of dropping the main one entirely. Yeah that could\n> work too. I'm just worried if it'll be too restrictive. #defines could\n> be replaced with consts, so it's just the base ControlInfoMap that's an\n> issue.\n\nIdeally, it would be nice, but you are right that it may be too\nrestrictive.\n\n> > > +class {{ interface_name }} : public IPAInterface\n> > > +{\n> > > +public:\n> > > +\tvirtual ~{{interface_name}}() {};\n> > \n> > No need for the trailing ;\n> > \n> > > +{% for method in interface_main.methods %}\n> > > +\tvirtual {{method|method_return_value}} {{method.mojom_name}}(\n> > > +{%- for param in method|method_parameters %}\n> > > +\t\t{{param}}{{- \",\" if not loop.last}}\n> > > +{%- endfor -%}\n> > > +) = 0;\n> > > +{% endfor %}\n> > > +\n> > > +{%- for method in interface_cb.methods %}\n> > > +\tSignal<\n> > > +{%- for param in method.parameters -%}\n> > > +\t\tconst {{param|name}}{{\" &\" if not param|is_pod}}\n> > \n> > Should the const be dropped if param|is_pod ? Same below.\n> \n> Ah right it's pass-by-value. Sure, let's drop it.\n> \n> > > +\t\t{{- \", \" if not loop.last }}\n> > > +{%- endfor -%}\n> > > +> {{method.mojom_name}};\n> > > +{% endfor -%}\n> > > +};\n> > > +\n> > > +{%- if has_namespace %}\n> > > +{% for ns in namespace|reverse %}\n> > > +} /* {{ns}} */\n> > > +{% endfor %}\n> > > +{%- endif %}\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__ */\n> > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > > new file mode 100644\n> > > index 00000000..f3eeeaaa\n> > > --- /dev/null\n> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> > > @@ -0,0 +1,233 @@\n> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > > +{%- import \"proxy_functions.tmpl\" as proxy_funcs -%}\n> > > +\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) {{ year }}, Google Inc.\n> > > + *\n> > > + * ipa_proxy_{{ module_name }}.cpp - Image Processing Algorithm proxy for {{ module_name }}\n> > > + *\n> > > + * This file is auto-generated. Do not edit.\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > \n> > I think you can drop this, as it's included by {{module_name}}.h. Same\n> > below.\n> \n> Oh, okay.\n> \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/control_serializer.h\"\n> > > +#include \"libcamera/internal/ipa_ipc.h\"\n> > > +#include \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> > > +#include \"libcamera/internal/ipa_data_serializer.h\"\n> > > +#include \"libcamera/internal/ipa_module.h\"\n> > > +#include \"libcamera/internal/ipa_proxy.h\"\n> > > +#include \"libcamera/internal/ipc_unixsocket.h\"\n> > > +#include \"libcamera/internal/log.h\"\n> > > +#include \"libcamera/internal/process.h\"\n> > > +#include \"libcamera/internal/thread.h\"\n> > > +\n> > > +#include \n> > \n> > Should this be moved above with the other libcamera/ipa/ headers ?\n> \n> This is the header that corresponds to this cpp file. I thought that\n> gets special treatment and goes on its own at the end like here.\n\nIt gets special treatement, but goes at the top, not the bottom :-) The\nidea is to get every header compiled without anything else included\nbefore, to ensure they're self-contained.\n\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(IPAProxy)\n> > > +\n> > > +{%- if has_namespace %}\n> > > +{% for ns in namespace %}\n> > > +namespace {{ns}} {\n> > > +{% endfor %}\n> > > +{%- endif %}\n> > > +\n> > > +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)\n> > > +\t: IPAProxy(ipam), running_(false),\n> > > +\t isolate_(isolate)\n> > > +{\n> > > +\tLOG(IPAProxy, Debug)\n> > > +\t\t<< \"initializing dummy proxy: loading IPA from \"\n> > \n> > It's not a dummy proxy anymore :-)\n> \n> Oops :p\n> \n> > > +\t\t<< ipam->path();\n> > > +\n> > > +\tif (isolate_) {\n> > > +\t\tconst std::string proxy_worker_path = resolvePath(\"ipa_proxy_{{module_name}}\");\n> > > +\t\tif (proxy_worker_path.empty()) {\n> > > +\t\t\tLOG(IPAProxy, Error)\n> > > +\t\t\t\t<< \"Failed to get proxy worker path\";\n> > > +\t\t\treturn;\n> > > +\t\t}\n> > > +\n> > > +\t\tipc_ = std::make_unique(ipam->path().c_str(), proxy_worker_path.c_str());\n> > > +\t\tif (!ipc_->isValid()) {\n> > > +\t\t\tLOG(IPAProxy, Error) << \"Failed to create IPAIPC\";\n> > > +\t\t\treturn;\n> > > +\t\t}\n> > > +\n> > > +{% if interface_cb.methods|length > 0 %}\n> > > +\t\tipc_->recvIPC.connect(this, &{{proxy_name}}::recvIPC);\n> > > +{% endif %}\n> > > +\n> > > +\t\tvalid_ = true;\n> > > +\t\treturn;\n> > > +\t}\n> > \n> > There's very little code shared between the isolated and non-isolated\n> > paths. It could make sense to split this class in two. On the other\n> > hand, that would require the ability for the IPAManager to create the\n> > right class type, which may complicate things a bit, but maybe not much.\n> > For instance we could have the two classes called {{proxy_name}}Thread\n> > and {{proxy_name}}IPC, and have a {{proxy_name}} class defined as\n> > \n> > class {{proxy_name}}\n> > {\n> > public:\n> > \tusing IPC = {{proxy_name}}IPC;\n> > \tusing Thread = {{proxy_name}}Thread;\n> > };\n> > \n> > Then, in IPAManager::createIPA(), you would have\n> > \n> > \tstd::unique_ptr

proxy;\n> > \tif (self_->isSignatureValid(m)))\n> > \t\tproxy = std::make_unique(m);\n> > \telse\n> > \t\tproxy = std::make_unique(m);\n> \n> Yeah maybe. The amount of generated code doesn't change though, it'll\n> just be organized differently, and the only one that will notice the\n> difference is IPAManager as shown in this snippet.\n\nThe proxy wouldn't have to check isolate_ in every function. The\noverhead is negligible, but the generated code (and the template) would\nbe a bit easier to follow I think. It would also possibly be a good path\nforward to add support for different proxy types in the future if needed\n(for instance a Chrome OS-specific proxy). If it's an easy change this\nwould be nice, otherwise it's no big deal for now.\n\n> > > +\n> > > +\tif (!ipam->load())\n> > > +\t\treturn;\n> > > +\n> > > +\tIPAInterface *ipai = ipam->createInterface();\n> > > +\tif (!ipai) {\n> > > +\t\tLOG(IPAProxy, Error)\n> > > +\t\t\t<< \"Failed to create IPA context for \" << ipam->path();\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tipa_ = std::unique_ptr<{{interface_name}}>(dynamic_cast<{{interface_name}} *>(ipai));\n> > > +\tproxy_.setIPA(ipa_.get());\n> > > +\n> > > +{% for method in interface_cb.methods %}\n> > > +\tipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);\n> > > +{%- endfor %}\n> > > +\n> > > +\tvalid_ = true;\n> > > +}\n> > > +\n> > > +{{proxy_name}}::~{{proxy_name}}()\n> > > +{\n> > > +\tif (isolate_)\n> > > +\t\tipc_->sendAsync(CMD_EXIT, {}, {});\n> > > +}\n> > > +\n> > > +{% if interface_cb.methods|length > 0 %}\n> > > +void {{proxy_name}}::recvIPC(std::vector &data, std::vector &fds)\n> > > +{\n> > > +\tuint32_t cmd = (data[0] & 0xff) |\n> > > +\t\t ((data[1] & 0xff) << 8) |\n> > > +\t\t ((data[2] & 0xff) << 16) |\n> > > +\t\t ((data[3] & 0xff) << 24);\n> > \n> > As data is a vector of uint8_t, I think the & 0xff is unnecessary.\n> > \n> > Should the data length be validated ?\n> \n> tbh I was thinking about it. I think cmd and vec are guaranteed, though.\n> Maybe we should check it just in case.\n> \n> > > +\n> > > +\t/* Need to skip another 4 bytes for the sequence number. */\n> > > +\tstd::vector vec(data.begin() + 8, data.end());\n> > \n> > This creates a copy, which isn't very nice :-/ I think the IPC class\n> > interface would benefit from switching from std::vector to Span.\n> \n> Ah okay, I'll look into it.\n> \n> > > +\tswitch (cmd) {\n> > > +{%- for method in interface_cb.methods %}\n> > > +\tcase CMD_{{method.mojom_name|upper}}: {\n> > > +\t\t{{method.mojom_name}}IPC(vec, fds);\n> > > +\t\tbreak;\n> > > +\t}\n> > > +{%- endfor %}\n> > \n> > Should we log unknown commands ?\n> \n> Oh maybe.\n> \n> > > +\t}\n> > > +}\n> > > +{%- endif %}\n> > > +\n> > > +{% for method in interface_main.methods %}\n> > > +{{proxy_funcs.func_sig(proxy_name, method)}}\n> > > +{\n> > > +\tif (isolate_)\n> > > +\t\t{{\"return \" if method|method_return_value != \"void\"}}{{method.mojom_name}}IPC(\n> > > +{%- for param in method|method_param_names -%}\n> > > +\t\t{{param}}{{- \", \" if not loop.last}}\n> > > +{%- endfor -%}\n> > > +);\n> > > +\telse\n> > > +\t\t{{\"return \" if method|method_return_value != \"void\"}}{{method.mojom_name}}Thread(\n> > > +{%- for param in method|method_param_names -%}\n> > > +\t\t{{param}}{{- \", \" if not loop.last}}\n> > > +{%- endfor -%}\n> > > +);\n> > > +}\n> > > +\n> > > +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> > > +{\n> > > +{%- if method.mojom_name == \"init\" %}\n> > > +\t{{proxy_funcs.init_thread_body()}}\n> > > +{% elif method.mojom_name == \"start\" %}\n> > \n> > I think you're missing a - before elif, here and below.\n> \n> This, I don't think so.\n> \n> > > +\t{{proxy_funcs.start_thread_body()}}\n> > > +{% elif method.mojom_name == \"stop\" %}\n> > > +\t{{proxy_funcs.stop_thread_body()}}\n> > \n> > Instead of using macros, would it make sense to move the code to the\n> > base IPAProxy class ?\n> \n> Maybe...\n> \n> > > +{% elif not method|is_async %}\n> > > +\tipa_->{{method.mojom_name}}(\n> > > +\t{%- for param in method|method_param_names -%}\n> > > +\t\t{{param}}{{- \", \" if not loop.last}}\n> > > +\t{%- endfor -%}\n> > > +);\n> > > +{% elif method|is_async %}\n> > > +\tproxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,\n> > > +\t{%- for param in method|method_param_names -%}\n> > > +\t\t{{param}}{{- \", \" if not loop.last}}\n> > > +\t{%- endfor -%}\n> > > +);\n> > > +{%- endif %}\n> > > +}\n> > > +\n> > > +{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n> > > +{\n> > > +{%- set has_input = true if method|method_param_inputs|length > 0 %}\n> > > +{%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != \"void\" %}\n> > > +{% if has_input %}\n> > \n> > Shouldn't it be {%- ?\n> \n> Probably... I don't remember if this specific one caused other problems.\n> \n> > > +\tstd::vector _ipcInputBuf;\n> > > +{%- if method|method_input_has_fd %}\n> > > +\tstd::vector _ipcInputFds;\n> > > +{%- endif %}\n> > > +{%- endif %}\n> > > +{%- if has_output %}\n> > > +\tstd::vector _ipcOutputBuf;\n> > > +{%- if method|method_output_has_fd %}\n> > > +\tstd::vector _ipcOutputFds;\n> > > +{%- endif %}\n> > > +{%- endif %}\n> > > +\n> > > +{{proxy_funcs.serialize_call(method|method_param_inputs, '_ipcInputBuf', '_ipcInputFds', base_controls)}}\n> > > +\n> > > +{%- set input_buf = \"_ipcInputBuf\" if has_input else \"{}\" %}\n> > > +{%- set fds_buf = \"_ipcInputFds\" if method|method_input_has_fd else \"{}\" %}\n> > > +{%- set cmd = \"CMD_\" + method.mojom_name|upper %}\n> > > +{% if method|is_async %}\n> > > +\tint ret = ipc_->sendAsync({{cmd}}, {{input_buf}}, {{fds_buf}});\n> > > +{%- else %}\n> > > +\tint ret = ipc_->sendSync({{cmd}}, {{input_buf}}, {{fds_buf}}\n> > > +{{- \", &_ipcOutputBuf\" if has_output -}}\n> > > +{{- \", &_ipcOutputFds\" if has_output and method|method_output_has_fd -}}\n> > > +);\n> > > +{%- endif %}\n> > > +\tif (ret < 0) {\n> > > +\t\tLOG(IPAProxy, Error) << \"Failed to call {{method.mojom_name}}\";\n> > > +{%- if method|method_return_value != \"void\" %}\n> > > +\t\treturn static_cast<{{method|method_return_value}}>(ret);\n> > > +{%- else %}\n> > > +\t\treturn;\n> > > +{%- endif %}\n> > > +\t}\n> > > +\tLOG(IPAProxy, Debug) << \"Done calling {{method.mojom_name}}\";\n> > \n> > Is this message helpful for debugging ? I think we should work at some\n> > point on a tracing mechanism instead of manually tracing calls.\n> \n> It was while I wasn't sure the calls were going through. I think at this\n> point it's stable enough that we can remove it.\n> \n> Yeah tracing would be nice :)\n\nI've experimented with lttng-ust, I think we'll generate tracepoints\nautomatically in the future :-)\n\n> > > +{% if method|method_return_value != \"void\" %}\n> > > +\treturn IPADataSerializer<{{method.response_parameters|first|name}}>::deserialize(_ipcOutputBuf, 0);\n> > > +{% elif method|method_param_outputs|length > 0 %}\n> > > +{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf', '_ipcOutputFds')}}\n> > > +{% endif -%}\n> > > +}\n> > > +\n> > > +{% endfor %}\n> > > +\n> > > +\n> > \n> > One blank line should be enough.\n> \n> Ah, right.\n> \n> > > +{% for method in interface_cb.methods %}\n> > > +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\")}}\n> > > +{\n> > > +\t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> > > +}\n> > > +\n> > > +void {{proxy_name}}::{{method.mojom_name}}IPC(\n> > > +\tstd::vector &data,\n> > > +\t[[maybe_unused]] std::vector &fds)\n> > > +{ \n> > > +{%- for param in method.parameters %}\n> > > +\t{{param|name}} {{param.mojom_name}};\n> > > +{%- endfor %}\n> > > +{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false)}}\n> > > +\t{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});\n> > > +}\n> > > +{% endfor %}\n> > > +\n> > > +{%- if has_namespace %}\n> > > +{% for ns in namespace|reverse %}\n> > > +} /* {{ns}} */\n> > > +{% endfor %}\n> > > +{%- endif %}\n> > > +} /* namespace libcamera */\n> > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > > new file mode 100644\n> > > index 00000000..7d2bfb6c\n> > > --- /dev/null\n> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl\n> > > @@ -0,0 +1,117 @@\n> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > > +{%- import \"proxy_functions.tmpl\" as proxy_funcs -%}\n> > > +\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) {{ year }}, Google Inc.\n> > > + *\n> > > + * ipa_proxy_{{ module_name }}.h - Image Processing Algorithm proxy for {{ module_name }}\n> > > + *\n> > > + * This file is auto-generated. Do not edit.\n> > > + */\n> > > +\n> > > +#ifndef __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__\n> > > +#define __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/control_serializer.h\"\n> > > +#include \"libcamera/internal/ipa_ipc.h\"\n> > > +#include \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> > > +#include \"libcamera/internal/ipa_proxy.h\"\n> > > +#include \"libcamera/internal/ipc_unixsocket.h\"\n> > > +#include \"libcamera/internal/thread.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +{%- if has_namespace %}\n> > > +{% for ns in namespace %}\n> > > +namespace {{ns}} {\n> > > +{% endfor %}\n> > > +{%- endif %}\n> > > +\n> > > +class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object\n> > > +{\n> > > +public:\n> > > +\t{{proxy_name}}(IPAModule *ipam, bool isolate);\n> > > +\t~{{proxy_name}}();\n> > > +\n> > > +{% for method in interface_main.methods %}\n> > > +{{proxy_funcs.func_sig(proxy_name, method, \"\", false, true)|indent(8, true)}};\n> > > +{% endfor %}\n> > > +\n> > > +{%- for method in interface_cb.methods %}\n> > > +\tSignal<\n> > > +{%- for param in method.parameters -%}\n> > > +\t\tconst {{param|name}}{{\" &\" if not param|is_pod}}\n> > > +\t\t{{- \", \" if not loop.last }}\n> > > +{%- endfor -%}\n> > > +> {{method.mojom_name}};\n> > > +{% endfor %}\n> > > +\n> > > +private:\n> > > +\tvoid recvIPC(std::vector &data, std::vector &fds);\n> > > +\n> > > +{% for method in interface_main.methods %}\n> > > +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> > > +{{proxy_funcs.func_sig(proxy_name, method, \"IPC\", false)|indent(8, true)}};\n> > > +{% endfor %}\n> > > +{% for method in interface_cb.methods %}\n> > > +{{proxy_funcs.func_sig(proxy_name, method, \"Thread\", false)|indent(8, true)}};\n> > > +\tvoid {{method.mojom_name}}IPC(\n> > > +\t\tstd::vector &data,\n> > > +\t\tstd::vector &fds);\n> > \n> > Should these be const ?\n> \n> Yeah.\n> \n> > > +{% endfor %}\n> > > +\n> > > +\t/* Helper class to invoke async functions in another thread. */\n> > > +\tclass ThreadProxy : public Object\n> > > +\t{\n> > > +\tpublic:\n> > > +\t\tvoid setIPA({{interface_name}} *ipa)\n> > > +\t\t{\n> > > +\t\t\tipa_ = ipa;\n> > > +\t\t}\n> > > +\n> > > +\t\tint start()\n> > > +\t\t{\n> > > +\t\t\treturn ipa_->start();\n> > > +\t\t}\n> > > +\n> > > +\t\tvoid stop()\n> > > +\t\t{\n> > > +\t\t\tipa_->stop();\n> > > +\t\t}\n> > > +{% for method in interface_main.methods %}\n> > > +{%- if method|is_async %}\n> > > +\t\t{{proxy_funcs.func_sig(proxy_name, method, \"\", false)|indent(16)}}\n> > > +\t\t{\n> > > +\t\t\tipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> > > +\t\t}\n> > > +{%- endif %}\n> > > +{%- endfor %}\n> > \n> > I think this is mixing two concepts, which shouldn't cause any issue for\n> > now, but may come bite us back later.\n> > \n> > As you've noticed in the existing IPAProxyThread implementation, we call\n> > some methods directly, and proxy other methods across threads. The idea\n> > is that anything called before start() is a direct call, and anything\n> > from start() until but not including stop() is indirect. The reason is\n> > that before start() there's no thread running, so we can't make a\n> > cross-thread call.\n> \n> Ah, so sync vs async is different from non-thread call vs thread call...\n> \n> > One direct consequence of this architecture is that all calls before\n> > start() are synchronous, and can thus return a value. Calls after\n> > start() are asynchronous, but that's a separate design decision. We\n> > could synchronize cross-thread calls if we wanted, we have chosen not to\n> > in order to avoid delays that could damage the real time performance of\n> > the pipeline handler.\n> > \n> > With IPC the situation is different, all calls are cross-process. Still,\n> > we want to expose a similar behaviour to pipeline handlers, so calls\n> > before start() are synchronous, and further calls are asynchronous.\n> > \n> > I don't think we can easily enforce this through the IDL, but I believe\n> > it would make sense to document this requirements, so that authors will\n> > not create an async method meant to be called before start(), or a\n> > synchronous method after start().\n> \n> Yeah, I think documentation should be sufficient.\n> \n> > > +\n> > > +\tprivate:\n> > > +\t\t{{interface_name}} *ipa_;\n> > > +\t};\n> > > +\n> > > +\tbool running_;\n> > > +\tThread thread_;\n> > > +\tThreadProxy proxy_;\n> > > +\tstd::unique_ptr<{{interface_name}}> ipa_;\n> > > +\n> > > +\tconst bool isolate_;\n> > > +\n> > > +\tstd::unique_ptr ipc_;\n> > > +\n> > > +\tControlSerializer controlSerializer_;\n> > > +};\n> > > +\n> > > +{%- if has_namespace %}\n> > > +{% for ns in namespace|reverse %}\n> > > +} /* {{ns}} */\n> > > +{% endfor %}\n> > > +{%- endif %}\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__ */\n> > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > > new file mode 100644\n> > > index 00000000..1647a9ca\n> > > --- /dev/null\n> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl\n> > > @@ -0,0 +1,184 @@\n> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > > +{%- import \"proxy_functions.tmpl\" as proxy_funcs -%}\n> > > +\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) {{ year }}, Google Inc.\n> > > + *\n> > > + * ipa_proxy_{{ module_name }}_worker.cpp - Image Processing Algorithm proxy worker for {{ module_name }}\n> > > + *\n> > > + * This file is auto-generated. Do not edit.\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/camera_sensor.h\"\n> > > +#include \"libcamera/internal/control_serializer.h\"\n> > > +#include \"libcamera/internal/ipa_data_serializer.h\"\n> > > +#include \"libcamera/internal/ipa_ipc_unixsocket.h\"\n> > > +#include \"libcamera/internal/ipa_module.h\"\n> > > +#include \"libcamera/internal/ipa_proxy.h\"\n> > > +#include \"libcamera/internal/ipc_unixsocket.h\"\n> > > +#include \"libcamera/internal/log.h\"\n> > > +#include \"libcamera/internal/thread.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +LOG_DEFINE_CATEGORY({{proxy_name}}Worker)\n> > > +\n> > > +{%- if has_namespace %}\n> > > +{% for ns in namespace -%}\n> > > +using namespace {{ns}};\n> > > +{% endfor %}\n> > > +{%- endif %}\n> > > +\n> > > +struct CallData {\n> > > +\tIPCUnixSocket::Payload *response;\n> > > +\tbool done;\n> > > +};\n> > > +\n> > > +{{interface_name}} *ipa_;\n> > > +IPCUnixSocket socket_;\n> > > +std::map callData_;\n> > \n> > callData_ doesn't seem to be used (and thus struct CallData shouldn't be\n> > needed).\n> \n> Oops, remnant of copying from UnixSocket.\n> \n> > > +\n> > > +ControlSerializer controlSerializer_;\n> > > +\n> > > +bool exit_ = false;\n> > \n> > Should you group all this, as well as the functions below, in a\n> > {{module_name}}ProxyWorker class ?\n> \n> I could. It's standalone so I didn't see much point in doing so though.\n\nJust for a readability point of view, up to you.\n\n> > > +\n> > > +void readyRead(IPCUnixSocket *socket)\n> > > +{\n> > > +\tIPCUnixSocket::Payload _message, _response;\n> > > +\tint _ret = socket->receive(&_message);\n> > > +\tif (_ret) {\n> > > +\t\tLOG({{proxy_name}}Worker, Error)\n> > > +\t\t\t<< \"Receive message failed\" << _ret;\n> > > +\t\treturn;\n> > > +\t}\n> > > +\n> > > +\tuint32_t _cmd, _seq;\n> > > +\tstd::tie(_cmd, _seq) = IPAIPCUnixSocket::readHeader(_message);\n> > > +\tIPAIPCUnixSocket::eraseHeader(_message);\n> > \n> > Using Span would help avoiding copies.\n> \n> Ah...\n> \n> > > +\n> > > +\tswitch (_cmd) {\n> > > +\tcase CMD_EXIT: {\n> > > +\t\texit_ = true;\n> > > +\t\tbreak;\n> > > +\t}\n> > > +\n> > > +{% for method in interface_main.methods %}\n> > > +\tcase CMD_{{method.mojom_name|upper}}: {\n> > > +\t{{proxy_funcs.deserialize_call(method|method_param_inputs, '_message.data', '_message.fds', false, true)|indent(8, true)}}\n> > > +{% for param in method|method_param_outputs %}\n> > > +\t\t{{param|name}} {{param.mojom_name}};\n> > > +{% endfor %}\n> > > +{%- if method|method_return_value != \"void\" %}\n> > > +\t\t{{method|method_return_value}} _callRet = ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});\n> > > +{%- else %}\n> > > +\t\tipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}}\n> > > +{{- \", \" if method|method_param_outputs|params_comma_sep -}}\n> > > +{%- for param in method|method_param_outputs -%}\n> > > +&{{param.mojom_name}}{{\", \" if not loop.last}}\n> > > +{%- endfor -%}\n> > > +);\n> > > +{%- endif %}\n> > > +{% if not method|is_async %}\n> > > +\t\tIPAIPCUnixSocket::writeHeader(_response, _cmd, _seq);\n> > > +{%- if method|method_return_value != \"void\" %}\n> > > +\t\tstd::vector _callRetBuf;\n> > > +\t\tstd::tie(_callRetBuf, std::ignore) =\n> > > +\t\t\tIPADataSerializer<{{method|method_return_value}}>::serialize(_callRet);\n> > > +\t\t_response.data.insert(_response.data.end(), _callRetBuf.begin(), _callRetBuf.end());\n> > > +{%- else %}\n> > > +\t{{proxy_funcs.serialize_call(method|method_param_outputs, \"_response.data\", \"_response.fds\", base_controls)|indent(8, true)}}\n> > > +{%- endif %}\n> > > +\t\tint _ret = socket_.send(_response);\n> > > +\t\tif (_ret < 0) {\n> > > +\t\t\tLOG({{proxy_name}}Worker, Error)\n> > > +\t\t\t\t<< \"Reply to {{method.mojom_name}}() failed\" << _ret;\n> > > +\t\t}\n> > > +\t\tLOG({{proxy_name}}Worker, Debug) << \"Done replying to {{method.mojom_name}}()\";\n> > > +{%- endif %}\n> > > +\t\tbreak;\n> > > +\t}\n> > > +{% endfor %}\n> > \n> > This isn't a huge deal, but the generated code my be a bit clearer if\n> > each case called a function.\n> \n> It does! ipa_->function(); :) It just comes with deserialization\n> beforehand and serialization afterward for synchronous calls. I think\n> it's good enough...\n> \n> > > +\t}\n> > > +}\n> > > +\n> > > +{% for method in interface_cb.methods %}\n> > > +{{proxy_funcs.func_sig(proxy_name, method, \"\", false)}}\n> > > +{\n> > > +\tIPCUnixSocket::Payload _message;\n> > > +\n> > > +\tIPAIPCUnixSocket::writeHeader(_message, CMD_{{method.mojom_name|upper}}, 0);\n> > > +\t{{proxy_funcs.serialize_call(method|method_param_inputs, \"_message.data\", \"_message.fds\", base_controls)}}\n> > > +\n> > > +\tsocket_.send(_message);\n> > > +\n> > > +\tLOG({{proxy_name}}Worker, Debug) << \"{{method.mojom_name}} done\";\n> > > +}\n> > > +{%- endfor %}\n> > \n> > You can drop the -\n> \n> Yeah, this one needs to be dropped.\n> \n> > > +\n> > > +int main(int argc, char **argv)\n> > > +{\n> > > +\t/* Uncomment this for debugging. */\n> > > +#if 0\n> > > +\tstd::string logPath = \"/tmp/libcamera.worker.\" +\n> > > +\t\t\t std::to_string(getpid()) + \".log\";\n> > > +\tlogSetFile(logPath.c_str());\n> > > +#endif\n> > > +\n> > > +\tif (argc < 3) {\n> > > +\t\tLOG({{proxy_name}}Worker, Debug)\n> > \n> > Maybe s/Debug/Error/ ?\n> \n> Ah yes.\n> \n> > > +\t\t\t<< \"Tried to start worker with no args\";\n> > > +\t\treturn EXIT_FAILURE;\n> > > +\t}\n> > > +\n> > > +\tint fd = std::stoi(argv[2]);\n> > > +\tLOG({{proxy_name}}Worker, Debug)\n> > \n> > And Info ?\n> \n> Yeah that's better too.\n> \n> > > +\t\t<< \"Starting worker for IPA module \" << argv[1]\n> > > +\t\t<< \" with IPC fd = \" << fd;\n> > > +\n> > > +\tstd::unique_ptr ipam = std::make_unique(argv[1]);\n> > > +\tif (!ipam->isValid() || !ipam->load()) {\n> > > +\t\tLOG({{proxy_name}}Worker, Error)\n> > > +\t\t\t<< \"IPAModule \" << argv[1] << \" should be valid but isn't\";\n> > \n> > Maybe just \" isn't valid\" ?\n> \n> ack\n> \n> > > +\t\treturn EXIT_FAILURE;\n> > > +\t}\n> > > +\n> > > +\tif (socket_.bind(fd) < 0) {\n> > > +\t\tLOG({{proxy_name}}Worker, Error) << \"IPC socket binding failed\";\n> > > +\t\treturn EXIT_FAILURE;\n> > > +\t}\n> > > +\tsocket_.readyRead.connect(&readyRead);\n> > > +\n> > > +\tipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());\n> > > +\tif (!ipa_) {\n> > > +\t\tLOG({{proxy_name}}Worker, Error) << \"Failed to create IPA interface instance\";\n> > > +\t\treturn EXIT_FAILURE;\n> > > +\t}\n> > > +{% for method in interface_cb.methods %}\n> > > +\tipa_->{{method.mojom_name}}.connect(&{{method.mojom_name}});\n> > > +{%- endfor %}\n> > > +\n> > > +\tLOG({{proxy_name}}Worker, Debug) << \"Proxy worker successfully started\";\n> > > +\n> > > +\t/* \\todo upgrade listening loop */\n> > \n> > Good point. We should make an EventLoop class at some point.\n> \n> I'm pretty sure I copied this from somewhere... I can't remember where.\n> \n> > > +\tEventDispatcher *dispatcher = Thread::current()->eventDispatcher();\n> > > +\twhile (!exit_)\n> > > +\t\tdispatcher->processEvents();\n> > > +\n> > > +\tdelete ipa_;\n> > > +\tsocket_.close();\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > diff --git a/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl\n> > > new file mode 100644\n> > > index 00000000..37352c1f\n> > > --- /dev/null\n> > > +++ b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl\n> > > @@ -0,0 +1,44 @@\n> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > > +{%- import \"serializer.tmpl\" as serializer -%}\n> > > +\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) {{ year }}, Google Inc.\n> > > + *\n> > > + * {{ module_name }}_serializer.h - Image Processing Algorithm data serializer for {{ module_name }}\n> > > + *\n> > > + * This file is auto-generated. Do not edit.\n> > > + */\n> > > +\n> > > +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__\n> > > +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/control_serializer.h\"\n> > > +#include \"libcamera/internal/ipa_data_serializer.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DECLARE_CATEGORY(IPADataSerializer)\n> > > +{% for struct in structs_nonempty %}\n> > > +template<>\n> > > +class IPADataSerializer<{{ struct|name_full(namespace_str) }}>\n> > > +{\n> > > +public:\n> > > +{{- serializer.serializer(struct, base_controls, namespace_str) }}\n> > > +{%- if struct|has_fd %}\n> > > +{{ serializer.deserializer_fd(struct, namespace_str) }}\n> > > +{%- else %}\n> > > +{{ serializer.deserializer_no_fd(struct, namespace_str) }}\n> > > +{%- endif %}\n> > > +};\n> > > +{% endfor %}\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__ */\n> > \n> > I'll review proxy_functions.tmpl and serializer.tmpl separately, this is\n> > growing quite big.\n> \n> Okay. Thank you for the reviews so far.\n> \n> > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> > > new file mode 100644\n> > > index 00000000..e1f5a20c\n> > > --- /dev/null\n> > > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl\n> > > @@ -0,0 +1,185 @@\n> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > > +{#\n> > > + # \\brief Generate fuction prototype\n> > > + #\n> > > + # \\param class Class name\n> > > + # \\param method mojom Method object\n> > > + # \\param suffix Suffix to append to \\a method function name\n> > > + # \\param need_class_name True to generate class name with function\n> > > + # \\param override True to generate override tag after the function prototype\n> > > + #}\n> > > +{%- macro func_sig(class, method, suffix, need_class_name = true, override = false) -%}\n> > > +{{method|method_return_value}} {{ class + \"::\" if need_class_name }}{{method.mojom_name}}{{suffix}}(\n> > > +{%- for param in method|method_parameters %}\n> > > +\t{{param}}{{- \",\" if not loop.last}}\n> > > +{%- endfor -%}\n> > > +){{\" override\" if override}}\n> > > +{%- endmacro -%}\n> > > +\n> > > +{#\n> > > + # \\brief Generate function body for IPA init() function for thread\n> > > + #}\n> > > +{%- macro init_thread_body() -%}\n> > > +\tint ret = ipa_->init(settings);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tproxy_.moveToThread(&thread_);\n> > > +\n> > > +\treturn 0;\n> > > +{%- endmacro -%}\n> > > +\n> > > +{#\n> > > + # \\brief Generate function body for IPA start() function for thread\n> > > + #}\n> > > +{%- macro start_thread_body() -%}\n> > > +\trunning_ = true;\n> > > +\tthread_.start();\n> > > +\n> > > +\treturn proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);\n> > > +{%- endmacro -%}\n> > > +\n> > > +{#\n> > > + # \\brief Generate function body for IPA stop() function for thread\n> > > + #}\n> > > +{%- macro stop_thread_body() -%}\n> > > +\tif (!running_)\n> > > +\t\treturn;\n> > > +\n> > > +\trunning_ = false;\n> > > +\n> > > +\tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n> > > +\n> > > +\tthread_.exit();\n> > > +\tthread_.wait();\n> > > +{%- endmacro -%}\n> > > +\n> > > +\n> > > +{#\n> > > + # \\brief Serialize multiple objects into data buffer and fd vector\n> > > + #\n> > > + # Generate code to serialize multiple objects, as specified in \\a params\n> > > + # (which are the parameters to some function), into \\a buf data buffer and\n> > > + # \\a fds fd vector.\n> > > + # This code is meant to be used by the proxy, for serializing prior to IPC calls.\n> > > + #}\n> > > +{%- macro serialize_call(params, buf, fds, base_controls) %}\n> > > +{%- for param in params %}\n> > > +\tstd::vector {{param.mojom_name}}Buf;\n> > > +{%- if param|has_fd %}\n> > > +\tstd::vector {{param.mojom_name}}Fds;\n> > > +\tstd::tie({{param.mojom_name}}Buf, {{param.mojom_name}}Fds) =\n> > > +{%- else %}\n> > > +\tstd::tie({{param.mojom_name}}Buf, std::ignore) =\n> > > +{%- endif %}\n> > > +{%- if param|is_controls %}\n> > > +\t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}, {{param.mojom_name}}.infoMap() ? *{{param.mojom_name}}.infoMap() : {{base_controls}}\n> > > +{%- else %}\n> > > +\t\tIPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}\n> > > +{%- endif %}\n> > > +{{- \", &controlSerializer_\" if param|needs_control_serializer -}}\n> > > +);\n> > > +{%- endfor %}\n> > > +\n> > > +{%- if params|length > 1 %}\n> > > +{%- for param in params %}\n> > > +\tappendUInt({{buf}}, {{param.mojom_name}}Buf.size());\n> > > +{%- if param|has_fd %}\n> > > +\tappendUInt({{buf}}, {{param.mojom_name}}Fds.size());\n> > > +{%- endif %}\n> > > +{%- endfor %}\n> > > +{%- endif %}\n> > > +\n> > > +{%- for param in params %}\n> > > +\t{{buf}}.insert({{buf}}.end(), {{param.mojom_name}}Buf.begin(), {{param.mojom_name}}Buf.end());\n> > > +{%- endfor %}\n> > > +\n> > > +{%- for param in params %}\n> > > +{%- if param|has_fd %}\n> > > +\t{{fds}}.insert({{fds}}.end(), {{param.mojom_name}}Fds.begin(), {{param.mojom_name}}Fds.end());\n> > > +{%- endif %}\n> > > +{%- endfor %}\n> > > +{%- endmacro -%}\n> > > +\n> > > +\n> > > +{#\n> > > + # \\brief Deserialize a single object from data buffer and fd vector\n> > > + #\n> > > + # \\param pointer True deserializes the object into a dereferenced pointer\n> > > + #\n> > > + # Generate code to deserialize a single object, as specified in \\a param,\n> > > + # from \\a buf data buffer and \\a fds fd vector.\n> > > + # This code is meant to be used by macro deserialize_call.\n> > > + #}\n> > > +{%- macro deserialize_param(param, pointer, loop, buf, fds) -%}\n> > > +{{\"*\" if pointer}}{{param.mojom_name}} = IPADataSerializer<{{param|name}}>::deserialize(\n> > > +\t{{buf}}.begin() + {{param.mojom_name}}Start,\n> > > +{%- if loop.last %}\n> > > +\t{{buf}}.end()\n> > > +{%- else %}\n> > > +\t{{buf}}.begin() + {{param.mojom_name}}Start + {{param.mojom_name}}BufSize\n> > > +{%- endif -%}\n> > > +{{- \",\" if param|has_fd }}\n> > > +{%- if param|has_fd %}\n> > > +\t{{fds}}.begin() + {{param.mojom_name}}FdStart,\n> > > +{%- if loop.last %}\n> > > +\t{{fds}}.end()\n> > > +{%- else %}\n> > > +\t{{fds}}.begin() + {{param.mojom_name}}FdStart + {{param.mojom_name}}FdsSize\n> > > +{%- endif -%}\n> > > +{%- endif -%}\n> > > +{{- \",\" if param|needs_control_serializer }}\n> > > +{%- if param|needs_control_serializer %}\n> > > +\t&controlSerializer_\n> > > +{%- endif -%}\n> > > +);\n> > > +{%- endmacro -%}\n> > > +\n> > > +\n> > > +{#\n> > > + # \\brief Deserialize multiple objects from data buffer and fd vector\n> > > + #\n> > > + # \\param pointer True deserializes objects into pointers, and adds a null check.\n> > > + # \\param declare True declares the objects in addition to deserialization.\n> > > + #\n> > > + # Generate code to deserialize multiple objects, as specified in \\a params\n> > > + # (which are the parameters to some function), from \\a buf data buffer and\n> > > + # \\a fds fd vector.\n> > > + # This code is meant to be used by the proxy, for serializing prior to IPC calls.\n> > > + #}\n> > > +{%- macro deserialize_call(params, buf, fds, pointer = true, declare = false) -%}\n> > > +{% set ns = namespace(size_offset = 0) %}\n> > > +{%- if params|length > 1 %}\n> > > +{%- for param in params %}\n> > > +\t[[maybe_unused]] size_t {{param.mojom_name}}BufSize = readUInt({{buf}}, {{ns.size_offset}});\n> > > +\t{%- set ns.size_offset = ns.size_offset + 4 %}\n> > > +{%- if param|has_fd %}\n> > > +\t[[maybe_unused]] size_t {{param.mojom_name}}FdsSize = readUInt({{buf}}, {{ns.size_offset}});\n> > > +\t{%- set ns.size_offset = ns.size_offset + 4 %}\n> > > +{%- endif %}\n> > > +{%- endfor %}\n> > > +{%- endif %}\n> > > +{% for param in params %}\n> > > +{%- if loop.first %}\n> > > +\tsize_t {{param.mojom_name}}Start = {{ns.size_offset}};\n> > > +{%- else %}\n> > > +\tsize_t {{param.mojom_name}}Start = {{loop.previtem.mojom_name}}Start + {{loop.previtem.mojom_name}}BufSize;\n> > > +{%- endif %}\n> > > +{%- endfor %}\n> > > +{% for param in params|with_fds %}\n> > > +{%- if loop.first %}\n> > > +\tsize_t {{param.mojom_name}}FdStart = 0;\n> > > +{%- elif not loop.last %}\n> > > +\tsize_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;\n> > > +{%- endif %}\n> > > +{%- endfor %}\n> > > +{% for param in params %}\n> > > +\t{%- if pointer %}\n> > > +\tif ({{param.mojom_name}}) {\n> > > +{{deserialize_param(param, pointer, loop, buf, fds)|indent(16, True)}}\n> > > +\t}\n> > > +\t{%- else %}\n> > > +\t{{param|name + \" \" if declare}}{{deserialize_param(param, pointer, loop, buf, fds)|indent(8)}}\n> > > +\t{%- endif %}\n> > > +{% endfor %}\n> > > +{%- endmacro -%}\n> > > diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> > > new file mode 100644\n> > > index 00000000..3c071c4e\n> > > --- /dev/null\n> > > +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl\n> > > @@ -0,0 +1,276 @@\n> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}\n> > > +{# Turn this into a C macro? #}\n> > > +{#\n> > > + # \\brief Verify that there is enough bytes to deserialize\n> > > + #\n> > > + # Generate code that verifies that \\a size is not greater than \\a dataSize.\n> > > + # Otherwise log an error with \\a name and \\a typename.\n> > > + #}\n> > > +{%- macro check_data_size(size, dataSize, name, typename) %}\n> > > +\t\tif ({{size}} > {{dataSize}}) {\n> > > +\t\t\tLOG(IPADataSerializer, Error)\n> > > +\t\t\t\t<< \"Failed to deserialize {{name}}: not enough {{typename}}, expected \"\n> > > +\t\t\t\t<< ({{size}}) << \", got \" << ({{dataSize}});\n> > > +\t\t\treturn ret;\n> > > +\t\t}\n> > > +{%- endmacro %}\n> > > +\n> > > +\n> > > +{#\n> > > + # \\brief Serialize some field into return vector\n> > > + #\n> > > + # Generate code to serialize \\a field into ret_data, including size of the\n> > > + # field and fds (where appropriate). \\a base_controls indicates the\n> > > + # default ControlInfoMap in the event that the ControlList does not have one.\n> > > + # This code is meant to be used by the IPADataSerializer specialization.\n> > > + #}\n> > > +{%- macro serializer_field(field, base_controls, namespace, loop) %}\n> > > +{%- if field|is_pod or field|is_enum %}\n> > > +\t\tstd::vector {{field.mojom_name}};\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_enum %}\n> > > +\t\t\tIPADataSerializer::serialize(data.{{field.mojom_name}}_);\n> > > +\t{%- endif %}\n> > > +\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> > > +{%- elif field|is_fd %}\n> > > +\t\tstd::vector {{field.mojom_name}};\n> > > +\t\tstd::vector {{field.mojom_name}}Fds;\n> > > +\t\tstd::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =\n> > > +\t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);\n> > > +\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> > > +\t\tret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());\n> > > +{%- elif field|is_controls %}\n> > > +\t\tif (data.{{field.mojom_name}}_.size() > 0) {\n> > > +\t\t\tstd::vector {{field.mojom_name}};\n> > > +\t\t\tstd::tie({{field.mojom_name}}, std::ignore) =\n> > > +\t\t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_,\n> > > +\t\t\t\t\tdata.{{field.mojom_name}}_.infoMap() ? *data.{{field.mojom_name}}_.infoMap() : {{base_controls}},\n> > > +\t\t\t\t\tcs);\n> > > +\t\t\tappendUInt(ret_data, {{field.mojom_name}}.size());\n> > > +\t\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> > > +\t\t} else {\n> > > +\t\t\tappendUInt(ret_data, 0);\n> > > +\t\t}\n> > > +{%- elif field|is_plain_struct or field|is_array or field|is_map %}\n> > > +\t\tstd::vector {{field.mojom_name}};\n> > > +\t{%- if field|has_fd %}\n> > > +\t\tstd::vector {{field.mojom_name}}Fds;\n> > > +\t\tstd::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =\n> > > +\t{%- else %}\n> > > +\t\tstd::tie({{field.mojom_name}}, std::ignore) =\n> > > +\t{%- endif %}\n> > > +\t{%- if field|is_array or field|is_map %}\n> > > +\t\t\tIPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_, cs);\n> > > +\t{%- else %}\n> > > +\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::serialize(data.{{field.mojom_name}}_, cs);\n> > > +\t{%- endif %}\n> > > +\t\tappendUInt(ret_data, {{field.mojom_name}}.size());\n> > > +\t{%- if field|has_fd %}\n> > > +\t\tappendUInt(ret_data, {{field.mojom_name}}Fds.size());\n> > > +\t{%- endif %}\n> > > +\t\tret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());\n> > > +\t{%- if field|has_fd %}\n> > > +\t\tret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());\n> > > +\t{%- endif %}\n> > > +{%- else %}\n> > > +\t\t/* Unknown serialization for {{field.mojom_name}}. */\n> > > +{%- endif %}\n> > > +{%- endmacro %}\n> > > +\n> > > +\n> > > +{#\n> > > + # \\brief Deserialize some field into return struct\n> > > + #\n> > > + # Generate code to deserialize \\a field into object ret.\n> > > + # This code is meant to be used by the IPADataSerializer specialization.\n> > > + #}\n> > > +{%- macro deserializer_field(field, namespace, loop) %}\n> > > +{% if field|is_pod or field|is_enum %}\n> > > +\t{%- set field_size = (field|bit_width|int / 8)|int %}\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}}_ = static_cast<{{field|name}}>(IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}));\n> > > +\t\t{%- else %}\n> > > +\t\tret.{{field.mojom_name}}_ = static_cast<{{field|name}}>(IPADataSerializer::deserialize(m, m + {{field_size}}));\n> > > +\t\t{%- endif %}\n> > > +\t{%- if not loop.last %}\n> > > +\t\tm += {{field_size}};\n> > > +\t\tdataSize -= {{field_size}};\n> > > +\t{%- endif %}\n> > > +{% elif field|is_fd %}\n> > > +\t{%- set field_size = 1 %}\n> > > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}\n> > > +\t\tret.{{field.mojom_name}}_ = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs);\n> > > +\t{%- if not loop.last %}\n> > > +\t\tm += {{field_size}};\n> > > +\t\tdataSize -= {{field_size}};\n> > > +\t\tn += ret.{{field.mojom_name}}_.isValid() ? 1 : 0;\n> > > +\t\tfdsSize -= ret.{{field.mojom_name}}_.isValid() ? 1 : 0;\n> > > +\t{%- endif %}\n> > > +{% elif field|is_controls %}\n> > > +\t{%- set field_size = 4 %}\n> > > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}\n> > > +\t\tsize_t {{field.mojom_name}}Size = readUInt(m);\n> > > +\t{%- set field_size = '4 + ' + field.mojom_name + 'Size' -%}\n> > > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}\n> > > +\t\tif ({{field.mojom_name}}Size > 0)\n> > > +\t\t\tret.{{field.mojom_name}}_ =\n> > > +\t\t\t\tIPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);\n> > > +\t{%- if not loop.last %}\n> > > +\t\tm += {{field_size}};\n> > > +\t\tdataSize -= {{field_size}};\n> > > +\t{%- endif %}\n> > > +{% elif field|is_plain_struct or field|is_array or field|is_map%}\n> > > +\t{%- set field_size = 4 %}\n> > > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}\n> > > +\t\tsize_t {{field.mojom_name}}Size = readUInt(m);\n> > > +\t{%- if field|has_fd %}\n> > > +\t{%- set field_size = 8 %}\n> > > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'FdsSize', 'data') }}\n> > > +\t\tsize_t {{field.mojom_name}}FdsSize = readUInt(m + 4);\n> > > +\t\t{{- check_data_size(field.mojom_name + 'FdsSize', 'fdsSize', field.mojom_name, 'fds') }}\n> > > +\t{%- endif %}\n> > > +\t{%- set field_size = field|has_fd|choose('8 + ', '4 + ') + field.mojom_name + 'Size' -%}\n> > > +\t\t{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}\n> > > +\t\tret.{{field.mojom_name}}_ =\n> > > +\t{%- if field|has_fd and (field|is_array or field|is_map) %}\n> > > +\t\t\tIPADataSerializer<{{field|name}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);\n> > > +\t{%- elif field|has_fd and (not (field|is_array or field|is_map)) %}\n> > > +\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);\n> > > +\t{%- elif (not field|has_fd) and (field|is_array or field|is_map) %}\n> > > +\t\t\tIPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);\n> > > +\t{%- else %}\n> > > +\t\t\tIPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);\n> > > +\t{%- endif %}\n> > > +\t{%- if not loop.last %}\n> > > +\t\tm += {{field_size}};\n> > > +\t\tdataSize -= {{field_size}};\n> > > +\t{%- if field|has_fd %}\n> > > +\t\tn += {{field.mojom_name}}FdsSize;\n> > > +\t\tfdsSize -= {{field.mojom_name}}FdsSize;\n> > > +\t{%- endif %}\n> > > +\t{%- endif %}\n> > > +{% else %}\n> > > +\t\t/* Unknown deserialization for {{field.mojom_name}}. */\n> > > +{%- endif %}\n> > > +{%- endmacro %}\n> > > +\n> > > +\n> > > +{#\n> > > + # \\brief Serialize a struct\n> > > + #\n> > > + # Generate code for IPADataSerializer specialization, for serializing\n> > > + # \\a struct. \\a base_controls indicates the default ControlInfoMap\n> > > + # in the event that the ControlList does not have one.\n> > > + #}\n> > > +{%- macro serializer(struct, base_controls, namespace) %}\n> > > +\tstatic std::tuple, std::vector>\n> > > +\tserialize(const {{ struct|name_full(namespace) }} &data,\n> > > +{%- if struct|needs_control_serializer %}\n> > > +\t\t ControlSerializer *cs)\n> > > +{%- else %}\n> > > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +{%- endif %}\n> > > +\t{\n> > > +\t\tstd::vector ret_data;\n> > > +{%- if struct|has_fd %}\n> > > +\t\tstd::vector ret_fds;\n> > > +{%- endif %}\n> > > +{%- for field in struct.fields %}\n> > > +{{serializer_field(field, base_controls, namespace, loop)}}\n> > > +{%- endfor %}\n> > > +{% if struct|has_fd %}\n> > > +\t\treturn {ret_data, ret_fds};\n> > > +{%- else %}\n> > > +\t\treturn {ret_data, {}};\n> > > +{%- endif %}\n> > > +\t}\n> > > +{%- endmacro %}\n> > > +\n> > > +\n> > > +{#\n> > > + # \\brief Deserialize a struct that has fds\n> > > + #\n> > > + # Generate code for IPADataSerializer specialization, for deserializing\n> > > + # \\a struct, in the case that \\a struct has file descriptors.\n> > > + #}\n> > > +{%- macro deserializer_fd(struct, namespace) %}\n> > > +\tstatic {{ struct|name_full(namespace) }}\n> > > +\tdeserialize(std::vector &data,\n> > > +\t\t std::vector &fds,\n> > > +{%- if struct|needs_control_serializer %}\n> > > +\t\t ControlSerializer *cs)\n> > > +{%- else %}\n> > > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +{%- endif %}\n> > > +\t{\n> > > +\t\t{{struct|name_full(namespace)}} ret;\n> > > +\t\tstd::vector::iterator m = data.begin();\n> > > +\t\tstd::vector::iterator n = fds.begin();\n> > > +\n> > > +\t\tsize_t dataSize = data.size();\n> > > +\t\tsize_t fdsSize = fds.size();\n> > > +{%- for field in struct.fields -%}\n> > > +{{deserializer_field(field, namespace, loop)}}\n> > > +{%- endfor %}\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tstatic {{ struct|name_full(namespace) }}\n> > > +\tdeserialize(std::vector::iterator data_it1,\n> > > +\t\t std::vector::iterator data_it2,\n> > > +\t\t std::vector::iterator fds_it1,\n> > > +\t\t std::vector::iterator fds_it2,\n> > > +{%- if struct|needs_control_serializer %}\n> > > +\t\t ControlSerializer *cs)\n> > > +{%- else %}\n> > > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +{%- endif %}\n> > > +\t{\n> > > +\t\tstd::vector data(data_it1, data_it2);\n> > > +\t\tstd::vector fds(fds_it1, fds_it2);\n> > > +\t\treturn IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, fds, cs);\n> > > +\t}\n> > > +{%- endmacro %}\n> > > +\n> > > +\n> > > +{#\n> > > + # \\brief Deserialize a struct that has no fds\n> > > + #\n> > > + # Generate code for IPADataSerializer specialization, for deserializing\n> > > + # \\a struct, in the case that \\a struct does not have file descriptors.\n> > > + #}\n> > > +{%- macro deserializer_no_fd(struct, namespace) %}\n> > > +\tstatic {{ struct|name_full(namespace) }}\n> > > +\tdeserialize(std::vector &data,\n> > > +{%- if struct|needs_control_serializer %}\n> > > +\t\t ControlSerializer *cs)\n> > > +{%- else %}\n> > > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +{%- endif %}\n> > > +\t{\n> > > +\t\t{{struct|name_full(namespace)}} ret;\n> > > +\t\tstd::vector::iterator m = data.begin();\n> > > +\n> > > +\t\tsize_t dataSize = data.size();\n> > > +{%- for field in struct.fields -%}\n> > > +{{deserializer_field(field, namespace, loop)}}\n> > > +{%- endfor %}\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tstatic {{ struct|name_full(namespace) }}\n> > > +\tdeserialize(std::vector::iterator data_it1,\n> > > +\t\t std::vector::iterator data_it2,\n> > > +{%- if struct|needs_control_serializer %}\n> > > +\t\t ControlSerializer *cs)\n> > > +{%- else %}\n> > > +\t\t [[maybe_unused]] ControlSerializer *cs = nullptr)\n> > > +{%- endif %}\n> > > +\t{\n> > > +\t\tstd::vector data(data_it1, data_it2);\n> > > +\t\treturn IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, cs);\n> > > +\t}\n> > > +{%- endmacro %}\n> > > diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py\n> > > new file mode 100644\n> > > index 00000000..a9269c82\n> > > --- /dev/null\n> > > +++ b/utils/ipc/generators/mojom_libcamera_generator.py\n> > > @@ -0,0 +1,461 @@\n> > > +'''Generates libcamera files from a mojom.Module.'''\n> > > +\n> > > +import argparse\n> > > +import ast\n> > > +import datetime\n> > > +import contextlib\n> > > +import os\n> > > +import re\n> > > +import shutil\n> > > +import sys\n> > > +import tempfile\n> > > +\n> > > +from jinja2 import contextfilter\n> > > +\n> > > +import mojom.fileutil as fileutil\n> > > +import mojom.generate.generator as generator\n> > > +import mojom.generate.module as mojom\n> > > +from mojom.generate.template_expander import UseJinja\n> > > +\n> > > +\n> > > +GENERATOR_PREFIX = 'libcamera'\n> > > +\n> > > +_kind_to_cpp_type = {\n> > > + mojom.BOOL: 'bool',\n> > > + mojom.INT8: 'int8_t',\n> > > + mojom.UINT8: 'uint8_t',\n> > > + mojom.INT16: 'int16_t',\n> > > + mojom.UINT16: 'uint16_t',\n> > > + mojom.INT32: 'int32_t',\n> > > + mojom.UINT32: 'uint32_t',\n> > > + mojom.FLOAT: 'float',\n> > > + mojom.INT64: 'int64_t',\n> > > + mojom.UINT64: 'uint64_t',\n> > > + mojom.DOUBLE: 'double',\n> > > +}\n> > > +\n> > > +_bit_widths = {\n> > > + mojom.BOOL: '8',\n> > > + mojom.INT8: '8',\n> > > + mojom.UINT8: '8',\n> > > + mojom.INT16: '16',\n> > > + mojom.UINT16: '16',\n> > > + mojom.INT32: '32',\n> > > + mojom.UINT32: '32',\n> > > + mojom.FLOAT: '32',\n> > > + mojom.INT64: '64',\n> > > + mojom.UINT64: '64',\n> > > + mojom.DOUBLE: '64',\n> > > +}\n> > > +\n> > > +def ModuleName(path):\n> > > + return path.split('/')[-1].split('.')[0]\n> > > +\n> > > +def ModuleClassName(module):\n> > > + s = re.sub(r'^IPA', '', module.interfaces[0].mojom_name)\n> > > + return re.sub(r'Interface$', '', s)\n> > > +\n> > > +def UpperCamelCase(name):\n> > > + return ''.join([x.capitalize() for x in generator.SplitCamelCase(name)])\n> > > +\n> > > +def CamelCase(name):\n> > > + uccc = UpperCamelCase(name)\n> > > + return uccc[0].lower() + uccc[1:]\n> > > +\n> > > +def ConstantStyle(name):\n> > > + return generator.ToUpperSnakeCase(name)\n> > > +\n> > > +def Choose(cond, t, f):\n> > > + return t if cond else f\n> > > +\n> > > +def CommaSep(l):\n> > > + return ', '.join([m for m in l])\n> > > +\n> > > +def ParamsCommaSep(l):\n> > > + return ', '.join([m.mojom_name for m in l])\n> > > +\n> > > +def GetDefaultValue(element):\n> > > + if element.default is not None:\n> > > + return element.default\n> > > + if type(element.kind) == mojom.Kind:\n> > > + return '0'\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> > > + element.kind.mojom_name == 'FileDescriptor'):\n> > > + return '-1'\n> > > + return ''\n> > > +\n> > > +def WithDefaultValues(element):\n> > > + return [x for x in element if HasDefaultValue(x)]\n> > > +\n> > > +def WithFds(element):\n> > > + return [x for x in element if HasFd(x)]\n> > > +\n> > > +def HasDefaultValue(element):\n> > > + return GetDefaultValue(element) != ''\n> > > +\n> > > +def HasDefaultFields(element):\n> > > + return True in [HasDefaultValue(x) for x in element.fields]\n> > > +\n> > > +def GetAllTypes(element):\n> > > + if mojom.IsArrayKind(element):\n> > > + return GetAllTypes(element.kind)\n> > > + if mojom.IsMapKind(element):\n> > > + return GetAllTypes(element.key_kind) + GetAllTypes(element.value_kind)\n> > > + if isinstance(element, mojom.Parameter):\n> > > + return GetAllTypes(element.kind)\n> > > + if mojom.IsEnumKind(element):\n> > > + return [element.mojom_name]\n> > > + if not mojom.IsStructKind(element):\n> > > + return [element.spec]\n> > > + if len(element.fields) == 0:\n> > > + return [element.mojom_name]\n> > > + ret = [GetAllTypes(x.kind) for x in element.fields]\n> > > + ret = [x for sublist in ret for x in sublist]\n> > > + return list(set(ret))\n> > > +\n> > > +def GetAllAttrs(element):\n> > > + if mojom.IsArrayKind(element):\n> > > + return GetAllAttrs(element.kind)\n> > > + if mojom.IsMapKind(element):\n> > > + return {**GetAllAttrs(element.key_kind), **GetAllAttrs(element.value_kind)}\n> > > + if isinstance(element, mojom.Parameter):\n> > > + return GetAllAttrs(element.kind)\n> > > + if mojom.IsEnumKind(element):\n> > > + return element.attributes if element.attributes is not None else {}\n> > > + if mojom.IsStructKind(element) and len(element.fields) == 0:\n> > > + return element.attributes if element.attributes is not None else {}\n> > > + if not mojom.IsStructKind(element):\n> > > + return {}\n> > > + attrs = [(x.attributes) for x in element.fields]\n> > > + ret = {}\n> > > + for d in attrs:\n> > > + if d is not None:\n> > > + ret = {**ret, **d}\n> > > + return ret\n> > > +\n> > > +def NeedsControlSerializer(element):\n> > > + types = GetAllTypes(element)\n> > > + return \"ControlList\" in types or \"ControlInfoMap\" in types\n> > > +\n> > > +def HasFd(element):\n> > > + attrs = GetAllAttrs(element)\n> > > + if isinstance(element, mojom.Kind):\n> > > + types = GetAllTypes(element)\n> > > + else:\n> > > + types = GetAllTypes(element.kind)\n> > > + return \"FileDescriptor\" in types or (attrs is not None and \"hasFd\" in attrs)\n> > > +\n> > > +def MethodInputHasFd(method):\n> > > + if len([x for x in method.parameters if HasFd(x)]) > 0:\n> > > + return True\n> > > + return False\n> > > +\n> > > +def MethodOutputHasFd(method):\n> > > + if (MethodReturnValue(method) != 'void' or\n> > > + method.response_parameters is None):\n> > > + return False\n> > > + if len([x for x in method.parameters if HasFd(x)]) > 0:\n> > > + return True\n> > > + return False\n> > > +\n> > > +def MethodParamInputs(method):\n> > > + return method.parameters\n> > > +\n> > > +def MethodParamOutputs(method):\n> > > + if (MethodReturnValue(method) != 'void' or\n> > > + method.response_parameters is None):\n> > > + return []\n> > > + return method.response_parameters\n> > > +\n> > > +def MethodParamNames(method):\n> > > + params = []\n> > > + for param in method.parameters:\n> > > + params.append(param.mojom_name)\n> > > + if MethodReturnValue(method) == 'void':\n> > > + if method.response_parameters is None:\n> > > + return params\n> > > + for param in method.response_parameters:\n> > > + params.append(param.mojom_name)\n> > > + return params\n> > > +\n> > > +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> > > + param.mojom_name))\n> > > + if MethodReturnValue(method) == 'void':\n> > > + if method.response_parameters is None:\n> > > + return params\n> > > + for param in method.response_parameters:\n> > > + params.append(f'{GetNameForElement(param)} *{param.mojom_name}')\n> > > + return params\n> > > +\n> > > +def MethodReturnValue(method):\n> > > + if method.response_parameters is None:\n> > > + return 'void'\n> > > + if len(method.response_parameters) == 1 and IsPod(method.response_parameters[0]):\n> > > + return GetNameForElement(method.response_parameters[0])\n> > > + return 'void'\n> > > +\n> > > +def IsAsync(method):\n> > > + # callbacks are always async\n> > > + if re.match(\"^IPA.*CallbackInterface$\", method.interface.mojom_name):\n> > > + return True\n> > > + elif re.match(\"^IPA.*Interface$\", method.interface.mojom_name):\n> > > + if method.attributes is None:\n> > > + return False\n> > > + elif 'async' in method.attributes and method.attributes['async']:\n> > > + return True\n> > > + return False\n> > > +\n> > > +def IsArray(element):\n> > > + return mojom.IsArrayKind(element.kind)\n> > > +\n> > > +def IsControls(element):\n> > > + return mojom.IsStructKind(element.kind) and (element.kind.mojom_name == \"ControlList\" or\n> > > + element.kind.mojom_name == \"ControlInfoMap\")\n> > > +\n> > > +def IsEnum(element):\n> > > + return mojom.IsEnumKind(element.kind)\n> > > +\n> > > +def IsFd(element):\n> > > + return mojom.IsStructKind(element.kind) and element.kind.mojom_name == \"FileDescriptor\"\n> > > +\n> > > +def IsMap(element):\n> > > + return mojom.IsMapKind(element.kind)\n> > > +\n> > > +def IsPlainStruct(element):\n> > > + return mojom.IsStructKind(element.kind) and not IsControls(element) and not IsFd(element)\n> > > +\n> > > +def IsPod(element):\n> > > + return element.kind in _kind_to_cpp_type\n> > > +\n> > > +def BitWidth(element):\n> > > + if element.kind in _bit_widths:\n> > > + return _bit_widths[element.kind]\n> > > + if mojom.IsEnumKind(element.kind):\n> > > + return '32'\n> > > + return ''\n> > > +\n> > > +def GetNameForElement(element):\n> > > + if (mojom.IsEnumKind(element) or\n> > > + mojom.IsInterfaceKind(element) or\n> > > + mojom.IsStructKind(element)):\n> > > + return element.mojom_name\n> > > + if (mojom.IsArrayKind(element)):\n> > > + elem_name = GetNameForElement(element.kind)\n> > > + return f'std::vector<{elem_name}>'\n> > > + if (mojom.IsMapKind(element)):\n> > > + key_name = GetNameForElement(element.key_kind)\n> > > + value_name = GetNameForElement(element.value_kind)\n> > > + return f'std::map<{key_name}, {value_name}>'\n> > > + if isinstance(element, (mojom.Field, mojom.Method, mojom.Parameter)):\n> > > + if (mojom.IsArrayKind(element.kind) or mojom.IsMapKind(element.kind)):\n> > > + return GetNameForElement(element.kind)\n> > > + if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):\n> > > + return _kind_to_cpp_type[element.kind]\n> > > + return element.kind.mojom_name\n> > > + if isinstance(element, mojom.EnumValue):\n> > > + return (GetNameForElement(element.enum) + '.' +\n> > > + ConstantStyle(element.name))\n> > > + if isinstance(element, (mojom.NamedValue,\n> > > + mojom.Constant,\n> > > + mojom.EnumField)):\n> > > + return ConstantStyle(element.name)\n> > > + if (hasattr(element, '__hash__') and element in _kind_to_cpp_type):\n> > > + return _kind_to_cpp_type[element]\n> > > + if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):\n> > > + return _kind_to_cpp_type[element.kind]\n> > > + if (hasattr(element, 'spec')):\n> > > + return element.spec.split(':')[-1]\n> > > + if (mojom.IsInterfaceRequestKind(element) or\n> > > + mojom.IsAssociatedKind(element) or\n> > > + mojom.IsPendingRemoteKind(element) or\n> > > + mojom.IsPendingReceiverKind(element) or\n> > > + mojom.IsUnionKind(element)):\n> > > + raise Exception('Unsupported element: %s' % element)\n> > > + raise Exception('Unexpected element: %s' % element)\n> > > +\n> > > +def GetFullNameForElement(element, namespace_str):\n> > > + name = GetNameForElement(element)\n> > > + if namespace_str == '':\n> > > + return name\n> > > + return f'{namespace_str}::{name}'\n> > > +\n> > > +def ValidateZeroLength(l, s, cap=True):\n> > > + if len(l) > 0:\n> > > + raise Exception(f'{s.capitalize() if cap else s} should be empty')\n> > > +\n> > > +def ValidateSingleLength(l, s, cap=True):\n> > > + if len(l) > 1:\n> > > + raise Exception(f'Only one {s} allowed')\n> > > + if len(l) < 1:\n> > > + raise Exception(f'{s.capitalize() if cap else s} is required')\n> > > +\n> > > +def ValidateInterfaces(interfaces):\n> > > + # Validate presence of main interface\n> > > + intf = [x for x in interfaces\n> > > + if re.match(\"^IPA.*Interface\", x.mojom_name) and\n> > > + not re.match(\"^IPA.*CallbackInterface\", x.mojom_name)]\n> > > + ValidateSingleLength(intf, 'main interface')\n> > > +\n> > > + # Validate presence of callback interface\n> > > + cb = [x for x in interfaces if re.match(\"^IPA.*CallbackInterface\", x.mojom_name)]\n> > > + ValidateSingleLength(cb, 'callback interface')\n> > > +\n> > > + # Validate required main interface functions\n> > > + intf = intf[0]\n> > > + f_init = [x for x in intf.methods if x.mojom_name == 'init']\n> > > + f_start = [x for x in intf.methods if x.mojom_name == 'start']\n> > > + f_stop = [x for x in intf.methods if x.mojom_name == 'stop']\n> > > +\n> > > + ValidateSingleLength(f_init, 'init()', False)\n> > > + ValidateSingleLength(f_start, 'start()', False)\n> > > + ValidateSingleLength(f_stop, 'stop()', False)\n> > > +\n> > > + f_init = f_init[0]\n> > > + f_start = f_start[0]\n> > > + f_stop = f_stop[0]\n> > > +\n> > > + # Validate parameters to init()\n> > > + ValidateSingleLength(f_init.parameters, 'input parameter to init()')\n> > > + ValidateSingleLength(f_init.response_parameters, 'output parameter from init()')\n> > > + if f_init.parameters[0].kind.mojom_name != 'IPASettings':\n> > > + raise Exception('init() must have single IPASettings input parameter')\n> > > + if f_init.response_parameters[0].kind.spec != 'i32':\n> > > + raise Exception('init() must have single int32 output parameter')\n> > > +\n> > > + # Validate parameters to start()\n> > > + ValidateZeroLength(f_start.parameters, 'input parameter to start()')\n> > > + ValidateSingleLength(f_start.response_parameters, 'output parameter from start()')\n> > > + if f_start.response_parameters[0].kind.spec != 'i32':\n> > > + raise Exception('start() must have single int32 output parameter')\n> > > +\n> > > + # Validate parameters to stop()\n> > > + ValidateZeroLength(f_stop.parameters, 'input parameter to stop()')\n> > > + ValidateZeroLength(f_stop.parameters, 'output parameter from stop()')\n> > > +\n> > > +class Generator(generator.Generator):\n> > > +\n> > > + @staticmethod\n> > > + def GetTemplatePrefix():\n> > > + return 'libcamera_templates'\n> > > +\n> > > + def GetFilters(self):\n> > > + libcamera_filters = {\n> > > + 'all_types': GetAllTypes,\n> > > + 'bit_width': BitWidth,\n> > > + 'choose': Choose,\n> > > + 'comma_sep': CommaSep,\n> > > + 'default_value': GetDefaultValue,\n> > > + 'has_default_fields': HasDefaultFields,\n> > > + 'has_fd': HasFd,\n> > > + 'is_async': IsAsync,\n> > > + 'is_array': IsArray,\n> > > + 'is_controls': IsControls,\n> > > + 'is_enum': IsEnum,\n> > > + 'is_fd': IsFd,\n> > > + 'is_map': IsMap,\n> > > + 'is_plain_struct': IsPlainStruct,\n> > > + 'is_pod': IsPod,\n> > > + 'method_input_has_fd': MethodInputHasFd,\n> > > + 'method_output_has_fd': MethodOutputHasFd,\n> > > + 'method_param_names': MethodParamNames,\n> > > + 'method_param_inputs': MethodParamInputs,\n> > > + 'method_param_outputs': MethodParamOutputs,\n> > > + 'method_parameters': MethodParameters,\n> > > + 'method_return_value': MethodReturnValue,\n> > > + 'name': GetNameForElement,\n> > > + 'name_full': GetFullNameForElement,\n> > > + 'needs_control_serializer': NeedsControlSerializer,\n> > > + 'params_comma_sep': ParamsCommaSep,\n> > > + 'with_default_values': WithDefaultValues,\n> > > + 'with_fds': WithFds,\n> > > + }\n> > > + return libcamera_filters\n> > > +\n> > > + def _GetJinjaExports(self):\n> > > + return {\n> > > + 'base_controls': '%s::Controls' % ModuleClassName(self.module),\n> > > + 'cmd_enum_name': '_%sCMD' % ModuleClassName(self.module),\n> > \n> > s/CMD/Command/ ?\n> > \n> > > + 'enums': self.module.enums,\n> > > + 'has_array': len([x for x in self.module.kinds.keys() if x[0] == 'a']) > 0,\n> > > + 'has_map': len([x for x in self.module.kinds.keys() if x[0] == 'm']) > 0,\n> > > + 'has_namespace': self.module.mojom_namespace != '',\n> > > + 'imports': self.module.imports,\n> > > + 'interface_cb': self.module.interfaces[1],\n> > > + 'interface_main': self.module.interfaces[0],\n> > > + 'interface_name': 'IPA%sInterface' % ModuleClassName(self.module),\n> > > + 'ipc_name': 'IPAIPCUnixSocket',\n> > > + 'kinds': self.module.kinds,\n> > > + 'module': self.module,\n> > > + 'module_name': ModuleName(self.module.path),\n> > > + 'module_class_name': ModuleClassName(self.module),\n> > > + 'namespace': self.module.mojom_namespace.split('.'),\n> > > + 'namespace_str': self.module.mojom_namespace.replace('.', '::') if\n> > > + self.module.mojom_namespace is not None else '',\n> > > + 'proxy_name': 'IPAProxy%s' % ModuleClassName(self.module),\n> > > + 'proxy_worker_name': 'IPAProxy%sWorker' % ModuleClassName(self.module),\n> > > + 'structs_nonempty': [x for x in self.module.structs if len(x.fields) > 0],\n> > > + 'year': datetime.datetime.now().year,\n> > > + }\n> > > +\n> > > +\n> > > + @UseJinja('module_generated.h.tmpl')\n> > > + def _GenerateDataHeader(self):\n> > > + return self._GetJinjaExports()\n> > > +\n> > > + @UseJinja('module_serializer.h.tmpl')\n> > > + def _GenerateSerializer(self):\n> > > + return self._GetJinjaExports()\n> > > +\n> > > + @UseJinja('module_ipa_proxy.cpp.tmpl')\n> > > + def _GenerateProxyCpp(self):\n> > > + return self._GetJinjaExports()\n> > > +\n> > > + @UseJinja('module_ipa_proxy.h.tmpl')\n> > > + def _GenerateProxyHeader(self):\n> > > + return self._GetJinjaExports()\n> > > +\n> > > + @UseJinja('module_ipa_proxy_worker.cpp.tmpl')\n> > > + def _GenerateProxyWorker(self):\n> > > + return self._GetJinjaExports()\n> > > +\n> > > + def GenerateFiles(self, unparsed_args):\n> > > + parser = argparse.ArgumentParser()\n> > > + parser.add_argument('--libcamera_generate_header', action='store_true')\n> > > + parser.add_argument('--libcamera_generate_serializer', action='store_true')\n> > > + parser.add_argument('--libcamera_generate_proxy_cpp', action='store_true')\n> > > + parser.add_argument('--libcamera_generate_proxy_h', action='store_true')\n> > > + parser.add_argument('--libcamera_generate_proxy_worker', action='store_true')\n> > > + parser.add_argument('--libcamera_output_path')\n> > > + args = parser.parse_args(unparsed_args)\n> > > +\n> > > + ValidateInterfaces(self.module.interfaces)\n> > > +\n> > > + fileutil.EnsureDirectoryExists(os.path.dirname(args.libcamera_output_path))\n> > > +\n> > > + module_name = ModuleName(self.module.path)\n> > > +\n> > > + if args.libcamera_generate_header:\n> > > + self.Write(self._GenerateDataHeader(),\n> > > + args.libcamera_output_path)\n> > > +\n> > > + if args.libcamera_generate_serializer:\n> > > + self.Write(self._GenerateSerializer(),\n> > > + args.libcamera_output_path)\n> > > +\n> > > + if args.libcamera_generate_proxy_cpp:\n> > > + self.Write(self._GenerateProxyCpp(),\n> > > + args.libcamera_output_path)\n> > > +\n> > > + if args.libcamera_generate_proxy_h:\n> > > + self.Write(self._GenerateProxyHeader(),\n> > > + args.libcamera_output_path)\n> > > +\n> > > + if args.libcamera_generate_proxy_worker:\n> > > + self.Write(self._GenerateProxyWorker(),\n> > > + args.libcamera_output_path)","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 753DFC3B5D\n\tfor ;\n\tTue, 6 Oct 2020 00:26:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC1A763C13;\n\tTue, 6 Oct 2020 02:26:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A57EF63B29\n\tfor ;\n\tTue, 6 Oct 2020 02:26:51 +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 E84F13B;\n\tTue, 6 Oct 2020 02:26:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sOPOXPvL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601944011;\n\tbh=oNY23EDnzh3jTMfQqRG3Pj6Mbxwi7ox3NHvXUcM3KvQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sOPOXPvLAByVrfQwcoSYaQ+Ipn9PXza1haXlvCk+HMkOHcwOWnVN9ZG76xztCsyyE\n\tV1ATwHRCpl8PWBSetZ6iCnP+UwyH5v5xzpLlscFl+19fwDrUuHhJ/HkwsskKzxDaDa\n\tn2JU9av+mJ+ig3AqgPlOCu0Fz5wkRR+7HWqGfl+I=","Date":"Tue, 6 Oct 2020 03:26:11 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"<20201006002611.GE11021@pendragon.ideasonboard.com>","References":"<20201002143154.468162-1-paul.elder@ideasonboard.com>\n\t<20201002143154.468162-7-paul.elder@ideasonboard.com>\n\t<20201004155640.GD8774@pendragon.ideasonboard.com>\n\t<20201005100857.GH45948@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201005100857.GH45948@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v3 06/38] utils: ipc: add templates\n\tfor code generation for IPC mechanism","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]