| Message ID | 20250930124208.14391-1-robert.mader@collabora.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 09. 30. 14:42 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 buffer counts between 1 and 32 buffers - following the default > maximum from vb2 core - while keeping the default to the previous 4. > > While on it mark the config as adjusted when appropriate. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > > --- > > Changes in v3: > 1. Adopeted code cleanup suggestion - no change in behavior. > 2. Split out change of kNumInternalBuffers. > 3. Minor commit message changes. > > 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 | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index c816cffc9..2dcba04ec 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; I was testing this with `cam`, and it brought up an issue. Specifically, `ipa::soft::kMaxFrameContexts` is just 16, which is less than 32. So `cam`, which creates requests based on the number of buffers, may run into the following problem with more than 16 buffers: [0:04:25.186460320] [3298] FATAL FCQueue fc_queue.h:85 Frame context for 0 has been overwritten by 16 Backtrace: libcamera::ipa::FCQueue<libcamera::ipa::soft::IPAFrameContext>::get(unsigned int)+0x5ef (../src/ipa/libipa/fc_queue.h:85) libcamera::ipa::soft::IPASoftSimple::computeParams(unsigned int)+0x165 (../src/ipa/simple/soft_simple.cpp:294) libcamera::ipa::soft::IPAProxySoftThreaded::ThreadProxy::computeParams(unsigned int)+0x637 (include/libcamera/ipa/soft_ipa_proxy.h:125) ... So I think it would be nice to fix this as well. I believe we should pass 16 to the `PipelineHandler` base constructor as `maxQueuedRequestsDevice`. But it would be nice to be able to access that 16 constant somehow so that they don't go out of sync. `cam` patch for testing: diff --git a/src/apps/common/stream_options.cpp b/src/apps/common/stream_options.cpp index 288f86530..b8e1a6d2e 100644 --- a/src/apps/common/stream_options.cpp +++ b/src/apps/common/stream_options.cpp @@ -25,6 +25,8 @@ StreamKeyValueParser::StreamKeyValueParser() ArgumentRequired); addOption("colorspace", OptionString, "Color space", ArgumentRequired); + addOption("buffers", OptionInteger, "Number of buffers", + ArgumentRequired); } KeyValueParser::Options StreamKeyValueParser::parse(const char *arguments) @@ -95,6 +97,9 @@ int StreamKeyValueParser::updateConfiguration(CameraConfiguration *config, if (opts.isSet("colorspace")) cfg.colorSpace = ColorSpace::fromString(opts["colorspace"].toString()); + + if (opts.isSet("buffers")) + cfg.bufferCount = opts["buffers"]; } return 0; Regards, Barnabás Pőcze > + > /* > * The SimpleCameraData instance is guaranteed to be valid as long as > * the corresponding Camera instance is valid. In order to borrow a > @@ -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,18 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > cfg.frameSize = format.planes[0].size; > } > > - cfg.bufferCount = 4; > + 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) > + << "Adjusting bufferCount from " << bufferCount > + << " to " << cfg.bufferCount; > + status = Adjusted; > + } > } > > return status;
Hi, On 10/1/25 10:10, Barnabás Pőcze wrote: > Hi > > 2025. 09. 30. 14:42 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 buffer counts between 1 and 32 buffers - following the >> default >> maximum from vb2 core - while keeping the default to the previous 4. >> >> While on it mark the config as adjusted when appropriate. >> >> Signed-off-by: Robert Mader <robert.mader@collabora.com> >> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> Changes in v3: >> 1. Adopeted code cleanup suggestion - no change in behavior. >> 2. Split out change of kNumInternalBuffers. >> 3. Minor commit message changes. >> >> 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 | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp >> b/src/libcamera/pipeline/simple/simple.cpp >> index c816cffc9..2dcba04ec 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; > > I was testing this with `cam`, and it brought up an issue. Specifically, > `ipa::soft::kMaxFrameContexts` is just 16, which is less than 32. So > `cam`, > which creates requests based on the number of buffers, may run into the > following problem with more than 16 buffers: > > [0:04:25.186460320] [3298] FATAL FCQueue fc_queue.h:85 Frame context > for 0 has been overwritten by 16 > Backtrace: > libcamera::ipa::FCQueue<libcamera::ipa::soft::IPAFrameContext>::get(unsigned > int)+0x5ef (../src/ipa/libipa/fc_queue.h:85) > libcamera::ipa::soft::IPASoftSimple::computeParams(unsigned > int)+0x165 (../src/ipa/simple/soft_simple.cpp:294) > libcamera::ipa::soft::IPAProxySoftThreaded::ThreadProxy::computeParams(unsigned > int)+0x637 (include/libcamera/ipa/soft_ipa_proxy.h:125) > ... > So I think it would be nice to fix this as well. I believe we should > pass > 16 to the `PipelineHandler` base constructor as > `maxQueuedRequestsDevice`. > But it would be nice to be able to access that 16 constant somehow so > that > they don't go out of sync. > `cam` patch for testing: Nice catch, thanks for testing! I haven't got around to test that solution yet, however assuming it'll work I wonder: should we maybe be even more conservative and set maxQueuedRequestsDevice to something like 4, in order to avoid regressions? I.e. could there be valid reasons why drivers don't actually handle that many queued requests? > > diff --git a/src/apps/common/stream_options.cpp > b/src/apps/common/stream_options.cpp > index 288f86530..b8e1a6d2e 100644 > --- a/src/apps/common/stream_options.cpp > +++ b/src/apps/common/stream_options.cpp > @@ -25,6 +25,8 @@ StreamKeyValueParser::StreamKeyValueParser() > ArgumentRequired); > addOption("colorspace", OptionString, "Color space", > ArgumentRequired); > + addOption("buffers", OptionInteger, "Number of buffers", > + ArgumentRequired); > } > > KeyValueParser::Options StreamKeyValueParser::parse(const char > *arguments) > @@ -95,6 +97,9 @@ int > StreamKeyValueParser::updateConfiguration(CameraConfiguration *config, > > if (opts.isSet("colorspace")) > cfg.colorSpace = > ColorSpace::fromString(opts["colorspace"].toString()); > + > + if (opts.isSet("buffers")) > + cfg.bufferCount = opts["buffers"]; > } > > return 0; > > > Regards, > Barnabás Pőcze > >> + >> /* >> * The SimpleCameraData instance is guaranteed to be valid as >> long as >> * the corresponding Camera instance is valid. In order to >> borrow a >> @@ -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,18 @@ CameraConfiguration::Status >> SimpleCameraConfiguration::validate() >> cfg.frameSize = format.planes[0].size; >> } >> - cfg.bufferCount = 4; >> + 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) >> + << "Adjusting bufferCount from " << bufferCount >> + << " to " << cfg.bufferCount; >> + status = Adjusted; >> + } >> } >> return status; >
On 10/4/25 14:33, Robert Mader wrote: > Hi, > > On 10/1/25 10:10, Barnabás Pőcze wrote: >> Hi >> >> 2025. 09. 30. 14:42 keltezéssel, Robert Mader írta: >> >> I was testing this with `cam`, and it brought up an issue. Specifically, >> `ipa::soft::kMaxFrameContexts` is just 16, which is less than 32. So >> `cam`, >> which creates requests based on the number of buffers, may run into the >> following problem with more than 16 buffers: >> >> [0:04:25.186460320] [3298] FATAL FCQueue fc_queue.h:85 Frame >> context for 0 has been overwritten by 16 >> Backtrace: >> libcamera::ipa::FCQueue<libcamera::ipa::soft::IPAFrameContext>::get(unsigned >> int)+0x5ef (../src/ipa/libipa/fc_queue.h:85) >> libcamera::ipa::soft::IPASoftSimple::computeParams(unsigned >> int)+0x165 (../src/ipa/simple/soft_simple.cpp:294) >> libcamera::ipa::soft::IPAProxySoftThreaded::ThreadProxy::computeParams(unsigned >> int)+0x637 (include/libcamera/ipa/soft_ipa_proxy.h:125) >> ... >> So I think it would be nice to fix this as well. I believe we >> should pass >> 16 to the `PipelineHandler` base constructor as >> `maxQueuedRequestsDevice`. >> But it would be nice to be able to access that 16 constant somehow so >> that >> they don't go out of sync. >> `cam` patch for testing: > > Nice catch, thanks for testing! > > I haven't got around to test that solution yet, however assuming it'll > work I wonder: should we maybe be even more conservative and set > maxQueuedRequestsDevice to something like 4, in order to avoid > regressions? I.e. could there be valid reasons why drivers don't > actually handle that many queued requests? Went with a limit of 4 in v4 now. Barnabás: unfortunately I still didn't get around to reproduce your test case and 0.6 is close - do you think you could give it another test run to check if the issue is fixed?
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index c816cffc9..2dcba04ec 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 @@ -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,18 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() cfg.frameSize = format.planes[0].size; } - cfg.bufferCount = 4; + 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) + << "Adjusting bufferCount from " << bufferCount + << " to " << cfg.bufferCount; + status = Adjusted; + } } return status;