| Message ID | 20190121172705.19985-3-jacopo@jmondi.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Mon, Jan 21, 2019 at 06:27:01PM +0100, Jacopo Mondi wrote: > Add function to set controls on a V4L2Device object. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/v4l2_device.h | 2 ++ > src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++++++++++++++ > 2 files changed, 40 insertions(+) > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index c6f3d9a..2787847 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -51,6 +51,8 @@ public: > const char *deviceName() const { return caps_.card(); } > const char *busName() const { return caps_.bus_info(); } > > + int setControl(unsigned int control, int value); > + I think this is needed, but requires a little more thoughts to define a good API. To start with, not all controls have integer values, we also have string controls. Furthermore, we will also likely need to use the extended controls API to set multiple controls at a time, requiring a vector to be passed to the function. Here's a very preliminary proposal. class V4L2ControlValue { enum Type { /* The control types. */ }; /* * This should store the type and value, with appropriate * constructors and accessors. / }; int setControl(unsigned int id, const V4L2ControlValue &control); int setControls(const std::vector<std::pair<unsigned int, V4L2ControlValue>> &controls); setControl() could be implemented by calling setControls(). setControls() should use the extended controls API, and possibly fall back to the old controls API if the extended API isn't available, although I think we can depend on the extended API in libcamera. We could also create a class that groups together the id and value, possibly named V4L2Control, but I have a feeling we will later need to enumerate controls and store the data queried from the kernel, and V4L2Control would be a good class name for that. > private: > std::string devnode_; > int fd_; > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 59a1ad9..aef0996 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -195,4 +195,42 @@ void V4L2Device::close() > * \return The string containing the device location > */ > > +/** > + * \brief Set a control value > + * \param control The control identifier > + * \param value The new control value > + * > + * Set a user control to the requested value. The function returns the > + * new value actually assigned to the control, which might be different > + * from the requested \a value one. > + * > + * \return The new control's value for success, or a negative error code otherwise > + */ > +int V4L2Device::setControl(unsigned int control, int value) > +{ > + struct v4l2_control v4l2_ctrl = { > + .id = control, > + .value = value, > + }; > + > + int ret = ioctl(fd_, VIDIOC_S_CTRL, &v4l2_ctrl); > + if (ret) { > + ret = -errno; > + LOG(Error) << "Failed to set control: " << strerror(-ret); > + return ret; > + } > + > + v4l2_ctrl = { }; > + v4l2_ctrl.id = control; > + > + ret = ioctl(fd_, VIDIOC_G_CTRL, &v4l2_ctrl); > + if (ret) { > + ret = -errno; > + LOG(Error) << "Failed to get control: " << strerror(-ret); > + return ret; > + } > + > + return v4l2_ctrl.value; > +} > + > } /* namespace libcamera */
Hi Laurent, On Mon, Jan 21, 2019 at 10:42:28PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. You know, I had this patch around and I will soon need that to set a single int control on IPU3. For the moment, I would ditch this one and do it right later. I'll take your suggestions into account. Thanks j > > On Mon, Jan 21, 2019 at 06:27:01PM +0100, Jacopo Mondi wrote: > > Add function to set controls on a V4L2Device object. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/include/v4l2_device.h | 2 ++ > > src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++++++++++++++ > > 2 files changed, 40 insertions(+) > > > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > > index c6f3d9a..2787847 100644 > > --- a/src/libcamera/include/v4l2_device.h > > +++ b/src/libcamera/include/v4l2_device.h > > @@ -51,6 +51,8 @@ public: > > const char *deviceName() const { return caps_.card(); } > > const char *busName() const { return caps_.bus_info(); } > > > > + int setControl(unsigned int control, int value); > > + > > I think this is needed, but requires a little more thoughts to define a > good API. To start with, not all controls have integer values, we also > have string controls. Furthermore, we will also likely need to use the > extended controls API to set multiple controls at a time, requiring a > vector to be passed to the function. > > Here's a very preliminary proposal. > > class V4L2ControlValue > { > enum Type { > /* The control types. */ > }; > > /* > * This should store the type and value, with appropriate > * constructors and accessors. > / > }; > > int setControl(unsigned int id, const V4L2ControlValue &control); > int setControls(const std::vector<std::pair<unsigned int, V4L2ControlValue>> &controls); > > setControl() could be implemented by calling setControls(). > setControls() should use the extended controls API, and possibly fall > back to the old controls API if the extended API isn't available, > although I think we can depend on the extended API in libcamera. > > We could also create a class that groups together the id and value, > possibly named V4L2Control, but I have a feeling we will later need to > enumerate controls and store the data queried from the kernel, and > V4L2Control would be a good class name for that. > > > private: > > std::string devnode_; > > int fd_; > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 59a1ad9..aef0996 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -195,4 +195,42 @@ void V4L2Device::close() > > * \return The string containing the device location > > */ > > > > +/** > > + * \brief Set a control value > > + * \param control The control identifier > > + * \param value The new control value > > + * > > + * Set a user control to the requested value. The function returns the > > + * new value actually assigned to the control, which might be different > > + * from the requested \a value one. > > + * > > + * \return The new control's value for success, or a negative error code otherwise > > + */ > > +int V4L2Device::setControl(unsigned int control, int value) > > +{ > > + struct v4l2_control v4l2_ctrl = { > > + .id = control, > > + .value = value, > > + }; > > + > > + int ret = ioctl(fd_, VIDIOC_S_CTRL, &v4l2_ctrl); > > + if (ret) { > > + ret = -errno; > > + LOG(Error) << "Failed to set control: " << strerror(-ret); > > + return ret; > > + } > > + > > + v4l2_ctrl = { }; > > + v4l2_ctrl.id = control; > > + > > + ret = ioctl(fd_, VIDIOC_G_CTRL, &v4l2_ctrl); > > + if (ret) { > > + ret = -errno; > > + LOG(Error) << "Failed to get control: " << strerror(-ret); > > + return ret; > > + } > > + > > + return v4l2_ctrl.value; > > +} > > + > > } /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Tue, Jan 22, 2019 at 11:41:05AM +0100, Jacopo Mondi wrote: > On Mon, Jan 21, 2019 at 10:42:28PM +0200, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > You know, I had this patch around and I will soon need that to set a > single int control on IPU3. For the moment, I would ditch this one and > do it right later. > > I'll take your suggestions into account. Thank you. For the single control you have to set, I think it would make sense for V4L2Device to expose an ioctl() method, as I expect pipeline handlers to need private ioctls. You can then call VIDIOC_S_CTRL directly in the IPU3 pipeline handler, and replace that with the control API when we'll have it. Or we would also merge this patch in the meantime, with a \todo comment to explain it will be replaced by a better API. > > On Mon, Jan 21, 2019 at 06:27:01PM +0100, Jacopo Mondi wrote: > >> Add function to set controls on a V4L2Device object. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/include/v4l2_device.h | 2 ++ > >> src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++++++++++++++ > >> 2 files changed, 40 insertions(+) > >> > >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > >> index c6f3d9a..2787847 100644 > >> --- a/src/libcamera/include/v4l2_device.h > >> +++ b/src/libcamera/include/v4l2_device.h > >> @@ -51,6 +51,8 @@ public: > >> const char *deviceName() const { return caps_.card(); } > >> const char *busName() const { return caps_.bus_info(); } > >> > >> + int setControl(unsigned int control, int value); > >> + > > > > I think this is needed, but requires a little more thoughts to define a > > good API. To start with, not all controls have integer values, we also > > have string controls. Furthermore, we will also likely need to use the > > extended controls API to set multiple controls at a time, requiring a > > vector to be passed to the function. > > > > Here's a very preliminary proposal. > > > > class V4L2ControlValue > > { > > enum Type { > > /* The control types. */ > > }; > > > > /* > > * This should store the type and value, with appropriate > > * constructors and accessors. > > / > > }; > > > > int setControl(unsigned int id, const V4L2ControlValue &control); > > int setControls(const std::vector<std::pair<unsigned int, V4L2ControlValue>> &controls); > > > > setControl() could be implemented by calling setControls(). > > setControls() should use the extended controls API, and possibly fall > > back to the old controls API if the extended API isn't available, > > although I think we can depend on the extended API in libcamera. > > > > We could also create a class that groups together the id and value, > > possibly named V4L2Control, but I have a feeling we will later need to > > enumerate controls and store the data queried from the kernel, and > > V4L2Control would be a good class name for that. > > > >> private: > >> std::string devnode_; > >> int fd_; > >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > >> index 59a1ad9..aef0996 100644 > >> --- a/src/libcamera/v4l2_device.cpp > >> +++ b/src/libcamera/v4l2_device.cpp > >> @@ -195,4 +195,42 @@ void V4L2Device::close() > >> * \return The string containing the device location > >> */ > >> > >> +/** > >> + * \brief Set a control value > >> + * \param control The control identifier > >> + * \param value The new control value > >> + * > >> + * Set a user control to the requested value. The function returns the > >> + * new value actually assigned to the control, which might be different > >> + * from the requested \a value one. > >> + * > >> + * \return The new control's value for success, or a negative error code otherwise > >> + */ > >> +int V4L2Device::setControl(unsigned int control, int value) > >> +{ > >> + struct v4l2_control v4l2_ctrl = { > >> + .id = control, > >> + .value = value, > >> + }; > >> + > >> + int ret = ioctl(fd_, VIDIOC_S_CTRL, &v4l2_ctrl); > >> + if (ret) { > >> + ret = -errno; > >> + LOG(Error) << "Failed to set control: " << strerror(-ret); > >> + return ret; > >> + } > >> + > >> + v4l2_ctrl = { }; > >> + v4l2_ctrl.id = control; > >> + > >> + ret = ioctl(fd_, VIDIOC_G_CTRL, &v4l2_ctrl); > >> + if (ret) { > >> + ret = -errno; > >> + LOG(Error) << "Failed to get control: " << strerror(-ret); > >> + return ret; > >> + } > >> + > >> + return v4l2_ctrl.value; > >> +} > >> + > >> } /* namespace libcamera */
diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h index c6f3d9a..2787847 100644 --- a/src/libcamera/include/v4l2_device.h +++ b/src/libcamera/include/v4l2_device.h @@ -51,6 +51,8 @@ public: const char *deviceName() const { return caps_.card(); } const char *busName() const { return caps_.bus_info(); } + int setControl(unsigned int control, int value); + private: std::string devnode_; int fd_; diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 59a1ad9..aef0996 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -195,4 +195,42 @@ void V4L2Device::close() * \return The string containing the device location */ +/** + * \brief Set a control value + * \param control The control identifier + * \param value The new control value + * + * Set a user control to the requested value. The function returns the + * new value actually assigned to the control, which might be different + * from the requested \a value one. + * + * \return The new control's value for success, or a negative error code otherwise + */ +int V4L2Device::setControl(unsigned int control, int value) +{ + struct v4l2_control v4l2_ctrl = { + .id = control, + .value = value, + }; + + int ret = ioctl(fd_, VIDIOC_S_CTRL, &v4l2_ctrl); + if (ret) { + ret = -errno; + LOG(Error) << "Failed to set control: " << strerror(-ret); + return ret; + } + + v4l2_ctrl = { }; + v4l2_ctrl.id = control; + + ret = ioctl(fd_, VIDIOC_G_CTRL, &v4l2_ctrl); + if (ret) { + ret = -errno; + LOG(Error) << "Failed to get control: " << strerror(-ret); + return ret; + } + + return v4l2_ctrl.value; +} + } /* namespace libcamera */
Add function to set controls on a V4L2Device object. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/v4l2_device.h | 2 ++ src/libcamera/v4l2_device.cpp | 38 +++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+)