[RFC 3/4] libcamera: software_isp: Move benchmark code to its own class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 16 16:50:22 CEST 2024
On Wed, Oct 16, 2024 at 03:10:06PM +0200, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart at 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 at ideasonboard.com>
>
> Reviewed-by: Milan Zamazal <mzamazal at redhat.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