[libcamera-devel] [PATCH 1/3] libcamera: ipu3: Move link disabling at configure time

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Jul 15, 2019 at 07:59:33AM +0200, Jacopo Mondi wrote:
> With the current IPU3 kernel driver implementation, if links are enabled on
> an ImgU pipe buffers should be queued on its output devices in order not to
> block the other pipe.
> 
> Currently all links on the ImgU device are disabled at match() time,
> implying that once an ImgU pipe gets linked, it should be used until the
> whole pipeline handler is not re-matched and links disabled again. This is a
> severe limitation for applications that wants to switch between cameras
> using different pipes without going through a full library tear-down and
> re-initialization.
> 
> Move the link disabling at configure() time, so that a camera
> configuration operation always unlock the usage of the assigned pipe,
> regardless of the status of the one previously in use.
> 
> 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.

How about disabling links in both match() and stop() then ?

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 55 +++++++++++++++-------------
>  1 file changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 22987dbf1460..5204487684c2 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -496,6 +496,35 @@ 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  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 usage of two 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.
> @@ -778,32 +807,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;
>  
>  	ret = registerCameras();
>  

-- 
Regards,

Laurent Pinchart