[v2,12/32] pipeline: rkisp1: Fix controls in raw mode
diff mbox series

Message ID 20260325151416.2114564-13-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 
After the pipeline restructuring setSensorControls is no longer emitted
within process() but within computeParams(). In raw mode computeParams
is not called and therefore setSensorControls is never emitted. Fix that
by allowing computeParams to be called with an empty bufferId. This
strategy is also used for processStats() to ensure that metadata gets
filled in raw mode.  Then call computeParams within frameStart() when in
raw mode.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v2:
- Call computeParams() within queueRequestDevice for now to make the
  patch easier to follow. Moving computeParams to frameStart() will
happen in a later patch.
---
 src/ipa/rkisp1/rkisp1.cpp                | 12 ++++++++++++
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |  6 ++++++
 2 files changed, 18 insertions(+)

Comments

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

On Wed, Mar 25, 2026 at 04:13:44PM +0100, Stefan Klug wrote:
> After the pipeline restructuring setSensorControls is no longer emitted
> within process() but within computeParams(). In raw mode computeParams
> is not called and therefore setSensorControls is never emitted. Fix that
> by allowing computeParams to be called with an empty bufferId. This
> strategy is also used for processStats() to ensure that metadata gets
> filled in raw mode.  Then call computeParams within frameStart() when in
> raw mode.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
>
> ---
>
> Changes in v2:
> - Call computeParams() within queueRequestDevice for now to make the
>   patch easier to follow. Moving computeParams to frameStart() will
> happen in a later patch.
> ---
>  src/ipa/rkisp1/rkisp1.cpp                | 12 ++++++++++++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  6 ++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 81430d6532ac..e06238a7abe9 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -343,6 +343,18 @@ void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId)
>  {
>  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>
> +	/*
> +	 * \todo: This needs discussion. In raw mode, computeParams is
> +	 * called without a params buffer, to trigger the setSensorControls
> +	 * signal. Currently our algorithms don't support prepare calls with
> +	 * a nullptr. Do we need that or can we safely skip it?
> +	 */
> +	if (bufferId == 0) {
> +		ControlList ctrls = getSensorControls(frameContext);
> +		setSensorControls.emit(frame, ctrls);

I might have missed who updates the sensor's exposure/gain if we do
not call algo->prepare() ?

> +		return;
> +	}
> +
>  	RkISP1Params params(context_.configuration.paramFormat,
>  			    mappedBuffers_.at(bufferId).planes()[0]);
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index a3b78bf4dc6b..7352237dbb86 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1350,6 +1350,12 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>
>  		if (data->selfPath_ && info->selfPathBuffer)
>  			data->selfPath_->queueBuffer(info->selfPathBuffer);
> +
> +		/*
> +		 * Call computeParams with an empty param buffer to trigger the
> +		 * setSensorControls signal.
> +		 */
> +		data->ipa_->computeParams(data->frame_, 0);
>  	} else {
>  		data->ipa_->computeParams(data->frame_,
>  					  info->paramBuffer->cookie());
> --
> 2.51.0
>
Stefan Klug May 28, 2026, 1:31 p.m. UTC | #2 
Hi Jacopo,


Quoting Jacopo Mondi (2026-05-28 11:43:07)
> Hi Stefan
> 
> On Wed, Mar 25, 2026 at 04:13:44PM +0100, Stefan Klug wrote:
> > After the pipeline restructuring setSensorControls is no longer emitted
> > within process() but within computeParams(). In raw mode computeParams
> > is not called and therefore setSensorControls is never emitted. Fix that
> > by allowing computeParams to be called with an empty bufferId. This
> > strategy is also used for processStats() to ensure that metadata gets
> > filled in raw mode.  Then call computeParams within frameStart() when in
> > raw mode.
> >
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> >
> > ---
> >
> > Changes in v2:
> > - Call computeParams() within queueRequestDevice for now to make the
> >   patch easier to follow. Moving computeParams to frameStart() will
> > happen in a later patch.
> > ---
> >  src/ipa/rkisp1/rkisp1.cpp                | 12 ++++++++++++
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  6 ++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 81430d6532ac..e06238a7abe9 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -343,6 +343,18 @@ void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId)
> >  {
> >       IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> >
> > +     /*
> > +      * \todo: This needs discussion. In raw mode, computeParams is
> > +      * called without a params buffer, to trigger the setSensorControls
> > +      * signal. Currently our algorithms don't support prepare calls with
> > +      * a nullptr. Do we need that or can we safely skip it?
> > +      */
> > +     if (bufferId == 0) {
> > +             ControlList ctrls = getSensorControls(frameContext);
> > +             setSensorControls.emit(frame, ctrls);
> 
> I might have missed who updates the sensor's exposure/gain if we do
> not call algo->prepare() ?

As this path is exercised in RAW mode where we don't have auto
regulation I think it makes sense to ensure that algorithms have all the
sensor related fields set up in queueRequest. But you are right, this
only works properly if we actually call queueRequest on all the
algorithms. This is done in [PATCH v2 22/32] ipa: rkisp1: Lazy
initialise frame context.

Regards,
Stefan

> 
> > +             return;
> > +     }
> > +
> >       RkISP1Params params(context_.configuration.paramFormat,
> >                           mappedBuffers_.at(bufferId).planes()[0]);
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index a3b78bf4dc6b..7352237dbb86 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1350,6 +1350,12 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
> >
> >               if (data->selfPath_ && info->selfPathBuffer)
> >                       data->selfPath_->queueBuffer(info->selfPathBuffer);
> > +
> > +             /*
> > +              * Call computeParams with an empty param buffer to trigger the
> > +              * setSensorControls signal.
> > +              */
> > +             data->ipa_->computeParams(data->frame_, 0);
> >       } else {
> >               data->ipa_->computeParams(data->frame_,
> >                                         info->paramBuffer->cookie());
> > --
> > 2.51.0
> >

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 81430d6532ac..e06238a7abe9 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -343,6 +343,18 @@  void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId)
 {
 	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
 
+	/*
+	 * \todo: This needs discussion. In raw mode, computeParams is
+	 * called without a params buffer, to trigger the setSensorControls
+	 * signal. Currently our algorithms don't support prepare calls with
+	 * a nullptr. Do we need that or can we safely skip it?
+	 */
+	if (bufferId == 0) {
+		ControlList ctrls = getSensorControls(frameContext);
+		setSensorControls.emit(frame, ctrls);
+		return;
+	}
+
 	RkISP1Params params(context_.configuration.paramFormat,
 			    mappedBuffers_.at(bufferId).planes()[0]);
 
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index a3b78bf4dc6b..7352237dbb86 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -1350,6 +1350,12 @@  int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
 
 		if (data->selfPath_ && info->selfPathBuffer)
 			data->selfPath_->queueBuffer(info->selfPathBuffer);
+
+		/*
+		 * Call computeParams with an empty param buffer to trigger the
+		 * setSensorControls signal.
+		 */
+		data->ipa_->computeParams(data->frame_, 0);
 	} else {
 		data->ipa_->computeParams(data->frame_,
 					  info->paramBuffer->cookie());