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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thu, Jan 07, 2021 at 05:28:17PM +0100, Jacopo Mondi wrote:
> On Tue, Dec 29, 2020 at 05:03:18PM +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.
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 115 +++++++++++++++++++++++++--
> >  1 file changed, 107 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 141066c528890c8e..6cd1879a76b195bf 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 {
> > @@ -60,6 +61,8 @@ public:
> >
> >  	void imguOutputBufferReady(FrameBuffer *buffer);
> >  	void cio2BufferReady(FrameBuffer *buffer);
> > +	void paramBufferReady(FrameBuffer *buffer);
> > +	void statBufferReady(FrameBuffer *buffer);
> >
> >  	CIO2Device cio2_;
> >  	ImgUDevice *imgu_;
> > @@ -69,6 +72,7 @@ public:
> >  	Stream rawStream_;
> >
> >  	std::unique_ptr<DelayedControls> delayedCtrls_;
> > +	std::unique_ptr<IPU3Frames> frameInfos_;

Can this be embedded instead of allocated dynamically ?

> >
> >  private:
> >  	void actOnIpa(unsigned int id, const IPAOperationData &op);
> > @@ -602,6 +606,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> >
> >  	data->ipa_->mapBuffers(ipaBuffers_);
> >
> > +	data->frameInfos_->init(imgu->paramBuffers_, imgu->statBuffers_);
> > +
> >  	return 0;
> >  }
> >
> > @@ -609,6 +615,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);
> > @@ -720,6 +728,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> >  	IPU3CameraData *data = cameraData(camera);
> >  	int error = 0;
> >
> > +	IPU3Frames::Info *info = data->frameInfos_->create(request);
> > +	if (!info)
> > +		return -ENOENT;

-ENOBUFS ?

