| Message ID | 20181222230041.29999-6-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, a few minor nits On Sun, Dec 23, 2018 at 12:00:34AM +0100, Niklas S??derlund wrote: > Provide a DeviceEnumerator base class which enumerates all media devices > in the system and information about them, resolving V4L2 data structures > to paths and a method to search in all the enumerated information. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/deviceenumerator.cpp | 144 +++++++++++++++++++++++ > src/libcamera/include/deviceenumerator.h | 22 ++++ > 2 files changed, 166 insertions(+) > > diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp > index 17b6874d229c791c..b2f59017e94d14aa 100644 > --- a/src/libcamera/deviceenumerator.cpp > +++ b/src/libcamera/deviceenumerator.cpp > @@ -5,6 +5,10 @@ > * deviceenumerator.cpp - Enumeration and matching > */ > > +#include <fcntl.h> > +#include <sys/ioctl.h> > +#include <unistd.h> > + > #include "deviceenumerator.h" > #include "log.h" > > @@ -129,4 +133,144 @@ bool DeviceMatch::matchEntities(const std::vector<std::string> &entities) 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) > + return fd; return -errno and printout error with strerror() ? We should standardize on error handline as much as we could. > + > + 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) > + return -errno; I found having strerror printing the error out a good practice. > + > + return 0; > +} > + > +int DeviceEnumerator::readTopology(int fd, std::map<std::string, std::string> &entities) > +{ > + struct media_v2_topology topology; > + struct media_v2_entity *ents = NULL; > + struct media_v2_interface *ifaces = NULL; > + struct media_v2_link *links = NULL; > + 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](); why the ending braces? > + ifaces = new media_v2_interface[topology.num_interfaces](); > + links = new media_v2_link[topology.num_links](); > + topology.ptr_entities = (__u64)ents; C style cast should be avoided even if that's trivial. I have: + struct media_v2_entity *_ptr_e = + new struct media_v2_entity[topology.num_entities]; + topology.ptr_entities = reinterpret_cast<__u64>(_ptr_e); I think the nicer one would be: ents = new media_v2_entity[topology.num_entities]; topology.ptr_entities = reinterpret_cast<__u64>(ents); > + topology.ptr_interfaces = (__u64)ifaces; > + topology.ptr_links = (__u64)links; > + > + ret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology); > + if (ret < 0) { > + ret = -errno; > + goto done; > + } Again, strerror is nice to have > + > + 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) This is debatable, but if not strictly necessary, let's aim for 80 columns. if ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) != MEDIA_LNK_FL_INTERFACE_LINK) Is equally readable > + 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) Same here if (ent_id >= topology.num_entities || iface_id >= topology.num_interfaces) > + continue; > + > + ret = lookupDevnode(devnode, > + ifaces[iface_id].devnode.major, > + ifaces[iface_id].devnode.minor); > + if (ret) > + break; > + > + entities[ents[ent_id].name] = devnode; > + } > +done: > + delete[] links; > + delete[] ifaces; > + delete[] ents; > + > + return ret; > +} > + > +DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const > +{ > + DeviceInfo *info = NULL; In C++ code is more commonly used 'nullptr' I know... :) > + > + 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/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h > index fb412b8840cb2ffe..cbe17774edb7fcc5 100644 > --- a/src/libcamera/include/deviceenumerator.h > +++ b/src/libcamera/include/deviceenumerator.h > @@ -56,6 +56,28 @@ private: > bool matchEntities(const std::vector<std::string> &entities) const; > }; > > +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 int lookupDevnode(std::string &devnode, int major, int minor) = 0; > +}; > + > } /* namespace libcamera */ > > #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */ > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
HI Jacopo, Thanks for your comments. On 2018-12-24 12:22:32 +0100, Jacopo Mondi wrote: > Hi Niklas, > a few minor nits > > On Sun, Dec 23, 2018 at 12:00:34AM +0100, Niklas S??derlund wrote: > > Provide a DeviceEnumerator base class which enumerates all media devices > > in the system and information about them, resolving V4L2 data structures > > to paths and a method to search in all the enumerated information. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/deviceenumerator.cpp | 144 +++++++++++++++++++++++ > > src/libcamera/include/deviceenumerator.h | 22 ++++ > > 2 files changed, 166 insertions(+) > > > > diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp > > index 17b6874d229c791c..b2f59017e94d14aa 100644 > > --- a/src/libcamera/deviceenumerator.cpp > > +++ b/src/libcamera/deviceenumerator.cpp > > @@ -5,6 +5,10 @@ > > * deviceenumerator.cpp - Enumeration and matching > > */ > > > > +#include <fcntl.h> > > +#include <sys/ioctl.h> > > +#include <unistd.h> > > + > > #include "deviceenumerator.h" > > #include "log.h" > > > > @@ -129,4 +133,144 @@ bool DeviceMatch::matchEntities(const std::vector<std::string> &entities) 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) > > + return fd; > > return -errno and printout error with strerror() ? > We should standardize on error handline as much as we could. I agree. What path should we take? > > > + > > + 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) > > + return -errno; > > I found having strerror printing the error out a good practice. No objection, see comment above. > > > + > > + return 0; > > +} > > + > > +int DeviceEnumerator::readTopology(int fd, std::map<std::string, std::string> &entities) > > +{ > > + struct media_v2_topology topology; > > + struct media_v2_entity *ents = NULL; > > + struct media_v2_interface *ifaces = NULL; > > + struct media_v2_link *links = NULL; > > + 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](); > > why the ending braces? In C++ they zero the object, think memset(). > > > + ifaces = new media_v2_interface[topology.num_interfaces](); > > + links = new media_v2_link[topology.num_links](); > > + topology.ptr_entities = (__u64)ents; > > C style cast should be avoided even if that's trivial. I have: > > + struct media_v2_entity *_ptr_e = > + new struct media_v2_entity[topology.num_entities]; > + topology.ptr_entities = reinterpret_cast<__u64>(_ptr_e); > > I think the nicer one would be: > > ents = new media_v2_entity[topology.num_entities]; > topology.ptr_entities = reinterpret_cast<__u64>(ents); > You might be right. > > + topology.ptr_interfaces = (__u64)ifaces; > > + topology.ptr_links = (__u64)links; > > + > > + ret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology); > > + if (ret < 0) { > > + ret = -errno; > > + goto done; > > + } > > Again, strerror is nice to have > > + > > + 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) > > This is debatable, but if not strictly necessary, let's aim for 80 > columns. > > if ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) != > MEDIA_LNK_FL_INTERFACE_LINK) > > Is equally readable > > > + 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) > > Same here > > if (ent_id >= topology.num_entities || > iface_id >= topology.num_interfaces) > No strong opinion, with a 120 limit I think this and the thing above is more readable. I will fix as the group judge. > > + continue; > > + > > + ret = lookupDevnode(devnode, > > + ifaces[iface_id].devnode.major, > > + ifaces[iface_id].devnode.minor); > > + if (ret) > > + break; > > + > > + entities[ents[ent_id].name] = devnode; > > + } > > +done: > > + delete[] links; > > + delete[] ifaces; > > + delete[] ents; > > + > > + return ret; > > +} > > + > > +DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const > > +{ > > + DeviceInfo *info = NULL; > > In C++ code is more commonly used 'nullptr' > I know... :) No preference, I let the group mob rule. > > > + > > + 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/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h > > index fb412b8840cb2ffe..cbe17774edb7fcc5 100644 > > --- a/src/libcamera/include/deviceenumerator.h > > +++ b/src/libcamera/include/deviceenumerator.h > > @@ -56,6 +56,28 @@ private: > > bool matchEntities(const std::vector<std::string> &entities) const; > > }; > > > > +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 int lookupDevnode(std::string &devnode, int major, int minor) = 0; > > +}; > > + > > } /* namespace libcamera */ > > > > #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */ > > -- > > 2.20.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Friday, 28 December 2018 05:23:25 EET Niklas S??derlund wrote: > On 2018-12-24 12:22:32 +0100, Jacopo Mondi wrote: > > On Sun, Dec 23, 2018 at 12:00:34AM +0100, Niklas S??derlund wrote: > > > Provide a DeviceEnumerator base class which enumerates all media devices > > > in the system and information about them, resolving V4L2 data structures > > > to paths and a method to search in all the enumerated information. It's not V4L2 but Media Controller. > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > > > > src/libcamera/deviceenumerator.cpp | 144 +++++++++++++++++++++++ > > > src/libcamera/include/deviceenumerator.h | 22 ++++ > > > 2 files changed, 166 insertions(+) > > > > > > diff --git a/src/libcamera/deviceenumerator.cpp > > > b/src/libcamera/deviceenumerator.cpp index > > > 17b6874d229c791c..b2f59017e94d14aa 100644 > > > --- a/src/libcamera/deviceenumerator.cpp > > > +++ b/src/libcamera/deviceenumerator.cpp > > > @@ -5,6 +5,10 @@ > > > * deviceenumerator.cpp - Enumeration and matching > > > */ > > > > > > +#include <fcntl.h> > > > +#include <sys/ioctl.h> > > > +#include <unistd.h> > > > + > > > #include "deviceenumerator.h" > > > #include "log.h" > > > > > > @@ -129,4 +133,144 @@ bool DeviceMatch::matchEntities(const > > > std::vector<std::string> &entities) 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) > > > + return fd; > > > > return -errno and printout error with strerror() ? > > We should standardize on error handline as much as we could. > > I agree. What path should we take? I propose returning a negative error code on error, which would be -errno here. Logging a message would also be useful, but please note that errno could be modified by the message logging, so you need to save the variable first. 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) > > > + return -errno; > > > > I found having strerror printing the error out a good practice. > > No objection, see comment above. > > > > + > > > + return 0; > > > +} > > > + > > > +int DeviceEnumerator::readTopology(int fd, std::map<std::string, > > > std::string> &entities) > > > +{ > > > + struct media_v2_topology topology; > > > + struct media_v2_entity *ents = NULL; > > > + struct media_v2_interface *ifaces = NULL; > > > + struct media_v2_link *links = NULL; > > > + 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](); > > > > why the ending braces? > > In C++ they zero the object, think memset(). > > > > + ifaces = new media_v2_interface[topology.num_interfaces](); > > > + links = new media_v2_link[topology.num_links](); > > > + topology.ptr_entities = (__u64)ents; > > > > C style cast should be avoided even if that's trivial. I have: > > > > + struct media_v2_entity *_ptr_e = > > + new struct media_v2_entity[topology.num_entities]; > > + topology.ptr_entities = reinterpret_cast<__u64>(_ptr_e); > > > > I think the nicer one would be: > > ents = new media_v2_entity[topology.num_entities]; > > topology.ptr_entities = reinterpret_cast<__u64>(ents); > > You might be right. > > > > + topology.ptr_interfaces = (__u64)ifaces; > > > + topology.ptr_links = (__u64)links; > > > + > > > + ret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology); > > > + if (ret < 0) { > > > + ret = -errno; > > > + goto done; > > > + } > > > > Again, strerror is nice to have > > > > > + > > > + 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) > > > > This is debatable, but if not strictly necessary, let's aim for 80 > > columns. > > > > if ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) != > > MEDIA_LNK_FL_INTERFACE_LINK) > > > > Is equally readable > > > > > + 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)> > > > > Same here > > > > if (ent_id >= topology.num_entities || > > iface_id >= topology.num_interfaces) > > No strong opinion, with a 120 limit I think this and the thing above is > more readable. I will fix as the group judge. I find Jacopo's suggestion here more readable, but perhaps because I'm used to wrap lines at the 80 columns limit. I don't want to force a particular choice here. > > > + continue; > > > + > > > + ret = lookupDevnode(devnode, > > > + ifaces[iface_id].devnode.major, > > > + ifaces[iface_id].devnode.minor); > > > + if (ret) > > > + break; > > > + > > > + entities[ents[ent_id].name] = devnode; > > > + } > > > +done: > > > + delete[] links; > > > + delete[] ifaces; > > > + delete[] ents; > > > + > > > + return ret; > > > +} > > > + > > > +DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const > > > +{ > > > + DeviceInfo *info = NULL; > > > > In C++ code is more commonly used 'nullptr' > > I know... :) > > No preference, I let the group mob rule. > > > > + > > > + 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/deviceenumerator.h > > > b/src/libcamera/include/deviceenumerator.h index > > > fb412b8840cb2ffe..cbe17774edb7fcc5 100644 > > > --- a/src/libcamera/include/deviceenumerator.h > > > +++ b/src/libcamera/include/deviceenumerator.h > > > > > > @@ -56,6 +56,28 @@ private: > > > bool matchEntities(const std::vector<std::string> &entities) const; > > > }; > > > > > > +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); Do you foresee the above two methods being moved to MediaDevice ? > > > + virtual int lookupDevnode(std::string &devnode, int major, int minor) > > > = 0; > > > +}; > > > + > > > > > > } /* namespace libcamera */ > > > > > > #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */
Hi Laurent, Thanks for your feedback. On 2018-12-29 00:53:22 +0200, Laurent Pinchart wrote: > Hello, > > On Friday, 28 December 2018 05:23:25 EET Niklas S??derlund wrote: > > On 2018-12-24 12:22:32 +0100, Jacopo Mondi wrote: > > > On Sun, Dec 23, 2018 at 12:00:34AM +0100, Niklas S??derlund wrote: > > > > Provide a DeviceEnumerator base class which enumerates all media devices > > > > in the system and information about them, resolving V4L2 data structures > > > > to paths and a method to search in all the enumerated information. > > It's not V4L2 but Media Controller. Thanks. > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > --- > > > > > > > > src/libcamera/deviceenumerator.cpp | 144 +++++++++++++++++++++++ > > > > src/libcamera/include/deviceenumerator.h | 22 ++++ > > > > 2 files changed, 166 insertions(+) > > > > > > > > diff --git a/src/libcamera/deviceenumerator.cpp > > > > b/src/libcamera/deviceenumerator.cpp index > > > > 17b6874d229c791c..b2f59017e94d14aa 100644 > > > > --- a/src/libcamera/deviceenumerator.cpp > > > > +++ b/src/libcamera/deviceenumerator.cpp > > > > @@ -5,6 +5,10 @@ > > > > * deviceenumerator.cpp - Enumeration and matching > > > > */ > > > > > > > > +#include <fcntl.h> > > > > +#include <sys/ioctl.h> > > > > +#include <unistd.h> > > > > + > > > > #include "deviceenumerator.h" > > > > #include "log.h" > > > > > > > > @@ -129,4 +133,144 @@ bool DeviceMatch::matchEntities(const > > > > std::vector<std::string> &entities) 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) > > > > + return fd; > > > > > > return -errno and printout error with strerror() ? > > > We should standardize on error handline as much as we could. > > > > I agree. What path should we take? > > I propose returning a negative error code on error, which would be -errno > here. Logging a message would also be useful, but please note that errno could > be modified by the message logging, so you need to save the variable first. > > fd = open(devnode.c_str(), O_RDWR); > if (fd < 0) { > ret = -errno; > Log(Info) << "Unable to open " << devnode <<" (" << strerror(-ret) << > "), skipping"; > return ret; > } Mob rule, I have taken this approach where Jacopo pointed out it could be beneficial. > > > > > + > > > > + 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) > > > > + return -errno; > > > > > > I found having strerror printing the error out a good practice. > > > > No objection, see comment above. > > > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +int DeviceEnumerator::readTopology(int fd, std::map<std::string, > > > > std::string> &entities) > > > > +{ > > > > + struct media_v2_topology topology; > > > > + struct media_v2_entity *ents = NULL; > > > > + struct media_v2_interface *ifaces = NULL; > > > > + struct media_v2_link *links = NULL; > > > > + 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](); > > > > > > why the ending braces? > > > > In C++ they zero the object, think memset(). > > > > > > + ifaces = new media_v2_interface[topology.num_interfaces](); > > > > + links = new media_v2_link[topology.num_links](); > > > > + topology.ptr_entities = (__u64)ents; > > > > > > C style cast should be avoided even if that's trivial. I have: > > > > > > + struct media_v2_entity *_ptr_e = > > > + new struct media_v2_entity[topology.num_entities]; > > > + topology.ptr_entities = reinterpret_cast<__u64>(_ptr_e); > > > > > > I think the nicer one would be: > > > ents = new media_v2_entity[topology.num_entities]; > > > topology.ptr_entities = reinterpret_cast<__u64>(ents); > > > > You might be right. > > > > > > + topology.ptr_interfaces = (__u64)ifaces; > > > > + topology.ptr_links = (__u64)links; > > > > + > > > > + ret = ioctl(fd, MEDIA_IOC_G_TOPOLOGY, &topology); > > > > + if (ret < 0) { > > > > + ret = -errno; > > > > + goto done; > > > > + } > > > > > > Again, strerror is nice to have > > > > > > > + > > > > + 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) > > > > > > This is debatable, but if not strictly necessary, let's aim for 80 > > > columns. > > > > > > if ((links[link_id].flags & MEDIA_LNK_FL_LINK_TYPE) != > > > MEDIA_LNK_FL_INTERFACE_LINK) > > > > > > Is equally readable > > > > > > > + 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)> > > > > > > Same here > > > > > > if (ent_id >= topology.num_entities || > > > iface_id >= topology.num_interfaces) > > > > No strong opinion, with a 120 limit I think this and the thing above is > > more readable. I will fix as the group judge. > > I find Jacopo's suggestion here more readable, but perhaps because I'm used to > wrap lines at the 80 columns limit. I don't want to force a particular choice > here. 80 it is then :-) > > > > > + continue; > > > > + > > > > + ret = lookupDevnode(devnode, > > > > + ifaces[iface_id].devnode.major, > > > > + ifaces[iface_id].devnode.minor); > > > > + if (ret) > > > > + break; > > > > + > > > > + entities[ents[ent_id].name] = devnode; > > > > + } > > > > +done: > > > > + delete[] links; > > > > + delete[] ifaces; > > > > + delete[] ents; > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const > > > > +{ > > > > + DeviceInfo *info = NULL; > > > > > > In C++ code is more commonly used 'nullptr' > > > I know... :) > > > > No preference, I let the group mob rule. > > > > > > + > > > > + 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/deviceenumerator.h > > > > b/src/libcamera/include/deviceenumerator.h index > > > > fb412b8840cb2ffe..cbe17774edb7fcc5 100644 > > > > --- a/src/libcamera/include/deviceenumerator.h > > > > +++ b/src/libcamera/include/deviceenumerator.h > > > > > > > > @@ -56,6 +56,28 @@ private: > > > > bool matchEntities(const std::vector<std::string> &entities) const; > > > > }; > > > > > > > > +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); > > Do you foresee the above two methods being moved to MediaDevice ? I think so, or adopt Jacopo's implementation. As readTopology() equivalent would be needed anyhow for a MediaDevice to get links for example. > > > > > + virtual int lookupDevnode(std::string &devnode, int major, int > minor) > > > > = 0; > > > > +}; > > > > + > > > > > > > > } /* namespace libcamera */ > > > > > > > > #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */ > > -- > Regards, > > Laurent Pinchart > > >
diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp index 17b6874d229c791c..b2f59017e94d14aa 100644 --- a/src/libcamera/deviceenumerator.cpp +++ b/src/libcamera/deviceenumerator.cpp @@ -5,6 +5,10 @@ * deviceenumerator.cpp - Enumeration and matching */ +#include <fcntl.h> +#include <sys/ioctl.h> +#include <unistd.h> + #include "deviceenumerator.h" #include "log.h" @@ -129,4 +133,144 @@ bool DeviceMatch::matchEntities(const std::vector<std::string> &entities) 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) + return fd; + + 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) + return -errno; + + return 0; +} + +int DeviceEnumerator::readTopology(int fd, std::map<std::string, std::string> &entities) +{ + struct media_v2_topology topology; + struct media_v2_entity *ents = NULL; + struct media_v2_interface *ifaces = NULL; + struct media_v2_link *links = NULL; + 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 = (__u64)ents; + topology.ptr_interfaces = (__u64)ifaces; + topology.ptr_links = (__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; + + ret = lookupDevnode(devnode, + ifaces[iface_id].devnode.major, + ifaces[iface_id].devnode.minor); + if (ret) + break; + + entities[ents[ent_id].name] = devnode; + } +done: + delete[] links; + delete[] ifaces; + delete[] ents; + + return ret; +} + +DeviceInfo *DeviceEnumerator::search(DeviceMatch &dm) const +{ + DeviceInfo *info = NULL; + + 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/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h index fb412b8840cb2ffe..cbe17774edb7fcc5 100644 --- a/src/libcamera/include/deviceenumerator.h +++ b/src/libcamera/include/deviceenumerator.h @@ -56,6 +56,28 @@ private: bool matchEntities(const std::vector<std::string> &entities) const; }; +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 int lookupDevnode(std::string &devnode, int major, int minor) = 0; +}; + } /* namespace libcamera */ #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */
Provide a DeviceEnumerator base class which enumerates all media devices in the system and information about them, resolving V4L2 data structures to paths and a method to search in all the enumerated information. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/deviceenumerator.cpp | 144 +++++++++++++++++++++++ src/libcamera/include/deviceenumerator.h | 22 ++++ 2 files changed, 166 insertions(+)