[{"id":26929,"web_url":"https://patchwork.libcamera.org/comment/26929/","msgid":"","date":"2023-04-24T07:46:41","subject":"Re: [libcamera-devel] [PATCH 1/3] apps: cam: kms_sink: Do not\n\tprocess requests after stop()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 4/24/23 6:35 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Apr 24, 2023 at 02:09:29AM +0530, Umang Jain via libcamera-devel wrote:\n>> KMSSink might process completed page flip requests from DRM\n>> after stop() has been called. This is not right hence connect the\n>> Device::requestCompleted signal on start() and disconnect it on stop().\n> I wonder if this doesn't hide another issue. The stop() function queues\n> a synchronous request to disable the display pipeline. This should flush\n> all the other in-flight requests. If another request completes after\n> that, it may indicate that we queue it after calling stop(), which I'd\n> rather fix than hiding the problem.\n\nOk - partially agreeing with you here.\n\nI agree the application should not queue requests if the sink has \nstopped.  But if they do (erratic behaviour by the app), KMSSink should \nreturn false in processRequest() instead of actually processing it.  It \ncan be implemented with state (isStopped_) boolean check or similar.\n\n\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/apps/cam/kms_sink.cpp | 5 ++++-\n>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp\n>> index 353209cd..a508977d 100644\n>> --- a/src/apps/cam/kms_sink.cpp\n>> +++ b/src/apps/cam/kms_sink.cpp\n>> @@ -63,7 +63,6 @@ KMSSink::KMSSink(const std::string &connectorName)\n>> \t\treturn;\n>> \t}\n>> \n>> -\tdev_.requestComplete.connect(this, &KMSSink::requestComplete);\n>> }\n>> \n>> void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)\n>> @@ -328,11 +327,15 @@ int KMSSink::start()\n>> \t\treturn ret;\n>> \t}\n>> \n>> +\tdev_.requestComplete.connect(this, &KMSSink::requestComplete);\n>> +\n>> \treturn 0;\n>> }\n>> \n>> int KMSSink::stop()\n>> {\n>> +\tdev_.requestComplete.disconnect();\n>> +\n>> \t/* Display pipeline. */\n>> \tDRM::AtomicRequest request(&dev_);\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 F0DDDBE173\n\tfor ;\n\tMon, 24 Apr 2023 07:46:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 463E2627D1;\n\tMon, 24 Apr 2023 09:46:55 +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 C1A00627C4\n\tfor ;\n\tMon, 24 Apr 2023 09:46:47 +0200 (CEST)","from [IPV6:2401:4900:1c80:80b3:1513:623f:d6d2:90ed] (unknown\n\t[IPv6:2401:4900:1c80:80b3:1513:623f:d6d2:90ed])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3EABBD97;\n\tMon, 24 Apr 2023 09:46:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682322415;\n\tbh=ZIpkmupPbX4BamrLB4Wzten2weHuzTo1H+2CyxjS6A0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=M23Jz2cQoWkLYhUUIgCdR1tl8LYxdf7kJ5L2D3qtbl1co2Z2xkGtG02j7i6HULqbq\n\tnF0w5c+9qpQIHAHsEerabXR2sJSKBEmdu/HXGpGEeKLS1JAIquOTf05Ik7UtCaOQ7d\n\tvqA5DDfWwUHyVAu7uQgRyDRfERl8++ZX/NwYZ8211RhlPGGKlqrhMPNL5pg/WAPYOr\n\tzyE2JuIuXvRwuKrQLuBFL1pJeXV/mSe1i/J0sL+Iqqd/zmLy9Z+ylFVZuboSEugdYs\n\tseeWa5RTtgYJi4PrmavAQhw5fNLW5d7g6LOEIYvt+zvpiAHRGaVnpwfiqtzgEMNfoM\n\tSmNY7HtdYdc+g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682322397;\n\tbh=ZIpkmupPbX4BamrLB4Wzten2weHuzTo1H+2CyxjS6A0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=e2TWXhjMVRTmDhYSo8iTxd66YDpU1exvY55TpuAbkVRF0iFjnjIrUCRu89zhR7WiR\n\tjiLml8viAA3pgBFW60sxR4KM5RRI7/cgJXnV4BLEaKePyM9Xvpu0M2dJmlaOT1eXlT\n\tdI7/oFgV+J8kOh80LKT2TJn27F3IoHtfU2XJlnac="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"e2TWXhjM\"; dkim-atps=neutral","Message-ID":"","Date":"Mon, 24 Apr 2023 13:16:41 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","To":"Laurent Pinchart ","References":"<20230423203931.108022-1-umang.jain@ideasonboard.com>\n\t<20230423203931.108022-2-umang.jain@ideasonboard.com>\n\t<20230424010503.GP21943@pendragon.ideasonboard.com>","Content-Language":"en-US","In-Reply-To":"<20230424010503.GP21943@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/3] apps: cam: kms_sink: Do not\n\tprocess requests after stop()","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26930,"web_url":"https://patchwork.libcamera.org/comment/26930/","msgid":"<20230424082530.GJ4926@pendragon.ideasonboard.com>","date":"2023-04-24T08:25:30","subject":"Re: [libcamera-devel] [PATCH 1/3] apps: cam: kms_sink: Do not\n\tprocess requests after stop()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Apr 24, 2023 at 01:16:41PM +0530, Umang Jain wrote:\n> On 4/24/23 6:35 AM, Laurent Pinchart wrote:\n> > On Mon, Apr 24, 2023 at 02:09:29AM +0530, Umang Jain via libcamera-devel wrote:\n> >> KMSSink might process completed page flip requests from DRM\n> >> after stop() has been called. This is not right hence connect the\n> >> Device::requestCompleted signal on start() and disconnect it on stop().\n> >\n> > I wonder if this doesn't hide another issue. The stop() function queues\n> > a synchronous request to disable the display pipeline. This should flush\n> > all the other in-flight requests. If another request completes after\n> > that, it may indicate that we queue it after calling stop(), which I'd\n> > rather fix than hiding the problem.\n> \n> Ok - partially agreeing with you here.\n> \n> I agree the application should not queue requests if the sink has \n> stopped.  But if they do (erratic behaviour by the app), KMSSink should \n> return false in processRequest() instead of actually processing it.  It \n> can be implemented with state (isStopped_) boolean check or similar.\n\nI'm fine with catching the issue in KMSSink as well as fixing the\ncaller.\n\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/apps/cam/kms_sink.cpp | 5 ++++-\n> >> 1 file changed, 4 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp\n> >> index 353209cd..a508977d 100644\n> >> --- a/src/apps/cam/kms_sink.cpp\n> >> +++ b/src/apps/cam/kms_sink.cpp\n> >> @@ -63,7 +63,6 @@ KMSSink::KMSSink(const std::string &connectorName)\n> >> \t\treturn;\n> >> \t}\n> >> \n> >> -\tdev_.requestComplete.connect(this, &KMSSink::requestComplete);\n> >> }\n> >> \n> >> void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer)\n> >> @@ -328,11 +327,15 @@ int KMSSink::start()\n> >> \t\treturn ret;\n> >> \t}\n> >> \n> >> +\tdev_.requestComplete.connect(this, &KMSSink::requestComplete);\n> >> +\n> >> \treturn 0;\n> >> }\n> >> \n> >> int KMSSink::stop()\n> >> {\n> >> +\tdev_.requestComplete.disconnect();\n> >> +\n> >> \t/* Display pipeline. */\n> >> \tDRM::AtomicRequest request(&dev_);\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 231A2C3272\n\tfor ;\n\tMon, 24 Apr 2023 08:25:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7EFF7627DB;\n\tMon, 24 Apr 2023 10:25:20 +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 41BD4627C4\n\tfor ;\n\tMon, 24 Apr 2023 10:25:19 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 54C709DE;\n\tMon, 24 Apr 2023 10:25:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682324720;\n\tbh=dkCjocgUG/njpuVFopNsSqsfSiubyl7dSo9zjyiWOoE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=O1sfNXxEev5x+NAGcpePDEg3GIazUvoDojZ6DT7vJM1Q/lL+rKPwFVFWQq34aK2iQ\n\t0SbTZ9zlI4yPwwzirZ+AmNMnNCtVfLF1cfq8n3xGYyRnHuUUvH774ABw8OLUJxdNA4\n\t2QLmtQWYqC6ULZ7M1k7FIJTAi0bEcPrn7ilOcSRwhF1DWvRnjNFLDckmbCStCNyWfe\n\t8KrZthH7k/mx0mDpb/ggBJ+1g7FfJPXD5Dd9YHByI2HczDuejiqQs6g/VNtD6RByaS\n\tgh1DhrLPJjj81sn4c8D1NlRwNL/J2IKzy3nJ9Ksq9mpIEbxARFjqy9a+YNR5nbGMaK\n\thCO/0m5U0LJpA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682324709;\n\tbh=dkCjocgUG/njpuVFopNsSqsfSiubyl7dSo9zjyiWOoE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cBjnA2VCxvJWG0j2oNMs65VOPzG6eTqfrwBPe2+BK/5WqRHrNQdVeTHZvreEW22V8\n\t8fzOJ0YvXbADRlQS26fDTyKnVq7xDv9LhYgCeKlAQ3BrSzYiMH89Fah31mD+XtIGQZ\n\tVXIhBZKSIUcsEbM1slxtoniHGR/zff1/n0kQBxrI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cBjnA2VC\"; dkim-atps=neutral","Date":"Mon, 24 Apr 2023 11:25:30 +0300","To":"Umang Jain ","Message-ID":"<20230424082530.GJ4926@pendragon.ideasonboard.com>","References":"<20230423203931.108022-1-umang.jain@ideasonboard.com>\n\t<20230423203931.108022-2-umang.jain@ideasonboard.com>\n\t<20230424010503.GP21943@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/3] apps: cam: kms_sink: Do not\n\tprocess requests after stop()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]