[RFC 3/4] libcamera: software_isp: Move benchmark code to its own class
Milan Zamazal
mzamazal at redhat.com
Wed Oct 16 15:10:06 CEST 2024
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.
>> 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',
More information about the libcamera-devel
mailing list