[libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Support M2M devices
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Aug 8 14:23:22 CEST 2019
Hi Laurent,
On 08/08/2019 12:33, Laurent Pinchart wrote:
> 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.
I think it's "ok" ... I've changed it for now. So lets try it.
>>>> 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.
If it was extended to do so - then the isM2M check could be removed.
I've done more refactoring - and ended up with a reasonable way to
refactor these into a single open() call in the end, I think I had
code-blindness before.
Dropping this check makes it more possible, so I'll just drop it if you
don't like it.
>> 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().
open() with overloaded arguments turns out to be a good name. I must be
stuck in 'C' and keep forgetting we can have functions with the same
name, (or as I have now set) optional arguments...
>
>>>> + 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 :-)
Ignore that - I've got a good way now I think.
>>>> +
>>>> + 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
--
Kieran
More information about the libcamera-devel
mailing list