[libcamera-devel] [PATCH 4/4] libcamera: device_enumerator_udev: Support entities sharing device nodes
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Sep 13 12:21:33 CEST 2019
On 13/09/2019 11:10, Kieran Bingham wrote:
> Hi Laurent,
>
> On 12/09/2019 21:03, Laurent Pinchart wrote:
>> Some media devices, such as V4L2 M2M devices, share the same device node
>> for multiple entities. The udev enumerator used to support this, but
>> commit 6e620349009d ("libcamera: device_enumerator: fix udev media graph
>> loading dependency") broke this.
>>
>> To fix the problem, rework the media device to V4L2 devices matching
>> code. A new MediaDeviceDeps internal struct stores unmet device number
>> dependencies for a media device, and is stored in a list of pending
>> media devices. To avoid linear lookups, the dependencies are cached in a
>> reverse map of device number to media device dependencies.
>>
>> Fixes: 6e620349009d ("libcamera: device_enumerator: fix udev media graph loading dependency")
>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> ---
>> src/libcamera/device_enumerator_udev.cpp | 100 ++++++++++++------
>> .../include/device_enumerator_udev.h | 25 ++++-
>> 2 files changed, 89 insertions(+), 36 deletions(-)
>>
>> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
>> index c40770911d3d..ddcd59ea52c1 100644
>> --- a/src/libcamera/device_enumerator_udev.cpp
>> +++ b/src/libcamera/device_enumerator_udev.cpp
>> @@ -170,8 +170,8 @@ done:
>>
>> int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr<MediaDevice> &media)
>> {
>> - unsigned int pendingNodes = 0;
>> - int ret;
>> + std::set<dev_t> children;
>> + DependencyMap deps;
>>
>> /* Associate entities to device node paths. */
>> for (MediaEntity *entity : media->entities()) {
>> @@ -181,28 +181,50 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr<MediaDevice>
>> dev_t devnum = makedev(entity->deviceMajor(),
>> entity->deviceMinor());
>>
>> - /* Take device from orphan list first, if it is in the list. */
>> - auto orphan = std::find(orphans_.begin(), orphans_.end(), devnum);
>> - if (orphan != orphans_.end()) {
>> - std::string deviceNode = lookupDeviceNode(devnum);
>> - if (deviceNode.empty())
>> - return -EINVAL;
>> -
>> - ret = entity->setDeviceNode(deviceNode);
>> - if (ret)
>> - return ret;
>> -
>> - orphans_.erase(orphan);
>> + /*
>> + * If the devnum isn't in the orphans list, add it to the unmet
>> + * dependencies.
>> + */
>> + if (orphans_.find(devnum) == orphans_.end()) {
>> + deps[devnum].push_back(entity);
>> continue;
>> }
>>
>> - deps_[media].push_back(devnum);
>> - devnumToDevice_[devnum] = media;
>> - devnumToEntity_[devnum] = entity;
>> - pendingNodes++;
>> + /*
>> + * Otherwise take it from the orphans list. Don't remove the
>> + * entry from the list yet as other entities in this media
>> + * device may need the same device.
>> + */
>> + std::string deviceNode = lookupDeviceNode(devnum);
>> + if (deviceNode.empty())
>> + return -EINVAL;
>> +
>> + int ret = entity->setDeviceNode(deviceNode);
>> + if (ret)
>> + return ret;
>> +
>> + children.insert(devnum);
>> + }
>> +
>> + /* Remove all found children from the orphans list. */
>> + for (auto it = orphans_.begin(), last = orphans_.end(); it != last;) {
>
> What 'type' is last? Where is it declared? Is this it's only use?
>
> Is it automatically defined as an auto? or an iterator?
Ah - sorry I was confused reading it, It's the same as :
int a = 1, b = 2;
>
> And is it needed? or can the loop exit check just compare against it !=
> orphans_.end() ?
And I think it's needed because we're modifying the iterators with the
.erase(it)...
--
Kieran
>> + if (children.find(*it) != children.end())
>> + it = orphans_.erase(it);
>> + else
>> + ++it;
>> + }
>> +
>> + /*
>> + * If the media device has unmet dependencies, add it to the pending
>> + * list and update the devnum map accordingly.
>> + */
>> + if (!deps.empty()) {
>> + pending_.emplace_back(media, deps);
>> + for (const auto &dep : deps)
>> + devMap_[dep.first] = &pending_.back();
>> }
>>
>> - return pendingNodes;
>> + return deps.size();
>> }
>>
>> /**
>> @@ -247,28 +269,42 @@ std::string DeviceEnumeratorUdev::lookupDeviceNode(dev_t devnum)
>> */
>> int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)
>> {
>> - MediaEntity *entity = devnumToEntity_[devnum];
>> - if (!entity) {
>> - orphans_.push_back(devnum);
>> + /*
>> + * If the devnum doesn't belong to any media device, add it to the
>> + * orphans list.
>> + */
>> + auto it = devMap_.find(devnum);
>> + if (it == devMap_.end()) {
>> + orphans_.insert(devnum);
>> return 0;
>> }
>>
>> + /*
>> + * Set the device node for all entities matching the devnum. Multiple
>> + * entities can share the same device node, for instance for V4L2 M2M
>> + * devices.
>> + */
>> std::string deviceNode = lookupDeviceNode(devnum);
>> if (deviceNode.empty())
>> return -EINVAL;
>>
>> - int ret = entity->setDeviceNode(deviceNode);
>> - if (ret)
>> - return ret;
>> + MediaDeviceDeps *deps = it->second;
>> + for (MediaEntity *entity : deps->deps_[devnum]) {
>> + 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);
>> + /*
>> + * Remove the devnum from the unmet dependencies for this media device.
>> + * If no more dependency is unmet, add the media device to the
>> + * enumerator.
>> + */
>> + deps->deps_.erase(devnum);
>>
>> - if (deps_[media].empty()) {
>> - addDevice(media);
>> - deps_.erase(media);
>> + if (deps->deps_.empty()) {
>> + addDevice(deps->media_);
>> + pending_.remove(*deps);
>> }
>>
>> return 0;
>> diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h
>> index fb7cac8011a1..6d8268620185 100644
>> --- a/src/libcamera/include/device_enumerator_udev.h
>> +++ b/src/libcamera/include/device_enumerator_udev.h
>> @@ -10,6 +10,7 @@
>> #include <list>
>> #include <map>
>> #include <memory>
>> +#include <set>
>> #include <string>
>> #include <sys/types.h>
>>
>> @@ -39,11 +40,27 @@ 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_;
>> + using DependencyMap = std::map<dev_t, std::list<MediaEntity *>>;
>>
>> - std::list<dev_t> orphans_;
>> + struct MediaDeviceDeps {
>> + MediaDeviceDeps(const std::shared_ptr<MediaDevice> &media,
>> + const DependencyMap &deps)
>> + : media_(media), deps_(deps)
>> + {
>> + }
>> +
>> + bool operator==(const MediaDeviceDeps &other) const
>> + {
>> + return media_ == other.media_;
>> + }
>> +
>> + std::shared_ptr<MediaDevice> media_;
>> + DependencyMap deps_;
>> + };
>> +
>> + std::set<dev_t> orphans_;
>> + std::list<MediaDeviceDeps> pending_;
>> + std::map<dev_t, MediaDeviceDeps *> devMap_;
>>
>> int addUdevDevice(struct udev_device *dev);
>> int populateMediaDevice(const std::shared_ptr<MediaDevice> &media);
>>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list