[{"id":16254,"web_url":"https://patchwork.libcamera.org/comment/16254/","msgid":"","date":"2021-04-13T22:20:20","subject":"Re: [libcamera-devel] [RFC PATCH 1/3] lc-compliance: Test queuing\n\tmore requests than hardware depth","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Nícolas,\n\nThanks for your work.\n\nOn 2021-04-09 18:38:13 -0300, Nícolas F. R. A. Prado wrote:\n> Currently some pipeline handlers allocate a fixed number of buffers for\n> internal usage, and if no internal buffer is available when a request is\n> received, it fails [1].\n\nI think the idea of a overflow test is really good! But I think your \napproach (and solution in 2/3) needs to be slightly modified. I will try \nto sum it up both sides of the problem here instead of splitting it \nbetween the two patches.\n\n* Provoking and detecting the problem in lc-compliance\n- It should probably be a new test and not extend the current tests with \n another dimension tripling the runtime. I think it makes more sens to \n have a single test to try and provoke overflow as this check does not \n really benefit from the existing start/stop tests.\n\n- Don't think the overflow test shall inform the camera that it will \n attempt to overflow it by configuring it for more internal buffers. It \n should use a BufferAllocator and allocate a _large_ amount of buffers \n and create a _large_ amount of requests. It should then queue all the \n requests as quickly as it can to the camera and check that it manages \n to process all of them without crashing.\n\n* Solution in pipeline handlers\n- I don't think the solution to this problem is to to increase the \n number of internal buffers used inside a pipeline handler, that is \n just playing wack-a-mole. Instead I think a queue of incoming requests \n shall be added and only N requests shall be poped of this queue and \n actively being processed by the pipeline handler / hardware. Once a \n request completes a new request may be poped of the queue.\n\n The number N will then become a matter of pipeline tuning. It needs to \n be large enough as to not stall the pipeline due to starvation while \n at the same time small enough to not waste memory. This is not so \n important for the RkISP1 pipeline as its internal buffers are quiet \n small, but look at the IPU3 pipeline which needs internal RAW buffers \n a large N will quickly consume a lot of memory.\n\n> \n> Extend the current lc-compliance tests so they also test sending more\n> simultaneous requests in order to detect that issue.\n> \n> [1] https://bugs.libcamera.org/show_bug.cgi?id=24\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> src/lc-compliance/simple_capture.cpp | 25 +++++++++++++++-------\n> src/lc-compliance/simple_capture.h | 2 +-\n> src/lc-compliance/single_stream.cpp | 31 ++++++++++++++++------------\n> 3 files changed, 37 insertions(+), 21 deletions(-)\n> \n> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> index 0ff720bfd005..61a8c0f40eef 100644\n> --- a/src/lc-compliance/simple_capture.cpp\n> +++ b/src/lc-compliance/simple_capture.cpp\n> @@ -18,10 +18,13 @@ SimpleCapture::~SimpleCapture()\n> {\n> }\n> \n> -Results::Result SimpleCapture::configure(StreamRole role)\n> +Results::Result SimpleCapture::configure(StreamRole role, unsigned int numBuffers)\n> {\n> \tconfig_ = camera_->generateConfiguration({ role });\n> \n> +\tif (numBuffers > 0)\n> +\t\tconfig_->at(0).bufferCount = numBuffers;\n> +\n> \tif (config_->validate() != CameraConfiguration::Valid) {\n> \t\tconfig_.reset();\n> \t\treturn { Results::Fail, \"Configuration not valid\" };\n> @@ -32,6 +35,10 @@ Results::Result SimpleCapture::configure(StreamRole role)\n> \t\treturn { Results::Fail, \"Failed to configure camera\" };\n> \t}\n> \n> +\tif (numBuffers > 0 && config_->at(0).bufferCount != numBuffers)\n> +\t\treturn { Results::Skip,\n> +\t\t\t\"Pipeline doesn't support overriding bufferCount\" };\n> +\n> \treturn { Results::Pass, \"Configure camera\" };\n> }\n> \n> @@ -77,14 +84,14 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> \n> \tStream *stream = config_->at(0).stream();\n> \tconst std::vector> &buffers = allocator_->buffers(stream);\n> +\t/* Cache buffers.size() before we destroy it in stop() */\n> +\tunsigned int bufSize = buffers.size();\n> \n> \t/* No point in testing less requests then the camera depth. */\n> -\tif (buffers.size() > numRequests) {\n> -\t\t/* Cache buffers.size() before we destroy it in stop() */\n> -\t\tint buffers_size = buffers.size();\n> +\tif (bufSize > numRequests) {\n> \t\tstop();\n> \n> -\t\treturn { Results::Skip, \"Camera needs \" + std::to_string(buffers_size)\n> +\t\treturn { Results::Skip, \"Camera needs \" + std::to_string(bufSize)\n> \t\t\t+ \" requests, can't test only \" + std::to_string(numRequests) };\n> \t}\n> \n> @@ -125,7 +132,8 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> \t\t\t\" request, wanted \" + std::to_string(captureLimit_) };\n> \n> \treturn { Results::Pass, \"Balanced capture of \" +\n> -\t\tstd::to_string(numRequests) + \" requests\" };\n> +\t\tstd::to_string(numRequests) + \" requests, using \" +\n> +\t\tstd::to_string(bufSize) + \" buffers\"};\n> }\n> \n> int SimpleCaptureBalanced::queueRequest(Request *request)\n> @@ -197,7 +205,10 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> \tstop();\n> \tdelete loop_;\n> \n> -\treturn { status ? Results::Fail : Results::Pass, \"Unbalanced capture of \" + std::to_string(numRequests) + \" requests\" };\n> +\treturn { status ? Results::Fail :\n> +\t\tResults::Pass, \"Unbalanced capture of \" +\n> +\t\tstd::to_string(numRequests) + \" requests, using \" +\n> +\t\tstd::to_string(buffers.size()) + \" buffers\"};\n> }\n> \n> void SimpleCaptureUnbalanced::requestComplete(Request *request)\n> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> index 4693c13404ce..f96fb3224290 100644\n> --- a/src/lc-compliance/simple_capture.h\n> +++ b/src/lc-compliance/simple_capture.h\n> @@ -17,7 +17,7 @@\n> class SimpleCapture\n> {\n> public:\n> -\tResults::Result configure(libcamera::StreamRole role);\n> +\tResults::Result configure(libcamera::StreamRole role, unsigned int numBuffers);\n> \n> protected:\n> \tSimpleCapture(std::shared_ptr camera);\n> diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp\n> index 60dacd63bf2b..9e33e231f4a9 100644\n> --- a/src/lc-compliance/single_stream.cpp\n> +++ b/src/lc-compliance/single_stream.cpp\n> @@ -14,11 +14,11 @@ using namespace libcamera;\n> \n> Results::Result testRequestBalance(std::shared_ptr camera,\n> \t\t\t\t StreamRole role, unsigned int startCycles,\n> -\t\t\t\t unsigned int numRequests)\n> +\t\t\t\t unsigned int numRequests, unsigned int numBuffers)\n> {\n> \tSimpleCaptureBalanced capture(camera);\n> \n> -\tResults::Result ret = capture.configure(role);\n> +\tResults::Result ret = capture.configure(role, numBuffers);\n> \tif (ret.first != Results::Pass)\n> \t\treturn ret;\n> \n> @@ -28,17 +28,17 @@ Results::Result testRequestBalance(std::shared_ptr camera,\n> \t\t\treturn ret;\n> \t}\n> \n> -\treturn { Results::Pass, \"Balanced capture of \" +\n> -\t\tstd::to_string(numRequests) + \" requests with \" +\n> +\treturn { Results::Pass, ret.second + \" and \" +\n> \t\tstd::to_string(startCycles) + \" start cycles\" };\n> }\n> \n> Results::Result testRequestUnbalance(std::shared_ptr camera,\n> -\t\t\t\t StreamRole role, unsigned int numRequests)\n> +\t\t\t\t StreamRole role, unsigned int numRequests,\n> +\t\t\t\t unsigned int numBuffers)\n> {\n> \tSimpleCaptureUnbalanced capture(camera);\n> \n> -\tResults::Result ret = capture.configure(role);\n> +\tResults::Result ret = capture.configure(role, numBuffers);\n> \tif (ret.first != Results::Pass)\n> \t\treturn ret;\n> \n> @@ -54,8 +54,10 @@ Results testSingleStream(std::shared_ptr camera)\n> \t\t{ \"viewfinder\", Viewfinder },\n> \t};\n> \tstatic const std::vector numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> +\t/* 0 means default */\n> +\tstatic const std::vector numBuffers = { 0, 5, 8 };\n> \n> -\tResults results(numRequests.size() * roles.size() * 3);\n> +\tResults results(numRequests.size() * roles.size() * numBuffers.size() * 3);\n> \n> \tfor (const auto &role : roles) {\n> \t\tstd::cout << \"= Test role \" << role.first << std::endl;\n> @@ -67,8 +69,9 @@ Results testSingleStream(std::shared_ptr camera)\n> \t\t * complete N requests to the application.\n> \t\t */\n> \t\tstd::cout << \"* Test single capture cycles\" << std::endl;\n> -\t\tfor (unsigned int num : numRequests)\n> -\t\t\tresults.add(testRequestBalance(camera, role.second, 1, num));\n> +\t\tfor (unsigned int buf : numBuffers)\n> +\t\t\tfor (unsigned int num : numRequests)\n> +\t\t\t\tresults.add(testRequestBalance(camera, role.second, 1, num, buf));\n> \n> \t\t/*\n> \t\t * Test multiple start/stop cycles\n> @@ -78,8 +81,9 @@ Results testSingleStream(std::shared_ptr camera)\n> \t\t * error path but is only tested by single-capture applications.\n> \t\t */\n> \t\tstd::cout << \"* Test multiple start/stop cycles\" << std::endl;\n> -\t\tfor (unsigned int num : numRequests)\n> -\t\t\tresults.add(testRequestBalance(camera, role.second, 3, num));\n> +\t\tfor (unsigned int buf : numBuffers)\n> +\t\t\tfor (unsigned int num : numRequests)\n> +\t\t\t\tresults.add(testRequestBalance(camera, role.second, 3, num, buf));\n> \n> \t\t/*\n> \t\t * Test unbalanced stop\n> @@ -89,8 +93,9 @@ Results testSingleStream(std::shared_ptr camera)\n> \t\t * of buffers coming back from the video device while stopping.\n> \t\t */\n> \t\tstd::cout << \"* Test unbalanced stop\" << std::endl;\n> -\t\tfor (unsigned int num : numRequests)\n> -\t\t\tresults.add(testRequestUnbalance(camera, role.second, num));\n> +\t\tfor (unsigned int buf : numBuffers)\n> +\t\t\tfor (unsigned int num : numRequests)\n> +\t\t\t\tresults.add(testRequestUnbalance(camera, role.second, num, buf));\n> \t}\n> \n> \treturn results;\n> -- \n> 2.31.1\n>","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 AB421BD1F6\n\tfor ;\n\tTue, 13 Apr 2021 22:20:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FE5068800;\n\tWed, 14 Apr 2021 00:20:24 +0200 (CEST)","from mail-lf1-x134.google.com (mail-lf1-x134.google.com\n\t[IPv6:2a00:1450:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDD27602CD\n\tfor ;\n\tWed, 14 Apr 2021 00:20:22 +0200 (CEST)","by mail-lf1-x134.google.com with SMTP id e14so17066599lfn.11\n\tfor ;\n\tTue, 13 Apr 2021 15:20:22 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tq3sm3735513lfu.304.2021.04.13.15.20.21\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 13 Apr 2021 15:20:21 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"B/GovyQ+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=RoYRFCcK6Q4nkvK1uD78JOsAwN2yF45676HGhr65pb4=;\n\tb=B/GovyQ+2gHbtLk5trs84bi0IYKLRuBPuoAfCnl4D8GEWgBGtvMrBtmmErWJEO/csM\n\tyOhfhzf6XXR3xSc9bVE8glmSmNQ3STBoSpyogLP4njqPgCtsV857f5lfhfwIBW7BzjJc\n\teO1TVgBpHaWibdywx47IGy2hx2nBIjOo7zpNDdlgKmWio/LAeJJBECLAccEr9ZeeUHxW\n\tZKxlYMVNkv8ysi9Rmm1P5cWcYnhlz236dRRr/PzrtAE4M4+oXwmBJGxcZbnbmp9Dd41s\n\tf3LhrTBNneWAC7XELh+KrVYL6gP06wXcOCTWYFInF3dqSvp1QwytEpdeUQhtbfMwaKwO\n\tP1Tg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=RoYRFCcK6Q4nkvK1uD78JOsAwN2yF45676HGhr65pb4=;\n\tb=D78rqyjLjZ9NROs0FTutdWyaW6eFR+0RNom3KAb7/rlvLdqr/4z7SWqMmK7KQ7gnlB\n\tej1eDgPLSJSvARjnIhvSIqowW7OI57hytVVPqTw14qqIXOlPoisX3X2DIRQ8Wj9+yvys\n\tGkJPXSvdAEzfrCBo+Y/pmPvqbrkunSuiqfe5uQR9yb3PEbAmH0d6tN5PkfYXkohcYZv8\n\tAxHkpg3YXXXbsTO/JhcqAk8hXIxj3h4gV3IY7nIPuKb/kUwPA+7GtNIMtiF30VHjL7zU\n\tWZuC+cqeSLL0Fq5DDO7J0N0AB1UXQJbywjZ1Z3UKlw3goMuGf85mNum6+1Zr8yUkju6H\n\tgNBg==","X-Gm-Message-State":"AOAM530oRGlSM65KZZUAOsJmrquDryxotSAUUBZGqPGK3FcyMPP7XKyc\n\tRt6oeZ8w6//RlnL55EMrgiz0XA==","X-Google-Smtp-Source":"ABdhPJy6XHKLeTtEqjGNy+9CKHnBXdVNBGgqeXgBulFTBqqJEE8zWJdj8jo8ajRNzvzoY2EDFbbvwA==","X-Received":"by 2002:ac2:5330:: with SMTP id\n\tf16mr14805119lfh.269.1618352421970; \n\tTue, 13 Apr 2021 15:20:21 -0700 (PDT)","Date":"Wed, 14 Apr 2021 00:20:20 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"=?iso-8859-1?q?N=EDcolas_F=2E_R=2E_A=2E?= Prado ","Message-ID":"","References":"<20210409213815.356837-1-nfraprado@collabora.com>\n\t<20210409213815.356837-2-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210409213815.356837-2-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 1/3] lc-compliance: Test queuing\n\tmore 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":"Collabora Kernel ML ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]