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

[Kieran Bingham]

On 03/09/2021 10:16, Jacopo Mondi wrote:
> Hi Kieran,
> 
> 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)

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?


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}};
>>>