[libcamera-devel] [PATCH 2/3] libcamera: MediaDevice: Create entities with major and minor numbers

[Laurent Pinchart]

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.

> >  	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 :-)

> >  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 ?

[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.

> > +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 :-)

> > +
> > +	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;
> > +
> > +	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.

> >   */
> > -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