Patch Detail
Show a patch.
GET /api/patches/12955/?format=api
{ "id": 12955, "url": "https://patchwork.libcamera.org/api/patches/12955/?format=api", "web_url": "https://patchwork.libcamera.org/patch/12955/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/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": "<20210714183857.2046425-10-nfraprado@collabora.com>", "date": "2021-07-14T18:38:56", "name": "[libcamera-devel,v6,09/10] libcamera: pipeline: Don't rely on bufferCount", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "b30d7b8a2c77c80b5721482425d01d9ade8a935d", "submitter": { "id": 84, "url": "https://patchwork.libcamera.org/api/people/84/?format=api", "name": "Nícolas F. R. A. Prado", "email": "nfraprado@collabora.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/12955/mbox/", "series": [ { "id": 2232, "url": "https://patchwork.libcamera.org/api/series/2232/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=2232", "date": "2021-07-14T18:38:47", "name": "lc-compliance: Add test to queue more requests than hardware depth", "version": 6, "mbox": "https://patchwork.libcamera.org/series/2232/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/12955/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/12955/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 B4698C3225\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 14 Jul 2021 18:39:27 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7CEC168530;\n\tWed, 14 Jul 2021 20:39:27 +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 31E2468523\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 14 Jul 2021 20:39:26 +0200 (CEST)", "from localhost.localdomain (unknown\n\t[IPv6:2804:14c:1a9:2434:6553:ad0c:9d2a:24db])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id A16AA1F4324D;\n\tWed, 14 Jul 2021 19:39:24 +0100 (BST)" ], "From": "=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= <nfraprado@collabora.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Wed, 14 Jul 2021 15:38:56 -0300", "Message-Id": "<20210714183857.2046425-10-nfraprado@collabora.com>", "X-Mailer": "git-send-email 2.32.0", "In-Reply-To": "<20210714183857.2046425-1-nfraprado@collabora.com>", "References": "<20210714183857.2046425-1-nfraprado@collabora.com>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=UTF-8", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v6 09/10] libcamera: pipeline: Don't rely\n\ton bufferCount", "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>", "Cc": "kernel@collabora.com, =?utf-8?q?Andr=C3=A9_Almeida?=\n\t<andrealmeid@collabora.com>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "Pipelines have relied on bufferCount to decide on the number of buffers\nto allocate internally through allocateBuffers() and on the number of\nV4L2 buffer slots to reserve through importBuffers(). Instead, the\nnumber of internal buffers should be the minimum required by the\nalgorithms to avoid wasting memory, and the number of V4L2 buffer slots\nshould overallocate to avoid thrashing dmabuf mappings.\n\nFor now, just set them to constants and stop relying on bufferCount, to\nallow for its removal.\n\nSigned-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>\n---\n\nChanges in v6:\n- Added pipeline name as prefix to each BUFFER_SLOT_COUNT and\n INTERNAL_BUFFER_COUNT constant\n\n src/libcamera/pipeline/ipu3/imgu.cpp | 12 ++++++------\n src/libcamera/pipeline/ipu3/imgu.h | 5 ++++-\n src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +--------\n .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++----------\n src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++-------\n src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 +-\n src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +++\n src/libcamera/pipeline/simple/converter.cpp | 4 ++--\n src/libcamera/pipeline/simple/converter.h | 3 +++\n src/libcamera/pipeline/simple/simple.cpp | 6 ++----\n src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +++--\n src/libcamera/pipeline/vimc/vimc.cpp | 5 +++--\n 12 files changed, 35 insertions(+), 43 deletions(-)", "diff": "diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\nindex e955bc3456ba..f36e99dacbe7 100644\n--- a/src/libcamera/pipeline/ipu3/imgu.cpp\n+++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n@@ -593,22 +593,22 @@ 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()\n {\n \t/* Share buffers between CIO2 output and ImgU input. */\n-\tint ret = input_->importBuffers(bufferCount);\n+\tint ret = input_->importBuffers(IPU3_BUFFER_SLOT_COUNT);\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(IPU3_INTERNAL_BUFFER_COUNT, ¶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(IPU3_INTERNAL_BUFFER_COUNT, &statBuffers_);\n \tif (ret < 0) {\n \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU stat buffers\";\n \t\tgoto error;\n@@ -619,13 +619,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(IPU3_BUFFER_SLOT_COUNT);\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(IPU3_BUFFER_SLOT_COUNT);\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 9d4915116087..f934a951fc75 100644\n--- a/src/libcamera/pipeline/ipu3/imgu.h\n+++ b/src/libcamera/pipeline/ipu3/imgu.h\n@@ -61,7 +61,7 @@ public:\n \t\t\t\t\t outputFormat);\n \t}\n \n-\tint allocateBuffers(unsigned int bufferCount);\n+\tint allocateBuffers();\n \tvoid freeBuffers();\n \n \tint start();\n@@ -86,6 +86,9 @@ private:\n \tstatic constexpr unsigned int PAD_VF = 3;\n \tstatic constexpr unsigned int PAD_STAT = 4;\n \n+\tstatic constexpr unsigned int IPU3_INTERNAL_BUFFER_COUNT = 4;\n+\tstatic constexpr unsigned int IPU3_BUFFER_SLOT_COUNT = 5;\n+\n \tint linkSetup(const std::string &source, unsigned int sourcePad,\n \t\t const std::string &sink, unsigned int sinkPad,\n \t\t bool enable);\ndiff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\nindex f3b456ba3afa..e80b0aaea8dc 100644\n--- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n@@ -681,16 +681,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\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+\tret = imgu->allocateBuffers();\n \tif (ret < 0)\n \t\treturn ret;\n \ndiff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\nindex f357e6c16ba8..8db7f651b6c4 100644\n--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n@@ -1151,20 +1151,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n {\n \tRPiCameraData *data = cameraData(camera);\n \tint ret;\n+\tconstexpr unsigned int bufferCount = 4;\n \n \t/*\n-\t * Decide how many internal buffers to allocate. For now, simply look\n-\t * at how many external buffers will be provided. We'll need to improve\n-\t * this logic. However, we really must have all streams allocate the same\n-\t * number of buffers to simplify error handling in queueRequestDevice().\n+\t * Allocate internal buffers. We really must have all streams allocate\n+\t * the same number of buffers to simplify error handling in\n+\t * queueRequestDevice().\n \t */\n-\tunsigned int maxBuffers = 0;\n-\tfor (const Stream *s : camera->streams())\n-\t\tif (static_cast<const RPi::Stream *>(s)->isExternal())\n-\t\t\tmaxBuffers = std::max(maxBuffers, s->configuration().bufferCount);\n-\n \tfor (auto const stream : data->streams_) {\n-\t\tret = stream->prepareBuffers(maxBuffers);\n+\t\tret = stream->prepareBuffers(bufferCount);\n \t\tif (ret < 0)\n \t\t\treturn ret;\n \t}\ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\nindex 4d52a0eb1413..840b853214c4 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n@@ -690,16 +690,11 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n \tunsigned int ipaBufferId = 1;\n \tint ret;\n \n-\tunsigned int maxCount = std::max({\n-\t\tdata->mainPathStream_.configuration().bufferCount,\n-\t\tdata->selfPathStream_.configuration().bufferCount,\n-\t});\n-\n-\tret = param_->allocateBuffers(maxCount, ¶mBuffers_);\n+\tret = param_->allocateBuffers(RKISP1_INTERNAL_BUFFER_COUNT, ¶mBuffers_);\n \tif (ret < 0)\n \t\tgoto error;\n \n-\tret = stat_->allocateBuffers(maxCount, &statBuffers_);\n+\tret = stat_->allocateBuffers(RKISP1_INTERNAL_BUFFER_COUNT, &statBuffers_);\n \tif (ret < 0)\n \t\tgoto error;\n \ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\nindex 25f482eb8d8e..fea330f72886 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n@@ -172,7 +172,7 @@ int RkISP1Path::start()\n \t\treturn -EBUSY;\n \n \t/* \\todo Make buffer count user configurable. */\n-\tret = video_->importBuffers(RKISP1_BUFFER_COUNT);\n+\tret = video_->importBuffers(RKISP1_BUFFER_SLOT_COUNT);\n \tif (ret)\n \t\treturn ret;\n \ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\nindex 91757600ccdc..3c5891009c58 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n@@ -27,6 +27,9 @@ class V4L2Subdevice;\n struct StreamConfiguration;\n struct V4L2SubdeviceFormat;\n \n+static constexpr unsigned int RKISP1_INTERNAL_BUFFER_COUNT = 4;\n+static constexpr unsigned int RKISP1_BUFFER_SLOT_COUNT = 5;\n+\n class RkISP1Path\n {\n public:\ndiff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp\nindex b5e34c4cd0c5..b3bcf01483f7 100644\n--- a/src/libcamera/pipeline/simple/converter.cpp\n+++ b/src/libcamera/pipeline/simple/converter.cpp\n@@ -103,11 +103,11 @@ int SimpleConverter::Stream::exportBuffers(unsigned int count,\n \n int SimpleConverter::Stream::start()\n {\n-\tint ret = m2m_->output()->importBuffers(inputBufferCount_);\n+\tint ret = m2m_->output()->importBuffers(SIMPLE_BUFFER_SLOT_COUNT);\n \tif (ret < 0)\n \t\treturn ret;\n \n-\tret = m2m_->capture()->importBuffers(outputBufferCount_);\n+\tret = m2m_->capture()->importBuffers(SIMPLE_BUFFER_SLOT_COUNT);\n \tif (ret < 0) {\n \t\tstop();\n \t\treturn ret;\ndiff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h\nindex 276a2a291c21..7e1d60674f62 100644\n--- a/src/libcamera/pipeline/simple/converter.h\n+++ b/src/libcamera/pipeline/simple/converter.h\n@@ -29,6 +29,9 @@ class SizeRange;\n struct StreamConfiguration;\n class V4L2M2MDevice;\n \n+constexpr unsigned int SIMPLE_INTERNAL_BUFFER_COUNT = 5;\n+constexpr unsigned int SIMPLE_BUFFER_SLOT_COUNT = 5;\n+\n class SimpleConverter\n {\n public:\ndiff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\nindex 37e880db0782..a14eb228b733 100644\n--- a/src/libcamera/pipeline/simple/simple.cpp\n+++ b/src/libcamera/pipeline/simple/simple.cpp\n@@ -803,12 +803,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\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(SIMPLE_INTERNAL_BUFFER_COUNT,\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\tret = video->importBuffers(SIMPLE_BUFFER_SLOT_COUNT);\n \t}\n \tif (ret < 0)\n \t\treturn ret;\ndiff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\nindex a1fa295a6456..4ae417914516 100644\n--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n@@ -91,6 +91,8 @@ private:\n \t\treturn static_cast<UVCCameraData *>(\n \t\t\tPipelineHandler::cameraData(camera));\n \t}\n+\n+\tstatic constexpr unsigned int UVC_BUFFER_SLOT_COUNT = 5;\n };\n \n UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data)\n@@ -236,9 +238,8 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera,\n int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n {\n \tUVCCameraData *data = cameraData(camera);\n-\tunsigned int count = data->stream_.configuration().bufferCount;\n \n-\tint ret = data->video_->importBuffers(count);\n+\tint ret = data->video_->importBuffers(UVC_BUFFER_SLOT_COUNT);\n \tif (ret < 0)\n \t\treturn ret;\n \ndiff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\nindex 082631b76702..6069f665c8f2 100644\n--- a/src/libcamera/pipeline/vimc/vimc.cpp\n+++ b/src/libcamera/pipeline/vimc/vimc.cpp\n@@ -102,6 +102,8 @@ private:\n \t\treturn static_cast<VimcCameraData *>(\n \t\t\tPipelineHandler::cameraData(camera));\n \t}\n+\n+\tstatic constexpr unsigned int VIMC_BUFFER_SLOT_COUNT = 5;\n };\n \n namespace {\n@@ -312,9 +314,8 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera,\n int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n {\n \tVimcCameraData *data = cameraData(camera);\n-\tunsigned int count = data->stream_.configuration().bufferCount;\n \n-\tint ret = data->video_->importBuffers(count);\n+\tint ret = data->video_->importBuffers(VIMC_BUFFER_SLOT_COUNT);\n \tif (ret < 0)\n \t\treturn ret;\n \n", "prefixes": [ "libcamera-devel", "v6", "09/10" ] }