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

[Laurent Pinchart]

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.

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

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

> + * \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.

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

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

> + *
> + * \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

-- 
Regards,

Laurent Pinchart