From patchwork Fri Dec 16 12:29:25 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Paul Elder X-Patchwork-Id: 18018 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 D811EC31E9 for ; Fri, 16 Dec 2022 12:30:11 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 9CC82633A4; Fri, 16 Dec 2022 13:30:11 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1671193811; bh=+VGjpxv0w1TaA3+XrC+I8rEgzQ9lwcHkDQKe06UxPO8=; 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=q1XYAPa7ZhqJn5Uj4YaBUyn4TP6LAy28ksL0Rl3ekGwYJWv4JFdgPZlDUA3+k9/La 1xQe4OIqX7t/vY59TY9lu/zYTypZzyvosg4gCWCgsBM7EeI8f4c4y1fSYb6q2N/opS 3rGhD7oNi5BIhFbvTVi+BTYKyiureLMvedzFpWonxe2K0Wato6N4euV0Fn3D1F7WnQ 4LGapyoI2OCrGJhmJiMXbO/6nMzkAEmffFMDj5azFkQzPtZW1pglhW0j/3h2riUto4 7/POg9zNm01hPEOXgjSqZKi4oHFNh1n3UNRqJWQVxUFWihlXuteUL+jxwNbDwQBXvb fq6v1UXFDwwCw== 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 065436339D for ; Fri, 16 Dec 2022 13:30:09 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="nT/0Iop3"; dkim-atps=neutral Received: from pyrite.tail37cf.ts.net (h175-177-042-159.catv02.itscom.jp [175.177.42.159]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 93F80110E; Fri, 16 Dec 2022 13:30:07 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1671193808; bh=+VGjpxv0w1TaA3+XrC+I8rEgzQ9lwcHkDQKe06UxPO8=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=nT/0Iop3tMgQDi7LT51f6+6+GFCkatz84fOgoWZkRLt2z2T/dn96EJaUOWY6bVN+Q wS60brAuqj/CrIk3RqtsM7baO7C+qqbzp+OoBgOH1btqVyzYo+/PSLEzeg/rdWWtB8 +5ChNbX1L53eoUhLyOS31KTeOxjecFQOfZmEVCrY= To: libcamera-devel@lists.libcamera.org Date: Fri, 16 Dec 2022 21:29:25 +0900 Message-Id: <20221216122939.256534-5-paul.elder@ideasonboard.com> X-Mailer: git-send-email 2.35.1 In-Reply-To: <20221216122939.256534-1-paul.elder@ideasonboard.com> References: <20221216122939.256534-1-paul.elder@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v9 04/18] libcamera: pipeline: raspberrypi: 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 raspberrypi pipeline handler relies on bufferCount to decide on the number of buffers to allocate internally 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, in order to avoid thrashing dmabuf mappings, which would degrade performance. Extend the Stream class to receive the number of internal buffers that should be allocated, and optionally the number of buffer slots to reserve. Use these new member variables to specify better suited numbers for internal buffers and buffer slots for each stream individually, which also allows us to remove bufferCount. Signed-off-by: Nícolas F. R. A. Prado Signed-off-by: Paul Elder --- Changes in v9: - rebased - I've decided that the buffer allocation decisions that Nícolas implemented covered the same cases that were added in PipelineHandlerRPi::prepareBuffers(), but in a slightly nicer way, especially considering that bufferCount is to be removed from StreamConfiguration in this series. Comments welcome, of course. Changes in v8: - Reworked buffer allocation handling in the raspberrypi pipeline handler - New --- .../pipeline/raspberrypi/raspberrypi.cpp | 83 +++++++------------ .../pipeline/raspberrypi/rpi_stream.cpp | 39 +++++---- .../pipeline/raspberrypi/rpi_stream.h | 24 +++++- 3 files changed, 71 insertions(+), 75 deletions(-) diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 4641c76f..72502c36 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -341,6 +341,25 @@ public: void releaseDevice(Camera *camera) override; private: + /* + * The number of buffers to allocate internally for transferring raw + * frames from the Unicam Image stream to the ISP Input stream. It is + * defined such that: + * - one buffer is being written to in Unicam Image + * - one buffer is being processed in ISP Input + * - one extra buffer is queued to Unicam Image + */ + static constexpr unsigned int kInternalRawBufferCount = 3; + + /* + * The number of buffers to allocate internally for the external + * streams. They're used to drop the first few frames while the IPA + * algorithms converge. It is defined such that: + * - one buffer is being used on the stream + * - one extra buffer is queued + */ + static constexpr unsigned int kInternalDropoutBufferCount = 2; + RPiCameraData *cameraData(Camera *camera) { return static_cast(camera->_d()); @@ -1221,21 +1240,23 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me return -ENOENT; /* Locate and open the unicam video streams. */ - data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage); + data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage, + kInternalRawBufferCount); /* An embedded data node will not be present if the sensor does not support it. */ MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded"); if (unicamEmbedded) { - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded); + data->unicam_[Unicam::Embedded] = + RPi::Stream("Unicam Embedded", unicamEmbedded, kInternalRawBufferCount); data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(), &RPiCameraData::unicamBufferDequeue); } /* Tag the ISP input stream as an import stream. */ - data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true); - data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1); - data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2); - data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3); + data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, 0, kInternalRawBufferCount, true); + data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1, kInternalDropoutBufferCount); + data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2, kInternalDropoutBufferCount); + data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3, kInternalDropoutBufferCount); /* Wire up all the buffer connections. */ data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout); @@ -1428,57 +1449,11 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera) int PipelineHandlerRPi::prepareBuffers(Camera *camera) { RPiCameraData *data = cameraData(camera); - unsigned int numRawBuffers = 0; int ret; - for (Stream *s : camera->streams()) { - if (isRaw(s->configuration().pixelFormat)) { - numRawBuffers = s->configuration().bufferCount; - break; - } - } - - /* Decide how many internal buffers to allocate. */ + /* Allocate internal buffers. */ for (auto const stream : data->streams_) { - unsigned int numBuffers; - /* - * For Unicam, allocate a minimum of 4 buffers as we want - * to avoid any frame drops. - */ - constexpr unsigned int minBuffers = 4; - if (stream == &data->unicam_[Unicam::Image]) { - /* - * If an application has configured a RAW stream, allocate - * additional buffers to make up the minimum, but ensure - * we have at least 2 sets of internal buffers to use to - * minimise frame drops. - */ - numBuffers = std::max(2, minBuffers - numRawBuffers); - } else if (stream == &data->isp_[Isp::Input]) { - /* - * ISP input buffers are imported from Unicam, so follow - * similar logic as above to count all the RAW buffers - * available. - */ - numBuffers = numRawBuffers + std::max(2, minBuffers - numRawBuffers); - - } else if (stream == &data->unicam_[Unicam::Embedded]) { - /* - * Embedded data buffers are (currently) for internal use, - * so allocate the minimum required to avoid frame drops. - */ - numBuffers = minBuffers; - } else { - /* - * Since the ISP runs synchronous with the IPA and requests, - * we only ever need one set of internal buffers. Any buffers - * the application wants to hold onto will already be exported - * through PipelineHandlerRPi::exportFrameBuffers(). - */ - numBuffers = 1; - } - - ret = stream->prepareBuffers(numBuffers); + ret = stream->prepareBuffers(); if (ret < 0) return ret; } diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp index 2bb10f25..cde04efa 100644 --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp @@ -84,14 +84,24 @@ void Stream::removeExternalBuffer(FrameBuffer *buffer) bufferMap_.erase(id); } -int Stream::prepareBuffers(unsigned int count) +int Stream::prepareBuffers() { + unsigned int slotCount; int ret; if (!importOnly_) { - if (count) { + /* + * External streams overallocate buffer slots in order to handle + * the buffers queued from applications without degrading + * performance. Internal streams only need to have enough buffer + * slots to have the internal buffers queued. + */ + slotCount = isExternal() ? kRPIExternalBufferSlotCount + : internalBufferCount_; + + if (internalBufferCount_) { /* Export some frame buffers for internal use. */ - ret = dev_->exportBuffers(count, &internalBuffers_); + ret = dev_->exportBuffers(internalBufferCount_, &internalBuffers_); if (ret < 0) return ret; @@ -100,23 +110,16 @@ int Stream::prepareBuffers(unsigned int count) resetBuffers(); } - /* We must import all internal/external exported buffers. */ - count = bufferMap_.size(); + } else { + /* + * An importOnly stream doesn't have its own internal buffers, + * so it needs to have the number of buffer slots to reserve + * explicitly declared. + */ + slotCount = bufferSlotCount_; } - /* - * If this is an external stream, we must allocate slots for buffers that - * might be externally allocated. We have no indication of how many buffers - * may be used, so this might overallocate slots in the buffer cache. - * Similarly, if this stream is only importing buffers, we do the same. - * - * \todo Find a better heuristic, or, even better, an exact solution to - * this issue. - */ - if (isExternal() || importOnly_) - count = count * 2; - - return dev_->importBuffers(count); + return dev_->importBuffers(slotCount); } int Stream::queueBuffer(FrameBuffer *buffer) diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h index b8bd79cf..e63bf02b 100644 --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h @@ -42,9 +42,13 @@ public: { } - Stream(const char *name, MediaEntity *dev, bool importOnly = false) + Stream(const char *name, MediaEntity *dev, + unsigned int internalBufferCount, + unsigned int bufferSlotCount = 0, bool importOnly = false) : external_(false), importOnly_(importOnly), name_(name), - dev_(std::make_unique(dev)), id_(BufferMask::MaskID) + dev_(std::make_unique(dev)), id_(BufferMask::MaskID), + internalBufferCount_(internalBufferCount), + bufferSlotCount_(bufferSlotCount) { } @@ -63,7 +67,7 @@ public: void setExternalBuffer(FrameBuffer *buffer); void removeExternalBuffer(FrameBuffer *buffer); - int prepareBuffers(unsigned int count); + int prepareBuffers(); int queueBuffer(FrameBuffer *buffer); void returnBuffer(FrameBuffer *buffer); @@ -71,6 +75,8 @@ public: void releaseBuffers(); private: + static constexpr unsigned int kRPIExternalBufferSlotCount = 16; + class IdGenerator { public: @@ -133,6 +139,18 @@ private: /* All frame buffers associated with this device stream. */ BufferMap bufferMap_; + /* + * The number of buffers that should be allocated internally for this + * stream. + */ + unsigned int internalBufferCount_; + + /* + * The number of buffer slots that should be reserved for this stream. + * Only relevant for importOnly streams. + */ + unsigned int bufferSlotCount_; + /* * List of frame buffers that we can use if none have been provided by * the application for external streams. This is populated by the