[libcamera-devel,v2,06/12] libcamera: device_enumerator: add DeviceEnumeratorUdev class
diff mbox series

Message ID 20181229032855.26249-7-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • Add basic camera enumeration
Related show

Commit Message

Niklas Söderlund Dec. 29, 2018, 3:28 a.m. UTC 
Provide a DeviceEnumeratorUdev class which is a specialization
of DeviceEnumerator which uses udev to enumerate information in the
system.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
* Changes since v1
- s/NULL/nullptr/
- s/foo == NULL/!foo/
- Break log messages to not exceed 80 chars.
- Return a std::string from lookupDevnode().
---
 src/libcamera/device_enumerator.cpp       | 98 +++++++++++++++++++++++
 src/libcamera/include/device_enumerator.h | 15 ++++
 2 files changed, 113 insertions(+)

Comments

Jacopo Mondi Dec. 30, 2018, 10:07 a.m. UTC | #1 
Hi Niklas,

On Sat, Dec 29, 2018 at 04:28:49AM +0100, Niklas Söderlund wrote:
> Provide a DeviceEnumeratorUdev class which is a specialization
> of DeviceEnumerator which uses udev to enumerate information in the
> system.
>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Thanks
  j

> ---
> * Changes since v1
> - s/NULL/nullptr/
> - s/foo == NULL/!foo/
> - Break log messages to not exceed 80 chars.
> - Return a std::string from lookupDevnode().
> ---
>  src/libcamera/device_enumerator.cpp       | 98 +++++++++++++++++++++++
>  src/libcamera/include/device_enumerator.h | 15 ++++
>  2 files changed, 113 insertions(+)
>
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index df9e89a1afeecda1..3cafd0d3703dac99 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -266,4 +266,102 @@ DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const
>  	return info;
>  }
>
> +/* -----------------------------------------------------------------------------
> + * Enumerator Udev
> + */
> +
> +DeviceEnumeratorUdev::DeviceEnumeratorUdev()
> +	: udev_(nullptr)
> +{
> +}
> +
> +DeviceEnumeratorUdev::~DeviceEnumeratorUdev()
> +{
> +	if (udev_)
> +		udev_unref(udev_);
> +}
> +
> +int DeviceEnumeratorUdev::init()
> +{
> +	if (udev_)
> +		return -EBUSY;
> +
> +	udev_ = udev_new();
> +	if (!udev_)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +int DeviceEnumeratorUdev::enumerate()
> +{
> +	struct udev_enumerate *udev_enum = nullptr;
> +	struct udev_list_entry *ents, *ent;
> +	int ret;
> +
> +	udev_enum = udev_enumerate_new(udev_);
> +	if (!udev_enum)
> +		return -ENOMEM;
> +
> +	ret = udev_enumerate_add_match_subsystem(udev_enum, "media");
> +	if (ret < 0)
> +		goto done;
> +
> +	ret = udev_enumerate_scan_devices(udev_enum);
> +	if (ret < 0)
> +		goto done;
> +
> +	ents = udev_enumerate_get_list_entry(udev_enum);
> +	if (!ents)
> +		goto done;
> +
> +	udev_list_entry_foreach(ent, ents) {
> +		struct udev_device *dev;
> +		const char *devnode;
> +		const char *syspath = udev_list_entry_get_name(ent);
> +
> +		dev = udev_device_new_from_syspath(udev_, syspath);
> +		if (!dev) {
> +			LOG(Error) << "Failed to get device for '" <<
> +				   syspath << "', skipping";
> +			continue;
> +		}
> +
> +		devnode = udev_device_get_devnode(dev);
> +		if (!devnode) {
> +			udev_device_unref(dev);
> +			ret = -ENODEV;
> +			goto done;
> +		}
> +
> +		addDevice(devnode);
> +
> +		udev_device_unref(dev);
> +	}
> +done:
> +	udev_enumerate_unref(udev_enum);
> +	return ret >= 0 ? 0 : ret;
> +}
> +
> +std::string DeviceEnumeratorUdev::lookupDevnode(int major, int minor)
> +{
> +	struct udev_device *device;
> +	const char *name;
> +	dev_t devnum;
> +	std::string devnode("");
> +
> +	devnum = makedev(major, minor);
> +	device = udev_device_new_from_devnum(udev_, 'c', devnum);
> +	if (!device)
> +		return "";
> +
> +	name = udev_device_get_devnode(device);
> +	if (name)
> +		devnode = name;
> +
> +	udev_device_unref(device);
> +
> +	return devnode;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h
> index 1f8cef3311012308..5348e6cf583dbd15 100644
> --- a/src/libcamera/include/device_enumerator.h
> +++ b/src/libcamera/include/device_enumerator.h
> @@ -75,6 +75,21 @@ private:
>  	virtual std::string lookupDevnode(int major, int minor) = 0;
>  };
>
> +class DeviceEnumeratorUdev: public DeviceEnumerator
> +{
> +public:
> +	DeviceEnumeratorUdev();
> +	~DeviceEnumeratorUdev();
> +
> +	int init() final;
> +	int enumerate() final;
> +
> +private:
> +	struct udev *udev_;
> +
> +	std::string lookupDevnode(int major, int minor) final;
> +};
> +
>  } /* namespace libcamera */
>
>  #endif	/* __LIBCAMERA_DEVICE_ENUMERATOR_H__ */
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Dec. 30, 2018, 8:41 p.m. UTC | #2 
Hi Niklas,

