| Message ID | 20210708082145.122160-3-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Thu, Jul 08, 2021 at 01:51:45PM +0530, 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. Oh, I didn't notice that this was the issue. If it is, then this patch ought to fix the issue indeed. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=58 > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > .../generators/libcamera_templates/module_ipa_proxy.cpp.tmpl | 4 ++++ > 1 file changed, 4 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..3d27067a 100644 > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > @@ -125,6 +125,10 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > {% for method in interface_main.methods %} > {{proxy_funcs.func_sig(proxy_name, method)}} > { > +{%- if method.mojom_name == "configure" %} > + controlSerializer_.reset(); > +{%- endif %} > + I don't think we need to run this when we're not isolated, so probably moving this to below {{proxy_funcs.func_sig(proxy_name, method, "IPC")}} is better. I haven't tested it, but with the suggested change, looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > if (isolate_) > {{"return " if method|method_return_value != "void"}}{{method.mojom_name}}IPC( > {%- for param in method|method_param_names -%} > -- > 2.31.1 >
Hi Paul, On 7/8/21 3:35 PM, paul.elder@ideasonboard.com wrote: > Hi Umang, > > On Thu, Jul 08, 2021 at 01:51:45PM +0530, 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. > Oh, I didn't notice that this was the issue. If it is, then this patch > ought to fix the issue indeed. > >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=58 >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> .../generators/libcamera_templates/module_ipa_proxy.cpp.tmpl | 4 ++++ >> 1 file changed, 4 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..3d27067a 100644 >> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl >> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl >> @@ -125,6 +125,10 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) >> {% for method in interface_main.methods %} >> {{proxy_funcs.func_sig(proxy_name, method)}} >> { >> +{%- if method.mojom_name == "configure" %} >> + controlSerializer_.reset(); >> +{%- endif %} >> + > I don't think we need to run this when we're not isolated, so probably > moving this to below > > {{proxy_funcs.func_sig(proxy_name, method, "IPC")}} > > is better. Okay, as I understand from broad level, no serialization/de-serialization happens during non-isolated mode, so ControlSerializer is entirely out the picture in that case. > > I haven't tested it, but with the suggested change, looks good to me. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > >> if (isolate_) >> {{"return " if method|method_return_value != "void"}}{{method.mojom_name}}IPC( >> {%- for param in method|method_param_names -%} >> -- >> 2.31.1 >>
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..3d27067a 100644 --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl @@ -125,6 +125,10 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) {% for method in interface_main.methods %} {{proxy_funcs.func_sig(proxy_name, method)}} { +{%- if method.mojom_name == "configure" %} + controlSerializer_.reset(); +{%- endif %} + if (isolate_) {{"return " if method|method_return_value != "void"}}{{method.mojom_name}}IPC( {%- for param in method|method_param_names -%}
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> --- .../generators/libcamera_templates/module_ipa_proxy.cpp.tmpl | 4 ++++ 1 file changed, 4 insertions(+)