[libcamera-devel] [PATCH v2 1/2] libcamera: controls: Add read-only flag to ControlInfo

Kieran Bingham kieran.bingham at ideasonboard.com
Tue Sep 5 01:04:36 CEST 2023


Quoting Kieran Bingham (2023-01-30 13:39:07)
> Quoting Naushir Patuck via libcamera-devel (2022-12-12 10:16:35)
> > Hi Jacopo,
> > 
> > Thank you for your feedback!
> > 
> > On Mon, 12 Dec 2022 at 09:50, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > 
> > > Hi Naush
> > >
> > > On Fri, Dec 02, 2022 at 12:40:04PM +0000, Naushir Patuck via
> > > libcamera-devel wrote:
> > > > Add a read-only flag to the ControlInfo flag to indicate whether a V4L2
> > > control
> > > > is read-only or not. This flag only makes sense for V2L2 controls at
> > > preset, so
> > > > only update the ControlInfo constructor signature used by the V4L2Device
> > > class
> > > > to set the value of the flag.
> > > >
> > >
> > > It feels a bit the three constructors are mis-aligned now :/
> > >
> > 
> > > I would have gone for a default parameter for all three of them to be
> > > honest... What do others think ?
> > >
> > 
> > I do kind of agree with this, the constructor signatures were the one bit
> > where I was unsure what to do.
> > 
> > Happy to change all the constructors to have a readOnly param if others
> > agree!
> 
> It's tough to know without knowing future use-cases, but if we have one
> type of read-only control, it's probably fair to assume or expect that
> it's a 'feature' of controls, and should be possible to set on all
> types.
> 
> So I think this is worth having on all of them yes.

So - I've been hitting the issue resolved by this series with a driver
that has a read-only HBLANK control but the min/max are not == value.

I don't know if that makes sense yet in the driver, and I 'fixed' this
locally by setting min==max==value in the driver - but it feels somewhat
like changing the kernel to fix a libcamera limitation...

So - resuming this series.

It seems quite difficult to introduce the extra argument to all the
constructors without facing issues though, where this affects which
constructor gets chosen ...

../../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp: In member function ‘void libcamera::UVCCameraData::addControl(uint32_t, const libcamera::ControlInfo&, libcamera::ControlInfoMap::Map*)’:
../../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:629:57: error: narrowing conversion of ‘((float)(min - def) / scale)’ from ‘float’ to ‘bool’ [-Werror=narrowing]
  629 |                         { static_cast<float>(min - def) / scale },
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
../../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:629:57: error: narrowing conversion of ‘((float)(min - def) / scale)’ from ‘float’ to ‘bool’ [-Werror=narrowing]
../../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:630:57: error: narrowing conversion of ‘((float)(max - def) / scale)’ from ‘float’ to ‘bool’ [-Werror=narrowing]
  630 |                         { static_cast<float>(max - def) / scale },
      |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
../../src/libcamera/pipeline/uvcvideo/uvcvideo.cpp:632:17: error: narrowing conversion of ‘0.0f’ from ‘float’ to ‘bool’ [-Wnarrowing]
  632 |                 };
      |                 ^

I presume this is because the compiler could implicitly map those types
to bool when trying to construct the usual 3 parameter ControlInfo which
could match the now 3 parameter bool constructor.

I wonder if maybe it's easier to just leave it as supported on the
single min/max/def constructor indeed. It could be extended if it became
required later...



Here's my current diff/fixup on this patch:





diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 802dd906937b..8af304bdeac9 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -484,7 +484,7 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
  * \param[in] min The control minimum value
  * \param[in] max The control maximum value
  * \param[in] def The control default value
