[libcamera-devel] [PATCH v3 5/5] libcamera: control_serializer: Separate the handles space

[Kieran Bingham]

Hi Jacopo

On 24/09/2021 18:25, Jacopo Mondi wrote:
> Two indipendnt instancess of the ControlSerializer class are in use at

s/indipendnt instancess/independent instances/

> the IPC boundaries, one in the Proxy class that serializes data from the
> pipeline handler to the IPA, and one in the ProxyWorker which serializes
> data in the opposite direction.
> 
> Each instance operates autonomously, without any centralized point of
> control, and each one assigns a numerical handle to each ControlInfoMap
> it serializes. This creates a risk of potential collision on the handle
> values, as both instances will use the same numerical space and
> are not aware of what handles has been already used by the instance "on
> the other side".
> 
> To fix that, partition the handles numerical space by initializing the
> control serializer with a seed according to the role of the component
> that creates the serializer and increment the handle number by 2, to
> avoid any collision risk.
> 
> While this is temporary and rather hacky solution, it solves an issue

Careful ... it might not be temporary ... ;-)

> with isolated IPA modules without too much complexity added.
> 

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  .../libcamera/internal/control_serializer.h   |  8 ++-
>  src/libcamera/control_serializer.cpp          | 63 +++++++++++++++++--
>  test/serialization/control_serialization.cpp  |  4 +-
>  .../ipa_data_serializer_test.cpp              |  6 +-
>  .../module_ipa_proxy.cpp.tmpl                 |  3 +-
>  .../module_ipa_proxy_worker.cpp.tmpl          |  4 +-
>  6 files changed, 75 insertions(+), 13 deletions(-)
> 
> diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
> index 8a66be324138..caeafa11bc54 100644
> --- a/include/libcamera/internal/control_serializer.h
> +++ b/include/libcamera/internal/control_serializer.h
> @@ -20,7 +20,12 @@ class ByteStreamBuffer;
>  class ControlSerializer
>  {
>  public:
> -	ControlSerializer();
> +	enum class Role {
> +		Proxy,
> +		Worker
> +	};
> +
> +	ControlSerializer(Role role);
>  
>  	void reset();
>  
> @@ -47,6 +52,7 @@ private:
>  	ControlInfo loadControlInfo(ControlType type, ByteStreamBuffer &buffer);
>  
>  	unsigned int serial_;
> +	unsigned int serialSeed_;
>  	std::vector<std::unique_ptr<ControlId>> controlIds_;
>  	std::vector<std::unique_ptr<ControlIdMap>> controlIdMaps_;
>  	std::map<unsigned int, ControlInfoMap> infoMaps_;
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index f1245ea620ab..025222d77e4d 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -62,6 +62,14 @@ LOG_DEFINE_CATEGORY(Serializer)
>   * corresponding ControlInfoMap handle in the binary data, and when
>   * deserializing to retrieve the corresponding ControlInfoMap.
>   *
> + * As independent ControlSerializer instances are used on both sides of the IPC
> + * boundary, and the two instances operate without a shared point of control,
> + * there is a potential risk of collision of the numerical handles assigned to
> + * each serialized ControlInfoMap. For this reason the control serializer is
> + * initialized with a seed and the handle is incremented by 2, so that instances
> + * initialized with a different seed operate on a separate numerical space,
> + * avoiding any collision risk.
> + *
>   * In order to perform those tasks, the serializer keeps an internal state that
>   * needs to be properly populated. This mechanism requires the ControlInfoMap
>   * corresponding to a ControlList to have been serialized or deserialized
> @@ -77,9 +85,45 @@ LOG_DEFINE_CATEGORY(Serializer)
>   * proceed with care to avoid stale references.
>   */
>  
> -ControlSerializer::ControlSerializer()
> -	: serial_(0)
> +/**
> + * \enum ControlSerializer::Role
> + * \brief Define the role of the IPC component using the control serializer.
> + *
> + * The role of the component that creates the serializer is used to initialize
> + * the handles numerical space.
> + *
> + * \var ControlSerializer::Role::Proxy
> + * \brief The control serializer is used by the IPC Proxy classes
> + *
> + * \var ControlSerializer::Role::Worker
> + * \brief The control serializer is used by the IPC ProxyWorker classes
> + */
> +
> +/**
> + * \brief Construct a new ControlSerializer
> + * \param[in] role The role of the IPC component using the serializer
> + */
> +ControlSerializer::ControlSerializer(Role role)
>  {
> +	/*
> +	 * Initialize the handle numerical space using the role of the
> +	 * component that created the instance.
> +	 *
> +	 * Instances initialized for a different role will use a different
> +	 * numerical handle space, avoiding any collision risk when, in example,
> +	 * two instances of the ControlSerializer class are used at the IPC
> +	 * boundaries.
> +	 *
> +	 * Start counting handles from '1' as '0' is a special value used as
> +	 * place holder when serializing lists that do not have a ControlInfoMap
> +	 * associated (in example list of libcamera controls::controls).
> +	 *
> +	 * \todo This is a temporary hack and should probably be better
> +	 * engineered, but for the time being it avoid collisions on the handle
> +	 * value when using IPC.
> +	 */
> +	serialSeed_ = role == Role::Proxy ? 1 : 2;
> +	serial_ = serialSeed_;
>  }
>  
>  /**
> @@ -90,7 +134,7 @@ ControlSerializer::ControlSerializer()
>   */
>  void ControlSerializer::reset()
>  {
> -	serial_ = 0;
> +	serial_ = serialSeed_;
>  
>  	infoMapHandles_.clear();
>  	infoMaps_.clear();
> @@ -200,10 +244,10 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
>  	else
>  		idMapType = IPA_CONTROL_ID_MAP_V4L2;
>  
> -	/* Prepare the packet header, assign a handle to the ControlInfoMap. */
> +	/* Prepare the packet header. */
>  	struct ipa_controls_header hdr;
>  	hdr.version = IPA_CONTROLS_FORMAT_VERSION;
> -	hdr.handle = ++serial_;
> +	hdr.handle = serial_;
>  	hdr.entries = infoMap.size();
>  	hdr.size = sizeof(hdr) + entriesSize + valuesSize;
>  	hdr.data_offset = sizeof(hdr) + entriesSize;
> @@ -211,6 +255,15 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
>  
>  	buffer.write(&hdr);
>  
> +	/*
> +	 * Increment the handle for the ControlInfoMap by 2 to keep the handles
> +	 * numerical space partitioned between instances initialized for a
> +	 * different role.
> +	 *
> +	 * \sa ControlSerializer::Role
> +	 */
> +	serial_ += 2;
> +
>  	/*
>  	 * Serialize all entries.
>  	 * \todo Serialize the control name too
> diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp
> index 5ac9c4ede5f9..a507d98a152d 100644
> --- a/test/serialization/control_serialization.cpp
> +++ b/test/serialization/control_serialization.cpp
> @@ -30,8 +30,8 @@ protected:
>  
>  	int run() override
>  	{
> -		ControlSerializer serializer;
> -		ControlSerializer deserializer;
> +		ControlSerializer serializer(ControlSerializer::Role::Proxy);
> +		ControlSerializer deserializer(ControlSerializer::Role::Worker);
>  
>  		std::vector<uint8_t> infoData;
>  		std::vector<uint8_t> listData;
> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp
> index eca19a6642e1..5fcdcb8eae92 100644
> --- a/test/serialization/ipa_data_serializer_test.cpp
> +++ b/test/serialization/ipa_data_serializer_test.cpp
> @@ -162,7 +162,7 @@ private:
>  
>  	int testControls()
>  	{
> -		ControlSerializer cs;
> +		ControlSerializer cs(ControlSerializer::Role::Proxy);
>  
>  		const ControlInfoMap &infoMap = camera_->controls();
>  		ControlList list = generateControlList(infoMap);
> @@ -195,7 +195,7 @@ private:
>  
>  	int testVector()
>  	{
> -		ControlSerializer cs;
> +		ControlSerializer cs(ControlSerializer::Role::Proxy);
>  
>  		/*
>  		 * We don't test FileDescriptor serdes because it dup()s, so we
> @@ -265,7 +265,7 @@ private:
>  
>  	int testMap()
>  	{
> -		ControlSerializer cs;
> +		ControlSerializer cs(ControlSerializer::Role::Proxy);
>  
>  		/*
>  		 * Realistically, only string and integral keys.
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> index aab1fffbcaf0..d856339aa9ee 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -46,7 +46,8 @@ namespace {{ns}} {
>  {%- endif %}
>  
>  {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate)
> -	: IPAProxy(ipam), isolate_(isolate), seq_(0)
> +	: IPAProxy(ipam), isolate_(isolate),
> +	  controlSerializer_(ControlSerializer::Role::Proxy), seq_(0)
>  {
>  	LOG(IPAProxy, Debug)
>  		<< "initializing {{module_name}} proxy: loading IPA from "
> 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 890246a8849b..764e7a3af63a 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
> @@ -53,7 +53,9 @@ class {{proxy_worker_name}}
>  {
>  public:
>  	{{proxy_worker_name}}()
> -		: ipa_(nullptr), exit_(false) {}
> +		: ipa_(nullptr),
> +		  controlSerializer_(ControlSerializer::Role::Worker),
> +		  exit_(false) {}
>  
>  	~{{proxy_worker_name}}() {}
>  
>