[{"id":17946,"web_url":"https://patchwork.libcamera.org/comment/17946/","msgid":"","date":"2021-07-02T00:13:40","subject":"Re: [libcamera-devel] [PATCH v2 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 Thu, Jul 01, 2021 at 12:34:39PM +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 used.\n> The scaling factor is unfortunately not returned back in metadata.\n\nThat's not nice :-(\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> Reviewed-by: David Plowman \n> ---\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 69 +++++++++++++++++++++++\n> 1 file changed, 69 insertions(+)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 4098fde6f322..139cc7dbd39a 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> @@ -31,6 +33,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> @@ -41,6 +46,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> \tvoid PopulateMetadata(const MdParser::RegisterMap ®isters,\n> \t\t\t Metadata &metadata) const override;\n> @@ -61,6 +70,66 @@ 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\nCouldn't this be simplified by just checking if the shutter speed\nprovided by the delayed controls is larger than the long exposure mode\nthreshold ?\n\n> +\n> +\tif (replace)\n> +\t\tmetadata.Set(\"device.status\", deviceStatus);\n\nIs there any risk in replacing the whole status instead of only the\nshutter speed ?\n\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\nDo I understand correctly that the driver will perform the same\ncomputation ? Is the exposure time handled similarly, or is it only the\nvblank ?\n\n> +\n> +\tif (shift) {\n> +\t\t/* Account for any rounding in the scaled frame length value. */\n> +\t\tframeLength <<= shift;\n> +\t\texposureLines = CamHelper::ExposureLines(exposure);\n> +\t\texposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);\n> +\t\texposure = CamHelper::Exposure(exposureLines);\n> +\t}\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 4A242C3222\n\tfor ;\n\tFri, 2 Jul 2021 00:14:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A1C53684E7;\n\tFri, 2 Jul 2021 02:14:21 +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 3581D60288\n\tfor ;\n\tFri, 2 Jul 2021 02:14:20 +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 A51754AB;\n\tFri, 2 Jul 2021 02:14:19 +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=\"Qr0skk0z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625184859;\n\tbh=0K+sqE4V4/9jLZv1FxWBeNfglG+hNm0nNz5Tbhp0zjY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Qr0skk0zAT0R+9Ut9QCmr8miM8zfhgyPzMslnYFEGKh4hGB3wu1EhvvGvPb/0EA5g\n\tfCD9MRgz/SY2YenmIghpABAVKPhKs04+utAiQAT6Ja9Ocr6DyiHL/oHg7UgkqZGWFw\n\tvUBTI0AOFIvLpeLXRS7GqQ/l6hvclEkmQxvJS4Ho=","Date":"Fri, 2 Jul 2021 03:13:40 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210701113442.111718-1-naush@raspberrypi.com>\n\t<20210701113442.111718-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210701113442.111718-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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":17948,"web_url":"https://patchwork.libcamera.org/comment/17948/","msgid":"","date":"2021-07-02T07:45:21","subject":"Re: [libcamera-devel] [PATCH v2 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\nThank you for your review feedback.\n\nOn Fri, 2 Jul 2021 at 01:14, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Thu, Jul 01, 2021 at 12:34:39PM +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> used.\n> > The scaling factor is unfortunately not returned back in metadata.\n>\n> That's not nice :-(\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> > Reviewed-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 69 +++++++++++++++++++++++\n> > 1 file changed, 69 insertions(+)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index 4098fde6f322..139cc7dbd39a 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> > @@ -31,6 +33,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> > @@ -41,6 +46,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> > void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > Metadata &metadata) const override;\n> > @@ -61,6 +70,66 @@ 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> Couldn't this be simplified by just checking if the shutter speed\n> provided by the delayed controls is larger than the long exposure mode\n> threshold ?\n>\n\nYes, perhaps I was being overly defensive with the 2^N test.\n\n\n> > +\n> > + if (replace)\n> > + metadata.Set(\"device.status\", deviceStatus);\n>\n> Is there any risk in replacing the whole status instead of only the\n> shutter speed ?\n>\n\nStrictly speaking, yes, I should really only touch the shutter speed field.\n\nI will probably refactor this function a bit to reduce the number of\nmetadata\nset/get operations for the next revision.\n\nThanks,\nNaush\n\n\n>\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> Do I understand correctly that the driver will perform the same\n> computation ? Is the exposure time handled similarly, or is it only the\n> vblank ?\n>\n\nYes, the sensor driver has pretty much this exact loop to compute the\nlong exposure factor to program in. The exposure time is handled in\nthe same way as line length.\n\nRegards,\nNaush\n\n\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> > +\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 3B2E9C3222\n\tfor ;\n\tFri, 2 Jul 2021 07:45:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFA4C684E6;\n\tFri, 2 Jul 2021 09:45:40 +0200 (CEST)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DF586050A\n\tfor ;\n\tFri, 2 Jul 2021 09:45:38 +0200 (CEST)","by mail-lj1-x22b.google.com with SMTP id f13so12083349ljp.10\n\tfor ;\n\tFri, 02 Jul 2021 00:45:38 -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=\"lmIR8iz/\"; 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=1H/EUQZVsmeyHra4P9ow5TMfM/++KhQNMRo01epogwQ=;\n\tb=lmIR8iz/JiAx65av3rIRObpiVxIlyOupCaWDoOnKQoDcDNYS6di6HEz4B3zq9SMdS1\n\tFfCyehbfQeL7ttJ4YZt3Vh4NgQSlvbU/IGkgub7+AlVPaWGbihoDMNu/3hQ/XkuZWbXW\n\tTRgB+O5yjfwBAGK4v+vekK/147rstQSBdmPoVq2EYRE+FM5FW+nvXhVHAsdyFdhvQvmq\n\t4blzCdG6v5bz5pIgyKYG48935mgoY9IvhopMc71bALKmkm4iAT2cT6ILOWFsT6eti9F6\n\t5uxzxbEtw30/g2TJAEeYJQPEAAMKnGILAC2fTHL/euVzxrCeWuJQd75Ybn8q+JpASgt+\n\tTKbg==","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=1H/EUQZVsmeyHra4P9ow5TMfM/++KhQNMRo01epogwQ=;\n\tb=fg9/ImnnhRGyWhzXSAxzI6hQdwKWBcrTYFzNEw4osbEnVef03aaInU/2svX9M/xc3H\n\thMW0t1RJw2D3oUcKceOcyRwgqe1CdzJt2LBZTaHbGUuaRVUje0R4dwNDd/Q4lbdHs9PE\n\t2iuat52dqTXfx1I3NO1T2HLpA3xdNw90xiwz5qt9nh44MYzLtP+yPwT0Y7m2g4abgvQ4\n\tdKxtTym2mulsDsbQ/J0WLRsN3McxJ3+u8x+snwS+np9Qo5a8Rwj+b2JG+Ho5UbHzTlW8\n\tEOeOVdYJTI7aOjQvQir++tkYHEoZ7ERsWXX7axOYU36zyPzM6P1n38dImLgPQc4Qsxcq\n\tWumA==","X-Gm-Message-State":"AOAM530eatgbXs3XBXi5VDdhJvcjoFLl7SxVEXzbtsbmOpQWhGd7hN25\n\tNfKrIH28y5RbdYIBmMQZ/l5BdgmCS4sx8GB5MSwnGleyRr4=","X-Google-Smtp-Source":"ABdhPJw4HCatfBLvk2bEn8KtSC2RF9DME286/ivZ+67I0XCoUOxNO9XJw+ChMuqPppa1w4w8POpqmTVYYDqVdbN2bWI=","X-Received":"by 2002:a2e:5046:: with SMTP id v6mr2771552ljd.253.1625211937887;\n\tFri, 02 Jul 2021 00:45:37 -0700 (PDT)","MIME-Version":"1.0","References":"<20210701113442.111718-1-naush@raspberrypi.com>\n\t<20210701113442.111718-2-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 2 Jul 2021 08:45:21 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000003e27df05c61f2431\"","Subject":"Re: [libcamera-devel] [PATCH v2 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":17950,"web_url":"https://patchwork.libcamera.org/comment/17950/","msgid":"","date":"2021-07-02T12:58:51","subject":"Re: [libcamera-devel] [PATCH v2 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 again,\n\nOn Fri, 2 Jul 2021 at 08:45, Naushir Patuck wrote:\n\n> Hi Laurent,\n>\n> Thank you for your review feedback.\n>\n> On Fri, 2 Jul 2021 at 01:14, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n>> Hi Naush,\n>>\n>> Thank you for the patch.\n>>\n>> On Thu, Jul 01, 2021 at 12:34:39PM +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\n>> 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\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>> That's not nice :-(\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>> > Reviewed-by: David Plowman \n>> > ---\n>> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 69 +++++++++++++++++++++++\n>> > 1 file changed, 69 insertions(+)\n>> >\n>> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n>> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n>> > index 4098fde6f322..139cc7dbd39a 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>> > @@ -31,6 +33,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>> > @@ -41,6 +46,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\n>> the frame length. */\n>> > + static constexpr int longExposureShiftMax = 7;\n>> >\n>> > void PopulateMetadata(const MdParser::RegisterMap ®isters,\n>> > Metadata &metadata) const override;\n>> > @@ -61,6 +70,66 @@ double CamHelperImx477::Gain(uint32_t gain_code)\n>> 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>> Couldn't this be simplified by just checking if the shutter speed\n>> provided by the delayed controls is larger than the long exposure mode\n>> threshold ?\n>>\n>\nThis comment got me thinking that the logic above is not strictly correct.\nThere may\nbe a case when a user asks for a low exposure with a very large frame\nlength, e.g.\n33ms shutter speed with a framerate of 1/10 fps (don't ask me why). In\nthese cases,\nthe condition above would return the shutter speed that has been scaled,\nand no way\nof un-scaling it.\n\nInstead, what I need to do is do the test above on frame length rather than\nexposure,\nas long exposure implies long frame length mode, and not necessarily the\nother way\nround. So I have to add a frame length field to DeviceStatus and replace\nshutter_speed\nwith frame_length in the above calculations.\n\nNaush\n\n\n>\n> Yes, perhaps I was being overly defensive with the 2^N test.\n>\n>\n>> > +\n>> > + if (replace)\n>> > + metadata.Set(\"device.status\", deviceStatus);\n>>\n>> Is there any risk in replacing the whole status instead of only the\n>> shutter speed ?\n>>\n>\n> Strictly speaking, yes, I should really only touch the shutter speed field.\n>\n> I will probably refactor this function a bit to reduce the number of\n> metadata\n> set/get operations for the next revision.\n>\n> Thanks,\n> Naush\n>\n>\n>>\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\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>> Do I understand correctly that the driver will perform the same\n>> computation ? Is the exposure time handled similarly, or is it only the\n>> vblank ?\n>>\n>\n> Yes, the sensor driver has pretty much this exact loop to compute the\n> long exposure factor to program in. The exposure time is handled in\n> the same way as line length.\n>\n> Regards,\n> Naush\n>\n>\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>> > +\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>>\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 0A03AC3220\n\tfor ;\n\tFri, 2 Jul 2021 12:59:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47B31684F6;\n\tFri, 2 Jul 2021 14:59:10 +0200 (CEST)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F20C0684E8\n\tfor ;\n\tFri, 2 Jul 2021 14:59:07 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id k10so17837569lfv.13\n\tfor ;\n\tFri, 02 Jul 2021 05:59:07 -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=\"JI7d+qtX\"; 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=QZetMVG5MzJ76SjgvWy57JSuTqY96l+m/CfUKI0ZNMw=;\n\tb=JI7d+qtX7VKUUw0VoeRnLg5snBtgcWBFf5Nim7ZHSnGgniPEM4lAXVQtMUTaOlpEm4\n\t0dVDsZF9apq4EjpCHR7jWkWvbLMgkTqMOd17DuPVqyFvxSeVIb7rmCLlGepTZRXtSocL\n\trzabulrR+AnSSbNpYb4Sif1ZMJi4toqa75k2Bdsy/saTGTdT04Hyx97obrh36j0p5pdq\n\tcWHxefeqU+IrV8s/kQ8TLkiVIQfBZc6Va6eupJvmBSKR3VWzrTH6K0+RoCawX1scOCgF\n\to/BFzMf/5GZk6p5KfbOo8JzcXp7wGVCm31iSMXSRNw43ajbH9gwZNeJ1xJ8wrWJvwoQp\n\tS2cQ==","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=QZetMVG5MzJ76SjgvWy57JSuTqY96l+m/CfUKI0ZNMw=;\n\tb=I/9Jn9KYkzybYgur37mm3GwJ5AWxOhVOa78m2kzQxZZ3qFmUG6vrj1XYzGTjVR9y1v\n\tq3eE8Vrugo7N2LaYsuHimCnuYoq7D4GZ1T5/V+PsuWWfnms+wC+5uRyVnnVlt7ii2xKD\n\tPo/QIadkaRiU9M+ebdDe7cN8w5w1e+2rCaUXlxF03SsXruCOrtt6BVVRu4jucKNhRQic\n\tOT/UohVAQGrHqRRL3pv0yG66mUX9Ch0bmzy6k9JhHIEIQNNU9lXWVZWCD7rEzFmE/QGU\n\tGBODu+640/o+rUExhNTbLDUUQsgiHro2KoQa2plLq7eiZJhpe9d5J9Zob9Rb8fAOfVLO\n\tl/jg==","X-Gm-Message-State":"AOAM530WqDTeM/FXEJrIFUzelXbAwT5lv2Tc98qd4c393aa1wmtJOm1O\n\twKPYVbAm+7+Y7zJeQB2VLT6L6tdV0NxBMntGvfJ5dA==","X-Google-Smtp-Source":"ABdhPJxebPRbPswWdhFsE8dExhY9nveqjk24zs2gNyIHCXgOu+7myqoSr+Snp7cmUfhSTCYUUI7+85gBP51Ybw0XE3w=","X-Received":"by 2002:a05:6512:1288:: with SMTP id\n\tu8mr2851582lfs.272.1625230747078; \n\tFri, 02 Jul 2021 05:59:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20210701113442.111718-1-naush@raspberrypi.com>\n\t<20210701113442.111718-2-naush@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 2 Jul 2021 13:58:51 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000005bb1b605c623851e\"","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}}]