[libcamera-devel,v8,2/6] libcamera: V4L2Device: Support v4l2 menu control
diff mbox series

Message ID 20210610082539.529739-2-hiroh@chromium.org
State Accepted
Headers show
Series
  • [libcamera-devel,v8,1/6] libcamera: controls: Add sensor test pattern mode
Related show

Commit Message

Hirokazu Honda June 10, 2021, 8:25 a.m. UTC 
This adds a support of v4l2 menu. The control info for v4l2 menu
contains indices without names and 64bit values of querymenu.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/v4l2_device.h |   5 +
 src/libcamera/v4l2_device.cpp            | 186 +++++++++++++++--------
 2 files changed, 127 insertions(+), 64 deletions(-)

Comments

Jacopo Mondi June 10, 2021, 4:05 p.m. UTC | #1 
Hi Hiro,

On Thu, Jun 10, 2021 at 05:25:35PM +0900, Hirokazu Honda wrote:
> This adds a support of v4l2 menu. The control info for v4l2 menu
> contains indices without names and 64bit values of querymenu.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_device.h |   5 +
>  src/libcamera/v4l2_device.cpp            | 186 +++++++++++++++--------
>  2 files changed, 127 insertions(+), 64 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index b82e2a14..c318e976 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -56,6 +56,11 @@ protected:
>  	int fd() const { return fd_; }
>
>  private:
> +	static ControlType v4l2CtrlType(uint32_t ctrlType);
> +	static std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl);
> +	ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);
> +	ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);
> +
>  	void listControls();
>  	void updateControls(ControlList *ctrls,
>  			    Span<const v4l2_ext_control> v4l2Ctrls);
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index b39c6266..cda3769a 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -32,69 +32,6 @@ LOG_DEFINE_CATEGORY(V4L2)
>
>  namespace {
>
> -ControlType v4l2CtrlType(uint32_t ctrlType)
> -{
> -	switch (ctrlType) {
> -	case V4L2_CTRL_TYPE_U8:
> -		return ControlTypeByte;
> -
> -	case V4L2_CTRL_TYPE_BOOLEAN:
> -		return ControlTypeBool;
> -
> -	case V4L2_CTRL_TYPE_INTEGER:
> -		return ControlTypeInteger32;
> -
> -	case V4L2_CTRL_TYPE_INTEGER64:
> -		return ControlTypeInteger64;
> -
> -	case V4L2_CTRL_TYPE_MENU:
> -	case V4L2_CTRL_TYPE_BUTTON:
> -	case V4L2_CTRL_TYPE_BITMASK:
> -	case V4L2_CTRL_TYPE_INTEGER_MENU:
> -		/*
> -		 * More precise types may be needed, for now use a 32-bit
> -		 * integer type.
> -		 */
> -		return ControlTypeInteger32;
> -
> -	default:
> -		return ControlTypeNone;
> -	}
> -}
> -
> -std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl)
> -{
> -	const size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
> -	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, type);
> -}
> -
> -ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
> -{
> -	switch (ctrl.type) {
> -	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));
> -
> -	case V4L2_CTRL_TYPE_BOOLEAN:
> -		return ControlInfo(static_cast<bool>(ctrl.minimum),
> -				   static_cast<bool>(ctrl.maximum),
> -				   static_cast<bool>(ctrl.default_value));
> -
> -	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));
> -
> -	default:
> -		return ControlInfo(static_cast<int32_t>(ctrl.minimum),
> -				   static_cast<int32_t>(ctrl.maximum),
> -				   static_cast<int32_t>(ctrl.default_value));
> -	}
> -}
>   } /* namespace */

Thanks for addressing my request, but now we have an empty namespace
which could be dropped.

I have a few more picky comments on the newly added documentation.
With your ack they can be applied when merging.

