| Message ID | 20190828034020.17365-1-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Tue, Aug 27, 2019 at 11:40:20PM -0400, Paul Elder wrote: > When a MediaDevice is enumerated and populated by the > DeviceEnumeratorUdev, there is a possibility that the member device > nodes of the media graph would not be ready. The MediaDevice is still I would detail this with "... would not be ready (either not created, or without proper permissions set by udev yet)." > passed up to the pipeline handler, where an attempt to access the device > nodes will fail in EPERM. This whole issue is especially likely to > happen when libcamera is run at system init time. > > To fix this, we first split DeviceEnumerator::addDevice() into three > methods: > - createDevice() to simply create the MediaDevice > - (abstract method) populateMediaDevice() to populate the MediaDevice As populateMediaDevice() isn't called by the base DeviceEnumerator class, you don't need to make it a virtual method, and it can be defined in the derived classes. Another option would be to move the implementation of DeviceEnumeratorSysfs::populateMediaDevice() to the DeviceEnumerator class as that code could possibly be reused, but we don't have plan for a new device enumerator at the moment, so I wouldn't attempt premature code sharing. > - addDevice() to pass the MediaDevice up to the pipeline handler > > DeviceEnumeratorSysfs calls these methods in succession, similar to what > it did before when they were all together as addDevice(). > > DeviceEnumeratorUdev additionally keeps a map of MediaDevices to a list > of pending device nodes (plus some other auxillary maps), and a simple > list of orphan device nodes. If a v4l device node is ready and there > does not exist any MediaDevice node for it, then it goes to the orphan > list, otherwise it is initialized and removed from the pending list of > the corresponding MediaDevice in the dependency map. When a MediaDevice > is populated via DeviceEnumeratorUdev::populateMediaDevice(), it first > checks the orphan list to see if the device node it needs is there, s/device node/device nodes/ and plural for the rest of the sentence ? > otherwise it tries to initialize the device node and if it fails, then > it adds the device node it wants to its list in the dependency map. > > This allows MediaDevices to be created and initialized properly with s/MediaDevices/media devices/ (or MediaDevice instances) > udev when v4l device nodes in the media graph may not be ready when the > MediaDevice is populated. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > todos: > - add documentation > - better name for the maps > - better name for populateMediaDevice() > > src/libcamera/device_enumerator.cpp | 27 +--- > src/libcamera/device_enumerator_sysfs.cpp | 33 +++- > src/libcamera/device_enumerator_udev.cpp | 142 +++++++++++++++++- > src/libcamera/include/device_enumerator.h | 4 +- > .../include/device_enumerator_sysfs.h | 5 +- > .../include/device_enumerator_udev.h | 14 ++ > 6 files changed, 197 insertions(+), 28 deletions(-) > > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp > index 60c918f..fd5a20c 100644 > --- a/src/libcamera/device_enumerator.cpp > +++ b/src/libcamera/device_enumerator.cpp > @@ -204,7 +204,7 @@ DeviceEnumerator::~DeviceEnumerator() > * > * \return 0 on success or a negative error code otherwise > */ > -int DeviceEnumerator::addDevice(const std::string &deviceNode) > +std::shared_ptr<MediaDevice> DeviceEnumerator::createDevice(const std::string &deviceNode) > { > std::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode); > > @@ -213,33 +213,22 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode) > LOG(DeviceEnumerator, Info) > << "Unable to populate media device " << deviceNode > << " (" << strerror(-ret) << "), skipping"; > - return ret; > + return nullptr; > } > > LOG(DeviceEnumerator, Debug) > << "New media device \"" << media->driver() > << "\" created from " << deviceNode; > > - /* Associate entities to device node paths. */ > - for (MediaEntity *entity : media->entities()) { > - if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) > - continue; > - > - std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > - entity->deviceMinor()); > - if (deviceNode.empty()) > - return -EINVAL; > - > - ret = entity->setDeviceNode(deviceNode); > - if (ret) > - return ret; > - } > + return media; > +} > > +int DeviceEnumerator::addDevice(std::shared_ptr<MediaDevice> media) As this method never fails, and as you never check the return value, let's make it void. > +{ > LOG(DeviceEnumerator, Debug) > - << "Added device " << deviceNode << ": " << media->driver(); > - > - devices_.push_back(std::move(media)); > + << "Added device " << media->deviceNode() << ": " << media->driver(); > > + devices_.push_back(media); > return 0; > } > > diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp > index f3054d5..3b5bc90 100644 > --- a/src/libcamera/device_enumerator_sysfs.cpp > +++ b/src/libcamera/device_enumerator_sysfs.cpp > @@ -71,7 +71,18 @@ int DeviceEnumeratorSysfs::enumerate() > continue; > } > > - addDevice(devnode); > + std::shared_ptr<MediaDevice> media = createDevice(devnode); > + if (!media) { > + closedir(dir); Instead of duplicating closedir() calls, how about ret == -ENODEV; break; and a return ret at the end of the method ? You will need to initialise ret to 0 when declaring it at the top. > + return -ENODEV; > + } > + > + if (populateMediaDevice(media) < 0) { > + closedir(dir); > + return -ENODEV; > + } > + > + addDevice(media); > } > > closedir(dir); > @@ -79,6 +90,26 @@ int DeviceEnumeratorSysfs::enumerate() > return 0; > } > > +int DeviceEnumeratorSysfs::populateMediaDevice(std::shared_ptr<MediaDevice> media) > +{ > + /* Associate entities to device node paths. */ > + for (MediaEntity *entity : media->entities()) { > + if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) > + continue; > + > + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > + entity->deviceMinor()); > + if (deviceNode.empty()) > + return -EINVAL; > + > + int ret = entity->setDeviceNode(deviceNode); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > std::string DeviceEnumeratorSysfs::lookupDeviceNode(int major, int minor) > { > std::string deviceNode; > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > index 86f6ca1..ef9a31d 100644 > --- a/src/libcamera/device_enumerator_udev.cpp > +++ b/src/libcamera/device_enumerator_udev.cpp > @@ -7,6 +7,10 @@ > > #include "device_enumerator_udev.h" > > +#include <algorithm> > +#include <list> > +#include <map> > + > #include <fcntl.h> > #include <libudev.h> > #include <string.h> > @@ -57,6 +61,11 @@ int DeviceEnumeratorUdev::init() > if (ret < 0) > return ret; > > + ret = udev_monitor_filter_add_match_subsystem_devtype(monitor_, "video4linux", > + nullptr); > + if (ret < 0) > + return ret; > + > return 0; > } > > @@ -74,6 +83,14 @@ int DeviceEnumeratorUdev::enumerate() > if (ret < 0) > goto done; > > + ret = udev_enumerate_add_match_subsystem(udev_enum, "video4linux"); > + if (ret < 0) > + goto done; > + > + ret = udev_enumerate_add_match_is_initialized(udev_enum); > + if (ret < 0) > + goto done; > + > ret = udev_enumerate_scan_devices(udev_enum); > if (ret < 0) > goto done; > @@ -85,6 +102,7 @@ int DeviceEnumeratorUdev::enumerate() > udev_list_entry_foreach(ent, ents) { > struct udev_device *dev; > const char *devnode; > + const char *subsystem; > const char *syspath = udev_list_entry_get_name(ent); > > dev = udev_device_new_from_syspath(udev_, syspath); > @@ -96,16 +114,36 @@ int DeviceEnumeratorUdev::enumerate() > } > > devnode = udev_device_get_devnode(dev); > - if (!devnode) { > - udev_device_unref(dev); > - ret = -ENODEV; > - goto done; > + if (!devnode) > + goto unref; > + > + subsystem = udev_device_get_subsystem(dev); > + if (!subsystem) > + goto unref; Can this happen ? > + > + if (!strcmp(subsystem, "media")) { > + std::shared_ptr<MediaDevice> media = createDevice(devnode); > + if (!media) > + goto unref; > + > + ret = populateMediaDevice(media); > + if (ret == 0) > + addDevice(media); > + } else if (!strcmp(subsystem, "video4linux")) { > + addV4L2Device(udev_device_get_devnum(dev)); > + } else { > + goto unref; > } > > - addDevice(devnode); > + udev_device_unref(dev); > + continue; > > + unref: > udev_device_unref(dev); > + ret = -ENODEV; > + goto done; This isn't nice, and may call for moving part of the code in the loop to separate method. Especially as there's code duplication with DeviceEnumeratorUdev::udevNotify(). > } > + > done: > udev_enumerate_unref(udev_enum); > if (ret < 0) > @@ -122,6 +160,49 @@ done: > return 0; > } > > +int DeviceEnumeratorUdev::populateMediaDevice(std::shared_ptr<MediaDevice> media) > +{ > + /* number of pending devnodes */ Instead of requiring a comment, how about renaming the variable to make its purpose explicit ? > + int count = 0; count is never negative, you can make it an unsigned int. > + int ret; > + > + /* Associate entities to device node paths. */ > + for (MediaEntity *entity : media->entities()) { > + if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) > + continue; > + > + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > + entity->deviceMinor()); Let's not attempt a lookup of the device node before we locate the device in the orphans list, as this may fail. > + dev_t devnum = makedev(entity->deviceMajor(), > + entity->deviceMinor()); > + > + /* take device from orphan list first, if it is in the list */ > + if (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) { > + if (deviceNode.empty()) > + return -EINVAL; > + > + ret = entity->setDeviceNode(deviceNode); > + if (ret) > + return ret; > + > + orphans_.remove(devnum); > + continue; > + } > + > + if (deviceNode.empty() || (ret = ::access(deviceNode.c_str(), R_OK | W_OK)) < 0) { Assigning variables inside conditions is frowned upon. And I don't think there's a need to test ::access() here, as if udev hasn't notified us of the device being ready, there's little point in proceding. There's already an access() test in MediaEntity::setDeviceNode(). > + map_[media].push_back(devnum); > + revMap_[devnum] = media; > + devnumToEntity_[devnum] = entity; > + count++; > + } > + > + if (!ret) If deviceNode is empty, and no device has been previously found in the orphaned list, ret will be used uninitialised. > + entity->setDeviceNode(deviceNode); I think you can just drop this, the setDeviceNode() call when the device is found in the orphaned list should be enough. > + } > + > + return count; > +} > + > std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor) > { > struct udev_device *device; > @@ -143,17 +224,66 @@ std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor) > return deviceNode; > } > > +// add V4L2 device to the media device if it exists, otherwise to the orphan list > +// if adding to media device, and it's the last one, then send up the media device > +int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > +{ > + MediaEntity *entity = devnumToEntity_[devnum]; > + if (!entity) { > + orphans_.push_back(devnum); > + return 0; > + } > + > + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > + entity->deviceMinor()); > + if (deviceNode.empty()) > + return -EINVAL; > + > + int ret = entity->setDeviceNode(deviceNode); > + if (ret) > + return ret; > + > + std::shared_ptr<MediaDevice> media = revMap_[devnum]; > + map_[media].remove(devnum); > + revMap_.erase(devnum); > + devnumToEntity_.erase(devnum); > + > + if (map_[media].empty()) > + addDevice(media); > + > + return 0; > +} > + > void DeviceEnumeratorUdev::udevNotify(EventNotifier *notifier) > { > struct udev_device *dev = udev_monitor_receive_device(monitor_); > std::string action(udev_device_get_action(dev)); > std::string deviceNode(udev_device_get_devnode(dev)); > + dev_t deviceNum(udev_device_get_devnum(dev)); > > LOG(DeviceEnumerator, Debug) > << action << " device " << udev_device_get_devnode(dev); > > if (action == "add") { > - addDevice(deviceNode); > + const char *subsystem = udev_device_get_subsystem(dev); > + if (!subsystem) { > + udev_device_unref(dev); > + return; > + } > + > + if (!strcmp(subsystem, "media")) { > + std::shared_ptr<MediaDevice> media = createDevice(deviceNode); > + if (!media) { > + udev_device_unref(dev); > + return; > + } > + > + int ret = populateMediaDevice(media); > + if (ret == 0) > + addDevice(media); As populateMediaDevice() is always followed by addDevice(), should you call the latter inside the former ? > + } else if (!strcmp(subsystem, "video4linux")) { > + addV4L2Device(deviceNum); > + } > } else if (action == "remove") { > removeDevice(deviceNode); Should this be conditioned to subsystem == media ? > } > diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h > index 02aec3b..4c3e500 100644 > --- a/src/libcamera/include/device_enumerator.h > +++ b/src/libcamera/include/device_enumerator.h > @@ -44,12 +44,14 @@ public: > std::shared_ptr<MediaDevice> search(const DeviceMatch &dm); > > protected: > - int addDevice(const std::string &deviceNode); > + std::shared_ptr<MediaDevice> createDevice(const std::string &deviceNode); > + int addDevice(std::shared_ptr<MediaDevice> media); You can pass a const reference to the shared pointer to avoid a copy of the shared pointer itself (the MediaDevice it points to is of course not copied). Same comment for populateMediaDevice() method. > void removeDevice(const std::string &deviceNode); > > private: > std::vector<std::shared_ptr<MediaDevice>> devices_; > > + virtual int populateMediaDevice(std::shared_ptr<MediaDevice>) = 0; Please name the arguments. > virtual std::string lookupDeviceNode(int major, int minor) = 0; > }; > > diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h > index 8d3adc9..d14bbf8 100644 > --- a/src/libcamera/include/device_enumerator_sysfs.h > +++ b/src/libcamera/include/device_enumerator_sysfs.h > @@ -9,6 +9,8 @@ > > #include <string> > > +#include "media_device.h" > + > #include "device_enumerator.h" > > namespace libcamera { > @@ -20,7 +22,8 @@ public: > int enumerate(); > > private: > - std::string lookupDeviceNode(int major, int minor); > + int populateMediaDevice(std::shared_ptr<MediaDevice> media) final; Should we make the class itself final instead of marking every single method ? Same comment for DeviceEnumeratorUdev. > + std::string lookupDeviceNode(int major, int minor) final; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h > index 80f9372..b3bbcb0 100644 > --- a/src/libcamera/include/device_enumerator_udev.h > +++ b/src/libcamera/include/device_enumerator_udev.h > @@ -7,8 +7,14 @@ > #ifndef __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__ > #define __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__ > > +#include <list> > +#include <map> > #include <string> > > +#include <sys/sysmacros.h> This header is only needed in the .cpp. Here you can just include sys/types.h for dev_t. > + > +#include "media_device.h" A forward declaration of class MediaDevice should be enough. > + > #include "device_enumerator.h" > > struct udev; > @@ -32,8 +38,16 @@ private: > struct udev_monitor *monitor_; > EventNotifier *notifier_; > > + std::map<std::shared_ptr<MediaDevice>, std::list<dev_t>> map_; > + std::map<dev_t, std::shared_ptr<MediaDevice>> revMap_; > + std::map<dev_t, MediaEntity *> devnumToEntity_; > + > + std::list<dev_t> orphans_; That's quite a few maps and lists, they indeed need better names and/or documentation. I think you could also merge the revMap_ and devnumToEntity_ maps if you made it a std::map<std::pair<std::shared_ptr<MediaDevice>, MediaEntity *> (a bit of a long type though). I also wonder if we really need to deal with shared pointers internally, or if we could create the shared pointer in the addDevice() method. This could be done on top of this patch, but please keep it in mind, I'd like your opinion. > + > + int populateMediaDevice(std::shared_ptr<MediaDevice> media) final; > std::string lookupDeviceNode(int major, int minor) final; > > + int addV4L2Device(dev_t devnum); > void udevNotify(EventNotifier *notifier); > }; >
Hi Laurent, On Wed, Aug 28, 2019 at 01:02:01PM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. Thank you for the review. > On Tue, Aug 27, 2019 at 11:40:20PM -0400, Paul Elder wrote: > > When a MediaDevice is enumerated and populated by the > > DeviceEnumeratorUdev, there is a possibility that the member device > > nodes of the media graph would not be ready. The MediaDevice is still > > I would detail this with > > "... would not be ready (either not created, or without proper > permissions set by udev yet)." > > > passed up to the pipeline handler, where an attempt to access the device > > nodes will fail in EPERM. This whole issue is especially likely to > > happen when libcamera is run at system init time. > > > > To fix this, we first split DeviceEnumerator::addDevice() into three > > methods: > > - createDevice() to simply create the MediaDevice > > - (abstract method) populateMediaDevice() to populate the MediaDevice > > As populateMediaDevice() isn't called by the base DeviceEnumerator > class, you don't need to make it a virtual method, and it can be defined > in the derived classes. Another option would be to move the > implementation of DeviceEnumeratorSysfs::populateMediaDevice() to the > DeviceEnumerator class as that code could possibly be reused, but we > don't have plan for a new device enumerator at the moment, so I wouldn't > attempt premature code sharing. > > > - addDevice() to pass the MediaDevice up to the pipeline handler > > > > DeviceEnumeratorSysfs calls these methods in succession, similar to what > > it did before when they were all together as addDevice(). > > > > DeviceEnumeratorUdev additionally keeps a map of MediaDevices to a list > > of pending device nodes (plus some other auxillary maps), and a simple > > list of orphan device nodes. If a v4l device node is ready and there > > does not exist any MediaDevice node for it, then it goes to the orphan > > list, otherwise it is initialized and removed from the pending list of > > the corresponding MediaDevice in the dependency map. When a MediaDevice > > is populated via DeviceEnumeratorUdev::populateMediaDevice(), it first > > checks the orphan list to see if the device node it needs is there, > > s/device node/device nodes/ and plural for the rest of the sentence ? > > > otherwise it tries to initialize the device node and if it fails, then > > it adds the device node it wants to its list in the dependency map. > > > > This allows MediaDevices to be created and initialized properly with > > s/MediaDevices/media devices/ (or MediaDevice instances) > > > udev when v4l device nodes in the media graph may not be ready when the > > MediaDevice is populated. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > todos: > > - add documentation > > - better name for the maps > > - better name for populateMediaDevice() > > > > src/libcamera/device_enumerator.cpp | 27 +--- > > src/libcamera/device_enumerator_sysfs.cpp | 33 +++- > > src/libcamera/device_enumerator_udev.cpp | 142 +++++++++++++++++- > > src/libcamera/include/device_enumerator.h | 4 +- > > .../include/device_enumerator_sysfs.h | 5 +- > > .../include/device_enumerator_udev.h | 14 ++ > > 6 files changed, 197 insertions(+), 28 deletions(-) > > > > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp > > index 60c918f..fd5a20c 100644 > > --- a/src/libcamera/device_enumerator.cpp > > +++ b/src/libcamera/device_enumerator.cpp > > @@ -204,7 +204,7 @@ DeviceEnumerator::~DeviceEnumerator() > > * > > * \return 0 on success or a negative error code otherwise > > */ > > -int DeviceEnumerator::addDevice(const std::string &deviceNode) > > +std::shared_ptr<MediaDevice> DeviceEnumerator::createDevice(const std::string &deviceNode) > > { > > std::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode); > > > > @@ -213,33 +213,22 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode) > > LOG(DeviceEnumerator, Info) > > << "Unable to populate media device " << deviceNode > > << " (" << strerror(-ret) << "), skipping"; > > - return ret; > > + return nullptr; > > } > > > > LOG(DeviceEnumerator, Debug) > > << "New media device \"" << media->driver() > > << "\" created from " << deviceNode; > > > > - /* Associate entities to device node paths. */ > > - for (MediaEntity *entity : media->entities()) { > > - if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) > > - continue; > > - > > - std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > > - entity->deviceMinor()); > > - if (deviceNode.empty()) > > - return -EINVAL; > > - > > - ret = entity->setDeviceNode(deviceNode); > > - if (ret) > > - return ret; > > - } > > + return media; > > +} > > > > +int DeviceEnumerator::addDevice(std::shared_ptr<MediaDevice> media) > > As this method never fails, and as you never check the return value, > let's make it void. > > > +{ > > LOG(DeviceEnumerator, Debug) > > - << "Added device " << deviceNode << ": " << media->driver(); > > - > > - devices_.push_back(std::move(media)); > > + << "Added device " << media->deviceNode() << ": " << media->driver(); > > > > + devices_.push_back(media); > > return 0; > > } > > > > diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp > > index f3054d5..3b5bc90 100644 > > --- a/src/libcamera/device_enumerator_sysfs.cpp > > +++ b/src/libcamera/device_enumerator_sysfs.cpp > > @@ -71,7 +71,18 @@ int DeviceEnumeratorSysfs::enumerate() > > continue; > > } > > > > - addDevice(devnode); > > + std::shared_ptr<MediaDevice> media = createDevice(devnode); > > + if (!media) { > > + closedir(dir); > > Instead of duplicating closedir() calls, how about > > ret == -ENODEV; > break; > > and a return ret at the end of the method ? You will need to initialise > ret to 0 when declaring it at the top. > > > + return -ENODEV; > > + } > > + > > + if (populateMediaDevice(media) < 0) { > > + closedir(dir); > > + return -ENODEV; > > + } > > + > > + addDevice(media); > > } > > > > closedir(dir); > > @@ -79,6 +90,26 @@ int DeviceEnumeratorSysfs::enumerate() > > return 0; > > } > > > > +int DeviceEnumeratorSysfs::populateMediaDevice(std::shared_ptr<MediaDevice> media) > > +{ > > + /* Associate entities to device node paths. */ > > + for (MediaEntity *entity : media->entities()) { > > + if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) > > + continue; > > + > > + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > > + entity->deviceMinor()); > > + if (deviceNode.empty()) > > + return -EINVAL; > > + > > + int ret = entity->setDeviceNode(deviceNode); > > + if (ret) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > std::string DeviceEnumeratorSysfs::lookupDeviceNode(int major, int minor) > > { > > std::string deviceNode; > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > > index 86f6ca1..ef9a31d 100644 > > --- a/src/libcamera/device_enumerator_udev.cpp > > +++ b/src/libcamera/device_enumerator_udev.cpp > > @@ -7,6 +7,10 @@ > > > > #include "device_enumerator_udev.h" > > > > +#include <algorithm> > > +#include <list> > > +#include <map> > > + > > #include <fcntl.h> > > #include <libudev.h> > > #include <string.h> > > @@ -57,6 +61,11 @@ int DeviceEnumeratorUdev::init() > > if (ret < 0) > > return ret; > > > > + ret = udev_monitor_filter_add_match_subsystem_devtype(monitor_, "video4linux", > > + nullptr); > > + if (ret < 0) > > + return ret; > > + > > return 0; > > } > > > > @@ -74,6 +83,14 @@ int DeviceEnumeratorUdev::enumerate() > > if (ret < 0) > > goto done; > > > > + ret = udev_enumerate_add_match_subsystem(udev_enum, "video4linux"); > > + if (ret < 0) > > + goto done; > > + > > + ret = udev_enumerate_add_match_is_initialized(udev_enum); > > + if (ret < 0) > > + goto done; > > + > > ret = udev_enumerate_scan_devices(udev_enum); > > if (ret < 0) > > goto done; > > @@ -85,6 +102,7 @@ int DeviceEnumeratorUdev::enumerate() > > udev_list_entry_foreach(ent, ents) { > > struct udev_device *dev; > > const char *devnode; > > + const char *subsystem; > > const char *syspath = udev_list_entry_get_name(ent); > > > > dev = udev_device_new_from_syspath(udev_, syspath); > > @@ -96,16 +114,36 @@ int DeviceEnumeratorUdev::enumerate() > > } > > > > devnode = udev_device_get_devnode(dev); > > - if (!devnode) { > > - udev_device_unref(dev); > > - ret = -ENODEV; > > - goto done; > > + if (!devnode) > > + goto unref; > > + > > + subsystem = udev_device_get_subsystem(dev); > > + if (!subsystem) > > + goto unref; > > Can this happen ? tbh, I'm not sure. But the documentation for udev_device_get_subsystem() says that it can return NULL, and I don't think we want to continue if it ever does. > > + > > + if (!strcmp(subsystem, "media")) { > > + std::shared_ptr<MediaDevice> media = createDevice(devnode); > > + if (!media) > > + goto unref; > > + > > + ret = populateMediaDevice(media); > > + if (ret == 0) > > + addDevice(media); > > + } else if (!strcmp(subsystem, "video4linux")) { > > + addV4L2Device(udev_device_get_devnum(dev)); > > + } else { > > + goto unref; > > } > > > > - addDevice(devnode); > > + udev_device_unref(dev); > > + continue; > > > > + unref: > > udev_device_unref(dev); > > + ret = -ENODEV; > > + goto done; > > This isn't nice, and may call for moving part of the code in the loop to > separate method. Especially as there's code duplication with > DeviceEnumeratorUdev::udevNotify(). I split that into a separate method, and just expanded this error handling. See v2 (coming soon!). > > } > > + > > done: > > udev_enumerate_unref(udev_enum); > > if (ret < 0) > > @@ -122,6 +160,49 @@ done: > > return 0; > > } > > > > +int DeviceEnumeratorUdev::populateMediaDevice(std::shared_ptr<MediaDevice> media) > > +{ > > + /* number of pending devnodes */ > > Instead of requiring a comment, how about renaming the variable to make > its purpose explicit ? > > > + int count = 0; > > count is never negative, you can make it an unsigned int. > > > + int ret; > > + > > + /* Associate entities to device node paths. */ > > + for (MediaEntity *entity : media->entities()) { > > + if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) > > + continue; > > + > > + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > > + entity->deviceMinor()); > > Let's not attempt a lookup of the device node before we locate the > device in the orphans list, as this may fail. It's possible for the device to be ready and not be in the orphans list, so I think it's okay to keep this here. If it fails then it'll be an empty string, and we check for that anyway. > > + dev_t devnum = makedev(entity->deviceMajor(), > > + entity->deviceMinor()); > > + > > + /* take device from orphan list first, if it is in the list */ > > + if (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) { > > + if (deviceNode.empty()) > > + return -EINVAL; > > + > > + ret = entity->setDeviceNode(deviceNode); > > + if (ret) > > + return ret; > > + > > + orphans_.remove(devnum); > > + continue; > > + } > > + > > + if (deviceNode.empty() || (ret = ::access(deviceNode.c_str(), R_OK | W_OK)) < 0) { > > Assigning variables inside conditions is frowned upon. And I don't think > there's a need to test ::access() here, as if udev hasn't notified us of > the device being ready, there's little point in proceding. There's > already an access() test in MediaEntity::setDeviceNode(). I did call and check the return value of setDeviceNode(), but then there will be a bunch of misleading ERROR log messages complaining about EPERM, so I think it's better to test ::access() here instead. > > + map_[media].push_back(devnum); > > + revMap_[devnum] = media; > > + devnumToEntity_[devnum] = entity; > > + count++; > > + } > > + > > + if (!ret) > > If deviceNode is empty, and no device has been previously found in the > orphaned list, ret will be used uninitialised. > > > + entity->setDeviceNode(deviceNode); > > I think you can just drop this, the setDeviceNode() call when the device > is found in the orphaned list should be enough. I'm going to keep this, to cover the case that a device is ready but not in the orphans list. > > + } > > + > > + return count; > > +} > > + > > std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor) > > { > > struct udev_device *device; > > @@ -143,17 +224,66 @@ std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor) > > return deviceNode; > > } > > > > +// add V4L2 device to the media device if it exists, otherwise to the orphan list > > +// if adding to media device, and it's the last one, then send up the media device > > +int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > > +{ > > + MediaEntity *entity = devnumToEntity_[devnum]; > > + if (!entity) { > > + orphans_.push_back(devnum); > > + return 0; > > + } > > + > > + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > > + entity->deviceMinor()); > > + if (deviceNode.empty()) > > + return -EINVAL; > > + > > + int ret = entity->setDeviceNode(deviceNode); > > + if (ret) > > + return ret; > > + > > + std::shared_ptr<MediaDevice> media = revMap_[devnum]; > > + map_[media].remove(devnum); > > + revMap_.erase(devnum); > > + devnumToEntity_.erase(devnum); > > + > > + if (map_[media].empty()) > > + addDevice(media); > > + > > + return 0; > > +} > > + > > void DeviceEnumeratorUdev::udevNotify(EventNotifier *notifier) > > { > > struct udev_device *dev = udev_monitor_receive_device(monitor_); > > std::string action(udev_device_get_action(dev)); > > std::string deviceNode(udev_device_get_devnode(dev)); > > + dev_t deviceNum(udev_device_get_devnum(dev)); > > > > LOG(DeviceEnumerator, Debug) > > << action << " device " << udev_device_get_devnode(dev); > > > > if (action == "add") { > > - addDevice(deviceNode); > > + const char *subsystem = udev_device_get_subsystem(dev); > > + if (!subsystem) { > > + udev_device_unref(dev); > > + return; > > + } > > + > > + if (!strcmp(subsystem, "media")) { > > + std::shared_ptr<MediaDevice> media = createDevice(deviceNode); > > + if (!media) { > > + udev_device_unref(dev); > > + return; > > + } > > + > > + int ret = populateMediaDevice(media); > > + if (ret == 0) > > + addDevice(media); > > As populateMediaDevice() is always followed by addDevice(), should you > call the latter inside the former ? I don't think addDevice() belongs in populateMediaDevice(). > > + } else if (!strcmp(subsystem, "video4linux")) { > > + addV4L2Device(deviceNum); > > + } > > } else if (action == "remove") { > > removeDevice(deviceNode); > > > Should this be conditioned to subsystem == media ? Yes. > > } > > diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h > > index 02aec3b..4c3e500 100644 > > --- a/src/libcamera/include/device_enumerator.h > > +++ b/src/libcamera/include/device_enumerator.h > > @@ -44,12 +44,14 @@ public: > > std::shared_ptr<MediaDevice> search(const DeviceMatch &dm); > > > > protected: > > - int addDevice(const std::string &deviceNode); > > + std::shared_ptr<MediaDevice> createDevice(const std::string &deviceNode); > > + int addDevice(std::shared_ptr<MediaDevice> media); > > You can pass a const reference to the shared pointer to avoid a copy of > the shared pointer itself (the MediaDevice it points to is of course not > copied). Same comment for populateMediaDevice() method. addDevice() calls devices_.push_back(), which wants a shared pointer. I think it's easier to keep it as a shared pointer. populateMediaDevice() doesn't have this issue, but I think keeping them all uniform makes life easier. > > void removeDevice(const std::string &deviceNode); > > > > private: > > std::vector<std::shared_ptr<MediaDevice>> devices_; > > > > + virtual int populateMediaDevice(std::shared_ptr<MediaDevice>) = 0; > > Please name the arguments. > > > virtual std::string lookupDeviceNode(int major, int minor) = 0; > > }; > > > > diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h > > index 8d3adc9..d14bbf8 100644 > > --- a/src/libcamera/include/device_enumerator_sysfs.h > > +++ b/src/libcamera/include/device_enumerator_sysfs.h > > @@ -9,6 +9,8 @@ > > > > #include <string> > > > > +#include "media_device.h" > > + > > #include "device_enumerator.h" > > > > namespace libcamera { > > @@ -20,7 +22,8 @@ public: > > int enumerate(); > > > > private: > > - std::string lookupDeviceNode(int major, int minor); > > + int populateMediaDevice(std::shared_ptr<MediaDevice> media) final; > > Should we make the class itself final instead of marking every single > method ? Same comment for DeviceEnumeratorUdev. populateMediaDevice() is now no longer part of DeviceEnumerator. Otherwise, I'm not sure. > > + std::string lookupDeviceNode(int major, int minor) final; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h > > index 80f9372..b3bbcb0 100644 > > --- a/src/libcamera/include/device_enumerator_udev.h > > +++ b/src/libcamera/include/device_enumerator_udev.h > > @@ -7,8 +7,14 @@ > > #ifndef __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__ > > #define __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__ > > > > +#include <list> > > +#include <map> > > #include <string> > > > > +#include <sys/sysmacros.h> > > This header is only needed in the .cpp. Here you can just include > sys/types.h for dev_t. > > > + > > +#include "media_device.h" > > A forward declaration of class MediaDevice should be enough. I needed class MediaEntity too. > > + > > #include "device_enumerator.h" > > > > struct udev; > > @@ -32,8 +38,16 @@ private: > > struct udev_monitor *monitor_; > > EventNotifier *notifier_; > > > > + std::map<std::shared_ptr<MediaDevice>, std::list<dev_t>> map_; > > + std::map<dev_t, std::shared_ptr<MediaDevice>> revMap_; > > + std::map<dev_t, MediaEntity *> devnumToEntity_; > > + > > + std::list<dev_t> orphans_; > > That's quite a few maps and lists, they indeed need better names and/or > documentation. I think you could also merge the revMap_ and > devnumToEntity_ maps if you made it a > std::map<std::pair<std::shared_ptr<MediaDevice>, MediaEntity *> (a bit > of a long type though). I decided to keep the maps separate. With nicer names (deps_, devnumToDevice_) it ought to be easier to follow. > I also wonder if we really need to deal with shared pointers internally, > or if we could create the shared pointer in the addDevice() method. This > could be done on top of this patch, but please keep it in mind, I'd like > your opinion. I think it's easier to keep them as shared pointers. For one, what I mentioned earlier, that addDevice() needs share pointers anyway. Another is that addV4L2Device(), which is one of the main users of these maps, calls addDevice(). > > + > > + int populateMediaDevice(std::shared_ptr<MediaDevice> media) final; > > std::string lookupDeviceNode(int major, int minor) final; > > > > + int addV4L2Device(dev_t devnum); > > void udevNotify(EventNotifier *notifier); > > }; > > Thanks, Paul
Hi Paul, On Mon, Sep 02, 2019 at 12:15:17AM -0400, Paul Elder wrote: > On Wed, Aug 28, 2019 at 01:02:01PM +0300, Laurent Pinchart wrote: > > On Tue, Aug 27, 2019 at 11:40:20PM -0400, Paul Elder wrote: > >> When a MediaDevice is enumerated and populated by the > >> DeviceEnumeratorUdev, there is a possibility that the member device > >> nodes of the media graph would not be ready. The MediaDevice is still > > > > I would detail this with > > > > "... would not be ready (either not created, or without proper > > permissions set by udev yet)." > > > >> passed up to the pipeline handler, where an attempt to access the device > >> nodes will fail in EPERM. This whole issue is especially likely to > >> happen when libcamera is run at system init time. > >> > >> To fix this, we first split DeviceEnumerator::addDevice() into three > >> methods: > >> - createDevice() to simply create the MediaDevice > >> - (abstract method) populateMediaDevice() to populate the MediaDevice > > > > As populateMediaDevice() isn't called by the base DeviceEnumerator > > class, you don't need to make it a virtual method, and it can be defined > > in the derived classes. Another option would be to move the > > implementation of DeviceEnumeratorSysfs::populateMediaDevice() to the > > DeviceEnumerator class as that code could possibly be reused, but we > > don't have plan for a new device enumerator at the moment, so I wouldn't > > attempt premature code sharing. > > > >> - addDevice() to pass the MediaDevice up to the pipeline handler > >> > >> DeviceEnumeratorSysfs calls these methods in succession, similar to what > >> it did before when they were all together as addDevice(). > >> > >> DeviceEnumeratorUdev additionally keeps a map of MediaDevices to a list > >> of pending device nodes (plus some other auxillary maps), and a simple > >> list of orphan device nodes. If a v4l device node is ready and there > >> does not exist any MediaDevice node for it, then it goes to the orphan > >> list, otherwise it is initialized and removed from the pending list of > >> the corresponding MediaDevice in the dependency map. When a MediaDevice > >> is populated via DeviceEnumeratorUdev::populateMediaDevice(), it first > >> checks the orphan list to see if the device node it needs is there, > > > > s/device node/device nodes/ and plural for the rest of the sentence ? > > > >> otherwise it tries to initialize the device node and if it fails, then > >> it adds the device node it wants to its list in the dependency map. > >> > >> This allows MediaDevices to be created and initialized properly with > > > > s/MediaDevices/media devices/ (or MediaDevice instances) > > > >> udev when v4l device nodes in the media graph may not be ready when the > >> MediaDevice is populated. > >> > >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > >> > >> --- > >> todos: > >> - add documentation > >> - better name for the maps > >> - better name for populateMediaDevice() > >> > >> src/libcamera/device_enumerator.cpp | 27 +--- > >> src/libcamera/device_enumerator_sysfs.cpp | 33 +++- > >> src/libcamera/device_enumerator_udev.cpp | 142 +++++++++++++++++- > >> src/libcamera/include/device_enumerator.h | 4 +- > >> .../include/device_enumerator_sysfs.h | 5 +- > >> .../include/device_enumerator_udev.h | 14 ++ > >> 6 files changed, 197 insertions(+), 28 deletions(-) > >> > >> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp > >> index 60c918f..fd5a20c 100644 > >> --- a/src/libcamera/device_enumerator.cpp > >> +++ b/src/libcamera/device_enumerator.cpp > >> @@ -204,7 +204,7 @@ DeviceEnumerator::~DeviceEnumerator() > >> * > >> * \return 0 on success or a negative error code otherwise > >> */ > >> -int DeviceEnumerator::addDevice(const std::string &deviceNode) > >> +std::shared_ptr<MediaDevice> DeviceEnumerator::createDevice(const std::string &deviceNode) > >> { > >> std::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode); > >> > >> @@ -213,33 +213,22 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode) > >> LOG(DeviceEnumerator, Info) > >> << "Unable to populate media device " << deviceNode > >> << " (" << strerror(-ret) << "), skipping"; > >> - return ret; > >> + return nullptr; > >> } > >> > >> LOG(DeviceEnumerator, Debug) > >> << "New media device \"" << media->driver() > >> << "\" created from " << deviceNode; > >> > >> - /* Associate entities to device node paths. */ > >> - for (MediaEntity *entity : media->entities()) { > >> - if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) > >> - continue; > >> - > >> - std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > >> - entity->deviceMinor()); > >> - if (deviceNode.empty()) > >> - return -EINVAL; > >> - > >> - ret = entity->setDeviceNode(deviceNode); > >> - if (ret) > >> - return ret; > >> - } > >> + return media; > >> +} > >> > >> +int DeviceEnumerator::addDevice(std::shared_ptr<MediaDevice> media) > > > > As this method never fails, and as you never check the return value, > > let's make it void. > > > >> +{ > >> LOG(DeviceEnumerator, Debug) > >> - << "Added device " << deviceNode << ": " << media->driver(); > >> - > >> - devices_.push_back(std::move(media)); > >> + << "Added device " << media->deviceNode() << ": " << media->driver(); > >> > >> + devices_.push_back(media); > >> return 0; > >> } > >> > >> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp > >> index f3054d5..3b5bc90 100644 > >> --- a/src/libcamera/device_enumerator_sysfs.cpp > >> +++ b/src/libcamera/device_enumerator_sysfs.cpp > >> @@ -71,7 +71,18 @@ int DeviceEnumeratorSysfs::enumerate() > >> continue; > >> } > >> > >> - addDevice(devnode); > >> + std::shared_ptr<MediaDevice> media = createDevice(devnode); > >> + if (!media) { > >> + closedir(dir); > > > > Instead of duplicating closedir() calls, how about > > > > ret == -ENODEV; > > break; > > > > and a return ret at the end of the method ? You will need to initialise > > ret to 0 when declaring it at the top. > > > >> + return -ENODEV; > >> + } > >> + > >> + if (populateMediaDevice(media) < 0) { > >> + closedir(dir); > >> + return -ENODEV; > >> + } > >> + > >> + addDevice(media); > >> } > >> > >> closedir(dir); > >> @@ -79,6 +90,26 @@ int DeviceEnumeratorSysfs::enumerate() > >> return 0; > >> } > >> > >> +int DeviceEnumeratorSysfs::populateMediaDevice(std::shared_ptr<MediaDevice> media) > >> +{ > >> + /* Associate entities to device node paths. */ > >> + for (MediaEntity *entity : media->entities()) { > >> + if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) > >> + continue; > >> + > >> + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > >> + entity->deviceMinor()); > >> + if (deviceNode.empty()) > >> + return -EINVAL; > >> + > >> + int ret = entity->setDeviceNode(deviceNode); > >> + if (ret) > >> + return ret; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> std::string DeviceEnumeratorSysfs::lookupDeviceNode(int major, int minor) > >> { > >> std::string deviceNode; > >> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > >> index 86f6ca1..ef9a31d 100644 > >> --- a/src/libcamera/device_enumerator_udev.cpp > >> +++ b/src/libcamera/device_enumerator_udev.cpp > >> @@ -7,6 +7,10 @@ > >> > >> #include "device_enumerator_udev.h" > >> > >> +#include <algorithm> > >> +#include <list> > >> +#include <map> > >> + > >> #include <fcntl.h> > >> #include <libudev.h> > >> #include <string.h> > >> @@ -57,6 +61,11 @@ int DeviceEnumeratorUdev::init() > >> if (ret < 0) > >> return ret; > >> > >> + ret = udev_monitor_filter_add_match_subsystem_devtype(monitor_, "video4linux", > >> + nullptr); > >> + if (ret < 0) > >> + return ret; > >> + > >> return 0; > >> } > >> > >> @@ -74,6 +83,14 @@ int DeviceEnumeratorUdev::enumerate() > >> if (ret < 0) > >> goto done; > >> > >> + ret = udev_enumerate_add_match_subsystem(udev_enum, "video4linux"); > >> + if (ret < 0) > >> + goto done; > >> + > >> + ret = udev_enumerate_add_match_is_initialized(udev_enum); > >> + if (ret < 0) > >> + goto done; > >> + > >> ret = udev_enumerate_scan_devices(udev_enum); > >> if (ret < 0) > >> goto done; > >> @@ -85,6 +102,7 @@ int DeviceEnumeratorUdev::enumerate() > >> udev_list_entry_foreach(ent, ents) { > >> struct udev_device *dev; > >> const char *devnode; > >> + const char *subsystem; > >> const char *syspath = udev_list_entry_get_name(ent); > >> > >> dev = udev_device_new_from_syspath(udev_, syspath); > >> @@ -96,16 +114,36 @@ int DeviceEnumeratorUdev::enumerate() > >> } > >> > >> devnode = udev_device_get_devnode(dev); > >> - if (!devnode) { > >> - udev_device_unref(dev); > >> - ret = -ENODEV; > >> - goto done; > >> + if (!devnode) > >> + goto unref; > >> + > >> + subsystem = udev_device_get_subsystem(dev); > >> + if (!subsystem) > >> + goto unref; > > > > Can this happen ? > > tbh, I'm not sure. But the documentation for udev_device_get_subsystem() > says that it can return NULL, and I don't think we want to continue if > it ever does. OK, let's keep the check. > >> + > >> + if (!strcmp(subsystem, "media")) { > >> + std::shared_ptr<MediaDevice> media = createDevice(devnode); > >> + if (!media) > >> + goto unref; > >> + > >> + ret = populateMediaDevice(media); > >> + if (ret == 0) > >> + addDevice(media); > >> + } else if (!strcmp(subsystem, "video4linux")) { > >> + addV4L2Device(udev_device_get_devnum(dev)); > >> + } else { > >> + goto unref; > >> } > >> > >> - addDevice(devnode); > >> + udev_device_unref(dev); > >> + continue; > >> > >> + unref: > >> udev_device_unref(dev); > >> + ret = -ENODEV; > >> + goto done; > > > > This isn't nice, and may call for moving part of the code in the loop to > > separate method. Especially as there's code duplication with > > DeviceEnumeratorUdev::udevNotify(). > > I split that into a separate method, and just expanded this error > handling. See v2 (coming soon!). > > >> } > >> + > >> done: > >> udev_enumerate_unref(udev_enum); > >> if (ret < 0) > >> @@ -122,6 +160,49 @@ done: > >> return 0; > >> } > >> > >> +int DeviceEnumeratorUdev::populateMediaDevice(std::shared_ptr<MediaDevice> media) > >> +{ > >> + /* number of pending devnodes */ > > > > Instead of requiring a comment, how about renaming the variable to make > > its purpose explicit ? > > > >> + int count = 0; > > > > count is never negative, you can make it an unsigned int. > > > >> + int ret; > >> + > >> + /* Associate entities to device node paths. */ > >> + for (MediaEntity *entity : media->entities()) { > >> + if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) > >> + continue; > >> + > >> + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > >> + entity->deviceMinor()); > > > > Let's not attempt a lookup of the device node before we locate the > > device in the orphans list, as this may fail. > > It's possible for the device to be ready and not be in the orphans list, > so I think it's okay to keep this here. If it fails then it'll be an > empty string, and we check for that anyway. > > >> + dev_t devnum = makedev(entity->deviceMajor(), > >> + entity->deviceMinor()); > >> + > >> + /* take device from orphan list first, if it is in the list */ > >> + if (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) { > >> + if (deviceNode.empty()) > >> + return -EINVAL; > >> + > >> + ret = entity->setDeviceNode(deviceNode); > >> + if (ret) > >> + return ret; > >> + > >> + orphans_.remove(devnum); > >> + continue; > >> + } > >> + > >> + if (deviceNode.empty() || (ret = ::access(deviceNode.c_str(), R_OK | W_OK)) < 0) { > > > > Assigning variables inside conditions is frowned upon. And I don't think > > there's a need to test ::access() here, as if udev hasn't notified us of > > the device being ready, there's little point in proceding. There's > > already an access() test in MediaEntity::setDeviceNode(). > > I did call and check the return value of setDeviceNode(), but then > there will be a bunch of misleading ERROR log messages complaining about > EPERM, so I think it's better to test ::access() here instead. > > >> + map_[media].push_back(devnum); > >> + revMap_[devnum] = media; > >> + devnumToEntity_[devnum] = entity; > >> + count++; > >> + } > >> + > >> + if (!ret) > > > > If deviceNode is empty, and no device has been previously found in the > > orphaned list, ret will be used uninitialised. > > > >> + entity->setDeviceNode(deviceNode); > > > > I think you can just drop this, the setDeviceNode() call when the device > > is found in the orphaned list should be enough. > > I'm going to keep this, to cover the case that a device is ready but not > in the orphans list. I don't think we need to. It can indeed happen, but udev will notify us of the device being ready right after anyway. Relying on that will simplify the code. > >> + } > >> + > >> + return count; > >> +} > >> + > >> std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor) > >> { > >> struct udev_device *device; > >> @@ -143,17 +224,66 @@ std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor) > >> return deviceNode; > >> } > >> > >> +// add V4L2 device to the media device if it exists, otherwise to the orphan list > >> +// if adding to media device, and it's the last one, then send up the media device > >> +int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > >> +{ > >> + MediaEntity *entity = devnumToEntity_[devnum]; > >> + if (!entity) { > >> + orphans_.push_back(devnum); > >> + return 0; > >> + } > >> + > >> + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), > >> + entity->deviceMinor()); > >> + if (deviceNode.empty()) > >> + return -EINVAL; > >> + > >> + int ret = entity->setDeviceNode(deviceNode); > >> + if (ret) > >> + return ret; > >> + > >> + std::shared_ptr<MediaDevice> media = revMap_[devnum]; > >> + map_[media].remove(devnum); > >> + revMap_.erase(devnum); > >> + devnumToEntity_.erase(devnum); > >> + > >> + if (map_[media].empty()) > >> + addDevice(media); > >> + > >> + return 0; > >> +} > >> + > >> void DeviceEnumeratorUdev::udevNotify(EventNotifier *notifier) > >> { > >> struct udev_device *dev = udev_monitor_receive_device(monitor_); > >> std::string action(udev_device_get_action(dev)); > >> std::string deviceNode(udev_device_get_devnode(dev)); > >> + dev_t deviceNum(udev_device_get_devnum(dev)); > >> > >> LOG(DeviceEnumerator, Debug) > >> << action << " device " << udev_device_get_devnode(dev); > >> > >> if (action == "add") { > >> - addDevice(deviceNode); > >> + const char *subsystem = udev_device_get_subsystem(dev); > >> + if (!subsystem) { > >> + udev_device_unref(dev); > >> + return; > >> + } > >> + > >> + if (!strcmp(subsystem, "media")) { > >> + std::shared_ptr<MediaDevice> media = createDevice(deviceNode); > >> + if (!media) { > >> + udev_device_unref(dev); > >> + return; > >> + } > >> + > >> + int ret = populateMediaDevice(media); > >> + if (ret == 0) > >> + addDevice(media); > > > > As populateMediaDevice() is always followed by addDevice(), should you > > call the latter inside the former ? > > I don't think addDevice() belongs in populateMediaDevice(). Not if the method is named populateMediaDevice(), but maybe we could combine and rename them ? > >> + } else if (!strcmp(subsystem, "video4linux")) { > >> + addV4L2Device(deviceNum); > >> + } > >> } else if (action == "remove") { > >> removeDevice(deviceNode); > > > > > > Should this be conditioned to subsystem == media ? > > Yes. > > >> } > >> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h > >> index 02aec3b..4c3e500 100644 > >> --- a/src/libcamera/include/device_enumerator.h > >> +++ b/src/libcamera/include/device_enumerator.h > >> @@ -44,12 +44,14 @@ public: > >> std::shared_ptr<MediaDevice> search(const DeviceMatch &dm); > >> > >> protected: > >> - int addDevice(const std::string &deviceNode); > >> + std::shared_ptr<MediaDevice> createDevice(const std::string &deviceNode); > >> + int addDevice(std::shared_ptr<MediaDevice> media); > > > > You can pass a const reference to the shared pointer to avoid a copy of > > the shared pointer itself (the MediaDevice it points to is of course not > > copied). Same comment for populateMediaDevice() method. > > addDevice() calls devices_.push_back(), which wants a shared pointer. You could create the shared pointer there though. > I think it's easier to keep it as a shared pointer. populateMediaDevice() > doesn't have this issue, but I think keeping them all uniform makes life > easier. > > >> void removeDevice(const std::string &deviceNode); > >> > >> private: > >> std::vector<std::shared_ptr<MediaDevice>> devices_; > >> > >> + virtual int populateMediaDevice(std::shared_ptr<MediaDevice>) = 0; > > > > Please name the arguments. > > > >> virtual std::string lookupDeviceNode(int major, int minor) = 0; > >> }; > >> > >> diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h > >> index 8d3adc9..d14bbf8 100644 > >> --- a/src/libcamera/include/device_enumerator_sysfs.h > >> +++ b/src/libcamera/include/device_enumerator_sysfs.h > >> @@ -9,6 +9,8 @@ > >> > >> #include <string> > >> > >> +#include "media_device.h" > >> + > >> #include "device_enumerator.h" > >> > >> namespace libcamera { > >> @@ -20,7 +22,8 @@ public: > >> int enumerate(); > >> > >> private: > >> - std::string lookupDeviceNode(int major, int minor); > >> + int populateMediaDevice(std::shared_ptr<MediaDevice> media) final; > > > > Should we make the class itself final instead of marking every single > > method ? Same comment for DeviceEnumeratorUdev. > > populateMediaDevice() is now no longer part of DeviceEnumerator. > Otherwise, I'm not sure. > > >> + std::string lookupDeviceNode(int major, int minor) final; > >> }; > >> > >> } /* namespace libcamera */ > >> diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h > >> index 80f9372..b3bbcb0 100644 > >> --- a/src/libcamera/include/device_enumerator_udev.h > >> +++ b/src/libcamera/include/device_enumerator_udev.h > >> @@ -7,8 +7,14 @@ > >> #ifndef __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__ > >> #define __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__ > >> > >> +#include <list> > >> +#include <map> > >> #include <string> > >> > >> +#include <sys/sysmacros.h> > > > > This header is only needed in the .cpp. Here you can just include > > sys/types.h for dev_t. > > > >> + > >> +#include "media_device.h" > > > > A forward declaration of class MediaDevice should be enough. > > I needed class MediaEntity too. > > >> + > >> #include "device_enumerator.h" > >> > >> struct udev; > >> @@ -32,8 +38,16 @@ private: > >> struct udev_monitor *monitor_; > >> EventNotifier *notifier_; > >> > >> + std::map<std::shared_ptr<MediaDevice>, std::list<dev_t>> map_; > >> + std::map<dev_t, std::shared_ptr<MediaDevice>> revMap_; > >> + std::map<dev_t, MediaEntity *> devnumToEntity_; > >> + > >> + std::list<dev_t> orphans_; > > > > That's quite a few maps and lists, they indeed need better names and/or > > documentation. I think you could also merge the revMap_ and > > devnumToEntity_ maps if you made it a > > std::map<std::pair<std::shared_ptr<MediaDevice>, MediaEntity *> (a bit > > of a long type though). > > I decided to keep the maps separate. With nicer names (deps_, > devnumToDevice_) it ought to be easier to follow. OK, I'll check v2. > > I also wonder if we really need to deal with shared pointers internally, > > or if we could create the shared pointer in the addDevice() method. This > > could be done on top of this patch, but please keep it in mind, I'd like > > your opinion. > > I think it's easier to keep them as shared pointers. For one, what I > mentioned earlier, that addDevice() needs share pointers anyway. Another > is that addV4L2Device(), which is one of the main users of these maps, > calls addDevice(). > > >> + > >> + int populateMediaDevice(std::shared_ptr<MediaDevice> media) final; > >> std::string lookupDeviceNode(int major, int minor) final; > >> > >> + int addV4L2Device(dev_t devnum); > >> void udevNotify(EventNotifier *notifier); > >> }; > >>
diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp index 60c918f..fd5a20c 100644 --- a/src/libcamera/device_enumerator.cpp +++ b/src/libcamera/device_enumerator.cpp @@ -204,7 +204,7 @@ DeviceEnumerator::~DeviceEnumerator() * * \return 0 on success or a negative error code otherwise */ -int DeviceEnumerator::addDevice(const std::string &deviceNode) +std::shared_ptr<MediaDevice> DeviceEnumerator::createDevice(const std::string &deviceNode) { std::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode); @@ -213,33 +213,22 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode) LOG(DeviceEnumerator, Info) << "Unable to populate media device " << deviceNode << " (" << strerror(-ret) << "), skipping"; - return ret; + return nullptr; } LOG(DeviceEnumerator, Debug) << "New media device \"" << media->driver() << "\" created from " << deviceNode; - /* Associate entities to device node paths. */ - for (MediaEntity *entity : media->entities()) { - if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) - continue; - - std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), - entity->deviceMinor()); - if (deviceNode.empty()) - return -EINVAL; - - ret = entity->setDeviceNode(deviceNode); - if (ret) - return ret; - } + return media; +} +int DeviceEnumerator::addDevice(std::shared_ptr<MediaDevice> media) +{ LOG(DeviceEnumerator, Debug) - << "Added device " << deviceNode << ": " << media->driver(); - - devices_.push_back(std::move(media)); + << "Added device " << media->deviceNode() << ": " << media->driver(); + devices_.push_back(media); return 0; } diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp index f3054d5..3b5bc90 100644 --- a/src/libcamera/device_enumerator_sysfs.cpp +++ b/src/libcamera/device_enumerator_sysfs.cpp @@ -71,7 +71,18 @@ int DeviceEnumeratorSysfs::enumerate() continue; } - addDevice(devnode); + std::shared_ptr<MediaDevice> media = createDevice(devnode); + if (!media) { + closedir(dir); + return -ENODEV; + } + + if (populateMediaDevice(media) < 0) { + closedir(dir); + return -ENODEV; + } + + addDevice(media); } closedir(dir); @@ -79,6 +90,26 @@ int DeviceEnumeratorSysfs::enumerate() return 0; } +int DeviceEnumeratorSysfs::populateMediaDevice(std::shared_ptr<MediaDevice> media) +{ + /* Associate entities to device node paths. */ + for (MediaEntity *entity : media->entities()) { + if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) + continue; + + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), + entity->deviceMinor()); + if (deviceNode.empty()) + return -EINVAL; + + int ret = entity->setDeviceNode(deviceNode); + if (ret) + return ret; + } + + return 0; +} + std::string DeviceEnumeratorSysfs::lookupDeviceNode(int major, int minor) { std::string deviceNode; diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp index 86f6ca1..ef9a31d 100644 --- a/src/libcamera/device_enumerator_udev.cpp +++ b/src/libcamera/device_enumerator_udev.cpp @@ -7,6 +7,10 @@ #include "device_enumerator_udev.h" +#include <algorithm> +#include <list> +#include <map> + #include <fcntl.h> #include <libudev.h> #include <string.h> @@ -57,6 +61,11 @@ int DeviceEnumeratorUdev::init() if (ret < 0) return ret; + ret = udev_monitor_filter_add_match_subsystem_devtype(monitor_, "video4linux", + nullptr); + if (ret < 0) + return ret; + return 0; } @@ -74,6 +83,14 @@ int DeviceEnumeratorUdev::enumerate() if (ret < 0) goto done; + ret = udev_enumerate_add_match_subsystem(udev_enum, "video4linux"); + if (ret < 0) + goto done; + + ret = udev_enumerate_add_match_is_initialized(udev_enum); + if (ret < 0) + goto done; + ret = udev_enumerate_scan_devices(udev_enum); if (ret < 0) goto done; @@ -85,6 +102,7 @@ int DeviceEnumeratorUdev::enumerate() udev_list_entry_foreach(ent, ents) { struct udev_device *dev; const char *devnode; + const char *subsystem; const char *syspath = udev_list_entry_get_name(ent); dev = udev_device_new_from_syspath(udev_, syspath); @@ -96,16 +114,36 @@ int DeviceEnumeratorUdev::enumerate() } devnode = udev_device_get_devnode(dev); - if (!devnode) { - udev_device_unref(dev); - ret = -ENODEV; - goto done; + if (!devnode) + goto unref; + + subsystem = udev_device_get_subsystem(dev); + if (!subsystem) + goto unref; + + if (!strcmp(subsystem, "media")) { + std::shared_ptr<MediaDevice> media = createDevice(devnode); + if (!media) + goto unref; + + ret = populateMediaDevice(media); + if (ret == 0) + addDevice(media); + } else if (!strcmp(subsystem, "video4linux")) { + addV4L2Device(udev_device_get_devnum(dev)); + } else { + goto unref; } - addDevice(devnode); + udev_device_unref(dev); + continue; + unref: udev_device_unref(dev); + ret = -ENODEV; + goto done; } + done: udev_enumerate_unref(udev_enum); if (ret < 0) @@ -122,6 +160,49 @@ done: return 0; } +int DeviceEnumeratorUdev::populateMediaDevice(std::shared_ptr<MediaDevice> media) +{ + /* number of pending devnodes */ + int count = 0; + int ret; + + /* Associate entities to device node paths. */ + for (MediaEntity *entity : media->entities()) { + if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0) + continue; + + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), + entity->deviceMinor()); + dev_t devnum = makedev(entity->deviceMajor(), + entity->deviceMinor()); + + /* take device from orphan list first, if it is in the list */ + if (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) { + if (deviceNode.empty()) + return -EINVAL; + + ret = entity->setDeviceNode(deviceNode); + if (ret) + return ret; + + orphans_.remove(devnum); + continue; + } + + if (deviceNode.empty() || (ret = ::access(deviceNode.c_str(), R_OK | W_OK)) < 0) { + map_[media].push_back(devnum); + revMap_[devnum] = media; + devnumToEntity_[devnum] = entity; + count++; + } + + if (!ret) + entity->setDeviceNode(deviceNode); + } + + return count; +} + std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor) { struct udev_device *device; @@ -143,17 +224,66 @@ std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor) return deviceNode; } +// add V4L2 device to the media device if it exists, otherwise to the orphan list +// if adding to media device, and it's the last one, then send up the media device +int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) +{ + MediaEntity *entity = devnumToEntity_[devnum]; + if (!entity) { + orphans_.push_back(devnum); + return 0; + } + + std::string deviceNode = lookupDeviceNode(entity->deviceMajor(), + entity->deviceMinor()); + if (deviceNode.empty()) + return -EINVAL; + + int ret = entity->setDeviceNode(deviceNode); + if (ret) + return ret; + + std::shared_ptr<MediaDevice> media = revMap_[devnum]; + map_[media].remove(devnum); + revMap_.erase(devnum); + devnumToEntity_.erase(devnum); + + if (map_[media].empty()) + addDevice(media); + + return 0; +} + void DeviceEnumeratorUdev::udevNotify(EventNotifier *notifier) { struct udev_device *dev = udev_monitor_receive_device(monitor_); std::string action(udev_device_get_action(dev)); std::string deviceNode(udev_device_get_devnode(dev)); + dev_t deviceNum(udev_device_get_devnum(dev)); LOG(DeviceEnumerator, Debug) << action << " device " << udev_device_get_devnode(dev); if (action == "add") { - addDevice(deviceNode); + const char *subsystem = udev_device_get_subsystem(dev); + if (!subsystem) { + udev_device_unref(dev); + return; + } + + if (!strcmp(subsystem, "media")) { + std::shared_ptr<MediaDevice> media = createDevice(deviceNode); + if (!media) { + udev_device_unref(dev); + return; + } + + int ret = populateMediaDevice(media); + if (ret == 0) + addDevice(media); + } else if (!strcmp(subsystem, "video4linux")) { + addV4L2Device(deviceNum); + } } else if (action == "remove") { removeDevice(deviceNode); } diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h index 02aec3b..4c3e500 100644 --- a/src/libcamera/include/device_enumerator.h +++ b/src/libcamera/include/device_enumerator.h @@ -44,12 +44,14 @@ public: std::shared_ptr<MediaDevice> search(const DeviceMatch &dm); protected: - int addDevice(const std::string &deviceNode); + std::shared_ptr<MediaDevice> createDevice(const std::string &deviceNode); + int addDevice(std::shared_ptr<MediaDevice> media); void removeDevice(const std::string &deviceNode); private: std::vector<std::shared_ptr<MediaDevice>> devices_; + virtual int populateMediaDevice(std::shared_ptr<MediaDevice>) = 0; virtual std::string lookupDeviceNode(int major, int minor) = 0; }; diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h index 8d3adc9..d14bbf8 100644 --- a/src/libcamera/include/device_enumerator_sysfs.h +++ b/src/libcamera/include/device_enumerator_sysfs.h @@ -9,6 +9,8 @@ #include <string> +#include "media_device.h" + #include "device_enumerator.h" namespace libcamera { @@ -20,7 +22,8 @@ public: int enumerate(); private: - std::string lookupDeviceNode(int major, int minor); + int populateMediaDevice(std::shared_ptr<MediaDevice> media) final; + std::string lookupDeviceNode(int major, int minor) final; }; } /* namespace libcamera */ diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h index 80f9372..b3bbcb0 100644 --- a/src/libcamera/include/device_enumerator_udev.h +++ b/src/libcamera/include/device_enumerator_udev.h @@ -7,8 +7,14 @@ #ifndef __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__ #define __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__ +#include <list> +#include <map> #include <string> +#include <sys/sysmacros.h> + +#include "media_device.h" + #include "device_enumerator.h" struct udev; @@ -32,8 +38,16 @@ private: struct udev_monitor *monitor_; EventNotifier *notifier_; + std::map<std::shared_ptr<MediaDevice>, std::list<dev_t>> map_; + std::map<dev_t, std::shared_ptr<MediaDevice>> revMap_; + std::map<dev_t, MediaEntity *> devnumToEntity_; + + std::list<dev_t> orphans_; + + int populateMediaDevice(std::shared_ptr<MediaDevice> media) final; std::string lookupDeviceNode(int major, int minor) final; + int addV4L2Device(dev_t devnum); void udevNotify(EventNotifier *notifier); };
When a MediaDevice is enumerated and populated by the DeviceEnumeratorUdev, there is a possibility that the member device nodes of the media graph would not be ready. The MediaDevice is still passed up to the pipeline handler, where an attempt to access the device nodes will fail in EPERM. This whole issue is especially likely to happen when libcamera is run at system init time. To fix this, we first split DeviceEnumerator::addDevice() into three methods: - createDevice() to simply create the MediaDevice - (abstract method) populateMediaDevice() to populate the MediaDevice - addDevice() to pass the MediaDevice up to the pipeline handler DeviceEnumeratorSysfs calls these methods in succession, similar to what it did before when they were all together as addDevice(). DeviceEnumeratorUdev additionally keeps a map of MediaDevices to a list of pending device nodes (plus some other auxillary maps), and a simple list of orphan device nodes. If a v4l device node is ready and there does not exist any MediaDevice node for it, then it goes to the orphan list, otherwise it is initialized and removed from the pending list of the corresponding MediaDevice in the dependency map. When a MediaDevice is populated via DeviceEnumeratorUdev::populateMediaDevice(), it first checks the orphan list to see if the device node it needs is there, otherwise it tries to initialize the device node and if it fails, then it adds the device node it wants to its list in the dependency map. This allows MediaDevices to be created and initialized properly with udev when v4l device nodes in the media graph may not be ready when the MediaDevice is populated. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- todos: - add documentation - better name for the maps - better name for populateMediaDevice() src/libcamera/device_enumerator.cpp | 27 +--- src/libcamera/device_enumerator_sysfs.cpp | 33 +++- src/libcamera/device_enumerator_udev.cpp | 142 +++++++++++++++++- src/libcamera/include/device_enumerator.h | 4 +- .../include/device_enumerator_sysfs.h | 5 +- .../include/device_enumerator_udev.h | 14 ++ 6 files changed, 197 insertions(+), 28 deletions(-)