[v2,14/32] pipeline: rkisp1: Apply initial controls
diff mbox series

Message ID 20260325151416.2114564-15-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • rkisp1: pipeline rework for PFC
Related show

Commit Message

Stefan Klug March 25, 2026, 3:13 p.m. UTC 
Controls passed to Camera::start() are not handled at the moment.
Implement that by initializing a separate frame
context and then returning the controls synchronously to the caller. In
the pipeline the controls get passed to the sensor before the call to
streamOn() to ensure the controls are applied right away. Passing the
controls to the pipeline using the setSensorControls signal is not
appropriate because it is asynchronous and would reach the sensor too
late (initial controls need to be applied before the sensor starts and
before delayed controls initializes it's start condition.)

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 include/libcamera/ipa/rkisp1.mojom       |  7 ++++++-
 src/ipa/rkisp1/rkisp1.cpp                | 10 ++++++----
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +++++++++--
 3 files changed, 21 insertions(+), 7 deletions(-)

Comments

Jacopo Mondi May 28, 2026, 10:04 a.m. UTC | #1 
Hi Stefan

On Wed, Mar 25, 2026 at 04:13:46PM +0100, Stefan Klug wrote:
> Controls passed to Camera::start() are not handled at the moment.
> Implement that by initializing a separate frame
> context and then returning the controls synchronously to the caller. In

please reflow the above lines

> the pipeline the controls get passed to the sensor before the call to
> streamOn() to ensure the controls are applied right away. Passing the
> controls to the pipeline using the setSensorControls signal is not
> appropriate because it is asynchronous and would reach the sensor too
> late (initial controls need to be applied before the sensor starts and
> before delayed controls initializes it's start condition.)

Am I correct this only support manual controls ?

As I understand it we emit 3 sensor controls from the RkISP1 pipeline
(EXPOSURE, VBLANK, ANALOGUE_GAIN) and these are populated by
getSensorControls() with the values from the FrameContext.

As you call initializeFrameContext() which runs algo->queueRequest(),
gains and exposure and frame duration are only populated by the Agc if
controls are in manual mode:

	if (!frameContext.agc.autoExposureEnabled)
		frameContext.agc.exposure = agc.manual.exposure;
	if (!frameContext.agc.autoGainEnabled)
		frameContext.agc.gain = agc.manual.gain;

(vblank seems not handled. I presume it's properly initialized ?)

is it worth clarifying this only apply to manual controls, or is it
obvious and it's just me having realized it now ?

>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  include/libcamera/ipa/rkisp1.mojom       |  7 ++++++-
>  src/ipa/rkisp1/rkisp1.cpp                | 10 ++++++----
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +++++++++--
>  3 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index 068e898848c4..4c29b53cd7f9 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -14,13 +14,18 @@ struct IPAConfigInfo {
>  	uint32 paramFormat;
>  };
>
> +struct StartResult {
> +	libcamera.ControlList controls;
> +	int32 code;
> +};
> +
>  interface IPARkISP1Interface {
>  	init(libcamera.IPASettings settings,
>  	     uint32 hwRevision, uint32 supportedBlocks,
>  	     libcamera.IPACameraSensorInfo sensorInfo,
>  	     libcamera.ControlInfoMap sensorControls)
>  		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
> -	start() => (int32 ret);
> +	start(libcamera.ControlList controls) => (StartResult result);
>  	stop();
>
>  	configure(IPAConfigInfo configInfo,
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 98ed4a8ff16d..3f943a08d011 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -56,7 +56,7 @@ public:
>  		 const IPACameraSensorInfo &sensorInfo,
>  		 const ControlInfoMap &sensorControls,
>  		 ControlInfoMap *ipaControls) override;
> -	int start() override;
> +	void start(const ControlList &controls, StartResult *result) override;
>  	void stop() override;
>
>  	int configure(const IPAConfigInfo &ipaConfig,
> @@ -215,10 +215,12 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>  	return 0;
>  }
>
> -int IPARkISP1::start()
> +void IPARkISP1::start(const ControlList &controls, StartResult *result)
>  {
> -	/* \todo Properly handle startup controls. */
> -	return 0;
> +	IPAFrameContext frameContext = {};
> +	initializeFrameContext(0, frameContext, controls);
> +	result->controls = getSensorControls(frameContext);
> +	result->code = 0;
>  }
>
>  void IPARkISP1::stop()
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 7352237dbb86..89e8ab0999f4 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1238,13 +1238,20 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
>  		return ret;
>  	actions += [&]() { freeBuffers(camera); };
>
> -	ret = data->ipa_->start();
> -	if (ret) {
> +	ControlList ctrls;
> +	if (!!controls)

Do you need !! ?

> +		ctrls = *controls;

Why a copy ?

> +
> +	ipa::rkisp1::StartResult res;
> +	data->ipa_->start(ctrls, &res);

I see rpi passing in an empty control list in case !controls

	data->ipa_->start(controls ? *controls : ControlList{ controls::controls },
			  &result);

Can't we do the same ?

> +	if (res.code) {

What are the possibile error conditions ?
I don't see IPA::start() ever initalizing it to anything != 0

>  		LOG(RkISP1, Error)
>  			<< "Failed to start IPA " << camera->id();
>  		return ret;
>  	}
>  	actions += [&]() { data->ipa_->stop(); };
> +	data->sensor_->setControls(&res.controls);
> +	data->delayedCtrls_->reset();
>
>  	data->frame_ = 0;
>
> --
> 2.51.0
>
Stefan Klug May 28, 2026, 1:44 p.m. UTC | #2 
Hi Jacopo,

Quoting Jacopo Mondi (2026-05-28 12:04:34)
> Hi Stefan
> 
> On Wed, Mar 25, 2026 at 04:13:46PM +0100, Stefan Klug wrote:
> > Controls passed to Camera::start() are not handled at the moment.
> > Implement that by initializing a separate frame
> > context and then returning the controls synchronously to the caller. In
> 
> please reflow the above lines
> 
> > the pipeline the controls get passed to the sensor before the call to
> > streamOn() to ensure the controls are applied right away. Passing the
> > controls to the pipeline using the setSensorControls signal is not
> > appropriate because it is asynchronous and would reach the sensor too
> > late (initial controls need to be applied before the sensor starts and
> > before delayed controls initializes it's start condition.)
> 
> Am I correct this only support manual controls ?
> 
> As I understand it we emit 3 sensor controls from the RkISP1 pipeline
> (EXPOSURE, VBLANK, ANALOGUE_GAIN) and these are populated by
> getSensorControls() with the values from the FrameContext.
> 
> As you call initializeFrameContext() which runs algo->queueRequest(),
> gains and exposure and frame duration are only populated by the Agc if
> controls are in manual mode:
> 
>         if (!frameContext.agc.autoExposureEnabled)
>                 frameContext.agc.exposure = agc.manual.exposure;
>         if (!frameContext.agc.autoGainEnabled)
>                 frameContext.agc.gain = agc.manual.gain;
> 
> (vblank seems not handled. I presume it's properly initialized ?)

Yes, you're righht. This is fixed in [PATCH v2 16/32] ipa: rkisp1: agc:
Process frame duration at the right time.

> 
> is it worth clarifying this only apply to manual controls, or is it
> obvious and it's just me having realized it now ?

Yes, I might need to reorder the patches here. Setting these in auto
mode also is implemented in the next patch "[PATCH v2 15/32] ipa:
rkisp1: Set frameContext.agc in queueRequest for auto mode also"

Best regards,
Stefan


> 
> >
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  include/libcamera/ipa/rkisp1.mojom       |  7 ++++++-
> >  src/ipa/rkisp1/rkisp1.cpp                | 10 ++++++----
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +++++++++--
> >  3 files changed, 21 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index 068e898848c4..4c29b53cd7f9 100644
> > --- a/include/libcamera/ipa/rkisp1.mojom
> > +++ b/include/libcamera/ipa/rkisp1.mojom
> > @@ -14,13 +14,18 @@ struct IPAConfigInfo {
> >       uint32 paramFormat;
> >  };
> >
> > +struct StartResult {
> > +     libcamera.ControlList controls;
> > +     int32 code;
> > +};
> > +
> >  interface IPARkISP1Interface {
> >       init(libcamera.IPASettings settings,
> >            uint32 hwRevision, uint32 supportedBlocks,
> >            libcamera.IPACameraSensorInfo sensorInfo,
> >            libcamera.ControlInfoMap sensorControls)
> >               => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > -     start() => (int32 ret);
> > +     start(libcamera.ControlList controls) => (StartResult result);
> >       stop();
> >
> >       configure(IPAConfigInfo configInfo,
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 98ed4a8ff16d..3f943a08d011 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -56,7 +56,7 @@ public:
> >                const IPACameraSensorInfo &sensorInfo,
> >                const ControlInfoMap &sensorControls,
> >                ControlInfoMap *ipaControls) override;
> > -     int start() override;
> > +     void start(const ControlList &controls, StartResult *result) override;
> >       void stop() override;
> >
> >       int configure(const IPAConfigInfo &ipaConfig,
> > @@ -215,10 +215,12 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> >       return 0;
> >  }
> >
> > -int IPARkISP1::start()
> > +void IPARkISP1::start(const ControlList &controls, StartResult *result)
> >  {
> > -     /* \todo Properly handle startup controls. */
> > -     return 0;
> > +     IPAFrameContext frameContext = {};
> > +     initializeFrameContext(0, frameContext, controls);
> > +     result->controls = getSensorControls(frameContext);
> > +     result->code = 0;
> >  }
> >
> >  void IPARkISP1::stop()
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 7352237dbb86..89e8ab0999f4 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1238,13 +1238,20 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
> >               return ret;
> >       actions += [&]() { freeBuffers(camera); };
> >
> > -     ret = data->ipa_->start();
> > -     if (ret) {
> > +     ControlList ctrls;
> > +     if (!!controls)
> 
> Do you need !! ?
> 
> > +             ctrls = *controls;
> 
> Why a copy ?
> 
> > +
> > +     ipa::rkisp1::StartResult res;
> > +     data->ipa_->start(ctrls, &res);
> 
> I see rpi passing in an empty control list in case !controls
> 
>         data->ipa_->start(controls ? *controls : ControlList{ controls::controls },
>                           &result);
> 
> Can't we do the same ?
> 
> > +     if (res.code) {
> 
> What are the possibile error conditions ?
> I don't see IPA::start() ever initalizing it to anything != 0
> 
> >               LOG(RkISP1, Error)
> >                       << "Failed to start IPA " << camera->id();
> >               return ret;
> >       }
> >       actions += [&]() { data->ipa_->stop(); };
> > +     data->sensor_->setControls(&res.controls);
> > +     data->delayedCtrls_->reset();
> >
> >       data->frame_ = 0;
> >
> > --
> > 2.51.0
> >

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
index 068e898848c4..4c29b53cd7f9 100644
--- a/include/libcamera/ipa/rkisp1.mojom
+++ b/include/libcamera/ipa/rkisp1.mojom
@@ -14,13 +14,18 @@  struct IPAConfigInfo {
 	uint32 paramFormat;
 };
 
+struct StartResult {
+	libcamera.ControlList controls;
+	int32 code;
+};
+
 interface IPARkISP1Interface {
 	init(libcamera.IPASettings settings,
 	     uint32 hwRevision, uint32 supportedBlocks,
 	     libcamera.IPACameraSensorInfo sensorInfo,
 	     libcamera.ControlInfoMap sensorControls)
 		=> (int32 ret, libcamera.ControlInfoMap ipaControls);
-	start() => (int32 ret);
+	start(libcamera.ControlList controls) => (StartResult result);
 	stop();
 
 	configure(IPAConfigInfo configInfo,
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 98ed4a8ff16d..3f943a08d011 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -56,7 +56,7 @@  public:
 		 const IPACameraSensorInfo &sensorInfo,
 		 const ControlInfoMap &sensorControls,
 		 ControlInfoMap *ipaControls) override;
-	int start() override;
+	void start(const ControlList &controls, StartResult *result) override;
 	void stop() override;
 
 	int configure(const IPAConfigInfo &ipaConfig,
@@ -215,10 +215,12 @@  int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
 	return 0;
 }
 
-int IPARkISP1::start()
+void IPARkISP1::start(const ControlList &controls, StartResult *result)
 {
-	/* \todo Properly handle startup controls. */
-	return 0;
+	IPAFrameContext frameContext = {};
+	initializeFrameContext(0, frameContext, controls);
+	result->controls = getSensorControls(frameContext);
+	result->code = 0;
 }
 
 void IPARkISP1::stop()
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 7352237dbb86..89e8ab0999f4 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -1238,13 +1238,20 @@  int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL
 		return ret;
 	actions += [&]() { freeBuffers(camera); };
 
-	ret = data->ipa_->start();
-	if (ret) {
+	ControlList ctrls;
+	if (!!controls)
+		ctrls = *controls;
+
+	ipa::rkisp1::StartResult res;
+	data->ipa_->start(ctrls, &res);
+	if (res.code) {
 		LOG(RkISP1, Error)
 			<< "Failed to start IPA " << camera->id();
 		return ret;
 	}
 	actions += [&]() { data->ipa_->stop(); };
+	data->sensor_->setControls(&res.controls);
+	data->delayedCtrls_->reset();
 
 	data->frame_ = 0;