[libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create entities with major and minor numbers
Jacopo Mondi
jacopo at jmondi.org
Wed Jan 2 11:38:23 CET 2019
Hi Laurent,
On Wed, Jan 02, 2019 at 02:49:02AM +0200, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo at jmondi.org>
>
> Extend the MediaEntity object with device node major and minor numbers,
> and retrieve them from the media graph using interfaces. They will be
> used by the DeviceEnumerator to retrieve the devnode path.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/libcamera/include/media_device.h | 2 +
> src/libcamera/include/media_object.h | 9 +++-
> src/libcamera/media_device.cpp | 65 +++++++++++++++++++++++++++-
> src/libcamera/media_object.cpp | 46 ++++++++++++++++++--
> 4 files changed, 116 insertions(+), 6 deletions(-)
>
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index 3fcdb4b4d5f8..8d491a87867c 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -54,6 +54,8 @@ private:
>
> std::vector<MediaEntity *> entities_;
>
> + struct media_v2_interface *findInterface(const struct media_v2_topology &topology,
> + unsigned int entityId);
To respect the 80-col, I dropped 'struct' from media_v2_ variables in
the 'populate*()' functions.
> bool populateEntities(const struct media_v2_topology &topology);
> bool populatePads(const struct media_v2_topology &topology);
> bool populateLinks(const struct media_v2_topology &topology);
> diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h
> index 65b55085a3b0..950a33286690 100644
> --- a/src/libcamera/include/media_object.h
> +++ b/src/libcamera/include/media_object.h
> @@ -80,21 +80,28 @@ class MediaEntity : public MediaObject
> {
> public:
> const std::string &name() const { return name_; }
> + unsigned int major() const { return major_; }
> + unsigned int minor() const { return minor_; }
>
> const std::vector<MediaPad *> &pads() const { return pads_; }
>
> const MediaPad *getPadByIndex(unsigned int index) const;
> const MediaPad *getPadById(unsigned int id) const;
>
> + int setDeviceNode(const std::string &devnode);
> +
I'm fine adding it here, but it is not currently used.
> private:
> friend class MediaDevice;
>
> - MediaEntity(const struct media_v2_entity *entity);
> + MediaEntity(const struct media_v2_entity *entity,
> + unsigned int major = 0, unsigned int minor = 0);
> MediaEntity(const MediaEntity &) = delete;
> ~MediaEntity();
>
> std::string name_;
> std::string devnode_;
> + unsigned int major_;
> + unsigned int minor_;
>
> std::vector<MediaPad *> pads_;
>
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 605e504be124..70b3fff3f492 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -208,6 +208,7 @@ int MediaDevice::populate()
> struct media_v2_entity *ents = nullptr;
> struct media_v2_link *links = nullptr;
> struct media_v2_pad *pads = nullptr;
> + struct media_v2_interface *interfaces = nullptr;
That's my OCD, but could you move this line 3 lines up? This gives
lines a nicer ordering
> __u64 version = -1;
> int ret;
>
> @@ -221,6 +222,7 @@ int MediaDevice::populate()
> topology.ptr_entities = reinterpret_cast<__u64>(ents);
> topology.ptr_links = reinterpret_cast<__u64>(links);
> topology.ptr_pads = reinterpret_cast<__u64>(pads);
> + topology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);
Same here
>
> ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> if (ret < 0) {
> @@ -236,10 +238,12 @@ int MediaDevice::populate()
> delete[] links;
> delete[] ents;
> delete[] pads;
> + delete[] interfaces;
Same here
>
> ents = new media_v2_entity[topology.num_entities];
> links = new media_v2_link[topology.num_links];
> pads = new media_v2_pad[topology.num_pads];
> + interfaces = new media_v2_interface[topology.num_interfaces];
And here.
>
> version = topology.topology_version;
> }
> @@ -253,6 +257,7 @@ int MediaDevice::populate()
> delete[] links;
> delete[] ents;
> delete[] pads;
> + delete[] interfaces;
And here.
The patch will look weird, but the resulting code will be nicer.
>
> if (!valid_) {
> clear();
> @@ -367,6 +372,45 @@ void MediaDevice::clear()
> * \brief Global list of media entities in the media graph
> */
>
> +/**
> + * \brief Find the interface associated with an entity
> + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> + * \param entityId The entity id
> + * \return A pointer to the interface if found, or nullptr otherwise
> + */
This is a private function, we can document it, I agree, but leave
doxygen commands out.
> +struct media_v2_interface *MediaDevice::findInterface(const struct media_v2_topology &topology,
> + unsigned int entityId)
> +{
> + struct media_v2_link *links = reinterpret_cast<struct media_v2_link *>
> + (topology.ptr_links);
media_v2_link *links = reinterpret_cast<media_v2_link *>
(topology.ptr_links);
To make it the same as in other populate*() functions
> + unsigned int ifaceId = -1;
unsigned int initialized with -1 ?
> +
> + for (unsigned int i = 0; i < topology.num_links; ++i) {
> + /* Search for the interface to entity link. */
> + if (links[i].sink_id != entityId)
> + continue;
> +
> + if ((links[i].flags & MEDIA_LNK_FL_LINK_TYPE) !=
> + MEDIA_LNK_FL_INTERFACE_LINK)
> + continue;
> +
> + ifaceId = links[i].source_id;
break?
> + }
> +
> + if (ifaceId == static_cast<unsigned int>(-1))
> + return nullptr;
> +
> + struct media_v2_interface *ifaces = reinterpret_cast<struct media_v2_interface *>
> + (topology.ptr_interfaces);
drop struct here as well?
> +
> + for (unsigned int i = 0; i < topology.num_interfaces; ++i) {
> + if (ifaces[i].id == ifaceId)
> + return &ifaces[i];
> + }
> +
> + return nullptr;
> +}
> +
> /*
> * For each entity in the media graph create a MediaEntity and store a
> * reference in the media device objects map and entities list.
> @@ -377,7 +421,26 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology)
> (topology.ptr_entities);
>
> for (unsigned int i = 0; i < topology.num_entities; ++i) {
> - MediaEntity *entity = new MediaEntity(&mediaEntities[i]);
> + /*
> + * Find the interface linked to this entity to get the device
> + * node major and minor numbers.
> + */
> + struct media_v2_interface *iface =
> + findInterface(topology, mediaEntities[i].id);
> + if (!iface) {
> + LOG(Error) << "Failed to find interface link for "
> + << "entity with id: " << mediaEntities[i].id;
> + return false;
> + }
As you pointed out already, it is fine to have entities without an
interface associated.
> +
> + MediaEntity *entity;
> + if (iface)
> + entity = new MediaEntity(&mediaEntities[i],
> + iface->devnode.major,
> + iface->devnode.minor);
> + else
> + entity = new MediaEntity(&mediaEntities[i]);
> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> index b64dcc3c8fb4..69e5cc74264d 100644
> --- a/src/libcamera/media_object.cpp
> +++ b/src/libcamera/media_object.cpp
> @@ -6,9 +6,8 @@
> */
>
> #include <errno.h>
> -#include <fcntl.h>
> #include <string.h>
> -#include <sys/stat.h>
> +#include <unistd.h>
>
> #include <string>
> #include <vector>
> @@ -212,6 +211,20 @@ void MediaPad::addLink(MediaLink *link)
> * \return The entity name
> */
>
> +/**
> + * \fn MediaEntity::major()
> + * \brief Retrieve the major number of the interface associated with the entity
> + * \return The interface major number, or 0 if the entity isn't associated with
> + * an interface
> + */
> +
> +/**
> + * \fn MediaEntity::minor()
> + * \brief Retrieve the minor number of the interface associated with the entity
> + * \return The interface minor number, or 0 if the entity isn't associated with
> + * an interface
> + */
> +
> /**
> * \fn MediaEntity::pads()
> * \brief Retrieve all pads of the entity
> @@ -238,6 +251,7 @@ const MediaPad *MediaEntity::getPadByIndex(unsigned int index) const
> * \param id The pad id
> * \return The pad identified by \a id, or nullptr if no such pad exist
> */
> +
Is this needed?
> const MediaPad *MediaEntity::getPadById(unsigned int id) const
> {
> for (MediaPad *p : pads_) {
> @@ -248,12 +262,36 @@ const MediaPad *MediaEntity::getPadById(unsigned int id) const
> return nullptr;
> }
>
> +/**
> + * \brief Set the path to the device node for the associated interface
> + * \param devnode The interface device node path associated with this entity
> + * \return 0 on success, or a negative error code if the device node can't be
> + * accessed
> + */
> +int MediaEntity::setDeviceNode(const std::string &devnode)
> +{
> + /* Make sure the device node can be accessed. */
> + int ret = ::access(devnode.c_str(), R_OK | W_OK);
> + if (ret < 0) {
> + ret = -errno;
> + LOG(Error) << "Device node " << devnode << " can't be accessed: "
> + << strerror(-ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> /**
> * \brief Construct a MediaEntity
> * \param entity The media entity kernel data
> + * \param major The major number of the entity associated interface
> + * \param minor The minor number of the entity associated interface
Should we say they're defaulted to 0 ?
Thanks
j
> */
> -MediaEntity::MediaEntity(const struct media_v2_entity *entity)
> - : MediaObject(entity->id), name_(entity->name)
> +MediaEntity::MediaEntity(const struct media_v2_entity *entity,
> + unsigned int major, unsigned int minor)
> + : MediaObject(entity->id), name_(entity->name),
> + major_(major), minor_(minor)
> {
> }
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> 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/20190102/fd1b2bb0/attachment.sig>
More information about the libcamera-devel
mailing list