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

12756 diff mbox series

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 used.
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@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper_imx477.cpp | 69 +++++++++++++++++++++++
 1 file changed, 69 insertions(+)

Hi Naush,

Thank you for the patch.

On Thu, Jul 01, 2021 at 12:34:39PM +0100, Naushir Patuck 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 used.
> The scaling factor is unfortunately not returned back in metadata.

That's not nice :-(

> 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@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 69 +++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 4098fde6f322..139cc7dbd39a 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
> @@ -31,6 +33,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;
> @@ -41,6 +46,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;
>  
>  	void PopulateMetadata(const MdParser::RegisterMap &registers,
>  			      Metadata &metadata) const override;
> @@ -61,6 +70,66 @@ 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);

Couldn't this be simplified by just checking if the shutter speed
provided by the delayed controls is larger than the long exposure mode
threshold ?

> +
> +	if (replace)
> +		metadata.Set("device.status", deviceStatus);

Is there any risk in replacing the whole status instead of only the
shutter speed ?

> +}
> +
> +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;
> +	}

Do I understand correctly that the driver will perform the same
computation ? Is the exposure time handled similarly, or is it only the
vblank ?

> +
> +	if (shift) {
> +		/* 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);
> +	}
> +
> +	return frameLength - mode_.height;
> +}
> +
>  void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,
>  				int &vblank_delay) const
>  {

Hi Laurent,

Thank you for your review feedback.

On Fri, 2 Jul 2021 at 01:14, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Jul 01, 2021 at 12:34:39PM +0100, Naushir Patuck 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
> used.
> > The scaling factor is unfortunately not returned back in metadata.
>
> That's not nice :-(
>
> > 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@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/cam_helper_imx477.cpp | 69 +++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > index 4098fde6f322..139cc7dbd39a 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
> > @@ -31,6 +33,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;
> > @@ -41,6 +46,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;
> >
> >       void PopulateMetadata(const MdParser::RegisterMap &registers,
> >                             Metadata &metadata) const override;
> > @@ -61,6 +70,66 @@ 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);
>
> Couldn't this be simplified by just checking if the shutter speed
> provided by the delayed controls is larger than the long exposure mode
> threshold ?
>

Yes, perhaps I was being overly defensive with the 2^N test.


> > +
> > +     if (replace)
> > +             metadata.Set("device.status", deviceStatus);
>
> Is there any risk in replacing the whole status instead of only the
> shutter speed ?
>

Strictly speaking, yes, I should really only touch the shutter speed field.

I will probably refactor this function a bit to reduce the number of
metadata
set/get operations for the next revision.

Thanks,
Naush


>
> > +}
> > +
> > +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;
> > +     }
>
> Do I understand correctly that the driver will perform the same
> computation ? Is the exposure time handled similarly, or is it only the
> vblank ?
>

Yes, the sensor driver has pretty much this exact loop to compute the
long exposure factor to program in.  The exposure time is handled in
the same way as line length.

Regards,
Naush


>
> > +
> > +     if (shift) {
> > +             /* 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);
> > +     }
> > +
> > +     return frameLength - mode_.height;
> > +}
> > +
> >  void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,
> >                               int &vblank_delay) const
> >  {
>
> --
> Regards,
>
> Laurent Pinchart
>

Hi again,

On Fri, 2 Jul 2021 at 08:45, Naushir Patuck <naush@raspberrypi.com> wrote:

> Hi Laurent,
>
> Thank you for your review feedback.
>
> On Fri, 2 Jul 2021 at 01:14, Laurent Pinchart <
> laurent.pinchart@ideasonboard.com> wrote:
>
>> Hi Naush,
>>
>> Thank you for the patch.
>>
>> On Thu, Jul 01, 2021 at 12:34:39PM +0100, Naushir Patuck 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
>> used.
>> > The scaling factor is unfortunately not returned back in metadata.
>>
>> That's not nice :-(
>>
>> > 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@raspberrypi.com>
>> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
>> > ---
>> >  src/ipa/raspberrypi/cam_helper_imx477.cpp | 69 +++++++++++++++++++++++
>> >  1 file changed, 69 insertions(+)
>> >
>> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp
>> b/src/ipa/raspberrypi/cam_helper_imx477.cpp
>> > index 4098fde6f322..139cc7dbd39a 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
>> > @@ -31,6 +33,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;
>> > @@ -41,6 +46,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;
>> >
>> >       void PopulateMetadata(const MdParser::RegisterMap &registers,
>> >                             Metadata &metadata) const override;
>> > @@ -61,6 +70,66 @@ 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);
>>
>> Couldn't this be simplified by just checking if the shutter speed
>> provided by the delayed controls is larger than the long exposure mode
>> threshold ?
>>
>
This comment got me thinking that the logic above is not strictly correct.
There may
be a case when a user asks for a low exposure with a very large frame
length, e.g.
33ms shutter speed with a framerate of 1/10 fps (don't ask me why).  In
these cases,
the condition above would return the shutter speed that has been scaled,
and no way
of un-scaling it.

Instead, what I need to do is do the test above on frame length rather than
exposure,
as long exposure implies long frame length mode, and not necessarily the
other way
round.  So I have to add a frame length field to DeviceStatus and replace
shutter_speed
with frame_length in the above calculations.

Naush


>
> Yes, perhaps I was being overly defensive with the 2^N test.
>
>
>> > +
>> > +     if (replace)
>> > +             metadata.Set("device.status", deviceStatus);
>>
>> Is there any risk in replacing the whole status instead of only the
>> shutter speed ?
>>
>
> Strictly speaking, yes, I should really only touch the shutter speed field.
>
> I will probably refactor this function a bit to reduce the number of
> metadata
> set/get operations for the next revision.
>
> Thanks,
> Naush
>
>
>>
>> > +}
>> > +
>> > +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;
>> > +     }
>>
>> Do I understand correctly that the driver will perform the same
>> computation ? Is the exposure time handled similarly, or is it only the
>> vblank ?
>>
>
> Yes, the sensor driver has pretty much this exact loop to compute the
> long exposure factor to program in.  The exposure time is handled in
> the same way as line length.
>
> Regards,
> Naush
>
>
>>
>> > +
>> > +     if (shift) {
>> > +             /* 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);
>> > +     }
>> > +
>> > +     return frameLength - mode_.height;
>> > +}
>> > +
>> >  void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,
>> >                               int &vblank_delay) const
>> >  {
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>