[libcamera-devel] [PATCH v3] libcamera: device_enumerator: fix udev media graph loading dependency
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Sep 9 01:11:01 CEST 2019
Hi Paul,
Thank you for the patch.
On Sun, Sep 08, 2019 at 01:00:06AM -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 (either not created, or
> without proper permissions set by udev yet). 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
> - 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 nodes it needs are there,
> otherwise it tries to initialize the device nodes and if it fails, then
> it adds the device nodes it wants to its list in the dependency map.
>
> This allows MediaDevice instances 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 at ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Changes in v3:
> - remove adding any V4L2 nodes that are already ready, to prevent any
> potential racing
> - other minor changes
> - we're keeing the name populateMediaDevice()
>
> Changes in v2:
> - add documentation
> - better name for the maps
> - other minor changes
>
> ---
> src/libcamera/device_enumerator.cpp | 54 +++----
> src/libcamera/device_enumerator_sysfs.cpp | 35 ++++-
> src/libcamera/device_enumerator_udev.cpp | 135 +++++++++++++++++-
> src/libcamera/include/device_enumerator.h | 3 +-
> .../include/device_enumerator_sysfs.h | 6 +-
> .../include/device_enumerator_udev.h | 16 +++
> 6 files changed, 216 insertions(+), 33 deletions(-)
>
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index 60c918f..e76438a 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -195,16 +195,21 @@ DeviceEnumerator::~DeviceEnumerator()
> */
>
> /**
> - * \brief Add a media device to the enumerator
> - * \param[in] deviceNode path to the media device to add
> + * \brief Create a media device instance
> + * \param[in] deviceNode path to the media device to create
> *
> - * Create a media device for the \a deviceNode, open it, populate its media graph,
> - * and look up device nodes associated with all entities. Store the media device
> - * in the internal list for later matching with pipeline handlers.
> + * Create a media device for the \a deviceNode, open it, and populate its
> + * media graph. The device enumerator shall then populate the media device by
> + * associating device nodes with entities using MediaEntity::setDeviceNode().
> + * This process is specific to each device enumerator, and the device enumerator
> + * shall ensure that device nodes are ready to be used (for instance, if
> + * applicable, by waiting for device nodes to be created and access permissions
> + * to be set by the system). Once done, it shall add the media device to the
> + * system with addDevice().
> *
> - * \return 0 on success or a negative error code otherwise
> + * \return Created media device instance on success, or nullptr 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,34 +218,31 @@ 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;
> +}
>
> +/**
> + * \brief Add a media device to the enumerator
> + * \param[in] media media device instance to add
> + *
> + * Store the media device in the internal list for later matching with
> + * pipeline handlers. \a media shall be created with createDevice() first.
> + * This method shall be called after all members of the entities of the
> + * media graph have been confirmed to be initialized.
> + */
> +void DeviceEnumerator::addDevice(const 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();
>
> - return 0;
> + devices_.push_back(media);
> }
>
> /**
> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp
> index f3054d5..78a7da8 100644
> --- a/src/libcamera/device_enumerator_sysfs.cpp
> +++ b/src/libcamera/device_enumerator_sysfs.cpp
> @@ -18,6 +18,7 @@
> #include <unistd.h>
>
> #include "log.h"
> +#include "media_device.h"
>
> namespace libcamera {
>
> @@ -32,6 +33,7 @@ int DeviceEnumeratorSysfs::enumerate()
> {
> struct dirent *ent;
> DIR *dir;
> + int ret = 0;
>
> static const char * const sysfs_dirs[] = {
> "/sys/subsystem/media/devices",
> @@ -71,11 +73,42 @@ int DeviceEnumeratorSysfs::enumerate()
> continue;
> }
>
> - addDevice(devnode);
> + std::shared_ptr<MediaDevice> media = createDevice(devnode);
> + if (!media) {
> + ret = -ENODEV;
> + break;
> + }
> +
> + if (populateMediaDevice(media) < 0) {
> + ret = -ENODEV;
> + break;
> + }
> +
> + addDevice(media);
> }
>
> closedir(dir);
>
> + return ret;
> +}
> +
> +int DeviceEnumeratorSysfs::populateMediaDevice(const 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;
> }
>
> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> index 86f6ca1..40853e7 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>
> @@ -17,6 +21,7 @@
> #include <libcamera/event_notifier.h>
>
> #include "log.h"
> +#include "media_device.h"
>
> namespace libcamera {
>
> @@ -57,9 +62,40 @@ 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;
> }
>
> +int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev)
> +{
> + const char *subsystem = udev_device_get_subsystem(dev);
> + if (!subsystem)
> + return -ENODEV;
> +
> + if (!strcmp(subsystem, "media")) {
> + std::shared_ptr<MediaDevice> media =
> + createDevice(udev_device_get_devnode(dev));
> + if (!media)
> + return -ENODEV;
> +
> + int ret = populateMediaDevice(media);
> + if (ret == 0)
> + addDevice(media);
> + return 0;
> + }
> +
> + if (!strcmp(subsystem, "video4linux")) {
> + addV4L2Device(udev_device_get_devnum(dev));
> + return 0;
> + }
> +
> + return -ENODEV;
> +}
> +
> int DeviceEnumeratorUdev::enumerate()
> {
> struct udev_enumerate *udev_enum = nullptr;
> @@ -74,6 +110,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;
> @@ -102,10 +146,12 @@ int DeviceEnumeratorUdev::enumerate()
> goto done;
> }
>
> - addDevice(devnode);
> -
> + ret = addUdevDevice(dev);
> udev_device_unref(dev);
> + if (ret < 0)
> + break;
> }
> +
> done:
> udev_enumerate_unref(udev_enum);
> if (ret < 0)
> @@ -122,6 +168,43 @@ done:
> return 0;
> }
>
> +int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr<MediaDevice> &media)
> +{
> + unsigned int pendingNodes = 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;
> + }
> +
> + deps_[media].push_back(devnum);
> + devnumToDevice_[devnum] = media;
> + devnumToEntity_[devnum] = entity;
> + pendingNodes++;
> + }
> +
> + return pendingNodes;
> +}
> +
> std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor)
> {
> struct udev_device *device;
> @@ -143,6 +226,48 @@ std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor)
> return deviceNode;
> }
>
> +/**
> + * \brief Add a V4L2 device to the media device that it belongs to
> + * \param[in] devnum major:minor number of V4L2 device to add, as a dev_t
> + *
> + * Add V4L2 device identified by \a devnum to the MediaDevice that it belongs
> + * to, if such a MediaDevice has been created. Otherwise add the V4L2 device
> + * to the orphan list. If the V4L2 device is added to a MediaDevice, and it is
> + * the last V4L2 device that the MediaDevice needs, then the MediaDevice is
> + * added to the DeviceEnumerator, where it is available for pipeline handlers.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +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 = devnumToDevice_[devnum];
> + deps_[media].remove(devnum);
> + devnumToDevice_.erase(devnum);
> + devnumToEntity_.erase(devnum);
> +
> + if (deps_[media].empty()) {
> + addDevice(media);
> + deps_.erase(media);
> + }
> +
> + return 0;
> +}
> +
> void DeviceEnumeratorUdev::udevNotify(EventNotifier *notifier)
> {
> struct udev_device *dev = udev_monitor_receive_device(monitor_);
> @@ -153,9 +278,11 @@ void DeviceEnumeratorUdev::udevNotify(EventNotifier *notifier)
> << action << " device " << udev_device_get_devnode(dev);
>
> if (action == "add") {
> - addDevice(deviceNode);
> + addUdevDevice(dev);
> } else if (action == "remove") {
> - removeDevice(deviceNode);
> + const char *subsystem = udev_device_get_subsystem(dev);
> + if (subsystem && !strcmp(subsystem, "media"))
> + removeDevice(deviceNode);
> }
>
> udev_device_unref(dev);
> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h
> index 02aec3b..c5d26f1 100644
> --- a/src/libcamera/include/device_enumerator.h
> +++ b/src/libcamera/include/device_enumerator.h
> @@ -44,7 +44,8 @@ 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);
> + void addDevice(const std::shared_ptr<MediaDevice> &media);
> void removeDevice(const std::string &deviceNode);
>
> private:
> diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h
> index 8d3adc9..242b22b 100644
> --- a/src/libcamera/include/device_enumerator_sysfs.h
> +++ b/src/libcamera/include/device_enumerator_sysfs.h
> @@ -7,10 +7,13 @@
> #ifndef __LIBCAMERA_DEVICE_ENUMERATOR_SYSFS_H__
> #define __LIBCAMERA_DEVICE_ENUMERATOR_SYSFS_H__
>
> +#include <memory>
> #include <string>
>
> #include "device_enumerator.h"
>
> +class MediaDevice;
> +
> namespace libcamera {
>
> class DeviceEnumeratorSysfs final : public DeviceEnumerator
> @@ -20,7 +23,8 @@ public:
> int enumerate();
>
> private:
> - std::string lookupDeviceNode(int major, int minor);
> + int populateMediaDevice(const std::shared_ptr<MediaDevice> &media);
> + 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..5bdcdea 100644
> --- a/src/libcamera/include/device_enumerator_udev.h
> +++ b/src/libcamera/include/device_enumerator_udev.h
> @@ -7,16 +7,23 @@
> #ifndef __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__
> #define __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__
>
> +#include <list>
> +#include <map>
> +#include <memory>
> #include <string>
> +#include <sys/types.h>
>
> #include "device_enumerator.h"
>
> struct udev;
> +struct udev_device;
> struct udev_monitor;
>
> namespace libcamera {
>
> class EventNotifier;
> +class MediaDevice;
> +class MediaEntity;
>
> class DeviceEnumeratorUdev : public DeviceEnumerator
> {
> @@ -32,8 +39,17 @@ private:
> struct udev_monitor *monitor_;
> EventNotifier *notifier_;
>
> + std::map<std::shared_ptr<MediaDevice>, std::list<dev_t>> deps_;
> + std::map<dev_t, std::shared_ptr<MediaDevice>> devnumToDevice_;
> + std::map<dev_t, MediaEntity *> devnumToEntity_;
> +
> + std::list<dev_t> orphans_;
> +
> + int addUdevDevice(struct udev_device *dev);
> + int populateMediaDevice(const std::shared_ptr<MediaDevice> &media);
> std::string lookupDeviceNode(int major, int minor) final;
>
> + int addV4L2Device(dev_t devnum);
> void udevNotify(EventNotifier *notifier);
> };
>
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list