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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Mar 25, 2019 at 04:42:22PM +0100, Jacopo Mondi wrote:
> Hi Laurent,
>    one note here below, mostly for the records...
> 
> On Thu, Mar 21, 2019 at 12:17:00PM +0200, Laurent Pinchart wrote:
> > 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.

What does the ImgU write the output to if there's no output buffer
available ?

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

It makes no sense to execute one processing run of the ImgU if there's
no buffer available for its output. The driver may do something now (and
it's unclear what it does), but it shouldn't be the case, and that
should be fixed. We'll have to eventually handle this in the IPU3
pipeline handler.

> >> +}
> >> +
> >>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> >>
> >>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart