From patchwork Wed Dec 28 22:29:50 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Paul Elder X-Patchwork-Id: 18062 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 23DD4C3226 for ; Wed, 28 Dec 2022 22:30:29 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id C5E18625DF; Wed, 28 Dec 2022 23:30:28 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1672266628; bh=fm5mBlWrh1ps5b66vFDjfPs9kLP9NmvtJw6JPr2+r0g=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=DYCj6q3Pa9uKnWylWp70yEnDKXNeHzfmaQd4YsQiB7KqalOMeg2y5TxVJA45FD7pj VgllIw5CCb0aqdH0hiaUpM0kqkMERzutO8uRZ78cU+Qiqf2wFvdeRKzW/Bz/R+yivb 1WkpwZ328m+dsAnntvn3Y8AiDYmT51V7kCiSxZVmzZ8jLnfGk3xu8cRVuRosInhF5W eSZ/SloSNdd4VPJESM89NarlnHfO3LaBhzsk/TJdpK38JnsaGJUzbSqwU4oZDPGMZP KC4WChBq1uR6TcxuUm3QRDeEYKZQoVJOfSbTn/e3CchXvbpbfvw9ehzlXcFUqgFqGK gFrQ+2uhVBefA== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 98A44625D7 for ; Wed, 28 Dec 2022 23:30:24 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="Yhxe+qvk"; dkim-atps=neutral Received: from pyrite.mediacom.info (unknown [IPv6:2604:2d80:ad8a:9000:1bf9:855b:22de:3645]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B674109; Wed, 28 Dec 2022 23:30:23 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1672266624; bh=fm5mBlWrh1ps5b66vFDjfPs9kLP9NmvtJw6JPr2+r0g=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Yhxe+qvkjFCuQK/UnfSk88dCPB9medHtXtZ7grNoUp9qJAcNHmPEQw+/65f8QRwmW MPEpYRofSufyCD5a4TDOgjex7/WTH1N2Y4vWNSKt8oLqq+1N2SKTWvXTBrKUEfLKIF 688OJw9haYrjTIwvbLnuFHbK9F674JqoROgLOuEw= To: libcamera-devel@lists.libcamera.org Date: Wed, 28 Dec 2022 16:29:50 -0600 Message-Id: <20221228223003.2265712-7-paul.elder@ideasonboard.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20221228223003.2265712-1-paul.elder@ideasonboard.com> References: <20221228223003.2265712-1-paul.elder@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v10 06/19] libcamera: pipeline: simple: Don't rely on bufferCount X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Paul Elder via libcamera-devel From: Paul Elder Reply-To: Paul Elder Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" From: Nícolas F. R. A. Prado Currently the simple pipeline handler relies on bufferCount to decide on the number of buffers to allocate internally when a converter is in use and for the number of V4L2 buffer slots to reserve. Instead, the number of internal buffers should be the minimum required by the pipeline to keep the requests flowing, in order to avoid wasting memory, while the number of V4L2 buffer slots should be greater than the expected number of requests queued by the application, in order to avoid thrashing dmabuf mappings, which would degrade performance. Stop relying on bufferCount for these numbers and instead set them to appropriate, and independent, constants. Signed-off-by: Nícolas F. R. A. Prado Reviewed-by: Paul Elder Signed-off-by: Paul Elder --- Changes in v10: - rename internalBufferCount and bufferSlotCount to inputBufferCount and outputBufferCount, respectively - remove outputBufferCount from Converter::start() parameter - instead, each Converter will decide on their own (how do we like this?) Changes in v9: - rebased Changes in v8: - New --- include/libcamera/internal/converter.h | 2 +- .../internal/converter/converter_v4l2_m2m.h | 4 ++-- src/libcamera/converter.cpp | 2 ++ .../converter/converter_v4l2_m2m.cpp | 12 ++++++----- src/libcamera/pipeline/simple/simple.cpp | 21 ++++++++++++++----- 5 files changed, 28 insertions(+), 13 deletions(-) diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h index 834ec5ab..e8a5aeca 100644 --- a/include/libcamera/internal/converter.h +++ b/include/libcamera/internal/converter.h @@ -49,7 +49,7 @@ public: virtual int exportBuffers(unsigned int output, unsigned int count, std::vector> *buffers) = 0; - virtual int start() = 0; + virtual int start(unsigned int inputBufferCount) = 0; virtual void stop() = 0; virtual int queueBuffers(FrameBuffer *input, diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h index 815916d0..8aedb6df 100644 --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h @@ -50,7 +50,7 @@ public: int exportBuffers(unsigned int ouput, unsigned int count, std::vector> *buffers); - int start(); + int start(unsigned int inputBufferCount); void stop(); int queueBuffers(FrameBuffer *input, @@ -69,7 +69,7 @@ private: int exportBuffers(unsigned int count, std::vector> *buffers); - int start(); + int start(unsigned int inputBufferCount); void stop(); int queueBuffers(FrameBuffer *input, FrameBuffer *output); diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp index 3de39cff..2413424b 100644 --- a/src/libcamera/converter.cpp +++ b/src/libcamera/converter.cpp @@ -127,6 +127,8 @@ Converter::~Converter() /** * \fn Converter::start() * \brief Start the converter streaming operation + * \param[in] inputBufferCount Number of input buffers that will be used to + * move video capture device frames into the converter. * \return 0 on success or a negative error code otherwise */ diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp index 2a4d1d99..261c3329 100644 --- a/src/libcamera/converter/converter_v4l2_m2m.cpp +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp @@ -107,13 +107,15 @@ int V4L2M2MConverter::Stream::exportBuffers(unsigned int count, return m2m_->capture()->exportBuffers(count, buffers); } -int V4L2M2MConverter::Stream::start() +int V4L2M2MConverter::Stream::start(unsigned int inputBufferCount) { - int ret = m2m_->output()->importBuffers(inputBufferCount_); + static constexpr unsigned int kOutputBufferCount = 16; + + int ret = m2m_->output()->importBuffers(inputBufferCount); if (ret < 0) return ret; - ret = m2m_->capture()->importBuffers(outputBufferCount_); + ret = m2m_->capture()->importBuffers(kOutputBufferCount); if (ret < 0) { stop(); return ret; @@ -373,12 +375,12 @@ int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count, /** * \copydoc libcamera::Converter::start */ -int V4L2M2MConverter::start() +int V4L2M2MConverter::start(unsigned int inputBufferCount) { int ret; for (Stream &stream : streams_) { - ret = stream.start(); + ret = stream.start(inputBufferCount); if (ret < 0) { stop(); return ret; diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 6b7c6d5c..64f341e1 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -339,6 +339,7 @@ protected: private: static constexpr unsigned int kNumInternalBuffers = 3; + static constexpr unsigned int kSimpleBufferSlotCount = 16; struct EntityData { std::unique_ptr video; @@ -1232,17 +1233,27 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL return -EBUSY; } + /* + * Number of internal buffers that will be used to move video capture + * device frames to the converter. + * + * \todo Make this depend on the driver in use instead of being + * hardcoded. In order to not drop frames, the realtime requirements for + * each device should be checked, so it may not be as simple as just + * using the minimum number of buffers required by the driver. + */ + static constexpr unsigned int internalBufferCount = 3; + if (data->useConverter_) { /* * When using the converter allocate a fixed number of internal * buffers. */ - ret = video->allocateBuffers(kNumInternalBuffers, + ret = video->allocateBuffers(internalBufferCount, &data->converterBuffers_); } else { - /* Otherwise, prepare for using buffers from the only stream. */ - Stream *stream = &data->streams_[0]; - ret = video->importBuffers(stream->configuration().bufferCount); + /* Otherwise, prepare for using buffers. */ + ret = video->importBuffers(kSimpleBufferSlotCount); } if (ret < 0) { releasePipeline(data); @@ -1258,7 +1269,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL } if (data->useConverter_) { - ret = data->converter_->start(); + ret = data->converter_->start(internalBufferCount); if (ret < 0) { stop(camera); return ret;