[libcamera-devel,v2,4/6] libcamera: v4l2_base: Add V4L2 control support

1385 diff mbox series

Implement operations to set and get V4L2 controls in the V4L2Base class.
The operations allow to set and get a single or a list of controls.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/v4l2_base.h |  11 ++
 src/libcamera/v4l2_base.cpp       | 238 ++++++++++++++++++++++++++++++
 2 files changed, 249 insertions(+)

Hi Jacopo,

Thank you for the patch.

On Mon, Jun 10, 2019 at 06:40:50PM +0200, Jacopo Mondi wrote:
> Implement operations to set and get V4L2 controls in the V4L2Base class.
> The operations allow to set and get a single or a list of controls.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_base.h |  11 ++
>  src/libcamera/v4l2_base.cpp       | 238 ++++++++++++++++++++++++++++++
>  2 files changed, 249 insertions(+)
> 
> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h
> index 2fda81a960d2..17ea734c8f49 100644
> --- a/src/libcamera/include/v4l2_base.h
> +++ b/src/libcamera/include/v4l2_base.h
> @@ -9,6 +9,8 @@
>  
>  #include <vector>
>  
> +#include <v4l2_controls.h>

Just forward-declare the required classes and structures.

> +
>  namespace libcamera {
>  
>  class V4L2Base
> @@ -22,8 +24,17 @@ public:
>  	virtual void close() = 0;
>  	bool isOpen() const;
>  
> +	int getControls(std::vector<unsigned int> &ctrl_ids,
> +			V4L2Controls *ctrls);

	const std::vector<unsigned int> &ctrl_ids,

I still wonder if it wouldn't be simpler for applications to fill the
V4L2Controls with the control IDs to read, and just pass the
V4L2Controls pointer to this function. In any case, this is better than
the first version.

> +	int setControls(V4L2Controls &ctrls);

You should pass a const V4L2Controls &ctrls. Or, maybe you should
instead pass a non-const pointer, as setControls() should return the
value set for each control.

> +
>  protected:
>  	int fd_;
> +
> +private:
> +	int queryControl(unsigned int ctrl_id,
> +			 struct v4l2_query_ext_ctrl *qext_ctrl);
> +
>  };
>  
>  }; /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp
> index 7d05a3c82e4d..cbd7a551130f 100644
> --- a/src/libcamera/v4l2_base.cpp
> +++ b/src/libcamera/v4l2_base.cpp
> @@ -5,7 +5,13 @@
>   * v4l2_base.cpp - Common base for V4L2 devices and subdevices
>   */
>  
> +#include <linux/v4l2-subdev.h>
> +#include <linux/videodev2.h>
> +#include <sys/ioctl.h>
> +
> +#include "log.h"
>  #include "v4l2_base.h"
> +#include "v4l2_controls.h"
>  
>  /**
>   * \file v4l2_base.h
> @@ -14,6 +20,8 @@
>  
>  namespace libcamera {
>  
> +LOG_DEFINE_CATEGORY(V4L2Base)
> +
>  /**
>   * \class V4L2Base
>   * \brief Base class for V4L2Device and V4L2Subdevice
> @@ -31,6 +39,7 @@ namespace libcamera {
>   */
>  
>  /**
> + * \fn V4L2Base::open()
>   * \brief Open a V4L2 device or subdevice
>   *
>   * Pure virtual method to be implemented by the derived classes.
> @@ -39,6 +48,7 @@ namespace libcamera {
>   */
>  
>  /**
> + * \fn V4L2Base::close()
>   * \brief Close the device or subdevice
>   *
>   * Pure virtual method to be implemented by the derived classes.
> @@ -53,6 +63,234 @@ bool V4L2Base::isOpen() const
>  	return fd_ != -1;
>  }
>  
> +int V4L2Base::queryControl(unsigned int id,
> +			   struct v4l2_query_ext_ctrl *qext_ctrl)
> +{
> +	qext_ctrl->id = id;
> +
> +	int ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(V4L2Base, Error)
> +			<< "Unable to query extended control 0x"
> +			<< std::hex << qext_ctrl->id
> +			<< ": " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	if (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
> +		LOG(V4L2Base, Error)
> +			<< "Extended control 0x" << std::hex
> +			<< qext_ctrl->id << " is disabled";
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Get values of a list of control IDs
> + * \param ctrl_ids The list of control IDs to retrieve value of
> + * \return A list of V4L2Control on success or a empty list otherwise
> + */
> +int V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids,
> +			  V4L2Controls *controls)
> +{
> +	unsigned int count = ctrl_ids.size();
> +	if (!count)
> +		return -EINVAL;
> +
> +	controls->clear();
> +
> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];

You're leaking memory. The simplest solution would be

	struct v4l2_ext_controls v4l2_ctrls[count];

