[v2,1/2] libcamera: Allow enumerating u32 control type
diff mbox series

Message ID 20241022095737.4127210-2-chenghaoyang@chromium.org
State Superseded
Headers show
Series
  • Add U16 & U32 support in controls
Related show

Commit Message

Harvey Yang Oct. 22, 2024, 9:56 a.m. UTC 
From: Yudhistira Erlandinata <yerlandinata@chromium.org>

Only allowing V4L2_CTRL_TYPE_U32 control type to be listed in the
V4L2Device::controls_, so it can be used together with function
V4L2Device::setExtControl. Like many other control types, this type
is still not supported in the V4L2Device::getControls and
V4L2Device::setControls.

Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 include/libcamera/controls.h  |  7 +++++++
 src/libcamera/controls.cpp    | 12 ++++++++++--
 src/libcamera/v4l2_device.cpp | 13 +++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

Comments

Kieran Bingham Oct. 24, 2024, 10:58 p.m. UTC | #1 
Quoting Harvey Yang (2024-10-22 10:56:04)
> From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> 
> Only allowing V4L2_CTRL_TYPE_U32 control type to be listed in the
> V4L2Device::controls_, so it can be used together with function
> V4L2Device::setExtControl. Like many other control types, this type
> is still not supported in the V4L2Device::getControls and
> V4L2Device::setControls.

I like this patch, but the commit message ...

'Only allowing' ... sure - of course it's only allowing...  And indeed -
many other types aren't supported, if they aren't used yet... Or are you
saying there is some missing implementation that even this patch doesn't
yet add? Or does this patch 'fix' {get,set}Controls?


Lets try something more simple and clear:

"""
V4L2 Controls support a wide variety of types not yet supported by the
ControlValue type system.

Extend the libcamera ControlValue types to support an explicit 32 bit
unsigned integer type, and map that to the corresponding
V4L2_CTRL_TYPE_U32 type within the v4l2_device support class.
"""

I think the same/similar could be said for the Unsigned 16-bits Control
type too..

> 
> Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  include/libcamera/controls.h  |  7 +++++++
>  src/libcamera/controls.cpp    | 12 ++++++++++--
>  src/libcamera/v4l2_device.cpp | 13 +++++++++++++

All the other ControlTypes look like they have unit tests in
test/controls/control_value.cpp.

Could you add something in there for Unsigned32 please?

With that to align with the other types I'd like to see this merged.



