{"id":13466,"url":"https://patchwork.libcamera.org/api/1.1/patches/13466/?format=json","web_url":"https://patchwork.libcamera.org/patch/13466/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/1.1/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":"<20210824195636.1110845-6-nfraprado@collabora.com>","date":"2021-08-24T19:56:24","name":"[libcamera-devel,v8,05/17] libcamera: pipeline: simple: Don't rely on bufferCount","commit_ref":null,"pull_url":null,"state":"superseded","archived":false,"hash":"5a059a9f0e6374376753d2ec077d8fb70dc58519","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/1.1/people/84/?format=json","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/13466/mbox/","series":[{"id":2389,"url":"https://patchwork.libcamera.org/api/1.1/series/2389/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=2389","date":"2021-08-24T19:56:19","name":"lc-compliance: Add test to queue more requests than hardware depth","version":8,"mbox":"https://patchwork.libcamera.org/series/2389/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/13466/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/13466/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 BFD33C3245\n\tfor ;\n\tTue, 24 Aug 2021 19:57:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E16468911;\n\tTue, 24 Aug 2021 21:57:28 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3989268892\n\tfor ;\n\tTue, 24 Aug 2021 21:57:27 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nfraprado) with ESMTPSA id C9F481F4339D"],"From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","To":"libcamera-devel@lists.libcamera.org","Date":"Tue, 24 Aug 2021 16:56:24 -0300","Message-Id":"<20210824195636.1110845-6-nfraprado@collabora.com>","X-Mailer":"git-send-email 2.33.0","In-Reply-To":"<20210824195636.1110845-1-nfraprado@collabora.com>","References":"<20210824195636.1110845-1-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit","Subject":"[libcamera-devel] [PATCH v8 05/17] 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","Cc":"kernel@collabora.com, =?utf-8?q?Andr=C3=A9_Almeida?=\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "},"content":"Currently 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 \n\n---\n\nChanges in v8:\n- New\n\n src/libcamera/pipeline/simple/converter.cpp | 12 ++++++-----\n src/libcamera/pipeline/simple/converter.h | 6 ++++--\n src/libcamera/pipeline/simple/simple.cpp | 22 ++++++++++++++++-----\n 3 files changed, 28 insertions(+), 12 deletions(-)","diff":"diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp\nindex b5e34c4cd0c5..3133b3dbda07 100644\n--- a/src/libcamera/pipeline/simple/converter.cpp\n+++ b/src/libcamera/pipeline/simple/converter.cpp\n@@ -101,13 +101,14 @@ int SimpleConverter::Stream::exportBuffers(unsigned int count,\n \treturn m2m_->capture()->exportBuffers(count, buffers);\n }\n \n-int SimpleConverter::Stream::start()\n+int SimpleConverter::Stream::start(unsigned int internalBufferCount,\n+\t\t\t\t unsigned int bufferSlotCount)\n {\n-\tint ret = m2m_->output()->importBuffers(inputBufferCount_);\n+\tint ret = m2m_->output()->importBuffers(internalBufferCount);\n \tif (ret < 0)\n \t\treturn ret;\n \n-\tret = m2m_->capture()->importBuffers(outputBufferCount_);\n+\tret = m2m_->capture()->importBuffers(bufferSlotCount);\n \tif (ret < 0) {\n \t\tstop();\n \t\treturn ret;\n@@ -331,12 +332,13 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,\n \treturn streams_[output].exportBuffers(count, buffers);\n }\n \n-int SimpleConverter::start()\n+int SimpleConverter::start(unsigned int internalBufferCount,\n+\t\t\t unsigned int bufferSlotCount)\n {\n \tint ret;\n \n \tfor (Stream &stream : streams_) {\n-\t\tret = stream.start();\n+\t\tret = stream.start(internalBufferCount, bufferSlotCount);\n \t\tif (ret < 0) {\n \t\t\tstop();\n \t\t\treturn ret;\ndiff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h\nindex 276a2a291c21..deb3df0d08df 100644\n--- a/src/libcamera/pipeline/simple/converter.h\n+++ b/src/libcamera/pipeline/simple/converter.h\n@@ -47,7 +47,8 @@ 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 internalBufferCount,\n+\t\t unsigned int bufferSlotCount);\n \tvoid stop();\n \n \tint queueBuffers(FrameBuffer *input,\n@@ -69,7 +70,8 @@ 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 internalBufferCount,\n+\t\t\t unsigned int bufferSlotCount);\n \t\tvoid stop();\n \n \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\ndiff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\nindex 737815452bae..d0a658a23be8 100644\n--- a/src/libcamera/pipeline/simple/simple.cpp\n+++ b/src/libcamera/pipeline/simple/simple.cpp\n@@ -253,6 +253,7 @@ protected:\n \n private:\n \tstatic constexpr unsigned int kNumInternalBuffers = 3;\n+\tstatic constexpr unsigned int kSimpleBufferSlotCount = 16;\n \n \tSimpleCameraData *cameraData(Camera *camera)\n \t{\n@@ -830,17 +831,27 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n \tV4L2VideoDevice *video = data->video_;\n \tint ret;\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\treturn ret;\n@@ -852,7 +863,8 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n \t}\n \n \tif (data->useConverter_) {\n-\t\tret = converter_->start();\n+\t\tret = converter_->start(internalBufferCount,\n+\t\t\t\t\tkSimpleBufferSlotCount);\n \t\tif (ret < 0) {\n \t\t\tstop(camera);\n \t\t\treturn ret;\n","prefixes":["libcamera-devel","v8","05/17"]}