[libcamera-devel] [PATCH 3/5] libcamera: Add MediaLink link setup function

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2019-01-03 18:38:57 +0100, 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
> setup() function itself.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/include/media_device.h |  8 ++++++
>  src/libcamera/include/media_object.h |  1 +
>  src/libcamera/media_device.cpp       | 30 +++++++++++++++++++++
>  src/libcamera/media_object.cpp       | 40 ++++++++++++++++++++++++++++
>  4 files changed, 79 insertions(+)
> 
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index 3228ad5..d019639 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -65,6 +65,14 @@ 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);
> +
> +	/*
> +	 * The MediaLink enable method needs to access the private
> +	 * setLink() function.
> +	 */
> +	friend int MediaLink::enable(bool enable);

I'm not saying we won't need this. But reading this makes me a bit 
uncomfortable that the over all design is not sound. Would it not be OK 
to make setLink() public instead? This interface will not be exposed to 
applications so I don't feel there is a great need to protect us from us 
self :-)

> +	int setLink(const MediaPad *source, const MediaPad *sink,
> +		    unsigned int flags);
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h
> index 0f0bb29..e00c639 100644
> --- a/src/libcamera/include/media_object.h
> +++ b/src/libcamera/include/media_object.h
> @@ -40,6 +40,7 @@ public:
>  	MediaPad *source() const { return source_; }
>  	MediaPad *sink() const { return sink_; }
>  	unsigned int flags() const { return flags_; }
> +	int enable(bool enable);
>  
>  private:
>  	friend class MediaDevice;
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 6892300..b86d0c4 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -641,4 +641,34 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)
>  	return true;
>  }
>  
> +int MediaDevice::setLink(const MediaPad *source, const MediaPad *sink,
> +			 unsigned int flags)
> +{
> +	struct media_link_desc linkDesc = { };
> +
> +	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) << "\n";
> +		return ret;
> +	}
> +
> +	LOG(Debug) << source->entity()->name() << "["
> +		   << source->index() << "] -> "
> +		   << sink->entity()->name() << "["
> +		   << sink->index() << "]: " << flags;

Should you not log linkDesc.flags here in case the IOCTL changed the 
flags? Or maybe even compare flags to linkDesc.flags to make sure the 
flags you want set is set. This is a open question and I'm fine with 
this approach if MEDIA_IOC_SETUP_LINK can not modify the flags.

> +
> +	return 0;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> index f1535e6..1ee8823 100644
> --- a/src/libcamera/media_object.cpp
> +++ b/src/libcamera/media_object.cpp
> @@ -16,6 +16,7 @@
>  
>  #include "log.h"
>  #include "media_object.h"
> +#include "media_device.h"
>  
>  /**
>   * \file media_object.h
> @@ -87,6 +88,45 @@ namespace libcamera {
>   * Each link is referenced in the link array of both of the pads it connect.
>   */
>  
> +/**
> + * \brief Set a link state to enabled or disabled

Enable or disable a link in the media graph

> + * \param enable The enable flag, true enables the link, false disables it
> + *
> + * Set the status of a link, according to the value of \a enable.
> + * 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. Caller should inspect the link flags before trying to
> + * disable an immutable link.
> + *
> + * Enabling a link establishes a data connection between two pads, while
> + * disabling it interrupts such a connections.
> + *
> + * \return 0 for success, negative error code otherwise
> + */
> +int MediaLink::enable(bool enable)
> +{
> +	unsigned int flags = enable ? MEDIA_LNK_FL_ENABLED : 0;
> +
> +	if ((flags_ & MEDIA_LNK_FL_IMMUTABLE) && !enable) {
> +		LOG(Error) << "Immutable links cannot be disabled";
> +		return -EINVAL;
> +	}

Is it sound to relay on cached values here? What if the kernel have 
changed the state of the link to once more be mutable (not sure any 
drivers to this today)? I would drop this check and let the kernel deal 
with this and relay on the return code from the MEDIA_IOC_SETUP_LINK 
ioctl.

> +
> +	/* Do not try to enable an already enabled link. */
> +	if ((flags_ & MEDIA_LNK_FL_ENABLED) && enable)
> +		return 0;

Same here, I would not trust the cached value. What if another process 
have modified the media graph unknown to the library? Better let the 
request happen and let the kernel deal with it.

> +
> +	int ret = media_->setLink(source_, sink_, flags);
> +	if (ret)
> +		return ret;
> +
> +	/* Only update flags if 'setLink()' didn't fail. */
> +	flags_ = flags;
> +
> +	return 0;

I think you can drop the comment and change this to:

    int ret = media_->setLink(source_, sink_, flags);

    if (!ret)
        flags_ = flags;

    return ret;

> +}
> +
>  /**
>   * \brief Construct a MediaLink
>   * \param media The media device this entity belongs to
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund