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

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

Commit Message

Hirokazu Honda May 28, 2021, 3:05 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>
---
 include/libcamera/internal/v4l2_device.h |  2 +
 src/libcamera/v4l2_device.cpp            | 47 ++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart June 6, 2021, 11:22 p.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Fri, May 28, 2021 at 12:05:27PM +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>
> ---
>  include/libcamera/internal/v4l2_device.h |  2 +
>  src/libcamera/v4l2_device.cpp            | 47 ++++++++++++++++++++++--
>  2 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index b82e2a14..687be9b2 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -56,6 +56,8 @@ protected:
>  	int fd() const { return fd_; }
>  
>  private:
> +	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 caafbc2d..032252bb 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -530,6 +530,33 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
>   * \return The V4L2 device file descriptor, -1 if the device node is not open
>   */
>  
> +/**
> + * \brief Create ControlInfo for v4l2 menu ctrl

s/for v4l2 menu ctrl/for a V4L2 menu control/

> + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
> + * \param[out] ctrlInfo The created controlInfo

It's not an out parameter, it's the return value.

> + *
> + * The created ControlInfo contains indices acquired by VIDIOC_QUERYMENU.
> + *
> + * \return ControlInfo for v4l2 menu ctrl.
> + */
> +ControlInfo V4L2Device::v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl)
> +{
> +	std::vector<ControlValue> indices;
> +	v4l2_querymenu menu;

We usually prefix the V4L2 structures with struct. While not strictly
mandatory, it emphasizes that there are C structs from V4L2.

> +	memset(&menu, 0, sizeof(menu));

You could also write this

	struct v4l2_querymenu menu = { };

(I really wish designated initializers were available before C++20 :-().

> +	menu.id = ctrl.id;

As a sanity check, should we add

	if (ctrl.minimum <= 0)
		return ControlInfo();

?

> +
> +	for (menu.index = ctrl.minimum;
> +	     static_cast<int32_t>(menu.index) <= ctrl.maximum; menu.index++) {

	for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) }
		menu.index = index;

would allow dropping the cast. Up to you.

> +		if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
> +			continue;
> +
> +		indices.emplace_back(static_cast<int32_t>(menu.index));

This could be

		indices.emplace_back(index);

or even

		indices.push_back(index);

as it won't make much of a difference.

> +	}
> +
> +	return ControlInfo(indices);

Would it be useful to also pass the default value to the ControlInfo
constructor ?

