From patchwork Tue Sep 8 07:49:08 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Naushir Patuck X-Patchwork-Id: 9524 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 2639DBDB1C for ; Tue, 8 Sep 2020 07:49:29 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id EA84E62BBA; Tue, 8 Sep 2020 09:49:28 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="LTX/wby/"; dkim-atps=neutral Received: from mail-wm1-x336.google.com (mail-wm1-x336.google.com [IPv6:2a00:1450:4864:20::336]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 9299D62C13 for ; Tue, 8 Sep 2020 09:49:27 +0200 (CEST) Received: by mail-wm1-x336.google.com with SMTP id w2so16277950wmi.1 for ; Tue, 08 Sep 2020 00:49:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=pO9m/3WEaO+YsGGYT5406UzcJktNRpMQ9mP+ggh1N14=; b=LTX/wby/ZkHuV92Ph+JDEJ751L3hh3Ruye4NiDeRA+EctvY+JjagSz6T34S7p5VPje S4Jrl+N1PIdxvVDtWn4RftxtADofxPyJeUDhMBNrv41BTvoXPVAhKhamNL6D8DOMjbG+ FbIbAYP/BqW+vbDV0o62SRYLjAYH8VVTswhxVwG9ERgyUcsjt6NHg2KyK5kTUGjU/ULt ns5Grb9XYpECV1TVmZl7iSvrZFUwlsp/gldt1BanV8ClaDhsCHvqnP13HlF11/5hYmZu CmWP9Om3tQRfYEqGeKLARoa2iSR1YgsibfFCQMKV2baRp3Hn1dyMRNLDhErn97XHzwRF fRAA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=pO9m/3WEaO+YsGGYT5406UzcJktNRpMQ9mP+ggh1N14=; b=qW8Q3S1SQ5mVwLRxIZdZz08XPT4vnjA/Ytw1+Yi4Nbzy0pbwPGqK6H/fKW2SIlXIX3 JMaszBFfefz095yKRJWCWK0NnWvR9sd3k61CJdlPDSIc4B1L8uHsFkr8FAXR5Ct0BlFt Xw2t1TxQzR8l/cS/XMisRSycLVp+MPDn45qLWtZ2X7fxu9/KxKpnqT66EfHDmGrksxoX 7Is340KbCGgAlYih+V2zNdXHCkn+PxyDT6VnJQ2LBW6oiUjtasMWJ3+EAL093e2cOLUH w9oepyhTxYrl2PacnadPJTMMqJwOBK2/ZZed0UgfKXur+NfWVd2uqO0HbkUE4268ZQTj qgag== X-Gm-Message-State: AOAM531y9y6gonwRmG1M2Y1dDoRdgwBWi6QDdbHyo3ZSJ2Nt4WyO3qbn 1/9z6/d/fIaAsl7nVo7nEIQ06vX8S+jBFg== X-Google-Smtp-Source: ABdhPJxsPEBwWORaVVhGKhRj2gwhFQGCLftKiQNMM+RvdRrPpis7G/mDxCa/Dvd+FNgPgGElVnUlUQ== X-Received: by 2002:a7b:c5c6:: with SMTP id n6mr2940968wmk.120.1599551366892; Tue, 08 Sep 2020 00:49:26 -0700 (PDT) Received: from naushir-VirtualBox.patuck.local ([88.97.76.4]) by smtp.gmail.com with ESMTPSA id q15sm32534502wrr.8.2020.09.08.00.49.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 08 Sep 2020 00:49:26 -0700 (PDT) From: Naushir Patuck To: libcamera-devel@lists.libcamera.org Date: Tue, 8 Sep 2020 08:49:08 +0100 Message-Id: <20200908074913.109244-9-naush@raspberrypi.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20200908074913.109244-1-naush@raspberrypi.com> References: <20200908074913.109244-1-naush@raspberrypi.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v7 08/12] libcamera: pipeline: raspberrypi: Add more robust stream buffer logic 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" Add further queueing into the RPiStream object to ensure that we always follow the buffer ordering (be it internal or external) given by incoming Requests. This is essential, otherwise we risk dropping frames that are meant to be part of a Request, and can cause the pipeline to stall indefinitely. This also prevents any possibility of mismatched frame buffers going through the pipeline and out to the application. Signed-off-by: Naushir Patuck Reviewed-by: Niklas Söderlund Tested-by: David Plowman Reviewed-by: Kieran Bingham --- .../pipeline/raspberrypi/rpi_stream.cpp | 76 +++++++++++++++---- .../pipeline/raspberrypi/rpi_stream.h | 13 +++- 2 files changed, 75 insertions(+), 14 deletions(-) diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp index b4d737db..b2a5dfa7 100644 --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp @@ -94,38 +94,75 @@ int RPiStream::queueBuffer(FrameBuffer *buffer) { /* * A nullptr buffer implies an external stream, but no external - * buffer has been supplied. So, pick one from the availableBuffers_ - * queue. + * buffer has been supplied in the Request. So, pick one from the + * availableBuffers_ queue. */ if (!buffer) { if (availableBuffers_.empty()) { - LOG(RPISTREAM, Warning) << "No buffers available for " + LOG(RPISTREAM, Info) << "No buffers available for " << name_; - return -EINVAL; + /* + * Note that we need to queue an internal buffer as soon + * as one becomes available. + */ + requestBuffers_.push(nullptr); + return 0; } buffer = availableBuffers_.front(); availableBuffers_.pop(); } - LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie() - << " for " << name_; + /* + * If no earlier requests are pending to be queued we can go ahead and + * queue this buffer into the device. + */ + if (requestBuffers_.empty()) + return queueToDevice(buffer); - int ret = dev_->queueBuffer(buffer); - if (ret) { - LOG(RPISTREAM, Error) << "Failed to queue buffer for " - << name_; - } + /* + * There are earlier Request buffers to be queued, so this buffer must go + * on the waiting list. + */ + requestBuffers_.push(buffer); - return ret; + return 0; } void RPiStream::returnBuffer(FrameBuffer *buffer) { /* This can only be called for external streams. */ - assert(external_); + ASSERT(external_); + /* Push this buffer back into the queue to be used again. */ availableBuffers_.push(buffer); + + /* + * Do we have any Request buffers that are waiting to be queued? + * If so, do it now as availableBuffers_ will not be empty. + */ + while (!requestBuffers_.empty()) { + FrameBuffer *buffer = requestBuffers_.front(); + + if (!buffer) { + /* + * We want to queue an internal buffer, but none + * are available. Can't do anything, quit the loop. + */ + if (availableBuffers_.empty()) + break; + + /* + * We want to queue an internal buffer, and at least one + * is available. + */ + buffer = availableBuffers_.front(); + availableBuffers_.pop(); + } + + requestBuffers_.pop(); + queueToDevice(buffer); + } } int RPiStream::queueAllBuffers() @@ -155,10 +192,23 @@ void RPiStream::releaseBuffers() void RPiStream::clearBuffers() { availableBuffers_ = std::queue{}; + requestBuffers_ = std::queue{}; internalBuffers_.clear(); bufferList_.clear(); } +int RPiStream::queueToDevice(FrameBuffer *buffer) +{ + LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie() + << " for " << name_; + + int ret = dev_->queueBuffer(buffer); + if (ret) + LOG(RPISTREAM, Error) << "Failed to queue buffer for " + << name_; + return ret; +} + } /* namespace RPi */ } /* namespace libcamera */ diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h index 73954ec7..f42e25b9 100644 --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h @@ -57,6 +57,7 @@ public: private: void clearBuffers(); + int queueToDevice(FrameBuffer *buffer); /* * Indicates that this stream is active externally, i.e. the buffers @@ -73,7 +74,7 @@ private: /* The actual device stream. */ std::unique_ptr dev_; - /* All framebuffers associated with this device stream. */ + /* All frame buffers associated with this device stream. */ std::vector bufferList_; /* @@ -83,6 +84,16 @@ private: */ std::queue availableBuffers_; + /* + * List of frame buffers that are to be queued into the device from a Request. + * A nullptr indicates any internal buffer can be used (from availableBuffers_), + * whereas a valid pointer indicates an external buffer to be queued. + * + * Ordering buffers to be queued is important here as it must match the + * requests coming from the application. + */ + std::queue requestBuffers_; + /* * This is a list of buffers exported internally. Need to keep this around * as the stream needs to maintain ownership of these buffers.