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

[Laurent Pinchart]

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 at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at 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
>  {

-- 
Regards,

Laurent Pinchart