> +}
> +
>  /*
>   * \brief List and store information about all controls supported by the
>   * V4L2 device
> @@ -539,7 +566,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;
> @@ -550,16 +576,22 @@ void V4L2Device::listControls()
>  		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
>  			continue;
>  
> +		ControlInfo ctrlInfo;

I'd add a blank line here.

>  		switch (ctrl.type) {
>  		case V4L2_CTRL_TYPE_INTEGER:
>  		case V4L2_CTRL_TYPE_BOOLEAN:
> -		case V4L2_CTRL_TYPE_MENU:
>  		case V4L2_CTRL_TYPE_BUTTON:
>  		case V4L2_CTRL_TYPE_INTEGER64:
>  		case V4L2_CTRL_TYPE_BITMASK:
> -		case V4L2_CTRL_TYPE_INTEGER_MENU:
>  		case V4L2_CTRL_TYPE_U8:
> +			ctrlInfo = v4l2ControlInfo(ctrl);
> +			break;
> +
> +		case V4L2_CTRL_TYPE_INTEGER_MENU:
> +		case V4L2_CTRL_TYPE_MENU:
> +			ctrlInfo = v4l2MenuControlInfo(ctrl);
>  			break;
> +
>  		/* \todo Support other control types. */
>  		default:
>  			LOG(V4L2, Debug)
> @@ -568,10 +600,13 @@ void V4L2Device::listControls()
>  			continue;
>  		}
>  
> +		LOG(V4L2, Debug) << "Control: " << ctrl.name
> +				 << " (" << utils::hex(ctrl.id) << ")";
> +
>  		controlIds_.emplace_back(v4l2ControlId(ctrl));
>  		controlInfo_.emplace(ctrl.id, ctrl);
>  
> -		ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));
> +		ctrls.emplace(controlIds_.back().get(), std::move(ctrlInfo));
>  	}
>  
>  	controls_ = std::move(ctrls);
> @@ -638,6 +673,10 @@ void V4L2Device::updateControls(ControlList *ctrls,
>  			value.set<int64_t>(v4l2Ctrl->value64);
>  			break;
>  
> +		case ControlTypeInteger32:
> +			value.set<int32_t>(v4l2Ctrl->value);

This needs a rebase, it's now v4l2Ctrl.value.

With these minor issues addressed, the patch looks good to me. Thanks
for your continuous effort.

> +			break;
> +
>  		case ControlTypeByte:
>  			/*
>  			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
Hirokazu Honda June 7, 2021, 12:14 a.m. UTC | #2 
Hi Laurent, thank you for reviewing.

On Mon, Jun 7, 2021 at 8:22 AM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Hiro,
>
> Thank you for the patch.
>
> On Fri, May 28, 2021 at 12:05:27PM +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>
> > ---
> >  include/libcamera/internal/v4l2_device.h |  2 +
> >  src/libcamera/v4l2_device.cpp            | 47 ++++++++++++++++++++++--
> >  2 files changed, 45 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_device.h
> b/include/libcamera/internal/v4l2_device.h
> > index b82e2a14..687be9b2 100644
> > --- a/include/libcamera/internal/v4l2_device.h
> > +++ b/include/libcamera/internal/v4l2_device.h
> > @@ -56,6 +56,8 @@ protected:
> >       int fd() const { return fd_; }
> >
> >  private:
> > +     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 caafbc2d..032252bb 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -530,6 +530,33 @@ int V4L2Device::ioctl(unsigned long request, void
> *argp)
> >   * \return The V4L2 device file descriptor, -1 if the device node is
> not open
> >   */
> >
> > +/**
> > + * \brief Create ControlInfo for v4l2 menu ctrl
>
> s/for v4l2 menu ctrl/for a V4L2 menu control/
>
> > + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
> > + * \param[out] ctrlInfo The created controlInfo
>
> It's not an out parameter, it's the return value.
>
> > + *
> > + * The created ControlInfo contains indices acquired by
> VIDIOC_QUERYMENU.
> > + *
> > + * \return ControlInfo for v4l2 menu ctrl.
> > + */
> > +ControlInfo V4L2Device::v4l2MenuControlInfo(const v4l2_query_ext_ctrl
> &ctrl)
> > +{
> > +     std::vector<ControlValue> indices;
> > +     v4l2_querymenu menu;
>
> We usually prefix the V4L2 structures with struct. While not strictly
> mandatory, it emphasizes that there are C structs from V4L2.
>
> > +     memset(&menu, 0, sizeof(menu));
>
> You could also write this
>
>         struct v4l2_querymenu menu = { };
>
> (I really wish designated initializers were available before C++20 :-().
>
> > +     menu.id = ctrl.id;
>
> As a sanity check, should we add
>
>         if (ctrl.minimum <= 0)
>                 return ControlInfo();
>
> ?
>
> > +
> > +     for (menu.index = ctrl.minimum;
> > +          static_cast<int32_t>(menu.index) <= ctrl.maximum;
> menu.index++) {
>
>         for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index)
> }
>                 menu.index = index;
>
> would allow dropping the cast. Up to you.
>
> > +             if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
> > +                     continue;
> > +
> > +             indices.emplace_back(static_cast<int32_t>(menu.index));
>
> This could be
>
>                 indices.emplace_back(index);
>
> or even
>
>                 indices.push_back(index);
>
> as it won't make much of a difference.
>
> > +     }
> > +
> > +     return ControlInfo(indices);
>
> Would it be useful to also pass the default value to the ControlInfo
> constructor ?
>
>
You mean, I should set ctrl.default_value or some value in indices like
indices.begin()?


> > +}
> > +
> >  /*
> >   * \brief List and store information about all controls supported by the
> >   * V4L2 device
> > @@ -539,7 +566,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;
> > @@ -550,16 +576,22 @@ void V4L2Device::listControls()
> >                   ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
> >                       continue;
> >
> > +             ControlInfo ctrlInfo;
>
> I'd add a blank line here.
>
> >               switch (ctrl.type) {
> >               case V4L2_CTRL_TYPE_INTEGER:
> >               case V4L2_CTRL_TYPE_BOOLEAN:
> > -             case V4L2_CTRL_TYPE_MENU:
> >               case V4L2_CTRL_TYPE_BUTTON:
> >               case V4L2_CTRL_TYPE_INTEGER64:
> >               case V4L2_CTRL_TYPE_BITMASK:
> > -             case V4L2_CTRL_TYPE_INTEGER_MENU:
> >               case V4L2_CTRL_TYPE_U8:
> > +                     ctrlInfo = v4l2ControlInfo(ctrl);
> > +                     break;
> > +
> > +             case V4L2_CTRL_TYPE_INTEGER_MENU:
> > +             case V4L2_CTRL_TYPE_MENU:
> > +                     ctrlInfo = v4l2MenuControlInfo(ctrl);
> >                       break;
> > +
> >               /* \todo Support other control types. */
> >               default:
> >                       LOG(V4L2, Debug)
> > @@ -568,10 +600,13 @@ void V4L2Device::listControls()
> >                       continue;
> >               }
> >
> > +             LOG(V4L2, Debug) << "Control: " << ctrl.name
> > +                              << " (" << utils::hex(ctrl.id) << ")";
> > +
> >               controlIds_.emplace_back(v4l2ControlId(ctrl));
> >               controlInfo_.emplace(ctrl.id, ctrl);
> >
> > -             ctrls.emplace(controlIds_.back().get(),
> v4l2ControlInfo(ctrl));
> > +             ctrls.emplace(controlIds_.back().get(),
> std::move(ctrlInfo));
> >       }
> >
> >       controls_ = std::move(ctrls);
> > @@ -638,6 +673,10 @@ void V4L2Device::updateControls(ControlList *ctrls,
> >                       value.set<int64_t>(v4l2Ctrl->value64);
> >                       break;
> >
> > +             case ControlTypeInteger32:
> > +                     value.set<int32_t>(v4l2Ctrl->value);
>
> This needs a rebase, it's now v4l2Ctrl.value.
>

By which patch? On top of tree, v4l2Ctrl is still a const pointer.

Thanks,
-Hiro

>
> With these minor issues addressed, the patch looks good to me. Thanks
> for your continuous effort.
>
> > +                     break;
> > +
> >               case ControlTypeByte:
> >                       /*
> >                        * No action required, the VIDIOC_[GS]_EXT_CTRLS
> ioctl
>
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart June 7, 2021, 12:46 a.m. UTC | #3 
Hi Hiro,

On Mon, Jun 07, 2021 at 09:14:21AM +0900, Hirokazu Honda wrote:
> On Mon, Jun 7, 2021 at 8:22 AM Laurent Pinchart wrote:
> > On Fri, May 28, 2021 at 12:05:27PM +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>
> > > ---
> > >  include/libcamera/internal/v4l2_device.h |  2 +
> > >  src/libcamera/v4l2_device.cpp            | 47 ++++++++++++++++++++++--
> > >  2 files changed, 45 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> > > index b82e2a14..687be9b2 100644
> > > --- a/include/libcamera/internal/v4l2_device.h
> > > +++ b/include/libcamera/internal/v4l2_device.h
> > > @@ -56,6 +56,8 @@ protected:
> > >       int fd() const { return fd_; }
> > >
> > >  private:
> > > +     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 caafbc2d..032252bb 100644
> > > --- a/src/libcamera/v4l2_device.cpp
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -530,6 +530,33 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> > >   * \return The V4L2 device file descriptor, -1 if the device node is not open
> > >   */
> > >
> > > +/**
> > > + * \brief Create ControlInfo for v4l2 menu ctrl
> >
> > s/for v4l2 menu ctrl/for a V4L2 menu control/
> >
> > > + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
> > > + * \param[out] ctrlInfo The created controlInfo
> >
> > It's not an out parameter, it's the return value.
> >
> > > + *
> > > + * The created ControlInfo contains indices acquired by VIDIOC_QUERYMENU.
> > > + *
> > > + * \return ControlInfo for v4l2 menu ctrl.
> > > + */
> > > +ControlInfo V4L2Device::v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl)
> > > +{
> > > +     std::vector<ControlValue> indices;
> > > +     v4l2_querymenu menu;
> >
> > We usually prefix the V4L2 structures with struct. While not strictly
> > mandatory, it emphasizes that there are C structs from V4L2.
> >
> > > +     memset(&menu, 0, sizeof(menu));
> >
> > You could also write this
> >
> >         struct v4l2_querymenu menu = { };
> >
> > (I really wish designated initializers were available before C++20 :-().
> >
> > > +     menu.id = ctrl.id;
> >
> > As a sanity check, should we add
> >
> >         if (ctrl.minimum <= 0)
> >                 return ControlInfo();
> >
> > ?
> >
> > > +
> > > +     for (menu.index = ctrl.minimum;
> > > +          static_cast<int32_t>(menu.index) <= ctrl.maximum; menu.index++) {
> >
> >         for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index)
> > }
> >                 menu.index = index;
> >
> > would allow dropping the cast. Up to you.
> >
> > > +             if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
> > > +                     continue;
> > > +
> > > +             indices.emplace_back(static_cast<int32_t>(menu.index));
> >
> > This could be
> >
> >                 indices.emplace_back(index);
> >
> > or even
> >
> >                 indices.push_back(index);
> >
> > as it won't make much of a difference.
> >
> > > +     }
> > > +
> > > +     return ControlInfo(indices);
> >
> > Would it be useful to also pass the default value to the ControlInfo
> > constructor ?
>
> You mean, I should set ctrl.default_value or some value in indices like
> indices.begin()?

