[libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share parameter and statistic buffers with IPA

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2021-02-04 20:54:41 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, Feb 04, 2021 at 05:29:43PM +0100, Niklas Söderlund wrote:
> > Use the IPU3Frames helper to share parameters and statistics buffers
> > with the IPA. Track which parameter and statistic buffer is used for
> > which request and make sure the parameter buffers is filled in by the
> > IPA before it's needed and that the statistic buffer is consumed and
> > meta data generated before completing the request.
> > 
> > With this change the IPU3 pipeline is prepared to fully operate with an
> > IPA component.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > * Changes since v1
> > - Update commit message.
> > - s/frameInfo_/frameInfos_/
> > - Refactor away isComplete variable.
> > 
> > * Changes since v2
> > - Emedd the IPU3Frames instance instead of allocating it.
> > - Use -ENOBUFS instead of _ENOENT in queueRequestDevice().
> > - Keep isComplete in cio2BufferReady()
> > - Rework the queue logic.
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 149 ++++++++++++++++++++-------
> >  1 file changed, 112 insertions(+), 37 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 092db6389833a481..b79db25050d2c1d6 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -29,6 +29,7 @@
> >  #include "libcamera/internal/v4l2_controls.h"
> >  
> >  #include "cio2.h"
> > +#include "frames.h"
> >  #include "imgu.h"
> >  
> >  namespace libcamera {
> > @@ -61,6 +62,8 @@ public:
> >  
> >  	void imguOutputBufferReady(FrameBuffer *buffer);
> >  	void cio2BufferReady(FrameBuffer *buffer);
> > +	void paramBufferReady(FrameBuffer *buffer);
> > +	void statBufferReady(FrameBuffer *buffer);
> >  
> >  	CIO2Device cio2_;
> >  	ImgUDevice *imgu_;
> > @@ -71,6 +74,7 @@ public:
> >  
> >  	uint32_t exposureTime_;
> >  	std::unique_ptr<DelayedControls> delayedCtrls_;
> > +	IPU3Frames frameInfos_;
> >  
> >  private:
> >  	void queueFrameAction(unsigned int id, const IPAOperationData &op);
> > @@ -609,6 +613,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> >  
> >  	data->ipa_->mapBuffers(ipaBuffers_);
> >  
> > +	data->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -616,6 +622,8 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> >  
> > +	data->frameInfos_.clear();
> > +
> >  	std::vector<unsigned int> ids;
> >  	for (IPABuffer &ipabuf : ipaBuffers_)
> >  		ids.push_back(ipabuf.id);
> > @@ -713,7 +721,10 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> >  int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> > -	int error = 0;
> > +
> > +	IPU3Frames::Info *info = data->frameInfos_.create(request);
> > +	if (!info)
> > +		return -ENOBUFS;
> >  
> >  	/*
> >  	 * Queue a buffer on the CIO2, using the raw stream buffer provided in
> > @@ -724,24 +735,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> >  	if (!rawBuffer)
> >  		return -ENOMEM;
> 
> What will happen to info in this case ? As there will be no raw buffer
> associated with it, won't it linger forever in a list ?

Good point. I will add a IPU3Frames::remove(Info *info); interface which 
may be used to remove tracking of a request prematurely. It also makes 
the case where the CIO2 buffer is canceled cleaner.

> 
> >  
> > -	/* Queue all buffers from the request aimed for the ImgU. */
> > -	for (auto it : request->buffers()) {
> > -		const Stream *stream = it.first;
> > -		FrameBuffer *buffer = it.second;
> > -		int ret;
> > +	info->rawBuffer = rawBuffer;
> >  
> > -		if (stream == &data->outStream_)
> > -			ret = data->imgu_->output_->queueBuffer(buffer);
> > -		else if (stream == &data->vfStream_)
> > -			ret = data->imgu_->viewfinder_->queueBuffer(buffer);
> > -		else
> > -			continue;
> > -
> > -		if (ret < 0)
> > -			error = ret;
> > -	}
> > -
> > -	return error;
> > +	return 0;
> >  }
> >  
> >  bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> > @@ -1007,6 +1003,10 @@ int PipelineHandlerIPU3::registerCameras()
> >  					&IPU3CameraData::imguOutputBufferReady);
> >  		data->imgu_->viewfinder_->bufferReady.connect(data.get(),
> >  					&IPU3CameraData::imguOutputBufferReady);
> > +		data->imgu_->param_->bufferReady.connect(data.get(),
> > +					&IPU3CameraData::paramBufferReady);
> > +		data->imgu_->stat_->bufferReady.connect(data.get(),
> > +					&IPU3CameraData::statBufferReady);
> >  
> >  		/* Create and register the Camera instance. */
> >  		std::string cameraId = cio2->sensor()->id();
> > @@ -1039,7 +1039,7 @@ int IPU3CameraData::loadIPA()
> >  	return 0;
> >  }
> >  
> > -void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,
> > +void IPU3CameraData::queueFrameAction(unsigned int id,
> >  				      const IPAOperationData &action)
> >  {
> >  	switch (action.operation) {
> > @@ -1048,6 +1048,41 @@ void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,
> >  		delayedCtrls_->push(controls);
> >  		break;
> >  	}
> > +	case IPU3_IPA_ACTION_PARAM_FILLED: {
> > +		IPU3Frames::Info *info = frameInfos_.find(id);
> > +		if (!info)
> > +			break;
> 
> Should we warn in this case ? Maybe in IPU3Frames::info() itself to
> avoid duplicating log calls ? I'm tempted to even assert.

I will add LOG() to IPU3Frames::find() at the Error level. We can then 
easily turn them into Fatal if we think it better in the future.

> 
> > +
> > +		/* Queue all buffers from the request aimed for the ImgU. */
> > +		for (auto it : info->request->buffers()) {
> > +			const Stream *stream = it.first;
> > +			FrameBuffer *outbuffer = it.second;
> > +
> > +			if (stream == &outStream_)
> > +				imgu_->output_->queueBuffer(outbuffer);
> > +			else if (stream == &vfStream_)
> > +				imgu_->viewfinder_->queueBuffer(outbuffer);
> > +		}
> > +
> > +		imgu_->param_->queueBuffer(info->paramBuffer);
> > +		imgu_->stat_->queueBuffer(info->statBuffer);
> > +		imgu_->input_->queueBuffer(info->rawBuffer);
> > +
> > +		break;
> > +	}
> > +	case IPU3_IPA_ACTION_METADATA_READY: {
> > +		IPU3Frames::Info *info = frameInfos_.find(id);
> > +		if (!info)
> > +			break;
> > +
> > +		Request *request = info->request;
> > +		request->metadata() = action.controls[0];
> > +		info->metadataProcessed = true;
> > +		if (frameInfos_.tryComplete(info))
> > +			pipe_->completeRequest(request);
> 
> Maybe we could pass the request to IPU3Frames, and handle completion
> automatically internally, but that's for later.

We did that in earlier versions but it was disliked as a reference to 
the pipeline handler would then need to be stored inside the IPU3Frames.  
I still think it's nicer then the code duplication you point out here.  
But will keep it as is for now.

> 
> > +
> > +		break;
> > +	}
> >  	default:
> >  		LOG(IPU3, Error) << "Unknown action " << action.operation;
> >  		break;
> > @@ -1068,11 +1103,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> >  {
> >  	Request *request = buffer->request();
> >  
> > -	if (!pipe_->completeBuffer(request, buffer))
> > -		/* Request not completed yet, return here. */
> > +	pipe_->completeBuffer(request, buffer);
> > +
> > +	IPU3Frames::Info *info = frameInfos_.find(buffer);
> > +	if (!info)
> >  		return;
> >  
> > -	/* Mark the request as complete. */
> >  	request->metadata().set(controls::draft::PipelineDepth, 3);
> >  	/* \todo Move the ExposureTime control to the IPA. */
> >  	request->metadata().set(controls::ExposureTime, exposureTime_);
> > @@ -1081,7 +1117,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> >  		Rectangle cropRegion = request->controls().get(controls::ScalerCrop);
> >  		request->metadata().set(controls::ScalerCrop, cropRegion);
> >  	}
> > -	pipe_->completeRequest(request);
> > +
> > +	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> > +		info->metadataProcessed = true;
> > +
> > +	if (frameInfos_.tryComplete(info))
> > +		pipe_->completeRequest(request);
> >  }
> >  
> >  /**
> > @@ -1093,26 +1134,60 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> >   */
> >  void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> >  {
> > -	/* \todo Handle buffer failures when state is set to BufferError. */
> > -	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> > +	IPU3Frames::Info *info = frameInfos_.find(buffer);
> > +	if (!info)
> >  		return;
> >  
> >  	Request *request = buffer->request();
> >  
> > -	/*
> > -	 * If the request contains a buffer for the RAW stream only, complete it
> > -	 * now as there's no need for ImgU processing.
> > -	 */
> > -	if (request->findBuffer(&rawStream_)) {
> > -		bool isComplete = pipe_->completeBuffer(request, buffer);
> > -		if (isComplete) {
> > -			request->metadata().set(controls::draft::PipelineDepth, 2);
> > -			pipe_->completeRequest(request);
> > -			return;
> > -		}
> > +	if (request->findBuffer(&rawStream_))
> > +		pipe_->completeBuffer(request, buffer);
> 
> If you moved this after the next if (...) { } block, you could...
> 
> > +
> > +	/* If the buffer is cancelled force a complete of the whole request. */
> > +	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> > +		for (auto it : request->buffers())
> > +			if (it.second != buffer)
> 
> ... drop this check.

Good point.

> 
> > +				pipe_->completeBuffer(request, it.second);
> > +
> > +		info->paramDequeued = true;
> > +		info->metadataProcessed = true;
> > +		ASSERT(frameInfos_.tryComplete(info));
> > +		pipe_->completeRequest(request);
> > +
> > +		return;
> >  	}
> >  
> > -	imgu_->input_->queueBuffer(buffer);
> > +	IPAOperationData op;
> > +	op.operation = IPU3_IPA_EVENT_FILL_PARAMS;
> > +	op.data = { info->id, info->paramBuffer->cookie() };
> > +	op.controls = { request->controls() };
> 
> This is the right time to call IPU3_IPA_EVENT_FILL_PARAMS, but the
> request controls need to be passed to the IPA earlier, right when the
> request is queued, as the IPA needs to look ahead in the request queue
> to compute parameters.
> 
> This is the main issue in this patch, the rest is fairly minor.

I see your point I will fix this for v4.

> 
> > +	ipa_->processEvent(op);
> > +}
> > +
> > +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
> > +{
> > +	IPU3Frames::Info *info = frameInfos_.find(buffer);
> > +	if (!info)
> > +		return;
> > +
> > +	info->paramDequeued = true;
> > +	if (frameInfos_.tryComplete(info))
> > +		pipe_->completeRequest(buffer->request());
> > +}
> > +
> > +void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> > +{
> > +	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> > +		return;
> 
> Why do we need to check for the status for stats buffers but not for
> params buffers ?

Because there is no point in giving a cancelled statistics buffer to the 
IPA. While for the param all we really care about is marking the buffer 
free and moving on. But this error path can't have been tested it should 
set info->metadataProcessed = true and try to complete the request. Will 
fix for v4.

> 
> > +
> > +	IPU3Frames::Info *info = frameInfos_.find(buffer);
> > +	if (!info)
> > +		return;
> > +
> > +	IPAOperationData op;
> > +	op.operation = IPU3_IPA_EVENT_STAT_READY;
> > +	op.data = { info->id, info->statBuffer->cookie() };
> > +	ipa_->processEvent(op);
> >  }
> >  
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund