[libcamera-devel] [PATCH v3 3/4] lc-compliance: Add test to queue more requests than hardware depth

[Niklas Söderlund]

Hi "Nícolas,

Thanks for your work.

On 2021-04-21 13:51:38 -0300, Nícolas F. R. A. Prado wrote:
> A pipeline handler should be able to handle an arbitrary amount of
> simultaneous requests by submitting what it can to the video device and
> queuing the rest internally until resources are available. This isn't
> currently done by some pipeline handlers however [1].
> 
> Add a new test to lc-compliance that submits a lot of requests at once
> to check if the pipeline handler is behaving well in this situation.
> 
> [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 | 70 ++++++++++++++++++++++++++++
>  src/lc-compliance/simple_capture.h   | 15 ++++++
>  src/lc-compliance/single_stream.cpp  | 31 +++++++++++-
>  3 files changed, 115 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index 875772a80c27..01d76380e998 100644
> --- a/src/lc-compliance/simple_capture.cpp
> +++ b/src/lc-compliance/simple_capture.cpp
> @@ -223,3 +223,73 @@ void SimpleCaptureUnbalanced::requestComplete(Request *request)
>  	if (camera_->queueRequest(request))
>  		loop_->exit(-EINVAL);
>  }
> +
> +SimpleCaptureOverflow::SimpleCaptureOverflow(std::shared_ptr<Camera> camera)
> +	: SimpleCapture(camera)
> +{
> +}
> +
> +Results::Result SimpleCaptureOverflow::capture()
> +{
> +	Results::Result ret = start();
> +	if (ret.first != Results::Pass)
> +		return ret;
> +
> +	Stream *stream = config_->at(0).stream();
> +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> +
> +	captureCount_ = 0;
> +	captureLimit_ = buffers.size();
> +
> +	std::vector<std::unique_ptr<libcamera::Request>> requests;
> +	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> +		std::unique_ptr<Request> request = camera_->createRequest();
> +		if (!request) {
> +			stop();
> +			return { Results::Fail, "Can't create request" };
> +		}
> +
> +		if (request->addBuffer(stream, buffer.get())) {
> +			stop();
> +			return { Results::Fail, "Can't set buffer for request" };
> +		}
> +
> +		if (camera_->queueRequest(request.get()) < 0) {
> +			stop();
> +			return { Results::Fail, "Failed to queue request" };
> +		}
> +
> +		requests.push_back(std::move(request));
> +	}
> +
> +	/* Run capture session. */
> +	loop_ = new EventLoop();
> +	loop_->exec();
> +	stop();
> +	delete loop_;
> +
> +	if (captureCount_ != captureLimit_)
> +		return { Results::Fail, "Got " + std::to_string(captureCount_) +
> +			" request, wanted " + std::to_string(captureLimit_) };
> +
> +	return { Results::Pass, "Overflow capture of " +
> +		std::to_string(captureLimit_) + " requests" };
> +}

Reading this I now see there is quiet a bit of duplication between 
SimpleCapture*::capture() implementations. This existed before this 
patch and can be addressed on-top.

> +
> +void SimpleCaptureOverflow::requestComplete([[maybe_unused]]Request *request)
> +{
> +	captureCount_++;
> +	if (captureCount_ >= captureLimit_) {
> +		loop_->exit(0);
> +		return;
> +	}
> +}

Like above I now see this is could be moved to SimpleCapture without 
much work and the code can then be shared between all SimpleCapture* 
tests. Maybe this one is so straight forward you could do it as a pre 
patch to this one? If you like it can also be done on-top.

> +
> +Results::Result SimpleCaptureOverflow::allocateBuffers(unsigned int count)
> +{
> +	Stream *stream = config_->at(0).stream();
> +	if (allocator_->allocate(stream, count) < 0)
> +		return { Results::Fail, "Failed to allocate buffers" };
> +
> +	return { Results::Pass, "Allocated buffers" };
> +}
> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h
> index 82e2c56a55f1..cafcdafe10c2 100644
> --- a/src/lc-compliance/simple_capture.h
> +++ b/src/lc-compliance/simple_capture.h
> @@ -66,4 +66,19 @@ private:
>  	unsigned int captureLimit_;
>  };
>  
> +class SimpleCaptureOverflow : public SimpleCapture
> +{
> +public:
> +	SimpleCaptureOverflow(std::shared_ptr<libcamera::Camera> camera);
> +
> +	Results::Result allocateBuffers(unsigned int count);
> +	Results::Result capture();
> +
> +private:
> +	void requestComplete(libcamera::Request *request) override;
> +
> +	unsigned int captureCount_;
> +	unsigned int captureLimit_;
> +};
> +
>  #endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */
> diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp
> index 649291c7bb73..61979f66d92d 100644
> --- a/src/lc-compliance/single_stream.cpp
> +++ b/src/lc-compliance/single_stream.cpp
> @@ -12,6 +12,23 @@
>  
>  using namespace libcamera;
>  
> +static const unsigned int MAX_SIMULTANEOUS_REQUESTS = 8;
> +
> +Results::Result testRequestOverflow(std::shared_ptr<Camera> camera,
> +				   StreamRole role, unsigned int numRequests)
> +{
> +	SimpleCaptureOverflow capture(camera);
> +
> +	Results::Result ret = capture.configure(role);
> +	if (ret.first != Results::Pass)
> +		return ret;
> +	capture.allocateBuffers(numRequests);
> +	if (ret.first != Results::Pass)
> +		return ret;
> +
> +	return capture.capture();
> +}
> +
>  Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
>  				   StreamRole role, unsigned int startCycles,
>  				   unsigned int numRequests)
> @@ -61,7 +78,7 @@ Results testSingleStream(std::shared_ptr<Camera> camera)
>  	};
>  	static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
>  
> -	Results results(numRequests.size() * roles.size() * 3);
> +	Results results(numRequests.size() * roles.size() * 3 + roles.size() * 1);

I think you should drop the '* 1' at the end.

>  
>  	for (const auto &role : roles) {
>  		std::cout << "= Test role " << role.first << std::endl;
> @@ -97,6 +114,18 @@ Results testSingleStream(std::shared_ptr<Camera> camera)
>  		std::cout << "* Test unbalanced stop" << std::endl;
>  		for (unsigned int num : numRequests)
>  			results.add(testRequestUnbalance(camera, role.second, num));
> +
> +		/*
> +		 * Test overflowing pipeline with requests
> +		 *
> +		 * Makes sure that the camera supports receiving a large amount
> +		 * of requests at once. Example failure is a camera that doesn't
> +		 * check if there are available resources (free internal
> +		 * buffers, free buffers in the video devices) before handling a
> +		 * request.
> +		 */
> +		std::cout << "* Test overflowing requests" << std::endl;
> +		results.add(testRequestOverflow(camera, role.second, MAX_SIMULTANEOUS_REQUESTS));
>  	}
>  
>  	return results;
> -- 
> 2.31.1
> 

-- 
Regards,
Niklas Söderlund