[{"id":12501,"web_url":"https://patchwork.libcamera.org/comment/12501/","msgid":"<20200914184227.GA11901@pendragon.ideasonboard.com>","date":"2020-09-14T18:42:27","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Guard\n\tagainst IPA posting actions when we have no camera","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Mon, Sep 14, 2020 at 04:02:17PM +0200, Niklas Söderlund wrote:\n> The IPA is running asynchronously from the pipeline and may be in the\n> process of completing some action while the pipeline is stopping the\n> camera. Prevent processing actions after the camera is stopped by\n> checking that the pipeline is running with an active camera or not.\n\nHave you seen this happening ? The right way to handle such issues\nshould be to stop the IPA synchronously when stopping the camera, and\nensuring that all pending messages are delivered.\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++++\n> 1 file changed, 5 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index a6fc3b8e36f3b00a..73d1e9c4ef21fd45 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -413,6 +413,11 @@ int RkISP1CameraData::loadIPA()\n> void RkISP1CameraData::queueFrameAction(unsigned int frame,\n> \t\t\t\t\tconst IPAOperationData &action)\n> {\n> +\t/* Guard again IPA queuing actions when we have no camera. */\n> +\tPipelineHandlerRkISP1 *pipe = static_cast(pipe_);\n> +\tif (!pipe->activeCamera_)\n> +\t\treturn;\n> +\n> \tswitch (action.operation) {\n> \tcase RKISP1_IPA_ACTION_V4L2_SET: {\n> \t\tconst ControlList &controls = action.controls[0];","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 C97D2BF01C\n\tfor ;\n\tMon, 14 Sep 2020 18:42:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5560D62D27;\n\tMon, 14 Sep 2020 20:42:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D18C62C8C\n\tfor ;\n\tMon, 14 Sep 2020 20:42:55 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B6E87275;\n\tMon, 14 Sep 2020 20:42:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ClpFj7zu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600108974;\n\tbh=//Es3VqLmHea4XQPVGv1tTVfz5jENhjaYYQXLmAuzVI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ClpFj7zuoc5JQkR1HNHETRl46GhHkLztHldC2L/uidogX6zM37c/ODPe2SagcWUcD\n\tMYJXZqtiGJzWZVhY9s1iFrCRoi0Z4rzlTk+wM96bxGUF8dbbUUEAd021PiEynh6Kv2\n\t7CZM538pfEE8odB5YpKOU6zKy2GMq8ys/qrB4RU0=","Date":"Mon, 14 Sep 2020 21:42:27 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200914184227.GA11901@pendragon.ideasonboard.com>","References":"<20200914140217.54060-1-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200914140217.54060-1-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Guard\n\tagainst IPA posting actions when we have no camera","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12502,"web_url":"https://patchwork.libcamera.org/comment/12502/","msgid":"<20200914220057.GN1127199@oden.dyn.berto.se>","date":"2020-09-14T22:00:57","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Guard\n\tagainst IPA posting actions when we have no camera","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-09-14 21:42:27 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Mon, Sep 14, 2020 at 04:02:17PM +0200, Niklas Söderlund wrote:\n> > The IPA is running asynchronously from the pipeline and may be in the\n> > process of completing some action while the pipeline is stopping the\n> > camera. Prevent processing actions after the camera is stopped by\n> > checking that the pipeline is running with an active camera or not.\n> \n> Have you seen this happening ? The right way to handle such issues\n> should be to stop the IPA synchronously when stopping the camera, and\n> ensuring that all pending messages are delivered.\n\nI noticed it when playing around with some local hacks but not with \nsomething real. I thought it worth a guard as right now an out-of-tree \nIPA could very well misbehave and queue actions before/after \nstart()/stop(). But maybe this is better addressed in the new IPC layer \nto make it behave the same for all pipelines/IPAs?\n\n> \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++++\n> > 1 file changed, 5 insertions(+)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index a6fc3b8e36f3b00a..73d1e9c4ef21fd45 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -413,6 +413,11 @@ int RkISP1CameraData::loadIPA()\n> > void RkISP1CameraData::queueFrameAction(unsigned int frame,\n> > \t\t\t\t\tconst IPAOperationData &action)\n> > {\n> > +\t/* Guard again IPA queuing actions when we have no camera. */\n> > +\tPipelineHandlerRkISP1 *pipe = static_cast(pipe_);\n> > +\tif (!pipe->activeCamera_)\n> > +\t\treturn;\n> > +\n> > \tswitch (action.operation) {\n> > \tcase RKISP1_IPA_ACTION_V4L2_SET: {\n> > \t\tconst ControlList &controls = action.controls[0];\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 ECB06C3B5B\n\tfor ;\n\tMon, 14 Sep 2020 22:01:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5CFA962DAB;\n\tTue, 15 Sep 2020 00:01:01 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A63F562901\n\tfor ;\n\tTue, 15 Sep 2020 00:00:59 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id b19so973048lji.11\n\tfor ;\n\tMon, 14 Sep 2020 15:00:59 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tb7sm3860028lfq.36.2020.09.14.15.00.58\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 14 Sep 2020 15:00:58 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"i4cP4Ec7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=CtJKcfNwSofUl+9qBJLLaK7wMUuahq1LK2Sz857M3BE=;\n\tb=i4cP4Ec7ciqPPa8GYrVkZuXrQHIsPY751HmKP6H/1+z8OnKGYM/29hDTOGbQ2YzXwK\n\tD1GmtL0H5FT0rWCgwVlFBUHQ547g7UlWWYclrh8lrKY9BOBveokTrNYKdGVV1NpgdIVg\n\tyq+duT4B3gUrAnJjD75t1j57F8vItbdtrHj7T5dQU6icduXwsP4wOZjh4f7aDth9mtCx\n\tVg8S+TJHsEoyaeOf+pmP8Jt2FuaxTAXEFh+VqSZZQYp4ynnESQswU71cv7YVbTln/X14\n\t2CmcoThck1SnJpwZHgviAxwnOCbLMSsDuEm/mFlgHFMfFS+YKkUAwsSD8boXs7L2iMKE\n\tAA4A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=CtJKcfNwSofUl+9qBJLLaK7wMUuahq1LK2Sz857M3BE=;\n\tb=ozPIjApEqRqwgS6eqbibsfa5t+LKYE2PZQp4+JF1iQyQqHrOrhhjSr+OjLEyt7zLUz\n\t0xv26NMO/FREavBWv7k7MfxlxBwusgZOIsLfPgWWGB88nrsC6jQIVWr9pQhvB7iPBDbV\n\tSG1wIj+Gd+P3Z9bZmq1tTAce3ZJVMKyS+cCiAaBxdxfRpolCId+p+mWn8E6yrboKXhbh\n\tc4iAPxbBn9tnpp9kri4665nBCA+KoB+fdxK1qyL76oPwHuFLrc75WzNJrfL/bVwTODtl\n\tduF2aloVJd02ktN+nRaI6q3egfqT4enIszZ9IwPDul4AFKGglDqcTf0bLfYQ/71ym6m2\n\tgeZA==","X-Gm-Message-State":"AOAM530lGYrtq65B/EwsDITuPFBHDcovYC90o2xo0KMmDTO6NAx453C0\n\tDPpTpDQEWeXLRvqVHQ1RNLB5tuKAx1oY/g==","X-Google-Smtp-Source":"ABdhPJzC8l1YITNVEtWmUcyKWGuE6eAXIf4UIrPg0huuINjsEANeNXLHOz+I2WSQiLmWnLUO2Noi3A==","X-Received":"by 2002:a2e:99d6:: with SMTP id\n\tl22mr1451247ljj.195.1600120858713; \n\tMon, 14 Sep 2020 15:00:58 -0700 (PDT)","Date":"Tue, 15 Sep 2020 00:00:57 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20200914220057.GN1127199@oden.dyn.berto.se>","References":"<20200914140217.54060-1-niklas.soderlund@ragnatech.se>\n\t<20200914184227.GA11901@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200914184227.GA11901@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Guard\n\tagainst IPA posting actions when we have no camera","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12503,"web_url":"https://patchwork.libcamera.org/comment/12503/","msgid":"<20200914220658.GA2396@pendragon.ideasonboard.com>","date":"2020-09-14T22:06:58","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Guard\n\tagainst IPA posting actions when we have no camera","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Sep 15, 2020 at 12:00:57AM +0200, Niklas Söderlund wrote:\n> On 2020-09-14 21:42:27 +0300, Laurent Pinchart wrote:\n> > On Mon, Sep 14, 2020 at 04:02:17PM +0200, Niklas Söderlund wrote:\n> > > The IPA is running asynchronously from the pipeline and may be in the\n> > > process of completing some action while the pipeline is stopping the\n> > > camera. Prevent processing actions after the camera is stopped by\n> > > checking that the pipeline is running with an active camera or not.\n> > \n> > Have you seen this happening ? The right way to handle such issues\n> > should be to stop the IPA synchronously when stopping the camera, and\n> > ensuring that all pending messages are delivered.\n> \n> I noticed it when playing around with some local hacks but not with \n> something real. I thought it worth a guard as right now an out-of-tree \n> IPA could very well misbehave and queue actions before/after \n> start()/stop(). But maybe this is better addressed in the new IPC layer \n> to make it behave the same for all pipelines/IPAs?\n\nThe IPA stop() function is called asynchronously across threads, and the\nIPA proxy waits for the IPA to reply to make the call synchronous from a\nPH point of view. All messages sent from the IPA to the PH are thus\nprocessed by the PH thread before the stop() call returns. The only\npossibility of error (if I'm not mistaken) would be if the IPA queues a\nframe action *after* returning from stop(). I think that would be a bug\nin the IPA. We could add assertions in the proxy to check for that, I\nthink that would be better than adding checks in all PHs.\n\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 +++++\n> > > 1 file changed, 5 insertions(+)\n> > > \n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index a6fc3b8e36f3b00a..73d1e9c4ef21fd45 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -413,6 +413,11 @@ int RkISP1CameraData::loadIPA()\n> > > void RkISP1CameraData::queueFrameAction(unsigned int frame,\n> > > \t\t\t\t\tconst IPAOperationData &action)\n> > > {\n> > > +\t/* Guard again IPA queuing actions when we have no camera. */\n> > > +\tPipelineHandlerRkISP1 *pipe = static_cast(pipe_);\n> > > +\tif (!pipe->activeCamera_)\n> > > +\t\treturn;\n> > > +\n> > > \tswitch (action.operation) {\n> > > \tcase RKISP1_IPA_ACTION_V4L2_SET: {\n> > > \t\tconst ControlList &controls = action.controls[0];","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 CE4B0BF01C\n\tfor ;\n\tMon, 14 Sep 2020 22:07:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63CFB62DAB;\n\tTue, 15 Sep 2020 00:07:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70E8462901\n\tfor ;\n\tTue, 15 Sep 2020 00:07:27 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 65FB7275;\n\tTue, 15 Sep 2020 00:07:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rUohk6Gh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600121246;\n\tbh=jnfhcN2fJcoiELIRRuffEsKPVk+ztyR+HyK+v6YoB7A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rUohk6GhWcq0HV77i+c7r65skcQac1QQTlsY3sWmMQSXqJVg48R9K7zJpYFmnnm2t\n\tstSSsUsaBq4wLGD82jtqgkNag6DbLO5CRR9amOJIcIsGsgEDqzKqZU9iNi66L+EBiT\n\tPz3U5bLBLb55EqnZacJnZgYoZLPevR9MMPV20iIs=","Date":"Tue, 15 Sep 2020 01:06:58 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200914220658.GA2396@pendragon.ideasonboard.com>","References":"<20200914140217.54060-1-niklas.soderlund@ragnatech.se>\n\t<20200914184227.GA11901@pendragon.ideasonboard.com>\n\t<20200914220057.GN1127199@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200914220057.GN1127199@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: rkisp1: Guard\n\tagainst IPA posting actions when we have no camera","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]