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

13935 diff mbox series

Two indipendnt instancess of the ControlSerializer class are in use at
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
with isolated IPA modules without too much complexity added.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Paul Elder <paul.elder@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(-)

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@ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Paul Elder <paul.elder@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}}() {}
>  
>

Hi Jacopo,

Thank you for the patch.

On Fri, Sep 24, 2021 at 07:25:25PM +0200, Jacopo Mondi wrote:
> Two indipendnt instancess of the ControlSerializer class are in use at

"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
> with isolated IPA modules without too much complexity added.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Paul Elder <paul.elder@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.

s/.$//

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

s/avoid/avoids/

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 * 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}}() {}
>