[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")

This fixes the issue I reported.

Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

And I can't see anything glaring out at me in the code - so LGTM:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> 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;) {
> +		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