[{"id":34446,"web_url":"https://patchwork.libcamera.org/comment/34446/","msgid":"<20250611121055.GA31840@pendragon.ideasonboard.com>","date":"2025-06-11T12:10:55","subject":"Re: [PATCH 04/35] libcamera: software_isp: Move benchmark code to\n\tits own class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jun 11, 2025 at 02:32:14AM +0100, Bryan O'Donoghue wrote:\n> From: Hans de Goede \n> \n> Move the code for the builtin benchmark to its own small\n> Benchmark class.\n> \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Milan Zamazal \n> Signed-off-by: Hans de Goede \n> Signed-off-by: Bryan O'Donoghue \n> ---\n> .../internal/software_isp/benchmark.h | 36 +++++++\n> .../internal/software_isp/meson.build | 1 +\n> src/libcamera/software_isp/benchmark.cpp | 93 +++++++++++++++++++\n> src/libcamera/software_isp/debayer_cpu.cpp | 36 +------\n> src/libcamera/software_isp/debayer_cpu.h | 7 +-\n> src/libcamera/software_isp/meson.build | 1 +\n> 6 files changed, 135 insertions(+), 39 deletions(-)\n> create mode 100644 include/libcamera/internal/software_isp/benchmark.h\n> create mode 100644 src/libcamera/software_isp/benchmark.cpp\n> \n> diff --git a/include/libcamera/internal/software_isp/benchmark.h b/include/libcamera/internal/software_isp/benchmark.h\n> new file mode 100644\n> index 00000000..8af25015\n> --- /dev/null\n> +++ b/include/libcamera/internal/software_isp/benchmark.h\n> @@ -0,0 +1,36 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * Authors:\n> + * Hans de Goede \n> + *\n> + * Simple builtin benchmark to measure software ISP processing times\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class Benchmark\n\nThis is a bit generic of a name for something specific to the soft ISP.\nIs it time to move the soft ISP to a namespace ?\n\n> +{\n> +public:\n> +\tBenchmark();\n> +\t~Benchmark();\n> +\n> +\tvoid startFrame(void);\n> +\tvoid finishFrame(void);\n> +\n> +private:\n> +\tunsigned int measuredFrames_;\n> +\tint64_t frameProcessTime_;\n> +\ttimespec frameStartTime_;\n> +\t/* Skip 30 frames for things to stabilize then measure 30 frames */\n> +\tstatic constexpr unsigned int kFramesToSkip = 30;\n> +\tstatic constexpr unsigned int kLastFrameToMeasure = 60;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build\n> index ea3f3f1c..df7c3b97 100644\n> --- a/include/libcamera/internal/software_isp/meson.build\n> +++ b/include/libcamera/internal/software_isp/meson.build\n> @@ -1,6 +1,7 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> libcamera_internal_headers += files([\n> + 'benchmark.h',\n> 'debayer_params.h',\n> 'software_isp.h',\n> 'swisp_stats.h',\n> diff --git a/src/libcamera/software_isp/benchmark.cpp b/src/libcamera/software_isp/benchmark.cpp\n> new file mode 100644\n> index 00000000..b3da3c41\n> --- /dev/null\n> +++ b/src/libcamera/software_isp/benchmark.cpp\n> @@ -0,0 +1,93 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * Authors:\n> + * Hans de Goede \n> + *\n> + * Simple builtin benchmark to measure software ISP processing times\n> + */\n> +\n> +#include \"libcamera/internal/software_isp/benchmark.h\"\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Benchmark)\n> +\n> +/**\n> + * \\class Benchmark\n> + * \\brief Simple builtin benchmark\n> + *\n> + * Simple builtin benchmark to measure software ISP processing times.\n> + */\n> +\n> +/**\n> + * \\brief Constructs a Benchmark object\n> + */\n> +Benchmark::Benchmark()\n> +\t: measuredFrames_(0), frameProcessTime_(0)\n> +{\n> +}\n> +\n> +Benchmark::~Benchmark()\n> +{\n> +}\n> +\n> +static inline int64_t timeDiff(timespec &after, timespec &before)\n> +{\n> +\treturn (after.tv_sec - before.tv_sec) * 1000000000LL +\n> +\t (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> +}\n> +\n> +/**\n> + * \\brief Start measuring process time for a single frame\n> + *\n> + * Call this function before processing frame data to start measuring\n> + * the process time for a frame.\n> + */\n> +void Benchmark::startFrame(void)\n> +{\n> +\tif (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n> +\t\treturn;\n> +\n> +\tframeStartTime_ = {};\n> +\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_);\n> +}\n> +\n> +/**\n> + * \\brief Finish measuring process time for a single frame\n> + *\n> + * Call this function after processing frame data to finish measuring\n> + * the process time for a frame.\n> + *\n> + * This function will log frame processing time information after\n> + * Benchmark::kLastFrameToMeasure frames have been processed.\n> + */\n> +void Benchmark::finishFrame(void)\n> +{\n> +\tif (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n> +\t\treturn;\n> +\n> +\tmeasuredFrames_++;\n> +\n> +\tif (measuredFrames_ <= Benchmark::kFramesToSkip)\n> +\t\treturn;\n> +\n> +\ttimespec frameEndTime = {};\n> +\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> +\tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime_);\n> +\n> +\tif (measuredFrames_ == Benchmark::kLastFrameToMeasure) {\n> +\t\tconst unsigned int measuredFrames = Benchmark::kLastFrameToMeasure -\n> +\t\t\t\t\t\t Benchmark::kFramesToSkip;\n> +\t\tLOG(Benchmark, Info)\n> +\t\t\t<< \"Processed \" << measuredFrames\n> +\t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> +\t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n> +\t\t\t<< \" us/frame\";\n> +\t}\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 66f6038c..8d30bf4a 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -554,9 +554,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> \t\t\tlineBuffers_[i].resize(lineBufferLength_);\n> \t}\n> \n> -\tmeasuredFrames_ = 0;\n> -\tframeProcessTime_ = 0;\n> -\n> \treturn 0;\n> }\n> \n> @@ -746,24 +743,9 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> \t}\n> }\n> \n> -namespace {\n> -\n> -inline int64_t timeDiff(timespec &after, timespec &before)\n> -{\n> -\treturn (after.tv_sec - before.tv_sec) * 1000000000LL +\n> -\t (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> -}\n> -\n> -} /* namespace */\n> -\n> void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n> {\n> -\ttimespec frameStartTime;\n> -\n> -\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n> -\t\tframeStartTime = {};\n> -\t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> -\t}\n> +\tbench_.startFrame();\n> \n> \tstd::vector dmaSyncers;\n> \tfor (const FrameBuffer::Plane &plane : input->planes())\n> @@ -817,21 +799,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> \tdmaSyncers.clear();\n> \n> \t/* Measure before emitting signals */\n> -\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> -\t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> -\t\ttimespec frameEndTime = {};\n> -\t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> -\t\tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n> -\t\tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n> -\t\t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n> -\t\t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n> -\t\t\tLOG(Debayer, Info)\n> -\t\t\t\t<< \"Processed \" << measuredFrames\n> -\t\t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> -\t\t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n> -\t\t\t\t<< \" us/frame\";\n> -\t\t}\n> -\t}\n> +\tbench_.finishFrame();\n> \n> \t/*\n> \t * Buffer ids are currently not used, so pass zeros as its parameter.\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index 89a89893..182607cd 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -17,6 +17,7 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/software_isp/benchmark.h\"\n> #include \"libcamera/internal/bayer_format.h\"\n> #include \"libcamera/internal/software_isp/swstats_cpu.h\"\n> \n> @@ -160,11 +161,7 @@ private:\n> \tunsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> \tbool enableInputMemcpy_;\n> \tbool swapRedBlueGains_;\n> -\tunsigned int measuredFrames_;\n> -\tint64_t frameProcessTime_;\n> -\t/* Skip 30 frames for things to stabilize then measure 30 frames */\n> -\tstatic constexpr unsigned int kFramesToSkip = 30;\n> -\tstatic constexpr unsigned int kLastFrameToMeasure = 60;\n> +\tBenchmark bench_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build\n> index aac7eda7..59fa5f02 100644\n> --- a/src/libcamera/software_isp/meson.build\n> +++ b/src/libcamera/software_isp/meson.build\n> @@ -8,6 +8,7 @@ if not softisp_enabled\n> endif\n> \n> libcamera_internal_sources += files([\n> + 'benchmark.cpp',\n> 'debayer.cpp',\n> 'debayer_cpu.cpp',\n> 'software_isp.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2F48FBDE6B\n\tfor ;\n\tWed, 11 Jun 2025 12:11:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0032E68DBF;\n\tWed, 11 Jun 2025 14:11:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BBC6868DB3\n\tfor ;\n\tWed, 11 Jun 2025 14:11:11 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 451B76BE;\n\tWed, 11 Jun 2025 14:11:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NlIMt6Jw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1749643863;\n\tbh=q3Tx0CSWjqUh1WHyFAhV1t/NELPqryFE5WD3Xu8jtNU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NlIMt6JwXHI2VW6SntLggC9/i6fQbzacdXk7g3d4wfRQYZl77wqNcaienX92D3EBi\n\tN4KuIlxNO36esInTDJLnrems4M8ryC8znbxSDx8pPV8sq4vLAVMuFwKeCVx8VXoiqd\n\t01jaI/rNatUKsen/9irSeBT9Cy/UTrTZYhH3ftZI=","Date":"Wed, 11 Jun 2025 15:10:55 +0300","From":"Laurent Pinchart ","To":"Bryan O'Donoghue ","Cc":"libcamera-devel@lists.libcamera.org, Hans de Goede ,\n\tKieran Bingham ,\n\tMilan Zamazal ","Subject":"Re: [PATCH 04/35] libcamera: software_isp: Move benchmark code to\n\tits own class","Message-ID":"<20250611121055.GA31840@pendragon.ideasonboard.com>","References":"<20250611013245.133785-1-bryan.odonoghue@linaro.org>\n\t<20250611013245.133785-5-bryan.odonoghue@linaro.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250611013245.133785-5-bryan.odonoghue@linaro.org>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]