[libcamera-devel,RESEND,v2,2/4] libcamera: Add MediaDevice class

92 diff mbox series

The MediaDevice object implements handling and configuration of the media
graph associated with a V4L2 media device.

The class allows enumeration of all pads, links and entities registered in
the media graph, and provides methods to setup and reset media links.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/media_device.h |  71 ++++
 src/libcamera/media_device.cpp       | 596 +++++++++++++++++++++++++++
 src/libcamera/meson.build            |   1 +
 3 files changed, 668 insertions(+)
 create mode 100644 src/libcamera/include/media_device.h
 create mode 100644 src/libcamera/media_device.cpp

--
2.20.1

Hi Jacopo,

Thank you for the patch.

On Friday, 28 December 2018 09:57:41 EET Jacopo Mondi wrote:
> The MediaDevice object implements handling and configuration of the media
> graph associated with a V4L2 media device.

Let's not talk about V4L2 here, it's just MC, not V4L2.

> The class allows enumeration of all pads, links and entities registered in
> the media graph, and provides methods to setup and reset media links.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/media_device.h |  71 ++++
>  src/libcamera/media_device.cpp       | 596 +++++++++++++++++++++++++++
>  src/libcamera/meson.build            |   1 +
>  3 files changed, 668 insertions(+)
>  create mode 100644 src/libcamera/include/media_device.h
>  create mode 100644 src/libcamera/media_device.cpp
> 
> diff --git a/src/libcamera/include/media_device.h
> b/src/libcamera/include/media_device.h new file mode 100644
> index 0000000..642eea9
> --- /dev/null
> +++ b/src/libcamera/include/media_device.h
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * media_device.h - Media device handler
> + */
> +#ifndef __LIBCAMERA_MEDIA_DEVICE_H__
> +#define __LIBCAMERA_MEDIA_DEVICE_H__
> +
> +#include <map>
> +#include <string>
> +#include <sstream>

sort

> +#include <vector>
> +
> +#include <linux/media.h>
> +
> +#include "log.h"

Is this needed ?

