From patchwork Wed Dec 28 22:29:49 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Paul Elder X-Patchwork-Id: 18061 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 0A2DEC3220 for ; Wed, 28 Dec 2022 22:30:28 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 856A0625D8; Wed, 28 Dec 2022 23:30:27 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1672266627; bh=UxTRy6b7TsWo1BB4KSjqmp/xXP73ssNrieC+W2NcEck=; 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=0lzX374wJMRdvP6C29Whoh2oILdlqg6XJ0RLZGkhqyUIIaTThfRVtEid7vsVisUeW myIFD4EFu2QzW2e+hHlBHiBfXknGg9i5/R5ipf+HnUZKH544vKkoAABK6VB+TTAghz DV6nA9PGUuk32zXtPAw53sClbiLY/6BxoLL075SM6qL/gHsunjAuGmjnXX4UOH8WIR 63c3yjmZuFiKEEBogGfA95kJgkvCgwx0wnWGgrAbNyBz94SyIYNaYn5TQKvK3MHTY7 VgUUh8sLTQngPE58DNFYVItvK4sNYaPstNHJMZebrfhxpxVFavDEIC0obo2KfyB+V8 /xxIcP+jsM9tw== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 79328625DD for ; Wed, 28 Dec 2022 23:30:23 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="v7hlpfnS"; 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 32F1525B; Wed, 28 Dec 2022 23:30:22 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1672266623; bh=UxTRy6b7TsWo1BB4KSjqmp/xXP73ssNrieC+W2NcEck=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=v7hlpfnSNSbJH5z+jQKEdf0SA8eSqJNWfCm89tCIBZMvXUvxQlQIxQor94GkZNgho S0WjfvVik44SDaH6v7nq6geWiOAp1UK/Xvu6W0tDh4btXpQeEhW5ElaQ9VWWs01Oyi CKTDMDQs9eJfFQ94bwUDcK5hEO6lmafEbHm91CFE= To: libcamera-devel@lists.libcamera.org Date: Wed, 28 Dec 2022 16:29:49 -0600 Message-Id: <20221228223003.2265712-6-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 05/19] libcamera: pipeline: ipu3: 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 ipu3 pipeline handler relies on bufferCount to decide on the number of V4L2 buffer slots to reserve as well as for the number of buffers to allocate internally for the CIO2 raw buffers and parameter-statistics ImgU buffer pairs. Instead, the number of internal buffers should be the minimum required by the pipeline to keep the requests flowing without frame drops, 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: - make hardcoded internal buffer allocation counts for CIO2 and IMGU come from MinimumRequests Changes in v9: - rebase Changes in v8: - New --- src/libcamera/pipeline/ipu3/cio2.cpp | 8 +++--- src/libcamera/pipeline/ipu3/cio2.h | 4 +-- src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++---- src/libcamera/pipeline/ipu3/imgu.h | 3 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 43 ++++++++++++++++++++-------- 5 files changed, 45 insertions(+), 26 deletions(-) diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index d4e523af..e999b7ae 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -234,7 +234,6 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const cfg.size = sensorFormat.size; cfg.pixelFormat = mbusCodesToPixelFormat.at(sensorFormat.mbus_code); - cfg.bufferCount = kBufferCount; return cfg; } @@ -335,13 +334,14 @@ int CIO2Device::exportBuffers(unsigned int count, return output_->exportBuffers(count, buffers); } -int CIO2Device::start() +int CIO2Device::start(unsigned int internalBufferCount, + unsigned int bufferSlotCount) { - int ret = output_->exportBuffers(kBufferCount, &buffers_); + int ret = output_->exportBuffers(internalBufferCount, &buffers_); if (ret < 0) return ret; - ret = output_->importBuffers(kBufferCount); + ret = output_->importBuffers(bufferSlotCount); if (ret) LOG(IPU3, Error) << "Failed to import CIO2 buffers"; diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h index 68504a2d..14372710 100644 --- a/src/libcamera/pipeline/ipu3/cio2.h +++ b/src/libcamera/pipeline/ipu3/cio2.h @@ -30,8 +30,6 @@ struct StreamConfiguration; class CIO2Device { public: - static constexpr unsigned int kBufferCount = 4; - CIO2Device(); std::vector formats() const; @@ -48,7 +46,7 @@ public: V4L2SubdeviceFormat getSensorFormat(const std::vector &mbusCodes, const Size &size) const; - int start(); + int start(unsigned int internalBufferCount, unsigned int bufferSlotCount); int stop(); CameraSensor *sensor() { return sensor_.get(); } diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp index 531879f1..e5ec2cba 100644 --- a/src/libcamera/pipeline/ipu3/imgu.cpp +++ b/src/libcamera/pipeline/ipu3/imgu.cpp @@ -576,22 +576,23 @@ 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(unsigned int internalBufferCount, + unsigned int bufferSlotCount) { /* Share buffers between CIO2 output and ImgU input. */ - int ret = input_->importBuffers(bufferCount); + int ret = input_->importBuffers(bufferSlotCount); if (ret) { LOG(IPU3, Error) << "Failed to import ImgU input buffers"; return ret; } - ret = param_->allocateBuffers(bufferCount, ¶mBuffers_); + ret = param_->allocateBuffers(internalBufferCount, ¶mBuffers_); if (ret < 0) { LOG(IPU3, Error) << "Failed to allocate ImgU param buffers"; goto error; } - ret = stat_->allocateBuffers(bufferCount, &statBuffers_); + ret = stat_->allocateBuffers(internalBufferCount, &statBuffers_); if (ret < 0) { LOG(IPU3, Error) << "Failed to allocate ImgU stat buffers"; goto error; @@ -602,13 +603,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(bufferSlotCount); if (ret < 0) { LOG(IPU3, Error) << "Failed to import ImgU output buffers"; goto error; } - ret = viewfinder_->importBuffers(bufferCount); + ret = viewfinder_->importBuffers(bufferSlotCount); 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 0af4dd8a..668fa427 100644 --- a/src/libcamera/pipeline/ipu3/imgu.h +++ b/src/libcamera/pipeline/ipu3/imgu.h @@ -84,7 +84,8 @@ public: outputFormat); } - int allocateBuffers(unsigned int bufferCount); + int allocateBuffers(unsigned int internalBufferCount, + unsigned int bufferSlotCount); void freeBuffers(); int start(); diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index f4cc2467..790b2665 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -160,7 +160,7 @@ private: int updateControls(IPU3CameraData *data); int registerCameras(); - int allocateBuffers(Camera *camera); + int allocateBuffers(Camera *camera, unsigned int bufferSlotCount); int freeBuffers(Camera *camera); ImgUDevice imgu0_; @@ -171,6 +171,7 @@ private: std::vector ipaBuffers_; static constexpr unsigned int kMinimumRequests = 3; + static constexpr unsigned int kIPU3BufferSlotCount = 16; }; IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data) @@ -712,20 +713,25 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, * In order to be able to start the 'viewfinder' and 'stat' nodes, we need * memory to be reserved. */ -int PipelineHandlerIPU3::allocateBuffers(Camera *camera) +int PipelineHandlerIPU3::allocateBuffers(Camera *camera, + unsigned int bufferSlotCount) { 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); + /* + * This many internal buffers (or rather parameter and statistics buffer + * pairs) for the ImgU ensures that the pipeline runs smoothly, without + * frame drops. This number considers: + * - three buffers queued to the CIO2 (Since these buffers are bound to + * CIO2 buffers before queuing to the CIO2) + * - one buffer under processing in ImgU + * + * \todo Update this number when we make these buffers only get added to + * the FrameInfo after the raw buffers are dequeued from CIO2. + */ + ret = imgu->allocateBuffers(kMinimumRequests + 1, bufferSlotCount); if (ret < 0) return ret; @@ -783,7 +789,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis return ret; /* Allocate buffers for internal pipeline usage. */ - ret = allocateBuffers(camera); + ret = allocateBuffers(camera, kIPU3BufferSlotCount); if (ret) return ret; @@ -796,8 +802,21 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis /* * Start the ImgU video devices, buffers will be queued to the * ImgU output and viewfinder when requests will be queued. + * + * This many internal buffers for the CIO2 ensures that the pipeline + * runs smoothly, without frame drops. This number considers: + * - one buffer being DMA'ed to in CIO2 + * - one buffer programmed by the CIO2 as the next buffer + * - one buffer under processing in ImgU + * - one extra idle buffer queued to CIO2, to account for possible + * delays in requeuing the buffer from ImgU back to CIO2 + * + * Transient situations can arise when one of the parts, CIO2 or ImgU, + * finishes its processing first and experiences a lack of buffers, but + * they will shortly after return to the state described above as the + * other part catches up. */ - ret = cio2->start(); + ret = cio2->start(kMinimumRequests + 1, kIPU3BufferSlotCount); if (ret) goto error;