[{"id":31636,"web_url":"https://patchwork.libcamera.org/comment/31636/","msgid":"<87o73tr7q1.fsf@redhat.com>","date":"2024-10-09T08:39:02","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Milan Zamazal writes:\n\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.\n>\n> This patch fixes the omission. The request must be also canceled, which\n> requires introducing a little PipelineHandler::cancelRequest helper in\n> order to be able to access the private cancel() method.\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\nChanges in v2:\n- The request is additionally canceled.\n- New helper method PipelineHandler::cancelRequest() introduced.\n\nRobert, could you please test v2?\n\n> ---\n> include/libcamera/internal/pipeline_handler.h | 1 +\n> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> 3 files changed, 31 insertions(+), 7 deletions(-)\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 0d3808036..fb28a18d0 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -60,6 +60,7 @@ public:\n> \n> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> \tvoid completeRequest(Request *request);\n> +\tvoid cancelRequest(Request *request);\n> \n> \tstd::string configurationFile(const std::string &subdir,\n> \t\t\t\t const std::string &name) const;\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(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 +1420,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> }\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index e59404691..c9cb11f0f 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> \twhile (!waitingRequests_.empty()) {\n> \t\tRequest *request = waitingRequests_.front();\n> \t\twaitingRequests_.pop();\n> -\n> -\t\trequest->_d()->cancel();\n> -\t\tcompleteRequest(request);\n> +\t\tcancelRequest(request);\n> \t}\n> \n> \t/* Make sure no requests are pending. */\n> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> \t}\n> \n> \tint ret = queueRequestDevice(camera, request);\n> -\tif (ret) {\n> -\t\trequest->_d()->cancel();\n> -\t\tcompleteRequest(request);\n> -\t}\n> +\tif (ret)\n> +\t\tcancelRequest(request);\n> }\n> \n> /**\n> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> \t}\n> }\n> \n> +/**\n> + * \\brief Cancel request and signal its completion\n> + * \\param[in] request The request to cancel\n> + *\n> + * This function cancels the request in addition to its completion. The same\n> + * rules as for completeRequest() apply.\n> + */\n> +void PipelineHandler::cancelRequest(Request *request)\n> +{\n> +\trequest->_d()->cancel();\n> +\tcompleteRequest(request);\n> +}\n> +\n> /**\n> * \\brief Retrieve the absolute path to a platform configuration file\n> * \\param[in] subdir The pipeline handler specific subdirectory name","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 3AD87C32E0\n\tfor ;\n\tWed, 9 Oct 2024 08:39:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D39D365370;\n\tWed, 9 Oct 2024 10:39:09 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA0B263527\n\tfor ;\n\tWed, 9 Oct 2024 10:39:08 +0200 (CEST)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-658-YBsePGa0McyO3dKovwnfuA-1; Wed, 09 Oct 2024 04:39:06 -0400","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-37d3986f5a4so238259f8f.3\n\tfor ;\n\tWed, 09 Oct 2024 01:39:06 -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\tffacd0b85a97d-37d1691a713sm9794826f8f.42.2024.10.09.01.39.03\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 09 Oct 2024 01:39:04 -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=\"Whfgxe5H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1728463147;\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=VVRgMJfEmsJHs0Bb9d103kDFSyuyttocyQcZKx0jTYQ=;\n\tb=Whfgxe5H/pbGZJcHy7cwfd3+8t+wUCj9CQBCrrC/RprcHeyaFqfXseF8x4Ky40zCNRsNla\n\trI//+RfFEqQGY01YiuSm1qdEw/POdj/IKtWNAL4uujzvJVazBOVkDunJH+W3F/G+v17acI\n\tl+PM0dWjZoW/jWTJk8WuZKFz2og1oh0=","X-MC-Unique":"YBsePGa0McyO3dKovwnfuA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728463145; x=1729067945;\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=VVRgMJfEmsJHs0Bb9d103kDFSyuyttocyQcZKx0jTYQ=;\n\tb=YStCSvKkh/q2l+1n1GOJ6qr2nDLdvVRTqCa792Bg7PFY0k7pmRnNh/7Pa5JW/2TEhh\n\tGYp0r9oInC1DVMV4Sbw8vNaTL4II/K7RVM/21/+QLxm9THjMAo06xQkVSNKKAMApgmMB\n\t+UBvFoH6Zmv5ecC/BHyyEwKQwffrcqR2Sk3o77mIgPpkiACVc3VcECeqJMi9lw0d6DNF\n\t5kxq45MAngqK9GnUyKS07fpfBq9Ahzolha5Pi0yzRXHWkuMLh+NpjmJwkAo+NgfoV5eJ\n\tX/7Iifo+J5vMcXqponDfX97oN0lcExFR5Dn1WGnCta8RUxrjO9fv0/jxWs9bdZo5hXv5\n\tLMow==","X-Gm-Message-State":"AOJu0Yx8wtlf5iJBI6KJb0i+ERyK5/moE8M8Z7IaRhNmWPeRd/6IrXO6\n\t/W6TXW1fOsPkMRR0WI/0zlmETg9I5lD0L9wt8Hfuk0xUBgrInK+boq1zVMKXJv/WgiHFO8iPTqe\n\tXyckac6FLHPyIYAS1QsQ7RW2tp6QF5oc79SN1oH6HCS9Vx1mRHeNrIbWfkrPwsraR3+aOAcA=","X-Received":["by 2002:a5d:5745:0:b0:37c:d244:bdb1 with SMTP id\n\tffacd0b85a97d-37d3aa57b97mr857807f8f.26.1728463145075; \n\tWed, 09 Oct 2024 01:39:05 -0700 (PDT)","by 2002:a5d:5745:0:b0:37c:d244:bdb1 with SMTP id\n\tffacd0b85a97d-37d3aa57b97mr857794f8f.26.1728463144668; \n\tWed, 09 Oct 2024 01:39:04 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEFyc4L2CVXhG/Cb0KZtSmL7xEKxzJKeWGjc8dNn6qQhF0L/WGxM4jpRY9/KTv90WBYujuLbA==","From":"Milan Zamazal ","To":"libcamera-devel@lists.libcamera.org","Cc":"robert.mader@posteo.de, kieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","In-Reply-To":"<20241009083556.330325-1-mzamazal@redhat.com> (Milan Zamazal's\n\tmessage of \"Wed, 9 Oct 2024 10:35:56 +0200\")","References":"<20241009083556.330325-1-mzamazal@redhat.com>","Date":"Wed, 09 Oct 2024 10:39:02 +0200","Message-ID":"<87o73tr7q1.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":31637,"web_url":"https://patchwork.libcamera.org/comment/31637/","msgid":"<0bd96666-952a-47f9-8bec-d5a6e8652a26@collabora.com>","date":"2024-10-09T09:41:00","subject":"Re: [PATCH v2] 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":"Hi, thanks for the patch!\n\nOn 09.10.24 10:39, Milan Zamazal wrote:\n> Milan Zamazal writes:\n>\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.\n>>\n>> This patch fixes the omission. The request must be also canceled, which\n>> requires introducing a little PipelineHandler::cancelRequest helper in\n>> order to be able to access the private cancel() method.\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> Changes in v2:\n> - The request is additionally canceled.\n> - New helper method PipelineHandler::cancelRequest() introduced.\n>\n> Robert, could you please test v2?\n\nI gave it a quick go for 5 minutes - generally it seems to work great, \nhowever trying enough quick camera switching I still run into the assert.\n\nSo probably, on top of this, we'll need something implementing what \nKieran suggested:\n\n > (Though I think we should reject any incoming requests as soon as we \nhit stop())\n\n>> ---\n>> include/libcamera/internal/pipeline_handler.h | 1 +\n>> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n>> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n>> 3 files changed, 31 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>> index 0d3808036..fb28a18d0 100644\n>> --- a/include/libcamera/internal/pipeline_handler.h\n>> +++ b/include/libcamera/internal/pipeline_handler.h\n>> @@ -60,6 +60,7 @@ public:\n>> \n>> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n>> \tvoid completeRequest(Request *request);\n>> +\tvoid cancelRequest(Request *request);\n>> \n>> \tstd::string configurationFile(const std::string &subdir,\n>> \t\t\t\t const std::string &name) const;\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(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 +1420,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>> }\n>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>> index e59404691..c9cb11f0f 100644\n>> --- a/src/libcamera/pipeline_handler.cpp\n>> +++ b/src/libcamera/pipeline_handler.cpp\n>> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n>> \twhile (!waitingRequests_.empty()) {\n>> \t\tRequest *request = waitingRequests_.front();\n>> \t\twaitingRequests_.pop();\n>> -\n>> -\t\trequest->_d()->cancel();\n>> -\t\tcompleteRequest(request);\n>> +\t\tcancelRequest(request);\n>> \t}\n>> \n>> \t/* Make sure no requests are pending. */\n>> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>> \t}\n>> \n>> \tint ret = queueRequestDevice(camera, request);\n>> -\tif (ret) {\n>> -\t\trequest->_d()->cancel();\n>> -\t\tcompleteRequest(request);\n>> -\t}\n>> +\tif (ret)\n>> +\t\tcancelRequest(request);\n>> }\n>> \n>> /**\n>> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n>> \t}\n>> }\n>> \n>> +/**\n>> + * \\brief Cancel request and signal its completion\n>> + * \\param[in] request The request to cancel\n\nSmall typo here\n\n\n>> + *\n>> + * This function cancels the request in addition to its completion. The same\n>> + * rules as for completeRequest() apply.\n>> + */\n>> +void PipelineHandler::cancelRequest(Request *request)\n>> +{\n>> +\trequest->_d()->cancel();\n>> +\tcompleteRequest(request);\n>> +}\n>> +\n>> /**\n>> * \\brief Retrieve the absolute path to a platform configuration file\n>> * \\param[in] subdir The pipeline handler specific subdirectory name","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 BB72AC32E0\n\tfor ;\n\tWed, 9 Oct 2024 09:41:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 530096536D;\n\tWed, 9 Oct 2024 11:41:11 +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 AC5F76536A\n\tfor ;\n\tWed, 9 Oct 2024 11:41:08 +0200 (CEST)","by mx.zohomail.com with SMTPS id 1728466862915681.6622563720849;\n\tWed, 9 Oct 2024 02:41:02 -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=\"KgMfTSEm\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1728466865; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=MhssqxKWjaZXogxON070+7SZ6mjiGykRKRhXM9NYRNhBn+jk3dPY2jhxGFmQ/31c0ASkSwwYK1CcFgLKMz39jkuw3pljBr7IuK8DCUtHaMD85KRtq7bd3UCg0gL2Iq8rVHcU/aIRv6CBqqirT/iSFvUM1yBDY9TuksPbLqI1CYM=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1728466865;\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=EQLNk99WzeAK+7EutndvGJe4pW0BMvKI0zBK4YyC8jI=; \n\tb=ZGi0V3fHFMcRTCEIehPzuvBLbuMP9tkcRZBH5r7dlaC9UVYlda+n0cxMm/IWcLwsuZiOcCO5Ia0XdRd6N+mXBClzCrVy+Rarew0S9SW/R53C++BZcQ3b9EncqIfpt0mT62kaB877jjSuahAaSrQHAvUpnfUB7h/3XW4l1ZyO9UM=","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=1728466865;\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=EQLNk99WzeAK+7EutndvGJe4pW0BMvKI0zBK4YyC8jI=;\n\tb=KgMfTSEm2mbSiM9T1hSMfSLdEtN6ADcz8U3EHCodRg10GYhSQOBIC4UCmXIOWZ3t\n\t26u5kwbdkGjd7lVOkTCEGeFdjUGpRI2KGGPg4MMR5pkCG5MbPbYdTMi3oNeUzGUDzGb\n\tYwT1GoRsFDlwLk412UFQ6LpvbqU8yzyxGhxXxxLU=","Message-ID":"<0bd96666-952a-47f9-8bec-d5a6e8652a26@collabora.com>","Date":"Wed, 9 Oct 2024 11:41:00 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","To":"libcamera-devel@lists.libcamera.org","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<87o73tr7q1.fsf@redhat.com>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<87o73tr7q1.fsf@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":31640,"web_url":"https://patchwork.libcamera.org/comment/31640/","msgid":"<172847267964.3353069.508849088657641080@ping.linuxembedded.co.uk>","date":"2024-10-09T11:17:59","subject":"Re: [PATCH v2] 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-09 10:41:00)\n> Hi, thanks for the patch!\n> \n> On 09.10.24 10:39, Milan Zamazal wrote:\n> > Milan Zamazal writes:\n> >\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.\n> >>\n> >> This patch fixes the omission. The request must be also canceled, which\n> >> requires introducing a little PipelineHandler::cancelRequest helper in\n> >> order to be able to access the private cancel() method.\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> > Changes in v2:\n> > - The request is additionally canceled.\n> > - New helper method PipelineHandler::cancelRequest() introduced.\n> >\n> > Robert, could you please test v2?\n> \n> I gave it a quick go for 5 minutes - generally it seems to work great, \n> however trying enough quick camera switching I still run into the assert.\n> \n> So probably, on top of this, we'll need something implementing what \n> Kieran suggested:\n> \n> > (Though I think we should reject any incoming requests as soon as we \n> hit stop())\n\nSorry - there I was saying that should /already/ be happening. The state\nmachine should put the camera in to a stopping state where new incoming\nrequests will be rejected...\n\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1401\n\nUnless this is still somehow racy ;-(\n\n--\nKieran\n\n\n> \n> >> ---\n> >> include/libcamera/internal/pipeline_handler.h | 1 +\n> >> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n> >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> >> 3 files changed, 31 insertions(+), 7 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> >> index 0d3808036..fb28a18d0 100644\n> >> --- a/include/libcamera/internal/pipeline_handler.h\n> >> +++ b/include/libcamera/internal/pipeline_handler.h\n> >> @@ -60,6 +60,7 @@ public:\n> >> \n> >> bool completeBuffer(Request *request, FrameBuffer *buffer);\n> >> void completeRequest(Request *request);\n> >> + void cancelRequest(Request *request);\n> >> \n> >> std::string configurationFile(const std::string &subdir,\n> >> const std::string &name) const;\n> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(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 +1420,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> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> >> index e59404691..c9cb11f0f 100644\n> >> --- a/src/libcamera/pipeline_handler.cpp\n> >> +++ b/src/libcamera/pipeline_handler.cpp\n> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> >> while (!waitingRequests_.empty()) {\n> >> Request *request = waitingRequests_.front();\n> >> waitingRequests_.pop();\n> >> -\n> >> - request->_d()->cancel();\n> >> - completeRequest(request);\n> >> + cancelRequest(request);\n> >> }\n> >> \n> >> /* Make sure no requests are pending. */\n> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> >> }\n> >> \n> >> int ret = queueRequestDevice(camera, request);\n> >> - if (ret) {\n> >> - request->_d()->cancel();\n> >> - completeRequest(request);\n> >> - }\n> >> + if (ret)\n> >> + cancelRequest(request);\n> >> }\n> >> \n> >> /**\n> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> >> }\n> >> }\n> >> \n> >> +/**\n> >> + * \\brief Cancel request and signal its completion\n> >> + * \\param[in] request The request to cancel\n> \n> Small typo here\n> \n> \n> >> + *\n> >> + * This function cancels the request in addition to its completion. The same\n> >> + * rules as for completeRequest() apply.\n> >> + */\n> >> +void PipelineHandler::cancelRequest(Request *request)\n> >> +{\n> >> + request->_d()->cancel();\n> >> + completeRequest(request);\n> >> +}\n> >> +\n> >> /**\n> >> * \\brief Retrieve the absolute path to a platform configuration file\n> >> * \\param[in] subdir The pipeline handler specific subdirectory name\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 A84D0C32DE\n\tfor ;\n\tWed, 9 Oct 2024 11:18:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93AE6618C9;\n\tWed, 9 Oct 2024 13:18:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 73FFE618C5\n\tfor ;\n\tWed, 9 Oct 2024 13:18:02 +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 ED181670;\n\tWed, 9 Oct 2024 13:16:24 +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=\"SxmDTjfs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728472585;\n\tbh=dcWvsYj1GJsJwfdFOgZd79q693yB9yMPILKh2Snde28=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=SxmDTjfsmS1PUapVY+4v7rlAojCb+zXhK05Grd1N8XcMOJA064hHJwDeH6hGJV8MX\n\tWlE2vjdgxT7SIpws+sE8v5av27pmXP6HfL7tH1nEHTIJEInNUQ2vCmAHFeJM8I2WDp\n\tKzeQRmbrHD2S8IbTQU6GTeEPHEus9RDxFz6KiA9Y=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<0bd96666-952a-47f9-8bec-d5a6e8652a26@collabora.com>","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<87o73tr7q1.fsf@redhat.com>\n\t<0bd96666-952a-47f9-8bec-d5a6e8652a26@collabora.com>","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","From":"Kieran Bingham ","To":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 09 Oct 2024 12:17:59 +0100","Message-ID":"<172847267964.3353069.508849088657641080@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":31641,"web_url":"https://patchwork.libcamera.org/comment/31641/","msgid":"<172847450913.532453.16048433257603965868@ping.linuxembedded.co.uk>","date":"2024-10-09T11:48:29","subject":"Re: [PATCH v2] 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 09:35:56)\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.\n> \n> This patch fixes the omission. The request must be also canceled, which\n> requires introducing a little PipelineHandler::cancelRequest helper in\n> order to be able to access the private cancel() method.\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> include/libcamera/internal/pipeline_handler.h | 1 +\n> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> 3 files changed, 31 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 0d3808036..fb28a18d0 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -60,6 +60,7 @@ public:\n> \n> bool completeBuffer(Request *request, FrameBuffer *buffer);\n> void completeRequest(Request *request);\n> + void cancelRequest(Request *request);\n> \n> std::string configurationFile(const std::string &subdir,\n> const std::string &name) const;\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(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 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);\n> \n> data->conversionBuffers_.clear();\n> + data->clearIncompleteRequests();\n\nDo the requests have any existing reference to anything in teh\nconversionBuffers_ that would need to make sure we cancel/clear before\nreleasing the conversionBuffers_? \n\nProbably not - but just checking.\n\n> \n> releasePipeline(data);\n> }\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index e59404691..c9cb11f0f 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n\nSorry to be a pain on nitpicking - but the changes to pipeline_handler\nare core - not SoftISP and the changes below will function standalone,\nso this should be a separate patch.\n\nThen the softISP can use it!\n\nSomething like \"libcamera: pipeline_handler: Provide cancelRequest\"\n\nis definitely something I want to be visible in the changelogs, and\nclear that other pipeline handlers can/should use it.\n\nOnce split to two - I'd probably say this already goes in the right\ndirection to get merged even if there is possible still another race to\ninvestigate on top...\n\n--\nKieran\n\n> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> while (!waitingRequests_.empty()) {\n> Request *request = waitingRequests_.front();\n> waitingRequests_.pop();\n> -\n> - request->_d()->cancel();\n> - completeRequest(request);\n> + cancelRequest(request);\n> }\n> \n> /* Make sure no requests are pending. */\n> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> }\n> \n> int ret = queueRequestDevice(camera, request);\n> - if (ret) {\n> - request->_d()->cancel();\n> - completeRequest(request);\n> - }\n> + if (ret)\n> + cancelRequest(request);\n> }\n> \n> /**\n> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> }\n> }\n> \n> +/**\n> + * \\brief Cancel request and signal its completion\n> + * \\param[in] request The request to cancel\n> + *\n> + * This function cancels the request in addition to its completion. The same\n> + * rules as for completeRequest() apply.\n> + */\n> +void PipelineHandler::cancelRequest(Request *request)\n> +{\n> + request->_d()->cancel();\n> + completeRequest(request);\n> +}\n> +\n> /**\n> * \\brief Retrieve the absolute path to a platform configuration file\n> * \\param[in] subdir The pipeline handler specific subdirectory name\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 94F8CC32E0\n\tfor ;\n\tWed, 9 Oct 2024 11:48:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2C45618C5;\n\tWed, 9 Oct 2024 13:48:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E737618C5\n\tfor ;\n\tWed, 9 Oct 2024 13:48:32 +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 D38BF594;\n\tWed, 9 Oct 2024 13:46:54 +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=\"vYJgVMDq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728474414;\n\tbh=pu1vhaVM4qIrmQwJxnXMw5x/nUwxI3qFsl9Bug1GU7I=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=vYJgVMDqHDttCrvdUIOuJ//SXXiQkEKKz0d6LUflcNQs5vhd5psAix6O5OJiuf38E\n\tqjUL4G/DP+dRzq7t1KRwq4MBJkwRTyc2krcSuuAh2xUFxJIGFRzqpQM+25gKkbnqiM\n\tqjXsIUqRmX6V/YGjD0W4du7fohPQa09lGnMZG3xg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241009083556.330325-1-mzamazal@redhat.com>","References":"<20241009083556.330325-1-mzamazal@redhat.com>","Subject":"Re: [PATCH v2] 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":"Wed, 09 Oct 2024 12:48:29 +0100","Message-ID":"<172847450913.532453.16048433257603965868@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":31643,"web_url":"https://patchwork.libcamera.org/comment/31643/","msgid":"<87jzehqsmo.fsf@redhat.com>","date":"2024-10-09T14:05:03","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Robert,\n\nthank you for testing and review.\n\nRobert Mader writes:\n\n> Hi, thanks for the patch!\n>\n> On 09.10.24 10:39, Milan Zamazal wrote:\n>> Milan Zamazal writes:\n>>\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.\n>>>\n>>> This patch fixes the omission. The request must be also canceled, which\n>>> requires introducing a little PipelineHandler::cancelRequest helper in\n>>> order to be able to access the private cancel() method.\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>> Changes in v2:\n>> - The request is additionally canceled.\n>> - New helper method PipelineHandler::cancelRequest() introduced.\n>>\n>> Robert, could you please test v2?\n>\n> I gave it a quick go for 5 minutes - generally it seems to work great, however trying enough quick camera\n> switching I still run into the assert.\n>\n> So probably, on top of this, we'll need something implementing what Kieran suggested:\n>\n>> (Though I think we should reject any incoming requests as soon as we hit stop())\n>\n>>> ---\n>>> include/libcamera/internal/pipeline_handler.h | 1 +\n>>> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n>>> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n>>> 3 files changed, 31 insertions(+), 7 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>>> index 0d3808036..fb28a18d0 100644\n>>> --- a/include/libcamera/internal/pipeline_handler.h\n>>> +++ b/include/libcamera/internal/pipeline_handler.h\n>>> @@ -60,6 +60,7 @@ public:\n>>> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n>>> \tvoid completeRequest(Request *request);\n>>> +\tvoid cancelRequest(Request *request);\n>>> \tstd::string configurationFile(const std::string &subdir,\n>>> \t\t\t\t const std::string &name) const;\n>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>>> index 3ddce71d3..e862ef00f 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>>> \tstd::unique_ptr converter_;\n>>> \tstd::unique_ptr swIsp_;\n>>> @@ -897,6 +898,18 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n>>> \t\tpipe->completeRequest(request);\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()->cancelRequest(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 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n>>> \tvideo->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);\n>>> \tdata->conversionBuffers_.clear();\n>>> +\tdata->clearIncompleteRequests();\n>>> \treleasePipeline(data);\n>>> }\n>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>>> index e59404691..c9cb11f0f 100644\n>>> --- a/src/libcamera/pipeline_handler.cpp\n>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n>>> \twhile (!waitingRequests_.empty()) {\n>>> \t\tRequest *request = waitingRequests_.front();\n>>> \t\twaitingRequests_.pop();\n>>> -\n>>> -\t\trequest->_d()->cancel();\n>>> -\t\tcompleteRequest(request);\n>>> +\t\tcancelRequest(request);\n>>> \t}\n>>> \t/* Make sure no requests are pending. */\n>>> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>>> \t}\n>>> \tint ret = queueRequestDevice(camera, request);\n>>> -\tif (ret) {\n>>> -\t\trequest->_d()->cancel();\n>>> -\t\tcompleteRequest(request);\n>>> -\t}\n>>> +\tif (ret)\n>>> +\t\tcancelRequest(request);\n>>> }\n>>> /**\n>>> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n>>> \t}\n>>> }\n>>> +/**\n>>> + * \\brief Cancel request and signal its completion\n>>> + * \\param[in] request The request to cancel\n>\n> Small typo here\n\nSorry, I can't spot the typo, what typo exactly?\n\n>>> + *\n>>> + * This function cancels the request in addition to its completion. The same\n>>> + * rules as for completeRequest() apply.\n>>> + */\n>>> +void PipelineHandler::cancelRequest(Request *request)\n>>> +{\n>>> +\trequest->_d()->cancel();\n>>> +\tcompleteRequest(request);\n>>> +}\n>>> +\n>>> /**\n>>> * \\brief Retrieve the absolute path to a platform configuration file\n>>> * \\param[in] subdir The pipeline handler specific subdirectory name","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 92314C32DE\n\tfor ;\n\tWed, 9 Oct 2024 14:05:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97C3463538;\n\tWed, 9 Oct 2024 16:05:11 +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 EBC1A618C5\n\tfor ;\n\tWed, 9 Oct 2024 16:05:09 +0200 (CEST)","from mail-lj1-f199.google.com (mail-lj1-f199.google.com\n\t[209.85.208.199]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-151-FJVfth4xPTaef49y_8MyvA-1; Wed, 09 Oct 2024 10:05:07 -0400","by mail-lj1-f199.google.com with SMTP id\n\t38308e7fff4ca-2fac9702605so36407891fa.2\n\tfor ;\n\tWed, 09 Oct 2024 07:05:06 -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\t5b1f17b1804b1-430d748f0a5sm21228995e9.48.2024.10.09.07.05.03\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 09 Oct 2024 07:05:03 -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=\"BVPZINUh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1728482708;\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=qU5skLqmy8YDD42F/vHgzMNwIbO5wMmcn2WRDCk29z0=;\n\tb=BVPZINUhb24spPoqsTY5tLcyTTcby0K/69IdYYRCLvr0+EmuMQQbl96MtO+iSUZXLRbfHB\n\t9quybEpnjXIZInRJ4LynRzzxVY1W05PUQ6AJNaHnrTVkgR28sz1Ju291CRym/yswtQEV/g\n\tV6ZWctsVoE9f+slxzfxIOhFwH5rBmZM=","X-MC-Unique":"FJVfth4xPTaef49y_8MyvA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728482705; x=1729087505;\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=qU5skLqmy8YDD42F/vHgzMNwIbO5wMmcn2WRDCk29z0=;\n\tb=Iw1MWQMpB1Me+O63nSfh2F2OQH6ZNqjxYO+bUDbTefZuvoCWh3Ah2rzPfvBp0K+Go7\n\td8QTXLDieE24JTJqY4h7jybQMIxJnQr0U8owbsBn2egB9OilV20A8KWM/T3j96R763KM\n\tFyjKSjUpjVDEwj4DY+NNGNqCB85uFuFbdIWLHz4sn0f057l+fhmrXW42AaCBcIUGwY9I\n\t9QgSBC9dL55KUbTMjlKtV3CEO0mqk/IXnROtu5xV16pqa/IIs+an517ga62wriXdrfzf\n\thQny8tg2nkXBR1RJCVGTnEqlDmoLxZwj0LKSgUSABKUPes6aLImFncPF0jfYvBhMjJ3h\n\tCQiQ==","X-Gm-Message-State":"AOJu0YyC3WXyH2zonPyt4PC5njl7wGJgMyXmg6kySDziJWJ2k2NoVGI2\n\tqEq6mx2znsT3/GBS2BSGSKpkbv/DOzwlLLqEpF4yAd6jWu399GfjjKcqqcEnXR6tQbxFUdruN6B\n\tVyPUHiQAHYM+QYCGiup58Rr7du1Y5BLEg6dhUutwlZnCL4AKi2ZGqz5OuEtBHfy1bXgZTd2zseK\n\tEPaIeeikrUuADDv0y5u/oOELwGgXrJpdEOrWp2/BuGM7Wn/MCWzswPpuw=","X-Received":["by 2002:a05:6512:39cb:b0:536:7a88:616b with SMTP id\n\t2adb3069b0e04-539c48d65famr1537703e87.26.1728482704970; \n\tWed, 09 Oct 2024 07:05:04 -0700 (PDT)","by 2002:a05:6512:39cb:b0:536:7a88:616b with SMTP id\n\t2adb3069b0e04-539c48d65famr1537658e87.26.1728482704448; \n\tWed, 09 Oct 2024 07:05:04 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHnf5Y0E5UAO2D9xu2TC9qP0ESL4BnWnEq2/T6qGJ+JWK94god6EvrHgsf/Y9w2dnb+4O6BTA==","From":"Milan Zamazal ","To":"Robert Mader ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","In-Reply-To":"<0bd96666-952a-47f9-8bec-d5a6e8652a26@collabora.com> (Robert\n\tMader's message of \"Wed, 9 Oct 2024 11:41:00 +0200\")","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<87o73tr7q1.fsf@redhat.com>\n\t<0bd96666-952a-47f9-8bec-d5a6e8652a26@collabora.com>","Date":"Wed, 09 Oct 2024 16:05:03 +0200","Message-ID":"<87jzehqsmo.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":31645,"web_url":"https://patchwork.libcamera.org/comment/31645/","msgid":"<397b3a0b-6e5c-4829-977c-df2919624c9d@collabora.com>","date":"2024-10-09T14:08:56","subject":"Re: [PATCH v2] 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":"On 09.10.24 16:05, Milan Zamazal wrote:\n> Hi Robert,\n>\n> thank you for testing and review.\n>\n> Robert Mader writes:\n>\n>> Hi, thanks for the patch!\n>>\n>> On 09.10.24 10:39, Milan Zamazal wrote:\n>>> Milan Zamazal writes:\n>>>\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.\n>>>>\n>>>> This patch fixes the omission. The request must be also canceled, which\n>>>> requires introducing a little PipelineHandler::cancelRequest helper in\n>>>> order to be able to access the private cancel() method.\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>>> Changes in v2:\n>>> - The request is additionally canceled.\n>>> - New helper method PipelineHandler::cancelRequest() introduced.\n>>>\n>>> Robert, could you please test v2?\n>> I gave it a quick go for 5 minutes - generally it seems to work great, however trying enough quick camera\n>> switching I still run into the assert.\n>>\n>> So probably, on top of this, we'll need something implementing what Kieran suggested:\n>>\n>>> (Though I think we should reject any incoming requests as soon as we hit stop())\n>>>> ---\n>>>> include/libcamera/internal/pipeline_handler.h | 1 +\n>>>> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n>>>> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n>>>> 3 files changed, 31 insertions(+), 7 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>>>> index 0d3808036..fb28a18d0 100644\n>>>> --- a/include/libcamera/internal/pipeline_handler.h\n>>>> +++ b/include/libcamera/internal/pipeline_handler.h\n>>>> @@ -60,6 +60,7 @@ public:\n>>>> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n>>>> \tvoid completeRequest(Request *request);\n>>>> +\tvoid cancelRequest(Request *request);\n>>>> \tstd::string configurationFile(const std::string &subdir,\n>>>> \t\t\t\t const std::string &name) const;\n>>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>>>> index 3ddce71d3..e862ef00f 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>>>> \tstd::unique_ptr converter_;\n>>>> \tstd::unique_ptr swIsp_;\n>>>> @@ -897,6 +898,18 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n>>>> \t\tpipe->completeRequest(request);\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()->cancelRequest(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 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n>>>> \tvideo->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);\n>>>> \tdata->conversionBuffers_.clear();\n>>>> +\tdata->clearIncompleteRequests();\n>>>> \treleasePipeline(data);\n>>>> }\n>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>>>> index e59404691..c9cb11f0f 100644\n>>>> --- a/src/libcamera/pipeline_handler.cpp\n>>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>>> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n>>>> \twhile (!waitingRequests_.empty()) {\n>>>> \t\tRequest *request = waitingRequests_.front();\n>>>> \t\twaitingRequests_.pop();\n>>>> -\n>>>> -\t\trequest->_d()->cancel();\n>>>> -\t\tcompleteRequest(request);\n>>>> +\t\tcancelRequest(request);\n>>>> \t}\n>>>> \t/* Make sure no requests are pending. */\n>>>> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>>>> \t}\n>>>> \tint ret = queueRequestDevice(camera, request);\n>>>> -\tif (ret) {\n>>>> -\t\trequest->_d()->cancel();\n>>>> -\t\tcompleteRequest(request);\n>>>> -\t}\n>>>> +\tif (ret)\n>>>> +\t\tcancelRequest(request);\n>>>> }\n>>>> /**\n>>>> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n>>>> \t}\n>>>> }\n>>>> +/**\n>>>> + * \\brief Cancel request and signal its completion\n>>>> + * \\param[in] request The request to cancel\n>> Small typo here\n> Sorry, I can't spot the typo, what typo exactly?\n\nUrgh, never mind, I though the \"request The request..\" part was a typo - \nbut that's the arg name 🤦\n\n>\n>>>> + *\n>>>> + * This function cancels the request in addition to its completion. The same\n>>>> + * rules as for completeRequest() apply.\n>>>> + */\n>>>> +void PipelineHandler::cancelRequest(Request *request)\n>>>> +{\n>>>> +\trequest->_d()->cancel();\n>>>> +\tcompleteRequest(request);\n>>>> +}\n>>>> +\n>>>> /**\n>>>> * \\brief Retrieve the absolute path to a platform configuration file\n>>>> * \\param[in] subdir The pipeline handler specific subdirectory name","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 6B2B2C32E0\n\tfor ;\n\tWed, 9 Oct 2024 14:09:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67C396536C;\n\tWed, 9 Oct 2024 16:09:08 +0200 (CEST)","from sender4-pp-f112.zoho.com (sender4-pp-f112.zoho.com\n\t[136.143.188.112])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DEE79618C9\n\tfor ;\n\tWed, 9 Oct 2024 16:09:06 +0200 (CEST)","by mx.zohomail.com with SMTPS id 1728482939253918.2820806919975;\n\tWed, 9 Oct 2024 07:08:59 -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=\"Nnwo4yZH\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1728482940; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=Cl6LVNwFyb7H2g8p5XaslABFxZUygZAn7xw2DXOK00NcgFj4AGqRzXREbxkZVYfFKt7s4psKR4xVIL7ssp0HJEHEjAVG/BMICkY66yDL36esE8t/LCCsZI4YCt0nkcsNuRhVbVAOvdhfDsRRJSTBKyoJfN/lLFzF/BGFos2of34=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1728482940;\n\th=Content-Type:Content-Transfer-Encoding:Cc:Cc:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To;\n\tbh=6vSqOwd7fCYjV/di/T2VNdh/V7q0bCRAceZGJd1wUUc=; \n\tb=B2mjYJK1J7LWq8kMkS7rrLoV5mp3PWJ8NHRz2V1bmKqPQRBLxM7lmimmE19pzL24OyBUIhITNLoNvBNIzmJLhfeGrs0bND8Ek/BWMztiZXr2S3VWZX1lqgvCvz2/VvdVEVfG891housuOCjT+khCnEvQwt/EDUd5jo2wBo3xmBk=","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=1728482940;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:Cc:Cc:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To;\n\tbh=6vSqOwd7fCYjV/di/T2VNdh/V7q0bCRAceZGJd1wUUc=;\n\tb=Nnwo4yZHaPmTgEjZDxX95qtuOlFbbMZ3tNgaHl1Kbi29/9/u9sBcScmsj3aJoERf\n\tUjCr2vcABtiA3xaaTy7c/RTAHd8oGLvSZ6eGhlxCDWYg2YXz38q4eLwZA23vmm8UDqd\n\tf4U3Lrvyr8jbAb6b00lG4C3wWF/UgqkcYkgudeQs=","Message-ID":"<397b3a0b-6e5c-4829-977c-df2919624c9d@collabora.com>","Date":"Wed, 9 Oct 2024 16:08:56 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<87o73tr7q1.fsf@redhat.com>\n\t<0bd96666-952a-47f9-8bec-d5a6e8652a26@collabora.com>\n\t<87jzehqsmo.fsf@redhat.com>","Content-Language":"en-US, de-DE","From":"Robert Mader ","In-Reply-To":"<87jzehqsmo.fsf@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":31646,"web_url":"https://patchwork.libcamera.org/comment/31646/","msgid":"<20241009142037.GE2733@pendragon.ideasonboard.com>","date":"2024-10-09T14:20:37","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 09, 2024 at 10:35:56AM +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.\n> \n> This patch fixes the omission. The request must be also canceled, which\n> requires introducing a little PipelineHandler::cancelRequest helper in\n> order to be able to access the private cancel() method.\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> include/libcamera/internal/pipeline_handler.h | 1 +\n> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> 3 files changed, 31 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 0d3808036..fb28a18d0 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -60,6 +60,7 @@ public:\n> \n> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> \tvoid completeRequest(Request *request);\n> +\tvoid cancelRequest(Request *request);\n> \n> \tstd::string configurationFile(const std::string &subdir,\n> \t\t\t\t const std::string &name) const;\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);\n\nAren't you cancelling the same request multiple times ?\n\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 +1420,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> }\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index e59404691..c9cb11f0f 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> \twhile (!waitingRequests_.empty()) {\n> \t\tRequest *request = waitingRequests_.front();\n> \t\twaitingRequests_.pop();\n> -\n> -\t\trequest->_d()->cancel();\n> -\t\tcompleteRequest(request);\n> +\t\tcancelRequest(request);\n> \t}\n> \n> \t/* Make sure no requests are pending. */\n> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> \t}\n> \n> \tint ret = queueRequestDevice(camera, request);\n> -\tif (ret) {\n> -\t\trequest->_d()->cancel();\n> -\t\tcompleteRequest(request);\n> -\t}\n> +\tif (ret)\n> +\t\tcancelRequest(request);\n> }\n> \n> /**\n> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> \t}\n> }\n> \n> +/**\n> + * \\brief Cancel request and signal its completion\n> + * \\param[in] request The request to cancel\n> + *\n> + * This function cancels the request in addition to its completion. The same\n> + * rules as for completeRequest() apply.\n> + */\n> +void PipelineHandler::cancelRequest(Request *request)\n> +{\n> +\trequest->_d()->cancel();\n> +\tcompleteRequest(request);\n> +}\n> +\n> /**\n> * \\brief Retrieve the absolute path to a platform configuration file\n> * \\param[in] subdir The pipeline handler specific subdirectory name","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 40241C32DE\n\tfor ;\n\tWed, 9 Oct 2024 14:20:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32A6363538;\n\tWed, 9 Oct 2024 16:20:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 311B4618C5\n\tfor ;\n\tWed, 9 Oct 2024 16:20:42 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.3])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 20C17670;\n\tWed, 9 Oct 2024 16:19:04 +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=\"ntEWOQiu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728483544;\n\tbh=lPS0AWj5rF5NuF3rTm3MbpZqfgcj0kHDDoJe38VNa/k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ntEWOQiuqieSwcTPOaKUb9EcKcZS9kVj22NTWftMV3hSqv+oSj1kn43Y2RtObEuvh\n\tRovrwzFU/QEml0EqpPKKpqgCR8hbyRrXrWNKegSe/n7rBNTf5kq9uZDRLVGG8yO0c4\n\tuSKdTgF8tl08WvXqJRgnViq/HQah2LWrIyckv+00=","Date":"Wed, 9 Oct 2024 17:20:37 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","Message-ID":"<20241009142037.GE2733@pendragon.ideasonboard.com>","References":"<20241009083556.330325-1-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241009083556.330325-1-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\" "}},{"id":31651,"web_url":"https://patchwork.libcamera.org/comment/31651/","msgid":"<87ed4pqpnw.fsf@redhat.com>","date":"2024-10-09T15:09:07","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Kieran Bingham writes:\n\n> Quoting Milan Zamazal (2024-10-09 09:35:56)\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.\n>> \n>> This patch fixes the omission. The request must be also canceled, which\n>> requires introducing a little PipelineHandler::cancelRequest helper in\n>> order to be able to access the private cancel() method.\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>> include/libcamera/internal/pipeline_handler.h | 1 +\n>> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n>> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n>> 3 files changed, 31 insertions(+), 7 deletions(-)\n>> \n>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>> index 0d3808036..fb28a18d0 100644\n>> --- a/include/libcamera/internal/pipeline_handler.h\n>> +++ b/include/libcamera/internal/pipeline_handler.h\n>> @@ -60,6 +60,7 @@ public:\n>> \n>> bool completeBuffer(Request *request, FrameBuffer *buffer);\n>> void completeRequest(Request *request);\n>> + void cancelRequest(Request *request);\n>> \n>> std::string configurationFile(const std::string &subdir,\n>> const std::string &name) const;\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(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 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n>> video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);\n>> \n>> data->conversionBuffers_.clear();\n>> + data->clearIncompleteRequests();\n>\n> Do the requests have any existing reference to anything in teh\n> conversionBuffers_ that would need to make sure we cancel/clear before\n> releasing the conversionBuffers_? \n\nI don't think so -- conversionBuffers_ contains buffers obtained from\nV4L2VideoDevice while clearIncompleteRequests examines conversionQueue_,\nwhich contains buffers from requests. I.e. they are input x output\nbuffers if I understand the things correctly.\n\nI can swap the two lines to avoid any doubts or future mistakes.\n\n> Probably not - but just checking.\n>\n>> \n>> releasePipeline(data);\n>> }\n>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>> index e59404691..c9cb11f0f 100644\n>> --- a/src/libcamera/pipeline_handler.cpp\n>> +++ b/src/libcamera/pipeline_handler.cpp\n>\n> Sorry to be a pain on nitpicking - but the changes to pipeline_handler\n> are core - not SoftISP and the changes below will function standalone,\n> so this should be a separate patch.\n>\n> Then the softISP can use it!\n>\n> Something like \"libcamera: pipeline_handler: Provide cancelRequest\"\n>\n> is definitely something I want to be visible in the changelogs, and\n> clear that other pipeline handlers can/should use it.\n\nYes, I'll split it.\n\n> Once split to two - I'd probably say this already goes in the right\n> direction to get merged even if there is possible still another race to\n> investigate on top...\n\nGood deal. :-)\n\n> --\n> Kieran\n>\n>> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n>> while (!waitingRequests_.empty()) {\n>> Request *request = waitingRequests_.front();\n>> waitingRequests_.pop();\n>> -\n>> - request->_d()->cancel();\n>> - completeRequest(request);\n>> + cancelRequest(request);\n>> }\n>> \n>> /* Make sure no requests are pending. */\n>> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>> }\n>> \n>> int ret = queueRequestDevice(camera, request);\n>> - if (ret) {\n>> - request->_d()->cancel();\n>> - completeRequest(request);\n>> - }\n>> + if (ret)\n>> + cancelRequest(request);\n>> }\n>> \n>> /**\n>> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n>> }\n>> }\n>> \n>> +/**\n>> + * \\brief Cancel request and signal its completion\n>> + * \\param[in] request The request to cancel\n>> + *\n>> + * This function cancels the request in addition to its completion. The same\n>> + * rules as for completeRequest() apply.\n>> + */\n>> +void PipelineHandler::cancelRequest(Request *request)\n>> +{\n>> + request->_d()->cancel();\n>> + completeRequest(request);\n>> +}\n>> +\n>> /**\n>> * \\brief Retrieve the absolute path to a platform configuration file\n>> * \\param[in] subdir The pipeline handler specific subdirectory name\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 B29E2C32DE\n\tfor ;\n\tWed, 9 Oct 2024 15:09:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E2F876536B;\n\tWed, 9 Oct 2024 17:09:16 +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 28660618C9\n\tfor ;\n\tWed, 9 Oct 2024 17:09:15 +0200 (CEST)","from mail-wm1-f70.google.com (mail-wm1-f70.google.com\n\t[209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-493-DPOdV1r_NCiHdULlx8w3ug-1; Wed, 09 Oct 2024 11:09:10 -0400","by mail-wm1-f70.google.com with SMTP id\n\t5b1f17b1804b1-42f8ceb8783so35111415e9.1\n\tfor ;\n\tWed, 09 Oct 2024 08:09:10 -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\t5b1f17b1804b1-430ccf4f8desm22937045e9.13.2024.10.09.08.09.07\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 09 Oct 2024 08:09: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=\"UAi+RlwE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1728486554;\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=HgIt+1IVb6AG2/nWnZ9sKC5Axpv8wQcP+HOZTIKO8Ug=;\n\tb=UAi+RlwEVbgFaDtFA4676zrhaFGRW0axpsQ+TLff41XpnlIlgk2htCANa8sKmh3l166zC2\n\tsy5/Vbzoc2GqRXPiaJSsOBGuUvKka27mIsDBZA3SXkRE187k1gq9o7daO+Vl2lb+TpDj3o\n\tj5RT2WA6cj3iEo3BiQFqutVOrdd0Y74=","X-MC-Unique":"DPOdV1r_NCiHdULlx8w3ug-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728486549; x=1729091349;\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=HgIt+1IVb6AG2/nWnZ9sKC5Axpv8wQcP+HOZTIKO8Ug=;\n\tb=fpWkWq6H4PHFcMzin+IVsH54XCEqsM8jqZKaAnOWQX8u/qFgxlZ+fBfUHDt0FyN9fS\n\t8odHp3Gyx8q9ZJb3HAxO95eMb7nVwXFR6wEjRIYxXOgPzxDkoH5u9Me6EnJNaYX8ktxu\n\tNhxKebpJSFeXX3rvNiyjT/qNFg1TGG1bz9RnWhAhuYuiwc1zsD7tSfjX+iGc1bonZWKp\n\taFFFUXwdQNWF3gZMh61amJOy7bnpyuaqhn69FjZcqJavCqXhuMHgz6MoRTGA1mmUTNIc\n\tkr0HAYvYgUt7/vj2h1LLWH3Vwb0LT4ktY4nNKc6D0hNIUQuNsjsUwaaeeHiq+AnMTXit\n\tiLkw==","X-Gm-Message-State":"AOJu0Yxd8zwXeUiP0lpMA9jjHM2FcTelJnkbOvEZdBK7+1Flde6irdzw\n\tWLlAsa8r6CzdDUGp5L68hof0sOryGjLJa2j4QecxfKbw9rwbE9wmIdej7mMGesRoHdoMWjbJjME\n\tTnVoGsElvxMxB0QMs5ZXxROxPYoQUkJhLLwcVQVtrpuFhEoqXwI7qUbDVop4ia8T6QKOolmqm2J\n\tA6C8s=","X-Received":["by 2002:a05:600c:284:b0:42f:8d36:855e with SMTP id\n\t5b1f17b1804b1-431157a3bc5mr2391285e9.5.1728486549016; \n\tWed, 09 Oct 2024 08:09:09 -0700 (PDT)","by 2002:a05:600c:284:b0:42f:8d36:855e with SMTP id\n\t5b1f17b1804b1-431157a3bc5mr2391025e9.5.1728486548593; \n\tWed, 09 Oct 2024 08:09:08 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFzcZv+qJYg3ymb9EhIGK6ArniIHgDPrsntTS2eKeh4IyCUpHwgpQSb0MuYob+2rwHB6XCeiA==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","In-Reply-To":"<172847450913.532453.16048433257603965868@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Wed, 09 Oct 2024 12:48:29 +0100\")","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<172847450913.532453.16048433257603965868@ping.linuxembedded.co.uk>","Date":"Wed, 09 Oct 2024 17:09:07 +0200","Message-ID":"<87ed4pqpnw.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":31665,"web_url":"https://patchwork.libcamera.org/comment/31665/","msgid":"<87a5fdqkx3.fsf@redhat.com>","date":"2024-10-09T16:51:36","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> On Wed, Oct 09, 2024 at 10:35:56AM +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>\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.\n>> \n>> This patch fixes the omission. The request must be also canceled, which\n>> requires introducing a little PipelineHandler::cancelRequest helper in\n>> order to be able to access the private cancel() method.\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>> include/libcamera/internal/pipeline_handler.h | 1 +\n>> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n>> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n>> 3 files changed, 31 insertions(+), 7 deletions(-)\n>> \n>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>> index 0d3808036..fb28a18d0 100644\n>> --- a/include/libcamera/internal/pipeline_handler.h\n>> +++ b/include/libcamera/internal/pipeline_handler.h\n>> @@ -60,6 +60,7 @@ public:\n>> \n>> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n>> \tvoid completeRequest(Request *request);\n>> +\tvoid cancelRequest(Request *request);\n>> \n>> \tstd::string configurationFile(const std::string &subdir,\n>> \t\t\t\t const std::string &name) const;\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);\n>\n> Aren't you cancelling the same request multiple times ?\n\nPossibly yes. I'll add a check for RequestPending status.\n\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 +1420,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>> }\n>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>> index e59404691..c9cb11f0f 100644\n>> --- a/src/libcamera/pipeline_handler.cpp\n>> +++ b/src/libcamera/pipeline_handler.cpp\n>> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n>> \twhile (!waitingRequests_.empty()) {\n>> \t\tRequest *request = waitingRequests_.front();\n>> \t\twaitingRequests_.pop();\n>> -\n>> -\t\trequest->_d()->cancel();\n>> -\t\tcompleteRequest(request);\n>> +\t\tcancelRequest(request);\n>> \t}\n>> \n>> \t/* Make sure no requests are pending. */\n>> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>> \t}\n>> \n>> \tint ret = queueRequestDevice(camera, request);\n>> -\tif (ret) {\n>> -\t\trequest->_d()->cancel();\n>> -\t\tcompleteRequest(request);\n>> -\t}\n>> +\tif (ret)\n>> +\t\tcancelRequest(request);\n>> }\n>> \n>> /**\n>> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n>> \t}\n>> }\n>> \n>> +/**\n>> + * \\brief Cancel request and signal its completion\n>> + * \\param[in] request The request to cancel\n>> + *\n>> + * This function cancels the request in addition to its completion. The same\n>> + * rules as for completeRequest() apply.\n>> + */\n>> +void PipelineHandler::cancelRequest(Request *request)\n>> +{\n>> +\trequest->_d()->cancel();\n>> +\tcompleteRequest(request);\n>> +}\n>> +\n>> /**\n>> * \\brief Retrieve the absolute path to a platform configuration file\n>> * \\param[in] subdir The pipeline handler specific subdirectory name","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 7432CC32E0\n\tfor ;\n\tWed, 9 Oct 2024 16:51:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 78F6A63538;\n\tWed, 9 Oct 2024 18:51:42 +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 5F244618C5\n\tfor ;\n\tWed, 9 Oct 2024 18:51:41 +0200 (CEST)","from mail-ej1-f72.google.com (mail-ej1-f72.google.com\n\t[209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-645-bQMaYGD6PRq5na7Q2YnoNg-1; Wed, 09 Oct 2024 12:51:39 -0400","by mail-ej1-f72.google.com with SMTP id\n\ta640c23a62f3a-a7d2d414949so2212766b.0\n\tfor ;\n\tWed, 09 Oct 2024 09:51:39 -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-a99594a1e57sm406123866b.116.2024.10.09.09.51.36\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 09 Oct 2024 09:51:36 -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=\"VgoDHmec\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1728492700;\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=RzaTID2ift7BHksX6JD1UHE4YgGvinQqXlq7SDUJCg8=;\n\tb=VgoDHmecR2Y9bZamk/H+Eg2IdVa4tVbFLcrrNpt/l840KGhWCsAQzMcm4ZahE3+b6htamP\n\tr/4BOnOBwD2CGVaHCIYzclbHZiDQdA6sW+8E4BuBdhBEHgt5qPT1QV6QAxczWcljDqlp2l\n\th0RNoTO36WiZQ0I4S0dTeSVL518I5ec=","X-MC-Unique":"bQMaYGD6PRq5na7Q2YnoNg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728492698; x=1729097498;\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=RzaTID2ift7BHksX6JD1UHE4YgGvinQqXlq7SDUJCg8=;\n\tb=if4PT7b95o0+E2oa6kWVzDVI7vTRqDGc9PcYpA2/SKDGykcLUK5M6y0nl/Vb1PsKJM\n\ti6T30+CKnicIRotUDGeHgN2O2KFXys7tPCv/h9xfhyZNeFzvlxoGmXEgHafc6v8GZwqr\n\tuCRjqCbK8rdo0m/QvszbohU6gYV+ELvhzICEyrxPZp8qkxfZxZRjeQg3u3shEBRVYOrj\n\tRel1H+nDnaf2qvXxTThEFisx5fcJ7OOcQXfttojPGhn99MmKGEIAHVYEY97w4L/0gTZj\n\ttMRSDhCdr90tgJbvoPWR/AJuwB4ilPaCCnBZ1AOwA9cNDsHZnDWVRFamB+tpdfRzxToI\n\tmrlg==","X-Gm-Message-State":"AOJu0YxHG5cyW+FxGJN2m7qbKm8rP0P7kKP2xe7kch1KWiOmoeSaiSyF\n\tBVCm/gIu0lxt8JOX831vQDvC4eJywk2QoVD8gvU8k8tB3zIm6GcOkdaRuGu8yd0iw852ezszArN\n\tb8DESSZnZaPmtNUMnBh3PWF3l3BtEbPiEeD9ZXmxPLQ9kiQqa6q0TlGyS4yqZtmGq4UWaUiQ=","X-Received":["by 2002:a17:907:7206:b0:a99:509b:f524 with SMTP id\n\ta640c23a62f3a-a998d3496d3mr260950966b.57.1728492697973; \n\tWed, 09 Oct 2024 09:51:37 -0700 (PDT)","by 2002:a17:907:7206:b0:a99:509b:f524 with SMTP id\n\ta640c23a62f3a-a998d3496d3mr260948466b.57.1728492697488; \n\tWed, 09 Oct 2024 09:51:37 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IH31mOBkevajpl5UBxk0kbiCFPZ42EckGtf5ptAi1YYsi4GpdECZFfxcmWttKxQD57X1f0Q1w==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","In-Reply-To":"<20241009142037.GE2733@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 9 Oct 2024 17:20:37 +0300\")","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<20241009142037.GE2733@pendragon.ideasonboard.com>","Date":"Wed, 09 Oct 2024 18:51:36 +0200","Message-ID":"<87a5fdqkx3.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":31666,"web_url":"https://patchwork.libcamera.org/comment/31666/","msgid":"<20241009191955.GE16232@pendragon.ideasonboard.com>","date":"2024-10-09T19:19:55","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Wed, Oct 09, 2024 at 10:35:56AM +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> >\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.\n> >> \n> >> This patch fixes the omission. The request must be also canceled, which\n> >> requires introducing a little PipelineHandler::cancelRequest helper in\n> >> order to be able to access the private cancel() method.\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> >> include/libcamera/internal/pipeline_handler.h | 1 +\n> >> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n> >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> >> 3 files changed, 31 insertions(+), 7 deletions(-)\n> >> \n> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> >> index 0d3808036..fb28a18d0 100644\n> >> --- a/include/libcamera/internal/pipeline_handler.h\n> >> +++ b/include/libcamera/internal/pipeline_handler.h\n> >> @@ -60,6 +60,7 @@ public:\n> >> \n> >> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> >> \tvoid completeRequest(Request *request);\n> >> +\tvoid cancelRequest(Request *request);\n> >> \n> >> \tstd::string configurationFile(const std::string &subdir,\n> >> \t\t\t\t const std::string &name) const;\n> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);\n> >\n> > Aren't you cancelling the same request multiple times ?\n> \n> Possibly yes. I'll add a check for RequestPending status.\n\nHow about modifying conversionQueue_ to store instances of a structure\nthat contain a Request pointer in addition to the std::map ?\n\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 +1420,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> >> }\n> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> >> index e59404691..c9cb11f0f 100644\n> >> --- a/src/libcamera/pipeline_handler.cpp\n> >> +++ b/src/libcamera/pipeline_handler.cpp\n> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> >> \twhile (!waitingRequests_.empty()) {\n> >> \t\tRequest *request = waitingRequests_.front();\n> >> \t\twaitingRequests_.pop();\n> >> -\n> >> -\t\trequest->_d()->cancel();\n> >> -\t\tcompleteRequest(request);\n> >> +\t\tcancelRequest(request);\n> >> \t}\n> >> \n> >> \t/* Make sure no requests are pending. */\n> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> >> \t}\n> >> \n> >> \tint ret = queueRequestDevice(camera, request);\n> >> -\tif (ret) {\n> >> -\t\trequest->_d()->cancel();\n> >> -\t\tcompleteRequest(request);\n> >> -\t}\n> >> +\tif (ret)\n> >> +\t\tcancelRequest(request);\n> >> }\n> >> \n> >> /**\n> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> >> \t}\n> >> }\n> >> \n> >> +/**\n> >> + * \\brief Cancel request and signal its completion\n> >> + * \\param[in] request The request to cancel\n> >> + *\n> >> + * This function cancels the request in addition to its completion. The same\n> >> + * rules as for completeRequest() apply.\n> >> + */\n> >> +void PipelineHandler::cancelRequest(Request *request)\n> >> +{\n> >> +\trequest->_d()->cancel();\n> >> +\tcompleteRequest(request);\n> >> +}\n> >> +\n> >> /**\n> >> * \\brief Retrieve the absolute path to a platform configuration file\n> >> * \\param[in] subdir The pipeline handler specific subdirectory name\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 90468C32E0\n\tfor ;\n\tWed, 9 Oct 2024 19:20:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3DDAF6536C;\n\tWed, 9 Oct 2024 21:20:01 +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 49C0C63527\n\tfor ;\n\tWed, 9 Oct 2024 21:20:00 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.3])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6608E670;\n\tWed, 9 Oct 2024 21:18:22 +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=\"UTfvn4U1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728501502;\n\tbh=7on49klq0DTt3nI6xYVv8ys1wwjhc26Q+iJ0VrzUD1E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UTfvn4U17PCuUYB8OekcwyjG5jMZkpbFz21xE4BFuoaK7l6h0jkqIsXVQAimXkSPt\n\tKjQmR4eaGqGR09DnHZwr210HhWOpnrTCMvrUvyiSHXLd168bm8VJCOZ+5ttuaaO7WP\n\tXOGCJQpfaaWSWqvkg9lTUQE7/JYXURKl7usFg08o=","Date":"Wed, 9 Oct 2024 22:19:55 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","Message-ID":"<20241009191955.GE16232@pendragon.ideasonboard.com>","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<20241009142037.GE2733@pendragon.ideasonboard.com>\n\t<87a5fdqkx3.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87a5fdqkx3.fsf@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\" "}},{"id":31667,"web_url":"https://patchwork.libcamera.org/comment/31667/","msgid":"<875xq1qcy9.fsf@redhat.com>","date":"2024-10-09T19:43:42","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:\n>> Laurent Pinchart writes:\n>> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote:\n>\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.\n>> >> \n>> >> This patch fixes the omission. The request must be also canceled, which\n>> >> requires introducing a little PipelineHandler::cancelRequest helper in\n>> >> order to be able to access the private cancel() method.\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>> >> include/libcamera/internal/pipeline_handler.h | 1 +\n>> >> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n>> >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n>> >> 3 files changed, 31 insertions(+), 7 deletions(-)\n>> >> \n>> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>> >> index 0d3808036..fb28a18d0 100644\n>> >> --- a/include/libcamera/internal/pipeline_handler.h\n>> >> +++ b/include/libcamera/internal/pipeline_handler.h\n>> >> @@ -60,6 +60,7 @@ public:\n>> >> \n>> >> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n>> >> \tvoid completeRequest(Request *request);\n>> >> +\tvoid cancelRequest(Request *request);\n>> >> \n>> >> \tstd::string configurationFile(const std::string &subdir,\n>> >> \t\t\t\t const std::string &name) const;\n>> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);\n>> >\n>> > Aren't you cancelling the same request multiple times ?\n>> \n>> Possibly yes. I'll add a check for RequestPending status.\n>\n> How about modifying conversionQueue_ to store instances of a structure\n> that contain a Request pointer in addition to the std::map ?\n\nI prefer keeping data structures simple. What would be the advantage\nworth of maintaining the extra pointer? I'd be inclined to have it if\nit served more purposes, but is it worth just for the cleanup?\n\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 +1420,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>> >> }\n>> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>> >> index e59404691..c9cb11f0f 100644\n>> >> --- a/src/libcamera/pipeline_handler.cpp\n>> >> +++ b/src/libcamera/pipeline_handler.cpp\n>> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n>> >> \twhile (!waitingRequests_.empty()) {\n>> >> \t\tRequest *request = waitingRequests_.front();\n>> >> \t\twaitingRequests_.pop();\n>> >> -\n>> >> -\t\trequest->_d()->cancel();\n>> >> -\t\tcompleteRequest(request);\n>> >> +\t\tcancelRequest(request);\n>> >> \t}\n>> >> \n>> >> \t/* Make sure no requests are pending. */\n>> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>> >> \t}\n>> >> \n>> >> \tint ret = queueRequestDevice(camera, request);\n>> >> -\tif (ret) {\n>> >> -\t\trequest->_d()->cancel();\n>> >> -\t\tcompleteRequest(request);\n>> >> -\t}\n>> >> +\tif (ret)\n>> >> +\t\tcancelRequest(request);\n>> >> }\n>> >> \n>> >> /**\n>> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n>> >> \t}\n>> >> }\n>> >> \n>> >> +/**\n>> >> + * \\brief Cancel request and signal its completion\n>> >> + * \\param[in] request The request to cancel\n>> >> + *\n>> >> + * This function cancels the request in addition to its completion. The same\n>> >> + * rules as for completeRequest() apply.\n>> >> + */\n>> >> +void PipelineHandler::cancelRequest(Request *request)\n>> >> +{\n>> >> +\trequest->_d()->cancel();\n>> >> +\tcompleteRequest(request);\n>> >> +}\n>> >> +\n>> >> /**\n>> >> * \\brief Retrieve the absolute path to a platform configuration file\n>> >> * \\param[in] subdir The pipeline handler specific subdirectory name\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 7B2B0C32E0\n\tfor ;\n\tWed, 9 Oct 2024 19:43:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A5D5618C9;\n\tWed, 9 Oct 2024 21:43:50 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A5215618C9\n\tfor ;\n\tWed, 9 Oct 2024 21:43:48 +0200 (CEST)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-114-lm0431M7OYGnrQTX6rkkpg-1; Wed, 09 Oct 2024 15:43:46 -0400","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-37cd26ac362so38010f8f.2\n\tfor ;\n\tWed, 09 Oct 2024 12:43:45 -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\tffacd0b85a97d-37d1695e462sm11189412f8f.87.2024.10.09.12.43.42\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 09 Oct 2024 12:43:43 -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=\"HbV1R5Ui\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1728503027;\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=c9t7AxBDjj4GA1jRVKsz8VfgkNskhC4xWGiuKPQqoew=;\n\tb=HbV1R5UiueHlWA3PyNM7EDaOeqDBz0q9eGQ5YqHz6abGcZGamspdib5TTfDuMIYAJo1i0i\n\tsopGiMdregAoJSM9l7S1aW7LKP0mJMuhZHzAv4Gw1X+s3G4Y+xCdLjUdS2QAQSmBXvAt+0\n\t79FMq9JKl+DkI0eY0xvFTLol56HYZ8w=","X-MC-Unique":"lm0431M7OYGnrQTX6rkkpg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728503025; x=1729107825;\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=c9t7AxBDjj4GA1jRVKsz8VfgkNskhC4xWGiuKPQqoew=;\n\tb=Yga/7o4QxtmgH2ZwtP1LJfR+WBVtLmbdzhkOUmMq8T6tsGCEiGE66OQbmZY9BzF7VL\n\tEJj2e5u9buKMa8Ud11IWM8UMzuQejIGMEOQtUPiA6cJYnDaFJ38O/O9XGIIXI38VWyj3\n\tLv8OGk/QchYVHvwrCN/tX9l9f3cxkhu4QYxFp/YqyfIOapHA3kCdC6X0GCzV4hwngpV7\n\ttKwK8+RtDgDs+ILdZTS0fHzeuerpoGxA3R7oqlpLu8+pmYP34cMdZw5lXycD8VoSssC4\n\tVWMzielBcGqdJM/G4rMzJbugvn7c+215cBBO+x3l5CwaNYWoPnd7bW4H8GzRSl5V5Yd8\n\tUO8w==","X-Gm-Message-State":"AOJu0YwaR/Vpa2R4YCtdeV6cC8h77QU7jaZ18gjmYV8T7HL19sFrGnuB\n\t4wn31hMZLFLdN/TV4hWwbDLWJG+jiXigxunf+3HBJWhwDzEnVXkKXNPPAe+ZSiK9a3529sCAJKv\n\t0SLVR0CpwfOisk85JLqQixddTVskvU9ywxvjqh8UeP+NGBIXEI52vqJTdbZu+l4VRX7IhNf4=","X-Received":["by 2002:adf:e5c3:0:b0:37c:c842:a16e with SMTP id\n\tffacd0b85a97d-37d3a9b5f4emr1937218f8f.5.1728503024838; \n\tWed, 09 Oct 2024 12:43:44 -0700 (PDT)","by 2002:adf:e5c3:0:b0:37c:c842:a16e with SMTP id\n\tffacd0b85a97d-37d3a9b5f4emr1937207f8f.5.1728503024317; \n\tWed, 09 Oct 2024 12:43:44 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IH+J6gAMHwmS4ICP04Df7peT24LoCTQXJQEEMn/E9Gh0DhAfQcNowBXCvGAb+CtQpsJI9XhMw==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","In-Reply-To":"<20241009191955.GE16232@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 9 Oct 2024 22:19:55 +0300\")","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<20241009142037.GE2733@pendragon.ideasonboard.com>\n\t<87a5fdqkx3.fsf@redhat.com>\n\t<20241009191955.GE16232@pendragon.ideasonboard.com>","Date":"Wed, 09 Oct 2024 21:43:42 +0200","Message-ID":"<875xq1qcy9.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":31670,"web_url":"https://patchwork.libcamera.org/comment/31670/","msgid":"","date":"2024-10-09T20:53:17","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. október 9., szerda 13:17 keltezéssel, Kieran Bingham írta:\n\n> Quoting Robert Mader (2024-10-09 10:41:00)\n> > Hi, thanks for the patch!\n> >\n> > On 09.10.24 10:39, Milan Zamazal wrote:\n> > > Milan Zamazal writes:\n> > >\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.\n> > >>\n> > >> This patch fixes the omission. The request must be also canceled, which\n> > >> requires introducing a little PipelineHandler::cancelRequest helper in\n> > >> order to be able to access the private cancel() method.\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> > > Changes in v2:\n> > > - The request is additionally canceled.\n> > > - New helper method PipelineHandler::cancelRequest() introduced.\n> > >\n> > > Robert, could you please test v2?\n> >\n> > I gave it a quick go for 5 minutes - generally it seems to work great,\n> > however trying enough quick camera switching I still run into the assert.\n> >\n> > So probably, on top of this, we'll need something implementing what\n> > Kieran suggested:\n> >\n> > > (Though I think we should reject any incoming requests as soon as we\n> > hit stop())\n> \n> Sorry - there I was saying that should /already/ be happening. The state\n> machine should put the camera in to a stopping state where new incoming\n> requests will be rejected...\n> \n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1401\n> \n> Unless this is still somehow racy ;-(\n\nIt seems to me that one thread calling Camera::stop() and another thread calling\nCamera::queueRequest() can cause this issue. Specifically, if the thread executing\nCamera::stop() is stopped e.g. after `LOG(Camera, Debug) << \"Stopping capture\";`,\nthen the thread running Camera::queueRequest() can progress and queue the request\nwithout anything stopping it. Afterwards, if the thread running Camera::stop() continues\nsuch that the request hasn't been completed, then the assertion will be hit\nunless PipelineHandler::stopDevice() does something with the requests. At least\nthat what I can see, it is possible that I have overlooked something.\n\nSlightly unrelated, but I think the state transitions of the Camera should be done\nwith atomic compare exchange operations, otherwise multiple threads could successfully\ncall e.g. Camera::stop() concurrently.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> --\n> Kieran\n> \n> \n> >\n> > >> ---\n> > >> include/libcamera/internal/pipeline_handler.h | 1 +\n> > >> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n> > >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> > >> 3 files changed, 31 insertions(+), 7 deletions(-)\n> > >>\n> > >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > >> index 0d3808036..fb28a18d0 100644\n> > >> --- a/include/libcamera/internal/pipeline_handler.h\n> > >> +++ b/include/libcamera/internal/pipeline_handler.h\n> > >> @@ -60,6 +60,7 @@ public:\n> > >>\n> > >> bool completeBuffer(Request *request, FrameBuffer *buffer);\n> > >> void completeRequest(Request *request);\n> > >> + void cancelRequest(Request *request);\n> > >>\n> > >> std::string configurationFile(const std::string &subdir,\n> > >> const std::string &name) const;\n> > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(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 +1420,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> > >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > >> index e59404691..c9cb11f0f 100644\n> > >> --- a/src/libcamera/pipeline_handler.cpp\n> > >> +++ b/src/libcamera/pipeline_handler.cpp\n> > >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> > >> while (!waitingRequests_.empty()) {\n> > >> Request *request = waitingRequests_.front();\n> > >> waitingRequests_.pop();\n> > >> -\n> > >> - request->_d()->cancel();\n> > >> - completeRequest(request);\n> > >> + cancelRequest(request);\n> > >> }\n> > >>\n> > >> /* Make sure no requests are pending. */\n> > >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> > >> }\n> > >>\n> > >> int ret = queueRequestDevice(camera, request);\n> > >> - if (ret) {\n> > >> - request->_d()->cancel();\n> > >> - completeRequest(request);\n> > >> - }\n> > >> + if (ret)\n> > >> + cancelRequest(request);\n> > >> }\n> > >>\n> > >> /**\n> > >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> > >> }\n> > >> }\n> > >>\n> > >> +/**\n> > >> + * \\brief Cancel request and signal its completion\n> > >> + * \\param[in] request The request to cancel\n> >\n> > Small typo here\n> >\n> >\n> > >> + *\n> > >> + * This function cancels the request in addition to its completion. The same\n> > >> + * rules as for completeRequest() apply.\n> > >> + */\n> > >> +void PipelineHandler::cancelRequest(Request *request)\n> > >> +{\n> > >> + request->_d()->cancel();\n> > >> + completeRequest(request);\n> > >> +}\n> > >> +\n> > >> /**\n> > >> * \\brief Retrieve the absolute path to a platform configuration file\n> > >> * \\param[in] subdir The pipeline handler specific subdirectory name\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 7430AC32E0\n\tfor ;\n\tWed, 9 Oct 2024 20:53:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9541163538;\n\tWed, 9 Oct 2024 22:53:26 +0200 (CEST)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A2E26618C5\n\tfor ;\n\tWed, 9 Oct 2024 22:53:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"rx7f1IgJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1728507203; x=1728766403;\n\tbh=2kQOCdbmFJP9F4us9QtigI9gR/OSwd6vME3VEQCmN2k=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=rx7f1IgJHf47JxRJyGV9fQqFf02YPM2df+pfrZudWhFD0ntD8ndWnIeknEv4xP2O7\n\tc5AYXTzEzVp3P2gaOhlwV1cn/AjQOorxZbOGzmsVa0tc5EyGzovXrPqhPoIz0xlr+h\n\tjnxn6SrsbbJu/xW/ahR4P2on4ojxXSZl7oSwKXMBHuArZJS+tQlRvBZVPnYoA8y0DL\n\t836DM+y43dLIgVkS9P3I7Swx4RDlY4aw8PcPHB4j+XeR41CcicGUKjdgQ9Tz6tn0L0\n\tv6HKDPk32lgCHvzk/M9/phJtNT1aios8DoXe4hEHv+YGBP8f01QzDOKPsSjcWI44yL\n\tyG7Qd3s31Xa+g==","Date":"Wed, 09 Oct 2024 20:53:17 +0000","To":"Kieran Bingham ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Robert Mader ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","Message-ID":"","In-Reply-To":"<172847267964.3353069.508849088657641080@ping.linuxembedded.co.uk>","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<87o73tr7q1.fsf@redhat.com>\n\t<0bd96666-952a-47f9-8bec-d5a6e8652a26@collabora.com>\n\t<172847267964.3353069.508849088657641080@ping.linuxembedded.co.uk>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"8ddd8ac12429e83e1980d59b5f64891265dda59f","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":31702,"web_url":"https://patchwork.libcamera.org/comment/31702/","msgid":"<20241010144706.GB26015@pendragon.ideasonboard.com>","date":"2024-10-10T14:47:06","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:\n> >> Laurent Pinchart writes:\n> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote:\n> >\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.\n> >> >> \n> >> >> This patch fixes the omission. The request must be also canceled, which\n> >> >> requires introducing a little PipelineHandler::cancelRequest helper in\n> >> >> order to be able to access the private cancel() method.\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> >> >> include/libcamera/internal/pipeline_handler.h | 1 +\n> >> >> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n> >> >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> >> >> 3 files changed, 31 insertions(+), 7 deletions(-)\n> >> >> \n> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> >> >> index 0d3808036..fb28a18d0 100644\n> >> >> --- a/include/libcamera/internal/pipeline_handler.h\n> >> >> +++ b/include/libcamera/internal/pipeline_handler.h\n> >> >> @@ -60,6 +60,7 @@ public:\n> >> >> \n> >> >> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> >> >> \tvoid completeRequest(Request *request);\n> >> >> +\tvoid cancelRequest(Request *request);\n> >> >> \n> >> >> \tstd::string configurationFile(const std::string &subdir,\n> >> >> \t\t\t\t const std::string &name) const;\n> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);\n> >> >\n> >> > Aren't you cancelling the same request multiple times ?\n> >> \n> >> Possibly yes. I'll add a check for RequestPending status.\n> >\n> > How about modifying conversionQueue_ to store instances of a structure\n> > that contain a Request pointer in addition to the std::map ?\n> \n> I prefer keeping data structures simple. What would be the advantage\n> worth of maintaining the extra pointer? I'd be inclined to have it if\n> it served more purposes, but is it worth just for the cleanup?\n\nAs far as I understand, the conversionQueue_ contains a map of streams\nto output buffers for one request. If you look at\nSimpleCameraData::bufferReady(), you'll see, in the\n!FrameMetadata::FrameSuccess path at the top of the function,\n\n\t\tRequest *request = nullptr;\n\t\tfor (auto &item : conversionQueue_.front()) {\n\t\t\tFrameBuffer *outputBuffer = item.second;\n\t\t\trequest = outputBuffer->request();\n\t\t\tpipe->completeBuffer(request, outputBuffer);\n\t\t}\n\t\tconversionQueue_.pop();\n\n\t\tif (request)\n\t\t\tpipe->completeRequest(request);\n\nAll buffers need to be completed individually, but the request needs to\nbe completed once only.\n\nAll the output buffers in the map relate to the same request, so I think\nit makes sense to store the request pointer separately in the queue for\neasy access. Alternatively, you could have a code construct similar to\nthe above, getting the request pointer from the buffer, and calling\ncompleteRequest() once only. It would be nice if we could share more\ncode, replacing the above construct with something shared by the cancel\npath.\n\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 +1420,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> >> >> }\n> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> >> >> index e59404691..c9cb11f0f 100644\n> >> >> --- a/src/libcamera/pipeline_handler.cpp\n> >> >> +++ b/src/libcamera/pipeline_handler.cpp\n> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> >> >> \twhile (!waitingRequests_.empty()) {\n> >> >> \t\tRequest *request = waitingRequests_.front();\n> >> >> \t\twaitingRequests_.pop();\n> >> >> -\n> >> >> -\t\trequest->_d()->cancel();\n> >> >> -\t\tcompleteRequest(request);\n> >> >> +\t\tcancelRequest(request);\n> >> >> \t}\n> >> >> \n> >> >> \t/* Make sure no requests are pending. */\n> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> >> >> \t}\n> >> >> \n> >> >> \tint ret = queueRequestDevice(camera, request);\n> >> >> -\tif (ret) {\n> >> >> -\t\trequest->_d()->cancel();\n> >> >> -\t\tcompleteRequest(request);\n> >> >> -\t}\n> >> >> +\tif (ret)\n> >> >> +\t\tcancelRequest(request);\n> >> >> }\n> >> >> \n> >> >> /**\n> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> >> >> \t}\n> >> >> }\n> >> >> \n> >> >> +/**\n> >> >> + * \\brief Cancel request and signal its completion\n> >> >> + * \\param[in] request The request to cancel\n> >> >> + *\n> >> >> + * This function cancels the request in addition to its completion. The same\n> >> >> + * rules as for completeRequest() apply.\n> >> >> + */\n> >> >> +void PipelineHandler::cancelRequest(Request *request)\n> >> >> +{\n> >> >> +\trequest->_d()->cancel();\n> >> >> +\tcompleteRequest(request);\n> >> >> +}\n> >> >> +\n> >> >> /**\n> >> >> * \\brief Retrieve the absolute path to a platform configuration file\n> >> >> * \\param[in] subdir The pipeline handler specific subdirectory name","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 45914C32E0\n\tfor ;\n\tThu, 10 Oct 2024 14:47:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3AAEE6536D;\n\tThu, 10 Oct 2024 16:47:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E6FD6353A\n\tfor ;\n\tThu, 10 Oct 2024 16:47:11 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [23.233.251.139])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CF81B4D4;\n\tThu, 10 Oct 2024 16:45:32 +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=\"Iu98hD66\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728571533;\n\tbh=LHT20m/pxa7Xh5YEbEmPAeJ81O0NIaOOHlyjx/7RLrI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Iu98hD664LHH+CMm5I9mPSv0aFc+wOq5/nJgEnNNrBnG2Ld7P6VLriZ6LTo6VxiwU\n\tvoKgtupN5rpRX6M1+bKS9NpUZmRZTaCOuqJudGJnrqwp424dJ7IB5gH9Kz8hx8Ihqd\n\txI45/5HfGMeBnDcMXZQfSKBgZH2OYdbhpWdLsQlI=","Date":"Thu, 10 Oct 2024 17:47:06 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","Message-ID":"<20241010144706.GB26015@pendragon.ideasonboard.com>","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<20241009142037.GE2733@pendragon.ideasonboard.com>\n\t<87a5fdqkx3.fsf@redhat.com>\n\t<20241009191955.GE16232@pendragon.ideasonboard.com>\n\t<875xq1qcy9.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<875xq1qcy9.fsf@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\" "}},{"id":31715,"web_url":"https://patchwork.libcamera.org/comment/31715/","msgid":"<8734l2wx2d.fsf@redhat.com>","date":"2024-10-11T14:09:30","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:\n>> Laurent Pinchart writes:\n>> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:\n>\n>> >> Laurent Pinchart writes:\n>> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote:\n>> >\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.\n>> >> >> \n>> >> >> This patch fixes the omission. The request must be also canceled, which\n>> >> >> requires introducing a little PipelineHandler::cancelRequest helper in\n>> >> >> order to be able to access the private cancel() method.\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>> >> >> include/libcamera/internal/pipeline_handler.h | 1 +\n>> >> >> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n>> >> >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n>> >> >> 3 files changed, 31 insertions(+), 7 deletions(-)\n>> >> >> \n>> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>> >> >> index 0d3808036..fb28a18d0 100644\n>> >> >> --- a/include/libcamera/internal/pipeline_handler.h\n>> >> >> +++ b/include/libcamera/internal/pipeline_handler.h\n>> >> >> @@ -60,6 +60,7 @@ public:\n>> >> >> \n>> >> >> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n>> >> >> \tvoid completeRequest(Request *request);\n>> >> >> +\tvoid cancelRequest(Request *request);\n>> >> >> \n>> >> >> \tstd::string configurationFile(const std::string &subdir,\n>> >> >> \t\t\t\t const std::string &name) const;\n>> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> >> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);\n>> >> >\n>> >> > Aren't you cancelling the same request multiple times ?\n>> >> \n>> >> Possibly yes. I'll add a check for RequestPending status.\n>> >\n>> > How about modifying conversionQueue_ to store instances of a structure\n>> > that contain a Request pointer in addition to the std::map ?\n>> \n>> I prefer keeping data structures simple. What would be the advantage\n>> worth of maintaining the extra pointer? I'd be inclined to have it if\n>> it served more purposes, but is it worth just for the cleanup?\n>\n> As far as I understand, the conversionQueue_ contains a map of streams\n> to output buffers for one request. If you look at\n> SimpleCameraData::bufferReady(), you'll see, in the\n> !FrameMetadata::FrameSuccess path at the top of the function,\n>\n> \t\tRequest *request = nullptr;\n> \t\tfor (auto &item : conversionQueue_.front()) {\n> \t\t\tFrameBuffer *outputBuffer = item.second;\n> \t\t\trequest = outputBuffer->request();\n> \t\t\tpipe->completeBuffer(request, outputBuffer);\n> \t\t}\n> \t\tconversionQueue_.pop();\n>\n> \t\tif (request)\n> \t\t\tpipe->completeRequest(request);\n>\n> All buffers need to be completed individually, but the request needs to\n> be completed once only.\n\nBut it is completed only if:\n\n- There is at least one output buffer\n- AND the buffer refers to the given request.\n\nThose look like reasonable assumptions but I'm not sure there is nothing\nsubtle behind. You authored the code structure above when adding\nsupport for multiple streams so can you confirm that something like\n\n std::queue>> conversionQueue_;\n\n ...\n\n Request *request = conversionQueue_.front().first;\n for (auto &item : conversionQueue_.front().second)\n \tpipe->completeBuffer(request, item.second);\n pipe->completeRequest(request);\n conversionQueue_.pop();\n\nwould be equivalent under the right assumptions?\n\n> All the output buffers in the map relate to the same request, so I think\n> it makes sense to store the request pointer separately in the queue for\n> easy access. Alternatively, you could have a code construct similar to\n> the above, getting the request pointer from the buffer, and calling\n> completeRequest() once only. It would be nice if we could share more\n> code, replacing the above construct with something shared by the cancel\n> path.\n>\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 +1420,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>> >> >> }\n>> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>> >> >> index e59404691..c9cb11f0f 100644\n>> >> >> --- a/src/libcamera/pipeline_handler.cpp\n>> >> >> +++ b/src/libcamera/pipeline_handler.cpp\n>> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n>> >> >> \twhile (!waitingRequests_.empty()) {\n>> >> >> \t\tRequest *request = waitingRequests_.front();\n>> >> >> \t\twaitingRequests_.pop();\n>> >> >> -\n>> >> >> -\t\trequest->_d()->cancel();\n>> >> >> -\t\tcompleteRequest(request);\n>> >> >> +\t\tcancelRequest(request);\n>> >> >> \t}\n>> >> >> \n>> >> >> \t/* Make sure no requests are pending. */\n>> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>> >> >> \t}\n>> >> >> \n>> >> >> \tint ret = queueRequestDevice(camera, request);\n>> >> >> -\tif (ret) {\n>> >> >> -\t\trequest->_d()->cancel();\n>> >> >> -\t\tcompleteRequest(request);\n>> >> >> -\t}\n>> >> >> +\tif (ret)\n>> >> >> +\t\tcancelRequest(request);\n>> >> >> }\n>> >> >> \n>> >> >> /**\n>> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n>> >> >> \t}\n>> >> >> }\n>> >> >> \n>> >> >> +/**\n>> >> >> + * \\brief Cancel request and signal its completion\n>> >> >> + * \\param[in] request The request to cancel\n>> >> >> + *\n>> >> >> + * This function cancels the request in addition to its completion. The same\n>> >> >> + * rules as for completeRequest() apply.\n>> >> >> + */\n>> >> >> +void PipelineHandler::cancelRequest(Request *request)\n>> >> >> +{\n>> >> >> +\trequest->_d()->cancel();\n>> >> >> +\tcompleteRequest(request);\n>> >> >> +}\n>> >> >> +\n>> >> >> /**\n>> >> >> * \\brief Retrieve the absolute path to a platform configuration file\n>> >> >> * \\param[in] subdir The pipeline handler specific subdirectory name","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 8BE1EC326B\n\tfor ;\n\tFri, 11 Oct 2024 14:09:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDD5D6536C;\n\tFri, 11 Oct 2024 16:09:37 +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 03E996353B\n\tfor ;\n\tFri, 11 Oct 2024 16:09:35 +0200 (CEST)","from mail-ej1-f72.google.com (mail-ej1-f72.google.com\n\t[209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-315-nXvxXNwzPpOhge0A_YzOWA-1; Fri, 11 Oct 2024 10:09:33 -0400","by mail-ej1-f72.google.com with SMTP id\n\ta640c23a62f3a-a9960ef689dso145090866b.0\n\tfor ;\n\tFri, 11 Oct 2024 07:09:33 -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-a99d31480cesm13967166b.20.2024.10.11.07.09.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 11 Oct 2024 07:09:31 -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=\"VyL+nucI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1728655774;\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=mYZcpDX9FIaa85eCSTtCuvk3S3h8RAyfdcDdnTQXJuk=;\n\tb=VyL+nucI/Fo0qUkvA35BlOIqapo3jOacmOAeWTGRnvXrHCxjzhDzQAZJkbRFFTgdrD0jpi\n\tOHjpH4Q6lM6NKlDUsg8tzVVWkT1RJdlaF05Q7nYVNbW1MC8oSgtqvH+XPl1h03XTG7c8rp\n\tJsomOffM0NR5JsDOHUDVImgykFLitbw=","X-MC-Unique":"nXvxXNwzPpOhge0A_YzOWA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728655772; x=1729260572;\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=mYZcpDX9FIaa85eCSTtCuvk3S3h8RAyfdcDdnTQXJuk=;\n\tb=U/0Zl3L5Evu2VCiPlhCEwR8V67FWNNG0OALe0xwMhJqJDeiH70we4LpUkHufCsPywt\n\tTZPHwmvWKO4OoUREzQttg/EKgc5YvDze60n5z9B5fKl9No50BK1PcVueKT+Hyii1AFgR\n\t7V70eYl3CUJc6qvFn2PQCy+IfWNufjl4A71K5DqC7vl535iT0bT9SB4a3DqAPwGZTGCL\n\tqVBjz6NNX22tFZ9KHwvxsTIz9w29Pqi1Vn4csVef5VXyPzS/0xFBufWrp1lTMll3+XrC\n\thnk4KNXRvztbBkCYu0pcA7bKfRpiU2IaHqTAFuqdl5pIsWAjv6d75XER+zX9jxW11o59\n\tFXOg==","X-Gm-Message-State":"AOJu0YyB4JpgI5Is4du0uV2iChi9WzGdBTn39cGCCC4deUiHUdoYIPKT\n\tswy9Wt8og9T152kzNFqcrCwKjO/r8c5Dat1DhplZo0dbKoLoOR7xMpynfZZfzEY4XmbrVZhuCFg\n\t7M4oDV2HzL43auQ3oLOTMxKn8oNZUsCDgnyAdMo8THWXkAMt6hH1lLIjrYTnVzoKoPfNBeS4=","X-Received":["by 2002:a17:907:7fa1:b0:a99:8ed2:7e0a with SMTP id\n\ta640c23a62f3a-a99b9300117mr214782066b.3.1728655772318; \n\tFri, 11 Oct 2024 07:09:32 -0700 (PDT)","by 2002:a17:907:7fa1:b0:a99:8ed2:7e0a with SMTP id\n\ta640c23a62f3a-a99b9300117mr214779366b.3.1728655771757; \n\tFri, 11 Oct 2024 07:09:31 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFH+vGfFb8gKRmyHK8KfiV+nhFd/JdiBP9TrG5FtpMLGlok29DxwHnjxJm+abfG7go8OzIrkA==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","In-Reply-To":"<20241010144706.GB26015@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Thu, 10 Oct 2024 17:47:06 +0300\")","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<20241009142037.GE2733@pendragon.ideasonboard.com>\n\t<87a5fdqkx3.fsf@redhat.com>\n\t<20241009191955.GE16232@pendragon.ideasonboard.com>\n\t<875xq1qcy9.fsf@redhat.com>\n\t<20241010144706.GB26015@pendragon.ideasonboard.com>","Date":"Fri, 11 Oct 2024 16:09:30 +0200","Message-ID":"<8734l2wx2d.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":31790,"web_url":"https://patchwork.libcamera.org/comment/31790/","msgid":"<87wmi53gjv.fsf@redhat.com>","date":"2024-10-18T09:29:24","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Milan Zamazal writes:\n\n> Laurent Pinchart writes:\n>\n>> On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:\n>>> Laurent Pinchart writes:\n>>> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:\n>>\n>>> >> Laurent Pinchart writes:\n>>> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote:\n>>> >\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.\n>>> >> >> \n>>> >> >> This patch fixes the omission. The request must be also canceled, which\n>>> >> >> requires introducing a little PipelineHandler::cancelRequest helper in\n>>> >> >> order to be able to access the private cancel() method.\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>>> >> >> include/libcamera/internal/pipeline_handler.h | 1 +\n>>> >> >> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n>>> >> >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n>>> >> >> 3 files changed, 31 insertions(+), 7 deletions(-)\n>>> >> >> \n>>> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>>> >> >> index 0d3808036..fb28a18d0 100644\n>>> >> >> --- a/include/libcamera/internal/pipeline_handler.h\n>>> >> >> +++ b/include/libcamera/internal/pipeline_handler.h\n>>> >> >> @@ -60,6 +60,7 @@ public:\n>>> >> >> \n>>> >> >> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n>>> >> >> \tvoid completeRequest(Request *request);\n>>> >> >> +\tvoid cancelRequest(Request *request);\n>>> >> >> \n>>> >> >> \tstd::string configurationFile(const std::string &subdir,\n>>> >> >> \t\t\t\t const std::string &name) const;\n>>> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>>> >> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);\n>>> >> >\n>>> >> > Aren't you cancelling the same request multiple times ?\n>>> >> \n>>> >> Possibly yes. I'll add a check for RequestPending status.\n>>> >\n>>> > How about modifying conversionQueue_ to store instances of a structure\n>>> > that contain a Request pointer in addition to the std::map ?\n>>> \n>>> I prefer keeping data structures simple. What would be the advantage\n>>> worth of maintaining the extra pointer? I'd be inclined to have it if\n>>> it served more purposes, but is it worth just for the cleanup?\n>>\n>> As far as I understand, the conversionQueue_ contains a map of streams\n>> to output buffers for one request. If you look at\n>> SimpleCameraData::bufferReady(), you'll see, in the\n>> !FrameMetadata::FrameSuccess path at the top of the function,\n>>\n>> \t\tRequest *request = nullptr;\n>> \t\tfor (auto &item : conversionQueue_.front()) {\n>> \t\t\tFrameBuffer *outputBuffer = item.second;\n>> \t\t\trequest = outputBuffer->request();\n>> \t\t\tpipe->completeBuffer(request, outputBuffer);\n>> \t\t}\n>> \t\tconversionQueue_.pop();\n>>\n>> \t\tif (request)\n>> \t\t\tpipe->completeRequest(request);\n>>\n>> All buffers need to be completed individually, but the request needs to\n>> be completed once only.\n>\n> But it is completed only if:\n>\n> - There is at least one output buffer\n> - AND the buffer refers to the given request.\n>\n> Those look like reasonable assumptions but I'm not sure there is nothing\n> subtle behind. You authored the code structure above when adding\n> support for multiple streams so can you confirm that something like\n>\n> std::queue>> conversionQueue_;\n>\n> ...\n>\n> Request *request = conversionQueue_.front().first;\n> for (auto &item : conversionQueue_.front().second)\n> \tpipe->completeBuffer(request, item.second);\n> pipe->completeRequest(request);\n> conversionQueue_.pop();\n>\n> would be equivalent under the right assumptions?\n\nI take the silence as yes, so posted v4 with this change.\n\n>> All the output buffers in the map relate to the same request, so I think\n>> it makes sense to store the request pointer separately in the queue for\n>> easy access. Alternatively, you could have a code construct similar to\n>> the above, getting the request pointer from the buffer, and calling\n>> completeRequest() once only. It would be nice if we could share more\n>> code, replacing the above construct with something shared by the cancel\n>> path.\n>>\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 +1420,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>>> >> >> }\n>>> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>>> >> >> index e59404691..c9cb11f0f 100644\n>>> >> >> --- a/src/libcamera/pipeline_handler.cpp\n>>> >> >> +++ b/src/libcamera/pipeline_handler.cpp\n>>> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n>>> >> >> \twhile (!waitingRequests_.empty()) {\n>>> >> >> \t\tRequest *request = waitingRequests_.front();\n>>> >> >> \t\twaitingRequests_.pop();\n>>> >> >> -\n>>> >> >> -\t\trequest->_d()->cancel();\n>>> >> >> -\t\tcompleteRequest(request);\n>>> >> >> +\t\tcancelRequest(request);\n>>> >> >> \t}\n>>> >> >> \n>>> >> >> \t/* Make sure no requests are pending. */\n>>> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>>> >> >> \t}\n>>> >> >> \n>>> >> >> \tint ret = queueRequestDevice(camera, request);\n>>> >> >> -\tif (ret) {\n>>> >> >> -\t\trequest->_d()->cancel();\n>>> >> >> -\t\tcompleteRequest(request);\n>>> >> >> -\t}\n>>> >> >> +\tif (ret)\n>>> >> >> +\t\tcancelRequest(request);\n>>> >> >> }\n>>> >> >> \n>>> >> >> /**\n>>> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n>>> >> >> \t}\n>>> >> >> }\n>>> >> >> \n>>> >> >> +/**\n>>> >> >> + * \\brief Cancel request and signal its completion\n>>> >> >> + * \\param[in] request The request to cancel\n>>> >> >> + *\n>>> >> >> + * This function cancels the request in addition to its completion. The same\n>>> >> >> + * rules as for completeRequest() apply.\n>>> >> >> + */\n>>> >> >> +void PipelineHandler::cancelRequest(Request *request)\n>>> >> >> +{\n>>> >> >> +\trequest->_d()->cancel();\n>>> >> >> +\tcompleteRequest(request);\n>>> >> >> +}\n>>> >> >> +\n>>> >> >> /**\n>>> >> >> * \\brief Retrieve the absolute path to a platform configuration file\n>>> >> >> * \\param[in] subdir The pipeline handler specific subdirectory name","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 3D7D0C32FE\n\tfor ;\n\tFri, 18 Oct 2024 09:29:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A25EE6538F;\n\tFri, 18 Oct 2024 11:29:31 +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 8969165379\n\tfor ;\n\tFri, 18 Oct 2024 11:29:30 +0200 (CEST)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-353-YOf1lbKANbGrjFwSupU_gQ-1; Fri, 18 Oct 2024 05:29:28 -0400","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-37d458087c0so1707591f8f.1\n\tfor ;\n\tFri, 18 Oct 2024 02:29:27 -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\t5b1f17b1804b1-4316067dc4bsm22388735e9.4.2024.10.18.02.29.25\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 18 Oct 2024 02:29:25 -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=\"I7deyduk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1729243769;\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=PjuDez8WGTo0InGo7ngXUI27hfOh5XWG6HFSaWS/C8w=;\n\tb=I7deydukTWyRtdCmTuq0F8X2tTMZZqdORzRbrvnPuV2gxvxITrfTeWZ3jZ8k1obc4yGgoT\n\tMoEVgvilLhYB3NF/ia+mPFTqFe21JXzTnAJ1kUCtSYYzabmgbV/DKeo416F7rCkgH2E8jk\n\tkUxYtTsXdMRyMMH+R7C/V7MEP3jUXr0=","X-MC-Unique":"YOf1lbKANbGrjFwSupU_gQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1729243767; x=1729848567;\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=PjuDez8WGTo0InGo7ngXUI27hfOh5XWG6HFSaWS/C8w=;\n\tb=asEMq0ZgGSi0Nb1AgUc86vMaJINg6XMsGnZxt/c6FARnjTUp58EI3OfNX2Dtzn0zxr\n\tIv6dG+MR1xPhsTMDi8nhv/SplaT2b30qbPeiI729lBoZLSr8lR6dG3fIq7QantTxbWLn\n\t32idSZ5bcIJPLIr6BVyS/gQYTZquhZGWyU7wyj77mpEkLXvVpzp7yn84+xQGcFec980c\n\tqwbfJFavPAludjGDrR9hb+xg5/RXZhJd7CttFKLimzGtV1pTV0PJ7Cq58m4ZYYQl7b7S\n\tZHTthl/3I1nHPLPEHHNTuNR0HmhlXcCiWygpYhMw/T9sWS/mdD0fOUOm4zAn2LdDbvff\n\tN2+Q==","X-Gm-Message-State":"AOJu0YxSChY6cZ/h3FPGR9plre7PpJKYTXrQf7E715DUZ3DsAnVPSW+N\n\tkoydVFErOK+v79OEdIJ4J4x2uoXavWoG60lLA/BvI/kSfqil5GZC6y+EgQOj5GTuKOQGo+Bf9Gl\n\tjO/onaR0qYqGTv6jaG/pp+XtrGm/kjYcufxi7vuLZryDp/3E+eyUJHzmsrRmvYCIQVFuW9B8=","X-Received":["by 2002:a05:6000:504:b0:37c:f997:5b94 with SMTP id\n\tffacd0b85a97d-37d93d759f7mr4385500f8f.12.1729243766794; \n\tFri, 18 Oct 2024 02:29:26 -0700 (PDT)","by 2002:a05:6000:504:b0:37c:f997:5b94 with SMTP id\n\tffacd0b85a97d-37d93d759f7mr4385473f8f.12.1729243766362; \n\tFri, 18 Oct 2024 02:29:26 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEuMtsxGFpvxME08FoVVlheO43SWlWoQ8JaY/3AlsXrmi/NHt4upF8y4bEKgERC/vzns1DWhg==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","In-Reply-To":"<8734l2wx2d.fsf@redhat.com> (Milan Zamazal's message of \"Fri, 11\n\tOct 2024 16:09:30 +0200\")","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<20241009142037.GE2733@pendragon.ideasonboard.com>\n\t<87a5fdqkx3.fsf@redhat.com>\n\t<20241009191955.GE16232@pendragon.ideasonboard.com>\n\t<875xq1qcy9.fsf@redhat.com>\n\t<20241010144706.GB26015@pendragon.ideasonboard.com>\n\t<8734l2wx2d.fsf@redhat.com>","Date":"Fri, 18 Oct 2024 11:29:24 +0200","Message-ID":"<87wmi53gjv.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":32042,"web_url":"https://patchwork.libcamera.org/comment/32042/","msgid":"<20241106111326.GA30053@pendragon.ideasonboard.com>","date":"2024-11-06T11:13:26","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests on\n\tstop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nDiscussing this fix today made me realized I forgot to reply to this\ne-mail. Sorry about that.\n\nOn Fri, Oct 11, 2024 at 04:09:30PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:\n> >> Laurent Pinchart writes:\n> >> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:\n> >> >> Laurent Pinchart writes:\n> >> >> > On Wed, Oct 09, 2024 at 10:35:56AM +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.\n> >> >> >> \n> >> >> >> This patch fixes the omission. The request must be also canceled, which\n> >> >> >> requires introducing a little PipelineHandler::cancelRequest helper in\n> >> >> >> order to be able to access the private cancel() method.\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> >> >> >> include/libcamera/internal/pipeline_handler.h | 1 +\n> >> >> >> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n> >> >> >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n> >> >> >> 3 files changed, 31 insertions(+), 7 deletions(-)\n> >> >> >> \n> >> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> >> >> >> index 0d3808036..fb28a18d0 100644\n> >> >> >> --- a/include/libcamera/internal/pipeline_handler.h\n> >> >> >> +++ b/include/libcamera/internal/pipeline_handler.h\n> >> >> >> @@ -60,6 +60,7 @@ public:\n> >> >> >> \n> >> >> >> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> >> >> >> \tvoid completeRequest(Request *request);\n> >> >> >> +\tvoid cancelRequest(Request *request);\n> >> >> >> \n> >> >> >> \tstd::string configurationFile(const std::string &subdir,\n> >> >> >> \t\t\t\t const std::string &name) const;\n> >> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >> >> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);\n> >> >> >\n> >> >> > Aren't you cancelling the same request multiple times ?\n> >> >> \n> >> >> Possibly yes. I'll add a check for RequestPending status.\n> >> >\n> >> > How about modifying conversionQueue_ to store instances of a structure\n> >> > that contain a Request pointer in addition to the std::map ?\n> >> \n> >> I prefer keeping data structures simple. What would be the advantage\n> >> worth of maintaining the extra pointer? I'd be inclined to have it if\n> >> it served more purposes, but is it worth just for the cleanup?\n> >\n> > As far as I understand, the conversionQueue_ contains a map of streams\n> > to output buffers for one request. If you look at\n> > SimpleCameraData::bufferReady(), you'll see, in the\n> > !FrameMetadata::FrameSuccess path at the top of the function,\n> >\n> > \t\tRequest *request = nullptr;\n> > \t\tfor (auto &item : conversionQueue_.front()) {\n> > \t\t\tFrameBuffer *outputBuffer = item.second;\n> > \t\t\trequest = outputBuffer->request();\n> > \t\t\tpipe->completeBuffer(request, outputBuffer);\n> > \t\t}\n> > \t\tconversionQueue_.pop();\n> >\n> > \t\tif (request)\n> > \t\t\tpipe->completeRequest(request);\n> >\n> > All buffers need to be completed individually, but the request needs to\n> > be completed once only.\n> \n> But it is completed only if:\n> \n> - There is at least one output buffer\n> - AND the buffer refers to the given request.\n> \n> Those look like reasonable assumptions but I'm not sure there is nothing\n> subtle behind. You authored the code structure above when adding\n> support for multiple streams so can you confirm that something like\n> \n> std::queue>> conversionQueue_;\n> \n> ...\n> \n> Request *request = conversionQueue_.front().first;\n> for (auto &item : conversionQueue_.front().second)\n> \tpipe->completeBuffer(request, item.second);\n> pipe->completeRequest(request);\n> conversionQueue_.pop();\n> \n> would be equivalent under the right assumptions?\n\nMy assumption (please tell me if I'm wrong) is that an entry in the\nconversion queue corresponds to one request, and holds a map of streams\nto frame buffers for that particular request. We have code that needs to\ndeal with those buffers, and also code that needs to deal with the\nrequest. Currently, the request is retrieved from the buffer. In the\ncase of SimpleCameraData::bufferReady(), we use the request from the\nlast buffer in the map, but any buffer would do, as they all belong to\nthe same request.\n\nI find this code confusing, and I believe we should explicitly store the\nrequest pointer in the conversion queue entry, and retrieve it from\nthere, instead of retrieving it from the buffer. It would make the code\nclearer. Your code snippet above looks fine, althouh I would probably\ncreate a new structure to hold the request pointer and map:\n\n\tstruct ConversionJob {\n\t\tRequest *request;\n\t\tstd::map buffers;\n\t};\n\n\tstd::queue conversionQueue_;\n\n(we can bikeshed the ConversionJob name). The code could then look like\n\n \tRequest *request = nullptr;\n\tconst ConversionJob &job = conversionQueue_.front();\n\n \tfor (auto &[stream, outputBuffer] : job)\n \t\tpipe->completeBuffer(request, outputBuffer);\n\n\tpipe->completeRequest(job.request);\n \tconversionQueue_.pop();\n\nwhich I think is much more readable.\n\n> > All the output buffers in the map relate to the same request, so I think\n> > it makes sense to store the request pointer separately in the queue for\n> > easy access. Alternatively, you could have a code construct similar to\n> > the above, getting the request pointer from the buffer, and calling\n> > completeRequest() once only. It would be nice if we could share more\n> > code, replacing the above construct with something shared by the cancel\n> > path.\n> >\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 +1420,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> >> >> >> }\n> >> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> >> >> >> index e59404691..c9cb11f0f 100644\n> >> >> >> --- a/src/libcamera/pipeline_handler.cpp\n> >> >> >> +++ b/src/libcamera/pipeline_handler.cpp\n> >> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n> >> >> >> \twhile (!waitingRequests_.empty()) {\n> >> >> >> \t\tRequest *request = waitingRequests_.front();\n> >> >> >> \t\twaitingRequests_.pop();\n> >> >> >> -\n> >> >> >> -\t\trequest->_d()->cancel();\n> >> >> >> -\t\tcompleteRequest(request);\n> >> >> >> +\t\tcancelRequest(request);\n> >> >> >> \t}\n> >> >> >> \n> >> >> >> \t/* Make sure no requests are pending. */\n> >> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n> >> >> >> \t}\n> >> >> >> \n> >> >> >> \tint ret = queueRequestDevice(camera, request);\n> >> >> >> -\tif (ret) {\n> >> >> >> -\t\trequest->_d()->cancel();\n> >> >> >> -\t\tcompleteRequest(request);\n> >> >> >> -\t}\n> >> >> >> +\tif (ret)\n> >> >> >> +\t\tcancelRequest(request);\n> >> >> >> }\n> >> >> >> \n> >> >> >> /**\n> >> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n> >> >> >> \t}\n> >> >> >> }\n> >> >> >> \n> >> >> >> +/**\n> >> >> >> + * \\brief Cancel request and signal its completion\n> >> >> >> + * \\param[in] request The request to cancel\n> >> >> >> + *\n> >> >> >> + * This function cancels the request in addition to its completion. The same\n> >> >> >> + * rules as for completeRequest() apply.\n> >> >> >> + */\n> >> >> >> +void PipelineHandler::cancelRequest(Request *request)\n> >> >> >> +{\n> >> >> >> +\trequest->_d()->cancel();\n> >> >> >> +\tcompleteRequest(request);\n> >> >> >> +}\n> >> >> >> +\n> >> >> >> /**\n> >> >> >> * \\brief Retrieve the absolute path to a platform configuration file\n> >> >> >> * \\param[in] subdir The pipeline handler specific subdirectory name","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 1A99CBE173\n\tfor ;\n\tWed, 6 Nov 2024 11:13:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FF3C6541D;\n\tWed, 6 Nov 2024 12:13:34 +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 D74C9653C5\n\tfor ;\n\tWed, 6 Nov 2024 12:13:32 +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 7BDD3475;\n\tWed, 6 Nov 2024 12:13:24 +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=\"Ihgky/ug\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730891604;\n\tbh=4sfZs/Elw6rRRwMI0ZxTX4KLBL8qETEhdqlDoaM3QMU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ihgky/ugQagkFzMyX896PUwdQLF5kMh492sHuc8aNqhioBCh/5UWX+ip25JtB6KnV\n\tb+xh7cTcD3w3XT121wbN8kVJktm9Wl8FXKOoeOzJUJGHYN+482BfoLEUzVnhTqf7Ut\n\tmHqfK83zjzvFZLrjsqKhPP/igpBZGzHmJjV7pAw0=","Date":"Wed, 6 Nov 2024 13:13:26 +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 v2] libcamera: software_isp: Clean up pending requests on\n\tstop","Message-ID":"<20241106111326.GA30053@pendragon.ideasonboard.com>","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<20241009142037.GE2733@pendragon.ideasonboard.com>\n\t<87a5fdqkx3.fsf@redhat.com>\n\t<20241009191955.GE16232@pendragon.ideasonboard.com>\n\t<875xq1qcy9.fsf@redhat.com>\n\t<20241010144706.GB26015@pendragon.ideasonboard.com>\n\t<8734l2wx2d.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<8734l2wx2d.fsf@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\" "}},{"id":32063,"web_url":"https://patchwork.libcamera.org/comment/32063/","msgid":"<871pzogks0.fsf@redhat.com>","date":"2024-11-06T20:38:39","subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Discussing this fix today made me realized I forgot to reply to this\n> e-mail. Sorry about that.\n>\n> On Fri, Oct 11, 2024 at 04:09:30PM +0200, Milan Zamazal wrote:\n>> Laurent Pinchart writes:\n>> > On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:\n>> >> Laurent Pinchart writes:\n>> >> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:\n>> >> >> Laurent Pinchart writes:\n>> >> >> > On Wed, Oct 09, 2024 at 10:35:56AM +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.\n>> >> >> >> \n>> >> >> >> This patch fixes the omission. The request must be also canceled, which\n>> >> >> >> requires introducing a little PipelineHandler::cancelRequest helper in\n>> >> >> >> order to be able to access the private cancel() method.\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>> >> >> >> include/libcamera/internal/pipeline_handler.h | 1 +\n>> >> >> >> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++\n>> >> >> >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------\n>> >> >> >> 3 files changed, 31 insertions(+), 7 deletions(-)\n>> >> >> >> \n>> >> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>> >> >> >> index 0d3808036..fb28a18d0 100644\n>> >> >> >> --- a/include/libcamera/internal/pipeline_handler.h\n>> >> >> >> +++ b/include/libcamera/internal/pipeline_handler.h\n>> >> >> >> @@ -60,6 +60,7 @@ public:\n>> >> >> >> \n>> >> >> >> \tbool completeBuffer(Request *request, FrameBuffer *buffer);\n>> >> >> >> \tvoid completeRequest(Request *request);\n>> >> >> >> +\tvoid cancelRequest(Request *request);\n>> >> >> >> \n>> >> >> >> \tstd::string configurationFile(const std::string &subdir,\n>> >> >> >> \t\t\t\t const std::string &name) const;\n>> >> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> >> >> >> index 3ddce71d3..e862ef00f 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,18 @@ 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()->cancelRequest(request);\n>> >> >> >\n>> >> >> > Aren't you cancelling the same request multiple times ?\n>> >> >> \n>> >> >> Possibly yes. I'll add a check for RequestPending status.\n>> >> >\n>> >> > How about modifying conversionQueue_ to store instances of a structure\n>> >> > that contain a Request pointer in addition to the std::map ?\n>> >> \n>> >> I prefer keeping data structures simple. What would be the advantage\n>> >> worth of maintaining the extra pointer? I'd be inclined to have it if\n>> >> it served more purposes, but is it worth just for the cleanup?\n>> >\n>> > As far as I understand, the conversionQueue_ contains a map of streams\n>> > to output buffers for one request. If you look at\n>> > SimpleCameraData::bufferReady(), you'll see, in the\n>> > !FrameMetadata::FrameSuccess path at the top of the function,\n>> >\n>> > \t\tRequest *request = nullptr;\n>> > \t\tfor (auto &item : conversionQueue_.front()) {\n>> > \t\t\tFrameBuffer *outputBuffer = item.second;\n>> > \t\t\trequest = outputBuffer->request();\n>> > \t\t\tpipe->completeBuffer(request, outputBuffer);\n>> > \t\t}\n>> > \t\tconversionQueue_.pop();\n>> >\n>> > \t\tif (request)\n>> > \t\t\tpipe->completeRequest(request);\n>> >\n>> > All buffers need to be completed individually, but the request needs to\n>> > be completed once only.\n>> \n>> But it is completed only if:\n>> \n>> - There is at least one output buffer\n>> - AND the buffer refers to the given request.\n>> \n>> Those look like reasonable assumptions but I'm not sure there is nothing\n>> subtle behind. You authored the code structure above when adding\n>> support for multiple streams so can you confirm that something like\n>> \n>> std::queue>> conversionQueue_;\n>> \n>> ...\n>> \n>> Request *request = conversionQueue_.front().first;\n>> for (auto &item : conversionQueue_.front().second)\n>> \tpipe->completeBuffer(request, item.second);\n>> pipe->completeRequest(request);\n>> conversionQueue_.pop();\n>> \n>> would be equivalent under the right assumptions?\n>\n> My assumption (please tell me if I'm wrong) is that an entry in the\n> conversion queue corresponds to one request, and holds a map of streams\n> to frame buffers for that particular request. \n\nOK.\n\n> We have code that needs to deal with those buffers, and also code that\n> needs to deal with the request. Currently, the request is retrieved\n> from the buffer. In the case of SimpleCameraData::bufferReady(), we\n> use the request from the last buffer in the map, but any buffer would\n> do, as they all belong to the same request.\n>\n> I find this code confusing, and I believe we should explicitly store the\n> request pointer in the conversion queue entry, and retrieve it from\n> there, instead of retrieving it from the buffer. It would make the code\n> clearer. Your code snippet above looks fine, althouh I would probably\n> create a new structure to hold the request pointer and map:\n>\n> \tstruct ConversionJob {\n> \t\tRequest *request;\n> \t\tstd::map buffers;\n> \t};\n>\n> \tstd::queue conversionQueue_;\n>\n> (we can bikeshed the ConversionJob name). The code could then look like\n>\n> \tRequest *request = nullptr;\n> \tconst ConversionJob &job = conversionQueue_.front();\n>\n> \tfor (auto &[stream, outputBuffer] : job)\n> \t\tpipe->completeBuffer(request, outputBuffer);\n>\n> \tpipe->completeRequest(job.request);\n> \tconversionQueue_.pop();\n>\n> which I think is much more readable.\n\nI think the eventual code in v6 is basically the same, minus naming and\nomitting `request' variable. And since it's based on a snippet you\nwrote, I suppose it should be OK for you :-) but tell me in case any\nfurther adjustments are needed.\n\n>> > All the output buffers in the map relate to the same request, so I think\n>> > it makes sense to store the request pointer separately in the queue for\n>> > easy access. Alternatively, you could have a code construct similar to\n>> > the above, getting the request pointer from the buffer, and calling\n>> > completeRequest() once only. It would be nice if we could share more\n>> > code, replacing the above construct with something shared by the cancel\n>> > path.\n>> >\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 +1420,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>> >> >> >> }\n>> >> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>> >> >> >> index e59404691..c9cb11f0f 100644\n>> >> >> >> --- a/src/libcamera/pipeline_handler.cpp\n>> >> >> >> +++ b/src/libcamera/pipeline_handler.cpp\n>> >> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)\n>> >> >> >> \twhile (!waitingRequests_.empty()) {\n>> >> >> >> \t\tRequest *request = waitingRequests_.front();\n>> >> >> >> \t\twaitingRequests_.pop();\n>> >> >> >> -\n>> >> >> >> -\t\trequest->_d()->cancel();\n>> >> >> >> -\t\tcompleteRequest(request);\n>> >> >> >> +\t\tcancelRequest(request);\n>> >> >> >> \t}\n>> >> >> >> \n>> >> >> >> \t/* Make sure no requests are pending. */\n>> >> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)\n>> >> >> >> \t}\n>> >> >> >> \n>> >> >> >> \tint ret = queueRequestDevice(camera, request);\n>> >> >> >> -\tif (ret) {\n>> >> >> >> -\t\trequest->_d()->cancel();\n>> >> >> >> -\t\tcompleteRequest(request);\n>> >> >> >> -\t}\n>> >> >> >> +\tif (ret)\n>> >> >> >> +\t\tcancelRequest(request);\n>> >> >> >> }\n>> >> >> >> \n>> >> >> >> /**\n>> >> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)\n>> >> >> >> \t}\n>> >> >> >> }\n>> >> >> >> \n>> >> >> >> +/**\n>> >> >> >> + * \\brief Cancel request and signal its completion\n>> >> >> >> + * \\param[in] request The request to cancel\n>> >> >> >> + *\n>> >> >> >> + * This function cancels the request in addition to its completion. The same\n>> >> >> >> + * rules as for completeRequest() apply.\n>> >> >> >> + */\n>> >> >> >> +void PipelineHandler::cancelRequest(Request *request)\n>> >> >> >> +{\n>> >> >> >> +\trequest->_d()->cancel();\n>> >> >> >> +\tcompleteRequest(request);\n>> >> >> >> +}\n>> >> >> >> +\n>> >> >> >> /**\n>> >> >> >> * \\brief Retrieve the absolute path to a platform configuration file\n>> >> >> >> * \\param[in] subdir The pipeline handler specific subdirectory name","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 F20F4BDB13\n\tfor ;\n\tWed, 6 Nov 2024 20:38:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4327E65431;\n\tWed, 6 Nov 2024 21:38:47 +0100 (CET)","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 5E64F65431\n\tfor ;\n\tWed, 6 Nov 2024 21:38:45 +0100 (CET)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-455-ECQjD3OANbyI-DUUpnEfiw-1; Wed, 06 Nov 2024 15:38:43 -0500","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-37d59ad50f3so72316f8f.0\n\tfor ;\n\tWed, 06 Nov 2024 12:38:43 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-432aa6b30d9sm37037145e9.17.2024.11.06.12.38.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 06 Nov 2024 12:38:40 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"FBpkisxL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1730925524;\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=k19FoWID6pPwQITiQAqMPz9Y/K/VBiAGRhsNSjvbhUA=;\n\tb=FBpkisxLp5Z10Dly3hi7I6lMoPmgS6fKcDvugGAMwKoRXs562VrD427eaA1c5pbYzntuo6\n\t/ICCBUNJE71l2RbjqOqcC+vbMfMFbzKuDJ4030Y4t1txjH6B0snH1xfOFm/ePD4WfatoCa\n\tXOARGmCVTz0XvOMdcPA88h2dhVDpcUw=","X-MC-Unique":"ECQjD3OANbyI-DUUpnEfiw-1","X-Mimecast-MFC-AGG-ID":"ECQjD3OANbyI-DUUpnEfiw","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1730925522; x=1731530322;\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=k19FoWID6pPwQITiQAqMPz9Y/K/VBiAGRhsNSjvbhUA=;\n\tb=k4W5LiJimUC5kywl2q7P0/JwKv5QBuurwJvEXdNyCI+4AWj25w3VKHpT3E12uiHxsB\n\t1pEOYXDIJGoyg/vEA4Xl5wpKxfY6DvaoxKPuutV3FJjFoB0XRzYBT7OyQHTnhwCvz8rh\n\tCiW9aJ36jcX0BEFRjsvxsGN5Dkv/wxZvb76O+Tdk3xFpPNoalFkcmiczz6QFhXu7EhhT\n\tBGl6m9T+6c1NngoYkaIKWiOk51HqcRxXoeQ6UrjK9K008CdKbp5HH6hCKQdT99BYMGbP\n\tRAB/lVbuVmd5qj7QWXpVPnRBkSoRVer3xRaPg1NHUELjbSK8+sAVItvTCmNC5rcHJn31\n\tgfJQ==","X-Gm-Message-State":"AOJu0YyFTxOgzsgBVUFGKlkaktgKfMkz7GCikDcqn3CKjgCeWM5BSxnZ\n\tV8EHatE+w1cvJjmpKAqTnMDyatK7DiJZA5BibMSc2ub1gxU7/Yj6ojMdPUkCBQzdJOFk4jofp6l\n\toy1C7jRDagoVpnu0zZtilr4/2ihu2CWM+MVC/reARITKfrl4iPfLRSCoJu9QH+uLoCJr3+01IKY\n\tdm1fE=","X-Received":["by 2002:adf:b345:0:b0:37d:2d27:cd93 with SMTP id\n\tffacd0b85a97d-380612008dbmr31008552f8f.43.1730925521769; \n\tWed, 06 Nov 2024 12:38:41 -0800 (PST)","by 2002:adf:b345:0:b0:37d:2d27:cd93 with SMTP id\n\tffacd0b85a97d-380612008dbmr31008532f8f.43.1730925521313; \n\tWed, 06 Nov 2024 12:38:41 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IE1KAZ7ieB1XQ9ygWWHco0QQRcwfof+Lv7sztCOrR9g3TKbesOzjRIMKQ3qQZedfPWmb7+hRw==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v2] libcamera: software_isp: Clean up pending requests\n\ton stop","In-Reply-To":"<20241106111326.GA30053@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 6 Nov 2024 13:13:26 +0200\")","References":"<20241009083556.330325-1-mzamazal@redhat.com>\n\t<20241009142037.GE2733@pendragon.ideasonboard.com>\n\t<87a5fdqkx3.fsf@redhat.com>\n\t<20241009191955.GE16232@pendragon.ideasonboard.com>\n\t<875xq1qcy9.fsf@redhat.com>\n\t<20241010144706.GB26015@pendragon.ideasonboard.com>\n\t<8734l2wx2d.fsf@redhat.com>\n\t<20241106111326.GA30053@pendragon.ideasonboard.com>","Date":"Wed, 06 Nov 2024 21:38:39 +0100","Message-ID":"<871pzogks0.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"Hua61nve17R2ye3PNTJuJdjQh9Ym6metGNQp8nom7xo_1730925522","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\" "}}]