[libcamera-devel,3/3] pipeline: ipa: raspberrypi: Pass controls to IPA on start
diff mbox series

Message ID 20201112085915.3053-4-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Pass controls on camera:start()
Related show

Commit Message

Naushir Patuck Nov. 12, 2020, 8:59 a.m. UTC 
Forward any controls passed into the pipeline handler to the IPA.
The IPA then sets up the Raspberry Pi controller with these settings
appropriately, and passes back any V4L2 sensor controls that need
to be applied.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.h           |  1 +
 src/ipa/raspberrypi/raspberrypi.cpp           | 51 ++++++++++++-------
 .../pipeline/raspberrypi/raspberrypi.cpp      | 14 ++++-
 3 files changed, 46 insertions(+), 20 deletions(-)

Comments

David Plowman Nov. 17, 2020, 4:36 p.m. UTC | #1 
Hi Naush

Thanks for the patch! Only a couple of very minor things...

On Thu, 12 Nov 2020 at 08:59, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Forward any controls passed into the pipeline handler to the IPA.
> The IPA then sets up the Raspberry Pi controller with these settings
> appropriately, and passes back any V4L2 sensor controls that need
> to be applied.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.h           |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp           | 51 ++++++++++++-------
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 14 ++++-
>  3 files changed, 46 insertions(+), 20 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 2b55dbfc..c91a14bd 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -21,6 +21,7 @@ enum ConfigParameters {
>         IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
>         IPA_CONFIG_SENSOR = (1 << 2),
>         IPA_CONFIG_DROP_FRAMES = (1 << 3),
> +       IPA_CONFIG_STARTUP = (1 << 4)
>  };
>
>  enum Operations {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 7a07b477..d4471f02 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -77,8 +77,7 @@ public:
>         }
>
>         int init(const IPASettings &settings) override;
> -       int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> -                 [[maybe_unused]] IPAOperationData *result) override { return 0; }
> +       int start(const IPAOperationData &ipaConfig, IPAOperationData *result) override;
>         void stop() override {}
>
>         void configure(const CameraSensorInfo &sensorInfo,
> @@ -154,6 +153,35 @@ int IPARPi::init(const IPASettings &settings)
>         return 0;
>  }
>
> +int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)
> +{
> +       RPiController::Metadata metadata;
> +
> +       result->operation = 0;

Ah, I see we zero this here, answering some of my questions about the
previous patch!

> +       if (ipaConfig.operation & RPi::IPA_CONFIG_STARTUP) {
> +               /* We have been given some controls to action before start. */
> +               queueRequest(ipaConfig.controls[0]);
> +       }
> +
> +       controller_.SwitchMode(mode_, &metadata);
> +
> +       /* SwitchMode may supply updated exposure/gain values to use. */
> +       AgcStatus agcStatus;
> +       agcStatus.shutter_time = 0.0;
> +       agcStatus.analogue_gain = 0.0;
> +
> +       /* 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) {
> +               ControlList ctrls(unicamCtrls_);
> +               applyAGC(&agcStatus, ctrls);
> +               result->controls.push_back(ctrls);

I wonder if there's just a little bit more copying of controls going
on here than is strictly necessary (maybe emplace_back?), but I agree
that it's entirely inconsequential.

> +               result->operation |= RPi::IPA_CONFIG_SENSOR;
> +       }
> +
> +       return 0;
> +}
> +
>  void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
>  {
>         mode_.bitdepth = sensorInfo.bitsPerPixel;
> @@ -229,7 +257,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>                 result->data.push_back(gainDelay);
>                 result->data.push_back(exposureDelay);
>                 result->data.push_back(sensorMetadata);
> -
>                 result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
>         }
>
> @@ -285,11 +312,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>         result->data.push_back(dropFrame);
>         result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
>
> -       /* These zero values mean not program anything (unless overwritten). */
> -       struct AgcStatus agcStatus;
> -       agcStatus.shutter_time = 0.0;
> -       agcStatus.analogue_gain = 0.0;
> -
>         if (!controllerInit_) {
>                 /* Load the tuning file for this sensor. */
>                 controller_.Read(tuningFile_.c_str());
> @@ -297,20 +319,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>                 controllerInit_ = true;
>
>                 /* Supply initial values for gain and exposure. */
> +               ControlList ctrls(unicamCtrls_);
> +               AgcStatus agcStatus;
>                 agcStatus.shutter_time = DefaultExposureTime;
>                 agcStatus.analogue_gain = DefaultAnalogueGain;
> -       }
> -
> -       RPiController::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) {
> -               ControlList ctrls(unicamCtrls_);
>                 applyAGC(&agcStatus, ctrls);
> -               result->controls.push_back(ctrls);
>
> +               result->controls.push_back(ctrls);

Same comment here. Though I wonder if maybe I wrote this originally,
which would make it all my fault (most things are!).

>                 result->operation |= RPi::IPA_CONFIG_SENSOR;
>         }
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index a8e4997a..6efe2403 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -748,7 +748,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>
>         /* Start the IPA. */
>         IPAOperationData ipaConfig, result;
> -       ipaConfig.controls.emplace_back(*controls);
> +       if (controls) {
> +               ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;
> +               ipaConfig.controls.emplace_back(*controls);
> +       }

I guess this is the spot where I'd just want to convince myself
nothing bad can happen if ipaConfig.operation is undefined but
controls was NULL. I'm probably worrying over nothing...

Those little things aside:

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Tested-by: David Plowman <david.plowman@raspberrypi.com>

Thanks and best regards
David

