[libcamera-devel,v3,06/38] utils: ipc: add templates for code generation for IPC mechanism

9914 diff mbox series

Add templates to mojo to generate code for the IPC mechanism. These
templates generate:
- module header
- module serializer
- IPA proxy cpp, header, and worker

Given an input data definition mojom file for a pipeline.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v3:
- add support for namespaces
- fix enum assignment (used to have +1 for CMD applied to all enums)
- use readHeader, writeHeader, and eraseHeader as static class functions
  of IPAIPCUnixSocket (in the proxy worker)
- add requirement that base controls *must* be defined in
  libcamera::{pipeline_name}::Controls

Changes in v2:
- mandate the main and callback interfaces, and init(), start(), stop()
  and their parameters
- fix returning single pod value from IPC-called function
- add licenses
- improve auto-generated message
- other fixes related to serdes
---
 .../module_generated.h.tmpl                   | 106 ++++
 .../module_ipa_proxy.cpp.tmpl                 | 233 +++++++++
 .../module_ipa_proxy.h.tmpl                   | 117 +++++
 .../module_ipa_proxy_worker.cpp.tmpl          | 184 +++++++
 .../module_serializer.h.tmpl                  |  44 ++
 .../libcamera_templates/proxy_functions.tmpl  | 185 +++++++
 .../libcamera_templates/serializer.tmpl       | 276 +++++++++++
 .../generators/mojom_libcamera_generator.py   | 461 ++++++++++++++++++
 8 files changed, 1606 insertions(+)
 create mode 100644 utils/ipc/generators/libcamera_templates/module_generated.h.tmpl
 create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
 create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
 create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
 create mode 100644 utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl
 create mode 100644 utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
 create mode 100644 utils/ipc/generators/libcamera_templates/serializer.tmpl
 create mode 100644 utils/ipc/generators/mojom_libcamera_generator.py

Hi Paul,

Thank you for the patch.

On Fri, Oct 02, 2020 at 11:31:22PM +0900, Paul Elder wrote:
> Add templates to mojo to generate code for the IPC mechanism. These
> templates generate:
> - module header
> - module serializer
> - IPA proxy cpp, header, and worker
> 
> Given an input data definition mojom file for a pipeline.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v3:
> - add support for namespaces
> - fix enum assignment (used to have +1 for CMD applied to all enums)
> - use readHeader, writeHeader, and eraseHeader as static class functions
>   of IPAIPCUnixSocket (in the proxy worker)
> - add requirement that base controls *must* be defined in
>   libcamera::{pipeline_name}::Controls
> 
> Changes in v2:
> - mandate the main and callback interfaces, and init(), start(), stop()
>   and their parameters
> - fix returning single pod value from IPC-called function
> - add licenses
> - improve auto-generated message
> - other fixes related to serdes
> ---
>  .../module_generated.h.tmpl                   | 106 ++++

This isn't a great name, as _generated tells how the header is produced,
but not what it actually is. How about module_ipa_interface.h.tmpl ?

>  .../module_ipa_proxy.cpp.tmpl                 | 233 +++++++++
>  .../module_ipa_proxy.h.tmpl                   | 117 +++++
>  .../module_ipa_proxy_worker.cpp.tmpl          | 184 +++++++
>  .../module_serializer.h.tmpl                  |  44 ++

And maybe module_ipa_serializer.h.tmpl for consistency ?

>  .../libcamera_templates/proxy_functions.tmpl  | 185 +++++++
>  .../libcamera_templates/serializer.tmpl       | 276 +++++++++++
>  .../generators/mojom_libcamera_generator.py   | 461 ++++++++++++++++++
>  8 files changed, 1606 insertions(+)
>  create mode 100644 utils/ipc/generators/libcamera_templates/module_generated.h.tmpl
>  create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
>  create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
>  create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
>  create mode 100644 utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl
>  create mode 100644 utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
>  create mode 100644 utils/ipc/generators/libcamera_templates/serializer.tmpl
>  create mode 100644 utils/ipc/generators/mojom_libcamera_generator.py
> 
> diff --git a/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl
> new file mode 100644
> index 00000000..7dd48b0b
> --- /dev/null
> +++ b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl
> @@ -0,0 +1,106 @@
> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) {{ year }}, Google Inc.
> + *
> + * {{ module_name }}_generated.h - Image Processing Algorithm interface for {{ module_name }}
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#ifndef __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__
> +#define __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__
> +
> +#include <libcamera/ipa/ipa_interface.h>
> +

I think you can drop this blank line.

> +#include <libcamera/ipa/{{ module_name }}.h>
> +
> +{% if has_map %}#include <map>{% endif %}
> +{% if has_array %}#include <vector>{% endif %}
> +
> +namespace libcamera {
> +{%- if has_namespace %}
> +{% for ns in namespace %}
> +namespace {{ns}} {

There's a mix of {{ var }} and {{var}}. I don't mind much which one is
used, but I think we should be consistent.

> +{% endfor %}
> +{%- endif %}
> +
> +enum {{ cmd_enum_name }} {

It would be nice to make this an enum class, to avoid namespace clashes.
Alternatively, we could enforce the usage of namespaces for all IPAs.

enum class also brings the added benefit of catching improper usages,
but comes at a cost of requiring additional static_cast<> to convert
from and to uint32_t.

> +	CMD_EXIT = 0,

I'd drop the CMD_ prefix, especially if we use enum class, and use
CamelCase for names instead of uppercase.

> +{%- for method in interface_main.methods %}
> +	CMD_{{ method.mojom_name|upper }} = {{loop.index}},
> +{%- endfor %}
> +{%- set count = interface_main.methods|length -%}
> +{%- for method in interface_cb.methods %}
> +	CMD_{{ method.mojom_name|upper }} = {{loop.index + count}},
> +{%- endfor %}

Should we split the enum in two, one for each interface ?

> +};
> +
> +{% for enum in enums %}
> +enum {{ enum.mojom_name }} {
> +{%- for field in enum.fields %}
> +	{{ field.mojom_name }} = {{ field.numeric_value }},
> +{%- endfor %}
> +};
> +{% endfor %}
> +
> +{%- for struct in structs_nonempty %}
> +struct {{ struct.mojom_name }}
> +{
> +public:
> +	{{ struct.mojom_name }}() {%- if struct|has_default_fields %} :{% endif %}
> +{%- for field in struct.fields|with_default_values -%}
> +{{" " if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ ", " if not loop.last }}
> +{%- endfor %} {}

This generates really long lines. How about

	{{ struct.mojom_name }}() {%- if struct|has_default_fields %}
		:{% endif %}
{%- for field in struct.fields|with_default_values -%}
{{" " if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ ", " if not loop.last }}
{%- endfor %}
	{
	}

Would be great to wrap the initializers line too, but I think that's
overkill.

> +	~{{ struct.mojom_name }}() {}
> +
> +	{{ struct.mojom_name }}(
> +{%- for field in struct.fields -%}
> +{{ field|name }} {{ field.mojom_name }}{{ ", " if not loop.last }}
> +{%- endfor -%}
> +) :
> +{%- for field in struct.fields -%}
> +{{" " if loop.first}}{{ field.mojom_name}}_({{ field.mojom_name }}){{ ", " if not loop.last }}
> +{%- endfor %} {}

Same here.

> +{% for field in struct.fields %}
> +	{{ field|name }} {{ field.mojom_name }}_;
> +{%- endfor %}
> +};
> +{% endfor %}
> +
> +{#-
> +What to do about the #includes? Should we forward-declare, or
> +require {{module_name}}.h to include the required headers?
> +For now I'm going for the latter, coz this header essentially
> +extends {{module_name}}.h.
> +#}

Looking at the existing {{module_name}}.h, there are a few enums and
constants that could be moved to {{module_name}}_generated.h, such as
enum BufferMark or MaxLsGridSize in raspberrypi.h. What do you think ?

Then we have libcamera::RPi::Controls, which I'm wondering how to handle
best. It's not strictly related to this series, but wouldn't they be
better stored in the pipeline handler, and passed to the IPA ?

Finally there's the odd beast such as

#define VIMC_IPA_FIFO_PATH "/tmp/libcamera_ipa_vimc_fifo"

which I also wonder if it would make sense to pass it to the IPA at init
time. {{module_name}}.h could then possibly be dropped.

> +class {{ interface_name }} : public IPAInterface
> +{
> +public:
> +	virtual ~{{interface_name}}() {};

No need for the trailing ;

> +{% for method in interface_main.methods %}
> +	virtual {{method|method_return_value}} {{method.mojom_name}}(
> +{%- for param in method|method_parameters %}
> +		{{param}}{{- "," if not loop.last}}
> +{%- endfor -%}
> +) = 0;
> +{% endfor %}
> +
> +{%- for method in interface_cb.methods %}
> +	Signal<
> +{%- for param in method.parameters -%}
> +		const {{param|name}}{{" &" if not param|is_pod}}

Should the const be dropped if param|is_pod ? Same below.

> +		{{- ", " if not loop.last }}
> +{%- endfor -%}
> +> {{method.mojom_name}};
> +{% endfor -%}
> +};
> +
> +{%- if has_namespace %}
> +{% for ns in namespace|reverse %}
> +} /* {{ns}} */
> +{% endfor %}
> +{%- endif %}
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__ */
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> new file mode 100644
> index 00000000..f3eeeaaa
> --- /dev/null
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -0,0 +1,233 @@
> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> +
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) {{ year }}, Google Inc.
> + *
> + * ipa_proxy_{{ module_name }}.cpp - Image Processing Algorithm proxy for {{ module_name }}
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#include <vector>
> +
> +#include <libcamera/ipa/ipa_interface.h>

I think you can drop this, as it's included by {{module_name}}.h. Same
below.

> +#include <libcamera/ipa/ipa_module_info.h>
> +#include <libcamera/ipa/{{module_name}}.h>
> +#include <libcamera/ipa/{{module_name}}_generated.h>
> +#include <libcamera/ipa/{{module_name}}_serializer.h>
> +
> +#include "libcamera/internal/control_serializer.h"
> +#include "libcamera/internal/ipa_ipc.h"
> +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> +#include "libcamera/internal/ipa_data_serializer.h"
> +#include "libcamera/internal/ipa_module.h"
> +#include "libcamera/internal/ipa_proxy.h"
> +#include "libcamera/internal/ipc_unixsocket.h"
> +#include "libcamera/internal/log.h"
> +#include "libcamera/internal/process.h"
> +#include "libcamera/internal/thread.h"
> +
> +#include <libcamera/ipa/ipa_proxy_{{module_name}}.h>

Should this be moved above with the other libcamera/ipa/ headers ?

> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPAProxy)
> +
> +{%- if has_namespace %}
> +{% for ns in namespace %}
> +namespace {{ns}} {
> +{% endfor %}
> +{%- endif %}
> +
> +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)
> +	: IPAProxy(ipam), running_(false),
> +	  isolate_(isolate)
> +{
> +	LOG(IPAProxy, Debug)
> +		<< "initializing dummy proxy: loading IPA from "

It's not a dummy proxy anymore :-)

> +		<< ipam->path();
> +
> +	if (isolate_) {
> +		const std::string proxy_worker_path = resolvePath("ipa_proxy_{{module_name}}");
> +		if (proxy_worker_path.empty()) {
> +			LOG(IPAProxy, Error)
> +				<< "Failed to get proxy worker path";
> +			return;
> +		}
> +
> +		ipc_ = std::make_unique<IPAIPCUnixSocket>(ipam->path().c_str(), proxy_worker_path.c_str());
> +		if (!ipc_->isValid()) {
> +			LOG(IPAProxy, Error) << "Failed to create IPAIPC";
> +			return;
> +		}
> +
> +{% if interface_cb.methods|length > 0 %}
> +		ipc_->recvIPC.connect(this, &{{proxy_name}}::recvIPC);
> +{% endif %}
> +
> +		valid_ = true;
> +		return;
> +	}

There's very little code shared between the isolated and non-isolated
paths. It could make sense to split this class in two. On the other
hand, that would require the ability for the IPAManager to create the
right class type, which may complicate things a bit, but maybe not much.
For instance we could have the two classes called {{proxy_name}}Thread
and {{proxy_name}}IPC, and have a {{proxy_name}} class defined as

class {{proxy_name}}
{
public:
	using IPC = {{proxy_name}}IPC;
	using Thread = {{proxy_name}}Thread;
};

Then, in IPAManager::createIPA(), you would have

	std::unique_ptr<P> proxy;
	if (self_->isSignatureValid(m)))
		proxy = std::make_unique<P::Thread>(m);
	else
		proxy = std::make_unique<P::IPC>(m);

> +
> +	if (!ipam->load())
> +		return;
> +
> +	IPAInterface *ipai = ipam->createInterface();
> +	if (!ipai) {
> +		LOG(IPAProxy, Error)
> +			<< "Failed to create IPA context for " << ipam->path();
> +		return;
> +	}
> +
> +	ipa_ = std::unique_ptr<{{interface_name}}>(dynamic_cast<{{interface_name}} *>(ipai));
> +	proxy_.setIPA(ipa_.get());
> +
> +{% for method in interface_cb.methods %}
> +	ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);
> +{%- endfor %}
> +
> +	valid_ = true;
> +}
> +
> +{{proxy_name}}::~{{proxy_name}}()
> +{
> +	if (isolate_)
> +		ipc_->sendAsync(CMD_EXIT, {}, {});
> +}
> +
> +{% if interface_cb.methods|length > 0 %}
> +void {{proxy_name}}::recvIPC(std::vector<uint8_t> &data, std::vector<int32_t> &fds)
> +{
> +	uint32_t cmd = (data[0] & 0xff) |
> +		       ((data[1] & 0xff) << 8) |
> +		       ((data[2] & 0xff) << 16) |
> +		       ((data[3] & 0xff) << 24);

As data is a vector of uint8_t, I think the & 0xff is unnecessary.

Should the data length be validated ?

> +
> +	/* Need to skip another 4 bytes for the sequence number. */
> +	std::vector<uint8_t> vec(data.begin() + 8, data.end());

This creates a copy, which isn't very nice :-/ I think the IPC class
interface would benefit from switching from std::vector to Span.

> +	switch (cmd) {
> +{%- for method in interface_cb.methods %}
> +	case CMD_{{method.mojom_name|upper}}: {
> +		{{method.mojom_name}}IPC(vec, fds);
> +		break;
> +	}
> +{%- endfor %}

Should we log unknown commands ?

> +	}
> +}
> +{%- endif %}
> +
> +{% for method in interface_main.methods %}
> +{{proxy_funcs.func_sig(proxy_name, method)}}
> +{
> +	if (isolate_)
> +		{{"return " if method|method_return_value != "void"}}{{method.mojom_name}}IPC(
> +{%- for param in method|method_param_names -%}
> +		{{param}}{{- ", " if not loop.last}}
> +{%- endfor -%}
> +);
> +	else
> +		{{"return " if method|method_return_value != "void"}}{{method.mojom_name}}Thread(
> +{%- for param in method|method_param_names -%}
> +		{{param}}{{- ", " if not loop.last}}
> +{%- endfor -%}
> +);
> +}
> +
> +{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> +{
> +{%- if method.mojom_name == "init" %}
> +	{{proxy_funcs.init_thread_body()}}
> +{% elif method.mojom_name == "start" %}

I think you're missing a - before elif, here and below.

> +	{{proxy_funcs.start_thread_body()}}
> +{% elif method.mojom_name == "stop" %}
> +	{{proxy_funcs.stop_thread_body()}}

Instead of using macros, would it make sense to move the code to the
base IPAProxy class ?

> +{% elif not method|is_async %}
> +	ipa_->{{method.mojom_name}}(
> +	{%- for param in method|method_param_names -%}
> +		{{param}}{{- ", " if not loop.last}}
> +	{%- endfor -%}
> +);
> +{% elif method|is_async %}
> +	proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,
> +	{%- for param in method|method_param_names -%}
> +		{{param}}{{- ", " if not loop.last}}
> +	{%- endfor -%}
> +);
> +{%- endif %}
> +}
> +
> +{{proxy_funcs.func_sig(proxy_name, method, "IPC")}}
> +{
> +{%- set has_input = true if method|method_param_inputs|length > 0 %}
> +{%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != "void" %}
> +{% if has_input %}

Shouldn't it be {%- ?

> +	std::vector<uint8_t> _ipcInputBuf;
> +{%- if method|method_input_has_fd %}
> +	std::vector<int32_t> _ipcInputFds;
> +{%- endif %}
> +{%- endif %}
> +{%- if has_output %}
> +	std::vector<uint8_t> _ipcOutputBuf;
> +{%- if method|method_output_has_fd %}
> +	std::vector<int32_t> _ipcOutputFds;
> +{%- endif %}
> +{%- endif %}
> +
> +{{proxy_funcs.serialize_call(method|method_param_inputs, '_ipcInputBuf', '_ipcInputFds', base_controls)}}
> +
> +{%- set input_buf = "_ipcInputBuf" if has_input else "{}" %}
> +{%- set fds_buf = "_ipcInputFds" if method|method_input_has_fd else "{}" %}
> +{%- set cmd = "CMD_" + method.mojom_name|upper %}
> +{% if method|is_async %}
> +	int ret = ipc_->sendAsync({{cmd}}, {{input_buf}}, {{fds_buf}});
> +{%- else %}
> +	int ret = ipc_->sendSync({{cmd}}, {{input_buf}}, {{fds_buf}}
> +{{- ", &_ipcOutputBuf" if has_output -}}
> +{{- ", &_ipcOutputFds" if has_output and method|method_output_has_fd -}}
> +);
> +{%- endif %}
> +	if (ret < 0) {
> +		LOG(IPAProxy, Error) << "Failed to call {{method.mojom_name}}";
> +{%- if method|method_return_value != "void" %}
> +		return static_cast<{{method|method_return_value}}>(ret);
> +{%- else %}
> +		return;
> +{%- endif %}
> +	}
> +	LOG(IPAProxy, Debug) << "Done calling {{method.mojom_name}}";

Is this message helpful for debugging ? I think we should work at some
point on a tracing mechanism instead of manually tracing calls.

> +{% if method|method_return_value != "void" %}
> +	return IPADataSerializer<{{method.response_parameters|first|name}}>::deserialize(_ipcOutputBuf, 0);
> +{% elif method|method_param_outputs|length > 0 %}
> +{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf', '_ipcOutputFds')}}
> +{% endif -%}
> +}
> +
> +{% endfor %}
> +
> +

One blank line should be enough.

> +{% for method in interface_cb.methods %}
> +{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> +{
> +	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> +}
> +
> +void {{proxy_name}}::{{method.mojom_name}}IPC(
> +	std::vector<uint8_t> &data,
> +	[[maybe_unused]] std::vector<int32_t> &fds)
> +{ 
> +{%- for param in method.parameters %}
> +	{{param|name}} {{param.mojom_name}};
> +{%- endfor %}
> +{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false)}}
> +	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> +}
> +{% endfor %}
> +
> +{%- if has_namespace %}
> +{% for ns in namespace|reverse %}
> +} /* {{ns}} */
> +{% endfor %}
> +{%- endif %}
> +} /* namespace libcamera */
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> new file mode 100644
> index 00000000..7d2bfb6c
> --- /dev/null
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> @@ -0,0 +1,117 @@
> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> +
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) {{ year }}, Google Inc.
> + *
> + * ipa_proxy_{{ module_name }}.h - Image Processing Algorithm proxy for {{ module_name }}
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#ifndef __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__
> +#define __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__
> +
> +#include <libcamera/ipa/ipa_interface.h>
> +#include <libcamera/ipa/{{module_name}}.h>
> +#include <libcamera/ipa/{{module_name}}_generated.h>
> +
> +#include "libcamera/internal/control_serializer.h"
> +#include "libcamera/internal/ipa_ipc.h"
> +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> +#include "libcamera/internal/ipa_proxy.h"
> +#include "libcamera/internal/ipc_unixsocket.h"
> +#include "libcamera/internal/thread.h"
> +
> +namespace libcamera {
> +{%- if has_namespace %}
> +{% for ns in namespace %}
> +namespace {{ns}} {
> +{% endfor %}
> +{%- endif %}
> +
> +class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
> +{
> +public:
> +	{{proxy_name}}(IPAModule *ipam, bool isolate);
> +	~{{proxy_name}}();
> +
> +{% for method in interface_main.methods %}
> +{{proxy_funcs.func_sig(proxy_name, method, "", false, true)|indent(8, true)}};
> +{% endfor %}
> +
> +{%- for method in interface_cb.methods %}
> +	Signal<
> +{%- for param in method.parameters -%}
> +		const {{param|name}}{{" &" if not param|is_pod}}
> +		{{- ", " if not loop.last }}
> +{%- endfor -%}
> +> {{method.mojom_name}};
> +{% endfor %}
> +
> +private:
> +	void recvIPC(std::vector<uint8_t> &data, std::vector<int32_t> &fds);
> +
> +{% for method in interface_main.methods %}
> +{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> +{{proxy_funcs.func_sig(proxy_name, method, "IPC", false)|indent(8, true)}};
> +{% endfor %}
> +{% for method in interface_cb.methods %}
> +{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> +	void {{method.mojom_name}}IPC(
> +		std::vector<uint8_t> &data,
> +		std::vector<int32_t> &fds);

Should these be const ?

> +{% endfor %}
> +
> +	/* Helper class to invoke async functions in another thread. */
> +	class ThreadProxy : public Object
> +	{
> +	public:
> +		void setIPA({{interface_name}} *ipa)
> +		{
> +			ipa_ = ipa;
> +		}
> +
> +		int start()
> +		{
> +			return ipa_->start();
> +		}
> +
> +		void stop()
> +		{
> +			ipa_->stop();
> +		}
> +{% for method in interface_main.methods %}
> +{%- if method|is_async %}
> +		{{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
> +		{
> +			ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> +		}
> +{%- endif %}
> +{%- endfor %}

I think this is mixing two concepts, which shouldn't cause any issue for
now, but may come bite us back later.

As you've noticed in the existing IPAProxyThread implementation, we call
some methods directly, and proxy other methods across threads. The idea
is that anything called before start() is a direct call, and anything
from start() until but not including stop() is indirect. The reason is
that before start() there's no thread running, so we can't make a
cross-thread call.

One direct consequence of this architecture is that all calls before
start() are synchronous, and can thus return a value. Calls after
start() are asynchronous, but that's a separate design decision. We
could synchronize cross-thread calls if we wanted, we have chosen not to
in order to avoid delays that could damage the real time performance of
the pipeline handler.

With IPC the situation is different, all calls are cross-process. Still,
we want to expose a similar behaviour to pipeline handlers, so calls
before start() are synchronous, and further calls are asynchronous.

I don't think we can easily enforce this through the IDL, but I believe
it would make sense to document this requirements, so that authors will
not create an async method meant to be called before start(), or a
synchronous method after start().

> +
> +	private:
> +		{{interface_name}} *ipa_;
> +	};
> +
> +	bool running_;
> +	Thread thread_;
> +	ThreadProxy proxy_;
> +	std::unique_ptr<{{interface_name}}> ipa_;
> +
> +	const bool isolate_;
> +
> +	std::unique_ptr<IPAIPCUnixSocket> ipc_;
> +
> +	ControlSerializer controlSerializer_;
> +};
> +
> +{%- if has_namespace %}
> +{% for ns in namespace|reverse %}
> +} /* {{ns}} */
> +{% endfor %}
> +{%- endif %}
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__ */
> 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
> new file mode 100644
> index 00000000..1647a9ca
> --- /dev/null
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> @@ -0,0 +1,184 @@
> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> +
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) {{ year }}, Google Inc.
> + *
> + * ipa_proxy_{{ module_name }}_worker.cpp - Image Processing Algorithm proxy worker for {{ module_name }}
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#include <algorithm>
> +#include <iostream>
> +#include <sys/types.h>
> +#include <tuple>
> +#include <unistd.h>
> +
> +#include <libcamera/event_dispatcher.h>
> +#include <libcamera/ipa/ipa_interface.h>
> +#include <libcamera/ipa/{{module_name}}_generated.h>
> +#include <libcamera/ipa/{{module_name}}_serializer.h>
> +#include <libcamera/logging.h>
> +
> +#include "libcamera/internal/camera_sensor.h"
> +#include "libcamera/internal/control_serializer.h"
> +#include "libcamera/internal/ipa_data_serializer.h"
> +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> +#include "libcamera/internal/ipa_module.h"
> +#include "libcamera/internal/ipa_proxy.h"
> +#include "libcamera/internal/ipc_unixsocket.h"
> +#include "libcamera/internal/log.h"
> +#include "libcamera/internal/thread.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY({{proxy_name}}Worker)
> +
> +{%- if has_namespace %}
> +{% for ns in namespace -%}
> +using namespace {{ns}};
> +{% endfor %}
> +{%- endif %}
> +
> +struct CallData {
> +	IPCUnixSocket::Payload *response;
> +	bool done;
> +};
> +
> +{{interface_name}} *ipa_;
> +IPCUnixSocket socket_;
> +std::map<uint32_t, struct CallData> callData_;

callData_ doesn't seem to be used (and thus struct CallData shouldn't be
needed).

> +
> +ControlSerializer controlSerializer_;
> +
> +bool exit_ = false;

Should you group all this, as well as the functions below, in a
{{module_name}}ProxyWorker class ?

> +
> +void readyRead(IPCUnixSocket *socket)
> +{
> +	IPCUnixSocket::Payload _message, _response;
> +	int _ret = socket->receive(&_message);
> +	if (_ret) {
> +		LOG({{proxy_name}}Worker, Error)
> +			<< "Receive message failed" << _ret;
> +		return;
> +	}
> +
> +	uint32_t _cmd, _seq;
> +	std::tie(_cmd, _seq) = IPAIPCUnixSocket::readHeader(_message);
> +	IPAIPCUnixSocket::eraseHeader(_message);

Using Span would help avoiding copies.

> +
> +	switch (_cmd) {
> +	case CMD_EXIT: {
> +		exit_ = true;
> +		break;
> +	}
> +
> +{% for method in interface_main.methods %}
> +	case CMD_{{method.mojom_name|upper}}: {
> +	{{proxy_funcs.deserialize_call(method|method_param_inputs, '_message.data', '_message.fds', false, true)|indent(8, true)}}
> +{% for param in method|method_param_outputs %}
> +		{{param|name}} {{param.mojom_name}};
> +{% endfor %}
> +{%- if method|method_return_value != "void" %}
> +		{{method|method_return_value}} _callRet = ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> +{%- else %}
> +		ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}}
> +{{- ", " if method|method_param_outputs|params_comma_sep -}}
> +{%- for param in method|method_param_outputs -%}
> +&{{param.mojom_name}}{{", " if not loop.last}}
> +{%- endfor -%}
> +);
> +{%- endif %}
> +{% if not method|is_async %}
> +		IPAIPCUnixSocket::writeHeader(_response, _cmd, _seq);
> +{%- if method|method_return_value != "void" %}
> +		std::vector<uint8_t> _callRetBuf;
> +		std::tie(_callRetBuf, std::ignore) =
> +			IPADataSerializer<{{method|method_return_value}}>::serialize(_callRet);
> +		_response.data.insert(_response.data.end(), _callRetBuf.begin(), _callRetBuf.end());
> +{%- else %}
> +	{{proxy_funcs.serialize_call(method|method_param_outputs, "_response.data", "_response.fds", base_controls)|indent(8, true)}}
> +{%- endif %}
> +		int _ret = socket_.send(_response);
> +		if (_ret < 0) {
> +			LOG({{proxy_name}}Worker, Error)
> +				<< "Reply to {{method.mojom_name}}() failed" << _ret;
> +		}
> +		LOG({{proxy_name}}Worker, Debug) << "Done replying to {{method.mojom_name}}()";
> +{%- endif %}
> +		break;
> +	}
> +{% endfor %}

This isn't a huge deal, but the generated code my be a bit clearer if
each case called a function.

> +	}
> +}
> +
> +{% for method in interface_cb.methods %}
> +{{proxy_funcs.func_sig(proxy_name, method, "", false)}}
> +{
> +	IPCUnixSocket::Payload _message;
> +
> +	IPAIPCUnixSocket::writeHeader(_message, CMD_{{method.mojom_name|upper}}, 0);
> +	{{proxy_funcs.serialize_call(method|method_param_inputs, "_message.data", "_message.fds", base_controls)}}
> +
> +	socket_.send(_message);
> +
> +	LOG({{proxy_name}}Worker, Debug) << "{{method.mojom_name}} done";
> +}
> +{%- endfor %}

You can drop the -

