[{"id":14777,"web_url":"https://patchwork.libcamera.org/comment/14777/","msgid":"","date":"2021-01-26T10:01:03","subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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 Sun, Jan 24, 2021 at 02:05:04PM +0000, Naushir Patuck wrote:\n> The existing framerate/vblank calculations did not account for the\n> sensor mode limits. This commit extracts vblank limits from the sensor\n> v4l2 controls, and stores it in the camera modes structure.\n> \n> Exposure and vblank value calculations now use values clamped to the\n> sensor mode limits. The libcamera::controls::FrameDurations metadata\n> return values now reflect the minimum and maximum after clamping.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 16 +++---\n> src/ipa/raspberrypi/cam_helper.hpp | 5 +-\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-\n> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-\n> src/ipa/raspberrypi/controller/camera_mode.h | 2 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++------\n> 7 files changed, 49 insertions(+), 39 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index b7b8faf09c69..3e17255737b2 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const &cam_name)\n> \treturn nullptr;\n> }\n> \n> -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> -\t\t unsigned int frameIntegrationDiff)\n> -\t: parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),\n> +CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)\n> +\t: parser_(parser), initialized_(false),\n> \t frameIntegrationDiff_(frameIntegrationDiff)\n> {\n> }\n> @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\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 * minFrameDuration and maxFrameDuration will have been clamped to the\n\nMaybe \"minFrameDuration and maxFrameDuration are clamped by the caller\nbased on the limits for the active sensor mode\" to make clear who is\nresponsible for clamping those values ?\n\n> +\t * maximum allowable values in the sensor for this mode.\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> +\tframeLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> +\tframeLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\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> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index b1739a57e342..14d70112cb62 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -62,8 +62,7 @@ class CamHelper\n> {\n> public:\n> \tstatic CamHelper *Create(std::string const &cam_name);\n> -\tCamHelper(MdParser *parser, unsigned int maxFrameLength,\n> -\t\t unsigned int frameIntegrationDiff);\n> +\tCamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> \tvirtual ~CamHelper();\n> \tvoid SetCameraMode(const CameraMode &mode);\n> \tMdParser &Parser() const { return *parser_; }\n> @@ -86,8 +85,6 @@ protected:\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> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index 8688ec091c44..95b8e698fe3b 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -56,15 +56,13 @@ private:\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(), maxFrameLength, frameIntegrationDiff)\n> +\t: CamHelper(new MdParserImx219(), frameIntegrationDiff)\n> #else\n> -\t: CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> +\t: CamHelper(new MdParserRPi(), 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 5396131003f1..eaa7be16d20e 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -45,12 +45,10 @@ private:\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(), maxFrameLength, frameIntegrationDiff)\n> +\t: CamHelper(new MdParserImx477(), 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 2bd8a754f133..a7f417324048 100644\n> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> @@ -30,8 +30,6 @@ private:\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> @@ -40,7 +38,7 @@ private:\n> */\n> \n> CamHelperOv5647::CamHelperOv5647()\n> -\t: CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> +\t: CamHelper(new MdParserRPi(), frameIntegrationDiff)\n> {\n> }\n> \n> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h\n> index 920f11beb0b0..3baeadaca076 100644\n> --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> @@ -37,6 +37,8 @@ struct CameraMode {\n> \tdouble line_length;\n> \t// any camera transform *not* reflected already in the camera tuning\n> \tlibcamera::Transform transform;\n> +\t// minimum and maximum vblanking limits for this mode\n> +\tuint32_t vblank_min, vblank_max;\n> };\n> \n> #ifdef __cplusplus\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 5ccc7a6551f5..30ba9aef9d97 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -102,6 +102,7 @@ private:\n> \tvoid reportMetadata();\n> \tbool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);\n> \tvoid processStats(unsigned int bufferId);\n> +\tvoid applyFrameDurations(double minFrameDuration, double maxFrameDuration);\n> \tvoid applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> \tvoid applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> \tvoid applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> \t * the line length in pixels and the pixel rate.\n> \t */\n> \tmode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;\n> +\n> +\t/*\n> +\t * Set the vertical blanking (frame duration) limits for the mode to\n> +\t * ensure exposure and framerate calculations are clipped appropriately.\n> +\t */\n> +\tconst auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n> +\tmode_.vblank_min = itVblank->second.min().get();\n> +\tmode_.vblank_max = itVblank->second.max().get();\n\nThe trouble here is that sensorCtrls_ will not be updated after the\nsensor configuration has changed. It comes from the\nV4L2Device::controls() call in RPiCameraData::configureIPA(), and that\nfunction returns a cached copy.\n\nI really dislike the fact that V4L2 provides us with a control whose\nlimits change when the analog crop rectangle height changes, while the\nsensors usually have a fixed vertical total size limit. Wouldn't it be\nbetter to base the code on the vertical total size limit in libcamera,\ninstead of vertical blanking ? We would still have the issue of\nretrieving this value, and I can see multiple options:\n\n- Compute it once at initialization from vblank limits + analog crop\n- Extend V4L2 with a new vertical total size control and use it (that\n will be more work as we need to synchronize with the kernel changes)\n- Hardcode the value in a sensor database in libcamera, bypassing the\n kernel completly\n\nOne question that I'm not sure I can answer by myself is whether we can\nrely on the vertical total size limits being constant.\n\n> }\n> \n> void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tcontroller_.Initialise();\n> \t\tcontrollerInit_ = true;\n> \n> -\t\tminFrameDuration_ = defaultMinFrameDuration;\n> -\t\tmaxFrameDuration_ = defaultMaxFrameDuration;\n> +\t\t/* Supply initial values for frame durations. */\n> +\t\tapplyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);\n> \n> \t\t/* Supply initial values for gain and exposure. */\n> \t\tControlList ctrls(sensorCtrls_);\n> @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList &controls)\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 supports and\n> -\t\t\t * what the AGC exposure mode or manual shutter speed limits\n> -\t\t\t */\n> -\t\t\tlibcameraMetadata_.set(controls::FrameDurations,\n> -\t\t\t\t\t { static_cast(minFrameDuration_),\n> -\t\t\t\t\t\t static_cast(maxFrameDuration_) });\n> +\t\t\tapplyFrameDurations(frameDurations[0], frameDurations[1]);\n> \t\t\tbreak;\n> \t\t}\n> \n> @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> \t\t static_cast(awbStatus->gain_b * 1000));\n> }\n> \n> +void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration)\n> +{\n> +\tconst double minSensorFrameDuration = 1e-3 * (mode_.vblank_min + mode_.height) *\n> +\t\t\t\t\t mode_.line_length;\n> +\tconst double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max + mode_.height) *\n> +\t\t\t\t\t mode_.line_length;\n> +\t/*\n> +\t * This will only be applied once AGC recalculations occur.\n> +\t * The values may be clamped based on the sensor mode capabilities as well.\n> +\t */\n> +\tminFrameDuration_ = minFrameDuration ? minFrameDuration : defaultMaxFrameDuration;\n> +\tmaxFrameDuration_ = maxFrameDuration ? maxFrameDuration : defaultMinFrameDuration;\n> +\tminFrameDuration_ = std::clamp(minFrameDuration_,\n> +\t\t\t\t minSensorFrameDuration, maxSensorFrameDuration);\n> +\tmaxFrameDuration_ = std::clamp(maxFrameDuration_,\n> +\t\t\t\t minSensorFrameDuration, maxSensorFrameDuration);\n> +\n> +\t/* Return the validated limits out though metadata. */\n> +\tlibcameraMetadata_.set(controls::FrameDurations,\n> +\t\t\t { static_cast(minFrameDuration_),\n> +\t\t\t\t static_cast(maxFrameDuration_) });\n> +}\n> +\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\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 00C41BD808\n\tfor ;\n\tTue, 26 Jan 2021 10:01:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69A77682F2;\n\tTue, 26 Jan 2021 11:01:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41D79682D4\n\tfor ;\n\tTue, 26 Jan 2021 11:01:23 +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 B7CD32C1;\n\tTue, 26 Jan 2021 11:01:22 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MgccFskC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611655282;\n\tbh=5ovsIwm6aSg93KAMUBaHbqbqC4u54Jv3fRwNmFWbmhQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MgccFskCP2YA7De/95R8JjiXeHVIvd9vnRfpG2xRMhX1BAqdYnFJFTtWsITSvJ2iy\n\tMMjn0TFSzg1miSm8ZcLp3rkEw4eLu4DiX9+JNlzz5yhlReva452tfz+3nX8N7OezRu\n\tzjitTLJbCOL5gydvHDjs9rsUMCohGAp/rSaD8is8=","Date":"Tue, 26 Jan 2021 12:01:03 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210124140506.786503-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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":14778,"web_url":"https://patchwork.libcamera.org/comment/14778/","msgid":"","date":"2021-01-26T12:51:12","subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your review feedback.\n\nOn Tue, 26 Jan 2021 at 10:01, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Sun, Jan 24, 2021 at 02:05:04PM +0000, Naushir Patuck wrote:\n> > The existing framerate/vblank calculations did not account for the\n> > sensor mode limits. This commit extracts vblank limits from the sensor\n> > v4l2 controls, and stores it in the camera modes structure.\n> >\n> > Exposure and vblank value calculations now use values clamped to the\n> > sensor mode limits. The libcamera::controls::FrameDurations metadata\n> > return values now reflect the minimum and maximum after clamping.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 16 +++---\n> > src/ipa/raspberrypi/cam_helper.hpp | 5 +-\n> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-\n> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-\n> > src/ipa/raspberrypi/controller/camera_mode.h | 2 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++------\n> > 7 files changed, 49 insertions(+), 39 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > index b7b8faf09c69..3e17255737b2 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const\n> &cam_name)\n> > return nullptr;\n> > }\n> >\n> > -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > - unsigned int frameIntegrationDiff)\n> > - : parser_(parser), initialized_(false),\n> maxFrameLength_(maxFrameLength),\n> > +CamHelper::CamHelper(MdParser *parser, unsigned int\n> frameIntegrationDiff)\n> > + : parser_(parser), initialized_(false),\n> > frameIntegrationDiff_(frameIntegrationDiff)\n> > {\n> > }\n> > @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure,\n> double minFrameDuration,\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> > + * minFrameDuration and maxFrameDuration will have been clamped to\n> the\n>\n> Maybe \"minFrameDuration and maxFrameDuration are clamped by the caller\n> based on the limits for the active sensor mode\" to make clear who is\n> responsible for clamping those values ?\n>\n\nThat wording sounds good to me.\n\n\n>\n> > + * maximum allowable values in the sensor for this mode.\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> > + frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> > + frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> > +\n> > /*\n> > * Limit the exposure to the maximum frame duration requested, and\n> > * re-calculate if it has been clipped.\n> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> > index b1739a57e342..14d70112cb62 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -62,8 +62,7 @@ class CamHelper\n> > {\n> > public:\n> > static CamHelper *Create(std::string const &cam_name);\n> > - CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > - unsigned int frameIntegrationDiff);\n> > + CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> > virtual ~CamHelper();\n> > void SetCameraMode(const CameraMode &mode);\n> > MdParser &Parser() const { return *parser_; }\n> > @@ -86,8 +85,6 @@ protected:\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> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > index 8688ec091c44..95b8e698fe3b 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > @@ -56,15 +56,13 @@ private:\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(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserImx219(), frameIntegrationDiff)\n> > #else\n> > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserRPi(), 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 5396131003f1..eaa7be16d20e 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -45,12 +45,10 @@ private:\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(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserImx477(), 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 2bd8a754f133..a7f417324048 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > @@ -30,8 +30,6 @@ private:\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> > @@ -40,7 +38,7 @@ private:\n> > */\n> >\n> > CamHelperOv5647::CamHelperOv5647()\n> > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserRPi(), frameIntegrationDiff)\n> > {\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> b/src/ipa/raspberrypi/controller/camera_mode.h\n> > index 920f11beb0b0..3baeadaca076 100644\n> > --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> > +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> > @@ -37,6 +37,8 @@ struct CameraMode {\n> > double line_length;\n> > // any camera transform *not* reflected already in the camera\n> tuning\n> > libcamera::Transform transform;\n> > + // minimum and maximum vblanking limits for this mode\n> > + uint32_t vblank_min, vblank_max;\n> > };\n> >\n> > #ifdef __cplusplus\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 5ccc7a6551f5..30ba9aef9d97 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -102,6 +102,7 @@ private:\n> > void reportMetadata();\n> > bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus\n> &deviceStatus);\n> > void processStats(unsigned int bufferId);\n> > + void applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration);\n> > void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls);\n> > void applyAWB(const struct AwbStatus *awbStatus, ControlList\n> &ctrls);\n> > void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> > @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo\n> &sensorInfo)\n> > * the line length in pixels and the pixel rate.\n> > */\n> > mode_.line_length = 1e9 * sensorInfo.lineLength /\n> sensorInfo.pixelRate;\n> > +\n> > + /*\n> > + * Set the vertical blanking (frame duration) limits for the mode\n> to\n> > + * ensure exposure and framerate calculations are clipped\n> appropriately.\n> > + */\n> > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n> > + mode_.vblank_min = itVblank->second.min().get();\n> > + mode_.vblank_max = itVblank->second.max().get();\n>\n> The trouble here is that sensorCtrls_ will not be updated after the\n> sensor configuration has changed. It comes from the\n> V4L2Device::controls() call in RPiCameraData::configureIPA(), and that\n> function returns a cached copy.\n>\n\nI don't exactly know all of this in very much detail, so apologies if the\nfollowing statements sound basic, or don't make much sense :-)\n\nIn RPiCameraData::configureIPA(), would it be possible to \"update\" the\ncached values of V4L2Device::controls_ by\ncalling V4L2Device::listControls(), or maybe a new method such\nas V4L2Device::updateControls(). This only needs to be called once\nin PipelineHandlerRPi::configure(), after the sensor mode has been set.\nThis way, the VBLANK control will have up-to-date numbers for the IPA to\nuse.\n\nAlternatively, CameraSensorInfo gets populated in\nRPiCameraData::configureIPA() with a call\nto CameraSensor::sensorInfo(). Looks like this directly queries the device\nfor controls via V4L2Device::getControls(). Perhaps we should re-visit\nJacopo's suggestion and add the min/max vblank values to the mode?\n\n\n>\n> I really dislike the fact that V4L2 provides us with a control whose\n> limits change when the analog crop rectangle height changes, while the\n> sensors usually have a fixed vertical total size limit. Wouldn't it be\n> better to base the code on the vertical total size limit in libcamera,\n> instead of vertical blanking ?\n\n\nBy vertical total size limit, I assume you mean what sensors generally call\nvts or frame length? If so, then yes, it does make sense. In fact, you\ncan see from my patch, the first thing I do is convert from vblank to frame\nlength for all of my calculations.\n\n\n> We would still have the issue of\n> retrieving this value, and I can see multiple options:\n>\n> - Compute it once at initialization from vblank limits + analog crop\n>\n\nThis is assuming that vblank + crop height is always going to be equal to\nframe length? We need to think if that is strictly true for all sensors,\nI'm not sure.\n\n\n\n> - Extend V4L2 with a new vertical total size control and use it (that\n> will be more work as we need to synchronize with the kernel changes)\n>\n\nI expect this would have the longest lead time :-)\n\n\n> - Hardcode the value in a sensor database in libcamera, bypassing the\n> kernel completly\n>\n\nThis is possible, but...\n\n\n>\n> One question that I'm not sure I can answer by myself is whether we can\n> rely on the vertical total size limits being constant.\n>\n\n... I don't think you can assume total vertical size (frame length) is\ngoing to be the same for all modes a sensor advertises. In this case, the\nsensor database would have to know about every mode available in the kernel\ndriver. This is something we moved away from with the Raspberry Pi\nCamHelper, so I doubt you want to re-introduce that link.\n\nMy feeling is that we could work something out by forcing a \"refresh\" of\nthe cached controls on every configure, and I think that should cover our\n(or at least Raspberry Pi's in this particular instance) usage.\n\nLet me know your thoughts.\n\nRegards,\nNaush\n\n\n\n> > }\n> >\n> > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > controller_.Initialise();\n> > controllerInit_ = true;\n> >\n> > - minFrameDuration_ = defaultMinFrameDuration;\n> > - maxFrameDuration_ = defaultMaxFrameDuration;\n> > + /* Supply initial values for frame durations. */\n> > + applyFrameDurations(defaultMinFrameDuration,\n> defaultMaxFrameDuration);\n> >\n> > /* Supply initial values for gain and exposure. */\n> > ControlList ctrls(sensorCtrls_);\n> > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\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> > - /*\n> > - * \\todo The values returned in the metadata below\n> must be\n> > - * correctly clipped by what the sensor mode\n> supports and\n> > - * what the AGC exposure mode or manual shutter\n> speed limits\n> > - */\n> > - libcameraMetadata_.set(controls::FrameDurations,\n> > - {\n> static_cast(minFrameDuration_),\n> > -\n> static_cast(maxFrameDuration_) });\n> > + applyFrameDurations(frameDurations[0],\n> frameDurations[1]);\n> > break;\n> > }\n> >\n> > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> > static_cast(awbStatus->gain_b * 1000));\n> > }\n> >\n> > +void IPARPi::applyFrameDurations(double minFrameDuration, double\n> maxFrameDuration)\n> > +{\n> > + const double minSensorFrameDuration = 1e-3 * (mode_.vblank_min +\n> mode_.height) *\n> > + mode_.line_length;\n> > + const double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max +\n> mode_.height) *\n> > + mode_.line_length;\n> > + /*\n> > + * This will only be applied once AGC recalculations occur.\n> > + * The values may be clamped based on the sensor mode capabilities\n> as well.\n> > + */\n> > + minFrameDuration_ = minFrameDuration ? minFrameDuration :\n> defaultMaxFrameDuration;\n> > + maxFrameDuration_ = maxFrameDuration ? maxFrameDuration :\n> defaultMinFrameDuration;\n> > + minFrameDuration_ = std::clamp(minFrameDuration_,\n> > + minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > + minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > +\n> > + /* Return the validated limits out though metadata. */\n> > + libcameraMetadata_.set(controls::FrameDurations,\n> > + { static_cast(minFrameDuration_),\n> > + static_cast(maxFrameDuration_)\n> });\n> > +}\n> > +\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\n> > {\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3EBE5BD808\n\tfor ;\n\tTue, 26 Jan 2021 12:51:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A34E3682FB;\n\tTue, 26 Jan 2021 13:51:31 +0100 (CET)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6E7DA6010B\n\tfor ;\n\tTue, 26 Jan 2021 13:51:29 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id b2so536895lfq.0\n\tfor ;\n\tTue, 26 Jan 2021 04:51:29 -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=\"FjrCKf0h\"; 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=JHZ8d+u9VdDpunjiS9bejZJTp5V/6L5ZT81uywxOQss=;\n\tb=FjrCKf0hm6z9Jm24DZZomg7gs0ZgeNtI+1oBvunAyiYy24m+n7gT5o+t+mZb4rhcX3\n\tIuKJSDGQn+jk5hXyLEy9TaVNtlImtH/OQF3zDO5x9shqdjOU45y9+5qTgipu+JDxwER1\n\t9gO00mKlPTK9Qsoe1aB+6KEwlsDlVRWl++d0bHEPRpQDelY4To5daTdGMsSVNq9Ew23/\n\t9mKb+FOI7LFu/UHbRMflk2i06rl2JpPovQnwxCfWIHbWAVj4nhKaaT+75Yw+5iop2g1z\n\t/WceqlARdXbOqiwxEMD+W+y6UsihT1xzbNQWyiO1nohIDySoG0pytufmqntn4CRGGy3g\n\tAQMA==","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=JHZ8d+u9VdDpunjiS9bejZJTp5V/6L5ZT81uywxOQss=;\n\tb=VNAFhZ5nhX1TEbXkhpwL8y1LLZT+TD2fMIYnt7EWJu49dT1qN2WWpGGGiXl8myeDgz\n\tNjCumErXRBnQC+JcRlErKCggArRNYSFMXN8Ep2ziy6ksPju1OY1b2wR+GIGTiWiUeWvc\n\t3coV+Dv8RTce1lWGyL0a38rU2wt/CkVWpdxeomTU9/LC3wtcixO4WjxPBNYxjCa9ZX8T\n\tSuukmKM4V/0NBCqGT4CDdCgPM09T9wLGlX7XmHY/gjd81E8+DMhuyuD8v5up1d1+yySb\n\tzLTF+46hkmR0Tx2B548h6wnekHFe21Pq6/CcuHp3gOGSSqxdZz1TJrtulW+Etr4SiLA8\n\trCFw==","X-Gm-Message-State":"AOAM533m1UZ/VmYgIfubxiS/SH4NQt3jMjLOBuCgw6Ig2jc0j8faLs55\n\tFxPBFubi33eTnvb8GCyZV35lxnCJQBP5AN1L0ha09A==","X-Google-Smtp-Source":"ABdhPJwsVaFE+Da6GHybsE/3pe5n302GSJIeZpgO+QDq2sP4r4qy8RaNdYqiAPi63qTl74MifrOTdDlljyTuP+6ritc=","X-Received":"by 2002:a19:8805:: with SMTP id k5mr2549128lfd.567.1611665488567;\n\tTue, 26 Jan 2021 04:51:28 -0800 (PST)","MIME-Version":"1.0","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-2-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 26 Jan 2021 12:51:12 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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=\"===============0608908698025067831==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14785,"web_url":"https://patchwork.libcamera.org/comment/14785/","msgid":"<20210126134459.zcwgiklzqgaahbr2@uno.localdomain>","date":"2021-01-26T13:44:59","subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush, Laurent,\n\n one question on the patch and one more input on the below\n discussion on where to retrieve vblank limits from\n\nOn Tue, Jan 26, 2021 at 12:51:12PM +0000, Naushir Patuck wrote:\n> Hi Laurent,\n>\n> Thank you for your review feedback.\n>\n> On Tue, 26 Jan 2021 at 10:01, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > On Sun, Jan 24, 2021 at 02:05:04PM +0000, Naushir Patuck wrote:\n> > > The existing framerate/vblank calculations did not account for the\n> > > sensor mode limits. This commit extracts vblank limits from the sensor\n> > > v4l2 controls, and stores it in the camera modes structure.\n> > >\n> > > Exposure and vblank value calculations now use values clamped to the\n> > > sensor mode limits. The libcamera::controls::FrameDurations metadata\n> > > return values now reflect the minimum and maximum after clamping.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper.cpp | 16 +++---\n> > > src/ipa/raspberrypi/cam_helper.hpp | 5 +-\n> > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--\n> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-\n> > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-\n> > > src/ipa/raspberrypi/controller/camera_mode.h | 2 +\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++------\n> > > 7 files changed, 49 insertions(+), 39 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> > b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index b7b8faf09c69..3e17255737b2 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const\n> > &cam_name)\n> > > return nullptr;\n> > > }\n> > >\n> > > -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > > - unsigned int frameIntegrationDiff)\n> > > - : parser_(parser), initialized_(false),\n> > maxFrameLength_(maxFrameLength),\n> > > +CamHelper::CamHelper(MdParser *parser, unsigned int\n> > frameIntegrationDiff)\n> > > + : parser_(parser), initialized_(false),\n> > > frameIntegrationDiff_(frameIntegrationDiff)\n> > > {\n> > > }\n> > > @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure,\n> > double minFrameDuration,\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> > > + * minFrameDuration and maxFrameDuration will have been clamped to\n> > the\n> >\n> > Maybe \"minFrameDuration and maxFrameDuration are clamped by the caller\n> > based on the limits for the active sensor mode\" to make clear who is\n> > responsible for clamping those values ?\n> >\n>\n> That wording sounds good to me.\n>\n>\n> >\n> > > + * maximum allowable values in the sensor for this mode.\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> > > + frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> > > + frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> > > +\n> > > /*\n> > > * Limit the exposure to the maximum frame duration requested, and\n> > > * re-calculate if it has been clipped.\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> > b/src/ipa/raspberrypi/cam_helper.hpp\n> > > index b1739a57e342..14d70112cb62 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > @@ -62,8 +62,7 @@ class CamHelper\n> > > {\n> > > public:\n> > > static CamHelper *Create(std::string const &cam_name);\n> > > - CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > > - unsigned int frameIntegrationDiff);\n> > > + CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> > > virtual ~CamHelper();\n> > > void SetCameraMode(const CameraMode &mode);\n> > > MdParser &Parser() const { return *parser_; }\n> > > @@ -86,8 +85,6 @@ protected:\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> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > index 8688ec091c44..95b8e698fe3b 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > @@ -56,15 +56,13 @@ private:\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(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > + : CamHelper(new MdParserImx219(), frameIntegrationDiff)\n> > > #else\n> > > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > + : CamHelper(new MdParserRPi(), 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 5396131003f1..eaa7be16d20e 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > @@ -45,12 +45,10 @@ private:\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(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > + : CamHelper(new MdParserImx477(), 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 2bd8a754f133..a7f417324048 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > @@ -30,8 +30,6 @@ private:\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> > > @@ -40,7 +38,7 @@ private:\n> > > */\n> > >\n> > > CamHelperOv5647::CamHelperOv5647()\n> > > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > + : CamHelper(new MdParserRPi(), frameIntegrationDiff)\n> > > {\n> > > }\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> > b/src/ipa/raspberrypi/controller/camera_mode.h\n> > > index 920f11beb0b0..3baeadaca076 100644\n> > > --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> > > +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> > > @@ -37,6 +37,8 @@ struct CameraMode {\n> > > double line_length;\n> > > // any camera transform *not* reflected already in the camera\n> > tuning\n> > > libcamera::Transform transform;\n> > > + // minimum and maximum vblanking limits for this mode\n> > > + uint32_t vblank_min, vblank_max;\n> > > };\n> > >\n> > > #ifdef __cplusplus\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 5ccc7a6551f5..30ba9aef9d97 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -102,6 +102,7 @@ private:\n> > > void reportMetadata();\n> > > bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus\n> > &deviceStatus);\n> > > void processStats(unsigned int bufferId);\n> > > + void applyFrameDurations(double minFrameDuration, double\n> > maxFrameDuration);\n> > > void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> > &ctrls);\n> > > void applyAWB(const struct AwbStatus *awbStatus, ControlList\n> > &ctrls);\n> > > void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> > > @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo\n> > &sensorInfo)\n> > > * the line length in pixels and the pixel rate.\n> > > */\n> > > mode_.line_length = 1e9 * sensorInfo.lineLength /\n> > sensorInfo.pixelRate;\n> > > +\n> > > + /*\n> > > + * Set the vertical blanking (frame duration) limits for the mode\n> > to\n> > > + * ensure exposure and framerate calculations are clipped\n> > appropriately.\n> > > + */\n> > > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n> > > + mode_.vblank_min = itVblank->second.min().get();\n> > > + mode_.vblank_max = itVblank->second.max().get();\n> >\n> > The trouble here is that sensorCtrls_ will not be updated after the\n> > sensor configuration has changed. It comes from the\n> > V4L2Device::controls() call in RPiCameraData::configureIPA(), and that\n> > function returns a cached copy.\n> >\n>\n> I don't exactly know all of this in very much detail, so apologies if the\n> following statements sound basic, or don't make much sense :-)\n>\n> In RPiCameraData::configureIPA(), would it be possible to \"update\" the\n> cached values of V4L2Device::controls_ by\n> calling V4L2Device::listControls(), or maybe a new method such\n> as V4L2Device::updateControls(). This only needs to be called once\n> in PipelineHandlerRPi::configure(), after the sensor mode has been set.\n> This way, the VBLANK control will have up-to-date numbers for the IPA to\n> use.\n>\n> Alternatively, CameraSensorInfo gets populated in\n> RPiCameraData::configureIPA() with a call\n> to CameraSensor::sensorInfo(). Looks like this directly queries the device\n> for controls via V4L2Device::getControls(). Perhaps we should re-visit\n> Jacopo's suggestion and add the min/max vblank values to the mode?\n>\n>\n> >\n> > I really dislike the fact that V4L2 provides us with a control whose\n> > limits change when the analog crop rectangle height changes, while the\n> > sensors usually have a fixed vertical total size limit. Wouldn't it be\n> > better to base the code on the vertical total size limit in libcamera,\n> > instead of vertical blanking ?\n>\n>\n> By vertical total size limit, I assume you mean what sensors generally call\n> vts or frame length? If so, then yes, it does make sense. In fact, you\n> can see from my patch, the first thing I do is convert from vblank to frame\n> length for all of my calculations.\n>\n>\n\nLet me summarize your understandings:\n- You need the VBLANK limits to clamp frame durations in the sensor\n limits\n- VBLANK min and max are combined with the current mode height and\n line length to get the min and max frame sizes, from where the min\n and max frame duration limits are calculated\n- min frame size = (mode_.height + frameIntegrationDiff) * mode_.line_length\n- max frame size = (mode_.height + max vblank) * mode_.line_length\n\nFrom the CameraSensorInfo you already know the visibile height and the\nline_length. Wouldn't be enough to add to that structure\n - maxFrameHeight = (mode_.height + max vblank)\n - frameIntegrationDiff\n This is sensor property and will be made available in the\n sensor database. You can keep it in the IPA as long as no\n sensor database is available\n\nTo sum it up: is it enough for you to pass to the IPA the maximum\nframe length (mode_.height + max vblank) in the CameraSensorClass ?\n\n\n> > We would still have the issue of\n> > retrieving this value, and I can see multiple options:\n> >\n> > - Compute it once at initialization from vblank limits + analog crop\n> >\n>\n> This is assuming that vblank + crop height is always going to be equal to\n> frame length? We need to think if that is strictly true for all sensors,\n> I'm not sure.\n>\n>\n>\n> > - Extend V4L2 with a new vertical total size control and use it (that\n> > will be more work as we need to synchronize with the kernel changes)\n> >\n>\n> I expect this would have the longest lead time :-)\n>\n>\n> > - Hardcode the value in a sensor database in libcamera, bypassing the\n> > kernel completly\n> >\n>\n> This is possible, but...\n>\n>\n> >\n> > One question that I'm not sure I can answer by myself is whether we can\n> > rely on the vertical total size limits being constant.\n> >\n>\n> ... I don't think you can assume total vertical size (frame length) is\n> going to be the same for all modes a sensor advertises. In this case, the\n> sensor database would have to know about every mode available in the kernel\n> driver. This is something we moved away from with the Raspberry Pi\n> CamHelper, so I doubt you want to re-introduce that link.\n>\n> My feeling is that we could work something out by forcing a \"refresh\" of\n> the cached controls on every configure, and I think that should cover our\n> (or at least Raspberry Pi's in this particular instance) usage.\n>\n> Let me know your thoughts.\n>\n> Regards,\n> Naush\n>\n>\n>\n> > > }\n> > >\n> > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo\n> > &sensorInfo,\n> > > controller_.Initialise();\n> > > controllerInit_ = true;\n> > >\n> > > - minFrameDuration_ = defaultMinFrameDuration;\n> > > - maxFrameDuration_ = defaultMaxFrameDuration;\n> > > + /* Supply initial values for frame durations. */\n> > > + applyFrameDurations(defaultMinFrameDuration,\n> > defaultMaxFrameDuration);\n> > >\n> > > /* Supply initial values for gain and exposure. */\n> > > ControlList ctrls(sensorCtrls_);\n> > > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList\n> > &controls)\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> > > - /*\n> > > - * \\todo The values returned in the metadata below\n> > must be\n> > > - * correctly clipped by what the sensor mode\n> > supports and\n> > > - * what the AGC exposure mode or manual shutter\n> > speed limits\n> > > - */\n> > > - libcameraMetadata_.set(controls::FrameDurations,\n> > > - {\n> > static_cast(minFrameDuration_),\n> > > -\n> > static_cast(maxFrameDuration_) });\n> > > + applyFrameDurations(frameDurations[0],\n> > frameDurations[1]);\n> > > break;\n> > > }\n> > >\n> > > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus\n> > *awbStatus, ControlList &ctrls)\n> > > static_cast(awbStatus->gain_b * 1000));\n> > > }\n> > >\n> > > +void IPARPi::applyFrameDurations(double minFrameDuration, double\n> > maxFrameDuration)\n> > > +{\n> > > + const double minSensorFrameDuration = 1e-3 * (mode_.vblank_min +\n> > mode_.height) *\n> > > + mode_.line_length;\n> > > + const double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max +\n> > mode_.height) *\n> > > + mode_.line_length;\n\nTo transform the frame size expressed in pixels:\n (mode_.vblank + mode_.height) * mode_.line_length\n\nin a duration expressed in seconds sub-units, don't you need to use\nthe pixel clock (which is available in CameraSensorInfo afair)\n frameDuration (usec) = frame size (pixels)\n / pixel rate (pixels/usec)\n\nThanks\n j\n\n> > > + /*\n> > > + * This will only be applied once AGC recalculations occur.\n> > > + * The values may be clamped based on the sensor mode capabilities\n> > as well.\n> > > + */\n> > > + minFrameDuration_ = minFrameDuration ? minFrameDuration :\n> > defaultMaxFrameDuration;\n> > > + maxFrameDuration_ = maxFrameDuration ? maxFrameDuration :\n> > defaultMinFrameDuration;\n> > > + minFrameDuration_ = std::clamp(minFrameDuration_,\n> > > + minSensorFrameDuration,\n> > maxSensorFrameDuration);\n> > > + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > > + minSensorFrameDuration,\n> > maxSensorFrameDuration);\n> > > +\n> > > + /* Return the validated limits out though metadata. */\n> > > + libcameraMetadata_.set(controls::FrameDurations,\n> > > + { static_cast(minFrameDuration_),\n> > > + static_cast(maxFrameDuration_)\n> > });\n> > > +}\n> > > +\n> > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> > &ctrls)\n> > > {\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >\n\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 39128C0F2B\n\tfor ;\n\tTue, 26 Jan 2021 13:44:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BCECB6830C;\n\tTue, 26 Jan 2021 14:44:40 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 50DDF682EC\n\tfor ;\n\tTue, 26 Jan 2021 14:44:40 +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 relay10.mail.gandi.net (Postfix) with ESMTPSA id 91A3F240013;\n\tTue, 26 Jan 2021 13:44:39 +0000 (UTC)"],"Date":"Tue, 26 Jan 2021 14:44:59 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210126134459.zcwgiklzqgaahbr2@uno.localdomain>","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-2-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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":14786,"web_url":"https://patchwork.libcamera.org/comment/14786/","msgid":"","date":"2021-01-26T14:12:50","subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nOn Tue, 26 Jan 2021 at 13:44, Jacopo Mondi wrote:\n\n> Hi Naush, Laurent,\n>\n> one question on the patch and one more input on the below\n> discussion on where to retrieve vblank limits from\n>\n> On Tue, Jan 26, 2021 at 12:51:12PM +0000, Naushir Patuck wrote:\n> > Hi Laurent,\n> >\n> > Thank you for your review feedback.\n> >\n> > On Tue, 26 Jan 2021 at 10:01, Laurent Pinchart <\n> > laurent.pinchart@ideasonboard.com> wrote:\n> >\n> > > Hi Naush,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Sun, Jan 24, 2021 at 02:05:04PM +0000, Naushir Patuck wrote:\n> > > > The existing framerate/vblank calculations did not account for the\n> > > > sensor mode limits. This commit extracts vblank limits from the\n> sensor\n> > > > v4l2 controls, and stores it in the camera modes structure.\n> > > >\n> > > > Exposure and vblank value calculations now use values clamped to the\n> > > > sensor mode limits. The libcamera::controls::FrameDurations metadata\n> > > > return values now reflect the minimum and maximum after clamping.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: David Plowman \n> > > > ---\n> > > > src/ipa/raspberrypi/cam_helper.cpp | 16 +++---\n> > > > src/ipa/raspberrypi/cam_helper.hpp | 5 +-\n> > > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--\n> > > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-\n> > > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-\n> > > > src/ipa/raspberrypi/controller/camera_mode.h | 2 +\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 51\n> ++++++++++++++------\n> > > > 7 files changed, 49 insertions(+), 39 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> > > b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > index b7b8faf09c69..3e17255737b2 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const\n> > > &cam_name)\n> > > > return nullptr;\n> > > > }\n> > > >\n> > > > -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > > > - unsigned int frameIntegrationDiff)\n> > > > - : parser_(parser), initialized_(false),\n> > > maxFrameLength_(maxFrameLength),\n> > > > +CamHelper::CamHelper(MdParser *parser, unsigned int\n> > > frameIntegrationDiff)\n> > > > + : parser_(parser), initialized_(false),\n> > > > frameIntegrationDiff_(frameIntegrationDiff)\n> > > > {\n> > > > }\n> > > > @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double\n> &exposure,\n> > > double minFrameDuration,\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> > > > + * minFrameDuration and maxFrameDuration will have been\n> clamped to\n> > > the\n> > >\n> > > Maybe \"minFrameDuration and maxFrameDuration are clamped by the caller\n> > > based on the limits for the active sensor mode\" to make clear who is\n> > > responsible for clamping those values ?\n> > >\n> >\n> > That wording sounds good to me.\n> >\n> >\n> > >\n> > > > + * maximum allowable values in the sensor for this mode.\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> > > > + frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> > > > + frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> > > > +\n> > > > /*\n> > > > * Limit the exposure to the maximum frame duration requested,\n> and\n> > > > * re-calculate if it has been clipped.\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> > > b/src/ipa/raspberrypi/cam_helper.hpp\n> > > > index b1739a57e342..14d70112cb62 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > > @@ -62,8 +62,7 @@ class CamHelper\n> > > > {\n> > > > public:\n> > > > static CamHelper *Create(std::string const &cam_name);\n> > > > - CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > > > - unsigned int frameIntegrationDiff);\n> > > > + CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> > > > virtual ~CamHelper();\n> > > > void SetCameraMode(const CameraMode &mode);\n> > > > MdParser &Parser() const { return *parser_; }\n> > > > @@ -86,8 +85,6 @@ protected:\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> > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > index 8688ec091c44..95b8e698fe3b 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > @@ -56,15 +56,13 @@ private:\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(), maxFrameLength,\n> > > frameIntegrationDiff)\n> > > > + : CamHelper(new MdParserImx219(), frameIntegrationDiff)\n> > > > #else\n> > > > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> > > frameIntegrationDiff)\n> > > > + : CamHelper(new MdParserRPi(), 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 5396131003f1..eaa7be16d20e 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > > @@ -45,12 +45,10 @@ private:\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(), maxFrameLength,\n> > > frameIntegrationDiff)\n> > > > + : CamHelper(new MdParserImx477(), 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 2bd8a754f133..a7f417324048 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > > @@ -30,8 +30,6 @@ private:\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> > > > @@ -40,7 +38,7 @@ private:\n> > > > */\n> > > >\n> > > > CamHelperOv5647::CamHelperOv5647()\n> > > > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> > > frameIntegrationDiff)\n> > > > + : CamHelper(new MdParserRPi(), frameIntegrationDiff)\n> > > > {\n> > > > }\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> > > b/src/ipa/raspberrypi/controller/camera_mode.h\n> > > > index 920f11beb0b0..3baeadaca076 100644\n> > > > --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> > > > +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> > > > @@ -37,6 +37,8 @@ struct CameraMode {\n> > > > double line_length;\n> > > > // any camera transform *not* reflected already in the camera\n> > > tuning\n> > > > libcamera::Transform transform;\n> > > > + // minimum and maximum vblanking limits for this mode\n> > > > + uint32_t vblank_min, vblank_max;\n> > > > };\n> > > >\n> > > > #ifdef __cplusplus\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 5ccc7a6551f5..30ba9aef9d97 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -102,6 +102,7 @@ private:\n> > > > void reportMetadata();\n> > > > bool parseEmbeddedData(unsigned int bufferId, struct\n> DeviceStatus\n> > > &deviceStatus);\n> > > > void processStats(unsigned int bufferId);\n> > > > + void applyFrameDurations(double minFrameDuration, double\n> > > maxFrameDuration);\n> > > > void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> > > &ctrls);\n> > > > void applyAWB(const struct AwbStatus *awbStatus, ControlList\n> > > &ctrls);\n> > > > void applyDG(const struct AgcStatus *dgStatus, ControlList\n> &ctrls);\n> > > > @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo\n> > > &sensorInfo)\n> > > > * the line length in pixels and the pixel rate.\n> > > > */\n> > > > mode_.line_length = 1e9 * sensorInfo.lineLength /\n> > > sensorInfo.pixelRate;\n> > > > +\n> > > > + /*\n> > > > + * Set the vertical blanking (frame duration) limits for the\n> mode\n> > > to\n> > > > + * ensure exposure and framerate calculations are clipped\n> > > appropriately.\n> > > > + */\n> > > > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n> > > > + mode_.vblank_min = itVblank->second.min().get();\n> > > > + mode_.vblank_max = itVblank->second.max().get();\n> > >\n> > > The trouble here is that sensorCtrls_ will not be updated after the\n> > > sensor configuration has changed. It comes from the\n> > > V4L2Device::controls() call in RPiCameraData::configureIPA(), and that\n> > > function returns a cached copy.\n> > >\n> >\n> > I don't exactly know all of this in very much detail, so apologies if the\n> > following statements sound basic, or don't make much sense :-)\n> >\n> > In RPiCameraData::configureIPA(), would it be possible to \"update\" the\n> > cached values of V4L2Device::controls_ by\n> > calling V4L2Device::listControls(), or maybe a new method such\n> > as V4L2Device::updateControls(). This only needs to be called once\n> > in PipelineHandlerRPi::configure(), after the sensor mode has been set.\n> > This way, the VBLANK control will have up-to-date numbers for the IPA to\n> > use.\n> >\n> > Alternatively, CameraSensorInfo gets populated in\n> > RPiCameraData::configureIPA() with a call\n> > to CameraSensor::sensorInfo(). Looks like this directly queries the\n> device\n> > for controls via V4L2Device::getControls(). Perhaps we should re-visit\n> > Jacopo's suggestion and add the min/max vblank values to the mode?\n> >\n> >\n> > >\n> > > I really dislike the fact that V4L2 provides us with a control whose\n> > > limits change when the analog crop rectangle height changes, while the\n> > > sensors usually have a fixed vertical total size limit. Wouldn't it be\n> > > better to base the code on the vertical total size limit in libcamera,\n> > > instead of vertical blanking ?\n> >\n> >\n> > By vertical total size limit, I assume you mean what sensors generally\n> call\n> > vts or frame length? If so, then yes, it does make sense. In fact, you\n> > can see from my patch, the first thing I do is convert from vblank to\n> frame\n> > length for all of my calculations.\n> >\n> >\n>\n> Let me summarize your understandings:\n> - You need the VBLANK limits to clamp frame durations in the sensor\n> limits\n> - VBLANK min and max are combined with the current mode height and\n> line length to get the min and max frame sizes, from where the min\n> and max frame duration limits are calculated\n> - min frame size = (mode_.height + frameIntegrationDiff) *\n> mode_.line_length\n> - max frame size = (mode_.height + max vblank) * mode_.line_length\n>\n> From the CameraSensorInfo you already know the visibile height and the\n> line_length. Wouldn't be enough to add to that structure\n> - maxFrameHeight = (mode_.height + max vblank)\n> - frameIntegrationDiff\n> This is sensor property and will be made available in the\n> sensor database. You can keep it in the IPA as long as no\n> sensor database is available\n>\n> To sum it up: is it enough for you to pass to the IPA the maximum\n> frame length (mode_.height + max vblank) in the CameraSensorClass ?\n>\n\nThis is *mostly* correct. max frame size as calculated above is fine.\nHowever, I do also need min frame size. Your calculation above assumes\nmin_vblank = 0. This may not be the case always, so min frame size must be\ncomputed and presented in CameraSensorInfo.\n\n\n\n>\n>\n> > > We would still have the issue of\n> > > retrieving this value, and I can see multiple options:\n> > >\n> > > - Compute it once at initialization from vblank limits + analog crop\n> > >\n> >\n> > This is assuming that vblank + crop height is always going to be equal to\n> > frame length? We need to think if that is strictly true for all sensors,\n> > I'm not sure.\n> >\n> >\n> >\n> > > - Extend V4L2 with a new vertical total size control and use it (that\n> > > will be more work as we need to synchronize with the kernel changes)\n> > >\n> >\n> > I expect this would have the longest lead time :-)\n> >\n> >\n> > > - Hardcode the value in a sensor database in libcamera, bypassing the\n> > > kernel completly\n> > >\n> >\n> > This is possible, but...\n> >\n> >\n> > >\n> > > One question that I'm not sure I can answer by myself is whether we can\n> > > rely on the vertical total size limits being constant.\n> > >\n> >\n> > ... I don't think you can assume total vertical size (frame length) is\n> > going to be the same for all modes a sensor advertises. In this case,\n> the\n> > sensor database would have to know about every mode available in the\n> kernel\n> > driver. This is something we moved away from with the Raspberry Pi\n> > CamHelper, so I doubt you want to re-introduce that link.\n> >\n> > My feeling is that we could work something out by forcing a \"refresh\" of\n> > the cached controls on every configure, and I think that should cover our\n> > (or at least Raspberry Pi's in this particular instance) usage.\n> >\n> > Let me know your thoughts.\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > > > }\n> > > >\n> > > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > > @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo\n> > > &sensorInfo,\n> > > > controller_.Initialise();\n> > > > controllerInit_ = true;\n> > > >\n> > > > - minFrameDuration_ = defaultMinFrameDuration;\n> > > > - maxFrameDuration_ = defaultMaxFrameDuration;\n> > > > + /* Supply initial values for frame durations. */\n> > > > + applyFrameDurations(defaultMinFrameDuration,\n> > > defaultMaxFrameDuration);\n> > > >\n> > > > /* Supply initial values for gain and exposure. */\n> > > > ControlList ctrls(sensorCtrls_);\n> > > > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList\n> > > &controls)\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> > > > - /*\n> > > > - * \\todo The values returned in the metadata\n> below\n> > > must be\n> > > > - * correctly clipped by what the sensor mode\n> > > supports and\n> > > > - * what the AGC exposure mode or manual shutter\n> > > speed limits\n> > > > - */\n> > > > -\n> libcameraMetadata_.set(controls::FrameDurations,\n> > > > - {\n> > > static_cast(minFrameDuration_),\n> > > > -\n> > > static_cast(maxFrameDuration_) });\n> > > > + applyFrameDurations(frameDurations[0],\n> > > frameDurations[1]);\n> > > > break;\n> > > > }\n> > > >\n> > > > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus\n> > > *awbStatus, ControlList &ctrls)\n> > > > static_cast(awbStatus->gain_b * 1000));\n> > > > }\n> > > >\n> > > > +void IPARPi::applyFrameDurations(double minFrameDuration, double\n> > > maxFrameDuration)\n> > > > +{\n> > > > + const double minSensorFrameDuration = 1e-3 * (mode_.vblank_min\n> +\n> > > mode_.height) *\n> > > > + mode_.line_length;\n> > > > + const double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max\n> +\n> > > mode_.height) *\n> > > > + mode_.line_length;\n>\n> To transform the frame size expressed in pixels:\n> (mode_.vblank + mode_.height) * mode_.line_length\n>\n> in a duration expressed in seconds sub-units, don't you need to use\n> the pixel clock (which is available in CameraSensorInfo afair)\n> frameDuration (usec) = frame size (pixels)\n> / pixel rate (pixels/usec)\n>\n\nYes we do. However, mode_.line_length already factors the pixel rate:\nmode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate\n\nSo mode_.line_length is already converted into units of time.\n\nRegards,\nNaush\n\n\n\n>\n> Thanks\n> j\n>\n> > > > + /*\n> > > > + * This will only be applied once AGC recalculations occur.\n> > > > + * The values may be clamped based on the sensor mode\n> capabilities\n> > > as well.\n> > > > + */\n> > > > + minFrameDuration_ = minFrameDuration ? minFrameDuration :\n> > > defaultMaxFrameDuration;\n> > > > + maxFrameDuration_ = maxFrameDuration ? maxFrameDuration :\n> > > defaultMinFrameDuration;\n> > > > + minFrameDuration_ = std::clamp(minFrameDuration_,\n> > > > + minSensorFrameDuration,\n> > > maxSensorFrameDuration);\n> > > > + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > > > + minSensorFrameDuration,\n> > > maxSensorFrameDuration);\n> > > > +\n> > > > + /* Return the validated limits out though metadata. */\n> > > > + libcameraMetadata_.set(controls::FrameDurations,\n> > > > + {\n> static_cast(minFrameDuration_),\n> > > > +\n> static_cast(maxFrameDuration_)\n> > > });\n> > > > +}\n> > > > +\n> > > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> > > &ctrls)\n> > > > {\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\n> > >\n>\n>\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 B9014BD808\n\tfor ;\n\tTue, 26 Jan 2021 14:13:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C7B76830C;\n\tTue, 26 Jan 2021 15:13:08 +0100 (CET)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48135682FF\n\tfor ;\n\tTue, 26 Jan 2021 15:13:07 +0100 (CET)","by mail-lj1-x22d.google.com with SMTP id i17so19742445ljn.1\n\tfor ;\n\tTue, 26 Jan 2021 06:13:07 -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=\"LjWkrnyO\"; 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=odnbkYqYKeN7YulObz56XfFzEPhqHz/vB64Fe+6n5kE=;\n\tb=LjWkrnyOWsTkO94A6y4WHU/IWlEA0rpL4YVaz7/6ed8Eny123+lU13etYREAmJ6BCH\n\tHidLYIUYbtun4jPbMH8fjnVwNJMvoHTXsoLqlR4J2nz/xrnqPEf28+EAopvxETKtQJjM\n\tS+uBgk4dD0m6w4uhZOBO8y7bIE01PnmwgzOoc/OWWJxFqFsaNKB+j3kn7dS41xaQ3FQ7\n\tM+xq61lQjNYHkrHB8s39LJIVZcE70A4hkRyvMxXg7mVbJpT/u9SEwtTQl/D5rVLYvta7\n\tTEyXYMDAFkqllLshbr/z7v7brTqg5EovgCMJaxU2s9LLPdFxg8h0K+Byh3MnK6G7gQ6Y\n\t6FRg==","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=odnbkYqYKeN7YulObz56XfFzEPhqHz/vB64Fe+6n5kE=;\n\tb=nFJFKsuFWQlBej7VY801aO+/rMC1j5ce7+O/Tb4PwgyL3+QnzndM3poaXsaDwB4JMV\n\tpQA1Ex+/E/+7Cl4gyz8/yQtiIWepsrHMWkypapg1j+MiYRy0fkGtTpVmEHovRo2z3dRk\n\tBuB+JcYmuOdZaS64zPfeyw5sA6wtlLCFZ89XsED9ipxYIaW0TCBkBLLgJ1T1TVlxxRNd\n\tnuM6w9+54vZ//Y9O5AGSiB6yFlcopalhexgcstm37iwMqDqGnfDLO2BWjXIfz7pOu9GT\n\tQzmFFqGx5z043jQdDye96YVQss/xMPVu5UwT7qm8kzUWp9K0WUHfScPt1FtZ92Ygj7X4\n\tgs+w==","X-Gm-Message-State":"AOAM533/xUlztnSOlbZXrhp4KOSNsvKiV+LP0kAnV51AzcLgkwXYPell\n\tzi/hlF1nwIX3Tp5tYFL82Miud/PhMBfuz8vOSNhfJ7b/GOmq8wGh","X-Google-Smtp-Source":"ABdhPJy9OutxRwgb9Qrc4FPyJfEVeZ/es3ItUxyyLxLGLhr4W07LkbwPT0MGPSm6/QnApdE02oHK7kXrzTfxysSvMUU=","X-Received":"by 2002:a2e:7605:: with SMTP id r5mr2874510ljc.299.1611670386440;\n\tTue, 26 Jan 2021 06:13:06 -0800 (PST)","MIME-Version":"1.0","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-2-naush@raspberrypi.com>\n\t\n\t\n\t<20210126134459.zcwgiklzqgaahbr2@uno.localdomain>","In-Reply-To":"<20210126134459.zcwgiklzqgaahbr2@uno.localdomain>","From":"Naushir Patuck ","Date":"Tue, 26 Jan 2021 14:12:50 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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=\"===============1786723969266150540==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14787,"web_url":"https://patchwork.libcamera.org/comment/14787/","msgid":"<20210126142205.mvw7o2bl2gxf3ghl@uno.localdomain>","date":"2021-01-26T14:22:05","subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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 26, 2021 at 02:12:50PM +0000, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi wrote:\n>\n\n[snip]\n\n> > > > > + */\n> > > > > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n> > > > > + mode_.vblank_min = itVblank->second.min().get();\n> > > > > + mode_.vblank_max = itVblank->second.max().get();\n> > > >\n> > > > The trouble here is that sensorCtrls_ will not be updated after the\n> > > > sensor configuration has changed. It comes from the\n> > > > V4L2Device::controls() call in RPiCameraData::configureIPA(), and that\n> > > > function returns a cached copy.\n> > > >\n> > >\n> > > I don't exactly know all of this in very much detail, so apologies if the\n> > > following statements sound basic, or don't make much sense :-)\n> > >\n> > > In RPiCameraData::configureIPA(), would it be possible to \"update\" the\n> > > cached values of V4L2Device::controls_ by\n> > > calling V4L2Device::listControls(), or maybe a new method such\n> > > as V4L2Device::updateControls(). This only needs to be called once\n> > > in PipelineHandlerRPi::configure(), after the sensor mode has been set.\n> > > This way, the VBLANK control will have up-to-date numbers for the IPA to\n> > > use.\n> > >\n> > > Alternatively, CameraSensorInfo gets populated in\n> > > RPiCameraData::configureIPA() with a call\n> > > to CameraSensor::sensorInfo(). Looks like this directly queries the\n> > device\n> > > for controls via V4L2Device::getControls(). Perhaps we should re-visit\n> > > Jacopo's suggestion and add the min/max vblank values to the mode?\n> > >\n> > >\n> > > >\n> > > > I really dislike the fact that V4L2 provides us with a control whose\n> > > > limits change when the analog crop rectangle height changes, while the\n> > > > sensors usually have a fixed vertical total size limit. Wouldn't it be\n> > > > better to base the code on the vertical total size limit in libcamera,\n> > > > instead of vertical blanking ?\n> > >\n> > >\n> > > By vertical total size limit, I assume you mean what sensors generally\n> > call\n> > > vts or frame length? If so, then yes, it does make sense. In fact, you\n> > > can see from my patch, the first thing I do is convert from vblank to\n> > frame\n> > > length for all of my calculations.\n> > >\n> > >\n> >\n> > Let me summarize your understandings:\n> > - You need the VBLANK limits to clamp frame durations in the sensor\n> > limits\n> > - VBLANK min and max are combined with the current mode height and\n> > line length to get the min and max frame sizes, from where the min\n> > and max frame duration limits are calculated\n> > - min frame size = (mode_.height + frameIntegrationDiff) *\n> > mode_.line_length\n> > - max frame size = (mode_.height + max vblank) * mode_.line_length\n> >\n> > From the CameraSensorInfo you already know the visibile height and the\n> > line_length. Wouldn't be enough to add to that structure\n> > - maxFrameHeight = (mode_.height + max vblank)\n> > - frameIntegrationDiff\n> > This is sensor property and will be made available in the\n> > sensor database. You can keep it in the IPA as long as no\n> > sensor database is available\n> >\n> > To sum it up: is it enough for you to pass to the IPA the maximum\n> > frame length (mode_.height + max vblank) in the CameraSensorClass ?\n> >\n>\n> This is *mostly* correct. max frame size as calculated above is fine.\n> However, I do also need min frame size. Your calculation above assumes\n> min_vblank = 0. This may not be the case always, so min frame size must be\n> computed and presented in CameraSensorInfo.\n\nCorrect, I had assumed min_vblank = frameIntegrationDiff, which might\nnot always be the case.\n\nConsidering the requirement for 'updates' and the fact the\nCameraSensorInfo is guaranteed to be fresh at IPA::Configure() time,\nI would be happy to see minFrameSize (or Height) and maxFrameSize (or\nHeight) added there an computed once for all IPAs in the CameraSensor\nclass.\n\n>\n>\n>\n> >\n> >\n> > > > We would still have the issue of\n> > > > retrieving this value, and I can see multiple options:\n> > > >\n> > > > - Compute it once at initialization from vblank limits + analog crop\n> > > >\n> > >\n> > > This is assuming that vblank + crop height is always going to be equal to\n> > > frame length? We need to think if that is strictly true for all sensors,\n> > > I'm not sure.\n> > >\n> > >\n> > >\n> > > > - Extend V4L2 with a new vertical total size control and use it (that\n> > > > will be more work as we need to synchronize with the kernel changes)\n> > > >\n> > >\n> > > I expect this would have the longest lead time :-)\n> > >\n> > >\n> > > > - Hardcode the value in a sensor database in libcamera, bypassing the\n> > > > kernel completly\n> > > >\n> > >\n> > > This is possible, but...\n> > >\n> > >\n> > > >\n> > > > One question that I'm not sure I can answer by myself is whether we can\n> > > > rely on the vertical total size limits being constant.\n> > > >\n> > >\n> > > ... I don't think you can assume total vertical size (frame length) is\n> > > going to be the same for all modes a sensor advertises. In this case,\n> > the\n> > > sensor database would have to know about every mode available in the\n> > kernel\n> > > driver. This is something we moved away from with the Raspberry Pi\n> > > CamHelper, so I doubt you want to re-introduce that link.\n> > >\n> > > My feeling is that we could work something out by forcing a \"refresh\" of\n> > > the cached controls on every configure, and I think that should cover our\n> > > (or at least Raspberry Pi's in this particular instance) usage.\n> > >\n> > > Let me know your thoughts.\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > >\n> > > > > }\n> > > > >\n\n[snip]\n\n> > > > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > > > @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo\n> > > > &sensorInfo,\n> > > > > controller_.Initialise();\n> > > > > controllerInit_ = true;\n> > > > >\n> > > > > - minFrameDuration_ = defaultMinFrameDuration;\n> > > > > - maxFrameDuration_ = defaultMaxFrameDuration;\n> > > > > + /* Supply initial values for frame durations. */\n> > > > > + applyFrameDurations(defaultMinFrameDuration,\n> > > > defaultMaxFrameDuration);\n> > > > >\n> > > > > /* Supply initial values for gain and exposure. */\n> > > > > ControlList ctrls(sensorCtrls_);\n> > > > > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList\n> > > > &controls)\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> > > > > - /*\n> > > > > - * \\todo The values returned in the metadata\n> > below\n> > > > must be\n> > > > > - * correctly clipped by what the sensor mode\n> > > > supports and\n> > > > > - * what the AGC exposure mode or manual shutter\n> > > > speed limits\n> > > > > - */\n> > > > > -\n> > libcameraMetadata_.set(controls::FrameDurations,\n> > > > > - {\n> > > > static_cast(minFrameDuration_),\n> > > > > -\n> > > > static_cast(maxFrameDuration_) });\n> > > > > + applyFrameDurations(frameDurations[0],\n> > > > frameDurations[1]);\n> > > > > break;\n> > > > > }\n> > > > >\n> > > > > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus\n> > > > *awbStatus, ControlList &ctrls)\n> > > > > static_cast(awbStatus->gain_b * 1000));\n> > > > > }\n> > > > >\n> > > > > +void IPARPi::applyFrameDurations(double minFrameDuration, double\n> > > > maxFrameDuration)\n> > > > > +{\n> > > > > + const double minSensorFrameDuration = 1e-3 * (mode_.vblank_min\n> > +\n> > > > mode_.height) *\n> > > > > + mode_.line_length;\n> > > > > + const double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max\n> > +\n> > > > mode_.height) *\n> > > > > + mode_.line_length;\n> >\n> > To transform the frame size expressed in pixels:\n> > (mode_.vblank + mode_.height) * mode_.line_length\n> >\n> > in a duration expressed in seconds sub-units, don't you need to use\n> > the pixel clock (which is available in CameraSensorInfo afair)\n> > frameDuration (usec) = frame size (pixels)\n> > / pixel rate (pixels/usec)\n> >\n>\n> Yes we do. However, mode_.line_length already factors the pixel rate:\n> mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate\n>\n> So mode_.line_length is already converted into units of time.\n\nOh sorry, I assumed CameraMode.line_lenght == CameraSensorInfo.lineLength\n\nI should have checked.\n\nThanks\n j\n\n>\n> Regards,\n> Naush\n>\n>\n>\n> >\n> > Thanks\n> > j\n> >\n> > > > > + /*\n> > > > > + * This will only be applied once AGC recalculations occur.\n> > > > > + * The values may be clamped based on the sensor mode\n> > capabilities\n> > > > as well.\n> > > > > + */\n> > > > > + minFrameDuration_ = minFrameDuration ? minFrameDuration :\n> > > > defaultMaxFrameDuration;\n> > > > > + maxFrameDuration_ = maxFrameDuration ? maxFrameDuration :\n> > > > defaultMinFrameDuration;\n> > > > > + minFrameDuration_ = std::clamp(minFrameDuration_,\n> > > > > + minSensorFrameDuration,\n> > > > maxSensorFrameDuration);\n> > > > > + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > > > > + minSensorFrameDuration,\n> > > > maxSensorFrameDuration);\n> > > > > +\n> > > > > + /* Return the validated limits out though metadata. */\n> > > > > + libcameraMetadata_.set(controls::FrameDurations,\n> > > > > + {\n> > static_cast(minFrameDuration_),\n> > > > > +\n> > static_cast(maxFrameDuration_)\n> > > > });\n> > > > > +}\n> > > > > +\n> > > > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> > > > &ctrls)\n> > > > > {\n> > > > --\n> > > > Regards,\n> > > >\n> > > > Laurent Pinchart\n> > > >\n> >\n> >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\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 ECC59C0F2B\n\tfor ;\n\tTue, 26 Jan 2021 14:21:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50F6E6830D;\n\tTue, 26 Jan 2021 15:21:48 +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 E0F0C68303\n\tfor ;\n\tTue, 26 Jan 2021 15:21:46 +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 relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 273D8FF816;\n\tTue, 26 Jan 2021 14:21:45 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 26 Jan 2021 15:22:05 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210126142205.mvw7o2bl2gxf3ghl@uno.localdomain>","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-2-naush@raspberrypi.com>\n\t\n\t\n\t<20210126134459.zcwgiklzqgaahbr2@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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":14788,"web_url":"https://patchwork.libcamera.org/comment/14788/","msgid":"","date":"2021-01-26T14:26:59","subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nOn Tue, 26 Jan 2021 at 14:21, Jacopo Mondi wrote:\n\n> Hi Naush,\n>\n> On Tue, Jan 26, 2021 at 02:12:50PM +0000, Naushir Patuck wrote:\n> > Hi Jacopo,\n> >\n> > On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi wrote:\n> >\n>\n> [snip]\n>\n> > > > > > + */\n> > > > > > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n> > > > > > + mode_.vblank_min = itVblank->second.min().get();\n> > > > > > + mode_.vblank_max = itVblank->second.max().get();\n> > > > >\n> > > > > The trouble here is that sensorCtrls_ will not be updated after the\n> > > > > sensor configuration has changed. It comes from the\n> > > > > V4L2Device::controls() call in RPiCameraData::configureIPA(), and\n> that\n> > > > > function returns a cached copy.\n> > > > >\n> > > >\n> > > > I don't exactly know all of this in very much detail, so apologies\n> if the\n> > > > following statements sound basic, or don't make much sense :-)\n> > > >\n> > > > In RPiCameraData::configureIPA(), would it be possible to \"update\"\n> the\n> > > > cached values of V4L2Device::controls_ by\n> > > > calling V4L2Device::listControls(), or maybe a new method such\n> > > > as V4L2Device::updateControls(). This only needs to be called once\n> > > > in PipelineHandlerRPi::configure(), after the sensor mode has been\n> set.\n> > > > This way, the VBLANK control will have up-to-date numbers for the\n> IPA to\n> > > > use.\n> > > >\n> > > > Alternatively, CameraSensorInfo gets populated in\n> > > > RPiCameraData::configureIPA() with a call\n> > > > to CameraSensor::sensorInfo(). Looks like this directly queries the\n> > > device\n> > > > for controls via V4L2Device::getControls(). Perhaps we should\n> re-visit\n> > > > Jacopo's suggestion and add the min/max vblank values to the mode?\n> > > >\n> > > >\n> > > > >\n> > > > > I really dislike the fact that V4L2 provides us with a control\n> whose\n> > > > > limits change when the analog crop rectangle height changes, while\n> the\n> > > > > sensors usually have a fixed vertical total size limit. Wouldn't\n> it be\n> > > > > better to base the code on the vertical total size limit in\n> libcamera,\n> > > > > instead of vertical blanking ?\n> > > >\n> > > >\n> > > > By vertical total size limit, I assume you mean what sensors\n> generally\n> > > call\n> > > > vts or frame length? If so, then yes, it does make sense. In fact,\n> you\n> > > > can see from my patch, the first thing I do is convert from vblank to\n> > > frame\n> > > > length for all of my calculations.\n> > > >\n> > > >\n> > >\n> > > Let me summarize your understandings:\n> > > - You need the VBLANK limits to clamp frame durations in the sensor\n> > > limits\n> > > - VBLANK min and max are combined with the current mode height and\n> > > line length to get the min and max frame sizes, from where the min\n> > > and max frame duration limits are calculated\n> > > - min frame size = (mode_.height + frameIntegrationDiff) *\n> > > mode_.line_length\n> > > - max frame size = (mode_.height + max vblank) * mode_.line_length\n> > >\n> > > From the CameraSensorInfo you already know the visibile height and the\n> > > line_length. Wouldn't be enough to add to that structure\n> > > - maxFrameHeight = (mode_.height + max vblank)\n> > > - frameIntegrationDiff\n> > > This is sensor property and will be made available in the\n> > > sensor database. You can keep it in the IPA as long as no\n> > > sensor database is available\n> > >\n> > > To sum it up: is it enough for you to pass to the IPA the maximum\n> > > frame length (mode_.height + max vblank) in the CameraSensorClass ?\n> > >\n> >\n> > This is *mostly* correct. max frame size as calculated above is fine.\n> > However, I do also need min frame size. Your calculation above assumes\n> > min_vblank = 0. This may not be the case always, so min frame size must\n> be\n> > computed and presented in CameraSensorInfo.\n>\n> Correct, I had assumed min_vblank = frameIntegrationDiff, which might\n> not always be the case.\n>\n> Considering the requirement for 'updates' and the fact the\n> CameraSensorInfo is guaranteed to be fresh at IPA::Configure() time,\n> I would be happy to see minFrameSize (or Height) and maxFrameSize (or\n> Height) added there an computed once for all IPAs in the CameraSensor\n> class.\n>\n\nGreat, I can put together a change for this and submit for review as part\nof this series. One question, what name would you and Laurent prefer for\nthis field - maxFrameSize or maxFrameLength (and equivalent for min)?\nGenerally, I think sensors prefer using frame length in their terminology,\nbut v4l2 probably prefers frame size. I am ok with either.\n\nRegards,\nNaush\n\n\n\n> >\n> >\n> >\n> > >\n> > >\n> > > > > We would still have the issue of\n> > > > > retrieving this value, and I can see multiple options:\n> > > > >\n> > > > > - Compute it once at initialization from vblank limits + analog\n> crop\n> > > > >\n> > > >\n> > > > This is assuming that vblank + crop height is always going to be\n> equal to\n> > > > frame length? We need to think if that is strictly true for all\n> sensors,\n> > > > I'm not sure.\n> > > >\n> > > >\n> > > >\n> > > > > - Extend V4L2 with a new vertical total size control and use it\n> (that\n> > > > > will be more work as we need to synchronize with the kernel\n> changes)\n> > > > >\n> > > >\n> > > > I expect this would have the longest lead time :-)\n> > > >\n> > > >\n> > > > > - Hardcode the value in a sensor database in libcamera, bypassing\n> the\n> > > > > kernel completly\n> > > > >\n> > > >\n> > > > This is possible, but...\n> > > >\n> > > >\n> > > > >\n> > > > > One question that I'm not sure I can answer by myself is whether\n> we can\n> > > > > rely on the vertical total size limits being constant.\n> > > > >\n> > > >\n> > > > ... I don't think you can assume total vertical size (frame length)\n> is\n> > > > going to be the same for all modes a sensor advertises. In this\n> case,\n> > > the\n> > > > sensor database would have to know about every mode available in the\n> > > kernel\n> > > > driver. This is something we moved away from with the Raspberry Pi\n> > > > CamHelper, so I doubt you want to re-introduce that link.\n> > > >\n> > > > My feeling is that we could work something out by forcing a\n> \"refresh\" of\n> > > > the cached controls on every configure, and I think that should\n> cover our\n> > > > (or at least Raspberry Pi's in this particular instance) usage.\n> > > >\n> > > > Let me know your thoughts.\n> > > >\n> > > > Regards,\n> > > > Naush\n> > > >\n> > > >\n> > > >\n> > > > > > }\n> > > > > >\n>\n> [snip]\n>\n> > > > > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > > > > @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo\n> > > > > &sensorInfo,\n> > > > > > controller_.Initialise();\n> > > > > > controllerInit_ = true;\n> > > > > >\n> > > > > > - minFrameDuration_ = defaultMinFrameDuration;\n> > > > > > - maxFrameDuration_ = defaultMaxFrameDuration;\n> > > > > > + /* Supply initial values for frame durations. */\n> > > > > > + applyFrameDurations(defaultMinFrameDuration,\n> > > > > defaultMaxFrameDuration);\n> > > > > >\n> > > > > > /* Supply initial values for gain and exposure. */\n> > > > > > ControlList ctrls(sensorCtrls_);\n> > > > > > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList\n> > > > > &controls)\n> > > > > >\n> > > > > > case controls::FRAME_DURATIONS: {\n> > > > > > auto frameDurations =\n> > > ctrl.second.get > > > > int64_t>>();\n> > > > > > -\n> > > > > > - /* This will be applied once AGC\n> recalculations\n> > > > > occur. */\n> > > > > > - minFrameDuration_ = frameDurations[0] ?\n> > > > > frameDurations[0] : defaultMinFrameDuration;\n> > > > > > - maxFrameDuration_ = frameDurations[1] ?\n> > > > > frameDurations[1] : defaultMaxFrameDuration;\n> > > > > > - maxFrameDuration_ =\n> std::max(maxFrameDuration_,\n> > > > > minFrameDuration_);\n> > > > > > -\n> > > > > > - /*\n> > > > > > - * \\todo The values returned in the\n> metadata\n> > > below\n> > > > > must be\n> > > > > > - * correctly clipped by what the sensor\n> mode\n> > > > > supports and\n> > > > > > - * what the AGC exposure mode or manual\n> shutter\n> > > > > speed limits\n> > > > > > - */\n> > > > > > -\n> > > libcameraMetadata_.set(controls::FrameDurations,\n> > > > > > - {\n> > > > > static_cast(minFrameDuration_),\n> > > > > > -\n> > > > > static_cast(maxFrameDuration_) });\n> > > > > > + applyFrameDurations(frameDurations[0],\n> > > > > frameDurations[1]);\n> > > > > > break;\n> > > > > > }\n> > > > > >\n> > > > > > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus\n> > > > > *awbStatus, ControlList &ctrls)\n> > > > > > static_cast(awbStatus->gain_b * 1000));\n> > > > > > }\n> > > > > >\n> > > > > > +void IPARPi::applyFrameDurations(double minFrameDuration, double\n> > > > > maxFrameDuration)\n> > > > > > +{\n> > > > > > + const double minSensorFrameDuration = 1e-3 *\n> (mode_.vblank_min\n> > > +\n> > > > > mode_.height) *\n> > > > > > + mode_.line_length;\n> > > > > > + const double maxSensorFrameDuration = 1e-3 *\n> (mode_.vblank_max\n> > > +\n> > > > > mode_.height) *\n> > > > > > + mode_.line_length;\n> > >\n> > > To transform the frame size expressed in pixels:\n> > > (mode_.vblank + mode_.height) * mode_.line_length\n> > >\n> > > in a duration expressed in seconds sub-units, don't you need to use\n> > > the pixel clock (which is available in CameraSensorInfo afair)\n> > > frameDuration (usec) = frame size (pixels)\n> > > / pixel rate (pixels/usec)\n> > >\n> >\n> > Yes we do. However, mode_.line_length already factors the pixel rate:\n> > mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate\n> >\n> > So mode_.line_length is already converted into units of time.\n>\n> Oh sorry, I assumed CameraMode.line_lenght == CameraSensorInfo.lineLength\n>\n> I should have checked.\n>\n> Thanks\n> j\n>\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > > > + /*\n> > > > > > + * This will only be applied once AGC recalculations occur.\n> > > > > > + * The values may be clamped based on the sensor mode\n> > > capabilities\n> > > > > as well.\n> > > > > > + */\n> > > > > > + minFrameDuration_ = minFrameDuration ? minFrameDuration :\n> > > > > defaultMaxFrameDuration;\n> > > > > > + maxFrameDuration_ = maxFrameDuration ? maxFrameDuration :\n> > > > > defaultMinFrameDuration;\n> > > > > > + minFrameDuration_ = std::clamp(minFrameDuration_,\n> > > > > > + minSensorFrameDuration,\n> > > > > maxSensorFrameDuration);\n> > > > > > + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > > > > > + minSensorFrameDuration,\n> > > > > maxSensorFrameDuration);\n> > > > > > +\n> > > > > > + /* Return the validated limits out though metadata. */\n> > > > > > + libcameraMetadata_.set(controls::FrameDurations,\n> > > > > > + {\n> > > static_cast(minFrameDuration_),\n> > > > > > +\n> > > static_cast(maxFrameDuration_)\n> > > > > });\n> > > > > > +}\n> > > > > > +\n> > > > > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus,\n> ControlList\n> > > > > &ctrls)\n> > > > > > {\n> > > > > --\n> > > > > Regards,\n> > > > >\n> > > > > Laurent Pinchart\n> > > > >\n> > >\n> > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\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 DA032BD808\n\tfor ;\n\tTue, 26 Jan 2021 14:27:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42AFE68312;\n\tTue, 26 Jan 2021 15:27:17 +0100 (CET)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BA9B68303\n\tfor ;\n\tTue, 26 Jan 2021 15:27:16 +0100 (CET)","by mail-lj1-x233.google.com with SMTP id f11so19748460ljm.8\n\tfor ;\n\tTue, 26 Jan 2021 06:27:16 -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=\"ATbRcEg7\"; 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=kckMB5JCiLkisFHDDGHeKlEAmykeScrEusj9VYu+TZw=;\n\tb=ATbRcEg7lpA7osaLDDte51dA18uk8WD9PuoUqOU3Cr0CAiBmNewBNDRBn5fhee+5QJ\n\t9CJYFill7lRdNOb8jvi49H26EaG8njxH2V9LfRf2DrEgmKubf1DyZFEA4NBh4Cn3FdA5\n\tqwdqeVEO+Gi2cJ/edXEPKlofwnmo3RaUTysdJizCi5shO+lHZPeXRqIHsIIi1M/V+341\n\tpaSNa/POhRlNrzF/j4Mfcm9Yqh9iqGcyJqDSwmSQC9g2kP+z/f7D+tsUvTqzPLFkDTS6\n\tMbLI+7TKVyMsJYKK0MOihQ5vn1H3KBTaejTuTDKm/nEH+FOisibryr/7ImsKyZ+dCzlM\n\toolw==","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=kckMB5JCiLkisFHDDGHeKlEAmykeScrEusj9VYu+TZw=;\n\tb=mYs/l5ntZxqBxa9jGR9v9DQOe4rorXk/rt3o39Bsn2Nj8o+epRtIEs/w3MGE1IppD9\n\t7IAFOSX4UamZKwm2vs5JPm7M8DVizi3a6c1A5Q0xGcxd04rs+d0LeFsZ8fA9jT85b7OZ\n\tl8oW0GKyz7qgqDAMJPIoARbEYmnTj6dKbSqSia2NvfPs2nELUskih/8QrOxGfQgByaD/\n\tGTtMGLVvjT+psvHhYrPWYN7ZzVh8KvlZjE491oWNIDw7qedjA5KTNeg6LdXE80CsdKkd\n\tQxLM04TvCRMrQz5a1PYEUQ1Tc+fWOLVhmrHSHlM6fB7PtErX3ye8ReKC5whGjKwRpKRn\n\tu3Dg==","X-Gm-Message-State":"AOAM53033GDRtbs+cj0azuakvO6MzPrtF5IcwMhOKyQYiTpePTSjW69F\n\tWMAy4GPKJQNkZs83OKCIa2o4bS/usgZth01KAj3jWg==","X-Google-Smtp-Source":"ABdhPJyCtr8/nZod78IxG8uSvcAPDjBHGgbuDKnotoXaJae4Ku87OZQsAe/zB3Mlv6lvoasHm8L33pDlEfij9Hu5wNM=","X-Received":"by 2002:a05:651c:2108:: with SMTP id\n\ta8mr2961987ljq.329.1611671235853; \n\tTue, 26 Jan 2021 06:27:15 -0800 (PST)","MIME-Version":"1.0","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-2-naush@raspberrypi.com>\n\t\n\t\n\t<20210126134459.zcwgiklzqgaahbr2@uno.localdomain>\n\t\n\t<20210126142205.mvw7o2bl2gxf3ghl@uno.localdomain>","In-Reply-To":"<20210126142205.mvw7o2bl2gxf3ghl@uno.localdomain>","From":"Naushir Patuck ","Date":"Tue, 26 Jan 2021 14:26:59 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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=\"===============7328694457723494941==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14789,"web_url":"https://patchwork.libcamera.org/comment/14789/","msgid":"<20210126143929.ymiiuxz22gp3swso@uno.localdomain>","date":"2021-01-26T14:39:29","subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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 26, 2021 at 02:26:59PM +0000, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> On Tue, 26 Jan 2021 at 14:21, Jacopo Mondi wrote:\n>\n> > Hi Naush,\n> >\n> > On Tue, Jan 26, 2021 at 02:12:50PM +0000, Naushir Patuck wrote:\n> > > Hi Jacopo,\n> > >\n> > > On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi wrote:\n> > >\n> >\n> > [snip]\n> >\n> > > > > > > + */\n> > > > > > > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n> > > > > > > + mode_.vblank_min = itVblank->second.min().get();\n> > > > > > > + mode_.vblank_max = itVblank->second.max().get();\n> > > > > >\n> > > > > > The trouble here is that sensorCtrls_ will not be updated after the\n> > > > > > sensor configuration has changed. It comes from the\n> > > > > > V4L2Device::controls() call in RPiCameraData::configureIPA(), and\n> > that\n> > > > > > function returns a cached copy.\n> > > > > >\n> > > > >\n> > > > > I don't exactly know all of this in very much detail, so apologies\n> > if the\n> > > > > following statements sound basic, or don't make much sense :-)\n> > > > >\n> > > > > In RPiCameraData::configureIPA(), would it be possible to \"update\"\n> > the\n> > > > > cached values of V4L2Device::controls_ by\n> > > > > calling V4L2Device::listControls(), or maybe a new method such\n> > > > > as V4L2Device::updateControls(). This only needs to be called once\n> > > > > in PipelineHandlerRPi::configure(), after the sensor mode has been\n> > set.\n> > > > > This way, the VBLANK control will have up-to-date numbers for the\n> > IPA to\n> > > > > use.\n> > > > >\n> > > > > Alternatively, CameraSensorInfo gets populated in\n> > > > > RPiCameraData::configureIPA() with a call\n> > > > > to CameraSensor::sensorInfo(). Looks like this directly queries the\n> > > > device\n> > > > > for controls via V4L2Device::getControls(). Perhaps we should\n> > re-visit\n> > > > > Jacopo's suggestion and add the min/max vblank values to the mode?\n> > > > >\n> > > > >\n> > > > > >\n> > > > > > I really dislike the fact that V4L2 provides us with a control\n> > whose\n> > > > > > limits change when the analog crop rectangle height changes, while\n> > the\n> > > > > > sensors usually have a fixed vertical total size limit. Wouldn't\n> > it be\n> > > > > > better to base the code on the vertical total size limit in\n> > libcamera,\n> > > > > > instead of vertical blanking ?\n> > > > >\n> > > > >\n> > > > > By vertical total size limit, I assume you mean what sensors\n> > generally\n> > > > call\n> > > > > vts or frame length? If so, then yes, it does make sense. In fact,\n> > you\n> > > > > can see from my patch, the first thing I do is convert from vblank to\n> > > > frame\n> > > > > length for all of my calculations.\n> > > > >\n> > > > >\n> > > >\n> > > > Let me summarize your understandings:\n> > > > - You need the VBLANK limits to clamp frame durations in the sensor\n> > > > limits\n> > > > - VBLANK min and max are combined with the current mode height and\n> > > > line length to get the min and max frame sizes, from where the min\n> > > > and max frame duration limits are calculated\n> > > > - min frame size = (mode_.height + frameIntegrationDiff) *\n> > > > mode_.line_length\n> > > > - max frame size = (mode_.height + max vblank) * mode_.line_length\n> > > >\n> > > > From the CameraSensorInfo you already know the visibile height and the\n> > > > line_length. Wouldn't be enough to add to that structure\n> > > > - maxFrameHeight = (mode_.height + max vblank)\n> > > > - frameIntegrationDiff\n> > > > This is sensor property and will be made available in the\n> > > > sensor database. You can keep it in the IPA as long as no\n> > > > sensor database is available\n> > > >\n> > > > To sum it up: is it enough for you to pass to the IPA the maximum\n> > > > frame length (mode_.height + max vblank) in the CameraSensorClass ?\n> > > >\n> > >\n> > > This is *mostly* correct. max frame size as calculated above is fine.\n> > > However, I do also need min frame size. Your calculation above assumes\n> > > min_vblank = 0. This may not be the case always, so min frame size must\n> > be\n> > > computed and presented in CameraSensorInfo.\n> >\n> > Correct, I had assumed min_vblank = frameIntegrationDiff, which might\n> > not always be the case.\n> >\n> > Considering the requirement for 'updates' and the fact the\n> > CameraSensorInfo is guaranteed to be fresh at IPA::Configure() time,\n> > I would be happy to see minFrameSize (or Height) and maxFrameSize (or\n> > Height) added there an computed once for all IPAs in the CameraSensor\n> > class.\n> >\n>\n> Great, I can put together a change for this and submit for review as part\n> of this series. One question, what name would you and Laurent prefer for\n> this field - maxFrameSize or maxFrameLength (and equivalent for min)?\n> Generally, I think sensors prefer using frame length in their terminology,\n> but v4l2 probably prefers frame size. I am ok with either.\n>\n\nThis and also if we want to report only the frame heights limits\nand let the IPA compute the frame size, or report the actual frame\nsize. It's a detail, I know, you can easily get from one to the other\nand viceversa..\n\n\n> Regards,\n> Naush\n>\n>\n>\n> > >\n> > >\n> > >\n> > > >\n> > > >\n> > > > > > We would still have the issue of\n> > > > > > retrieving this value, and I can see multiple options:\n> > > > > >\n> > > > > > - Compute it once at initialization from vblank limits + analog\n> > crop\n> > > > > >\n> > > > >\n> > > > > This is assuming that vblank + crop height is always going to be\n> > equal to\n> > > > > frame length? We need to think if that is strictly true for all\n> > sensors,\n> > > > > I'm not sure.\n> > > > >\n> > > > >\n> > > > >\n> > > > > > - Extend V4L2 with a new vertical total size control and use it\n> > (that\n> > > > > > will be more work as we need to synchronize with the kernel\n> > changes)\n> > > > > >\n> > > > >\n> > > > > I expect this would have the longest lead time :-)\n> > > > >\n> > > > >\n> > > > > > - Hardcode the value in a sensor database in libcamera, bypassing\n> > the\n> > > > > > kernel completly\n> > > > > >\n> > > > >\n> > > > > This is possible, but...\n> > > > >\n> > > > >\n> > > > > >\n> > > > > > One question that I'm not sure I can answer by myself is whether\n> > we can\n> > > > > > rely on the vertical total size limits being constant.\n> > > > > >\n> > > > >\n> > > > > ... I don't think you can assume total vertical size (frame length)\n> > is\n> > > > > going to be the same for all modes a sensor advertises. In this\n> > case,\n> > > > the\n> > > > > sensor database would have to know about every mode available in the\n> > > > kernel\n> > > > > driver. This is something we moved away from with the Raspberry Pi\n> > > > > CamHelper, so I doubt you want to re-introduce that link.\n> > > > >\n> > > > > My feeling is that we could work something out by forcing a\n> > \"refresh\" of\n> > > > > the cached controls on every configure, and I think that should\n> > cover our\n> > > > > (or at least Raspberry Pi's in this particular instance) usage.\n> > > > >\n> > > > > Let me know your thoughts.\n> > > > >\n> > > > > Regards,\n> > > > > Naush\n> > > > >\n> > > > >\n> > > > >\n> > > > > > > }\n> > > > > > >\n> >\n> > [snip]\n> >\n> > > > > > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > > > > > @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo\n> > > > > > &sensorInfo,\n> > > > > > > controller_.Initialise();\n> > > > > > > controllerInit_ = true;\n> > > > > > >\n> > > > > > > - minFrameDuration_ = defaultMinFrameDuration;\n> > > > > > > - maxFrameDuration_ = defaultMaxFrameDuration;\n> > > > > > > + /* Supply initial values for frame durations. */\n> > > > > > > + applyFrameDurations(defaultMinFrameDuration,\n> > > > > > defaultMaxFrameDuration);\n> > > > > > >\n> > > > > > > /* Supply initial values for gain and exposure. */\n> > > > > > > ControlList ctrls(sensorCtrls_);\n> > > > > > > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList\n> > > > > > &controls)\n> > > > > > >\n> > > > > > > case controls::FRAME_DURATIONS: {\n> > > > > > > auto frameDurations =\n> > > > ctrl.second.get > > > > > int64_t>>();\n> > > > > > > -\n> > > > > > > - /* This will be applied once AGC\n> > recalculations\n> > > > > > occur. */\n> > > > > > > - minFrameDuration_ = frameDurations[0] ?\n> > > > > > frameDurations[0] : defaultMinFrameDuration;\n> > > > > > > - maxFrameDuration_ = frameDurations[1] ?\n> > > > > > frameDurations[1] : defaultMaxFrameDuration;\n> > > > > > > - maxFrameDuration_ =\n> > std::max(maxFrameDuration_,\n> > > > > > minFrameDuration_);\n> > > > > > > -\n> > > > > > > - /*\n> > > > > > > - * \\todo The values returned in the\n> > metadata\n> > > > below\n> > > > > > must be\n> > > > > > > - * correctly clipped by what the sensor\n> > mode\n> > > > > > supports and\n> > > > > > > - * what the AGC exposure mode or manual\n> > shutter\n> > > > > > speed limits\n> > > > > > > - */\n> > > > > > > -\n> > > > libcameraMetadata_.set(controls::FrameDurations,\n> > > > > > > - {\n> > > > > > static_cast(minFrameDuration_),\n> > > > > > > -\n> > > > > > static_cast(maxFrameDuration_) });\n> > > > > > > + applyFrameDurations(frameDurations[0],\n> > > > > > frameDurations[1]);\n> > > > > > > break;\n> > > > > > > }\n> > > > > > >\n> > > > > > > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus\n> > > > > > *awbStatus, ControlList &ctrls)\n> > > > > > > static_cast(awbStatus->gain_b * 1000));\n> > > > > > > }\n> > > > > > >\n> > > > > > > +void IPARPi::applyFrameDurations(double minFrameDuration, double\n> > > > > > maxFrameDuration)\n> > > > > > > +{\n> > > > > > > + const double minSensorFrameDuration = 1e-3 *\n> > (mode_.vblank_min\n> > > > +\n> > > > > > mode_.height) *\n> > > > > > > + mode_.line_length;\n> > > > > > > + const double maxSensorFrameDuration = 1e-3 *\n> > (mode_.vblank_max\n> > > > +\n> > > > > > mode_.height) *\n> > > > > > > + mode_.line_length;\n> > > >\n> > > > To transform the frame size expressed in pixels:\n> > > > (mode_.vblank + mode_.height) * mode_.line_length\n> > > >\n> > > > in a duration expressed in seconds sub-units, don't you need to use\n> > > > the pixel clock (which is available in CameraSensorInfo afair)\n> > > > frameDuration (usec) = frame size (pixels)\n> > > > / pixel rate (pixels/usec)\n> > > >\n> > >\n> > > Yes we do. However, mode_.line_length already factors the pixel rate:\n> > > mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate\n> > >\n> > > So mode_.line_length is already converted into units of time.\n> >\n> > Oh sorry, I assumed CameraMode.line_lenght == CameraSensorInfo.lineLength\n> >\n> > I should have checked.\n> >\n> > Thanks\n> > j\n> >\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > > > > + /*\n> > > > > > > + * This will only be applied once AGC recalculations occur.\n> > > > > > > + * The values may be clamped based on the sensor mode\n> > > > capabilities\n> > > > > > as well.\n> > > > > > > + */\n> > > > > > > + minFrameDuration_ = minFrameDuration ? minFrameDuration :\n> > > > > > defaultMaxFrameDuration;\n> > > > > > > + maxFrameDuration_ = maxFrameDuration ? maxFrameDuration :\n> > > > > > defaultMinFrameDuration;\n> > > > > > > + minFrameDuration_ = std::clamp(minFrameDuration_,\n> > > > > > > + minSensorFrameDuration,\n> > > > > > maxSensorFrameDuration);\n> > > > > > > + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > > > > > > + minSensorFrameDuration,\n> > > > > > maxSensorFrameDuration);\n> > > > > > > +\n> > > > > > > + /* Return the validated limits out though metadata. */\n> > > > > > > + libcameraMetadata_.set(controls::FrameDurations,\n> > > > > > > + {\n> > > > static_cast(minFrameDuration_),\n> > > > > > > +\n> > > > static_cast(maxFrameDuration_)\n> > > > > > });\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus,\n> > ControlList\n> > > > > > &ctrls)\n> > > > > > > {\n> > > > > > --\n> > > > > > Regards,\n> > > > > >\n> > > > > > Laurent Pinchart\n> > > > > >\n> > > >\n> > > >\n> > > > > _______________________________________________\n> > > > > libcamera-devel mailing list\n> > > > > libcamera-devel@lists.libcamera.org\n> > > > > https://lists.libcamera.org/listinfo/libcamera-devel\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 C79AEBD808\n\tfor ;\n\tTue, 26 Jan 2021 14:39:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3012068312;\n\tTue, 26 Jan 2021 15:39:11 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 049E26830B\n\tfor ;\n\tTue, 26 Jan 2021 15:39:10 +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 relay10.mail.gandi.net (Postfix) with ESMTPSA id 51DF6240013;\n\tTue, 26 Jan 2021 14:39:09 +0000 (UTC)"],"Date":"Tue, 26 Jan 2021 15:39:29 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210126143929.ymiiuxz22gp3swso@uno.localdomain>","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-2-naush@raspberrypi.com>\n\t\n\t\n\t<20210126134459.zcwgiklzqgaahbr2@uno.localdomain>\n\t\n\t<20210126142205.mvw7o2bl2gxf3ghl@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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":14790,"web_url":"https://patchwork.libcamera.org/comment/14790/","msgid":"","date":"2021-01-26T16:08:41","subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo and Laurent,\n\n\nOn Tue, 26 Jan 2021 at 14:39, Jacopo Mondi wrote:\n\n> Hi Naush,\n>\n> On Tue, Jan 26, 2021 at 02:26:59PM +0000, Naushir Patuck wrote:\n> > Hi Jacopo,\n> >\n> > On Tue, 26 Jan 2021 at 14:21, Jacopo Mondi wrote:\n> >\n> > > Hi Naush,\n> > >\n> > > On Tue, Jan 26, 2021 at 02:12:50PM +0000, Naushir Patuck wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi \n> wrote:\n> > > >\n> > >\n> > > [snip]\n> > >\n> > > > > > > > + */\n> > > > > > > > + const auto itVblank =\n> sensorCtrls_.find(V4L2_CID_VBLANK);\n> > > > > > > > + mode_.vblank_min =\n> itVblank->second.min().get();\n> > > > > > > > + mode_.vblank_max =\n> itVblank->second.max().get();\n> > > > > > >\n> > > > > > > The trouble here is that sensorCtrls_ will not be updated\n> after the\n> > > > > > > sensor configuration has changed. It comes from the\n> > > > > > > V4L2Device::controls() call in RPiCameraData::configureIPA(),\n> and\n> > > that\n> > > > > > > function returns a cached copy.\n> > > > > > >\n> > > > > >\n> > > > > > I don't exactly know all of this in very much detail, so\n> apologies\n> > > if the\n> > > > > > following statements sound basic, or don't make much sense :-)\n> > > > > >\n> > > > > > In RPiCameraData::configureIPA(), would it be possible to\n> \"update\"\n> > > the\n> > > > > > cached values of V4L2Device::controls_ by\n> > > > > > calling V4L2Device::listControls(), or maybe a new method such\n> > > > > > as V4L2Device::updateControls(). This only needs to be called\n> once\n> > > > > > in PipelineHandlerRPi::configure(), after the sensor mode has\n> been\n> > > set.\n> > > > > > This way, the VBLANK control will have up-to-date numbers for the\n> > > IPA to\n> > > > > > use.\n> > > > > >\n> > > > > > Alternatively, CameraSensorInfo gets populated in\n> > > > > > RPiCameraData::configureIPA() with a call\n> > > > > > to CameraSensor::sensorInfo(). Looks like this directly queries\n> the\n> > > > > device\n> > > > > > for controls via V4L2Device::getControls(). Perhaps we should\n> > > re-visit\n> > > > > > Jacopo's suggestion and add the min/max vblank values to the\n> mode?\n> > > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > I really dislike the fact that V4L2 provides us with a control\n> > > whose\n> > > > > > > limits change when the analog crop rectangle height changes,\n> while\n> > > the\n> > > > > > > sensors usually have a fixed vertical total size limit.\n> Wouldn't\n> > > it be\n> > > > > > > better to base the code on the vertical total size limit in\n> > > libcamera,\n> > > > > > > instead of vertical blanking ?\n> > > > > >\n> > > > > >\n> > > > > > By vertical total size limit, I assume you mean what sensors\n> > > generally\n> > > > > call\n> > > > > > vts or frame length? If so, then yes, it does make sense. In\n> fact,\n> > > you\n> > > > > > can see from my patch, the first thing I do is convert from\n> vblank to\n> > > > > frame\n> > > > > > length for all of my calculations.\n> > > > > >\n> > > > > >\n> > > > >\n> > > > > Let me summarize your understandings:\n> > > > > - You need the VBLANK limits to clamp frame durations in the sensor\n> > > > > limits\n> > > > > - VBLANK min and max are combined with the current mode height and\n> > > > > line length to get the min and max frame sizes, from where the\n> min\n> > > > > and max frame duration limits are calculated\n> > > > > - min frame size = (mode_.height + frameIntegrationDiff) *\n> > > > > mode_.line_length\n> > > > > - max frame size = (mode_.height + max vblank) * mode_.line_length\n> > > > >\n> > > > > From the CameraSensorInfo you already know the visibile height and\n> the\n> > > > > line_length. Wouldn't be enough to add to that structure\n> > > > > - maxFrameHeight = (mode_.height + max vblank)\n> > > > > - frameIntegrationDiff\n> > > > > This is sensor property and will be made available in the\n> > > > > sensor database. You can keep it in the IPA as long as no\n> > > > > sensor database is available\n> > > > >\n> > > > > To sum it up: is it enough for you to pass to the IPA the maximum\n> > > > > frame length (mode_.height + max vblank) in the CameraSensorClass ?\n> > > > >\n> > > >\n> > > > This is *mostly* correct. max frame size as calculated above is\n> fine.\n> > > > However, I do also need min frame size. Your calculation above\n> assumes\n> > > > min_vblank = 0. This may not be the case always, so min frame size\n> must\n> > > be\n> > > > computed and presented in CameraSensorInfo.\n> > >\n> > > Correct, I had assumed min_vblank = frameIntegrationDiff, which might\n> > > not always be the case.\n> > >\n> > > Considering the requirement for 'updates' and the fact the\n> > > CameraSensorInfo is guaranteed to be fresh at IPA::Configure() time,\n> > > I would be happy to see minFrameSize (or Height) and maxFrameSize (or\n> > > Height) added there an computed once for all IPAs in the CameraSensor\n> > > class.\n> > >\n> >\n> > Great, I can put together a change for this and submit for review as part\n> > of this series. One question, what name would you and Laurent prefer for\n> > this field - maxFrameSize or maxFrameLength (and equivalent for min)?\n> > Generally, I think sensors prefer using frame length in their\n> terminology,\n> > but v4l2 probably prefers frame size. I am ok with either.\n> >\n>\n> This and also if we want to report only the frame heights limits\n> and let the IPA compute the frame size, or report the actual frame\n> size. It's a detail, I know, you can easily get from one to the other\n> and viceversa..\n>\n\nSo just to confirm before I make a submission, is something like this what\nyou had in mind?\n\ndiff --git a/src/libcamera/camera_sensor.cpp\nb/src/libcamera/camera_sensor.cpp\nindex 251691aedfa3..6037f5f72897 100644\n--- a/src/libcamera/camera_sensor.cpp\n+++ b/src/libcamera/camera_sensor.cpp\n/**\n * \\class CameraSensor\n * \\brief A camera sensor based on V4L2 subdevices\n@@ -698,11 +728,12 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info)\nconst\n\n /*\n * Retrieve the pixel rate and the line length through V4L2\ncontrols.\n- * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls\nis\n- * mandatory.\n+ * Support for the V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and\n+ * V4L2_CID_VBLANK controls is mandatory.\n */\n ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n- V4L2_CID_HBLANK });\n+ V4L2_CID_HBLANK,\n+ V4L2_CID_VBLANK });\n if (ctrls.empty()) {\n LOG(CameraSensor, Error)\n << \"Failed to retrieve camera info controls\";\n@@ -713,6 +744,10 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info)\nconst\n info->lineLength = info->outputSize.width + hblank;\n info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get();\n\n+ const ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);\n+ info->minFrameLength = info->outputSize.height +\nvblank.min().get();\n+ info->maxFrameLength = info->outputSize.height +\nvblank.max().get();\n+\n return 0;\n }\n\nLet me know if you think that is suitable or not.\n\n\n\n>\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > > >\n> > > >\n> > > >\n> > > > >\n> > > > >\n> > > > > > > We would still have the issue of\n> > > > > > > retrieving this value, and I can see multiple options:\n> > > > > > >\n> > > > > > > - Compute it once at initialization from vblank limits + analog\n> > > crop\n> > > > > > >\n> > > > > >\n> > > > > > This is assuming that vblank + crop height is always going to be\n> > > equal to\n> > > > > > frame length? We need to think if that is strictly true for all\n> > > sensors,\n> > > > > > I'm not sure.\n> > > > > >\n> > > > > >\n> > > > > >\n> > > > > > > - Extend V4L2 with a new vertical total size control and use it\n> > > (that\n> > > > > > > will be more work as we need to synchronize with the kernel\n> > > changes)\n> > > > > > >\n> > > > > >\n> > > > > > I expect this would have the longest lead time :-)\n> > > > > >\n> > > > > >\n> > > > > > > - Hardcode the value in a sensor database in libcamera,\n> bypassing\n> > > the\n> > > > > > > kernel completly\n> > > > > > >\n> > > > > >\n> > > > > > This is possible, but...\n> > > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > One question that I'm not sure I can answer by myself is\n> whether\n> > > we can\n> > > > > > > rely on the vertical total size limits being constant.\n> > > > > > >\n> > > > > >\n> > > > > > ... I don't think you can assume total vertical size (frame\n> length)\n> > > is\n> > > > > > going to be the same for all modes a sensor advertises. In this\n> > > case,\n> > > > > the\n> > > > > > sensor database would have to know about every mode available in\n> the\n> > > > > kernel\n> > > > > > driver. This is something we moved away from with the Raspberry\n> Pi\n> > > > > > CamHelper, so I doubt you want to re-introduce that link.\n> > > > > >\n> > > > > > My feeling is that we could work something out by forcing a\n> > > \"refresh\" of\n> > > > > > the cached controls on every configure, and I think that should\n> > > cover our\n> > > > > > (or at least Raspberry Pi's in this particular instance) usage.\n> > > > > >\n> > > > > > Let me know your thoughts.\n> > > > > >\n> > > > > > Regards,\n> > > > > > Naush\n> > > > > >\n> > > > > >\n> > > > > >\n> > > > > > > > }\n> > > > > > > >\n> > >\n> > > [snip]\n> > >\n> > > > > > > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > > > > > > @@ -384,8 +393,8 @@ void IPARPi::configure(const\n> CameraSensorInfo\n> > > > > > > &sensorInfo,\n> > > > > > > > controller_.Initialise();\n> > > > > > > > controllerInit_ = true;\n> > > > > > > >\n> > > > > > > > - minFrameDuration_ = defaultMinFrameDuration;\n> > > > > > > > - maxFrameDuration_ = defaultMaxFrameDuration;\n> > > > > > > > + /* Supply initial values for frame durations.\n> */\n> > > > > > > > + applyFrameDurations(defaultMinFrameDuration,\n> > > > > > > defaultMaxFrameDuration);\n> > > > > > > >\n> > > > > > > > /* Supply initial values for gain and\n> exposure. */\n> > > > > > > > ControlList ctrls(sensorCtrls_);\n> > > > > > > > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const\n> ControlList\n> > > > > > > &controls)\n> > > > > > > >\n> > > > > > > > case controls::FRAME_DURATIONS: {\n> > > > > > > > auto frameDurations =\n> > > > > ctrl.second.get > > > > > > int64_t>>();\n> > > > > > > > -\n> > > > > > > > - /* This will be applied once AGC\n> > > recalculations\n> > > > > > > occur. */\n> > > > > > > > - minFrameDuration_ = frameDurations[0] ?\n> > > > > > > frameDurations[0] : defaultMinFrameDuration;\n> > > > > > > > - maxFrameDuration_ = frameDurations[1] ?\n> > > > > > > frameDurations[1] : defaultMaxFrameDuration;\n> > > > > > > > - maxFrameDuration_ =\n> > > std::max(maxFrameDuration_,\n> > > > > > > minFrameDuration_);\n> > > > > > > > -\n> > > > > > > > - /*\n> > > > > > > > - * \\todo The values returned in the\n> > > metadata\n> > > > > below\n> > > > > > > must be\n> > > > > > > > - * correctly clipped by what the sensor\n> > > mode\n> > > > > > > supports and\n> > > > > > > > - * what the AGC exposure mode or manual\n> > > shutter\n> > > > > > > speed limits\n> > > > > > > > - */\n> > > > > > > > -\n> > > > > libcameraMetadata_.set(controls::FrameDurations,\n> > > > > > > > - {\n> > > > > > > static_cast(minFrameDuration_),\n> > > > > > > > -\n> > > > > > > static_cast(maxFrameDuration_) });\n> > > > > > > > + applyFrameDurations(frameDurations[0],\n> > > > > > > frameDurations[1]);\n> > > > > > > > break;\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct\n> AwbStatus\n> > > > > > > *awbStatus, ControlList &ctrls)\n> > > > > > > > static_cast(awbStatus->gain_b *\n> 1000));\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > +void IPARPi::applyFrameDurations(double minFrameDuration,\n> double\n> > > > > > > maxFrameDuration)\n> > > > > > > > +{\n> > > > > > > > + const double minSensorFrameDuration = 1e-3 *\n> > > (mode_.vblank_min\n> > > > > +\n> > > > > > > mode_.height) *\n> > > > > > > > +\n> mode_.line_length;\n> > > > > > > > + const double maxSensorFrameDuration = 1e-3 *\n> > > (mode_.vblank_max\n> > > > > +\n> > > > > > > mode_.height) *\n> > > > > > > > +\n> mode_.line_length;\n> > > > >\n> > > > > To transform the frame size expressed in pixels:\n> > > > > (mode_.vblank + mode_.height) * mode_.line_length\n> > > > >\n> > > > > in a duration expressed in seconds sub-units, don't you need to use\n> > > > > the pixel clock (which is available in CameraSensorInfo afair)\n> > > > > frameDuration (usec) = frame size (pixels)\n> > > > > / pixel rate (pixels/usec)\n> > > > >\n> > > >\n> > > > Yes we do. However, mode_.line_length already factors the pixel\n> rate:\n> > > > mode_.line_length = 1e9 * sensorInfo.lineLength /\n> sensorInfo.pixelRate\n> > > >\n> > > > So mode_.line_length is already converted into units of time.\n> > >\n> > > Oh sorry, I assumed CameraMode.line_lenght ==\n> CameraSensorInfo.lineLength\n> > >\n> > > I should have checked.\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > >\n> > > > Regards,\n> > > > Naush\n> > > >\n> > > >\n> > > >\n> > > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > > > > > + /*\n> > > > > > > > + * This will only be applied once AGC recalculations\n> occur.\n> > > > > > > > + * The values may be clamped based on the sensor mode\n> > > > > capabilities\n> > > > > > > as well.\n> > > > > > > > + */\n> > > > > > > > + minFrameDuration_ = minFrameDuration ?\n> minFrameDuration :\n> > > > > > > defaultMaxFrameDuration;\n> > > > > > > > + maxFrameDuration_ = maxFrameDuration ?\n> maxFrameDuration :\n> > > > > > > defaultMinFrameDuration;\n> > > > > > > > + minFrameDuration_ = std::clamp(minFrameDuration_,\n> > > > > > > > + minSensorFrameDuration,\n> > > > > > > maxSensorFrameDuration);\n> > > > > > > > + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > > > > > > > + minSensorFrameDuration,\n> > > > > > > maxSensorFrameDuration);\n> > > > > > > > +\n> > > > > > > > + /* Return the validated limits out though metadata. */\n> > > > > > > > + libcameraMetadata_.set(controls::FrameDurations,\n> > > > > > > > + {\n> > > > > static_cast(minFrameDuration_),\n> > > > > > > > +\n> > > > > static_cast(maxFrameDuration_)\n> > > > > > > });\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus,\n> > > ControlList\n> > > > > > > &ctrls)\n> > > > > > > > {\n> > > > > > > --\n> > > > > > > Regards,\n> > > > > > >\n> > > > > > > Laurent Pinchart\n> > > > > > >\n> > > > >\n> > > > >\n> > > > > > _______________________________________________\n> > > > > > libcamera-devel mailing list\n> > > > > > libcamera-devel@lists.libcamera.org\n> > > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > >\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 4322EBD808\n\tfor ;\n\tTue, 26 Jan 2021 16:09:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1EBF68317;\n\tTue, 26 Jan 2021 17:09:00 +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 3E38168301\n\tfor ;\n\tTue, 26 Jan 2021 17:08:59 +0100 (CET)","by mail-lf1-x129.google.com with SMTP id i187so13148062lfd.4\n\tfor ;\n\tTue, 26 Jan 2021 08:08:59 -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=\"s0BtysN+\"; 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=TAbFMn0CL6icUJoRLLqrjZCSzqe1tSpTJYqeQOUD5RQ=;\n\tb=s0BtysN+w+5ObKv+Z9mu7AvzBXiR3pERjZtu4SUjDSNgDRXgE/ZY5jPs8ysvMkmumz\n\tAn6L2XoztNvM8ylVwG5AjUVBhmxBN3sIlN14J5nxsq342mmlw3NBeSr/0sDpnq+Upvxx\n\tifEUyogSYV0dE3JtZ2H9TuTL1gL+Q1dLMTaxDB5PWkTMjFGlJjGpnpVh7RbzsPnrSaTD\n\tuGFsjygzPVd8QCci4aG0eMMhJI+oldr6xgzCN7WHQTApynrqfr8wl5F3poNSh+ZFaA0L\n\tRkJr2VREveAWUfJ9LP3i+e8o1ioIZo5xXFMKllUK8GowdEvdU35tl7WU6/sgQP97PF0u\n\tAvpw==","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=TAbFMn0CL6icUJoRLLqrjZCSzqe1tSpTJYqeQOUD5RQ=;\n\tb=YXu1tJT/JYSxbD9MFDVh6Cq9IbXLgK7DxypFWzr44g6q24Tv90Cv6Y/eFQttA211Ik\n\tW1Moye5FpCOZCAWPJHGrQ+gkyE8eXu+BBu9NcHybUO5SFQxsCHRLW1Wt/xMkIwngXXAy\n\tWIPNlBrdnCv5JmviiGdLXHJs6w1nKz+ctvOGLnqFlYBWXMVRkLJnI48u4s6md4FrC5Zn\n\tbtxoqzPc4CsaM5SNtv5eEO8bCrRJyP3agw+wXYSns0F6SmqlpK+GMRdXt+ko760z0h9Q\n\tVdEw/wJh1pHy+gICw/GLcLrgUw5LMLJFVAD3XOo2CNMgXsWiEE7OPHhXMkdDOI6U3X8z\n\t7ijA==","X-Gm-Message-State":"AOAM533pQnUVPx7mIWDcnU/E3Vun8MNN3A0EIJFi3kzoSKwomKLuJk20\n\tc+KPDoEXGbz1LaZn2MiA/SsOoGI2O0ttG+qFVh2n+Q==","X-Google-Smtp-Source":"ABdhPJyHjdBxpz9v0ePH8cHXQgroo5Oa0XKqNBIclvG2zHL6GBSNjBUr7FhsWjAj7/5fmedQA0mcoZQ54D5pjUB31+I=","X-Received":"by 2002:ac2:5f5b:: with SMTP id 27mr2944442lfz.375.1611677338357;\n\tTue, 26 Jan 2021 08:08:58 -0800 (PST)","MIME-Version":"1.0","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-2-naush@raspberrypi.com>\n\t\n\t\n\t<20210126134459.zcwgiklzqgaahbr2@uno.localdomain>\n\t\n\t<20210126142205.mvw7o2bl2gxf3ghl@uno.localdomain>\n\t\n\t<20210126143929.ymiiuxz22gp3swso@uno.localdomain>","In-Reply-To":"<20210126143929.ymiiuxz22gp3swso@uno.localdomain>","From":"Naushir Patuck ","Date":"Tue, 26 Jan 2021 16:08:41 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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=\"===============4395520086133353883==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14791,"web_url":"https://patchwork.libcamera.org/comment/14791/","msgid":"<20210126162045.std5ykrgfryx35t4@uno.localdomain>","date":"2021-01-26T16:20:45","subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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 26, 2021 at 04:08:41PM +0000, Naushir Patuck wrote:\n> Hi Jacopo and Laurent,\n>\n>\n> On Tue, 26 Jan 2021 at 14:39, Jacopo Mondi wrote:\n>\n> > Hi Naush,\n> >\n> > On Tue, Jan 26, 2021 at 02:26:59PM +0000, Naushir Patuck wrote:\n> > > Hi Jacopo,\n> > >\n> > > On Tue, 26 Jan 2021 at 14:21, Jacopo Mondi wrote:\n> > >\n> > > > Hi Naush,\n> > > >\n> > > > On Tue, Jan 26, 2021 at 02:12:50PM +0000, Naushir Patuck wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi \n> > wrote:\n> > > > >\n> > > >\n> > > > [snip]\n> > > >\n> > > > > > > > > + */\n> > > > > > > > > + const auto itVblank =\n> > sensorCtrls_.find(V4L2_CID_VBLANK);\n> > > > > > > > > + mode_.vblank_min =\n> > itVblank->second.min().get();\n> > > > > > > > > + mode_.vblank_max =\n> > itVblank->second.max().get();\n> > > > > > > >\n> > > > > > > > The trouble here is that sensorCtrls_ will not be updated\n> > after the\n> > > > > > > > sensor configuration has changed. It comes from the\n> > > > > > > > V4L2Device::controls() call in RPiCameraData::configureIPA(),\n> > and\n> > > > that\n> > > > > > > > function returns a cached copy.\n> > > > > > > >\n> > > > > > >\n> > > > > > > I don't exactly know all of this in very much detail, so\n> > apologies\n> > > > if the\n> > > > > > > following statements sound basic, or don't make much sense :-)\n> > > > > > >\n> > > > > > > In RPiCameraData::configureIPA(), would it be possible to\n> > \"update\"\n> > > > the\n> > > > > > > cached values of V4L2Device::controls_ by\n> > > > > > > calling V4L2Device::listControls(), or maybe a new method such\n> > > > > > > as V4L2Device::updateControls(). This only needs to be called\n> > once\n> > > > > > > in PipelineHandlerRPi::configure(), after the sensor mode has\n> > been\n> > > > set.\n> > > > > > > This way, the VBLANK control will have up-to-date numbers for the\n> > > > IPA to\n> > > > > > > use.\n> > > > > > >\n> > > > > > > Alternatively, CameraSensorInfo gets populated in\n> > > > > > > RPiCameraData::configureIPA() with a call\n> > > > > > > to CameraSensor::sensorInfo(). Looks like this directly queries\n> > the\n> > > > > > device\n> > > > > > > for controls via V4L2Device::getControls(). Perhaps we should\n> > > > re-visit\n> > > > > > > Jacopo's suggestion and add the min/max vblank values to the\n> > mode?\n> > > > > > >\n> > > > > > >\n> > > > > > > >\n> > > > > > > > I really dislike the fact that V4L2 provides us with a control\n> > > > whose\n> > > > > > > > limits change when the analog crop rectangle height changes,\n> > while\n> > > > the\n> > > > > > > > sensors usually have a fixed vertical total size limit.\n> > Wouldn't\n> > > > it be\n> > > > > > > > better to base the code on the vertical total size limit in\n> > > > libcamera,\n> > > > > > > > instead of vertical blanking ?\n> > > > > > >\n> > > > > > >\n> > > > > > > By vertical total size limit, I assume you mean what sensors\n> > > > generally\n> > > > > > call\n> > > > > > > vts or frame length? If so, then yes, it does make sense. In\n> > fact,\n> > > > you\n> > > > > > > can see from my patch, the first thing I do is convert from\n> > vblank to\n> > > > > > frame\n> > > > > > > length for all of my calculations.\n> > > > > > >\n> > > > > > >\n> > > > > >\n> > > > > > Let me summarize your understandings:\n> > > > > > - You need the VBLANK limits to clamp frame durations in the sensor\n> > > > > > limits\n> > > > > > - VBLANK min and max are combined with the current mode height and\n> > > > > > line length to get the min and max frame sizes, from where the\n> > min\n> > > > > > and max frame duration limits are calculated\n> > > > > > - min frame size = (mode_.height + frameIntegrationDiff) *\n> > > > > > mode_.line_length\n> > > > > > - max frame size = (mode_.height + max vblank) * mode_.line_length\n> > > > > >\n> > > > > > From the CameraSensorInfo you already know the visibile height and\n> > the\n> > > > > > line_length. Wouldn't be enough to add to that structure\n> > > > > > - maxFrameHeight = (mode_.height + max vblank)\n> > > > > > - frameIntegrationDiff\n> > > > > > This is sensor property and will be made available in the\n> > > > > > sensor database. You can keep it in the IPA as long as no\n> > > > > > sensor database is available\n> > > > > >\n> > > > > > To sum it up: is it enough for you to pass to the IPA the maximum\n> > > > > > frame length (mode_.height + max vblank) in the CameraSensorClass ?\n> > > > > >\n> > > > >\n> > > > > This is *mostly* correct. max frame size as calculated above is\n> > fine.\n> > > > > However, I do also need min frame size. Your calculation above\n> > assumes\n> > > > > min_vblank = 0. This may not be the case always, so min frame size\n> > must\n> > > > be\n> > > > > computed and presented in CameraSensorInfo.\n> > > >\n> > > > Correct, I had assumed min_vblank = frameIntegrationDiff, which might\n> > > > not always be the case.\n> > > >\n> > > > Considering the requirement for 'updates' and the fact the\n> > > > CameraSensorInfo is guaranteed to be fresh at IPA::Configure() time,\n> > > > I would be happy to see minFrameSize (or Height) and maxFrameSize (or\n> > > > Height) added there an computed once for all IPAs in the CameraSensor\n> > > > class.\n> > > >\n> > >\n> > > Great, I can put together a change for this and submit for review as part\n> > > of this series. One question, what name would you and Laurent prefer for\n> > > this field - maxFrameSize or maxFrameLength (and equivalent for min)?\n> > > Generally, I think sensors prefer using frame length in their\n> > terminology,\n> > > but v4l2 probably prefers frame size. I am ok with either.\n> > >\n> >\n> > This and also if we want to report only the frame heights limits\n> > and let the IPA compute the frame size, or report the actual frame\n> > size. It's a detail, I know, you can easily get from one to the other\n> > and viceversa..\n> >\n>\n> So just to confirm before I make a submission, is something like this what\n> you had in mind?\n>\n> diff --git a/src/libcamera/camera_sensor.cpp\n> b/src/libcamera/camera_sensor.cpp\n> index 251691aedfa3..6037f5f72897 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> /**\n> * \\class CameraSensor\n> * \\brief A camera sensor based on V4L2 subdevices\n> @@ -698,11 +728,12 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info)\n> const\n>\n> /*\n> * Retrieve the pixel rate and the line length through V4L2\n> controls.\n> - * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls\n> is\n> - * mandatory.\n> + * Support for the V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and\n> + * V4L2_CID_VBLANK controls is mandatory.\n> */\n> ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n> - V4L2_CID_HBLANK });\n> + V4L2_CID_HBLANK,\n> + V4L2_CID_VBLANK });\n\nPlease be aware of\nhttps://patchwork.libcamera.org/patch/10937/\n\nYou can rebase on that patch or do something similar and the first\nthat lands it wins :)\n\n> if (ctrls.empty()) {\n> LOG(CameraSensor, Error)\n> << \"Failed to retrieve camera info controls\";\n> @@ -713,6 +744,10 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info)\n> const\n> info->lineLength = info->outputSize.width + hblank;\n> info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get();\n>\n> + const ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);\n> + info->minFrameLength = info->outputSize.height +\n> vblank.min().get();\n> + info->maxFrameLength = info->outputSize.height +\n> vblank.max().get();\n> +\n\nThat's what I had in mind, yes!\n\nWorks for me, let's see what Laurent thinks\n\nThanks\n j\n\n> return 0;\n> }\n>\n> Let me know if you think that is suitable or not.\n>\n>\n>\n> >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > >\n> > > > >\n> > > > >\n> > > > >\n> > > > > >\n> > > > > >\n> > > > > > > > We would still have the issue of\n> > > > > > > > retrieving this value, and I can see multiple options:\n> > > > > > > >\n> > > > > > > > - Compute it once at initialization from vblank limits + analog\n> > > > crop\n> > > > > > > >\n> > > > > > >\n> > > > > > > This is assuming that vblank + crop height is always going to be\n> > > > equal to\n> > > > > > > frame length? We need to think if that is strictly true for all\n> > > > sensors,\n> > > > > > > I'm not sure.\n> > > > > > >\n> > > > > > >\n> > > > > > >\n> > > > > > > > - Extend V4L2 with a new vertical total size control and use it\n> > > > (that\n> > > > > > > > will be more work as we need to synchronize with the kernel\n> > > > changes)\n> > > > > > > >\n> > > > > > >\n> > > > > > > I expect this would have the longest lead time :-)\n> > > > > > >\n> > > > > > >\n> > > > > > > > - Hardcode the value in a sensor database in libcamera,\n> > bypassing\n> > > > the\n> > > > > > > > kernel completly\n> > > > > > > >\n> > > > > > >\n> > > > > > > This is possible, but...\n> > > > > > >\n> > > > > > >\n> > > > > > > >\n> > > > > > > > One question that I'm not sure I can answer by myself is\n> > whether\n> > > > we can\n> > > > > > > > rely on the vertical total size limits being constant.\n> > > > > > > >\n> > > > > > >\n> > > > > > > ... I don't think you can assume total vertical size (frame\n> > length)\n> > > > is\n> > > > > > > going to be the same for all modes a sensor advertises. In this\n> > > > case,\n> > > > > > the\n> > > > > > > sensor database would have to know about every mode available in\n> > the\n> > > > > > kernel\n> > > > > > > driver. This is something we moved away from with the Raspberry\n> > Pi\n> > > > > > > CamHelper, so I doubt you want to re-introduce that link.\n> > > > > > >\n> > > > > > > My feeling is that we could work something out by forcing a\n> > > > \"refresh\" of\n> > > > > > > the cached controls on every configure, and I think that should\n> > > > cover our\n> > > > > > > (or at least Raspberry Pi's in this particular instance) usage.\n> > > > > > >\n> > > > > > > Let me know your thoughts.\n> > > > > > >\n> > > > > > > Regards,\n> > > > > > > Naush\n> > > > > > >\n> > > > > > >\n> > > > > > >\n> > > > > > > > > }\n> > > > > > > > >\n> > > >\n> > > > [snip]\n> > > >\n> > > > > > > > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > > > > > > > @@ -384,8 +393,8 @@ void IPARPi::configure(const\n> > CameraSensorInfo\n> > > > > > > > &sensorInfo,\n> > > > > > > > > controller_.Initialise();\n> > > > > > > > > controllerInit_ = true;\n> > > > > > > > >\n> > > > > > > > > - minFrameDuration_ = defaultMinFrameDuration;\n> > > > > > > > > - maxFrameDuration_ = defaultMaxFrameDuration;\n> > > > > > > > > + /* Supply initial values for frame durations.\n> > */\n> > > > > > > > > + applyFrameDurations(defaultMinFrameDuration,\n> > > > > > > > defaultMaxFrameDuration);\n> > > > > > > > >\n> > > > > > > > > /* Supply initial values for gain and\n> > exposure. */\n> > > > > > > > > ControlList ctrls(sensorCtrls_);\n> > > > > > > > > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const\n> > ControlList\n> > > > > > > > &controls)\n> > > > > > > > >\n> > > > > > > > > case controls::FRAME_DURATIONS: {\n> > > > > > > > > auto frameDurations =\n> > > > > > ctrl.second.get > > > > > > > int64_t>>();\n> > > > > > > > > -\n> > > > > > > > > - /* This will be applied once AGC\n> > > > recalculations\n> > > > > > > > occur. */\n> > > > > > > > > - minFrameDuration_ = frameDurations[0] ?\n> > > > > > > > frameDurations[0] : defaultMinFrameDuration;\n> > > > > > > > > - maxFrameDuration_ = frameDurations[1] ?\n> > > > > > > > frameDurations[1] : defaultMaxFrameDuration;\n> > > > > > > > > - maxFrameDuration_ =\n> > > > std::max(maxFrameDuration_,\n> > > > > > > > minFrameDuration_);\n> > > > > > > > > -\n> > > > > > > > > - /*\n> > > > > > > > > - * \\todo The values returned in the\n> > > > metadata\n> > > > > > below\n> > > > > > > > must be\n> > > > > > > > > - * correctly clipped by what the sensor\n> > > > mode\n> > > > > > > > supports and\n> > > > > > > > > - * what the AGC exposure mode or manual\n> > > > shutter\n> > > > > > > > speed limits\n> > > > > > > > > - */\n> > > > > > > > > -\n> > > > > > libcameraMetadata_.set(controls::FrameDurations,\n> > > > > > > > > - {\n> > > > > > > > static_cast(minFrameDuration_),\n> > > > > > > > > -\n> > > > > > > > static_cast(maxFrameDuration_) });\n> > > > > > > > > + applyFrameDurations(frameDurations[0],\n> > > > > > > > frameDurations[1]);\n> > > > > > > > > break;\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct\n> > AwbStatus\n> > > > > > > > *awbStatus, ControlList &ctrls)\n> > > > > > > > > static_cast(awbStatus->gain_b *\n> > 1000));\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > +void IPARPi::applyFrameDurations(double minFrameDuration,\n> > double\n> > > > > > > > maxFrameDuration)\n> > > > > > > > > +{\n> > > > > > > > > + const double minSensorFrameDuration = 1e-3 *\n> > > > (mode_.vblank_min\n> > > > > > +\n> > > > > > > > mode_.height) *\n> > > > > > > > > +\n> > mode_.line_length;\n> > > > > > > > > + const double maxSensorFrameDuration = 1e-3 *\n> > > > (mode_.vblank_max\n> > > > > > +\n> > > > > > > > mode_.height) *\n> > > > > > > > > +\n> > mode_.line_length;\n> > > > > >\n> > > > > > To transform the frame size expressed in pixels:\n> > > > > > (mode_.vblank + mode_.height) * mode_.line_length\n> > > > > >\n> > > > > > in a duration expressed in seconds sub-units, don't you need to use\n> > > > > > the pixel clock (which is available in CameraSensorInfo afair)\n> > > > > > frameDuration (usec) = frame size (pixels)\n> > > > > > / pixel rate (pixels/usec)\n> > > > > >\n> > > > >\n> > > > > Yes we do. However, mode_.line_length already factors the pixel\n> > rate:\n> > > > > mode_.line_length = 1e9 * sensorInfo.lineLength /\n> > sensorInfo.pixelRate\n> > > > >\n> > > > > So mode_.line_length is already converted into units of time.\n> > > >\n> > > > Oh sorry, I assumed CameraMode.line_lenght ==\n> > CameraSensorInfo.lineLength\n> > > >\n> > > > I should have checked.\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > >\n> > > > > Regards,\n> > > > > Naush\n> > > > >\n> > > > >\n> > > > >\n> > > > > >\n> > > > > > Thanks\n> > > > > > j\n> > > > > >\n> > > > > > > > > + /*\n> > > > > > > > > + * This will only be applied once AGC recalculations\n> > occur.\n> > > > > > > > > + * The values may be clamped based on the sensor mode\n> > > > > > capabilities\n> > > > > > > > as well.\n> > > > > > > > > + */\n> > > > > > > > > + minFrameDuration_ = minFrameDuration ?\n> > minFrameDuration :\n> > > > > > > > defaultMaxFrameDuration;\n> > > > > > > > > + maxFrameDuration_ = maxFrameDuration ?\n> > maxFrameDuration :\n> > > > > > > > defaultMinFrameDuration;\n> > > > > > > > > + minFrameDuration_ = std::clamp(minFrameDuration_,\n> > > > > > > > > + minSensorFrameDuration,\n> > > > > > > > maxSensorFrameDuration);\n> > > > > > > > > + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > > > > > > > > + minSensorFrameDuration,\n> > > > > > > > maxSensorFrameDuration);\n> > > > > > > > > +\n> > > > > > > > > + /* Return the validated limits out though metadata. */\n> > > > > > > > > + libcameraMetadata_.set(controls::FrameDurations,\n> > > > > > > > > + {\n> > > > > > static_cast(minFrameDuration_),\n> > > > > > > > > +\n> > > > > > static_cast(maxFrameDuration_)\n> > > > > > > > });\n> > > > > > > > > +}\n> > > > > > > > > +\n> > > > > > > > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus,\n> > > > ControlList\n> > > > > > > > &ctrls)\n> > > > > > > > > {\n> > > > > > > > --\n> > > > > > > > Regards,\n> > > > > > > >\n> > > > > > > > Laurent Pinchart\n> > > > > > > >\n> > > > > >\n> > > > > >\n> > > > > > > _______________________________________________\n> > > > > > > libcamera-devel mailing list\n> > > > > > > libcamera-devel@lists.libcamera.org\n> > > > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > > >\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 91C55BD808\n\tfor ;\n\tTue, 26 Jan 2021 16:20:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 225ED68316;\n\tTue, 26 Jan 2021 17:20:28 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 761E36830F\n\tfor ;\n\tTue, 26 Jan 2021 17:20: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 relay6-d.mail.gandi.net (Postfix) with ESMTPSA id BCD28C0003;\n\tTue, 26 Jan 2021 16:20:25 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 26 Jan 2021 17:20:45 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210126162045.std5ykrgfryx35t4@uno.localdomain>","References":"<20210124140506.786503-1-naush@raspberrypi.com>\n\t<20210124140506.786503-2-naush@raspberrypi.com>\n\t\n\t\n\t<20210126134459.zcwgiklzqgaahbr2@uno.localdomain>\n\t\n\t<20210126142205.mvw7o2bl2gxf3ghl@uno.localdomain>\n\t\n\t<20210126143929.ymiiuxz22gp3swso@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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":14809,"web_url":"https://patchwork.libcamera.org/comment/14809/","msgid":"","date":"2021-01-27T01:34:13","subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush and Jacopo,\n\nOn Tue, Jan 26, 2021 at 05:20:45PM +0100, Jacopo Mondi wrote:\n> On Tue, Jan 26, 2021 at 04:08:41PM +0000, Naushir Patuck wrote:\n> > On Tue, 26 Jan 2021 at 14:39, Jacopo Mondi wrote:\n> > > On Tue, Jan 26, 2021 at 02:26:59PM +0000, Naushir Patuck wrote:\n> > > > On Tue, 26 Jan 2021 at 14:21, Jacopo Mondi wrote:\n> > > > > On Tue, Jan 26, 2021 at 02:12:50PM +0000, Naushir Patuck wrote:\n> > > > > > On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi wrote:\n> > > > >\n> > > > > [snip]\n> > > > >\n> > > > > > > > > > + */\n> > > > > > > > > > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n> > > > > > > > > > + mode_.vblank_min = itVblank->second.min().get();\n> > > > > > > > > > + mode_.vblank_max = itVblank->second.max().get();\n> > > > > > > > >\n> > > > > > > > > The trouble here is that sensorCtrls_ will not be updated after the\n> > > > > > > > > sensor configuration has changed. It comes from the\n> > > > > > > > > V4L2Device::controls() call in RPiCameraData::configureIPA(), and that\n> > > > > > > > > function returns a cached copy.\n> > > > > > > >\n> > > > > > > > I don't exactly know all of this in very much detail, so apologies if the\n> > > > > > > > following statements sound basic, or don't make much sense :-)\n> > > > > > > >\n> > > > > > > > In RPiCameraData::configureIPA(), would it be possible to \"update\" the\n> > > > > > > > cached values of V4L2Device::controls_ by\n> > > > > > > > calling V4L2Device::listControls(), or maybe a new method such\n> > > > > > > > as V4L2Device::updateControls(). This only needs to be called once\n> > > > > > > > in PipelineHandlerRPi::configure(), after the sensor mode has been set.\n> > > > > > > > This way, the VBLANK control will have up-to-date numbers for the IPA to\n> > > > > > > > use.\n\nI believe that should be technically feasible. Whether it is desired is\na different question :-) More on that below.\n\n> > > > > > > > Alternatively, CameraSensorInfo gets populated in\n> > > > > > > > RPiCameraData::configureIPA() with a call\n> > > > > > > > to CameraSensor::sensorInfo(). Looks like this directly queries the device\n> > > > > > > > for controls via V4L2Device::getControls(). Perhaps we should re-visit\n> > > > > > > > Jacopo's suggestion and add the min/max vblank values to the mode?\n\nRegardless of how we retrieve the information (directly from the V4L2\ndevice or from the CameraSensor class), the problem stays the same.\nHowever, I believe CameraSensor is a better match, as it will help\nabstracting V4L2 from pipeline handlers. We don't have to switch right\nnow, but I'm pretty sure we will down the line, so if it makes sense in\nthe grand scheme of things to already make the transition, I'd welcome\nthat.\n\n> > > > > > > > > I really dislike the fact that V4L2 provides us with a control whose\n> > > > > > > > > limits change when the analog crop rectangle height changes, while the\n> > > > > > > > > sensors usually have a fixed vertical total size limit. Wouldn't it be\n> > > > > > > > > better to base the code on the vertical total size limit in libcamera,\n> > > > > > > > > instead of vertical blanking ?\n> > > > > > > >\n> > > > > > > > By vertical total size limit, I assume you mean what sensors generally call\n> > > > > > > > vts or frame length? If so, then yes, it does make sense. In fact, you\n> > > > > > > > can see from my patch, the first thing I do is convert from vblank to frame\n> > > > > > > > length for all of my calculations.\n\nYes, that's what I meant, the total of active lines and blanking lines.\nBlanking lines is a bit of a loosely defined concept here as it includes\nthe embedded data lines, optical black lines, ... It's really the frame\ntime expressed as a number of lines (which leads to interesting\nquestions if we consider sensors that would natively express the frame\ntime in a different way).\n\n> > > > > > > Let me summarize your understandings:\n> > > > > > > - You need the VBLANK limits to clamp frame durations in the sensor\n> > > > > > > limits\n> > > > > > > - VBLANK min and max are combined with the current mode height and\n> > > > > > > line length to get the min and max frame sizes, from where the min\n> > > > > > > and max frame duration limits are calculated\n> > > > > > > - min frame size = (mode_.height + frameIntegrationDiff) * mode_.line_length\n> > > > > > > - max frame size = (mode_.height + max vblank) * mode_.line_length\n\nAs we assume everywhere that frames are made of an integral number of\nlines, I would express the minimum and maximum frame size as a number of\nlines, not a number of pixels.\n\n> > > > > > > From the CameraSensorInfo you already know the visibile height and the\n> > > > > > > line_length. Wouldn't be enough to add to that structure\n> > > > > > > - maxFrameHeight = (mode_.height + max vblank)\n> > > > > > > - frameIntegrationDiff\n> > > > > > > This is sensor property and will be made available in the\n> > > > > > > sensor database. You can keep it in the IPA as long as no\n> > > > > > > sensor database is available\n> > > > > > >\n> > > > > > > To sum it up: is it enough for you to pass to the IPA the maximum\n> > > > > > > frame length (mode_.height + max vblank) in the CameraSensorClass ?\n> > > > > >\n> > > > > > This is *mostly* correct. max frame size as calculated above is fine.\n> > > > > > However, I do also need min frame size. Your calculation above assumes\n> > > > > > min_vblank = 0. This may not be the case always, so min frame size must be\n> > > > > > computed and presented in CameraSensorInfo.\n> > > > >\n> > > > > Correct, I had assumed min_vblank = frameIntegrationDiff, which might\n> > > > > not always be the case.\n> > > > >\n> > > > > Considering the requirement for 'updates' and the fact the\n> > > > > CameraSensorInfo is guaranteed to be fresh at IPA::Configure() time,\n> > > > > I would be happy to see minFrameSize (or Height) and maxFrameSize (or\n> > > > > Height) added there an computed once for all IPAs in the CameraSensor\n> > > > > class.\n> > > >\n> > > > Great, I can put together a change for this and submit for review as part\n> > > > of this series. One question, what name would you and Laurent prefer for\n> > > > this field - maxFrameSize or maxFrameLength (and equivalent for min)?\n> > > > Generally, I think sensors prefer using frame length in their terminology,\n> > > > but v4l2 probably prefers frame size. I am ok with either.\n\nI'd use *Length or *Height here, as we usually use *Size for\ntwo-dimensional sizes.\n\n> > > This and also if we want to report only the frame heights limits\n> > > and let the IPA compute the frame size, or report the actual frame\n> > > size. It's a detail, I know, you can easily get from one to the other\n> > > and viceversa..\n> >\n> > So just to confirm before I make a submission, is something like this what\n> > you had in mind?\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 251691aedfa3..6037f5f72897 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > /**\n> > * \\class CameraSensor\n> > * \\brief A camera sensor based on V4L2 subdevices\n> > @@ -698,11 +728,12 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> >\n> > /*\n> > * Retrieve the pixel rate and the line length through V4L2 controls.\n> > - * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is\n> > - * mandatory.\n> > + * Support for the V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and\n> > + * V4L2_CID_VBLANK controls is mandatory.\n> > */\n> > ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n> > - V4L2_CID_HBLANK });\n> > + V4L2_CID_HBLANK,\n> > + V4L2_CID_VBLANK });\n> \n> Please be aware of\n> https://patchwork.libcamera.org/patch/10937/\n> \n> You can rebase on that patch or do something similar and the first\n> that lands it wins :)\n> \n> > if (ctrls.empty()) {\n> > LOG(CameraSensor, Error)\n> > << \"Failed to retrieve camera info controls\";\n> > @@ -713,6 +744,10 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const\n> > info->lineLength = info->outputSize.width + hblank;\n> > info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get();\n> >\n> > + const ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);\n> > + info->minFrameLength = info->outputSize.height + vblank.min().get();\n> > + info->maxFrameLength = info->outputSize.height + vblank.max().get();\n> > +\n> \n> That's what I had in mind, yes!\n> \n> Works for me, let's see what Laurent thinks\n\nThis looks good to me.\n\n> > return 0;\n> > }\n> >\n> > Let me know if you think that is suitable or not.\n> >\n> > > > > > > > > We would still have the issue of\n> > > > > > > > > retrieving this value, and I can see multiple options:\n> > > > > > > > >\n> > > > > > > > > - Compute it once at initialization from vblank limits + analog crop\n> > > > > > > >\n> > > > > > > > This is assuming that vblank + crop height is always going to be equal to\n> > > > > > > > frame length? We need to think if that is strictly true for all sensors,\n> > > > > > > > I'm not sure.\n\nIt depends on how we define vblank. Its current purpose in libcamera is\nto control the frame rate, so from that point of view isn't it\neffectively always the frame length minus the active height ?\n\n> > > > > > > > > - Extend V4L2 with a new vertical total size control and use it (that\n> > > > > > > > > will be more work as we need to synchronize with the kernel changes)\n> > > > > > > >\n> > > > > > > > I expect this would have the longest lead time :-)\n\nYes, I'm not looking forward to that :-S\n\n> > > > > > > > > - Hardcode the value in a sensor database in libcamera, bypassing the\n> > > > > > > > > kernel completly\n> > > > > > > >\n> > > > > > > > This is possible, but...\n> > > > > > > >\n> > > > > > > > > One question that I'm not sure I can answer by myself is whether we can\n> > > > > > > > > rely on the vertical total size limits being constant.\n> > > > > > > >\n> > > > > > > > ... I don't think you can assume total vertical size (frame length) is\n> > > > > > > > going to be the same for all modes a sensor advertises.\n\nThat's my worry as well. Do you have any example of a sensor that\nwouldn't guarantee the maximum frame length being a constant ?\n\n> > > > > > > > In this case, the\n> > > > > > > > sensor database would have to know about every mode available in the kernel\n> > > > > > > > driver. This is something we moved away from with the Raspberry Pi\n> > > > > > > > CamHelper, so I doubt you want to re-introduce that link.\n\nFully agreed, I don't want that either. If we decide to solve this\nthrough a camera sensor database, we would need to encode how the sensor\nhandles the frame length constraints, not their numerical values for\nspecific modes. For sensors that have a fixed maximum frame length it\nwould just be a number, for other sensors it may be a few numerical\nconstants for a fixed formula, or even sensor-specific code that would\ntake as input the sensor mode.\n\n> > > > > > > > My feeling is that we could work something out by forcing a \"refresh\" of\n> > > > > > > > the cached controls on every configure, and I think that should cover our\n> > > > > > > > (or at least Raspberry Pi's in this particular instance) usage.\n> > > > > > > >\n> > > > > > > > Let me know your thoughts.\n\nLet's start with a refresh, through the CameraSensor class, as discussed\nabove. This will move the problem away from pipeline handlers and IPA\nmodules, and will allow us to switch to a different method to retrieve\n(or compute) the information later if desired.","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 19EDEC0F2B\n\tfor ;\n\tWed, 27 Jan 2021 01:34:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 600E26833D;\n\tWed, 27 Jan 2021 02:34:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D607660309\n\tfor ;\n\tWed, 27 Jan 2021 02:34:33 +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 459CD240;\n\tWed, 27 Jan 2021 02:34:33 +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=\"rVpreGy0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611711273;\n\tbh=Q1MqkP10ROiCzkzXyesyIaWH/rcANsN6AalrY4c76JY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rVpreGy09EJ7xjEijG1B2rH/Ao6ExWtPm7fJfL6mLm3iJXI8N65GvGvJNu93Gc8OA\n\tvcRWujBD6hQSL43e4Lr12GCnOvpr/B8NzVCCuaU4gGiDuC+UMtIrLynEqyANkP9S12\n\th9OrEcrfw5FYCqOkxIuBNeEQifOK5IYYA+VpINQA=","Date":"Wed, 27 Jan 2021 03:34:13 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210124140506.786503-2-naush@raspberrypi.com>\n\t\n\t\n\t<20210126134459.zcwgiklzqgaahbr2@uno.localdomain>\n\t\n\t<20210126142205.mvw7o2bl2gxf3ghl@uno.localdomain>\n\t\n\t<20210126143929.ymiiuxz22gp3swso@uno.localdomain>\n\t\n\t<20210126162045.std5ykrgfryx35t4@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210126162045.std5ykrgfryx35t4@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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":14831,"web_url":"https://patchwork.libcamera.org/comment/14831/","msgid":"","date":"2021-01-27T11:38:55","subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent and Jacopo,\n\nOn Wed, 27 Jan 2021 at 01:34, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush and Jacopo,\n>\n> On Tue, Jan 26, 2021 at 05:20:45PM +0100, Jacopo Mondi wrote:\n> > On Tue, Jan 26, 2021 at 04:08:41PM +0000, Naushir Patuck wrote:\n> > > On Tue, 26 Jan 2021 at 14:39, Jacopo Mondi wrote:\n> > > > On Tue, Jan 26, 2021 at 02:26:59PM +0000, Naushir Patuck wrote:\n> > > > > On Tue, 26 Jan 2021 at 14:21, Jacopo Mondi wrote:\n> > > > > > On Tue, Jan 26, 2021 at 02:12:50PM +0000, Naushir Patuck wrote:\n> > > > > > > On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi wrote:\n> > > > > >\n> > > > > > [snip]\n> > > > > >\n> > > > > > > > > > > + */\n> > > > > > > > > > > + const auto itVblank =\n> sensorCtrls_.find(V4L2_CID_VBLANK);\n> > > > > > > > > > > + mode_.vblank_min =\n> itVblank->second.min().get();\n> > > > > > > > > > > + mode_.vblank_max =\n> itVblank->second.max().get();\n> > > > > > > > > >\n> > > > > > > > > > The trouble here is that sensorCtrls_ will not be\n> updated after the\n> > > > > > > > > > sensor configuration has changed. It comes from the\n> > > > > > > > > > V4L2Device::controls() call in\n> RPiCameraData::configureIPA(), and that\n> > > > > > > > > > function returns a cached copy.\n> > > > > > > > >\n> > > > > > > > > I don't exactly know all of this in very much detail, so\n> apologies if the\n> > > > > > > > > following statements sound basic, or don't make much sense\n> :-)\n> > > > > > > > >\n> > > > > > > > > In RPiCameraData::configureIPA(), would it be possible to\n> \"update\" the\n> > > > > > > > > cached values of V4L2Device::controls_ by\n> > > > > > > > > calling V4L2Device::listControls(), or maybe a new method\n> such\n> > > > > > > > > as V4L2Device::updateControls(). This only needs to be\n> called once\n> > > > > > > > > in PipelineHandlerRPi::configure(), after the sensor mode\n> has been set.\n> > > > > > > > > This way, the VBLANK control will have up-to-date numbers\n> for the IPA to\n> > > > > > > > > use.\n>\n> I believe that should be technically feasible. Whether it is desired is\n> a different question :-) More on that below.\n>\n> > > > > > > > > Alternatively, CameraSensorInfo gets populated in\n> > > > > > > > > RPiCameraData::configureIPA() with a call\n> > > > > > > > > to CameraSensor::sensorInfo(). Looks like this directly\n> queries the device\n> > > > > > > > > for controls via V4L2Device::getControls(). Perhaps we\n> should re-visit\n> > > > > > > > > Jacopo's suggestion and add the min/max vblank values to\n> the mode?\n>\n> Regardless of how we retrieve the information (directly from the V4L2\n> device or from the CameraSensor class), the problem stays the same.\n> However, I believe CameraSensor is a better match, as it will help\n> abstracting V4L2 from pipeline handlers. We don't have to switch right\n> now, but I'm pretty sure we will down the line, so if it makes sense in\n> the grand scheme of things to already make the transition, I'd welcome\n> that.\n>\n> > > > > > > > > > I really dislike the fact that V4L2 provides us with a\n> control whose\n> > > > > > > > > > limits change when the analog crop rectangle height\n> changes, while the\n> > > > > > > > > > sensors usually have a fixed vertical total size limit.\n> Wouldn't it be\n> > > > > > > > > > better to base the code on the vertical total size limit\n> in libcamera,\n> > > > > > > > > > instead of vertical blanking ?\n> > > > > > > > >\n> > > > > > > > > By vertical total size limit, I assume you mean what\n> sensors generally call\n> > > > > > > > > vts or frame length? If so, then yes, it does make\n> sense. In fact, you\n> > > > > > > > > can see from my patch, the first thing I do is convert\n> from vblank to frame\n> > > > > > > > > length for all of my calculations.\n>\n> Yes, that's what I meant, the total of active lines and blanking lines.\n> Blanking lines is a bit of a loosely defined concept here as it includes\n> the embedded data lines, optical black lines, ... It's really the frame\n> time expressed as a number of lines (which leads to interesting\n> questions if we consider sensors that would natively express the frame\n> time in a different way).\n>\n> > > > > > > > Let me summarize your understandings:\n> > > > > > > > - You need the VBLANK limits to clamp frame durations in the\n> sensor\n> > > > > > > > limits\n> > > > > > > > - VBLANK min and max are combined with the current mode\n> height and\n> > > > > > > > line length to get the min and max frame sizes, from where\n> the min\n> > > > > > > > and max frame duration limits are calculated\n> > > > > > > > - min frame size = (mode_.height + frameIntegrationDiff) *\n> mode_.line_length\n> > > > > > > > - max frame size = (mode_.height + max vblank) *\n> mode_.line_length\n>\n> As we assume everywhere that frames are made of an integral number of\n> lines, I would express the minimum and maximum frame size as a number of\n> lines, not a number of pixels.\n>\n\nFrom the above calculations, I think Jacopo did use lines as the units, but\nlabelled it as size :-)\n\n\n>\n> > > > > > > > From the CameraSensorInfo you already know the visibile\n> height and the\n> > > > > > > > line_length. Wouldn't be enough to add to that structure\n> > > > > > > > - maxFrameHeight = (mode_.height + max vblank)\n> > > > > > > > - frameIntegrationDiff\n> > > > > > > > This is sensor property and will be made available\n> in the\n> > > > > > > > sensor database. You can keep it in the IPA as\n> long as no\n> > > > > > > > sensor database is available\n> > > > > > > >\n> > > > > > > > To sum it up: is it enough for you to pass to the IPA the\n> maximum\n> > > > > > > > frame length (mode_.height + max vblank) in the\n> CameraSensorClass ?\n> > > > > > >\n> > > > > > > This is *mostly* correct. max frame size as calculated above\n> is fine.\n> > > > > > > However, I do also need min frame size. Your calculation\n> above assumes\n> > > > > > > min_vblank = 0. This may not be the case always, so min frame\n> size must be\n> > > > > > > computed and presented in CameraSensorInfo.\n> > > > > >\n> > > > > > Correct, I had assumed min_vblank = frameIntegrationDiff, which\n> might\n> > > > > > not always be the case.\n> > > > > >\n> > > > > > Considering the requirement for 'updates' and the fact the\n> > > > > > CameraSensorInfo is guaranteed to be fresh at IPA::Configure()\n> time,\n> > > > > > I would be happy to see minFrameSize (or Height) and\n> maxFrameSize (or\n> > > > > > Height) added there an computed once for all IPAs in the\n> CameraSensor\n> > > > > > class.\n> > > > >\n> > > > > Great, I can put together a change for this and submit for review\n> as part\n> > > > > of this series. One question, what name would you and Laurent\n> prefer for\n> > > > > this field - maxFrameSize or maxFrameLength (and equivalent for\n> min)?\n> > > > > Generally, I think sensors prefer using frame length in their\n> terminology,\n> > > > > but v4l2 probably prefers frame size. I am ok with either.\n>\n> I'd use *Length or *Height here, as we usually use *Size for\n> two-dimensional sizes.\n>\n\n*Length it is!\n\n\n>\n> > > > This and also if we want to report only the frame heights limits\n> > > > and let the IPA compute the frame size, or report the actual frame\n> > > > size. It's a detail, I know, you can easily get from one to the other\n> > > > and viceversa..\n> > >\n> > > So just to confirm before I make a submission, is something like this\n> what\n> > > you had in mind?\n> > >\n> > > diff --git a/src/libcamera/camera_sensor.cpp\n> b/src/libcamera/camera_sensor.cpp\n> > > index 251691aedfa3..6037f5f72897 100644\n> > > --- a/src/libcamera/camera_sensor.cpp\n> > > +++ b/src/libcamera/camera_sensor.cpp\n> > > /**\n> > > * \\class CameraSensor\n> > > * \\brief A camera sensor based on V4L2 subdevices\n> > > @@ -698,11 +728,12 @@ int CameraSensor::sensorInfo(CameraSensorInfo\n> *info) const\n> > >\n> > > /*\n> > > * Retrieve the pixel rate and the line length through V4L2\n> controls.\n> > > - * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK\n> controls is\n> > > - * mandatory.\n> > > + * Support for the V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and\n> > > + * V4L2_CID_VBLANK controls is mandatory.\n> > > */\n> > > ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,\n> > > - V4L2_CID_HBLANK });\n> > > + V4L2_CID_HBLANK,\n> > > + V4L2_CID_VBLANK });\n> >\n> > Please be aware of\n> > https://patchwork.libcamera.org/patch/10937/\n> >\n> > You can rebase on that patch or do something similar and the first\n> > that lands it wins :)\n> >\n> > > if (ctrls.empty()) {\n> > > LOG(CameraSensor, Error)\n> > > << \"Failed to retrieve camera info controls\";\n> > > @@ -713,6 +744,10 @@ int CameraSensor::sensorInfo(CameraSensorInfo\n> *info) const\n> > > info->lineLength = info->outputSize.width + hblank;\n> > > info->pixelRate =\n> ctrls.get(V4L2_CID_PIXEL_RATE).get();\n> > >\n> > > + const ControlInfo vblank =\n> ctrls.infoMap()->at(V4L2_CID_VBLANK);\n> > > + info->minFrameLength = info->outputSize.height +\n> vblank.min().get();\n> > > + info->maxFrameLength = info->outputSize.height +\n> vblank.max().get();\n> > > +\n> >\n> > That's what I had in mind, yes!\n> >\n> > Works for me, let's see what Laurent thinks\n>\n> This looks good to me.\n>\n> > > return 0;\n> > > }\n> > >\n> > > Let me know if you think that is suitable or not.\n> > >\n> > > > > > > > > > We would still have the issue of\n> > > > > > > > > > retrieving this value, and I can see multiple options:\n> > > > > > > > > >\n> > > > > > > > > > - Compute it once at initialization from vblank limits +\n> analog crop\n> > > > > > > > >\n> > > > > > > > > This is assuming that vblank + crop height is always going\n> to be equal to\n> > > > > > > > > frame length? We need to think if that is strictly true\n> for all sensors,\n> > > > > > > > > I'm not sure.\n>\n> It depends on how we define vblank. Its current purpose in libcamera is\n> to control the frame rate, so from that point of view isn't it\n> effectively always the frame length minus the active height ?\n>\n\nYes, this is how I have always treated it in the calculations.\n\n\n>\n> > > > > > > > > > - Extend V4L2 with a new vertical total size control and\n> use it (that\n> > > > > > > > > > will be more work as we need to synchronize with the\n> kernel changes)\n> > > > > > > > >\n> > > > > > > > > I expect this would have the longest lead time :-)\n>\n> Yes, I'm not looking forward to that :-S\n>\n> > > > > > > > > > - Hardcode the value in a sensor database in libcamera,\n> bypassing the\n> > > > > > > > > > kernel completly\n> > > > > > > > >\n> > > > > > > > > This is possible, but...\n> > > > > > > > >\n> > > > > > > > > > One question that I'm not sure I can answer by myself is\n> whether we can\n> > > > > > > > > > rely on the vertical total size limits being constant.\n> > > > > > > > >\n> > > > > > > > > ... I don't think you can assume total vertical size\n> (frame length) is\n> > > > > > > > > going to be the same for all modes a sensor advertises.\n>\n> That's my worry as well. Do you have any example of a sensor that\n> wouldn't guarantee the maximum frame length being a constant ?\n>\n\nThere is one example I can think of, but it's a pretty weak one. For the\nimx477, we have a long exposure mode (hence very very long frame length)\napplied to the full resolution mode. This long exposure mode is not\nenabled for binned modes, so the frame length is advertised as much\nshorter. I say weak example because I did not see much need to run long\nexposures in binned mode, and thereby artificially forced the user to use\nfull res mode instead.\n\n\n>\n> > > > > > > > > In this case, the\n> > > > > > > > > sensor database would have to know about every mode\n> available in the kernel\n> > > > > > > > > driver. This is something we moved away from with the\n> Raspberry Pi\n> > > > > > > > > CamHelper, so I doubt you want to re-introduce that link.\n>\n> Fully agreed, I don't want that either. If we decide to solve this\n> through a camera sensor database, we would need to encode how the sensor\n> handles the frame length constraints, not their numerical values for\n> specific modes. For sensors that have a fixed maximum frame length it\n> would just be a number, for other sensors it may be a few numerical\n> constants for a fixed formula, or even sensor-specific code that would\n> take as input the sensor mode.\n>\n> > > > > > > > > My feeling is that we could work something out by forcing\n> a \"refresh\" of\n> > > > > > > > > the cached controls on every configure, and I think that\n> should cover our\n> > > > > > > > > (or at least Raspberry Pi's in this particular instance)\n> usage.\n> > > > > > > > >\n> > > > > > > > > Let me know your thoughts.\n>\n> Let's start with a refresh, through the CameraSensor class, as discussed\n> above. This will move the problem away from pipeline handlers and IPA\n> modules, and will allow us to switch to a different method to retrieve\n> (or compute) the information later if desired.\n>\n\nSure, I will prepare a change to the CameraSensor class and submit as part\nof the next version.\n\nRegards,\nNaush\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 6A0B6BD808\n\tfor ;\n\tWed, 27 Jan 2021 11:39:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D227F68370;\n\tWed, 27 Jan 2021 12:39:14 +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 B8D2A60F1F\n\tfor ;\n\tWed, 27 Jan 2021 12:39:12 +0100 (CET)","by mail-lf1-x129.google.com with SMTP id v24so2177094lfr.7\n\tfor ;\n\tWed, 27 Jan 2021 03:39: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=\"aI2GFo0T\"; 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=n4nXD8vMKIzidkug0bP7aVSKb8v2H1hL3WTULbGAoaE=;\n\tb=aI2GFo0TaBi1LIGG3J9nKJf81Prmd1SvuiaIKN3jTA8PxJbzEFLx3e8hlUYoCzY9sI\n\tiF8837NVeS9vDv2HDLYFZHzZB3xjl4ti4KWXncP/Yx7srvdqP4Jl/3u1tns9OLuhsq+Y\n\tb2VhWHBNtGQAT1raD0UPjiK8Ff1mM28ntDk2vYn1iPPv9Ey8att4PLhLnVMeqQo07hVj\n\t/z5IkcUMrI/qYSFmAQk37G2YVfrnRQmvOoaALEVmQ6G0IQPe6wpgGrCgksW5MJOaWtt/\n\tt0QHRNy2fqnnIpbYujx/uP4FEDUMI4vgw8TN/7qshemhq/sF/TMdYL+gMRqcOwzKu7Or\n\tgplg==","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=n4nXD8vMKIzidkug0bP7aVSKb8v2H1hL3WTULbGAoaE=;\n\tb=ZbW+6QEq9w6M9kXxMPHHsclH0KkMjQN3ItoZK/t2IDqlbjvrkbZtraD5NvuZK2qBeP\n\tDCVZRf24SxfsmR/wc76Pk3JzE9DGLjawXp2l7S+bosGgA2LrXAoxoO34oGRD2tZFLnfG\n\t6NPNxZPuM8s4lWfvLi2ckc4OLAPzuxLY3C9+1EIXXpMXxBA0mV7qv1gfh8L1XaqLWRLq\n\txbULz4IqcJyxQWJHrhi7gslaOOylAK3yhFnjFe6BX2rgjshoyPgw6HVsH2FtbWHBqsRx\n\tqmXp+krly/fG69+UECiGIGo7Cq6fXAcFT5zEZjGo+0q3IsZBQiR1GHIdYedToutEAVcw\n\tJ4aw==","X-Gm-Message-State":"AOAM5323JBh7GSUK6uEXYobn87m9Ys+f/JiLq0UAUY38JHuBsrp/5U6D\n\t0eJGB9ZAqurH17g9cpXvoMmwOVarOt9N1FrHiKc6LlEiqg3fvuvu","X-Google-Smtp-Source":"ABdhPJwhzCPJMJeVN/LAvhqoh/src4cPXqm1iA58P5Nkv9v8B8siHxROgnwlVoJWew3t/9IgQYAp3JV2EYlsnmRnRQM=","X-Received":"by 2002:ac2:44a3:: with SMTP id c3mr4730662lfm.210.1611747551944;\n\tWed, 27 Jan 2021 03:39:11 -0800 (PST)","MIME-Version":"1.0","References":"<20210124140506.786503-2-naush@raspberrypi.com>\n\t\n\t\n\t<20210126134459.zcwgiklzqgaahbr2@uno.localdomain>\n\t\n\t<20210126142205.mvw7o2bl2gxf3ghl@uno.localdomain>\n\t\n\t<20210126143929.ymiiuxz22gp3swso@uno.localdomain>\n\t\n\t<20210126162045.std5ykrgfryx35t4@uno.localdomain>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 27 Jan 2021 11:38:55 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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=\"===============1046564986267139601==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]