>         ret = data->ipa_->start(ipaConfig, &result);
>         if (ret) {
>                 LOG(RPI, Error)
> @@ -757,6 +760,14 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>                 return ret;
>         }
>
> +       /* Apply any gain/exposure settings that the IPA may have passed back. */
> +       ASSERT(data->staggeredCtrl_);
> +       if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> +               const ControlList &ctrls = result.controls[0];
> +               if (!data->staggeredCtrl_.set(ctrls))
> +                       LOG(RPI, Error) << "V4L2 staggered set failed";
> +       }
> +
>         /*
>          * IPA configure may have changed the sensor flips - hence the bayer
>          * order. Get the sensor format and set the ISP input now.
> @@ -777,7 +788,6 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>          * starting. First check that the staggered ctrl has been initialised
>          * by configure().
>          */
> -       ASSERT(data->staggeredCtrl_);
>         data->staggeredCtrl_.reset();
>         data->staggeredCtrl_.write();
>         data->expectedSequence_ = 0;
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index 2b55dbfc..c91a14bd 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -21,6 +21,7 @@  enum ConfigParameters {
 	IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
 	IPA_CONFIG_SENSOR = (1 << 2),
 	IPA_CONFIG_DROP_FRAMES = (1 << 3),
+	IPA_CONFIG_STARTUP = (1 << 4)
 };
 
 enum Operations {
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 7a07b477..d4471f02 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -77,8 +77,7 @@  public:
 	}
 
 	int init(const IPASettings &settings) override;
-	int start([[maybe_unused]] const IPAOperationData &ipaConfig,
-		  [[maybe_unused]] IPAOperationData *result) override { return 0; }
+	int start(const IPAOperationData &ipaConfig, IPAOperationData *result) override;
 	void stop() override {}
 
 	void configure(const CameraSensorInfo &sensorInfo,
@@ -154,6 +153,35 @@  int IPARPi::init(const IPASettings &settings)
 	return 0;
 }
 
+int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)
+{
+	RPiController::Metadata metadata;
+
+	result->operation = 0;
+	if (ipaConfig.operation & RPi::IPA_CONFIG_STARTUP) {
+		/* We have been given some controls to action before start. */
+		queueRequest(ipaConfig.controls[0]);
+	}
+
+	controller_.SwitchMode(mode_, &metadata);
+
+	/* SwitchMode may supply updated exposure/gain values to use. */
+	AgcStatus agcStatus;
+	agcStatus.shutter_time = 0.0;
+	agcStatus.analogue_gain = 0.0;
+
+	/* 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) {
+		ControlList ctrls(unicamCtrls_);
+		applyAGC(&agcStatus, ctrls);
+		result->controls.push_back(ctrls);
+		result->operation |= RPi::IPA_CONFIG_SENSOR;
+	}
+
+	return 0;
+}
+
 void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
 {
 	mode_.bitdepth = sensorInfo.bitsPerPixel;
@@ -229,7 +257,6 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 		result->data.push_back(gainDelay);
 		result->data.push_back(exposureDelay);
 		result->data.push_back(sensorMetadata);
-
 		result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
 	}
 
@@ -285,11 +312,6 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 	result->data.push_back(dropFrame);
 	result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
 
-	/* These zero values mean not program anything (unless overwritten). */
-	struct AgcStatus agcStatus;
-	agcStatus.shutter_time = 0.0;
-	agcStatus.analogue_gain = 0.0;
-
 	if (!controllerInit_) {
 		/* Load the tuning file for this sensor. */
 		controller_.Read(tuningFile_.c_str());
@@ -297,20 +319,13 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 		controllerInit_ = true;
 
 		/* Supply initial values for gain and exposure. */
+		ControlList ctrls(unicamCtrls_);
+		AgcStatus agcStatus;
 		agcStatus.shutter_time = DefaultExposureTime;
 		agcStatus.analogue_gain = DefaultAnalogueGain;
-	}
-
-	RPiController::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) {
-		ControlList ctrls(unicamCtrls_);
 		applyAGC(&agcStatus, ctrls);
-		result->controls.push_back(ctrls);
 
+		result->controls.push_back(ctrls);
 		result->operation |= RPi::IPA_CONFIG_SENSOR;
 	}
 
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index a8e4997a..6efe2403 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -748,7 +748,10 @@  int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
 
 	/* Start the IPA. */
 	IPAOperationData ipaConfig, result;
-	ipaConfig.controls.emplace_back(*controls);
+	if (controls) {
+		ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;
+		ipaConfig.controls.emplace_back(*controls);
+	}
 	ret = data->ipa_->start(ipaConfig, &result);
 	if (ret) {
 		LOG(RPI, Error)
@@ -757,6 +760,14 @@  int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
 		return ret;
 	}
 
+	/* Apply any gain/exposure settings that the IPA may have passed back. */
+	ASSERT(data->staggeredCtrl_);
+	if (result.operation & RPi::IPA_CONFIG_SENSOR) {
+		const ControlList &ctrls = result.controls[0];
+		if (!data->staggeredCtrl_.set(ctrls))
+			LOG(RPI, Error) << "V4L2 staggered set failed";
+	}
+
 	/*
 	 * IPA configure may have changed the sensor flips - hence the bayer
 	 * order. Get the sensor format and set the ISP input now.
@@ -777,7 +788,6 @@  int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
 	 * starting. First check that the staggered ctrl has been initialised
 	 * by configure().
 	 */
-	ASSERT(data->staggeredCtrl_);
 	data->staggeredCtrl_.reset();
 	data->staggeredCtrl_.write();
 	data->expectedSequence_ = 0;