[libcamera-devel] [PATCH v4 3/6] libcamera: v4l2_device: Implement get and set controls

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Jun 19, 2019 at 01:08:55PM +0200, Jacopo Mondi wrote:
> Implement getControls() and setControls() operations in V4L2Device class.
> 
> Both operations take a V4L2Controls instance and read or write the V4L2
> controls on the V4L2 device.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/include/v4l2_device.h |   6 +
>  src/libcamera/v4l2_device.cpp       | 189 ++++++++++++++++++++++++++++
>  2 files changed, 195 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 563be151c257..e66cc7ebe737 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -15,6 +15,7 @@
>  namespace libcamera {
>  
>  class V4L2ControlInfo;
> +class V4L2Controls;
>  
>  class V4L2Device : protected Loggable
>  {
> @@ -23,6 +24,10 @@ public:
>  	bool isOpen() const { return fd_ != -1; }
>  	const std::string &deviceNode() const { return deviceNode_; }
>  
> +	V4L2ControlInfo *getControlInfo(unsigned int id);

Should this be moved to the previous patch ?

The returned pointer should be const, and the function should be const
too.

> +	int getControls(V4L2Controls *ctrls);
> +	int setControls(V4L2Controls *ctrls);
> +
>  protected:
>  	V4L2Device(const std::string &deviceNode, const std::string &logTag);
>  	~V4L2Device();
> @@ -37,6 +42,7 @@ protected:
>  
>  private:
>  	void listControls();
> +	int validateControlType(V4L2ControlInfo *info);
>  
>  	std::map<unsigned int, V4L2ControlInfo *> controls_;
>  	std::string deviceNode_;
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 58dd94836686..5af0ddc468fe 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -66,6 +66,170 @@ void V4L2Device::close()
>   * \return The device node path
>   */
>  
> +/**
> + * \brief Get informations for control with \a id

"Retrieve information about a control" ?

> + * \param[in] id The control id to search info for

"The control ID" should be enough.

> + * \return The V4L2ControlInfo associated to the V4L2 control with \a id or
> + * nullptr if the control is not supported by the V4L2Device

\return A pointer to the V4L2ControlInfo for control \a id, or nullptr
if the device doesn't have such a control

?

> + */
> +V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id)
> +{
> +	auto it = controls_.find(id);
> +	if (it == controls_.end())
> +		return nullptr;
> +
> +	return it->second;
> +}
> +
> +/**
> + * \brief Read values of a list of controls from the device

"Read controls from the device"

> + * \param[inout] ctrls The list of controls to read
> + *
> + * Read the value of each V4L2Controls contained in \a ctrls, overwriting
> + * it's current value with the one returned by the device.

It's hard to write precise documentation without making it difficult to
read :-/ How about

"Read the value of each control contained in \a ctrls, and store the
value in the corresponding \a ctrls entry."

> + *
> + * Each V4L2Control instance in \a ctrls should be supported by the device.

"If one control in \a ctrls is not supported by the device this method
returns an error. In that case the values stored in \a ctrls are
undefined."

> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2Device::getControls(V4L2Controls *ctrls)
> +{
> +	unsigned int count = ctrls->size();
> +	if (count == 0)
> +		return 0;
> +
> +	struct V4L2ControlInfo *controlInfo[count];
> +	struct v4l2_ext_control v4l2Ctrls[count];
> +	for (unsigned int i = 0; i < count; ++i) {
> +		V4L2Control *ctrl = (*ctrls)[i];
> +
> +		/* Validate the control. */
> +		V4L2ControlInfo *info = getControlInfo(ctrl->id());
> +		if (!info) {
> +			LOG(V4L2, Error) << "Control '" << ctrl->id()
> +					 << "' not valid";

s/not valid/not found/

> +			return -EINVAL;
> +		}
> +
> +		if (validateControlType(info))
> +			return -EINVAL;
> +
> +		controlInfo[i] = info;
> +
> +		/* Prepare the v4l2_ext_control entry for the read operation. */
> +		struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> +		v4l2Ctrl->id = info->id();
> +		v4l2Ctrl->size = info->size();

size is only needed for controls with payloads, which we don't support.
I would leave it set to 0, otherwise if we try to get a payload control
by mistake, and the value happens to be a valid pointer to our memory
address space, the kernel could overwrite that.

I would then also remove the v4l2Ctrl variable and write

		v4l2Ctrls[i].id = ctrl->id();

> +	}
> +
> +	struct v4l2_ext_controls v4l2ExtCtrls = {};
> +	v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> +	v4l2ExtCtrls.controls = v4l2Ctrls;
> +	v4l2ExtCtrls.count = count;
> +
> +	int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);
> +	if (ret) {
> +		LOG(V4L2, Error) << "Unable to read controls: "
> +				 << strerror(ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * For each control read from the device, set the value in the
> +	 * V4L2ControlValue provided by the caller.

That's called V4L2Control now, not V4L2ControlValue.

> +	 */
> +	for (unsigned int i = 0; i < count; ++i) {
> +		struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> +		V4L2ControlInfo *info = controlInfo[i];
> +		V4L2Control *ctrl = (*ctrls)[i];
> +
> +		switch (info->type()) {
> +		case V4L2_CTRL_TYPE_INTEGER64:
> +			ctrl->setValue(v4l2Ctrl->value64);
> +			break;
> +		case V4L2_CTRL_TYPE_INTEGER:
> +		case V4L2_CTRL_TYPE_BOOLEAN:
> +		case V4L2_CTRL_TYPE_BUTTON:
> +		case V4L2_CTRL_TYPE_MENU:
> +			ctrl->setValue(static_cast<long int>(v4l2Ctrl->value));
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Write a list of controls to the device

"Write controls to the device"

> + * \param[in] ctrls The list of controls to apply

s/apply/write/

> + *
> + * Write the value of each V4L2Control contained in \a ctrls. Each
> + * control should be initialized by the caller with a value, otherwise
> + * the result of the operation is undefined.
> + *
> + * The value of each control is not updated to reflect what has been
> + * actually applied on the device. To read the actual value of a control
> + * after a setControls(), users should read the control value back with
> + * getControls().

Why is that ? It would require another potentially expensive operation.

> + *
> + * Each V4L2Control instance in \a ctrls should be supported by the device.

"If one control in \a ctrls is not supported by the device this method
returns an error." to copy the text from getControls() ?

> + *
> + * \return 0 on success or a negative error code otherwise

I wonder if we should return the number of controls successfully
written. This would be 0 if the pre-validation fails, and we could use
v4l2_ext_controls::error_idx to get the index of the control that failed
(note that if error_idx == count then no control have been written).

> + */
> +int V4L2Device::setControls(V4L2Controls *ctrls)
> +{
> +	unsigned int count = ctrls->size();
> +	if (count == 0)
> +		return 0;
> +
> +	struct v4l2_ext_control v4l2Ctrls[count];
> +	for (unsigned int i = 0; i < count; ++i) {
> +		V4L2Control *ctrl = (*ctrls)[i];
> +
> +		/* Validate the control. */
> +		V4L2ControlInfo *info = getControlInfo(ctrl->id());
> +		if (!info) {
> +			LOG(V4L2, Error) << "Control '" << ctrl->id()
> +					 << "' not valid";
> +			return -EINVAL;
> +		}
> +
> +		if (validateControlType(info))
> +			return -EINVAL;
> +
> +		/* Prepare the v4l2_ext_control entry for the write operation. */
> +		struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> +		v4l2Ctrl->id = info->id();
> +		v4l2Ctrl->size = info->size();
> +
> +		switch (info->type()) {
> +		case V4L2_CTRL_TYPE_INTEGER64:
> +			v4l2Ctrl->value64 = ctrl->value();
> +			break;
> +		case V4L2_CTRL_TYPE_INTEGER:
> +		case V4L2_CTRL_TYPE_BOOLEAN:
> +		case V4L2_CTRL_TYPE_BUTTON:
> +		case V4L2_CTRL_TYPE_MENU:
> +			v4l2Ctrl->value = static_cast<int32_t>(ctrl->value());
> +			break;
> +		}

This switch duplicates the one in validateControlType(). I would just
add a default case here and return an error, removing the call to
validateControlType(). The method would be left iwht a single caller, so
I would then inline it in getControls().

> +	}
> +
> +	struct v4l2_ext_controls v4l2ExtCtrls = {};
> +	v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> +	v4l2ExtCtrls.controls = v4l2Ctrls;
> +	v4l2ExtCtrls.count = count;
> +
> +	int ret = ioctl(VIDIOC_S_EXT_CTRLS, &v4l2ExtCtrls);
> +	if (ret) {
> +		LOG(V4L2, Error) << "Unable to write controls: "
> +				 << strerror(ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Construct a V4L2Device
>   * \param[in] deviceNode The device node filesystem path
> @@ -171,4 +335,29 @@ void V4L2Device::listControls()
>  	}
>  }
>  
> +/*
> + * \brief Make sure the control type is supported
> + * \param[in] info The V4L2ControlInfo to inspect type of
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EINVAL The control type is not supported
> + */
> +int V4L2Device::validateControlType(V4L2ControlInfo *info)
> +{
> +	/* \todo support compound and menu controls. */
> +	switch (info->type()) {
> +	case V4L2_CTRL_TYPE_INTEGER64:
> +	case V4L2_CTRL_TYPE_INTEGER:
> +	case V4L2_CTRL_TYPE_BOOLEAN:
> +	case V4L2_CTRL_TYPE_BUTTON:
> +	case V4L2_CTRL_TYPE_MENU:
> +		break;
> +	default:
> +		LOG(V4L2, Error) << "Control type '" << info->type()
> +				 << "' not supported";
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

Another option would be to skip unsupported control types when
enumerating them, so the getControlInfo() call would return a nullptr.

> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart