[{"id":18458,"web_url":"https://patchwork.libcamera.org/comment/18458/","msgid":"","date":"2021-08-01T22:09:10","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: pipeline: vimc: Add\n\tinternal request queue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nícolas,\n\nThank you for the patch.\n\nOn Mon, Jul 19, 2021 at 04:14:36PM -0300, Nícolas F. R. A. Prado wrote:\n> Add an internal queue that stores requests until there are V4L2 buffer\n> slots available. This avoids the need to cancel requests when there is a\n> shortage of said buffers.\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> src/libcamera/pipeline/vimc/vimc.cpp | 65 +++++++++++++++++++++++-----\n> 1 file changed, 55 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index a698427c4361..c9092bec9a74 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -9,6 +9,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -53,6 +54,9 @@ public:\n> \tint init();\n> \tvoid bufferReady(FrameBuffer *buffer);\n> \n> +\tvoid queuePendingRequests();\n> +\tvoid cancelPendingRequests();\n> +\n> \tMediaDevice *media_;\n> \tstd::unique_ptr sensor_;\n> \tstd::unique_ptr debayer_;\n> @@ -62,6 +66,8 @@ public:\n> \tStream stream_;\n> \n> \tstd::unique_ptr ipa_;\n> +\n> +\tstd::queue pendingRequests_;\n> };\n> \n> class VimcCameraConfiguration : public CameraConfiguration\n> @@ -94,9 +100,9 @@ public:\n> \n> \tbool match(DeviceEnumerator *enumerator) override;\n> \n> -private:\n> \tint processControls(VimcCameraData *data, Request *request);\n\nInstead of making this function public, shouldn't it be moved to the\nVimcCameraData class ?\n\n> \n> +private:\n> \tVimcCameraData *cameraData(const Camera *camera)\n> \t{\n> \t\treturn static_cast(\n> @@ -335,6 +341,7 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis\n> void PipelineHandlerVimc::stop(Camera *camera)\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> +\tdata->cancelPendingRequests();\n> \tdata->video_->streamOff();\n> \tdata->ipa_->stop();\n> \tdata->video_->releaseBuffers();\n> @@ -383,21 +390,16 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> int PipelineHandlerVimc::queueRequestDevice(Camera *camera, Request *request)\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> -\tFrameBuffer *buffer = request->findBuffer(&data->stream_);\n> -\tif (!buffer) {\n> +\n> +\tif (!request->findBuffer(&data->stream_)) {\n> \t\tLOG(VIMC, Error)\n> \t\t\t<< \"Attempt to queue request with invalid stream\";\n> \n> \t\treturn -ENOENT;\n> \t}\n> \n> -\tint ret = processControls(data, request);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> -\n> -\tret = data->video_->queueBuffer(buffer);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> +\tdata->pendingRequests_.push(request);\n> +\tdata->queuePendingRequests();\n> \n> \treturn 0;\n> }\n> @@ -531,6 +533,49 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> \n> \tpipe_->completeBuffer(request, buffer);\n> \tpipe_->completeRequest(request);\n> +\n> +\tqueuePendingRequests();\n> +}\n> +\n> +void VimcCameraData::queuePendingRequests()\n> +{\n> +\tPipelineHandlerVimc *pipe = static_cast(pipe_);\n> +\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tRequest *request = pendingRequests_.front();\n> +\t\tFrameBuffer *buffer = request->findBuffer(&stream_);\n> +\n> +\t\tint ret = pipe->processControls(this, request);\n> +\t\tif (ret < 0) {\n> +\t\t\tbuffer->cancel();\n> +\t\t\tpipe_->completeBuffer(request, buffer);\n> +\t\t\tpipe_->completeRequest(request);\n> +\t\t\tpendingRequests_.pop();\n> +\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/* If we're missing V4L2 buffer slots, try again later */\n\ns/later/later./\n\n> +\t\tret = video_->queueBuffer(buffer);\n> +\t\tif (ret < 0)\n> +\t\t\tbreak;\n\nThis is problematic, because the controls will be applied already, and\nthey shouldn't. One possible way to fix this is to maintain a counter of\nthe available V4L2 buffer slots, to stop the loop right away without\nhaving to rely on queueBuffer().\n\nAnother option is to consider that we don't implement per-frame control\nyet anyway, as the controls are applied when the buffer is queued to the\ndriver, while we should delay them until the corresponding buffer gets\nfilled by the device, and ignore this issue for now given that per-frame\ncontrol will involve quite a bit of refactoring anyway.\n\nSame comments for 2/3.\n\n> +\n> +\t\tpendingRequests_.pop();\n> +\t}\n> +}\n> +\n> +void VimcCameraData::cancelPendingRequests()\n> +{\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tRequest *request = pendingRequests_.front();\n> +\t\tFrameBuffer *buffer = request->findBuffer(&stream_);\n> +\n> +\t\tbuffer->cancel();\n> +\t\tpipe_->completeBuffer(request, buffer);\n> +\t\tpipe_->completeRequest(request);\n> +\n> +\t\tpendingRequests_.pop();\n> +\t}\n> }\n> \n> REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)","headers":{"Return-Path":"","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 BFDFDBD878\n\tfor ;\n\tSun, 1 Aug 2021 22:09:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2FF78687BD;\n\tMon, 2 Aug 2021 00:09:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E004F687B6\n\tfor ;\n\tMon, 2 Aug 2021 00:09:19 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5E0C187C;\n\tMon, 2 Aug 2021 00:09:19 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"B18+4J//\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627855759;\n\tbh=R59Uq/2rNKwaG5L5aCoGXcm+vv97ZPa/jiZVLOdT2LU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=B18+4J//NWPrl3PmHhloN32eJ6sX7Nqs+AHuYR3areL2O/vHxkBOCEGGrlZa0cFsq\n\tW4Lh3/sVt3xe/kPFrc9VzttAlbFpXp1ohvgYrB8OnAlJiUyR95CtOXBZXEFe670F3P\n\ttmjZj+gBZrFgz4IejwZ/LO6m3WbzB/oPeAzlFgTo=","Date":"Mon, 2 Aug 2021 01:09:10 +0300","From":"Laurent Pinchart ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"","References":"<20210719191438.189046-1-nfraprado@collabora.com>\n\t<20210719191438.189046-2-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210719191438.189046-2-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: pipeline: vimc: Add\n\tinternal request queue","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]