[libcamera-devel,v2,2/2] libcamera: raspberrypi: recalculate camera exposure/gain when camera mode changes
diff mbox series

Message ID 20200618111236.26897-3-david.plowman@raspberrypi.com
State Accepted
Commit 846e7d999a15ce0c3784a790cd5c249ba7aaa5c6
Headers show
Series
  • Raspberry pi IPAs: update sensor exposure/gain when camera mode changes
Related show

Commit Message

David Plowman June 18, 2020, 11:12 a.m. UTC 
This commit causes the AGC to recalculate its camera exposure/gain
values when the camera mode changes. For example it's possible
that the exposure profile could be changed by the application so
the division between exposure time and analogue gain may be
different.

The other underlying reason (and which this commit accomplishes too)
is that the sensor's line timing may change in a new mode, and because
V4L2 drivers store a number of exposure _lines_, the resulting _time_
will "change under our feet". So we have to go through the process of
recalculating the correct number of lines and writing this back to the
sensor with every mode switch, regardless of anything else.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 +++++++++++
 src/ipa/raspberrypi/controller/rpi/agc.hpp |  1 +
 src/ipa/raspberrypi/raspberrypi.cpp        | 25 +++++++++++-----------
 3 files changed, 26 insertions(+), 12 deletions(-)

Comments

Laurent Pinchart June 22, 2020, 4:04 a.m. UTC | #1 
Hi David,

Thank you for the patch.

Nitpicking a little bit, the commit message subject prefix should be
"libcamera: ipa: raspberrypi:". Same for 1/2.

On Thu, Jun 18, 2020 at 12:12:36PM +0100, David Plowman wrote:
> This commit causes the AGC to recalculate its camera exposure/gain
> values when the camera mode changes. For example it's possible
> that the exposure profile could be changed by the application so
> the division between exposure time and analogue gain may be
> different.
> 
> The other underlying reason (and which this commit accomplishes too)
> is that the sensor's line timing may change in a new mode, and because
> V4L2 drivers store a number of exposure _lines_, the resulting _time_
> will "change under our feet". So we have to go through the process of
> recalculating the correct number of lines and writing this back to the
> sensor with every mode switch, regardless of anything else.

I wonder if in the future we shouldn't try to abstract this in the
CameraSensor class, to avoid having to deal with this sensor-specific
behaviour in IPAs. Not something for this patch series.

> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Both patches applied to my tree with the subjects updated. I'll push to
master after running the compilation tests.

> ---
>  src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 +++++++++++
>  src/ipa/raspberrypi/controller/rpi/agc.hpp |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp        | 25 +++++++++++-----------
>  3 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> index a474287..c02b5ec 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)
>  	constraint_mode_name_ = constraint_mode_name;
>  }
>  
> +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> +{
> +	// On a mode switch, it's possible the exposure profile could change,
> +	// so we run through the dividing up of exposure/gain again and
> +	// write the results into the metadata we've been given.
> +	if (status_.total_exposure_value) {
> +		housekeepConfig();
> +		divvyupExposure();
> +		writeAndFinish(metadata, false);
> +	}
> +}
> +
>  void Agc::Prepare(Metadata *image_metadata)
>  {
>  	AgcStatus status;
> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> index dbcefba..9a7e89c 100644
> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> @@ -75,6 +75,7 @@ public:
>  	void SetMeteringMode(std::string const &metering_mode_name) override;
>  	void SetExposureMode(std::string const &exposure_mode_name) override;
>  	void SetConstraintMode(std::string const &contraint_mode_name) override;
> +	void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;
>  	void Prepare(Metadata *image_metadata) override;
>  	void Process(StatisticsPtr &stats, Metadata *image_metadata) override;
>  
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index d6fd3df..42c84b1 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -247,29 +247,30 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		mistrust_count_ = helper_->MistrustFramesStartup();
>  	}
>  
> +	struct AgcStatus agcStatus;
> +	/* These zero values mean not program anything (unless overwritten). */
> +	agcStatus.shutter_time = 0.0;
> +	agcStatus.analogue_gain = 0.0;
> +
>  	if (!controllerInit_) {
>  		/* Load the tuning file for this sensor. */
>  		controller_.Read(tuningFile_.c_str());
>  		controller_.Initialise();
>  		controllerInit_ = true;
>  
> -		/* Calculate initial values for gain and exposure. */
> -		int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN);
> -		int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME);
> -
> -		ControlList ctrls(unicam_ctrls_);
> -		ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
> -		ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> -
> -		IPAOperationData op;
> -		op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
> -		op.controls.push_back(ctrls);
> -		queueFrameAction.emit(0, op);
> +		/* Supply initial values for gain and exposure. */
> +		agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;
> +		agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;
>  	}
>  
>  	RPi::Metadata metadata;
>  	controller_.SwitchMode(mode_, &metadata);
>  
> +	/* SwitchMode may supply updated exposure/gain values to use. */
> +	metadata.Get("agc.status", agcStatus);
> +	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)
> +		applyAGC(&agcStatus);
> +
>  	lastMode_ = mode_;
>  }
>
David Plowman June 22, 2020, 8:09 a.m. UTC | #2 
Hi Laurent