> +
> +int main(int argc, char **argv)
> +{
> +	/* Uncomment this for debugging. */
> +#if 0
> +	std::string logPath = "/tmp/libcamera.worker." +
> +			      std::to_string(getpid()) + ".log";
> +	logSetFile(logPath.c_str());
> +#endif
> +
> +	if (argc < 3) {
> +		LOG({{proxy_name}}Worker, Debug)

Maybe s/Debug/Error/ ?

> +			<< "Tried to start worker with no args";
> +		return EXIT_FAILURE;
> +	}
> +
> +	int fd = std::stoi(argv[2]);
> +	LOG({{proxy_name}}Worker, Debug)

And Info ?

> +		<< "Starting worker for IPA module " << argv[1]
> +		<< " with IPC fd = " << fd;
> +
> +	std::unique_ptr<IPAModule> ipam = std::make_unique<IPAModule>(argv[1]);
> +	if (!ipam->isValid() || !ipam->load()) {
> +		LOG({{proxy_name}}Worker, Error)
> +			<< "IPAModule " << argv[1] << " should be valid but isn't";

Maybe just " isn't valid" ?

> +		return EXIT_FAILURE;
> +	}
> +
> +	if (socket_.bind(fd) < 0) {
> +		LOG({{proxy_name}}Worker, Error) << "IPC socket binding failed";
> +		return EXIT_FAILURE;
> +	}
> +	socket_.readyRead.connect(&readyRead);
> +
> +	ipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());
> +	if (!ipa_) {
> +		LOG({{proxy_name}}Worker, Error) << "Failed to create IPA interface instance";
> +		return EXIT_FAILURE;
> +	}
> +{% for method in interface_cb.methods %}
> +	ipa_->{{method.mojom_name}}.connect(&{{method.mojom_name}});
> +{%- endfor %}
> +
> +	LOG({{proxy_name}}Worker, Debug) << "Proxy worker successfully started";
> +
> +	/* \todo upgrade listening loop */

Good point. We should make an EventLoop class at some point.

> +	EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
> +	while (!exit_)
> +		dispatcher->processEvents();
> +
> +	delete ipa_;
> +	socket_.close();
> +
> +	return 0;
> +}
> diff --git a/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl
> new file mode 100644
> index 00000000..37352c1f
> --- /dev/null
> +++ b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl
> @@ -0,0 +1,44 @@
> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> +{%- import "serializer.tmpl" as serializer -%}
> +
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) {{ year }}, Google Inc.
> + *
> + * {{ module_name }}_serializer.h - Image Processing Algorithm data serializer for {{ module_name }}
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__
> +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__
> +
> +#include <libcamera/ipa/{{ module_name }}.h>
> +#include <libcamera/ipa/{{ module_name }}_generated.h>
> +
> +#include "libcamera/internal/control_serializer.h"
> +#include "libcamera/internal/ipa_data_serializer.h"
> +
> +#include <tuple>
> +#include <vector>
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPADataSerializer)
> +{% for struct in structs_nonempty %}
> +template<>
> +class IPADataSerializer<{{ struct|name_full(namespace_str) }}>
> +{
> +public:
> +{{- serializer.serializer(struct, base_controls, namespace_str) }}
> +{%- if struct|has_fd %}
> +{{ serializer.deserializer_fd(struct, namespace_str) }}
> +{%- else %}
> +{{ serializer.deserializer_no_fd(struct, namespace_str) }}
> +{%- endif %}
> +};
> +{% endfor %}
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__ */

I'll review proxy_functions.tmpl and serializer.tmpl separately, this is
growing quite big.

> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> new file mode 100644
> index 00000000..e1f5a20c
> --- /dev/null
> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> @@ -0,0 +1,185 @@
> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> +{#
> + # \brief Generate fuction prototype
> + #
> + # \param class Class name
> + # \param method mojom Method object
> + # \param suffix Suffix to append to \a method function name
> + # \param need_class_name True to generate class name with function
> + # \param override True to generate override tag after the function prototype
> + #}
> +{%- macro func_sig(class, method, suffix, need_class_name = true, override = false) -%}
> +{{method|method_return_value}} {{ class + "::" if need_class_name }}{{method.mojom_name}}{{suffix}}(
> +{%- for param in method|method_parameters %}
> +	{{param}}{{- "," if not loop.last}}
> +{%- endfor -%}
> +){{" override" if override}}
> +{%- endmacro -%}
> +
> +{#
> + # \brief Generate function body for IPA init() function for thread
> + #}
> +{%- macro init_thread_body() -%}
> +	int ret = ipa_->init(settings);
> +	if (ret)
> +		return ret;
> +
> +	proxy_.moveToThread(&thread_);
> +
> +	return 0;
> +{%- endmacro -%}
> +
> +{#
> + # \brief Generate function body for IPA start() function for thread
> + #}
> +{%- macro start_thread_body() -%}
> +	running_ = true;
> +	thread_.start();
> +
> +	return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);
> +{%- endmacro -%}
> +
> +{#
> + # \brief Generate function body for IPA stop() function for thread
> + #}
> +{%- macro stop_thread_body() -%}
> +	if (!running_)
> +		return;
> +
> +	running_ = false;
> +
> +	proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
> +
> +	thread_.exit();
> +	thread_.wait();
> +{%- endmacro -%}
> +
> +
> +{#
> + # \brief Serialize multiple objects into data buffer and fd vector
> + #
> + # Generate code to serialize multiple objects, as specified in \a params
> + # (which are the parameters to some function), into \a buf data buffer and
> + # \a fds fd vector.
> + # This code is meant to be used by the proxy, for serializing prior to IPC calls.
> + #}
> +{%- macro serialize_call(params, buf, fds, base_controls) %}
> +{%- for param in params %}
> +	std::vector<uint8_t> {{param.mojom_name}}Buf;
> +{%- if param|has_fd %}
> +	std::vector<int32_t> {{param.mojom_name}}Fds;
> +	std::tie({{param.mojom_name}}Buf, {{param.mojom_name}}Fds) =
> +{%- else %}
> +	std::tie({{param.mojom_name}}Buf, std::ignore) =
> +{%- endif %}
> +{%- if param|is_controls %}
> +		IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}, {{param.mojom_name}}.infoMap() ? *{{param.mojom_name}}.infoMap() : {{base_controls}}
> +{%- else %}
> +		IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}
> +{%- endif %}
> +{{- ", &controlSerializer_" if param|needs_control_serializer -}}
> +);
> +{%- endfor %}
> +
> +{%- if params|length > 1 %}
> +{%- for param in params %}
> +	appendUInt<uint32_t>({{buf}}, {{param.mojom_name}}Buf.size());
> +{%- if param|has_fd %}
> +	appendUInt<uint32_t>({{buf}}, {{param.mojom_name}}Fds.size());
> +{%- endif %}
> +{%- endfor %}
> +{%- endif %}
> +
> +{%- for param in params %}
> +	{{buf}}.insert({{buf}}.end(), {{param.mojom_name}}Buf.begin(), {{param.mojom_name}}Buf.end());
> +{%- endfor %}
> +
> +{%- for param in params %}
> +{%- if param|has_fd %}
> +	{{fds}}.insert({{fds}}.end(), {{param.mojom_name}}Fds.begin(), {{param.mojom_name}}Fds.end());
> +{%- endif %}
> +{%- endfor %}
> +{%- endmacro -%}
> +
> +
> +{#
> + # \brief Deserialize a single object from data buffer and fd vector
> + #
> + # \param pointer True deserializes the object into a dereferenced pointer
> + #
> + # Generate code to deserialize a single object, as specified in \a param,
> + # from \a buf data buffer and \a fds fd vector.
> + # This code is meant to be used by macro deserialize_call.
> + #}
> +{%- macro deserialize_param(param, pointer, loop, buf, fds) -%}
> +{{"*" if pointer}}{{param.mojom_name}} = IPADataSerializer<{{param|name}}>::deserialize(
> +	{{buf}}.begin() + {{param.mojom_name}}Start,
> +{%- if loop.last %}
> +	{{buf}}.end()
> +{%- else %}
> +	{{buf}}.begin() + {{param.mojom_name}}Start + {{param.mojom_name}}BufSize
> +{%- endif -%}
> +{{- "," if param|has_fd }}
> +{%- if param|has_fd %}
> +	{{fds}}.begin() + {{param.mojom_name}}FdStart,
> +{%- if loop.last %}
> +	{{fds}}.end()
> +{%- else %}
> +	{{fds}}.begin() + {{param.mojom_name}}FdStart + {{param.mojom_name}}FdsSize
> +{%- endif -%}
> +{%- endif -%}
> +{{- "," if param|needs_control_serializer }}
> +{%- if param|needs_control_serializer %}
> +	&controlSerializer_
> +{%- endif -%}
> +);
> +{%- endmacro -%}
> +
> +
> +{#
> + # \brief Deserialize multiple objects from data buffer and fd vector
> + #
> + # \param pointer True deserializes objects into pointers, and adds a null check.
> + # \param declare True declares the objects in addition to deserialization.
> + #
> + # Generate code to deserialize multiple objects, as specified in \a params
> + # (which are the parameters to some function), from \a buf data buffer and
> + # \a fds fd vector.
> + # This code is meant to be used by the proxy, for serializing prior to IPC calls.
> + #}
> +{%- macro deserialize_call(params, buf, fds, pointer = true, declare = false) -%}
> +{% set ns = namespace(size_offset = 0) %}
> +{%- if params|length > 1 %}
> +{%- for param in params %}
> +	[[maybe_unused]] size_t {{param.mojom_name}}BufSize = readUInt<uint32_t>({{buf}}, {{ns.size_offset}});
> +	{%- set ns.size_offset = ns.size_offset + 4 %}
> +{%- if param|has_fd %}
> +	[[maybe_unused]] size_t {{param.mojom_name}}FdsSize = readUInt<uint32_t>({{buf}}, {{ns.size_offset}});
> +	{%- set ns.size_offset = ns.size_offset + 4 %}
> +{%- endif %}
> +{%- endfor %}
> +{%- endif %}
> +{% for param in params %}
> +{%- if loop.first %}
> +	size_t {{param.mojom_name}}Start = {{ns.size_offset}};
> +{%- else %}
> +	size_t {{param.mojom_name}}Start = {{loop.previtem.mojom_name}}Start + {{loop.previtem.mojom_name}}BufSize;
> +{%- endif %}
> +{%- endfor %}
> +{% for param in params|with_fds %}
> +{%- if loop.first %}
> +	size_t {{param.mojom_name}}FdStart = 0;
> +{%- elif not loop.last %}
> +	size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
> +{%- endif %}
> +{%- endfor %}
> +{% for param in params %}
> +	{%- if pointer %}
> +	if ({{param.mojom_name}}) {
> +{{deserialize_param(param, pointer, loop, buf, fds)|indent(16, True)}}
> +	}
> +	{%- else %}
> +	{{param|name + " " if declare}}{{deserialize_param(param, pointer, loop, buf, fds)|indent(8)}}
> +	{%- endif %}
> +{% endfor %}
> +{%- endmacro -%}
> diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> new file mode 100644
> index 00000000..3c071c4e
> --- /dev/null
> +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> @@ -0,0 +1,276 @@
> +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> +{# Turn this into a C macro? #}
> +{#
> + # \brief Verify that there is enough bytes to deserialize
> + #
> + # Generate code that verifies that \a size is not greater than \a dataSize.
> + # Otherwise log an error with \a name and \a typename.
> + #}
> +{%- macro check_data_size(size, dataSize, name, typename) %}
> +		if ({{size}} > {{dataSize}}) {
> +			LOG(IPADataSerializer, Error)
> +				<< "Failed to deserialize {{name}}: not enough {{typename}}, expected "
> +				<< ({{size}}) << ", got " << ({{dataSize}});
> +			return ret;
> +		}
> +{%- endmacro %}
> +
> +
> +{#
> + # \brief Serialize some field into return vector
> + #
> + # Generate code to serialize \a field into ret_data, including size of the
> + # field and fds (where appropriate). \a base_controls indicates the
> + # default ControlInfoMap in the event that the ControlList does not have one.
> + # This code is meant to be used by the IPADataSerializer specialization.
> + #}
> +{%- macro serializer_field(field, base_controls, namespace, loop) %}
> +{%- if field|is_pod or field|is_enum %}
> +		std::vector<uint8_t> {{field.mojom_name}};
> +		std::tie({{field.mojom_name}}, std::ignore) =
> +	{%- if field|is_pod %}
> +			IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);
> +	{%- elif field|is_enum %}
> +			IPADataSerializer<uint{{field|bit_width}}_t>::serialize(data.{{field.mojom_name}}_);
> +	{%- endif %}
> +		ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> +{%- elif field|is_fd %}
> +		std::vector<uint8_t> {{field.mojom_name}};
> +		std::vector<int32_t> {{field.mojom_name}}Fds;
> +		std::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =
> +			IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);
> +		ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> +		ret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());
> +{%- elif field|is_controls %}
> +		if (data.{{field.mojom_name}}_.size() > 0) {
> +			std::vector<uint8_t> {{field.mojom_name}};
> +			std::tie({{field.mojom_name}}, std::ignore) =
> +				IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_,
> +					data.{{field.mojom_name}}_.infoMap() ? *data.{{field.mojom_name}}_.infoMap() : {{base_controls}},
> +					cs);
> +			appendUInt<uint32_t>(ret_data, {{field.mojom_name}}.size());
> +			ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> +		} else {
> +			appendUInt<uint32_t>(ret_data, 0);
> +		}
> +{%- elif field|is_plain_struct or field|is_array or field|is_map %}
> +		std::vector<uint8_t> {{field.mojom_name}};
> +	{%- if field|has_fd %}
> +		std::vector<int32_t> {{field.mojom_name}}Fds;
> +		std::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =
> +	{%- else %}
> +		std::tie({{field.mojom_name}}, std::ignore) =
> +	{%- endif %}
> +	{%- if field|is_array or field|is_map %}
> +			IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_, cs);
> +	{%- else %}
> +			IPADataSerializer<{{field|name_full(namespace)}}>::serialize(data.{{field.mojom_name}}_, cs);
> +	{%- endif %}
> +		appendUInt<uint32_t>(ret_data, {{field.mojom_name}}.size());
> +	{%- if field|has_fd %}
> +		appendUInt<uint32_t>(ret_data, {{field.mojom_name}}Fds.size());
> +	{%- endif %}
> +		ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> +	{%- if field|has_fd %}
> +		ret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());
> +	{%- endif %}
> +{%- else %}
> +		/* Unknown serialization for {{field.mojom_name}}. */
> +{%- endif %}
> +{%- endmacro %}
> +
> +
> +{#
> + # \brief Deserialize some field into return struct
> + #
> + # Generate code to deserialize \a field into object ret.
> + # This code is meant to be used by the IPADataSerializer specialization.
> + #}
> +{%- macro deserializer_field(field, namespace, loop) %}
> +{% if field|is_pod or field|is_enum %}
> +	{%- set field_size = (field|bit_width|int / 8)|int %}
> +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> +		{%- if field|is_pod %}
> +		ret.{{field.mojom_name}}_ = static_cast<{{field|name}}>(IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}));
> +		{%- else %}
> +		ret.{{field.mojom_name}}_ = static_cast<{{field|name}}>(IPADataSerializer<uint{{field|bit_width}}_t>::deserialize(m, m + {{field_size}}));
> +		{%- endif %}
> +	{%- if not loop.last %}
> +		m += {{field_size}};
> +		dataSize -= {{field_size}};
> +	{%- endif %}
> +{% elif field|is_fd %}
> +	{%- set field_size = 1 %}
> +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> +		ret.{{field.mojom_name}}_ = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs);
> +	{%- if not loop.last %}
> +		m += {{field_size}};
> +		dataSize -= {{field_size}};
> +		n += ret.{{field.mojom_name}}_.isValid() ? 1 : 0;
> +		fdsSize -= ret.{{field.mojom_name}}_.isValid() ? 1 : 0;
> +	{%- endif %}
> +{% elif field|is_controls %}
> +	{%- set field_size = 4 %}
> +		{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}
> +		size_t {{field.mojom_name}}Size = readUInt<uint32_t>(m);
> +	{%- set field_size = '4 + ' + field.mojom_name + 'Size' -%}
> +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> +		if ({{field.mojom_name}}Size > 0)
> +			ret.{{field.mojom_name}}_ =
> +				IPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> +	{%- if not loop.last %}
> +		m += {{field_size}};
> +		dataSize -= {{field_size}};
> +	{%- endif %}
> +{% elif field|is_plain_struct or field|is_array or field|is_map%}
> +	{%- set field_size = 4 %}
> +		{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}
> +		size_t {{field.mojom_name}}Size = readUInt<uint32_t>(m);
> +	{%- if field|has_fd %}
> +	{%- set field_size = 8 %}
> +		{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'FdsSize', 'data') }}
> +		size_t {{field.mojom_name}}FdsSize = readUInt<uint32_t>(m + 4);
> +		{{- check_data_size(field.mojom_name + 'FdsSize', 'fdsSize', field.mojom_name, 'fds') }}
> +	{%- endif %}
> +	{%- set field_size = field|has_fd|choose('8 + ', '4 + ') + field.mojom_name + 'Size' -%}
> +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> +		ret.{{field.mojom_name}}_ =
> +	{%- if field|has_fd and (field|is_array or field|is_map) %}
> +			IPADataSerializer<{{field|name}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);
> +	{%- elif field|has_fd and (not (field|is_array or field|is_map)) %}
> +			IPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);
> +	{%- elif (not field|has_fd) and (field|is_array or field|is_map) %}
> +			IPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> +	{%- else %}
> +			IPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> +	{%- endif %}
> +	{%- if not loop.last %}
> +		m += {{field_size}};
> +		dataSize -= {{field_size}};
> +	{%- if field|has_fd %}
> +		n += {{field.mojom_name}}FdsSize;
> +		fdsSize -= {{field.mojom_name}}FdsSize;
> +	{%- endif %}
> +	{%- endif %}
> +{% else %}
> +		/* Unknown deserialization for {{field.mojom_name}}. */
> +{%- endif %}
> +{%- endmacro %}
> +
> +
> +{#
> + # \brief Serialize a struct
> + #
> + # Generate code for IPADataSerializer specialization, for serializing
> + # \a struct. \a base_controls indicates the default ControlInfoMap
> + # in the event that the ControlList does not have one.
> + #}
> +{%- macro serializer(struct, base_controls, namespace) %}
> +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +	serialize(const {{ struct|name_full(namespace) }} &data,
> +{%- if struct|needs_control_serializer %}
> +		  ControlSerializer *cs)
> +{%- else %}
> +		  [[maybe_unused]] ControlSerializer *cs = nullptr)
> +{%- endif %}
> +	{
> +		std::vector<uint8_t> ret_data;
> +{%- if struct|has_fd %}
> +		std::vector<int32_t> ret_fds;
> +{%- endif %}
> +{%- for field in struct.fields %}
> +{{serializer_field(field, base_controls, namespace, loop)}}
> +{%- endfor %}
> +{% if struct|has_fd %}
> +		return {ret_data, ret_fds};
> +{%- else %}
> +		return {ret_data, {}};
> +{%- endif %}
> +	}
> +{%- endmacro %}
> +
> +
> +{#
> + # \brief Deserialize a struct that has fds
> + #
> + # Generate code for IPADataSerializer specialization, for deserializing
> + # \a struct, in the case that \a struct has file descriptors.
> + #}
> +{%- macro deserializer_fd(struct, namespace) %}
> +	static {{ struct|name_full(namespace) }}
> +	deserialize(std::vector<uint8_t> &data,
> +		    std::vector<int32_t> &fds,
> +{%- if struct|needs_control_serializer %}
> +		    ControlSerializer *cs)
> +{%- else %}
> +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> +{%- endif %}
> +	{
> +		{{struct|name_full(namespace)}} ret;
> +		std::vector<uint8_t>::iterator m = data.begin();
> +		std::vector<int32_t>::iterator n = fds.begin();
> +
> +		size_t dataSize = data.size();
> +		size_t fdsSize = fds.size();
> +{%- for field in struct.fields -%}
> +{{deserializer_field(field, namespace, loop)}}
> +{%- endfor %}
> +		return ret;
> +	}
> +
> +	static {{ struct|name_full(namespace) }}
> +	deserialize(std::vector<uint8_t>::iterator data_it1,
> +		    std::vector<uint8_t>::iterator data_it2,
> +		    std::vector<int32_t>::iterator fds_it1,
> +		    std::vector<int32_t>::iterator fds_it2,
> +{%- if struct|needs_control_serializer %}
> +		    ControlSerializer *cs)
> +{%- else %}
> +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> +{%- endif %}
> +	{
> +		std::vector<uint8_t> data(data_it1, data_it2);
> +		std::vector<int32_t> fds(fds_it1, fds_it2);
> +		return IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, fds, cs);
> +	}
> +{%- endmacro %}
> +
> +
> +{#
> + # \brief Deserialize a struct that has no fds
> + #
> + # Generate code for IPADataSerializer specialization, for deserializing
> + # \a struct, in the case that \a struct does not have file descriptors.
> + #}
> +{%- macro deserializer_no_fd(struct, namespace) %}
> +	static {{ struct|name_full(namespace) }}
> +	deserialize(std::vector<uint8_t> &data,
> +{%- if struct|needs_control_serializer %}
> +		    ControlSerializer *cs)
> +{%- else %}
> +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> +{%- endif %}
> +	{
> +		{{struct|name_full(namespace)}} ret;
> +		std::vector<uint8_t>::iterator m = data.begin();
> +
> +		size_t dataSize = data.size();
> +{%- for field in struct.fields -%}
> +{{deserializer_field(field, namespace, loop)}}
> +{%- endfor %}
> +		return ret;
> +	}
> +
> +	static {{ struct|name_full(namespace) }}
> +	deserialize(std::vector<uint8_t>::iterator data_it1,
> +		    std::vector<uint8_t>::iterator data_it2,
> +{%- if struct|needs_control_serializer %}
> +		    ControlSerializer *cs)
> +{%- else %}
> +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> +{%- endif %}
> +	{
> +		std::vector<uint8_t> data(data_it1, data_it2);
> +		return IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, cs);
> +	}
> +{%- endmacro %}
> diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py
> new file mode 100644
> index 00000000..a9269c82
> --- /dev/null
> +++ b/utils/ipc/generators/mojom_libcamera_generator.py
> @@ -0,0 +1,461 @@
> +'''Generates libcamera files from a mojom.Module.'''
> +
> +import argparse
> +import ast
> +import datetime
> +import contextlib
> +import os
> +import re
> +import shutil
> +import sys
> +import tempfile
> +
> +from jinja2 import contextfilter
> +
> +import mojom.fileutil as fileutil
> +import mojom.generate.generator as generator
> +import mojom.generate.module as mojom
> +from mojom.generate.template_expander import UseJinja
> +
> +
> +GENERATOR_PREFIX = 'libcamera'
> +
> +_kind_to_cpp_type = {
> +    mojom.BOOL:   'bool',
> +    mojom.INT8:   'int8_t',
> +    mojom.UINT8:  'uint8_t',
> +    mojom.INT16:  'int16_t',
> +    mojom.UINT16: 'uint16_t',
> +    mojom.INT32:  'int32_t',
> +    mojom.UINT32: 'uint32_t',
> +    mojom.FLOAT:  'float',
> +    mojom.INT64:  'int64_t',
> +    mojom.UINT64: 'uint64_t',
> +    mojom.DOUBLE: 'double',
> +}
> +
> +_bit_widths = {
> +    mojom.BOOL:   '8',
> +    mojom.INT8:   '8',
> +    mojom.UINT8:  '8',
> +    mojom.INT16:  '16',
> +    mojom.UINT16: '16',
> +    mojom.INT32:  '32',
> +    mojom.UINT32: '32',
> +    mojom.FLOAT:  '32',
> +    mojom.INT64:  '64',
> +    mojom.UINT64: '64',
> +    mojom.DOUBLE: '64',
> +}
> +
> +def ModuleName(path):
> +    return path.split('/')[-1].split('.')[0]
> +
> +def ModuleClassName(module):
> +    s = re.sub(r'^IPA', '',  module.interfaces[0].mojom_name)
> +    return re.sub(r'Interface$', '', s)
> +
> +def UpperCamelCase(name):
> +    return ''.join([x.capitalize() for x in generator.SplitCamelCase(name)])
> +
> +def CamelCase(name):
> +    uccc = UpperCamelCase(name)
> +    return uccc[0].lower() + uccc[1:]
> +
> +def ConstantStyle(name):
> +    return generator.ToUpperSnakeCase(name)
> +
> +def Choose(cond, t, f):
> +    return t if cond else f
> +
> +def CommaSep(l):
> +    return ', '.join([m for m in l])
> +
> +def ParamsCommaSep(l):
> +    return ', '.join([m.mojom_name for m in l])
> +
> +def GetDefaultValue(element):
> +    if element.default is not None:
> +        return element.default
> +    if type(element.kind) == mojom.Kind:
> +        return '0'
> +    if mojom.IsEnumKind(element.kind):
> +        return f'static_cast<{element.kind.mojom_name}>(0)'
> +    if (isinstance(element.kind, mojom.Struct) and
> +       element.kind.mojom_name == 'FileDescriptor'):
> +        return '-1'
> +    return ''
> +
> +def WithDefaultValues(element):
> +    return [x for x in element if HasDefaultValue(x)]
> +
> +def WithFds(element):
> +    return [x for x in element if HasFd(x)]
> +
> +def HasDefaultValue(element):
> +    return GetDefaultValue(element) != ''
> +
> +def HasDefaultFields(element):
> +    return True in [HasDefaultValue(x) for x in element.fields]
> +
> +def GetAllTypes(element):
> +    if mojom.IsArrayKind(element):
> +        return GetAllTypes(element.kind)
> +    if mojom.IsMapKind(element):
> +        return GetAllTypes(element.key_kind) + GetAllTypes(element.value_kind)
> +    if isinstance(element, mojom.Parameter):
> +        return GetAllTypes(element.kind)
> +    if mojom.IsEnumKind(element):
> +        return [element.mojom_name]
> +    if not mojom.IsStructKind(element):
> +        return [element.spec]
> +    if len(element.fields) == 0:
> +        return [element.mojom_name]
> +    ret = [GetAllTypes(x.kind) for x in element.fields]
> +    ret = [x for sublist in ret for x in sublist]
> +    return list(set(ret))
> +
> +def GetAllAttrs(element):
> +    if mojom.IsArrayKind(element):
> +        return GetAllAttrs(element.kind)
> +    if mojom.IsMapKind(element):
> +        return {**GetAllAttrs(element.key_kind), **GetAllAttrs(element.value_kind)}
> +    if isinstance(element, mojom.Parameter):
> +        return GetAllAttrs(element.kind)
> +    if mojom.IsEnumKind(element):
> +        return element.attributes if element.attributes is not None else {}
> +    if mojom.IsStructKind(element) and len(element.fields) == 0:
> +        return element.attributes if element.attributes is not None else {}
> +    if not mojom.IsStructKind(element):
> +        return {}
> +    attrs = [(x.attributes) for x in element.fields]
> +    ret = {}
> +    for d in attrs:
> +        if d is not None:
> +            ret = {**ret, **d}
> +    return ret
> +
> +def NeedsControlSerializer(element):
> +    types = GetAllTypes(element)
> +    return "ControlList" in types or "ControlInfoMap" in types
> +
> +def HasFd(element):
> +    attrs = GetAllAttrs(element)
> +    if isinstance(element, mojom.Kind):
> +        types = GetAllTypes(element)
> +    else:
> +        types = GetAllTypes(element.kind)
> +    return "FileDescriptor" in types or (attrs is not None and "hasFd" in attrs)
> +
> +def MethodInputHasFd(method):
> +    if len([x for x in method.parameters if HasFd(x)]) > 0:
> +        return True
> +    return False
> +
> +def MethodOutputHasFd(method):
> +    if (MethodReturnValue(method) != 'void' or
> +        method.response_parameters is None):
> +        return False
> +    if len([x for x in method.parameters if HasFd(x)]) > 0:
> +        return True
> +    return False
> +
> +def MethodParamInputs(method):
> +    return method.parameters
> +
> +def MethodParamOutputs(method):
> +    if (MethodReturnValue(method) != 'void' or
> +        method.response_parameters is None):
> +        return []
> +    return method.response_parameters
> +
> +def MethodParamNames(method):
> +    params = []
> +    for param in method.parameters:
> +        params.append(param.mojom_name)
> +    if MethodReturnValue(method) == 'void':
> +        if method.response_parameters is None:
> +            return params
> +        for param in method.response_parameters:
> +            params.append(param.mojom_name)
> +    return params
> +
> +def MethodParameters(method):
> +    params = []
> +    for param in method.parameters:
> +        params.append('const %s %s%s' % (GetNameForElement(param),
> +                                         ('&' if not IsPod(param) else ''),
> +                                         param.mojom_name))
> +    if MethodReturnValue(method) == 'void':
> +        if method.response_parameters is None:
> +            return params
> +        for param in method.response_parameters:
> +            params.append(f'{GetNameForElement(param)} *{param.mojom_name}')
> +    return params
> +
> +def MethodReturnValue(method):
> +    if method.response_parameters is None:
> +        return 'void'
> +    if len(method.response_parameters) == 1 and IsPod(method.response_parameters[0]):
> +        return GetNameForElement(method.response_parameters[0])
> +    return 'void'
> +
> +def IsAsync(method):
> +    # callbacks are always async
> +    if re.match("^IPA.*CallbackInterface$", method.interface.mojom_name):
> +        return True
> +    elif re.match("^IPA.*Interface$", method.interface.mojom_name):
> +        if method.attributes is None:
> +            return False
> +        elif 'async' in method.attributes and method.attributes['async']:
> +            return True
> +    return False
> +
> +def IsArray(element):
> +    return mojom.IsArrayKind(element.kind)
> +
> +def IsControls(element):
> +    return mojom.IsStructKind(element.kind) and (element.kind.mojom_name == "ControlList" or
> +                                                 element.kind.mojom_name == "ControlInfoMap")
> +
> +def IsEnum(element):
> +    return mojom.IsEnumKind(element.kind)
> +
> +def IsFd(element):
> +    return mojom.IsStructKind(element.kind) and element.kind.mojom_name == "FileDescriptor"
> +
> +def IsMap(element):
> +    return mojom.IsMapKind(element.kind)
> +
> +def IsPlainStruct(element):
> +    return mojom.IsStructKind(element.kind) and not IsControls(element) and not IsFd(element)
> +
> +def IsPod(element):
> +    return element.kind in _kind_to_cpp_type
> +
> +def BitWidth(element):
> +    if element.kind in _bit_widths:
> +        return _bit_widths[element.kind]
> +    if mojom.IsEnumKind(element.kind):
> +        return '32'
> +    return ''
> +
> +def GetNameForElement(element):
> +    if (mojom.IsEnumKind(element) or
> +        mojom.IsInterfaceKind(element) or
> +        mojom.IsStructKind(element)):
> +        return element.mojom_name
> +    if (mojom.IsArrayKind(element)):
> +        elem_name = GetNameForElement(element.kind)
> +        return f'std::vector<{elem_name}>'
> +    if (mojom.IsMapKind(element)):
> +        key_name = GetNameForElement(element.key_kind)
> +        value_name = GetNameForElement(element.value_kind)
> +        return f'std::map<{key_name}, {value_name}>'
> +    if isinstance(element, (mojom.Field, mojom.Method, mojom.Parameter)):
> +        if (mojom.IsArrayKind(element.kind) or mojom.IsMapKind(element.kind)):
> +            return GetNameForElement(element.kind)
> +        if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):
> +            return _kind_to_cpp_type[element.kind]
> +        return element.kind.mojom_name
> +    if isinstance(element,  mojom.EnumValue):
> +        return (GetNameForElement(element.enum) + '.' +
> +                ConstantStyle(element.name))
> +    if isinstance(element, (mojom.NamedValue,
> +                            mojom.Constant,
> +                            mojom.EnumField)):
> +        return ConstantStyle(element.name)
> +    if (hasattr(element, '__hash__') and element in _kind_to_cpp_type):
> +        return _kind_to_cpp_type[element]
> +    if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):
> +        return _kind_to_cpp_type[element.kind]
> +    if (hasattr(element, 'spec')):
> +        return element.spec.split(':')[-1]
> +    if (mojom.IsInterfaceRequestKind(element) or
> +        mojom.IsAssociatedKind(element) or
> +        mojom.IsPendingRemoteKind(element) or
> +        mojom.IsPendingReceiverKind(element) or
> +        mojom.IsUnionKind(element)):
> +        raise Exception('Unsupported element: %s' % element)
> +    raise Exception('Unexpected element: %s' % element)
> +
> +def GetFullNameForElement(element, namespace_str):
> +    name = GetNameForElement(element)
> +    if namespace_str == '':
> +        return name
> +    return f'{namespace_str}::{name}'
> +
> +def ValidateZeroLength(l, s, cap=True):
> +    if len(l) > 0:
> +        raise Exception(f'{s.capitalize() if cap else s} should be empty')
> +
> +def ValidateSingleLength(l, s, cap=True):
> +    if len(l) > 1:
> +        raise Exception(f'Only one {s} allowed')
> +    if len(l) < 1:
> +        raise Exception(f'{s.capitalize() if cap else s} is required')
> +
> +def ValidateInterfaces(interfaces):
> +    # Validate presence of main interface
> +    intf = [x for x in interfaces
> +            if re.match("^IPA.*Interface", x.mojom_name) and
> +               not re.match("^IPA.*CallbackInterface", x.mojom_name)]
> +    ValidateSingleLength(intf, 'main interface')
> +
> +    # Validate presence of callback interface
> +    cb = [x for x in interfaces if re.match("^IPA.*CallbackInterface", x.mojom_name)]
> +    ValidateSingleLength(cb, 'callback interface')
> +
> +    # Validate required main interface functions
> +    intf = intf[0]
> +    f_init  = [x for x in intf.methods if x.mojom_name == 'init']
> +    f_start = [x for x in intf.methods if x.mojom_name == 'start']
> +    f_stop  = [x for x in intf.methods if x.mojom_name == 'stop']
> +
> +    ValidateSingleLength(f_init, 'init()', False)
> +    ValidateSingleLength(f_start, 'start()', False)
> +    ValidateSingleLength(f_stop, 'stop()', False)
> +
> +    f_init  = f_init[0]
> +    f_start = f_start[0]
> +    f_stop  = f_stop[0]
> +
> +    # Validate parameters to init()
> +    ValidateSingleLength(f_init.parameters, 'input parameter to init()')
> +    ValidateSingleLength(f_init.response_parameters, 'output parameter from init()')
> +    if f_init.parameters[0].kind.mojom_name != 'IPASettings':
> +        raise Exception('init() must have single IPASettings input parameter')
> +    if f_init.response_parameters[0].kind.spec != 'i32':
> +        raise Exception('init() must have single int32 output parameter')
> +
> +    # Validate parameters to start()
> +    ValidateZeroLength(f_start.parameters, 'input parameter to start()')
> +    ValidateSingleLength(f_start.response_parameters, 'output parameter from start()')
> +    if f_start.response_parameters[0].kind.spec != 'i32':
> +        raise Exception('start() must have single int32 output parameter')
> +
> +    # Validate parameters to stop()
> +    ValidateZeroLength(f_stop.parameters, 'input parameter to stop()')
> +    ValidateZeroLength(f_stop.parameters, 'output parameter from stop()')
> +
> +class Generator(generator.Generator):
> +
> +    @staticmethod
> +    def GetTemplatePrefix():
> +        return 'libcamera_templates'
> +
> +    def GetFilters(self):
> +        libcamera_filters = {
> +            'all_types': GetAllTypes,
> +            'bit_width': BitWidth,
> +            'choose': Choose,
> +            'comma_sep': CommaSep,
> +            'default_value': GetDefaultValue,
> +            'has_default_fields': HasDefaultFields,
> +            'has_fd': HasFd,
> +            'is_async': IsAsync,
> +            'is_array': IsArray,
> +            'is_controls': IsControls,
> +            'is_enum': IsEnum,
> +            'is_fd': IsFd,
> +            'is_map': IsMap,
> +            'is_plain_struct': IsPlainStruct,
> +            'is_pod': IsPod,
> +            'method_input_has_fd': MethodInputHasFd,
> +            'method_output_has_fd': MethodOutputHasFd,
> +            'method_param_names': MethodParamNames,
> +            'method_param_inputs': MethodParamInputs,
> +            'method_param_outputs': MethodParamOutputs,
> +            'method_parameters': MethodParameters,
> +            'method_return_value': MethodReturnValue,
> +            'name': GetNameForElement,
> +            'name_full': GetFullNameForElement,
> +            'needs_control_serializer': NeedsControlSerializer,
> +            'params_comma_sep': ParamsCommaSep,
> +            'with_default_values': WithDefaultValues,
> +            'with_fds': WithFds,
> +        }
> +        return libcamera_filters
> +
> +    def _GetJinjaExports(self):
> +        return {
> +            'base_controls': '%s::Controls' % ModuleClassName(self.module),
> +            'cmd_enum_name': '_%sCMD' % ModuleClassName(self.module),

s/CMD/Command/ ?

> +            'enums': self.module.enums,
> +            'has_array': len([x for x in self.module.kinds.keys() if x[0] == 'a']) > 0,
> +            'has_map': len([x for x in self.module.kinds.keys() if x[0] == 'm']) > 0,
> +            'has_namespace': self.module.mojom_namespace != '',
> +            'imports': self.module.imports,
> +            'interface_cb': self.module.interfaces[1],
> +            'interface_main': self.module.interfaces[0],
> +            'interface_name': 'IPA%sInterface' % ModuleClassName(self.module),
> +            'ipc_name': 'IPAIPCUnixSocket',
> +            'kinds': self.module.kinds,
> +            'module': self.module,
> +            'module_name': ModuleName(self.module.path),
> +            'module_class_name': ModuleClassName(self.module),
> +            'namespace': self.module.mojom_namespace.split('.'),
> +            'namespace_str': self.module.mojom_namespace.replace('.', '::') if
> +                             self.module.mojom_namespace is not None else '',
> +            'proxy_name': 'IPAProxy%s' % ModuleClassName(self.module),
> +            'proxy_worker_name': 'IPAProxy%sWorker' % ModuleClassName(self.module),
> +            'structs_nonempty': [x for x in self.module.structs if len(x.fields) > 0],
> +            'year': datetime.datetime.now().year,
> +        }
> +
> +
> +    @UseJinja('module_generated.h.tmpl')
> +    def _GenerateDataHeader(self):
> +        return self._GetJinjaExports()
> +
> +    @UseJinja('module_serializer.h.tmpl')
> +    def _GenerateSerializer(self):
> +        return self._GetJinjaExports()
> +
> +    @UseJinja('module_ipa_proxy.cpp.tmpl')
> +    def _GenerateProxyCpp(self):
> +        return self._GetJinjaExports()
> +
> +    @UseJinja('module_ipa_proxy.h.tmpl')
> +    def _GenerateProxyHeader(self):
> +        return self._GetJinjaExports()
> +
> +    @UseJinja('module_ipa_proxy_worker.cpp.tmpl')
> +    def _GenerateProxyWorker(self):
> +        return self._GetJinjaExports()
> +
> +    def GenerateFiles(self, unparsed_args):
> +        parser = argparse.ArgumentParser()
> +        parser.add_argument('--libcamera_generate_header',       action='store_true')
> +        parser.add_argument('--libcamera_generate_serializer',   action='store_true')
> +        parser.add_argument('--libcamera_generate_proxy_cpp',    action='store_true')
> +        parser.add_argument('--libcamera_generate_proxy_h',      action='store_true')
> +        parser.add_argument('--libcamera_generate_proxy_worker', action='store_true')
> +        parser.add_argument('--libcamera_output_path')
> +        args = parser.parse_args(unparsed_args)
> +
> +        ValidateInterfaces(self.module.interfaces)
> +
> +        fileutil.EnsureDirectoryExists(os.path.dirname(args.libcamera_output_path))
> +
> +        module_name = ModuleName(self.module.path)
> +
> +        if args.libcamera_generate_header:
> +            self.Write(self._GenerateDataHeader(),
> +                       args.libcamera_output_path)
> +
> +        if args.libcamera_generate_serializer:
> +            self.Write(self._GenerateSerializer(),
> +                       args.libcamera_output_path)
> +
> +        if args.libcamera_generate_proxy_cpp:
> +            self.Write(self._GenerateProxyCpp(),
> +                       args.libcamera_output_path)
> +
> +        if args.libcamera_generate_proxy_h:
> +            self.Write(self._GenerateProxyHeader(),
> +                       args.libcamera_output_path)
> +
> +        if args.libcamera_generate_proxy_worker:
> +            self.Write(self._GenerateProxyWorker(),
> +                       args.libcamera_output_path)

