[{"id":1953,"web_url":"https://patchwork.libcamera.org/comment/1953/","msgid":"<20190619203655.GN21753@pendragon.ideasonboard.com>","date":"2019-06-19T20:36:55","subject":"Re: [libcamera-devel] [PATCH v4 3/6] libcamera: v4l2_device:\n\tImplement get and set controls","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 Wed, Jun 19, 2019 at 01:08:55PM +0200, Jacopo Mondi wrote:\n> Implement getControls() and setControls() operations in V4L2Device class.\n> \n> Both operations take a V4L2Controls instance and read or write the V4L2\n> controls on the V4L2 device.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/v4l2_device.h | 6 +\n> src/libcamera/v4l2_device.cpp | 189 ++++++++++++++++++++++++++++\n> 2 files changed, 195 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index 563be151c257..e66cc7ebe737 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -15,6 +15,7 @@\n> namespace libcamera {\n> \n> class V4L2ControlInfo;\n> +class V4L2Controls;\n> \n> class V4L2Device : protected Loggable\n> {\n> @@ -23,6 +24,10 @@ public:\n> \tbool isOpen() const { return fd_ != -1; }\n> \tconst std::string &deviceNode() const { return deviceNode_; }\n> \n> +\tV4L2ControlInfo *getControlInfo(unsigned int id);\n\nShould this be moved to the previous patch ?\n\nThe returned pointer should be const, and the function should be const\ntoo.\n\n> +\tint getControls(V4L2Controls *ctrls);\n> +\tint setControls(V4L2Controls *ctrls);\n> +\n> protected:\n> \tV4L2Device(const std::string &deviceNode, const std::string &logTag);\n> \t~V4L2Device();\n> @@ -37,6 +42,7 @@ protected:\n> \n> private:\n> \tvoid listControls();\n> +\tint validateControlType(V4L2ControlInfo *info);\n> \n> \tstd::map controls_;\n> \tstd::string deviceNode_;\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 58dd94836686..5af0ddc468fe 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -66,6 +66,170 @@ void V4L2Device::close()\n> * \\return The device node path\n> */\n> \n> +/**\n> + * \\brief Get informations for control with \\a id\n\n\"Retrieve information about a control\" ?\n\n> + * \\param[in] id The control id to search info for\n\n\"The control ID\" should be enough.\n\n> + * \\return The V4L2ControlInfo associated to the V4L2 control with \\a id or\n> + * nullptr if the control is not supported by the V4L2Device\n\n\\return A pointer to the V4L2ControlInfo for control \\a id, or nullptr\nif the device doesn't have such a control\n\n?\n\n> + */\n> +V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id)\n> +{\n> +\tauto it = controls_.find(id);\n> +\tif (it == controls_.end())\n> +\t\treturn nullptr;\n> +\n> +\treturn it->second;\n> +}\n> +\n> +/**\n> + * \\brief Read values of a list of controls from the device\n\n\"Read controls from the device\"\n\n> + * \\param[inout] ctrls The list of controls to read\n> + *\n> + * Read the value of each V4L2Controls contained in \\a ctrls, overwriting\n> + * it's current value with the one returned by the device.\n\nIt's hard to write precise documentation without making it difficult to\nread :-/ How about\n\n\"Read the value of each control contained in \\a ctrls, and store the\nvalue in the corresponding \\a ctrls entry.\"\n\n> + *\n> + * Each V4L2Control instance in \\a ctrls should be supported by the device.\n\n\"If one control in \\a ctrls is not supported by the device this method\nreturns an error. In that case the values stored in \\a ctrls are\nundefined.\"\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2Device::getControls(V4L2Controls *ctrls)\n> +{\n> +\tunsigned int count = ctrls->size();\n> +\tif (count == 0)\n> +\t\treturn 0;\n> +\n> +\tstruct V4L2ControlInfo *controlInfo[count];\n> +\tstruct v4l2_ext_control v4l2Ctrls[count];\n> +\tfor (unsigned int i = 0; i < count; ++i) {\n> +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> +\n> +\t\t/* Validate the control. */\n> +\t\tV4L2ControlInfo *info = getControlInfo(ctrl->id());\n> +\t\tif (!info) {\n> +\t\t\tLOG(V4L2, Error) << \"Control '\" << ctrl->id()\n> +\t\t\t\t\t << \"' not valid\";\n\ns/not valid/not found/\n\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tif (validateControlType(info))\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tcontrolInfo[i] = info;\n> +\n> +\t\t/* Prepare the v4l2_ext_control entry for the read operation. */\n> +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> +\t\tv4l2Ctrl->id = info->id();\n> +\t\tv4l2Ctrl->size = info->size();\n\nsize is only needed for controls with payloads, which we don't support.\nI would leave it set to 0, otherwise if we try to get a payload control\nby mistake, and the value happens to be a valid pointer to our memory\naddress space, the kernel could overwrite that.\n\nI would then also remove the v4l2Ctrl variable and write\n\n\t\tv4l2Ctrls[i].id = ctrl->id();\n\n> +\t}\n> +\n> +\tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> +\tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> +\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> +\tv4l2ExtCtrls.count = count;\n> +\n> +\tint ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n> +\tif (ret) {\n> +\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> +\t\t\t\t << strerror(ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/*\n> +\t * For each control read from the device, set the value in the\n> +\t * V4L2ControlValue provided by the caller.\n\nThat's called V4L2Control now, not V4L2ControlValue.\n\n> +\t */\n> +\tfor (unsigned int i = 0; i < count; ++i) {\n> +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> +\t\tV4L2ControlInfo *info = controlInfo[i];\n> +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> +\n> +\t\tswitch (info->type()) {\n> +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\t\t\tctrl->setValue(v4l2Ctrl->value64);\n> +\t\t\tbreak;\n> +\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> +\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> +\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> +\t\tcase V4L2_CTRL_TYPE_MENU:\n> +\t\t\tctrl->setValue(static_cast(v4l2Ctrl->value));\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Write a list of controls to the device\n\n\"Write controls to the device\"\n\n> + * \\param[in] ctrls The list of controls to apply\n\ns/apply/write/\n\n> + *\n> + * Write the value of each V4L2Control contained in \\a ctrls. Each\n> + * control should be initialized by the caller with a value, otherwise\n> + * the result of the operation is undefined.\n> + *\n> + * The value of each control is not updated to reflect what has been\n> + * actually applied on the device. To read the actual value of a control\n> + * after a setControls(), users should read the control value back with\n> + * getControls().\n\nWhy is that ? It would require another potentially expensive operation.\n\n> + *\n> + * Each V4L2Control instance in \\a ctrls should be supported by the device.\n\n\"If one control in \\a ctrls is not supported by the device this method\nreturns an error.\" to copy the text from getControls() ?\n\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n\nI wonder if we should return the number of controls successfully\nwritten. This would be 0 if the pre-validation fails, and we could use\nv4l2_ext_controls::error_idx to get the index of the control that failed\n(note that if error_idx == count then no control have been written).\n\n> + */\n> +int V4L2Device::setControls(V4L2Controls *ctrls)\n> +{\n> +\tunsigned int count = ctrls->size();\n> +\tif (count == 0)\n> +\t\treturn 0;\n> +\n> +\tstruct v4l2_ext_control v4l2Ctrls[count];\n> +\tfor (unsigned int i = 0; i < count; ++i) {\n> +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> +\n> +\t\t/* Validate the control. */\n> +\t\tV4L2ControlInfo *info = getControlInfo(ctrl->id());\n> +\t\tif (!info) {\n> +\t\t\tLOG(V4L2, Error) << \"Control '\" << ctrl->id()\n> +\t\t\t\t\t << \"' not valid\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tif (validateControlType(info))\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\t/* Prepare the v4l2_ext_control entry for the write operation. */\n> +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> +\t\tv4l2Ctrl->id = info->id();\n> +\t\tv4l2Ctrl->size = info->size();\n> +\n> +\t\tswitch (info->type()) {\n> +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\t\t\tv4l2Ctrl->value64 = ctrl->value();\n> +\t\t\tbreak;\n> +\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> +\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> +\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> +\t\tcase V4L2_CTRL_TYPE_MENU:\n> +\t\t\tv4l2Ctrl->value = static_cast(ctrl->value());\n> +\t\t\tbreak;\n> +\t\t}\n\nThis switch duplicates the one in validateControlType(). I would just\nadd a default case here and return an error, removing the call to\nvalidateControlType(). The method would be left iwht a single caller, so\nI would then inline it in getControls().\n\n> +\t}\n> +\n> +\tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> +\tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> +\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> +\tv4l2ExtCtrls.count = count;\n> +\n> +\tint ret = ioctl(VIDIOC_S_EXT_CTRLS, &v4l2ExtCtrls);\n> +\tif (ret) {\n> +\t\tLOG(V4L2, Error) << \"Unable to write controls: \"\n> +\t\t\t\t << strerror(ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Construct a V4L2Device\n> * \\param[in] deviceNode The device node filesystem path\n> @@ -171,4 +335,29 @@ void V4L2Device::listControls()\n> \t}\n> }\n> \n> +/*\n> + * \\brief Make sure the control type is supported\n> + * \\param[in] info The V4L2ControlInfo to inspect type of\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The control type is not supported\n> + */\n> +int V4L2Device::validateControlType(V4L2ControlInfo *info)\n> +{\n> +\t/* \\todo support compound and menu controls. */\n> +\tswitch (info->type()) {\n> +\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\tcase V4L2_CTRL_TYPE_INTEGER:\n> +\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> +\tcase V4L2_CTRL_TYPE_BUTTON:\n> +\tcase V4L2_CTRL_TYPE_MENU:\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tLOG(V4L2, Error) << \"Control type '\" << info->type()\n> +\t\t\t\t << \"' not supported\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n\nAnother option would be to skip unsupported control types when\nenumerating them, so the getControlInfo() call would return a nullptr.\n\n> +\n> } /* namespace libcamera */","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 ADB5E60BC5\n\tfor ;\n\tWed, 19 Jun 2019 22:37:13 +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 27C3F333;\n\tWed, 19 Jun 2019 22:37:13 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560976633;\n\tbh=7g9P9eOqBYvVpfw9HKh8u9xxa9DJKwvpQCL4s+FPtpE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=D/GuOsiGYAl4dygnijiu7xo5aiTWLtCE3XRN1ZlK2kjs5iGUXhR4ZyuQ3zaIWuw1x\n\tixUv7orG+RHinlJqq0crQLSqAgjKvtuepUTRgHWGu4thqUrdPjlQOkPawUZKIdZi7y\n\tpgjlE/h3+6DWJflHTFsF21I+NPNmS6mXDontMib8=","Date":"Wed, 19 Jun 2019 23:36:55 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190619203655.GN21753@pendragon.ideasonboard.com>","References":"<20190619110858.20980-1-jacopo@jmondi.org>\n\t<20190619110858.20980-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190619110858.20980-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 3/6] libcamera: v4l2_device:\n\tImplement get and set controls","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, 19 Jun 2019 20:37:13 -0000"}},{"id":1959,"web_url":"https://patchwork.libcamera.org/comment/1959/","msgid":"<20190620101119.xbxfv4nsxeisyy7j@uno.localdomain>","date":"2019-06-20T10:11:48","subject":"Re: [libcamera-devel] [PATCH v4 3/6] libcamera: v4l2_device:\n\tImplement get and set controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n thanks for review.\n\nOn Wed, Jun 19, 2019 at 11:36:55PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Jun 19, 2019 at 01:08:55PM +0200, Jacopo Mondi wrote:\n> > Implement getControls() and setControls() operations in V4L2Device class.\n> >\n> > Both operations take a V4L2Controls instance and read or write the V4L2\n> > controls on the V4L2 device.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/v4l2_device.h | 6 +\n> > src/libcamera/v4l2_device.cpp | 189 ++++++++++++++++++++++++++++\n> > 2 files changed, 195 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > index 563be151c257..e66cc7ebe737 100644\n> > --- a/src/libcamera/include/v4l2_device.h\n> > +++ b/src/libcamera/include/v4l2_device.h\n> > @@ -15,6 +15,7 @@\n> > namespace libcamera {\n> >\n> > class V4L2ControlInfo;\n> > +class V4L2Controls;\n> >\n> > class V4L2Device : protected Loggable\n> > {\n> > @@ -23,6 +24,10 @@ public:\n> > \tbool isOpen() const { return fd_ != -1; }\n> > \tconst std::string &deviceNode() const { return deviceNode_; }\n> >\n> > +\tV4L2ControlInfo *getControlInfo(unsigned int id);\n>\n> Should this be moved to the previous patch ?\n\nWhy so, it is only used here...\n\n>\n> The returned pointer should be const, and the function should be const\n> too.\n\nIndeed. Fixed.\n\n>\n> > +\tint getControls(V4L2Controls *ctrls);\n> > +\tint setControls(V4L2Controls *ctrls);\n> > +\n> > protected:\n> > \tV4L2Device(const std::string &deviceNode, const std::string &logTag);\n> > \t~V4L2Device();\n> > @@ -37,6 +42,7 @@ protected:\n> >\n> > private:\n> > \tvoid listControls();\n> > +\tint validateControlType(V4L2ControlInfo *info);\n> >\n> > \tstd::map controls_;\n> > \tstd::string deviceNode_;\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 58dd94836686..5af0ddc468fe 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -66,6 +66,170 @@ void V4L2Device::close()\n> > * \\return The device node path\n> > */\n> >\n> > +/**\n> > + * \\brief Get informations for control with \\a id\n>\n> \"Retrieve information about a control\" ?\n>\n> > + * \\param[in] id The control id to search info for\n>\n> \"The control ID\" should be enough.\n>\n> > + * \\return The V4L2ControlInfo associated to the V4L2 control with \\a id or\n> > + * nullptr if the control is not supported by the V4L2Device\n>\n> \\return A pointer to the V4L2ControlInfo for control \\a id, or nullptr\n> if the device doesn't have such a control\n>\n> ?\n>\n\nTaken in.\n\n> > + */\n> > +V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id)\n> > +{\n> > +\tauto it = controls_.find(id);\n> > +\tif (it == controls_.end())\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn it->second;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Read values of a list of controls from the device\n>\n> \"Read controls from the device\"\n>\n> > + * \\param[inout] ctrls The list of controls to read\n> > + *\n> > + * Read the value of each V4L2Controls contained in \\a ctrls, overwriting\n> > + * it's current value with the one returned by the device.\n>\n> It's hard to write precise documentation without making it difficult to\n> read :-/ How about\n>\n> \"Read the value of each control contained in \\a ctrls, and store the\n> value in the corresponding \\a ctrls entry.\"\n>\n> > + *\n> > + * Each V4L2Control instance in \\a ctrls should be supported by the device.\n>\n> \"If one control in \\a ctrls is not supported by the device this method\n> returns an error. In that case the values stored in \\a ctrls are\n> undefined.\"\n>\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +int V4L2Device::getControls(V4L2Controls *ctrls)\n> > +{\n> > +\tunsigned int count = ctrls->size();\n> > +\tif (count == 0)\n> > +\t\treturn 0;\n> > +\n> > +\tstruct V4L2ControlInfo *controlInfo[count];\n> > +\tstruct v4l2_ext_control v4l2Ctrls[count];\n> > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> > +\n> > +\t\t/* Validate the control. */\n> > +\t\tV4L2ControlInfo *info = getControlInfo(ctrl->id());\n> > +\t\tif (!info) {\n> > +\t\t\tLOG(V4L2, Error) << \"Control '\" << ctrl->id()\n> > +\t\t\t\t\t << \"' not valid\";\n>\n> s/not valid/not found/\n>\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tif (validateControlType(info))\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\tcontrolInfo[i] = info;\n> > +\n> > +\t\t/* Prepare the v4l2_ext_control entry for the read operation. */\n> > +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> > +\t\tv4l2Ctrl->id = info->id();\n> > +\t\tv4l2Ctrl->size = info->size();\n>\n> size is only needed for controls with payloads, which we don't support.\n> I would leave it set to 0, otherwise if we try to get a payload control\n> by mistake, and the value happens to be a valid pointer to our memory\n> address space, the kernel could overwrite that.\n>\n> I would then also remove the v4l2Ctrl variable and write\n>\n> \t\tv4l2Ctrls[i].id = ctrl->id();\n\nAck!\n\n>\n> > +\t}\n> > +\n> > +\tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> > +\tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > +\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> > +\tv4l2ExtCtrls.count = count;\n> > +\n> > +\tint ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n> > +\tif (ret) {\n> > +\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> > +\t\t\t\t << strerror(ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * For each control read from the device, set the value in the\n> > +\t * V4L2ControlValue provided by the caller.\n>\n> That's called V4L2Control now, not V4L2ControlValue.\n>\n> > +\t */\n> > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> > +\t\tV4L2ControlInfo *info = controlInfo[i];\n> > +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> > +\n> > +\t\tswitch (info->type()) {\n> > +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > +\t\t\tctrl->setValue(v4l2Ctrl->value64);\n> > +\t\t\tbreak;\n> > +\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> > +\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > +\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> > +\t\tcase V4L2_CTRL_TYPE_MENU:\n> > +\t\t\tctrl->setValue(static_cast(v4l2Ctrl->value));\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Write a list of controls to the device\n>\n> \"Write controls to the device\"\n>\n> > + * \\param[in] ctrls The list of controls to apply\n>\n> s/apply/write/\n>\n> > + *\n> > + * Write the value of each V4L2Control contained in \\a ctrls. Each\n> > + * control should be initialized by the caller with a value, otherwise\n> > + * the result of the operation is undefined.\n> > + *\n> > + * The value of each control is not updated to reflect what has been\n> > + * actually applied on the device. To read the actual value of a control\n> > + * after a setControls(), users should read the control value back with\n> > + * getControls().\n>\n> Why is that ? It would require another potentially expensive operation.\n>\n\nThis was actually something I was not sure of. Should we update the\nvalue after the control is written to HW? So every write is equivalent\nto a write+read?\n\n> > + *\n> > + * Each V4L2Control instance in \\a ctrls should be supported by the device.\n>\n> \"If one control in \\a ctrls is not supported by the device this method\n> returns an error.\" to copy the text from getControls() ?\n>\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n>\n> I wonder if we should return the number of controls successfully\n> written. This would be 0 if the pre-validation fails, and we could use\n> v4l2_ext_controls::error_idx to get the index of the control that failed\n> (note that if error_idx == count then no control have been written).\n>\n\nI generally prefer to have 0 for success, but in this case signaling\nthe error back is quite tricky, also because the extended ctrls API\ndoes not help here... Returning the number of written controls might\nbe a good idea. Should we do the same on read?\n\n\n> > + */\n> > +int V4L2Device::setControls(V4L2Controls *ctrls)\n> > +{\n> > +\tunsigned int count = ctrls->size();\n> > +\tif (count == 0)\n> > +\t\treturn 0;\n> > +\n> > +\tstruct v4l2_ext_control v4l2Ctrls[count];\n> > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> > +\n> > +\t\t/* Validate the control. */\n> > +\t\tV4L2ControlInfo *info = getControlInfo(ctrl->id());\n> > +\t\tif (!info) {\n> > +\t\t\tLOG(V4L2, Error) << \"Control '\" << ctrl->id()\n> > +\t\t\t\t\t << \"' not valid\";\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tif (validateControlType(info))\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\t/* Prepare the v4l2_ext_control entry for the write operation. */\n> > +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> > +\t\tv4l2Ctrl->id = info->id();\n> > +\t\tv4l2Ctrl->size = info->size();\n> > +\n> > +\t\tswitch (info->type()) {\n> > +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > +\t\t\tv4l2Ctrl->value64 = ctrl->value();\n> > +\t\t\tbreak;\n> > +\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> > +\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > +\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> > +\t\tcase V4L2_CTRL_TYPE_MENU:\n> > +\t\t\tv4l2Ctrl->value = static_cast(ctrl->value());\n> > +\t\t\tbreak;\n> > +\t\t}\n>\n> This switch duplicates the one in validateControlType(). I would just\n> add a default case here and return an error, removing the call to\n> validateControlType(). The method would be left iwht a single caller, so\n> I would then inline it in getControls().\n>\n\nAh yes, the switch is repeated. I can fail in the second one yes.\n\n> > +\t}\n> > +\n> > +\tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> > +\tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > +\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> > +\tv4l2ExtCtrls.count = count;\n> > +\n> > +\tint ret = ioctl(VIDIOC_S_EXT_CTRLS, &v4l2ExtCtrls);\n> > +\tif (ret) {\n> > +\t\tLOG(V4L2, Error) << \"Unable to write controls: \"\n> > +\t\t\t\t << strerror(ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > /**\n> > * \\brief Construct a V4L2Device\n> > * \\param[in] deviceNode The device node filesystem path\n> > @@ -171,4 +335,29 @@ void V4L2Device::listControls()\n> > \t}\n> > }\n> >\n> > +/*\n> > + * \\brief Make sure the control type is supported\n> > + * \\param[in] info The V4L2ControlInfo to inspect type of\n> > + * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -EINVAL The control type is not supported\n> > + */\n> > +int V4L2Device::validateControlType(V4L2ControlInfo *info)\n> > +{\n> > +\t/* \\todo support compound and menu controls. */\n> > +\tswitch (info->type()) {\n> > +\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > +\tcase V4L2_CTRL_TYPE_INTEGER:\n> > +\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > +\tcase V4L2_CTRL_TYPE_BUTTON:\n> > +\tcase V4L2_CTRL_TYPE_MENU:\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tLOG(V4L2, Error) << \"Control type '\" << info->type()\n> > +\t\t\t\t << \"' not supported\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n>\n> Another option would be to skip unsupported control types when\n> enumerating them, so the getControlInfo() call would return a nullptr.\n>\n\nThat too. I could call validateType there and remove the checks from\nget and set.\n\nThanks\n j\n\n> > +\n> > } /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8E65560B19\n\tfor ;\n\tThu, 20 Jun 2019 12:10:34 +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 relay12.mail.gandi.net (Postfix) with ESMTPSA id 29307200003;\n\tThu, 20 Jun 2019 10:10:33 +0000 (UTC)"],"Date":"Thu, 20 Jun 2019 12:11:48 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190620101119.xbxfv4nsxeisyy7j@uno.localdomain>","References":"<20190619110858.20980-1-jacopo@jmondi.org>\n\t<20190619110858.20980-4-jacopo@jmondi.org>\n\t<20190619203655.GN21753@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"a56tm7fpgoxdexro\"","Content-Disposition":"inline","In-Reply-To":"<20190619203655.GN21753@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 3/6] libcamera: v4l2_device:\n\tImplement get and set controls","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":"Thu, 20 Jun 2019 10:10:34 -0000"}},{"id":1960,"web_url":"https://patchwork.libcamera.org/comment/1960/","msgid":"<20190620105550.GC5720@pendragon.ideasonboard.com>","date":"2019-06-20T10:55:50","subject":"Re: [libcamera-devel] [PATCH v4 3/6] libcamera: v4l2_device:\n\tImplement get and set controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Jun 20, 2019 at 12:11:48PM +0200, Jacopo Mondi wrote:\n> On Wed, Jun 19, 2019 at 11:36:55PM +0300, Laurent Pinchart wrote:\n> > On Wed, Jun 19, 2019 at 01:08:55PM +0200, Jacopo Mondi wrote:\n> > > Implement getControls() and setControls() operations in V4L2Device class.\n> > >\n> > > Both operations take a V4L2Controls instance and read or write the V4L2\n> > > controls on the V4L2 device.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/include/v4l2_device.h | 6 +\n> > > src/libcamera/v4l2_device.cpp | 189 ++++++++++++++++++++++++++++\n> > > 2 files changed, 195 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > > index 563be151c257..e66cc7ebe737 100644\n> > > --- a/src/libcamera/include/v4l2_device.h\n> > > +++ b/src/libcamera/include/v4l2_device.h\n> > > @@ -15,6 +15,7 @@\n> > > namespace libcamera {\n> > >\n> > > class V4L2ControlInfo;\n> > > +class V4L2Controls;\n> > >\n> > > class V4L2Device : protected Loggable\n> > > {\n> > > @@ -23,6 +24,10 @@ public:\n> > > \tbool isOpen() const { return fd_ != -1; }\n> > > \tconst std::string &deviceNode() const { return deviceNode_; }\n> > >\n> > > +\tV4L2ControlInfo *getControlInfo(unsigned int id);\n> >\n> > Should this be moved to the previous patch ?\n> \n> Why so, it is only used here...\n\nBecause control info got implemented in the previous patch, and the\ncommit message of this one only mentions get and set controls.\n\n> > The returned pointer should be const, and the function should be const\n> > too.\n> \n> Indeed. Fixed.\n> \n> > > +\tint getControls(V4L2Controls *ctrls);\n> > > +\tint setControls(V4L2Controls *ctrls);\n> > > +\n> > > protected:\n> > > \tV4L2Device(const std::string &deviceNode, const std::string &logTag);\n> > > \t~V4L2Device();\n> > > @@ -37,6 +42,7 @@ protected:\n> > >\n> > > private:\n> > > \tvoid listControls();\n> > > +\tint validateControlType(V4L2ControlInfo *info);\n> > >\n> > > \tstd::map controls_;\n> > > \tstd::string deviceNode_;\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index 58dd94836686..5af0ddc468fe 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -66,6 +66,170 @@ void V4L2Device::close()\n> > > * \\return The device node path\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Get informations for control with \\a id\n> >\n> > \"Retrieve information about a control\" ?\n> >\n> > > + * \\param[in] id The control id to search info for\n> >\n> > \"The control ID\" should be enough.\n> >\n> > > + * \\return The V4L2ControlInfo associated to the V4L2 control with \\a id or\n> > > + * nullptr if the control is not supported by the V4L2Device\n> >\n> > \\return A pointer to the V4L2ControlInfo for control \\a id, or nullptr\n> > if the device doesn't have such a control\n> >\n> > ?\n> \n> Taken in.\n> \n> > > + */\n> > > +V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id)\n> > > +{\n> > > +\tauto it = controls_.find(id);\n> > > +\tif (it == controls_.end())\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\treturn it->second;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Read values of a list of controls from the device\n> >\n> > \"Read controls from the device\"\n> >\n> > > + * \\param[inout] ctrls The list of controls to read\n> > > + *\n> > > + * Read the value of each V4L2Controls contained in \\a ctrls, overwriting\n> > > + * it's current value with the one returned by the device.\n> >\n> > It's hard to write precise documentation without making it difficult to\n> > read :-/ How about\n> >\n> > \"Read the value of each control contained in \\a ctrls, and store the\n> > value in the corresponding \\a ctrls entry.\"\n> >\n> > > + *\n> > > + * Each V4L2Control instance in \\a ctrls should be supported by the device.\n> >\n> > \"If one control in \\a ctrls is not supported by the device this method\n> > returns an error. In that case the values stored in \\a ctrls are\n> > undefined.\"\n> >\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int V4L2Device::getControls(V4L2Controls *ctrls)\n> > > +{\n> > > +\tunsigned int count = ctrls->size();\n> > > +\tif (count == 0)\n> > > +\t\treturn 0;\n> > > +\n> > > +\tstruct V4L2ControlInfo *controlInfo[count];\n> > > +\tstruct v4l2_ext_control v4l2Ctrls[count];\n> > > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > > +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> > > +\n> > > +\t\t/* Validate the control. */\n> > > +\t\tV4L2ControlInfo *info = getControlInfo(ctrl->id());\n> > > +\t\tif (!info) {\n> > > +\t\t\tLOG(V4L2, Error) << \"Control '\" << ctrl->id()\n> > > +\t\t\t\t\t << \"' not valid\";\n> >\n> > s/not valid/not found/\n> >\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (validateControlType(info))\n> > > +\t\t\treturn -EINVAL;\n> > > +\n> > > +\t\tcontrolInfo[i] = info;\n> > > +\n> > > +\t\t/* Prepare the v4l2_ext_control entry for the read operation. */\n> > > +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> > > +\t\tv4l2Ctrl->id = info->id();\n> > > +\t\tv4l2Ctrl->size = info->size();\n> >\n> > size is only needed for controls with payloads, which we don't support.\n> > I would leave it set to 0, otherwise if we try to get a payload control\n> > by mistake, and the value happens to be a valid pointer to our memory\n> > address space, the kernel could overwrite that.\n> >\n> > I would then also remove the v4l2Ctrl variable and write\n> >\n> > \t\tv4l2Ctrls[i].id = ctrl->id();\n> \n> Ack!\n> \n> > > +\t}\n> > > +\n> > > +\tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> > > +\tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > > +\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> > > +\tv4l2ExtCtrls.count = count;\n> > > +\n> > > +\tint ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n> > > +\tif (ret) {\n> > > +\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> > > +\t\t\t\t << strerror(ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * For each control read from the device, set the value in the\n> > > +\t * V4L2ControlValue provided by the caller.\n> >\n> > That's called V4L2Control now, not V4L2ControlValue.\n> >\n> > > +\t */\n> > > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > > +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> > > +\t\tV4L2ControlInfo *info = controlInfo[i];\n> > > +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> > > +\n> > > +\t\tswitch (info->type()) {\n> > > +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > > +\t\t\tctrl->setValue(v4l2Ctrl->value64);\n> > > +\t\t\tbreak;\n> > > +\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> > > +\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > > +\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> > > +\t\tcase V4L2_CTRL_TYPE_MENU:\n> > > +\t\t\tctrl->setValue(static_cast(v4l2Ctrl->value));\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Write a list of controls to the device\n> >\n> > \"Write controls to the device\"\n> >\n> > > + * \\param[in] ctrls The list of controls to apply\n> >\n> > s/apply/write/\n> >\n> > > + *\n> > > + * Write the value of each V4L2Control contained in \\a ctrls. Each\n> > > + * control should be initialized by the caller with a value, otherwise\n> > > + * the result of the operation is undefined.\n> > > + *\n> > > + * The value of each control is not updated to reflect what has been\n> > > + * actually applied on the device. To read the actual value of a control\n> > > + * after a setControls(), users should read the control value back with\n> > > + * getControls().\n> >\n> > Why is that ? It would require another potentially expensive operation.\n> \n> This was actually something I was not sure of. Should we update the\n> value after the control is written to HW? So every write is equivalent\n> to a write+read?\n\nI would do so, as the kernel API offers that feature, and it could be\nuseful. The update itself will be cheap compared to another\ngetControls() call.\n\n> > > + *\n> > > + * Each V4L2Control instance in \\a ctrls should be supported by the device.\n> >\n> > \"If one control in \\a ctrls is not supported by the device this method\n> > returns an error.\" to copy the text from getControls() ?\n> >\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> >\n> > I wonder if we should return the number of controls successfully\n> > written. This would be 0 if the pre-validation fails, and we could use\n> > v4l2_ext_controls::error_idx to get the index of the control that failed\n> > (note that if error_idx == count then no control have been written).\n> \n> I generally prefer to have 0 for success, but in this case signaling\n> the error back is quite tricky, also because the extended ctrls API\n> does not help here... Returning the number of written controls might\n> be a good idea. Should we do the same on read?\n\nWe could do the same on read for consistency, yes.\n\n> > > + */\n> > > +int V4L2Device::setControls(V4L2Controls *ctrls)\n> > > +{\n> > > +\tunsigned int count = ctrls->size();\n> > > +\tif (count == 0)\n> > > +\t\treturn 0;\n> > > +\n> > > +\tstruct v4l2_ext_control v4l2Ctrls[count];\n> > > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > > +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> > > +\n> > > +\t\t/* Validate the control. */\n> > > +\t\tV4L2ControlInfo *info = getControlInfo(ctrl->id());\n> > > +\t\tif (!info) {\n> > > +\t\t\tLOG(V4L2, Error) << \"Control '\" << ctrl->id()\n> > > +\t\t\t\t\t << \"' not valid\";\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (validateControlType(info))\n> > > +\t\t\treturn -EINVAL;\n> > > +\n> > > +\t\t/* Prepare the v4l2_ext_control entry for the write operation. */\n> > > +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> > > +\t\tv4l2Ctrl->id = info->id();\n> > > +\t\tv4l2Ctrl->size = info->size();\n> > > +\n> > > +\t\tswitch (info->type()) {\n> > > +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > > +\t\t\tv4l2Ctrl->value64 = ctrl->value();\n> > > +\t\t\tbreak;\n> > > +\t\tcase V4L2_CTRL_TYPE_INTEGER:\n> > > +\t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > > +\t\tcase V4L2_CTRL_TYPE_BUTTON:\n> > > +\t\tcase V4L2_CTRL_TYPE_MENU:\n> > > +\t\t\tv4l2Ctrl->value = static_cast(ctrl->value());\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> >\n> > This switch duplicates the one in validateControlType(). I would just\n> > add a default case here and return an error, removing the call to\n> > validateControlType(). The method would be left iwht a single caller, so\n> > I would then inline it in getControls().\n> \n> Ah yes, the switch is repeated. I can fail in the second one yes.\n> \n> > > +\t}\n> > > +\n> > > +\tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> > > +\tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > > +\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> > > +\tv4l2ExtCtrls.count = count;\n> > > +\n> > > +\tint ret = ioctl(VIDIOC_S_EXT_CTRLS, &v4l2ExtCtrls);\n> > > +\tif (ret) {\n> > > +\t\tLOG(V4L2, Error) << \"Unable to write controls: \"\n> > > +\t\t\t\t << strerror(ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Construct a V4L2Device\n> > > * \\param[in] deviceNode The device node filesystem path\n> > > @@ -171,4 +335,29 @@ void V4L2Device::listControls()\n> > > \t}\n> > > }\n> > >\n> > > +/*\n> > > + * \\brief Make sure the control type is supported\n> > > + * \\param[in] info The V4L2ControlInfo to inspect type of\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + * \\retval -EINVAL The control type is not supported\n> > > + */\n> > > +int V4L2Device::validateControlType(V4L2ControlInfo *info)\n> > > +{\n> > > +\t/* \\todo support compound and menu controls. */\n> > > +\tswitch (info->type()) {\n> > > +\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > > +\tcase V4L2_CTRL_TYPE_INTEGER:\n> > > +\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > > +\tcase V4L2_CTRL_TYPE_BUTTON:\n> > > +\tcase V4L2_CTRL_TYPE_MENU:\n> > > +\t\tbreak;\n> > > +\tdefault:\n> > > +\t\tLOG(V4L2, Error) << \"Control type '\" << info->type()\n> > > +\t\t\t\t << \"' not supported\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> >\n> > Another option would be to skip unsupported control types when\n> > enumerating them, so the getControlInfo() call would return a nullptr.\n> \n> That too. I could call validateType there and remove the checks from\n> get and set.\n> \n> > > +\n> > > } /* namespace libcamera */","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 81C4360B19\n\tfor ;\n\tThu, 20 Jun 2019 12:56:10 +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 CBFC552A;\n\tThu, 20 Jun 2019 12:56:09 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561028169;\n\tbh=E/u9oenPHWoYaggAv+8EhdD0iYDgcL6U9PpurliNOSc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m4f7JLIVdaNkna6U0Mn/i06kqv5N9ngPE6AsaguO4VAEEWl95KoB2R1yWcLHzTNG8\n\t9PWKPJKTb7alde3OQz9W+rv/8RjUx3NnSSRt00ZWs4F37vBxBh9Sc2g+e2Lzwttleo\n\t79cywNFUt1k3bJlpAh14Mtobwnaz5Rc7MvyB4Ww0=","Date":"Thu, 20 Jun 2019 13:55:50 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190620105550.GC5720@pendragon.ideasonboard.com>","References":"<20190619110858.20980-1-jacopo@jmondi.org>\n\t<20190619110858.20980-4-jacopo@jmondi.org>\n\t<20190619203655.GN21753@pendragon.ideasonboard.com>\n\t<20190620101119.xbxfv4nsxeisyy7j@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190620101119.xbxfv4nsxeisyy7j@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 3/6] libcamera: v4l2_device:\n\tImplement get and set controls","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":"Thu, 20 Jun 2019 10:56:10 -0000"}}]