[libcamera-devel] [PATCH v2 3/6] libcamera: v4l2_videodevice: Support M2M devices
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Aug 12 14:42:30 CEST 2019
Hi Laurent,
On 12/08/2019 12:16, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Mon, Aug 12, 2019 at 10:01:53AM +0100, Kieran Bingham wrote:
>> On 10/08/2019 14:41, Laurent Pinchart wrote:
>>> On Fri, Aug 09, 2019 at 04:04:56PM +0100, Kieran Bingham wrote:
>>>> V4L2 M2M devices represent a V4L2Device with two queues: One output, and
>>>> 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 | 28 ++++
>>>> src/libcamera/v4l2_videodevice.cpp | 186 +++++++++++++++++++++++
>>>> 2 files changed, 214 insertions(+)
>>>>
>>>> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
>>>> index f5c8da93fcb5..72dc8c63e4bb 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 |
>>>> @@ -124,6 +129,7 @@ public:
>>>> V4L2VideoDevice &operator=(const V4L2VideoDevice &) = delete;
>>>>
>>>> int open();
>>>> + int open(int handle, enum v4l2_buf_type type);
>>>> void close();
>>>>
>>>> const char *driverName() const { return caps_.driver(); }
>>>> @@ -152,6 +158,9 @@ protected:
>>>> std::string logPrefix() const;
>>>>
>>>> private:
>>>> + int queryBufferType();
>>>> + int queryBufferType(enum v4l2_buf_type type);
>>>
>>> I think these are leftovers.
>>
>> Yes, removed.
>>
>>>> +
>>>> int getFormatMeta(V4L2DeviceFormat *format);
>>>> int setFormatMeta(V4L2DeviceFormat *format);
>>>>
>>>> @@ -182,6 +191,25 @@ private:
>>>> EventNotifier *fdEvent_;
>>>> };
>>>>
>>>> +class V4L2M2MDevice
>>>> +{
>>>> +public:
>>>> + V4L2M2MDevice(const std::string &deviceNode);
>>>> + ~V4L2M2MDevice();
>>>> +
>>>> + int open();
>>>> + void close();
>>>> +
>>>> + V4L2VideoDevice *output() { return output_; }
>>>> + V4L2VideoDevice *capture() { return capture_; }
>>>> +
>>>> +private:
>>>> + std::string deviceNode_;
>>>> +
>>>> + V4L2VideoDevice *output_;
>>>> + V4L2VideoDevice *capture_;
>>>> +};
>>>> +
>>>> } /* namespace libcamera */
>>>>
>>>> #endif /* __LIBCAMERA_V4L2_VIDEODEVICE_H__ */
>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>>>> index 81098dd70190..9c5638995577 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
>>>> @@ -296,6 +302,10 @@ V4L2VideoDevice::~V4L2VideoDevice()
>>>>
>>>> /**
>>>> * \brief Open a V4L2 video device and query its capabilities
>>>> + *
>>>> + * Opens a video device, then queries the capabilities of the device and
>>>
>>> s/Opens/This method opens/
>>>
>>>> + * establishes the buffer types and device events accordingly.
>>>
>>> The last part is internal matters I think. I would just drop this
>>> paragraph, and update the brief to state "Open a V4L2 video device node
>>> and query its capabilities".
>>
>> You mean simply add the word 'node' to the existing brief?
>
> Yes, as the main difference between this method and the fd-based open
> method is that this one operates on a device node.
>
>> The 'a' doesn't give enough context in that case any more (or perhaps
>> before), because it only opens 'the' device node that exists in this
>> class instance. It doesn't open any device node. Only the one which was
>> previously set in the constructor.
>>
>> I'll change it to :
>>
>> "open /the/ V4L2 video device node and ..."
>
> Sounds good to me.
>
>>>> + *
>>>> * \return 0 on success or a negative error code otherwise
>>>> */
>>>> int V4L2VideoDevice::open()
>>>> @@ -355,6 +365,83 @@ int V4L2VideoDevice::open()
>>>> return 0;
>>>> }
>>>>
>>>> +/**
>>>> + * \brief Open a V4L2 video device and query its capabilities
>>>
>>> And here "Open a V4L2 video device from an opened file handle and query
>>> its capabilities".
>>
>> Changed.
>>
>>>> + *
>>>
>>> No need for this blank line.
>>>
>>>> + * \param[in] handle The file descriptor to set
>>>
>>> s/handle/fd/ ?
>>
>> No,.
>>
>>>> + * \param[in] type The device type to operate on
>>>> + *
>>>> + * Sets the file descriptor for the device and then queries the capabilities of
>>>
>>> s/Sets/This method sets/
>>>
>>> You need a subject before a conjugated verb.
>>
>> We're /in/ the subject. It can be implicit.
>
> It could just be me, and my over-exposure to pesky French grammar and
> conjugation :-) I've always been taught that a sentence that contain a
> cnjugated verb at any mood other than the imperative requires an
> explicit subject. Are the rules different in English ?
Maybe we need someone who's taken languages as a degree for that.
<checks with Mrs B>
Well Mrs-B agrees with you, saying it should be at least "This sets"
(with the 'method' being optional)
> Interestingly enough the style of the \brief seems fine to me, even
> though I'm not sure why. "Open a V4L2 video device" sounds correct to my
> ears (but what tense is "open" conjugated in ?), while starting a
Open is not conjugated - it's just the imperative form.
> sentence with "Opens a V4L2 video device" sounds like something is
> missing.
>
> I'm obviously not challenging your native English knowledge :-) I would
> however like to understand what the grammatical rules are, in order to
> improve my own knowledge (and stop perstering everybody with similar
> grammatical comments).
Well, I don't think my native language processing follows written rules,
it just goes with what sounds right :-D ... and provides the subject
from the context - so "Opens a V4L2 video device" sounds right to me
(when in the context of a description of a function).
But if it's not right - then it's not right.
>>>> + * the device and establishes the buffer types and device events accordingly.
>>>
>>> * This methods opens a video device from the existing file descriptor \a fd.
>>
>> We can't use fd as an argument, because we use fd().
>
> Ah yes, I didn't notice that. We could use the qualified
> V4L2Device::fd() version and name the argument fd, but it's not worth
> it. Let's thus keep handle.
That's possibly a good option too, but I do like the clarity of having a
distinct name.
>> We can't use newFd, because that's the result of the dup().
>>
>> That's why I've used handle.
>>
>>> * Like open() queries the capabilities of the device, but handles it according
>>
>> ^
>> Isn't this the same issue?
>
> Absolutely :-) Sorry for the mistake.
>
>>
>>> * to the given device \a type instead of determining its type from the
>>> * capabilities. This can be used to force a given device type for M2M devices.
>>> *
>>> * The file descriptor \a fd is duplicated, and the caller shall close \a fd
>>> * when it has no further use for it. The close() method will close the
>>> * duplicated file descriptor, leaving \a fd untouched.
>>
>> Changed to:
>>
>> * This methods opens a video device from the existing file descriptor \a
>
> s/methods/method/ ?
Good spot - fixed.
>> * handle. Like open(), this method queries the capabilities of the device, but
>
> I would have written s/this method/it/ but that's up to you.
I started with 'it', but it seemed we may as well fully-qualify the subject.
Maybe I'll drop the 'method', and keep "This queries,' to reduce the
repetition.
>
>> * handles it according to the given device \a type instead of determining its
>> * type from the capabilities. This can be used to force a given device type for
>> * M2M devices.
>
> Sounds very good to me.
>
>> *
>> * The file descriptor \a handle is duplicated, and the caller is responsible
>> * for closing the \a handle when it has no further use for it. The close()
>> * method will close the duplicated file descriptor, leaving \a handle
>> * untouched.
>>
>>>> + *
>>>> + * \return 0 on success or a negative error code otherwise
>>>> + */
>>>> +int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
>>>> +{
>>>> + int ret;
>>>> + int newFd;
>>>> +
>>>> + newFd = dup(handle);
>>>> + if (newFd < 0) {
>>>> + ret = -errno;
>>>> + LOG(V4L2, Error) << "Failed to duplicate file handle: "
>>>> + << strerror(-ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = V4L2Device::setFd(newFd);
>>>> + if (ret < 0) {
>>>> + LOG(V4L2, Error) << "Failed to set file handle: "
>>>> + << strerror(-ret);
>>>
>>> You need to ::close(newFd) here.
>>>
>>
>> Ah yes.
>>
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = ioctl(VIDIOC_QUERYCAP, &caps_);
>>>> + if (ret < 0) {
>>>> + LOG(V4L2, Error)
>>>> + << "Failed to query device capabilities: "
>>>> + << strerror(-ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + if (!caps_.hasStreaming()) {
>>>> + LOG(V4L2, Error) << "Device does not support streaming I/O";
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + /*
>>>> + * Set buffer type and wait for read notifications on CAPTURE video
>>>> + * devices (POLLIN), and write notifications for OUTPUT video devices
>>>> + * (POLLOUT).
>>>> + */
>>>> + 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;
>>>> + }
>>>> +
>>>> + fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);
>>>> + fdEvent_->setEnabled(false);
>>>> +
>>>> + LOG(V4L2, Debug)
>>>> + << "Opened device " << caps_.bus_info() << ": "
>>>> + << caps_.driver() << ": " << caps_.card();
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> /**
>>>> * \brief Close the video device, releasing any resources acquired by open()
>>>> */
>>>> @@ -1143,4 +1230,103 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,
>>>> return new V4L2VideoDevice(mediaEntity);
>>>> }
>>>>
>>>> +/**
>>>> + * \class V4L2M2MDevice
>>>> + * \brief Memory2Memory video device
>>>> + *
>>>> + * The V4L2M2MDevice manages two V4L2VideoDevice instances on the same
>>>> + * deviceNode which operate together using two queues to implement the V4L2
>>>> + * Memory to Memory API.
>>>> + *
>>>> + * The two devices should be opened by calling open() on the V4L2M2MDevice, and
>>>> + * can be closed by calling close on the V4L2M2MDevice.
>>>> + *
>>>> + * Calling V4L2VideoDevice::open() and V4L2VideoDevice::close() on the capture
>>>> + * or output V4L2VideoDevice is not permitted.
>>>> + */
>>>> +
>>>> +/**
>>>> + * \fn V4L2M2MDevice::output
>>>> + * \brief Provide access to the output V4L2VideoDevice instance
>>>
>>> s/Provide access to/Retrieve/
>>
>> Changed.
>>
>>>> + * \return The output V4L2Device instance
>>>> + */
>>>> +
>>>> +/**
>>>> + * \fn V4L2M2MDevice::capture
>>>> + * \brief Provide access to the capture V4L2VideoDevice instance
>>>
>>> s/Provide access to/Retrieve/
>>
>> Changed.
>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
>> Thanks,
>>
>>>> + * \return The capture V4L2Device instance
>>>> + */
>>>> +
>>>> +/**
>>>> + * \brief Create a new V4L2M2MDevice from the \a deviceNode
>>>> + */
>>>> +V4L2M2MDevice::V4L2M2MDevice(const std::string &deviceNode)
>>>> + : deviceNode_(deviceNode)
>>>> +{
>>>> + output_ = new V4L2VideoDevice(deviceNode);
>>>> + capture_ = new V4L2VideoDevice(deviceNode);
>>>> +}
>>>> +
>>>> +V4L2M2MDevice::~V4L2M2MDevice()
>>>> +{
>>>> + delete capture_;
>>>> + delete output_;
>>>> +}
>>>> +
>>>> +/**
>>>> + * \brief Open a V4L2 Memory to Memory device
>>>> + *
>>>> + * Open the device node and prepare the two V4L2VideoDevice instances to handle
>>>> + * their respective buffer queues.
>>>> + *
>>>> + * \return 0 on success or a negative error code otherwise
>>>> + */
>>>> +int V4L2M2MDevice::open()
>>>> +{
>>>> + int fd;
>>>> + int ret;
>>>> +
>>>> + /*
>>>> + * 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.
>>>> + */
>>>> + fd = ::open(deviceNode_.c_str(), O_RDWR | O_NONBLOCK);
>>>> + if (fd < 0) {
>>>> + ret = -errno;
>>>> + LOG(V4L2, Error)
>>>> + << "Failed to open V4L2 M2M device: " << strerror(-ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = output_->open(fd, V4L2_BUF_TYPE_VIDEO_OUTPUT);
>>>> + if (ret)
>>>> + goto err;
>>>> +
>>>> + ret = capture_->open(fd, V4L2_BUF_TYPE_VIDEO_CAPTURE);
>>>> + if (ret)
>>>> + goto err;
>>>> +
>>>> + ::close(fd);
>>>> +
>>>> + return 0;
>>>> +
>>>> +err:
>>>> + close();
>>>> + ::close(fd);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * \brief Close the memory-to-memory device, releasing any resources acquired by
>>>> + * open()
>>>> + */
>>>> +void V4L2M2MDevice::close()
>>>> +{
>>>> + capture_->close();
>>>> + output_->close();
>>>> +}
>>>> +
>>>> } /* namespace libcamera */
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list