Hi Laurent,

Thank you for the review.

On Sun, Oct 04, 2020 at 06:56:40PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Fri, Oct 02, 2020 at 11:31:22PM +0900, Paul Elder wrote:
> > Add templates to mojo to generate code for the IPC mechanism. These
> > templates generate:
> > - module header
> > - module serializer
> > - IPA proxy cpp, header, and worker
> > 
> > Given an input data definition mojom file for a pipeline.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v3:
> > - add support for namespaces
> > - fix enum assignment (used to have +1 for CMD applied to all enums)
> > - use readHeader, writeHeader, and eraseHeader as static class functions
> >   of IPAIPCUnixSocket (in the proxy worker)
> > - add requirement that base controls *must* be defined in
> >   libcamera::{pipeline_name}::Controls
> > 
> > Changes in v2:
> > - mandate the main and callback interfaces, and init(), start(), stop()
> >   and their parameters
> > - fix returning single pod value from IPC-called function
> > - add licenses
> > - improve auto-generated message
> > - other fixes related to serdes
> > ---
> >  .../module_generated.h.tmpl                   | 106 ++++
> 
> This isn't a great name, as _generated tells how the header is produced,
> but not what it actually is. How about module_ipa_interface.h.tmpl ?

I named it like that because that's what the generated headers end up
getting called. There's the main header, like raspberrypi.h, and then
there's the generated one, like raspberrypi_generated.h.

raspberrypi_ipa_interface.h ...?

> >  .../module_ipa_proxy.cpp.tmpl                 | 233 +++++++++
> >  .../module_ipa_proxy.h.tmpl                   | 117 +++++
> >  .../module_ipa_proxy_worker.cpp.tmpl          | 184 +++++++
> >  .../module_serializer.h.tmpl                  |  44 ++
> 
> And maybe module_ipa_serializer.h.tmpl for consistency ?

This becomes raspberrypi_serializer.h. raspberrypi_ipa_serializer.h ?

Or just the template file name?

> >  .../libcamera_templates/proxy_functions.tmpl  | 185 +++++++
> >  .../libcamera_templates/serializer.tmpl       | 276 +++++++++++
> >  .../generators/mojom_libcamera_generator.py   | 461 ++++++++++++++++++
> >  8 files changed, 1606 insertions(+)
> >  create mode 100644 utils/ipc/generators/libcamera_templates/module_generated.h.tmpl
> >  create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> >  create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> >  create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> >  create mode 100644 utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl
> >  create mode 100644 utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> >  create mode 100644 utils/ipc/generators/libcamera_templates/serializer.tmpl
> >  create mode 100644 utils/ipc/generators/mojom_libcamera_generator.py
> > 
> > diff --git a/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl
> > new file mode 100644
> > index 00000000..7dd48b0b
> > --- /dev/null
> > +++ b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl
> > @@ -0,0 +1,106 @@
> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) {{ year }}, Google Inc.
> > + *
> > + * {{ module_name }}_generated.h - Image Processing Algorithm interface for {{ module_name }}
> > + *
> > + * This file is auto-generated. Do not edit.
> > + */
> > +
> > +#ifndef __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__
> > +#define __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__
> > +
> > +#include <libcamera/ipa/ipa_interface.h>
> > +
> 
> I think you can drop this blank line.
> 
> > +#include <libcamera/ipa/{{ module_name }}.h>
> > +
> > +{% if has_map %}#include <map>{% endif %}
> > +{% if has_array %}#include <vector>{% endif %}
> > +
> > +namespace libcamera {
> > +{%- if has_namespace %}
> > +{% for ns in namespace %}
> > +namespace {{ns}} {
> 
> There's a mix of {{ var }} and {{var}}. I don't mind much which one is
> used, but I think we should be consistent.

Ah right, I forgot to fix that.

> > +{% endfor %}
> > +{%- endif %}
> > +
> > +enum {{ cmd_enum_name }} {
> 
> It would be nice to make this an enum class, to avoid namespace clashes.
> Alternatively, we could enforce the usage of namespaces for all IPAs.

I made cmd_enum_name start with an underscore to avoid namespace
clashes.

'cmd_enum_name': '_%sCMD' % ModuleClassName(self.module),

So like enum _RPiCMD. I thought the underscore would be sufficient for
anti name clashes.

> enum class also brings the added benefit of catching improper usages,
> but comes at a cost of requiring additional static_cast<> to convert
> from and to uint32_t.

I thought you could use enum as a type anyway? That's what I did with
the enums generated from the mojom file, and they all needed
static_cast<>s. They're in the generated serializers:

ret.op_ = static_cast<RkISP1Operations>(IPADataSerializer<uint32_t>::deserialize(m, m + 4));

> > +	CMD_EXIT = 0,
> 
> I'd drop the CMD_ prefix, especially if we use enum class, and use
> CamelCase for names instead of uppercase.

This cmd enum is for RPC, so I thought it was fine for just this enum to
be prefix with CMD_, just to make it clear in the generated code that
that's what this is for. UnixSocketTest does this too (that's where I
got it from).

> > +{%- for method in interface_main.methods %}
> > +	CMD_{{ method.mojom_name|upper }} = {{loop.index}},
> > +{%- endfor %}
> > +{%- set count = interface_main.methods|length -%}
> > +{%- for method in interface_cb.methods %}
> > +	CMD_{{ method.mojom_name|upper }} = {{loop.index + count}},
> > +{%- endfor %}
> 
> Should we split the enum in two, one for each interface ?

Either way is fine, it doesn't matter much. That's why I put them
together. The code generator loops over interface_{main,cb}.methods and
not the enum when it generates both sides of the RPC calls, so putting
them in different or the same enum makes literally no difference.

> > +};
> > +
> > +{% for enum in enums %}
> > +enum {{ enum.mojom_name }} {
> > +{%- for field in enum.fields %}
> > +	{{ field.mojom_name }} = {{ field.numeric_value }},
> > +{%- endfor %}
> > +};
> > +{% endfor %}
> > +
> > +{%- for struct in structs_nonempty %}
> > +struct {{ struct.mojom_name }}
> > +{
> > +public:
> > +	{{ struct.mojom_name }}() {%- if struct|has_default_fields %} :{% endif %}
> > +{%- for field in struct.fields|with_default_values -%}
> > +{{" " if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ ", " if not loop.last }}
> > +{%- endfor %} {}
> 
> This generates really long lines. How about
> 
> 	{{ struct.mojom_name }}() {%- if struct|has_default_fields %}
> 		:{% endif %}
> {%- for field in struct.fields|with_default_values -%}
> {{" " if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ ", " if not loop.last }}
> {%- endfor %}
> 	{
> 	}

Ah, right.

> Would be great to wrap the initializers line too, but I think that's
> overkill.
> 
> > +	~{{ struct.mojom_name }}() {}
> > +
> > +	{{ struct.mojom_name }}(
> > +{%- for field in struct.fields -%}
> > +{{ field|name }} {{ field.mojom_name }}{{ ", " if not loop.last }}
> > +{%- endfor -%}
> > +) :
> > +{%- for field in struct.fields -%}
> > +{{" " if loop.first}}{{ field.mojom_name}}_({{ field.mojom_name }}){{ ", " if not loop.last }}
> > +{%- endfor %} {}
> 
> Same here.
> 
> > +{% for field in struct.fields %}
> > +	{{ field|name }} {{ field.mojom_name }}_;
> > +{%- endfor %}
> > +};
> > +{% endfor %}
> > +
> > +{#-
> > +What to do about the #includes? Should we forward-declare, or
> > +require {{module_name}}.h to include the required headers?
> > +For now I'm going for the latter, coz this header essentially
> > +extends {{module_name}}.h.
> > +#}
> 
> Looking at the existing {{module_name}}.h, there are a few enums and
> constants that could be moved to {{module_name}}_generated.h, such as
> enum BufferMark or MaxLsGridSize in raspberrypi.h. What do you think ?

Yeah enums I think are better to move to the mojom file. I did that for
ConfigParameters.

mojo supports const (like if we had const uint32 MaxLsGridSize = 0x8000;)
but the code generator doesn't yet.

> Then we have libcamera::RPi::Controls, which I'm wondering how to handle
> best. It's not strictly related to this series, but wouldn't they be
> better stored in the pipeline handler, and passed to the IPA ?

I think it would be best if all ControlLists had a ControlInfoMap, then
it wouldn't matter where libcamera::RPi::controls goes.

> Finally there's the odd beast such as
> 
> #define VIMC_IPA_FIFO_PATH "/tmp/libcamera_ipa_vimc_fifo"
> 
> which I also wonder if it would make sense to pass it to the IPA at init
> time. {{module_name}}.h could then possibly be dropped.

Oh you're thinking of dropping the main one entirely. Yeah that could
work too. I'm just worried if it'll be too restrictive. #defines could
be replaced with consts, so it's just the base ControlInfoMap that's an
issue.

> > +class {{ interface_name }} : public IPAInterface
> > +{
> > +public:
> > +	virtual ~{{interface_name}}() {};
> 
> No need for the trailing ;
> 
> > +{% for method in interface_main.methods %}
> > +	virtual {{method|method_return_value}} {{method.mojom_name}}(
> > +{%- for param in method|method_parameters %}
> > +		{{param}}{{- "," if not loop.last}}
> > +{%- endfor -%}
> > +) = 0;
> > +{% endfor %}
> > +
> > +{%- for method in interface_cb.methods %}
> > +	Signal<
> > +{%- for param in method.parameters -%}
> > +		const {{param|name}}{{" &" if not param|is_pod}}
> 
> Should the const be dropped if param|is_pod ? Same below.

Ah right it's pass-by-value. Sure, let's drop it.

> > +		{{- ", " if not loop.last }}
> > +{%- endfor -%}
> > +> {{method.mojom_name}};
> > +{% endfor -%}
> > +};
> > +
> > +{%- if has_namespace %}
> > +{% for ns in namespace|reverse %}
> > +} /* {{ns}} */
> > +{% endfor %}
> > +{%- endif %}
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__ */
> > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > new file mode 100644
> > index 00000000..f3eeeaaa
> > --- /dev/null
> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > @@ -0,0 +1,233 @@
> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> > +
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) {{ year }}, Google Inc.
> > + *
> > + * ipa_proxy_{{ module_name }}.cpp - Image Processing Algorithm proxy for {{ module_name }}
> > + *
> > + * This file is auto-generated. Do not edit.
> > + */
> > +
> > +#include <vector>
> > +
> > +#include <libcamera/ipa/ipa_interface.h>
> 
> I think you can drop this, as it's included by {{module_name}}.h. Same
> below.

Oh, okay.

> > +#include <libcamera/ipa/ipa_module_info.h>
> > +#include <libcamera/ipa/{{module_name}}.h>
> > +#include <libcamera/ipa/{{module_name}}_generated.h>
> > +#include <libcamera/ipa/{{module_name}}_serializer.h>
> > +
> > +#include "libcamera/internal/control_serializer.h"
> > +#include "libcamera/internal/ipa_ipc.h"
> > +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> > +#include "libcamera/internal/ipa_data_serializer.h"
> > +#include "libcamera/internal/ipa_module.h"
> > +#include "libcamera/internal/ipa_proxy.h"
> > +#include "libcamera/internal/ipc_unixsocket.h"
> > +#include "libcamera/internal/log.h"
> > +#include "libcamera/internal/process.h"
> > +#include "libcamera/internal/thread.h"
> > +
> > +#include <libcamera/ipa/ipa_proxy_{{module_name}}.h>
> 
> Should this be moved above with the other libcamera/ipa/ headers ?

This is the header that corresponds to this cpp file. I thought that
gets special treatment and goes on its own at the end like here.

> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(IPAProxy)
> > +
> > +{%- if has_namespace %}
> > +{% for ns in namespace %}
> > +namespace {{ns}} {
> > +{% endfor %}
> > +{%- endif %}
> > +
> > +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)
> > +	: IPAProxy(ipam), running_(false),
> > +	  isolate_(isolate)
> > +{
> > +	LOG(IPAProxy, Debug)
> > +		<< "initializing dummy proxy: loading IPA from "
> 
> It's not a dummy proxy anymore :-)

Oops :p

> > +		<< ipam->path();
> > +
> > +	if (isolate_) {
> > +		const std::string proxy_worker_path = resolvePath("ipa_proxy_{{module_name}}");
> > +		if (proxy_worker_path.empty()) {
> > +			LOG(IPAProxy, Error)
> > +				<< "Failed to get proxy worker path";
> > +			return;
> > +		}
> > +
> > +		ipc_ = std::make_unique<IPAIPCUnixSocket>(ipam->path().c_str(), proxy_worker_path.c_str());
> > +		if (!ipc_->isValid()) {
> > +			LOG(IPAProxy, Error) << "Failed to create IPAIPC";
> > +			return;
> > +		}
> > +
> > +{% if interface_cb.methods|length > 0 %}
> > +		ipc_->recvIPC.connect(this, &{{proxy_name}}::recvIPC);
> > +{% endif %}
> > +
> > +		valid_ = true;
> > +		return;
> > +	}
> 
> There's very little code shared between the isolated and non-isolated
> paths. It could make sense to split this class in two. On the other
> hand, that would require the ability for the IPAManager to create the
> right class type, which may complicate things a bit, but maybe not much.
> For instance we could have the two classes called {{proxy_name}}Thread
> and {{proxy_name}}IPC, and have a {{proxy_name}} class defined as
> 
> class {{proxy_name}}
> {
> public:
> 	using IPC = {{proxy_name}}IPC;
> 	using Thread = {{proxy_name}}Thread;
> };
> 
> Then, in IPAManager::createIPA(), you would have
> 
> 	std::unique_ptr<P> proxy;
> 	if (self_->isSignatureValid(m)))
> 		proxy = std::make_unique<P::Thread>(m);
> 	else
> 		proxy = std::make_unique<P::IPC>(m);

Yeah maybe. The amount of generated code doesn't change though, it'll
just be organized differently, and the only one that will notice the
difference is IPAManager as shown in this snippet.

> > +
> > +	if (!ipam->load())
> > +		return;
> > +
> > +	IPAInterface *ipai = ipam->createInterface();
> > +	if (!ipai) {
> > +		LOG(IPAProxy, Error)
> > +			<< "Failed to create IPA context for " << ipam->path();
> > +		return;
> > +	}
> > +
> > +	ipa_ = std::unique_ptr<{{interface_name}}>(dynamic_cast<{{interface_name}} *>(ipai));
> > +	proxy_.setIPA(ipa_.get());
> > +
> > +{% for method in interface_cb.methods %}
> > +	ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);
> > +{%- endfor %}
> > +
> > +	valid_ = true;
> > +}
> > +
> > +{{proxy_name}}::~{{proxy_name}}()
> > +{
> > +	if (isolate_)
> > +		ipc_->sendAsync(CMD_EXIT, {}, {});
> > +}
> > +
> > +{% if interface_cb.methods|length > 0 %}
> > +void {{proxy_name}}::recvIPC(std::vector<uint8_t> &data, std::vector<int32_t> &fds)
> > +{
> > +	uint32_t cmd = (data[0] & 0xff) |
> > +		       ((data[1] & 0xff) << 8) |
> > +		       ((data[2] & 0xff) << 16) |
> > +		       ((data[3] & 0xff) << 24);
> 
> As data is a vector of uint8_t, I think the & 0xff is unnecessary.
> 
> Should the data length be validated ?

tbh I was thinking about it. I think cmd and vec are guaranteed, though.
Maybe we should check it just in case.

> > +
> > +	/* Need to skip another 4 bytes for the sequence number. */
> > +	std::vector<uint8_t> vec(data.begin() + 8, data.end());
> 
> This creates a copy, which isn't very nice :-/ I think the IPC class
> interface would benefit from switching from std::vector to Span.

Ah okay, I'll look into it.

> > +	switch (cmd) {
> > +{%- for method in interface_cb.methods %}
> > +	case CMD_{{method.mojom_name|upper}}: {
> > +		{{method.mojom_name}}IPC(vec, fds);
> > +		break;
> > +	}
> > +{%- endfor %}
> 
> Should we log unknown commands ?

Oh maybe.

> > +	}
> > +}
> > +{%- endif %}
> > +
> > +{% for method in interface_main.methods %}
> > +{{proxy_funcs.func_sig(proxy_name, method)}}
> > +{
> > +	if (isolate_)
> > +		{{"return " if method|method_return_value != "void"}}{{method.mojom_name}}IPC(
> > +{%- for param in method|method_param_names -%}
> > +		{{param}}{{- ", " if not loop.last}}
> > +{%- endfor -%}
> > +);
> > +	else
> > +		{{"return " if method|method_return_value != "void"}}{{method.mojom_name}}Thread(
> > +{%- for param in method|method_param_names -%}
> > +		{{param}}{{- ", " if not loop.last}}
> > +{%- endfor -%}
> > +);
> > +}
> > +
> > +{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> > +{
> > +{%- if method.mojom_name == "init" %}
> > +	{{proxy_funcs.init_thread_body()}}
> > +{% elif method.mojom_name == "start" %}
> 
> I think you're missing a - before elif, here and below.

This, I don't think so.

> > +	{{proxy_funcs.start_thread_body()}}
> > +{% elif method.mojom_name == "stop" %}
> > +	{{proxy_funcs.stop_thread_body()}}
> 
> Instead of using macros, would it make sense to move the code to the
> base IPAProxy class ?

Maybe...

> > +{% elif not method|is_async %}
> > +	ipa_->{{method.mojom_name}}(
> > +	{%- for param in method|method_param_names -%}
> > +		{{param}}{{- ", " if not loop.last}}
> > +	{%- endfor -%}
> > +);
> > +{% elif method|is_async %}
> > +	proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,
> > +	{%- for param in method|method_param_names -%}
> > +		{{param}}{{- ", " if not loop.last}}
> > +	{%- endfor -%}
> > +);
> > +{%- endif %}
> > +}
> > +
> > +{{proxy_funcs.func_sig(proxy_name, method, "IPC")}}
> > +{
> > +{%- set has_input = true if method|method_param_inputs|length > 0 %}
> > +{%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != "void" %}
> > +{% if has_input %}
> 
> Shouldn't it be {%- ?

Probably... I don't remember if this specific one caused other problems.

> > +	std::vector<uint8_t> _ipcInputBuf;
> > +{%- if method|method_input_has_fd %}
> > +	std::vector<int32_t> _ipcInputFds;
> > +{%- endif %}
> > +{%- endif %}
> > +{%- if has_output %}
> > +	std::vector<uint8_t> _ipcOutputBuf;
> > +{%- if method|method_output_has_fd %}
> > +	std::vector<int32_t> _ipcOutputFds;
> > +{%- endif %}
> > +{%- endif %}
> > +
> > +{{proxy_funcs.serialize_call(method|method_param_inputs, '_ipcInputBuf', '_ipcInputFds', base_controls)}}
> > +
> > +{%- set input_buf = "_ipcInputBuf" if has_input else "{}" %}
> > +{%- set fds_buf = "_ipcInputFds" if method|method_input_has_fd else "{}" %}
> > +{%- set cmd = "CMD_" + method.mojom_name|upper %}
> > +{% if method|is_async %}
> > +	int ret = ipc_->sendAsync({{cmd}}, {{input_buf}}, {{fds_buf}});
> > +{%- else %}
> > +	int ret = ipc_->sendSync({{cmd}}, {{input_buf}}, {{fds_buf}}
> > +{{- ", &_ipcOutputBuf" if has_output -}}
> > +{{- ", &_ipcOutputFds" if has_output and method|method_output_has_fd -}}
> > +);
> > +{%- endif %}
> > +	if (ret < 0) {
> > +		LOG(IPAProxy, Error) << "Failed to call {{method.mojom_name}}";
> > +{%- if method|method_return_value != "void" %}
> > +		return static_cast<{{method|method_return_value}}>(ret);
> > +{%- else %}
> > +		return;
> > +{%- endif %}
> > +	}
> > +	LOG(IPAProxy, Debug) << "Done calling {{method.mojom_name}}";
> 
> Is this message helpful for debugging ? I think we should work at some
> point on a tracing mechanism instead of manually tracing calls.

It was while I wasn't sure the calls were going through. I think at this
point it's stable enough that we can remove it.

Yeah tracing would be nice :)

> > +{% if method|method_return_value != "void" %}
> > +	return IPADataSerializer<{{method.response_parameters|first|name}}>::deserialize(_ipcOutputBuf, 0);
> > +{% elif method|method_param_outputs|length > 0 %}
> > +{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf', '_ipcOutputFds')}}
> > +{% endif -%}
> > +}
> > +
> > +{% endfor %}
> > +
> > +
> 
> One blank line should be enough.

Ah, right.

> > +{% for method in interface_cb.methods %}
> > +{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> > +{
> > +	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> > +}
> > +
> > +void {{proxy_name}}::{{method.mojom_name}}IPC(
> > +	std::vector<uint8_t> &data,
> > +	[[maybe_unused]] std::vector<int32_t> &fds)
> > +{ 
> > +{%- for param in method.parameters %}
> > +	{{param|name}} {{param.mojom_name}};
> > +{%- endfor %}
> > +{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false)}}
> > +	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> > +}
> > +{% endfor %}
> > +
> > +{%- if has_namespace %}
> > +{% for ns in namespace|reverse %}
> > +} /* {{ns}} */
> > +{% endfor %}
> > +{%- endif %}
> > +} /* namespace libcamera */
> > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > new file mode 100644
> > index 00000000..7d2bfb6c
> > --- /dev/null
> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > @@ -0,0 +1,117 @@
> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> > +
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) {{ year }}, Google Inc.
> > + *
> > + * ipa_proxy_{{ module_name }}.h - Image Processing Algorithm proxy for {{ module_name }}
> > + *
> > + * This file is auto-generated. Do not edit.
> > + */
> > +
> > +#ifndef __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__
> > +#define __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__
> > +
> > +#include <libcamera/ipa/ipa_interface.h>
> > +#include <libcamera/ipa/{{module_name}}.h>
> > +#include <libcamera/ipa/{{module_name}}_generated.h>
> > +
> > +#include "libcamera/internal/control_serializer.h"
> > +#include "libcamera/internal/ipa_ipc.h"
> > +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> > +#include "libcamera/internal/ipa_proxy.h"
> > +#include "libcamera/internal/ipc_unixsocket.h"
> > +#include "libcamera/internal/thread.h"
> > +
> > +namespace libcamera {
> > +{%- if has_namespace %}
> > +{% for ns in namespace %}
> > +namespace {{ns}} {
> > +{% endfor %}
> > +{%- endif %}
> > +
> > +class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
> > +{
> > +public:
> > +	{{proxy_name}}(IPAModule *ipam, bool isolate);
> > +	~{{proxy_name}}();
> > +
> > +{% for method in interface_main.methods %}
> > +{{proxy_funcs.func_sig(proxy_name, method, "", false, true)|indent(8, true)}};
> > +{% endfor %}
> > +
> > +{%- for method in interface_cb.methods %}
> > +	Signal<
> > +{%- for param in method.parameters -%}
> > +		const {{param|name}}{{" &" if not param|is_pod}}
> > +		{{- ", " if not loop.last }}
> > +{%- endfor -%}
> > +> {{method.mojom_name}};
> > +{% endfor %}
> > +
> > +private:
> > +	void recvIPC(std::vector<uint8_t> &data, std::vector<int32_t> &fds);
> > +
> > +{% for method in interface_main.methods %}
> > +{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> > +{{proxy_funcs.func_sig(proxy_name, method, "IPC", false)|indent(8, true)}};
> > +{% endfor %}
> > +{% for method in interface_cb.methods %}
> > +{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> > +	void {{method.mojom_name}}IPC(
> > +		std::vector<uint8_t> &data,
> > +		std::vector<int32_t> &fds);
> 
> Should these be const ?

Yeah.

> > +{% endfor %}
> > +
> > +	/* Helper class to invoke async functions in another thread. */
> > +	class ThreadProxy : public Object
> > +	{
> > +	public:
> > +		void setIPA({{interface_name}} *ipa)
> > +		{
> > +			ipa_ = ipa;
> > +		}
> > +
> > +		int start()
> > +		{
> > +			return ipa_->start();
> > +		}
> > +
> > +		void stop()
> > +		{
> > +			ipa_->stop();
> > +		}
> > +{% for method in interface_main.methods %}
> > +{%- if method|is_async %}
> > +		{{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
> > +		{
> > +			ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> > +		}
> > +{%- endif %}
> > +{%- endfor %}
> 
> I think this is mixing two concepts, which shouldn't cause any issue for
> now, but may come bite us back later.
> 
> As you've noticed in the existing IPAProxyThread implementation, we call
> some methods directly, and proxy other methods across threads. The idea
> is that anything called before start() is a direct call, and anything
> from start() until but not including stop() is indirect. The reason is
> that before start() there's no thread running, so we can't make a
> cross-thread call.

Ah, so sync vs async is different from non-thread call vs thread call...

> One direct consequence of this architecture is that all calls before
> start() are synchronous, and can thus return a value. Calls after
> start() are asynchronous, but that's a separate design decision. We
> could synchronize cross-thread calls if we wanted, we have chosen not to
> in order to avoid delays that could damage the real time performance of
> the pipeline handler.
> 
> With IPC the situation is different, all calls are cross-process. Still,
> we want to expose a similar behaviour to pipeline handlers, so calls
> before start() are synchronous, and further calls are asynchronous.
> 
> I don't think we can easily enforce this through the IDL, but I believe
> it would make sense to document this requirements, so that authors will
> not create an async method meant to be called before start(), or a
> synchronous method after start().

Yeah, I think documentation should be sufficient.

> > +
> > +	private:
> > +		{{interface_name}} *ipa_;
> > +	};
> > +
> > +	bool running_;
> > +	Thread thread_;
> > +	ThreadProxy proxy_;
> > +	std::unique_ptr<{{interface_name}}> ipa_;
> > +
> > +	const bool isolate_;
> > +
> > +	std::unique_ptr<IPAIPCUnixSocket> ipc_;
> > +
> > +	ControlSerializer controlSerializer_;
> > +};
> > +
> > +{%- if has_namespace %}
> > +{% for ns in namespace|reverse %}
> > +} /* {{ns}} */
> > +{% endfor %}
> > +{%- endif %}
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__ */
> > 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
> > new file mode 100644
> > index 00000000..1647a9ca
> > --- /dev/null
> > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > @@ -0,0 +1,184 @@
> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> > +
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) {{ year }}, Google Inc.
> > + *
> > + * ipa_proxy_{{ module_name }}_worker.cpp - Image Processing Algorithm proxy worker for {{ module_name }}
> > + *
> > + * This file is auto-generated. Do not edit.
> > + */
> > +
> > +#include <algorithm>
> > +#include <iostream>
> > +#include <sys/types.h>
> > +#include <tuple>
> > +#include <unistd.h>
> > +
> > +#include <libcamera/event_dispatcher.h>
> > +#include <libcamera/ipa/ipa_interface.h>
> > +#include <libcamera/ipa/{{module_name}}_generated.h>
> > +#include <libcamera/ipa/{{module_name}}_serializer.h>
> > +#include <libcamera/logging.h>
> > +
> > +#include "libcamera/internal/camera_sensor.h"
> > +#include "libcamera/internal/control_serializer.h"
> > +#include "libcamera/internal/ipa_data_serializer.h"
> > +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> > +#include "libcamera/internal/ipa_module.h"
> > +#include "libcamera/internal/ipa_proxy.h"
> > +#include "libcamera/internal/ipc_unixsocket.h"
> > +#include "libcamera/internal/log.h"
> > +#include "libcamera/internal/thread.h"
> > +
> > +using namespace libcamera;
> > +
> > +LOG_DEFINE_CATEGORY({{proxy_name}}Worker)
> > +
> > +{%- if has_namespace %}
> > +{% for ns in namespace -%}
> > +using namespace {{ns}};
> > +{% endfor %}
> > +{%- endif %}
> > +
> > +struct CallData {
> > +	IPCUnixSocket::Payload *response;
> > +	bool done;
> > +};
> > +
> > +{{interface_name}} *ipa_;
> > +IPCUnixSocket socket_;
> > +std::map<uint32_t, struct CallData> callData_;
> 
> callData_ doesn't seem to be used (and thus struct CallData shouldn't be
> needed).

Oops, remnant of copying from UnixSocket.

> > +
> > +ControlSerializer controlSerializer_;
> > +
> > +bool exit_ = false;
> 
> Should you group all this, as well as the functions below, in a
> {{module_name}}ProxyWorker class ?

I could. It's standalone so I didn't see much point in doing so though.

> > +
> > +void readyRead(IPCUnixSocket *socket)
> > +{
> > +	IPCUnixSocket::Payload _message, _response;
> > +	int _ret = socket->receive(&_message);
> > +	if (_ret) {
> > +		LOG({{proxy_name}}Worker, Error)
> > +			<< "Receive message failed" << _ret;
> > +		return;
> > +	}
> > +
> > +	uint32_t _cmd, _seq;
> > +	std::tie(_cmd, _seq) = IPAIPCUnixSocket::readHeader(_message);
> > +	IPAIPCUnixSocket::eraseHeader(_message);
> 
> Using Span would help avoiding copies.

Ah...

> > +
> > +	switch (_cmd) {
> > +	case CMD_EXIT: {
> > +		exit_ = true;
> > +		break;
> > +	}
> > +
> > +{% for method in interface_main.methods %}
> > +	case CMD_{{method.mojom_name|upper}}: {
> > +	{{proxy_funcs.deserialize_call(method|method_param_inputs, '_message.data', '_message.fds', false, true)|indent(8, true)}}
> > +{% for param in method|method_param_outputs %}
> > +		{{param|name}} {{param.mojom_name}};
> > +{% endfor %}
> > +{%- if method|method_return_value != "void" %}
> > +		{{method|method_return_value}} _callRet = ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> > +{%- else %}
> > +		ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}}
> > +{{- ", " if method|method_param_outputs|params_comma_sep -}}
> > +{%- for param in method|method_param_outputs -%}
> > +&{{param.mojom_name}}{{", " if not loop.last}}
> > +{%- endfor -%}
> > +);
> > +{%- endif %}
> > +{% if not method|is_async %}
> > +		IPAIPCUnixSocket::writeHeader(_response, _cmd, _seq);
> > +{%- if method|method_return_value != "void" %}
> > +		std::vector<uint8_t> _callRetBuf;
> > +		std::tie(_callRetBuf, std::ignore) =
> > +			IPADataSerializer<{{method|method_return_value}}>::serialize(_callRet);
> > +		_response.data.insert(_response.data.end(), _callRetBuf.begin(), _callRetBuf.end());
> > +{%- else %}
> > +	{{proxy_funcs.serialize_call(method|method_param_outputs, "_response.data", "_response.fds", base_controls)|indent(8, true)}}
> > +{%- endif %}
> > +		int _ret = socket_.send(_response);
> > +		if (_ret < 0) {
> > +			LOG({{proxy_name}}Worker, Error)
> > +				<< "Reply to {{method.mojom_name}}() failed" << _ret;
> > +		}
> > +		LOG({{proxy_name}}Worker, Debug) << "Done replying to {{method.mojom_name}}()";
> > +{%- endif %}
> > +		break;
> > +	}
> > +{% endfor %}
> 
> This isn't a huge deal, but the generated code my be a bit clearer if
> each case called a function.

It does! ipa_->function(); :) It just comes with deserialization
beforehand and serialization afterward for synchronous calls. I think
it's good enough...

> > +	}
> > +}
> > +
> > +{% for method in interface_cb.methods %}
> > +{{proxy_funcs.func_sig(proxy_name, method, "", false)}}
> > +{
> > +	IPCUnixSocket::Payload _message;
> > +
> > +	IPAIPCUnixSocket::writeHeader(_message, CMD_{{method.mojom_name|upper}}, 0);
> > +	{{proxy_funcs.serialize_call(method|method_param_inputs, "_message.data", "_message.fds", base_controls)}}
> > +
> > +	socket_.send(_message);
> > +
> > +	LOG({{proxy_name}}Worker, Debug) << "{{method.mojom_name}} done";
> > +}
> > +{%- endfor %}
> 
> You can drop the -

Yeah, this one needs to be dropped.

> > +
> > +int main(int argc, char **argv)
> > +{
> > +	/* Uncomment this for debugging. */
> > +#if 0
> > +	std::string logPath = "/tmp/libcamera.worker." +
> > +			      std::to_string(getpid()) + ".log";
> > +	logSetFile(logPath.c_str());
> > +#endif
> > +
> > +	if (argc < 3) {
> > +		LOG({{proxy_name}}Worker, Debug)
> 
> Maybe s/Debug/Error/ ?

Ah yes.

> > +			<< "Tried to start worker with no args";
> > +		return EXIT_FAILURE;
> > +	}
> > +
> > +	int fd = std::stoi(argv[2]);
> > +	LOG({{proxy_name}}Worker, Debug)
> 
> And Info ?

Yeah that's better too.

> > +		<< "Starting worker for IPA module " << argv[1]
> > +		<< " with IPC fd = " << fd;
> > +
> > +	std::unique_ptr<IPAModule> ipam = std::make_unique<IPAModule>(argv[1]);
> > +	if (!ipam->isValid() || !ipam->load()) {
> > +		LOG({{proxy_name}}Worker, Error)
> > +			<< "IPAModule " << argv[1] << " should be valid but isn't";
> 
> Maybe just " isn't valid" ?

ack

> > +		return EXIT_FAILURE;
> > +	}
> > +
> > +	if (socket_.bind(fd) < 0) {
> > +		LOG({{proxy_name}}Worker, Error) << "IPC socket binding failed";
> > +		return EXIT_FAILURE;
> > +	}
> > +	socket_.readyRead.connect(&readyRead);
> > +
> > +	ipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());
> > +	if (!ipa_) {
> > +		LOG({{proxy_name}}Worker, Error) << "Failed to create IPA interface instance";
> > +		return EXIT_FAILURE;
> > +	}
> > +{% for method in interface_cb.methods %}
> > +	ipa_->{{method.mojom_name}}.connect(&{{method.mojom_name}});
> > +{%- endfor %}
> > +
> > +	LOG({{proxy_name}}Worker, Debug) << "Proxy worker successfully started";
> > +
> > +	/* \todo upgrade listening loop */
> 
> Good point. We should make an EventLoop class at some point.

I'm pretty sure I copied this from somewhere... I can't remember where.

> > +	EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
> > +	while (!exit_)
> > +		dispatcher->processEvents();
> > +
> > +	delete ipa_;
> > +	socket_.close();
> > +
> > +	return 0;
> > +}
> > diff --git a/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl
> > new file mode 100644
> > index 00000000..37352c1f
> > --- /dev/null
> > +++ b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl
> > @@ -0,0 +1,44 @@
> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > +{%- import "serializer.tmpl" as serializer -%}
> > +
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) {{ year }}, Google Inc.
> > + *
> > + * {{ module_name }}_serializer.h - Image Processing Algorithm data serializer for {{ module_name }}
> > + *
> > + * This file is auto-generated. Do not edit.
> > + */
> > +
> > +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__
> > +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__
> > +
> > +#include <libcamera/ipa/{{ module_name }}.h>
> > +#include <libcamera/ipa/{{ module_name }}_generated.h>
> > +
> > +#include "libcamera/internal/control_serializer.h"
> > +#include "libcamera/internal/ipa_data_serializer.h"
> > +
> > +#include <tuple>
> > +#include <vector>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(IPADataSerializer)
> > +{% for struct in structs_nonempty %}
> > +template<>
> > +class IPADataSerializer<{{ struct|name_full(namespace_str) }}>
> > +{
> > +public:
> > +{{- serializer.serializer(struct, base_controls, namespace_str) }}
> > +{%- if struct|has_fd %}
> > +{{ serializer.deserializer_fd(struct, namespace_str) }}
> > +{%- else %}
> > +{{ serializer.deserializer_no_fd(struct, namespace_str) }}
> > +{%- endif %}
> > +};
> > +{% endfor %}
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__ */
> 
> I'll review proxy_functions.tmpl and serializer.tmpl separately, this is
> growing quite big.

Okay. Thank you for the reviews so far.


Paul

> > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > new file mode 100644
> > index 00000000..e1f5a20c
> > --- /dev/null
> > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > @@ -0,0 +1,185 @@
> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > +{#
> > + # \brief Generate fuction prototype
> > + #
> > + # \param class Class name
> > + # \param method mojom Method object
> > + # \param suffix Suffix to append to \a method function name
> > + # \param need_class_name True to generate class name with function
> > + # \param override True to generate override tag after the function prototype
> > + #}
> > +{%- macro func_sig(class, method, suffix, need_class_name = true, override = false) -%}
> > +{{method|method_return_value}} {{ class + "::" if need_class_name }}{{method.mojom_name}}{{suffix}}(
> > +{%- for param in method|method_parameters %}
> > +	{{param}}{{- "," if not loop.last}}
> > +{%- endfor -%}
> > +){{" override" if override}}
> > +{%- endmacro -%}
> > +
> > +{#
> > + # \brief Generate function body for IPA init() function for thread
> > + #}
> > +{%- macro init_thread_body() -%}
> > +	int ret = ipa_->init(settings);
> > +	if (ret)
> > +		return ret;
> > +
> > +	proxy_.moveToThread(&thread_);
> > +
> > +	return 0;
> > +{%- endmacro -%}
> > +
> > +{#
> > + # \brief Generate function body for IPA start() function for thread
> > + #}
> > +{%- macro start_thread_body() -%}
> > +	running_ = true;
> > +	thread_.start();
> > +
> > +	return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);
> > +{%- endmacro -%}
> > +
> > +{#
> > + # \brief Generate function body for IPA stop() function for thread
> > + #}
> > +{%- macro stop_thread_body() -%}
> > +	if (!running_)
> > +		return;
> > +
> > +	running_ = false;
> > +
> > +	proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
> > +
> > +	thread_.exit();
> > +	thread_.wait();
> > +{%- endmacro -%}
> > +
> > +
> > +{#
> > + # \brief Serialize multiple objects into data buffer and fd vector
> > + #
> > + # Generate code to serialize multiple objects, as specified in \a params
> > + # (which are the parameters to some function), into \a buf data buffer and
> > + # \a fds fd vector.
> > + # This code is meant to be used by the proxy, for serializing prior to IPC calls.
> > + #}
> > +{%- macro serialize_call(params, buf, fds, base_controls) %}
> > +{%- for param in params %}
> > +	std::vector<uint8_t> {{param.mojom_name}}Buf;
> > +{%- if param|has_fd %}
> > +	std::vector<int32_t> {{param.mojom_name}}Fds;
> > +	std::tie({{param.mojom_name}}Buf, {{param.mojom_name}}Fds) =
> > +{%- else %}
> > +	std::tie({{param.mojom_name}}Buf, std::ignore) =
> > +{%- endif %}
> > +{%- if param|is_controls %}
> > +		IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}, {{param.mojom_name}}.infoMap() ? *{{param.mojom_name}}.infoMap() : {{base_controls}}
> > +{%- else %}
> > +		IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}
> > +{%- endif %}
> > +{{- ", &controlSerializer_" if param|needs_control_serializer -}}
> > +);
> > +{%- endfor %}
> > +
> > +{%- if params|length > 1 %}
> > +{%- for param in params %}
> > +	appendUInt<uint32_t>({{buf}}, {{param.mojom_name}}Buf.size());
> > +{%- if param|has_fd %}
> > +	appendUInt<uint32_t>({{buf}}, {{param.mojom_name}}Fds.size());
> > +{%- endif %}
> > +{%- endfor %}
> > +{%- endif %}
> > +
> > +{%- for param in params %}
> > +	{{buf}}.insert({{buf}}.end(), {{param.mojom_name}}Buf.begin(), {{param.mojom_name}}Buf.end());
> > +{%- endfor %}
> > +
> > +{%- for param in params %}
> > +{%- if param|has_fd %}
> > +	{{fds}}.insert({{fds}}.end(), {{param.mojom_name}}Fds.begin(), {{param.mojom_name}}Fds.end());
> > +{%- endif %}
> > +{%- endfor %}
> > +{%- endmacro -%}
> > +
> > +
> > +{#
> > + # \brief Deserialize a single object from data buffer and fd vector
> > + #
> > + # \param pointer True deserializes the object into a dereferenced pointer
> > + #
> > + # Generate code to deserialize a single object, as specified in \a param,
> > + # from \a buf data buffer and \a fds fd vector.
> > + # This code is meant to be used by macro deserialize_call.
> > + #}
> > +{%- macro deserialize_param(param, pointer, loop, buf, fds) -%}
> > +{{"*" if pointer}}{{param.mojom_name}} = IPADataSerializer<{{param|name}}>::deserialize(
> > +	{{buf}}.begin() + {{param.mojom_name}}Start,
> > +{%- if loop.last %}
> > +	{{buf}}.end()
> > +{%- else %}
> > +	{{buf}}.begin() + {{param.mojom_name}}Start + {{param.mojom_name}}BufSize
> > +{%- endif -%}
> > +{{- "," if param|has_fd }}
> > +{%- if param|has_fd %}
> > +	{{fds}}.begin() + {{param.mojom_name}}FdStart,
> > +{%- if loop.last %}
> > +	{{fds}}.end()
> > +{%- else %}
> > +	{{fds}}.begin() + {{param.mojom_name}}FdStart + {{param.mojom_name}}FdsSize
> > +{%- endif -%}
> > +{%- endif -%}
> > +{{- "," if param|needs_control_serializer }}
> > +{%- if param|needs_control_serializer %}
> > +	&controlSerializer_
> > +{%- endif -%}
> > +);
> > +{%- endmacro -%}
> > +
> > +
> > +{#
> > + # \brief Deserialize multiple objects from data buffer and fd vector
> > + #
> > + # \param pointer True deserializes objects into pointers, and adds a null check.
> > + # \param declare True declares the objects in addition to deserialization.
> > + #
> > + # Generate code to deserialize multiple objects, as specified in \a params
> > + # (which are the parameters to some function), from \a buf data buffer and
> > + # \a fds fd vector.
> > + # This code is meant to be used by the proxy, for serializing prior to IPC calls.
> > + #}
> > +{%- macro deserialize_call(params, buf, fds, pointer = true, declare = false) -%}
> > +{% set ns = namespace(size_offset = 0) %}
> > +{%- if params|length > 1 %}
> > +{%- for param in params %}
> > +	[[maybe_unused]] size_t {{param.mojom_name}}BufSize = readUInt<uint32_t>({{buf}}, {{ns.size_offset}});
> > +	{%- set ns.size_offset = ns.size_offset + 4 %}
> > +{%- if param|has_fd %}
> > +	[[maybe_unused]] size_t {{param.mojom_name}}FdsSize = readUInt<uint32_t>({{buf}}, {{ns.size_offset}});
> > +	{%- set ns.size_offset = ns.size_offset + 4 %}
> > +{%- endif %}
> > +{%- endfor %}
> > +{%- endif %}
> > +{% for param in params %}
> > +{%- if loop.first %}
> > +	size_t {{param.mojom_name}}Start = {{ns.size_offset}};
> > +{%- else %}
> > +	size_t {{param.mojom_name}}Start = {{loop.previtem.mojom_name}}Start + {{loop.previtem.mojom_name}}BufSize;
> > +{%- endif %}
> > +{%- endfor %}
> > +{% for param in params|with_fds %}
> > +{%- if loop.first %}
> > +	size_t {{param.mojom_name}}FdStart = 0;
> > +{%- elif not loop.last %}
> > +	size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
> > +{%- endif %}
> > +{%- endfor %}
> > +{% for param in params %}
> > +	{%- if pointer %}
> > +	if ({{param.mojom_name}}) {
> > +{{deserialize_param(param, pointer, loop, buf, fds)|indent(16, True)}}
> > +	}
> > +	{%- else %}
> > +	{{param|name + " " if declare}}{{deserialize_param(param, pointer, loop, buf, fds)|indent(8)}}
> > +	{%- endif %}
> > +{% endfor %}
> > +{%- endmacro -%}
> > diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> > new file mode 100644
> > index 00000000..3c071c4e
> > --- /dev/null
> > +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> > @@ -0,0 +1,276 @@
> > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > +{# Turn this into a C macro? #}
> > +{#
> > + # \brief Verify that there is enough bytes to deserialize
> > + #
> > + # Generate code that verifies that \a size is not greater than \a dataSize.
> > + # Otherwise log an error with \a name and \a typename.
> > + #}
> > +{%- macro check_data_size(size, dataSize, name, typename) %}
> > +		if ({{size}} > {{dataSize}}) {
> > +			LOG(IPADataSerializer, Error)
> > +				<< "Failed to deserialize {{name}}: not enough {{typename}}, expected "
> > +				<< ({{size}}) << ", got " << ({{dataSize}});
> > +			return ret;
> > +		}
> > +{%- endmacro %}
> > +
> > +
> > +{#
> > + # \brief Serialize some field into return vector
> > + #
> > + # Generate code to serialize \a field into ret_data, including size of the
> > + # field and fds (where appropriate). \a base_controls indicates the
> > + # default ControlInfoMap in the event that the ControlList does not have one.
> > + # This code is meant to be used by the IPADataSerializer specialization.
> > + #}
> > +{%- macro serializer_field(field, base_controls, namespace, loop) %}
> > +{%- if field|is_pod or field|is_enum %}
> > +		std::vector<uint8_t> {{field.mojom_name}};
> > +		std::tie({{field.mojom_name}}, std::ignore) =
> > +	{%- if field|is_pod %}
> > +			IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);
> > +	{%- elif field|is_enum %}
> > +			IPADataSerializer<uint{{field|bit_width}}_t>::serialize(data.{{field.mojom_name}}_);
> > +	{%- endif %}
> > +		ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > +{%- elif field|is_fd %}
> > +		std::vector<uint8_t> {{field.mojom_name}};
> > +		std::vector<int32_t> {{field.mojom_name}}Fds;
> > +		std::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =
> > +			IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);
> > +		ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > +		ret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());
> > +{%- elif field|is_controls %}
> > +		if (data.{{field.mojom_name}}_.size() > 0) {
> > +			std::vector<uint8_t> {{field.mojom_name}};
> > +			std::tie({{field.mojom_name}}, std::ignore) =
> > +				IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_,
> > +					data.{{field.mojom_name}}_.infoMap() ? *data.{{field.mojom_name}}_.infoMap() : {{base_controls}},
> > +					cs);
> > +			appendUInt<uint32_t>(ret_data, {{field.mojom_name}}.size());
> > +			ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > +		} else {
> > +			appendUInt<uint32_t>(ret_data, 0);
> > +		}
> > +{%- elif field|is_plain_struct or field|is_array or field|is_map %}
> > +		std::vector<uint8_t> {{field.mojom_name}};
> > +	{%- if field|has_fd %}
> > +		std::vector<int32_t> {{field.mojom_name}}Fds;
> > +		std::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =
> > +	{%- else %}
> > +		std::tie({{field.mojom_name}}, std::ignore) =
> > +	{%- endif %}
> > +	{%- if field|is_array or field|is_map %}
> > +			IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_, cs);
> > +	{%- else %}
> > +			IPADataSerializer<{{field|name_full(namespace)}}>::serialize(data.{{field.mojom_name}}_, cs);
> > +	{%- endif %}
> > +		appendUInt<uint32_t>(ret_data, {{field.mojom_name}}.size());
> > +	{%- if field|has_fd %}
> > +		appendUInt<uint32_t>(ret_data, {{field.mojom_name}}Fds.size());
> > +	{%- endif %}
> > +		ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > +	{%- if field|has_fd %}
> > +		ret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());
> > +	{%- endif %}
> > +{%- else %}
> > +		/* Unknown serialization for {{field.mojom_name}}. */
> > +{%- endif %}
> > +{%- endmacro %}
> > +
> > +
> > +{#
> > + # \brief Deserialize some field into return struct
> > + #
> > + # Generate code to deserialize \a field into object ret.
> > + # This code is meant to be used by the IPADataSerializer specialization.
> > + #}
> > +{%- macro deserializer_field(field, namespace, loop) %}
> > +{% if field|is_pod or field|is_enum %}
> > +	{%- set field_size = (field|bit_width|int / 8)|int %}
> > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> > +		{%- if field|is_pod %}
> > +		ret.{{field.mojom_name}}_ = static_cast<{{field|name}}>(IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}));
> > +		{%- else %}
> > +		ret.{{field.mojom_name}}_ = static_cast<{{field|name}}>(IPADataSerializer<uint{{field|bit_width}}_t>::deserialize(m, m + {{field_size}}));
> > +		{%- endif %}
> > +	{%- if not loop.last %}
> > +		m += {{field_size}};
> > +		dataSize -= {{field_size}};
> > +	{%- endif %}
> > +{% elif field|is_fd %}
> > +	{%- set field_size = 1 %}
> > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> > +		ret.{{field.mojom_name}}_ = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs);
> > +	{%- if not loop.last %}
> > +		m += {{field_size}};
> > +		dataSize -= {{field_size}};
> > +		n += ret.{{field.mojom_name}}_.isValid() ? 1 : 0;
> > +		fdsSize -= ret.{{field.mojom_name}}_.isValid() ? 1 : 0;
> > +	{%- endif %}
> > +{% elif field|is_controls %}
> > +	{%- set field_size = 4 %}
> > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}
> > +		size_t {{field.mojom_name}}Size = readUInt<uint32_t>(m);
> > +	{%- set field_size = '4 + ' + field.mojom_name + 'Size' -%}
> > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> > +		if ({{field.mojom_name}}Size > 0)
> > +			ret.{{field.mojom_name}}_ =
> > +				IPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> > +	{%- if not loop.last %}
> > +		m += {{field_size}};
> > +		dataSize -= {{field_size}};
> > +	{%- endif %}
> > +{% elif field|is_plain_struct or field|is_array or field|is_map%}
> > +	{%- set field_size = 4 %}
> > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}
> > +		size_t {{field.mojom_name}}Size = readUInt<uint32_t>(m);
> > +	{%- if field|has_fd %}
> > +	{%- set field_size = 8 %}
> > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'FdsSize', 'data') }}
> > +		size_t {{field.mojom_name}}FdsSize = readUInt<uint32_t>(m + 4);
> > +		{{- check_data_size(field.mojom_name + 'FdsSize', 'fdsSize', field.mojom_name, 'fds') }}
> > +	{%- endif %}
> > +	{%- set field_size = field|has_fd|choose('8 + ', '4 + ') + field.mojom_name + 'Size' -%}
> > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> > +		ret.{{field.mojom_name}}_ =
> > +	{%- if field|has_fd and (field|is_array or field|is_map) %}
> > +			IPADataSerializer<{{field|name}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);
> > +	{%- elif field|has_fd and (not (field|is_array or field|is_map)) %}
> > +			IPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);
> > +	{%- elif (not field|has_fd) and (field|is_array or field|is_map) %}
> > +			IPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> > +	{%- else %}
> > +			IPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> > +	{%- endif %}
> > +	{%- if not loop.last %}
> > +		m += {{field_size}};
> > +		dataSize -= {{field_size}};
> > +	{%- if field|has_fd %}
> > +		n += {{field.mojom_name}}FdsSize;
> > +		fdsSize -= {{field.mojom_name}}FdsSize;
> > +	{%- endif %}
> > +	{%- endif %}
> > +{% else %}
> > +		/* Unknown deserialization for {{field.mojom_name}}. */
> > +{%- endif %}
> > +{%- endmacro %}
> > +
> > +
> > +{#
> > + # \brief Serialize a struct
> > + #
> > + # Generate code for IPADataSerializer specialization, for serializing
> > + # \a struct. \a base_controls indicates the default ControlInfoMap
> > + # in the event that the ControlList does not have one.
> > + #}
> > +{%- macro serializer(struct, base_controls, namespace) %}
> > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > +	serialize(const {{ struct|name_full(namespace) }} &data,
> > +{%- if struct|needs_control_serializer %}
> > +		  ControlSerializer *cs)
> > +{%- else %}
> > +		  [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +{%- endif %}
> > +	{
> > +		std::vector<uint8_t> ret_data;
> > +{%- if struct|has_fd %}
> > +		std::vector<int32_t> ret_fds;
> > +{%- endif %}
> > +{%- for field in struct.fields %}
> > +{{serializer_field(field, base_controls, namespace, loop)}}
> > +{%- endfor %}
> > +{% if struct|has_fd %}
> > +		return {ret_data, ret_fds};
> > +{%- else %}
> > +		return {ret_data, {}};
> > +{%- endif %}
> > +	}
> > +{%- endmacro %}
> > +
> > +
> > +{#
> > + # \brief Deserialize a struct that has fds
> > + #
> > + # Generate code for IPADataSerializer specialization, for deserializing
> > + # \a struct, in the case that \a struct has file descriptors.
> > + #}
> > +{%- macro deserializer_fd(struct, namespace) %}
> > +	static {{ struct|name_full(namespace) }}
> > +	deserialize(std::vector<uint8_t> &data,
> > +		    std::vector<int32_t> &fds,
> > +{%- if struct|needs_control_serializer %}
> > +		    ControlSerializer *cs)
> > +{%- else %}
> > +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +{%- endif %}
> > +	{
> > +		{{struct|name_full(namespace)}} ret;
> > +		std::vector<uint8_t>::iterator m = data.begin();
> > +		std::vector<int32_t>::iterator n = fds.begin();
> > +
> > +		size_t dataSize = data.size();
> > +		size_t fdsSize = fds.size();
> > +{%- for field in struct.fields -%}
> > +{{deserializer_field(field, namespace, loop)}}
> > +{%- endfor %}
> > +		return ret;
> > +	}
> > +
> > +	static {{ struct|name_full(namespace) }}
> > +	deserialize(std::vector<uint8_t>::iterator data_it1,
> > +		    std::vector<uint8_t>::iterator data_it2,
> > +		    std::vector<int32_t>::iterator fds_it1,
> > +		    std::vector<int32_t>::iterator fds_it2,
> > +{%- if struct|needs_control_serializer %}
> > +		    ControlSerializer *cs)
> > +{%- else %}
> > +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +{%- endif %}
> > +	{
> > +		std::vector<uint8_t> data(data_it1, data_it2);
> > +		std::vector<int32_t> fds(fds_it1, fds_it2);
> > +		return IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, fds, cs);
> > +	}
> > +{%- endmacro %}
> > +
> > +
> > +{#
> > + # \brief Deserialize a struct that has no fds
> > + #
> > + # Generate code for IPADataSerializer specialization, for deserializing
> > + # \a struct, in the case that \a struct does not have file descriptors.
> > + #}
> > +{%- macro deserializer_no_fd(struct, namespace) %}
> > +	static {{ struct|name_full(namespace) }}
> > +	deserialize(std::vector<uint8_t> &data,
> > +{%- if struct|needs_control_serializer %}
> > +		    ControlSerializer *cs)
> > +{%- else %}
> > +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +{%- endif %}
> > +	{
> > +		{{struct|name_full(namespace)}} ret;
> > +		std::vector<uint8_t>::iterator m = data.begin();
> > +
> > +		size_t dataSize = data.size();
> > +{%- for field in struct.fields -%}
> > +{{deserializer_field(field, namespace, loop)}}
> > +{%- endfor %}
> > +		return ret;
> > +	}
> > +
> > +	static {{ struct|name_full(namespace) }}
> > +	deserialize(std::vector<uint8_t>::iterator data_it1,
> > +		    std::vector<uint8_t>::iterator data_it2,
> > +{%- if struct|needs_control_serializer %}
> > +		    ControlSerializer *cs)
> > +{%- else %}
> > +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > +{%- endif %}
> > +	{
> > +		std::vector<uint8_t> data(data_it1, data_it2);
> > +		return IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, cs);
> > +	}
> > +{%- endmacro %}
> > diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py
> > new file mode 100644
> > index 00000000..a9269c82
> > --- /dev/null
> > +++ b/utils/ipc/generators/mojom_libcamera_generator.py
> > @@ -0,0 +1,461 @@
> > +'''Generates libcamera files from a mojom.Module.'''
> > +
> > +import argparse
> > +import ast
> > +import datetime
> > +import contextlib
> > +import os
> > +import re
> > +import shutil
> > +import sys
> > +import tempfile
> > +
> > +from jinja2 import contextfilter
> > +
> > +import mojom.fileutil as fileutil
> > +import mojom.generate.generator as generator
> > +import mojom.generate.module as mojom
> > +from mojom.generate.template_expander import UseJinja
> > +
> > +
> > +GENERATOR_PREFIX = 'libcamera'
> > +
> > +_kind_to_cpp_type = {
> > +    mojom.BOOL:   'bool',
> > +    mojom.INT8:   'int8_t',
> > +    mojom.UINT8:  'uint8_t',
> > +    mojom.INT16:  'int16_t',
> > +    mojom.UINT16: 'uint16_t',
> > +    mojom.INT32:  'int32_t',
> > +    mojom.UINT32: 'uint32_t',
> > +    mojom.FLOAT:  'float',
> > +    mojom.INT64:  'int64_t',
> > +    mojom.UINT64: 'uint64_t',
> > +    mojom.DOUBLE: 'double',
> > +}
> > +
> > +_bit_widths = {
> > +    mojom.BOOL:   '8',
> > +    mojom.INT8:   '8',
> > +    mojom.UINT8:  '8',
> > +    mojom.INT16:  '16',
> > +    mojom.UINT16: '16',
> > +    mojom.INT32:  '32',
> > +    mojom.UINT32: '32',
> > +    mojom.FLOAT:  '32',
> > +    mojom.INT64:  '64',
> > +    mojom.UINT64: '64',
> > +    mojom.DOUBLE: '64',
> > +}
> > +
> > +def ModuleName(path):
> > +    return path.split('/')[-1].split('.')[0]
> > +
> > +def ModuleClassName(module):
> > +    s = re.sub(r'^IPA', '',  module.interfaces[0].mojom_name)
> > +    return re.sub(r'Interface$', '', s)
> > +
> > +def UpperCamelCase(name):
> > +    return ''.join([x.capitalize() for x in generator.SplitCamelCase(name)])
> > +
> > +def CamelCase(name):
> > +    uccc = UpperCamelCase(name)
> > +    return uccc[0].lower() + uccc[1:]
> > +
> > +def ConstantStyle(name):
> > +    return generator.ToUpperSnakeCase(name)
> > +
> > +def Choose(cond, t, f):
> > +    return t if cond else f
> > +
> > +def CommaSep(l):
> > +    return ', '.join([m for m in l])
> > +
> > +def ParamsCommaSep(l):
> > +    return ', '.join([m.mojom_name for m in l])
> > +
> > +def GetDefaultValue(element):
> > +    if element.default is not None:
> > +        return element.default
> > +    if type(element.kind) == mojom.Kind:
> > +        return '0'
> > +    if mojom.IsEnumKind(element.kind):
> > +        return f'static_cast<{element.kind.mojom_name}>(0)'
> > +    if (isinstance(element.kind, mojom.Struct) and
> > +       element.kind.mojom_name == 'FileDescriptor'):
> > +        return '-1'
> > +    return ''
> > +
> > +def WithDefaultValues(element):
> > +    return [x for x in element if HasDefaultValue(x)]
> > +
> > +def WithFds(element):
> > +    return [x for x in element if HasFd(x)]
> > +
> > +def HasDefaultValue(element):
> > +    return GetDefaultValue(element) != ''
> > +
> > +def HasDefaultFields(element):
> > +    return True in [HasDefaultValue(x) for x in element.fields]
> > +
> > +def GetAllTypes(element):
> > +    if mojom.IsArrayKind(element):
> > +        return GetAllTypes(element.kind)
> > +    if mojom.IsMapKind(element):
> > +        return GetAllTypes(element.key_kind) + GetAllTypes(element.value_kind)
> > +    if isinstance(element, mojom.Parameter):
> > +        return GetAllTypes(element.kind)
> > +    if mojom.IsEnumKind(element):
> > +        return [element.mojom_name]
> > +    if not mojom.IsStructKind(element):
> > +        return [element.spec]
> > +    if len(element.fields) == 0:
> > +        return [element.mojom_name]
> > +    ret = [GetAllTypes(x.kind) for x in element.fields]
> > +    ret = [x for sublist in ret for x in sublist]
> > +    return list(set(ret))
> > +
> > +def GetAllAttrs(element):
> > +    if mojom.IsArrayKind(element):
> > +        return GetAllAttrs(element.kind)
> > +    if mojom.IsMapKind(element):
> > +        return {**GetAllAttrs(element.key_kind), **GetAllAttrs(element.value_kind)}
> > +    if isinstance(element, mojom.Parameter):
> > +        return GetAllAttrs(element.kind)
> > +    if mojom.IsEnumKind(element):
> > +        return element.attributes if element.attributes is not None else {}
> > +    if mojom.IsStructKind(element) and len(element.fields) == 0:
> > +        return element.attributes if element.attributes is not None else {}
> > +    if not mojom.IsStructKind(element):
> > +        return {}
> > +    attrs = [(x.attributes) for x in element.fields]
> > +    ret = {}
> > +    for d in attrs:
> > +        if d is not None:
> > +            ret = {**ret, **d}
> > +    return ret
> > +
> > +def NeedsControlSerializer(element):
> > +    types = GetAllTypes(element)
> > +    return "ControlList" in types or "ControlInfoMap" in types
> > +
> > +def HasFd(element):
> > +    attrs = GetAllAttrs(element)
> > +    if isinstance(element, mojom.Kind):
> > +        types = GetAllTypes(element)
> > +    else:
> > +        types = GetAllTypes(element.kind)
> > +    return "FileDescriptor" in types or (attrs is not None and "hasFd" in attrs)
> > +
> > +def MethodInputHasFd(method):
> > +    if len([x for x in method.parameters if HasFd(x)]) > 0:
> > +        return True
> > +    return False
> > +
> > +def MethodOutputHasFd(method):
> > +    if (MethodReturnValue(method) != 'void' or
> > +        method.response_parameters is None):
> > +        return False
> > +    if len([x for x in method.parameters if HasFd(x)]) > 0:
> > +        return True
> > +    return False
> > +
> > +def MethodParamInputs(method):
> > +    return method.parameters
> > +
> > +def MethodParamOutputs(method):
> > +    if (MethodReturnValue(method) != 'void' or
> > +        method.response_parameters is None):
> > +        return []
> > +    return method.response_parameters
> > +
> > +def MethodParamNames(method):
> > +    params = []
> > +    for param in method.parameters:
> > +        params.append(param.mojom_name)
> > +    if MethodReturnValue(method) == 'void':
> > +        if method.response_parameters is None:
> > +            return params
> > +        for param in method.response_parameters:
> > +            params.append(param.mojom_name)
> > +    return params
> > +
> > +def MethodParameters(method):
> > +    params = []
> > +    for param in method.parameters:
> > +        params.append('const %s %s%s' % (GetNameForElement(param),
> > +                                         ('&' if not IsPod(param) else ''),
> > +                                         param.mojom_name))
> > +    if MethodReturnValue(method) == 'void':
> > +        if method.response_parameters is None:
> > +            return params
> > +        for param in method.response_parameters:
> > +            params.append(f'{GetNameForElement(param)} *{param.mojom_name}')
> > +    return params
> > +
> > +def MethodReturnValue(method):
> > +    if method.response_parameters is None:
> > +        return 'void'
> > +    if len(method.response_parameters) == 1 and IsPod(method.response_parameters[0]):
> > +        return GetNameForElement(method.response_parameters[0])
> > +    return 'void'
> > +
> > +def IsAsync(method):
> > +    # callbacks are always async
> > +    if re.match("^IPA.*CallbackInterface$", method.interface.mojom_name):
> > +        return True
> > +    elif re.match("^IPA.*Interface$", method.interface.mojom_name):
> > +        if method.attributes is None:
> > +            return False
> > +        elif 'async' in method.attributes and method.attributes['async']:
> > +            return True
> > +    return False
> > +
> > +def IsArray(element):
> > +    return mojom.IsArrayKind(element.kind)
> > +
> > +def IsControls(element):
> > +    return mojom.IsStructKind(element.kind) and (element.kind.mojom_name == "ControlList" or
> > +                                                 element.kind.mojom_name == "ControlInfoMap")
> > +
> > +def IsEnum(element):
> > +    return mojom.IsEnumKind(element.kind)
> > +
> > +def IsFd(element):
> > +    return mojom.IsStructKind(element.kind) and element.kind.mojom_name == "FileDescriptor"
> > +
> > +def IsMap(element):
> > +    return mojom.IsMapKind(element.kind)
> > +
> > +def IsPlainStruct(element):
> > +    return mojom.IsStructKind(element.kind) and not IsControls(element) and not IsFd(element)
> > +
> > +def IsPod(element):
> > +    return element.kind in _kind_to_cpp_type
> > +
> > +def BitWidth(element):
> > +    if element.kind in _bit_widths:
> > +        return _bit_widths[element.kind]
> > +    if mojom.IsEnumKind(element.kind):
> > +        return '32'
> > +    return ''
> > +
> > +def GetNameForElement(element):
> > +    if (mojom.IsEnumKind(element) or
> > +        mojom.IsInterfaceKind(element) or
> > +        mojom.IsStructKind(element)):
> > +        return element.mojom_name
> > +    if (mojom.IsArrayKind(element)):
> > +        elem_name = GetNameForElement(element.kind)
> > +        return f'std::vector<{elem_name}>'
> > +    if (mojom.IsMapKind(element)):
> > +        key_name = GetNameForElement(element.key_kind)
> > +        value_name = GetNameForElement(element.value_kind)
> > +        return f'std::map<{key_name}, {value_name}>'
> > +    if isinstance(element, (mojom.Field, mojom.Method, mojom.Parameter)):
> > +        if (mojom.IsArrayKind(element.kind) or mojom.IsMapKind(element.kind)):
> > +            return GetNameForElement(element.kind)
> > +        if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):
> > +            return _kind_to_cpp_type[element.kind]
> > +        return element.kind.mojom_name
> > +    if isinstance(element,  mojom.EnumValue):
> > +        return (GetNameForElement(element.enum) + '.' +
> > +                ConstantStyle(element.name))
> > +    if isinstance(element, (mojom.NamedValue,
> > +                            mojom.Constant,
> > +                            mojom.EnumField)):
> > +        return ConstantStyle(element.name)
> > +    if (hasattr(element, '__hash__') and element in _kind_to_cpp_type):
> > +        return _kind_to_cpp_type[element]
> > +    if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):
> > +        return _kind_to_cpp_type[element.kind]
> > +    if (hasattr(element, 'spec')):
> > +        return element.spec.split(':')[-1]
> > +    if (mojom.IsInterfaceRequestKind(element) or
> > +        mojom.IsAssociatedKind(element) or
> > +        mojom.IsPendingRemoteKind(element) or
> > +        mojom.IsPendingReceiverKind(element) or
> > +        mojom.IsUnionKind(element)):
> > +        raise Exception('Unsupported element: %s' % element)
> > +    raise Exception('Unexpected element: %s' % element)
> > +
> > +def GetFullNameForElement(element, namespace_str):
> > +    name = GetNameForElement(element)
> > +    if namespace_str == '':
> > +        return name
> > +    return f'{namespace_str}::{name}'
> > +
> > +def ValidateZeroLength(l, s, cap=True):
> > +    if len(l) > 0:
> > +        raise Exception(f'{s.capitalize() if cap else s} should be empty')
> > +
> > +def ValidateSingleLength(l, s, cap=True):
> > +    if len(l) > 1:
> > +        raise Exception(f'Only one {s} allowed')
> > +    if len(l) < 1:
> > +        raise Exception(f'{s.capitalize() if cap else s} is required')
> > +
> > +def ValidateInterfaces(interfaces):
> > +    # Validate presence of main interface
> > +    intf = [x for x in interfaces
> > +            if re.match("^IPA.*Interface", x.mojom_name) and
> > +               not re.match("^IPA.*CallbackInterface", x.mojom_name)]
> > +    ValidateSingleLength(intf, 'main interface')
> > +
> > +    # Validate presence of callback interface
> > +    cb = [x for x in interfaces if re.match("^IPA.*CallbackInterface", x.mojom_name)]
> > +    ValidateSingleLength(cb, 'callback interface')
> > +
> > +    # Validate required main interface functions
> > +    intf = intf[0]
> > +    f_init  = [x for x in intf.methods if x.mojom_name == 'init']
> > +    f_start = [x for x in intf.methods if x.mojom_name == 'start']
> > +    f_stop  = [x for x in intf.methods if x.mojom_name == 'stop']
> > +
> > +    ValidateSingleLength(f_init, 'init()', False)
> > +    ValidateSingleLength(f_start, 'start()', False)
> > +    ValidateSingleLength(f_stop, 'stop()', False)
> > +
> > +    f_init  = f_init[0]
> > +    f_start = f_start[0]
> > +    f_stop  = f_stop[0]
> > +
> > +    # Validate parameters to init()
> > +    ValidateSingleLength(f_init.parameters, 'input parameter to init()')
> > +    ValidateSingleLength(f_init.response_parameters, 'output parameter from init()')
> > +    if f_init.parameters[0].kind.mojom_name != 'IPASettings':
> > +        raise Exception('init() must have single IPASettings input parameter')
> > +    if f_init.response_parameters[0].kind.spec != 'i32':
> > +        raise Exception('init() must have single int32 output parameter')
> > +
> > +    # Validate parameters to start()
> > +    ValidateZeroLength(f_start.parameters, 'input parameter to start()')
> > +    ValidateSingleLength(f_start.response_parameters, 'output parameter from start()')
> > +    if f_start.response_parameters[0].kind.spec != 'i32':
> > +        raise Exception('start() must have single int32 output parameter')
> > +
> > +    # Validate parameters to stop()
> > +    ValidateZeroLength(f_stop.parameters, 'input parameter to stop()')
> > +    ValidateZeroLength(f_stop.parameters, 'output parameter from stop()')
> > +
> > +class Generator(generator.Generator):
> > +
> > +    @staticmethod
> > +    def GetTemplatePrefix():
> > +        return 'libcamera_templates'
> > +
> > +    def GetFilters(self):
> > +        libcamera_filters = {
> > +            'all_types': GetAllTypes,
> > +            'bit_width': BitWidth,
> > +            'choose': Choose,
> > +            'comma_sep': CommaSep,
> > +            'default_value': GetDefaultValue,
> > +            'has_default_fields': HasDefaultFields,
> > +            'has_fd': HasFd,
> > +            'is_async': IsAsync,
> > +            'is_array': IsArray,
> > +            'is_controls': IsControls,
> > +            'is_enum': IsEnum,
> > +            'is_fd': IsFd,
> > +            'is_map': IsMap,
> > +            'is_plain_struct': IsPlainStruct,
> > +            'is_pod': IsPod,
> > +            'method_input_has_fd': MethodInputHasFd,
> > +            'method_output_has_fd': MethodOutputHasFd,
> > +            'method_param_names': MethodParamNames,
> > +            'method_param_inputs': MethodParamInputs,
> > +            'method_param_outputs': MethodParamOutputs,
> > +            'method_parameters': MethodParameters,
> > +            'method_return_value': MethodReturnValue,
> > +            'name': GetNameForElement,
> > +            'name_full': GetFullNameForElement,
> > +            'needs_control_serializer': NeedsControlSerializer,
> > +            'params_comma_sep': ParamsCommaSep,
> > +            'with_default_values': WithDefaultValues,
> > +            'with_fds': WithFds,
> > +        }
> > +        return libcamera_filters
> > +
> > +    def _GetJinjaExports(self):
> > +        return {
> > +            'base_controls': '%s::Controls' % ModuleClassName(self.module),
> > +            'cmd_enum_name': '_%sCMD' % ModuleClassName(self.module),
> 
> s/CMD/Command/ ?
> 
> > +            'enums': self.module.enums,
> > +            'has_array': len([x for x in self.module.kinds.keys() if x[0] == 'a']) > 0,
> > +            'has_map': len([x for x in self.module.kinds.keys() if x[0] == 'm']) > 0,
> > +            'has_namespace': self.module.mojom_namespace != '',
> > +            'imports': self.module.imports,
> > +            'interface_cb': self.module.interfaces[1],
> > +            'interface_main': self.module.interfaces[0],
> > +            'interface_name': 'IPA%sInterface' % ModuleClassName(self.module),
> > +            'ipc_name': 'IPAIPCUnixSocket',
> > +            'kinds': self.module.kinds,
> > +            'module': self.module,
> > +            'module_name': ModuleName(self.module.path),
> > +            'module_class_name': ModuleClassName(self.module),
> > +            'namespace': self.module.mojom_namespace.split('.'),
> > +            'namespace_str': self.module.mojom_namespace.replace('.', '::') if
> > +                             self.module.mojom_namespace is not None else '',
> > +            'proxy_name': 'IPAProxy%s' % ModuleClassName(self.module),
> > +            'proxy_worker_name': 'IPAProxy%sWorker' % ModuleClassName(self.module),
> > +            'structs_nonempty': [x for x in self.module.structs if len(x.fields) > 0],
> > +            'year': datetime.datetime.now().year,
> > +        }
> > +
> > +
> > +    @UseJinja('module_generated.h.tmpl')
> > +    def _GenerateDataHeader(self):
> > +        return self._GetJinjaExports()
> > +
> > +    @UseJinja('module_serializer.h.tmpl')
> > +    def _GenerateSerializer(self):
> > +        return self._GetJinjaExports()
> > +
> > +    @UseJinja('module_ipa_proxy.cpp.tmpl')
> > +    def _GenerateProxyCpp(self):
> > +        return self._GetJinjaExports()
> > +
> > +    @UseJinja('module_ipa_proxy.h.tmpl')
> > +    def _GenerateProxyHeader(self):
> > +        return self._GetJinjaExports()
> > +
> > +    @UseJinja('module_ipa_proxy_worker.cpp.tmpl')
> > +    def _GenerateProxyWorker(self):
> > +        return self._GetJinjaExports()
> > +
> > +    def GenerateFiles(self, unparsed_args):
> > +        parser = argparse.ArgumentParser()
> > +        parser.add_argument('--libcamera_generate_header',       action='store_true')
> > +        parser.add_argument('--libcamera_generate_serializer',   action='store_true')
> > +        parser.add_argument('--libcamera_generate_proxy_cpp',    action='store_true')
> > +        parser.add_argument('--libcamera_generate_proxy_h',      action='store_true')
> > +        parser.add_argument('--libcamera_generate_proxy_worker', action='store_true')
> > +        parser.add_argument('--libcamera_output_path')
> > +        args = parser.parse_args(unparsed_args)
> > +
> > +        ValidateInterfaces(self.module.interfaces)
> > +
> > +        fileutil.EnsureDirectoryExists(os.path.dirname(args.libcamera_output_path))
> > +
> > +        module_name = ModuleName(self.module.path)
> > +
> > +        if args.libcamera_generate_header:
> > +            self.Write(self._GenerateDataHeader(),
> > +                       args.libcamera_output_path)
> > +
> > +        if args.libcamera_generate_serializer:
> > +            self.Write(self._GenerateSerializer(),
> > +                       args.libcamera_output_path)
> > +
> > +        if args.libcamera_generate_proxy_cpp:
> > +            self.Write(self._GenerateProxyCpp(),
> > +                       args.libcamera_output_path)
> > +
> > +        if args.libcamera_generate_proxy_h:
> > +            self.Write(self._GenerateProxyHeader(),
> > +                       args.libcamera_output_path)
> > +
> > +        if args.libcamera_generate_proxy_worker:
> > +            self.Write(self._GenerateProxyWorker(),
> > +                       args.libcamera_output_path)

