[libcamera-devel,v2,2/2] utils: ipc: proxy: Reset ControlSerializer during IPA configure
diff mbox series

Message ID 20210708105405.147172-3-umang.jain@ideasonboard.com
State Accepted
Delegated to: Umang Jain
Headers show
Series
  • utils: ipc: proxy: Always reset ControlSerializer during IPA configure
Related show

Commit Message

Umang Jain July 8, 2021, 10:54 a.m. UTC
ControlSerializer should be reset during IPA (re)configuration,
so that it doesn't look up stale deserialized cache built from
consecutive previous runs. This is already recommended in
ControlSerializer docs but the implementation seems missing.

The stale cache lookup seems to the core issue with Bug #58.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=58
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 .../generators/libcamera_templates/module_ipa_proxy.cpp.tmpl   | 3 +++
 1 file changed, 3 insertions(+)

Comments

Kieran Bingham July 8, 2021, 3:41 p.m. UTC | #1
Hi Umang,

On 08/07/2021 11:54, Umang Jain wrote:
> ControlSerializer should be reset during IPA (re)configuration,
> so that it doesn't look up stale deserialized cache built from
> consecutive previous runs. This is already recommended in
> ControlSerializer docs but the implementation seems missing.
> 
> The stale cache lookup seems to the core issue with Bug #58.

Yikes. I guess this is also partially related to
https://bugs.libcamera.org/show_bug.cgi?id=31, but that is showing a
slightly different symptom of the effects of having to synchronise
control lists across the IPC.

> Bug: https://bugs.libcamera.org/show_bug.cgi?id=58
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

Tested locally, and fixes things as far as I can tell.

Indeed there are further issues that appear after this related to an fd
leak, but they are separate and I do not believe caused by this patch
(but only visible when the system can run long enough thanks to this fix).

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

> ---
>  .../generators/libcamera_templates/module_ipa_proxy.cpp.tmpl   | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> index 5a64fe9c..a4e008c7 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -184,6 +184,9 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
>  
>  {{proxy_funcs.func_sig(proxy_name, method, "IPC")}}
>  {
> +{%- if method.mojom_name == "configure" %}
> +	controlSerializer_.reset();
> +{%- endif %}
>  {%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != "void" %}
>  {%- set cmd = cmd_enum_name + "::" + method.mojom_name|cap %}
>  	IPCMessage::Header _header = { static_cast<uint32_t>({{cmd}}), seq_++ };
>

Patch
diff mbox series

diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
index 5a64fe9c..a4e008c7 100644
--- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
+++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
@@ -184,6 +184,9 @@  void {{proxy_name}}::recvMessage(const IPCMessage &data)
 
 {{proxy_funcs.func_sig(proxy_name, method, "IPC")}}
 {
+{%- if method.mojom_name == "configure" %}
+	controlSerializer_.reset();
+{%- endif %}
 {%- set has_output = true if method|method_param_outputs|length > 0 or method|method_return_value != "void" %}
 {%- set cmd = cmd_enum_name + "::" + method.mojom_name|cap %}
 	IPCMessage::Header _header = { static_cast<uint32_t>({{cmd}}), seq_++ };