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

[Jacopo Mondi]

Hi Niklas,

On Tue, Dec 15, 2020 at 01:48:10AM +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 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>

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> ---
>  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 513a60b04e5f2e21..3851d94e7b133356 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -41,7 +41,6 @@ namespace libcamera {
>  LOG_DEFINE_CATEGORY(RkISP1)
>
>  class PipelineHandlerRkISP1;
> -class RkISP1ActionQueueBuffers;
>  class RkISP1CameraData;
>
>  enum RkISP1ActionType {
> @@ -202,7 +201,6 @@ private:
>  			PipelineHandler::cameraData(camera));
>  	}
>
> -	friend RkISP1ActionQueueBuffers;
>  	friend RkISP1CameraData;
>  	friend RkISP1Frames;
>
> @@ -213,6 +211,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);
> @@ -347,53 +346,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);
> @@ -950,9 +902,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);
>
>  	data->frame_++;
>
> @@ -1102,6 +1059,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);
>
>  	/*
> @@ -1181,8 +1139,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;
>
> @@ -1192,6 +1148,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;
> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)
>
>  } /* namespace libcamera */
> --
> 2.29.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel