[libcamera-devel] [PATCH 05/12] libcamera: deviceenumerator: add DeviceEnumerator class
Niklas S??derlund
niklas.soderlund at ragnatech.se
Fri Dec 28 04:23:25 CET 2018
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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list