[v2,3/7] libcamera: rkisp1: Track request->sequence() instead of frame_
diff mbox series

Message ID 20240220164317.998477-4-dan.scally@ideasonboard.com
State New
Headers show
Series
  • Remove RkISP1FrameInfo and IPU3Frames classes
Related show

Commit Message

Daniel Scally Feb. 20, 2024, 4:43 p.m. UTC 
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.

Signed-off-by: Daniel Scally <dan.scally@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(-)

Comments

Jacopo Mondi Feb. 21, 2024, 10 a.m. UTC | #1 
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@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
>
Daniel Scally Feb. 21, 2024, 10:36 a.m. UTC | #2 
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@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
>>

Patch
diff mbox series 

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));
 }