> > +
> >  	/*
> >  	 * Queue a buffer on the CIO2, using the raw stream buffer provided in
> >  	 * the request, if any, or a CIO2 internal buffer otherwise.
> > @@ -729,7 +741,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> >  	if (!rawBuffer)
> >  		return -ENOMEM;
> >
> > +	info->rawBuffer = rawBuffer;
> > +
> >  	/* Queue all buffers from the request aimed for the ImgU. */
> > +	bool onlyRaw = true;
> >  	for (auto it : request->buffers()) {
> >  		const Stream *stream = it.first;
> >  		FrameBuffer *buffer = it.second;
> > @@ -744,6 +759,23 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> >
> >  		if (ret < 0)
> >  			error = ret;
> > +
> > +		onlyRaw = false;
> > +	}
> > +
> > +	/* If request only contains a raw buffer do not involve IPA. */
> > +	if (onlyRaw) {
> > +		info->paramDequeued = true;
> > +		info->metadataProcessed = true;

I don't think that's right. Even if we only capture raw frames, the IPA
has to run or all the algorithms will stop.

> > +	} else {
> > +		IPAOperationData op;
> > +		op.operation = IPU3_IPA_EVENT_FILL_PARAMS;
> > +		op.data = { info->id, info->paramBuffer->cookie() };
> > +		op.controls = { request->controls() };
> 
> I'm a bit skeptical about this one. I would leave controls out of the
> IPA interface for the moment. We have towards the API a mixed
> v4l2/libcamera controls interface which might require a second
> thought, and not all the controls have to be sent to the IPA. I would
> drop this for the moment.

I'm not sure to agree here. The controls that are received as part of
the request are all libcamera controls, and the IPA needs them. The
separate set of controls for the sensor is currently using V4L2 controls
and that will change, but it's separate from the controls stored in the
request.

> > +		data->ipa_->processEvent(op);
> > +
> > +		data->imgu_->param_->queueBuffer(info->paramBuffer);

That's not right. The processEvent() call is not synchronous, the
parameters buffer will be filled later, you're queuing an empty buffer
here.

Furthermore, even if processEvent() was synchronous, you would end up
requesting ImgU parameters too early if the application has a large
queue of requests.

Controls need to be sent to the IPA as soon as the request is received,
but ISP parameters should be requested only when we get close to needing
them.

> > +		data->imgu_->stat_->queueBuffer(info->statBuffer);
> >  	}
> >
> >  	return error;
> > @@ -893,6 +925,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();
> > @@ -922,10 +958,12 @@ int IPU3CameraData::loadIPA()
> >
> >  	ipa_->init(IPASettings{});
> >
> > +	frameInfos_ = std::make_unique<IPU3Frames>();
> > +
> >  	return 0;
> >  }
> >
> > -void IPU3CameraData::actOnIpa([[maybe_unused]] unsigned int id,
> > +void IPU3CameraData::actOnIpa(unsigned int id,
> >  			      const IPAOperationData &action)
> >  {
> >  	switch (action.operation) {
> > @@ -934,6 +972,27 @@ void IPU3CameraData::actOnIpa([[maybe_unused]] unsigned int id,
> >  		delayedCtrls_->push(controls);
> >  		break;
> >  	}
> > +	case IPU3_IPA_ACTION_PARAM_FILLED: {
> > +		IPU3Frames::Info *info = frameInfos_->find(id);
> > +		if (!info)
> > +			break;
> > +
> > +		info->paramFilled = true;
> > +		break;
> > +	}
> > +	case IPU3_IPA_ACTION_METADATA_READY: {
> 
> The corresponding action when notifying IPA about tha availability of
> the metadata buffer is called IPU3_IPA_EVENT_STAT_READY. The ipa uses
> the terms 'parseStatistics'. Can we stabilize on one name, being it
> metadata, STATS or Statistics ?

Isn't "statistics" (or "stats") used for statistics computed by the ImgU
and sent by the pipeline handler to the IPA, and "metadata" used for
request metadata computed by the IPA and sent by the IPA to the pipeline
handler ?

> > +		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);
> > +
> > +		break;
> > +	}
> >  	default:
> >  		LOG(IPU3, Error) << "Unknown action " << action.operation;
> >  		break;
> > @@ -954,13 +1013,16 @@ 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);
> > -	pipe_->completeRequest(request);
> > +
> > +	if (frameInfos_->tryComplete(info))
> > +		pipe_->completeRequest(request);
> >  }
> >
> >  /**
> > @@ -976,6 +1038,10 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> >  	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> >  		return;
> >
> > +	IPU3Frames::Info *info = frameInfos_->find(buffer);
> > +	if (!info)
> > +		return;
> > +
> >  	Request *request = buffer->request();
> >
> >  	/*
> > @@ -983,17 +1049,50 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> >  	 * now as there's no need for ImgU processing.
> >  	 */
> >  	if (request->findBuffer(&rawStream_)) {
> > -		bool isComplete = pipe_->completeBuffer(request, buffer);
> > -		if (isComplete) {
> 
> I would be kept it. We're familiar with the fact completeBuffer()
> return 1 if the request is complete, but it's not intuitive and
> 'isComplete' helps clarifying it.

Do you think we should add

	enum class RequestStatus {
		RequestPending,
		RequestComplete,
	};

to the PipelineHandler class, and use it as the return type for
completeBuffer() ? This could would then become

		if (pipe_->completeBuffer(request, buffer) == RequestComplete) {

It's more explicit, at the cost of being longer.

> > +		if (pipe_->completeBuffer(request, buffer)) {
> >  			request->metadata().set(controls::draft::PipelineDepth, 2);
> > -			pipe_->completeRequest(request);
> > +			if (frameInfos_->tryComplete(info))
> > +				pipe_->completeRequest(request);
> >  			return;
> >  		}
> >  	}
> >
> > +	if (!info->paramFilled)
> > +		LOG(IPU3, Info)
> > +			<< "Parameters not ready on time for id " << info->id;
> 
> Can this happen ?
> 
> Currently it's not super clear to me how parameters are expected to be
> elaborated by the IPA, we currently send the IPU3_IPA_EVENT_FILL_PARAMS
> in queueRequestDevice(), which immediately returns the
> IPU3_IPA_ACTION_PARAM_FILLED to the pipeline (and a set of sensor
> controls immediately after).
> 
> Isn't actually the parsing of the metadata produced by a capture,
> combined with the user controls, that should generate the new set of
> parameters ?
> 
> It does not make any difference here actually as they're empty.
> 
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > +
> >  	imgu_->input_->queueBuffer(buffer);
> >  }
> >
> > +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
> > +{
> > +	if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> > +		return;
> > +
> > +	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;
> > +
> > +	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)
> >
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart