[libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Add CamHelper::ColourGainCode method

[Kieran Bingham]

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 at 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 at 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;
> 

-- 
Regards
--
Kieran