[libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create entities with major and minor numbers
Jacopo Mondi
jacopo at jmondi.org
Wed Jan 2 12:13:40 CET 2019
Hi Laurent,
On Wed, Jan 02, 2019 at 12:55:00PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wednesday, 2 January 2019 12:38:23 EET Jacopo Mondi wrote:
> > 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.
>
> C++ allows us to do that, but I've kept it to emphasize that we're dealing
> with kernel structures. We're free to pick one option or another, but it
> should be applied globally.
I'm not a fan of omitting 'struct' neither, but in that case the code
was more readable and fit in 80 columns.
Let's define a rule globally. I would tend to keep struct
Please not the populate*() functions will have to be re-adjusted, I
could do that, send a patch, and then you can rebase on top, or you
could do it here.
>
> > > 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.
>
> I think it was added here in your original patch :-)
>
Told you: "I'm fine adding it here"
:)
> > > 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
>
> I wanted to keep the same order as in the media_v2_topology structure, but I
> now realize that interfaces come just after entities (and links after pads).
> How about reordering them in that order in a follow-up patch ?
How about reordering them in reverse-xmas-tree order in a follow-up
patch instead :)
Ultimate bikeshedding here... I'm fine with both actually
>
> [snip]
>
> > > @@ -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.
>
> Why so ? I think we should standardize on a single syntax for documentation.
>
Ok for consistency. Be aware Doxygen does not parse them though.
> > > +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
>
> Let's discuss struct media_v2_link vs. media_v2_link globally as mentioned
> above, and update the code accordingly.
>
> > > + unsigned int ifaceId = -1;
> >
> > unsigned int initialized with -1 ?
>
> The compiler doesn't warn :-)
>
So for the purpose of using this variable to check for a match, this
should read s/-1/UINT_MAX/ ?
> > > +
> > > + 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?
>
> Indeed. Fixed.
>
> > > + }
> > > +
> > > + if (ifaceId == static_cast<unsigned int>(-1))
> > > + return nullptr;
I wonder what is the value of making it unsigned, and having to go
through a static cast to use -1 as UINT_MAX.
I mean, I now if you declare it as signed int, it has then to be
casted to unsigned when comparing it with ids in kernel structure, but
what about checking here for
if (i == topology.num_links)
instead, and keep using unsigned for ifaceId (dropping the assignament and
comparison to -1) ?
> > > +
> > > + 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.
>
> Yes, I've dropped this check.
>
> > > +
> > > + 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
>
> [snip]
>
> > > @@ -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?
>
> Not at all, dropped.
>
> > > 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 ?
>
> Doxygen shows the default value in the function prototype already, I don't
> think we need to duplicate that information.
>
Ah fine, I didn't know.
Fine with me, with the few bit addressed (as you like the most, that's
minor stuff) please push this.
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
>
>
>
-------------- 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/d357279d/attachment.sig>
More information about the libcamera-devel
mailing list