[RFC 3/4] libcamera: software_isp: Move benchmark code to its own class
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Oct 10 01:06:00 CEST 2024
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 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',
> --
> 2.46.2
>
More information about the libcamera-devel
mailing list