[libcamera-devel] [PATCH 3/5] libcamera: Add MediaLink link setup function
Jacopo Mondi
jacopo at jmondi.org
Mon Jan 7 09:41:17 CET 2019
Hi Niklas,
let me start from this comment
On Fri, Jan 04, 2019 at 05:45:09PM +0100, Niklas Söderlund wrote:
> 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 :-)
>
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.
Thanks
j
> > + 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
-------------- 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/20190107/68c222db/attachment.sig>
More information about the libcamera-devel
mailing list