From patchwork Wed Aug 28 03:40:20 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Elder X-Patchwork-Id: 1879 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 38E1F60C18 for ; Wed, 28 Aug 2019 05:40:33 +0200 (CEST) Received: from neptunite.amanokami.net (unknown [96.44.9.94]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 5120B310; Wed, 28 Aug 2019 05:40:32 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1566963632; bh=E6/pgX7XmBXxtc1VZOlbqZfTDtVUyidKS2+yG6RnbG4=; h=From:To:Cc:Subject:Date:From; b=oo2uw3KfklNE+u0obzRvtr4XNMe17xagKgsLJBTLUFjIj/l7+6HdadWYJDXegOI1D 9zoewu0Z5sotWXvBUg0pFNM6DeQT84UgSAKw54rvWrn9XAEwiQRU8C0LaM1eyVjfsG sW0qUUZ8hK0y7kKUcEpmrUCkY0r2X1wnSD74nSRY= From: Paul Elder To: libcamera-devel@lists.libcamera.org Date: Tue, 27 Aug 2019 23:40:20 -0400 Message-Id: <20190828034020.17365-1-paul.elder@ideasonboard.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Subject: [libcamera-devel] [RFC PATCH] libcamera: device_enumerator: fix udev media graph loading dependency X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Aug 2019 03:40:33 -0000 When a MediaDevice is enumerated and populated by the DeviceEnumeratorUdev, there is a possibility that the member device nodes of the media graph would not be ready. The MediaDevice is still passed up to the pipeline handler, where an attempt to access the device nodes will fail in EPERM. This whole issue is especially likely to happen when libcamera is run at system init time. To fix this, we first split DeviceEnumerator::addDevice() into three methods: - createDevice() to simply create the MediaDevice - (abstract method) populateMediaDevice() to populate the MediaDevice - addDevice() to pass the MediaDevice up to the pipeline handler DeviceEnumeratorSysfs calls these methods in succession, similar to what it did before when they were all together as addDevice(). DeviceEnumeratorUdev additionally keeps a map of MediaDevices to a list of pending device nodes (plus some other auxillary maps), and a simple list of orphan device nodes. If a v4l device node is ready and there does not exist any MediaDevice node for it, then it goes to the orphan list, otherwise it is initialized and removed from the pending list of the corresponding MediaDevice in the dependency map. When a MediaDevice is populated via DeviceEnumeratorUdev::populateMediaDevice(), it first checks the orphan list to see if the device node it needs is there, otherwise it tries to initialize the device node and if it fails, then it adds the device node it wants to its list in the dependency map. This allows MediaDevices to be created and initialized properly with udev when v4l device nodes in the media graph may not be ready when the MediaDevice is populated. Signed-off-by: Paul Elder --- todos: - add documentation - better name for the maps - better name for populateMediaDevice() src/libcamera/device_enumerator.cpp | 27 +--- src/libcamera/device_enumerator_sysfs.cpp | 33 +++- src/libcamera/device_enumerator_udev.cpp | 142 +++++++++++++++++- src/libcamera/include/device_enumerator.h | 4 +- .../include/device_enumerator_sysfs.h | 5 +- .../include/device_enumerator_udev.h | 14 ++ 6 files changed, 197 insertions(+), 28 deletions(-) diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp index 60c918f..fd5a20c 100644 --- a/src/libcamera/device_enumerator.cpp +++ b/src/libcamera/device_enumerator.cpp @@ -204,7 +204,7 @@ DeviceEnumerator::~DeviceEnumerator() * * \return 0 on success or a negative error code otherwise */ -int DeviceEnumerator::addDevice(const std::string &deviceNode) +std::shared_ptr DeviceEnumerator::createDevice(const std::string &deviceNode) { std::shared_ptr media = std::make_shared(deviceNode); @@ -213,33 +213,22 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode) LOG(DeviceEnumerator, Info) << "Unable to populate media device " << deviceNode << " (" << strerror(-ret) << "), skipping"; - return ret; + return nullptr; } LOG(DeviceEnumerator, Debug) << "New media device \"" << media->driver() << "\" created from " << deviceNode; - /* Associate entities to device node paths. */ - for (MediaEntity *entity : media->entities()) { - if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) - continue; - - std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), - entity->deviceMinor()); - if (deviceNode.empty()) - return -EINVAL; - - ret = entity->setDeviceNode(deviceNode); - if (ret) - return ret; - } + return media; +} +int DeviceEnumerator::addDevice(std::shared_ptr media) +{ LOG(DeviceEnumerator, Debug) - << "Added device " << deviceNode << ": " << media->driver(); - - devices_.push_back(std::move(media)); + << "Added device " << media->deviceNode() << ": " << media->driver(); + devices_.push_back(media); return 0; } diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp index f3054d5..3b5bc90 100644 --- a/src/libcamera/device_enumerator_sysfs.cpp +++ b/src/libcamera/device_enumerator_sysfs.cpp @@ -71,7 +71,18 @@ int DeviceEnumeratorSysfs::enumerate() continue; } - addDevice(devnode); + std::shared_ptr media = createDevice(devnode); + if (!media) { + closedir(dir); + return -ENODEV; + } + + if (populateMediaDevice(media) < 0) { + closedir(dir); + return -ENODEV; + } + + addDevice(media); } closedir(dir); @@ -79,6 +90,26 @@ int DeviceEnumeratorSysfs::enumerate() return 0; } +int DeviceEnumeratorSysfs::populateMediaDevice(std::shared_ptr media) +{ + /* Associate entities to device node paths. */ + for (MediaEntity *entity : media->entities()) { + if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) + continue; + + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), + entity->deviceMinor()); + if (deviceNode.empty()) + return -EINVAL; + + int ret = entity->setDeviceNode(deviceNode); + if (ret) + return ret; + } + + return 0; +} + std::string DeviceEnumeratorSysfs::lookupDeviceNode(int major, int minor) { std::string deviceNode; diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp index 86f6ca1..ef9a31d 100644 --- a/src/libcamera/device_enumerator_udev.cpp +++ b/src/libcamera/device_enumerator_udev.cpp @@ -7,6 +7,10 @@ #include "device_enumerator_udev.h" +#include +#include +#include + #include #include #include @@ -57,6 +61,11 @@ int DeviceEnumeratorUdev::init() if (ret < 0) return ret; + ret = udev_monitor_filter_add_match_subsystem_devtype(monitor_, "video4linux", + nullptr); + if (ret < 0) + return ret; + return 0; } @@ -74,6 +83,14 @@ int DeviceEnumeratorUdev::enumerate() if (ret < 0) goto done; + ret = udev_enumerate_add_match_subsystem(udev_enum, "video4linux"); + if (ret < 0) + goto done; + + ret = udev_enumerate_add_match_is_initialized(udev_enum); + if (ret < 0) + goto done; + ret = udev_enumerate_scan_devices(udev_enum); if (ret < 0) goto done; @@ -85,6 +102,7 @@ int DeviceEnumeratorUdev::enumerate() udev_list_entry_foreach(ent, ents) { struct udev_device *dev; const char *devnode; + const char *subsystem; const char *syspath = udev_list_entry_get_name(ent); dev = udev_device_new_from_syspath(udev_, syspath); @@ -96,16 +114,36 @@ int DeviceEnumeratorUdev::enumerate() } devnode = udev_device_get_devnode(dev); - if (!devnode) { - udev_device_unref(dev); - ret = -ENODEV; - goto done; + if (!devnode) + goto unref; + + subsystem = udev_device_get_subsystem(dev); + if (!subsystem) + goto unref; + + if (!strcmp(subsystem, "media")) { + std::shared_ptr media = createDevice(devnode); + if (!media) + goto unref; + + ret = populateMediaDevice(media); + if (ret == 0) + addDevice(media); + } else if (!strcmp(subsystem, "video4linux")) { + addV4L2Device(udev_device_get_devnum(dev)); + } else { + goto unref; } - addDevice(devnode); + udev_device_unref(dev); + continue; + unref: udev_device_unref(dev); + ret = -ENODEV; + goto done; } + done: udev_enumerate_unref(udev_enum); if (ret < 0) @@ -122,6 +160,49 @@ done: return 0; } +int DeviceEnumeratorUdev::populateMediaDevice(std::shared_ptr media) +{ + /* number of pending devnodes */ + int count = 0; + int ret; + + /* Associate entities to device node paths. */ + for (MediaEntity *entity : media->entities()) { + if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) + continue; + + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), + entity->deviceMinor()); + dev_t devnum = makedev(entity->deviceMajor(), + entity->deviceMinor()); + + /* take device from orphan list first, if it is in the list */ + if (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) { + if (deviceNode.empty()) + return -EINVAL; + + ret = entity->setDeviceNode(deviceNode); + if (ret) + return ret; + + orphans_.remove(devnum); + continue; + } + + if (deviceNode.empty() || (ret = ::access(deviceNode.c_str(), R_OK | W_OK)) < 0) { + map_[media].push_back(devnum); + revMap_[devnum] = media; + devnumToEntity_[devnum] = entity; + count++; + } + + if (!ret) + entity->setDeviceNode(deviceNode); + } + + return count; +} + std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor) { struct udev_device *device; @@ -143,17 +224,66 @@ std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor) return deviceNode; } +// add V4L2 device to the media device if it exists, otherwise to the orphan list +// if adding to media device, and it's the last one, then send up the media device +int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) +{ + MediaEntity *entity = devnumToEntity_[devnum]; + if (!entity) { + orphans_.push_back(devnum); + return 0; + } + + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), + entity->deviceMinor()); + if (deviceNode.empty()) + return -EINVAL; + + int ret = entity->setDeviceNode(deviceNode); + if (ret) + return ret; + + std::shared_ptr media = revMap_[devnum]; + map_[media].remove(devnum); + revMap_.erase(devnum); + devnumToEntity_.erase(devnum); + + if (map_[media].empty()) + addDevice(media); + + return 0; +} + void DeviceEnumeratorUdev::udevNotify(EventNotifier *notifier) { struct udev_device *dev = udev_monitor_receive_device(monitor_); std::string action(udev_device_get_action(dev)); std::string deviceNode(udev_device_get_devnode(dev)); + dev_t deviceNum(udev_device_get_devnum(dev)); LOG(DeviceEnumerator, Debug) << action << " device " << udev_device_get_devnode(dev); if (action == "add") { - addDevice(deviceNode); + const char *subsystem = udev_device_get_subsystem(dev); + if (!subsystem) { + udev_device_unref(dev); + return; + } + + if (!strcmp(subsystem, "media")) { + std::shared_ptr media = createDevice(deviceNode); + if (!media) { + udev_device_unref(dev); + return; + } + + int ret = populateMediaDevice(media); + if (ret == 0) + addDevice(media); + } else if (!strcmp(subsystem, "video4linux")) { + addV4L2Device(deviceNum); + } } else if (action == "remove") { removeDevice(deviceNode); } diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h index 02aec3b..4c3e500 100644 --- a/src/libcamera/include/device_enumerator.h +++ b/src/libcamera/include/device_enumerator.h @@ -44,12 +44,14 @@ public: std::shared_ptr search(const DeviceMatch &dm); protected: - int addDevice(const std::string &deviceNode); + std::shared_ptr createDevice(const std::string &deviceNode); + int addDevice(std::shared_ptr media); void removeDevice(const std::string &deviceNode); private: std::vector> devices_; + virtual int populateMediaDevice(std::shared_ptr) = 0; virtual std::string lookupDeviceNode(int major, int minor) = 0; }; diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h index 8d3adc9..d14bbf8 100644 --- a/src/libcamera/include/device_enumerator_sysfs.h +++ b/src/libcamera/include/device_enumerator_sysfs.h @@ -9,6 +9,8 @@ #include +#include "media_device.h" + #include "device_enumerator.h" namespace libcamera { @@ -20,7 +22,8 @@ public: int enumerate(); private: - std::string lookupDeviceNode(int major, int minor); + int populateMediaDevice(std::shared_ptr media) final; + std::string lookupDeviceNode(int major, int minor) final; }; } /* namespace libcamera */ diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h index 80f9372..b3bbcb0 100644 --- a/src/libcamera/include/device_enumerator_udev.h +++ b/src/libcamera/include/device_enumerator_udev.h @@ -7,8 +7,14 @@ #ifndef __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__ #define __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__ +#include +#include #include +#include + +#include "media_device.h" + #include "device_enumerator.h" struct udev; @@ -32,8 +38,16 @@ private: struct udev_monitor *monitor_; EventNotifier *notifier_; + std::map, std::list> map_; + std::map> revMap_; + std::map devnumToEntity_; + + std::list orphans_; + + int populateMediaDevice(std::shared_ptr media) final; std::string lookupDeviceNode(int major, int minor) final; + int addV4L2Device(dev_t devnum); void udevNotify(EventNotifier *notifier); };