| Message ID | 20210409213815.356837-2-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Nícolas, Thanks for your work. On 2021-04-09 18:38:13 -0300, Nícolas F. R. A. Prado wrote: > Currently some pipeline handlers allocate a fixed number of buffers for > internal usage, and if no internal buffer is available when a request is > received, it fails [1]. I think the idea of a overflow test is really good! But I think your approach (and solution in 2/3) needs to be slightly modified. I will try to sum it up both sides of the problem here instead of splitting it between the two patches. * Provoking and detecting the problem in lc-compliance - It should probably be a new test and not extend the current tests with another dimension tripling the runtime. I think it makes more sens to have a single test to try and provoke overflow as this check does not really benefit from the existing start/stop tests. - Don't think the overflow test shall inform the camera that it will attempt to overflow it by configuring it for more internal buffers. It should use a BufferAllocator and allocate a _large_ amount of buffers and create a _large_ amount of requests. It should then queue all the requests as quickly as it can to the camera and check that it manages to process all of them without crashing. * Solution in pipeline handlers - I don't think the solution to this problem is to to increase the number of internal buffers used inside a pipeline handler, that is just playing wack-a-mole. Instead I think a queue of incoming requests shall be added and only N requests shall be poped of this queue and actively being processed by the pipeline handler / hardware. Once a request completes a new request may be poped of the queue. The number N will then become a matter of pipeline tuning. It needs to be large enough as to not stall the pipeline due to starvation while at the same time small enough to not waste memory. This is not so important for the RkISP1 pipeline as its internal buffers are quiet small, but look at the IPU3 pipeline which needs internal RAW buffers a large N will quickly consume a lot of memory. > > Extend the current lc-compliance tests so they also test sending more > simultaneous requests in order to detect that issue. > > [1] https://bugs.libcamera.org/show_bug.cgi?id=24 > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > src/lc-compliance/simple_capture.cpp | 25 +++++++++++++++------- > src/lc-compliance/simple_capture.h | 2 +- > src/lc-compliance/single_stream.cpp | 31 ++++++++++++++++------------ > 3 files changed, 37 insertions(+), 21 deletions(-) > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp > index 0ff720bfd005..61a8c0f40eef 100644 > --- a/src/lc-compliance/simple_capture.cpp > +++ b/src/lc-compliance/simple_capture.cpp > @@ -18,10 +18,13 @@ SimpleCapture::~SimpleCapture() > { > } > > -Results::Result SimpleCapture::configure(StreamRole role) > +Results::Result SimpleCapture::configure(StreamRole role, unsigned int numBuffers) > { > config_ = camera_->generateConfiguration({ role }); > > + if (numBuffers > 0) > + config_->at(0).bufferCount = numBuffers; > + > if (config_->validate() != CameraConfiguration::Valid) { > config_.reset(); > return { Results::Fail, "Configuration not valid" }; > @@ -32,6 +35,10 @@ Results::Result SimpleCapture::configure(StreamRole role) > return { Results::Fail, "Failed to configure camera" }; > } > > + if (numBuffers > 0 && config_->at(0).bufferCount != numBuffers) > + return { Results::Skip, > + "Pipeline doesn't support overriding bufferCount" }; > + > return { Results::Pass, "Configure camera" }; > } > > @@ -77,14 +84,14 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > Stream *stream = config_->at(0).stream(); > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > + /* Cache buffers.size() before we destroy it in stop() */ > + unsigned int bufSize = buffers.size(); > > /* No point in testing less requests then the camera depth. */ > - if (buffers.size() > numRequests) { > - /* Cache buffers.size() before we destroy it in stop() */ > - int buffers_size = buffers.size(); > + if (bufSize > numRequests) { > stop(); > > - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) > + return { Results::Skip, "Camera needs " + std::to_string(bufSize) > + " requests, can't test only " + std::to_string(numRequests) }; > } > > @@ -125,7 +132,8 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > " request, wanted " + std::to_string(captureLimit_) }; > > return { Results::Pass, "Balanced capture of " + > - std::to_string(numRequests) + " requests" }; > + std::to_string(numRequests) + " requests, using " + > + std::to_string(bufSize) + " buffers"}; > } > > int SimpleCaptureBalanced::queueRequest(Request *request) > @@ -197,7 +205,10 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > stop(); > delete loop_; > > - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; > + return { status ? Results::Fail : > + Results::Pass, "Unbalanced capture of " + > + std::to_string(numRequests) + " requests, using " + > + std::to_string(buffers.size()) + " buffers"}; > } > > void SimpleCaptureUnbalanced::requestComplete(Request *request) > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h > index 4693c13404ce..f96fb3224290 100644 > --- a/src/lc-compliance/simple_capture.h > +++ b/src/lc-compliance/simple_capture.h > @@ -17,7 +17,7 @@ > class SimpleCapture > { > public: > - Results::Result configure(libcamera::StreamRole role); > + Results::Result configure(libcamera::StreamRole role, unsigned int numBuffers); > > protected: > SimpleCapture(std::shared_ptr<libcamera::Camera> camera); > diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp > index 60dacd63bf2b..9e33e231f4a9 100644 > --- a/src/lc-compliance/single_stream.cpp > +++ b/src/lc-compliance/single_stream.cpp > @@ -14,11 +14,11 @@ using namespace libcamera; > > Results::Result testRequestBalance(std::shared_ptr<Camera> camera, > StreamRole role, unsigned int startCycles, > - unsigned int numRequests) > + unsigned int numRequests, unsigned int numBuffers) > { > SimpleCaptureBalanced capture(camera); > > - Results::Result ret = capture.configure(role); > + Results::Result ret = capture.configure(role, numBuffers); > if (ret.first != Results::Pass) > return ret; > > @@ -28,17 +28,17 @@ Results::Result testRequestBalance(std::shared_ptr<Camera> camera, > return ret; > } > > - return { Results::Pass, "Balanced capture of " + > - std::to_string(numRequests) + " requests with " + > + return { Results::Pass, ret.second + " and " + > std::to_string(startCycles) + " start cycles" }; > } > > Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, > - StreamRole role, unsigned int numRequests) > + StreamRole role, unsigned int numRequests, > + unsigned int numBuffers) > { > SimpleCaptureUnbalanced capture(camera); > > - Results::Result ret = capture.configure(role); > + Results::Result ret = capture.configure(role, numBuffers); > if (ret.first != Results::Pass) > return ret; > > @@ -54,8 +54,10 @@ Results testSingleStream(std::shared_ptr<Camera> camera) > { "viewfinder", Viewfinder }, > }; > static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > + /* 0 means default */ > + static const std::vector<unsigned int> numBuffers = { 0, 5, 8 }; > > - Results results(numRequests.size() * roles.size() * 3); > + Results results(numRequests.size() * roles.size() * numBuffers.size() * 3); > > for (const auto &role : roles) { > std::cout << "= Test role " << role.first << std::endl; > @@ -67,8 +69,9 @@ Results testSingleStream(std::shared_ptr<Camera> camera) > * complete N requests to the application. > */ > std::cout << "* Test single capture cycles" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestBalance(camera, role.second, 1, num)); > + for (unsigned int buf : numBuffers) > + for (unsigned int num : numRequests) > + results.add(testRequestBalance(camera, role.second, 1, num, buf)); > > /* > * Test multiple start/stop cycles > @@ -78,8 +81,9 @@ Results testSingleStream(std::shared_ptr<Camera> camera) > * error path but is only tested by single-capture applications. > */ > std::cout << "* Test multiple start/stop cycles" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestBalance(camera, role.second, 3, num)); > + for (unsigned int buf : numBuffers) > + for (unsigned int num : numRequests) > + results.add(testRequestBalance(camera, role.second, 3, num, buf)); > > /* > * Test unbalanced stop > @@ -89,8 +93,9 @@ Results testSingleStream(std::shared_ptr<Camera> camera) > * of buffers coming back from the video device while stopping. > */ > std::cout << "* Test unbalanced stop" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestUnbalance(camera, role.second, num)); > + for (unsigned int buf : numBuffers) > + for (unsigned int num : numRequests) > + results.add(testRequestUnbalance(camera, role.second, num, buf)); > } > > return results; > -- > 2.31.1 >
diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp index 0ff720bfd005..61a8c0f40eef 100644 --- a/src/lc-compliance/simple_capture.cpp +++ b/src/lc-compliance/simple_capture.cpp @@ -18,10 +18,13 @@ SimpleCapture::~SimpleCapture() { } -Results::Result SimpleCapture::configure(StreamRole role) +Results::Result SimpleCapture::configure(StreamRole role, unsigned int numBuffers) { config_ = camera_->generateConfiguration({ role }); + if (numBuffers > 0) + config_->at(0).bufferCount = numBuffers; + if (config_->validate() != CameraConfiguration::Valid) { config_.reset(); return { Results::Fail, "Configuration not valid" }; @@ -32,6 +35,10 @@ Results::Result SimpleCapture::configure(StreamRole role) return { Results::Fail, "Failed to configure camera" }; } + if (numBuffers > 0 && config_->at(0).bufferCount != numBuffers) + return { Results::Skip, + "Pipeline doesn't support overriding bufferCount" }; + return { Results::Pass, "Configure camera" }; } @@ -77,14 +84,14 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) Stream *stream = config_->at(0).stream(); const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); + /* Cache buffers.size() before we destroy it in stop() */ + unsigned int bufSize = buffers.size(); /* No point in testing less requests then the camera depth. */ - if (buffers.size() > numRequests) { - /* Cache buffers.size() before we destroy it in stop() */ - int buffers_size = buffers.size(); + if (bufSize > numRequests) { stop(); - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) + return { Results::Skip, "Camera needs " + std::to_string(bufSize) + " requests, can't test only " + std::to_string(numRequests) }; } @@ -125,7 +132,8 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) " request, wanted " + std::to_string(captureLimit_) }; return { Results::Pass, "Balanced capture of " + - std::to_string(numRequests) + " requests" }; + std::to_string(numRequests) + " requests, using " + + std::to_string(bufSize) + " buffers"}; } int SimpleCaptureBalanced::queueRequest(Request *request) @@ -197,7 +205,10 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) stop(); delete loop_; - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; + return { status ? Results::Fail : + Results::Pass, "Unbalanced capture of " + + std::to_string(numRequests) + " requests, using " + + std::to_string(buffers.size()) + " buffers"}; } void SimpleCaptureUnbalanced::requestComplete(Request *request) diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h index 4693c13404ce..f96fb3224290 100644 --- a/src/lc-compliance/simple_capture.h +++ b/src/lc-compliance/simple_capture.h @@ -17,7 +17,7 @@ class SimpleCapture { public: - Results::Result configure(libcamera::StreamRole role); + Results::Result configure(libcamera::StreamRole role, unsigned int numBuffers); protected: SimpleCapture(std::shared_ptr<libcamera::Camera> camera); diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp index 60dacd63bf2b..9e33e231f4a9 100644 --- a/src/lc-compliance/single_stream.cpp +++ b/src/lc-compliance/single_stream.cpp @@ -14,11 +14,11 @@ using namespace libcamera; Results::Result testRequestBalance(std::shared_ptr<Camera> camera, StreamRole role, unsigned int startCycles, - unsigned int numRequests) + unsigned int numRequests, unsigned int numBuffers) { SimpleCaptureBalanced capture(camera); - Results::Result ret = capture.configure(role); + Results::Result ret = capture.configure(role, numBuffers); if (ret.first != Results::Pass) return ret; @@ -28,17 +28,17 @@ Results::Result testRequestBalance(std::shared_ptr<Camera> camera, return ret; } - return { Results::Pass, "Balanced capture of " + - std::to_string(numRequests) + " requests with " + + return { Results::Pass, ret.second + " and " + std::to_string(startCycles) + " start cycles" }; } Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, - StreamRole role, unsigned int numRequests) + StreamRole role, unsigned int numRequests, + unsigned int numBuffers) { SimpleCaptureUnbalanced capture(camera); - Results::Result ret = capture.configure(role); + Results::Result ret = capture.configure(role, numBuffers); if (ret.first != Results::Pass) return ret; @@ -54,8 +54,10 @@ Results testSingleStream(std::shared_ptr<Camera> camera) { "viewfinder", Viewfinder }, }; static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; + /* 0 means default */ + static const std::vector<unsigned int> numBuffers = { 0, 5, 8 }; - Results results(numRequests.size() * roles.size() * 3); + Results results(numRequests.size() * roles.size() * numBuffers.size() * 3); for (const auto &role : roles) { std::cout << "= Test role " << role.first << std::endl; @@ -67,8 +69,9 @@ Results testSingleStream(std::shared_ptr<Camera> camera) * complete N requests to the application. */ std::cout << "* Test single capture cycles" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestBalance(camera, role.second, 1, num)); + for (unsigned int buf : numBuffers) + for (unsigned int num : numRequests) + results.add(testRequestBalance(camera, role.second, 1, num, buf)); /* * Test multiple start/stop cycles @@ -78,8 +81,9 @@ Results testSingleStream(std::shared_ptr<Camera> camera) * error path but is only tested by single-capture applications. */ std::cout << "* Test multiple start/stop cycles" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestBalance(camera, role.second, 3, num)); + for (unsigned int buf : numBuffers) + for (unsigned int num : numRequests) + results.add(testRequestBalance(camera, role.second, 3, num, buf)); /* * Test unbalanced stop @@ -89,8 +93,9 @@ Results testSingleStream(std::shared_ptr<Camera> camera) * of buffers coming back from the video device while stopping. */ std::cout << "* Test unbalanced stop" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestUnbalance(camera, role.second, num)); + for (unsigned int buf : numBuffers) + for (unsigned int num : numRequests) + results.add(testRequestUnbalance(camera, role.second, num, buf)); } return results;
Currently some pipeline handlers allocate a fixed number of buffers for internal usage, and if no internal buffer is available when a request is received, it fails [1]. Extend the current lc-compliance tests so they also test sending more simultaneous requests in order to detect that issue. [1] https://bugs.libcamera.org/show_bug.cgi?id=24 Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- src/lc-compliance/simple_capture.cpp | 25 +++++++++++++++------- src/lc-compliance/simple_capture.h | 2 +- src/lc-compliance/single_stream.cpp | 31 ++++++++++++++++------------ 3 files changed, 37 insertions(+), 21 deletions(-)