[libcamera-devel] [PATCH RESEND v2 2/4] libcamera: Add MediaDevice class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Dec 28 17:40:51 CET 2018
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 at 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([
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list