{"id":23276,"url":"https://patchwork.libcamera.org/api/patches/23276/?format=json","web_url":"https://patchwork.libcamera.org/patch/23276/","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":"<20250428090413.38234-5-s.pueschel@pengutronix.de>","date":"2025-04-28T09:02:29","name":"[v11,04/19] libcamera: pipeline: ipu3: Don't rely on bufferCount","commit_ref":null,"pull_url":null,"state":"superseded","archived":false,"hash":"0f547e9fd05cead4e438fba7a76e738919adb9a7","submitter":{"id":225,"url":"https://patchwork.libcamera.org/api/people/225/?format=json","name":"Sven Püschel","email":"s.pueschel@pengutronix.de"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/23276/mbox/","series":[{"id":5148,"url":"https://patchwork.libcamera.org/api/series/5148/?format=json","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/23276/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/23276/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 7935FC331E\n\tfor ;\n\tMon, 28 Apr 2025 09:05:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38C0068B26;\n\tMon, 28 Apr 2025 11:05:16 +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 AC1F568AD5\n\tfor ;\n\tMon, 28 Apr 2025 11:05:06 +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 )\n\tid 1u9KQE-0001au-AL; Mon, 28 Apr 2025 11:05:06 +0200"],"From":"=?utf-8?q?Sven_P=C3=BCschel?= ","To":"libcamera-devel@lists.libcamera.org","Cc":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ,\n\tPaul Elder , =?utf-8?q?Sven_P=C3=BCschel?=\n\t","Subject":"[PATCH v11 04/19] libcamera: pipeline: ipu3: Don't rely on\n\tbufferCount","Date":"Mon, 28 Apr 2025 11:02:29 +0200","Message-ID":"<20250428090413.38234-5-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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "},"content":"From: Nícolas F. R. A. Prado \n\nCurrently the ipu3 pipeline handler relies on bufferCount to decide on\nthe number of V4L2 buffer slots to reserve as well as for the number of\nbuffers to allocate internally for the CIO2 raw buffers and\nparameter-statistics ImgU buffer pairs. Instead, the number of internal\nbuffers should be the minimum required by the pipeline to keep the\nrequests flowing without frame drops, in order to avoid wasting memory,\nwhile the number of V4L2 buffer slots should be greater than the\nexpected number of requests queued by the application, in order to avoid\nthrashing dmabuf 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 \nSigned-off-by: Sven Püschel \n\n---\nChanges in v11:\n- rebased\n\nChanges in v10:\n- make hardcoded internal buffer allocation counts for CIO2 and IMGU\n come from MinimumRequests\n\nChanges in v9:\n- rebase\n\nChanges in v8:\n- New\n---\n src/libcamera/pipeline/ipu3/cio2.cpp | 8 +++---\n src/libcamera/pipeline/ipu3/cio2.h | 4 +--\n src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++----\n src/libcamera/pipeline/ipu3/imgu.h | 3 +-\n src/libcamera/pipeline/ipu3/ipu3.cpp | 43 ++++++++++++++++++++--------\n 5 files changed, 45 insertions(+), 26 deletions(-)","diff":"diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\nindex aa544d7b..5a2c5644 100644\n--- a/src/libcamera/pipeline/ipu3/cio2.cpp\n+++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n@@ -236,7 +236,6 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const\n \n \tcfg.size = sensorFormat.size;\n \tcfg.pixelFormat = mbusCodesToPixelFormat.at(sensorFormat.code);\n-\tcfg.bufferCount = kBufferCount;\n \n \treturn cfg;\n }\n@@ -337,13 +336,14 @@ int CIO2Device::exportBuffers(unsigned int count,\n \treturn output_->exportBuffers(count, buffers);\n }\n \n-int CIO2Device::start()\n+int CIO2Device::start(unsigned int internalBufferCount,\n+\t\t unsigned int bufferSlotCount)\n {\n-\tint ret = output_->exportBuffers(kBufferCount, &buffers_);\n+\tint ret = output_->exportBuffers(internalBufferCount, &buffers_);\n \tif (ret < 0)\n \t\treturn ret;\n \n-\tret = output_->importBuffers(kBufferCount);\n+\tret = output_->importBuffers(bufferSlotCount);\n \tif (ret)\n \t\tLOG(IPU3, Error) << \"Failed to import CIO2 buffers\";\n \ndiff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\nindex 963c2f6b..3c6cc8bc 100644\n--- a/src/libcamera/pipeline/ipu3/cio2.h\n+++ b/src/libcamera/pipeline/ipu3/cio2.h\n@@ -31,8 +31,6 @@ enum class Transform;\n class CIO2Device\n {\n public:\n-\tstatic constexpr unsigned int kBufferCount = 4;\n-\n \tCIO2Device();\n \n \tstd::vector formats() const;\n@@ -50,7 +48,7 @@ public:\n \tV4L2SubdeviceFormat getSensorFormat(const std::vector &mbusCodes,\n \t\t\t\t\t const Size &size) const;\n \n-\tint start();\n+\tint start(unsigned int internalBufferCount, unsigned int bufferSlotCount);\n \tint stop();\n \n \tCameraSensor *sensor() { return sensor_.get(); }\ndiff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\nindex 7be78091..50b981f2 100644\n--- a/src/libcamera/pipeline/ipu3/imgu.cpp\n+++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n@@ -576,22 +576,23 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,\n /**\n * \\brief Allocate buffers for all the ImgU video devices\n */\n-int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n+int ImgUDevice::allocateBuffers(unsigned int internalBufferCount,\n+\t\t\t\tunsigned int bufferSlotCount)\n {\n \t/* Share buffers between CIO2 output and ImgU input. */\n-\tint ret = input_->importBuffers(bufferCount);\n+\tint ret = input_->importBuffers(bufferSlotCount);\n \tif (ret) {\n \t\tLOG(IPU3, Error) << \"Failed to import ImgU input buffers\";\n \t\treturn ret;\n \t}\n \n-\tret = param_->allocateBuffers(bufferCount, ¶mBuffers_);\n+\tret = param_->allocateBuffers(internalBufferCount, ¶mBuffers_);\n \tif (ret < 0) {\n \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU param buffers\";\n \t\tgoto error;\n \t}\n \n-\tret = stat_->allocateBuffers(bufferCount, &statBuffers_);\n+\tret = stat_->allocateBuffers(internalBufferCount, &statBuffers_);\n \tif (ret < 0) {\n \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU stat buffers\";\n \t\tgoto error;\n@@ -602,13 +603,13 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)\n \t * corresponding stream is active or inactive, as the driver needs\n \t * buffers to be requested on the V4L2 devices in order to operate.\n \t */\n-\tret = output_->importBuffers(bufferCount);\n+\tret = output_->importBuffers(bufferSlotCount);\n \tif (ret < 0) {\n \t\tLOG(IPU3, Error) << \"Failed to import ImgU output buffers\";\n \t\tgoto error;\n \t}\n \n-\tret = viewfinder_->importBuffers(bufferCount);\n+\tret = viewfinder_->importBuffers(bufferSlotCount);\n \tif (ret < 0) {\n \t\tLOG(IPU3, Error) << \"Failed to import ImgU viewfinder buffers\";\n \t\tgoto error;\ndiff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h\nindex fa508316..47d6ed0c 100644\n--- a/src/libcamera/pipeline/ipu3/imgu.h\n+++ b/src/libcamera/pipeline/ipu3/imgu.h\n@@ -84,7 +84,8 @@ public:\n \t\t\t\t\t outputFormat);\n \t}\n \n-\tint allocateBuffers(unsigned int bufferCount);\n+\tint allocateBuffers(unsigned int internalBufferCount,\n+\t\t\t unsigned int bufferSlotCount);\n \tvoid freeBuffers();\n \n \tint start();\ndiff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\nindex 30b74d4e..665466c7 100644\n--- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n@@ -159,7 +159,7 @@ private:\n \tint updateControls(IPU3CameraData *data);\n \tint registerCameras();\n \n-\tint allocateBuffers(Camera *camera);\n+\tint allocateBuffers(Camera *camera, unsigned int bufferSlotCount);\n \tint freeBuffers(Camera *camera);\n \n \tImgUDevice imgu0_;\n@@ -170,6 +170,7 @@ private:\n \tstd::vector ipaBuffers_;\n \n \tstatic constexpr unsigned int kMinimumRequests = 3;\n+\tstatic constexpr unsigned int kIPU3BufferSlotCount = 16;\n };\n \n IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data)\n@@ -660,20 +661,25 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n * In order to be able to start the 'viewfinder' and 'stat' nodes, we need\n * memory to be reserved.\n */\n-int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n+int PipelineHandlerIPU3::allocateBuffers(Camera *camera,\n+\t\t\t\t\t unsigned int bufferSlotCount)\n {\n \tIPU3CameraData *data = cameraData(camera);\n \tImgUDevice *imgu = data->imgu_;\n-\tunsigned int bufferCount;\n \tint ret;\n \n-\tbufferCount = std::max({\n-\t\tdata->outStream_.configuration().bufferCount,\n-\t\tdata->vfStream_.configuration().bufferCount,\n-\t\tdata->rawStream_.configuration().bufferCount,\n-\t});\n-\n-\tret = imgu->allocateBuffers(bufferCount);\n+\t/*\n+\t * This many internal buffers (or rather parameter and statistics buffer\n+\t * pairs) for the ImgU ensures that the pipeline runs smoothly, without\n+\t * frame drops. This number considers:\n+\t * - three buffers queued to the CIO2 (Since these buffers are bound to\n+\t * CIO2 buffers before queuing to the CIO2)\n+\t * - one buffer under processing in ImgU\n+\t *\n+\t * \\todo Update this number when we make these buffers only get added to\n+\t * the FrameInfo after the raw buffers are dequeued from CIO2.\n+\t */\n+\tret = imgu->allocateBuffers(kMinimumRequests + 1, bufferSlotCount);\n \tif (ret < 0)\n \t\treturn ret;\n \n@@ -731,7 +737,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n \t\treturn ret;\n \n \t/* Allocate buffers for internal pipeline usage. */\n-\tret = allocateBuffers(camera);\n+\tret = allocateBuffers(camera, kIPU3BufferSlotCount);\n \tif (ret)\n \t\treturn ret;\n \n@@ -744,8 +750,21 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\n \t/*\n \t * Start the ImgU video devices, buffers will be queued to the\n \t * ImgU output and viewfinder when requests will be queued.\n+\t *\n+\t * This many internal buffers for the CIO2 ensures that the pipeline\n+\t * runs smoothly, without frame drops. This number considers:\n+\t * - one buffer being DMA'ed to in CIO2\n+\t * - one buffer programmed by the CIO2 as the next buffer\n+\t * - one buffer under processing in ImgU\n+\t * - one extra idle buffer queued to CIO2, to account for possible\n+\t * delays in requeuing the buffer from ImgU back to CIO2\n+\t *\n+\t * Transient situations can arise when one of the parts, CIO2 or ImgU,\n+\t * finishes its processing first and experiences a lack of buffers, but\n+\t * they will shortly after return to the state described above as the\n+\t * other part catches up.\n \t */\n-\tret = cio2->start();\n+\tret = cio2->start(kMinimumRequests + 1, kIPU3BufferSlotCount);\n \tif (ret)\n \t\tgoto error;\n \n","prefixes":["v11","04/19"]}