[{"id":15876,"web_url":"https://patchwork.libcamera.org/comment/15876/","msgid":"<YFue7eBd/l8sRStJ@pendragon.ideasonboard.com>","date":"2021-03-24T20:19:57","subject":"Re: [libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Wed, Mar 24, 2021 at 08:06:24PM +0000, Kieran Bingham wrote:\n> When the CIO2 returns a cancelled buffer, we will not queue buffers\n> to the IMGU.\n> \n> These buffers should be explicitly marked as cancelled to ensure\n> the application knows there is no valid metadata or frame data\n> provided in the buffer.\n> \n> Provide a cancel() method on the FrameBuffer to allow explicitly\n> cancelling a buffer.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  include/libcamera/buffer.h           | 1 +\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n>  2 files changed, 6 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 302fe3d3e86b..6557fb1e59b5 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -49,6 +49,7 @@ public:\n>  \tRequest *request() const { return request_; }\n>  \tvoid setRequest(Request *request) { request_ = request; }\n>  \tconst FrameMetadata &metadata() const { return metadata_; }\n> +\tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n\nMissing documentation ? I think we should document the function, but\nalso explained at a higher level in the FrameBuffer documentation how\nit's meant to be used.\n\n>  \n>  \tunsigned int cookie() const { return cookie_; }\n>  \tvoid setCookie(unsigned int cookie) { cookie_ = cookie; }\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index fc40dcb381ea..44bd5a9d042b 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1256,8 +1256,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>  \n>  \t/* If the buffer is cancelled force a complete of the whole request. */\n>  \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> -\t\tfor (auto it : request->buffers())\n> -\t\t\tpipe_->completeBuffer(request, it.second);\n> +\t\tfor (auto it : request->buffers()) {\n> +\t\t\tFrameBuffer *b = it.second;\n> +\t\t\tb->cancel();\n> +\t\t\tpipe_->completeBuffer(request, b);\n> +\t\t}\n>  \n>  \t\tframeInfos_.remove(info);\n>  \t\tpipe_->completeRequest(request);\n\nPipeline handlers never set the buffer status explicitly today, as this\nis handled in V4L2VideoDevice. As you've found out, it causes issues we\nrequests whose buffers haven't been queued. Adding a\nFrameBuffer::cancel() function makes the API a bit asymmetrical, which\nbothers me a bit. There's also the issue that this function shouldn't be\nvisible to applications.\n\nI wonder, would it be better to create a Request::cancel() function\ninstead, that will cancel all buffers automatically and complete the\nrequest ?","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 AC0CDC32E7\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 20:20:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13D2368D61;\n\tWed, 24 Mar 2021 21:20:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7CCD3602E3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 21:20:41 +0100 (CET)","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 F2E2F580;\n\tWed, 24 Mar 2021 21:20:40 +0100 (CET)"],"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=\"nKdoRxAL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616617241;\n\tbh=n46/C6sOT76N1+ipxd5dy29Jl+W3POvOQDyJNhTEFA4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nKdoRxALaYBzP8TJ9X7nEjA0o9Ay+FH/1eREbUKHhhUqbDO+ELuDttTMhp1eke55/\n\t4NlP2kwmZEEp9Ks4PmeUl8GQ1z4ACi22klX/8GTjAhEKX5wpo2k8No4kzn2FNowCkL\n\tjzLvJ+F4E+5seb+nmznP9xi+xQpl7xrcWQZyuqoo=","Date":"Wed, 24 Mar 2021 22:19:57 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YFue7eBd/l8sRStJ@pendragon.ideasonboard.com>","References":"<YFtp4tEBzwD9lQa+@pendragon.ideasonboard.com>\n\t<20210324200624.1347604-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210324200624.1347604-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15877,"web_url":"https://patchwork.libcamera.org/comment/15877/","msgid":"<14e773d4-58d9-6882-f2cb-64ff11879d0b@ideasonboard.com>","date":"2021-03-24T20:30:11","subject":"Re: [libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 24/03/2021 20:19, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Wed, Mar 24, 2021 at 08:06:24PM +0000, Kieran Bingham wrote:\n>> When the CIO2 returns a cancelled buffer, we will not queue buffers\n>> to the IMGU.\n>>\n>> These buffers should be explicitly marked as cancelled to ensure\n>> the application knows there is no valid metadata or frame data\n>> provided in the buffer.\n>>\n>> Provide a cancel() method on the FrameBuffer to allow explicitly\n>> cancelling a buffer.\n>>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>> ---\n>>  include/libcamera/buffer.h           | 1 +\n>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n>>  2 files changed, 6 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n>> index 302fe3d3e86b..6557fb1e59b5 100644\n>> --- a/include/libcamera/buffer.h\n>> +++ b/include/libcamera/buffer.h\n>> @@ -49,6 +49,7 @@ public:\n>>  \tRequest *request() const { return request_; }\n>>  \tvoid setRequest(Request *request) { request_ = request; }\n>>  \tconst FrameMetadata &metadata() const { return metadata_; }\n>> +\tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> \n> Missing documentation ? I think we should document the function, but\n> also explained at a higher level in the FrameBuffer documentation how\n> it's meant to be used.\n\nWell, I'll do the documentation when I know the patch won't get thrown\naway ;-)\n\n> \n>>  \n>>  \tunsigned int cookie() const { return cookie_; }\n>>  \tvoid setCookie(unsigned int cookie) { cookie_ = cookie; }\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index fc40dcb381ea..44bd5a9d042b 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -1256,8 +1256,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>>  \n>>  \t/* If the buffer is cancelled force a complete of the whole request. */\n>>  \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n>> -\t\tfor (auto it : request->buffers())\n>> -\t\t\tpipe_->completeBuffer(request, it.second);\n>> +\t\tfor (auto it : request->buffers()) {\n>> +\t\t\tFrameBuffer *b = it.second;\n>> +\t\t\tb->cancel();\n>> +\t\t\tpipe_->completeBuffer(request, b);\n>> +\t\t}\n>>  \n>>  \t\tframeInfos_.remove(info);\n>>  \t\tpipe_->completeRequest(request);\n> \n> Pipeline handlers never set the buffer status explicitly today, as this\n> is handled in V4L2VideoDevice. As you've found out, it causes issues we\n> requests whose buffers haven't been queued. Adding a\n\nYes, which wasn't a problem when the Buffer is initialised to a state\nwhich means it must be explicitly marked as successful.\n\n\n> FrameBuffer::cancel() function makes the API a bit asymmetrical, which\n> bothers me a bit. There's also the issue that this function shouldn't be\n> visible to applications.\n> \n> I wonder, would it be better to create a Request::cancel() function\n> instead, that will cancel all buffers automatically and complete the\n> request ?\n\nIf one buffer is cancelled, should all buffers be cancelled?\n\nWhat happens to other pending events like buffers which are still being\nreturned and may have succeeded, or will be cancelled.\n\nCancelling and automatically completing the request sounds like it would\ncause some difficulties ... Just as we've seen with the IPU3 Frames\ncompleting too soon...","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 4591CC32E5\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 20:30:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8DE8668D61;\n\tWed, 24 Mar 2021 21:30:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 18031602E3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 21:30:15 +0100 (CET)","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 8FC6F580;\n\tWed, 24 Mar 2021 21:30:14 +0100 (CET)"],"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=\"FrtCbzqb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616617814;\n\tbh=m5e+r72x/98KACnNOhCTa4RKLxjAb1O7KIZ2SCuXwpo=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=FrtCbzqbm2JP52wBf1bsNnlDJ8y+wO5ue6uP7zjt2ckZVD4TYU8u2fw1wlLgIQlMi\n\tDVxy4qS/Tg4gFrtK+aPuFt3mvLna10KUV1I+YsyAJtJiJHcKsjqm3R5/kMvnE3VXaj\n\t4rzmkIdvrzj/UW8clvHMgj4L7inn7as3K+XmIbNU=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<YFtp4tEBzwD9lQa+@pendragon.ideasonboard.com>\n\t<20210324200624.1347604-1-kieran.bingham@ideasonboard.com>\n\t<YFue7eBd/l8sRStJ@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<14e773d4-58d9-6882-f2cb-64ff11879d0b@ideasonboard.com>","Date":"Wed, 24 Mar 2021 20:30:11 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<YFue7eBd/l8sRStJ@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15878,"web_url":"https://patchwork.libcamera.org/comment/15878/","msgid":"<945159a2-7277-43c3-e431-7424e5554324@ideasonboard.com>","date":"2021-03-24T20:34:06","subject":"Re: [libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran, Laurent,\n\nOn 24/03/2021 21:19, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Wed, Mar 24, 2021 at 08:06:24PM +0000, Kieran Bingham wrote:\n>> When the CIO2 returns a cancelled buffer, we will not queue buffers\n>> to the IMGU.\n>>\n>> These buffers should be explicitly marked as cancelled to ensure\n>> the application knows there is no valid metadata or frame data\n>> provided in the buffer.\n>>\n>> Provide a cancel() method on the FrameBuffer to allow explicitly\n>> cancelling a buffer.\n>>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>> ---\n>>  include/libcamera/buffer.h           | 1 +\n>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n>>  2 files changed, 6 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n>> index 302fe3d3e86b..6557fb1e59b5 100644\n>> --- a/include/libcamera/buffer.h\n>> +++ b/include/libcamera/buffer.h\n>> @@ -49,6 +49,7 @@ public:\n>>  \tRequest *request() const { return request_; }\n>>  \tvoid setRequest(Request *request) { request_ = request; }\n>>  \tconst FrameMetadata &metadata() const { return metadata_; }\n>> +\tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> \n> Missing documentation ? I think we should document the function, but\n> also explained at a higher level in the FrameBuffer documentation how\n> it's meant to be used.\n> \n>>  \n>>  \tunsigned int cookie() const { return cookie_; }\n>>  \tvoid setCookie(unsigned int cookie) { cookie_ = cookie; }\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index fc40dcb381ea..44bd5a9d042b 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -1256,8 +1256,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>>  \n>>  \t/* If the buffer is cancelled force a complete of the whole request. */\n>>  \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n>> -\t\tfor (auto it : request->buffers())\n>> -\t\t\tpipe_->completeBuffer(request, it.second);\n>> +\t\tfor (auto it : request->buffers()) {\n>> +\t\t\tFrameBuffer *b = it.second;\n>> +\t\t\tb->cancel();\n>> +\t\t\tpipe_->completeBuffer(request, b);\n>> +\t\t}\n>>  \n>>  \t\tframeInfos_.remove(info);\n>>  \t\tpipe_->completeRequest(request);\n> \n> Pipeline handlers never set the buffer status explicitly today, as this\n> is handled in V4L2VideoDevice. As you've found out, it causes issues we\n> requests whose buffers haven't been queued. Adding a\n> FrameBuffer::cancel() function makes the API a bit asymmetrical, which\n> bothers me a bit. There's also the issue that this function shouldn't be\n> visible to applications.\n> \n> I wonder, would it be better to create a Request::cancel() function\n> instead, that will cancel all buffers automatically and complete the\n> request ?\n\nI like that latest idea better.\nThe comment in cio2BufferReady() is \"If the buffer is cancelled\" => but\nisn't it indeed the request which is cancelled :-) ?","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 58883C32E7\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 20:34:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9FC3668D61;\n\tWed, 24 Mar 2021 21:34:08 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BF43602E3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 21:34:07 +0100 (CET)","from [IPv6:2a01:e0a:169:7140:21bd:98cc:c21e:1364] (unknown\n\t[IPv6:2a01:e0a:169:7140:21bd:98cc:c21e:1364])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F2C84580;\n\tWed, 24 Mar 2021 21:34:06 +0100 (CET)"],"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=\"E87WSlwV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616618047;\n\tbh=8BlhnHjUwbv1mspQfkTSHToYrYXc+jKFOIurd6J2BR4=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=E87WSlwVcHo6x5zJ9RzplCf6epfXHVfiWZG3IRyEdNl3DbZABWaLXq0SQkukC5LzK\n\tmiiuzSI4Msdrqa28a8QWa8BecA/97TSOq3mvCNNTjnFhaLHgW3S6yUFjOIyaml34Ph\n\tY22b8nD2N4dYWvWegt9S7VX6EghMSeJSeG7WvaP0=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tKieran Bingham <kieran.bingham@ideasonboard.com>","References":"<YFtp4tEBzwD9lQa+@pendragon.ideasonboard.com>\n\t<20210324200624.1347604-1-kieran.bingham@ideasonboard.com>\n\t<YFue7eBd/l8sRStJ@pendragon.ideasonboard.com>","From":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Message-ID":"<945159a2-7277-43c3-e431-7424e5554324@ideasonboard.com>","Date":"Wed, 24 Mar 2021 21:34:06 +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":"<YFue7eBd/l8sRStJ@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15882,"web_url":"https://patchwork.libcamera.org/comment/15882/","msgid":"<CAEmqJPoE0D-Y7G9rVxOQcNdwTWBEz4fKu3ga96vWsi8ACm+VaA@mail.gmail.com>","date":"2021-03-24T21:44:18","subject":"Re: [libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\n\nOn Wed, 24 Mar 2021 at 20:34, Jean-Michel Hautbois <\njeanmichel.hautbois@ideasonboard.com> wrote:\n\n> Hi Kieran, Laurent,\n>\n> On 24/03/2021 21:19, Laurent Pinchart wrote:\n> > Hi Kieran,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Mar 24, 2021 at 08:06:24PM +0000, Kieran Bingham wrote:\n> >> When the CIO2 returns a cancelled buffer, we will not queue buffers\n> >> to the IMGU.\n> >>\n> >> These buffers should be explicitly marked as cancelled to ensure\n> >> the application knows there is no valid metadata or frame data\n> >> provided in the buffer.\n> >>\n> >> Provide a cancel() method on the FrameBuffer to allow explicitly\n> >> cancelling a buffer.\n> >>\n> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >> ---\n> >>  include/libcamera/buffer.h           | 1 +\n> >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n> >>  2 files changed, 6 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> >> index 302fe3d3e86b..6557fb1e59b5 100644\n> >> --- a/include/libcamera/buffer.h\n> >> +++ b/include/libcamera/buffer.h\n> >> @@ -49,6 +49,7 @@ public:\n> >>      Request *request() const { return request_; }\n> >>      void setRequest(Request *request) { request_ = request; }\n> >>      const FrameMetadata &metadata() const { return metadata_; }\n> >> +    void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> >\n> > Missing documentation ? I think we should document the function, but\n> > also explained at a higher level in the FrameBuffer documentation how\n> > it's meant to be used.\n> >\n> >>\n> >>      unsigned int cookie() const { return cookie_; }\n> >>      void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> index fc40dcb381ea..44bd5a9d042b 100644\n> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >> @@ -1256,8 +1256,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer\n> *buffer)\n> >>\n> >>      /* If the buffer is cancelled force a complete of the whole\n> request. */\n> >>      if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> >> -            for (auto it : request->buffers())\n> >> -                    pipe_->completeBuffer(request, it.second);\n> >> +            for (auto it : request->buffers()) {\n> >> +                    FrameBuffer *b = it.second;\n> >> +                    b->cancel();\n> >> +                    pipe_->completeBuffer(request, b);\n> >> +            }\n> >>\n> >>              frameInfos_.remove(info);\n> >>              pipe_->completeRequest(request);\n> >\n> > Pipeline handlers never set the buffer status explicitly today, as this\n> > is handled in V4L2VideoDevice. As you've found out, it causes issues we\n> > requests whose buffers haven't been queued. Adding a\n> > FrameBuffer::cancel() function makes the API a bit asymmetrical, which\n> > bothers me a bit. There's also the issue that this function shouldn't be\n> > visible to applications.\n> >\n> > I wonder, would it be better to create a Request::cancel() function\n> > instead, that will cancel all buffers automatically and complete the\n> > request ?\n>\n\nThought I might chime in as I had to deal with exactly this scenario\nfor the Raspberry Pi pipeline handler.  My handling of incomplete\nrequests can be found at [1].  It's not the nicest bit of code, but it does\nthe job.\n\nIf there was a Request::cancel() method, that would significantly tidy\nup the function at [1], so it has my vote.  I recall mentioning this ages\nback when we first released our pipeline handler, and there was\nagreement that such a function would eventually be needed.  That time\nlooks to be now :-)\n\nThanks,\nNaush\n\n[1]\nhttps://git.linuxtv.org/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1485\n\n\n>\n> I like that latest idea better.\n> The comment in cio2BufferReady() is \"If the buffer is cancelled\" => but\n> isn't it indeed the request which is cancelled :-) ?\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 A62C1C32E7\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 21:44:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3DEE68D61;\n\tWed, 24 Mar 2021 22:44:38 +0100 (CET)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C77BE602E3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 22:44:36 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id u10so509426lju.7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 14:44:36 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"pszrXJuC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=l7A+B6HgrI6QtMEwfzmf+5dh+8jTgR6rrX7C5rdj254=;\n\tb=pszrXJuCgjO/vsLK4YVwFOaJFB8vNBePvPcfkWLaQVTxOEk5iemszyOMU+OBjqKzoo\n\twE0uR9+h+Y6IQXJ5+sRbY4cCV0kUlRCF3v95jye6Kqb9CXxOTjUi3JNaehOEqcHH5aOn\n\t7zmyTYCHqy36q1GT2ksuLCUXN4mZkmqe3EX+yPJyvmCUoUmUEj2vAfPg7CepKceyK5Hp\n\tdVG9egbpmQQD4lci2LeWUgNaTYNa1JhokeIt+rACycQMAGHDwowWNjKkb48jwPqgBPmc\n\tx01zqmLF96GY2u9yTndANUKdjWriz13RijW4Rj8XPDC0u9e2veCG5KAr6wleFiByu3yN\n\tFdhg==","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=l7A+B6HgrI6QtMEwfzmf+5dh+8jTgR6rrX7C5rdj254=;\n\tb=F+B9ySnwQsmU1SDDeM5JvJxqe49zLJyDnuaxJrg5gHcYgjjjDdqfCYn5+N7HSHUmD+\n\tsduWQt0y0z29iP0qNoNTfQGNL9rIzGWh4bYg0CIAyfIzBjyrnlWG+FJEFyAKsBycsDkK\n\twfjkEi+QXGddJ66S7koF+6tBLIUqwrz7W4QVrE566klJ0iX4ieJSEujVn3RPThwv9LrM\n\tgbX3zBnalXZsPReJ3dIsNbg7tdqvmwSaq0XLxc/mg5Y9pTr8n7mDhRliNKZ1AzEZyhZG\n\t4nZakUup2oAo1B6/uvu3JgTto7B4J1xU3+s2H8m8Z5CGzbdVTk6EvVuQUEh9bknEqCji\n\tkeCA==","X-Gm-Message-State":"AOAM532dCKbjW8aRHHxCsz/4Y0Vdxp5TRYU03HPRZGJQ10+d6qV9HPwG\n\tMiIfOm/buz+9oMOc7IRzBvVf1tj9De6RYVnd7/oREtpFBctY9Q==","X-Google-Smtp-Source":"ABdhPJySj6JCEZobkI+RYP9m/7dk9zjDQz0Cv1usv1VGFGTmk7A94FrsZqHSXPHu85yhaJxi+TXjiXMfhRGxhgKCUj4=","X-Received":"by 2002:a2e:89d4:: with SMTP id\n\tc20mr3406896ljk.169.1616622275807; \n\tWed, 24 Mar 2021 14:44:35 -0700 (PDT)","MIME-Version":"1.0","References":"<YFtp4tEBzwD9lQa+@pendragon.ideasonboard.com>\n\t<20210324200624.1347604-1-kieran.bingham@ideasonboard.com>\n\t<YFue7eBd/l8sRStJ@pendragon.ideasonboard.com>\n\t<945159a2-7277-43c3-e431-7424e5554324@ideasonboard.com>","In-Reply-To":"<945159a2-7277-43c3-e431-7424e5554324@ideasonboard.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Wed, 24 Mar 2021 21:44:18 +0000","Message-ID":"<CAEmqJPoE0D-Y7G9rVxOQcNdwTWBEz4fKu3ga96vWsi8ACm+VaA@mail.gmail.com>","To":"Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"multipart/mixed;\n\tboundary=\"===============5450834733250812251==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":15883,"web_url":"https://patchwork.libcamera.org/comment/15883/","msgid":"<c61842ff-abdd-9b8e-f0ae-215ffbf17125@ideasonboard.com>","date":"2021-03-24T22:13:25","subject":"Re: [libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3:\n\tCancel unused buffers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 24/03/2021 21:44, Naushir Patuck wrote:\n> Hi all,\n> \n> \n> On Wed, 24 Mar 2021 at 20:34, Jean-Michel Hautbois\n> <jeanmichel.hautbois@ideasonboard.com\n> <mailto:jeanmichel.hautbois@ideasonboard.com>> wrote:\n> \n>     Hi Kieran, Laurent,\n> \n>     On 24/03/2021 21:19, Laurent Pinchart wrote:\n>     > Hi Kieran,\n>     >\n>     > Thank you for the patch.\n>     >\n>     > On Wed, Mar 24, 2021 at 08:06:24PM +0000, Kieran Bingham wrote:\n>     >> When the CIO2 returns a cancelled buffer, we will not queue buffers\n>     >> to the IMGU.\n>     >>\n>     >> These buffers should be explicitly marked as cancelled to ensure\n>     >> the application knows there is no valid metadata or frame data\n>     >> provided in the buffer.\n>     >>\n>     >> Provide a cancel() method on the FrameBuffer to allow explicitly\n>     >> cancelling a buffer.\n>     >>\n>     >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com\n>     <mailto:kieran.bingham@ideasonboard.com>>\n>     >> ---\n>     >>  include/libcamera/buffer.h           | 1 +\n>     >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--\n>     >>  2 files changed, 6 insertions(+), 2 deletions(-)\n>     >>\n>     >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n>     >> index 302fe3d3e86b..6557fb1e59b5 100644\n>     >> --- a/include/libcamera/buffer.h\n>     >> +++ b/include/libcamera/buffer.h\n>     >> @@ -49,6 +49,7 @@ public:\n>     >>      Request *request() const { return request_; }\n>     >>      void setRequest(Request *request) { request_ = request; }\n>     >>      const FrameMetadata &metadata() const { return metadata_; }\n>     >> +    void cancel() { metadata_.status =\n>     FrameMetadata::FrameCancelled; }\n>     >\n>     > Missing documentation ? I think we should document the function, but\n>     > also explained at a higher level in the FrameBuffer documentation how\n>     > it's meant to be used.\n>     >\n>     >> \n>     >>      unsigned int cookie() const { return cookie_; }\n>     >>      void setCookie(unsigned int cookie) { cookie_ = cookie; }\n>     >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>     b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>     >> index fc40dcb381ea..44bd5a9d042b 100644\n>     >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>     >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>     >> @@ -1256,8 +1256,11 @@ void\n>     IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n>     >> \n>     >>      /* If the buffer is cancelled force a complete of the whole\n>     request. */\n>     >>      if (buffer->metadata().status ==\n>     FrameMetadata::FrameCancelled) {\n>     >> -            for (auto it : request->buffers())\n>     >> -                    pipe_->completeBuffer(request, it.second);\n>     >> +            for (auto it : request->buffers()) {\n>     >> +                    FrameBuffer *b = it.second;\n>     >> +                    b->cancel();\n>     >> +                    pipe_->completeBuffer(request, b);\n>     >> +            }\n>     >> \n>     >>              frameInfos_.remove(info);\n>     >>              pipe_->completeRequest(request);\n>     >\n>     > Pipeline handlers never set the buffer status explicitly today, as\n>     this\n>     > is handled in V4L2VideoDevice. As you've found out, it causes\n>     issues we\n>     > requests whose buffers haven't been queued. Adding a\n>     > FrameBuffer::cancel() function makes the API a bit asymmetrical, which\n>     > bothers me a bit. There's also the issue that this function\n>     shouldn't be\n>     > visible to applications.\n>     >\n>     > I wonder, would it be better to create a Request::cancel() function\n>     > instead, that will cancel all buffers automatically and complete the\n>     > request ?\n> \n> \n> Thought I might chime in as I had to deal with exactly this scenario\n> for the Raspberry Pi pipeline handler.  My handling of incomplete\n> requests can be found at [1].  It's not the nicest bit of code, but it does\n> the job.\n> \n> If there was a Request::cancel() method, that would significantly tidy\n> up the function at [1], so it has my vote.  I recall mentioning this ages\n> back when we first released our pipeline handler, and there was\n> agreement that such a function would eventually be needed.  That time\n> looks to be now :-)\n\nThanks for chiming in! I'm afraid to admit, I hadn't really looked at\nthe RPi pipeline handler in detail to check how this was handled there,\nand I should have indeed.\n\nAnd clearly this is going to be a repeating pattern for pipeline\nhandlers so indeed ... I suspect I know what I'm doing tomorrow.\n\nI think for the purposes of the other patches in this series, I'm going\nto split this thread out.\n\nThe others in this series all fix issues directly experienced on the\nIPU3 pipeline handler, where as this specific issue (not to reduce the\nvalue of it) is 'only' an uninitialised variable access warning from\nvalgrind.\n\nBut if we can clear up the clean-up/cancellation paths for all pipeline\nhandlers with common code, hopefully that will lead to well used/tested\ncode paths ...\n\nI'm a bit fearful of how the internal buffers may get tracked / handled\non cancellation, and what races that might cause - but that's a tomorrow\nproblem. Now if only tomorrow was more than 2 hours away ;-)\n\n--\nKieran\n\n\n\n> Thanks,\n> Naush\n> \n> [1]\n> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1485\n>  \n> \n> \n>     I like that latest idea better.\n>     The comment in cio2BufferReady() is \"If the buffer is cancelled\" => but\n>     isn't it indeed the request which is cancelled :-) ?\n> \n>     _______________________________________________\n>     libcamera-devel mailing list\n>     libcamera-devel@lists.libcamera.org\n>     <mailto:libcamera-devel@lists.libcamera.org>\n>     https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5FFF4C32E5\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 24 Mar 2021 22:13:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9E0168D61;\n\tWed, 24 Mar 2021 23:13:30 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 38D13602E3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 24 Mar 2021 23:13:29 +0100 (CET)","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 6B36F580;\n\tWed, 24 Mar 2021 23:13:28 +0100 (CET)"],"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=\"XzG3MYWQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616624008;\n\tbh=02ujTqyETDWqSE5KEEz4pg0sG/s6UWloQcuExjUHlUQ=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=XzG3MYWQHCOnlj+irK9LNRn89iCeMI1WVw0wWmDW4ghX7mUATYJ5BvA/zvH116vOh\n\tTD78o+1SfsfZDP73NusW8YZdk5NK6tMFMj/69KIxWjfCSCECgDbQ4tUujgVfZTkJMS\n\tZUFC1nHfBcBnpFC9M1/XLFIFnrAUcZnzzN+M43wM=","To":"Naushir Patuck <naush@raspberrypi.com>,\n\tJean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>","References":"<YFtp4tEBzwD9lQa+@pendragon.ideasonboard.com>\n\t<20210324200624.1347604-1-kieran.bingham@ideasonboard.com>\n\t<YFue7eBd/l8sRStJ@pendragon.ideasonboard.com>\n\t<945159a2-7277-43c3-e431-7424e5554324@ideasonboard.com>\n\t<CAEmqJPoE0D-Y7G9rVxOQcNdwTWBEz4fKu3ga96vWsi8ACm+VaA@mail.gmail.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<c61842ff-abdd-9b8e-f0ae-215ffbf17125@ideasonboard.com>","Date":"Wed, 24 Mar 2021 22:13:25 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<CAEmqJPoE0D-Y7G9rVxOQcNdwTWBEz4fKu3ga96vWsi8ACm+VaA@mail.gmail.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3.1] libcamera: pipeline: ipu3:\n\tCancel unused buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]