[libcamera-devel] [PATCH v2 3/6] libcamera: v4l2_videodevice: Support M2M devices
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 12 13:16:37 CEST 2019
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 ?
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
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).
> >> + * 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.
> 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/ ?
> * 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.
> * 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list