[libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: rkisp1: Use SOF event to warn about late parameters

[Laurent Pinchart]

Hi Niklas,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

On Fri, Nov 27, 2020 at 12:05:10PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 23, 2020 at 11:12:33PM +0100, Niklas Söderlund wrote:
> > In the Timeline approach the idea was to delay queuing buffers to the
> > device until the IPA had a chance to prepare the parameters buffer. A
> > check was still added to warn if the IPA queued buffers before the
> > parameters buffer was filled in.
> >
> > This check happened much sooner then needed as the parameter buffers
> > does not have to be ready when the buffer is queued but just before its

s/its/it's/

> > consumed. As the pipeline now has true knowledge of each SOF we can move
> > the check there and remove the delaying of queuing of buffers.
> >
> > This change really speeds up the IPA reactions as the delays used in the
> > Timeline where approximated while with this change they are driven by
> > events reported by the device.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 78 ++++++++----------------
> >  1 file changed, 24 insertions(+), 54 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index c3c4b5a65e3d9afe..3662a53ac4a43fcd 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -40,7 +40,6 @@ namespace libcamera {
> >  LOG_DEFINE_CATEGORY(RkISP1)
> >
> >  class PipelineHandlerRkISP1;
> > -class RkISP1ActionQueueBuffers;
> >  class RkISP1CameraData;
> >
> >  enum RkISP1ActionType {
> > @@ -203,7 +202,6 @@ private:
> >  			PipelineHandler::cameraData(camera));
> >  	}
> >
> > -	friend RkISP1ActionQueueBuffers;
> >  	friend RkISP1CameraData;
> >  	friend RkISP1Frames;
> >
> > @@ -214,6 +212,7 @@ private:
> >  	void bufferReady(FrameBuffer *buffer);
> >  	void paramReady(FrameBuffer *buffer);
> >  	void statReady(FrameBuffer *buffer);
> > +	void frameStart(uint32_t sequence);
> >
> >  	int allocateBuffers(Camera *camera);
> >  	int freeBuffers(Camera *camera);
> > @@ -348,53 +347,6 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)
> >  	return nullptr;
> >  }
> >
> > -class RkISP1ActionQueueBuffers : public FrameAction
> > -{
> > -public:
> > -	RkISP1ActionQueueBuffers(unsigned int frame, RkISP1CameraData *data,
> > -				 PipelineHandlerRkISP1 *pipe)
> > -		: FrameAction(frame, QueueBuffers), data_(data), pipe_(pipe)
> > -	{
> > -	}
> > -
> > -protected:
> > -	void run() override
> > -	{
> > -		RkISP1FrameInfo *info = data_->frameInfo_.find(frame());
> > -		if (!info)
> > -			LOG(RkISP1, Fatal) << "Frame not known";
> > -
> > -		/*
> > -		 * \todo: If parameters are not filled a better method to handle
> > -		 * the situation than queuing a buffer with unknown content
> > -		 * should be used.
> > -		 *
> > -		 * It seems excessive to keep an internal zeroed scratch
> > -		 * parameters buffer around as this should not happen unless the
> > -		 * devices is under too much load. Perhaps failing the request
> > -		 * and returning it to the application with an error code is
> > -		 * better than queue it to hardware?
> > -		 */
> > -		if (!info->paramFilled)
> > -			LOG(RkISP1, Error)
> > -				<< "Parameters not ready on time for frame "
> > -				<< frame();
> > -
> > -		pipe_->param_->queueBuffer(info->paramBuffer);
> > -		pipe_->stat_->queueBuffer(info->statBuffer);
> > -
> > -		if (info->mainPathBuffer)
> > -			pipe_->mainPath_.queueBuffer(info->mainPathBuffer);
> > -
> > -		if (info->selfPathBuffer)
> > -			pipe_->selfPath_.queueBuffer(info->selfPathBuffer);
> > -	}
> > -
> > -private:
> > -	RkISP1CameraData *data_;
> > -	PipelineHandlerRkISP1 *pipe_;
> > -};
> > -
> >  int RkISP1CameraData::loadIPA()
> >  {
> >  	ipa_ = IPAManager::createIPA(pipe_, 1, 1);
> > @@ -958,9 +910,14 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> >  	op.controls = { request->controls() };
> >  	data->ipa_->processEvent(op);
> >
> > -	data->timeline_.scheduleAction(std::make_unique<RkISP1ActionQueueBuffers>(data->frame_,
> > -										  data,
> > -										  this));
> > +	param_->queueBuffer(info->paramBuffer);
> > +	stat_->queueBuffer(info->statBuffer);
> > +
> > +	if (info->mainPathBuffer)
> > +		mainPath_.queueBuffer(info->mainPathBuffer);
> > +
> > +	if (info->selfPathBuffer)
> > +		selfPath_.queueBuffer(info->selfPathBuffer);

You're queuing the buffers to the ISP, including the ISP parameters,
before the IPA gets a change to even take into account the parameters
for this request to compute the corresponding ISP parameters. Do we
really want to do that ?

> >
> >  	data->frame_++;
> >
> > @@ -1098,6 +1055,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
> >  	mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> >  	selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> >  	stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
> > +	isp_->frameStart.connect(this, &PipelineHandlerRkISP1::frameStart);
> >  	param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
> >
> >  	/*
> > @@ -1177,8 +1135,6 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> >  	if (!info)
> >  		return;
> >
> > -	data->timeline_.bufferReady(buffer);
> > -
> >  	if (data->frame_ <= buffer->metadata().sequence)
> >  		data->frame_ = buffer->metadata().sequence + 1;
> >
> > @@ -1188,6 +1144,20 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> >  	data->ipa_->processEvent(op);
> >  }
> >
> > +void PipelineHandlerRkISP1::frameStart(uint32_t sequence)
> > +{
> > +	if (!activeCamera_)
> > +		return;
> > +
> > +	RkISP1CameraData *data = cameraData(activeCamera_);
> > +
> > +	RkISP1FrameInfo *info = data->frameInfo_.find(sequence);
> > +	if (!info || !info->paramFilled)
> > +		LOG(RkISP1, Info)
> > +			<< "Parameters not ready on time for frame "
> > +			<< sequence;
> > +}
> > +
> 
> This surely is my lack of understanding, but shouldn't we worry about
> this a param buffer queue time ?
> 
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)
> >
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart