[libcamera-devel] [PATCH 3/3] libcamera: pipeline: rkisp1: Add internal request queue

[Laurent Pinchart]

Hi Nícolas,

Thank you for the patch.

On Mon, Jul 19, 2021 at 04:14:38PM -0300, Nícolas F. R. A. Prado wrote:
> Add an internal queue that stores requests until there are internal
> buffers and V4L2 buffer slots available. This avoids the need to cancel
> requests when there is a shortage of said buffers.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 73 +++++++++++++++++++-----
>  1 file changed, 59 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index f4ea2fd4d4d0..f1c75b7d37c5 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -89,6 +89,9 @@ public:
>  
>  	int loadIPA(unsigned int hwRevision);
>  
> +	void queuePendingRequests();
> +	void cancelPendingRequests();
> +
>  	Stream mainPathStream_;
>  	Stream selfPathStream_;
>  	std::unique_ptr<CameraSensor> sensor_;
> @@ -102,6 +105,8 @@ public:
>  
>  	std::unique_ptr<ipa::rkisp1::IPAProxyRkISP1> ipa_;
>  
> +	std::queue<Request *> pendingRequests_;
> +
>  private:
>  	void queueFrameAction(unsigned int frame,
>  			      const ipa::rkisp1::RkISP1Action &action);
> @@ -199,13 +204,13 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
>  	unsigned int frame = data->frame_;
>  
>  	if (pipe_->availableParamBuffers_.empty()) {
> -		LOG(RkISP1, Error) << "Parameters buffer underrun";
> +		LOG(RkISP1, Debug) << "Parameters buffer underrun";
>  		return nullptr;
>  	}
>  	FrameBuffer *paramBuffer = pipe_->availableParamBuffers_.front();
>  
>  	if (pipe_->availableStatBuffers_.empty()) {
> -		LOG(RkISP1, Error) << "Statisitc buffer underrun";
> +		LOG(RkISP1, Debug) << "Statistic buffer underrun";
>  		return nullptr;
>  	}
>  	FrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();
> @@ -373,6 +378,52 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta
>  	pipe->tryCompleteRequest(info->request);
>  }
>  
> +void RkISP1CameraData::queuePendingRequests()
> +{
> +	while (!pendingRequests_.empty()) {
> +		Request *request = pendingRequests_.front();
> +
> +		/*
> +		 * If there aren't internal buffers available, we break and try
> +		 * again later. If there are, we're guaranteed to also have V4L2
> +		 * buffer slots available to queue the request, since we should
> +		 * always have more (or equal) buffer slots than internal
> +		 * buffers.
> +		 */
> +		RkISP1FrameInfo *info = frameInfo_.create(this, request);
> +		if (!info)
> +			break;
> +
> +		ipa::rkisp1::RkISP1Event ev;
> +		ev.op = ipa::rkisp1::EventQueueRequest;
> +		ev.frame = frame_;
> +		ev.bufferId = info->paramBuffer->cookie();
> +		ev.controls = request->controls();
> +		ipa_->processEvent(ev);
> +
> +		frame_++;
> +
> +		pendingRequests_.pop();
> +	}
> +}
> +
> +void RkISP1CameraData::cancelPendingRequests()
> +{
> +	while (!pendingRequests_.empty()) {
> +		Request *request = pendingRequests_.front();
> +
> +		for (auto it : request->buffers()) {
> +			FrameBuffer *buffer = it.second;
> +			buffer->cancel();
> +			pipe_->completeBuffer(request, buffer);
> +		}
> +
> +		pipe_->completeRequest(request);
> +
> +		pendingRequests_.pop();
> +	}
> +}
> +
>  RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
>  						     RkISP1CameraData *data)
>  	: CameraConfiguration()
> @@ -827,6 +878,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
>  
>  	isp_->setFrameStartEnabled(false);
>  
> +	data->cancelPendingRequests();
> +

I was going to say that this should be done after stopping the video
devices, otherwise you'll complete the pending requests out of order,
before the requests that have been queued already, but the pipeline
handler core will reorder the completions, so it should be fine. Still,
if they can be cancelled after stopping the video devices, it would be a
more logical order, so I'd prefer that. Same comment for the other
patches in this series.

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

>  	data->ipa_->stop();
>  
>  	selfPath_.stop();
> @@ -854,18 +907,8 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
>  
> -	RkISP1FrameInfo *info = data->frameInfo_.create(data, request);
> -	if (!info)
> -		return -ENOENT;
> -
> -	ipa::rkisp1::RkISP1Event ev;
> -	ev.op = ipa::rkisp1::EventQueueRequest;
> -	ev.frame = data->frame_;
> -	ev.bufferId = info->paramBuffer->cookie();
> -	ev.controls = request->controls();
> -	data->ipa_->processEvent(ev);
> -
> -	data->frame_++;
> +	data->pendingRequests_.push(request);
> +	data->queuePendingRequests();
>  
>  	return 0;
>  }
> @@ -1062,6 +1105,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
>  	data->frameInfo_.destroy(info->frame);
>  
>  	completeRequest(request);
> +
> +	data->queuePendingRequests();
>  }
>  
>  void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)

-- 
Regards,

Laurent Pinchart