[{"id":16379,"web_url":"https://patchwork.libcamera.org/comment/16379/","msgid":"","date":"2021-04-20T02:51:50","subject":"Re: [libcamera-devel] [RFC PATCH v3 3/3] libcamera: ipu3: Cancel\n\tpending requests correctly","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:01PM +0900, Hirokazu Honda wrote:\n> IPU3CameraData stores requests that are not queued yet to a\n> camera node. They should be reported as cancel upon\n> PipelineHandlerIPU3::stop().\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/buffer.h | 3 +++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++++++++++++-\n> 2 files changed, 21 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 620f8a66..72c62144 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -58,6 +58,9 @@ private:\n> \n> \tfriend class V4L2VideoDevice; /* Needed to update metadata_. */\n> \n> +\t/*! HACK! */\n> +\tfriend class IPU3CameraData; /* Needed to update metadata_. */\n\nLooks like we need a way to handle this cleanly :-) It may relate to the\ndiscussion we had earlier, about the rework of the buffer and requests\nstate handling. I think a redesign of that mechanism, with a cleaner API\nfor completion, would be nice. Making the Request class inherit from\nExtensible, with Request::Private being defined in\ninclude/libcamera/internal/request.h and offering public functions for\nthe pipeline handlers may be part of the solution. The \"Private\" name in\nthe our Extensible design pattern (which implements the p-impl design\npattern) means private to applications, in the public API, but it can be\naccessed within libcamera.\n\nThe rest of the patch looks good.\n\n> +\n> \tstd::vector planes_;\n> \n> \tRequest *request_;\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 462a0d9b..f89b8f3f 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> +\tvoid cancelPendingRequests();\n> \tint queuePendingRequests();\n> \n> \tCIO2Device cio2_;\n> @@ -768,7 +769,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tIPU3CameraData *data = cameraData(camera);\n> \tint ret = 0;\n> \n> -\tdata->pendingRequests_ = {};\n> +\tdata->cancelPendingRequests();\n> \n> \tdata->ipa_->stop();\n> \n> @@ -780,6 +781,22 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> \tfreeBuffers(camera);\n> }\n> \n> +void IPU3CameraData::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->metadata_.status = FrameMetadata::FrameCancelled;\n> +\t\t\tpipe_->completeBuffer(request, buffer);\n> +\t\t}\n> +\n> +\t\tpipe_->completeRequest(request);\n> +\t\tpendingRequests_.pop();\n> +\t}\n> +}\n> +\n> int IPU3CameraData::queuePendingRequests()\n> {\n> \twhile (!pendingRequests_.empty()) {","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 60F54BD816\n\tfor ;\n\tTue, 20 Apr 2021 02:51:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A96D568838;\n\tTue, 20 Apr 2021 04:51:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F87C602CA\n\tfor ;\n\tTue, 20 Apr 2021 04:51:54 +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 B185C470;\n\tTue, 20 Apr 2021 04:51:53 +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=\"Ll94q9Ea\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618887113;\n\tbh=GvsRUW5kjLID5apurj6uVutDVo2rfU7sAbC6+c02Xj4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ll94q9EaQTikLVezXkP4/AVGiG1i1r/4wKxOB/h2+7jVLf/JNJygAuoI+xzFWDyAg\n\tbjjSPTNF+MNFGITK0nFH6qDdz/AaWiPiM3FB4Ba1QK3jo4ru7+eWblotv/P3Qi69UQ\n\thvkxt0hBTuQXP+V4VWaUV1XEqzB9wVhD7ZyTVn2Y=","Date":"Tue, 20 Apr 2021 05:51:50 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210408085101.1691729-1-hiroh@chromium.org>\n\t<20210408085101.1691729-4-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210408085101.1691729-4-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 3/3] libcamera: ipu3: Cancel\n\tpending requests correctly","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\" "}},{"id":16383,"web_url":"https://patchwork.libcamera.org/comment/16383/","msgid":"<7e06c54d-e9d0-453f-0015-02780c4a99e4@ideasonboard.com>","date":"2021-04-20T10:48:16","subject":"Re: [libcamera-devel] [RFC PATCH v3 3/3] libcamera: ipu3: Cancel\n\tpending requests correctly","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent, Hiro,\n\nOn 20/04/2021 03:51, Laurent Pinchart wrote:\n> Hi Hiro,\n> \n> Thank you for the patch.\n> \n> On Thu, Apr 08, 2021 at 05:51:01PM +0900, Hirokazu Honda wrote:\n>> IPU3CameraData stores requests that are not queued yet to a\n>> camera node. They should be reported as cancel upon\n>> PipelineHandlerIPU3::stop().\n>>\n>> Signed-off-by: Hirokazu Honda \n>> ---\n>> include/libcamera/buffer.h | 3 +++\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++++++++++++-\n>> 2 files changed, 21 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n>> index 620f8a66..72c62144 100644\n>> --- a/include/libcamera/buffer.h\n>> +++ b/include/libcamera/buffer.h\n>> @@ -58,6 +58,9 @@ private:\n>> \n>> \tfriend class V4L2VideoDevice; /* Needed to update metadata_. */\n>> \n>> +\t/*! HACK! */\n>> +\tfriend class IPU3CameraData; /* Needed to update metadata_. */\n> \n> Looks like we need a way to handle this cleanly :-) It may relate to the\n> discussion we had earlier, about the rework of the buffer and requests\n> state handling. I think a redesign of that mechanism, with a cleaner API\n> for completion, would be nice. Making the Request class inherit from\n> Extensible, with Request::Private being defined in\n> include/libcamera/internal/request.h and offering public functions for\n> the pipeline handlers may be part of the solution. The \"Private\" name in\n> the our Extensible design pattern (which implements the p-impl design\n> pattern) means private to applications, in the public API, but it can be\n> accessed within libcamera.\n> \n\nI have an as-yet-unpublished patch to make Request use Extensible, and I\nalso have a patch to provide a 'cancel' operation on Buffers.\n\nPerhaps I should send those out ...\n\n\n\n> The rest of the patch looks good.\n> \n>> +\n>> \tstd::vector planes_;\n>> \n>> \tRequest *request_;\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index 462a0d9b..f89b8f3f 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>> +\tvoid cancelPendingRequests();\n>> \tint queuePendingRequests();\n>> \n>> \tCIO2Device cio2_;\n>> @@ -768,7 +769,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>> \tIPU3CameraData *data = cameraData(camera);\n>> \tint ret = 0;\n>> \n>> -\tdata->pendingRequests_ = {};\n>> +\tdata->cancelPendingRequests();\n>> \n>> \tdata->ipa_->stop();\n>> \n>> @@ -780,6 +781,22 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n>> \tfreeBuffers(camera);\n>> }\n>> \n>> +void IPU3CameraData::cancelPendingRequests()>> +{\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->metadata_.status = FrameMetadata::FrameCancelled;\n>> +\t\t\tpipe_->completeBuffer(request, buffer);\n>> +\t\t}\n>> +\n>> +\t\tpipe_->completeRequest(request);\n>> +\t\tpendingRequests_.pop();\n>> +\t}\n>> +}\n>> +\n>> int IPU3CameraData::queuePendingRequests()\n>> {\n>> \twhile (!pendingRequests_.empty()) {\n>","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 814DDBDB15\n\tfor ;\n\tTue, 20 Apr 2021 10:48:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE0D168830;\n\tTue, 20 Apr 2021 12:48:19 +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 3A074602D1\n\tfor ;\n\tTue, 20 Apr 2021 12:48:19 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 87FEC470;\n\tTue, 20 Apr 2021 12:48:18 +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=\"NfyHJF9c\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618915698;\n\tbh=laYHspOd4NeiS6nABEgPuC+X+YcPwyLuPxF97D6q9Lk=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=NfyHJF9cxTUVP9/9FQGcw1C+Ifhx4Yopq0XxoQJaRy1iSa8BzyX+iueAcGywfR+eu\n\tkYSrN0fT6bEeqbvsW8YBKRWoDOb6rNCzBp8z9cS8otKXZ9f0awY/vmbpqSaUUuzzwZ\n\tpeypF5FCZno2wumCsJSWSvc0m2KI7RB19Vhcv/VI=","To":"Laurent Pinchart ,\n\tHirokazu Honda ","References":"<20210408085101.1691729-1-hiroh@chromium.org>\n\t<20210408085101.1691729-4-hiroh@chromium.org>\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<7e06c54d-e9d0-453f-0015-02780c4a99e4@ideasonboard.com>","Date":"Tue, 20 Apr 2021 11:48:16 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [RFC PATCH v3 3/3] libcamera: ipu3: Cancel\n\tpending requests correctly","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","Reply-To":"kieran.bingham@ideasonboard.com","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\" "}},{"id":16398,"web_url":"https://patchwork.libcamera.org/comment/16398/","msgid":"","date":"2021-04-20T20:44:22","subject":"Re: [libcamera-devel] [RFC PATCH v3 3/3] libcamera: ipu3: Cancel\n\tpending requests correctly","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Apr 20, 2021 at 11:48:16AM +0100, Kieran Bingham wrote:\n> On 20/04/2021 03:51, Laurent Pinchart wrote:\n> > On Thu, Apr 08, 2021 at 05:51:01PM +0900, Hirokazu Honda wrote:\n> >> IPU3CameraData stores requests that are not queued yet to a\n> >> camera node. They should be reported as cancel upon\n> >> PipelineHandlerIPU3::stop().\n> >>\n> >> Signed-off-by: Hirokazu Honda \n> >> ---\n> >> include/libcamera/buffer.h | 3 +++\n> >> src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++++++++++++-\n> >> 2 files changed, 21 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> >> index 620f8a66..72c62144 100644\n> >> --- a/include/libcamera/buffer.h\n> >> +++ b/include/libcamera/buffer.h\n> >> @@ -58,6 +58,9 @@ private:\n> >> \n> >> \tfriend class V4L2VideoDevice; /* Needed to update metadata_. */\n> >> \n> >> +\t/*! HACK! */\n> >> +\tfriend class IPU3CameraData; /* Needed to update metadata_. */\n> > \n> > Looks like we need a way to handle this cleanly :-) It may relate to the\n> > discussion we had earlier, about the rework of the buffer and requests\n> > state handling. I think a redesign of that mechanism, with a cleaner API\n> > for completion, would be nice. Making the Request class inherit from\n> > Extensible, with Request::Private being defined in\n> > include/libcamera/internal/request.h and offering public functions for\n> > the pipeline handlers may be part of the solution. The \"Private\" name in\n> > the our Extensible design pattern (which implements the p-impl design\n> > pattern) means private to applications, in the public API, but it can be\n> > accessed within libcamera.\n> \n> I have an as-yet-unpublished patch to make Request use Extensible, and I\n> also have a patch to provide a 'cancel' operation on Buffers.\n> \n> Perhaps I should send those out ...\n\nThat would be a good way to move the discussion forward.\n\n> > The rest of the patch looks good.\n> > \n> >> +\n> >> \tstd::vector planes_;\n> >> \n> >> \tRequest *request_;\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index 462a0d9b..f89b8f3f 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> >> +\tvoid cancelPendingRequests();\n> >> \tint queuePendingRequests();\n> >> \n> >> \tCIO2Device cio2_;\n> >> @@ -768,7 +769,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> >> \tIPU3CameraData *data = cameraData(camera);\n> >> \tint ret = 0;\n> >> \n> >> -\tdata->pendingRequests_ = {};\n> >> +\tdata->cancelPendingRequests();\n> >> \n> >> \tdata->ipa_->stop();\n> >> \n> >> @@ -780,6 +781,22 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> >> \tfreeBuffers(camera);\n> >> }\n> >> \n> >> +void IPU3CameraData::cancelPendingRequests()>> +{\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->metadata_.status = FrameMetadata::FrameCancelled;\n> >> +\t\t\tpipe_->completeBuffer(request, buffer);\n> >> +\t\t}\n> >> +\n> >> +\t\tpipe_->completeRequest(request);\n> >> +\t\tpendingRequests_.pop();\n> >> +\t}\n> >> +}\n> >> +\n> >> int IPU3CameraData::queuePendingRequests()\n> >> {\n> >> \twhile (!pendingRequests_.empty()) {","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 B6D45BDB16\n\tfor ;\n\tTue, 20 Apr 2021 20:44:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 35C4668840;\n\tTue, 20 Apr 2021 22:44:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D9F360516\n\tfor ;\n\tTue, 20 Apr 2021 22:44:27 +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 CEE7A411;\n\tTue, 20 Apr 2021 22:44:26 +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=\"QcED7wFh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618951467;\n\tbh=pLU6REBPQPQAn2av3ZCqgcS2YGit1Jg5tGJIS1kNGF4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QcED7wFhfEJSZhOS9WDqBCA9O9STSHkHn8IkwsYXWo2khnUi6hBuUfsvjTqcqkxut\n\t4778aydNFhk3Y8RG3fwUC2RFTwPny1NHcSt3NUkWLouSm29xK9P2mHJfEA7noeyQUN\n\tlK/lyAuxndRmh5nFGDOyLHYhkCrJfYE/QI/7schI=","Date":"Tue, 20 Apr 2021 23:44:22 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210408085101.1691729-1-hiroh@chromium.org>\n\t<20210408085101.1691729-4-hiroh@chromium.org>\n\t\n\t<7e06c54d-e9d0-453f-0015-02780c4a99e4@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<7e06c54d-e9d0-453f-0015-02780c4a99e4@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 3/3] libcamera: ipu3: Cancel\n\tpending requests correctly","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\" "}},{"id":16421,"web_url":"https://patchwork.libcamera.org/comment/16421/","msgid":"","date":"2021-04-21T02:09:51","subject":"Re: [libcamera-devel] [RFC PATCH v3 3/3] libcamera: ipu3: Cancel\n\tpending requests correctly","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent and Kieran,\n\nOn Wed, Apr 21, 2021 at 5:44 AM Laurent Pinchart\n wrote:\n>\n> Hi Kieran,\n>\n> On Tue, Apr 20, 2021 at 11:48:16AM +0100, Kieran Bingham wrote:\n> > On 20/04/2021 03:51, Laurent Pinchart wrote:\n> > > On Thu, Apr 08, 2021 at 05:51:01PM +0900, Hirokazu Honda wrote:\n> > >> IPU3CameraData stores requests that are not queued yet to a\n> > >> camera node. They should be reported as cancel upon\n> > >> PipelineHandlerIPU3::stop().\n> > >>\n> > >> Signed-off-by: Hirokazu Honda \n> > >> ---\n> > >> include/libcamera/buffer.h | 3 +++\n> > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++++++++++++-\n> > >> 2 files changed, 21 insertions(+), 1 deletion(-)\n> > >>\n> > >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > >> index 620f8a66..72c62144 100644\n> > >> --- a/include/libcamera/buffer.h\n> > >> +++ b/include/libcamera/buffer.h\n> > >> @@ -58,6 +58,9 @@ private:\n> > >>\n> > >> friend class V4L2VideoDevice; /* Needed to update metadata_. */\n> > >>\n> > >> + /*! HACK! */\n> > >> + friend class IPU3CameraData; /* Needed to update metadata_. */\n> > >\n> > > Looks like we need a way to handle this cleanly :-) It may relate to the\n> > > discussion we had earlier, about the rework of the buffer and requests\n> > > state handling. I think a redesign of that mechanism, with a cleaner API\n> > > for completion, would be nice. Making the Request class inherit from\n> > > Extensible, with Request::Private being defined in\n> > > include/libcamera/internal/request.h and offering public functions for\n> > > the pipeline handlers may be part of the solution. The \"Private\" name in\n> > > the our Extensible design pattern (which implements the p-impl design\n> > > pattern) means private to applications, in the public API, but it can be\n> > > accessed within libcamera.\n> >\n> > I have an as-yet-unpublished patch to make Request use Extensible, and I\n> > also have a patch to provide a 'cancel' operation on Buffers.\n> >\n> > Perhaps I should send those out ...\n>\n> That would be a good way to move the discussion forward.\n>\n\nI got it.\nI will submit the next patch series on top of Kieran's patch then.\n-Hiro\n\n> > > The rest of the patch looks good.\n> > >\n> > >> +\n> > >> std::vector planes_;\n> > >>\n> > >> Request *request_;\n> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> index 462a0d9b..f89b8f3f 100644\n> > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >> @@ -66,6 +66,7 @@ public:\n> > >> void cio2BufferReady(FrameBuffer *buffer);\n> > >> void paramBufferReady(FrameBuffer *buffer);\n> > >> void statBufferReady(FrameBuffer *buffer);\n> > >> + void cancelPendingRequests();\n> > >> int queuePendingRequests();\n> > >>\n> > >> CIO2Device cio2_;\n> > >> @@ -768,7 +769,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > >> IPU3CameraData *data = cameraData(camera);\n> > >> int ret = 0;\n> > >>\n> > >> - data->pendingRequests_ = {};\n> > >> + data->cancelPendingRequests();\n> > >>\n> > >> data->ipa_->stop();\n> > >>\n> > >> @@ -780,6 +781,22 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n> > >> freeBuffers(camera);\n> > >> }\n> > >>\n> > >> +void IPU3CameraData::cancelPendingRequests()>> +{\n> > >> + while (!pendingRequests_.empty()) {\n> > >> + Request *request = pendingRequests_.front();\n> > >> +\n> > >> + for (auto it : request->buffers()) {\n> > >> + FrameBuffer *buffer = it.second;\n> > >> + buffer->metadata_.status = FrameMetadata::FrameCancelled;\n> > >> + pipe_->completeBuffer(request, buffer);\n> > >> + }\n> > >> +\n> > >> + pipe_->completeRequest(request);\n> > >> + pendingRequests_.pop();\n> > >> + }\n> > >> +}\n> > >> +\n> > >> int IPU3CameraData::queuePendingRequests()\n> > >> {\n> > >> while (!pendingRequests_.empty()) {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 D804FBDB15\n\tfor ;\n\tWed, 21 Apr 2021 02:10:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA21D68843;\n\tWed, 21 Apr 2021 04:10:03 +0200 (CEST)","from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com\n\t[IPv6:2a00:1450:4864:20::62f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E90F602C8\n\tfor ;\n\tWed, 21 Apr 2021 04:10:03 +0200 (CEST)","by mail-ej1-x62f.google.com with SMTP id u17so61219875ejk.2\n\tfor ;\n\tTue, 20 Apr 2021 19:10:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"eOYtHw6i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=V/KL5yzDo7of+TGf03F9Djj0AFYGzLn3o8xPiOhwx64=;\n\tb=eOYtHw6iz6ANfxpfVgAUWWFCuvERc1CbGI7bIo0z0DDY/exs1bN4meeun2VaTaQooq\n\tgKy3Vksq43QVjqIRGXcVUN8v2El/gvVaInVQLaBXxr06ZPnDdPgjNi7Z9/QrvaXLYR1f\n\tm68fcwtb0UpFOHivUxEu1K0/4Jey90Xgsa3Zs=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=V/KL5yzDo7of+TGf03F9Djj0AFYGzLn3o8xPiOhwx64=;\n\tb=PsFCQcyyjAzqXzPGqtmo2ISD/CG+2JIz3zrOX6NQjuev7Pyz+QTjVyz1I6GLChJXZy\n\taQRrfG32Poat9G4H9+q8LvWhWD8gmv9P+7/oGm0wv72LUhG2tyZTq5yVNWdBP2QFqtsH\n\thqqUlW15AbkGm2jwXqwQV364dJIMfceeEN3oS7yDHJZylJRvz7j1UiZfmu7f6aLHT1ZL\n\toGrZgBpvuwAexzlfBybWRkcpylgKdi++lVOiLwJGRGe4QiQ6Hc/czBFEn/aOA9+Pt2zi\n\tVdCxGuS5qq3ZanMshgVeyAgdJaSwBvqixKCMXWa5yZumSLnBE+34tjRyv7F9Y4iFFqgL\n\tWSsQ==","X-Gm-Message-State":"AOAM533/nAuHQHOxQ2qWMjyTQsdcDznpjmHDL3gRk9V4iETXwM0rkiux\n\tIySfxW7HjRwftA6xDqRKIfMwlZzxSqd5Y8ZU60qGPP5UGaw=","X-Google-Smtp-Source":"ABdhPJxhBrGN8GiyswsHQ4Nus6OVsV/yenuw3J5XjYA9fgNoS8UXc7gBMH/e4NYX6Ka3kF8mXu2ns66FHyO2iPk1B+o=","X-Received":"by 2002:a17:906:b890:: with SMTP id\n\thb16mr30424297ejb.221.1618971002670; \n\tTue, 20 Apr 2021 19:10:02 -0700 (PDT)","MIME-Version":"1.0","References":"<20210408085101.1691729-1-hiroh@chromium.org>\n\t<20210408085101.1691729-4-hiroh@chromium.org>\n\t\n\t<7e06c54d-e9d0-453f-0015-02780c4a99e4@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 21 Apr 2021 11:09:51 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [RFC PATCH v3 3/3] libcamera: ipu3: Cancel\n\tpending requests correctly","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]