[libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice: Support M2M devices
Jacopo Mondi
jacopo at jmondi.org
Fri Aug 9 16:12:23 CEST 2019
Hi Kieran,
On Fri, Aug 09, 2019 at 09:13:42AM +0100, Kieran Bingham wrote:
> Hi Laurent,
>
> On 08/08/2019 22:03, Laurent Pinchart wrote:
> > Hi Kieran,
> >
> > Thank you for the patch.
I don't have other comments than Laurent's one, but...
> >
> > 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.
>
> Ah yes, that should be int. Doesn't matter it's going to be dropped.
>
> >
> > 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.
>
> I'll split it. in that case I'll add close() as well.
>
>
> >> +
> >> +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 :-(
>
> It was the simplest way to satisfy your earlier request to share code.
> This segment is the only part which is not shared across the two
> possible open() call paths...
>
> >> +
> >> /**
> >> * \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.
>
> Ok - back to the RFC version then (with the rename)
I too find quite confusing using the default arguments to
differentiate between the use cases. I mean, not weird, but hard to
follow. I would overload the operation or either make a new one for
the purpose and open code queryBufferType there.
Thanks
j
>
>
>
> >> {
> >> 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.
>
>
> It was required from your request to share the code paths, and use the
> v4l2_buf_type. It was the only value I could (half-legitimately) use to
> prevent getting:
>
> vimc.cpp:340:19: error: invalid conversion from ‘int’ to
> ‘v4l2_buf_type’ [-fpermissive]
> if (video_->open())
>
> If the arguments are not overloaded, then we don't need to specify a
> default or switch on it.
>
>
> >
> >> + 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)
>
>
> Indeed.
>
>
> > 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.
>
> Haha - yes, I'd already come to the close() conclusion as well.
>
>
> >
> >> + *
> >> + * 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.
>
> Will add.
>
> >
> >> + * \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.
>
> I'll go with adding open()/close()
>
>
> >
> >> + */
> >> +
> >> +/**
> >> + * \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.
>
> Yes, there should be a semi-colon after instances instead of a full-stop.
>
> >
> >> + *
> >> + * 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.
>
> Would you consider two handles (after they are dup()d) to be the same
> file handle?
>
> Aren't they separate handles representing the same device context which
> can have their own lifetime?
>
>
> > /*
> > * 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.
>
> I'll move the dup() call to the overloaded open call, and always close
> the local handle instead.
>
> >> +}
> >> +
> >> +V4L2M2MDevice::~V4L2M2MDevice()
> >> +{
> >> + delete capture_;
> >> + delete output_;
> >> +}
> >> +
> >> } /* namespace libcamera */
> >
>
> --
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- 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/20190809/4b1b93b6/attachment.sig>
More information about the libcamera-devel
mailing list