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

[Laurent Pinchart]

Hi David,

On Thu, Dec 30, 2021 at 06:22:33PM +0200, Laurent Pinchart wrote:
> 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

Or even

@@ -681,16 +689,6 @@ void V4L2Device::updateControls(ControlList *ctrls,
 			break;

 		case ControlTypeInteger32:
-			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