>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index ca60bbaca..6da8ad2c3 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -29,6 +29,7 @@ enum ControlType {
>         ControlTypeNone,
>         ControlTypeBool,
>         ControlTypeByte,
> +       ControlTypeUnsigned32,
>         ControlTypeInteger32,
>         ControlTypeInteger64,
>         ControlTypeFloat,
> @@ -62,6 +63,12 @@ struct control_type<uint8_t> {
>         static constexpr std::size_t size = 0;
>  };
>  
> +template<>
> +struct control_type<uint32_t> {
> +       static constexpr ControlType value = ControlTypeUnsigned32;
> +       static constexpr std::size_t size = 0;
> +};
> +
>  template<>
>  struct control_type<int32_t> {
>         static constexpr ControlType value = ControlTypeInteger32;
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 62185d643..8ae295191 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -54,6 +54,7 @@ static constexpr size_t ControlValueSize[] = {
>         [ControlTypeNone]               = 0,
>         [ControlTypeBool]               = sizeof(bool),
>         [ControlTypeByte]               = sizeof(uint8_t),
> +       [ControlTypeUnsigned32]         = sizeof(uint32_t),
>         [ControlTypeInteger32]          = sizeof(int32_t),
>         [ControlTypeInteger64]          = sizeof(int64_t),
>         [ControlTypeFloat]              = sizeof(float),
> @@ -74,10 +75,12 @@ static constexpr size_t ControlValueSize[] = {
>   * The control stores a boolean value
>   * \var ControlTypeByte
>   * The control stores a byte value as an unsigned 8-bit integer
> + * \var ControlTypeUnsigned32
> + * The control stores an unsigned 32-bit integer value
>   * \var ControlTypeInteger32
> - * The control stores a 32-bit integer value
> + * The control stores a signed 32-bit integer value
>   * \var ControlTypeInteger64
> - * The control stores a 64-bit integer value
> + * The control stores a signed 64-bit integer value
>   * \var ControlTypeFloat
>   * The control stores a 32-bit floating point value
>   * \var ControlTypeString
> @@ -230,6 +233,11 @@ std::string ControlValue::toString() const
>                         str += std::to_string(*value);
>                         break;
>                 }
> +               case ControlTypeUnsigned32: {
> +                       const uint32_t *value = reinterpret_cast<const uint32_t *>(data);
> +                       str += std::to_string(*value);
> +                       break;
> +               }
>                 case ControlTypeInteger32: {
>                         const int32_t *value = reinterpret_cast<const int32_t *>(data);
>                         str += std::to_string(*value);
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 68add4f2e..0ba8dcfa0 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <fcntl.h>
>  #include <map>
> +#include <stdint.h>

Curious that this wasn't already in, as the types are already used - but
I think it's correct to add it ...

Regards
--
Kieran

>  #include <stdlib.h>
>  #include <string.h>
>  #include <sys/ioctl.h>
> @@ -17,11 +18,14 @@
>  #include <vector>
>  
>  #include <linux/v4l2-mediabus.h>
> +#include <linux/videodev2.h>
>  
>  #include <libcamera/base/event_notifier.h>
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/utils.h>
>  
> +#include <libcamera/controls.h>
> +
>  #include "libcamera/internal/formats.h"
>  #include "libcamera/internal/sysfs.h"
>  
> @@ -488,6 +492,9 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
>         case V4L2_CTRL_TYPE_BOOLEAN:
>                 return ControlTypeBool;
>  
> +       case V4L2_CTRL_TYPE_U32:
> +               return ControlTypeUnsigned32;
> +
>         case V4L2_CTRL_TYPE_INTEGER:
>                 return ControlTypeInteger32;
>  
> @@ -536,6 +543,11 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl
>                                    static_cast<uint8_t>(ctrl.maximum),
>                                    static_cast<uint8_t>(ctrl.default_value));
>  
> +       case V4L2_CTRL_TYPE_U32:
> +               return ControlInfo(static_cast<uint32_t>(ctrl.minimum),
> +                                  static_cast<uint32_t>(ctrl.maximum),
> +                                  static_cast<uint32_t>(ctrl.default_value));
> +
>         case V4L2_CTRL_TYPE_BOOLEAN:
>                 return ControlInfo(static_cast<bool>(ctrl.minimum),
>                                    static_cast<bool>(ctrl.maximum),
> @@ -622,6 +634,7 @@ void V4L2Device::listControls()
>                 case V4L2_CTRL_TYPE_BITMASK:
>                 case V4L2_CTRL_TYPE_INTEGER_MENU:
>                 case V4L2_CTRL_TYPE_U8:
> +               case V4L2_CTRL_TYPE_U32:
>                         break;
>                 /* \todo Support other control types. */
>                 default:
> -- 
> 2.47.0.105.g07ac214952-goog
>
Harvey Yang Oct. 25, 2024, 5:32 a.m. UTC | #2 
Hi Kieran,

On Fri, Oct 25, 2024 at 6:58 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Harvey Yang (2024-10-22 10:56:04)
> > From: Yudhistira Erlandinata <yerlandinata@chromium.org>
> >
> > Only allowing V4L2_CTRL_TYPE_U32 control type to be listed in the
> > V4L2Device::controls_, so it can be used together with function
> > V4L2Device::setExtControl. Like many other control types, this type
> > is still not supported in the V4L2Device::getControls and
> > V4L2Device::setControls.
>
> I like this patch, but the commit message ...
>
> 'Only allowing' ... sure - of course it's only allowing...  And indeed -
> many other types aren't supported, if they aren't used yet... Or are you
> saying there is some missing implementation that even this patch doesn't
> yet add? Or does this patch 'fix' {get,set}Controls?
>
>
> Lets try something more simple and clear:
>
> """
> V4L2 Controls support a wide variety of types not yet supported by the
> ControlValue type system.
>
> Extend the libcamera ControlValue types to support an explicit 32 bit
> unsigned integer type, and map that to the corresponding
> V4L2_CTRL_TYPE_U32 type within the v4l2_device support class.
> """
>
> I think the same/similar could be said for the Unsigned 16-bits Control
> type too..

Thanks! Updated.

>
> >
> > Signed-off-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  include/libcamera/controls.h  |  7 +++++++
> >  src/libcamera/controls.cpp    | 12 ++++++++++--
> >  src/libcamera/v4l2_device.cpp | 13 +++++++++++++
>
> All the other ControlTypes look like they have unit tests in
> test/controls/control_value.cpp.
>
> Could you add something in there for Unsigned32 please?

Sure, added and tested on gitlab pipeline.

BR,
Harvey

>
> With that to align with the other types I'd like to see this merged.
>
>
>
> >  3 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index ca60bbaca..6da8ad2c3 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -29,6 +29,7 @@ enum ControlType {
> >         ControlTypeNone,
> >         ControlTypeBool,
> >         ControlTypeByte,
> > +       ControlTypeUnsigned32,
> >         ControlTypeInteger32,
> >         ControlTypeInteger64,
> >         ControlTypeFloat,
> > @@ -62,6 +63,12 @@ struct control_type<uint8_t> {
> >         static constexpr std::size_t size = 0;
> >  };
> >
> > +template<>
> > +struct control_type<uint32_t> {
> > +       static constexpr ControlType value = ControlTypeUnsigned32;
> > +       static constexpr std::size_t size = 0;
> > +};
> > +
> >  template<>
> >  struct control_type<int32_t> {
> >         static constexpr ControlType value = ControlTypeInteger32;
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 62185d643..8ae295191 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -54,6 +54,7 @@ static constexpr size_t ControlValueSize[] = {
> >         [ControlTypeNone]               = 0,
> >         [ControlTypeBool]               = sizeof(bool),
> >         [ControlTypeByte]               = sizeof(uint8_t),
> > +       [ControlTypeUnsigned32]         = sizeof(uint32_t),
> >         [ControlTypeInteger32]          = sizeof(int32_t),
> >         [ControlTypeInteger64]          = sizeof(int64_t),
> >         [ControlTypeFloat]              = sizeof(float),
> > @@ -74,10 +75,12 @@ static constexpr size_t ControlValueSize[] = {
> >   * The control stores a boolean value
> >   * \var ControlTypeByte
> >   * The control stores a byte value as an unsigned 8-bit integer
> > + * \var ControlTypeUnsigned32
> > + * The control stores an unsigned 32-bit integer value
> >   * \var ControlTypeInteger32
> > - * The control stores a 32-bit integer value
> > + * The control stores a signed 32-bit integer value
> >   * \var ControlTypeInteger64
> > - * The control stores a 64-bit integer value
> > + * The control stores a signed 64-bit integer value
> >   * \var ControlTypeFloat
> >   * The control stores a 32-bit floating point value
> >   * \var ControlTypeString
> > @@ -230,6 +233,11 @@ std::string ControlValue::toString() const
> >                         str += std::to_string(*value);
> >                         break;
> >                 }
> > +               case ControlTypeUnsigned32: {
> > +                       const uint32_t *value = reinterpret_cast<const uint32_t *>(data);
> > +                       str += std::to_string(*value);
> > +                       break;
> > +               }
> >                 case ControlTypeInteger32: {
> >                         const int32_t *value = reinterpret_cast<const int32_t *>(data);
> >                         str += std::to_string(*value);
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 68add4f2e..0ba8dcfa0 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -9,6 +9,7 @@
> >
> >  #include <fcntl.h>
> >  #include <map>
> > +#include <stdint.h>
>
> Curious that this wasn't already in, as the types are already used - but
> I think it's correct to add it ...
>
> Regards
> --
> Kieran
>
> >  #include <stdlib.h>
> >  #include <string.h>
> >  #include <sys/ioctl.h>
> > @@ -17,11 +18,14 @@
> >  #include <vector>
> >
> >  #include <linux/v4l2-mediabus.h>
> > +#include <linux/videodev2.h>
> >
> >  #include <libcamera/base/event_notifier.h>
> >  #include <libcamera/base/log.h>
> >  #include <libcamera/base/utils.h>
> >
> > +#include <libcamera/controls.h>
> > +
> >  #include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/sysfs.h"
> >
> > @@ -488,6 +492,9 @@ ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
> >         case V4L2_CTRL_TYPE_BOOLEAN:
> >                 return ControlTypeBool;
> >
> > +       case V4L2_CTRL_TYPE_U32:
> > +               return ControlTypeUnsigned32;
> > +
> >         case V4L2_CTRL_TYPE_INTEGER:
> >                 return ControlTypeInteger32;
> >
> > @@ -536,6 +543,11 @@ std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl
> >                                    static_cast<uint8_t>(ctrl.maximum),
> >                                    static_cast<uint8_t>(ctrl.default_value));
> >
> > +       case V4L2_CTRL_TYPE_U32:
> > +               return ControlInfo(static_cast<uint32_t>(ctrl.minimum),
> > +                                  static_cast<uint32_t>(ctrl.maximum),
> > +                                  static_cast<uint32_t>(ctrl.default_value));
> > +
> >         case V4L2_CTRL_TYPE_BOOLEAN:
> >                 return ControlInfo(static_cast<bool>(ctrl.minimum),
> >                                    static_cast<bool>(ctrl.maximum),
> > @@ -622,6 +634,7 @@ void V4L2Device::listControls()
> >                 case V4L2_CTRL_TYPE_BITMASK:
> >                 case V4L2_CTRL_TYPE_INTEGER_MENU:
> >                 case V4L2_CTRL_TYPE_U8:
> > +               case V4L2_CTRL_TYPE_U32:
> >                         break;
> >                 /* \todo Support other control types. */
> >                 default:
> > --
> > 2.47.0.105.g07ac214952-goog
> >

Patch
diff mbox series 

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index ca60bbaca..6da8ad2c3 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -29,6 +29,7 @@  enum ControlType {
 	ControlTypeNone,
 	ControlTypeBool,
 	ControlTypeByte,
+	ControlTypeUnsigned32,
 	ControlTypeInteger32,
 	ControlTypeInteger64,
 	ControlTypeFloat,
@@ -62,6 +63,12 @@  struct control_type<uint8_t> {
 	static constexpr std::size_t size = 0;
 };
 
+template<>
+struct control_type<uint32_t> {
+	static constexpr ControlType value = ControlTypeUnsigned32;
+	static constexpr std::size_t size = 0;
+};
+
 template<>
 struct control_type<int32_t> {
 	static constexpr ControlType value = ControlTypeInteger32;
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 62185d643..8ae295191 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -54,6 +54,7 @@  static constexpr size_t ControlValueSize[] = {
 	[ControlTypeNone]		= 0,
 	[ControlTypeBool]		= sizeof(bool),
 	[ControlTypeByte]		= sizeof(uint8_t),
+	[ControlTypeUnsigned32]		= sizeof(uint32_t),
 	[ControlTypeInteger32]		= sizeof(int32_t),
 	[ControlTypeInteger64]		= sizeof(int64_t),
 	[ControlTypeFloat]		= sizeof(float),
@@ -74,10 +75,12 @@  static constexpr size_t ControlValueSize[] = {
  * The control stores a boolean value
  * \var ControlTypeByte
  * The control stores a byte value as an unsigned 8-bit integer
+ * \var ControlTypeUnsigned32
+ * The control stores an unsigned 32-bit integer value
  * \var ControlTypeInteger32
- * The control stores a 32-bit integer value
+ * The control stores a signed 32-bit integer value
  * \var ControlTypeInteger64
- * The control stores a 64-bit integer value
+ * The control stores a signed 64-bit integer value
  * \var ControlTypeFloat
  * The control stores a 32-bit floating point value
  * \var ControlTypeString
@@ -230,6 +233,11 @@  std::string ControlValue::toString() const
 			str += std::to_string(*value);
 			break;
 		}
+		case ControlTypeUnsigned32: {
+			const uint32_t *value = reinterpret_cast<const uint32_t *>(data);
+			str += std::to_string(*value);
+			break;
+		}
 		case ControlTypeInteger32: {
 			const int32_t *value = reinterpret_cast<const int32_t *>(data);
 			str += std::to_string(*value);
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 68add4f2e..0ba8dcfa0 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -9,6 +9,7 @@ 
 
 #include <fcntl.h>
 #include <map>
+#include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/ioctl.h>
@@ -17,11 +18,14 @@ 
 #include <vector>
 
 #include <linux/v4l2-mediabus.h>
+#include <linux/videodev2.h>
 
 #include <libcamera/base/event_notifier.h>
 #include <libcamera/base/log.h>
 #include <libcamera/base/utils.h>
 
+#include <libcamera/controls.h>
+
 #include "libcamera/internal/formats.h"
 #include "libcamera/internal/sysfs.h"
 
@@ -488,6 +492,9 @@  ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
 	case V4L2_CTRL_TYPE_BOOLEAN:
 		return ControlTypeBool;
 
+	case V4L2_CTRL_TYPE_U32:
+		return ControlTypeUnsigned32;
+
 	case V4L2_CTRL_TYPE_INTEGER:
 		return ControlTypeInteger32;
 
@@ -536,6 +543,11 @@  std::optional<ControlInfo> V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl
 				   static_cast<uint8_t>(ctrl.maximum),
 				   static_cast<uint8_t>(ctrl.default_value));
 
+	case V4L2_CTRL_TYPE_U32:
+		return ControlInfo(static_cast<uint32_t>(ctrl.minimum),
+				   static_cast<uint32_t>(ctrl.maximum),
+				   static_cast<uint32_t>(ctrl.default_value));
+
 	case V4L2_CTRL_TYPE_BOOLEAN:
 		return ControlInfo(static_cast<bool>(ctrl.minimum),
 				   static_cast<bool>(ctrl.maximum),
@@ -622,6 +634,7 @@  void V4L2Device::listControls()
 		case V4L2_CTRL_TYPE_BITMASK:
 		case V4L2_CTRL_TYPE_INTEGER_MENU:
 		case V4L2_CTRL_TYPE_U8:
+		case V4L2_CTRL_TYPE_U32:
 			break;
 		/* \todo Support other control types. */
 		default: