{"id":18062,"url":"https://patchwork.libcamera.org/api/patches/18062/?format=json","web_url":"https://patchwork.libcamera.org/patch/18062/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20221228223003.2265712-7-paul.elder@ideasonboard.com>","date":"2022-12-28T22:29:50","name":"[libcamera-devel,v10,06/19] libcamera: pipeline: simple: Don't rely on bufferCount","commit_ref":null,"pull_url":null,"state":"superseded","archived":false,"hash":"7dbf6591c958ae5948112482c9dd736a51526d5e","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/?format=json","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/18062/mbox/","series":[{"id":3691,"url":"https://patchwork.libcamera.org/api/series/3691/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=3691","date":"2022-12-28T22:29:44","name":"lc-compliance: Add test to queue more requests than hardware depth","version":10,"mbox":"https://patchwork.libcamera.org/series/3691/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/18062/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/18062/checks/","tags":{},"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 23DD4C3226\n\tfor ;\n\tWed, 28 Dec 2022 22:30:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5E18625DF;\n\tWed, 28 Dec 2022 23:30:28 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 98A44625D7\n\tfor ;\n\tWed, 28 Dec 2022 23:30:24 +0100 (CET)","from pyrite.mediacom.info (unknown\n\t[IPv6:2604:2d80:ad8a:9000:1bf9:855b:22de:3645])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B674109;\n\tWed, 28 Dec 2022 23:30:23 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1672266628;\n\tbh=fm5mBlWrh1ps5b66vFDjfPs9kLP9NmvtJw6JPr2+r0g=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=DYCj6q3Pa9uKnWylWp70yEnDKXNeHzfmaQd4YsQiB7KqalOMeg2y5TxVJA45FD7pj\n\tVgllIw5CCb0aqdH0hiaUpM0kqkMERzutO8uRZ78cU+Qiqf2wFvdeRKzW/Bz/R+yivb\n\t1WkpwZ328m+dsAnntvn3Y8AiDYmT51V7kCiSxZVmzZ8jLnfGk3xu8cRVuRosInhF5W\n\teSZ/SloSNdd4VPJESM89NarlnHfO3LaBhzsk/TJdpK38JnsaGJUzbSqwU4oZDPGMZP\n\tKC4WChBq1uR6TcxuUm3QRDeEYKZQoVJOfSbTn/e3CchXvbpbfvw9ehzlXcFUqgFqGK\n\tgFrQ+2uhVBefA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1672266624;\n\tbh=fm5mBlWrh1ps5b66vFDjfPs9kLP9NmvtJw6JPr2+r0g=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=Yhxe+qvkjFCuQK/UnfSk88dCPB9medHtXtZ7grNoUp9qJAcNHmPEQw+/65f8QRwmW\n\tMPEpYRofSufyCD5a4TDOgjex7/WTH1N2Y4vWNSKt8oLqq+1N2SKTWvXTBrKUEfLKIF\n\t688OJw9haYrjTIwvbLnuFHbK9F674JqoROgLOuEw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Yhxe+qvk\"; dkim-atps=neutral","To":"libcamera-devel@lists.libcamera.org","Date":"Wed, 28 Dec 2022 16:29:50 -0600","Message-Id":"<20221228223003.2265712-7-paul.elder@ideasonboard.com>","X-Mailer":"git-send-email 2.35.1","In-Reply-To":"<20221228223003.2265712-1-paul.elder@ideasonboard.com>","References":"<20221228223003.2265712-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit","Subject":"[libcamera-devel] [PATCH v10 06/19] libcamera: pipeline: simple:\n\tDon't rely on bufferCount","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","From":"Paul Elder via libcamera-devel ","Reply-To":"Paul Elder ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "},"content":"From: Nícolas F. R. A. Prado \n\nCurrently the simple pipeline handler relies on bufferCount to decide on\nthe number of buffers to allocate internally when a converter is in use\nand for the number of V4L2 buffer slots to reserve. Instead, the number\nof internal buffers should be the minimum required by the pipeline to\nkeep the requests flowing, in order to avoid wasting memory, while the\nnumber of V4L2 buffer slots should be greater than the expected number\nof requests queued by the application, in order to avoid thrashing\ndmabuf mappings, which would degrade performance.\n\nStop relying on bufferCount for these numbers and instead set them to\nappropriate, and independent, constants.\n\nSigned-off-by: Nícolas F. R. A. Prado \nReviewed-by: Paul Elder \nSigned-off-by: Paul Elder \n\n---\nChanges in v10:\n- rename internalBufferCount and bufferSlotCount to inputBufferCount and\n outputBufferCount, respectively\n- remove outputBufferCount from Converter::start() parameter\n - instead, each Converter will decide on their own (how do we like\n this?)\n\nChanges in v9:\n- rebased\n\nChanges in v8:\n- New\n---\n include/libcamera/internal/converter.h | 2 +-\n .../internal/converter/converter_v4l2_m2m.h | 4 ++--\n src/libcamera/converter.cpp | 2 ++\n .../converter/converter_v4l2_m2m.cpp | 12 ++++++-----\n src/libcamera/pipeline/simple/simple.cpp | 21 ++++++++++++++-----\n 5 files changed, 28 insertions(+), 13 deletions(-)","diff":"diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\nindex 834ec5ab..e8a5aeca 100644\n--- a/include/libcamera/internal/converter.h\n+++ b/include/libcamera/internal/converter.h\n@@ -49,7 +49,7 @@ public:\n \tvirtual int exportBuffers(unsigned int output, unsigned int count,\n \t\t\t\t std::vector> *buffers) = 0;\n \n-\tvirtual int start() = 0;\n+\tvirtual int start(unsigned int inputBufferCount) = 0;\n \tvirtual void stop() = 0;\n \n \tvirtual int queueBuffers(FrameBuffer *input,\ndiff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\nindex 815916d0..8aedb6df 100644\n--- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n+++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n@@ -50,7 +50,7 @@ public:\n \tint exportBuffers(unsigned int ouput, unsigned int count,\n \t\t\t std::vector> *buffers);\n \n-\tint start();\n+\tint start(unsigned int inputBufferCount);\n \tvoid stop();\n \n \tint queueBuffers(FrameBuffer *input,\n@@ -69,7 +69,7 @@ private:\n \t\tint exportBuffers(unsigned int count,\n \t\t\t\t std::vector> *buffers);\n \n-\t\tint start();\n+\t\tint start(unsigned int inputBufferCount);\n \t\tvoid stop();\n \n \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\ndiff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\nindex 3de39cff..2413424b 100644\n--- a/src/libcamera/converter.cpp\n+++ b/src/libcamera/converter.cpp\n@@ -127,6 +127,8 @@ Converter::~Converter()\n /**\n * \\fn Converter::start()\n * \\brief Start the converter streaming operation\n+ * \\param[in] inputBufferCount Number of input buffers that will be used to\n+ * move video capture device frames into the converter.\n * \\return 0 on success or a negative error code otherwise\n */\n \ndiff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\nindex 2a4d1d99..261c3329 100644\n--- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n+++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n@@ -107,13 +107,15 @@ int V4L2M2MConverter::Stream::exportBuffers(unsigned int count,\n \treturn m2m_->capture()->exportBuffers(count, buffers);\n }\n \n-int V4L2M2MConverter::Stream::start()\n+int V4L2M2MConverter::Stream::start(unsigned int inputBufferCount)\n {\n-\tint ret = m2m_->output()->importBuffers(inputBufferCount_);\n+\tstatic constexpr unsigned int kOutputBufferCount = 16;\n+\n+\tint ret = m2m_->output()->importBuffers(inputBufferCount);\n \tif (ret < 0)\n \t\treturn ret;\n \n-\tret = m2m_->capture()->importBuffers(outputBufferCount_);\n+\tret = m2m_->capture()->importBuffers(kOutputBufferCount);\n \tif (ret < 0) {\n \t\tstop();\n \t\treturn ret;\n@@ -373,12 +375,12 @@ int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,\n /**\n * \\copydoc libcamera::Converter::start\n */\n-int V4L2M2MConverter::start()\n+int V4L2M2MConverter::start(unsigned int inputBufferCount)\n {\n \tint ret;\n \n \tfor (Stream &stream : streams_) {\n-\t\tret = stream.start();\n+\t\tret = stream.start(inputBufferCount);\n \t\tif (ret < 0) {\n \t\t\tstop();\n \t\t\treturn ret;\ndiff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\nindex 6b7c6d5c..64f341e1 100644\n--- a/src/libcamera/pipeline/simple/simple.cpp\n+++ b/src/libcamera/pipeline/simple/simple.cpp\n@@ -339,6 +339,7 @@ protected:\n \n private:\n \tstatic constexpr unsigned int kNumInternalBuffers = 3;\n+\tstatic constexpr unsigned int kSimpleBufferSlotCount = 16;\n \n \tstruct EntityData {\n \t\tstd::unique_ptr video;\n@@ -1232,17 +1233,27 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n \t\treturn -EBUSY;\n \t}\n \n+\t/*\n+\t * Number of internal buffers that will be used to move video capture\n+\t * device frames to the converter.\n+\t *\n+\t * \\todo Make this depend on the driver in use instead of being\n+\t * hardcoded. In order to not drop frames, the realtime requirements for\n+\t * each device should be checked, so it may not be as simple as just\n+\t * using the minimum number of buffers required by the driver.\n+\t */\n+\tstatic constexpr unsigned int internalBufferCount = 3;\n+\n \tif (data->useConverter_) {\n \t\t/*\n \t\t * When using the converter allocate a fixed number of internal\n \t\t * buffers.\n \t\t */\n-\t\tret = video->allocateBuffers(kNumInternalBuffers,\n+\t\tret = video->allocateBuffers(internalBufferCount,\n \t\t\t\t\t &data->converterBuffers_);\n \t} else {\n-\t\t/* Otherwise, prepare for using buffers from the only stream. */\n-\t\tStream *stream = &data->streams_[0];\n-\t\tret = video->importBuffers(stream->configuration().bufferCount);\n+\t\t/* Otherwise, prepare for using buffers. */\n+\t\tret = video->importBuffers(kSimpleBufferSlotCount);\n \t}\n \tif (ret < 0) {\n \t\treleasePipeline(data);\n@@ -1258,7 +1269,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n \t}\n \n \tif (data->useConverter_) {\n-\t\tret = data->converter_->start();\n+\t\tret = data->converter_->start(internalBufferCount);\n \t\tif (ret < 0) {\n \t\t\tstop(camera);\n \t\t\treturn ret;\n","prefixes":["libcamera-devel","v10","06/19"]}