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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-12-30 22:30:15 +0200, Laurent Pinchart wrote:
> 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

I agree this can be reworked for the MediaDevice.

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

Thanks.

> 
> > +		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
> 
> 
> 

-- 
Regards,
Niklas Söderlund