| Message ID | 20241009200110.275544-4-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Hans de Goede (2024-10-09 21:01:09) > Move the code for the builtin benchmark to its own small > Benchmark class. I think this is a good idea, and I'm not even going to quibble on the implementation detail below, (assuming it compiles cleanly though the CI) as it's currently mostly just moving code out from debayer_cpu. I could envisage this being more generically useful for any measurements we might want to do, but in the event that crops up - that's when things could be generalised. But essentially I think this is a good cleanup to a helper. I could imagine that the instances might need to be named so we can determine 'what' measurement is being reported, but if it's only currently single use per pipeline maybe that isn't essential yet. I could also see a total time being recorded so we could track some sort of utilisation percentage? And finally - I could imagine that it could be helpful to report the benchmark/processing time measured for each frame in metadata, or even a rolling average for metadata... But all of that is feature creep on top so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > src/libcamera/software_isp/benchmark.cpp | 78 ++++++++++++++++++++++ > src/libcamera/software_isp/benchmark.h | 36 ++++++++++ > src/libcamera/software_isp/debayer_cpu.cpp | 32 +-------- > src/libcamera/software_isp/debayer_cpu.h | 7 +- > src/libcamera/software_isp/meson.build | 1 + > 5 files changed, 119 insertions(+), 35 deletions(-) > create mode 100644 src/libcamera/software_isp/benchmark.cpp > create mode 100644 src/libcamera/software_isp/benchmark.h > > diff --git a/src/libcamera/software_isp/benchmark.cpp b/src/libcamera/software_isp/benchmark.cpp > new file mode 100644 > index 00000000..eecad51c > --- /dev/null > +++ b/src/libcamera/software_isp/benchmark.cpp > @@ -0,0 +1,78 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2024, Red Hat Inc. > + * > + * Authors: > + * Hans de Goede <hdegoede@redhat.com> > + * > + * Simple builtin benchmark to measure software ISP processing times > + */ > + > +#include "benchmark.h" > + > +#include <libcamera/base/log.h> > + > +namespace libcamera { > + > +LOG_DEFINE_CATEGORY(Benchmark) > + > +/** > + * \class Benchmark > + * \brief Simple builtin benchmark > + * > + * Simple builtin benchmark to measure software ISP processing times. > + */ > + > +/** > + * \brief Constructs a Benchmark object > + */ > +Benchmark::Benchmark() > + : measuredFrames_(0), frameProcessTime_(0) > +{ > +} > + > +Benchmark::~Benchmark() > +{ > +} > + > +static inline int64_t timeDiff(timespec &after, timespec &before) > +{ > + return (after.tv_sec - before.tv_sec) * 1000000000LL + > + (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; > +} > + > +void Benchmark::startFrame(void) > +{ > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) > + return; > + > + frameStartTime_ = {}; > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_); > +} > + > +void Benchmark::finishFrame(void) > +{ > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) > + return; > + > + measuredFrames_++; > + > + if (measuredFrames_ <= Benchmark::kFramesToSkip) > + return; > + > + timespec frameEndTime = {}; > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); > + frameProcessTime_ += timeDiff(frameEndTime, frameStartTime_); > + > + if (measuredFrames_ == Benchmark::kLastFrameToMeasure) { > + const unsigned int measuredFrames = Benchmark::kLastFrameToMeasure - > + Benchmark::kFramesToSkip; > + LOG(Benchmark, Info) > + << "Processed " << measuredFrames > + << " frames in " << frameProcessTime_ / 1000 << "us, " > + << frameProcessTime_ / (1000 * measuredFrames) > + << " us/frame"; > + } > +} > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/software_isp/benchmark.h b/src/libcamera/software_isp/benchmark.h > new file mode 100644 > index 00000000..8af25015 > --- /dev/null > +++ b/src/libcamera/software_isp/benchmark.h > @@ -0,0 +1,36 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2024, Red Hat Inc. > + * > + * Authors: > + * Hans de Goede <hdegoede@redhat.com> > + * > + * Simple builtin benchmark to measure software ISP processing times > + */ > + > +#pragma once > + > +#include <stdint.h> > +#include <time.h> > + > +namespace libcamera { > + > +class Benchmark > +{ > +public: > + Benchmark(); > + ~Benchmark(); > + > + void startFrame(void); > + void finishFrame(void); > + > +private: > + unsigned int measuredFrames_; > + int64_t frameProcessTime_; > + timespec frameStartTime_; > + /* Skip 30 frames for things to stabilize then measure 30 frames */ > + static constexpr unsigned int kFramesToSkip = 30; > + static constexpr unsigned int kLastFrameToMeasure = 60; > +}; > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index cf5ecdf7..897fb83b 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -528,9 +528,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, > lineBuffers_[i].resize(lineBufferLength_); > } > > - measuredFrames_ = 0; > - frameProcessTime_ = 0; > - > return 0; > } > > @@ -739,22 +736,11 @@ void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags) > } > } > > -inline int64_t timeDiff(timespec &after, timespec &before) > -{ > - return (after.tv_sec - before.tv_sec) * 1000000000LL + > - (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; > -} > - > } /* namespace */ > > void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > { > - timespec frameStartTime; > - > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) { > - frameStartTime = {}; > - clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > - } > + bench_.startFrame(); > > syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); > syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); > @@ -790,21 +776,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output > syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); > > /* 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"; > - } > - } > + bench_.finishFrame(); > > /* > * Buffer ids are currently not used, so pass zeros as its parameter. > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > index 2c47e7c6..59015479 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -19,6 +19,7 @@ > > #include "libcamera/internal/bayer_format.h" > > +#include "benchmark.h" > #include "debayer.h" > #include "swstats_cpu.h" > > @@ -153,11 +154,7 @@ private: > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ > bool enableInputMemcpy_; > bool swapRedBlueGains_; > - unsigned int measuredFrames_; > - 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; > + Benchmark bench_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build > index aac7eda7..59fa5f02 100644 > --- a/src/libcamera/software_isp/meson.build > +++ b/src/libcamera/software_isp/meson.build > @@ -8,6 +8,7 @@ if not softisp_enabled > endif > > libcamera_internal_sources += files([ > + 'benchmark.cpp', > 'debayer.cpp', > 'debayer_cpu.cpp', > 'software_isp.cpp', > -- > 2.46.2 >
On Thu, Oct 10, 2024 at 12:06:00AM +0100, Kieran Bingham wrote: > Quoting Hans de Goede (2024-10-09 21:01:09) > > Move the code for the builtin benchmark to its own small > > Benchmark class. > > I think this is a good idea, and I'm not even going to quibble on the > implementation detail below, (assuming it compiles cleanly though the > CI) as it's currently mostly just moving code out from debayer_cpu. > > I could envisage this being more generically useful for any measurements > we might want to do, but in the event that crops up - that's when things > could be generalised. > > But essentially I think this is a good cleanup to a helper. I think it's fine for now for the soft ISP, but going forward, I think we should consider using the tracing infrastructure again. > I could imagine that the instances might need to be named so we can > determine 'what' measurement is being reported, but if it's only > currently single use per pipeline maybe that isn't essential yet. > > I could also see a total time being recorded so we could track some sort > of utilisation percentage? > > And finally - I could imagine that it could be helpful to report the > benchmark/processing time measured for each frame in metadata, or even a > rolling average for metadata... > > But all of that is feature creep on top so: > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > src/libcamera/software_isp/benchmark.cpp | 78 ++++++++++++++++++++++ > > src/libcamera/software_isp/benchmark.h | 36 ++++++++++ > > src/libcamera/software_isp/debayer_cpu.cpp | 32 +-------- > > src/libcamera/software_isp/debayer_cpu.h | 7 +- > > src/libcamera/software_isp/meson.build | 1 + > > 5 files changed, 119 insertions(+), 35 deletions(-) > > create mode 100644 src/libcamera/software_isp/benchmark.cpp > > create mode 100644 src/libcamera/software_isp/benchmark.h > > > > diff --git a/src/libcamera/software_isp/benchmark.cpp b/src/libcamera/software_isp/benchmark.cpp > > new file mode 100644 > > index 00000000..eecad51c > > --- /dev/null > > +++ b/src/libcamera/software_isp/benchmark.cpp > > @@ -0,0 +1,78 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2024, Red Hat Inc. > > + * > > + * Authors: > > + * Hans de Goede <hdegoede@redhat.com> > > + * > > + * Simple builtin benchmark to measure software ISP processing times > > + */ > > + > > +#include "benchmark.h" > > + > > +#include <libcamera/base/log.h> > > + > > +namespace libcamera { > > + > > +LOG_DEFINE_CATEGORY(Benchmark) > > + > > +/** > > + * \class Benchmark > > + * \brief Simple builtin benchmark > > + * > > + * Simple builtin benchmark to measure software ISP processing times. > > + */ > > + > > +/** > > + * \brief Constructs a Benchmark object > > + */ > > +Benchmark::Benchmark() > > + : measuredFrames_(0), frameProcessTime_(0) > > +{ > > +} > > + > > +Benchmark::~Benchmark() > > +{ > > +} > > + > > +static inline int64_t timeDiff(timespec &after, timespec &before) > > +{ > > + return (after.tv_sec - before.tv_sec) * 1000000000LL + > > + (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; > > +} > > + > > +void Benchmark::startFrame(void) > > +{ > > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) > > + return; > > + > > + frameStartTime_ = {}; > > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_); > > +} > > + > > +void Benchmark::finishFrame(void) > > +{ > > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) > > + return; > > + > > + measuredFrames_++; > > + > > + if (measuredFrames_ <= Benchmark::kFramesToSkip) > > + return; > > + > > + timespec frameEndTime = {}; > > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); > > + frameProcessTime_ += timeDiff(frameEndTime, frameStartTime_); > > + > > + if (measuredFrames_ == Benchmark::kLastFrameToMeasure) { > > + const unsigned int measuredFrames = Benchmark::kLastFrameToMeasure - > > + Benchmark::kFramesToSkip; > > + LOG(Benchmark, Info) > > + << "Processed " << measuredFrames > > + << " frames in " << frameProcessTime_ / 1000 << "us, " > > + << frameProcessTime_ / (1000 * measuredFrames) > > + << " us/frame"; > > + } > > +} > > + > > +} /* namespace libcamera */ > > diff --git a/src/libcamera/software_isp/benchmark.h b/src/libcamera/software_isp/benchmark.h > > new file mode 100644 > > index 00000000..8af25015 > > --- /dev/null > > +++ b/src/libcamera/software_isp/benchmark.h > > @@ -0,0 +1,36 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2024, Red Hat Inc. > > + * > > + * Authors: > > + * Hans de Goede <hdegoede@redhat.com> > > + * > > + * Simple builtin benchmark to measure software ISP processing times > > + */ > > + > > +#pragma once > > + > > +#include <stdint.h> > > +#include <time.h> > > + > > +namespace libcamera { > > + > > +class Benchmark > > +{ > > +public: > > + Benchmark(); > > + ~Benchmark(); > > + > > + void startFrame(void); > > + void finishFrame(void); > > + > > +private: > > + unsigned int measuredFrames_; > > + int64_t frameProcessTime_; > > + timespec frameStartTime_; > > + /* Skip 30 frames for things to stabilize then measure 30 frames */ > > + static constexpr unsigned int kFramesToSkip = 30; > > + static constexpr unsigned int kLastFrameToMeasure = 60; > > +}; > > + > > +} /* namespace libcamera */ > > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > > index cf5ecdf7..897fb83b 100644 > > --- a/src/libcamera/software_isp/debayer_cpu.cpp > > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > > @@ -528,9 +528,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, > > lineBuffers_[i].resize(lineBufferLength_); > > } > > > > - measuredFrames_ = 0; > > - frameProcessTime_ = 0; > > - > > return 0; > > } > > > > @@ -739,22 +736,11 @@ void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags) > > } > > } > > > > -inline int64_t timeDiff(timespec &after, timespec &before) > > -{ > > - return (after.tv_sec - before.tv_sec) * 1000000000LL + > > - (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; > > -} > > - > > } /* namespace */ > > > > void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > > { > > - timespec frameStartTime; > > - > > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) { > > - frameStartTime = {}; > > - clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > > - } > > + bench_.startFrame(); > > > > syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); > > syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); > > @@ -790,21 +776,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output > > syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); > > > > /* 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"; > > - } > > - } > > + bench_.finishFrame(); > > > > /* > > * Buffer ids are currently not used, so pass zeros as its parameter. > > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > > index 2c47e7c6..59015479 100644 > > --- a/src/libcamera/software_isp/debayer_cpu.h > > +++ b/src/libcamera/software_isp/debayer_cpu.h > > @@ -19,6 +19,7 @@ > > > > #include "libcamera/internal/bayer_format.h" > > > > +#include "benchmark.h" > > #include "debayer.h" > > #include "swstats_cpu.h" > > > > @@ -153,11 +154,7 @@ private: > > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ > > bool enableInputMemcpy_; > > bool swapRedBlueGains_; > > - unsigned int measuredFrames_; > > - 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; > > + Benchmark bench_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build > > index aac7eda7..59fa5f02 100644 > > --- a/src/libcamera/software_isp/meson.build > > +++ b/src/libcamera/software_isp/meson.build > > @@ -8,6 +8,7 @@ if not softisp_enabled > > endif > > > > libcamera_internal_sources += files([ > > + 'benchmark.cpp', > > 'debayer.cpp', > > 'debayer_cpu.cpp', > > 'software_isp.cpp',
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Thu, Oct 10, 2024 at 12:06:00AM +0100, Kieran Bingham wrote: >> Quoting Hans de Goede (2024-10-09 21:01:09) >> > Move the code for the builtin benchmark to its own small > >> > Benchmark class. >> >> I think this is a good idea, and I'm not even going to quibble on the >> implementation detail below, (assuming it compiles cleanly though the >> CI) as it's currently mostly just moving code out from debayer_cpu. >> >> I could envisage this being more generically useful for any measurements >> we might want to do, but in the event that crops up - that's when things >> could be generalised. >> >> But essentially I think this is a good cleanup to a helper. > > I think it's fine for now for the soft ISP, but going forward, I think > we should consider using the tracing infrastructure again. Maybe. But the current helper makes it super-easy to see contingent performance changes. >> I could imagine that the instances might need to be named so we can >> determine 'what' measurement is being reported, but if it's only >> currently single use per pipeline maybe that isn't essential yet. >> >> I could also see a total time being recorded so we could track some sort >> of utilisation percentage? >> >> And finally - I could imagine that it could be helpful to report the >> benchmark/processing time measured for each frame in metadata, or even a >> rolling average for metadata... >> >> But all of that is feature creep on top so: >> >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> >> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> > --- >> > src/libcamera/software_isp/benchmark.cpp | 78 ++++++++++++++++++++++ >> > src/libcamera/software_isp/benchmark.h | 36 ++++++++++ >> > src/libcamera/software_isp/debayer_cpu.cpp | 32 +-------- >> > src/libcamera/software_isp/debayer_cpu.h | 7 +- >> > src/libcamera/software_isp/meson.build | 1 + >> > 5 files changed, 119 insertions(+), 35 deletions(-) >> > create mode 100644 src/libcamera/software_isp/benchmark.cpp >> > create mode 100644 src/libcamera/software_isp/benchmark.h >> > >> > diff --git a/src/libcamera/software_isp/benchmark.cpp b/src/libcamera/software_isp/benchmark.cpp >> > new file mode 100644 >> > index 00000000..eecad51c >> > --- /dev/null >> > +++ b/src/libcamera/software_isp/benchmark.cpp >> > @@ -0,0 +1,78 @@ >> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> > +/* >> > + * Copyright (C) 2024, Red Hat Inc. >> > + * >> > + * Authors: >> > + * Hans de Goede <hdegoede@redhat.com> >> > + * >> > + * Simple builtin benchmark to measure software ISP processing times >> > + */ >> > + >> > +#include "benchmark.h" >> > + >> > +#include <libcamera/base/log.h> >> > + >> > +namespace libcamera { >> > + >> > +LOG_DEFINE_CATEGORY(Benchmark) >> > + >> > +/** >> > + * \class Benchmark >> > + * \brief Simple builtin benchmark >> > + * >> > + * Simple builtin benchmark to measure software ISP processing times. >> > + */ >> > + >> > +/** >> > + * \brief Constructs a Benchmark object >> > + */ >> > +Benchmark::Benchmark() >> > + : measuredFrames_(0), frameProcessTime_(0) >> > +{ >> > +} >> > + >> > +Benchmark::~Benchmark() >> > +{ >> > +} >> > + >> > +static inline int64_t timeDiff(timespec &after, timespec &before) >> > +{ >> > + return (after.tv_sec - before.tv_sec) * 1000000000LL + >> > + (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; >> > +} >> > + >> > +void Benchmark::startFrame(void) >> > +{ >> > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) >> > + return; >> > + >> > + frameStartTime_ = {}; >> > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_); >> > +} >> > + >> > +void Benchmark::finishFrame(void) >> > +{ >> > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) >> > + return; >> > + >> > + measuredFrames_++; >> > + >> > + if (measuredFrames_ <= Benchmark::kFramesToSkip) >> > + return; >> > + >> > + timespec frameEndTime = {}; >> > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); >> > + frameProcessTime_ += timeDiff(frameEndTime, frameStartTime_); >> > + >> > + if (measuredFrames_ == Benchmark::kLastFrameToMeasure) { >> > + const unsigned int measuredFrames = Benchmark::kLastFrameToMeasure - >> > + Benchmark::kFramesToSkip; >> > + LOG(Benchmark, Info) >> > + << "Processed " << measuredFrames >> > + << " frames in " << frameProcessTime_ / 1000 << "us, " >> > + << frameProcessTime_ / (1000 * measuredFrames) >> > + << " us/frame"; >> > + } >> > +} >> > + >> > +} /* namespace libcamera */ >> > diff --git a/src/libcamera/software_isp/benchmark.h b/src/libcamera/software_isp/benchmark.h >> > new file mode 100644 >> > index 00000000..8af25015 >> > --- /dev/null >> > +++ b/src/libcamera/software_isp/benchmark.h >> > @@ -0,0 +1,36 @@ >> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> > +/* >> > + * Copyright (C) 2024, Red Hat Inc. >> > + * >> > + * Authors: >> > + * Hans de Goede <hdegoede@redhat.com> >> > + * >> > + * Simple builtin benchmark to measure software ISP processing times >> > + */ >> > + >> > +#pragma once >> > + >> > +#include <stdint.h> >> > +#include <time.h> >> > + >> > +namespace libcamera { >> > + >> > +class Benchmark >> > +{ >> > +public: >> > + Benchmark(); >> > + ~Benchmark(); >> > + >> > + void startFrame(void); >> > + void finishFrame(void); >> > + >> > +private: >> > + unsigned int measuredFrames_; >> > + int64_t frameProcessTime_; >> > + timespec frameStartTime_; >> > + /* Skip 30 frames for things to stabilize then measure 30 frames */ >> > + static constexpr unsigned int kFramesToSkip = 30; >> > + static constexpr unsigned int kLastFrameToMeasure = 60; >> > +}; >> > + >> > +} /* namespace libcamera */ >> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >> > index cf5ecdf7..897fb83b 100644 >> > --- a/src/libcamera/software_isp/debayer_cpu.cpp >> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> > @@ -528,9 +528,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, >> > lineBuffers_[i].resize(lineBufferLength_); >> > } >> > >> > - measuredFrames_ = 0; >> > - frameProcessTime_ = 0; >> > - >> > return 0; >> > } >> > >> > @@ -739,22 +736,11 @@ void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags) >> > } >> > } >> > >> > -inline int64_t timeDiff(timespec &after, timespec &before) >> > -{ >> > - return (after.tv_sec - before.tv_sec) * 1000000000LL + >> > - (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; >> > -} >> > - >> > } /* namespace */ >> > >> > void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) >> > { >> > - timespec frameStartTime; >> > - >> > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) { >> > - frameStartTime = {}; >> > - clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); >> > - } >> > + bench_.startFrame(); >> > >> > syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); >> > syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); >> > @@ -790,21 +776,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >> > syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); >> > >> > /* 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"; >> > - } >> > - } >> > + bench_.finishFrame(); >> > >> > /* >> > * Buffer ids are currently not used, so pass zeros as its parameter. >> > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h >> > index 2c47e7c6..59015479 100644 >> > --- a/src/libcamera/software_isp/debayer_cpu.h >> > +++ b/src/libcamera/software_isp/debayer_cpu.h >> > @@ -19,6 +19,7 @@ >> > >> > #include "libcamera/internal/bayer_format.h" >> > >> > +#include "benchmark.h" >> > #include "debayer.h" >> > #include "swstats_cpu.h" >> > >> > @@ -153,11 +154,7 @@ private: >> > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ >> > bool enableInputMemcpy_; >> > bool swapRedBlueGains_; >> > - unsigned int measuredFrames_; >> > - 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; >> > + Benchmark bench_; >> > }; >> > >> > } /* namespace libcamera */ >> > diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build >> > index aac7eda7..59fa5f02 100644 >> > --- a/src/libcamera/software_isp/meson.build >> > +++ b/src/libcamera/software_isp/meson.build >> > @@ -8,6 +8,7 @@ if not softisp_enabled >> > endif >> > >> > libcamera_internal_sources += files([ >> > + 'benchmark.cpp', >> > 'debayer.cpp', >> > 'debayer_cpu.cpp', >> > 'software_isp.cpp',
On Wed, Oct 16, 2024 at 03:10:06PM +0200, Milan Zamazal wrote: > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > > On Thu, Oct 10, 2024 at 12:06:00AM +0100, Kieran Bingham wrote: > >> Quoting Hans de Goede (2024-10-09 21:01:09) > >> > Move the code for the builtin benchmark to its own small > >> > Benchmark class. > >> > >> I think this is a good idea, and I'm not even going to quibble on the > >> implementation detail below, (assuming it compiles cleanly though the > >> CI) as it's currently mostly just moving code out from debayer_cpu. > >> > >> I could envisage this being more generically useful for any measurements > >> we might want to do, but in the event that crops up - that's when things > >> could be generalised. > >> > >> But essentially I think this is a good cleanup to a helper. > > > > I think it's fine for now for the soft ISP, but going forward, I think > > we should consider using the tracing infrastructure again. > > Maybe. But the current helper makes it super-easy to see contingent > performance changes. True, but that can be said of pretty much any tracing-related needs, it's easier to implement a ad-hoc solution compared to overcoming the tracing barrier. It doesn't scale though, so at some point we have to bite the bullet, spend the time required to use tracing (and likely implement some trace analysis scripts, or document specific tracing use cases), and benefit from that in the long term. > >> I could imagine that the instances might need to be named so we can > >> determine 'what' measurement is being reported, but if it's only > >> currently single use per pipeline maybe that isn't essential yet. > >> > >> I could also see a total time being recorded so we could track some sort > >> of utilisation percentage? > >> > >> And finally - I could imagine that it could be helpful to report the > >> benchmark/processing time measured for each frame in metadata, or even a > >> rolling average for metadata... > >> > >> But all of that is feature creep on top so: > >> > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > > >> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> > --- > >> > src/libcamera/software_isp/benchmark.cpp | 78 ++++++++++++++++++++++ > >> > src/libcamera/software_isp/benchmark.h | 36 ++++++++++ > >> > src/libcamera/software_isp/debayer_cpu.cpp | 32 +-------- > >> > src/libcamera/software_isp/debayer_cpu.h | 7 +- > >> > src/libcamera/software_isp/meson.build | 1 + > >> > 5 files changed, 119 insertions(+), 35 deletions(-) > >> > create mode 100644 src/libcamera/software_isp/benchmark.cpp > >> > create mode 100644 src/libcamera/software_isp/benchmark.h > >> > > >> > diff --git a/src/libcamera/software_isp/benchmark.cpp b/src/libcamera/software_isp/benchmark.cpp > >> > new file mode 100644 > >> > index 00000000..eecad51c > >> > --- /dev/null > >> > +++ b/src/libcamera/software_isp/benchmark.cpp > >> > @@ -0,0 +1,78 @@ > >> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >> > +/* > >> > + * Copyright (C) 2024, Red Hat Inc. > >> > + * > >> > + * Authors: > >> > + * Hans de Goede <hdegoede@redhat.com> > >> > + * > >> > + * Simple builtin benchmark to measure software ISP processing times > >> > + */ > >> > + > >> > +#include "benchmark.h" > >> > + > >> > +#include <libcamera/base/log.h> > >> > + > >> > +namespace libcamera { > >> > + > >> > +LOG_DEFINE_CATEGORY(Benchmark) > >> > + > >> > +/** > >> > + * \class Benchmark > >> > + * \brief Simple builtin benchmark > >> > + * > >> > + * Simple builtin benchmark to measure software ISP processing times. > >> > + */ > >> > + > >> > +/** > >> > + * \brief Constructs a Benchmark object > >> > + */ > >> > +Benchmark::Benchmark() > >> > + : measuredFrames_(0), frameProcessTime_(0) > >> > +{ > >> > +} > >> > + > >> > +Benchmark::~Benchmark() > >> > +{ > >> > +} > >> > + > >> > +static inline int64_t timeDiff(timespec &after, timespec &before) > >> > +{ > >> > + return (after.tv_sec - before.tv_sec) * 1000000000LL + > >> > + (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; > >> > +} > >> > + > >> > +void Benchmark::startFrame(void) > >> > +{ > >> > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) > >> > + return; > >> > + > >> > + frameStartTime_ = {}; > >> > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_); > >> > +} > >> > + > >> > +void Benchmark::finishFrame(void) > >> > +{ > >> > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) > >> > + return; > >> > + > >> > + measuredFrames_++; > >> > + > >> > + if (measuredFrames_ <= Benchmark::kFramesToSkip) > >> > + return; > >> > + > >> > + timespec frameEndTime = {}; > >> > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); > >> > + frameProcessTime_ += timeDiff(frameEndTime, frameStartTime_); > >> > + > >> > + if (measuredFrames_ == Benchmark::kLastFrameToMeasure) { > >> > + const unsigned int measuredFrames = Benchmark::kLastFrameToMeasure - > >> > + Benchmark::kFramesToSkip; > >> > + LOG(Benchmark, Info) > >> > + << "Processed " << measuredFrames > >> > + << " frames in " << frameProcessTime_ / 1000 << "us, " > >> > + << frameProcessTime_ / (1000 * measuredFrames) > >> > + << " us/frame"; > >> > + } > >> > +} > >> > + > >> > +} /* namespace libcamera */ > >> > diff --git a/src/libcamera/software_isp/benchmark.h b/src/libcamera/software_isp/benchmark.h > >> > new file mode 100644 > >> > index 00000000..8af25015 > >> > --- /dev/null > >> > +++ b/src/libcamera/software_isp/benchmark.h > >> > @@ -0,0 +1,36 @@ > >> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >> > +/* > >> > + * Copyright (C) 2024, Red Hat Inc. > >> > + * > >> > + * Authors: > >> > + * Hans de Goede <hdegoede@redhat.com> > >> > + * > >> > + * Simple builtin benchmark to measure software ISP processing times > >> > + */ > >> > + > >> > +#pragma once > >> > + > >> > +#include <stdint.h> > >> > +#include <time.h> > >> > + > >> > +namespace libcamera { > >> > + > >> > +class Benchmark > >> > +{ > >> > +public: > >> > + Benchmark(); > >> > + ~Benchmark(); > >> > + > >> > + void startFrame(void); > >> > + void finishFrame(void); > >> > + > >> > +private: > >> > + unsigned int measuredFrames_; > >> > + int64_t frameProcessTime_; > >> > + timespec frameStartTime_; > >> > + /* Skip 30 frames for things to stabilize then measure 30 frames */ > >> > + static constexpr unsigned int kFramesToSkip = 30; > >> > + static constexpr unsigned int kLastFrameToMeasure = 60; > >> > +}; > >> > + > >> > +} /* namespace libcamera */ > >> > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > >> > index cf5ecdf7..897fb83b 100644 > >> > --- a/src/libcamera/software_isp/debayer_cpu.cpp > >> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > >> > @@ -528,9 +528,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, > >> > lineBuffers_[i].resize(lineBufferLength_); > >> > } > >> > > >> > - measuredFrames_ = 0; > >> > - frameProcessTime_ = 0; > >> > - > >> > return 0; > >> > } > >> > > >> > @@ -739,22 +736,11 @@ void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags) > >> > } > >> > } > >> > > >> > -inline int64_t timeDiff(timespec &after, timespec &before) > >> > -{ > >> > - return (after.tv_sec - before.tv_sec) * 1000000000LL + > >> > - (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; > >> > -} > >> > - > >> > } /* namespace */ > >> > > >> > void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > >> > { > >> > - timespec frameStartTime; > >> > - > >> > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) { > >> > - frameStartTime = {}; > >> > - clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > >> > - } > >> > + bench_.startFrame(); > >> > > >> > syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); > >> > syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); > >> > @@ -790,21 +776,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output > >> > syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); > >> > > >> > /* 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"; > >> > - } > >> > - } > >> > + bench_.finishFrame(); > >> > > >> > /* > >> > * Buffer ids are currently not used, so pass zeros as its parameter. > >> > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > >> > index 2c47e7c6..59015479 100644 > >> > --- a/src/libcamera/software_isp/debayer_cpu.h > >> > +++ b/src/libcamera/software_isp/debayer_cpu.h > >> > @@ -19,6 +19,7 @@ > >> > > >> > #include "libcamera/internal/bayer_format.h" > >> > > >> > +#include "benchmark.h" > >> > #include "debayer.h" > >> > #include "swstats_cpu.h" > >> > > >> > @@ -153,11 +154,7 @@ private: > >> > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ > >> > bool enableInputMemcpy_; > >> > bool swapRedBlueGains_; > >> > - unsigned int measuredFrames_; > >> > - 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; > >> > + Benchmark bench_; > >> > }; > >> > > >> > } /* namespace libcamera */ > >> > diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build > >> > index aac7eda7..59fa5f02 100644 > >> > --- a/src/libcamera/software_isp/meson.build > >> > +++ b/src/libcamera/software_isp/meson.build > >> > @@ -8,6 +8,7 @@ if not softisp_enabled > >> > endif > >> > > >> > libcamera_internal_sources += files([ > >> > + 'benchmark.cpp', > >> > 'debayer.cpp', > >> > 'debayer_cpu.cpp', > >> > 'software_isp.cpp',
Quoting Hans de Goede (2024-10-09 21:01:09) > Move the code for the builtin benchmark to its own small > Benchmark class. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > src/libcamera/software_isp/benchmark.cpp | 78 ++++++++++++++++++++++ > src/libcamera/software_isp/benchmark.h | 36 ++++++++++ > src/libcamera/software_isp/debayer_cpu.cpp | 32 +-------- > src/libcamera/software_isp/debayer_cpu.h | 7 +- > src/libcamera/software_isp/meson.build | 1 + > 5 files changed, 119 insertions(+), 35 deletions(-) > create mode 100644 src/libcamera/software_isp/benchmark.cpp > create mode 100644 src/libcamera/software_isp/benchmark.h > > diff --git a/src/libcamera/software_isp/benchmark.cpp b/src/libcamera/software_isp/benchmark.cpp > new file mode 100644 > index 00000000..eecad51c > --- /dev/null > +++ b/src/libcamera/software_isp/benchmark.cpp > @@ -0,0 +1,78 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2024, Red Hat Inc. > + * > + * Authors: > + * Hans de Goede <hdegoede@redhat.com> > + * > + * Simple builtin benchmark to measure software ISP processing times > + */ > + > +#include "benchmark.h" > + > +#include <libcamera/base/log.h> > + > +namespace libcamera { > + > +LOG_DEFINE_CATEGORY(Benchmark) > + > +/** > + * \class Benchmark > + * \brief Simple builtin benchmark > + * > + * Simple builtin benchmark to measure software ISP processing times. > + */ > + > +/** > + * \brief Constructs a Benchmark object > + */ > +Benchmark::Benchmark() > + : measuredFrames_(0), frameProcessTime_(0) > +{ > +} > + > +Benchmark::~Benchmark() > +{ > +} > + > +static inline int64_t timeDiff(timespec &after, timespec &before) > +{ > + return (after.tv_sec - before.tv_sec) * 1000000000LL + > + (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; > +} > + > +void Benchmark::startFrame(void) I was /so/ close to merging the first 3 patches of this series (4/4 isn't "used" so I don't think it's so easy to just merge) - but doxygen kicks out at the undocumented class entries: https://gitlab.freedesktop.org/camera/libcamera/-/jobs/65324937 -- Kieran > +{ > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) > + return; > + > + frameStartTime_ = {}; > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_); > +} > + > +void Benchmark::finishFrame(void) > +{ > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) > + return; > + > + measuredFrames_++; > + > + if (measuredFrames_ <= Benchmark::kFramesToSkip) > + return; > + > + timespec frameEndTime = {}; > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); > + frameProcessTime_ += timeDiff(frameEndTime, frameStartTime_); > + > + if (measuredFrames_ == Benchmark::kLastFrameToMeasure) { > + const unsigned int measuredFrames = Benchmark::kLastFrameToMeasure - > + Benchmark::kFramesToSkip; > + LOG(Benchmark, Info) > + << "Processed " << measuredFrames > + << " frames in " << frameProcessTime_ / 1000 << "us, " > + << frameProcessTime_ / (1000 * measuredFrames) > + << " us/frame"; > + } > +} > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/software_isp/benchmark.h b/src/libcamera/software_isp/benchmark.h > new file mode 100644 > index 00000000..8af25015 > --- /dev/null > +++ b/src/libcamera/software_isp/benchmark.h > @@ -0,0 +1,36 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2024, Red Hat Inc. > + * > + * Authors: > + * Hans de Goede <hdegoede@redhat.com> > + * > + * Simple builtin benchmark to measure software ISP processing times > + */ > + > +#pragma once > + > +#include <stdint.h> > +#include <time.h> > + > +namespace libcamera { > + > +class Benchmark > +{ > +public: > + Benchmark(); > + ~Benchmark(); > + > + void startFrame(void); > + void finishFrame(void); > + > +private: > + unsigned int measuredFrames_; > + int64_t frameProcessTime_; > + timespec frameStartTime_; > + /* Skip 30 frames for things to stabilize then measure 30 frames */ > + static constexpr unsigned int kFramesToSkip = 30; > + static constexpr unsigned int kLastFrameToMeasure = 60; > +}; > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index cf5ecdf7..897fb83b 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -528,9 +528,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, > lineBuffers_[i].resize(lineBufferLength_); > } > > - measuredFrames_ = 0; > - frameProcessTime_ = 0; > - > return 0; > } > > @@ -739,22 +736,11 @@ void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags) > } > } > > -inline int64_t timeDiff(timespec &after, timespec &before) > -{ > - return (after.tv_sec - before.tv_sec) * 1000000000LL + > - (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; > -} > - > } /* namespace */ > > void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > { > - timespec frameStartTime; > - > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) { > - frameStartTime = {}; > - clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > - } > + bench_.startFrame(); > > syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); > syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); > @@ -790,21 +776,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output > syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); > > /* 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"; > - } > - } > + bench_.finishFrame(); > > /* > * Buffer ids are currently not used, so pass zeros as its parameter. > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > index 2c47e7c6..59015479 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -19,6 +19,7 @@ > > #include "libcamera/internal/bayer_format.h" > > +#include "benchmark.h" > #include "debayer.h" > #include "swstats_cpu.h" > > @@ -153,11 +154,7 @@ private: > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ > bool enableInputMemcpy_; > bool swapRedBlueGains_; > - unsigned int measuredFrames_; > - 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; > + Benchmark bench_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build > index aac7eda7..59fa5f02 100644 > --- a/src/libcamera/software_isp/meson.build > +++ b/src/libcamera/software_isp/meson.build > @@ -8,6 +8,7 @@ if not softisp_enabled > endif > > libcamera_internal_sources += files([ > + 'benchmark.cpp', > 'debayer.cpp', > 'debayer_cpu.cpp', > 'software_isp.cpp', > -- > 2.46.2 >
diff --git a/src/libcamera/software_isp/benchmark.cpp b/src/libcamera/software_isp/benchmark.cpp new file mode 100644 index 00000000..eecad51c --- /dev/null +++ b/src/libcamera/software_isp/benchmark.cpp @@ -0,0 +1,78 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2024, Red Hat Inc. + * + * Authors: + * Hans de Goede <hdegoede@redhat.com> + * + * Simple builtin benchmark to measure software ISP processing times + */ + +#include "benchmark.h" + +#include <libcamera/base/log.h> + +namespace libcamera { + +LOG_DEFINE_CATEGORY(Benchmark) + +/** + * \class Benchmark + * \brief Simple builtin benchmark + * + * Simple builtin benchmark to measure software ISP processing times. + */ + +/** + * \brief Constructs a Benchmark object + */ +Benchmark::Benchmark() + : measuredFrames_(0), frameProcessTime_(0) +{ +} + +Benchmark::~Benchmark() +{ +} + +static inline int64_t timeDiff(timespec &after, timespec &before) +{ + return (after.tv_sec - before.tv_sec) * 1000000000LL + + (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; +} + +void Benchmark::startFrame(void) +{ + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) + return; + + frameStartTime_ = {}; + clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_); +} + +void Benchmark::finishFrame(void) +{ + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure) + return; + + measuredFrames_++; + + if (measuredFrames_ <= Benchmark::kFramesToSkip) + return; + + timespec frameEndTime = {}; + clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime); + frameProcessTime_ += timeDiff(frameEndTime, frameStartTime_); + + if (measuredFrames_ == Benchmark::kLastFrameToMeasure) { + const unsigned int measuredFrames = Benchmark::kLastFrameToMeasure - + Benchmark::kFramesToSkip; + LOG(Benchmark, Info) + << "Processed " << measuredFrames + << " frames in " << frameProcessTime_ / 1000 << "us, " + << frameProcessTime_ / (1000 * measuredFrames) + << " us/frame"; + } +} + +} /* namespace libcamera */ diff --git a/src/libcamera/software_isp/benchmark.h b/src/libcamera/software_isp/benchmark.h new file mode 100644 index 00000000..8af25015 --- /dev/null +++ b/src/libcamera/software_isp/benchmark.h @@ -0,0 +1,36 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2024, Red Hat Inc. + * + * Authors: + * Hans de Goede <hdegoede@redhat.com> + * + * Simple builtin benchmark to measure software ISP processing times + */ + +#pragma once + +#include <stdint.h> +#include <time.h> + +namespace libcamera { + +class Benchmark +{ +public: + Benchmark(); + ~Benchmark(); + + void startFrame(void); + void finishFrame(void); + +private: + unsigned int measuredFrames_; + int64_t frameProcessTime_; + timespec frameStartTime_; + /* Skip 30 frames for things to stabilize then measure 30 frames */ + static constexpr unsigned int kFramesToSkip = 30; + static constexpr unsigned int kLastFrameToMeasure = 60; +}; + +} /* namespace libcamera */ diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index cf5ecdf7..897fb83b 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -528,9 +528,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg, lineBuffers_[i].resize(lineBufferLength_); } - measuredFrames_ = 0; - frameProcessTime_ = 0; - return 0; } @@ -739,22 +736,11 @@ void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags) } } -inline int64_t timeDiff(timespec &after, timespec &before) -{ - return (after.tv_sec - before.tv_sec) * 1000000000LL + - (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; -} - } /* namespace */ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) { - timespec frameStartTime; - - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) { - frameStartTime = {}; - clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); - } + bench_.startFrame(); syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ); syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE); @@ -790,21 +776,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ); /* 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"; - } - } + bench_.finishFrame(); /* * Buffer ids are currently not used, so pass zeros as its parameter. diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h index 2c47e7c6..59015479 100644 --- a/src/libcamera/software_isp/debayer_cpu.h +++ b/src/libcamera/software_isp/debayer_cpu.h @@ -19,6 +19,7 @@ #include "libcamera/internal/bayer_format.h" +#include "benchmark.h" #include "debayer.h" #include "swstats_cpu.h" @@ -153,11 +154,7 @@ private: unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ bool enableInputMemcpy_; bool swapRedBlueGains_; - unsigned int measuredFrames_; - 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; + Benchmark bench_; }; } /* namespace libcamera */ diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build index aac7eda7..59fa5f02 100644 --- a/src/libcamera/software_isp/meson.build +++ b/src/libcamera/software_isp/meson.build @@ -8,6 +8,7 @@ if not softisp_enabled endif libcamera_internal_sources += files([ + 'benchmark.cpp', 'debayer.cpp', 'debayer_cpu.cpp', 'software_isp.cpp',
Move the code for the builtin benchmark to its own small Benchmark class. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- src/libcamera/software_isp/benchmark.cpp | 78 ++++++++++++++++++++++ src/libcamera/software_isp/benchmark.h | 36 ++++++++++ src/libcamera/software_isp/debayer_cpu.cpp | 32 +-------- src/libcamera/software_isp/debayer_cpu.h | 7 +- src/libcamera/software_isp/meson.build | 1 + 5 files changed, 119 insertions(+), 35 deletions(-) create mode 100644 src/libcamera/software_isp/benchmark.cpp create mode 100644 src/libcamera/software_isp/benchmark.h