[{"id":26548,"web_url":"https://patchwork.libcamera.org/comment/26548/","msgid":"<20230305113724.u44o4dy3guhjnq46@uno.localdomain>","date":"2023-03-05T11:37:24","subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Thu, Mar 02, 2023 at 01:54:28PM +0000, Naushir Patuck via libcamera-devel wrote:\n> Date: Thu,  2 Mar 2023 13:54:28 +0000\n> From: Naushir Patuck <naush@raspberrypi.com>\n> To: libcamera-devel@lists.libcamera.org\n> Subject: [PATCH v2 2/3] ipa: raspberrypi: Better heruistics for calculating\n>  Unicam timeout\n> X-Mailer: git-send-email 2.34.1\n>\n> The existing mechanism of setting a timeout value simply uses the\n> maximum possible frame length advertised by the sensor mode. This can be\n> problematic when, for example, the IMX477 sensor can use a frame length\n> of over 600 seconds. However, for typical usage the frame length will\n> never go over several 100s of milliseconds, making the timeout very\n> impractical.\n>\n> Store a list of the last 10 frame length values requested by the AGC. On\n> startup, and at the end of every frame, take the maximum frame length\n> value from this list and return that to the pipeline handler through the\n> setCameraTimeoutValue() signal. This allows the timeout value to better\n> track the actual sensor usage.\n>\n> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n\nReviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\n> ---\n>  src/ipa/raspberrypi/raspberrypi.cpp | 44 +++++++++++++++++++++++++++--\n>  1 file changed, 41 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f6826bf27fe1..7b5aed644a67 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -8,6 +8,7 @@\n>  #include <algorithm>\n>  #include <array>\n>  #include <cstring>\n> +#include <deque>\n>  #include <fcntl.h>\n>  #include <math.h>\n>  #include <stdint.h>\n> @@ -64,6 +65,9 @@ using utils::Duration;\n>  /* Number of metadata objects available in the context list. */\n>  constexpr unsigned int numMetadataContexts = 16;\n>\n> +/* Number of frame length times to hold in the queue. */\n> +constexpr unsigned int FrameLengthsQueueSize = 10;\n> +\n>  /* Configure the sensor with these values initially. */\n>  constexpr double defaultAnalogueGain = 1.0;\n>  constexpr Duration defaultExposureTime = 20.0ms;\n> @@ -121,7 +125,8 @@ class IPARPi : public IPARPiInterface\n>  public:\n>  \tIPARPi()\n>  \t\t: controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0),\n> -\t\t  lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true)\n> +\t\t  lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true),\n> +\t\t  lastTimeout_(0s)\n>  \t{\n>  \t}\n>\n> @@ -155,6 +160,7 @@ private:\n>  \tvoid fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n>  \tRPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n>  \tvoid processStats(unsigned int bufferId, unsigned int ipaContext);\n> +\tvoid setCameraTimeoutValue();\n>  \tvoid applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);\n>  \tvoid applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n>  \tvoid applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> @@ -220,6 +226,10 @@ private:\n>\n>  \t/* Maximum gain code for the sensor. */\n>  \tuint32_t maxSensorGainCode_;\n> +\n> +\t/* Track the frame length times over FrameLengthsQueueSize frames. */\n> +\tstd::deque<Duration> frameLengths_;\n> +\tDuration lastTimeout_;\n>  };\n>\n>  int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> @@ -284,6 +294,10 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n>\n>  \tcontroller_.switchMode(mode_, &metadata);\n>\n> +\t/* Reset the frame lengths queue */\n> +\tframeLengths_.clear();\n> +\tframeLengths_.resize(FrameLengthsQueueSize, 0s);\n> +\n>  \t/* SwitchMode may supply updated exposure/gain values to use. */\n>  \tAgcStatus agcStatus;\n>  \tagcStatus.shutterTime = 0.0s;\n> @@ -294,6 +308,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n>  \t\tControlList ctrls(sensorCtrls_);\n>  \t\tapplyAGC(&agcStatus, ctrls);\n>  \t\tstartConfig->controls = std::move(ctrls);\n> +\t\tsetCameraTimeoutValue();\n>  \t}\n>\n>  \t/*\n> @@ -340,8 +355,6 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n>  \t}\n>\n>  \tstartConfig->dropFrameCount = dropFrameCount_;\n> -\tconst Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> -\tsetCameraTimeout.emit(maxSensorFrameDuration.get<std::milli>());\n>\n>  \tfirstStart_ = false;\n>  \tlastRunTimestamp_ = 0;\n> @@ -1434,6 +1447,22 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n>  \t\tapplyAGC(&agcStatus, ctrls);\n>\n>  \t\tsetDelayedControls.emit(ctrls, ipaContext);\n> +\t\tsetCameraTimeoutValue();\n> +\t}\n> +}\n> +\n> +void IPARPi::setCameraTimeoutValue()\n> +{\n> +\t/*\n> +\t * Take the maximum value of the exposure queue as the camera timeout\n> +\t * value to pass back to the pipeline handler. Only signal if it has changed\n> +\t * from the last set value.\n> +\t */\n> +\tauto max = std::max_element(frameLengths_.begin(), frameLengths_.end());\n> +\n> +\tif (*max != lastTimeout_) {\n> +\t\tsetCameraTimeout.emit(max->get<std::milli>());\n> +\t\tlastTimeout_ = *max;\n>  \t}\n>  }\n>\n> @@ -1522,6 +1551,15 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n>  \t */\n>  \tif (mode_.minLineLength != mode_.maxLineLength)\n>  \t\tctrls.set(V4L2_CID_HBLANK, static_cast<int32_t>(hblank));\n> +\n> +\t/*\n> +\t * Store the frame length times in a circular queue, up-to FrameLengthsQueueSize\n> +\t * elements. This will be used to advertise a camera timeout value to the\n> +\t * pipeline handler.\n> +\t */\n> +\tframeLengths_.pop_front();\n> +\tframeLengths_.push_back(helper_->exposure(vblank + mode_.height,\n> +\t\t\t\t\t\t  helper_->hblankToLineLength(hblank)));\n>  }\n>\n>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> --\n> 2.34.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 CBD03BDE17\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun,  5 Mar 2023 11:37:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 030066265A;\n\tSun,  5 Mar 2023 12:37:30 +0100 (CET)","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 901206261A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  5 Mar 2023 12:37:28 +0100 (CET)","from ideasonboard.com (host-79-47-54-87.retail.telecomitalia.it\n\t[79.47.54.87])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E3C31904;\n\tSun,  5 Mar 2023 12:37:27 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678016250;\n\tbh=Ufc8SYN1H0McsQMhclT0K6w+CspJLc6r5WuCX3dQpOs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=RgxVLGPj/ku1Nx02kv7O2Ma/3RWQ49oxJM3Ub8NqxmU+OxDZbFbvNcMRYkwn7KnyA\n\th+VonCSpmmEY6u7CTnAPHJZET2SDEu1m0BPb1k7FBEACCKExg59T3O6nE7NewfHtlT\n\tJuZhN5L9vMHTeKvuDxlZPhznCIt1msXVT+PEmeauz7dn2zVXJXglaT/T3oJwxU0t8Y\n\tanOsIuBCxsv5KYOAZnql1wBOggpUOS9Fvc3Z7+7XApTZLUbHd45qzkf1arfF9IrPmG\n\tLlk47V0F8bWlW+qVc5HbDJI3XPcQeVAviRFk/0Se2atfYb4ee2STs9AHEhTY7M48lx\n\ttLqu+1INBMYbA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1678016248;\n\tbh=Ufc8SYN1H0McsQMhclT0K6w+CspJLc6r5WuCX3dQpOs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AbaQ9vNDS0xam5ox6ltVh6J4S0Mhav1VxChnfXon9kRmmZmsfcEJSGEdCzGYd34Oj\n\twJU3TQhwQyYoT3XPBuiI0K7qwBkqds7U+N6kREs9GBfyVjdhk7tsFrVIZzFy2rd406\n\tvG5Wjcs3tGsJvpsOq++QIg8dAZSnE15Kurb9MLpc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AbaQ9vND\"; dkim-atps=neutral","Date":"Sun, 5 Mar 2023 12:37:24 +0100","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<20230305113724.u44o4dy3guhjnq46@uno.localdomain>","References":"<20230302135429.23821-1-naush@raspberrypi.com>\n\t<mailman.57.1677765273.25031.libcamera-devel@lists.libcamera.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<mailman.57.1677765273.25031.libcamera-devel@lists.libcamera.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26565,"web_url":"https://patchwork.libcamera.org/comment/26565/","msgid":"<20230306172522.GA27186@pendragon.ideasonboard.com>","date":"2023-03-06T17:25:22","subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Thu, Mar 02, 2023 at 01:54:28PM +0000, Naushir Patuck via libcamera-devel wrote:\n> \n> The existing mechanism of setting a timeout value simply uses the\n> maximum possible frame length advertised by the sensor mode. This can be\n> problematic when, for example, the IMX477 sensor can use a frame length\n> of over 600 seconds. However, for typical usage the frame length will\n> never go over several 100s of milliseconds, making the timeout very\n> impractical.\n> \n> Store a list of the last 10 frame length values requested by the AGC. On\n> startup, and at the end of every frame, take the maximum frame length\n> value from this list and return that to the pipeline handler through the\n> setCameraTimeoutValue() signal. This allows the timeout value to better\n> track the actual sensor usage.\n> \n> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> ---\n>  src/ipa/raspberrypi/raspberrypi.cpp | 44 +++++++++++++++++++++++++++--\n>  1 file changed, 41 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f6826bf27fe1..7b5aed644a67 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -8,6 +8,7 @@\n>  #include <algorithm>\n>  #include <array>\n>  #include <cstring>\n> +#include <deque>\n>  #include <fcntl.h>\n>  #include <math.h>\n>  #include <stdint.h>\n> @@ -64,6 +65,9 @@ using utils::Duration;\n>  /* Number of metadata objects available in the context list. */\n>  constexpr unsigned int numMetadataContexts = 16;\n>  \n> +/* Number of frame length times to hold in the queue. */\n> +constexpr unsigned int FrameLengthsQueueSize = 10;\n> +\n>  /* Configure the sensor with these values initially. */\n>  constexpr double defaultAnalogueGain = 1.0;\n>  constexpr Duration defaultExposureTime = 20.0ms;\n> @@ -121,7 +125,8 @@ class IPARPi : public IPARPiInterface\n>  public:\n>  \tIPARPi()\n>  \t\t: controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0),\n> -\t\t  lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true)\n> +\t\t  lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true),\n> +\t\t  lastTimeout_(0s)\n>  \t{\n>  \t}\n>  \n> @@ -155,6 +160,7 @@ private:\n>  \tvoid fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n>  \tRPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n>  \tvoid processStats(unsigned int bufferId, unsigned int ipaContext);\n> +\tvoid setCameraTimeoutValue();\n>  \tvoid applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);\n>  \tvoid applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n>  \tvoid applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> @@ -220,6 +226,10 @@ private:\n>  \n>  \t/* Maximum gain code for the sensor. */\n>  \tuint32_t maxSensorGainCode_;\n> +\n> +\t/* Track the frame length times over FrameLengthsQueueSize frames. */\n> +\tstd::deque<Duration> frameLengths_;\n\nAnother note to self: create a generic ring buffer class.\n\n> +\tDuration lastTimeout_;\n>  };\n>  \n>  int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> @@ -284,6 +294,10 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n>  \n>  \tcontroller_.switchMode(mode_, &metadata);\n>  \n> +\t/* Reset the frame lengths queue */\n\ns/queue/queue./\n\n> +\tframeLengths_.clear();\n> +\tframeLengths_.resize(FrameLengthsQueueSize, 0s);\n\nShould you also reset lastTimeout_ to 0s to ensure a consistent\nbehaviour at start time ?\n\n> +\n>  \t/* SwitchMode may supply updated exposure/gain values to use. */\n>  \tAgcStatus agcStatus;\n>  \tagcStatus.shutterTime = 0.0s;\n> @@ -294,6 +308,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n>  \t\tControlList ctrls(sensorCtrls_);\n>  \t\tapplyAGC(&agcStatus, ctrls);\n>  \t\tstartConfig->controls = std::move(ctrls);\n> +\t\tsetCameraTimeoutValue();\n>  \t}\n>  \n>  \t/*\n> @@ -340,8 +355,6 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n>  \t}\n>  \n>  \tstartConfig->dropFrameCount = dropFrameCount_;\n> -\tconst Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> -\tsetCameraTimeout.emit(maxSensorFrameDuration.get<std::milli>());\n>  \n>  \tfirstStart_ = false;\n>  \tlastRunTimestamp_ = 0;\n> @@ -1434,6 +1447,22 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n\nAdding more context:\n\n\tif (agcStatus.shutterTime && agcStatus.analogueGain) {\n\t\tControlList ctrls(sensorCtrls_);\n\n>  \t\tapplyAGC(&agcStatus, ctrls);\n>  \n>  \t\tsetDelayedControls.emit(ctrls, ipaContext);\n> +\t\tsetCameraTimeoutValue();\n\nIf the above condition is false, setCameraTimeoutValue() won't be\ncalled. When can that happen, and could it cause any issue ?\n\n> +\t}\n> +}\n> +\n> +void IPARPi::setCameraTimeoutValue()\n> +{\n> +\t/*\n> +\t * Take the maximum value of the exposure queue as the camera timeout\n> +\t * value to pass back to the pipeline handler. Only signal if it has changed\n\nLine wrap.\n\n> +\t * from the last set value.\n> +\t */\n> +\tauto max = std::max_element(frameLengths_.begin(), frameLengths_.end());\n> +\n> +\tif (*max != lastTimeout_) {\n> +\t\tsetCameraTimeout.emit(max->get<std::milli>());\n> +\t\tlastTimeout_ = *max;\n>  \t}\n>  }\n>  \n> @@ -1522,6 +1551,15 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n>  \t */\n>  \tif (mode_.minLineLength != mode_.maxLineLength)\n>  \t\tctrls.set(V4L2_CID_HBLANK, static_cast<int32_t>(hblank));\n> +\n> +\t/*\n> +\t * Store the frame length times in a circular queue, up-to FrameLengthsQueueSize\n> +\t * elements. This will be used to advertise a camera timeout value to the\n> +\t * pipeline handler.\n\nLine wraps here too.\n\n> +\t */\n> +\tframeLengths_.pop_front();\n> +\tframeLengths_.push_back(helper_->exposure(vblank + mode_.height,\n> +\t\t\t\t\t\t  helper_->hblankToLineLength(hblank)));\n>  }\n>  \n>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 34428BE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  6 Mar 2023 17:25:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD524626A5;\n\tMon,  6 Mar 2023 18:25:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A3D05603B1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  6 Mar 2023 18:25:19 +0100 (CET)","from pendragon.ideasonboard.com\n\t(153.162-64-87.adsl-dyn.isp.belgacom.be [87.64.162.153])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 458A4308;\n\tMon,  6 Mar 2023 18:25:19 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678123520;\n\tbh=xd8nZ0zovzE3jYGxzMYYFom2FP0wTdS+4XYq/YCs7ZM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=dF3lpxMfbvc7qA2mLGoA1TS1DIzxo7glhR8wY7q8MTaMb7BpX8L5YwCEGvUORCOi2\n\tS5EjFwItOvOrwnS3Zfe6/kMGNdo1oXCNHKBZHKSVEXrEjRvt+HbG2+7EtrCq4ZttmE\n\tditNm3UDgULINcTxTdWdZ0LjBQ7MviQeL26RCRjYR0oTK9qOpyoOpeynMGp1hKLv/2\n\tZcBvDnsx0FTgT+CSpvBc4vK2MAqijsm5RN2DtW+HS9R67is5fRZFrIky547lUCfZ2v\n\tqDdEgJ8VJ+s4gkMceVKwgN8O9NV4xCfsY3bdFBL1c+yx3bLO0GIq2KZ+sj2StGFTDL\n\ttI0K25k5Tuw+Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1678123519;\n\tbh=xd8nZ0zovzE3jYGxzMYYFom2FP0wTdS+4XYq/YCs7ZM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hVovk5X9Rc04sylLUmOtfSh9ueGOCG5EG4u36KTyMxsec9zVSENUet980mJ9xFR15\n\t/0WK/YLeGSOHehYJ/LL0ATWfSZdYVa5AC/TpF0J+03P044wlh3mnyFgf8IC1QA/XCH\n\tlsZfKn/Nif1zzF3USo74AZy75YpHFHLC9zwNhxdo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"hVovk5X9\"; dkim-atps=neutral","Date":"Mon, 6 Mar 2023 19:25:22 +0200","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<20230306172522.GA27186@pendragon.ideasonboard.com>","References":"<20230302135429.23821-1-naush@raspberrypi.com>\n\t<mailman.57.1677765273.25031.libcamera-devel@lists.libcamera.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<mailman.57.1677765273.25031.libcamera-devel@lists.libcamera.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26570,"web_url":"https://patchwork.libcamera.org/comment/26570/","msgid":"<CAEmqJPpqPMbxP3oF4g-PUUmpGViSpLptr2cjy6kXJFoPU-Vt6A@mail.gmail.com>","date":"2023-03-07T08:45:44","subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\n\nOn Mon, 6 Mar 2023 at 17:25, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Thu, Mar 02, 2023 at 01:54:28PM +0000, Naushir Patuck via\n> libcamera-devel wrote:\n> >\n> > The existing mechanism of setting a timeout value simply uses the\n> > maximum possible frame length advertised by the sensor mode. This can be\n> > problematic when, for example, the IMX477 sensor can use a frame length\n> > of over 600 seconds. However, for typical usage the frame length will\n> > never go over several 100s of milliseconds, making the timeout very\n> > impractical.\n> >\n> > Store a list of the last 10 frame length values requested by the AGC. On\n> > startup, and at the end of every frame, take the maximum frame length\n> > value from this list and return that to the pipeline handler through the\n> > setCameraTimeoutValue() signal. This allows the timeout value to better\n> > track the actual sensor usage.\n> >\n> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> > ---\n> >  src/ipa/raspberrypi/raspberrypi.cpp | 44 +++++++++++++++++++++++++++--\n> >  1 file changed, 41 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index f6826bf27fe1..7b5aed644a67 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -8,6 +8,7 @@\n> >  #include <algorithm>\n> >  #include <array>\n> >  #include <cstring>\n> > +#include <deque>\n> >  #include <fcntl.h>\n> >  #include <math.h>\n> >  #include <stdint.h>\n> > @@ -64,6 +65,9 @@ using utils::Duration;\n> >  /* Number of metadata objects available in the context list. */\n> >  constexpr unsigned int numMetadataContexts = 16;\n> >\n> > +/* Number of frame length times to hold in the queue. */\n> > +constexpr unsigned int FrameLengthsQueueSize = 10;\n> > +\n> >  /* Configure the sensor with these values initially. */\n> >  constexpr double defaultAnalogueGain = 1.0;\n> >  constexpr Duration defaultExposureTime = 20.0ms;\n> > @@ -121,7 +125,8 @@ class IPARPi : public IPARPiInterface\n> >  public:\n> >       IPARPi()\n> >               : controller_(), frameCount_(0), checkCount_(0),\n> mistrustCount_(0),\n> > -               lastRunTimestamp_(0), lsTable_(nullptr),\n> firstStart_(true)\n> > +               lastRunTimestamp_(0), lsTable_(nullptr),\n> firstStart_(true),\n> > +               lastTimeout_(0s)\n> >       {\n> >       }\n> >\n> > @@ -155,6 +160,7 @@ private:\n> >       void fillDeviceStatus(const ControlList &sensorControls, unsigned\n> int ipaContext);\n> >       RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats\n> *stats) const;\n> >       void processStats(unsigned int bufferId, unsigned int ipaContext);\n> > +     void setCameraTimeoutValue();\n> >       void applyFrameDurations(Duration minFrameDuration, Duration\n> maxFrameDuration);\n> >       void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls);\n> >       void applyAWB(const struct AwbStatus *awbStatus, ControlList\n> &ctrls);\n> > @@ -220,6 +226,10 @@ private:\n> >\n> >       /* Maximum gain code for the sensor. */\n> >       uint32_t maxSensorGainCode_;\n> > +\n> > +     /* Track the frame length times over FrameLengthsQueueSize frames.\n> */\n> > +     std::deque<Duration> frameLengths_;\n>\n> Another note to self: create a generic ring buffer class.\n>\n> > +     Duration lastTimeout_;\n> >  };\n> >\n> >  int IPARPi::init(const IPASettings &settings, bool lensPresent,\n> IPAInitResult *result)\n> > @@ -284,6 +294,10 @@ void IPARPi::start(const ControlList &controls,\n> StartConfig *startConfig)\n> >\n> >       controller_.switchMode(mode_, &metadata);\n> >\n> > +     /* Reset the frame lengths queue */\n>\n> s/queue/queue./\n>\n> > +     frameLengths_.clear();\n> > +     frameLengths_.resize(FrameLengthsQueueSize, 0s);\n>\n> Should you also reset lastTimeout_ to 0s to ensure a consistent\n> behaviour at start time ?\n>\n\nGood catch, I will reset lastTimeout_ here.\n\n\n>\n> > +\n> >       /* SwitchMode may supply updated exposure/gain values to use. */\n> >       AgcStatus agcStatus;\n> >       agcStatus.shutterTime = 0.0s;\n> > @@ -294,6 +308,7 @@ void IPARPi::start(const ControlList &controls,\n> StartConfig *startConfig)\n> >               ControlList ctrls(sensorCtrls_);\n> >               applyAGC(&agcStatus, ctrls);\n> >               startConfig->controls = std::move(ctrls);\n> > +             setCameraTimeoutValue();\n> >       }\n> >\n> >       /*\n> > @@ -340,8 +355,6 @@ void IPARPi::start(const ControlList &controls,\n> StartConfig *startConfig)\n> >       }\n> >\n> >       startConfig->dropFrameCount = dropFrameCount_;\n> > -     const Duration maxSensorFrameDuration = mode_.maxFrameLength *\n> mode_.maxLineLength;\n> > -     setCameraTimeout.emit(maxSensorFrameDuration.get<std::milli>());\n> >\n> >       firstStart_ = false;\n> >       lastRunTimestamp_ = 0;\n> > @@ -1434,6 +1447,22 @@ void IPARPi::processStats(unsigned int bufferId,\n> unsigned int ipaContext)\n>\n> Adding more context:\n>\n>         if (agcStatus.shutterTime && agcStatus.analogueGain) {\n>                 ControlList ctrls(sensorCtrls_);\n>\n> >               applyAGC(&agcStatus, ctrls);\n> >\n> >               setDelayedControls.emit(ctrls, ipaContext);\n> > +             setCameraTimeoutValue();\n>\n> If the above condition is false, setCameraTimeoutValue() won't be\n> called. When can that happen, and could it cause any issue ?\n>\n\nShort answer is no.  Really the test is a bit redundant, agc can never ever\nreturn with agcStatus.shutterTime or agcStatus.analogueGain set to 0.  I\nought\nto remove that if () clause in a future tidy up.\n\nRegards,\nNaush\n\n\n>\n> > +     }\n> > +}\n> > +\n> > +void IPARPi::setCameraTimeoutValue()\n> > +{\n> > +     /*\n> > +      * Take the maximum value of the exposure queue as the camera\n> timeout\n> > +      * value to pass back to the pipeline handler. Only signal if it\n> has changed\n>\n> Line wrap.\n>\n> > +      * from the last set value.\n> > +      */\n> > +     auto max = std::max_element(frameLengths_.begin(),\n> frameLengths_.end());\n> > +\n> > +     if (*max != lastTimeout_) {\n> > +             setCameraTimeout.emit(max->get<std::milli>());\n> > +             lastTimeout_ = *max;\n> >       }\n> >  }\n> >\n> > @@ -1522,6 +1551,15 @@ void IPARPi::applyAGC(const struct AgcStatus\n> *agcStatus, ControlList &ctrls)\n> >        */\n> >       if (mode_.minLineLength != mode_.maxLineLength)\n> >               ctrls.set(V4L2_CID_HBLANK, static_cast<int32_t>(hblank));\n> > +\n> > +     /*\n> > +      * Store the frame length times in a circular queue, up-to\n> FrameLengthsQueueSize\n> > +      * elements. This will be used to advertise a camera timeout value\n> to the\n> > +      * pipeline handler.\n>\n> Line wraps here too.\n>\n> > +      */\n> > +     frameLengths_.pop_front();\n> > +     frameLengths_.push_back(helper_->exposure(vblank + mode_.height,\n> > +\n>  helper_->hblankToLineLength(hblank)));\n> >  }\n> >\n> >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList\n> &ctrls)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 099CFBDE17\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  7 Mar 2023 08:45:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80F8F626AE;\n\tTue,  7 Mar 2023 09:45:55 +0100 (CET)","from mail-yb1-xb33.google.com (mail-yb1-xb33.google.com\n\t[IPv6:2607:f8b0:4864:20::b33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 748E56266A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  7 Mar 2023 09:45:53 +0100 (CET)","by mail-yb1-xb33.google.com with SMTP id v13so10690455ybu.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 07 Mar 2023 00:45:53 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678178755;\n\tbh=J9ht+lncDP8sP9UgVWc8JhDhflZXmRKnwASFUU67Mp8=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=wt6es3UF9qHkdMi9Wsp0hg6wlusHUhT6id0XxwJJz//pNF9VIS7WyrrRcJj66kFyo\n\tnUYHXiHs+YxGbZhSGFhHJW44RpH5hfEMciREjG7qP7Jnx2GNXEvqsqhSgueXvxk4qi\n\tiUEPVgjzg5AQ/e889/mR2kHxIDQbZJBCAoUrMxKdwAI7Jhf5HUjvzhyG9pgv0Qspq/\n\tdPjDCVmdf54c1/whQsSZPMCIIjbLtk+g+Ntd7reRSVb9fXE4dGw/avb/4fxqCF71Pa\n\tnBESmqK7IhwraqFNsn8akyMcgTNeHJ9LCGEbBixU0elXexLPCrREnTmrSuckjV/AlD\n\tyrb/lps97SpYw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1678178752;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=sVTFeEQwWEPyH3c4esZcu5pe2WObcbRajX3oftbiemU=;\n\tb=s3s6HpKEbaylXUpVQiktaUMtoVaHFJ2Fiyk8yvUbLRUg1TvruuCWB2eO8SwGIbj9jZ\n\tB4Jm5YSgGRWiWKlWO3z6KFHLA3Zi2IvXFHw+8J7O8LtvBr0j18ld3ma6MuyRpyhWscRo\n\tz7RU8OJxjDrf3Vzeul36X/GLL+ujDWy8qwHLNpffzksC6Y7ag4ZHUF0QqA4VhZOy6qcv\n\tHAVGWUsPkXPojYU9oBi6mFzK+IM1LslFtm/hnbjgvUa5uPHMrGFYW7NrRDodzFTZyNOK\n\tufZHPLRID1j4zTyQj3rwAzHVu+uXPTQt6F7yHr+BZ6xRMFUSzwDTITWiVMnOGYAPQYQJ\n\tFPLQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"s3s6HpKE\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112; t=1678178752;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=sVTFeEQwWEPyH3c4esZcu5pe2WObcbRajX3oftbiemU=;\n\tb=ejTkHuMjVEwrSSlNd0itrsuVXtJFOxSFGNeNhlTh20ycyEBoaXyJNgSklUMPHtMWfg\n\tBhdBTSyc4UItGAwGGpdFqYh+HAFqviuHJdZ5cNa9XEtE0vGZijLa6Vgs00kTLqTHhuoq\n\txBRuwo0TnFGBPJqEvdzzU1YRIjnyXjpaoPxKAEmSVAL2//XK3wGfWen3XkJP/SrMCIFY\n\tBRbREGA8s+DxRI6ndUZZp4DMTPMDGCTpRroSntN5VgUH3blok0vpMuRNc/4aYuNI9bTd\n\tmWZUv1gU48giAX6U8pbvLfFkuKGgAQJBXgxfQ5zlNJJ+PanIDVY7hkaSF3TfrcoyVDad\n\tf7nw==","X-Gm-Message-State":"AO0yUKUm6uJGwZQfltTCXd/FtfLqsWHtJrVJC5yvieLqsyVDdNOWeSNW\n\tfbUHCl9LYpigW8C7TYXoWtohNMYnPIjVpl8Qd5jA+kwlBeSSaewJTsVdxQ==","X-Google-Smtp-Source":"AK7set+lG8xMRFoAAEnq/zu76TXCbi5Oqo6ttV9vIJGGtjokhBVfGBVWxSUiY3zRqZEvU+4GyGKLCMZWx/yZhNK87Cs=","X-Received":"by 2002:a5b:c08:0:b0:9fe:1493:8b8 with SMTP id\n\tf8-20020a5b0c08000000b009fe149308b8mr8292988ybq.6.1678178752249; \n\tTue, 07 Mar 2023 00:45:52 -0800 (PST)","MIME-Version":"1.0","References":"<20230302135429.23821-1-naush@raspberrypi.com>\n\t<mailman.57.1677765273.25031.libcamera-devel@lists.libcamera.org>\n\t<20230306172522.GA27186@pendragon.ideasonboard.com>","In-Reply-To":"<20230306172522.GA27186@pendragon.ideasonboard.com>","Date":"Tue, 7 Mar 2023 08:45:44 +0000","Message-ID":"<CAEmqJPpqPMbxP3oF4g-PUUmpGViSpLptr2cjy6kXJFoPU-Vt6A@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"00000000000065d78b05f64b704e\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26578,"web_url":"https://patchwork.libcamera.org/comment/26578/","msgid":"<20230307100143.GA22827@pendragon.ideasonboard.com>","date":"2023-03-07T10:01:43","subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nBtw, I just noticed the \"heruistics\" typo in the subject line.\n\nOn Tue, Mar 07, 2023 at 08:45:44AM +0000, Naushir Patuck wrote:\n> On Mon, 6 Mar 2023 at 17:25, Laurent Pinchart wrote:\n> \n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > On Thu, Mar 02, 2023 at 01:54:28PM +0000, Naushir Patuck via\n> > libcamera-devel wrote:\n> > >\n> > > The existing mechanism of setting a timeout value simply uses the\n> > > maximum possible frame length advertised by the sensor mode. This can be\n> > > problematic when, for example, the IMX477 sensor can use a frame length\n> > > of over 600 seconds. However, for typical usage the frame length will\n> > > never go over several 100s of milliseconds, making the timeout very\n> > > impractical.\n> > >\n> > > Store a list of the last 10 frame length values requested by the AGC. On\n> > > startup, and at the end of every frame, take the maximum frame length\n> > > value from this list and return that to the pipeline handler through the\n> > > setCameraTimeoutValue() signal. This allows the timeout value to better\n> > > track the actual sensor usage.\n> > >\n> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> > > ---\n> > >  src/ipa/raspberrypi/raspberrypi.cpp | 44 +++++++++++++++++++++++++++--\n> > >  1 file changed, 41 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index f6826bf27fe1..7b5aed644a67 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -8,6 +8,7 @@\n> > >  #include <algorithm>\n> > >  #include <array>\n> > >  #include <cstring>\n> > > +#include <deque>\n> > >  #include <fcntl.h>\n> > >  #include <math.h>\n> > >  #include <stdint.h>\n> > > @@ -64,6 +65,9 @@ using utils::Duration;\n> > >  /* Number of metadata objects available in the context list. */\n> > >  constexpr unsigned int numMetadataContexts = 16;\n> > >\n> > > +/* Number of frame length times to hold in the queue. */\n> > > +constexpr unsigned int FrameLengthsQueueSize = 10;\n> > > +\n> > >  /* Configure the sensor with these values initially. */\n> > >  constexpr double defaultAnalogueGain = 1.0;\n> > >  constexpr Duration defaultExposureTime = 20.0ms;\n> > > @@ -121,7 +125,8 @@ class IPARPi : public IPARPiInterface\n> > >  public:\n> > >       IPARPi()\n> > >               : controller_(), frameCount_(0), checkCount_(0), mistrustCount_(0),\n> > > -               lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true)\n> > > +               lastRunTimestamp_(0), lsTable_(nullptr), firstStart_(true),\n> > > +               lastTimeout_(0s)\n> > >       {\n> > >       }\n> > >\n> > > @@ -155,6 +160,7 @@ private:\n> > >       void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);\n> > >       RPiController::StatisticsPtr fillStatistics(bcm2835_isp_stats *stats) const;\n> > >       void processStats(unsigned int bufferId, unsigned int ipaContext);\n> > > +     void setCameraTimeoutValue();\n> > >       void applyFrameDurations(Duration minFrameDuration, Duration maxFrameDuration);\n> > >       void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> > >       void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> > > @@ -220,6 +226,10 @@ private:\n> > >\n> > >       /* Maximum gain code for the sensor. */\n> > >       uint32_t maxSensorGainCode_;\n> > > +\n> > > +     /* Track the frame length times over FrameLengthsQueueSize frames.\n> > */\n> > > +     std::deque<Duration> frameLengths_;\n> >\n> > Another note to self: create a generic ring buffer class.\n> >\n> > > +     Duration lastTimeout_;\n> > >  };\n> > >\n> > >  int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> > > @@ -284,6 +294,10 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > >\n> > >       controller_.switchMode(mode_, &metadata);\n> > >\n> > > +     /* Reset the frame lengths queue */\n> >\n> > s/queue/queue./\n> >\n> > > +     frameLengths_.clear();\n> > > +     frameLengths_.resize(FrameLengthsQueueSize, 0s);\n> >\n> > Should you also reset lastTimeout_ to 0s to ensure a consistent\n> > behaviour at start time ?\n> \n> Good catch, I will reset lastTimeout_ here.\n> \n> > > +\n> > >       /* SwitchMode may supply updated exposure/gain values to use. */\n> > >       AgcStatus agcStatus;\n> > >       agcStatus.shutterTime = 0.0s;\n> > > @@ -294,6 +308,7 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > >               ControlList ctrls(sensorCtrls_);\n> > >               applyAGC(&agcStatus, ctrls);\n> > >               startConfig->controls = std::move(ctrls);\n> > > +             setCameraTimeoutValue();\n> > >       }\n> > >\n> > >       /*\n> > > @@ -340,8 +355,6 @@ void IPARPi::start(const ControlList &controls, StartConfig *startConfig)\n> > >       }\n> > >\n> > >       startConfig->dropFrameCount = dropFrameCount_;\n> > > -     const Duration maxSensorFrameDuration = mode_.maxFrameLength * mode_.maxLineLength;\n> > > -     setCameraTimeout.emit(maxSensorFrameDuration.get<std::milli>());\n> > >\n> > >       firstStart_ = false;\n> > >       lastRunTimestamp_ = 0;\n> > > @@ -1434,6 +1447,22 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> >\n> > Adding more context:\n> >\n> >         if (agcStatus.shutterTime && agcStatus.analogueGain) {\n> >                 ControlList ctrls(sensorCtrls_);\n> >\n> > >               applyAGC(&agcStatus, ctrls);\n> > >\n> > >               setDelayedControls.emit(ctrls, ipaContext);\n> > > +             setCameraTimeoutValue();\n> >\n> > If the above condition is false, setCameraTimeoutValue() won't be\n> > called. When can that happen, and could it cause any issue ?\n> \n> Short answer is no.  Really the test is a bit redundant, agc can never ever\n> return with agcStatus.shutterTime or agcStatus.analogueGain set to 0. I ought\n> to remove that if () clause in a future tidy up.\n\nThat would be nice, thanks.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nwith the other issues fixed.\n\n> > > +     }\n> > > +}\n> > > +\n> > > +void IPARPi::setCameraTimeoutValue()\n> > > +{\n> > > +     /*\n> > > +      * Take the maximum value of the exposure queue as the camera timeout\n> > > +      * value to pass back to the pipeline handler. Only signal if it has changed\n> >\n> > Line wrap.\n> >\n> > > +      * from the last set value.\n> > > +      */\n> > > +     auto max = std::max_element(frameLengths_.begin(), frameLengths_.end());\n> > > +\n> > > +     if (*max != lastTimeout_) {\n> > > +             setCameraTimeout.emit(max->get<std::milli>());\n> > > +             lastTimeout_ = *max;\n> > >       }\n> > >  }\n> > >\n> > > @@ -1522,6 +1551,15 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> > >        */\n> > >       if (mode_.minLineLength != mode_.maxLineLength)\n> > >               ctrls.set(V4L2_CID_HBLANK, static_cast<int32_t>(hblank));\n> > > +\n> > > +     /*\n> > > +      * Store the frame length times in a circular queue, up-to FrameLengthsQueueSize\n> > > +      * elements. This will be used to advertise a camera timeout value to the\n> > > +      * pipeline handler.\n> >\n> > Line wraps here too.\n> >\n> > > +      */\n> > > +     frameLengths_.pop_front();\n> > > +     frameLengths_.push_back(helper_->exposure(vblank + mode_.height,\n> > > +                             helper_->hblankToLineLength(hblank)));\n> > >  }\n> > >\n> > >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6DC52BDE17\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  7 Mar 2023 10:01:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C47CB626B0;\n\tTue,  7 Mar 2023 11:01:41 +0100 (CET)","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 227856265E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  7 Mar 2023 11:01:40 +0100 (CET)","from pendragon.ideasonboard.com\n\t(153.162-64-87.adsl-dyn.isp.belgacom.be [87.64.162.153])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A97AE4AD;\n\tTue,  7 Mar 2023 11:01:39 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1678183301;\n\tbh=ifgQYI5WouVgC1dpqbhZCx4xM5+UsmhNK/7nUs9XGrs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=kInkQZnco7qTNvnybput5cfbmGy7VCUJRZ240iO4oY5s09qQ6Y/UobQkKlRP697ai\n\to2u28FuDup6S/ne1e+LxQXPHaGxkexQJn1r6QtRalX3p0HcwiXFKDbGO+zTsKM/U/b\n\t2AP4K/a2RwV1guuSA5xlhKE5HbdiTDPm1A/bRlT4XBbl93yOBDVAKxY+osnLjluCNa\n\t+6oVRyKDQK1Y9av/iTGy5ZhHe54Gx3NAImJLwLa8LBshYaLQkhYKch6m4MODO/O7fP\n\topX+3Z2XsPfK8/4a0tb4hiOdmSK9oGPlZTaHumcxmJFfYb2S0vAFRUnQncRGRov5KB\n\tB3t+TbgvBSL/g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1678183299;\n\tbh=ifgQYI5WouVgC1dpqbhZCx4xM5+UsmhNK/7nUs9XGrs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PYDWJBXt/QRFGjma2+Zm/4Ki4Gvmn3j8G53yFOh9jAD2ProIYwQQro0gnV/ggFAXv\n\tMeSPcVi5bpE1J+yIB69ncSYAxy89wE0yZ6uaiE0LZd5sNwBEa6/FxBTcFzwFKAefBD\n\tQHMGhWbUVaLqlrgLMXC1r4fPhu/MwH5XelSj9WDg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"PYDWJBXt\"; dkim-atps=neutral","Date":"Tue, 7 Mar 2023 12:01:43 +0200","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<20230307100143.GA22827@pendragon.ideasonboard.com>","References":"<20230302135429.23821-1-naush@raspberrypi.com>\n\t<mailman.57.1677765273.25031.libcamera-devel@lists.libcamera.org>\n\t<20230306172522.GA27186@pendragon.ideasonboard.com>\n\t<CAEmqJPpqPMbxP3oF4g-PUUmpGViSpLptr2cjy6kXJFoPU-Vt6A@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAEmqJPpqPMbxP3oF4g-PUUmpGViSpLptr2cjy6kXJFoPU-Vt6A@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]