| Message ID | 20181229032855.26249-6-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@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__ */
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@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@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 > > >
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; + } + + 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__ */
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@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(+)