[libcamera-devel,v3,3/3] ipa: raspberrypi: Update sensor's RED/BLUE balance controls when present

11929 diff mbox series

If the sensor exposes V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE
controls, assume it wants to be told the colour gains.

We want these to be applied as soon as possible so we add a new
setSensorControls signal to the IPA which passes these back to the
pipeline handler without using the DelayedControls mechanism.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/ipa/raspberrypi.mojom            |  1 +
 src/ipa/raspberrypi/raspberrypi.cpp                | 13 +++++++++++++
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++++
 3 files changed, 24 insertions(+)

Hi David,

On Wed, 14 Apr 2021 at 11:30, David Plowman <david.plowman@raspberrypi.com>
wrote:

> If the sensor exposes V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE
> controls, assume it wants to be told the colour gains.
>
> We want these to be applied as soon as possible so we add a new
> setSensorControls signal to the IPA which passes these back to the
> pipeline handler without using the DelayedControls mechanism.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>


>  include/libcamera/ipa/raspberrypi.mojom            |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp                | 13 +++++++++++++
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++++
>  3 files changed, 24 insertions(+)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> index f38c2261..ebaf0a04 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -117,6 +117,7 @@ interface IPARPiEventInterface {
>         statsMetadataComplete(uint32 bufferId, ControlList controls);
>         runIsp(uint32 bufferId);
>         embeddedComplete(uint32 bufferId);
> +       setSensorControls(ControlList controls);
>         setIspControls(ControlList controls);
>         setDelayedControls(ControlList controls);
>  };
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index dad6395f..f95896d2 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1036,6 +1036,19 @@ void IPARPi::processStats(unsigned int bufferId)
>
>                 setDelayedControls.emit(ctrls);
>         }
> +
> +       struct AwbStatus awbStatus;
> +       if (rpiMetadata_.Get("awb.status", awbStatus) == 0 &&
> +           sensorCtrls_.find(V4L2_CID_RED_BALANCE) != sensorCtrls_.end()
> &&
> +           sensorCtrls_.find(V4L2_CID_BLUE_BALANCE) !=
> sensorCtrls_.end()) {
> +               ControlList ctrls(sensorCtrls_);
> +               ctrls.set(V4L2_CID_RED_BALANCE,
> +
>  static_cast<int32_t>(helper_->ColourGainCode(awbStatus.gain_r)));
> +               ctrls.set(V4L2_CID_BLUE_BALANCE,
> +
>  static_cast<int32_t>(helper_->ColourGainCode(awbStatus.gain_b)));
> +
> +               setSensorControls.emit(ctrls);
> +       }
>  }
>
>  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList
> &ctrls)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index f22e286e..8dae4912 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -152,6 +152,7 @@ public:
>         void statsMetadataComplete(uint32_t bufferId, const ControlList
> &controls);
>         void runIsp(uint32_t bufferId);
>         void embeddedComplete(uint32_t bufferId);
> +       void setSensorControls(const ControlList &controls);
>         void setIspControls(const ControlList &controls);
>         void setDelayedControls(const ControlList &controls);
>
> @@ -1219,6 +1220,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig
> *sensorConfig)
>         ipa_->statsMetadataComplete.connect(this,
> &RPiCameraData::statsMetadataComplete);
>         ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
>         ipa_->embeddedComplete.connect(this,
> &RPiCameraData::embeddedComplete);
> +       ipa_->setSensorControls.connect(this,
> &RPiCameraData::setSensorControls);
>         ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>         ipa_->setDelayedControls.connect(this,
> &RPiCameraData::setDelayedControls);
>
> @@ -1361,6 +1363,14 @@ void RPiCameraData::embeddedComplete(uint32_t
> bufferId)
>         handleState();
>  }
>
> +void RPiCameraData::setSensorControls(const ControlList &controls)
> +{
> +       ControlList ctrls = controls;
> +
> +       unicam_[Unicam::Image].dev()->setControls(&ctrls);
> +       handleState();
> +}
> +
>  void RPiCameraData::setIspControls(const ControlList &controls)
>  {
>         ControlList ctrls = controls;
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

Hi David,

Thank you for the patch.

On Wed, Apr 14, 2021 at 11:29:55AM +0100, David Plowman wrote:
> If the sensor exposes V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE
> controls, assume it wants to be told the colour gains.
> 
> We want these to be applied as soon as possible so we add a new
> setSensorControls signal to the IPA which passes these back to the
> pipeline handler without using the DelayedControls mechanism.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.mojom            |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp                | 13 +++++++++++++
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index f38c2261..ebaf0a04 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -117,6 +117,7 @@ interface IPARPiEventInterface {
>  	statsMetadataComplete(uint32 bufferId, ControlList controls);
>  	runIsp(uint32 bufferId);
>  	embeddedComplete(uint32 bufferId);
> +	setSensorControls(ControlList controls);
>  	setIspControls(ControlList controls);
>  	setDelayedControls(ControlList controls);
>  };
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index dad6395f..f95896d2 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1036,6 +1036,19 @@ void IPARPi::processStats(unsigned int bufferId)
>  
>  		setDelayedControls.emit(ctrls);
>  	}
> +
> +	struct AwbStatus awbStatus;
> +	if (rpiMetadata_.Get("awb.status", awbStatus) == 0 &&
> +	    sensorCtrls_.find(V4L2_CID_RED_BALANCE) != sensorCtrls_.end() &&
> +	    sensorCtrls_.find(V4L2_CID_BLUE_BALANCE) != sensorCtrls_.end()) {
> +		ControlList ctrls(sensorCtrls_);
> +		ctrls.set(V4L2_CID_RED_BALANCE,
> +			  static_cast<int32_t>(helper_->ColourGainCode(awbStatus.gain_r)));
> +		ctrls.set(V4L2_CID_BLUE_BALANCE,
> +			  static_cast<int32_t>(helper_->ColourGainCode(awbStatus.gain_b)));
> +
> +		setSensorControls.emit(ctrls);
> +	}
>  }

Given that this series doesn't make use of this feature, it's hard to
review the patch. What bothers me is usage of V4L2_CID_RED_BALANCE and
V4L2_CID_BLUE_BALANCE. Those two controls haven't been designed for
camera sensors, and while a few drivers in the mainline kernel implement
them, it's most likely an abuse. Sensors that provide per-colour gains
typically have one gain for each Bayer component. We need new V4L2
controls for this.

>  
>  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index f22e286e..8dae4912 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -152,6 +152,7 @@ public:
>  	void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
>  	void runIsp(uint32_t bufferId);
>  	void embeddedComplete(uint32_t bufferId);
> +	void setSensorControls(const ControlList &controls);
>  	void setIspControls(const ControlList &controls);
>  	void setDelayedControls(const ControlList &controls);
>  
> @@ -1219,6 +1220,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
>  	ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
>  	ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
>  	ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> +	ipa_->setSensorControls.connect(this, &RPiCameraData::setSensorControls);
>  	ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>  	ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
>  
> @@ -1361,6 +1363,14 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)
>  	handleState();
>  }
>  
> +void RPiCameraData::setSensorControls(const ControlList &controls)
> +{
> +	ControlList ctrls = controls;
> +
> +	unicam_[Unicam::Image].dev()->setControls(&ctrls);
> +	handleState();
> +}
> +
>  void RPiCameraData::setIspControls(const ControlList &controls)
>  {
>  	ControlList ctrls = controls;