[libcamera-devel] [PATCH 4/4] libcamera: device_enumerator_udev: Support entities sharing device nodes

[Kieran Bingham]

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?

And is it needed? or can the loop exit check just compare against it  !=
orphans_.end() ?


> +		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