| Message ID | 20250927203947.126092-1-robert.mader@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 09. 27. 22:39 keltezéssel, Robert Mader írta: > While a default value of 4 buffers appears to be a good default that is > used by other pipelines as well, allowing both higher and lower values > can be desirable, notably for: > 1. Video encoding, e.g. encoding multiple buffers in parallel. > 2. Clients requesting a single buffer - e.g. in multi-stream scenarios. > > Thus allow up to 32 buffers - following the default from vb2 core - while > keeping the default to the previous 4. > > While on it: > 1. mark the config as adjusted when appropriate. > 2. increase the number of internal buffer used by the swISP to 4 as well. > This has been shipped downstream in postmarketOS for a while and, in > some cases, seems to improve stability on no-so-great drivers. I feel like (2) should be a separate change. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > --- > > Changes since v1 with title "pipeline: simple: Allow buffer counts from 1 to 16 for swISP" > 1: Cover all cases, not just the swISP one. > 2: Increase maximum to 32 to match vb2 core. > 3: Change constant naming to better match similar ones. > 4: Bump kNumInternalBuffers to 4. > --- > src/libcamera/pipeline/simple/simple.cpp | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index c816cffc9..23585692c 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -378,6 +378,9 @@ public: > const Transform &combinedTransform() const { return combinedTransform_; } > > private: > + static constexpr unsigned int kNumBuffersDefault = 4; > + static constexpr unsigned int kNumBuffersMax = 32; > + > /* > * The SimpleCameraData instance is guaranteed to be valid as long as > * the corresponding Camera instance is valid. In order to borrow a > @@ -417,7 +420,7 @@ protected: > int queueRequestDevice(Camera *camera, Request *request) override; > > private: > - static constexpr unsigned int kNumInternalBuffers = 3; > + static constexpr unsigned int kNumInternalBuffers = 4; > > struct EntityData { > std::unique_ptr<V4L2VideoDevice> video; > @@ -1239,7 +1242,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > cfg.size != pipeConfig_->captureSize) > needConversion_ = true; > > - /* Set the stride, frameSize and bufferCount. */ > + /* Set the stride and frameSize. */ > if (needConversion_) { > std::tie(cfg.stride, cfg.frameSize) = > data_->converter_ > @@ -1262,7 +1265,19 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > cfg.frameSize = format.planes[0].size; > } > > - cfg.bufferCount = 4; > + if (cfg.bufferCount == 0) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersDefault; > + cfg.bufferCount = kNumBuffersDefault; > + status = Adjusted; > + } else if (cfg.bufferCount > kNumBuffersMax) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersMax; > + cfg.bufferCount = kNumBuffersMax; > + status = Adjusted; > + } I think maybe I would const auto bufferCount = cfg.bufferCount; if (bufferCount <= 0) cfg.bufferCount = kNumBuffersDefault; else if (bufferCount > kNumBuffersMax) cfg.bufferCount = kNumBuffersMax; if (cfg.bufferCount != bufferCount) { LOG(SimplePipeline, Debug) << ...; status = Adjusted; } Regards, Barnabás Pőcze > } > > return status;
Hi Robert, Robert Mader <robert.mader@collabora.com> writes: > While a default value of 4 buffers appears to be a good default that is > used by other pipelines as well, allowing both higher and lower values > can be desirable, notably for: > 1. Video encoding, e.g. encoding multiple buffers in parallel. > 2. Clients requesting a single buffer - e.g. in multi-stream scenarios. > > Thus allow up to 32 buffers - following the default from vb2 core - while > keeping the default to the previous 4. > > While on it: > 1. mark the config as adjusted when appropriate. > 2. increase the number of internal buffer used by the swISP to 4 as well. > This has been shipped downstream in postmarketOS for a while and, in > some cases, seems to improve stability on no-so-great drivers. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > --- > > Changes since v1 with title "pipeline: simple: Allow buffer counts from 1 to 16 for swISP" > 1: Cover all cases, not just the swISP one. > 2: Increase maximum to 32 to match vb2 core. > 3: Change constant naming to better match similar ones. > 4: Bump kNumInternalBuffers to 4. > --- > src/libcamera/pipeline/simple/simple.cpp | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index c816cffc9..23585692c 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -378,6 +378,9 @@ public: > const Transform &combinedTransform() const { return combinedTransform_; } > > private: > + static constexpr unsigned int kNumBuffersDefault = 4; > + static constexpr unsigned int kNumBuffersMax = 32; > + > /* > * The SimpleCameraData instance is guaranteed to be valid as long as > * the corresponding Camera instance is valid. In order to borrow a > @@ -417,7 +420,7 @@ protected: > int queueRequestDevice(Camera *camera, Request *request) override; > > private: > - static constexpr unsigned int kNumInternalBuffers = 3; > + static constexpr unsigned int kNumInternalBuffers = 4; > > struct EntityData { > std::unique_ptr<V4L2VideoDevice> video; > @@ -1239,7 +1242,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > cfg.size != pipeConfig_->captureSize) > needConversion_ = true; > > - /* Set the stride, frameSize and bufferCount. */ > + /* Set the stride and frameSize. */ > if (needConversion_) { > std::tie(cfg.stride, cfg.frameSize) = > data_->converter_ > @@ -1262,7 +1265,19 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > cfg.frameSize = format.planes[0].size; > } > > - cfg.bufferCount = 4; > + if (cfg.bufferCount == 0) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersDefault; > + cfg.bufferCount = kNumBuffersDefault; > + status = Adjusted; > + } else if (cfg.bufferCount > kNumBuffersMax) { > + LOG(SimplePipeline, Debug) > + << "Adjusting bufferCount from " << cfg.bufferCount > + << " to " << kNumBuffersMax; > + cfg.bufferCount = kNumBuffersMax; > + status = Adjusted; > + } > } > > return status;
Hi Barnabás, On 9/29/25 11:43, Barnabás Pőcze wrote: > Hi > > 2025. 09. 27. 22:39 keltezéssel, Robert Mader írta: >> While a default value of 4 buffers appears to be a good default that is >> used by other pipelines as well, allowing both higher and lower values >> can be desirable, notably for: >> 1. Video encoding, e.g. encoding multiple buffers in parallel. >> 2. Clients requesting a single buffer - e.g. in multi-stream scenarios. >> >> Thus allow up to 32 buffers - following the default from vb2 core - >> while >> keeping the default to the previous 4. >> >> While on it: >> 1. mark the config as adjusted when appropriate. >> 2. increase the number of internal buffer used by the swISP to 4 as >> well. >> This has been shipped downstream in postmarketOS for a while and, in >> some cases, seems to improve stability on no-so-great drivers. > > I feel like (2) should be a separate change. Split it into its own commit in v3. >> >> Signed-off-by: Robert Mader <robert.mader@collabora.com> >> >> --- >> >> Changes since v1 with title "pipeline: simple: Allow buffer counts >> from 1 to 16 for swISP" >> 1: Cover all cases, not just the swISP one. >> 2: Increase maximum to 32 to match vb2 core. >> 3: Change constant naming to better match similar ones. >> 4: Bump kNumInternalBuffers to 4. >> --- >> src/libcamera/pipeline/simple/simple.cpp | 21 ++++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp >> b/src/libcamera/pipeline/simple/simple.cpp >> index c816cffc9..23585692c 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -378,6 +378,9 @@ public: >> const Transform &combinedTransform() const { return >> combinedTransform_; } >> private: >> + static constexpr unsigned int kNumBuffersDefault = 4; >> + static constexpr unsigned int kNumBuffersMax = 32; >> + >> /* >> * The SimpleCameraData instance is guaranteed to be valid as >> long as >> * the corresponding Camera instance is valid. In order to >> borrow a >> @@ -417,7 +420,7 @@ protected: >> int queueRequestDevice(Camera *camera, Request *request) override; >> private: >> - static constexpr unsigned int kNumInternalBuffers = 3; >> + static constexpr unsigned int kNumInternalBuffers = 4; >> struct EntityData { >> std::unique_ptr<V4L2VideoDevice> video; >> @@ -1239,7 +1242,7 @@ CameraConfiguration::Status >> SimpleCameraConfiguration::validate() >> cfg.size != pipeConfig_->captureSize) >> needConversion_ = true; >> - /* Set the stride, frameSize and bufferCount. */ >> + /* Set the stride and frameSize. */ >> if (needConversion_) { >> std::tie(cfg.stride, cfg.frameSize) = >> data_->converter_ >> @@ -1262,7 +1265,19 @@ CameraConfiguration::Status >> SimpleCameraConfiguration::validate() >> cfg.frameSize = format.planes[0].size; >> } >> - cfg.bufferCount = 4; >> + if (cfg.bufferCount == 0) { >> + LOG(SimplePipeline, Debug) >> + << "Adjusting bufferCount from " << cfg.bufferCount >> + << " to " << kNumBuffersDefault; >> + cfg.bufferCount = kNumBuffersDefault; >> + status = Adjusted; >> + } else if (cfg.bufferCount > kNumBuffersMax) { >> + LOG(SimplePipeline, Debug) >> + << "Adjusting bufferCount from " << cfg.bufferCount >> + << " to " << kNumBuffersMax; >> + cfg.bufferCount = kNumBuffersMax; >> + status = Adjusted; >> + } > > I think maybe I would > > const auto bufferCount = cfg.bufferCount; > if (bufferCount <= 0) > cfg.bufferCount = kNumBuffersDefault; > else if (bufferCount > kNumBuffersMax) > cfg.bufferCount = kNumBuffersMax; > > if (cfg.bufferCount != bufferCount) { > LOG(SimplePipeline, Debug) << ...; > status = Adjusted; > } That's indeed better, thanks! > > > Regards, > Barnabás Pőcze > >> } >> return status; >
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index c816cffc9..23585692c 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -378,6 +378,9 @@ public: const Transform &combinedTransform() const { return combinedTransform_; } private: + static constexpr unsigned int kNumBuffersDefault = 4; + static constexpr unsigned int kNumBuffersMax = 32; + /* * The SimpleCameraData instance is guaranteed to be valid as long as * the corresponding Camera instance is valid. In order to borrow a @@ -417,7 +420,7 @@ protected: int queueRequestDevice(Camera *camera, Request *request) override; private: - static constexpr unsigned int kNumInternalBuffers = 3; + static constexpr unsigned int kNumInternalBuffers = 4; struct EntityData { std::unique_ptr<V4L2VideoDevice> video; @@ -1239,7 +1242,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() cfg.size != pipeConfig_->captureSize) needConversion_ = true; - /* Set the stride, frameSize and bufferCount. */ + /* Set the stride and frameSize. */ if (needConversion_) { std::tie(cfg.stride, cfg.frameSize) = data_->converter_ @@ -1262,7 +1265,19 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() cfg.frameSize = format.planes[0].size; } - cfg.bufferCount = 4; + if (cfg.bufferCount == 0) { + LOG(SimplePipeline, Debug) + << "Adjusting bufferCount from " << cfg.bufferCount + << " to " << kNumBuffersDefault; + cfg.bufferCount = kNumBuffersDefault; + status = Adjusted; + } else if (cfg.bufferCount > kNumBuffersMax) { + LOG(SimplePipeline, Debug) + << "Adjusting bufferCount from " << cfg.bufferCount + << " to " << kNumBuffersMax; + cfg.bufferCount = kNumBuffersMax; + status = Adjusted; + } } return status;
While a default value of 4 buffers appears to be a good default that is used by other pipelines as well, allowing both higher and lower values can be desirable, notably for: 1. Video encoding, e.g. encoding multiple buffers in parallel. 2. Clients requesting a single buffer - e.g. in multi-stream scenarios. Thus allow up to 32 buffers - following the default from vb2 core - while keeping the default to the previous 4. While on it: 1. mark the config as adjusted when appropriate. 2. increase the number of internal buffer used by the swISP to 4 as well. This has been shipped downstream in postmarketOS for a while and, in some cases, seems to improve stability on no-so-great drivers. Signed-off-by: Robert Mader <robert.mader@collabora.com> --- Changes since v1 with title "pipeline: simple: Allow buffer counts from 1 to 16 for swISP" 1: Cover all cases, not just the swISP one. 2: Increase maximum to 32 to match vb2 core. 3: Change constant naming to better match similar ones. 4: Bump kNumInternalBuffers to 4. --- src/libcamera/pipeline/simple/simple.cpp | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)