[{"id":1741,"web_url":"https://patchwork.libcamera.org/comment/1741/","msgid":"<5742d7bb-4c09-5e70-f3b7-81cac34c5247@ideasonboard.com>","date":"2019-06-03T12:32:58","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: v4l2_base: Add V4L2\n\tcontrol support","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nSome thoughts out loud inline....\n\nOn 02/06/2019 14:04, Jacopo Mondi wrote:\n> Implement operations to set and get V4L2 controls in the V4L2Base class.\n> The operations allow to set and get a single or a list of controls.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/include/v4l2_base.h |  12 ++\n>  src/libcamera/v4l2_base.cpp       | 290 ++++++++++++++++++++++++++++++\n>  2 files changed, 302 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> index 2fda81a960d2..7d4e238dadfa 100644\n> --- a/src/libcamera/include/v4l2_base.h\n> +++ b/src/libcamera/include/v4l2_base.h\n> @@ -9,6 +9,8 @@\n>  \n>  #include <vector>\n>  \n> +#include <v4l2_controls.h>\n> +\n>  namespace libcamera {\n>  \n>  class V4L2Base\n> @@ -22,8 +24,18 @@ public:\n>  \tvirtual void close() = 0;\n>  \tbool isOpen() const;\n>  \n> +\tstd::vector<V4L2Control *> getControls(std::vector<unsigned int> &ctrl_ids);\n\nIn my controls, I pass a list <well a map, or a set; I have two\nimplementations currently> of the actual ControlValue objects, which get\nfilled in.\n\nI wonder if there will be much differences between the two interface\ndesigns.\n\n> +\tV4L2Control *getControl(unsigned int ctrl_id);\n> +\tint setControls(std::vector<V4L2Control *> &ctrls);\n> +\tint setControl(V4L2Control *ctrl);\n\nI think I'd group those as getControls setControls, and then getControl,\nsetControl, so that they are grouped by 'functionality' but that might\nnot be a preferred sorting... so either way.\n\n\n\n> +\n>  protected:\n>  \tint fd_;\n> +\n> +private:\n> +\tint queryControl(unsigned int ctrl_id,\n> +\t\t\t struct v4l2_query_ext_ctrl *qext_ctrl);\n> +\n>  };\n>  \n>  }; /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp\n> index 7d05a3c82e4d..423a4d84e276 100644\n> --- a/src/libcamera/v4l2_base.cpp\n> +++ b/src/libcamera/v4l2_base.cpp\n> @@ -5,7 +5,13 @@\n>   * v4l2_base.cpp - Common base for V4L2 devices and subdevices\n>   */\n>  \n> +#include <linux/v4l2-subdev.h>\n> +#include <linux/videodev2.h>\n> +#include <sys/ioctl.h>\n> +\n> +#include \"log.h\"\n>  #include \"v4l2_base.h\"\n> +#include \"v4l2_controls.h\"\n>  \n>  /**\n>   * \\file v4l2_base.h\n> @@ -14,6 +20,8 @@\n>  \n>  namespace libcamera {\n>  \n> +LOG_DEFINE_CATEGORY(V4L2Base)\n> +\n>  /**\n>   * \\class V4L2Base\n>   * \\brief Base class for V4L2Device and V4L2Subdevice\n> @@ -31,6 +39,7 @@ namespace libcamera {\n>   */\n>  \n>  /**\n> + * \\fn V4L2Base::open()\n>   * \\brief Open a V4L2 device or subdevice\n>   *\n>   * Pure virtual method to be implemented by the derived classes.\n> @@ -39,6 +48,7 @@ namespace libcamera {\n>   */\n>  \n>  /**\n> + * \\fn V4L2Base::close()\n>   * \\brief Close the device or subdevice\n>   *\n>   * Pure virtual method to be implemented by the derived classes.\n> @@ -53,6 +63,286 @@ bool V4L2Base::isOpen() const\n>  \treturn fd_ != -1;\n>  }\n>  \n> +int V4L2Base::queryControl(unsigned int id,\n> +\t\t\t   struct v4l2_query_ext_ctrl *qext_ctrl)\n> +{\n> +\tqext_ctrl->id = id;\n> +\n> +\tint ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(V4L2Base, Error)\n> +\t\t\t<< \"Unable to query extended control 0x\"\n> +\t\t\t<< std::hex << qext_ctrl->id\n> +\t\t\t<< \": \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {\n> +\t\tLOG(V4L2Base, Error)\n> +\t\t\t<< \"Extended control 0x\" << std::hex\n> +\t\t\t<< qext_ctrl->id << \" is disabled\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Get values of a list of control IDs\n> + * \\param ctrl_ids The list of control IDs to retrieve value of\n> + * \\return A list of V4L2Control on success or a empty list otherwise\n> + */\n> +std::vector<V4L2Control *> V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids)\n> +{\n> +\tunsigned int count = ctrl_ids.size();\n> +\tif (!count)\n> +\t\treturn std::vector<V4L2Control *> {};\n> +\n> +\tstruct v4l2_ext_control *v4l2_ctrls = static_cast<struct v4l2_ext_control *>\n> +\t\t\t\t\t      (new struct v4l2_ext_control[count]);\n> +\tif (!v4l2_ctrls)\n> +\t\treturn std::vector<V4L2Control *> {};\n> +\n> +\t/*\n> +\t * Query each control in the vector to get back its type and expected\n> +\t * size in order to get its current value.\n> +\t */\n> +\tfor (unsigned int i = 0; i < count; ++i) {\n> +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> +\t\tunsigned int ctrl_id = ctrl_ids[i];\n> +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> +\n> +\t\tint ret = queryControl(ctrl_id, &qext_ctrl);\n> +\t\tif (ret)\n> +\t\t\treturn std::vector<V4L2Control *> {};\n> +\n> +\t\tv4l2_ctrl->id = ctrl_id;\n> +\t\tv4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;\n> +\n> +\t\t/* Reserve memory for controls with payload. */\n> +\t\tif (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))\n> +\t\t\tcontinue;\n> +\n> +\t\tswitch (qext_ctrl.type) {\n> +\t\tcase V4L2_CTRL_TYPE_U8:\n> +\t\t\tv4l2_ctrl->p_u8 = static_cast<uint8_t *>\n> +\t\t\t\t\t  (new uint8_t[v4l2_ctrl->size]);\n> +\t\t\tbreak;\n> +\t\tcase V4L2_CTRL_TYPE_U16:\n> +\t\t\tv4l2_ctrl->p_u16 = static_cast<uint16_t *>\n> +\t\t\t\t\t  (new uint16_t[v4l2_ctrl->size]);\n> +\t\t\tbreak;\n> +\t\tcase V4L2_CTRL_TYPE_U32:\n> +\t\t\tv4l2_ctrl->p_u32 = static_cast<uint32_t *>\n> +\t\t\t\t\t  (new uint32_t[v4l2_ctrl->size]);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tstruct v4l2_ext_controls v4l2_ext_ctrls = {};\n> +\tv4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> +\tv4l2_ext_ctrls.count = count;\n> +\tv4l2_ext_ctrls.controls = v4l2_ctrls;\n> +\n> +\tint ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(V4L2Base, Error)\n> +\t\t\t<< \"Unable to get extended controls: \" << strerror(-ret);\n> +\t\t\treturn std::vector<V4L2Control *> {};\n> +\t}\n> +\n> +\t/*\n> +\t * For each requested control, create a V4L2Control which contains\n> +\t * the current value and store it in the vector returned to the caller.\n> +\t */\n> +\tstd::vector<V4L2Control *> controls = {};\n> +\tfor (unsigned int i = 0; i < count; ++i) {\n> +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> +\n> +\t\tint ret = queryControl(v4l2_ctrl->id, &qext_ctrl);\n> +\t\tif (ret)\n> +\t\t\treturn std::vector<V4L2Control *> {};\n> +\n> +\t\tunsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;\n> +\t\tif (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> +\t\t\t/* Control types with payload */\n> +\t\t\tswitch (qext_ctrl.type) {\n> +\t\t\tcase V4L2_CTRL_TYPE_U8:\n> +\t\t\t{\n> +\t\t\t\tV4L2U8Control *c =\n> +\t\t\t\t\tnew V4L2U8Control(v4l2_ctrl->id, size,\n> +\t\t\t\t\t\t\t  v4l2_ctrl->p_u8);\n> +\t\t\t\tcontrols.push_back(c);\n> +\t\t\t}\n> +\t\t\t\tbreak;\n> +\t\t\tcase V4L2_CTRL_TYPE_U16:\n> +\t\t\t{\n> +\t\t\t\tV4L2U16Control *c =\n> +\t\t\t\t\tnew V4L2U16Control(v4l2_ctrl->id, size,\n> +\t\t\t\t\t\t\t   v4l2_ctrl->p_u16);\n> +\t\t\t\tcontrols.push_back(c);\n> +\t\t\t}\n> +\t\t\t\tbreak;\n> +\t\t\tcase V4L2_CTRL_TYPE_U32:\n> +\t\t\t{\n> +\t\t\t\tV4L2U32Control *c =\n> +\t\t\t\t\tnew V4L2U32Control(v4l2_ctrl->id, size,\n> +\t\t\t\t\t\t\t  v4l2_ctrl->p_u32);\n> +\t\t\t\tcontrols.push_back(c);\n> +\t\t\t}\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tLOG(V4L2Base, Error)\n> +\t\t\t\t\t<< \"Compound control types not supported: \"\n> +\t\t\t\t\t<< std::hex << qext_ctrl.type;\n> +\t\t\t\treturn std::vector<V4L2Control *> {};\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\t/* Control types without payload. */\n> +\t\t\tswitch (qext_ctrl.type) {\n> +\t\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\t\t\t{\n> +\t\t\t\tV4L2Int64Control *c =\n> +\t\t\t\t\tnew V4L2Int64Control(v4l2_ctrl->id,\n> +\t\t\t\t\t\t\t     v4l2_ctrl->value64);\n> +\t\t\t\tcontrols.push_back(c);\n\nI think this could be a\n\tcontrols.emplace(V4L2Int64Control(v4l2_ctrl->id,\n\t\t\t\t\t  v4l2_ctrl->value64);\n\nif you prefer that style ...\n\nAha - except you have a vector of pointers, rather than a vector of\nobjects ...\n\n\n\n> +\t\t\t}\n> +\t\t\t\tbreak;\n> +\t\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> +\t\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> +\t\t\tcase V4L2_CTRL_TYPE_MENU:\n> +\t\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> +\t\t\t{\n> +\t\t\t\tV4L2IntControl *c =\n> +\t\t\t\t\tnew V4L2IntControl(v4l2_ctrl->id,\n> +\t\t\t\t\t\t\t   v4l2_ctrl->value);\n> +\t\t\t\tcontrols.push_back(c);\n> +\t\t\t}\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tLOG(V4L2Base, Error)\n> +\t\t\t\t\t<< \"Invalid non-compound control type :\"\n> +\t\t\t\t\t<< std::hex << qext_ctrl.type;\n> +\t\t\t\treturn std::vector<V4L2Control *> {};\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\treturn controls;\n> +}\n> +\n> +/**\n> + * \\brief Get the value of control with id \\a ctrl_id\n> + * \\param ctrl_id The control id\n> + * \\return The V4L2Control which represent the value of the control on success\n> + * nullptr otherwise\n> + */\n> +V4L2Control *V4L2Base::getControl(unsigned int ctrl_id)\n> +{\n> +\tstd::vector<unsigned int> v = { ctrl_id, };\n> +\tstd::vector<V4L2Control *> ctrls = getControls(v);\n> +\n> +\tif (ctrls.empty())\n> +\t\treturn nullptr;\n> +\n> +\treturn ctrls[0];\n> +}\n> +\n> +/**\n> + * \\brief Apply a list of controls to the device\n> + * \\param ctrls The list of controls to apply\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2Base::setControls(std::vector<V4L2Control *> &ctrls)\n> +{\n> +\tunsigned int count = ctrls.size();\n> +\tif (!count)\n> +\t\treturn -EINVAL;\n> +\n> +\tstruct v4l2_ext_control *v4l2_ctrls = static_cast<struct v4l2_ext_control *>\n> +\t\t\t\t\t      (new struct v4l2_ext_control[count]);\n> +\tif (!v4l2_ctrls)\n> +\t\treturn -ENOMEM;\n> +\n> +\t/*\n> +\t * Query each control in the vector to get back its type and expected\n> +\t * size and set the new desired value in each v4l2_ext_control member.\n> +\t */\n> +\tfor (unsigned int i = 0; i < count; ++i) {\n> +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> +\t\tV4L2Control *ctrl = ctrls[i];\n> +\n> +\t\tint ret = queryControl(ctrl->id(), &qext_ctrl);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tv4l2_ctrl->id = ctrl->id();\n> +\t\tv4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;\n> +\n> +\t\tif (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> +\t\t\t/** \\todo: Support set controls with payload. */\n> +\t\t\tLOG(V4L2Base, Error)\n> +\t\t\t\t<< \"Controls with payload not supported\";\n> +\t\t\treturn -EINVAL;\n> +\t\t} else {\n> +\t\t\tswitch (qext_ctrl.type) {\n> +\t\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\t\t\t{\n> +\t\t\t\tV4L2Int64Control *c =\n> +\t\t\t\t\tstatic_cast<V4L2Int64Control *>(ctrl);\n\n\nIt's these static_casts that feel a bit horrible to me. In my\nimplementation there is just single class, so there is no casting necessary,\n\n> +\t\t\t\tv4l2_ctrl->value64 = c->value();\n\nHowever, this would be c->getInteger64();\n\n\n> +\t\t\t}\n> +\t\t\t\tbreak;\n> +\t\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> +\t\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> +\t\t\tcase V4L2_CTRL_TYPE_MENU:\n> +\t\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> +\t\t\t{\n> +\t\t\t\tV4L2IntControl *c =\n> +\t\t\t\t\tstatic_cast<V4L2IntControl *>(ctrl);\n> +\t\t\t\tv4l2_ctrl->value = c->value();\n\nand getInteger(); (although getBoolean() can be separated as well);\n\nAs we know the types in this switch, I wonder if there is much\ndifference int taste between the two approaches...\n\n\n> +\t\t\t}\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tLOG(V4L2Base, Error)\n> +\t\t\t\t\t<< \"Invalid non-compound control type :\"\n> +\t\t\t\t\t<< qext_ctrl.type;\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\tstruct v4l2_ext_controls v4l2_ext_ctrls = {};\n> +\tv4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> +\tv4l2_ext_ctrls.count = count;\n> +\tv4l2_ext_ctrls.controls = v4l2_ctrls;\n> +\n> +\tint ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(V4L2Base, Error)\n> +\t\t\t<< \"Unable to set extended controls: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Apply the control \\a ctrl to the device\n> + * \\param ctrl The control to apply to the device\n> + * \\return 0 on success, a negative error code otherwise\n> + */\n> +int V4L2Base::setControl(V4L2Control *ctrl)\n> +{\n> +\tstd::vector<V4L2Control *> v = { ctrl, };\n> +\treturn setControls(v);\n> +}\n> +\n>  /**\n>   * \\var V4L2Base::fd_\n>   * \\brief The V4L2 device or subdevice device node file descriptor\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 243F560BD7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  3 Jun 2019 14:33:02 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 79CDE530;\n\tMon,  3 Jun 2019 14:33:01 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1559565181;\n\tbh=+IlGJpp37Vd5qzo49sluDjHe+1Y5ElpSbq3AHZbuqQc=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=AVP3NKIdQpAOQME7QEuhZcmWlBol1WzgKelhxUv7A4Q4aDsBAXnGMr9pJqf+LqXhG\n\tI535N65tqi4c5a4DqTNLXGxV91D6RozUlyUTIvb9aKr+sFgOfD2GiaJ3z75WwytNlO\n\tljjHOvc9LnYCsADN3Ag0Rugb282ZMtP+EL+TH0xg=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org","References":"<20190602130435.18780-1-jacopo@jmondi.org>\n\t<20190602130435.18780-5-jacopo@jmondi.org>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<5742d7bb-4c09-5e70-f3b7-81cac34c5247@ideasonboard.com>","Date":"Mon, 3 Jun 2019 13:32:58 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190602130435.18780-5-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: v4l2_base: Add V4L2\n\tcontrol support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 03 Jun 2019 12:33:02 -0000"}},{"id":1791,"web_url":"https://patchwork.libcamera.org/comment/1791/","msgid":"<20190608160708.GA23730@pendragon.ideasonboard.com>","date":"2019-06-08T16:07:08","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: v4l2_base: Add V4L2\n\tcontrol support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Jun 03, 2019 at 01:32:58PM +0100, Kieran Bingham wrote:\n> Hi Jacopo,\n> \n> Some thoughts out loud inline....\n> \n> On 02/06/2019 14:04, Jacopo Mondi wrote:\n> > Implement operations to set and get V4L2 controls in the V4L2Base class.\n> > The operations allow to set and get a single or a list of controls.\n> > \n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/libcamera/include/v4l2_base.h |  12 ++\n> >  src/libcamera/v4l2_base.cpp       | 290 ++++++++++++++++++++++++++++++\n> >  2 files changed, 302 insertions(+)\n> > \n> > diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> > index 2fda81a960d2..7d4e238dadfa 100644\n> > --- a/src/libcamera/include/v4l2_base.h\n> > +++ b/src/libcamera/include/v4l2_base.h\n> > @@ -9,6 +9,8 @@\n> >  \n> >  #include <vector>\n> >  \n> > +#include <v4l2_controls.h>\n\nThis should be \"v4l2_controls.h\", but you can simply forward-declare the\nV4L2Control class instead.\n\n> > +\n> >  namespace libcamera {\n> >  \n> >  class V4L2Base\n> > @@ -22,8 +24,18 @@ public:\n> >  \tvirtual void close() = 0;\n> >  \tbool isOpen() const;\n> >  \n> > +\tstd::vector<V4L2Control *> getControls(std::vector<unsigned int> &ctrl_ids);\n\nA vector of pointer isn't very nice as it requires the caller to delete\neach item. I would prefer passing a reference to a vector (or map) of\nvalues to the function, and having it fill the values.\n\n> In my controls, I pass a list <well a map, or a set; I have two\n> implementations currently> of the actual ControlValue objects, which get\n> filled in.\n>\n> I wonder if there will be much differences between the two interface\n> designs.\n\nOne big difference is that a vector is ordered while a map or set isn't.\nThe order in which controls are set can make a difference.\n\n> > +\tV4L2Control *getControl(unsigned int ctrl_id);\n> > +\tint setControls(std::vector<V4L2Control *> &ctrls);\n> > +\tint setControl(V4L2Control *ctrl);\n> \n> I think I'd group those as getControls setControls, and then getControl,\n> setControl, so that they are grouped by 'functionality' but that might\n> not be a preferred sorting... so either way.\n\nI would prefer that as well.\n\n> > +\n> >  protected:\n> >  \tint fd_;\n> > +\n> > +private:\n> > +\tint queryControl(unsigned int ctrl_id,\n> > +\t\t\t struct v4l2_query_ext_ctrl *qext_ctrl);\n> > +\n> >  };\n> >  \n> >  }; /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp\n> > index 7d05a3c82e4d..423a4d84e276 100644\n> > --- a/src/libcamera/v4l2_base.cpp\n> > +++ b/src/libcamera/v4l2_base.cpp\n> > @@ -5,7 +5,13 @@\n> >   * v4l2_base.cpp - Common base for V4L2 devices and subdevices\n> >   */\n> >  \n> > +#include <linux/v4l2-subdev.h>\n> > +#include <linux/videodev2.h>\n> > +#include <sys/ioctl.h>\n> > +\n> > +#include \"log.h\"\n> >  #include \"v4l2_base.h\"\n\nThe issue comes from a previous patch, but v4l2_base.h should be the\nvery first header included, to help ensuring it is self-contained.\n\n> > +#include \"v4l2_controls.h\"\n> >  \n> >  /**\n> >   * \\file v4l2_base.h\n> > @@ -14,6 +20,8 @@\n> >  \n> >  namespace libcamera {\n> >  \n> > +LOG_DEFINE_CATEGORY(V4L2Base)\n> > +\n> >  /**\n> >   * \\class V4L2Base\n> >   * \\brief Base class for V4L2Device and V4L2Subdevice\n> > @@ -31,6 +39,7 @@ namespace libcamera {\n> >   */\n> >  \n> >  /**\n> > + * \\fn V4L2Base::open()\n\nDoes this belong to this patch ? Same for the next method.\n\n> >   * \\brief Open a V4L2 device or subdevice\n> >   *\n> >   * Pure virtual method to be implemented by the derived classes.\n> > @@ -39,6 +48,7 @@ namespace libcamera {\n> >   */\n> >  \n> >  /**\n> > + * \\fn V4L2Base::close()\n> >   * \\brief Close the device or subdevice\n> >   *\n> >   * Pure virtual method to be implemented by the derived classes.\n> > @@ -53,6 +63,286 @@ bool V4L2Base::isOpen() const\n> >  \treturn fd_ != -1;\n> >  }\n> >  \n> > +int V4L2Base::queryControl(unsigned int id,\n> > +\t\t\t   struct v4l2_query_ext_ctrl *qext_ctrl)\n> > +{\n> > +\tqext_ctrl->id = id;\n> > +\n> > +\tint ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(V4L2Base, Error)\n> > +\t\t\t<< \"Unable to query extended control 0x\"\n\ns/extended //\n\n> > +\t\t\t<< std::hex << qext_ctrl->id\n> > +\t\t\t<< \": \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tif (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {\n> > +\t\tLOG(V4L2Base, Error)\n> > +\t\t\t<< \"Extended control 0x\" << std::hex\n> > +\t\t\t<< qext_ctrl->id << \" is disabled\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n\nI would handle this in the caller, it's not a queryControl() error as\nsuch.\n\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Get values of a list of control IDs\n> > + * \\param ctrl_ids The list of control IDs to retrieve value of\n> > + * \\return A list of V4L2Control on success or a empty list otherwise\n> > + */\n> > +std::vector<V4L2Control *> V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids)\n> > +{\n> > +\tunsigned int count = ctrl_ids.size();\n> > +\tif (!count)\n> > +\t\treturn std::vector<V4L2Control *> {};\n> > +\n> > +\tstruct v4l2_ext_control *v4l2_ctrls = static_cast<struct v4l2_ext_control *>\n> > +\t\t\t\t\t      (new struct v4l2_ext_control[count]);\n> > +\tif (!v4l2_ctrls)\n> > +\t\treturn std::vector<V4L2Control *> {};\n> > +\n> > +\t/*\n> > +\t * Query each control in the vector to get back its type and expected\n> > +\t * size in order to get its current value.\n> > +\t */\n> > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> > +\t\tunsigned int ctrl_id = ctrl_ids[i];\n> > +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> > +\n> > +\t\tint ret = queryControl(ctrl_id, &qext_ctrl);\n> > +\t\tif (ret)\n> > +\t\t\treturn std::vector<V4L2Control *> {};\n> > +\n> > +\t\tv4l2_ctrl->id = ctrl_id;\n> > +\t\tv4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;\n> > +\n> > +\t\t/* Reserve memory for controls with payload. */\n> > +\t\tif (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tswitch (qext_ctrl.type) {\n> > +\t\tcase V4L2_CTRL_TYPE_U8:\n> > +\t\t\tv4l2_ctrl->p_u8 = static_cast<uint8_t *>\n> > +\t\t\t\t\t  (new uint8_t[v4l2_ctrl->size]);\n> > +\t\t\tbreak;\n> > +\t\tcase V4L2_CTRL_TYPE_U16:\n> > +\t\t\tv4l2_ctrl->p_u16 = static_cast<uint16_t *>\n> > +\t\t\t\t\t  (new uint16_t[v4l2_ctrl->size]);\n> > +\t\t\tbreak;\n> > +\t\tcase V4L2_CTRL_TYPE_U32:\n> > +\t\t\tv4l2_ctrl->p_u32 = static_cast<uint32_t *>\n> > +\t\t\t\t\t  (new uint32_t[v4l2_ctrl->size]);\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tstruct v4l2_ext_controls v4l2_ext_ctrls = {};\n> > +\tv4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > +\tv4l2_ext_ctrls.count = count;\n> > +\tv4l2_ext_ctrls.controls = v4l2_ctrls;\n> > +\n> > +\tint ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(V4L2Base, Error)\n> > +\t\t\t<< \"Unable to get extended controls: \" << strerror(-ret);\n> > +\t\t\treturn std::vector<V4L2Control *> {};\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * For each requested control, create a V4L2Control which contains\n> > +\t * the current value and store it in the vector returned to the caller.\n> > +\t */\n> > +\tstd::vector<V4L2Control *> controls = {};\n> > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> > +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> > +\n> > +\t\tint ret = queryControl(v4l2_ctrl->id, &qext_ctrl);\n> > +\t\tif (ret)\n> > +\t\t\treturn std::vector<V4L2Control *> {};\n> > +\n> > +\t\tunsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;\n> > +\t\tif (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> > +\t\t\t/* Control types with payload */\n> > +\t\t\tswitch (qext_ctrl.type) {\n> > +\t\t\tcase V4L2_CTRL_TYPE_U8:\n> > +\t\t\t{\n> > +\t\t\t\tV4L2U8Control *c =\n> > +\t\t\t\t\tnew V4L2U8Control(v4l2_ctrl->id, size,\n> > +\t\t\t\t\t\t\t  v4l2_ctrl->p_u8);\n> > +\t\t\t\tcontrols.push_back(c);\n> > +\t\t\t}\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase V4L2_CTRL_TYPE_U16:\n> > +\t\t\t{\n> > +\t\t\t\tV4L2U16Control *c =\n> > +\t\t\t\t\tnew V4L2U16Control(v4l2_ctrl->id, size,\n> > +\t\t\t\t\t\t\t   v4l2_ctrl->p_u16);\n> > +\t\t\t\tcontrols.push_back(c);\n> > +\t\t\t}\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase V4L2_CTRL_TYPE_U32:\n> > +\t\t\t{\n> > +\t\t\t\tV4L2U32Control *c =\n> > +\t\t\t\t\tnew V4L2U32Control(v4l2_ctrl->id, size,\n> > +\t\t\t\t\t\t\t  v4l2_ctrl->p_u32);\n> > +\t\t\t\tcontrols.push_back(c);\n> > +\t\t\t}\n> > +\t\t\t\tbreak;\n> > +\t\t\tdefault:\n> > +\t\t\t\tLOG(V4L2Base, Error)\n> > +\t\t\t\t\t<< \"Compound control types not supported: \"\n> > +\t\t\t\t\t<< std::hex << qext_ctrl.type;\n> > +\t\t\t\treturn std::vector<V4L2Control *> {};\n> > +\t\t\t}\n> > +\t\t} else {\n> > +\t\t\t/* Control types without payload. */\n> > +\t\t\tswitch (qext_ctrl.type) {\n> > +\t\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > +\t\t\t{\n> > +\t\t\t\tV4L2Int64Control *c =\n> > +\t\t\t\t\tnew V4L2Int64Control(v4l2_ctrl->id,\n> > +\t\t\t\t\t\t\t     v4l2_ctrl->value64);\n> > +\t\t\t\tcontrols.push_back(c);\n> \n> I think this could be a\n> \tcontrols.emplace(V4L2Int64Control(v4l2_ctrl->id,\n> \t\t\t\t\t  v4l2_ctrl->value64);\n> \n> if you prefer that style ...\n> \n> Aha - except you have a vector of pointers, rather than a vector of\n> objects ...\n> \n> > +\t\t\t}\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> > +\t\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > +\t\t\tcase V4L2_CTRL_TYPE_MENU:\n> > +\t\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> > +\t\t\t{\n> > +\t\t\t\tV4L2IntControl *c =\n> > +\t\t\t\t\tnew V4L2IntControl(v4l2_ctrl->id,\n> > +\t\t\t\t\t\t\t   v4l2_ctrl->value);\n> > +\t\t\t\tcontrols.push_back(c);\n> > +\t\t\t}\n> > +\t\t\t\tbreak;\n> > +\t\t\tdefault:\n> > +\t\t\t\tLOG(V4L2Base, Error)\n> > +\t\t\t\t\t<< \"Invalid non-compound control type :\"\n> > +\t\t\t\t\t<< std::hex << qext_ctrl.type;\n> > +\t\t\t\treturn std::vector<V4L2Control *> {};\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn controls;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Get the value of control with id \\a ctrl_id\n> > + * \\param ctrl_id The control id\n> > + * \\return The V4L2Control which represent the value of the control on success\n> > + * nullptr otherwise\n> > + */\n> > +V4L2Control *V4L2Base::getControl(unsigned int ctrl_id)\n> > +{\n> > +\tstd::vector<unsigned int> v = { ctrl_id, };\n> > +\tstd::vector<V4L2Control *> ctrls = getControls(v);\n> > +\n> > +\tif (ctrls.empty())\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn ctrls[0];\n> > +}\n> > +\n> > +/**\n> > + * \\brief Apply a list of controls to the device\n> > + * \\param ctrls The list of controls to apply\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int V4L2Base::setControls(std::vector<V4L2Control *> &ctrls)\n> > +{\n> > +\tunsigned int count = ctrls.size();\n\nsize_t instead of unsigned int ?\n\n> > +\tif (!count)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tstruct v4l2_ext_control *v4l2_ctrls = static_cast<struct v4l2_ext_control *>\n> > +\t\t\t\t\t      (new struct v4l2_ext_control[count]);\n\nDo you need the cast ?\n\n> > +\tif (!v4l2_ctrls)\n> > +\t\treturn -ENOMEM;\n\nnew() will throw an exception if it can't construct the object, so\nthere's no need to check the return value.\n\n> > +\n> > +\t/*\n> > +\t * Query each control in the vector to get back its type and expected\n> > +\t * size and set the new desired value in each v4l2_ext_control member.\n> > +\t */\n> > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> > +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> > +\t\tV4L2Control *ctrl = ctrls[i];\n> > +\n> > +\t\tint ret = queryControl(ctrl->id(), &qext_ctrl);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n\nIt's annoying to have to query each control every time. We should either\nenumerate all of them when opening the device, or query them at runtime\nbut cache the control information.\n\n> > +\n> > +\t\tv4l2_ctrl->id = ctrl->id();\n> > +\t\tv4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;\n> > +\n> > +\t\tif (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> > +\t\t\t/** \\todo: Support set controls with payload. */\n> > +\t\t\tLOG(V4L2Base, Error)\n> > +\t\t\t\t<< \"Controls with payload not supported\";\n> > +\t\t\treturn -EINVAL;\n> > +\t\t} else {\n> > +\t\t\tswitch (qext_ctrl.type) {\n> > +\t\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > +\t\t\t{\n> > +\t\t\t\tV4L2Int64Control *c =\n> > +\t\t\t\t\tstatic_cast<V4L2Int64Control *>(ctrl);\n> \n> It's these static_casts that feel a bit horrible to me. In my\n> implementation there is just single class, so there is no casting necessary,\n\nI don't like the casts much either, especially given that you cast based\non the type returned by the kernel, which could possibly not match the\ntype of the control as set by the application. This would likely lead to\nmemory corruption and crashes.\n\n> > +\t\t\t\tv4l2_ctrl->value64 = c->value();\n> \n> However, this would be c->getInteger64();\n> \n> > +\t\t\t}\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> > +\t\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > +\t\t\tcase V4L2_CTRL_TYPE_MENU:\n> > +\t\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> > +\t\t\t{\n> > +\t\t\t\tV4L2IntControl *c =\n> > +\t\t\t\t\tstatic_cast<V4L2IntControl *>(ctrl);\n> > +\t\t\t\tv4l2_ctrl->value = c->value();\n> \n> and getInteger(); (although getBoolean() can be separated as well);\n> \n> As we know the types in this switch, I wonder if there is much\n> difference int taste between the two approaches...\n> \n> > +\t\t\t}\n> > +\t\t\t\tbreak;\n> > +\t\t\tdefault:\n> > +\t\t\t\tLOG(V4L2Base, Error)\n> > +\t\t\t\t\t<< \"Invalid non-compound control type :\"\n> > +\t\t\t\t\t<< qext_ctrl.type;\n> > +\t\t\t\treturn -EINVAL;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tstruct v4l2_ext_controls v4l2_ext_ctrls = {};\n> > +\tv4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > +\tv4l2_ext_ctrls.count = count;\n> > +\tv4l2_ext_ctrls.controls = v4l2_ctrls;\n> > +\n> > +\tint ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(V4L2Base, Error)\n> > +\t\t\t<< \"Unable to set extended controls: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Apply the control \\a ctrl to the device\n> > + * \\param ctrl The control to apply to the device\n> > + * \\return 0 on success, a negative error code otherwise\n> > + */\n> > +int V4L2Base::setControl(V4L2Control *ctrl)\n> > +{\n> > +\tstd::vector<V4L2Control *> v = { ctrl, };\n> > +\treturn setControls(v);\n> > +}\n> > +\n> >  /**\n> >   * \\var V4L2Base::fd_\n> >   * \\brief The V4L2 device or subdevice device node file descriptor","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78F5A6BDE5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  8 Jun 2019 18:07:23 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:a03f:44f0:8500:ca05:8177:199c:fed4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F322C5D;\n\tSat,  8 Jun 2019 18:07:22 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560010043;\n\tbh=04RSYrLe6mdvDYEPtYRKmFV+fPagi5lU7PdmCj3hFoc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PzBVY+uHcMLgXjN4nvsaWFucGEGdk+XuW8wAmEjf0mjwRmr0UguByapDwKRKzALzS\n\tIKrPZgEE3n1iaYZQ6af2cMI/z1e/09dYrqnQOPiagzeYvEc3RnZVymOt7tAXOpYVqF\n\tjlgHe35j2oHEqds1SGFDhfhpehlwnMDW8XVKZF7M=","Date":"Sat, 8 Jun 2019 19:07:08 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190608160708.GA23730@pendragon.ideasonboard.com>","References":"<20190602130435.18780-1-jacopo@jmondi.org>\n\t<20190602130435.18780-5-jacopo@jmondi.org>\n\t<5742d7bb-4c09-5e70-f3b7-81cac34c5247@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<5742d7bb-4c09-5e70-f3b7-81cac34c5247@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: v4l2_base: Add V4L2\n\tcontrol support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 08 Jun 2019 16:07:23 -0000"}},{"id":1818,"web_url":"https://patchwork.libcamera.org/comment/1818/","msgid":"<20190610073842.gxw3oinjbmpvxfdm@uno.localdomain>","date":"2019-06-10T07:38:42","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: v4l2_base: Add V4L2\n\tcontrol support","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran, Laurent,\n\nOn Sat, Jun 08, 2019 at 07:07:08PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Jun 03, 2019 at 01:32:58PM +0100, Kieran Bingham wrote:\n> > Hi Jacopo,\n> >\n> > Some thoughts out loud inline....\n> >\n> > On 02/06/2019 14:04, Jacopo Mondi wrote:\n> > > Implement operations to set and get V4L2 controls in the V4L2Base class.\n> > > The operations allow to set and get a single or a list of controls.\n> > >\n> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > >  src/libcamera/include/v4l2_base.h |  12 ++\n> > >  src/libcamera/v4l2_base.cpp       | 290 ++++++++++++++++++++++++++++++\n> > >  2 files changed, 302 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> > > index 2fda81a960d2..7d4e238dadfa 100644\n> > > --- a/src/libcamera/include/v4l2_base.h\n> > > +++ b/src/libcamera/include/v4l2_base.h\n> > > @@ -9,6 +9,8 @@\n> > >\n> > >  #include <vector>\n> > >\n> > > +#include <v4l2_controls.h>\n>\n> This should be \"v4l2_controls.h\", but you can simply forward-declare the\n> V4L2Control class instead.\n>\n> > > +\n> > >  namespace libcamera {\n> > >\n> > >  class V4L2Base\n> > > @@ -22,8 +24,18 @@ public:\n> > >  \tvirtual void close() = 0;\n> > >  \tbool isOpen() const;\n> > >\n> > > +\tstd::vector<V4L2Control *> getControls(std::vector<unsigned int> &ctrl_ids);\n>\n> A vector of pointer isn't very nice as it requires the caller to delete\n> each item. I would prefer passing a reference to a vector (or map) of\n> values to the function, and having it fill the values.\n>\n\nI thought about it, but how would you define the state of a Control ?\nWould you create a control with id but without any associated value ?\nAnd what if you call value() on it? I fear keeping track of that is a\npain, and I preferred to have controls that once created always have a\nvalue associated.\n\nI can reconsider it though.\n\n> > In my controls, I pass a list <well a map, or a set; I have two\n> > implementations currently> of the actual ControlValue objects, which get\n> > filled in.\n> >\n> > I wonder if there will be much differences between the two interface\n> > designs.\n>\n> One big difference is that a vector is ordered while a map or set isn't.\n> The order in which controls are set can make a difference.\n>\n\nI think Kieran referred to the \"container of controls to fill\" vs the\n\"get control ids and return a container of controls\" different\napproaches.\n\n> > > +\tV4L2Control *getControl(unsigned int ctrl_id);\n> > > +\tint setControls(std::vector<V4L2Control *> &ctrls);\n> > > +\tint setControl(V4L2Control *ctrl);\n> >\n> > I think I'd group those as getControls setControls, and then getControl,\n> > setControl, so that they are grouped by 'functionality' but that might\n> > not be a preferred sorting... so either way.\n>\n> I would prefer that as well.\n>\n\nYup.\n\n> > > +\n> > >  protected:\n> > >  \tint fd_;\n> > > +\n> > > +private:\n> > > +\tint queryControl(unsigned int ctrl_id,\n> > > +\t\t\t struct v4l2_query_ext_ctrl *qext_ctrl);\n> > > +\n> > >  };\n> > >\n> > >  }; /* namespace libcamera */\n> > > diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp\n> > > index 7d05a3c82e4d..423a4d84e276 100644\n> > > --- a/src/libcamera/v4l2_base.cpp\n> > > +++ b/src/libcamera/v4l2_base.cpp\n> > > @@ -5,7 +5,13 @@\n> > >   * v4l2_base.cpp - Common base for V4L2 devices and subdevices\n> > >   */\n> > >\n> > > +#include <linux/v4l2-subdev.h>\n> > > +#include <linux/videodev2.h>\n> > > +#include <sys/ioctl.h>\n> > > +\n> > > +#include \"log.h\"\n> > >  #include \"v4l2_base.h\"\n>\n> The issue comes from a previous patch, but v4l2_base.h should be the\n> very first header included, to help ensuring it is self-contained.\n>\n> > > +#include \"v4l2_controls.h\"\n> > >\n> > >  /**\n> > >   * \\file v4l2_base.h\n> > > @@ -14,6 +20,8 @@\n> > >\n> > >  namespace libcamera {\n> > >\n> > > +LOG_DEFINE_CATEGORY(V4L2Base)\n> > > +\n> > >  /**\n> > >   * \\class V4L2Base\n> > >   * \\brief Base class for V4L2Device and V4L2Subdevice\n> > > @@ -31,6 +39,7 @@ namespace libcamera {\n> > >   */\n> > >\n> > >  /**\n> > > + * \\fn V4L2Base::open()\n>\n> Does this belong to this patch ? Same for the next method.\n>\n\nNo, sorry, bad rebasing\n\n> > >   * \\brief Open a V4L2 device or subdevice\n> > >   *\n> > >   * Pure virtual method to be implemented by the derived classes.\n> > > @@ -39,6 +48,7 @@ namespace libcamera {\n> > >   */\n> > >\n> > >  /**\n> > > + * \\fn V4L2Base::close()\n> > >   * \\brief Close the device or subdevice\n> > >   *\n> > >   * Pure virtual method to be implemented by the derived classes.\n> > > @@ -53,6 +63,286 @@ bool V4L2Base::isOpen() const\n> > >  \treturn fd_ != -1;\n> > >  }\n> > >\n> > > +int V4L2Base::queryControl(unsigned int id,\n> > > +\t\t\t   struct v4l2_query_ext_ctrl *qext_ctrl)\n> > > +{\n> > > +\tqext_ctrl->id = id;\n> > > +\n> > > +\tint ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);\n> > > +\tif (ret) {\n> > > +\t\tret = -errno;\n> > > +\t\tLOG(V4L2Base, Error)\n> > > +\t\t\t<< \"Unable to query extended control 0x\"\n>\n> s/extended //\n>\n> > > +\t\t\t<< std::hex << qext_ctrl->id\n> > > +\t\t\t<< \": \" << strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tif (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {\n> > > +\t\tLOG(V4L2Base, Error)\n> > > +\t\t\t<< \"Extended control 0x\" << std::hex\n> > > +\t\t\t<< qext_ctrl->id << \" is disabled\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n>\n> I would handle this in the caller, it's not a queryControl() error as\n> such.\n>\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Get values of a list of control IDs\n> > > + * \\param ctrl_ids The list of control IDs to retrieve value of\n> > > + * \\return A list of V4L2Control on success or a empty list otherwise\n> > > + */\n> > > +std::vector<V4L2Control *> V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids)\n> > > +{\n> > > +\tunsigned int count = ctrl_ids.size();\n> > > +\tif (!count)\n> > > +\t\treturn std::vector<V4L2Control *> {};\n> > > +\n> > > +\tstruct v4l2_ext_control *v4l2_ctrls = static_cast<struct v4l2_ext_control *>\n> > > +\t\t\t\t\t      (new struct v4l2_ext_control[count]);\n> > > +\tif (!v4l2_ctrls)\n> > > +\t\treturn std::vector<V4L2Control *> {};\n> > > +\n> > > +\t/*\n> > > +\t * Query each control in the vector to get back its type and expected\n> > > +\t * size in order to get its current value.\n> > > +\t */\n> > > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > > +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> > > +\t\tunsigned int ctrl_id = ctrl_ids[i];\n> > > +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> > > +\n> > > +\t\tint ret = queryControl(ctrl_id, &qext_ctrl);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn std::vector<V4L2Control *> {};\n> > > +\n> > > +\t\tv4l2_ctrl->id = ctrl_id;\n> > > +\t\tv4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;\n> > > +\n> > > +\t\t/* Reserve memory for controls with payload. */\n> > > +\t\tif (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tswitch (qext_ctrl.type) {\n> > > +\t\tcase V4L2_CTRL_TYPE_U8:\n> > > +\t\t\tv4l2_ctrl->p_u8 = static_cast<uint8_t *>\n> > > +\t\t\t\t\t  (new uint8_t[v4l2_ctrl->size]);\n> > > +\t\t\tbreak;\n> > > +\t\tcase V4L2_CTRL_TYPE_U16:\n> > > +\t\t\tv4l2_ctrl->p_u16 = static_cast<uint16_t *>\n> > > +\t\t\t\t\t  (new uint16_t[v4l2_ctrl->size]);\n> > > +\t\t\tbreak;\n> > > +\t\tcase V4L2_CTRL_TYPE_U32:\n> > > +\t\t\tv4l2_ctrl->p_u32 = static_cast<uint32_t *>\n> > > +\t\t\t\t\t  (new uint32_t[v4l2_ctrl->size]);\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tstruct v4l2_ext_controls v4l2_ext_ctrls = {};\n> > > +\tv4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > > +\tv4l2_ext_ctrls.count = count;\n> > > +\tv4l2_ext_ctrls.controls = v4l2_ctrls;\n> > > +\n> > > +\tint ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);\n> > > +\tif (ret) {\n> > > +\t\tret = -errno;\n> > > +\t\tLOG(V4L2Base, Error)\n> > > +\t\t\t<< \"Unable to get extended controls: \" << strerror(-ret);\n> > > +\t\t\treturn std::vector<V4L2Control *> {};\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * For each requested control, create a V4L2Control which contains\n> > > +\t * the current value and store it in the vector returned to the caller.\n> > > +\t */\n> > > +\tstd::vector<V4L2Control *> controls = {};\n> > > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > > +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> > > +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> > > +\n> > > +\t\tint ret = queryControl(v4l2_ctrl->id, &qext_ctrl);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn std::vector<V4L2Control *> {};\n> > > +\n> > > +\t\tunsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;\n> > > +\t\tif (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> > > +\t\t\t/* Control types with payload */\n> > > +\t\t\tswitch (qext_ctrl.type) {\n> > > +\t\t\tcase V4L2_CTRL_TYPE_U8:\n> > > +\t\t\t{\n> > > +\t\t\t\tV4L2U8Control *c =\n> > > +\t\t\t\t\tnew V4L2U8Control(v4l2_ctrl->id, size,\n> > > +\t\t\t\t\t\t\t  v4l2_ctrl->p_u8);\n> > > +\t\t\t\tcontrols.push_back(c);\n> > > +\t\t\t}\n> > > +\t\t\t\tbreak;\n> > > +\t\t\tcase V4L2_CTRL_TYPE_U16:\n> > > +\t\t\t{\n> > > +\t\t\t\tV4L2U16Control *c =\n> > > +\t\t\t\t\tnew V4L2U16Control(v4l2_ctrl->id, size,\n> > > +\t\t\t\t\t\t\t   v4l2_ctrl->p_u16);\n> > > +\t\t\t\tcontrols.push_back(c);\n> > > +\t\t\t}\n> > > +\t\t\t\tbreak;\n> > > +\t\t\tcase V4L2_CTRL_TYPE_U32:\n> > > +\t\t\t{\n> > > +\t\t\t\tV4L2U32Control *c =\n> > > +\t\t\t\t\tnew V4L2U32Control(v4l2_ctrl->id, size,\n> > > +\t\t\t\t\t\t\t  v4l2_ctrl->p_u32);\n> > > +\t\t\t\tcontrols.push_back(c);\n> > > +\t\t\t}\n> > > +\t\t\t\tbreak;\n> > > +\t\t\tdefault:\n> > > +\t\t\t\tLOG(V4L2Base, Error)\n> > > +\t\t\t\t\t<< \"Compound control types not supported: \"\n> > > +\t\t\t\t\t<< std::hex << qext_ctrl.type;\n> > > +\t\t\t\treturn std::vector<V4L2Control *> {};\n> > > +\t\t\t}\n> > > +\t\t} else {\n> > > +\t\t\t/* Control types without payload. */\n> > > +\t\t\tswitch (qext_ctrl.type) {\n> > > +\t\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > > +\t\t\t{\n> > > +\t\t\t\tV4L2Int64Control *c =\n> > > +\t\t\t\t\tnew V4L2Int64Control(v4l2_ctrl->id,\n> > > +\t\t\t\t\t\t\t     v4l2_ctrl->value64);\n> > > +\t\t\t\tcontrols.push_back(c);\n> >\n> > I think this could be a\n> > \tcontrols.emplace(V4L2Int64Control(v4l2_ctrl->id,\n> > \t\t\t\t\t  v4l2_ctrl->value64);\n> >\n> > if you prefer that style ...\n> >\n> > Aha - except you have a vector of pointers, rather than a vector of\n> > objects ...\n> >\n\nYes, I can't do that.\n\n> > > +\t\t\t}\n> > > +\t\t\t\tbreak;\n> > > +\t\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> > > +\t\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > > +\t\t\tcase V4L2_CTRL_TYPE_MENU:\n> > > +\t\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> > > +\t\t\t{\n> > > +\t\t\t\tV4L2IntControl *c =\n> > > +\t\t\t\t\tnew V4L2IntControl(v4l2_ctrl->id,\n> > > +\t\t\t\t\t\t\t   v4l2_ctrl->value);\n> > > +\t\t\t\tcontrols.push_back(c);\n> > > +\t\t\t}\n> > > +\t\t\t\tbreak;\n> > > +\t\t\tdefault:\n> > > +\t\t\t\tLOG(V4L2Base, Error)\n> > > +\t\t\t\t\t<< \"Invalid non-compound control type :\"\n> > > +\t\t\t\t\t<< std::hex << qext_ctrl.type;\n> > > +\t\t\t\treturn std::vector<V4L2Control *> {};\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\treturn controls;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Get the value of control with id \\a ctrl_id\n> > > + * \\param ctrl_id The control id\n> > > + * \\return The V4L2Control which represent the value of the control on success\n> > > + * nullptr otherwise\n> > > + */\n> > > +V4L2Control *V4L2Base::getControl(unsigned int ctrl_id)\n> > > +{\n> > > +\tstd::vector<unsigned int> v = { ctrl_id, };\n> > > +\tstd::vector<V4L2Control *> ctrls = getControls(v);\n> > > +\n> > > +\tif (ctrls.empty())\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\treturn ctrls[0];\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Apply a list of controls to the device\n> > > + * \\param ctrls The list of controls to apply\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int V4L2Base::setControls(std::vector<V4L2Control *> &ctrls)\n> > > +{\n> > > +\tunsigned int count = ctrls.size();\n>\n> size_t instead of unsigned int ?\n>\n> > > +\tif (!count)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tstruct v4l2_ext_control *v4l2_ctrls = static_cast<struct v4l2_ext_control *>\n> > > +\t\t\t\t\t      (new struct v4l2_ext_control[count]);\n>\n> Do you need the cast ?\n>\n> > > +\tif (!v4l2_ctrls)\n> > > +\t\treturn -ENOMEM;\n>\n> new() will throw an exception if it can't construct the object, so\n> there's no need to check the return value.\n>\n> > > +\n> > > +\t/*\n> > > +\t * Query each control in the vector to get back its type and expected\n> > > +\t * size and set the new desired value in each v4l2_ext_control member.\n> > > +\t */\n> > > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > > +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> > > +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> > > +\t\tV4L2Control *ctrl = ctrls[i];\n> > > +\n> > > +\t\tint ret = queryControl(ctrl->id(), &qext_ctrl);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n>\n> It's annoying to have to query each control every time. We should either\n> enumerate all of them when opening the device, or query them at runtime\n> but cache the control information.\n\nI'm not a fan of caching control info at open() time tbh. It's quite a\nrelevant amount of memory to be kept around. Indeed I call\nqueryControl twice for each control, and this is bad. I could cache\nthe information -inside this method- and reduce the number of calls to\n1, which seems acceptable to me.\n\n>\n> > > +\n> > > +\t\tv4l2_ctrl->id = ctrl->id();\n> > > +\t\tv4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;\n> > > +\n> > > +\t\tif (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> > > +\t\t\t/** \\todo: Support set controls with payload. */\n> > > +\t\t\tLOG(V4L2Base, Error)\n> > > +\t\t\t\t<< \"Controls with payload not supported\";\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t} else {\n> > > +\t\t\tswitch (qext_ctrl.type) {\n> > > +\t\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > > +\t\t\t{\n> > > +\t\t\t\tV4L2Int64Control *c =\n> > > +\t\t\t\t\tstatic_cast<V4L2Int64Control *>(ctrl);\n> >\n> > It's these static_casts that feel a bit horrible to me. In my\n> > implementation there is just single class, so there is no casting necessary,\n>\n> I don't like the casts much either, especially given that you cast based\n> on the type returned by the kernel, which could possibly not match the\n> type of the control as set by the application. This would likely lead to\n> memory corruption and crashes.\n\nI see. Wrapping the container as you suggested and let users interface\nwith that would probably mask the casting inside the V4L2Control\nclass, and it would alway happen on the control type as returned by\nthe kernel. That should be better...\n\n>\n> > > +\t\t\t\tv4l2_ctrl->value64 = c->value();\n> >\n> > However, this would be c->getInteger64();\n> >\n> > > +\t\t\t}\n> > > +\t\t\t\tbreak;\n> > > +\t\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> > > +\t\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > > +\t\t\tcase V4L2_CTRL_TYPE_MENU:\n> > > +\t\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> > > +\t\t\t{\n> > > +\t\t\t\tV4L2IntControl *c =\n> > > +\t\t\t\t\tstatic_cast<V4L2IntControl *>(ctrl);\n> > > +\t\t\t\tv4l2_ctrl->value = c->value();\n> >\n> > and getInteger(); (although getBoolean() can be separated as well);\n> >\n> > As we know the types in this switch, I wonder if there is much\n> > difference int taste between the two approaches...\n> >\n\nI think I should try to mask the casting in the V4L2Control class as\nmuch as I could. I'm still not sure this is completely possible\nthough.\n\nThanks\n   j\n\n> > > +\t\t\t}\n> > > +\t\t\t\tbreak;\n> > > +\t\t\tdefault:\n> > > +\t\t\t\tLOG(V4L2Base, Error)\n> > > +\t\t\t\t\t<< \"Invalid non-compound control type :\"\n> > > +\t\t\t\t\t<< qext_ctrl.type;\n> > > +\t\t\t\treturn -EINVAL;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tstruct v4l2_ext_controls v4l2_ext_ctrls = {};\n> > > +\tv4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > > +\tv4l2_ext_ctrls.count = count;\n> > > +\tv4l2_ext_ctrls.controls = v4l2_ctrls;\n> > > +\n> > > +\tint ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);\n> > > +\tif (ret) {\n> > > +\t\tret = -errno;\n> > > +\t\tLOG(V4L2Base, Error)\n> > > +\t\t\t<< \"Unable to set extended controls: \" << strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Apply the control \\a ctrl to the device\n> > > + * \\param ctrl The control to apply to the device\n> > > + * \\return 0 on success, a negative error code otherwise\n> > > + */\n> > > +int V4L2Base::setControl(V4L2Control *ctrl)\n> > > +{\n> > > +\tstd::vector<V4L2Control *> v = { ctrl, };\n> > > +\treturn setControls(v);\n> > > +}\n> > > +\n> > >  /**\n> > >   * \\var V4L2Base::fd_\n> > >   * \\brief The V4L2 device or subdevice device node file descriptor\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CAA3263772\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2019 09:37:29 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 1DD194000B;\n\tMon, 10 Jun 2019 07:37:28 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 10 Jun 2019 09:38:42 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190610073842.gxw3oinjbmpvxfdm@uno.localdomain>","References":"<20190602130435.18780-1-jacopo@jmondi.org>\n\t<20190602130435.18780-5-jacopo@jmondi.org>\n\t<5742d7bb-4c09-5e70-f3b7-81cac34c5247@ideasonboard.com>\n\t<20190608160708.GA23730@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"aob7gwgp5bxugjof\"","Content-Disposition":"inline","In-Reply-To":"<20190608160708.GA23730@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: v4l2_base: Add V4L2\n\tcontrol support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 10 Jun 2019 07:37:30 -0000"}},{"id":1822,"web_url":"https://patchwork.libcamera.org/comment/1822/","msgid":"<20190610083459.GL4806@pendragon.ideasonboard.com>","date":"2019-06-10T08:35:00","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: v4l2_base: Add V4L2\n\tcontrol support","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jun 10, 2019 at 09:38:42AM +0200, Jacopo Mondi wrote:\n> On Sat, Jun 08, 2019 at 07:07:08PM +0300, Laurent Pinchart wrote:\n> > On Mon, Jun 03, 2019 at 01:32:58PM +0100, Kieran Bingham wrote:\n> >> On 02/06/2019 14:04, Jacopo Mondi wrote:\n> >>> Implement operations to set and get V4L2 controls in the V4L2Base class.\n> >>> The operations allow to set and get a single or a list of controls.\n> >>>\n> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> >>> ---\n> >>>  src/libcamera/include/v4l2_base.h |  12 ++\n> >>>  src/libcamera/v4l2_base.cpp       | 290 ++++++++++++++++++++++++++++++\n> >>>  2 files changed, 302 insertions(+)\n> >>>\n> >>> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> >>> index 2fda81a960d2..7d4e238dadfa 100644\n> >>> --- a/src/libcamera/include/v4l2_base.h\n> >>> +++ b/src/libcamera/include/v4l2_base.h\n> >>> @@ -9,6 +9,8 @@\n> >>>\n> >>>  #include <vector>\n> >>>\n> >>> +#include <v4l2_controls.h>\n> >\n> > This should be \"v4l2_controls.h\", but you can simply forward-declare the\n> > V4L2Control class instead.\n> >\n> >>> +\n> >>>  namespace libcamera {\n> >>>\n> >>>  class V4L2Base\n> >>> @@ -22,8 +24,18 @@ public:\n> >>>  \tvirtual void close() = 0;\n> >>>  \tbool isOpen() const;\n> >>>\n> >>> +\tstd::vector<V4L2Control *> getControls(std::vector<unsigned int> &ctrl_ids);\n> >\n> > A vector of pointer isn't very nice as it requires the caller to delete\n> > each item. I would prefer passing a reference to a vector (or map) of\n> > values to the function, and having it fill the values.\n> \n> I thought about it, but how would you define the state of a Control ?\n> Would you create a control with id but without any associated value ?\n> And what if you call value() on it? I fear keeping track of that is a\n> pain, and I preferred to have controls that once created always have a\n> value associated.\n> \n> I can reconsider it though.\n\nHaving to delete them explicitly is a real no-go in my opinion. And even\nif we could solve that, allocating them separately isn't nice from a\nperformance point of view. I'm sure multiple APIs are possible, so feel\nfree to propose an alternative :-)\n\n> >> In my controls, I pass a list <well a map, or a set; I have two\n> >> implementations currently> of the actual ControlValue objects, which get\n> >> filled in.\n> >>\n> >> I wonder if there will be much differences between the two interface\n> >> designs.\n> >\n> > One big difference is that a vector is ordered while a map or set isn't.\n> > The order in which controls are set can make a difference.\n> \n> I think Kieran referred to the \"container of controls to fill\" vs the\n> \"get control ids and return a container of controls\" different\n> approaches.\n> \n> >>> +\tV4L2Control *getControl(unsigned int ctrl_id);\n> >>> +\tint setControls(std::vector<V4L2Control *> &ctrls);\n> >>> +\tint setControl(V4L2Control *ctrl);\n> >>\n> >> I think I'd group those as getControls setControls, and then getControl,\n> >> setControl, so that they are grouped by 'functionality' but that might\n> >> not be a preferred sorting... so either way.\n> >\n> > I would prefer that as well.\n> \n> Yup.\n> \n> >>> +\n> >>>  protected:\n> >>>  \tint fd_;\n> >>> +\n> >>> +private:\n> >>> +\tint queryControl(unsigned int ctrl_id,\n> >>> +\t\t\t struct v4l2_query_ext_ctrl *qext_ctrl);\n> >>> +\n> >>>  };\n> >>>\n> >>>  }; /* namespace libcamera */\n> >>> diff --git a/src/libcamera/v4l2_base.cpp b/src/libcamera/v4l2_base.cpp\n> >>> index 7d05a3c82e4d..423a4d84e276 100644\n> >>> --- a/src/libcamera/v4l2_base.cpp\n> >>> +++ b/src/libcamera/v4l2_base.cpp\n> >>> @@ -5,7 +5,13 @@\n> >>>   * v4l2_base.cpp - Common base for V4L2 devices and subdevices\n> >>>   */\n> >>>\n> >>> +#include <linux/v4l2-subdev.h>\n> >>> +#include <linux/videodev2.h>\n> >>> +#include <sys/ioctl.h>\n> >>> +\n> >>> +#include \"log.h\"\n> >>>  #include \"v4l2_base.h\"\n> >\n> > The issue comes from a previous patch, but v4l2_base.h should be the\n> > very first header included, to help ensuring it is self-contained.\n> >\n> >>> +#include \"v4l2_controls.h\"\n> >>>\n> >>>  /**\n> >>>   * \\file v4l2_base.h\n> >>> @@ -14,6 +20,8 @@\n> >>>\n> >>>  namespace libcamera {\n> >>>\n> >>> +LOG_DEFINE_CATEGORY(V4L2Base)\n> >>> +\n> >>>  /**\n> >>>   * \\class V4L2Base\n> >>>   * \\brief Base class for V4L2Device and V4L2Subdevice\n> >>> @@ -31,6 +39,7 @@ namespace libcamera {\n> >>>   */\n> >>>\n> >>>  /**\n> >>> + * \\fn V4L2Base::open()\n> >\n> > Does this belong to this patch ? Same for the next method.\n> \n> No, sorry, bad rebasing\n> \n> >>>   * \\brief Open a V4L2 device or subdevice\n> >>>   *\n> >>>   * Pure virtual method to be implemented by the derived classes.\n> >>> @@ -39,6 +48,7 @@ namespace libcamera {\n> >>>   */\n> >>>\n> >>>  /**\n> >>> + * \\fn V4L2Base::close()\n> >>>   * \\brief Close the device or subdevice\n> >>>   *\n> >>>   * Pure virtual method to be implemented by the derived classes.\n> >>> @@ -53,6 +63,286 @@ bool V4L2Base::isOpen() const\n> >>>  \treturn fd_ != -1;\n> >>>  }\n> >>>\n> >>> +int V4L2Base::queryControl(unsigned int id,\n> >>> +\t\t\t   struct v4l2_query_ext_ctrl *qext_ctrl)\n> >>> +{\n> >>> +\tqext_ctrl->id = id;\n> >>> +\n> >>> +\tint ret = ioctl(fd_, VIDIOC_QUERY_EXT_CTRL, qext_ctrl);\n> >>> +\tif (ret) {\n> >>> +\t\tret = -errno;\n> >>> +\t\tLOG(V4L2Base, Error)\n> >>> +\t\t\t<< \"Unable to query extended control 0x\"\n> >\n> > s/extended //\n> >\n> >>> +\t\t\t<< std::hex << qext_ctrl->id\n> >>> +\t\t\t<< \": \" << strerror(-ret);\n> >>> +\t\treturn ret;\n> >>> +\t}\n> >>> +\n> >>> +\tif (qext_ctrl->flags & V4L2_CTRL_FLAG_DISABLED) {\n> >>> +\t\tLOG(V4L2Base, Error)\n> >>> +\t\t\t<< \"Extended control 0x\" << std::hex\n> >>> +\t\t\t<< qext_ctrl->id << \" is disabled\";\n> >>> +\t\treturn -EINVAL;\n> >>> +\t}\n> >\n> > I would handle this in the caller, it's not a queryControl() error as\n> > such.\n> >\n> >>> +\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Get values of a list of control IDs\n> >>> + * \\param ctrl_ids The list of control IDs to retrieve value of\n> >>> + * \\return A list of V4L2Control on success or a empty list otherwise\n> >>> + */\n> >>> +std::vector<V4L2Control *> V4L2Base::getControls(std::vector<unsigned int> &ctrl_ids)\n> >>> +{\n> >>> +\tunsigned int count = ctrl_ids.size();\n> >>> +\tif (!count)\n> >>> +\t\treturn std::vector<V4L2Control *> {};\n> >>> +\n> >>> +\tstruct v4l2_ext_control *v4l2_ctrls = static_cast<struct v4l2_ext_control *>\n> >>> +\t\t\t\t\t      (new struct v4l2_ext_control[count]);\n> >>> +\tif (!v4l2_ctrls)\n> >>> +\t\treturn std::vector<V4L2Control *> {};\n> >>> +\n> >>> +\t/*\n> >>> +\t * Query each control in the vector to get back its type and expected\n> >>> +\t * size in order to get its current value.\n> >>> +\t */\n> >>> +\tfor (unsigned int i = 0; i < count; ++i) {\n> >>> +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> >>> +\t\tunsigned int ctrl_id = ctrl_ids[i];\n> >>> +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> >>> +\n> >>> +\t\tint ret = queryControl(ctrl_id, &qext_ctrl);\n> >>> +\t\tif (ret)\n> >>> +\t\t\treturn std::vector<V4L2Control *> {};\n> >>> +\n> >>> +\t\tv4l2_ctrl->id = ctrl_id;\n> >>> +\t\tv4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;\n> >>> +\n> >>> +\t\t/* Reserve memory for controls with payload. */\n> >>> +\t\tif (!(qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD))\n> >>> +\t\t\tcontinue;\n> >>> +\n> >>> +\t\tswitch (qext_ctrl.type) {\n> >>> +\t\tcase V4L2_CTRL_TYPE_U8:\n> >>> +\t\t\tv4l2_ctrl->p_u8 = static_cast<uint8_t *>\n> >>> +\t\t\t\t\t  (new uint8_t[v4l2_ctrl->size]);\n> >>> +\t\t\tbreak;\n> >>> +\t\tcase V4L2_CTRL_TYPE_U16:\n> >>> +\t\t\tv4l2_ctrl->p_u16 = static_cast<uint16_t *>\n> >>> +\t\t\t\t\t  (new uint16_t[v4l2_ctrl->size]);\n> >>> +\t\t\tbreak;\n> >>> +\t\tcase V4L2_CTRL_TYPE_U32:\n> >>> +\t\t\tv4l2_ctrl->p_u32 = static_cast<uint32_t *>\n> >>> +\t\t\t\t\t  (new uint32_t[v4l2_ctrl->size]);\n> >>> +\t\t\tbreak;\n> >>> +\t\t}\n> >>> +\t}\n> >>> +\n> >>> +\tstruct v4l2_ext_controls v4l2_ext_ctrls = {};\n> >>> +\tv4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> >>> +\tv4l2_ext_ctrls.count = count;\n> >>> +\tv4l2_ext_ctrls.controls = v4l2_ctrls;\n> >>> +\n> >>> +\tint ret = ioctl(fd_, VIDIOC_G_EXT_CTRLS, &v4l2_ext_ctrls);\n> >>> +\tif (ret) {\n> >>> +\t\tret = -errno;\n> >>> +\t\tLOG(V4L2Base, Error)\n> >>> +\t\t\t<< \"Unable to get extended controls: \" << strerror(-ret);\n> >>> +\t\t\treturn std::vector<V4L2Control *> {};\n> >>> +\t}\n> >>> +\n> >>> +\t/*\n> >>> +\t * For each requested control, create a V4L2Control which contains\n> >>> +\t * the current value and store it in the vector returned to the caller.\n> >>> +\t */\n> >>> +\tstd::vector<V4L2Control *> controls = {};\n> >>> +\tfor (unsigned int i = 0; i < count; ++i) {\n> >>> +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> >>> +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> >>> +\n> >>> +\t\tint ret = queryControl(v4l2_ctrl->id, &qext_ctrl);\n> >>> +\t\tif (ret)\n> >>> +\t\t\treturn std::vector<V4L2Control *> {};\n> >>> +\n> >>> +\t\tunsigned int size = qext_ctrl.elem_size * qext_ctrl.elem_size;\n> >>> +\t\tif (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> >>> +\t\t\t/* Control types with payload */\n> >>> +\t\t\tswitch (qext_ctrl.type) {\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_U8:\n> >>> +\t\t\t{\n> >>> +\t\t\t\tV4L2U8Control *c =\n> >>> +\t\t\t\t\tnew V4L2U8Control(v4l2_ctrl->id, size,\n> >>> +\t\t\t\t\t\t\t  v4l2_ctrl->p_u8);\n> >>> +\t\t\t\tcontrols.push_back(c);\n> >>> +\t\t\t}\n> >>> +\t\t\t\tbreak;\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_U16:\n> >>> +\t\t\t{\n> >>> +\t\t\t\tV4L2U16Control *c =\n> >>> +\t\t\t\t\tnew V4L2U16Control(v4l2_ctrl->id, size,\n> >>> +\t\t\t\t\t\t\t   v4l2_ctrl->p_u16);\n> >>> +\t\t\t\tcontrols.push_back(c);\n> >>> +\t\t\t}\n> >>> +\t\t\t\tbreak;\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_U32:\n> >>> +\t\t\t{\n> >>> +\t\t\t\tV4L2U32Control *c =\n> >>> +\t\t\t\t\tnew V4L2U32Control(v4l2_ctrl->id, size,\n> >>> +\t\t\t\t\t\t\t  v4l2_ctrl->p_u32);\n> >>> +\t\t\t\tcontrols.push_back(c);\n> >>> +\t\t\t}\n> >>> +\t\t\t\tbreak;\n> >>> +\t\t\tdefault:\n> >>> +\t\t\t\tLOG(V4L2Base, Error)\n> >>> +\t\t\t\t\t<< \"Compound control types not supported: \"\n> >>> +\t\t\t\t\t<< std::hex << qext_ctrl.type;\n> >>> +\t\t\t\treturn std::vector<V4L2Control *> {};\n> >>> +\t\t\t}\n> >>> +\t\t} else {\n> >>> +\t\t\t/* Control types without payload. */\n> >>> +\t\t\tswitch (qext_ctrl.type) {\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> >>> +\t\t\t{\n> >>> +\t\t\t\tV4L2Int64Control *c =\n> >>> +\t\t\t\t\tnew V4L2Int64Control(v4l2_ctrl->id,\n> >>> +\t\t\t\t\t\t\t     v4l2_ctrl->value64);\n> >>> +\t\t\t\tcontrols.push_back(c);\n> >>\n> >> I think this could be a\n> >> \tcontrols.emplace(V4L2Int64Control(v4l2_ctrl->id,\n> >> \t\t\t\t\t  v4l2_ctrl->value64);\n> >>\n> >> if you prefer that style ...\n> >>\n> >> Aha - except you have a vector of pointers, rather than a vector of\n> >> objects ...\n> \n> Yes, I can't do that.\n> \n> >>> +\t\t\t}\n> >>> +\t\t\t\tbreak;\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_MENU:\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> >>> +\t\t\t{\n> >>> +\t\t\t\tV4L2IntControl *c =\n> >>> +\t\t\t\t\tnew V4L2IntControl(v4l2_ctrl->id,\n> >>> +\t\t\t\t\t\t\t   v4l2_ctrl->value);\n> >>> +\t\t\t\tcontrols.push_back(c);\n> >>> +\t\t\t}\n> >>> +\t\t\t\tbreak;\n> >>> +\t\t\tdefault:\n> >>> +\t\t\t\tLOG(V4L2Base, Error)\n> >>> +\t\t\t\t\t<< \"Invalid non-compound control type :\"\n> >>> +\t\t\t\t\t<< std::hex << qext_ctrl.type;\n> >>> +\t\t\t\treturn std::vector<V4L2Control *> {};\n> >>> +\t\t\t}\n> >>> +\t\t}\n> >>> +\t}\n> >>> +\n> >>> +\treturn controls;\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Get the value of control with id \\a ctrl_id\n> >>> + * \\param ctrl_id The control id\n> >>> + * \\return The V4L2Control which represent the value of the control on success\n> >>> + * nullptr otherwise\n> >>> + */\n> >>> +V4L2Control *V4L2Base::getControl(unsigned int ctrl_id)\n> >>> +{\n> >>> +\tstd::vector<unsigned int> v = { ctrl_id, };\n> >>> +\tstd::vector<V4L2Control *> ctrls = getControls(v);\n> >>> +\n> >>> +\tif (ctrls.empty())\n> >>> +\t\treturn nullptr;\n> >>> +\n> >>> +\treturn ctrls[0];\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Apply a list of controls to the device\n> >>> + * \\param ctrls The list of controls to apply\n> >>> + * \\return 0 on success or a negative error code otherwise\n> >>> + */\n> >>> +int V4L2Base::setControls(std::vector<V4L2Control *> &ctrls)\n> >>> +{\n> >>> +\tunsigned int count = ctrls.size();\n> >\n> > size_t instead of unsigned int ?\n> >\n> >>> +\tif (!count)\n> >>> +\t\treturn -EINVAL;\n> >>> +\n> >>> +\tstruct v4l2_ext_control *v4l2_ctrls = static_cast<struct v4l2_ext_control *>\n> >>> +\t\t\t\t\t      (new struct v4l2_ext_control[count]);\n> >\n> > Do you need the cast ?\n> >\n> >>> +\tif (!v4l2_ctrls)\n> >>> +\t\treturn -ENOMEM;\n> >\n> > new() will throw an exception if it can't construct the object, so\n> > there's no need to check the return value.\n> >\n> >>> +\n> >>> +\t/*\n> >>> +\t * Query each control in the vector to get back its type and expected\n> >>> +\t * size and set the new desired value in each v4l2_ext_control member.\n> >>> +\t */\n> >>> +\tfor (unsigned int i = 0; i < count; ++i) {\n> >>> +\t\tstruct v4l2_ext_control *v4l2_ctrl = &v4l2_ctrls[i];\n> >>> +\t\tstruct v4l2_query_ext_ctrl qext_ctrl = {};\n> >>> +\t\tV4L2Control *ctrl = ctrls[i];\n> >>> +\n> >>> +\t\tint ret = queryControl(ctrl->id(), &qext_ctrl);\n> >>> +\t\tif (ret)\n> >>> +\t\t\treturn ret;\n> >\n> > It's annoying to have to query each control every time. We should either\n> > enumerate all of them when opening the device, or query them at runtime\n> > but cache the control information.\n> \n> I'm not a fan of caching control info at open() time tbh. It's quite a\n> relevant amount of memory to be kept around. Indeed I call\n> queryControl twice for each control, and this is bad. I could cache\n> the information -inside this method- and reduce the number of calls to\n> 1, which seems acceptable to me.\n\nThis will potentially be called for every request. The memory cost of a\ncache that would avoid calling queryControl() once per request is a\nsmall price to pay. We don't necessarily have to do it at open() time,\nit could be cached the first time the control is queried (although that\nwould save time at open() time but introduce less-deterministic delays\nat run time, which may not be a good idea).\n\n> >>> +\n> >>> +\t\tv4l2_ctrl->id = ctrl->id();\n> >>> +\t\tv4l2_ctrl->size = qext_ctrl.elem_size * qext_ctrl.elems;\n> >>> +\n> >>> +\t\tif (qext_ctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> >>> +\t\t\t/** \\todo: Support set controls with payload. */\n> >>> +\t\t\tLOG(V4L2Base, Error)\n> >>> +\t\t\t\t<< \"Controls with payload not supported\";\n> >>> +\t\t\treturn -EINVAL;\n> >>> +\t\t} else {\n> >>> +\t\t\tswitch (qext_ctrl.type) {\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> >>> +\t\t\t{\n> >>> +\t\t\t\tV4L2Int64Control *c =\n> >>> +\t\t\t\t\tstatic_cast<V4L2Int64Control *>(ctrl);\n> >>\n> >> It's these static_casts that feel a bit horrible to me. In my\n> >> implementation there is just single class, so there is no casting necessary,\n> >\n> > I don't like the casts much either, especially given that you cast based\n> > on the type returned by the kernel, which could possibly not match the\n> > type of the control as set by the application. This would likely lead to\n> > memory corruption and crashes.\n> \n> I see. Wrapping the container as you suggested and let users interface\n> with that would probably mask the casting inside the V4L2Control\n> class, and it would alway happen on the control type as returned by\n> the kernel. That should be better...\n> \n> >>> +\t\t\t\tv4l2_ctrl->value64 = c->value();\n> >>\n> >> However, this would be c->getInteger64();\n> >>\n> >>> +\t\t\t}\n> >>> +\t\t\t\tbreak;\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_MENU:\n> >>> +\t\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> >>> +\t\t\t{\n> >>> +\t\t\t\tV4L2IntControl *c =\n> >>> +\t\t\t\t\tstatic_cast<V4L2IntControl *>(ctrl);\n> >>> +\t\t\t\tv4l2_ctrl->value = c->value();\n> >>\n> >> and getInteger(); (although getBoolean() can be separated as well);\n> >>\n> >> As we know the types in this switch, I wonder if there is much\n> >> difference int taste between the two approaches...\n> \n> I think I should try to mask the casting in the V4L2Control class as\n> much as I could. I'm still not sure this is completely possible\n> though.\n\nMaybe not completely possible, but if we can avoid it in most cases it\nwould already be a good improvement.\n\n> >>> +\t\t\t}\n> >>> +\t\t\t\tbreak;\n> >>> +\t\t\tdefault:\n> >>> +\t\t\t\tLOG(V4L2Base, Error)\n> >>> +\t\t\t\t\t<< \"Invalid non-compound control type :\"\n> >>> +\t\t\t\t\t<< qext_ctrl.type;\n> >>> +\t\t\t\treturn -EINVAL;\n> >>> +\t\t\t}\n> >>> +\t\t}\n> >>> +\t}\n> >>> +\n> >>> +\tstruct v4l2_ext_controls v4l2_ext_ctrls = {};\n> >>> +\tv4l2_ext_ctrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> >>> +\tv4l2_ext_ctrls.count = count;\n> >>> +\tv4l2_ext_ctrls.controls = v4l2_ctrls;\n> >>> +\n> >>> +\tint ret = ioctl(fd_, VIDIOC_S_EXT_CTRLS, &v4l2_ext_ctrls);\n> >>> +\tif (ret) {\n> >>> +\t\tret = -errno;\n> >>> +\t\tLOG(V4L2Base, Error)\n> >>> +\t\t\t<< \"Unable to set extended controls: \" << strerror(-ret);\n> >>> +\t\treturn ret;\n> >>> +\t}\n> >>> +\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\brief Apply the control \\a ctrl to the device\n> >>> + * \\param ctrl The control to apply to the device\n> >>> + * \\return 0 on success, a negative error code otherwise\n> >>> + */\n> >>> +int V4L2Base::setControl(V4L2Control *ctrl)\n> >>> +{\n> >>> +\tstd::vector<V4L2Control *> v = { ctrl, };\n> >>> +\treturn setControls(v);\n> >>> +}\n> >>> +\n> >>>  /**\n> >>>   * \\var V4L2Base::fd_\n> >>>   * \\brief The V4L2 device or subdevice device node file descriptor","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD6106374A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2019 10:35:15 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(233.56-78-194.adsl-static.isp.belgacom.be [194.78.56.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A5699CB;\n\tMon, 10 Jun 2019 10:35:15 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560155715;\n\tbh=jUzJMgG/9vkvCB49+UoTX7WjuylqE0Bl+LY8mOuEGHg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VmqY32tYYO9e7Ffv2EwO/YM7hhtc3OgeLe5TRzrBL2qPpOlpOu9kR5Icvfaj2SDru\n\tjQEJuI3wkbd7bcuBGwtSK01cmTOu2hfRyL48x+2VJ8Z2I4PaPSJxb+8A6DsRIjWH7t\n\tjJeiKgh9t3vYsofT57UN32rat5EHZVAE0+3OLeXk=","Date":"Mon, 10 Jun 2019 11:35:00 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"Kieran Bingham <kieran.bingham@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190610083459.GL4806@pendragon.ideasonboard.com>","References":"<20190602130435.18780-1-jacopo@jmondi.org>\n\t<20190602130435.18780-5-jacopo@jmondi.org>\n\t<5742d7bb-4c09-5e70-f3b7-81cac34c5247@ideasonboard.com>\n\t<20190608160708.GA23730@pendragon.ideasonboard.com>\n\t<20190610073842.gxw3oinjbmpvxfdm@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190610073842.gxw3oinjbmpvxfdm@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: v4l2_base: Add V4L2\n\tcontrol support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 10 Jun 2019 08:35:16 -0000"}}]