[libcamera-devel] [PATCH v2 3/6] libcamera: v4l2_videodevice: Support M2M devices
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Aug 12 11:01:53 CEST 2019
Hi Laurent,
On 10/08/2019 14:41, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> 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?
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 ..."
>> + *
>> * \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.
>> + * 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().
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?
> * 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
* handle. Like open(), this method queries the capabilities of the
device, but
* 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.
*
* 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