| Message ID | 20241129091916.298359-4-paul.elder@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Paul Elder (2024-11-29 09:19:15) > Add support to ControlId for querying direction information. This allows > applications to query whether a ControlId is meant for being set in > controls or to be returned in metadata or both. This also has a side > effect of properly encoding this information, as previously it was only > mentioned losely and inconsistently in the control id definition. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v3: > - make direction parameter to ControlId and Control consutrctors > mandatory > - this entails expanding the control serializer/deserializer for > V4L2Device and ControlSerializer to properly handle the direction > - also the constructor parameters have to be reordered, compared to > the last vesrion at least > > Changes in v2: > - simplify code > --- > include/libcamera/controls.h | 19 ++++++++++-- > include/libcamera/ipa/ipa_controls.h | 3 +- > src/libcamera/control_ids.cpp.in | 4 +-- > src/libcamera/control_serializer.cpp | 11 ++++++- > src/libcamera/controls.cpp | 45 ++++++++++++++++++++++++++-- > src/libcamera/v4l2_device.cpp | 10 ++++++- > 6 files changed, 82 insertions(+), 10 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 3cfe2de5973c..13bb914badbf 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -17,6 +17,7 @@ > #include <vector> > > #include <libcamera/base/class.h> > +#include <libcamera/base/flags.h> > #include <libcamera/base/span.h> > > #include <libcamera/geometry.h> > @@ -235,14 +236,24 @@ private: > class ControlId > { > public: > + enum class Direction { > + In = (1 << 0), > + Out = (1 << 1), > + }; I'm curious that I think the previous patch is using the flags that are only just defined here ... > + > + using DirectionFlags = Flags<Direction>; > + > ControlId(unsigned int id, const std::string &name, const std::string &vendor, > - ControlType type, std::size_t size = 0, > + ControlType type, DirectionFlags direction, > + std::size_t size = 0, > const std::map<std::string, int32_t> &enumStrMap = {}); > > unsigned int id() const { return id_; } > const std::string &name() const { return name_; } > const std::string &vendor() const { return vendor_; } > ControlType type() const { return type_; } > + bool isInput() const { return !!(direction_ & Direction::In); } > + bool isOutput() const { return !!(direction_ & Direction::Out); } > bool isArray() const { return size_ > 0; } > std::size_t size() const { return size_; } > const std::map<int32_t, std::string> &enumerators() const { return reverseMap_; } > @@ -254,11 +265,14 @@ private: > std::string name_; > std::string vendor_; > ControlType type_; > + DirectionFlags direction_; > std::size_t size_; > std::map<std::string, int32_t> enumStrMap_; > std::map<int32_t, std::string> reverseMap_; > }; > > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction) > + > static inline bool operator==(unsigned int lhs, const ControlId &rhs) > { > return lhs == rhs.id(); > @@ -286,9 +300,10 @@ public: > using type = T; > > Control(unsigned int id, const char *name, const char *vendor, > + ControlId::DirectionFlags direction, > const std::map<std::string, int32_t> &enumStrMap = {}) > : ControlId(id, name, vendor, details::control_type<std::remove_cv_t<T>>::value, > - details::control_type<std::remove_cv_t<T>>::size, enumStrMap) > + direction, details::control_type<std::remove_cv_t<T>>::size, enumStrMap) > { > } > > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h > index 5fd13394fcef..980668c86bcc 100644 > --- a/include/libcamera/ipa/ipa_controls.h > +++ b/include/libcamera/ipa/ipa_controls.h > @@ -46,7 +46,8 @@ struct ipa_control_info_entry { > uint32_t id; > uint32_t type; > uint32_t offset; > - uint32_t padding[1]; > + uint8_t direction; > + uint8_t padding[3]; What's the padding for here? Alignment ? or something else? Should we 'add more' while we're in an ABI break ? or is it needed at all? > }; > > #ifdef __cplusplus > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in > index afe9e2c96610..65668d486dbc 100644 > --- a/src/libcamera/control_ids.cpp.in > +++ b/src/libcamera/control_ids.cpp.in > @@ -89,9 +89,9 @@ extern const std::map<std::string, {{ctrl.type}}> {{ctrl.name}}NameValueMap = { > { "{{enum.name}}", {{enum.name}} }, > {%- endfor %} > }; > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.name}}NameValueMap); > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.direction}}, {{ctrl.name}}NameValueMap); > {% else -%} > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}"); > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.direction}}); > {% endif -%} > {%- endfor %} > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 0a5e8220a0ff..0804c178f0d4 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -281,6 +281,9 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, > entry.id = id->id(); > entry.type = id->type(); > entry.offset = values.offset(); > + entry.direction = > + (id->isInput() ? static_cast<uint8_t>(ControlId::Direction::In) : 0) | > + (id->isOutput() ? static_cast<uint8_t>(ControlId::Direction::Out) : 0); Is this necessary ? Can't the raw value just be serialized? (oh or perhaps the raw value can't be accessed in which case it probably is needed ... unless we add an id->direction(); which is probably better than re-encoding the same thing ...) > entries.write(&entry); > > store(info, values); > @@ -493,12 +496,18 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > > /* If we're using a local id map, populate it. */ > if (localIdMap) { > + ControlId::DirectionFlags flags; > + if (entry->direction & static_cast<uint8_t>(ControlId::Direction::In)) > + flags |= ControlId::Direction::In; > + if (entry->direction & static_cast<uint8_t>(ControlId::Direction::Out)) > + flags |= ControlId::Direction::Out; and here - can't we just do a direct usage? or is there an issue with serialising 'Flags' ? > /** > * \todo Find a way to preserve the control name for > * debugging purpose. > */ > controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, > - "", "local", type)); > + "", "local", type, > + flags)); > (*localIdMap)[entry->id] = controlIds_.back().get(); > } > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 2efecf0fc52b..23b71fac8e84 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -396,15 +396,16 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen > * \param[in] name The control name > * \param[in] vendor The vendor name > * \param[in] type The control data type > + * \param[in] direction The direction of the control, if it can be used in Controls or Metadata > * \param[in] size The size of the array control, or 0 if scalar control > * \param[in] enumStrMap The map from enum names to values (optional) > */ > ControlId::ControlId(unsigned int id, const std::string &name, > const std::string &vendor, ControlType type, > - std::size_t size, > + DirectionFlags direction, std::size_t size, > const std::map<std::string, int32_t> &enumStrMap) > - : id_(id), name_(name), vendor_(vendor), type_(type), size_(size), > - enumStrMap_(enumStrMap) > + : id_(id), name_(name), vendor_(vendor), type_(type), > + direction_(direction), size_(size), enumStrMap_(enumStrMap) > { > for (const auto &pair : enumStrMap_) > reverseMap_[pair.second] = pair.first; > @@ -434,6 +435,26 @@ ControlId::ControlId(unsigned int id, const std::string &name, > * \return The control data type > */ > > +/** > + * \fn bool ControlId::isInput() const > + * \brief Determine if the control is available to be used as an input control > + * > + * Controls can be used either as input in controls, or as output in metadata. > + * This function checks if the control is allowed to be used as the former. > + * > + * \return True if the control can be used as an input control, false otherwise > + */ > + > +/** > + * \fn bool ControlId::isOutput() const > + * \brief Determine if the control is available to be used in output metadata > + * > + * Controls can be used either as input in controls, or as output in metadata. > + * This function checks if the control is allowed to be used as the latter. > + * > + * \return True if the control can be returned in output metadata, false otherwise > + */ > + > /** > * \fn bool ControlId::isArray() const > * \brief Determine if the control is an array control > @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name, > * \return True if \a lhs.id() is equal to \a rhs, false otherwise > */ > > +/** > + * \enum ControlId::Direction > + * \brief The direction the control is capable of being passed from/to > + * > + * \var ControlId::Direction::In > + * \brief The control can be passed as input in controls > + * > + * \var ControlId::Direction::Out > + * \brief The control can be returned as output in metadata > + */ > + > +/** > + * \typedef ControlId::DirectionFlags > + * \brief A wrapper for ControlId::Direction so that it can be used as flags > + */ > + > /** > * \class Control > * \brief Describe a control and its intrinsic properties > @@ -504,6 +541,8 @@ ControlId::ControlId(unsigned int id, const std::string &name, > * \param[in] id The control numerical ID > * \param[in] name The control name > * \param[in] vendor The vendor name > + * \param[in] direction The direction of the control, if it can be used in > + * Controls or Metadata > * \param[in] enumStrMap The map from enum names to values (optional) > * > * The control data type is automatically deduced from the template type T. > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 7d21cf15fec1..4634ccc392b6 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -520,7 +520,15 @@ std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl & > const std::string name(static_cast<const char *>(ctrl.name), len); > const ControlType type = v4l2CtrlType(ctrl.type); > > - return std::make_unique<ControlId>(ctrl.id, name, "v4l2", type); > + ControlId::DirectionFlags flags; I expect it's fine, but the above is uninitialised, > + if (ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY) > + flags |= ControlId::Direction::Out; So you shouldn't be 'or'ing in here, just assign directly. Same for the two below. > + else if (ctrl.flags & V4L2_CTRL_FLAG_WRITE_ONLY) > + flags |= ControlId::Direction::In; > + else > + flags |= ControlId::Direction::In | ControlId::Direction::Out; > + > + return std::make_unique<ControlId>(ctrl.id, name, "v4l2", type, flags); > } > > /** > -- > 2.39.2 >
Hi Paul, Thank you for the patch. On Fri, Nov 29, 2024 at 06:19:15PM +0900, Paul Elder wrote: > Add support to ControlId for querying direction information. This allows > applications to query whether a ControlId is meant for being set in > controls or to be returned in metadata or both. This also has a side > effect of properly encoding this information, as previously it was only > mentioned losely and inconsistently in the control id definition. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v3: > - make direction parameter to ControlId and Control consutrctors > mandatory > - this entails expanding the control serializer/deserializer for > V4L2Device and ControlSerializer to properly handle the direction > - also the constructor parameters have to be reordered, compared to > the last vesrion at least > > Changes in v2: > - simplify code > --- > include/libcamera/controls.h | 19 ++++++++++-- > include/libcamera/ipa/ipa_controls.h | 3 +- > src/libcamera/control_ids.cpp.in | 4 +-- > src/libcamera/control_serializer.cpp | 11 ++++++- > src/libcamera/controls.cpp | 45 ++++++++++++++++++++++++++-- > src/libcamera/v4l2_device.cpp | 10 ++++++- > 6 files changed, 82 insertions(+), 10 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 3cfe2de5973c..13bb914badbf 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -17,6 +17,7 @@ > #include <vector> > > #include <libcamera/base/class.h> > +#include <libcamera/base/flags.h> > #include <libcamera/base/span.h> > > #include <libcamera/geometry.h> > @@ -235,14 +236,24 @@ private: > class ControlId > { > public: > + enum class Direction { > + In = (1 << 0), > + Out = (1 << 1), > + }; > + > + using DirectionFlags = Flags<Direction>; > + > ControlId(unsigned int id, const std::string &name, const std::string &vendor, > - ControlType type, std::size_t size = 0, > + ControlType type, DirectionFlags direction, > + std::size_t size = 0, > const std::map<std::string, int32_t> &enumStrMap = {}); > > unsigned int id() const { return id_; } > const std::string &name() const { return name_; } > const std::string &vendor() const { return vendor_; } > ControlType type() const { return type_; } > + bool isInput() const { return !!(direction_ & Direction::In); } > + bool isOutput() const { return !!(direction_ & Direction::Out); } > bool isArray() const { return size_ > 0; } > std::size_t size() const { return size_; } > const std::map<int32_t, std::string> &enumerators() const { return reverseMap_; } > @@ -254,11 +265,14 @@ private: > std::string name_; > std::string vendor_; > ControlType type_; > + DirectionFlags direction_; > std::size_t size_; > std::map<std::string, int32_t> enumStrMap_; > std::map<int32_t, std::string> reverseMap_; > }; > > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction) > + > static inline bool operator==(unsigned int lhs, const ControlId &rhs) > { > return lhs == rhs.id(); > @@ -286,9 +300,10 @@ public: > using type = T; > > Control(unsigned int id, const char *name, const char *vendor, > + ControlId::DirectionFlags direction, > const std::map<std::string, int32_t> &enumStrMap = {}) > : ControlId(id, name, vendor, details::control_type<std::remove_cv_t<T>>::value, > - details::control_type<std::remove_cv_t<T>>::size, enumStrMap) > + direction, details::control_type<std::remove_cv_t<T>>::size, enumStrMap) > { > } > > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h > index 5fd13394fcef..980668c86bcc 100644 > --- a/include/libcamera/ipa/ipa_controls.h > +++ b/include/libcamera/ipa/ipa_controls.h > @@ -46,7 +46,8 @@ struct ipa_control_info_entry { > uint32_t id; > uint32_t type; > uint32_t offset; > - uint32_t padding[1]; > + uint8_t direction; > + uint8_t padding[3]; > }; > > #ifdef __cplusplus > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in > index afe9e2c96610..65668d486dbc 100644 > --- a/src/libcamera/control_ids.cpp.in > +++ b/src/libcamera/control_ids.cpp.in > @@ -89,9 +89,9 @@ extern const std::map<std::string, {{ctrl.type}}> {{ctrl.name}}NameValueMap = { > { "{{enum.name}}", {{enum.name}} }, > {%- endfor %} > }; > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.name}}NameValueMap); > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.direction}}, {{ctrl.name}}NameValueMap); > {% else -%} > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}"); > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.direction}}); > {% endif -%} > {%- endfor %} > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index 0a5e8220a0ff..0804c178f0d4 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -281,6 +281,9 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, > entry.id = id->id(); > entry.type = id->type(); > entry.offset = values.offset(); > + entry.direction = > + (id->isInput() ? static_cast<uint8_t>(ControlId::Direction::In) : 0) | > + (id->isOutput() ? static_cast<uint8_t>(ControlId::Direction::Out) : 0); > entries.write(&entry); > > store(info, values); > @@ -493,12 +496,18 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > > /* If we're using a local id map, populate it. */ > if (localIdMap) { > + ControlId::DirectionFlags flags; > + if (entry->direction & static_cast<uint8_t>(ControlId::Direction::In)) > + flags |= ControlId::Direction::In; > + if (entry->direction & static_cast<uint8_t>(ControlId::Direction::Out)) > + flags |= ControlId::Direction::Out; > /** > * \todo Find a way to preserve the control name for > * debugging purpose. > */ > controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, > - "", "local", type)); > + "", "local", type, > + flags)); > (*localIdMap)[entry->id] = controlIds_.back().get(); > } > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 2efecf0fc52b..23b71fac8e84 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -396,15 +396,16 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen > * \param[in] name The control name > * \param[in] vendor The vendor name > * \param[in] type The control data type > + * \param[in] direction The direction of the control, if it can be used in Controls or Metadata > * \param[in] size The size of the array control, or 0 if scalar control > * \param[in] enumStrMap The map from enum names to values (optional) > */ > ControlId::ControlId(unsigned int id, const std::string &name, > const std::string &vendor, ControlType type, > - std::size_t size, > + DirectionFlags direction, std::size_t size, > const std::map<std::string, int32_t> &enumStrMap) > - : id_(id), name_(name), vendor_(vendor), type_(type), size_(size), > - enumStrMap_(enumStrMap) > + : id_(id), name_(name), vendor_(vendor), type_(type), > + direction_(direction), size_(size), enumStrMap_(enumStrMap) > { > for (const auto &pair : enumStrMap_) > reverseMap_[pair.second] = pair.first; > @@ -434,6 +435,26 @@ ControlId::ControlId(unsigned int id, const std::string &name, > * \return The control data type > */ > > +/** > + * \fn bool ControlId::isInput() const > + * \brief Determine if the control is available to be used as an input control > + * > + * Controls can be used either as input in controls, or as output in metadata. > + * This function checks if the control is allowed to be used as the former. > + * > + * \return True if the control can be used as an input control, false otherwise > + */ > + > +/** > + * \fn bool ControlId::isOutput() const > + * \brief Determine if the control is available to be used in output metadata > + * > + * Controls can be used either as input in controls, or as output in metadata. > + * This function checks if the control is allowed to be used as the latter. > + * > + * \return True if the control can be returned in output metadata, false otherwise > + */ > + > /** > * \fn bool ControlId::isArray() const > * \brief Determine if the control is an array control > @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name, > * \return True if \a lhs.id() is equal to \a rhs, false otherwise > */ > > +/** > + * \enum ControlId::Direction > + * \brief The direction the control is capable of being passed from/to > + * > + * \var ControlId::Direction::In > + * \brief The control can be passed as input in controls > + * > + * \var ControlId::Direction::Out > + * \brief The control can be returned as output in metadata > + */ > + > +/** > + * \typedef ControlId::DirectionFlags > + * \brief A wrapper for ControlId::Direction so that it can be used as flags > + */ > + > /** > * \class Control > * \brief Describe a control and its intrinsic properties > @@ -504,6 +541,8 @@ ControlId::ControlId(unsigned int id, const std::string &name, > * \param[in] id The control numerical ID > * \param[in] name The control name > * \param[in] vendor The vendor name > + * \param[in] direction The direction of the control, if it can be used in > + * Controls or Metadata > * \param[in] enumStrMap The map from enum names to values (optional) > * > * The control data type is automatically deduced from the template type T. > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 7d21cf15fec1..4634ccc392b6 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -520,7 +520,15 @@ std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl & > const std::string name(static_cast<const char *>(ctrl.name), len); > const ControlType type = v4l2CtrlType(ctrl.type); > > - return std::make_unique<ControlId>(ctrl.id, name, "v4l2", type); > + ControlId::DirectionFlags flags; > + if (ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY) > + flags |= ControlId::Direction::Out; > + else if (ctrl.flags & V4L2_CTRL_FLAG_WRITE_ONLY) > + flags |= ControlId::Direction::In; > + else > + flags |= ControlId::Direction::In | ControlId::Direction::Out; You could replace |= with =. Not sure it makes a difference. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + return std::make_unique<ControlId>(ctrl.id, name, "v4l2", type, flags); > } > > /**
On Fri, Dec 06, 2024 at 12:59:09PM +0000, Kieran Bingham wrote: > Quoting Paul Elder (2024-11-29 09:19:15) > > Add support to ControlId for querying direction information. This allows > > applications to query whether a ControlId is meant for being set in > > controls or to be returned in metadata or both. This also has a side > > effect of properly encoding this information, as previously it was only > > mentioned losely and inconsistently in the control id definition. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > Changes in v3: > > - make direction parameter to ControlId and Control consutrctors > > mandatory > > - this entails expanding the control serializer/deserializer for > > V4L2Device and ControlSerializer to properly handle the direction > > - also the constructor parameters have to be reordered, compared to > > the last vesrion at least > > > > Changes in v2: > > - simplify code > > --- > > include/libcamera/controls.h | 19 ++++++++++-- > > include/libcamera/ipa/ipa_controls.h | 3 +- > > src/libcamera/control_ids.cpp.in | 4 +-- > > src/libcamera/control_serializer.cpp | 11 ++++++- > > src/libcamera/controls.cpp | 45 ++++++++++++++++++++++++++-- > > src/libcamera/v4l2_device.cpp | 10 ++++++- > > 6 files changed, 82 insertions(+), 10 deletions(-) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index 3cfe2de5973c..13bb914badbf 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -17,6 +17,7 @@ > > #include <vector> > > > > #include <libcamera/base/class.h> > > +#include <libcamera/base/flags.h> > > #include <libcamera/base/span.h> > > > > #include <libcamera/geometry.h> > > @@ -235,14 +236,24 @@ private: > > class ControlId > > { > > public: > > + enum class Direction { > > + In = (1 << 0), > > + Out = (1 << 1), > > + }; > > I'm curious that I think the previous patch is using the flags that are > only just defined here ... > > > + > > + using DirectionFlags = Flags<Direction>; > > + > > ControlId(unsigned int id, const std::string &name, const std::string &vendor, > > - ControlType type, std::size_t size = 0, > > + ControlType type, DirectionFlags direction, > > + std::size_t size = 0, > > const std::map<std::string, int32_t> &enumStrMap = {}); > > > > unsigned int id() const { return id_; } > > const std::string &name() const { return name_; } > > const std::string &vendor() const { return vendor_; } > > ControlType type() const { return type_; } > > + bool isInput() const { return !!(direction_ & Direction::In); } > > + bool isOutput() const { return !!(direction_ & Direction::Out); } > > bool isArray() const { return size_ > 0; } > > std::size_t size() const { return size_; } > > const std::map<int32_t, std::string> &enumerators() const { return reverseMap_; } > > @@ -254,11 +265,14 @@ private: > > std::string name_; > > std::string vendor_; > > ControlType type_; > > + DirectionFlags direction_; > > std::size_t size_; > > std::map<std::string, int32_t> enumStrMap_; > > std::map<int32_t, std::string> reverseMap_; > > }; > > > > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction) > > + > > static inline bool operator==(unsigned int lhs, const ControlId &rhs) > > { > > return lhs == rhs.id(); > > @@ -286,9 +300,10 @@ public: > > using type = T; > > > > Control(unsigned int id, const char *name, const char *vendor, > > + ControlId::DirectionFlags direction, > > const std::map<std::string, int32_t> &enumStrMap = {}) > > : ControlId(id, name, vendor, details::control_type<std::remove_cv_t<T>>::value, > > - details::control_type<std::remove_cv_t<T>>::size, enumStrMap) > > + direction, details::control_type<std::remove_cv_t<T>>::size, enumStrMap) > > { > > } > > > > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h > > index 5fd13394fcef..980668c86bcc 100644 > > --- a/include/libcamera/ipa/ipa_controls.h > > +++ b/include/libcamera/ipa/ipa_controls.h > > @@ -46,7 +46,8 @@ struct ipa_control_info_entry { > > uint32_t id; > > uint32_t type; > > uint32_t offset; > > - uint32_t padding[1]; > > + uint8_t direction; > > + uint8_t padding[3]; > > What's the padding for here? Alignment ? or something else? > Should we 'add more' while we're in an ABI break ? or is it needed at > all? > > > }; > > > > #ifdef __cplusplus > > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in > > index afe9e2c96610..65668d486dbc 100644 > > --- a/src/libcamera/control_ids.cpp.in > > +++ b/src/libcamera/control_ids.cpp.in > > @@ -89,9 +89,9 @@ extern const std::map<std::string, {{ctrl.type}}> {{ctrl.name}}NameValueMap = { > > { "{{enum.name}}", {{enum.name}} }, > > {%- endfor %} > > }; > > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.name}}NameValueMap); > > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.direction}}, {{ctrl.name}}NameValueMap); > > {% else -%} > > -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}"); > > +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.direction}}); > > {% endif -%} > > {%- endfor %} > > > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > > index 0a5e8220a0ff..0804c178f0d4 100644 > > --- a/src/libcamera/control_serializer.cpp > > +++ b/src/libcamera/control_serializer.cpp > > @@ -281,6 +281,9 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, > > entry.id = id->id(); > > entry.type = id->type(); > > entry.offset = values.offset(); > > + entry.direction = > > + (id->isInput() ? static_cast<uint8_t>(ControlId::Direction::In) : 0) | > > + (id->isOutput() ? static_cast<uint8_t>(ControlId::Direction::Out) : 0); > > Is this necessary ? Can't the raw value just be serialized? (oh or > perhaps the raw value can't be accessed in which case it probably is > needed ... unless we add an id->direction(); which is probably better > than re-encoding the same thing ...) A direction() function could indeed be useful. > > entries.write(&entry); > > > > store(info, values); > > @@ -493,12 +496,18 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > > > > /* If we're using a local id map, populate it. */ > > if (localIdMap) { > > + ControlId::DirectionFlags flags; > > + if (entry->direction & static_cast<uint8_t>(ControlId::Direction::In)) > > + flags |= ControlId::Direction::In; > > + if (entry->direction & static_cast<uint8_t>(ControlId::Direction::Out)) > > + flags |= ControlId::Direction::Out; > > > and here - can't we just do a direct usage? or is there an issue with > serialising 'Flags' ? > > > /** > > * \todo Find a way to preserve the control name for > > * debugging purpose. > > */ > > controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, > > - "", "local", type)); > > + "", "local", type, > > + flags)); > > (*localIdMap)[entry->id] = controlIds_.back().get(); > > } > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index 2efecf0fc52b..23b71fac8e84 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -396,15 +396,16 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen > > * \param[in] name The control name > > * \param[in] vendor The vendor name > > * \param[in] type The control data type > > + * \param[in] direction The direction of the control, if it can be used in Controls or Metadata > > * \param[in] size The size of the array control, or 0 if scalar control > > * \param[in] enumStrMap The map from enum names to values (optional) > > */ > > ControlId::ControlId(unsigned int id, const std::string &name, > > const std::string &vendor, ControlType type, > > - std::size_t size, > > + DirectionFlags direction, std::size_t size, > > const std::map<std::string, int32_t> &enumStrMap) > > - : id_(id), name_(name), vendor_(vendor), type_(type), size_(size), > > - enumStrMap_(enumStrMap) > > + : id_(id), name_(name), vendor_(vendor), type_(type), > > + direction_(direction), size_(size), enumStrMap_(enumStrMap) > > { > > for (const auto &pair : enumStrMap_) > > reverseMap_[pair.second] = pair.first; > > @@ -434,6 +435,26 @@ ControlId::ControlId(unsigned int id, const std::string &name, > > * \return The control data type > > */ > > > > +/** > > + * \fn bool ControlId::isInput() const > > + * \brief Determine if the control is available to be used as an input control > > + * > > + * Controls can be used either as input in controls, or as output in metadata. > > + * This function checks if the control is allowed to be used as the former. > > + * > > + * \return True if the control can be used as an input control, false otherwise > > + */ > > + > > +/** > > + * \fn bool ControlId::isOutput() const > > + * \brief Determine if the control is available to be used in output metadata > > + * > > + * Controls can be used either as input in controls, or as output in metadata. > > + * This function checks if the control is allowed to be used as the latter. > > + * > > + * \return True if the control can be returned in output metadata, false otherwise > > + */ > > + > > /** > > * \fn bool ControlId::isArray() const > > * \brief Determine if the control is an array control > > @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name, > > * \return True if \a lhs.id() is equal to \a rhs, false otherwise > > */ > > > > +/** > > + * \enum ControlId::Direction > > + * \brief The direction the control is capable of being passed from/to > > + * > > + * \var ControlId::Direction::In > > + * \brief The control can be passed as input in controls > > + * > > + * \var ControlId::Direction::Out > > + * \brief The control can be returned as output in metadata > > + */ > > + > > +/** > > + * \typedef ControlId::DirectionFlags > > + * \brief A wrapper for ControlId::Direction so that it can be used as flags > > + */ > > + > > /** > > * \class Control > > * \brief Describe a control and its intrinsic properties > > @@ -504,6 +541,8 @@ ControlId::ControlId(unsigned int id, const std::string &name, > > * \param[in] id The control numerical ID > > * \param[in] name The control name > > * \param[in] vendor The vendor name > > + * \param[in] direction The direction of the control, if it can be used in > > + * Controls or Metadata > > * \param[in] enumStrMap The map from enum names to values (optional) > > * > > * The control data type is automatically deduced from the template type T. > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 7d21cf15fec1..4634ccc392b6 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -520,7 +520,15 @@ std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl & > > const std::string name(static_cast<const char *>(ctrl.name), len); > > const ControlType type = v4l2CtrlType(ctrl.type); > > > > - return std::make_unique<ControlId>(ctrl.id, name, "v4l2", type); > > + ControlId::DirectionFlags flags; > > I expect it's fine, but the above is uninitialised, The Flags class has a default constructor that initializes the value to °. > > + if (ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY) > > + flags |= ControlId::Direction::Out; > > So you shouldn't be 'or'ing in here, just assign directly. > > Same for the two below. > > > + else if (ctrl.flags & V4L2_CTRL_FLAG_WRITE_ONLY) > > + flags |= ControlId::Direction::In; > > + else > > + flags |= ControlId::Direction::In | ControlId::Direction::Out; > > + > > + return std::make_unique<ControlId>(ctrl.id, name, "v4l2", type, flags); > > } > > > > /**
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 3cfe2de5973c..13bb914badbf 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -17,6 +17,7 @@ #include <vector> #include <libcamera/base/class.h> +#include <libcamera/base/flags.h> #include <libcamera/base/span.h> #include <libcamera/geometry.h> @@ -235,14 +236,24 @@ private: class ControlId { public: + enum class Direction { + In = (1 << 0), + Out = (1 << 1), + }; + + using DirectionFlags = Flags<Direction>; + ControlId(unsigned int id, const std::string &name, const std::string &vendor, - ControlType type, std::size_t size = 0, + ControlType type, DirectionFlags direction, + std::size_t size = 0, const std::map<std::string, int32_t> &enumStrMap = {}); unsigned int id() const { return id_; } const std::string &name() const { return name_; } const std::string &vendor() const { return vendor_; } ControlType type() const { return type_; } + bool isInput() const { return !!(direction_ & Direction::In); } + bool isOutput() const { return !!(direction_ & Direction::Out); } bool isArray() const { return size_ > 0; } std::size_t size() const { return size_; } const std::map<int32_t, std::string> &enumerators() const { return reverseMap_; } @@ -254,11 +265,14 @@ private: std::string name_; std::string vendor_; ControlType type_; + DirectionFlags direction_; std::size_t size_; std::map<std::string, int32_t> enumStrMap_; std::map<int32_t, std::string> reverseMap_; }; +LIBCAMERA_FLAGS_ENABLE_OPERATORS(ControlId::Direction) + static inline bool operator==(unsigned int lhs, const ControlId &rhs) { return lhs == rhs.id(); @@ -286,9 +300,10 @@ public: using type = T; Control(unsigned int id, const char *name, const char *vendor, + ControlId::DirectionFlags direction, const std::map<std::string, int32_t> &enumStrMap = {}) : ControlId(id, name, vendor, details::control_type<std::remove_cv_t<T>>::value, - details::control_type<std::remove_cv_t<T>>::size, enumStrMap) + direction, details::control_type<std::remove_cv_t<T>>::size, enumStrMap) { } diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h index 5fd13394fcef..980668c86bcc 100644 --- a/include/libcamera/ipa/ipa_controls.h +++ b/include/libcamera/ipa/ipa_controls.h @@ -46,7 +46,8 @@ struct ipa_control_info_entry { uint32_t id; uint32_t type; uint32_t offset; - uint32_t padding[1]; + uint8_t direction; + uint8_t padding[3]; }; #ifdef __cplusplus diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in index afe9e2c96610..65668d486dbc 100644 --- a/src/libcamera/control_ids.cpp.in +++ b/src/libcamera/control_ids.cpp.in @@ -89,9 +89,9 @@ extern const std::map<std::string, {{ctrl.type}}> {{ctrl.name}}NameValueMap = { { "{{enum.name}}", {{enum.name}} }, {%- endfor %} }; -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.name}}NameValueMap); +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.direction}}, {{ctrl.name}}NameValueMap); {% else -%} -extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}"); +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", "{{vendor}}", {{ctrl.direction}}); {% endif -%} {%- endfor %} diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index 0a5e8220a0ff..0804c178f0d4 100644 --- a/src/libcamera/control_serializer.cpp +++ b/src/libcamera/control_serializer.cpp @@ -281,6 +281,9 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, entry.id = id->id(); entry.type = id->type(); entry.offset = values.offset(); + entry.direction = + (id->isInput() ? static_cast<uint8_t>(ControlId::Direction::In) : 0) | + (id->isOutput() ? static_cast<uint8_t>(ControlId::Direction::Out) : 0); entries.write(&entry); store(info, values); @@ -493,12 +496,18 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & /* If we're using a local id map, populate it. */ if (localIdMap) { + ControlId::DirectionFlags flags; + if (entry->direction & static_cast<uint8_t>(ControlId::Direction::In)) + flags |= ControlId::Direction::In; + if (entry->direction & static_cast<uint8_t>(ControlId::Direction::Out)) + flags |= ControlId::Direction::Out; /** * \todo Find a way to preserve the control name for * debugging purpose. */ controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, - "", "local", type)); + "", "local", type, + flags)); (*localIdMap)[entry->id] = controlIds_.back().get(); } diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 2efecf0fc52b..23b71fac8e84 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -396,15 +396,16 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen * \param[in] name The control name * \param[in] vendor The vendor name * \param[in] type The control data type + * \param[in] direction The direction of the control, if it can be used in Controls or Metadata * \param[in] size The size of the array control, or 0 if scalar control * \param[in] enumStrMap The map from enum names to values (optional) */ ControlId::ControlId(unsigned int id, const std::string &name, const std::string &vendor, ControlType type, - std::size_t size, + DirectionFlags direction, std::size_t size, const std::map<std::string, int32_t> &enumStrMap) - : id_(id), name_(name), vendor_(vendor), type_(type), size_(size), - enumStrMap_(enumStrMap) + : id_(id), name_(name), vendor_(vendor), type_(type), + direction_(direction), size_(size), enumStrMap_(enumStrMap) { for (const auto &pair : enumStrMap_) reverseMap_[pair.second] = pair.first; @@ -434,6 +435,26 @@ ControlId::ControlId(unsigned int id, const std::string &name, * \return The control data type */ +/** + * \fn bool ControlId::isInput() const + * \brief Determine if the control is available to be used as an input control + * + * Controls can be used either as input in controls, or as output in metadata. + * This function checks if the control is allowed to be used as the former. + * + * \return True if the control can be used as an input control, false otherwise + */ + +/** + * \fn bool ControlId::isOutput() const + * \brief Determine if the control is available to be used in output metadata + * + * Controls can be used either as input in controls, or as output in metadata. + * This function checks if the control is allowed to be used as the latter. + * + * \return True if the control can be returned in output metadata, false otherwise + */ + /** * \fn bool ControlId::isArray() const * \brief Determine if the control is an array control @@ -471,6 +492,22 @@ ControlId::ControlId(unsigned int id, const std::string &name, * \return True if \a lhs.id() is equal to \a rhs, false otherwise */ +/** + * \enum ControlId::Direction + * \brief The direction the control is capable of being passed from/to + * + * \var ControlId::Direction::In + * \brief The control can be passed as input in controls + * + * \var ControlId::Direction::Out + * \brief The control can be returned as output in metadata + */ + +/** + * \typedef ControlId::DirectionFlags + * \brief A wrapper for ControlId::Direction so that it can be used as flags + */ + /** * \class Control * \brief Describe a control and its intrinsic properties @@ -504,6 +541,8 @@ ControlId::ControlId(unsigned int id, const std::string &name, * \param[in] id The control numerical ID * \param[in] name The control name * \param[in] vendor The vendor name + * \param[in] direction The direction of the control, if it can be used in + * Controls or Metadata * \param[in] enumStrMap The map from enum names to values (optional) * * The control data type is automatically deduced from the template type T. diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 7d21cf15fec1..4634ccc392b6 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -520,7 +520,15 @@ std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl & const std::string name(static_cast<const char *>(ctrl.name), len); const ControlType type = v4l2CtrlType(ctrl.type); - return std::make_unique<ControlId>(ctrl.id, name, "v4l2", type); + ControlId::DirectionFlags flags; + if (ctrl.flags & V4L2_CTRL_FLAG_READ_ONLY) + flags |= ControlId::Direction::Out; + else if (ctrl.flags & V4L2_CTRL_FLAG_WRITE_ONLY) + flags |= ControlId::Direction::In; + else + flags |= ControlId::Direction::In | ControlId::Direction::Out; + + return std::make_unique<ControlId>(ctrl.id, name, "v4l2", type, flags); } /**
Add support to ControlId for querying direction information. This allows applications to query whether a ControlId is meant for being set in controls or to be returned in metadata or both. This also has a side effect of properly encoding this information, as previously it was only mentioned losely and inconsistently in the control id definition. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Changes in v3: - make direction parameter to ControlId and Control consutrctors mandatory - this entails expanding the control serializer/deserializer for V4L2Device and ControlSerializer to properly handle the direction - also the constructor parameters have to be reordered, compared to the last vesrion at least Changes in v2: - simplify code --- include/libcamera/controls.h | 19 ++++++++++-- include/libcamera/ipa/ipa_controls.h | 3 +- src/libcamera/control_ids.cpp.in | 4 +-- src/libcamera/control_serializer.cpp | 11 ++++++- src/libcamera/controls.cpp | 45 ++++++++++++++++++++++++++-- src/libcamera/v4l2_device.cpp | 10 ++++++- 6 files changed, 82 insertions(+), 10 deletions(-)