[libcamera-devel] [PATCH 2/3] libcamera: v4l2_device: Add support for integer array controls

[Laurent Pinchart]

Hi David,

On Thu, Dec 30, 2021 at 09:28:40AM +0000, David Plowman wrote:
> On Fri, 24 Dec 2021 at 03:16, Laurent Pinchart wrote:
> > On Thu, Dec 23, 2021 at 08:01:09AM +0000, David Plowman wrote:
> >
> > A commit message would be nice.
> >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  src/libcamera/v4l2_device.cpp | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > index 62c91779..6f9de8ad 100644
> > > --- a/src/libcamera/v4l2_device.cpp
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -316,6 +316,18 @@ int V4L2Device::setControls(ControlList *ctrls)
> > >                       break;
> > >               }
> > >
> > > +             case ControlTypeInteger32: {
> >
> > Could you move this above ControlTypeInteger32 ?
> 
> I'm guessing you mean this should go between ControlTypeInteger64 and
> ControlTypeIntegerByte, like I see in updateControls()? Will do!

I would actually put 32 before 64 (so just before ControlTypeInteger64).
That doesn't match V4L2Device::updateControls(), but as you'll have to
update that function anyway, you can also fix the order there :-)

> >
> > > +                     if (value.isArray()) {
> > > +                             Span<uint8_t> data = value.data();
> > > +                             v4l2Ctrl.p_u32 = reinterpret_cast<uint32_t *>(data.data());
> > > +                             v4l2Ctrl.size = data.size();
> > > +                     } else {
> > > +                             v4l2Ctrl.value = value.get<int32_t>();
> > > +                     }
> >
> > This looks fine, but I think you also need to update
> > V4L2Device::updateControls().
> 
> Just to check (my understanding of how ControlLists work has always
> been a bit hazy...), the same logic will apply there as in the "Byte"
> case, i.e. I just need to check for the array type and then do
> nothing?

That's correct. Actually, maybe the following would be more generic ?

diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 1e26305742db..be66b8eb523a 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -671,6 +671,14 @@ void V4L2Device::updateControls(ControlList *ctrls,
 		const unsigned int id = v4l2Ctrl.id;

 		ControlValue value = ctrls->get(id);
+		if (value.isArray()) {
+			/*
+			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
+			 * accessed the ControlValue storage directly for array
+			 * controls.
+			 */
+			continue;
+		}

 		const auto iter = controls_.find(id);
 		ASSERT(iter != controls_.end());
@@ -684,13 +692,6 @@ void V4L2Device::updateControls(ControlList *ctrls,
 			value.set<int32_t>(v4l2Ctrl.value);
 			break;

-		case ControlTypeByte:
-			/*
-			 * No action required, the VIDIOC_[GS]_EXT_CTRLS ioctl
-			 * accessed the ControlValue storage directly.
-			 */
-			break;
-
 		default:
 			/*
 			 * \todo To be changed when support for string controls


I haven't tested it, maybe I'm missing something.

> > > +
> > > +                     break;
> > > +             }
> > > +
> > >               default:
> > >                       /* \todo To be changed to support strings. */
> > >                       v4l2Ctrl.value = value.get<int32_t>();

-- 
Regards,

Laurent Pinchart