From patchwork Mon Jul 13 08:47:27 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Naushir Patuck X-Patchwork-Id: 8758 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 55B09BD790 for ; Mon, 13 Jul 2020 08:48:09 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 1F7C4605BD; Mon, 13 Jul 2020 10:48:09 +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="pLt+a5it"; dkim-atps=neutral Received: from mail-wr1-x434.google.com (mail-wr1-x434.google.com [IPv6:2a00:1450:4864:20::434]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 8E9EB605C4 for ; Mon, 13 Jul 2020 10:48:06 +0200 (CEST) Received: by mail-wr1-x434.google.com with SMTP id b6so15028196wrs.11 for ; Mon, 13 Jul 2020 01:48:06 -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=uWihPdm75vu/VFOlLQkZP5/xs711ROqPH6Z4lRafXUc=; b=pLt+a5itpEHAGzasMGkA/eOE5GADK/ppi/0oRIFDPWWvtlTD8NQqN9p6MioyDKyuuF m1AvXANsjvGwAHCexke32q1uf9MEmLnbezDWAaMj2LqXOIv5tpIR7ZcMLJDDduKLWGNr q5G/zwYIpoxdXKuhwpURTnrR4ofcKV9K+tiglpVo6bTjz5S6MbDA96ITtvsLMzGjB9jW /U/5aHzY0hcucSn867NY24hEc25p43Ds+RIqMe7MotrcFTKWPPzzBMQLrCSQhRbGIEoo M9CgMCKDjNqvyx3Z/DX4lVJVcOYT2cPumRAcGnQzIMwXLtVi9uWhH9mbpPQxnQc+NHA3 uiHw== 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=uWihPdm75vu/VFOlLQkZP5/xs711ROqPH6Z4lRafXUc=; b=KX4nZArJAuAv73AgiAY7b9nZNbvocaPwk1hhIcV2jiUE5ymEA1WCMNBv6znaHizy7G RSzSqKCAo3Llb9+0bKSoLbW+dUOKoSZjFxxmF3qEK9TOT6NnJl8SqNG6jxs7J3qRG8aS hU3rMxrnfU1TRe/264WUpl7ottaPZtlEw8Q5adeVL7+nsVTx1Ifar0G5z5ravuRlZCuP 9OzfFgF3X6aNgqzWiHFS8kGSaauvIHyA4+hvDE7WiNMz+6+M8CN7fLMXJObLkYPNwv/2 L7yNLfHoYhs3Ve8dA6cEx3XqQLxlxgo+1ce7TDigqq01i3+IBinmYMg8E3knK4qeSlYz hNDA== X-Gm-Message-State: AOAM532+wcLE1N3G6dcZc+aXHPye32miU7JBIO4rSJKlp+aJjjupdqGp KFBTk24HXekYQa3A3JPxg21XOPFcnUE= X-Google-Smtp-Source: ABdhPJzuELv2LVcXxczijUpD4dfjnpFTa3Vb+73Lw7KMr2raHl3v0qVKkkV4Wqw+3tX/M+PTcj/JKA== X-Received: by 2002:a5d:420b:: with SMTP id n11mr78307170wrq.91.1594630085632; Mon, 13 Jul 2020 01:48:05 -0700 (PDT) Received: from naushir-VirtualBox.patuck.local ([88.97.76.4]) by smtp.gmail.com with ESMTPSA id g14sm23203357wrm.93.2020.07.13.01.48.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 13 Jul 2020 01:48:05 -0700 (PDT) From: Naushir Patuck To: libcamera-devel@lists.libcamera.org Date: Mon, 13 Jul 2020 09:47:27 +0100 Message-Id: <20200713084727.232422-9-naush@raspberrypi.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20200713084727.232422-1-naush@raspberrypi.com> References: <20200713084727.232422-1-naush@raspberrypi.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 8/9] 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 --- .../pipeline/raspberrypi/raspberrypi.cpp | 4 +- .../pipeline/raspberrypi/rpi_stream.h | 81 ++++++++++++++++--- 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index e1a74d89..2c5af05b 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -732,7 +732,9 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request) for (auto stream : data->streams_) { if (stream->isExternal()) { FrameBuffer *buffer = request->findBuffer(stream); - stream->queueBuffer(data->dropFrameCount_ ? nullptr : buffer); + int ret = stream->queueBuffer(data->dropFrameCount_ ? nullptr : buffer); + if (ret) + return ret; } } diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h index a6f573fa..3309edfb 100644 --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h @@ -139,28 +139,77 @@ public: if (availableBuffers_.empty()) { LOG(RPISTREAM, Warning) << "No buffers available for " << name_; - return -EINVAL; + /* + * Note that we need to requeue an internal buffer as soon + * as one becomes available. + */ + requeueBuffers_.push(nullptr); + return -ENOMEM; } buffer = availableBuffers_.front(); availableBuffers_.pop(); } - 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_; + if (requeueBuffers_.empty()) { + /* + * No earlier requests are pending to be queued, so we can + * go ahead and queue the buffer into the device. + */ + 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; + } else { + /* + * There are earlier buffers to be queued, so this buffer + * must go on the waiting list. + */ + requeueBuffers_.push(buffer); + return 0; } - - return ret; } void returnBuffer(FrameBuffer *buffer) { + /* Push this buffer back into the queue to be used again. */ availableBuffers_.push(buffer); + + /* + * Do we have any buffers that are waiting to be queued? + * If so, do it now as availableBuffers_ will not be empty. + */ + while (!requeueBuffers_.empty()) { + FrameBuffer *buffer = requeueBuffers_.front(); + requeueBuffers_.pop(); + + if (!buffer && !availableBuffers_.empty()) { + /* + * We want to queue an internal buffer, and at + * least one is available. + */ + buffer = availableBuffers_.front(); + availableBuffers_.pop(); + } else if (!buffer && !availableBuffers_.empty()) { + /* + * We want to queue an internal buffer, but none + * are available. + */ + break; + } + + LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie() + << " for " << name_ << " from returnBuffer"; + + int ret = dev_->queueBuffer(buffer); + if (ret) + LOG(RPISTREAM, Error) << "Failed to queue buffer for " + << name_ << " from returnBuffer"; + } } bool findFrameBuffer(FrameBuffer *buffer) const @@ -178,6 +227,7 @@ private: void clearBuffers() { availableBuffers_ = std::queue{}; + requeueBuffers_ = std::queue{}; internalBuffers_.clear(); bufferList_.clear(); } @@ -193,7 +243,7 @@ private: std::string name_; /* 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_; /* * List of frame buffer that we can use if none have been provided by @@ -201,6 +251,15 @@ private: * buffers exported internally. */ std::queue availableBuffers_; + /* + * List of frame buffer that are to be re-queued into the device. + * 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 requeueBuffers_; /* * This is a list of buffers exported internally. Need to keep this around * as the stream needs to maintain ownership of these buffers.