[{"id":18026,"web_url":"https://patchwork.libcamera.org/comment/18026/","msgid":"<20210708100547.GI2510@pyrite.rasen.tech>","date":"2021-07-08T10:05:47","subject":"Re: [libcamera-devel] [PATCH 2/2] utils: ipc: proxy: Always reset\n\tControlSerializer during IPA configure","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Umang,\n\nOn Thu, Jul 08, 2021 at 01:51:45PM +0530, Umang Jain wrote:\n> ControlSerializer should be reset during IPA (re)configuration,\n> so that it doesn't look up stale deserialized cache built from\n> consecutive previous runs. This is already recommended in\n> ControlSerializer docs but the implementation seems missing.\n> \n> The stale cache lookup seems to the core issue with Bug #58.\n\nOh, I didn't notice that this was the issue. If it is, then this patch\nought to fix the issue indeed.\n\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=58\n> Signed-off-by: Umang Jain \n> ---\n> .../generators/libcamera_templates/module_ipa_proxy.cpp.tmpl | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> index 5a64fe9c..3d27067a 100644\n> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n> @@ -125,6 +125,10 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n> {% for method in interface_main.methods %}\n> {{proxy_funcs.func_sig(proxy_name, method)}}\n> {\n> +{%- if method.mojom_name == \"configure\" %}\n> +\tcontrolSerializer_.reset();\n> +{%- endif %}\n> +\n\nI don't think we need to run this when we're not isolated, so probably\nmoving this to below\n\n{{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n\nis better.\n\nI haven't tested it, but with the suggested change, looks good to me.\n\nReviewed-by: Paul Elder \n\n\n> \tif (isolate_)\n> \t\t{{\"return \" if method|method_return_value != \"void\"}}{{method.mojom_name}}IPC(\n> {%- for param in method|method_param_names -%}\n> -- \n> 2.31.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 47357BD794\n\tfor ;\n\tThu, 8 Jul 2021 10:05:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 832986851B;\n\tThu, 8 Jul 2021 12:05:57 +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 3D461684E7\n\tfor ;\n\tThu, 8 Jul 2021 12:05:56 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 93CB7E7;\n\tThu, 8 Jul 2021 12:05: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=\"ZPO62nhw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625738755;\n\tbh=MyLiLwHawcFR3lkS5Fb45p/lSCvtRqqagPxtbYbi8Tk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZPO62nhw+b3/9B+H6FNRLanIDi8ZLbdpKuQli3uq1rDdNsh3H/sG6hcTgshPf8zOM\n\twaqJw8rvpOlKGVipuuOBGhOQDBVKGFAFGxlg5i4BXLmq51L03EGcFVtT0AzIVcuT92\n\t2yuGPxc073Q5CUeO77rHfYAI3/BYwOt2H2ruKpAA=","Date":"Thu, 8 Jul 2021 19:05:47 +0900","From":"paul.elder@ideasonboard.com","To":"Umang Jain ","Message-ID":"<20210708100547.GI2510@pyrite.rasen.tech>","References":"<20210708082145.122160-1-umang.jain@ideasonboard.com>\n\t<20210708082145.122160-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210708082145.122160-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] utils: ipc: proxy: Always reset\n\tControlSerializer during IPA 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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18027,"web_url":"https://patchwork.libcamera.org/comment/18027/","msgid":"","date":"2021-07-08T10:09:53","subject":"Re: [libcamera-devel] [PATCH 2/2] utils: ipc: proxy: Always reset\n\tControlSerializer during IPA configure","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nOn 7/8/21 3:35 PM, paul.elder@ideasonboard.com wrote:\n> Hi Umang,\n>\n> On Thu, Jul 08, 2021 at 01:51:45PM +0530, Umang Jain wrote:\n>> ControlSerializer should be reset during IPA (re)configuration,\n>> so that it doesn't look up stale deserialized cache built from\n>> consecutive previous runs. This is already recommended in\n>> ControlSerializer docs but the implementation seems missing.\n>>\n>> The stale cache lookup seems to the core issue with Bug #58.\n> Oh, I didn't notice that this was the issue. If it is, then this patch\n> ought to fix the issue indeed.\n>\n>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=58\n>> Signed-off-by: Umang Jain \n>> ---\n>> .../generators/libcamera_templates/module_ipa_proxy.cpp.tmpl | 4 ++++\n>> 1 file changed, 4 insertions(+)\n>>\n>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>> index 5a64fe9c..3d27067a 100644\n>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl\n>> @@ -125,6 +125,10 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)\n>> {% for method in interface_main.methods %}\n>> {{proxy_funcs.func_sig(proxy_name, method)}}\n>> {\n>> +{%- if method.mojom_name == \"configure\" %}\n>> +\tcontrolSerializer_.reset();\n>> +{%- endif %}\n>> +\n> I don't think we need to run this when we're not isolated, so probably\n> moving this to below\n>\n> {{proxy_funcs.func_sig(proxy_name, method, \"IPC\")}}\n>\n> is better.\n\nOkay, as I understand from broad level, no \nserialization/de-serialization happens during non-isolated mode,\n\nso ControlSerializer is entirely out the picture in that case.\n\n>\n> I haven't tested it, but with the suggested change, looks good to me.\n>\n> Reviewed-by: Paul Elder \n>\n>\n>> \tif (isolate_)\n>> \t\t{{\"return \" if method|method_return_value != \"void\"}}{{method.mojom_name}}IPC(\n>> {%- for param in method|method_param_names -%}\n>> -- \n>> 2.31.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 F37B0C3224\n\tfor ;\n\tThu, 8 Jul 2021 10:10:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E959684E7;\n\tThu, 8 Jul 2021 12:10:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 217B5684E7\n\tfor ;\n\tThu, 8 Jul 2021 12:09:59 +0200 (CEST)","from [192.168.0.107] (unknown [103.251.226.59])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2ABD6E7;\n\tThu, 8 Jul 2021 12:09:57 +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=\"Vm5EC6V6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625738998;\n\tbh=vxGR+hgXZuSZ8meFX7nu6Qoi1ZD48Az5WBmYXbvH6kk=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Vm5EC6V6O7I9JRFwdDVJfaWlShBq4fZA5bmLXdAP0rLD5C5xmzjnNqCXwz75h42f4\n\t5IXurPsavBOHE23LBrktEOqQJm5JAPrjSH/eKYyrsEXRPLk1KCrLsmf8CeOSlj3S8O\n\tGrn4p5L02i5BwpnNFSco0Hv7rCN8XYvCF8UQKpus=","To":"paul.elder@ideasonboard.com","References":"<20210708082145.122160-1-umang.jain@ideasonboard.com>\n\t<20210708082145.122160-3-umang.jain@ideasonboard.com>\n\t<20210708100547.GI2510@pyrite.rasen.tech>","From":"Umang Jain ","Message-ID":"","Date":"Thu, 8 Jul 2021 15:39:53 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210708100547.GI2510@pyrite.rasen.tech>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 2/2] utils: ipc: proxy: Always reset\n\tControlSerializer during IPA 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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]