> +
> +	/*
> +	 * Query each control in the vector to get back its type and expected
> +	 * size in order to get its current value.
> +	 */
> +	for (unsigned int i = 0; i < count; ++i) {
> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> +		unsigned int ctrl_id = ctrl_ids[i];
> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> +
> +		int ret = queryControl(ctrl_id, &qext_ctrl);
> +		if (ret)
> +			return ret;
> +
> +		v4l2_ctrl->id = ctrl_id;
> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> +
> +		/* Reserve memory for controls with payload. */
> +		if (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))
> +			continue;
> +
> +		switch (qext_ctrl.type) {
> +		case V4L2_CTRL_TYPE_U8:
> +			v4l2_ctrl->p_u8 = static_cast<uint8_t *>
> +					  (new uint8_t[v4l2_ctrl->size]);
> +			break;
> +		case V4L2_CTRL_TYPE_U16:
> +			v4l2_ctrl->p_u16 = static_cast<uint16_t *>
> +					  (new uint16_t[v4l2_ctrl->size]);
> +			break;
> +		case V4L2_CTRL_TYPE_U32:
> +			v4l2_ctrl->p_u32 = static_cast<uint32_t *>
> +					  (new uint32_t[v4l2_ctrl->size]);
> +			break;

This is lots of new's, and they're all leaked. A better solution would
be to populate ctrls with the V4L2Control instances already, and use the
pointer to the internal memory buffers directly.

> +		}
> +	}
> +
> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> +	v4l2_ext_ctrls.count = count;
> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> +
> +	int ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(V4L2Base, Error)
> +			<< "Unable to get extended controls: " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * For each requested control, create a V4L2Control which contains
> +	 * the current value and store it in the vector returned to the caller.
> +	 */
> +	LOG(Error) << __func__;
> +	for (unsigned int i = 0; i < count; ++i) {
> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> +
> +		int ret = queryControl(v4l2_ctrl->id, &qext_ctrl);
> +		if (ret)
> +			return ret;
> +
> +		unsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;
> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> +			/* Control types with payload */
> +			switch (qext_ctrl.type) {
> +			case V4L2_CTRL_TYPE_U8:
> +				controls->set(v4l2_ctrl->id, size,
> +					      v4l2_ctrl->p_u8);
> +				break;
> +			case V4L2_CTRL_TYPE_U16:
> +				controls->set(v4l2_ctrl->id, size,
> +					      v4l2_ctrl->p_u16);
> +				break;
> +			case V4L2_CTRL_TYPE_U32:
> +				controls->set(v4l2_ctrl->id, size,
> +					      v4l2_ctrl->p_u32);
> +				break;
> +			default:
> +				LOG(V4L2Base, Error)
> +					<< "Compound control types not supported: "
> +					<< std::hex << qext_ctrl.type;
> +				return -EINVAL;
> +			}
> +		} else {
> +			/* Control types without payload. */
> +			switch (qext_ctrl.type) {
> +			case V4L2_CTRL_TYPE_INTEGER64:
> +				controls->set(v4l2_ctrl->id, static_cast<int64_t>
> +					     (v4l2_ctrl->value64));
> +				break;
> +			case V4L2_CTRL_TYPE_INTEGER:
> +			case V4L2_CTRL_TYPE_BOOLEAN:
> +			case V4L2_CTRL_TYPE_MENU:
> +			case V4L2_CTRL_TYPE_BUTTON:
> +				controls->set(v4l2_ctrl->id, v4l2_ctrl->value);
> +				break;
> +			default:
> +				LOG(V4L2Base, Error)
> +					<< "Invalid non-compound control type :"
> +					<< std::hex << qext_ctrl.type;
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Apply a list of controls to the device
> + * \param ctrls The list of controls to apply
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2Base::setControls(V4L2Controls &ctrls)
> +{
> +	unsigned int count = ctrls.size();
> +	if (!count)
> +		return -EINVAL;
> +
> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> +
> +	/*
> +	 * Query each control in the vector to get back its type and expected
> +	 * size and set the new desired value in each v4l2_ext_control member.
> +	 */
> +	for (unsigned int i = 0; i < count; ++i) {
> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> +		V4L2Control *ctrl = ctrls[i];
> +
> +		int ret = queryControl(ctrl->id(), &qext_ctrl);
> +		if (ret)
> +			return ret;
> +
> +		v4l2_ctrl->id = ctrl->id();
> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> +
> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> +			/** \todo: Support set controls with payload. */
> +			LOG(V4L2Base, Error)
> +				<< "Controls with payload not supported";
> +			return -EINVAL;
> +		} else {
> +			switch (qext_ctrl.type) {
> +			case V4L2_CTRL_TYPE_INTEGER64:
> +			{
> +				V4L2Int64Control *c =
> +					static_cast<V4L2Int64Control *>(ctrl);
> +				v4l2_ctrl->value64 = c->value();
> +			}
> +				break;
> +			case V4L2_CTRL_TYPE_INTEGER:
> +			case V4L2_CTRL_TYPE_BOOLEAN:
> +			case V4L2_CTRL_TYPE_MENU:
> +			case V4L2_CTRL_TYPE_BUTTON:
> +			{
> +				V4L2IntControl *c =
> +					static_cast<V4L2IntControl *>(ctrl);
> +				v4l2_ctrl->value = c->value();
> +			}
> +				break;
> +			default:
> +				LOG(V4L2Base, Error)
> +					<< "Invalid non-compound control type :"
> +					<< qext_ctrl.type;
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> +	v4l2_ext_ctrls.count = count;
> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> +
> +	int ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);
> +	if (ret) {
> +		ret = -errno;
> +		LOG(V4L2Base, Error)
> +			<< "Unable to set controls: " << strerror(-ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * \var V4L2Base::fd_
>   * \brief The V4L2 device or subdevice device node file descriptor

Hi Laurent, Jacopo

On 11/06/2019 12:41, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Mon, Jun 10, 2019 at 06:40:50PM +0200, Jacopo Mondi wrote:
>> Implement operations to set and get V4L2 controls in the V4L2Base class.
>> The operations allow to set and get a single or a list of controls.
>>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  src/libcamera/include/v4l2_base.h |  11 ++
>>  src/libcamera/v4l2_base.cpp       | 238 ++++++++++++++++++++++++++++++
>>  2 files changed, 249 insertions(+)
>>
>> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h
>> index 2fda81a960d2..17ea734c8f49 100644
>> --- a/src/libcamera/include/v4l2_base.h
>> +++ b/src/libcamera/include/v4l2_base.h
>> @@ -9,6 +9,8 @@
>>  
>>  #include <vector>
>>  
>> +#include <v4l2_controls.h>
> 
> Just forward-declare the required classes and structures.
> 
>> +
>>  namespace libcamera {
>>  
>>  class V4L2Base
>> @@ -22,8 +24,17 @@ public:
>>  	virtual void close() = 0;
>>  	bool isOpen() const;
>>  
>> +	int getControls(std::vector<unsigned int> &ctrl_ids,
>> +			V4L2Controls *ctrls);
> 
> 	const std::vector<unsigned int> &ctrl_ids,
> 
> I still wonder if it wouldn't be simpler for applications to fill the
> V4L2Controls with the control IDs to read, and just pass the
> V4L2Controls pointer to this function. In any case, this is better than
> the first version.


Remembering that 'applications' do not use or see these types of course.

Only Pipelinehandlers should every see or use these objects.


> 
>> +	int setControls(V4L2Controls &ctrls);
> 
> You should pass a const V4L2Controls &ctrls. Or, maybe you should
> instead pass a non-const pointer, as setControls() should return the
> value set for each control.
> 
>> +
>>  protected:
>>  	int fd_;
>> +
>> +private:
>> +	int queryControl(unsigned int ctrl_id,
>> +			 struct v4l2_query_ext_ctrl *qext_ctrl);
>> +
>>  };
>>  
>>  }; /* namespace libcamera */
>> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp
>> index 7d05a3c82e4d..cbd7a551130f 100644
>> --- a/src/libcamera/v4l2_base.cpp
>> +++ b/src/libcamera/v4l2_base.cpp
>> @@ -5,7 +5,13 @@
>>   * v4l2_base.cpp - Common base for V4L2 devices and subdevices
>>   */
>>  
>> +#include <linux/v4l2-subdev.h>
>> +#include <linux/videodev2.h>
>> +#include <sys/ioctl.h>
>> +
>> +#include "log.h"
>>  #include "v4l2_base.h"
>> +#include "v4l2_controls.h"
>>  
>>  /**
>>   * \file v4l2_base.h
>> @@ -14,6 +20,8 @@
>>  
>>  namespace libcamera {
>>  
>> +LOG_DEFINE_CATEGORY(V4L2Base)
>> +
>>  /**
>>   * \class V4L2Base
>>   * \brief Base class for V4L2Device and V4L2Subdevice
>> @@ -31,6 +39,7 @@ namespace libcamera {
>>   */
>>  
>>  /**
>> + * \fn V4L2Base::open()
>>   * \brief Open a V4L2 device or subdevice
>>   *
>>   * Pure virtual method to be implemented by the derived classes.
>> @@ -39,6 +48,7 @@ namespace libcamera {
>>   */
>>  
>>  /**
>> + * \fn V4L2Base::close()
>>   * \brief Close the device or subdevice
>>   *
>>   * Pure virtual method to be implemented by the derived classes.
>> @@ -53,6 +63,234 @@ bool V4L2Base::isOpen() const
>>  	return fd_ != -1;
>>  }
>>  
>> +int V4L2Base::queryControl(unsigned int id,
>> +			   struct v4l2_query_ext_ctrl *qext_ctrl)
>> +{
>> +	qext_ctrl->id = id;
>> +
>> +	int ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);
>> +	if (ret) {
>> +		ret = -errno;
>> +		LOG(V4L2Base, Error)
>> +			<< "Unable to query extended control 0x"
>> +			<< std::hex << qext_ctrl->id
>> +			<< ": " << strerror(-ret);
>> +		return ret;
>> +	}
>> +
>> +	if (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
>> +		LOG(V4L2Base, Error)
>> +			<< "Extended control 0x" << std::hex
>> +			<< qext_ctrl->id << " is disabled";
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * \brief Get values of a list of control IDs
>> + * \param ctrl_ids The list of control IDs to retrieve value of
>> + * \return A list of V4L2Control on success or a empty list otherwise
>> + */
>> +int V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids,
>> +			  V4L2Controls *controls)
>> +{
>> +	unsigned int count = ctrl_ids.size();
>> +	if (!count)
>> +		return -EINVAL;
>> +
>> +	controls->clear();
>> +
>> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> 
> You're leaking memory. The simplest solution would be
> 
> 	struct v4l2_ext_controls v4l2_ctrls[count];
> 
>> +
>> +	/*
>> +	 * Query each control in the vector to get back its type and expected
>> +	 * size in order to get its current value.
>> +	 */
>> +	for (unsigned int i = 0; i < count; ++i) {
>> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
>> +		unsigned int ctrl_id = ctrl_ids[i];
>> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
>> +
>> +		int ret = queryControl(ctrl_id, &qext_ctrl);
>> +		if (ret)
>> +			return ret;
>> +
>> +		v4l2_ctrl->id = ctrl_id;
>> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
>> +
>> +		/* Reserve memory for controls with payload. */
>> +		if (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))
>> +			continue;
>> +
>> +		switch (qext_ctrl.type) {
>> +		case V4L2_CTRL_TYPE_U8:
>> +			v4l2_ctrl->p_u8 = static_cast<uint8_t *>
>> +					  (new uint8_t[v4l2_ctrl->size]);
>> +			break;
>> +		case V4L2_CTRL_TYPE_U16:
>> +			v4l2_ctrl->p_u16 = static_cast<uint16_t *>
>> +					  (new uint16_t[v4l2_ctrl->size]);
>> +			break;
>> +		case V4L2_CTRL_TYPE_U32:
>> +			v4l2_ctrl->p_u32 = static_cast<uint32_t *>
>> +					  (new uint32_t[v4l2_ctrl->size]);
>> +			break;
> 
> This is lots of new's, and they're all leaked. A better solution would
> be to populate ctrls with the V4L2Control instances already, and use the
> pointer to the internal memory buffers directly.


We're doing new's here with v4l2_ctrl->size. Does that imply that these
are payload control values ?

In fact - yes the statement above says so.

I think if the pipeline handler wants to read or set a 'payload' then it
should be responsible for providing that memory - otherwise we will have
more copying involved.


Perhaps we should just drop support for payload controls for now until
we actually hit a need to use it?

Or do you know taht you'll need it for IPU3?



>> +		}
>> +	}
>> +
>> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
>> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
>> +	v4l2_ext_ctrls.count = count;
>> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
>> +
>> +	int ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);
>> +	if (ret) {
>> +		ret = -errno;
>> +		LOG(V4L2Base, Error)
>> +			<< "Unable to get extended controls: " << strerror(-ret);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * For each requested control, create a V4L2Control which contains
>> +	 * the current value and store it in the vector returned to the caller.
>> +	 */
>> +	LOG(Error) << __func__;
>> +	for (unsigned int i = 0; i < count; ++i) {
>> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
>> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
>> +
>> +		int ret = queryControl(v4l2_ctrl->id, &qext_ctrl);
>> +		if (ret)
>> +			return ret;
>> +
>> +		unsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;
>> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
>> +			/* Control types with payload */
>> +			switch (qext_ctrl.type) {
>> +			case V4L2_CTRL_TYPE_U8:
>> +				controls->set(v4l2_ctrl->id, size,
>> +					      v4l2_ctrl->p_u8);
>> +				break;
>> +			case V4L2_CTRL_TYPE_U16:
>> +				controls->set(v4l2_ctrl->id, size,
>> +					      v4l2_ctrl->p_u16);
>> +				break;
>> +			case V4L2_CTRL_TYPE_U32:
>> +				controls->set(v4l2_ctrl->id, size,
>> +					      v4l2_ctrl->p_u32);
>> +				break;
>> +			default:
>> +				LOG(V4L2Base, Error)
>> +					<< "Compound control types not supported: "
>> +					<< std::hex << qext_ctrl.type;
>> +				return -EINVAL;
>> +			}
>> +		} else {
>> +			/* Control types without payload. */
>> +			switch (qext_ctrl.type) {
>> +			case V4L2_CTRL_TYPE_INTEGER64:
>> +				controls->set(v4l2_ctrl->id, static_cast<int64_t>
>> +					     (v4l2_ctrl->value64));
>> +				break;
>> +			case V4L2_CTRL_TYPE_INTEGER:
>> +			case V4L2_CTRL_TYPE_BOOLEAN:
>> +			case V4L2_CTRL_TYPE_MENU:
>> +			case V4L2_CTRL_TYPE_BUTTON:
>> +				controls->set(v4l2_ctrl->id, v4l2_ctrl->value);
>> +				break;
>> +			default:
>> +				LOG(V4L2Base, Error)
>> +					<< "Invalid non-compound control type :"
>> +					<< std::hex << qext_ctrl.type;
>> +				return -EINVAL;
>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * \brief Apply a list of controls to the device
>> + * \param ctrls The list of controls to apply
>> + * \return 0 on success or a negative error code otherwise
>> + */
>> +int V4L2Base::setControls(V4L2Controls &ctrls)
>> +{
>> +	unsigned int count = ctrls.size();
>> +	if (!count)
>> +		return -EINVAL;
>> +
>> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
>> +
>> +	/*
>> +	 * Query each control in the vector to get back its type and expected
>> +	 * size and set the new desired value in each v4l2_ext_control member.
>> +	 */
>> +	for (unsigned int i = 0; i < count; ++i) {
>> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
>> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
>> +		V4L2Control *ctrl = ctrls[i];
>> +
>> +		int ret = queryControl(ctrl->id(), &qext_ctrl);
>> +		if (ret)
>> +			return ret;
>> +
>> +		v4l2_ctrl->id = ctrl->id();
>> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
>> +
>> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
>> +			/** \todo: Support set controls with payload. */
>> +			LOG(V4L2Base, Error)
>> +				<< "Controls with payload not supported";
>> +			return -EINVAL;
>> +		} else {

I think only supporting the types below would help accelerate this
series and get it integrated faster, which will help unblock other
development paths.



>> +			switch (qext_ctrl.type) {
>> +			case V4L2_CTRL_TYPE_INTEGER64:
>> +			{
>> +				V4L2Int64Control *c =
>> +					static_cast<V4L2Int64Control *>(ctrl);
>> +				v4l2_ctrl->value64 = c->value();
>> +			}
>> +				break;
>> +			case V4L2_CTRL_TYPE_INTEGER:
>> +			case V4L2_CTRL_TYPE_BOOLEAN:
>> +			case V4L2_CTRL_TYPE_MENU:
>> +			case V4L2_CTRL_TYPE_BUTTON:
>> +			{
>> +				V4L2IntControl *c =
>> +					static_cast<V4L2IntControl *>(ctrl);
>> +				v4l2_ctrl->value = c->value();
>> +			}
>> +				break;
>> +			default:
>> +				LOG(V4L2Base, Error)
>> +					<< "Invalid non-compound control type :"
>> +					<< qext_ctrl.type;
>> +				return -EINVAL;
>> +			}
>> +		}
>> +	}
>> +
>> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
>> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
>> +	v4l2_ext_ctrls.count = count;
>> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
>> +
>> +	int ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);
>> +	if (ret) {
>> +		ret = -errno;
>> +		LOG(V4L2Base, Error)
>> +			<< "Unable to set controls: " << strerror(-ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * \var V4L2Base::fd_
>>   * \brief The V4L2 device or subdevice device node file descriptor
>

Hi Kieran,

On Tue, Jun 11, 2019 at 12:55:07PM +0100, Kieran Bingham wrote:
> On 11/06/2019 12:41, Laurent Pinchart wrote:
> > On Mon, Jun 10, 2019 at 06:40:50PM +0200, Jacopo Mondi wrote:
> >> Implement operations to set and get V4L2 controls in the V4L2Base class.
> >> The operations allow to set and get a single or a list of controls.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  src/libcamera/include/v4l2_base.h |  11 ++
> >>  src/libcamera/v4l2_base.cpp       | 238 ++++++++++++++++++++++++++++++
> >>  2 files changed, 249 insertions(+)
> >>
> >> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h
> >> index 2fda81a960d2..17ea734c8f49 100644
> >> --- a/src/libcamera/include/v4l2_base.h
> >> +++ b/src/libcamera/include/v4l2_base.h
> >> @@ -9,6 +9,8 @@
> >>  
> >>  #include <vector>
> >>  
> >> +#include <v4l2_controls.h>
> > 
> > Just forward-declare the required classes and structures.
> > 
> >> +
> >>  namespace libcamera {
> >>  
> >>  class V4L2Base
> >> @@ -22,8 +24,17 @@ public:
> >>  	virtual void close() = 0;
> >>  	bool isOpen() const;
> >>  
> >> +	int getControls(std::vector<unsigned int> &ctrl_ids,
> >> +			V4L2Controls *ctrls);
> > 
> > 	const std::vector<unsigned int> &ctrl_ids,
> > 
> > I still wonder if it wouldn't be simpler for applications to fill the
> > V4L2Controls with the control IDs to read, and just pass the
> > V4L2Controls pointer to this function. In any case, this is better than
> > the first version.
> 
> Remembering that 'applications' do not use or see these types of course.
> 
> Only Pipelinehandlers should every see or use these objects.

Yes, that's what I meant, sorry.

> >> +	int setControls(V4L2Controls &ctrls);
> > 
> > You should pass a const V4L2Controls &ctrls. Or, maybe you should
> > instead pass a non-const pointer, as setControls() should return the
> > value set for each control.
> > 
> >> +
> >>  protected:
> >>  	int fd_;
> >> +
> >> +private:
> >> +	int queryControl(unsigned int ctrl_id,
> >> +			 struct v4l2_query_ext_ctrl *qext_ctrl);
> >> +
> >>  };
> >>  
> >>  }; /* namespace libcamera */
> >> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp
> >> index 7d05a3c82e4d..cbd7a551130f 100644
> >> --- a/src/libcamera/v4l2_base.cpp
> >> +++ b/src/libcamera/v4l2_base.cpp
> >> @@ -5,7 +5,13 @@
> >>   * v4l2_base.cpp - Common base for V4L2 devices and subdevices
> >>   */
> >>  
> >> +#include <linux/v4l2-subdev.h>
> >> +#include <linux/videodev2.h>
> >> +#include <sys/ioctl.h>
> >> +
> >> +#include "log.h"
> >>  #include "v4l2_base.h"
> >> +#include "v4l2_controls.h"
> >>  
> >>  /**
> >>   * \file v4l2_base.h
> >> @@ -14,6 +20,8 @@
> >>  
> >>  namespace libcamera {
> >>  
> >> +LOG_DEFINE_CATEGORY(V4L2Base)
> >> +
> >>  /**
> >>   * \class V4L2Base
> >>   * \brief Base class for V4L2Device and V4L2Subdevice
> >> @@ -31,6 +39,7 @@ namespace libcamera {
> >>   */
> >>  
> >>  /**
> >> + * \fn V4L2Base::open()
> >>   * \brief Open a V4L2 device or subdevice
> >>   *
> >>   * Pure virtual method to be implemented by the derived classes.
> >> @@ -39,6 +48,7 @@ namespace libcamera {
> >>   */
> >>  
> >>  /**
> >> + * \fn V4L2Base::close()
> >>   * \brief Close the device or subdevice
> >>   *
> >>   * Pure virtual method to be implemented by the derived classes.
> >> @@ -53,6 +63,234 @@ bool V4L2Base::isOpen() const
> >>  	return fd_ != -1;
> >>  }
> >>  
> >> +int V4L2Base::queryControl(unsigned int id,
> >> +			   struct v4l2_query_ext_ctrl *qext_ctrl)
> >> +{
> >> +	qext_ctrl->id = id;
> >> +
> >> +	int ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);
> >> +	if (ret) {
> >> +		ret = -errno;
> >> +		LOG(V4L2Base, Error)
> >> +			<< "Unable to query extended control 0x"
> >> +			<< std::hex << qext_ctrl->id
> >> +			<< ": " << strerror(-ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	if (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
> >> +		LOG(V4L2Base, Error)
> >> +			<< "Extended control 0x" << std::hex
> >> +			<< qext_ctrl->id << " is disabled";
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * \brief Get values of a list of control IDs
> >> + * \param ctrl_ids The list of control IDs to retrieve value of
> >> + * \return A list of V4L2Control on success or a empty list otherwise
> >> + */
> >> +int V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids,
> >> +			  V4L2Controls *controls)
> >> +{
> >> +	unsigned int count = ctrl_ids.size();
> >> +	if (!count)
> >> +		return -EINVAL;
> >> +
> >> +	controls->clear();
> >> +
> >> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> > 
> > You're leaking memory. The simplest solution would be
> > 
> > 	struct v4l2_ext_controls v4l2_ctrls[count];
> > 
> >> +
> >> +	/*
> >> +	 * Query each control in the vector to get back its type and expected
> >> +	 * size in order to get its current value.
> >> +	 */
> >> +	for (unsigned int i = 0; i < count; ++i) {
> >> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> >> +		unsigned int ctrl_id = ctrl_ids[i];
> >> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> >> +
> >> +		int ret = queryControl(ctrl_id, &qext_ctrl);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		v4l2_ctrl->id = ctrl_id;
> >> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> >> +
> >> +		/* Reserve memory for controls with payload. */
> >> +		if (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))
> >> +			continue;
> >> +
> >> +		switch (qext_ctrl.type) {
> >> +		case V4L2_CTRL_TYPE_U8:
> >> +			v4l2_ctrl->p_u8 = static_cast<uint8_t *>
> >> +					  (new uint8_t[v4l2_ctrl->size]);
> >> +			break;
> >> +		case V4L2_CTRL_TYPE_U16:
> >> +			v4l2_ctrl->p_u16 = static_cast<uint16_t *>
> >> +					  (new uint16_t[v4l2_ctrl->size]);
> >> +			break;
> >> +		case V4L2_CTRL_TYPE_U32:
> >> +			v4l2_ctrl->p_u32 = static_cast<uint32_t *>
> >> +					  (new uint32_t[v4l2_ctrl->size]);
> >> +			break;
> > 
> > This is lots of new's, and they're all leaked. A better solution would
> > be to populate ctrls with the V4L2Control instances already, and use the
> > pointer to the internal memory buffers directly.
> 
> We're doing new's here with v4l2_ctrl->size. Does that imply that these
> are payload control values ?
> 
> In fact - yes the statement above says so.
> 
> I think if the pipeline handler wants to read or set a 'payload' then it
> should be responsible for providing that memory - otherwise we will have
> more copying involved.

The V4L2ControlValue derived classes allocate memory internally for the
payload, so I think we should use that instead of copying.

> Perhaps we should just drop support for payload controls for now until
> we actually hit a need to use it?

That's one option.

> Or do you know taht you'll need it for IPU3?

Not to my knowledge.

> >> +		}
> >> +	}
> >> +
> >> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> >> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> >> +	v4l2_ext_ctrls.count = count;
> >> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> >> +
> >> +	int ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);
> >> +	if (ret) {
> >> +		ret = -errno;
> >> +		LOG(V4L2Base, Error)
> >> +			<< "Unable to get extended controls: " << strerror(-ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	/*
> >> +	 * For each requested control, create a V4L2Control which contains
> >> +	 * the current value and store it in the vector returned to the caller.
> >> +	 */
> >> +	LOG(Error) << __func__;
> >> +	for (unsigned int i = 0; i < count; ++i) {
> >> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> >> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> >> +
> >> +		int ret = queryControl(v4l2_ctrl->id, &qext_ctrl);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		unsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;
> >> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> >> +			/* Control types with payload */
> >> +			switch (qext_ctrl.type) {
> >> +			case V4L2_CTRL_TYPE_U8:
> >> +				controls->set(v4l2_ctrl->id, size,
> >> +					      v4l2_ctrl->p_u8);
> >> +				break;
> >> +			case V4L2_CTRL_TYPE_U16:
> >> +				controls->set(v4l2_ctrl->id, size,
> >> +					      v4l2_ctrl->p_u16);
> >> +				break;
> >> +			case V4L2_CTRL_TYPE_U32:
> >> +				controls->set(v4l2_ctrl->id, size,
> >> +					      v4l2_ctrl->p_u32);
> >> +				break;
> >> +			default:
> >> +				LOG(V4L2Base, Error)
> >> +					<< "Compound control types not supported: "
> >> +					<< std::hex << qext_ctrl.type;
> >> +				return -EINVAL;
> >> +			}
> >> +		} else {
> >> +			/* Control types without payload. */
> >> +			switch (qext_ctrl.type) {
> >> +			case V4L2_CTRL_TYPE_INTEGER64:
> >> +				controls->set(v4l2_ctrl->id, static_cast<int64_t>
> >> +					     (v4l2_ctrl->value64));
> >> +				break;
> >> +			case V4L2_CTRL_TYPE_INTEGER:
> >> +			case V4L2_CTRL_TYPE_BOOLEAN:
> >> +			case V4L2_CTRL_TYPE_MENU:
> >> +			case V4L2_CTRL_TYPE_BUTTON:
> >> +				controls->set(v4l2_ctrl->id, v4l2_ctrl->value);
> >> +				break;
> >> +			default:
> >> +				LOG(V4L2Base, Error)
> >> +					<< "Invalid non-compound control type :"
> >> +					<< std::hex << qext_ctrl.type;
> >> +				return -EINVAL;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * \brief Apply a list of controls to the device
> >> + * \param ctrls The list of controls to apply
> >> + * \return 0 on success or a negative error code otherwise
> >> + */
> >> +int V4L2Base::setControls(V4L2Controls &ctrls)
> >> +{
> >> +	unsigned int count = ctrls.size();
> >> +	if (!count)
> >> +		return -EINVAL;
> >> +
> >> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> >> +
> >> +	/*
> >> +	 * Query each control in the vector to get back its type and expected
> >> +	 * size and set the new desired value in each v4l2_ext_control member.
> >> +	 */
> >> +	for (unsigned int i = 0; i < count; ++i) {
> >> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> >> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> >> +		V4L2Control *ctrl = ctrls[i];
> >> +
> >> +		int ret = queryControl(ctrl->id(), &qext_ctrl);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		v4l2_ctrl->id = ctrl->id();
> >> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> >> +
> >> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> >> +			/** \todo: Support set controls with payload. */
> >> +			LOG(V4L2Base, Error)
> >> +				<< "Controls with payload not supported";
> >> +			return -EINVAL;
> >> +		} else {
> 
> I think only supporting the types below would help accelerate this
> series and get it integrated faster, which will help unblock other
> development paths.
> 
> >> +			switch (qext_ctrl.type) {
> >> +			case V4L2_CTRL_TYPE_INTEGER64:
> >> +			{
> >> +				V4L2Int64Control *c =
> >> +					static_cast<V4L2Int64Control *>(ctrl);
> >> +				v4l2_ctrl->value64 = c->value();
> >> +			}
> >> +				break;
> >> +			case V4L2_CTRL_TYPE_INTEGER:
> >> +			case V4L2_CTRL_TYPE_BOOLEAN:
> >> +			case V4L2_CTRL_TYPE_MENU:
> >> +			case V4L2_CTRL_TYPE_BUTTON:
> >> +			{
> >> +				V4L2IntControl *c =
> >> +					static_cast<V4L2IntControl *>(ctrl);
> >> +				v4l2_ctrl->value = c->value();
> >> +			}
> >> +				break;
> >> +			default:
> >> +				LOG(V4L2Base, Error)
> >> +					<< "Invalid non-compound control type :"
> >> +					<< qext_ctrl.type;
> >> +				return -EINVAL;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> >> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> >> +	v4l2_ext_ctrls.count = count;
> >> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> >> +
> >> +	int ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);
> >> +	if (ret) {
> >> +		ret = -errno;
> >> +		LOG(V4L2Base, Error)
> >> +			<< "Unable to set controls: " << strerror(-ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  /**
> >>   * \var V4L2Base::fd_
> >>   * \brief The V4L2 device or subdevice device node file descriptor

Hi Laurent,

On Tue, Jun 11, 2019 at 02:41:26PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Jun 10, 2019 at 06:40:50PM +0200, Jacopo Mondi wrote:
> > Implement operations to set and get V4L2 controls in the V4L2Base class.
> > The operations allow to set and get a single or a list of controls.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/include/v4l2_base.h |  11 ++
> >  src/libcamera/v4l2_base.cpp       | 238 ++++++++++++++++++++++++++++++
> >  2 files changed, 249 insertions(+)
> >
> > diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h
> > index 2fda81a960d2..17ea734c8f49 100644
> > --- a/src/libcamera/include/v4l2_base.h
> > +++ b/src/libcamera/include/v4l2_base.h
> > @@ -9,6 +9,8 @@
> >
> >  #include <vector>
> >
> > +#include <v4l2_controls.h>
>
> Just forward-declare the required classes and structures.
>
> > +
> >  namespace libcamera {
> >
> >  class V4L2Base
> > @@ -22,8 +24,17 @@ public:
> >  	virtual void close() = 0;
> >  	bool isOpen() const;
> >
> > +	int getControls(std::vector<unsigned int> &ctrl_ids,
> > +			V4L2Controls *ctrls);
>
> 	const std::vector<unsigned int> &ctrl_ids,
>
> I still wonder if it wouldn't be simpler for applications to fill the
> V4L2Controls with the control IDs to read, and just pass the
> V4L2Controls pointer to this function. In any case, this is better than
> the first version.
>

I thought about that, but I don't like the fact that providing
controls without a value would require to keep track of the state of a
control (initialized? empty? with a default value? read from the HW?).

In this way, we know that the V4L2Controls instance will contain
controls with a value read from the hardware, as well as the one
provided to setControl will contain values set by the user and applied
to the HW. It's much easier for me not having to keep track of the
control state (to prevent, say, reading a control without a value
assigned). I can change this by popular demand though. No issues.


> > +	int setControls(V4L2Controls &ctrls);
>
> You should pass a const V4L2Controls &ctrls. Or, maybe you should
> instead pass a non-const pointer, as setControls() should return the
> value set for each control.

Not sure about const, the value of the single V4L2Control should be
updated to reflect what is applied to the HW.

>
> > +
> >  protected:
> >  	int fd_;
> > +
> > +private:
> > +	int queryControl(unsigned int ctrl_id,
> > +			 struct v4l2_query_ext_ctrl *qext_ctrl);
> > +
> >  };
> >
> >  }; /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp
> > index 7d05a3c82e4d..cbd7a551130f 100644
> > --- a/src/libcamera/v4l2_base.cpp
> > +++ b/src/libcamera/v4l2_base.cpp
> > @@ -5,7 +5,13 @@
> >   * v4l2_base.cpp - Common base for V4L2 devices and subdevices
> >   */
> >
> > +#include <linux/v4l2-subdev.h>
> > +#include <linux/videodev2.h>
> > +#include <sys/ioctl.h>
> > +
> > +#include "log.h"
> >  #include "v4l2_base.h"
> > +#include "v4l2_controls.h"
> >
> >  /**
> >   * \file v4l2_base.h
> > @@ -14,6 +20,8 @@
> >
> >  namespace libcamera {
> >
> > +LOG_DEFINE_CATEGORY(V4L2Base)
> > +
> >  /**
> >   * \class V4L2Base
> >   * \brief Base class for V4L2Device and V4L2Subdevice
> > @@ -31,6 +39,7 @@ namespace libcamera {
> >   */
> >
> >  /**
> > + * \fn V4L2Base::open()
> >   * \brief Open a V4L2 device or subdevice
> >   *
> >   * Pure virtual method to be implemented by the derived classes.
> > @@ -39,6 +48,7 @@ namespace libcamera {
> >   */
> >
> >  /**
> > + * \fn V4L2Base::close()
> >   * \brief Close the device or subdevice
> >   *
> >   * Pure virtual method to be implemented by the derived classes.
> > @@ -53,6 +63,234 @@ bool V4L2Base::isOpen() const
> >  	return fd_ != -1;
> >  }
> >
> > +int V4L2Base::queryControl(unsigned int id,
> > +			   struct v4l2_query_ext_ctrl *qext_ctrl)
> > +{
> > +	qext_ctrl->id = id;
> > +
> > +	int ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);
> > +	if (ret) {
> > +		ret = -errno;
> > +		LOG(V4L2Base, Error)
> > +			<< "Unable to query extended control 0x"
> > +			<< std::hex << qext_ctrl->id
> > +			<< ": " << strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	if (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
> > +		LOG(V4L2Base, Error)
> > +			<< "Extended control 0x" << std::hex
> > +			<< qext_ctrl->id << " is disabled";
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \brief Get values of a list of control IDs
> > + * \param ctrl_ids The list of control IDs to retrieve value of
> > + * \return A list of V4L2Control on success or a empty list otherwise
> > + */
> > +int V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids,
> > +			  V4L2Controls *controls)
> > +{
> > +	unsigned int count = ctrl_ids.size();
> > +	if (!count)
> > +		return -EINVAL;
> > +
> > +	controls->clear();
> > +
> > +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
>
> You're leaking memory. The simplest solution would be
>
> 	struct v4l2_ext_controls v4l2_ctrls[count];

Indeed!
I wonder why valgrind reported no leaks...

>
> > +
> > +	/*
> > +	 * Query each control in the vector to get back its type and expected
> > +	 * size in order to get its current value.
> > +	 */
> > +	for (unsigned int i = 0; i < count; ++i) {
> > +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > +		unsigned int ctrl_id = ctrl_ids[i];
> > +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> > +
> > +		int ret = queryControl(ctrl_id, &qext_ctrl);
> > +		if (ret)
> > +			return ret;
> > +
> > +		v4l2_ctrl->id = ctrl_id;
> > +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> > +
> > +		/* Reserve memory for controls with payload. */
> > +		if (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))
> > +			continue;
> > +
> > +		switch (qext_ctrl.type) {
> > +		case V4L2_CTRL_TYPE_U8:
> > +			v4l2_ctrl->p_u8 = static_cast<uint8_t *>
> > +					  (new uint8_t[v4l2_ctrl->size]);
> > +			break;
> > +		case V4L2_CTRL_TYPE_U16:
> > +			v4l2_ctrl->p_u16 = static_cast<uint16_t *>
> > +					  (new uint16_t[v4l2_ctrl->size]);
> > +			break;
> > +		case V4L2_CTRL_TYPE_U32:
> > +			v4l2_ctrl->p_u32 = static_cast<uint32_t *>
> > +					  (new uint32_t[v4l2_ctrl->size]);
> > +			break;
>
> This is lots of new's, and they're all leaked. A better solution would
> be to populate ctrls with the V4L2Control instances already, and use the
> pointer to the internal memory buffers directly.

Sorry, I didn't get you, what's 'ctrls' here?

The memory here allocated (and not released, my bad) serves to the
kernel to copy the data payload to userspace. It is then copied inside
the V4L2Control, where memory is reserved at creation time.

What I could do, and I think that's what you suggested is to create
the V4L2Control in advance (once I know the required memory size) and
point the 'struct v4l2_ext_control' data pointer to that memory, and
save one copy.

Personally, I'm also fine dropping support for payload controls for
this first version, as far as I could tell, there's no strict
requirement for any of them on IPU3 (as Kieran suggested).

What's your opinion?

>
> > +		}
> > +	}
> > +
> > +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> > +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> > +	v4l2_ext_ctrls.count = count;
> > +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> > +
> > +	int ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);
> > +	if (ret) {
> > +		ret = -errno;
> > +		LOG(V4L2Base, Error)
> > +			<< "Unable to get extended controls: " << strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * For each requested control, create a V4L2Control which contains
> > +	 * the current value and store it in the vector returned to the caller.
> > +	 */
> > +	LOG(Error) << __func__;
> > +	for (unsigned int i = 0; i < count; ++i) {
> > +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> > +
> > +		int ret = queryControl(v4l2_ctrl->id, &qext_ctrl);
> > +		if (ret)
> > +			return ret;
> > +
> > +		unsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;
> > +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> > +			/* Control types with payload */
> > +			switch (qext_ctrl.type) {
> > +			case V4L2_CTRL_TYPE_U8:
> > +				controls->set(v4l2_ctrl->id, size,
> > +					      v4l2_ctrl->p_u8);
> > +				break;
> > +			case V4L2_CTRL_TYPE_U16:
> > +				controls->set(v4l2_ctrl->id, size,
> > +					      v4l2_ctrl->p_u16);
> > +				break;
> > +			case V4L2_CTRL_TYPE_U32:
> > +				controls->set(v4l2_ctrl->id, size,
> > +					      v4l2_ctrl->p_u32);
> > +				break;
> > +			default:
> > +				LOG(V4L2Base, Error)
> > +					<< "Compound control types not supported: "
> > +					<< std::hex << qext_ctrl.type;
> > +				return -EINVAL;
> > +			}
> > +		} else {
> > +			/* Control types without payload. */
> > +			switch (qext_ctrl.type) {
> > +			case V4L2_CTRL_TYPE_INTEGER64:
> > +				controls->set(v4l2_ctrl->id, static_cast<int64_t>
> > +					     (v4l2_ctrl->value64));
> > +				break;
> > +			case V4L2_CTRL_TYPE_INTEGER:
> > +			case V4L2_CTRL_TYPE_BOOLEAN:
> > +			case V4L2_CTRL_TYPE_MENU:
> > +			case V4L2_CTRL_TYPE_BUTTON:
> > +				controls->set(v4l2_ctrl->id, v4l2_ctrl->value);
> > +				break;
> > +			default:
> > +				LOG(V4L2Base, Error)
> > +					<< "Invalid non-compound control type :"
> > +					<< std::hex << qext_ctrl.type;
> > +				return -EINVAL;
> > +			}
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \brief Apply a list of controls to the device
> > + * \param ctrls The list of controls to apply
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int V4L2Base::setControls(V4L2Controls &ctrls)
> > +{
> > +	unsigned int count = ctrls.size();
> > +	if (!count)
> > +		return -EINVAL;
> > +
> > +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> > +
> > +	/*
> > +	 * Query each control in the vector to get back its type and expected
> > +	 * size and set the new desired value in each v4l2_ext_control member.
> > +	 */
> > +	for (unsigned int i = 0; i < count; ++i) {
> > +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> > +		V4L2Control *ctrl = ctrls[i];
> > +
> > +		int ret = queryControl(ctrl->id(), &qext_ctrl);
> > +		if (ret)
> > +			return ret;
> > +
> > +		v4l2_ctrl->id = ctrl->id();
> > +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> > +
> > +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> > +			/** \todo: Support set controls with payload. */
> > +			LOG(V4L2Base, Error)
> > +				<< "Controls with payload not supported";
> > +			return -EINVAL;
> > +		} else {
> > +			switch (qext_ctrl.type) {
> > +			case V4L2_CTRL_TYPE_INTEGER64:
> > +			{
> > +				V4L2Int64Control *c =
> > +					static_cast<V4L2Int64Control *>(ctrl);
> > +				v4l2_ctrl->value64 = c->value();
> > +			}
> > +				break;
> > +			case V4L2_CTRL_TYPE_INTEGER:
> > +			case V4L2_CTRL_TYPE_BOOLEAN:
> > +			case V4L2_CTRL_TYPE_MENU:
> > +			case V4L2_CTRL_TYPE_BUTTON:
> > +			{
> > +				V4L2IntControl *c =
> > +					static_cast<V4L2IntControl *>(ctrl);
> > +				v4l2_ctrl->value = c->value();
> > +			}
> > +				break;
> > +			default:
> > +				LOG(V4L2Base, Error)
> > +					<< "Invalid non-compound control type :"
> > +					<< qext_ctrl.type;
> > +				return -EINVAL;
> > +			}
> > +		}
> > +	}
> > +
> > +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> > +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> > +	v4l2_ext_ctrls.count = count;
> > +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> > +
> > +	int ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);
> > +	if (ret) {
> > +		ret = -errno;
> > +		LOG(V4L2Base, Error)
> > +			<< "Unable to set controls: " << strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * \var V4L2Base::fd_
> >   * \brief The V4L2 device or subdevice device node file descriptor
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Tue, Jun 11, 2019 at 03:10:21PM +0200, Jacopo Mondi wrote:
> On Tue, Jun 11, 2019 at 02:41:26PM +0300, Laurent Pinchart wrote:
> > On Mon, Jun 10, 2019 at 06:40:50PM +0200, Jacopo Mondi wrote:
> >> Implement operations to set and get V4L2 controls in the V4L2Base class.
> >> The operations allow to set and get a single or a list of controls.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  src/libcamera/include/v4l2_base.h |  11 ++
> >>  src/libcamera/v4l2_base.cpp       | 238 ++++++++++++++++++++++++++++++
> >>  2 files changed, 249 insertions(+)
> >>
> >> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h
> >> index 2fda81a960d2..17ea734c8f49 100644
> >> --- a/src/libcamera/include/v4l2_base.h
> >> +++ b/src/libcamera/include/v4l2_base.h
> >> @@ -9,6 +9,8 @@
> >>
> >>  #include <vector>
> >>
> >> +#include <v4l2_controls.h>
> >
> > Just forward-declare the required classes and structures.
> >
> >> +
> >>  namespace libcamera {
> >>
> >>  class V4L2Base
> >> @@ -22,8 +24,17 @@ public:
> >>  	virtual void close() = 0;
> >>  	bool isOpen() const;
> >>
> >> +	int getControls(std::vector<unsigned int> &ctrl_ids,
> >> +			V4L2Controls *ctrls);
> >
> > 	const std::vector<unsigned int> &ctrl_ids,
> >
> > I still wonder if it wouldn't be simpler for applications to fill the
> > V4L2Controls with the control IDs to read, and just pass the
> > V4L2Controls pointer to this function. In any case, this is better than
> > the first version.
> 
> I thought about that, but I don't like the fact that providing
> controls without a value would require to keep track of the state of a
> control (initialized? empty? with a default value? read from the HW?).
> 
> In this way, we know that the V4L2Controls instance will contain
> controls with a value read from the hardware, as well as the one
> provided to setControl will contain values set by the user and applied
> to the HW. It's much easier for me not having to keep track of the
> control state (to prevent, say, reading a control without a value
> assigned). I can change this by popular demand though. No issues.

I won't push too hard :-) But I'm wondering what your real concern is
here. Are you worried that an application will create a V4L2Controls
instance, populate it with controls to be set and pass it to
V4L2Base::setControls(), and then later pass the same instance to
V4L2Base::getControls() ?

> >> +	int setControls(V4L2Controls &ctrls);
> >
> > You should pass a const V4L2Controls &ctrls. Or, maybe you should
> > instead pass a non-const pointer, as setControls() should return the
> > value set for each control.
> 
> Not sure about const, the value of the single V4L2Control should be
> updated to reflect what is applied to the HW.

I agree, hence the second sentence :-) I would then pass the
V4L2Controls by pointer to reflect that.

> >> +
> >>  protected:
> >>  	int fd_;
> >> +
> >> +private:
> >> +	int queryControl(unsigned int ctrl_id,
> >> +			 struct v4l2_query_ext_ctrl *qext_ctrl);
> >> +
> >>  };
> >>
> >>  }; /* namespace libcamera */
> >> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp
> >> index 7d05a3c82e4d..cbd7a551130f 100644
> >> --- a/src/libcamera/v4l2_base.cpp
> >> +++ b/src/libcamera/v4l2_base.cpp
> >> @@ -5,7 +5,13 @@
> >>   * v4l2_base.cpp - Common base for V4L2 devices and subdevices
> >>   */
> >>
> >> +#include <linux/v4l2-subdev.h>
> >> +#include <linux/videodev2.h>
> >> +#include <sys/ioctl.h>
> >> +
> >> +#include "log.h"
> >>  #include "v4l2_base.h"
> >> +#include "v4l2_controls.h"
> >>
> >>  /**
> >>   * \file v4l2_base.h
> >> @@ -14,6 +20,8 @@
> >>
> >>  namespace libcamera {
> >>
> >> +LOG_DEFINE_CATEGORY(V4L2Base)
> >> +
> >>  /**
> >>   * \class V4L2Base
> >>   * \brief Base class for V4L2Device and V4L2Subdevice
> >> @@ -31,6 +39,7 @@ namespace libcamera {
> >>   */
> >>
> >>  /**
> >> + * \fn V4L2Base::open()
> >>   * \brief Open a V4L2 device or subdevice
> >>   *
> >>   * Pure virtual method to be implemented by the derived classes.
> >> @@ -39,6 +48,7 @@ namespace libcamera {
> >>   */
> >>
> >>  /**
> >> + * \fn V4L2Base::close()
> >>   * \brief Close the device or subdevice
> >>   *
> >>   * Pure virtual method to be implemented by the derived classes.
> >> @@ -53,6 +63,234 @@ bool V4L2Base::isOpen() const
> >>  	return fd_ != -1;
> >>  }
> >>
> >> +int V4L2Base::queryControl(unsigned int id,
> >> +			   struct v4l2_query_ext_ctrl *qext_ctrl)
> >> +{
> >> +	qext_ctrl->id = id;
> >> +
> >> +	int ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);
> >> +	if (ret) {
> >> +		ret = -errno;
> >> +		LOG(V4L2Base, Error)
> >> +			<< "Unable to query extended control 0x"
> >> +			<< std::hex << qext_ctrl->id
> >> +			<< ": " << strerror(-ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	if (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
> >> +		LOG(V4L2Base, Error)
> >> +			<< "Extended control 0x" << std::hex
> >> +			<< qext_ctrl->id << " is disabled";
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * \brief Get values of a list of control IDs
> >> + * \param ctrl_ids The list of control IDs to retrieve value of
> >> + * \return A list of V4L2Control on success or a empty list otherwise
> >> + */
> >> +int V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids,
> >> +			  V4L2Controls *controls)
> >> +{
> >> +	unsigned int count = ctrl_ids.size();
> >> +	if (!count)
> >> +		return -EINVAL;
> >> +
> >> +	controls->clear();
> >> +
> >> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> >
> > You're leaking memory. The simplest solution would be
> >
> > 	struct v4l2_ext_controls v4l2_ctrls[count];
> 
> Indeed!
> I wonder why valgrind reported no leaks...
> 
> >> +
> >> +	/*
> >> +	 * Query each control in the vector to get back its type and expected
> >> +	 * size in order to get its current value.
> >> +	 */
> >> +	for (unsigned int i = 0; i < count; ++i) {
> >> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> >> +		unsigned int ctrl_id = ctrl_ids[i];
> >> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> >> +
> >> +		int ret = queryControl(ctrl_id, &qext_ctrl);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		v4l2_ctrl->id = ctrl_id;
> >> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> >> +
> >> +		/* Reserve memory for controls with payload. */
> >> +		if (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))
> >> +			continue;
> >> +
> >> +		switch (qext_ctrl.type) {
> >> +		case V4L2_CTRL_TYPE_U8:
> >> +			v4l2_ctrl->p_u8 = static_cast<uint8_t *>
> >> +					  (new uint8_t[v4l2_ctrl->size]);
> >> +			break;
> >> +		case V4L2_CTRL_TYPE_U16:
> >> +			v4l2_ctrl->p_u16 = static_cast<uint16_t *>
> >> +					  (new uint16_t[v4l2_ctrl->size]);
> >> +			break;
> >> +		case V4L2_CTRL_TYPE_U32:
> >> +			v4l2_ctrl->p_u32 = static_cast<uint32_t *>
> >> +					  (new uint32_t[v4l2_ctrl->size]);
> >> +			break;
> >
> > This is lots of new's, and they're all leaked. A better solution would
> > be to populate ctrls with the V4L2Control instances already, and use the
> > pointer to the internal memory buffers directly.
> 
> Sorry, I didn't get you, what's 'ctrls' here?

Sorry, I meant the controls parameter.

> The memory here allocated (and not released, my bad) serves to the
> kernel to copy the data payload to userspace. It is then copied inside
> the V4L2Control, where memory is reserved at creation time.
> 
> What I could do, and I think that's what you suggested is to create
> the V4L2Control in advance (once I know the required memory size) and
> point the 'struct v4l2_ext_control' data pointer to that memory, and
> save one copy.

Correct, that's what I meant.

> Personally, I'm also fine dropping support for payload controls for
> this first version, as far as I could tell, there's no strict
> requirement for any of them on IPU3 (as Kieran suggested).
> 
> What's your opinion?

We have very few payload controls in mainline, so that could indeed be a
good idea. I would however experiment with creation of the V4L2Control
instance before calling VIDIOC_G_EXT_CTRLS to see if it would be an
option for a later support of compound controls.

> >> +		}
> >> +	}
> >> +
> >> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> >> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> >> +	v4l2_ext_ctrls.count = count;
> >> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> >> +
> >> +	int ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);
> >> +	if (ret) {
> >> +		ret = -errno;
> >> +		LOG(V4L2Base, Error)
> >> +			<< "Unable to get extended controls: " << strerror(-ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	/*
> >> +	 * For each requested control, create a V4L2Control which contains
> >> +	 * the current value and store it in the vector returned to the caller.
> >> +	 */
> >> +	LOG(Error) << __func__;
> >> +	for (unsigned int i = 0; i < count; ++i) {
> >> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> >> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> >> +
> >> +		int ret = queryControl(v4l2_ctrl->id, &qext_ctrl);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		unsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;
> >> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> >> +			/* Control types with payload */
> >> +			switch (qext_ctrl.type) {
> >> +			case V4L2_CTRL_TYPE_U8:
> >> +				controls->set(v4l2_ctrl->id, size,
> >> +					      v4l2_ctrl->p_u8);
> >> +				break;
> >> +			case V4L2_CTRL_TYPE_U16:
> >> +				controls->set(v4l2_ctrl->id, size,
> >> +					      v4l2_ctrl->p_u16);
> >> +				break;
> >> +			case V4L2_CTRL_TYPE_U32:
> >> +				controls->set(v4l2_ctrl->id, size,
> >> +					      v4l2_ctrl->p_u32);
> >> +				break;
> >> +			default:
> >> +				LOG(V4L2Base, Error)
> >> +					<< "Compound control types not supported: "
> >> +					<< std::hex << qext_ctrl.type;
> >> +				return -EINVAL;
> >> +			}
> >> +		} else {
> >> +			/* Control types without payload. */
> >> +			switch (qext_ctrl.type) {
> >> +			case V4L2_CTRL_TYPE_INTEGER64:
> >> +				controls->set(v4l2_ctrl->id, static_cast<int64_t>
> >> +					     (v4l2_ctrl->value64));
> >> +				break;
> >> +			case V4L2_CTRL_TYPE_INTEGER:
> >> +			case V4L2_CTRL_TYPE_BOOLEAN:
> >> +			case V4L2_CTRL_TYPE_MENU:
> >> +			case V4L2_CTRL_TYPE_BUTTON:
> >> +				controls->set(v4l2_ctrl->id, v4l2_ctrl->value);
> >> +				break;
> >> +			default:
> >> +				LOG(V4L2Base, Error)
> >> +					<< "Invalid non-compound control type :"
> >> +					<< std::hex << qext_ctrl.type;
> >> +				return -EINVAL;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * \brief Apply a list of controls to the device
> >> + * \param ctrls The list of controls to apply
> >> + * \return 0 on success or a negative error code otherwise
> >> + */
> >> +int V4L2Base::setControls(V4L2Controls &ctrls)
> >> +{
> >> +	unsigned int count = ctrls.size();
> >> +	if (!count)
> >> +		return -EINVAL;
> >> +
> >> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> >> +
> >> +	/*
> >> +	 * Query each control in the vector to get back its type and expected
> >> +	 * size and set the new desired value in each v4l2_ext_control member.
> >> +	 */
> >> +	for (unsigned int i = 0; i < count; ++i) {
> >> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> >> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> >> +		V4L2Control *ctrl = ctrls[i];
> >> +
> >> +		int ret = queryControl(ctrl->id(), &qext_ctrl);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		v4l2_ctrl->id = ctrl->id();
> >> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> >> +
> >> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> >> +			/** \todo: Support set controls with payload. */
> >> +			LOG(V4L2Base, Error)
> >> +				<< "Controls with payload not supported";
> >> +			return -EINVAL;
> >> +		} else {
> >> +			switch (qext_ctrl.type) {
> >> +			case V4L2_CTRL_TYPE_INTEGER64:
> >> +			{
> >> +				V4L2Int64Control *c =
> >> +					static_cast<V4L2Int64Control *>(ctrl);
> >> +				v4l2_ctrl->value64 = c->value();
> >> +			}
> >> +				break;
> >> +			case V4L2_CTRL_TYPE_INTEGER:
> >> +			case V4L2_CTRL_TYPE_BOOLEAN:
> >> +			case V4L2_CTRL_TYPE_MENU:
> >> +			case V4L2_CTRL_TYPE_BUTTON:
> >> +			{
> >> +				V4L2IntControl *c =
> >> +					static_cast<V4L2IntControl *>(ctrl);
> >> +				v4l2_ctrl->value = c->value();
> >> +			}
> >> +				break;
> >> +			default:
> >> +				LOG(V4L2Base, Error)
> >> +					<< "Invalid non-compound control type :"
> >> +					<< qext_ctrl.type;
> >> +				return -EINVAL;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> >> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> >> +	v4l2_ext_ctrls.count = count;
> >> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> >> +
> >> +	int ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);
> >> +	if (ret) {
> >> +		ret = -errno;
> >> +		LOG(V4L2Base, Error)
> >> +			<< "Unable to set controls: " << strerror(-ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  /**
> >>   * \var V4L2Base::fd_
> >>   * \brief The V4L2 device or subdevice device node file descriptor

HI Laurent,

On Tue, Jun 11, 2019 at 06:26:58PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Jun 11, 2019 at 03:10:21PM +0200, Jacopo Mondi wrote:
> > On Tue, Jun 11, 2019 at 02:41:26PM +0300, Laurent Pinchart wrote:
> > > On Mon, Jun 10, 2019 at 06:40:50PM +0200, Jacopo Mondi wrote:
> > >> Implement operations to set and get V4L2 controls in the V4L2Base class.
> > >> The operations allow to set and get a single or a list of controls.
> > >>
> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >> ---
> > >>  src/libcamera/include/v4l2_base.h |  11 ++
> > >>  src/libcamera/v4l2_base.cpp       | 238 ++++++++++++++++++++++++++++++
> > >>  2 files changed, 249 insertions(+)
> > >>
> > >> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h
> > >> index 2fda81a960d2..17ea734c8f49 100644
> > >> --- a/src/libcamera/include/v4l2_base.h
> > >> +++ b/src/libcamera/include/v4l2_base.h
> > >> @@ -9,6 +9,8 @@
> > >>
> > >>  #include <vector>
> > >>
> > >> +#include <v4l2_controls.h>
> > >
> > > Just forward-declare the required classes and structures.
> > >
> > >> +
> > >>  namespace libcamera {
> > >>
> > >>  class V4L2Base
> > >> @@ -22,8 +24,17 @@ public:
> > >>  	virtual void close() = 0;
> > >>  	bool isOpen() const;
> > >>
> > >> +	int getControls(std::vector<unsigned int> &ctrl_ids,
> > >> +			V4L2Controls *ctrls);
> > >
> > > 	const std::vector<unsigned int> &ctrl_ids,
> > >
> > > I still wonder if it wouldn't be simpler for applications to fill the
> > > V4L2Controls with the control IDs to read, and just pass the
> > > V4L2Controls pointer to this function. In any case, this is better than
> > > the first version.
> >
> > I thought about that, but I don't like the fact that providing
> > controls without a value would require to keep track of the state of a
> > control (initialized? empty? with a default value? read from the HW?).
> >
> > In this way, we know that the V4L2Controls instance will contain
> > controls with a value read from the hardware, as well as the one
> > provided to setControl will contain values set by the user and applied
> > to the HW. It's much easier for me not having to keep track of the
> > control state (to prevent, say, reading a control without a value
> > assigned). I can change this by popular demand though. No issues.
>
> I won't push too hard :-) But I'm wondering what your real concern is
> here. Are you worried that an application will create a V4L2Controls
> instance, populate it with controls to be set and pass it to
> V4L2Base::setControls(), and then later pass the same instance to
> V4L2Base::getControls() ?

What worries me is that application will create controls without a
value and controls with a value, and there is no distinction between
the two, so they could call a get$TYPE() and get back meaningless
values from it.

That calls for keeping track of the control state, or distinguish
read/write controls, which I fear will be cumbersome and unecessary
complicated. Am I worrying too much?

>
> > >> +	int setControls(V4L2Controls &ctrls);
> > >
> > > You should pass a const V4L2Controls &ctrls. Or, maybe you should
> > > instead pass a non-const pointer, as setControls() should return the
> > > value set for each control.
> >
> > Not sure about const, the value of the single V4L2Control should be
> > updated to reflect what is applied to the HW.
>
> I agree, hence the second sentence :-) I would then pass the
> V4L2Controls by pointer to reflect that.
>

Yes indeed.

> > >> +
> > >>  protected:
> > >>  	int fd_;
> > >> +
> > >> +private:
> > >> +	int queryControl(unsigned int ctrl_id,
> > >> +			 struct v4l2_query_ext_ctrl *qext_ctrl);
> > >> +
> > >>  };
> > >>
> > >>  }; /* namespace libcamera */
> > >> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp
> > >> index 7d05a3c82e4d..cbd7a551130f 100644
> > >> --- a/src/libcamera/v4l2_base.cpp
> > >> +++ b/src/libcamera/v4l2_base.cpp
> > >> @@ -5,7 +5,13 @@
> > >>   * v4l2_base.cpp - Common base for V4L2 devices and subdevices
> > >>   */
> > >>
> > >> +#include <linux/v4l2-subdev.h>
> > >> +#include <linux/videodev2.h>
> > >> +#include <sys/ioctl.h>
> > >> +
> > >> +#include "log.h"
> > >>  #include "v4l2_base.h"
> > >> +#include "v4l2_controls.h"
> > >>
> > >>  /**
> > >>   * \file v4l2_base.h
> > >> @@ -14,6 +20,8 @@
> > >>
> > >>  namespace libcamera {
> > >>
> > >> +LOG_DEFINE_CATEGORY(V4L2Base)
> > >> +
> > >>  /**
> > >>   * \class V4L2Base
> > >>   * \brief Base class for V4L2Device and V4L2Subdevice
> > >> @@ -31,6 +39,7 @@ namespace libcamera {
> > >>   */
> > >>
> > >>  /**
> > >> + * \fn V4L2Base::open()
> > >>   * \brief Open a V4L2 device or subdevice
> > >>   *
> > >>   * Pure virtual method to be implemented by the derived classes.
> > >> @@ -39,6 +48,7 @@ namespace libcamera {
> > >>   */
> > >>
> > >>  /**
> > >> + * \fn V4L2Base::close()
> > >>   * \brief Close the device or subdevice
> > >>   *
> > >>   * Pure virtual method to be implemented by the derived classes.
> > >> @@ -53,6 +63,234 @@ bool V4L2Base::isOpen() const
> > >>  	return fd_ != -1;
> > >>  }
> > >>
> > >> +int V4L2Base::queryControl(unsigned int id,
> > >> +			   struct v4l2_query_ext_ctrl *qext_ctrl)
> > >> +{
> > >> +	qext_ctrl->id = id;
> > >> +
> > >> +	int ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);
> > >> +	if (ret) {
> > >> +		ret = -errno;
> > >> +		LOG(V4L2Base, Error)
> > >> +			<< "Unable to query extended control 0x"
> > >> +			<< std::hex << qext_ctrl->id
> > >> +			<< ": " << strerror(-ret);
> > >> +		return ret;
> > >> +	}
> > >> +
> > >> +	if (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
> > >> +		LOG(V4L2Base, Error)
> > >> +			<< "Extended control 0x" << std::hex
> > >> +			<< qext_ctrl->id << " is disabled";
> > >> +		return -EINVAL;
> > >> +	}
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +/**
> > >> + * \brief Get values of a list of control IDs
> > >> + * \param ctrl_ids The list of control IDs to retrieve value of
> > >> + * \return A list of V4L2Control on success or a empty list otherwise
> > >> + */
> > >> +int V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids,
> > >> +			  V4L2Controls *controls)
> > >> +{
> > >> +	unsigned int count = ctrl_ids.size();
> > >> +	if (!count)
> > >> +		return -EINVAL;
> > >> +
> > >> +	controls->clear();
> > >> +
> > >> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> > >
> > > You're leaking memory. The simplest solution would be
> > >
> > > 	struct v4l2_ext_controls v4l2_ctrls[count];
> >
> > Indeed!
> > I wonder why valgrind reported no leaks...
> >
> > >> +
> > >> +	/*
> > >> +	 * Query each control in the vector to get back its type and expected
> > >> +	 * size in order to get its current value.
> > >> +	 */
> > >> +	for (unsigned int i = 0; i < count; ++i) {
> > >> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > >> +		unsigned int ctrl_id = ctrl_ids[i];
> > >> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> > >> +
> > >> +		int ret = queryControl(ctrl_id, &qext_ctrl);
> > >> +		if (ret)
> > >> +			return ret;
> > >> +
> > >> +		v4l2_ctrl->id = ctrl_id;
> > >> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> > >> +
> > >> +		/* Reserve memory for controls with payload. */
> > >> +		if (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))
> > >> +			continue;
> > >> +
> > >> +		switch (qext_ctrl.type) {
> > >> +		case V4L2_CTRL_TYPE_U8:
> > >> +			v4l2_ctrl->p_u8 = static_cast<uint8_t *>
> > >> +					  (new uint8_t[v4l2_ctrl->size]);
> > >> +			break;
> > >> +		case V4L2_CTRL_TYPE_U16:
> > >> +			v4l2_ctrl->p_u16 = static_cast<uint16_t *>
> > >> +					  (new uint16_t[v4l2_ctrl->size]);
> > >> +			break;
> > >> +		case V4L2_CTRL_TYPE_U32:
> > >> +			v4l2_ctrl->p_u32 = static_cast<uint32_t *>
> > >> +					  (new uint32_t[v4l2_ctrl->size]);
> > >> +			break;
> > >
> > > This is lots of new's, and they're all leaked. A better solution would
> > > be to populate ctrls with the V4L2Control instances already, and use the
> > > pointer to the internal memory buffers directly.
> >
> > Sorry, I didn't get you, what's 'ctrls' here?
>
> Sorry, I meant the controls parameter.
>
> > The memory here allocated (and not released, my bad) serves to the
> > kernel to copy the data payload to userspace. It is then copied inside
> > the V4L2Control, where memory is reserved at creation time.
> >
> > What I could do, and I think that's what you suggested is to create
> > the V4L2Control in advance (once I know the required memory size) and
> > point the 'struct v4l2_ext_control' data pointer to that memory, and
> > save one copy.
>
> Correct, that's what I meant.
>
> > Personally, I'm also fine dropping support for payload controls for
> > this first version, as far as I could tell, there's no strict
> > requirement for any of them on IPU3 (as Kieran suggested).
> >
> > What's your opinion?
>
> We have very few payload controls in mainline, so that could indeed be a
> good idea. I would however experiment with creation of the V4L2Control
> instance before calling VIDIOC_G_EXT_CTRLS to see if it would be an
> option for a later support of compound controls.
>

I'm a bit in two minds here:
If we drop support for controls with payload it calls for dropping the
template and use Kieran's ControlValue. But then, once we'll ever have
to support them, I fear the ControlValue class would be a waste of space
and less flexible to use with controls with payload...

It seems to me that dropping payload support and use ControlValue as
backing storage would make it easier to get the series in, so I'm
tempted to go for the "is good for now" way and do what is outlined
here above...


> > >> +		}
> > >> +	}
> > >> +
> > >> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> > >> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> > >> +	v4l2_ext_ctrls.count = count;
> > >> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> > >> +
> > >> +	int ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);
> > >> +	if (ret) {
> > >> +		ret = -errno;
> > >> +		LOG(V4L2Base, Error)
> > >> +			<< "Unable to get extended controls: " << strerror(-ret);
> > >> +		return ret;
> > >> +	}
> > >> +
> > >> +	/*
> > >> +	 * For each requested control, create a V4L2Control which contains
> > >> +	 * the current value and store it in the vector returned to the caller.
> > >> +	 */
> > >> +	LOG(Error) << __func__;
> > >> +	for (unsigned int i = 0; i < count; ++i) {
> > >> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > >> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> > >> +
> > >> +		int ret = queryControl(v4l2_ctrl->id, &qext_ctrl);
> > >> +		if (ret)
> > >> +			return ret;
> > >> +
> > >> +		unsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;
> > >> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> > >> +			/* Control types with payload */
> > >> +			switch (qext_ctrl.type) {
> > >> +			case V4L2_CTRL_TYPE_U8:
> > >> +				controls->set(v4l2_ctrl->id, size,
> > >> +					      v4l2_ctrl->p_u8);
> > >> +				break;
> > >> +			case V4L2_CTRL_TYPE_U16:
> > >> +				controls->set(v4l2_ctrl->id, size,
> > >> +					      v4l2_ctrl->p_u16);
> > >> +				break;
> > >> +			case V4L2_CTRL_TYPE_U32:
> > >> +				controls->set(v4l2_ctrl->id, size,
> > >> +					      v4l2_ctrl->p_u32);
> > >> +				break;
> > >> +			default:
> > >> +				LOG(V4L2Base, Error)
> > >> +					<< "Compound control types not supported: "
> > >> +					<< std::hex << qext_ctrl.type;
> > >> +				return -EINVAL;
> > >> +			}
> > >> +		} else {
> > >> +			/* Control types without payload. */
> > >> +			switch (qext_ctrl.type) {
> > >> +			case V4L2_CTRL_TYPE_INTEGER64:
> > >> +				controls->set(v4l2_ctrl->id, static_cast<int64_t>
> > >> +					     (v4l2_ctrl->value64));
> > >> +				break;
> > >> +			case V4L2_CTRL_TYPE_INTEGER:
> > >> +			case V4L2_CTRL_TYPE_BOOLEAN:
> > >> +			case V4L2_CTRL_TYPE_MENU:
> > >> +			case V4L2_CTRL_TYPE_BUTTON:
> > >> +				controls->set(v4l2_ctrl->id, v4l2_ctrl->value);
> > >> +				break;
> > >> +			default:
> > >> +				LOG(V4L2Base, Error)
> > >> +					<< "Invalid non-compound control type :"
> > >> +					<< std::hex << qext_ctrl.type;
> > >> +				return -EINVAL;
> > >> +			}
> > >> +		}
> > >> +	}
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +/**
> > >> + * \brief Apply a list of controls to the device
> > >> + * \param ctrls The list of controls to apply
> > >> + * \return 0 on success or a negative error code otherwise
> > >> + */
> > >> +int V4L2Base::setControls(V4L2Controls &ctrls)
> > >> +{
> > >> +	unsigned int count = ctrls.size();
> > >> +	if (!count)
> > >> +		return -EINVAL;
> > >> +
> > >> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> > >> +
> > >> +	/*
> > >> +	 * Query each control in the vector to get back its type and expected
> > >> +	 * size and set the new desired value in each v4l2_ext_control member.
> > >> +	 */
> > >> +	for (unsigned int i = 0; i < count; ++i) {
> > >> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > >> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> > >> +		V4L2Control *ctrl = ctrls[i];
> > >> +
> > >> +		int ret = queryControl(ctrl->id(), &qext_ctrl);
> > >> +		if (ret)
> > >> +			return ret;
> > >> +
> > >> +		v4l2_ctrl->id = ctrl->id();
> > >> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> > >> +
> > >> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> > >> +			/** \todo: Support set controls with payload. */
> > >> +			LOG(V4L2Base, Error)
> > >> +				<< "Controls with payload not supported";
> > >> +			return -EINVAL;
> > >> +		} else {
> > >> +			switch (qext_ctrl.type) {
> > >> +			case V4L2_CTRL_TYPE_INTEGER64:
> > >> +			{
> > >> +				V4L2Int64Control *c =
> > >> +					static_cast<V4L2Int64Control *>(ctrl);
> > >> +				v4l2_ctrl->value64 = c->value();
> > >> +			}
> > >> +				break;
> > >> +			case V4L2_CTRL_TYPE_INTEGER:
> > >> +			case V4L2_CTRL_TYPE_BOOLEAN:
> > >> +			case V4L2_CTRL_TYPE_MENU:
> > >> +			case V4L2_CTRL_TYPE_BUTTON:
> > >> +			{
> > >> +				V4L2IntControl *c =
> > >> +					static_cast<V4L2IntControl *>(ctrl);
> > >> +				v4l2_ctrl->value = c->value();
> > >> +			}
> > >> +				break;
> > >> +			default:
> > >> +				LOG(V4L2Base, Error)
> > >> +					<< "Invalid non-compound control type :"
> > >> +					<< qext_ctrl.type;
> > >> +				return -EINVAL;
> > >> +			}
> > >> +		}
> > >> +	}
> > >> +
> > >> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> > >> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> > >> +	v4l2_ext_ctrls.count = count;
> > >> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> > >> +
> > >> +	int ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);
> > >> +	if (ret) {
> > >> +		ret = -errno;
> > >> +		LOG(V4L2Base, Error)
> > >> +			<< "Unable to set controls: " << strerror(-ret);
> > >> +		return ret;
> > >> +	}
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >>  /**
> > >>   * \var V4L2Base::fd_
> > >>   * \brief The V4L2 device or subdevice device node file descriptor
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Tue, Jun 11, 2019 at 07:35:28PM +0200, Jacopo Mondi wrote:
> On Tue, Jun 11, 2019 at 06:26:58PM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 11, 2019 at 03:10:21PM +0200, Jacopo Mondi wrote:
> >> On Tue, Jun 11, 2019 at 02:41:26PM +0300, Laurent Pinchart wrote:
> >>> On Mon, Jun 10, 2019 at 06:40:50PM +0200, Jacopo Mondi wrote:
> >>>> Implement operations to set and get V4L2 controls in the V4L2Base class.
> >>>> The operations allow to set and get a single or a list of controls.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>> ---
> >>>>  src/libcamera/include/v4l2_base.h |  11 ++
> >>>>  src/libcamera/v4l2_base.cpp       | 238 ++++++++++++++++++++++++++++++
> >>>>  2 files changed, 249 insertions(+)
> >>>>
> >>>> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h
> >>>> index 2fda81a960d2..17ea734c8f49 100644
> >>>> --- a/src/libcamera/include/v4l2_base.h
> >>>> +++ b/src/libcamera/include/v4l2_base.h
> >>>> @@ -9,6 +9,8 @@
> >>>>
> >>>>  #include <vector>
> >>>>
> >>>> +#include <v4l2_controls.h>
> >>>
> >>> Just forward-declare the required classes and structures.
> >>>
> >>>> +
> >>>>  namespace libcamera {
> >>>>
> >>>>  class V4L2Base
> >>>> @@ -22,8 +24,17 @@ public:
> >>>>  	virtual void close() = 0;
> >>>>  	bool isOpen() const;
> >>>>
> >>>> +	int getControls(std::vector<unsigned int> &ctrl_ids,
> >>>> +			V4L2Controls *ctrls);
> >>>
> >>> 	const std::vector<unsigned int> &ctrl_ids,
> >>>
> >>> I still wonder if it wouldn't be simpler for applications to fill the
> >>> V4L2Controls with the control IDs to read, and just pass the
> >>> V4L2Controls pointer to this function. In any case, this is better than
> >>> the first version.
> >>
> >> I thought about that, but I don't like the fact that providing
> >> controls without a value would require to keep track of the state of a
> >> control (initialized? empty? with a default value? read from the HW?).
> >>
> >> In this way, we know that the V4L2Controls instance will contain
> >> controls with a value read from the hardware, as well as the one
> >> provided to setControl will contain values set by the user and applied
> >> to the HW. It's much easier for me not having to keep track of the
> >> control state (to prevent, say, reading a control without a value
> >> assigned). I can change this by popular demand though. No issues.
> >
> > I won't push too hard :-) But I'm wondering what your real concern is
> > here. Are you worried that an application will create a V4L2Controls
> > instance, populate it with controls to be set and pass it to
> > V4L2Base::setControls(), and then later pass the same instance to
> > V4L2Base::getControls() ?
> 
> What worries me is that application will create controls without a
> value and controls with a value, and there is no distinction between
> the two, so they could call a get$TYPE() and get back meaningless
> values from it.

So what worries you would be

	V4L2Controls ctrls;
	int value;

	ctrls.get(V4L2_CID_EXPOSURE);
	value = ctrls.getIntegerV4L2_CID_EXPOSURE);

and then using value ? If an application shoots itself in the foot with
so much dedication, do we really need to feel sorry for it ? :-)

> That calls for keeping track of the control state, or distinguish
> read/write controls, which I fear will be cumbersome and unecessary
> complicated. Am I worrying too much?
> 
> >>>> +	int setControls(V4L2Controls &ctrls);
> >>>
> >>> You should pass a const V4L2Controls &ctrls. Or, maybe you should
> >>> instead pass a non-const pointer, as setControls() should return the
> >>> value set for each control.
> >>
> >> Not sure about const, the value of the single V4L2Control should be
> >> updated to reflect what is applied to the HW.
> >
> > I agree, hence the second sentence :-) I would then pass the
> > V4L2Controls by pointer to reflect that.
> 
> Yes indeed.
> 
> >>>> +
> >>>>  protected:
> >>>>  	int fd_;
> >>>> +
> >>>> +private:
> >>>> +	int queryControl(unsigned int ctrl_id,
> >>>> +			 struct v4l2_query_ext_ctrl *qext_ctrl);
> >>>> +
> >>>>  };
> >>>>
> >>>>  }; /* namespace libcamera */
> >>>> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp
> >>>> index 7d05a3c82e4d..cbd7a551130f 100644
> >>>> --- a/src/libcamera/v4l2_base.cpp
> >>>> +++ b/src/libcamera/v4l2_base.cpp
> >>>> @@ -5,7 +5,13 @@
> >>>>   * v4l2_base.cpp - Common base for V4L2 devices and subdevices
> >>>>   */
> >>>>
> >>>> +#include <linux/v4l2-subdev.h>
> >>>> +#include <linux/videodev2.h>
> >>>> +#include <sys/ioctl.h>
> >>>> +
> >>>> +#include "log.h"
> >>>>  #include "v4l2_base.h"
> >>>> +#include "v4l2_controls.h"
> >>>>
> >>>>  /**
> >>>>   * \file v4l2_base.h
> >>>> @@ -14,6 +20,8 @@
> >>>>
> >>>>  namespace libcamera {
> >>>>
> >>>> +LOG_DEFINE_CATEGORY(V4L2Base)
> >>>> +
> >>>>  /**
> >>>>   * \class V4L2Base
> >>>>   * \brief Base class for V4L2Device and V4L2Subdevice
> >>>> @@ -31,6 +39,7 @@ namespace libcamera {
> >>>>   */
> >>>>
> >>>>  /**
> >>>> + * \fn V4L2Base::open()
> >>>>   * \brief Open a V4L2 device or subdevice
> >>>>   *
> >>>>   * Pure virtual method to be implemented by the derived classes.
> >>>> @@ -39,6 +48,7 @@ namespace libcamera {
> >>>>   */
> >>>>
> >>>>  /**
> >>>> + * \fn V4L2Base::close()
> >>>>   * \brief Close the device or subdevice
> >>>>   *
> >>>>   * Pure virtual method to be implemented by the derived classes.
> >>>> @@ -53,6 +63,234 @@ bool V4L2Base::isOpen() const
> >>>>  	return fd_ != -1;
> >>>>  }
> >>>>
> >>>> +int V4L2Base::queryControl(unsigned int id,
> >>>> +			   struct v4l2_query_ext_ctrl *qext_ctrl)
> >>>> +{
> >>>> +	qext_ctrl->id = id;
> >>>> +
> >>>> +	int ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);
> >>>> +	if (ret) {
> >>>> +		ret = -errno;
> >>>> +		LOG(V4L2Base, Error)
> >>>> +			<< "Unable to query extended control 0x"
> >>>> +			<< std::hex << qext_ctrl->id
> >>>> +			<< ": " << strerror(-ret);
> >>>> +		return ret;
> >>>> +	}
> >>>> +
> >>>> +	if (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
> >>>> +		LOG(V4L2Base, Error)
> >>>> +			<< "Extended control 0x" << std::hex
> >>>> +			<< qext_ctrl->id << " is disabled";
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \brief Get values of a list of control IDs
> >>>> + * \param ctrl_ids The list of control IDs to retrieve value of
> >>>> + * \return A list of V4L2Control on success or a empty list otherwise
> >>>> + */
> >>>> +int V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids,
> >>>> +			  V4L2Controls *controls)
> >>>> +{
> >>>> +	unsigned int count = ctrl_ids.size();
> >>>> +	if (!count)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	controls->clear();
> >>>> +
> >>>> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> >>>
> >>> You're leaking memory. The simplest solution would be
> >>>
> >>> 	struct v4l2_ext_controls v4l2_ctrls[count];
> >>
> >> Indeed!
> >> I wonder why valgrind reported no leaks...
> >>
> >>>> +
> >>>> +	/*
> >>>> +	 * Query each control in the vector to get back its type and expected
> >>>> +	 * size in order to get its current value.
> >>>> +	 */
> >>>> +	for (unsigned int i = 0; i < count; ++i) {
> >>>> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> >>>> +		unsigned int ctrl_id = ctrl_ids[i];
> >>>> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> >>>> +
> >>>> +		int ret = queryControl(ctrl_id, &qext_ctrl);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +
> >>>> +		v4l2_ctrl->id = ctrl_id;
> >>>> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> >>>> +
> >>>> +		/* Reserve memory for controls with payload. */
> >>>> +		if (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))
> >>>> +			continue;
> >>>> +
> >>>> +		switch (qext_ctrl.type) {
> >>>> +		case V4L2_CTRL_TYPE_U8:
> >>>> +			v4l2_ctrl->p_u8 = static_cast<uint8_t *>
> >>>> +					  (new uint8_t[v4l2_ctrl->size]);
> >>>> +			break;
> >>>> +		case V4L2_CTRL_TYPE_U16:
> >>>> +			v4l2_ctrl->p_u16 = static_cast<uint16_t *>
> >>>> +					  (new uint16_t[v4l2_ctrl->size]);
> >>>> +			break;
> >>>> +		case V4L2_CTRL_TYPE_U32:
> >>>> +			v4l2_ctrl->p_u32 = static_cast<uint32_t *>
> >>>> +					  (new uint32_t[v4l2_ctrl->size]);
> >>>> +			break;
> >>>
> >>> This is lots of new's, and they're all leaked. A better solution would
> >>> be to populate ctrls with the V4L2Control instances already, and use the
> >>> pointer to the internal memory buffers directly.
> >>
> >> Sorry, I didn't get you, what's 'ctrls' here?
> >
> > Sorry, I meant the controls parameter.
> >
> >> The memory here allocated (and not released, my bad) serves to the
> >> kernel to copy the data payload to userspace. It is then copied inside
> >> the V4L2Control, where memory is reserved at creation time.
> >>
> >> What I could do, and I think that's what you suggested is to create
> >> the V4L2Control in advance (once I know the required memory size) and
> >> point the 'struct v4l2_ext_control' data pointer to that memory, and
> >> save one copy.
> >
> > Correct, that's what I meant.
> >
> >> Personally, I'm also fine dropping support for payload controls for
> >> this first version, as far as I could tell, there's no strict
> >> requirement for any of them on IPU3 (as Kieran suggested).
> >>
> >> What's your opinion?
> >
> > We have very few payload controls in mainline, so that could indeed be a
> > good idea. I would however experiment with creation of the V4L2Control
> > instance before calling VIDIOC_G_EXT_CTRLS to see if it would be an
> > option for a later support of compound controls.
> 
> I'm a bit in two minds here:
> If we drop support for controls with payload it calls for dropping the
> template and use Kieran's ControlValue. But then, once we'll ever have
> to support them, I fear the ControlValue class would be a waste of space
> and less flexible to use with controls with payload...
> 
> It seems to me that dropping payload support and use ControlValue as
> backing storage would make it easier to get the series in, so I'm
> tempted to go for the "is good for now" way and do what is outlined
> here above...

You could keep the template, but it would indeed be nice to use the same
class to store the control value. It's not a hard requirement though, if
there are good reasons not to do so, I'm fine keeping them separate.

> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> >>>> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> >>>> +	v4l2_ext_ctrls.count = count;
> >>>> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> >>>> +
> >>>> +	int ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);
> >>>> +	if (ret) {
> >>>> +		ret = -errno;
> >>>> +		LOG(V4L2Base, Error)
> >>>> +			<< "Unable to get extended controls: " << strerror(-ret);
> >>>> +		return ret;
> >>>> +	}
> >>>> +
> >>>> +	/*
> >>>> +	 * For each requested control, create a V4L2Control which contains
> >>>> +	 * the current value and store it in the vector returned to the caller.
> >>>> +	 */
> >>>> +	LOG(Error) << __func__;
> >>>> +	for (unsigned int i = 0; i < count; ++i) {
> >>>> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> >>>> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> >>>> +
> >>>> +		int ret = queryControl(v4l2_ctrl->id, &qext_ctrl);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +
> >>>> +		unsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;
> >>>> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> >>>> +			/* Control types with payload */
> >>>> +			switch (qext_ctrl.type) {
> >>>> +			case V4L2_CTRL_TYPE_U8:
> >>>> +				controls->set(v4l2_ctrl->id, size,
> >>>> +					      v4l2_ctrl->p_u8);
> >>>> +				break;
> >>>> +			case V4L2_CTRL_TYPE_U16:
> >>>> +				controls->set(v4l2_ctrl->id, size,
> >>>> +					      v4l2_ctrl->p_u16);
> >>>> +				break;
> >>>> +			case V4L2_CTRL_TYPE_U32:
> >>>> +				controls->set(v4l2_ctrl->id, size,
> >>>> +					      v4l2_ctrl->p_u32);
> >>>> +				break;
> >>>> +			default:
> >>>> +				LOG(V4L2Base, Error)
> >>>> +					<< "Compound control types not supported: "
> >>>> +					<< std::hex << qext_ctrl.type;
> >>>> +				return -EINVAL;
> >>>> +			}
> >>>> +		} else {
> >>>> +			/* Control types without payload. */
> >>>> +			switch (qext_ctrl.type) {
> >>>> +			case V4L2_CTRL_TYPE_INTEGER64:
> >>>> +				controls->set(v4l2_ctrl->id, static_cast<int64_t>
> >>>> +					     (v4l2_ctrl->value64));
> >>>> +				break;
> >>>> +			case V4L2_CTRL_TYPE_INTEGER:
> >>>> +			case V4L2_CTRL_TYPE_BOOLEAN:
> >>>> +			case V4L2_CTRL_TYPE_MENU:
> >>>> +			case V4L2_CTRL_TYPE_BUTTON:
> >>>> +				controls->set(v4l2_ctrl->id, v4l2_ctrl->value);
> >>>> +				break;
> >>>> +			default:
> >>>> +				LOG(V4L2Base, Error)
> >>>> +					<< "Invalid non-compound control type :"
> >>>> +					<< std::hex << qext_ctrl.type;
> >>>> +				return -EINVAL;
> >>>> +			}
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \brief Apply a list of controls to the device
> >>>> + * \param ctrls The list of controls to apply
> >>>> + * \return 0 on success or a negative error code otherwise
> >>>> + */
> >>>> +int V4L2Base::setControls(V4L2Controls &ctrls)
> >>>> +{
> >>>> +	unsigned int count = ctrls.size();
> >>>> +	if (!count)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> >>>> +
> >>>> +	/*
> >>>> +	 * Query each control in the vector to get back its type and expected
> >>>> +	 * size and set the new desired value in each v4l2_ext_control member.
> >>>> +	 */
> >>>> +	for (unsigned int i = 0; i < count; ++i) {
> >>>> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> >>>> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> >>>> +		V4L2Control *ctrl = ctrls[i];
> >>>> +
> >>>> +		int ret = queryControl(ctrl->id(), &qext_ctrl);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +
> >>>> +		v4l2_ctrl->id = ctrl->id();
> >>>> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> >>>> +
> >>>> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> >>>> +			/** \todo: Support set controls with payload. */
> >>>> +			LOG(V4L2Base, Error)
> >>>> +				<< "Controls with payload not supported";
> >>>> +			return -EINVAL;
> >>>> +		} else {
> >>>> +			switch (qext_ctrl.type) {
> >>>> +			case V4L2_CTRL_TYPE_INTEGER64:
> >>>> +			{
> >>>> +				V4L2Int64Control *c =
> >>>> +					static_cast<V4L2Int64Control *>(ctrl);
> >>>> +				v4l2_ctrl->value64 = c->value();
> >>>> +			}
> >>>> +				break;
> >>>> +			case V4L2_CTRL_TYPE_INTEGER:
> >>>> +			case V4L2_CTRL_TYPE_BOOLEAN:
> >>>> +			case V4L2_CTRL_TYPE_MENU:
> >>>> +			case V4L2_CTRL_TYPE_BUTTON:
> >>>> +			{
> >>>> +				V4L2IntControl *c =
> >>>> +					static_cast<V4L2IntControl *>(ctrl);
> >>>> +				v4l2_ctrl->value = c->value();
> >>>> +			}
> >>>> +				break;
> >>>> +			default:
> >>>> +				LOG(V4L2Base, Error)
> >>>> +					<< "Invalid non-compound control type :"
> >>>> +					<< qext_ctrl.type;
> >>>> +				return -EINVAL;
> >>>> +			}
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> >>>> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> >>>> +	v4l2_ext_ctrls.count = count;
> >>>> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> >>>> +
> >>>> +	int ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);
> >>>> +	if (ret) {
> >>>> +		ret = -errno;
> >>>> +		LOG(V4L2Base, Error)
> >>>> +			<< "Unable to set controls: " << strerror(-ret);
> >>>> +		return ret;
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>  /**
> >>>>   * \var V4L2Base::fd_
> >>>>   * \brief The V4L2 device or subdevice device node file descriptor

HI Laurent,

On Tue, Jun 11, 2019 at 08:55:27PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Jun 11, 2019 at 07:35:28PM +0200, Jacopo Mondi wrote:
> > On Tue, Jun 11, 2019 at 06:26:58PM +0300, Laurent Pinchart wrote:
> > > On Tue, Jun 11, 2019 at 03:10:21PM +0200, Jacopo Mondi wrote:
> > >> On Tue, Jun 11, 2019 at 02:41:26PM +0300, Laurent Pinchart wrote:
> > >>> On Mon, Jun 10, 2019 at 06:40:50PM +0200, Jacopo Mondi wrote:
> > >>>> Implement operations to set and get V4L2 controls in the V4L2Base class.
> > >>>> The operations allow to set and get a single or a list of controls.
> > >>>>
> > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>>> ---
> > >>>>  src/libcamera/include/v4l2_base.h |  11 ++
> > >>>>  src/libcamera/v4l2_base.cpp       | 238 ++++++++++++++++++++++++++++++
> > >>>>  2 files changed, 249 insertions(+)
> > >>>>
> > >>>> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h
> > >>>> index 2fda81a960d2..17ea734c8f49 100644
> > >>>> --- a/src/libcamera/include/v4l2_base.h
> > >>>> +++ b/src/libcamera/include/v4l2_base.h
> > >>>> @@ -9,6 +9,8 @@
> > >>>>
> > >>>>  #include <vector>
> > >>>>
> > >>>> +#include <v4l2_controls.h>
> > >>>
> > >>> Just forward-declare the required classes and structures.
> > >>>
> > >>>> +
> > >>>>  namespace libcamera {
> > >>>>
> > >>>>  class V4L2Base
> > >>>> @@ -22,8 +24,17 @@ public:
> > >>>>  	virtual void close() = 0;
> > >>>>  	bool isOpen() const;
> > >>>>
> > >>>> +	int getControls(std::vector<unsigned int> &ctrl_ids,
> > >>>> +			V4L2Controls *ctrls);
> > >>>
> > >>> 	const std::vector<unsigned int> &ctrl_ids,
> > >>>
> > >>> I still wonder if it wouldn't be simpler for applications to fill the
> > >>> V4L2Controls with the control IDs to read, and just pass the
> > >>> V4L2Controls pointer to this function. In any case, this is better than
> > >>> the first version.
> > >>
> > >> I thought about that, but I don't like the fact that providing
> > >> controls without a value would require to keep track of the state of a
> > >> control (initialized? empty? with a default value? read from the HW?).
> > >>
> > >> In this way, we know that the V4L2Controls instance will contain
> > >> controls with a value read from the hardware, as well as the one
> > >> provided to setControl will contain values set by the user and applied
> > >> to the HW. It's much easier for me not having to keep track of the
> > >> control state (to prevent, say, reading a control without a value
> > >> assigned). I can change this by popular demand though. No issues.
> > >
> > > I won't push too hard :-) But I'm wondering what your real concern is
> > > here. Are you worried that an application will create a V4L2Controls
> > > instance, populate it with controls to be set and pass it to
> > > V4L2Base::setControls(), and then later pass the same instance to
> > > V4L2Base::getControls() ?
> >
> > What worries me is that application will create controls without a
> > value and controls with a value, and there is no distinction between
> > the two, so they could call a get$TYPE() and get back meaningless
> > values from it.
>
> So what worries you would be
>
> 	V4L2Controls ctrls;
> 	int value;
>
> 	ctrls.get(V4L2_CID_EXPOSURE);
> 	value = ctrls.getIntegerV4L2_CID_EXPOSURE);
>
> and then using value ? If an application shoots itself in the foot with
> so much dedication, do we really need to feel sorry for it ? :-)

Well, yes, in this case the application is defintely looking for
troubles. I think in more complex sequences, keeping track of the
state of a control might be add complications both for the app, both
in the framework, as we'll have to deal with non-initialized values.

Anyway, you and Kieran do not seem to be worried about this, so it
might just be me. I'll try go your suggested way then and see how it
looks.

Side note: I would not have ctrls.set(ID, value) and ctrls.get(ID) but
rather ctrls.add(ID, value) and ctrls.add(ID). The same ctrls can be
used as argument in getControls() and setControls() and a control
initialized with a value can be read as well as a not initialized one.

>
> > That calls for keeping track of the control state, or distinguish
> > read/write controls, which I fear will be cumbersome and unecessary
> > complicated. Am I worrying too much?
> >
> > >>>> +	int setControls(V4L2Controls &ctrls);
> > >>>
> > >>> You should pass a const V4L2Controls &ctrls. Or, maybe you should
> > >>> instead pass a non-const pointer, as setControls() should return the
> > >>> value set for each control.
> > >>
> > >> Not sure about const, the value of the single V4L2Control should be
> > >> updated to reflect what is applied to the HW.
> > >
> > > I agree, hence the second sentence :-) I would then pass the
> > > V4L2Controls by pointer to reflect that.
> >
> > Yes indeed.
> >
> > >>>> +
> > >>>>  protected:
> > >>>>  	int fd_;
> > >>>> +
> > >>>> +private:
> > >>>> +	int queryControl(unsigned int ctrl_id,
> > >>>> +			 struct v4l2_query_ext_ctrl *qext_ctrl);
> > >>>> +
> > >>>>  };
> > >>>>
> > >>>>  }; /* namespace libcamera */
> > >>>> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp
> > >>>> index 7d05a3c82e4d..cbd7a551130f 100644
> > >>>> --- a/src/libcamera/v4l2_base.cpp
> > >>>> +++ b/src/libcamera/v4l2_base.cpp
> > >>>> @@ -5,7 +5,13 @@
> > >>>>   * v4l2_base.cpp - Common base for V4L2 devices and subdevices
> > >>>>   */
> > >>>>
> > >>>> +#include <linux/v4l2-subdev.h>
> > >>>> +#include <linux/videodev2.h>
> > >>>> +#include <sys/ioctl.h>
> > >>>> +
> > >>>> +#include "log.h"
> > >>>>  #include "v4l2_base.h"
> > >>>> +#include "v4l2_controls.h"
> > >>>>
> > >>>>  /**
> > >>>>   * \file v4l2_base.h
> > >>>> @@ -14,6 +20,8 @@
> > >>>>
> > >>>>  namespace libcamera {
> > >>>>
> > >>>> +LOG_DEFINE_CATEGORY(V4L2Base)
> > >>>> +
> > >>>>  /**
> > >>>>   * \class V4L2Base
> > >>>>   * \brief Base class for V4L2Device and V4L2Subdevice
> > >>>> @@ -31,6 +39,7 @@ namespace libcamera {
> > >>>>   */
> > >>>>
> > >>>>  /**
> > >>>> + * \fn V4L2Base::open()
> > >>>>   * \brief Open a V4L2 device or subdevice
> > >>>>   *
> > >>>>   * Pure virtual method to be implemented by the derived classes.
> > >>>> @@ -39,6 +48,7 @@ namespace libcamera {
> > >>>>   */
> > >>>>
> > >>>>  /**
> > >>>> + * \fn V4L2Base::close()
> > >>>>   * \brief Close the device or subdevice
> > >>>>   *
> > >>>>   * Pure virtual method to be implemented by the derived classes.
> > >>>> @@ -53,6 +63,234 @@ bool V4L2Base::isOpen() const
> > >>>>  	return fd_ != -1;
> > >>>>  }
> > >>>>
> > >>>> +int V4L2Base::queryControl(unsigned int id,
> > >>>> +			   struct v4l2_query_ext_ctrl *qext_ctrl)
> > >>>> +{
> > >>>> +	qext_ctrl->id = id;
> > >>>> +
> > >>>> +	int ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);
> > >>>> +	if (ret) {
> > >>>> +		ret = -errno;
> > >>>> +		LOG(V4L2Base, Error)
> > >>>> +			<< "Unable to query extended control 0x"
> > >>>> +			<< std::hex << qext_ctrl->id
> > >>>> +			<< ": " << strerror(-ret);
> > >>>> +		return ret;
> > >>>> +	}
> > >>>> +
> > >>>> +	if (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {
> > >>>> +		LOG(V4L2Base, Error)
> > >>>> +			<< "Extended control 0x" << std::hex
> > >>>> +			<< qext_ctrl->id << " is disabled";
> > >>>> +		return -EINVAL;
> > >>>> +	}
> > >>>> +
> > >>>> +	return 0;
> > >>>> +}
> > >>>> +
> > >>>> +/**
> > >>>> + * \brief Get values of a list of control IDs
> > >>>> + * \param ctrl_ids The list of control IDs to retrieve value of
> > >>>> + * \return A list of V4L2Control on success or a empty list otherwise
> > >>>> + */
> > >>>> +int V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids,
> > >>>> +			  V4L2Controls *controls)
> > >>>> +{
> > >>>> +	unsigned int count = ctrl_ids.size();
> > >>>> +	if (!count)
> > >>>> +		return -EINVAL;
> > >>>> +
> > >>>> +	controls->clear();
> > >>>> +
> > >>>> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> > >>>
> > >>> You're leaking memory. The simplest solution would be
> > >>>
> > >>> 	struct v4l2_ext_controls v4l2_ctrls[count];
> > >>
> > >> Indeed!
> > >> I wonder why valgrind reported no leaks...
> > >>
> > >>>> +
> > >>>> +	/*
> > >>>> +	 * Query each control in the vector to get back its type and expected
> > >>>> +	 * size in order to get its current value.
> > >>>> +	 */
> > >>>> +	for (unsigned int i = 0; i < count; ++i) {
> > >>>> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > >>>> +		unsigned int ctrl_id = ctrl_ids[i];
> > >>>> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> > >>>> +
> > >>>> +		int ret = queryControl(ctrl_id, &qext_ctrl);
> > >>>> +		if (ret)
> > >>>> +			return ret;
> > >>>> +
> > >>>> +		v4l2_ctrl->id = ctrl_id;
> > >>>> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> > >>>> +
> > >>>> +		/* Reserve memory for controls with payload. */
> > >>>> +		if (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))
> > >>>> +			continue;
> > >>>> +
> > >>>> +		switch (qext_ctrl.type) {
> > >>>> +		case V4L2_CTRL_TYPE_U8:
> > >>>> +			v4l2_ctrl->p_u8 = static_cast<uint8_t *>
> > >>>> +					  (new uint8_t[v4l2_ctrl->size]);
> > >>>> +			break;
> > >>>> +		case V4L2_CTRL_TYPE_U16:
> > >>>> +			v4l2_ctrl->p_u16 = static_cast<uint16_t *>
> > >>>> +					  (new uint16_t[v4l2_ctrl->size]);
> > >>>> +			break;
> > >>>> +		case V4L2_CTRL_TYPE_U32:
> > >>>> +			v4l2_ctrl->p_u32 = static_cast<uint32_t *>
> > >>>> +					  (new uint32_t[v4l2_ctrl->size]);
> > >>>> +			break;
> > >>>
> > >>> This is lots of new's, and they're all leaked. A better solution would
> > >>> be to populate ctrls with the V4L2Control instances already, and use the
> > >>> pointer to the internal memory buffers directly.
> > >>
> > >> Sorry, I didn't get you, what's 'ctrls' here?
> > >
> > > Sorry, I meant the controls parameter.
> > >
> > >> The memory here allocated (and not released, my bad) serves to the
> > >> kernel to copy the data payload to userspace. It is then copied inside
> > >> the V4L2Control, where memory is reserved at creation time.
> > >>
> > >> What I could do, and I think that's what you suggested is to create
> > >> the V4L2Control in advance (once I know the required memory size) and
> > >> point the 'struct v4l2_ext_control' data pointer to that memory, and
> > >> save one copy.
> > >
> > > Correct, that's what I meant.
> > >
> > >> Personally, I'm also fine dropping support for payload controls for
> > >> this first version, as far as I could tell, there's no strict
> > >> requirement for any of them on IPU3 (as Kieran suggested).
> > >>
> > >> What's your opinion?
> > >
> > > We have very few payload controls in mainline, so that could indeed be a
> > > good idea. I would however experiment with creation of the V4L2Control
> > > instance before calling VIDIOC_G_EXT_CTRLS to see if it would be an
> > > option for a later support of compound controls.
> >
> > I'm a bit in two minds here:
> > If we drop support for controls with payload it calls for dropping the
> > template and use Kieran's ControlValue. But then, once we'll ever have
> > to support them, I fear the ControlValue class would be a waste of space
> > and less flexible to use with controls with payload...
> >
> > It seems to me that dropping payload support and use ControlValue as
> > backing storage would make it easier to get the series in, so I'm
> > tempted to go for the "is good for now" way and do what is outlined
> > here above...
>
> You could keep the template, but it would indeed be nice to use the same
> class to store the control value. It's not a hard requirement though, if
> there are good reasons not to do so, I'm fine keeping them separate.
>

I'll try to use Kieran's ControlValue in the next version and
drop support for compound and strings controls.

Thanks
  j

> > >>>> +		}
> > >>>> +	}
> > >>>> +
> > >>>> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> > >>>> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> > >>>> +	v4l2_ext_ctrls.count = count;
> > >>>> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> > >>>> +
> > >>>> +	int ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);
> > >>>> +	if (ret) {
> > >>>> +		ret = -errno;
> > >>>> +		LOG(V4L2Base, Error)
> > >>>> +			<< "Unable to get extended controls: " << strerror(-ret);
> > >>>> +		return ret;
> > >>>> +	}
> > >>>> +
> > >>>> +	/*
> > >>>> +	 * For each requested control, create a V4L2Control which contains
> > >>>> +	 * the current value and store it in the vector returned to the caller.
> > >>>> +	 */
> > >>>> +	LOG(Error) << __func__;
> > >>>> +	for (unsigned int i = 0; i < count; ++i) {
> > >>>> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > >>>> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> > >>>> +
> > >>>> +		int ret = queryControl(v4l2_ctrl->id, &qext_ctrl);
> > >>>> +		if (ret)
> > >>>> +			return ret;
> > >>>> +
> > >>>> +		unsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;
> > >>>> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> > >>>> +			/* Control types with payload */
> > >>>> +			switch (qext_ctrl.type) {
> > >>>> +			case V4L2_CTRL_TYPE_U8:
> > >>>> +				controls->set(v4l2_ctrl->id, size,
> > >>>> +					      v4l2_ctrl->p_u8);
> > >>>> +				break;
> > >>>> +			case V4L2_CTRL_TYPE_U16:
> > >>>> +				controls->set(v4l2_ctrl->id, size,
> > >>>> +					      v4l2_ctrl->p_u16);
> > >>>> +				break;
> > >>>> +			case V4L2_CTRL_TYPE_U32:
> > >>>> +				controls->set(v4l2_ctrl->id, size,
> > >>>> +					      v4l2_ctrl->p_u32);
> > >>>> +				break;
> > >>>> +			default:
> > >>>> +				LOG(V4L2Base, Error)
> > >>>> +					<< "Compound control types not supported: "
> > >>>> +					<< std::hex << qext_ctrl.type;
> > >>>> +				return -EINVAL;
> > >>>> +			}
> > >>>> +		} else {
> > >>>> +			/* Control types without payload. */
> > >>>> +			switch (qext_ctrl.type) {
> > >>>> +			case V4L2_CTRL_TYPE_INTEGER64:
> > >>>> +				controls->set(v4l2_ctrl->id, static_cast<int64_t>
> > >>>> +					     (v4l2_ctrl->value64));
> > >>>> +				break;
> > >>>> +			case V4L2_CTRL_TYPE_INTEGER:
> > >>>> +			case V4L2_CTRL_TYPE_BOOLEAN:
> > >>>> +			case V4L2_CTRL_TYPE_MENU:
> > >>>> +			case V4L2_CTRL_TYPE_BUTTON:
> > >>>> +				controls->set(v4l2_ctrl->id, v4l2_ctrl->value);
> > >>>> +				break;
> > >>>> +			default:
> > >>>> +				LOG(V4L2Base, Error)
> > >>>> +					<< "Invalid non-compound control type :"
> > >>>> +					<< std::hex << qext_ctrl.type;
> > >>>> +				return -EINVAL;
> > >>>> +			}
> > >>>> +		}
> > >>>> +	}
> > >>>> +
> > >>>> +	return 0;
> > >>>> +}
> > >>>> +
> > >>>> +/**
> > >>>> + * \brief Apply a list of controls to the device
> > >>>> + * \param ctrls The list of controls to apply
> > >>>> + * \return 0 on success or a negative error code otherwise
> > >>>> + */
> > >>>> +int V4L2Base::setControls(V4L2Controls &ctrls)
> > >>>> +{
> > >>>> +	unsigned int count = ctrls.size();
> > >>>> +	if (!count)
> > >>>> +		return -EINVAL;
> > >>>> +
> > >>>> +	struct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];
> > >>>> +
> > >>>> +	/*
> > >>>> +	 * Query each control in the vector to get back its type and expected
> > >>>> +	 * size and set the new desired value in each v4l2_ext_control member.
> > >>>> +	 */
> > >>>> +	for (unsigned int i = 0; i < count; ++i) {
> > >>>> +		struct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];
> > >>>> +		struct v4l2_query_ext_ctrl qext_ctrl = {};
> > >>>> +		V4L2Control *ctrl = ctrls[i];
> > >>>> +
> > >>>> +		int ret = queryControl(ctrl->id(), &qext_ctrl);
> > >>>> +		if (ret)
> > >>>> +			return ret;
> > >>>> +
> > >>>> +		v4l2_ctrl->id = ctrl->id();
> > >>>> +		v4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;
> > >>>> +
> > >>>> +		if (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> > >>>> +			/** \todo: Support set controls with payload. */
> > >>>> +			LOG(V4L2Base, Error)
> > >>>> +				<< "Controls with payload not supported";
> > >>>> +			return -EINVAL;
> > >>>> +		} else {
> > >>>> +			switch (qext_ctrl.type) {
> > >>>> +			case V4L2_CTRL_TYPE_INTEGER64:
> > >>>> +			{
> > >>>> +				V4L2Int64Control *c =
> > >>>> +					static_cast<V4L2Int64Control *>(ctrl);
> > >>>> +				v4l2_ctrl->value64 = c->value();
> > >>>> +			}
> > >>>> +				break;
> > >>>> +			case V4L2_CTRL_TYPE_INTEGER:
> > >>>> +			case V4L2_CTRL_TYPE_BOOLEAN:
> > >>>> +			case V4L2_CTRL_TYPE_MENU:
> > >>>> +			case V4L2_CTRL_TYPE_BUTTON:
> > >>>> +			{
> > >>>> +				V4L2IntControl *c =
> > >>>> +					static_cast<V4L2IntControl *>(ctrl);
> > >>>> +				v4l2_ctrl->value = c->value();
> > >>>> +			}
> > >>>> +				break;
> > >>>> +			default:
> > >>>> +				LOG(V4L2Base, Error)
> > >>>> +					<< "Invalid non-compound control type :"
> > >>>> +					<< qext_ctrl.type;
> > >>>> +				return -EINVAL;
> > >>>> +			}
> > >>>> +		}
> > >>>> +	}
> > >>>> +
> > >>>> +	struct v4l2_ext_controls v4l2_ext_ctrls = {};
> > >>>> +	v4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> > >>>> +	v4l2_ext_ctrls.count = count;
> > >>>> +	v4l2_ext_ctrls.controls = v4l2_ctrls;
> > >>>> +
> > >>>> +	int ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);
> > >>>> +	if (ret) {
> > >>>> +		ret = -errno;
> > >>>> +		LOG(V4L2Base, Error)
> > >>>> +			<< "Unable to set controls: " << strerror(-ret);
> > >>>> +		return ret;
> > >>>> +	}
> > >>>> +
> > >>>> +	return 0;
> > >>>> +}
> > >>>> +
> > >>>>  /**
> > >>>>   * \var V4L2Base::fd_
> > >>>>   * \brief The V4L2 device or subdevice device node file descriptor
>
> --
> Regards,
>
> Laurent Pinchart