[libcamera-devel,1/2] libcamera: raspberrypi: Add control of sensor vblanking
diff mbox series

Message ID 20200511100150.5205-2-naush@raspberrypi.com
State Superseded
Headers show
Series
  • raspberrypi: FPS control
Related show

Commit Message

Naushir Patuck May 11, 2020, 10:01 a.m. UTC 
Add support for setting V4L2_CID_VBLANK appropriately when setting
V4L2_CID_EXPOSURE. This will allow adaptive framerates during
viewfinder use cases (e.g. when the exposure time goes above 33ms, we
can reduce the framerate to lower than 30fps).

Currently, the minimum framerate is dictated by the lowest available
exposure time in the tuning, and the maximum framerate is limited by
the sensor mode.

V4L2_CID_VBLANK is controlled through the staggered writer, just like
the exposure and gain controls.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper.hpp                 |  1 +
 src/ipa/raspberrypi/cam_helper_imx219.cpp          | 13 +++++++++++++
 src/ipa/raspberrypi/cam_helper_imx477.cpp          | 13 +++++++++++++
 src/ipa/raspberrypi/cam_helper_ov5647.cpp          | 13 +++++++++++++
 src/ipa/raspberrypi/raspberrypi.cpp                |  8 +++++++-
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  2 ++
 6 files changed, 49 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart May 12, 2020, 12:35 a.m. UTC | #1 
Hi Naush,

Thank you for the patch.

