[{"id":13747,"web_url":"https://patchwork.libcamera.org/comment/13747/","msgid":"<20201117122801.GE3940@pendragon.ideasonboard.com>","date":"2020-11-17T12:28:01","subject":"Re: [libcamera-devel] [PATCH v2] pipeline: raspberrypi: Rework\n\tbayer/embedded data buffer matching","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Nov 17, 2020 at 10:10:36AM +0000, Naushir Patuck wrote:\n> There is a condition that would cause the buffers to never be in sync\n> when we using only a single buffer in the bayer and embedded data streams.\n> This occurred because even though both streams would get flushed to resync,\n> one stream's only buffer was already queued in the device, and would end\n> up never matching.\n> \n> Rework the buffer matching logic by combining updateQueue() and\n> tryFlushQueue() into a single function findMatchingBuffers(). This would\n> allow us to flush the queues at the same time as we match buffers, avoiding\n> the the above condition.\n> \n> Signed-off-by: Naushir Patuck \n> Tested-by: David Plowman \n> Acked-by: Laurent Pinchart \n\nPushed to master.\n\n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 176 +++++++++---------\n> 1 file changed, 88 insertions(+), 88 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 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 &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 &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)","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 67D2BBE082\n\tfor ;\n\tTue, 17 Nov 2020 12:28:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DBB4363346;\n\tTue, 17 Nov 2020 13:28:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 62C4B6331E\n\tfor ;\n\tTue, 17 Nov 2020 13:28:06 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D71322A3;\n\tTue, 17 Nov 2020 13:28:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eVEiS7eM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1605616086;\n\tbh=Vrjg28DFHcCmqz6oNDb/OBVKXyEWAGPwIgi0RbyRq60=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eVEiS7eMfHjxsGBNugWkLCtDJnh4k8JdlBvpKzZ/vxxaV6qzkvUlUxPLmgRy4VdJW\n\tS7BQU/L6+K9LCGntoqfhR6rH6qvyMh5knRAaqZ+4RByd1ozEqZIuoUcwJE26VyonWA\n\tgZyeOcnbeAP5cPeHWjRwZ+TwRsOSJDCViTlm0U04=","Date":"Tue, 17 Nov 2020 14:28:01 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20201117122801.GE3940@pendragon.ideasonboard.com>","References":"<20201117101036.166323-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201117101036.166323-1-naush@raspberrypi.com>","Subject":"Re: [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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]