| Message ID | 20190503153951.9037-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. 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@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/ ? 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. > 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 ? > + 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() ? > + > + fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable); > + fdEvent_->setEnabled(false); > + > + return 0; > +} > + > + > /** > * \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. > + ret = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK); You can assign fd[0] directly instead of going through ret. > + 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. > + } > + fd[0] = ret; > + > + ret = dup(fd[0]); And you can assign fd[1] here. > + if (ret < 0) { > + ret = -errno; > + LOG(V4L2, Error) > + << "Failed to duplicate M2M device: " << strerror(-ret); > + > + close(fd[0]); > + > + goto err_dup; Double close ? > + } > + 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. > + > + 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 */
Hi Laurent, On 04/05/2019 15:47, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > 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@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); ? 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? >> 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() 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 > >> + 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. >> + >> + 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 */ >
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@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 */
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@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 */ >
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); 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; + } + + 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; + } + + fdEvent_->activated.connect(this, &V4L2Device::bufferAvailable); + fdEvent_->setEnabled(false); + + return 0; +} + + /** * \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. + * Closing the device will require recreating a new pair. + */ + +/** + * \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 + */ +struct V4L2M2MDevice createM2MPair(const std::string &deviceNode) +{ + V4L2M2MDevice m2m; + int ret; + int fd[2]; + + /* Both V4L2Devices will have the same deviceNode */ + ret = ::open(deviceNode.c_str(), O_RDWR | O_NONBLOCK); + if (ret < 0) { + ret = -errno; + LOG(V4L2, Error) + << "Failed to open V4L2 M2M device: " << strerror(-ret); + return m2m; + } + fd[0] = ret; + + ret = dup(fd[0]); + if (ret < 0) { + ret = -errno; + LOG(V4L2, Error) + << "Failed to duplicate M2M device: " << strerror(-ret); + + close(fd[0]); + + goto err_dup; + } + fd[1] = ret; + + m2m.output = new V4L2Device(deviceNode); + m2m.capture = new V4L2Device(deviceNode); + + 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 */
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@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(+)