[libcamera-devel] [PATCH v2 3/4] libcamera: Add link handling functions

[Jacopo Mondi]

Hi Laurent,

On Tue, Jan 08, 2019 at 08:18:45PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tuesday, 8 January 2019 19:04:06 EET Jacopo Mondi wrote:
> > Add a function to the MediaLink class to set the state of a link to
> > enabled or disabled. The function makes use of an internal MediaDevice
> > method, which is defined private and only accessible by the MediaLink
> > setEnable() function itself.
> >
> > Also add to MediaDevice a function to reset all links registered in the
> > media graph.
>
> I would have split this to a separate patch, but it's not a big deal.
>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > v1->v2:
> > - Add resetLinks() function
> > - s/enable()/setEnable()
> > - s/setLink()/setupLink()
> > - Pass MediaLink and not two MediaPads to MediaDevice::setupLink
> >
> >  src/libcamera/include/media_device.h |  4 ++
> >  src/libcamera/include/media_object.h |  1 +
> >  src/libcamera/media_device.cpp       | 75 ++++++++++++++++++++++++++++
> >  src/libcamera/media_object.cpp       | 29 +++++++++++
> >  4 files changed, 109 insertions(+)
> >
> > diff --git a/src/libcamera/include/media_device.h
> > b/src/libcamera/include/media_device.h index 397d349..0f423aa 100644
> > --- a/src/libcamera/include/media_device.h
> > +++ b/src/libcamera/include/media_device.h
> > @@ -45,6 +45,7 @@ public:
> >  	MediaLink *link(const MediaEntity *source, unsigned int sourceIdx,
> >  			const MediaEntity *sink, unsigned int sinkIdx);
> >  	MediaLink *link(const MediaPad *source, const MediaPad *sink);
> > +	int resetLinks();
>
> I wonder if this should be called disableLinks() or disableAllLinks() to
> better reflect the purpose.
>

I find reset confusing as well, although it is used already, but I
find the enable/disable terms better... I'll go with disableLinks()
> >
> >  private:
> >  	std::string driver_;
> > @@ -65,6 +66,9 @@ private:
> >  	bool populateEntities(const struct media_v2_topology &topology);
> >  	bool populatePads(const struct media_v2_topology &topology);
> >  	bool populateLinks(const struct media_v2_topology &topology);
> > +
> > +	friend int MediaLink::setEnable(bool enable);
> > +	int setupLink(const MediaLink *link, unsigned int flags);
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/include/media_object.h
> > b/src/libcamera/include/media_object.h index d92aab3..bb46fac 100644
> > --- a/src/libcamera/include/media_object.h
> > +++ b/src/libcamera/include/media_object.h
> > @@ -41,6 +41,7 @@ public:
> >  	MediaPad *source() const { return source_; }
> >  	MediaPad *sink() const { return sink_; }
> >  	unsigned int flags() const { return flags_; }
> > +	int setEnable(bool enable);
>
> s/setEnable/setEnabled/ ?
>

It's maybe stupid but, since we have disableLinks() should we have an
enable() and a disable() function?

