[{"id":17457,"web_url":"https://patchwork.libcamera.org/comment/17457/","msgid":"","date":"2021-06-07T23:00:57","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Copy\n\tIPACameraSensorInfo for future usage","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Wed, Jun 02, 2021 at 03:53:25PM +0530, Umang Jain wrote:\n> IPACameraSensorInfo members will be needed at various places in the\n> IPAIPU3 class, in subsequent commits. Hence, it seems trivial to copy\n> this structure for wider availability throughout the class.\n> \n> This commit does not introduce any functional changes.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/ipa/ipu3/ipu3.cpp | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 2496b0a0..97ddb863 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -63,6 +63,8 @@ private:\n> \n> \tControlInfoMap ctrls_;\n> \n> +\tIPACameraSensorInfo sensorInfo_;\n\nThis is OK for now, but in the not too distant future I think we should\nstore fields individually as we should probably convert them. For\ninstance, the line duration should be converted to a time unit when we\nreceive it, and then used as such, instead of converting between pixels\nand time in multiple places.\n\nJean-Michel, how about moving IPU3Agc to handling exposure time in time\nunits, and doing the conversion in ipu3.cpp ? Is this something you have\nconsidered, or maybe you're already planning it ?\n\nReviewed-by: Laurent Pinchart \n\n> +\n> \t/* Camera sensor controls. */\n> \tuint32_t exposure_;\n> \tuint32_t minExposure_;\n> @@ -144,6 +146,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \tif (configInfo.entityControls.empty())\n> \t\treturn;\n> \n> +\tsensorInfo_ = configInfo.sensorInfo;\n> +\n> \tctrls_ = configInfo.entityControls.at(0);\n> \n> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n> @@ -174,7 +178,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n> \tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n> \n> \tagcAlgo_ = std::make_unique();\n> -\tagcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo);\n> +\tagcAlgo_->initialise(bdsGrid_, sensorInfo_);\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 A2689BD22E\n\tfor ;\n\tMon, 7 Jun 2021 23:01:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 200AD6892B;\n\tTue, 8 Jun 2021 01:01:14 +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 7C7516891C\n\tfor ;\n\tTue, 8 Jun 2021 01:01:12 +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 C0B48B75;\n\tTue, 8 Jun 2021 01:01:11 +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=\"wKHukj+g\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623106871;\n\tbh=E4FnCo3REdwGN7Zd9KvnHX1Qt4W8LfREuClxjzG/IRg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wKHukj+gc+2ueBiJtCs42e+apOSGWJDzHEle8LpbVzughpMWLV1P0HMGdp+SxCI7I\n\tVo0uDSc2lqpB0qFGgvI0wCoYQzft0YgB3IZBEL35uJ1ftCP/B+sJnNgtrxF7E2F3VH\n\tXW7G/m8/J9KsCr5zD66mLZHYMH7mCHLGtTHRPPLg=","Date":"Tue, 8 Jun 2021 02:00:57 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210602102326.106549-1-umang.jain@ideasonboard.com>\n\t<20210602102326.106549-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210602102326.106549-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Copy\n\tIPACameraSensorInfo for future usage","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":17463,"web_url":"https://patchwork.libcamera.org/comment/17463/","msgid":"<544230a5-2c8d-cc37-6321-c6a8dc66921f@ideasonboard.com>","date":"2021-06-08T05:31:54","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Copy\n\tIPACameraSensorInfo for future usage","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent, Umang,\n\nOn 08/06/2021 01:00, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> On Wed, Jun 02, 2021 at 03:53:25PM +0530, Umang Jain wrote:\n>> IPACameraSensorInfo members will be needed at various places in the\n>> IPAIPU3 class, in subsequent commits. Hence, it seems trivial to copy\n>> this structure for wider availability throughout the class.\n>>\n>> This commit does not introduce any functional changes.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/ipa/ipu3/ipu3.cpp | 6 +++++-\n>> 1 file changed, 5 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 2496b0a0..97ddb863 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -63,6 +63,8 @@ private:\n>> \n>> \tControlInfoMap ctrls_;\n>> \n>> +\tIPACameraSensorInfo sensorInfo_;\n> \n> This is OK for now, but in the not too distant future I think we should\n> store fields individually as we should probably convert them. For\n> instance, the line duration should be converted to a time unit when we\n> receive it, and then used as such, instead of converting between pixels\n> and time in multiple places.\n> \n> Jean-Michel, how about moving IPU3Agc to handling exposure time in time\n> units, and doing the conversion in ipu3.cpp ? Is this something you have\n> considered, or maybe you're already planning it ?\n> \n> Reviewed-by: Laurent Pinchart \n\nI indeed have a local branch where exposure is a time unit and the line\nduration is calculated in the agcAlgo_->initialise() call.\nThose ctrls disappear from ipu3.cpp except for setting those in\nsetControls().\nThere is no need for a local variable in IPAIPU3 as IPU3Agc receives the\nconfigInfo reference directly.\nWould you prefer that new patch instead ? Or we can go with this one for\nthe moment and I will fix it when I will post the new agc algorithm series ?\n\n>> +\n>> \t/* Camera sensor controls. */\n>> \tuint32_t exposure_;\n>> \tuint32_t minExposure_;\n>> @@ -144,6 +146,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n>> \tif (configInfo.entityControls.empty())\n>> \t\treturn;\n>> \n>> +\tsensorInfo_ = configInfo.sensorInfo;\n>> +\n>> \tctrls_ = configInfo.entityControls.at(0);\n>> \n>> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n>> @@ -174,7 +178,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n>> \tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n>> \n>> \tagcAlgo_ = std::make_unique();\n>> -\tagcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo);\n>> +\tagcAlgo_->initialise(bdsGrid_, sensorInfo_);\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 177CFC3206\n\tfor ;\n\tTue, 8 Jun 2021 05:31:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 893FB6892D;\n\tTue, 8 Jun 2021 07:31:56 +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 7F8B56029D\n\tfor ;\n\tTue, 8 Jun 2021 07:31:55 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:2d50:21bc:f0b3:7d2f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E494C3E6;\n\tTue, 8 Jun 2021 07:31: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=\"ar8ifeKF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623130315;\n\tbh=AG07eS5hXBec9QU0FVBrjZOLQnx8eEH/k1Txi48SLOE=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ar8ifeKFgVy2jPs7pI2r/orPLDIKmDinqCZW5MnuJOhriHVjhgBrXCg0OayErGF7O\n\tI0rkueqToaB3maHWtlNVOI/c28vJsxMDMxl7QTa+t4quctZ2g3ZZrPYIDOclJv/qmu\n\tH4PZB1OBT1KJk8GcVyuvkDQNEqa5RD/sMaKKWBy8=","To":"Laurent Pinchart ,\n\tUmang Jain ","References":"<20210602102326.106549-1-umang.jain@ideasonboard.com>\n\t<20210602102326.106549-4-umang.jain@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","Message-ID":"<544230a5-2c8d-cc37-6321-c6a8dc66921f@ideasonboard.com>","Date":"Tue, 8 Jun 2021 07:31:54 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Copy\n\tIPACameraSensorInfo for future usage","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":17466,"web_url":"https://patchwork.libcamera.org/comment/17466/","msgid":"","date":"2021-06-08T06:18:05","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Copy\n\tIPACameraSensorInfo for future usage","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi JM.\n\nOn 6/8/21 11:01 AM, Jean-Michel Hautbois wrote:\n> Hi Laurent, Umang,\n>\n> On 08/06/2021 01:00, Laurent Pinchart wrote:\n>> Hi Umang,\n>>\n>> Thank you for the patch.\n>>\n>> On Wed, Jun 02, 2021 at 03:53:25PM +0530, Umang Jain wrote:\n>>> IPACameraSensorInfo members will be needed at various places in the\n>>> IPAIPU3 class, in subsequent commits. Hence, it seems trivial to copy\n>>> this structure for wider availability throughout the class.\n>>>\n>>> This commit does not introduce any functional changes.\n>>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> src/ipa/ipu3/ipu3.cpp | 6 +++++-\n>>> 1 file changed, 5 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index 2496b0a0..97ddb863 100644\n>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>> @@ -63,6 +63,8 @@ private:\n>>> \n>>> \tControlInfoMap ctrls_;\n>>> \n>>> +\tIPACameraSensorInfo sensorInfo_;\n>> This is OK for now, but in the not too distant future I think we should\n>> store fields individually as we should probably convert them. For\n>> instance, the line duration should be converted to a time unit when we\n>> receive it, and then used as such, instead of converting between pixels\n>> and time in multiple places.\n>>\n>> Jean-Michel, how about moving IPU3Agc to handling exposure time in time\n>> units, and doing the conversion in ipu3.cpp ? Is this something you have\n>> considered, or maybe you're already planning it ?\n>>\n>> Reviewed-by: Laurent Pinchart \n> I indeed have a local branch where exposure is a time unit and the line\n> duration is calculated in the agcAlgo_->initialise() call.\n> Those ctrls disappear from ipu3.cpp except for setting those in\n> setControls().\n> There is no need for a local variable in IPAIPU3 as IPU3Agc receives the\n> configInfo reference directly.\n> Would you prefer that new patch instead ? Or we can go with this one for\n> the moment and I will fix it when I will post the new agc algorithm series ?\n\n\nI plan to keep this as is for now assuming that the context of the \nrework, will be better when you post your agc algorithm series.\n\n>\n>>> +\n>>> \t/* Camera sensor controls. */\n>>> \tuint32_t exposure_;\n>>> \tuint32_t minExposure_;\n>>> @@ -144,6 +146,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n>>> \tif (configInfo.entityControls.empty())\n>>> \t\treturn;\n>>> \n>>> +\tsensorInfo_ = configInfo.sensorInfo;\n>>> +\n>>> \tctrls_ = configInfo.entityControls.at(0);\n>>> \n>>> \tconst auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);\n>>> @@ -174,7 +178,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)\n>>> \tawbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);\n>>> \n>>> \tagcAlgo_ = std::make_unique();\n>>> -\tagcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo);\n>>> +\tagcAlgo_->initialise(bdsGrid_, sensorInfo_);\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 C23A6BD22E\n\tfor ;\n\tTue, 8 Jun 2021 06:18:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B52F6892D;\n\tTue, 8 Jun 2021 08:18:12 +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 1F60F6029D\n\tfor ;\n\tTue, 8 Jun 2021 08:18:11 +0200 (CEST)","from [192.168.0.107] (unknown [103.251.226.98])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BA0F43E6;\n\tTue, 8 Jun 2021 08:18:09 +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=\"ijkAoMgY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623133090;\n\tbh=GwVt/QAug0bBg69RF73XMTPLzOkI9195PojBSlgh/Xc=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ijkAoMgY8MjLTbKKt6dBhuF438CFlxbcxA5HHVeo6dPMItt6D4I6HJ//2Y+MOZGLx\n\ts+WMAVsZBffqSe7xZ459XwomkbwlcriN6bvBM7/gkQdgEJN+FmW//fjAnYGF0btKbA\n\taOCqarxrX4VtlKmVHEuRzUFZXD32DUP+P+3IxRc0=","To":"Jean-Michel Hautbois ,\n\tLaurent Pinchart ","References":"<20210602102326.106549-1-umang.jain@ideasonboard.com>\n\t<20210602102326.106549-4-umang.jain@ideasonboard.com>\n\t\n\t<544230a5-2c8d-cc37-6321-c6a8dc66921f@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Tue, 8 Jun 2021 11:48:05 +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":"<544230a5-2c8d-cc37-6321-c6a8dc66921f@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: ipu3: Copy\n\tIPACameraSensorInfo for future usage","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\" "}}]