[{"id":2511,"web_url":"https://patchwork.libcamera.org/comment/2511/","msgid":"<20190828100201.GB27842@pendragon.ideasonboard.com>","date":"2019-08-28T10:02:01","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: device_enumerator: fix\n\tudev media graph loading dependency","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Aug 27, 2019 at 11:40:20PM -0400, Paul Elder wrote:\n> When a MediaDevice is enumerated and populated by the\n> DeviceEnumeratorUdev, there is a possibility that the member device\n> nodes of the media graph would not be ready. The MediaDevice is still\n\nI would detail this with\n\n\"... would not be ready (either not created, or without proper\npermissions set by udev yet).\"\n\n> passed up to the pipeline handler, where an attempt to access the device\n> nodes will fail in EPERM. This whole issue is especially likely to\n> happen when libcamera is run at system init time.\n> \n> To fix this, we first split DeviceEnumerator::addDevice() into three\n> methods:\n> - createDevice() to simply create the MediaDevice\n> - (abstract method) populateMediaDevice() to populate the MediaDevice\n\nAs populateMediaDevice() isn't called by the base DeviceEnumerator\nclass, you don't need to make it a virtual method, and it can be defined\nin the derived classes. Another option would be to move the\nimplementation of DeviceEnumeratorSysfs::populateMediaDevice() to the\nDeviceEnumerator class as that code could possibly be reused, but we\ndon't have plan for a new device enumerator at the moment, so I wouldn't\nattempt premature code sharing.\n\n> - addDevice() to pass the MediaDevice up to the pipeline handler\n> \n> DeviceEnumeratorSysfs calls these methods in succession, similar to what\n> it did before when they were all together as addDevice().\n> \n> DeviceEnumeratorUdev additionally keeps a map of MediaDevices to a list\n> of pending device nodes (plus some other auxillary maps), and a simple\n> list of orphan device nodes. If a v4l device node is ready and there\n> does not exist any MediaDevice node for it, then it goes to the orphan\n> list, otherwise it is initialized and removed from the pending list of\n> the corresponding MediaDevice in the dependency map. When a MediaDevice\n> is populated via DeviceEnumeratorUdev::populateMediaDevice(), it first\n> checks the orphan list to see if the device node it needs is there,\n\ns/device node/device nodes/ and plural for the rest of the sentence ?\n\n> otherwise it tries to initialize the device node and if it fails, then\n> it adds the device node it wants to its list in the dependency map.\n> \n> This allows MediaDevices to be created and initialized properly with\n\ns/MediaDevices/media devices/ (or MediaDevice instances)\n\n> udev when v4l device nodes in the media graph may not be ready when the\n> MediaDevice is populated.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> todos:\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(-)\n> \n> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> index 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 DeviceEnumerator::createDevice(const std::string &deviceNode)\n> {\n> \tstd::shared_ptr media = std::make_shared(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 media)\n\nAs this method never fails, and as you never check the return value,\nlet's make it void.\n\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> \n> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp\n> index 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 media = createDevice(devnode);\n> +\t\tif (!media) {\n> +\t\t\tclosedir(dir);\n\nInstead of duplicating closedir() calls, how about\n\n\t\t\tret == -ENODEV;\n\t\t\tbreak;\n\nand a return ret at the end of the method ? You will need to initialise\nret to 0 when declaring it at the top.\n\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 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;\n> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\n> index 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 \n> +#include \n> +#include \n> +\n> #include \n> #include \n> #include \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\nCan this happen ?\n\n> +\n> +\t\tif (!strcmp(subsystem, \"media\")) {\n> +\t\t\tstd::shared_ptr 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\nThis isn't nice, and may call for moving part of the code in the loop to\nseparate method. Especially as there's code duplication with\nDeviceEnumeratorUdev::udevNotify().\n\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 media)\n> +{\n> +\t/* number of pending devnodes */\n\nInstead of requiring a comment, how about renaming the variable to make\nits purpose explicit ?\n\n> +\tint count = 0;\n\ncount is never negative, you can make it an unsigned int.\n\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\nLet's not attempt a lookup of the device node before we locate the\ndevice in the orphans list, as this may fail.\n\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\nAssigning variables inside conditions is frowned upon. And I don't think\nthere's a need to test ::access() here, as if udev hasn't notified us of\nthe device being ready, there's little point in proceding. There's\nalready an access() test in MediaEntity::setDeviceNode().\n\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\nIf deviceNode is empty, and no device has been previously found in the\norphaned list, ret will be used uninitialised.\n\n> +\t\t\tentity->setDeviceNode(deviceNode);\n\nI think you can just drop this, the setDeviceNode() call when the device\nis found in the orphaned list should be enough.\n\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 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 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\nAs populateMediaDevice() is always followed by addDevice(), should you\ncall the latter inside the former ?\n\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\n\nShould this be conditioned to subsystem == media ?\n\n> \t}\n> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\n> index 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 search(const DeviceMatch &dm);\n> \n> protected:\n> -\tint addDevice(const std::string &deviceNode);\n> +\tstd::shared_ptr createDevice(const std::string &deviceNode);\n> +\tint addDevice(std::shared_ptr media);\n\nYou can pass a const reference to the shared pointer to avoid a copy of\nthe shared pointer itself (the MediaDevice it points to is of course not\ncopied). Same comment for populateMediaDevice() method.\n\n> \tvoid removeDevice(const std::string &deviceNode);\n> \n> private:\n> \tstd::vector> devices_;\n> \n> +\tvirtual int populateMediaDevice(std::shared_ptr) = 0;\n\nPlease name the arguments.\n\n> \tvirtual std::string lookupDeviceNode(int major, int minor) = 0;\n> };\n> \n> diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h\n> index 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 \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 media) final;\n\nShould we make the class itself final instead of marking every single\nmethod ? Same comment for DeviceEnumeratorUdev.\n\n> +\tstd::string lookupDeviceNode(int major, int minor) final;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h\n> index 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 \n> +#include \n> #include \n> \n> +#include \n\nThis header is only needed in the .cpp. Here you can just include\nsys/types.h for dev_t.\n\n> +\n> +#include \"media_device.h\"\n\nA forward declaration of class MediaDevice should be enough.\n\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::list> map_;\n> +\tstd::map> revMap_;\n> +\tstd::map devnumToEntity_;\n> +\n> +\tstd::list orphans_;\n\nThat's quite a few maps and lists, they indeed need better names and/or\ndocumentation. I think you could also merge the revMap_ and\ndevnumToEntity_ maps if you made it a\nstd::map, MediaEntity *> (a bit\nof a long type though).\n\nI also wonder if we really need to deal with shared pointers internally,\nor if we could create the shared pointer in the addDevice() method. This\ncould be done on top of this patch, but please keep it in mind, I'd like\nyour opinion.\n\n> +\n> +\tint populateMediaDevice(std::shared_ptr 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>","headers":{"Return-Path":"","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 76CB960BF6\n\tfor ;\n\tWed, 28 Aug 2019 12:02:09 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D665C310;\n\tWed, 28 Aug 2019 12:02:08 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1566986529;\n\tbh=SSY05PPEK+iCQMZCdZzkdJ16ii5uWew/xHeHV5PEEqk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rr6gIrZyHHKjr/nnDySL2tzHZTDmxJRadm7hmPtXDi6mwP0xDolZ5z+ucdLxWxcEW\n\t+sIn0jRIwv6itK3WYDsR8SMmCA8Mq5xis8mKQDQCaq7KGpsrwmitzwSlLkIDNlfn0H\n\tbL9+MvwW5cWe07ZVj6crB+ZgjljTW9BYYBmxuzl8=","Date":"Wed, 28 Aug 2019 13:02:01 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190828100201.GB27842@pendragon.ideasonboard.com>","References":"<20190828034020.17365-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190828034020.17365-1-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 28 Aug 2019 10:02:09 -0000"}},{"id":2567,"web_url":"https://patchwork.libcamera.org/comment/2567/","msgid":"<20190902041517.GL4818@localhost.localdomain>","date":"2019-09-02T04:15:17","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: device_enumerator: fix\n\tudev media graph loading dependency","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Wed, Aug 28, 2019 at 01:02:01PM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n\nThank you for the review.\n\n> On Tue, Aug 27, 2019 at 11:40:20PM -0400, Paul Elder wrote:\n> > When a MediaDevice is enumerated and populated by the\n> > DeviceEnumeratorUdev, there is a possibility that the member device\n> > nodes of the media graph would not be ready. The MediaDevice is still\n> \n> I would detail this with\n> \n> \"... would not be ready (either not created, or without proper\n> permissions set by udev yet).\"\n> \n> > passed up to the pipeline handler, where an attempt to access the device\n> > nodes will fail in EPERM. This whole issue is especially likely to\n> > happen when libcamera is run at system init time.\n> > \n> > To fix this, we first split DeviceEnumerator::addDevice() into three\n> > methods:\n> > - createDevice() to simply create the MediaDevice\n> > - (abstract method) populateMediaDevice() to populate the MediaDevice\n> \n> As populateMediaDevice() isn't called by the base DeviceEnumerator\n> class, you don't need to make it a virtual method, and it can be defined\n> in the derived classes. Another option would be to move the\n> implementation of DeviceEnumeratorSysfs::populateMediaDevice() to the\n> DeviceEnumerator class as that code could possibly be reused, but we\n> don't have plan for a new device enumerator at the moment, so I wouldn't\n> attempt premature code sharing.\n> \n> > - addDevice() to pass the MediaDevice up to the pipeline handler\n> > \n> > DeviceEnumeratorSysfs calls these methods in succession, similar to what\n> > it did before when they were all together as addDevice().\n> > \n> > DeviceEnumeratorUdev additionally keeps a map of MediaDevices to a list\n> > of pending device nodes (plus some other auxillary maps), and a simple\n> > list of orphan device nodes. If a v4l device node is ready and there\n> > does not exist any MediaDevice node for it, then it goes to the orphan\n> > list, otherwise it is initialized and removed from the pending list of\n> > the corresponding MediaDevice in the dependency map. When a MediaDevice\n> > is populated via DeviceEnumeratorUdev::populateMediaDevice(), it first\n> > checks the orphan list to see if the device node it needs is there,\n> \n> s/device node/device nodes/ and plural for the rest of the sentence ?\n> \n> > otherwise it tries to initialize the device node and if it fails, then\n> > it adds the device node it wants to its list in the dependency map.\n> > \n> > This allows MediaDevices to be created and initialized properly with\n> \n> s/MediaDevices/media devices/ (or MediaDevice instances)\n> \n> > udev when v4l device nodes in the media graph may not be ready when the\n> > MediaDevice is populated.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > todos:\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(-)\n> > \n> > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> > index 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 DeviceEnumerator::createDevice(const std::string &deviceNode)\n> > {\n> > \tstd::shared_ptr media = std::make_shared(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 media)\n> \n> As this method never fails, and as you never check the return value,\n> let's make it void.\n> \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> > \n> > diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp\n> > index 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 media = createDevice(devnode);\n> > +\t\tif (!media) {\n> > +\t\t\tclosedir(dir);\n> \n> Instead of duplicating closedir() calls, how about\n> \n> \t\t\tret == -ENODEV;\n> \t\t\tbreak;\n> \n> and a return ret at the end of the method ? You will need to initialise\n> ret to 0 when declaring it at the top.\n> \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 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;\n> > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\n> > index 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 \n> > +#include \n> > +#include \n> > +\n> > #include \n> > #include \n> > #include \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> Can this happen ?\n\ntbh, I'm not sure. But the documentation for udev_device_get_subsystem()\nsays that it can return NULL, and I don't think we want to continue if\nit ever does.\n\n> > +\n> > +\t\tif (!strcmp(subsystem, \"media\")) {\n> > +\t\t\tstd::shared_ptr 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> \n> This isn't nice, and may call for moving part of the code in the loop to\n> separate method. Especially as there's code duplication with\n> DeviceEnumeratorUdev::udevNotify().\n\nI split that into a separate method, and just expanded this error\nhandling. See v2 (coming soon!).\n\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 media)\n> > +{\n> > +\t/* number of pending devnodes */\n> \n> Instead of requiring a comment, how about renaming the variable to make\n> its purpose explicit ?\n> \n> > +\tint count = 0;\n> \n> count is never negative, you can make it an unsigned int.\n> \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> \n> Let's not attempt a lookup of the device node before we locate the\n> device in the orphans list, as this may fail.\n\nIt's possible for the device to be ready and not be in the orphans list,\nso I think it's okay to keep this here. If it fails then it'll be an\nempty string, and we check for that anyway.\n\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> \n> Assigning variables inside conditions is frowned upon. And I don't think\n> there's a need to test ::access() here, as if udev hasn't notified us of\n> the device being ready, there's little point in proceding. There's\n> already an access() test in MediaEntity::setDeviceNode().\n\nI did call and check the return value of setDeviceNode(), but then\nthere will be a bunch of misleading ERROR log messages complaining about\nEPERM, so I think it's better to test ::access() here instead.\n\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> \n> If deviceNode is empty, and no device has been previously found in the\n> orphaned list, ret will be used uninitialised.\n> \n> > +\t\t\tentity->setDeviceNode(deviceNode);\n> \n> I think you can just drop this, the setDeviceNode() call when the device\n> is found in the orphaned list should be enough.\n\nI'm going to keep this, to cover the case that a device is ready but not\nin the orphans list.\n\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 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 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> \n> As populateMediaDevice() is always followed by addDevice(), should you\n> call the latter inside the former ?\n\nI don't think addDevice() belongs in populateMediaDevice().\n\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> \n> \n> Should this be conditioned to subsystem == media ?\n\nYes.\n\n> > \t}\n> > diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\n> > index 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 search(const DeviceMatch &dm);\n> > \n> > protected:\n> > -\tint addDevice(const std::string &deviceNode);\n> > +\tstd::shared_ptr createDevice(const std::string &deviceNode);\n> > +\tint addDevice(std::shared_ptr media);\n> \n> You can pass a const reference to the shared pointer to avoid a copy of\n> the shared pointer itself (the MediaDevice it points to is of course not\n> copied). Same comment for populateMediaDevice() method.\n\naddDevice() calls devices_.push_back(), which wants a shared pointer. I\nthink it's easier to keep it as a shared pointer. populateMediaDevice()\ndoesn't have this issue, but I think keeping them all uniform makes life\neasier.\n\n> > \tvoid removeDevice(const std::string &deviceNode);\n> > \n> > private:\n> > \tstd::vector> devices_;\n> > \n> > +\tvirtual int populateMediaDevice(std::shared_ptr) = 0;\n> \n> Please name the arguments.\n> \n> > \tvirtual std::string lookupDeviceNode(int major, int minor) = 0;\n> > };\n> > \n> > diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h\n> > index 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 \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 media) final;\n> \n> Should we make the class itself final instead of marking every single\n> method ? Same comment for DeviceEnumeratorUdev.\n\npopulateMediaDevice() is now no longer part of DeviceEnumerator.\nOtherwise, I'm not sure.\n\n> > +\tstd::string lookupDeviceNode(int major, int minor) final;\n> > };\n> > \n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h\n> > index 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 \n> > +#include \n> > #include \n> > \n> > +#include \n> \n> This header is only needed in the .cpp. Here you can just include\n> sys/types.h for dev_t.\n> \n> > +\n> > +#include \"media_device.h\"\n> \n> A forward declaration of class MediaDevice should be enough.\n\nI needed class MediaEntity too.\n\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::list> map_;\n> > +\tstd::map> revMap_;\n> > +\tstd::map devnumToEntity_;\n> > +\n> > +\tstd::list orphans_;\n> \n> That's quite a few maps and lists, they indeed need better names and/or\n> documentation. I think you could also merge the revMap_ and\n> devnumToEntity_ maps if you made it a\n> std::map, MediaEntity *> (a bit\n> of a long type though).\n\nI decided to keep the maps separate. With nicer names (deps_,\ndevnumToDevice_) it ought to be easier to follow.\n\n> I also wonder if we really need to deal with shared pointers internally,\n> or if we could create the shared pointer in the addDevice() method. This\n> could be done on top of this patch, but please keep it in mind, I'd like\n> your opinion.\n\nI think it's easier to keep them as shared pointers. For one, what I\nmentioned earlier, that addDevice() needs share pointers anyway. Another\nis that addV4L2Device(), which is one of the main users of these maps,\ncalls addDevice().\n\n> > +\n> > +\tint populateMediaDevice(std::shared_ptr 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\nThanks,\n\nPaul","headers":{"Return-Path":"","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 7BA0260BB2\n\tfor ;\n\tMon, 2 Sep 2019 06:15:25 +0200 (CEST)","from localhost.localdomain (unknown [96.44.9.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A7E9B26D;\n\tMon, 2 Sep 2019 06:15:24 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567397725;\n\tbh=PhyzYWU/vl4DJe+CxzwoP95J0+2nlRfbbVCiVYj8aw8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d4pVHi7ZIbmw7fJtnXR50yMYXS7ZSou7eeT/J0mcyXJKXNJRGCb+NA2jr6wHIZhBX\n\tEaLuYqgO10xTB9qZVPONtrn70i2eqHvYoU2urhiozLyzmpFfb9PqSmKxQQswlUeHBe\n\t+EM08W+iPLvWJ0i1gT8KvYw2DDYPgjSZ+t8wXVhE=","Date":"Mon, 2 Sep 2019 00:15:17 -0400","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190902041517.GL4818@localhost.localdomain>","References":"<20190828034020.17365-1-paul.elder@ideasonboard.com>\n\t<20190828100201.GB27842@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20190828100201.GB27842@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 02 Sep 2019 04:15:25 -0000"}},{"id":2568,"web_url":"https://patchwork.libcamera.org/comment/2568/","msgid":"<20190902075535.GB4777@pendragon.ideasonboard.com>","date":"2019-09-02T07:55:35","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: device_enumerator: fix\n\tudev media graph loading dependency","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Mon, Sep 02, 2019 at 12:15:17AM -0400, Paul Elder wrote:\n> On Wed, Aug 28, 2019 at 01:02:01PM +0300, Laurent Pinchart wrote:\n> > On Tue, Aug 27, 2019 at 11:40:20PM -0400, Paul Elder wrote:\n> >> When a MediaDevice is enumerated and populated by the\n> >> DeviceEnumeratorUdev, there is a possibility that the member device\n> >> nodes of the media graph would not be ready. The MediaDevice is still\n> > \n> > I would detail this with\n> > \n> > \"... would not be ready (either not created, or without proper\n> > permissions set by udev yet).\"\n> > \n> >> passed up to the pipeline handler, where an attempt to access the device\n> >> nodes will fail in EPERM. This whole issue is especially likely to\n> >> happen when libcamera is run at system init time.\n> >> \n> >> To fix this, we first split DeviceEnumerator::addDevice() into three\n> >> methods:\n> >> - createDevice() to simply create the MediaDevice\n> >> - (abstract method) populateMediaDevice() to populate the MediaDevice\n> > \n> > As populateMediaDevice() isn't called by the base DeviceEnumerator\n> > class, you don't need to make it a virtual method, and it can be defined\n> > in the derived classes. Another option would be to move the\n> > implementation of DeviceEnumeratorSysfs::populateMediaDevice() to the\n> > DeviceEnumerator class as that code could possibly be reused, but we\n> > don't have plan for a new device enumerator at the moment, so I wouldn't\n> > attempt premature code sharing.\n> > \n> >> - addDevice() to pass the MediaDevice up to the pipeline handler\n> >> \n> >> DeviceEnumeratorSysfs calls these methods in succession, similar to what\n> >> it did before when they were all together as addDevice().\n> >> \n> >> DeviceEnumeratorUdev additionally keeps a map of MediaDevices to a list\n> >> of pending device nodes (plus some other auxillary maps), and a simple\n> >> list of orphan device nodes. If a v4l device node is ready and there\n> >> does not exist any MediaDevice node for it, then it goes to the orphan\n> >> list, otherwise it is initialized and removed from the pending list of\n> >> the corresponding MediaDevice in the dependency map. When a MediaDevice\n> >> is populated via DeviceEnumeratorUdev::populateMediaDevice(), it first\n> >> checks the orphan list to see if the device node it needs is there,\n> > \n> > s/device node/device nodes/ and plural for the rest of the sentence ?\n> > \n> >> otherwise it tries to initialize the device node and if it fails, then\n> >> it adds the device node it wants to its list in the dependency map.\n> >> \n> >> This allows MediaDevices to be created and initialized properly with\n> > \n> > s/MediaDevices/media devices/ (or MediaDevice instances)\n> > \n> >> udev when v4l device nodes in the media graph may not be ready when the\n> >> MediaDevice is populated.\n> >> \n> >> Signed-off-by: Paul Elder \n> >> \n> >> ---\n> >> todos:\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(-)\n> >> \n> >> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> >> index 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 DeviceEnumerator::createDevice(const std::string &deviceNode)\n> >> {\n> >> \tstd::shared_ptr media = std::make_shared(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 media)\n> > \n> > As this method never fails, and as you never check the return value,\n> > let's make it void.\n> > \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> >> \n> >> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp\n> >> index 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 media = createDevice(devnode);\n> >> +\t\tif (!media) {\n> >> +\t\t\tclosedir(dir);\n> > \n> > Instead of duplicating closedir() calls, how about\n> > \n> > \t\t\tret == -ENODEV;\n> > \t\t\tbreak;\n> > \n> > and a return ret at the end of the method ? You will need to initialise\n> > ret to 0 when declaring it at the top.\n> > \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 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;\n> >> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp\n> >> index 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 \n> >> +#include \n> >> +#include \n> >> +\n> >> #include \n> >> #include \n> >> #include \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> > Can this happen ?\n> \n> tbh, I'm not sure. But the documentation for udev_device_get_subsystem()\n> says that it can return NULL, and I don't think we want to continue if\n> it ever does.\n\nOK, let's keep the check.\n\n> >> +\n> >> +\t\tif (!strcmp(subsystem, \"media\")) {\n> >> +\t\t\tstd::shared_ptr 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> > \n> > This isn't nice, and may call for moving part of the code in the loop to\n> > separate method. Especially as there's code duplication with\n> > DeviceEnumeratorUdev::udevNotify().\n> \n> I split that into a separate method, and just expanded this error\n> handling. See v2 (coming soon!).\n> \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 media)\n> >> +{\n> >> +\t/* number of pending devnodes */\n> > \n> > Instead of requiring a comment, how about renaming the variable to make\n> > its purpose explicit ?\n> > \n> >> +\tint count = 0;\n> > \n> > count is never negative, you can make it an unsigned int.\n> > \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> > \n> > Let's not attempt a lookup of the device node before we locate the\n> > device in the orphans list, as this may fail.\n> \n> It's possible for the device to be ready and not be in the orphans list,\n> so I think it's okay to keep this here. If it fails then it'll be an\n> empty string, and we check for that anyway.\n> \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> > \n> > Assigning variables inside conditions is frowned upon. And I don't think\n> > there's a need to test ::access() here, as if udev hasn't notified us of\n> > the device being ready, there's little point in proceding. There's\n> > already an access() test in MediaEntity::setDeviceNode().\n> \n> I did call and check the return value of setDeviceNode(), but then\n> there will be a bunch of misleading ERROR log messages complaining about\n> EPERM, so I think it's better to test ::access() here instead.\n> \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> > \n> > If deviceNode is empty, and no device has been previously found in the\n> > orphaned list, ret will be used uninitialised.\n> > \n> >> +\t\t\tentity->setDeviceNode(deviceNode);\n> > \n> > I think you can just drop this, the setDeviceNode() call when the device\n> > is found in the orphaned list should be enough.\n> \n> I'm going to keep this, to cover the case that a device is ready but not\n> in the orphans list.\n\nI don't think we need to. It can indeed happen, but udev will notify us\nof the device being ready right after anyway. Relying on that will\nsimplify the code.\n\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 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 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> > \n> > As populateMediaDevice() is always followed by addDevice(), should you\n> > call the latter inside the former ?\n> \n> I don't think addDevice() belongs in populateMediaDevice().\n\nNot if the method is named populateMediaDevice(), but maybe we could\ncombine and rename them ?\n\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> > \n> > \n> > Should this be conditioned to subsystem == media ?\n> \n> Yes.\n> \n> >> \t}\n> >> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\n> >> index 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 search(const DeviceMatch &dm);\n> >> \n> >> protected:\n> >> -\tint addDevice(const std::string &deviceNode);\n> >> +\tstd::shared_ptr createDevice(const std::string &deviceNode);\n> >> +\tint addDevice(std::shared_ptr media);\n> > \n> > You can pass a const reference to the shared pointer to avoid a copy of\n> > the shared pointer itself (the MediaDevice it points to is of course not\n> > copied). Same comment for populateMediaDevice() method.\n> \n> addDevice() calls devices_.push_back(), which wants a shared pointer.\n\nYou could create the shared pointer there though.\n\n> I think it's easier to keep it as a shared pointer. populateMediaDevice()\n> doesn't have this issue, but I think keeping them all uniform makes life\n> easier.\n> \n> >> \tvoid removeDevice(const std::string &deviceNode);\n> >> \n> >> private:\n> >> \tstd::vector> devices_;\n> >> \n> >> +\tvirtual int populateMediaDevice(std::shared_ptr) = 0;\n> > \n> > Please name the arguments.\n> > \n> >> \tvirtual std::string lookupDeviceNode(int major, int minor) = 0;\n> >> };\n> >> \n> >> diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h\n> >> index 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 \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 media) final;\n> > \n> > Should we make the class itself final instead of marking every single\n> > method ? Same comment for DeviceEnumeratorUdev.\n> \n> populateMediaDevice() is now no longer part of DeviceEnumerator.\n> Otherwise, I'm not sure.\n> \n> >> +\tstd::string lookupDeviceNode(int major, int minor) final;\n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h\n> >> index 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 \n> >> +#include \n> >> #include \n> >> \n> >> +#include \n> > \n> > This header is only needed in the .cpp. Here you can just include\n> > sys/types.h for dev_t.\n> > \n> >> +\n> >> +#include \"media_device.h\"\n> > \n> > A forward declaration of class MediaDevice should be enough.\n> \n> I needed class MediaEntity too.\n> \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::list> map_;\n> >> +\tstd::map> revMap_;\n> >> +\tstd::map devnumToEntity_;\n> >> +\n> >> +\tstd::list orphans_;\n> > \n> > That's quite a few maps and lists, they indeed need better names and/or\n> > documentation. I think you could also merge the revMap_ and\n> > devnumToEntity_ maps if you made it a\n> > std::map, MediaEntity *> (a bit\n> > of a long type though).\n> \n> I decided to keep the maps separate. With nicer names (deps_,\n> devnumToDevice_) it ought to be easier to follow.\n\nOK, I'll check v2.\n\n> > I also wonder if we really need to deal with shared pointers internally,\n> > or if we could create the shared pointer in the addDevice() method. This\n> > could be done on top of this patch, but please keep it in mind, I'd like\n> > your opinion.\n> \n> I think it's easier to keep them as shared pointers. For one, what I\n> mentioned earlier, that addDevice() needs share pointers anyway. Another\n> is that addV4L2Device(), which is one of the main users of these maps,\n> calls addDevice().\n> \n> >> +\n> >> +\tint populateMediaDevice(std::shared_ptr 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> >>","headers":{"Return-Path":"","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 456C860BCF\n\tfor ;\n\tMon, 2 Sep 2019 09:55:44 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(231.125-247-81.adsl-dyn.isp.belgacom.be [81.247.125.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A6A90303;\n\tMon, 2 Sep 2019 09:55:43 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567410943;\n\tbh=ttQuYcJ8GIA++lSyyu4vvLHmOqm9LzrO3rUnWRUZUqQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YHJkdW0w5Ywf8xsnzWBmXa1+AKJxSXgVjEZ99VIaDgWRncgHVdSu7Y8oRcvDARtof\n\tecqgI2vxYvo18ur6CfWouBg2Ig1OVkio8Hw/GHUxrXvZTbd8SDhXYbyuy4C4c+QYlW\n\tSgopknQuduVJcmvZjJIt6+YYvYCIj9Do0AKki3JM=","Date":"Mon, 2 Sep 2019 10:55:35 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190902075535.GB4777@pendragon.ideasonboard.com>","References":"<20190828034020.17365-1-paul.elder@ideasonboard.com>\n\t<20190828100201.GB27842@pendragon.ideasonboard.com>\n\t<20190902041517.GL4818@localhost.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190902041517.GL4818@localhost.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 02 Sep 2019 07:55:44 -0000"}}]