[{"id":25267,"web_url":"https://patchwork.libcamera.org/comment/25267/","msgid":"<YzxnCPyafTt7h4J4@pendragon.ideasonboard.com>","date":"2022-10-04T17:02:00","subject":"Re: [libcamera-devel] [PATCH v1 3/9] ipa: raspberrypi: Pass\n\tlineLength into the CamHelper API","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Mon, Oct 03, 2022 at 09:39:29AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Update CamHelper::exposureLines() and CamHelper::exposure() to take a\n> line length duration parameter for use in the exposure calculations.\n> \n> For now, only use the minimum line length for all the calculations to match\n> the existing IPA behavior.\n> \n> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> ---\n>  src/ipa/raspberrypi/cam_helper.cpp           | 12 ++++++------\n>  src/ipa/raspberrypi/cam_helper.h             |  6 ++++--\n>  src/ipa/raspberrypi/cam_helper_imx219.cpp    |  3 ++-\n>  src/ipa/raspberrypi/cam_helper_imx296.cpp    | 10 ++++++----\n>  src/ipa/raspberrypi/cam_helper_imx477.cpp    |  7 ++++---\n>  src/ipa/raspberrypi/cam_helper_imx519.cpp    |  7 ++++---\n>  src/ipa/raspberrypi/controller/camera_mode.h |  2 +-\n>  src/ipa/raspberrypi/raspberrypi.cpp          |  8 ++++----\n>  8 files changed, 31 insertions(+), 24 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 42251ba29682..fd3527b94501 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -61,16 +61,16 @@ void CamHelper::process([[maybe_unused]] StatisticsPtr &stats,\n>  {\n>  }\n>  \n> -uint32_t CamHelper::exposureLines(const Duration exposure) const\n> +uint32_t CamHelper::exposureLines(const Duration exposure, const Duration lineLength) const\n>  {\n>  \tassert(initialized_);\n\nIs the assert still needed ? Same below.\n\n> -\treturn exposure / mode_.minLineLength;\n> +\treturn exposure / lineLength;\n>  }\n>  \n> -Duration CamHelper::exposure(uint32_t exposureLines) const\n> +Duration CamHelper::exposure(uint32_t exposureLines, const Duration lineLength) const\n>  {\n>  \tassert(initialized_);\n> -\treturn exposureLines * mode_.minLineLength;\n> +\treturn exposureLines * lineLength;\n>  }\n>  \n>  uint32_t CamHelper::getVBlanking(Duration &exposure,\n> @@ -78,7 +78,7 @@ uint32_t CamHelper::getVBlanking(Duration &exposure,\n>  \t\t\t\t Duration maxFrameDuration) const\n>  {\n>  \tuint32_t frameLengthMin, frameLengthMax, vblank;\n> -\tuint32_t exposureLines = CamHelper::exposureLines(exposure);\n> +\tuint32_t exposureLines = CamHelper::exposureLines(exposure, mode_.minLineLength);\n>  \n>  \tassert(initialized_);\n>  \n> @@ -94,7 +94,7 @@ uint32_t CamHelper::getVBlanking(Duration &exposure,\n>  \t * re-calculate if it has been clipped.\n>  \t */\n>  \texposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);\n> -\texposure = CamHelper::exposure(exposureLines);\n> +\texposure = CamHelper::exposure(exposureLines, mode_.minLineLength);\n>  \n>  \t/* Limit the vblank to the range allowed by the frame length limits. */\n>  \tvblank = std::clamp(exposureLines + frameIntegrationDiff_,\n> diff --git a/src/ipa/raspberrypi/cam_helper.h b/src/ipa/raspberrypi/cam_helper.h\n> index 70d62719da86..9b5e602689f3 100644\n> --- a/src/ipa/raspberrypi/cam_helper.h\n> +++ b/src/ipa/raspberrypi/cam_helper.h\n> @@ -78,8 +78,10 @@ public:\n>  \tvirtual void prepare(libcamera::Span<const uint8_t> buffer,\n>  \t\t\t     Metadata &metadata);\n>  \tvirtual void process(StatisticsPtr &stats, Metadata &metadata);\n> -\tvirtual uint32_t exposureLines(libcamera::utils::Duration exposure) const;\n> -\tvirtual libcamera::utils::Duration exposure(uint32_t exposureLines) const;\n> +\tvirtual uint32_t exposureLines(const libcamera::utils::Duration exposure,\n> +\t\t\t\t       const libcamera::utils::Duration lineLength) const;\n> +\tvirtual libcamera::utils::Duration exposure(uint32_t exposureLines,\n> +\t\t\t\t\t\t    const libcamera::utils::Duration lineLength) const;\n>  \tvirtual uint32_t getVBlanking(libcamera::utils::Duration &exposure,\n>  \t\t\t\t      libcamera::utils::Duration minFrameDuration,\n>  \t\t\t\t      libcamera::utils::Duration maxFrameDuration) const;\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index 7ded07a213cd..98a3b31956ec 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -94,7 +94,8 @@ void CamHelperImx219::populateMetadata(const MdParser::RegisterMap &registers,\n>  {\n>  \tDeviceStatus deviceStatus;\n>  \n> -\tdeviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> +\tdeviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg),\n> +\t\t\t\t\t     mode_.minLineLength);\n>  \tdeviceStatus.analogueGain = gain(registers.at(gainReg));\n>  \tdeviceStatus.frameLength = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg);\n>  \n> diff --git a/src/ipa/raspberrypi/cam_helper_imx296.cpp b/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> index ab1d157aaf45..74361ffe7cc2 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> @@ -21,8 +21,8 @@ public:\n>  \tCamHelperImx296();\n>  \tuint32_t gainCode(double gain) const override;\n>  \tdouble gain(uint32_t gainCode) const override;\n> -\tuint32_t exposureLines(Duration exposure) const override;\n> -\tDuration exposure(uint32_t exposureLines) const override;\n> +\tuint32_t exposureLines(const Duration exposure, const Duration lineLength) const override;\n> +\tDuration exposure(uint32_t exposureLines, const Duration lineLength) const override;\n>  \n>  private:\n>  \tstatic constexpr uint32_t maxGainCode = 239;\n> @@ -51,12 +51,14 @@ double CamHelperImx296::gain(uint32_t gainCode) const\n>  \treturn std::pow(10.0, gainCode / 200.0);\n>  }\n>  \n> -uint32_t CamHelperImx296::exposureLines(Duration exposure) const\n> +uint32_t CamHelperImx296::exposureLines(const Duration exposure,\n> +\t\t\t\t\t[[maybe_unused]] const Duration lineLength) const\n\nDoes this mean that the IMX296 has no configurable horizontal blanking ?\n\n>  {\n>  \treturn (exposure - 14.26us) / timePerLine;\n>  }\n>  \n> -Duration CamHelperImx296::exposure(uint32_t exposureLines) const\n> +Duration CamHelperImx296::exposure(uint32_t exposureLines,\n> +\t\t\t\t   [[maybe_unused]] const Duration lineLength) const\n>  {\n>  \treturn exposureLines * timePerLine + 14.26us;\n>  }\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index aa306d6661c6..71529bdd3034 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -144,9 +144,9 @@ uint32_t CamHelperImx477::getVBlanking(Duration &exposure,\n>  \tif (shift) {\n>  \t\t/* Account for any rounding in the scaled frame length value. */\n>  \t\tframeLength <<= shift;\n> -\t\texposureLines = CamHelperImx477::exposureLines(exposure);\n> +\t\texposureLines = CamHelperImx477::exposureLines(exposure, mode_.minLineLength);\n>  \t\texposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);\n> -\t\texposure = CamHelperImx477::exposure(exposureLines);\n> +\t\texposure = CamHelperImx477::exposure(exposureLines, mode_.minLineLength);\n>  \t}\n>  \n>  \treturn frameLength - mode_.height;\n> @@ -170,7 +170,8 @@ void CamHelperImx477::populateMetadata(const MdParser::RegisterMap &registers,\n>  {\n>  \tDeviceStatus deviceStatus;\n>  \n> -\tdeviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> +\tdeviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg),\n> +\t\t\t\t\t     mode_.minLineLength);\n>  \tdeviceStatus.analogueGain = gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));\n>  \tdeviceStatus.frameLength = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg);\n>  \tdeviceStatus.sensorTemperature = std::clamp<int8_t>(registers.at(temperatureReg), -20, 80);\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx519.cpp b/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> index 54e104e7659a..2c120dad1680 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> @@ -144,9 +144,9 @@ uint32_t CamHelperImx519::getVBlanking(Duration &exposure,\n>  \tif (shift) {\n>  \t\t/* Account for any rounding in the scaled frame length value. */\n>  \t\tframeLength <<= shift;\n> -\t\texposureLines = CamHelperImx519::exposureLines(exposure);\n> +\t\texposureLines = CamHelperImx519::exposureLines(exposure, mode_.minLineLength);\n>  \t\texposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);\n> -\t\texposure = CamHelperImx519::exposure(exposureLines);\n> +\t\texposure = CamHelperImx519::exposure(exposureLines, mode_.minLineLength);\n>  \t}\n>  \n>  \treturn frameLength - mode_.height;\n> @@ -170,7 +170,8 @@ void CamHelperImx519::populateMetadata(const MdParser::RegisterMap &registers,\n>  {\n>  \tDeviceStatus deviceStatus;\n>  \n> -\tdeviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> +\tdeviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg),\n> +\t\t\t\t\t     mode_.minLineLength);\n>  \tdeviceStatus.analogueGain = gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));\n>  \tdeviceStatus.frameLength = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg);\n>  \n> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h\n> index b7e73aae4698..cda6ac4e200a 100644\n> --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> @@ -31,7 +31,7 @@ struct CameraMode {\n>  \tdouble scaleX, scaleY;\n>  \t/* scaling of the noise compared to the native sensor mode */\n>  \tdouble noiseFactor;\n> -\t/* line time */\n> +\t/* minimum and maximum line times */\n\nThis belongs to the previous patch.\n\n>  \tlibcamera::utils::Duration minLineLength, maxLineLength;\n>  \t/* any camera transform *not* reflected already in the camera tuning */\n>  \tlibcamera::Transform transform;\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 67326bcf4a14..13807f4d47f7 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -477,7 +477,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>  \tconst uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();\n>  \n>  \tctrlMap[&controls::ExposureTime] =\n> -\t\tControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),\n> +\t\tControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin, mode_.minLineLength).get<std::micro>()),\n>  \t\t\t    static_cast<int32_t>(maxShutter.get<std::micro>()));\n>  \n>  \tresult->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> @@ -550,7 +550,7 @@ void IPARPi::reportMetadata()\n>  \t\t\t\t       deviceStatus->shutterSpeed.get<std::micro>());\n>  \t\tlibcameraMetadata_.set(controls::AnalogueGain, deviceStatus->analogueGain);\n>  \t\tlibcameraMetadata_.set(controls::FrameDuration,\n> -\t\t\t\t       helper_->exposure(deviceStatus->frameLength).get<std::micro>());\n> +\t\t\t\t       helper_->exposure(deviceStatus->frameLength, mode_.minLineLength).get<std::micro>());\n>  \t\tif (deviceStatus->sensorTemperature)\n>  \t\t\tlibcameraMetadata_.set(controls::SensorTemperature, *deviceStatus->sensorTemperature);\n>  \t}\n> @@ -1106,7 +1106,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n>  \tint32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();\n>  \tint32_t vblank = sensorControls.get(V4L2_CID_VBLANK).get<int32_t>();\n>  \n> -\tdeviceStatus.shutterSpeed = helper_->exposure(exposureLines);\n> +\tdeviceStatus.shutterSpeed = helper_->exposure(exposureLines, mode_.minLineLength);\n>  \tdeviceStatus.analogueGain = helper_->gain(gainCode);\n>  \tdeviceStatus.frameLength = mode_.height + vblank;\n>  \n> @@ -1198,7 +1198,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n>  \t/* getVBlanking might clip exposure time to the fps limits. */\n>  \tDuration exposure = agcStatus->shutterTime;\n>  \tint32_t vblanking = helper_->getVBlanking(exposure, minFrameDuration_, maxFrameDuration_);\n> -\tint32_t exposureLines = helper_->exposureLines(exposure);\n> +\tint32_t exposureLines = helper_->exposureLines(exposure, mode_.minLineLength);\n>  \n>  \tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n>  \t\t\t   << \" (Shutter lines: \" << exposureLines << \", AGC requested \"","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3EC29BD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  4 Oct 2022 17:02:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9B5160AAE;\n\tTue,  4 Oct 2022 19:02:05 +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 0BEA9601C7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  4 Oct 2022 19:02:04 +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 5F0AB2D9;\n\tTue,  4 Oct 2022 19:02:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664902925;\n\tbh=QJSdLFPITr3HeXTzhLKwY7BlAP6pWhB+f3LlVHbWUh0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=CPLruYqkwFNRcVgxr0sVb+eHW1zCkaUv9yoDhKUAHpUBULJ5Y8uZnGFv7dpNIeJk7\n\tNJkLkaYOrXCuFKwsWRlbkrKv0m57vWNWrlXaTvkKUN4qipYuKZo4odJBx2UD9gbqGw\n\tYc9zlb36auyim5br/vmzCcb9fryDH1Zd9E1g2I6iuDBq0pLAx/aFaPq+KnfOSePont\n\tbtVvR98BKNceTWJYq317s7MQokB+g0i7F/o7pf5A5hkp7S23e2EyFuYkqPOEW7dONY\n\t8rqRg2J5hY62omhJz97nuoTyUxKICg4QjsyinxrMR7LbN4PojfWgM96oTm6SeC8jRb\n\tqjyh0SWS8ZSaA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664902923;\n\tbh=QJSdLFPITr3HeXTzhLKwY7BlAP6pWhB+f3LlVHbWUh0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pYMRFDDttUE8rwSZsHPkk9otkgTTLhV737qQCBr2x9qWMXL0HUFjVsrfBhTe/hxD+\n\tnrxv/rDAvQvkyIaNyIvliMo/a8OJakSZuAU5jAK/mkY3lZPA0+8tv8G2N9GsicTyJE\n\tYV7Kh+g5o9Uedby2kxY96mjca3Jx4u10KggiIQlY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"pYMRFDDt\"; dkim-atps=neutral","Date":"Tue, 4 Oct 2022 20:02:00 +0300","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<YzxnCPyafTt7h4J4@pendragon.ideasonboard.com>","References":"<20221003083934.31629-1-naush@raspberrypi.com>\n\t<20221003083934.31629-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221003083934.31629-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 3/9] ipa: raspberrypi: Pass\n\tlineLength into the CamHelper API","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":25309,"web_url":"https://patchwork.libcamera.org/comment/25309/","msgid":"<CAEmqJPowROUtvK2cnHFQXV1gNtwg4MqC9Q6gHP1CDT-W=Z2ATw@mail.gmail.com>","date":"2022-10-05T13:00:48","subject":"Re: [libcamera-devel] [PATCH v1 3/9] ipa: raspberrypi: Pass\n\tlineLength into the CamHelper API","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Tue, 4 Oct 2022 at 18:02, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Oct 03, 2022 at 09:39:29AM +0100, Naushir Patuck via\n> libcamera-devel wrote:\n> > Update CamHelper::exposureLines() and CamHelper::exposure() to take a\n> > line length duration parameter for use in the exposure calculations.\n> >\n> > For now, only use the minimum line length for all the calculations to\n> match\n> > the existing IPA behavior.\n> >\n> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > ---\n> >  src/ipa/raspberrypi/cam_helper.cpp           | 12 ++++++------\n> >  src/ipa/raspberrypi/cam_helper.h             |  6 ++++--\n> >  src/ipa/raspberrypi/cam_helper_imx219.cpp    |  3 ++-\n> >  src/ipa/raspberrypi/cam_helper_imx296.cpp    | 10 ++++++----\n> >  src/ipa/raspberrypi/cam_helper_imx477.cpp    |  7 ++++---\n> >  src/ipa/raspberrypi/cam_helper_imx519.cpp    |  7 ++++---\n> >  src/ipa/raspberrypi/controller/camera_mode.h |  2 +-\n> >  src/ipa/raspberrypi/raspberrypi.cpp          |  8 ++++----\n> >  8 files changed, 31 insertions(+), 24 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > index 42251ba29682..fd3527b94501 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -61,16 +61,16 @@ void CamHelper::process([[maybe_unused]]\n> StatisticsPtr &stats,\n> >  {\n> >  }\n> >\n> > -uint32_t CamHelper::exposureLines(const Duration exposure) const\n> > +uint32_t CamHelper::exposureLines(const Duration exposure, const\n> Duration lineLength) const\n> >  {\n> >       assert(initialized_);\n>\n> Is the assert still needed ? Same below.\n>\n\nNot strictly needed any more.  I can remove it in another patch.\n\n\n>\n> > -     return exposure / mode_.minLineLength;\n> > +     return exposure / lineLength;\n> >  }\n> >\n> > -Duration CamHelper::exposure(uint32_t exposureLines) const\n> > +Duration CamHelper::exposure(uint32_t exposureLines, const Duration\n> lineLength) const\n> >  {\n> >       assert(initialized_);\n> > -     return exposureLines * mode_.minLineLength;\n> > +     return exposureLines * lineLength;\n> >  }\n> >\n> >  uint32_t CamHelper::getVBlanking(Duration &exposure,\n> > @@ -78,7 +78,7 @@ uint32_t CamHelper::getVBlanking(Duration &exposure,\n> >                                Duration maxFrameDuration) const\n> >  {\n> >       uint32_t frameLengthMin, frameLengthMax, vblank;\n> > -     uint32_t exposureLines = CamHelper::exposureLines(exposure);\n> > +     uint32_t exposureLines = CamHelper::exposureLines(exposure,\n> mode_.minLineLength);\n> >\n> >       assert(initialized_);\n> >\n> > @@ -94,7 +94,7 @@ uint32_t CamHelper::getVBlanking(Duration &exposure,\n> >        * re-calculate if it has been clipped.\n> >        */\n> >       exposureLines = std::min(frameLengthMax - frameIntegrationDiff_,\n> exposureLines);\n> > -     exposure = CamHelper::exposure(exposureLines);\n> > +     exposure = CamHelper::exposure(exposureLines, mode_.minLineLength);\n> >\n> >       /* Limit the vblank to the range allowed by the frame length\n> limits. */\n> >       vblank = std::clamp(exposureLines + frameIntegrationDiff_,\n> > diff --git a/src/ipa/raspberrypi/cam_helper.h\n> b/src/ipa/raspberrypi/cam_helper.h\n> > index 70d62719da86..9b5e602689f3 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.h\n> > +++ b/src/ipa/raspberrypi/cam_helper.h\n> > @@ -78,8 +78,10 @@ public:\n> >       virtual void prepare(libcamera::Span<const uint8_t> buffer,\n> >                            Metadata &metadata);\n> >       virtual void process(StatisticsPtr &stats, Metadata &metadata);\n> > -     virtual uint32_t exposureLines(libcamera::utils::Duration\n> exposure) const;\n> > -     virtual libcamera::utils::Duration exposure(uint32_t\n> exposureLines) const;\n> > +     virtual uint32_t exposureLines(const libcamera::utils::Duration\n> exposure,\n> > +                                    const libcamera::utils::Duration\n> lineLength) const;\n> > +     virtual libcamera::utils::Duration exposure(uint32_t exposureLines,\n> > +                                                 const\n> libcamera::utils::Duration lineLength) const;\n> >       virtual uint32_t getVBlanking(libcamera::utils::Duration &exposure,\n> >                                     libcamera::utils::Duration\n> minFrameDuration,\n> >                                     libcamera::utils::Duration\n> maxFrameDuration) const;\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > index 7ded07a213cd..98a3b31956ec 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > @@ -94,7 +94,8 @@ void CamHelperImx219::populateMetadata(const\n> MdParser::RegisterMap &registers,\n> >  {\n> >       DeviceStatus deviceStatus;\n> >\n> > -     deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256\n> + registers.at(expLoReg));\n> > +     deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256\n> + registers.at(expLoReg),\n> > +                                          mode_.minLineLength);\n> >       deviceStatus.analogueGain = gain(registers.at(gainReg));\n> >       deviceStatus.frameLength = registers.at(frameLengthHiReg) * 256 +\n> registers.at(frameLengthLoReg);\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> > index ab1d157aaf45..74361ffe7cc2 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> > @@ -21,8 +21,8 @@ public:\n> >       CamHelperImx296();\n> >       uint32_t gainCode(double gain) const override;\n> >       double gain(uint32_t gainCode) const override;\n> > -     uint32_t exposureLines(Duration exposure) const override;\n> > -     Duration exposure(uint32_t exposureLines) const override;\n> > +     uint32_t exposureLines(const Duration exposure, const Duration\n> lineLength) const override;\n> > +     Duration exposure(uint32_t exposureLines, const Duration\n> lineLength) const override;\n> >\n> >  private:\n> >       static constexpr uint32_t maxGainCode = 239;\n> > @@ -51,12 +51,14 @@ double CamHelperImx296::gain(uint32_t gainCode) const\n> >       return std::pow(10.0, gainCode / 200.0);\n> >  }\n> >\n> > -uint32_t CamHelperImx296::exposureLines(Duration exposure) const\n> > +uint32_t CamHelperImx296::exposureLines(const Duration exposure,\n> > +                                     [[maybe_unused]] const Duration\n> lineLength) const\n>\n> Does this mean that the IMX296 has no configurable horizontal blanking ?\n>\n\nI've not looked into it in detail yet.  Hence leaving this as-is.  Once I\nverify this,\nI will update this code.\n\nRegards,\nNaush\n\n\n\n>\n> >  {\n> >       return (exposure - 14.26us) / timePerLine;\n> >  }\n> >\n> > -Duration CamHelperImx296::exposure(uint32_t exposureLines) const\n> > +Duration CamHelperImx296::exposure(uint32_t exposureLines,\n> > +                                [[maybe_unused]] const Duration\n> lineLength) const\n> >  {\n> >       return exposureLines * timePerLine + 14.26us;\n> >  }\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index aa306d6661c6..71529bdd3034 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -144,9 +144,9 @@ uint32_t CamHelperImx477::getVBlanking(Duration\n> &exposure,\n> >       if (shift) {\n> >               /* Account for any rounding in the scaled frame length\n> value. */\n> >               frameLength <<= shift;\n> > -             exposureLines = CamHelperImx477::exposureLines(exposure);\n> > +             exposureLines = CamHelperImx477::exposureLines(exposure,\n> mode_.minLineLength);\n> >               exposureLines = std::min(exposureLines, frameLength -\n> frameIntegrationDiff);\n> > -             exposure = CamHelperImx477::exposure(exposureLines);\n> > +             exposure = CamHelperImx477::exposure(exposureLines,\n> mode_.minLineLength);\n> >       }\n> >\n> >       return frameLength - mode_.height;\n> > @@ -170,7 +170,8 @@ void CamHelperImx477::populateMetadata(const\n> MdParser::RegisterMap &registers,\n> >  {\n> >       DeviceStatus deviceStatus;\n> >\n> > -     deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256\n> + registers.at(expLoReg));\n> > +     deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256\n> + registers.at(expLoReg),\n> > +                                          mode_.minLineLength);\n> >       deviceStatus.analogueGain = gain(registers.at(gainHiReg) * 256 +\n> registers.at(gainLoReg));\n> >       deviceStatus.frameLength = registers.at(frameLengthHiReg) * 256 +\n> registers.at(frameLengthLoReg);\n> >       deviceStatus.sensorTemperature = std::clamp<int8_t>(registers.at(temperatureReg),\n> -20, 80);\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> > index 54e104e7659a..2c120dad1680 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> > @@ -144,9 +144,9 @@ uint32_t CamHelperImx519::getVBlanking(Duration\n> &exposure,\n> >       if (shift) {\n> >               /* Account for any rounding in the scaled frame length\n> value. */\n> >               frameLength <<= shift;\n> > -             exposureLines = CamHelperImx519::exposureLines(exposure);\n> > +             exposureLines = CamHelperImx519::exposureLines(exposure,\n> mode_.minLineLength);\n> >               exposureLines = std::min(exposureLines, frameLength -\n> frameIntegrationDiff);\n> > -             exposure = CamHelperImx519::exposure(exposureLines);\n> > +             exposure = CamHelperImx519::exposure(exposureLines,\n> mode_.minLineLength);\n> >       }\n> >\n> >       return frameLength - mode_.height;\n> > @@ -170,7 +170,8 @@ void CamHelperImx519::populateMetadata(const\n> MdParser::RegisterMap &registers,\n> >  {\n> >       DeviceStatus deviceStatus;\n> >\n> > -     deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256\n> + registers.at(expLoReg));\n> > +     deviceStatus.shutterSpeed = exposure(registers.at(expHiReg) * 256\n> + registers.at(expLoReg),\n> > +                                          mode_.minLineLength);\n> >       deviceStatus.analogueGain = gain(registers.at(gainHiReg) * 256 +\n> registers.at(gainLoReg));\n> >       deviceStatus.frameLength = registers.at(frameLengthHiReg) * 256 +\n> registers.at(frameLengthLoReg);\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> b/src/ipa/raspberrypi/controller/camera_mode.h\n> > index b7e73aae4698..cda6ac4e200a 100644\n> > --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> > +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> > @@ -31,7 +31,7 @@ struct CameraMode {\n> >       double scaleX, scaleY;\n> >       /* scaling of the noise compared to the native sensor mode */\n> >       double noiseFactor;\n> > -     /* line time */\n> > +     /* minimum and maximum line times */\n>\n> This belongs to the previous patch.\n>\n> >       libcamera::utils::Duration minLineLength, maxLineLength;\n> >       /* any camera transform *not* reflected already in the camera\n> tuning */\n> >       libcamera::Transform transform;\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 67326bcf4a14..13807f4d47f7 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -477,7 +477,7 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> >       const uint32_t exposureMin =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();\n> >\n> >       ctrlMap[&controls::ExposureTime] =\n> > -\n>  ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin).get<std::micro>()),\n> > +\n>  ControlInfo(static_cast<int32_t>(helper_->exposure(exposureMin,\n> mode_.minLineLength).get<std::micro>()),\n> >\n>  static_cast<int32_t>(maxShutter.get<std::micro>()));\n> >\n> >       result->controlInfo = ControlInfoMap(std::move(ctrlMap),\n> controls::controls);\n> > @@ -550,7 +550,7 @@ void IPARPi::reportMetadata()\n> >\n> deviceStatus->shutterSpeed.get<std::micro>());\n> >               libcameraMetadata_.set(controls::AnalogueGain,\n> deviceStatus->analogueGain);\n> >               libcameraMetadata_.set(controls::FrameDuration,\n> > -\n> helper_->exposure(deviceStatus->frameLength).get<std::micro>());\n> > +\n> helper_->exposure(deviceStatus->frameLength,\n> mode_.minLineLength).get<std::micro>());\n> >               if (deviceStatus->sensorTemperature)\n> >\n>  libcameraMetadata_.set(controls::SensorTemperature,\n> *deviceStatus->sensorTemperature);\n> >       }\n> > @@ -1106,7 +1106,7 @@ void IPARPi::fillDeviceStatus(const ControlList\n> &sensorControls)\n> >       int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();\n> >       int32_t vblank =\n> sensorControls.get(V4L2_CID_VBLANK).get<int32_t>();\n> >\n> > -     deviceStatus.shutterSpeed = helper_->exposure(exposureLines);\n> > +     deviceStatus.shutterSpeed = helper_->exposure(exposureLines,\n> mode_.minLineLength);\n> >       deviceStatus.analogueGain = helper_->gain(gainCode);\n> >       deviceStatus.frameLength = mode_.height + vblank;\n> >\n> > @@ -1198,7 +1198,7 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> >       /* getVBlanking might clip exposure time to the fps limits. */\n> >       Duration exposure = agcStatus->shutterTime;\n> >       int32_t vblanking = helper_->getVBlanking(exposure,\n> minFrameDuration_, maxFrameDuration_);\n> > -     int32_t exposureLines = helper_->exposureLines(exposure);\n> > +     int32_t exposureLines = helper_->exposureLines(exposure,\n> mode_.minLineLength);\n> >\n> >       LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> >                          << \" (Shutter lines: \" << exposureLines << \",\n> AGC requested \"\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 B4C8DC0DA4\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  5 Oct 2022 13:01:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 105C762379;\n\tWed,  5 Oct 2022 15:01:08 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CA5A4601C7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  5 Oct 2022 15:01:05 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id o7so18174784lfk.7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 05 Oct 2022 06:01:05 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664974868;\n\tbh=jCL1XR4aKPCWp4nPKJPDYNOoDgUieFYlTijN/y42m3o=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=eXV8fQyhBMyt5jqyrOW/mUYxTdJ8j5U0D3QiRWSRec+/78vZdYkScPIyVFcNChb/X\n\tvgk7XMo2WUrk/Nq9GSWEWzMpffDNECfyJrvJXUIj5sd/8NkuiZksblohPAjCVeFuso\n\tlSSTqj7QdvkkW5VgNBQCAtKf57EwcK667iMU2y8C7s9XqxPdAvdk0vV380NgyLdy9t\n\tCEF3/uE3OGuc+cO+QIK2P3R8pHJsJmy5gByveAXti07punD1Bdjc1toRWYTa6E/p+I\n\tejNEbKKYEK3txkUXFd2AAWvCew5BkzmynT4SZW4MBN2XuVF7f+Vmz/t2xVXaha+JzM\n\tLEofSmTj9BEtA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date;\n\tbh=A9opdXfaik9sGhSWyP/lEwpLGtSGl0Vz10NMwT66JUM=;\n\tb=nfUob4InmnFQG7Sf9slKANiCHNDBbZSevT8Ik2jrveRCBbC+A5VYW8imNO2XNm2F1O\n\t6lQVz8EtKZ304ZdyJoS5IpeKONAypVZqsl8b8bG3EdSoLX3pFAsnHoW0ufie61PJma+l\n\t8ABM7N5poRadXk/MvTrJ+Do01PM57JxRxDmSnQQ4IGUmVoFslkdmSXbURDT6YZZz0nvo\n\tIcc5gZw14kwjum+fIBzGSHYIppv4tUGqRFLE95MipM61mVBRjlz5vlK1OmcU/xF4A/Gf\n\tID/iAt3llpjW3QQYAcCrLgyIwrU5YuQZHgXwU/FwFwWiMVqFYqrB2hKzjIJel0jZF5VS\n\thCYw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"nfUob4In\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date;\n\tbh=A9opdXfaik9sGhSWyP/lEwpLGtSGl0Vz10NMwT66JUM=;\n\tb=kBP14PoLKxK2zWWYL492jtinwxp2qp0mEXA0XiBBWu4ul9Y0rIJ6ABB0tZnLH/dc+x\n\t+pVBb/Bo4NH4YHHWauoviox/iruVS1VO4Wc+at3XRZHz+DUttVoNrFw8u82q5JrUiXAD\n\tH2MRzzWViq0EaGrFgm5PJEiHC1NsHIlQU5V2diFqucI4b7F/3aWW97NvjYcOpwP4j77u\n\tuffWMfeupIRekYYaJ0W0A1msB/LSPBtchYWz/fKF/yHELXSqlfDJM/e1yg3mTjNpDGoH\n\tjXnGT5JxtRQXBOvweMbCBBne0ioe2BH5+JtD5okDtyXgByCwRqsdSJt0t245ozv/Qju9\n\tJr5w==","X-Gm-Message-State":"ACrzQf1MQmuSczIXOsAX/PcMgUGVAD0mgu9bdcVCUAOIfaopAG9EMjMj\n\trO5J8m5S1EvErI5YBzSE5fgE1cjqc+Uy/8g0DpV94Q==","X-Google-Smtp-Source":"AMsMyM6fuNEow7tRh/D23ZbRWx+nOii7FZlvLbyOLotaS6MefXK3L+xMdYa3eDbyB0jF4EKH+rloH67RWkvPJG/9200=","X-Received":"by 2002:a05:6512:1152:b0:4a0:5555:15ee with SMTP id\n\tm18-20020a056512115200b004a0555515eemr11938932lfg.38.1664974864770;\n\tWed, 05 Oct 2022 06:01:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20221003083934.31629-1-naush@raspberrypi.com>\n\t<20221003083934.31629-4-naush@raspberrypi.com>\n\t<YzxnCPyafTt7h4J4@pendragon.ideasonboard.com>","In-Reply-To":"<YzxnCPyafTt7h4J4@pendragon.ideasonboard.com>","Date":"Wed, 5 Oct 2022 14:00:48 +0100","Message-ID":"<CAEmqJPowROUtvK2cnHFQXV1gNtwg4MqC9Q6gHP1CDT-W=Z2ATw@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"0000000000006000f805ea492bb9\"","Subject":"Re: [libcamera-devel] [PATCH v1 3/9] ipa: raspberrypi: Pass\n\tlineLength into the CamHelper API","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]