[libcamera-devel] [PATCH 1/4] ipa: raspberrypi: Allow long exposure modes for imx477.

[David Plowman]

Hi Naush

Thanks for this feature!

On Mon, 14 Jun 2021 at 11:00, Naushir Patuck <naush at raspberrypi.com> wrote:
>
> Update the imx477 CamHelper to use long exposure modes if needed.
> This is done by overloading the CamHelper::GetVBlanking function to return a
> frame length (and vblank value) computed using a scaling factor when the value
> would be larger than what the sensor register could otherwise hold.
>
> CamHelperImx477::Prepare is also overloaded to ensure that the "device.status"
> metadata returns the right value if the long exposure scaling factor is use.
> The scaling factor is unfortunately not returned back in metadata.
>
> With the current imx477 driver, we can achieve a maximum exposure time of approx
> 127 seconds since the HBLANK control is read-only.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 67 +++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 7a1100c25afc..549c2620765a 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -6,6 +6,7 @@
>   */
>
>  #include <assert.h>
> +#include <cmath>
>  #include <stddef.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -14,6 +15,7 @@
>  #include "md_parser.hpp"
>
>  using namespace RPiController;
> +using libcamera::utils::Duration;
>
>  /*
>   * We care about two gain registers and a pair of exposure registers. Their
> @@ -30,6 +32,9 @@ public:
>         CamHelperImx477();
>         uint32_t GainCode(double gain) const override;
>         double Gain(uint32_t gain_code) const override;
> +       void Prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) override;
> +       uint32_t GetVBlanking(Duration &exposure, Duration minFrameDuration,
> +                             Duration maxFrameDuration) const override;
>         void GetDelays(int &exposure_delay, int &gain_delay,
>                        int &vblank_delay) const override;
>         bool SensorEmbeddedDataPresent() const override;
> @@ -40,6 +45,10 @@ private:
>          * in units of lines.
>          */
>         static constexpr int frameIntegrationDiff = 22;
> +       /* Maximum frame length allowable for long exposure calculations. */
> +       static constexpr int frameLengthMax = 0xffdc;
> +       /* Largest long exposure scale factor given as a left shift on the frame length. */
> +       static constexpr int longExposureShiftMax = 7;
>
>         MdParserSmia imx477_parser;
>
> @@ -72,6 +81,64 @@ double CamHelperImx477::Gain(uint32_t gain_code) const
>         return 1024.0 / (1024 - gain_code);
>  }
>
> +void CamHelperImx477::Prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata)
> +{
> +       DeviceStatus deviceStatus, parsedStatus;
> +
> +       /* Get the device status provided by DelayedControls */
> +       if (metadata.Get("device.status", deviceStatus) != 0)
> +               return;
> +
> +       /* Get the device status provided by the embedded data buffer. */
> +       CamHelper::parseEmbeddedData(buffer, metadata);
> +       metadata.Get("device.status", parsedStatus);
> +
> +       /*
> +        * If the ratio of DelayedControls to embedded data shutter speed is > 1
> +        * and is a factor of 2^N, then we can assume this is a long exposure mode
> +        * frame. Since embedded data does not provide any hints of long exposure
> +        * modes, make sure we use the DelayedControls values in the metadata.
> +        * Otherwise, just go with the embedded data values.
> +        */
> +       unsigned long ratio = std::lround(deviceStatus.shutter_speed / parsedStatus.shutter_speed);
> +       bool replace = (ratio > 1) && ((ratio & (~ratio + 1)) == ratio);

Is there a case we need to worry about where "ratio" was only rounded
to a power of 2, but wouldn't otherwise have been one? (e.g.
deviceStatus.shutter_speed = 11, parsedStatus.shutter_speed = 5)

> +
> +       if (replace)
> +               metadata.Set("device.status", deviceStatus);
> +}
> +
> +uint32_t CamHelperImx477::GetVBlanking(Duration &exposure,
> +                                      Duration minFrameDuration,
> +                                      Duration maxFrameDuration) const
> +{
> +       uint32_t frameLength, exposureLines;
> +       unsigned int shift = 0;
> +
> +       frameLength = mode_.height + CamHelper::GetVBlanking(exposure, minFrameDuration,
> +                                                            maxFrameDuration);
> +       /*
> +        * Check if the frame length calculated needs to be setup for long
> +        * exposure mode. This will require us to use a long exposure scale
> +        * factor provided by a shift operation in the sensor.
> +        */
> +       while (frameLength > frameLengthMax) {
> +               if (++shift > longExposureShiftMax) {
> +                       shift = longExposureShiftMax;
> +                       frameLength = frameLengthMax;
> +                       break;
> +               }
> +               frameLength >>= 1;
> +       }
> +
> +       /* Account for any rounding in the scaled frame length value. */
> +       frameLength <<= shift;
> +       exposureLines = CamHelper::ExposureLines(exposure);
> +       exposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);
> +       exposure = CamHelper::Exposure(exposureLines);

Any reason for invoking the base class functions explicitly here? Not
that it bothers me, just curious...

Subject to clarification on my ratio rounding question:

Reviewed-by: David Plowman <david.plowman at raspberrypi.com>

Thanks!
David

> +
> +       return frameLength - mode_.height;
> +}
> +
>  void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,
>                                 int &vblank_delay) const
>  {
> --
> 2.25.1
>