[libcamera-devel] [PATCH 05/12] libcamera: deviceenumerator: add DeviceEnumerator class
Niklas S??derlund
niklas.soderlund at ragnatech.se
Sat Dec 29 02:27:06 CET 2018
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 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?
>
> 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
>
>
>
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list