[libcamera-devel] [PATCH v5 3/6] libcamera: v4l2_device: Implement get and set controls
Jacopo Mondi
jacopo at jmondi.org
Mon Jun 24 10:22:16 CEST 2019
Hi Laurent,
On Sat, Jun 22, 2019 at 11:24:20PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
Thanks, I'll take comments on this patch in.
>
> On Thu, Jun 20, 2019 at 05:31:41PM +0200, Jacopo Mondi wrote:
> > Implement getControls() and setControls() operations in V4L2Device class.
> >
> > Both operations take a V4L2Controls instance and read or write the V4L2
> > controls on the V4L2 device.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/libcamera/include/v4l2_device.h | 3 +
> > src/libcamera/v4l2_device.cpp | 193 ++++++++++++++++++++++++++++
> > 2 files changed, 196 insertions(+)
> >
> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > index 91a72fcecbcc..3af27c9f7af5 100644
> > --- a/src/libcamera/include/v4l2_device.h
> > +++ b/src/libcamera/include/v4l2_device.h
> > @@ -15,6 +15,7 @@
> > namespace libcamera {
> >
> > class V4L2ControlInfo;
> > +class V4L2Controls;
> >
> > class V4L2Device : protected Loggable
> > {
> > @@ -23,6 +24,8 @@ public:
> > bool isOpen() const { return fd_ != -1; }
> >
> > const V4L2ControlInfo *getControlInfo(unsigned int id) const;
> > + int getControls(V4L2Controls *ctrls);
> > + int setControls(V4L2Controls *ctrls);
> >
> > const std::string &deviceNode() const { return deviceNode_; }
> >
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index b113ff77e3cf..efe30ef856ae 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -125,6 +125,199 @@ const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const
> > return &it->second;
> > }
> >
> > +/**
> > + * \brief Read controls from the device
> > + * \param[inout] ctrls The list of controls to read
> > + *
> > + * Read the value of each control contained in \a ctrls, and store the
> > + * value in the corresponding \a ctrls entry.
> > + *
> > + * If an integer number less than the requested number of controls is returned,
> > + * only controls up to that index have been successfully read.
> > + *
> > + * Each V4L2Control instance in \a ctrls should be supported by the device and
> > + * accessible (ie the V4L2_CTRL_FLAG_DISABLED flag should not be set), otherwise
> > + * a negative error code (-EINVAL) is returned
>
> * This method reads the value of all controls contained in \a ctrls, and stores
> * their values in the corresponding \a ctrls entry.
> *
> * If any control in \a ctrls is not supported by the device, is disabled (i.e.
> * has the V4L2_CTRL_FLAG_DISABLED flag set), is a compound control, or if any
> * other error occurs during validation of the requested controls, no control is
> * read and this method returns -EINVAL.
> *
> * If an error occurs while reading the controls, the index of the first
> * control that couldn't be read is returned. The value of all controls
> * below that index are updated in \a ctrls, while the value of all the
> * other controls are not changed.
>
> > + *
> > + * \return 0 on success or an error code otherwise
> > + * \retval -EINVAL One of the control is not supported or not accessible
> > + * \retval i The index of the control that failed
> > + */
> > +int V4L2Device::getControls(V4L2Controls *ctrls)
> > +{
> > + unsigned int count = ctrls->size();
> > + if (count == 0)
> > + return 0;
> > +
> > + const V4L2ControlInfo *controlInfo[count] = {};
> > + struct v4l2_ext_control v4l2Ctrls[count] = {};
> > +
> > + for (unsigned int i = 0; i < count; ++i) {
> > + V4L2Control *ctrl = (*ctrls)[i];
> > +
> > + const V4L2ControlInfo *info = getControlInfo(ctrl->id());
> > + if (!info) {
> > + LOG(V4L2, Error)
> > + << "Control '" << ctrl->id() << "' not found";
> > + return -EINVAL;
> > + }
> > +
> > + controlInfo[i] = info;
> > + v4l2Ctrls[i].id = info->id();
> > + }
> > +
> > + struct v4l2_ext_controls v4l2ExtCtrls = {};
> > + v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> > + v4l2ExtCtrls.controls = v4l2Ctrls;
> > + v4l2ExtCtrls.count = count;
> > +
> > + int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);
> > + if (ret) {
> > + unsigned int errorIdx = v4l2ExtCtrls.error_idx;
> > +
> > + /* Generic validation error. */
> > + if (errorIdx == count) {
> > + LOG(V4L2, Error) << "Unable to read controls: "
> > + << strerror(ret);
> > + return -EINVAL;
> > + }
> > +
> > + /* A specific control failed. */
> > + LOG(V4L2, Error) << "Unable to read control " << errorIdx
> > + << ": " << strerror(ret);
> > + return errorIdx;
>
> This contradicts with the documentation (OK, with my documentation only
> :-)), the values of all controls under the error index should be stored
> in ctrls. Alternatively we could modify the documentation and not update
> any value, but then would returning the error index be useful ? I would
> thus write this
>
> int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);
> if (ret) {
> {
> unsigned int errorIdx = v4l2ExtCtrls.error_idx;
>
> /* Generic validation error. */
> if (errorIdx == 0 || errorIdx >= count) {
> LOG(V4L2, Error) << "Unable to read controls: "
> << strerror(ret);
> return -EINVAL;
> }
>
> /* A specific control failed. */
> LOG(V4L2, Error) << "Unable to read control " << errorIdx
> << ": " << strerror(ret);
> count = errorIdx - 1;
> ret = errorIdx;
> }
>
> and return ret at the end of the function.
>
> > + }
> > +
> > + /*
> > + * For each control read from the device, set the value in the
> > + * V4L2Control provided by the caller.
> > + */
> > + for (unsigned int i = 0; i < count; ++i) {
> > + struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> > + const V4L2ControlInfo *info = controlInfo[i];
> > + V4L2Control *ctrl = (*ctrls)[i];
> > +
> > + switch (info->type()) {
> > + case V4L2_CTRL_TYPE_INTEGER64:
> > + ctrl->setValue(v4l2Ctrl->value64);
> > + break;
> > + default:
> > + /*
> > + * \todo To be changed when support for string and
> > + * compound controls will be added.
> > + */
> > + ctrl->setValue(static_cast<long int>(v4l2Ctrl->value));
>
> Do you need the cast ? If you do it should be cast to int64_t.
>
> > + break;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * \brief Write controls to the device
> > + * \param[in] ctrls The list of controls to write
> > + *
> > + * Write the value of each V4L2Control contained in \a ctrls. Each
> > + * control should be initialized by the caller with a value, otherwise
> > + * the result of the operation is undefined.
> > + *
> > + * The value of each control is updated to reflect what has been
> > + * actually applied on the device.
>
> No need to wrap lines well below the 80 characters limit :-)
>
> > + *
> > + * If an integer number less than the requested number of controls is returned,
> > + * only controls up to that index have been successfully applied.
> > + *
> > + * Each V4L2Control instance in \a ctrls should be supported by the device and
> > + * accessible (ie the V4L2_CTRL_FLAG_DISABLED flag should not be set), otherwise
> > + * a negative error code (-EINVAL) is returned
> > + *
>
> To mimick getControls(),
>
>
> * This method writes the value of all controls contained in \a ctrls, and
> * stores the values actually applied to the device in the corresponding
> * \a ctrls entry.
> *
> * If any control in \a ctrls is not supported by the device, is disabled (i.e.
> * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, is a
> * compound control, or if any other error occurs during validation of
> * the requested controls, no control is written and this method returns
> * -EINVAL.
> *
> * If an error occurs while writing the controls, the index of the first
> * control that couldn't be written is returned. All controls below that index
> * are written and their values are updated in \a ctrls, while all other
> * controls are not written and their values are not changed.
>
> > + * \return 0 on success or an error code otherwise
> > + * \retval -EINVAL One of the control is not supported or not accessible
> > + * \retval i The index of the control that failed
> > + */
> > +int V4L2Device::setControls(V4L2Controls *ctrls)
> > +{
> > + unsigned int count = ctrls->size();
> > + if (count == 0)
> > + return 0;
> > +
> > + const V4L2ControlInfo *controlInfo[count] = {};
> > + struct v4l2_ext_control v4l2Ctrls[count] = {};
> > +
> > + for (unsigned int i = 0; i < count; ++i) {
> > + V4L2Control *ctrl = (*ctrls)[i];
> > +
> > + const V4L2ControlInfo *info = getControlInfo(ctrl->id());
> > + if (!info) {
> > + LOG(V4L2, Error)
> > + << "Control '" << ctrl->id() << "' not found";
> > + return -EINVAL;
> > + }
> > +
> > + controlInfo[i] = info;
> > + v4l2Ctrls[i].id = info->id();
> > +
> > + /* Set the v4l2_ext_control value for the write operation. */
> > + switch (info->type()) {
> > + case V4L2_CTRL_TYPE_INTEGER64:
> > + v4l2Ctrls[i].value64 = ctrl->value();
> > + break;
> > + default:
> > + /*
> > + * \todo To be changed when support for string and
> > + * compound controls will be added.
> > + */
> > + v4l2Ctrls[i].value = static_cast<int32_t>(ctrl->value());
>
> Same here, do we need the cast ?
>
> > + break;
> > + }
> > + }
> > +
> > + struct v4l2_ext_controls v4l2ExtCtrls = {};
> > + v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;
> > + v4l2ExtCtrls.controls = v4l2Ctrls;
> > + v4l2ExtCtrls.count = count;
> > +
> > + int ret = ioctl(VIDIOC_S_EXT_CTRLS, &v4l2ExtCtrls);
> > + if (ret) {
> > + unsigned int errorIdx = v4l2ExtCtrls.error_idx;
> > +
> > + /* Generic validation error. */
> > + if (errorIdx == count) {
> > + LOG(V4L2, Error) << "Unable to read controls: "
> > + << strerror(ret);
> > + return -EINVAL;
> > + }
> > +
> > + /* A specific control failed. */
> > + LOG(V4L2, Error) << "Unable to read control " << errorIdx
> > + << ": " << strerror(ret);
> > + return errorIdx;
>
> Same here, I think you should update the controls that have been
> written.
>
> > + }
> > +
> > + /* Update the control value to what have actually been applied. */
> > + for (unsigned int i = 0; i < count; ++i) {
> > + struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> > + const V4L2ControlInfo *info = controlInfo[i];
> > + V4L2Control *ctrl = (*ctrls)[i];
> > +
> > + switch (info->type()) {
> > + case V4L2_CTRL_TYPE_INTEGER64:
> > + ctrl->setValue(v4l2Ctrl->value64);
> > + break;
> > + default:
> > + /*
> > + * \todo To be changed when support for string and
> > + * compound controls will be added.
> > + */
> > + ctrl->setValue(static_cast<long int>(v4l2Ctrl->value));
>
> And here too.
>
> > + break;
> > + }
> > + }
>
> This loop could possibly be moved to an updateControls() function and
> shared between getControls() and setControls().
>
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * \brief Perform an IOCTL system call on the device node
> > * \param[in] request The IOCTL request code
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190624/47792094/attachment.sig>
More information about the libcamera-devel
mailing list