[RFC PATCH 4/7] pipeline: rkisp1: Refactor setControls()

[Laurent Pinchart]

Hi Stefan,

Thank you for the patch.

The subject line is incorrect, this patch isn't about the pipeline
handler but the IPA module.

On Fri, Dec 20, 2024 at 05:26:50PM +0100, Stefan Klug wrote:
> IPARkISP1::setControls() is called when new sensor controls shall be
> queued in the pipeline handler. It constructs a list of sensor controls
> and then emits the setSensorControls signal.
> 
> To be able to return initial sensor controls from the IPARkISP1::start()
> function, similar functionality will be needed. Prepare for that by
> changing the setControls() function to a getSensorControls() and by moving
> the setSensorControls.emit() out of the function.
> 
> This patch contains no functional changes.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>  src/ipa/rkisp1/rkisp1.cpp | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 2ffdd99b158a..45a539a61fb1 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -76,7 +76,7 @@ private:
>  	void updateControls(const IPACameraSensorInfo &sensorInfo,
>  			    const ControlInfoMap &sensorControls,
>  			    ControlInfoMap *ipaControls);
> -	void setControls(unsigned int frame);
> +	ControlList getSensorControls(unsigned int frame);
>  
>  	std::map<unsigned int, FrameBuffer> buffers_;
>  	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
> @@ -211,7 +211,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>  
>  int IPARkISP1::start()
>  {
> -	setControls(0);
> +	ControlList ctrls = getSensorControls(0);

Will this work fine before patch 5/7 ? Or at least with no regression
(such as crashes) ?

> +	setSensorControls.emit(0, ctrls);
>  
>  	return 0;
>  }
> @@ -378,7 +379,12 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,
>  		algo->process(context_, frame, frameContext, stats, metadata);
>  	}
>  
> -	setControls(frame);
> +	/*
> +	 * \todo: Here we should do a lookahead that takes the sensor delays

The doxygen tag is '\todo', not '\todo:'.

> +	 * into account.
> +	 */

I'm not sure how accurate that is, but I suppose it's not worse than the
existing comment. I'd keep the mention of frame number though.

> +	ControlList ctrls = getSensorControls(frame);
> +	setSensorControls.emit(frame, ctrls);
>  
>  	context_.debugMetadata.moveEntries(metadata);
>  	metadataReady.emit(frame, metadata);
> @@ -443,13 +449,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>  	*ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
>  }
>  
> -void IPARkISP1::setControls(unsigned int frame)
> +ControlList IPARkISP1::getSensorControls(unsigned int frame)
>  {
> -	/*
> -	 * \todo The frame number is most likely wrong here, we need to take
> -	 * internal sensor delays and other timing parameters into account.
> -	 */
> -
>  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>  	uint32_t exposure = frameContext.agc.exposure;
>  	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
> @@ -457,8 +458,7 @@ void IPARkISP1::setControls(unsigned int frame)
>  	ControlList ctrls(sensorControls_);
>  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
>  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> -
> -	setSensorControls.emit(frame, ctrls);
> +	return ctrls;
>  }
>  
>  } /* namespace ipa::rkisp1 */

-- 
Regards,

Laurent Pinchart