[{"id":17978,"web_url":"https://patchwork.libcamera.org/comment/17978/","msgid":"<CAHW6GYKJ7XY388pma+bSjK5UcTv2QJw4u=cSEBd3E_Gwx+GSYQ@mail.gmail.com>","date":"2021-07-05T11:47:20","subject":"Re: [libcamera-devel] [PATCH v3 5/8] 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 patch!\n\nOn Fri, 2 Jul 2021 at 16:09, Naushir Patuck <naush@raspberrypi.com> 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 used.\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 <naush@raspberrypi.com>\n> ---\n>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 95 +++++++++++++++++++++++\n>  1 file changed, 95 insertions(+)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 91d05d9226ff..ddf0863c11b4 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -6,14 +6,23 @@\n>   */\n>\n>  #include <assert.h>\n> +#include <cmath>\n>  #include <stddef.h>\n>  #include <stdio.h>\n>  #include <stdlib.h>\n>\n> +#include <libcamera/base/log.h>\n> +\n>  #include \"cam_helper.hpp\"\n>  #include \"md_parser.hpp\"\n>\n>  using namespace RPiController;\n> +using namespace libcamera;\n> +using libcamera::utils::Duration;\n> +\n> +namespace libcamera {\n> +LOG_DECLARE_CATEGORY(IPARPI)\n> +}\n>\n>  /*\n>   * We care about two gain registers and a pair of exposure registers. Their\n> @@ -34,6 +43,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<const uint8_t> 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> @@ -44,6 +56,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>         void PopulateMetadata(const MdParser::RegisterMap &registers,\n>                               Metadata &metadata) const override;\n> @@ -64,6 +80,85 @@ double CamHelperImx477::Gain(uint32_t gain_code) const\n>         return 1024.0 / (1024 - gain_code);\n>  }\n>\n> +void CamHelperImx477::Prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata)\n> +{\n> +       MdParser::RegisterMap registers;\n> +       Metadata parsedMetadata;\n> +\n> +       if (buffer.empty())\n> +               return;\n> +\n> +       if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {\n> +               LOG(IPARPI, Error) << \"Embedded data buffer parsing failed\";\n> +               return;\n> +       }\n> +\n> +       PopulateMetadata(registers, parsedMetadata);\n> +       metadata.Merge(parsedMetadata);\n> +\n> +       DeviceStatus deviceStatus, parsedDeviceStatus;\n> +       if (metadata.Get(\"device.status\", deviceStatus) ||\n> +           parsedMetadata.Get(\"device.status\", parsedDeviceStatus)) {\n> +               LOG(IPARPI, Error) << \"DeviceStatus not found\";\n> +               return;\n> +       }\n> +\n> +       /*\n> +        * The DeviceStatus struct is first populated with values obtained from\n> +        * DelayedControls. If this reports frame length is > frameLengthMax,\n> +        * it means we are using a long exposure mode. Since the long exposure\n> +        * scale factor is not returned back through embedded data, we must rely\n> +        * on the existing exposure lines and frame length values returned by\n> +        * DelayedControls.\n> +        *\n> +        * Otherwise, all values are updated with what is reported in the\n> +        * embedded data.\n> +        */\n> +       if (deviceStatus.frame_length <= frameLengthMax) {\n> +               deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> +               deviceStatus.frame_length = parsedDeviceStatus.frame_length;\n> +       }\n> +       deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> +\n> +       LOG(IPARPI, Debug) << \"Metadata updated - \" << deviceStatus;\n> +\n> +       metadata.Set(\"device.status\", deviceStatus);\n> +}\n\nI guess my only feeling about this is how much it copies from the base\nclass version (well, parseEmbeddedData, if that still exists?). I\nexpect you've stared at this yourself for far too long and are\nprobably quite sick of it by now!!\n\nHere's one last thought, but please feel free to ignore it because\nI'll be fine with what you have above!\n\nThe thing is, our default parseEmbeddedData method gives embedded data\na chance to overwrite what came from the delayed controls. What if we\nderived PopulateMetadata instead. I think we'd have to pass it the\nregular metadata too, but then it would get a chance to populate the\nparsedMetadata from whichever source it liked better. And everything\nelse would be untouched. Possibly.\n\nBut please don't feel obliged. Maybe that's just more devious than we\nwant - I'll leave it up to you!\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> +       if (shift) {\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\nI think I asked once before about qualifying these calls with the base\nclass name when it isn't necessary. Do we do it elsewhere? Does it\nmake you worry that these methods might be virtual when they aren't?\nBut I really don't mind, and am happy either way.\n\nAs I said, I'm fine with this as it stands, so:\n\nReviewed-by: David Plowman <david.plowman@raspberrypi.com>\n\nThanks\nDavid\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> 2.25.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 7BA73BD794\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  5 Jul 2021 11:47:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0166F684F9;\n\tMon,  5 Jul 2021 13:47:33 +0200 (CEST)","from mail-wm1-x32c.google.com (mail-wm1-x32c.google.com\n\t[IPv6:2a00:1450:4864:20::32c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D9796050A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  5 Jul 2021 13:47:32 +0200 (CEST)","by mail-wm1-x32c.google.com with SMTP id\n\tt14-20020a05600c198eb029020c8aac53d4so2887510wmq.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 05 Jul 2021 04:47:32 -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=\"Vg0hFGhl\"; 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=0/A+dhaSlruwexdpfLkLT+T2pLA/575/nV+nQwHJ92A=;\n\tb=Vg0hFGhlrruGMIWmQre6QvrVSQ0P8eoxF8JG+4aGKNxeV7Oroe3ExsDHE9g8/nR1k2\n\tRpjvkh3V/rgF8kTaJtCB5iAxHLeJok3yTSZCyI4235ufoFFDz5WM4sVQO7pmdluGth0A\n\tZSpQ3Nvn2OnleVrbBZK/7fgV0oQjG3x9hdgekAaJpdySW4Jj6QZHSEa4+f1rvcnTUxo7\n\tmjWBfKfmaT9EN7Jr8hWeF0T7oeFb8zlCxIfMwEgYTvfccFk2fVwc0WA3y8EvmP/G/o2C\n\tNrjyq5Q/TuYyUwFNHMWxtHRrLio+Ljx4s2VCrAogvzYVyf46hKJKnxSwbffqnBYCsRNh\n\txQEg==","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=0/A+dhaSlruwexdpfLkLT+T2pLA/575/nV+nQwHJ92A=;\n\tb=nICFkL1aHMxO+Au+rFFIEoMSH8ZJRMmmrzp2mks5mFBY6PXYTAjVwzNpC1Xbcpj91l\n\t5HV1YvhYxBNlV5lb6jwrraFoFJiP46aPrNGEv1JPuK44h7plrGNyOlE9F4MqYtM8iDqP\n\t4fdoKk/uNyr3Lr0i/51O8O24qZGOgEysmgOK0FHepuSz4MvibLLLFlYAR8ijG2BOcWe3\n\tkt7XAv5pghMQDsDx/r9G5xpabSym5MzYQdHM27nZ5A0Y+vTQLCTEWklJc5TgJQ6hKQWi\n\t4bgBVeiSbVxKQOJPw3kAfFO/jx7wuygpjLsq/f5LB53MJs00JRMnrG0QDxH8dUfLRmGi\n\tyfYQ==","X-Gm-Message-State":"AOAM532ixgyuXgsey2T/1OiUr131igEW3YmdPLnKb1P/blZ3LIWDv+lS\n\tFJv1xEVmQSXD+CyKYMPMrVY7AxMUMvPp5qF0VlIUSA==","X-Google-Smtp-Source":"ABdhPJw4g0Is4pXlow+ypMuEm0XdBedv8c1v5EC0xJBvRjlo9If4boY9N56xPSNNqioCqup5MkQJM49NjSADu6VoL5M=","X-Received":"by 2002:a05:600c:1d28:: with SMTP id\n\tl40mr14795607wms.130.1625485652134; \n\tMon, 05 Jul 2021 04:47:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20210702150940.226941-1-naush@raspberrypi.com>\n\t<20210702150940.226941-6-naush@raspberrypi.com>","In-Reply-To":"<20210702150940.226941-6-naush@raspberrypi.com>","From":"David Plowman <david.plowman@raspberrypi.com>","Date":"Mon, 5 Jul 2021 12:47:20 +0100","Message-ID":"<CAHW6GYKJ7XY388pma+bSjK5UcTv2QJw4u=cSEBd3E_Gwx+GSYQ@mail.gmail.com>","To":"Naushir Patuck <naush@raspberrypi.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 5/8] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":17984,"web_url":"https://patchwork.libcamera.org/comment/17984/","msgid":"<CAEmqJPpQ9FFTG+Wrh_dCzOnFG46Rdz+i0Zxr9H6dDZ1jBf1uoQ@mail.gmail.com>","date":"2021-07-05T12:43:37","subject":"Re: [libcamera-devel] [PATCH v3 5/8] 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 review feedback.\n\nOn Mon, 5 Jul 2021 at 12:47, David Plowman <david.plowman@raspberrypi.com>\nwrote:\n\n> Hi Naush\n>\n> Thanks for this patch!\n>\n> On Fri, 2 Jul 2021 at 16:09, Naushir Patuck <naush@raspberrypi.com> 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> used.\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 <naush@raspberrypi.com>\n> > ---\n> >  src/ipa/raspberrypi/cam_helper_imx477.cpp | 95 +++++++++++++++++++++++\n> >  1 file changed, 95 insertions(+)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index 91d05d9226ff..ddf0863c11b4 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -6,14 +6,23 @@\n> >   */\n> >\n> >  #include <assert.h>\n> > +#include <cmath>\n> >  #include <stddef.h>\n> >  #include <stdio.h>\n> >  #include <stdlib.h>\n> >\n> > +#include <libcamera/base/log.h>\n> > +\n> >  #include \"cam_helper.hpp\"\n> >  #include \"md_parser.hpp\"\n> >\n> >  using namespace RPiController;\n> > +using namespace libcamera;\n> > +using libcamera::utils::Duration;\n> > +\n> > +namespace libcamera {\n> > +LOG_DECLARE_CATEGORY(IPARPI)\n> > +}\n> >\n> >  /*\n> >   * We care about two gain registers and a pair of exposure registers.\n> Their\n> > @@ -34,6 +43,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<const uint8_t> 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> > @@ -44,6 +56,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> >         void PopulateMetadata(const MdParser::RegisterMap &registers,\n> >                               Metadata &metadata) const override;\n> > @@ -64,6 +80,85 @@ double CamHelperImx477::Gain(uint32_t gain_code) const\n> >         return 1024.0 / (1024 - gain_code);\n> >  }\n> >\n> > +void CamHelperImx477::Prepare(libcamera::Span<const uint8_t> buffer,\n> Metadata &metadata)\n> > +{\n> > +       MdParser::RegisterMap registers;\n> > +       Metadata parsedMetadata;\n> > +\n> > +       if (buffer.empty())\n> > +               return;\n> > +\n> > +       if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {\n> > +               LOG(IPARPI, Error) << \"Embedded data buffer parsing\n> failed\";\n> > +               return;\n> > +       }\n> > +\n> > +       PopulateMetadata(registers, parsedMetadata);\n> > +       metadata.Merge(parsedMetadata);\n> > +\n> > +       DeviceStatus deviceStatus, parsedDeviceStatus;\n> > +       if (metadata.Get(\"device.status\", deviceStatus) ||\n> > +           parsedMetadata.Get(\"device.status\", parsedDeviceStatus)) {\n> > +               LOG(IPARPI, Error) << \"DeviceStatus not found\";\n> > +               return;\n> > +       }\n> > +\n> > +       /*\n> > +        * The DeviceStatus struct is first populated with values\n> obtained from\n> > +        * DelayedControls. If this reports frame length is >\n> frameLengthMax,\n> > +        * it means we are using a long exposure mode. Since the long\n> exposure\n> > +        * scale factor is not returned back through embedded data, we\n> must rely\n> > +        * on the existing exposure lines and frame length values\n> returned by\n> > +        * DelayedControls.\n> > +        *\n> > +        * Otherwise, all values are updated with what is reported in the\n> > +        * embedded data.\n> > +        */\n> > +       if (deviceStatus.frame_length <= frameLengthMax) {\n> > +               deviceStatus.shutter_speed =\n> parsedDeviceStatus.shutter_speed;\n> > +               deviceStatus.frame_length =\n> parsedDeviceStatus.frame_length;\n> > +       }\n> > +       deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> > +\n> > +       LOG(IPARPI, Debug) << \"Metadata updated - \" << deviceStatus;\n> > +\n> > +       metadata.Set(\"device.status\", deviceStatus);\n> > +}\n>\n> I guess my only feeling about this is how much it copies from the base\n> class version (well, parseEmbeddedData, if that still exists?). I\n> expect you've stared at this yourself for far too long and are\n> probably quite sick of it by now!!\n>\n\n> Here's one last thought, but please feel free to ignore it because\n> I'll be fine with what you have above!\n>\n> The thing is, our default parseEmbeddedData method gives embedded data\n> a chance to overwrite what came from the delayed controls.\n\n\nI do agree that CamHelperImx477::Prepare() does look slightly similar to\nthe default CamHelper::parseEmbeddedData().  In fact, in the last revision,\nthe former would call the latter!  I decided to do it the above way to be\nslightly more efficient in how many times we touch the various metadata\nbuffers flying around within the general Prepare() operation.  This way does\nsave 2 accesses, but that does not seem like much to be honest.\n\n\nWhat if we\n> derived PopulateMetadata instead. I think we'd have to pass it the\n> regular metadata too, but then it would get a chance to populate the\n> parsedMetadata from whichever source it liked better. And everything\n> else would be untouched. Possibly.\n>\n\nI wouldn't mind doing this, except I don't really like passing in two\nmetadata\nbuffers into PopulateMetadata(), it just feels wrong to me.\n\nPerhaps I should put back the original logic where\nCamHelperImx477::Prepare()\nwould call CamHelper::parseEmbeddedData() but at the expense of a\nfew more accesses to the metadata buffers?\n\n\n\n> But please don't feel obliged. Maybe that's just more devious than we\n> want - I'll leave it up to you!\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> > +       if (shift) {\n> > +               /* Account for any rounding in the scaled frame length\n> 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 think I asked once before about qualifying these calls with the base\n> class name when it isn't necessary. Do we do it elsewhere? Does it\n> make you worry that these methods might be virtual when they aren't?\n> But I really don't mind, and am happy either way.\n>\n\nYou are right, it should be fine to remove the base class name from this\ncall.  I will amend that in the next revision.\n\nRegards,\nNaush\n\n\n>\n> As I said, I'm fine with this as it stands, so:\n>\n> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n>\n> Thanks\n> David\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> > 2.25.1\n> >\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 C2FF3C0100\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  5 Jul 2021 12:43:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40DC068503;\n\tMon,  5 Jul 2021 14:43:56 +0200 (CEST)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85A986050A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  5 Jul 2021 14:43:54 +0200 (CEST)","by mail-lj1-x22c.google.com with SMTP id u25so24469908ljj.11\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 05 Jul 2021 05:43:54 -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=\"GaWS6HXz\"; 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=X6QZqwniOuExJVv5/VG6HuLDN2XAQpRRbjWojogOJug=;\n\tb=GaWS6HXzoApqjlEI9i2aG4vJoiGyWioHN/Np9iHgmUXjLIRD7yblOk+c9WyWfOp7tT\n\tK7rc8WP+8tYmytRovCHM8lbGayJv252nzCHnWCv9uT0YTQmkLbP5tXem9DSk+AmnFACZ\n\tXPIPLi4wqS5HkzjlqOY3NGus8G/tR/i0KN2uSv298ZFZH6XcAdm3g9qCHcLiiQsDlj7R\n\theUiMMpIC6NOFim25x9EzZy8xzx8h5dUe95luaNDcE7lKlsjUo9kuApNPtzcqT44SQ5G\n\tAdUlB1z8CnzKtv9ZczvkHyigO7CT8ARjd9GoMgTyLwkwzgioIzFysscrYE3LBgp9zgCr\n\tr0vg==","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=X6QZqwniOuExJVv5/VG6HuLDN2XAQpRRbjWojogOJug=;\n\tb=SmUXY9hTrHcxP6MITEV6+5+J1dj9MTsBm+khcgnW+LO5HOihUVr7jJmcj9aru6ucb3\n\tvKB9vqt3CV83eXpEyjELLH172Mf11EPAURnXNr/HdJP1guu3UXwghkgBh4guhyCJf8Sl\n\tH9N6MBlmppQecACR26rnC+V2oETazp0JVA34co4CAAPHAGeEfkJbOHYBcDCC0vAGHXx1\n\to0pNMiqyemszVWkdVmYUs5HT43C2TCK/dEXaTH/y01WlsUFmH3r8iIdvb+MD7GNNCFRR\n\tzXl4uiZzPPaG9GBPEMgCvRpbWTC77Oz+ryl2ghm0ewr9F6RUHFPOA+KtneLI2gA3mrxf\n\ti1rA==","X-Gm-Message-State":"AOAM532IFXoRLEdXkB7obGNS0Jc+yH+iOigpkmsNt9J75q/qWE08/KAd\n\t5IiKYPxuZaFB0khA/v79Pjgy5Q2j5e1okt4raBIjmA==","X-Google-Smtp-Source":"ABdhPJxKtbOvOt7Z31sIGAG46e+YW19osz3LKcC5s2EmZlK6KIu83NBrZOeXcnzRkapdKlJwHuJlMVnHvjiK1rRzIU4=","X-Received":"by 2002:a2e:a706:: with SMTP id\n\ts6mr11019517lje.169.1625489033730; \n\tMon, 05 Jul 2021 05:43:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20210702150940.226941-1-naush@raspberrypi.com>\n\t<20210702150940.226941-6-naush@raspberrypi.com>\n\t<CAHW6GYKJ7XY388pma+bSjK5UcTv2QJw4u=cSEBd3E_Gwx+GSYQ@mail.gmail.com>","In-Reply-To":"<CAHW6GYKJ7XY388pma+bSjK5UcTv2QJw4u=cSEBd3E_Gwx+GSYQ@mail.gmail.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Mon, 5 Jul 2021 13:43:37 +0100","Message-ID":"<CAEmqJPpQ9FFTG+Wrh_dCzOnFG46Rdz+i0Zxr9H6dDZ1jBf1uoQ@mail.gmail.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Content-Type":"multipart/alternative; boundary=\"000000000000712c1805c65fa8a2\"","Subject":"Re: [libcamera-devel] [PATCH v3 5/8] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]