[libcamera-devel,v6,2/9] libcamera: device_enumerator_udev: Add specialization for sysfs path

9156 diff mbox series

As udev may rename the V4L2 devices in /dev add a specialization that
asks udev for the sysfs path instead of using the default method that
makes the assumption that V4L2 devices are never renamed.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 .../internal/device_enumerator_udev.h         |  2 ++
 src/libcamera/device_enumerator_udev.cpp      | 24 +++++++++++++++++++
 2 files changed, 26 insertions(+)

Hi Niklas,

On Mon, Aug 03, 2020 at 11:17:26PM +0200, Niklas Söderlund wrote:
> As udev may rename the V4L2 devices in /dev add a specialization that
> asks udev for the sysfs path instead of using the default method that
> makes the assumption that V4L2 devices are never renamed.
>
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  .../internal/device_enumerator_udev.h         |  2 ++
>  src/libcamera/device_enumerator_udev.cpp      | 24 +++++++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h
> index 6f45be0c1c423d02..595b16acb89d98bb 100644
> --- a/include/libcamera/internal/device_enumerator_udev.h
> +++ b/include/libcamera/internal/device_enumerator_udev.h
> @@ -35,6 +35,8 @@ public:
>  	int init();
>  	int enumerate();
>
> +	std::string lookupSysfsPath(const std::string &deviceNode);

override

> +
>  private:
>  	using DependencyMap = std::map<dev_t, std::list<MediaEntity *>>;
>
> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> index 96689daa5dd113dc..2e43b84f62e0333d 100644
> --- a/src/libcamera/device_enumerator_udev.cpp
> +++ b/src/libcamera/device_enumerator_udev.cpp
> @@ -14,6 +14,7 @@
>  #include <map>
>  #include <string.h>
>  #include <sys/ioctl.h>
> +#include <sys/stat.h>
>  #include <sys/sysmacros.h>
>  #include <unistd.h>
>
> @@ -193,6 +194,29 @@ done:
>  	return 0;
>  }
>
> +std::string DeviceEnumeratorUdev::lookupSysfsPath(const std::string &deviceNode)

I might be confused, but if we construct the udev device from
'/dev/video0' to avoid assuming '/dev/video0' is stable, what do we
gain ?

> +{
> +	struct stat st;
> +	int ret = stat(deviceNode.c_str(), &st);
> +	if (ret < 0) {
> +		LOG(DeviceEnumerator, Error)
> +			<< "Unable to stat '" << deviceNode << "'";

I think printing out why stat failed has value.

                        << strerror(ret);

> +		return {};
> +	}
> +
> +	struct udev_device *dev = udev_device_new_from_devnum(udev_, 'c',
> +							      st.st_rdev);
> +	if (!dev) {
> +		LOG(DeviceEnumerator, Error)
> +			<< "Unable to make device from devnum " << st.st_rdev;
> +		return {};
> +	}
> +
> +	std::string path{ udev_device_get_syspath(dev) };

I'm curious, why do you use aggregate initialization for strings
instead of the constructor ?

> +	udev_device_unref(dev);
> +	return path;
> +}
> +
>  int DeviceEnumeratorUdev::populateMediaDevice(MediaDevice *media, DependencyMap *deps)
>  {
>  	std::set<dev_t> children;
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Tue, Aug 04, 2020 at 09:35:18AM +0200, Jacopo Mondi wrote:
> On Mon, Aug 03, 2020 at 11:17:26PM +0200, Niklas Söderlund wrote:
> > As udev may rename the V4L2 devices in /dev add a specialization that
> > asks udev for the sysfs path instead of using the default method that
> > makes the assumption that V4L2 devices are never renamed.
> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  .../internal/device_enumerator_udev.h         |  2 ++
> >  src/libcamera/device_enumerator_udev.cpp      | 24 +++++++++++++++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h
> > index 6f45be0c1c423d02..595b16acb89d98bb 100644
> > --- a/include/libcamera/internal/device_enumerator_udev.h
> > +++ b/include/libcamera/internal/device_enumerator_udev.h
> > @@ -35,6 +35,8 @@ public:
> >  	int init();
> >  	int enumerate();
> >
> > +	std::string lookupSysfsPath(const std::string &deviceNode);
> 
> override
> 
> > +
> >  private:
> >  	using DependencyMap = std::map<dev_t, std::list<MediaEntity *>>;
> >
> > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> > index 96689daa5dd113dc..2e43b84f62e0333d 100644
> > --- a/src/libcamera/device_enumerator_udev.cpp
> > +++ b/src/libcamera/device_enumerator_udev.cpp
> > @@ -14,6 +14,7 @@
> >  #include <map>
> >  #include <string.h>
> >  #include <sys/ioctl.h>
> > +#include <sys/stat.h>
> >  #include <sys/sysmacros.h>
> >  #include <unistd.h>
> >
> > @@ -193,6 +194,29 @@ done:
> >  	return 0;
> >  }
> >
> > +std::string DeviceEnumeratorUdev::lookupSysfsPath(const std::string &deviceNode)
> 
> I might be confused, but if we construct the udev device from
> '/dev/video0' to avoid assuming '/dev/video0' is stable, what do we
> gain ?

This will give us the real sysfs path for the device. For instance
/sys/devices/pci0000:00/0000:00:14.0/usb1/1-5/1-5:1.0/video4linux/video0.
The video0 in that string is the kernel device name, so it's not
influenced by udev rules. This function will return the same string
regardless of whether deviceNode has been renamed by udev or not. It's
not about stable names, it's about finding the correct device regardless
of udev rules.

> > +{
> > +	struct stat st;
> > +	int ret = stat(deviceNode.c_str(), &st);
> > +	if (ret < 0) {
> > +		LOG(DeviceEnumerator, Error)
> > +			<< "Unable to stat '" << deviceNode << "'";
> 
> I think printing out why stat failed has value.
> 
>                         << strerror(ret);

Should be

	ret = -errno;
	LOG(...) << ... << strerror(-ret);

> > +		return {};
> > +	}
> > +
> > +	struct udev_device *dev = udev_device_new_from_devnum(udev_, 'c',
> > +							      st.st_rdev);
> > +	if (!dev) {
> > +		LOG(DeviceEnumerator, Error)
> > +			<< "Unable to make device from devnum " << st.st_rdev;
> > +		return {};
> > +	}
> > +
> > +	std::string path{ udev_device_get_syspath(dev) };
> 
> I'm curious, why do you use aggregate initialization for strings
> instead of the constructor ?

I think this came from code I've provided. This isn't aggregate
initialization but direct list initialization ([1]). Compared to direct
initialization with parentheses ([2]), list initialization limits the
allowed implicit conversions by prohibiting narrowing (e.g. converting a
float to an integer). It's thus preferred in many cases to catch issues
at compile time. There are however drawbacks, especially when using the
auto keyword, but also in the fact that different constructors will be
selected by default. For instance,

	std::vector<int> v1(10, 1);

will create a vector of 10 elements initialized to 1, while

	std::vector<int> v2{10, 1};

will create a vector of two elements, 10 and 1.

[1] https://en.cppreference.com/w/cpp/language/list_initialization
[2] https://en.cppreference.com/w/cpp/language/direct_initialization

> > +	udev_device_unref(dev);
> > +	return path;
> > +}
> > +
> >  int DeviceEnumeratorUdev::populateMediaDevice(MediaDevice *media, DependencyMap *deps)
> >  {
> >  	std::set<dev_t> children;