Patch Detail
Show a patch.
GET /api/1.1/patches/18019/?format=api
{ "id": 18019, "url": "https://patchwork.libcamera.org/api/1.1/patches/18019/?format=api", "web_url": "https://patchwork.libcamera.org/patch/18019/", "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": "<20221216122939.256534-6-paul.elder@ideasonboard.com>", "date": "2022-12-16T12:29:26", "name": "[libcamera-devel,v9,05/18] libcamera: pipeline: ipu3: Don't rely on bufferCount", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "022655e052e2aa8c7214d44177330c893bc2cd74", "submitter": { "id": 17, "url": "https://patchwork.libcamera.org/api/1.1/people/17/?format=api", "name": "Paul Elder", "email": "paul.elder@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/18019/mbox/", "series": [ { "id": 3675, "url": "https://patchwork.libcamera.org/api/1.1/series/3675/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=3675", "date": "2022-12-16T12:29:21", "name": "lc-compliance: Add test to queue more requests than hardware depth", "version": 9, "mbox": "https://patchwork.libcamera.org/series/3675/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/18019/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/18019/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 53E1EC328E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 16 Dec 2022 12:30:12 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 187216339E;\n\tFri, 16 Dec 2022 13:30:12 +0100 (CET)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B6464633A2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 16 Dec 2022 13:30:10 +0100 (CET)", "from pyrite.tail37cf.ts.net (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 526E6A31;\n\tFri, 16 Dec 2022 13:30:09 +0100 (CET)" ], "DKIM-Signature": [ "v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671193812;\n\tbh=CuCRJbuND3NwTG+nvCb+uGmvkczxn9s47Jjyw2RC8/Y=;\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=IRoC3HsITPVWFv/bn9BA1xKxbH+cagChn2ZechSfTEjdrhmA/a/u0iTmCVvG0WuUa\n\t4xN+9ez5n7uYCLVieL2dUebLptETqkZntUwvKTWT2qGX5vs1chUk0zznRZ9mtzlMlZ\n\t+zO9KFu+K6lfSEafI1cQfYmPnhgcg/1d5yd1zMWmyREKrIp10nr2gqWWOlhE5z0fuR\n\tZ329sDFfxVwE8/sgyEzrcE5rUmTMHjBvxNpxm/4r3x4TKV7Vl03BhR3uU8ZiUpnZWP\n\tSg+JWXBnZT4Fp3FgEhKetXZpW2ZOTcN9U+qPfblVTnYaafm8Uuv/aNvjePrNFzHNcc\n\tjWxh58whbta/g==", "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671193810;\n\tbh=CuCRJbuND3NwTG+nvCb+uGmvkczxn9s47Jjyw2RC8/Y=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=L7dH6d7CF76U4pPXowR+agKwQI2CK28ljgKFCxFR9Iif8JG7dms70pNsXTbUcgdQT\n\tKU80maH70dvMY+8Rn5IBfPwWRgg67AHB4l1ksP67fZVDufuOCj8tim/BRTouOV9lcp\n\tjcemO6wou7KQ459qqPJ2FrdUfDSAS2Exy/tXq5xc=" ], "Authentication-Results": "lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"L7dH6d7C\"; dkim-atps=neutral", "To": "libcamera-devel@lists.libcamera.org", "Date": "Fri, 16 Dec 2022 21:29:26 +0900", "Message-Id": "<20221216122939.256534-6-paul.elder@ideasonboard.com>", "X-Mailer": "git-send-email 2.35.1", "In-Reply-To": "<20221216122939.256534-1-paul.elder@ideasonboard.com>", "References": "<20221216122939.256534-1-paul.elder@ideasonboard.com>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=UTF-8", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3: Don't\n\trely on 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>", "From": "Paul Elder via libcamera-devel <libcamera-devel@lists.libcamera.org>", "Reply-To": "Paul Elder <paul.elder@ideasonboard.com>", "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 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 <nfraprado@collabora.com>\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\nSigned-off-by: Paul Elder <paul.elder@ideasonboard.com>\n\n---\nChanges in v9:\n- rebase\n\nChanges in v8:\n- New\n---\n src/libcamera/pipeline/ipu3/cio2.cpp | 4 ++--\n src/libcamera/pipeline/ipu3/cio2.h | 16 +++++++++++++++-\n src/libcamera/pipeline/ipu3/imgu.cpp | 12 ++++++------\n src/libcamera/pipeline/ipu3/imgu.h | 15 ++++++++++++++-\n src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++------------\n 5 files changed, 45 insertions(+), 22 deletions(-)", "diff": "diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\nindex d4e523af..feb69991 100644\n--- a/src/libcamera/pipeline/ipu3/cio2.cpp\n+++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n@@ -335,13 +335,13 @@ int CIO2Device::exportBuffers(unsigned int count,\n \treturn output_->exportBuffers(count, buffers);\n }\n \n-int CIO2Device::start()\n+int CIO2Device::start(unsigned int bufferSlotCount)\n {\n \tint ret = output_->exportBuffers(kBufferCount, &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 68504a2d..8ed208d3 100644\n--- a/src/libcamera/pipeline/ipu3/cio2.h\n+++ b/src/libcamera/pipeline/ipu3/cio2.h\n@@ -30,6 +30,20 @@ struct StreamConfiguration;\n class CIO2Device\n {\n public:\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 \tstatic constexpr unsigned int kBufferCount = 4;\n \n \tCIO2Device();\n@@ -48,7 +62,7 @@ public:\n \tV4L2SubdeviceFormat getSensorFormat(const std::vector<unsigned int> &mbusCodes,\n \t\t\t\t\t const Size &size) const;\n \n-\tint start();\n+\tint start(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 531879f1..fa920d87 100644\n--- a/src/libcamera/pipeline/ipu3/imgu.cpp\n+++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n@@ -576,22 +576,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(unsigned 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(kImgUInternalBufferCount, ¶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(kImgUInternalBufferCount, &statBuffers_);\n \tif (ret < 0) {\n \t\tLOG(IPU3, Error) << \"Failed to allocate ImgU stat buffers\";\n \t\tgoto error;\n@@ -602,13 +602,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 0af4dd8a..85873961 100644\n--- a/src/libcamera/pipeline/ipu3/imgu.h\n+++ b/src/libcamera/pipeline/ipu3/imgu.h\n@@ -84,7 +84,7 @@ public:\n \t\t\t\t\t outputFormat);\n \t}\n \n-\tint allocateBuffers(unsigned int bufferCount);\n+\tint allocateBuffers(unsigned int bufferSlotCount);\n \tvoid freeBuffers();\n \n \tint start();\n@@ -119,6 +119,19 @@ private:\n \n \tstd::string name_;\n \tMediaDevice *media_;\n+\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+\tstatic constexpr unsigned int kImgUInternalBufferCount = 4;\n };\n \n } /* namespace libcamera */\ndiff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\nindex bab2db65..4d8fcfeb 100644\n--- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n@@ -160,7 +160,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@@ -169,6 +169,8 @@ private:\n \tMediaDevice *imguMediaDev_;\n \n \tstd::vector<IPABuffer> ipaBuffers_;\n+\n+\tstatic constexpr unsigned int kIPU3BufferSlotCount = 16;\n };\n \n IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data)\n@@ -710,20 +712,14 @@ 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+\tret = imgu->allocateBuffers(bufferSlotCount);\n \tif (ret < 0)\n \t\treturn ret;\n \n@@ -781,7 +777,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@@ -795,7 +791,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis\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-\tret = cio2->start();\n+\tret = cio2->start(kIPU3BufferSlotCount);\n \tif (ret)\n \t\tgoto error;\n \n", "prefixes": [ "libcamera-devel", "v9", "05/18" ] }