[{"id":31624,"web_url":"https://patchwork.libcamera.org/comment/31624/","msgid":"<172840209426.532453.797124096469926439@ping.linuxembedded.co.uk>","date":"2024-10-08T15:41:34","subject":"Re: [PATCH] libcamera: software_isp: Clean up pending requests on\n\tstop","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-08 16:09:16)\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\nIndeed, I think that's why I added the assertion to make sure we force\nPipeline handlers to do 'the right thing for that handler'. Maybe we\ncould relax that in the future if it becomes apparetnly safe ... but I\nthink this patch is probably the right direction for now.\n\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 | 15 +++++++++++++++\n> 1 file changed, 15 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3ddce71d3..4e8504922 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -284,6 +284,7 @@ public:\n> std::vector> conversionBuffers_;\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,19 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> pipe->completeRequest(request);\n> }\n> \n> +void SimpleCameraData::clearIncompleteRequests()\n> +{\n> + while (!conversionQueue_.empty()) {\n> + for (auto &item : conversionQueue_.front()) {\n> + FrameBuffer *outputBuffer = item.second;\n> + Request *request = outputBuffer->request();\n\n\nI think somewhere here we should be marking the buffers as cancelled\nbefore we complete it:\n\t\t\toutputBuffer->_d()->cancel();\n\n\n\n> + pipe()->completeBuffer(request, outputBuffer);\n> + pipe()->completeRequest(request);\n\nin fact both those lines might be better as:\n\n request->_d()->cancel();\n pipe()->completeRequest(request);\n\nas I think request->_d()->cancel(); will correctly mark all buffers as\ncancelled, and the request itself.\n\nWith that handled, and retested:\n\n\nReviewed-by: Kieran Bingham \n\n\n> + }\n> + conversionQueue_.pop();\n> + }\n> +}\n> +\n> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> {\n> swIsp_->processStats(frame, bufferId,\n> @@ -1407,6 +1421,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);\n> \n> data->conversionBuffers_.clear();\n> + data->clearIncompleteRequests();\n> \n> releasePipeline(data);\n> }\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 A9274C32DE\n\tfor ;\n\tTue, 8 Oct 2024 15:41:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92D876352E;\n\tTue, 8 Oct 2024 17:41:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1207D618C9\n\tfor ;\n\tTue, 8 Oct 2024 17:41:37 +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 6064D514;\n\tTue, 8 Oct 2024 17:40:00 +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=\"uhooccjn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728402000;\n\tbh=/GErZVWKvORYIuX29BsF+yWYctaFqAqh0lsvZVr97CA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=uhooccjn7cMlQaxxAHzMojArew2opIarKnnhPf3SwhfjNIbefociMjrh9yMMGknEB\n\tpedo+prVV+oPuEfOHdU1NsPCTBbLJ712DaiwStP8UuoRm9pMXxTaFgJ1f9d4QVDHCv\n\t2RXiL4fnvOGZoPtg2GjkriJeedu79ihfBiph9NTw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241008150916.1041703-1-mzamazal@redhat.com>","References":"<20241008150916.1041703-1-mzamazal@redhat.com>","Subject":"Re: [PATCH] libcamera: software_isp: Clean up pending requests on\n\tstop","From":"Kieran Bingham ","Cc":"Milan Zamazal , robert.mader@posteo.de","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Tue, 08 Oct 2024 16:41:34 +0100","Message-ID":"<172840209426.532453.797124096469926439@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":31626,"web_url":"https://patchwork.libcamera.org/comment/31626/","msgid":"","date":"2024-10-08T16:19:51","subject":"Re: [PATCH] libcamera: software_isp: Clean up pending requests on\n\tstop","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"Hey, thanks a lot for the patch!\n\nI gave it a quick try on my Pixel 3a and while the revision here avoids \nthe assertion most of the time and makes things much more stable, I \nstill managed to trigger it with enough trying (4 buffers, \nstarting/stopping/switching cameras in Gnome Snapshot). I haven't tried \nthe suggestion from Kieran yet, but to me it looks like Stop() is \ncurrently still somehow racy - at least the way it's used by Pipewire.\n\nRegards\n\nOn 08.10.24 17:09, 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 | 15 +++++++++++++++\n> 1 file changed, 15 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3ddce71d3..4e8504922 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -284,6 +284,7 @@ public:\n> \tstd::vector> conversionBuffers_;\n> \tstd::queue> conversionQueue_;\n> \tbool useConversion_;\n> +\tvoid clearIncompleteRequests();\n> \n> \tstd::unique_ptr converter_;\n> \tstd::unique_ptr swIsp_;\n> @@ -897,6 +898,19 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> \t\tpipe->completeRequest(request);\n> }\n> \n> +void SimpleCameraData::clearIncompleteRequests()\n> +{\n> +\twhile (!conversionQueue_.empty()) {\n> +\t\tfor (auto &item : conversionQueue_.front()) {\n> +\t\t\tFrameBuffer *outputBuffer = item.second;\n> +\t\t\tRequest *request = outputBuffer->request();\n> +\t\t\tpipe()->completeBuffer(request, outputBuffer);\n> +\t\t\tpipe()->completeRequest(request);\n> +\t\t}\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> @@ -1407,6 +1421,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> \tvideo->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);\n> \n> \tdata->conversionBuffers_.clear();\n> +\tdata->clearIncompleteRequests();\n> \n> \treleasePipeline(data);\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 8D522C32DE\n\tfor ;\n\tTue, 8 Oct 2024 16:20:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31F6E6352E;\n\tTue, 8 Oct 2024 18:19:59 +0200 (CEST)","from sender4-op-o12.zoho.com (sender4-op-o12.zoho.com\n\t[136.143.188.12])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 449D2618C9\n\tfor ;\n\tTue, 8 Oct 2024 18:19:57 +0200 (CEST)","by mx.zohomail.com with SMTPS id 1728404393505619.0142265280892;\n\tTue, 8 Oct 2024 09:19:53 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"e8LzuHfO\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1728404395; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=CgzDoICn0AfNaGVwstP2g17T1sikz67gNqyAVQSrDOrdeEjkexJ9Ll5tQef32GitKrOjAjn49mCD8g7JU7wNHKx8+QGRKsAl0ya0JNIQdNulLOJoQgJdoD/Rnb3V6dk9BK8FCBSh2l1ZM7/TYfPVPN4h32VArho5nZzkCcPwfnE=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1728404395;\n\th=Content-Type:Content-Transfer-Encoding:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To:Cc;\n\tbh=7tplO8blI26Id+aQ/ckMbfqGGhfYroOAS4TIZWfBu+w=; \n\tb=aVXPG4V4cdOrKuw/e9E2RFSCRRCFQ8LUHzlWXNgFuUAzmsL4KWHp4SPnugD4CSWa/S01KtYCilI6vZXy/TKaEant3+HDgvh2XHJutDS+Fw0sPpchp321gfjwHWtJOkpceOoB+yh4j9aXeEFU104sVwKoDfzp7BLWgnPU3v+hOk0=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1728404395;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To:Cc;\n\tbh=7tplO8blI26Id+aQ/ckMbfqGGhfYroOAS4TIZWfBu+w=;\n\tb=e8LzuHfO0styXRr1y8if5vNdOzqTkA8niXi0VTbH1Rtvv8yO6YrC/IglRaPyvZ2B\n\t/2iVz4nxAZUrLwzaRpLf9Zv5raEr7eJ0Ply43aCyrlIfbVRox+1h3ONGvgg6JJ8ssS0\n\tTCS8B6RBKvaxuY1z9nOD6aG1A6esgFKxgfaZ5jLs=","Message-ID":"","Date":"Tue, 8 Oct 2024 18:19:51 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: software_isp: Clean up pending requests on\n\tstop","To":"libcamera-devel@lists.libcamera.org","References":"<20241008150916.1041703-1-mzamazal@redhat.com>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<20241008150916.1041703-1-mzamazal@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":31627,"web_url":"https://patchwork.libcamera.org/comment/31627/","msgid":"<172840489802.532453.7778157678843750580@ping.linuxembedded.co.uk>","date":"2024-10-08T16:28:18","subject":"Re: [PATCH] libcamera: software_isp: Clean up pending requests on\n\tstop","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Robert Mader (2024-10-08 17:19:51)\n> Hey, thanks a lot for the patch!\n> \n> I gave it a quick try on my Pixel 3a and while the revision here avoids \n> the assertion most of the time and makes things much more stable, I \n> still managed to trigger it with enough trying (4 buffers, \n> starting/stopping/switching cameras in Gnome Snapshot). I haven't tried \n> the suggestion from Kieran yet, but to me it looks like Stop() is \n> currently still somehow racy - at least the way it's used by Pipewire.\n\nThis current version of the patch 'completes' the buffers/requests - so I am\nweary pipewire probably tries to immediately resend them back into\nlibcamera! (Though I think we should reject any incoming requests as\nsoon as we hit stop())\n\nBut I think it's important to make sure they are 'cancelled'.\n\n--\nKieran\n\n\n> \n> Regards\n> \n> On 08.10.24 17:09, 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 | 15 +++++++++++++++\n> > 1 file changed, 15 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 3ddce71d3..4e8504922 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -284,6 +284,7 @@ public:\n> > std::vector> conversionBuffers_;\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,19 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> > pipe->completeRequest(request);\n> > }\n> > \n> > +void SimpleCameraData::clearIncompleteRequests()\n> > +{\n> > + while (!conversionQueue_.empty()) {\n> > + for (auto &item : conversionQueue_.front()) {\n> > + FrameBuffer *outputBuffer = item.second;\n> > + Request *request = outputBuffer->request();\n> > + pipe()->completeBuffer(request, outputBuffer);\n> > + pipe()->completeRequest(request);\n> > + }\n> > + conversionQueue_.pop();\n> > + }\n> > +}\n> > +\n> > void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> > {\n> > swIsp_->processStats(frame, bufferId,\n> > @@ -1407,6 +1421,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> > video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);\n> > \n> > data->conversionBuffers_.clear();\n> > + data->clearIncompleteRequests();\n> > \n> > releasePipeline(data);\n> > }\n> \n> -- \n> Robert Mader\n> Consultant Software Developer\n> \n> Collabora Ltd.\n> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK\n> Registered in England & Wales, no. 5513718\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 BE9F8BE080\n\tfor ;\n\tTue, 8 Oct 2024 16:28:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F1289618C9;\n\tTue, 8 Oct 2024 18:28:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 909CC618C9\n\tfor ;\n\tTue, 8 Oct 2024 18:28:21 +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 B989C3A2;\n\tTue, 8 Oct 2024 18:26:44 +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=\"jrBt6Lsp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728404804;\n\tbh=w2SN8bV3sitWYsg5ebMP6I1SZCaNbR/NykfpumGzTX4=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=jrBt6LspW8vRa7CtrLgX8Nt+sc3P7a7gdhuGpwaNvewMTAAJ87IJ8ksRYzOtu+du4\n\tpRGDAgOKPCi8roKoySimTDvloCTAhTay70ICAb5l9Qo2jSh+qWI5vrUMVSELH3sMiC\n\tWEnsUhFiLE72QCGC+GaHG2D6jSmZM8b6cGAHA1sY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20241008150916.1041703-1-mzamazal@redhat.com>\n\t","Subject":"Re: [PATCH] libcamera: software_isp: Clean up pending requests on\n\tstop","From":"Kieran Bingham ","To":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 08 Oct 2024 17:28:18 +0100","Message-ID":"<172840489802.532453.7778157678843750580@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":31633,"web_url":"https://patchwork.libcamera.org/comment/31633/","msgid":"<87set5r9kk.fsf@redhat.com>","date":"2024-10-09T07:59:07","subject":"Re: [PATCH] libcamera: software_isp: Clean up pending requests on\n\tstop","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Kieran,\n\nthank you for review.\n\nKieran Bingham writes:\n\n> Quoting Milan Zamazal (2024-10-08 16:09:16)\n>> PipelineHandler::stop() calls stopDevice() method to perform pipeline\n>> specific cleanup and then completes waiting requests. If any queued\n>\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> Indeed, I think that's why I added the assertion to make sure we force\n> Pipeline handlers to do 'the right thing for that handler'. Maybe we\n> could relax that in the future if it becomes apparetnly safe ... but I\n> think this patch is probably the right direction for now.\n>\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 | 15 +++++++++++++++\n>> 1 file changed, 15 insertions(+)\n>> \n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 3ddce71d3..4e8504922 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -284,6 +284,7 @@ public:\n>> std::vector> conversionBuffers_;\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,19 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n>> pipe->completeRequest(request);\n>> }\n>> \n>> +void SimpleCameraData::clearIncompleteRequests()\n>> +{\n>> + while (!conversionQueue_.empty()) {\n>> + for (auto &item : conversionQueue_.front()) {\n>> + FrameBuffer *outputBuffer = item.second;\n>> + Request *request = outputBuffer->request();\n>\n>\n> I think somewhere here we should be marking the buffers as cancelled\n> before we complete it:\n> \t\t\toutputBuffer->_d()->cancel();\n\nAh, right.\n\n>> + pipe()->completeBuffer(request, outputBuffer);\n>> + pipe()->completeRequest(request);\n>\n> in fact both those lines might be better as:\n>\n> request->_d()->cancel();\n> pipe()->completeRequest(request);\n>\n> as I think request->_d()->cancel(); will correctly mark all buffers as\n> cancelled, and the request itself.\n\nYes, it should. request->_d()->cancel() is private; but as those two\nlines are already used twice in pipeline_handler.cpp, I'll extract them\nto a separate method and will call it from here.\n\n> With that handled, and retested:\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n>> + }\n>> + conversionQueue_.pop();\n>> + }\n>> +}\n>> +\n>> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n>> {\n>> swIsp_->processStats(frame, bufferId,\n>> @@ -1407,6 +1421,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n>> video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);\n>> \n>> data->conversionBuffers_.clear();\n>> + data->clearIncompleteRequests();\n>> \n>> releasePipeline(data);\n>> }\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 D52F5C32E0\n\tfor ;\n\tWed, 9 Oct 2024 08:00:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C8056536D;\n\tWed, 9 Oct 2024 10:00:09 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B77863527\n\tfor ;\n\tWed, 9 Oct 2024 10:00:07 +0200 (CEST)","from mail-ej1-f70.google.com (mail-ej1-f70.google.com\n\t[209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-655-2nCUtUeROrigtvgqfcRc9A-1; Wed, 09 Oct 2024 03:59:26 -0400","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-a996c29edaaso117348266b.0\n\tfor ;\n\tWed, 09 Oct 2024 00:59:11 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a992e664fb9sm622708366b.88.2024.10.09.00.59.07\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 09 Oct 2024 00:59:08 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"H55t92yx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1728460806;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=jirn4676nCgLtu4fo00933i4ja6+adph8a8cC2Vchw0=;\n\tb=H55t92yxn+fRsaWqdCKPJQ299jHvam9/KQfEbE1Z5iXfsRlaM7V3h/Q2dE7BIuESwSjGCB\n\tS5vlOJb5W27uPusAwJiLtX3RhwQwzOYgnuhY26Y55Sdo58kl3pJHeS+6NsWTPH3lt/GgI8\n\t3Ak5GHSmIg+0xYqI9SGwumqo6dHAxcc=","X-MC-Unique":"2nCUtUeROrigtvgqfcRc9A-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728460749; x=1729065549;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=jirn4676nCgLtu4fo00933i4ja6+adph8a8cC2Vchw0=;\n\tb=V7mJh1fi8VVo4N8JNf1/27bsmvujuMLb4qYTST7TLGoqsqxAuVfESM/WOvOKS4F49M\n\tqo5RmjeQ3piUNgWyV7vlypE5D+jPhhjDe++po4ELdrJpJHox4+H1yaqLS+pnx8fBDyJI\n\tUpRkFLAeXsejVBfNsTty/U0yA7pvgONIJWfBX21/KQbKWMVJP2m8qO5oYP3HIxjD+f9Q\n\tPLI+UFDcs5d4Ko/39MxyRlht+oKX4OqB14liKo2xoLCdeunCCUSxGppFX5FTSjzU7QES\n\tnaI1FiA0FpGgbocygwAZGlGWVgGZI1TXS55/H4xpOZEAcNnVcIBWZGt7nojFHYBhSeLb\n\tFRfQ==","X-Gm-Message-State":"AOJu0Yx6c3anM1LhDjZXLTGHQtnd89P55nBgKwLJ4LKVBSLR0G+rbjj9\n\tjn7JWSr12gJmFlyVqq4pP2upv2uH07+WvlviiVXIgx2Xa2bLbelHnjQSD/oBS+ZIGRyKMkjg6Qw\n\tXXJXKL6Wvm5xRFdNOHSZbX/Edf3tbYtAMOHQu8frl0No18nhe3Nq5kXBVLKMLynvU+CFhG1nH6k\n\tNffDM=","X-Received":["by 2002:a17:906:7953:b0:a99:6476:ce78 with SMTP id\n\ta640c23a62f3a-a998d19f8e5mr111996566b.17.1728460749212; \n\tWed, 09 Oct 2024 00:59:09 -0700 (PDT)","by 2002:a17:906:7953:b0:a99:6476:ce78 with SMTP id\n\ta640c23a62f3a-a998d19f8e5mr111995366b.17.1728460748737; \n\tWed, 09 Oct 2024 00:59:08 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGaWHLryYgaDTWD15DV+S9mIRm/dNZ0f/ml/8WwTK1elcAgUMWM8Ugtz4E1jVyxjK6Hrlhw0A==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de","Subject":"Re: [PATCH] libcamera: software_isp: Clean up pending requests on\n\tstop","In-Reply-To":"<172840209426.532453.797124096469926439@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Tue, 08 Oct 2024 16:41:34 +0100\")","References":"<20241008150916.1041703-1-mzamazal@redhat.com>\n\t<172840209426.532453.797124096469926439@ping.linuxembedded.co.uk>","Date":"Wed, 09 Oct 2024 09:59:07 +0200","Message-ID":"<87set5r9kk.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":31635,"web_url":"https://patchwork.libcamera.org/comment/31635/","msgid":"<172846304713.3353069.16909387466039151415@ping.linuxembedded.co.uk>","date":"2024-10-09T08:37:27","subject":"Re: [PATCH] libcamera: software_isp: Clean up pending requests on\n\tstop","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-09 08:59:07)\n> Hi Kieran,\n> \n> thank you for review.\n> \n> Kieran Bingham writes:\n> \n> > Quoting Milan Zamazal (2024-10-08 16:09:16)\n> >> PipelineHandler::stop() calls stopDevice() method to perform pipeline\n> >> specific cleanup and then completes waiting requests. If any queued\n> >\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> > Indeed, I think that's why I added the assertion to make sure we force\n> > Pipeline handlers to do 'the right thing for that handler'. Maybe we\n> > could relax that in the future if it becomes apparetnly safe ... but I\n> > think this patch is probably the right direction for now.\n> >\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 | 15 +++++++++++++++\n> >> 1 file changed, 15 insertions(+)\n> >> \n> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >> index 3ddce71d3..4e8504922 100644\n> >> --- a/src/libcamera/pipeline/simple/simple.cpp\n> >> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> >> @@ -284,6 +284,7 @@ public:\n> >> std::vector> conversionBuffers_;\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,19 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> >> pipe->completeRequest(request);\n> >> }\n> >> \n> >> +void SimpleCameraData::clearIncompleteRequests()\n> >> +{\n> >> + while (!conversionQueue_.empty()) {\n> >> + for (auto &item : conversionQueue_.front()) {\n> >> + FrameBuffer *outputBuffer = item.second;\n> >> + Request *request = outputBuffer->request();\n> >\n> >\n> > I think somewhere here we should be marking the buffers as cancelled\n> > before we complete it:\n> > outputBuffer->_d()->cancel();\n> \n> Ah, right.\n> \n> >> + pipe()->completeBuffer(request, outputBuffer);\n> >> + pipe()->completeRequest(request);\n> >\n> > in fact both those lines might be better as:\n> >\n> > request->_d()->cancel();\n> > pipe()->completeRequest(request);\n> >\n> > as I think request->_d()->cancel(); will correctly mark all buffers as\n> > cancelled, and the request itself.\n> \n> Yes, it should. request->_d()->cancel() is private; but as those two\n> lines are already used twice in pipeline_handler.cpp, I'll extract them\n> to a separate method and will call it from here.\n\nGreat, - indeed the first thing I went looking for was\n\"pipe()->cancelReqeust(request)\" which isn't there (yet :D)\n\n--\nKieran\n\n\n> \n> > With that handled, and retested:\n> >\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> >\n> >> + }\n> >> + conversionQueue_.pop();\n> >> + }\n> >> +}\n> >> +\n> >> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> >> {\n> >> swIsp_->processStats(frame, bufferId,\n> >> @@ -1407,6 +1421,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> >> video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);\n> >> \n> >> data->conversionBuffers_.clear();\n> >> + data->clearIncompleteRequests();\n> >> \n> >> releasePipeline(data);\n> >> }\n> >> -- \n> >> 2.44.1\n> >>\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 4CB3AC32E0\n\tfor ;\n\tWed, 9 Oct 2024 08:37:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D922D63527;\n\tWed, 9 Oct 2024 10:37:30 +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 D5E5E63527\n\tfor ;\n\tWed, 9 Oct 2024 10:37:29 +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 B09F92EC;\n\tWed, 9 Oct 2024 10:35:52 +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=\"SmI9nbT+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728462952;\n\tbh=BhbiADHFNPYwWOkN37TqRBTs8yeojFCayDyFCbrtBTI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=SmI9nbT+vkL4Xann2u6Q5dEE4jym4MsBhTEZPbxQYrno+OAoGBBAHqV0YDLufLobE\n\tHuIf7M3R4esJ25Qc1I1x7lunJjibqZzqCcXYUhw0R0+rihe/7L/IT5mhV+YDXxp7FI\n\tIezy6qVUP3YDMqSrFwMlgSkjEYeAkpmYyqlvxSzM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<87set5r9kk.fsf@redhat.com>","References":"<20241008150916.1041703-1-mzamazal@redhat.com>\n\t<172840209426.532453.797124096469926439@ping.linuxembedded.co.uk>\n\t<87set5r9kk.fsf@redhat.com>","Subject":"Re: [PATCH] libcamera: software_isp: Clean up pending requests on\n\tstop","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de","To":"Milan Zamazal ","Date":"Wed, 09 Oct 2024 09:37:27 +0100","Message-ID":"<172846304713.3353069.16909387466039151415@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\" "}}]