[libcamera-devel] [PATCH v4 3/6] libcamera: v4l2_device: Implement get and set controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 20 12:55:50 CEST 2019
Hi Jacopo,
On Thu, Jun 20, 2019 at 12:11:48PM +0200, Jacopo Mondi wrote:
> On Wed, Jun 19, 2019 at 11:36:55PM +0300, Laurent Pinchart wrote:
> > On Wed, Jun 19, 2019 at 01:08:55PM +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 | 6 +
> > > src/libcamera/v4l2_device.cpp | 189 ++++++++++++++++++++++++++++
> > > 2 files changed, 195 insertions(+)
> > >
> > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> > > index 563be151c257..e66cc7ebe737 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,10 @@ public:
> > > bool isOpen() const { return fd_ != -1; }
> > > const std::string &deviceNode() const { return deviceNode_; }
> > >
> > > + V4L2ControlInfo *getControlInfo(unsigned int id);
> >
> > Should this be moved to the previous patch ?
>
> Why so, it is only used here...
Because control info got implemented in the previous patch, and the
commit message of this one only mentions get and set controls.
> > The returned pointer should be const, and the function should be const
> > too.
>
> Indeed. Fixed.
>
> > > + int getControls(V4L2Controls *ctrls);
> > > + int setControls(V4L2Controls *ctrls);
> > > +
> > > protected:
> > > V4L2Device(const std::string &deviceNode, const std::string &logTag);
> > > ~V4L2Device();
> > > @@ -37,6 +42,7 @@ protected:
> > >
> > > private:
> > > void listControls();
> > > + int validateControlType(V4L2ControlInfo *info);
> > >
> > > std::map<unsigned int, V4L2ControlInfo *> controls_;
> > > std::string deviceNode_;
> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > > index 58dd94836686..5af0ddc468fe 100644
> > > --- a/src/libcamera/v4l2_device.cpp
> > > +++ b/src/libcamera/v4l2_device.cpp
> > > @@ -66,6 +66,170 @@ void V4L2Device::close()
> > > * \return The device node path
> > > */
> > >
> > > +/**
> > > + * \brief Get informations for control with \a id
> >
> > "Retrieve information about a control" ?
> >
> > > + * \param[in] id The control id to search info for
> >
> > "The control ID" should be enough.
> >
> > > + * \return The V4L2ControlInfo associated to the V4L2 control with \a id or
> > > + * nullptr if the control is not supported by the V4L2Device
> >
> > \return A pointer to the V4L2ControlInfo for control \a id, or nullptr
> > if the device doesn't have such a control
> >
> > ?
>
> Taken in.
>
> > > + */
> > > +V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id)
> > > +{
> > > + auto it = controls_.find(id);
> > > + if (it == controls_.end())
> > > + return nullptr;
> > > +
> > > + return it->second;
> > > +}
> > > +
> > > +/**
> > > + * \brief Read values of a list of controls from the device
> >
> > "Read controls from the device"
> >
> > > + * \param[inout] ctrls The list of controls to read
> > > + *
> > > + * Read the value of each V4L2Controls contained in \a ctrls, overwriting
> > > + * it's current value with the one returned by the device.
> >
> > It's hard to write precise documentation without making it difficult to
> > read :-/ How about
> >
> > "Read the value of each control contained in \a ctrls, and store the
> > value in the corresponding \a ctrls entry."
> >
> > > + *
> > > + * Each V4L2Control instance in \a ctrls should be supported by the device.
> >
> > "If one control in \a ctrls is not supported by the device this method
> > returns an error. In that case the values stored in \a ctrls are
> > undefined."
> >
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int V4L2Device::getControls(V4L2Controls *ctrls)
> > > +{
> > > + unsigned int count = ctrls->size();
> > > + if (count == 0)
> > > + return 0;
> > > +
> > > + struct V4L2ControlInfo *controlInfo[count];
> > > + struct v4l2_ext_control v4l2Ctrls[count];
> > > + for (unsigned int i = 0; i < count; ++i) {
> > > + V4L2Control *ctrl = (*ctrls)[i];
> > > +
> > > + /* Validate the control. */
> > > + V4L2ControlInfo *info = getControlInfo(ctrl->id());
> > > + if (!info) {
> > > + LOG(V4L2, Error) << "Control '" << ctrl->id()
> > > + << "' not valid";
> >
> > s/not valid/not found/
> >
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (validateControlType(info))
> > > + return -EINVAL;
> > > +
> > > + controlInfo[i] = info;
> > > +
> > > + /* Prepare the v4l2_ext_control entry for the read operation. */
> > > + struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> > > + v4l2Ctrl->id = info->id();
> > > + v4l2Ctrl->size = info->size();
> >
> > size is only needed for controls with payloads, which we don't support.
> > I would leave it set to 0, otherwise if we try to get a payload control
> > by mistake, and the value happens to be a valid pointer to our memory
> > address space, the kernel could overwrite that.
> >
> > I would then also remove the v4l2Ctrl variable and write
> >
> > v4l2Ctrls[i].id = ctrl->id();
>
> Ack!
>
> > > + }
> > > +
> > > + 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) {
> > > + LOG(V4L2, Error) << "Unable to read controls: "
> > > + << strerror(ret);
> > > + return ret;
> > > + }
> > > +
> > > + /*
> > > + * For each control read from the device, set the value in the
> > > + * V4L2ControlValue provided by the caller.
> >
> > That's called V4L2Control now, not V4L2ControlValue.
> >
> > > + */
> > > + for (unsigned int i = 0; i < count; ++i) {
> > > + struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> > > + V4L2ControlInfo *info = controlInfo[i];
> > > + V4L2Control *ctrl = (*ctrls)[i];
> > > +
> > > + switch (info->type()) {
> > > + case V4L2_CTRL_TYPE_INTEGER64:
> > > + ctrl->setValue(v4l2Ctrl->value64);
> > > + break;
> > > + case V4L2_CTRL_TYPE_INTEGER:
> > > + case V4L2_CTRL_TYPE_BOOLEAN:
> > > + case V4L2_CTRL_TYPE_BUTTON:
> > > + case V4L2_CTRL_TYPE_MENU:
> > > + ctrl->setValue(static_cast<long int>(v4l2Ctrl->value));
> > > + break;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * \brief Write a list of controls to the device
> >
> > "Write controls to the device"
> >
> > > + * \param[in] ctrls The list of controls to apply
> >
> > s/apply/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 not updated to reflect what has been
> > > + * actually applied on the device. To read the actual value of a control
> > > + * after a setControls(), users should read the control value back with
> > > + * getControls().
> >
> > Why is that ? It would require another potentially expensive operation.
>
> This was actually something I was not sure of. Should we update the
> value after the control is written to HW? So every write is equivalent
> to a write+read?
I would do so, as the kernel API offers that feature, and it could be
useful. The update itself will be cheap compared to another
getControls() call.
> > > + *
> > > + * Each V4L2Control instance in \a ctrls should be supported by the device.
> >
> > "If one control in \a ctrls is not supported by the device this method
> > returns an error." to copy the text from getControls() ?
> >
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> >
> > I wonder if we should return the number of controls successfully
> > written. This would be 0 if the pre-validation fails, and we could use
> > v4l2_ext_controls::error_idx to get the index of the control that failed
> > (note that if error_idx == count then no control have been written).
>
> I generally prefer to have 0 for success, but in this case signaling
> the error back is quite tricky, also because the extended ctrls API
> does not help here... Returning the number of written controls might
> be a good idea. Should we do the same on read?
We could do the same on read for consistency, yes.
> > > + */
> > > +int V4L2Device::setControls(V4L2Controls *ctrls)
> > > +{
> > > + unsigned int count = ctrls->size();
> > > + if (count == 0)
> > > + return 0;
> > > +
> > > + struct v4l2_ext_control v4l2Ctrls[count];
> > > + for (unsigned int i = 0; i < count; ++i) {
> > > + V4L2Control *ctrl = (*ctrls)[i];
> > > +
> > > + /* Validate the control. */
> > > + V4L2ControlInfo *info = getControlInfo(ctrl->id());
> > > + if (!info) {
> > > + LOG(V4L2, Error) << "Control '" << ctrl->id()
> > > + << "' not valid";
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (validateControlType(info))
> > > + return -EINVAL;
> > > +
> > > + /* Prepare the v4l2_ext_control entry for the write operation. */
> > > + struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];
> > > + v4l2Ctrl->id = info->id();
> > > + v4l2Ctrl->size = info->size();
> > > +
> > > + switch (info->type()) {
> > > + case V4L2_CTRL_TYPE_INTEGER64:
> > > + v4l2Ctrl->value64 = ctrl->value();
> > > + break;
> > > + case V4L2_CTRL_TYPE_INTEGER:
> > > + case V4L2_CTRL_TYPE_BOOLEAN:
> > > + case V4L2_CTRL_TYPE_BUTTON:
> > > + case V4L2_CTRL_TYPE_MENU:
> > > + v4l2Ctrl->value = static_cast<int32_t>(ctrl->value());
> > > + break;
> > > + }
> >
> > This switch duplicates the one in validateControlType(). I would just
> > add a default case here and return an error, removing the call to
> > validateControlType(). The method would be left iwht a single caller, so
> > I would then inline it in getControls().
>
> Ah yes, the switch is repeated. I can fail in the second one yes.
>
> > > + }
> > > +
> > > + 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) {
> > > + LOG(V4L2, Error) << "Unable to write controls: "
> > > + << strerror(ret);
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /**
> > > * \brief Construct a V4L2Device
> > > * \param[in] deviceNode The device node filesystem path
> > > @@ -171,4 +335,29 @@ void V4L2Device::listControls()
> > > }
> > > }
> > >
> > > +/*
> > > + * \brief Make sure the control type is supported
> > > + * \param[in] info The V4L2ControlInfo to inspect type of
> > > + * \return 0 on success or a negative error code otherwise
> > > + * \retval -EINVAL The control type is not supported
> > > + */
> > > +int V4L2Device::validateControlType(V4L2ControlInfo *info)
> > > +{
> > > + /* \todo support compound and menu controls. */
> > > + switch (info->type()) {
> > > + case V4L2_CTRL_TYPE_INTEGER64:
> > > + case V4L2_CTRL_TYPE_INTEGER:
> > > + case V4L2_CTRL_TYPE_BOOLEAN:
> > > + case V4L2_CTRL_TYPE_BUTTON:
> > > + case V4L2_CTRL_TYPE_MENU:
> > > + break;
> > > + default:
> > > + LOG(V4L2, Error) << "Control type '" << info->type()
> > > + << "' not supported";
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > Another option would be to skip unsupported control types when
> > enumerating them, so the getControlInfo() call would return a nullptr.
>
> That too. I could call validateType there and remove the checks from
> get and set.
>
> > > +
> > > } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list