I mean (untested)

	return ControlInfo(indices, ControlValue<int32_t>(ctrl.dev));

> > > +}
> > > +
> > >  /*
> > >   * \brief List and store information about all controls supported by the
> > >   * V4L2 device
> > > @@ -539,7 +566,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;
> > > @@ -550,16 +576,22 @@ void V4L2Device::listControls()
> > >                   ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
> > >                       continue;
> > >
> > > +             ControlInfo ctrlInfo;
> >
> > I'd add a blank line here.
> >
> > >               switch (ctrl.type) {
> > >               case V4L2_CTRL_TYPE_INTEGER:
> > >               case V4L2_CTRL_TYPE_BOOLEAN:
> > > -             case V4L2_CTRL_TYPE_MENU:
> > >               case V4L2_CTRL_TYPE_BUTTON:
> > >               case V4L2_CTRL_TYPE_INTEGER64:
> > >               case V4L2_CTRL_TYPE_BITMASK:
> > > -             case V4L2_CTRL_TYPE_INTEGER_MENU:
> > >               case V4L2_CTRL_TYPE_U8:
> > > +                     ctrlInfo = v4l2ControlInfo(ctrl);
> > > +                     break;
> > > +
> > > +             case V4L2_CTRL_TYPE_INTEGER_MENU:
> > > +             case V4L2_CTRL_TYPE_MENU:
> > > +                     ctrlInfo = v4l2MenuControlInfo(ctrl);
> > >                       break;
> > > +
> > >               /* \todo Support other control types. */
> > >               default:
> > >                       LOG(V4L2, Debug)
> > > @@ -568,10 +600,13 @@ void V4L2Device::listControls()
> > >                       continue;
> > >               }
> > >
> > > +             LOG(V4L2, Debug) << "Control: " << ctrl.name
> > > +                              << " (" << utils::hex(ctrl.id) << ")";
> > > +
> > >               controlIds_.emplace_back(v4l2ControlId(ctrl));
> > >               controlInfo_.emplace(ctrl.id, ctrl);
> > >
> > > -             ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));
> > > +             ctrls.emplace(controlIds_.back().get(), std::move(ctrlInfo));
> > >       }
> > >
> > >       controls_ = std::move(ctrls);
> > > @@ -638,6 +673,10 @@ void V4L2Device::updateControls(ControlList *ctrls,
> > >                       value.set<int64_t>(v4l2Ctrl->value64);
> > >                       break;
> > >
> > > +             case ControlTypeInteger32:
> > > +                     value.set<int32_t>(v4l2Ctrl->value);
> >
> > This needs a rebase, it's now v4l2Ctrl.value.
> 
> By which patch? On top of tree, v4l2Ctrl is still a const pointer.

By commit abdb11d9ccad ("libcamera: V4L2Device: Remove the controls
order assumption in updateControls()")  :-)

> > With these minor issues addressed, the patch looks good to me. Thanks
> > for your continuous effort.
> >
> > > +                     break;
> > > +
> > >               case ControlTypeByte:
> > >                       /*
> > >                        * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
Hirokazu Honda June 7, 2021, 12:54 a.m. UTC | #4 
Hi Laurent,

On Mon, Jun 7, 2021 at 9:46 AM Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Hiro,
>
> On Mon, Jun 07, 2021 at 09:14:21AM +0900, Hirokazu Honda wrote:
> > On Mon, Jun 7, 2021 at 8:22 AM Laurent Pinchart wrote:
> > > On Fri, May 28, 2021 at 12:05:27PM +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>
> > > > ---
> > > >  include/libcamera/internal/v4l2_device.h |  2 +
> > > >  src/libcamera/v4l2_device.cpp            | 47
> ++++++++++++++++++++++--
> > > >  2 files changed, 45 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/v4l2_device.h
> b/include/libcamera/internal/v4l2_device.h
> > > > index b82e2a14..687be9b2 100644
> > > > --- a/include/libcamera/internal/v4l2_device.h
> > > > +++ b/include/libcamera/internal/v4l2_device.h
> > > > @@ -56,6 +56,8 @@ protected:
> > > >       int fd() const { return fd_; }
> > > >
> > > >  private:
> > > > +     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 caafbc2d..032252bb 100644
> > > > --- a/src/libcamera/v4l2_device.cpp
> > > > +++ b/src/libcamera/v4l2_device.cpp
> > > > @@ -530,6 +530,33 @@ int V4L2Device::ioctl(unsigned long request,
> void *argp)
> > > >   * \return The V4L2 device file descriptor, -1 if the device node
> is not open
> > > >   */
> > > >
> > > > +/**
> > > > + * \brief Create ControlInfo for v4l2 menu ctrl
> > >
> > > s/for v4l2 menu ctrl/for a V4L2 menu control/
> > >
> > > > + * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
> > > > + * \param[out] ctrlInfo The created controlInfo
> > >
> > > It's not an out parameter, it's the return value.
> > >
> > > > + *
> > > > + * The created ControlInfo contains indices acquired by
> VIDIOC_QUERYMENU.
> > > > + *
> > > > + * \return ControlInfo for v4l2 menu ctrl.
> > > > + */
> > > > +ControlInfo V4L2Device::v4l2MenuControlInfo(const
> v4l2_query_ext_ctrl &ctrl)
> > > > +{
> > > > +     std::vector<ControlValue> indices;
> > > > +     v4l2_querymenu menu;
> > >
> > > We usually prefix the V4L2 structures with struct. While not strictly
> > > mandatory, it emphasizes that there are C structs from V4L2.
> > >
> > > > +     memset(&menu, 0, sizeof(menu));
> > >
> > > You could also write this
> > >
> > >         struct v4l2_querymenu menu = { };
> > >
> > > (I really wish designated initializers were available before C++20
> :-().
> > >
> > > > +     menu.id = ctrl.id;
> > >
> > > As a sanity check, should we add
> > >
> > >         if (ctrl.minimum <= 0)
> > >                 return ControlInfo();
> > >
> > > ?
> > >
> > > > +
> > > > +     for (menu.index = ctrl.minimum;
> > > > +          static_cast<int32_t>(menu.index) <= ctrl.maximum;
> menu.index++) {
> > >
> > >         for (int32_t index = ctrl.minimum; index <= ctrl.maximum;
> ++index)
> > > }
> > >                 menu.index = index;
> > >
> > > would allow dropping the cast. Up to you.
> > >
> > > > +             if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
> > > > +                     continue;
> > > > +
> > > > +             indices.emplace_back(static_cast<int32_t>(menu.index));
> > >
> > > This could be
> > >
> > >                 indices.emplace_back(index);
> > >
> > > or even
> > >
> > >                 indices.push_back(index);
> > >
> > > as it won't make much of a difference.
> > >
> > > > +     }
> > > > +
> > > > +     return ControlInfo(indices);
> > >
> > > Would it be useful to also pass the default value to the ControlInfo
> > > constructor ?
> >
> > You mean, I should set ctrl.default_value or some value in indices like
> > indices.begin()?
>
> I mean (untested)
>
>         return ControlInfo(indices, ControlValue<int32_t>(ctrl.dev));
>
>
I guess you mean ctrl.default_value. Filled so if querymenu for the value
is successful.


> > > > +}
> > > > +
> > > >  /*
> > > >   * \brief List and store information about all controls supported
> by the
> > > >   * V4L2 device
> > > > @@ -539,7 +566,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;
> > > > @@ -550,16 +576,22 @@ void V4L2Device::listControls()
> > > >                   ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
> > > >                       continue;
> > > >
> > > > +             ControlInfo ctrlInfo;
> > >
> > > I'd add a blank line here.
> > >
> > > >               switch (ctrl.type) {
> > > >               case V4L2_CTRL_TYPE_INTEGER:
> > > >               case V4L2_CTRL_TYPE_BOOLEAN:
> > > > -             case V4L2_CTRL_TYPE_MENU:
> > > >               case V4L2_CTRL_TYPE_BUTTON:
> > > >               case V4L2_CTRL_TYPE_INTEGER64:
> > > >               case V4L2_CTRL_TYPE_BITMASK:
> > > > -             case V4L2_CTRL_TYPE_INTEGER_MENU:
> > > >               case V4L2_CTRL_TYPE_U8:
> > > > +                     ctrlInfo = v4l2ControlInfo(ctrl);
> > > > +                     break;
> > > > +
> > > > +             case V4L2_CTRL_TYPE_INTEGER_MENU:
> > > > +             case V4L2_CTRL_TYPE_MENU:
> > > > +                     ctrlInfo = v4l2MenuControlInfo(ctrl);
> > > >                       break;
> > > > +
> > > >               /* \todo Support other control types. */
> > > >               default:
> > > >                       LOG(V4L2, Debug)
> > > > @@ -568,10 +600,13 @@ void V4L2Device::listControls()
> > > >                       continue;
> > > >               }
> > > >
> > > > +             LOG(V4L2, Debug) << "Control: " << ctrl.name
> > > > +                              << " (" << utils::hex(ctrl.id) <<
> ")";
> > > > +
> > > >               controlIds_.emplace_back(v4l2ControlId(ctrl));
> > > >               controlInfo_.emplace(ctrl.id, ctrl);
> > > >
> > > > -             ctrls.emplace(controlIds_.back().get(),
> v4l2ControlInfo(ctrl));
> > > > +             ctrls.emplace(controlIds_.back().get(),
> std::move(ctrlInfo));
> > > >       }
> > > >
> > > >       controls_ = std::move(ctrls);
> > > > @@ -638,6 +673,10 @@ void V4L2Device::updateControls(ControlList
> *ctrls,
> > > >                       value.set<int64_t>(v4l2Ctrl->value64);
> > > >                       break;
> > > >
> > > > +             case ControlTypeInteger32:
> > > > +                     value.set<int32_t>(v4l2Ctrl->value);
> > >
> > > This needs a rebase, it's now v4l2Ctrl.value.
> >
> > By which patch? On top of tree, v4l2Ctrl is still a const pointer.
>
> By commit abdb11d9ccad ("libcamera: V4L2Device: Remove the controls
> order assumption in updateControls()")  :-)
>
>
Okay, I saw it was merged just while ago. Thanks for merging.

-Hiro

> > > With these minor issues addressed, the patch looks good to me. Thanks
> > > for your continuous effort.
> > >
> > > > +                     break;
> > > > +
> > > >               case ControlTypeByte:
> > > >                       /*
> > > >                        * No action required, the
> VIDIOC_[GS]_EXT_CTRLS ioctl
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
index b82e2a14..687be9b2 100644
--- a/include/libcamera/internal/v4l2_device.h
+++ b/include/libcamera/internal/v4l2_device.h
@@ -56,6 +56,8 @@  protected:
 	int fd() const { return fd_; }
 
 private:
+	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 caafbc2d..032252bb 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -530,6 +530,33 @@  int V4L2Device::ioctl(unsigned long request, void *argp)
  * \return The V4L2 device file descriptor, -1 if the device node is not open
  */
 
+/**
+ * \brief Create ControlInfo for v4l2 menu ctrl
+ * \param[in] ctrl The v4l2_query_ext_ctrl that represents a menu
+ * \param[out] ctrlInfo The created controlInfo
+ *
+ * The created ControlInfo contains indices acquired by VIDIOC_QUERYMENU.
+ *
+ * \return ControlInfo for v4l2 menu ctrl.
+ */
+ControlInfo V4L2Device::v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl)
+{
+	std::vector<ControlValue> indices;
+	v4l2_querymenu menu;
+	memset(&menu, 0, sizeof(menu));
+	menu.id = ctrl.id;
+
+	for (menu.index = ctrl.minimum;
+	     static_cast<int32_t>(menu.index) <= ctrl.maximum; menu.index++) {
+		if (ioctl(VIDIOC_QUERYMENU, &menu) != 0)
+			continue;
+
+		indices.emplace_back(static_cast<int32_t>(menu.index));
+	}
+
+	return ControlInfo(indices);
+}
+
 /*
  * \brief List and store information about all controls supported by the
  * V4L2 device
@@ -539,7 +566,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;
@@ -550,16 +576,22 @@  void V4L2Device::listControls()
 		    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)
 			continue;
 
+		ControlInfo ctrlInfo;
 		switch (ctrl.type) {
 		case V4L2_CTRL_TYPE_INTEGER:
 		case V4L2_CTRL_TYPE_BOOLEAN:
-		case V4L2_CTRL_TYPE_MENU:
 		case V4L2_CTRL_TYPE_BUTTON:
 		case V4L2_CTRL_TYPE_INTEGER64:
 		case V4L2_CTRL_TYPE_BITMASK:
-		case V4L2_CTRL_TYPE_INTEGER_MENU:
 		case V4L2_CTRL_TYPE_U8:
+			ctrlInfo = v4l2ControlInfo(ctrl);
+			break;
+
+		case V4L2_CTRL_TYPE_INTEGER_MENU:
+		case V4L2_CTRL_TYPE_MENU:
+			ctrlInfo = v4l2MenuControlInfo(ctrl);
 			break;
+
 		/* \todo Support other control types. */
 		default:
 			LOG(V4L2, Debug)
@@ -568,10 +600,13 @@  void V4L2Device::listControls()
 			continue;
 		}
 
+		LOG(V4L2, Debug) << "Control: " << ctrl.name
+				 << " (" << utils::hex(ctrl.id) << ")";
+
 		controlIds_.emplace_back(v4l2ControlId(ctrl));
 		controlInfo_.emplace(ctrl.id, ctrl);
 
-		ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));
+		ctrls.emplace(controlIds_.back().get(), std::move(ctrlInfo));
 	}
 
 	controls_ = std::move(ctrls);
@@ -638,6 +673,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