Hi Paul,

On Mon, Oct 05, 2020 at 07:08:57PM +0900, paul.elder@ideasonboard.com wrote:
> On Sun, Oct 04, 2020 at 06:56:40PM +0300, Laurent Pinchart wrote:
> > On Fri, Oct 02, 2020 at 11:31:22PM +0900, Paul Elder wrote:
> > > Add templates to mojo to generate code for the IPC mechanism. These
> > > templates generate:
> > > - module header
> > > - module serializer
> > > - IPA proxy cpp, header, and worker
> > > 
> > > Given an input data definition mojom file for a pipeline.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > ---
> > > Changes in v3:
> > > - add support for namespaces
> > > - fix enum assignment (used to have +1 for CMD applied to all enums)
> > > - use readHeader, writeHeader, and eraseHeader as static class functions
> > >   of IPAIPCUnixSocket (in the proxy worker)
> > > - add requirement that base controls *must* be defined in
> > >   libcamera::{pipeline_name}::Controls
> > > 
> > > Changes in v2:
> > > - mandate the main and callback interfaces, and init(), start(), stop()
> > >   and their parameters
> > > - fix returning single pod value from IPC-called function
> > > - add licenses
> > > - improve auto-generated message
> > > - other fixes related to serdes
> > > ---
> > >  .../module_generated.h.tmpl                   | 106 ++++
> > 
> > This isn't a great name, as _generated tells how the header is produced,
> > but not what it actually is. How about module_ipa_interface.h.tmpl ?
> 
> I named it like that because that's what the generated headers end up
> getting called. There's the main header, like raspberrypi.h, and then
> there's the generated one, like raspberrypi_generated.h.
> 
> raspberrypi_ipa_interface.h ...?

Yes, I meant that the generated header could be called
raspberrypi_ipa_interface.h (name subject to bikeshedding, the point is
to not have a _generated suffix). It would actually, I think, be useful
to name the generated header with the same base name as the .mojom file.
That way, from a pipeline handler and IPA author point of view, writing
<foo>.mojom and including <foo>.h would be more intuitive than including
<foo>_generated.h. A _generated suffix wouldn't cause the world to do of
course, but making it more intuitive helps keeping the whole experience
smoother. That's a design rule I've learnt from Qt, if a user who hasn't
read detailed documentation writes code intuitively and gets it right,
then your API design is good.

> > >  .../module_ipa_proxy.cpp.tmpl                 | 233 +++++++++
> > >  .../module_ipa_proxy.h.tmpl                   | 117 +++++
> > >  .../module_ipa_proxy_worker.cpp.tmpl          | 184 +++++++
> > >  .../module_serializer.h.tmpl                  |  44 ++
> > 
> > And maybe module_ipa_serializer.h.tmpl for consistency ?
> 
> This becomes raspberrypi_serializer.h. raspberrypi_ipa_serializer.h ?
> 
> Or just the template file name?

Both the template and generated files, yes.

> > >  .../libcamera_templates/proxy_functions.tmpl  | 185 +++++++
> > >  .../libcamera_templates/serializer.tmpl       | 276 +++++++++++
> > >  .../generators/mojom_libcamera_generator.py   | 461 ++++++++++++++++++
> > >  8 files changed, 1606 insertions(+)
> > >  create mode 100644 utils/ipc/generators/libcamera_templates/module_generated.h.tmpl
> > >  create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > >  create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > >  create mode 100644 utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > >  create mode 100644 utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl
> > >  create mode 100644 utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > >  create mode 100644 utils/ipc/generators/libcamera_templates/serializer.tmpl
> > >  create mode 100644 utils/ipc/generators/mojom_libcamera_generator.py
> > > 
> > > diff --git a/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl
> > > new file mode 100644
> > > index 00000000..7dd48b0b
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/module_generated.h.tmpl
> > > @@ -0,0 +1,106 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) {{ year }}, Google Inc.
> > > + *
> > > + * {{ module_name }}_generated.h - Image Processing Algorithm interface for {{ module_name }}
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__
> > > +#define __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__
> > > +
> > > +#include <libcamera/ipa/ipa_interface.h>
> > > +
> > 
> > I think you can drop this blank line.
> > 
> > > +#include <libcamera/ipa/{{ module_name }}.h>
> > > +
> > > +{% if has_map %}#include <map>{% endif %}
> > > +{% if has_array %}#include <vector>{% endif %}
> > > +
> > > +namespace libcamera {
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace %}
> > > +namespace {{ns}} {
> > 
> > There's a mix of {{ var }} and {{var}}. I don't mind much which one is
> > used, but I think we should be consistent.
> 
> Ah right, I forgot to fix that.
> 
> > > +{% endfor %}
> > > +{%- endif %}
> > > +
> > > +enum {{ cmd_enum_name }} {
> > 
> > It would be nice to make this an enum class, to avoid namespace clashes.
> > Alternatively, we could enforce the usage of namespaces for all IPAs.
> 
> I made cmd_enum_name start with an underscore to avoid namespace
> clashes.
> 
> 'cmd_enum_name': '_%sCMD' % ModuleClassName(self.module),
> 
> So like enum _RPiCMD. I thought the underscore would be sufficient for
> anti name clashes.

I meant namespace clashes for the enumerated values.

enum Foo {
	EnumValue,
};

enum Bar {
	EnumValue,
};

would make

	uint32_t val = EnumValue;

ambiguous, while with enum class you always have to specify the Foo:: or
Bar:: prefix.

> > enum class also brings the added benefit of catching improper usages,
> > but comes at a cost of requiring additional static_cast<> to convert
> > from and to uint32_t.
> 
> I thought you could use enum as a type anyway? That's what I did with
> the enums generated from the mojom file, and they all needed
> static_cast<>s. They're in the generated serializers:
> 
> ret.op_ = static_cast<RkISP1Operations>(IPADataSerializer<uint32_t>::deserialize(m, m + 4));

Both unscoped enums (i.e. non-class) and scoped enums (enum class) are
types, so this good is good. The two main differences is that enumerated
values always have to be specified with the type specified for an enum
class (Foo::Value), and implicit casts between integers and enum class
are not allowed (you always need an explicit cast).

"Each enumerator becomes a named constant of the enumeration's type
(that is, name), which is contained within the scope of the enumeration,
and can be accessed using scope resolution operator. There are no
implicit conversions from the values of a scoped enumerator to integral
types, although static_cast may be used to obtain the numeric value of
the enumerator."

(https://en.cppreference.com/w/cpp/language/enum)

Scoped enums thus bring additional safety, but always require explicit
casts. For the generated serialization code that's not an issue at all
as it's just a matter of getting templates right. For enums that define
bitfields meant to be combined, that gets more difficult:

enum class Foo {
	Value0 = (1 << 0),
	Value1 = (1 << 1),
};

void foo(Foo f);

...

foo(Value0);			/* OK */
foo(Value0 | Value1);		/* Error */

> > > +	CMD_EXIT = 0,
> > 
> > I'd drop the CMD_ prefix, especially if we use enum class, and use
> > CamelCase for names instead of uppercase.
> 
> This cmd enum is for RPC, so I thought it was fine for just this enum to
> be prefix with CMD_, just to make it clear in the generated code that
> that's what this is for. UnixSocketTest does this too (that's where I
> got it from).

The current implementation is temporary code, so you can depart from it
:-) No big deal though as this only matters in generated code, but if we
go for enum class and have to write _RPiCMD::CMD_EXIT, I think
_RPiCMD::EXIT would be better.

On this topic, would it be difficult to go for CamelCase for types
(_RPiCmd, CmdExit / Exit, ...) ? It's generated code, but if one has to
read it for any reason, keeping the coding style consistent would be
nice.

> > > +{%- for method in interface_main.methods %}
> > > +	CMD_{{ method.mojom_name|upper }} = {{loop.index}},
> > > +{%- endfor %}
> > > +{%- set count = interface_main.methods|length -%}
> > > +{%- for method in interface_cb.methods %}
> > > +	CMD_{{ method.mojom_name|upper }} = {{loop.index + count}},
> > > +{%- endfor %}
> > 
> > Should we split the enum in two, one for each interface ?
> 
> Either way is fine, it doesn't matter much. That's why I put them
> together. The code generator loops over interface_{main,cb}.methods and
> not the enum when it generates both sides of the RPC calls, so putting
> them in different or the same enum makes literally no difference.

You're right, shouldn't be a big deal.

> > > +};
> > > +
> > > +{% for enum in enums %}
> > > +enum {{ enum.mojom_name }} {
> > > +{%- for field in enum.fields %}
> > > +	{{ field.mojom_name }} = {{ field.numeric_value }},
> > > +{%- endfor %}
> > > +};
> > > +{% endfor %}
> > > +
> > > +{%- for struct in structs_nonempty %}
> > > +struct {{ struct.mojom_name }}
> > > +{
> > > +public:
> > > +	{{ struct.mojom_name }}() {%- if struct|has_default_fields %} :{% endif %}
> > > +{%- for field in struct.fields|with_default_values -%}
> > > +{{" " if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ ", " if not loop.last }}
> > > +{%- endfor %} {}
> > 
> > This generates really long lines. How about
> > 
> > 	{{ struct.mojom_name }}() {%- if struct|has_default_fields %}
> > 		:{% endif %}
> > {%- for field in struct.fields|with_default_values -%}
> > {{" " if loop.first}}{{ field.mojom_name}}_({{ field|default_value }}){{ ", " if not loop.last }}
> > {%- endfor %}
> > 	{
> > 	}
> 
> Ah, right.
> 
> > Would be great to wrap the initializers line too, but I think that's
> > overkill.
> > 
> > > +	~{{ struct.mojom_name }}() {}
> > > +
> > > +	{{ struct.mojom_name }}(
> > > +{%- for field in struct.fields -%}
> > > +{{ field|name }} {{ field.mojom_name }}{{ ", " if not loop.last }}
> > > +{%- endfor -%}
> > > +) :
> > > +{%- for field in struct.fields -%}
> > > +{{" " if loop.first}}{{ field.mojom_name}}_({{ field.mojom_name }}){{ ", " if not loop.last }}
> > > +{%- endfor %} {}
> > 
> > Same here.
> > 
> > > +{% for field in struct.fields %}
> > > +	{{ field|name }} {{ field.mojom_name }}_;
> > > +{%- endfor %}
> > > +};
> > > +{% endfor %}
> > > +
> > > +{#-
> > > +What to do about the #includes? Should we forward-declare, or
> > > +require {{module_name}}.h to include the required headers?
> > > +For now I'm going for the latter, coz this header essentially
> > > +extends {{module_name}}.h.
> > > +#}
> > 
> > Looking at the existing {{module_name}}.h, there are a few enums and
> > constants that could be moved to {{module_name}}_generated.h, such as
> > enum BufferMark or MaxLsGridSize in raspberrypi.h. What do you think ?
> 
> Yeah enums I think are better to move to the mojom file. I did that for
> ConfigParameters.
> 
> mojo supports const (like if we had const uint32 MaxLsGridSize = 0x8000;)
> but the code generator doesn't yet.
> 
> > Then we have libcamera::RPi::Controls, which I'm wondering how to handle
> > best. It's not strictly related to this series, but wouldn't they be
> > better stored in the pipeline handler, and passed to the IPA ?
> 
> I think it would be best if all ControlLists had a ControlInfoMap, then
> it wouldn't matter where libcamera::RPi::controls goes.

I think we should move in a direction that doesn't declare controls in a
shared header. It could be done on top though.

> > Finally there's the odd beast such as
> > 
> > #define VIMC_IPA_FIFO_PATH "/tmp/libcamera_ipa_vimc_fifo"
> > 
> > which I also wonder if it would make sense to pass it to the IPA at init
> > time. {{module_name}}.h could then possibly be dropped.
> 
> Oh you're thinking of dropping the main one entirely. Yeah that could
> work too. I'm just worried if it'll be too restrictive. #defines could
> be replaced with consts, so it's just the base ControlInfoMap that's an
> issue.

Ideally, it would be nice, but you are right that it may be too
restrictive.

> > > +class {{ interface_name }} : public IPAInterface
> > > +{
> > > +public:
> > > +	virtual ~{{interface_name}}() {};
> > 
> > No need for the trailing ;
> > 
> > > +{% for method in interface_main.methods %}
> > > +	virtual {{method|method_return_value}} {{method.mojom_name}}(
> > > +{%- for param in method|method_parameters %}
> > > +		{{param}}{{- "," if not loop.last}}
> > > +{%- endfor -%}
> > > +) = 0;
> > > +{% endfor %}
> > > +
> > > +{%- for method in interface_cb.methods %}
> > > +	Signal<
> > > +{%- for param in method.parameters -%}
> > > +		const {{param|name}}{{" &" if not param|is_pod}}
> > 
> > Should the const be dropped if param|is_pod ? Same below.
> 
> Ah right it's pass-by-value. Sure, let's drop it.
> 
> > > +		{{- ", " if not loop.last }}
> > > +{%- endfor -%}
> > > +> {{method.mojom_name}};
> > > +{% endfor -%}
> > > +};
> > > +
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace|reverse %}
> > > +} /* {{ns}} */
> > > +{% endfor %}
> > > +{%- endif %}
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_IPA_INTERFACE_{{ module_name|upper }}_GENERATED_H__ */
> > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > new file mode 100644
> > > index 00000000..f3eeeaaa
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> > > @@ -0,0 +1,233 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> > > +
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) {{ year }}, Google Inc.
> > > + *
> > > + * ipa_proxy_{{ module_name }}.cpp - Image Processing Algorithm proxy for {{ module_name }}
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#include <vector>
> > > +
> > > +#include <libcamera/ipa/ipa_interface.h>
> > 
> > I think you can drop this, as it's included by {{module_name}}.h. Same
> > below.
> 
> Oh, okay.
> 
> > > +#include <libcamera/ipa/ipa_module_info.h>
> > > +#include <libcamera/ipa/{{module_name}}.h>
> > > +#include <libcamera/ipa/{{module_name}}_generated.h>
> > > +#include <libcamera/ipa/{{module_name}}_serializer.h>
> > > +
> > > +#include "libcamera/internal/control_serializer.h"
> > > +#include "libcamera/internal/ipa_ipc.h"
> > > +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> > > +#include "libcamera/internal/ipa_data_serializer.h"
> > > +#include "libcamera/internal/ipa_module.h"
> > > +#include "libcamera/internal/ipa_proxy.h"
> > > +#include "libcamera/internal/ipc_unixsocket.h"
> > > +#include "libcamera/internal/log.h"
> > > +#include "libcamera/internal/process.h"
> > > +#include "libcamera/internal/thread.h"
> > > +
> > > +#include <libcamera/ipa/ipa_proxy_{{module_name}}.h>
> > 
> > Should this be moved above with the other libcamera/ipa/ headers ?
> 
> This is the header that corresponds to this cpp file. I thought that
> gets special treatment and goes on its own at the end like here.

It gets special treatement, but goes at the top, not the bottom :-) The
idea is to get every header compiled without anything else included
before, to ensure they're self-contained.

> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DECLARE_CATEGORY(IPAProxy)
> > > +
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace %}
> > > +namespace {{ns}} {
> > > +{% endfor %}
> > > +{%- endif %}
> > > +
> > > +{{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)
> > > +	: IPAProxy(ipam), running_(false),
> > > +	  isolate_(isolate)
> > > +{
> > > +	LOG(IPAProxy, Debug)
> > > +		<< "initializing dummy proxy: loading IPA from "
> > 
> > It's not a dummy proxy anymore :-)
> 
> Oops :p
> 
> > > +		<< ipam->path();
> > > +
> > > +	if (isolate_) {
> > > +		const std::string proxy_worker_path = resolvePath("ipa_proxy_{{module_name}}");
> > > +		if (proxy_worker_path.empty()) {
> > > +			LOG(IPAProxy, Error)
> > > +				<< "Failed to get proxy worker path";
> > > +			return;
> > > +		}
> > > +
> > > +		ipc_ = std::make_unique<IPAIPCUnixSocket>(ipam->path().c_str(), proxy_worker_path.c_str());
> > > +		if (!ipc_->isValid()) {
> > > +			LOG(IPAProxy, Error) << "Failed to create IPAIPC";
> > > +			return;
> > > +		}
> > > +
> > > +{% if interface_cb.methods|length > 0 %}
> > > +		ipc_->recvIPC.connect(this, &{{proxy_name}}::recvIPC);
> > > +{% endif %}
> > > +
> > > +		valid_ = true;
> > > +		return;
> > > +	}
> > 
> > There's very little code shared between the isolated and non-isolated
> > paths. It could make sense to split this class in two. On the other
> > hand, that would require the ability for the IPAManager to create the
> > right class type, which may complicate things a bit, but maybe not much.
> > For instance we could have the two classes called {{proxy_name}}Thread
> > and {{proxy_name}}IPC, and have a {{proxy_name}} class defined as
> > 
> > class {{proxy_name}}
> > {
> > public:
> > 	using IPC = {{proxy_name}}IPC;
> > 	using Thread = {{proxy_name}}Thread;
> > };
> > 
> > Then, in IPAManager::createIPA(), you would have
> > 
> > 	std::unique_ptr<P> proxy;
> > 	if (self_->isSignatureValid(m)))
> > 		proxy = std::make_unique<P::Thread>(m);
> > 	else
> > 		proxy = std::make_unique<P::IPC>(m);
> 
> Yeah maybe. The amount of generated code doesn't change though, it'll
> just be organized differently, and the only one that will notice the
> difference is IPAManager as shown in this snippet.

The proxy wouldn't have to check isolate_ in every function. The
overhead is negligible, but the generated code (and the template) would
be a bit easier to follow I think. It would also possibly be a good path
forward to add support for different proxy types in the future if needed
(for instance a Chrome OS-specific proxy). If it's an easy change this
would be nice, otherwise it's no big deal for now.

> > > +
> > > +	if (!ipam->load())
> > > +		return;
> > > +
> > > +	IPAInterface *ipai = ipam->createInterface();
> > > +	if (!ipai) {
> > > +		LOG(IPAProxy, Error)
> > > +			<< "Failed to create IPA context for " << ipam->path();
> > > +		return;
> > > +	}
> > > +
> > > +	ipa_ = std::unique_ptr<{{interface_name}}>(dynamic_cast<{{interface_name}} *>(ipai));
> > > +	proxy_.setIPA(ipa_.get());
> > > +
> > > +{% for method in interface_cb.methods %}
> > > +	ipa_->{{method.mojom_name}}.connect(this, &{{proxy_name}}::{{method.mojom_name}}Thread);
> > > +{%- endfor %}
> > > +
> > > +	valid_ = true;
> > > +}
> > > +
> > > +{{proxy_name}}::~{{proxy_name}}()
> > > +{
> > > +	if (isolate_)
> > > +		ipc_->sendAsync(CMD_EXIT, {}, {});
> > > +}
> > > +
> > > +{% if interface_cb.methods|length > 0 %}
> > > +void {{proxy_name}}::recvIPC(std::vector<uint8_t> &data, std::vector<int32_t> &fds)
> > > +{
> > > +	uint32_t cmd = (data[0] & 0xff) |
> > > +		       ((data[1] & 0xff) << 8) |
> > > +		       ((data[2] & 0xff) << 16) |
> > > +		       ((data[3] & 0xff) << 24);
> > 
> > As data is a vector of uint8_t, I think the & 0xff is unnecessary.
> > 
> > Should the data length be validated ?
> 
> tbh I was thinking about it. I think cmd and vec are guaranteed, though.
> Maybe we should check it just in case.
> 
> > > +
> > > +	/* Need to skip another 4 bytes for the sequence number. */
> > > +	std::vector<uint8_t> vec(data.begin() + 8, data.end());
> > 
> > This creates a copy, which isn't very nice :-/ I think the IPC class
> > interface would benefit from switching from std::vector to Span.
> 
> Ah okay, I'll look into it.
> 
> > > +	switch (cmd) {
> > > +{%- for method in interface_cb.methods %}
> > > +	case CMD_{{method.mojom_name|upper}}: {
> > > +		{{method.mojom_name}}IPC(vec, fds);
> > > +		break;
> > > +	}
> > > +{%- endfor %}
> > 
> > Should we log unknown commands ?
> 
> Oh maybe.
> 
> > > +	}
> > > +}
> > > +{%- endif %}
> > > +
> > > +{% for method in interface_main.methods %}
> > > +{{proxy_funcs.func_sig(proxy_name, method)}}
> > > +{
> > > +	if (isolate_)
> > > +		{{"return " if method|method_return_value != "void"}}{{method.mojom_name}}IPC(
> > > +{%- for param in method|method_param_names -%}
> > > +		{{param}}{{- ", " if not loop.last}}
> > > +{%- endfor -%}
> > > +);
> > > +	else
> > > +		{{"return " if method|method_return_value != "void"}}{{method.mojom_name}}Thread(
> > > +{%- for param in method|method_param_names -%}
> > > +		{{param}}{{- ", " if not loop.last}}
> > > +{%- endfor -%}
> > > +);
> > > +}
> > > +
> > > +{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> > > +{
> > > +{%- if method.mojom_name == "init" %}
> > > +	{{proxy_funcs.init_thread_body()}}
> > > +{% elif method.mojom_name == "start" %}
> > 
> > I think you're missing a - before elif, here and below.
> 
> This, I don't think so.
> 
> > > +	{{proxy_funcs.start_thread_body()}}
> > > +{% elif method.mojom_name == "stop" %}
> > > +	{{proxy_funcs.stop_thread_body()}}
> > 
> > Instead of using macros, would it make sense to move the code to the
> > base IPAProxy class ?
> 
> Maybe...
> 
> > > +{% elif not method|is_async %}
> > > +	ipa_->{{method.mojom_name}}(
> > > +	{%- for param in method|method_param_names -%}
> > > +		{{param}}{{- ", " if not loop.last}}
> > > +	{%- endfor -%}
> > > +);
> > > +{% elif method|is_async %}
> > > +	proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued,
> > > +	{%- for param in method|method_param_names -%}
> > > +		{{param}}{{- ", " if not loop.last}}
> > > +	{%- endfor -%}
> > > +);
> > > +{%- endif %}
> > > +}
> > > +
> > > +{{proxy_funcs.func_sig(proxy_name, method, "IPC")}}
> > > +{
> > > +{%- set has_input = true if method|method_param_inputs|length > 0 %}
> > > +{%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != "void" %}
> > > +{% if has_input %}
> > 
> > Shouldn't it be {%- ?
> 
> Probably... I don't remember if this specific one caused other problems.
> 
> > > +	std::vector<uint8_t> _ipcInputBuf;
> > > +{%- if method|method_input_has_fd %}
> > > +	std::vector<int32_t> _ipcInputFds;
> > > +{%- endif %}
> > > +{%- endif %}
> > > +{%- if has_output %}
> > > +	std::vector<uint8_t> _ipcOutputBuf;
> > > +{%- if method|method_output_has_fd %}
> > > +	std::vector<int32_t> _ipcOutputFds;
> > > +{%- endif %}
> > > +{%- endif %}
> > > +
> > > +{{proxy_funcs.serialize_call(method|method_param_inputs, '_ipcInputBuf', '_ipcInputFds', base_controls)}}
> > > +
> > > +{%- set input_buf = "_ipcInputBuf" if has_input else "{}" %}
> > > +{%- set fds_buf = "_ipcInputFds" if method|method_input_has_fd else "{}" %}
> > > +{%- set cmd = "CMD_" + method.mojom_name|upper %}
> > > +{% if method|is_async %}
> > > +	int ret = ipc_->sendAsync({{cmd}}, {{input_buf}}, {{fds_buf}});
> > > +{%- else %}
> > > +	int ret = ipc_->sendSync({{cmd}}, {{input_buf}}, {{fds_buf}}
> > > +{{- ", &_ipcOutputBuf" if has_output -}}
> > > +{{- ", &_ipcOutputFds" if has_output and method|method_output_has_fd -}}
> > > +);
> > > +{%- endif %}
> > > +	if (ret < 0) {
> > > +		LOG(IPAProxy, Error) << "Failed to call {{method.mojom_name}}";
> > > +{%- if method|method_return_value != "void" %}
> > > +		return static_cast<{{method|method_return_value}}>(ret);
> > > +{%- else %}
> > > +		return;
> > > +{%- endif %}
> > > +	}
> > > +	LOG(IPAProxy, Debug) << "Done calling {{method.mojom_name}}";
> > 
> > Is this message helpful for debugging ? I think we should work at some
> > point on a tracing mechanism instead of manually tracing calls.
> 
> It was while I wasn't sure the calls were going through. I think at this
> point it's stable enough that we can remove it.
> 
> Yeah tracing would be nice :)

I've experimented with lttng-ust, I think we'll generate tracepoints
automatically in the future :-)

