Patch Detail
Show a patch.
GET /api/patches/18018/?format=api
{ "id": 18018, "url": "https://patchwork.libcamera.org/api/patches/18018/?format=api", "web_url": "https://patchwork.libcamera.org/patch/18018/", "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": "<20221216122939.256534-5-paul.elder@ideasonboard.com>", "date": "2022-12-16T12:29:25", "name": "[libcamera-devel,v9,04/18] libcamera: pipeline: raspberrypi: Don't rely on bufferCount", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "f697e87b63b825fc6b2591153cad9aaf7d15e053", "submitter": { "id": 17, "url": "https://patchwork.libcamera.org/api/people/17/?format=api", "name": "Paul Elder", "email": "paul.elder@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/18018/mbox/", "series": [ { "id": 3675, "url": "https://patchwork.libcamera.org/api/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/18018/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/18018/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 D811EC31E9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 16 Dec 2022 12:30:11 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9CC82633A4;\n\tFri, 16 Dec 2022 13:30:11 +0100 (CET)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 065436339D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 16 Dec 2022 13:30:09 +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 93F80110E;\n\tFri, 16 Dec 2022 13:30:07 +0100 (CET)" ], "DKIM-Signature": [ "v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671193811;\n\tbh=+VGjpxv0w1TaA3+XrC+I8rEgzQ9lwcHkDQKe06UxPO8=;\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=q1XYAPa7ZhqJn5Uj4YaBUyn4TP6LAy28ksL0Rl3ekGwYJWv4JFdgPZlDUA3+k9/La\n\t1xQe4OIqX7t/vY59TY9lu/zYTypZzyvosg4gCWCgsBM7EeI8f4c4y1fSYb6q2N/opS\n\t3rGhD7oNi5BIhFbvTVi+BTYKyiureLMvedzFpWonxe2K0Wato6N4euV0Fn3D1F7WnQ\n\t4LGapyoI2OCrGJhmJiMXbO/6nMzkAEmffFMDj5azFkQzPtZW1pglhW0j/3h2riUto4\n\t7/POg9zNm01hPEOXgjSqZKi4oHFNh1n3UNRqJWQVxUFWihlXuteUL+jxwNbDwQBXvb\n\tfq6v1UXFDwwCw==", "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671193808;\n\tbh=+VGjpxv0w1TaA3+XrC+I8rEgzQ9lwcHkDQKe06UxPO8=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=nT/0Iop3tMgQDi7LT51f6+6+GFCkatz84fOgoWZkRLt2z2T/dn96EJaUOWY6bVN+Q\n\twS60brAuqj/CrIk3RqtsM7baO7C+qqbzp+OoBgOH1btqVyzYo+/PSLEzeg/rdWWtB8\n\t+5ChNbX1L53eoUhLyOS31KTeOxjecFQOfZmEVCrY=" ], "Authentication-Results": "lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nT/0Iop3\"; dkim-atps=neutral", "To": "libcamera-devel@lists.libcamera.org", "Date": "Fri, 16 Dec 2022 21:29:25 +0900", "Message-Id": "<20221216122939.256534-5-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 04/18] libcamera: pipeline:\n\traspberrypi: Don't rely 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 raspberrypi pipeline handler relies on bufferCount to\ndecide on the number of buffers to allocate internally and for the\nnumber of V4L2 buffer slots to reserve. Instead, the number of internal\nbuffers should be the minimum required by the pipeline to keep the\nrequests flowing, in order to avoid wasting memory, while the number of\nV4L2 buffer slots should be greater than the expected number of requests\nqueued, in order to avoid thrashing dmabuf mappings, which would degrade\nperformance.\n\nExtend the Stream class to receive the number of internal buffers that\nshould be allocated, and optionally the number of buffer slots to\nreserve. Use these new member variables to specify better suited numbers\nfor internal buffers and buffer slots for each stream individually,\nwhich also allows us to remove bufferCount.\n\nSigned-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>\nSigned-off-by: Paul Elder <paul.elder@ideasonboard.com>\n\n---\nChanges in v9:\n- rebased\n - I've decided that the buffer allocation decisions that Nícolas\n implemented covered the same cases that were added in\n PipelineHandlerRPi::prepareBuffers(), but in a slightly nicer way,\n especially considering that bufferCount is to be removed from\n StreamConfiguration in this series. Comments welcome, of course.\n\nChanges in v8:\n- Reworked buffer allocation handling in the raspberrypi pipeline handler\n- New\n---\n .../pipeline/raspberrypi/raspberrypi.cpp | 83 +++++++------------\n .../pipeline/raspberrypi/rpi_stream.cpp | 39 +++++----\n .../pipeline/raspberrypi/rpi_stream.h | 24 +++++-\n 3 files changed, 71 insertions(+), 75 deletions(-)", "diff": "diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\nindex 4641c76f..72502c36 100644\n--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n@@ -341,6 +341,25 @@ public:\n \tvoid releaseDevice(Camera *camera) override;\n \n private:\n+\t/*\n+\t * The number of buffers to allocate internally for transferring raw\n+\t * frames from the Unicam Image stream to the ISP Input stream. It is\n+\t * defined such that:\n+\t * - one buffer is being written to in Unicam Image\n+\t * - one buffer is being processed in ISP Input\n+\t * - one extra buffer is queued to Unicam Image\n+\t */\n+\tstatic constexpr unsigned int kInternalRawBufferCount = 3;\n+\n+\t/*\n+\t * The number of buffers to allocate internally for the external\n+\t * streams. They're used to drop the first few frames while the IPA\n+\t * algorithms converge. It is defined such that:\n+\t * - one buffer is being used on the stream\n+\t * - one extra buffer is queued\n+\t */\n+\tstatic constexpr unsigned int kInternalDropoutBufferCount = 2;\n+\n \tRPiCameraData *cameraData(Camera *camera)\n \t{\n \t\treturn static_cast<RPiCameraData *>(camera->_d());\n@@ -1221,21 +1240,23 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n \t\treturn -ENOENT;\n \n \t/* Locate and open the unicam video streams. */\n-\tdata->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicamImage);\n+\tdata->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\", unicamImage,\n+\t\t\t\t\t\t kInternalRawBufferCount);\n \n \t/* An embedded data node will not be present if the sensor does not support it. */\n \tMediaEntity *unicamEmbedded = unicam->getEntityByName(\"unicam-embedded\");\n \tif (unicamEmbedded) {\n-\t\tdata->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam Embedded\", unicamEmbedded);\n+\t\tdata->unicam_[Unicam::Embedded] =\n+\t\t\tRPi::Stream(\"Unicam Embedded\", unicamEmbedded, kInternalRawBufferCount);\n \t\tdata->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n \t\t\t\t\t\t\t\t\t &RPiCameraData::unicamBufferDequeue);\n \t}\n \n \t/* Tag the ISP input stream as an import stream. */\n-\tdata->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, true);\n-\tdata->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n-\tdata->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n-\tdata->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n+\tdata->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, 0, kInternalRawBufferCount, true);\n+\tdata->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1, kInternalDropoutBufferCount);\n+\tdata->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2, kInternalDropoutBufferCount);\n+\tdata->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3, kInternalDropoutBufferCount);\n \n \t/* Wire up all the buffer connections. */\n \tdata->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout);\n@@ -1428,57 +1449,11 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n {\n \tRPiCameraData *data = cameraData(camera);\n-\tunsigned int numRawBuffers = 0;\n \tint ret;\n \n-\tfor (Stream *s : camera->streams()) {\n-\t\tif (isRaw(s->configuration().pixelFormat)) {\n-\t\t\tnumRawBuffers = s->configuration().bufferCount;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\n-\t/* Decide how many internal buffers to allocate. */\n+\t/* Allocate internal buffers. */\n \tfor (auto const stream : data->streams_) {\n-\t\tunsigned int numBuffers;\n-\t\t/*\n-\t\t * For Unicam, allocate a minimum of 4 buffers as we want\n-\t\t * to avoid any frame drops.\n-\t\t */\n-\t\tconstexpr unsigned int minBuffers = 4;\n-\t\tif (stream == &data->unicam_[Unicam::Image]) {\n-\t\t\t/*\n-\t\t\t * If an application has configured a RAW stream, allocate\n-\t\t\t * additional buffers to make up the minimum, but ensure\n-\t\t\t * we have at least 2 sets of internal buffers to use to\n-\t\t\t * minimise frame drops.\n-\t\t\t */\n-\t\t\tnumBuffers = std::max<int>(2, minBuffers - numRawBuffers);\n-\t\t} else if (stream == &data->isp_[Isp::Input]) {\n-\t\t\t/*\n-\t\t\t * ISP input buffers are imported from Unicam, so follow\n-\t\t\t * similar logic as above to count all the RAW buffers\n-\t\t\t * available.\n-\t\t\t */\n-\t\t\tnumBuffers = numRawBuffers + std::max<int>(2, minBuffers - numRawBuffers);\n-\n-\t\t} else if (stream == &data->unicam_[Unicam::Embedded]) {\n-\t\t\t/*\n-\t\t\t * Embedded data buffers are (currently) for internal use,\n-\t\t\t * so allocate the minimum required to avoid frame drops.\n-\t\t\t */\n-\t\t\tnumBuffers = minBuffers;\n-\t\t} else {\n-\t\t\t/*\n-\t\t\t * Since the ISP runs synchronous with the IPA and requests,\n-\t\t\t * we only ever need one set of internal buffers. Any buffers\n-\t\t\t * the application wants to hold onto will already be exported\n-\t\t\t * through PipelineHandlerRPi::exportFrameBuffers().\n-\t\t\t */\n-\t\t\tnumBuffers = 1;\n-\t\t}\n-\n-\t\tret = stream->prepareBuffers(numBuffers);\n+\t\tret = stream->prepareBuffers();\n \t\tif (ret < 0)\n \t\t\treturn ret;\n \t}\ndiff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\nindex 2bb10f25..cde04efa 100644\n--- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n+++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n@@ -84,14 +84,24 @@ void Stream::removeExternalBuffer(FrameBuffer *buffer)\n \tbufferMap_.erase(id);\n }\n \n-int Stream::prepareBuffers(unsigned int count)\n+int Stream::prepareBuffers()\n {\n+\tunsigned int slotCount;\n \tint ret;\n \n \tif (!importOnly_) {\n-\t\tif (count) {\n+\t\t/*\n+\t\t * External streams overallocate buffer slots in order to handle\n+\t\t * the buffers queued from applications without degrading\n+\t\t * performance. Internal streams only need to have enough buffer\n+\t\t * slots to have the internal buffers queued.\n+\t\t */\n+\t\tslotCount = isExternal() ? kRPIExternalBufferSlotCount\n+\t\t\t\t\t : internalBufferCount_;\n+\n+\t\tif (internalBufferCount_) {\n \t\t\t/* Export some frame buffers for internal use. */\n-\t\t\tret = dev_->exportBuffers(count, &internalBuffers_);\n+\t\t\tret = dev_->exportBuffers(internalBufferCount_, &internalBuffers_);\n \t\t\tif (ret < 0)\n \t\t\t\treturn ret;\n \n@@ -100,23 +110,16 @@ int Stream::prepareBuffers(unsigned int count)\n \t\t\tresetBuffers();\n \t\t}\n \n-\t\t/* We must import all internal/external exported buffers. */\n-\t\tcount = bufferMap_.size();\n+\t} else {\n+\t\t/*\n+\t\t * An importOnly stream doesn't have its own internal buffers,\n+\t\t * so it needs to have the number of buffer slots to reserve\n+\t\t * explicitly declared.\n+\t\t */\n+\t\tslotCount = bufferSlotCount_;\n \t}\n \n-\t/*\n-\t * If this is an external stream, we must allocate slots for buffers that\n-\t * might be externally allocated. We have no indication of how many buffers\n-\t * may be used, so this might overallocate slots in the buffer cache.\n-\t * Similarly, if this stream is only importing buffers, we do the same.\n-\t *\n-\t * \\todo Find a better heuristic, or, even better, an exact solution to\n-\t * this issue.\n-\t */\n-\tif (isExternal() || importOnly_)\n-\t\tcount = count * 2;\n-\n-\treturn dev_->importBuffers(count);\n+\treturn dev_->importBuffers(slotCount);\n }\n \n int Stream::queueBuffer(FrameBuffer *buffer)\ndiff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\nindex b8bd79cf..e63bf02b 100644\n--- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n+++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n@@ -42,9 +42,13 @@ public:\n \t{\n \t}\n \n-\tStream(const char *name, MediaEntity *dev, bool importOnly = false)\n+\tStream(const char *name, MediaEntity *dev,\n+\t unsigned int internalBufferCount,\n+\t unsigned int bufferSlotCount = 0, bool importOnly = false)\n \t\t: external_(false), importOnly_(importOnly), name_(name),\n-\t\t dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(BufferMask::MaskID)\n+\t\t dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(BufferMask::MaskID),\n+\t\t internalBufferCount_(internalBufferCount),\n+\t\t bufferSlotCount_(bufferSlotCount)\n \t{\n \t}\n \n@@ -63,7 +67,7 @@ public:\n \tvoid setExternalBuffer(FrameBuffer *buffer);\n \tvoid removeExternalBuffer(FrameBuffer *buffer);\n \n-\tint prepareBuffers(unsigned int count);\n+\tint prepareBuffers();\n \tint queueBuffer(FrameBuffer *buffer);\n \tvoid returnBuffer(FrameBuffer *buffer);\n \n@@ -71,6 +75,8 @@ public:\n \tvoid releaseBuffers();\n \n private:\n+\tstatic constexpr unsigned int kRPIExternalBufferSlotCount = 16;\n+\n \tclass IdGenerator\n \t{\n \tpublic:\n@@ -133,6 +139,18 @@ private:\n \t/* All frame buffers associated with this device stream. */\n \tBufferMap bufferMap_;\n \n+\t/*\n+\t * The number of buffers that should be allocated internally for this\n+\t * stream.\n+\t */\n+\tunsigned int internalBufferCount_;\n+\n+\t/*\n+\t * The number of buffer slots that should be reserved for this stream.\n+\t * Only relevant for importOnly streams.\n+\t */\n+\tunsigned int bufferSlotCount_;\n+\n \t/*\n \t * List of frame buffers that we can use if none have been provided by\n \t * the application for external streams. This is populated by the\n", "prefixes": [ "libcamera-devel", "v9", "04/18" ] }