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