| Message ID | 20260403173106.473431-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Fri, Apr 03, 2026 at 07:31:06PM +0200, Barnabás Pőcze wrote: > `RkISP1Frames::{clear,destroy}()` always push all buffers from the > `RkISP1FrameInfo` object to the corresponding `available*Buffers_` > queues. This is not entirely correct. > > If no dewarper is used, then `availableMainPathBuffers_` is never > consumed, so it will grow with each request. Fix it by only queueing > the buffer if `RkISP1CameraData::usesDewarper_`. > > Additionally, in raw capture mode, no parameter or statistics buffers are > used, so those queues will be filled up with `nullptr`s without limit. > Fix that by only queueing them if they are not null. > > Fixes: c8f63760e55a ("pipeline: rkisp1: Support raw Bayer capture at runtime") > Fixes: 12b553d691d4 ("libcamera: rkisp1: Plumb the dw100 dewarper as V4L2M2M converter") > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 26 +++++++++++++++--------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index faeeecd96..a8756cefa 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -86,6 +86,8 @@ public: > RkISP1FrameInfo *find(Request *request); > > private: > + void recycleBuffers(RkISP1FrameInfo &info); > + > PipelineHandlerRkISP1 *pipe_; > std::map<unsigned int, RkISP1FrameInfo> frameInfo_; > }; > @@ -316,12 +318,7 @@ int RkISP1Frames::destroy(unsigned int frame) > if (it == frameInfo_.end()) > return -ENOENT; > > - auto &info = it->second; > - > - pipe_->availableParamBuffers_.push(info.paramBuffer); > - pipe_->availableStatBuffers_.push(info.statBuffer); > - pipe_->availableMainPathBuffers_.push(info.mainPathBuffer); > - > + recycleBuffers(it->second); > frameInfo_.erase(it); > > return 0; > @@ -329,13 +326,22 @@ int RkISP1Frames::destroy(unsigned int frame) > > void RkISP1Frames::clear() > { > - for (const auto &[frame, info] : frameInfo_) { > + for (auto &[frame, info] : frameInfo_) > + recycleBuffers(info); > + > + frameInfo_.clear(); > +} > + > +void RkISP1Frames::recycleBuffers(RkISP1FrameInfo &info) > +{ > + if (info.paramBuffer) > pipe_->availableParamBuffers_.push(info.paramBuffer); > + > + if (info.statBuffer) > pipe_->availableStatBuffers_.push(info.statBuffer); > - pipe_->availableMainPathBuffers_.push(info.mainPathBuffer); > - } > > - frameInfo_.clear(); > + if (pipe_->cameraData(pipe_->activeCamera_)->usesDewarper_) I'm wondering if this check is enough or we should also check for info.mainPathBuffer validity. Looking at RkISP1Frames::create() FrameBuffer *mainPathBuffer = nullptr; if (!isRaw) { ... if (data->usesDewarper_) { mainPathBuffer = pipe_->availableMainPathBuffers_.front(); ... } } if (!mainPathBuffer) mainPathBuffer = request->findBuffer(&data->mainPathStream_); info.mainPathBuffer = mainPathBuffer; It seems that it will always be populated and the only case where it has to be returned to the pool is actually caught by the above condition. Good catch, we're certainly wasting memory. Also, one more reason to get rid of the several differernt implementation of *FrameInfo in all our pipelines. Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer); > } > > RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame) > -- > 2.53.0 >
2026. 04. 07. 11:54 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Fri, Apr 03, 2026 at 07:31:06PM +0200, Barnabás Pőcze wrote: >> `RkISP1Frames::{clear,destroy}()` always push all buffers from the >> `RkISP1FrameInfo` object to the corresponding `available*Buffers_` >> queues. This is not entirely correct. >> >> If no dewarper is used, then `availableMainPathBuffers_` is never >> consumed, so it will grow with each request. Fix it by only queueing >> the buffer if `RkISP1CameraData::usesDewarper_`. >> >> Additionally, in raw capture mode, no parameter or statistics buffers are >> used, so those queues will be filled up with `nullptr`s without limit. >> Fix that by only queueing them if they are not null. >> >> Fixes: c8f63760e55a ("pipeline: rkisp1: Support raw Bayer capture at runtime") >> Fixes: 12b553d691d4 ("libcamera: rkisp1: Plumb the dw100 dewarper as V4L2M2M converter") >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 26 +++++++++++++++--------- >> 1 file changed, 16 insertions(+), 10 deletions(-) >> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> index faeeecd96..a8756cefa 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> @@ -86,6 +86,8 @@ public: >> RkISP1FrameInfo *find(Request *request); >> >> private: >> + void recycleBuffers(RkISP1FrameInfo &info); >> + >> PipelineHandlerRkISP1 *pipe_; >> std::map<unsigned int, RkISP1FrameInfo> frameInfo_; >> }; >> @@ -316,12 +318,7 @@ int RkISP1Frames::destroy(unsigned int frame) >> if (it == frameInfo_.end()) >> return -ENOENT; >> >> - auto &info = it->second; >> - >> - pipe_->availableParamBuffers_.push(info.paramBuffer); >> - pipe_->availableStatBuffers_.push(info.statBuffer); >> - pipe_->availableMainPathBuffers_.push(info.mainPathBuffer); >> - >> + recycleBuffers(it->second); >> frameInfo_.erase(it); >> >> return 0; >> @@ -329,13 +326,22 @@ int RkISP1Frames::destroy(unsigned int frame) >> >> void RkISP1Frames::clear() >> { >> - for (const auto &[frame, info] : frameInfo_) { >> + for (auto &[frame, info] : frameInfo_) >> + recycleBuffers(info); >> + >> + frameInfo_.clear(); >> +} >> + >> +void RkISP1Frames::recycleBuffers(RkISP1FrameInfo &info) >> +{ >> + if (info.paramBuffer) >> pipe_->availableParamBuffers_.push(info.paramBuffer); >> + >> + if (info.statBuffer) >> pipe_->availableStatBuffers_.push(info.statBuffer); >> - pipe_->availableMainPathBuffers_.push(info.mainPathBuffer); >> - } >> >> - frameInfo_.clear(); >> + if (pipe_->cameraData(pipe_->activeCamera_)->usesDewarper_) > > I'm wondering if this check is enough or we should also check for > info.mainPathBuffer validity. Not sure, it shouldn't be possible for it to be be nullptr if usesDewarper_, but I suppose it cannot hurt. > > Looking at RkISP1Frames::create() > > FrameBuffer *mainPathBuffer = nullptr; > if (!isRaw) { > > ... > > if (data->usesDewarper_) { > mainPathBuffer = pipe_->availableMainPathBuffers_.front(); > ... > } > } > > if (!mainPathBuffer) > mainPathBuffer = request->findBuffer(&data->mainPathStream_); > > info.mainPathBuffer = mainPathBuffer; > > It seems that it will always be populated and the only case where it > has to be returned to the pool is actually caught by the above > condition. > > Good catch, we're certainly wasting memory. > Also, one more reason to get rid of the several differernt > implementation of *FrameInfo in all our pipelines. > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > >> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer); >> } >> >> RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame) >> -- >> 2.53.0 >>
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index faeeecd96..a8756cefa 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -86,6 +86,8 @@ public: RkISP1FrameInfo *find(Request *request); private: + void recycleBuffers(RkISP1FrameInfo &info); + PipelineHandlerRkISP1 *pipe_; std::map<unsigned int, RkISP1FrameInfo> frameInfo_; }; @@ -316,12 +318,7 @@ int RkISP1Frames::destroy(unsigned int frame) if (it == frameInfo_.end()) return -ENOENT; - auto &info = it->second; - - pipe_->availableParamBuffers_.push(info.paramBuffer); - pipe_->availableStatBuffers_.push(info.statBuffer); - pipe_->availableMainPathBuffers_.push(info.mainPathBuffer); - + recycleBuffers(it->second); frameInfo_.erase(it); return 0; @@ -329,13 +326,22 @@ int RkISP1Frames::destroy(unsigned int frame) void RkISP1Frames::clear() { - for (const auto &[frame, info] : frameInfo_) { + for (auto &[frame, info] : frameInfo_) + recycleBuffers(info); + + frameInfo_.clear(); +} + +void RkISP1Frames::recycleBuffers(RkISP1FrameInfo &info) +{ + if (info.paramBuffer) pipe_->availableParamBuffers_.push(info.paramBuffer); + + if (info.statBuffer) pipe_->availableStatBuffers_.push(info.statBuffer); - pipe_->availableMainPathBuffers_.push(info.mainPathBuffer); - } - frameInfo_.clear(); + if (pipe_->cameraData(pipe_->activeCamera_)->usesDewarper_) + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer); } RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)
`RkISP1Frames::{clear,destroy}()` always push all buffers from the `RkISP1FrameInfo` object to the corresponding `available*Buffers_` queues. This is not entirely correct. If no dewarper is used, then `availableMainPathBuffers_` is never consumed, so it will grow with each request. Fix it by only queueing the buffer if `RkISP1CameraData::usesDewarper_`. Additionally, in raw capture mode, no parameter or statistics buffers are used, so those queues will be filled up with `nullptr`s without limit. Fix that by only queueing them if they are not null. Fixes: c8f63760e55a ("pipeline: rkisp1: Support raw Bayer capture at runtime") Fixes: 12b553d691d4 ("libcamera: rkisp1: Plumb the dw100 dewarper as V4L2M2M converter") Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 26 +++++++++++++++--------- 1 file changed, 16 insertions(+), 10 deletions(-)