[RFC PATCH 5/7] pipeline: rkisp1: Apply initial controls

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

On Fri, Dec 20, 2024 at 05:26:51PM +0100, Stefan Klug wrote:
> Controls passed to Camera::start() are not handled at the moment.
> Implement that by issuing a dummy queueRequest() to initialize the 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
> stremOn() to ensure the controls are applied right away. Passing the

s/stremOn/streamOn/

> 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 at ideasonboard.com>
> ---
>  include/libcamera/ipa/rkisp1.mojom       |  7 ++++++-
>  src/ipa/rkisp1/rkisp1.cpp                | 15 ++++++++++-----
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +++++++++--
>  3 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index 043ad27ea199..86eae55cd530 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,
>  	     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 45a539a61fb1..799fe0846635 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -55,7 +55,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,
> @@ -209,12 +209,17 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>  	return 0;
>  }
>  
> -int IPARkISP1::start()
> +void IPARkISP1::start(const ControlList &controls, StartResult *result)
>  {
> -	ControlList ctrls = getSensorControls(0);
> -	setSensorControls.emit(0, ctrls);
> +	/*
> +	 * \todo: This feels a bit counter intuitive, as the queueRequest() for frame
> +	 * 0 will be called again when the actual request for frame 0 get's
> +	 * queued.
> +	 */

s/todo:/todo/

It's more than counterintuitive, I think it's an important design
problem. I'm OK with this dirty hack for now, but I think it should be
addressed sooner than later. Can you word the \todo comment a bit
stronger ?

> +	queueRequest(0, controls);
>  
> -	return 0;
> +	result->controls = getSensorControls(0);
> +	result->code = 0;
>  }
>  
>  void IPARkISP1::stop()
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 71a07c6027b9..ee6ed1b9c84a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1086,13 +1086,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();

Resetting the delayed controls seems to be an important bug fix. It
should be mentioned in the commit message, or better split to a separate
commit.

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

>  
>  	data->frame_ = 0;
>  

-- 
Regards,

Laurent Pinchart