> > > +{% if method|method_return_value != "void" %}
> > > +	return IPADataSerializer<{{method.response_parameters|first|name}}>::deserialize(_ipcOutputBuf, 0);
> > > +{% elif method|method_param_outputs|length > 0 %}
> > > +{{proxy_funcs.deserialize_call(method|method_param_outputs, '_ipcOutputBuf', '_ipcOutputFds')}}
> > > +{% endif -%}
> > > +}
> > > +
> > > +{% endfor %}
> > > +
> > > +
> > 
> > One blank line should be enough.
> 
> Ah, right.
> 
> > > +{% for method in interface_cb.methods %}
> > > +{{proxy_funcs.func_sig(proxy_name, method, "Thread")}}
> > > +{
> > > +	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> > > +}
> > > +
> > > +void {{proxy_name}}::{{method.mojom_name}}IPC(
> > > +	std::vector<uint8_t> &data,
> > > +	[[maybe_unused]] std::vector<int32_t> &fds)
> > > +{ 
> > > +{%- for param in method.parameters %}
> > > +	{{param|name}} {{param.mojom_name}};
> > > +{%- endfor %}
> > > +{{proxy_funcs.deserialize_call(method.parameters, 'data', 'fds', false)}}
> > > +	{{method.mojom_name}}.emit({{method.parameters|params_comma_sep}});
> > > +}
> > > +{% endfor %}
> > > +
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace|reverse %}
> > > +} /* {{ns}} */
> > > +{% endfor %}
> > > +{%- endif %}
> > > +} /* namespace libcamera */
> > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > > new file mode 100644
> > > index 00000000..7d2bfb6c
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> > > @@ -0,0 +1,117 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> > > +
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) {{ year }}, Google Inc.
> > > + *
> > > + * ipa_proxy_{{ module_name }}.h - Image Processing Algorithm proxy for {{ module_name }}
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__
> > > +#define __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__
> > > +
> > > +#include <libcamera/ipa/ipa_interface.h>
> > > +#include <libcamera/ipa/{{module_name}}.h>
> > > +#include <libcamera/ipa/{{module_name}}_generated.h>
> > > +
> > > +#include "libcamera/internal/control_serializer.h"
> > > +#include "libcamera/internal/ipa_ipc.h"
> > > +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> > > +#include "libcamera/internal/ipa_proxy.h"
> > > +#include "libcamera/internal/ipc_unixsocket.h"
> > > +#include "libcamera/internal/thread.h"
> > > +
> > > +namespace libcamera {
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace %}
> > > +namespace {{ns}} {
> > > +{% endfor %}
> > > +{%- endif %}
> > > +
> > > +class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object
> > > +{
> > > +public:
> > > +	{{proxy_name}}(IPAModule *ipam, bool isolate);
> > > +	~{{proxy_name}}();
> > > +
> > > +{% for method in interface_main.methods %}
> > > +{{proxy_funcs.func_sig(proxy_name, method, "", false, true)|indent(8, true)}};
> > > +{% endfor %}
> > > +
> > > +{%- for method in interface_cb.methods %}
> > > +	Signal<
> > > +{%- for param in method.parameters -%}
> > > +		const {{param|name}}{{" &" if not param|is_pod}}
> > > +		{{- ", " if not loop.last }}
> > > +{%- endfor -%}
> > > +> {{method.mojom_name}};
> > > +{% endfor %}
> > > +
> > > +private:
> > > +	void recvIPC(std::vector<uint8_t> &data, std::vector<int32_t> &fds);
> > > +
> > > +{% for method in interface_main.methods %}
> > > +{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> > > +{{proxy_funcs.func_sig(proxy_name, method, "IPC", false)|indent(8, true)}};
> > > +{% endfor %}
> > > +{% for method in interface_cb.methods %}
> > > +{{proxy_funcs.func_sig(proxy_name, method, "Thread", false)|indent(8, true)}};
> > > +	void {{method.mojom_name}}IPC(
> > > +		std::vector<uint8_t> &data,
> > > +		std::vector<int32_t> &fds);
> > 
> > Should these be const ?
> 
> Yeah.
> 
> > > +{% endfor %}
> > > +
> > > +	/* Helper class to invoke async functions in another thread. */
> > > +	class ThreadProxy : public Object
> > > +	{
> > > +	public:
> > > +		void setIPA({{interface_name}} *ipa)
> > > +		{
> > > +			ipa_ = ipa;
> > > +		}
> > > +
> > > +		int start()
> > > +		{
> > > +			return ipa_->start();
> > > +		}
> > > +
> > > +		void stop()
> > > +		{
> > > +			ipa_->stop();
> > > +		}
> > > +{% for method in interface_main.methods %}
> > > +{%- if method|is_async %}
> > > +		{{proxy_funcs.func_sig(proxy_name, method, "", false)|indent(16)}}
> > > +		{
> > > +			ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> > > +		}
> > > +{%- endif %}
> > > +{%- endfor %}
> > 
> > I think this is mixing two concepts, which shouldn't cause any issue for
> > now, but may come bite us back later.
> > 
> > As you've noticed in the existing IPAProxyThread implementation, we call
> > some methods directly, and proxy other methods across threads. The idea
> > is that anything called before start() is a direct call, and anything
> > from start() until but not including stop() is indirect. The reason is
> > that before start() there's no thread running, so we can't make a
> > cross-thread call.
> 
> Ah, so sync vs async is different from non-thread call vs thread call...
> 
> > One direct consequence of this architecture is that all calls before
> > start() are synchronous, and can thus return a value. Calls after
> > start() are asynchronous, but that's a separate design decision. We
> > could synchronize cross-thread calls if we wanted, we have chosen not to
> > in order to avoid delays that could damage the real time performance of
> > the pipeline handler.
> > 
> > With IPC the situation is different, all calls are cross-process. Still,
> > we want to expose a similar behaviour to pipeline handlers, so calls
> > before start() are synchronous, and further calls are asynchronous.
> > 
> > I don't think we can easily enforce this through the IDL, but I believe
> > it would make sense to document this requirements, so that authors will
> > not create an async method meant to be called before start(), or a
> > synchronous method after start().
> 
> Yeah, I think documentation should be sufficient.
> 
> > > +
> > > +	private:
> > > +		{{interface_name}} *ipa_;
> > > +	};
> > > +
> > > +	bool running_;
> > > +	Thread thread_;
> > > +	ThreadProxy proxy_;
> > > +	std::unique_ptr<{{interface_name}}> ipa_;
> > > +
> > > +	const bool isolate_;
> > > +
> > > +	std::unique_ptr<IPAIPCUnixSocket> ipc_;
> > > +
> > > +	ControlSerializer controlSerializer_;
> > > +};
> > > +
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace|reverse %}
> > > +} /* {{ns}} */
> > > +{% endfor %}
> > > +{%- endif %}
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_INTERNAL_IPA_PROXY_{{ module_name|upper }}_H__ */
> > > 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
> > > new file mode 100644
> > > index 00000000..1647a9ca
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> > > @@ -0,0 +1,184 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{%- import "proxy_functions.tmpl" as proxy_funcs -%}
> > > +
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) {{ year }}, Google Inc.
> > > + *
> > > + * ipa_proxy_{{ module_name }}_worker.cpp - Image Processing Algorithm proxy worker for {{ module_name }}
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#include <algorithm>
> > > +#include <iostream>
> > > +#include <sys/types.h>
> > > +#include <tuple>
> > > +#include <unistd.h>
> > > +
> > > +#include <libcamera/event_dispatcher.h>
> > > +#include <libcamera/ipa/ipa_interface.h>
> > > +#include <libcamera/ipa/{{module_name}}_generated.h>
> > > +#include <libcamera/ipa/{{module_name}}_serializer.h>
> > > +#include <libcamera/logging.h>
> > > +
> > > +#include "libcamera/internal/camera_sensor.h"
> > > +#include "libcamera/internal/control_serializer.h"
> > > +#include "libcamera/internal/ipa_data_serializer.h"
> > > +#include "libcamera/internal/ipa_ipc_unixsocket.h"
> > > +#include "libcamera/internal/ipa_module.h"
> > > +#include "libcamera/internal/ipa_proxy.h"
> > > +#include "libcamera/internal/ipc_unixsocket.h"
> > > +#include "libcamera/internal/log.h"
> > > +#include "libcamera/internal/thread.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +LOG_DEFINE_CATEGORY({{proxy_name}}Worker)
> > > +
> > > +{%- if has_namespace %}
> > > +{% for ns in namespace -%}
> > > +using namespace {{ns}};
> > > +{% endfor %}
> > > +{%- endif %}
> > > +
> > > +struct CallData {
> > > +	IPCUnixSocket::Payload *response;
> > > +	bool done;
> > > +};
> > > +
> > > +{{interface_name}} *ipa_;
> > > +IPCUnixSocket socket_;
> > > +std::map<uint32_t, struct CallData> callData_;
> > 
> > callData_ doesn't seem to be used (and thus struct CallData shouldn't be
> > needed).
> 
> Oops, remnant of copying from UnixSocket.
> 
> > > +
> > > +ControlSerializer controlSerializer_;
> > > +
> > > +bool exit_ = false;
> > 
> > Should you group all this, as well as the functions below, in a
> > {{module_name}}ProxyWorker class ?
> 
> I could. It's standalone so I didn't see much point in doing so though.

Just for a readability point of view, up to you.