On Mon, 22 Jun 2020 at 05:04, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> Nitpicking a little bit, the commit message subject prefix should be
> "libcamera: ipa: raspberrypi:". Same for 1/2.

Noted for next time!  :)

>
> On Thu, Jun 18, 2020 at 12:12:36PM +0100, David Plowman wrote:
> > This commit causes the AGC to recalculate its camera exposure/gain
> > values when the camera mode changes. For example it's possible
> > that the exposure profile could be changed by the application so
> > the division between exposure time and analogue gain may be
> > different.
> >
> > The other underlying reason (and which this commit accomplishes too)
> > is that the sensor's line timing may change in a new mode, and because
> > V4L2 drivers store a number of exposure _lines_, the resulting _time_
> > will "change under our feet". So we have to go through the process of
> > recalculating the correct number of lines and writing this back to the
> > sensor with every mode switch, regardless of anything else.
>
> I wonder if in the future we shouldn't try to abstract this in the
> CameraSensor class, to avoid having to deal with this sensor-specific
> behaviour in IPAs. Not something for this patch series.

Agree. I think the current behaviour is probably unhelpful for many
pipeline handlers.

Best regards
David

>
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Both patches applied to my tree with the subjects updated. I'll push to
> master after running the compilation tests.
>
> > ---
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp | 12 +++++++++++
> >  src/ipa/raspberrypi/controller/rpi/agc.hpp |  1 +
> >  src/ipa/raspberrypi/raspberrypi.cpp        | 25 +++++++++++-----------
> >  3 files changed, 26 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index a474287..c02b5ec 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -221,6 +221,18 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)
> >       constraint_mode_name_ = constraint_mode_name;
> >  }
> >
> > +void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
> > +{
> > +     // On a mode switch, it's possible the exposure profile could change,
> > +     // so we run through the dividing up of exposure/gain again and
> > +     // write the results into the metadata we've been given.
> > +     if (status_.total_exposure_value) {
> > +             housekeepConfig();
> > +             divvyupExposure();
> > +             writeAndFinish(metadata, false);
> > +     }
> > +}
> > +
> >  void Agc::Prepare(Metadata *image_metadata)
> >  {
> >       AgcStatus status;
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > index dbcefba..9a7e89c 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > @@ -75,6 +75,7 @@ public:
> >       void SetMeteringMode(std::string const &metering_mode_name) override;
> >       void SetExposureMode(std::string const &exposure_mode_name) override;
> >       void SetConstraintMode(std::string const &contraint_mode_name) override;
> > +     void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;
> >       void Prepare(Metadata *image_metadata) override;
> >       void Process(StatisticsPtr &stats, Metadata *image_metadata) override;
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index d6fd3df..42c84b1 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -247,29 +247,30 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >               mistrust_count_ = helper_->MistrustFramesStartup();
> >       }
> >
> > +     struct AgcStatus agcStatus;
> > +     /* These zero values mean not program anything (unless overwritten). */
> > +     agcStatus.shutter_time = 0.0;
> > +     agcStatus.analogue_gain = 0.0;
> > +
> >       if (!controllerInit_) {
> >               /* Load the tuning file for this sensor. */
> >               controller_.Read(tuningFile_.c_str());
> >               controller_.Initialise();
> >               controllerInit_ = true;
> >
> > -             /* Calculate initial values for gain and exposure. */
> > -             int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN);
> > -             int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME);
> > -
> > -             ControlList ctrls(unicam_ctrls_);
> > -             ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
> > -             ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> > -
> > -             IPAOperationData op;
> > -             op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
> > -             op.controls.push_back(ctrls);
> > -             queueFrameAction.emit(0, op);
> > +             /* Supply initial values for gain and exposure. */
> > +             agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;
> > +             agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;
> >       }
> >
> >       RPi::Metadata metadata;
> >       controller_.SwitchMode(mode_, &metadata);
> >
> > +     /* SwitchMode may supply updated exposure/gain values to use. */
> > +     metadata.Get("agc.status", agcStatus);
> > +     if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)
> > +             applyAGC(&agcStatus);
> > +
> >       lastMode_ = mode_;
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
index a474287..c02b5ec 100644
--- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
+++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
@@ -221,6 +221,18 @@  void Agc::SetConstraintMode(std::string const &constraint_mode_name)
 	constraint_mode_name_ = constraint_mode_name;
 }
 
