[libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Support M2M devices
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 8 13:33:51 CEST 2019
Hi Kieran,
On Thu, Aug 08, 2019 at 11:32:59AM +0100, Kieran Bingham wrote:
> On 04/05/2019 15:47, Laurent Pinchart wrote:
> > On Fri, May 03, 2019 at 04:39:51PM +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 V4L2Device for each queue type, and
> >> preparing each device for its queue.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>
> >> Marking this patch as RFC as yet it does not have a user or test case associated with it.
> >>
> >> However this patch is part of a development to support a RaspberryPi pipeline
> >> handler and is sent independently for some early review and bikeshedding :-)
> >>
> >> It has been used successfully to interact with the RaspberryPi ISP M2M driver.
> >>
> >> src/libcamera/include/v4l2_device.h | 13 +++
> >> src/libcamera/v4l2_device.cpp | 159 ++++++++++++++++++++++++++++
> >> 2 files changed, 172 insertions(+)
> >>
> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> >> index 689e18dea7a6..4eaf140f452f 100644
> >> --- a/src/libcamera/include/v4l2_device.h
> >> +++ b/src/libcamera/include/v4l2_device.h
> >> @@ -62,6 +62,11 @@ struct V4L2Capability final : v4l2_capability {
> >> return device_caps() & (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 isVideo() const
> >> {
> >> return device_caps() & (V4L2_CAP_VIDEO_CAPTURE |
> >> @@ -117,6 +122,7 @@ public:
> >> V4L2Device &operator=(const V4L2Device &) = delete;
> >>
> >> int open();
> >> + int setFD(int fd, int type);
> >
> > s/setFD/setFd/ ?
>
> Done.
>
> > I would replace int type with an enum to make the API clearer.
> > v4l2_buf_type is a candidate. Using the V4L2M2MDeviceType enum defined
> > below would make this function specific to M2M, while I see no reason
> > not to make it generic.
>
> I'm not sure I fully understand you here.
> Do you mean you would see
>
> int V4L2VideoDevice::setFd(int fd, enum v4l2_buf_type type)
> {
> }
>
> Where we call with:
>
> ret = m2m.output->setFd(fd[0], V4L2_BUF_TYPE_VIDEO_OUTPUT);
> ret = m2m.capture->setFd(fd[1], V4L2_BUF_TYPE_VIDEO_CAPTURE);
>
> ?
Yes that's what I mean.
> Doesn't that then lead people to think they could call with arbitrary
> valid V4L2_BUF_TYPE_VIDEO_xxx types which would not be the case?
Possibly, but it's an internal API, so if we document it properly I
don't think that's an issue. If you think it's a problem then we can use
a custom enum.
> >> bool isOpen() const;
> >> void close();
> >>
> >> @@ -178,6 +184,13 @@ private:
> >> EventNotifier *fdEvent_;
> >> };
> >>
> >> +struct V4L2M2MDevice {
> >> + V4L2Device *output;
> >> + V4L2Device *capture;
> >> +};
> >> +
> >> +struct V4L2M2MDevice createM2MPair(const std::string &deviceNode);
> >> +
> >> } /* namespace libcamera */
> >>
> >> #endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> >> index 916f0360503f..cba1c1799596 100644
> >> --- a/src/libcamera/v4l2_device.cpp
> >> +++ b/src/libcamera/v4l2_device.cpp
> >> @@ -110,6 +110,12 @@ LOG_DEFINE_CATEGORY(V4L2)
> >> * \return True if the device can 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::isMetaCapture()
> >> * \brief Identify if the device captures image meta-data
> >> @@ -359,6 +365,74 @@ int V4L2Device::open()
> >> return 0;
> >> }
> >>
> >> +enum V4L2M2MDeviceType {
> >> + V4L2M2MOutput,
> >> + V4L2M2MCapture,
> >> +};
> >> +
> >> +/**
> >> + * \brief Open a V4L2 M2M device for a specific queue type using an existing fd
> >> + * \return 0 on success or a negative error code otherwise
> >> + */
> >> +int V4L2Device::setFD(int fd, int type)
> >> +{
> >> + int ret;
> >> +
> >> + fd_ = fd;
> >> +
> >> + ret = ioctl(fd_, VIDIOC_QUERYCAP, &caps_);
> >> + if (ret < 0) {
> >> + ret = -errno;
> >> + LOG(V4L2, Error)
> >> + << "Failed to query device capabilities: "
> >> + << strerror(-ret);
> >> + return ret;
> >> + }
> >> +
> >> + LOG(V4L2, Debug)
> >> + << "Opened device" << caps_.bus_info() << ": "
> >> + << caps_.driver() << ": " << caps_.card();
> >> +
> >> + if (!caps_.isM2M()) {
> >> + LOG(V4L2, Error) << "Device is not an M2M device";
> >> + return -EINVAL;
> >> + }
> >> +
> >
> > Is this a problem ?
>
> SetFd is only used by M2M devices. Other devices should use open()
But couldn't there be cases where a non-M2M device would benefit from
this API ? Not in our current implementation, but I could imagine this
being the case in the future. For instance an IPA could receive the FD
of a device node that it would otherwise not have permission to open
itself.
> I've looked through and I really can't see a good way of making open()
> use SetFd effectively.
>
> It's not really about setting the Fd so we could rename the function
> instead. It's an alternate open() call really. (Where open() does more
> than just call ::open() anyway )
>
> I'm 'open' to suggestions on the name :D
>
> how about calling this openM2M() (even though it doesn't do the open())
>
> otherwise we could call it queryM2M(), but that would suggest that
> open() should be called query() ... :S
We could call it open().
> >> + 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 devices
> >> + * (POLLIN), and write notifications for OUTPUT devices (POLLOUT).
> >> + */
> >> + switch(type) {
> >> + case V4L2M2MOutput:
> >> + fdEvent_ = new EventNotifier(fd_, EventNotifier::Write);
> >> + bufferType_ = caps_.isMultiplanar()
> >> + ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> >> + : V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >> + break;
> >> + case V4L2M2MCapture:
> >> + fdEvent_ = new EventNotifier(fd_, EventNotifier::Read);
> >> + bufferType_ = caps_.isMultiplanar()
> >> + ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> >> + : V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >> +
> >> + break;
> >> + default:
> >> + LOG(V4L2, Debug) << "M2M type is not supported";
> >> + return -EINVAL;
> >> + }
> >
> > Could we share some of the code with open() ? MAybe open() should call
> > setFd() ?
>
> This doesn't seem to work well. This switch statement is distinctly
> different from the one in open, and the two calls {open(), setFd()}
> revolve heavily around the decisions made here.
>
> I can't see a clean way to share code between the two code paths that
> would be easier to maintain. (other than making several smaller
> functions which I'm not sure would be better)
>
> Feel free to propose something if you want the code paths altered.
If it can't be done, so be it :-)
> >> +
> >> + fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable);
> >> + fdEvent_->setEnabled(false);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +
>
> Extra line to remove there. <done>
>
> >> /**
> >> * \brief Check if device is successfully opened
> >> * \return True if the device is open, false otherwise
> >> @@ -1049,4 +1123,89 @@ V4L2Device *V4L2Device::fromEntityName(const MediaDevice *media,
> >> return new V4L2Device(mediaEntity);
> >> }
> >>
> >> +/**
> >> + * \struct V4L2M2MDevice
> >> + * \brief Memory2Memory device container
> >> + *
> >> + * The V4L2M2MDevice structure manages two V4L2Device instances on the same
> >> + * deviceNode which operate together using two queues to implement the V4L2
> >> + * Memory to Memory API.
> >> + *
> >> + * V4L2M2MDevices are opened at the point they are created, and will return
> >> + * both a capture and an output device or neither in the event of failure.
> >> + *
> >> + * The two devices should be configured and started as a normal V4L2Device.
> >
> > s/a normal V4L2Device/normal V4L2Device instances/
> >
> >> + * Closing the device will require recreating a new pair.
> >
> > "Closing any of the devices" ? I think you should expand this a bit to
> > explain that the open() call isn't valid on a device created this way.
> >
> >> + */
> >> +
> >> +/**
> >> + * \var V4L2M2MDevice::output
> >> + * \brief The output V4L2Device to send images to
> >> + */
> >> +
> >> +/**
> >> + * \var V4L2M2MDevice::capture
> >> + * \brief The capture V4L2Device to receive processed images from
> >> + */
> >> +
> >> +/**
> >> + * \brief Create a new V4L2M2MDevice from the \a deviceNode
> >> + *
> >> + * \return a new instantiation
> >
> > This should be expanded a bit.
> >
> >> + */
> >> +struct V4L2M2MDevice createM2MPair(const std::string &deviceNode)
> >> +{
> >> + V4L2M2MDevice m2m;
> >> + int ret;
> >> + int fd[2];
> >> +
> >> + /* Both V4L2Devices will have the same deviceNode */
> >
> > s/deviceNode/deviceNode.
>
> Done.
>
> >> + ret = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK);
> >
> > You can assign fd[0] directly instead of going through ret.
>
> Done.
>
> >> + if (ret < 0) {
> >> + ret = -errno;
> >> + LOG(V4L2, Error)
> >> + << "Failed to open V4L2 M2M device: " << strerror(-ret);
> >> + return m2m;
> >
> > m2m hasn't been initialised at this point. You should probably
> > initialise it when declaring the variable. If you do the same for fd
> > (initialising them both to -1) you could merge the two error labels
> > below.
>
> I've refactored to initialise first by creating the new V4L2VideoDevice
> instances as suggested below.
>
> On any error, the instances are deleted, and the pointers set to
> nullptr, simplifying the error path.
>
> >> + }
> >> + fd[0] = ret;
> >> +
> >> + ret = dup(fd[0]);
> >
> > And you can assign fd[1] here.
>
> Done.
>
> >> + if (ret < 0) {
> >> + ret = -errno;
> >> + LOG(V4L2, Error)
> >> + << "Failed to duplicate M2M device: " << strerror(-ret);
> >> +
> >> + close(fd[0]);
> >> +
> >> + goto err_dup;
> >
> > Double close ?
>
> Fixed.
>
> >> + }
> >> + fd[1] = ret;
> >> +
> >> + m2m.output = new V4L2Device(deviceNode);
> >> + m2m.capture = new V4L2Device(deviceNode);
> >
> > You could move this to the beginning of the function, avoiding the need
> > for initialising m2m to nullptr when declaring it.
>
> Done.
>
> >> +
> >> + ret = m2m.output->setFD(fd[0], V4L2M2MOutput);
> >> + if (ret)
> >> + goto err_device;
> >> +
> >> + ret = m2m.capture->setFD(fd[1], V4L2M2MCapture);
> >> + if (ret)
> >> + goto err_device;
> >> +
> >> + return m2m;
> >> +
> >> +err_device:
> >> + delete m2m.capture;
> >> + delete m2m.output;
> >> +
> >> + close(fd[1]);
> >> +err_dup:
> >> + close(fd[0]);
> >> +
> >> + m2m.capture = nullptr;
> >> + m2m.output = nullptr;
> >> +
> >> + return m2m;
> >> +}
> >> +
> >> } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list