[{"id":17548,"web_url":"https://patchwork.libcamera.org/comment/17548/","msgid":"","date":"2021-06-15T03:19:08","subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Allow long\n\texposure modes for imx477.","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Mon, Jun 14, 2021 at 11:00:37AM +0100, Naushir Patuck wrote:\n> Update the imx477 CamHelper to use long exposure modes if needed.\n> This is done by overloading the CamHelper::GetVBlanking function to return a\n> frame length (and vblank value) computed using a scaling factor when the value\n> would be larger than what the sensor register could otherwise hold.\n> \n> CamHelperImx477::Prepare is also overloaded to ensure that the \"device.status\"\n> metadata returns the right value if the long exposure scaling factor is use.\n\ns/use/used/\n\n> The scaling factor is unfortunately not returned back in metadata.\n> \n> With the current imx477 driver, we can achieve a maximum exposure time of approx\n> 127 seconds since the HBLANK control is read-only.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 67 +++++++++++++++++++++++\n> 1 file changed, 67 insertions(+)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 7a1100c25afc..549c2620765a 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -6,6 +6,7 @@\n> */\n> \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -14,6 +15,7 @@\n> #include \"md_parser.hpp\"\n> \n> using namespace RPiController;\n> +using libcamera::utils::Duration;\n> \n> /*\n> * We care about two gain registers and a pair of exposure registers. Their\n> @@ -30,6 +32,9 @@ public:\n> \tCamHelperImx477();\n> \tuint32_t GainCode(double gain) const override;\n> \tdouble Gain(uint32_t gain_code) const override;\n> +\tvoid Prepare(libcamera::Span buffer, Metadata &metadata) override;\n> +\tuint32_t GetVBlanking(Duration &exposure, Duration minFrameDuration,\n> +\t\t\t Duration maxFrameDuration) const override;\n> \tvoid GetDelays(int &exposure_delay, int &gain_delay,\n> \t\t int &vblank_delay) const override;\n> \tbool SensorEmbeddedDataPresent() const override;\n> @@ -40,6 +45,10 @@ private:\n> \t * in units of lines.\n> \t */\n> \tstatic constexpr int frameIntegrationDiff = 22;\n> +\t/* Maximum frame length allowable for long exposure calculations. */\n> +\tstatic constexpr int frameLengthMax = 0xffdc;\n> +\t/* Largest long exposure scale factor given as a left shift on the frame length. */\n> +\tstatic constexpr int longExposureShiftMax = 7;\n> \n> \tMdParserSmia imx477_parser;\n> \n> @@ -72,6 +81,64 @@ double CamHelperImx477::Gain(uint32_t gain_code) const\n> \treturn 1024.0 / (1024 - gain_code);\n> }\n> \n> +void CamHelperImx477::Prepare(libcamera::Span buffer, Metadata &metadata)\n> +{\n> +\tDeviceStatus deviceStatus, parsedStatus;\n> +\n> +\t/* Get the device status provided by DelayedControls */\n> +\tif (metadata.Get(\"device.status\", deviceStatus) != 0)\n> +\t\treturn;\n> +\n> +\t/* Get the device status provided by the embedded data buffer. */\n> +\tCamHelper::parseEmbeddedData(buffer, metadata);\n> +\tmetadata.Get(\"device.status\", parsedStatus);\n> +\n> +\t/*\n> +\t * If the ratio of DelayedControls to embedded data shutter speed is > 1\n> +\t * and is a factor of 2^N, then we can assume this is a long exposure mode\n> +\t * frame. Since embedded data does not provide any hints of long exposure\n> +\t * modes, make sure we use the DelayedControls values in the metadata.\n> +\t * Otherwise, just go with the embedded data values.\n> +\t */\n> +\tunsigned long ratio = std::lround(deviceStatus.shutter_speed / parsedStatus.shutter_speed);\n> +\tbool replace = (ratio > 1) && ((ratio & (~ratio + 1)) == ratio);\n> +\n> +\tif (replace)\n> +\t\tmetadata.Set(\"device.status\", deviceStatus);\n> +}\n> +\n> +uint32_t CamHelperImx477::GetVBlanking(Duration &exposure,\n> +\t\t\t\t Duration minFrameDuration,\n> +\t\t\t\t Duration maxFrameDuration) const\n> +{\n> +\tuint32_t frameLength, exposureLines;\n> +\tunsigned int shift = 0;\n> +\n> +\tframeLength = mode_.height + CamHelper::GetVBlanking(exposure, minFrameDuration,\n> +\t\t\t\t\t\t\t maxFrameDuration);\n> +\t/*\n> +\t * Check if the frame length calculated needs to be setup for long\n> +\t * exposure mode. This will require us to use a long exposure scale\n> +\t * factor provided by a shift operation in the sensor.\n> +\t */\n> +\twhile (frameLength > frameLengthMax) {\n> +\t\tif (++shift > longExposureShiftMax) {\n> +\t\t\tshift = longExposureShiftMax;\n> +\t\t\tframeLength = frameLengthMax;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tframeLength >>= 1;\n> +\t}\n> +\n> +\t/* Account for any rounding in the scaled frame length value. */\n> +\tframeLength <<= shift;\n> +\texposureLines = CamHelper::ExposureLines(exposure);\n> +\texposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);\n> +\texposure = CamHelper::Exposure(exposureLines);\n\nI'll try to review this series in more details shortly, but I'd like to\nalready take this as an opportunity to discuss an idea related to the\ncam helpers.\n\nAs they stand today, the cam helpers translate between natural units\n(time for exposure, and a linear multipler for gain) and V4L2 control\nvalues. This pushes knowledge of V4L2 to the IPA, which isn't great.\nI've been toying with the idea of gathering all the V4L2 knowledge in\nthe libcamera core, which would move these conversions to the pipeline\nhandler (with the helper of camera helpers on the libcamera side of\ncourse).\n\nOne issue that bothered me is that the IPA wouldn't know about rounding\n(or at least not right away, it could get information back from the\npipeline handler, but with a delay, and in any case that wouldn't be\nvery clean). The code above takes rounding into account. I'd thus like\nyour feedback on how important this is, and, in case (as I suspect) the\nIPA needs to know about rounding, whether you think there would still be\na way to not add V4L2 knowledge to the IPA.\n\n> +\n> +\treturn frameLength - mode_.height;\n> +}\n> +\n> void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,\n> \t\t\t\tint &vblank_delay) const\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 5C190C3218\n\tfor ;\n\tTue, 15 Jun 2021 03:19:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC28F68944;\n\tTue, 15 Jun 2021 05:19:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F1D416050D\n\tfor ;\n\tTue, 15 Jun 2021 05:19:28 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 67DEF436;\n\tTue, 15 Jun 2021 05:19:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vhr4+gRr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623727168;\n\tbh=M1VKdXXph/mfhAQO/pqgS5+J4PnLDsQH28L/ONWVxpM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vhr4+gRrf+YOqMJaknCdrwQgBHcpqWrHlH98s97pE4qX4AUu7SZMSk0ChfYY0tLe6\n\turix/Mot0cXhc4WTxmx5P+NqNw8JXXArSjbqI9m600OhQW3X5WxhTMBN+RkgEHHudA\n\t+xzYfTu9UnVNQ66Jl0fQcspEVSEXdv87ffaGAVLU=","Date":"Tue, 15 Jun 2021 06:19:08 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210614100040.3054433-1-naush@raspberrypi.com>\n\t<20210614100040.3054433-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210614100040.3054433-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Allow long\n\texposure modes for imx477.","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17567,"web_url":"https://patchwork.libcamera.org/comment/17567/","msgid":"","date":"2021-06-15T12:55:55","subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Allow long\n\texposure modes for imx477.","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\n\nOn Tue, 15 Jun 2021 at 04:19, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Jun 14, 2021 at 11:00:37AM +0100, Naushir Patuck wrote:\n> > Update the imx477 CamHelper to use long exposure modes if needed.\n> > This is done by overloading the CamHelper::GetVBlanking function to\n> return a\n> > frame length (and vblank value) computed using a scaling factor when the\n> value\n> > would be larger than what the sensor register could otherwise hold.\n> >\n> > CamHelperImx477::Prepare is also overloaded to ensure that the\n> \"device.status\"\n> > metadata returns the right value if the long exposure scaling factor is\n> use.\n>\n> s/use/used/\n>\n> > The scaling factor is unfortunately not returned back in metadata.\n> >\n> > With the current imx477 driver, we can achieve a maximum exposure time\n> of approx\n> > 127 seconds since the HBLANK control is read-only.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 67 +++++++++++++++++++++++\n> > 1 file changed, 67 insertions(+)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index 7a1100c25afc..549c2620765a 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -6,6 +6,7 @@\n> > */\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -14,6 +15,7 @@\n> > #include \"md_parser.hpp\"\n> >\n> > using namespace RPiController;\n> > +using libcamera::utils::Duration;\n> >\n> > /*\n> > * We care about two gain registers and a pair of exposure registers.\n> Their\n> > @@ -30,6 +32,9 @@ public:\n> > CamHelperImx477();\n> > uint32_t GainCode(double gain) const override;\n> > double Gain(uint32_t gain_code) const override;\n> > + void Prepare(libcamera::Span buffer, Metadata\n> &metadata) override;\n> > + uint32_t GetVBlanking(Duration &exposure, Duration\n> minFrameDuration,\n> > + Duration maxFrameDuration) const override;\n> > void GetDelays(int &exposure_delay, int &gain_delay,\n> > int &vblank_delay) const override;\n> > bool SensorEmbeddedDataPresent() const override;\n> > @@ -40,6 +45,10 @@ private:\n> > * in units of lines.\n> > */\n> > static constexpr int frameIntegrationDiff = 22;\n> > + /* Maximum frame length allowable for long exposure calculations.\n> */\n> > + static constexpr int frameLengthMax = 0xffdc;\n> > + /* Largest long exposure scale factor given as a left shift on the\n> frame length. */\n> > + static constexpr int longExposureShiftMax = 7;\n> >\n> > MdParserSmia imx477_parser;\n> >\n> > @@ -72,6 +81,64 @@ double CamHelperImx477::Gain(uint32_t gain_code) const\n> > return 1024.0 / (1024 - gain_code);\n> > }\n> >\n> > +void CamHelperImx477::Prepare(libcamera::Span buffer,\n> Metadata &metadata)\n> > +{\n> > + DeviceStatus deviceStatus, parsedStatus;\n> > +\n> > + /* Get the device status provided by DelayedControls */\n> > + if (metadata.Get(\"device.status\", deviceStatus) != 0)\n> > + return;\n> > +\n> > + /* Get the device status provided by the embedded data buffer. */\n> > + CamHelper::parseEmbeddedData(buffer, metadata);\n> > + metadata.Get(\"device.status\", parsedStatus);\n> > +\n> > + /*\n> > + * If the ratio of DelayedControls to embedded data shutter speed\n> is > 1\n> > + * and is a factor of 2^N, then we can assume this is a long\n> exposure mode\n> > + * frame. Since embedded data does not provide any hints of long\n> exposure\n> > + * modes, make sure we use the DelayedControls values in the\n> metadata.\n> > + * Otherwise, just go with the embedded data values.\n> > + */\n> > + unsigned long ratio = std::lround(deviceStatus.shutter_speed /\n> parsedStatus.shutter_speed);\n> > + bool replace = (ratio > 1) && ((ratio & (~ratio + 1)) == ratio);\n> > +\n> > + if (replace)\n> > + metadata.Set(\"device.status\", deviceStatus);\n> > +}\n> > +\n> > +uint32_t CamHelperImx477::GetVBlanking(Duration &exposure,\n> > + Duration minFrameDuration,\n> > + Duration maxFrameDuration) const\n> > +{\n> > + uint32_t frameLength, exposureLines;\n> > + unsigned int shift = 0;\n> > +\n> > + frameLength = mode_.height + CamHelper::GetVBlanking(exposure,\n> minFrameDuration,\n> > +\n> maxFrameDuration);\n> > + /*\n> > + * Check if the frame length calculated needs to be setup for long\n> > + * exposure mode. This will require us to use a long exposure scale\n> > + * factor provided by a shift operation in the sensor.\n> > + */\n> > + while (frameLength > frameLengthMax) {\n> > + if (++shift > longExposureShiftMax) {\n> > + shift = longExposureShiftMax;\n> > + frameLength = frameLengthMax;\n> > + break;\n> > + }\n> > + frameLength >>= 1;\n> > + }\n> > +\n> > + /* Account for any rounding in the scaled frame length value. */\n> > + frameLength <<= shift;\n> > + exposureLines = CamHelper::ExposureLines(exposure);\n> > + exposureLines = std::min(exposureLines, frameLength -\n> frameIntegrationDiff);\n> > + exposure = CamHelper::Exposure(exposureLines);\n>\n> I'll try to review this series in more details shortly, but I'd like to\n> already take this as an opportunity to discuss an idea related to the\n> cam helpers.\n>\n> As they stand today, the cam helpers translate between natural units\n> (time for exposure, and a linear multipler for gain) and V4L2 control\n> values. This pushes knowledge of V4L2 to the IPA, which isn't great.\n> I've been toying with the idea of gathering all the V4L2 knowledge in\n> the libcamera core, which would move these conversions to the pipeline\n> handler (with the helper of camera helpers on the libcamera side of\n> course).\n>\n> One issue that bothered me is that the IPA wouldn't know about rounding\n> (or at least not right away, it could get information back from the\n> pipeline handler, but with a delay, and in any case that wouldn't be\n> very clean). The code above takes rounding into account. I'd thus like\n> your feedback on how important this is, and, in case (as I suspect) the\n> IPA needs to know about rounding, whether you think there would still be\n> a way to not add V4L2 knowledge to the IPA.\n>\n\nThis is purely from the perspective of the Raspberry Pi IPA, so other\nvendors\nmay have different opinions or requirements.\n\nFor the RPi IPA, the key is that we get given the *actual* shutter speed\nand gain\nvalues used by the sensor for that frame, and these may or may not be\nrounded\nfrom what the AGC originally requested. When fetching shutter speed and\ngain\nvalues from embedded data, this is implicit. When no embedded data is\navailable,\nthe CamHelper does the rounding before passing the values into\nDelayedControls,\nand the value given back to the IPA when the frame comes through (with\nappropriate\nframe delay) will also be rounded. So, if this CamHelper functionality were\nto be\nmoved to the pipeline handler domain mostly as-is, we would see no ill\neffect. If the\nIPA were to somehow get shutter speed and gain values that were not rounded\nappropriately, you would likely see some tiny oscillations in the AGC, and\nDavid\nwould not be happy with that :-)\n\nSo in summary, I don't think the rounding will give us any problems,\nother IPAs\nmight have to take a similar approach to ours to claim the same. I do think\nremoving\nthese sorts of conversions from the IPA domain is the right thing to do.\nHaving the\nIPA deal with only natural units will be a welcome change!\n\nRegards,\nNaush\n\n\n>\n> > +\n> > + return frameLength - mode_.height;\n> > +}\n> > +\n> > void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,\n> > int &vblank_delay) const\n> > {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 377D3BD78E\n\tfor ;\n\tTue, 15 Jun 2021 12:56:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7C096029D;\n\tTue, 15 Jun 2021 14:56:14 +0200 (CEST)","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 DCBAF6029A\n\tfor ;\n\tTue, 15 Jun 2021 14:56:12 +0200 (CEST)","by mail-lf1-x130.google.com with SMTP id j2so26726566lfg.9\n\tfor ;\n\tTue, 15 Jun 2021 05:56:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"fCja2dA2\"; 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=wGuKZ7TLF0HRAl1OyJ4/hlZ57PsAobH1h7//GMvUORI=;\n\tb=fCja2dA2+YyXqSqhCrBaVKNjeUh2Jsljb42XB242LPCq7wwd1kCikVKYr9TGGg+JcZ\n\tZrz1PeDFKhgwpwMny8/ZzxRnDuc+L4bLPp52lZ3aOWH5ZIzUQQR2zAX623J0cNqrw8mj\n\tGq5oLuDgi4EgAISyQMlLFfWignVUbmi7lwTCZil+HLn3W6LMp6sJLpnr8UuIs2MtiXtb\n\tCcESlwDtUJUfqaK8BSQfQrcgal8NfuCHpZvJyzoPDWuriHrUwIKhBH0jGGd85G1F4GKy\n\tGwa+24D0Pi/IAGhFbIHPR5YrQLXA1TftHS8yNInu5f7uXaqAXs2+rUWVjao7si8srXbf\n\tT3qg==","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=wGuKZ7TLF0HRAl1OyJ4/hlZ57PsAobH1h7//GMvUORI=;\n\tb=A27vqzHbnj5dg2o2H11twSlXbN+rEFcPGyLK8FIEyhUmV30v+HBCiABnbHdYAfjApN\n\tzvegChuNEYLpSz9gKaTocrxEm8AOkWLi5Ff9g5nWAlAndscZOTglxkAMiNsVUUmfBTLn\n\tnOh60YQdGqyIOLHE4V7wMCh/yQGbp7RlpJ8Fu84TtU3f28UQW62aIdDftgxSPYCMcjbR\n\tTwsqOg2zlBxQKbXw8MU59Y6wW0h+inx1x5tCH/dTWEZmxif1NJwg8tDtRlUMSryWPbPs\n\tWZXVVOX8+aPu/Va9ubn4oD5gwPs0CjdsnF6XozkmHKlz3shJ6RSCFbH0i4I2TIwW6tMm\n\tJ2VA==","X-Gm-Message-State":"AOAM53091F0KSRFdQepnj6F035DDxmGdA7ckKsONnGLx7Uin5eMEbyuk\n\tbdnYxj68hARRG0WF4H2KjXUf4Cu4Ulf5VjmDfgk/vNhT9qB5Yw==","X-Google-Smtp-Source":"ABdhPJyBXufYFiTQjdUYRZxsEedBJKj65GjYpOhuLnXgozRlTXmxkWTmy3n7hsSoX+GquejPA46L+MLnBhJKDkdsm5M=","X-Received":"by 2002:a05:6512:3fa5:: with SMTP id\n\tx37mr15680318lfa.617.1623761771893; \n\tTue, 15 Jun 2021 05:56:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20210614100040.3054433-1-naush@raspberrypi.com>\n\t<20210614100040.3054433-2-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 15 Jun 2021 13:55:55 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000009d2afb05c4cd7fa4\"","Subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Allow long\n\texposure modes for imx477.","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17568,"web_url":"https://patchwork.libcamera.org/comment/17568/","msgid":"","date":"2021-06-15T13:27:52","subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Allow long\n\texposure modes for imx477.","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for this feature!\n\nOn Mon, 14 Jun 2021 at 11:00, Naushir Patuck wrote:\n>\n> Update the imx477 CamHelper to use long exposure modes if needed.\n> This is done by overloading the CamHelper::GetVBlanking function to return a\n> frame length (and vblank value) computed using a scaling factor when the value\n> would be larger than what the sensor register could otherwise hold.\n>\n> CamHelperImx477::Prepare is also overloaded to ensure that the \"device.status\"\n> metadata returns the right value if the long exposure scaling factor is use.\n> The scaling factor is unfortunately not returned back in metadata.\n>\n> With the current imx477 driver, we can achieve a maximum exposure time of approx\n> 127 seconds since the HBLANK control is read-only.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 67 +++++++++++++++++++++++\n> 1 file changed, 67 insertions(+)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 7a1100c25afc..549c2620765a 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -6,6 +6,7 @@\n> */\n>\n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -14,6 +15,7 @@\n> #include \"md_parser.hpp\"\n>\n> using namespace RPiController;\n> +using libcamera::utils::Duration;\n>\n> /*\n> * We care about two gain registers and a pair of exposure registers. Their\n> @@ -30,6 +32,9 @@ public:\n> CamHelperImx477();\n> uint32_t GainCode(double gain) const override;\n> double Gain(uint32_t gain_code) const override;\n> + void Prepare(libcamera::Span buffer, Metadata &metadata) override;\n> + uint32_t GetVBlanking(Duration &exposure, Duration minFrameDuration,\n> + Duration maxFrameDuration) const override;\n> void GetDelays(int &exposure_delay, int &gain_delay,\n> int &vblank_delay) const override;\n> bool SensorEmbeddedDataPresent() const override;\n> @@ -40,6 +45,10 @@ private:\n> * in units of lines.\n> */\n> static constexpr int frameIntegrationDiff = 22;\n> + /* Maximum frame length allowable for long exposure calculations. */\n> + static constexpr int frameLengthMax = 0xffdc;\n> + /* Largest long exposure scale factor given as a left shift on the frame length. */\n> + static constexpr int longExposureShiftMax = 7;\n>\n> MdParserSmia imx477_parser;\n>\n> @@ -72,6 +81,64 @@ double CamHelperImx477::Gain(uint32_t gain_code) const\n> return 1024.0 / (1024 - gain_code);\n> }\n>\n> +void CamHelperImx477::Prepare(libcamera::Span buffer, Metadata &metadata)\n> +{\n> + DeviceStatus deviceStatus, parsedStatus;\n> +\n> + /* Get the device status provided by DelayedControls */\n> + if (metadata.Get(\"device.status\", deviceStatus) != 0)\n> + return;\n> +\n> + /* Get the device status provided by the embedded data buffer. */\n> + CamHelper::parseEmbeddedData(buffer, metadata);\n> + metadata.Get(\"device.status\", parsedStatus);\n> +\n> + /*\n> + * If the ratio of DelayedControls to embedded data shutter speed is > 1\n> + * and is a factor of 2^N, then we can assume this is a long exposure mode\n> + * frame. Since embedded data does not provide any hints of long exposure\n> + * modes, make sure we use the DelayedControls values in the metadata.\n> + * Otherwise, just go with the embedded data values.\n> + */\n> + unsigned long ratio = std::lround(deviceStatus.shutter_speed / parsedStatus.shutter_speed);\n> + bool replace = (ratio > 1) && ((ratio & (~ratio + 1)) == ratio);\n\nIs there a case we need to worry about where \"ratio\" was only rounded\nto a power of 2, but wouldn't otherwise have been one? (e.g.\ndeviceStatus.shutter_speed = 11, parsedStatus.shutter_speed = 5)\n\n> +\n> + if (replace)\n> + metadata.Set(\"device.status\", deviceStatus);\n> +}\n> +\n> +uint32_t CamHelperImx477::GetVBlanking(Duration &exposure,\n> + Duration minFrameDuration,\n> + Duration maxFrameDuration) const\n> +{\n> + uint32_t frameLength, exposureLines;\n> + unsigned int shift = 0;\n> +\n> + frameLength = mode_.height + CamHelper::GetVBlanking(exposure, minFrameDuration,\n> + maxFrameDuration);\n> + /*\n> + * Check if the frame length calculated needs to be setup for long\n> + * exposure mode. This will require us to use a long exposure scale\n> + * factor provided by a shift operation in the sensor.\n> + */\n> + while (frameLength > frameLengthMax) {\n> + if (++shift > longExposureShiftMax) {\n> + shift = longExposureShiftMax;\n> + frameLength = frameLengthMax;\n> + break;\n> + }\n> + frameLength >>= 1;\n> + }\n> +\n> + /* Account for any rounding in the scaled frame length value. */\n> + frameLength <<= shift;\n> + exposureLines = CamHelper::ExposureLines(exposure);\n> + exposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);\n> + exposure = CamHelper::Exposure(exposureLines);\n\nAny reason for invoking the base class functions explicitly here? Not\nthat it bothers me, just curious...\n\nSubject to clarification on my ratio rounding question:\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n> +\n> + return frameLength - mode_.height;\n> +}\n> +\n> void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,\n> int &vblank_delay) const\n> {\n> --\n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0A789C3218\n\tfor ;\n\tTue, 15 Jun 2021 13:28:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E1F868946;\n\tTue, 15 Jun 2021 15:28:07 +0200 (CEST)","from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com\n\t[IPv6:2a00:1450:4864:20::42c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 99A506029A\n\tfor ;\n\tTue, 15 Jun 2021 15:28:05 +0200 (CEST)","by mail-wr1-x42c.google.com with SMTP id y7so18332458wrh.7\n\tfor ;\n\tTue, 15 Jun 2021 06:28:05 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Sw3IVXHS\"; 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=EybyIQFUpvIGJZjPjb14x4T4i6VMAR1ZpBxnrUiVm24=;\n\tb=Sw3IVXHSsqFFQVN+IbqtINIOBZJHwLlnF0F2U81hsGCYWZtOtp+RoaJbzelcz+4m9p\n\tB0secRtIx4IgCe137g1d4LOsXdKAU617z9+ItD4nGD/coLJQCZYzfLJsmRyCdzqaX+nZ\n\tuEAcb8IGiu4Py8RJft2u5XcBfKMNCNPsr+hMGqmsD2Gw+0F6PFF80LYXY3+M+4XCVdjt\n\tg4Z7D2PPdZ8QXQL3L2S8G6ap14p4FaD1sc6PXnSiUZh5L43L+6xfn3khsqfAheL7UiJz\n\tcRt+jZ2uzNZxBoK51m3iMP4SYtKNdwiLoSxHKXdVdYsRehtELzyCVvgvt+jbWYH7pqrA\n\tpONg==","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=EybyIQFUpvIGJZjPjb14x4T4i6VMAR1ZpBxnrUiVm24=;\n\tb=DcWne5DImPC122Kalg09fZNfZwAnvAd2Xmek82LL/lHa+gh88HNBF/fmWNZ6oXHp+a\n\tKN7Q1QPOFjPfpvZW6XPagmtE/HhTmOiCD1Y52Dtl+5Vv4qvglExqFZX/kWnnhXLo2KnU\n\taEr+IWCsv8Rre13NShUIZnNQwwSB1c02EQWV0F8AkraiD6Ahaj1p8OOi1ukzuG9b3bY/\n\tpWbGYOU0m6Kdu0F6ZGLGgwAZbtswhp1lgMmIo+G4tkRTE1J4vhLsZJSekTkEqwN8klxE\n\tKKl7EnkblMHc8mxdAmOol+qRMQ21wf7IySbZTP/JbNqEBuB4PvCoH7tPjkGJUKUi6uXV\n\t2Ztw==","X-Gm-Message-State":"AOAM533MgUgNjqZnXTUhAh+gxiopJjUPsNDYk1KNTv/83gWQuA2m8ZRV\n\tz5c68O6GZq8sd1V+wk+hYTd366K0uyYPTPBNK5CmfA==","X-Google-Smtp-Source":"ABdhPJyqVczL8FLoxh92VcjVlDaTL3oc7eNQ0KlYNH6xld0salpoABkX7iHjA9QFEXZH8wFuQUWot2sH1x2snRj2icY=","X-Received":"by 2002:adf:d0cb:: with SMTP id\n\tz11mr10257047wrh.236.1623763685135; \n\tTue, 15 Jun 2021 06:28:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20210614100040.3054433-1-naush@raspberrypi.com>\n\t<20210614100040.3054433-2-naush@raspberrypi.com>","In-Reply-To":"<20210614100040.3054433-2-naush@raspberrypi.com>","From":"David Plowman ","Date":"Tue, 15 Jun 2021 14:27:52 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Allow long\n\texposure modes for imx477.","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17575,"web_url":"https://patchwork.libcamera.org/comment/17575/","msgid":"","date":"2021-06-15T13:46:11","subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Allow long\n\texposure modes for imx477.","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your feedback!\n\nOn Tue, 15 Jun 2021 at 14:28, David Plowman \nwrote:\n\n> Hi Naush\n>\n> Thanks for this feature!\n>\n> On Mon, 14 Jun 2021 at 11:00, Naushir Patuck \n> wrote:\n> >\n> > Update the imx477 CamHelper to use long exposure modes if needed.\n> > This is done by overloading the CamHelper::GetVBlanking function to\n> return a\n> > frame length (and vblank value) computed using a scaling factor when the\n> value\n> > would be larger than what the sensor register could otherwise hold.\n> >\n> > CamHelperImx477::Prepare is also overloaded to ensure that the\n> \"device.status\"\n> > metadata returns the right value if the long exposure scaling factor is\n> use.\n> > The scaling factor is unfortunately not returned back in metadata.\n> >\n> > With the current imx477 driver, we can achieve a maximum exposure time\n> of approx\n> > 127 seconds since the HBLANK control is read-only.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 67 +++++++++++++++++++++++\n> > 1 file changed, 67 insertions(+)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index 7a1100c25afc..549c2620765a 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -6,6 +6,7 @@\n> > */\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -14,6 +15,7 @@\n> > #include \"md_parser.hpp\"\n> >\n> > using namespace RPiController;\n> > +using libcamera::utils::Duration;\n> >\n> > /*\n> > * We care about two gain registers and a pair of exposure registers.\n> Their\n> > @@ -30,6 +32,9 @@ public:\n> > CamHelperImx477();\n> > uint32_t GainCode(double gain) const override;\n> > double Gain(uint32_t gain_code) const override;\n> > + void Prepare(libcamera::Span buffer, Metadata\n> &metadata) override;\n> > + uint32_t GetVBlanking(Duration &exposure, Duration\n> minFrameDuration,\n> > + Duration maxFrameDuration) const override;\n> > void GetDelays(int &exposure_delay, int &gain_delay,\n> > int &vblank_delay) const override;\n> > bool SensorEmbeddedDataPresent() const override;\n> > @@ -40,6 +45,10 @@ private:\n> > * in units of lines.\n> > */\n> > static constexpr int frameIntegrationDiff = 22;\n> > + /* Maximum frame length allowable for long exposure\n> calculations. */\n> > + static constexpr int frameLengthMax = 0xffdc;\n> > + /* Largest long exposure scale factor given as a left shift on\n> the frame length. */\n> > + static constexpr int longExposureShiftMax = 7;\n> >\n> > MdParserSmia imx477_parser;\n> >\n> > @@ -72,6 +81,64 @@ double CamHelperImx477::Gain(uint32_t gain_code) const\n> > return 1024.0 / (1024 - gain_code);\n> > }\n> >\n> > +void CamHelperImx477::Prepare(libcamera::Span buffer,\n> Metadata &metadata)\n> > +{\n> > + DeviceStatus deviceStatus, parsedStatus;\n> > +\n> > + /* Get the device status provided by DelayedControls */\n> > + if (metadata.Get(\"device.status\", deviceStatus) != 0)\n> > + return;\n> > +\n> > + /* Get the device status provided by the embedded data buffer. */\n> > + CamHelper::parseEmbeddedData(buffer, metadata);\n> > + metadata.Get(\"device.status\", parsedStatus);\n> > +\n> > + /*\n> > + * If the ratio of DelayedControls to embedded data shutter\n> speed is > 1\n> > + * and is a factor of 2^N, then we can assume this is a long\n> exposure mode\n> > + * frame. Since embedded data does not provide any hints of long\n> exposure\n> > + * modes, make sure we use the DelayedControls values in the\n> metadata.\n> > + * Otherwise, just go with the embedded data values.\n> > + */\n> > + unsigned long ratio = std::lround(deviceStatus.shutter_speed /\n> parsedStatus.shutter_speed);\n> > + bool replace = (ratio > 1) && ((ratio & (~ratio + 1)) == ratio);\n>\n> Is there a case we need to worry about where \"ratio\" was only rounded\n> to a power of 2, but wouldn't otherwise have been one? (e.g.\n> deviceStatus.shutter_speed = 11, parsedStatus.shutter_speed = 5)\n>\n\nThe ratio must be a power of 2 or exactly 1 for the imx477. Any other value\nand something has gone wrong :-) In the failure case, I assume that the\nembedded data values are to be trusted and use them over DelatedControl\nvalues.\nPerhaps it may be worth adding a log point if this condition ever hits?\n\n\n>\n> > +\n> > + if (replace)\n> > + metadata.Set(\"device.status\", deviceStatus);\n> > +}\n> > +\n> > +uint32_t CamHelperImx477::GetVBlanking(Duration &exposure,\n> > + Duration minFrameDuration,\n> > + Duration maxFrameDuration) const\n> > +{\n> > + uint32_t frameLength, exposureLines;\n> > + unsigned int shift = 0;\n> > +\n> > + frameLength = mode_.height + CamHelper::GetVBlanking(exposure,\n> minFrameDuration,\n> > +\n> maxFrameDuration);\n> > + /*\n> > + * Check if the frame length calculated needs to be setup for\n> long\n> > + * exposure mode. This will require us to use a long exposure\n> scale\n> > + * factor provided by a shift operation in the sensor.\n> > + */\n> > + while (frameLength > frameLengthMax) {\n> > + if (++shift > longExposureShiftMax) {\n> > + shift = longExposureShiftMax;\n> > + frameLength = frameLengthMax;\n> > + break;\n> > + }\n> > + frameLength >>= 1;\n> > + }\n> > +\n> > + /* Account for any rounding in the scaled frame length value. */\n> > + frameLength <<= shift;\n> > + exposureLines = CamHelper::ExposureLines(exposure);\n> > + exposureLines = std::min(exposureLines, frameLength -\n> frameIntegrationDiff);\n> > + exposure = CamHelper::Exposure(exposureLines);\n>\n> Any reason for invoking the base class functions explicitly here? Not\n> that it bothers me, just curious...\n>\n\nI use CamHelper::GetVBlanking to compute the vblank and exposure values with\ncorrect clipping and rounding based on the frame length limits. I could\npossibly move\nthat entire calculation into this function, but it would be very similar,\nso thought it\nwould be better to call the base class implementation.\n\nRegards,\nNaush\n\n\n>\n> Subject to clarification on my ratio rounding question:\n>\n> Reviewed-by: David Plowman \n>\n> Thanks!\n> David\n>\n> > +\n> > + return frameLength - mode_.height;\n> > +}\n> > +\n> > void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,\n> > int &vblank_delay) const\n> > {\n> > --\n> > 2.25.1\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8A9C7C3218\n\tfor ;\n\tTue, 15 Jun 2021 13:46:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC4DF68946;\n\tTue, 15 Jun 2021 15:46:29 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A4DF6029A\n\tfor ;\n\tTue, 15 Jun 2021 15:46:29 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id p17so27117196lfc.6\n\tfor ;\n\tTue, 15 Jun 2021 06:46:28 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"h9cTgoxM\"; 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=pxP2b0XD07Tj3WpAeP5yFEzrhBuSn7tfRkuot6OAsu8=;\n\tb=h9cTgoxM6uRxJzE2iP5V19HsanjU6It17WOTpZ1/mSvmjjTG0R2pMmvWJb5GUjEm+y\n\tbTFx1Ea7qGft3H0w9wkFwWKSD16aVZv1OcgslZl2n0xPbOd0C/484Cbplfppnfo0nHJv\n\t1bcR1miEa8InO9HIqz2v2wdo3ouockwBVav85+ZLaebYO8kzfogofxi3uaUy3LcnPH92\n\tp1WmHhbVMVgB+63E99jHR0yetHVFs85IIlFtMyZYcdLgvtmPxefjKuOaDXXSI1hTq2H0\n\tQ6Wvs2wx3zkr0l2lx0PvDLpk1cA7BOnkDfiZAVk0SqtVw6+Rl8q9lifgvLtsUyn/AB9d\n\tw1kA==","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=pxP2b0XD07Tj3WpAeP5yFEzrhBuSn7tfRkuot6OAsu8=;\n\tb=TgzKXTq41Sz3cK8OGYamPr/GeZeWv29N+KbyyZaCCV+7eNl24MBNNJJamaoYN87So2\n\t9Slgs+bZuKQMEfIal935AopbzyrUSPxdPl5VcLVn+X4YBoschJDJsNqi86Xp4QNYD/0j\n\tRI+0r9ue1gjh0dOXbl24BP1GZNT1eI1AT/MsaQMcQSzmRY8gwLcbXExqo4s7PzXmVtlh\n\tBxum4iRCGAug46+XxClUxG7Gpeew3IbeYm8Jgkfi0kZEQj0d2mV8/yI4PkxlTR1C9j4A\n\tCdtNmcXuV1/BK33TSIwupa1BlT+SELojQbRqbUMFh/8OrzmB4Hqg4LPlKJfEYlB3q4FB\n\tbbZQ==","X-Gm-Message-State":"AOAM530uPiW3MyLTtFdxaD+LGYBvPRKM0VHvQ39PqtOJz7sLJjtf5jXK\n\tNVdFyF1dCE4srIctmC8fPdkudhfT4NH+xVGyvVooPw==","X-Google-Smtp-Source":"ABdhPJwKfadNEOPnUstCr8NF8Avg2fhCYghwMBHS992kwCax95RG7RWSrc5elqBml7Kt9Y5jNGpt+HK+eTCKDFgHttY=","X-Received":"by 2002:ac2:5193:: with SMTP id\n\tu19mr15524434lfi.210.1623764788331; \n\tTue, 15 Jun 2021 06:46:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20210614100040.3054433-1-naush@raspberrypi.com>\n\t<20210614100040.3054433-2-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 15 Jun 2021 14:46:11 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"000000000000685bb305c4ce3332\"","Subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Allow long\n\texposure modes for imx477.","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17942,"web_url":"https://patchwork.libcamera.org/comment/17942/","msgid":"","date":"2021-07-01T23:55:31","subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Allow long\n\texposure modes for imx477.","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Jun 15, 2021 at 01:55:55PM +0100, Naushir Patuck wrote:\n> On Tue, 15 Jun 2021 at 04:19, Laurent Pinchart wrote:\n> > On Mon, Jun 14, 2021 at 11:00:37AM +0100, Naushir Patuck wrote:\n> > > Update the imx477 CamHelper to use long exposure modes if needed.\n> > > This is done by overloading the CamHelper::GetVBlanking function to return a\n> > > frame length (and vblank value) computed using a scaling factor when the value\n> > > would be larger than what the sensor register could otherwise hold.\n> > >\n> > > CamHelperImx477::Prepare is also overloaded to ensure that the \"device.status\"\n> > > metadata returns the right value if the long exposure scaling factor is use.\n> >\n> > s/use/used/\n> >\n> > > The scaling factor is unfortunately not returned back in metadata.\n> > >\n> > > With the current imx477 driver, we can achieve a maximum exposure time of approx\n> > > 127 seconds since the HBLANK control is read-only.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 67 +++++++++++++++++++++++\n> > > 1 file changed, 67 insertions(+)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > index 7a1100c25afc..549c2620765a 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > @@ -6,6 +6,7 @@\n> > > */\n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -14,6 +15,7 @@\n> > > #include \"md_parser.hpp\"\n> > >\n> > > using namespace RPiController;\n> > > +using libcamera::utils::Duration;\n> > >\n> > > /*\n> > > * We care about two gain registers and a pair of exposure registers. Their\n> > > @@ -30,6 +32,9 @@ public:\n> > > CamHelperImx477();\n> > > uint32_t GainCode(double gain) const override;\n> > > double Gain(uint32_t gain_code) const override;\n> > > + void Prepare(libcamera::Span buffer, Metadata &metadata) override;\n> > > + uint32_t GetVBlanking(Duration &exposure, Duration minFrameDuration,\n> > > + Duration maxFrameDuration) const override;\n> > > void GetDelays(int &exposure_delay, int &gain_delay,\n> > > int &vblank_delay) const override;\n> > > bool SensorEmbeddedDataPresent() const override;\n> > > @@ -40,6 +45,10 @@ private:\n> > > * in units of lines.\n> > > */\n> > > static constexpr int frameIntegrationDiff = 22;\n> > > + /* Maximum frame length allowable for long exposure calculations. */\n> > > + static constexpr int frameLengthMax = 0xffdc;\n> > > + /* Largest long exposure scale factor given as a left shift on the frame length. */\n> > > + static constexpr int longExposureShiftMax = 7;\n> > >\n> > > MdParserSmia imx477_parser;\n> > >\n> > > @@ -72,6 +81,64 @@ double CamHelperImx477::Gain(uint32_t gain_code) const\n> > > return 1024.0 / (1024 - gain_code);\n> > > }\n> > >\n> > > +void CamHelperImx477::Prepare(libcamera::Span buffer, Metadata &metadata)\n> > > +{\n> > > + DeviceStatus deviceStatus, parsedStatus;\n> > > +\n> > > + /* Get the device status provided by DelayedControls */\n> > > + if (metadata.Get(\"device.status\", deviceStatus) != 0)\n> > > + return;\n> > > +\n> > > + /* Get the device status provided by the embedded data buffer. */\n> > > + CamHelper::parseEmbeddedData(buffer, metadata);\n> > > + metadata.Get(\"device.status\", parsedStatus);\n> > > +\n> > > + /*\n> > > + * If the ratio of DelayedControls to embedded data shutter speed is > 1\n> > > + * and is a factor of 2^N, then we can assume this is a long exposure mode\n> > > + * frame. Since embedded data does not provide any hints of long exposure\n> > > + * modes, make sure we use the DelayedControls values in the metadata.\n> > > + * Otherwise, just go with the embedded data values.\n> > > + */\n> > > + unsigned long ratio = std::lround(deviceStatus.shutter_speed / parsedStatus.shutter_speed);\n> > > + bool replace = (ratio > 1) && ((ratio & (~ratio + 1)) == ratio);\n> > > +\n> > > + if (replace)\n> > > + metadata.Set(\"device.status\", deviceStatus);\n> > > +}\n> > > +\n> > > +uint32_t CamHelperImx477::GetVBlanking(Duration &exposure,\n> > > + Duration minFrameDuration,\n> > > + Duration maxFrameDuration) const\n> > > +{\n> > > + uint32_t frameLength, exposureLines;\n> > > + unsigned int shift = 0;\n> > > +\n> > > + frameLength = mode_.height + CamHelper::GetVBlanking(exposure, minFrameDuration,\n> > > + maxFrameDuration);\n> > > + /*\n> > > + * Check if the frame length calculated needs to be setup for long\n> > > + * exposure mode. This will require us to use a long exposure scale\n> > > + * factor provided by a shift operation in the sensor.\n> > > + */\n> > > + while (frameLength > frameLengthMax) {\n> > > + if (++shift > longExposureShiftMax) {\n> > > + shift = longExposureShiftMax;\n> > > + frameLength = frameLengthMax;\n> > > + break;\n> > > + }\n> > > + frameLength >>= 1;\n> > > + }\n> > > +\n> > > + /* Account for any rounding in the scaled frame length value. */\n> > > + frameLength <<= shift;\n> > > + exposureLines = CamHelper::ExposureLines(exposure);\n> > > + exposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);\n> > > + exposure = CamHelper::Exposure(exposureLines);\n> >\n> > I'll try to review this series in more details shortly, but I'd like to\n> > already take this as an opportunity to discuss an idea related to the\n> > cam helpers.\n> >\n> > As they stand today, the cam helpers translate between natural units\n> > (time for exposure, and a linear multipler for gain) and V4L2 control\n> > values. This pushes knowledge of V4L2 to the IPA, which isn't great.\n> > I've been toying with the idea of gathering all the V4L2 knowledge in\n> > the libcamera core, which would move these conversions to the pipeline\n> > handler (with the helper of camera helpers on the libcamera side of\n> > course).\n> >\n> > One issue that bothered me is that the IPA wouldn't know about rounding\n> > (or at least not right away, it could get information back from the\n> > pipeline handler, but with a delay, and in any case that wouldn't be\n> > very clean). The code above takes rounding into account. I'd thus like\n> > your feedback on how important this is, and, in case (as I suspect) the\n> > IPA needs to know about rounding, whether you think there would still be\n> > a way to not add V4L2 knowledge to the IPA.\n> \n> This is purely from the perspective of the Raspberry Pi IPA, so other vendors\n> may have different opinions or requirements.\n> \n> For the RPi IPA, the key is that we get given the *actual* shutter speed and gain\n> values used by the sensor for that frame, and these may or may not be rounded\n> from what the AGC originally requested. When fetching shutter speed and gain\n> values from embedded data, this is implicit. When no embedded data is available,\n> the CamHelper does the rounding before passing the values into DelayedControls,\n> and the value given back to the IPA when the frame comes through (with appropriate\n> frame delay) will also be rounded. So, if this CamHelper functionality were to be\n> moved to the pipeline handler domain mostly as-is, we would see no ill effect. If the\n> IPA were to somehow get shutter speed and gain values that were not rounded\n> appropriately, you would likely see some tiny oscillations in the AGC, and David\n> would not be happy with that :-)\n> \n> So in summary, I don't think the rounding will give us any problems, other IPAs\n> might have to take a similar approach to ours to claim the same. I do think removing\n> these sorts of conversions from the IPA domain is the right thing to do. Having the\n> IPA deal with only natural units will be a welcome change!\n\nThanks for the information. I would also like to see moving out of the\nIPA, as it will simplify the code on the IPA side.\n\nI do however fear that some pipeline handlers will want to know how\nvalues are rounded before they get the data back along with the captured\nframe. Maybe I'm too paranoid, and this will never be an issue. I think\nit's worth exploring moving this to the pipeline handler side in any\ncase, one more item for my long todo list :-)\n\n> > > +\n> > > + return frameLength - mode_.height;\n> > > +}\n> > > +\n> > > void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,\n> > > int &vblank_delay) const\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 47471C3220\n\tfor ;\n\tThu, 1 Jul 2021 23:56:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7FBF9684E3;\n\tFri, 2 Jul 2021 01:56:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9CE6360288\n\tfor ;\n\tFri, 2 Jul 2021 01:56:11 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 023854AB;\n\tFri, 2 Jul 2021 01:56:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"g0pp8NZ9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625183771;\n\tbh=fSOxQod+EvcKDVZu3KDWRhSd5eCozVIqa1hRQl4pcuE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=g0pp8NZ95mV+DaVdakfsrZcyS/SbZrseDx6auSc182LNyjz/hX5jkEe12s5D1CvMP\n\tm4PWkjomFZX/CTnDFwwi+fgLdEIEtMAX2gV664W2s9CHl2Ps4m2PeoycB9Tf9ZMF8x\n\t502o+7Wgy4vtAP89VjpfawULemUJ4sg2sHxXuABM=","Date":"Fri, 2 Jul 2021 02:55:31 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210614100040.3054433-1-naush@raspberrypi.com>\n\t<20210614100040.3054433-2-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Allow long\n\texposure modes for imx477.","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]