| Message ID | 20241025053111.519449-2-chenghaoyang@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey On Fri, Oct 25, 2024 at 05:30:13AM +0000, Harvey Yang wrote: > From: Yudhistira Erlandinata <yerlandinata@chromium.org> > > 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. V4L2_CTRL_TYPE_U32 is a control compound type. /* Compound types are >= 0x0100 */ V4L2_CTRL_COMPOUND_TYPES = 0x0100, V4L2_CTRL_TYPE_U8 = 0x0100, V4L2_CTRL_TYPE_U16 = 0x0101, V4L2_CTRL_TYPE_U32 = 0x0102, V4L2_CTRL_TYPE_AREA = 0x0106, This means it carries a payload and needs to be handled like V4L2_CTRL_TYPE_U8 is, specifcially when it comes to parsing it from the V4L2 interface into a libcamera control. I'm surprised you don't hit this in V4L2Device::getControls() if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { ControlType type; switch (info.type) { case V4L2_CTRL_TYPE_U8: type = ControlTypeByte; break; default: LOG(V4L2, Error) << "Unsupported payload control type " << info.type; return {}; } > > 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 +++++++++++ > test/controls/control_value.cpp | 40 +++++++++++++++++++++++++++++++++ > 4 files changed, 70 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> > #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> Included by the header file > > #include <libcamera/base/event_notifier.h> > #include <libcamera/base/log.h> > #include <libcamera/base/utils.h> > > +#include <libcamera/controls.h> > + This as well > #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: > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > index 344107fae..6ca85b739 100644 > --- a/test/controls/control_value.cpp > +++ b/test/controls/control_value.cpp > @@ -109,6 +109,46 @@ protected: > return TestFail; > } > > + /* > + * Unsigned Integer32 type. > + */ > + value.set(static_cast<uint32_t>(42)); > + if (value.isNone() || value.isArray() || > + value.type() != ControlTypeUnsigned32) { > + cerr << "Control type mismatch after setting to uint32_t" << endl; > + return TestFail; > + } > + > + if (value.get<uint32_t>() != 42) { > + cerr << "Control value mismatch after setting to uint32_t" << endl; > + return TestFail; > + } > + > + if (value.toString() != "42") { > + cerr << "Control string mismatch after setting to uint32_t" << endl; > + return TestFail; > + } > + > + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; > + value.set(Span<uint32_t>(uint32s)); > + if (value.isNone() || !value.isArray() || > + value.type() != ControlTypeUnsigned32) { > + cerr << "Control type mismatch after setting to uint32_t array" << endl; > + return TestFail; > + } > + > + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); > + if (uint32s.size() != uint32sResult.size() || > + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { > + cerr << "Control value mismatch after setting to uint32_t array" << endl; > + return TestFail; > + } > + > + if (value.toString() != "[ 3, 14, 15, 9 ]") { > + cerr << "Control string mismatch after setting to uint32_t array" << endl; > + return TestFail; > + } > + > /* > * Integer32 type. > */ > -- > 2.47.0.163.g1226f6d8fa-goog >
Hi Jacopo, On Fri, Oct 25, 2024 at 5:15 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey > > On Fri, Oct 25, 2024 at 05:30:13AM +0000, Harvey Yang wrote: > > From: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > 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. > > V4L2_CTRL_TYPE_U32 is a control compound type. > > /* Compound types are >= 0x0100 */ > V4L2_CTRL_COMPOUND_TYPES = 0x0100, > V4L2_CTRL_TYPE_U8 = 0x0100, > V4L2_CTRL_TYPE_U16 = 0x0101, > V4L2_CTRL_TYPE_U32 = 0x0102, > V4L2_CTRL_TYPE_AREA = 0x0106, > > This means it carries a payload and needs to be handled like > V4L2_CTRL_TYPE_U8 is, specifcially when it comes to parsing it from > the V4L2 interface into a libcamera control. > > I'm surprised you don't hit this in V4L2Device::getControls() > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > ControlType type; > > switch (info.type) { > case V4L2_CTRL_TYPE_U8: > type = ControlTypeByte; > break; > > default: > LOG(V4L2, Error) > << "Unsupported payload control type " > << info.type; > return {}; > } > Thanks! Added for U32 and U16. > > > > 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 +++++++++++ > > test/controls/control_value.cpp | 40 +++++++++++++++++++++++++++++++++ > > 4 files changed, 70 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> > > #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> > > Included by the header file Removed. > > > > > #include <libcamera/base/event_notifier.h> > > #include <libcamera/base/log.h> > > #include <libcamera/base/utils.h> > > > > +#include <libcamera/controls.h> > > + > > This as well Removed. BR, Harvey > > > #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: > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > index 344107fae..6ca85b739 100644 > > --- a/test/controls/control_value.cpp > > +++ b/test/controls/control_value.cpp > > @@ -109,6 +109,46 @@ protected: > > return TestFail; > > } > > > > + /* > > + * Unsigned Integer32 type. > > + */ > > + value.set(static_cast<uint32_t>(42)); > > + if (value.isNone() || value.isArray() || > > + value.type() != ControlTypeUnsigned32) { > > + cerr << "Control type mismatch after setting to uint32_t" << endl; > > + return TestFail; > > + } > > + > > + if (value.get<uint32_t>() != 42) { > > + cerr << "Control value mismatch after setting to uint32_t" << endl; > > + return TestFail; > > + } > > + > > + if (value.toString() != "42") { > > + cerr << "Control string mismatch after setting to uint32_t" << endl; > > + return TestFail; > > + } > > + > > + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; > > + value.set(Span<uint32_t>(uint32s)); > > + if (value.isNone() || !value.isArray() || > > + value.type() != ControlTypeUnsigned32) { > > + cerr << "Control type mismatch after setting to uint32_t array" << endl; > > + return TestFail; > > + } > > + > > + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); > > + if (uint32s.size() != uint32sResult.size() || > > + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { > > + cerr << "Control value mismatch after setting to uint32_t array" << endl; > > + return TestFail; > > + } > > + > > + if (value.toString() != "[ 3, 14, 15, 9 ]") { > > + cerr << "Control string mismatch after setting to uint32_t array" << endl; > > + return TestFail; > > + } > > + > > /* > > * Integer32 type. > > */ > > -- > > 2.47.0.163.g1226f6d8fa-goog > >
Hi Harvey On Fri, Oct 25, 2024 at 11:40:02PM +0800, Cheng-Hao Yang wrote: > Hi Jacopo, > > On Fri, Oct 25, 2024 at 5:15 PM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Harvey > > > > On Fri, Oct 25, 2024 at 05:30:13AM +0000, Harvey Yang wrote: > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > > > 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. > > > > V4L2_CTRL_TYPE_U32 is a control compound type. > > > > /* Compound types are >= 0x0100 */ > > V4L2_CTRL_COMPOUND_TYPES = 0x0100, > > V4L2_CTRL_TYPE_U8 = 0x0100, > > V4L2_CTRL_TYPE_U16 = 0x0101, > > V4L2_CTRL_TYPE_U32 = 0x0102, > > V4L2_CTRL_TYPE_AREA = 0x0106, > > > > This means it carries a payload and needs to be handled like > > V4L2_CTRL_TYPE_U8 is, specifcially when it comes to parsing it from > > the V4L2 interface into a libcamera control. > > > > I'm surprised you don't hit this in V4L2Device::getControls() > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > ControlType type; > > > > switch (info.type) { > > case V4L2_CTRL_TYPE_U8: > > type = ControlTypeByte; > > break; > > > > default: > > LOG(V4L2, Error) > > << "Unsupported payload control type " > > << info.type; > > return {}; > > } > > > > Thanks! Added for U32 and U16. Thank you. Any idea why LOG(V4L2, Error) << "Unsupported payload control type " << info.type; return {}; wasn't hit ? > > > > > > > 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 +++++++++++ > > > test/controls/control_value.cpp | 40 +++++++++++++++++++++++++++++++++ > > > 4 files changed, 70 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> > > > #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> > > > > Included by the header file > > Removed. > > > > > > > > > #include <libcamera/base/event_notifier.h> > > > #include <libcamera/base/log.h> > > > #include <libcamera/base/utils.h> > > > > > > +#include <libcamera/controls.h> > > > + > > > > This as well > > Removed. > > BR, > Harvey > > > > > > #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: > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > > index 344107fae..6ca85b739 100644 > > > --- a/test/controls/control_value.cpp > > > +++ b/test/controls/control_value.cpp > > > @@ -109,6 +109,46 @@ protected: > > > return TestFail; > > > } > > > > > > + /* > > > + * Unsigned Integer32 type. > > > + */ > > > + value.set(static_cast<uint32_t>(42)); > > > + if (value.isNone() || value.isArray() || > > > + value.type() != ControlTypeUnsigned32) { > > > + cerr << "Control type mismatch after setting to uint32_t" << endl; > > > + return TestFail; > > > + } > > > + > > > + if (value.get<uint32_t>() != 42) { > > > + cerr << "Control value mismatch after setting to uint32_t" << endl; > > > + return TestFail; > > > + } > > > + > > > + if (value.toString() != "42") { > > > + cerr << "Control string mismatch after setting to uint32_t" << endl; > > > + return TestFail; > > > + } > > > + > > > + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; > > > + value.set(Span<uint32_t>(uint32s)); > > > + if (value.isNone() || !value.isArray() || > > > + value.type() != ControlTypeUnsigned32) { > > > + cerr << "Control type mismatch after setting to uint32_t array" << endl; > > > + return TestFail; > > > + } > > > + > > > + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); > > > + if (uint32s.size() != uint32sResult.size() || > > > + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { > > > + cerr << "Control value mismatch after setting to uint32_t array" << endl; > > > + return TestFail; > > > + } > > > + > > > + if (value.toString() != "[ 3, 14, 15, 9 ]") { > > > + cerr << "Control string mismatch after setting to uint32_t array" << endl; > > > + return TestFail; > > > + } > > > + > > > /* > > > * Integer32 type. > > > */ > > > -- > > > 2.47.0.163.g1226f6d8fa-goog > > >
Hi Jacopo, On Sat, Oct 26, 2024 at 12:46 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey > > On Fri, Oct 25, 2024 at 11:40:02PM +0800, Cheng-Hao Yang wrote: > > Hi Jacopo, > > > > On Fri, Oct 25, 2024 at 5:15 PM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Harvey > > > > > > On Fri, Oct 25, 2024 at 05:30:13AM +0000, Harvey Yang wrote: > > > > From: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > > > > > 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. > > > > > > V4L2_CTRL_TYPE_U32 is a control compound type. > > > > > > /* Compound types are >= 0x0100 */ > > > V4L2_CTRL_COMPOUND_TYPES = 0x0100, > > > V4L2_CTRL_TYPE_U8 = 0x0100, > > > V4L2_CTRL_TYPE_U16 = 0x0101, > > > V4L2_CTRL_TYPE_U32 = 0x0102, > > > V4L2_CTRL_TYPE_AREA = 0x0106, > > > > > > This means it carries a payload and needs to be handled like > > > V4L2_CTRL_TYPE_U8 is, specifcially when it comes to parsing it from > > > the V4L2 interface into a libcamera control. > > > > > > I'm surprised you don't hit this in V4L2Device::getControls() > > > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > > ControlType type; > > > > > > switch (info.type) { > > > case V4L2_CTRL_TYPE_U8: > > > type = ControlTypeByte; > > > break; > > > > > > default: > > > LOG(V4L2, Error) > > > << "Unsupported payload control type " > > > << info.type; > > > return {}; > > > } > > > > > > > Thanks! Added for U32 and U16. > > Thank you. > > Any idea why > > LOG(V4L2, Error) > << "Unsupported payload control type " > << info.type; > return {}; > > wasn't hit ? I think the original purpose of adding it is not for getControls/setControls: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5669226/1//COMMIT_MSG#12 BR, Harvey > > > > > > > > > > > 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 +++++++++++ > > > > test/controls/control_value.cpp | 40 +++++++++++++++++++++++++++++++++ > > > > 4 files changed, 70 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> > > > > #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> > > > > > > Included by the header file > > > > Removed. > > > > > > > > > > > > > #include <libcamera/base/event_notifier.h> > > > > #include <libcamera/base/log.h> > > > > #include <libcamera/base/utils.h> > > > > > > > > +#include <libcamera/controls.h> > > > > + > > > > > > This as well > > > > Removed. > > > > BR, > > Harvey > > > > > > > > > #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: > > > > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp > > > > index 344107fae..6ca85b739 100644 > > > > --- a/test/controls/control_value.cpp > > > > +++ b/test/controls/control_value.cpp > > > > @@ -109,6 +109,46 @@ protected: > > > > return TestFail; > > > > } > > > > > > > > + /* > > > > + * Unsigned Integer32 type. > > > > + */ > > > > + value.set(static_cast<uint32_t>(42)); > > > > + if (value.isNone() || value.isArray() || > > > > + value.type() != ControlTypeUnsigned32) { > > > > + cerr << "Control type mismatch after setting to uint32_t" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > + if (value.get<uint32_t>() != 42) { > > > > + cerr << "Control value mismatch after setting to uint32_t" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > + if (value.toString() != "42") { > > > > + cerr << "Control string mismatch after setting to uint32_t" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; > > > > + value.set(Span<uint32_t>(uint32s)); > > > > + if (value.isNone() || !value.isArray() || > > > > + value.type() != ControlTypeUnsigned32) { > > > > + cerr << "Control type mismatch after setting to uint32_t array" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); > > > > + if (uint32s.size() != uint32sResult.size() || > > > > + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { > > > > + cerr << "Control value mismatch after setting to uint32_t array" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > + if (value.toString() != "[ 3, 14, 15, 9 ]") { > > > > + cerr << "Control string mismatch after setting to uint32_t array" << endl; > > > > + return TestFail; > > > > + } > > > > + > > > > /* > > > > * Integer32 type. > > > > */ > > > > -- > > > > 2.47.0.163.g1226f6d8fa-goog > > > >
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: diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp index 344107fae..6ca85b739 100644 --- a/test/controls/control_value.cpp +++ b/test/controls/control_value.cpp @@ -109,6 +109,46 @@ protected: return TestFail; } + /* + * Unsigned Integer32 type. + */ + value.set(static_cast<uint32_t>(42)); + if (value.isNone() || value.isArray() || + value.type() != ControlTypeUnsigned32) { + cerr << "Control type mismatch after setting to uint32_t" << endl; + return TestFail; + } + + if (value.get<uint32_t>() != 42) { + cerr << "Control value mismatch after setting to uint32_t" << endl; + return TestFail; + } + + if (value.toString() != "42") { + cerr << "Control string mismatch after setting to uint32_t" << endl; + return TestFail; + } + + std::array<uint32_t, 4> uint32s{ 3, 14, 15, 9 }; + value.set(Span<uint32_t>(uint32s)); + if (value.isNone() || !value.isArray() || + value.type() != ControlTypeUnsigned32) { + cerr << "Control type mismatch after setting to uint32_t array" << endl; + return TestFail; + } + + Span<const uint32_t> uint32sResult = value.get<Span<const uint32_t>>(); + if (uint32s.size() != uint32sResult.size() || + !std::equal(uint32s.begin(), uint32s.end(), uint32sResult.begin())) { + cerr << "Control value mismatch after setting to uint32_t array" << endl; + return TestFail; + } + + if (value.toString() != "[ 3, 14, 15, 9 ]") { + cerr << "Control string mismatch after setting to uint32_t array" << endl; + return TestFail; + } + /* * Integer32 type. */