>
>  /**
> @@ -524,6 +461,121 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
>   * \return The V4L2 device file descriptor, -1 if the device node is not open
>   */
>
> +/**
> + * \brief Retrieve ControlType for V4L2 control type

Retrieve the libcamera control type associated with the V4L2 control type

> + * \param[in] ctrlType V4L2 control type
> + *
> + * \return ControlType for \a ctrlType

The ControlType corresponding to \a ctrlType

> + */
> +ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
> +{
> +	switch (ctrlType) {
> +	case V4L2_CTRL_TYPE_U8:
> +		return ControlTypeByte;
> +
> +	case V4L2_CTRL_TYPE_BOOLEAN:
> +		return ControlTypeBool;
> +
> +	case V4L2_CTRL_TYPE_INTEGER:
> +		return ControlTypeInteger32;
> +
> +	case V4L2_CTRL_TYPE_INTEGER64:
> +		return ControlTypeInteger64;
> +
> +	case V4L2_CTRL_TYPE_MENU:
> +	case V4L2_CTRL_TYPE_BUTTON:
> +	case V4L2_CTRL_TYPE_BITMASK:
> +	case V4L2_CTRL_TYPE_INTEGER_MENU:
> +		/*
> +		 * More precise types may be needed, for now use a 32-bit
> +		 * integer type.
> +		 */
> +		return ControlTypeInteger32;
> +
> +	default:
> +		return ControlTypeNone;
> +	}
> +}
> +
> +/**
> + * \brief Create ControlId for v4l2_query_ext_ctrl

\brief Create a ControlId for a V4L2 control

> + * \param[in] ctrl v4l2_query_ext_ctrl

Your below description sound better:

\param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control

> + *
> + * \return ControlId for \a ctrl
> + */
> +std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &ctrl)
> +{
> +	const size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
> +	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, type);
> +}
> +
> +/**
> + * \brief Create ControlInfo from v4l2_query_ext_ctrl
> + * \param[in] ctrl v4l2_query_ext_ctrl

Your below description sound better:

\param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control


> + *
> + * \return ControlInfo from \a v4l2_query_ext_ctrl

A ControlInfo created from \a ctrl

> + */
> +ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
> +{
> +	switch (ctrl.type) {
> +	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));
> +
> +	case V4L2_CTRL_TYPE_BOOLEAN:
> +		return ControlInfo(static_cast<bool>(ctrl.minimum),
> +				   static_cast<bool>(ctrl.maximum),
> +				   static_cast<bool>(ctrl.default_value));
> +
> +	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));
> +
> +	case V4L2_CTRL_TYPE_INTEGER_MENU:
> +	case V4L2_CTRL_TYPE_MENU:
> +		return v4l2MenuControlInfo(ctrl);
> +
> +	default:
> +		return ControlInfo(static_cast<int32_t>(ctrl.minimum),
> +				   static_cast<int32_t>(ctrl.maximum),
> +				   static_cast<int32_t>(ctrl.default_value));
> +	}
> +}
> +
> +/**
> + * \brief Create ControlInfo for a V4L2 menu control
> + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu

.. a V4L2 menu control

> + *
> + * The created ControlInfo contains indices acquired by VIDIOC_QUERYMENU.
> + *
> + * \return ControlInfo for a V4L2 menu control

I would apply the same suggestion given above

> + */
> +ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> +{
> +	std::vector<ControlValue> indices;
> +	struct v4l2_querymenu menu = {};
> +	menu.id = ctrl.id;
> +
> +	if (ctrl.minimum < 0)
> +		return ControlInfo();
> +
> +	for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) {
> +		menu.index = index;
> +		if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
> +			continue;
> +
> +		indices.push_back(index);
> +	}
> +
> +	return ControlInfo(indices,
> +			   ControlValue(static_cast<int32_t>(ctrl.default_value)));
> +}
> +
>  /*
>   * \brief List and store information about all controls supported by the
>   * V4L2 device
> @@ -533,7 +585,6 @@ void V4L2Device::listControls()
>  	ControlInfoMap::Map ctrls;
>  	struct v4l2_query_ext_ctrl ctrl = {};
>
> -	/* \todo Add support for menu controls. */
>  	while (1) {
>  		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL |
>  			   V4L2_CTRL_FLAG_NEXT_COMPOUND;
> @@ -562,6 +613,9 @@ void V4L2Device::listControls()
>  			continue;
>  		}
>
> +		LOG(V4L2, Debug) << "Control: " << ctrl.name
> +				 << " (" << utils::hex(ctrl.id) << ")";
> +
>  		controlIds_.emplace_back(v4l2ControlId(ctrl));
>  		controlInfo_.emplace(ctrl.id, ctrl);
>
> @@ -630,6 +684,10 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  			value.set<int64_t>(v4l2Ctrl.value64);
>  			break;
>
> +		case ControlTypeInteger32:
> +			value.set<int32_t>(v4l2Ctrl.value);
> +			break;
> +
>  		case ControlTypeByte:
>  			/*
>  			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
> --
> 2.32.0.rc1.229.g3e70b5a671-goog
>
Hirokazu Honda June 11, 2021, 3:54 a.m. UTC | #2 
Hi Jacopo,

On Fri, Jun 11, 2021 at 1:04 AM Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Hiro,
>
> On Thu, Jun 10, 2021 at 05:25:35PM +0900, Hirokazu Honda wrote:
> > This adds a support of v4l2 menu. The control info for v4l2 menu
> > contains indices without names and 64bit values of querymenu.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/internal/v4l2_device.h |   5 +
> >  src/libcamera/v4l2_device.cpp            | 186 +++++++++++++++--------
> >  2 files changed, 127 insertions(+), 64 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_device.h
> b/include/libcamera/internal/v4l2_device.h
> > index b82e2a14..c318e976 100644
> > --- a/include/libcamera/internal/v4l2_device.h
> > +++ b/include/libcamera/internal/v4l2_device.h
> > @@ -56,6 +56,11 @@ protected:
> >       int fd() const { return fd_; }
> >
> >  private:
> > +     static ControlType v4l2CtrlType(uint32_t ctrlType);
> > +     static std::unique_ptr<ControlId> v4l2ControlId(const
> v4l2_query_ext_ctrl &ctrl);
> > +     ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);
> > +     ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);
> > +
> >       void listControls();
> >       void updateControls(ControlList *ctrls,
> >                           Span<const v4l2_ext_control> v4l2Ctrls);
> > diff --git a/src/libcamera/v4l2_device.cpp
> b/src/libcamera/v4l2_device.cpp
> > index b39c6266..cda3769a 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -32,69 +32,6 @@ LOG_DEFINE_CATEGORY(V4L2)
> >
> >  namespace {
> >
> > -ControlType v4l2CtrlType(uint32_t ctrlType)
> > -{
> > -     switch (ctrlType) {
> > -     case V4L2_CTRL_TYPE_U8:
> > -             return ControlTypeByte;
> > -
> > -     case V4L2_CTRL_TYPE_BOOLEAN:
> > -             return ControlTypeBool;
> > -
> > -     case V4L2_CTRL_TYPE_INTEGER:
> > -             return ControlTypeInteger32;
> > -
> > -     case V4L2_CTRL_TYPE_INTEGER64:
> > -             return ControlTypeInteger64;
> > -
> > -     case V4L2_CTRL_TYPE_MENU:
> > -     case V4L2_CTRL_TYPE_BUTTON:
> > -     case V4L2_CTRL_TYPE_BITMASK:
> > -     case V4L2_CTRL_TYPE_INTEGER_MENU:
> > -             /*
> > -              * More precise types may be needed, for now use a 32-bit
> > -              * integer type.
> > -              */
> > -             return ControlTypeInteger32;
> > -
> > -     default:
> > -             return ControlTypeNone;
> > -     }
> > -}
> > -
> > -std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl
> &ctrl)
> > -{
> > -     const size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
> > -     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, type);
> > -}
> > -
> > -ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
> > -{
> > -     switch (ctrl.type) {
> > -     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));
> > -
> > -     case V4L2_CTRL_TYPE_BOOLEAN:
> > -             return ControlInfo(static_cast<bool>(ctrl.minimum),
> > -                                static_cast<bool>(ctrl.maximum),
> > -                                static_cast<bool>(ctrl.default_value));
> > -
> > -     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));
> > -
> > -     default:
> > -             return ControlInfo(static_cast<int32_t>(ctrl.minimum),
> > -                                static_cast<int32_t>(ctrl.maximum),
> > -
> static_cast<int32_t>(ctrl.default_value));
> > -     }
> > -}
> >   } /* namespace */
>
> Thanks for addressing my request, but now we have an empty namespace
> which could be dropped.
>
> I have a few more picky comments on the newly added documentation.
> With your ack they can be applied when merging.
>
> >
> >  /**
> > @@ -524,6 +461,121 @@ int V4L2Device::ioctl(unsigned long request, void
> *argp)
> >   * \return The V4L2 device file descriptor, -1 if the device node is
> not open
> >   */
> >
> > +/**
> > + * \brief Retrieve ControlType for V4L2 control type
>
> Retrieve the libcamera control type associated with the V4L2 control type
>
> > + * \param[in] ctrlType V4L2 control type
> > + *
> > + * \return ControlType for \a ctrlType
>
> The ControlType corresponding to \a ctrlType
>
> > + */
> > +ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
> > +{
> > +     switch (ctrlType) {
> > +     case V4L2_CTRL_TYPE_U8:
> > +             return ControlTypeByte;
> > +
> > +     case V4L2_CTRL_TYPE_BOOLEAN:
> > +             return ControlTypeBool;
> > +
> > +     case V4L2_CTRL_TYPE_INTEGER:
> > +             return ControlTypeInteger32;
> > +
> > +     case V4L2_CTRL_TYPE_INTEGER64:
> > +             return ControlTypeInteger64;
> > +
> > +     case V4L2_CTRL_TYPE_MENU:
> > +     case V4L2_CTRL_TYPE_BUTTON:
> > +     case V4L2_CTRL_TYPE_BITMASK:
> > +     case V4L2_CTRL_TYPE_INTEGER_MENU:
> > +             /*
> > +              * More precise types may be needed, for now use a 32-bit
> > +              * integer type.
> > +              */
> > +             return ControlTypeInteger32;
> > +
> > +     default:
> > +             return ControlTypeNone;
> > +     }
> > +}
> > +
> > +/**
> > + * \brief Create ControlId for v4l2_query_ext_ctrl
>
> \brief Create a ControlId for a V4L2 control
>
> > + * \param[in] ctrl v4l2_query_ext_ctrl
>
> Your below description sound better:
>
> \param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control
>
> > + *
> > + * \return ControlId for \a ctrl
> > + */
> > +std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const
> v4l2_query_ext_ctrl &ctrl)
> > +{
> > +     const size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
> > +     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, type);
> > +}
> > +
> > +/**
> > + * \brief Create ControlInfo from v4l2_query_ext_ctrl
> > + * \param[in] ctrl v4l2_query_ext_ctrl
>
> Your below description sound better:
>
> \param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control
>
>
> > + *
> > + * \return ControlInfo from \a v4l2_query_ext_ctrl
>
> A ControlInfo created from \a ctrl
>
> > + */
> > +ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
> > +{
> > +     switch (ctrl.type) {
> > +     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));
> > +
> > +     case V4L2_CTRL_TYPE_BOOLEAN:
> > +             return ControlInfo(static_cast<bool>(ctrl.minimum),
> > +                                static_cast<bool>(ctrl.maximum),
> > +                                static_cast<bool>(ctrl.default_value));
> > +
> > +     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));
> > +
> > +     case V4L2_CTRL_TYPE_INTEGER_MENU:
> > +     case V4L2_CTRL_TYPE_MENU:
> > +             return v4l2MenuControlInfo(ctrl);
> > +
> > +     default:
> > +             return ControlInfo(static_cast<int32_t>(ctrl.minimum),
> > +                                static_cast<int32_t>(ctrl.maximum),
> > +
> static_cast<int32_t>(ctrl.default_value));
> > +     }
> > +}
> > +
> > +/**
> > + * \brief Create ControlInfo for a V4L2 menu control
> > + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
>
> .. a V4L2 menu control
>
> > + *
> > + * The created ControlInfo contains indices acquired by
> VIDIOC_QUERYMENU.
> > + *
> > + * \return ControlInfo for a V4L2 menu control
>
> I would apply the same suggestion given above
>
>
All of them sound good to me.
Thanks.

-Hiro


> > + */
> > +ControlInfo V4L2Device::v4l2MenuControlInfo(const struct
> v4l2_query_ext_ctrl &ctrl)
> > +{
> > +     std::vector<ControlValue> indices;
> > +     struct v4l2_querymenu menu = {};
> > +     menu.id = ctrl.id;
> > +
> > +     if (ctrl.minimum < 0)
> > +             return ControlInfo();
> > +
> > +     for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index)
> {
> > +             menu.index = index;
> > +             if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
> > +                     continue;
> > +
> > +             indices.push_back(index);
> > +     }
> > +
> > +     return ControlInfo(indices,
> > +
> ControlValue(static_cast<int32_t>(ctrl.default_value)));
> > +}
> > +
> >  /*
> >   * \brief List and store information about all controls supported by the
> >   * V4L2 device
> > @@ -533,7 +585,6 @@ void V4L2Device::listControls()
> >       ControlInfoMap::Map ctrls;
> >       struct v4l2_query_ext_ctrl ctrl = {};
> >
> > -     /* \todo Add support for menu controls. */
> >       while (1) {
> >               ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL |
> >                          V4L2_CTRL_FLAG_NEXT_COMPOUND;
> > @@ -562,6 +613,9 @@ void V4L2Device::listControls()
> >                       continue;
> >               }
> >
> > +             LOG(V4L2, Debug) << "Control: " << ctrl.name
> > +                              << " (" << utils::hex(ctrl.id) << ")";
> > +
> >               controlIds_.emplace_back(v4l2ControlId(ctrl));
> >               controlInfo_.emplace(ctrl.id, ctrl);
> >
> > @@ -630,6 +684,10 @@ void V4L2Device::updateControls(ControlList *ctrls,
> >                       value.set<int64_t>(v4l2Ctrl.value64);
> >                       break;
> >
> > +             case ControlTypeInteger32:
> > +                     value.set<int32_t>(v4l2Ctrl.value);
> > +                     break;
> > +
> >               case ControlTypeByte:
> >                       /*
> >                        * No action required, the VIDIOC_[GS]_EXT_CTRLS
> ioctl
> > --
> > 2.32.0.rc1.229.g3e70b5a671-goog
> >
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
index b82e2a14..c318e976 100644
--- a/include/libcamera/internal/v4l2_device.h
+++ b/include/libcamera/internal/v4l2_device.h
@@ -56,6 +56,11 @@  protected:
 	int fd() const { return fd_; }
 
 private:
+	static ControlType v4l2CtrlType(uint32_t ctrlType);
+	static std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl);
+	ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);
+	ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);
+
 	void listControls();
 	void updateControls(ControlList *ctrls,
 			    Span<const v4l2_ext_control> v4l2Ctrls);
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index b39c6266..cda3769a 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -32,69 +32,6 @@  LOG_DEFINE_CATEGORY(V4L2)
 
 namespace {
 
-ControlType v4l2CtrlType(uint32_t ctrlType)
-{
-	switch (ctrlType) {
-	case V4L2_CTRL_TYPE_U8:
-		return ControlTypeByte;
-
-	case V4L2_CTRL_TYPE_BOOLEAN:
-		return ControlTypeBool;
-
-	case V4L2_CTRL_TYPE_INTEGER:
-		return ControlTypeInteger32;
-
-	case V4L2_CTRL_TYPE_INTEGER64:
-		return ControlTypeInteger64;
-
-	case V4L2_CTRL_TYPE_MENU:
-	case V4L2_CTRL_TYPE_BUTTON:
-	case V4L2_CTRL_TYPE_BITMASK:
-	case V4L2_CTRL_TYPE_INTEGER_MENU:
-		/*
-		 * More precise types may be needed, for now use a 32-bit
-		 * integer type.
-		 */
-		return ControlTypeInteger32;
-
-	default:
-		return ControlTypeNone;
-	}
-}
-
-std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl)
-{
-	const size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
-	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, type);
-}
-
-ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
-{
-	switch (ctrl.type) {
-	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));
-
-	case V4L2_CTRL_TYPE_BOOLEAN:
-		return ControlInfo(static_cast<bool>(ctrl.minimum),
-				   static_cast<bool>(ctrl.maximum),
-				   static_cast<bool>(ctrl.default_value));
-
-	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));
-
-	default:
-		return ControlInfo(static_cast<int32_t>(ctrl.minimum),
-				   static_cast<int32_t>(ctrl.maximum),
-				   static_cast<int32_t>(ctrl.default_value));
-	}
-}
 } /* namespace */
 
 /**
@@ -524,6 +461,121 @@  int V4L2Device::ioctl(unsigned long request, void *argp)
  * \return The V4L2 device file descriptor, -1 if the device node is not open
  */
 
