[v18,10/12] libcamera: software_isp: Make measurement configurable
diff mbox series

Message ID 20250912142915.53949-12-mzamazal@redhat.com
State Accepted
Headers show
Series
  • Add global configuration file
Related show

Commit Message

Milan Zamazal Sept. 12, 2025, 2:29 p.m. UTC 
Software ISP performs performance measurement on certain part of initial
frames.  Let's make this range configurable.

For this purpose, this patch introduces new configuration options
pipelines.simple.measure.skip and pipelines.simple.measure.number.
Setting the latter one to 0 disables the measurement.

Instead of the last frame, the class member and its configuration
specify the number of frames to measure.  This is easier to use for
users and doesn't require to adjust two configuration parameters when
the number of the initially skipped frames is changed.

The patch also changes the names of the class members to make them more
accurate.

Completes software ISP TODO #7.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 Documentation/runtime_configuration.rst    | 18 ++++++++++++++++
 src/libcamera/software_isp/TODO            | 25 ----------------------
 src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++--------
 src/libcamera/software_isp/debayer_cpu.h   |  7 +++---
 4 files changed, 37 insertions(+), 38 deletions(-)

Comments

Laurent Pinchart Sept. 21, 2025, 3:05 a.m. UTC | #1 
Hi Milan,

On Fri, Sep 12, 2025 at 04:29:12PM +0200, Milan Zamazal wrote:
> Software ISP performs performance measurement on certain part of initial
> frames.  Let's make this range configurable.
> 
> For this purpose, this patch introduces new configuration options
> pipelines.simple.measure.skip and pipelines.simple.measure.number.

It's software_isp.measure.skip and software_isp.measure.number. I'll fix
it.

> Setting the latter one to 0 disables the measurement.
> 
> Instead of the last frame, the class member and its configuration
> specify the number of frames to measure.  This is easier to use for
> users and doesn't require to adjust two configuration parameters when
> the number of the initially skipped frames is changed.
> 
> The patch also changes the names of the class members to make them more
> accurate.
> 
> Completes software ISP TODO #7.
> 
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  Documentation/runtime_configuration.rst    | 18 ++++++++++++++++
>  src/libcamera/software_isp/TODO            | 25 ----------------------
>  src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++--------
>  src/libcamera/software_isp/debayer_cpu.h   |  7 +++---
>  4 files changed, 37 insertions(+), 38 deletions(-)
> 
> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
> index 6610e8bec..702139544 100644
> --- a/Documentation/runtime_configuration.rst
> +++ b/Documentation/runtime_configuration.rst
> @@ -57,6 +57,9 @@ file structure:
>            software_isp: # true/false
>      software_isp:
>        copy_input_buffer: # true/false
> +      measure:

I'd name this perf_measure (or something similar). I'll send a patch on
top to propose the change.

> +        skip: # non-negative integer, frames to skip initially
> +        number: # non-negative integer, frames to measure
>  
>  Configuration file example
>  --------------------------
> @@ -92,6 +95,9 @@ Configuration file example
>             software_isp: true
>       software_isp:
>         copy_input_buffer: false
> +       measure:
> +         skip: 50
> +         number: 30
>  
>  List of variables and configuration options
>  -------------------------------------------
> @@ -167,6 +173,18 @@ software_isp.copy_input_buffer
>  
>     Example value: ``false``
>  
> +software_isp.measure.skip, software_isp.measure.number
> +   Define per-frame time measurement parameters in software ISP. `skip`

"Define performance measurement parameters" would be more accurate I
think. I'll also send a patch on top.

> +   defines how many initial frames are skipped before starting the
> +   measurement; `number` defines how many frames then participate in the
> +   measurement.
> +
> +   Set `software_isp.measure.number` to 0 to disable the measurement.
> +
> +   Example `skip` value: ``50``
> +
> +   Example `number` value: ``30``
> +
>  Further details
>  ---------------
>  
> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
> index 2c919f442..f19e15ae2 100644
> --- a/src/libcamera/software_isp/TODO
> +++ b/src/libcamera/software_isp/TODO
> @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs.
>  
>  ---
>  
> -7. Performance measurement configuration
> -
> -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)
> -> /* Measure before emitting signals */
> -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
> ->     ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
> -> 	timespec frameEndTime = {};
> -> 	clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);
> -> 	frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);
> -> 	if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {
> -> 		const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -
> -> 						    DebayerCpu::kFramesToSkip;
> -> 		LOG(Debayer, Info)
> -> 			<< "Processed " << measuredFrames
> -> 			<< " frames in " << frameProcessTime_ / 1000 << "us, "
> -> 			<< frameProcessTime_ / (1000 * measuredFrames)
> -> 			<< " us/frame";
> -> 	}
> -> }
> -
> -I wonder if there would be a way to control at runtime when/how to
> -perform those measurements. Maybe that's a bit overkill.
> -
> ----
> -
>  8. DebayerCpu cleanups
>  
>  > >> class DebayerCpu : public Debayer, public Object
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index cec6cc6be..2f57857ad 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfigurat
>  	enableInputMemcpy_ =
>  		configuration.option<bool>({ "software_isp", "copy_input_buffer" }).value_or(true);
>  
> +	skipBeforeMeasure_ = configuration.option<unsigned int>(
> +						  { "software_isp", "measure", "skip" })
> +				     .value_or(skipBeforeMeasure_);
> +	framesToMeasure_ = configuration.option<unsigned int>(
> +						{ "software_isp", "measure", "number" })
> +				   .value_or(framesToMeasure_);