Thank you for the patch.

On Saturday, 29 December 2018 05:28:49 EET Niklas Söderlund wrote:
> Provide a DeviceEnumeratorUdev class which is a specialization
> of DeviceEnumerator which uses udev to enumerate information in the
> system.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * Changes since v1
> - s/NULL/nullptr/
> - s/foo == NULL/!foo/
> - Break log messages to not exceed 80 chars.
> - Return a std::string from lookupDevnode().
> ---
>  src/libcamera/device_enumerator.cpp       | 98 +++++++++++++++++++++++
>  src/libcamera/include/device_enumerator.h | 15 ++++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/src/libcamera/device_enumerator.cpp
> b/src/libcamera/device_enumerator.cpp index
> df9e89a1afeecda1..3cafd0d3703dac99 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -266,4 +266,102 @@ DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm)
> const return info;
>  }
> 
> +/*
> ---------------------------------------------------------------------------
> -- + * Enumerator Udev
> + */
> +
> +DeviceEnumeratorUdev::DeviceEnumeratorUdev()
> +	: udev_(nullptr)
> +{
> +}
> +
> +DeviceEnumeratorUdev::~DeviceEnumeratorUdev()
> +{
> +	if (udev_)
> +		udev_unref(udev_);
> +}
> +
> +int DeviceEnumeratorUdev::init()
> +{
> +	if (udev_)
> +		return -EBUSY;
> +
> +	udev_ = udev_new();
> +	if (!udev_)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +int DeviceEnumeratorUdev::enumerate()
> +{
> +	struct udev_enumerate *udev_enum = nullptr;
> +	struct udev_list_entry *ents, *ent;
> +	int ret;
> +
> +	udev_enum = udev_enumerate_new(udev_);
> +	if (!udev_enum)
> +		return -ENOMEM;
> +
> +	ret = udev_enumerate_add_match_subsystem(udev_enum, "media");
> +	if (ret < 0)
> +		goto done;
> +
> +	ret = udev_enumerate_scan_devices(udev_enum);
> +	if (ret < 0)
> +		goto done;
> +
> +	ents = udev_enumerate_get_list_entry(udev_enum);
> +	if (!ents)
> +		goto done;
> +
> +	udev_list_entry_foreach(ent, ents) {
> +		struct udev_device *dev;
> +		const char *devnode;
> +		const char *syspath = udev_list_entry_get_name(ent);
> +
> +		dev = udev_device_new_from_syspath(udev_, syspath);
> +		if (!dev) {
> +			LOG(Error) << "Failed to get device for '" <<
> +				   syspath << "', skipping";
> +			continue;
> +		}
> +
> +		devnode = udev_device_get_devnode(dev);
> +		if (!devnode) {
> +			udev_device_unref(dev);
> +			ret = -ENODEV;
> +			goto done;
> +		}
> +
> +		addDevice(devnode);
> +
> +		udev_device_unref(dev);
> +	}
> +done:
> +	udev_enumerate_unref(udev_enum);
> +	return ret >= 0 ? 0 : ret;
> +}
> +
> +std::string DeviceEnumeratorUdev::lookupDevnode(int major, int minor)
> +{
> +	struct udev_device *device;
> +	const char *name;
> +	dev_t devnum;
> +	std::string devnode("");

Wouldn't std::string devnode(); be a little more efficient as it won't need to 
parse the const char * ?

> +
> +	devnum = makedev(major, minor);
> +	device = udev_device_new_from_devnum(udev_, 'c', devnum);
> +	if (!device)
> +		return "";

Same here, return std::string() ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	name = udev_device_get_devnode(device);
> +	if (name)
> +		devnode = name;
> +
> +	udev_device_unref(device);
> +
> +	return devnode;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/device_enumerator.h
> b/src/libcamera/include/device_enumerator.h index
> 1f8cef3311012308..5348e6cf583dbd15 100644
> --- a/src/libcamera/include/device_enumerator.h
> +++ b/src/libcamera/include/device_enumerator.h
> @@ -75,6 +75,21 @@ private:
>  	virtual std::string lookupDevnode(int major, int minor) = 0;
>  };
> 
> +class DeviceEnumeratorUdev: public DeviceEnumerator
> +{
> +public:
> +	DeviceEnumeratorUdev();
> +	~DeviceEnumeratorUdev();
> +
> +	int init() final;
> +	int enumerate() final;
> +
> +private:
> +	struct udev *udev_;
> +
> +	std::string lookupDevnode(int major, int minor) final;
> +};
> +
>  } /* namespace libcamera */
> 
>  #endif	/* __LIBCAMERA_DEVICE_ENUMERATOR_H__ */
Niklas Söderlund Dec. 30, 2018, 9 p.m. UTC | #3 
Hi Laurent,

