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

[Kieran Bingham]

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 at 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
> 

-- 
Regards
--
Kieran