[libcamera-devel] [PATCH v2 4/6] libcamera: v4l2_base: Add V4L2 control support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 11 17:26:58 CEST 2019
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 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.
>
> 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list