[libcamera-devel] [PATCH v2 1/2] libcamera: ipu3: Disable links at configure() time

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jul 16, 2019 at 09:27:38AM +0200, Jacopo Mondi wrote:
> With the current IPU3 kernel driver implementation, a linked pipe shall be
> used (buffers should be queued on it) in order not to block all other pipes.
> 
> Currently all links on the ImgU device are only disabled at match() time,
> implying that once an ImgU pipe gets linked, it should be used until the
> whole pipeline is not re-matched and links disabled again. This is a severe
> limitation for applications that wants to switch between cameras using
> different pipes going through a full library tear-down and reload.
> 
> Perform link disabling at configure() time as well, so that a camera
> configuration operation always unlock the usage of the assigned pipe,
> regardless of the previously linked ones.
> 
> Unfortunately this requires a camera start/stop sequence to always go
> through a configure step, a requirement that is not enforced by the
> Camera state machine.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 54 ++++++++++++++++------------
>  1 file changed, 31 insertions(+), 23 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 159a9312f95e..bae3072b177f 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -499,6 +499,37 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	ImgUDevice *imgu = data->imgu_;
>  	int ret;
>  
> +	/*
> +	 * FIXME: enabled links in one ImgU pipe 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 on the first used
> +	 * pipe.
> +	 *
> +	 * 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 configure(),
> +	 * 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 configure() in between.
> +	 *
> +	 * As of now, disable all links in the ImgU media graph before
> +	 * configuring the device, to allow alternate the usage of the two
> +	 * ImgU pipes.
> +	 *
> +	 * As a consequence, a Camera using an ImgU shall be configured before
> +	 * any start()/stop() sequence. An application that wants to
> +	 * pre-configure all the camera and then start/stop them alternatively
> +	 * without going through any re-configuration (a sequence that is
> +	 * allowed by the Camera state machine) would now fail on the IPU3.
> +	 */
> +	ret = imguMediaDev_->disableLinks();
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * \todo: Enable links selectively based on the requested streams.
>  	 * As of now, enable all links unconditionally.
> @@ -784,29 +815,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>  	if (cio2MediaDev_->disableLinks())
>  		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 configure(),
> -	 * 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 configure() 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.
> -	 */
>  	ret = imguMediaDev_->disableLinks();
>  	if (ret)
>  		return ret;

-- 
Regards,

Laurent Pinchart