[{"id":35934,"web_url":"https://patchwork.libcamera.org/comment/35934/","msgid":"<20250921030538.GB30137@pendragon.ideasonboard.com>","date":"2025-09-21T03:05:38","subject":"Re: [PATCH v18 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Fri, Sep 12, 2025 at 04:29:12PM +0200, Milan Zamazal wrote:\n> Software ISP performs performance measurement on certain part of initial\n> frames. Let's make this range configurable.\n> \n> For this purpose, this patch introduces new configuration options\n> pipelines.simple.measure.skip and pipelines.simple.measure.number.\n\nIt's software_isp.measure.skip and software_isp.measure.number. I'll fix\nit.\n\n> Setting the latter one to 0 disables the measurement.\n> \n> Instead of the last frame, the class member and its configuration\n> specify the number of frames to measure. This is easier to use for\n> users and doesn't require to adjust two configuration parameters when\n> the number of the initially skipped frames is changed.\n> \n> The patch also changes the names of the class members to make them more\n> accurate.\n> \n> Completes software ISP TODO #7.\n> \n> Reviewed-by: Paul Elder \n> Signed-off-by: Milan Zamazal \n> ---\n> Documentation/runtime_configuration.rst | 18 ++++++++++++++++\n> src/libcamera/software_isp/TODO | 25 ----------------------\n> src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++--------\n> src/libcamera/software_isp/debayer_cpu.h | 7 +++---\n> 4 files changed, 37 insertions(+), 38 deletions(-)\n> \n> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst\n> index 6610e8bec..702139544 100644\n> --- a/Documentation/runtime_configuration.rst\n> +++ b/Documentation/runtime_configuration.rst\n> @@ -57,6 +57,9 @@ file structure:\n> software_isp: # true/false\n> software_isp:\n> copy_input_buffer: # true/false\n> + measure:\n\nI'd name this perf_measure (or something similar). I'll send a patch on\ntop to propose the change.\n\n> + skip: # non-negative integer, frames to skip initially\n> + number: # non-negative integer, frames to measure\n> \n> Configuration file example\n> --------------------------\n> @@ -92,6 +95,9 @@ Configuration file example\n> software_isp: true\n> software_isp:\n> copy_input_buffer: false\n> + measure:\n> + skip: 50\n> + number: 30\n> \n> List of variables and configuration options\n> -------------------------------------------\n> @@ -167,6 +173,18 @@ software_isp.copy_input_buffer\n> \n> Example value: ``false``\n> \n> +software_isp.measure.skip, software_isp.measure.number\n> + Define per-frame time measurement parameters in software ISP. `skip`\n\n\"Define performance measurement parameters\" would be more accurate I\nthink. I'll also send a patch on top.\n\n> + defines how many initial frames are skipped before starting the\n> + measurement; `number` defines how many frames then participate in the\n> + measurement.\n> +\n> + Set `software_isp.measure.number` to 0 to disable the measurement.\n> +\n> + Example `skip` value: ``50``\n> +\n> + Example `number` value: ``30``\n> +\n> Further details\n> ---------------\n> \n> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO\n> index 2c919f442..f19e15ae2 100644\n> --- a/src/libcamera/software_isp/TODO\n> +++ b/src/libcamera/software_isp/TODO\n> @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs.\n> \n> ---\n> \n> -7. Performance measurement configuration\n> -\n> -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n> -> /* Measure before emitting signals */\n> -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> -> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> -> \ttimespec frameEndTime = {};\n> -> \tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> -> \tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n> -> \tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n> -> \t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n> -> \t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n> -> \t\tLOG(Debayer, 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> -I wonder if there would be a way to control at runtime when/how to\n> -perform those measurements. Maybe that's a bit overkill.\n> -\n> ----\n> -\n> 8. DebayerCpu cleanups\n> \n> > >> class DebayerCpu : public Debayer, public Object\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index cec6cc6be..2f57857ad 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats, const GlobalConfigurat\n> \tenableInputMemcpy_ =\n> \t\tconfiguration.option({ \"software_isp\", \"copy_input_buffer\" }).value_or(true);\n> \n> +\tskipBeforeMeasure_ = configuration.option(\n> +\t\t\t\t\t\t { \"software_isp\", \"measure\", \"skip\" })\n> +\t\t\t\t .value_or(skipBeforeMeasure_);\n> +\tframesToMeasure_ = configuration.option(\n> +\t\t\t\t\t\t{ \"software_isp\", \"measure\", \"number\" })\n> +\t\t\t\t .value_or(framesToMeasure_);\n\nSeeing how the GlobalConfiguration class API is used through the series\nfor different purposes gave me a good view of usage patterns, and a few\nideas for improvements. There will be RFC patches coming.\n\n> +\n> \t/* Initialize color lookup tables */\n> \tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> \t\tred_[i] = green_[i] = blue_[i] = i;\n> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> \t\t\tlineBuffers_[i].resize(lineBufferLength_);\n> \t}\n> \n> -\tmeasuredFrames_ = 0;\n> +\tencounteredFrames_ = 0;\n\nNot too fond of this new name as the counter is used for performance\nmeasurement only, I think the old name was better (even if not perfect).\nThis can be addressed later.\n\nReviewed-by: Laurent Pinchart \n\n> \tframeProcessTime_ = 0;\n> \n> \treturn 0;\n> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> {\n> \ttimespec frameStartTime;\n> \n> -\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n> +\tbool measure = framesToMeasure_ > 0 &&\n> +\t\t encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&\n> +\t\t ++encounteredFrames_ > skipBeforeMeasure_;\n> +\tif (measure) {\n> \t\tframeStartTime = {};\n> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> \t}\n> @@ -820,18 +830,15 @@ 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> +\tif (measure) {\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\tif (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {\n> \t\t\tLOG(Debayer, Info)\n> -\t\t\t\t<< \"Processed \" << measuredFrames\n> +\t\t\t\t<< \"Processed \" << framesToMeasure_\n> \t\t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> -\t\t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n> +\t\t\t\t<< frameProcessTime_ / (1000 * framesToMeasure_)\n> \t\t\t\t<< \" us/frame\";\n> \t\t}\n> \t}\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index 2f35aa18b..9d343e464 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -161,11 +161,10 @@ private:\n> \tunsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> \tbool enableInputMemcpy_;\n> \tbool swapRedBlueGains_;\n> -\tunsigned int measuredFrames_;\n> +\tunsigned int encounteredFrames_;\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> +\tunsigned int skipBeforeMeasure_ = 30;\n> +\tunsigned int framesToMeasure_ = 30;\n> };\n> \n> } /* namespace libcamera */","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 2148CBE173\n\tfor ;\n\tSun, 21 Sep 2025 03:06:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C6A396B59C;\n\tSun, 21 Sep 2025 05:06:10 +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 E938162C35\n\tfor ;\n\tSun, 21 Sep 2025 05:06:08 +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 UTF8SMTPSA id 384DE7CE;\n\tSun, 21 Sep 2025 05:04:47 +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=\"SRb3Is7r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758423887;\n\tbh=ZAMoHCzKQTq5UUZnSWiZ01qJajDFlkTqDiGVGS6xyyI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SRb3Is7rMatyu97Z/zEUwRQHe0g3RIICnyW0Ymf+agyGucMdcQm791oEcqrRfPEaP\n\tXpy14f06gg/56DjsyrKZ6rMttLqaJMhn6YhEzhQ0tr8Sf9gWYVP9KLucrgbLAmGKsf\n\tNjOxddnP4moxWTDAUawX3uTuy77urMbo4bT4jZ9g=","Date":"Sun, 21 Sep 2025 06:05:38 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?=\n\t, Paul Elder\n\t","Subject":"Re: [PATCH v18 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","Message-ID":"<20250921030538.GB30137@pendragon.ideasonboard.com>","References":"<20250912142915.53949-1-mzamazal@redhat.com>\n\t<20250912142915.53949-12-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250912142915.53949-12-mzamazal@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\" "}}]