+void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)
+{
+	// On a mode switch, it's possible the exposure profile could change,
+	// so we run through the dividing up of exposure/gain again and
+	// write the results into the metadata we've been given.
+	if (status_.total_exposure_value) {
+		housekeepConfig();
+		divvyupExposure();
+		writeAndFinish(metadata, false);
+	}
+}
+
 void Agc::Prepare(Metadata *image_metadata)
 {
 	AgcStatus status;
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
index dbcefba..9a7e89c 100644
--- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
+++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
@@ -75,6 +75,7 @@  public:
 	void SetMeteringMode(std::string const &metering_mode_name) override;
 	void SetExposureMode(std::string const &exposure_mode_name) override;
 	void SetConstraintMode(std::string const &contraint_mode_name) override;
+	void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;
 	void Prepare(Metadata *image_metadata) override;
 	void Process(StatisticsPtr &stats, Metadata *image_metadata) override;
 
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index d6fd3df..42c84b1 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -247,29 +247,30 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 		mistrust_count_ = helper_->MistrustFramesStartup();
 	}
 
+	struct AgcStatus agcStatus;
+	/* These zero values mean not program anything (unless overwritten). */
+	agcStatus.shutter_time = 0.0;
+	agcStatus.analogue_gain = 0.0;
+
 	if (!controllerInit_) {
 		/* Load the tuning file for this sensor. */
 		controller_.Read(tuningFile_.c_str());
 		controller_.Initialise();
 		controllerInit_ = true;
 
-		/* Calculate initial values for gain and exposure. */
-		int32_t gain_code = helper_->GainCode(DEFAULT_ANALOGUE_GAIN);
-		int32_t exposure_lines = helper_->ExposureLines(DEFAULT_EXPOSURE_TIME);
-
-		ControlList ctrls(unicam_ctrls_);
-		ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
-		ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
-
-		IPAOperationData op;
-		op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
-		op.controls.push_back(ctrls);
-		queueFrameAction.emit(0, op);
+		/* Supply initial values for gain and exposure. */
+		agcStatus.shutter_time = DEFAULT_EXPOSURE_TIME;
+		agcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;
 	}
 
 	RPi::Metadata metadata;
 	controller_.SwitchMode(mode_, &metadata);
 
+	/* SwitchMode may supply updated exposure/gain values to use. */
+	metadata.Get("agc.status", agcStatus);
+	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)
+		applyAGC(&agcStatus);
+
 	lastMode_ = mode_;
 }