[libcamera-devel,3/3] ipa: raspberrypi: Update sensor's V4L2_CID_NOTIFY_GAINS control when present
diff mbox series

Message ID 20211223080110.9766-4-david.plowman@raspberrypi.com
State Accepted
Headers show
Series
  • Make use of V4L2_CID_NOTIFY_GAINS
Related show

Commit Message

David Plowman Dec. 23, 2021, 8:01 a.m. UTC 
If the sensor exposes the V4L2_CID_NOTIFY_GAINS 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>
---
 include/libcamera/ipa/raspberrypi.mojom       |  1 +
 src/ipa/raspberrypi/raspberrypi.cpp           | 22 +++++++++++++++++++
 .../pipeline/raspberrypi/raspberrypi.cpp      | 10 +++++++++
 3 files changed, 33 insertions(+)

Comments

Laurent Pinchart Dec. 29, 2021, 11:53 p.m. UTC | #1 
Hi David,

Thank you for the patch.

On Thu, Dec 23, 2021 at 08:01:10AM +0000, David Plowman wrote:
> If the sensor exposes the V4L2_CID_NOTIFY_GAINS 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>
> ---
>  include/libcamera/ipa/raspberrypi.mojom       |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp           | 22 +++++++++++++++++++
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 10 +++++++++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index acd3cafe..e7647724 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -123,6 +123,7 @@ interface IPARPiEventInterface {
>  	statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
>  	runIsp(uint32 bufferId);
>  	embeddedComplete(uint32 bufferId);
> +	setSensorControls(libcamera.ControlList controls);
>  	setIspControls(libcamera.ControlList controls);
>  	setDelayedControls(libcamera.ControlList controls);
>  };
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 0ed41385..8c20c066 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1061,6 +1061,28 @@ void IPARPi::processStats(unsigned int bufferId)
>  
>  		setDelayedControls.emit(ctrls);
>  	}
> +
> +	auto itCtrl = sensorCtrls_.find(V4L2_CID_NOTIFY_GAINS);
> +	if (itCtrl != sensorCtrls_.end()) {
> +		struct AwbStatus awbStatus;
> +		if (rpiMetadata_.Get("awb.status", awbStatus) == 0) {
> +			/*
> +			 * This control is a linear gain value where the default is
> +			 * defined to correspond to a gain of 1. The order of the gains
> +			 * is always B, Gb, Gr, R.
> +			 */
> +			int unity = itCtrl->second.def().get<int32_t>();
> +			ControlList ctrls(sensorCtrls_);
> +			int32_t gains[4] = { static_cast<int32_t>(awbStatus.gain_b * unity),
> +					     unity, unity,
> +					     static_cast<int32_t>(awbStatus.gain_r * unity) };
> +			ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(gains),
> +							    sizeof(gains) });
> +			ctrls.set(V4L2_CID_NOTIFY_GAINS, c);
> +
> +			setSensorControls.emit(ctrls);

Could we avoid adding another operation by setting V4L2_CID_NOTIFY_GAINS
in the pipeline handler, when receiving the request metadata in
RPiCameraData::statsMetadataComplete() ?

> +		}
> +	}
>  }
>  
>  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 b5c687da..59d3bbd8 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -196,6 +196,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);
>  	void setSensorControls(ControlList &controls);
> @@ -1405,6 +1406,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);
>  
> @@ -1524,6 +1526,14 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)
>  	handleState();
>  }
>  
> +void RPiCameraData::setSensorControls(const ControlList &controls)
> +{
> +	ControlList ctrls = controls;
> +
> +	sensor_->setControls(&ctrls);
> +	handleState();

Do we need to call handleState() here ?

> +}
> +
>  void RPiCameraData::setIspControls(const ControlList &controls)
>  {
>  	ControlList ctrls = controls;
David Plowman Dec. 30, 2021, 11:21 a.m. UTC | #2 
Hi Laurent

Thanks for the comments.

On Wed, 29 Dec 2021 at 23:53, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Thu, Dec 23, 2021 at 08:01:10AM +0000, David Plowman wrote:
> > If the sensor exposes the V4L2_CID_NOTIFY_GAINS 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>
> > ---
> >  include/libcamera/ipa/raspberrypi.mojom       |  1 +
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 22 +++++++++++++++++++
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 10 +++++++++
> >  3 files changed, 33 insertions(+)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > index acd3cafe..e7647724 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -123,6 +123,7 @@ interface IPARPiEventInterface {
> >       statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
> >       runIsp(uint32 bufferId);
> >       embeddedComplete(uint32 bufferId);
> > +     setSensorControls(libcamera.ControlList controls);
> >       setIspControls(libcamera.ControlList controls);
> >       setDelayedControls(libcamera.ControlList controls);
> >  };
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 0ed41385..8c20c066 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -1061,6 +1061,28 @@ void IPARPi::processStats(unsigned int bufferId)
> >
> >               setDelayedControls.emit(ctrls);
> >       }
> > +
> > +     auto itCtrl = sensorCtrls_.find(V4L2_CID_NOTIFY_GAINS);
> > +     if (itCtrl != sensorCtrls_.end()) {
> > +             struct AwbStatus awbStatus;
> > +             if (rpiMetadata_.Get("awb.status", awbStatus) == 0) {
> > +                     /*
> > +                      * This control is a linear gain value where the default is
> > +                      * defined to correspond to a gain of 1. The order of the gains
> > +                      * is always B, Gb, Gr, R.
> > +                      */
> > +                     int unity = itCtrl->second.def().get<int32_t>();
> > +                     ControlList ctrls(sensorCtrls_);
> > +                     int32_t gains[4] = { static_cast<int32_t>(awbStatus.gain_b * unity),
> > +                                          unity, unity,
> > +                                          static_cast<int32_t>(awbStatus.gain_r * unity) };
> > +                     ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(gains),
> > +                                                         sizeof(gains) });
> > +                     ctrls.set(V4L2_CID_NOTIFY_GAINS, c);
> > +
> > +                     setSensorControls.emit(ctrls);
>
> Could we avoid adding another operation by setting V4L2_CID_NOTIFY_GAINS
> in the pipeline handler, when receiving the request metadata in
> RPiCameraData::statsMetadataComplete() ?

That's an interesting thought. Let me check with Naush once he's back,
but I guess we could "intercept" the colour gain values there and
update those controls in much the same way.

>
> > +             }
> > +     }
> >  }
> >
> >  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 b5c687da..59d3bbd8 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -196,6 +196,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);
> >       void setSensorControls(ControlList &controls);
> > @@ -1405,6 +1406,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);
> >
> > @@ -1524,6 +1526,14 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)
> >       handleState();
> >  }
> >
> > +void RPiCameraData::setSensorControls(const ControlList &controls)
> > +{
> > +     ControlList ctrls = controls;
> > +
> > +     sensor_->setControls(&ctrls);
> > +     handleState();
>
> Do we need to call handleState() here ?

Probably not! But maybe this disappears altogether...

Thanks!
David

>
> > +}
> > +
> >  void RPiCameraData::setIspControls(const ControlList &controls)
> >  {
> >       ControlList ctrls = controls;
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
index acd3cafe..e7647724 100644
--- a/include/libcamera/ipa/raspberrypi.mojom
+++ b/include/libcamera/ipa/raspberrypi.mojom
@@ -123,6 +123,7 @@  interface IPARPiEventInterface {
 	statsMetadataComplete(uint32 bufferId, libcamera.ControlList controls);
 	runIsp(uint32 bufferId);
 	embeddedComplete(uint32 bufferId);
+	setSensorControls(libcamera.ControlList controls);
 	setIspControls(libcamera.ControlList controls);
 	setDelayedControls(libcamera.ControlList controls);
 };
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 0ed41385..8c20c066 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -1061,6 +1061,28 @@  void IPARPi::processStats(unsigned int bufferId)
 
 		setDelayedControls.emit(ctrls);
 	}
+
+	auto itCtrl = sensorCtrls_.find(V4L2_CID_NOTIFY_GAINS);
+	if (itCtrl != sensorCtrls_.end()) {
+		struct AwbStatus awbStatus;
+		if (rpiMetadata_.Get("awb.status", awbStatus) == 0) {
+			/*
+			 * This control is a linear gain value where the default is
+			 * defined to correspond to a gain of 1. The order of the gains
+			 * is always B, Gb, Gr, R.
+			 */
+			int unity = itCtrl->second.def().get<int32_t>();
+			ControlList ctrls(sensorCtrls_);
+			int32_t gains[4] = { static_cast<int32_t>(awbStatus.gain_b * unity),
+					     unity, unity,
+					     static_cast<int32_t>(awbStatus.gain_r * unity) };
+			ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(gains),
+							    sizeof(gains) });
+			ctrls.set(V4L2_CID_NOTIFY_GAINS, c);
+
+			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 b5c687da..59d3bbd8 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -196,6 +196,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);
 	void setSensorControls(ControlList &controls);
@@ -1405,6 +1406,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);
 
@@ -1524,6 +1526,14 @@  void RPiCameraData::embeddedComplete(uint32_t bufferId)
 	handleState();
 }
 
+void RPiCameraData::setSensorControls(const ControlList &controls)
+{
+	ControlList ctrls = controls;
+
+	sensor_->setControls(&ctrls);
+	handleState();
+}
+
 void RPiCameraData::setIspControls(const ControlList &controls)
 {
 	ControlList ctrls = controls;