[libcamera-devel] [RFC PATCH] libcamera: device_enumerator: fix udev media graph loading dependency
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Sep 2 09:55:35 CEST 2019
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 at 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);
> >> };
> >>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list