[{"id":26092,"web_url":"https://patchwork.libcamera.org/comment/26092/","msgid":"","date":"2022-12-16T13:39:23","subject":"Re: [libcamera-devel] [PATCH v9 04/18] libcamera: pipeline:\n\traspberrypi: Don't rely on bufferCount","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Paul,\n\nOn Fri, 16 Dec 2022 at 12:30, Paul Elder via libcamera-devel <\nlibcamera-devel@lists.libcamera.org> wrote:\n\n> From: Nícolas F. R. A. Prado \n>\n> Currently the raspberrypi pipeline handler relies on bufferCount to\n> decide on the number of buffers to allocate internally and for the\n> number of V4L2 buffer slots to reserve. Instead, the number of internal\n> buffers should be the minimum required by the pipeline to keep the\n> requests flowing, in order to avoid wasting memory, while the number of\n> V4L2 buffer slots should be greater than the expected number of requests\n> queued, in order to avoid thrashing dmabuf mappings, which would degrade\n> performance.\n>\n> Extend the Stream class to receive the number of internal buffers that\n> should be allocated, and optionally the number of buffer slots to\n> reserve. Use these new member variables to specify better suited numbers\n> for internal buffers and buffer slots for each stream individually,\n> which also allows us to remove bufferCount.\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> Signed-off-by: Paul Elder \n>\n>\nI'm afraid this is going to conflict badly with my changes in [1] where I've\nmade substantial optimisations to the buffer allocation logic :-(\n\nHowever, if the aim of this commit is to remove the use of bufferCount from\nprepareBuffers(), then this ought to be a simple change on top of [1]:\n\ndiff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\nb/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\nindex ec416ab655c7..77f529ea0b46 100644\n--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n@@ -1511,7 +1511,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n\n for (Stream *s : camera->streams()) {\n if (s == &data->unicam_[Unicam::Image]) {\n- numRawBuffers = s->configuration().bufferCount;\n+ numRawBuffers =\ndata->unicam_[Unicam::Image].getBuffers().size();\n /*\n * If the application provides a guarantees that\nUnicam\n * image buffers will always be provided for the\nRAW stream\n\nThere is also the parallel discussion of using StreamConfig::bufferCout to\nprovide applications with minimum buffer/requests required for the pipeline\nto\nrun correctly. So we may not be able to remove this just yet...\n\nRegards,\nNaush\n\n[1] https://patchwork.libcamera.org/project/libcamera/list/?series=3663\n\n\n\n> ---\n> Changes 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>\n> Changes 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(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 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> void releaseDevice(Camera *camera) override;\n>\n> private:\n> + /*\n> + * The number of buffers to allocate internally for transferring\n> raw\n> + * frames from the Unicam Image stream to the ISP Input stream. It\n> is\n> + * defined such that:\n> + * - one buffer is being written to in Unicam Image\n> + * - one buffer is being processed in ISP Input\n> + * - one extra buffer is queued to Unicam Image\n> + */\n> + static constexpr unsigned int kInternalRawBufferCount = 3;\n> +\n> + /*\n> + * The number of buffers to allocate internally for the external\n> + * streams. They're used to drop the first few frames while the IPA\n> + * algorithms converge. It is defined such that:\n> + * - one buffer is being used on the stream\n> + * - one extra buffer is queued\n> + */\n> + static constexpr unsigned int kInternalDropoutBufferCount = 2;\n> +\n> RPiCameraData *cameraData(Camera *camera)\n> {\n> return static_cast(camera->_d());\n> @@ -1221,21 +1240,23 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *unicam, MediaDevice *isp, Me\n> return -ENOENT;\n>\n> /* Locate and open the unicam video streams. */\n> - data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> unicamImage);\n> + data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> unicamImage,\n> +\n> kInternalRawBufferCount);\n>\n> /* An embedded data node will not be present if the sensor does\n> not support it. */\n> MediaEntity *unicamEmbedded =\n> unicam->getEntityByName(\"unicam-embedded\");\n> if (unicamEmbedded) {\n> - data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> Embedded\", unicamEmbedded);\n> + data->unicam_[Unicam::Embedded] =\n> + RPi::Stream(\"Unicam Embedded\", unicamEmbedded,\n> kInternalRawBufferCount);\n>\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),\n>\n> &RPiCameraData::unicamBufferDequeue);\n> }\n>\n> /* Tag the ISP input stream as an import stream. */\n> - data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0,\n> true);\n> - data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n> - data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> - data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> + data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, 0,\n> kInternalRawBufferCount, true);\n> + data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1,\n> kInternalDropoutBufferCount);\n> + data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2,\n> kInternalDropoutBufferCount);\n> + data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3,\n> kInternalDropoutBufferCount);\n>\n> /* Wire up all the buffer connections. */\n>\n> data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(),\n> &RPiCameraData::unicamTimeout);\n> @@ -1428,57 +1449,11 @@ int PipelineHandlerRPi::queueAllBuffers(Camera\n> *camera)\n> int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> {\n> RPiCameraData *data = cameraData(camera);\n> - unsigned int numRawBuffers = 0;\n> int ret;\n>\n> - for (Stream *s : camera->streams()) {\n> - if (isRaw(s->configuration().pixelFormat)) {\n> - numRawBuffers = s->configuration().bufferCount;\n> - break;\n> - }\n> - }\n> -\n> - /* Decide how many internal buffers to allocate. */\n> + /* Allocate internal buffers. */\n> for (auto const stream : data->streams_) {\n> - unsigned int numBuffers;\n> - /*\n> - * For Unicam, allocate a minimum of 4 buffers as we want\n> - * to avoid any frame drops.\n> - */\n> - constexpr unsigned int minBuffers = 4;\n> - if (stream == &data->unicam_[Unicam::Image]) {\n> - /*\n> - * If an application has configured a RAW stream,\n> allocate\n> - * additional buffers to make up the minimum, but\n> ensure\n> - * we have at least 2 sets of internal buffers to\n> use to\n> - * minimise frame drops.\n> - */\n> - numBuffers = std::max(2, minBuffers -\n> numRawBuffers);\n> - } else if (stream == &data->isp_[Isp::Input]) {\n> - /*\n> - * ISP input buffers are imported from Unicam, so\n> follow\n> - * similar logic as above to count all the RAW\n> buffers\n> - * available.\n> - */\n> - numBuffers = numRawBuffers + std::max(2,\n> minBuffers - numRawBuffers);\n> -\n> - } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> - /*\n> - * Embedded data buffers are (currently) for\n> internal use,\n> - * so allocate the minimum required to avoid frame\n> drops.\n> - */\n> - numBuffers = minBuffers;\n> - } else {\n> - /*\n> - * Since the ISP runs synchronous with the IPA and\n> requests,\n> - * we only ever need one set of internal buffers.\n> Any buffers\n> - * the application wants to hold onto will already\n> be exported\n> - * through\n> PipelineHandlerRPi::exportFrameBuffers().\n> - */\n> - numBuffers = 1;\n> - }\n> -\n> - ret = stream->prepareBuffers(numBuffers);\n> + ret = stream->prepareBuffers();\n> if (ret < 0)\n> return ret;\n> }\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index 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> bufferMap_.erase(id);\n> }\n>\n> -int Stream::prepareBuffers(unsigned int count)\n> +int Stream::prepareBuffers()\n> {\n> + unsigned int slotCount;\n> int ret;\n>\n> if (!importOnly_) {\n> - if (count) {\n> + /*\n> + * External streams overallocate buffer slots in order to\n> handle\n> + * the buffers queued from applications without degrading\n> + * performance. Internal streams only need to have enough\n> buffer\n> + * slots to have the internal buffers queued.\n> + */\n> + slotCount = isExternal() ? kRPIExternalBufferSlotCount\n> + : internalBufferCount_;\n> +\n> + if (internalBufferCount_) {\n> /* Export some frame buffers for internal use. */\n> - ret = dev_->exportBuffers(count,\n> &internalBuffers_);\n> + ret = dev_->exportBuffers(internalBufferCount_,\n> &internalBuffers_);\n> if (ret < 0)\n> return ret;\n>\n> @@ -100,23 +110,16 @@ int Stream::prepareBuffers(unsigned int count)\n> resetBuffers();\n> }\n>\n> - /* We must import all internal/external exported buffers.\n> */\n> - count = bufferMap_.size();\n> + } else {\n> + /*\n> + * An importOnly stream doesn't have its own internal\n> buffers,\n> + * so it needs to have the number of buffer slots to\n> reserve\n> + * explicitly declared.\n> + */\n> + slotCount = bufferSlotCount_;\n> }\n>\n> - /*\n> - * If this is an external stream, we must allocate slots for\n> buffers that\n> - * might be externally allocated. We have no indication of how\n> many buffers\n> - * may be used, so this might overallocate slots in the buffer\n> cache.\n> - * Similarly, if this stream is only importing buffers, we do the\n> same.\n> - *\n> - * \\todo Find a better heuristic, or, even better, an exact\n> solution to\n> - * this issue.\n> - */\n> - if (isExternal() || importOnly_)\n> - count = count * 2;\n> -\n> - return dev_->importBuffers(count);\n> + return dev_->importBuffers(slotCount);\n> }\n>\n> int Stream::queueBuffer(FrameBuffer *buffer)\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index 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> {\n> }\n>\n> - Stream(const char *name, MediaEntity *dev, bool importOnly = false)\n> + Stream(const char *name, MediaEntity *dev,\n> + unsigned int internalBufferCount,\n> + unsigned int bufferSlotCount = 0, bool importOnly = false)\n> : external_(false), importOnly_(importOnly), name_(name),\n> - dev_(std::make_unique(dev)),\n> id_(BufferMask::MaskID)\n> + dev_(std::make_unique(dev)),\n> id_(BufferMask::MaskID),\n> + internalBufferCount_(internalBufferCount),\n> + bufferSlotCount_(bufferSlotCount)\n> {\n> }\n>\n> @@ -63,7 +67,7 @@ public:\n> void setExternalBuffer(FrameBuffer *buffer);\n> void removeExternalBuffer(FrameBuffer *buffer);\n>\n> - int prepareBuffers(unsigned int count);\n> + int prepareBuffers();\n> int queueBuffer(FrameBuffer *buffer);\n> void returnBuffer(FrameBuffer *buffer);\n>\n> @@ -71,6 +75,8 @@ public:\n> void releaseBuffers();\n>\n> private:\n> + static constexpr unsigned int kRPIExternalBufferSlotCount = 16;\n> +\n> class IdGenerator\n> {\n> public:\n> @@ -133,6 +139,18 @@ private:\n> /* All frame buffers associated with this device stream. */\n> BufferMap bufferMap_;\n>\n> + /*\n> + * The number of buffers that should be allocated internally for\n> this\n> + * stream.\n> + */\n> + unsigned int internalBufferCount_;\n> +\n> + /*\n> + * The number of buffer slots that should be reserved for this\n> stream.\n> + * Only relevant for importOnly streams.\n> + */\n> + unsigned int bufferSlotCount_;\n> +\n> /*\n> * List of frame buffers that we can use if none have been\n> provided by\n> * the application for external streams. This is populated by the\n> --\n> 2.35.1\n>\n>","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 8E1ABC3200\n\tfor ;\n\tFri, 16 Dec 2022 13:39:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B0ABA6339D;\n\tFri, 16 Dec 2022 14:39:42 +0100 (CET)","from mail-yb1-xb29.google.com (mail-yb1-xb29.google.com\n\t[IPv6:2607:f8b0:4864:20::b29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EDDCB603D0\n\tfor ;\n\tFri, 16 Dec 2022 14:39:40 +0100 (CET)","by mail-yb1-xb29.google.com with SMTP id i186so2348283ybc.9\n\tfor ;\n\tFri, 16 Dec 2022 05:39:40 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671197982;\n\tbh=Z7uIxO3198CtUcDLdHPSk+s+NGrOoIjbb1cWw07hctg=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zfsAgRHbtgYTD7UjKLEbxDE7uzcG0hiTelKoTcQiKJbofBPPXJDPjPFVORGOoajIt\n\t5mZjug6xW0NRHCaGYw1FIspgE8x4eXCACG2VpDBD+zmC+R4ham7Cltq/0bLpQOQ3Vw\n\tKielNk0LfOJHD1loZJuK74uoUhoWigi2vtobg/1cPLyP2iR0ZCLfi/UqkjYN/iiubj\n\t8dT/V3WfOd5nyCUbImIoHNNZKmL6PcEilK5VY8iDAmvPT6gK2uesFyrn9hEtZwmfeh\n\tuLL8RSeUbEqf+XbtUfsiE15d+BXZt03trwH3/x62AHi7huPZNsQdi7M8EKrR9yCBJ+\n\tUdfeOPj4gNdEw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Pgi61v0n8jYFbk7ERG9QLpDkmmlKce+gYyTU7ueLthY=;\n\tb=X9h0KH/mTHKT3l4G2LSEcaZGVzB6VzYlwEcnhkPu8A0M3bKkThuBEbwPgJ8I9y1xy6\n\tEsT2QT3FVZ6pvk4v17ETNEGtvXeUUp3MMNIbjFETIZ3VMuj9ZrCCn9HKbgSfc2U9uRav\n\tWqmx7yGXvgJN0vIgqeuaDP9UP1UENjaW551iVWa0NwoiMAYeT6Jey772EZGr7fIXRVYV\n\t1GoB0qRpN8usN06yRYn/14Gf1kbFhBjCvuZS6yP+1Sk/k7fN8eJsO3V8YOnNo5nossAf\n\tdRbMaLyJf8zONMuoX7sAp59aHfHnynp7AmfoWFn0c6a4lSRUKBVpUTiZKZ1vOiooWWv1\n\tBVZg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"X9h0KH/m\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=Pgi61v0n8jYFbk7ERG9QLpDkmmlKce+gYyTU7ueLthY=;\n\tb=jmXjIezCclbaJkdqZG5oPjjVAc9qhx2L0KcmBWIaQLFBT6ITTZYkcpLb5GnKflxBod\n\tJhqXVw6zKorO9HUt39j9jNW0w5bZm9I+2E90Tgt2oVT/4pcm7y7MWtEYQeYCfdrompli\n\t5OunEwcmusm7XzRU7FXylq4V+9biEoKVeEqbWdESage42nFPeGsc8UfkpXuNWDFyfhMp\n\t72Lkp+BDjvizwSTzqXCwGNacfi/KqNo3ANgQ9BdB88AvWFcLTdK+ybf92FdxdOWHMYcU\n\tmceS6lh3EnWBXUrXc6uJ7ukxOtWf5GDja6pBDk9jKo4ms4cKkRW7UdFGhTRMF36Jhl8G\n\tWKaA==","X-Gm-Message-State":"ANoB5pl3gnhG52pbKBC4ws7jCvzFgIToD58ik5+ABnCcPq6+5mM4il/W\n\tQS6PUVoURx2FUT9BlBisIxneqMS0wHT/Jy1BtDqK2JDDpbQ1FA8Lm9I=","X-Google-Smtp-Source":"AA0mqf7kbWjSNObu2djiB0YcgYzPrC8NcpB4H/1mu2F3tf1TGz1nmm54YHLwG9Yh+0vRlPbQT8XPtlsnR/PPJTBOq/A=","X-Received":"by 2002:a25:254:0:b0:71b:3d13:5c12 with SMTP id\n\t81-20020a250254000000b0071b3d135c12mr2522539ybc.606.1671197979592;\n\tFri, 16 Dec 2022 05:39:39 -0800 (PST)","MIME-Version":"1.0","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-5-paul.elder@ideasonboard.com>","In-Reply-To":"<20221216122939.256534-5-paul.elder@ideasonboard.com>","Date":"Fri, 16 Dec 2022 13:39:23 +0000","Message-ID":"","To":"Paul Elder ","Content-Type":"multipart/alternative; boundary=\"000000000000ec615705eff21942\"","Subject":"Re: [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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26098,"web_url":"https://patchwork.libcamera.org/comment/26098/","msgid":"<20221216144633.kgzslooyzkpaarrh@uno.localdomain>","date":"2022-12-16T14:46:33","subject":"Re: [libcamera-devel] [PATCH v9 04/18] libcamera: pipeline:\n\traspberrypi: Don't rely on bufferCount","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul\n\nOn Fri, Dec 16, 2022 at 09:29:25PM +0900, Paul Elder via libcamera-devel wrote:\n> From: Nícolas F. R. A. Prado \n>\n> Currently the raspberrypi pipeline handler relies on bufferCount to\n> decide on the number of buffers to allocate internally and for the\n> number of V4L2 buffer slots to reserve. Instead, the number of internal\n> buffers should be the minimum required by the pipeline to keep the\n> requests flowing, in order to avoid wasting memory, while the number of\n> V4L2 buffer slots should be greater than the expected number of requests\n> queued, in order to avoid thrashing dmabuf mappings, which would degrade\n> performance.\n>\n> Extend the Stream class to receive the number of internal buffers that\n> should be allocated, and optionally the number of buffer slots to\n> reserve. Use these new member variables to specify better suited numbers\n> for internal buffers and buffer slots for each stream individually,\n> which also allows us to remove bufferCount.\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes 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>\n> Changes 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(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 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(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\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(2, minBuffers - numRawBuffers);\n\nFor Unicam the old code took into account the external RAW buffers,\nand if none was requested 4 buffers where allocated. So it seems a\nnumber between 2 and 4 was possible. That's why you chose 3 in\n\n\tstatic constexpr unsigned int kInternalRawBufferCount = 3;\n\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(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\nThis was 4 and now it's 3\n\nThe rest of the code seems to work as it used to, but it would be\ngreat to have a confirmation from RPi and a test run on the platform\n\nThanks\n j\n\n\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}\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index 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)\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index 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(dev)), id_(BufferMask::MaskID)\n> +\t\t dev_(std::make_unique(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> --\n> 2.35.1\n>","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 0A249C31E9\n\tfor ;\n\tFri, 16 Dec 2022 14:46:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BE5D63360;\n\tFri, 16 Dec 2022 15:46:36 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::222])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B9EC4603D0\n\tfor ;\n\tFri, 16 Dec 2022 15:46:34 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 479FA40006;\n\tFri, 16 Dec 2022 14:46:34 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671201996;\n\tbh=W1NjGbka6Z4vDHIMSmtoOu4x121cIUfGThVuXEDpVTM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ZSGWP5pg5uy75fnn85TCnyGdzpkfbElrET9S9T+YIvK8To+oUzO5Y/CwLeyLHeRMI\n\tIG9gqEjgn5aeDla+sG5CUnQ1WBw5Wb8Rwms6kwG4nUAaphvs5g+v1Qoumbw5RNpj48\n\tayiysPgN9ycRCC3SqT5fL5XINGZiBjAxrGtw3pSGF24pcMKiJoDtn6VuphSZo5UhFT\n\tbK2udNjD5tRI5Ap7R/dwSthPrpTY4So59+hxhrfllsWvJUTVhOLhKY5+XI30LVgaLd\n\tp/44MQ+ZeONO8X7mfFTC2upZVoVsIAO8VwrOBjL6uogobNkorxeUhOLvhZSU/ub4Uf\n\tqMugv0CLbMQiw==","Date":"Fri, 16 Dec 2022 15:46:33 +0100","To":"Paul Elder ","Message-ID":"<20221216144633.kgzslooyzkpaarrh@uno.localdomain>","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20221216122939.256534-5-paul.elder@ideasonboard.com>","Subject":"Re: [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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26166,"web_url":"https://patchwork.libcamera.org/comment/26166/","msgid":"","date":"2022-12-28T02:21:49","subject":"Re: [libcamera-devel] [PATCH v9 04/18] libcamera: pipeline:\n\traspberrypi: Don't rely on bufferCount","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Naush,\n\nOn Fri, Dec 16, 2022 at 01:39:23PM +0000, Naushir Patuck wrote:\n> Hi Paul,\n> \n> On Fri, 16 Dec 2022 at 12:30, Paul Elder via libcamera-devel <\n> libcamera-devel@lists.libcamera.org> wrote:\n> \n> From: Nícolas F. R. A. Prado \n> \n> Currently the raspberrypi pipeline handler relies on bufferCount to\n> decide on the number of buffers to allocate internally and for the\n> number of V4L2 buffer slots to reserve. Instead, the number of internal\n> buffers should be the minimum required by the pipeline to keep the\n> requests flowing, in order to avoid wasting memory, while the number of\n> V4L2 buffer slots should be greater than the expected number of requests\n> queued, in order to avoid thrashing dmabuf mappings, which would degrade\n> performance.\n> \n> Extend the Stream class to receive the number of internal buffers that\n> should be allocated, and optionally the number of buffer slots to\n> reserve. Use these new member variables to specify better suited numbers\n> for internal buffers and buffer slots for each stream individually,\n> which also allows us to remove bufferCount.\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n> Signed-off-by: Paul Elder \n> \n> \n> \n> I'm afraid this is going to conflict badly with my changes in [1] where I've\n> made substantial optimisations to the buffer allocation logic :-(\n\nOh... yeah.\n\n> \n> However, if the aim of this commit is to remove the use of bufferCount from\n\nYeah that's the goal.\n\n> prepareBuffers(), then this ought to be a simple change on top of [1]:\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera\n> /pipeline/raspberrypi/raspberrypi.cpp\n> index ec416ab655c7..77f529ea0b46 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1511,7 +1511,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \n>         for (Stream *s : camera->streams()) {\n>                 if (s == &data->unicam_[Unicam::Image]) {\n> -                       numRawBuffers = s->configuration().bufferCount;\n> +                       numRawBuffers = data->unicam_[Unicam::Image].getBuffers\n> ().size();\n>                         /*\n>                          * If the application provides a guarantees that Unicam\n>                          * image buffers will always be provided for the RAW\n> stream\n\nAre you saying that this complex patch can just be reduced to that\nhunk...?\n\nI have a feeling that this series might make it in first.\n\n> There is also the parallel discussion of using StreamConfig::bufferCout to\n> provide applications with minimum buffer/requests required for the pipeline to\n> run correctly.  So we may not be able to remove this just yet...\n\nThis series introduces a MinimumRequests property which reports to\napplications the minimum number of requests required for the pipeline to\nrun without frame drops, so I think this covers that.\n\n\nThanks,\n\nPaul\n\n> \n> [1] https://patchwork.libcamera.org/project/libcamera/list/?series=3663\n> \n>  \n> \n> ---\n> Changes 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> \n> Changes 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(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/\n> libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 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>         void releaseDevice(Camera *camera) override;\n> \n>  private:\n> +       /*\n> +        * The number of buffers to allocate internally for transferring\n> raw\n> +        * frames from the Unicam Image stream to the ISP Input stream. It\n> is\n> +        * defined such that:\n> +        * - one buffer is being written to in Unicam Image\n> +        * - one buffer is being processed in ISP Input\n> +        * - one extra buffer is queued to Unicam Image\n> +        */\n> +       static constexpr unsigned int kInternalRawBufferCount = 3;\n> +\n> +       /*\n> +        * The number of buffers to allocate internally for the external\n> +        * streams. They're used to drop the first few frames while the IPA\n> +        * algorithms converge. It is defined such that:\n> +        * - one buffer is being used on the stream\n> +        * - one extra buffer is queued\n> +        */\n> +       static constexpr unsigned int kInternalDropoutBufferCount = 2;\n> +\n>         RPiCameraData *cameraData(Camera *camera)\n>         {\n>                 return static_cast(camera->_d());\n> @@ -1221,21 +1240,23 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *unicam, MediaDevice *isp, Me\n>                 return -ENOENT;\n> \n>         /* Locate and open the unicam video streams. */\n> -       data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> unicamImage);\n> +       data->unicam_[Unicam::Image] = RPi::Stream(\"Unicam Image\",\n> unicamImage,\n> +                                                 \n> kInternalRawBufferCount);\n> \n>         /* An embedded data node will not be present if the sensor does not\n> support it. */\n>         MediaEntity *unicamEmbedded = unicam->getEntityByName\n> (\"unicam-embedded\");\n>         if (unicamEmbedded) {\n> -               data->unicam_[Unicam::Embedded] = RPi::Stream(\"Unicam\n> Embedded\", unicamEmbedded);\n> +               data->unicam_[Unicam::Embedded] =\n> +                       RPi::Stream(\"Unicam Embedded\", unicamEmbedded,\n> kInternalRawBufferCount);\n>                 data->unicam_[Unicam::Embedded].dev()->bufferReady.connect\n> (data.get(),\n>                                                                            \n> &RPiCameraData::unicamBufferDequeue);\n>         }\n> \n>         /* Tag the ISP input stream as an import stream. */\n> -       data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0,\n> true);\n> -       data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1);\n> -       data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2);\n> -       data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3);\n> +       data->isp_[Isp::Input] = RPi::Stream(\"ISP Input\", ispOutput0, 0,\n> kInternalRawBufferCount, true);\n> +       data->isp_[Isp::Output0] = RPi::Stream(\"ISP Output0\", ispCapture1,\n> kInternalDropoutBufferCount);\n> +       data->isp_[Isp::Output1] = RPi::Stream(\"ISP Output1\", ispCapture2,\n> kInternalDropoutBufferCount);\n> +       data->isp_[Isp::Stats] = RPi::Stream(\"ISP Stats\", ispCapture3,\n> kInternalDropoutBufferCount);\n> \n>         /* Wire up all the buffer connections. */\n>         data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get\n> (), &RPiCameraData::unicamTimeout);\n> @@ -1428,57 +1449,11 @@ int PipelineHandlerRPi::queueAllBuffers(Camera\n> *camera)\n>  int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n>  {\n>         RPiCameraData *data = cameraData(camera);\n> -       unsigned int numRawBuffers = 0;\n>         int ret;\n> \n> -       for (Stream *s : camera->streams()) {\n> -               if (isRaw(s->configuration().pixelFormat)) {\n> -                       numRawBuffers = s->configuration().bufferCount;\n> -                       break;\n> -               }\n> -       }\n> -\n> -       /* Decide how many internal buffers to allocate. */\n> +       /* Allocate internal buffers. */\n>         for (auto const stream : data->streams_) {\n> -               unsigned int numBuffers;\n> -               /*\n> -                * For Unicam, allocate a minimum of 4 buffers as we want\n> -                * to avoid any frame drops.\n> -                */\n> -               constexpr unsigned int minBuffers = 4;\n> -               if (stream == &data->unicam_[Unicam::Image]) {\n> -                       /*\n> -                        * If an application has configured a RAW stream,\n> allocate\n> -                        * additional buffers to make up the minimum, but\n> ensure\n> -                        * we have at least 2 sets of internal buffers to\n> use to\n> -                        * minimise frame drops.\n> -                        */\n> -                       numBuffers = std::max(2, minBuffers -\n> numRawBuffers);\n> -               } else if (stream == &data->isp_[Isp::Input]) {\n> -                       /*\n> -                        * ISP input buffers are imported from Unicam, so\n> follow\n> -                        * similar logic as above to count all the RAW\n> buffers\n> -                        * available.\n> -                        */\n> -                       numBuffers = numRawBuffers + std::max(2,\n> minBuffers - numRawBuffers);\n> -\n> -               } else if (stream == &data->unicam_[Unicam::Embedded]) {\n> -                       /*\n> -                        * Embedded data buffers are (currently) for\n> internal use,\n> -                        * so allocate the minimum required to avoid frame\n> drops.\n> -                        */\n> -                       numBuffers = minBuffers;\n> -               } else {\n> -                       /*\n> -                        * Since the ISP runs synchronous with the IPA and\n> requests,\n> -                        * we only ever need one set of internal buffers.\n> Any buffers\n> -                        * the application wants to hold onto will already\n> be exported\n> -                        * through PipelineHandlerRPi::exportFrameBuffers\n> ().\n> -                        */\n> -                       numBuffers = 1;\n> -               }\n> -\n> -               ret = stream->prepareBuffers(numBuffers);\n> +               ret = stream->prepareBuffers();\n>                 if (ret < 0)\n>                         return ret;\n>         }\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/\n> libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index 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>         bufferMap_.erase(id);\n>  }\n> \n> -int Stream::prepareBuffers(unsigned int count)\n> +int Stream::prepareBuffers()\n>  {\n> +       unsigned int slotCount;\n>         int ret;\n> \n>         if (!importOnly_) {\n> -               if (count) {\n> +               /*\n> +                * External streams overallocate buffer slots in order to\n> handle\n> +                * the buffers queued from applications without degrading\n> +                * performance. Internal streams only need to have enough\n> buffer\n> +                * slots to have the internal buffers queued.\n> +                */\n> +               slotCount = isExternal() ? kRPIExternalBufferSlotCount\n> +                                        : internalBufferCount_;\n> +\n> +               if (internalBufferCount_) {\n>                         /* Export some frame buffers for internal use. */\n> -                       ret = dev_->exportBuffers(count, &\n> internalBuffers_);\n> +                       ret = dev_->exportBuffers(internalBufferCount_, &\n> internalBuffers_);\n>                         if (ret < 0)\n>                                 return ret;\n> \n> @@ -100,23 +110,16 @@ int Stream::prepareBuffers(unsigned int count)\n>                         resetBuffers();\n>                 }\n> \n> -               /* We must import all internal/external exported buffers. *\n> /\n> -               count = bufferMap_.size();\n> +       } else {\n> +               /*\n> +                * An importOnly stream doesn't have its own internal\n> buffers,\n> +                * so it needs to have the number of buffer slots to\n> reserve\n> +                * explicitly declared.\n> +                */\n> +               slotCount = bufferSlotCount_;\n>         }\n> \n> -       /*\n> -        * If this is an external stream, we must allocate slots for\n> buffers that\n> -        * might be externally allocated. We have no indication of how many\n> buffers\n> -        * may be used, so this might overallocate slots in the buffer\n> cache.\n> -        * Similarly, if this stream is only importing buffers, we do the\n> same.\n> -        *\n> -        * \\todo Find a better heuristic, or, even better, an exact\n> solution to\n> -        * this issue.\n> -        */\n> -       if (isExternal() || importOnly_)\n> -               count = count * 2;\n> -\n> -       return dev_->importBuffers(count);\n> +       return dev_->importBuffers(slotCount);\n>  }\n> \n>  int Stream::queueBuffer(FrameBuffer *buffer)\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/\n> libcamera/pipeline/raspberrypi/rpi_stream.h\n> index 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>         {\n>         }\n> \n> -       Stream(const char *name, MediaEntity *dev, bool importOnly = false)\n> +       Stream(const char *name, MediaEntity *dev,\n> +              unsigned int internalBufferCount,\n> +              unsigned int bufferSlotCount = 0, bool importOnly = false)\n>                 : external_(false), importOnly_(importOnly), name_(name),\n> -                 dev_(std::make_unique(dev)), id_\n> (BufferMask::MaskID)\n> +                 dev_(std::make_unique(dev)), id_\n> (BufferMask::MaskID),\n> +                 internalBufferCount_(internalBufferCount),\n> +                 bufferSlotCount_(bufferSlotCount)\n>         {\n>         }\n> \n> @@ -63,7 +67,7 @@ public:\n>         void setExternalBuffer(FrameBuffer *buffer);\n>         void removeExternalBuffer(FrameBuffer *buffer);\n> \n> -       int prepareBuffers(unsigned int count);\n> +       int prepareBuffers();\n>         int queueBuffer(FrameBuffer *buffer);\n>         void returnBuffer(FrameBuffer *buffer);\n> \n> @@ -71,6 +75,8 @@ public:\n>         void releaseBuffers();\n> \n>  private:\n> +       static constexpr unsigned int kRPIExternalBufferSlotCount = 16;\n> +\n>         class IdGenerator\n>         {\n>         public:\n> @@ -133,6 +139,18 @@ private:\n>         /* All frame buffers associated with this device stream. */\n>         BufferMap bufferMap_;\n> \n> +       /*\n> +        * The number of buffers that should be allocated internally for\n> this\n> +        * stream.\n> +        */\n> +       unsigned int internalBufferCount_;\n> +\n> +       /*\n> +        * The number of buffer slots that should be reserved for this\n> stream.\n> +        * Only relevant for importOnly streams.\n> +        */\n> +       unsigned int bufferSlotCount_;\n> +\n>         /*\n>          * List of frame buffers that we can use if none have been provided\n> by\n>          * the application for external streams. This is populated by the\n> --\n> 2.35.1\n> \n>","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 8F6AFC3200\n\tfor ;\n\tWed, 28 Dec 2022 02:21:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADDC6625CF;\n\tWed, 28 Dec 2022 03:21:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A3374625CB\n\tfor ;\n\tWed, 28 Dec 2022 03:21:56 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2604:2d80:ad8a:9000:1bf9:855b:22de:3645])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 77B0125B;\n\tWed, 28 Dec 2022 03:21:55 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1672194118;\n\tbh=fux/IKRNTwqBTnZRf1fp6QvY0CWTerpru2VOv9Dgs/U=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=w6fs4r6q+NA8rCUmJksEQPm8wia2kfruXNEHm+4yc1cBnr3822Fy2Th6YVkNmlbvh\n\tWkEmi+vuzXPGjblIezKeZlI8//rlIvE1U98A/obWi7Hggmrm5HjSvHpiOGr6X1RDcY\n\tC0XywP8bHCNOZp6yDN5ChPt2X3cnLNJC4RaSv+rzhY8rRDbPv1QP2eSd5NDnjhT4Fs\n\txX4E1vbru8wtbnVhEz7bEaEk9K3IUugo2Z32wQKr4hnxNmdEj8GXEwbqcGcIx1vjAr\n\tjmDaGuCuCskb5xU8xLG3XRoez4qPfzGTPoWxQc99Lxsx8wsfYbZsYOOjVW1GJLUB6N\n\tRl9I8Wl7NOUPw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1672194116;\n\tbh=fux/IKRNTwqBTnZRf1fp6QvY0CWTerpru2VOv9Dgs/U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EI4qbis0UxcXPpnBfRuuK2pl0JKhbk4ybGdMR+eoMxEAWomWmjTIcbz4VUMIfqyFI\n\tmSbZbCey7bw71BTRml81AmNhkcHwNcP8AcWJtkU+oeSK0yHZcluODur+lU/JlivKY5\n\tlXjvSTbzahnwXM1MQJH5c6S0/7Ku1dlhU0qMYFYw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"EI4qbis0\"; dkim-atps=neutral","Date":"Tue, 27 Dec 2022 20:21:49 -0600","To":"Naushir Patuck ","Message-ID":"","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-5-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Paul Elder via libcamera-devel ","Reply-To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]