[{"id":20832,"web_url":"https://patchwork.libcamera.org/comment/20832/","msgid":"<163658584909.2121661.10352661553784463627@Monstersaurus>","date":"2021-11-10T23:10:49","subject":"Re: [libcamera-devel] [PATCH v2 12/14] ipa: ipu3: Cache line\n\tduration at configure call","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2021-11-10 19:58:59)\n> We use the line duration several times in the IPAIPU3. Instead of\n> recalculating it each time, cache the value as a utils::Duration. When\n> the value is needed in double, call the intern count() function to get\n> it.\n\ncaching it I like ... using the internal count() ... sounds scary...\n\nOh ... in fact, it does look like the 'correct' way to do this.\nBut I think that's a limitation on our implementation of\nutils::Duration.\n\nI think we can do:\n\tmaxShutterSpeed = maxExposure * lineDuration.get();\n\nBut that essentially just casts a std::nano to a std::nano and then\ncalls .count() on it ... so that's a bit redundant compared to what\nyou're already doing...\n\nPerhaps we should have our Duration class implement a default .get()\nwhich always returns nanoseconds.... (and simply calls .count() or an\nexplicit operator double() ... ?)\n\nAnyway, that isn't a fault of this patch as far as I can tell - so lets\nconsider that on top.\n\nIf you prefer the explicit named cast with .get() I think\nit's ok to use that as the compiler will probably optimise it ... but\neither way is up to you I think...\n\nReviewed-by: Kieran Bingham \n\n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3.cpp | 17 +++++++++--------\n> 1 file changed, 9 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index eb8c0dc4..68a97e8e 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -174,6 +174,8 @@ private:\n> uint32_t minGain_;\n> uint32_t maxGain_;\n> \n> + utils::Duration lineDuration_;\n> +\n> /* Interface to the Camera Helper */\n> std::unique_ptr camHelper_;\n> \n> @@ -195,8 +197,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,\n> int32_t minExposure = v4l2Exposure.min().get();\n> int32_t maxExposure = v4l2Exposure.max().get();\n> \n> - utils::Duration lineDuration = sensorInfo.lineLength * 1.0s\n> - / sensorInfo.pixelRate;\n> + lineDuration_ = sensorInfo.lineLength * 1.0s\n> + / sensorInfo.pixelRate;\n> \n> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> int32_t minGain = v4l2Gain.min().get();\n> @@ -209,8 +211,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,\n> *\n> * \\todo take VBLANK into account for maximum shutter speed\n> */\n> - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration;\n> - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration;\n> + context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_;\n> + context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_;\n> context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> }\n> @@ -239,11 +241,10 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n> * exposure min, max and default and convert it from lines to\n> * microseconds.\n> */\n> - double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);\n\nI like that we get rid of this 1e6 though ;-)\n\n> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> - int32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> - int32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> - int32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> + int32_t minExposure = v4l2Exposure.min().get() * lineDuration_.count();\n> + int32_t maxExposure = v4l2Exposure.max().get() * lineDuration_.count();\n> + int32_t defExposure = v4l2Exposure.def().get() * lineDuration_.count();\n> controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> defExposure);\n> \n> -- \n> 2.32.0\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 5C115BF415\n\tfor ;\n\tWed, 10 Nov 2021 23:10:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 811E46035D;\n\tThu, 11 Nov 2021 00:10:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C889E600BF\n\tfor ;\n\tThu, 11 Nov 2021 00:10:51 +0100 (CET)","from pendragon.ideasonboard.com\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 5DFE2292;\n\tThu, 11 Nov 2021 00:10:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"P+4kazw9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636585851;\n\tbh=K3GxoAx3IgYS2Phxs/FSJYr2oKnLkLhyjmmjHxGSJK8=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=P+4kazw9fmkQ+pSXTtHAM2bcEjrVPIQRHnNCTKE1WhlyTwjQ8dQ6QfZBoU4xo4QWQ\n\tmU2xRtyAzrdomwjnYRjM+sV9uJ3r6x2q6PYpQFkA56FOHdNoRn02q0Iu4Dgp9+i571\n\tWxy3IekEvs7DZRbtZLFVeeWO0rh6qkn83wrFGigY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211110195901.85597-13-jeanmichel.hautbois@ideasonboard.com>","References":"<20211110195901.85597-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211110195901.85597-13-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 10 Nov 2021 23:10:49 +0000","Message-ID":"<163658584909.2121661.10352661553784463627@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 12/14] ipa: ipu3: Cache line\n\tduration at configure call","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20835,"web_url":"https://patchwork.libcamera.org/comment/20835/","msgid":"<163658794838.2121661.7439081080566270242@Monstersaurus>","date":"2021-11-10T23:45:48","subject":"Re: [libcamera-devel] [PATCH v2 12/14] ipa: ipu3: Cache line\n\tduration at configure call","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2021-11-10 23:10:49)\n> Quoting Jean-Michel Hautbois (2021-11-10 19:58:59)\n> > We use the line duration several times in the IPAIPU3. Instead of\n> > recalculating it each time, cache the value as a utils::Duration. When\n> > the value is needed in double, call the intern count() function to get\n> > it.\n> \n> caching it I like ... using the internal count() ... sounds scary...\n> \n> Oh ... in fact, it does look like the 'correct' way to do this.\n> But I think that's a limitation on our implementation of\n> utils::Duration.\n> \n> I think we can do:\n> maxShutterSpeed = maxExposure * lineDuration.get();\n> \n> But that essentially just casts a std::nano to a std::nano and then\n> calls .count() on it ... so that's a bit redundant compared to what\n> you're already doing...\n> \n> Perhaps we should have our Duration class implement a default .get()\n> which always returns nanoseconds.... (and simply calls .count() or an\n> explicit operator double() ... ?)\n> \n> Anyway, that isn't a fault of this patch as far as I can tell - so lets\n> consider that on top.\n> \n> If you prefer the explicit named cast with .get() I think\n> it's ok to use that as the compiler will probably optimise it ... but\n> either way is up to you I think...\n> \n> Reviewed-by: Kieran Bingham \n\nHrm... perhaps except for one more thing to check first ...\n\n\n> \n> > Signed-off-by: Jean-Michel Hautbois \n> > ---\n> > src/ipa/ipu3/ipu3.cpp | 17 +++++++++--------\n> > 1 file changed, 9 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index eb8c0dc4..68a97e8e 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -174,6 +174,8 @@ private:\n> > uint32_t minGain_;\n> > uint32_t maxGain_;\n> > \n> > + utils::Duration lineDuration_;\n> > +\n> > /* Interface to the Camera Helper */\n> > std::unique_ptr camHelper_;\n> > \n> > @@ -195,8 +197,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,\n> > int32_t minExposure = v4l2Exposure.min().get();\n> > int32_t maxExposure = v4l2Exposure.max().get();\n> > \n> > - utils::Duration lineDuration = sensorInfo.lineLength * 1.0s\n> > - / sensorInfo.pixelRate;\n> > + lineDuration_ = sensorInfo.lineLength * 1.0s\n> > + / sensorInfo.pixelRate;\n> > \n> > const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n> > int32_t minGain = v4l2Gain.min().get();\n> > @@ -209,8 +211,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,\n> > *\n> > * \\todo take VBLANK into account for maximum shutter speed\n> > */\n> > - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration;\n> > - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration;\n> > + context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_;\n> > + context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_;\n> > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n> > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n> > }\n> > @@ -239,11 +241,10 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > * exposure min, max and default and convert it from lines to\n> > * microseconds.\n> > */\n> > - double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);\n> \n> I like that we get rid of this 1e6 though ;-)\n\nIt 'might' still be right - but needs a correct/thorough checking (or\nuse the explicit .get() / .get() ....\n\nThe 1e6 is I assume taking us to micro's?... but...\n\nI believe the Duration.count() is going to be returning nano seconds...\ncan you validate/check that this calculation is using the correct units\nas expected please?\n\nI think this makes me favour the explicit units / period in the get<>()\ncall to make it clear what the calculation does..\n\n\n\n> \n> > const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n> > - int32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n> > - int32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n> > - int32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n> > + int32_t minExposure = v4l2Exposure.min().get() * lineDuration_.count();\n> > + int32_t maxExposure = v4l2Exposure.max().get() * lineDuration_.count();\n> > + int32_t defExposure = v4l2Exposure.def().get() * lineDuration_.count();\n> > controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > defExposure);\n> > \n> > -- \n> > 2.32.0\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 9ECBDBF415\n\tfor ;\n\tWed, 10 Nov 2021 23:45:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 064C06035A;\n\tThu, 11 Nov 2021 00:45:53 +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 34F13600BF\n\tfor ;\n\tThu, 11 Nov 2021 00:45:51 +0100 (CET)","from pendragon.ideasonboard.com\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 BDDC7501;\n\tThu, 11 Nov 2021 00:45:50 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XqjJknIc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636587950;\n\tbh=jLwgOUe9cmShLJD5LAIxqtAETCRSfW++8eJ6dyEy0+A=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=XqjJknIc+JhUd3fNkt0BeLr01JaEIK0VEV+E1EmrkRsGG4Nq1jmT628eikNghDdRQ\n\tKEgA2AFeJQNQC9I3DrMyi86t+eiUB0Dt8hekFAiJBQS0LOQNS/jSmrcwM4rliVfDft\n\tgfSBSH1YCDX1rMtXKfPJvMrETbW694ddhWbTJiiM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<163658584909.2121661.10352661553784463627@Monstersaurus>","References":"<20211110195901.85597-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211110195901.85597-13-jeanmichel.hautbois@ideasonboard.com>\n\t<163658584909.2121661.10352661553784463627@Monstersaurus>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 10 Nov 2021 23:45:48 +0000","Message-ID":"<163658794838.2121661.7439081080566270242@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 12/14] ipa: ipu3: Cache line\n\tduration at configure call","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20840,"web_url":"https://patchwork.libcamera.org/comment/20840/","msgid":"<1adc32eb-b0df-10c6-dda8-8f12978f3d17@ideasonboard.com>","date":"2021-11-11T07:25:52","subject":"Re: [libcamera-devel] [PATCH v2 12/14] ipa: ipu3: Cache line\n\tduration at configure call","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 11/11/2021 00:45, Kieran Bingham wrote:\n> Quoting Kieran Bingham (2021-11-10 23:10:49)\n>> Quoting Jean-Michel Hautbois (2021-11-10 19:58:59)\n>>> We use the line duration several times in the IPAIPU3. Instead of\n>>> recalculating it each time, cache the value as a utils::Duration. When\n>>> the value is needed in double, call the intern count() function to get\n>>> it.\n>>\n>> caching it I like ... using the internal count() ... sounds scary...\n>>\n>> Oh ... in fact, it does look like the 'correct' way to do this.\n>> But I think that's a limitation on our implementation of\n>> utils::Duration.\n>>\n>> I think we can do:\n>> maxShutterSpeed = maxExposure * lineDuration.get();\n>>\n>> But that essentially just casts a std::nano to a std::nano and then\n>> calls .count() on it ... so that's a bit redundant compared to what\n>> you're already doing...\n>>\n>> Perhaps we should have our Duration class implement a default .get()\n>> which always returns nanoseconds.... (and simply calls .count() or an\n>> explicit operator double() ... ?)\n>>\n>> Anyway, that isn't a fault of this patch as far as I can tell - so lets\n>> consider that on top.\n>>\n>> If you prefer the explicit named cast with .get() I think\n>> it's ok to use that as the compiler will probably optimise it ... but\n>> either way is up to you I think...\n>>\n>> Reviewed-by: Kieran Bingham \n> \n> Hrm... perhaps except for one more thing to check first ...\n> \n> \n>>\n>>> Signed-off-by: Jean-Michel Hautbois \n>>> ---\n>>> src/ipa/ipu3/ipu3.cpp | 17 +++++++++--------\n>>> 1 file changed, 9 insertions(+), 8 deletions(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index eb8c0dc4..68a97e8e 100644\n>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>> @@ -174,6 +174,8 @@ private:\n>>> uint32_t minGain_;\n>>> uint32_t maxGain_;\n>>> \n>>> + utils::Duration lineDuration_;\n>>> +\n>>> /* Interface to the Camera Helper */\n>>> std::unique_ptr camHelper_;\n>>> \n>>> @@ -195,8 +197,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,\n>>> int32_t minExposure = v4l2Exposure.min().get();\n>>> int32_t maxExposure = v4l2Exposure.max().get();\n>>> \n>>> - utils::Duration lineDuration = sensorInfo.lineLength * 1.0s\n>>> - / sensorInfo.pixelRate;\n>>> + lineDuration_ = sensorInfo.lineLength * 1.0s\n>>> + / sensorInfo.pixelRate;\n\nI just see this is bad, it should not be set here, but in configure() \nbefore calling updateControls...\n\n>>> \n>>> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>>> int32_t minGain = v4l2Gain.min().get();\n>>> @@ -209,8 +211,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,\n>>> *\n>>> * \\todo take VBLANK into account for maximum shutter speed\n>>> */\n>>> - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration;\n>>> - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration;\n>>> + context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_;\n>>> + context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_;\n>>> context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n>>> context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n>>> }\n>>> @@ -239,11 +241,10 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n>>> * exposure min, max and default and convert it from lines to\n>>> * microseconds.\n>>> */\n>>> - double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);\n>>\n>> I like that we get rid of this 1e6 though ;-)\n> \n> It 'might' still be right - but needs a correct/thorough checking (or\n> use the explicit .get() / .get() ....\n> \n> The 1e6 is I assume taking us to micro's?... but...\n> \n> I believe the Duration.count() is going to be returning nano seconds...\n> can you validate/check that this calculation is using the correct units\n> as expected please?\n> \n> I think this makes me favour the explicit units / period in the get<>()\n> call to make it clear what the calculation does..\n> \n> \n> \n>>\n>>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n>>> - int32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n>>> - int32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n>>> - int32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n>>> + int32_t minExposure = v4l2Exposure.min().get() * lineDuration_.count();\n>>> + int32_t maxExposure = v4l2Exposure.max().get() * lineDuration_.count();\n>>> + int32_t defExposure = v4l2Exposure.def().get() * lineDuration_.count();\n>>> controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n>>> defExposure);\n>>> \n>>> -- \n>>> 2.32.0\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 8CEFABDB1C\n\tfor ;\n\tThu, 11 Nov 2021 07:25:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D688D6035A;\n\tThu, 11 Nov 2021 08:25:56 +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 53D71600BF\n\tfor ;\n\tThu, 11 Nov 2021 08:25:55 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:e627:8337:a781:d98] (unknown\n\t[IPv6:2a01:e0a:169:7140:e627:8337:a781:d98])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 02C2BE51;\n\tThu, 11 Nov 2021 08:25:54 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OzdZCR7O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636615555;\n\tbh=GVNZyfWD34d9FEeWNAuUbB28wcpUeUCDz6jfnU1IZOo=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=OzdZCR7Oi0PELigZDO09YfQMHrtbNcac6VVWgTEH42bJzhHb+Z6hgK14Zr9s9KNXh\n\tMpn1aQgGBmlVPhO2rTvyQCJDCni83g2AHB4y/N5mp1md6y1n3aOw6G7/0sC+cM2JC8\n\t2cMBdqwVjZ/9QqqDw0W3P8Tw+414+0Ht0TcbITqY=","Message-ID":"<1adc32eb-b0df-10c6-dda8-8f12978f3d17@ideasonboard.com>","Date":"Thu, 11 Nov 2021 08:25:52 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.2.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211110195901.85597-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211110195901.85597-13-jeanmichel.hautbois@ideasonboard.com>\n\t<163658584909.2121661.10352661553784463627@Monstersaurus>\n\t<163658794838.2121661.7439081080566270242@Monstersaurus>","From":"Jean-Michel Hautbois ","In-Reply-To":"<163658794838.2121661.7439081080566270242@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 12/14] ipa: ipu3: Cache line\n\tduration at configure call","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20841,"web_url":"https://patchwork.libcamera.org/comment/20841/","msgid":"<861cc656-3440-9150-6cc6-2ab6c41e5ae3@ideasonboard.com>","date":"2021-11-11T07:40:13","subject":"Re: [libcamera-devel] [PATCH v2 12/14] ipa: ipu3: Cache line\n\tduration at configure call","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 11/11/2021 00:45, Kieran Bingham wrote:\n> Quoting Kieran Bingham (2021-11-10 23:10:49)\n>> Quoting Jean-Michel Hautbois (2021-11-10 19:58:59)\n>>> We use the line duration several times in the IPAIPU3. Instead of\n>>> recalculating it each time, cache the value as a utils::Duration. When\n>>> the value is needed in double, call the intern count() function to get\n>>> it.\n>>\n>> caching it I like ... using the internal count() ... sounds scary...\n>>\n>> Oh ... in fact, it does look like the 'correct' way to do this.\n>> But I think that's a limitation on our implementation of\n>> utils::Duration.\n>>\n>> I think we can do:\n>> maxShutterSpeed = maxExposure * lineDuration.get();\n>>\n>> But that essentially just casts a std::nano to a std::nano and then\n>> calls .count() on it ... so that's a bit redundant compared to what\n>> you're already doing...\n>>\n>> Perhaps we should have our Duration class implement a default .get()\n>> which always returns nanoseconds.... (and simply calls .count() or an\n>> explicit operator double() ... ?)\n>>\n>> Anyway, that isn't a fault of this patch as far as I can tell - so lets\n>> consider that on top.\n>>\n>> If you prefer the explicit named cast with .get() I think\n>> it's ok to use that as the compiler will probably optimise it ... but\n>> either way is up to you I think...\n>>\n>> Reviewed-by: Kieran Bingham \n> \n> Hrm... perhaps except for one more thing to check first ...\n> \n> \n>>\n>>> Signed-off-by: Jean-Michel Hautbois \n>>> ---\n>>> src/ipa/ipu3/ipu3.cpp | 17 +++++++++--------\n>>> 1 file changed, 9 insertions(+), 8 deletions(-)\n>>>\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index eb8c0dc4..68a97e8e 100644\n>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>> @@ -174,6 +174,8 @@ private:\n>>> uint32_t minGain_;\n>>> uint32_t maxGain_;\n>>> \n>>> + utils::Duration lineDuration_;\n>>> +\n>>> /* Interface to the Camera Helper */\n>>> std::unique_ptr camHelper_;\n>>> \n>>> @@ -195,8 +197,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,\n>>> int32_t minExposure = v4l2Exposure.min().get();\n>>> int32_t maxExposure = v4l2Exposure.max().get();\n>>> \n>>> - utils::Duration lineDuration = sensorInfo.lineLength * 1.0s\n>>> - / sensorInfo.pixelRate;\n>>> + lineDuration_ = sensorInfo.lineLength * 1.0s\n>>> + / sensorInfo.pixelRate;\n>>> \n>>> const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second;\n>>> int32_t minGain = v4l2Gain.min().get();\n>>> @@ -209,8 +211,8 @@ void IPAIPU3::updateSessionConfiguration(const IPACameraSensorInfo &sensorInfo,\n>>> *\n>>> * \\todo take VBLANK into account for maximum shutter speed\n>>> */\n>>> - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration;\n>>> - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration;\n>>> + context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_;\n>>> + context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_;\n>>> context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);\n>>> context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);\n>>> }\n>>> @@ -239,11 +241,10 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,\n>>> * exposure min, max and default and convert it from lines to\n>>> * microseconds.\n>>> */\n>>> - double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6);\n>>\n>> I like that we get rid of this 1e6 though ;-)\n> \n> It 'might' still be right - but needs a correct/thorough checking (or\n> use the explicit .get() / .get() ....\n> \n> The 1e6 is I assume taking us to micro's?... but...\n> \n> I believe the Duration.count() is going to be returning nano seconds...\n> can you validate/check that this calculation is using the correct units\n> as expected please?\n\nIt returns the number of ticks for this duration in a number of ns \nindeed. In our case:\n\nlineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;\nLOG(IPAIPU3, Debug) << \"Line duration: \" << lineDuration_\n << \" is \" << lineDuration_.count() << \" ticks\"\n << \" and \" << lineDuration.get() << \" microseconds\";\n\n=> output is:\nLine duration: 16.80us is 16800 ticks and 16.8 microseconds\n\nSo, I guess you are right ;-), we should use .get().\n\n\n> \n> I think this makes me favour the explicit units / period in the get<>()\n> call to make it clear what the calculation does..\n> \n> \n> \n>>\n>>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;\n>>> - int32_t minExposure = v4l2Exposure.min().get() * lineDuration;\n>>> - int32_t maxExposure = v4l2Exposure.max().get() * lineDuration;\n>>> - int32_t defExposure = v4l2Exposure.def().get() * lineDuration;\n>>> + int32_t minExposure = v4l2Exposure.min().get() * lineDuration_.count();\n>>> + int32_t maxExposure = v4l2Exposure.max().get() * lineDuration_.count();\n>>> + int32_t defExposure = v4l2Exposure.def().get() * lineDuration_.count();\n>>> controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n>>> defExposure);\n>>> \n>>> -- \n>>> 2.32.0\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 5CE6CBF415\n\tfor ;\n\tThu, 11 Nov 2021 07:40:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A37E46035A;\n\tThu, 11 Nov 2021 08:40:16 +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 5A921600BF\n\tfor ;\n\tThu, 11 Nov 2021 08:40:15 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:e627:8337:a781:d98] (unknown\n\t[IPv6:2a01:e0a:169:7140:e627:8337:a781:d98])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E9273E51;\n\tThu, 11 Nov 2021 08:40:14 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AQZiaREo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636616415;\n\tbh=Bzl6bWyvWLVQ7AkHB6T37lEOcVGtXe77oMiJPABCVkM=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=AQZiaREojdkEZMocu7bsi+M0M9UyO5k2HW2YZhx2DM0ka9PjVeoxKdwa6Cg/mYZ9c\n\tSW3acsbrGt/w9eMi5iNDm/HhjdOvJ5I9wxh0BzK4VAT6jciZhjJQQBfMFvaWqNkQu4\n\tqJlh4EDujYWA1nDYroocJChDGlfXAa06u52uLajE=","Message-ID":"<861cc656-3440-9150-6cc6-2ab6c41e5ae3@ideasonboard.com>","Date":"Thu, 11 Nov 2021 08:40:13 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.2.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211110195901.85597-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211110195901.85597-13-jeanmichel.hautbois@ideasonboard.com>\n\t<163658584909.2121661.10352661553784463627@Monstersaurus>\n\t<163658794838.2121661.7439081080566270242@Monstersaurus>","From":"Jean-Michel Hautbois ","In-Reply-To":"<163658794838.2121661.7439081080566270242@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 12/14] ipa: ipu3: Cache line\n\tduration at configure call","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]