[5/5] libcamera: rkisp1: Remove RkISP1FrameInfo

19482 diff mbox series

The RkISP1FrameInfo class existed to hold varying bits of info to
help guarantee that a Request is not completed until all of the
image buffers are returned to userspace and the metadata has been
filled by the IPA module. Now that we're no longer using it for
that function it can be removed.

Remove the class and refactor its code out to the rest of the
Pipeline Handler.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 252 +++++------------------
 1 file changed, 46 insertions(+), 206 deletions(-)

Hi Dan

On Mon, Feb 12, 2024 at 03:35:32PM +0000, Daniel Scally wrote:
> The RkISP1FrameInfo class existed to hold varying bits of info to
> help guarantee that a Request is not completed until all of the
> image buffers are returned to userspace and the metadata has been
> filled by the IPA module. Now that we're no longer using it for
> that function it can be removed.
>
> Remove the class and refactor its code out to the rest of the
> Pipeline Handler.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

This is clearly missing something

6015.846619 (0.00 fps) cam0-stream0 seq: 000000 bytesused: 2073600/1036800
[1:40:15.848820375] [297] ERROR RkISP1 rkisp1.cpp:973 Parameters buffer underrun

In particular, the parameters buffers, once completed never gets
inserted back in the availableParamBuffers_ queue. This used to happen
at RkISP1FrameInfo::destroy and we in a similar way as you do for the
stat buffers

The following patch fixes it

 -------------------------------------------------------------------------------
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 4f3e4095c8f0..6381c23bbb4d 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -145,6 +145,7 @@ private:
        void tryCompleteRequest(Request *request);
        void bufferReady(FrameBuffer *buffer);
        void statReady(FrameBuffer *buffer);
+       void paramReady(FrameBuffer *buffer);
        void frameStart(uint32_t sequence);

        int allocateBuffers(Camera *camera);
@@ -167,6 +168,7 @@ private:
        std::queue<FrameBuffer *> availableParamBuffers_;
        std::queue<FrameBuffer *> availableStatBuffers_;
        std::map<int, FrameBuffer *> inFlightStatBuffers_;
+       std::map<int, FrameBuffer *> inFlightParamBuffers_;

        Camera *activeCamera_;

@@ -976,6 +978,7 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)

                paramBuffer = availableParamBuffers_.front();
                availableParamBuffers_.pop();
+               inFlightParamBuffers_[request->sequence()] = paramBuffer;
                paramBuffer->_d()->setRequest(request);
        }

@@ -1178,6 +1181,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
        if (hasSelfPath_)
                selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
        stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
+       param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);

        /*
         * Enumerate all sensors connected to the ISP and create one
@@ -1205,6 +1209,10 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
            inFlightStatBuffers_.end())
                return;

+       if (inFlightParamBuffers_.find(request->sequence()) !=
+           inFlightParamBuffers_.end())
+               return;
+
        completeRequest(request);
 }

@@ -1243,7 +1251,6 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
        RkISP1CameraData *data = cameraData(activeCamera_);
        Request *request = buffer->request();

-
        if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
                inFlightStatBuffers_.erase(request->sequence());
                availableStatBuffers_.push(buffer);
@@ -1255,6 +1262,16 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
                                       data->delayedCtrls_->get(request->sequence()));
 }

+void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
+{
+       ASSERT(activeCamera_);
+       Request *request = buffer->request();
+
+       inFlightParamBuffers_.erase(request->sequence());
+       availableParamBuffers_.push(buffer);
+       tryCompleteRequest(request);
+}
+
 REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)

 } /* namespace libcamera */
 -------------------------------------------------------------------------------

I'm noticing a slower startup of the agc algorithm with this patch. I
can't tell why yet or if it's due to other issues with my testing
setup. How have you validated the series ?

Anyway, I think this is a good first step to remove RkISP1FramesInfo (as
well as IPU3FramesInfo) but we shouldn't be stopping here.

Now that we have the ability to easily creare ::Private subclasses for
application facing classes, I would like to experiment with the
creation of a per-pipeline Request::Private class, which should serve
the same purpose as RkISP1FramesInfo and IPU3FramesInfo did and save
some boilerplate code for pipeline handlers

Even if the net total of code lines added/removed by this series is
negative (\o/) there still is quite some manual work in keeping track
of the inFlighStat/Params buffer and a few more code patterns that
will end up being replicated by all pipelines handlers (reversing the
association between a buffer id and a request, in example).