> >
> >  private:
> >  	friend class MediaDevice;
> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> > index 7ce5c22..9900c3f 100644
> > --- a/src/libcamera/media_device.cpp
> > +++ b/src/libcamera/media_device.cpp
> > @@ -385,6 +385,35 @@ MediaLink *MediaDevice::link(const MediaPad *source,
> > const MediaPad *sink) return nullptr;
> >  }
> >
> > +/**
> > + * \brief Reset all links in the media device
>
> How about "Disable all links in the media device" ?
>
> > + *
> > + * Reset the media device links, clearing the MEDIA_LNK_FL_ENABLED flag
> > + * on links which are not flagged as IMMUTABLE.
>
> And updating this accordingly. The "reset" concept isn't clearly defined in
> the MC API, and why media-ctl implements it, I think stating the purpose more
> explicitly would be clearer.
>

Agree

> > + * \return 0 for success, or a negative error code otherwise
>
> s/for/on/
>
> > + */
> > +int MediaDevice::resetLinks()
> > +{
> > +	for (MediaEntity *entity : entities_) {
> > +		for (MediaPad *pad : entity->pads()) {
> > +			if (pad->flags() & MEDIA_PAD_FL_SINK)
>
> I'd rather write this !(pad->flags() & MEDIA_PAD_FL_SOURCE) in case we later
> get pads that have neither flag set.
>

ack

> > +				continue;
> > +
> > +			for (MediaLink *link : pad->links()) {
> > +				if (link->flags() & MEDIA_LNK_FL_IMMUTABLE)
> > +					continue;
> > +
> > +				int ret = link->setEnable(false);
> > +				if (ret)
> > +					return ret;
> > +			}
> > +		}
> > +	}
>
> We don't need to do so in this patch, but do you think we should keep a list
> of links internally in MediaDevice for the purpose of iterating over them
> easily here ?
>

I thought about it... If it is just for this maybe no.. it's not a big
deal though, it's just a list of references...

> > +	return 0;
> > +}
> > +
> >  /**
> >   * \var MediaDevice::objects_
> >   * \brief Global map of media objects (entities, pads, links) keyed by
> > their @@ -601,4 +630,50 @@ bool MediaDevice::populateLinks(const struct
> > media_v2_topology &topology) return true;
> >  }
> >
> > +/**
> > + * \brief Apply \a flags to a link between two pads
> > + * \param link The link to apply flags on
>
> s/on/to/
>
> > + * \param flags The flags to apply to the link
> > + *
> > + * This function applies the link \a flags (as defined by the
> > MEDIA_LNK_FL_* + * macros from the Media Controller API) to the given \a
> > link. It implements + * low-level link setup as it performs no checks on
> > the validity of the \a + * flags, and assumes that the supplied \a flags
> > are valid for the link (e.g. + * immutable links cannot be disabled)."
>
> s/"//
>
> > +*
> > + * \sa MediaLink::setEnable(bool enable)
> > + *
> > + * \return 0 for success, or a negative error code otherwise
>
> s/for/on/
>
> > + */
> > +int MediaDevice::setupLink(const MediaLink *link, unsigned int flags)
> > +{
> > +	struct media_link_desc linkDesc = { };
> > +	MediaPad *source = link->source();
> > +	MediaPad *sink = link->sink();
> > +
> > +	linkDesc.source.entity = source->entity()->id();
> > +	linkDesc.source.index = source->index();
> > +	linkDesc.source.flags = MEDIA_PAD_FL_SOURCE;
> > +
> > +	linkDesc.sink.entity = sink->entity()->id();
> > +	linkDesc.sink.index = sink->index();
> > +	linkDesc.sink.flags = MEDIA_PAD_FL_SINK;
> > +
> > +	linkDesc.flags = flags;
> > +
> > +	int ret = ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc);
> > +	if (ret) {
> > +		ret = -errno;
> > +		LOG(Error) << "Failed to setup link: " << strerror(-ret);
> > +		return ret;
> > +	}
> > +
> > +	LOG(Debug) << source->entity()->name() << "["
> > +		   << source->index() << "] -> "
> > +		   << sink->entity()->name() << "["
> > +		   << sink->index() << "]: " << flags;
> > +
> > +	return 0;
> > +}
> > +
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> > index 612550d..c6cb02b 100644
> > --- a/src/libcamera/media_object.cpp
> > +++ b/src/libcamera/media_object.cpp
> > @@ -15,6 +15,7 @@
> >  #include <linux/media.h>
> >
> >  #include "log.h"
> > +#include "media_device.h"
> >  #include "media_object.h"
> >
> >  /**
> > @@ -89,6 +90,34 @@ namespace libcamera {
> >   * Each link is referenced in the link array of both of the pads it
> > connect. */
> >
> > +/**
> > + * \brief Enable or disable a lik
>
> s/lik/link/
>
> > + * \param enable True to enable the link, false to disable it
> > + *
> > + * Set the status of a link, according to the value of \a enable.
>
> s/link,/link/
>
> > + * Links between two pads can be set to the enabled or disabled state
> > freely, + * unless they're immutable links, whose status cannot be changed.
> > + * Enabling an immutable link is not considered an error, while trying to
> > + * disable it is.
> > + *
> > + * Enabling a link establishes a data connection between two pads, while
> > + * disabling it interrupts such a connections.
>
> s/connections/connection/
>
> or possibly better
>
> s/such a connections/that connection/
>

Yes, better

> > + *
> > + * \return 0 on success, or a negative error code otherwise
> > + */
> > +int MediaLink::setEnable(bool enable)
> > +{
> > +	unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;
> > +
> > +	int ret = dev_->setupLink(this, flags);
> > +	if (ret)
> > +		return ret;
> > +
> > +	flags_ = flags;
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * \brief Construct a MediaLink
> >   * \param link The media link kernel data

Thanks
  j

>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190108/fd3cfdf7/attachment.sig>