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

[Laurent Pinchart]

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.

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