[{"id":4787,"web_url":"https://patchwork.libcamera.org/comment/4787/","msgid":"<20200512003520.GI5830@pendragon.ideasonboard.com>","date":"2020-05-12T00:35:20","subject":"Re: [libcamera-devel] [PATCH 1/2] 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 Mon, May 11, 2020 at 11:01:49AM +0100, 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> Currently, the minimum framerate is dictated by the lowest available\n> exposure time in the tuning, and the maximum framerate is limited by\n> the sensor mode.\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> ---\n> src/ipa/raspberrypi/cam_helper.hpp | 1 +\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 +++++++++++++\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 13 +++++++++++++\n> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 13 +++++++++++++\n> src/ipa/raspberrypi/raspberrypi.cpp | 8 +++++++-\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++\n> 6 files changed, 49 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index 0c8aa29a..37281c78 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -74,6 +74,7 @@ public:\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) const = 0;\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> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index 35c6597c..d34a1f1f 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -45,11 +45,16 @@ class CamHelperImx219 : public CamHelper\n> {\n> public:\n> \tCamHelperImx219();\n> +\tuint32_t GetVBlanking(double exposure) const override;\n> \tuint32_t GainCode(double gain) const override;\n> \tdouble Gain(uint32_t gain_code) const override;\n> \tunsigned int MistrustFramesModeSwitch() const override;\n> \tbool SensorEmbeddedDataPresent() const override;\n> \tCamTransform GetOrientation() const override;\n> +\n> +private:\n> +\t/* Smallest difference between the frame length and integration time. */\n\nWorth mentioning that the unit is lines ?\n\n> +\tstatic constexpr int frameIntegrationDiff = 4;\n> };\n> \n> CamHelperImx219::CamHelperImx219()\n> @@ -61,6 +66,14 @@ CamHelperImx219::CamHelperImx219()\n> {\n> }\n> \n> +uint32_t CamHelperImx219::GetVBlanking(double exposure) const\n> +{\n> +\tuint32_t exposureLines = ExposureLines(exposure);\n> +\n> +\treturn std::max(mode_.height, exposureLines) -\n> +\t mode_.height + frameIntegrationDiff;\n> +}\n> +\n> uint32_t CamHelperImx219::GainCode(double gain) const\n> {\n> \treturn (uint32_t)(256 - 256 / gain);\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 69544456..242d9689 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -35,10 +35,15 @@ class CamHelperImx477 : public CamHelper\n> {\n> public:\n> \tCamHelperImx477();\n> +\tuint32_t GetVBlanking(double exposure) const override;\n> \tuint32_t GainCode(double gain) const override;\n> \tdouble Gain(uint32_t gain_code) const override;\n> \tbool SensorEmbeddedDataPresent() const override;\n> \tCamTransform GetOrientation() const override;\n> +\n> +private:\n> +\t/* Smallest difference between the frame length and integration time. */\n> +\tstatic constexpr int frameIntegrationDiff = 22;\n> };\n> \n> CamHelperImx477::CamHelperImx477()\n> @@ -46,6 +51,14 @@ CamHelperImx477::CamHelperImx477()\n> {\n> }\n> \n> +uint32_t CamHelperImx477::GetVBlanking(double exposure) const\n> +{\n> +\tuint32_t exposureLines = ExposureLines(exposure);\n> +\n> +\treturn std::max(mode_.height, exposureLines) -\n> +\t mode_.height + frameIntegrationDiff;\n> +}\n> +\n> uint32_t CamHelperImx477::GainCode(double gain) const\n> {\n> \treturn static_cast(1024 - 1024 / gain);\n> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> index 3dbcb164..ff37779c 100644\n> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> @@ -16,12 +16,17 @@ class CamHelperOv5647 : public CamHelper\n> {\n> public:\n> \tCamHelperOv5647();\n> +\tuint32_t GetVBlanking(double exposure) const override;\n> \tuint32_t GainCode(double gain) const override;\n> \tdouble Gain(uint32_t gain_code) const override;\n> \tvoid GetDelays(int &exposure_delay, int &gain_delay) const override;\n> \tunsigned int HideFramesModeSwitch() const override;\n> \tunsigned int MistrustFramesStartup() const override;\n> \tunsigned int MistrustFramesModeSwitch() const override;\n> +\n> +private:\n> +\t/* Smallest difference between the frame length and integration time. */\n> +\tstatic constexpr int frameIntegrationDiff = 4;\n> };\n> \n> /*\n> @@ -34,6 +39,14 @@ CamHelperOv5647::CamHelperOv5647()\n> {\n> }\n> \n> +uint32_t CamHelperOv5647::GetVBlanking(double exposure) const\n> +{\n> +\tuint32_t exposureLines = ExposureLines(exposure);\n> +\n> +\treturn std::max(mode_.height, exposureLines) -\n> +\t mode_.height + frameIntegrationDiff;\n> +}\n> +\n\nThe three implementations are identical, with the only parameters that\ncould vary being frameIntegrationDiff. Would it make sense to implement\nGetVBlanking in the base class, and expose frameIntegrationDiff in\nderived classes ? Or do you expect more variation in implementations in\nthe future ?\n\n> uint32_t CamHelperOv5647::GainCode(double gain) const\n> {\n> \treturn static_cast(gain * 16.0);\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 80dc77a3..104727ea 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -795,6 +795,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus)\n> \n> \tint32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);\n> \tint32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);\n> +\tint32_t vblanking = helper_->GetVBlanking(agcStatus->shutter_time);\n\nDoesn't this effectively hardcodes a variable frame rate ? Should there\nbe a maximum frame rate limit ?\n\n> \n> \tif (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {\n> \t\tLOG(IPARPI, Error) << \"Can't find analogue gain control\";\n> @@ -812,8 +813,13 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus)\n> \t\t\t << gain_code << \")\";\n> \n> \tControlList ctrls(unicam_ctrls_);\n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> +\t/*\n> +\t * VBLANK must be set before EXPOSURE as the former will adjust the\n> +\t * limits of the latter control.\n> +\t */\n> +\tctrls.set(V4L2_CID_VBLANK, vblanking);\n> \tctrls.set(V4L2_CID_EXPOSURE, exposure_lines);\n> +\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);\n> \top.controls.push_back(ctrls);\n> \tqueueFrameAction.emit(0, op);\n> }\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 16850819..e4c5d8f0 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1159,7 +1159,9 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData\n> \t\tif (!staggeredCtrl_) {\n> \t\t\tstaggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> \t\t\t\t\t { { V4L2_CID_ANALOGUE_GAIN, action.data[0] },\n> +\t\t\t\t\t { V4L2_CID_VBLANK, action.data[1] },\n> \t\t\t\t\t { V4L2_CID_EXPOSURE, action.data[1] } });\n> +\n> \t\t\tsensorMetadata_ = action.data[2];\n> \t\t}\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E543F603DD\n\tfor ;\n\tTue, 12 May 2020 02:35:28 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB48C33E;\n\tTue, 12 May 2020 02:35:27 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"v+Jie+1q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1589243728;\n\tbh=E53JJCzMEG3SHqc/imrll1IWALLexSYAdDiwq1HMQZY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v+Jie+1qUcmhs/mj0As2rB3eGSmGiT6c5Oux7US9YLEQqewqnmuLlAY5CpsmCOI2i\n\taTqYHW6Z12XTAi7fhBFOaRDjXZzDAbxzRqUSTe795E30lPVOkOqydMt2rUX4Ncjw7O\n\tnc+HYQfFK/4EJhwBz1IbIPynrWevzCDfkFO96wWA=","Date":"Tue, 12 May 2020 03:35:20 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200512003520.GI5830@pendragon.ideasonboard.com>","References":"<20200511100150.5205-1-naush@raspberrypi.com>\n\t<20200511100150.5205-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200511100150.5205-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] 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","X-List-Received-Date":"Tue, 12 May 2020 00:35:29 -0000"}}]