[v1,14/35] pipeline: rkisp1: Apply initial controls

24780 diff mbox series

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(-)

Hi Stefan

On Fri, Oct 24, 2025 at 10:50:38AM +0200, 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
> 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(-)
>
> 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 a55bac260026..7a7b7682e242 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 = {};

Shouldn't this go through the frame context queue alloc() function ?

Or as we have no real frame context associated with start, we
shouldn't allocate one ?

Or is it fine to allocate a frame context for frame #0 as the sensor's
sequence starts from #1 ?


> +	initializeFrameContext(0, frameContext, controls);

What would really be nice would be for the frame context queue to be
able to call in the IPA.

The frame context queue already has logic in place to call
FrameContext::init() at the right time according to the condition
where get() and alloc() are called.

It seems that with the introduction of initializeFrameContext() you're
replicating part of that in the IPA (an indication of that is that the
base struct FrameContext has an initalised_ member which you have now
duplicated in RkISP1FrameContext).

In
ipa: libipa: Reduce log level when obtaining an uninitialized frame context

you have demoted the Warning message when a get() is called before an
alloc(). This is a sign that possibily the two functions are now
equally valid as receiving a computeParams() or processStats() call
before a queueRequest() is a valid use case ?

More on this on
ipa: rkisp1: Lazy initialise frame context

> +	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 0e6e45bba75b..d937c94e351f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1290,13 +1290,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;
>
> --
> 2.48.1
>