| Message ID | 20190619110858.20980-4-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. 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@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 ? The returned pointer should be const, and the function should be const too. > + 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 ? > + */ > +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(); > + } > + > + 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. > + * > + * 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). > + */ > +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(). > + } > + > + 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. > + > } /* namespace libcamera */
Hi Laurent, thanks for review. On Wed, Jun 19, 2019 at 11:36:55PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > 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@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... > > 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? > > + * > > + * 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? > > + */ > > +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. Thanks j > > + > > } /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
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@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 */
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); + 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 + * \param[in] id The control id to search info for + * \return The V4L2ControlInfo associated to the V4L2 control with \a id or + * nullptr if the control is not supported by the V4L2Device + */ +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 + * \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. + * + * Each V4L2Control instance in \a ctrls should be supported by the device. + * + * \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"; + 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(); + } + + 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. + */ + 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 + * \param[in] ctrls The list of controls to apply + * + * 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(). + * + * Each V4L2Control instance in \a ctrls should be supported by the device. + * + * \return 0 on success or a negative error code otherwise + */ +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; + } + } + + 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; +} + } /* namespace libcamera */
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@jmondi.org> --- src/libcamera/include/v4l2_device.h | 6 + src/libcamera/v4l2_device.cpp | 189 ++++++++++++++++++++++++++++ 2 files changed, 195 insertions(+)