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

[Dan Scally]

Hi Jacopo, thanks for the review

On 21/02/2024 10:00, Jacopo Mondi wrote:
> 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 ?


I'm not really clear how that is envisioned to work, so I don't know.

> 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) ?


On bufferComplete we will, because buffer->metadata().sequence will differ from 
buffer->request()->sequence.

>
> 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 ?)


It's required in the sense that by the time bufferReady()/statReady() is called the data->frame_ 
member will have been advanced by a subsequent call to queueRequestDevice(), so we couldn't stick to 
the current behaviour of using frame number to index the FCQ without somehow tracking the frame 
number for a particular request (currently that's done through info->frame which is set in 
queueRequestDevice()). If we need to do that to maintain the current behaviour then let's just go 
with your plan of deriving a pipeline specific Request::Private class so we can store it there.


Thanks

Dan

>
>> 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
>>