[{"id":25273,"web_url":"https://patchwork.libcamera.org/comment/25273/","msgid":"","date":"2022-10-04T17:46:11","subject":"Re: [libcamera-devel] [PATCH v1 8/9] ipa: raspberrypi: Allow full\n\tline length control","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:34AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Rename CamHelper::getVBlanking to CamHelper::getBlanking, and update the\n> calculations in that function to return both horizontal and vertical blanking\n> values for a given exposure time and frame duration limits. The calculations\n> are setup such that vertical blanking is extended to the maximum allowable\n> value, and any remainder gets put into horizontal blanking.\n\nHmmm... It would be nice if the heuristics was implemented in the IPA\nmodule itself and not in the helpers, but given that the helpers are\nspecific to the Raspberry Pi IPA module, I think that's fine for now.\n\n> The calculated horizontal blanking value is now return to the pipeline handler\n\ns/return/returned/\n\n> to pass into DelayedControls to program into the sensor.\n> \n> Update the IPA to now specify the maximum frame duration from the maximum\n> horizontal + vertical blanking values provided by the sensor mode. Additionally,\n> the IPA now uses the frame specific horizontal blanking value (as returned by\n> DelayedControls) in all instances.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 47 +++++++++++++++++------\n> src/ipa/raspberrypi/cam_helper.h | 7 ++--\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 24 +++++++-----\n> src/ipa/raspberrypi/cam_helper_imx519.cpp | 24 +++++++-----\n> src/ipa/raspberrypi/raspberrypi.cpp | 45 +++++++++++++---------\n> 5 files changed, 95 insertions(+), 52 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index c255ab0cb53f..f5f034ece711 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -8,6 +8,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -74,33 +75,57 @@ Duration CamHelper::exposure(uint32_t exposureLines, const Duration lineLength)\n> \treturn exposureLines * lineLength;\n> }\n> \n> -uint32_t CamHelper::getVBlanking(Duration &exposure,\n> -\t\t\t\t Duration minFrameDuration,\n> -\t\t\t\t Duration maxFrameDuration) const\n> +std::pair CamHelper::getBlanking(Duration &exposure,\n> +\t\t\t\t\t\t Duration minFrameDuration,\n> +\t\t\t\t\t\t Duration maxFrameDuration) const\n> {\n> -\tuint32_t frameLengthMin, frameLengthMax, vblank;\n> -\tuint32_t exposureLines = CamHelper::exposureLines(exposure, mode_.minLineLength);\n> +\tuint32_t frameLengthMin, frameLengthMax, vblank, hblank;\n> +\tDuration lineLength = mode_.minLineLength;\n> \n> \tassert(initialized_);\n> \n> \t/*\n> \t * minFrameDuration and maxFrameDuration are clamped by the caller\n> \t * based on the limits for the active sensor mode.\n> +\t *\n> +\t * frameLengthMax gets calculated on the smallest line length as we do\n> +\t * not want to extend that unless absolutely necessary.\n> \t */\n> \tframeLengthMin = minFrameDuration / mode_.minLineLength;\n> \tframeLengthMax = maxFrameDuration / mode_.minLineLength;\n> \n> +\t/*\n> +\t * Watch out for exposureLines overflowing a uint32_t when the exposure\n> +\t * time is extremely (extremely!) long - as happens when the IPA calculates\n> +\t * the maximum possible exposure time.\n> +\t */\n> +\tuint32_t exposureLines = std::min(CamHelper::exposureLines(exposure, lineLength),\n> +\t\t\t\t\t std::numeric_limits::max() - frameIntegrationDiff_);\n> +\tuint32_t frameLengthLines = std::clamp(exposureLines + frameIntegrationDiff_,\n> +\t\t\t\t\t frameLengthMin, frameLengthMax);\n\nThis doesn't seem to match the comment. You're protecting against\nframeLenghtLines overflowing, but not exposureLines().\n\n> +\n> +\t/*\n> +\t * If our frame length lines is above the maximum allowed, see if we can\n> +\t * extend the line length to accommodate the requested frame length.\n> +\t */\n> +\tif (frameLengthLines > mode_.maxFrameLength) {\n> +\t\tDuration lineLengthAdjusted = lineLength * frameLengthLines / mode_.maxFrameLength;\n> +\t\tlineLength = std::min(mode_.maxLineLength, lineLengthAdjusted);\n> +\t\tframeLengthLines = mode_.maxFrameLength;\n> +\t}\n> +\n> +\thblank = lineLengthToHblank(lineLength);\n> +\tvblank = frameLengthLines - mode_.height;\n> +\n> \t/*\n> \t * Limit the exposure to the maximum frame duration requested, and\n> \t * re-calculate if it has been clipped.\n> \t */\n> -\texposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);\n> -\texposure = CamHelper::exposure(exposureLines, mode_.minLineLength);\n> +\texposureLines = std::min(frameLengthLines - frameIntegrationDiff_,\n> +\t\t\t\t CamHelper::exposureLines(exposure, lineLength));\n> +\texposure = CamHelper::exposure(exposureLines, lineLength);\n> \n> -\t/* Limit the vblank to the range allowed by the frame length limits. */\n> -\tvblank = std::clamp(exposureLines + frameIntegrationDiff_,\n> -\t\t\t frameLengthMin, frameLengthMax) - mode_.height;\n> -\treturn vblank;\n> +\treturn { vblank, hblank };\n\nI can imagine this being error-prone, with a caller interpreting the\nvalue as { hblank, vblank } (which is what I would have written\nintuitively). A blanking structure with named fields would solve that.\nThe Size class could also be used, but blanking.width and\nblanking.height may be confusing to read.\n\nIt doesn't have to be done in this series, up to you.\n\n> }\n> \n> Duration CamHelper::hblankToLineLength(uint32_t hblank) const\n> diff --git a/src/ipa/raspberrypi/cam_helper.h b/src/ipa/raspberrypi/cam_helper.h\n> index b5c0726ff00e..21ac101a0a0b 100644\n> --- a/src/ipa/raspberrypi/cam_helper.h\n> +++ b/src/ipa/raspberrypi/cam_helper.h\n> @@ -8,6 +8,7 @@\n> \n> #include \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -82,9 +83,9 @@ public:\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> +\tvirtual std::pair getBlanking(libcamera::utils::Duration &exposure,\n> +\t\t\t\t\t\t\t libcamera::utils::Duration minFrameDuration,\n> +\t\t\t\t\t\t\t libcamera::utils::Duration maxFrameDuration) const;\n> \tlibcamera::utils::Duration hblankToLineLength(uint32_t hblank) const;\n> \tuint32_t lineLengthToHblank(const libcamera::utils::Duration &duration) const;\n> \tlibcamera::utils::Duration lineLengthPckToDuration(uint32_t lineLengthPck) const;\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 76a82cc51378..19a5e471c27e 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -46,8 +46,8 @@ public:\n> \tuint32_t gainCode(double gain) const override;\n> \tdouble gain(uint32_t gainCode) const override;\n> \tvoid prepare(libcamera::Span buffer, Metadata &metadata) override;\n> -\tuint32_t getVBlanking(Duration &exposure, Duration minFrameDuration,\n> -\t\t\t Duration maxFrameDuration) const override;\n> +\tstd::pair getBlanking(Duration &exposure, Duration minFrameDuration,\n> +\t\t\t\t\t\t Duration maxFrameDuration) const override;\n> \tvoid getDelays(int &exposureDelay, int &gainDelay,\n> \t\t int &vblankDelay, int &hblankDelay) const override;\n> \tbool sensorEmbeddedDataPresent() const override;\n> @@ -118,15 +118,19 @@ void CamHelperImx477::prepare(libcamera::Span buffer, Metadata &m\n> \t}\n> }\n> \n> -uint32_t CamHelperImx477::getVBlanking(Duration &exposure,\n> -\t\t\t\t Duration minFrameDuration,\n> -\t\t\t\t Duration maxFrameDuration) const\n> +std::pair CamHelperImx477::getBlanking(Duration &exposure,\n> +\t\t\t\t\t\t\t Duration minFrameDuration,\n> +\t\t\t\t\t\t\t Duration maxFrameDuration) const\n> {\n> \tuint32_t frameLength, exposureLines;\n> \tunsigned int shift = 0;\n> \n> -\tframeLength = mode_.height + CamHelper::getVBlanking(exposure, minFrameDuration,\n> -\t\t\t\t\t\t\t maxFrameDuration);\n> +\tauto [vblank, hblank] = CamHelper::getBlanking(exposure, minFrameDuration,\n> +\t\t\t\t\t\t maxFrameDuration);\n> +\n> +\tframeLength = mode_.height + vblank;\n> +\tDuration lineLength = hblankToLineLength(hblank);\n> +\n> \t/*\n> \t * Check if the frame length calculated needs to be setup for long\n> \t * exposure mode. This will require us to use a long exposure scale\n> @@ -144,12 +148,12 @@ 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, mode_.minLineLength);\n> +\t\texposureLines = CamHelperImx477::exposureLines(exposure, lineLength);\n> \t\texposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);\n> -\t\texposure = CamHelperImx477::exposure(exposureLines, mode_.minLineLength);\n> +\t\texposure = CamHelperImx477::exposure(exposureLines, lineLength);\n> \t}\n> \n> -\treturn frameLength - mode_.height;\n> +\treturn { frameLength - mode_.height, hblank };\n> }\n> \n> void CamHelperImx477::getDelays(int &exposureDelay, int &gainDelay,\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx519.cpp b/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> index 9dff1eeb899f..d2eb171912da 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> @@ -46,8 +46,8 @@ public:\n> \tuint32_t gainCode(double gain) const override;\n> \tdouble gain(uint32_t gainCode) const override;\n> \tvoid prepare(libcamera::Span buffer, Metadata &metadata) override;\n> -\tuint32_t getVBlanking(Duration &exposure, Duration minFrameDuration,\n> -\t\t\t Duration maxFrameDuration) const override;\n> +\tstd::pair getBlanking(Duration &exposure, Duration minFrameDuration,\n> +\t\t\t\t\t\t Duration maxFrameDuration) const override;\n> \tvoid getDelays(int &exposureDelay, int &gainDelay,\n> \t\t int &vblankDelay, int &hblankDelay) const override;\n> \tbool sensorEmbeddedDataPresent() const override;\n> @@ -118,15 +118,19 @@ void CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n> \t}\n> }\n> \n> -uint32_t CamHelperImx519::getVBlanking(Duration &exposure,\n> -\t\t\t\t Duration minFrameDuration,\n> -\t\t\t\t Duration maxFrameDuration) const\n> +std::pair CamHelperImx519::getBlanking(Duration &exposure,\n> +\t\t\t\t\t\t\t Duration minFrameDuration,\n> +\t\t\t\t\t\t\t Duration maxFrameDuration) const\n> {\n> \tuint32_t frameLength, exposureLines;\n> \tunsigned int shift = 0;\n> \n> -\tframeLength = mode_.height + CamHelper::getVBlanking(exposure, minFrameDuration,\n> -\t\t\t\t\t\t\t maxFrameDuration);\n> +\tauto [vblank, hblank] = CamHelper::getBlanking(exposure, minFrameDuration,\n> +\t\t\t\t\t\t maxFrameDuration);\n> +\n> +\tframeLength = mode_.height + vblank;\n> +\tDuration lineLength = hblankToLineLength(hblank);\n> +\n> \t/*\n> \t * Check if the frame length calculated needs to be setup for long\n> \t * exposure mode. This will require us to use a long exposure scale\n> @@ -144,12 +148,12 @@ 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, mode_.minLineLength);\n> +\t\texposureLines = CamHelperImx519::exposureLines(exposure, lineLength);\n> \t\texposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);\n> -\t\texposure = CamHelperImx519::exposure(exposureLines, mode_.minLineLength);\n> +\t\texposure = CamHelperImx519::exposure(exposureLines, lineLength);\n> \t}\n> \n> -\treturn frameLength - mode_.height;\n> +\treturn { frameLength - mode_.height, hblank };\n> }\n> \n> void CamHelperImx519::getDelays(int &exposureDelay, int &gainDelay,\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 5d6b22ef6813..497a83939ae6 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -315,7 +315,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> \t}\n> \n> \tstartConfig->dropFrameCount = dropFrameCount_;\n> -\tconst Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength;\n> +\tconst Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> \tstartConfig->maxSensorFrameLengthMs = maxSensorFrameDuration.get();\n> \n> \tfirstStart_ = false;\n> @@ -462,7 +462,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> \t */\n> \tControlInfoMap::Map ctrlMap = ipaControls;\n> \tconst Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;\n> -\tconst Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength;\n> +\tconst Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> \tctrlMap[&controls::FrameDurationLimits] =\n> \t\tControlInfo(static_cast(minSensorFrameDuration.get()),\n> \t\t\t static_cast(maxSensorFrameDuration.get()));\n> @@ -475,7 +475,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> \t * will limit the maximum control value based on the current VBLANK value.\n> \t */\n> \tDuration maxShutter = Duration::max();\n> -\thelper_->getVBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration);\n> +\thelper_->getBlanking(maxShutter, minSensorFrameDuration, maxSensorFrameDuration);\n> \tconst uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get();\n> \n> \tctrlMap[&controls::ExposureTime] =\n> @@ -552,7 +552,7 @@ void IPARPi::reportMetadata()\n> \t\t\t\t deviceStatus->shutterSpeed.get());\n> \t\tlibcameraMetadata_.set(controls::AnalogueGain, deviceStatus->analogueGain);\n> \t\tlibcameraMetadata_.set(controls::FrameDuration,\n> -\t\t\t\t helper_->exposure(deviceStatus->frameLength, mode_.minLineLength).get());\n> +\t\t\t\t helper_->exposure(deviceStatus->frameLength, deviceStatus->lineLength).get());\n> \t\tif (deviceStatus->sensorTemperature)\n> \t\t\tlibcameraMetadata_.set(controls::SensorTemperature, *deviceStatus->sensorTemperature);\n> \t}\n> @@ -1110,8 +1110,8 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n> \tint32_t vblank = sensorControls.get(V4L2_CID_VBLANK).get();\n> \tint32_t hblank = sensorControls.get(V4L2_CID_HBLANK).get();\n> \n> -\tdeviceStatus.lineLength = (mode_.width + hblank) * (1.0s / mode_.pixelRate);\n> -\tdeviceStatus.shutterSpeed = helper_->exposure(exposureLines, mode_.minLineLength);\n> +\tdeviceStatus.lineLength = helper_->hblankToLineLength(hblank);\n> +\tdeviceStatus.shutterSpeed = helper_->exposure(exposureLines, deviceStatus.lineLength);\n> \tdeviceStatus.analogueGain = helper_->gain(gainCode);\n> \tdeviceStatus.frameLength = mode_.height + vblank;\n> \n> @@ -1157,7 +1157,7 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration)\n> {\n> \tconst Duration minSensorFrameDuration = mode_.minFrameLength * mode_.minLineLength;\n> -\tconst Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.minLineLength;\n> +\tconst Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> \n> \t/*\n> \t * This will only be applied once AGC recalculations occur.\n> @@ -1178,11 +1178,11 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur\n> \n> \t/*\n> \t * Calculate the maximum exposure time possible for the AGC to use.\n> -\t * getVBlanking() will update maxShutter with the largest exposure\n> +\t * getBlanking() will update maxShutter with the largest exposure\n> \t * value possible.\n> \t */\n> \tDuration maxShutter = Duration::max();\n> -\thelper_->getVBlanking(maxShutter, minFrameDuration_, maxFrameDuration_);\n> +\thelper_->getBlanking(maxShutter, minFrameDuration_, maxFrameDuration_);\n> \n> \tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\tcontroller_.getAlgorithm(\"agc\"));\n> @@ -1200,10 +1200,11 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> \t */\n> \tgainCode = std::min(gainCode, maxSensorGainCode_);\n> \n> -\t/* getVBlanking might clip exposure time to the fps limits. */\n> +\t/* getBlanking 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, mode_.minLineLength);\n> +\tauto [vblank, hblank] = helper_->getBlanking(exposure, minFrameDuration_, maxFrameDuration_);\n> +\tint32_t exposureLines = helper_->exposureLines(exposure,\n> +\t\t\t\t\t\t helper_->hblankToLineLength(hblank));\n> \n> \tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> \t\t\t << \" (Shutter lines: \" << exposureLines << \", AGC requested \"\n> @@ -1211,14 +1212,22 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> \t\t\t << agcStatus->analogueGain << \" (Gain Code: \"\n> \t\t\t << gainCode << \")\";\n> \n> -\t/*\n> -\t * Due to the behavior of V4L2, the current value of VBLANK could clip the\n> -\t * exposure time without us knowing. The next time though this function should\n> -\t * clip exposure correctly.\n> -\t */\n> -\tctrls.set(V4L2_CID_VBLANK, vblanking);\n> +\tctrls.set(V4L2_CID_VBLANK, static_cast(vblank));\n> \tctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> \tctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> +\n> +\t/*\n> +\t * At present, there is no way of knowing if a control is read-only.\n> +\t * As a workaround, assume that if the minimum and maximum values of\n> +\t * the V4L2_CID_HBLANK control are the same, it implies the control\n> +\t * is read-only. This seems to be the case for all the cameras our IPA\n> +\t * works with.\n> +\t *\n> +\t * \\todo The control API ought to have a flag to specify if a control\n> +\t * is read-only which could be used below.\n\nIndeed, that could be useful.\n\n> +\t */\n> +\tif (mode_.minLineLength != mode_.maxLineLength)\n> +\t\tctrls.set(V4L2_CID_HBLANK, static_cast(hblank));\n> }\n> \n> void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)","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 6D0C2BD16B\n\tfor ;\n\tTue, 4 Oct 2022 17:46:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F9E660AB5;\n\tTue, 4 Oct 2022 19:46:16 +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 03772601C7\n\tfor ;\n\tTue, 4 Oct 2022 19:46:14 +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 4F33B2D9;\n\tTue, 4 Oct 2022 19:46:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664905576;\n\tbh=EcdguX9mtz288u8B6I+UX/dKPijgbqh6KNMVBC1I5F8=;\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=xdUihdd9lU8/CUNLJUtGIUeU79o13C99wwf4dWPhsz24NHJv1UUC6xkrzomYyA0Vc\n\t8dJWxmkKhjLCS1IG0v4bwtmNuf+ia+VcfxcygrstzfEWTpoFeAl/50w8E2bHC4CjVS\n\t3jIovRoADjPLMvM4JyKWPc8nKNGngbV/3H239dR4P0XRm2IyV6CPUy1ACrhZoW069p\n\tshdV/YiZESQaLjh6f1yTGZsZMqo+L2hku58ew51YghXncF0vb2C1ssmXn0JGfjvyX7\n\tnVCF+lxBGtMJVQht8QYw2HCm1unT2mopbj8iG8ItD+OkmI87EaJlDgikHBpJ3ukVak\n\tl2mxmZaVGSECw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1664905574;\n\tbh=EcdguX9mtz288u8B6I+UX/dKPijgbqh6KNMVBC1I5F8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=haIIGHix5P6Z7V4lxLC6o2BrIQ4lofo27A7jKyE6942GEqBWTQfoBadzf8fWoSesg\n\t3qFsOEfjfIRWHwCglwohjhKSTv6+zhJFjcpB3hRVbGKSava0DEPysQ3B4jQN6yAxVs\n\tJJlUs33XVsM5z3XS+XH/z6EVb3WKreaEuziOc5bs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"haIIGHix\"; dkim-atps=neutral","Date":"Tue, 4 Oct 2022 20:46:11 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20221003083934.31629-1-naush@raspberrypi.com>\n\t<20221003083934.31629-9-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221003083934.31629-9-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 8/9] ipa: raspberrypi: Allow full\n\tline length control","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25312,"web_url":"https://patchwork.libcamera.org/comment/25312/","msgid":"","date":"2022-10-05T13:38:49","subject":"Re: [libcamera-devel] [PATCH v1 8/9] ipa: raspberrypi: Allow full\n\tline length control","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 4 Oct 2022 at 18:46, 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:34AM +0100, Naushir Patuck via\n> libcamera-devel wrote:\n> > Rename CamHelper::getVBlanking to CamHelper::getBlanking, and update the\n> > calculations in that function to return both horizontal and vertical\n> blanking\n> > values for a given exposure time and frame duration limits. The\n> calculations\n> > are setup such that vertical blanking is extended to the maximum\n> allowable\n> > value, and any remainder gets put into horizontal blanking.\n>\n> Hmmm... It would be nice if the heuristics was implemented in the IPA\n> module itself and not in the helpers, but given that the helpers are\n> specific to the Raspberry Pi IPA module, I think that's fine for now.\n>\n> > The calculated horizontal blanking value is now return to the pipeline\n> handler\n>\n> s/return/returned/\n>\n> > to pass into DelayedControls to program into the sensor.\n> >\n> > Update the IPA to now specify the maximum frame duration from the maximum\n> > horizontal + vertical blanking values provided by the sensor mode.\n> Additionally,\n> > the IPA now uses the frame specific horizontal blanking value (as\n> returned by\n> > DelayedControls) in all instances.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 47 +++++++++++++++++------\n> > src/ipa/raspberrypi/cam_helper.h | 7 ++--\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 24 +++++++-----\n> > src/ipa/raspberrypi/cam_helper_imx519.cpp | 24 +++++++-----\n> > src/ipa/raspberrypi/raspberrypi.cpp | 45 +++++++++++++---------\n> > 5 files changed, 95 insertions(+), 52 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > index c255ab0cb53f..f5f034ece711 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -74,33 +75,57 @@ Duration CamHelper::exposure(uint32_t exposureLines,\n> const Duration lineLength)\n> > return exposureLines * lineLength;\n> > }\n> >\n> > -uint32_t CamHelper::getVBlanking(Duration &exposure,\n> > - Duration minFrameDuration,\n> > - Duration maxFrameDuration) const\n> > +std::pair CamHelper::getBlanking(Duration &exposure,\n> > + Duration\n> minFrameDuration,\n> > + Duration\n> maxFrameDuration) const\n> > {\n> > - uint32_t frameLengthMin, frameLengthMax, vblank;\n> > - uint32_t exposureLines = CamHelper::exposureLines(exposure,\n> mode_.minLineLength);\n> > + uint32_t frameLengthMin, frameLengthMax, vblank, hblank;\n> > + Duration lineLength = mode_.minLineLength;\n> >\n> > assert(initialized_);\n> >\n> > /*\n> > * minFrameDuration and maxFrameDuration are clamped by the caller\n> > * based on the limits for the active sensor mode.\n> > + *\n> > + * frameLengthMax gets calculated on the smallest line length as\n> we do\n> > + * not want to extend that unless absolutely necessary.\n> > */\n> > frameLengthMin = minFrameDuration / mode_.minLineLength;\n> > frameLengthMax = maxFrameDuration / mode_.minLineLength;\n> >\n> > + /*\n> > + * Watch out for exposureLines overflowing a uint32_t when the\n> exposure\n> > + * time is extremely (extremely!) long - as happens when the IPA\n> calculates\n> > + * the maximum possible exposure time.\n> > + */\n> > + uint32_t exposureLines =\n> std::min(CamHelper::exposureLines(exposure, lineLength),\n> > +\n> std::numeric_limits::max() - frameIntegrationDiff_);\n> > + uint32_t frameLengthLines = std::clamp(exposureLines +\n> frameIntegrationDiff_,\n> > + frameLengthMin,\n> frameLengthMax);\n>\n> This doesn't seem to match the comment. You're protecting against\n> frameLenghtLines overflowing, but not exposureLines().\n>\n\nI'll clarify the comment above. What I meant to convey here was the\nexposureLines + frameIntegrationDiff_ bit of the clamp() can overflow the\nuint32_t storage so frameLengthLines could clamp to the wrong value. To\navoid this we do the std::min() statement just above.\n\nRegards,\nNaush\n\n\n>\n> > +\n> > + /*\n> > + * If our frame length lines is above the maximum allowed, see if\n> we can\n> > + * extend the line length to accommodate the requested frame\n> length.\n> > + */\n> > + if (frameLengthLines > mode_.maxFrameLength) {\n> > + Duration lineLengthAdjusted = lineLength *\n> frameLengthLines / mode_.maxFrameLength;\n> > + lineLength = std::min(mode_.maxLineLength,\n> lineLengthAdjusted);\n> > + frameLengthLines = mode_.maxFrameLength;\n> > + }\n> > +\n> > + hblank = lineLengthToHblank(lineLength);\n> > + vblank = frameLengthLines - mode_.height;\n> > +\n> > /*\n> > * Limit the exposure to the maximum frame duration requested, and\n> > * re-calculate if it has been clipped.\n> > */\n> > - exposureLines = std::min(frameLengthMax - frameIntegrationDiff_,\n> exposureLines);\n> > - exposure = CamHelper::exposure(exposureLines, mode_.minLineLength);\n> > + exposureLines = std::min(frameLengthLines - frameIntegrationDiff_,\n> > + CamHelper::exposureLines(exposure,\n> lineLength));\n> > + exposure = CamHelper::exposure(exposureLines, lineLength);\n> >\n> > - /* Limit the vblank to the range allowed by the frame length\n> limits. */\n> > - vblank = std::clamp(exposureLines + frameIntegrationDiff_,\n> > - frameLengthMin, frameLengthMax) - mode_.height;\n> > - return vblank;\n> > + return { vblank, hblank };\n>\n> I can imagine this being error-prone, with a caller interpreting the\n> value as { hblank, vblank } (which is what I would have written\n> intuitively). A blanking structure with named fields would solve that.\n> The Size class could also be used, but blanking.width and\n> blanking.height may be confusing to read.\n>\n> It doesn't have to be done in this series, up to you.\n>\n> > }\n> >\n> > Duration CamHelper::hblankToLineLength(uint32_t hblank) const\n> > diff --git a/src/ipa/raspberrypi/cam_helper.h\n> b/src/ipa/raspberrypi/cam_helper.h\n> > index b5c0726ff00e..21ac101a0a0b 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.h\n> > +++ b/src/ipa/raspberrypi/cam_helper.h\n> > @@ -8,6 +8,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> >\n> > #include \n> > #include \n> > @@ -82,9 +83,9 @@ public:\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> > + virtual std::pair\n> getBlanking(libcamera::utils::Duration &exposure,\n> > +\n> libcamera::utils::Duration minFrameDuration,\n> > +\n> libcamera::utils::Duration maxFrameDuration) const;\n> > libcamera::utils::Duration hblankToLineLength(uint32_t hblank)\n> const;\n> > uint32_t lineLengthToHblank(const libcamera::utils::Duration\n> &duration) const;\n> > libcamera::utils::Duration lineLengthPckToDuration(uint32_t\n> lineLengthPck) const;\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index 76a82cc51378..19a5e471c27e 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -46,8 +46,8 @@ public:\n> > uint32_t gainCode(double gain) const override;\n> > double gain(uint32_t gainCode) const override;\n> > void prepare(libcamera::Span buffer, Metadata\n> &metadata) override;\n> > - uint32_t getVBlanking(Duration &exposure, Duration\n> minFrameDuration,\n> > - Duration maxFrameDuration) const override;\n> > + std::pair getBlanking(Duration &exposure,\n> Duration minFrameDuration,\n> > + Duration\n> maxFrameDuration) const override;\n> > void getDelays(int &exposureDelay, int &gainDelay,\n> > int &vblankDelay, int &hblankDelay) const override;\n> > bool sensorEmbeddedDataPresent() const override;\n> > @@ -118,15 +118,19 @@ void\n> CamHelperImx477::prepare(libcamera::Span buffer, Metadata &m\n> > }\n> > }\n> >\n> > -uint32_t CamHelperImx477::getVBlanking(Duration &exposure,\n> > - Duration minFrameDuration,\n> > - Duration maxFrameDuration) const\n> > +std::pair CamHelperImx477::getBlanking(Duration\n> &exposure,\n> > + Duration\n> minFrameDuration,\n> > + Duration\n> maxFrameDuration) const\n> > {\n> > uint32_t frameLength, exposureLines;\n> > unsigned int shift = 0;\n> >\n> > - frameLength = mode_.height + CamHelper::getVBlanking(exposure,\n> minFrameDuration,\n> > -\n> maxFrameDuration);\n> > + auto [vblank, hblank] = CamHelper::getBlanking(exposure,\n> minFrameDuration,\n> > + maxFrameDuration);\n> > +\n> > + frameLength = mode_.height + vblank;\n> > + Duration lineLength = hblankToLineLength(hblank);\n> > +\n> > /*\n> > * Check if the frame length calculated needs to be setup for long\n> > * exposure mode. This will require us to use a long exposure scale\n> > @@ -144,12 +148,12 @@ 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> mode_.minLineLength);\n> > + exposureLines = CamHelperImx477::exposureLines(exposure,\n> lineLength);\n> > exposureLines = std::min(exposureLines, frameLength -\n> frameIntegrationDiff);\n> > - exposure = CamHelperImx477::exposure(exposureLines,\n> mode_.minLineLength);\n> > + exposure = CamHelperImx477::exposure(exposureLines,\n> lineLength);\n> > }\n> >\n> > - return frameLength - mode_.height;\n> > + return { frameLength - mode_.height, hblank };\n> > }\n> >\n> > void CamHelperImx477::getDelays(int &exposureDelay, int &gainDelay,\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> > index 9dff1eeb899f..d2eb171912da 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx519.cpp\n> > @@ -46,8 +46,8 @@ public:\n> > uint32_t gainCode(double gain) const override;\n> > double gain(uint32_t gainCode) const override;\n> > void prepare(libcamera::Span buffer, Metadata\n> &metadata) override;\n> > - uint32_t getVBlanking(Duration &exposure, Duration\n> minFrameDuration,\n> > - Duration maxFrameDuration) const override;\n> > + std::pair getBlanking(Duration &exposure,\n> Duration minFrameDuration,\n> > + Duration\n> maxFrameDuration) const override;\n> > void getDelays(int &exposureDelay, int &gainDelay,\n> > int &vblankDelay, int &hblankDelay) const override;\n> > bool sensorEmbeddedDataPresent() const override;\n> > @@ -118,15 +118,19 @@ void\n> CamHelperImx519::prepare(libcamera::Span buffer, Metadata &m\n> > }\n> > }\n> >\n> > -uint32_t CamHelperImx519::getVBlanking(Duration &exposure,\n> > - Duration minFrameDuration,\n> > - Duration maxFrameDuration) const\n> > +std::pair CamHelperImx519::getBlanking(Duration\n> &exposure,\n> > + Duration\n> minFrameDuration,\n> > + Duration\n> maxFrameDuration) const\n> > {\n> > uint32_t frameLength, exposureLines;\n> > unsigned int shift = 0;\n> >\n> > - frameLength = mode_.height + CamHelper::getVBlanking(exposure,\n> minFrameDuration,\n> > -\n> maxFrameDuration);\n> > + auto [vblank, hblank] = CamHelper::getBlanking(exposure,\n> minFrameDuration,\n> > + maxFrameDuration);\n> > +\n> > + frameLength = mode_.height + vblank;\n> > + Duration lineLength = hblankToLineLength(hblank);\n> > +\n> > /*\n> > * Check if the frame length calculated needs to be setup for long\n> > * exposure mode. This will require us to use a long exposure scale\n> > @@ -144,12 +148,12 @@ 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> mode_.minLineLength);\n> > + exposureLines = CamHelperImx519::exposureLines(exposure,\n> lineLength);\n> > exposureLines = std::min(exposureLines, frameLength -\n> frameIntegrationDiff);\n> > - exposure = CamHelperImx519::exposure(exposureLines,\n> mode_.minLineLength);\n> > + exposure = CamHelperImx519::exposure(exposureLines,\n> lineLength);\n> > }\n> >\n> > - return frameLength - mode_.height;\n> > + return { frameLength - mode_.height, hblank };\n> > }\n> >\n> > void CamHelperImx519::getDelays(int &exposureDelay, int &gainDelay,\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 5d6b22ef6813..497a83939ae6 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -315,7 +315,7 @@ void IPARPi::start(const ControlList &controls,\n> StartConfig *startConfig)\n> > }\n> >\n> > startConfig->dropFrameCount = dropFrameCount_;\n> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength *\n> mode_.minLineLength;\n> > + const Duration maxSensorFrameDuration = mode_.maxFrameLength *\n> mode_.maxLineLength;\n> > startConfig->maxSensorFrameLengthMs =\n> maxSensorFrameDuration.get();\n> >\n> > firstStart_ = false;\n> > @@ -462,7 +462,7 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> > */\n> > ControlInfoMap::Map ctrlMap = ipaControls;\n> > const Duration minSensorFrameDuration = mode_.minFrameLength *\n> mode_.minLineLength;\n> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength *\n> mode_.minLineLength;\n> > + const Duration maxSensorFrameDuration = mode_.maxFrameLength *\n> mode_.maxLineLength;\n> > ctrlMap[&controls::FrameDurationLimits] =\n> >\n> ControlInfo(static_cast(minSensorFrameDuration.get()),\n> >\n> static_cast(maxSensorFrameDuration.get()));\n> > @@ -475,7 +475,7 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> > * will limit the maximum control value based on the current\n> VBLANK value.\n> > */\n> > Duration maxShutter = Duration::max();\n> > - helper_->getVBlanking(maxShutter, minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > + helper_->getBlanking(maxShutter, minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > const uint32_t exposureMin =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get();\n> >\n> > ctrlMap[&controls::ExposureTime] =\n> > @@ -552,7 +552,7 @@ void IPARPi::reportMetadata()\n> >\n> deviceStatus->shutterSpeed.get());\n> > libcameraMetadata_.set(controls::AnalogueGain,\n> deviceStatus->analogueGain);\n> > libcameraMetadata_.set(controls::FrameDuration,\n> > -\n> helper_->exposure(deviceStatus->frameLength,\n> mode_.minLineLength).get());\n> > +\n> helper_->exposure(deviceStatus->frameLength,\n> deviceStatus->lineLength).get());\n> > if (deviceStatus->sensorTemperature)\n> >\n> libcameraMetadata_.set(controls::SensorTemperature,\n> *deviceStatus->sensorTemperature);\n> > }\n> > @@ -1110,8 +1110,8 @@ void IPARPi::fillDeviceStatus(const ControlList\n> &sensorControls)\n> > int32_t vblank =\n> sensorControls.get(V4L2_CID_VBLANK).get();\n> > int32_t hblank =\n> sensorControls.get(V4L2_CID_HBLANK).get();\n> >\n> > - deviceStatus.lineLength = (mode_.width + hblank) * (1.0s /\n> mode_.pixelRate);\n> > - deviceStatus.shutterSpeed = helper_->exposure(exposureLines,\n> mode_.minLineLength);\n> > + deviceStatus.lineLength = helper_->hblankToLineLength(hblank);\n> > + deviceStatus.shutterSpeed = helper_->exposure(exposureLines,\n> deviceStatus.lineLength);\n> > deviceStatus.analogueGain = helper_->gain(gainCode);\n> > deviceStatus.frameLength = mode_.height + vblank;\n> >\n> > @@ -1157,7 +1157,7 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> > void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration\n> maxFrameDuration)\n> > {\n> > const Duration minSensorFrameDuration = mode_.minFrameLength *\n> mode_.minLineLength;\n> > - const Duration maxSensorFrameDuration = mode_.maxFrameLength *\n> mode_.minLineLength;\n> > + const Duration maxSensorFrameDuration = mode_.maxFrameLength *\n> mode_.maxLineLength;\n> >\n> > /*\n> > * This will only be applied once AGC recalculations occur.\n> > @@ -1178,11 +1178,11 @@ void IPARPi::applyFrameDurations(Duration\n> minFrameDuration, Duration maxFrameDur\n> >\n> > /*\n> > * Calculate the maximum exposure time possible for the AGC to use.\n> > - * getVBlanking() will update maxShutter with the largest exposure\n> > + * getBlanking() will update maxShutter with the largest exposure\n> > * value possible.\n> > */\n> > Duration maxShutter = Duration::max();\n> > - helper_->getVBlanking(maxShutter, minFrameDuration_,\n> maxFrameDuration_);\n> > + helper_->getBlanking(maxShutter, minFrameDuration_,\n> maxFrameDuration_);\n> >\n> > RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> > controller_.getAlgorithm(\"agc\"));\n> > @@ -1200,10 +1200,11 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> > */\n> > gainCode = std::min(gainCode, maxSensorGainCode_);\n> >\n> > - /* getVBlanking might clip exposure time to the fps limits. */\n> > + /* getBlanking 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> mode_.minLineLength);\n> > + auto [vblank, hblank] = helper_->getBlanking(exposure,\n> minFrameDuration_, maxFrameDuration_);\n> > + int32_t exposureLines = helper_->exposureLines(exposure,\n> > +\n> helper_->hblankToLineLength(hblank));\n> >\n> > LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> > << \" (Shutter lines: \" << exposureLines << \",\n> AGC requested \"\n> > @@ -1211,14 +1212,22 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> > << agcStatus->analogueGain << \" (Gain Code: \"\n> > << gainCode << \")\";\n> >\n> > - /*\n> > - * Due to the behavior of V4L2, the current value of VBLANK could\n> clip the\n> > - * exposure time without us knowing. The next time though this\n> function should\n> > - * clip exposure correctly.\n> > - */\n> > - ctrls.set(V4L2_CID_VBLANK, vblanking);\n> > + ctrls.set(V4L2_CID_VBLANK, static_cast(vblank));\n> > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> > ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > +\n> > + /*\n> > + * At present, there is no way of knowing if a control is\n> read-only.\n> > + * As a workaround, assume that if the minimum and maximum values\n> of\n> > + * the V4L2_CID_HBLANK control are the same, it implies the control\n> > + * is read-only. This seems to be the case for all the cameras our\n> IPA\n> > + * works with.\n> > + *\n> > + * \\todo The control API ought to have a flag to specify if a\n> control\n> > + * is read-only which could be used below.\n>\n> Indeed, that could be useful.\n>\n> > + */\n> > + if (mode_.minLineLength != mode_.maxLineLength)\n> > + ctrls.set(V4L2_CID_HBLANK, static_cast(hblank));\n> > }\n> >\n> > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList\n> &ctrls)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 6AA36C0DA4\n\tfor ;\n\tWed, 5 Oct 2022 13:39:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC54D6239B;\n\tWed, 5 Oct 2022 15:39:08 +0200 (CEST)","from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n\t[IPv6:2a00:1450:4864:20::12f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 719BD601C7\n\tfor ;\n\tWed, 5 Oct 2022 15:39:06 +0200 (CEST)","by mail-lf1-x12f.google.com with SMTP id z4so25608857lft.2\n\tfor ;\n\tWed, 05 Oct 2022 06:39:06 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1664977148;\n\tbh=Eu5P4v/9yH3aiMdgU0kibpQLva8tWGnGK1lTT0BJg/Q=;\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=MsS1bKUc9YfxUwJ3sCF+17KTrHfZOCSuV6Kwb240CqEVA4Sgu9Kvf9SPb/ri4tmZU\n\taPpMyKNNbbQm9x2wdOpKFG38LFKALsPtklHKYnNNILlcAnGrt355HqOoQ63aR80b+w\n\tnH0yKYba303ZmjHazd3f6kfPHDc3/r2/RUK/BS92TWaVX9RKXhQM4blEiL5Bd+Ww/z\n\te5iFWIjz0yKHppK5Zvaelsj1+zK0MZ8tZz6fauBA0Haq6LnaQisr880yOq8SOEp5vY\n\tBp5pHAZWgQelnod8b2/A5aqcU/eEsZDE4b4YXn9JjX638YD+tOQP+YV2caSNei+c/7\n\tg6QuPpO8cfJLw==","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=r/n4/deb0gQ3ROQ5eTa4L+0y1dmy/X8sqk2MKOOs0Q8=;\n\tb=SXVXOwKWoCUH7ywC+AEQuJOmnQXLzIwbysQc7FPdfRibZ8Md7AASS9D3h1IQY5rPOL\n\tS8PYwbXSaky280oO8mnaCdux25kDO3vGne1xRBlVPP69cNOAw7mELzJElAfFi9sMrR+T\n\tqvS2k30iBVDJqxlTZEhL/8Mc4bZLU3g22TwRqYbWPoG1egWMllY9Sn+C2oXLgr3Wk4mu\n\ts7dAWVrOjH0rrODFR4KdgLyYhjBzKLjlnMUgqbQwSYtVXDSBaF+kULLlwRa91D+ILMT/\n\tD/SJn0JJizyLI0IBwtoMcrKudd/hdbr+Ibl8ohmrGhibO4FcFP/6GZRwkqWSsTe90YmA\n\tz/VA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"SXVXOwKW\"; 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=r/n4/deb0gQ3ROQ5eTa4L+0y1dmy/X8sqk2MKOOs0Q8=;\n\tb=w7nrThJKNENMtge0XkkQ3e1Bhiwf/+/OYnEIxrBTjULJLb+8HpWi5E4DtV4vzcU+LT\n\tLexd39xtlo3SYFluI6NgHfYmQ1v4cGcyfp5wNTsUTim6o+dBSDt6uehsBn9mGwSRHJwG\n\tTKsrOj8MQGYx89ugg2jgCTkCpmlPIF8W9rPsEpvIme4HN9LEHz8PWkZf9obmquqT9hAr\n\tpd3X8sFr1OiMqSvLfoid/C8visok6YNRea4Ng9D3s2t2EacQ220MeXm/k+7V5EE5MsBU\n\tolubbfuH5hKv/M2CTZ4T984kJqy56uOGYCmcRNxKEueHcCWSRYok87h5wvvHPOwoi83K\n\tF58w==","X-Gm-Message-State":"ACrzQf1B86AxHy+HNWKWD1eV1CpM2lHhbGeIe7BB0s/rp1dFbwEuXDnn\n\t05orm5SRBBBUkecEik5ipcuYwXMofXMnCmUhWugNP84skLd5Cw==","X-Google-Smtp-Source":"AMsMyM5xCeogkmnOf6TSQuwY9X7rQ8avdXEwXe34JjxtmVyuqJgQXe43CU6OSNyeeqqvcrQ6RwMhAmbD/SoDj+ZOmWU=","X-Received":"by 2002:a05:6512:419:b0:4a2:2e45:bf0 with SMTP id\n\tu25-20020a056512041900b004a22e450bf0mr6752510lfk.356.1664977145739;\n\tWed, 05 Oct 2022 06:39:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20221003083934.31629-1-naush@raspberrypi.com>\n\t<20221003083934.31629-9-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Wed, 5 Oct 2022 14:38:49 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000054ce4005ea49b377\"","Subject":"Re: [libcamera-devel] [PATCH v1 8/9] ipa: raspberrypi: Allow full\n\tline length control","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]