| Message ID | 20190326083902.26121-20-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Tue, Mar 26, 2019 at 09:39:02AM +0100, Jacopo Mondi wrote: > As the lenghty comment reports, link enable/disable is not trivial, as s/lenghty/lengthy/ > links in one ImgU instance interfere with capture operations in the > other one. As it is not yet clear if this behaviour is intended, > as of now reset the media graph during pipeline's match() and enable > the required links at streamConfiguration time. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 111 +++++++++++++++++++++++++++ > 1 file changed, 111 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 66efcc37d695..f92728ae5b85 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -114,6 +114,11 @@ public: > int start(); > void stop(); > > + int linkSetup(const std::string &source, unsigned int sourcePad, > + const std::string &sink, unsigned int sinkPad, > + bool enable); > + int enableLinks(bool enable); > + > unsigned int index_; > std::string name_; > MediaDevice *media_; > @@ -304,6 +309,14 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > return -EINVAL; > } > > + /* > + * \todo: Enable links selectively based on the requested streams. > + * As of now, enable all links unconditionally. > + */ > + ret = data->imgu_->enableLinks(true); > + if (ret) > + return ret; > + > /* > * Pass the requested output image size to the sensor and get back the > * adjusted one to be propagated to the to the ImgU devices. > @@ -506,6 +519,30 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > return false; > } > > + /* > + * FIXME: enabled links in one ImgU instance interfere with capture > + * operations on the other one. This can be easily triggered by > + * capturing from one camera and then trying to capture from the other > + * one right after, without disabling media links in the media graph > + * first. > + * > + * The tricky part here is where to disable links on the ImgU instance > + * which is currently not in use: > + * 1) Link enable/disable cannot be done at start/stop time as video > + * devices needs to be linked first before format can be configured on > + * them. > + * 2) As link enable has to be done at the least in configureStreams, > + * before configuring formats, the only place where to disable links > + * would be 'stop()', but the Camera class state machine allows > + * start()<->stop() sequences without any streamConfiguration() in > + * between. > + * > + * As of now, disable all links in the media graph at 'match()' time, > + * to allow testing different cameras in different test applications > + * runs. A test application that would use two distinct cameras without > + * going through a library teardown->match() sequence would fail > + * at the moment. > + */ > if (imguMediaDev_->disableLinks()) > goto error; > > @@ -946,6 +983,80 @@ void ImgUDevice::stop() > input_->streamOff(); > } > > +/** > + * \brief Enable or disable a single link on the ImgU instance > + * > + * This method assumes the media device associated with the ImgU instance > + * is open. > + * > + * \return 0 on success or a negative error code otherwise > + */ > +int ImgUDevice::linkSetup(const std::string &source, unsigned int sourcePad, > + const std::string &sink, unsigned int sinkPad, > + bool enable) > +{ > + MediaLink *link = media_->link(source, sourcePad, sink, sinkPad); > + if (!link) { > + LOG(IPU3, Error) > + << "Failed to get link: '" << source << "':" > + << sourcePad << " -> '" << sink << "':" << sinkPad; > + return -ENODEV; > + } > + > + return link->setEnabled(enable); > +} Is this worth a helper in MediaDevice ? > + > +/** > + * \brief Enable or disable all media links in the ImgU instance to prepare > + * for capture operations > + * > + * \todo This method will probably be removed or changed once links will be > + * enabled or disabled selectively. > + * > + * \return 0 on success or a negative error code otherwise > + */ > +int ImgUDevice::enableLinks(bool enable) > +{ > + int ret; > + > + /* \todo Establish rules to handle media devices open/close. */ > + ret = media_->open(); > + if (ret) > + return ret; > + > + std::string inputName = name_ + " input"; > + ret = linkSetup(inputName, 0, name_, PAD_INPUT, enable); > + if (ret) { > + media_->close(); > + return ret; > + } > + > + std::string outputName = name_ + " output"; > + ret = linkSetup(name_, PAD_OUTPUT, outputName, 0, enable); > + if (ret) { > + media_->close(); > + return ret; > + } > + > + std::string viewfinderName = name_ + " viewfinder"; > + ret = linkSetup(name_, PAD_VF, viewfinderName, 0, enable); > + if (ret) { > + media_->close(); > + return ret; > + } > + > + std::string statName = name_ + " 3a stat"; > + ret = linkSetup(name_, PAD_STAT, statName, 0, enable); > + if (ret) { > + media_->close(); > + return ret; > + } > + You can add a done label here, return ret at the end of the function and replace all the media_->close(); return ret; with a goto done. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> until the IPU3 driver gets fixed. > + media_->close(); > + > + return 0; > +} > + > /*------------------------------------------------------------------------------ > * CIO2 Device > */
Hi Jacopo, Thanks for your work. On 2019-03-26 09:39:02 +0100, Jacopo Mondi wrote: > As the lenghty comment reports, link enable/disable is not trivial, as > links in one ImgU instance interfere with capture operations in the > other one. As it is not yet clear if this behaviour is intended, > as of now reset the media graph during pipeline's match() and enable > the required links at streamConfiguration time. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 111 +++++++++++++++++++++++++++ > 1 file changed, 111 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 66efcc37d695..f92728ae5b85 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -114,6 +114,11 @@ public: > int start(); > void stop(); > > + int linkSetup(const std::string &source, unsigned int sourcePad, > + const std::string &sink, unsigned int sinkPad, > + bool enable); > + int enableLinks(bool enable); > + > unsigned int index_; > std::string name_; > MediaDevice *media_; > @@ -304,6 +309,14 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > return -EINVAL; > } > > + /* > + * \todo: Enable links selectively based on the requested streams. > + * As of now, enable all links unconditionally. > + */ > + ret = data->imgu_->enableLinks(true); > + if (ret) > + return ret; > + > /* > * Pass the requested output image size to the sensor and get back the > * adjusted one to be propagated to the to the ImgU devices. > @@ -506,6 +519,30 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) > return false; > } > > + /* > + * FIXME: enabled links in one ImgU instance interfere with capture > + * operations on the other one. This can be easily triggered by > + * capturing from one camera and then trying to capture from the other > + * one right after, without disabling media links in the media graph > + * first. > + * > + * The tricky part here is where to disable links on the ImgU instance > + * which is currently not in use: > + * 1) Link enable/disable cannot be done at start/stop time as video > + * devices needs to be linked first before format can be configured on > + * them. > + * 2) As link enable has to be done at the least in configureStreams, > + * before configuring formats, the only place where to disable links > + * would be 'stop()', but the Camera class state machine allows > + * start()<->stop() sequences without any streamConfiguration() in > + * between. > + * > + * As of now, disable all links in the media graph at 'match()' time, > + * to allow testing different cameras in different test applications > + * runs. A test application that would use two distinct cameras without > + * going through a library teardown->match() sequence would fail > + * at the moment. > + */ > if (imguMediaDev_->disableLinks()) > goto error; > > @@ -946,6 +983,80 @@ void ImgUDevice::stop() > input_->streamOff(); > } > > +/** > + * \brief Enable or disable a single link on the ImgU instance > + * > + * This method assumes the media device associated with the ImgU instance > + * is open. > + * > + * \return 0 on success or a negative error code otherwise > + */ > +int ImgUDevice::linkSetup(const std::string &source, unsigned int sourcePad, > + const std::string &sink, unsigned int sinkPad, > + bool enable) > +{ > + MediaLink *link = media_->link(source, sourcePad, sink, sinkPad); > + if (!link) { > + LOG(IPU3, Error) > + << "Failed to get link: '" << source << "':" > + << sourcePad << " -> '" << sink << "':" << sinkPad; > + return -ENODEV; > + } > + > + return link->setEnabled(enable); > +} > + > +/** > + * \brief Enable or disable all media links in the ImgU instance to prepare > + * for capture operations > + * > + * \todo This method will probably be removed or changed once links will be > + * enabled or disabled selectively. > + * > + * \return 0 on success or a negative error code otherwise > + */ > +int ImgUDevice::enableLinks(bool enable) > +{ > + int ret; > + > + /* \todo Establish rules to handle media devices open/close. */ > + ret = media_->open(); > + if (ret) > + return ret; > + > + std::string inputName = name_ + " input"; > + ret = linkSetup(inputName, 0, name_, PAD_INPUT, enable); > + if (ret) { > + media_->close(); > + return ret; > + } > + > + std::string outputName = name_ + " output"; > + ret = linkSetup(name_, PAD_OUTPUT, outputName, 0, enable); > + if (ret) { > + media_->close(); > + return ret; > + } > + > + std::string viewfinderName = name_ + " viewfinder"; > + ret = linkSetup(name_, PAD_VF, viewfinderName, 0, enable); > + if (ret) { > + media_->close(); > + return ret; > + } > + > + std::string statName = name_ + " 3a stat"; > + ret = linkSetup(name_, PAD_STAT, statName, 0, enable); > + if (ret) { > + media_->close(); > + return ret; > + } > + > + media_->close(); > + > + return 0; With Laurents suggestion to use a done label here, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > +} > + > /*------------------------------------------------------------------------------ > * CIO2 Device > */ > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 66efcc37d695..f92728ae5b85 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -114,6 +114,11 @@ public: int start(); void stop(); + int linkSetup(const std::string &source, unsigned int sourcePad, + const std::string &sink, unsigned int sinkPad, + bool enable); + int enableLinks(bool enable); + unsigned int index_; std::string name_; MediaDevice *media_; @@ -304,6 +309,14 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, return -EINVAL; } + /* + * \todo: Enable links selectively based on the requested streams. + * As of now, enable all links unconditionally. + */ + ret = data->imgu_->enableLinks(true); + if (ret) + return ret; + /* * Pass the requested output image size to the sensor and get back the * adjusted one to be propagated to the to the ImgU devices. @@ -506,6 +519,30 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator) return false; } + /* + * FIXME: enabled links in one ImgU instance interfere with capture + * operations on the other one. This can be easily triggered by + * capturing from one camera and then trying to capture from the other + * one right after, without disabling media links in the media graph + * first. + * + * The tricky part here is where to disable links on the ImgU instance + * which is currently not in use: + * 1) Link enable/disable cannot be done at start/stop time as video + * devices needs to be linked first before format can be configured on + * them. + * 2) As link enable has to be done at the least in configureStreams, + * before configuring formats, the only place where to disable links + * would be 'stop()', but the Camera class state machine allows + * start()<->stop() sequences without any streamConfiguration() in + * between. + * + * As of now, disable all links in the media graph at 'match()' time, + * to allow testing different cameras in different test applications + * runs. A test application that would use two distinct cameras without + * going through a library teardown->match() sequence would fail + * at the moment. + */ if (imguMediaDev_->disableLinks()) goto error; @@ -946,6 +983,80 @@ void ImgUDevice::stop() input_->streamOff(); } +/** + * \brief Enable or disable a single link on the ImgU instance + * + * This method assumes the media device associated with the ImgU instance + * is open. + * + * \return 0 on success or a negative error code otherwise + */ +int ImgUDevice::linkSetup(const std::string &source, unsigned int sourcePad, + const std::string &sink, unsigned int sinkPad, + bool enable) +{ + MediaLink *link = media_->link(source, sourcePad, sink, sinkPad); + if (!link) { + LOG(IPU3, Error) + << "Failed to get link: '" << source << "':" + << sourcePad << " -> '" << sink << "':" << sinkPad; + return -ENODEV; + } + + return link->setEnabled(enable); +} + +/** + * \brief Enable or disable all media links in the ImgU instance to prepare + * for capture operations + * + * \todo This method will probably be removed or changed once links will be + * enabled or disabled selectively. + * + * \return 0 on success or a negative error code otherwise + */ +int ImgUDevice::enableLinks(bool enable) +{ + int ret; + + /* \todo Establish rules to handle media devices open/close. */ + ret = media_->open(); + if (ret) + return ret; + + std::string inputName = name_ + " input"; + ret = linkSetup(inputName, 0, name_, PAD_INPUT, enable); + if (ret) { + media_->close(); + return ret; + } + + std::string outputName = name_ + " output"; + ret = linkSetup(name_, PAD_OUTPUT, outputName, 0, enable); + if (ret) { + media_->close(); + return ret; + } + + std::string viewfinderName = name_ + " viewfinder"; + ret = linkSetup(name_, PAD_VF, viewfinderName, 0, enable); + if (ret) { + media_->close(); + return ret; + } + + std::string statName = name_ + " 3a stat"; + ret = linkSetup(name_, PAD_STAT, statName, 0, enable); + if (ret) { + media_->close(); + return ret; + } + + media_->close(); + + return 0; +} + /*------------------------------------------------------------------------------ * CIO2 Device */
As the lenghty comment reports, link enable/disable is not trivial, as links in one ImgU instance interfere with capture operations in the other one. As it is not yet clear if this behaviour is intended, as of now reset the media graph during pipeline's match() and enable the required links at streamConfiguration time. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 111 +++++++++++++++++++++++++++ 1 file changed, 111 insertions(+)