[PATCH 5/5] libcamera: rkisp1: Remove RkISP1FrameInfo
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Feb 13 14:16:33 CET 2024
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 at 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
>
More information about the libcamera-devel
mailing list