[libcamera-devel] [PATCH v2 05/12] libcamera: device_enumerator: add DeviceEnumerator class

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Saturday, 29 December 2018 05:28:48 EET Niklas Söderlund wrote:
> Provide a DeviceEnumerator base class which enumerates all media devices
> in the system and information about them, resolving Media Controller
> data structures to paths and a method to search in all the enumerated
> information.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v1
> - Add strerror() logging if open() or ioctl() fails.
> - Use reinterpret_cast.
> - s/NULL/nullptr.
> - Break some lines to better fit 80 character lines.
> ---
>  src/libcamera/device_enumerator.cpp       | 155 ++++++++++++++++++++++
>  src/libcamera/include/device_enumerator.h |  22 +++
>  2 files changed, 177 insertions(+)
> 
> diff --git a/src/libcamera/device_enumerator.cpp
> b/src/libcamera/device_enumerator.cpp index
> 61b32bb921581f49..df9e89a1afeecda1 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -5,6 +5,12 @@
>   * device_enumerator.cpp - Enumeration and matching
>   */
> 
> +#include <fcntl.h>
> +#include <libudev.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
>  #include "device_enumerator.h"
>  #include "log.h"
> 
> @@ -111,4 +117,153 @@ bool DeviceMatch::match(const DeviceInfo *info) const
>  	return true;
>  }
> 
> +/*
> ---------------------------------------------------------------------------
> -- + * Enumerator Base
> + */
> +
> +DeviceEnumerator::~DeviceEnumerator()
> +{
> +	for (DeviceInfo *dev : devices_) {
> +		if (dev->busy())
> +			LOG(Error) << "Removing device info while still in use";
> +
> +		delete dev;
> +	}
> +}
> +
> +int DeviceEnumerator::addDevice(const std::string &devnode)
> +{
> +	int fd, ret;
> +
> +	struct media_device_info info = {};
> +	std::map<std::string, std::string> entities;
> +
> +	fd = open(devnode.c_str(), O_RDWR);
> +	if (fd < 0) {
> +		ret = -errno;
> +		LOG(Info) << "Unable to open " << devnode <<
> +			  " (" << strerror(-ret) << "), skipping";
> +		return ret;
> +	}
> +
> +	ret = readInfo(fd, info);
> +	if (ret)
> +		goto out;
> +
> +	ret = readTopology(fd, entities);
> +	if (ret)
> +		goto out;
> +
> +	devices_.push_back(new DeviceInfo(devnode, info, entities));
> +out:
> +	close(fd);
> +
> +	return ret;
> +}
> +
> +int DeviceEnumerator::readInfo(int fd, struct media_device_info &info)
> +{
> +	int ret;
> +
> +	ret = ioctl(fd, MEDIA_IOC_DEVICE_INFO, &info);
> +	if (ret < 0) {
> +		ret = -errno;
> +		LOG(Info) << "Unable to read device info " <<
> +			  " (" << strerror(-ret) << "), skipping";
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int DeviceEnumerator::readTopology(int fd, std::map<std::string,
> std::string> &entities) +{
> +	struct media_v2_topology topology;
> +	struct media_v2_entity *ents = nullptr;
> +	struct media_v2_interface *ifaces = nullptr;
> +	struct media_v2_link *links = nullptr;
> +	int ret;
> +
> +	while (true) {
> +		topology = {};
> +
> +		ret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology);
> +		if (ret < 0)
> +			return -errno;
> +
> +		__u64 version = topology.topology_version;
> +
> +		ents = new media_v2_entity[topology.num_entities]();
> +		ifaces = new media_v2_interface[topology.num_interfaces]();
> +		links = new media_v2_link[topology.num_links]();
> +		topology.ptr_entities = reinterpret_cast<__u64>(ents);
> +		topology.ptr_interfaces = reinterpret_cast<__u64>(ifaces);
> +		topology.ptr_links = reinterpret_cast<__u64>(links);
> +
> +		ret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology);
> +		if (ret < 0) {
> +			ret = -errno;
> +			goto done;
> +		}

I think this could be simplified with a single ioctl call inside the loop. As 
the code will be replaced with MediaDevice it's no big deal though, so

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

> +		if (version == topology.topology_version)
> +			break;
> +
> +		delete[] links;
> +		delete[] ifaces;
> +		delete[] ents;
> +	}
> +
> +	for (unsigned int link_id = 0; link_id < topology.num_links; link_id++) {
> +		unsigned int iface_id, ent_id;
> +		std::string devnode;
> +
> +		if ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) !=
> +		    MEDIA_LNK_FL_INTERFACE_LINK)
> +			continue;
> +
> +		for (iface_id = 0; iface_id < topology.num_interfaces; iface_id++)
> +			if (links[link_id].source_id == ifaces[iface_id].id)
> +				break;
> +
> +		for (ent_id = 0; ent_id < topology.num_entities; ent_id++)
> +			if (links[link_id].sink_id == ents[ent_id].id)
> +				break;
> +
> +		if (ent_id >= topology.num_entities ||
> +		    iface_id >= topology.num_interfaces)
> +			continue;
> +
> +		devnode = lookupDevnode(ifaces[iface_id].devnode.major,
> +					ifaces[iface_id].devnode.minor);
> +		if (devnode == "")
> +			break;
> +
> +		entities[ents[ent_id].name] = devnode;
> +	}
> +done:
> +	delete[] links;
> +	delete[] ifaces;
> +	delete[] ents;
> +
> +	return ret;
> +}
> +
> +DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const
> +{
> +	DeviceInfo *info = nullptr;
> +
> +	for (DeviceInfo *dev : devices_) {
> +		if (dev->busy())
> +			continue;
> +
> +		if (dm.match(dev)) {
> +			info = dev;
> +			break;
> +		}
> +	}
> +
> +	return info;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/device_enumerator.h
> b/src/libcamera/include/device_enumerator.h index
> ed1e986ff45f95f5..1f8cef3311012308 100644
> --- a/src/libcamera/include/device_enumerator.h
> +++ b/src/libcamera/include/device_enumerator.h
> @@ -53,6 +53,28 @@ private:
>  	std::vector<std::string> entities_;
>  };
> 
> +class DeviceEnumerator
> +{
> +public:
> +	virtual ~DeviceEnumerator();
> +
> +	virtual int init() = 0;
> +	virtual int enumerate() = 0;
> +
> +	DeviceInfo *search(DeviceMatch &dm) const;
> +
> +protected:
> +	int addDevice(const std::string &devnode);
> +
> +private:
> +	std::vector<DeviceInfo *> devices_;
> +
> +	int readInfo(int fd, struct media_device_info &info);
> +	int readTopology(int fd, std::map<std::string, std::string> &entities);
> +
> +	virtual std::string lookupDevnode(int major, int minor) = 0;
> +};
> +
>  } /* namespace libcamera */
> 
>  #endif	/* __LIBCAMERA_DEVICE_ENUMERATOR_H__ */


-- 
Regards,

Laurent Pinchart