[libcamera-devel] [PATCH v4 12/31] libcamera: ipu3: Connect CIO2 and ImgU bufferReady signals

[Jacopo Mondi]

Hi Laurent,
   one note here below, mostly for the records...

On Thu, Mar 21, 2019 at 12:17:00PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Mar 20, 2019 at 05:30:36PM +0100, Jacopo Mondi wrote:
> > Connect the CIO2 output bufferRead signal to a slot that simply
> > queue the received buffer to ImgU for processing, and connect the ImgU
> > main output bufferReady signal to the cameraData slot that notifies
> > to applications that a new image buffer is available.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 58 +++++++++++++++++++++++++---
> >  1 file changed, 53 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 8410e1f4b4a6..2623b2fe65f1 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -124,7 +124,9 @@ private:
> >  		{
> >  		}
> >
> > -		void bufferReady(Buffer *buffer);
> > +		void imguOutputBufferReady(Buffer *buffer);
> > +		void imguInputBufferReady(Buffer *buffer);
> > +		void cio2BufferReady(Buffer *buffer);
> >
> >  		CIO2Device cio2;
> >  		ImgUDevice *imgu;
> > @@ -398,6 +400,21 @@ int PipelineHandlerIPU3::start(Camera *camera)
> >  	IPU3CameraData *data = cameraData(camera);
> >  	int ret;
> >
> > +	/*
> > +	 * Connect video devices' 'bufferReady' signals to their slot to
> > +	 * implement the image processing pipeline.
> > +	 *
> > +	 * Frames produced by the CIO2 unit are shared with the associated
> > +	 * ImgU input where they get processed and returned through the ImgU
> > +	 * main and secondary outputs.
> > +	 */
> > +	data->cio2.output->bufferReady.connect(data,
> > +					&IPU3CameraData::cio2BufferReady);
> > +	data->imgu->input->bufferReady.connect(data,
> > +					&IPU3CameraData::imguInputBufferReady);
> > +	data->imgu->output->bufferReady.connect(data,
> > +					&IPU3CameraData::imguOutputBufferReady);
> > +
>
> This should be done when registering cameras, not at start() time,
> otherwise you'll have multiple connections when restarting cameras. This
> call for a camera stream restart test.
>
> >  	/*
> >  	 * Enqueue all available buffers to the CIO2 unit to start frame
> >  	 * capture. Start ImgU video devices and queue buffers to the output
> > @@ -986,9 +1003,6 @@ void PipelineHandlerIPU3::registerCameras()
> >  								cameraName,
> >  								streams);
> >
> > -		cio2->output->bufferReady.connect(data.get(),
> > -						  &IPU3CameraData::bufferReady);
> > -
> >  		registerCamera(std::move(camera), std::move(data));
> >
> >  		LOG(IPU3, Info)
> > @@ -1000,7 +1014,29 @@ void PipelineHandlerIPU3::registerCameras()
> >  	}
> >  }
> >
> > -void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)
> > +/* ----------------------------------------------------------------------------
>
> You're allowed one more dash ;-)
>
> > + * Buffer Ready slots
> > + */
> > +
> > +/**
> > + * \brief ImgU input BufferReady slot
>
> How about "Handle buffers completion at the ImgU input" ? Same for the
> other functions below.
>
> > + * \param buffer The completed buffer
> > + *
> > + * Buffer completed from the ImgU input are immediately queued back to the
>
> s/Buffer/Buffers/
>
> Same below.
>
> > + * CIO2 unit to continue frame capture.
> > + */
> > +void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> > +{
> > +	cio2.output->queueBuffer(buffer);
> > +}
> > +
> > +/**
> > + * \brief ImgU output BufferReady slot
> > + * \param buffer The completed buffer
> > + *
> > + * Buffer completed from the ImgU output are directed to the applications.
>
> s/applications/application/
>
> > + */
> > +void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> >  {
> >  	Request *request = queuedRequests_.front();
> >
> > @@ -1008,6 +1044,18 @@ void PipelineHandlerIPU3::IPU3CameraData::bufferReady(Buffer *buffer)
> >  	pipe_->completeRequest(camera_, request);
> >  }
> >
> > +/**
> > + * \brief CIO2 BufferReady slot
> > + * \param buffer The completed buffer
> > + *
> > + * Buffer completed from the CIO2 are immediately queued to the ImgU unit
> > + * for further processing.
> > + */
> > +void PipelineHandlerIPU3::IPU3CameraData::cio2BufferReady(Buffer *buffer)
> > +{
> > +	imgu->input->queueBuffer(buffer);
>
> Now this is where the fun will begin. What happens if the application
> gets slow and has no request queued ? I think you'll need to requeue the
> buffer to the cio2 instead of the imgu input in that case.
>

So, I've done some profiling of the buffer queue/requeue sequences,
inserting random delays in the 'cam' application, both during the
first request queuing sequence and both at request re-queueing time
after a request has completed.

What I've seen is that regardless of the availability of output
buffers to capture, the CIO2->ImgU buffer sharing does not stall.

In facts, once a buffer has been queued to the ImgU, as soon as its
processing is completed it is immediately returned to userspace, and
it is thus possible to re-queue it back immediately to the CIO2.

In other words, the buffer sharing between CIO2 output and ImgU input
does not depend on the availability of output buffers, and does not
stall if none is queued to the ImgU's main or secondary outputs.

This seems to be different to what is reported by mainline IPU3
developers, that describes the usage of main output as mandatory.

Thanks
   j

> > +}
> > +
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> >
> >  } /* namespace libcamera */
>
> --
> 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/20190325/c927cea3/attachment.sig>