[libcamera-devel,v2,2/3] ipa: raspberrypi: Add CamHelper::ColourGainCode method
diff mbox series

Message ID 20210408133634.16815-3-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: handle sensors more flexibly
Related show

Commit Message

David Plowman April 8, 2021, 1:36 p.m. UTC
Some sensors may require the V4L2_RED/BLUE_BALANCE to be set. These
will require a ColourGainCode method in the CamHelper, analogous to
the GainCode method, but which converts the red and blue gains to the
codes that the driver will want.

Most sensors don't require these values so it's helpful to provide a
default method - noting that it should then never be called.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper.cpp | 11 +++++++++++
 src/ipa/raspberrypi/cam_helper.hpp |  4 +++-
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Kieran Bingham April 14, 2021, 8:34 a.m. UTC | #1
Hi David,

On 08/04/2021 14:36, David Plowman wrote:
> Some sensors may require the V4L2_RED/BLUE_BALANCE to be set. These
> will require a ColourGainCode method in the CamHelper, analogous to
> the GainCode method, but which converts the red and blue gains to the
> codes that the driver will want.
> 
> Most sensors don't require these values so it's helpful to provide a
> default method - noting that it should then never be called.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp | 11 +++++++++++
>  src/ipa/raspberrypi/cam_helper.hpp |  4 +++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 0ae0baa0..556a7c03 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -57,6 +57,17 @@ double CamHelper::Exposure(uint32_t exposure_lines) const
>  	return exposure_lines * mode_.line_length / 1000.0;
>  }
>  
> +uint32_t CamHelper::ColourGainCode([[maybe_unused]] double gain) const
> +{
> +	/*
> +	 * This is only used when the sensor has RED/BLUE_BALANCE controls.
> +	 * Most do not, so supplying a default is helpful - though no one
> +	 * should be calling it.
> +	 */
> +	assert(0);

Should this be ASSERT()? Or a LOG(Fatal) ?

If the sensor does have RED/BLUE_BALANCE, presumably the CamHelper
'must' then implement ColourGainCode for that sensor right?
Otherwise we'd hit this assert.

In which case, perhaps this should be a

   LOG(xxxx, Fatal) << "ColourGainCode not implemented for "
                    << $SensorName;



But either way,

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

> +	return 0;
> +}
> +
>  uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
>  				 double maxFrameDuration) const
>  {
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> index 4053a870..a0b6b987 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -26,7 +26,8 @@ namespace RPiController {
>  //
>  // The ability to convert between number of lines of exposure and actual
>  // exposure time, and to convert between the sensor's gain codes and actual
> -// gains.
> +// gains. Some sensors like to be told the colour (red and blue) gains, so
> +// there's a method for converting these to gain codes too.
>  //
>  // 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
> @@ -72,6 +73,7 @@ public:
>  				      double maxFrameDuration) const;
>  	virtual uint32_t GainCode(double gain) const = 0;
>  	virtual double Gain(uint32_t gain_code) const = 0;
> +	virtual uint32_t ColourGainCode(double gain) const;
>  	virtual void GetDelays(int &exposure_delay, int &gain_delay,
>  			       int &vblank_delay) const;
>  	virtual bool SensorEmbeddedDataPresent() const;
>

Patch
diff mbox series

diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
index 0ae0baa0..556a7c03 100644
--- a/src/ipa/raspberrypi/cam_helper.cpp
+++ b/src/ipa/raspberrypi/cam_helper.cpp
@@ -57,6 +57,17 @@  double CamHelper::Exposure(uint32_t exposure_lines) const
 	return exposure_lines * mode_.line_length / 1000.0;
 }
 
+uint32_t CamHelper::ColourGainCode([[maybe_unused]] double gain) const
+{
+	/*
+	 * This is only used when the sensor has RED/BLUE_BALANCE controls.
+	 * Most do not, so supplying a default is helpful - though no one
+	 * should be calling it.
+	 */
+	assert(0);
+	return 0;
+}
+
 uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,
 				 double maxFrameDuration) const
 {
diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
index 4053a870..a0b6b987 100644
--- a/src/ipa/raspberrypi/cam_helper.hpp
+++ b/src/ipa/raspberrypi/cam_helper.hpp
@@ -26,7 +26,8 @@  namespace RPiController {
 //
 // The ability to convert between number of lines of exposure and actual
 // exposure time, and to convert between the sensor's gain codes and actual
-// gains.
+// gains. Some sensors like to be told the colour (red and blue) gains, so
+// there's a method for converting these to gain codes too.
 //
 // 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
@@ -72,6 +73,7 @@  public:
 				      double maxFrameDuration) const;
 	virtual uint32_t GainCode(double gain) const = 0;
 	virtual double Gain(uint32_t gain_code) const = 0;
+	virtual uint32_t ColourGainCode(double gain) const;
 	virtual void GetDelays(int &exposure_delay, int &gain_delay,
 			       int &vblank_delay) const;
 	virtual bool SensorEmbeddedDataPresent() const;