Patch Detail
Show a patch.
GET /api/1.1/patches/1909/?format=api
{ "id": 1909, "url": "https://patchwork.libcamera.org/api/1.1/patches/1909/?format=api", "web_url": "https://patchwork.libcamera.org/patch/1909/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20190902041713.11229-1-paul.elder@ideasonboard.com>", "date": "2019-09-02T04:17:13", "name": "[libcamera-devel,v2] libcamera: device_enumerator: fix udev media graph loading dependency", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "35085a56920372586d048ada7acaf65445b9b2d1", "submitter": { "id": 17, "url": "https://patchwork.libcamera.org/api/1.1/people/17/?format=api", "name": "Paul Elder", "email": "paul.elder@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/1909/mbox/", "series": [ { "id": 477, "url": "https://patchwork.libcamera.org/api/1.1/series/477/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=477", "date": "2019-09-02T04:17:13", "name": "[libcamera-devel,v2] libcamera: device_enumerator: fix udev media graph loading dependency", "version": 2, "mbox": "https://patchwork.libcamera.org/series/477/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/1909/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/1909/checks/", "tags": {}, "headers": { "Return-Path": "<paul.elder@ideasonboard.com>", "Received": [ "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBAD160BB2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 2 Sep 2019 06:17:22 +0200 (CEST)", "from neptunite.amanokami.net (unknown [96.44.9.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3531126D;\n\tMon, 2 Sep 2019 06:17:22 +0200 (CEST)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567397842;\n\tbh=Ab/KXugn33JbSXo7M8CHGNEOin4zUXvA281+MDkPrac=;\n\th=From:To:Cc:Subject:Date:From;\n\tb=KQpNjBE/o2FN1XggfUeMLJ2plLVyl7lyKcrn+2UOdoS7GkC/ic3lTcw8PS0r6s+W0\n\tHppiaMVeesSfPGWHPNgAKInPlNkJqwkQa2v9i2Pn5P0ToMS3siJNEP9L37kxCkApSU\n\tzMUvwxxtQ5SlnBXctmZfRqH2puNAxDGmmj+SK15Y=", "From": "Paul Elder <paul.elder@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Mon, 2 Sep 2019 00:17:13 -0400", "Message-Id": "<20190902041713.11229-1-paul.elder@ideasonboard.com>", "X-Mailer": "git-send-email 2.20.1", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v2] libcamera: device_enumerator: fix udev\n\tmedia graph loading dependency", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.23", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "X-List-Received-Date": "Mon, 02 Sep 2019 04:17:23 -0000" }, "content": "When a MediaDevice is enumerated and populated by the\nDeviceEnumeratorUdev, there is a possibility that the member device\nnodes of the media graph would not be ready (either not created, or\nwithout proper permissions set by udev yet) The MediaDevice is still\npassed up to the pipeline handler, where an attempt to access the device\nnodes will fail in EPERM. This whole issue is especially likely to\nhappen when libcamera is run at system init time.\n\nTo fix this, we first split DeviceEnumerator::addDevice() into three\nmethods:\n- createDevice() to simply create the MediaDevice\n- populateMediaDevice() to populate the MediaDevice\n- addDevice() to pass the MediaDevice up to the pipeline handler\n\nDeviceEnumeratorSysfs calls these methods in succession, similar to what\nit did before when they were all together as addDevice().\n\nDeviceEnumeratorUdev additionally keeps a map of MediaDevices to a list\nof pending device nodes (plus some other auxillary maps), and a simple\nlist of orphan device nodes. If a v4l device node is ready and there\ndoes not exist any MediaDevice node for it, then it goes to the orphan\nlist, otherwise it is initialized and removed from the pending list of\nthe corresponding MediaDevice in the dependency map. When a MediaDevice\nis populated via DeviceEnumeratorUdev::populateMediaDevice(), it first\nchecks the orphan list to see if the device nodes it needs are there,\notherwise it tries to initialize the device nodes and if it fails, then\nit adds the device nodes it wants to its list in the dependency map.\n\nThis allows MediaDevice instances to be created and initialized properly\nwith udev when v4l device nodes in the media graph may not be ready when\nthe MediaDevice is populated.\n\nSigned-off-by: Paul Elder <paul.elder@ideasonboard.com>\n\n---\nChanges in v2:\n- add documentation\n- better name for the maps\n- other minor changes\n\ntodos:\n- do we need a better name for populateMediaDevice() ?\n\n---\n src/libcamera/device_enumerator.cpp | 52 +++----\n src/libcamera/device_enumerator_sysfs.cpp | 34 ++++-\n src/libcamera/device_enumerator_udev.cpp | 140 +++++++++++++++++-\n src/libcamera/include/device_enumerator.h | 3 +-\n .../include/device_enumerator_sysfs.h | 5 +-\n .../include/device_enumerator_udev.h | 16 ++\n 6 files changed, 218 insertions(+), 32 deletions(-)", "diff": "diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\nindex 60c918f..0290832 100644\n--- a/src/libcamera/device_enumerator.cpp\n+++ b/src/libcamera/device_enumerator.cpp\n@@ -195,16 +195,19 @@ DeviceEnumerator::~DeviceEnumerator()\n */\n \n /**\n- * \\brief Add a media device to the enumerator\n- * \\param[in] deviceNode path to the media device to add\n+ * \\brief Create a media device instance\n+ * \\param[in] deviceNode path to the media device to create\n *\n- * Create a media device for the \\a deviceNode, open it, populate its media graph,\n- * and look up device nodes associated with all entities. Store the media device\n- * in the internal list for later matching with pipeline handlers.\n+ * Create a media device for the \\a deviceNode, open it, and populate its\n+ * media graph. populateMediaDevice() should be called after this to look\n+ * up device nodes associated with all entities and to initialize them.\n+ * addDevice() shall be called with the media device returned from this\n+ * method, after populateMediaDevice() and after all devices nodes in the\n+ * media graph have been confirmed to be initialized.\n *\n- * \\return 0 on success or a negative error code otherwise\n+ * \\return Created media device instance on success, or nullptr otherwise\n */\n-int DeviceEnumerator::addDevice(const std::string &deviceNode)\n+std::shared_ptr<MediaDevice> DeviceEnumerator::createDevice(const std::string &deviceNode)\n {\n \tstd::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode);\n \n@@ -213,34 +216,31 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)\n \t\tLOG(DeviceEnumerator, Info)\n \t\t\t<< \"Unable to populate media device \" << deviceNode\n \t\t\t<< \" (\" << strerror(-ret) << \"), skipping\";\n-\t\treturn ret;\n+\t\treturn nullptr;\n \t}\n \n \tLOG(DeviceEnumerator, Debug)\n \t\t<< \"New media device \\\"\" << media->driver()\n \t\t<< \"\\\" created from \" << deviceNode;\n \n-\t/* Associate entities to device node paths. */\n-\tfor (MediaEntity *entity : media->entities()) {\n-\t\tif (entity->deviceMajor() == 0 && entity->deviceMinor() == 0)\n-\t\t\tcontinue;\n-\n-\t\tstd::string deviceNode = lookupDeviceNode(entity->deviceMajor(),\n-\t\t\t\t\t\t\t entity->deviceMinor());\n-\t\tif (deviceNode.empty())\n-\t\t\treturn -EINVAL;\n-\n-\t\tret = entity->setDeviceNode(deviceNode);\n-\t\tif (ret)\n-\t\t\treturn ret;\n-\t}\n+\treturn media;\n+}\n \n+/**\n+ * \\brief Add a media device to the enumerator\n+ * \\param[in] media media device instance to add\n+ *\n+ * Store the media device in the internal list for later matching with\n+ * pipeline handlers. \\a media shall be created with createDevice() first.\n+ * This method shall be called after all members of the members of the\n+ * media graph have been confirmed to be initialized.\n+ */\n+void DeviceEnumerator::addDevice(std::shared_ptr<MediaDevice> media)\n+{\n \tLOG(DeviceEnumerator, Debug)\n-\t\t<< \"Added device \" << deviceNode << \": \" << media->driver();\n-\n-\tdevices_.push_back(std::move(media));\n+\t\t<< \"Added device \" << media->deviceNode() << \": \" << media->driver();\n \n-\treturn 0;\n+\tdevices_.push_back(media);\n }\n \n /**\ndiff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp\nindex f3054d5..98299d7 100644\n--- a/src/libcamera/device_enumerator_sysfs.cpp\n+++ b/src/libcamera/device_enumerator_sysfs.cpp\n@@ -32,6 +32,7 @@ int DeviceEnumeratorSysfs::enumerate()\n {\n \tstruct dirent *ent;\n \tDIR *dir;\n+\tint ret = 0;\n \n \tstatic const char * const sysfs_dirs[] = {\n \t\t\"/sys/subsystem/media/devices\",\n@@ -71,11 +72,42 @@ int DeviceEnumeratorSysfs::enumerate()\n \t\t\tcontinue;\n \t\t}\n \n-\t\taddDevice(devnode);\n+\t\tstd::shared_ptr<MediaDevice> media = createDevice(devnode);\n+\t\tif (!media) {\n+\t\t\tret = -ENODEV;\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tif (populateMediaDevice(media) < 0) {\n+\t\t\tret = -ENODEV;\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\taddDevice(media);\n \t}\n \n \tclosedir(dir);\n \n+\treturn ret;\n+}\n+\n+int DeviceEnumeratorSysfs::populateMediaDevice(std::shared_ptr<MediaDevice> media)\n+{\n+\t/* Associate entities to device node paths. */\n+\tfor (MediaEntity *entity : media->entities()) {\n+\t\tif (entity->deviceMajor() == 0 && entity->deviceMinor() == 0)\n+\t\t\tcontinue;\n+\n+\t\tstd::string deviceNode = lookupDeviceNode(entity->deviceMajor(),\n+\t\t\t\t\t\t\t entity->deviceMinor());\n+\t\tif (deviceNode.empty())\n+\t\t\treturn -EINVAL;\n+\n+\t\tint ret = entity->setDeviceNode(deviceNode);\n+\t\tif (ret)\n+\t\t\treturn ret;\n+\t}\n+\n \treturn 0;\n }\n \ndiff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\nindex 86f6ca1..c89f152 100644\n--- a/src/libcamera/device_enumerator_udev.cpp\n+++ b/src/libcamera/device_enumerator_udev.cpp\n@@ -7,6 +7,10 @@\n \n #include \"device_enumerator_udev.h\"\n \n+#include <algorithm>\n+#include <list>\n+#include <map>\n+\n #include <fcntl.h>\n #include <libudev.h>\n #include <string.h>\n@@ -17,6 +21,7 @@\n #include <libcamera/event_notifier.h>\n \n #include \"log.h\"\n+#include \"media_device.h\"\n \n namespace libcamera {\n \n@@ -57,9 +62,40 @@ int DeviceEnumeratorUdev::init()\n \tif (ret < 0)\n \t\treturn ret;\n \n+\tret = udev_monitor_filter_add_match_subsystem_devtype(monitor_, \"video4linux\",\n+\t\t\t\t\t\t\t nullptr);\n+\tif (ret < 0)\n+\t\treturn ret;\n+\n \treturn 0;\n }\n \n+int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev)\n+{\n+\tconst char *subsystem = udev_device_get_subsystem(dev);\n+\tif (!subsystem)\n+\t\treturn -ENODEV;\n+\n+\tif (!strcmp(subsystem, \"media\")) {\n+\t\tstd::shared_ptr<MediaDevice> media =\n+\t\t\tcreateDevice(udev_device_get_devnode(dev));\n+\t\tif (!media)\n+\t\t\treturn -ENODEV;\n+\n+\t\tint ret = populateMediaDevice(media);\n+\t\tif (ret == 0)\n+\t\t\taddDevice(media);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!strcmp(subsystem, \"video4linux\")) {\n+\t\taddV4L2Device(udev_device_get_devnum(dev));\n+\t\treturn 0;\n+\t}\n+\n+\treturn -ENODEV;\n+}\n+\n int DeviceEnumeratorUdev::enumerate()\n {\n \tstruct udev_enumerate *udev_enum = nullptr;\n@@ -74,6 +110,14 @@ int DeviceEnumeratorUdev::enumerate()\n \tif (ret < 0)\n \t\tgoto done;\n \n+\tret = udev_enumerate_add_match_subsystem(udev_enum, \"video4linux\");\n+\tif (ret < 0)\n+\t\tgoto done;\n+\n+\tret = udev_enumerate_add_match_is_initialized(udev_enum);\n+\tif (ret < 0)\n+\t\tgoto done;\n+\n \tret = udev_enumerate_scan_devices(udev_enum);\n \tif (ret < 0)\n \t\tgoto done;\n@@ -102,10 +146,16 @@ int DeviceEnumeratorUdev::enumerate()\n \t\t\tgoto done;\n \t\t}\n \n-\t\taddDevice(devnode);\n+\t\tif (addUdevDevice(dev) < 0) {\n+\t\t\tudev_device_unref(dev);\n+\t\t\tret = -ENODEV;\n+\t\t\tgoto done;\n+\t\t}\n \n \t\tudev_device_unref(dev);\n+\t\tcontinue;\n \t}\n+\n done:\n \tudev_enumerate_unref(udev_enum);\n \tif (ret < 0)\n@@ -122,6 +172,48 @@ done:\n \treturn 0;\n }\n \n+int DeviceEnumeratorUdev::populateMediaDevice(std::shared_ptr<MediaDevice> media)\n+{\n+\tunsigned int pendingNodes = 0;\n+\tint ret;\n+\n+\t/* Associate entities to device node paths. */\n+\tfor (MediaEntity *entity : media->entities()) {\n+\t\tif (entity->deviceMajor() == 0 && entity->deviceMinor() == 0)\n+\t\t\tcontinue;\n+\n+\t\tstd::string deviceNode = lookupDeviceNode(entity->deviceMajor(),\n+\t\t\t\t\t\t\t entity->deviceMinor());\n+\t\tdev_t devnum = makedev(entity->deviceMajor(),\n+\t\t\t\t entity->deviceMinor());\n+\n+\t\t/* Take device from orphan list first, if it is in the list. */\n+\t\tif (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) {\n+\t\t\tif (deviceNode.empty())\n+\t\t\t\treturn -EINVAL;\n+\n+\t\t\tret = entity->setDeviceNode(deviceNode);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n+\n+\t\t\torphans_.remove(devnum);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (::access(deviceNode.c_str(), R_OK | W_OK) < 0) {\n+\t\t\tdeps_[media].push_back(devnum);\n+\t\t\tdevnumToDevice_[devnum] = media;\n+\t\t\tdevnumToEntity_[devnum] = entity;\n+\t\t\tpendingNodes++;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tentity->setDeviceNode(deviceNode);\n+\t}\n+\n+\treturn pendingNodes;\n+}\n+\n std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor)\n {\n \tstruct udev_device *device;\n@@ -143,6 +235,46 @@ std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor)\n \treturn deviceNode;\n }\n \n+/**\n+ * \\brief Add a V4L2 device to the media device that needs it\n+ * \\param[in] devnum major:minor number of V4L2 device to add, as a dev_t\n+ *\n+ * Add V4L2 device identified by \\a devnum to the MediaDevice that needs it,\n+ * if such a MediaDevice has been created. Otherwise add the V4L2 device to\n+ * the orphan list. If the V4L2 device is added to a MediaDevice, and it is\n+ * the last V4L2 device that the MediaDevice needs, then the MediaDevice\n+ * will be sent up to the pipeline handler.\n+ *\n+ * \\return 0 on success or a negative error code otherwise\n+ */\n+int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)\n+{\n+\tMediaEntity *entity = devnumToEntity_[devnum];\n+\tif (!entity) {\n+\t\torphans_.push_back(devnum);\n+\t\treturn 0;\n+\t}\n+\n+\tstd::string deviceNode = lookupDeviceNode(entity->deviceMajor(),\n+\t\t\t\t\t\t entity->deviceMinor());\n+\tif (deviceNode.empty())\n+\t\treturn -EINVAL;\n+\n+\tint ret = entity->setDeviceNode(deviceNode);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\tstd::shared_ptr<MediaDevice> media = devnumToDevice_[devnum];\n+\tdeps_[media].remove(devnum);\n+\tdevnumToDevice_.erase(devnum);\n+\tdevnumToEntity_.erase(devnum);\n+\n+\tif (deps_[media].empty())\n+\t\taddDevice(media);\n+\n+\treturn 0;\n+}\n+\n void DeviceEnumeratorUdev::udevNotify(EventNotifier *notifier)\n {\n \tstruct udev_device *dev = udev_monitor_receive_device(monitor_);\n@@ -153,9 +285,11 @@ void DeviceEnumeratorUdev::udevNotify(EventNotifier *notifier)\n \t\t<< action << \" device \" << udev_device_get_devnode(dev);\n \n \tif (action == \"add\") {\n-\t\taddDevice(deviceNode);\n+\t\taddUdevDevice(dev);\n \t} else if (action == \"remove\") {\n-\t\tremoveDevice(deviceNode);\n+\t\tconst char *subsystem = udev_device_get_subsystem(dev);\n+\t\tif (subsystem && !strcmp(subsystem, \"media\"))\n+\t\t\tremoveDevice(deviceNode);\n \t}\n \n \tudev_device_unref(dev);\ndiff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\nindex 02aec3b..b7d1b0a 100644\n--- a/src/libcamera/include/device_enumerator.h\n+++ b/src/libcamera/include/device_enumerator.h\n@@ -44,7 +44,8 @@ public:\n \tstd::shared_ptr<MediaDevice> search(const DeviceMatch &dm);\n \n protected:\n-\tint addDevice(const std::string &deviceNode);\n+\tstd::shared_ptr<MediaDevice> createDevice(const std::string &deviceNode);\n+\tvoid addDevice(std::shared_ptr<MediaDevice> media);\n \tvoid removeDevice(const std::string &deviceNode);\n \n private:\ndiff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h\nindex 8d3adc9..cc33f75 100644\n--- a/src/libcamera/include/device_enumerator_sysfs.h\n+++ b/src/libcamera/include/device_enumerator_sysfs.h\n@@ -9,6 +9,8 @@\n \n #include <string>\n \n+#include \"media_device.h\"\n+\n #include \"device_enumerator.h\"\n \n namespace libcamera {\n@@ -20,7 +22,8 @@ public:\n \tint enumerate();\n \n private:\n-\tstd::string lookupDeviceNode(int major, int minor);\n+\tint populateMediaDevice(std::shared_ptr<MediaDevice> media);\n+\tstd::string lookupDeviceNode(int major, int minor) final;\n };\n \n } /* namespace libcamera */\ndiff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h\nindex 80f9372..3f8af52 100644\n--- a/src/libcamera/include/device_enumerator_udev.h\n+++ b/src/libcamera/include/device_enumerator_udev.h\n@@ -7,8 +7,13 @@\n #ifndef __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__\n #define __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__\n \n+#include <list>\n+#include <map>\n #include <string>\n \n+#include <libudev.h>\n+#include <sys/types.h>\n+\n #include \"device_enumerator.h\"\n \n struct udev;\n@@ -17,6 +22,8 @@ struct udev_monitor;\n namespace libcamera {\n \n class EventNotifier;\n+class MediaDevice;\n+class MediaEntity;\n \n class DeviceEnumeratorUdev : public DeviceEnumerator\n {\n@@ -32,8 +39,17 @@ private:\n \tstruct udev_monitor *monitor_;\n \tEventNotifier *notifier_;\n \n+\tstd::map<std::shared_ptr<MediaDevice>, std::list<dev_t>> deps_;\n+\tstd::map<dev_t, std::shared_ptr<MediaDevice>> devnumToDevice_;\n+\tstd::map<dev_t, MediaEntity *> devnumToEntity_;\n+\n+\tstd::list<dev_t> orphans_;\n+\n+\tint addUdevDevice(struct udev_device *dev);\n+\tint populateMediaDevice(std::shared_ptr<MediaDevice> media);\n \tstd::string lookupDeviceNode(int major, int minor) final;\n \n+\tint addV4L2Device(dev_t devnum);\n \tvoid udevNotify(EventNotifier *notifier);\n };\n \n", "prefixes": [ "libcamera-devel", "v2" ] }