Thanks for your feedback.

On 2018-12-30 22:41:09 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Saturday, 29 December 2018 05:28:49 EET Niklas Söderlund wrote:
> > Provide a DeviceEnumeratorUdev class which is a specialization
> > of DeviceEnumerator which uses udev to enumerate information in the
> > system.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> > * Changes since v1
> > - s/NULL/nullptr/
> > - s/foo == NULL/!foo/
> > - Break log messages to not exceed 80 chars.
> > - Return a std::string from lookupDevnode().
> > ---
> >  src/libcamera/device_enumerator.cpp       | 98 +++++++++++++++++++++++
> >  src/libcamera/include/device_enumerator.h | 15 ++++
> >  2 files changed, 113 insertions(+)
> > 
> > diff --git a/src/libcamera/device_enumerator.cpp
> > b/src/libcamera/device_enumerator.cpp index
> > df9e89a1afeecda1..3cafd0d3703dac99 100644
> > --- a/src/libcamera/device_enumerator.cpp
> > +++ b/src/libcamera/device_enumerator.cpp
> > @@ -266,4 +266,102 @@ DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm)
> > const return info;
> >  }
> > 
> > +/*
> > ---------------------------------------------------------------------------
> > -- + * Enumerator Udev
> > + */
> > +
> > +DeviceEnumeratorUdev::DeviceEnumeratorUdev()
> > +	: udev_(nullptr)
> > +{
> > +}
> > +
> > +DeviceEnumeratorUdev::~DeviceEnumeratorUdev()
> > +{
> > +	if (udev_)
> > +		udev_unref(udev_);
> > +}
> > +
> > +int DeviceEnumeratorUdev::init()
> > +{
> > +	if (udev_)
> > +		return -EBUSY;
> > +
> > +	udev_ = udev_new();
> > +	if (!udev_)
> > +		return -ENODEV;
> > +
> > +	return 0;
> > +}
> > +
> > +int DeviceEnumeratorUdev::enumerate()
> > +{
> > +	struct udev_enumerate *udev_enum = nullptr;
> > +	struct udev_list_entry *ents, *ent;
> > +	int ret;
> > +
> > +	udev_enum = udev_enumerate_new(udev_);
> > +	if (!udev_enum)
> > +		return -ENOMEM;
> > +
> > +	ret = udev_enumerate_add_match_subsystem(udev_enum, "media");
> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	ret = udev_enumerate_scan_devices(udev_enum);
> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	ents = udev_enumerate_get_list_entry(udev_enum);
> > +	if (!ents)
> > +		goto done;
> > +
> > +	udev_list_entry_foreach(ent, ents) {
> > +		struct udev_device *dev;
> > +		const char *devnode;
> > +		const char *syspath = udev_list_entry_get_name(ent);
> > +
> > +		dev = udev_device_new_from_syspath(udev_, syspath);
> > +		if (!dev) {
> > +			LOG(Error) << "Failed to get device for '" <<
> > +				   syspath << "', skipping";
> > +			continue;
> > +		}
> > +
> > +		devnode = udev_device_get_devnode(dev);
> > +		if (!devnode) {
> > +			udev_device_unref(dev);
> > +			ret = -ENODEV;
> > +			goto done;
> > +		}
> > +
> > +		addDevice(devnode);
> > +
> > +		udev_device_unref(dev);
> > +	}
> > +done:
> > +	udev_enumerate_unref(udev_enum);
> > +	return ret >= 0 ? 0 : ret;
> > +}
> > +
> > +std::string DeviceEnumeratorUdev::lookupDevnode(int major, int minor)
> > +{
> > +	struct udev_device *device;
> > +	const char *name;
> > +	dev_t devnum;
> > +	std::string devnode("");
> 
> Wouldn't std::string devnode(); be a little more efficient as it won't need to 
> parse the const char * ?

std::string devnode();

Would be considered a function declaration and is not valid, confusing I 
know. One option is

std::string devnode = std::string()

This would create a copy, but IIRC the compiler is allowed to optimize 
away the copy. In any case it's better so I'm going with this for now.

> 
> > +
> > +	devnum = makedev(major, minor);
> > +	device = udev_device_new_from_devnum(udev_, 'c', devnum);
> > +	if (!device)
> > +		return "";
> 
> Same here, return std::string() ?

Here it make sens, using this.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

> 
> > +
> > +	name = udev_device_get_devnode(device);
> > +	if (name)
> > +		devnode = name;
> > +
> > +	udev_device_unref(device);
> > +
> > +	return devnode;
> > +}
> > +
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/include/device_enumerator.h
> > b/src/libcamera/include/device_enumerator.h index
> > 1f8cef3311012308..5348e6cf583dbd15 100644
> > --- a/src/libcamera/include/device_enumerator.h
> > +++ b/src/libcamera/include/device_enumerator.h
> > @@ -75,6 +75,21 @@ private:
> >  	virtual std::string lookupDevnode(int major, int minor) = 0;
> >  };
> > 
> > +class DeviceEnumeratorUdev: public DeviceEnumerator
> > +{
> > +public:
> > +	DeviceEnumeratorUdev();
> > +	~DeviceEnumeratorUdev();
> > +
> > +	int init() final;
> > +	int enumerate() final;
> > +
> > +private:
> > +	struct udev *udev_;
> > +
> > +	std::string lookupDevnode(int major, int minor) final;
> > +};
> > +
> >  } /* namespace libcamera */
> > 
> >  #endif	/* __LIBCAMERA_DEVICE_ENUMERATOR_H__ */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
>

