[libcamera-devel] [PATCH 3/6] libcamera: control_serializer: Keep handles in sync

[Jacopo Mondi]

Hi Laurent,

On Fri, Sep 03, 2021 at 02:46:35PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Sep 01, 2021 at 04:37:57PM +0200, Jacopo Mondi wrote:
> > When running the IPA in isolated mode, each side of the IPC boundary
> > has an instance of the ControlSerializer class.
> >
> > Each serializer instance tags with a numerical id (handle) each
> > ControlInfoMap instance it serializes, to be capable of associating
> > ControlList with it at serialization and deserialization time, and
> > increments the numerical counter for each newly serialized control info
> > map.
> >
> > Having two instances of the ControlSerializer class running in two
> > different process spaces, each instance increments its own counter,
> > preventing from maintaining a globally shared state where each
> > ControlInfoMap instance is reference by a unique identifier.
> >
> > Fix this by advancing the serial_ counter at ControlInfoMap
> > de-serialization time, so that each newly serialized map will have a
> > globally unique identifier.
>
> That's an interesting one. The control serializer was developed with the
> assumption that a ControlInfoMap would only be sent from the pipeline
> handler to the IPA, not the other way around.
>
> > Fixes: 2c5f0ad23aa4 ("libcamera: Add controls serializer")
>
> Technically, the breakage was introduced by the first change in the IPA
> protocol that sent a ControlInfoMap in the IPA to pipeline handler
> direction, but I don't mind keeping the Fixes tag as-is.
>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/control_serializer.cpp | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > index 27360526f9eb..08cfecca3078 100644
> > --- a/src/libcamera/control_serializer.cpp
> > +++ b/src/libcamera/control_serializer.cpp
> > @@ -380,6 +380,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> >  		return {};
> >  	}
> >
> > +	/* Keep the info map handles in sync between the two sides of IPC. */
> > +	serial_ = std::max(serial_, hdr->handle);
>
> Can it be this simple ? What if the pipeline handler sends a
> ControlInfoMap to the IPA at the same time as the IPA sends a
> ControlInfoMap to the pipeline handler ? Each side will increment their
> serial number, but the same number will refer to two different maps.
>

Yes, this might happen during a capture session, where the PH/IPA
interactions are aysnchronous. It can't happen before, as there won't
be concurrect calls between the two sides of the IPC

A regular run looks like

-----------------------------------------------------------------------------
PH        handle#     IPC    IPA        handl#
-----------------------------------------------------------------------------
ser(map)   1           --->
                             deser(map)
                                        max(0,1)=1

                       <---  ser(map)  2
           max(1,2)=2
deser(map)

ser(map)  3
                        ...
-----------------------------------------------------------------------------

This could break only if a ser/deser sequence is interleaved with
another serialization event

-----------------------------------------------------------------------------
PH        handle#     IPC    IPA        handl#
-----------------------------------------------------------------------------
ser(map)   1           ---->

                       <---- ser(map)  1

                             deser(map)
                                        max(1,1)=1
           max(1,1)=1
deser(map)

ser(map)  2
                        ...
-----------------------------------------------------------------------------

Which is very unfortunate but possible.

We have no centralized state between the two processes and we have no
way to assign handles in the proxies, as they have the same
information as the control serializer has.

This might seem rather stupid, but we have two sides of the IPC, can't
we use even handles on one side and odd ones on the other ? The
proxies initialize the controls serializer with a seed (0 or 1) and we
simply serial_ += 2 ?


> > +
> >  	/*
> >  	 * Use the ControlIdMap corresponding to the id map type. If the type
> >  	 * references a globally defined id map (such as controls::controls
>
> --
> Regards,
>
> Laurent Pinchart