| Message ID | 20250624122830.726987-2-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote: > Most pipeline handlers release buffers when a camera is stopped. However, > the raspberry pi pipelines do not do that. Hence they are incompatible The RPi pipeline is the only one that, apart from UVC, that implements PipelineHandler::releaseDevice(), called in the Camera::release() path. The RPi implementation of ::releaseDevice() calls "data->freeBuffer()" which ends up calling the RPiStream Stream::releaseBuffers() which first releases buffers from the video device and then clears the FrameBuffers created around them. Now if I get this right, this also clears bufferMap_ which is populated through Stream::setExportedBuffers() in the PipelineHandlerBase::exportFrameBuffers() call. To make this short, my understanding is that rpi releases all allocated buffers on the video device at configure() and release() time. I don't think we specify anywhere how pipelines should behave, but this could certainly save allocations/deallocation at every start/stop cycle. > with the current model and all "CaptureStartStop" tests fail because > lc-compliance allocates buffers before starting and releases them after > stopping. Fix this by allocating buffers after the camera is configured, > and do not release them after stopping. So we now have two models at play: one where buffers allocated on the video device through FrameBufferAllocator are released at stop() time and one where they are released at configure()/release() time. I wonder, but that's a general question, not specifically on this patch which does anyway implement the behaviour I'm about to describe: what happens if a FrameBuffers created with FrameBufferAllocator outlives the buffers allocated on the video device it wraps ? With this patch applied, FrameBuffers in the frame buffer allocator are released only at ~Capture(), while when running this on, say, rkisp1, the memory buffers on the video device are released at every stop(). I feel like I'm missing something obvious, otherwise things should go booom quite fast on pipelines !rpi with this change in.. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++----- > src/apps/lc-compliance/helpers/capture.h | 1 + > 2 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > index d076e964a..df0ad00ac 100644 > --- a/src/apps/lc-compliance/helpers/capture.cpp > +++ b/src/apps/lc-compliance/helpers/capture.cpp > @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr<Camera> camera) > Capture::~Capture() > { > stop(); > + unconfigure(); a little bikeshedding to make this review more fun: I would call this freeBuffers() or something similar. Thanks j > } > > void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > ASSERT_EQ(config->validate(), CameraConfiguration::Valid); > ASSERT_EQ(camera_->configure(config.get()), 0); > > + const auto bufferCount = config->at(0).bufferCount; > + > + for (const auto &cfg : *config) { > + Stream *stream = cfg.stream(); > + > + ASSERT_GE(allocator_.allocate(stream), 0) > + << "Failed to allocate buffers"; > + > + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount) > + << "Mismatching buffer count"; > + } > + > + ASSERT_TRUE(allocator_.allocated()); > + > config_ = std::move(config); > } > > @@ -112,7 +127,7 @@ void Capture::start() > { > assert(config_); > assert(!config_->empty()); > - assert(!allocator_.allocated()); > + assert(allocator_.allocated()); > assert(requests_.empty()); > > const auto bufferCount = config_->at(0).bufferCount; > @@ -132,9 +147,6 @@ void Capture::start() > for (const auto &cfg : *config_) { > Stream *stream = cfg.stream(); > > - int count = allocator_.allocate(stream); > - ASSERT_GE(count, 0) << "Failed to allocate buffers"; > - > const auto &buffers = allocator_.buffers(stream); > ASSERT_EQ(buffers.size(), bufferCount) << "Mismatching buffer count"; > > @@ -144,8 +156,6 @@ void Capture::start() > } > } > > - ASSERT_TRUE(allocator_.allocated()); > - > camera_->requestCompleted.connect(this, &Capture::requestComplete); > > ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; > @@ -161,6 +171,12 @@ void Capture::stop() > camera_->requestCompleted.disconnect(this); > > requests_.clear(); > +} > + > +void Capture::unconfigure() > +{ > + if (!config_ || !allocator_.allocated()) > + return; > > for (const auto &cfg : *config_) { > EXPECT_EQ(allocator_.free(cfg.stream()), 0) > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > index ea01c11c1..41ce4e5a6 100644 > --- a/src/apps/lc-compliance/helpers/capture.h > +++ b/src/apps/lc-compliance/helpers/capture.h > @@ -28,6 +28,7 @@ private: > > void start(); > void stop(); > + void unconfigure(); > > int queueRequest(libcamera::Request *request); > void requestComplete(libcamera::Request *request); > -- > 2.50.0 >
Hi 2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote: >> Most pipeline handlers release buffers when a camera is stopped. However, >> the raspberry pi pipelines do not do that. Hence they are incompatible > > > The RPi pipeline is the only one that, apart from UVC, that implements > PipelineHandler::releaseDevice(), called in the Camera::release() > path. > > The RPi implementation of ::releaseDevice() calls "data->freeBuffer()" > which ends up calling the RPiStream Stream::releaseBuffers() > which first releases buffers from the video device and then clears the > FrameBuffers created around them. > > Now if I get this right, this also clears bufferMap_ which is > populated through Stream::setExportedBuffers() in the > PipelineHandlerBase::exportFrameBuffers() call. > > To make this short, my understanding is that rpi releases all > allocated buffers on the video device at configure() and release() time. That is my understanding as well. > > I don't think we specify anywhere how pipelines should behave, but > this could certainly save allocations/deallocation at every start/stop > cycle. > > >> with the current model and all "CaptureStartStop" tests fail because >> lc-compliance allocates buffers before starting and releases them after >> stopping. Fix this by allocating buffers after the camera is configured, >> and do not release them after stopping. > > So we now have two models at play: one where buffers allocated on the > video device through FrameBufferAllocator are released at stop() time > and one where they are released at configure()/release() time. > > I wonder, but that's a general question, not specifically on this > patch which does anyway implement the behaviour I'm about to describe: > what happens if a FrameBuffers created with FrameBufferAllocator > outlives the buffers allocated on the video device it wraps ? I don't think that can happen. A `FrameBuffer` object has a `SharedFD` in each of its `Plane`s, keeping the kernel resource alive. This matches the documentation of `V4L2VideoDevice::releaseBuffers()`: [...] Buffer exported by exportBuffers(), if any, are not affected. Regards, Barnabás Pőcze > > With this patch applied, FrameBuffers in the frame buffer allocator > are released only at ~Capture(), while when running this on, say, > rkisp1, the memory buffers on the video device are released at every stop(). > > I feel like I'm missing something obvious, otherwise things should go > booom quite fast on pipelines !rpi with this change in.. > >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++----- >> src/apps/lc-compliance/helpers/capture.h | 1 + >> 2 files changed, 23 insertions(+), 6 deletions(-) >> >> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp >> index d076e964a..df0ad00ac 100644 >> --- a/src/apps/lc-compliance/helpers/capture.cpp >> +++ b/src/apps/lc-compliance/helpers/capture.cpp >> @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr<Camera> camera) >> Capture::~Capture() >> { >> stop(); >> + unconfigure(); > > a little bikeshedding to make this review more fun: I would call this > freeBuffers() or something similar. > > Thanks > j > >> } >> >> void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) >> @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) >> ASSERT_EQ(config->validate(), CameraConfiguration::Valid); >> ASSERT_EQ(camera_->configure(config.get()), 0); >> >> + const auto bufferCount = config->at(0).bufferCount; >> + >> + for (const auto &cfg : *config) { >> + Stream *stream = cfg.stream(); >> + >> + ASSERT_GE(allocator_.allocate(stream), 0) >> + << "Failed to allocate buffers"; >> + >> + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount) >> + << "Mismatching buffer count"; >> + } >> + >> + ASSERT_TRUE(allocator_.allocated()); >> + >> config_ = std::move(config); >> } >> >> @@ -112,7 +127,7 @@ void Capture::start() >> { >> assert(config_); >> assert(!config_->empty()); >> - assert(!allocator_.allocated()); >> + assert(allocator_.allocated()); >> assert(requests_.empty()); >> >> const auto bufferCount = config_->at(0).bufferCount; >> @@ -132,9 +147,6 @@ void Capture::start() >> for (const auto &cfg : *config_) { >> Stream *stream = cfg.stream(); >> >> - int count = allocator_.allocate(stream); >> - ASSERT_GE(count, 0) << "Failed to allocate buffers"; >> - >> const auto &buffers = allocator_.buffers(stream); >> ASSERT_EQ(buffers.size(), bufferCount) << "Mismatching buffer count"; >> >> @@ -144,8 +156,6 @@ void Capture::start() >> } >> } >> >> - ASSERT_TRUE(allocator_.allocated()); >> - >> camera_->requestCompleted.connect(this, &Capture::requestComplete); >> >> ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; >> @@ -161,6 +171,12 @@ void Capture::stop() >> camera_->requestCompleted.disconnect(this); >> >> requests_.clear(); >> +} >> + >> +void Capture::unconfigure() >> +{ >> + if (!config_ || !allocator_.allocated()) >> + return; >> >> for (const auto &cfg : *config_) { >> EXPECT_EQ(allocator_.free(cfg.stream()), 0) >> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h >> index ea01c11c1..41ce4e5a6 100644 >> --- a/src/apps/lc-compliance/helpers/capture.h >> +++ b/src/apps/lc-compliance/helpers/capture.h >> @@ -28,6 +28,7 @@ private: >> >> void start(); >> void stop(); >> + void unconfigure(); >> >> int queueRequest(libcamera::Request *request); >> void requestComplete(libcamera::Request *request); >> -- >> 2.50.0 >>
Hi Barnabás and Jacopo, On Mon, 3 Nov 2025 at 12:37, Barnabás Pőcze <barnabas.pocze@ideasonboard.com> wrote: > > Hi > > 2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote: > >> Most pipeline handlers release buffers when a camera is stopped. However, > >> the raspberry pi pipelines do not do that. Hence they are incompatible > > > > > > The RPi pipeline is the only one that, apart from UVC, that implements > > PipelineHandler::releaseDevice(), called in the Camera::release() > > path. > > > > The RPi implementation of ::releaseDevice() calls "data->freeBuffer()" > > which ends up calling the RPiStream Stream::releaseBuffers() > > which first releases buffers from the video device and then clears the > > FrameBuffers created around them. > > > > Now if I get this right, this also clears bufferMap_ which is > > populated through Stream::setExportedBuffers() in the > > PipelineHandlerBase::exportFrameBuffers() call. > > > > To make this short, my understanding is that rpi releases all > > allocated buffers on the video device at configure() and release() time. > > That is my understanding as well. This is correct and a bit of a pain point in our popeline handler. To summarise, our PH does not free internal buffer allocations on the stop() call, but instead waits until configure() and frees/re-allocates buffers if the configuration (resolution/pixelformat) changes. This is to avoid fragmentation on earlier Pi platforms that can only use a limited CMA carveout for framebuffers where we can very quickly run out of free contiguous space if we run many start/stop cycles. Many moons ago I suggested a parameter in stop() to allow the application to either request holding on to resources (like we do in our PH) or free them completely. That did not go anywhere, but perhaps it's time to revisit a change like that? Naush > > > > > > > I don't think we specify anywhere how pipelines should behave, but > > this could certainly save allocations/deallocation at every start/stop > > cycle. > > > > > >> with the current model and all "CaptureStartStop" tests fail because > >> lc-compliance allocates buffers before starting and releases them after > >> stopping. Fix this by allocating buffers after the camera is configured, > >> and do not release them after stopping. > > > > So we now have two models at play: one where buffers allocated on the > > video device through FrameBufferAllocator are released at stop() time > > and one where they are released at configure()/release() time. > > > > I wonder, but that's a general question, not specifically on this > > patch which does anyway implement the behaviour I'm about to describe: > > what happens if a FrameBuffers created with FrameBufferAllocator > > outlives the buffers allocated on the video device it wraps ? > > I don't think that can happen. A `FrameBuffer` object has a `SharedFD` > in each of its `Plane`s, keeping the kernel resource alive. This matches > the documentation of `V4L2VideoDevice::releaseBuffers()`: > > [...] Buffer exported by exportBuffers(), if any, are not affected. > > > Regards, > Barnabás Pőcze > > > > > With this patch applied, FrameBuffers in the frame buffer allocator > > are released only at ~Capture(), while when running this on, say, > > rkisp1, the memory buffers on the video device are released at every stop(). > > > > I feel like I'm missing something obvious, otherwise things should go > > booom quite fast on pipelines !rpi with this change in.. > > > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++----- > >> src/apps/lc-compliance/helpers/capture.h | 1 + > >> 2 files changed, 23 insertions(+), 6 deletions(-) > >> > >> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > >> index d076e964a..df0ad00ac 100644 > >> --- a/src/apps/lc-compliance/helpers/capture.cpp > >> +++ b/src/apps/lc-compliance/helpers/capture.cpp > >> @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr<Camera> camera) > >> Capture::~Capture() > >> { > >> stop(); > >> + unconfigure(); > > > > a little bikeshedding to make this review more fun: I would call this > > freeBuffers() or something similar. > > > > Thanks > > j > > > >> } > >> > >> void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > >> @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > >> ASSERT_EQ(config->validate(), CameraConfiguration::Valid); > >> ASSERT_EQ(camera_->configure(config.get()), 0); > >> > >> + const auto bufferCount = config->at(0).bufferCount; > >> + > >> + for (const auto &cfg : *config) { > >> + Stream *stream = cfg.stream(); > >> + > >> + ASSERT_GE(allocator_.allocate(stream), 0) > >> + << "Failed to allocate buffers"; > >> + > >> + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount) > >> + << "Mismatching buffer count"; > >> + } > >> + > >> + ASSERT_TRUE(allocator_.allocated()); > >> + > >> config_ = std::move(config); > >> } > >> > >> @@ -112,7 +127,7 @@ void Capture::start() > >> { > >> assert(config_); > >> assert(!config_->empty()); > >> - assert(!allocator_.allocated()); > >> + assert(allocator_.allocated()); > >> assert(requests_.empty()); > >> > >> const auto bufferCount = config_->at(0).bufferCount; > >> @@ -132,9 +147,6 @@ void Capture::start() > >> for (const auto &cfg : *config_) { > >> Stream *stream = cfg.stream(); > >> > >> - int count = allocator_.allocate(stream); > >> - ASSERT_GE(count, 0) << "Failed to allocate buffers"; > >> - > >> const auto &buffers = allocator_.buffers(stream); > >> ASSERT_EQ(buffers.size(), bufferCount) << "Mismatching buffer count"; > >> > >> @@ -144,8 +156,6 @@ void Capture::start() > >> } > >> } > >> > >> - ASSERT_TRUE(allocator_.allocated()); > >> - > >> camera_->requestCompleted.connect(this, &Capture::requestComplete); > >> > >> ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; > >> @@ -161,6 +171,12 @@ void Capture::stop() > >> camera_->requestCompleted.disconnect(this); > >> > >> requests_.clear(); > >> +} > >> + > >> +void Capture::unconfigure() > >> +{ > >> + if (!config_ || !allocator_.allocated()) > >> + return; > >> > >> for (const auto &cfg : *config_) { > >> EXPECT_EQ(allocator_.free(cfg.stream()), 0) > >> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > >> index ea01c11c1..41ce4e5a6 100644 > >> --- a/src/apps/lc-compliance/helpers/capture.h > >> +++ b/src/apps/lc-compliance/helpers/capture.h > >> @@ -28,6 +28,7 @@ private: > >> > >> void start(); > >> void stop(); > >> + void unconfigure(); > >> > >> int queueRequest(libcamera::Request *request); > >> void requestComplete(libcamera::Request *request); > >> -- > >> 2.50.0 > >> >
Hi 2025. 11. 03. 13:46 keltezéssel, Naushir Patuck írta: > Hi Barnabás and Jacopo, > > On Mon, 3 Nov 2025 at 12:37, Barnabás Pőcze > <barnabas.pocze@ideasonboard.com> wrote: >> >> Hi >> >> 2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote: >>>> Most pipeline handlers release buffers when a camera is stopped. However, >>>> the raspberry pi pipelines do not do that. Hence they are incompatible >>> >>> >>> The RPi pipeline is the only one that, apart from UVC, that implements >>> PipelineHandler::releaseDevice(), called in the Camera::release() >>> path. >>> >>> The RPi implementation of ::releaseDevice() calls "data->freeBuffer()" >>> which ends up calling the RPiStream Stream::releaseBuffers() >>> which first releases buffers from the video device and then clears the >>> FrameBuffers created around them. >>> >>> Now if I get this right, this also clears bufferMap_ which is >>> populated through Stream::setExportedBuffers() in the >>> PipelineHandlerBase::exportFrameBuffers() call. >>> >>> To make this short, my understanding is that rpi releases all >>> allocated buffers on the video device at configure() and release() time. >> >> That is my understanding as well. > > This is correct and a bit of a pain point in our popeline handler. To > summarise, our PH does not free internal buffer allocations on the > stop() call, but instead waits until configure() and > frees/re-allocates buffers if the configuration > (resolution/pixelformat) changes. This is to avoid fragmentation on > earlier Pi platforms that can only use a limited CMA carveout for > framebuffers where we can very quickly run out of free contiguous > space if we run many start/stop cycles. > > Many moons ago I suggested a parameter in stop() to allow the > application to either request holding on to resources (like we do in > our PH) or free them completely. That did not go anywhere, but > perhaps it's time to revisit a change like that? As for the "user facing" buffers (i.e. ones from `Request::buffers()`), I think a pipeline handler must remove any references to them at `stop()` time because it should have no control over their lifetimes. If they wish, users can keep them alive simply by not destroying them (e.g. by calling `FrameBufferAllocator::free()`), which should avoid fragmentation. The above includes calling `V4L2VideoDevice::releaseBuffers()` (or similar) to remove any references from `V4L2VideoDevice::cache_` so as not to keep them alive longer than the user wishes. I believe this last part is one thing that the rpi pipeline is missing. Regards, Barnabás Pőcze > > Naush > >> >> >> >>> >>> I don't think we specify anywhere how pipelines should behave, but >>> this could certainly save allocations/deallocation at every start/stop >>> cycle. >>> >>> >>>> with the current model and all "CaptureStartStop" tests fail because >>>> lc-compliance allocates buffers before starting and releases them after >>>> stopping. Fix this by allocating buffers after the camera is configured, >>>> and do not release them after stopping. >>> >>> So we now have two models at play: one where buffers allocated on the >>> video device through FrameBufferAllocator are released at stop() time >>> and one where they are released at configure()/release() time. >>> >>> I wonder, but that's a general question, not specifically on this >>> patch which does anyway implement the behaviour I'm about to describe: >>> what happens if a FrameBuffers created with FrameBufferAllocator >>> outlives the buffers allocated on the video device it wraps ? >> >> I don't think that can happen. A `FrameBuffer` object has a `SharedFD` >> in each of its `Plane`s, keeping the kernel resource alive. This matches >> the documentation of `V4L2VideoDevice::releaseBuffers()`: >> >> [...] Buffer exported by exportBuffers(), if any, are not affected. >> >> >> Regards, >> Barnabás Pőcze >> >>> >>> With this patch applied, FrameBuffers in the frame buffer allocator >>> are released only at ~Capture(), while when running this on, say, >>> rkisp1, the memory buffers on the video device are released at every stop(). >>> >>> I feel like I'm missing something obvious, otherwise things should go >>> booom quite fast on pipelines !rpi with this change in.. >>> >>>> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++----- >>>> src/apps/lc-compliance/helpers/capture.h | 1 + >>>> 2 files changed, 23 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp >>>> index d076e964a..df0ad00ac 100644 >>>> --- a/src/apps/lc-compliance/helpers/capture.cpp >>>> +++ b/src/apps/lc-compliance/helpers/capture.cpp >>>> @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr<Camera> camera) >>>> Capture::~Capture() >>>> { >>>> stop(); >>>> + unconfigure(); >>> >>> a little bikeshedding to make this review more fun: I would call this >>> freeBuffers() or something similar. >>> >>> Thanks >>> j >>> >>>> } >>>> >>>> void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) >>>> @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) >>>> ASSERT_EQ(config->validate(), CameraConfiguration::Valid); >>>> ASSERT_EQ(camera_->configure(config.get()), 0); >>>> >>>> + const auto bufferCount = config->at(0).bufferCount; >>>> + >>>> + for (const auto &cfg : *config) { >>>> + Stream *stream = cfg.stream(); >>>> + >>>> + ASSERT_GE(allocator_.allocate(stream), 0) >>>> + << "Failed to allocate buffers"; >>>> + >>>> + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount) >>>> + << "Mismatching buffer count"; >>>> + } >>>> + >>>> + ASSERT_TRUE(allocator_.allocated()); >>>> + >>>> config_ = std::move(config); >>>> } >>>> >>>> @@ -112,7 +127,7 @@ void Capture::start() >>>> { >>>> assert(config_); >>>> assert(!config_->empty()); >>>> - assert(!allocator_.allocated()); >>>> + assert(allocator_.allocated()); >>>> assert(requests_.empty()); >>>> >>>> const auto bufferCount = config_->at(0).bufferCount; >>>> @@ -132,9 +147,6 @@ void Capture::start() >>>> for (const auto &cfg : *config_) { >>>> Stream *stream = cfg.stream(); >>>> >>>> - int count = allocator_.allocate(stream); >>>> - ASSERT_GE(count, 0) << "Failed to allocate buffers"; >>>> - >>>> const auto &buffers = allocator_.buffers(stream); >>>> ASSERT_EQ(buffers.size(), bufferCount) << "Mismatching buffer count"; >>>> >>>> @@ -144,8 +156,6 @@ void Capture::start() >>>> } >>>> } >>>> >>>> - ASSERT_TRUE(allocator_.allocated()); >>>> - >>>> camera_->requestCompleted.connect(this, &Capture::requestComplete); >>>> >>>> ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; >>>> @@ -161,6 +171,12 @@ void Capture::stop() >>>> camera_->requestCompleted.disconnect(this); >>>> >>>> requests_.clear(); >>>> +} >>>> + >>>> +void Capture::unconfigure() >>>> +{ >>>> + if (!config_ || !allocator_.allocated()) >>>> + return; >>>> >>>> for (const auto &cfg : *config_) { >>>> EXPECT_EQ(allocator_.free(cfg.stream()), 0) >>>> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h >>>> index ea01c11c1..41ce4e5a6 100644 >>>> --- a/src/apps/lc-compliance/helpers/capture.h >>>> +++ b/src/apps/lc-compliance/helpers/capture.h >>>> @@ -28,6 +28,7 @@ private: >>>> >>>> void start(); >>>> void stop(); >>>> + void unconfigure(); >>>> >>>> int queueRequest(libcamera::Request *request); >>>> void requestComplete(libcamera::Request *request); >>>> -- >>>> 2.50.0 >>>> >>
Hi Barnabás, On Mon, 3 Nov 2025 at 13:16, Barnabás Pőcze <barnabas.pocze@ideasonboard.com> wrote: > > Hi > > 2025. 11. 03. 13:46 keltezéssel, Naushir Patuck írta: > > Hi Barnabás and Jacopo, > > > > On Mon, 3 Nov 2025 at 12:37, Barnabás Pőcze > > <barnabas.pocze@ideasonboard.com> wrote: > >> > >> Hi > >> > >> 2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta: > >>> Hi Barnabás > >>> > >>> On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote: > >>>> Most pipeline handlers release buffers when a camera is stopped. However, > >>>> the raspberry pi pipelines do not do that. Hence they are incompatible > >>> > >>> > >>> The RPi pipeline is the only one that, apart from UVC, that implements > >>> PipelineHandler::releaseDevice(), called in the Camera::release() > >>> path. > >>> > >>> The RPi implementation of ::releaseDevice() calls "data->freeBuffer()" > >>> which ends up calling the RPiStream Stream::releaseBuffers() > >>> which first releases buffers from the video device and then clears the > >>> FrameBuffers created around them. > >>> > >>> Now if I get this right, this also clears bufferMap_ which is > >>> populated through Stream::setExportedBuffers() in the > >>> PipelineHandlerBase::exportFrameBuffers() call. > >>> > >>> To make this short, my understanding is that rpi releases all > >>> allocated buffers on the video device at configure() and release() time. > >> > >> That is my understanding as well. > > > > This is correct and a bit of a pain point in our popeline handler. To > > summarise, our PH does not free internal buffer allocations on the > > stop() call, but instead waits until configure() and > > frees/re-allocates buffers if the configuration > > (resolution/pixelformat) changes. This is to avoid fragmentation on > > earlier Pi platforms that can only use a limited CMA carveout for > > framebuffers where we can very quickly run out of free contiguous > > space if we run many start/stop cycles. > > > > Many moons ago I suggested a parameter in stop() to allow the > > application to either request holding on to resources (like we do in > > our PH) or free them completely. That did not go anywhere, but > > perhaps it's time to revisit a change like that? > > As for the "user facing" buffers (i.e. ones from `Request::buffers()`), I think > a pipeline handler must remove any references to them at `stop()` time because > it should have no control over their lifetimes. If they wish, users can keep them > alive simply by not destroying them (e.g. by calling `FrameBufferAllocator::free()`), > which should avoid fragmentation. > > The above includes calling `V4L2VideoDevice::releaseBuffers()` (or similar) to > remove any references from `V4L2VideoDevice::cache_` so as not to keep them > alive longer than the user wishes. > > I believe this last part is one thing that the rpi pipeline is missing. Won't calling V4L2VideoDevice::releaseBuffers() also release buffers allocated through VIDIOC_REQBUFS? This is crucial, as we have a mix of buffers in use, some allocated this way for internal pipeline handler only use, and some allocated through (typically) the CMA heap or system heap for framebuffers exported to the application. We could (perhap should?) switch our pipeline handler to never allocate through V4L2VideoDevice::allocateBuffers() and have allocations managed by the pipeline handler exclusively. It's been a while since I've looked into this so apologies if the above is wrong! Naush > > > Regards, > Barnabás Pőcze > > > > > Naush > > > >> > >> > >> > >>> > >>> I don't think we specify anywhere how pipelines should behave, but > >>> this could certainly save allocations/deallocation at every start/stop > >>> cycle. > >>> > >>> > >>>> with the current model and all "CaptureStartStop" tests fail because > >>>> lc-compliance allocates buffers before starting and releases them after > >>>> stopping. Fix this by allocating buffers after the camera is configured, > >>>> and do not release them after stopping. > >>> > >>> So we now have two models at play: one where buffers allocated on the > >>> video device through FrameBufferAllocator are released at stop() time > >>> and one where they are released at configure()/release() time. > >>> > >>> I wonder, but that's a general question, not specifically on this > >>> patch which does anyway implement the behaviour I'm about to describe: > >>> what happens if a FrameBuffers created with FrameBufferAllocator > >>> outlives the buffers allocated on the video device it wraps ? > >> > >> I don't think that can happen. A `FrameBuffer` object has a `SharedFD` > >> in each of its `Plane`s, keeping the kernel resource alive. This matches > >> the documentation of `V4L2VideoDevice::releaseBuffers()`: > >> > >> [...] Buffer exported by exportBuffers(), if any, are not affected. > >> > >> > >> Regards, > >> Barnabás Pőcze > >> > >>> > >>> With this patch applied, FrameBuffers in the frame buffer allocator > >>> are released only at ~Capture(), while when running this on, say, > >>> rkisp1, the memory buffers on the video device are released at every stop(). > >>> > >>> I feel like I'm missing something obvious, otherwise things should go > >>> booom quite fast on pipelines !rpi with this change in.. > >>> > >>>> > >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>> --- > >>>> src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++----- > >>>> src/apps/lc-compliance/helpers/capture.h | 1 + > >>>> 2 files changed, 23 insertions(+), 6 deletions(-) > >>>> > >>>> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > >>>> index d076e964a..df0ad00ac 100644 > >>>> --- a/src/apps/lc-compliance/helpers/capture.cpp > >>>> +++ b/src/apps/lc-compliance/helpers/capture.cpp > >>>> @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr<Camera> camera) > >>>> Capture::~Capture() > >>>> { > >>>> stop(); > >>>> + unconfigure(); > >>> > >>> a little bikeshedding to make this review more fun: I would call this > >>> freeBuffers() or something similar. > >>> > >>> Thanks > >>> j > >>> > >>>> } > >>>> > >>>> void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > >>>> @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > >>>> ASSERT_EQ(config->validate(), CameraConfiguration::Valid); > >>>> ASSERT_EQ(camera_->configure(config.get()), 0); > >>>> > >>>> + const auto bufferCount = config->at(0).bufferCount; > >>>> + > >>>> + for (const auto &cfg : *config) { > >>>> + Stream *stream = cfg.stream(); > >>>> + > >>>> + ASSERT_GE(allocator_.allocate(stream), 0) > >>>> + << "Failed to allocate buffers"; > >>>> + > >>>> + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount) > >>>> + << "Mismatching buffer count"; > >>>> + } > >>>> + > >>>> + ASSERT_TRUE(allocator_.allocated()); > >>>> + > >>>> config_ = std::move(config); > >>>> } > >>>> > >>>> @@ -112,7 +127,7 @@ void Capture::start() > >>>> { > >>>> assert(config_); > >>>> assert(!config_->empty()); > >>>> - assert(!allocator_.allocated()); > >>>> + assert(allocator_.allocated()); > >>>> assert(requests_.empty()); > >>>> > >>>> const auto bufferCount = config_->at(0).bufferCount; > >>>> @@ -132,9 +147,6 @@ void Capture::start() > >>>> for (const auto &cfg : *config_) { > >>>> Stream *stream = cfg.stream(); > >>>> > >>>> - int count = allocator_.allocate(stream); > >>>> - ASSERT_GE(count, 0) << "Failed to allocate buffers"; > >>>> - > >>>> const auto &buffers = allocator_.buffers(stream); > >>>> ASSERT_EQ(buffers.size(), bufferCount) << "Mismatching buffer count"; > >>>> > >>>> @@ -144,8 +156,6 @@ void Capture::start() > >>>> } > >>>> } > >>>> > >>>> - ASSERT_TRUE(allocator_.allocated()); > >>>> - > >>>> camera_->requestCompleted.connect(this, &Capture::requestComplete); > >>>> > >>>> ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; > >>>> @@ -161,6 +171,12 @@ void Capture::stop() > >>>> camera_->requestCompleted.disconnect(this); > >>>> > >>>> requests_.clear(); > >>>> +} > >>>> + > >>>> +void Capture::unconfigure() > >>>> +{ > >>>> + if (!config_ || !allocator_.allocated()) > >>>> + return; > >>>> > >>>> for (const auto &cfg : *config_) { > >>>> EXPECT_EQ(allocator_.free(cfg.stream()), 0) > >>>> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > >>>> index ea01c11c1..41ce4e5a6 100644 > >>>> --- a/src/apps/lc-compliance/helpers/capture.h > >>>> +++ b/src/apps/lc-compliance/helpers/capture.h > >>>> @@ -28,6 +28,7 @@ private: > >>>> > >>>> void start(); > >>>> void stop(); > >>>> + void unconfigure(); > >>>> > >>>> int queueRequest(libcamera::Request *request); > >>>> void requestComplete(libcamera::Request *request); > >>>> -- > >>>> 2.50.0 > >>>> > >> >
2025. 11. 03. 14:28 keltezéssel, Naushir Patuck írta: > Hi Barnabás, > > On Mon, 3 Nov 2025 at 13:16, Barnabás Pőcze > <barnabas.pocze@ideasonboard.com> wrote: >> >> Hi >> >> 2025. 11. 03. 13:46 keltezéssel, Naushir Patuck írta: >>> Hi Barnabás and Jacopo, >>> >>> On Mon, 3 Nov 2025 at 12:37, Barnabás Pőcze >>> <barnabas.pocze@ideasonboard.com> wrote: >>>> >>>> Hi >>>> >>>> 2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta: >>>>> Hi Barnabás >>>>> >>>>> On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote: >>>>>> Most pipeline handlers release buffers when a camera is stopped. However, >>>>>> the raspberry pi pipelines do not do that. Hence they are incompatible >>>>> >>>>> >>>>> The RPi pipeline is the only one that, apart from UVC, that implements >>>>> PipelineHandler::releaseDevice(), called in the Camera::release() >>>>> path. >>>>> >>>>> The RPi implementation of ::releaseDevice() calls "data->freeBuffer()" >>>>> which ends up calling the RPiStream Stream::releaseBuffers() >>>>> which first releases buffers from the video device and then clears the >>>>> FrameBuffers created around them. >>>>> >>>>> Now if I get this right, this also clears bufferMap_ which is >>>>> populated through Stream::setExportedBuffers() in the >>>>> PipelineHandlerBase::exportFrameBuffers() call. >>>>> >>>>> To make this short, my understanding is that rpi releases all >>>>> allocated buffers on the video device at configure() and release() time. >>>> >>>> That is my understanding as well. >>> >>> This is correct and a bit of a pain point in our popeline handler. To >>> summarise, our PH does not free internal buffer allocations on the >>> stop() call, but instead waits until configure() and >>> frees/re-allocates buffers if the configuration >>> (resolution/pixelformat) changes. This is to avoid fragmentation on >>> earlier Pi platforms that can only use a limited CMA carveout for >>> framebuffers where we can very quickly run out of free contiguous >>> space if we run many start/stop cycles. >>> >>> Many moons ago I suggested a parameter in stop() to allow the >>> application to either request holding on to resources (like we do in >>> our PH) or free them completely. That did not go anywhere, but >>> perhaps it's time to revisit a change like that? >> >> As for the "user facing" buffers (i.e. ones from `Request::buffers()`), I think >> a pipeline handler must remove any references to them at `stop()` time because >> it should have no control over their lifetimes. If they wish, users can keep them >> alive simply by not destroying them (e.g. by calling `FrameBufferAllocator::free()`), >> which should avoid fragmentation. >> >> The above includes calling `V4L2VideoDevice::releaseBuffers()` (or similar) to >> remove any references from `V4L2VideoDevice::cache_` so as not to keep them >> alive longer than the user wishes. >> >> I believe this last part is one thing that the rpi pipeline is missing. > > Won't calling V4L2VideoDevice::releaseBuffers() also release buffers > allocated through VIDIOC_REQBUFS? This is crucial, as we have a mix > of buffers in use, some allocated this way for internal pipeline > handler only use, and some allocated through (typically) the CMA heap > or system heap for framebuffers exported to the application. We could > (perhap should?) switch our pipeline handler to never allocate through > V4L2VideoDevice::allocateBuffers() and have allocations managed by the > pipeline handler exclusively. Sorry, I'm not familiar with what exactly the rpi parts do. But based on my understanding, it depends. My interpretation of https://docs.kernel.org/userspace-api/media/v4l/vidioc-reqbufs.html says that VIDIOC_REQBUFS only really allocates memory if V4L2_MEMORY_MMAP. So my understanding is that if `V4L2VideoDevice::allocateBuffers()` is not used, which does not seem to be the case for rpi, then there will only be real allocations when it does `V4L2VideoDevice::exportBuffers()`, and those buffers will be alive until the planes' `SharedFD`s are destroyed. So importBuffers()/releaseBuffers() can be used at start/stop time, and they won't do any buffer allocation. At least if I understand it correctly. Regards, Barnabás Pőcze > > It's been a while since I've looked into this so apologies if the > above is wrong! > > Naush > > >> >> >> Regards, >> Barnabás Pőcze >> >>> >>> Naush >>> >>>> >>>> >>>> >>>>> >>>>> I don't think we specify anywhere how pipelines should behave, but >>>>> this could certainly save allocations/deallocation at every start/stop >>>>> cycle. >>>>> >>>>> >>>>>> with the current model and all "CaptureStartStop" tests fail because >>>>>> lc-compliance allocates buffers before starting and releases them after >>>>>> stopping. Fix this by allocating buffers after the camera is configured, >>>>>> and do not release them after stopping. >>>>> >>>>> So we now have two models at play: one where buffers allocated on the >>>>> video device through FrameBufferAllocator are released at stop() time >>>>> and one where they are released at configure()/release() time. >>>>> >>>>> I wonder, but that's a general question, not specifically on this >>>>> patch which does anyway implement the behaviour I'm about to describe: >>>>> what happens if a FrameBuffers created with FrameBufferAllocator >>>>> outlives the buffers allocated on the video device it wraps ? >>>> >>>> I don't think that can happen. A `FrameBuffer` object has a `SharedFD` >>>> in each of its `Plane`s, keeping the kernel resource alive. This matches >>>> the documentation of `V4L2VideoDevice::releaseBuffers()`: >>>> >>>> [...] Buffer exported by exportBuffers(), if any, are not affected. >>>> >>>> >>>> Regards, >>>> Barnabás Pőcze >>>> >>>>> >>>>> With this patch applied, FrameBuffers in the frame buffer allocator >>>>> are released only at ~Capture(), while when running this on, say, >>>>> rkisp1, the memory buffers on the video device are released at every stop(). >>>>> >>>>> I feel like I'm missing something obvious, otherwise things should go >>>>> booom quite fast on pipelines !rpi with this change in.. >>>>> >>>>>> >>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>>> --- >>>>>> src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++----- >>>>>> src/apps/lc-compliance/helpers/capture.h | 1 + >>>>>> 2 files changed, 23 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp >>>>>> index d076e964a..df0ad00ac 100644 >>>>>> --- a/src/apps/lc-compliance/helpers/capture.cpp >>>>>> +++ b/src/apps/lc-compliance/helpers/capture.cpp >>>>>> @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr<Camera> camera) >>>>>> Capture::~Capture() >>>>>> { >>>>>> stop(); >>>>>> + unconfigure(); >>>>> >>>>> a little bikeshedding to make this review more fun: I would call this >>>>> freeBuffers() or something similar. >>>>> >>>>> Thanks >>>>> j >>>>> >>>>>> } >>>>>> >>>>>> void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) >>>>>> @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) >>>>>> ASSERT_EQ(config->validate(), CameraConfiguration::Valid); >>>>>> ASSERT_EQ(camera_->configure(config.get()), 0); >>>>>> >>>>>> + const auto bufferCount = config->at(0).bufferCount; >>>>>> + >>>>>> + for (const auto &cfg : *config) { >>>>>> + Stream *stream = cfg.stream(); >>>>>> + >>>>>> + ASSERT_GE(allocator_.allocate(stream), 0) >>>>>> + << "Failed to allocate buffers"; >>>>>> + >>>>>> + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount) >>>>>> + << "Mismatching buffer count"; >>>>>> + } >>>>>> + >>>>>> + ASSERT_TRUE(allocator_.allocated()); >>>>>> + >>>>>> config_ = std::move(config); >>>>>> } >>>>>> >>>>>> @@ -112,7 +127,7 @@ void Capture::start() >>>>>> { >>>>>> assert(config_); >>>>>> assert(!config_->empty()); >>>>>> - assert(!allocator_.allocated()); >>>>>> + assert(allocator_.allocated()); >>>>>> assert(requests_.empty()); >>>>>> >>>>>> const auto bufferCount = config_->at(0).bufferCount; >>>>>> @@ -132,9 +147,6 @@ void Capture::start() >>>>>> for (const auto &cfg : *config_) { >>>>>> Stream *stream = cfg.stream(); >>>>>> >>>>>> - int count = allocator_.allocate(stream); >>>>>> - ASSERT_GE(count, 0) << "Failed to allocate buffers"; >>>>>> - >>>>>> const auto &buffers = allocator_.buffers(stream); >>>>>> ASSERT_EQ(buffers.size(), bufferCount) << "Mismatching buffer count"; >>>>>> >>>>>> @@ -144,8 +156,6 @@ void Capture::start() >>>>>> } >>>>>> } >>>>>> >>>>>> - ASSERT_TRUE(allocator_.allocated()); >>>>>> - >>>>>> camera_->requestCompleted.connect(this, &Capture::requestComplete); >>>>>> >>>>>> ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; >>>>>> @@ -161,6 +171,12 @@ void Capture::stop() >>>>>> camera_->requestCompleted.disconnect(this); >>>>>> >>>>>> requests_.clear(); >>>>>> +} >>>>>> + >>>>>> +void Capture::unconfigure() >>>>>> +{ >>>>>> + if (!config_ || !allocator_.allocated()) >>>>>> + return; >>>>>> >>>>>> for (const auto &cfg : *config_) { >>>>>> EXPECT_EQ(allocator_.free(cfg.stream()), 0) >>>>>> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h >>>>>> index ea01c11c1..41ce4e5a6 100644 >>>>>> --- a/src/apps/lc-compliance/helpers/capture.h >>>>>> +++ b/src/apps/lc-compliance/helpers/capture.h >>>>>> @@ -28,6 +28,7 @@ private: >>>>>> >>>>>> void start(); >>>>>> void stop(); >>>>>> + void unconfigure(); >>>>>> >>>>>> int queueRequest(libcamera::Request *request); >>>>>> void requestComplete(libcamera::Request *request); >>>>>> -- >>>>>> 2.50.0 >>>>>> >>>> >>
Hi Barnabás On Mon, Nov 03, 2025 at 01:37:39PM +0100, Barnabás Pőcze wrote: > Hi > > 2025. 11. 01. 18:22 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Tue, Jun 24, 2025 at 02:28:30PM +0200, Barnabás Pőcze wrote: > > > Most pipeline handlers release buffers when a camera is stopped. However, > > > the raspberry pi pipelines do not do that. Hence they are incompatible > > > > > > The RPi pipeline is the only one that, apart from UVC, that implements > > PipelineHandler::releaseDevice(), called in the Camera::release() > > path. > > > > The RPi implementation of ::releaseDevice() calls "data->freeBuffer()" > > which ends up calling the RPiStream Stream::releaseBuffers() > > which first releases buffers from the video device and then clears the > > FrameBuffers created around them. > > > > Now if I get this right, this also clears bufferMap_ which is > > populated through Stream::setExportedBuffers() in the > > PipelineHandlerBase::exportFrameBuffers() call. > > > > To make this short, my understanding is that rpi releases all > > allocated buffers on the video device at configure() and release() time. > > That is my understanding as well. > > > > > > I don't think we specify anywhere how pipelines should behave, but > > this could certainly save allocations/deallocation at every start/stop > > cycle. > > > > > > > with the current model and all "CaptureStartStop" tests fail because > > > lc-compliance allocates buffers before starting and releases them after > > > stopping. Fix this by allocating buffers after the camera is configured, > > > and do not release them after stopping. > > > > So we now have two models at play: one where buffers allocated on the > > video device through FrameBufferAllocator are released at stop() time > > and one where they are released at configure()/release() time. > > > > I wonder, but that's a general question, not specifically on this > > patch which does anyway implement the behaviour I'm about to describe: > > what happens if a FrameBuffers created with FrameBufferAllocator > > outlives the buffers allocated on the video device it wraps ? > > I don't think that can happen. A `FrameBuffer` object has a `SharedFD` > in each of its `Plane`s, keeping the kernel resource alive. This matches > the documentation of `V4L2VideoDevice::releaseBuffers()`: > > [...] Buffer exported by exportBuffers(), if any, are not affected. My question was a bit of nonsense, as to implement exportBuffers() we use V4L2 buffer orphaning. According to the V4L2 REQBUFS ioctl documentation If ``V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS`` is set, then these buffers are orphaned and will be freed when they are unmapped or when the exported DMABUF fds are closed. A ``count`` value of zero frees or orphans all buffers, after aborting or finishing any DMA in progress, an implicit :ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>`. so, yeah, there is no risk of an "exported" buffer to outlive the memory associated with it in the video device, because that memory has been orphaned and its lifetime is now bound to the one of the dma-buf associated with it. This flips the way I interpreted this patch. I thought this was about avoiding an early release of the exported buffers, but this is actually about avoiding allocation from the FrameBufferAllocator at every start() as on RPi it results in ERROR V4L2 v4l2_videodevice.cpp:1434 /dev/video24[19:cap]: Buffers already allocated triggered by the fact that the V4L2VideoDevice cache has not been reset by a call to V4L2VideoDevice::releaseBuffers(), which doesn't happen at stop() as it happens for all other pipelines, As said, I think the what the RPi pipeline does is legit, and we make no restrictions on how pipelines should handle internal buffers allocations but at the same time it requires a bit of a more "controlled" handling of exported buffers as they can't be released freely at every start/stop sequence but only after the video device where they have been exported from has been reset with V4L2VideoDevice::releaseBuffers(). So yeah, unless we formalize more strict requirements on how the pipeline handlers should handle their buffers, I guess this patch is required to support RPi. This also show that FrameBufferAllocator is an hack but we knew that and we all hope we can get rid of it sooner or later once Linux gets a unified memory allocator. To summarize: Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks! j > > > Regards, > Barnabás Pőcze > > > > > With this patch applied, FrameBuffers in the frame buffer allocator > > are released only at ~Capture(), while when running this on, say, > > rkisp1, the memory buffers on the video device are released at every stop(). > > > > I feel like I'm missing something obvious, otherwise things should go > > booom quite fast on pipelines !rpi with this change in.. > > > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++----- > > > src/apps/lc-compliance/helpers/capture.h | 1 + > > > 2 files changed, 23 insertions(+), 6 deletions(-) > > > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > > > index d076e964a..df0ad00ac 100644 > > > --- a/src/apps/lc-compliance/helpers/capture.cpp > > > +++ b/src/apps/lc-compliance/helpers/capture.cpp > > > @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr<Camera> camera) > > > Capture::~Capture() > > > { > > > stop(); > > > + unconfigure(); > > > > a little bikeshedding to make this review more fun: I would call this > > freeBuffers() or something similar. > > > > Thanks > > j > > > > > } > > > > > > void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > > > @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > > > ASSERT_EQ(config->validate(), CameraConfiguration::Valid); > > > ASSERT_EQ(camera_->configure(config.get()), 0); > > > > > > + const auto bufferCount = config->at(0).bufferCount; > > > + > > > + for (const auto &cfg : *config) { > > > + Stream *stream = cfg.stream(); > > > + > > > + ASSERT_GE(allocator_.allocate(stream), 0) > > > + << "Failed to allocate buffers"; > > > + > > > + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount) > > > + << "Mismatching buffer count"; > > > + } > > > + > > > + ASSERT_TRUE(allocator_.allocated()); > > > + > > > config_ = std::move(config); > > > } > > > > > > @@ -112,7 +127,7 @@ void Capture::start() > > > { > > > assert(config_); > > > assert(!config_->empty()); > > > - assert(!allocator_.allocated()); > > > + assert(allocator_.allocated()); > > > assert(requests_.empty()); > > > > > > const auto bufferCount = config_->at(0).bufferCount; > > > @@ -132,9 +147,6 @@ void Capture::start() > > > for (const auto &cfg : *config_) { > > > Stream *stream = cfg.stream(); > > > > > > - int count = allocator_.allocate(stream); > > > - ASSERT_GE(count, 0) << "Failed to allocate buffers"; > > > - > > > const auto &buffers = allocator_.buffers(stream); > > > ASSERT_EQ(buffers.size(), bufferCount) << "Mismatching buffer count"; > > > > > > @@ -144,8 +156,6 @@ void Capture::start() > > > } > > > } > > > > > > - ASSERT_TRUE(allocator_.allocated()); > > > - > > > camera_->requestCompleted.connect(this, &Capture::requestComplete); > > > > > > ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; > > > @@ -161,6 +171,12 @@ void Capture::stop() > > > camera_->requestCompleted.disconnect(this); > > > > > > requests_.clear(); > > > +} > > > + > > > +void Capture::unconfigure() > > > +{ > > > + if (!config_ || !allocator_.allocated()) > > > + return; > > > > > > for (const auto &cfg : *config_) { > > > EXPECT_EQ(allocator_.free(cfg.stream()), 0) > > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > > > index ea01c11c1..41ce4e5a6 100644 > > > --- a/src/apps/lc-compliance/helpers/capture.h > > > +++ b/src/apps/lc-compliance/helpers/capture.h > > > @@ -28,6 +28,7 @@ private: > > > > > > void start(); > > > void stop(); > > > + void unconfigure(); > > > > > > int queueRequest(libcamera::Request *request); > > > void requestComplete(libcamera::Request *request); > > > -- > > > 2.50.0 > > > >
diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp index d076e964a..df0ad00ac 100644 --- a/src/apps/lc-compliance/helpers/capture.cpp +++ b/src/apps/lc-compliance/helpers/capture.cpp @@ -21,6 +21,7 @@ Capture::Capture(std::shared_ptr<Camera> camera) Capture::~Capture() { stop(); + unconfigure(); } void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) @@ -50,6 +51,20 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) ASSERT_EQ(config->validate(), CameraConfiguration::Valid); ASSERT_EQ(camera_->configure(config.get()), 0); + const auto bufferCount = config->at(0).bufferCount; + + for (const auto &cfg : *config) { + Stream *stream = cfg.stream(); + + ASSERT_GE(allocator_.allocate(stream), 0) + << "Failed to allocate buffers"; + + ASSERT_EQ(allocator_.buffers(stream).size(), bufferCount) + << "Mismatching buffer count"; + } + + ASSERT_TRUE(allocator_.allocated()); + config_ = std::move(config); } @@ -112,7 +127,7 @@ void Capture::start() { assert(config_); assert(!config_->empty()); - assert(!allocator_.allocated()); + assert(allocator_.allocated()); assert(requests_.empty()); const auto bufferCount = config_->at(0).bufferCount; @@ -132,9 +147,6 @@ void Capture::start() for (const auto &cfg : *config_) { Stream *stream = cfg.stream(); - int count = allocator_.allocate(stream); - ASSERT_GE(count, 0) << "Failed to allocate buffers"; - const auto &buffers = allocator_.buffers(stream); ASSERT_EQ(buffers.size(), bufferCount) << "Mismatching buffer count"; @@ -144,8 +156,6 @@ void Capture::start() } } - ASSERT_TRUE(allocator_.allocated()); - camera_->requestCompleted.connect(this, &Capture::requestComplete); ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; @@ -161,6 +171,12 @@ void Capture::stop() camera_->requestCompleted.disconnect(this); requests_.clear(); +} + +void Capture::unconfigure() +{ + if (!config_ || !allocator_.allocated()) + return; for (const auto &cfg : *config_) { EXPECT_EQ(allocator_.free(cfg.stream()), 0) diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h index ea01c11c1..41ce4e5a6 100644 --- a/src/apps/lc-compliance/helpers/capture.h +++ b/src/apps/lc-compliance/helpers/capture.h @@ -28,6 +28,7 @@ private: void start(); void stop(); + void unconfigure(); int queueRequest(libcamera::Request *request); void requestComplete(libcamera::Request *request);
Most pipeline handlers release buffers when a camera is stopped. However, the raspberry pi pipelines do not do that. Hence they are incompatible with the current model and all "CaptureStartStop" tests fail because lc-compliance allocates buffers before starting and releases them after stopping. Fix this by allocating buffers after the camera is configured, and do not release them after stopping. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/apps/lc-compliance/helpers/capture.cpp | 28 +++++++++++++++++----- src/apps/lc-compliance/helpers/capture.h | 1 + 2 files changed, 23 insertions(+), 6 deletions(-)