[{"id":15866,"web_url":"https://patchwork.libcamera.org/comment/15866/","msgid":"","date":"2021-03-24T15:47:01","subject":"Re: [libcamera-devel] [PATCH v3 6/6] ipa: ipu3: Do not set controls\n\tduring configure","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Wed, Mar 24, 2021 at 03:01:25PM +0000, Kieran Bingham wrote:\n> The configure operation is synchronous and should not send events back\n> to the pipeline handler.\n> \n> If information needs to be returned from configure it should be handled\n> through the interface directly.\n> \n> Move the initial call to setControls() out of configure() and into the\n> start() method which is called after the IPA running_ state is updated.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/ipu3/ipu3.cpp | 11 ++++++++---\n> 1 file changed, 8 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index a5c5e029f465..34a907f23ef5 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -32,7 +32,7 @@ public:\n> \t{\n> \t\treturn 0;\n> \t}\n> -\tint start() override { return 0; }\n> +\tint start() override;\n> \tvoid stop() override {}\n> \n> \tvoid configure(const std::map &entityControls,\n> @@ -63,6 +63,13 @@ private:\n> \tuint32_t maxGain_;\n> };\n> \n> +int IPAIPU3::start()\n> +{\n> +\tsetControls(0);\n\nThis will send an asynchronous event to the pipeline handler, which will\nprocess it after starting the device. It defeats a little bit the point\nof setting initial controls, doesn't it ?\n\nThe IPU3 IPA protocol seems ill-defined here, as it uses a frame\ncounter, and it's not clear if that counter is 0-based or 1-based.\n\nAll this doesn't matter much right now as we never call setControls() at\nruntime, so we could merge the patch as is, but please keep in mind that\nthe mechanism is likely broken, and JM would then need to fix it\n(including redesigning part of the IPA protocol) as part of his pending\nIPU3 IPA series. This will involve figuring out how to properly interact\nwith delayed controls.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> void IPAIPU3::configure(const std::map &entityControls,\n> \t\t\t[[maybe_unused]] const Size &bdsOutputSize)\n> {\n> @@ -90,8 +97,6 @@ void IPAIPU3::configure(const std::map &entityControls\n> \tminGain_ = std::max(itGain->second.min().get(), 1);\n> \tmaxGain_ = itGain->second.max().get();\n> \tgain_ = maxGain_;\n> -\n> -\tsetControls(0);\n> }\n> \n> void IPAIPU3::mapBuffers(const std::vector &buffers)","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 D830CC32E5\n\tfor ;\n\tWed, 24 Mar 2021 15:47:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3313468D62;\n\tWed, 24 Mar 2021 16:47:47 +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 94860602E3\n\tfor ;\n\tWed, 24 Mar 2021 16:47:45 +0100 (CET)","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 0F0E8580;\n\tWed, 24 Mar 2021 16:47:45 +0100 (CET)"],"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=\"nvuGYExp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616600865;\n\tbh=nRv+lyqivhBWJ+L2ZtNzQySXYTXZzdZoGvhr/rcpYSo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nvuGYExpSKcaXhx9ueBRCYEHmDry0HUQWzHyuq7832f96MaIKeT3JymreJ0chNbWD\n\tHfp6O0IqMRkwPDNn0iSjSi9u8jLmJt6l+H6Ojm5vI4EJ2+clSTc1JdV+n+y/0VYo0a\n\tqU8c9bHIZN4EH+yhnnTLbsLlx/zgbaqwbyA2TO6g=","Date":"Wed, 24 Mar 2021 17:47:01 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210324150125.1318325-1-kieran.bingham@ideasonboard.com>\n\t<20210324150125.1318325-7-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210324150125.1318325-7-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 6/6] ipa: ipu3: Do not set controls\n\tduring configure","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15869,"web_url":"https://patchwork.libcamera.org/comment/15869/","msgid":"<42c31181-8d6c-bfd9-b9cf-343eb13c4265@ideasonboard.com>","date":"2021-03-24T16:14:43","subject":"Re: [libcamera-devel] [PATCH v3 6/6] ipa: ipu3: Do not set controls\n\tduring configure","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 24/03/2021 15:47, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Wed, Mar 24, 2021 at 03:01:25PM +0000, Kieran Bingham wrote:\n>> The configure operation is synchronous and should not send events back\n>> to the pipeline handler.\n>>\n>> If information needs to be returned from configure it should be handled\n>> through the interface directly.\n>>\n>> Move the initial call to setControls() out of configure() and into the\n>> start() method which is called after the IPA running_ state is updated.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/ipa/ipu3/ipu3.cpp | 11 ++++++++---\n>> 1 file changed, 8 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index a5c5e029f465..34a907f23ef5 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -32,7 +32,7 @@ public:\n>> \t{\n>> \t\treturn 0;\n>> \t}\n>> -\tint start() override { return 0; }\n>> +\tint start() override;\n>> \tvoid stop() override {}\n>> \n>> \tvoid configure(const std::map &entityControls,\n>> @@ -63,6 +63,13 @@ private:\n>> \tuint32_t maxGain_;\n>> };\n>> \n>> +int IPAIPU3::start()\n>> +{\n>> +\tsetControls(0);\n> \n> This will send an asynchronous event to the pipeline handler, which will\n> process it after starting the device. It defeats a little bit the point\n> of setting initial controls, doesn't it ?\n\nBut it will set the controls before the first request, which was what I\nthought was the aim.\n\nI believe setControls() was just an initial skeleton implementation to\ndemonstrate how to set controls. I don't think it's really been thought\nabout correctly at all indeed.\n\nBut the aim of this patch isn't to 'fix' the design, just stop the call\nbeing made in an invalid state.\n\nThe actual design process is separate, and on-going, and I don't think\nthis is the place to design how the controls should be set.\n\nThe actual design of the IPA should determine that.\n\n\n> The IPU3 IPA protocol seems ill-defined here, as it uses a frame\n> counter, and it's not clear if that counter is 0-based or 1-based.\n> \n> All this doesn't matter much right now as we never call setControls() at\n> runtime, so we could merge the patch as is, but please keep in mind that\n> the mechanism is likely broken, and JM would then need to fix it\n> (including redesigning part of the IPA protocol) as part of his pending\n> IPU3 IPA series. This will involve figuring out how to properly interact\n> with delayed controls.\n\nThis was detailed in the cover letter:\n\n Finally patch [6/6] moves an asynchronous callback out of the\n synchronous configure() call, and into start(). This is not expected to\n be a substantial change in functionality currently, but should be\n considered in follow on developments with the IPA.\n\n\n> \n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> void IPAIPU3::configure(const std::map &entityControls,\n>> \t\t\t[[maybe_unused]] const Size &bdsOutputSize)\n>> {\n>> @@ -90,8 +97,6 @@ void IPAIPU3::configure(const std::map &entityControls\n>> \tminGain_ = std::max(itGain->second.min().get(), 1);\n>> \tmaxGain_ = itGain->second.max().get();\n>> \tgain_ = maxGain_;\n>> -\n>> -\tsetControls(0);\n>> }\n>> \n>> void IPAIPU3::mapBuffers(const std::vector &buffers)\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 D46EEC32E7\n\tfor ;\n\tWed, 24 Mar 2021 16:14:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4971F602E3;\n\tWed, 24 Mar 2021 17:14:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3F4CE602E3\n\tfor ;\n\tWed, 24 Mar 2021 17:14:47 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CAFF4580;\n\tWed, 24 Mar 2021 17:14:45 +0100 (CET)"],"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=\"bta1WyS1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616602486;\n\tbh=vlA2F2OjrdW6SRWbkNFT4KDREJIHGmteERW3Hj7Nb+U=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=bta1WyS1IG8eErIYJYY0kxXd0aI16oBCnA20t2t4sm7scgbLvQtKz5R66dKw4xk+z\n\teWvJD/n6GAMNxSbX/fGWGHq8ca1FnTer2swmuZ+IkzAQHOJIFKaqdACDuhK4OD7jq1\n\tiK2p2U481gRxhr7a9QJr1mX6Ct25GasleOaPSwbQ=","To":"Laurent Pinchart ","References":"<20210324150125.1318325-1-kieran.bingham@ideasonboard.com>\n\t<20210324150125.1318325-7-kieran.bingham@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<42c31181-8d6c-bfd9-b9cf-343eb13c4265@ideasonboard.com>","Date":"Wed, 24 Mar 2021 16:14:43 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 6/6] ipa: ipu3: Do not set controls\n\tduring configure","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15873,"web_url":"https://patchwork.libcamera.org/comment/15873/","msgid":"","date":"2021-03-24T16:40:02","subject":"Re: [libcamera-devel] [PATCH v3 6/6] ipa: ipu3: Do not set controls\n\tduring configure","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Mar 24, 2021 at 04:14:43PM +0000, Kieran Bingham wrote:\n> On 24/03/2021 15:47, Laurent Pinchart wrote:\n> > On Wed, Mar 24, 2021 at 03:01:25PM +0000, Kieran Bingham wrote:\n> >> The configure operation is synchronous and should not send events back\n> >> to the pipeline handler.\n> >>\n> >> If information needs to be returned from configure it should be handled\n> >> through the interface directly.\n> >>\n> >> Move the initial call to setControls() out of configure() and into the\n> >> start() method which is called after the IPA running_ state is updated.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> src/ipa/ipu3/ipu3.cpp | 11 ++++++++---\n> >> 1 file changed, 8 insertions(+), 3 deletions(-)\n> >>\n> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >> index a5c5e029f465..34a907f23ef5 100644\n> >> --- a/src/ipa/ipu3/ipu3.cpp\n> >> +++ b/src/ipa/ipu3/ipu3.cpp\n> >> @@ -32,7 +32,7 @@ public:\n> >> \t{\n> >> \t\treturn 0;\n> >> \t}\n> >> -\tint start() override { return 0; }\n> >> +\tint start() override;\n> >> \tvoid stop() override {}\n> >> \n> >> \tvoid configure(const std::map &entityControls,\n> >> @@ -63,6 +63,13 @@ private:\n> >> \tuint32_t maxGain_;\n> >> };\n> >> \n> >> +int IPAIPU3::start()\n> >> +{\n> >> +\tsetControls(0);\n> > \n> > This will send an asynchronous event to the pipeline handler, which will\n> > process it after starting the device. It defeats a little bit the point\n> > of setting initial controls, doesn't it ?\n> \n> But it will set the controls before the first request, which was what I\n> thought was the aim.\n\nThat depends on how fast the first request is queued after start()\nthough, as the event from the IPA could race with the application and\ncamera manager thread. Even if the controls are set before the first\nrequest is queued, if the sensor is started before setting controls,\nthen the first controls will be subject to delays, while if they're set\nwhen the sensor is stopped, they will be taken into account immediately.\n\nWe don't have to handle all of this as part of this patch, but I want\neverybody to be aware of this issue as it will become a problem pretty\nquickly.\n\n> I believe setControls() was just an initial skeleton implementation to\n> demonstrate how to set controls. I don't think it's really been thought\n> about correctly at all indeed.\n\nWe agree on that, yes. One option could be to just drop the\nsetControls() call too, until Jean-Michel's patches get merged :-)\n\n> But the aim of this patch isn't to 'fix' the design, just stop the call\n> being made in an invalid state.\n> \n> The actual design process is separate, and on-going, and I don't think\n> this is the place to design how the controls should be set.\n> \n> The actual design of the IPA should determine that.\n> \n> > The IPU3 IPA protocol seems ill-defined here, as it uses a frame\n> > counter, and it's not clear if that counter is 0-based or 1-based.\n> > \n> > All this doesn't matter much right now as we never call setControls() at\n> > runtime, so we could merge the patch as is, but please keep in mind that\n> > the mechanism is likely broken, and JM would then need to fix it\n> > (including redesigning part of the IPA protocol) as part of his pending\n> > IPU3 IPA series. This will involve figuring out how to properly interact\n> > with delayed controls.\n> \n> This was detailed in the cover letter:\n> \n> Finally patch [6/6] moves an asynchronous callback out of the\n> synchronous configure() call, and into start(). This is not expected to\n> be a substantial change in functionality currently, but should be\n> considered in follow on developments with the IPA.\n\nI should have read the cover letter first :-S\n\nNow that we're all aware that there's an issue that needs to be fixed\nlater,\n\nReviewed-by: Laurent Pinchart \n\nAs an initial idea, fixing the issue correctly could involve returning\ninitial controls from the start() function instead of going through a\nseparate async call.\n\nAnother thing that needs to be taken into account is that, in the\nisolated case, the event sent by setControls() (with this patch applied)\nwill be received by the libcamera process before the IPC reply to the\nstart() call. This means that we may want to forbid sending any\nasynchronous events from start() (with appropriate documentation).\n\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> void IPAIPU3::configure(const std::map &entityControls,\n> >> \t\t\t[[maybe_unused]] const Size &bdsOutputSize)\n> >> {\n> >> @@ -90,8 +97,6 @@ void IPAIPU3::configure(const std::map &entityControls\n> >> \tminGain_ = std::max(itGain->second.min().get(), 1);\n> >> \tmaxGain_ = itGain->second.max().get();\n> >> \tgain_ = maxGain_;\n> >> -\n> >> -\tsetControls(0);\n> >> }\n> >> \n> >> void IPAIPU3::mapBuffers(const std::vector &buffers)","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 D709BC32E7\n\tfor ;\n\tWed, 24 Mar 2021 16:40:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 56E136051A;\n\tWed, 24 Mar 2021 17:40:48 +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 C4B946051A\n\tfor ;\n\tWed, 24 Mar 2021 17:40:46 +0100 (CET)","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 CAD06580;\n\tWed, 24 Mar 2021 17:40:45 +0100 (CET)"],"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=\"DuvpcWoe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616604046;\n\tbh=zVr19ELCdlqNHDdoyUKh8ypDf/k4wbv2nGLH12oonkk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DuvpcWoew3wmq3bn37yqbcYiI2ZpKp1W+x8V14hWeCIMB171IY/zkguDOK0KLOmOY\n\tKmKtvfkRMRUSb1oFM65q85WsULG8YAVZq8oS9/zwOii9kFAN91zs8Xwv9HcEs0Kmrm\n\tU2c/oYkfcvnN0CA3vejE9TLo9MxRaNvOOmlVp6BY=","Date":"Wed, 24 Mar 2021 18:40:02 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210324150125.1318325-1-kieran.bingham@ideasonboard.com>\n\t<20210324150125.1318325-7-kieran.bingham@ideasonboard.com>\n\t\n\t<42c31181-8d6c-bfd9-b9cf-343eb13c4265@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<42c31181-8d6c-bfd9-b9cf-343eb13c4265@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 6/6] ipa: ipu3: Do not set controls\n\tduring configure","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15874,"web_url":"https://patchwork.libcamera.org/comment/15874/","msgid":"<0e2808c5-02cd-5056-a14c-acc99d5bcfed@ideasonboard.com>","date":"2021-03-24T16:44:00","subject":"Re: [libcamera-devel] [PATCH v3 6/6] ipa: ipu3: Do not set controls\n\tduring configure","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 24/03/2021 16:40, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Wed, Mar 24, 2021 at 04:14:43PM +0000, Kieran Bingham wrote:\n>> On 24/03/2021 15:47, Laurent Pinchart wrote:\n>>> On Wed, Mar 24, 2021 at 03:01:25PM +0000, Kieran Bingham wrote:\n>>>> The configure operation is synchronous and should not send events back\n>>>> to the pipeline handler.\n>>>>\n>>>> If information needs to be returned from configure it should be handled\n>>>> through the interface directly.\n>>>>\n>>>> Move the initial call to setControls() out of configure() and into the\n>>>> start() method which is called after the IPA running_ state is updated.\n>>>>\n>>>> Signed-off-by: Kieran Bingham \n>>>> ---\n>>>> src/ipa/ipu3/ipu3.cpp | 11 ++++++++---\n>>>> 1 file changed, 8 insertions(+), 3 deletions(-)\n>>>>\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index a5c5e029f465..34a907f23ef5 100644\n>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>> @@ -32,7 +32,7 @@ public:\n>>>> \t{\n>>>> \t\treturn 0;\n>>>> \t}\n>>>> -\tint start() override { return 0; }\n>>>> +\tint start() override;\n>>>> \tvoid stop() override {}\n>>>> \n>>>> \tvoid configure(const std::map &entityControls,\n>>>> @@ -63,6 +63,13 @@ private:\n>>>> \tuint32_t maxGain_;\n>>>> };\n>>>> \n>>>> +int IPAIPU3::start()\n>>>> +{\n>>>> +\tsetControls(0);\n>>>\n>>> This will send an asynchronous event to the pipeline handler, which will\n>>> process it after starting the device. It defeats a little bit the point\n>>> of setting initial controls, doesn't it ?\n>>\n>> But it will set the controls before the first request, which was what I\n>> thought was the aim.\n> \n> That depends on how fast the first request is queued after start()\n> though, as the event from the IPA could race with the application and\n> camera manager thread. Even if the controls are set before the first\n> request is queued, if the sensor is started before setting controls,\n> then the first controls will be subject to delays, while if they're set\n> when the sensor is stopped, they will be taken into account immediately.\n> \n> We don't have to handle all of this as part of this patch, but I want\n> everybody to be aware of this issue as it will become a problem pretty\n> quickly.\n> \n>> I believe setControls() was just an initial skeleton implementation to\n>> demonstrate how to set controls. I don't think it's really been thought\n>> about correctly at all indeed.\n> \n> We agree on that, yes. One option could be to just drop the\n> setControls() call too, until Jean-Michel's patches get merged :-)\n> \n>> But the aim of this patch isn't to 'fix' the design, just stop the call\n>> being made in an invalid state.\n>>\n>> The actual design process is separate, and on-going, and I don't think\n>> this is the place to design how the controls should be set.\n>>\n>> The actual design of the IPA should determine that.\n>>\n>>> The IPU3 IPA protocol seems ill-defined here, as it uses a frame\n>>> counter, and it's not clear if that counter is 0-based or 1-based.\n>>>\n>>> All this doesn't matter much right now as we never call setControls() at\n>>> runtime, so we could merge the patch as is, but please keep in mind that\n>>> the mechanism is likely broken, and JM would then need to fix it\n>>> (including redesigning part of the IPA protocol) as part of his pending\n>>> IPU3 IPA series. This will involve figuring out how to properly interact\n>>> with delayed controls.\n>>\n>> This was detailed in the cover letter:\n>>\n>> Finally patch [6/6] moves an asynchronous callback out of the\n>> synchronous configure() call, and into start(). This is not expected to\n>> be a substantial change in functionality currently, but should be\n>> considered in follow on developments with the IPA.\n> \n> I should have read the cover letter first :-S\n> \n> Now that we're all aware that there's an issue that needs to be fixed\n> later,\n> \n> Reviewed-by: Laurent Pinchart \n> \n> As an initial idea, fixing the issue correctly could involve returning\n> initial controls from the start() function instead of going through a\n> separate async call.\n> \n> Another thing that needs to be taken into account is that, in the\n> isolated case, the event sent by setControls() (with this patch applied)\n> will be received by the libcamera process before the IPC reply to the\n> start() call. This means that we may want to forbid sending any\n> asynchronous events from start() (with appropriate documentation).\n\nIf we want to do that, we can change the running_ state after start()\ncompletes, rather than before.\n\n(There's already a patch to change running_ to state_ in my next series,\nso lets try to remember to discuss that there).\n\n\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> void IPAIPU3::configure(const std::map &entityControls,\n>>>> \t\t\t[[maybe_unused]] const Size &bdsOutputSize)\n>>>> {\n>>>> @@ -90,8 +97,6 @@ void IPAIPU3::configure(const std::map &entityControls\n>>>> \tminGain_ = std::max(itGain->second.min().get(), 1);\n>>>> \tmaxGain_ = itGain->second.max().get();\n>>>> \tgain_ = maxGain_;\n>>>> -\n>>>> -\tsetControls(0);\n>>>> }\n>>>> \n>>>> void IPAIPU3::mapBuffers(const std::vector &buffers)\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 796FFC32E5\n\tfor ;\n\tWed, 24 Mar 2021 16:44:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E99B668D69;\n\tWed, 24 Mar 2021 17:44:04 +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 B640C6051A\n\tfor ;\n\tWed, 24 Mar 2021 17:44:03 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 223CE580;\n\tWed, 24 Mar 2021 17:44:03 +0100 (CET)"],"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=\"Gpj5npkc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616604243;\n\tbh=67lTb5bOAYpmJGBKEPH74KTB5W9vkKk3RJJSTX94SMs=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Gpj5npkcO0/JOOsPktUsFuwb20/uaSes8w2sqgJUQtbfhK+uI4yJI7LsxZD61XRsP\n\tuuBrGvD3xspBWjcazJHjB+CflAicpJYaytNJ5ADpQtdgcP5sWVPK9hiIwFNKHiMNht\n\twiD4zTIihAhRRdW1aVRcNqbV7Q9VPX8idgHKCq34=","To":"Laurent Pinchart ","References":"<20210324150125.1318325-1-kieran.bingham@ideasonboard.com>\n\t<20210324150125.1318325-7-kieran.bingham@ideasonboard.com>\n\t\n\t<42c31181-8d6c-bfd9-b9cf-343eb13c4265@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<0e2808c5-02cd-5056-a14c-acc99d5bcfed@ideasonboard.com>","Date":"Wed, 24 Mar 2021 16:44:00 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 6/6] ipa: ipu3: Do not set controls\n\tduring configure","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]