| Message ID | 20210824195636.1110845-6-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Tue, Aug 24, 2021 at 04:56:24PM -0300, Nícolas F. R. A. Prado wrote: > Currently the simple pipeline handler relies on bufferCount to decide on > the number of buffers to allocate internally when a converter is in use > and for the number of V4L2 buffer slots to reserve. Instead, the number > of internal buffers should be the minimum required by the pipeline to > keep the requests flowing, in order to avoid wasting memory, while the > number of V4L2 buffer slots should be greater than the expected number > of requests queued by the application, in order to avoid thrashing > dmabuf mappings, which would degrade performance. > > Stop relying on bufferCount for these numbers and instead set them to > appropriate, and independent, constants. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Changes in v8: > - New > > src/libcamera/pipeline/simple/converter.cpp | 12 ++++++----- > src/libcamera/pipeline/simple/converter.h | 6 ++++-- > src/libcamera/pipeline/simple/simple.cpp | 22 ++++++++++++++++----- > 3 files changed, 28 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp > index b5e34c4cd0c5..3133b3dbda07 100644 > --- a/src/libcamera/pipeline/simple/converter.cpp > +++ b/src/libcamera/pipeline/simple/converter.cpp > @@ -101,13 +101,14 @@ int SimpleConverter::Stream::exportBuffers(unsigned int count, > return m2m_->capture()->exportBuffers(count, buffers); > } > > -int SimpleConverter::Stream::start() > +int SimpleConverter::Stream::start(unsigned int internalBufferCount, > + unsigned int bufferSlotCount) > { > - int ret = m2m_->output()->importBuffers(inputBufferCount_); > + int ret = m2m_->output()->importBuffers(internalBufferCount); > if (ret < 0) > return ret; > > - ret = m2m_->capture()->importBuffers(outputBufferCount_); > + ret = m2m_->capture()->importBuffers(bufferSlotCount); > if (ret < 0) { > stop(); > return ret; > @@ -331,12 +332,13 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count, > return streams_[output].exportBuffers(count, buffers); > } > > -int SimpleConverter::start() > +int SimpleConverter::start(unsigned int internalBufferCount, > + unsigned int bufferSlotCount) > { > int ret; > > for (Stream &stream : streams_) { > - ret = stream.start(); > + ret = stream.start(internalBufferCount, bufferSlotCount); > if (ret < 0) { > stop(); > return ret; > diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h > index 276a2a291c21..deb3df0d08df 100644 > --- a/src/libcamera/pipeline/simple/converter.h > +++ b/src/libcamera/pipeline/simple/converter.h > @@ -47,7 +47,8 @@ public: > int exportBuffers(unsigned int ouput, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > > - int start(); > + int start(unsigned int internalBufferCount, > + unsigned int bufferSlotCount); > void stop(); > > int queueBuffers(FrameBuffer *input, > @@ -69,7 +70,8 @@ private: > int exportBuffers(unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > > - int start(); > + int start(unsigned int internalBufferCount, > + unsigned int bufferSlotCount); > void stop(); > > int queueBuffers(FrameBuffer *input, FrameBuffer *output); > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 737815452bae..d0a658a23be8 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -253,6 +253,7 @@ protected: > > private: > static constexpr unsigned int kNumInternalBuffers = 3; > + static constexpr unsigned int kSimpleBufferSlotCount = 16; > > SimpleCameraData *cameraData(Camera *camera) > { > @@ -830,17 +831,27 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > V4L2VideoDevice *video = data->video_; > int ret; > > + /* > + * Number of internal buffers that will be used to move video capture > + * device frames to the converter. > + * > + * \todo Make this depend on the driver in use instead of being > + * hardcoded. In order to not drop frames, the realtime requirements for > + * each device should be checked, so it may not be as simple as just > + * using the minimum number of buffers required by the driver. > + */ > + static constexpr unsigned int internalBufferCount = 3; > + > if (data->useConverter_) { > /* > * When using the converter allocate a fixed number of internal > * buffers. > */ > - ret = video->allocateBuffers(kNumInternalBuffers, > + ret = video->allocateBuffers(internalBufferCount, > &data->converterBuffers_); > } else { > - /* Otherwise, prepare for using buffers from the only stream. */ > - Stream *stream = &data->streams_[0]; > - ret = video->importBuffers(stream->configuration().bufferCount); > + /* Otherwise, prepare for using buffers. */ > + ret = video->importBuffers(kSimpleBufferSlotCount); > } > if (ret < 0) > return ret; > @@ -852,7 +863,8 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL > } > > if (data->useConverter_) { > - ret = converter_->start(); > + ret = converter_->start(internalBufferCount, > + kSimpleBufferSlotCount); > if (ret < 0) { > stop(camera); > return ret; > -- > 2.33.0 >
diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp index b5e34c4cd0c5..3133b3dbda07 100644 --- a/src/libcamera/pipeline/simple/converter.cpp +++ b/src/libcamera/pipeline/simple/converter.cpp @@ -101,13 +101,14 @@ int SimpleConverter::Stream::exportBuffers(unsigned int count, return m2m_->capture()->exportBuffers(count, buffers); } -int SimpleConverter::Stream::start() +int SimpleConverter::Stream::start(unsigned int internalBufferCount, + unsigned int bufferSlotCount) { - int ret = m2m_->output()->importBuffers(inputBufferCount_); + int ret = m2m_->output()->importBuffers(internalBufferCount); if (ret < 0) return ret; - ret = m2m_->capture()->importBuffers(outputBufferCount_); + ret = m2m_->capture()->importBuffers(bufferSlotCount); if (ret < 0) { stop(); return ret; @@ -331,12 +332,13 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count, return streams_[output].exportBuffers(count, buffers); } -int SimpleConverter::start() +int SimpleConverter::start(unsigned int internalBufferCount, + unsigned int bufferSlotCount) { int ret; for (Stream &stream : streams_) { - ret = stream.start(); + ret = stream.start(internalBufferCount, bufferSlotCount); if (ret < 0) { stop(); return ret; diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h index 276a2a291c21..deb3df0d08df 100644 --- a/src/libcamera/pipeline/simple/converter.h +++ b/src/libcamera/pipeline/simple/converter.h @@ -47,7 +47,8 @@ public: int exportBuffers(unsigned int ouput, unsigned int count, std::vector<std::unique_ptr<FrameBuffer>> *buffers); - int start(); + int start(unsigned int internalBufferCount, + unsigned int bufferSlotCount); void stop(); int queueBuffers(FrameBuffer *input, @@ -69,7 +70,8 @@ private: int exportBuffers(unsigned int count, std::vector<std::unique_ptr<FrameBuffer>> *buffers); - int start(); + int start(unsigned int internalBufferCount, + unsigned int bufferSlotCount); void stop(); int queueBuffers(FrameBuffer *input, FrameBuffer *output); diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 737815452bae..d0a658a23be8 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -253,6 +253,7 @@ protected: private: static constexpr unsigned int kNumInternalBuffers = 3; + static constexpr unsigned int kSimpleBufferSlotCount = 16; SimpleCameraData *cameraData(Camera *camera) { @@ -830,17 +831,27 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL V4L2VideoDevice *video = data->video_; int ret; + /* + * Number of internal buffers that will be used to move video capture + * device frames to the converter. + * + * \todo Make this depend on the driver in use instead of being + * hardcoded. In order to not drop frames, the realtime requirements for + * each device should be checked, so it may not be as simple as just + * using the minimum number of buffers required by the driver. + */ + static constexpr unsigned int internalBufferCount = 3; + if (data->useConverter_) { /* * When using the converter allocate a fixed number of internal * buffers. */ - ret = video->allocateBuffers(kNumInternalBuffers, + ret = video->allocateBuffers(internalBufferCount, &data->converterBuffers_); } else { - /* Otherwise, prepare for using buffers from the only stream. */ - Stream *stream = &data->streams_[0]; - ret = video->importBuffers(stream->configuration().bufferCount); + /* Otherwise, prepare for using buffers. */ + ret = video->importBuffers(kSimpleBufferSlotCount); } if (ret < 0) return ret; @@ -852,7 +863,8 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL } if (data->useConverter_) { - ret = converter_->start(); + ret = converter_->start(internalBufferCount, + kSimpleBufferSlotCount); if (ret < 0) { stop(camera); return ret;
Currently the simple pipeline handler relies on bufferCount to decide on the number of buffers to allocate internally when a converter is in use and for the number of V4L2 buffer slots to reserve. Instead, the number of internal buffers should be the minimum required by the pipeline to keep the requests flowing, in order to avoid wasting memory, while the number of V4L2 buffer slots should be greater than the expected number of requests queued by the application, in order to avoid thrashing dmabuf mappings, which would degrade performance. Stop relying on bufferCount for these numbers and instead set them to appropriate, and independent, constants. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- Changes in v8: - New src/libcamera/pipeline/simple/converter.cpp | 12 ++++++----- src/libcamera/pipeline/simple/converter.h | 6 ++++-- src/libcamera/pipeline/simple/simple.cpp | 22 ++++++++++++++++----- 3 files changed, 28 insertions(+), 12 deletions(-)