[libcamera-devel] [PATCH 2/6] libcamera: v4l2_device: Add setControl function

[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 at 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