[libcamera-devel] [PATCH 3/6] libcamera: v4l2_videodevice: Support M2M devices
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Aug 9 16:29:37 CEST 2019
Hi Laurent,
On 09/08/2019 09:25, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Fri, Aug 09, 2019 at 09:13:42AM +0100, Kieran Bingham wrote:
>> On 08/08/2019 22:03, Laurent Pinchart wrote:
>>> 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")
>>>
Done
>>>> 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.
Split done.
>>
>>>> +
>>>> +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...
>
> Yes, I'm sorry. I was hoping code could be shared in a clean way, but
> the end result is more difficult to read, and thus not worth it in my
> opinion :-( Not your fault of course, just the code's fault :-)
Ok - I've gone back to two open() calls. One for normal code paths, and
one for the M2M code path.
>
>>>> +
>>>> /**
>>>> * \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)
>>
>>>> {
>>>> 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?
>
> What's the right word for the middle part of fd -> file -> inode ?
> "file" usually refers to a file on disk, and so does device node.
> According to man dup,
>
> "After a successful return, the old and new file descriptors may be used
> interchangeably. They refer to the same open file description (see
> open(2)) and thus share file offset and file status flags; for example,
> if the file offset is modified by using lseek(2) on one of the file
> descriptors, the offset is also changed for the other."
>
> "File description" sounds weird.
I've added
/*
* The output and capture V4L2VideoDevice instances use the same file
* handle for the same device node. The local file handle can be closed
* as the V4L2VideoDevice::open() retains a handle by duplicating the
* fd passed in.
*/
(which is correct in v2, where the dup happens in the open(fd, type) call)
>
>>> /*
>>> * 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.
>
> Good idea !
>
> The other option would be to skip close() on the fd in V4L2Device if the
> fd was set with setFd(), and close it in V4L2M2MDevice::close(). This
> couldn't be done before as there was no close() for V4L2M2MDevice. What
> do you think would be best ?
I prefer handling the dup in the open call so that each device clearly
'takes' it's own handle.
And that's what I've done.
V2 post imminent.
>>>> +}
>>>> +
>>>> +V4L2M2MDevice::~V4L2M2MDevice()
>>>> +{
>>>> + delete capture_;
>>>> + delete output_;
>>>> +}
>>>> +
>>>> } /* namespace libcamera */
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list