| Message ID | 20190103173859.22624-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
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@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
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); + 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; + + 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 + * \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; + } + + /* Do not try to enable an already enabled link. */ + if ((flags_ & MEDIA_LNK_FL_ENABLED) && enable) + return 0; + + int ret = media_->setLink(source_, sink_, flags); + if (ret) + return ret; + + /* Only update flags if 'setLink()' didn't fail. */ + flags_ = flags; + + return 0; +} + /** * \brief Construct a MediaLink * \param media The media device this entity belongs to
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@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(+)