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

[paul.elder at ideasonboard.com]

Hi Laurent and Jacopo,

On Mon, Sep 06, 2021 at 02:01:30AM +0300, Laurent Pinchart wrote:
> 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 ?

Ah, I see, you separate the serial number spaces, that way it'll never
conflict. I think that's a good idea.


Paul

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