[libcamera-devel] [RFC PATCH] libcamera: device_enumerator: fix udev media graph loading dependency
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Aug 28 12:02:01 CEST 2019
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 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 ?
> +
> + 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);
> };
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list