[{"id":4821,"web_url":"https://patchwork.libcamera.org/comment/4821/","msgid":"<20200514143957.GC5955@pendragon.ideasonboard.com>","date":"2020-05-14T14:39:57","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline: raspberrypi:\n\tDon't inline all of StaggeredCtrl","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nI wonder if I forgot to CC you on this.\n\nOn Tue, May 12, 2020 at 03:39:02AM +0300, Laurent Pinchart wrote:\n> The StaggeredCtrl class has large functions, move them to a .cpp file\n> instead of inlining them all to reduce the binary size.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  .../pipeline/raspberrypi/meson.build          |   3 +-\n>  .../pipeline/raspberrypi/staggered_ctrl.cpp   | 173 ++++++++++++++++++\n>  .../pipeline/raspberrypi/staggered_ctrl.h     | 168 ++---------------\n>  3 files changed, 186 insertions(+), 158 deletions(-)\n>  create mode 100644 src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> index 737857977831..565a8902f1f4 100644\n> --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> @@ -1,3 +1,4 @@\n>  libcamera_sources += files([\n> -    'raspberrypi.cpp'\n> +    'raspberrypi.cpp',\n> +    'staggered_ctrl.cpp',\n>  ])\n> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp\n> new file mode 100644\n> index 000000000000..fbd87d3e791e\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp\n> @@ -0,0 +1,173 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * staggered_ctrl.cpp - Helper for writing staggered ctrls to a V4L2 device.\n> + */\n> +\n> +#include \"staggered_ctrl.h\"\n> +\n> +#include <algorithm>\n> +\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +\n> +/* For logging... */\n> +using libcamera::LogCategory;\n> +using libcamera::LogDebug;\n> +using libcamera::LogInfo;\n> +using libcamera::utils::hex;\n> +\n> +LOG_DEFINE_CATEGORY(RPI_S_W);\n> +\n> +namespace RPi {\n> +\n> +void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev,\n> +\t  std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList)\n> +{\n> +\tstd::lock_guard<std::mutex> lock(lock_);\n> +\n> +\tdev_ = dev;\n> +\tdelay_ = delayList;\n> +\tctrl_.clear();\n> +\n> +\t/* Find the largest delay across all controls. */\n> +\tmaxDelay_ = 0;\n> +\tfor (auto const &p : delay_) {\n> +\t\tLOG(RPI_S_W, Info) << \"Init ctrl \"\n> +\t\t\t\t   << hex(p.first) << \" with delay \"\n> +\t\t\t\t   << static_cast<int>(p.second);\n> +\t\tmaxDelay_ = std::max(maxDelay_, p.second);\n> +\t}\n> +\n> +\tinit_ = true;\n> +}\n> +\n> +void StaggeredCtrl::reset()\n> +{\n> +\tstd::lock_guard<std::mutex> lock(lock_);\n> +\n> +\tint lastSetCount = std::max<int>(0, setCount_ - 1);\n> +\tstd::unordered_map<uint32_t, int32_t> lastVal;\n> +\n> +\t/* Reset the counters. */\n> +\tsetCount_ = getCount_ = 0;\n> +\n> +\t/* Look for the last set values. */\n> +\tfor (auto const &c : ctrl_)\n> +\t\tlastVal[c.first] = c.second[lastSetCount].value;\n> +\n> +\t/* Apply the last set values as the next to be applied. */\n> +\tctrl_.clear();\n> +\tfor (auto &c : lastVal)\n> +\t\tctrl_[c.first][setCount_] = CtrlInfo(c.second);\n> +}\n> +\n> +bool StaggeredCtrl::set(uint32_t ctrl, int32_t value)\n> +{\n> +\tstd::lock_guard<std::mutex> lock(lock_);\n> +\n> +\t/* Can we find this ctrl as one that is registered? */\n> +\tif (delay_.find(ctrl) == delay_.end())\n> +\t\treturn false;\n> +\n> +\tctrl_[ctrl][setCount_].value = value;\n> +\tctrl_[ctrl][setCount_].updated = true;\n> +\n> +\treturn true;\n> +}\n> +\n> +bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList)\n> +{\n> +\tstd::lock_guard<std::mutex> lock(lock_);\n> +\n> +\tfor (auto const &p : ctrlList) {\n> +\t\t/* Can we find this ctrl? */\n> +\t\tif (delay_.find(p.first) == delay_.end())\n> +\t\t\treturn false;\n> +\n> +\t\tctrl_[p.first][setCount_] = CtrlInfo(p.second);\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +bool StaggeredCtrl::set(libcamera::ControlList &controls)\n> +{\n> +\tstd::lock_guard<std::mutex> lock(lock_);\n> +\n> +\tfor (auto const &p : controls) {\n> +\t\t/* Can we find this ctrl? */\n> +\t\tif (delay_.find(p.first) == delay_.end())\n> +\t\t\treturn false;\n> +\n> +\t\tctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>());\n> +\t\tLOG(RPI_S_W, Debug) << \"Setting ctrl \"\n> +\t\t\t\t    << hex(p.first) << \" to \"\n> +\t\t\t\t    << ctrl_[p.first][setCount_].value\n> +\t\t\t\t    << \" at index \"\n> +\t\t\t\t    << setCount_;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +int StaggeredCtrl::write()\n> +{\n> +\tstd::lock_guard<std::mutex> lock(lock_);\n> +\tlibcamera::ControlList controls(dev_->controls());\n> +\n> +\tfor (auto &p : ctrl_) {\n> +\t\tint delayDiff = maxDelay_ - delay_[p.first];\n> +\t\tint index = std::max<int>(0, setCount_ - delayDiff);\n> +\n> +\t\tif (p.second[index].updated) {\n> +\t\t\t/* We need to write this value out. */\n> +\t\t\tcontrols.set(p.first, p.second[index].value);\n> +\t\t\tp.second[index].updated = false;\n> +\t\t\tLOG(RPI_S_W, Debug) << \"Writing ctrl \"\n> +\t\t\t\t\t    << hex(p.first) << \" to \"\n> +\t\t\t\t\t    << p.second[index].value\n> +\t\t\t\t\t    << \" at index \"\n> +\t\t\t\t\t    << index;\n> +\t\t}\n> +\t}\n> +\n> +\tnextFrame();\n> +\treturn dev_->setControls(&controls);\n> +}\n> +\n> +void StaggeredCtrl::get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t offset)\n> +{\n> +\tstd::lock_guard<std::mutex> lock(lock_);\n> +\n> +\t/* Account for the offset to reset the getCounter. */\n> +\tgetCount_ += offset + 1;\n> +\n> +\tctrl.clear();\n> +\tfor (auto &p : ctrl_) {\n> +\t\tint index = std::max<int>(0, getCount_ - maxDelay_);\n> +\t\tctrl[p.first] = p.second[index].value;\n> +\t\tLOG(RPI_S_W, Debug) << \"Getting ctrl \"\n> +\t\t\t\t    << hex(p.first) << \" to \"\n> +\t\t\t\t    << p.second[index].value\n> +\t\t\t\t    << \" at index \"\n> +\t\t\t\t    << index;\n> +\t}\n> +}\n> +\n> +void StaggeredCtrl::nextFrame()\n> +{\n> +\t/* Advance the control history to the next frame */\n> +\tint prevCount = setCount_;\n> +\tsetCount_++;\n> +\n> +\tLOG(RPI_S_W, Debug) << \"Next frame, set index is \" << setCount_;\n> +\n> +\tfor (auto &p : ctrl_) {\n> +\t\tp.second[setCount_].value = p.second[prevCount].value;\n> +\t\tp.second[setCount_].updated = false;\n> +\t}\n> +}\n> +\n> +} /* namespace RPi */\n> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h\n> index 0403c087c686..c8f000a0b43c 100644\n> --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h\n> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h\n> @@ -6,24 +6,16 @@\n>   */\n>  #pragma once\n>  \n> -#include <algorithm>\n> +#include <array>\n>  #include <initializer_list>\n>  #include <mutex>\n>  #include <unordered_map>\n> +#include <utility>\n>  \n>  #include <libcamera/controls.h>\n> -#include \"log.h\"\n> -#include \"utils.h\"\n> +\n>  #include \"v4l2_videodevice.h\"\n>  \n> -/* For logging... */\n> -using libcamera::LogCategory;\n> -using libcamera::LogDebug;\n> -using libcamera::LogInfo;\n> -using libcamera::utils::hex;\n> -\n> -LOG_DEFINE_CATEGORY(RPI_S_W);\n> -\n>  namespace RPi {\n>  \n>  class StaggeredCtrl\n> @@ -34,163 +26,25 @@ public:\n>  \t{\n>  \t}\n>  \n> -\t~StaggeredCtrl()\n> -\t{\n> -\t}\n> -\n>  \toperator bool() const\n>  \t{\n>  \t\treturn init_;\n>  \t}\n>  \n>  \tvoid init(libcamera::V4L2VideoDevice *dev,\n> -\t\t  std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList)\n> -\t{\n> -\t\tstd::lock_guard<std::mutex> lock(lock_);\n> +\t\t  std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList);\n> +\tvoid reset();\n>  \n> -\t\tdev_ = dev;\n> -\t\tdelay_ = delayList;\n> -\t\tctrl_.clear();\n> +\tvoid get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t offset = 0);\n>  \n> -\t\t/* Find the largest delay across all controls. */\n> -\t\tmaxDelay_ = 0;\n> -\t\tfor (auto const &p : delay_) {\n> -\t\t\tLOG(RPI_S_W, Info) << \"Init ctrl \"\n> -\t\t\t\t\t   << hex(p.first) << \" with delay \"\n> -\t\t\t\t\t   << static_cast<int>(p.second);\n> -\t\t\tmaxDelay_ = std::max(maxDelay_, p.second);\n> -\t\t}\n> +\tbool set(uint32_t ctrl, int32_t value);\n> +\tbool set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList);\n> +\tbool set(libcamera::ControlList &controls);\n>  \n> -\t\tinit_ = true;\n> -\t}\n> -\n> -\tvoid reset()\n> -\t{\n> -\t\tstd::lock_guard<std::mutex> lock(lock_);\n> -\n> -\t\tint lastSetCount = std::max<int>(0, setCount_ - 1);\n> -\t\tstd::unordered_map<uint32_t, int32_t> lastVal;\n> -\n> -\t\t/* Reset the counters. */\n> -\t\tsetCount_ = getCount_ = 0;\n> -\n> -\t\t/* Look for the last set values. */\n> -\t\tfor (auto const &c : ctrl_)\n> -\t\t\tlastVal[c.first] = c.second[lastSetCount].value;\n> -\n> -\t\t/* Apply the last set values as the next to be applied. */\n> -\t\tctrl_.clear();\n> -\t\tfor (auto &c : lastVal)\n> -\t\t\tctrl_[c.first][setCount_] = CtrlInfo(c.second);\n> -\t}\n> -\n> -\tbool set(uint32_t ctrl, int32_t value)\n> -\t{\n> -\t\tstd::lock_guard<std::mutex> lock(lock_);\n> -\n> -\t\t/* Can we find this ctrl as one that is registered? */\n> -\t\tif (delay_.find(ctrl) == delay_.end())\n> -\t\t\treturn false;\n> -\n> -\t\tctrl_[ctrl][setCount_].value = value;\n> -\t\tctrl_[ctrl][setCount_].updated = true;\n> -\n> -\t\treturn true;\n> -\t}\n> -\n> -\tbool set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList)\n> -\t{\n> -\t\tstd::lock_guard<std::mutex> lock(lock_);\n> -\n> -\t\tfor (auto const &p : ctrlList) {\n> -\t\t\t/* Can we find this ctrl? */\n> -\t\t\tif (delay_.find(p.first) == delay_.end())\n> -\t\t\t\treturn false;\n> -\n> -\t\t\tctrl_[p.first][setCount_] = CtrlInfo(p.second);\n> -\t\t}\n> -\n> -\t\treturn true;\n> -\t}\n> -\n> -\tbool set(libcamera::ControlList &controls)\n> -\t{\n> -\t\tstd::lock_guard<std::mutex> lock(lock_);\n> -\n> -\t\tfor (auto const &p : controls) {\n> -\t\t\t/* Can we find this ctrl? */\n> -\t\t\tif (delay_.find(p.first) == delay_.end())\n> -\t\t\t\treturn false;\n> -\n> -\t\t\tctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>());\n> -\t\t\tLOG(RPI_S_W, Debug) << \"Setting ctrl \"\n> -\t\t\t\t\t    << hex(p.first) << \" to \"\n> -\t\t\t\t\t    << ctrl_[p.first][setCount_].value\n> -\t\t\t\t\t    << \" at index \"\n> -\t\t\t\t\t    << setCount_;\n> -\t\t}\n> -\n> -\t\treturn true;\n> -\t}\n> -\n> -\tint write()\n> -\t{\n> -\t\tstd::lock_guard<std::mutex> lock(lock_);\n> -\t\tlibcamera::ControlList controls(dev_->controls());\n> -\n> -\t\tfor (auto &p : ctrl_) {\n> -\t\t\tint delayDiff = maxDelay_ - delay_[p.first];\n> -\t\t\tint index = std::max<int>(0, setCount_ - delayDiff);\n> -\n> -\t\t\tif (p.second[index].updated) {\n> -\t\t\t\t/* We need to write this value out. */\n> -\t\t\t\tcontrols.set(p.first, p.second[index].value);\n> -\t\t\t\tp.second[index].updated = false;\n> -\t\t\t\tLOG(RPI_S_W, Debug) << \"Writing ctrl \"\n> -\t\t\t\t\t\t    << hex(p.first) << \" to \"\n> -\t\t\t\t\t\t    << p.second[index].value\n> -\t\t\t\t\t\t    << \" at index \"\n> -\t\t\t\t\t\t    << index;\n> -\t\t\t}\n> -\t\t}\n> -\n> -\t\tnextFrame();\n> -\t\treturn dev_->setControls(&controls);\n> -\t}\n> -\n> -\tvoid get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t offset = 0)\n> -\t{\n> -\t\tstd::lock_guard<std::mutex> lock(lock_);\n> -\n> -\t\t/* Account for the offset to reset the getCounter. */\n> -\t\tgetCount_ += offset + 1;\n> -\n> -\t\tctrl.clear();\n> -\t\tfor (auto &p : ctrl_) {\n> -\t\t\tint index = std::max<int>(0, getCount_ - maxDelay_);\n> -\t\t\tctrl[p.first] = p.second[index].value;\n> -\t\t\tLOG(RPI_S_W, Debug) << \"Getting ctrl \"\n> -\t\t\t\t\t    << hex(p.first) << \" to \"\n> -\t\t\t\t\t    << p.second[index].value\n> -\t\t\t\t\t    << \" at index \"\n> -\t\t\t\t\t    << index;\n> -\t\t}\n> -\t}\n> +\tint write();\n>  \n>  private:\n> -\tvoid nextFrame()\n> -\t{\n> -\t\t/* Advance the control history to the next frame */\n> -\t\tint prevCount = setCount_;\n> -\t\tsetCount_++;\n> -\n> -\t\tLOG(RPI_S_W, Debug) << \"Next frame, set index is \" << setCount_;\n> -\n> -\t\tfor (auto &p : ctrl_) {\n> -\t\t\tp.second[setCount_].value = p.second[prevCount].value;\n> -\t\t\tp.second[setCount_].updated = false;\n> -\t\t}\n> -\t}\n> +\tvoid nextFrame();\n>  \n>  \t/* listSize must be a power of 2. */\n>  \tstatic constexpr int listSize = (1 << 4);","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["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 0578F60DF2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 14 May 2020 16:40:07 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0A951259;\n\tThu, 14 May 2020 16:40:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NEqcW0e3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1589467206;\n\tbh=I6p6IUZNkYyym6pAxQw627okaWASVA8k32mUCXzmNHg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NEqcW0e3iEX/GUhYZS66FuLN4W+qvCipwS/YYptV8sCwqxCnLpKWE6UDWtUuVxFw8\n\tPO79qIBKDpqJVBc3dxq7Xthc/29ekV4ig0S/T78yJsl1LU48s9t2wseQkZDidZOY1T\n\tez9vsBgVlnaS1MaSXUjy+hND593SMrp4shqCw6/Y=","Date":"Thu, 14 May 2020 17:39:57 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200514143957.GC5955@pendragon.ideasonboard.com>","References":"<20200512003903.20986-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200512003903.20986-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline: raspberrypi:\n\tDon't inline all of StaggeredCtrl","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>","X-List-Received-Date":"Thu, 14 May 2020 14:40:07 -0000"}},{"id":4823,"web_url":"https://patchwork.libcamera.org/comment/4823/","msgid":"<CAEmqJPrVYRqAQofHG8V2xWi1HyRvFq2ZqPHjAPtQvZhF8FDhNQ@mail.gmail.com>","date":"2020-05-14T14:55:23","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline: raspberrypi:\n\tDon't inline all of StaggeredCtrl","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 12 May 2020 at 01:39, Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> The StaggeredCtrl class has large functions, move them to a .cpp file\n> instead of inlining them all to reduce the binary size.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  .../pipeline/raspberrypi/meson.build          |   3 +-\n>  .../pipeline/raspberrypi/staggered_ctrl.cpp   | 173 ++++++++++++++++++\n>  .../pipeline/raspberrypi/staggered_ctrl.h     | 168 ++---------------\n>  3 files changed, 186 insertions(+), 158 deletions(-)\n>  create mode 100644 src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/raspberrypi/meson.build\n> index 737857977831..565a8902f1f4 100644\n> --- a/src/libcamera/pipeline/raspberrypi/meson.build\n> +++ b/src/libcamera/pipeline/raspberrypi/meson.build\n> @@ -1,3 +1,4 @@\n>  libcamera_sources += files([\n> -    'raspberrypi.cpp'\n> +    'raspberrypi.cpp',\n> +    'staggered_ctrl.cpp',\n>  ])\n> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp\n> new file mode 100644\n> index 000000000000..fbd87d3e791e\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp\n> @@ -0,0 +1,173 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * staggered_ctrl.cpp - Helper for writing staggered ctrls to a V4L2 device.\n> + */\n> +\n> +#include \"staggered_ctrl.h\"\n> +\n> +#include <algorithm>\n> +\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +\n> +/* For logging... */\n> +using libcamera::LogCategory;\n> +using libcamera::LogDebug;\n> +using libcamera::LogInfo;\n> +using libcamera::utils::hex;\n> +\n> +LOG_DEFINE_CATEGORY(RPI_S_W);\n> +\n> +namespace RPi {\n> +\n> +void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev,\n> +         std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList)\n> +{\n> +       std::lock_guard<std::mutex> lock(lock_);\n> +\n> +       dev_ = dev;\n> +       delay_ = delayList;\n> +       ctrl_.clear();\n> +\n> +       /* Find the largest delay across all controls. */\n> +       maxDelay_ = 0;\n> +       for (auto const &p : delay_) {\n> +               LOG(RPI_S_W, Info) << \"Init ctrl \"\n> +                                  << hex(p.first) << \" with delay \"\n> +                                  << static_cast<int>(p.second);\n> +               maxDelay_ = std::max(maxDelay_, p.second);\n> +       }\n> +\n> +       init_ = true;\n> +}\n> +\n> +void StaggeredCtrl::reset()\n> +{\n> +       std::lock_guard<std::mutex> lock(lock_);\n> +\n> +       int lastSetCount = std::max<int>(0, setCount_ - 1);\n> +       std::unordered_map<uint32_t, int32_t> lastVal;\n> +\n> +       /* Reset the counters. */\n> +       setCount_ = getCount_ = 0;\n> +\n> +       /* Look for the last set values. */\n> +       for (auto const &c : ctrl_)\n> +               lastVal[c.first] = c.second[lastSetCount].value;\n> +\n> +       /* Apply the last set values as the next to be applied. */\n> +       ctrl_.clear();\n> +       for (auto &c : lastVal)\n> +               ctrl_[c.first][setCount_] = CtrlInfo(c.second);\n> +}\n> +\n> +bool StaggeredCtrl::set(uint32_t ctrl, int32_t value)\n> +{\n> +       std::lock_guard<std::mutex> lock(lock_);\n> +\n> +       /* Can we find this ctrl as one that is registered? */\n> +       if (delay_.find(ctrl) == delay_.end())\n> +               return false;\n> +\n> +       ctrl_[ctrl][setCount_].value = value;\n> +       ctrl_[ctrl][setCount_].updated = true;\n> +\n> +       return true;\n> +}\n> +\n> +bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList)\n> +{\n> +       std::lock_guard<std::mutex> lock(lock_);\n> +\n> +       for (auto const &p : ctrlList) {\n> +               /* Can we find this ctrl? */\n> +               if (delay_.find(p.first) == delay_.end())\n> +                       return false;\n> +\n> +               ctrl_[p.first][setCount_] = CtrlInfo(p.second);\n> +       }\n> +\n> +       return true;\n> +}\n> +\n> +bool StaggeredCtrl::set(libcamera::ControlList &controls)\n> +{\n> +       std::lock_guard<std::mutex> lock(lock_);\n> +\n> +       for (auto const &p : controls) {\n> +               /* Can we find this ctrl? */\n> +               if (delay_.find(p.first) == delay_.end())\n> +                       return false;\n> +\n> +               ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>());\n> +               LOG(RPI_S_W, Debug) << \"Setting ctrl \"\n> +                                   << hex(p.first) << \" to \"\n> +                                   << ctrl_[p.first][setCount_].value\n> +                                   << \" at index \"\n> +                                   << setCount_;\n> +       }\n> +\n> +       return true;\n> +}\n> +\n> +int StaggeredCtrl::write()\n> +{\n> +       std::lock_guard<std::mutex> lock(lock_);\n> +       libcamera::ControlList controls(dev_->controls());\n> +\n> +       for (auto &p : ctrl_) {\n> +               int delayDiff = maxDelay_ - delay_[p.first];\n> +               int index = std::max<int>(0, setCount_ - delayDiff);\n> +\n> +               if (p.second[index].updated) {\n> +                       /* We need to write this value out. */\n> +                       controls.set(p.first, p.second[index].value);\n> +                       p.second[index].updated = false;\n> +                       LOG(RPI_S_W, Debug) << \"Writing ctrl \"\n> +                                           << hex(p.first) << \" to \"\n> +                                           << p.second[index].value\n> +                                           << \" at index \"\n> +                                           << index;\n> +               }\n> +       }\n> +\n> +       nextFrame();\n> +       return dev_->setControls(&controls);\n> +}\n> +\n> +void StaggeredCtrl::get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t offset)\n> +{\n> +       std::lock_guard<std::mutex> lock(lock_);\n> +\n> +       /* Account for the offset to reset the getCounter. */\n> +       getCount_ += offset + 1;\n> +\n> +       ctrl.clear();\n> +       for (auto &p : ctrl_) {\n> +               int index = std::max<int>(0, getCount_ - maxDelay_);\n> +               ctrl[p.first] = p.second[index].value;\n> +               LOG(RPI_S_W, Debug) << \"Getting ctrl \"\n> +                                   << hex(p.first) << \" to \"\n> +                                   << p.second[index].value\n> +                                   << \" at index \"\n> +                                   << index;\n> +       }\n> +}\n> +\n> +void StaggeredCtrl::nextFrame()\n> +{\n> +       /* Advance the control history to the next frame */\n> +       int prevCount = setCount_;\n> +       setCount_++;\n> +\n> +       LOG(RPI_S_W, Debug) << \"Next frame, set index is \" << setCount_;\n> +\n> +       for (auto &p : ctrl_) {\n> +               p.second[setCount_].value = p.second[prevCount].value;\n> +               p.second[setCount_].updated = false;\n> +       }\n> +}\n> +\n> +} /* namespace RPi */\n> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h\n> index 0403c087c686..c8f000a0b43c 100644\n> --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h\n> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h\n> @@ -6,24 +6,16 @@\n>   */\n>  #pragma once\n>\n> -#include <algorithm>\n> +#include <array>\n>  #include <initializer_list>\n>  #include <mutex>\n>  #include <unordered_map>\n> +#include <utility>\n>\n>  #include <libcamera/controls.h>\n> -#include \"log.h\"\n> -#include \"utils.h\"\n> +\n>  #include \"v4l2_videodevice.h\"\n>\n> -/* For logging... */\n> -using libcamera::LogCategory;\n> -using libcamera::LogDebug;\n> -using libcamera::LogInfo;\n> -using libcamera::utils::hex;\n> -\n> -LOG_DEFINE_CATEGORY(RPI_S_W);\n> -\n>  namespace RPi {\n>\n>  class StaggeredCtrl\n> @@ -34,163 +26,25 @@ public:\n>         {\n>         }\n>\n> -       ~StaggeredCtrl()\n> -       {\n> -       }\n> -\n>         operator bool() const\n>         {\n>                 return init_;\n>         }\n>\n>         void init(libcamera::V4L2VideoDevice *dev,\n> -                 std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList)\n> -       {\n> -               std::lock_guard<std::mutex> lock(lock_);\n> +                 std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList);\n> +       void reset();\n>\n> -               dev_ = dev;\n> -               delay_ = delayList;\n> -               ctrl_.clear();\n> +       void get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t offset = 0);\n>\n> -               /* Find the largest delay across all controls. */\n> -               maxDelay_ = 0;\n> -               for (auto const &p : delay_) {\n> -                       LOG(RPI_S_W, Info) << \"Init ctrl \"\n> -                                          << hex(p.first) << \" with delay \"\n> -                                          << static_cast<int>(p.second);\n> -                       maxDelay_ = std::max(maxDelay_, p.second);\n> -               }\n> +       bool set(uint32_t ctrl, int32_t value);\n> +       bool set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList);\n> +       bool set(libcamera::ControlList &controls);\n>\n> -               init_ = true;\n> -       }\n> -\n> -       void reset()\n> -       {\n> -               std::lock_guard<std::mutex> lock(lock_);\n> -\n> -               int lastSetCount = std::max<int>(0, setCount_ - 1);\n> -               std::unordered_map<uint32_t, int32_t> lastVal;\n> -\n> -               /* Reset the counters. */\n> -               setCount_ = getCount_ = 0;\n> -\n> -               /* Look for the last set values. */\n> -               for (auto const &c : ctrl_)\n> -                       lastVal[c.first] = c.second[lastSetCount].value;\n> -\n> -               /* Apply the last set values as the next to be applied. */\n> -               ctrl_.clear();\n> -               for (auto &c : lastVal)\n> -                       ctrl_[c.first][setCount_] = CtrlInfo(c.second);\n> -       }\n> -\n> -       bool set(uint32_t ctrl, int32_t value)\n> -       {\n> -               std::lock_guard<std::mutex> lock(lock_);\n> -\n> -               /* Can we find this ctrl as one that is registered? */\n> -               if (delay_.find(ctrl) == delay_.end())\n> -                       return false;\n> -\n> -               ctrl_[ctrl][setCount_].value = value;\n> -               ctrl_[ctrl][setCount_].updated = true;\n> -\n> -               return true;\n> -       }\n> -\n> -       bool set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList)\n> -       {\n> -               std::lock_guard<std::mutex> lock(lock_);\n> -\n> -               for (auto const &p : ctrlList) {\n> -                       /* Can we find this ctrl? */\n> -                       if (delay_.find(p.first) == delay_.end())\n> -                               return false;\n> -\n> -                       ctrl_[p.first][setCount_] = CtrlInfo(p.second);\n> -               }\n> -\n> -               return true;\n> -       }\n> -\n> -       bool set(libcamera::ControlList &controls)\n> -       {\n> -               std::lock_guard<std::mutex> lock(lock_);\n> -\n> -               for (auto const &p : controls) {\n> -                       /* Can we find this ctrl? */\n> -                       if (delay_.find(p.first) == delay_.end())\n> -                               return false;\n> -\n> -                       ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>());\n> -                       LOG(RPI_S_W, Debug) << \"Setting ctrl \"\n> -                                           << hex(p.first) << \" to \"\n> -                                           << ctrl_[p.first][setCount_].value\n> -                                           << \" at index \"\n> -                                           << setCount_;\n> -               }\n> -\n> -               return true;\n> -       }\n> -\n> -       int write()\n> -       {\n> -               std::lock_guard<std::mutex> lock(lock_);\n> -               libcamera::ControlList controls(dev_->controls());\n> -\n> -               for (auto &p : ctrl_) {\n> -                       int delayDiff = maxDelay_ - delay_[p.first];\n> -                       int index = std::max<int>(0, setCount_ - delayDiff);\n> -\n> -                       if (p.second[index].updated) {\n> -                               /* We need to write this value out. */\n> -                               controls.set(p.first, p.second[index].value);\n> -                               p.second[index].updated = false;\n> -                               LOG(RPI_S_W, Debug) << \"Writing ctrl \"\n> -                                                   << hex(p.first) << \" to \"\n> -                                                   << p.second[index].value\n> -                                                   << \" at index \"\n> -                                                   << index;\n> -                       }\n> -               }\n> -\n> -               nextFrame();\n> -               return dev_->setControls(&controls);\n> -       }\n> -\n> -       void get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t offset = 0)\n> -       {\n> -               std::lock_guard<std::mutex> lock(lock_);\n> -\n> -               /* Account for the offset to reset the getCounter. */\n> -               getCount_ += offset + 1;\n> -\n> -               ctrl.clear();\n> -               for (auto &p : ctrl_) {\n> -                       int index = std::max<int>(0, getCount_ - maxDelay_);\n> -                       ctrl[p.first] = p.second[index].value;\n> -                       LOG(RPI_S_W, Debug) << \"Getting ctrl \"\n> -                                           << hex(p.first) << \" to \"\n> -                                           << p.second[index].value\n> -                                           << \" at index \"\n> -                                           << index;\n> -               }\n> -       }\n> +       int write();\n>\n>  private:\n> -       void nextFrame()\n> -       {\n> -               /* Advance the control history to the next frame */\n> -               int prevCount = setCount_;\n> -               setCount_++;\n> -\n> -               LOG(RPI_S_W, Debug) << \"Next frame, set index is \" << setCount_;\n> -\n> -               for (auto &p : ctrl_) {\n> -                       p.second[setCount_].value = p.second[prevCount].value;\n> -                       p.second[setCount_].updated = false;\n> -               }\n> -       }\n> +       void nextFrame();\n>\n>         /* listSize must be a power of 2. */\n>         static constexpr int listSize = (1 << 4);\n> --\n\nSorry about the delay.  I did see the email, but it just got lost in my inbox!\n\nReviewed-by: Naushir Patuck <naush@raspberrypi.com>\n\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"<naush@raspberrypi.com>","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 701C660DF2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 14 May 2020 16:55:40 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id j3so3836043ljg.8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 14 May 2020 07:55:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"f68h+g5X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=37pAh2pHWseDJjN3XPql9LbAlfMsLinxdz6M+7aKvWI=;\n\tb=f68h+g5X7MXsUQMv/ZSnXbcOR5Qr1j58wPeKdkRVei8hX8bM3rp9kdiQ4i+viCl5Q4\n\ttyqx0C06moipDaavl5uj3E8V/2v0dXIsYxJD3WcvDShlW8EFfGwtDzkN74s9KtkEokLg\n\t9kgjPZ7HFEg3UmxGn+M07e2jA40URFG10eEChbg409+GtcLC5dKE7WXZV5eUZ54PvFWH\n\tSQTlWIRvybX4SE+5Jd3NlJbytnY9E/16EG0MsLg5NYs7q7OJHThJ7QRvCvdffO+z9c8N\n\tZvOEw5QodB+q1uH+MbcwIXymd/arhG0nalE79+jpwqScfpl+/ZZ72CpKRuQYYqSBIINg\n\teGcA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=37pAh2pHWseDJjN3XPql9LbAlfMsLinxdz6M+7aKvWI=;\n\tb=MPxrq9pnOXJrSKFr+E5RxgOwteaCuT/rPK5QE21Pz1rK7WV8w+JxTIS1lKW38MtJs2\n\teiIKNd/oe5U0N9gMZyaKYHNgDa7V878ZxLf8/4cEb54EtrmfPue3b3Ni3+MCsFHJ1lJ4\n\tr+Dlm2AQ91tcTF6Zud3go8va/Fi2Gh46WCcXeRYEsYVESyUxYtWTQoYk06SVEkgBTITh\n\tfQJUSbbWR1VpxR+tUv+/e9oP2TyZUb4+0M4I4bB9hFSrZ43ODbSdIfaPjNrAJQS+b5Zp\n\tI0I3FjvnyjNsgV1nxle68DG7iT5GIhG/pxOH3WOEtNE4ZiL+lFzRxS51Vf6OSOc9/zJa\n\tKDIg==","X-Gm-Message-State":"AOAM533hRNZqgKqLxwaAoBD255CUdf1dkotU9twAEsi+hLnL/kTxK5TB\n\trIWm4UwpjGpolfjG7UGhPEKfTq68Gkz8GyugbASb89AHMOI=","X-Google-Smtp-Source":"ABdhPJwLpL/J1vdtb7e8D5KE4gDS+CAvgy2M++hMzYOQoZnyDzpvQy0KIMtRJ/KI059LF09YTSoRYyIzBWj4PmKe/Vc=","X-Received":"by 2002:a2e:3517:: with SMTP id\n\tz23mr3035431ljz.147.1589468139150; \n\tThu, 14 May 2020 07:55:39 -0700 (PDT)","MIME-Version":"1.0","References":"<20200512003903.20986-1-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20200512003903.20986-1-laurent.pinchart@ideasonboard.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Thu, 14 May 2020 15:55:23 +0100","Message-ID":"<CAEmqJPrVYRqAQofHG8V2xWi1HyRvFq2ZqPHjAPtQvZhF8FDhNQ@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: pipeline: raspberrypi:\n\tDon't inline all of StaggeredCtrl","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>","X-List-Received-Date":"Thu, 14 May 2020 14:55:40 -0000"}}]