> > > +
> > > +void readyRead(IPCUnixSocket *socket)
> > > +{
> > > +	IPCUnixSocket::Payload _message, _response;
> > > +	int _ret = socket->receive(&_message);
> > > +	if (_ret) {
> > > +		LOG({{proxy_name}}Worker, Error)
> > > +			<< "Receive message failed" << _ret;
> > > +		return;
> > > +	}
> > > +
> > > +	uint32_t _cmd, _seq;
> > > +	std::tie(_cmd, _seq) = IPAIPCUnixSocket::readHeader(_message);
> > > +	IPAIPCUnixSocket::eraseHeader(_message);
> > 
> > Using Span would help avoiding copies.
> 
> Ah...
> 
> > > +
> > > +	switch (_cmd) {
> > > +	case CMD_EXIT: {
> > > +		exit_ = true;
> > > +		break;
> > > +	}
> > > +
> > > +{% for method in interface_main.methods %}
> > > +	case CMD_{{method.mojom_name|upper}}: {
> > > +	{{proxy_funcs.deserialize_call(method|method_param_inputs, '_message.data', '_message.fds', false, true)|indent(8, true)}}
> > > +{% for param in method|method_param_outputs %}
> > > +		{{param|name}} {{param.mojom_name}};
> > > +{% endfor %}
> > > +{%- if method|method_return_value != "void" %}
> > > +		{{method|method_return_value}} _callRet = ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}});
> > > +{%- else %}
> > > +		ipa_->{{method.mojom_name}}({{method.parameters|params_comma_sep}}
> > > +{{- ", " if method|method_param_outputs|params_comma_sep -}}
> > > +{%- for param in method|method_param_outputs -%}
> > > +&{{param.mojom_name}}{{", " if not loop.last}}
> > > +{%- endfor -%}
> > > +);
> > > +{%- endif %}
> > > +{% if not method|is_async %}
> > > +		IPAIPCUnixSocket::writeHeader(_response, _cmd, _seq);
> > > +{%- if method|method_return_value != "void" %}
> > > +		std::vector<uint8_t> _callRetBuf;
> > > +		std::tie(_callRetBuf, std::ignore) =
> > > +			IPADataSerializer<{{method|method_return_value}}>::serialize(_callRet);
> > > +		_response.data.insert(_response.data.end(), _callRetBuf.begin(), _callRetBuf.end());
> > > +{%- else %}
> > > +	{{proxy_funcs.serialize_call(method|method_param_outputs, "_response.data", "_response.fds", base_controls)|indent(8, true)}}
> > > +{%- endif %}
> > > +		int _ret = socket_.send(_response);
> > > +		if (_ret < 0) {
> > > +			LOG({{proxy_name}}Worker, Error)
> > > +				<< "Reply to {{method.mojom_name}}() failed" << _ret;
> > > +		}
> > > +		LOG({{proxy_name}}Worker, Debug) << "Done replying to {{method.mojom_name}}()";
> > > +{%- endif %}
> > > +		break;
> > > +	}
> > > +{% endfor %}
> > 
> > This isn't a huge deal, but the generated code my be a bit clearer if
> > each case called a function.
> 
> It does! ipa_->function(); :) It just comes with deserialization
> beforehand and serialization afterward for synchronous calls. I think
> it's good enough...
> 
> > > +	}
> > > +}
> > > +
> > > +{% for method in interface_cb.methods %}
> > > +{{proxy_funcs.func_sig(proxy_name, method, "", false)}}
> > > +{
> > > +	IPCUnixSocket::Payload _message;
> > > +
> > > +	IPAIPCUnixSocket::writeHeader(_message, CMD_{{method.mojom_name|upper}}, 0);
> > > +	{{proxy_funcs.serialize_call(method|method_param_inputs, "_message.data", "_message.fds", base_controls)}}
> > > +
> > > +	socket_.send(_message);
> > > +
> > > +	LOG({{proxy_name}}Worker, Debug) << "{{method.mojom_name}} done";
> > > +}
> > > +{%- endfor %}
> > 
> > You can drop the -
> 
> Yeah, this one needs to be dropped.
> 
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > +	/* Uncomment this for debugging. */
> > > +#if 0
> > > +	std::string logPath = "/tmp/libcamera.worker." +
> > > +			      std::to_string(getpid()) + ".log";
> > > +	logSetFile(logPath.c_str());
> > > +#endif
> > > +
> > > +	if (argc < 3) {
> > > +		LOG({{proxy_name}}Worker, Debug)
> > 
> > Maybe s/Debug/Error/ ?
> 
> Ah yes.
> 
> > > +			<< "Tried to start worker with no args";
> > > +		return EXIT_FAILURE;
> > > +	}
> > > +
> > > +	int fd = std::stoi(argv[2]);
> > > +	LOG({{proxy_name}}Worker, Debug)
> > 
> > And Info ?
> 
> Yeah that's better too.
> 
> > > +		<< "Starting worker for IPA module " << argv[1]
> > > +		<< " with IPC fd = " << fd;
> > > +
> > > +	std::unique_ptr<IPAModule> ipam = std::make_unique<IPAModule>(argv[1]);
> > > +	if (!ipam->isValid() || !ipam->load()) {
> > > +		LOG({{proxy_name}}Worker, Error)
> > > +			<< "IPAModule " << argv[1] << " should be valid but isn't";
> > 
> > Maybe just " isn't valid" ?
> 
> ack
> 
> > > +		return EXIT_FAILURE;
> > > +	}
> > > +
> > > +	if (socket_.bind(fd) < 0) {
> > > +		LOG({{proxy_name}}Worker, Error) << "IPC socket binding failed";
> > > +		return EXIT_FAILURE;
> > > +	}
> > > +	socket_.readyRead.connect(&readyRead);
> > > +
> > > +	ipa_ = dynamic_cast<{{interface_name}} *>(ipam->createInterface());
> > > +	if (!ipa_) {
> > > +		LOG({{proxy_name}}Worker, Error) << "Failed to create IPA interface instance";
> > > +		return EXIT_FAILURE;
> > > +	}
> > > +{% for method in interface_cb.methods %}
> > > +	ipa_->{{method.mojom_name}}.connect(&{{method.mojom_name}});
> > > +{%- endfor %}
> > > +
> > > +	LOG({{proxy_name}}Worker, Debug) << "Proxy worker successfully started";
> > > +
> > > +	/* \todo upgrade listening loop */
> > 
> > Good point. We should make an EventLoop class at some point.
> 
> I'm pretty sure I copied this from somewhere... I can't remember where.
> 
> > > +	EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
> > > +	while (!exit_)
> > > +		dispatcher->processEvents();
> > > +
> > > +	delete ipa_;
> > > +	socket_.close();
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl
> > > new file mode 100644
> > > index 00000000..37352c1f
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/module_serializer.h.tmpl
> > > @@ -0,0 +1,44 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{%- import "serializer.tmpl" as serializer -%}
> > > +
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) {{ year }}, Google Inc.
> > > + *
> > > + * {{ module_name }}_serializer.h - Image Processing Algorithm data serializer for {{ module_name }}
> > > + *
> > > + * This file is auto-generated. Do not edit.
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__
> > > +#define __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__
> > > +
> > > +#include <libcamera/ipa/{{ module_name }}.h>
> > > +#include <libcamera/ipa/{{ module_name }}_generated.h>
> > > +
> > > +#include "libcamera/internal/control_serializer.h"
> > > +#include "libcamera/internal/ipa_data_serializer.h"
> > > +
> > > +#include <tuple>
> > > +#include <vector>
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DECLARE_CATEGORY(IPADataSerializer)
> > > +{% for struct in structs_nonempty %}
> > > +template<>
> > > +class IPADataSerializer<{{ struct|name_full(namespace_str) }}>
> > > +{
> > > +public:
> > > +{{- serializer.serializer(struct, base_controls, namespace_str) }}
> > > +{%- if struct|has_fd %}
> > > +{{ serializer.deserializer_fd(struct, namespace_str) }}
> > > +{%- else %}
> > > +{{ serializer.deserializer_no_fd(struct, namespace_str) }}
> > > +{%- endif %}
> > > +};
> > > +{% endfor %}
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_INTERNAL_IPA_DATA_SERIALIZER_{{ module_name|upper }}_H__ */
> > 
> > I'll review proxy_functions.tmpl and serializer.tmpl separately, this is
> > growing quite big.
> 
> Okay. Thank you for the reviews so far.
> 
> > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > new file mode 100644
> > > index 00000000..e1f5a20c
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> > > @@ -0,0 +1,185 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{#
> > > + # \brief Generate fuction prototype
> > > + #
> > > + # \param class Class name
> > > + # \param method mojom Method object
> > > + # \param suffix Suffix to append to \a method function name
> > > + # \param need_class_name True to generate class name with function
> > > + # \param override True to generate override tag after the function prototype
> > > + #}
> > > +{%- macro func_sig(class, method, suffix, need_class_name = true, override = false) -%}
> > > +{{method|method_return_value}} {{ class + "::" if need_class_name }}{{method.mojom_name}}{{suffix}}(
> > > +{%- for param in method|method_parameters %}
> > > +	{{param}}{{- "," if not loop.last}}
> > > +{%- endfor -%}
> > > +){{" override" if override}}
> > > +{%- endmacro -%}
> > > +
> > > +{#
> > > + # \brief Generate function body for IPA init() function for thread
> > > + #}
> > > +{%- macro init_thread_body() -%}
> > > +	int ret = ipa_->init(settings);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	proxy_.moveToThread(&thread_);
> > > +
> > > +	return 0;
> > > +{%- endmacro -%}
> > > +
> > > +{#
> > > + # \brief Generate function body for IPA start() function for thread
> > > + #}
> > > +{%- macro start_thread_body() -%}
> > > +	running_ = true;
> > > +	thread_.start();
> > > +
> > > +	return proxy_.invokeMethod(&ThreadProxy::start, ConnectionTypeBlocking);
> > > +{%- endmacro -%}
> > > +
> > > +{#
> > > + # \brief Generate function body for IPA stop() function for thread
> > > + #}
> > > +{%- macro stop_thread_body() -%}
> > > +	if (!running_)
> > > +		return;
> > > +
> > > +	running_ = false;
> > > +
> > > +	proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
> > > +
> > > +	thread_.exit();
> > > +	thread_.wait();
> > > +{%- endmacro -%}
> > > +
> > > +
> > > +{#
> > > + # \brief Serialize multiple objects into data buffer and fd vector
> > > + #
> > > + # Generate code to serialize multiple objects, as specified in \a params
> > > + # (which are the parameters to some function), into \a buf data buffer and
> > > + # \a fds fd vector.
> > > + # This code is meant to be used by the proxy, for serializing prior to IPC calls.
> > > + #}
> > > +{%- macro serialize_call(params, buf, fds, base_controls) %}
> > > +{%- for param in params %}
> > > +	std::vector<uint8_t> {{param.mojom_name}}Buf;
> > > +{%- if param|has_fd %}
> > > +	std::vector<int32_t> {{param.mojom_name}}Fds;
> > > +	std::tie({{param.mojom_name}}Buf, {{param.mojom_name}}Fds) =
> > > +{%- else %}
> > > +	std::tie({{param.mojom_name}}Buf, std::ignore) =
> > > +{%- endif %}
> > > +{%- if param|is_controls %}
> > > +		IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}, {{param.mojom_name}}.infoMap() ? *{{param.mojom_name}}.infoMap() : {{base_controls}}
> > > +{%- else %}
> > > +		IPADataSerializer<{{param|name}}>::serialize({{param.mojom_name}}
> > > +{%- endif %}
> > > +{{- ", &controlSerializer_" if param|needs_control_serializer -}}
> > > +);
> > > +{%- endfor %}
> > > +
> > > +{%- if params|length > 1 %}
> > > +{%- for param in params %}
> > > +	appendUInt<uint32_t>({{buf}}, {{param.mojom_name}}Buf.size());
> > > +{%- if param|has_fd %}
> > > +	appendUInt<uint32_t>({{buf}}, {{param.mojom_name}}Fds.size());
> > > +{%- endif %}
> > > +{%- endfor %}
> > > +{%- endif %}
> > > +
> > > +{%- for param in params %}
> > > +	{{buf}}.insert({{buf}}.end(), {{param.mojom_name}}Buf.begin(), {{param.mojom_name}}Buf.end());
> > > +{%- endfor %}
> > > +
> > > +{%- for param in params %}
> > > +{%- if param|has_fd %}
> > > +	{{fds}}.insert({{fds}}.end(), {{param.mojom_name}}Fds.begin(), {{param.mojom_name}}Fds.end());
> > > +{%- endif %}
> > > +{%- endfor %}
> > > +{%- endmacro -%}
> > > +
> > > +
> > > +{#
> > > + # \brief Deserialize a single object from data buffer and fd vector
> > > + #
> > > + # \param pointer True deserializes the object into a dereferenced pointer
> > > + #
> > > + # Generate code to deserialize a single object, as specified in \a param,
> > > + # from \a buf data buffer and \a fds fd vector.
> > > + # This code is meant to be used by macro deserialize_call.
> > > + #}
> > > +{%- macro deserialize_param(param, pointer, loop, buf, fds) -%}
> > > +{{"*" if pointer}}{{param.mojom_name}} = IPADataSerializer<{{param|name}}>::deserialize(
> > > +	{{buf}}.begin() + {{param.mojom_name}}Start,
> > > +{%- if loop.last %}
> > > +	{{buf}}.end()
> > > +{%- else %}
> > > +	{{buf}}.begin() + {{param.mojom_name}}Start + {{param.mojom_name}}BufSize
> > > +{%- endif -%}
> > > +{{- "," if param|has_fd }}
> > > +{%- if param|has_fd %}
> > > +	{{fds}}.begin() + {{param.mojom_name}}FdStart,
> > > +{%- if loop.last %}
> > > +	{{fds}}.end()
> > > +{%- else %}
> > > +	{{fds}}.begin() + {{param.mojom_name}}FdStart + {{param.mojom_name}}FdsSize
> > > +{%- endif -%}
> > > +{%- endif -%}
> > > +{{- "," if param|needs_control_serializer }}
> > > +{%- if param|needs_control_serializer %}
> > > +	&controlSerializer_
> > > +{%- endif -%}
> > > +);
> > > +{%- endmacro -%}
> > > +
> > > +
> > > +{#
> > > + # \brief Deserialize multiple objects from data buffer and fd vector
> > > + #
> > > + # \param pointer True deserializes objects into pointers, and adds a null check.
> > > + # \param declare True declares the objects in addition to deserialization.
> > > + #
> > > + # Generate code to deserialize multiple objects, as specified in \a params
> > > + # (which are the parameters to some function), from \a buf data buffer and
> > > + # \a fds fd vector.
> > > + # This code is meant to be used by the proxy, for serializing prior to IPC calls.
> > > + #}
> > > +{%- macro deserialize_call(params, buf, fds, pointer = true, declare = false) -%}
> > > +{% set ns = namespace(size_offset = 0) %}
> > > +{%- if params|length > 1 %}
> > > +{%- for param in params %}
> > > +	[[maybe_unused]] size_t {{param.mojom_name}}BufSize = readUInt<uint32_t>({{buf}}, {{ns.size_offset}});
> > > +	{%- set ns.size_offset = ns.size_offset + 4 %}
> > > +{%- if param|has_fd %}
> > > +	[[maybe_unused]] size_t {{param.mojom_name}}FdsSize = readUInt<uint32_t>({{buf}}, {{ns.size_offset}});
> > > +	{%- set ns.size_offset = ns.size_offset + 4 %}
> > > +{%- endif %}
> > > +{%- endfor %}
> > > +{%- endif %}
> > > +{% for param in params %}
> > > +{%- if loop.first %}
> > > +	size_t {{param.mojom_name}}Start = {{ns.size_offset}};
> > > +{%- else %}
> > > +	size_t {{param.mojom_name}}Start = {{loop.previtem.mojom_name}}Start + {{loop.previtem.mojom_name}}BufSize;
> > > +{%- endif %}
> > > +{%- endfor %}
> > > +{% for param in params|with_fds %}
> > > +{%- if loop.first %}
> > > +	size_t {{param.mojom_name}}FdStart = 0;
> > > +{%- elif not loop.last %}
> > > +	size_t {{param.mojom_name}}FdStart = {{loop.previtem.mojom_name}}FdStart + {{loop.previtem.mojom_name}}FdsSize;
> > > +{%- endif %}
> > > +{%- endfor %}
> > > +{% for param in params %}
> > > +	{%- if pointer %}
> > > +	if ({{param.mojom_name}}) {
> > > +{{deserialize_param(param, pointer, loop, buf, fds)|indent(16, True)}}
> > > +	}
> > > +	{%- else %}
> > > +	{{param|name + " " if declare}}{{deserialize_param(param, pointer, loop, buf, fds)|indent(8)}}
> > > +	{%- endif %}
> > > +{% endfor %}
> > > +{%- endmacro -%}
> > > diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> > > new file mode 100644
> > > index 00000000..3c071c4e
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> > > @@ -0,0 +1,276 @@
> > > +{#- SPDX-License-Identifier: LGPL-2.1-or-later -#}
> > > +{# Turn this into a C macro? #}
> > > +{#
> > > + # \brief Verify that there is enough bytes to deserialize
> > > + #
> > > + # Generate code that verifies that \a size is not greater than \a dataSize.
> > > + # Otherwise log an error with \a name and \a typename.
> > > + #}
> > > +{%- macro check_data_size(size, dataSize, name, typename) %}
> > > +		if ({{size}} > {{dataSize}}) {
> > > +			LOG(IPADataSerializer, Error)
> > > +				<< "Failed to deserialize {{name}}: not enough {{typename}}, expected "
> > > +				<< ({{size}}) << ", got " << ({{dataSize}});
> > > +			return ret;
> > > +		}
> > > +{%- endmacro %}
> > > +
> > > +
> > > +{#
> > > + # \brief Serialize some field into return vector
> > > + #
> > > + # Generate code to serialize \a field into ret_data, including size of the
> > > + # field and fds (where appropriate). \a base_controls indicates the
> > > + # default ControlInfoMap in the event that the ControlList does not have one.
> > > + # This code is meant to be used by the IPADataSerializer specialization.
> > > + #}
> > > +{%- macro serializer_field(field, base_controls, namespace, loop) %}
> > > +{%- if field|is_pod or field|is_enum %}
> > > +		std::vector<uint8_t> {{field.mojom_name}};
> > > +		std::tie({{field.mojom_name}}, std::ignore) =
> > > +	{%- if field|is_pod %}
> > > +			IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);
> > > +	{%- elif field|is_enum %}
> > > +			IPADataSerializer<uint{{field|bit_width}}_t>::serialize(data.{{field.mojom_name}}_);
> > > +	{%- endif %}
> > > +		ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > > +{%- elif field|is_fd %}
> > > +		std::vector<uint8_t> {{field.mojom_name}};
> > > +		std::vector<int32_t> {{field.mojom_name}}Fds;
> > > +		std::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =
> > > +			IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_);
> > > +		ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > > +		ret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());
> > > +{%- elif field|is_controls %}
> > > +		if (data.{{field.mojom_name}}_.size() > 0) {
> > > +			std::vector<uint8_t> {{field.mojom_name}};
> > > +			std::tie({{field.mojom_name}}, std::ignore) =
> > > +				IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_,
> > > +					data.{{field.mojom_name}}_.infoMap() ? *data.{{field.mojom_name}}_.infoMap() : {{base_controls}},
> > > +					cs);
> > > +			appendUInt<uint32_t>(ret_data, {{field.mojom_name}}.size());
> > > +			ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > > +		} else {
> > > +			appendUInt<uint32_t>(ret_data, 0);
> > > +		}
> > > +{%- elif field|is_plain_struct or field|is_array or field|is_map %}
> > > +		std::vector<uint8_t> {{field.mojom_name}};
> > > +	{%- if field|has_fd %}
> > > +		std::vector<int32_t> {{field.mojom_name}}Fds;
> > > +		std::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =
> > > +	{%- else %}
> > > +		std::tie({{field.mojom_name}}, std::ignore) =
> > > +	{%- endif %}
> > > +	{%- if field|is_array or field|is_map %}
> > > +			IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}}_, cs);
> > > +	{%- else %}
> > > +			IPADataSerializer<{{field|name_full(namespace)}}>::serialize(data.{{field.mojom_name}}_, cs);
> > > +	{%- endif %}
> > > +		appendUInt<uint32_t>(ret_data, {{field.mojom_name}}.size());
> > > +	{%- if field|has_fd %}
> > > +		appendUInt<uint32_t>(ret_data, {{field.mojom_name}}Fds.size());
> > > +	{%- endif %}
> > > +		ret_data.insert(ret_data.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> > > +	{%- if field|has_fd %}
> > > +		ret_fds.insert(ret_fds.end(), {{field.mojom_name}}Fds.begin(), {{field.mojom_name}}Fds.end());
> > > +	{%- endif %}
> > > +{%- else %}
> > > +		/* Unknown serialization for {{field.mojom_name}}. */
> > > +{%- endif %}
> > > +{%- endmacro %}
> > > +
> > > +
> > > +{#
> > > + # \brief Deserialize some field into return struct
> > > + #
> > > + # Generate code to deserialize \a field into object ret.
> > > + # This code is meant to be used by the IPADataSerializer specialization.
> > > + #}
> > > +{%- macro deserializer_field(field, namespace, loop) %}
> > > +{% if field|is_pod or field|is_enum %}
> > > +	{%- set field_size = (field|bit_width|int / 8)|int %}
> > > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> > > +		{%- if field|is_pod %}
> > > +		ret.{{field.mojom_name}}_ = static_cast<{{field|name}}>(IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}));
> > > +		{%- else %}
> > > +		ret.{{field.mojom_name}}_ = static_cast<{{field|name}}>(IPADataSerializer<uint{{field|bit_width}}_t>::deserialize(m, m + {{field_size}}));
> > > +		{%- endif %}
> > > +	{%- if not loop.last %}
> > > +		m += {{field_size}};
> > > +		dataSize -= {{field_size}};
> > > +	{%- endif %}
> > > +{% elif field|is_fd %}
> > > +	{%- set field_size = 1 %}
> > > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> > > +		ret.{{field.mojom_name}}_ = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs);
> > > +	{%- if not loop.last %}
> > > +		m += {{field_size}};
> > > +		dataSize -= {{field_size}};
> > > +		n += ret.{{field.mojom_name}}_.isValid() ? 1 : 0;
> > > +		fdsSize -= ret.{{field.mojom_name}}_.isValid() ? 1 : 0;
> > > +	{%- endif %}
> > > +{% elif field|is_controls %}
> > > +	{%- set field_size = 4 %}
> > > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}
> > > +		size_t {{field.mojom_name}}Size = readUInt<uint32_t>(m);
> > > +	{%- set field_size = '4 + ' + field.mojom_name + 'Size' -%}
> > > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> > > +		if ({{field.mojom_name}}Size > 0)
> > > +			ret.{{field.mojom_name}}_ =
> > > +				IPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> > > +	{%- if not loop.last %}
> > > +		m += {{field_size}};
> > > +		dataSize -= {{field_size}};
> > > +	{%- endif %}
> > > +{% elif field|is_plain_struct or field|is_array or field|is_map%}
> > > +	{%- set field_size = 4 %}
> > > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'Size', 'data') }}
> > > +		size_t {{field.mojom_name}}Size = readUInt<uint32_t>(m);
> > > +	{%- if field|has_fd %}
> > > +	{%- set field_size = 8 %}
> > > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name + 'FdsSize', 'data') }}
> > > +		size_t {{field.mojom_name}}FdsSize = readUInt<uint32_t>(m + 4);
> > > +		{{- check_data_size(field.mojom_name + 'FdsSize', 'fdsSize', field.mojom_name, 'fds') }}
> > > +	{%- endif %}
> > > +	{%- set field_size = field|has_fd|choose('8 + ', '4 + ') + field.mojom_name + 'Size' -%}
> > > +		{{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data') }}
> > > +		ret.{{field.mojom_name}}_ =
> > > +	{%- if field|has_fd and (field|is_array or field|is_map) %}
> > > +			IPADataSerializer<{{field|name}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);
> > > +	{%- elif field|has_fd and (not (field|is_array or field|is_map)) %}
> > > +			IPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 8, m + 8 + {{field.mojom_name}}Size, n, n + {{field.mojom_name}}FdsSize, cs);
> > > +	{%- elif (not field|has_fd) and (field|is_array or field|is_map) %}
> > > +			IPADataSerializer<{{field|name}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> > > +	{%- else %}
> > > +			IPADataSerializer<{{field|name_full(namespace)}}>::deserialize(m + 4, m + 4 + {{field.mojom_name}}Size, cs);
> > > +	{%- endif %}
> > > +	{%- if not loop.last %}
> > > +		m += {{field_size}};
> > > +		dataSize -= {{field_size}};
> > > +	{%- if field|has_fd %}
> > > +		n += {{field.mojom_name}}FdsSize;
> > > +		fdsSize -= {{field.mojom_name}}FdsSize;
> > > +	{%- endif %}
> > > +	{%- endif %}
> > > +{% else %}
> > > +		/* Unknown deserialization for {{field.mojom_name}}. */
> > > +{%- endif %}
> > > +{%- endmacro %}
> > > +
> > > +
> > > +{#
> > > + # \brief Serialize a struct
> > > + #
> > > + # Generate code for IPADataSerializer specialization, for serializing
> > > + # \a struct. \a base_controls indicates the default ControlInfoMap
> > > + # in the event that the ControlList does not have one.
> > > + #}
> > > +{%- macro serializer(struct, base_controls, namespace) %}
> > > +	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> > > +	serialize(const {{ struct|name_full(namespace) }} &data,
> > > +{%- if struct|needs_control_serializer %}
> > > +		  ControlSerializer *cs)
> > > +{%- else %}
> > > +		  [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +{%- endif %}
> > > +	{
> > > +		std::vector<uint8_t> ret_data;
> > > +{%- if struct|has_fd %}
> > > +		std::vector<int32_t> ret_fds;
> > > +{%- endif %}
> > > +{%- for field in struct.fields %}
> > > +{{serializer_field(field, base_controls, namespace, loop)}}
> > > +{%- endfor %}
> > > +{% if struct|has_fd %}
> > > +		return {ret_data, ret_fds};
> > > +{%- else %}
> > > +		return {ret_data, {}};
> > > +{%- endif %}
> > > +	}
> > > +{%- endmacro %}
> > > +
> > > +
> > > +{#
> > > + # \brief Deserialize a struct that has fds
> > > + #
> > > + # Generate code for IPADataSerializer specialization, for deserializing
> > > + # \a struct, in the case that \a struct has file descriptors.
> > > + #}
> > > +{%- macro deserializer_fd(struct, namespace) %}
> > > +	static {{ struct|name_full(namespace) }}
> > > +	deserialize(std::vector<uint8_t> &data,
> > > +		    std::vector<int32_t> &fds,
> > > +{%- if struct|needs_control_serializer %}
> > > +		    ControlSerializer *cs)
> > > +{%- else %}
> > > +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +{%- endif %}
> > > +	{
> > > +		{{struct|name_full(namespace)}} ret;
> > > +		std::vector<uint8_t>::iterator m = data.begin();
> > > +		std::vector<int32_t>::iterator n = fds.begin();
> > > +
> > > +		size_t dataSize = data.size();
> > > +		size_t fdsSize = fds.size();
> > > +{%- for field in struct.fields -%}
> > > +{{deserializer_field(field, namespace, loop)}}
> > > +{%- endfor %}
> > > +		return ret;
> > > +	}
> > > +
> > > +	static {{ struct|name_full(namespace) }}
> > > +	deserialize(std::vector<uint8_t>::iterator data_it1,
> > > +		    std::vector<uint8_t>::iterator data_it2,
> > > +		    std::vector<int32_t>::iterator fds_it1,
> > > +		    std::vector<int32_t>::iterator fds_it2,
> > > +{%- if struct|needs_control_serializer %}
> > > +		    ControlSerializer *cs)
> > > +{%- else %}
> > > +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +{%- endif %}
> > > +	{
> > > +		std::vector<uint8_t> data(data_it1, data_it2);
> > > +		std::vector<int32_t> fds(fds_it1, fds_it2);
> > > +		return IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, fds, cs);
> > > +	}
> > > +{%- endmacro %}
> > > +
> > > +
> > > +{#
> > > + # \brief Deserialize a struct that has no fds
> > > + #
> > > + # Generate code for IPADataSerializer specialization, for deserializing
> > > + # \a struct, in the case that \a struct does not have file descriptors.
> > > + #}
> > > +{%- macro deserializer_no_fd(struct, namespace) %}
> > > +	static {{ struct|name_full(namespace) }}
> > > +	deserialize(std::vector<uint8_t> &data,
> > > +{%- if struct|needs_control_serializer %}
> > > +		    ControlSerializer *cs)
> > > +{%- else %}
> > > +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +{%- endif %}
> > > +	{
> > > +		{{struct|name_full(namespace)}} ret;
> > > +		std::vector<uint8_t>::iterator m = data.begin();
> > > +
> > > +		size_t dataSize = data.size();
> > > +{%- for field in struct.fields -%}
> > > +{{deserializer_field(field, namespace, loop)}}
> > > +{%- endfor %}
> > > +		return ret;
> > > +	}
> > > +
> > > +	static {{ struct|name_full(namespace) }}
> > > +	deserialize(std::vector<uint8_t>::iterator data_it1,
> > > +		    std::vector<uint8_t>::iterator data_it2,
> > > +{%- if struct|needs_control_serializer %}
> > > +		    ControlSerializer *cs)
> > > +{%- else %}
> > > +		    [[maybe_unused]] ControlSerializer *cs = nullptr)
> > > +{%- endif %}
> > > +	{
> > > +		std::vector<uint8_t> data(data_it1, data_it2);
> > > +		return IPADataSerializer<{{struct|name_full(namespace)}}>::deserialize(data, cs);
> > > +	}
> > > +{%- endmacro %}
> > > diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py
> > > new file mode 100644
> > > index 00000000..a9269c82
> > > --- /dev/null
> > > +++ b/utils/ipc/generators/mojom_libcamera_generator.py
> > > @@ -0,0 +1,461 @@
> > > +'''Generates libcamera files from a mojom.Module.'''
> > > +
> > > +import argparse
> > > +import ast
> > > +import datetime
> > > +import contextlib
> > > +import os
> > > +import re
> > > +import shutil
> > > +import sys
> > > +import tempfile
> > > +
> > > +from jinja2 import contextfilter
> > > +
> > > +import mojom.fileutil as fileutil
> > > +import mojom.generate.generator as generator
> > > +import mojom.generate.module as mojom
> > > +from mojom.generate.template_expander import UseJinja
> > > +
> > > +
> > > +GENERATOR_PREFIX = 'libcamera'
> > > +
> > > +_kind_to_cpp_type = {
> > > +    mojom.BOOL:   'bool',
> > > +    mojom.INT8:   'int8_t',
> > > +    mojom.UINT8:  'uint8_t',
> > > +    mojom.INT16:  'int16_t',
> > > +    mojom.UINT16: 'uint16_t',
> > > +    mojom.INT32:  'int32_t',
> > > +    mojom.UINT32: 'uint32_t',
> > > +    mojom.FLOAT:  'float',
> > > +    mojom.INT64:  'int64_t',
> > > +    mojom.UINT64: 'uint64_t',
> > > +    mojom.DOUBLE: 'double',
> > > +}
> > > +
> > > +_bit_widths = {
> > > +    mojom.BOOL:   '8',
> > > +    mojom.INT8:   '8',
> > > +    mojom.UINT8:  '8',
> > > +    mojom.INT16:  '16',
> > > +    mojom.UINT16: '16',
> > > +    mojom.INT32:  '32',
> > > +    mojom.UINT32: '32',
> > > +    mojom.FLOAT:  '32',
> > > +    mojom.INT64:  '64',
> > > +    mojom.UINT64: '64',
> > > +    mojom.DOUBLE: '64',
> > > +}
> > > +
> > > +def ModuleName(path):
> > > +    return path.split('/')[-1].split('.')[0]
> > > +
> > > +def ModuleClassName(module):
> > > +    s = re.sub(r'^IPA', '',  module.interfaces[0].mojom_name)
> > > +    return re.sub(r'Interface$', '', s)
> > > +
> > > +def UpperCamelCase(name):
> > > +    return ''.join([x.capitalize() for x in generator.SplitCamelCase(name)])
> > > +
> > > +def CamelCase(name):
> > > +    uccc = UpperCamelCase(name)
> > > +    return uccc[0].lower() + uccc[1:]
> > > +
> > > +def ConstantStyle(name):
> > > +    return generator.ToUpperSnakeCase(name)
> > > +
> > > +def Choose(cond, t, f):
> > > +    return t if cond else f
> > > +
> > > +def CommaSep(l):
> > > +    return ', '.join([m for m in l])
> > > +
> > > +def ParamsCommaSep(l):
> > > +    return ', '.join([m.mojom_name for m in l])
> > > +
> > > +def GetDefaultValue(element):
> > > +    if element.default is not None:
> > > +        return element.default
> > > +    if type(element.kind) == mojom.Kind:
> > > +        return '0'
> > > +    if mojom.IsEnumKind(element.kind):
> > > +        return f'static_cast<{element.kind.mojom_name}>(0)'
> > > +    if (isinstance(element.kind, mojom.Struct) and
> > > +       element.kind.mojom_name == 'FileDescriptor'):
> > > +        return '-1'
> > > +    return ''
> > > +
> > > +def WithDefaultValues(element):
> > > +    return [x for x in element if HasDefaultValue(x)]
> > > +
> > > +def WithFds(element):
> > > +    return [x for x in element if HasFd(x)]
> > > +
> > > +def HasDefaultValue(element):
> > > +    return GetDefaultValue(element) != ''
> > > +
> > > +def HasDefaultFields(element):
> > > +    return True in [HasDefaultValue(x) for x in element.fields]
> > > +
> > > +def GetAllTypes(element):
> > > +    if mojom.IsArrayKind(element):
> > > +        return GetAllTypes(element.kind)
> > > +    if mojom.IsMapKind(element):
> > > +        return GetAllTypes(element.key_kind) + GetAllTypes(element.value_kind)
> > > +    if isinstance(element, mojom.Parameter):
> > > +        return GetAllTypes(element.kind)
> > > +    if mojom.IsEnumKind(element):
> > > +        return [element.mojom_name]
> > > +    if not mojom.IsStructKind(element):
> > > +        return [element.spec]
> > > +    if len(element.fields) == 0:
> > > +        return [element.mojom_name]
> > > +    ret = [GetAllTypes(x.kind) for x in element.fields]
> > > +    ret = [x for sublist in ret for x in sublist]
> > > +    return list(set(ret))
> > > +
> > > +def GetAllAttrs(element):
> > > +    if mojom.IsArrayKind(element):
> > > +        return GetAllAttrs(element.kind)
> > > +    if mojom.IsMapKind(element):
> > > +        return {**GetAllAttrs(element.key_kind), **GetAllAttrs(element.value_kind)}
> > > +    if isinstance(element, mojom.Parameter):
> > > +        return GetAllAttrs(element.kind)
> > > +    if mojom.IsEnumKind(element):
> > > +        return element.attributes if element.attributes is not None else {}
> > > +    if mojom.IsStructKind(element) and len(element.fields) == 0:
> > > +        return element.attributes if element.attributes is not None else {}
> > > +    if not mojom.IsStructKind(element):
> > > +        return {}
> > > +    attrs = [(x.attributes) for x in element.fields]
> > > +    ret = {}
> > > +    for d in attrs:
> > > +        if d is not None:
> > > +            ret = {**ret, **d}
> > > +    return ret
> > > +
> > > +def NeedsControlSerializer(element):
> > > +    types = GetAllTypes(element)
> > > +    return "ControlList" in types or "ControlInfoMap" in types
> > > +
> > > +def HasFd(element):
> > > +    attrs = GetAllAttrs(element)
> > > +    if isinstance(element, mojom.Kind):
> > > +        types = GetAllTypes(element)
> > > +    else:
> > > +        types = GetAllTypes(element.kind)
> > > +    return "FileDescriptor" in types or (attrs is not None and "hasFd" in attrs)
> > > +
> > > +def MethodInputHasFd(method):
> > > +    if len([x for x in method.parameters if HasFd(x)]) > 0:
> > > +        return True
> > > +    return False
> > > +
> > > +def MethodOutputHasFd(method):
> > > +    if (MethodReturnValue(method) != 'void' or
> > > +        method.response_parameters is None):
> > > +        return False
> > > +    if len([x for x in method.parameters if HasFd(x)]) > 0:
> > > +        return True
> > > +    return False
> > > +
> > > +def MethodParamInputs(method):
> > > +    return method.parameters
> > > +
> > > +def MethodParamOutputs(method):
> > > +    if (MethodReturnValue(method) != 'void' or
> > > +        method.response_parameters is None):
> > > +        return []
> > > +    return method.response_parameters
> > > +
> > > +def MethodParamNames(method):
> > > +    params = []
> > > +    for param in method.parameters:
> > > +        params.append(param.mojom_name)
> > > +    if MethodReturnValue(method) == 'void':
> > > +        if method.response_parameters is None:
> > > +            return params
> > > +        for param in method.response_parameters:
> > > +            params.append(param.mojom_name)
> > > +    return params
> > > +
> > > +def MethodParameters(method):
> > > +    params = []
> > > +    for param in method.parameters:
> > > +        params.append('const %s %s%s' % (GetNameForElement(param),
> > > +                                         ('&' if not IsPod(param) else ''),
> > > +                                         param.mojom_name))
> > > +    if MethodReturnValue(method) == 'void':
> > > +        if method.response_parameters is None:
> > > +            return params
> > > +        for param in method.response_parameters:
> > > +            params.append(f'{GetNameForElement(param)} *{param.mojom_name}')
> > > +    return params
> > > +
> > > +def MethodReturnValue(method):
> > > +    if method.response_parameters is None:
> > > +        return 'void'
> > > +    if len(method.response_parameters) == 1 and IsPod(method.response_parameters[0]):
> > > +        return GetNameForElement(method.response_parameters[0])
> > > +    return 'void'
> > > +
> > > +def IsAsync(method):
> > > +    # callbacks are always async
> > > +    if re.match("^IPA.*CallbackInterface$", method.interface.mojom_name):
> > > +        return True
> > > +    elif re.match("^IPA.*Interface$", method.interface.mojom_name):
> > > +        if method.attributes is None:
> > > +            return False
> > > +        elif 'async' in method.attributes and method.attributes['async']:
> > > +            return True
> > > +    return False
> > > +
> > > +def IsArray(element):
> > > +    return mojom.IsArrayKind(element.kind)
> > > +
> > > +def IsControls(element):
> > > +    return mojom.IsStructKind(element.kind) and (element.kind.mojom_name == "ControlList" or
> > > +                                                 element.kind.mojom_name == "ControlInfoMap")
> > > +
> > > +def IsEnum(element):
> > > +    return mojom.IsEnumKind(element.kind)
> > > +
> > > +def IsFd(element):
> > > +    return mojom.IsStructKind(element.kind) and element.kind.mojom_name == "FileDescriptor"
> > > +
> > > +def IsMap(element):
> > > +    return mojom.IsMapKind(element.kind)
> > > +
> > > +def IsPlainStruct(element):
> > > +    return mojom.IsStructKind(element.kind) and not IsControls(element) and not IsFd(element)
> > > +
> > > +def IsPod(element):
> > > +    return element.kind in _kind_to_cpp_type
> > > +
> > > +def BitWidth(element):
> > > +    if element.kind in _bit_widths:
> > > +        return _bit_widths[element.kind]
> > > +    if mojom.IsEnumKind(element.kind):
> > > +        return '32'
> > > +    return ''
> > > +
> > > +def GetNameForElement(element):
> > > +    if (mojom.IsEnumKind(element) or
> > > +        mojom.IsInterfaceKind(element) or
> > > +        mojom.IsStructKind(element)):
> > > +        return element.mojom_name
> > > +    if (mojom.IsArrayKind(element)):
> > > +        elem_name = GetNameForElement(element.kind)
> > > +        return f'std::vector<{elem_name}>'
> > > +    if (mojom.IsMapKind(element)):
> > > +        key_name = GetNameForElement(element.key_kind)
> > > +        value_name = GetNameForElement(element.value_kind)
> > > +        return f'std::map<{key_name}, {value_name}>'
> > > +    if isinstance(element, (mojom.Field, mojom.Method, mojom.Parameter)):
> > > +        if (mojom.IsArrayKind(element.kind) or mojom.IsMapKind(element.kind)):
> > > +            return GetNameForElement(element.kind)
> > > +        if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):
> > > +            return _kind_to_cpp_type[element.kind]
> > > +        return element.kind.mojom_name
> > > +    if isinstance(element,  mojom.EnumValue):
> > > +        return (GetNameForElement(element.enum) + '.' +
> > > +                ConstantStyle(element.name))
> > > +    if isinstance(element, (mojom.NamedValue,
> > > +                            mojom.Constant,
> > > +                            mojom.EnumField)):
> > > +        return ConstantStyle(element.name)
> > > +    if (hasattr(element, '__hash__') and element in _kind_to_cpp_type):
> > > +        return _kind_to_cpp_type[element]
> > > +    if (hasattr(element, 'kind') and element.kind in _kind_to_cpp_type):
> > > +        return _kind_to_cpp_type[element.kind]
> > > +    if (hasattr(element, 'spec')):
> > > +        return element.spec.split(':')[-1]
> > > +    if (mojom.IsInterfaceRequestKind(element) or
> > > +        mojom.IsAssociatedKind(element) or
> > > +        mojom.IsPendingRemoteKind(element) or
> > > +        mojom.IsPendingReceiverKind(element) or
> > > +        mojom.IsUnionKind(element)):
> > > +        raise Exception('Unsupported element: %s' % element)
> > > +    raise Exception('Unexpected element: %s' % element)
> > > +
> > > +def GetFullNameForElement(element, namespace_str):
> > > +    name = GetNameForElement(element)
> > > +    if namespace_str == '':
> > > +        return name
> > > +    return f'{namespace_str}::{name}'
> > > +
> > > +def ValidateZeroLength(l, s, cap=True):
> > > +    if len(l) > 0:
> > > +        raise Exception(f'{s.capitalize() if cap else s} should be empty')
> > > +
> > > +def ValidateSingleLength(l, s, cap=True):
> > > +    if len(l) > 1:
> > > +        raise Exception(f'Only one {s} allowed')
> > > +    if len(l) < 1:
> > > +        raise Exception(f'{s.capitalize() if cap else s} is required')
> > > +
> > > +def ValidateInterfaces(interfaces):
> > > +    # Validate presence of main interface
> > > +    intf = [x for x in interfaces
> > > +            if re.match("^IPA.*Interface", x.mojom_name) and
> > > +               not re.match("^IPA.*CallbackInterface", x.mojom_name)]
> > > +    ValidateSingleLength(intf, 'main interface')
> > > +
> > > +    # Validate presence of callback interface
> > > +    cb = [x for x in interfaces if re.match("^IPA.*CallbackInterface", x.mojom_name)]
> > > +    ValidateSingleLength(cb, 'callback interface')
> > > +
> > > +    # Validate required main interface functions
> > > +    intf = intf[0]
> > > +    f_init  = [x for x in intf.methods if x.mojom_name == 'init']
> > > +    f_start = [x for x in intf.methods if x.mojom_name == 'start']
> > > +    f_stop  = [x for x in intf.methods if x.mojom_name == 'stop']
> > > +
> > > +    ValidateSingleLength(f_init, 'init()', False)
> > > +    ValidateSingleLength(f_start, 'start()', False)
> > > +    ValidateSingleLength(f_stop, 'stop()', False)
> > > +
> > > +    f_init  = f_init[0]
> > > +    f_start = f_start[0]
> > > +    f_stop  = f_stop[0]
> > > +
> > > +    # Validate parameters to init()
> > > +    ValidateSingleLength(f_init.parameters, 'input parameter to init()')
> > > +    ValidateSingleLength(f_init.response_parameters, 'output parameter from init()')
> > > +    if f_init.parameters[0].kind.mojom_name != 'IPASettings':
> > > +        raise Exception('init() must have single IPASettings input parameter')
> > > +    if f_init.response_parameters[0].kind.spec != 'i32':
> > > +        raise Exception('init() must have single int32 output parameter')
> > > +
> > > +    # Validate parameters to start()
> > > +    ValidateZeroLength(f_start.parameters, 'input parameter to start()')
> > > +    ValidateSingleLength(f_start.response_parameters, 'output parameter from start()')
> > > +    if f_start.response_parameters[0].kind.spec != 'i32':
> > > +        raise Exception('start() must have single int32 output parameter')
> > > +
> > > +    # Validate parameters to stop()
> > > +    ValidateZeroLength(f_stop.parameters, 'input parameter to stop()')
> > > +    ValidateZeroLength(f_stop.parameters, 'output parameter from stop()')
> > > +
> > > +class Generator(generator.Generator):
> > > +
> > > +    @staticmethod
> > > +    def GetTemplatePrefix():
> > > +        return 'libcamera_templates'
> > > +
> > > +    def GetFilters(self):
> > > +        libcamera_filters = {
> > > +            'all_types': GetAllTypes,
> > > +            'bit_width': BitWidth,
> > > +            'choose': Choose,
> > > +            'comma_sep': CommaSep,
> > > +            'default_value': GetDefaultValue,
> > > +            'has_default_fields': HasDefaultFields,
> > > +            'has_fd': HasFd,
> > > +            'is_async': IsAsync,
> > > +            'is_array': IsArray,
> > > +            'is_controls': IsControls,
> > > +            'is_enum': IsEnum,
> > > +            'is_fd': IsFd,
> > > +            'is_map': IsMap,
> > > +            'is_plain_struct': IsPlainStruct,
> > > +            'is_pod': IsPod,
> > > +            'method_input_has_fd': MethodInputHasFd,
> > > +            'method_output_has_fd': MethodOutputHasFd,
> > > +            'method_param_names': MethodParamNames,
> > > +            'method_param_inputs': MethodParamInputs,
> > > +            'method_param_outputs': MethodParamOutputs,
> > > +            'method_parameters': MethodParameters,
> > > +            'method_return_value': MethodReturnValue,
> > > +            'name': GetNameForElement,
> > > +            'name_full': GetFullNameForElement,
> > > +            'needs_control_serializer': NeedsControlSerializer,
> > > +            'params_comma_sep': ParamsCommaSep,
> > > +            'with_default_values': WithDefaultValues,
> > > +            'with_fds': WithFds,
> > > +        }
> > > +        return libcamera_filters
> > > +
> > > +    def _GetJinjaExports(self):
> > > +        return {
> > > +            'base_controls': '%s::Controls' % ModuleClassName(self.module),
> > > +            'cmd_enum_name': '_%sCMD' % ModuleClassName(self.module),
> > 
> > s/CMD/Command/ ?
> > 
> > > +            'enums': self.module.enums,
> > > +            'has_array': len([x for x in self.module.kinds.keys() if x[0] == 'a']) > 0,
> > > +            'has_map': len([x for x in self.module.kinds.keys() if x[0] == 'm']) > 0,
> > > +            'has_namespace': self.module.mojom_namespace != '',
> > > +            'imports': self.module.imports,
> > > +            'interface_cb': self.module.interfaces[1],
> > > +            'interface_main': self.module.interfaces[0],
> > > +            'interface_name': 'IPA%sInterface' % ModuleClassName(self.module),
> > > +            'ipc_name': 'IPAIPCUnixSocket',
> > > +            'kinds': self.module.kinds,
> > > +            'module': self.module,
> > > +            'module_name': ModuleName(self.module.path),
> > > +            'module_class_name': ModuleClassName(self.module),
> > > +            'namespace': self.module.mojom_namespace.split('.'),
> > > +            'namespace_str': self.module.mojom_namespace.replace('.', '::') if
> > > +                             self.module.mojom_namespace is not None else '',
> > > +            'proxy_name': 'IPAProxy%s' % ModuleClassName(self.module),
> > > +            'proxy_worker_name': 'IPAProxy%sWorker' % ModuleClassName(self.module),
> > > +            'structs_nonempty': [x for x in self.module.structs if len(x.fields) > 0],
> > > +            'year': datetime.datetime.now().year,
> > > +        }
> > > +
> > > +
> > > +    @UseJinja('module_generated.h.tmpl')
> > > +    def _GenerateDataHeader(self):
> > > +        return self._GetJinjaExports()
> > > +
> > > +    @UseJinja('module_serializer.h.tmpl')
> > > +    def _GenerateSerializer(self):
> > > +        return self._GetJinjaExports()
> > > +
> > > +    @UseJinja('module_ipa_proxy.cpp.tmpl')
> > > +    def _GenerateProxyCpp(self):
> > > +        return self._GetJinjaExports()
> > > +
> > > +    @UseJinja('module_ipa_proxy.h.tmpl')
> > > +    def _GenerateProxyHeader(self):
> > > +        return self._GetJinjaExports()
> > > +
> > > +    @UseJinja('module_ipa_proxy_worker.cpp.tmpl')
> > > +    def _GenerateProxyWorker(self):
> > > +        return self._GetJinjaExports()
> > > +
> > > +    def GenerateFiles(self, unparsed_args):
> > > +        parser = argparse.ArgumentParser()
> > > +        parser.add_argument('--libcamera_generate_header',       action='store_true')
> > > +        parser.add_argument('--libcamera_generate_serializer',   action='store_true')
> > > +        parser.add_argument('--libcamera_generate_proxy_cpp',    action='store_true')
> > > +        parser.add_argument('--libcamera_generate_proxy_h',      action='store_true')
> > > +        parser.add_argument('--libcamera_generate_proxy_worker', action='store_true')
> > > +        parser.add_argument('--libcamera_output_path')
> > > +        args = parser.parse_args(unparsed_args)
> > > +
> > > +        ValidateInterfaces(self.module.interfaces)
> > > +
> > > +        fileutil.EnsureDirectoryExists(os.path.dirname(args.libcamera_output_path))
> > > +
> > > +        module_name = ModuleName(self.module.path)
> > > +
> > > +        if args.libcamera_generate_header:
> > > +            self.Write(self._GenerateDataHeader(),
> > > +                       args.libcamera_output_path)
> > > +
> > > +        if args.libcamera_generate_serializer:
> > > +            self.Write(self._GenerateSerializer(),
> > > +                       args.libcamera_output_path)
> > > +
> > > +        if args.libcamera_generate_proxy_cpp:
> > > +            self.Write(self._GenerateProxyCpp(),
> > > +                       args.libcamera_output_path)
> > > +
> > > +        if args.libcamera_generate_proxy_h:
> > > +            self.Write(self._GenerateProxyHeader(),
> > > +                       args.libcamera_output_path)
> > > +
> > > +        if args.libcamera_generate_proxy_worker:
> > > +            self.Write(self._GenerateProxyWorker(),
> > > +                       args.libcamera_output_path)