[{"id":11368,"web_url":"https://patchwork.libcamera.org/comment/11368/","msgid":"<20200713120348.GJ2866302@oden.dyn.berto.se>","date":"2020-07-13T12:03:48","subject":"Re: [libcamera-devel] [PATCH 8/9] libcamera: pipeline: raspberrypi:\n\tAdd more robust stream buffer logic","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Naushir,\n\nThanks for your work.\n\nOn 2020-07-13 09:47:27 +0100, Naushir Patuck wrote:\n> Add further queueing into the RPiStream object to ensure that we always\n> follow the buffer ordering (be it internal or external) given by\n> incoming Requests.\n> \n> This is essential, otherwise we risk dropping frames that are meant to\n> be part of a Request, and can cause the pipeline to stall indefinitely.\n> This also prevents any possibility of mismatched frame buffers going\n> through the pipeline and out to the application.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 4 +-\n> .../pipeline/raspberrypi/rpi_stream.h | 81 ++++++++++++++++---\n> 2 files changed, 73 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index e1a74d89..2c5af05b 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -732,7 +732,9 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)\n> \tfor (auto stream : data->streams_) {\n> \t\tif (stream->isExternal()) {\n> \t\t\tFrameBuffer *buffer = request->findBuffer(stream);\n> -\t\t\tstream->queueBuffer(data->dropFrameCount_ ? nullptr : buffer);\n> +\t\t\tint ret = stream->queueBuffer(data->dropFrameCount_ ? nullptr : buffer);\n> +\t\t\tif (ret)\n> +\t\t\t\treturn ret;\n\nI think this can be squashed in an earlier patch in this series which \nadds this code right?\n\n> \t\t}\n> \t}\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index a6f573fa..3309edfb 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> @@ -139,28 +139,77 @@ public:\n> \t\t\tif (availableBuffers_.empty()) {\n> \t\t\t\tLOG(RPISTREAM, Warning) << \"No buffers available for \"\n> \t\t\t\t\t\t\t<< name_;\n> -\t\t\t\treturn -EINVAL;\n> +\t\t\t\t/*\n> +\t\t\t\t * Note that we need to requeue an internal buffer as soon\n> +\t\t\t\t * as one becomes available.\n> +\t\t\t\t */\n> +\t\t\t\trequeueBuffers_.push(nullptr);\n> +\t\t\t\treturn -ENOMEM;\n> \t\t\t}\n> \n> \t\t\tbuffer = availableBuffers_.front();\n> \t\t\tavailableBuffers_.pop();\n> \t\t}\n> \n> -\t\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> -\t\t\t\t << \" for \" << name_;\n> -\n> -\t\tint ret = dev_->queueBuffer(buffer);\n> -\t\tif (ret) {\n> -\t\t\tLOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> -\t\t\t\t\t << name_;\n> +\t\tif (requeueBuffers_.empty()) {\n> +\t\t\t/*\n> +\t\t\t* No earlier requests are pending to be queued, so we can\n> +\t\t\t* go ahead and queue the buffer into the device.\n> +\t\t\t*/\n> +\t\t\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> +\t\t\t\t\t << \" for \" << name_;\n> +\n> +\t\t\tint ret = dev_->queueBuffer(buffer);\n> +\t\t\tif (ret)\n> +\t\t\t\tLOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> +\t\t\t\t\t\t << name_;\n> +\t\t\treturn ret;\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * There are earlier buffers to be queued, so this buffer\n> +\t\t\t * must go on the waiting list.\n> +\t\t\t */\n> +\t\t\trequeueBuffers_.push(buffer);\n> +\t\t\treturn 0;\n> \t\t}\n> -\n> -\t\treturn ret;\n> \t}\n> \n> \tvoid returnBuffer(FrameBuffer *buffer)\n> \t{\n> +\t\t/* Push this buffer back into the queue to be used again. */\n> \t\tavailableBuffers_.push(buffer);\n> +\n> +\t\t/*\n> +\t\t * Do we have any buffers that are waiting to be queued?\n> +\t\t * If so, do it now as availableBuffers_ will not be empty.\n> +\t\t */\n> +\t\twhile (!requeueBuffers_.empty()) {\n> +\t\t\tFrameBuffer *buffer = requeueBuffers_.front();\n> +\t\t\trequeueBuffers_.pop();\n> +\n> +\t\t\tif (!buffer && !availableBuffers_.empty()) {\n> +\t\t\t\t/*\n> +\t\t\t\t * We want to queue an internal buffer, and at\n> +\t\t\t\t * least one is available.\n> +\t\t\t\t */\n> +\t\t\t\tbuffer = availableBuffers_.front();\n> +\t\t\t\tavailableBuffers_.pop();\n> +\t\t\t} else if (!buffer && !availableBuffers_.empty()) {\n> +\t\t\t\t/*\n> +\t\t\t\t * We want to queue an internal buffer, but none\n> +\t\t\t\t * are available.\n> +\t\t\t\t */\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> +\t\t\t\t\t << \" for \" << name_ << \" from returnBuffer\";\n> +\n> +\t\t\tint ret = dev_->queueBuffer(buffer);\n> +\t\t\tif (ret)\n> +\t\t\t\tLOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> +\t\t\t\t\t\t << name_ << \" from returnBuffer\";\n> +\t\t}\n\nThe code is fine but is feels like it should be broken out to it's own \nfunction and maybe share some of it with the code above which puts \nbuffers into requeueBuffers_ ?\n\n> \t}\n> \n> \tbool findFrameBuffer(FrameBuffer *buffer) const\n> @@ -178,6 +227,7 @@ private:\n> \tvoid clearBuffers()\n> \t{\n> \t\tavailableBuffers_ = std::queue{};\n> +\t\trequeueBuffers_ = std::queue{};\n> \t\tinternalBuffers_.clear();\n> \t\tbufferList_.clear();\n> \t}\n> @@ -193,7 +243,7 @@ private:\n> \tstd::string name_;\n> \t/* The actual device stream. */\n> \tstd::unique_ptr dev_;\n> -\t/* All framebuffers associated with this device stream. */\n> +\t/* All frame buffers associated with this device stream. */\n> \tstd::vector bufferList_;\n> \t/*\n> \t * List of frame buffer that we can use if none have been provided by\n> @@ -201,6 +251,15 @@ private:\n> \t * buffers exported internally.\n> \t */\n> \tstd::queue availableBuffers_;\n> +\t/*\n> +\t * List of frame buffer that are to be re-queued into the device.\n> +\t * A nullptr indicates any internal buffer can be used (from availableBuffers_),\n> +\t * whereas a valid pointer indicates an external buffer to be queued.\n> +\t *\n> +\t * Ordering buffers to be queued is important here as it must match the\n> +\t * requests coming from the application.\n> +\t */\n> +\tstd::queue requeueBuffers_;\n> \t/*\n> \t * This is a list of buffers exported internally. Need to keep this around\n> \t * as the stream needs to maintain ownership of these buffers.\n> -- \n> 2.25.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 01C72BD792\n\tfor ;\n\tMon, 13 Jul 2020 12:03:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7AC3F6055F;\n\tMon, 13 Jul 2020 14:03:51 +0200 (CEST)","from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EEE326048F\n\tfor ;\n\tMon, 13 Jul 2020 14:03:49 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id r19so17369363ljn.12\n\tfor ;\n\tMon, 13 Jul 2020 05:03:49 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ts1sm4036114ljj.96.2020.07.13.05.03.48\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 13 Jul 2020 05:03:48 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"OcNtyYee\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=N4mPVqwdkUgyZUqJDqrmxaTKcXAUI7EDo3Q97qb+OYw=;\n\tb=OcNtyYeeB/UBL69eNP75K6RWEuoIIhfUEgu45Q7rNZNVILx0pNR9DJdDYTtzs5J2D4\n\tVhZUxTaX6yuLjxeXdfGvrLkn1eJy8fj5+2/xQTUsoQ/OeY/8VbwK83Hd5tBZYDFNNH2U\n\t0aI67LCjEEpc07UcjukaUZGBCuwbxm4pX0mMbG28FdWcgXaiVlAoCuce/4JexiNL1lN8\n\tBEVhNw+ofDrdl5EoUq2PxeD16InevCDNXSAmFt+iJZcynurati2MjJiz45LzbaxOOP68\n\tlDkZd/UOSvQkIJ5ET426oZEsfOZfzF6FbtK/fUVq51P3tA/aE5v2xl3IeOvFUIikrN0b\n\t6tXg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=N4mPVqwdkUgyZUqJDqrmxaTKcXAUI7EDo3Q97qb+OYw=;\n\tb=OC0u9CpukgnmIKML7jrwlxUdyqp27kRTg2uaUscVZetBa3izzk4yYjgKo3a8mlyB5d\n\tn5nNFszLEBOIk56ndrsp7T7AFPUnmP2m2ubsevEYOo0xaGcxhWYzO6UTIf8pojFldIe/\n\taSXL002WiLLSbbKBn3no83VG2xXETI4bhlqHzkMfoVp1guecwX/5bcYsaIvQ2rtr023i\n\tXZpsNcXl+Esx59gjrc9qcLYhdspNDlZeXKjrpr+aA+CMYWWumH04D8NjMRHMkQNLSSvC\n\ticWdLsxgR+AZC47uhPTrypaToHcwEhNg+pUgudDKum2unPTUJhz3PLHk5cbDVAu/nOiU\n\tWVfw==","X-Gm-Message-State":"AOAM5305WEJH5a4uTq2oH7VDLtvyhgjTfaDvlTVF5ura+xvEyVd1OuMB\n\tHRrgu+ya19chc/L3WviOa2lSpg==","X-Google-Smtp-Source":"ABdhPJwUVJ9bUp0IABDT8DLazYa0VyeZ7dgPP6PpYU8LetR2In3wvvr+d9ct4FjjytvLKtsZh5e+dg==","X-Received":"by 2002:a2e:9f4d:: with SMTP id\n\tv13mr17578798ljk.122.1594641829146; \n\tMon, 13 Jul 2020 05:03:49 -0700 (PDT)","Date":"Mon, 13 Jul 2020 14:03:48 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Naushir Patuck ","Message-ID":"<20200713120348.GJ2866302@oden.dyn.berto.se>","References":"<20200713084727.232422-1-naush@raspberrypi.com>\n\t<20200713084727.232422-9-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200713084727.232422-9-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 8/9] libcamera: pipeline: raspberrypi:\n\tAdd more robust stream buffer logic","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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]