[libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice: Support M2M devices
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 8 23:03:11 CEST 2019
Hi Kieran,
Thank you for the patch.
On Thu, Aug 08, 2019 at 04:12:18PM +0100, Kieran Bingham wrote:
> V4L2 M2M devices represent a V4L2Device with two queues. One output, and
s/queues. One/queues: one/ (or "queues, one")
> one capture on a single device node.
>
> Represent this by instantiating a V4L2VideoDevice for each queue type,
> and preparing each device for its queue.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> src/libcamera/include/v4l2_videodevice.h | 26 ++-
> src/libcamera/v4l2_videodevice.cpp | 200 ++++++++++++++++++++---
> 2 files changed, 201 insertions(+), 25 deletions(-)
>
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index f5c8da93fcb5..24c58d787fde 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -71,6 +71,11 @@ struct V4L2Capability final : v4l2_capability {
> V4L2_CAP_VIDEO_OUTPUT |
> V4L2_CAP_VIDEO_OUTPUT_MPLANE);
> }
> + bool isM2M() const
> + {
> + return device_caps() & (V4L2_CAP_VIDEO_M2M |
> + V4L2_CAP_VIDEO_M2M_MPLANE);
> + }
> bool isMeta() const
> {
> return device_caps() & (V4L2_CAP_META_CAPTURE |
> @@ -123,7 +128,7 @@ public:
>
> V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
>
> - int open();
> + int open(int fd = -1, enum v4l2_buf_type type = V4L2_BUF_TYPE_PRIVATE);
> void close();
>
> const char *driverName() const { return caps_.driver(); }
> @@ -152,6 +157,9 @@ protected:
> std::string logPrefix() const;
>
> private:
> + int queryBufferType();
> + int queryBufferType(enum v4l2_buf_type type);
> +
> int getFormatMeta(V4L2DeviceFormat *format);
> int setFormatMeta(V4L2DeviceFormat *format);
>
> @@ -182,6 +190,22 @@ private:
> EventNotifier *fdEvent_;
> };
>
> +class V4L2M2MDevice
> +{
> +public:
> + V4L2M2MDevice(const std::string &deviceNode);
> + ~V4L2M2MDevice();
> +
> + V4L2VideoDevice *output() { return output_; }
> + V4L2VideoDevice *capture() { return capture_; }
> + unsigned int status() { return status_; }
The status is an unsigned int, and stores an error value that can be
negative, so there's a problem.
Furthermore, we have no status methods for the other V4L2-related
classes, so I think the open() should be split out from the constructor
to make the API consistent.
> +
> +private:
> + V4L2VideoDevice *output_;
> + V4L2VideoDevice *capture_;
> + unsigned int status_;
> +};
> +
> } /* namespace libcamera */
>
> #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 81098dd70190..4bd33af5f3de 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -89,6 +89,12 @@ LOG_DECLARE_CATEGORY(V4L2)
> * \return True if the video device can capture or output images
> */
>
> +/**
> + * \fn V4L2Capability::isM2M()
> + * \brief Identify if the device is an M2M device
> + * \return True if the device can capture and output images using the M2M API
> + */
> +
> /**
> * \fn V4L2Capability::isMeta()
> * \brief Identify if the video device captures or outputs image meta-data
> @@ -294,17 +300,80 @@ V4L2VideoDevice::~V4L2VideoDevice()
> close();
> }
>
> +int V4L2VideoDevice::queryBufferType()
> +{
> + if (caps_.isVideoCapture()) {
> + fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> + bufferType_ = caps_.isMultiplanar()
> + ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> + : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + } else if (caps_.isVideoOutput()) {
> + fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> + bufferType_ = caps_.isMultiplanar()
> + ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> + : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> + } else if (caps_.isMetaCapture()) {
> + fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> + bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> + } else if (caps_.isMetaOutput()) {
> + fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> + bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> + } else {
> + LOG(V4L2, Error) << "Device is not a supported type";
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +int V4L2VideoDevice::queryBufferType(enum v4l2_buf_type type)
> +{
> + switch (type) {
> + case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> + fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> + bufferType_ = caps_.isMultiplanar()
> + ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> + : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> + break;
> + case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> + fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> + bufferType_ = caps_.isMultiplanar()
> + ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> + : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> + break;
> + default:
> + LOG(V4L2, Error) << "Unsupported buffer type";
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
These methods don't rely query the buffer type, they should be renamed.
The two overloaded version are also a bit confusing :-(
> +
> /**
> * \brief Open a V4L2 video device and query its capabilities
> + *
> + * \param[in] fd The file descriptor to set (optional)
> + * \param[in] type The device type to operate on (optional)
> + *
> + * Opens a device or sets the file descriptor if provided, and then queries the
> + * capabilities of the device and establishes the buffer types and device events
> + * accordingly.
> + *
> * \return 0 on success or a negative error code otherwise
> */
> -int V4L2VideoDevice::open()
> +int V4L2VideoDevice::open(int fd, enum v4l2_buf_type type)
There's very little code shared between the two implementations of
open(), I think I would make it two separate methods (both with the same
name) and remove the default parameter values in the method declaration.
The code would be easier to read, and you could keep the
queryBufferType() code inlined.
> {
> int ret;
>
> - ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> - if (ret < 0)
> - return ret;
> + if (fd != -1) {
> + ret = V4L2Device::setFd(fd);
> + if (ret < 0)
> + return ret;
> + } else {
> + ret = V4L2Device::open(O_RDWR | O_NONBLOCK);
> + if (ret < 0)
> + return ret;
> + }
>
> ret = ioctl(VIDIOC_QUERYCAP, &caps_);
> if (ret < 0) {
> @@ -324,26 +393,10 @@ int V4L2VideoDevice::open()
> * devices (POLLIN), and write notifications for OUTPUT video devices
> * (POLLOUT).
> */
> - if (caps_.isVideoCapture()) {
> - fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> - bufferType_ = caps_.isMultiplanar()
> - ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> - : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> - } else if (caps_.isVideoOutput()) {
> - fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> - bufferType_ = caps_.isMultiplanar()
> - ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> - : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> - } else if (caps_.isMetaCapture()) {
> - fdEvent_ = new EventNotifier(fd(), EventNotifier::Read);
> - bufferType_ = V4L2_BUF_TYPE_META_CAPTURE;
> - } else if (caps_.isMetaOutput()) {
> - fdEvent_ = new EventNotifier(fd(), EventNotifier::Write);
> - bufferType_ = V4L2_BUF_TYPE_META_OUTPUT;
> - } else {
> - LOG(V4L2, Error) << "Device is not a supported type";
> - return -EINVAL;
> - }
> + if (type != V4L2_BUF_TYPE_PRIVATE)
This check in particular is very confusing, using V4L2_BUF_TYPE_PRIVATE
as a magic default value for the non-M2M case is quite a bit of a hack.
> + queryBufferType(type);
> + else
> + queryBufferType();
>
> fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);
> fdEvent_->setEnabled(false);
> @@ -1143,4 +1196,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,
> return new V4L2VideoDevice(mediaEntity);
> }
>
> +/**
> + * \class V4L2M2MDevice
> + * \brief Memory2Memory device container
> + *
> + * The V4L2M2MDevice manages two V4L2Device instances on the same
s/V4L2Device/V4L2VideoDevice/
> + * deviceNode which operate together using two queues to implement the V4L2
s/deviceNode/device node/
> + * Memory to Memory API.
> + *
> + * V4L2M2MDevices are opened at the point they are created, and will return
s/V4L2M2MDevices/V4L2M2MDevice instances/ (class name shall not use the
plural, otherwise doxygen won't generate links properly)
s/will //
> + * both a capture and an output device or neither in the event of failure.
> + *
> + * The two devices should be configured and started as normal V4L2Device
s/should/shall/
> + * instances. Closing either of the devices will require recreating a new
> + * V4L2M2MDevice.
With the proposal to add an open() method, I would add a close() method
to V4L2M2MDevice that closes both V4L2VideoDevice, and document here
that the managed V4L2VideoDevice instances shall not be closed manually.
> + *
> + * The two V4L2Device instances share a single device node which is opened at
s/V4L2Device/V4L2VideoDevice/
> + * the point the V4L2M2MDevice is created.
> + */
> +
> +/**
> + * \fn V4L2M2MDevice::output
Missing \brief, and below too.
> + * \return The output V4L2Device instance
> + */
> +
> +/**
> + * \fn V4L2M2MDevice::capture
> + * \return The capture V4L2Device instance
> + */
> +
> +/**
> + * \fn V4L2M2MDevice::status
> + * \return The construction status of the V4L2M2MDevice
You should document what the status value contains. Or rather drop this
method as it won't be needed with open(). Otherwise I'd recommend
replacing status() with an isValid() method that returns a bool, that
would make the API more explicit.
> + */
> +
> +/**
> + * \brief Create a new V4L2M2MDevice from the \a deviceNode
> + *
> + * The deviceNode will be opened and shared across two V4L2VideoDevice
"will" ? It's done in this method, not later :-)
> + * instances. One to control the output stream, and one to control the capture
> + * stream.
The second sentence has no verb.
> + *
> + * After construction, the status() must be checked to validate the instance.
> + */
> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)
> + : status_(0)
> +{
> + int fd[2] = { -1, -1 };
> + int ret;
> +
> + output_ = new V4L2VideoDevice(deviceNode);
> + capture_ = new V4L2VideoDevice(deviceNode);
> +
> + /* Both V4L2Devices will have the same deviceNode. */
More than the same device node, they use the same file handle.
/*
* Both the output and capture V4L2Device instances use the same
* file handle for the same device node.
*/
> + fd[0] = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);
> + if (fd[0] < 0) {
> + ret = -errno;
> + LOG(V4L2, Error)
> + << "Failed to open V4L2 M2M device: " << strerror(-ret);
> + goto err;
> + }
> +
> + fd[1] = dup(fd[0]);
> + if (fd[1] < 0) {
> + ret = -errno;
> + LOG(V4L2, Error)
> + << "Failed to duplicate M2M device: " << strerror(-ret);
> +
> + goto err;
> + }
> +
> + ret = output_->open(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);
> + if (ret)
> + goto err;
> +
> + ret = capture_->open(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);
> + if (ret)
> + goto err;
> +
> + return;
> +
> +err:
> + delete capture_;
> + delete output_;
> +
> + capture_ = nullptr;
> + output_ = nullptr;
> +
> + status_ = ret;
> +
> + close(fd[1]);
> + close(fd[0]);
This is racy. delete output_ above will close fd[0] if the
output_->open() call succeeded, and another thread could open a file
that reuses the same file descriptor before the close() call here.
> +}
> +
> +V4L2M2MDevice::~V4L2M2MDevice()
> +{
> + delete capture_;
> + delete output_;
> +}
> +
> } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list