[{"id":18459,"web_url":"https://patchwork.libcamera.org/comment/18459/","msgid":"","date":"2021-08-01T22:17:39","subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: pipeline: rkisp1: 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:38PM -0300, Nícolas F. R. A. Prado wrote:\n> Add an internal queue that stores requests until there are internal\n> buffers and V4L2 buffer slots available. This avoids the need to cancel\n> requests when there is a shortage of said buffers.\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 73 +++++++++++++++++++-----\n> 1 file changed, 59 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index f4ea2fd4d4d0..f1c75b7d37c5 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -89,6 +89,9 @@ public:\n> \n> \tint loadIPA(unsigned int hwRevision);\n> \n> +\tvoid queuePendingRequests();\n> +\tvoid cancelPendingRequests();\n> +\n> \tStream mainPathStream_;\n> \tStream selfPathStream_;\n> \tstd::unique_ptr sensor_;\n> @@ -102,6 +105,8 @@ public:\n> \n> \tstd::unique_ptr ipa_;\n> \n> +\tstd::queue pendingRequests_;\n> +\n> private:\n> \tvoid queueFrameAction(unsigned int frame,\n> \t\t\t const ipa::rkisp1::RkISP1Action &action);\n> @@ -199,13 +204,13 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> \tunsigned int frame = data->frame_;\n> \n> \tif (pipe_->availableParamBuffers_.empty()) {\n> -\t\tLOG(RkISP1, Error) << \"Parameters buffer underrun\";\n> +\t\tLOG(RkISP1, Debug) << \"Parameters buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n> \tFrameBuffer *paramBuffer = pipe_->availableParamBuffers_.front();\n> \n> \tif (pipe_->availableStatBuffers_.empty()) {\n> -\t\tLOG(RkISP1, Error) << \"Statisitc buffer underrun\";\n> +\t\tLOG(RkISP1, Debug) << \"Statistic buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n> \tFrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();\n> @@ -373,6 +378,52 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta\n> \tpipe->tryCompleteRequest(info->request);\n> }\n> \n> +void RkISP1CameraData::queuePendingRequests()\n> +{\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tRequest *request = pendingRequests_.front();\n> +\n> +\t\t/*\n> +\t\t * If there aren't internal buffers available, we break and try\n> +\t\t * again later. If there are, we're guaranteed to also have V4L2\n> +\t\t * buffer slots available to queue the request, since we should\n> +\t\t * always have more (or equal) buffer slots than internal\n> +\t\t * buffers.\n> +\t\t */\n> +\t\tRkISP1FrameInfo *info = frameInfo_.create(this, request);\n> +\t\tif (!info)\n> +\t\t\tbreak;\n> +\n> +\t\tipa::rkisp1::RkISP1Event ev;\n> +\t\tev.op = ipa::rkisp1::EventQueueRequest;\n> +\t\tev.frame = frame_;\n> +\t\tev.bufferId = info->paramBuffer->cookie();\n> +\t\tev.controls = request->controls();\n> +\t\tipa_->processEvent(ev);\n> +\n> +\t\tframe_++;\n> +\n> +\t\tpendingRequests_.pop();\n> +\t}\n> +}\n> +\n> +void RkISP1CameraData::cancelPendingRequests()\n> +{\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tRequest *request = pendingRequests_.front();\n> +\n> +\t\tfor (auto it : request->buffers()) {\n> +\t\t\tFrameBuffer *buffer = it.second;\n> +\t\t\tbuffer->cancel();\n> +\t\t\tpipe_->completeBuffer(request, buffer);\n> +\t\t}\n> +\n> +\t\tpipe_->completeRequest(request);\n> +\n> +\t\tpendingRequests_.pop();\n> +\t}\n> +}\n> +\n> RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> \t\t\t\t\t\t RkISP1CameraData *data)\n> \t: CameraConfiguration()\n> @@ -827,6 +878,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n> \n> \tisp_->setFrameStartEnabled(false);\n> \n> +\tdata->cancelPendingRequests();\n> +\n\nI was going to say that this should be done after stopping the video\ndevices, otherwise you'll complete the pending requests out of order,\nbefore the requests that have been queued already, but the pipeline\nhandler core will reorder the completions, so it should be fine. Still,\nif they can be cancelled after stopping the video devices, it would be a\nmore logical order, so I'd prefer that. Same comment for the other\npatches in this series.\n\nReviewed-by: Laurent Pinchart \n\n> \tdata->ipa_->stop();\n> \n> \tselfPath_.stop();\n> @@ -854,18 +907,8 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> {\n> \tRkISP1CameraData *data = cameraData(camera);\n> \n> -\tRkISP1FrameInfo *info = data->frameInfo_.create(data, request);\n> -\tif (!info)\n> -\t\treturn -ENOENT;\n> -\n> -\tipa::rkisp1::RkISP1Event ev;\n> -\tev.op = ipa::rkisp1::EventQueueRequest;\n> -\tev.frame = data->frame_;\n> -\tev.bufferId = info->paramBuffer->cookie();\n> -\tev.controls = request->controls();\n> -\tdata->ipa_->processEvent(ev);\n> -\n> -\tdata->frame_++;\n> +\tdata->pendingRequests_.push(request);\n> +\tdata->queuePendingRequests();\n> \n> \treturn 0;\n> }\n> @@ -1062,6 +1105,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)\n> \tdata->frameInfo_.destroy(info->frame);\n> \n> \tcompleteRequest(request);\n> +\n> +\tdata->queuePendingRequests();\n> }\n> \n> void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)","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 B337FC3232\n\tfor ;\n\tSun, 1 Aug 2021 22:17:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28D61687BF;\n\tMon, 2 Aug 2021 00:17:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CBCEA687BD\n\tfor ;\n\tMon, 2 Aug 2021 00:17:48 +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 53C7F87C;\n\tMon, 2 Aug 2021 00:17:48 +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=\"cP/ZNBMp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627856268;\n\tbh=1kfzvv/1o0McmExDKhR3Tqo7Tor7zVKOsVoCkcAyGkk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cP/ZNBMpqm/hT14+gE8d0vPaJWeDD5gLsT+ijCwB7F3n3os8TT4QmPNdzo1dr/yqn\n\tsux5Uw5JYSmYHVP/bvJ0cgXiBXxZCFFpV9PYXkPkTAvrvirWDJhjQiX/qQLa7oZYwM\n\t7uvrR6I3XPLUgeyO9wZHRO0WP0mf5C3YTdCOJuhY=","Date":"Mon, 2 Aug 2021 01:17:39 +0300","From":"Laurent Pinchart ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"","References":"<20210719191438.189046-1-nfraprado@collabora.com>\n\t<20210719191438.189046-4-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-4-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: pipeline: rkisp1: 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\" "}}]