[libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Make CamHelpers return the frame delay for vblanking

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Thu, Mar 04, 2021 at 03:31:19PM +0000, David Plowman wrote:
> For some sensors (e.g. imx477) we need to update the vblanking on the
> frame before the exposure.

That is really peculiar, and quite annoying. I'm OK with this change as
a temporary fix, do you think we could get some support from Sony to
figure out what's happening ?

> For this reason the GetDelays method must
> also return the number of frame delays for the vblanking control.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp        | 4 +++-
>  src/ipa/raspberrypi/cam_helper.hpp        | 9 ++++++---
>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 7 +++++--
>  src/ipa/raspberrypi/cam_helper_ov5647.cpp | 7 +++++--
>  src/ipa/raspberrypi/raspberrypi.cpp       | 6 +++---
>  5 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 2837fcce..0ae0baa0 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -95,7 +95,8 @@ void CamHelper::SetCameraMode(const CameraMode &mode)
>  	initialized_ = true;
>  }
>  
> -void CamHelper::GetDelays(int &exposure_delay, int &gain_delay) const
> +void CamHelper::GetDelays(int &exposure_delay, int &gain_delay,
> +			  int &vblank_delay) const
>  {
>  	/*
>  	 * These values are correct for many sensors. Other sensors will
> @@ -103,6 +104,7 @@ void CamHelper::GetDelays(int &exposure_delay, int &gain_delay) const
>  	 */
>  	exposure_delay = 2;
>  	gain_delay = 1;
> +	vblank_delay = 2;
>  }
>  
>  bool CamHelper::SensorEmbeddedDataPresent() const
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> index 14d70112..8c5659ed 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -28,8 +28,10 @@ namespace RPiController {
>  // exposure time, and to convert between the sensor's gain codes and actual
>  // gains.
>  //
> -// A method to return the number of frames of delay between updating exposure
> -// and analogue gain and the changes taking effect. For many sensors these
> +// A method to return the number of frames of delay between updating exposure,
> +// analogue gain and vblanking, and for the changes to take effect. For many
> +// sensors these take the values 2, 1 and 2 respectively, but sensors that are
> +// different will need to over-ride the default method provided.
>  // take the values 2 and 1 respectively, but sensors that are different will
>  // need to over-ride the default method provided.

Have you forgotten to remove the last two lines ?

>  //
> @@ -72,7 +74,8 @@ public:
>  				      double maxFrameDuration) const;
>  	virtual uint32_t GainCode(double gain) const = 0;
>  	virtual double Gain(uint32_t gain_code) const = 0;
> -	virtual void GetDelays(int &exposure_delay, int &gain_delay) const;
> +	virtual void GetDelays(int &exposure_delay, int &gain_delay,
> +			       int &vblank_delay) const;
>  	virtual bool SensorEmbeddedDataPresent() const;
>  	virtual unsigned int HideFramesStartup() const;
>  	virtual unsigned int HideFramesModeSwitch() const;
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 419f8e77..73a5ca7d 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -37,7 +37,8 @@ public:
>  	CamHelperImx477();
>  	uint32_t GainCode(double gain) const override;
>  	double Gain(uint32_t gain_code) const override;
> -	void GetDelays(int &exposure_delay, int &gain_delay) const override;
> +	void GetDelays(int &exposure_delay, int &gain_delay,
> +		       int &vblank_delay) const override;
>  	bool SensorEmbeddedDataPresent() const override;
>  
>  private:
> @@ -63,10 +64,12 @@ double CamHelperImx477::Gain(uint32_t gain_code) const
>  	return 1024.0 / (1024 - gain_code);
>  }
>  
> -void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay) const
> +void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,
> +				int &vblank_delay) const
>  {
>  	exposure_delay = 2;
>  	gain_delay = 2;
> +	vblank_delay = 3;
>  }
>  
>  bool CamHelperImx477::SensorEmbeddedDataPresent() const
> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> index 75486e90..12be6bf9 100644
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> @@ -17,7 +17,8 @@ public:
>  	CamHelperOv5647();
>  	uint32_t GainCode(double gain) const override;
>  	double Gain(uint32_t gain_code) const override;
> -	void GetDelays(int &exposure_delay, int &gain_delay) const override;
> +	void GetDelays(int &exposure_delay, int &gain_delay,
> +		       int &vblank_delay) const override;
>  	unsigned int HideFramesStartup() const override;
>  	unsigned int HideFramesModeSwitch() const override;
>  	unsigned int MistrustFramesStartup() const override;
> @@ -51,7 +52,8 @@ double CamHelperOv5647::Gain(uint32_t gain_code) const
>  	return static_cast<double>(gain_code) / 16.0;
>  }
>  
> -void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const
> +void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay,
> +				int &vblank_delay) const
>  {
>  	/*
>  	 * We run this sensor in a mode where the gain delay is bumped up to
> @@ -59,6 +61,7 @@ void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const
>  	 */
>  	exposure_delay = 2;
>  	gain_delay = 2;
> +	vblank_delay = 2;
>  }
>  
>  unsigned int CamHelperOv5647::HideFramesStartup() const
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 6348d071..741bff4c 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -342,14 +342,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		 * Pass out the sensor config to the pipeline handler in order
>  		 * to setup the staggered writer class.
>  		 */
> -		int gainDelay, exposureDelay, sensorMetadata;
> -		helper_->GetDelays(exposureDelay, gainDelay);
> +		int gainDelay, exposureDelay, vblankDelay, sensorMetadata;
> +		helper_->GetDelays(exposureDelay, gainDelay, vblankDelay);
>  		sensorMetadata = helper_->SensorEmbeddedDataPresent();
>  
>  		result->params |= ipa::RPi::ConfigSensorParams;
>  		result->sensorConfig.gainDelay = gainDelay;
>  		result->sensorConfig.exposureDelay = exposureDelay;
> -		result->sensorConfig.vblank = exposureDelay;
> +		result->sensorConfig.vblank = vblankDelay;

Unrelated to this patch, should the vblank field be renamed to
vblankDelay ?

At some point it could be useful to make this interface a bit more
generic, but that can wait.

With the above issue fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  		result->sensorConfig.sensorMetadata = sensorMetadata;
>  	}
>  

-- 
Regards,

Laurent Pinchart