Patch Detail
Show a patch.
GET /api/1.1/patches/23278/?format=api
{ "id": 23278, "url": "https://patchwork.libcamera.org/api/1.1/patches/23278/?format=api", "web_url": "https://patchwork.libcamera.org/patch/23278/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20250428090413.38234-7-s.pueschel@pengutronix.de>", "date": "2025-04-28T09:02:31", "name": "[v11,06/19] libcamera: pipeline: simple: Don't rely on bufferCount", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "07b236d11e2d1d0fd4529fb924582ab55f846ac5", "submitter": { "id": 225, "url": "https://patchwork.libcamera.org/api/1.1/people/225/?format=api", "name": "Sven Püschel", "email": "s.pueschel@pengutronix.de" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/23278/mbox/", "series": [ { "id": 5148, "url": "https://patchwork.libcamera.org/api/1.1/series/5148/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=5148", "date": "2025-04-28T09:02:25", "name": "lc-compliance: Add test to queue more requests than hardware depth", "version": 11, "mbox": "https://patchwork.libcamera.org/series/5148/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/23278/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/23278/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "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 702A3C3320\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Apr 2025 09:05:22 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C34AF68B31;\n\tMon, 28 Apr 2025 11:05:19 +0200 (CEST)", "from metis.whiteo.stw.pengutronix.de\n\t(metis.whiteo.stw.pengutronix.de [IPv6:2a0a:edc0:2:b01:1d::104])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16FEF68AD7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Apr 2025 11:05:07 +0200 (CEST)", "from ptz.office.stw.pengutronix.de ([2a0a:edc0:0:900:1d::77]\n\thelo=peter.guest.stw.pengutronix.de)\n\tby metis.whiteo.stw.pengutronix.de with esmtp (Exim 4.92)\n\t(envelope-from <s.pueschel@pengutronix.de>)\n\tid 1u9KQE-0001au-Mx; Mon, 28 Apr 2025 11:05:06 +0200" ], "From": "=?utf-8?q?Sven_P=C3=BCschel?= <s.pueschel@pengutronix.de>", "To": "libcamera-devel@lists.libcamera.org", "Cc": "=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= <nfraprado@collabora.com>,\n\tPaul Elder <paul.elder@ideasonboard.com>, =?utf-8?q?Sven_P=C3=BCschel?=\n\t<s.pueschel@pengutronix.de>", "Subject": "[PATCH v11 06/19] libcamera: pipeline: simple: Don't rely on\n\tbufferCount", "Date": "Mon, 28 Apr 2025 11:02:31 +0200", "Message-ID": "<20250428090413.38234-7-s.pueschel@pengutronix.de>", "X-Mailer": "git-send-email 2.49.0", "In-Reply-To": "<20250428090413.38234-1-s.pueschel@pengutronix.de>", "References": "<20250428090413.38234-1-s.pueschel@pengutronix.de>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=UTF-8", "Content-Transfer-Encoding": "8bit", "X-SA-Exim-Connect-IP": "2a0a:edc0:0:900:1d::77", "X-SA-Exim-Mail-From": "s.pueschel@pengutronix.de", "X-SA-Exim-Scanned": "No (on metis.whiteo.stw.pengutronix.de);\n\tSAEximRunCond expanded to false", "X-PTX-Original-Recipient": "libcamera-devel@lists.libcamera.org", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "From: Nícolas F. R. A. Prado <nfraprado@collabora.com>\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 <nfraprado@collabora.com>\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\nSigned-off-by: Paul Elder <paul.elder@ideasonboard.com>\nSigned-off-by: Sven Püschel <s.pueschel@pengutronix.de>\n\n---\nChanges in v11:\n- added rkisp1 adjustments for the dewarper\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/rkisp1/rkisp1.cpp | 2 +-\n src/libcamera/pipeline/simple/simple.cpp | 21 ++++++++++++++-----\n 6 files changed, 29 insertions(+), 14 deletions(-)", "diff": "diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\nindex 644ec429..1da51d1e 100644\n--- a/include/libcamera/internal/converter.h\n+++ b/include/libcamera/internal/converter.h\n@@ -75,7 +75,7 @@ public:\n \tvirtual int exportBuffers(const Stream *stream, unsigned int count,\n \t\t\t\t std::vector<std::unique_ptr<FrameBuffer>> *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 0ad7bf7f..24997c6f 100644\n--- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n+++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n@@ -59,7 +59,7 @@ public:\n \tint exportBuffers(const Stream *stream, unsigned int count,\n \t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;\n \n-\tint start() override;\n+\tint start(unsigned int inputBufferCount) override;\n \tvoid stop() override;\n \n \tint validateOutput(StreamConfiguration *cfg, bool *adjusted,\n@@ -85,7 +85,7 @@ private:\n \t\tint exportBuffers(unsigned int count,\n \t\t\t\t std::vector<std::unique_ptr<FrameBuffer>> *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 d551b908..2c6f1e4b 100644\n--- a/src/libcamera/converter.cpp\n+++ b/src/libcamera/converter.cpp\n@@ -190,6 +190,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 ee05b798..242e0d85 100644\n--- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n+++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n@@ -160,13 +160,15 @@ int V4L2M2MConverter::V4L2M2MStream::exportBuffers(unsigned int count,\n \treturn m2m_->capture()->exportBuffers(count, buffers);\n }\n \n-int V4L2M2MConverter::V4L2M2MStream::start()\n+int V4L2M2MConverter::V4L2M2MStream::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@@ -620,12 +622,12 @@ V4L2M2MConverter::inputCropBounds(const Stream *stream)\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 (auto &iter : streams_) {\n-\t\tret = iter.second->start();\n+\t\tret = iter.second->start(inputBufferCount);\n \t\tif (ret < 0) {\n \t\t\tstop();\n \t\t\treturn ret;\ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\nindex 81bf1c55..623bcfe5 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n@@ -1110,7 +1110,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n \t\tactions += [&]() { stat_->streamOff(); };\n \n \t\tif (useDewarper_) {\n-\t\t\tret = dewarper_->start();\n+\t\t\tret = dewarper_->start(kRkISP1InternalBufferCount);\n \t\t\tif (ret) {\n \t\t\t\tLOG(RkISP1, Error) << \"Failed to start dewarper\";\n \t\t\t\treturn ret;\ndiff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\nindex 1e77ccdc..4f4cba06 100644\n--- a/src/libcamera/pipeline/simple/simple.cpp\n+++ b/src/libcamera/pipeline/simple/simple.cpp\n@@ -422,6 +422,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<V4L2VideoDevice> video;\n@@ -1466,17 +1467,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->useConversion_) {\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->conversionBuffers_);\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@@ -1504,7 +1515,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n \n \tif (data->useConversion_) {\n \t\tif (data->converter_)\n-\t\t\tret = data->converter_->start();\n+\t\t\tret = data->converter_->start(internalBufferCount);\n \t\telse if (data->swIsp_)\n \t\t\tret = data->swIsp_->start();\n \t\telse\n", "prefixes": [ "v11", "06/19" ] }