[{"id":16378,"web_url":"https://patchwork.libcamera.org/comment/16378/","msgid":"","date":"2021-04-20T02:47:40","subject":"Re: [libcamera-devel] [RFC PATCH v3 2/3] libcamera: ipu3: Try\n\tqueuing pending requests if a buffer is available","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:51:00PM +0900, Hirokazu Honda wrote:\n> IPU3CameraData stores requests that have been failed due to a\n> buffer shortage. The requests should be retried once enough\n> buffers are available. This sets the retry function as signal to\n> CIO2Device and IPU3Frame, and invokes it from\n> CIO2Device::tryReturnBuffer() and IPU3Frame::remove().\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 4 +++-\n> src/libcamera/pipeline/ipu3/cio2.h | 3 +++\n> src/libcamera/pipeline/ipu3/frames.cpp | 6 ++++--\n> src/libcamera/pipeline/ipu3/frames.h | 5 +++++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++++\n> 5 files changed, 19 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 3cd777d1..6490fd46 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -272,7 +272,7 @@ FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)\n> \t/* If no buffer is provided in the request, use an internal one. */\n> \tif (!buffer) {\n> \t\tif (availableBuffers_.empty()) {\n> -\t\t\tLOG(IPU3, Error) << \"CIO2 buffer underrun\";\n> +\t\t\tLOG(IPU3, Debug) << \"CIO2 buffer underrun\";\n\nThis belongs to 1/3. Same for the two similar changes below.\n\n> \t\t\treturn nullptr;\n> \t\t}\n> \n> @@ -302,6 +302,8 @@ void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)\n> \t\t\tbreak;\n> \t\t}\n> \t}\n> +\n> +\tbufferAvailable_.emit();\n> }\n> \n> void CIO2Device::freeBuffers()\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> index 5ecc4f47..8f37ee35 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.h\n> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> @@ -54,6 +54,7 @@ public:\n> \tFrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer);\n> \tvoid tryReturnBuffer(FrameBuffer *buffer);\n> \tSignal &bufferReady() { return output_->bufferReady; }\n> +\tSignal<> &bufferAvailable() { return bufferAvailable_; }\n\nYou can declare the signal publicly directly, with\n\n\tSignal <> bufferAvailable;\n\nThe reason why the bufferReady() and frameStart() wrappers exist is to\nproxy the signals from the signal from the csi2_ (V4L2Subdevice), but\nfor bufferAvailable, we create the signal directly in this class.\n\n(On a side note, I've been thinking about making all signals private,\nwith public accessor functions, but it's because I'm used to the Qt\nsyntax ;-) So I'm not sure it would be useful, beyond the fact that it\nwould remove a violation of our coding style that makes all member\nvariable names end with _, which we don't follow for signals. If anyone\nhas any opinion, it's one of those bikeshedding discussions that we all\nlove.)\n\n> \tSignal &frameStart() { return csi2_->frameStart; }\n> \n> private:\n> @@ -65,6 +66,8 @@ private:\n> \tstd::unique_ptr csi2_;\n> \tstd::unique_ptr output_;\n> \n> +\tSignal<> bufferAvailable_;\n> +\n> \tstd::vector> buffers_;\n> \tstd::queue availableBuffers_;\n> };\n> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n> index 03e8131c..58a47f98 100644\n> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> @@ -44,12 +44,12 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n> \tunsigned int id = request->sequence();\n> \n> \tif (availableParamBuffers_.empty()) {\n> -\t\tLOG(IPU3, Error) << \"Parameters buffer underrun\";\n> +\t\tLOG(IPU3, Debug) << \"Parameters buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n> \n> \tif (availableStatBuffers_.empty()) {\n> -\t\tLOG(IPU3, Error) << \"Statistics buffer underrun\";\n> +\t\tLOG(IPU3, Debug) << \"Statistics buffer underrun\";\n> \t\treturn nullptr;\n> \t}\n> \n> @@ -86,6 +86,8 @@ void IPU3Frames::remove(IPU3Frames::Info *info)\n> \n> \t/* Delete the extended frame information. */\n> \tframeInfo_.erase(info->id);\n> +\n> +\tbufferAvailable_.emit();\n\nI think this should be emitted in IPU3Frames::tryComplete(). You call\nIPU3Frames::remove() in IPU3CameraData::queuePendingRequests() in patch\n1/3, which would then emit the signal, which would call back into\nIPU3CameraData::queuePendingRequests(), and that's a possible infinite\nloop.\n\n> }\n> \n> bool IPU3Frames::tryComplete(IPU3Frames::Info *info)\n> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n> index 4acdf48e..d2e5fbb9 100644\n> --- a/src/libcamera/pipeline/ipu3/frames.h\n> +++ b/src/libcamera/pipeline/ipu3/frames.h\n> @@ -12,6 +12,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> namespace libcamera {\n> \n> class FrameBuffer;\n> @@ -49,10 +51,13 @@ public:\n> \tInfo *find(unsigned int id);\n> \tInfo *find(FrameBuffer *buffer);\n> \n> +\tSignal<> &bufferAvailable() { return bufferAvailable_; }\n\nMissing blank line.\n\n> private:\n> \tstd::queue availableParamBuffers_;\n> \tstd::queue availableStatBuffers_;\n> \n> +\tSignal<> bufferAvailable_;\n> +\n> \tstd::map> frameInfo_;\n> };\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c73e4f7c..462a0d9b 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -699,6 +699,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)\n> \tdata->ipa_->mapBuffers(ipaBuffers_);\n> \n> \tdata->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);\n> +\tdata->frameInfos_.bufferAvailable().connect(\n> +\t\tdata, &IPU3CameraData::queuePendingRequests);\n> \n> \treturn 0;\n> }\n> @@ -1123,6 +1125,8 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t */\n> \t\tdata->cio2_.bufferReady().connect(data.get(),\n> \t\t\t\t\t&IPU3CameraData::cio2BufferReady);\n> +\t\tdata->cio2_.bufferAvailable().connect(\n> +\t\t\tdata.get(), &IPU3CameraData::queuePendingRequests);\n> \t\tdata->imgu_->input_->bufferReady.connect(&data->cio2_,\n> \t\t\t\t\t&CIO2Device::tryReturnBuffer);\n> \t\tdata->imgu_->output_->bufferReady.connect(data.get(),","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 90A95BDB14\n\tfor ;\n\tTue, 20 Apr 2021 02:47:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE91F68838;\n\tTue, 20 Apr 2021 04:47:45 +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 B01DA602CA\n\tfor ;\n\tTue, 20 Apr 2021 04:47:44 +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 13B84470;\n\tTue, 20 Apr 2021 04:47:44 +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=\"nhnDuQg9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618886864;\n\tbh=ToE2lF2974fGTTwgJ3OCXyOKPtZAyjIaC6KV2LY9hi8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nhnDuQg9Sv0IIX8cM/z7Fr220Xe9dN5nn/6S02+8mWD/vUd0q+DPOeIwkdszqnsyT\n\t4zk1LWdaQL8un3faCRbGrHQFqPHxI0e8BqilRqOQwcz0rH5/vYdKZbMZJHuVl/INl8\n\tQ6W7rmsoKcW2OHpncXpSFwyz1CcNxrjcVgsZy4CM=","Date":"Tue, 20 Apr 2021 05:47:40 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210408085101.1691729-1-hiroh@chromium.org>\n\t<20210408085101.1691729-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210408085101.1691729-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 2/3] libcamera: ipu3: Try\n\tqueuing pending requests if a buffer is available","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\" "}}]