[libcamera-devel] [PATCH 2/6] ipa: proxy_worker: Reset ControlSerializer on worker

[Kieran Bingham]

On 01/09/2021 15:37, Jacopo Mondi wrote:
> When running the IPA in isolated mode, each side of the IPC boundary
> has an instance of the ControlSerializer class which is used to
> serializer/deserialize controls before transmitting them on the wire.
> 
> The IPAProxyWorker, which creates and manages the process the IPA runs into,
> does not reset its ControlSerializer upon an IPA::configure() call, while
> the IPAProxy does so, effectively creating a misalignment between the
> two sides of the fence.
> 
> This obviously creates issues as one side of the IPC runs with a
> populated and possibly stale cache of ControlInfoMap references, while the
> other side gets reset every time a new configuration is applied to the
> Camera.
> 
> Fix that by resetting the IPAProxyWorker ControlSerializer on an
> IPA configure() call.
> 
> This change fixes an issue which is easily triggered  by running two
> consecutive capture sessions with the IPA running in isolated mode:
> ERROR Serializer control_serializer.cpp:520 Can't deserialize ControlList: unknown ControlInfoMap
> 
> Fixes: 7832e19a599e ("utils: ipc: add templates for code generation for IPC mechanism")

Is it also technically fixing the patch that added the reset on the
other side? as that's when the misalignment begins?



> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl      | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> 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
> index 8a88bd467da7..22cbc15c5eff 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> @@ -79,6 +79,10 @@ public:
>  
>  {% for method in interface_main.methods %}
>  		case {{cmd_enum_name}}::{{method.mojom_name|cap}}: {
> +
> +{%- if method.mojom_name == "configure" %}
> +		controlSerializer_.reset();

one more level indentation >> ?

It looks like we're "inside" this case statement, so it should be one
level deeper.


> +{%- endif %}


Wow, these templates are hard to parse :-)

But ... it looks like this is right ;-)

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>  		{{proxy_funcs.deserialize_call(method|method_param_inputs, '_ipcMessage.data()', '_ipcMessage.fds()', false, true)|indent(8, true)}}
>  {% for param in method|method_param_outputs %}
>  			{{param|name}} {{param.mojom_name}};
>