[libcamera-devel] [PATCH v5 5/8] ipa: raspberrypi: Allow long exposure modes for imx477.

[Kieran Bingham]

Hi Naush,

On 09/07/2021 10:56, 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.
> 
> 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 | 86 +++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 91d05d9226ff..338fdc0c416a 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -6,14 +6,23 @@
>   */
>  
>  #include <assert.h>
> +#include <cmath>
>  #include <stddef.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  
> +#include <libcamera/base/log.h>
> +
>  #include "cam_helper.hpp"
>  #include "md_parser.hpp"
>  
>  using namespace RPiController;
> +using namespace libcamera;
> +using libcamera::utils::Duration;
> +
> +namespace libcamera {
> +LOG_DECLARE_CATEGORY(IPARPI)
> +}

Seems a bit odd to be wrapping this - but I see how it would be needed
to declare.


Interestingly, I think I see that these cam helpers aren't in any
namespace ?

I don't think that's an issue in particular, and it's not related to
this patch anyway.



>  /*
>   * We care about two gain registers and a pair of exposure registers. Their
> @@ -34,6 +43,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;
> @@ -44,6 +56,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;
> @@ -64,6 +80,76 @@ 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)
> +{
> +	MdParser::RegisterMap registers;
> +	DeviceStatus deviceStatus;
> +
> +	if (metadata.Get("device.status", deviceStatus)) {
> +		LOG(IPARPI, Error) << "DeviceStatus not found from DelayedControls";
> +		return;
> +	}
> +
> +	parseEmbeddedData(buffer, metadata);
> +
> +	/*
> +	 * The DeviceStatus struct is first populated with values obtained from
> +	 * DelayedControls. If this reports frame length is > frameLengthMax,
> +	 * it means we are using a long exposure mode. Since the long exposure
> +	 * scale factor is not returned back through embedded data, we must rely
> +	 * on the existing exposure lines and frame length values returned by
> +	 * DelayedControls.
> +	 *
> +	 * Otherwise, all values are updated with what is reported in the
> +	 * embedded data.
> +	 */
> +	if (deviceStatus.frame_length > frameLengthMax) {
> +		DeviceStatus parsedDeviceStatus;
> +
> +		metadata.Get("device.status", parsedDeviceStatus);
> +		parsedDeviceStatus.shutter_speed = deviceStatus.shutter_speed;
> +		parsedDeviceStatus.frame_length = deviceStatus.frame_length;
> +		metadata.Set("device.status", parsedDeviceStatus);
> +
> +		LOG(IPARPI, Debug) << "Metadata updated for long exposure: "
> +				   << parsedDeviceStatus;
> +	}
> +}
> +
> +uint32_t CamHelperImx477::GetVBlanking(Duration &exposure,
> +				       Duration minFrameDuration,
> +				       Duration maxFrameDuration) const
> +{
> +	uint32_t frameLength, exposureLines;

I think libcamera codestyle usually keeps one variable declaration per
line, but as before we're in src/ipa/raspberrypi here, so I think this
can be relaxed if this is your IPA preference.

Otherwise, the rest seems quite device specific, and I can't see
anything wrong - but I don't know enough about what the IMX477 needs to
comment in particular,

But I'm happy enough to put:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> +	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;
> +	}
> +
> +	if (shift) {
> +		/* Account for any rounding in the scaled frame length value. */
> +		frameLength <<= shift;
> +		exposureLines = ExposureLines(exposure);
> +		exposureLines = std::min(exposureLines, frameLength - frameIntegrationDiff);
> +		exposure = Exposure(exposureLines);
> +	}
> +
> +	return frameLength - mode_.height;
> +}
> +
>  void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,
>  				int &vblank_delay) const
>  {
>