[{"id":14610,"web_url":"https://patchwork.libcamera.org/comment/14610/","msgid":"<20210119170011.wtqfjnmm7osj4nkt@uno.localdomain>","date":"2021-01-19T17:00:11","subject":"Re: [libcamera-devel] [PATCH v12 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\n I know I have my tag here, and I'm possibly going to repeat some\ncomments. I don't think any of them should block your series though\n\nOn Tue, Jan 19, 2021 at 03:30:46PM +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 | 56 ++++++++++++++++---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> 8 files changed, 130 insertions(+), 15 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 01fe5abce9a6..1de36039cee0 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\nThe static initialization of the control limits worries me a bit.\nThis should match the sensor limits. I can't ask you to go to fully\nimplement this, but I fear the arbitrary initialization of the\ncontrols limits the RPi implementation does will be problematic for\napplication that might what to know in which range they can safely\noperate.\n\nI presume your forthcoming applications will probably know those limits\nfrom a per-sensor configuration file ?\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 6efa0d7fd030..b7b8faf09c69 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 * Clamp 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\nmin and max frame durations are here adjusted to respect the sensor's\nconfiguration. Shouldn't this be reflected in the metadata ?\n\n> +\t/*\n> +\t * Limit the exposure to the maximum frame duration requested, and\n> +\t * re-calculate if it has been clipped.\n> +\t */\n> +\texposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);\n\nCan't you here\n exposureLines = std::max(frameLengthMin, exposureLines + frameIntegrationDiff_);\n\nTo avoid the below clamp, which is partially useless as we're already\nsure exposureLines < (frameLengthMax - frameIntegrationDiff_)\n\n(and I'm not sure if frameIntegrationDiff_ has only to be considered\nwhen inspecting the upper limit or also when comparing to the bottom\none)\n\nit will also make sure the below recalculation of 'exposure' is in the limits\n\n\n\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\nThen you can simply\n return exposureLines - mode_.height;\n\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 044c28667f75..b1739a57e342 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\nThe first parameter is called just 'exposure' in the function\nimplementation. Mixing of snake_case and camelCase, but it's in many\nplaces already in the IPA, so...\n\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\nnit: different declaration order than in the derived classes, but\nwell, minor..\n\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 db8ab879deb7..8688ec091c44 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 0e896ac74f88..5396131003f1 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 0b841cd175fa..2bd8a754f133 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 d087b07e186b..ba9bc398ef87 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\nQuite arbitrary, isn't it ?\n\nI would have expected this to be sent by the pipeline to the IPA by\npopulating the FrameDuration ControlInfoMap at configure() time. The\n'sensorControls' provided at configure might very well contain the\nVBLANK control limits, which once combined with the sensor mode\ninformation should give you precise limits\n\n minFrameDuration = (mode.height + min(VBLANK)) * mode.line_lenght\n / (mode.pixel_rate / 1e6)\n\nOr is this not required ?\n\n>\n> LOG_DEFINE_CATEGORY(IPARPI)\n>\n> @@ -150,6 +152,10 @@ private:\n>\n> \t/* Distinguish the first camera start from others. */\n> \tbool firstStart_;\n> +\n> +\t/* Frame duration (1/fps) limits, given in microseconds. */\n> +\tdouble minFrameDuration_;\n> +\tdouble maxFrameDuration_;\n> };\n>\n> int IPARPi::init(const IPASettings &settings)\n> @@ -332,7 +338,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 +384,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 +534,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> @@ -551,7 +562,7 @@ bool IPARPi::validateIspControls()\n> \t\tV4L2_CID_USER_BCM2835_ISP_DENOISE,\n> \t\tV4L2_CID_USER_BCM2835_ISP_SHARPEN,\n> \t\tV4L2_CID_USER_BCM2835_ISP_DPC,\n> -\t\tV4L2_CID_USER_BCM2835_ISP_LENS_SHADING\n> +\t\tV4L2_CID_USER_BCM2835_ISP_LENS_SHADING,\n\nUnrelated it seems\n\n> \t};\n>\n> \tfor (auto c : ctrls) {\n> @@ -804,6 +815,25 @@ 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> +\t\t\tmaxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);\n\nWhy this last line ?\nAlso, it really seems seems arbitrary defaults clips the exposure to\npretty abitrary values. If you don't want to do the calculation of the\ndefaults, what about skipping the clipping completely ? Or is this\nyour intetion ? \"Pick default values large enough that no clipping\noccours\" ?\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 suppots and\n> +\t\t\t * what the AGC exposure mode or manual shutter speed limits\n> +\t\t\t */\n\nWhy this metadata can't be set in reportMetadata() which is called\nafter processStat() which calls GetVBlanking() and updates the limits\n?\n\n\nSorry for the many questions. The only thing that worries me is the\nControlInfo intialization, but that's not something you should address\nat the moment. Other than that with the metadata and default value\nassignement clarified, you can retain my tag.\n\nThanks\n j\n\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 +992,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> +\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 7a5f5881b9b3..252cab64023e 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> --\n> 2.25.1\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 98758C0F1C\n\tfor ;\n\tTue, 19 Jan 2021 16:59:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A81168161;\n\tTue, 19 Jan 2021 17:59:56 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A43F36814E\n\tfor ;\n\tTue, 19 Jan 2021 17:59:54 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id D9E7424000A;\n\tTue, 19 Jan 2021 16:59:53 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Tue, 19 Jan 2021 18:00:11 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210119170011.wtqfjnmm7osj4nkt@uno.localdomain>","References":"<20210119153047.468190-1-naush@raspberrypi.com>\n\t<20210119153047.468190-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210119153047.468190-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v12 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":14612,"web_url":"https://patchwork.libcamera.org/comment/14612/","msgid":"<20210119171112.3yzmxu6iw24kedjk@uno.localdomain>","date":"2021-01-19T17:11:12","subject":"Re: [libcamera-devel] [PATCH v12 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi again\n\nOn Tue, Jan 19, 2021 at 06:00:11PM +0100, Jacopo Mondi wrote:\n> Hi Naush,\n>\n> I know I have my tag here, and I'm possibly going to repeat some\n> comments. I don't think any of them should block your series though\n>\n> On Tue, Jan 19, 2021 at 03:30:46PM +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 | 56 ++++++++++++++++---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> > 8 files changed, 130 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index 01fe5abce9a6..1de36039cee0 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> The static initialization of the control limits worries me a bit.\n> This should match the sensor limits. I can't ask you to go to fully\n> implement this, but I fear the arbitrary initialization of the\n> controls limits the RPi implementation does will be problematic for\n> application that might what to know in which range they can safely\n> operate.\n>\n> I presume your forthcoming applications will probably know those limits\n> from a per-sensor configuration file ?\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 6efa0d7fd030..b7b8faf09c69 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 * Clamp 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>\n> min and max frame durations are here adjusted to respect the sensor's\n> configuration. Shouldn't this be reflected in the metadata ?\n>\n> > +\t/*\n> > +\t * Limit the exposure to the maximum frame duration requested, and\n> > +\t * re-calculate if it has been clipped.\n> > +\t */\n> > +\texposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);\n>\n> Can't you here\n> exposureLines = std::max(frameLengthMin, exposureLines + frameIntegrationDiff_);\n>\n> To avoid the below clamp, which is partially useless as we're already\n> sure exposureLines < (frameLengthMax - frameIntegrationDiff_)\n>\n> (and I'm not sure if frameIntegrationDiff_ has only to be considered\n> when inspecting the upper limit or also when comparing to the bottom\n> one)\n>\n> it will also make sure the below recalculation of 'exposure' is in the limits\n>\n>\n>\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>\n> Then you can simply\n> return exposureLines - mode_.height;\n>\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 044c28667f75..b1739a57e342 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>\n> The first parameter is called just 'exposure' in the function\n> implementation. Mixing of snake_case and camelCase, but it's in many\n> places already in the IPA, so...\n>\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> nit: different declaration order than in the derived classes, but\n> well, minor..\n>\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 db8ab879deb7..8688ec091c44 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 0e896ac74f88..5396131003f1 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 0b841cd175fa..2bd8a754f133 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 d087b07e186b..ba9bc398ef87 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> Quite arbitrary, isn't it ?\n>\n> I would have expected this to be sent by the pipeline to the IPA by\n> populating the FrameDuration ControlInfoMap at configure() time. The\n\nSorry, I meant by \"populating the entityControls map at configure()\ntime\". We exchange V4L2 controls at configure() not libcamera\ncontrols, and in fact in my proposed calculation here I use VBLANK.\n\n> 'sensorControls' provided at configure might very well contain the\n> VBLANK control limits, which once combined with the sensor mode\n> information should give you precise limits\n>\n> minFrameDuration = (mode.height + min(VBLANK)) * mode.line_lenght\n> / (mode.pixel_rate / 1e6)\n>\n> Or is this not required ?\n>\n> >\n> > LOG_DEFINE_CATEGORY(IPARPI)\n> >\n> > @@ -150,6 +152,10 @@ private:\n> >\n> > \t/* Distinguish the first camera start from others. */\n> > \tbool firstStart_;\n> > +\n> > +\t/* Frame duration (1/fps) limits, given in microseconds. */\n> > +\tdouble minFrameDuration_;\n> > +\tdouble maxFrameDuration_;\n> > };\n> >\n> > int IPARPi::init(const IPASettings &settings)\n> > @@ -332,7 +338,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 +384,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 +534,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> > @@ -551,7 +562,7 @@ bool IPARPi::validateIspControls()\n> > \t\tV4L2_CID_USER_BCM2835_ISP_DENOISE,\n> > \t\tV4L2_CID_USER_BCM2835_ISP_SHARPEN,\n> > \t\tV4L2_CID_USER_BCM2835_ISP_DPC,\n> > -\t\tV4L2_CID_USER_BCM2835_ISP_LENS_SHADING\n> > +\t\tV4L2_CID_USER_BCM2835_ISP_LENS_SHADING,\n>\n> Unrelated it seems\n>\n> > \t};\n> >\n> > \tfor (auto c : ctrls) {\n> > @@ -804,6 +815,25 @@ 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> > +\t\t\tmaxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);\n>\n> Why this last line ?\n> Also, it really seems seems arbitrary defaults clips the exposure to\n> pretty abitrary values. If you don't want to do the calculation of the\n> defaults, what about skipping the clipping completely ? Or is this\n> your intetion ? \"Pick default values large enough that no clipping\n> occours\" ?\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 suppots and\n> > +\t\t\t * what the AGC exposure mode or manual shutter speed limits\n> > +\t\t\t */\n>\n> Why this metadata can't be set in reportMetadata() which is called\n> after processStat() which calls GetVBlanking() and updates the limits\n> ?\n>\n>\n> Sorry for the many questions. The only thing that worries me is the\n> ControlInfo intialization, but that's not something you should address\n> at the moment. Other than that with the metadata and default value\n> assignement clarified, you can retain my tag.\n>\n> Thanks\n> j\n>\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 +992,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> > +\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 7a5f5881b9b3..252cab64023e 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> > --\n> > 2.25.1\n> >\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 BC775BD808\n\tfor ;\n\tTue, 19 Jan 2021 17:10:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93ECC68163;\n\tTue, 19 Jan 2021 18:10:55 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B0F96814E\n\tfor ;\n\tTue, 19 Jan 2021 18:10:54 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 14F3F1BF20B;\n\tTue, 19 Jan 2021 17:10:53 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Tue, 19 Jan 2021 18:11:12 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210119171112.3yzmxu6iw24kedjk@uno.localdomain>","References":"<20210119153047.468190-1-naush@raspberrypi.com>\n\t<20210119153047.468190-2-naush@raspberrypi.com>\n\t<20210119170011.wtqfjnmm7osj4nkt@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210119170011.wtqfjnmm7osj4nkt@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v12 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":14614,"web_url":"https://patchwork.libcamera.org/comment/14614/","msgid":"","date":"2021-01-19T18:13:33","subject":"Re: [libcamera-devel] [PATCH v12 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 Jacopo,\n\nThank you for your review comments.\n\nOn Tue, 19 Jan 2021 at 16:59, Jacopo Mondi wrote:\n\n> Hi Naush,\n>\n> I know I have my tag here, and I'm possibly going to repeat some\n> comments. I don't think any of them should block your series though\n>\n> On Tue, Jan 19, 2021 at 03:30:46PM +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 | 56 ++++++++++++++++---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> > 8 files changed, 130 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> > index 01fe5abce9a6..1de36039cee0 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> The static initialization of the control limits worries me a bit.\n> This should match the sensor limits. I can't ask you to go to fully\n> implement this, but I fear the arbitrary initialization of the\n> controls limits the RPi implementation does will be problematic for\n> application that might what to know in which range they can safely\n> operate.\n>\n> I presume your forthcoming applications will probably know those limits\n> from a per-sensor configuration file ?\n>\n\nYou are correct here, the static initialization should match the current\nsensor mode limits. There currently exists no mechanism for this, so these\nvalues above are indeed somewhat arbitrary for now.\n\n\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 6efa0d7fd030..b7b8faf09c69 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> > + * Clamp 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> min and max frame durations are here adjusted to respect the sensor's\n> configuration. Shouldn't this be reflected in the metadata ?\n>\n\nCorrect, they should - but currently don't. We discussed this in an\nearlier thread, but I have another series related to fps for plumbing in\nframe duration limits into our AGC - and with this change we return back\nthe fully constrained durations in the metadata. There is a \\todo in the\nhandling of the FrameDurations control saying this is a pending change to\nbe made.\n\n\n>\n> > + /*\n> > + * Limit the exposure to the maximum frame duration requested, and\n> > + * re-calculate if it has been clipped.\n> > + */\n> > + exposureLines = std::min(frameLengthMax - frameIntegrationDiff_,\n> exposureLines);\n>\n> Can't you here\n> exposureLines = std::max(frameLengthMin, exposureLines +\n> frameIntegrationDiff_);\n>\n\nIs this correct? For rolling shutter sensors, it is typically the case that\n\nframe length - exposure lines must be >= frame integration difference\n\nThe line in question is ensuring the maximum exposure cannot exceed the\nmaximum frame length - frame integration difference. With the suggested\nchange, we are instead saying that it might be possible to have\nexposureLine = frameLengthMin, so not accounting for\nframeIntegrationDiff_. But also, if there is no clipping, we are\nincreasing the requested exposure time by frameIntegrationDiff_ lines. My\nhead is spinning a bit with all of this, so please do correct me if what I\nhave understood is rubbish :-)\n\n\n> To avoid the below clamp, which is partially useless as we're already\n> sure exposureLines < (frameLengthMax - frameIntegrationDiff_)\n>\n> (and I'm not sure if frameIntegrationDiff_ has only to be considered\n> when inspecting the upper limit or also when comparing to the bottom\n> one)\n>\n\nAh, I *think* maybe I understand where you are going with this. In my\ncalculations, I modify frame length to match the exposure requested (within\nallowable limits). Hence I only consider frameIntegrationDiff_ on the\nupper limit, and explicitly subtract it for the vblank calculation below.\nI think your change is doing things the other way round, i.e. adjusting\nexposure to match the given frame length? If that is the case, I would\nprefer to keep it as is, so we get the exposure as close as possible to the\nrequest. If that is not the case, I'm sorry, but I have misunderstood :-)\n\n\n>\n> it will also make sure the below recalculation of 'exposure' is in the\n> limits\n>\n>\n>\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>\n> Then you can simply\n> return exposureLines - mode_.height;\n>\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 044c28667f75..b1739a57e342 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>\n> The first parameter is called just 'exposure' in the function\n> implementation. Mixing of snake_case and camelCase, but it's in many\n> places already in the IPA, so...\n>\n\nIndeed, this was also brought up in earlier comments. Our \"controller\"\nuses snake case throughout (mostly) and we do have a task of converting to\ncamel case as some point to match our IPA (which is camel case throughout).\n\n\n>\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> nit: different declaration order than in the derived classes, but\n> well, minor..\n>\n\nAck.\n\n\n>\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 db8ab879deb7..8688ec091c44 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 0e896ac74f88..5396131003f1 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 0b841cd175fa..2bd8a754f133 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 d087b07e186b..ba9bc398ef87 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> Quite arbitrary, isn't it ?\n>\n\nYes it is somewhat. I wanted to establish a default frameduration interval\nto start with in the absence of a request from the user. Now this could be\nvariable framerate (as I have done here), or a fixed framerate. We have a\npreference to run at variable framerate by default, hence the values\nabove. Note that these defaults are not necessarily what the sensor min\nand max are. For example, a sensor may be able to do 200fps, and we do not\nwant to go to such extreme exposure times for a simple viewfinder as it\nwould probably degrade image quality. But if an application really wants\nto do that, it is free to set the frame duration limits appropriately. In\nthat respect, having the default values as a const in the IPA seemed to\nmake more sense.\n\n\n>\n> I would have expected this to be sent by the pipeline to the IPA by\n> populating the FrameDuration ControlInfoMap at configure() time. The\n> 'sensorControls' provided at configure might very well contain the\n> VBLANK control limits, which once combined with the sensor mode\n> information should give you precise limits\n>\n> minFrameDuration = (mode.height + min(VBLANK)) * mode.line_lenght\n> / (mode.pixel_rate / 1e6)\n>\n> Or is this not required ?\n>\n\nCorrect, this is a calculation that is required, but not yet in place. My\nnext patch series for fps does add this calculation in to control sensor\nlimits. We discussed this previously, and decided not to introduce that\nseries just yet to get this one through with basic functionality.\n\n\n>\n> >\n> > LOG_DEFINE_CATEGORY(IPARPI)\n> >\n> > @@ -150,6 +152,10 @@ private:\n> >\n> > /* Distinguish the first camera start from others. */\n> > bool firstStart_;\n> > +\n> > + /* Frame duration (1/fps) limits, given in microseconds. */\n> > + double minFrameDuration_;\n> > + double maxFrameDuration_;\n> > };\n> >\n> > int IPARPi::init(const IPASettings &settings)\n> > @@ -332,7 +338,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 +384,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 +534,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> > @@ -551,7 +562,7 @@ bool IPARPi::validateIspControls()\n> > V4L2_CID_USER_BCM2835_ISP_DENOISE,\n> > V4L2_CID_USER_BCM2835_ISP_SHARPEN,\n> > V4L2_CID_USER_BCM2835_ISP_DPC,\n> > - V4L2_CID_USER_BCM2835_ISP_LENS_SHADING\n> > + V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,\n>\n> Unrelated it seems\n>\n\nAck. Thank Kieran for pointing it out :-)\n\n\n>\n> > };\n> >\n> > for (auto c : ctrls) {\n> > @@ -804,6 +815,25 @@ 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> > + maxFrameDuration_ = std::max(maxFrameDuration_,\n> minFrameDuration_);\n>\n> Why this last line ?\n>\n\nThat last line was a recent addition. Laurent correctly pointed out that\nbad things would happen if maxFrameDuration_ < minFrameDuration_.\n\n\n> Also, it really seems seems arbitrary defaults clips the exposure to\n> pretty abitrary values. If you don't want to do the calculation of the\n> defaults, what about skipping the clipping completely ? Or is this\n> your intetion ? \"Pick default values large enough that no clipping\n> occours\" ?\n>\n\nThe idea was that setting a 0 for either min or max would revert to the ipa\ndefaults. With default values for either/both min and max values, we still\nneed to ensure maxFrameDuration_ >= minFrameDuration_. Perhaps the\ndisconnect here is the notion of using IPA provided default values?\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> suppots and\n> > + * what the AGC exposure mode or manual shutter\n> speed limits\n> > + */\n>\n> Why this metadata can't be set in reportMetadata() which is called\n> after processStat() which calls GetVBlanking() and updates the limits\n> ?\n>\n\nreportMetadata() is used for per-frame dynamic metadata.\nFrameDurationLimits metadata is only set once when a request comes\nthrough. This is to inform the app if any clipping has occurred over the\nuser requested values. I know we had quite a long discussion about what\nexactly the return metadata value should be, and I think we agreed on\nthis? The next patch series for fps does call GetVBlanking() from here to\ncorrectly clip the return values.\n\n\n>\n>\n> Sorry for the many questions. The only thing that worries me is the\n> ControlInfo intialization, but that's not something you should address\n> at the moment. Other than that with the metadata and default value\n> assignement clarified, you can retain my tag.\n>\n\nNot a problem. Good to ensure things are right by everyone. Please let me\nknow your thoughts on my reply, there may be more to discuss before\nsubmitting this.\n\nRegards,\nNaush\n\n\n>\n> Thanks\n> j\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 +992,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> > + 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 7a5f5881b9b3..252cab64023e 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> > 2.25.1\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 5DA8DBD808\n\tfor ;\n\tTue, 19 Jan 2021 18:13:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB8D66816D;\n\tTue, 19 Jan 2021 19:13:52 +0100 (CET)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3A60C6814E\n\tfor ;\n\tTue, 19 Jan 2021 19:13:50 +0100 (CET)","by mail-lf1-x129.google.com with SMTP id m25so30362914lfc.11\n\tfor ;\n\tTue, 19 Jan 2021 10:13:50 -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=\"S5McAdip\"; 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=XiMFUUKyJ61zTiU0s65ZG09hhCY63eoinJI/0XQJxrI=;\n\tb=S5McAdipzbIB0C42lKxxsmSNqAUhbeKCBdlGX84paE/Lh3WmsPm3QHtfIqigQ4AigH\n\tOaV2ATgEPOIIJiUftlx0ZFZyleaOTirsfBHho8Omo+MzzZZfHerSOQDKpQrWVFqs5SMF\n\tAcGVMtlB05XcQH59fDN/Q26Ue7yuhzMLhA/ufjXSjQznrQ+Jo2zZrgWL8Wmg5lXVdp1A\n\t1dLf3kTVh6rbJbxJCDQ85dWEMwNbhKbXo8f/+TLRc8Zf4kKHSsL25wwBcYmphNE+CYS+\n\t+06Pn1lE8PDbo5oRVmw3frSFwgsDNJHgoumdpVZbJrPh3NpFgq95BVqyNh7fGtsmrRDS\n\tW0uQ==","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=XiMFUUKyJ61zTiU0s65ZG09hhCY63eoinJI/0XQJxrI=;\n\tb=nu++2T+Saj2zYImQU5qbS2h7h16xeslJc3Y4ya03+fC3rqmBHiGKA1NYc1G+ed705q\n\toWFSB2awXuOGe2A392T0qmtPMBT/RUEiRsZbruLfcEEsubH9aHRD8wFZRBY4GzTkVLfN\n\tEyQDkdTzHrfYeghHt34IWblTJMn/0QrVLhZIhaTbMFLm6/+Lj5Y9tKDJaEe4Iz8Bz8Jv\n\tI9VOY8m+lu2R6F4G6ShtSRpvRQfxNGqj7IY5akF1awyxlMi2mOQGIrzKDN1Rx9ByiGRP\n\t2l0jtOmT+tddWDYUlAJ4tsiDNS6/DRrrOBRCVH1I5dG3k900lFY8GewTqhaYZi4A5y5J\n\tY6SA==","X-Gm-Message-State":"AOAM530bZAdbQsK+jcpYBAw78QrZDeEFgXrYY/EXouCUFK8ArODa1dGY\n\tBOg04FmJr+jNHhVRJ+bYrXoz5cTpU3Ud0AasB+ANyw==","X-Google-Smtp-Source":"ABdhPJztAx3cmUq8CFRNB57F/ABpauGYQ8TgAaua7RvsrVlvgAnibNpoFGW5yBK8sjfrNlYr16b8k32cFDhP/7B/pyc=","X-Received":"by 2002:ac2:4daa:: with SMTP id\n\th10mr2290765lfe.617.1611080029479; \n\tTue, 19 Jan 2021 10:13:49 -0800 (PST)","MIME-Version":"1.0","References":"<20210119153047.468190-1-naush@raspberrypi.com>\n\t<20210119153047.468190-2-naush@raspberrypi.com>\n\t<20210119170011.wtqfjnmm7osj4nkt@uno.localdomain>","In-Reply-To":"<20210119170011.wtqfjnmm7osj4nkt@uno.localdomain>","From":"Naushir Patuck ","Date":"Tue, 19 Jan 2021 18:13:33 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v12 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=\"===============4007968260258524519==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14615,"web_url":"https://patchwork.libcamera.org/comment/14615/","msgid":"<20210119184813.hxqy5fb3exh2x45m@uno.localdomain>","date":"2021-01-19T18:48:13","subject":"Re: [libcamera-devel] [PATCH v12 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Tue, Jan 19, 2021 at 06:13:33PM +0000, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> Thank you for your review comments.\n>\n> On Tue, 19 Jan 2021 at 16:59, Jacopo Mondi wrote:\n>\n> > Hi Naush,\n> >\n> > I know I have my tag here, and I'm possibly going to repeat some\n> > comments. I don't think any of them should block your series though\n> >\n> > On Tue, Jan 19, 2021 at 03:30:46PM +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 | 56 ++++++++++++++++---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> > > 8 files changed, 130 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h\n> > b/include/libcamera/ipa/raspberrypi.h\n> > > index 01fe5abce9a6..1de36039cee0 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> > The static initialization of the control limits worries me a bit.\n> > This should match the sensor limits. I can't ask you to go to fully\n> > implement this, but I fear the arbitrary initialization of the\n> > controls limits the RPi implementation does will be problematic for\n> > application that might what to know in which range they can safely\n> > operate.\n> >\n> > I presume your forthcoming applications will probably know those limits\n> > from a per-sensor configuration file ?\n> >\n>\n> You are correct here, the static initialization should match the current\n> sensor mode limits. There currently exists no mechanism for this, so these\n> values above are indeed somewhat arbitrary for now.\n>\n>\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 6efa0d7fd030..b7b8faf09c69 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> > > + * Clamp 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> > min and max frame durations are here adjusted to respect the sensor's\n> > configuration. Shouldn't this be reflected in the metadata ?\n> >\n>\n> Correct, they should - but currently don't. We discussed this in an\n> earlier thread, but I have another series related to fps for plumbing in\n> frame duration limits into our AGC - and with this change we return back\n> the fully constrained durations in the metadata. There is a \\todo in the\n> handling of the FrameDurations control saying this is a pending change to\n> be made.\n>\n>\n> >\n> > > + /*\n> > > + * Limit the exposure to the maximum frame duration requested, and\n> > > + * re-calculate if it has been clipped.\n> > > + */\n> > > + exposureLines = std::min(frameLengthMax - frameIntegrationDiff_,\n> > exposureLines);\n> >\n> > Can't you here\n> > exposureLines = std::max(frameLengthMin, exposureLines +\n> > frameIntegrationDiff_);\n> >\n>\n> Is this correct? For rolling shutter sensors, it is typically the case that\n>\n> frame length - exposure lines must be >= frame integration difference\n>\n> The line in question is ensuring the maximum exposure cannot exceed the\n> maximum frame length - frame integration difference. With the suggested\n> change, we are instead saying that it might be possible to have\n> exposureLine = frameLengthMin, so not accounting for\n> frameIntegrationDiff_. But also, if there is no clipping, we are\n> increasing the requested exposure time by frameIntegrationDiff_ lines. My\n> head is spinning a bit with all of this, so please do correct me if what I\n> have understood is rubbish :-)\n>\n>\n> > To avoid the below clamp, which is partially useless as we're already\n> > sure exposureLines < (frameLengthMax - frameIntegrationDiff_)\n> >\n> > (and I'm not sure if frameIntegrationDiff_ has only to be considered\n> > when inspecting the upper limit or also when comparing to the bottom\n> > one)\n> >\n>\n> Ah, I *think* maybe I understand where you are going with this. In my\n> calculations, I modify frame length to match the exposure requested (within\n> allowable limits). Hence I only consider frameIntegrationDiff_ on the\n> upper limit, and explicitly subtract it for the vblank calculation below.\n> I think your change is doing things the other way round, i.e. adjusting\n> exposure to match the given frame length? If that is the case, I would\n> prefer to keep it as is, so we get the exposure as close as possible to the\n> request. If that is not the case, I'm sorry, but I have misunderstood :-)\n>\n\nOk sorry for the mess. My suggestion was to:\n exposureLines = min(frameLengthMax - frameIntegrationDiff, exposureLine);\n exposureLines = max(frameLenghtMin, exposureLines + frameIntegrationDiff);\n\nBut as you said this might add (+ frameIntegrationDiff) to the\nexposure. I should have probably said:\n\n exposureLines = min(frameLengthMax - frameIntegrationDiff, exposureLines);\n exposureLines = max(frameLenghtMin - frameIntegrationDiff, exposureLines);\n\nto clamp exposureLines in [min - diff, max - diff] interval\nBut I now get this is not required, as exposure time might be\nshorter than the min frame size, and VBLANK gets adjusted to respect\nthe min frame size regardless of the exposure time.\n\nIOW the exposure time is clipped by the max frame length ( -\nintegration diff) but can be shorter than min frame length.\n\nSorry to have you re-think this twice to disprove my suggestion.\n\n>\n> >\n> > it will also make sure the below recalculation of 'exposure' is in the\n> > limits\n> >\n> >\n> >\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> >\n> > Then you can simply\n> > return exposureLines - mode_.height;\n> >\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 044c28667f75..b1739a57e342 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> >\n> > The first parameter is called just 'exposure' in the function\n> > implementation. Mixing of snake_case and camelCase, but it's in many\n> > places already in the IPA, so...\n> >\n>\n> Indeed, this was also brought up in earlier comments. Our \"controller\"\n> uses snake case throughout (mostly) and we do have a task of converting to\n> camel case as some point to match our IPA (which is camel case throughout).\n>\n>\n> >\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> > nit: different declaration order than in the derived classes, but\n> > well, minor..\n> >\n>\n> Ack.\n>\n>\n> >\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 db8ab879deb7..8688ec091c44 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 0e896ac74f88..5396131003f1 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 0b841cd175fa..2bd8a754f133 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 d087b07e186b..ba9bc398ef87 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> > Quite arbitrary, isn't it ?\n> >\n>\n> Yes it is somewhat. I wanted to establish a default frameduration interval\n> to start with in the absence of a request from the user. Now this could be\n> variable framerate (as I have done here), or a fixed framerate. We have a\n> preference to run at variable framerate by default, hence the values\n> above. Note that these defaults are not necessarily what the sensor min\n> and max are. For example, a sensor may be able to do 200fps, and we do not\n> want to go to such extreme exposure times for a simple viewfinder as it\n> would probably degrade image quality. But if an application really wants\n> to do that, it is free to set the frame duration limits appropriately. In\n> that respect, having the default values as a const in the IPA seemed to\n> make more sense.\n>\n>\n> >\n> > I would have expected this to be sent by the pipeline to the IPA by\n> > populating the FrameDuration ControlInfoMap at configure() time. The\n> > 'sensorControls' provided at configure might very well contain the\n> > VBLANK control limits, which once combined with the sensor mode\n> > information should give you precise limits\n> >\n> > minFrameDuration = (mode.height + min(VBLANK)) * mode.line_lenght\n> > / (mode.pixel_rate / 1e6)\n> >\n> > Or is this not required ?\n> >\n>\n> Correct, this is a calculation that is required, but not yet in place. My\n> next patch series for fps does add this calculation in to control sensor\n> limits. We discussed this previously, and decided not to introduce that\n> series just yet to get this one through with basic functionality.\n>\n>\n> >\n> > >\n> > > LOG_DEFINE_CATEGORY(IPARPI)\n> > >\n> > > @@ -150,6 +152,10 @@ private:\n> > >\n> > > /* Distinguish the first camera start from others. */\n> > > bool firstStart_;\n> > > +\n> > > + /* Frame duration (1/fps) limits, given in microseconds. */\n> > > + double minFrameDuration_;\n> > > + double maxFrameDuration_;\n> > > };\n> > >\n> > > int IPARPi::init(const IPASettings &settings)\n> > > @@ -332,7 +338,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 +384,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 +534,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> > > @@ -551,7 +562,7 @@ bool IPARPi::validateIspControls()\n> > > V4L2_CID_USER_BCM2835_ISP_DENOISE,\n> > > V4L2_CID_USER_BCM2835_ISP_SHARPEN,\n> > > V4L2_CID_USER_BCM2835_ISP_DPC,\n> > > - V4L2_CID_USER_BCM2835_ISP_LENS_SHADING\n> > > + V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,\n> >\n> > Unrelated it seems\n> >\n>\n> Ack. Thank Kieran for pointing it out :-)\n>\n>\n> >\n> > > };\n> > >\n> > > for (auto c : ctrls) {\n> > > @@ -804,6 +815,25 @@ 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> > > + maxFrameDuration_ = std::max(maxFrameDuration_,\n> > minFrameDuration_);\n> >\n> > Why this last line ?\n> >\n>\n> That last line was a recent addition. Laurent correctly pointed out that\n> bad things would happen if maxFrameDuration_ < minFrameDuration_.\n>\n>\n> > Also, it really seems seems arbitrary defaults clips the exposure to\n> > pretty abitrary values. If you don't want to do the calculation of the\n> > defaults, what about skipping the clipping completely ? Or is this\n> > your intetion ? \"Pick default values large enough that no clipping\n> > occours\" ?\n> >\n>\n> The idea was that setting a 0 for either min or max would revert to the ipa\n> defaults. With default values for either/both min and max values, we still\n> need to ensure maxFrameDuration_ >= minFrameDuration_. Perhaps the\n> disconnect here is the notion of using IPA provided default values?\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> > suppots and\n> > > + * what the AGC exposure mode or manual shutter\n> > speed limits\n> > > + */\n> >\n> > Why this metadata can't be set in reportMetadata() which is called\n> > after processStat() which calls GetVBlanking() and updates the limits\n> > ?\n> >\n>\n> reportMetadata() is used for per-frame dynamic metadata.\n> FrameDurationLimits metadata is only set once when a request comes\n> through. This is to inform the app if any clipping has occurred over the\n> user requested values. I know we had quite a long discussion about what\n> exactly the return metadata value should be, and I think we agreed on\n> this? The next patch series for fps does call GetVBlanking() from here to\n> correctly clip the return values.\n\nI'm sorry, it seems we already have gone through this in the past, I'm\nrepeating the same questions it seems.\n\nCould you add my tag twice ? :)\n\n>\n>\n> >\n> >\n> > Sorry for the many questions. The only thing that worries me is the\n> > ControlInfo intialization, but that's not something you should address\n> > at the moment. Other than that with the metadata and default value\n> > assignement clarified, you can retain my tag.\n> >\n>\n> Not a problem. Good to ensure things are right by everyone. Please let me\n> know your thoughts on my reply, there may be more to discuss before\n> submitting is.\n\nYeah, good to be on the same page but sorry for having asked the same\nquestions again.\n\nLet's get this merged soon so we can close all points highlighted here\nin follow-up patches..\n\nThanks\n j\n\n>\n> Regards,\n> Naush\n>\n>\n> >\n> > Thanks\n> > j\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 +992,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> > > + 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 7a5f5881b9b3..252cab64023e 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> > > 2.25.1\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 2CF9BC0F1C\n\tfor ;\n\tTue, 19 Jan 2021 18:47:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A448A6816D;\n\tTue, 19 Jan 2021 19:47:57 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8437D68167\n\tfor ;\n\tTue, 19 Jan 2021 19:47:56 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id BBB6CFF802;\n\tTue, 19 Jan 2021 18:47:55 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Tue, 19 Jan 2021 19:48:13 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210119184813.hxqy5fb3exh2x45m@uno.localdomain>","References":"<20210119153047.468190-1-naush@raspberrypi.com>\n\t<20210119153047.468190-2-naush@raspberrypi.com>\n\t<20210119170011.wtqfjnmm7osj4nkt@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v12 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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14616,"web_url":"https://patchwork.libcamera.org/comment/14616/","msgid":"","date":"2021-01-19T19:45:54","subject":"Re: [libcamera-devel] [PATCH v12 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 Jacopo,\n\nOn Tue, 19 Jan 2021 at 18:47, Jacopo Mondi wrote:\n\n> Hi Naush,\n>\n> On Tue, Jan 19, 2021 at 06:13:33PM +0000, Naushir Patuck wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for your review comments.\n> >\n> > On Tue, 19 Jan 2021 at 16:59, Jacopo Mondi wrote:\n> >\n> > > Hi Naush,\n> > >\n> > > I know I have my tag here, and I'm possibly going to repeat some\n> > > comments. I don't think any of them should block your series though\n> > >\n> > > On Tue, Jan 19, 2021 at 03:30:46PM +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\n> 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 | 56\n> ++++++++++++++++---\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> > > > 8 files changed, 130 insertions(+), 15 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/raspberrypi.h\n> > > b/include/libcamera/ipa/raspberrypi.h\n> > > > index 01fe5abce9a6..1de36039cee0 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,\n> 16.0f) },\n> > > > { &controls::ScalerCrop, ControlInfo(Rectangle{},\n> Rectangle(65535,\n> > > 65535, 65535, 65535), Rectangle{}) },\n> > > > + { &controls::FrameDurations, ControlInfo(1000, 1000000000) },\n> > >\n> > > The static initialization of the control limits worries me a bit.\n> > > This should match the sensor limits. I can't ask you to go to fully\n> > > implement this, but I fear the arbitrary initialization of the\n> > > controls limits the RPi implementation does will be problematic for\n> > > application that might what to know in which range they can safely\n> > > operate.\n> > >\n> > > I presume your forthcoming applications will probably know those limits\n> > > from a per-sensor configuration file ?\n> > >\n> >\n> > You are correct here, the static initialization should match the current\n> > sensor mode limits. There currently exists no mechanism for this, so\n> these\n> > values above are indeed somewhat arbitrary for now.\n> >\n> >\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 6efa0d7fd030..b7b8faf09c69 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\n> 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> > > > + * Clamp frame length by the frame duration range and the\n> 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> > > min and max frame durations are here adjusted to respect the sensor's\n> > > configuration. Shouldn't this be reflected in the metadata ?\n> > >\n> >\n> > Correct, they should - but currently don't. We discussed this in an\n> > earlier thread, but I have another series related to fps for plumbing in\n> > frame duration limits into our AGC - and with this change we return back\n> > the fully constrained durations in the metadata. There is a \\todo in the\n> > handling of the FrameDurations control saying this is a pending change to\n> > be made.\n> >\n> >\n> > >\n> > > > + /*\n> > > > + * Limit the exposure to the maximum frame duration requested,\n> and\n> > > > + * re-calculate if it has been clipped.\n> > > > + */\n> > > > + exposureLines = std::min(frameLengthMax -\n> frameIntegrationDiff_,\n> > > exposureLines);\n> > >\n> > > Can't you here\n> > > exposureLines = std::max(frameLengthMin, exposureLines +\n> > > frameIntegrationDiff_);\n> > >\n> >\n> > Is this correct? For rolling shutter sensors, it is typically the case\n> that\n> >\n> > frame length - exposure lines must be >= frame integration difference\n> >\n> > The line in question is ensuring the maximum exposure cannot exceed the\n> > maximum frame length - frame integration difference. With the suggested\n> > change, we are instead saying that it might be possible to have\n> > exposureLine = frameLengthMin, so not accounting for\n> > frameIntegrationDiff_. But also, if there is no clipping, we are\n> > increasing the requested exposure time by frameIntegrationDiff_ lines.\n> My\n> > head is spinning a bit with all of this, so please do correct me if what\n> I\n> > have understood is rubbish :-)\n> >\n> >\n> > > To avoid the below clamp, which is partially useless as we're already\n> > > sure exposureLines < (frameLengthMax - frameIntegrationDiff_)\n> > >\n> > > (and I'm not sure if frameIntegrationDiff_ has only to be considered\n> > > when inspecting the upper limit or also when comparing to the bottom\n> > > one)\n> > >\n> >\n> > Ah, I *think* maybe I understand where you are going with this. In my\n> > calculations, I modify frame length to match the exposure requested\n> (within\n> > allowable limits). Hence I only consider frameIntegrationDiff_ on the\n> > upper limit, and explicitly subtract it for the vblank calculation below.\n> > I think your change is doing things the other way round, i.e. adjusting\n> > exposure to match the given frame length? If that is the case, I would\n> > prefer to keep it as is, so we get the exposure as close as possible to\n> the\n> > request. If that is not the case, I'm sorry, but I have misunderstood\n> :-)\n> >\n>\n> Ok sorry for the mess. My suggestion was to:\n> exposureLines = min(frameLengthMax - frameIntegrationDiff,\n> exposureLine);\n> exposureLines = max(frameLenghtMin, exposureLines +\n> frameIntegrationDiff);\n>\n> But as you said this might add (+ frameIntegrationDiff) to the\n> exposure. I should have probably said:\n>\n> exposureLines = min(frameLengthMax - frameIntegrationDiff,\n> exposureLines);\n> exposureLines = max(frameLenghtMin - frameIntegrationDiff,\n> exposureLines);\n>\n> to clamp exposureLines in [min - diff, max - diff] interval\n> But I now get this is not required, as exposure time might be\n> shorter than the min frame size, and VBLANK gets adjusted to respect\n> the min frame size regardless of the exposure time.\n>\n> IOW the exposure time is clipped by the max frame length ( -\n> integration diff) but can be shorter than min frame length.\n>\n> Sorry to have you re-think this twice to disprove my suggestion.\n>\n> >\n> > >\n> > > it will also make sure the below recalculation of 'exposure' is in the\n> > > limits\n> > >\n> > >\n> > >\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) -\n> mode_.height;\n> > >\n> > > Then you can simply\n> > > return exposureLines - mode_.height;\n> > >\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 044c28667f75..b1739a57e342 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> > >\n> > > The first parameter is called just 'exposure' in the function\n> > > implementation. Mixing of snake_case and camelCase, but it's in many\n> > > places already in the IPA, so...\n> > >\n> >\n> > Indeed, this was also brought up in earlier comments. Our \"controller\"\n> > uses snake case throughout (mostly) and we do have a task of converting\n> to\n> > camel case as some point to match our IPA (which is camel case\n> throughout).\n> >\n> >\n> > >\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)\n> 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> > > nit: different declaration order than in the derived classes, but\n> > > well, minor..\n> > >\n> >\n> > Ack.\n> >\n> >\n> > >\n> > > > };\n> > > >\n> > > > // This is for registering camera helpers with the system, so that\n> the\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > index db8ab879deb7..8688ec091c44 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 0e896ac74f88..5396131003f1 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 0b841cd175fa..2bd8a754f133 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 d087b07e186b..ba9bc398ef87 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> > > Quite arbitrary, isn't it ?\n> > >\n> >\n> > Yes it is somewhat. I wanted to establish a default frameduration\n> interval\n> > to start with in the absence of a request from the user. Now this could\n> be\n> > variable framerate (as I have done here), or a fixed framerate. We have\n> a\n> > preference to run at variable framerate by default, hence the values\n> > above. Note that these defaults are not necessarily what the sensor min\n> > and max are. For example, a sensor may be able to do 200fps, and we do\n> not\n> > want to go to such extreme exposure times for a simple viewfinder as it\n> > would probably degrade image quality. But if an application really wants\n> > to do that, it is free to set the frame duration limits appropriately.\n> In\n> > that respect, having the default values as a const in the IPA seemed to\n> > make more sense.\n> >\n> >\n> > >\n> > > I would have expected this to be sent by the pipeline to the IPA by\n> > > populating the FrameDuration ControlInfoMap at configure() time. The\n> > > 'sensorControls' provided at configure might very well contain the\n> > > VBLANK control limits, which once combined with the sensor mode\n> > > information should give you precise limits\n> > >\n> > > minFrameDuration = (mode.height + min(VBLANK)) *\n> mode.line_lenght\n> > > / (mode.pixel_rate / 1e6)\n> > >\n> > > Or is this not required ?\n> > >\n> >\n> > Correct, this is a calculation that is required, but not yet in place.\n> My\n> > next patch series for fps does add this calculation in to control sensor\n> > limits. We discussed this previously, and decided not to introduce that\n> > series just yet to get this one through with basic functionality.\n> >\n> >\n> > >\n> > > >\n> > > > LOG_DEFINE_CATEGORY(IPARPI)\n> > > >\n> > > > @@ -150,6 +152,10 @@ private:\n> > > >\n> > > > /* Distinguish the first camera start from others. */\n> > > > bool firstStart_;\n> > > > +\n> > > > + /* Frame duration (1/fps) limits, given in microseconds. */\n> > > > + double minFrameDuration_;\n> > > > + double maxFrameDuration_;\n> > > > };\n> > > >\n> > > > int IPARPi::init(const IPASettings &settings)\n> > > > @@ -332,7 +338,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\n> ctrl\n> > > */\n> > > > result->data.push_back(sensorMetadata);\n> > > >\n> > > > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> > > > @@ -377,6 +384,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 +534,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> > > > @@ -551,7 +562,7 @@ bool IPARPi::validateIspControls()\n> > > > V4L2_CID_USER_BCM2835_ISP_DENOISE,\n> > > > V4L2_CID_USER_BCM2835_ISP_SHARPEN,\n> > > > V4L2_CID_USER_BCM2835_ISP_DPC,\n> > > > - V4L2_CID_USER_BCM2835_ISP_LENS_SHADING\n> > > > + V4L2_CID_USER_BCM2835_ISP_LENS_SHADING,\n> > >\n> > > Unrelated it seems\n> > >\n> >\n> > Ack. Thank Kieran for pointing it out :-)\n> >\n> >\n> > >\n> > > > };\n> > > >\n> > > > for (auto c : ctrls) {\n> > > > @@ -804,6 +815,25 @@ void IPARPi::queueRequest(const ControlList\n> > > &controls)\n> > > > break;\n> > > > }\n> > > >\n> > > > + case controls::FRAME_DURATIONS: {\n> > > > + auto frameDurations =\n> 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> > > > + maxFrameDuration_ = std::max(maxFrameDuration_,\n> > > minFrameDuration_);\n> > >\n> > > Why this last line ?\n> > >\n> >\n> > That last line was a recent addition. Laurent correctly pointed out that\n> > bad things would happen if maxFrameDuration_ < minFrameDuration_.\n> >\n> >\n> > > Also, it really seems seems arbitrary defaults clips the exposure to\n> > > pretty abitrary values. If you don't want to do the calculation of the\n> > > defaults, what about skipping the clipping completely ? Or is this\n> > > your intetion ? \"Pick default values large enough that no clipping\n> > > occours\" ?\n> > >\n> >\n> > The idea was that setting a 0 for either min or max would revert to the\n> ipa\n> > defaults. With default values for either/both min and max values, we\n> still\n> > need to ensure maxFrameDuration_ >= minFrameDuration_. Perhaps the\n> > disconnect here is the notion of using IPA provided default values?\n> >\n> >\n> > >\n> > > > +\n> > > > + /*\n> > > > + * \\todo The values returned in the metadata\n> below\n> > > must be\n> > > > + * correctly clipped by what the sensor mode\n> > > suppots and\n> > > > + * what the AGC exposure mode or manual shutter\n> > > speed limits\n> > > > + */\n> > >\n> > > Why this metadata can't be set in reportMetadata() which is called\n> > > after processStat() which calls GetVBlanking() and updates the limits\n> > > ?\n> > >\n> >\n> > reportMetadata() is used for per-frame dynamic metadata.\n> > FrameDurationLimits metadata is only set once when a request comes\n> > through. This is to inform the app if any clipping has occurred over the\n> > user requested values. I know we had quite a long discussion about what\n> > exactly the return metadata value should be, and I think we agreed on\n> > this? The next patch series for fps does call GetVBlanking() from here\n> to\n> > correctly clip the return values.\n>\n> I'm sorry, it seems we already have gone through this in the past, I'm\n> repeating the same questions it seems.\n>\n> Could you add my tag twice ? :)\n>\n> >\n> >\n> > >\n> > >\n> > > Sorry for the many questions. The only thing that worries me is the\n> > > ControlInfo intialization, but that's not something you should address\n> > > at the moment. Other than that with the metadata and default value\n> > > assignement clarified, you can retain my tag.\n> > >\n> >\n> > Not a problem. Good to ensure things are right by everyone. Please let\n> me\n> > know your thoughts on my reply, there may be more to discuss before\n> > submitting is.\n>\n> Yeah, good to be on the same page but sorry for having asked the same\n> questions again.\n>\n> Let's get this merged soon so we can close all points highlighted here\n> in follow-up patches..\n>\n>\nThanks! So would you be ok if I leave this v12 series as-is and pending\nfeedback comments from Laurent, merge this version if he raises no other\nissues? I note that there is a minor nit on standardising the ordering of\nmember variables in the base and derived classes, but I will fix that up\nin my next series. Hope that is ok.\n\nRegards,\nNaush\n\n\n\nThanks\n> j\n>\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\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 +992,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> \")\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> \",\n> > > AGC requested \"\n> > > > + << agcStatus->shutter_time << \") Gain: \"\n> > > > << agcStatus->analogue_gain << \" (Gain\n> 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\n> could\n> > > clip the\n> > > > + * exposure time without us knowing. The next time though this\n> > > function should\n> > > > + * clip exposure correctly.\n> > > > + */\n> > > > + ctrls.set(V4L2_CID_VBLANK, vblanking);\n> > > > ctrls.set(V4L2_CID_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 7a5f5881b9b3..252cab64023e 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> > > >\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> > > > 2.25.1\n> > > >\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 C3F61BD808\n\tfor ;\n\tTue, 19 Jan 2021 19:46:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 419FA68173;\n\tTue, 19 Jan 2021 20:46:14 +0100 (CET)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6CE4368167\n\tfor ;\n\tTue, 19 Jan 2021 20:46:12 +0100 (CET)","by mail-lf1-x135.google.com with SMTP id u25so30798637lfc.2\n\tfor ;\n\tTue, 19 Jan 2021 11:46:12 -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=\"d/cs9wkl\"; 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=1xQVWAWr1w83gEfMg0C6q0c3M6Wk9seH3ImvqzDflbM=;\n\tb=d/cs9wkl5EZE2KRn6AS3LdHRIvBpqJ9FTGL9i3dtQ5mJcV4Mga5ugeWtxGt7UgM+OT\n\tZDntotvpPZNL8ABlui7mUeS9Nj/GvVy3cyllM1d7G2/4QkcmRZzft3EdFsS2lnkgtA3c\n\tsiHoFprIHa+83MOqsVzQ0hf7IApkFG6FaynAkFqlZuiczU9EfXggdWh5sU2J9H00lTL5\n\tN92wzF2kBAYdPu/KN+qfhoPI9ioDJXQROXO/2IgCLgFRlNQt7oSCuqGIbS8CrN63ABnq\n\tr4ChEulkt9R9EkuQj/6skHFZQIT9hS9rPhm2OJyOXrjGAlvBC7XLiaxemGm9skHabbQo\n\tOVtA==","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=1xQVWAWr1w83gEfMg0C6q0c3M6Wk9seH3ImvqzDflbM=;\n\tb=CcQkaNlFbJfAI8hQOC2g0jcJjRsLHmdo17fIYejwu7wg7bpmduGySgfvvHmPdrezmH\n\tkf+n3aT0aEpUUZbmWTu8r3AeNfGWTitkwJ0cGPf35/gea7kJR6M2AWJDUiJorG0MLPoy\n\tuaGHnWwa0ej2tdcuM8dpRAL5fUE9rYKRNaPsYpsIUde3GnuFslF6p74xc4Pws+ksS7zQ\n\t65CecKZZgPE0nfH3kLwBGxaqjPcg4LZS9cEgftbDaLYwWKQa5wjKBaN9kIgh6jB08m0+\n\tcqXhF0R26XnIgT+qmmBnSsMtlcX7wj0c0CydIPfv0FPb7WtU6uiTFhEvsq/48q2onBMB\n\tZ3YQ==","X-Gm-Message-State":"AOAM530uyM8EQZSoBWdR8XsJMxnvRGc67v37eO3IKdBRa0DshNnMdj4Q\n\tTE4G5dIT5so+jQhOLqg/qIMFQ9IkBOF5py5kX1xR4Q==","X-Google-Smtp-Source":"ABdhPJziy0X42Ydu5id9459nHYNG7A1fe5VlQnHVobPg8iTr+g7gkvIEzUbPeCZbvXN2sTSvKGoTxBlEqchXsnLi5f8=","X-Received":"by 2002:a19:8357:: with SMTP id\n\tf84mr2469008lfd.567.1611085571536; \n\tTue, 19 Jan 2021 11:46:11 -0800 (PST)","MIME-Version":"1.0","References":"<20210119153047.468190-1-naush@raspberrypi.com>\n\t<20210119153047.468190-2-naush@raspberrypi.com>\n\t<20210119170011.wtqfjnmm7osj4nkt@uno.localdomain>\n\t\n\t<20210119184813.hxqy5fb3exh2x45m@uno.localdomain>","In-Reply-To":"<20210119184813.hxqy5fb3exh2x45m@uno.localdomain>","From":"Naushir Patuck ","Date":"Tue, 19 Jan 2021 19:45:54 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v12 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=\"===============7020303358573964915==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14620,"web_url":"https://patchwork.libcamera.org/comment/14620/","msgid":"<20210120091544.7un4x7erilhv322y@uno.localdomain>","date":"2021-01-20T09:15:44","subject":"Re: [libcamera-devel] [PATCH v12 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Tue, Jan 19, 2021 at 07:45:54PM +0000, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n\n[snip]\n\n> > > >\n> > > >\n> > > > Sorry for the many questions. The only thing that worries me is the\n> > > > ControlInfo intialization, but that's not something you should address\n> > > > at the moment. Other than that with the metadata and default value\n> > > > assignement clarified, you can retain my tag.\n> > > >\n> > >\n> > > Not a problem. Good to ensure things are right by everyone. Please let\n> > me\n> > > know your thoughts on my reply, there may be more to discuss before\n> > > submitting is.\n> >\n> > Yeah, good to be on the same page but sorry for having asked the same\n> > questions again.\n> >\n> > Let's get this merged soon so we can close all points highlighted here\n> > in follow-up patches..\n> >\n> >\n> Thanks! So would you be ok if I leave this v12 series as-is and pending\n> feedback comments from Laurent, merge this version if he raises no other\n> issues? I note that there is a minor nit on standardising the ordering of\n> member variables in the base and derived classes, but I will fix that up\n> in my next series. Hope that is ok.\n\nYeah, it's ok no worries, no need to resend\n\nI hope we can this in quickly, as I need this control as well to be\nable to report to the Camera HAL the frame duration limits.\n\nThanks\n j\n\n\n>\n> Regards,\n> Naush\n>\n>\n>\n> Thanks\n> > j\n> >\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\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 +992,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> > \")\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> > \",\n> > > > AGC requested \"\n> > > > > + << agcStatus->shutter_time << \") Gain: \"\n> > > > > << agcStatus->analogue_gain << \" (Gain\n> > 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\n> > could\n> > > > clip the\n> > > > > + * exposure time without us knowing. The next time though this\n> > > > function should\n> > > > > + * clip exposure correctly.\n> > > > > + */\n> > > > > + ctrls.set(V4L2_CID_VBLANK, vblanking);\n> > > > > ctrls.set(V4L2_CID_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 7a5f5881b9b3..252cab64023e 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> > > > >\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> > > > > 2.25.1\n> > > > >\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 68B5ABD808\n\tfor ;\n\tWed, 20 Jan 2021 09:15:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01A636817D;\n\tWed, 20 Jan 2021 10:15:28 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B89A68177\n\tfor ;\n\tWed, 20 Jan 2021 10:15:26 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 60F4B40005;\n\tWed, 20 Jan 2021 09:15:25 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 20 Jan 2021 10:15:44 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210120091544.7un4x7erilhv322y@uno.localdomain>","References":"<20210119153047.468190-1-naush@raspberrypi.com>\n\t<20210119153047.468190-2-naush@raspberrypi.com>\n\t<20210119170011.wtqfjnmm7osj4nkt@uno.localdomain>\n\t\n\t<20210119184813.hxqy5fb3exh2x45m@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v12 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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14637,"web_url":"https://patchwork.libcamera.org/comment/14637/","msgid":"","date":"2021-01-20T11:55:19","subject":"Re: [libcamera-devel] [PATCH v12 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 Tue, Jan 19, 2021 at 03:30:46PM +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 | 56 ++++++++++++++++---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> 8 files changed, 130 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 01fe5abce9a6..1de36039cee0 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 6efa0d7fd030..b7b8faf09c69 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 * Clamp 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 044c28667f75..b1739a57e342 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 db8ab879deb7..8688ec091c44 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 0e896ac74f88..5396131003f1 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 0b841cd175fa..2bd8a754f133 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 d087b07e186b..ba9bc398ef87 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,10 @@ private:\n> \n> \t/* Distinguish the first camera start from others. */\n> \tbool firstStart_;\n> +\n> +\t/* Frame duration (1/fps) limits, given in microseconds. */\n> +\tdouble minFrameDuration_;\n> +\tdouble maxFrameDuration_;\n> };\n> \n> int IPARPi::init(const IPASettings &settings)\n> @@ -332,7 +338,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 +384,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 +534,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> @@ -551,7 +562,7 @@ bool IPARPi::validateIspControls()\n> \t\tV4L2_CID_USER_BCM2835_ISP_DENOISE,\n> \t\tV4L2_CID_USER_BCM2835_ISP_SHARPEN,\n> \t\tV4L2_CID_USER_BCM2835_ISP_DPC,\n> -\t\tV4L2_CID_USER_BCM2835_ISP_LENS_SHADING\n> +\t\tV4L2_CID_USER_BCM2835_ISP_LENS_SHADING,\n> \t};\n> \n> \tfor (auto c : ctrls) {\n> @@ -804,6 +815,25 @@ 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> +\t\t\tmaxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);\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 suppots and\n\ns/suppots/supports/\n\nI'll handle this when applying.\n\nReviewed-by: Laurent Pinchart \n\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 +992,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> +\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 7a5f5881b9b3..252cab64023e 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 D0290BD808\n\tfor ;\n\tWed, 20 Jan 2021 11:55:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 96ADB681B3;\n\tWed, 20 Jan 2021 12:55:38 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DCA0A681AF\n\tfor ;\n\tWed, 20 Jan 2021 12:55:36 +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 6D217813;\n\tWed, 20 Jan 2021 12:55:36 +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=\"DePUWRRI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611143736;\n\tbh=EOeUOPYRE1TLhXzEpSSaIPjUHJpiYpBOnJjH3lmUD9s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DePUWRRIqA/mWPFWb+7Dt7yhBuFWEmdSb/ScwiHIn7s1ljNXjA8ofp+C27PNa5rla\n\tHgQMoCweneDOZ641flKNuYe7s6WsmvGjwCeQdEkP78RzHZ/Ssh5vSHsqmmr8UMQV3V\n\t0Ce5EuL85S9myCJCruUOdNg+tgBT5qZP99AjjALI=","Date":"Wed, 20 Jan 2021 13:55:19 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210119153047.468190-1-naush@raspberrypi.com>\n\t<20210119153047.468190-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210119153047.468190-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v12 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\" "}}]