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

[Laurent Pinchart]

Hello,

On Monday, 7 January 2019 10:41:17 EET Jacopo Mondi wrote:
> On Fri, Jan 04, 2019 at 05:45:09PM +0100, Niklas Söderlund wrote:
> > 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.
> > > +	 */

Do we need this comment ?

> > > +	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 :-)
> 
> I discussed it privately as well, as my initial idea was to have all
> of this link handling operations going through the media device object,
> in which case a public setLink() function would be the one the pipeline
> handlers would have to use. To me that would have made possible to
> serialize operations and guarantee that a configuration on the media
> graph is applied entirely, and through a single entity-oriented API,
> instead of providing an interface to retrieve a link and then operate
> on it asynchronously.
> 
> I've been convinced that for now is better to let pipeline handlers
> to retrieve links and then enable/disable them singularly, as the
> 'MediaLink::enable()' operation does, and in that design I had to do a
> few things I'm not super happy with:
> - Add a media device reference to MediaObjects
> - Add the here below friend method so that i can call setLink (which
>   in this way, is at least not public)
> 
> What are your concerns on this design for thinking it is not sound?
> 
> Ah, a note: we don't have to protect us from ourselves (which is not
> totally true anyway...) but remember pipeline handlers will be
> implemented by anyone who has a platform to run libcamera on, and it
> is important we define an unambigous interface to provide them.
> 
> > > +	int setLink(const MediaPad *source, const MediaPad *sink,
> > > +		    unsigned int flags);

Why do you pass the source and sink pointers instead of the link pointer ?

> > >  };
> > >  
> > >  } /* 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);

Should this be called setEnabled() to follow the "getters are named as 
properties, setters with a set prefix" rule ? Furthermore disabling a link by 
calling the enable() method is a bit confusing :-)

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

Alphabetical order please :-)

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

From the point of view of this method, it's not a link flag, but an enable/
disable boolean, so I wouldn't mention flag here.

 * \param enable True to enable the link, false to disable 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.

Shouldn't we reject attempts to enable immutable links ? At best they're no-op 
that should be removed, and at worst they're bugs and should be fixed.

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

I think we usually write this as "0 on success, or a 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)?

No driver does this, and the spec doesn't really allow it.

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

We indeed don't really need to optimize the error code path.

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

We're likely screwed in that case anyway, but I agree there's no real need to 
optimize this.

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

I think you can drop this comment.

> > > +	flags_ = flags;
> > > +
> > > +	return 0;
> > 
> > I think you can drop the comment and change this to:

Ah indeed :-)

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

I would have kept the existing test though, I find the code more readable when 
error paths returns as early as possible. It also makes it easier to later add 
code between the setLink() call and the flags_ assignment.

> >     return ret;
> > > 
> > > +}
> > > +
> > >  /**
> > >   * \brief Construct a MediaLink
> > >   * \param media The media device this entity belongs to

-- 
Regards,

Laurent Pinchart