[PATCH v2 3/7] libcamera: rkisp1: Track request->sequence() instead of frame_

[Jacopo Mondi]

Hi Dan

On Tue, Feb 20, 2024 at 04:43:13PM +0000, Daniel Scally wrote:
> The rkisp1 pipeline handler currently tries to track the frame number
> from V4L2 to provide an index to the IPA module's FCQueue - this is
> not necessary, the Request's sequence does the same job perfectly
> well.
>
> Note that the association of controls and calculated parameters with
> specific frames is not affected here - the parameters are always
> applied based on the most recently processed statistics, and the
> applied controls are fetched separately using the V4L2 provided
> buffer->metadata().sequence, which value is also internally tracked
> by DelayedControls.

Considering that currently data->frame_ gets incremented only when a
new request is queued, this doesn't (in principle) change the
current behaviour if not for the removal of:

	if (data->frame_ <= buffer->metadata().sequence)
		data->frame_ = buffer->metadata().sequence + 1;

The RkISP1 ISP driver has a global frame counter, shared by all video
devices, which is incremented at each vsync. Being the ISP in-line,
the sequence number in buffers coming from the video devices actually
represent the sensor's frame sequence numbers.

The above lines adjust the frame_ value to the 'next' sensor's frame
number in case of a request underrun from the application (ie
application does not provide requests fast enough to keep up with the
frames produced by the sensor).

Now, with your change we always index frames on the IPA FCQ with a
consecutive incresaing counter with no gaps in between (the request
sequence number). How does this work in the context of
per-frame-control ? Will we be able to detect request queueing
underrun and tell applications that they're late (the PFC error
condition we spoke when implementing FCQ) ?

With this change we lose the notion of "which frame (as produced by
the sensor) a request is associated with" and even if delayed controls
helps by tracking the sensor's controls separately, it's something which
might not have implications on the current non-PFC implementation, but
will have to be taken into account once we move in that direction ?

(How is this patch related to the rest of the series ? Is it required
to get rid of FrameInfo ?)

>
> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> ---
> Changes in v2:
>
> 	- In statReady() I no longer used request->sequence() to fetch from
> 	  delayedCtrls_ - ensuring that we only use the V4L2 reported sequence
> 	  there.
>
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 586b46d6..d926c83c 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -88,7 +88,7 @@ class RkISP1CameraData : public Camera::Private
>  public:
>  	RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,
>  			 RkISP1SelfPath *selfPath)
> -		: Camera::Private(pipe), frame_(0), frameInfo_(pipe),
> +		: Camera::Private(pipe), frameInfo_(pipe),
>  		  mainPath_(mainPath), selfPath_(selfPath)
>  	{
>  	}
> @@ -100,7 +100,6 @@ public:
>  	Stream selfPathStream_;
>  	std::unique_ptr<CameraSensor> sensor_;
>  	std::unique_ptr<DelayedControls> delayedCtrls_;
> -	unsigned int frame_;
>  	std::vector<IPABuffer> ipaBuffers_;
>  	RkISP1Frames frameInfo_;
>
> @@ -214,7 +213,7 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
>  RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,
>  				      bool isRaw)
>  {
> -	unsigned int frame = data->frame_;
> +	unsigned int frame = request->sequence();
>
>  	FrameBuffer *paramBuffer = nullptr;
>  	FrameBuffer *statBuffer = nullptr;
> @@ -235,6 +234,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
>
>  		statBuffer = pipe_->availableStatBuffers_.front();
>  		pipe_->availableStatBuffers_.pop();
> +		statBuffer->_d()->setRequest(request);
>  	}
>
>  	FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);
> @@ -935,8 +935,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>  		return ret;
>  	}
>
> -	data->frame_ = 0;
> -
>  	if (!isRaw_) {
>  		ret = param_->streamOn();
>  		if (ret) {
> @@ -1028,7 +1026,7 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>  	if (!info)
>  		return -ENOENT;
>
> -	data->ipa_->queueRequest(data->frame_, request->controls());
> +	data->ipa_->queueRequest(request->sequence(), request->controls());
>  	if (isRaw_) {
>  		if (info->mainPathBuffer)
>  			data->mainPath_->queueBuffer(info->mainPathBuffer);
> @@ -1036,12 +1034,10 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>  		if (data->selfPath_ && info->selfPathBuffer)
>  			data->selfPath_->queueBuffer(info->selfPathBuffer);
>  	} else {
> -		data->ipa_->fillParamsBuffer(data->frame_,
> +		data->ipa_->fillParamsBuffer(request->sequence(),
>  					     info->paramBuffer->cookie());
>  	}
>
> -	data->frame_++;
> -
>  	return 0;
>  }
>
> @@ -1276,7 +1272,8 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>  		if (isRaw_) {
>  			const ControlList &ctrls =
>  				data->delayedCtrls_->get(metadata.sequence);
> -			data->ipa_->processStatsBuffer(info->frame, 0, ctrls);
> +			data->ipa_->processStatsBuffer(request->sequence(),
> +						       0, ctrls);
>  		}
>  	} else {
>  		if (isRaw_)
> @@ -1304,6 +1301,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
>  	RkISP1CameraData *data = cameraData(activeCamera_);
> +	Request *request = buffer->request();
>
>  	RkISP1FrameInfo *info = data->frameInfo_.find(buffer);
>  	if (!info)
> @@ -1315,10 +1313,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>  		return;
>  	}
>
> -	if (data->frame_ <= buffer->metadata().sequence)
> -		data->frame_ = buffer->metadata().sequence + 1;
> -
> -	data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),
> +	data->ipa_->processStatsBuffer(request->sequence(), info->statBuffer->cookie(),
>  				       data->delayedCtrls_->get(buffer->metadata().sequence));
>  }
>
> --
> 2.34.1
>