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

[Paul Elder]

On Mon, Jan 13, 2025 at 12:09:18AM +0200, Laurent Pinchart wrote:
> 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) ?

This doesn't change functionality (when combined with the line below) so
I expect it to be fine.


Paul

> 
> > +	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