[{"id":16377,"web_url":"https://patchwork.libcamera.org/comment/16377/","msgid":"","date":"2021-04-20T01:37:53","subject":"Re: [libcamera-devel] [RFC PATCH v3 1/3] pipeline: ipu3: Store\n\trequests in the case a buffer shortage","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Thu, Apr 08, 2021 at 05:50:59PM +0900, Hirokazu Honda wrote:\n> PipelineHandlerIPU3 returns -ENOBUFS and -ENOMEM on queueing a\n> request when there are not sufficient buffers for the request.\n> Since the request will be successful if it is queued later when\n> enough buffers are available. The requests failed due to a buffer\n> shortage should be stored and retried later in the FIFO order.\n> This introduces the queue in IPU3CameraData to do that.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 58 ++++++++++++++++++----------\n> 1 file changed, 37 insertions(+), 21 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 519cad4f..c73e4f7c 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -66,6 +66,7 @@ public:\n> \tvoid cio2BufferReady(FrameBuffer *buffer);\n> \tvoid paramBufferReady(FrameBuffer *buffer);\n> \tvoid statBufferReady(FrameBuffer *buffer);\n> +\tint queuePendingRequests();\n> \n> \tCIO2Device cio2_;\n> \tImgUDevice *imgu_;\n> @@ -84,6 +85,7 @@ public:\n> \n> \tstd::unique_ptr ipa_;\n> \n> +\tstd::queue pendingRequests_;\n\nA blank line would be nice here.\n\n> private:\n> \tvoid queueFrameAction(unsigned int id,\n> \t\t\t const ipa::ipu3::IPU3Action &action);\n> @@ -764,6 +766,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tIPU3CameraData *data = cameraData(camera);\n> \tint ret = 0;\n> \n> +\tdata->pendingRequests_ = {};\n\nThis leaks any pending request, as it will never be completed. Patch 3/3\nin this series fixes the issue, but there's a bisection breakage, which\nwould be nice to avoid. One option would be to squash the three patches\ntogether.\n\n> +\n> \tdata->ipa_->stop();\n> \n> \tret |= data->imgu_->stop();\n> @@ -774,36 +778,48 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tfreeBuffers(camera);\n> }\n> \n> -int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> +int IPU3CameraData::queuePendingRequests()\n> {\n> -\tIPU3CameraData *data = cameraData(camera);\n> +\twhile (!pendingRequests_.empty()) {\n> +\t\tRequest *request = pendingRequests_.front();\n> \n> -\tIPU3Frames::Info *info = data->frameInfos_.create(request);\n> -\tif (!info)\n> -\t\treturn -ENOBUFS;\n> +\t\tIPU3Frames::Info *info = frameInfos_.create(request);\n> +\t\tif (!info)\n> +\t\t\tbreak;\n> \n> -\t/*\n> -\t * Queue a buffer on the CIO2, using the raw stream buffer provided in\n> -\t * the request, if any, or a CIO2 internal buffer otherwise.\n> -\t */\n> -\tFrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);\n> -\tFrameBuffer *rawBuffer = data->cio2_.queueBuffer(request, reqRawBuffer);\n> -\tif (!rawBuffer) {\n> -\t\tdata->frameInfos_.remove(info);\n> -\t\treturn -ENOMEM;\n> -\t}\n> +\t\t/*\n> +\t\t * Queue a buffer on the CIO2, using the raw stream buffer\n> +\t\t * provided in the request, if any, or a CIO2 internal buffer\n> +\t\t * otherwise.\n> +\t\t */\n> +\t\tFrameBuffer *reqRawBuffer = request->findBuffer(&rawStream_);\n> +\t\tFrameBuffer *rawBuffer = cio2_.queueBuffer(request, reqRawBuffer);\n> +\t\tif (!rawBuffer) {\n> +\t\t\tframeInfos_.remove(info);\n> +\t\t\tbreak;\n> +\t\t}\n> \n> -\tinfo->rawBuffer = rawBuffer;\n> +\t\tinfo->rawBuffer = rawBuffer;\n> \n> -\tipa::ipu3::IPU3Event ev;\n> -\tev.op = ipa::ipu3::EventProcessControls;\n> -\tev.frame = info->id;\n> -\tev.controls = request->controls();\n> -\tdata->ipa_->processEvent(ev);\n> +\t\tipa::ipu3::IPU3Event ev;\n> +\t\tev.op = ipa::ipu3::EventProcessControls;\n> +\t\tev.frame = info->id;\n> +\t\tev.controls = request->controls();\n> +\t\tipa_->processEvent(ev);\n\nI was going to say that the event should likely be sent to the IPA right\naway, to make sure that the IPA get's enough data aobut future frames to\nmake the best decision, but it will likely not need to peek ahead in the\nqueue for more requests than we have internal buffers. It should thus be\nfine, and we can address this later if it becomes a problem.\n\n> +\n> +\t\tpendingRequests_.pop();\n> +\t}\n> \n> \treturn 0;\n\nThis function now always returns 0, let's make it a void function.\n\n> }\n> \n> +int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)\n> +{\n> +\tIPU3CameraData *data = cameraData(camera);\n> +\tdata->pendingRequests_.emplace(request);\n\nIt's a pointer, you can call\n\n\tdata->pendingRequests_.push(request);\n\n> +\treturn data->queuePendingRequests();\n> +}\n> +\n> bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> {\n> \tint ret;","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 91E4EBD816\n\tfor ;\n\tTue, 20 Apr 2021 01:37:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A2F36883F;\n\tTue, 20 Apr 2021 03:37:59 +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 B96B8602CA\n\tfor ;\n\tTue, 20 Apr 2021 03:37:57 +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 293D9470;\n\tTue, 20 Apr 2021 03:37:57 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AW4Ryhuh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618882677;\n\tbh=9CrXSK0IFOPSPm3ykWOV2ZLmg7VgvIcvfsuGugOHW5o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AW4RyhuhrjwbA8r9yvD2Ap34sADaezi8R1rP2/jGurSP/NoMtbaQ65rv2T+42xwEa\n\t+uiFGz7/4gXrNRa5WIiFmy2aRRwT+6j2lZCuQXH5w5ORVxMRhe944bodoMUGqo4egI\n\tfXlaOexlPwevA0EJfeBhL2QS95bM7+mrSN0r69vk=","Date":"Tue, 20 Apr 2021 04:37:53 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210408085101.1691729-1-hiroh@chromium.org>\n\t<20210408085101.1691729-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210408085101.1691729-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 1/3] pipeline: ipu3: Store\n\trequests in the case a buffer shortage","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]