+/**
+ * \brief Retrieve ControlType for V4L2 control type
+ * \param[in] ctrlType V4L2 control type
+ *
+ * \return ControlType for \a ctrlType
+ */
+ControlType V4L2Device::v4l2CtrlType(uint32_t ctrlType)
+{
+	switch (ctrlType) {
+	case V4L2_CTRL_TYPE_U8:
+		return ControlTypeByte;
+
+	case V4L2_CTRL_TYPE_BOOLEAN:
+		return ControlTypeBool;
+
+	case V4L2_CTRL_TYPE_INTEGER:
+		return ControlTypeInteger32;
+
+	case V4L2_CTRL_TYPE_INTEGER64:
+		return ControlTypeInteger64;
+
+	case V4L2_CTRL_TYPE_MENU:
+	case V4L2_CTRL_TYPE_BUTTON:
+	case V4L2_CTRL_TYPE_BITMASK:
+	case V4L2_CTRL_TYPE_INTEGER_MENU:
+		/*
+		 * More precise types may be needed, for now use a 32-bit
+		 * integer type.
+		 */
+		return ControlTypeInteger32;
+
+	default:
+		return ControlTypeNone;
+	}
+}
+
+/**
+ * \brief Create ControlId for v4l2_query_ext_ctrl
+ * \param[in] ctrl v4l2_query_ext_ctrl
+ *
+ * \return ControlId for \a ctrl
+ */
+std::unique_ptr<ControlId> V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &ctrl)
+{
+	const size_t len = strnlen(ctrl.name, sizeof(ctrl.name));
+	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, type);
+}
+
+/**
+ * \brief Create ControlInfo from v4l2_query_ext_ctrl
+ * \param[in] ctrl v4l2_query_ext_ctrl
+ *
+ * \return ControlInfo from \a v4l2_query_ext_ctrl
+ */
+ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)
+{
+	switch (ctrl.type) {
+	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));
+
+	case V4L2_CTRL_TYPE_BOOLEAN:
+		return ControlInfo(static_cast<bool>(ctrl.minimum),
+				   static_cast<bool>(ctrl.maximum),
+				   static_cast<bool>(ctrl.default_value));
+
+	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));
+
+	case V4L2_CTRL_TYPE_INTEGER_MENU:
+	case V4L2_CTRL_TYPE_MENU:
+		return v4l2MenuControlInfo(ctrl);
+
+	default:
+		return ControlInfo(static_cast<int32_t>(ctrl.minimum),
+				   static_cast<int32_t>(ctrl.maximum),
+				   static_cast<int32_t>(ctrl.default_value));
+	}
+}
+
+/**
+ * \brief Create ControlInfo for a V4L2 menu control
+ * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
+ *
+ * The created ControlInfo contains indices acquired by VIDIOC_QUERYMENU.
+ *
+ * \return ControlInfo for a V4L2 menu control
+ */
+ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
+{
+	std::vector<ControlValue> indices;
+	struct v4l2_querymenu menu = {};
+	menu.id = ctrl.id;
+
+	if (ctrl.minimum < 0)
+		return ControlInfo();
+
+	for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) {
+		menu.index = index;
+		if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
+			continue;
+
+		indices.push_back(index);
+	}
+
+	return ControlInfo(indices,
+			   ControlValue(static_cast<int32_t>(ctrl.default_value)));
+}
+
 /*
  * \brief List and store information about all controls supported by the
  * V4L2 device
@@ -533,7 +585,6 @@  void V4L2Device::listControls()
 	ControlInfoMap::Map ctrls;
 	struct v4l2_query_ext_ctrl ctrl = {};
 
-	/* \todo Add support for menu controls. */
 	while (1) {
 		ctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL |
 			   V4L2_CTRL_FLAG_NEXT_COMPOUND;
@@ -562,6 +613,9 @@  void V4L2Device::listControls()
 			continue;
 		}
 
+		LOG(V4L2, Debug) << "Control: " << ctrl.name
+				 << " (" << utils::hex(ctrl.id) << ")";
+
 		controlIds_.emplace_back(v4l2ControlId(ctrl));
 		controlInfo_.emplace(ctrl.id, ctrl);
 
@@ -630,6 +684,10 @@  void V4L2Device::updateControls(ControlList *ctrls,
 			value.set<int64_t>(v4l2Ctrl.value64);
 			break;
 
+		case ControlTypeInteger32:
+			value.set<int32_t>(v4l2Ctrl.value);
+			break;
+
 		case ControlTypeByte:
 			/*
 			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl