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

[Jacopo Mondi]

Hi Laurent,

On Mon, Jul 15, 2019 at 09:41:35AM +0300, Laurent Pinchart wrote:
> 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 ?

Fine with match() as starting with a clean media device is desirable
(I'll move the comment here anyway).

If we move this at stop() time though, a start->stop->start sequence
would always fail, so I would avoid that.

Thanks
  j

>
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190716/76db2ab2/attachment-0001.sig>