[{"id":18466,"web_url":"https://patchwork.libcamera.org/comment/18466/","msgid":"","date":"2021-08-02T00:13:56","subject":"Re: [libcamera-devel] [PATCH v7 07/11] lc-compliance: Add test to\n\tqueue more requests than hardware depth","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nícolas,\n\nThank you for the patch.\n\nOn Thu, Jul 22, 2021 at 08:28:47PM -0300, Nícolas F. R. A. Prado wrote:\n> A pipeline handler should be able to handle an arbitrary number of\n> simultaneous requests by submitting what it can to the video device and\n> queuing the rest internally until resources are available. This isn't\n> currently done by some pipeline handlers however [1].\n> \n> Add a new test to lc-compliance that submits a lot of requests at once\n> to check if the pipeline handler is behaving well in this situation.\n> \n> [1] https://bugs.libcamera.org/show_bug.cgi?id=24\n\nThe commit message may need to be updated depending on what gets merged\nfirst :-)\n\n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> \n> No changes in v7\n> \n> Changes in v6:\n> - Made MAX_SIMULTANEOUS_REQUESTS a constexpr\n> \n> Changes in v5:\n> - Updated to use googletest, and CameraHolder and roleToString from previous\n> patches\n> \n> src/lc-compliance/capture_test.cpp | 45 ++++++++++++++++++++++++++++\n> src/lc-compliance/simple_capture.cpp | 30 +++++++++++++++++++\n> src/lc-compliance/simple_capture.h | 11 +++++++\n> 3 files changed, 86 insertions(+)\n> \n> diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp\n> index b4807486ee07..a7ba7448a21b 100644\n> --- a/src/lc-compliance/capture_test.cpp\n> +++ b/src/lc-compliance/capture_test.cpp\n> @@ -18,6 +18,8 @@ using namespace libcamera;\n> const std::vector NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> const std::vector ROLES = { Raw, StillCapture, VideoRecording, Viewfinder };\n> \n> +static constexpr unsigned int MAX_SIMULTANEOUS_REQUESTS = 8;\n> +\n> static const std::string &roleToString(const StreamRole &role)\n> {\n> \tstatic std::map rolesMap = { { Raw, \"Raw\" },\n> @@ -79,12 +81,37 @@ void SingleStream::TearDown()\n> \treleaseCamera();\n> }\n> \n> +class RoleParametrizedTest : public testing::TestWithParam, public CameraHolder\n\nMaybe it's due to my bad understanding of the test framework, but the\nname of this class confuses me. I understand it as a base class for\ntests that are parametrized by roles, but below it's used directly as\nthe test fixture when creating tests.\n\n> +{\n> +public:\n> +\tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> +\n> +protected:\n> +\tvoid SetUp() override;\n> +\tvoid TearDown() override;\n> +};\n> +\n> +void RoleParametrizedTest::SetUp()\n> +{\n> +\tacquireCamera();\n> +}\n> +\n> +void RoleParametrizedTest::TearDown()\n> +{\n> +\treleaseCamera();\n> +}\n> +\n> std::string SingleStream::nameParameters(const testing::TestParamInfo &info)\n> {\n> \treturn roleToString(std::get<0>(info.param)) + \"_\" +\n> \t std::to_string(std::get<1>(info.param));\n> }\n> \n> +std::string RoleParametrizedTest::nameParameters(const testing::TestParamInfo &info)\n> +{\n> +\treturn roleToString(info.param);\n> +}\n> +\n> /*\n> * Test single capture cycles\n> *\n> @@ -147,8 +174,26 @@ TEST_P(SingleStream, UnbalancedStop)\n> \tcapture.capture(numRequests);\n> }\n> \n> +TEST_P(RoleParametrizedTest, Overflow)\n> +{\n> +\tauto role = GetParam();\n> +\n> +\tSimpleCaptureOverflow capture(camera_);\n> +\n> +\tcapture.configure(role);\n> +\n> +\tcapture.allocateBuffers(MAX_SIMULTANEOUS_REQUESTS);\n> +\n> +\tcapture.capture();\n> +}\n> +\n> INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> \t\t\t SingleStream,\n> \t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> \t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> \t\t\t SingleStream::nameParameters);\n> +\n> +INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> +\t\t\t RoleParametrizedTest,\n> +\t\t\t testing::ValuesIn(ROLES),\n> +\t\t\t RoleParametrizedTest::nameParameters);\n> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> index 06ef44ef7e42..48ce8f088e71 100644\n> --- a/src/lc-compliance/simple_capture.cpp\n> +++ b/src/lc-compliance/simple_capture.cpp\n> @@ -220,3 +220,33 @@ void SimpleCaptureUnbalanced::requestComplete(Request *request)\n> \tif (camera_->queueRequest(request))\n> \t\tloop_->exit(-EINVAL);\n> }\n> +\n> +/* SimpleCaptureOverflow */\n> +\n> +SimpleCaptureOverflow::SimpleCaptureOverflow(std::shared_ptr camera)\n> +\t: SimpleCapture(camera)\n> +{\n> +}\n> +\n> +void SimpleCaptureOverflow::capture()\n> +{\n> +\tstart();\n> +\n> +\tStream *stream = config_->at(0).stream();\n> +\tconst std::vector> &buffers = allocator_->buffers(stream);\n> +\n> +\tcaptureCount_ = 0;\n> +\tcaptureLimit_ = buffers.size();\n> +\n> +\tstd::vector> requests;\n> +\tqueueRequests(stream, buffers, requests);\n> +\n> +\trunCaptureSession();\n> +\n> +\tASSERT_EQ(captureCount_, captureLimit_);\n> +}\n> +\n> +void SimpleCaptureOverflow::requestComplete([[maybe_unused]] Request *request)\n> +{\n> +\tcaptureCompleted();\n> +}\n> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> index 0f9a060fece3..2f4960584642 100644\n> --- a/src/lc-compliance/simple_capture.h\n> +++ b/src/lc-compliance/simple_capture.h\n> @@ -72,4 +72,15 @@ private:\n> \tvoid requestComplete(libcamera::Request *request) override;\n> };\n> \n> +class SimpleCaptureOverflow : public SimpleCapture\n> +{\n> +public:\n> +\tSimpleCaptureOverflow(std::shared_ptr camera);\n> +\n> +\tvoid capture();\n> +\n> +private:\n> +\tvoid requestComplete(libcamera::Request *request) override;\n> +};\n> +\n> #endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AD827C3232\n\tfor ;\n\tMon, 2 Aug 2021 00:14:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A370687BD;\n\tMon, 2 Aug 2021 02:14:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3615E687B6\n\tfor ;\n\tMon, 2 Aug 2021 02:14:06 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A99C187C;\n\tMon, 2 Aug 2021 02:14:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VkezY/qG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627863245;\n\tbh=Y9tPHYgN0i7DNy/+fbACDUPseNBP4x4LC5eixZOsm/E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VkezY/qGnyCPyfOlHy7CvJkMJFjkQwGp2/n8AnACgf5L7wFxJFhqEumOJHiRlINQM\n\tZLzvb1XTWawjRHmnWQqJ2u6PVZ95Jj+cauRBpnIAq2Qyq/BJHw9ZAo2MGoXybPpyUs\n\tPzsEymFfZkm3xbBcVsiCzsFMfe/cfretl96daSBw=","Date":"Mon, 2 Aug 2021 03:13:56 +0300","From":"Laurent Pinchart ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"","References":"<20210722232851.747614-1-nfraprado@collabora.com>\n\t<20210722232851.747614-8-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210722232851.747614-8-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v7 07/11] lc-compliance: Add test to\n\tqueue more requests than hardware depth","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18990,"web_url":"https://patchwork.libcamera.org/comment/18990/","msgid":"<20210820212431.uen3dj5cmps76jzu@notapiano>","date":"2021-08-20T21:24:31","subject":"Re: [libcamera-devel] [PATCH v7 07/11] lc-compliance: Add test to\n\tqueue more requests than hardware depth","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Laurent,\n\nOn Mon, Aug 02, 2021 at 03:13:56AM +0300, Laurent Pinchart wrote:\n> Hi Nícolas,\n> \n> Thank you for the patch.\n> \n> On Thu, Jul 22, 2021 at 08:28:47PM -0300, Nícolas F. R. A. Prado wrote:\n> > A pipeline handler should be able to handle an arbitrary number of\n> > simultaneous requests by submitting what it can to the video device and\n> > queuing the rest internally until resources are available. This isn't\n> > currently done by some pipeline handlers however [1].\n> > \n> > Add a new test to lc-compliance that submits a lot of requests at once\n> > to check if the pipeline handler is behaving well in this situation.\n> > \n> > [1] https://bugs.libcamera.org/show_bug.cgi?id=24\n> \n> The commit message may need to be updated depending on what gets merged\n> first :-)\n\nIndeed, I'll keep an eye on that ;).\n\n> \n> > Signed-off-by: Nícolas F. R. A. Prado \n> > ---\n> > \n> > No changes in v7\n> > \n> > Changes in v6:\n> > - Made MAX_SIMULTANEOUS_REQUESTS a constexpr\n> > \n> > Changes in v5:\n> > - Updated to use googletest, and CameraHolder and roleToString from previous\n> > patches\n> > \n> > src/lc-compliance/capture_test.cpp | 45 ++++++++++++++++++++++++++++\n> > src/lc-compliance/simple_capture.cpp | 30 +++++++++++++++++++\n> > src/lc-compliance/simple_capture.h | 11 +++++++\n> > 3 files changed, 86 insertions(+)\n> > \n> > diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp\n> > index b4807486ee07..a7ba7448a21b 100644\n> > --- a/src/lc-compliance/capture_test.cpp\n> > +++ b/src/lc-compliance/capture_test.cpp\n> > @@ -18,6 +18,8 @@ using namespace libcamera;\n> > const std::vector NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> > const std::vector ROLES = { Raw, StillCapture, VideoRecording, Viewfinder };\n> > \n> > +static constexpr unsigned int MAX_SIMULTANEOUS_REQUESTS = 8;\n> > +\n> > static const std::string &roleToString(const StreamRole &role)\n> > {\n> > \tstatic std::map rolesMap = { { Raw, \"Raw\" },\n> > @@ -79,12 +81,37 @@ void SingleStream::TearDown()\n> > \treleaseCamera();\n> > }\n> > \n> > +class RoleParametrizedTest : public testing::TestWithParam, public CameraHolder\n> \n> Maybe it's due to my bad understanding of the test framework, but the\n> name of this class confuses me. I understand it as a base class for\n> tests that are parametrized by roles, but below it's used directly as\n> the test fixture when creating tests.\n\nI'm not sure I get the concern, but a better name is certainly welcome. I tried\nto give it a name that conveyed exactly what it is, which is like you said, a\nbase class for tests that are parametrized by roles. Maybe still not the best\nname, but I feel it's too early to know which kind of tests will subclass this\nclass to give it a better name.\n\nIn any case see my comments below, perhaps it clears any misunderstanding.\n\n> \n> > +{\n> > +public:\n> > +\tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> > +\n> > +protected:\n> > +\tvoid SetUp() override;\n> > +\tvoid TearDown() override;\n> > +};\n> > +\n> > +void RoleParametrizedTest::SetUp()\n> > +{\n> > +\tacquireCamera();\n> > +}\n> > +\n> > +void RoleParametrizedTest::TearDown()\n> > +{\n> > +\treleaseCamera();\n> > +}\n> > +\n> > std::string SingleStream::nameParameters(const testing::TestParamInfo &info)\n> > {\n> > \treturn roleToString(std::get<0>(info.param)) + \"_\" +\n> > \t std::to_string(std::get<1>(info.param));\n> > }\n> > \n> > +std::string RoleParametrizedTest::nameParameters(const testing::TestParamInfo &info)\n> > +{\n> > +\treturn roleToString(info.param);\n> > +}\n> > +\n> > /*\n> > * Test single capture cycles\n> > *\n> > @@ -147,8 +174,26 @@ TEST_P(SingleStream, UnbalancedStop)\n> > \tcapture.capture(numRequests);\n> > }\n> > \n> > +TEST_P(RoleParametrizedTest, Overflow)\n> > +{\n> > +\tauto role = GetParam();\n> > +\n> > +\tSimpleCaptureOverflow capture(camera_);\n> > +\n> > +\tcapture.configure(role);\n> > +\n> > +\tcapture.allocateBuffers(MAX_SIMULTANEOUS_REQUESTS);\n> > +\n> > +\tcapture.capture();\n> > +}\n\nThis defines the Overflow test, which subclasses RoleParametrizedTest through\nthis definition. This means that the Overflow test should receive a role\nparameter, which is accessible through GetParam().\n\n> > +\n> > INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > \t\t\t SingleStream,\n> > \t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> > \t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> > \t\t\t SingleStream::nameParameters);\n> > +\n> > +INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > +\t\t\t RoleParametrizedTest,\n> > +\t\t\t testing::ValuesIn(ROLES),\n> > +\t\t\t RoleParametrizedTest::nameParameters);\n\nThis instantiates all tests that subclass RoleParametrizedTest by creating a\ntest for each of the available roles. This means that if we add another test,\nOverflow2, also subclassing RoleParametrizedTest, it will also get instantiated\nfor each of the roles through this same call. This is already currently done\nwith SingleStream above, which instantiates tests for Capture, CaptureStartStop,\nUnbalancedStop all at once. (Btw, the CaptureTests name is mostly irrelevant)\n\nHopefully this solves any misunderstanding. If not we can discuss a better name\nfor the class :).\n\nThanks,\nNícolas\n\n> > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> > index 06ef44ef7e42..48ce8f088e71 100644\n> > --- a/src/lc-compliance/simple_capture.cpp\n> > +++ b/src/lc-compliance/simple_capture.cpp\n> > @@ -220,3 +220,33 @@ void SimpleCaptureUnbalanced::requestComplete(Request *request)\n> > \tif (camera_->queueRequest(request))\n> > \t\tloop_->exit(-EINVAL);\n> > }\n> > +\n> > +/* SimpleCaptureOverflow */\n> > +\n> > +SimpleCaptureOverflow::SimpleCaptureOverflow(std::shared_ptr camera)\n> > +\t: SimpleCapture(camera)\n> > +{\n> > +}\n> > +\n> > +void SimpleCaptureOverflow::capture()\n> > +{\n> > +\tstart();\n> > +\n> > +\tStream *stream = config_->at(0).stream();\n> > +\tconst std::vector> &buffers = allocator_->buffers(stream);\n> > +\n> > +\tcaptureCount_ = 0;\n> > +\tcaptureLimit_ = buffers.size();\n> > +\n> > +\tstd::vector> requests;\n> > +\tqueueRequests(stream, buffers, requests);\n> > +\n> > +\trunCaptureSession();\n> > +\n> > +\tASSERT_EQ(captureCount_, captureLimit_);\n> > +}\n> > +\n> > +void SimpleCaptureOverflow::requestComplete([[maybe_unused]] Request *request)\n> > +{\n> > +\tcaptureCompleted();\n> > +}\n> > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> > index 0f9a060fece3..2f4960584642 100644\n> > --- a/src/lc-compliance/simple_capture.h\n> > +++ b/src/lc-compliance/simple_capture.h\n> > @@ -72,4 +72,15 @@ private:\n> > \tvoid requestComplete(libcamera::Request *request) override;\n> > };\n> > \n> > +class SimpleCaptureOverflow : public SimpleCapture\n> > +{\n> > +public:\n> > +\tSimpleCaptureOverflow(std::shared_ptr camera);\n> > +\n> > +\tvoid capture();\n> > +\n> > +private:\n> > +\tvoid requestComplete(libcamera::Request *request) override;\n> > +};\n> > +\n> > #endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AA4CFBD87C\n\tfor ;\n\tFri, 20 Aug 2021 21:24:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D1484688A2;\n\tFri, 20 Aug 2021 23:24:40 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E536D6888E\n\tfor ;\n\tFri, 20 Aug 2021 23:24:38 +0200 (CEST)","from notapiano (unknown [IPv6:2804:14c:1a9:2434:b693:c9:5cb6:b688])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\t(Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 297571F4494B;\n\tFri, 20 Aug 2021 22:24:36 +0100 (BST)"],"Date":"Fri, 20 Aug 2021 18:24:31 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","To":"Laurent Pinchart ","Message-ID":"<20210820212431.uen3dj5cmps76jzu@notapiano>","References":"<20210722232851.747614-1-nfraprado@collabora.com>\n\t<20210722232851.747614-8-nfraprado@collabora.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v7 07/11] lc-compliance: Add test to\n\tqueue more requests than hardware depth","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]