[libcamera-devel] [RFC PATCH] libcamera: device_enumerator: fix udev media graph loading dependency
Paul Elder
paul.elder at ideasonboard.com
Mon Sep 2 06:15:17 CEST 2019
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 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.
> > +
> > + 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
More information about the libcamera-devel
mailing list