[libcamera-devel] [PATCH v2] libcamera: device_enumerator: add DeviceEnumeratorSysfs class

[Laurent Pinchart]

On Thu, May 02, 2019 at 07:42:48PM -0400, Paul Elder wrote:
> A udev-based device enumerator is not sufficient, since libudev is an
> optional dependency, or udev might fail. In these cases, we should fall
> back to using sysfs to enumerate devices.
> 
> Add a DeviceEnumeratorSysfs class which is a specialization of
> DeviceEnumerator that uses sysfs to enumerate media devices on the
> system.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v2:
> - style changes
> - change sysfs directory checking list to remove block and add subsystem
> - add warning output to alert if media device (/dev/media*) should exist
>   based on sysfs but doesn't
> 
>  Documentation/Doxyfile.in                     |   4 +-
>  src/libcamera/device_enumerator.cpp           |   6 +-
>  src/libcamera/device_enumerator_sysfs.cpp     | 110 ++++++++++++++++++
>  .../include/device_enumerator_sysfs.h         |  31 +++++
>  src/libcamera/meson.build                     |   2 +
>  5 files changed, 150 insertions(+), 3 deletions(-)
>  create mode 100644 src/libcamera/device_enumerator_sysfs.cpp
>  create mode 100644 src/libcamera/include/device_enumerator_sysfs.h
> 
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 950ad4f..381f777 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -833,7 +833,9 @@ RECURSIVE              = YES
>  # Note that relative paths are relative to the directory from which doxygen is
>  # run.
>  
> -EXCLUDE                = ../src/libcamera/device_enumerator_udev.cpp \
> +EXCLUDE                = ../src/libcamera/device_enumerator_sysfs.cpp \
> +			 ../src/libcamera/device_enumerator_udev.cpp \
> +			 ../src/libcamera/include/device_enumerator_sysfs.h \
>  			 ../src/libcamera/include/device_enumerator_udev.h \
>  			 ../src/libcamera/pipeline/
>  
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index f6878b3..259eec4 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include "device_enumerator.h"
> +#include "device_enumerator_sysfs.h"
>  #include "device_enumerator_udev.h"
>  
>  #include <string.h>
> @@ -153,8 +154,9 @@ std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create()
>  	 * Either udev is not available or udev initialization failed. Fall back
>  	 * on the sysfs enumerator.
>  	 */
> -
> -	/** \todo Add a sysfs-based enumerator. */
> +	enumerator = utils::make_unique<DeviceEnumeratorSysfs>();
> +	if (!enumerator->init())
> +		return enumerator;
>  
>  	return nullptr;
>  }
> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp
> new file mode 100644
> index 0000000..943cab2
> --- /dev/null
> +++ b/src/libcamera/device_enumerator_sysfs.cpp
> @@ -0,0 +1,110 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * device_enumerator_sysfs.cpp - sysfs-based device enumerator
> + */
> +
> +#include "device_enumerator_sysfs.h"
> +
> +#include <dirent.h>
> +#include <fcntl.h>
> +#include <fstream>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include "log.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(DeviceEnumerator)
> +
> +DeviceEnumeratorSysfs::DeviceEnumeratorSysfs()
> +{
> +}
> +
> +DeviceEnumeratorSysfs::~DeviceEnumeratorSysfs()
> +{
> +}
> +
> +int DeviceEnumeratorSysfs::init()
> +{
> +	return 0;
> +}
> +
> +int DeviceEnumeratorSysfs::enumerate()
> +{
> +	struct dirent *ent;
> +	DIR *dir;
> +
> +	static const char *sysfs_dirs[] = {

To make both the array and its contents const, this should be

	static const char * const sysfs_dirs[] = {

> +		"/sys/subsystem/media/devices",
> +		"/sys/bus/media/devices",
> +		"/sys/class/media/devices",
> +	};
> +
> +	for (const char *dirname : sysfs_dirs) {
> +		dir = opendir(dirname);
> +		if (dir)
> +			break;
> +	}

Should you have a

	if (!dir) {
		LOG(...) << ...;
		return -ENODEV;
	}

> +
> +	while ((ent = readdir(dir)) != nullptr) {
> +		unsigned int idx;
> +		struct stat devstat;
> +		char *end;
> +		std::string devnode;
> +
> +		if (strncmp(ent->d_name, "media", 5))
> +			continue;
> +
> +		idx = strtoul(ent->d_name + 5, &end, 10);
> +		if (*end != '\0')
> +			continue;
> +
> +		devnode = "/dev/media" + std::to_string(idx);
> +
> +		/* Verify that the device node exists. */
> +		if (stat(devnode.c_str(), &devstat) < 0) {
> +			LOG(DeviceEnumerator, Warning)
> +				<< "Device node /dev/media" << idx
> +				<< " should exist but doesn't";
> +			continue;
> +		}

I know you'll need some time to get used to it, but in C++ variables
don't have to be declared at the beginning of the scope, so you can
write

		if (strncmp(ent->d_name, "media", 5))
			continue;

		char *end;
		unsigned int idx = strtoul(ent->d_name + 5, &end, 10);
		if (*end != '\0')
			continue;

		std::string devnode = "/dev/media" + std::to_string(idx);

		/* Verify that the device node exists. */
		struct stat devstat;
		if (stat(devnode.c_str(), &devstat) < 0) {
			LOG(DeviceEnumerator, Warning)
				<< "Device node /dev/media" << idx
				<< " should exist but doesn't";
			continue;
		}

> +
> +		addDevice(devnode);
> +	}
> +
> +	closedir(dir);
> +
> +	return 0;
> +}
> +
> +std::string DeviceEnumeratorSysfs::lookupDeviceNode(int major, int minor)
> +{
> +	std::string deviceNode;
> +	std::string line;
> +	std::ifstream ueventFile;
> +
> +	ueventFile.open("/sys/dev/char/" + std::to_string(major) + ":" +
> +			std::to_string(minor) + "/uevent");
> +	if (!ueventFile)
> +		return std::string();
> +
> +	while (ueventFile >> line) {
> +		if (line.find("DEVNAME") == 0) {

Should this be DEVNAME= instead of DEVNAME ?

> +			deviceNode = "/dev/" + line.substr(strlen("DEVNAME="));
> +			break;
> +		}
> +	}
> +
> +	ueventFile.close();
> +
> +	return deviceNode;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h
> new file mode 100644
> index 0000000..534b28c
> --- /dev/null
> +++ b/src/libcamera/include/device_enumerator_sysfs.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * device_enumerator_sysfs.h - sysfs-based device enumerator
> + */
> +#ifndef __LIBCAMERA_DEVICE_ENUMERATOR_SYSFS_H__
> +#define __LIBCAMERA_DEVICE_ENUMERATOR_SYSFS_H__
> +
> +#include <string>
> +
> +#include "device_enumerator.h"
> +
> +namespace libcamera {
> +
> +class DeviceEnumeratorSysfs final : public DeviceEnumerator
> +{
> +public:
> +	DeviceEnumeratorSysfs();
> +	~DeviceEnumeratorSysfs();
> +
> +	int init();
> +	int enumerate();
> +
> +private:
> +	std::string lookupDeviceNode(int major, int minor);
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_DEVICE_ENUMERATOR_SYSFS_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 2b67823..8796f49 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -4,6 +4,7 @@ libcamera_sources = files([
>      'camera_manager.cpp',
>      'camera_sensor.cpp',
>      'device_enumerator.cpp',
> +    'device_enumerator_sysfs.cpp',
>      'event_dispatcher.cpp',
>      'event_dispatcher_poll.cpp',
>      'event_notifier.cpp',
> @@ -26,6 +27,7 @@ libcamera_sources = files([
>  libcamera_headers = files([
>      'include/camera_sensor.h',
>      'include/device_enumerator.h',
> +    'include/device_enumerator_sysfs.h',
>      'include/device_enumerator_udev.h',
>      'include/event_dispatcher_poll.h',
>      'include/formats.h',

-- 
Regards,

Laurent Pinchart