[libcamera-devel,v3,3/5] libcamera: controls: Use ControlIdMap in deserialization

13264 diff mbox series

Introduce a new field in the controls serialization protocol to
allow discerning which ControlIdMap a ControlInfoMap refers to.

The newly introduced IdMapType enumeration describes the possible
info maps:
- Either the globally available controls::controls and
  properties::properties maps, which are valid across IPC boundaries
- A ControlIdMap created locally by the V4L2 device, which is not valid
  across the IPC boundaries

At de-serialization time the idMapType filed is inspected and
- If the idmap is a globally available one, there's no need to create
  new ControlId instances when populating the de-serialized
  ControlInfoMap. Use the globally available map to retrieve the
  ControlId reference and use it
- If the idmap is a map only available locally, create a new ControlId
  as it used to happen before this patch.

As a direct consequence, this change allows us to perform lookup by
ControlId reference on de-serialized ControlIdMap that refers to the
libcamera defined controls::controls and properties::properties.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 include/libcamera/ipa/ipa_controls.h |  9 +++-
 src/libcamera/control_serializer.cpp | 65 +++++++++++++++++++++++-----
 src/libcamera/ipa_controls.cpp       | 29 +++++++++++++
 3 files changed, 90 insertions(+), 13 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Mon, Aug 09, 2021 at 05:23:06PM +0200, Jacopo Mondi wrote:
> Introduce a new field in the controls serialization protocol to
> allow discerning which ControlIdMap a ControlInfoMap refers to.
> 
> The newly introduced IdMapType enumeration describes the possible
> info maps:
> - Either the globally available controls::controls and
>   properties::properties maps, which are valid across IPC boundaries
> - A ControlIdMap created locally by the V4L2 device, which is not valid
>   across the IPC boundaries
> 
> At de-serialization time the idMapType filed is inspected and
> - If the idmap is a globally available one, there's no need to create
>   new ControlId instances when populating the de-serialized
>   ControlInfoMap. Use the globally available map to retrieve the
>   ControlId reference and use it
> - If the idmap is a map only available locally, create a new ControlId
>   as it used to happen before this patch.
> 
> As a direct consequence, this change allows us to perform lookup by
> ControlId reference on de-serialized ControlIdMap that refers to the
> libcamera defined controls::controls and properties::properties.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  include/libcamera/ipa/ipa_controls.h |  9 +++-
>  src/libcamera/control_serializer.cpp | 65 +++++++++++++++++++++++-----
>  src/libcamera/ipa_controls.cpp       | 29 +++++++++++++
>  3 files changed, 90 insertions(+), 13 deletions(-)
> 
> diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h
> index 6d3bf279c22d..f4bb7b77a974 100644
> --- a/include/libcamera/ipa/ipa_controls.h
> +++ b/include/libcamera/ipa/ipa_controls.h
> @@ -15,13 +15,20 @@ extern "C" {
>  
>  #define IPA_CONTROLS_FORMAT_VERSION	1
>  
> +enum ipa_controls_id_map_type {
> +	IPA_CONTROL_ID_MAP_CONTROLS,
> +	IPA_CONTROL_ID_MAP_PROPERTIES,
> +	IPA_CONTROL_ID_MAP_V4L2,
> +};
> +
>  struct ipa_controls_header {
>  	uint32_t version;
>  	uint32_t handle;
>  	uint32_t entries;
>  	uint32_t size;
>  	uint32_t data_offset;
> -	uint32_t reserved[3];
> +	ipa_controls_id_map_type id_map_type;

In C you need to include the enum keyword.

> +	uint32_t reserved[2];
>  };
>  
>  struct ipa_control_value_entry {
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index df6ed93f477e..9127678f25f6 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -17,6 +17,7 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>  #include <libcamera/ipa/ipa_controls.h>
> +#include <libcamera/property_ids.h>
>  
>  #include "libcamera/internal/byte_stream_buffer.h"
>  
> @@ -188,6 +189,15 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
>  	for (const auto &ctrl : infoMap)
>  		valuesSize += binarySize(ctrl.second);
>  
> +	const ControlIdMap *idmap = &infoMap.idmap();
> +	enum ipa_controls_id_map_type idMapType;
> +	if (idmap == &controls::controls)
> +		idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
> +	else if (idmap == &properties::properties)
> +		idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;
> +	else
> +		idMapType = IPA_CONTROL_ID_MAP_V4L2;
> +
>  	/* Prepare the packet header, assign a handle to the ControlInfoMap. */
>  	struct ipa_controls_header hdr;
>  	hdr.version = IPA_CONTROLS_FORMAT_VERSION;
> @@ -195,6 +205,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
>  	hdr.entries = infoMap.size();
>  	hdr.size = sizeof(hdr) + entriesSize + valuesSize;
>  	hdr.data_offset = sizeof(hdr) + entriesSize;
> +	hdr.id_map_type = idMapType;
>  
>  	buffer.write(&hdr);
>  
> @@ -368,6 +379,33 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
>  		return {};
>  	}
>  
> +	/*
> +         * Use the ControlIdMap corresponding to the id map type. If the type
> +         * references a globally defined id map (such as controls::controls
> +         * or properties::properties), use it. Otherwise, create a local id map
> +         * that will be populated with dynamically created ControlId instances
> +         * when deserializing individual ControlInfoMap entries.

Copied and pasted from e-mail, with spaces instead of tabs ? :-)

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

> +	 */
> +	const ControlIdMap *idMap = nullptr;
> +	ControlIdMap *localIdMap = nullptr;
> +	switch (hdr->id_map_type) {
> +	case IPA_CONTROL_ID_MAP_CONTROLS:
> +		idMap = &controls::controls;
> +		break;
> +	case IPA_CONTROL_ID_MAP_PROPERTIES:
> +		idMap = &properties::properties;
> +		break;
> +	case IPA_CONTROL_ID_MAP_V4L2:
> +		controlIdMaps_.emplace_back(std::make_unique<ControlIdMap>());
> +		localIdMap = controlIdMaps_.back().get();
> +		idMap = localIdMap;
> +		break;
> +	default:
> +		LOG(Serializer, Error)
> +			<< "Unknown id map type: " << hdr->id_map_type;
> +		return {};
> +	}
> +
>  	ByteStreamBuffer entries = buffer.carveOut(hdr->data_offset - sizeof(*hdr));
>  	ByteStreamBuffer values = buffer.carveOut(hdr->size - hdr->data_offset);
>  
> @@ -377,9 +415,6 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
>  	}
>  
>  	ControlInfoMap::Map ctrls;
> -	controlIdMaps_.emplace_back(std::make_unique<ControlIdMap>());
> -	ControlIdMap *localIdMap = controlIdMaps_.back().get();
> -
>  	for (unsigned int i = 0; i < hdr->entries; ++i) {
>  		const struct ipa_control_info_entry *entry =
>  			entries.read<decltype(*entry)>();
> @@ -388,15 +423,21 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
>  			return {};
>  		}
>  
> -		/* Create and cache the individual ControlId. */
>  		ControlType type = static_cast<ControlType>(entry->type);
> -		/**
> -		 * \todo Find a way to preserve the control name for debugging
> -		 * purpose.
> -		 */
> -		controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, "", type));
> -		ControlId *controlId = controlIds_.back().get();
> -		(*localIdMap)[entry->id] = controlId;
> +
> +		/* If we're using a local id map, populate it. */
> +		if (localIdMap) {
> +			/**
> +			 * \todo Find a way to preserve the control name for
> +			 * debugging purpose.
> +			 */
> +			controlIds_.emplace_back(std::make_unique<ControlId>(entry->id,
> +									     "", type));
> +			(*localIdMap)[entry->id] = controlIds_.back().get();
> +		}
> +
> +		const ControlId *controlId = idMap->at(entry->id);
> +		ASSERT(controlId);
>  
>  		if (entry->offset != values.offset()) {
>  			LOG(Serializer, Error)
> @@ -413,7 +454,7 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
>  	 * Create the ControlInfoMap in the cache, and store the map to handle
>  	 * association.
>  	 */
> -	infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *localIdMap);
> +	infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *idMap);
>  	ControlInfoMap &map = infoMaps_[hdr->handle];
>  	infoMapHandles_[&map] = hdr->handle;
>  
> diff --git a/src/libcamera/ipa_controls.cpp b/src/libcamera/ipa_controls.cpp
> index 8fd726513182..fb98cda30ede 100644
> --- a/src/libcamera/ipa_controls.cpp
> +++ b/src/libcamera/ipa_controls.cpp
> @@ -134,6 +134,33 @@
>   * \brief The current control serialization format version
>   */
>  
> +/**
> + * \var ipa_controls_id_map_type
> + * \brief Enumerates the different control id map types
> + *
> + * Each ControlInfoMap and ControlList refers to a control id map that
> + * associates the ControlId references to a numerical identifier.
> + * During the serialization procedure the raw pointers to the ControlId
> + * instances cannot be transported on the wire, hence their numerical id is
> + * used to identify them in the serialized data buffer. At deserialization time
> + * it is required to associate back to the numerical id the ControlId instance
> + * it represents. This enumeration describes which ControlIdMap should be
> + * used to perform such operation.
> + *
> + * \var ipa_controls_id_map_type::IPA_CONTROL_ID_MAP_CONTROLS
> + * \brief The numerical control identifier are resolved to a ControlId * using
> + * the global controls::controls id map
> + * \var ipa_controls_id_map_type::IPA_CONTROL_ID_MAP_PROPERTIES
> + * \brief The numerical control identifier are resolved to a ControlId * using
> + * the global properties::properties id map
> + * \var ipa_controls_id_map_type::IPA_CONTROL_ID_MAP_V4L2
> + * \brief ControlId for V4L2 defined controls are created by the video device
> + * that enumerates them, and are not available across the IPC boundaries. The
> + * deserializer shall create new ControlId instances for them as well as store
> + * them in a dedicated ControlIdMap. Only lookup by numerical id can be
> + * performed on de-serialized ControlInfoMap that represents V4L2 controls.
> + */
> +
>  /**
>   * \struct ipa_controls_header
>   * \brief Serialized control packet header
> @@ -149,6 +176,8 @@
>   * The total packet size in bytes
>   * \var ipa_controls_header::data_offset
>   * Offset in bytes from the beginning of the packet of the data section start
> + * \var ipa_controls_header::id_map_type
> + * The id map type as defined by the ipa_controls_id_map_type enumeration
>   * \var ipa_controls_header::reserved
>   * Reserved for future extensions
>   */