> +#include "media_object.h"
> +
> +namespace libcamera {
> +
> +class MediaDevice
> +{
> +public:
> +	MediaDevice() : fd_(-1) { };
> +	~MediaDevice();
> +
> +	const std::string name() const { return name_; }
> +	const std::string path() const { return path_; }
> +
> +	int open(const std::string &path);
> +	int close();
> +	int enumerate(std::map<std::string, std::string> &entitiesMap);
> +	void dumpGraph(std::ostream &os);
> +
> +	int resetLinks();
> +	int link(const std::string &source, unsigned int sourceIdx,
> +		 const std::string &sink, unsigned int sinkIdx,
> +		 unsigned int flags);
> +
> +private:
> +	/** The media device file descriptor */

Wrong file ?

> +	int fd_;
> +	/** The media device name as returned by MEDIA_IOC_DEVICE_INFO */

Please take my comments on 1/4 into account regarding wording of documentation 
here.

This field stores the driver name, I would thus name it driver_ (or possibly 
driverName_).

> +	std::string name_;
> +	/** The media device path */
> +	std::string path_;
> +
> +	std::map<unsigned int, MediaObject *> mediaObjects_;

s/mediaObjects_/objects_/ ?

> +	MediaObject *getObject(unsigned int id);
> +	void addObject(MediaObject *obj);
> +	void deleteObjects();
> +
> +	std::vector<MediaEntity *> entities_;
> +	MediaEntity *getEntityByName(const std::string &name);
> +
> +	int enumerateEntities(std::map<std::string, std::string> &entitiesMap,
> +			      struct media_v2_topology &topology);
> +	int enumeratePads(struct media_v2_topology &topology);
> +	int enumerateLinks(struct media_v2_topology &topology);
> +
> +	int setupLink(const MediaPad *source, const MediaPad *sink,
> +		      MediaLink *link, unsigned int flags);
> +
> +	void dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os);
> +	void dumpRemote(MediaLink *l, std::ostream &os);
> +	void dumpLink(MediaLink *l, std::ostream &os);
> +};
> +
> +} /* namespace libcamera */
> +#endif /* __LIBCAMERA_MEDIA_DEVICE_H__ */
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> new file mode 100644
> index 0000000..b98d62f
> --- /dev/null
> +++ b/src/libcamera/media_device.cpp
> @@ -0,0 +1,596 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * media_device.cpp - Media device handler
> + */
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
> +#include <string>
> +#include <vector>
> +
> +#include <linux/media.h>
> +
> +#include "log.h"
> +#include "media_device.h"
> +
> +/**
> + * \file media_device.h
> + */
> +namespace libcamera {
> +
> +/**
> + * \class MediaDevice
> + * \brief Media device handler
> + *
> + * MediaDevice handles the media graph associated with a V4L2 media device.
> + */
> +
> +/**
> + * \fn MediaDevice::~MediaDevice()
> + * \brief Close the media device file descriptor and release entities
> + */
> +MediaDevice::~MediaDevice()
> +{
> +	if (fd_ > -1)

I would have written != -1.

> +		::close(fd_);
> +	deleteObjects();
> +}
> +
> +/**
> + * \fn MediaDevice::name()
> + * \brief Return the media device name
> + */
> +
> +/**
> + * \fn MediaDevice::path()
> + * \brief Return the media device path node associated with this
> MediaDevice

I'd name this devnode() instead of path().

> + */
> +
> +/**
> + * \fn MediaDevice::deleteObjects()
> + * \brief Delete all registered entities in the MediaDevice object
> + */
> +void MediaDevice::deleteObjects()
> +{
> +	for (auto const &e : mediaObjects_)
> +		delete e.second;
> +
> +	mediaObjects_.clear();
> +	entities_.clear();
> +}
> +
> +/**
> + * \fn int MediaDevice::open(std::string)

Do you need to specify the arguments when there's a single function by that 
name ? If so, shouldn't it be (const std::string &) ?

Same comment for other functions in this patch series.

> + * \brief Open a media device and initialize its components.
> + * \param path The media device path
> + */
> +int MediaDevice::open(const std::string &path)

s/path/devnode/ ?

> +{

What if the device is already open ? It would be useful to add some 
documentation to explain the "life cycle" of the object, as in how it is 
supposed to be used (and what happens when it's incorrectly used). This can be 
done in the open() documentation (and referenced from close() and possibly 
other functions), or in the \class documentation.

> +	fd_ = ::open(path.c_str(), O_RDWR);
> +	if (fd_ < 0) {
> +		LOG(Error) << "Failed to open media device at " << path
> +			   << ": " << strerror(errno);
> +		return -errno;

The LOG() call may overwrite errno. You will need a local variable. How about

	int ret;

	ret = ::open(path.c_str(), O_RDWR);
	if (ret < 0) {
		ret = -errno;
		LOG(Error) << "Failed to open media device at " << path
			   << ": " << strerror(-ret);
		return ret;
	}

	fd_ = ret;
	path_ = path;

> +	}
> +	path_ = path;
> +
> +	struct media_device_info info = { };
> +	int ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
> +	if (ret) {
> +		LOG(Error) << "Failed to get media device info "
> +			   << ": " << strerror(errno);
> +		return -errno;
> +	}
> +
> +	name_ = info.driver;
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn MediaDevice::close()
> + * \brief Close the file descriptor associated with the media device
> + */
> +int MediaDevice::close()
> +{
> +	if (fd_ > -1)

I'd write this != -1 too.

> +		return ::close(fd_);
> +
> +	return 0;

Do you use the return value ?

> +}
> +
> +void MediaDevice::addObject(MediaObject *obj)
> +{
> +
> +	if (mediaObjects_.find(obj->id()) != mediaObjects_.end()) {
> +		LOG(Error) << "Element with id " << obj->id()
> +			   << " already enumerated.";
> +		return;

Shouldn't you propagate the error to the caller ? This seems pretty fatal to 
me.

> +	}
> +
> +	mediaObjects_[obj->id()] = obj;
> +}
> +
> +MediaObject *MediaDevice::getObject(unsigned int id)
> +{
> +	auto it = mediaObjects_.find(id);
> +	return (it == mediaObjects_.end()) ?
> +	       nullptr : it->second;
> +}
> +
> +/**
> + * \fn MediaDevice::getEntityByName(std::string)
> + * \brief Return entity with name \a name
> + * \param name The entity name

Please expand documentation a little bit. For instance you should explain that 
this function returns nullptr if the entity can't be found. In general a one-
line documentation that just duplicates the function name isn't very useful, 
the real value comes from what isn't expressed in the name.

Documentation is hard :-)

> + */
> +MediaEntity *MediaDevice::getEntityByName(const std::string &name)
> +{
> +	auto it = entities_.begin();
> +
> +	while (it != entities_.end()) {
> +		MediaEntity *e = *it;
> +		if (!(e->name().compare(name)))
> +			return e;
> +		it++;
> +	}
> +
> +	return nullptr;
> +}
> +
> +/**
> + * \fn MediaDevice::enumerateLinks(struct media_v2_topology &topology)
> + * \brief Enumerate all links in the system and associate them with their
> + * source and sink pads
> + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> + */
> +int MediaDevice::enumerateLinks(struct media_v2_topology &topology)

Shouldn't the argument be const ? I would have used a pointer instead of a 
reference, but I'm not sure why.

> +{
> +	struct media_v2_link *link = reinterpret_cast<struct media_v2_link *>
> +				     (topology.ptr_links);
> +
> +	for (unsigned int i = 0; i < topology.num_links; i++, link++) {
> +		/*
> +		 * Skip links between entities and interfaces: we only care
> +		 * about pad-2-pad links here.
> +		 */
> +		if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> +		    MEDIA_LNK_FL_INTERFACE_LINK)
> +			continue;
> +
> +		MediaLink *mediaLink = new MediaLink(link);

s/mediaLink/link/ ?

You would need to rename the link variable above though, I'm not sure which 
one is best.

> +		addObject(mediaLink);
> +
> +		/* Store reference to this mediaLink in the link's source pad. */
> +		MediaPad *mediaPad = dynamic_cast<MediaPad *>
> +				     (getObject(mediaLink->source()));

s/mediaPad/pad/ ?

> +		if (!mediaPad) {
> +			LOG(Error) << "Failed to find pad with id: "

Nitpicking, s/://.

> +				   << mediaLink->source();
> +			return -ENODEV;
> +		}
> +		mediaPad->addLink(mediaLink);
> +
> +		/* Store reference to this mediaLink in the link's sink pad. */
> +		mediaPad = dynamic_cast<MediaPad *>(getObject(mediaLink->sink()));
> +		if (!mediaPad) {
> +			LOG(Error) << "Failed to find pad with id: "
> +				   << mediaLink->sink();
> +			return -ENODEV;
> +		}
> +		mediaPad->addLink(mediaLink);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn MediaDevice::enumeratePads(struct media_v2_topology &topology)
> + * \brief Enumerate all pads in the system and associate them with the
> + * entity they belong to
> + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> + */
> +int MediaDevice::enumeratePads(struct media_v2_topology &topology)
> +{
> +	struct media_v2_pad *pad = reinterpret_cast<struct media_v2_pad *>
> +				   (topology.ptr_pads);
> +
> +	for (unsigned int i = 0; i < topology.num_pads; i++, pad++) {
> +		MediaPad *mediaPad = new MediaPad(pad);
> +		addObject(mediaPad);
> +
> +		/* Store a reference to this MediaPad in pad's entity. */
> +		MediaEntity *mediaEntity = dynamic_cast<MediaEntity *>
> +					   (getObject(mediaPad->entity()));

s/mediaPad/pad/ and s/mediaEntity/entity/ ?

> +		if (!mediaEntity) {
> +			LOG(Error) << "Failed to find entity with id: "
> +				   << mediaPad->entity();
> +			return -ENODEV;
> +		}
> +
> +		mediaEntity->addPad(mediaPad);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn MediaDevice::enumerateEntities(std::map<std::string, std::string> &,
> + *                                    struct media_v2_topology &topology)
> + * \brief Enumerate and initialize entities in the media graph
> + * \param entitiesMap Map entities names to their video (sub)device node
> + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> + *
> + * Associate the video (sub)device path to the entity name as returned by

As stated before, I'd talk about device nodes, as this isn't V4L2-specific.

> + * MEDIA_IOC_G_TOPOLOGY
> + */
> +int MediaDevice::enumerateEntities(std::map<std::string, std::string>
> &entitiesMap,
> +				   struct media_v2_topology &topology)
> +{
> +	struct media_v2_entity *entities = reinterpret_cast<struct media_v2_entity
> *>
> +					   (topology.ptr_entities);
> +
> +	for (unsigned int i = 0; i < topology.num_entities; ++i) {
> +		auto it = entitiesMap.find(entities[i].name);
> +		if (it == entitiesMap.end()) {
> +			LOG(Error) << "Entity " << entities[i].name
> +				   << " not found in media entities map";
> +			return -ENOENT;
> +		}
> +
> +		MediaEntity *entity = new MediaEntity(&entities[i]);
> +		if (entity->setDevice(it->second)) {
> +			delete entity;
> +			goto delete_entities;

You could call deleteObjects() and return here directly.

> +		}
> +
> +		addObject(entity);
> +		entities_.push_back(entity);
> +	}
> +
> +	return 0;
> +
> +delete_entities:
> +	deleteObjects();
> +
> +	return -errno;

-errno isn't correct. You should assign the return value of setDevice() to a 
ret variable and return ret.

> +}
> +
> +/**
> + * \fn MediaDevice::enumerate(std::map<std::string, std::string>)
> + * \brief Enumerate the media graph topology
> + * \param entitiesMap Map entities names to their video (sub)device node
> + * FIXME: this is statically provided by the caller at the moment.
> + *
> + * This functions enumerates all media objects, registered in the media
> graph,

s/functions/function/
s/,//g

> + * through the MEDIA_IOC_G_TOPOLOGY ioctl. For each returned entity,
> + * it creates and store its representation for later reuse.

Let's expand this a little.

"This function enumerates all media graph objects in the media device and 
populates the internal list of objects. All entities, pads and links are 
stored as MediaEntity, MediaPad and MediaLink respectively, with cross-
references between objects. Media interfaces are not processed.

For entities paired with a device node, the path to the device node is stored 
in the MediaEntity object, based on the associations set in the \a entitiesMap 
parameter.

\return Return 0 on success or a negative error code on error."

> + */
> +int MediaDevice::enumerate(std::map<std::string, std::string> &entitiesMap)
> +{
> +	struct media_v2_topology topology = { };
> +	unsigned int num_interfaces;
> +	unsigned int num_links;
> +	unsigned int num_pads;
> +	unsigned int num_ent;
> +
> +	do {
> +		num_ent = topology.num_entities;
> +		num_pads = topology.num_pads;
> +		num_links = topology.num_links;
> +		num_interfaces = topology.num_interfaces;
> +
> +		/* Call G_TOPOLOGY the first time here to enumerate .*/
> +		if (ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology)) {
> +			LOG(Error) << "Failed to enumerate media topology on"
> +				   << path_ << ": " << strerror(errno);
> +			return -errno;
> +		}
> +
> +		/*
> +		 * Repeat the call until we don't get a 'stable' number
> +		 * of media objects.
> +		 */
> +	} while (num_ent != topology.num_entities ||
> +		 num_pads != topology.num_pads    ||
> +		 num_links != topology.num_links  ||
> +		 num_interfaces != topology.num_interfaces);

That's not very useful, as the need to ensure that the topology doesn't change 
stems from the fact we call MEDIA_IOC_G_TOPOLOGY (at least) twice, once to 
retrieve the number of entities, and a second time to retrieve the graph 
itself. The graph could be updated in the kernel after this loop and before 
the next MEDIA_IOC_G_TOPOLOGY.

I proposed a way to call MEDIA_IOC_G_TOPOLOGY from a single location in a 
previous e-mail. We can fix this on top of this series, or:


	struct media_v2_topology topology = { };
	struct media_v2_entity *entities;
	struct media_v2_pad *pads;
	struct media_v2_link *links;
	struct media_v2_interface *interfaces;
	unsigned int num_entities;
	unsigned int num_pads;
	unsigned int num_links;
	unsigned int num_interfaces;
	int ret;

	while (1) {
		num_entitites = topology.num_entities;
		num_pads = topology.num_pads;
		num_links = topology.num_links;
		num_interfaces = topology.num_interfaces;

		pads = new media_v2_pad[topology.num_pads];
		entities = new media_v2_entity[topology.num_entities];
		topology.ptr_entities = reinterpret_cast<__u64>(entities);

		topology.ptr_pads = reinterpret_cast<__u64>(pads);

		links = new media_v2_link[topology.num_links];
		topology.ptr_links = reinterpret_cast<__u64>(links);
		links = new media_v2_interface[topology.num_interfaces];
		topology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);

		ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
		if (ret < 0) {
			ret = -errno;
			LOG(Error) << "Failed to enumerate media topology on"
				   << path_ << ": " << strerror(-ret);
			goto done;
		}

		/*
		 * If all objects have been enumerated, stop now. If the number
		 * of objects has increased (after the first call or due to a
		 * topology change, retry the enumeration.
		 */
		if (num_entities >= topology.num_entities &&
		    num_pads >= topology.num_pads &&
		    num_links >= topology.num_links &&
		    num_interfaces >= topology.num_interfaces)
			break;
	}

	ret = enumerateEntities(entitiesMap, topology);
	if (ret)
		goto done;

	ret = enumeratePads(topology);
	if (ret)
		goto done;

	ret = enumerateLinks(topology);
	if (ret)
		goto done;

done:
	if (ret < 0)
		deleteObjects();

	delete[] entities;
	delete[] links;
	delete[] pads;
	delete[] interfaces;

	return ret;

(untested)

I wonder if enumerateEntities, enumeratePads and enumerateLinks should be 
renamed as s/enumerate/populate/.

> +	auto *_ptr_e = new media_v2_entity[topology.num_entities];
> +	topology.ptr_entities = reinterpret_cast<__u64>(_ptr_e);
> +
> +	auto *_ptr_p = new media_v2_pad[topology.num_pads];
> +	topology.ptr_pads = reinterpret_cast<__u64>(_ptr_p);
> +
> +	auto *_ptr_l = new media_v2_link[topology.num_links];
> +	topology.ptr_links = reinterpret_cast<__u64>(_ptr_l);
> +
> +	/* Call G_TOPOLOGY again, this time with memory reserved. */
> +	int ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> +	if (ret < 0) {
> +		LOG(Error) << "Failed to enumerate media topology on " << path_
> +			   << ": " << strerror(errno);
> +		ret = -errno;
> +		goto error_free_mem;
> +	}
> + 
> +	ret = enumerateEntities(entitiesMap, topology);
> +	if (ret)
> +		goto error_free_mem;
> +
> +	ret = enumeratePads(topology);
> +	if (ret)
> +		goto error_free_objs;
> +
> +	ret = enumerateLinks(topology);
> +	if (ret)
> +		goto error_free_objs;
> +
> +	delete[] _ptr_e;
> +	delete[] _ptr_p;
> +	delete[] _ptr_l;
> +
> +	return 0;
> +
> +error_free_objs:
> +	deleteObjects();
> +
> +error_free_mem:
> +	delete[] _ptr_e;
> +	delete[] _ptr_p;
> +	delete[] _ptr_l;
> +
> +	return ret;
> +}
> +
> +void MediaDevice::dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os)

s/p/pad/

Unless names get really long, let's try to spell them out, as it increases 
readability.

> +{
> +	os << "\t  \"" << e->name() << "\"["
> +	   << p->index() << "]";
> +}
> +
> +void MediaDevice::dumpRemote(MediaLink *l, std::ostream &os)

s/l/link/

> +{
> +

Stray blank line.

> +	MediaPad *remotePad = dynamic_cast<MediaPad *>
> +			      (getObject(l->sink()));

Would it make sense for the sink() and source() methods to return pointers to 
MediaPad instead of an id ? I would in that case store the pointers internally 
in MediaLink, to avoid a call to getObject() every time.

> +	if (!remotePad)
> +		return;

This check wouldn't be needed then.

> +
> +	MediaEntity *remoteEntity = dynamic_cast<MediaEntity *>
> +				    (getObject(remotePad->entity()));
> +	if (!remoteEntity)
> +		return;

Same here, returning a MediaEntity pointer, and removing the check.

> +	os << "\"" << remoteEntity->name() << "\"["
> +	   << remotePad->index() << "]";
> +}
> +
> +void MediaDevice::dumpLink(MediaLink *l, std::ostream &os)
> +{
> +	unsigned int flags = l->flags();
> +
> +	os << " [";
> +	if (flags) {
> +		os << (flags & MEDIA_LNK_FL_ENABLED ? "ENABLED," : "")
> +		   << (flags & MEDIA_LNK_FL_IMMUTABLE ? "IMMUTABLE" : "");
> +	}
> +	os  << "]\n";
> +}
> +
> +/**
> + * \fn MediaDevice::dumpGraph(std::ostream)
> + * \brief Dump the media device topology in textual form to an output
> stream
> + * \param os The output stream where to append the printed topology to

s/append/write/ ?

> + */
> +void MediaDevice::dumpGraph(std::ostream &os)
> +{
> +	os << "\n" << name_ << " - " << path_ << "\n\n";
> +
> +	for (auto const &e : entities_) {

s/auto const &e/const struct MediaEntity */

Let's restrict the use of auto to cases where it would be impractical to write 
the type out explicitly, as auto reduces readability and removes compile-time 
checks.

> +		os << "\"" << e->name() << "\"\n";
> +
> +		for (auto const &p : e->sinks()) {
> +			os << "  [" << p->index() << "]" << ": Sink\n";
> +			for (auto const &l : p->links()) {
> +				dumpLocal(e, p, os);
> +				os << " <- ";
> +				dumpRemote(l, os);
> +				dumpLink(l, os);
> +			}
> +			os << "\n";
> +		}
> +
> +		for (auto const &p : e->sources()) {
> +			os << "  [" << p->index() << "]" << ": Source\n";
> +			for (auto const &l : p->links()) {
> +				dumpLocal(e, p, os);
> +				os << " -> ";
> +				dumpRemote(l, os);
> +				dumpLink(l, os);
> +			}
> +			os << "\n";
> +		}
> +	}
> +}

I wonder whether dumpGraph() belongs here, or whether it should be moved to a 
test. MediaDevice isn't exposed through the public API, how do you foresee 
dumpGraph being used ? If it can never be called from the public API there's 
no point in having it in the library :-)

> +/**
> + * \fn MediaDevice::setupLink(MediaPad *source, MediaPad *sink)

you're missing const (assuming we need to list the parameters explicitly here, 
see the comment above).

> + * \brief Apply \a flags to the link between \a source and \a sink pads
> + * \param source The source MediaPad
> + * \param sink The sink MediaPad
> + * \param link The MediaLink to operate on

Why do you need to specify both source and sink, and the link ? The link 
should identify the source and sink already. If you don't expect the caller to 
have the link pointer you should look it up internally, otherwise you 
shouldn't pass the source and sink. And if you expect the caller to have the 
link pointer, maybe you could make this function a member of the MediaLink 
class ?

> + * \param flags Flags to be applied to the link (MEDIA_LNK_FL_*)
> + */
> +int MediaDevice::setupLink(const MediaPad *source, const MediaPad *sink,
> +			   MediaLink *link, unsigned int flags)
> +{
> +	struct media_link_desc linkDesc = { };
> +
> +	linkDesc.source.entity = source->entity();
> +	linkDesc.source.index = source->index();
> +	linkDesc.source.flags = MEDIA_PAD_FL_SOURCE;
> +
> +	linkDesc.sink.entity = sink->entity();
> +	linkDesc.sink.index = sink->index();
> +	linkDesc.sink.flags = MEDIA_PAD_FL_SINK;
> +
> +	linkDesc.flags = flags;
> +
> +	if (ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc)) {
> +		LOG(Error) << "Failed to setup link: "
> +			   << strerror(errno);
> +		return -errno;
> +	}
> +
> +	link->setFlags(0);
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn MediaDevice::resetLinks()
> + * \brief Reset all links on the media graph

Reset to what ? :-)

> + *
> + * Walk all registered entities, and disable all links from their
> + * source pads to other pads.

Wouldn't it be more efficient to walk links instead of entities and pads ?

Could you expand the documentation to explain the use cases for this function 
? Please also document the return value (this holds true for all other 
functions returning a value in the whole series).

> + */
> +int MediaDevice::resetLinks()
> +{
> +	for (MediaEntity *e : entities_) {
> +		for (MediaPad *sourcePad : e->sources()) {
> +			for (MediaLink *l : sourcePad->links()) {
> +				/*
> +				 * Do not reset links that are not enabled
> +				 * or immutable.
> +				 */
> +				if (l->flags() & MEDIA_LNK_FL_IMMUTABLE)
> +					continue;
> +
> +				if (!(l->flags() & MEDIA_LNK_FL_ENABLED))
> +					continue;
> +
> +				/* Get the remote sink pad. */
> +				MediaPad *sinkPad = dynamic_cast<MediaPad *>
> +						    (getObject(l->sink()));
> +				if (!sinkPad)
> +					return -ENOENT;
> +
> +				/* Also get entity to make sure IDs are ok. */
> +				MediaEntity *sinkEntity =
> +						dynamic_cast<MediaEntity *>
> +						(getObject(sinkPad->entity()));
> +				if (!sinkEntity)
> +					return -ENOENT;
> +
> +				int ret = setupLink(sourcePad, sinkPad, l, 0);
> +				if (ret) {
> +					LOG(Error) << "Link reset failed: "
> +						   << e->name() << "["
> +						   << sourcePad->index()
> +						   << "] -> "
> +						   << sinkEntity->name() << "["
> +						   << sinkPad->index() << "]";
> +					return ret;
> +				}
> +
> +				LOG(Info) << "Link reset: "
> +					  << e->name() << "["
> +					  << sourcePad->index()
> +					  << "] -> "
> +					  << sinkEntity->name() << "["
> +					  << sinkPad->index() << "]";
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn MediaDevice::link(std::string, unsigned int, std::string, unsigned
> int)
> + * \brief Setup a link identified by the entities name and their source and
> + * sink pad indexes
> + * \param source The source entity name
> + * \param sourceIdx The source pad index
> + * \param sink The sink entity name
> + * \param sinkIdx The sink pad index
> + * \param flags The link setup flag (see MEDIA_LNK_FL_*)

If I understand the purpose of this function properly (and the fact that I'm 
not sure I do means the documentation should be improved :-)), it serves the 
same purpose as MediaDevice::setupLink(), but with different arguments. I 
would thus name the two functions setupLink(), as C++ allows overloading 
functions.

I would go one step further though, at least if you agree with my proposition 
to move setupLink to MediaLink, and turn this function into a link lookup. The 
callers would then do something similar to

	MediaDevice *dev = ...;
	MediaLink *link = dev->link("source", 0, "sink", 3);
	if (!link)
		return -ENODEV;
	link->setup(flags);

and possibly cache the link pointer internally to avoid looking up links all 
the time.

> + */
> +int MediaDevice::link(const std::string &source, unsigned int sourceIdx,
> +		      const std::string &sink, unsigned int sinkIdx,

At the cost of longer names, sourcePadIndex and sinkPadIndex ?

> +		      unsigned int flags)
> +{
> +

Stray blank line.

> +	/* Make sure the supplied link is well formed. */
> +	MediaEntity *sourceEntity = getEntityByName(source);
> +	if (!sourceEntity) {
> +		LOG(Error) << "Entity name: " << source << "not found";
> +		return -ENOENT;
> +	}
> +
> +	MediaEntity *sinkEntity = getEntityByName(sink);
> +	if (!sinkEntity) {
> +		LOG(Error) << "Entity name: " << source << "not found";

s/source/sink/
s/Entity name:/Entity/

> +		return -ENOENT;
> +	}
> +
> +	const MediaPad *sourcePad = sourceEntity->getPadByIndex(sourceIdx);
> +	if (!sourcePad) {
> +		LOG(Error) << "Pad " << sourceIdx << "not found in entity "
> +			   << sourceEntity->name();
> +		return -ENOENT;
> +	}
> +
> +	const MediaPad *sinkPad = sinkEntity->getPadByIndex(sinkIdx);
> +	if (!sinkPad) {
> +		LOG(Error) << "Pad " << sinkIdx << "not found in entity "
> +			   << sinkEntity->name();
> +		return -ENOENT;
> +	}
> +
> +	/*
> +	 * Walk all links in the source and search for an entry matching the
> +	 * pad ids. If none, the requested link does not exists.
> +	 */
> +	MediaLink *validLink = nullptr;
> +	for (MediaLink *link : sourcePad->links()) {
> +		if (link->source() != sourcePad->id())
> +			continue;
> +
> +		if (link->sink() != sinkPad->id())
> +			continue;
> +
> +		validLink = link;
> +		break;
> +	}
> +
> +	if (!validLink) {
> +		LOG(Error) << "Link not found"
> +			   << "\"" << sourceEntity->name() << "\"["
> +			   << sourcePad->index() << "] -> "
> +			   << "\"" << sinkEntity->name() << "\"["
> +			   << sinkPad->index() << "]";
> +		return -EINVAL;
> +	}
> +
> +	int ret = setupLink(sourcePad, sinkPad, validLink, flags);
> +	if (ret)
> +		return ret;
> +
> +	LOG(Info) << "Setup link: "

LOG(Debug) at most.

> +		  << "\"" << sourceEntity->name() << "\"["
> +		  << sourcePad->index() << "] -> "
> +		  << "\"" << sinkEntity->name() << "\"["
> +		  << sinkPad->index() << "]"
> +		  << " [" << flags << "]";
> +
> +	return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index da06eba..4cac687 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -2,6 +2,7 @@ libcamera_sources = files([
>      'log.cpp',
>      'main.cpp',
>      'media_object.cpp',
> +    'media_device.cpp',

sort

>  ])
> 
>  libcamera_headers = files([

HI Laurent, quite a few comments...

On Fri, Dec 28, 2018 at 03:27:21PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Friday, 28 December 2018 09:57:41 EET Jacopo Mondi wrote:
> > The MediaDevice object implements handling and configuration of the media
> > graph associated with a V4L2 media device.
>
> Let's not talk about V4L2 here, it's just MC, not V4L2.
>
> > The class allows enumeration of all pads, links and entities registered in
> > the media graph, and provides methods to setup and reset media links.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/include/media_device.h |  71 ++++
> >  src/libcamera/media_device.cpp       | 596 +++++++++++++++++++++++++++
> >  src/libcamera/meson.build            |   1 +
> >  3 files changed, 668 insertions(+)
> >  create mode 100644 src/libcamera/include/media_device.h
> >  create mode 100644 src/libcamera/media_device.cpp
> >
> > diff --git a/src/libcamera/include/media_device.h
> > b/src/libcamera/include/media_device.h new file mode 100644
> > index 0000000..642eea9
> > --- /dev/null
> > +++ b/src/libcamera/include/media_device.h
> > @@ -0,0 +1,71 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * media_device.h - Media device handler
> > + */
> > +#ifndef __LIBCAMERA_MEDIA_DEVICE_H__
> > +#define __LIBCAMERA_MEDIA_DEVICE_H__
> > +
> > +#include <map>
> > +#include <string>
> > +#include <sstream>
>
> sort
>
> > +#include <vector>
> > +
> > +#include <linux/media.h>
> > +
> > +#include "log.h"
>
> Is this needed ?
>
> > +#include "media_object.h"
> > +
> > +namespace libcamera {
> > +
> > +class MediaDevice
> > +{
> > +public:
> > +	MediaDevice() : fd_(-1) { };
> > +	~MediaDevice();
> > +
> > +	const std::string name() const { return name_; }
> > +	const std::string path() const { return path_; }
> > +
> > +	int open(const std::string &path);
> > +	int close();
> > +	int enumerate(std::map<std::string, std::string> &entitiesMap);
> > +	void dumpGraph(std::ostream &os);
> > +
> > +	int resetLinks();
> > +	int link(const std::string &source, unsigned int sourceIdx,
> > +		 const std::string &sink, unsigned int sinkIdx,
> > +		 unsigned int flags);
> > +
> > +private:
> > +	/** The media device file descriptor */
>
> Wrong file ?
>
Sorry, I didn't get this.

> > +	int fd_;
> > +	/** The media device name as returned by MEDIA_IOC_DEVICE_INFO */
>
> Please take my comments on 1/4 into account regarding wording of documentation
> here.

I'll drop documentation from private members

>
> This field stores the driver name, I would thus name it driver_ (or possibly
> driverName_).
>
> > +	std::string name_;
> > +	/** The media device path */
> > +	std::string path_;
> > +
> > +	std::map<unsigned int, MediaObject *> mediaObjects_;
>
> s/mediaObjects_/objects_/ ?
>

Actually I don't agree, here and all other places in the patch where
an s/mediaSomething/something/ has been suggested.

This class deals both with struct_v2_something and MediaSomething. I
would like to keep clear when we're operating on the object
representation and when on the kernel provided structure.

> > +	MediaObject *getObject(unsigned int id);
> > +	void addObject(MediaObject *obj);
> > +	void deleteObjects();
> > +
> > +	std::vector<MediaEntity *> entities_;
> > +	MediaEntity *getEntityByName(const std::string &name);
> > +
> > +	int enumerateEntities(std::map<std::string, std::string> &entitiesMap,
> > +			      struct media_v2_topology &topology);
> > +	int enumeratePads(struct media_v2_topology &topology);
> > +	int enumerateLinks(struct media_v2_topology &topology);
> > +
> > +	int setupLink(const MediaPad *source, const MediaPad *sink,
> > +		      MediaLink *link, unsigned int flags);
> > +
> > +	void dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os);
> > +	void dumpRemote(MediaLink *l, std::ostream &os);
> > +	void dumpLink(MediaLink *l, std::ostream &os);
> > +};
> > +
> > +} /* namespace libcamera */
> > +#endif /* __LIBCAMERA_MEDIA_DEVICE_H__ */
> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> > new file mode 100644
> > index 0000000..b98d62f
> > --- /dev/null
> > +++ b/src/libcamera/media_device.cpp
> > @@ -0,0 +1,596 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * media_device.cpp - Media device handler
> > + */
> > +
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +#include <unistd.h>
> > +
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <linux/media.h>
> > +
> > +#include "log.h"
> > +#include "media_device.h"
> > +
> > +/**
> > + * \file media_device.h
> > + */
> > +namespace libcamera {
> > +
> > +/**
> > + * \class MediaDevice
> > + * \brief Media device handler
> > + *
> > + * MediaDevice handles the media graph associated with a V4L2 media device.
> > + */
> > +
> > +/**
> > + * \fn MediaDevice::~MediaDevice()
> > + * \brief Close the media device file descriptor and release entities
> > + */
> > +MediaDevice::~MediaDevice()
> > +{
> > +	if (fd_ > -1)
>
> I would have written != -1.
>

Any reason why it's better?

> > +		::close(fd_);
> > +	deleteObjects();
> > +}
> > +
> > +/**
> > + * \fn MediaDevice::name()
> > + * \brief Return the media device name
> > + */
> > +
> > +/**
> > + * \fn MediaDevice::path()
> > + * \brief Return the media device path node associated with this
> > MediaDevice
>
> I'd name this devnode() instead of path().
>

I already though about chnging this, right...

> > + */
> > +
> > +/**
> > + * \fn MediaDevice::deleteObjects()
> > + * \brief Delete all registered entities in the MediaDevice object
> > + */
> > +void MediaDevice::deleteObjects()
> > +{
> > +	for (auto const &e : mediaObjects_)
> > +		delete e.second;
> > +
> > +	mediaObjects_.clear();
> > +	entities_.clear();
> > +}
> > +
> > +/**
> > + * \fn int MediaDevice::open(std::string)
>
> Do you need to specify the arguments when there's a single function by that
> name ? If so, shouldn't it be (const std::string &) ?
>
> Same comment for other functions in this patch series.

I think I could drop the function name completely when the comment
block is just before the function definition.

>
> > + * \brief Open a media device and initialize its components.
> > + * \param path The media device path
> > + */
> > +int MediaDevice::open(const std::string &path)
>
> s/path/devnode/ ?
>
> > +{
>
> What if the device is already open ? It would be useful to add some
> documentation to explain the "life cycle" of the object, as in how it is
> supposed to be used (and what happens when it's incorrectly used). This can be
> done in the open() documentation (and referenced from close() and possibly
> other functions), or in the \class documentation.
>

Right, currently there's no protection for that in this
implementation. I should return an error if (fd != -1).

I could just state the caller is responsible for opening and closing
the device and that access is exclusive. In the future, this might be
superseded by ref-counted acquire/release operations, with open() and
close() then made private.

> > +	fd_ = ::open(path.c_str(), O_RDWR);
> > +	if (fd_ < 0) {
> > +		LOG(Error) << "Failed to open media device at " << path
> > +			   << ": " << strerror(errno);
> > +		return -errno;
>
> The LOG() call may overwrite errno. You will need a local variable. How about
>
> 	int ret;
>
> 	ret = ::open(path.c_str(), O_RDWR);
> 	if (ret < 0) {
> 		ret = -errno;
> 		LOG(Error) << "Failed to open media device at " << path
> 			   << ": " << strerror(-ret);
> 		return ret;
> 	}
>
> 	fd_ = ret;
> 	path_ = path;
>

Right. Thanks, this is important, I would like to see all error
handling standardized in the library.

> > +	}
> > +	path_ = path;
> > +
> > +	struct media_device_info info = { };
> > +	int ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
> > +	if (ret) {
> > +		LOG(Error) << "Failed to get media device info "
> > +			   << ": " << strerror(errno);
> > +		return -errno;
> > +	}
> > +
> > +	name_ = info.driver;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \fn MediaDevice::close()
> > + * \brief Close the file descriptor associated with the media device
> > + */
> > +int MediaDevice::close()
> > +{
> > +	if (fd_ > -1)
>
> I'd write this != -1 too.
>
> > +		return ::close(fd_);
> > +
> > +	return 0;
>
> Do you use the return value ?
>
> > +}
> > +
> > +void MediaDevice::addObject(MediaObject *obj)
> > +{
> > +
> > +	if (mediaObjects_.find(obj->id()) != mediaObjects_.end()) {
> > +		LOG(Error) << "Element with id " << obj->id()
> > +			   << " already enumerated.";
> > +		return;
>
> Shouldn't you propagate the error to the caller ? This seems pretty fatal to
> me.
>

This should not happen of course. In case it does, I'll return
-EEXIST.

> > +	}
> > +
> > +	mediaObjects_[obj->id()] = obj;
> > +}
> > +
> > +MediaObject *MediaDevice::getObject(unsigned int id)
> > +{
> > +	auto it = mediaObjects_.find(id);
> > +	return (it == mediaObjects_.end()) ?
> > +	       nullptr : it->second;
> > +}
> > +
> > +/**
> > + * \fn MediaDevice::getEntityByName(std::string)
> > + * \brief Return entity with name \a name
> > + * \param name The entity name
>
> Please expand documentation a little bit. For instance you should explain that
> this function returns nullptr if the entity can't be found. In general a one-
> line documentation that just duplicates the function name isn't very useful,
> the real value comes from what isn't expressed in the name.
>

I still feel that a function named getEntityByName() is pretty clear
without having to document it in detail. The return value though might
be pointed out, even if, it's exactly what one would expect, the
entity or nullptr.

> Documentation is hard :-)
>

Drawing the line between useful documentation and writing comments to
silence doxygen "not documented" warning is. I clearly went for the second
option and documented everything doxygen complained about but that's
clearly not useful if not for ticking a box. I would be happy to chop
documentation parts here and there.

> > + */
> > +MediaEntity *MediaDevice::getEntityByName(const std::string &name)
> > +{
> > +	auto it = entities_.begin();
> > +
> > +	while (it != entities_.end()) {
> > +		MediaEntity *e = *it;
> > +		if (!(e->name().compare(name)))
> > +			return e;
> > +		it++;
> > +	}
> > +
> > +	return nullptr;
> > +}
> > +
> > +/**
> > + * \fn MediaDevice::enumerateLinks(struct media_v2_topology &topology)
> > + * \brief Enumerate all links in the system and associate them with their
> > + * source and sink pads
> > + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> > + */
> > +int MediaDevice::enumerateLinks(struct media_v2_topology &topology)
>
> Shouldn't the argument be const ? I would have used a pointer instead of a
> reference, but I'm not sure why.
>
> > +{
> > +	struct media_v2_link *link = reinterpret_cast<struct media_v2_link *>
> > +				     (topology.ptr_links);
> > +
> > +	for (unsigned int i = 0; i < topology.num_links; i++, link++) {
> > +		/*
> > +		 * Skip links between entities and interfaces: we only care
> > +		 * about pad-2-pad links here.
> > +		 */
> > +		if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> > +		    MEDIA_LNK_FL_INTERFACE_LINK)
> > +			continue;
> > +
> > +		MediaLink *mediaLink = new MediaLink(link);
>
> s/mediaLink/link/ ?
>
> You would need to rename the link variable above though, I'm not sure which
> one is best.

That's what I meant. link for media_v2_link, and mediaLink for
MediaLink. It makes sense to me.

>
> > +		addObject(mediaLink);
> > +
> > +		/* Store reference to this mediaLink in the link's source pad. */
> > +		MediaPad *mediaPad = dynamic_cast<MediaPad *>
> > +				     (getObject(mediaLink->source()));
>
> s/mediaPad/pad/ ?
>

ditto

> > +		if (!mediaPad) {
> > +			LOG(Error) << "Failed to find pad with id: "
>
> Nitpicking, s/://.
>
> > +				   << mediaLink->source();
> > +			return -ENODEV;
> > +		}
> > +		mediaPad->addLink(mediaLink);
> > +
> > +		/* Store reference to this mediaLink in the link's sink pad. */
> > +		mediaPad = dynamic_cast<MediaPad *>(getObject(mediaLink->sink()));
> > +		if (!mediaPad) {
> > +			LOG(Error) << "Failed to find pad with id: "
> > +				   << mediaLink->sink();
> > +			return -ENODEV;
> > +		}
> > +		mediaPad->addLink(mediaLink);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \fn MediaDevice::enumeratePads(struct media_v2_topology &topology)
> > + * \brief Enumerate all pads in the system and associate them with the
> > + * entity they belong to
> > + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> > + */
> > +int MediaDevice::enumeratePads(struct media_v2_topology &topology)
> > +{
> > +	struct media_v2_pad *pad = reinterpret_cast<struct media_v2_pad *>
> > +				   (topology.ptr_pads);
> > +
> > +	for (unsigned int i = 0; i < topology.num_pads; i++, pad++) {
> > +		MediaPad *mediaPad = new MediaPad(pad);
> > +		addObject(mediaPad);
> > +
> > +		/* Store a reference to this MediaPad in pad's entity. */
> > +		MediaEntity *mediaEntity = dynamic_cast<MediaEntity *>
> > +					   (getObject(mediaPad->entity()));
>
> s/mediaPad/pad/ and s/mediaEntity/entity/ ?
>

ditto ditto ;)

> > +		if (!mediaEntity) {
> > +			LOG(Error) << "Failed to find entity with id: "
> > +				   << mediaPad->entity();
> > +			return -ENODEV;
> > +		}
> > +
> > +		mediaEntity->addPad(mediaPad);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \fn MediaDevice::enumerateEntities(std::map<std::string, std::string> &,
> > + *                                    struct media_v2_topology &topology)
> > + * \brief Enumerate and initialize entities in the media graph
> > + * \param entitiesMap Map entities names to their video (sub)device node
> > + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> > + *
> > + * Associate the video (sub)device path to the entity name as returned by
>
> As stated before, I'd talk about device nodes, as this isn't V4L2-specific.
>
> > + * MEDIA_IOC_G_TOPOLOGY
> > + */
> > +int MediaDevice::enumerateEntities(std::map<std::string, std::string>
> > &entitiesMap,
> > +				   struct media_v2_topology &topology)
> > +{
> > +	struct media_v2_entity *entities = reinterpret_cast<struct media_v2_entity
> > *>
> > +					   (topology.ptr_entities);
> > +
> > +	for (unsigned int i = 0; i < topology.num_entities; ++i) {
> > +		auto it = entitiesMap.find(entities[i].name);
> > +		if (it == entitiesMap.end()) {
> > +			LOG(Error) << "Entity " << entities[i].name
> > +				   << " not found in media entities map";
> > +			return -ENOENT;
> > +		}
> > +
> > +		MediaEntity *entity = new MediaEntity(&entities[i]);
> > +		if (entity->setDevice(it->second)) {
> > +			delete entity;
> > +			goto delete_entities;
>
> You could call deleteObjects() and return here directly.
>
> > +		}
> > +
> > +		addObject(entity);
> > +		entities_.push_back(entity);
> > +	}
> > +
> > +	return 0;
> > +
> > +delete_entities:
> > +	deleteObjects();
> > +
> > +	return -errno;
>
> -errno isn't correct. You should assign the return value of setDevice() to a
> ret variable and return ret.
>
> > +}
> > +
> > +/**
> > + * \fn MediaDevice::enumerate(std::map<std::string, std::string>)
> > + * \brief Enumerate the media graph topology
> > + * \param entitiesMap Map entities names to their video (sub)device node
> > + * FIXME: this is statically provided by the caller at the moment.
> > + *
> > + * This functions enumerates all media objects, registered in the media
> > graph,
>
> s/functions/function/
> s/,//g
>
> > + * through the MEDIA_IOC_G_TOPOLOGY ioctl. For each returned entity,
> > + * it creates and store its representation for later reuse.
>
> Let's expand this a little.
>
> "This function enumerates all media graph objects in the media device and
> populates the internal list of objects. All entities, pads and links are
> stored as MediaEntity, MediaPad and MediaLink respectively, with cross-
> references between objects. Media interfaces are not processed.
>
> For entities paired with a device node, the path to the device node is stored
> in the MediaEntity object, based on the associations set in the \a entitiesMap
> parameter.
>
> \return Return 0 on success or a negative error code on error."
>
> > + */
> > +int MediaDevice::enumerate(std::map<std::string, std::string> &entitiesMap)
> > +{
> > +	struct media_v2_topology topology = { };
> > +	unsigned int num_interfaces;
> > +	unsigned int num_links;
> > +	unsigned int num_pads;
> > +	unsigned int num_ent;
> > +
> > +	do {
> > +		num_ent = topology.num_entities;
> > +		num_pads = topology.num_pads;
> > +		num_links = topology.num_links;
> > +		num_interfaces = topology.num_interfaces;
> > +
> > +		/* Call G_TOPOLOGY the first time here to enumerate .*/
> > +		if (ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology)) {
> > +			LOG(Error) << "Failed to enumerate media topology on"
> > +				   << path_ << ": " << strerror(errno);
> > +			return -errno;
> > +		}
> > +
> > +		/*
> > +		 * Repeat the call until we don't get a 'stable' number
> > +		 * of media objects.
> > +		 */
> > +	} while (num_ent != topology.num_entities ||
> > +		 num_pads != topology.num_pads    ||
> > +		 num_links != topology.num_links  ||
> > +		 num_interfaces != topology.num_interfaces);
>
> That's not very useful, as the need to ensure that the topology doesn't change
> stems from the fact we call MEDIA_IOC_G_TOPOLOGY (at least) twice, once to
> retrieve the number of entities, and a second time to retrieve the graph
> itself. The graph could be updated in the kernel after this loop and before
> the next MEDIA_IOC_G_TOPOLOGY.
>
> I proposed a way to call MEDIA_IOC_G_TOPOLOGY from a single location in a
> previous e-mail. We can fix this on top of this series, or:
>
>
> 	struct media_v2_topology topology = { };
> 	struct media_v2_entity *entities;
> 	struct media_v2_pad *pads;
> 	struct media_v2_link *links;
> 	struct media_v2_interface *interfaces;
> 	unsigned int num_entities;
> 	unsigned int num_pads;
> 	unsigned int num_links;
> 	unsigned int num_interfaces;
> 	int ret;
>
> 	while (1) {
> 		num_entitites = topology.num_entities;
> 		num_pads = topology.num_pads;
> 		num_links = topology.num_links;
> 		num_interfaces = topology.num_interfaces;
>
> 		pads = new media_v2_pad[topology.num_pads];
> 		entities = new media_v2_entity[topology.num_entities];
> 		topology.ptr_entities = reinterpret_cast<__u64>(entities);
>
> 		topology.ptr_pads = reinterpret_cast<__u64>(pads);
>
> 		links = new media_v2_link[topology.num_links];
> 		topology.ptr_links = reinterpret_cast<__u64>(links);
> 		links = new media_v2_interface[topology.num_interfaces];
> 		topology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);
>
> 		ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> 		if (ret < 0) {
> 			ret = -errno;
> 			LOG(Error) << "Failed to enumerate media topology on"
> 				   << path_ << ": " << strerror(-ret);
> 			goto done;
> 		}
>
> 		/*
> 		 * If all objects have been enumerated, stop now. If the number
> 		 * of objects has increased (after the first call or due to a
> 		 * topology change, retry the enumeration.
> 		 */
> 		if (num_entities >= topology.num_entities &&
> 		    num_pads >= topology.num_pads &&
> 		    num_links >= topology.num_links &&
> 		    num_interfaces >= topology.num_interfaces)
> 			break;
> 	}

According to documentation (and Niklas' implementation I have copied,
checking that topology->version does not change should be enough. Do
you agree?

By the way, I appreciate the concern regarding dynamic media graphs,
but we're here checking that media graph does not change in between
two ioctl calls, which is quite a strict race window, while it could
change at any other point in time after this call, and we won't
notice. I wonder if this is worth, but since I have code from you a
Niklas, I'll simply copy it in.
>
> 	ret = enumerateEntities(entitiesMap, topology);
> 	if (ret)
> 		goto done;
>
> 	ret = enumeratePads(topology);
> 	if (ret)
> 		goto done;
>
> 	ret = enumerateLinks(topology);
> 	if (ret)
> 		goto done;
>
> done:
> 	if (ret < 0)
> 		deleteObjects();
>
> 	delete[] entities;
> 	delete[] links;
> 	delete[] pads;
> 	delete[] interfaces;
>
> 	return ret;
>
> (untested)
>
> I wonder if enumerateEntities, enumeratePads and enumerateLinks should be
> renamed as s/enumerate/populate/.
>
> > +	auto *_ptr_e = new media_v2_entity[topology.num_entities];
> > +	topology.ptr_entities = reinterpret_cast<__u64>(_ptr_e);
> > +
> > +	auto *_ptr_p = new media_v2_pad[topology.num_pads];
> > +	topology.ptr_pads = reinterpret_cast<__u64>(_ptr_p);
> > +
> > +	auto *_ptr_l = new media_v2_link[topology.num_links];
> > +	topology.ptr_links = reinterpret_cast<__u64>(_ptr_l);
> > +
> > +	/* Call G_TOPOLOGY again, this time with memory reserved. */
> > +	int ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> > +	if (ret < 0) {
> > +		LOG(Error) << "Failed to enumerate media topology on " << path_
> > +			   << ": " << strerror(errno);
> > +		ret = -errno;
> > +		goto error_free_mem;
> > +	}
> > +
> > +	ret = enumerateEntities(entitiesMap, topology);
> > +	if (ret)
> > +		goto error_free_mem;
> > +
> > +	ret = enumeratePads(topology);
> > +	if (ret)
> > +		goto error_free_objs;
> > +
> > +	ret = enumerateLinks(topology);
> > +	if (ret)
> > +		goto error_free_objs;
> > +
> > +	delete[] _ptr_e;
> > +	delete[] _ptr_p;
> > +	delete[] _ptr_l;
> > +
> > +	return 0;
> > +
> > +error_free_objs:
> > +	deleteObjects();
> > +
> > +error_free_mem:
> > +	delete[] _ptr_e;
> > +	delete[] _ptr_p;
> > +	delete[] _ptr_l;
> > +
> > +	return ret;
> > +}
> > +
> > +void MediaDevice::dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os)
>
> s/p/pad/
>
> Unless names get really long, let's try to spell them out, as it increases
> readability.
>
> > +{
> > +	os << "\t  \"" << e->name() << "\"["
> > +	   << p->index() << "]";
> > +}
> > +
> > +void MediaDevice::dumpRemote(MediaLink *l, std::ostream &os)
>
> s/l/link/
>
> > +{
> > +
>
> Stray blank line.
>
> > +	MediaPad *remotePad = dynamic_cast<MediaPad *>
> > +			      (getObject(l->sink()));
>
> Would it make sense for the sink() and source() methods to return pointers to
> MediaPad instead of an id ? I would in that case store the pointers internally
> in MediaLink, to avoid a call to getObject() every time.
>

That calls for initializing two pointers at MediaLink creation time.
The same could happen for MediaPads, instead of storing their entity
ID they could store a pointer to the MediaEntity... I'll see how it
might look.

> > +	if (!remotePad)
> > +		return;
>
> This check wouldn't be needed then.
>
> > +
> > +	MediaEntity *remoteEntity = dynamic_cast<MediaEntity *>
> > +				    (getObject(remotePad->entity()));
> > +	if (!remoteEntity)
> > +		return;
>
> Same here, returning a MediaEntity pointer, and removing the check.
>
> > +	os << "\"" << remoteEntity->name() << "\"["
> > +	   << remotePad->index() << "]";
> > +}
> > +
> > +void MediaDevice::dumpLink(MediaLink *l, std::ostream &os)
> > +{
> > +	unsigned int flags = l->flags();
> > +
> > +	os << " [";
> > +	if (flags) {
> > +		os << (flags & MEDIA_LNK_FL_ENABLED ? "ENABLED," : "")
> > +		   << (flags & MEDIA_LNK_FL_IMMUTABLE ? "IMMUTABLE" : "");
> > +	}
> > +	os  << "]\n";
> > +}
> > +
> > +/**
> > + * \fn MediaDevice::dumpGraph(std::ostream)
> > + * \brief Dump the media device topology in textual form to an output
> > stream
> > + * \param os The output stream where to append the printed topology to
>
> s/append/write/ ?
>

Append is more appropriate, as the stream could already have content.

> > + */
> > +void MediaDevice::dumpGraph(std::ostream &os)
> > +{
> > +	os << "\n" << name_ << " - " << path_ << "\n\n";
> > +
> > +	for (auto const &e : entities_) {
>
> s/auto const &e/const struct MediaEntity */
>
> Let's restrict the use of auto to cases where it would be impractical to write
> the type out explicitly, as auto reduces readability and removes compile-time
> checks.
>

Agreed

> > +		os << "\"" << e->name() << "\"\n";
> > +
> > +		for (auto const &p : e->sinks()) {
> > +			os << "  [" << p->index() << "]" << ": Sink\n";
> > +			for (auto const &l : p->links()) {
> > +				dumpLocal(e, p, os);
> > +				os << " <- ";
> > +				dumpRemote(l, os);
> > +				dumpLink(l, os);
> > +			}
> > +			os << "\n";
> > +		}
> > +
> > +		for (auto const &p : e->sources()) {
> > +			os << "  [" << p->index() << "]" << ": Source\n";
> > +			for (auto const &l : p->links()) {
> > +				dumpLocal(e, p, os);
> > +				os << " -> ";
> > +				dumpRemote(l, os);
> > +				dumpLink(l, os);
> > +			}
> > +			os << "\n";
> > +		}
> > +	}
> > +}
>
> I wonder whether dumpGraph() belongs here, or whether it should be moved to a
> test. MediaDevice isn't exposed through the public API, how do you foresee
> dumpGraph being used ? If it can never be called from the public API there's
> no point in having it in the library :-)
>

You are right. This was my validation test to make sure things where
working properly, so it's probably better to make a test out of this.

> > +/**
> > + * \fn MediaDevice::setupLink(MediaPad *source, MediaPad *sink)
>
> you're missing const (assuming we need to list the parameters explicitly here,
> see the comment above).
>
> > + * \brief Apply \a flags to the link between \a source and \a sink pads
> > + * \param source The source MediaPad
> > + * \param sink The sink MediaPad
> > + * \param link The MediaLink to operate on
>
> Why do you need to specify both source and sink, and the link ? The link
> should identify the source and sink already. If you don't expect the caller to
> have the link pointer you should look it up internally, otherwise you
> shouldn't pass the source and sink. And if you expect the caller to have the
> link pointer, maybe you could make this function a member of the MediaLink
> class ?

I'll consider making something like links->setup()

>
> > + * \param flags Flags to be applied to the link (MEDIA_LNK_FL_*)
> > + */
> > +int MediaDevice::setupLink(const MediaPad *source, const MediaPad *sink,
> > +			   MediaLink *link, unsigned int flags)
> > +{
> > +	struct media_link_desc linkDesc = { };
> > +
> > +	linkDesc.source.entity = source->entity();
> > +	linkDesc.source.index = source->index();
> > +	linkDesc.source.flags = MEDIA_PAD_FL_SOURCE;
> > +
> > +	linkDesc.sink.entity = sink->entity();
> > +	linkDesc.sink.index = sink->index();
> > +	linkDesc.sink.flags = MEDIA_PAD_FL_SINK;
> > +
> > +	linkDesc.flags = flags;
> > +
> > +	if (ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc)) {
> > +		LOG(Error) << "Failed to setup link: "
> > +			   << strerror(errno);
> > +		return -errno;
> > +	}
> > +
> > +	link->setFlags(0);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \fn MediaDevice::resetLinks()
> > + * \brief Reset all links on the media graph
>
> Reset to what ? :-)

Ok... "Clear the ENABLE flags" ? "Reset a link to the non-enabled
state?"
>
> > + *
> > + * Walk all registered entities, and disable all links from their
> > + * source pads to other pads.
>
> Wouldn't it be more efficient to walk links instead of entities and pads ?
>

Entities have pads
pads have links

I could create a global list of links in the media device, but if it's
just for this, I don't think it's worth.

> Could you expand the documentation to explain the use cases for this function
> ? Please also document the return value (this holds true for all other
> functions returning a value in the whole series).
>

What do you mean by use cases? Before operating on a media graph, or
whenever the pipeline handlers wants to reset enabled but not
immutable links to the non-enabled state, they have a utlity function
to do that. Where and how to use it it's up to the pipeline handler,
even if it were for me, that should happen before doing any operation
on the media graph.

Ah, you possibly meant what I just wrote? Damn documentation...

> > + */
> > +int MediaDevice::resetLinks()
> > +{
> > +	for (MediaEntity *e : entities_) {
> > +		for (MediaPad *sourcePad : e->sources()) {
> > +			for (MediaLink *l : sourcePad->links()) {
> > +				/*
> > +				 * Do not reset links that are not enabled
> > +				 * or immutable.
> > +				 */
> > +				if (l->flags() & MEDIA_LNK_FL_IMMUTABLE)
> > +					continue;
> > +
> > +				if (!(l->flags() & MEDIA_LNK_FL_ENABLED))
> > +					continue;
> > +
> > +				/* Get the remote sink pad. */
> > +				MediaPad *sinkPad = dynamic_cast<MediaPad *>
> > +						    (getObject(l->sink()));
> > +				if (!sinkPad)
> > +					return -ENOENT;
> > +
> > +				/* Also get entity to make sure IDs are ok. */
> > +				MediaEntity *sinkEntity =
> > +						dynamic_cast<MediaEntity *>
> > +						(getObject(sinkPad->entity()));
> > +				if (!sinkEntity)
> > +					return -ENOENT;
> > +
> > +				int ret = setupLink(sourcePad, sinkPad, l, 0);
> > +				if (ret) {
> > +					LOG(Error) << "Link reset failed: "
> > +						   << e->name() << "["
> > +						   << sourcePad->index()
> > +						   << "] -> "
> > +						   << sinkEntity->name() << "["
> > +						   << sinkPad->index() << "]";
> > +					return ret;
> > +				}
> > +
> > +				LOG(Info) << "Link reset: "
> > +					  << e->name() << "["
> > +					  << sourcePad->index()
> > +					  << "] -> "
> > +					  << sinkEntity->name() << "["
> > +					  << sinkPad->index() << "]";
> > +			}
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \fn MediaDevice::link(std::string, unsigned int, std::string, unsigned
> > int)
> > + * \brief Setup a link identified by the entities name and their source and
> > + * sink pad indexes
> > + * \param source The source entity name
> > + * \param sourceIdx The source pad index
> > + * \param sink The sink entity name
> > + * \param sinkIdx The sink pad index
> > + * \param flags The link setup flag (see MEDIA_LNK_FL_*)
>
> If I understand the purpose of this function properly (and the fact that I'm
> not sure I do means the documentation should be improved :-)), it serves the
> same purpose as MediaDevice::setupLink(), but with different arguments. I
> would thus name the two functions setupLink(), as C++ allows overloading
> functions.

No, they're different. setupLink() wants pads and a link where to
operate on, as is private, designed to back up this function which is
instead public and wants entities names and pad ids. I agree I should
look if I could make setupLink a link operation and how ita would look
like.

>
> I would go one step further though, at least if you agree with my proposition
> to move setupLink to MediaLink, and turn this function into a link lookup. The
> callers would then do something similar to
>
> 	MediaDevice *dev = ...;
> 	MediaLink *link = dev->link("source", 0, "sink", 3);
                                                        ^- why three?

Can links be set as immutable by userspace, or do they get created
immutable from the driver only?

> 	if (!link)
> 		return -ENODEV;
> 	link->setup(flags);
>
> and possibly cache the link pointer internally to avoid looking up links all
> the time.

Here I don't agree actually. I think there should be a MediaDevice
provided "link()" operation that might use the MediaLink "setupLink"
but from the caller, nothing should be seen that it's not the
MediaDevice itself. From the caller I would just like to see

        ret = mediaDev->link("source", 0, "sink", 1, 1);

There is no reason (as I immagined this) for the caller to deal with
links, and pads itself. Entities are an exception maybe, as we might
need to configure format and controls on their subdevices, but that's
it.

Don't you think this provides a better isolation?

>
> > + */
> > +int MediaDevice::link(const std::string &source, unsigned int sourceIdx,
> > +		      const std::string &sink, unsigned int sinkIdx,
>
> At the cost of longer names, sourcePadIndex and sinkPadIndex ?
>
> > +		      unsigned int flags)
> > +{
> > +
>
> Stray blank line.
>
> > +	/* Make sure the supplied link is well formed. */
> > +	MediaEntity *sourceEntity = getEntityByName(source);
> > +	if (!sourceEntity) {
> > +		LOG(Error) << "Entity name: " << source << "not found";
> > +		return -ENOENT;
> > +	}
> > +
> > +	MediaEntity *sinkEntity = getEntityByName(sink);
> > +	if (!sinkEntity) {
> > +		LOG(Error) << "Entity name: " << source << "not found";
>
> s/source/sink/
> s/Entity name:/Entity/
>
> > +		return -ENOENT;
> > +	}
> > +
> > +	const MediaPad *sourcePad = sourceEntity->getPadByIndex(sourceIdx);
> > +	if (!sourcePad) {
> > +		LOG(Error) << "Pad " << sourceIdx << "not found in entity "
> > +			   << sourceEntity->name();
> > +		return -ENOENT;
> > +	}
> > +
> > +	const MediaPad *sinkPad = sinkEntity->getPadByIndex(sinkIdx);
> > +	if (!sinkPad) {
> > +		LOG(Error) << "Pad " << sinkIdx << "not found in entity "
> > +			   << sinkEntity->name();
> > +		return -ENOENT;
> > +	}
> > +
> > +	/*
> > +	 * Walk all links in the source and search for an entry matching the
> > +	 * pad ids. If none, the requested link does not exists.
> > +	 */
> > +	MediaLink *validLink = nullptr;
> > +	for (MediaLink *link : sourcePad->links()) {
> > +		if (link->source() != sourcePad->id())
> > +			continue;
> > +
> > +		if (link->sink() != sinkPad->id())
> > +			continue;
> > +
> > +		validLink = link;
> > +		break;
> > +	}
> > +
> > +	if (!validLink) {
> > +		LOG(Error) << "Link not found"
> > +			   << "\"" << sourceEntity->name() << "\"["
> > +			   << sourcePad->index() << "] -> "
> > +			   << "\"" << sinkEntity->name() << "\"["
> > +			   << sinkPad->index() << "]";
> > +		return -EINVAL;
> > +	}
> > +
> > +	int ret = setupLink(sourcePad, sinkPad, validLink, flags);
> > +	if (ret)
> > +		return ret;
> > +
> > +	LOG(Info) << "Setup link: "
>
> LOG(Debug) at most.
>
> > +		  << "\"" << sourceEntity->name() << "\"["
> > +		  << sourcePad->index() << "] -> "
> > +		  << "\"" << sinkEntity->name() << "\"["
> > +		  << sinkPad->index() << "]"
> > +		  << " [" << flags << "]";
> > +
> > +	return 0;
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index da06eba..4cac687 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -2,6 +2,7 @@ libcamera_sources = files([
> >      'log.cpp',
> >      'main.cpp',
> >      'media_object.cpp',
> > +    'media_device.cpp',
>
> sort
>
> >  ])
> >
> >  libcamera_headers = files([


Thanks
  j

>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Hi Jacopo,

On Friday, 28 December 2018 16:36:52 EET Jacopo Mondi wrote:
> On Fri, Dec 28, 2018 at 03:27:21PM +0200, Laurent Pinchart wrote:
> > On Friday, 28 December 2018 09:57:41 EET Jacopo Mondi wrote:
> > > The MediaDevice object implements handling and configuration of the
> > > media graph associated with a V4L2 media device.
> > 
> > Let's not talk about V4L2 here, it's just MC, not V4L2.
> > 
> > > The class allows enumeration of all pads, links and entities registered
> > > in the media graph, and provides methods to setup and reset media links.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > 
> > >  src/libcamera/include/media_device.h |  71 ++++
> > >  src/libcamera/media_device.cpp       | 596 +++++++++++++++++++++++++++
> > >  src/libcamera/meson.build            |   1 +
> > >  3 files changed, 668 insertions(+)
> > >  create mode 100644 src/libcamera/include/media_device.h
> > >  create mode 100644 src/libcamera/media_device.cpp
> > > 
> > > diff --git a/src/libcamera/include/media_device.h
> > > b/src/libcamera/include/media_device.h new file mode 100644
> > > index 0000000..642eea9
> > > --- /dev/null
> > > +++ b/src/libcamera/include/media_device.h
> > > @@ -0,0 +1,71 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2018, Google Inc.
> > > + *
> > > + * media_device.h - Media device handler
> > > + */
> > > +#ifndef __LIBCAMERA_MEDIA_DEVICE_H__
> > > +#define __LIBCAMERA_MEDIA_DEVICE_H__
> > > +
> > > +#include <map>
> > > +#include <string>
> > > +#include <sstream>
> > 
> > sort
> > 
> > > +#include <vector>
> > > +
> > > +#include <linux/media.h>
> > > +
> > > +#include "log.h"
> > 
> > Is this needed ?
> > 
> > > +#include "media_object.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +class MediaDevice
> > > +{
> > > +public:
> > > +	MediaDevice() : fd_(-1) { };
> > > +	~MediaDevice();
> > > +
> > > +	const std::string name() const { return name_; }
> > > +	const std::string path() const { return path_; }
> > > +
> > > +	int open(const std::string &path);
> > > +	int close();
> > > +	int enumerate(std::map<std::string, std::string> &entitiesMap);
> > > +	void dumpGraph(std::ostream &os);
> > > +
> > > +	int resetLinks();
> > > +	int link(const std::string &source, unsigned int sourceIdx,
> > > +		 const std::string &sink, unsigned int sinkIdx,
> > > +		 unsigned int flags);
> > > +
> > > +private:
> > > +	/** The media device file descriptor */
> > 
> > Wrong file ?
> 
> Sorry, I didn't get this.

I meant that the doxygen comments are placed in the .cpp file.

> > > +	int fd_;
> > > +	/** The media device name as returned by MEDIA_IOC_DEVICE_INFO */
> > 
> > Please take my comments on 1/4 into account regarding wording of
> > documentation here.
> 
> I'll drop documentation from private members
> 
> > This field stores the driver name, I would thus name it driver_ (or
> > possibly driverName_).
> > 
> > > +	std::string name_;
> > > +	/** The media device path */
> > > +	std::string path_;
> > > +
> > > +	std::map<unsigned int, MediaObject *> mediaObjects_;
> > 
> > s/mediaObjects_/objects_/ ?
> 
> Actually I don't agree, here and all other places in the patch where
> an s/mediaSomething/something/ has been suggested.
> 
> This class deals both with struct_v2_something and MediaSomething. I
> would like to keep clear when we're operating on the object
> representation and when on the kernel provided structure.

I understand, and I agree that there's a naming issue when both types of 
objects are needed. Given that the kernel structures should all be 
encapsulated here and the Media* objects used everywhere else, I would go for 
shorter names for Media* (e.g. MediaPad *pad) and longer names for the kernel 
structures.

> > > +	MediaObject *getObject(unsigned int id);
> > > +	void addObject(MediaObject *obj);
> > > +	void deleteObjects();
> > > +
> > > +	std::vector<MediaEntity *> entities_;
> > > +	MediaEntity *getEntityByName(const std::string &name);
> > > +
> > > +	int enumerateEntities(std::map<std::string, std::string> 
&entitiesMap,
> > > +			      struct media_v2_topology &topology);
> > > +	int enumeratePads(struct media_v2_topology &topology);
> > > +	int enumerateLinks(struct media_v2_topology &topology);
> > > +
> > > +	int setupLink(const MediaPad *source, const MediaPad *sink,
> > > +		      MediaLink *link, unsigned int flags);
> > > +
> > > +	void dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os);
> > > +	void dumpRemote(MediaLink *l, std::ostream &os);
> > > +	void dumpLink(MediaLink *l, std::ostream &os);
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +#endif /* __LIBCAMERA_MEDIA_DEVICE_H__ */
> > > diff --git a/src/libcamera/media_device.cpp
> > > b/src/libcamera/media_device.cpp new file mode 100644
> > > index 0000000..b98d62f
> > > --- /dev/null
> > > +++ b/src/libcamera/media_device.cpp
> > > @@ -0,0 +1,596 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2018, Google Inc.
> > > + *
> > > + * media_device.cpp - Media device handler
> > > + */
> > > +
> > > +#include <errno.h>
> > > +#include <fcntl.h>
> > > +#include <string.h>
> > > +#include <sys/ioctl.h>
> > > +#include <unistd.h>
> > > +
> > > +#include <string>
> > > +#include <vector>
> > > +
> > > +#include <linux/media.h>
> > > +
> > > +#include "log.h"
> > > +#include "media_device.h"
> > > +
> > > +/**
> > > + * \file media_device.h
> > > + */
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \class MediaDevice
> > > + * \brief Media device handler
> > > + *
> > > + * MediaDevice handles the media graph associated with a V4L2 media
> > > device. + */
> > > +
> > > +/**
> > > + * \fn MediaDevice::~MediaDevice()
> > > + * \brief Close the media device file descriptor and release entities
> > > + */
> > > +MediaDevice::~MediaDevice()
> > > +{
> > > +	if (fd_ > -1)
> > 
> > I would have written != -1.
> 
> Any reason why it's better?

Probably not :-) We should never have negative values other than -1.

> > > +		::close(fd_);
> > > +	deleteObjects();
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaDevice::name()
> > > + * \brief Return the media device name
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaDevice::path()
> > > + * \brief Return the media device path node associated with this
> > > MediaDevice
> > 
> > I'd name this devnode() instead of path().
> 
> I already though about chnging this, right...
> 
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaDevice::deleteObjects()
> > > + * \brief Delete all registered entities in the MediaDevice object
> > > + */
> > > +void MediaDevice::deleteObjects()
> > > +{
> > > +	for (auto const &e : mediaObjects_)
> > > +		delete e.second;
> > > +
> > > +	mediaObjects_.clear();
> > > +	entities_.clear();
> > > +}
> > > +
> > > +/**
> > > + * \fn int MediaDevice::open(std::string)
> > 
> > Do you need to specify the arguments when there's a single function by
> > that name ? If so, shouldn't it be (const std::string &) ?
> > 
> > Same comment for other functions in this patch series.
> 
> I think I could drop the function name completely when the comment
> block is just before the function definition.
> 
> > > + * \brief Open a media device and initialize its components.
> > > + * \param path The media device path
> > > + */
> > > +int MediaDevice::open(const std::string &path)
> > 
> > s/path/devnode/ ?
> > 
> > > +{
> > 
> > What if the device is already open ? It would be useful to add some
> > documentation to explain the "life cycle" of the object, as in how it is
> > supposed to be used (and what happens when it's incorrectly used). This
> > can be done in the open() documentation (and referenced from close() and
> > possibly other functions), or in the \class documentation.
> 
> Right, currently there's no protection for that in this
> implementation. I should return an error if (fd != -1).
> 
> I could just state the caller is responsible for opening and closing
> the device and that access is exclusive. In the future, this might be
> superseded by ref-counted acquire/release operations, with open() and
> close() then made private.

As long as you document the expected behaviour, that's fine with me.

> > > +	fd_ = ::open(path.c_str(), O_RDWR);
> > > +	if (fd_ < 0) {
> > > +		LOG(Error) << "Failed to open media device at " << path
> > > +			   << ": " << strerror(errno);
> > > +		return -errno;
> > 
> > The LOG() call may overwrite errno. You will need a local variable. How
> > about> 
> > 	int ret;
> > 	
> > 	ret = ::open(path.c_str(), O_RDWR);
> > 	if (ret < 0) {
> > 		ret = -errno;
> > 		LOG(Error) << "Failed to open media device at " << path
> > 			   << ": " << strerror(-ret);
> > 		return ret;
> > 	}
> > 	
> > 	fd_ = ret;
> > 	path_ = path;
> 
> Right. Thanks, this is important, I would like to see all error
> handling standardized in the library.

So would I. I think the above construct could be used through the library.

> > > +	}
> > > +	path_ = path;
> > > +
> > > +	struct media_device_info info = { };
> > > +	int ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
> > > +	if (ret) {
> > > +		LOG(Error) << "Failed to get media device info "
> > > +			   << ": " << strerror(errno);
> > > +		return -errno;
> > > +	}
> > > +
> > > +	name_ = info.driver;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaDevice::close()
> > > + * \brief Close the file descriptor associated with the media device
> > > + */
> > > +int MediaDevice::close()
> > > +{
> > > +	if (fd_ > -1)
> > 
> > I'd write this != -1 too.
> > 
> > > +		return ::close(fd_);
> > > +
> > > +	return 0;
> > 
> > Do you use the return value ?
> > 
> > > +}
> > > +
> > > +void MediaDevice::addObject(MediaObject *obj)
> > > +{
> > > +
> > > +	if (mediaObjects_.find(obj->id()) != mediaObjects_.end()) {
> > > +		LOG(Error) << "Element with id " << obj->id()
> > > +			   << " already enumerated.";
> > > +		return;
> > 
> > Shouldn't you propagate the error to the caller ? This seems pretty fatal
> > to me.
> 
> This should not happen of course. In case it does, I'll return
> -EEXIST.
> 
> > > +	}
> > > +
> > > +	mediaObjects_[obj->id()] = obj;
> > > +}
> > > +
> > > +MediaObject *MediaDevice::getObject(unsigned int id)
> > > +{
> > > +	auto it = mediaObjects_.find(id);
> > > +	return (it == mediaObjects_.end()) ?
> > > +	       nullptr : it->second;
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaDevice::getEntityByName(std::string)
> > > + * \brief Return entity with name \a name
> > > + * \param name The entity name
> > 
> > Please expand documentation a little bit. For instance you should explain
> > that this function returns nullptr if the entity can't be found. In
> > general a one- line documentation that just duplicates the function name
> > isn't very useful, the real value comes from what isn't expressed in the
> > name.
> 
> I still feel that a function named getEntityByName() is pretty clear
> without having to document it in detail. The return value though might
> be pointed out, even if, it's exactly what one would expect, the
> entity or nullptr.
> 
> > Documentation is hard :-)
> 
> Drawing the line between useful documentation and writing comments to
> silence doxygen "not documented" warning is. I clearly went for the second
> option and documented everything doxygen complained about but that's
> clearly not useful if not for ticking a box. I would be happy to chop
> documentation parts here and there.

TODO: Check whether we could silence warnings in a better way when no 
documentation is needed.

> > > + */
> > > +MediaEntity *MediaDevice::getEntityByName(const std::string &name)
> > > +{
> > > +	auto it = entities_.begin();
> > > +
> > > +	while (it != entities_.end()) {
> > > +		MediaEntity *e = *it;
> > > +		if (!(e->name().compare(name)))
> > > +			return e;
> > > +		it++;
> > > +	}
> > > +
> > > +	return nullptr;
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaDevice::enumerateLinks(struct media_v2_topology &topology)
> > > + * \brief Enumerate all links in the system and associate them with
> > > their
> > > + * source and sink pads
> > > + * \param topology The media topology as returned by
> > > MEDIA_IOC_G_TOPOLOGY
> > > + */
> > > +int MediaDevice::enumerateLinks(struct media_v2_topology &topology)
> > 
> > Shouldn't the argument be const ? I would have used a pointer instead of a
> > reference, but I'm not sure why.
> > 
> > > +{
> > > +	struct media_v2_link *link = reinterpret_cast<struct media_v2_link 
*>
> > > +				     (topology.ptr_links);
> > > +
> > > +	for (unsigned int i = 0; i < topology.num_links; i++, link++) {
> > > +		/*
> > > +		 * Skip links between entities and interfaces: we only care
> > > +		 * about pad-2-pad links here.
> > > +		 */
> > > +		if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> > > +		    MEDIA_LNK_FL_INTERFACE_LINK)
> > > +			continue;
> > > +
> > > +		MediaLink *mediaLink = new MediaLink(link);
> > 
> > s/mediaLink/link/ ?
> > 
> > You would need to rename the link variable above though, I'm not sure
> > which one is best.
> 
> That's what I meant. link for media_v2_link, and mediaLink for
> MediaLink. It makes sense to me.

I would have gone for the opposite, as explained above, but that's OK too I 
suppose. We should however go for shorter names (no media prefix) in the API 
and in the code using those classes. The media prefix, if used for the Media* 
classes, should only appear in the media_object.cpp and media_device.cpp 
files.

> > > +		addObject(mediaLink);
> > > +
> > > +		/* Store reference to this mediaLink in the link's source pad. */
> > > +		MediaPad *mediaPad = dynamic_cast<MediaPad *>
> > > +				     (getObject(mediaLink->source()));
> > 
> > s/mediaPad/pad/ ?
> 
> ditto
> 
> > > +		if (!mediaPad) {
> > > +			LOG(Error) << "Failed to find pad with id: "
> > 
> > Nitpicking, s/://.
> > 
> > > +				   << mediaLink->source();
> > > +			return -ENODEV;
> > > +		}
> > > +		mediaPad->addLink(mediaLink);
> > > +
> > > +		/* Store reference to this mediaLink in the link's sink pad. */
> > > +		mediaPad = dynamic_cast<MediaPad *>(getObject(mediaLink-
>sink()));
> > > +		if (!mediaPad) {
> > > +			LOG(Error) << "Failed to find pad with id: "
> > > +				   << mediaLink->sink();
> > > +			return -ENODEV;
> > > +		}
> > > +		mediaPad->addLink(mediaLink);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaDevice::enumeratePads(struct media_v2_topology &topology)
> > > + * \brief Enumerate all pads in the system and associate them with the
> > > + * entity they belong to
> > > + * \param topology The media topology as returned by
> > > MEDIA_IOC_G_TOPOLOGY
> > > + */
> > > +int MediaDevice::enumeratePads(struct media_v2_topology &topology)
> > > +{
> > > +	struct media_v2_pad *pad = reinterpret_cast<struct media_v2_pad *>
> > > +				   (topology.ptr_pads);
> > > +
> > > +	for (unsigned int i = 0; i < topology.num_pads; i++, pad++) {
> > > +		MediaPad *mediaPad = new MediaPad(pad);
> > > +		addObject(mediaPad);
> > > +
> > > +		/* Store a reference to this MediaPad in pad's entity. */
> > > +		MediaEntity *mediaEntity = dynamic_cast<MediaEntity *>
> > > +					   (getObject(mediaPad->entity()));
> > 
> > s/mediaPad/pad/ and s/mediaEntity/entity/ ?
> 
> ditto ditto ;)
> 
> > > +		if (!mediaEntity) {
> > > +			LOG(Error) << "Failed to find entity with id: "
> > > +				   << mediaPad->entity();
> > > +			return -ENODEV;
> > > +		}
> > > +
> > > +		mediaEntity->addPad(mediaPad);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaDevice::enumerateEntities(std::map<std::string,
> > > std::string> &, + *                                    struct
> > > media_v2_topology &topology) + * \brief Enumerate and initialize
> > > entities in the media graph
> > > + * \param entitiesMap Map entities names to their video (sub)device
> > > node
> > > + * \param topology The media topology as returned by
> > > MEDIA_IOC_G_TOPOLOGY
> > > + *
> > > + * Associate the video (sub)device path to the entity name as returned
> > > by
> > 
> > As stated before, I'd talk about device nodes, as this isn't
> > V4L2-specific.
> > 
> > > + * MEDIA_IOC_G_TOPOLOGY
> > > + */
> > > +int MediaDevice::enumerateEntities(std::map<std::string, std::string>
> > > &entitiesMap,
> > > +				   struct media_v2_topology &topology)
> > > +{
> > > +	struct media_v2_entity *entities = reinterpret_cast<struct
> > > media_v2_entity *>
> > > +					   (topology.ptr_entities);
> > > +
> > > +	for (unsigned int i = 0; i < topology.num_entities; ++i) {
> > > +		auto it = entitiesMap.find(entities[i].name);
> > > +		if (it == entitiesMap.end()) {
> > > +			LOG(Error) << "Entity " << entities[i].name
> > > +				   << " not found in media entities map";
> > > +			return -ENOENT;
> > > +		}
> > > +
> > > +		MediaEntity *entity = new MediaEntity(&entities[i]);
> > > +		if (entity->setDevice(it->second)) {
> > > +			delete entity;
> > > +			goto delete_entities;
> > 
> > You could call deleteObjects() and return here directly.
> > 
> > > +		}
> > > +
> > > +		addObject(entity);
> > > +		entities_.push_back(entity);
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +delete_entities:
> > > +	deleteObjects();
> > > +
> > > +	return -errno;
> > 
> > -errno isn't correct. You should assign the return value of setDevice() to
> > a ret variable and return ret.
> > 
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaDevice::enumerate(std::map<std::string, std::string>)
> > > + * \brief Enumerate the media graph topology
> > > + * \param entitiesMap Map entities names to their video (sub)device
> > > node
> > > + * FIXME: this is statically provided by the caller at the moment.
> > > + *
> > > + * This functions enumerates all media objects, registered in the media
> > > graph,
> > 
> > s/functions/function/
> > s/,//g
> > 
> > > + * through the MEDIA_IOC_G_TOPOLOGY ioctl. For each returned entity,
> > > + * it creates and store its representation for later reuse.
> > 
> > Let's expand this a little.
> > 
> > "This function enumerates all media graph objects in the media device and
> > populates the internal list of objects. All entities, pads and links are
> > stored as MediaEntity, MediaPad and MediaLink respectively, with cross-
> > references between objects. Media interfaces are not processed.
> > 
> > For entities paired with a device node, the path to the device node is
> > stored in the MediaEntity object, based on the associations set in the \a
> > entitiesMap parameter.
> > 
> > \return Return 0 on success or a negative error code on error."
> > 
> > > + */
> > > +int MediaDevice::enumerate(std::map<std::string, std::string>
> > > &entitiesMap) +{
> > > +	struct media_v2_topology topology = { };
> > > +	unsigned int num_interfaces;
> > > +	unsigned int num_links;
> > > +	unsigned int num_pads;
> > > +	unsigned int num_ent;
> > > +
> > > +	do {
> > > +		num_ent = topology.num_entities;
> > > +		num_pads = topology.num_pads;
> > > +		num_links = topology.num_links;
> > > +		num_interfaces = topology.num_interfaces;
> > > +
> > > +		/* Call G_TOPOLOGY the first time here to enumerate .*/
> > > +		if (ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology)) {
> > > +			LOG(Error) << "Failed to enumerate media topology on"
> > > +				   << path_ << ": " << strerror(errno);
> > > +			return -errno;
> > > +		}
> > > +
> > > +		/*
> > > +		 * Repeat the call until we don't get a 'stable' number
> > > +		 * of media objects.
> > > +		 */
> > > +	} while (num_ent != topology.num_entities ||
> > > +		 num_pads != topology.num_pads    ||
> > > +		 num_links != topology.num_links  ||
> > > +		 num_interfaces != topology.num_interfaces);
> > 
> > That's not very useful, as the need to ensure that the topology doesn't
> > change stems from the fact we call MEDIA_IOC_G_TOPOLOGY (at least) twice,
> > once to retrieve the number of entities, and a second time to retrieve
> > the graph itself. The graph could be updated in the kernel after this
> > loop and before the next MEDIA_IOC_G_TOPOLOGY.
> > 
> > I proposed a way to call MEDIA_IOC_G_TOPOLOGY from a single location in a
> > previous e-mail. We can fix this on top of this series, or:
> > 
> > 	struct media_v2_topology topology = { };
> > 	struct media_v2_entity *entities;
> > 	struct media_v2_pad *pads;
> > 	struct media_v2_link *links;
> > 	struct media_v2_interface *interfaces;
> > 	unsigned int num_entities;
> > 	unsigned int num_pads;
> > 	unsigned int num_links;
> > 	unsigned int num_interfaces;
> > 	int ret;
> > 	
> > 	while (1) {
> > 		num_entitites = topology.num_entities;
> > 		num_pads = topology.num_pads;
> > 		num_links = topology.num_links;
> > 		num_interfaces = topology.num_interfaces;
> > 		
> > 		pads = new media_v2_pad[topology.num_pads];
> > 		entities = new media_v2_entity[topology.num_entities];
> > 		topology.ptr_entities = reinterpret_cast<__u64>(entities);
> > 		
> > 		topology.ptr_pads = reinterpret_cast<__u64>(pads);
> > 		
> > 		links = new media_v2_link[topology.num_links];
> > 		topology.ptr_links = reinterpret_cast<__u64>(links);
> > 		links = new media_v2_interface[topology.num_interfaces];
> > 		topology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);
> > 		
> > 		ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> > 		if (ret < 0) {
> > 			ret = -errno;
> > 			LOG(Error) << "Failed to enumerate media topology on"
> > 				   << path_ << ": " << strerror(-ret);
> > 			goto done;
> > 		}
> > 		
> > 		/*
> > 		 * If all objects have been enumerated, stop now. If the number
> > 		 * of objects has increased (after the first call or due to a
> > 		 * topology change, retry the enumeration.
> > 		 */
> > 		if (num_entities >= topology.num_entities &&
> > 		    num_pads >= topology.num_pads &&
> > 		    num_links >= topology.num_links &&
> > 		    num_interfaces >= topology.num_interfaces)
> > 			break;
> > 	}
> 
> According to documentation (and Niklas' implementation I have copied,
> checking that topology->version does not change should be enough. Do
> you agree?

Yes, that would work too. My implementation avoids an extra iteration in case 
the topology changes but the number of objects doesn't increase, but that's 
really a micro-optimization that will likely never matter in practice.

> By the way, I appreciate the concern regarding dynamic media graphs,
> but we're here checking that media graph does not change in between
> two ioctl calls, which is quite a strict race window, while it could
> change at any other point in time after this call, and we won't
> notice. I wonder if this is worth, but since I have code from you a
> Niklas, I'll simply copy it in.

You're right that other changes will be needed, but as you've pointed out, the 
code exists, so let's use it.

> > 	ret = enumerateEntities(entitiesMap, topology);
> > 	if (ret)
> > 		goto done;
> > 	
> > 	ret = enumeratePads(topology);
> > 	if (ret)
> > 		goto done;
> > 	
> > 	ret = enumerateLinks(topology);
> > 	if (ret)
> > 		goto done;
> > 
> > done:
> > 	if (ret < 0)
> > 		deleteObjects();
> > 	
> > 	delete[] entities;
> > 	delete[] links;
> > 	delete[] pads;
> > 	delete[] interfaces;
> > 	
> > 	return ret;
> > 
> > (untested)
> > 
> > I wonder if enumerateEntities, enumeratePads and enumerateLinks should be
> > renamed as s/enumerate/populate/.
> > 
> > > +	auto *_ptr_e = new media_v2_entity[topology.num_entities];
> > > +	topology.ptr_entities = reinterpret_cast<__u64>(_ptr_e);
> > > +
> > > +	auto *_ptr_p = new media_v2_pad[topology.num_pads];
> > > +	topology.ptr_pads = reinterpret_cast<__u64>(_ptr_p);
> > > +
> > > +	auto *_ptr_l = new media_v2_link[topology.num_links];
> > > +	topology.ptr_links = reinterpret_cast<__u64>(_ptr_l);
> > > +
> > > +	/* Call G_TOPOLOGY again, this time with memory reserved. */
> > > +	int ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> > > +	if (ret < 0) {
> > > +		LOG(Error) << "Failed to enumerate media topology on " << path_
> > > +			   << ": " << strerror(errno);
> > > +		ret = -errno;
> > > +		goto error_free_mem;
> > > +	}
> > > +
> > > +	ret = enumerateEntities(entitiesMap, topology);
> > > +	if (ret)
> > > +		goto error_free_mem;
> > > +
> > > +	ret = enumeratePads(topology);
> > > +	if (ret)
> > > +		goto error_free_objs;
> > > +
> > > +	ret = enumerateLinks(topology);
> > > +	if (ret)
> > > +		goto error_free_objs;
> > > +
> > > +	delete[] _ptr_e;
> > > +	delete[] _ptr_p;
> > > +	delete[] _ptr_l;
> > > +
> > > +	return 0;
> > > +
> > > +error_free_objs:
> > > +	deleteObjects();
> > > +
> > > +error_free_mem:
> > > +	delete[] _ptr_e;
> > > +	delete[] _ptr_p;
> > > +	delete[] _ptr_l;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +void MediaDevice::dumpLocal(MediaEntity *e, MediaPad *p, std::ostream
> > > &os)
> > 
> > s/p/pad/
> > 
> > Unless names get really long, let's try to spell them out, as it increases
> > readability.
> > 
> > > +{
> > > +	os << "\t  \"" << e->name() << "\"["
> > > +	   << p->index() << "]";
> > > +}
> > > +
> > > +void MediaDevice::dumpRemote(MediaLink *l, std::ostream &os)
> > 
> > s/l/link/
> > 
> > > +{
> > > +
> > 
> > Stray blank line.
> > 
> > > +	MediaPad *remotePad = dynamic_cast<MediaPad *>
> > > +			      (getObject(l->sink()));
> > 
> > Would it make sense for the sink() and source() methods to return pointers
> > to MediaPad instead of an id ? I would in that case store the pointers
> > internally in MediaLink, to avoid a call to getObject() every time.
> 
> That calls for initializing two pointers at MediaLink creation time.
> The same could happen for MediaPads, instead of storing their entity
> ID they could store a pointer to the MediaEntity... I'll see how it
> might look.

Initialization will be a bit more complex, but usage should be simpler.

> > > +	if (!remotePad)
> > > +		return;
> > 
> > This check wouldn't be needed then.
> > 
> > > +
> > > +	MediaEntity *remoteEntity = dynamic_cast<MediaEntity *>
> > > +				    (getObject(remotePad->entity()));
> > > +	if (!remoteEntity)
> > > +		return;
> > 
> > Same here, returning a MediaEntity pointer, and removing the check.
> > 
> > > +	os << "\"" << remoteEntity->name() << "\"["
> > > +	   << remotePad->index() << "]";
> > > +}
> > > +
> > > +void MediaDevice::dumpLink(MediaLink *l, std::ostream &os)
> > > +{
> > > +	unsigned int flags = l->flags();
> > > +
> > > +	os << " [";
> > > +	if (flags) {
> > > +		os << (flags & MEDIA_LNK_FL_ENABLED ? "ENABLED," : "")
> > > +		   << (flags & MEDIA_LNK_FL_IMMUTABLE ? "IMMUTABLE" : "");
> > > +	}
> > > +	os  << "]\n";
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaDevice::dumpGraph(std::ostream)
> > > + * \brief Dump the media device topology in textual form to an output
> > > stream
> > > + * \param os The output stream where to append the printed topology to
> > 
> > s/append/write/ ?
> 
> Append is more appropriate, as the stream could already have content.
> 
> > > + */
> > > +void MediaDevice::dumpGraph(std::ostream &os)
> > > +{
> > > +	os << "\n" << name_ << " - " << path_ << "\n\n";
> > > +
> > > +	for (auto const &e : entities_) {
> > 
> > s/auto const &e/const struct MediaEntity */
> > 
> > Let's restrict the use of auto to cases where it would be impractical to
> > write the type out explicitly, as auto reduces readability and removes
> > compile-time checks.
> 
> Agreed
> 
> > > +		os << "\"" << e->name() << "\"\n";
> > > +
> > > +		for (auto const &p : e->sinks()) {
> > > +			os << "  [" << p->index() << "]" << ": Sink\n";
> > > +			for (auto const &l : p->links()) {
> > > +				dumpLocal(e, p, os);
> > > +				os << " <- ";
> > > +				dumpRemote(l, os);
> > > +				dumpLink(l, os);
> > > +			}
> > > +			os << "\n";
> > > +		}
> > > +
> > > +		for (auto const &p : e->sources()) {
> > > +			os << "  [" << p->index() << "]" << ": Source\n";
> > > +			for (auto const &l : p->links()) {
> > > +				dumpLocal(e, p, os);
> > > +				os << " -> ";
> > > +				dumpRemote(l, os);
> > > +				dumpLink(l, os);
> > > +			}
> > > +			os << "\n";
> > > +		}
> > > +	}
> > > +}
> > 
> > I wonder whether dumpGraph() belongs here, or whether it should be moved
> > to a test. MediaDevice isn't exposed through the public API, how do you
> > foresee dumpGraph being used ? If it can never be called from the public
> > API there's no point in having it in the library :-)
> 
> You are right. This was my validation test to make sure things where
> working properly, so it's probably better to make a test out of this.

And it would be a test producing nicely looking results :-)

> > > +/**
> > > + * \fn MediaDevice::setupLink(MediaPad *source, MediaPad *sink)
> > 
> > you're missing const (assuming we need to list the parameters explicitly
> > here, see the comment above).
> > 
> > > + * \brief Apply \a flags to the link between \a source and \a sink pads
> > > + * \param source The source MediaPad
> > > + * \param sink The sink MediaPad
> > > + * \param link The MediaLink to operate on
> > 
> > Why do you need to specify both source and sink, and the link ? The link
> > should identify the source and sink already. If you don't expect the
> > caller to have the link pointer you should look it up internally,
> > otherwise you shouldn't pass the source and sink. And if you expect the
> > caller to have the link pointer, maybe you could make this function a
> > member of the MediaLink class ?
> 
> I'll consider making something like links->setup()
> 
> > > + * \param flags Flags to be applied to the link (MEDIA_LNK_FL_*)
> > > + */
> > > +int MediaDevice::setupLink(const MediaPad *source, const MediaPad
> > > *sink,
> > > +			   MediaLink *link, unsigned int flags)
> > > +{
> > > +	struct media_link_desc linkDesc = { };
> > > +
> > > +	linkDesc.source.entity = source->entity();
> > > +	linkDesc.source.index = source->index();
> > > +	linkDesc.source.flags = MEDIA_PAD_FL_SOURCE;
> > > +
> > > +	linkDesc.sink.entity = sink->entity();
> > > +	linkDesc.sink.index = sink->index();
> > > +	linkDesc.sink.flags = MEDIA_PAD_FL_SINK;
> > > +
> > > +	linkDesc.flags = flags;
> > > +
> > > +	if (ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc)) {
> > > +		LOG(Error) << "Failed to setup link: "
> > > +			   << strerror(errno);
> > > +		return -errno;
> > > +	}
> > > +
> > > +	link->setFlags(0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaDevice::resetLinks()
> > > + * \brief Reset all links on the media graph
> > 
> > Reset to what ? :-)
> 
> Ok... "Clear the ENABLE flags" ? "Reset a link to the non-enabled
> state?"

Maybe "reset all mutable links to disabled state" ?

> > > + *
> > > + * Walk all registered entities, and disable all links from their
> > > + * source pads to other pads.
> > 
> > Wouldn't it be more efficient to walk links instead of entities and pads ?
> 
> Entities have pads
> pads have links
> 
> I could create a global list of links in the media device, but if it's
> just for this, I don't think it's worth.

We could walk all objects and skip non-link objects. I'll let you decide what 
is simpler. As the function resets all links I find a single loop easier.

> > Could you expand the documentation to explain the use cases for this
> > function ? Please also document the return value (this holds true for all
> > other functions returning a value in the whole series).
> 
> What do you mean by use cases? Before operating on a media graph, or
> whenever the pipeline handlers wants to reset enabled but not
> immutable links to the non-enabled state, they have a utlity function
> to do that. Where and how to use it it's up to the pipeline handler,
> even if it were for me, that should happen before doing any operation
> on the media graph.
> 
> Ah, you possibly meant what I just wrote? Damn documentation...

:-)

> > > + */
> > > +int MediaDevice::resetLinks()
> > > +{
> > > +	for (MediaEntity *e : entities_) {
> > > +		for (MediaPad *sourcePad : e->sources()) {
> > > +			for (MediaLink *l : sourcePad->links()) {
> > > +				/*
> > > +				 * Do not reset links that are not enabled
> > > +				 * or immutable.
> > > +				 */
> > > +				if (l->flags() & MEDIA_LNK_FL_IMMUTABLE)
> > > +					continue;
> > > +
> > > +				if (!(l->flags() & MEDIA_LNK_FL_ENABLED))
> > > +					continue;
> > > +
> > > +				/* Get the remote sink pad. */
> > > +				MediaPad *sinkPad = dynamic_cast<MediaPad *>
> > > +						    (getObject(l->sink()));
> > > +				if (!sinkPad)
> > > +					return -ENOENT;
> > > +
> > > +				/* Also get entity to make sure IDs are ok. */
> > > +				MediaEntity *sinkEntity =
> > > +						dynamic_cast<MediaEntity *>
> > > +						(getObject(sinkPad->entity()));
> > > +				if (!sinkEntity)
> > > +					return -ENOENT;
> > > +
> > > +				int ret = setupLink(sourcePad, sinkPad, l, 0);
> > > +				if (ret) {
> > > +					LOG(Error) << "Link reset failed: "
> > > +						   << e->name() << "["
> > > +						   << sourcePad->index()
> > > +						   << "] -> "
> > > +						   << sinkEntity->name() << "["
> > > +						   << sinkPad->index() << "]";
> > > +					return ret;
> > > +				}
> > > +
> > > +				LOG(Info) << "Link reset: "
> > > +					  << e->name() << "["
> > > +					  << sourcePad->index()
> > > +					  << "] -> "
> > > +					  << sinkEntity->name() << "["
> > > +					  << sinkPad->index() << "]";
> > > +			}
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaDevice::link(std::string, unsigned int, std::string,
> > > unsigned int)
> > > + * \brief Setup a link identified by the entities name and their source
> > > and
> > > + * sink pad indexes
> > > + * \param source The source entity name
> > > + * \param sourceIdx The source pad index
> > > + * \param sink The sink entity name
> > > + * \param sinkIdx The sink pad index
> > > + * \param flags The link setup flag (see MEDIA_LNK_FL_*)
> > 
> > If I understand the purpose of this function properly (and the fact that
> > I'm not sure I do means the documentation should be improved :-)), it
> > serves the same purpose as MediaDevice::setupLink(), but with different
> > arguments. I would thus name the two functions setupLink(), as C++ allows
> > overloading functions.
> 
> No, they're different. setupLink() wants pads and a link where to
> operate on, as is private, designed to back up this function which is
> instead public and wants entities names and pad ids. I agree I should
> look if I could make setupLink a link operation and how ita would look
> like.
> 
> > I would go one step further though, at least if you agree with my
> > proposition to move setupLink to MediaLink, and turn this function into a
> > link lookup. The callers would then do something similar to
> > 
> > 	MediaDevice *dev = ...;
> > 	MediaLink *link = dev->link("source", 0, "sink", 3);
> 
>                                                         ^- why three?

It was just an example :-)

> Can links be set as immutable by userspace, or do they get created
> immutable from the driver only?

The immutable flag is set by drivers and not modifiable by userspace.

> > 	if (!link)
> > 		return -ENODEV;
> > 	
> > 	link->setup(flags);
> > 
> > and possibly cache the link pointer internally to avoid looking up links
> > all the time.
> 
> Here I don't agree actually. I think there should be a MediaDevice
> provided "link()" operation that might use the MediaLink "setupLink"

By the way the operation on MediaLink should be called setup(), not 
setupLink().

> but from the caller, nothing should be seen that it's not the
> MediaDevice itself. From the caller I would just like to see
> 
>         ret = mediaDev->link("source", 0, "sink", 1, 1);
> 
> There is no reason (as I immagined this) for the caller to deal with
> links, and pads itself. Entities are an exception maybe, as we might
> need to configure format and controls on their subdevices, but that's
> it.
> 
> Don't you think this provides a better isolation?

The reason is that the caller could look up the few links it's interested in 
at init time, and then use them at runtime.

> > > + */
> > > +int MediaDevice::link(const std::string &source, unsigned int
> > > sourceIdx,
> > > +		      const std::string &sink, unsigned int sinkIdx,
> > 
> > At the cost of longer names, sourcePadIndex and sinkPadIndex ?
> > 
> > > +		      unsigned int flags)
> > > +{
> > > +
> > 
> > Stray blank line.
> > 
> > > +	/* Make sure the supplied link is well formed. */
> > > +	MediaEntity *sourceEntity = getEntityByName(source);
> > > +	if (!sourceEntity) {
> > > +		LOG(Error) << "Entity name: " << source << "not found";
> > > +		return -ENOENT;
> > > +	}
> > > +
> > > +	MediaEntity *sinkEntity = getEntityByName(sink);
> > > +	if (!sinkEntity) {
> > > +		LOG(Error) << "Entity name: " << source << "not found";
> > 
> > s/source/sink/
> > s/Entity name:/Entity/
> > 
> > > +		return -ENOENT;
> > > +	}
> > > +
> > > +	const MediaPad *sourcePad = sourceEntity->getPadByIndex(sourceIdx);
> > > +	if (!sourcePad) {
> > > +		LOG(Error) << "Pad " << sourceIdx << "not found in entity "
> > > +			   << sourceEntity->name();
> > > +		return -ENOENT;
> > > +	}
> > > +
> > > +	const MediaPad *sinkPad = sinkEntity->getPadByIndex(sinkIdx);
> > > +	if (!sinkPad) {
> > > +		LOG(Error) << "Pad " << sinkIdx << "not found in entity "
> > > +			   << sinkEntity->name();
> > > +		return -ENOENT;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Walk all links in the source and search for an entry matching the
> > > +	 * pad ids. If none, the requested link does not exists.
> > > +	 */
> > > +	MediaLink *validLink = nullptr;
> > > +	for (MediaLink *link : sourcePad->links()) {
> > > +		if (link->source() != sourcePad->id())
> > > +			continue;
> > > +
> > > +		if (link->sink() != sinkPad->id())
> > > +			continue;
> > > +
> > > +		validLink = link;
> > > +		break;
> > > +	}
> > > +
> > > +	if (!validLink) {
> > > +		LOG(Error) << "Link not found"
> > > +			   << "\"" << sourceEntity->name() << "\"["
> > > +			   << sourcePad->index() << "] -> "
> > > +			   << "\"" << sinkEntity->name() << "\"["
> > > +			   << sinkPad->index() << "]";
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	int ret = setupLink(sourcePad, sinkPad, validLink, flags);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	LOG(Info) << "Setup link: "
> > 
> > LOG(Debug) at most.
> > 
> > > +		  << "\"" << sourceEntity->name() << "\"["
> > > +		  << sourcePad->index() << "] -> "
> > > +		  << "\"" << sinkEntity->name() << "\"["
> > > +		  << sinkPad->index() << "]"
> > > +		  << " [" << flags << "]";
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index da06eba..4cac687 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -2,6 +2,7 @@ libcamera_sources = files([
> > > 
> > >      'log.cpp',
> > >      'main.cpp',
> > >      'media_object.cpp',
> > > 
> > > +    'media_device.cpp',
> > 
> > sort
> > 
> > >  ])
> > >  
> > >  libcamera_headers = files([