[libcamera-devel] [RFC PATCH 1/3] lc-compliance: Test queuing more requests than hardware depth
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Apr 14 00:20:20 CEST 2021
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 at 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
>
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list