[libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create entities with major and minor numbers
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 2 12:29:02 CET 2019
Hi Jacopo,
On Wednesday, 2 January 2019 13:13:40 EET Jacopo Mondi wrote:
> On Wed, Jan 02, 2019 at 12:55:00PM +0200, Laurent Pinchart wrote:
> > 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.
Keeping the struct keyword has my preference too. You can submit a patch on
top of the master branch, or I can do it if you prefer.
> > > > bool populateEntities(const struct media_v2_topology &topology);
> > > > bool populatePads(const struct media_v2_topology &topology);
> > > > bool populateLinks(const struct media_v2_topology &topology);
[snip]
> > > > 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 :)
Follow-up patch it should be as I've pushed this patch already :-)
> 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.
I know. That's a bit of a shame.
> > > > +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) ?
It's a good point. Feel free to submit a follow-up patch if you think this
should be fixed.
> > > > +
> > > > + 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;
> > > > +}
[snip]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list