[libcamera-devel] [PATCH 05/12] libcamera: deviceenumerator: add DeviceEnumerator class
Jacopo Mondi
jacopo at jmondi.org
Mon Dec 24 12:22:32 CET 2018
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.
> +
> + 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181224/9e081edd/attachment.sig>
More information about the libcamera-devel
mailing list