Patch
diff mbox series 

diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
index df9e89a1afeecda1..3cafd0d3703dac99 100644
--- a/src/libcamera/device_enumerator.cpp
+++ b/src/libcamera/device_enumerator.cpp
@@ -266,4 +266,102 @@  DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const
 	return info;
 }
 
+/* -----------------------------------------------------------------------------
+ * Enumerator Udev
+ */
+
+DeviceEnumeratorUdev::DeviceEnumeratorUdev()
+	: udev_(nullptr)
+{
+}
+
+DeviceEnumeratorUdev::~DeviceEnumeratorUdev()
+{
+	if (udev_)
+		udev_unref(udev_);
+}
+
+int DeviceEnumeratorUdev::init()
+{
+	if (udev_)
+		return -EBUSY;
+
+	udev_ = udev_new();
+	if (!udev_)
+		return -ENODEV;
+
+	return 0;
+}
+
+int DeviceEnumeratorUdev::enumerate()
+{
+	struct udev_enumerate *udev_enum = nullptr;
+	struct udev_list_entry *ents, *ent;
+	int ret;
+
+	udev_enum = udev_enumerate_new(udev_);
+	if (!udev_enum)
+		return -ENOMEM;
+
+	ret = udev_enumerate_add_match_subsystem(udev_enum, "media");
+	if (ret < 0)
+		goto done;
+
+	ret = udev_enumerate_scan_devices(udev_enum);
+	if (ret < 0)
+		goto done;
+
+	ents = udev_enumerate_get_list_entry(udev_enum);
+	if (!ents)
+		goto done;
+
+	udev_list_entry_foreach(ent, ents) {
+		struct udev_device *dev;
+		const char *devnode;
+		const char *syspath = udev_list_entry_get_name(ent);
+
+		dev = udev_device_new_from_syspath(udev_, syspath);
+		if (!dev) {
+			LOG(Error) << "Failed to get device for '" <<
+				   syspath << "', skipping";
+			continue;
+		}
+
+		devnode = udev_device_get_devnode(dev);
+		if (!devnode) {
+			udev_device_unref(dev);
+			ret = -ENODEV;
+			goto done;
+		}
+
+		addDevice(devnode);
+
+		udev_device_unref(dev);
+	}
+done:
+	udev_enumerate_unref(udev_enum);
+	return ret >= 0 ? 0 : ret;
+}
+
+std::string DeviceEnumeratorUdev::lookupDevnode(int major, int minor)
+{
+	struct udev_device *device;
+	const char *name;
+	dev_t devnum;
+	std::string devnode("");
+
+	devnum = makedev(major, minor);
+	device = udev_device_new_from_devnum(udev_, 'c', devnum);
+	if (!device)
+		return "";
+
+	name = udev_device_get_devnode(device);
+	if (name)
+		devnode = name;
+
+	udev_device_unref(device);
+
+	return devnode;
+}
+
 } /* namespace libcamera */
diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h
index 1f8cef3311012308..5348e6cf583dbd15 100644
--- a/src/libcamera/include/device_enumerator.h
+++ b/src/libcamera/include/device_enumerator.h
@@ -75,6 +75,21 @@  private:
 	virtual std::string lookupDevnode(int major, int minor) = 0;
 };
 
+class DeviceEnumeratorUdev: public DeviceEnumerator
+{
+public:
+	DeviceEnumeratorUdev();
+	~DeviceEnumeratorUdev();
+
+	int init() final;
+	int enumerate() final;
+
+private:
+	struct udev *udev_;
+
+	std::string lookupDevnode(int major, int minor) final;
+};
+
 } /* namespace libcamera */
 
 #endif	/* __LIBCAMERA_DEVICE_ENUMERATOR_H__ */