[{"id":1838,"web_url":"https://patchwork.libcamera.org/comment/1838/","msgid":"<20190611114126.GA11268@pendragon.ideasonboard.com>","date":"2019-06-11T11:41:26","subject":"Re: [libcamera-devel] [PATCH v2 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 10, 2019 at 06:40:50PM +0200, 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 \n> ---\n> src/libcamera/include/v4l2_base.h | 11 ++\n> src/libcamera/v4l2_base.cpp | 238 ++++++++++++++++++++++++++++++\n> 2 files changed, 249 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> index 2fda81a960d2..17ea734c8f49 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 \n> \n> +#include \n\nJust forward-declare the required classes and structures.\n\n> +\n> namespace libcamera {\n> \n> class V4L2Base\n> @@ -22,8 +24,17 @@ public:\n> \tvirtual void close() = 0;\n> \tbool isOpen() const;\n> \n> +\tint getControls(std::vector &ctrl_ids,\n> +\t\t\tV4L2Controls *ctrls);\n\n\tconst std::vector &ctrl_ids,\n\nI still wonder if it wouldn't be simpler for applications to fill the\nV4L2Controls with the control IDs to read, and just pass the\nV4L2Controls pointer to this function. In any case, this is better than\nthe first version.\n\n> +\tint setControls(V4L2Controls &ctrls);\n\nYou should pass a const V4L2Controls &ctrls. Or, maybe you should\ninstead pass a non-const pointer, as setControls() should return the\nvalue set for each control.\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..cbd7a551130f 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 \n> +#include \n> +#include \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,234 @@ 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> +int V4L2Base::getControls(std::vector &ctrl_ids,\n> +\t\t\t V4L2Controls *controls)\n> +{\n> +\tunsigned int count = ctrl_ids.size();\n> +\tif (!count)\n> +\t\treturn -EINVAL;\n> +\n> +\tcontrols->clear();\n> +\n> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\n\nYou're leaking memory. The simplest solution would be\n\n\tstruct v4l2_ext_controls v4l2_ctrls[count];\n\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 ret;\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\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\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\n> +\t\t\t\t\t (new uint32_t[v4l2_ctrl->size]);\n> +\t\t\tbreak;\n\nThis is lots of new's, and they're all leaked. A better solution would\nbe to populate ctrls with the V4L2Control instances already, and use the\npointer to the internal memory buffers directly.\n\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\treturn ret;\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> +\tLOG(Error) << __func__;\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 ret;\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\tcontrols->set(v4l2_ctrl->id, size,\n> +\t\t\t\t\t v4l2_ctrl->p_u8);\n> +\t\t\t\tbreak;\n> +\t\t\tcase V4L2_CTRL_TYPE_U16:\n> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> +\t\t\t\t\t v4l2_ctrl->p_u16);\n> +\t\t\t\tbreak;\n> +\t\t\tcase V4L2_CTRL_TYPE_U32:\n> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> +\t\t\t\t\t v4l2_ctrl->p_u32);\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 -EINVAL;\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\tcontrols->set(v4l2_ctrl->id, static_cast\n> +\t\t\t\t\t (v4l2_ctrl->value64));\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\tcontrols->set(v4l2_ctrl->id, v4l2_ctrl->value);\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 -EINVAL;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\treturn 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(V4L2Controls &ctrls)\n> +{\n> +\tunsigned int count = ctrls.size();\n> +\tif (!count)\n> +\t\treturn -EINVAL;\n> +\n> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\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(ctrl);\n> +\t\t\t\tv4l2_ctrl->value64 = c->value();\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(ctrl);\n> +\t\t\t\tv4l2_ctrl->value = c->value();\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 controls: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\var V4L2Base::fd_\n> * \\brief The V4L2 device or subdevice device node file descriptor","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C4E962FB1\n\tfor ;\n\tTue, 11 Jun 2019 13:41:42 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 931D9FA0;\n\tTue, 11 Jun 2019 13:41:41 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560253301;\n\tbh=3lbAPTH+kuMkc+haaVGngblVO2zAr05FQuBR3g60yLY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AqHB5VbRC0uxmWjFCFB4uCmam83jz9u9x+GB9wdxY10wpKy43l281Nor+Ewsa6iKt\n\tXmfVVNU6e8u5/tkXD3n8hT+a2pUYEgcuTslzP1aRvSVGQrlhKIZzynLveLoFdyij1R\n\tJzd9fzHw37eD4bhe8j2P2exBPue0AhmeNX5wWeP0=","Date":"Tue, 11 Jun 2019 14:41:26 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190611114126.GA11268@pendragon.ideasonboard.com>","References":"<20190610164052.30741-1-jacopo@jmondi.org>\n\t<20190610164052.30741-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190610164052.30741-5-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Jun 2019 11:41:42 -0000"}},{"id":1840,"web_url":"https://patchwork.libcamera.org/comment/1840/","msgid":"","date":"2019-06-11T11:55:07","subject":"Re: [libcamera-devel] [PATCH v2 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 Laurent, Jacopo\n\nOn 11/06/2019 12:41, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> Thank you for the patch.\n> \n> On Mon, Jun 10, 2019 at 06:40:50PM +0200, 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 \n>> ---\n>> src/libcamera/include/v4l2_base.h | 11 ++\n>> src/libcamera/v4l2_base.cpp | 238 ++++++++++++++++++++++++++++++\n>> 2 files changed, 249 insertions(+)\n>>\n>> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n>> index 2fda81a960d2..17ea734c8f49 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 \n>> \n>> +#include \n> \n> Just forward-declare the required classes and structures.\n> \n>> +\n>> namespace libcamera {\n>> \n>> class V4L2Base\n>> @@ -22,8 +24,17 @@ public:\n>> \tvirtual void close() = 0;\n>> \tbool isOpen() const;\n>> \n>> +\tint getControls(std::vector &ctrl_ids,\n>> +\t\t\tV4L2Controls *ctrls);\n> \n> \tconst std::vector &ctrl_ids,\n> \n> I still wonder if it wouldn't be simpler for applications to fill the\n> V4L2Controls with the control IDs to read, and just pass the\n> V4L2Controls pointer to this function. In any case, this is better than\n> the first version.\n\n\nRemembering that 'applications' do not use or see these types of course.\n\nOnly Pipelinehandlers should every see or use these objects.\n\n\n> \n>> +\tint setControls(V4L2Controls &ctrls);\n> \n> You should pass a const V4L2Controls &ctrls. Or, maybe you should\n> instead pass a non-const pointer, as setControls() should return the\n> value set for each control.\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..cbd7a551130f 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 \n>> +#include \n>> +#include \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,234 @@ 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>> +int V4L2Base::getControls(std::vector &ctrl_ids,\n>> +\t\t\t V4L2Controls *controls)\n>> +{\n>> +\tunsigned int count = ctrl_ids.size();\n>> +\tif (!count)\n>> +\t\treturn -EINVAL;\n>> +\n>> +\tcontrols->clear();\n>> +\n>> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\n> \n> You're leaking memory. The simplest solution would be\n> \n> \tstruct v4l2_ext_controls v4l2_ctrls[count];\n> \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 ret;\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\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\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\n>> +\t\t\t\t\t (new uint32_t[v4l2_ctrl->size]);\n>> +\t\t\tbreak;\n> \n> This is lots of new's, and they're all leaked. A better solution would\n> be to populate ctrls with the V4L2Control instances already, and use the\n> pointer to the internal memory buffers directly.\n\n\nWe're doing new's here with v4l2_ctrl->size. Does that imply that these\nare payload control values ?\n\nIn fact - yes the statement above says so.\n\nI think if the pipeline handler wants to read or set a 'payload' then it\nshould be responsible for providing that memory - otherwise we will have\nmore copying involved.\n\n\nPerhaps we should just drop support for payload controls for now until\nwe actually hit a need to use it?\n\nOr do you know taht you'll need it for IPU3?\n\n\n\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\treturn ret;\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>> +\tLOG(Error) << __func__;\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 ret;\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\tcontrols->set(v4l2_ctrl->id, size,\n>> +\t\t\t\t\t v4l2_ctrl->p_u8);\n>> +\t\t\t\tbreak;\n>> +\t\t\tcase V4L2_CTRL_TYPE_U16:\n>> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n>> +\t\t\t\t\t v4l2_ctrl->p_u16);\n>> +\t\t\t\tbreak;\n>> +\t\t\tcase V4L2_CTRL_TYPE_U32:\n>> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n>> +\t\t\t\t\t v4l2_ctrl->p_u32);\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 -EINVAL;\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\tcontrols->set(v4l2_ctrl->id, static_cast\n>> +\t\t\t\t\t (v4l2_ctrl->value64));\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\tcontrols->set(v4l2_ctrl->id, v4l2_ctrl->value);\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 -EINVAL;\n>> +\t\t\t}\n>> +\t\t}\n>> +\t}\n>> +\n>> +\treturn 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(V4L2Controls &ctrls)\n>> +{\n>> +\tunsigned int count = ctrls.size();\n>> +\tif (!count)\n>> +\t\treturn -EINVAL;\n>> +\n>> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\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\nI think only supporting the types below would help accelerate this\nseries and get it integrated faster, which will help unblock other\ndevelopment paths.\n\n\n\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(ctrl);\n>> +\t\t\t\tv4l2_ctrl->value64 = c->value();\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(ctrl);\n>> +\t\t\t\tv4l2_ctrl->value = c->value();\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 controls: \" << strerror(-ret);\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> /**\n>> * \\var V4L2Base::fd_\n>> * \\brief The V4L2 device or subdevice device node file descriptor\n>","headers":{"Return-Path":"","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 3335A62FAC\n\tfor ;\n\tTue, 11 Jun 2019 13:55:11 +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 5BA54FA0;\n\tTue, 11 Jun 2019 13:55:10 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560254110;\n\tbh=Rg0lPlFkwcV+3GIGXOTYfvIba697b2ZSIu2UXFOVTNo=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=vuKw72KtDQBspZ+caoWBAokDOV4s30Q/5oJ2A58jOovA31VLxUaIynROZXvZn6dlc\n\tmlqq08zxlQCxharNUlL2+MadUuSA0eIdZfhkZxJZmbkVZKlbbo3kO31ci6JyW1dFsD\n\tSpbw4bN1LMscdroD6Gwzjm0etnqSlCj3aHALQ0V4=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tJacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20190610164052.30741-1-jacopo@jmondi.org>\n\t<20190610164052.30741-5-jacopo@jmondi.org>\n\t<20190611114126.GA11268@pendragon.ideasonboard.com>","From":"Kieran Bingham ","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":"","Date":"Tue, 11 Jun 2019 12:55:07 +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":"<20190611114126.GA11268@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Jun 2019 11:55:11 -0000"}},{"id":1843,"web_url":"https://patchwork.libcamera.org/comment/1843/","msgid":"<20190611121612.GI5016@pendragon.ideasonboard.com>","date":"2019-06-11T12:16:12","subject":"Re: [libcamera-devel] [PATCH v2 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 Kieran,\n\nOn Tue, Jun 11, 2019 at 12:55:07PM +0100, Kieran Bingham wrote:\n> On 11/06/2019 12:41, Laurent Pinchart wrote:\n> > On Mon, Jun 10, 2019 at 06:40:50PM +0200, 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 \n> >> ---\n> >> src/libcamera/include/v4l2_base.h | 11 ++\n> >> src/libcamera/v4l2_base.cpp | 238 ++++++++++++++++++++++++++++++\n> >> 2 files changed, 249 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> >> index 2fda81a960d2..17ea734c8f49 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 \n> >> \n> >> +#include \n> > \n> > Just forward-declare the required classes and structures.\n> > \n> >> +\n> >> namespace libcamera {\n> >> \n> >> class V4L2Base\n> >> @@ -22,8 +24,17 @@ public:\n> >> \tvirtual void close() = 0;\n> >> \tbool isOpen() const;\n> >> \n> >> +\tint getControls(std::vector &ctrl_ids,\n> >> +\t\t\tV4L2Controls *ctrls);\n> > \n> > \tconst std::vector &ctrl_ids,\n> > \n> > I still wonder if it wouldn't be simpler for applications to fill the\n> > V4L2Controls with the control IDs to read, and just pass the\n> > V4L2Controls pointer to this function. In any case, this is better than\n> > the first version.\n> \n> Remembering that 'applications' do not use or see these types of course.\n> \n> Only Pipelinehandlers should every see or use these objects.\n\nYes, that's what I meant, sorry.\n\n> >> +\tint setControls(V4L2Controls &ctrls);\n> > \n> > You should pass a const V4L2Controls &ctrls. Or, maybe you should\n> > instead pass a non-const pointer, as setControls() should return the\n> > value set for each control.\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..cbd7a551130f 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 \n> >> +#include \n> >> +#include \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,234 @@ 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> >> +int V4L2Base::getControls(std::vector &ctrl_ids,\n> >> +\t\t\t V4L2Controls *controls)\n> >> +{\n> >> +\tunsigned int count = ctrl_ids.size();\n> >> +\tif (!count)\n> >> +\t\treturn -EINVAL;\n> >> +\n> >> +\tcontrols->clear();\n> >> +\n> >> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\n> > \n> > You're leaking memory. The simplest solution would be\n> > \n> > \tstruct v4l2_ext_controls v4l2_ctrls[count];\n> > \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 ret;\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\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\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\n> >> +\t\t\t\t\t (new uint32_t[v4l2_ctrl->size]);\n> >> +\t\t\tbreak;\n> > \n> > This is lots of new's, and they're all leaked. A better solution would\n> > be to populate ctrls with the V4L2Control instances already, and use the\n> > pointer to the internal memory buffers directly.\n> \n> We're doing new's here with v4l2_ctrl->size. Does that imply that these\n> are payload control values ?\n> \n> In fact - yes the statement above says so.\n> \n> I think if the pipeline handler wants to read or set a 'payload' then it\n> should be responsible for providing that memory - otherwise we will have\n> more copying involved.\n\nThe V4L2ControlValue derived classes allocate memory internally for the\npayload, so I think we should use that instead of copying.\n\n> Perhaps we should just drop support for payload controls for now until\n> we actually hit a need to use it?\n\nThat's one option.\n\n> Or do you know taht you'll need it for IPU3?\n\nNot to my knowledge.\n\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\treturn ret;\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> >> +\tLOG(Error) << __func__;\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 ret;\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\tcontrols->set(v4l2_ctrl->id, size,\n> >> +\t\t\t\t\t v4l2_ctrl->p_u8);\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tcase V4L2_CTRL_TYPE_U16:\n> >> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> >> +\t\t\t\t\t v4l2_ctrl->p_u16);\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tcase V4L2_CTRL_TYPE_U32:\n> >> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> >> +\t\t\t\t\t v4l2_ctrl->p_u32);\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 -EINVAL;\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\tcontrols->set(v4l2_ctrl->id, static_cast\n> >> +\t\t\t\t\t (v4l2_ctrl->value64));\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\tcontrols->set(v4l2_ctrl->id, v4l2_ctrl->value);\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 -EINVAL;\n> >> +\t\t\t}\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\treturn 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(V4L2Controls &ctrls)\n> >> +{\n> >> +\tunsigned int count = ctrls.size();\n> >> +\tif (!count)\n> >> +\t\treturn -EINVAL;\n> >> +\n> >> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\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> \n> I think only supporting the types below would help accelerate this\n> series and get it integrated faster, which will help unblock other\n> development paths.\n> \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(ctrl);\n> >> +\t\t\t\tv4l2_ctrl->value64 = c->value();\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(ctrl);\n> >> +\t\t\t\tv4l2_ctrl->value = c->value();\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 controls: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> /**\n> >> * \\var V4L2Base::fd_\n> >> * \\brief The V4L2 device or subdevice device node file descriptor","headers":{"Return-Path":"","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 054AC61E1B\n\tfor ;\n\tTue, 11 Jun 2019 14:16:28 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 63412FA0;\n\tTue, 11 Jun 2019 14:16:27 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560255387;\n\tbh=NuCETvOLKC6fMk5u1qfB5/fFfIdALqm2HvfDUrn0Cn8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hpTBwQ5/wPKb0fJ4Jsa3Db8Xs6cFA5nQTNCrMbo/nmaGIPp6dG4RPQpSf1XOiIID7\n\thK+NH/s8nE9gyUBjG39kDCEphcUEDuUOBdV0ZFVQq2uUyMhJxhnw/L/hESRhd7TPqA\n\tDDWhfRqtSFGty9+nwQQ4tC9NLOhfAMrxyXfeYPvQ=","Date":"Tue, 11 Jun 2019 15:16:12 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20190611121612.GI5016@pendragon.ideasonboard.com>","References":"<20190610164052.30741-1-jacopo@jmondi.org>\n\t<20190610164052.30741-5-jacopo@jmondi.org>\n\t<20190611114126.GA11268@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Jun 2019 12:16:28 -0000"}},{"id":1846,"web_url":"https://patchwork.libcamera.org/comment/1846/","msgid":"<20190611131021.fuh2f4uhuw5acmwf@uno.localdomain>","date":"2019-06-11T13:10:21","subject":"Re: [libcamera-devel] [PATCH v2 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 Laurent,\n\nOn Tue, Jun 11, 2019 at 02:41:26PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Jun 10, 2019 at 06:40:50PM +0200, 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 \n> > ---\n> > src/libcamera/include/v4l2_base.h | 11 ++\n> > src/libcamera/v4l2_base.cpp | 238 ++++++++++++++++++++++++++++++\n> > 2 files changed, 249 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> > index 2fda81a960d2..17ea734c8f49 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 \n> >\n> > +#include \n>\n> Just forward-declare the required classes and structures.\n>\n> > +\n> > namespace libcamera {\n> >\n> > class V4L2Base\n> > @@ -22,8 +24,17 @@ public:\n> > \tvirtual void close() = 0;\n> > \tbool isOpen() const;\n> >\n> > +\tint getControls(std::vector &ctrl_ids,\n> > +\t\t\tV4L2Controls *ctrls);\n>\n> \tconst std::vector &ctrl_ids,\n>\n> I still wonder if it wouldn't be simpler for applications to fill the\n> V4L2Controls with the control IDs to read, and just pass the\n> V4L2Controls pointer to this function. In any case, this is better than\n> the first version.\n>\n\nI thought about that, but I don't like the fact that providing\ncontrols without a value would require to keep track of the state of a\ncontrol (initialized? empty? with a default value? read from the HW?).\n\nIn this way, we know that the V4L2Controls instance will contain\ncontrols with a value read from the hardware, as well as the one\nprovided to setControl will contain values set by the user and applied\nto the HW. It's much easier for me not having to keep track of the\ncontrol state (to prevent, say, reading a control without a value\nassigned). I can change this by popular demand though. No issues.\n\n\n> > +\tint setControls(V4L2Controls &ctrls);\n>\n> You should pass a const V4L2Controls &ctrls. Or, maybe you should\n> instead pass a non-const pointer, as setControls() should return the\n> value set for each control.\n\nNot sure about const, the value of the single V4L2Control should be\nupdated to reflect what is applied to the HW.\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..cbd7a551130f 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 \n> > +#include \n> > +#include \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,234 @@ 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> > +int V4L2Base::getControls(std::vector &ctrl_ids,\n> > +\t\t\t V4L2Controls *controls)\n> > +{\n> > +\tunsigned int count = ctrl_ids.size();\n> > +\tif (!count)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tcontrols->clear();\n> > +\n> > +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\n>\n> You're leaking memory. The simplest solution would be\n>\n> \tstruct v4l2_ext_controls v4l2_ctrls[count];\n\nIndeed!\nI wonder why valgrind reported no leaks...\n\n>\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 ret;\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\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\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\n> > +\t\t\t\t\t (new uint32_t[v4l2_ctrl->size]);\n> > +\t\t\tbreak;\n>\n> This is lots of new's, and they're all leaked. A better solution would\n> be to populate ctrls with the V4L2Control instances already, and use the\n> pointer to the internal memory buffers directly.\n\nSorry, I didn't get you, what's 'ctrls' here?\n\nThe memory here allocated (and not released, my bad) serves to the\nkernel to copy the data payload to userspace. It is then copied inside\nthe V4L2Control, where memory is reserved at creation time.\n\nWhat I could do, and I think that's what you suggested is to create\nthe V4L2Control in advance (once I know the required memory size) and\npoint the 'struct v4l2_ext_control' data pointer to that memory, and\nsave one copy.\n\nPersonally, I'm also fine dropping support for payload controls for\nthis first version, as far as I could tell, there's no strict\nrequirement for any of them on IPU3 (as Kieran suggested).\n\nWhat's your opinion?\n\n>\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\treturn ret;\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> > +\tLOG(Error) << __func__;\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 ret;\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\tcontrols->set(v4l2_ctrl->id, size,\n> > +\t\t\t\t\t v4l2_ctrl->p_u8);\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase V4L2_CTRL_TYPE_U16:\n> > +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> > +\t\t\t\t\t v4l2_ctrl->p_u16);\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase V4L2_CTRL_TYPE_U32:\n> > +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> > +\t\t\t\t\t v4l2_ctrl->p_u32);\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 -EINVAL;\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\tcontrols->set(v4l2_ctrl->id, static_cast\n> > +\t\t\t\t\t (v4l2_ctrl->value64));\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\tcontrols->set(v4l2_ctrl->id, v4l2_ctrl->value);\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 -EINVAL;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn 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(V4L2Controls &ctrls)\n> > +{\n> > +\tunsigned int count = ctrls.size();\n> > +\tif (!count)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\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(ctrl);\n> > +\t\t\t\tv4l2_ctrl->value64 = c->value();\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(ctrl);\n> > +\t\t\t\tv4l2_ctrl->value = c->value();\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 controls: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\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":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F185662F91\n\tfor ;\n\tTue, 11 Jun 2019 15:09:08 +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 relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 7F0441C0009;\n\tTue, 11 Jun 2019 13:09:08 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 11 Jun 2019 15:10:21 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190611131021.fuh2f4uhuw5acmwf@uno.localdomain>","References":"<20190610164052.30741-1-jacopo@jmondi.org>\n\t<20190610164052.30741-5-jacopo@jmondi.org>\n\t<20190611114126.GA11268@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ybitpuo6thinjuzg\"","Content-Disposition":"inline","In-Reply-To":"<20190611114126.GA11268@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Jun 2019 13:09:09 -0000"}},{"id":1860,"web_url":"https://patchwork.libcamera.org/comment/1860/","msgid":"<20190611152658.GP5016@pendragon.ideasonboard.com>","date":"2019-06-11T15:26:58","subject":"Re: [libcamera-devel] [PATCH v2 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 Tue, Jun 11, 2019 at 03:10:21PM +0200, Jacopo Mondi wrote:\n> On Tue, Jun 11, 2019 at 02:41:26PM +0300, Laurent Pinchart wrote:\n> > On Mon, Jun 10, 2019 at 06:40:50PM +0200, 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 \n> >> ---\n> >> src/libcamera/include/v4l2_base.h | 11 ++\n> >> src/libcamera/v4l2_base.cpp | 238 ++++++++++++++++++++++++++++++\n> >> 2 files changed, 249 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> >> index 2fda81a960d2..17ea734c8f49 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 \n> >>\n> >> +#include \n> >\n> > Just forward-declare the required classes and structures.\n> >\n> >> +\n> >> namespace libcamera {\n> >>\n> >> class V4L2Base\n> >> @@ -22,8 +24,17 @@ public:\n> >> \tvirtual void close() = 0;\n> >> \tbool isOpen() const;\n> >>\n> >> +\tint getControls(std::vector &ctrl_ids,\n> >> +\t\t\tV4L2Controls *ctrls);\n> >\n> > \tconst std::vector &ctrl_ids,\n> >\n> > I still wonder if it wouldn't be simpler for applications to fill the\n> > V4L2Controls with the control IDs to read, and just pass the\n> > V4L2Controls pointer to this function. In any case, this is better than\n> > the first version.\n> \n> I thought about that, but I don't like the fact that providing\n> controls without a value would require to keep track of the state of a\n> control (initialized? empty? with a default value? read from the HW?).\n> \n> In this way, we know that the V4L2Controls instance will contain\n> controls with a value read from the hardware, as well as the one\n> provided to setControl will contain values set by the user and applied\n> to the HW. It's much easier for me not having to keep track of the\n> control state (to prevent, say, reading a control without a value\n> assigned). I can change this by popular demand though. No issues.\n\nI won't push too hard :-) But I'm wondering what your real concern is\nhere. Are you worried that an application will create a V4L2Controls\ninstance, populate it with controls to be set and pass it to\nV4L2Base::setControls(), and then later pass the same instance to\nV4L2Base::getControls() ?\n\n> >> +\tint setControls(V4L2Controls &ctrls);\n> >\n> > You should pass a const V4L2Controls &ctrls. Or, maybe you should\n> > instead pass a non-const pointer, as setControls() should return the\n> > value set for each control.\n> \n> Not sure about const, the value of the single V4L2Control should be\n> updated to reflect what is applied to the HW.\n\nI agree, hence the second sentence :-) I would then pass the\nV4L2Controls by pointer to reflect that.\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..cbd7a551130f 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 \n> >> +#include \n> >> +#include \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,234 @@ 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> >> +int V4L2Base::getControls(std::vector &ctrl_ids,\n> >> +\t\t\t V4L2Controls *controls)\n> >> +{\n> >> +\tunsigned int count = ctrl_ids.size();\n> >> +\tif (!count)\n> >> +\t\treturn -EINVAL;\n> >> +\n> >> +\tcontrols->clear();\n> >> +\n> >> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\n> >\n> > You're leaking memory. The simplest solution would be\n> >\n> > \tstruct v4l2_ext_controls v4l2_ctrls[count];\n> \n> Indeed!\n> I wonder why valgrind reported no leaks...\n> \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 ret;\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\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\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\n> >> +\t\t\t\t\t (new uint32_t[v4l2_ctrl->size]);\n> >> +\t\t\tbreak;\n> >\n> > This is lots of new's, and they're all leaked. A better solution would\n> > be to populate ctrls with the V4L2Control instances already, and use the\n> > pointer to the internal memory buffers directly.\n> \n> Sorry, I didn't get you, what's 'ctrls' here?\n\nSorry, I meant the controls parameter.\n\n> The memory here allocated (and not released, my bad) serves to the\n> kernel to copy the data payload to userspace. It is then copied inside\n> the V4L2Control, where memory is reserved at creation time.\n> \n> What I could do, and I think that's what you suggested is to create\n> the V4L2Control in advance (once I know the required memory size) and\n> point the 'struct v4l2_ext_control' data pointer to that memory, and\n> save one copy.\n\nCorrect, that's what I meant.\n\n> Personally, I'm also fine dropping support for payload controls for\n> this first version, as far as I could tell, there's no strict\n> requirement for any of them on IPU3 (as Kieran suggested).\n> \n> What's your opinion?\n\nWe have very few payload controls in mainline, so that could indeed be a\ngood idea. I would however experiment with creation of the V4L2Control\ninstance before calling VIDIOC_G_EXT_CTRLS to see if it would be an\noption for a later support of compound controls.\n\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\treturn ret;\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> >> +\tLOG(Error) << __func__;\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 ret;\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\tcontrols->set(v4l2_ctrl->id, size,\n> >> +\t\t\t\t\t v4l2_ctrl->p_u8);\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tcase V4L2_CTRL_TYPE_U16:\n> >> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> >> +\t\t\t\t\t v4l2_ctrl->p_u16);\n> >> +\t\t\t\tbreak;\n> >> +\t\t\tcase V4L2_CTRL_TYPE_U32:\n> >> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> >> +\t\t\t\t\t v4l2_ctrl->p_u32);\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 -EINVAL;\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\tcontrols->set(v4l2_ctrl->id, static_cast\n> >> +\t\t\t\t\t (v4l2_ctrl->value64));\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\tcontrols->set(v4l2_ctrl->id, v4l2_ctrl->value);\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 -EINVAL;\n> >> +\t\t\t}\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> +\treturn 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(V4L2Controls &ctrls)\n> >> +{\n> >> +\tunsigned int count = ctrls.size();\n> >> +\tif (!count)\n> >> +\t\treturn -EINVAL;\n> >> +\n> >> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\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(ctrl);\n> >> +\t\t\t\tv4l2_ctrl->value64 = c->value();\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(ctrl);\n> >> +\t\t\t\tv4l2_ctrl->value = c->value();\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 controls: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> /**\n> >> * \\var V4L2Base::fd_\n> >> * \\brief The V4L2 device or subdevice device node file descriptor","headers":{"Return-Path":"","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 73D8B62F70\n\tfor ;\n\tTue, 11 Jun 2019 17:27:14 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E7163FA0;\n\tTue, 11 Jun 2019 17:27:13 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560266834;\n\tbh=7o7OW3Xfo8GoA3wPM8Vr64BzaUGiebUB4PNBCeXkQVM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YgWlGOu46OFeeTKElQD9F2kVRtLfayTq1K/G1gBi9Bc72NI5YFDgqPaQuYdVkt4wl\n\tVb01nhZR652VSgFTYVxqVobIl9WiIU0AiKJ0gKrFTBLCSCOtcOZuf9vn3enMktrN81\n\tBgIKBoAeVJWwaKPiqQXt7BAZqZdhGvs8DbeBydIM=","Date":"Tue, 11 Jun 2019 18:26:58 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190611152658.GP5016@pendragon.ideasonboard.com>","References":"<20190610164052.30741-1-jacopo@jmondi.org>\n\t<20190610164052.30741-5-jacopo@jmondi.org>\n\t<20190611114126.GA11268@pendragon.ideasonboard.com>\n\t<20190611131021.fuh2f4uhuw5acmwf@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190611131021.fuh2f4uhuw5acmwf@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Jun 2019 15:27:14 -0000"}},{"id":1861,"web_url":"https://patchwork.libcamera.org/comment/1861/","msgid":"<20190611173528.lhob6ni3hcegz2g7@uno.localdomain>","date":"2019-06-11T17:35:28","subject":"Re: [libcamera-devel] [PATCH v2 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 Laurent,\n\nOn Tue, Jun 11, 2019 at 06:26:58PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Tue, Jun 11, 2019 at 03:10:21PM +0200, Jacopo Mondi wrote:\n> > On Tue, Jun 11, 2019 at 02:41:26PM +0300, Laurent Pinchart wrote:\n> > > On Mon, Jun 10, 2019 at 06:40:50PM +0200, 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 \n> > >> ---\n> > >> src/libcamera/include/v4l2_base.h | 11 ++\n> > >> src/libcamera/v4l2_base.cpp | 238 ++++++++++++++++++++++++++++++\n> > >> 2 files changed, 249 insertions(+)\n> > >>\n> > >> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> > >> index 2fda81a960d2..17ea734c8f49 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 \n> > >>\n> > >> +#include \n> > >\n> > > Just forward-declare the required classes and structures.\n> > >\n> > >> +\n> > >> namespace libcamera {\n> > >>\n> > >> class V4L2Base\n> > >> @@ -22,8 +24,17 @@ public:\n> > >> \tvirtual void close() = 0;\n> > >> \tbool isOpen() const;\n> > >>\n> > >> +\tint getControls(std::vector &ctrl_ids,\n> > >> +\t\t\tV4L2Controls *ctrls);\n> > >\n> > > \tconst std::vector &ctrl_ids,\n> > >\n> > > I still wonder if it wouldn't be simpler for applications to fill the\n> > > V4L2Controls with the control IDs to read, and just pass the\n> > > V4L2Controls pointer to this function. In any case, this is better than\n> > > the first version.\n> >\n> > I thought about that, but I don't like the fact that providing\n> > controls without a value would require to keep track of the state of a\n> > control (initialized? empty? with a default value? read from the HW?).\n> >\n> > In this way, we know that the V4L2Controls instance will contain\n> > controls with a value read from the hardware, as well as the one\n> > provided to setControl will contain values set by the user and applied\n> > to the HW. It's much easier for me not having to keep track of the\n> > control state (to prevent, say, reading a control without a value\n> > assigned). I can change this by popular demand though. No issues.\n>\n> I won't push too hard :-) But I'm wondering what your real concern is\n> here. Are you worried that an application will create a V4L2Controls\n> instance, populate it with controls to be set and pass it to\n> V4L2Base::setControls(), and then later pass the same instance to\n> V4L2Base::getControls() ?\n\nWhat worries me is that application will create controls without a\nvalue and controls with a value, and there is no distinction between\nthe two, so they could call a get$TYPE() and get back meaningless\nvalues from it.\n\nThat calls for keeping track of the control state, or distinguish\nread/write controls, which I fear will be cumbersome and unecessary\ncomplicated. Am I worrying too much?\n\n>\n> > >> +\tint setControls(V4L2Controls &ctrls);\n> > >\n> > > You should pass a const V4L2Controls &ctrls. Or, maybe you should\n> > > instead pass a non-const pointer, as setControls() should return the\n> > > value set for each control.\n> >\n> > Not sure about const, the value of the single V4L2Control should be\n> > updated to reflect what is applied to the HW.\n>\n> I agree, hence the second sentence :-) I would then pass the\n> V4L2Controls by pointer to reflect that.\n>\n\nYes indeed.\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..cbd7a551130f 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 \n> > >> +#include \n> > >> +#include \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,234 @@ 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> > >> +int V4L2Base::getControls(std::vector &ctrl_ids,\n> > >> +\t\t\t V4L2Controls *controls)\n> > >> +{\n> > >> +\tunsigned int count = ctrl_ids.size();\n> > >> +\tif (!count)\n> > >> +\t\treturn -EINVAL;\n> > >> +\n> > >> +\tcontrols->clear();\n> > >> +\n> > >> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\n> > >\n> > > You're leaking memory. The simplest solution would be\n> > >\n> > > \tstruct v4l2_ext_controls v4l2_ctrls[count];\n> >\n> > Indeed!\n> > I wonder why valgrind reported no leaks...\n> >\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 ret;\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\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\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\n> > >> +\t\t\t\t\t (new uint32_t[v4l2_ctrl->size]);\n> > >> +\t\t\tbreak;\n> > >\n> > > This is lots of new's, and they're all leaked. A better solution would\n> > > be to populate ctrls with the V4L2Control instances already, and use the\n> > > pointer to the internal memory buffers directly.\n> >\n> > Sorry, I didn't get you, what's 'ctrls' here?\n>\n> Sorry, I meant the controls parameter.\n>\n> > The memory here allocated (and not released, my bad) serves to the\n> > kernel to copy the data payload to userspace. It is then copied inside\n> > the V4L2Control, where memory is reserved at creation time.\n> >\n> > What I could do, and I think that's what you suggested is to create\n> > the V4L2Control in advance (once I know the required memory size) and\n> > point the 'struct v4l2_ext_control' data pointer to that memory, and\n> > save one copy.\n>\n> Correct, that's what I meant.\n>\n> > Personally, I'm also fine dropping support for payload controls for\n> > this first version, as far as I could tell, there's no strict\n> > requirement for any of them on IPU3 (as Kieran suggested).\n> >\n> > What's your opinion?\n>\n> We have very few payload controls in mainline, so that could indeed be a\n> good idea. I would however experiment with creation of the V4L2Control\n> instance before calling VIDIOC_G_EXT_CTRLS to see if it would be an\n> option for a later support of compound controls.\n>\n\nI'm a bit in two minds here:\nIf we drop support for controls with payload it calls for dropping the\ntemplate and use Kieran's ControlValue. But then, once we'll ever have\nto support them, I fear the ControlValue class would be a waste of space\nand less flexible to use with controls with payload...\n\nIt seems to me that dropping payload support and use ControlValue as\nbacking storage would make it easier to get the series in, so I'm\ntempted to go for the \"is good for now\" way and do what is outlined\nhere above...\n\n\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\treturn ret;\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> > >> +\tLOG(Error) << __func__;\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 ret;\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\tcontrols->set(v4l2_ctrl->id, size,\n> > >> +\t\t\t\t\t v4l2_ctrl->p_u8);\n> > >> +\t\t\t\tbreak;\n> > >> +\t\t\tcase V4L2_CTRL_TYPE_U16:\n> > >> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> > >> +\t\t\t\t\t v4l2_ctrl->p_u16);\n> > >> +\t\t\t\tbreak;\n> > >> +\t\t\tcase V4L2_CTRL_TYPE_U32:\n> > >> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> > >> +\t\t\t\t\t v4l2_ctrl->p_u32);\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 -EINVAL;\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\tcontrols->set(v4l2_ctrl->id, static_cast\n> > >> +\t\t\t\t\t (v4l2_ctrl->value64));\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\tcontrols->set(v4l2_ctrl->id, v4l2_ctrl->value);\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 -EINVAL;\n> > >> +\t\t\t}\n> > >> +\t\t}\n> > >> +\t}\n> > >> +\n> > >> +\treturn 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(V4L2Controls &ctrls)\n> > >> +{\n> > >> +\tunsigned int count = ctrls.size();\n> > >> +\tif (!count)\n> > >> +\t\treturn -EINVAL;\n> > >> +\n> > >> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\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(ctrl);\n> > >> +\t\t\t\tv4l2_ctrl->value64 = c->value();\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(ctrl);\n> > >> +\t\t\t\tv4l2_ctrl->value = c->value();\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 controls: \" << strerror(-ret);\n> > >> +\t\treturn ret;\n> > >> +\t}\n> > >> +\n> > >> +\treturn 0;\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":"","Received":["from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 44DBE60B1B\n\tfor ;\n\tTue, 11 Jun 2019 19:34:16 +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 relay1-d.mail.gandi.net (Postfix) with ESMTPSA id AE08C240007;\n\tTue, 11 Jun 2019 17:34:15 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 11 Jun 2019 19:35:28 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190611173528.lhob6ni3hcegz2g7@uno.localdomain>","References":"<20190610164052.30741-1-jacopo@jmondi.org>\n\t<20190610164052.30741-5-jacopo@jmondi.org>\n\t<20190611114126.GA11268@pendragon.ideasonboard.com>\n\t<20190611131021.fuh2f4uhuw5acmwf@uno.localdomain>\n\t<20190611152658.GP5016@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"njuguk5cwqao5nn7\"","Content-Disposition":"inline","In-Reply-To":"<20190611152658.GP5016@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Jun 2019 17:34:16 -0000"}},{"id":1862,"web_url":"https://patchwork.libcamera.org/comment/1862/","msgid":"<20190611175527.GQ5016@pendragon.ideasonboard.com>","date":"2019-06-11T17:55:27","subject":"Re: [libcamera-devel] [PATCH v2 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 Tue, Jun 11, 2019 at 07:35:28PM +0200, Jacopo Mondi wrote:\n> On Tue, Jun 11, 2019 at 06:26:58PM +0300, Laurent Pinchart wrote:\n> > On Tue, Jun 11, 2019 at 03:10:21PM +0200, Jacopo Mondi wrote:\n> >> On Tue, Jun 11, 2019 at 02:41:26PM +0300, Laurent Pinchart wrote:\n> >>> On Mon, Jun 10, 2019 at 06:40:50PM +0200, 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 \n> >>>> ---\n> >>>> src/libcamera/include/v4l2_base.h | 11 ++\n> >>>> src/libcamera/v4l2_base.cpp | 238 ++++++++++++++++++++++++++++++\n> >>>> 2 files changed, 249 insertions(+)\n> >>>>\n> >>>> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> >>>> index 2fda81a960d2..17ea734c8f49 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 \n> >>>>\n> >>>> +#include \n> >>>\n> >>> Just forward-declare the required classes and structures.\n> >>>\n> >>>> +\n> >>>> namespace libcamera {\n> >>>>\n> >>>> class V4L2Base\n> >>>> @@ -22,8 +24,17 @@ public:\n> >>>> \tvirtual void close() = 0;\n> >>>> \tbool isOpen() const;\n> >>>>\n> >>>> +\tint getControls(std::vector &ctrl_ids,\n> >>>> +\t\t\tV4L2Controls *ctrls);\n> >>>\n> >>> \tconst std::vector &ctrl_ids,\n> >>>\n> >>> I still wonder if it wouldn't be simpler for applications to fill the\n> >>> V4L2Controls with the control IDs to read, and just pass the\n> >>> V4L2Controls pointer to this function. In any case, this is better than\n> >>> the first version.\n> >>\n> >> I thought about that, but I don't like the fact that providing\n> >> controls without a value would require to keep track of the state of a\n> >> control (initialized? empty? with a default value? read from the HW?).\n> >>\n> >> In this way, we know that the V4L2Controls instance will contain\n> >> controls with a value read from the hardware, as well as the one\n> >> provided to setControl will contain values set by the user and applied\n> >> to the HW. It's much easier for me not having to keep track of the\n> >> control state (to prevent, say, reading a control without a value\n> >> assigned). I can change this by popular demand though. No issues.\n> >\n> > I won't push too hard :-) But I'm wondering what your real concern is\n> > here. Are you worried that an application will create a V4L2Controls\n> > instance, populate it with controls to be set and pass it to\n> > V4L2Base::setControls(), and then later pass the same instance to\n> > V4L2Base::getControls() ?\n> \n> What worries me is that application will create controls without a\n> value and controls with a value, and there is no distinction between\n> the two, so they could call a get$TYPE() and get back meaningless\n> values from it.\n\nSo what worries you would be\n\n\tV4L2Controls ctrls;\n\tint value;\n\n\tctrls.get(V4L2_CID_EXPOSURE);\n\tvalue = ctrls.getIntegerV4L2_CID_EXPOSURE);\n\nand then using value ? If an application shoots itself in the foot with\nso much dedication, do we really need to feel sorry for it ? :-)\n\n> That calls for keeping track of the control state, or distinguish\n> read/write controls, which I fear will be cumbersome and unecessary\n> complicated. Am I worrying too much?\n> \n> >>>> +\tint setControls(V4L2Controls &ctrls);\n> >>>\n> >>> You should pass a const V4L2Controls &ctrls. Or, maybe you should\n> >>> instead pass a non-const pointer, as setControls() should return the\n> >>> value set for each control.\n> >>\n> >> Not sure about const, the value of the single V4L2Control should be\n> >> updated to reflect what is applied to the HW.\n> >\n> > I agree, hence the second sentence :-) I would then pass the\n> > V4L2Controls by pointer to reflect that.\n> \n> Yes indeed.\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..cbd7a551130f 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 \n> >>>> +#include \n> >>>> +#include \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,234 @@ 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> >>>> +int V4L2Base::getControls(std::vector &ctrl_ids,\n> >>>> +\t\t\t V4L2Controls *controls)\n> >>>> +{\n> >>>> +\tunsigned int count = ctrl_ids.size();\n> >>>> +\tif (!count)\n> >>>> +\t\treturn -EINVAL;\n> >>>> +\n> >>>> +\tcontrols->clear();\n> >>>> +\n> >>>> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\n> >>>\n> >>> You're leaking memory. The simplest solution would be\n> >>>\n> >>> \tstruct v4l2_ext_controls v4l2_ctrls[count];\n> >>\n> >> Indeed!\n> >> I wonder why valgrind reported no leaks...\n> >>\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 ret;\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\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\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\n> >>>> +\t\t\t\t\t (new uint32_t[v4l2_ctrl->size]);\n> >>>> +\t\t\tbreak;\n> >>>\n> >>> This is lots of new's, and they're all leaked. A better solution would\n> >>> be to populate ctrls with the V4L2Control instances already, and use the\n> >>> pointer to the internal memory buffers directly.\n> >>\n> >> Sorry, I didn't get you, what's 'ctrls' here?\n> >\n> > Sorry, I meant the controls parameter.\n> >\n> >> The memory here allocated (and not released, my bad) serves to the\n> >> kernel to copy the data payload to userspace. It is then copied inside\n> >> the V4L2Control, where memory is reserved at creation time.\n> >>\n> >> What I could do, and I think that's what you suggested is to create\n> >> the V4L2Control in advance (once I know the required memory size) and\n> >> point the 'struct v4l2_ext_control' data pointer to that memory, and\n> >> save one copy.\n> >\n> > Correct, that's what I meant.\n> >\n> >> Personally, I'm also fine dropping support for payload controls for\n> >> this first version, as far as I could tell, there's no strict\n> >> requirement for any of them on IPU3 (as Kieran suggested).\n> >>\n> >> What's your opinion?\n> >\n> > We have very few payload controls in mainline, so that could indeed be a\n> > good idea. I would however experiment with creation of the V4L2Control\n> > instance before calling VIDIOC_G_EXT_CTRLS to see if it would be an\n> > option for a later support of compound controls.\n> \n> I'm a bit in two minds here:\n> If we drop support for controls with payload it calls for dropping the\n> template and use Kieran's ControlValue. But then, once we'll ever have\n> to support them, I fear the ControlValue class would be a waste of space\n> and less flexible to use with controls with payload...\n> \n> It seems to me that dropping payload support and use ControlValue as\n> backing storage would make it easier to get the series in, so I'm\n> tempted to go for the \"is good for now\" way and do what is outlined\n> here above...\n\nYou could keep the template, but it would indeed be nice to use the same\nclass to store the control value. It's not a hard requirement though, if\nthere are good reasons not to do so, I'm fine keeping them separate.\n\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\treturn ret;\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> >>>> +\tLOG(Error) << __func__;\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 ret;\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\tcontrols->set(v4l2_ctrl->id, size,\n> >>>> +\t\t\t\t\t v4l2_ctrl->p_u8);\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\tcase V4L2_CTRL_TYPE_U16:\n> >>>> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> >>>> +\t\t\t\t\t v4l2_ctrl->p_u16);\n> >>>> +\t\t\t\tbreak;\n> >>>> +\t\t\tcase V4L2_CTRL_TYPE_U32:\n> >>>> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> >>>> +\t\t\t\t\t v4l2_ctrl->p_u32);\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 -EINVAL;\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\tcontrols->set(v4l2_ctrl->id, static_cast\n> >>>> +\t\t\t\t\t (v4l2_ctrl->value64));\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\tcontrols->set(v4l2_ctrl->id, v4l2_ctrl->value);\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 -EINVAL;\n> >>>> +\t\t\t}\n> >>>> +\t\t}\n> >>>> +\t}\n> >>>> +\n> >>>> +\treturn 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(V4L2Controls &ctrls)\n> >>>> +{\n> >>>> +\tunsigned int count = ctrls.size();\n> >>>> +\tif (!count)\n> >>>> +\t\treturn -EINVAL;\n> >>>> +\n> >>>> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\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(ctrl);\n> >>>> +\t\t\t\tv4l2_ctrl->value64 = c->value();\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(ctrl);\n> >>>> +\t\t\t\tv4l2_ctrl->value = c->value();\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 controls: \" << strerror(-ret);\n> >>>> +\t\treturn ret;\n> >>>> +\t}\n> >>>> +\n> >>>> +\treturn 0;\n> >>>> +}\n> >>>> +\n> >>>> /**\n> >>>> * \\var V4L2Base::fd_\n> >>>> * \\brief The V4L2 device or subdevice device node file descriptor","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F1F9618FC\n\tfor ;\n\tTue, 11 Jun 2019 19:55:43 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D46532AF;\n\tTue, 11 Jun 2019 19:55:42 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560275743;\n\tbh=Q0ysSE3gc7feT6+xbvIYhb99VZiaa2DOUdsHcxP3ylI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pKqkxKJKz1QSWdAieObHJnuEhHjU1XewLnQFogzH2dSLVA3xD6T0XbiHFtaHJ1aJI\n\tDb5thdyy39sz6HB3tmLjv4DEVgZmfgSebIH3Ytql9F02owNwoEyQZjTxlb3d374KNO\n\tTviB/8X6aOxBYHH734fXbvT3SOEiw+WF4g/K7buI=","Date":"Tue, 11 Jun 2019 20:55:27 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190611175527.GQ5016@pendragon.ideasonboard.com>","References":"<20190610164052.30741-1-jacopo@jmondi.org>\n\t<20190610164052.30741-5-jacopo@jmondi.org>\n\t<20190611114126.GA11268@pendragon.ideasonboard.com>\n\t<20190611131021.fuh2f4uhuw5acmwf@uno.localdomain>\n\t<20190611152658.GP5016@pendragon.ideasonboard.com>\n\t<20190611173528.lhob6ni3hcegz2g7@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190611173528.lhob6ni3hcegz2g7@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Jun 2019 17:55:43 -0000"}},{"id":1866,"web_url":"https://patchwork.libcamera.org/comment/1866/","msgid":"<20190612072934.32pd47a4zqfebiwk@uno.localdomain>","date":"2019-06-12T07:29:34","subject":"Re: [libcamera-devel] [PATCH v2 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 Laurent,\n\nOn Tue, Jun 11, 2019 at 08:55:27PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Tue, Jun 11, 2019 at 07:35:28PM +0200, Jacopo Mondi wrote:\n> > On Tue, Jun 11, 2019 at 06:26:58PM +0300, Laurent Pinchart wrote:\n> > > On Tue, Jun 11, 2019 at 03:10:21PM +0200, Jacopo Mondi wrote:\n> > >> On Tue, Jun 11, 2019 at 02:41:26PM +0300, Laurent Pinchart wrote:\n> > >>> On Mon, Jun 10, 2019 at 06:40:50PM +0200, 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 \n> > >>>> ---\n> > >>>> src/libcamera/include/v4l2_base.h | 11 ++\n> > >>>> src/libcamera/v4l2_base.cpp | 238 ++++++++++++++++++++++++++++++\n> > >>>> 2 files changed, 249 insertions(+)\n> > >>>>\n> > >>>> diff --git a/src/libcamera/include/v4l2_base.h b/src/libcamera/include/v4l2_base.h\n> > >>>> index 2fda81a960d2..17ea734c8f49 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 \n> > >>>>\n> > >>>> +#include \n> > >>>\n> > >>> Just forward-declare the required classes and structures.\n> > >>>\n> > >>>> +\n> > >>>> namespace libcamera {\n> > >>>>\n> > >>>> class V4L2Base\n> > >>>> @@ -22,8 +24,17 @@ public:\n> > >>>> \tvirtual void close() = 0;\n> > >>>> \tbool isOpen() const;\n> > >>>>\n> > >>>> +\tint getControls(std::vector &ctrl_ids,\n> > >>>> +\t\t\tV4L2Controls *ctrls);\n> > >>>\n> > >>> \tconst std::vector &ctrl_ids,\n> > >>>\n> > >>> I still wonder if it wouldn't be simpler for applications to fill the\n> > >>> V4L2Controls with the control IDs to read, and just pass the\n> > >>> V4L2Controls pointer to this function. In any case, this is better than\n> > >>> the first version.\n> > >>\n> > >> I thought about that, but I don't like the fact that providing\n> > >> controls without a value would require to keep track of the state of a\n> > >> control (initialized? empty? with a default value? read from the HW?).\n> > >>\n> > >> In this way, we know that the V4L2Controls instance will contain\n> > >> controls with a value read from the hardware, as well as the one\n> > >> provided to setControl will contain values set by the user and applied\n> > >> to the HW. It's much easier for me not having to keep track of the\n> > >> control state (to prevent, say, reading a control without a value\n> > >> assigned). I can change this by popular demand though. No issues.\n> > >\n> > > I won't push too hard :-) But I'm wondering what your real concern is\n> > > here. Are you worried that an application will create a V4L2Controls\n> > > instance, populate it with controls to be set and pass it to\n> > > V4L2Base::setControls(), and then later pass the same instance to\n> > > V4L2Base::getControls() ?\n> >\n> > What worries me is that application will create controls without a\n> > value and controls with a value, and there is no distinction between\n> > the two, so they could call a get$TYPE() and get back meaningless\n> > values from it.\n>\n> So what worries you would be\n>\n> \tV4L2Controls ctrls;\n> \tint value;\n>\n> \tctrls.get(V4L2_CID_EXPOSURE);\n> \tvalue = ctrls.getIntegerV4L2_CID_EXPOSURE);\n>\n> and then using value ? If an application shoots itself in the foot with\n> so much dedication, do we really need to feel sorry for it ? :-)\n\nWell, yes, in this case the application is defintely looking for\ntroubles. I think in more complex sequences, keeping track of the\nstate of a control might be add complications both for the app, both\nin the framework, as we'll have to deal with non-initialized values.\n\nAnyway, you and Kieran do not seem to be worried about this, so it\nmight just be me. I'll try go your suggested way then and see how it\nlooks.\n\nSide note: I would not have ctrls.set(ID, value) and ctrls.get(ID) but\nrather ctrls.add(ID, value) and ctrls.add(ID). The same ctrls can be\nused as argument in getControls() and setControls() and a control\ninitialized with a value can be read as well as a not initialized one.\n\n>\n> > That calls for keeping track of the control state, or distinguish\n> > read/write controls, which I fear will be cumbersome and unecessary\n> > complicated. Am I worrying too much?\n> >\n> > >>>> +\tint setControls(V4L2Controls &ctrls);\n> > >>>\n> > >>> You should pass a const V4L2Controls &ctrls. Or, maybe you should\n> > >>> instead pass a non-const pointer, as setControls() should return the\n> > >>> value set for each control.\n> > >>\n> > >> Not sure about const, the value of the single V4L2Control should be\n> > >> updated to reflect what is applied to the HW.\n> > >\n> > > I agree, hence the second sentence :-) I would then pass the\n> > > V4L2Controls by pointer to reflect that.\n> >\n> > Yes indeed.\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..cbd7a551130f 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 \n> > >>>> +#include \n> > >>>> +#include \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,234 @@ 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> > >>>> +int V4L2Base::getControls(std::vector &ctrl_ids,\n> > >>>> +\t\t\t V4L2Controls *controls)\n> > >>>> +{\n> > >>>> +\tunsigned int count = ctrl_ids.size();\n> > >>>> +\tif (!count)\n> > >>>> +\t\treturn -EINVAL;\n> > >>>> +\n> > >>>> +\tcontrols->clear();\n> > >>>> +\n> > >>>> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\n> > >>>\n> > >>> You're leaking memory. The simplest solution would be\n> > >>>\n> > >>> \tstruct v4l2_ext_controls v4l2_ctrls[count];\n> > >>\n> > >> Indeed!\n> > >> I wonder why valgrind reported no leaks...\n> > >>\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 ret;\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\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\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\n> > >>>> +\t\t\t\t\t (new uint32_t[v4l2_ctrl->size]);\n> > >>>> +\t\t\tbreak;\n> > >>>\n> > >>> This is lots of new's, and they're all leaked. A better solution would\n> > >>> be to populate ctrls with the V4L2Control instances already, and use the\n> > >>> pointer to the internal memory buffers directly.\n> > >>\n> > >> Sorry, I didn't get you, what's 'ctrls' here?\n> > >\n> > > Sorry, I meant the controls parameter.\n> > >\n> > >> The memory here allocated (and not released, my bad) serves to the\n> > >> kernel to copy the data payload to userspace. It is then copied inside\n> > >> the V4L2Control, where memory is reserved at creation time.\n> > >>\n> > >> What I could do, and I think that's what you suggested is to create\n> > >> the V4L2Control in advance (once I know the required memory size) and\n> > >> point the 'struct v4l2_ext_control' data pointer to that memory, and\n> > >> save one copy.\n> > >\n> > > Correct, that's what I meant.\n> > >\n> > >> Personally, I'm also fine dropping support for payload controls for\n> > >> this first version, as far as I could tell, there's no strict\n> > >> requirement for any of them on IPU3 (as Kieran suggested).\n> > >>\n> > >> What's your opinion?\n> > >\n> > > We have very few payload controls in mainline, so that could indeed be a\n> > > good idea. I would however experiment with creation of the V4L2Control\n> > > instance before calling VIDIOC_G_EXT_CTRLS to see if it would be an\n> > > option for a later support of compound controls.\n> >\n> > I'm a bit in two minds here:\n> > If we drop support for controls with payload it calls for dropping the\n> > template and use Kieran's ControlValue. But then, once we'll ever have\n> > to support them, I fear the ControlValue class would be a waste of space\n> > and less flexible to use with controls with payload...\n> >\n> > It seems to me that dropping payload support and use ControlValue as\n> > backing storage would make it easier to get the series in, so I'm\n> > tempted to go for the \"is good for now\" way and do what is outlined\n> > here above...\n>\n> You could keep the template, but it would indeed be nice to use the same\n> class to store the control value. It's not a hard requirement though, if\n> there are good reasons not to do so, I'm fine keeping them separate.\n>\n\nI'll try to use Kieran's ControlValue in the next version and\ndrop support for compound and strings controls.\n\nThanks\n j\n\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\treturn ret;\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> > >>>> +\tLOG(Error) << __func__;\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 ret;\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\tcontrols->set(v4l2_ctrl->id, size,\n> > >>>> +\t\t\t\t\t v4l2_ctrl->p_u8);\n> > >>>> +\t\t\t\tbreak;\n> > >>>> +\t\t\tcase V4L2_CTRL_TYPE_U16:\n> > >>>> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> > >>>> +\t\t\t\t\t v4l2_ctrl->p_u16);\n> > >>>> +\t\t\t\tbreak;\n> > >>>> +\t\t\tcase V4L2_CTRL_TYPE_U32:\n> > >>>> +\t\t\t\tcontrols->set(v4l2_ctrl->id, size,\n> > >>>> +\t\t\t\t\t v4l2_ctrl->p_u32);\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 -EINVAL;\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\tcontrols->set(v4l2_ctrl->id, static_cast\n> > >>>> +\t\t\t\t\t (v4l2_ctrl->value64));\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\tcontrols->set(v4l2_ctrl->id, v4l2_ctrl->value);\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 -EINVAL;\n> > >>>> +\t\t\t}\n> > >>>> +\t\t}\n> > >>>> +\t}\n> > >>>> +\n> > >>>> +\treturn 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(V4L2Controls &ctrls)\n> > >>>> +{\n> > >>>> +\tunsigned int count = ctrls.size();\n> > >>>> +\tif (!count)\n> > >>>> +\t\treturn -EINVAL;\n> > >>>> +\n> > >>>> +\tstruct v4l2_ext_control *v4l2_ctrls = new struct v4l2_ext_control[count];\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(ctrl);\n> > >>>> +\t\t\t\tv4l2_ctrl->value64 = c->value();\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(ctrl);\n> > >>>> +\t\t\t\tv4l2_ctrl->value = c->value();\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 controls: \" << strerror(-ret);\n> > >>>> +\t\treturn ret;\n> > >>>> +\t}\n> > >>>> +\n> > >>>> +\treturn 0;\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":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC51660B19\n\tfor ;\n\tWed, 12 Jun 2019 09:28:21 +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 relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 2A805C0005;\n\tWed, 12 Jun 2019 07:28:20 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 12 Jun 2019 09:29:34 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190612072934.32pd47a4zqfebiwk@uno.localdomain>","References":"<20190610164052.30741-1-jacopo@jmondi.org>\n\t<20190610164052.30741-5-jacopo@jmondi.org>\n\t<20190611114126.GA11268@pendragon.ideasonboard.com>\n\t<20190611131021.fuh2f4uhuw5acmwf@uno.localdomain>\n\t<20190611152658.GP5016@pendragon.ideasonboard.com>\n\t<20190611173528.lhob6ni3hcegz2g7@uno.localdomain>\n\t<20190611175527.GQ5016@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"dovso4p5btavah5a\"","Content-Disposition":"inline","In-Reply-To":"<20190611175527.GQ5016@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 12 Jun 2019 07:28:21 -0000"}}]