[{"id":14272,"web_url":"https://patchwork.libcamera.org/comment/14272/","msgid":"<7e343907-c17f-5489-4125-99415bf3eda4@ideasonboard.com>","date":"2020-12-18T13:04:02","subject":"Re: [libcamera-devel] [PATCH v10 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 18/12/2020 10:06, Naushir Patuck wrote:\n> Add support for setting V4L2_CID_VBLANK appropriately when setting\n> V4L2_CID_EXPOSURE. This will allow adaptive framerates during\n> viewfinder use cases (e.g. when the exposure time goes above 33ms, we\n> can reduce the framerate to lower than 30fps).\n> \n> The minimum and maximum frame durations are provided via libcamera\n> controls, and will prioritise exposure time limits over any AGC request.\n> \n> V4L2_CID_VBLANK is controlled through the staggered writer, just like\n> the exposure and gain controls.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Tested-by: David Plowman \n> Reviewed-by: Jacopo Mondi \n> ---\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/raspberrypi/cam_helper.cpp | 35 ++++++++++++-\n> src/ipa/raspberrypi/cam_helper.hpp | 15 +++++-\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 ++++-\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 +++-\n> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 +++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 52 ++++++++++++++++---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> 8 files changed, 127 insertions(+), 14 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 01fe5abc..1de36039 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {\n> \t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> \t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> \t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> +\t{ &controls::FrameDurations, ControlInfo(1000, 1 000 000 000) },\n\nPerhaps we should define some human readable $bignumbers to ensure we\ndon't mis-represent or mis-calculate large numbers like that.\n\nOr use some chrono class to be able to express the value in readable\nterms, but store it appropriately. Of course then it gets more difficult\nto handle that in the Control I think ...\n\nBut anyway, not in this patch.\n\n> };\n> \n> } /* namespace RPi */\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 6efa0d7f..018c17b9 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const &cam_name)\n> \treturn nullptr;\n> }\n> \n> -CamHelper::CamHelper(MdParser *parser)\n> -\t: parser_(parser), initialized_(false)\n> +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> +\t\t unsigned int frameIntegrationDiff)\n> +\t: parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),\n> +\t frameIntegrationDiff_(frameIntegrationDiff)\n> {\n> }\n> \n> @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines) const\n> \treturn exposure_lines * mode_.line_length / 1000.0;\n> }\n> \n> +uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\n> +\t\t\t\t double maxFrameDuration) const\n> +{\n> +\tuint32_t frameLengthMin, frameLengthMax, vblank;\n> +\tuint32_t exposureLines = ExposureLines(exposure);\n> +\n> +\tassert(initialized_);\n> +\n> +\t/*\n> +\t * Clip frame length by the frame duration range and the maximum allowable\n\nClip? or Clamp? Doesn't really matter though ;-) both make sense.\n\n> +\t * value in the sensor, given by maxFrameLength_.\n> +\t */\n> +\tframeLengthMin = std::clamp(1e3 * minFrameDuration / mode_.line_length,\n> +\t\t\t\t\t mode_.height, maxFrameLength_);\n> +\tframeLengthMax = std::clamp(1e3 * maxFrameDuration / mode_.line_length,\n> +\t\t\t\t\t mode_.height, maxFrameLength_);\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 = Exposure(exposureLines);\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> +}\n> +\n> void CamHelper::SetCameraMode(const CameraMode &mode)\n> {\n> \tmode_ = mode;\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index 044c2866..b1739a57 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -62,12 +62,15 @@ class CamHelper\n> {\n> public:\n> \tstatic CamHelper *Create(std::string const &cam_name);\n> -\tCamHelper(MdParser *parser);\n> +\tCamHelper(MdParser *parser, unsigned int maxFrameLength,\n> +\t\t unsigned int frameIntegrationDiff);\n> \tvirtual ~CamHelper();\n> \tvoid SetCameraMode(const CameraMode &mode);\n> \tMdParser &Parser() const { return *parser_; }\n> \tuint32_t ExposureLines(double exposure_us) const;\n> \tdouble Exposure(uint32_t exposure_lines) const; // in us\n> +\tvirtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,\n> +\t\t\t\t double maxFrameDuration) const;\n> \tvirtual uint32_t GainCode(double gain) const = 0;\n> \tvirtual double Gain(uint32_t gain_code) const = 0;\n> \tvirtual void GetDelays(int &exposure_delay, int &gain_delay) const;\n> @@ -76,10 +79,20 @@ public:\n> \tvirtual unsigned int HideFramesModeSwitch() const;\n> \tvirtual unsigned int MistrustFramesStartup() const;\n> \tvirtual unsigned int MistrustFramesModeSwitch() const;\n> +\n> protected:\n> \tMdParser *parser_;\n> \tCameraMode mode_;\n> +\n> +private:\n> \tbool initialized_;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tunsigned int maxFrameLength_;\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tunsigned int frameIntegrationDiff_;\n> };\n> \n> // This is for registering camera helpers with the system, so that the\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index db8ab879..8688ec09 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -49,13 +49,22 @@ public:\n> \tdouble Gain(uint32_t gain_code) const override;\n> \tunsigned int MistrustFramesModeSwitch() const override;\n> \tbool SensorEmbeddedDataPresent() const override;\n> +\n> +private:\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tstatic constexpr int frameIntegrationDiff = 4;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tstatic constexpr int maxFrameLength = 0xffff;\n> };\n> \n> CamHelperImx219::CamHelperImx219()\n> #if ENABLE_EMBEDDED_DATA\n> -\t: CamHelper(new MdParserImx219())\n> +\t: CamHelper(new MdParserImx219(), maxFrameLength, frameIntegrationDiff)\n> #else\n> -\t: CamHelper(new MdParserRPi())\n> +\t: CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> #endif\n> {\n> }\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 0e896ac7..53961310 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -38,10 +38,19 @@ public:\n> \tuint32_t GainCode(double gain) const override;\n> \tdouble Gain(uint32_t gain_code) const override;\n> \tbool SensorEmbeddedDataPresent() const override;\n> +\n> +private:\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tstatic constexpr int frameIntegrationDiff = 22;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tstatic constexpr int maxFrameLength = 0xffdc;\n> };\n> \n> CamHelperImx477::CamHelperImx477()\n> -\t: CamHelper(new MdParserImx477())\n> +\t: CamHelper(new MdParserImx477(), maxFrameLength, frameIntegrationDiff)\n> {\n> }\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> index 0b841cd1..2bd8a754 100644\n> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> @@ -23,6 +23,15 @@ public:\n> \tunsigned int HideFramesModeSwitch() const override;\n> \tunsigned int MistrustFramesStartup() const override;\n> \tunsigned int MistrustFramesModeSwitch() const override;\n> +\n> +private:\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tstatic constexpr int frameIntegrationDiff = 4;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tstatic constexpr int maxFrameLength = 0xffff;\n> };\n> \n> /*\n> @@ -31,7 +40,7 @@ public:\n> */\n> \n> CamHelperOv5647::CamHelperOv5647()\n> -\t: CamHelper(new MdParserRPi())\n> +\t: CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> {\n> }\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index d087b07e..d309536b 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -58,6 +58,8 @@ namespace libcamera {\n> /* Configure the sensor with these values initially. */\n> constexpr double DefaultAnalogueGain = 1.0;\n> constexpr unsigned int DefaultExposureTime = 20000;\n> +constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> \n> LOG_DEFINE_CATEGORY(IPARPI)\n> \n> @@ -150,6 +152,9 @@ private:\n> \n> \t/* Distinguish the first camera start from others. */\n> \tbool firstStart_;\n> +\n> +\t/* Frame duration (1/fps) given in microseconds. */\n> +\tdouble minFrameDuration_, maxFrameDuration_;\n\nHrm, there's nothing syntactically wrong with this - but I don't think\nwe've used multiple definitions on a single line anywhere else?\n\nBut this is src/ipa/raspberrypi ... so I think you might get away with\nit :-)\n\n> };\n> \n> int IPARPi::init(const IPASettings &settings)\n> @@ -332,7 +337,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tsensorMetadata = helper_->SensorEmbeddedDataPresent();\n> \n> \t\tresult->data.push_back(gainDelay);\n> -\t\tresult->data.push_back(exposureDelay);\n> +\t\tresult->data.push_back(exposureDelay); /* For EXPOSURE ctrl */\n> +\t\tresult->data.push_back(exposureDelay); /* For VBLANK ctrl */\n> \t\tresult->data.push_back(sensorMetadata);\n> \n> \t\tresult->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> @@ -377,6 +383,9 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tcontroller_.Initialise();\n> \t\tcontrollerInit_ = true;\n> \n> +\t\tminFrameDuration_ = defaultMinFrameDuration;\n> +\t\tmaxFrameDuration_ = defaultMaxFrameDuration;\n> +\n> \t\t/* Supply initial values for gain and exposure. */\n> \t\tControlList ctrls(sensorCtrls_);\n> \t\tAgcStatus agcStatus;\n> @@ -524,7 +533,8 @@ bool IPARPi::validateSensorControls()\n> {\n> \tstatic const uint32_t ctrls[] = {\n> \t\tV4L2_CID_ANALOGUE_GAIN,\n> -\t\tV4L2_CID_EXPOSURE\n> +\t\tV4L2_CID_EXPOSURE,\n> +\t\tV4L2_CID_VBLANK\n\nShould we keep a comma on the end so additions don't require a previous\nline replacement?\n\n> \t};\n> \n> \tfor (auto c : ctrls) {\n> @@ -804,6 +814,24 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n> \n> +\t\tcase controls::FRAME_DURATIONS: {\n> +\t\t\tauto frameDurations = ctrl.second.get>();\n> +\n> +\t\t\t/* This will be applied once AGC recalculations occur. */\n> +\t\t\tminFrameDuration_ = frameDurations[0] ? frameDurations[0] : defaultMinFrameDuration;\n> +\t\t\tmaxFrameDuration_ = frameDurations[1] ? frameDurations[1] : defaultMaxFrameDuration;\n> +\n> +\t\t\t/*\n> +\t\t\t * \\todo The values returned in the metadata below must be\n> +\t\t\t * correctly clipped by what the sensor mode supports and\n> +\t\t\t * what the AGC exposure mode or manual shutter speed limits\n> +\t\t\t */\n> +\t\t\tlibcameraMetadata_.set(controls::FrameDurations,\n> +\t\t\t\t\t { static_cast(minFrameDuration_),\n> +\t\t\t\t\t\t static_cast(maxFrameDuration_) });\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tdefault:\n> \t\t\tLOG(IPARPI, Warning)\n> \t\t\t\t<< \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> @@ -962,15 +990,27 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> {\n> \tint32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> -\tint32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);\n> \n> -\tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << agcStatus->shutter_time\n> -\t\t\t << \" (Shutter lines: \" << exposureLines << \") Gain: \"\n> +\t/* GetVBlanking might clip exposure time to the fps limits. */\n> +\tdouble exposure = agcStatus->shutter_time;\n> +\tint32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,\n> +\t\t\t\t\t\t maxFrameDuration_);\n> +\tint32_t exposureLines = helper_->ExposureLines(exposure);\n> +\n> +\tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> +\t\t\t << \" (Shutter lines: \" << exposureLines << \", AGC requested \"\n> +\t\t\t << agcStatus->shutter_time << \") Gain: \"\n> \t\t\t << agcStatus->analogue_gain << \" (Gain Code: \"\n> \t\t\t << gainCode << \")\";\n> \n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\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\nThe next time? You mean this only happens on the first call through? or\nsomething else?\n\n\nNothing blocking there that I see though.\n\nReviewed-by: Kieran Bingham \n\n> +\t * clip exposure correctly.\n> +\t */\n> +\tctrls.set(V4L2_CID_VBLANK, vblanking);\n> \tctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> +\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> }\n> \n> void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 7a5f5881..252cab64 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t\tif (!staggeredCtrl_) {\n> \t\t\tstaggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> \t\t\t\t\t { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },\n> -\t\t\t\t\t { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });\n> +\t\t\t\t\t { V4L2_CID_EXPOSURE, result.data[resultIdx++] },\n> +\t\t\t\t\t { V4L2_CID_VBLANK, result.data[resultIdx++] } });\n> \t\t\tsensorMetadata_ = result.data[resultIdx++];\n> \t\t}\n> \t}\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 AE98EC0F1A\n\tfor ;\n\tFri, 18 Dec 2020 13:04:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C2BA61595;\n\tFri, 18 Dec 2020 14:04:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 11A756052C\n\tfor ;\n\tFri, 18 Dec 2020 14:04:06 +0100 (CET)","from [192.168.0.20]\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 8D7A22CF;\n\tFri, 18 Dec 2020 14:04:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LXEAf+A4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1608296645;\n\tbh=9p8Ua+ANEKzJoRAvur1f5onBmtQzZJQ9SyIhnMXkDe4=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=LXEAf+A4RJJxettEqKwlSECV/rdwkkL1T3lTCd3aix6o5fuP++66064YLF/wWMoJ5\n\tBV9wXkMQxIPlV9oM14SbBzjy6PS5AbBKZFBf5VIzwuUV7tn7kGbUVGq19UrV2f/ypm\n\t7R76HFbF5itO9vE0KNfXNSGJWfmbXkBdWjlfhsMY=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20201218100626.205134-1-naush@raspberrypi.com>\n\t<20201218100626.205134-3-naush@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<7e343907-c17f-5489-4125-99415bf3eda4@ideasonboard.com>","Date":"Fri, 18 Dec 2020 13:04:02 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201218100626.205134-3-naush@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v10 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14275,"web_url":"https://patchwork.libcamera.org/comment/14275/","msgid":"","date":"2020-12-18T13:16:05","subject":"Re: [libcamera-devel] [PATCH v10 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for the review.\n\nOn Fri, 18 Dec 2020 at 13:04, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n>\n>\n> On 18/12/2020 10:06, Naushir Patuck wrote:\n> > Add support for setting V4L2_CID_VBLANK appropriately when setting\n> > V4L2_CID_EXPOSURE. This will allow adaptive framerates during\n> > viewfinder use cases (e.g. when the exposure time goes above 33ms, we\n> > can reduce the framerate to lower than 30fps).\n> >\n> > The minimum and maximum frame durations are provided via libcamera\n> > controls, and will prioritise exposure time limits over any AGC request.\n> >\n> > V4L2_CID_VBLANK is controlled through the staggered writer, just like\n> > the exposure and gain controls.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Tested-by: David Plowman \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 1 +\n> > src/ipa/raspberrypi/cam_helper.cpp | 35 ++++++++++++-\n> > src/ipa/raspberrypi/cam_helper.hpp | 15 +++++-\n> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 ++++-\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 +++-\n> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 +++-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 52 ++++++++++++++++---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> > 8 files changed, 127 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> > index 01fe5abc..1de36039 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {\n> > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535,\n> 65535, 65535, 65535), Rectangle{}) },\n> > + { &controls::FrameDurations, ControlInfo(1000, 1 000 000 000) },\n>\n> Perhaps we should define some human readable $bignumbers to ensure we\n> don't mis-represent or mis-calculate large numbers like that.\n>\n> Or use some chrono class to be able to express the value in readable\n> terms, but store it appropriately. Of course then it gets more difficult\n> to handle that in the Control I think ...\n>\n> But anyway, not in this patch.\n>\n> > };\n> >\n> > } /* namespace RPi */\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > index 6efa0d7f..018c17b9 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const\n> &cam_name)\n> > return nullptr;\n> > }\n> >\n> > -CamHelper::CamHelper(MdParser *parser)\n> > - : parser_(parser), initialized_(false)\n> > +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > + unsigned int frameIntegrationDiff)\n> > + : parser_(parser), initialized_(false),\n> maxFrameLength_(maxFrameLength),\n> > + frameIntegrationDiff_(frameIntegrationDiff)\n> > {\n> > }\n> >\n> > @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines)\n> const\n> > return exposure_lines * mode_.line_length / 1000.0;\n> > }\n> >\n> > +uint32_t CamHelper::GetVBlanking(double &exposure, double\n> minFrameDuration,\n> > + double maxFrameDuration) const\n> > +{\n> > + uint32_t frameLengthMin, frameLengthMax, vblank;\n> > + uint32_t exposureLines = ExposureLines(exposure);\n> > +\n> > + assert(initialized_);\n> > +\n> > + /*\n> > + * Clip frame length by the frame duration range and the maximum\n> allowable\n>\n> Clip? or Clamp? Doesn't really matter though ;-) both make sense.\n>\n> > + * value in the sensor, given by maxFrameLength_.\n> > + */\n> > + frameLengthMin = std::clamp(1e3 * minFrameDuration /\n> mode_.line_length,\n> > + mode_.height,\n> maxFrameLength_);\n> > + frameLengthMax = std::clamp(1e3 * maxFrameDuration /\n> mode_.line_length,\n> > + mode_.height,\n> maxFrameLength_);\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 = Exposure(exposureLines);\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> > +}\n> > +\n> > void CamHelper::SetCameraMode(const CameraMode &mode)\n> > {\n> > mode_ = mode;\n> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> > index 044c2866..b1739a57 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -62,12 +62,15 @@ class CamHelper\n> > {\n> > public:\n> > static CamHelper *Create(std::string const &cam_name);\n> > - CamHelper(MdParser *parser);\n> > + CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > + unsigned int frameIntegrationDiff);\n> > virtual ~CamHelper();\n> > void SetCameraMode(const CameraMode &mode);\n> > MdParser &Parser() const { return *parser_; }\n> > uint32_t ExposureLines(double exposure_us) const;\n> > double Exposure(uint32_t exposure_lines) const; // in us\n> > + virtual uint32_t GetVBlanking(double &exposure_us, double\n> minFrameDuration,\n> > + double maxFrameDuration) const;\n> > virtual uint32_t GainCode(double gain) const = 0;\n> > virtual double Gain(uint32_t gain_code) const = 0;\n> > virtual void GetDelays(int &exposure_delay, int &gain_delay) const;\n> > @@ -76,10 +79,20 @@ public:\n> > virtual unsigned int HideFramesModeSwitch() const;\n> > virtual unsigned int MistrustFramesStartup() const;\n> > virtual unsigned int MistrustFramesModeSwitch() const;\n> > +\n> > protected:\n> > MdParser *parser_;\n> > CameraMode mode_;\n> > +\n> > +private:\n> > bool initialized_;\n> > + /* Largest possible frame length, in units of lines. */\n> > + unsigned int maxFrameLength_;\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + unsigned int frameIntegrationDiff_;\n> > };\n> >\n> > // This is for registering camera helpers with the system, so that the\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > index db8ab879..8688ec09 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > @@ -49,13 +49,22 @@ public:\n> > double Gain(uint32_t gain_code) const override;\n> > unsigned int MistrustFramesModeSwitch() const override;\n> > bool SensorEmbeddedDataPresent() const override;\n> > +\n> > +private:\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + static constexpr int frameIntegrationDiff = 4;\n> > + /* Largest possible frame length, in units of lines. */\n> > + static constexpr int maxFrameLength = 0xffff;\n> > };\n> >\n> > CamHelperImx219::CamHelperImx219()\n> > #if ENABLE_EMBEDDED_DATA\n> > - : CamHelper(new MdParserImx219())\n> > + : CamHelper(new MdParserImx219(), maxFrameLength,\n> frameIntegrationDiff)\n> > #else\n> > - : CamHelper(new MdParserRPi())\n> > + : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > #endif\n> > {\n> > }\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index 0e896ac7..53961310 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -38,10 +38,19 @@ public:\n> > uint32_t GainCode(double gain) const override;\n> > double Gain(uint32_t gain_code) const override;\n> > bool SensorEmbeddedDataPresent() const override;\n> > +\n> > +private:\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + static constexpr int frameIntegrationDiff = 22;\n> > + /* Largest possible frame length, in units of lines. */\n> > + static constexpr int maxFrameLength = 0xffdc;\n> > };\n> >\n> > CamHelperImx477::CamHelperImx477()\n> > - : CamHelper(new MdParserImx477())\n> > + : CamHelper(new MdParserImx477(), maxFrameLength,\n> frameIntegrationDiff)\n> > {\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > index 0b841cd1..2bd8a754 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > @@ -23,6 +23,15 @@ public:\n> > unsigned int HideFramesModeSwitch() const override;\n> > unsigned int MistrustFramesStartup() const override;\n> > unsigned int MistrustFramesModeSwitch() const override;\n> > +\n> > +private:\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + static constexpr int frameIntegrationDiff = 4;\n> > + /* Largest possible frame length, in units of lines. */\n> > + static constexpr int maxFrameLength = 0xffff;\n> > };\n> >\n> > /*\n> > @@ -31,7 +40,7 @@ public:\n> > */\n> >\n> > CamHelperOv5647::CamHelperOv5647()\n> > - : CamHelper(new MdParserRPi())\n> > + : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > {\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index d087b07e..d309536b 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -58,6 +58,8 @@ namespace libcamera {\n> > /* Configure the sensor with these values initially. */\n> > constexpr double DefaultAnalogueGain = 1.0;\n> > constexpr unsigned int DefaultExposureTime = 20000;\n> > +constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> > +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> >\n> > LOG_DEFINE_CATEGORY(IPARPI)\n> >\n> > @@ -150,6 +152,9 @@ private:\n> >\n> > /* Distinguish the first camera start from others. */\n> > bool firstStart_;\n> > +\n> > + /* Frame duration (1/fps) given in microseconds. */\n> > + double minFrameDuration_, maxFrameDuration_;\n>\n> Hrm, there's nothing syntactically wrong with this - but I don't think\n> we've used multiple definitions on a single line anywhere else?\n>\n> But this is src/ipa/raspberrypi ... so I think you might get away with\n> it :-)\n>\n> > };\n> >\n> > int IPARPi::init(const IPASettings &settings)\n> > @@ -332,7 +337,8 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > sensorMetadata = helper_->SensorEmbeddedDataPresent();\n> >\n> > result->data.push_back(gainDelay);\n> > - result->data.push_back(exposureDelay);\n> > + result->data.push_back(exposureDelay); /* For EXPOSURE\n> ctrl */\n> > + result->data.push_back(exposureDelay); /* For VBLANK ctrl\n> */\n> > result->data.push_back(sensorMetadata);\n> >\n> > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> > @@ -377,6 +383,9 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > controller_.Initialise();\n> > controllerInit_ = true;\n> >\n> > + minFrameDuration_ = defaultMinFrameDuration;\n> > + maxFrameDuration_ = defaultMaxFrameDuration;\n> > +\n> > /* Supply initial values for gain and exposure. */\n> > ControlList ctrls(sensorCtrls_);\n> > AgcStatus agcStatus;\n> > @@ -524,7 +533,8 @@ bool IPARPi::validateSensorControls()\n> > {\n> > static const uint32_t ctrls[] = {\n> > V4L2_CID_ANALOGUE_GAIN,\n> > - V4L2_CID_EXPOSURE\n> > + V4L2_CID_EXPOSURE,\n> > + V4L2_CID_VBLANK\n>\n> Should we keep a comma on the end so additions don't require a previous\n> line replacement?\n>\n> > };\n> >\n> > for (auto c : ctrls) {\n> > @@ -804,6 +814,24 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> > break;\n> > }\n> >\n> > + case controls::FRAME_DURATIONS: {\n> > + auto frameDurations = ctrl.second.get int64_t>>();\n> > +\n> > + /* This will be applied once AGC recalculations\n> occur. */\n> > + minFrameDuration_ = frameDurations[0] ?\n> frameDurations[0] : defaultMinFrameDuration;\n> > + maxFrameDuration_ = frameDurations[1] ?\n> frameDurations[1] : defaultMaxFrameDuration;\n> > +\n> > + /*\n> > + * \\todo The values returned in the metadata below\n> must be\n> > + * correctly clipped by what the sensor mode\n> supports and\n> > + * what the AGC exposure mode or manual shutter\n> speed limits\n> > + */\n> > + libcameraMetadata_.set(controls::FrameDurations,\n> > + {\n> static_cast(minFrameDuration_),\n> > +\n> static_cast(maxFrameDuration_) });\n> > + break;\n> > + }\n> > +\n> > default:\n> > LOG(IPARPI, Warning)\n> > << \"Ctrl \" << controls::controls.at\n> (ctrl.first)->name()\n> > @@ -962,15 +990,27 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\n> > {\n> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > - int32_t exposureLines =\n> helper_->ExposureLines(agcStatus->shutter_time);\n> >\n> > - LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" <<\n> agcStatus->shutter_time\n> > - << \" (Shutter lines: \" << exposureLines << \")\n> Gain: \"\n> > + /* GetVBlanking might clip exposure time to the fps limits. */\n> > + double exposure = agcStatus->shutter_time;\n> > + int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_,\n> > + maxFrameDuration_);\n> > + int32_t exposureLines = helper_->ExposureLines(exposure);\n> > +\n> > + LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> > + << \" (Shutter lines: \" << exposureLines << \",\n> AGC requested \"\n> > + << agcStatus->shutter_time << \") Gain: \"\n> > << agcStatus->analogue_gain << \" (Gain Code: \"\n> > << gainCode << \")\";\n> >\n> > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\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>\n> The next time? You mean this only happens on the first call through? or\n> something else?\n>\n\nBy \"next time\", I mean the next time you go into this function to set\nexposure, it will be validated against the previous VBLANK ctrl value which\nis a bit annoything. And the cycle continues on every subsequent call to\napplyAGC(). Does not matter so much, I do have a workaround for this in my\nnext series.\n\nRegards,\nNaush\n\n\n\n>\n>\n> Nothing blocking there that I see though.\n>\n> Reviewed-by: Kieran Bingham \n>\n> > + * clip exposure correctly.\n> > + */\n> > + ctrls.set(V4L2_CID_VBLANK, vblanking);\n> > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > }\n> >\n> > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList\n> &ctrls)\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 7a5f5881..252cab64 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > if (!staggeredCtrl_) {\n> > staggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> > { { V4L2_CID_ANALOGUE_GAIN,\n> result.data[resultIdx++] },\n> > - { V4L2_CID_EXPOSURE,\n> result.data[resultIdx++] } });\n> > + { V4L2_CID_EXPOSURE,\n> result.data[resultIdx++] },\n> > + { V4L2_CID_VBLANK,\n> result.data[resultIdx++] } });\n> > sensorMetadata_ = result.data[resultIdx++];\n> > }\n> > }\n> >\n>\n> --\n> Regards\n> --\n> Kieran\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 06C6FBD80A\n\tfor ;\n\tFri, 18 Dec 2020 13:16:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8256961595;\n\tFri, 18 Dec 2020 14:16:24 +0100 (CET)","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B8E96052C\n\tfor ;\n\tFri, 18 Dec 2020 14:16:22 +0100 (CET)","by mail-lf1-x12a.google.com with SMTP id m25so5252661lfc.11\n\tfor ;\n\tFri, 18 Dec 2020 05:16:22 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"nIOghxlq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=T6mfVEfukHQdIFizO/SXgp+XDg8CdzXx1ErULDpomLE=;\n\tb=nIOghxlq8RvCePnGj0MswEBdzrfXgdxzmegbzLuj3HJxBpssSNPDm24fShXZIEh2f5\n\t849VBvSUX/DjKllhWnclrKJ8XGlA4AS1z1TW2Rl0d0MOfytrxajy2KtiBRCk4MNFoADd\n\tjX+FKOkCybddDcxj/aL96bG8fA4jGNmAd4Q/1xjH6yNInQvvxO6ajDZOqflsbzsGLxQO\n\tpMpQDSHQUTdBaAmrdWhR+o31m74FZvHXcJv7pHWoz30xteETgw6vvn/ol2PDkU8ICcJY\n\tGcM3rBqfx6OoVggAF/Z4H++Q2eW3kDp8J81VKy7nPKjO95tzoimcva5Zl9zNMZ2TGTg4\n\t3WXA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=T6mfVEfukHQdIFizO/SXgp+XDg8CdzXx1ErULDpomLE=;\n\tb=uGH3rD3FmwZXKV7Pxs87W2xEvwffoN3UAHqGhcUGQXZ8Mu0mBn8yZLm2HeTQN2BNL6\n\twLd1WeHX1HOI0qH+BtUpA1d0FlN7GcSTCabI2EFOCeg6yzPAx7wDWkgmtJYRstCzZ20r\n\t3C3Xq2m7KoF67pPBjsxCrlNnJMWx0Xr8Vs5JVBkviGFx89VEN1ypLu9FAPcahbTP6h8c\n\tF0wstPQUVqsTvoTJRQjZB7OAVR18MQybBmaGSywxhJ6B4z1JHI0uaA03nSMGMU7LfKY2\n\t3KZ4pms2GttXHyVpZnt8N2G7gPgsbAroWubAw5+W5mO/DKVALJ3tAuVIu/NZdzOTwqCd\n\tSoTw==","X-Gm-Message-State":"AOAM531LKRQWdYybhSwWZCrki+3l7yAS4SpiyEUQ2I3/4E0bR2S6+jIu\n\ttCNDmXT68fiK7BAAQxz0VsZyi37MwHQfJxJ13Siw9A==","X-Google-Smtp-Source":"ABdhPJyDrPjiR7sb0aGHa9HHtNmoGetHv9GdT+FdXXvBXmXye3PFXhJtV6v5T9pvnxjwYLGGNX9dwIk6eR3AZERqrZs=","X-Received":"by 2002:a19:8357:: with SMTP id\n\tf84mr1486069lfd.567.1608297381595; \n\tFri, 18 Dec 2020 05:16:21 -0800 (PST)","MIME-Version":"1.0","References":"<20201218100626.205134-1-naush@raspberrypi.com>\n\t<20201218100626.205134-3-naush@raspberrypi.com>\n\t<7e343907-c17f-5489-4125-99415bf3eda4@ideasonboard.com>","In-Reply-To":"<7e343907-c17f-5489-4125-99415bf3eda4@ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 18 Dec 2020 13:16:05 +0000","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v10 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============4113502266266252686==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14291,"web_url":"https://patchwork.libcamera.org/comment/14291/","msgid":"","date":"2020-12-20T05:52:13","subject":"Re: [libcamera-devel] [PATCH v10 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","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 Fri, Dec 18, 2020 at 10:06:26AM +0000, Naushir Patuck wrote:\n> Add support for setting V4L2_CID_VBLANK appropriately when setting\n> V4L2_CID_EXPOSURE. This will allow adaptive framerates during\n> viewfinder use cases (e.g. when the exposure time goes above 33ms, we\n> can reduce the framerate to lower than 30fps).\n> \n> The minimum and maximum frame durations are provided via libcamera\n> controls, and will prioritise exposure time limits over any AGC request.\n> \n> V4L2_CID_VBLANK is controlled through the staggered writer, just like\n> the exposure and gain controls.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Tested-by: David Plowman \n> Reviewed-by: Jacopo Mondi \n> ---\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/raspberrypi/cam_helper.cpp | 35 ++++++++++++-\n> src/ipa/raspberrypi/cam_helper.hpp | 15 +++++-\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 ++++-\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 +++-\n> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 +++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 52 ++++++++++++++++---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> 8 files changed, 127 insertions(+), 14 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 01fe5abc..1de36039 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {\n> \t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> \t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> \t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> +\t{ &controls::FrameDurations, ControlInfo(1000, 1000000000) },\n> };\n> \n> } /* namespace RPi */\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 6efa0d7f..018c17b9 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const &cam_name)\n> \treturn nullptr;\n> }\n> \n> -CamHelper::CamHelper(MdParser *parser)\n> -\t: parser_(parser), initialized_(false)\n> +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> +\t\t unsigned int frameIntegrationDiff)\n> +\t: parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),\n> +\t frameIntegrationDiff_(frameIntegrationDiff)\n> {\n> }\n> \n> @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines) const\n> \treturn exposure_lines * mode_.line_length / 1000.0;\n> }\n> \n> +uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\n> +\t\t\t\t double maxFrameDuration) const\n> +{\n> +\tuint32_t frameLengthMin, frameLengthMax, vblank;\n> +\tuint32_t exposureLines = ExposureLines(exposure);\n> +\n> +\tassert(initialized_);\n> +\n> +\t/*\n> +\t * Clip frame length by the frame duration range and the maximum allowable\n> +\t * value in the sensor, given by maxFrameLength_.\n> +\t */\n> +\tframeLengthMin = std::clamp(1e3 * minFrameDuration / mode_.line_length,\n> +\t\t\t\t\t mode_.height, maxFrameLength_);\n> +\tframeLengthMax = std::clamp(1e3 * maxFrameDuration / mode_.line_length,\n> +\t\t\t\t\t mode_.height, maxFrameLength_);\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 = Exposure(exposureLines);\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> +}\n> +\n> void CamHelper::SetCameraMode(const CameraMode &mode)\n> {\n> \tmode_ = mode;\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index 044c2866..b1739a57 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -62,12 +62,15 @@ class CamHelper\n> {\n> public:\n> \tstatic CamHelper *Create(std::string const &cam_name);\n> -\tCamHelper(MdParser *parser);\n> +\tCamHelper(MdParser *parser, unsigned int maxFrameLength,\n> +\t\t unsigned int frameIntegrationDiff);\n> \tvirtual ~CamHelper();\n> \tvoid SetCameraMode(const CameraMode &mode);\n> \tMdParser &Parser() const { return *parser_; }\n> \tuint32_t ExposureLines(double exposure_us) const;\n> \tdouble Exposure(uint32_t exposure_lines) const; // in us\n> +\tvirtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,\n> +\t\t\t\t double maxFrameDuration) const;\n> \tvirtual uint32_t GainCode(double gain) const = 0;\n> \tvirtual double Gain(uint32_t gain_code) const = 0;\n> \tvirtual void GetDelays(int &exposure_delay, int &gain_delay) const;\n> @@ -76,10 +79,20 @@ public:\n> \tvirtual unsigned int HideFramesModeSwitch() const;\n> \tvirtual unsigned int MistrustFramesStartup() const;\n> \tvirtual unsigned int MistrustFramesModeSwitch() const;\n> +\n> protected:\n> \tMdParser *parser_;\n> \tCameraMode mode_;\n> +\n> +private:\n> \tbool initialized_;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tunsigned int maxFrameLength_;\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tunsigned int frameIntegrationDiff_;\n> };\n> \n> // This is for registering camera helpers with the system, so that the\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index db8ab879..8688ec09 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -49,13 +49,22 @@ public:\n> \tdouble Gain(uint32_t gain_code) const override;\n> \tunsigned int MistrustFramesModeSwitch() const override;\n> \tbool SensorEmbeddedDataPresent() const override;\n> +\n> +private:\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tstatic constexpr int frameIntegrationDiff = 4;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tstatic constexpr int maxFrameLength = 0xffff;\n> };\n> \n> CamHelperImx219::CamHelperImx219()\n> #if ENABLE_EMBEDDED_DATA\n> -\t: CamHelper(new MdParserImx219())\n> +\t: CamHelper(new MdParserImx219(), maxFrameLength, frameIntegrationDiff)\n> #else\n> -\t: CamHelper(new MdParserRPi())\n> +\t: CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> #endif\n> {\n> }\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 0e896ac7..53961310 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -38,10 +38,19 @@ public:\n> \tuint32_t GainCode(double gain) const override;\n> \tdouble Gain(uint32_t gain_code) const override;\n> \tbool SensorEmbeddedDataPresent() const override;\n> +\n> +private:\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tstatic constexpr int frameIntegrationDiff = 22;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tstatic constexpr int maxFrameLength = 0xffdc;\n> };\n> \n> CamHelperImx477::CamHelperImx477()\n> -\t: CamHelper(new MdParserImx477())\n> +\t: CamHelper(new MdParserImx477(), maxFrameLength, frameIntegrationDiff)\n> {\n> }\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> index 0b841cd1..2bd8a754 100644\n> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> @@ -23,6 +23,15 @@ public:\n> \tunsigned int HideFramesModeSwitch() const override;\n> \tunsigned int MistrustFramesStartup() const override;\n> \tunsigned int MistrustFramesModeSwitch() const override;\n> +\n> +private:\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tstatic constexpr int frameIntegrationDiff = 4;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tstatic constexpr int maxFrameLength = 0xffff;\n> };\n> \n> /*\n> @@ -31,7 +40,7 @@ public:\n> */\n> \n> CamHelperOv5647::CamHelperOv5647()\n> -\t: CamHelper(new MdParserRPi())\n> +\t: CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> {\n> }\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index d087b07e..d309536b 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -58,6 +58,8 @@ namespace libcamera {\n> /* Configure the sensor with these values initially. */\n> constexpr double DefaultAnalogueGain = 1.0;\n> constexpr unsigned int DefaultExposureTime = 20000;\n> +constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> \n> LOG_DEFINE_CATEGORY(IPARPI)\n> \n> @@ -150,6 +152,9 @@ private:\n> \n> \t/* Distinguish the first camera start from others. */\n> \tbool firstStart_;\n> +\n> +\t/* Frame duration (1/fps) given in microseconds. */\n> +\tdouble minFrameDuration_, maxFrameDuration_;\n> };\n> \n> int IPARPi::init(const IPASettings &settings)\n> @@ -332,7 +337,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tsensorMetadata = helper_->SensorEmbeddedDataPresent();\n> \n> \t\tresult->data.push_back(gainDelay);\n> -\t\tresult->data.push_back(exposureDelay);\n> +\t\tresult->data.push_back(exposureDelay); /* For EXPOSURE ctrl */\n> +\t\tresult->data.push_back(exposureDelay); /* For VBLANK ctrl */\n> \t\tresult->data.push_back(sensorMetadata);\n> \n> \t\tresult->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> @@ -377,6 +383,9 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tcontroller_.Initialise();\n> \t\tcontrollerInit_ = true;\n> \n> +\t\tminFrameDuration_ = defaultMinFrameDuration;\n> +\t\tmaxFrameDuration_ = defaultMaxFrameDuration;\n> +\n> \t\t/* Supply initial values for gain and exposure. */\n> \t\tControlList ctrls(sensorCtrls_);\n> \t\tAgcStatus agcStatus;\n> @@ -524,7 +533,8 @@ bool IPARPi::validateSensorControls()\n> {\n> \tstatic const uint32_t ctrls[] = {\n> \t\tV4L2_CID_ANALOGUE_GAIN,\n> -\t\tV4L2_CID_EXPOSURE\n> +\t\tV4L2_CID_EXPOSURE,\n> +\t\tV4L2_CID_VBLANK\n> \t};\n> \n> \tfor (auto c : ctrls) {\n> @@ -804,6 +814,24 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n> \n> +\t\tcase controls::FRAME_DURATIONS: {\n> +\t\t\tauto frameDurations = ctrl.second.get>();\n> +\n> +\t\t\t/* This will be applied once AGC recalculations occur. */\n> +\t\t\tminFrameDuration_ = frameDurations[0] ? frameDurations[0] : defaultMinFrameDuration;\n> +\t\t\tmaxFrameDuration_ = frameDurations[1] ? frameDurations[1] : defaultMaxFrameDuration;\n\nShould it be documented in patch 1/3 that setting the min or max to 0\nwill reset the limit to the default ?\n\nWill the code behave correctly if min > max ?\n\n> +\n> +\t\t\t/*\n> +\t\t\t * \\todo The values returned in the metadata below must be\n> +\t\t\t * correctly clipped by what the sensor mode supports and\n> +\t\t\t * what the AGC exposure mode or manual shutter speed limits\n> +\t\t\t */\n> +\t\t\tlibcameraMetadata_.set(controls::FrameDurations,\n> +\t\t\t\t\t { static_cast(minFrameDuration_),\n> +\t\t\t\t\t\t static_cast(maxFrameDuration_) });\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tdefault:\n> \t\t\tLOG(IPARPI, Warning)\n> \t\t\t\t<< \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> @@ -962,15 +990,27 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> {\n> \tint32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> -\tint32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);\n> \n> -\tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << agcStatus->shutter_time\n> -\t\t\t << \" (Shutter lines: \" << exposureLines << \") Gain: \"\n> +\t/* GetVBlanking might clip exposure time to the fps limits. */\n> +\tdouble exposure = agcStatus->shutter_time;\n> +\tint32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,\n> +\t\t\t\t\t\t maxFrameDuration_);\n> +\tint32_t exposureLines = helper_->ExposureLines(exposure);\n> +\n> +\tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> +\t\t\t << \" (Shutter lines: \" << exposureLines << \", AGC requested \"\n> +\t\t\t << agcStatus->shutter_time << \") Gain: \"\n> \t\t\t << agcStatus->analogue_gain << \" (Gain Code: \"\n> \t\t\t << gainCode << \")\";\n> \n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\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\nThis is something we should address in V4L2 (obviously not a blocker for\nthis series). If you could design it from scratch, what would be a good\nAPI to configure the h/v blanking (possibly expressed as h/v active\ninstead of blanking) and exposure time ?\n\n> +\tctrls.set(V4L2_CID_VBLANK, vblanking);\n> \tctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> +\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> }\n> \n> void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 7a5f5881..252cab64 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t\tif (!staggeredCtrl_) {\n> \t\t\tstaggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> \t\t\t\t\t { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },\n> -\t\t\t\t\t { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });\n> +\t\t\t\t\t { V4L2_CID_EXPOSURE, result.data[resultIdx++] },\n> +\t\t\t\t\t { V4L2_CID_VBLANK, result.data[resultIdx++] } });\n> \t\t\tsensorMetadata_ = result.data[resultIdx++];\n> \t\t}\n> \t}","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 B5C51C0F1A\n\tfor ;\n\tSun, 20 Dec 2020 05:52:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 353A061595;\n\tSun, 20 Dec 2020 06:52:24 +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 D87B460526\n\tfor ;\n\tSun, 20 Dec 2020 06:52:22 +0100 (CET)","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 457EC593;\n\tSun, 20 Dec 2020 06:52:22 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kgJjCGJi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1608443542;\n\tbh=wpJA+AvKXuEke6uUiAtc8QZXnIUZ4GsfXC/u27cIiNI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kgJjCGJiuFZZmdLVIMzbNO50TJpGA+GHkZdxfCJJ3PXxZrHWvgVYIw0dkfh//q4CR\n\txL+gCcMfa2ReDeq0H87/JwBUeJGVa8nZbfydx7RBYB/bEclcFSSN7kfJBRvRpKDhVZ\n\t8qFgBgjZmT1S9iMHqzs9qfyalbLPU6YYoKz5dHFU=","Date":"Sun, 20 Dec 2020 07:52:13 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20201218100626.205134-1-naush@raspberrypi.com>\n\t<20201218100626.205134-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201218100626.205134-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v10 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14295,"web_url":"https://patchwork.libcamera.org/comment/14295/","msgid":"","date":"2020-12-21T11:26:58","subject":"Re: [libcamera-devel] [PATCH v10 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","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 review feedback.\n\nOn Sun, 20 Dec 2020 at 05:52, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Fri, Dec 18, 2020 at 10:06:26AM +0000, Naushir Patuck wrote:\n> > Add support for setting V4L2_CID_VBLANK appropriately when setting\n> > V4L2_CID_EXPOSURE. This will allow adaptive framerates during\n> > viewfinder use cases (e.g. when the exposure time goes above 33ms, we\n> > can reduce the framerate to lower than 30fps).\n> >\n> > The minimum and maximum frame durations are provided via libcamera\n> > controls, and will prioritise exposure time limits over any AGC request.\n> >\n> > V4L2_CID_VBLANK is controlled through the staggered writer, just like\n> > the exposure and gain controls.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Tested-by: David Plowman \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 1 +\n> > src/ipa/raspberrypi/cam_helper.cpp | 35 ++++++++++++-\n> > src/ipa/raspberrypi/cam_helper.hpp | 15 +++++-\n> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 ++++-\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 +++-\n> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 +++-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 52 ++++++++++++++++---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> > 8 files changed, 127 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> > index 01fe5abc..1de36039 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {\n> > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535,\n> 65535, 65535, 65535), Rectangle{}) },\n> > + { &controls::FrameDurations, ControlInfo(1000, 1000000000) },\n> > };\n> >\n> > } /* namespace RPi */\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > index 6efa0d7f..018c17b9 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const\n> &cam_name)\n> > return nullptr;\n> > }\n> >\n> > -CamHelper::CamHelper(MdParser *parser)\n> > - : parser_(parser), initialized_(false)\n> > +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > + unsigned int frameIntegrationDiff)\n> > + : parser_(parser), initialized_(false),\n> maxFrameLength_(maxFrameLength),\n> > + frameIntegrationDiff_(frameIntegrationDiff)\n> > {\n> > }\n> >\n> > @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines)\n> const\n> > return exposure_lines * mode_.line_length / 1000.0;\n> > }\n> >\n> > +uint32_t CamHelper::GetVBlanking(double &exposure, double\n> minFrameDuration,\n> > + double maxFrameDuration) const\n> > +{\n> > + uint32_t frameLengthMin, frameLengthMax, vblank;\n> > + uint32_t exposureLines = ExposureLines(exposure);\n> > +\n> > + assert(initialized_);\n> > +\n> > + /*\n> > + * Clip frame length by the frame duration range and the maximum\n> allowable\n> > + * value in the sensor, given by maxFrameLength_.\n> > + */\n> > + frameLengthMin = std::clamp(1e3 * minFrameDuration /\n> mode_.line_length,\n> > + mode_.height,\n> maxFrameLength_);\n> > + frameLengthMax = std::clamp(1e3 * maxFrameDuration /\n> mode_.line_length,\n> > + mode_.height,\n> maxFrameLength_);\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 = Exposure(exposureLines);\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> > +}\n> > +\n> > void CamHelper::SetCameraMode(const CameraMode &mode)\n> > {\n> > mode_ = mode;\n> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> > index 044c2866..b1739a57 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -62,12 +62,15 @@ class CamHelper\n> > {\n> > public:\n> > static CamHelper *Create(std::string const &cam_name);\n> > - CamHelper(MdParser *parser);\n> > + CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > + unsigned int frameIntegrationDiff);\n> > virtual ~CamHelper();\n> > void SetCameraMode(const CameraMode &mode);\n> > MdParser &Parser() const { return *parser_; }\n> > uint32_t ExposureLines(double exposure_us) const;\n> > double Exposure(uint32_t exposure_lines) const; // in us\n> > + virtual uint32_t GetVBlanking(double &exposure_us, double\n> minFrameDuration,\n> > + double maxFrameDuration) const;\n> > virtual uint32_t GainCode(double gain) const = 0;\n> > virtual double Gain(uint32_t gain_code) const = 0;\n> > virtual void GetDelays(int &exposure_delay, int &gain_delay) const;\n> > @@ -76,10 +79,20 @@ public:\n> > virtual unsigned int HideFramesModeSwitch() const;\n> > virtual unsigned int MistrustFramesStartup() const;\n> > virtual unsigned int MistrustFramesModeSwitch() const;\n> > +\n> > protected:\n> > MdParser *parser_;\n> > CameraMode mode_;\n> > +\n> > +private:\n> > bool initialized_;\n> > + /* Largest possible frame length, in units of lines. */\n> > + unsigned int maxFrameLength_;\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + unsigned int frameIntegrationDiff_;\n> > };\n> >\n> > // This is for registering camera helpers with the system, so that the\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > index db8ab879..8688ec09 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > @@ -49,13 +49,22 @@ public:\n> > double Gain(uint32_t gain_code) const override;\n> > unsigned int MistrustFramesModeSwitch() const override;\n> > bool SensorEmbeddedDataPresent() const override;\n> > +\n> > +private:\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + static constexpr int frameIntegrationDiff = 4;\n> > + /* Largest possible frame length, in units of lines. */\n> > + static constexpr int maxFrameLength = 0xffff;\n> > };\n> >\n> > CamHelperImx219::CamHelperImx219()\n> > #if ENABLE_EMBEDDED_DATA\n> > - : CamHelper(new MdParserImx219())\n> > + : CamHelper(new MdParserImx219(), maxFrameLength,\n> frameIntegrationDiff)\n> > #else\n> > - : CamHelper(new MdParserRPi())\n> > + : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > #endif\n> > {\n> > }\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index 0e896ac7..53961310 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -38,10 +38,19 @@ public:\n> > uint32_t GainCode(double gain) const override;\n> > double Gain(uint32_t gain_code) const override;\n> > bool SensorEmbeddedDataPresent() const override;\n> > +\n> > +private:\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + static constexpr int frameIntegrationDiff = 22;\n> > + /* Largest possible frame length, in units of lines. */\n> > + static constexpr int maxFrameLength = 0xffdc;\n> > };\n> >\n> > CamHelperImx477::CamHelperImx477()\n> > - : CamHelper(new MdParserImx477())\n> > + : CamHelper(new MdParserImx477(), maxFrameLength,\n> frameIntegrationDiff)\n> > {\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > index 0b841cd1..2bd8a754 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > @@ -23,6 +23,15 @@ public:\n> > unsigned int HideFramesModeSwitch() const override;\n> > unsigned int MistrustFramesStartup() const override;\n> > unsigned int MistrustFramesModeSwitch() const override;\n> > +\n> > +private:\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + static constexpr int frameIntegrationDiff = 4;\n> > + /* Largest possible frame length, in units of lines. */\n> > + static constexpr int maxFrameLength = 0xffff;\n> > };\n> >\n> > /*\n> > @@ -31,7 +40,7 @@ public:\n> > */\n> >\n> > CamHelperOv5647::CamHelperOv5647()\n> > - : CamHelper(new MdParserRPi())\n> > + : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > {\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index d087b07e..d309536b 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -58,6 +58,8 @@ namespace libcamera {\n> > /* Configure the sensor with these values initially. */\n> > constexpr double DefaultAnalogueGain = 1.0;\n> > constexpr unsigned int DefaultExposureTime = 20000;\n> > +constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> > +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> >\n> > LOG_DEFINE_CATEGORY(IPARPI)\n> >\n> > @@ -150,6 +152,9 @@ private:\n> >\n> > /* Distinguish the first camera start from others. */\n> > bool firstStart_;\n> > +\n> > + /* Frame duration (1/fps) given in microseconds. */\n> > + double minFrameDuration_, maxFrameDuration_;\n> > };\n> >\n> > int IPARPi::init(const IPASettings &settings)\n> > @@ -332,7 +337,8 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > sensorMetadata = helper_->SensorEmbeddedDataPresent();\n> >\n> > result->data.push_back(gainDelay);\n> > - result->data.push_back(exposureDelay);\n> > + result->data.push_back(exposureDelay); /* For EXPOSURE\n> ctrl */\n> > + result->data.push_back(exposureDelay); /* For VBLANK ctrl\n> */\n> > result->data.push_back(sensorMetadata);\n> >\n> > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> > @@ -377,6 +383,9 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > controller_.Initialise();\n> > controllerInit_ = true;\n> >\n> > + minFrameDuration_ = defaultMinFrameDuration;\n> > + maxFrameDuration_ = defaultMaxFrameDuration;\n> > +\n> > /* Supply initial values for gain and exposure. */\n> > ControlList ctrls(sensorCtrls_);\n> > AgcStatus agcStatus;\n> > @@ -524,7 +533,8 @@ bool IPARPi::validateSensorControls()\n> > {\n> > static const uint32_t ctrls[] = {\n> > V4L2_CID_ANALOGUE_GAIN,\n> > - V4L2_CID_EXPOSURE\n> > + V4L2_CID_EXPOSURE,\n> > + V4L2_CID_VBLANK\n> > };\n> >\n> > for (auto c : ctrls) {\n> > @@ -804,6 +814,24 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> > break;\n> > }\n> >\n> > + case controls::FRAME_DURATIONS: {\n> > + auto frameDurations = ctrl.second.get int64_t>>();\n> > +\n> > + /* This will be applied once AGC recalculations\n> occur. */\n> > + minFrameDuration_ = frameDurations[0] ?\n> frameDurations[0] : defaultMinFrameDuration;\n> > + maxFrameDuration_ = frameDurations[1] ?\n> frameDurations[1] : defaultMaxFrameDuration;\n>\n> Should it be documented in patch 1/3 that setting the min or max to 0\n> will reset the limit to the default ?\n>\n\nThere was a mention of this in the control documentation at an earlier\nrevision. Jacopo suggested we remove it for now, as it was a bit ambiguous\nas to what a \"default\" frame duration is. It could be a sensor provided\ndefault, or as is the case with Raspberry Pi, an IPA provided default,\nor... I was a bit torn as to whether we needed a way to set defaults at\nall. What do you think?\n\n\n>\n> Will the code behave correctly if min > max ?\n>\n\nNo, bad things will happen :) I will guard against this by ensuring max >=\nmin.\n\n\n>\n> > +\n> > + /*\n> > + * \\todo The values returned in the metadata below\n> must be\n> > + * correctly clipped by what the sensor mode\n> supports and\n> > + * what the AGC exposure mode or manual shutter\n> speed limits\n> > + */\n> > + libcameraMetadata_.set(controls::FrameDurations,\n> > + {\n> static_cast(minFrameDuration_),\n> > +\n> static_cast(maxFrameDuration_) });\n> > + break;\n> > + }\n> > +\n> > default:\n> > LOG(IPARPI, Warning)\n> > << \"Ctrl \" << controls::controls.at\n> (ctrl.first)->name()\n> > @@ -962,15 +990,27 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\n> > {\n> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > - int32_t exposureLines =\n> helper_->ExposureLines(agcStatus->shutter_time);\n> >\n> > - LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" <<\n> agcStatus->shutter_time\n> > - << \" (Shutter lines: \" << exposureLines << \")\n> Gain: \"\n> > + /* GetVBlanking might clip exposure time to the fps limits. */\n> > + double exposure = agcStatus->shutter_time;\n> > + int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_,\n> > + maxFrameDuration_);\n> > + int32_t exposureLines = helper_->ExposureLines(exposure);\n> > +\n> > + LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> > + << \" (Shutter lines: \" << exposureLines << \",\n> AGC requested \"\n> > + << agcStatus->shutter_time << \") Gain: \"\n> > << agcStatus->analogue_gain << \" (Gain Code: \"\n> > << gainCode << \")\";\n> >\n> > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\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>\n> This is something we should address in V4L2 (obviously not a blocker for\n> this series). If you could design it from scratch, what would be a good\n> API to configure the h/v blanking (possibly expressed as h/v active\n> instead of blanking) and exposure time ?\n>\n\nThis is purely personal preference of course, but I would love it if the\nworld worked like this:\n\n1) Userland sets a minimum and maximum frame rate (or frame duration) on a\nsensor device driver. This can happen at the start of day, or during\nruntime.\n2) Userland then provides an exposure time (in us or ns rather than in\nlines) to the sensor device driver during normal AE operation.\n3) Based on the frame rate/duration limits the sensor will\nautomatically adjust the V and H blanking to fulfil the exposure time if it\ncan.\n\nFor normal sensors, H/V blanking operations have very similar meaning, but\nfor more advanced modes (e.g. ultra long exposures or global shutter modes\nof operation) they lose that meaning. I feel trying to encode all of that\nin a kernel API might be hard as it's a moving target. This is why I think\nthat the blanking is probably left to the driver to deal with in a device\nspecific way. With useland in control of setting framerate/duration\nlimits, I see no reason why it would have any need to set blanking limits,\nbut of course, I could be missing use cases that I have never had to deal\nwith!\n\nRegards,\nNaush\n\n\n\n>\n> > + ctrls.set(V4L2_CID_VBLANK, vblanking);\n> > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > }\n> >\n> > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList\n> &ctrls)\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 7a5f5881..252cab64 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > if (!staggeredCtrl_) {\n> > staggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> > { { V4L2_CID_ANALOGUE_GAIN,\n> result.data[resultIdx++] },\n> > - { V4L2_CID_EXPOSURE,\n> result.data[resultIdx++] } });\n> > + { V4L2_CID_EXPOSURE,\n> result.data[resultIdx++] },\n> > + { V4L2_CID_VBLANK,\n> result.data[resultIdx++] } });\n> > sensorMetadata_ = result.data[resultIdx++];\n> > }\n> > }\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 36D87C0F1B\n\tfor ;\n\tMon, 21 Dec 2020 11:27:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AABBD6159A;\n\tMon, 21 Dec 2020 12:27:17 +0100 (CET)","from mail-lf1-x131.google.com (mail-lf1-x131.google.com\n\t[IPv6:2a00:1450:4864:20::131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F364961592\n\tfor ;\n\tMon, 21 Dec 2020 12:27:15 +0100 (CET)","by mail-lf1-x131.google.com with SMTP id l11so22890841lfg.0\n\tfor ;\n\tMon, 21 Dec 2020 03:27:15 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"X81TAC7R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=4tsI04ecQk6ory8LSb9P1Q19fwkdvUQe1XWjLl6QRUU=;\n\tb=X81TAC7RQouPtMJ6KjEYIUb9OBb5jpwAehZSZml3oGH2BDXVqo1xJaLSNmX2xNkw+t\n\t0RG4eb/IZWjIoe4ap4ClUhtdD83U4QIiD7PDPs01zsQYOY1Aoi2wCyvkc7fWql+Gigcy\n\tE7fll1CDWfnWcw9QBFIgZzWLCuhvBrnJuiaPuVAZwBHhRS0TlrLInC5cHT/fU0LvI6lO\n\thN95zaka7LmsZdRCs3+GwixNIldIKZLFa7Fl6U9JOJEcT6rCNnyE+TG2CJhp7o3aPGfy\n\tkEojFPkrpr0D0kRay7v90ldXXSN9g4Ga4f7fPv229PztyXE9VruRx19FKE5Ht4O8lkia\n\t5F8Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=4tsI04ecQk6ory8LSb9P1Q19fwkdvUQe1XWjLl6QRUU=;\n\tb=K9BawJAEv1R1pigGMU/QYo+55rpxAiiDfjCGaOO52zIS+EwuuWSW/cBF/HsHPrwsH/\n\ti6b3uRTJOyjsJUTFrAUA2347yNGHaQR6NTDH6nizUpdb+HregikHfjzBstXB/jte8kHr\n\tA3uETKzB5VAX8UhqKwGA/WYhIe5+huQo37TM1GaQPQExHiZN526Rdijs22ydln725yyf\n\tzU3EHQkZEU8KOWop3wyhN/0kuJeSES86bt0bAdemfubnLAipC/SPyS0xtcp/uGWtSTuH\n\tTAMPOgWWaI8S0MrInFUPyH3G+u7XSDqEgyYrA0pl+NQjlaK2En/tPbsR4rWMrrwk7F0K\n\tFVHg==","X-Gm-Message-State":"AOAM53347YC6ed6TcrtDGTaZfqMGyINS/nzR+jBkirna08+AaF8woVlq\n\tZqFQyF1lc1Xv1rS/boteZtaN452ZO6qljtxsZv+DXw==","X-Google-Smtp-Source":"ABdhPJxL444nKcHj1SQNIEV5mHcOC2zGAzCmwjTjkrqZuKHCnWeskAQ7QNH9dP8IiSHU1FpkZ8FHkcxIV4J5rsFdvAo=","X-Received":"by 2002:a19:950:: with SMTP id 77mr6301960lfj.133.1608550035291; \n\tMon, 21 Dec 2020 03:27:15 -0800 (PST)","MIME-Version":"1.0","References":"<20201218100626.205134-1-naush@raspberrypi.com>\n\t<20201218100626.205134-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Mon, 21 Dec 2020 11:26:58 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v10 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============7371763277096388307==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14449,"web_url":"https://patchwork.libcamera.org/comment/14449/","msgid":"","date":"2021-01-05T13:09:57","subject":"Re: [libcamera-devel] [PATCH v10 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Mon, Dec 21, 2020 at 11:26:58AM +0000, Naushir Patuck wrote:\n> On Sun, 20 Dec 2020 at 05:52, Laurent Pinchart wrote:\n> > On Fri, Dec 18, 2020 at 10:06:26AM +0000, Naushir Patuck wrote:\n> > > Add support for setting V4L2_CID_VBLANK appropriately when setting\n> > > V4L2_CID_EXPOSURE. This will allow adaptive framerates during\n> > > viewfinder use cases (e.g. when the exposure time goes above 33ms, we\n> > > can reduce the framerate to lower than 30fps).\n> > >\n> > > The minimum and maximum frame durations are provided via libcamera\n> > > controls, and will prioritise exposure time limits over any AGC request.\n> > >\n> > > V4L2_CID_VBLANK is controlled through the staggered writer, just like\n> > > the exposure and gain controls.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > Tested-by: David Plowman \n> > > Reviewed-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > src/ipa/raspberrypi/cam_helper.cpp | 35 ++++++++++++-\n> > > src/ipa/raspberrypi/cam_helper.hpp | 15 +++++-\n> > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 ++++-\n> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 +++-\n> > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 +++-\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 52 ++++++++++++++++---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> > > 8 files changed, 127 insertions(+), 14 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > index 01fe5abc..1de36039 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {\n> > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > > + { &controls::FrameDurations, ControlInfo(1000, 1000000000) },\n> > > };\n> > >\n> > > } /* namespace RPi */\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index 6efa0d7f..018c17b9 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const &cam_name)\n> > > return nullptr;\n> > > }\n> > >\n> > > -CamHelper::CamHelper(MdParser *parser)\n> > > - : parser_(parser), initialized_(false)\n> > > +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > > + unsigned int frameIntegrationDiff)\n> > > + : parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),\n> > > + frameIntegrationDiff_(frameIntegrationDiff)\n> > > {\n> > > }\n> > >\n> > > @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines) const\n> > > return exposure_lines * mode_.line_length / 1000.0;\n> > > }\n> > >\n> > > +uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\n> > > + double maxFrameDuration) const\n> > > +{\n> > > + uint32_t frameLengthMin, frameLengthMax, vblank;\n> > > + uint32_t exposureLines = ExposureLines(exposure);\n> > > +\n> > > + assert(initialized_);\n> > > +\n> > > + /*\n> > > + * Clip frame length by the frame duration range and the maximum allowable\n> > > + * value in the sensor, given by maxFrameLength_.\n> > > + */\n> > > + frameLengthMin = std::clamp(1e3 * minFrameDuration / mode_.line_length,\n> > > + mode_.height, maxFrameLength_);\n> > > + frameLengthMax = std::clamp(1e3 * maxFrameDuration / mode_.line_length,\n> > > + mode_.height, maxFrameLength_);\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_, exposureLines);\n> > > + exposure = Exposure(exposureLines);\n> > > +\n> > > + /* Limit the vblank to the range allowed by the frame length limits. */\n> > > + vblank = std::clamp(exposureLines + frameIntegrationDiff_,\n> > > + frameLengthMin, frameLengthMax) - mode_.height;\n> > > + return vblank;\n> > > +}\n> > > +\n> > > void CamHelper::SetCameraMode(const CameraMode &mode)\n> > > {\n> > > mode_ = mode;\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> > > index 044c2866..b1739a57 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > @@ -62,12 +62,15 @@ class CamHelper\n> > > {\n> > > public:\n> > > static CamHelper *Create(std::string const &cam_name);\n> > > - CamHelper(MdParser *parser);\n> > > + CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > > + unsigned int frameIntegrationDiff);\n> > > virtual ~CamHelper();\n> > > void SetCameraMode(const CameraMode &mode);\n> > > MdParser &Parser() const { return *parser_; }\n> > > uint32_t ExposureLines(double exposure_us) const;\n> > > double Exposure(uint32_t exposure_lines) const; // in us\n> > > + virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,\n> > > + double maxFrameDuration) const;\n> > > virtual uint32_t GainCode(double gain) const = 0;\n> > > virtual double Gain(uint32_t gain_code) const = 0;\n> > > virtual void GetDelays(int &exposure_delay, int &gain_delay) const;\n> > > @@ -76,10 +79,20 @@ public:\n> > > virtual unsigned int HideFramesModeSwitch() const;\n> > > virtual unsigned int MistrustFramesStartup() const;\n> > > virtual unsigned int MistrustFramesModeSwitch() const;\n> > > +\n> > > protected:\n> > > MdParser *parser_;\n> > > CameraMode mode_;\n> > > +\n> > > +private:\n> > > bool initialized_;\n> > > + /* Largest possible frame length, in units of lines. */\n> > > + unsigned int maxFrameLength_;\n> > > + /*\n> > > + * Smallest difference between the frame length and integration time,\n> > > + * in units of lines.\n> > > + */\n> > > + unsigned int frameIntegrationDiff_;\n> > > };\n> > >\n> > > // This is for registering camera helpers with the system, so that the\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > index db8ab879..8688ec09 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > @@ -49,13 +49,22 @@ public:\n> > > double Gain(uint32_t gain_code) const override;\n> > > unsigned int MistrustFramesModeSwitch() const override;\n> > > bool SensorEmbeddedDataPresent() const override;\n> > > +\n> > > +private:\n> > > + /*\n> > > + * Smallest difference between the frame length and integration time,\n> > > + * in units of lines.\n> > > + */\n> > > + static constexpr int frameIntegrationDiff = 4;\n> > > + /* Largest possible frame length, in units of lines. */\n> > > + static constexpr int maxFrameLength = 0xffff;\n> > > };\n> > >\n> > > CamHelperImx219::CamHelperImx219()\n> > > #if ENABLE_EMBEDDED_DATA\n> > > - : CamHelper(new MdParserImx219())\n> > > + : CamHelper(new MdParserImx219(), maxFrameLength, frameIntegrationDiff)\n> > > #else\n> > > - : CamHelper(new MdParserRPi())\n> > > + : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> > > #endif\n> > > {\n> > > }\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > index 0e896ac7..53961310 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > @@ -38,10 +38,19 @@ public:\n> > > uint32_t GainCode(double gain) const override;\n> > > double Gain(uint32_t gain_code) const override;\n> > > bool SensorEmbeddedDataPresent() const override;\n> > > +\n> > > +private:\n> > > + /*\n> > > + * Smallest difference between the frame length and integration time,\n> > > + * in units of lines.\n> > > + */\n> > > + static constexpr int frameIntegrationDiff = 22;\n> > > + /* Largest possible frame length, in units of lines. */\n> > > + static constexpr int maxFrameLength = 0xffdc;\n> > > };\n> > >\n> > > CamHelperImx477::CamHelperImx477()\n> > > - : CamHelper(new MdParserImx477())\n> > > + : CamHelper(new MdParserImx477(), maxFrameLength, frameIntegrationDiff)\n> > > {\n> > > }\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > index 0b841cd1..2bd8a754 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > @@ -23,6 +23,15 @@ public:\n> > > unsigned int HideFramesModeSwitch() const override;\n> > > unsigned int MistrustFramesStartup() const override;\n> > > unsigned int MistrustFramesModeSwitch() const override;\n> > > +\n> > > +private:\n> > > + /*\n> > > + * Smallest difference between the frame length and integration time,\n> > > + * in units of lines.\n> > > + */\n> > > + static constexpr int frameIntegrationDiff = 4;\n> > > + /* Largest possible frame length, in units of lines. */\n> > > + static constexpr int maxFrameLength = 0xffff;\n> > > };\n> > >\n> > > /*\n> > > @@ -31,7 +40,7 @@ public:\n> > > */\n> > >\n> > > CamHelperOv5647::CamHelperOv5647()\n> > > - : CamHelper(new MdParserRPi())\n> > > + : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> > > {\n> > > }\n> > >\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index d087b07e..d309536b 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -58,6 +58,8 @@ namespace libcamera {\n> > > /* Configure the sensor with these values initially. */\n> > > constexpr double DefaultAnalogueGain = 1.0;\n> > > constexpr unsigned int DefaultExposureTime = 20000;\n> > > +constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> > > +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> > >\n> > > LOG_DEFINE_CATEGORY(IPARPI)\n> > >\n> > > @@ -150,6 +152,9 @@ private:\n> > >\n> > > /* Distinguish the first camera start from others. */\n> > > bool firstStart_;\n> > > +\n> > > + /* Frame duration (1/fps) given in microseconds. */\n> > > + double minFrameDuration_, maxFrameDuration_;\n> > > };\n> > >\n> > > int IPARPi::init(const IPASettings &settings)\n> > > @@ -332,7 +337,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > sensorMetadata = helper_->SensorEmbeddedDataPresent();\n> > >\n> > > result->data.push_back(gainDelay);\n> > > - result->data.push_back(exposureDelay);\n> > > + result->data.push_back(exposureDelay); /* For EXPOSURE ctrl */\n> > > + result->data.push_back(exposureDelay); /* For VBLANK ctrl */\n> > > result->data.push_back(sensorMetadata);\n> > >\n> > > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> > > @@ -377,6 +383,9 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > controller_.Initialise();\n> > > controllerInit_ = true;\n> > >\n> > > + minFrameDuration_ = defaultMinFrameDuration;\n> > > + maxFrameDuration_ = defaultMaxFrameDuration;\n> > > +\n> > > /* Supply initial values for gain and exposure. */\n> > > ControlList ctrls(sensorCtrls_);\n> > > AgcStatus agcStatus;\n> > > @@ -524,7 +533,8 @@ bool IPARPi::validateSensorControls()\n> > > {\n> > > static const uint32_t ctrls[] = {\n> > > V4L2_CID_ANALOGUE_GAIN,\n> > > - V4L2_CID_EXPOSURE\n> > > + V4L2_CID_EXPOSURE,\n> > > + V4L2_CID_VBLANK\n> > > };\n> > >\n> > > for (auto c : ctrls) {\n> > > @@ -804,6 +814,24 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > break;\n> > > }\n> > >\n> > > + case controls::FRAME_DURATIONS: {\n> > > + auto frameDurations = ctrl.second.get>();\n> > > +\n> > > + /* This will be applied once AGC recalculations occur. */\n> > > + minFrameDuration_ = frameDurations[0] ? frameDurations[0] : defaultMinFrameDuration;\n> > > + maxFrameDuration_ = frameDurations[1] ? frameDurations[1] : defaultMaxFrameDuration;\n> >\n> > Should it be documented in patch 1/3 that setting the min or max to 0\n> > will reset the limit to the default ?\n> \n> There was a mention of this in the control documentation at an earlier\n> revision. Jacopo suggested we remove it for now, as it was a bit ambiguous\n> as to what a \"default\" frame duration is. It could be a sensor provided\n> default, or as is the case with Raspberry Pi, an IPA provided default,\n> or... I was a bit torn as to whether we needed a way to set defaults at\n> all. What do you think?\n\nThe concept of default control values is indeed not explicit in the\ndocumentation (something else for my todo list :-)), but we have\ndefaults already, they are used when applications don't set values for\nall controls in requests. My concern here is that we document how to\nconstraint the frame duration (which will be used, in many cases, to set\na fixed frame duration), but we don't tell how to go back to a variable\nframe rate.\n\nThe same issue exists with the ExposureTime and AnalogueGain controls,\napplications can go back to variable exposure time and gain but setting\nthe controls to 0, but this isn't documented. We need to fix this\nglobally.\n\nCould you add a \\todo comment in patch 1/3 to remind of the issue ? I\nthink it could be useful to also already include a sentence explaining\nhow to go back to unconstrained mode, but feel free to disagree.\n\n> > Will the code behave correctly if min > max ?\n> \n> No, bad things will happen :) I will guard against this by ensuring max >=\n> min.\n> \n> > > +\n> > > + /*\n> > > + * \\todo The values returned in the metadata below must be\n> > > + * correctly clipped by what the sensor mode supports and\n> > > + * what the AGC exposure mode or manual shutter speed limits\n> > > + */\n> > > + libcameraMetadata_.set(controls::FrameDurations,\n> > > + { static_cast(minFrameDuration_),\n> > > + static_cast(maxFrameDuration_) });\n> > > + break;\n> > > + }\n> > > +\n> > > default:\n> > > LOG(IPARPI, Warning)\n> > > << \"Ctrl \" << controls::controls.at (ctrl.first)->name()\n> > > @@ -962,15 +990,27 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> > > {\n> > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > > - int32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);\n> > >\n> > > - LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << agcStatus->shutter_time\n> > > - << \" (Shutter lines: \" << exposureLines << \") Gain: \"\n> > > + /* GetVBlanking might clip exposure time to the fps limits. */\n> > > + double exposure = agcStatus->shutter_time;\n> > > + int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,\n> > > + maxFrameDuration_);\n> > > + int32_t exposureLines = helper_->ExposureLines(exposure);\n> > > +\n> > > + LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> > > + << \" (Shutter lines: \" << exposureLines << \", AGC requested \"\n> > > + << agcStatus->shutter_time << \") Gain: \"\n> > > << agcStatus->analogue_gain << \" (Gain Code: \"\n> > > << gainCode << \")\";\n> > >\n> > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > > + /*\n> > > + * Due to the behavior of V4L2, the current value of VBLANK could clip the\n> > > + * exposure time without us knowing. The next time though this function should\n> > > + * clip exposure correctly.\n> > > + */\n> >\n> > This is something we should address in V4L2 (obviously not a blocker for\n> > this series). If you could design it from scratch, what would be a good\n> > API to configure the h/v blanking (possibly expressed as h/v active\n> > instead of blanking) and exposure time ?\n> \n> This is purely personal preference of course, but I would love it if the\n> world worked like this:\n> \n> 1) Userland sets a minimum and maximum frame rate (or frame duration) on a\n> sensor device driver. This can happen at the start of day, or during\n> runtime.\n> 2) Userland then provides an exposure time (in us or ns rather than in\n> lines) to the sensor device driver during normal AE operation.\n> 3) Based on the frame rate/duration limits the sensor will\n> automatically adjust the V and H blanking to fulfil the exposure time if it\n> can.\n> \n> For normal sensors, H/V blanking operations have very similar meaning, but\n> for more advanced modes (e.g. ultra long exposures or global shutter modes\n> of operation) they lose that meaning. I feel trying to encode all of that\n> in a kernel API might be hard as it's a moving target. This is why I think\n> that the blanking is probably left to the driver to deal with in a device\n> specific way. With useland in control of setting framerate/duration\n> limits, I see no reason why it would have any need to set blanking limits,\n> but of course, I could be missing use cases that I have never had to deal\n> with!\n\nIt's interesting that you would like to see this moved to the kernel\nside, I didn't expect that. There are multiple reasons we tried to push\nthis to userspace instead, the two main ones being that horizontal and\nvertical blanking may have different implications (as you mentioned\nabove, it's not always the case but can be), and the other one being\nthat leaving blanking calculation to driver will mean different\nbehaviours in different drivers, which would get messy for userspace\npretty quickly.\n\nOne (smallà change I was considering recently was a replacement of the\nblanking controls with H/V total controls. Sensors usually have hardware\nlimits on the horizontal and vertical total sizes, not on the blanking\nitself (but it may not be the case for all hardware...).\n\n> > > + ctrls.set(V4L2_CID_VBLANK, vblanking);\n> > > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> > > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > > }\n> > >\n> > > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 7a5f5881..252cab64 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > if (!staggeredCtrl_) {\n> > > staggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> > > { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },\n> > > - { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });\n> > > + { V4L2_CID_EXPOSURE, result.data[resultIdx++] },\n> > > + { V4L2_CID_VBLANK, result.data[resultIdx++] } });\n> > > sensorMetadata_ = result.data[resultIdx++];\n> > > }\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 26334C0F1A\n\tfor ;\n\tTue, 5 Jan 2021 13:10:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2A216278D;\n\tTue, 5 Jan 2021 14:10:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A41862012\n\tfor ;\n\tTue, 5 Jan 2021 14:10:10 +0100 (CET)","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 7EBC23D7;\n\tTue, 5 Jan 2021 14:10:09 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QJuHWwSw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1609852209;\n\tbh=SPfegKrgiJP3wqdPiULmONPuMwcjZvYzEPoY2vnlxrs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QJuHWwSwUOA49z++b6Pv5N0Aj/lafUByP4xHfhqRhgbNP8lraq4L79UV5IV73v/w+\n\tN2vFTiUWhjbpiiMj4yOPuv56Ppn5H/k1I/9VHTFOAt3KtDlxGRohlOK7FL0+HHnotN\n\tU4K1r9KcQPinsh5Nyxi4SKpSPhYg6W+AOzIOVm4o=","Date":"Tue, 5 Jan 2021 15:09:57 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20201218100626.205134-1-naush@raspberrypi.com>\n\t<20201218100626.205134-3-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v10 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]