- * \param[in] readOnly Read-only status of a V4L2 control
+ * \param[in] readOnly True if the control reports a dynamic property
  */
 ControlInfo::ControlInfo(const ControlValue &min,
                         const ControlValue &max,
@@ -510,6 +510,7 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
        min_ = values.front();
        max_ = values.back();
        def_ = !def.isNone() ? def : values.front();
+       readOnly_ = false;

        values_.reserve(values.size());
        for (const ControlValue &value : values)
@@ -541,7 +542,7 @@ ControlInfo::ControlInfo(std::set<bool> values, bool def)
  * value. The minimum, maximum, and default values will all be \a value.
  */
 ControlInfo::ControlInfo(bool value)
-       : min_(value), max_(value), def_(value), readOnly_(false)
+       : min_(value), max_(value), def_(value), readOnly_(true)
 {
        values_ = { value };
 }
@@ -576,8 +577,12 @@ ControlInfo::ControlInfo(bool value)

 /**
  * \fn ControlInfo::readOnly()
- * \brief Identifies if a V4L2 control is flagged as read-only
- * \return True if the V4L2 control is read-only, false otherwise
+ * \brief Identifies if a control is flagged as read-only
+ * \return True if the control is read-only, false otherwise
+ *
+ * Read-only controls may signify that the control is a volatile property and
+ * can not be set. Adding a read-only control to a control list may cause the
+ * list to fail when the list is submitted.
  */

 /**


--
Kieran


> 
> 
> 
> > 
> > Regards,
> > Naush
> > 
> > 
> > 
> > >
> > > > Add a ControlInfo::readOnly() member function to retrive this flag.
> 
> s/retrive/retrieve/
> 
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > ---
> > > >  include/libcamera/controls.h  |  5 ++++-
> > > >  src/libcamera/controls.cpp    | 17 +++++++++++++----
> > > >  src/libcamera/v4l2_device.cpp | 12 ++++++++----
> > > >  3 files changed, 25 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > index cf94205577a5..488663a7ba04 100644
> > > > --- a/include/libcamera/controls.h
> > > > +++ b/include/libcamera/controls.h
> > > > @@ -270,7 +270,8 @@ class ControlInfo
> > > >  public:
> > > >       explicit ControlInfo(const ControlValue &min = {},
> > > >                            const ControlValue &max = {},
> > > > -                          const ControlValue &def = {});
> > > > +                          const ControlValue &def = {},
> > > > +                          bool readOnly = false);
> > > >       explicit ControlInfo(Span<const ControlValue> values,
> > > >                            const ControlValue &def = {});
> > > >       explicit ControlInfo(std::set<bool> values, bool def);
> > > > @@ -279,6 +280,7 @@ public:
> > > >       const ControlValue &min() const { return min_; }
> > > >       const ControlValue &max() const { return max_; }
> > > >       const ControlValue &def() const { return def_; }
> > > > +     bool readOnly() const { return readOnly_; }
> > > >       const std::vector<ControlValue> &values() const { return values_; }
> > > >
> > > >       std::string toString() const;
> > > > @@ -297,6 +299,7 @@ private:
> > > >       ControlValue min_;
> > > >       ControlValue max_;
> > > >       ControlValue def_;
> > > > +     bool readOnly_;
> > > >       std::vector<ControlValue> values_;
> > > >  };
> > > >
> > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > > > index 6dbf9b348709..fc66abad600d 100644
> > > > --- a/src/libcamera/controls.cpp
> > > > +++ b/src/libcamera/controls.cpp
> > > > @@ -484,11 +484,13 @@ void ControlValue::reserve(ControlType type, bool
> > > isArray, std::size_t numElemen
> > > >   * \param[in] min The control minimum value
> > > >   * \param[in] max The control maximum value
> > > >   * \param[in] def The control default value
> > > > + * \param[in] readOnly Read-only status of a V4L2 control
> > > >   */
> > > >  ControlInfo::ControlInfo(const ControlValue &min,
> > > >                        const ControlValue &max,
> > > > -                      const ControlValue &def)
> > > > -     : min_(min), max_(max), def_(def)
> > > > +                      const ControlValue &def,
> > > > +                      bool readOnly)
> > > > +     : min_(min), max_(max), def_(def), readOnly_(readOnly)
> > > >  {
> > > >  }
> > > >
> > > > @@ -525,7 +527,8 @@ ControlInfo::ControlInfo(Span<const ControlValue>
> > > values,
> > > >   * default value is \a def.
> > > >   */
> > > >  ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > > > -     : min_(false), max_(true), def_(def), values_({ false, true })
> > > > +     : min_(false), max_(true), def_(def), readOnly_(false),
> > > > +       values_({ false, true })
> > > >  {
> > > >       ASSERT(values.count(def) && values.size() == 2);
> > > >  }
> > > > @@ -538,7 +541,7 @@ ControlInfo::ControlInfo(std::set<bool> values, bool
> > > def)
> > > >   * value. The minimum, maximum, and default values will all be \a value.
> > > >   */
> > > >  ControlInfo::ControlInfo(bool value)
> > > > -     : min_(value), max_(value), def_(value)
> > > > +     : min_(value), max_(value), def_(value), readOnly_(false)
> > > >  {
> > > >       values_ = { value };
> > > >  }
> > > > @@ -571,6 +574,12 @@ ControlInfo::ControlInfo(bool value)
> > > >   * \return A ControlValue with the default value for the control
> > > >   */
> > > >
> > > > +/**
> > > > + * \fn ControlInfo::readOnly()
> > > > + * \brief Identifies if a V4L2 control is flagged as read-only
> > > > + * \return True if the V4L2 control is read-only, false otherwise
> > >
> > > I would not mention V4L2 here and keep the documentation generic
> > >
> > > > + */
> > > > +
> > > >  /**
> > > >   * \fn ControlInfo::values()
> > > >   * \brief Retrieve the list of valid values
> > > > diff --git a/src/libcamera/v4l2_device.cpp
> > > b/src/libcamera/v4l2_device.cpp
> > > > index 57a88d96b12c..9018f1b0b9e1 100644
> > > > --- a/src/libcamera/v4l2_device.cpp
> > > > +++ b/src/libcamera/v4l2_device.cpp
> > > > @@ -535,17 +535,20 @@ std::optional<ControlInfo>
> > > V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl
> > > >       case V4L2_CTRL_TYPE_U8:
> > > >               return ControlInfo(static_cast<uint8_t>(ctrl.minimum),
> > > >                                  static_cast<uint8_t>(ctrl.maximum),
> > > > -
> > > static_cast<uint8_t>(ctrl.default_value));
> > > > +
> > > static_cast<uint8_t>(ctrl.default_value),
> > > > +                                !!(ctrl.flags &
> > > V4L2_CTRL_FLAG_READ_ONLY));
> > > >
> > > >       case V4L2_CTRL_TYPE_BOOLEAN:
> > > >               return ControlInfo(static_cast<bool>(ctrl.minimum),
> > > >                                  static_cast<bool>(ctrl.maximum),
> > > > -                                static_cast<bool>(ctrl.default_value));
> > > > +                                static_cast<bool>(ctrl.default_value),
> > > > +                                !!(ctrl.flags &
> > > V4L2_CTRL_FLAG_READ_ONLY));
> > > >
> > > >       case V4L2_CTRL_TYPE_INTEGER64:
> > > >               return ControlInfo(static_cast<int64_t>(ctrl.minimum),
> > > >                                  static_cast<int64_t>(ctrl.maximum),
> > > > -
> > > static_cast<int64_t>(ctrl.default_value));
> > > > +
> > > static_cast<int64_t>(ctrl.default_value),
> > > > +                                !!(ctrl.flags &
> > > V4L2_CTRL_FLAG_READ_ONLY));
> > > >
> > > >       case V4L2_CTRL_TYPE_INTEGER_MENU:
> > > >       case V4L2_CTRL_TYPE_MENU:
> > > > @@ -554,7 +557,8 @@ std::optional<ControlInfo>
> > > V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl
> > > >       default:
> > > >               return ControlInfo(static_cast<int32_t>(ctrl.minimum),
> > > >                                  static_cast<int32_t>(ctrl.maximum),
> > > > -
> > > static_cast<int32_t>(ctrl.default_value));
> > > > +
> > > static_cast<int32_t>(ctrl.default_value),
> > > > +                                !!(ctrl.flags &
> > > V4L2_CTRL_FLAG_READ_ONLY));
> > > >       }
> > > >  }
> > > >
> > > > --
> > > > 2.25.1
> > > >
> > >


More information about the libcamera-devel mailing list