[3/3] libcamera: device_enumerator_udev: Defer invalid media devices

24168 diff mbox series

A MediaDevice created by DeviceEnumeratorUdev may have an invalid
topology if the device nodes for some of its entities have not yet
been created. If that's the case, defer initialisation of the
MediaDevice. Re-try all deferred media devices whenever a new device
is added to see if they're ready to be initialised yet.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 .../internal/device_enumerator_udev.h         |  2 +
 src/libcamera/device_enumerator.cpp           |  2 +-
 src/libcamera/device_enumerator_udev.cpp      | 37 +++++++++++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

Hi

2025. 08. 20. 15:23 keltezéssel, Daniel Scally írta:
> A MediaDevice created by DeviceEnumeratorUdev may have an invalid
> topology if the device nodes for some of its entities have not yet
> been created. If that's the case, defer initialisation of the
> MediaDevice. Re-try all deferred media devices whenever a new device
> is added to see if they're ready to be initialised yet.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>   .../internal/device_enumerator_udev.h         |  2 +
>   src/libcamera/device_enumerator.cpp           |  2 +-
>   src/libcamera/device_enumerator_udev.cpp      | 37 +++++++++++++++++++
>   3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h
> index a5e5e6efe..b76315c63 100644
> --- a/include/libcamera/internal/device_enumerator_udev.h
> +++ b/include/libcamera/internal/device_enumerator_udev.h
> @@ -13,6 +13,7 @@
>   #include <set>
>   #include <string>
>   #include <sys/types.h>
> +#include <vector>
>   
>   #include "libcamera/internal/device_enumerator.h"
>   
> @@ -67,6 +68,7 @@ private:
>   	EventNotifier *notifier_;
>   
>   	std::set<dev_t> orphans_;
> +	std::vector<std::unique_ptr<MediaDevice>> topologyPending_;
>   	std::list<MediaDeviceDeps> pending_;
>   	std::map<dev_t, MediaDeviceDeps *> devMap_;
>   };
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index ae17862f6..ab7bceec4 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -220,7 +220,7 @@ std::unique_ptr<MediaDevice> DeviceEnumerator::createDevice(const std::string &d
>   	std::unique_ptr<MediaDevice> media = std::make_unique<MediaDevice>(deviceNode);
>   
>   	int ret = media->populate();
> -	if (ret < 0) {
> +	if (ret < 0 && ret != -EAGAIN) {
>   		LOG(DeviceEnumerator, Info)
>   			<< "Unable to populate media device " << deviceNode
>   			<< " (" << strerror(-ret) << "), skipping";
> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> index 53b4fac80..631e583e7 100644
> --- a/src/libcamera/device_enumerator_udev.cpp
> +++ b/src/libcamera/device_enumerator_udev.cpp
> @@ -82,6 +82,14 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev)
>   		if (!media)
>   			return -ENODEV;
>   
> +		if (!media->isValid()) {
> +			LOG(DeviceEnumerator, Debug)
> +				<< "Defer media device " << media->deviceNode()
> +				<< " due to an invalid topology";
> +			topologyPending_.emplace_back(std::move(media));
> +			return 0;
> +		}
> +
>   		return initMediaDevice(std::move(media));
>   	}
>   
> @@ -353,6 +361,35 @@ void DeviceEnumeratorUdev::udevNotify()
>   		<< action << " device " << deviceNode;
>   
>   	if (action == "add") {
> +		/*
> +		* The addition of a new device may signal that a previously
> +		* deferred media device has had its topology updated to the
> +		* extent that it's now valid - retry devices deferred due to
> +		* invalid topology to see if they're now ready.
> +		*/
> +
> +		LOG(DeviceEnumerator, Debug)
> +			<< "Re-evaluating " << topologyPending_.size()
> +			<< " deferred media devices";
> +
> +		for (auto media = topologyPending_.begin();
> +		     media != topologyPending_.end(); ++media) {
> +			LOG(DeviceEnumerator, Debug)
> +				<< "Evaluating media device " << (*media)->deviceNode()
> +				<< " again...";
> +
> +			int ret = (*media)->populate();
> +			if (ret == -EAGAIN) {
> +				LOG(DeviceEnumerator, Debug)
> +					<< "Media device " << (*media)->deviceNode()
> +					<< " still has invalid topology";
> +				continue;
> +			}
> +
> +			initMediaDevice(std::move(*media));
> +			topologyPending_.erase(media);

I'm not sure this is correct. Generally one is not allowed to modify containers
during iteration. If you omit this erase, after the loop, I believe something
like this could be done:

   topologyPending_.erase(std::remove(topologyPending_.begin(), topologyPending_.end(), nullptr),
                          topologyPending_.end());


Regards,
Barnabás Pőcze

> +		}
> +
>   		addUdevDevice(dev);
>   	} else if (action == "remove") {
>   		const char *subsystem = udev_device_get_subsystem(dev);