Message ID | 20250912142915.53949-12-mzamazal@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
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 */
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 */