I'll have a look, basing my work on this series, once you confirm you
don't see visual differences in the algorithm's behaviour.

Thanks
  j

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 252 +++++------------------
>  1 file changed, 46 insertions(+), 206 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index d8f27e96..f6dfd1cb 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -52,44 +52,13 @@ LOG_DEFINE_CATEGORY(RkISP1)
>  class PipelineHandlerRkISP1;
>  class RkISP1CameraData;
>
> -struct RkISP1FrameInfo {
> -	unsigned int frame;
> -	Request *request;
> -
> -	FrameBuffer *paramBuffer;
> -	FrameBuffer *statBuffer;
> -	FrameBuffer *mainPathBuffer;
> -	FrameBuffer *selfPathBuffer;
> -
> -	bool metadataProcessed;
> -};
> -
> -class RkISP1Frames
> -{
> -public:
> -	RkISP1Frames(PipelineHandler *pipe);
> -
> -	RkISP1FrameInfo *create(const RkISP1CameraData *data, Request *request,
> -				bool isRaw);
> -	int destroy(unsigned int frame);
> -	void clear();
> -
> -	RkISP1FrameInfo *find(unsigned int frame);
> -	RkISP1FrameInfo *find(FrameBuffer *buffer);
> -	RkISP1FrameInfo *find(Request *request);
> -
> -private:
> -	PipelineHandlerRkISP1 *pipe_;
> -	std::map<unsigned int, RkISP1FrameInfo *> frameInfo_;
> -};
> -
>  class RkISP1CameraData : public Camera::Private
>  {
>  public:
>  	RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,
>  			 RkISP1SelfPath *selfPath)
> -		: Camera::Private(pipe), frameInfo_(pipe),
> -		  mainPath_(mainPath), selfPath_(selfPath)
> +		: Camera::Private(pipe), mainPath_(mainPath),
> +		  selfPath_(selfPath)
>  	{
>  	}
>
> @@ -101,7 +70,6 @@ public:
>  	std::unique_ptr<CameraSensor> sensor_;
>  	std::unique_ptr<DelayedControls> delayedCtrls_;
>  	std::vector<IPABuffer> ipaBuffers_;
> -	RkISP1Frames frameInfo_;
>
>  	RkISP1MainPath *mainPath_;
>  	RkISP1SelfPath *selfPath_;
> @@ -169,7 +137,6 @@ private:
>  	}
>
>  	friend RkISP1CameraData;
> -	friend RkISP1Frames;
>
>  	int initLinks(Camera *camera, const CameraSensor *sensor,
>  		      const RkISP1CameraConfiguration &config);
> @@ -205,130 +172,6 @@ private:
>  	const MediaPad *ispSink_;
>  };
>
> -RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> -	: pipe_(static_cast<PipelineHandlerRkISP1 *>(pipe))
> -{
> -}
> -
> -RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,
> -				      bool isRaw)
> -{
> -	unsigned int frame = request->sequence();
> -
> -	FrameBuffer *paramBuffer = nullptr;
> -	FrameBuffer *statBuffer = nullptr;
> -
> -	if (!isRaw) {
> -		if (pipe_->availableParamBuffers_.empty()) {
> -			LOG(RkISP1, Error) << "Parameters buffer underrun";
> -			return nullptr;
> -		}
> -
> -		if (pipe_->availableStatBuffers_.empty()) {
> -			LOG(RkISP1, Error) << "Statistic buffer underrun";
> -			return nullptr;
> -		}
> -
> -		paramBuffer = pipe_->availableParamBuffers_.front();
> -		pipe_->availableParamBuffers_.pop();
> -		paramBuffer->_d()->setRequest(request);
> -
> -		statBuffer = pipe_->availableStatBuffers_.front();
> -		pipe_->availableStatBuffers_.pop();
> -		statBuffer->_d()->setRequest(request);
> -	}
> -
> -	FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);
> -	FrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);
> -
> -	RkISP1FrameInfo *info = new RkISP1FrameInfo;
> -
> -	info->frame = frame;
> -	info->request = request;
> -	info->paramBuffer = paramBuffer;
> -	info->mainPathBuffer = mainPathBuffer;
> -	info->selfPathBuffer = selfPathBuffer;
> -	info->statBuffer = statBuffer;
> -	info->metadataProcessed = false;
> -
> -	frameInfo_[frame] = info;
> -
> -	return info;
> -}
> -
> -int RkISP1Frames::destroy(unsigned int frame)
> -{
> -	RkISP1FrameInfo *info = find(frame);
> -	if (!info)
> -		return -ENOENT;
> -
> -	pipe_->availableParamBuffers_.push(info->paramBuffer);
> -	pipe_->availableStatBuffers_.push(info->statBuffer);
> -
> -	frameInfo_.erase(info->frame);
> -
> -	delete info;
> -
> -	return 0;
> -}
> -
> -void RkISP1Frames::clear()
> -{
> -	for (const auto &entry : frameInfo_) {
> -		RkISP1FrameInfo *info = entry.second;
> -
> -		pipe_->availableParamBuffers_.push(info->paramBuffer);
> -		pipe_->availableStatBuffers_.push(info->statBuffer);
> -
> -		delete info;
> -	}
> -
> -	frameInfo_.clear();
> -}
> -
> -RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)
> -{
> -	auto itInfo = frameInfo_.find(frame);
> -
> -	if (itInfo != frameInfo_.end())
> -		return itInfo->second;
> -
> -	LOG(RkISP1, Fatal) << "Can't locate info from frame";
> -
> -	return nullptr;
> -}
> -
> -RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)
> -{
> -	for (auto &itInfo : frameInfo_) {
> -		RkISP1FrameInfo *info = itInfo.second;
> -
> -		if (info->paramBuffer == buffer ||
> -		    info->statBuffer == buffer ||
> -		    info->mainPathBuffer == buffer ||
> -		    info->selfPathBuffer == buffer)
> -			return info;
> -	}
> -
> -	LOG(RkISP1, Fatal) << "Can't locate info from buffer";
> -
> -	return nullptr;
> -}
> -
> -RkISP1FrameInfo *RkISP1Frames::find(Request *request)
> -{
> -	for (auto &itInfo : frameInfo_) {
> -		RkISP1FrameInfo *info = itInfo.second;
> -
> -		if (info->request == request)
> -			return info;
> -	}
> -
> -	LOG(RkISP1, Fatal) << "Can't locate info from request";
> -
> -	return nullptr;
> -}
> -
>  PipelineHandlerRkISP1 *RkISP1CameraData::pipe()
>  {
>  	return static_cast<PipelineHandlerRkISP1 *>(Camera::Private::pipe());
> @@ -387,20 +230,30 @@ void RkISP1CameraData::paramFilled(unsigned int bufferId)
>  		if (paramBuffer->cookie() != bufferId)
>  			continue;
>
> -		RkISP1FrameInfo *info = frameInfo_.find(paramBuffer.get());
> -		if (!info)
> +		Request *request = paramBuffer->request();
> +		FrameBuffer *mainPathBuffer = request->findBuffer(&mainPathStream_);
> +		FrameBuffer *selfPathBuffer = request->findBuffer(&selfPathStream_);
> +
> +		if (pipe->availableStatBuffers_.empty()) {
> +			LOG(RkISP1, Fatal) << "Parameters buffer underrun";
>  			return;
> +		}
> +
> +		FrameBuffer *statBuffer = pipe->availableStatBuffers_.front();
> +		pipe->availableStatBuffers_.pop();
> +		pipe->inFlightStatBuffers_[request->sequence()] = statBuffer;
> +		statBuffer->_d()->setRequest(request);
>
> -		info->paramBuffer->_d()->metadata().planes()[0].bytesused =
> +		paramBuffer->_d()->metadata().planes()[0].bytesused =
>  			sizeof(struct rkisp1_params_cfg);
> -		pipe->param_->queueBuffer(info->paramBuffer);
> -		pipe->stat_->queueBuffer(info->statBuffer);
> +		pipe->param_->queueBuffer(paramBuffer.get());
> +		pipe->stat_->queueBuffer(statBuffer);
>
> -		if (info->mainPathBuffer)
> -			mainPath_->queueBuffer(info->mainPathBuffer);
> +		if (mainPathBuffer)
> +			mainPath_->queueBuffer(mainPathBuffer);
>
> -		if (selfPath_ && info->selfPathBuffer)
> -			selfPath_->queueBuffer(info->selfPathBuffer);
> +		if (selfPath_ && selfPathBuffer)
> +			selfPath_->queueBuffer(selfPathBuffer);
>
>  		return;
>  	}
> @@ -422,15 +275,13 @@ void RkISP1CameraData::metadataReady(unsigned int bufferId,
>  		if (statBuffer->cookie() != bufferId)
>  			continue;
>
> -		RkISP1FrameInfo *info = frameInfo_.find(statBuffer.get());
> -		if (!info)
> -			return;
> +		Request *request = statBuffer->request();
>
> -		info->request->metadata().merge(metadata);
> -		info->metadataProcessed = true;
> -		pipe()->inFlightStatBuffers_.erase(info->request->sequence());
> +		request->metadata().merge(metadata);
> +		pipe()->inFlightStatBuffers_.erase(request->sequence());
> +		pipe()->availableStatBuffers_.push(statBuffer.get());
> +		pipe()->tryCompleteRequest(request);
>
> -		pipe()->tryCompleteRequest(statBuffer->request());
>  		return;
>  	}
>
> @@ -1033,7 +884,6 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)
>  	}
>
>  	ASSERT(data->queuedRequests_.empty());
> -	data->frameInfo_.clear();
>
>  	freeBuffers(camera);
>
> @@ -1043,23 +893,32 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)
>  int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> +	FrameBuffer *paramBuffer = nullptr;
>
> -	RkISP1FrameInfo *info = data->frameInfo_.create(data, request, isRaw_);
> -	if (!info)
> -		return -ENOENT;
> +	if (!isRaw_) {
> +		if (availableParamBuffers_.empty()) {
> +			LOG(RkISP1, Error) << "Parameters buffer underrun";
> +			return -ENOENT;
> +		}
>
> -	inFlightStatBuffers_[request->sequence()] = info->statBuffer;
> +		paramBuffer = availableParamBuffers_.front();
> +		availableParamBuffers_.pop();
> +		paramBuffer->_d()->setRequest(request);
> +	}
>
>  	data->ipa_->queueRequest(request->sequence(), request->controls());
>  	if (isRaw_) {
> -		if (info->mainPathBuffer)
> -			data->mainPath_->queueBuffer(info->mainPathBuffer);
> +		FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);
> +		FrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);
> +
> +		if (mainPathBuffer)
> +			data->mainPath_->queueBuffer(mainPathBuffer);
>
> -		if (data->selfPath_ && info->selfPathBuffer)
> -			data->selfPath_->queueBuffer(info->selfPathBuffer);
> +		if (data->selfPath_ && selfPathBuffer)
> +			data->selfPath_->queueBuffer(selfPathBuffer);
>  	} else {
>  		data->ipa_->fillParamsBuffer(request->sequence(),
> -					     info->paramBuffer->cookie());
> +					     paramBuffer->cookie());
>  	}
>
>  	return 0;
> @@ -1253,12 +1112,6 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>
>  void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
>  {
> -	RkISP1CameraData *data = cameraData(activeCamera_);
> -
> -	RkISP1FrameInfo *info = data->frameInfo_.find(request);
> -	if (!info)
> -		return;
> -
>  	if (request->hasPendingBuffers())
>  		return;
>
> @@ -1266,8 +1119,6 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
>  	    inFlightStatBuffers_.end())
>  		return;
>
> -	data->frameInfo_.destroy(info->frame);
> -
>  	completeRequest(request);
>  }
>
> @@ -1275,11 +1126,6 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
>  	RkISP1CameraData *data = cameraData(activeCamera_);
> -
> -	RkISP1FrameInfo *info = data->frameInfo_.find(buffer);
> -	if (!info)
> -		return;
> -
>  	const FrameMetadata &metadata = buffer->metadata();
>  	Request *request = buffer->request();
>
> @@ -1299,9 +1145,6 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>  			data->ipa_->processStatsBuffer(request->sequence(),
>  						       0, ctrls);
>  		}
> -	} else {
> -		if (isRaw_)
> -			info->metadataProcessed = true;
>  	}
>
>  	completeBuffer(request, buffer);
> @@ -1314,18 +1157,15 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>  	RkISP1CameraData *data = cameraData(activeCamera_);
>  	Request *request = buffer->request();
>
> -	RkISP1FrameInfo *info = data->frameInfo_.find(buffer);
> -	if (!info)
> -		return;
>
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> -		info->metadataProcessed = true;
>  		inFlightStatBuffers_.erase(request->sequence());
> +		availableStatBuffers_.push(buffer);
>  		tryCompleteRequest(request);
>  		return;
>  	}
>
> -	data->ipa_->processStatsBuffer(request->sequence(), info->statBuffer->cookie(),
> +	data->ipa_->processStatsBuffer(request->sequence(), buffer->cookie(),
>  				       data->delayedCtrls_->get(request->sequence()));
>  }
>
> --
> 2.34.1
>