[PATCH v4 1/2] libcamera: Extend u32 control type
Cheng-Hao Yang
chenghaoyang at chromium.org
Tue Oct 29 17:50:08 CET 2024
Hi Laurent,
On Wed, Oct 30, 2024 at 12:14 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Harvey,
>
> On Wed, Oct 30, 2024 at 12:11:12AM +0800, Cheng-Hao Yang wrote:
> > On Tue, Oct 29, 2024 at 6:33 PM Jacopo Mondi wrote:
> > > On Tue, Oct 29, 2024 at 06:08:44PM +0800, Cheng-Hao Yang wrote:
> > > > On Mon, Oct 28, 2024 at 6:36 PM Jacopo Mondi wrote:
> > > > > On Fri, Oct 25, 2024 at 03:38:50PM +0000, Harvey Yang wrote:
> > > > > > From: Yudhistira Erlandinata <yerlandinata at 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.
> > > > > >
> > > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > > > > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > > > ---
> > > > > > include/libcamera/controls.h | 7 ++++++
> > > > > > src/libcamera/controls.cpp | 12 ++++++++--
> > > > > > src/libcamera/v4l2_device.cpp | 14 ++++++++++++
> > > > > > test/controls/control_value.cpp | 40 +++++++++++++++++++++++++++++++++
> > > > > > 4 files changed, 71 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..f39ec3760 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>
> > > > > > @@ -210,6 +211,10 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
> > > > > > type = ControlTypeByte;
> > > > > > break;
> > > > > >
> > > > > > + case V4L2_CTRL_TYPE_U32:
> > > > > > + type = ControlTypeUnsigned32;
> > > > > > + break;
> > > > > > +
> > > > >
> > > > > This is not enough.
> > > > >
> > > > > After the switch we have
> > > > >
> > > > > ControlValue &value = ctrl.second;
> > > > > value.reserve(type, true, info.elems);
> > > > > Span<uint8_t> data = value.data();
> > > > >
> > > > > v4l2Ctrl.p_u8 = data.data();
> > > > > v4l2Ctrl.size = data.size();
> > > > >
> > > > > However controls of type V4L2_CTRL_TYPE_U32 use the p_u32 union member
> > > > > of 'struct v4l2_ext_control'
> > > > > https://www.kernel.org/doc/html/v4.9/media/uapi/v4l/vidioc-g-ext-ctrls.html#id1
> > > > >
> > > > > Same for U16 control types.
> > > > >
> > > > > Be careful about one thing. v4l2Ctrl.p_u32 is of type __u32* so it
> > > > > would feel natural to
> > > > >
> > > > > Span<uint32_t> data = value.data();
> > > > > v4l2Ctrl.p_u32 = data.data();
> > > > >
> > > > > as data.data() returns a T*
> > > > >
> > > > > However v4l2Ctrl.size wants the size in bytes while data.size()
> > > > > returns the number of elements, so you should probably use
> > > > > data.size_bytes().
> > > > >
> > > > > Or you can do as we do in setControls and cast the data.data()
> > > > > pointer.
> > > > >
> > > > > Span<uint8_t> data = value.data();
> > > > > v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data());
> > > > > v4l2Ctrl.size = data.size();
> > > > >
> > > > > Also, you need to handle U32 and U16 also setControl().
> > > >
> > > > Thanks for the catch!
> > > > Please help take a look at my WIP patch:
> > > > https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/commit/faec47f605881062e58718371183a0c2d3ffe772
> > > > , before I upload another version.
> > > > Let me know if you think I should merge the two `switch`es and duplicate
> > > > the code in the middle.
> > >
> > >
> > > You would have to copy
> > >
> > > value.reserve(type, true, info.elems);
> > > Span<uint8_t> data = value.data();
> > >
> > > In the two switch branches but you can keep
> > >
> > > v4l2Ctrl.size = data.size();
> > >
> > > After the switch.
> >
> > Done.
> >
> > > Up to you, I would prefer a single switch but I don't mind too much
> > >
> > > > > If you can test this by reading and writing u16 and u32 controls this
> > > > > would make me way more confident this patch is correct.
> > > >
> > > > Hmm, I couldn't find such a use case. Can you suggest how we could
> > > > test it?
> > >
> > > Well, I presume you have a device with a u16/u32 control if you sent
> > > this :) Just calling getControls(), trying to set them to some value
> > > and reading them back would validate the implementation. Or have I
> > > missed your question ?
> >
> > Actually, the two new types are not being used in setControls() / getControls().
> > u32, for example, is only used for listControls with VIDIOC_QUERY_EXT_CTRL [1],
> > and V4L2Device needs to recognize u32.
>
> But surely that's used with a device ? Can you point to the driver that
> exposes these controls ?
Yes, the failure happens in Aie [1]
, and `sourceVideo_` should be "mtk-aie-5.3-source" [2].
Let me know if I missed anything.
BR,
Harvey
[1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/face_detect/aie.cpp;l=156
[2]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/face_detect/aie.cpp;l=121
>
> > That's also why I didn't have the changes in setControls() / getControls().
> >
> > BR,
> > Harvey
> >
> > [1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/v4l2_device.cpp;l=681
> >
> > > > > > default:
> > > > > > LOG(V4L2, Error)
> > > > > > << "Unsupported payload control type "
> > > > > > @@ -488,6 +493,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 +544,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 +635,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.
> > > > > > */
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list