From patchwork Mon Apr 28 09:02:31 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Sven_P=C3=BCschel?= X-Patchwork-Id: 23278 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 702A3C3320 for ; Mon, 28 Apr 2025 09:05:22 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id C34AF68B31; Mon, 28 Apr 2025 11:05:19 +0200 (CEST) Received: from metis.whiteo.stw.pengutronix.de (metis.whiteo.stw.pengutronix.de [IPv6:2a0a:edc0:2:b01:1d::104]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 16FEF68AD7 for ; Mon, 28 Apr 2025 11:05:07 +0200 (CEST) Received: from ptz.office.stw.pengutronix.de ([2a0a:edc0:0:900:1d::77] helo=peter.guest.stw.pengutronix.de) by metis.whiteo.stw.pengutronix.de with esmtp (Exim 4.92) (envelope-from ) id 1u9KQE-0001au-Mx; Mon, 28 Apr 2025 11:05:06 +0200 From: =?utf-8?q?Sven_P=C3=BCschel?= To: libcamera-devel@lists.libcamera.org Cc: =?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= , Paul Elder , =?utf-8?q?Sven_P=C3=BCschel?= Subject: [PATCH v11 06/19] libcamera: pipeline: simple: Don't rely on bufferCount Date: Mon, 28 Apr 2025 11:02:31 +0200 Message-ID: <20250428090413.38234-7-s.pueschel@pengutronix.de> X-Mailer: git-send-email 2.49.0 In-Reply-To: <20250428090413.38234-1-s.pueschel@pengutronix.de> References: <20250428090413.38234-1-s.pueschel@pengutronix.de> MIME-Version: 1.0 X-SA-Exim-Connect-IP: 2a0a:edc0:0:900:1d::77 X-SA-Exim-Mail-From: s.pueschel@pengutronix.de X-SA-Exim-Scanned: No (on metis.whiteo.stw.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: libcamera-devel@lists.libcamera.org 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: , 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 Signed-off-by: Sven Püschel --- Changes in v11: - added rkisp1 adjustments for the dewarper 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/rkisp1/rkisp1.cpp | 2 +- src/libcamera/pipeline/simple/simple.cpp | 21 ++++++++++++++----- 6 files changed, 29 insertions(+), 14 deletions(-) diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h index 644ec429..1da51d1e 100644 --- a/include/libcamera/internal/converter.h +++ b/include/libcamera/internal/converter.h @@ -75,7 +75,7 @@ public: virtual int exportBuffers(const Stream *stream, 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 0ad7bf7f..24997c6f 100644 --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h @@ -59,7 +59,7 @@ public: int exportBuffers(const Stream *stream, unsigned int count, std::vector> *buffers) override; - int start() override; + int start(unsigned int inputBufferCount) override; void stop() override; int validateOutput(StreamConfiguration *cfg, bool *adjusted, @@ -85,7 +85,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 d551b908..2c6f1e4b 100644 --- a/src/libcamera/converter.cpp +++ b/src/libcamera/converter.cpp @@ -190,6 +190,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 ee05b798..242e0d85 100644 --- a/src/libcamera/converter/converter_v4l2_m2m.cpp +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp @@ -160,13 +160,15 @@ int V4L2M2MConverter::V4L2M2MStream::exportBuffers(unsigned int count, return m2m_->capture()->exportBuffers(count, buffers); } -int V4L2M2MConverter::V4L2M2MStream::start() +int V4L2M2MConverter::V4L2M2MStream::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; @@ -620,12 +622,12 @@ V4L2M2MConverter::inputCropBounds(const Stream *stream) /** * \copydoc libcamera::Converter::start */ -int V4L2M2MConverter::start() +int V4L2M2MConverter::start(unsigned int inputBufferCount) { int ret; for (auto &iter : streams_) { - ret = iter.second->start(); + ret = iter.second->start(inputBufferCount); if (ret < 0) { stop(); return ret; diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 81bf1c55..623bcfe5 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -1110,7 +1110,7 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL actions += [&]() { stat_->streamOff(); }; if (useDewarper_) { - ret = dewarper_->start(); + ret = dewarper_->start(kRkISP1InternalBufferCount); if (ret) { LOG(RkISP1, Error) << "Failed to start dewarper"; return ret; diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 1e77ccdc..4f4cba06 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -422,6 +422,7 @@ protected: private: static constexpr unsigned int kNumInternalBuffers = 3; + static constexpr unsigned int kSimpleBufferSlotCount = 16; struct EntityData { std::unique_ptr video; @@ -1466,17 +1467,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->useConversion_) { /* * When using the converter allocate a fixed number of internal * buffers. */ - ret = video->allocateBuffers(kNumInternalBuffers, + ret = video->allocateBuffers(internalBufferCount, &data->conversionBuffers_); } 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); @@ -1504,7 +1515,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL if (data->useConversion_) { if (data->converter_) - ret = data->converter_->start(); + ret = data->converter_->start(internalBufferCount); else if (data->swIsp_) ret = data->swIsp_->start(); else