[{"id":11369,"web_url":"https://patchwork.libcamera.org/comment/11369/","msgid":"<20200713120918.GK2866302@oden.dyn.berto.se>","date":"2020-07-13T12:09:18","subject":"Re: [libcamera-devel] [PATCH 9/9] libcamera: pipeline: raspberrypi:\n\tMove RPiStream methods to a cpp file","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Naushir,\n\nThanks for your work.\n\nOn 2020-07-13 09:47:28 +0100, Naushir Patuck wrote:\n> Some of the RPiStream methods have grown considerably in code size.\n> Move those functions into a cpp file to stop inlining.\n> \n> No other functional changes in this commit.\n\nI would do as much of this as possible in 1/9. The reason being that \nwhen one looks back at the code with 'git blame' later it will point to \nsmaller commits that touch logical blocks instead of one that moves code \nfrom A to B. This is no big thing but a good practise to have. If you \nwish to keep it as is feel free to add my tag,\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> \n> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> ---\n>  .../pipeline/raspberrypi/meson.build          |   1 +\n>  .../pipeline/raspberrypi/rpi_stream.cpp       | 167 ++++++++++++++++++\n>  .../pipeline/raspberrypi/rpi_stream.h         | 155 +---------------\n>  3 files changed, 174 insertions(+), 149 deletions(-)\n>  create mode 100644 src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> index dcfe07c5..4cdfd8e1 100644\n> --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> @@ -3,4 +3,5 @@\n>  libcamera_sources += files([\n>      'raspberrypi.cpp',\n>      'staggered_ctrl.cpp',\n> +    'rpi_stream.cpp'\n>  ])\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> new file mode 100644\n> index 00000000..4eaa0686\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> @@ -0,0 +1,167 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * rpi_stream.cpp - Raspberry Pi device stream abstraction class.\n> + */\n> +\n> +#include \"rpi_stream.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(RPISTREAM)\n> +\n> +namespace RPi {\n> +\n> +int RPiStream::prepareBuffers(unsigned int count)\n> +{\n> +\tint ret;\n> +\n> +\tif (!importOnly_) {\n> +\t\tif (count) {\n> +\t\t\t/* Export some frame buffers for internal use. */\n> +\t\t\tret = dev_->exportBuffers(count, &internalBuffers_);\n> +\t\t\tif (ret < 0)\n> +\t\t\t\treturn ret;\n> +\n> +\t\t\t/* Add these exported buffers to the internal/external buffer list. */\n> +\t\t\tstd::transform(internalBuffers_.begin(), internalBuffers_.end(),\n> +\t\t\t\t       std::back_inserter(bufferList_),\n> +\t\t\t\t       [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });\n> +\n> +\t\t\t/* Add these buffers to the queue of internal usable buffers. */\n> +\t\t\tfor (auto const &buffer : internalBuffers_)\n> +\t\t\t\tavailableBuffers_.push(buffer.get());\n> +\t\t}\n> +\n> +\t\t/* We must import all internal/external exported buffers. */\n> +\t\tcount = bufferList_.size();\n> +\t}\n> +\n> +\treturn dev_->importBuffers(count);\n> +}\n> +\n> +int RPiStream::queueAllBuffers()\n> +{\n> +\tint ret;\n> +\n> +\tif (external_)\n> +\t\treturn 0;\n> +\n> +\twhile (!availableBuffers_.empty()) {\n> +\t\tret = queueBuffer(availableBuffers_.front());\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\n> +\t\tavailableBuffers_.pop();\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int RPiStream::queueBuffer(FrameBuffer *buffer)\n> +{\n> +\t/*\n> +\t * A nullptr buffer implies an external stream, but no external\n> +\t * buffer has been supplied. So, pick one from the availableBuffers_\n> +\t * queue.\n> +\t */\n> +\tif (!buffer) {\n> +\t\tif (availableBuffers_.empty()) {\n> +\t\t\tLOG(RPISTREAM, Warning) << \"No buffers available for \"\n> +\t\t\t\t\t\t<< name_;\n> +\t\t\t/*\n> +\t\t\t * Note that we need to requeue an internal buffer as soon\n> +\t\t\t * as one becomes available.\n> +\t\t\t */\n> +\t\t\trequeueBuffers_.push(nullptr);\n> +\t\t\treturn -ENOMEM;\n> +\t\t}\n> +\n> +\t\tbuffer = availableBuffers_.front();\n> +\t\tavailableBuffers_.pop();\n> +\t}\n> +\n> +\tif (requeueBuffers_.empty()) {\n> +\t\t/*\n> +\t\t * No earlier requests are pending to be queued, so we can\n> +\t\t * go ahead and queue the buffer into the device.\n> +\t\t */\n> +\t\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> +\t\t\t\t      << \" for \" << name_;\n> +\n> +\t\tint ret = dev_->queueBuffer(buffer);\n> +\t\tif (ret)\n> +\t\t\tLOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> +\t\t\t\t\t      << name_;\n> +\t\treturn ret;\n> +\t} else {\n> +\t\t/*\n> +\t\t * There are earlier buffers to be queued, so this buffer must\n> +\t\t * go on the waiting list.\n> +\t\t */\n> +\t\trequeueBuffers_.push(buffer);\n> +\t\treturn 0;\n> +\t}\n> +}\n> +\n> +void RPiStream::returnBuffer(FrameBuffer *buffer)\n> +{\n> +\t/* Push this buffer back into the queue to be used again. */\n> +\tavailableBuffers_.push(buffer);\n> +\n> +\t/*\n> +\t * Do we have any buffers that are waiting to be queued?\n> +\t * If so, do it now as availableBuffers_ will not be empty.\n> +\t */\n> +\twhile (!requeueBuffers_.empty()) {\n> +\t\tFrameBuffer *buffer = requeueBuffers_.front();\n> +\t\trequeueBuffers_.pop();\n> +\n> +\t\tif (!buffer && !availableBuffers_.empty()) {\n> +\t\t\t/*\n> +\t\t\t * We want to queue an internal buffer, and at\n> +\t\t\t * least one is available.\n> +\t\t\t */\n> +\t\t\tbuffer = availableBuffers_.front();\n> +\t\t\tavailableBuffers_.pop();\n> +\t\t} else if (!buffer && !availableBuffers_.empty()) {\n> +\t\t\t/*\n> +\t\t\t * We want to queue an internal buffer, but none\n> +\t\t\t * are available.\n> +\t\t\t */\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> +\t\t\t\t      << \" for \" << name_ << \" from returnBuffer\";\n> +\n> +\t\tint ret = dev_->queueBuffer(buffer);\n> +\t\tif (ret)\n> +\t\t\tLOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> +\t\t\t\t\t      << name_ << \" from returnBuffer\";\n> +\t}\n> +}\n> +\n> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const\n> +{\n> +\tif (importOnly_)\n> +\t\treturn false;\n> +\n> +\tif (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())\n> +\t\treturn true;\n> +\n> +\treturn false;\n> +}\n> +\n> +void RPiStream::clearBuffers()\n> +{\n> +\tavailableBuffers_ = std::queue<FrameBuffer *>{};\n> +\trequeueBuffers_ = std::queue<FrameBuffer *>{};\n> +\tinternalBuffers_.clear();\n> +\tbufferList_.clear();\n> +}\n> +\n> +} /* namespace RPi */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index 3309edfb..dbbff70b 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> @@ -16,8 +16,6 @@\n>  \n>  namespace libcamera {\n>  \n> -LOG_DEFINE_CATEGORY(RPISTREAM)\n> -\n>  namespace RPi {\n>  \n>  /*\n> @@ -82,155 +80,14 @@ public:\n>  \t\tclearBuffers();\n>  \t}\n>  \n> -\tint prepareBuffers(unsigned int count)\n> -\t{\n> -\t\tint ret;\n> -\n> -\t\tif (!importOnly_) {\n> -\t\t\tif (count) {\n> -\t\t\t\t/* Export some frame buffers for internal use. */\n> -\t\t\t\tret = dev_->exportBuffers(count, &internalBuffers_);\n> -\t\t\t\tif (ret < 0)\n> -\t\t\t\t\treturn ret;\n> -\n> -\t\t\t\t/* Add these exported buffers to the internal/external buffer list. */\n> -\t\t\t\tstd::transform(internalBuffers_.begin(), internalBuffers_.end(),\n> -\t\t\t\t\t       std::back_inserter(bufferList_),\n> -\t\t\t\t\t       [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });\n> -\n> -\t\t\t\t/* Add these buffers to the queue of internal usable buffers. */\n> -\t\t\t\tfor (auto const &buffer : internalBuffers_)\n> -\t\t\t\t\tavailableBuffers_.push(buffer.get());\n> -\t\t\t}\n> -\n> -\t\t\t/* We must import all internal/external exported buffers. */\n> -\t\t\tcount = bufferList_.size();\n> -\t\t}\n> -\n> -\t\treturn dev_->importBuffers(count);\n> -\t}\n> -\n> -\tint queueAllBuffers()\n> -\t{\n> -\t\tint ret;\n> -\n> -\t\tif (external_)\n> -\t\t\treturn 0;\n> -\n> -\t\twhile (!availableBuffers_.empty()) {\n> -\t\t\tret = queueBuffer(availableBuffers_.front());\n> -\t\t\tif (ret < 0)\n> -\t\t\t\treturn ret;\n> -\n> -\t\t\tavailableBuffers_.pop();\n> -\t\t}\n> -\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tint queueBuffer(FrameBuffer *buffer)\n> -\t{\n> -\t\t/*\n> -\t\t * A nullptr buffer implies an external stream, but no external\n> -\t\t * buffer has been supplied. So, pick one from the availableBuffers_\n> -\t\t * queue.\n> -\t\t */\n> -\t\tif (!buffer) {\n> -\t\t\tif (availableBuffers_.empty()) {\n> -\t\t\t\tLOG(RPISTREAM, Warning) << \"No buffers available for \"\n> -\t\t\t\t\t\t\t<< name_;\n> -\t\t\t\t/*\n> -\t\t\t\t * Note that we need to requeue an internal buffer as soon\n> -\t\t\t\t * as one becomes available.\n> -\t\t\t\t */\n> -\t\t\t\trequeueBuffers_.push(nullptr);\n> -\t\t\t\treturn -ENOMEM;\n> -\t\t\t}\n> -\n> -\t\t\tbuffer = availableBuffers_.front();\n> -\t\t\tavailableBuffers_.pop();\n> -\t\t}\n> -\n> -\t\tif (requeueBuffers_.empty()) {\n> -\t\t\t/*\n> -\t\t\t* No earlier requests are pending to be queued, so we can\n> -\t\t\t* go ahead and queue the buffer into the device.\n> -\t\t\t*/\n> -\t\t\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> -\t\t\t\t\t      << \" for \" << name_;\n> -\n> -\t\t\tint ret = dev_->queueBuffer(buffer);\n> -\t\t\tif (ret)\n> -\t\t\t\tLOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> -\t\t\t\t\t\t      << name_;\n> -\t\t\treturn ret;\n> -\t\t} else {\n> -\t\t\t/*\n> -\t\t\t * There are earlier buffers to be queued, so this buffer\n> -\t\t\t * must go on the waiting list.\n> -\t\t\t */\n> -\t\t\trequeueBuffers_.push(buffer);\n> -\t\t\treturn 0;\n> -\t\t}\n> -\t}\n> -\n> -\tvoid returnBuffer(FrameBuffer *buffer)\n> -\t{\n> -\t\t/* Push this buffer back into the queue to be used again. */\n> -\t\tavailableBuffers_.push(buffer);\n> -\n> -\t\t/*\n> -\t\t * Do we have any buffers that are waiting to be queued?\n> -\t\t * If so, do it now as availableBuffers_ will not be empty.\n> -\t\t */\n> -\t\twhile (!requeueBuffers_.empty()) {\n> -\t\t\tFrameBuffer *buffer = requeueBuffers_.front();\n> -\t\t\trequeueBuffers_.pop();\n> -\n> -\t\t\tif (!buffer && !availableBuffers_.empty()) {\n> -\t\t\t\t/*\n> -\t\t\t\t * We want to queue an internal buffer, and at\n> -\t\t\t\t * least one is available.\n> -\t\t\t\t */\n> -\t\t\t\tbuffer = availableBuffers_.front();\n> -\t\t\t\tavailableBuffers_.pop();\n> -\t\t\t} else if (!buffer && !availableBuffers_.empty()) {\n> -\t\t\t\t/*\n> -\t\t\t\t * We want to queue an internal buffer, but none\n> -\t\t\t\t * are available.\n> -\t\t\t\t */\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> -\n> -\t\t\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << buffer->cookie()\n> -\t\t\t\t\t      << \" for \" << name_ << \" from returnBuffer\";\n> -\n> -\t\t\tint ret = dev_->queueBuffer(buffer);\n> -\t\t\tif (ret)\n> -\t\t\t\tLOG(RPISTREAM, Error) << \"Failed to queue buffer for \"\n> -\t\t\t\t\t\t      << name_ << \" from returnBuffer\";\n> -\t\t}\n> -\t}\n> -\n> -\tbool findFrameBuffer(FrameBuffer *buffer) const\n> -\t{\n> -\t\tif (importOnly_)\n> -\t\t\treturn false;\n> -\n> -\t\tif (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())\n> -\t\t\treturn true;\n> -\n> -\t\treturn false;\n> -\t}\n> +\tint prepareBuffers(unsigned int count);\n> +\tint queueAllBuffers();\n> +\tint queueBuffer(FrameBuffer *buffer);\n> +\tvoid returnBuffer(FrameBuffer *buffer);\n> +\tbool findFrameBuffer(FrameBuffer *buffer) const;\n>  \n>  private:\n> -\tvoid clearBuffers()\n> -\t{\n> -\t\tavailableBuffers_ = std::queue<FrameBuffer *>{};\n> -\t\trequeueBuffers_ = std::queue<FrameBuffer *>{};\n> -\t\tinternalBuffers_.clear();\n> -\t\tbufferList_.clear();\n> -\t}\n> +\tvoid clearBuffers();\n>  \n>  \t/*\n>  \t * Indicates that this stream is active externally, i.e. the buffers\n> -- \n> 2.25.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E8897BD790\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 13 Jul 2020 12:09:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 49AC360721;\n\tMon, 13 Jul 2020 14:09:21 +0200 (CEST)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 559C96048F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 13 Jul 2020 14:09:20 +0200 (CEST)","by mail-lj1-x233.google.com with SMTP id q4so17456568lji.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 13 Jul 2020 05:09:20 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ts4sm4595077lfc.71.2020.07.13.05.09.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 13 Jul 2020 05:09:18 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"KHJj2LZD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=T7BmsiR45pattTD6rUiYSTOEB80IM6kAiRuBAnrq25o=;\n\tb=KHJj2LZDiunuecPGex/dvk5Q7GNdBuPfkzjoT64hUJtW/1ICNlIRzJrD/VYrv+4o7W\n\tdcxUo0nf89QwwG5NJmGHluPHMD0qBPvNCL8CcOx1GbBeDFOpnztJxpy4+Bt5CQ7w1nN8\n\tE1Oftmami6M7f8YRWWvCKZH9/rWdW91MQjTcGSDMy5mpMSs7gB5k4JO7m9oTP89vZz4Q\n\toP+95qytYiSOugB+dQabxB2L4MZZGAyCMY0iZkqyDAdOsJZ7LWdqQz/9sv3zRRFLdx18\n\tBaDI6jBdg/ZCrJRsE4K/OPx0VQ7jE3GZ/DLKkigje/zGZP2mYMY38ksEM3bqjC1q1bee\n\tSSzQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=T7BmsiR45pattTD6rUiYSTOEB80IM6kAiRuBAnrq25o=;\n\tb=miSs9DzSYn9e3rn1WA0nYqsnVt7x4FvrTelu9hXT1oHDEIlIWdWTJuiI6mqCOdEvcE\n\tPV2EenWn0HWETeL40yKOGFfn/uabrAcQeRuNLwOUDomjhaRoDNd6nAWQ/6QId/JkP4Dk\n\tPb+JNcYOb5HScpaIg22BoCN4TOBsT35STqVpr7mWOZJtYNiKywbvVdO4qZdBFJm1WeTg\n\t9HN4xIQsLmNnvbWyM46XQJKrI7JN40tfvF4u0wneM/3ooEI4zaofkDV/0sYwo1lhixQD\n\tUghG3Xq4SzW28mw6Uwmx2A4wQ5Fs6hqcqp6ENEqMEx12gBduZA5rP2P+B4JmVCYvHopv\n\t+cmw==","X-Gm-Message-State":"AOAM530SPNgYLqdr9mGvD84oO5jx2WqQfitsfzGEdRF6Mpq583F8ghKB\n\t8PIAnPWSWtxILBcsyTU0h/MJqQ==","X-Google-Smtp-Source":"ABdhPJyE/QrxuMLQF7CgDKg9gBuWG4Q6FXXVoomL//VYqGVQhnMzvCTmXvLv2qJ9ZtOHUa4skWEPbQ==","X-Received":"by 2002:a2e:3a14:: with SMTP id\n\th20mr37217265lja.331.1594642159325; \n\tMon, 13 Jul 2020 05:09:19 -0700 (PDT)","Date":"Mon, 13 Jul 2020 14:09:18 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<20200713120918.GK2866302@oden.dyn.berto.se>","References":"<20200713084727.232422-1-naush@raspberrypi.com>\n\t<20200713084727.232422-10-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200713084727.232422-10-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 9/9] libcamera: pipeline: raspberrypi:\n\tMove RPiStream methods to a cpp file","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]