[RFC 3/4] libcamera: software_isp: Move benchmark code to its own class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 10 21:41:10 CEST 2024
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 at ideasonboard.com>
>
>
> > Signed-off-by: Hans de Goede <hdegoede at 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 at 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 at 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',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list