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

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Wed, Jan 05, 2022 at 08:55:52AM +0000, David Plowman wrote:
> V4L2Device::setControl and V4L2Device::updateControl are both updated
> to handle ControlTypeInteger32 array controls.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>

I'm always a bit worried when changing low-level components like this,
due to the risk of regressions with V4L2. If any issue is found later,
that will mean the unit tests coverage is too narrow, so we'll fix that
:-)

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  src/libcamera/v4l2_device.cpp | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 62c91779..3fc8438f 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -297,6 +297,18 @@ int V4L2Device::setControls(ControlList *ctrls)
>  		/* Set the v4l2_ext_control value for the write operation. */
>  		ControlValue &value = ctrl->second;
>  		switch (iter->first->type()) {
> +		case ControlTypeInteger32: {
> +			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>();
> +			}
> +
> +			break;
> +		}
> +
>  		case ControlTypeInteger64:
>  			v4l2Ctrl.value64 = value.get<int64_t>();
>  			break;
> @@ -671,6 +683,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());
> @@ -680,19 +700,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
> -			 * accessed the ControlValue storage directly.
> -			 */
> -			break;
> -
>  		default:
>  			/*
> +			 * Note: this catches the ControlTypeInteger32 case.
> +			 *
>  			 * \todo To be changed when support for string controls
>  			 * will be added.
>  			 */

-- 
Regards,

Laurent Pinchart