From patchwork Thu Jul 22 23:28:49 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= X-Patchwork-Id: 13083 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 C5D81C0109 for ; Thu, 22 Jul 2021 23:29:45 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 8D3C6687A3; Fri, 23 Jul 2021 01:29:45 +0200 (CEST) Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 42761687A4 for ; Fri, 23 Jul 2021 01:29:44 +0200 (CEST) Received: from localhost.localdomain (unknown [IPv6:2804:14c:1a9:2434:a4c5:aa5e:1d8c:5e2c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: nfraprado) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id 4DD1A1F447CE; Fri, 23 Jul 2021 00:29:42 +0100 (BST) From: =?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= To: libcamera-devel@lists.libcamera.org Date: Thu, 22 Jul 2021 20:28:49 -0300 Message-Id: <20210722232851.747614-10-nfraprado@collabora.com> X-Mailer: git-send-email 2.32.0 In-Reply-To: <20210722232851.747614-1-nfraprado@collabora.com> References: <20210722232851.747614-1-nfraprado@collabora.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v7 09/11] libcamera: pipeline: 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: , Cc: kernel@collabora.com, =?utf-8?q?Andr=C3=A9_Almeida?= Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Pipelines have relied on bufferCount to decide on the number of buffers to allocate internally through allocateBuffers() and on the number of V4L2 buffer slots to reserve through importBuffers(). Instead, the number of internal buffers should be the minimum required by the algorithms to avoid wasting memory, and the number of V4L2 buffer slots should overallocate to avoid thrashing dmabuf mappings. For now, just set them to constants and stop relying on bufferCount, to allow for its removal. Signed-off-by: Nícolas F. R. A. Prado --- No changes in v7 Changes in v6: - Added pipeline name as prefix to each BUFFER_SLOT_COUNT and INTERNAL_BUFFER_COUNT constant src/libcamera/pipeline/ipu3/imgu.cpp | 12 ++++++------ src/libcamera/pipeline/ipu3/imgu.h | 5 ++++- src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +-------- .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++++---------- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++------- src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 +- src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +++ src/libcamera/pipeline/simple/converter.cpp | 4 ++-- src/libcamera/pipeline/simple/converter.h | 3 +++ src/libcamera/pipeline/simple/simple.cpp | 6 ++---- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +++-- src/libcamera/pipeline/vimc/vimc.cpp | 5 +++-- 12 files changed, 35 insertions(+), 43 deletions(-) diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp index e955bc3456ba..f36e99dacbe7 100644 --- a/src/libcamera/pipeline/ipu3/imgu.cpp +++ b/src/libcamera/pipeline/ipu3/imgu.cpp @@ -593,22 +593,22 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad, /** * \brief Allocate buffers for all the ImgU video devices */ -int ImgUDevice::allocateBuffers(unsigned int bufferCount) +int ImgUDevice::allocateBuffers() { /* Share buffers between CIO2 output and ImgU input. */ - int ret = input_->importBuffers(bufferCount); + int ret = input_->importBuffers(IPU3_BUFFER_SLOT_COUNT); if (ret) { LOG(IPU3, Error) << "Failed to import ImgU input buffers"; return ret; } - ret = param_->allocateBuffers(bufferCount, ¶mBuffers_); + ret = param_->allocateBuffers(IPU3_INTERNAL_BUFFER_COUNT, ¶mBuffers_); if (ret < 0) { LOG(IPU3, Error) << "Failed to allocate ImgU param buffers"; goto error; } - ret = stat_->allocateBuffers(bufferCount, &statBuffers_); + ret = stat_->allocateBuffers(IPU3_INTERNAL_BUFFER_COUNT, &statBuffers_); if (ret < 0) { LOG(IPU3, Error) << "Failed to allocate ImgU stat buffers"; goto error; @@ -619,13 +619,13 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount) * corresponding stream is active or inactive, as the driver needs * buffers to be requested on the V4L2 devices in order to operate. */ - ret = output_->importBuffers(bufferCount); + ret = output_->importBuffers(IPU3_BUFFER_SLOT_COUNT); if (ret < 0) { LOG(IPU3, Error) << "Failed to import ImgU output buffers"; goto error; } - ret = viewfinder_->importBuffers(bufferCount); + ret = viewfinder_->importBuffers(IPU3_BUFFER_SLOT_COUNT); if (ret < 0) { LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers"; goto error; diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h index 9d4915116087..f934a951fc75 100644 --- a/src/libcamera/pipeline/ipu3/imgu.h +++ b/src/libcamera/pipeline/ipu3/imgu.h @@ -61,7 +61,7 @@ public: outputFormat); } - int allocateBuffers(unsigned int bufferCount); + int allocateBuffers(); void freeBuffers(); int start(); @@ -86,6 +86,9 @@ private: static constexpr unsigned int PAD_VF = 3; static constexpr unsigned int PAD_STAT = 4; + static constexpr unsigned int IPU3_INTERNAL_BUFFER_COUNT = 4; + static constexpr unsigned int IPU3_BUFFER_SLOT_COUNT = 5; + int linkSetup(const std::string &source, unsigned int sourcePad, const std::string &sink, unsigned int sinkPad, bool enable); diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 5fd1757bfe13..4efd201c05e5 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -681,16 +681,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera) { IPU3CameraData *data = cameraData(camera); ImgUDevice *imgu = data->imgu_; - unsigned int bufferCount; int ret; - bufferCount = std::max({ - data->outStream_.configuration().bufferCount, - data->vfStream_.configuration().bufferCount, - data->rawStream_.configuration().bufferCount, - }); - - ret = imgu->allocateBuffers(bufferCount); + ret = imgu->allocateBuffers(); if (ret < 0) return ret; diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index d1cd3d9dc082..776e0f92aed1 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1149,20 +1149,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) { RPiCameraData *data = cameraData(camera); int ret; + constexpr unsigned int bufferCount = 4; /* - * Decide how many internal buffers to allocate. For now, simply look - * at how many external buffers will be provided. We'll need to improve - * this logic. However, we really must have all streams allocate the same - * number of buffers to simplify error handling in queueRequestDevice(). + * Allocate internal buffers. We really must have all streams allocate + * the same number of buffers to simplify error handling in + * queueRequestDevice(). */ - unsigned int maxBuffers = 0; - for (const Stream *s : camera->streams()) - if (static_cast(s)->isExternal()) - maxBuffers = std::max(maxBuffers, s->configuration().bufferCount); - for (auto const stream : data->streams_) { - ret = stream->prepareBuffers(maxBuffers); + ret = stream->prepareBuffers(bufferCount); if (ret < 0) return ret; } diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 11325875b929..f4ea2fd4d4d0 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -690,16 +690,11 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) unsigned int ipaBufferId = 1; int ret; - unsigned int maxCount = std::max({ - data->mainPathStream_.configuration().bufferCount, - data->selfPathStream_.configuration().bufferCount, - }); - - ret = param_->allocateBuffers(maxCount, ¶mBuffers_); + ret = param_->allocateBuffers(RKISP1_INTERNAL_BUFFER_COUNT, ¶mBuffers_); if (ret < 0) goto error; - ret = stat_->allocateBuffers(maxCount, &statBuffers_); + ret = stat_->allocateBuffers(RKISP1_INTERNAL_BUFFER_COUNT, &statBuffers_); if (ret < 0) goto error; diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp index 25f482eb8d8e..fea330f72886 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp @@ -172,7 +172,7 @@ int RkISP1Path::start() return -EBUSY; /* \todo Make buffer count user configurable. */ - ret = video_->importBuffers(RKISP1_BUFFER_COUNT); + ret = video_->importBuffers(RKISP1_BUFFER_SLOT_COUNT); if (ret) return ret; diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h index 91757600ccdc..3c5891009c58 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h @@ -27,6 +27,9 @@ class V4L2Subdevice; struct StreamConfiguration; struct V4L2SubdeviceFormat; +static constexpr unsigned int RKISP1_INTERNAL_BUFFER_COUNT = 4; +static constexpr unsigned int RKISP1_BUFFER_SLOT_COUNT = 5; + class RkISP1Path { public: diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp index b5e34c4cd0c5..b3bcf01483f7 100644 --- a/src/libcamera/pipeline/simple/converter.cpp +++ b/src/libcamera/pipeline/simple/converter.cpp @@ -103,11 +103,11 @@ int SimpleConverter::Stream::exportBuffers(unsigned int count, int SimpleConverter::Stream::start() { - int ret = m2m_->output()->importBuffers(inputBufferCount_); + int ret = m2m_->output()->importBuffers(SIMPLE_BUFFER_SLOT_COUNT); if (ret < 0) return ret; - ret = m2m_->capture()->importBuffers(outputBufferCount_); + ret = m2m_->capture()->importBuffers(SIMPLE_BUFFER_SLOT_COUNT); if (ret < 0) { stop(); return ret; diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h index 276a2a291c21..7e1d60674f62 100644 --- a/src/libcamera/pipeline/simple/converter.h +++ b/src/libcamera/pipeline/simple/converter.h @@ -29,6 +29,9 @@ class SizeRange; struct StreamConfiguration; class V4L2M2MDevice; +constexpr unsigned int SIMPLE_INTERNAL_BUFFER_COUNT = 5; +constexpr unsigned int SIMPLE_BUFFER_SLOT_COUNT = 5; + class SimpleConverter { public: diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 1c25a7344f5f..a1163eaf8be2 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -803,12 +803,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL * When using the converter allocate a fixed number of internal * buffers. */ - ret = video->allocateBuffers(kNumInternalBuffers, + ret = video->allocateBuffers(SIMPLE_INTERNAL_BUFFER_COUNT, &data->converterBuffers_); } else { - /* Otherwise, prepare for using buffers from the only stream. */ - Stream *stream = &data->streams_[0]; - ret = video->importBuffers(stream->configuration().bufferCount); + ret = video->importBuffers(SIMPLE_BUFFER_SLOT_COUNT); } if (ret < 0) return ret; diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index fd39b3d3c72c..755949e7a59a 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -91,6 +91,8 @@ private: return static_cast( PipelineHandler::cameraData(camera)); } + + static constexpr unsigned int UVC_BUFFER_SLOT_COUNT = 5; }; UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data) @@ -236,9 +238,8 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList *controls) { UVCCameraData *data = cameraData(camera); - unsigned int count = data->stream_.configuration().bufferCount; - int ret = data->video_->importBuffers(count); + int ret = data->video_->importBuffers(UVC_BUFFER_SLOT_COUNT); if (ret < 0) return ret; diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp index e89d53182c6d..24ba743a946c 100644 --- a/src/libcamera/pipeline/vimc/vimc.cpp +++ b/src/libcamera/pipeline/vimc/vimc.cpp @@ -102,6 +102,8 @@ private: return static_cast( PipelineHandler::cameraData(camera)); } + + static constexpr unsigned int VIMC_BUFFER_SLOT_COUNT = 5; }; namespace { @@ -312,9 +314,8 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlList *controls) { VimcCameraData *data = cameraData(camera); - unsigned int count = data->stream_.configuration().bufferCount; - int ret = data->video_->importBuffers(count); + int ret = data->video_->importBuffers(VIMC_BUFFER_SLOT_COUNT); if (ret < 0) return ret;