Seeing how the GlobalConfiguration class API is used through the series
for different purposes gave me a good view of usage patterns, and a few
ideas for improvements. There will be RFC patches coming.

> +
>  	/* Initialize color lookup tables */
>  	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>  		red_[i] = green_[i] = blue_[i] = i;
> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>  			lineBuffers_[i].resize(lineBufferLength_);
>  	}
>  
> -	measuredFrames_ = 0;
> +	encounteredFrames_ = 0;

Not too fond of this new name as the counter is used for performance
measurement only, I think the old name was better (even if not perfect).
This can be addressed later.

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

>  	frameProcessTime_ = 0;
>  
>  	return 0;
> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output
>  {
>  	timespec frameStartTime;
>  
> -	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {
> +	bool measure = framesToMeasure_ > 0 &&
> +		       encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&
> +		       ++encounteredFrames_ > skipBeforeMeasure_;
> +	if (measure) {
>  		frameStartTime = {};
>  		clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
>  	}
> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output
>  	dmaSyncers.clear();
>  
>  	/* Measure before emitting signals */
> -	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
> -	    ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
> +	if (measure) {
>  		timespec frameEndTime = {};
>  		clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);
>  		frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);
> -		if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {
> -			const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -
> -							    DebayerCpu::kFramesToSkip;
> +		if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {
>  			LOG(Debayer, Info)
> -				<< "Processed " << measuredFrames
> +				<< "Processed " << framesToMeasure_
>  				<< " frames in " << frameProcessTime_ / 1000 << "us, "
> -				<< frameProcessTime_ / (1000 * measuredFrames)
> +				<< frameProcessTime_ / (1000 * framesToMeasure_)
>  				<< " us/frame";
>  		}
>  	}
> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> index 2f35aa18b..9d343e464 100644
> --- a/src/libcamera/software_isp/debayer_cpu.h
> +++ b/src/libcamera/software_isp/debayer_cpu.h
> @@ -161,11 +161,10 @@ private:
>  	unsigned int xShift_; /* Offset of 0/1 applied to window_.x */
>  	bool enableInputMemcpy_;
>  	bool swapRedBlueGains_;
> -	unsigned int measuredFrames_;
> +	unsigned int encounteredFrames_;
>  	int64_t frameProcessTime_;
> -	/* Skip 30 frames for things to stabilize then measure 30 frames */
> -	static constexpr unsigned int kFramesToSkip = 30;
> -	static constexpr unsigned int kLastFrameToMeasure = 60;
> +	unsigned int skipBeforeMeasure_ = 30;
> +	unsigned int framesToMeasure_ = 30;
>  };
>  
>  } /* namespace libcamera */

Patch
diff mbox series 

diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
index 6610e8bec..702139544 100644
--- a/Documentation/runtime_configuration.rst
+++ b/Documentation/runtime_configuration.rst
@@ -57,6 +57,9 @@  file structure:
           software_isp: # true/false
     software_isp:
       copy_input_buffer: # true/false
+      measure:
+        skip: # non-negative integer, frames to skip initially
+        number: # non-negative integer, frames to measure
 
 Configuration file example
 --------------------------
@@ -92,6 +95,9 @@  Configuration file example
            software_isp: true
      software_isp:
        copy_input_buffer: false
+       measure:
+         skip: 50
+         number: 30
 
 List of variables and configuration options
 -------------------------------------------
@@ -167,6 +173,18 @@  software_isp.copy_input_buffer
 
    Example value: ``false``
 
