[{"id":462,"web_url":"https://patchwork.libcamera.org/comment/462/","msgid":"<20190121204228.GF4439@pendragon.ideasonboard.com>","date":"2019-01-21T20:42:28","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: v4l2_device: Add\n\tsetControl function","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, Jan 21, 2019 at 06:27:01PM +0100, Jacopo Mondi wrote:\n> Add function to set controls on a V4L2Device object.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/v4l2_device.h | 2 ++\n> src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++++++++++++++\n> 2 files changed, 40 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index c6f3d9a..2787847 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -51,6 +51,8 @@ public:\n> \tconst char *deviceName() const { return caps_.card(); }\n> \tconst char *busName() const { return caps_.bus_info(); }\n> \n> +\tint setControl(unsigned int control, int value);\n> +\n\nI think this is needed, but requires a little more thoughts to define a\ngood API. To start with, not all controls have integer values, we also\nhave string controls. Furthermore, we will also likely need to use the\nextended controls API to set multiple controls at a time, requiring a\nvector to be passed to the function.\n\nHere's a very preliminary proposal.\n\nclass V4L2ControlValue\n{\n\tenum Type {\n\t\t/* The control types. */\n\t};\n\n\t/*\n\t * This should store the type and value, with appropriate\n\t * constructors and accessors.\n\t /\n};\n\n\tint setControl(unsigned int id, const V4L2ControlValue &control);\n\tint setControls(const std::vector> &controls);\n\nsetControl() could be implemented by calling setControls().\nsetControls() should use the extended controls API, and possibly fall\nback to the old controls API if the extended API isn't available,\nalthough I think we can depend on the extended API in libcamera.\n\nWe could also create a class that groups together the id and value,\npossibly named V4L2Control, but I have a feeling we will later need to\nenumerate controls and store the data queried from the kernel, and\nV4L2Control would be a good class name for that.\n\n> private:\n> \tstd::string devnode_;\n> \tint fd_;\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 59a1ad9..aef0996 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -195,4 +195,42 @@ void V4L2Device::close()\n> * \\return The string containing the device location\n> */\n> \n> +/**\n> + * \\brief Set a control value\n> + * \\param control The control identifier\n> + * \\param value The new control value\n> + *\n> + * Set a user control to the requested value. The function returns the\n> + * new value actually assigned to the control, which might be different\n> + * from the requested \\a value one.\n> + *\n> + * \\return The new control's value for success, or a negative error code otherwise\n> + */\n> +int V4L2Device::setControl(unsigned int control, int value)\n> +{\n> +\tstruct v4l2_control v4l2_ctrl = {\n> +\t\t.id = control,\n> +\t\t.value = value,\n> +\t};\n> +\n> +\tint ret = ioctl(fd_, VIDIOC_S_CTRL, &v4l2_ctrl);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Failed to set control: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tv4l2_ctrl = { };\n> +\tv4l2_ctrl.id = control;\n> +\n> +\tret = ioctl(fd_, VIDIOC_G_CTRL, &v4l2_ctrl);\n> +\tif (ret) {\n> +\t\tret = -errno;\n> +\t\tLOG(Error) << \"Failed to get control: \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn v4l2_ctrl.value;\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 8E5BC60B1D\n\tfor ;\n\tMon, 21 Jan 2019 21:42:29 +0100 (CET)","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 0FE0B53E;\n\tMon, 21 Jan 2019 21:42:28 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548103349;\n\tbh=DzRgxgiCzdUSBbl+/DUNyHluh8G/AshtMzEKS5WiPZI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OPnPm/2xODy+tgkIWuG+D2v1RzSMyNZNa7hpjPigTIF1fEoXEtTT70neSwgfyIh1C\n\ts53ktmNx88mx8SaUUyKILbfWgh+J78s9id787Q53pA/b88wj+c1Z3EwZQtGsFIe0x/\n\t9L5raf2S6hc8GIoJ4MioQJYQJt7cXPRAI1F0lVRI=","Date":"Mon, 21 Jan 2019 22:42:28 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190121204228.GF4439@pendragon.ideasonboard.com>","References":"<20190121172705.19985-1-jacopo@jmondi.org>\n\t<20190121172705.19985-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190121172705.19985-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: v4l2_device: Add\n\tsetControl function","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":"Mon, 21 Jan 2019 20:42:29 -0000"}},{"id":475,"web_url":"https://patchwork.libcamera.org/comment/475/","msgid":"<20190122104105.2ujy4j7rdqjfoaur@uno.localdomain>","date":"2019-01-22T10:41:05","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: v4l2_device: Add\n\tsetControl function","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jan 21, 2019 at 10:42:28PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n\nYou know, I had this patch around and I will soon need that to set a\nsingle int control on IPU3. For the moment, I would ditch this one and\ndo it right later.\n\nI'll take your suggestions into account.\n\nThanks\n j\n\n>\n> On Mon, Jan 21, 2019 at 06:27:01PM +0100, Jacopo Mondi wrote:\n> > Add function to set controls on a V4L2Device object.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/v4l2_device.h | 2 ++\n> > src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++++++++++++++\n> > 2 files changed, 40 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > index c6f3d9a..2787847 100644\n> > --- a/src/libcamera/include/v4l2_device.h\n> > +++ b/src/libcamera/include/v4l2_device.h\n> > @@ -51,6 +51,8 @@ public:\n> > \tconst char *deviceName() const { return caps_.card(); }\n> > \tconst char *busName() const { return caps_.bus_info(); }\n> >\n> > +\tint setControl(unsigned int control, int value);\n> > +\n>\n> I think this is needed, but requires a little more thoughts to define a\n> good API. To start with, not all controls have integer values, we also\n> have string controls. Furthermore, we will also likely need to use the\n> extended controls API to set multiple controls at a time, requiring a\n> vector to be passed to the function.\n>\n> Here's a very preliminary proposal.\n>\n> class V4L2ControlValue\n> {\n> \tenum Type {\n> \t\t/* The control types. */\n> \t};\n>\n> \t/*\n> \t * This should store the type and value, with appropriate\n> \t * constructors and accessors.\n> \t /\n> };\n>\n> \tint setControl(unsigned int id, const V4L2ControlValue &control);\n> \tint setControls(const std::vector> &controls);\n>\n> setControl() could be implemented by calling setControls().\n> setControls() should use the extended controls API, and possibly fall\n> back to the old controls API if the extended API isn't available,\n> although I think we can depend on the extended API in libcamera.\n>\n> We could also create a class that groups together the id and value,\n> possibly named V4L2Control, but I have a feeling we will later need to\n> enumerate controls and store the data queried from the kernel, and\n> V4L2Control would be a good class name for that.\n>\n> > private:\n> > \tstd::string devnode_;\n> > \tint fd_;\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 59a1ad9..aef0996 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -195,4 +195,42 @@ void V4L2Device::close()\n> > * \\return The string containing the device location\n> > */\n> >\n> > +/**\n> > + * \\brief Set a control value\n> > + * \\param control The control identifier\n> > + * \\param value The new control value\n> > + *\n> > + * Set a user control to the requested value. The function returns the\n> > + * new value actually assigned to the control, which might be different\n> > + * from the requested \\a value one.\n> > + *\n> > + * \\return The new control's value for success, or a negative error code otherwise\n> > + */\n> > +int V4L2Device::setControl(unsigned int control, int value)\n> > +{\n> > +\tstruct v4l2_control v4l2_ctrl = {\n> > +\t\t.id = control,\n> > +\t\t.value = value,\n> > +\t};\n> > +\n> > +\tint ret = ioctl(fd_, VIDIOC_S_CTRL, &v4l2_ctrl);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Failed to set control: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tv4l2_ctrl = { };\n> > +\tv4l2_ctrl.id = control;\n> > +\n> > +\tret = ioctl(fd_, VIDIOC_G_CTRL, &v4l2_ctrl);\n> > +\tif (ret) {\n> > +\t\tret = -errno;\n> > +\t\tLOG(Error) << \"Failed to get control: \" << strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn v4l2_ctrl.value;\n> > +}\n> > +\n> > } /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3BAF960C80\n\tfor ;\n\tTue, 22 Jan 2019 11:40:53 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 9F361FF812;\n\tTue, 22 Jan 2019 10:40:52 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 22 Jan 2019 11:41:05 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190122104105.2ujy4j7rdqjfoaur@uno.localdomain>","References":"<20190121172705.19985-1-jacopo@jmondi.org>\n\t<20190121172705.19985-3-jacopo@jmondi.org>\n\t<20190121204228.GF4439@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"tq2zf3iefswzdzbm\"","Content-Disposition":"inline","In-Reply-To":"<20190121204228.GF4439@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: v4l2_device: Add\n\tsetControl function","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, 22 Jan 2019 10:40:53 -0000"}},{"id":488,"web_url":"https://patchwork.libcamera.org/comment/488/","msgid":"<20190122131012.GC4455@pendragon.ideasonboard.com>","date":"2019-01-22T13:10:12","subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: v4l2_device: Add\n\tsetControl function","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, Jan 22, 2019 at 11:41:05AM +0100, Jacopo Mondi wrote:\n> On Mon, Jan 21, 2019 at 10:42:28PM +0200, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> \n> You know, I had this patch around and I will soon need that to set a\n> single int control on IPU3. For the moment, I would ditch this one and\n> do it right later.\n> \n> I'll take your suggestions into account.\n\nThank you.\n\nFor the single control you have to set, I think it would make sense for\nV4L2Device to expose an ioctl() method, as I expect pipeline handlers to\nneed private ioctls. You can then call VIDIOC_S_CTRL directly in the\nIPU3 pipeline handler, and replace that with the control API when we'll\nhave it. Or we would also merge this patch in the meantime, with a \\todo\ncomment to explain it will be replaced by a better API.\n\n> > On Mon, Jan 21, 2019 at 06:27:01PM +0100, Jacopo Mondi wrote:\n> >> Add function to set controls on a V4L2Device object.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> src/libcamera/include/v4l2_device.h | 2 ++\n> >> src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++++++++++++++\n> >> 2 files changed, 40 insertions(+)\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> >> index c6f3d9a..2787847 100644\n> >> --- a/src/libcamera/include/v4l2_device.h\n> >> +++ b/src/libcamera/include/v4l2_device.h\n> >> @@ -51,6 +51,8 @@ public:\n> >> \tconst char *deviceName() const { return caps_.card(); }\n> >> \tconst char *busName() const { return caps_.bus_info(); }\n> >>\n> >> +\tint setControl(unsigned int control, int value);\n> >> +\n> >\n> > I think this is needed, but requires a little more thoughts to define a\n> > good API. To start with, not all controls have integer values, we also\n> > have string controls. Furthermore, we will also likely need to use the\n> > extended controls API to set multiple controls at a time, requiring a\n> > vector to be passed to the function.\n> >\n> > Here's a very preliminary proposal.\n> >\n> > class V4L2ControlValue\n> > {\n> > \tenum Type {\n> > \t\t/* The control types. */\n> > \t};\n> >\n> > \t/*\n> > \t * This should store the type and value, with appropriate\n> > \t * constructors and accessors.\n> > \t /\n> > };\n> >\n> > \tint setControl(unsigned int id, const V4L2ControlValue &control);\n> > \tint setControls(const std::vector> &controls);\n> >\n> > setControl() could be implemented by calling setControls().\n> > setControls() should use the extended controls API, and possibly fall\n> > back to the old controls API if the extended API isn't available,\n> > although I think we can depend on the extended API in libcamera.\n> >\n> > We could also create a class that groups together the id and value,\n> > possibly named V4L2Control, but I have a feeling we will later need to\n> > enumerate controls and store the data queried from the kernel, and\n> > V4L2Control would be a good class name for that.\n> >\n> >> private:\n> >> \tstd::string devnode_;\n> >> \tint fd_;\n> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >> index 59a1ad9..aef0996 100644\n> >> --- a/src/libcamera/v4l2_device.cpp\n> >> +++ b/src/libcamera/v4l2_device.cpp\n> >> @@ -195,4 +195,42 @@ void V4L2Device::close()\n> >> * \\return The string containing the device location\n> >> */\n> >>\n> >> +/**\n> >> + * \\brief Set a control value\n> >> + * \\param control The control identifier\n> >> + * \\param value The new control value\n> >> + *\n> >> + * Set a user control to the requested value. The function returns the\n> >> + * new value actually assigned to the control, which might be different\n> >> + * from the requested \\a value one.\n> >> + *\n> >> + * \\return The new control's value for success, or a negative error code otherwise\n> >> + */\n> >> +int V4L2Device::setControl(unsigned int control, int value)\n> >> +{\n> >> +\tstruct v4l2_control v4l2_ctrl = {\n> >> +\t\t.id = control,\n> >> +\t\t.value = value,\n> >> +\t};\n> >> +\n> >> +\tint ret = ioctl(fd_, VIDIOC_S_CTRL, &v4l2_ctrl);\n> >> +\tif (ret) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(Error) << \"Failed to set control: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\tv4l2_ctrl = { };\n> >> +\tv4l2_ctrl.id = control;\n> >> +\n> >> +\tret = ioctl(fd_, VIDIOC_G_CTRL, &v4l2_ctrl);\n> >> +\tif (ret) {\n> >> +\t\tret = -errno;\n> >> +\t\tLOG(Error) << \"Failed to get control: \" << strerror(-ret);\n> >> +\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\treturn v4l2_ctrl.value;\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 CDC9E60B21\n\tfor ;\n\tTue, 22 Jan 2019 14:12:24 +0100 (CET)","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 3136353E;\n\tTue, 22 Jan 2019 14:12:24 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548162744;\n\tbh=0+ytwQHi8BZeuz6yRFTM8uFUoH82vNH1fJcpaV/nq4Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rF5fL/MtRBw1LTwV5OMdbdi4zthnU+04Lb9UQ2wIjqP5kgaYxOuiev7GDag1FlBWL\n\tqV9g7Q07uPy+FnknaLS7bYhyhY/fFkQkFK79wAta2JvwuaBXtPdHGkmptB/P7izyS7\n\tMyXwVlQH5aSMSTCx5NrPOpQ8UB5n03xdYvfmdNTQ=","Date":"Tue, 22 Jan 2019 15:10:12 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190122131012.GC4455@pendragon.ideasonboard.com>","References":"<20190121172705.19985-1-jacopo@jmondi.org>\n\t<20190121172705.19985-3-jacopo@jmondi.org>\n\t<20190121204228.GF4439@pendragon.ideasonboard.com>\n\t<20190122104105.2ujy4j7rdqjfoaur@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190122104105.2ujy4j7rdqjfoaur@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/6] libcamera: v4l2_device: Add\n\tsetControl function","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, 22 Jan 2019 13:12:25 -0000"}}]