On Mon, May 11, 2020 at 11:01:49AM +0100, Naushir Patuck wrote:
> Add support for setting V4L2_CID_VBLANK appropriately when setting
> V4L2_CID_EXPOSURE. This will allow adaptive framerates during
> viewfinder use cases (e.g. when the exposure time goes above 33ms, we
> can reduce the framerate to lower than 30fps).
> 
> Currently, the minimum framerate is dictated by the lowest available
> exposure time in the tuning, and the maximum framerate is limited by
> the sensor mode.
> 
> V4L2_CID_VBLANK is controlled through the staggered writer, just like
> the exposure and gain controls.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.hpp                 |  1 +
>  src/ipa/raspberrypi/cam_helper_imx219.cpp          | 13 +++++++++++++
>  src/ipa/raspberrypi/cam_helper_imx477.cpp          | 13 +++++++++++++
>  src/ipa/raspberrypi/cam_helper_ov5647.cpp          | 13 +++++++++++++
>  src/ipa/raspberrypi/raspberrypi.cpp                |  8 +++++++-
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  2 ++
>  6 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> index 0c8aa29a..37281c78 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -74,6 +74,7 @@ public:
>  	MdParser &Parser() const { return *parser_; }
>  	uint32_t ExposureLines(double exposure_us) const;
>  	double Exposure(uint32_t exposure_lines) const; // in us
> +	virtual uint32_t GetVBlanking(double exposure_us) const = 0;
>  	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;
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> index 35c6597c..d34a1f1f 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -45,11 +45,16 @@ class CamHelperImx219 : public CamHelper
>  {
>  public:
>  	CamHelperImx219();
> +	uint32_t GetVBlanking(double exposure) const override;
>  	uint32_t GainCode(double gain) const override;
>  	double Gain(uint32_t gain_code) const override;
>  	unsigned int MistrustFramesModeSwitch() const override;
>  	bool SensorEmbeddedDataPresent() const override;
>  	CamTransform GetOrientation() const override;
> +
> +private:
> +	/* Smallest difference between the frame length and integration time. */

Worth mentioning that the unit is lines ?

> +	static constexpr int frameIntegrationDiff = 4;
>  };
>  
>  CamHelperImx219::CamHelperImx219()
> @@ -61,6 +66,14 @@ CamHelperImx219::CamHelperImx219()
>  {
>  }
>  
> +uint32_t CamHelperImx219::GetVBlanking(double exposure) const
> +{
> +	uint32_t exposureLines = ExposureLines(exposure);
> +
> +	return std::max<uint32_t>(mode_.height, exposureLines) -
> +	       mode_.height + frameIntegrationDiff;
> +}
> +
>  uint32_t CamHelperImx219::GainCode(double gain) const
>  {
>  	return (uint32_t)(256 - 256 / gain);
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index 69544456..242d9689 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -35,10 +35,15 @@ class CamHelperImx477 : public CamHelper
>  {
>  public:
>  	CamHelperImx477();
> +	uint32_t GetVBlanking(double exposure) const override;
>  	uint32_t GainCode(double gain) const override;
>  	double Gain(uint32_t gain_code) const override;
>  	bool SensorEmbeddedDataPresent() const override;
>  	CamTransform GetOrientation() const override;
> +
> +private:
> +	/* Smallest difference between the frame length and integration time. */
> +	static constexpr int frameIntegrationDiff = 22;
>  };
>  
>  CamHelperImx477::CamHelperImx477()
> @@ -46,6 +51,14 @@ CamHelperImx477::CamHelperImx477()
>  {
>  }
>  
> +uint32_t CamHelperImx477::GetVBlanking(double exposure) const
> +{
> +	uint32_t exposureLines = ExposureLines(exposure);
> +
> +	return std::max<uint32_t>(mode_.height, exposureLines) -
> +	       mode_.height + frameIntegrationDiff;
> +}
> +
>  uint32_t CamHelperImx477::GainCode(double gain) const
>  {
>  	return static_cast<uint32_t>(1024 - 1024 / gain);
> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> index 3dbcb164..ff37779c 100644
> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> @@ -16,12 +16,17 @@ class CamHelperOv5647 : public CamHelper
>  {
>  public:
>  	CamHelperOv5647();
> +	uint32_t GetVBlanking(double exposure) const override;
>  	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;
>  	unsigned int HideFramesModeSwitch() const override;
>  	unsigned int MistrustFramesStartup() const override;
>  	unsigned int MistrustFramesModeSwitch() const override;
> +
> +private:
> +	/* Smallest difference between the frame length and integration time. */
> +	static constexpr int frameIntegrationDiff = 4;
>  };
>  
>  /*
> @@ -34,6 +39,14 @@ CamHelperOv5647::CamHelperOv5647()
>  {
>  }
>  
> +uint32_t CamHelperOv5647::GetVBlanking(double exposure) const
> +{
> +	uint32_t exposureLines = ExposureLines(exposure);
> +
> +	return std::max<uint32_t>(mode_.height, exposureLines) -
> +	       mode_.height + frameIntegrationDiff;
> +}
> +

The three implementations are identical, with the only parameters that
could vary being frameIntegrationDiff. Would it make sense to implement
GetVBlanking in the base class, and expose frameIntegrationDiff in
derived classes ? Or do you expect more variation in implementations in
the future ?

>  uint32_t CamHelperOv5647::GainCode(double gain) const
>  {
>  	return static_cast<uint32_t>(gain * 16.0);
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 80dc77a3..104727ea 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -795,6 +795,7 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
>  
>  	int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
>  	int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
> +	int32_t vblanking = helper_->GetVBlanking(agcStatus->shutter_time);

Doesn't this effectively hardcodes a variable frame rate ? Should there
be a maximum frame rate limit ?

>  
>  	if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {
>  		LOG(IPARPI, Error) << "Can't find analogue gain control";
> @@ -812,8 +813,13 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
>  			   << gain_code << ")";
>  
>  	ControlList ctrls(unicam_ctrls_);
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
> +	/*
> +	 * VBLANK must be set before EXPOSURE as the former will adjust the
> +	 * limits of the latter control.
> +	 */
> +	ctrls.set(V4L2_CID_VBLANK, vblanking);
>  	ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> +	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
>  	op.controls.push_back(ctrls);
>  	queueFrameAction.emit(0, op);
>  }
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 16850819..e4c5d8f0 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1159,7 +1159,9 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>  		if (!staggeredCtrl_) {
>  			staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
>  					    { { V4L2_CID_ANALOGUE_GAIN, action.data[0] },
> +					      { V4L2_CID_VBLANK, action.data[1] },
>  					      { V4L2_CID_EXPOSURE, action.data[1] } });
> +
>  			sensorMetadata_ = action.data[2];
>  		}
>

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
index 0c8aa29a..37281c78 100644
--- a/src/ipa/raspberrypi/cam_helper.hpp
+++ b/src/ipa/raspberrypi/cam_helper.hpp
@@ -74,6 +74,7 @@  public:
 	MdParser &Parser() const { return *parser_; }
 	uint32_t ExposureLines(double exposure_us) const;
 	double Exposure(uint32_t exposure_lines) const; // in us
+	virtual uint32_t GetVBlanking(double exposure_us) const = 0;
 	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;
diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
index 35c6597c..d34a1f1f 100644
--- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
+++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
@@ -45,11 +45,16 @@  class CamHelperImx219 : public CamHelper
 {
 public:
 	CamHelperImx219();
+	uint32_t GetVBlanking(double exposure) const override;
 	uint32_t GainCode(double gain) const override;
 	double Gain(uint32_t gain_code) const override;
 	unsigned int MistrustFramesModeSwitch() const override;
 	bool SensorEmbeddedDataPresent() const override;
 	CamTransform GetOrientation() const override;
+
+private:
+	/* Smallest difference between the frame length and integration time. */
+	static constexpr int frameIntegrationDiff = 4;
 };
 
 CamHelperImx219::CamHelperImx219()
@@ -61,6 +66,14 @@  CamHelperImx219::CamHelperImx219()
 {
 }
 
+uint32_t CamHelperImx219::GetVBlanking(double exposure) const
+{
+	uint32_t exposureLines = ExposureLines(exposure);
+
+	return std::max<uint32_t>(mode_.height, exposureLines) -
+	       mode_.height + frameIntegrationDiff;
+}
+
 uint32_t CamHelperImx219::GainCode(double gain) const
 {
 	return (uint32_t)(256 - 256 / gain);
diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
index 69544456..242d9689 100644
--- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
+++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
@@ -35,10 +35,15 @@  class CamHelperImx477 : public CamHelper
 {
 public:
 	CamHelperImx477();
+	uint32_t GetVBlanking(double exposure) const override;
 	uint32_t GainCode(double gain) const override;
 	double Gain(uint32_t gain_code) const override;
 	bool SensorEmbeddedDataPresent() const override;
 	CamTransform GetOrientation() const override;
+
+private:
+	/* Smallest difference between the frame length and integration time. */
+	static constexpr int frameIntegrationDiff = 22;
 };
 
 CamHelperImx477::CamHelperImx477()
@@ -46,6 +51,14 @@  CamHelperImx477::CamHelperImx477()
 {
 }
 
+uint32_t CamHelperImx477::GetVBlanking(double exposure) const
+{
+	uint32_t exposureLines = ExposureLines(exposure);
+
+	return std::max<uint32_t>(mode_.height, exposureLines) -
+	       mode_.height + frameIntegrationDiff;
+}
+
 uint32_t CamHelperImx477::GainCode(double gain) const
 {
 	return static_cast<uint32_t>(1024 - 1024 / gain);
diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
index 3dbcb164..ff37779c 100644
--- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
+++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
@@ -16,12 +16,17 @@  class CamHelperOv5647 : public CamHelper
 {
 public:
 	CamHelperOv5647();
+	uint32_t GetVBlanking(double exposure) const override;
 	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;
 	unsigned int HideFramesModeSwitch() const override;
 	unsigned int MistrustFramesStartup() const override;
 	unsigned int MistrustFramesModeSwitch() const override;
+
+private:
+	/* Smallest difference between the frame length and integration time. */
+	static constexpr int frameIntegrationDiff = 4;
 };
 
 /*
@@ -34,6 +39,14 @@  CamHelperOv5647::CamHelperOv5647()
 {
 }
 
+uint32_t CamHelperOv5647::GetVBlanking(double exposure) const
+{
+	uint32_t exposureLines = ExposureLines(exposure);
+
+	return std::max<uint32_t>(mode_.height, exposureLines) -
+	       mode_.height + frameIntegrationDiff;
+}
+
 uint32_t CamHelperOv5647::GainCode(double gain) const
 {
 	return static_cast<uint32_t>(gain * 16.0);
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 80dc77a3..104727ea 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -795,6 +795,7 @@  void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
 
 	int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
 	int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
+	int32_t vblanking = helper_->GetVBlanking(agcStatus->shutter_time);
 
 	if (unicam_ctrls_.find(V4L2_CID_ANALOGUE_GAIN) == unicam_ctrls_.end()) {
 		LOG(IPARPI, Error) << "Can't find analogue gain control";
@@ -812,8 +813,13 @@  void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
 			   << gain_code << ")";
 
 	ControlList ctrls(unicam_ctrls_);
-	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
+	/*
+	 * VBLANK must be set before EXPOSURE as the former will adjust the
+	 * limits of the latter control.
+	 */
+	ctrls.set(V4L2_CID_VBLANK, vblanking);
 	ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
+	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
 	op.controls.push_back(ctrls);
 	queueFrameAction.emit(0, op);
 }
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 16850819..e4c5d8f0 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1159,7 +1159,9 @@  void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
 		if (!staggeredCtrl_) {
 			staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
 					    { { V4L2_CID_ANALOGUE_GAIN, action.data[0] },
+					      { V4L2_CID_VBLANK, action.data[1] },
 					      { V4L2_CID_EXPOSURE, action.data[1] } });
+
 			sensorMetadata_ = action.data[2];
 		}