[libcamera-devel] [PATCH v5 19/19] libcamera: ipu3: Enable ImgU media links

[Laurent Pinchart]

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 at 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 at ideasonboard.com>

until the IPU3 driver gets fixed.

> +	media_->close();
> +
> +	return 0;
> +}
> +
>  /*------------------------------------------------------------------------------
>   * CIO2 Device
>   */

-- 
Regards,

Laurent Pinchart