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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Sep 03, 2021 at 03:28:41PM +0200, Jacopo Mondi wrote:
> On Fri, Sep 03, 2021 at 02:46:35PM +0300, Laurent Pinchart wrote:
> > 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 ?

That's very much a hack. A pretty clever one actually :-) With a comment
to tell that this should likely be revisited (and a very brief
explanation of why it's done that way), I think I'm fine with it. Paul,
any comment ?

> > > +
> > >  	/*
> > >  	 * 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