[{"id":31679,"web_url":"https://patchwork.libcamera.org/comment/31679/","msgid":"<172851516064.532453.14529043744196704862@ping.linuxembedded.co.uk>","date":"2024-10-09T23:06:00","subject":"Re: [RFC 3/4] libcamera: software_isp: Move benchmark code to its\n\town class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2024-10-09 21:01:09)\n> Move the code for the builtin benchmark to its own small\n> Benchmark class.\n\nI think this is a good idea, and I'm not even going to quibble on the\nimplementation detail below, (assuming it compiles cleanly though the\nCI) as it's currently mostly just moving code out from debayer_cpu.\n\nI could envisage this being more generically useful for any measurements\nwe might want to do, but in the event that crops up - that's when things\ncould be generalised.\n\nBut essentially I think this is a good cleanup to a helper.\n\nI could imagine that the instances might need to be named so we can\ndetermine 'what' measurement is being reported, but if it's only\ncurrently single use per pipeline maybe that isn't essential yet.\n\nI could also see a total time being recorded so we could track some sort\nof utilisation percentage?\n\nAnd finally - I could imagine that it could be helpful to report the\nbenchmark/processing time measured for each frame in metadata, or even a\nrolling average for metadata...\n\nBut all of that is feature creep on top so:\n\n\nReviewed-by: Kieran Bingham \n\n\n> Signed-off-by: Hans de Goede \n> ---\n> src/libcamera/software_isp/benchmark.cpp | 78 ++++++++++++++++++++++\n> src/libcamera/software_isp/benchmark.h | 36 ++++++++++\n> src/libcamera/software_isp/debayer_cpu.cpp | 32 +--------\n> src/libcamera/software_isp/debayer_cpu.h | 7 +-\n> src/libcamera/software_isp/meson.build | 1 +\n> 5 files changed, 119 insertions(+), 35 deletions(-)\n> create mode 100644 src/libcamera/software_isp/benchmark.cpp\n> create mode 100644 src/libcamera/software_isp/benchmark.h\n> \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..eecad51c\n> --- /dev/null\n> +++ b/src/libcamera/software_isp/benchmark.cpp\n> @@ -0,0 +1,78 @@\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 \"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> + : 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> + return (after.tv_sec - before.tv_sec) * 1000000000LL +\n> + (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> +}\n> +\n> +void Benchmark::startFrame(void)\n> +{\n> + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n> + return;\n> +\n> + frameStartTime_ = {};\n> + clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_);\n> +}\n> +\n> +void Benchmark::finishFrame(void)\n> +{\n> + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n> + return;\n> +\n> + measuredFrames_++;\n> +\n> + if (measuredFrames_ <= Benchmark::kFramesToSkip)\n> + return;\n> +\n> + timespec frameEndTime = {};\n> + clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> + frameProcessTime_ += timeDiff(frameEndTime, frameStartTime_);\n> +\n> + if (measuredFrames_ == Benchmark::kLastFrameToMeasure) {\n> + const unsigned int measuredFrames = Benchmark::kLastFrameToMeasure -\n> + Benchmark::kFramesToSkip;\n> + LOG(Benchmark, Info)\n> + << \"Processed \" << measuredFrames\n> + << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> + << frameProcessTime_ / (1000 * measuredFrames)\n> + << \" us/frame\";\n> + }\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/software_isp/benchmark.h b/src/libcamera/software_isp/benchmark.h\n> new file mode 100644\n> index 00000000..8af25015\n> --- /dev/null\n> +++ b/src/libcamera/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> +{\n> +public:\n> + Benchmark();\n> + ~Benchmark();\n> +\n> + void startFrame(void);\n> + void finishFrame(void);\n> +\n> +private:\n> + unsigned int measuredFrames_;\n> + int64_t frameProcessTime_;\n> + timespec frameStartTime_;\n> + /* Skip 30 frames for things to stabilize then measure 30 frames */\n> + static constexpr unsigned int kFramesToSkip = 30;\n> + static constexpr unsigned int kLastFrameToMeasure = 60;\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 cf5ecdf7..897fb83b 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -528,9 +528,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> lineBuffers_[i].resize(lineBufferLength_);\n> }\n> \n> - measuredFrames_ = 0;\n> - frameProcessTime_ = 0;\n> -\n> return 0;\n> }\n> \n> @@ -739,22 +736,11 @@ void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n> }\n> }\n> \n> -inline int64_t timeDiff(timespec &after, timespec &before)\n> -{\n> - return (after.tv_sec - before.tv_sec) * 1000000000LL +\n> - (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> - timespec frameStartTime;\n> -\n> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n> - frameStartTime = {};\n> - clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> - }\n> + bench_.startFrame();\n> \n> syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> @@ -790,21 +776,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> \n> /* Measure before emitting signals */\n> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> - ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> - timespec frameEndTime = {};\n> - clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> - frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n> - if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n> - const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n> - DebayerCpu::kFramesToSkip;\n> - LOG(Debayer, Info)\n> - << \"Processed \" << measuredFrames\n> - << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> - << frameProcessTime_ / (1000 * measuredFrames)\n> - << \" us/frame\";\n> - }\n> - }\n> + bench_.finishFrame();\n> \n> /*\n> * 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 2c47e7c6..59015479 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -19,6 +19,7 @@\n> \n> #include \"libcamera/internal/bayer_format.h\"\n> \n> +#include \"benchmark.h\"\n> #include \"debayer.h\"\n> #include \"swstats_cpu.h\"\n> \n> @@ -153,11 +154,7 @@ private:\n> unsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> bool enableInputMemcpy_;\n> bool swapRedBlueGains_;\n> - unsigned int measuredFrames_;\n> - int64_t frameProcessTime_;\n> - /* Skip 30 frames for things to stabilize then measure 30 frames */\n> - static constexpr unsigned int kFramesToSkip = 30;\n> - static constexpr unsigned int kLastFrameToMeasure = 60;\n> + Benchmark 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',\n> -- \n> 2.46.2\n>","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 37A9AC32DE\n\tfor ;\n\tWed, 9 Oct 2024 23:06:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D08F63538;\n\tThu, 10 Oct 2024 01:06:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8CDDC618C5\n\tfor ;\n\tThu, 10 Oct 2024 01:06:03 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BE9EE226;\n\tThu, 10 Oct 2024 01:04:25 +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=\"MfhX5E9k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728515065;\n\tbh=J+Y4LnMAigIWVJEiafT/s1GSyyZpvjZ1o27hldNzTO4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=MfhX5E9kAE3CnSU5fgkvYbdIyMkxoi9gJfbIgn8H0xwFP8enaBJqM/73bxo8hjzfA\n\tqE/LoYvryfg5EOhIj/qce47Wt2Z3hymq+f6+eQPpl6V8mcjaOitiIZVHH4sMN3YRqr\n\tnhPj0PJBK9FnoYhBa+r8Tsc3Sq35JEIhI9AFSj3c=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241009200110.275544-4-hdegoede@redhat.com>","References":"<20241009200110.275544-1-hdegoede@redhat.com>\n\t<20241009200110.275544-4-hdegoede@redhat.com>","Subject":"Re: [RFC 3/4] libcamera: software_isp: Move benchmark code to its\n\town class","From":"Kieran Bingham ","Cc":"Milan Zamazal , Maxime Ripard , \n\tHans de Goede ","To":"Hans de Goede , libcamera-devel@lists.libcamera.org","Date":"Thu, 10 Oct 2024 00:06:00 +0100","Message-ID":"<172851516064.532453.14529043744196704862@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}},{"id":31703,"web_url":"https://patchwork.libcamera.org/comment/31703/","msgid":"<20241010194110.GH32107@pendragon.ideasonboard.com>","date":"2024-10-10T19:41:10","subject":"Re: [RFC 3/4] libcamera: software_isp: Move benchmark code to its\n\town class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Oct 10, 2024 at 12:06:00AM +0100, Kieran Bingham wrote:\n> Quoting Hans de Goede (2024-10-09 21:01:09)\n> > Move the code for the builtin benchmark to its own small\n> > Benchmark class.\n> \n> I think this is a good idea, and I'm not even going to quibble on the\n> implementation detail below, (assuming it compiles cleanly though the\n> CI) as it's currently mostly just moving code out from debayer_cpu.\n> \n> I could envisage this being more generically useful for any measurements\n> we might want to do, but in the event that crops up - that's when things\n> could be generalised.\n> \n> But essentially I think this is a good cleanup to a helper.\n\nI think it's fine for now for the soft ISP, but going forward, I think\nwe should consider using the tracing infrastructure again.\n\n> I could imagine that the instances might need to be named so we can\n> determine 'what' measurement is being reported, but if it's only\n> currently single use per pipeline maybe that isn't essential yet.\n> \n> I could also see a total time being recorded so we could track some sort\n> of utilisation percentage?\n> \n> And finally - I could imagine that it could be helpful to report the\n> benchmark/processing time measured for each frame in metadata, or even a\n> rolling average for metadata...\n> \n> But all of that is feature creep on top so:\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> > Signed-off-by: Hans de Goede \n> > ---\n> > src/libcamera/software_isp/benchmark.cpp | 78 ++++++++++++++++++++++\n> > src/libcamera/software_isp/benchmark.h | 36 ++++++++++\n> > src/libcamera/software_isp/debayer_cpu.cpp | 32 +--------\n> > src/libcamera/software_isp/debayer_cpu.h | 7 +-\n> > src/libcamera/software_isp/meson.build | 1 +\n> > 5 files changed, 119 insertions(+), 35 deletions(-)\n> > create mode 100644 src/libcamera/software_isp/benchmark.cpp\n> > create mode 100644 src/libcamera/software_isp/benchmark.h\n> > \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..eecad51c\n> > --- /dev/null\n> > +++ b/src/libcamera/software_isp/benchmark.cpp\n> > @@ -0,0 +1,78 @@\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 \"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> > + : 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> > + return (after.tv_sec - before.tv_sec) * 1000000000LL +\n> > + (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> > +}\n> > +\n> > +void Benchmark::startFrame(void)\n> > +{\n> > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n> > + return;\n> > +\n> > + frameStartTime_ = {};\n> > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_);\n> > +}\n> > +\n> > +void Benchmark::finishFrame(void)\n> > +{\n> > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n> > + return;\n> > +\n> > + measuredFrames_++;\n> > +\n> > + if (measuredFrames_ <= Benchmark::kFramesToSkip)\n> > + return;\n> > +\n> > + timespec frameEndTime = {};\n> > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> > + frameProcessTime_ += timeDiff(frameEndTime, frameStartTime_);\n> > +\n> > + if (measuredFrames_ == Benchmark::kLastFrameToMeasure) {\n> > + const unsigned int measuredFrames = Benchmark::kLastFrameToMeasure -\n> > + Benchmark::kFramesToSkip;\n> > + LOG(Benchmark, Info)\n> > + << \"Processed \" << measuredFrames\n> > + << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> > + << frameProcessTime_ / (1000 * measuredFrames)\n> > + << \" us/frame\";\n> > + }\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/software_isp/benchmark.h b/src/libcamera/software_isp/benchmark.h\n> > new file mode 100644\n> > index 00000000..8af25015\n> > --- /dev/null\n> > +++ b/src/libcamera/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> > +{\n> > +public:\n> > + Benchmark();\n> > + ~Benchmark();\n> > +\n> > + void startFrame(void);\n> > + void finishFrame(void);\n> > +\n> > +private:\n> > + unsigned int measuredFrames_;\n> > + int64_t frameProcessTime_;\n> > + timespec frameStartTime_;\n> > + /* Skip 30 frames for things to stabilize then measure 30 frames */\n> > + static constexpr unsigned int kFramesToSkip = 30;\n> > + static constexpr unsigned int kLastFrameToMeasure = 60;\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 cf5ecdf7..897fb83b 100644\n> > --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> > @@ -528,9 +528,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> > lineBuffers_[i].resize(lineBufferLength_);\n> > }\n> > \n> > - measuredFrames_ = 0;\n> > - frameProcessTime_ = 0;\n> > -\n> > return 0;\n> > }\n> > \n> > @@ -739,22 +736,11 @@ void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n> > }\n> > }\n> > \n> > -inline int64_t timeDiff(timespec &after, timespec &before)\n> > -{\n> > - return (after.tv_sec - before.tv_sec) * 1000000000LL +\n> > - (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> > - timespec frameStartTime;\n> > -\n> > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n> > - frameStartTime = {};\n> > - clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> > - }\n> > + bench_.startFrame();\n> > \n> > syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> > syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> > @@ -790,21 +776,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> > syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> > \n> > /* Measure before emitting signals */\n> > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> > - ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> > - timespec frameEndTime = {};\n> > - clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> > - frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n> > - if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n> > - const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n> > - DebayerCpu::kFramesToSkip;\n> > - LOG(Debayer, Info)\n> > - << \"Processed \" << measuredFrames\n> > - << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> > - << frameProcessTime_ / (1000 * measuredFrames)\n> > - << \" us/frame\";\n> > - }\n> > - }\n> > + bench_.finishFrame();\n> > \n> > /*\n> > * 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 2c47e7c6..59015479 100644\n> > --- a/src/libcamera/software_isp/debayer_cpu.h\n> > +++ b/src/libcamera/software_isp/debayer_cpu.h\n> > @@ -19,6 +19,7 @@\n> > \n> > #include \"libcamera/internal/bayer_format.h\"\n> > \n> > +#include \"benchmark.h\"\n> > #include \"debayer.h\"\n> > #include \"swstats_cpu.h\"\n> > \n> > @@ -153,11 +154,7 @@ private:\n> > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> > bool enableInputMemcpy_;\n> > bool swapRedBlueGains_;\n> > - unsigned int measuredFrames_;\n> > - int64_t frameProcessTime_;\n> > - /* Skip 30 frames for things to stabilize then measure 30 frames */\n> > - static constexpr unsigned int kFramesToSkip = 30;\n> > - static constexpr unsigned int kLastFrameToMeasure = 60;\n> > + Benchmark 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 E5735C32EA\n\tfor ;\n\tThu, 10 Oct 2024 19:41:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1086D6536B;\n\tThu, 10 Oct 2024 21:41:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6652F6353A\n\tfor ;\n\tThu, 10 Oct 2024 21:41:15 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [132.205.230.15])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B08FA5B2;\n\tThu, 10 Oct 2024 21:39:36 +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=\"OIegGSGN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728589177;\n\tbh=yPewdzAkso4jD9Xd+Swji+0SPiNwI9ENuIgEAkibSl8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OIegGSGNb2TV3gOHApp/CP1Sw5RuT8trLwS/p/fk66K4OYL4Fw4hl/0R7rJ35ULJf\n\tNEsqWpMsHuw/bDGOxaoN4BfipQ5zo9kcrSifP3KwRAyw9ztV/yBSnY7Zzcmi2p/SRU\n\tRdftMTu6Rr+RjmhJbS2bDaehow9r9JjoT0KZvYws=","Date":"Thu, 10 Oct 2024 22:41:10 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Hans de Goede , libcamera-devel@lists.libcamera.org,\n\tMilan Zamazal , Maxime Ripard ","Subject":"Re: [RFC 3/4] libcamera: software_isp: Move benchmark code to its\n\town class","Message-ID":"<20241010194110.GH32107@pendragon.ideasonboard.com>","References":"<20241009200110.275544-1-hdegoede@redhat.com>\n\t<20241009200110.275544-4-hdegoede@redhat.com>\n\t<172851516064.532453.14529043744196704862@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172851516064.532453.14529043744196704862@ping.linuxembedded.co.uk>","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\" "}},{"id":31762,"web_url":"https://patchwork.libcamera.org/comment/31762/","msgid":"<87cyk0gpn5.fsf@redhat.com>","date":"2024-10-16T13:10:06","subject":"Re: [RFC 3/4] libcamera: software_isp: Move benchmark code to its\n\town class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart writes:\n\n> On Thu, Oct 10, 2024 at 12:06:00AM +0100, Kieran Bingham wrote:\n>> Quoting Hans de Goede (2024-10-09 21:01:09)\n>> > Move the code for the builtin benchmark to its own small\n>\n>> > Benchmark class.\n>> \n>> I think this is a good idea, and I'm not even going to quibble on the\n>> implementation detail below, (assuming it compiles cleanly though the\n>> CI) as it's currently mostly just moving code out from debayer_cpu.\n>> \n>> I could envisage this being more generically useful for any measurements\n>> we might want to do, but in the event that crops up - that's when things\n>> could be generalised.\n>> \n>> But essentially I think this is a good cleanup to a helper.\n>\n> I think it's fine for now for the soft ISP, but going forward, I think\n> we should consider using the tracing infrastructure again.\n\nMaybe. But the current helper makes it super-easy to see contingent\nperformance changes.\n\n>> I could imagine that the instances might need to be named so we can\n>> determine 'what' measurement is being reported, but if it's only\n>> currently single use per pipeline maybe that isn't essential yet.\n>> \n>> I could also see a total time being recorded so we could track some sort\n>> of utilisation percentage?\n>> \n>> And finally - I could imagine that it could be helpful to report the\n>> benchmark/processing time measured for each frame in metadata, or even a\n>> rolling average for metadata...\n>> \n>> But all of that is feature creep on top so:\n>> \n>> \n>> Reviewed-by: Kieran Bingham \n\nReviewed-by: Milan Zamazal \n\n>> > Signed-off-by: Hans de Goede \n>> > ---\n>> > src/libcamera/software_isp/benchmark.cpp | 78 ++++++++++++++++++++++\n>> > src/libcamera/software_isp/benchmark.h | 36 ++++++++++\n>> > src/libcamera/software_isp/debayer_cpu.cpp | 32 +--------\n>> > src/libcamera/software_isp/debayer_cpu.h | 7 +-\n>> > src/libcamera/software_isp/meson.build | 1 +\n>> > 5 files changed, 119 insertions(+), 35 deletions(-)\n>> > create mode 100644 src/libcamera/software_isp/benchmark.cpp\n>> > create mode 100644 src/libcamera/software_isp/benchmark.h\n>> > \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..eecad51c\n>> > --- /dev/null\n>> > +++ b/src/libcamera/software_isp/benchmark.cpp\n>> > @@ -0,0 +1,78 @@\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 \"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>> > + : 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>> > + return (after.tv_sec - before.tv_sec) * 1000000000LL +\n>> > + (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n>> > +}\n>> > +\n>> > +void Benchmark::startFrame(void)\n>> > +{\n>> > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n>> > + return;\n>> > +\n>> > + frameStartTime_ = {};\n>> > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_);\n>> > +}\n>> > +\n>> > +void Benchmark::finishFrame(void)\n>> > +{\n>> > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n>> > + return;\n>> > +\n>> > + measuredFrames_++;\n>> > +\n>> > + if (measuredFrames_ <= Benchmark::kFramesToSkip)\n>> > + return;\n>> > +\n>> > + timespec frameEndTime = {};\n>> > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n>> > + frameProcessTime_ += timeDiff(frameEndTime, frameStartTime_);\n>> > +\n>> > + if (measuredFrames_ == Benchmark::kLastFrameToMeasure) {\n>> > + const unsigned int measuredFrames = Benchmark::kLastFrameToMeasure -\n>> > + Benchmark::kFramesToSkip;\n>> > + LOG(Benchmark, Info)\n>> > + << \"Processed \" << measuredFrames\n>> > + << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n>> > + << frameProcessTime_ / (1000 * measuredFrames)\n>> > + << \" us/frame\";\n>> > + }\n>> > +}\n>> > +\n>> > +} /* namespace libcamera */\n>> > diff --git a/src/libcamera/software_isp/benchmark.h b/src/libcamera/software_isp/benchmark.h\n>> > new file mode 100644\n>> > index 00000000..8af25015\n>> > --- /dev/null\n>> > +++ b/src/libcamera/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>> > +{\n>> > +public:\n>> > + Benchmark();\n>> > + ~Benchmark();\n>> > +\n>> > + void startFrame(void);\n>> > + void finishFrame(void);\n>> > +\n>> > +private:\n>> > + unsigned int measuredFrames_;\n>> > + int64_t frameProcessTime_;\n>> > + timespec frameStartTime_;\n>> > + /* Skip 30 frames for things to stabilize then measure 30 frames */\n>> > + static constexpr unsigned int kFramesToSkip = 30;\n>> > + static constexpr unsigned int kLastFrameToMeasure = 60;\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 cf5ecdf7..897fb83b 100644\n>> > --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> > @@ -528,9 +528,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>> > lineBuffers_[i].resize(lineBufferLength_);\n>> > }\n>> > \n>> > - measuredFrames_ = 0;\n>> > - frameProcessTime_ = 0;\n>> > -\n>> > return 0;\n>> > }\n>> > \n>> > @@ -739,22 +736,11 @@ void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n>> > }\n>> > }\n>> > \n>> > -inline int64_t timeDiff(timespec &after, timespec &before)\n>> > -{\n>> > - return (after.tv_sec - before.tv_sec) * 1000000000LL +\n>> > - (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>> > - timespec frameStartTime;\n>> > -\n>> > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n>> > - frameStartTime = {};\n>> > - clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n>> > - }\n>> > + bench_.startFrame();\n>> > \n>> > syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n>> > syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n>> > @@ -790,21 +776,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>> > syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n>> > \n>> > /* Measure before emitting signals */\n>> > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>> > - ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n>> > - timespec frameEndTime = {};\n>> > - clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n>> > - frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n>> > - if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n>> > - const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n>> > - DebayerCpu::kFramesToSkip;\n>> > - LOG(Debayer, Info)\n>> > - << \"Processed \" << measuredFrames\n>> > - << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n>> > - << frameProcessTime_ / (1000 * measuredFrames)\n>> > - << \" us/frame\";\n>> > - }\n>> > - }\n>> > + bench_.finishFrame();\n>> > \n>> > /*\n>> > * 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 2c47e7c6..59015479 100644\n>> > --- a/src/libcamera/software_isp/debayer_cpu.h\n>> > +++ b/src/libcamera/software_isp/debayer_cpu.h\n>> > @@ -19,6 +19,7 @@\n>> > \n>> > #include \"libcamera/internal/bayer_format.h\"\n>> > \n>> > +#include \"benchmark.h\"\n>> > #include \"debayer.h\"\n>> > #include \"swstats_cpu.h\"\n>> > \n>> > @@ -153,11 +154,7 @@ private:\n>> > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n>> > bool enableInputMemcpy_;\n>> > bool swapRedBlueGains_;\n>> > - unsigned int measuredFrames_;\n>> > - int64_t frameProcessTime_;\n>> > - /* Skip 30 frames for things to stabilize then measure 30 frames */\n>> > - static constexpr unsigned int kFramesToSkip = 30;\n>> > - static constexpr unsigned int kLastFrameToMeasure = 60;\n>> > + Benchmark 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 7EC64C326C\n\tfor ;\n\tWed, 16 Oct 2024 13:10:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E13165383;\n\tWed, 16 Oct 2024 15:10:14 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 50BA46537E\n\tfor ;\n\tWed, 16 Oct 2024 15:10:12 +0200 (CEST)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-77-H8BoRdx6MrSyYG5vQ25xjA-1; Wed, 16 Oct 2024 09:10:09 -0400","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-37d3e8dccc9so3231281f8f.1\n\tfor ;\n\tWed, 16 Oct 2024 06:10:09 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-37d7fa87adasm4277620f8f.29.2024.10.16.06.10.06\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 16 Oct 2024 06:10:07 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"jN2PLhwk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1729084211;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=tOC/q9NM/Drrt9Qk+keYoFIwRxi833ziM07RGHDUmIA=;\n\tb=jN2PLhwkMwb27OxWyTANsWXBIsMtXVUxa8VM7E++YXTlhbAhhgD+GO3gHwcOo3BiBcQCQP\n\tW27OQc5Ms2Gt2SkfE0aD/zwTIpWawh6HZvLGwZzc3juAv/NZDGt5G3yJyGe7S9yawH+v9h\n\tik9ZGFdbrFO9oY36bMnyMO0WrbH26zE=","X-MC-Unique":"H8BoRdx6MrSyYG5vQ25xjA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1729084208; x=1729689008;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=tOC/q9NM/Drrt9Qk+keYoFIwRxi833ziM07RGHDUmIA=;\n\tb=TJWItl6hqwyimTK6LDnqSnijx9z8verraF1/dwUZtmBxoDk7bfzNplMi5YCPKekufH\n\t68bbniCfeh+pmtJ9bB54K/Y5S+0D7vyOUv8t7F92o9N2M2wSyA3cwE/B0W+NiYdXy3y2\n\tGWYgGD3cpvHMT6OgmLAOl3O/a4HkzA1ys37Mwd2Y7vOV46WWTT3zP4U07Ht20a9KJXHT\n\tt/XRQ5BMZEYXSWzL/B6py8F7OWkm3k/41q4huwcastJvmu0wVJsZ6+rWkz/B2pOVf4T7\n\tGFCAbtqh9gEviw2njGOHqBF+PyGun9k0cksQfMIt17LK5yL2vr0F3HIsmUJRsSCQ/b8K\n\tFDbQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVpulZwtaOqn9w/xWAoVnScLi9F1/R+suxm3UlQv0149tFgCVVfWfvJ7O6GxAKRRGN2VkMGFWTr1ehbOO2MK5U=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YxXNkDyIZSVt7SwwQ+6ii+ej+d0H91JFmxcLfMANu766rwxDZmz\n\t3IMb4Dt5v7fDf19I0ox4osO6L6OqWDUP0olAz3X1mrdVy/c3WAa+yNq+7RRyKQ3IB15sobcwIeu\n\tLLcuedNb5NK/w2F7WDoSZRAbmDpqTqr99osvVxLBW0J2l1+hq45YqoJTL99UvIPsCi2dGqBedIC\n\tbZHvA=","X-Received":["by 2002:a5d:6112:0:b0:368:37ac:3f95 with SMTP id\n\tffacd0b85a97d-37d551fda4dmr12915547f8f.31.1729084208350; \n\tWed, 16 Oct 2024 06:10:08 -0700 (PDT)","by 2002:a5d:6112:0:b0:368:37ac:3f95 with SMTP id\n\tffacd0b85a97d-37d551fda4dmr12915525f8f.31.1729084207850; \n\tWed, 16 Oct 2024 06:10:07 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFw4OntMb4UBaUj9GxYSKNzwgOO90DMwu9rxX2BBbux1FX+lU1Ialu0eKnlyobN22NZFn2IbA==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"Kieran Bingham , Hans de Goede\n\t, libcamera-devel@lists.libcamera.org, Maxime\n\tRipard ","Subject":"Re: [RFC 3/4] libcamera: software_isp: Move benchmark code to its\n\town class","In-Reply-To":"<20241010194110.GH32107@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Thu, 10 Oct 2024 22:41:10 +0300\")","References":"<20241009200110.275544-1-hdegoede@redhat.com>\n\t<20241009200110.275544-4-hdegoede@redhat.com>\n\t<172851516064.532453.14529043744196704862@ping.linuxembedded.co.uk>\n\t<20241010194110.GH32107@pendragon.ideasonboard.com>","Date":"Wed, 16 Oct 2024 15:10:06 +0200","Message-ID":"<87cyk0gpn5.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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\" "}},{"id":31771,"web_url":"https://patchwork.libcamera.org/comment/31771/","msgid":"<20241016145022.GE30496@pendragon.ideasonboard.com>","date":"2024-10-16T14:50:22","subject":"Re: [RFC 3/4] libcamera: software_isp: Move benchmark code to its\n\town class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 16, 2024 at 03:10:06PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Thu, Oct 10, 2024 at 12:06:00AM +0100, Kieran Bingham wrote:\n> >> Quoting Hans de Goede (2024-10-09 21:01:09)\n> >> > Move the code for the builtin benchmark to its own small\n> >> > Benchmark class.\n> >> \n> >> I think this is a good idea, and I'm not even going to quibble on the\n> >> implementation detail below, (assuming it compiles cleanly though the\n> >> CI) as it's currently mostly just moving code out from debayer_cpu.\n> >> \n> >> I could envisage this being more generically useful for any measurements\n> >> we might want to do, but in the event that crops up - that's when things\n> >> could be generalised.\n> >> \n> >> But essentially I think this is a good cleanup to a helper.\n> >\n> > I think it's fine for now for the soft ISP, but going forward, I think\n> > we should consider using the tracing infrastructure again.\n> \n> Maybe. But the current helper makes it super-easy to see contingent\n> performance changes.\n\nTrue, but that can be said of pretty much any tracing-related needs,\nit's easier to implement a ad-hoc solution compared to overcoming the\ntracing barrier. It doesn't scale though, so at some point we have to\nbite the bullet, spend the time required to use tracing (and likely\nimplement some trace analysis scripts, or document specific tracing use\ncases), and benefit from that in the long term.\n\n> >> I could imagine that the instances might need to be named so we can\n> >> determine 'what' measurement is being reported, but if it's only\n> >> currently single use per pipeline maybe that isn't essential yet.\n> >> \n> >> I could also see a total time being recorded so we could track some sort\n> >> of utilisation percentage?\n> >> \n> >> And finally - I could imagine that it could be helpful to report the\n> >> benchmark/processing time measured for each frame in metadata, or even a\n> >> rolling average for metadata...\n> >> \n> >> But all of that is feature creep on top so:\n> >> \n> >> \n> >> Reviewed-by: Kieran Bingham \n> \n> Reviewed-by: Milan Zamazal \n> \n> >> > Signed-off-by: Hans de Goede \n> >> > ---\n> >> > src/libcamera/software_isp/benchmark.cpp | 78 ++++++++++++++++++++++\n> >> > src/libcamera/software_isp/benchmark.h | 36 ++++++++++\n> >> > src/libcamera/software_isp/debayer_cpu.cpp | 32 +--------\n> >> > src/libcamera/software_isp/debayer_cpu.h | 7 +-\n> >> > src/libcamera/software_isp/meson.build | 1 +\n> >> > 5 files changed, 119 insertions(+), 35 deletions(-)\n> >> > create mode 100644 src/libcamera/software_isp/benchmark.cpp\n> >> > create mode 100644 src/libcamera/software_isp/benchmark.h\n> >> > \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..eecad51c\n> >> > --- /dev/null\n> >> > +++ b/src/libcamera/software_isp/benchmark.cpp\n> >> > @@ -0,0 +1,78 @@\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 \"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> >> > + : 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> >> > + return (after.tv_sec - before.tv_sec) * 1000000000LL +\n> >> > + (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> >> > +}\n> >> > +\n> >> > +void Benchmark::startFrame(void)\n> >> > +{\n> >> > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n> >> > + return;\n> >> > +\n> >> > + frameStartTime_ = {};\n> >> > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_);\n> >> > +}\n> >> > +\n> >> > +void Benchmark::finishFrame(void)\n> >> > +{\n> >> > + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n> >> > + return;\n> >> > +\n> >> > + measuredFrames_++;\n> >> > +\n> >> > + if (measuredFrames_ <= Benchmark::kFramesToSkip)\n> >> > + return;\n> >> > +\n> >> > + timespec frameEndTime = {};\n> >> > + clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> >> > + frameProcessTime_ += timeDiff(frameEndTime, frameStartTime_);\n> >> > +\n> >> > + if (measuredFrames_ == Benchmark::kLastFrameToMeasure) {\n> >> > + const unsigned int measuredFrames = Benchmark::kLastFrameToMeasure -\n> >> > + Benchmark::kFramesToSkip;\n> >> > + LOG(Benchmark, Info)\n> >> > + << \"Processed \" << measuredFrames\n> >> > + << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> >> > + << frameProcessTime_ / (1000 * measuredFrames)\n> >> > + << \" us/frame\";\n> >> > + }\n> >> > +}\n> >> > +\n> >> > +} /* namespace libcamera */\n> >> > diff --git a/src/libcamera/software_isp/benchmark.h b/src/libcamera/software_isp/benchmark.h\n> >> > new file mode 100644\n> >> > index 00000000..8af25015\n> >> > --- /dev/null\n> >> > +++ b/src/libcamera/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> >> > +{\n> >> > +public:\n> >> > + Benchmark();\n> >> > + ~Benchmark();\n> >> > +\n> >> > + void startFrame(void);\n> >> > + void finishFrame(void);\n> >> > +\n> >> > +private:\n> >> > + unsigned int measuredFrames_;\n> >> > + int64_t frameProcessTime_;\n> >> > + timespec frameStartTime_;\n> >> > + /* Skip 30 frames for things to stabilize then measure 30 frames */\n> >> > + static constexpr unsigned int kFramesToSkip = 30;\n> >> > + static constexpr unsigned int kLastFrameToMeasure = 60;\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 cf5ecdf7..897fb83b 100644\n> >> > --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> >> > +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> >> > @@ -528,9 +528,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> >> > lineBuffers_[i].resize(lineBufferLength_);\n> >> > }\n> >> > \n> >> > - measuredFrames_ = 0;\n> >> > - frameProcessTime_ = 0;\n> >> > -\n> >> > return 0;\n> >> > }\n> >> > \n> >> > @@ -739,22 +736,11 @@ void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n> >> > }\n> >> > }\n> >> > \n> >> > -inline int64_t timeDiff(timespec &after, timespec &before)\n> >> > -{\n> >> > - return (after.tv_sec - before.tv_sec) * 1000000000LL +\n> >> > - (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> >> > - timespec frameStartTime;\n> >> > -\n> >> > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n> >> > - frameStartTime = {};\n> >> > - clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> >> > - }\n> >> > + bench_.startFrame();\n> >> > \n> >> > syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> >> > syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> >> > @@ -790,21 +776,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> >> > syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> >> > \n> >> > /* Measure before emitting signals */\n> >> > - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> >> > - ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> >> > - timespec frameEndTime = {};\n> >> > - clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> >> > - frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n> >> > - if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n> >> > - const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n> >> > - DebayerCpu::kFramesToSkip;\n> >> > - LOG(Debayer, Info)\n> >> > - << \"Processed \" << measuredFrames\n> >> > - << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> >> > - << frameProcessTime_ / (1000 * measuredFrames)\n> >> > - << \" us/frame\";\n> >> > - }\n> >> > - }\n> >> > + bench_.finishFrame();\n> >> > \n> >> > /*\n> >> > * 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 2c47e7c6..59015479 100644\n> >> > --- a/src/libcamera/software_isp/debayer_cpu.h\n> >> > +++ b/src/libcamera/software_isp/debayer_cpu.h\n> >> > @@ -19,6 +19,7 @@\n> >> > \n> >> > #include \"libcamera/internal/bayer_format.h\"\n> >> > \n> >> > +#include \"benchmark.h\"\n> >> > #include \"debayer.h\"\n> >> > #include \"swstats_cpu.h\"\n> >> > \n> >> > @@ -153,11 +154,7 @@ private:\n> >> > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> >> > bool enableInputMemcpy_;\n> >> > bool swapRedBlueGains_;\n> >> > - unsigned int measuredFrames_;\n> >> > - int64_t frameProcessTime_;\n> >> > - /* Skip 30 frames for things to stabilize then measure 30 frames */\n> >> > - static constexpr unsigned int kFramesToSkip = 30;\n> >> > - static constexpr unsigned int kLastFrameToMeasure = 60;\n> >> > + Benchmark 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 CD826C326C\n\tfor ;\n\tWed, 16 Oct 2024 14:50:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5758665383;\n\tWed, 16 Oct 2024 16:50:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D95C36537E\n\tfor ;\n\tWed, 16 Oct 2024 16:50:26 +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 54D5BA2F;\n\tWed, 16 Oct 2024 16:48:44 +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=\"I1N/VkkY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729090124;\n\tbh=hu2TDb0hHOCf4u3Quy2CRVlmzT/rHjn+ZHUvomIRfTY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I1N/VkkYviDKx3TKRuABZDZy7gOX1eu7PHFCzcWX4GMxS9EBkw0U8DmtxvpszlNw2\n\tQFVcaPbR5XX6DpbryNw/XJeKvT/hXYdeSPYawgbFWOhZDCueIfkKdIAwP9mIdW0fAc\n\tX5Xry82ZuukYzQu67irkhRpIrlvcDrk5kpwz0DuQ=","Date":"Wed, 16 Oct 2024 17:50:22 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"Kieran Bingham ,\n\tHans de Goede ,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard ","Subject":"Re: [RFC 3/4] libcamera: software_isp: Move benchmark code to its\n\town class","Message-ID":"<20241016145022.GE30496@pendragon.ideasonboard.com>","References":"<20241009200110.275544-1-hdegoede@redhat.com>\n\t<20241009200110.275544-4-hdegoede@redhat.com>\n\t<172851516064.532453.14529043744196704862@ping.linuxembedded.co.uk>\n\t<20241010194110.GH32107@pendragon.ideasonboard.com>\n\t<87cyk0gpn5.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87cyk0gpn5.fsf@redhat.com>","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\" "}},{"id":31809,"web_url":"https://patchwork.libcamera.org/comment/31809/","msgid":"<172928873803.532453.8134994112218356336@ping.linuxembedded.co.uk>","date":"2024-10-18T21:58:58","subject":"Re: [RFC 3/4] libcamera: software_isp: Move benchmark code to its\n\town class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2024-10-09 21:01:09)\n> Move the code for the builtin benchmark to its own small\n> Benchmark class.\n> \n> Signed-off-by: Hans de Goede \n> ---\n> src/libcamera/software_isp/benchmark.cpp | 78 ++++++++++++++++++++++\n> src/libcamera/software_isp/benchmark.h | 36 ++++++++++\n> src/libcamera/software_isp/debayer_cpu.cpp | 32 +--------\n> src/libcamera/software_isp/debayer_cpu.h | 7 +-\n> src/libcamera/software_isp/meson.build | 1 +\n> 5 files changed, 119 insertions(+), 35 deletions(-)\n> create mode 100644 src/libcamera/software_isp/benchmark.cpp\n> create mode 100644 src/libcamera/software_isp/benchmark.h\n> \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..eecad51c\n> --- /dev/null\n> +++ b/src/libcamera/software_isp/benchmark.cpp\n> @@ -0,0 +1,78 @@\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 \"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> + : 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> + return (after.tv_sec - before.tv_sec) * 1000000000LL +\n> + (int64_t)after.tv_nsec - (int64_t)before.tv_nsec;\n> +}\n> +\n> +void Benchmark::startFrame(void)\n\nI was /so/ close to merging the first 3 patches of this series (4/4\nisn't \"used\" so I don't think it's so easy to just merge) - but doxygen\nkicks out at the undocumented class entries:\n\nhttps://gitlab.freedesktop.org/camera/libcamera/-/jobs/65324937\n\n\n--\nKieran\n\n\n> +{\n> + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n> + return;\n> +\n> + frameStartTime_ = {};\n> + clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime_);\n> +}\n> +\n> +void Benchmark::finishFrame(void)\n> +{\n> + if (measuredFrames_ >= Benchmark::kLastFrameToMeasure)\n> + return;\n> +\n> + measuredFrames_++;\n> +\n> + if (measuredFrames_ <= Benchmark::kFramesToSkip)\n> + return;\n> +\n> + timespec frameEndTime = {};\n> + clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> + frameProcessTime_ += timeDiff(frameEndTime, frameStartTime_);\n> +\n> + if (measuredFrames_ == Benchmark::kLastFrameToMeasure) {\n> + const unsigned int measuredFrames = Benchmark::kLastFrameToMeasure -\n> + Benchmark::kFramesToSkip;\n> + LOG(Benchmark, Info)\n> + << \"Processed \" << measuredFrames\n> + << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> + << frameProcessTime_ / (1000 * measuredFrames)\n> + << \" us/frame\";\n> + }\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/software_isp/benchmark.h b/src/libcamera/software_isp/benchmark.h\n> new file mode 100644\n> index 00000000..8af25015\n> --- /dev/null\n> +++ b/src/libcamera/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> +{\n> +public:\n> + Benchmark();\n> + ~Benchmark();\n> +\n> + void startFrame(void);\n> + void finishFrame(void);\n> +\n> +private:\n> + unsigned int measuredFrames_;\n> + int64_t frameProcessTime_;\n> + timespec frameStartTime_;\n> + /* Skip 30 frames for things to stabilize then measure 30 frames */\n> + static constexpr unsigned int kFramesToSkip = 30;\n> + static constexpr unsigned int kLastFrameToMeasure = 60;\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 cf5ecdf7..897fb83b 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -528,9 +528,6 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> lineBuffers_[i].resize(lineBufferLength_);\n> }\n> \n> - measuredFrames_ = 0;\n> - frameProcessTime_ = 0;\n> -\n> return 0;\n> }\n> \n> @@ -739,22 +736,11 @@ void syncBufferForCPU(FrameBuffer *buffer, uint64_t syncFlags)\n> }\n> }\n> \n> -inline int64_t timeDiff(timespec &after, timespec &before)\n> -{\n> - return (after.tv_sec - before.tv_sec) * 1000000000LL +\n> - (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> - timespec frameStartTime;\n> -\n> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n> - frameStartTime = {};\n> - clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> - }\n> + bench_.startFrame();\n> \n> syncBufferForCPU(input, DMA_BUF_SYNC_START | DMA_BUF_SYNC_READ);\n> syncBufferForCPU(output, DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE);\n> @@ -790,21 +776,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> syncBufferForCPU(input, DMA_BUF_SYNC_END | DMA_BUF_SYNC_READ);\n> \n> /* Measure before emitting signals */\n> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> - ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> - timespec frameEndTime = {};\n> - clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> - frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n> - if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n> - const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n> - DebayerCpu::kFramesToSkip;\n> - LOG(Debayer, Info)\n> - << \"Processed \" << measuredFrames\n> - << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> - << frameProcessTime_ / (1000 * measuredFrames)\n> - << \" us/frame\";\n> - }\n> - }\n> + bench_.finishFrame();\n> \n> /*\n> * 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 2c47e7c6..59015479 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -19,6 +19,7 @@\n> \n> #include \"libcamera/internal/bayer_format.h\"\n> \n> +#include \"benchmark.h\"\n> #include \"debayer.h\"\n> #include \"swstats_cpu.h\"\n> \n> @@ -153,11 +154,7 @@ private:\n> unsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> bool enableInputMemcpy_;\n> bool swapRedBlueGains_;\n> - unsigned int measuredFrames_;\n> - int64_t frameProcessTime_;\n> - /* Skip 30 frames for things to stabilize then measure 30 frames */\n> - static constexpr unsigned int kFramesToSkip = 30;\n> - static constexpr unsigned int kLastFrameToMeasure = 60;\n> + Benchmark 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',\n> -- \n> 2.46.2\n>","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 50692C32FE\n\tfor ;\n\tFri, 18 Oct 2024 21:59:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0AAFF6538A;\n\tFri, 18 Oct 2024 23:59:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B6E44633C6\n\tfor ;\n\tFri, 18 Oct 2024 23:59:00 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8ACF5268;\n\tFri, 18 Oct 2024 23:57:16 +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=\"P9kWOsVf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729288636;\n\tbh=wTz/jnIlFp8etJAEDuR3ZIN7qALbcnykVzrxdFCBdPo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=P9kWOsVft/yalsGXHG1vWcPd54fyEWA+qh82sv42M62ngUK3R4bpuD6Rg7K9dsaGc\n\tc/khH7Lf9OagFTBTfRrzopW9hueRWvFnA5Fe9DgbAjiZHVavS69YeCSzaLuBkviYui\n\ttpIx9gkpKyRkcfbM1UpntKHWhuT9dadkKaxcHVqE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241009200110.275544-4-hdegoede@redhat.com>","References":"<20241009200110.275544-1-hdegoede@redhat.com>\n\t<20241009200110.275544-4-hdegoede@redhat.com>","Subject":"Re: [RFC 3/4] libcamera: software_isp: Move benchmark code to its\n\town class","From":"Kieran Bingham ","Cc":"Milan Zamazal , Maxime Ripard , \n\tHans de Goede ","To":"Hans de Goede , libcamera-devel@lists.libcamera.org","Date":"Fri, 18 Oct 2024 22:58:58 +0100","Message-ID":"<172928873803.532453.8134994112218356336@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}}]