| Message ID | 20251210131302.81887-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote: > In order to clear the V4L2VideoDevice cache, we must call > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this > without releasing the allocated buffers, we have to rework the internal > buffer allocation logic. > > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers() > from within the RPi::Stream class. Instead, move these calls to the > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear > the V4L2VideoDevice cache unconditionally on stop without de-allocating > the internal buffers. > > The loigc Stream::clearBuffers() can be then moved into releaseBuffers() logic > which will actually de-allocate the internal buffers when required. > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265 > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++- > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++------------- > .../pipeline/rpi/common/rpi_stream.h | 1 - > 3 files changed, 14 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 2b61b5d241c5..aa0af367d301 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera) > data->state_ = CameraData::State::Stopped; > data->platformStop(); > > - for (auto const stream : data->streams_) > + for (auto const stream : data->streams_) { > stream->dev()->streamOff(); > + stream->dev()->releaseBuffers(); > + } > > /* Disable SOF event generation. */ > data->frontendDevice()->setFrameStartEnabled(false); > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) > int ret; > > for (auto const stream : data->streams_) { > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME); > + if (ret < 0) > + return ret; > + I still have troubles understanding the buffer allocation policy of your pipeline, but that's certainly me. It seems to me you now call V4L2VideoDevice::importBuffers() on all your streams (and you have one stream for each video device in the media graph, it seems to me) While before this change, Stream::prepareBuffers() (where importBuffers() was used to be called) was only called on some devices and with a variable number of buffers to import (see PipelineHandlerPiSP::prepareBuffers()). Is this intended, or have I missed something ? > if (stream->getFlags() & StreamFlag::External) > continue; > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > index e73f4b7d31af..f420400dfe18 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > @@ -12,9 +12,6 @@ > > #include <libcamera/base/log.h> > > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */ > -static constexpr unsigned int maxV4L2BufferCount = 32; > - > namespace libcamera { > > LOG_DEFINE_CATEGORY(RPISTREAM) > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer) > > int Stream::allocateBuffers(unsigned int count) > { > - int ret; > + int ret = 0; > > if (!(flags_ & StreamFlag::ImportOnly)) { > /* Export some frame buffers for internal use. */ > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count) > resetBuffers(); > } > > - return dev_->importBuffers(maxV4L2BufferCount); > + return ret; > } > > int Stream::queueBuffer(FrameBuffer *buffer) > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers() > > void Stream::releaseBuffers() > { > - dev_->releaseBuffers(); > - clearBuffers(); > + availableBuffers_ = std::queue<FrameBuffer *>{}; > + requestBuffers_ = std::queue<FrameBuffer *>{}; > + internalBuffers_.clear(); > + bufferMap_.clear(); > + id_ = 0; > } > > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > std::forward_as_tuple(buffer, false)); > } > > -void Stream::clearBuffers() > -{ > - availableBuffers_ = std::queue<FrameBuffer *>{}; > - requestBuffers_ = std::queue<FrameBuffer *>{}; > - internalBuffers_.clear(); > - bufferMap_.clear(); > - id_ = 0; > -} > - > int Stream::queueToDevice(FrameBuffer *buffer) > { > LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > index c267447e5ab5..300a352a7d39 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > @@ -140,7 +140,6 @@ public: > > private: > void bufferEmplace(unsigned int id, FrameBuffer *buffer); > - void clearBuffers(); > int queueToDevice(FrameBuffer *buffer); > > StreamFlags flags_; > -- > 2.51.0 >
Hi Jacopo! On Fri, 12 Dec 2025 at 10:07, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote: > > In order to clear the V4L2VideoDevice cache, we must call > > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this > > without releasing the allocated buffers, we have to rework the internal > > buffer allocation logic. > > > > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers() > > from within the RPi::Stream class. Instead, move these calls to the > > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear > > the V4L2VideoDevice cache unconditionally on stop without de-allocating > > the internal buffers. > > > > The loigc Stream::clearBuffers() can be then moved into releaseBuffers() > > logic > > > which will actually de-allocate the internal buffers when required. > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265 > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++- > > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++------------- > > .../pipeline/rpi/common/rpi_stream.h | 1 - > > 3 files changed, 14 insertions(+), 18 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > index 2b61b5d241c5..aa0af367d301 100644 > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera) > > data->state_ = CameraData::State::Stopped; > > data->platformStop(); > > > > - for (auto const stream : data->streams_) > > + for (auto const stream : data->streams_) { > > stream->dev()->streamOff(); > > + stream->dev()->releaseBuffers(); > > + } > > > > /* Disable SOF event generation. */ > > data->frontendDevice()->setFrameStartEnabled(false); > > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) > > int ret; > > > > for (auto const stream : data->streams_) { > > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME); > > + if (ret < 0) > > + return ret; > > + > > I still have troubles understanding the buffer allocation policy of > your pipeline, but that's certainly me. > > It seems to me you now call V4L2VideoDevice::importBuffers() on all > your streams (and you have one stream for each video device in the > media graph, it seems to me) > > While before this change, Stream::prepareBuffers() (where > importBuffers() was used to be called) was only called on some devices > and with a variable number of buffers to import (see > PipelineHandlerPiSP::prepareBuffers()). > > Is this intended, or have I missed something ? I think the logic remains the same. On both cases, we call V4L2VideoDevice::importBuffers() on whichever device nodes (or streams) are enabled/active. The difference in behavior with this patch is that we now separate the calls to exportBuffers and importBuffers, and this allows us to keep the internal buffers still allocated when clearing the v4l2 cache. Does that make sense? Naush > > > > if (stream->getFlags() & StreamFlag::External) > > continue; > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > index e73f4b7d31af..f420400dfe18 100644 > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > @@ -12,9 +12,6 @@ > > > > #include <libcamera/base/log.h> > > > > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */ > > -static constexpr unsigned int maxV4L2BufferCount = 32; > > - > > namespace libcamera { > > > > LOG_DEFINE_CATEGORY(RPISTREAM) > > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer) > > > > int Stream::allocateBuffers(unsigned int count) > > { > > - int ret; > > + int ret = 0; > > > > if (!(flags_ & StreamFlag::ImportOnly)) { > > /* Export some frame buffers for internal use. */ > > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count) > > resetBuffers(); > > } > > > > - return dev_->importBuffers(maxV4L2BufferCount); > > + return ret; > > } > > > > int Stream::queueBuffer(FrameBuffer *buffer) > > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers() > > > > void Stream::releaseBuffers() > > { > > - dev_->releaseBuffers(); > > - clearBuffers(); > > + availableBuffers_ = std::queue<FrameBuffer *>{}; > > + requestBuffers_ = std::queue<FrameBuffer *>{}; > > + internalBuffers_.clear(); > > + bufferMap_.clear(); > > + id_ = 0; > > } > > > > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > > std::forward_as_tuple(buffer, false)); > > } > > > > -void Stream::clearBuffers() > > -{ > > - availableBuffers_ = std::queue<FrameBuffer *>{}; > > - requestBuffers_ = std::queue<FrameBuffer *>{}; > > - internalBuffers_.clear(); > > - bufferMap_.clear(); > > - id_ = 0; > > -} > > - > > int Stream::queueToDevice(FrameBuffer *buffer) > > { > > LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > index c267447e5ab5..300a352a7d39 100644 > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > @@ -140,7 +140,6 @@ public: > > > > private: > > void bufferEmplace(unsigned int id, FrameBuffer *buffer); > > - void clearBuffers(); > > int queueToDevice(FrameBuffer *buffer); > > > > StreamFlags flags_; > > -- > > 2.51.0 > >
Hi Naush On Fri, Dec 12, 2025 at 12:25:03PM +0000, Naushir Patuck wrote: > Hi Jacopo! > > On Fri, 12 Dec 2025 at 10:07, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Naush > > > > On Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote: > > > In order to clear the V4L2VideoDevice cache, we must call > > > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this > > > without releasing the allocated buffers, we have to rework the internal > > > buffer allocation logic. > > > > > > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers() > > > from within the RPi::Stream class. Instead, move these calls to the > > > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear > > > the V4L2VideoDevice cache unconditionally on stop without de-allocating > > > the internal buffers. > > > > > > The loigc Stream::clearBuffers() can be then moved into releaseBuffers() > > > > logic > > > > > which will actually de-allocate the internal buffers when required. > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265 > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++- > > > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++------------- > > > .../pipeline/rpi/common/rpi_stream.h | 1 - > > > 3 files changed, 14 insertions(+), 18 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > index 2b61b5d241c5..aa0af367d301 100644 > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera) > > > data->state_ = CameraData::State::Stopped; > > > data->platformStop(); > > > > > > - for (auto const stream : data->streams_) > > > + for (auto const stream : data->streams_) { > > > stream->dev()->streamOff(); > > > + stream->dev()->releaseBuffers(); > > > + } > > > > > > /* Disable SOF event generation. */ > > > data->frontendDevice()->setFrameStartEnabled(false); > > > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) > > > int ret; > > > > > > for (auto const stream : data->streams_) { > > > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME); > > > + if (ret < 0) > > > + return ret; > > > + > > > > I still have troubles understanding the buffer allocation policy of > > your pipeline, but that's certainly me. > > > > It seems to me you now call V4L2VideoDevice::importBuffers() on all > > your streams (and you have one stream for each video device in the > > media graph, it seems to me) > > > > While before this change, Stream::prepareBuffers() (where > > importBuffers() was used to be called) was only called on some devices > > and with a variable number of buffers to import (see > > PipelineHandlerPiSP::prepareBuffers()). > > > > Is this intended, or have I missed something ? > > I think the logic remains the same. On both cases, we call > V4L2VideoDevice::importBuffers() on whichever device nodes (or > streams) are enabled/active. The difference in behavior with this Ok, but why I see PipelineHandlerPiSP::prepareBuffers() cycling through streams and 1) Skipping some of them becaue they are not active 2) Setting a different number of buffers depending on the stream while I see PipelineHandlerBase::queueAllBuffers() importing the same number of buffers (VIDEO_MAX_FRAME) on all streams unconditionally ? > patch is that we now separate the calls to exportBuffers and > importBuffers, and this allows us to keep the internal buffers still > allocated when clearing the v4l2 cache. Does that make sense? > > Naush > > > > > > > > > if (stream->getFlags() & StreamFlag::External) > > > continue; > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > index e73f4b7d31af..f420400dfe18 100644 > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > @@ -12,9 +12,6 @@ > > > > > > #include <libcamera/base/log.h> > > > > > > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */ > > > -static constexpr unsigned int maxV4L2BufferCount = 32; > > > - > > > namespace libcamera { > > > > > > LOG_DEFINE_CATEGORY(RPISTREAM) > > > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer) > > > > > > int Stream::allocateBuffers(unsigned int count) > > > { > > > - int ret; > > > + int ret = 0; > > > > > > if (!(flags_ & StreamFlag::ImportOnly)) { > > > /* Export some frame buffers for internal use. */ > > > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count) > > > resetBuffers(); > > > } > > > > > > - return dev_->importBuffers(maxV4L2BufferCount); > > > + return ret; > > > } > > > > > > int Stream::queueBuffer(FrameBuffer *buffer) > > > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers() > > > > > > void Stream::releaseBuffers() > > > { > > > - dev_->releaseBuffers(); > > > - clearBuffers(); > > > + availableBuffers_ = std::queue<FrameBuffer *>{}; > > > + requestBuffers_ = std::queue<FrameBuffer *>{}; > > > + internalBuffers_.clear(); > > > + bufferMap_.clear(); > > > + id_ = 0; > > > } > > > > > > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > > > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > > > std::forward_as_tuple(buffer, false)); > > > } > > > > > > -void Stream::clearBuffers() > > > -{ > > > - availableBuffers_ = std::queue<FrameBuffer *>{}; > > > - requestBuffers_ = std::queue<FrameBuffer *>{}; > > > - internalBuffers_.clear(); > > > - bufferMap_.clear(); > > > - id_ = 0; > > > -} > > > - > > > int Stream::queueToDevice(FrameBuffer *buffer) > > > { > > > LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > index c267447e5ab5..300a352a7d39 100644 > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > @@ -140,7 +140,6 @@ public: > > > > > > private: > > > void bufferEmplace(unsigned int id, FrameBuffer *buffer); > > > - void clearBuffers(); > > > int queueToDevice(FrameBuffer *buffer); > > > > > > StreamFlags flags_; > > > -- > > > 2.51.0 > > >
Hi Jacopo, On Fri, 12 Dec 2025 at 12:35, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Fri, Dec 12, 2025 at 12:25:03PM +0000, Naushir Patuck wrote: > > Hi Jacopo! > > > > On Fri, 12 Dec 2025 at 10:07, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Naush > > > > > > On Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote: > > > > In order to clear the V4L2VideoDevice cache, we must call > > > > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this > > > > without releasing the allocated buffers, we have to rework the internal > > > > buffer allocation logic. > > > > > > > > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers() > > > > from within the RPi::Stream class. Instead, move these calls to the > > > > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear > > > > the V4L2VideoDevice cache unconditionally on stop without de-allocating > > > > the internal buffers. > > > > > > > > The loigc Stream::clearBuffers() can be then moved into releaseBuffers() > > > > > > logic > > > > > > > which will actually de-allocate the internal buffers when required. > > > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265 > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++- > > > > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++------------- > > > > .../pipeline/rpi/common/rpi_stream.h | 1 - > > > > 3 files changed, 14 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > index 2b61b5d241c5..aa0af367d301 100644 > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera) > > > > data->state_ = CameraData::State::Stopped; > > > > data->platformStop(); > > > > > > > > - for (auto const stream : data->streams_) > > > > + for (auto const stream : data->streams_) { > > > > stream->dev()->streamOff(); > > > > + stream->dev()->releaseBuffers(); > > > > + } > > > > > > > > /* Disable SOF event generation. */ > > > > data->frontendDevice()->setFrameStartEnabled(false); > > > > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) > > > > int ret; > > > > > > > > for (auto const stream : data->streams_) { > > > > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > > > I still have troubles understanding the buffer allocation policy of > > > your pipeline, but that's certainly me. > > > > > > It seems to me you now call V4L2VideoDevice::importBuffers() on all > > > your streams (and you have one stream for each video device in the > > > media graph, it seems to me) > > > > > > While before this change, Stream::prepareBuffers() (where > > > importBuffers() was used to be called) was only called on some devices > > > and with a variable number of buffers to import (see > > > PipelineHandlerPiSP::prepareBuffers()). > > > > > > Is this intended, or have I missed something ? > > > > I think the logic remains the same. On both cases, we call > > V4L2VideoDevice::importBuffers() on whichever device nodes (or > > streams) are enabled/active. The difference in behavior with this > > Ok, but why I see PipelineHandlerPiSP::prepareBuffers() cycling > through streams and > 1) Skipping some of them becaue they are not active > 2) Setting a different number of buffers depending on the stream > > while I see PipelineHandlerBase::queueAllBuffers() importing the same > number of buffers (VIDEO_MAX_FRAME) on all streams unconditionally ? I think they are still equivalent. In the old (existing) code: - in pipeilne->prepareBuffers() we cycle through all data->streams_ and choose the number of buffers to allocate for streams that need special handling. - Still in the pipeilne->prepareBuffers() loop, we call stream->prepareBuffers(count) for all streams, which does the allocation with exportBuffers(count) and then calls importBuffers(32). In the new code: - pipeilne->prepareBuffers() we cycle through all data->streams_ and choose the number of buffers to allocate in - Still in the pipeilne->prepareBuffers() loop, call stream->allocateBuffers(count), which does the allocation with exportBuffers(count). - in pipeline->queueAllBuffers() during startup we cycle through all data->streams_ and calls importBuffers(32). The 32 the semi-arbitrary constant in both cases. A long time ago we discussed what number to use, and we settled on VIDEO_MAX_FRAME. This is how the logic is *supposed* to be, but maybe I have messed up somewhere? :) > > > patch is that we now separate the calls to exportBuffers and > > importBuffers, and this allows us to keep the internal buffers still > > allocated when clearing the v4l2 cache. Does that make sense? > > > > Naush > > > > > > > > > > > > > > if (stream->getFlags() & StreamFlag::External) > > > > continue; > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > > index e73f4b7d31af..f420400dfe18 100644 > > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > > @@ -12,9 +12,6 @@ > > > > > > > > #include <libcamera/base/log.h> > > > > > > > > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */ > > > > -static constexpr unsigned int maxV4L2BufferCount = 32; > > > > - > > > > namespace libcamera { > > > > > > > > LOG_DEFINE_CATEGORY(RPISTREAM) > > > > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer) > > > > > > > > int Stream::allocateBuffers(unsigned int count) > > > > { > > > > - int ret; > > > > + int ret = 0; > > > > > > > > if (!(flags_ & StreamFlag::ImportOnly)) { > > > > /* Export some frame buffers for internal use. */ > > > > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count) > > > > resetBuffers(); > > > > } > > > > > > > > - return dev_->importBuffers(maxV4L2BufferCount); > > > > + return ret; > > > > } > > > > > > > > int Stream::queueBuffer(FrameBuffer *buffer) > > > > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers() > > > > > > > > void Stream::releaseBuffers() > > > > { > > > > - dev_->releaseBuffers(); > > > > - clearBuffers(); > > > > + availableBuffers_ = std::queue<FrameBuffer *>{}; > > > > + requestBuffers_ = std::queue<FrameBuffer *>{}; > > > > + internalBuffers_.clear(); > > > > + bufferMap_.clear(); > > > > + id_ = 0; > > > > } > > > > > > > > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > > > > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > > > > std::forward_as_tuple(buffer, false)); > > > > } > > > > > > > > -void Stream::clearBuffers() > > > > -{ > > > > - availableBuffers_ = std::queue<FrameBuffer *>{}; > > > > - requestBuffers_ = std::queue<FrameBuffer *>{}; > > > > - internalBuffers_.clear(); > > > > - bufferMap_.clear(); > > > > - id_ = 0; > > > > -} > > > > - > > > > int Stream::queueToDevice(FrameBuffer *buffer) > > > > { > > > > LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > index c267447e5ab5..300a352a7d39 100644 > > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > @@ -140,7 +140,6 @@ public: > > > > > > > > private: > > > > void bufferEmplace(unsigned int id, FrameBuffer *buffer); > > > > - void clearBuffers(); > > > > int queueToDevice(FrameBuffer *buffer); > > > > > > > > StreamFlags flags_; > > > > -- > > > > 2.51.0 > > > >
Hi Naush On Fri, Dec 12, 2025 at 12:47:55PM +0000, Naushir Patuck wrote: > Hi Jacopo, > > On Fri, 12 Dec 2025 at 12:35, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Naush > > > > On Fri, Dec 12, 2025 at 12:25:03PM +0000, Naushir Patuck wrote: > > > Hi Jacopo! > > > > > > On Fri, 12 Dec 2025 at 10:07, Jacopo Mondi > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > Hi Naush > > > > > > > > On Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote: > > > > > In order to clear the V4L2VideoDevice cache, we must call > > > > > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this > > > > > without releasing the allocated buffers, we have to rework the internal > > > > > buffer allocation logic. > > > > > > > > > > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers() > > > > > from within the RPi::Stream class. Instead, move these calls to the > > > > > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear > > > > > the V4L2VideoDevice cache unconditionally on stop without de-allocating > > > > > the internal buffers. > > > > > > > > > > The loigc Stream::clearBuffers() can be then moved into releaseBuffers() > > > > > > > > logic > > > > > > > > > which will actually de-allocate the internal buffers when required. > > > > > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265 > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > --- > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++- > > > > > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++------------- > > > > > .../pipeline/rpi/common/rpi_stream.h | 1 - > > > > > 3 files changed, 14 insertions(+), 18 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > > index 2b61b5d241c5..aa0af367d301 100644 > > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera) > > > > > data->state_ = CameraData::State::Stopped; > > > > > data->platformStop(); > > > > > > > > > > - for (auto const stream : data->streams_) > > > > > + for (auto const stream : data->streams_) { > > > > > stream->dev()->streamOff(); > > > > > + stream->dev()->releaseBuffers(); > > > > > + } > > > > > > > > > > /* Disable SOF event generation. */ > > > > > data->frontendDevice()->setFrameStartEnabled(false); > > > > > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) > > > > > int ret; > > > > > > > > > > for (auto const stream : data->streams_) { > > > > > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME); > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > > > > I still have troubles understanding the buffer allocation policy of > > > > your pipeline, but that's certainly me. > > > > > > > > It seems to me you now call V4L2VideoDevice::importBuffers() on all > > > > your streams (and you have one stream for each video device in the > > > > media graph, it seems to me) > > > > > > > > While before this change, Stream::prepareBuffers() (where > > > > importBuffers() was used to be called) was only called on some devices > > > > and with a variable number of buffers to import (see > > > > PipelineHandlerPiSP::prepareBuffers()). > > > > > > > > Is this intended, or have I missed something ? > > > > > > I think the logic remains the same. On both cases, we call > > > V4L2VideoDevice::importBuffers() on whichever device nodes (or > > > streams) are enabled/active. The difference in behavior with this > > > > Ok, but why I see PipelineHandlerPiSP::prepareBuffers() cycling > > through streams and > > 1) Skipping some of them becaue they are not active > > 2) Setting a different number of buffers depending on the stream > > > > while I see PipelineHandlerBase::queueAllBuffers() importing the same > > number of buffers (VIDEO_MAX_FRAME) on all streams unconditionally ? > > I think they are still equivalent. > > In the old (existing) code: > > - in pipeilne->prepareBuffers() we cycle through all data->streams_ > and choose the number of buffers to allocate for streams that need > special handling. > - Still in the pipeilne->prepareBuffers() loop, we call > stream->prepareBuffers(count) for all streams, which does the > allocation with exportBuffers(count) and then calls importBuffers(32). > > In the new code: > > - pipeilne->prepareBuffers() we cycle through all data->streams_ and > choose the number of buffers to allocate in > - Still in the pipeilne->prepareBuffers() loop, call > stream->allocateBuffers(count), which does the allocation with > exportBuffers(count). > - in pipeline->queueAllBuffers() during startup we cycle through all > data->streams_ and calls importBuffers(32). > > The 32 the semi-arbitrary constant in both cases. A long time ago we > discussed what number to use, and we settled on VIDEO_MAX_FRAME. > > This is how the logic is *supposed* to be, but maybe I have messed up > somewhere? :) No I don't thinks so, and you're right about that the buffer count is only used by exportBuffers() and not importBuffers() so that doesn't change. So my last remaining question (sorry, I don't want to bother too much, just making sure this is all intended) is that previously the call to Stream::prepareBuffers() was skipped for some streams (in example StitchOutput and TdnOutput in PipelineHandlerPiSP::prepareBuffers()) while it now happens for all of them. I don't think it's a big deal though. > > > > > > patch is that we now separate the calls to exportBuffers and > > > importBuffers, and this allows us to keep the internal buffers still > > > allocated when clearing the v4l2 cache. Does that make sense? > > > > > > Naush > > > > > > > > > > > > > > > > > > > if (stream->getFlags() & StreamFlag::External) > > > > > continue; > > > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > > > index e73f4b7d31af..f420400dfe18 100644 > > > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > > > @@ -12,9 +12,6 @@ > > > > > > > > > > #include <libcamera/base/log.h> > > > > > > > > > > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */ > > > > > -static constexpr unsigned int maxV4L2BufferCount = 32; > > > > > - > > > > > namespace libcamera { > > > > > > > > > > LOG_DEFINE_CATEGORY(RPISTREAM) > > > > > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer) > > > > > > > > > > int Stream::allocateBuffers(unsigned int count) > > > > > { > > > > > - int ret; > > > > > + int ret = 0; > > > > > > > > > > if (!(flags_ & StreamFlag::ImportOnly)) { > > > > > /* Export some frame buffers for internal use. */ > > > > > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count) > > > > > resetBuffers(); > > > > > } > > > > > > > > > > - return dev_->importBuffers(maxV4L2BufferCount); > > > > > + return ret; > > > > > } > > > > > > > > > > int Stream::queueBuffer(FrameBuffer *buffer) > > > > > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers() > > > > > > > > > > void Stream::releaseBuffers() > > > > > { > > > > > - dev_->releaseBuffers(); > > > > > - clearBuffers(); > > > > > + availableBuffers_ = std::queue<FrameBuffer *>{}; > > > > > + requestBuffers_ = std::queue<FrameBuffer *>{}; > > > > > + internalBuffers_.clear(); > > > > > + bufferMap_.clear(); > > > > > + id_ = 0; > > > > > } > > > > > > > > > > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > > > > > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > > > > > std::forward_as_tuple(buffer, false)); > > > > > } > > > > > > > > > > -void Stream::clearBuffers() > > > > > -{ > > > > > - availableBuffers_ = std::queue<FrameBuffer *>{}; > > > > > - requestBuffers_ = std::queue<FrameBuffer *>{}; > > > > > - internalBuffers_.clear(); > > > > > - bufferMap_.clear(); > > > > > - id_ = 0; > > > > > -} > > > > > - > > > > > int Stream::queueToDevice(FrameBuffer *buffer) > > > > > { > > > > > LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) > > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > > index c267447e5ab5..300a352a7d39 100644 > > > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > > @@ -140,7 +140,6 @@ public: > > > > > > > > > > private: > > > > > void bufferEmplace(unsigned int id, FrameBuffer *buffer); > > > > > - void clearBuffers(); > > > > > int queueToDevice(FrameBuffer *buffer); > > > > > > > > > > StreamFlags flags_; > > > > > -- > > > > > 2.51.0 > > > > >
Hey Jacopo, On Fri, 12 Dec 2025 at 12:58, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Fri, Dec 12, 2025 at 12:47:55PM +0000, Naushir Patuck wrote: > > Hi Jacopo, > > > > On Fri, 12 Dec 2025 at 12:35, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Naush > > > > > > On Fri, Dec 12, 2025 at 12:25:03PM +0000, Naushir Patuck wrote: > > > > Hi Jacopo! > > > > > > > > On Fri, 12 Dec 2025 at 10:07, Jacopo Mondi > > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > > > Hi Naush > > > > > > > > > > On Wed, Dec 10, 2025 at 01:09:14PM +0000, Naushir Patuck wrote: > > > > > > In order to clear the V4L2VideoDevice cache, we must call > > > > > > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this > > > > > > without releasing the allocated buffers, we have to rework the internal > > > > > > buffer allocation logic. > > > > > > > > > > > > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers() > > > > > > from within the RPi::Stream class. Instead, move these calls to the > > > > > > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear > > > > > > the V4L2VideoDevice cache unconditionally on stop without de-allocating > > > > > > the internal buffers. > > > > > > > > > > > > The loigc Stream::clearBuffers() can be then moved into releaseBuffers() > > > > > > > > > > logic > > > > > > > > > > > which will actually de-allocate the internal buffers when required. > > > > > > > > > > > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265 > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > --- > > > > > > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++- > > > > > > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++------------- > > > > > > .../pipeline/rpi/common/rpi_stream.h | 1 - > > > > > > 3 files changed, 14 insertions(+), 18 deletions(-) > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > > > index 2b61b5d241c5..aa0af367d301 100644 > > > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > > > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera) > > > > > > data->state_ = CameraData::State::Stopped; > > > > > > data->platformStop(); > > > > > > > > > > > > - for (auto const stream : data->streams_) > > > > > > + for (auto const stream : data->streams_) { > > > > > > stream->dev()->streamOff(); > > > > > > + stream->dev()->releaseBuffers(); > > > > > > + } > > > > > > > > > > > > /* Disable SOF event generation. */ > > > > > > data->frontendDevice()->setFrameStartEnabled(false); > > > > > > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) > > > > > > int ret; > > > > > > > > > > > > for (auto const stream : data->streams_) { > > > > > > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME); > > > > > > + if (ret < 0) > > > > > > + return ret; > > > > > > + > > > > > > > > > > I still have troubles understanding the buffer allocation policy of > > > > > your pipeline, but that's certainly me. > > > > > > > > > > It seems to me you now call V4L2VideoDevice::importBuffers() on all > > > > > your streams (and you have one stream for each video device in the > > > > > media graph, it seems to me) > > > > > > > > > > While before this change, Stream::prepareBuffers() (where > > > > > importBuffers() was used to be called) was only called on some devices > > > > > and with a variable number of buffers to import (see > > > > > PipelineHandlerPiSP::prepareBuffers()). > > > > > > > > > > Is this intended, or have I missed something ? > > > > > > > > I think the logic remains the same. On both cases, we call > > > > V4L2VideoDevice::importBuffers() on whichever device nodes (or > > > > streams) are enabled/active. The difference in behavior with this > > > > > > Ok, but why I see PipelineHandlerPiSP::prepareBuffers() cycling > > > through streams and > > > 1) Skipping some of them becaue they are not active > > > 2) Setting a different number of buffers depending on the stream > > > > > > while I see PipelineHandlerBase::queueAllBuffers() importing the same > > > number of buffers (VIDEO_MAX_FRAME) on all streams unconditionally ? > > > > I think they are still equivalent. > > > > In the old (existing) code: > > > > - in pipeilne->prepareBuffers() we cycle through all data->streams_ > > and choose the number of buffers to allocate for streams that need > > special handling. > > - Still in the pipeilne->prepareBuffers() loop, we call > > stream->prepareBuffers(count) for all streams, which does the > > allocation with exportBuffers(count) and then calls importBuffers(32). > > > > In the new code: > > > > - pipeilne->prepareBuffers() we cycle through all data->streams_ and > > choose the number of buffers to allocate in > > - Still in the pipeilne->prepareBuffers() loop, call > > stream->allocateBuffers(count), which does the allocation with > > exportBuffers(count). > > - in pipeline->queueAllBuffers() during startup we cycle through all > > data->streams_ and calls importBuffers(32). > > > > The 32 the semi-arbitrary constant in both cases. A long time ago we > > discussed what number to use, and we settled on VIDEO_MAX_FRAME. > > > > This is how the logic is *supposed* to be, but maybe I have messed up > > somewhere? :) > > No I don't thinks so, and you're right about that the buffer count is > only used by exportBuffers() and not importBuffers() so that doesn't > change. > > So my last remaining question (sorry, I don't want to bother too much, > just making sure this is all intended) is that previously the call to > Stream::prepareBuffers() was skipped for some streams (in example > StitchOutput and TdnOutput in PipelineHandlerPiSP::prepareBuffers()) > while it now happens for all of them. Oh good point! In the current logic in prepareBuffers() we have: } else if (stream == &data->isp_[Isp::TdnOutput] && data->config_.disableTdn) { /* TDN is explicitly disabled. */ continue; } else if (stream == &data->isp_[Isp::StitchOutput] && data->config_.disableHdr) { /* Stitch/HDR is explicitly disabled. */ continue; } which skips the exportBuffers() and importBuffers() call. In the new logic we call importBuffers() unconditionally when these features might be disabled. > I don't think it's a big deal though. Yes, it's not a big deal as we don't allocate buffers, just reserve v4l2 slots that will never be used. But I can change it and make the importBuffers() call conditional like above in v2. Regards, Naush > > > > > > > > > > patch is that we now separate the calls to exportBuffers and > > > > importBuffers, and this allows us to keep the internal buffers still > > > > allocated when clearing the v4l2 cache. Does that make sense? > > > > > > > > Naush > > > > > > > > > > > > > > > > > > > > > > > > if (stream->getFlags() & StreamFlag::External) > > > > > > continue; > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > > > > index e73f4b7d31af..f420400dfe18 100644 > > > > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > > > > > > @@ -12,9 +12,6 @@ > > > > > > > > > > > > #include <libcamera/base/log.h> > > > > > > > > > > > > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */ > > > > > > -static constexpr unsigned int maxV4L2BufferCount = 32; > > > > > > - > > > > > > namespace libcamera { > > > > > > > > > > > > LOG_DEFINE_CATEGORY(RPISTREAM) > > > > > > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer) > > > > > > > > > > > > int Stream::allocateBuffers(unsigned int count) > > > > > > { > > > > > > - int ret; > > > > > > + int ret = 0; > > > > > > > > > > > > if (!(flags_ & StreamFlag::ImportOnly)) { > > > > > > /* Export some frame buffers for internal use. */ > > > > > > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count) > > > > > > resetBuffers(); > > > > > > } > > > > > > > > > > > > - return dev_->importBuffers(maxV4L2BufferCount); > > > > > > + return ret; > > > > > > } > > > > > > > > > > > > int Stream::queueBuffer(FrameBuffer *buffer) > > > > > > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers() > > > > > > > > > > > > void Stream::releaseBuffers() > > > > > > { > > > > > > - dev_->releaseBuffers(); > > > > > > - clearBuffers(); > > > > > > + availableBuffers_ = std::queue<FrameBuffer *>{}; > > > > > > + requestBuffers_ = std::queue<FrameBuffer *>{}; > > > > > > + internalBuffers_.clear(); > > > > > > + bufferMap_.clear(); > > > > > > + id_ = 0; > > > > > > } > > > > > > > > > > > > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > > > > > > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > > > > > > std::forward_as_tuple(buffer, false)); > > > > > > } > > > > > > > > > > > > -void Stream::clearBuffers() > > > > > > -{ > > > > > > - availableBuffers_ = std::queue<FrameBuffer *>{}; > > > > > > - requestBuffers_ = std::queue<FrameBuffer *>{}; > > > > > > - internalBuffers_.clear(); > > > > > > - bufferMap_.clear(); > > > > > > - id_ = 0; > > > > > > -} > > > > > > - > > > > > > int Stream::queueToDevice(FrameBuffer *buffer) > > > > > > { > > > > > > LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > > > index c267447e5ab5..300a352a7d39 100644 > > > > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > > > @@ -140,7 +140,6 @@ public: > > > > > > > > > > > > private: > > > > > > void bufferEmplace(unsigned int id, FrameBuffer *buffer); > > > > > > - void clearBuffers(); > > > > > > int queueToDevice(FrameBuffer *buffer); > > > > > > > > > > > > StreamFlags flags_; > > > > > > -- > > > > > > 2.51.0 > > > > > >
Hi 2025. 12. 10. 14:09 keltezéssel, Naushir Patuck írta: > In order to clear the V4L2VideoDevice cache, we must call > V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this > without releasing the allocated buffers, we have to rework the internal > buffer allocation logic. > > Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers() > from within the RPi::Stream class. Instead, move these calls to the > PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear > the V4L2VideoDevice cache unconditionally on stop without de-allocating > the internal buffers. > > The loigc Stream::clearBuffers() can be then moved into releaseBuffers() > which will actually de-allocate the internal buffers when required. > > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265 > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- The `CaptureStartStop` tests in lc-compliance now seem to finish mostly without errors. Anything involving `StreamRole::Raw` streams still fails because it is expected that calling `validate()` on configuration returned from `Camera::generateConfiguration()` should return `Valid` if it is not modified. But it returns `Adjusted` (at least on the cases I tested). Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> # rpi4 > .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++- > .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++------------- > .../pipeline/rpi/common/rpi_stream.h | 1 - > 3 files changed, 14 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 2b61b5d241c5..aa0af367d301 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera) > data->state_ = CameraData::State::Stopped; > data->platformStop(); > > - for (auto const stream : data->streams_) > + for (auto const stream : data->streams_) { > stream->dev()->streamOff(); > + stream->dev()->releaseBuffers(); > + } > > /* Disable SOF event generation. */ > data->frontendDevice()->setFrameStartEnabled(false); > @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) > int ret; > > for (auto const stream : data->streams_) { > + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME); > + if (ret < 0) > + return ret; > + > if (stream->getFlags() & StreamFlag::External) > continue; > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > index e73f4b7d31af..f420400dfe18 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp > @@ -12,9 +12,6 @@ > > #include <libcamera/base/log.h> > > -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */ > -static constexpr unsigned int maxV4L2BufferCount = 32; > - > namespace libcamera { > > LOG_DEFINE_CATEGORY(RPISTREAM) > @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer) > > int Stream::allocateBuffers(unsigned int count) > { > - int ret; > + int ret = 0; > > if (!(flags_ & StreamFlag::ImportOnly)) { > /* Export some frame buffers for internal use. */ > @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count) > resetBuffers(); > } > > - return dev_->importBuffers(maxV4L2BufferCount); > + return ret; > } > > int Stream::queueBuffer(FrameBuffer *buffer) > @@ -243,8 +240,11 @@ int Stream::queueAllBuffers() > > void Stream::releaseBuffers() > { > - dev_->releaseBuffers(); > - clearBuffers(); > + availableBuffers_ = std::queue<FrameBuffer *>{}; > + requestBuffers_ = std::queue<FrameBuffer *>{}; > + internalBuffers_.clear(); > + bufferMap_.clear(); > + id_ = 0; > } > > void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) > std::forward_as_tuple(buffer, false)); > } > > -void Stream::clearBuffers() > -{ > - availableBuffers_ = std::queue<FrameBuffer *>{}; > - requestBuffers_ = std::queue<FrameBuffer *>{}; > - internalBuffers_.clear(); > - bufferMap_.clear(); > - id_ = 0; > -} > - > int Stream::queueToDevice(FrameBuffer *buffer) > { > LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > index c267447e5ab5..300a352a7d39 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > @@ -140,7 +140,6 @@ public: > > private: > void bufferEmplace(unsigned int id, FrameBuffer *buffer); > - void clearBuffers(); > int queueToDevice(FrameBuffer *buffer); > > StreamFlags flags_;
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index 2b61b5d241c5..aa0af367d301 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp @@ -719,8 +719,10 @@ void PipelineHandlerBase::stopDevice(Camera *camera) data->state_ = CameraData::State::Stopped; data->platformStop(); - for (auto const stream : data->streams_) + for (auto const stream : data->streams_) { stream->dev()->streamOff(); + stream->dev()->releaseBuffers(); + } /* Disable SOF event generation. */ data->frontendDevice()->setFrameStartEnabled(false); @@ -901,6 +903,10 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera) int ret; for (auto const stream : data->streams_) { + ret = stream->dev()->importBuffers(VIDEO_MAX_FRAME); + if (ret < 0) + return ret; + if (stream->getFlags() & StreamFlag::External) continue; diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp index e73f4b7d31af..f420400dfe18 100644 --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp @@ -12,9 +12,6 @@ #include <libcamera/base/log.h> -/* Maximum number of buffer slots to allocate in the V4L2 device driver. */ -static constexpr unsigned int maxV4L2BufferCount = 32; - namespace libcamera { LOG_DEFINE_CATEGORY(RPISTREAM) @@ -108,7 +105,7 @@ void Stream::setExportedBuffer(FrameBuffer *buffer) int Stream::allocateBuffers(unsigned int count) { - int ret; + int ret = 0; if (!(flags_ & StreamFlag::ImportOnly)) { /* Export some frame buffers for internal use. */ @@ -121,7 +118,7 @@ int Stream::allocateBuffers(unsigned int count) resetBuffers(); } - return dev_->importBuffers(maxV4L2BufferCount); + return ret; } int Stream::queueBuffer(FrameBuffer *buffer) @@ -243,8 +240,11 @@ int Stream::queueAllBuffers() void Stream::releaseBuffers() { - dev_->releaseBuffers(); - clearBuffers(); + availableBuffers_ = std::queue<FrameBuffer *>{}; + requestBuffers_ = std::queue<FrameBuffer *>{}; + internalBuffers_.clear(); + bufferMap_.clear(); + id_ = 0; } void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) @@ -257,15 +257,6 @@ void Stream::bufferEmplace(unsigned int id, FrameBuffer *buffer) std::forward_as_tuple(buffer, false)); } -void Stream::clearBuffers() -{ - availableBuffers_ = std::queue<FrameBuffer *>{}; - requestBuffers_ = std::queue<FrameBuffer *>{}; - internalBuffers_.clear(); - bufferMap_.clear(); - id_ = 0; -} - int Stream::queueToDevice(FrameBuffer *buffer) { LOG(RPISTREAM, Debug) << "Queuing buffer " << getBufferId(buffer) diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h index c267447e5ab5..300a352a7d39 100644 --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h @@ -140,7 +140,6 @@ public: private: void bufferEmplace(unsigned int id, FrameBuffer *buffer); - void clearBuffers(); int queueToDevice(FrameBuffer *buffer); StreamFlags flags_;
In order to clear the V4L2VideoDevice cache, we must call V4L2VideoDevice::releaseBuffers() when stopping the camera. To do this without releasing the allocated buffers, we have to rework the internal buffer allocation logic. Remove calls to V4L2VideoDevice::importBuffers() and releaseBuffers() from within the RPi::Stream class. Instead, move these calls to the PipelineHandlerBase class, where we can call releaseBuffers(), i.e. clear the V4L2VideoDevice cache unconditionally on stop without de-allocating the internal buffers. The loigc Stream::clearBuffers() can be then moved into releaseBuffers() which will actually de-allocate the internal buffers when required. Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/265 Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/rpi/common/pipeline_base.cpp | 8 ++++++- .../pipeline/rpi/common/rpi_stream.cpp | 23 ++++++------------- .../pipeline/rpi/common/rpi_stream.h | 1 - 3 files changed, 14 insertions(+), 18 deletions(-)