+software_isp.measure.skip, software_isp.measure.number
+   Define per-frame time measurement parameters in software ISP. `skip`
+   defines how many initial frames are skipped before starting the
+   measurement; `number` defines how many frames then participate in the
+   measurement.
+
+   Set `software_isp.measure.number` to 0 to disable the measurement.
+
+   Example `skip` value: ``50``
+
+   Example `number` value: ``30``
+
 Further details
 ---------------
 
diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
index 2c919f442..f19e15ae2 100644
--- a/src/libcamera/software_isp/TODO
+++ b/src/libcamera/software_isp/TODO
@@ -71,31 +71,6 @@  per-frame buffers like we do for hardware ISPs.
 
 ---
 
-7. Performance measurement configuration
-
-> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)
-> /* Measure before emitting signals */
-> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
->     ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
-> 	timespec frameEndTime = {};
-> 	clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);
-> 	frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);
-> 	if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {
-> 		const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -
-> 						    DebayerCpu::kFramesToSkip;
-> 		LOG(Debayer, Info)
-> 			<< "Processed " << measuredFrames
-> 			<< " frames in " << frameProcessTime_ / 1000 << "us, "
-> 			<< frameProcessTime_ / (1000 * measuredFrames)
-> 			<< " us/frame";
-> 	}
-> }
-
-I wonder if there would be a way to control at runtime when/how to
-perform those measurements. Maybe that's a bit overkill.
-
----
-
 8. DebayerCpu cleanups
 
 > >> class DebayerCpu : public Debayer, public Object
diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index cec6cc6be..2f57857ad 100644
--- a/src/libcamera/software_isp/debayer_cpu.cpp
+++ b/src/libcamera/software_isp/debayer_cpu.cpp
@@ -55,6 +55,13 @@  DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfigurat
 	enableInputMemcpy_ =
 		configuration.option<bool>({ "software_isp", "copy_input_buffer" }).value_or(true);
 
+	skipBeforeMeasure_ = configuration.option<unsigned int>(
+						  { "software_isp", "measure", "skip" })
+				     .value_or(skipBeforeMeasure_);
+	framesToMeasure_ = configuration.option<unsigned int>(
+						{ "software_isp", "measure", "number" })
+				   .value_or(framesToMeasure_);
+
 	/* Initialize color lookup tables */
 	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
 		red_[i] = green_[i] = blue_[i] = i;
@@ -557,7 +564,7 @@  int DebayerCpu::configure(const StreamConfiguration &inputCfg,
 			lineBuffers_[i].resize(lineBufferLength_);
 	}
 
-	measuredFrames_ = 0;
+	encounteredFrames_ = 0;
 	frameProcessTime_ = 0;
 
 	return 0;
@@ -763,7 +770,10 @@  void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output
 {
 	timespec frameStartTime;
 
-	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {
+	bool measure = framesToMeasure_ > 0 &&
+		       encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&
+		       ++encounteredFrames_ > skipBeforeMeasure_;
+	if (measure) {
 		frameStartTime = {};
 		clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
 	}
@@ -820,18 +830,15 @@  void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output
 	dmaSyncers.clear();
 
 	/* Measure before emitting signals */
-	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
-	    ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
+	if (measure) {
 		timespec frameEndTime = {};
 		clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);
 		frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);
-		if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {
-			const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -
-							    DebayerCpu::kFramesToSkip;
+		if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {
 			LOG(Debayer, Info)
-				<< "Processed " << measuredFrames
+				<< "Processed " << framesToMeasure_
 				<< " frames in " << frameProcessTime_ / 1000 << "us, "
-				<< frameProcessTime_ / (1000 * measuredFrames)
+				<< frameProcessTime_ / (1000 * framesToMeasure_)
 				<< " us/frame";
 		}
 	}
diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
index 2f35aa18b..9d343e464 100644
--- a/src/libcamera/software_isp/debayer_cpu.h
+++ b/src/libcamera/software_isp/debayer_cpu.h
@@ -161,11 +161,10 @@  private:
 	unsigned int xShift_; /* Offset of 0/1 applied to window_.x */
 	bool enableInputMemcpy_;
 	bool swapRedBlueGains_;
-	unsigned int measuredFrames_;
+	unsigned int encounteredFrames_;
 	int64_t frameProcessTime_;
-	/* Skip 30 frames for things to stabilize then measure 30 frames */
-	static constexpr unsigned int kFramesToSkip = 30;
-	static constexpr unsigned int kLastFrameToMeasure = 60;
+	unsigned int skipBeforeMeasure_ = 30;
+	unsigned int framesToMeasure_ = 30;
 };
 
 } /* namespace libcamera */