[{"id":26474,"web_url":"https://patchwork.libcamera.org/comment/26474/","msgid":"<mailman.21.1677237250.25031.libcamera-devel@lists.libcamera.org>","date":"2023-02-24T11:13:56","subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for the patch.\n\nOn Thu, 23 Feb 2023 at 12:49, Naushir Patuck via libcamera-devel\n<libcamera-devel@lists.libcamera.org> 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\nI guess just one thing I'm slightly curious about, what happens at\nstartup if nothing has run and then the camera decides not to deliver\nany frames? I guess we still set initial values in the frame length\nlist based on what we configure before starting the camera, so that we\ncan't find ourselves in the \"wait umpteen minutes\" situation (unless\nthe user expressly requested that).\n\n>\n> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n\nReviewed-by: David Plowman <david.plowman@raspberrypi.com>\n\nThanks!\nDavid\n\n> ---\n>  src/ipa/raspberrypi/raspberrypi.cpp | 33 +++++++++++++++++++++++++++--\n>  1 file changed, 31 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f6826bf27fe1..b8fe0c002a13 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> @@ -155,6 +159,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 +225,9 @@ private:\n>\n>         /* Maximum gain code for the sensor. */\n>         uint32_t maxSensorGainCode_;\n> +\n> +       /* Track the frame length times over FrameLengthsQueueSize frames. */\n> +       std::deque<Duration> frameLengths_;\n>  };\n>\n>  int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> @@ -294,6 +302,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 +349,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,9 +1441,20 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n>                 applyAGC(&agcStatus, ctrls);\n>\n>                 setDelayedControls.emit(ctrls, ipaContext);\n> +               setCameraTimeoutValue();\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 pipe line handler.\n> +        */\n> +       auto max = std::max_element(frameLengths_.begin(), frameLengths_.end());\n> +       setCameraTimeout.emit(max->get<std::milli>());\n> +}\n> +\n>  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n>  {\n>         LOG(IPARPI, Debug) << \"Applying WB R: \" << awbStatus->gainR << \" B: \"\n> @@ -1522,6 +1540,17 @@ 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> +       if (frameLengths_.size() == FrameLengthsQueueSize)\n> +               frameLengths_.pop_front();\n> +\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)\n> --\n> 2.25.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 C4FF8BDCBF\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 24 Feb 2023 11:14:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2FBF762669;\n\tFri, 24 Feb 2023 12:14:10 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677237250;\n\tbh=7J3hOQnGl302S9D7+12vJPN42POCHOCvd6Vgm5wWYH8=;\n\th=References:In-Reply-To:Date:To:List-Id:List-Post:From:Cc:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=dIiohBtknRRuvvLlt0zDA/YTVqVh/TUgMGNqSMyK9fdlbbegDG4jRa2aCzv1r+Cwc\n\tvnIN2JeLOY2bAId/QiuwdzHy3QIQjlHwPVzLnWyFfbuYyGZ+HdCF2f2h/R0MVWhfM7\n\tGUpmbT8H1zco8nfCcLXS6xfOxQ4jrR4BsZME5S1FdA8iCJ1Hy8YjKH8tFe80PLHO1W\n\tuT7r+52ylWcGiKk8JFjd79FghALsuGQN+jc14ppq4wd4yuIKktfCLc3S/s1KupWyWL\n\tZWUcp2jFJwtEr1mZb1f3bFR9eeayAJozh+gVyPPYaDQRZ/uGSPgRgKydbvAWwRrY93\n\t68jo87V/z093Q==","References":"<20230223124957.11084-1-naush@raspberrypi.com>\n\t<mailman.14.1677156593.25031.libcamera-devel@lists.libcamera.org>","In-Reply-To":"<mailman.14.1677156593.25031.libcamera-devel@lists.libcamera.org>","Date":"Fri, 24 Feb 2023 11:13:56 +0000","To":"Naushir Patuck <naush@raspberrypi.com>","MIME-Version":"1.0","Message-ID":"<mailman.21.1677237250.25031.libcamera-devel@lists.libcamera.org>","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","From":"David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org>","Precedence":"list","Cc":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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/>","Reply-To":"David Plowman <david.plowman@raspberrypi.com>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26475,"web_url":"https://patchwork.libcamera.org/comment/26475/","msgid":"<mailman.22.1677237695.25031.libcamera-devel@lists.libcamera.org>","date":"2023-02-24T11:21:19","subject":"Re: [libcamera-devel] [PATCH v1 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 David,\n\nThank you for the review!\n\nOn Fri, 24 Feb 2023 at 11:14, David Plowman\n<david.plowman@raspberrypi.com> wrote:\n>\n> Hi Naush\n>\n> Thanks for the patch.\n>\n> On Thu, 23 Feb 2023 at 12:49, Naushir Patuck via libcamera-devel\n> <libcamera-devel@lists.libcamera.org> 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> I guess just one thing I'm slightly curious about, what happens at\n> startup if nothing has run and then the camera decides not to deliver\n> any frames? I guess we still set initial values in the frame length\n> list based on what we configure before starting the camera, so that we\n> can't find ourselves in the \"wait umpteen minutes\" situation (unless\n> the user expressly requested that).\n\nAs part of the startup process, we will have the first frame length put into the\nlist, and this will be the timeout value used for the calculation.\n\nBut you do raise an interesting scenario where if an application configures the\ncamera for a RAW stream, sets the stream hint to say it will pass all buffers\n(so no internal buffers are allocated), then decides not to pass in any requests\nfor a period of time, it could cause the timeout to trigger.  This is all very\nimplausible, and I suppose we give the application an override in patch 3 that\ncould be used to \"fix\" this behavior.\n\nRegards,\nNaush\n\n>\n> >\n> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n>\n> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n>\n> Thanks!\n> David\n>\n> > ---\n> >  src/ipa/raspberrypi/raspberrypi.cpp | 33 +++++++++++++++++++++++++++--\n> >  1 file changed, 31 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index f6826bf27fe1..b8fe0c002a13 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> > @@ -155,6 +159,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 +225,9 @@ private:\n> >\n> >         /* Maximum gain code for the sensor. */\n> >         uint32_t maxSensorGainCode_;\n> > +\n> > +       /* Track the frame length times over FrameLengthsQueueSize frames. */\n> > +       std::deque<Duration> frameLengths_;\n> >  };\n> >\n> >  int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> > @@ -294,6 +302,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 +349,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,9 +1441,20 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> >                 applyAGC(&agcStatus, ctrls);\n> >\n> >                 setDelayedControls.emit(ctrls, ipaContext);\n> > +               setCameraTimeoutValue();\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 pipe line handler.\n> > +        */\n> > +       auto max = std::max_element(frameLengths_.begin(), frameLengths_.end());\n> > +       setCameraTimeout.emit(max->get<std::milli>());\n> > +}\n> > +\n> >  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> >  {\n> >         LOG(IPARPI, Debug) << \"Applying WB R: \" << awbStatus->gainR << \" B: \"\n> > @@ -1522,6 +1540,17 @@ 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> > +       if (frameLengths_.size() == FrameLengthsQueueSize)\n> > +               frameLengths_.pop_front();\n> > +\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)\n> > --\n> > 2.25.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 9597BBE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 24 Feb 2023 11:21:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 354A062668;\n\tFri, 24 Feb 2023 12:21:36 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677237696;\n\tbh=1kWaBf8P7tn5PqPlscHb4tNiZu06TMY8QaO97wajNzg=;\n\th=References:In-Reply-To:Date:To:List-Id:List-Post:From:Cc:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=NFqiLjEds/pNA7rcdP19RWobkwr37wBfi5+LkmAyHGI5kvXe/kTSbnt6/81SPogU+\n\th0MH1YRvVZC56SDPqoPOzQwWOud9U0d93nQS2xelaPTtl42XhJxJzvl7AKXi/Bl7aV\n\t7RsMMuHOM/H1kKFpWqJiDYamORoIT+P9fX9BqiDM24h902kVn05QUhVfoNCbxmu+QM\n\tSYOTiHHou4wvuyc18jJfu11nEgIifYxGSwB8iBPabj21Vux++pkHpBA1SOnH+xc3VQ\n\t4io7gtkpiOe47A8LuQ4Tsgmr3/H7seB57nERBdUADFf7j3MmUf08j8ALU+piPFeKCe\n\tofLq6z7A2n4tg==","References":"<20230223124957.11084-1-naush@raspberrypi.com>\n\t<mailman.14.1677156593.25031.libcamera-devel@lists.libcamera.org>\n\t<CAHW6GYJ=c8USJi1vmr9KUbJbCndMvCd0pJSZV62Pfd+m6a_veg@mail.gmail.com>","In-Reply-To":"<CAHW6GYJ=c8USJi1vmr9KUbJbCndMvCd0pJSZV62Pfd+m6a_veg@mail.gmail.com>","Date":"Fri, 24 Feb 2023 11:21:19 +0000","To":"David Plowman <david.plowman@raspberrypi.com>","MIME-Version":"1.0","Message-ID":"<mailman.22.1677237695.25031.libcamera-devel@lists.libcamera.org>","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Precedence":"list","Cc":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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/>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26516,"web_url":"https://patchwork.libcamera.org/comment/26516/","msgid":"<20230301143826.b54xbsuwzccsw6gu@uno.localdomain>","date":"2023-03-01T15:17:02","subject":"Re: [libcamera-devel] [PATCH v1 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, Feb 23, 2023 at 12:49:56PM +0000, Naushir Patuck via libcamera-devel wrote:\n> Date: Thu, 23 Feb 2023 12:49:56 +0000\n> From: Naushir Patuck <naush@raspberrypi.com>\n> To: libcamera-devel@lists.libcamera.org\n> Subject: [PATCH v1 2/3] ipa: raspberrypi: Better heruistics for calculating\n>  Unicam timeout\n> X-Mailer: git-send-email 2.25.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\nIs this a typo or do we have exposures time of 10 minutes ?\n\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> ---\n>  src/ipa/raspberrypi/raspberrypi.cpp | 33 +++++++++++++++++++++++++++--\n>  1 file changed, 31 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f6826bf27fe1..b8fe0c002a13 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> @@ -155,6 +159,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 +225,9 @@ 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>  };\n>\n>  int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> @@ -294,6 +302,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 +349,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,9 +1441,20 @@ 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 pipe line handler.\n> +\t */\n> +\tauto max = std::max_element(frameLengths_.begin(), frameLengths_.end());\n> +\tsetCameraTimeout.emit(max->get<std::milli>());\n\nI would store the last set timeout and only emit the signal when it\nchanges. Am I wrong or the signal is emitted every frame ?\n\n> +}\n> +\n>  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n>  {\n>  \tLOG(IPARPI, Debug) << \"Applying WB R: \" << awbStatus->gainR << \" B: \"\n> @@ -1522,6 +1540,17 @@ 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> +\tif (frameLengths_.size() == FrameLengthsQueueSize)\n> +\t\tframeLengths_.pop_front();\n> +\n> +\tframeLengths_.push_back(helper_->exposure(vblank + mode_.height,\n> +\t\t\t\t\t\t  helper_->hblankToLineLength(hblank)));\n\nAlternatively you can push_front() and resize(FrameLengthsQueueSize).\nIf I read the documentation right\n- When size < FrameLengthsQueueSize the deque is expanded to\n  FrameLengthsQueueSize with default constructed elements (0)\n- When size > the last elements are invalidated\n\nComplexity is probably similar ?\n\nAlternatively you can also construct the deque with 10 elements\ninitialized to 0 and unconditionally pop_front + push_back\n\n>  }\n>\n>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> --\n> 2.25.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 DA32BBF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  1 Mar 2023 15:17:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8F9C3626B9;\n\tWed,  1 Mar 2023 16:17:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B09D62666\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  1 Mar 2023 16:17:06 +0100 (CET)","from ideasonboard.com (mob-5-90-142-222.net.vodafone.it\n\t[5.90.142.222])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C08CC890;\n\tWed,  1 Mar 2023 16:17:05 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677683827;\n\tbh=3QKorHNb37VhtdXs4Tk1dzS6zQxUWARWWpAPoT7anbA=;\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=4Jtn5JF02t+xNGSzFM+VlUijmUyzNi2w25xc0FJXPNEVOwx4xwT48s4dQtze4TKn1\n\tlCH6eMOfphETGWW3ym/z3PhCjrCF+fAvBzBO/IMaksKKGKEL1I3ckLm1YM4Cfo6Fn9\n\t2GBOztu03/uHNkbeA859WF3C+BOWmXrjzR9To7XGI91x4yFYulifT55xrJkuKDR4Sc\n\tdo+qNAQaGO5Opg+0vJxNkSthH6su+Z10Pk/u7z3pH+sJVDSzlWH+HJAK3+q1+2h6DJ\n\tGaSD8ons+F9WAtYf2n/gpWEf8pp1rvWV4ihHWx/tsyvA/EsWx5kS/Af2PS/ahhkuHm\n\t1zkZeGLoCyEFw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1677683825;\n\tbh=3QKorHNb37VhtdXs4Tk1dzS6zQxUWARWWpAPoT7anbA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NrSpuIOGQfMqefTrD4A7b3SN6/L4tpu0OJ0G+OdAnrc8NBiUreE8HSjpYs7PzEBla\n\tyxJOOhqqp6HB3GwrCQmAITqDU6DieNSfNTILCialJaXDvxfhyYvxYZ2gS/7e0UGmiK\n\tdIx5XUXJFdYQo2NlkcV+vZu4gpvM3e8NVCgEj0oU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NrSpuIOG\"; dkim-atps=neutral","Date":"Wed, 1 Mar 2023 16:17:02 +0100","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<20230301143826.b54xbsuwzccsw6gu@uno.localdomain>","References":"<20230223124957.11084-1-naush@raspberrypi.com>\n\t<mailman.14.1677156593.25031.libcamera-devel@lists.libcamera.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<mailman.14.1677156593.25031.libcamera-devel@lists.libcamera.org>","Subject":"Re: [libcamera-devel] [PATCH v1 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":26520,"web_url":"https://patchwork.libcamera.org/comment/26520/","msgid":"<mailman.44.1677688197.25031.libcamera-devel@lists.libcamera.org>","date":"2023-03-01T16:29:38","subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Wed, 1 Mar 2023 at 15:17, Jacopo Mondi via libcamera-devel\n<libcamera-devel@lists.libcamera.org> wrote:\n>\n> Hi Naush\n>\n> On Thu, Feb 23, 2023 at 12:49:56PM +0000, Naushir Patuck via libcamera-devel wrote:\n> > Date: Thu, 23 Feb 2023 12:49:56 +0000\n> > From: Naushir Patuck <naush@raspberrypi.com>\n> > To: libcamera-devel@lists.libcamera.org\n> > Subject: [PATCH v1 2/3] ipa: raspberrypi: Better heruistics for calculating\n> >  Unicam timeout\n> > X-Mailer: git-send-email 2.25.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>\n> Is this a typo or do we have exposures time of 10 minutes ?\n\nNo typo.\n\nIMX477 will do FRAME_LENGTH of 0xFFFF << 7 = 8388480 lines, and\nLINE_LENGTH up to 0xfff0 (65520). PIXEL_RATE of 840MPix/s, gives\n654.3secs, or 10.9mins.\nExposure has to be 22lines shorter than that max, but for timeouts\nwe're really talking frame times instead of exposure times.\n\nIMX708 will also do FRAME_LENGTH of 0xFFFF << 7 = 8388480 lines, but\nLINE_LENGTH is currently fixed at 0x3d20 (15648) for full res.\nPIXEL_RATE of 595200000 pixels, gives max frame time of 220 seconds or\n3.67mins.\n\n:-)\n\n  Dave\n\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> > ---\n> >  src/ipa/raspberrypi/raspberrypi.cpp | 33 +++++++++++++++++++++++++++--\n> >  1 file changed, 31 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index f6826bf27fe1..b8fe0c002a13 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> > @@ -155,6 +159,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 +225,9 @@ private:\n> >\n> >       /* Maximum gain code for the sensor. */\n> >       uint32_t maxSensorGainCode_;\n> > +\n> > +     /* Track the frame length times over FrameLengthsQueueSize frames. */\n> > +     std::deque<Duration> frameLengths_;\n> >  };\n> >\n> >  int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> > @@ -294,6 +302,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 +349,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,9 +1441,20 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> >               applyAGC(&agcStatus, ctrls);\n> >\n> >               setDelayedControls.emit(ctrls, ipaContext);\n> > +             setCameraTimeoutValue();\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 pipe line handler.\n> > +      */\n> > +     auto max = std::max_element(frameLengths_.begin(), frameLengths_.end());\n> > +     setCameraTimeout.emit(max->get<std::milli>());\n>\n> I would store the last set timeout and only emit the signal when it\n> changes. Am I wrong or the signal is emitted every frame ?\n>\n> > +}\n> > +\n> >  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> >  {\n> >       LOG(IPARPI, Debug) << \"Applying WB R: \" << awbStatus->gainR << \" B: \"\n> > @@ -1522,6 +1540,17 @@ 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> > +     if (frameLengths_.size() == FrameLengthsQueueSize)\n> > +             frameLengths_.pop_front();\n> > +\n> > +     frameLengths_.push_back(helper_->exposure(vblank + mode_.height,\n> > +                                               helper_->hblankToLineLength(hblank)));\n>\n> Alternatively you can push_front() and resize(FrameLengthsQueueSize).\n> If I read the documentation right\n> - When size < FrameLengthsQueueSize the deque is expanded to\n>   FrameLengthsQueueSize with default constructed elements (0)\n> - When size > the last elements are invalidated\n>\n> Complexity is probably similar ?\n>\n> Alternatively you can also construct the deque with 10 elements\n> initialized to 0 and unconditionally pop_front + push_back\n>\n> >  }\n> >\n> >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> > --\n> > 2.25.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 9A3A3BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  1 Mar 2023 16:29:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 274E76266A;\n\tWed,  1 Mar 2023 17:29:58 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677688198;\n\tbh=djGNbCRDAnB7yk0mccUxk7F9N/HBgVFP/6vXZRi/B0s=;\n\th=References:In-Reply-To:Date:To:List-Id:List-Post:From:Cc:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=IqbgHDiIWAzgAqmTnTE5MD2BRMw7YlO8wg9Vps0El+2lPAOSIgtIKPCkuZUKQTdeU\n\tcqkeZS+gc512is7bqISJRjUZPvzL3y9fNfetJZKIk+nEE769mWK9o/bsbwPSP8eAkx\n\tQ3NOEulMKV/73aWoc6l8cqOlT6oDm/NZczKh0T4O7MCojYSZlx5ab01FEqk/c8kF9D\n\tlPulBrnLTIW1427mAtUJ73nFVIGhNeCbucHMptMJAyL/zIsYqS119Ifc+pBfRN5fKD\n\t/3iSZG4rh1VhbSVcBOKEatPLTzby/tjUrUJawxeR83F3bzprx13DjNWlBHCERsxv/H\n\tNTtwXKilsYIlw==","References":"<20230223124957.11084-1-naush@raspberrypi.com>\n\t<mailman.14.1677156593.25031.libcamera-devel@lists.libcamera.org>\n\t<20230301143826.b54xbsuwzccsw6gu@uno.localdomain>","In-Reply-To":"<20230301143826.b54xbsuwzccsw6gu@uno.localdomain>","Date":"Wed, 1 Mar 2023 16:29:38 +0000","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Message-ID":"<mailman.44.1677688197.25031.libcamera-devel@lists.libcamera.org>","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","From":"Dave Stevenson via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Precedence":"list","Cc":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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/>","Reply-To":"Dave Stevenson <dave.stevenson@raspberrypi.com>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26523,"web_url":"https://patchwork.libcamera.org/comment/26523/","msgid":"<mailman.53.1677746166.25031.libcamera-devel@lists.libcamera.org>","date":"2023-03-02T08:35:48","subject":"Re: [libcamera-devel] [PATCH v1 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 Jacopo,\n\nThanks for the feedback!\n\nOn Wed, 1 Mar 2023 at 15:17, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:\n>\n> Hi Naush\n>\n> On Thu, Feb 23, 2023 at 12:49:56PM +0000, Naushir Patuck via libcamera-devel wrote:\n> > Date: Thu, 23 Feb 2023 12:49:56 +0000\n> > From: Naushir Patuck <naush@raspberrypi.com>\n> > To: libcamera-devel@lists.libcamera.org\n> > Subject: [PATCH v1 2/3] ipa: raspberrypi: Better heruistics for calculating\n> >  Unicam timeout\n> > X-Mailer: git-send-email 2.25.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>\n> Is this a typo or do we have exposures time of 10 minutes ?\n>\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> > ---\n> >  src/ipa/raspberrypi/raspberrypi.cpp | 33 +++++++++++++++++++++++++++--\n> >  1 file changed, 31 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index f6826bf27fe1..b8fe0c002a13 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> > @@ -155,6 +159,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 +225,9 @@ private:\n> >\n> >       /* Maximum gain code for the sensor. */\n> >       uint32_t maxSensorGainCode_;\n> > +\n> > +     /* Track the frame length times over FrameLengthsQueueSize frames. */\n> > +     std::deque<Duration> frameLengths_;\n> >  };\n> >\n> >  int IPARPi::init(const IPASettings &settings, bool lensPresent, IPAInitResult *result)\n> > @@ -294,6 +302,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 +349,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,9 +1441,20 @@ void IPARPi::processStats(unsigned int bufferId, unsigned int ipaContext)\n> >               applyAGC(&agcStatus, ctrls);\n> >\n> >               setDelayedControls.emit(ctrls, ipaContext);\n> > +             setCameraTimeoutValue();\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 pipe line handler.\n> > +      */\n> > +     auto max = std::max_element(frameLengths_.begin(), frameLengths_.end());\n> > +     setCameraTimeout.emit(max->get<std::milli>());\n>\n> I would store the last set timeout and only emit the signal when it\n> changes. Am I wrong or the signal is emitted every frame ?\n\nYes, currently the signal is emitted every frame.  I can optimise this by\ncaching the last submitted value and only emitting the signal if the value\nchanges.\n\n>\n> > +}\n> > +\n> >  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> >  {\n> >       LOG(IPARPI, Debug) << \"Applying WB R: \" << awbStatus->gainR << \" B: \"\n> > @@ -1522,6 +1540,17 @@ 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> > +     if (frameLengths_.size() == FrameLengthsQueueSize)\n> > +             frameLengths_.pop_front();\n> > +\n> > +     frameLengths_.push_back(helper_->exposure(vblank + mode_.height,\n> > +                                               helper_->hblankToLineLength(hblank)));\n>\n> Alternatively you can push_front() and resize(FrameLengthsQueueSize).\n> If I read the documentation right\n> - When size < FrameLengthsQueueSize the deque is expanded to\n>   FrameLengthsQueueSize with default constructed elements (0)\n> - When size > the last elements are invalidated\n>\n> Complexity is probably similar ?\n>\n> Alternatively you can also construct the deque with 10 elements\n> initialized to 0 and unconditionally pop_front + push_back\n\nI like the suggestion to allocate 10 elements with 0 value to start with.  I'll\napply this change!\n\nRegards,\nNaush\n\n>\n> >  }\n> >\n> >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> > --\n> > 2.25.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 3CCCFBE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  2 Mar 2023 08:36:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E919626C8;\n\tThu,  2 Mar 2023 09:36:06 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677746166;\n\tbh=RTjJF91o5CzSwVU4DeYOzuDi3jq5GppzGwIE/BY4N8Y=;\n\th=References:In-Reply-To:Date:To:List-Id:List-Post:From:Cc:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=uJRkLMO10KAt8dyZPDeSm9iaIjeXNkrnoc59UMm2+uovyD97m2ufX1fUYLxQHQe2X\n\tny4QgyZ4gnGwnyBS7W3pccPtMIycdvMx6d7DPiil11wj72lnjOQYIpw2UAzRLjGlH8\n\tKdiUumctr1ETYmRYTRXAomJputdXMLxecrAVPz9dHi6JmP+MSx1/BJ08Z2Kbg4B9O+\n\t9MEOCe9GMyw5LU6nJ23uBjHxvzbtkFQFo8YKV8zK8914KiPKKkZUJy5nxoA4UCF8r+\n\tTVAObBfSHfhifTT7jcUdGHWL7022MlYoR/XSBMJcSNAyJbtKHNJuENIzFMoTcJx1Lv\n\tncGzr9wyzAyJQ==","References":"<20230223124957.11084-1-naush@raspberrypi.com>\n\t<mailman.14.1677156593.25031.libcamera-devel@lists.libcamera.org>\n\t<20230301143826.b54xbsuwzccsw6gu@uno.localdomain>","In-Reply-To":"<20230301143826.b54xbsuwzccsw6gu@uno.localdomain>","Date":"Thu, 2 Mar 2023 08:35:48 +0000","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Message-ID":"<mailman.53.1677746166.25031.libcamera-devel@lists.libcamera.org>","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Precedence":"list","Cc":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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/>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","Subject":"Re: [libcamera-devel] [PATCH v1 2/3] ipa: raspberrypi: Better\n\theruistics for calculating Unicam timeout","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]