Patch Detail
Show a patch.
GET /api/1.1/patches/10443/?format=api
{ "id": 10443, "url": "https://patchwork.libcamera.org/api/1.1/patches/10443/?format=api", "web_url": "https://patchwork.libcamera.org/patch/10443/", "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": "<20201117101036.166323-1-naush@raspberrypi.com>", "date": "2020-11-17T10:10:36", "name": "[libcamera-devel,v2] pipeline: raspberrypi: Rework bayer/embedded data buffer matching", "commit_ref": "de5d03673c41f5f117d03c23495de5620fd6f42e", "pull_url": null, "state": "accepted", "archived": false, "hash": "87a559c6fabe11f4cc4e117027e3faf582d47d11", "submitter": { "id": 34, "url": "https://patchwork.libcamera.org/api/1.1/people/34/?format=api", "name": "Naushir Patuck", "email": "naush@raspberrypi.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/10443/mbox/", "series": [ { "id": 1466, "url": "https://patchwork.libcamera.org/api/1.1/series/1466/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=1466", "date": "2020-11-17T10:10:36", "name": "[libcamera-devel,v2] pipeline: raspberrypi: Rework bayer/embedded data buffer matching", "version": 2, "mbox": "https://patchwork.libcamera.org/series/1466/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/10443/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/10443/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 E4C9CBE081\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 17 Nov 2020 10:10:43 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B095263321;\n\tTue, 17 Nov 2020 11:10:43 +0100 (CET)", "from mail-wm1-x334.google.com (mail-wm1-x334.google.com\n\t[IPv6:2a00:1450:4864:20::334])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB6CC63282\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 17 Nov 2020 11:10:41 +0100 (CET)", "by mail-wm1-x334.google.com with SMTP id 19so2671775wmf.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 17 Nov 2020 02:10:41 -0800 (PST)", "from naushir-VirtualBox.patuck.local ([88.97.76.4])\n\tby smtp.gmail.com with ESMTPSA id\n\tu8sm2874971wmg.6.2020.11.17.02.10.40\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 17 Nov 2020 02:10:40 -0800 (PST)" ], "Authentication-Results": "lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ax1N0y2t\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=from:to:cc:subject:date:message-id:mime-version\n\t:content-transfer-encoding;\n\tbh=jI7Up8GugWwQ85gC2Q/wVLikYmCjlJVPHbKnNY3L5mA=;\n\tb=ax1N0y2tWIyo38Jiz/sqyFbECJfsulWSEvKxZbs9FPk2yfAlz0YoiY3qnVseA38e/l\n\ttfFngT2RRPHi6brhwQBcYJGOTwnL4vXRq4krZi3Eb/PEWpdxgb2pHScomNRnrHfWo1oO\n\t2N09AQ0aTUFdVMScQ+bOKjRY7oerBuKGttTIfzrHpi2WU/Tki1ayQwwFKXZuc5MaTLyV\n\tYsgyrp9Z7tHVvNdLBrsW2r1Rd8lGWD1jTJfKxktRnyboNeK2ygCfSnHFtVGKJ+Me4Iay\n\todCdyxiklkG70viMCkycbY3eIK6NxLIyIlI67pQOs3ccJXuKn/Avli5NYqMzw2BdnPRp\n\t8/yQ==", "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version\n\t:content-transfer-encoding;\n\tbh=jI7Up8GugWwQ85gC2Q/wVLikYmCjlJVPHbKnNY3L5mA=;\n\tb=PFGPXLfVgWssUa+7otKeT2YPisl0CBMHwUD5FW9b21O7fW7AaGAXjRu2WzjhHDicK5\n\tZ7OynjdYrqzyWAFzl5BeQKBv77wbxYXR+RK9rbirhR0rlh18C9J3FMaAxvkd9rieNAdP\n\tqsClFGSLaBMP/zzid422w2gXAZqpoFIObU5OKfBEOU8d8/TYC6iWcBErzwUpfLsqu8CM\n\tAWq/1SsLFOeu+yJD565VXT801byFVpdzSvFx1Sc6ss1+k2jRTMxNUylRCDUQQICE/M1b\n\t7XRvHXo2k0V6i/DIYaw8G2e3Dl3Fzs9NP013pzFcr6N1GuppB+7zTOrxoKoEXdcRpbBu\n\tMzPQ==", "X-Gm-Message-State": "AOAM530mOqQ0RlW5RF6MuRjE1kGUsSmfT6dnvRzFto0PUynqYg24E9zI\n\t2g3xL59meW3OldO1VIOxHtq5BR/3Gbf37g==", "X-Google-Smtp-Source": "ABdhPJwJxVSNiTj2+jH8yL3p/Yb+GOepAf4y5NUZZHmUaRMYx+Hp+VkF2uDgoFbZTKPhH2N4Bf44ag==", "X-Received": "by 2002:a1c:bd08:: with SMTP id n8mr3473873wmf.136.1605607840950;\n\tTue, 17 Nov 2020 02:10:40 -0800 (PST)", "From": "Naushir Patuck <naush@raspberrypi.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Tue, 17 Nov 2020 10:10:36 +0000", "Message-Id": "<20201117101036.166323-1-naush@raspberrypi.com>", "X-Mailer": "git-send-email 2.25.1", "MIME-Version": "1.0", "Subject": "[libcamera-devel] [PATCH v2] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching", "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>", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "7bit", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "There is a condition that would cause the buffers to never be in sync\nwhen we using only a single buffer in the bayer and embedded data streams.\nThis occurred because even though both streams would get flushed to resync,\none stream's only buffer was already queued in the device, and would end\nup never matching.\n\nRework the buffer matching logic by combining updateQueue() and\ntryFlushQueue() into a single function findMatchingBuffers(). This would\nallow us to flush the queues at the same time as we match buffers, avoiding\nthe the above condition.\n\nSigned-off-by: Naushir Patuck <naush@raspberrypi.com>\nTested-by: David Plowman <david.plowman@raspberrypi.com>\nAcked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n .../pipeline/raspberrypi/raspberrypi.cpp | 176 +++++++++---------\n 1 file changed, 88 insertions(+), 88 deletions(-)", "diff": "diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\nindex 7ad66f21..7271573a 100644\n--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n@@ -205,9 +205,7 @@ public:\n private:\n \tvoid checkRequestCompleted();\n \tvoid tryRunPipeline();\n-\tvoid tryFlushQueues();\n-\tFrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,\n-\t\t\t\t RPi::Stream *stream);\n+\tbool findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer);\n \n \tunsigned int ispOutputCount_;\n };\n@@ -1537,7 +1535,6 @@ void RPiCameraData::handleState()\n \n \tcase State::Idle:\n \t\ttryRunPipeline();\n-\t\ttryFlushQueues();\n \t\tbreak;\n \t}\n }\n@@ -1588,41 +1585,8 @@ void RPiCameraData::tryRunPipeline()\n \t bayerQueue_.empty() || embeddedQueue_.empty())\n \t\treturn;\n \n-\t/* Start with the front of the bayer buffer queue. */\n-\tbayerBuffer = bayerQueue_.front();\n-\n-\t/*\n-\t * Find the embedded data buffer with a matching timestamp to pass to\n-\t * the IPA. Any embedded buffers with a timestamp lower than the\n-\t * current bayer buffer will be removed and re-queued to the driver.\n-\t */\n-\tembeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,\n-\t\t\t\t &unicam_[Unicam::Embedded]);\n-\n-\tif (!embeddedBuffer) {\n-\t\tLOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n-\n-\t\t/*\n-\t\t * Look the other way, try to match a bayer buffer with the\n-\t\t * first embedded buffer in the queue. This will also do some\n-\t\t * housekeeping on the bayer image queue - clear out any\n-\t\t * buffers that are older than the first buffer in the embedded\n-\t\t * queue.\n-\t\t *\n-\t\t * But first check if the embedded queue has emptied out.\n-\t\t */\n-\t\tif (embeddedQueue_.empty())\n-\t\t\treturn;\n-\n-\t\tembeddedBuffer = embeddedQueue_.front();\n-\t\tbayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,\n-\t\t\t\t\t &unicam_[Unicam::Image]);\n-\n-\t\tif (!bayerBuffer) {\n-\t\t\tLOG(RPI, Debug) << \"Could not find matching bayer buffer - ending.\";\n-\t\t\treturn;\n-\t\t}\n-\t}\n+\tif (!findMatchingBuffers(bayerBuffer, embeddedBuffer))\n+\t\treturn;\n \n \t/* Take the first request from the queue and action the IPA. */\n \tRequest *request = requestQueue_.front();\n@@ -1665,10 +1629,6 @@ void RPiCameraData::tryRunPipeline()\n \top.controls = { request->controls() };\n \tipa_->processEvent(op);\n \n-\t/* Ready to use the buffers, pop them off the queue. */\n-\tbayerQueue_.pop();\n-\tembeddedQueue_.pop();\n-\n \t/* Set our state to say the pipeline is active. */\n \tstate_ = State::Busy;\n \n@@ -1685,62 +1645,102 @@ void RPiCameraData::tryRunPipeline()\n \tipa_->processEvent(op);\n }\n \n-void RPiCameraData::tryFlushQueues()\n+bool RPiCameraData::findMatchingBuffers(FrameBuffer *&bayerBuffer, FrameBuffer *&embeddedBuffer)\n {\n-\t/*\n-\t * It is possible for us to end up in a situation where all available\n-\t * Unicam buffers have been dequeued but do not match. This can happen\n-\t * when the system is heavily loaded and we get out of lock-step with\n-\t * the two channels.\n-\t *\n-\t * In such cases, the best thing to do is the re-queue all the buffers\n-\t * and give a chance for the hardware to return to lock-step. We do have\n-\t * to drop all interim frames.\n-\t */\n-\tif (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&\n-\t unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {\n-\t\t/* This cannot happen when Unicam streams are external. */\n-\t\tassert(!unicam_[Unicam::Image].isExternal());\n+\tunsigned int embeddedRequeueCount = 0, bayerRequeueCount = 0;\n \n-\t\tLOG(RPI, Warning) << \"Flushing all buffer queues!\";\n-\n-\t\twhile (!bayerQueue_.empty()) {\n-\t\t\tunicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n-\t\t\tbayerQueue_.pop();\n-\t\t}\n+\t/* Loop until we find a matching bayer and embedded data buffer. */\n+\twhile (!bayerQueue_.empty()) {\n+\t\t/* Start with the front of the bayer queue. */\n+\t\tbayerBuffer = bayerQueue_.front();\n \n+\t\t/*\n+\t\t * Find the embedded data buffer with a matching timestamp to pass to\n+\t\t * the IPA. Any embedded buffers with a timestamp lower than the\n+\t\t * current bayer buffer will be removed and re-queued to the driver.\n+\t\t */\n+\t\tuint64_t ts = bayerBuffer->metadata().timestamp;\n+\t\tembeddedBuffer = nullptr;\n \t\twhile (!embeddedQueue_.empty()) {\n-\t\t\tunicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n-\t\t\tembeddedQueue_.pop();\n+\t\t\tFrameBuffer *b = embeddedQueue_.front();\n+\t\t\tif (!unicam_[Unicam::Embedded].isExternal() && b->metadata().timestamp < ts) {\n+\t\t\t\tembeddedQueue_.pop();\n+\t\t\t\tunicam_[Unicam::Embedded].queueBuffer(b);\n+\t\t\t\tembeddedRequeueCount++;\n+\t\t\t\tLOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n+\t\t\t\t\t\t << unicam_[Unicam::Embedded].name();\n+\t\t\t} else if (unicam_[Unicam::Embedded].isExternal() || b->metadata().timestamp == ts) {\n+\t\t\t\t/* We pop the item from the queue lower down. */\n+\t\t\t\tembeddedBuffer = b;\n+\t\t\t\tbreak;\n+\t\t\t} else {\n+\t\t\t\tbreak; /* Only higher timestamps from here. */\n+\t\t\t}\n \t\t}\n-\t}\n-}\n \n-FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,\n-\t\t\t\t\tRPi::Stream *stream)\n-{\n-\t/*\n-\t * If the unicam streams are external (both have be to the same), then we\n-\t * can only return out the top buffer in the queue, and assume they have\n-\t * been synced by queuing at the same time. We cannot drop these frames,\n-\t * as they may have been provided externally.\n-\t */\n-\twhile (!q.empty()) {\n-\t\tFrameBuffer *b = q.front();\n-\t\tif (!stream->isExternal() && b->metadata().timestamp < timestamp) {\n-\t\t\tq.pop();\n-\t\t\tstream->queueBuffer(b);\n+\t\tif (!embeddedBuffer) {\n+\t\t\tLOG(RPI, Debug) << \"Could not find matching embedded buffer\";\n+\n+\t\t\t/*\n+\t\t\t * If we have requeued all available embedded data buffers in this loop,\n+\t\t\t * then we are fully out of sync, so might as well requeue all the pending\n+\t\t\t * bayer buffers.\n+\t\t\t */\n+\t\t\tif (embeddedRequeueCount == unicam_[Unicam::Embedded].getBuffers().size()) {\n+\t\t\t\t/* The embedded queue must be empty at this point! */\n+\t\t\t\tASSERT(embeddedQueue_.empty());\n+\n+\t\t\t\tLOG(RPI, Warning) << \"Flushing bayer stream!\";\n+\t\t\t\twhile (!bayerQueue_.empty()) {\n+\t\t\t\t\tunicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n+\t\t\t\t\tbayerQueue_.pop();\n+\t\t\t\t}\n+\t\t\t\treturn false;\n+\t\t\t}\n+\n+\t\t\t/*\n+\t\t\t * Not found a matching embedded buffer for the bayer buffer in\n+\t\t\t * the front of the queue. This buffer is now orphaned, so requeue\n+\t\t\t * it back to the device.\n+\t\t\t */\n+\t\t\tunicam_[Unicam::Image].queueBuffer(bayerQueue_.front());\n+\t\t\tbayerQueue_.pop();\n+\t\t\tbayerRequeueCount++;\n \t\t\tLOG(RPI, Warning) << \"Dropping unmatched input frame in stream \"\n-\t\t\t\t\t << stream->name();\n-\t\t} else if (stream->isExternal() || b->metadata().timestamp == timestamp) {\n-\t\t\t/* The calling function will pop the item from the queue. */\n-\t\t\treturn b;\n+\t\t\t\t\t << unicam_[Unicam::Image].name();\n+\n+\t\t\t/*\n+\t\t\t * Similar to the above, if we have requeued all available bayer buffers in\n+\t\t\t * the loop, then we are fully out of sync, so might as well requeue all the\n+\t\t\t * pending embedded data buffers.\n+\t\t\t */\n+\t\t\tif (bayerRequeueCount == unicam_[Unicam::Image].getBuffers().size()) {\n+\t\t\t\t/* The embedded queue must be empty at this point! */\n+\t\t\t\tASSERT(bayerQueue_.empty());\n+\n+\t\t\t\tLOG(RPI, Warning) << \"Flushing embedded data stream!\";\n+\t\t\t\twhile (!embeddedQueue_.empty()) {\n+\t\t\t\t\tunicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());\n+\t\t\t\t\tembeddedQueue_.pop();\n+\t\t\t\t}\n+\t\t\t\treturn false;\n+\t\t\t}\n+\n+\t\t\t/* If the embedded queue has become empty, we cannot do any more. */\n+\t\t\tif (embeddedQueue_.empty())\n+\t\t\t\treturn false;\n \t\t} else {\n-\t\t\tbreak; /* Only higher timestamps from here. */\n+\t\t\t/*\n+\t\t\t * We have found a matching bayer and embedded data buffer, so\n+\t\t\t * nothing more to do apart from popping the buffers from the queue.\n+\t\t\t */\n+\t\t\tbayerQueue_.pop();\n+\t\t\tembeddedQueue_.pop();\n+\t\t\treturn true;\n \t\t}\n \t}\n \n-\treturn nullptr;\n+\treturn false;\n }\n \n REGISTER_PIPELINE_HANDLER(PipelineHandlerRPi)\n", "prefixes": [ "libcamera-devel", "v2" ] }