[{"id":25962,"web_url":"https://patchwork.libcamera.org/comment/25962/","msgid":"","date":"2022-12-01T12:29:39","subject":"Re: [libcamera-devel] [PATCH v8 05/17] libcamera: pipeline: simple:\n\tDon't rely on bufferCount","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Tue, Aug 24, 2021 at 04:56:24PM -0300, Nícolas F. R. A. Prado wrote:\n> Currently the simple pipeline handler relies on bufferCount to decide on\n> the number of buffers to allocate internally when a converter is in use\n> and for the number of V4L2 buffer slots to reserve. Instead, the number\n> of internal buffers should be the minimum required by the pipeline to\n> keep the requests flowing, in order to avoid wasting memory, while the\n> number of V4L2 buffer slots should be greater than the expected number\n> of requests queued by the application, in order to avoid thrashing\n> dmabuf mappings, which would degrade performance.\n> \n> Stop relying on bufferCount for these numbers and instead set them to\n> appropriate, and independent, constants.\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n\nReviewed-by: Paul Elder \n\n> \n> ---\n> \n> Changes in v8:\n> - New\n> \n> src/libcamera/pipeline/simple/converter.cpp | 12 ++++++-----\n> src/libcamera/pipeline/simple/converter.h | 6 ++++--\n> src/libcamera/pipeline/simple/simple.cpp | 22 ++++++++++++++++-----\n> 3 files changed, 28 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp\n> index b5e34c4cd0c5..3133b3dbda07 100644\n> --- a/src/libcamera/pipeline/simple/converter.cpp\n> +++ b/src/libcamera/pipeline/simple/converter.cpp\n> @@ -101,13 +101,14 @@ int SimpleConverter::Stream::exportBuffers(unsigned int count,\n> \treturn m2m_->capture()->exportBuffers(count, buffers);\n> }\n> \n> -int SimpleConverter::Stream::start()\n> +int SimpleConverter::Stream::start(unsigned int internalBufferCount,\n> +\t\t\t\t unsigned int bufferSlotCount)\n> {\n> -\tint ret = m2m_->output()->importBuffers(inputBufferCount_);\n> +\tint ret = m2m_->output()->importBuffers(internalBufferCount);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tret = m2m_->capture()->importBuffers(outputBufferCount_);\n> +\tret = m2m_->capture()->importBuffers(bufferSlotCount);\n> \tif (ret < 0) {\n> \t\tstop();\n> \t\treturn ret;\n> @@ -331,12 +332,13 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,\n> \treturn streams_[output].exportBuffers(count, buffers);\n> }\n> \n> -int SimpleConverter::start()\n> +int SimpleConverter::start(unsigned int internalBufferCount,\n> +\t\t\t unsigned int bufferSlotCount)\n> {\n> \tint ret;\n> \n> \tfor (Stream &stream : streams_) {\n> -\t\tret = stream.start();\n> +\t\tret = stream.start(internalBufferCount, bufferSlotCount);\n> \t\tif (ret < 0) {\n> \t\t\tstop();\n> \t\t\treturn ret;\n> diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h\n> index 276a2a291c21..deb3df0d08df 100644\n> --- a/src/libcamera/pipeline/simple/converter.h\n> +++ b/src/libcamera/pipeline/simple/converter.h\n> @@ -47,7 +47,8 @@ public:\n> \tint exportBuffers(unsigned int ouput, unsigned int count,\n> \t\t\t std::vector> *buffers);\n> \n> -\tint start();\n> +\tint start(unsigned int internalBufferCount,\n> +\t\t unsigned int bufferSlotCount);\n> \tvoid stop();\n> \n> \tint queueBuffers(FrameBuffer *input,\n> @@ -69,7 +70,8 @@ private:\n> \t\tint exportBuffers(unsigned int count,\n> \t\t\t\t std::vector> *buffers);\n> \n> -\t\tint start();\n> +\t\tint start(unsigned int internalBufferCount,\n> +\t\t\t unsigned int bufferSlotCount);\n> \t\tvoid stop();\n> \n> \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 737815452bae..d0a658a23be8 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -253,6 +253,7 @@ protected:\n> \n> private:\n> \tstatic constexpr unsigned int kNumInternalBuffers = 3;\n> +\tstatic constexpr unsigned int kSimpleBufferSlotCount = 16;\n> \n> \tSimpleCameraData *cameraData(Camera *camera)\n> \t{\n> @@ -830,17 +831,27 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> \tV4L2VideoDevice *video = data->video_;\n> \tint ret;\n> \n> +\t/*\n> +\t * Number of internal buffers that will be used to move video capture\n> +\t * device frames to the converter.\n> +\t *\n> +\t * \\todo Make this depend on the driver in use instead of being\n> +\t * hardcoded. In order to not drop frames, the realtime requirements for\n> +\t * each device should be checked, so it may not be as simple as just\n> +\t * using the minimum number of buffers required by the driver.\n> +\t */\n> +\tstatic constexpr unsigned int internalBufferCount = 3;\n> +\n> \tif (data->useConverter_) {\n> \t\t/*\n> \t\t * When using the converter allocate a fixed number of internal\n> \t\t * buffers.\n> \t\t */\n> -\t\tret = video->allocateBuffers(kNumInternalBuffers,\n> +\t\tret = video->allocateBuffers(internalBufferCount,\n> \t\t\t\t\t &data->converterBuffers_);\n> \t} else {\n> -\t\t/* Otherwise, prepare for using buffers from the only stream. */\n> -\t\tStream *stream = &data->streams_[0];\n> -\t\tret = video->importBuffers(stream->configuration().bufferCount);\n> +\t\t/* Otherwise, prepare for using buffers. */\n> +\t\tret = video->importBuffers(kSimpleBufferSlotCount);\n> \t}\n> \tif (ret < 0)\n> \t\treturn ret;\n> @@ -852,7 +863,8 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL\n> \t}\n> \n> \tif (data->useConverter_) {\n> -\t\tret = converter_->start();\n> +\t\tret = converter_->start(internalBufferCount,\n> +\t\t\t\t\tkSimpleBufferSlotCount);\n> \t\tif (ret < 0) {\n> \t\t\tstop(camera);\n> \t\t\treturn ret;\n> -- \n> 2.33.0\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 5018BBE08B\n\tfor ;\n\tThu, 1 Dec 2022 12:29:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0770863335;\n\tThu, 1 Dec 2022 13:29:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED5F763335\n\tfor ;\n\tThu, 1 Dec 2022 13:29:46 +0100 (CET)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 436AB33F;\n\tThu, 1 Dec 2022 13:29:45 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669897788;\n\tbh=rmyewQ395+6Rn0e6YL5KuAXbQe2iPA/4zwvl2Nfi3e4=;\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=fJxNwvTn8ZuobbVyjRg9Ng0WdafiNfbSeFzLHcdgbjFit7c2KyNqoKL9iCu6YFuAe\n\trp7+LH1FAwChcKJRUQHJfJkI4qEe0laQivroRGqK+60c86JbDqYoCdyZN/ZZKhr/lT\n\tvXdtaZlFezbB5OQmWCLOzMXdTgGaDAQcZigLY51sq8D4rcip5li41BSjgYpXV6/6AI\n\tCDMB+vJTsptEGLRrA9GdbuPh50hxJyCkw8VOGyzKk+sjwIIyXpegd6DtykngxgTSTD\n\tGXJvdrr7NTkodUpSR+W1Vs6oT/VOmvPJ+WGwiLBoaxSgNxv21ep8xsEA5bhsM4LlMX\n\tdFJtcHh1ycJuw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669897786;\n\tbh=rmyewQ395+6Rn0e6YL5KuAXbQe2iPA/4zwvl2Nfi3e4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tR6BsWKXzraYALQVuTQpbAPkqf1jaZ2tuNVl6ydXG9Iqn6QxTJUif4kjeiaCkonTK\n\tBIXy4+d25DR8fSRopoFsV+wTze1fTHlKZZV6mZX/FDAG+BwaJwWjTTzIvi5o+FhApI\n\tw1hPq+N9528q8FeS6WSZHRTElf7UBLwwNpiUE9h8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tR6BsWKX\"; dkim-atps=neutral","Date":"Thu, 1 Dec 2022 21:29:39 +0900","To":"=?iso-8859-1?q?N=EDcolas_F=2E_R=2E_A=2E?= Prado ","Message-ID":"","References":"<20210824195636.1110845-1-nfraprado@collabora.com>\n\t<20210824195636.1110845-6-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210824195636.1110845-6-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v8 05/17] libcamera: pipeline: simple:\n\tDon'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, kernel@collabora.com,\n\t=?iso-8859-1?q?Andr=E9?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]