[{"id":31857,"web_url":"https://patchwork.libcamera.org/comment/31857/","msgid":"<172952351008.884742.5096650111250935305@ping.linuxembedded.co.uk>","date":"2024-10-21T15:11:50","subject":"Re: [PATCH v5 3/3] libcamera: software_isp: Clean up pending\n\trequests on stop","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2024-10-21 14:37:18)\n> PipelineHandler::stop() calls stopDevice() method to perform pipeline\n> specific cleanup and then completes waiting requests. If any queued\n> requests remain, an assertion error is raised.\n> \n> Software ISP stores request buffers in\n> SimpleCameraData::conversionQueue_ and queues them as V4L2 signals\n> bufferReady. stopDevice() cleanup forgets to clean up the buffers and\n> their requests from conversionQueue_, possibly resulting in the\n> assertion error. This patch fixes the omission.\n> \n> The problem wasn't very visible when\n> SimplePipelineHandler::kNumInternalBuffers (the number of buffers\n> allocated in V4L2) was equal to the number of buffers exported from\n> software ISP. But when the number of the exported buffers was increased\n> by one in commit abe2ec64f9e4e97bbdfe3a50372611bd7b5315c2, the assertion\n> error started pop up in some environments. Increasing the number of the\n> buffers much more, e.g. to 9, makes the problem very reproducible.\n> \n> Each pipeline uses its own mechanism to track the requests to clean up\n> and it can't be excluded that similar omissions are present in other\n> places. But there is no obvious way to make a common cleanup for all\n> the pipelines (except for doing it instead of raising the assertion\n> error, which is probably undesirable, in order not to hide incomplete\n> pipeline specific cleanups).\n> \n\nReviewed-by: Kieran Bingham \nTested-by: Kieran Bingham \n\n\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=234\n> Signed-off-by: Milan Zamazal \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index f3a6b1635..6b1121c34 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -288,6 +288,7 @@ public:\n> };\n> std::queue conversionQueue_;\n> bool useConversion_;\n> + void clearIncompleteRequests();\n> \n> std::unique_ptr converter_;\n> std::unique_ptr swIsp_;\n> @@ -897,6 +898,14 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> pipe->completeRequest(request);\n> }\n> \n> +void SimpleCameraData::clearIncompleteRequests()\n> +{\n> + while (!conversionQueue_.empty()) {\n> + pipe()->cancelRequest(conversionQueue_.front().request);\n> + conversionQueue_.pop();\n> + }\n> +}\n> +\n> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> {\n> swIsp_->processStats(frame, bufferId,\n> @@ -1406,6 +1415,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> \n> video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);\n> \n> + data->clearIncompleteRequests();\n> data->conversionBuffers_.clear();\n> \n> releasePipeline(data);\n> -- \n> 2.44.1\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 825C0BD808\n\tfor ;\n\tMon, 21 Oct 2024 15:11:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33A1565392;\n\tMon, 21 Oct 2024 17:11:54 +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 1069E6538A\n\tfor ;\n\tMon, 21 Oct 2024 17:11:53 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2CE054CF;\n\tMon, 21 Oct 2024 17:10:07 +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=\"ISmuZZu1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729523407;\n\tbh=5k5qv2vW7xdAwnSdXcy207Z66Guhy+s6p5kgWfKujkg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ISmuZZu1bYwJKJrgcd28OjHvQNcZ2ucbG/b2+TezxCxNOKKpgTX1uKYj87fq6+MQn\n\t9O6ZUoKAV31Anwm/TbIEoLEulFE31hdPmEfY3N0EE3EKxZB1A+zIoeuhEeEtmQvMsE\n\tmXv0T407ZUDwXlU66xhT6TmtJZpp2/vlegjllTCk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241021133718.894374-4-mzamazal@redhat.com>","References":"<20241021133718.894374-1-mzamazal@redhat.com>\n\t<20241021133718.894374-4-mzamazal@redhat.com>","Subject":"Re: [PATCH v5 3/3] libcamera: software_isp: Clean up pending\n\trequests on stop","From":"Kieran Bingham ","Cc":"Milan Zamazal , robert.mader@posteo.de,\n\tlaurent.pinchart@ideasonboard.com","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Mon, 21 Oct 2024 16:11:50 +0100","Message-ID":"<172952351008.884742.5096650111250935305@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32046,"web_url":"https://patchwork.libcamera.org/comment/32046/","msgid":"<20241106120256.GB24752@pendragon.ideasonboard.com>","date":"2024-11-06T12:02:56","subject":"Re: [PATCH v5 3/3] libcamera: software_isp: Clean up pending\n\trequests on stop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Mon, Oct 21, 2024 at 03:37:18PM +0200, Milan Zamazal wrote:\n> PipelineHandler::stop() calls stopDevice() method to perform pipeline\n> specific cleanup and then completes waiting requests. If any queued\n> requests remain, an assertion error is raised.\n> \n> Software ISP stores request buffers in\n> SimpleCameraData::conversionQueue_ and queues them as V4L2 signals\n> bufferReady. stopDevice() cleanup forgets to clean up the buffers and\n> their requests from conversionQueue_, possibly resulting in the\n> assertion error. This patch fixes the omission.\n> \n> The problem wasn't very visible when\n> SimplePipelineHandler::kNumInternalBuffers (the number of buffers\n> allocated in V4L2) was equal to the number of buffers exported from\n> software ISP. But when the number of the exported buffers was increased\n> by one in commit abe2ec64f9e4e97bbdfe3a50372611bd7b5315c2, the assertion\n> error started pop up in some environments. Increasing the number of the\n> buffers much more, e.g. to 9, makes the problem very reproducible.\n> \n> Each pipeline uses its own mechanism to track the requests to clean up\n> and it can't be excluded that similar omissions are present in other\n> places. But there is no obvious way to make a common cleanup for all\n> the pipelines (except for doing it instead of raising the assertion\n> error, which is probably undesirable, in order not to hide incomplete\n> pipeline specific cleanups).\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=234\n> Signed-off-by: Milan Zamazal \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index f3a6b1635..6b1121c34 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -288,6 +288,7 @@ public:\n> \t};\n> \tstd::queue conversionQueue_;\n> \tbool useConversion_;\n> +\tvoid clearIncompleteRequests();\n\nMember functions go before member variables. Functions covered by the\nsame access specifier (public, protected or private) should also be\nordered the same way in the class definition and in the rest of the\nfile.\n\nWith that fixed,\n\nReviewed-by: Laurent Pinchart \n\n> \n> \tstd::unique_ptr converter_;\n> \tstd::unique_ptr swIsp_;\n> @@ -897,6 +898,14 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> \t\tpipe->completeRequest(request);\n> }\n> \n> +void SimpleCameraData::clearIncompleteRequests()\n> +{\n> +\twhile (!conversionQueue_.empty()) {\n> +\t\tpipe()->cancelRequest(conversionQueue_.front().request);\n> +\t\tconversionQueue_.pop();\n> +\t}\n> +}\n> +\n> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> {\n> \tswIsp_->processStats(frame, bufferId,\n> @@ -1406,6 +1415,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> \n> \tvideo->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);\n> \n> +\tdata->clearIncompleteRequests();\n> \tdata->conversionBuffers_.clear();\n> \n> \treleasePipeline(data);","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 BE7E3BE173\n\tfor ;\n\tWed, 6 Nov 2024 12:03:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19D4C6541D;\n\tWed, 6 Nov 2024 13:03:05 +0100 (CET)","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 1CB3F653C5\n\tfor ;\n\tWed, 6 Nov 2024 13:03:03 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B276259D;\n\tWed, 6 Nov 2024 13:02:54 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"H5X2qk7r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730894574;\n\tbh=L0mM6tmno7WYYhrmm84z11xEOiojB7CIqMJVnnnU4ds=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H5X2qk7rklRxKjJ4C/ZtaS9fMk1nXtnku1utniTr4jXZ2SzYO1YwCB44KERHATrgk\n\tc00TIoL80vWDI3YlHw5S16o36cuZC2ke2LTXNTl/UfAWNhsx6CZTYYtDU8rR6ZVG27\n\tx4XcCcyNYQrAohpDedViUi6dVll87LEaVOoSa5MQ=","Date":"Wed, 6 Nov 2024 14:02:56 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v5 3/3] libcamera: software_isp: Clean up pending\n\trequests on stop","Message-ID":"<20241106120256.GB24752@pendragon.ideasonboard.com>","References":"<20241021133718.894374-1-mzamazal@redhat.com>\n\t<20241021133718.894374-4-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241021133718.894374-4-mzamazal@redhat.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]