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

[Laurent Pinchart]

On Fri, Sep 03, 2021 at 09:11:30PM +0100, Kieran Bingham wrote:
> On 03/09/2021 10:16, Jacopo Mondi wrote:
> > On Thu, Sep 02, 2021 at 03:05:23PM +0100, Kieran Bingham wrote:
> >> 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?
> > 
> > The 'other side' was reset from the very beginning.
> > 
> > To be honest I would question the usage of Fixes in libcamera, as we
> > don't have versions to backport to, but I added this mosotly for cargo
> > cult.
> > 
> > Iff we want to keep using Fixes I think that's the opportune commit to
> > point to, as this patch should be ideally fixed-up in that one (if we
> > could travel back in time)

That's fine with me.

> I don't think we have any requirement to add a Fixes tag, except it
> helps promote awareness of /why/ or /what/ is being fixed.
> 
> It also helps provide context in some occasions in case the fix turns
> out to be wrong, so it's partly documentation. Imagine reading back
> through the history trying to find out why something changed that
> doesn't seem to make sense. Seeing a Fixes tag helps you see 'Oh, ok -
> so this change was to fix that patch, so between there and here, the
> thing isn't going to be right'.
> 
> 
> And that it's likely a good habit to keep from Kernel side?

There's probably a part of cargo-cult here, but I agree with Kieran that
the information can be useful. As it's not costly to keep using Fixes
tags, I'd rather keep them for reference.

> But no, I don't think we're backporting to a non-existent version.

:-)

> >>> 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.
> >>
> > 
> > Looking at the generated code
> > 
> > 		case _IPU3Cmd::Configure: {
> > 		controlSerializer_.reset();
> > 
> > 
> >         	const size_t configInfoStart = 0;
> > 
> > 
> >         	IPAConfigInfo configInfo = IPADataSerializer<IPAConfigInfo>::deserialize(
> > 
> > You are correct the indentation is wrong, but it does anyway matches
> > the rest of the file :)
> 
> Indeed, it matches the next two lines (A,B) - but the rest of the case
> statement is otherwise correct.
> 
> If I could figure out how to correct those two lines I would - but I
> rabbit-holed too long in this template yesterday.
> 
> I think it's still reasonable to put the controlSerializer_.reset(); at
> the correct indentation.
> 
> The args are incorrectly indented, but would be solved by fixing that
> (waves hand magically) "somewhere else" ;-) :
> 
> >                 case _IPU3Cmd::Configure: {
> > 
> > 
> >  A              const size_t configInfoStart = 0;
> > 
> > 
> >  B              IPAConfigInfo configInfo = IPADataSerializer<IPAConfigInfo>::deserialize(
> >  B                      _ipcMessage.data().cbegin() + configInfoStart,
> >  B                      _ipcMessage.data().cend(),
> >  B                      &controlSerializer_);
> > 
> > 
> >                         int32_t _callRet =ipa_->configure(configInfo);
> > 
> >                         IPCMessage::Header header = { _ipcMessage.header().cmd, _ipcMessage.header().cookie };
> >                         IPCMessage _response(header);
> >                         std::vector<uint8_t> _callRetBuf;
> >                         std::tie(_callRetBuf, std::ignore) =
> >                                 IPADataSerializer<int32_t>::serialize(_callRet);
> >                         _response.data().insert(_response.data().end(), _callRetBuf.cbegin(), _callRetBuf.cend());
> > 
> >                         int _ret = socket_.send(_response.payload());
> >                         if (_ret < 0) {
> >                                 LOG(IPAProxyIPU3Worker, Error)
> >                                         << "Reply to configure() failed: " << _ret;
> >                         }
> >                         LOG(IPAProxyIPU3Worker, Debug) << "Done replying to configure()";
> >                         break;
> >                 }
> 
> Paul, Are A and B easy fixes? They appear to be automagically generated
> to do with the IPADataSerializer for every call?
> 
> >>> +{%- 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}};

-- 
Regards,

Laurent Pinchart