[{"id":35740,"web_url":"https://patchwork.libcamera.org/comment/35740/","msgid":"<175741074480.2127323.7349577929317341989@neptunite.rasen.tech>","date":"2025-09-09T09:39:04","subject":"Re: [PATCH v16 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-07-29 16:31:58)\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> 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> Signed-off-by: Milan Zamazal \n> ---\n> Documentation/runtime_configuration.rst | 6 ++++++\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, 25 insertions(+), 38 deletions(-)\n> \n> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst\n> index 57285cadb..0708302d4 100644\n> --- a/Documentation/runtime_configuration.rst\n> +++ b/Documentation/runtime_configuration.rst\n> @@ -53,6 +53,9 @@ file structure:\n> ...\n> simple:\n> copy_input_buffer: # true/false\n> + measure:\n> + skip: # non-negative integer, frames to skip initially\n> + number: # non-negative integer, frames to measure\n\nWe might want to upgrade the documentation for the configuration file in the\nfuture, but that can be done on later.\n\n> supported_devices:\n> - driver: # driver name, e.g. `mxc-isi`\n> software_isp: # true/false\n> @@ -87,6 +90,9 @@ Configuration file example\n> min_total_unicam_buffers: 2\n> simple:\n> copy_input_buffer: false\n> + measure:\n> + skip: 50\n> + number: 30\n> supported_devices:\n> - driver: mxc-isi\n> software_isp: true\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> -> 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> -\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 704d4e487..d43687f75 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> enableInputMemcpy_ =\n> configuration.option({ \"pipelines\", \"simple\", \"copy_input_buffer\" }).value_or(true);\n> \n> + skipBeforeMeasure_ = configuration.option(\n> + { \"pipelines\", \"simple\", \"measure\", \"skip\" })\n> + .value_or(skipBeforeMeasure_);\n> + framesToMeasure_ = configuration.option(\n> + { \"pipelines\", \"simple\", \"measure\", \"number\" })\n> + .value_or(framesToMeasure_);\n> +\n> /* Initialize color lookup tables */\n> for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> red_[i] = green_[i] = blue_[i] = i;\n> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> lineBuffers_[i].resize(lineBufferLength_);\n> }\n> \n> - measuredFrames_ = 0;\n> + encounteredFrames_ = 0;\n> frameProcessTime_ = 0;\n> \n> return 0;\n> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> {\n> timespec frameStartTime;\n> \n> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n> + bool measure = framesToMeasure_ > 0 &&\n\nSince framesToMeasure_ is unsigned, won't this part always be true?\n\nReviewed-by: Paul Elder \n\n> + encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&\n> + ++encounteredFrames_ > skipBeforeMeasure_;\n> + if (measure) {\n> frameStartTime = {};\n> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> }\n> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> dmaSyncers.clear();\n> \n> /* Measure before emitting signals */\n> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> - ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> + if (measure) {\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> + if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {\n> LOG(Debayer, Info)\n> - << \"Processed \" << measuredFrames\n> + << \"Processed \" << framesToMeasure_\n> << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> - << frameProcessTime_ / (1000 * measuredFrames)\n> + << frameProcessTime_ / (1000 * framesToMeasure_)\n> << \" us/frame\";\n> }\n> }\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> unsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> bool enableInputMemcpy_;\n> bool swapRedBlueGains_;\n> - unsigned int measuredFrames_;\n> + unsigned int encounteredFrames_;\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> + unsigned int skipBeforeMeasure_ = 30;\n> + unsigned int framesToMeasure_ = 30;\n> };\n> \n> } /* namespace libcamera */\n> -- \n> 2.50.1\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 F1C1DC324E\n\tfor ;\n\tTue, 9 Sep 2025 09:39:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 14AD869361;\n\tTue, 9 Sep 2025 11:39:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4335669339\n\tfor ;\n\tTue, 9 Sep 2025 11:39:11 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:6f3a:4f34:f1fa:8b3])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id AC4C9605;\n\tTue, 9 Sep 2025 11:37:57 +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=\"urGsg2rl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1757410678;\n\tbh=AdjKQa6IXlSsdoskJQeTQ/krJI0E7u1sQE+hg/tEjrY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=urGsg2rl4a3W6QipJoo0Z6fQ9Kg5GnNU3/cWq1+Y9xxg9JXlsy1bJ4/OllS+aVQEK\n\t8M4/UiWoYyYs7z35Js5w7Lf+VG3JNrTtDjQsMORtwGI4Y35hs0cSs4/wz5+VySIVoc\n\tGCsBA0GRdPMv1VktDfxwQ541StzYQ/wZmYeHmghk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250729073201.5369-11-mzamazal@redhat.com>","References":"<20250729073201.5369-1-mzamazal@redhat.com>\n\t<20250729073201.5369-11-mzamazal@redhat.com>","Subject":"Re: [PATCH v16 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","From":"Paul Elder ","Cc":"Milan Zamazal , Kieran Bingham\n\t, =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?=\n\t, Laurent Pinchart\n\t","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Sep 2025 18:39:04 +0900","Message-ID":"<175741074480.2127323.7349577929317341989@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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":35744,"web_url":"https://patchwork.libcamera.org/comment/35744/","msgid":"<854itbrbvp.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-09-09T13:31:06","subject":"Re: [PATCH v16 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Paul,\n\nthank you for review.\n\nPaul Elder writes:\n\n> Quoting Milan Zamazal (2025-07-29 16:31:58)\n>> Software ISP performs performance measurement on certain part of initial\n>> frames. Let's make this range configurable.\n>\n>> \n>> For this purpose, this patch introduces new configuration options\n>> pipelines.simple.measure.skip and pipelines.simple.measure.number.\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>> Signed-off-by: Milan Zamazal \n>> ---\n>> Documentation/runtime_configuration.rst | 6 ++++++\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, 25 insertions(+), 38 deletions(-)\n>> \n>> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst\n>> index 57285cadb..0708302d4 100644\n>> --- a/Documentation/runtime_configuration.rst\n>> +++ b/Documentation/runtime_configuration.rst\n>> @@ -53,6 +53,9 @@ file structure:\n>> ...\n>> simple:\n>> copy_input_buffer: # true/false\n>> + measure:\n>> + skip: # non-negative integer, frames to skip initially\n>> + number: # non-negative integer, frames to measure\n>\n> We might want to upgrade the documentation for the configuration file in the\n> future, but that can be done on later.\n\nBetter now, I'll add it.\n\n>> supported_devices:\n>> - driver: # driver name, e.g. `mxc-isi`\n>> software_isp: # true/false\n>> @@ -87,6 +90,9 @@ Configuration file example\n>> min_total_unicam_buffers: 2\n>> simple:\n>> copy_input_buffer: false\n>> + measure:\n>> + skip: 50\n>> + number: 30\n>> supported_devices:\n>> - driver: mxc-isi\n>> software_isp: true\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>> -> 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>> -\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 704d4e487..d43687f75 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>> enableInputMemcpy_ =\n>> configuration.option({ \"pipelines\", \"simple\", \"copy_input_buffer\" }).value_or(true);\n>> \n>> + skipBeforeMeasure_ = configuration.option(\n>> + { \"pipelines\", \"simple\", \"measure\", \"skip\" })\n>> + .value_or(skipBeforeMeasure_);\n>> + framesToMeasure_ = configuration.option(\n>> + { \"pipelines\", \"simple\", \"measure\", \"number\" })\n>> + .value_or(framesToMeasure_);\n>> +\n>> /* Initialize color lookup tables */\n>> for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> red_[i] = green_[i] = blue_[i] = i;\n>> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>> lineBuffers_[i].resize(lineBufferLength_);\n>> }\n>> \n>> - measuredFrames_ = 0;\n>> + encounteredFrames_ = 0;\n>> frameProcessTime_ = 0;\n>> \n>> return 0;\n>> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>> {\n>> timespec frameStartTime;\n>> \n>> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n>> + bool measure = framesToMeasure_ > 0 &&\n>\n> Since framesToMeasure_ is unsigned, won't this part always be true?\n\nIt can be zero, which disables the measurement. In which case the first\npart of the condition is still redundant (the other parts produce false)\nbut I think it's better to keep it there for clarity.\n\n> Reviewed-by: Paul Elder \n>\n>> + encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&\n>> + ++encounteredFrames_ > skipBeforeMeasure_;\n>> + if (measure) {\n>> frameStartTime = {};\n>> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n>> }\n>> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>> dmaSyncers.clear();\n>> \n>> /* Measure before emitting signals */\n>> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>> - ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n>> + if (measure) {\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>> + if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {\n>> LOG(Debayer, Info)\n>> - << \"Processed \" << measuredFrames\n>> + << \"Processed \" << framesToMeasure_\n>> << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n>> - << frameProcessTime_ / (1000 * measuredFrames)\n>> + << frameProcessTime_ / (1000 * framesToMeasure_)\n>> << \" us/frame\";\n>> }\n>> }\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>> unsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n>> bool enableInputMemcpy_;\n>> bool swapRedBlueGains_;\n>> - unsigned int measuredFrames_;\n>> + unsigned int encounteredFrames_;\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>> + unsigned int skipBeforeMeasure_ = 30;\n>> + unsigned int framesToMeasure_ = 30;\n>> };\n>> \n>> } /* namespace libcamera */\n>> -- \n>> 2.50.1\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 D27BDC324E\n\tfor ;\n\tTue, 9 Sep 2025 13:31:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E1C669367;\n\tTue, 9 Sep 2025 15:31:16 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 68E876934B\n\tfor ;\n\tTue, 9 Sep 2025 15:31:14 +0200 (CEST)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-382-oviqoXA0M5W8G_7OVJu5Ug-1; Tue, 09 Sep 2025 09:31:10 -0400","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-45b990eb77cso39771405e9.0\n\tfor ;\n\tTue, 09 Sep 2025 06:31:09 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb ([85.93.96.130])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-45de16b8b58sm139156855e9.4.2025.09.09.06.31.07\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 09 Sep 2025 06:31: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=\"AIyuplN3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1757424673;\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=ZhtXY+cIAoFY/1tif44zHuDpVY7+QF/t2XIbbP9ByZg=;\n\tb=AIyuplN37c4rlA3c0/kKu3Y1JjpBtzckkzBpQ0y83JggUEuuEvzscNkqQ9QGfgcmQe1mac\n\t3QQgWzJkWtWQUNBHxEnj1hGQo23nMSraGnhWcwlnhWzElSK/uf0QknW37dw2SIQhPkLkEq\n\t6rGROEJMuOOVFMhvekgs39m8cLBxD10=","X-MC-Unique":"oviqoXA0M5W8G_7OVJu5Ug-1","X-Mimecast-MFC-AGG-ID":"oviqoXA0M5W8G_7OVJu5Ug_1757424669","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1757424669; x=1758029469;\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=ZhtXY+cIAoFY/1tif44zHuDpVY7+QF/t2XIbbP9ByZg=;\n\tb=wvcWPIZZchHW89xMbFNXjlE+plZrTkb++Mhk4k9gxLhFFtGZkNZKB0X0t1bDWs9H01\n\tRAHEu83pC/h736Roz4CZ/13od3JTi5MA36VX//rZSrFMG5cvHxhcRF1PRhk3v3iZJiY7\n\tG6AeZ1KBOuTg8qQQSdiRwwNs4I3aYDXk6E+PSbpXWfRS/76yvzEsUNUyUkoDK0uxfU0E\n\tTwNIzAqMrPYwuap6nE4UfQk0kuriOhr8NUo57/HwYdxRm2oLBbBWc9Uai7icC/LOvcMd\n\teWMUjz3T/KlL737JUGRfQv5lKOnTIYCteEn3JpsYX4mSfHp79ZlCUNKlPmY+OAM4DHal\n\tpsvA==","X-Gm-Message-State":"AOJu0YwIDqgG7bs/iZzRG4IfgPfnjSOPLTfromRaJLAv9WoGSAAUzNXf\n\trx63NduUWrziR8fZPtHB3xEbFPYvvB74lR+wCszqIcZiKlkuzr7hpJnd/tJIO3G9wKW3UCYs3Or\n\t5MBQdQEbRLr+XEHUKp5WdVv8xlocTidnxeOUEzWt8wXXmTNq1ZCfu+525JZ1h92UdiNqAnyuhMJ\n\tw=","X-Gm-Gg":"ASbGncukMn9HSfXEm0zVZF5TsI0Iirxxchurf42v/QSwh2f6qdlbEuxsdIG04Pz/Fjg\n\t2/GKaDfLh/EF5YoZwrsOoWjVVD1gTZmfzjXblC/dhV+P3hT38tSdq0J0fmqV/vpRDUEq4g2/t4j\n\tlz1SP+6fR7ZzfDr9k5OILElqeZbzcqwO2ZPANZ0wkehpSBQv1h0OpQQlG8mgRqPRNdjiPxXVM92\n\tw5Uwf504p9yH6qPL8S3Sa1oBISfaZus+lUtGUqlx+ANpknihFYgFd6ydZE4QkWDl0wodigq7mn4\n\t9cbXg7rnK4JUrDkRtFDuuGe6/Fjdm+Tgblw/o2/ON+dXwTemqRrkACrYtpRexLxtUQ==","X-Received":["by 2002:a05:600c:840f:b0:45d:dc10:a5ee with SMTP id\n\t5b1f17b1804b1-45dde20ee09mr108099045e9.15.1757424668710; \n\tTue, 09 Sep 2025 06:31:08 -0700 (PDT)","by 2002:a05:600c:840f:b0:45d:dc10:a5ee with SMTP id\n\t5b1f17b1804b1-45dde20ee09mr108098635e9.15.1757424668164; \n\tTue, 09 Sep 2025 06:31:08 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFuh0HTJ6udLfsUY6yQUBbI3TNDg+JqJyP43uoS40kC70GD0Gm4I5jxvOpo0c+1U6B5SDYANw==","From":"Milan Zamazal ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham , =?utf-8?q?Barnab?=\n\t=?utf-8?b?w6FzIFDFkWN6ZQ==?= ,\n\tLaurent Pinchart ","Subject":"Re: [PATCH v16 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","In-Reply-To":"<175741074480.2127323.7349577929317341989@neptunite.rasen.tech>\n\t(Paul Elder's message of \"Tue, 09 Sep 2025 18:39:04 +0900\")","References":"<20250729073201.5369-1-mzamazal@redhat.com>\n\t<20250729073201.5369-11-mzamazal@redhat.com>\n\t<175741074480.2127323.7349577929317341989@neptunite.rasen.tech>","Date":"Tue, 09 Sep 2025 15:31:06 +0200","Message-ID":"<854itbrbvp.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"F2v0ExUBv-g5DUlZjYeSNumeraC-PQ4G1sgLdT_ShvE_1757424669","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":35754,"web_url":"https://patchwork.libcamera.org/comment/35754/","msgid":"<175742889567.2127323.13012794963397411319@neptunite.rasen.tech>","date":"2025-09-09T14:41:35","subject":"Re: [PATCH v16 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-09-09 22:31:06)\n> Hi Paul,\n> \n> thank you for review.\n> \n> Paul Elder writes:\n> \n> > Quoting Milan Zamazal (2025-07-29 16:31:58)\n> >> Software ISP performs performance measurement on certain part of initial\n> >> frames. Let's make this range configurable.\n> >\n> >> \n> >> For this purpose, this patch introduces new configuration options\n> >> pipelines.simple.measure.skip and pipelines.simple.measure.number.\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> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> Documentation/runtime_configuration.rst | 6 ++++++\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, 25 insertions(+), 38 deletions(-)\n> >> \n> >> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst\n> >> index 57285cadb..0708302d4 100644\n> >> --- a/Documentation/runtime_configuration.rst\n> >> +++ b/Documentation/runtime_configuration.rst\n> >> @@ -53,6 +53,9 @@ file structure:\n> >> ...\n> >> simple:\n> >> copy_input_buffer: # true/false\n> >> + measure:\n> >> + skip: # non-negative integer, frames to skip initially\n> >> + number: # non-negative integer, frames to measure\n> >\n> > We might want to upgrade the documentation for the configuration file in the\n> > future, but that can be done on later.\n> \n> Better now, I'll add it.\n\nI meant something more verbose like what we have for controls, though I am\ncurious what you had in mind.\n\n> \n> >> supported_devices:\n> >> - driver: # driver name, e.g. `mxc-isi`\n> >> software_isp: # true/false\n> >> @@ -87,6 +90,9 @@ Configuration file example\n> >> min_total_unicam_buffers: 2\n> >> simple:\n> >> copy_input_buffer: false\n> >> + measure:\n> >> + skip: 50\n> >> + number: 30\n> >> supported_devices:\n> >> - driver: mxc-isi\n> >> software_isp: true\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> >> -> 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> >> -\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 704d4e487..d43687f75 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> >> enableInputMemcpy_ =\n> >> configuration.option({ \"pipelines\", \"simple\", \"copy_input_buffer\" }).value_or(true);\n> >> \n> >> + skipBeforeMeasure_ = configuration.option(\n> >> + { \"pipelines\", \"simple\", \"measure\", \"skip\" })\n> >> + .value_or(skipBeforeMeasure_);\n> >> + framesToMeasure_ = configuration.option(\n> >> + { \"pipelines\", \"simple\", \"measure\", \"number\" })\n> >> + .value_or(framesToMeasure_);\n> >> +\n> >> /* Initialize color lookup tables */\n> >> for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> >> red_[i] = green_[i] = blue_[i] = i;\n> >> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> >> lineBuffers_[i].resize(lineBufferLength_);\n> >> }\n> >> \n> >> - measuredFrames_ = 0;\n> >> + encounteredFrames_ = 0;\n> >> frameProcessTime_ = 0;\n> >> \n> >> return 0;\n> >> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> >> {\n> >> timespec frameStartTime;\n> >> \n> >> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n> >> + bool measure = framesToMeasure_ > 0 &&\n> >\n> > Since framesToMeasure_ is unsigned, won't this part always be true?\n> \n> It can be zero, which disables the measurement. In which case the first\n> part of the condition is still redundant (the other parts produce false)\n> but I think it's better to keep it there for clarity.\n\nOh, good point.\n\n\nPaul\n\n> \n> > Reviewed-by: Paul Elder \n> >\n> >> + encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&\n> >> + ++encounteredFrames_ > skipBeforeMeasure_;\n> >> + if (measure) {\n> >> frameStartTime = {};\n> >> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> >> }\n> >> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> >> dmaSyncers.clear();\n> >> \n> >> /* Measure before emitting signals */\n> >> - if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> >> - ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> >> + if (measure) {\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> >> + if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {\n> >> LOG(Debayer, Info)\n> >> - << \"Processed \" << measuredFrames\n> >> + << \"Processed \" << framesToMeasure_\n> >> << \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> >> - << frameProcessTime_ / (1000 * measuredFrames)\n> >> + << frameProcessTime_ / (1000 * framesToMeasure_)\n> >> << \" us/frame\";\n> >> }\n> >> }\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> >> unsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> >> bool enableInputMemcpy_;\n> >> bool swapRedBlueGains_;\n> >> - unsigned int measuredFrames_;\n> >> + unsigned int encounteredFrames_;\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> >> + unsigned int skipBeforeMeasure_ = 30;\n> >> + unsigned int framesToMeasure_ = 30;\n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> -- \n> >> 2.50.1\n> >>\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 62C29BDB13\n\tfor ;\n\tTue, 9 Sep 2025 14:41:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1FF326936D;\n\tTue, 9 Sep 2025 16:41:43 +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 BB10F6934B\n\tfor ;\n\tTue, 9 Sep 2025 16:41:41 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:6f3a:4f34:f1fa:8b3])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id E96F25B3;\n\tTue, 9 Sep 2025 16:40:27 +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=\"kBst5PNI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1757428828;\n\tbh=dGdBx4TbCfcnpSV+sDwH/6XO2sb3k6+gCujRuCrJvoU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=kBst5PNIqxlCyY0vzHW/jNRPNJCqL+DXm7CFo/PL6KcFfA6r9uzFYRRRCL7PtG9su\n\tN4HiQIIlR4FedDRBmG2TTpQJIuuzLBOQQAyyhnczqP3DV13sUOlXIaEo9B6rFhSfwW\n\to6zlt5RUDmtBZaakvNTx4Lca0OmAqs293OoNodNk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<854itbrbvp.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","References":"<20250729073201.5369-1-mzamazal@redhat.com>\n\t<20250729073201.5369-11-mzamazal@redhat.com>\n\t<175741074480.2127323.7349577929317341989@neptunite.rasen.tech>\n\t<854itbrbvp.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Subject":"Re: [PATCH v16 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","From":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?=\n\t, Laurent Pinchart\n\t","To":"Milan Zamazal ","Date":"Tue, 09 Sep 2025 23:41:35 +0900","Message-ID":"<175742889567.2127323.13012794963397411319@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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\" "}}]