Patch Detail
Show a patch.
GET /api/1.1/patches/1879/?format=api
{ "id": 1879, "url": "https://patchwork.libcamera.org/api/1.1/patches/1879/?format=api", "web_url": "https://patchwork.libcamera.org/patch/1879/", "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": "<20190828034020.17365-1-paul.elder@ideasonboard.com>", "date": "2019-08-28T03:40:20", "name": "[libcamera-devel,RFC] libcamera: device_enumerator: fix udev media graph loading dependency", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "9be1e4b703ad981d70a07c9007c5b7ca65e22f23", "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/1879/mbox/", "series": [ { "id": 471, "url": "https://patchwork.libcamera.org/api/1.1/series/471/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=471", "date": "2019-08-28T03:40:20", "name": "[libcamera-devel,RFC] libcamera: device_enumerator: fix udev media graph loading dependency", "version": 1, "mbox": "https://patchwork.libcamera.org/series/471/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/1879/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/1879/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 38E1F60C18\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Aug 2019 05:40:33 +0200 (CEST)", "from neptunite.amanokami.net (unknown [96.44.9.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5120B310;\n\tWed, 28 Aug 2019 05:40:32 +0200 (CEST)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1566963632;\n\tbh=E6/pgX7XmBXxtc1VZOlbqZfTDtVUyidKS2+yG6RnbG4=;\n\th=From:To:Cc:Subject:Date:From;\n\tb=oo2uw3KfklNE+u0obzRvtr4XNMe17xagKgsLJBTLUFjIj/l7+6HdadWYJDXegOI1D\n\t9zoewu0Z5sotWXvBUg0pFNM6DeQT84UgSAKw54rvWrn9XAEwiQRU8C0LaM1eyVjfsG\n\tsW0qUUZ8hK0y7kKUcEpmrUCkY0r2X1wnSD74nSRY=", "From": "Paul Elder <paul.elder@ideasonboard.com>", "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", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [RFC PATCH] libcamera: device_enumerator: fix\n\tudev media 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": "Wed, 28 Aug 2019 03:40:33 -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. 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- (abstract method) 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 node it needs is there,\notherwise it tries to initialize the device node and if it fails, then\nit adds the device node it wants to its list in the dependency map.\n\nThis allows MediaDevices to be created and initialized properly with\nudev when v4l device nodes in the media graph may not be ready when the\nMediaDevice is populated.\n\nSigned-off-by: Paul Elder <paul.elder@ideasonboard.com>\n\n---\ntodos:\n- add documentation\n- better name for the maps\n- better name for populateMediaDevice()\n\n src/libcamera/device_enumerator.cpp | 27 +---\n src/libcamera/device_enumerator_sysfs.cpp | 33 +++-\n src/libcamera/device_enumerator_udev.cpp | 142 +++++++++++++++++-\n src/libcamera/include/device_enumerator.h | 4 +-\n .../include/device_enumerator_sysfs.h | 5 +-\n .../include/device_enumerator_udev.h | 14 ++\n 6 files changed, 197 insertions(+), 28 deletions(-)", "diff": "diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\nindex 60c918f..fd5a20c 100644\n--- a/src/libcamera/device_enumerator.cpp\n+++ b/src/libcamera/device_enumerator.cpp\n@@ -204,7 +204,7 @@ DeviceEnumerator::~DeviceEnumerator()\n *\n * \\return 0 on success or a negative error code 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,33 +213,22 @@ 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+int 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+\tdevices_.push_back(media);\n \treturn 0;\n }\n \ndiff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp\nindex f3054d5..3b5bc90 100644\n--- a/src/libcamera/device_enumerator_sysfs.cpp\n+++ b/src/libcamera/device_enumerator_sysfs.cpp\n@@ -71,7 +71,18 @@ 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\tclosedir(dir);\n+\t\t\treturn -ENODEV;\n+\t\t}\n+\n+\t\tif (populateMediaDevice(media) < 0) {\n+\t\t\tclosedir(dir);\n+\t\t\treturn -ENODEV;\n+\t\t}\n+\n+\t\taddDevice(media);\n \t}\n \n \tclosedir(dir);\n@@ -79,6 +90,26 @@ int DeviceEnumeratorSysfs::enumerate()\n \treturn 0;\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+\n std::string DeviceEnumeratorSysfs::lookupDeviceNode(int major, int minor)\n {\n \tstd::string deviceNode;\ndiff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\nindex 86f6ca1..ef9a31d 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@@ -57,6 +61,11 @@ 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@@ -74,6 +83,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@@ -85,6 +102,7 @@ int DeviceEnumeratorUdev::enumerate()\n \tudev_list_entry_foreach(ent, ents) {\n \t\tstruct udev_device *dev;\n \t\tconst char *devnode;\n+\t\tconst char *subsystem;\n \t\tconst char *syspath = udev_list_entry_get_name(ent);\n \n \t\tdev = udev_device_new_from_syspath(udev_, syspath);\n@@ -96,16 +114,36 @@ int DeviceEnumeratorUdev::enumerate()\n \t\t}\n \n \t\tdevnode = udev_device_get_devnode(dev);\n-\t\tif (!devnode) {\n-\t\t\tudev_device_unref(dev);\n-\t\t\tret = -ENODEV;\n-\t\t\tgoto done;\n+\t\tif (!devnode)\n+\t\t\tgoto unref;\n+\n+\t\tsubsystem = udev_device_get_subsystem(dev);\n+\t\tif (!subsystem)\n+\t\t\tgoto unref;\n+\n+\t\tif (!strcmp(subsystem, \"media\")) {\n+\t\t\tstd::shared_ptr<MediaDevice> media = createDevice(devnode);\n+\t\t\tif (!media)\n+\t\t\t\tgoto unref;\n+\n+\t\t\tret = populateMediaDevice(media);\n+\t\t\tif (ret == 0)\n+\t\t\t\taddDevice(media);\n+\t\t} else if (!strcmp(subsystem, \"video4linux\")) {\n+\t\t\taddV4L2Device(udev_device_get_devnum(dev));\n+\t\t} else {\n+\t\t\tgoto unref;\n \t\t}\n \n-\t\taddDevice(devnode);\n+\t\tudev_device_unref(dev);\n+\t\tcontinue;\n \n+\tunref:\n \t\tudev_device_unref(dev);\n+\t\tret = -ENODEV;\n+\t\tgoto done;\n \t}\n+\n done:\n \tudev_enumerate_unref(udev_enum);\n \tif (ret < 0)\n@@ -122,6 +160,49 @@ done:\n \treturn 0;\n }\n \n+int DeviceEnumeratorUdev::populateMediaDevice(std::shared_ptr<MediaDevice> media)\n+{\n+\t/* number of pending devnodes */\n+\tint count = 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 (deviceNode.empty() || (ret = ::access(deviceNode.c_str(), R_OK | W_OK)) < 0) {\n+\t\t\tmap_[media].push_back(devnum);\n+\t\t\trevMap_[devnum] = media;\n+\t\t\tdevnumToEntity_[devnum] = entity;\n+\t\t\tcount++;\n+\t\t}\n+\n+\t\tif (!ret)\n+\t\t\tentity->setDeviceNode(deviceNode);\n+\t}\n+\n+\treturn count;\n+}\n+\n std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor)\n {\n \tstruct udev_device *device;\n@@ -143,17 +224,66 @@ std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor)\n \treturn deviceNode;\n }\n \n+// add V4L2 device to the media device if it exists, otherwise to the orphan list\n+// if adding to media device, and it's the last one, then send up the media device\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 = revMap_[devnum];\n+\tmap_[media].remove(devnum);\n+\trevMap_.erase(devnum);\n+\tdevnumToEntity_.erase(devnum);\n+\n+\tif (map_[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 \tstd::string action(udev_device_get_action(dev));\n \tstd::string deviceNode(udev_device_get_devnode(dev));\n+\tdev_t deviceNum(udev_device_get_devnum(dev));\n \n \tLOG(DeviceEnumerator, Debug)\n \t\t<< action << \" device \" << udev_device_get_devnode(dev);\n \n \tif (action == \"add\") {\n-\t\taddDevice(deviceNode);\n+\t\tconst char *subsystem = udev_device_get_subsystem(dev);\n+\t\tif (!subsystem) {\n+\t\t\tudev_device_unref(dev);\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tif (!strcmp(subsystem, \"media\")) {\n+\t\t\tstd::shared_ptr<MediaDevice> media = createDevice(deviceNode);\n+\t\t\tif (!media) {\n+\t\t\t\tudev_device_unref(dev);\n+\t\t\t\treturn;\n+\t\t\t}\n+\n+\t\t\tint ret = populateMediaDevice(media);\n+\t\t\tif (ret == 0)\n+\t\t\t\taddDevice(media);\n+\t\t} else if (!strcmp(subsystem, \"video4linux\")) {\n+\t\t\taddV4L2Device(deviceNum);\n+\t\t}\n \t} else if (action == \"remove\") {\n \t\tremoveDevice(deviceNode);\n \t}\ndiff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\nindex 02aec3b..4c3e500 100644\n--- a/src/libcamera/include/device_enumerator.h\n+++ b/src/libcamera/include/device_enumerator.h\n@@ -44,12 +44,14 @@ 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+\tint addDevice(std::shared_ptr<MediaDevice> media);\n \tvoid removeDevice(const std::string &deviceNode);\n \n private:\n \tstd::vector<std::shared_ptr<MediaDevice>> devices_;\n \n+\tvirtual int populateMediaDevice(std::shared_ptr<MediaDevice>) = 0;\n \tvirtual std::string lookupDeviceNode(int major, int minor) = 0;\n };\n \ndiff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h\nindex 8d3adc9..d14bbf8 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) final;\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..b3bbcb0 100644\n--- a/src/libcamera/include/device_enumerator_udev.h\n+++ b/src/libcamera/include/device_enumerator_udev.h\n@@ -7,8 +7,14 @@\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 <sys/sysmacros.h>\n+\n+#include \"media_device.h\"\n+\n #include \"device_enumerator.h\"\n \n struct udev;\n@@ -32,8 +38,16 @@ private:\n \tstruct udev_monitor *monitor_;\n \tEventNotifier *notifier_;\n \n+\tstd::map<std::shared_ptr<MediaDevice>, std::list<dev_t>> map_;\n+\tstd::map<dev_t, std::shared_ptr<MediaDevice>> revMap_;\n+\tstd::map<dev_t, MediaEntity *> devnumToEntity_;\n+\n+\tstd::list<dev_t> orphans_;\n+\n+\tint populateMediaDevice(std::shared_ptr<MediaDevice> media) final;\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", "RFC" ] }