[libcamera-devel] [PATCH v3 5/5] libcamera: control_serializer: Separate the handles space
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Sep 26 22:02:02 CEST 2021
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 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.
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 at 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}}() {}
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list