[RFC,4/7] pipeline: rkisp1: Refactor setControls()
diff mbox series

Message ID 20241220162724.756494-5-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • Improve regulation on rkisp1
Related show

Commit Message

Stefan Klug Dec. 20, 2024, 4:26 p.m. UTC 
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@ideasonboard.com>
---
 src/ipa/rkisp1/rkisp1.cpp | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Laurent Pinchart Jan. 12, 2025, 10:09 p.m. UTC | #1 
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@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 */
Paul Elder Feb. 20, 2025, 8:25 a.m. UTC | #2 
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@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
Paul Elder Feb. 20, 2025, 8:49 a.m. UTC | #3 
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@ideasonboard.com>

With Laurent's comments addressed,

Reviewed-by: Paul Elder <paul.elder@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);
> +	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
> +	 * into account.
> +	 */
> +	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 */
> -- 
> 2.43.0
>

Patch
diff mbox series 

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);
+	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
+	 * into account.
+	 */
+	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 */