[{"id":14252,"web_url":"https://patchwork.libcamera.org/comment/14252/","msgid":"","date":"2020-12-15T13:53:12","subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Niklas,\n\nThank you for your patch.\n\nOn Tue, 15 Dec 2020 at 00:48, Niklas Söderlund <\nniklas.soderlund@ragnatech.se> wrote:\n\n> Some sensor controls take effect with a delay as the sensor needs time\n> to adjust, for example exposure. Add an optional helper DelayedControls\n> to help pipelines deal with such controls.\n>\n> The idea is to provide a queue of controls towards the V4L2 device and\n> apply individual controls with the specified delay with the aim to get\n> predictable and retrievable control values for any given frame. To do\n> this the queue of controls needs to be at least as deep as the control\n> with the largest delay.\n>\n> The DelayedControls needs to be informed of every start of exposure.\n> This can be emulated but the helper is designed to be used with this\n> event being provide by the kernel thru V4L2 events.\n>\n> This helper is based on StaggeredCtrl from the Raspberry Pi pipeline\n> handler but expands on its API. This helpers aims to replace the\n> Raspberry Pi implementations and mimics it behavior perfectly.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> * Changes since v2\n> - Drop optional argument to reset().\n> - Update commit message.\n> - Remove usage of Mutex.\n> - Rename frameStart() to applyControls)_.\n> - Rename ControlInfo to Into.\n> - Rename ControlArray to ControlRingBuffer.\n> - Drop ControlsDelays and ControlsValues.\n> - Sort headers.\n> - Rename iterators.\n> - Simplify queueCount_ handeling in reset().\n> - Add more warnings.\n> - Update documentation.\n>\n> * Changes since v2\n> - Improve error logic in queue() as suggested by Jean-Michel Hautbois.\n> - s/fistSequence_/firstSequence_/\n>\n> * Changes since v1\n> - Correct copyright to reflect work is derived from Raspberry Pi\n> pipeline handler. This was always the intention and was wrong in v1.\n> - Rewrite large parts of the documentation.\n> - Join two loops to one in DelayedControls::DelayedControls()\n> ---\n> include/libcamera/internal/delayed_controls.h | 82 ++++++\n> src/libcamera/delayed_controls.cpp | 252 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 335 insertions(+)\n> create mode 100644 include/libcamera/internal/delayed_controls.h\n> create mode 100644 src/libcamera/delayed_controls.cpp\n>\n> diff --git a/include/libcamera/internal/delayed_controls.h\n> b/include/libcamera/internal/delayed_controls.h\n> new file mode 100644\n> index 0000000000000000..1292b484ec9f53e9\n> --- /dev/null\n> +++ b/include/libcamera/internal/delayed_controls.h\n> @@ -0,0 +1,82 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * delayed_controls.h - Helper to deal with controls that are applied\n> with a delay\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class V4L2Device;\n> +\n> +class DelayedControls\n> +{\n> +public:\n> + DelayedControls(V4L2Device *device,\n> + const std::unordered_map\n> &delays);\n> +\n> + void reset();\n> +\n> + bool push(const ControlList &controls);\n> + ControlList get(uint32_t sequence);\n> +\n> + void applyControls(uint32_t sequence);\n> +\n> +private:\n> + class Info\n> + {\n> + public:\n> + Info()\n> + : updated(false)\n> + {\n> + }\n> +\n> + Info(const ControlValue &v)\n> + : value(v), updated(true)\n> + {\n> + }\n> +\n> + ControlValue value;\n> + bool updated;\n> + };\n> +\n> + /* \\todo: Make the listSize configurable at instance creation\n> time. */\n> + static constexpr int listSize = 16;\n> + class ControlRingBuffer : public std::array\n> + {\n> + public:\n> + Info &operator[](unsigned int index)\n> + {\n> + return std::array listSize>::operator[](index % listSize);\n> + }\n> +\n> + const Info &operator[](unsigned int index) const\n> + {\n> + return std::array listSize>::operator[](index % listSize);\n> + }\n> + };\n> +\n> + bool queue(const ControlList &controls);\n> +\n> + V4L2Device *device_;\n> + std::unordered_map delays_;\n> + unsigned int maxDelay_;\n> +\n> + bool running_;\n> + uint32_t firstSequence_;\n> +\n> + uint32_t queueCount_;\n> + uint32_t writeCount_;\n> + std::unordered_map values_;\n> +};\n>\n\nStaggeredCtrl has a mutex to protect access to state. I wasn't entirely\nsure that was needed. Presumably the SoF event calling applyControls() and\nall other public method access occur in the same thread context, so no\nprotection is needed? Do you foresee this ever changing?\n\n+\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */\n> diff --git a/src/libcamera/delayed_controls.cpp\n> b/src/libcamera/delayed_controls.cpp\n> new file mode 100644\n> index 0000000000000000..db2e51f8c93c4755\n> --- /dev/null\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -0,0 +1,252 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * delayed_controls.h - Helper to deal with controls that are applied\n> with a delay\n> + */\n> +\n> +#include \"libcamera/internal/delayed_controls.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/v4l2_device.h\"\n> +\n> +/**\n> + * \\file delayed_controls.h\n> + * \\brief Helper to deal with controls that are applied with a delay\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(DelayedControls)\n> +\n> +/**\n> + * \\class DelayedControls\n> + * \\brief Helper to deal with controls that take effect with a delay\n> + *\n> + * Some sensor controls take effect with a delay as the sensor needs time\n> to\n> + * adjust, for example exposure and focus. This is a helper class to deal\n> with\n> + * such controls and the intended users are pipeline handlers.\n> + *\n> + * The idea is to extend the concept of the buffer depth of a pipeline the\n> + * application needs to maintain to also cover controls. Just as with\n> buffer\n> + * depth if the application keeps the number of requests queued above the\n> + * control depth the controls are guaranteed to take effect for the\n> correct\n> + * request. The control depth is determined by the control with the\n> greatest\n> + * delay.\n> + */\n> +\n> +/**\n> + * \\brief Construct a DelayedControls instance\n> + * \\param[in] device The V4L2 device the controls have to be applied to\n> + * \\param[in] delays Map of the numerical V4L2 control ids to their\n> associated\n> + * delays (in frames)\n> + *\n> + * Only controls specified in \\a delays are handled. If it's desired to\n> mix\n> + * delayed controls and controls that take effect immediately the\n> immediate\n> + * controls must be listed in the \\a delays map with a delay value of 0.\n> + */\n> +DelayedControls::DelayedControls(V4L2Device *device,\n> + const std::unordered_map unsigned int> &delays)\n> + : device_(device), maxDelay_(0)\n> +{\n> + const ControlInfoMap &controls = device_->controls();\n> +\n> + /*\n> + * Create a map of control ids to delays for controls exposed by\n> the\n> + * device.\n> + */\n> + for (auto const &delay : delays) {\n> + auto it = controls.find(delay.first);\n> + if (it == controls.end()) {\n> + LOG(DelayedControls, Error)\n> + << \"Delay request for control id \"\n> + << utils::hex(delay.first)\n> + << \" but control is not exposed by device \"\n> + << device_->deviceNode();\n> + continue;\n> + }\n> +\n> + const ControlId *id = it->first;\n> +\n> + delays_[id] = delay.second;\n> +\n> + LOG(DelayedControls, Debug)\n> + << \"Set a delay of \" << delays_[id]\n> + << \" for \" << id->name();\n> +\n> + maxDelay_ = std::max(maxDelay_, delays_[id]);\n> + }\n> +\n> + reset();\n> +}\n> +\n> +/**\n> + * \\brief Reset state machine\n> + *\n> + * Resets the state machine to a starting position based on control values\n> + * retrieved from the device.\n> + */\n> +void DelayedControls::reset()\n> +{\n> + running_ = false;\n> + firstSequence_ = 0;\n> + queueCount_ = 1;\n> + writeCount_ = 0;\n> +\n> + /* Retrieve control as reported by the device. */\n> + std::vector ids;\n> + for (auto const &delay : delays_)\n> + ids.push_back(delay.first->id());\n> +\n> + ControlList controls = device_->getControls(ids);\n> +\n> + /* Seed the control queue with the controls reported by the\n> device. */\n> + values_.clear();\n> + for (const auto &ctrl : controls) {\n> + const ControlId *id =\n> device_->controls().idmap().at(ctrl.first);\n> + values_[id][0] = Info(ctrl.second);\n> + }\n>\n\nI think this may have been mentioned in one of the earlier rounds - this\nmay not be correct as all the controls available to the device may not be\nregistered with DelayedControls. In those cases, the values_ map lookup\nwould fail. You would need to validate that id is an available key in the\nmap.\n\n\n> +}\n> +\n> +/**\n> + * \\brief Push a set of controls on the queue\n> + * \\param[in] controls List of controls to add to the device queue\n> + *\n> + * Push a set of controls to the control queue. This increases the\n> control queue\n> + * depth by one.\n> + *\n> + * \\returns true if \\a controls are accepted, or false otherwise\n> + */\n> +bool DelayedControls::push(const ControlList &controls)\n> +{\n> + return queue(controls);\n> +}\n>\n\nApologies, I may have missed something obvious, but why do we have the\npush() and queue() methods when one calls the other?\n\n\n> +\n> +bool DelayedControls::queue(const ControlList &controls)\n> +{\n> + /* Copy state from previous frame. */\n> + for (auto &ctrl : values_) {\n> + Info &info = ctrl.second[queueCount_];\n> + info.value = values_[ctrl.first][queueCount_ - 1].value;\n> + info.updated = false;\n> + }\n> +\n> + /* Update with new controls. */\n> + const ControlIdMap &idmap = device_->controls().idmap();\n> + for (const auto &control : controls) {\n> + const auto &it = idmap.find(control.first);\n> + if (it == idmap.end()) {\n> + LOG(DelayedControls, Warning)\n> + << \"Unknown control \" << control.first;\n> + return false;\n> + }\n> +\n> + const ControlId *id = it->second;\n> +\n> + if (delays_.find(id) == delays_.end())\n> + return false;\n> +\n> + Info &info = values_[id][queueCount_];\n> +\n> + info.value = control.second;\n> + info.updated = true;\n> +\n> + LOG(DelayedControls, Debug)\n> + << \"Queuing \" << id->name()\n> + << \" to \" << info.value.toString()\n> + << \" at index \" << queueCount_;\n> + }\n> +\n> + queueCount_++;\n> +\n> + return true;\n> +}\n> +\n> +/**\n> + * \\brief Read back controls in effect at a sequence number\n> + * \\param[in] sequence The sequence number to get controls for\n> + *\n> + * Read back what controls where in effect at a specific sequence number.\n> The\n> + * history is a ring buffer of 16 entries where new and old values\n> coexist. It's\n> + * the callers responsibility to not read too old sequence numbers that\n> have been\n> + * pushed out of the history.\n> + *\n> + * Historic values are evicted by pushing new values onto the queue using\n> + * push(). The max history from the current sequence number that yields\n> valid\n> + * values are thus 16 minus number of controls pushed.\n> + *\n> + * \\return The controls at \\a sequence number\n> + */\n> +ControlList DelayedControls::get(uint32_t sequence)\n> +{\n> + uint32_t adjustedSeq = sequence - firstSequence_ + 1;\n> + unsigned int index = std::max(0, adjustedSeq - maxDelay_);\n> +\n> + ControlList out(device_->controls());\n> + for (const auto &ctrl : values_) {\n> + const ControlId *id = ctrl.first;\n> + const Info &info = ctrl.second[index];\n> +\n> + out.set(id->id(), info.value);\n> +\n> + LOG(DelayedControls, Debug)\n> + << \"Reading \" << id->name()\n> + << \" to \" << info.value.toString()\n> + << \" at index \" << index;\n> + }\n> +\n> + return out;\n> +}\n> +\n> +/**\n> + * \\brief Inform DelayedControls of the start of a new frame\n> + * \\param[in] sequence Sequence number of the frame that started\n> + *\n> + * Inform the state machine that a new frame has started and of its\n> sequence\n> + * number. Any user of these helpers is responsible to inform the helper\n> about\n> + * the start of any frame.This can be connected with ease to the start of\n> a\n> + * exposure (SOE) V4L2 event.\n> + */\n> +void DelayedControls::applyControls(uint32_t sequence)\n> +{\n> + LOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> +\n> + if (!running_) {\n> + firstSequence_ = sequence;\n> + running_ = true;\n> + }\n> +\n> + /*\n> + * Create control list peaking ahead in the value queue to ensure\n> + * values are set in time to satisfy the sensor delay.\n> + */\n> + ControlList out(device_->controls());\n> + for (const auto &ctrl : values_) {\n> + const ControlId *id = ctrl.first;\n> + unsigned int delayDiff = maxDelay_ - delays_[id];\n> + unsigned int index = std::max(0, writeCount_ -\n> delayDiff);\n> + const Info &info = ctrl.second[index];\n> +\n> + if (info.updated) {\n> + out.set(id->id(), info.value);\n> + LOG(DelayedControls, Debug)\n> + << \"Setting \" << id->name()\n> + << \" to \" << info.value.toString()\n> + << \" at index \" << index;\n> + }\n> + }\n> +\n> + writeCount_++;\n> +\n> + while (writeCount_ >= queueCount_) {\n> + LOG(DelayedControls, Debug)\n> + << \"Queue is empty, auto queue no-op.\";\n> + queue({});\n> + }\n> +\n> + device_->setControls(&out);\n>\n\nAs mentioned previously, this will need to separate out controls that must\nbe written immediately (e.g. VBLANK) instead of batched together, but that\ncan be added at a later stage.\n\nRegards,\nNaush\n\n\n\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -12,6 +12,7 @@ libcamera_sources = files([\n> 'controls.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> + 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> 'event_dispatcher.cpp',\n> --\n> 2.29.2\n>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6EEF3C0F1A\n\tfor ;\n\tTue, 15 Dec 2020 13:53:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE46961590;\n\tTue, 15 Dec 2020 14:53:30 +0100 (CET)","from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D212C61589\n\tfor ;\n\tTue, 15 Dec 2020 14:53:28 +0100 (CET)","by mail-lf1-x143.google.com with SMTP id m25so39469702lfc.11\n\tfor ;\n\tTue, 15 Dec 2020 05:53:28 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Em6yKnGv\"; 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=1v+R/XmQS5fJK/ifAm5HiQfYZEaIMJWk7PXvcxZONfY=;\n\tb=Em6yKnGvNn20QGYbA7tzaiaeb3xfI+lBzscGEdIgzli/ANNH7HmJxq4/F2IxAAzNUa\n\tiLS+QEhqX1DNDKvJ4UaaKPEQUlWVbsps/TzUJt7OONNnRIWIpF68hLFD3lYD7Ld/RgfB\n\tw6Hi6XV7r1xVd4xecwsp1vWRPFgdR306Ysz+v08KpV7ghM/1FNUNvCRhMUbGV5BfGlOc\n\t0cMN5PX5amMeFpKE3Kscw/TH3udR37NDRdr1dNDGq9+V2GAD21YVFZCrFCaq5YNpbjFB\n\tGoGDHodFfATfAbDaYla1D3KsENgvWPNL5LRkNSPgFuX4kQlK6tFoVj7MMZaLIYQJRIvm\n\tS5Cg==","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=1v+R/XmQS5fJK/ifAm5HiQfYZEaIMJWk7PXvcxZONfY=;\n\tb=QP5Krs02WZsutBQaByvl+ziiSK1GfJly0R2XXhgSMXVH12yhFuVC6xJkFora7fI4xk\n\ttM1u+H9K1ZnPT13kWL2iPsg0GTT/PjGwtvxkmcchwjLVQxbdXe9h6RJsSZUbqZxOQLmK\n\tAihDhbySziEBwvuXEjvz7VsR+l8tnE5SIjhqiQjQsuHON3QFggBCsj5lCnqdvoozkepP\n\tTmBIoYKzcaA7V4AhhS3eXNFYSAbocXq5MibwrgOacS8gy969mcliBa+lDTPVsCIBaKB6\n\tZxAGg3qiOL9SFsUhaI1sQBnhRZKBLXm2mbFpEW46a+tLjQEWrYs/g9R1DMW2WkYy0AIJ\n\tYoGA==","X-Gm-Message-State":"AOAM531+kIW58RMKZYE1f6vnzB6xpmZ0P1NyfBVMujjcDlHeI66RKWka\n\tTG3lrfyFvdInO/T7BS9GAbN06ymERMptieGSFYJZEQ==","X-Google-Smtp-Source":"ABdhPJxlTn2h8WTsUeHLSp6RGSpnj0keIbKvaYr9VMe6abiJG8DFHNjxurp8ADKuBDGATyksA5XMvezPcxMsVgFeEpk=","X-Received":"by 2002:a19:7f90:: with SMTP id\n\ta138mr11673249lfd.617.1608040408027; \n\tTue, 15 Dec 2020 05:53:28 -0800 (PST)","MIME-Version":"1.0","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-2-niklas.soderlund@ragnatech.se>","In-Reply-To":"<20201215004811.602429-2-niklas.soderlund@ragnatech.se>","From":"Naushir Patuck ","Date":"Tue, 15 Dec 2020 13:53:12 +0000","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============4768462126794654437==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14253,"web_url":"https://patchwork.libcamera.org/comment/14253/","msgid":"","date":"2020-12-15T16:53:21","subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Dec 15, 2020 at 01:53:12PM +0000, Naushir Patuck wrote:\n> On Tue, 15 Dec 2020 at 00:48, Niklas Söderlund wrote:\n> \n> > Some sensor controls take effect with a delay as the sensor needs time\n> > to adjust, for example exposure. Add an optional helper DelayedControls\n> > to help pipelines deal with such controls.\n> >\n> > The idea is to provide a queue of controls towards the V4L2 device and\n> > apply individual controls with the specified delay with the aim to get\n> > predictable and retrievable control values for any given frame. To do\n> > this the queue of controls needs to be at least as deep as the control\n> > with the largest delay.\n> >\n> > The DelayedControls needs to be informed of every start of exposure.\n> > This can be emulated but the helper is designed to be used with this\n> > event being provide by the kernel thru V4L2 events.\n> >\n> > This helper is based on StaggeredCtrl from the Raspberry Pi pipeline\n> > handler but expands on its API. This helpers aims to replace the\n> > Raspberry Pi implementations and mimics it behavior perfectly.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > * Changes since v2\n> > - Drop optional argument to reset().\n> > - Update commit message.\n> > - Remove usage of Mutex.\n> > - Rename frameStart() to applyControls)_.\n> > - Rename ControlInfo to Into.\n> > - Rename ControlArray to ControlRingBuffer.\n> > - Drop ControlsDelays and ControlsValues.\n> > - Sort headers.\n> > - Rename iterators.\n> > - Simplify queueCount_ handeling in reset().\n> > - Add more warnings.\n> > - Update documentation.\n> >\n> > * Changes since v2\n> > - Improve error logic in queue() as suggested by Jean-Michel Hautbois.\n> > - s/fistSequence_/firstSequence_/\n> >\n> > * Changes since v1\n> > - Correct copyright to reflect work is derived from Raspberry Pi\n> > pipeline handler. This was always the intention and was wrong in v1.\n> > - Rewrite large parts of the documentation.\n> > - Join two loops to one in DelayedControls::DelayedControls()\n> > ---\n> > include/libcamera/internal/delayed_controls.h | 82 ++++++\n> > src/libcamera/delayed_controls.cpp | 252 ++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 3 files changed, 335 insertions(+)\n> > create mode 100644 include/libcamera/internal/delayed_controls.h\n> > create mode 100644 src/libcamera/delayed_controls.cpp\n> >\n> > diff --git a/include/libcamera/internal/delayed_controls.h\n> > b/include/libcamera/internal/delayed_controls.h\n> > new file mode 100644\n> > index 0000000000000000..1292b484ec9f53e9\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/delayed_controls.h\n> > @@ -0,0 +1,82 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> > + */\n> > +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class V4L2Device;\n> > +\n> > +class DelayedControls\n> > +{\n> > +public:\n> > + DelayedControls(V4L2Device *device,\n> > + const std::unordered_map &delays);\n> > +\n> > + void reset();\n> > +\n> > + bool push(const ControlList &controls);\n> > + ControlList get(uint32_t sequence);\n> > +\n> > + void applyControls(uint32_t sequence);\n> > +\n> > +private:\n> > + class Info\n> > + {\n> > + public:\n> > + Info()\n> > + : updated(false)\n> > + {\n> > + }\n> > +\n> > + Info(const ControlValue &v)\n> > + : value(v), updated(true)\n> > + {\n> > + }\n> > +\n> > + ControlValue value;\n> > + bool updated;\n> > + };\n> > +\n> > + /* \\todo: Make the listSize configurable at instance creation time. */\n> > + static constexpr int listSize = 16;\n> > + class ControlRingBuffer : public std::array\n> > + {\n> > + public:\n> > + Info &operator[](unsigned int index)\n> > + {\n> > + return std::array::operator[](index % listSize);\n> > + }\n> > +\n> > + const Info &operator[](unsigned int index) const\n> > + {\n> > + return std::array::operator[](index % listSize);\n> > + }\n> > + };\n> > +\n> > + bool queue(const ControlList &controls);\n> > +\n> > + V4L2Device *device_;\n> > + std::unordered_map delays_;\n> > + unsigned int maxDelay_;\n> > +\n> > + bool running_;\n> > + uint32_t firstSequence_;\n> > +\n> > + uint32_t queueCount_;\n> > + uint32_t writeCount_;\n> > + std::unordered_map values_;\n> > +};\n> \n> StaggeredCtrl has a mutex to protect access to state. I wasn't entirely\n> sure that was needed. Presumably the SoF event calling applyControls() and\n> all other public method access occur in the same thread context, so no\n> protection is needed? Do you foresee this ever changing?\n\nOnce the camera is started, the pipeline handler is event-based, with\nall events generated in the same thread (before the camera gets started,\nthe configure call runs in the application thread). Right now we have a\nsingle thread that covers all pipeline handler instances, and we may\nmove to one thread per pipeline handler in the future (or possibly a\nthread pool of N threads for M pipeline handlers, with N < M), but not\nto a model where different events would be handled in different threads.\nThe mutex is thus unnecessary. Niklas had one in v3, and I've asked him\nto drop it.\n\nI'll let Niklas reply to the other questions below.\n\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */\n> > diff --git a/src/libcamera/delayed_controls.cpp\n> > b/src/libcamera/delayed_controls.cpp\n> > new file mode 100644\n> > index 0000000000000000..db2e51f8c93c4755\n> > --- /dev/null\n> > +++ b/src/libcamera/delayed_controls.cpp\n> > @@ -0,0 +1,252 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> > + */\n> > +\n> > +#include \"libcamera/internal/delayed_controls.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/log.h\"\n> > +#include \"libcamera/internal/v4l2_device.h\"\n> > +\n> > +/**\n> > + * \\file delayed_controls.h\n> > + * \\brief Helper to deal with controls that are applied with a delay\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(DelayedControls)\n> > +\n> > +/**\n> > + * \\class DelayedControls\n> > + * \\brief Helper to deal with controls that take effect with a delay\n> > + *\n> > + * Some sensor controls take effect with a delay as the sensor needs time to\n> > + * adjust, for example exposure and focus. This is a helper class to deal with\n> > + * such controls and the intended users are pipeline handlers.\n> > + *\n> > + * The idea is to extend the concept of the buffer depth of a pipeline the\n> > + * application needs to maintain to also cover controls. Just as with buffer\n> > + * depth if the application keeps the number of requests queued above the\n> > + * control depth the controls are guaranteed to take effect for the correct\n> > + * request. The control depth is determined by the control with the greatest\n> > + * delay.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a DelayedControls instance\n> > + * \\param[in] device The V4L2 device the controls have to be applied to\n> > + * \\param[in] delays Map of the numerical V4L2 control ids to their associated\n> > + * delays (in frames)\n> > + *\n> > + * Only controls specified in \\a delays are handled. If it's desired to mix\n> > + * delayed controls and controls that take effect immediately the immediate\n> > + * controls must be listed in the \\a delays map with a delay value of 0.\n> > + */\n> > +DelayedControls::DelayedControls(V4L2Device *device,\n> > + const std::unordered_map &delays)\n> > + : device_(device), maxDelay_(0)\n> > +{\n> > + const ControlInfoMap &controls = device_->controls();\n> > +\n> > + /*\n> > + * Create a map of control ids to delays for controls exposed by the\n> > + * device.\n> > + */\n> > + for (auto const &delay : delays) {\n> > + auto it = controls.find(delay.first);\n> > + if (it == controls.end()) {\n> > + LOG(DelayedControls, Error)\n> > + << \"Delay request for control id \"\n> > + << utils::hex(delay.first)\n> > + << \" but control is not exposed by device \"\n> > + << device_->deviceNode();\n> > + continue;\n> > + }\n> > +\n> > + const ControlId *id = it->first;\n> > +\n> > + delays_[id] = delay.second;\n> > +\n> > + LOG(DelayedControls, Debug)\n> > + << \"Set a delay of \" << delays_[id]\n> > + << \" for \" << id->name();\n> > +\n> > + maxDelay_ = std::max(maxDelay_, delays_[id]);\n> > + }\n> > +\n> > + reset();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Reset state machine\n> > + *\n> > + * Resets the state machine to a starting position based on control values\n> > + * retrieved from the device.\n> > + */\n> > +void DelayedControls::reset()\n> > +{\n> > + running_ = false;\n> > + firstSequence_ = 0;\n> > + queueCount_ = 1;\n> > + writeCount_ = 0;\n> > +\n> > + /* Retrieve control as reported by the device. */\n> > + std::vector ids;\n> > + for (auto const &delay : delays_)\n> > + ids.push_back(delay.first->id());\n> > +\n> > + ControlList controls = device_->getControls(ids);\n> > +\n> > + /* Seed the control queue with the controls reported by the device. */\n> > + values_.clear();\n> > + for (const auto &ctrl : controls) {\n> > + const ControlId *id = device_->controls().idmap().at(ctrl.first);\n> > + values_[id][0] = Info(ctrl.second);\n> > + }\n> \n> I think this may have been mentioned in one of the earlier rounds - this\n> may not be correct as all the controls available to the device may not be\n> registered with DelayedControls. In those cases, the values_ map lookup\n> would fail. You would need to validate that id is an available key in the\n> map.\n> \n> > +}\n> > +\n> > +/**\n> > + * \\brief Push a set of controls on the queue\n> > + * \\param[in] controls List of controls to add to the device queue\n> > + *\n> > + * Push a set of controls to the control queue. This increases the control queue\n> > + * depth by one.\n> > + *\n> > + * \\returns true if \\a controls are accepted, or false otherwise\n> > + */\n> > +bool DelayedControls::push(const ControlList &controls)\n> > +{\n> > + return queue(controls);\n> > +}\n> \n> Apologies, I may have missed something obvious, but why do we have the\n> push() and queue() methods when one calls the other?\n> \n> > +\n> > +bool DelayedControls::queue(const ControlList &controls)\n> > +{\n> > + /* Copy state from previous frame. */\n> > + for (auto &ctrl : values_) {\n> > + Info &info = ctrl.second[queueCount_];\n> > + info.value = values_[ctrl.first][queueCount_ - 1].value;\n> > + info.updated = false;\n> > + }\n> > +\n> > + /* Update with new controls. */\n> > + const ControlIdMap &idmap = device_->controls().idmap();\n> > + for (const auto &control : controls) {\n> > + const auto &it = idmap.find(control.first);\n> > + if (it == idmap.end()) {\n> > + LOG(DelayedControls, Warning)\n> > + << \"Unknown control \" << control.first;\n> > + return false;\n> > + }\n> > +\n> > + const ControlId *id = it->second;\n> > +\n> > + if (delays_.find(id) == delays_.end())\n> > + return false;\n> > +\n> > + Info &info = values_[id][queueCount_];\n> > +\n> > + info.value = control.second;\n> > + info.updated = true;\n> > +\n> > + LOG(DelayedControls, Debug)\n> > + << \"Queuing \" << id->name()\n> > + << \" to \" << info.value.toString()\n> > + << \" at index \" << queueCount_;\n> > + }\n> > +\n> > + queueCount_++;\n> > +\n> > + return true;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Read back controls in effect at a sequence number\n> > + * \\param[in] sequence The sequence number to get controls for\n> > + *\n> > + * Read back what controls where in effect at a specific sequence number. The\n> > + * history is a ring buffer of 16 entries where new and old values coexist. It's\n> > + * the callers responsibility to not read too old sequence numbers that have been\n> > + * pushed out of the history.\n> > + *\n> > + * Historic values are evicted by pushing new values onto the queue using\n> > + * push(). The max history from the current sequence number that yields valid\n> > + * values are thus 16 minus number of controls pushed.\n> > + *\n> > + * \\return The controls at \\a sequence number\n> > + */\n> > +ControlList DelayedControls::get(uint32_t sequence)\n> > +{\n> > + uint32_t adjustedSeq = sequence - firstSequence_ + 1;\n> > + unsigned int index = std::max(0, adjustedSeq - maxDelay_);\n> > +\n> > + ControlList out(device_->controls());\n> > + for (const auto &ctrl : values_) {\n> > + const ControlId *id = ctrl.first;\n> > + const Info &info = ctrl.second[index];\n> > +\n> > + out.set(id->id(), info.value);\n> > +\n> > + LOG(DelayedControls, Debug)\n> > + << \"Reading \" << id->name()\n> > + << \" to \" << info.value.toString()\n> > + << \" at index \" << index;\n> > + }\n> > +\n> > + return out;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Inform DelayedControls of the start of a new frame\n> > + * \\param[in] sequence Sequence number of the frame that started\n> > + *\n> > + * Inform the state machine that a new frame has started and of its sequence\n> > + * number. Any user of these helpers is responsible to inform the helper about\n> > + * the start of any frame.This can be connected with ease to the start of a\n> > + * exposure (SOE) V4L2 event.\n> > + */\n> > +void DelayedControls::applyControls(uint32_t sequence)\n> > +{\n> > + LOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> > +\n> > + if (!running_) {\n> > + firstSequence_ = sequence;\n> > + running_ = true;\n> > + }\n> > +\n> > + /*\n> > + * Create control list peaking ahead in the value queue to ensure\n> > + * values are set in time to satisfy the sensor delay.\n> > + */\n> > + ControlList out(device_->controls());\n> > + for (const auto &ctrl : values_) {\n> > + const ControlId *id = ctrl.first;\n> > + unsigned int delayDiff = maxDelay_ - delays_[id];\n> > + unsigned int index = std::max(0, writeCount_ - delayDiff);\n> > + const Info &info = ctrl.second[index];\n> > +\n> > + if (info.updated) {\n> > + out.set(id->id(), info.value);\n> > + LOG(DelayedControls, Debug)\n> > + << \"Setting \" << id->name()\n> > + << \" to \" << info.value.toString()\n> > + << \" at index \" << index;\n> > + }\n> > + }\n> > +\n> > + writeCount_++;\n> > +\n> > + while (writeCount_ >= queueCount_) {\n> > + LOG(DelayedControls, Debug)\n> > + << \"Queue is empty, auto queue no-op.\";\n> > + queue({});\n> > + }\n> > +\n> > + device_->setControls(&out);\n> \n> As mentioned previously, this will need to separate out controls that must\n> be written immediately (e.g. VBLANK) instead of batched together, but that\n> can be added at a later stage.\n> \n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -12,6 +12,7 @@ libcamera_sources = files([\n> > 'controls.cpp',\n> > 'control_serializer.cpp',\n> > 'control_validator.cpp',\n> > + 'delayed_controls.cpp',\n> > 'device_enumerator.cpp',\n> > 'device_enumerator_sysfs.cpp',\n> > 'event_dispatcher.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 91039C0F1A\n\tfor ;\n\tTue, 15 Dec 2020 16:53:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 25F986158A;\n\tTue, 15 Dec 2020 17:53: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 9FB9961589\n\tfor ;\n\tTue, 15 Dec 2020 17:53:28 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E99CA593;\n\tTue, 15 Dec 2020 17:53:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ecj4sZc7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1608051208;\n\tbh=XhALgWA44yWOljbmCdrhoVpqWOnH6yJSCw1je0iBPAI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ecj4sZc7uTM5LEVMWBETi8Q/T/F1lyBgT91AdZzbXOwxMmjma05O51+zsi9xek6Tk\n\teQers1WRlxibtEEayj2dpXU0k+nMrxozMLnyBYulqgZcZjFvw/TUvXHvX2K1CPyVDr\n\t1VoYdXzXxSBWcRxeib/Z5huLRYoUkxoKymZsdmxk=","Date":"Tue, 15 Dec 2020 18:53:21 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-2-niklas.soderlund@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14258,"web_url":"https://patchwork.libcamera.org/comment/14258/","msgid":"<20201217140859.7nctistw3nwzcy6e@uno.localdomain>","date":"2020-12-17T14:08:59","subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Tue, Dec 15, 2020 at 01:48:04AM +0100, Niklas Söderlund wrote:\n> Some sensor controls take effect with a delay as the sensor needs time\n> to adjust, for example exposure. Add an optional helper DelayedControls\n> to help pipelines deal with such controls.\n>\n> The idea is to provide a queue of controls towards the V4L2 device and\n> apply individual controls with the specified delay with the aim to get\n> predictable and retrievable control values for any given frame. To do\n> this the queue of controls needs to be at least as deep as the control\n> with the largest delay.\n>\n> The DelayedControls needs to be informed of every start of exposure.\n> This can be emulated but the helper is designed to be used with this\n> event being provide by the kernel thru V4L2 events.\n>\n> This helper is based on StaggeredCtrl from the Raspberry Pi pipeline\n> handler but expands on its API. This helpers aims to replace the\n> Raspberry Pi implementations and mimics it behavior perfectly.\n>\n> Signed-off-by: Niklas Söderlund \n\nI guess comments on v3 not addressed were deemed not requested for\nthis iteration:\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> * Changes since v2\n> - Drop optional argument to reset().\n> - Update commit message.\n> - Remove usage of Mutex.\n> - Rename frameStart() to applyControls)_.\n> - Rename ControlInfo to Into.\n> - Rename ControlArray to ControlRingBuffer.\n> - Drop ControlsDelays and ControlsValues.\n> - Sort headers.\n> - Rename iterators.\n> - Simplify queueCount_ handeling in reset().\n> - Add more warnings.\n> - Update documentation.\n>\n> * Changes since v2\n> - Improve error logic in queue() as suggested by Jean-Michel Hautbois.\n> - s/fistSequence_/firstSequence_/\n>\n> * Changes since v1\n> - Correct copyright to reflect work is derived from Raspberry Pi\n> pipeline handler. This was always the intention and was wrong in v1.\n> - Rewrite large parts of the documentation.\n> - Join two loops to one in DelayedControls::DelayedControls()\n> ---\n> include/libcamera/internal/delayed_controls.h | 82 ++++++\n> src/libcamera/delayed_controls.cpp | 252 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 335 insertions(+)\n> create mode 100644 include/libcamera/internal/delayed_controls.h\n> create mode 100644 src/libcamera/delayed_controls.cpp\n>\n> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> new file mode 100644\n> index 0000000000000000..1292b484ec9f53e9\n> --- /dev/null\n> +++ b/include/libcamera/internal/delayed_controls.h\n> @@ -0,0 +1,82 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class V4L2Device;\n> +\n> +class DelayedControls\n> +{\n> +public:\n> +\tDelayedControls(V4L2Device *device,\n> +\t\t\tconst std::unordered_map &delays);\n> +\n> +\tvoid reset();\n> +\n> +\tbool push(const ControlList &controls);\n> +\tControlList get(uint32_t sequence);\n> +\n> +\tvoid applyControls(uint32_t sequence);\n> +\n> +private:\n> +\tclass Info\n> +\t{\n> +\tpublic:\n> +\t\tInfo()\n> +\t\t\t: updated(false)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tInfo(const ControlValue &v)\n> +\t\t\t: value(v), updated(true)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tControlValue value;\n> +\t\tbool updated;\n> +\t};\n> +\n> +\t/* \\todo: Make the listSize configurable at instance creation time. */\n> +\tstatic constexpr int listSize = 16;\n> +\tclass ControlRingBuffer : public std::array\n> +\t{\n> +\tpublic:\n> +\t\tInfo &operator[](unsigned int index)\n> +\t\t{\n> +\t\t\treturn std::array::operator[](index % listSize);\n> +\t\t}\n> +\n> +\t\tconst Info &operator[](unsigned int index) const\n> +\t\t{\n> +\t\t\treturn std::array::operator[](index % listSize);\n> +\t\t}\n> +\t};\n> +\n> +\tbool queue(const ControlList &controls);\n> +\n> +\tV4L2Device *device_;\n> +\tstd::unordered_map delays_;\n> +\tunsigned int maxDelay_;\n> +\n> +\tbool running_;\n> +\tuint32_t firstSequence_;\n> +\n> +\tuint32_t queueCount_;\n> +\tuint32_t writeCount_;\n> +\tstd::unordered_map values_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */\n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> new file mode 100644\n> index 0000000000000000..db2e51f8c93c4755\n> --- /dev/null\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -0,0 +1,252 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> + */\n> +\n> +#include \"libcamera/internal/delayed_controls.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/v4l2_device.h\"\n> +\n> +/**\n> + * \\file delayed_controls.h\n> + * \\brief Helper to deal with controls that are applied with a delay\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(DelayedControls)\n> +\n> +/**\n> + * \\class DelayedControls\n> + * \\brief Helper to deal with controls that take effect with a delay\n> + *\n> + * Some sensor controls take effect with a delay as the sensor needs time to\n> + * adjust, for example exposure and focus. This is a helper class to deal with\n> + * such controls and the intended users are pipeline handlers.\n> + *\n> + * The idea is to extend the concept of the buffer depth of a pipeline the\n> + * application needs to maintain to also cover controls. Just as with buffer\n> + * depth if the application keeps the number of requests queued above the\n> + * control depth the controls are guaranteed to take effect for the correct\n> + * request. The control depth is determined by the control with the greatest\n> + * delay.\n> + */\n> +\n> +/**\n> + * \\brief Construct a DelayedControls instance\n> + * \\param[in] device The V4L2 device the controls have to be applied to\n> + * \\param[in] delays Map of the numerical V4L2 control ids to their associated\n> + * delays (in frames)\n> + *\n> + * Only controls specified in \\a delays are handled. If it's desired to mix\n> + * delayed controls and controls that take effect immediately the immediate\n> + * controls must be listed in the \\a delays map with a delay value of 0.\n> + */\n> +DelayedControls::DelayedControls(V4L2Device *device,\n> +\t\t\t\t const std::unordered_map &delays)\n> +\t: device_(device), maxDelay_(0)\n> +{\n> +\tconst ControlInfoMap &controls = device_->controls();\n> +\n> +\t/*\n> +\t * Create a map of control ids to delays for controls exposed by the\n> +\t * device.\n> +\t */\n> +\tfor (auto const &delay : delays) {\n> +\t\tauto it = controls.find(delay.first);\n> +\t\tif (it == controls.end()) {\n> +\t\t\tLOG(DelayedControls, Error)\n> +\t\t\t\t<< \"Delay request for control id \"\n> +\t\t\t\t<< utils::hex(delay.first)\n> +\t\t\t\t<< \" but control is not exposed by device \"\n> +\t\t\t\t<< device_->deviceNode();\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tconst ControlId *id = it->first;\n> +\n> +\t\tdelays_[id] = delay.second;\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Set a delay of \" << delays_[id]\n> +\t\t\t<< \" for \" << id->name();\n> +\n> +\t\tmaxDelay_ = std::max(maxDelay_, delays_[id]);\n> +\t}\n> +\n> +\treset();\n> +}\n> +\n> +/**\n> + * \\brief Reset state machine\n> + *\n> + * Resets the state machine to a starting position based on control values\n> + * retrieved from the device.\n> + */\n> +void DelayedControls::reset()\n> +{\n> +\trunning_ = false;\n> +\tfirstSequence_ = 0;\n> +\tqueueCount_ = 1;\n> +\twriteCount_ = 0;\n> +\n> +\t/* Retrieve control as reported by the device. */\n> +\tstd::vector ids;\n> +\tfor (auto const &delay : delays_)\n> +\t\tids.push_back(delay.first->id());\n> +\n> +\tControlList controls = device_->getControls(ids);\n> +\n> +\t/* Seed the control queue with the controls reported by the device. */\n> +\tvalues_.clear();\n> +\tfor (const auto &ctrl : controls) {\n> +\t\tconst ControlId *id = device_->controls().idmap().at(ctrl.first);\n> +\t\tvalues_[id][0] = Info(ctrl.second);\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief Push a set of controls on the queue\n> + * \\param[in] controls List of controls to add to the device queue\n> + *\n> + * Push a set of controls to the control queue. This increases the control queue\n> + * depth by one.\n> + *\n> + * \\returns true if \\a controls are accepted, or false otherwise\n> + */\n> +bool DelayedControls::push(const ControlList &controls)\n> +{\n> +\treturn queue(controls);\n> +}\n> +\n> +bool DelayedControls::queue(const ControlList &controls)\n> +{\n> +\t/* Copy state from previous frame. */\n> +\tfor (auto &ctrl : values_) {\n> +\t\tInfo &info = ctrl.second[queueCount_];\n> +\t\tinfo.value = values_[ctrl.first][queueCount_ - 1].value;\n> +\t\tinfo.updated = false;\n> +\t}\n> +\n> +\t/* Update with new controls. */\n> +\tconst ControlIdMap &idmap = device_->controls().idmap();\n> +\tfor (const auto &control : controls) {\n> +\t\tconst auto &it = idmap.find(control.first);\n> +\t\tif (it == idmap.end()) {\n> +\t\t\tLOG(DelayedControls, Warning)\n> +\t\t\t\t<< \"Unknown control \" << control.first;\n> +\t\t\treturn false;\n> +\t\t}\n> +\n> +\t\tconst ControlId *id = it->second;\n> +\n> +\t\tif (delays_.find(id) == delays_.end())\n> +\t\t\treturn false;\n> +\n> +\t\tInfo &info = values_[id][queueCount_];\n> +\n> +\t\tinfo.value = control.second;\n> +\t\tinfo.updated = true;\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Queuing \" << id->name()\n> +\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t<< \" at index \" << queueCount_;\n> +\t}\n> +\n> +\tqueueCount_++;\n> +\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\brief Read back controls in effect at a sequence number\n> + * \\param[in] sequence The sequence number to get controls for\n> + *\n> + * Read back what controls where in effect at a specific sequence number. The\n> + * history is a ring buffer of 16 entries where new and old values coexist. It's\n> + * the callers responsibility to not read too old sequence numbers that have been\n> + * pushed out of the history.\n> + *\n> + * Historic values are evicted by pushing new values onto the queue using\n> + * push(). The max history from the current sequence number that yields valid\n> + * values are thus 16 minus number of controls pushed.\n> + *\n> + * \\return The controls at \\a sequence number\n> + */\n> +ControlList DelayedControls::get(uint32_t sequence)\n> +{\n> +\tuint32_t adjustedSeq = sequence - firstSequence_ + 1;\n> +\tunsigned int index = std::max(0, adjustedSeq - maxDelay_);\n> +\n> +\tControlList out(device_->controls());\n> +\tfor (const auto &ctrl : values_) {\n> +\t\tconst ControlId *id = ctrl.first;\n> +\t\tconst Info &info = ctrl.second[index];\n> +\n> +\t\tout.set(id->id(), info.value);\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Reading \" << id->name()\n> +\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t<< \" at index \" << index;\n> +\t}\n> +\n> +\treturn out;\n> +}\n> +\n> +/**\n> + * \\brief Inform DelayedControls of the start of a new frame\n> + * \\param[in] sequence Sequence number of the frame that started\n> + *\n> + * Inform the state machine that a new frame has started and of its sequence\n> + * number. Any user of these helpers is responsible to inform the helper about\n> + * the start of any frame.This can be connected with ease to the start of a\n> + * exposure (SOE) V4L2 event.\n> + */\n> +void DelayedControls::applyControls(uint32_t sequence)\n> +{\n> +\tLOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> +\n> +\tif (!running_) {\n> +\t\tfirstSequence_ = sequence;\n> +\t\trunning_ = true;\n> +\t}\n> +\n> +\t/*\n> +\t * Create control list peaking ahead in the value queue to ensure\n> +\t * values are set in time to satisfy the sensor delay.\n> +\t */\n> +\tControlList out(device_->controls());\n> +\tfor (const auto &ctrl : values_) {\n> +\t\tconst ControlId *id = ctrl.first;\n> +\t\tunsigned int delayDiff = maxDelay_ - delays_[id];\n> +\t\tunsigned int index = std::max(0, writeCount_ - delayDiff);\n> +\t\tconst Info &info = ctrl.second[index];\n> +\n> +\t\tif (info.updated) {\n> +\t\t\tout.set(id->id(), info.value);\n> +\t\t\tLOG(DelayedControls, Debug)\n> +\t\t\t\t<< \"Setting \" << id->name()\n> +\t\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t\t<< \" at index \" << index;\n> +\t\t}\n> +\t}\n> +\n> +\twriteCount_++;\n> +\n> +\twhile (writeCount_ >= queueCount_) {\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Queue is empty, auto queue no-op.\";\n> +\t\tqueue({});\n> +\t}\n> +\n> +\tdevice_->setControls(&out);\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -12,6 +12,7 @@ libcamera_sources = files([\n> 'controls.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> + 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> 'event_dispatcher.cpp',\n> --\n> 2.29.2\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 77009C0F1A\n\tfor ;\n\tThu, 17 Dec 2020 14:08:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE75261595;\n\tThu, 17 Dec 2020 15:08:51 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F24B161591\n\tfor ;\n\tThu, 17 Dec 2020 15:08:49 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id DDAB42000F;\n\tThu, 17 Dec 2020 14:08:48 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 17 Dec 2020 15:08:59 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201217140859.7nctistw3nwzcy6e@uno.localdomain>","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201215004811.602429-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14477,"web_url":"https://patchwork.libcamera.org/comment/14477/","msgid":"","date":"2021-01-08T15:21:17","subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Naushir,\n\nThanks for your comments.\n\nOn 2020-12-15 13:53:12 +0000, Naushir Patuck wrote:\n> Hi Niklas,\n> \n> Thank you for your patch.\n> \n> On Tue, 15 Dec 2020 at 00:48, Niklas Söderlund <\n> niklas.soderlund@ragnatech.se> wrote:\n> \n> > Some sensor controls take effect with a delay as the sensor needs time\n> > to adjust, for example exposure. Add an optional helper DelayedControls\n> > to help pipelines deal with such controls.\n> >\n> > The idea is to provide a queue of controls towards the V4L2 device and\n> > apply individual controls with the specified delay with the aim to get\n> > predictable and retrievable control values for any given frame. To do\n> > this the queue of controls needs to be at least as deep as the control\n> > with the largest delay.\n> >\n> > The DelayedControls needs to be informed of every start of exposure.\n> > This can be emulated but the helper is designed to be used with this\n> > event being provide by the kernel thru V4L2 events.\n> >\n> > This helper is based on StaggeredCtrl from the Raspberry Pi pipeline\n> > handler but expands on its API. This helpers aims to replace the\n> > Raspberry Pi implementations and mimics it behavior perfectly.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > * Changes since v2\n> > - Drop optional argument to reset().\n> > - Update commit message.\n> > - Remove usage of Mutex.\n> > - Rename frameStart() to applyControls)_.\n> > - Rename ControlInfo to Into.\n> > - Rename ControlArray to ControlRingBuffer.\n> > - Drop ControlsDelays and ControlsValues.\n> > - Sort headers.\n> > - Rename iterators.\n> > - Simplify queueCount_ handeling in reset().\n> > - Add more warnings.\n> > - Update documentation.\n> >\n> > * Changes since v2\n> > - Improve error logic in queue() as suggested by Jean-Michel Hautbois.\n> > - s/fistSequence_/firstSequence_/\n> >\n> > * Changes since v1\n> > - Correct copyright to reflect work is derived from Raspberry Pi\n> > pipeline handler. This was always the intention and was wrong in v1.\n> > - Rewrite large parts of the documentation.\n> > - Join two loops to one in DelayedControls::DelayedControls()\n> > ---\n> > include/libcamera/internal/delayed_controls.h | 82 ++++++\n> > src/libcamera/delayed_controls.cpp | 252 ++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 3 files changed, 335 insertions(+)\n> > create mode 100644 include/libcamera/internal/delayed_controls.h\n> > create mode 100644 src/libcamera/delayed_controls.cpp\n> >\n> > diff --git a/include/libcamera/internal/delayed_controls.h\n> > b/include/libcamera/internal/delayed_controls.h\n> > new file mode 100644\n> > index 0000000000000000..1292b484ec9f53e9\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/delayed_controls.h\n> > @@ -0,0 +1,82 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * delayed_controls.h - Helper to deal with controls that are applied\n> > with a delay\n> > + */\n> > +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class V4L2Device;\n> > +\n> > +class DelayedControls\n> > +{\n> > +public:\n> > + DelayedControls(V4L2Device *device,\n> > + const std::unordered_map\n> > &delays);\n> > +\n> > + void reset();\n> > +\n> > + bool push(const ControlList &controls);\n> > + ControlList get(uint32_t sequence);\n> > +\n> > + void applyControls(uint32_t sequence);\n> > +\n> > +private:\n> > + class Info\n> > + {\n> > + public:\n> > + Info()\n> > + : updated(false)\n> > + {\n> > + }\n> > +\n> > + Info(const ControlValue &v)\n> > + : value(v), updated(true)\n> > + {\n> > + }\n> > +\n> > + ControlValue value;\n> > + bool updated;\n> > + };\n> > +\n> > + /* \\todo: Make the listSize configurable at instance creation\n> > time. */\n> > + static constexpr int listSize = 16;\n> > + class ControlRingBuffer : public std::array\n> > + {\n> > + public:\n> > + Info &operator[](unsigned int index)\n> > + {\n> > + return std::array > listSize>::operator[](index % listSize);\n> > + }\n> > +\n> > + const Info &operator[](unsigned int index) const\n> > + {\n> > + return std::array > listSize>::operator[](index % listSize);\n> > + }\n> > + };\n> > +\n> > + bool queue(const ControlList &controls);\n> > +\n> > + V4L2Device *device_;\n> > + std::unordered_map delays_;\n> > + unsigned int maxDelay_;\n> > +\n> > + bool running_;\n> > + uint32_t firstSequence_;\n> > +\n> > + uint32_t queueCount_;\n> > + uint32_t writeCount_;\n> > + std::unordered_map values_;\n> > +};\n> >\n> \n> StaggeredCtrl has a mutex to protect access to state. I wasn't entirely\n> sure that was needed. Presumably the SoF event calling applyControls() and\n> all other public method access occur in the same thread context, so no\n> protection is needed? Do you foresee this ever changing?\n\nAs Laurent already replied, the mutex is not needed in out current \ndesign.\n\n> \n> +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */\n> > diff --git a/src/libcamera/delayed_controls.cpp\n> > b/src/libcamera/delayed_controls.cpp\n> > new file mode 100644\n> > index 0000000000000000..db2e51f8c93c4755\n> > --- /dev/null\n> > +++ b/src/libcamera/delayed_controls.cpp\n> > @@ -0,0 +1,252 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * delayed_controls.h - Helper to deal with controls that are applied\n> > with a delay\n> > + */\n> > +\n> > +#include \"libcamera/internal/delayed_controls.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/log.h\"\n> > +#include \"libcamera/internal/v4l2_device.h\"\n> > +\n> > +/**\n> > + * \\file delayed_controls.h\n> > + * \\brief Helper to deal with controls that are applied with a delay\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(DelayedControls)\n> > +\n> > +/**\n> > + * \\class DelayedControls\n> > + * \\brief Helper to deal with controls that take effect with a delay\n> > + *\n> > + * Some sensor controls take effect with a delay as the sensor needs time\n> > to\n> > + * adjust, for example exposure and focus. This is a helper class to deal\n> > with\n> > + * such controls and the intended users are pipeline handlers.\n> > + *\n> > + * The idea is to extend the concept of the buffer depth of a pipeline the\n> > + * application needs to maintain to also cover controls. Just as with\n> > buffer\n> > + * depth if the application keeps the number of requests queued above the\n> > + * control depth the controls are guaranteed to take effect for the\n> > correct\n> > + * request. The control depth is determined by the control with the\n> > greatest\n> > + * delay.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a DelayedControls instance\n> > + * \\param[in] device The V4L2 device the controls have to be applied to\n> > + * \\param[in] delays Map of the numerical V4L2 control ids to their\n> > associated\n> > + * delays (in frames)\n> > + *\n> > + * Only controls specified in \\a delays are handled. If it's desired to\n> > mix\n> > + * delayed controls and controls that take effect immediately the\n> > immediate\n> > + * controls must be listed in the \\a delays map with a delay value of 0.\n> > + */\n> > +DelayedControls::DelayedControls(V4L2Device *device,\n> > + const std::unordered_map > unsigned int> &delays)\n> > + : device_(device), maxDelay_(0)\n> > +{\n> > + const ControlInfoMap &controls = device_->controls();\n> > +\n> > + /*\n> > + * Create a map of control ids to delays for controls exposed by\n> > the\n> > + * device.\n> > + */\n> > + for (auto const &delay : delays) {\n> > + auto it = controls.find(delay.first);\n> > + if (it == controls.end()) {\n> > + LOG(DelayedControls, Error)\n> > + << \"Delay request for control id \"\n> > + << utils::hex(delay.first)\n> > + << \" but control is not exposed by device \"\n> > + << device_->deviceNode();\n> > + continue;\n> > + }\n> > +\n> > + const ControlId *id = it->first;\n> > +\n> > + delays_[id] = delay.second;\n> > +\n> > + LOG(DelayedControls, Debug)\n> > + << \"Set a delay of \" << delays_[id]\n> > + << \" for \" << id->name();\n> > +\n> > + maxDelay_ = std::max(maxDelay_, delays_[id]);\n> > + }\n> > +\n> > + reset();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Reset state machine\n> > + *\n> > + * Resets the state machine to a starting position based on control values\n> > + * retrieved from the device.\n> > + */\n> > +void DelayedControls::reset()\n> > +{\n> > + running_ = false;\n> > + firstSequence_ = 0;\n> > + queueCount_ = 1;\n> > + writeCount_ = 0;\n> > +\n> > + /* Retrieve control as reported by the device. */\n> > + std::vector ids;\n> > + for (auto const &delay : delays_)\n> > + ids.push_back(delay.first->id());\n> > +\n> > + ControlList controls = device_->getControls(ids);\n> > +\n> > + /* Seed the control queue with the controls reported by the\n> > device. */\n> > + values_.clear();\n> > + for (const auto &ctrl : controls) {\n> > + const ControlId *id =\n> > device_->controls().idmap().at(ctrl.first);\n> > + values_[id][0] = Info(ctrl.second);\n> > + }\n> >\n> \n> I think this may have been mentioned in one of the earlier rounds - this\n> may not be correct as all the controls available to the device may not be\n> registered with DelayedControls. In those cases, the values_ map lookup\n> would fail. You would need to validate that id is an available key in the\n> map.\n\nIs this not already done? The 'controls' that are iterated in this loop \nis constructed from the IDs in delays_ which only contains the controls \nwith an associated delay.\n\n> \n> \n> > +}\n> > +\n> > +/**\n> > + * \\brief Push a set of controls on the queue\n> > + * \\param[in] controls List of controls to add to the device queue\n> > + *\n> > + * Push a set of controls to the control queue. This increases the\n> > control queue\n> > + * depth by one.\n> > + *\n> > + * \\returns true if \\a controls are accepted, or false otherwise\n> > + */\n> > +bool DelayedControls::push(const ControlList &controls)\n> > +{\n> > + return queue(controls);\n> > +}\n> >\n> \n> Apologies, I may have missed something obvious, but why do we have the\n> push() and queue() methods when one calls the other?\n\nGood call! This is a leftover from when there was a mutex that needed to \nbe locked when called from the public API but not when the functionality \nwas used internally where the mutex was already locked. Will fold these \ntwo together.\n\n> \n> \n> > +\n> > +bool DelayedControls::queue(const ControlList &controls)\n> > +{\n> > + /* Copy state from previous frame. */\n> > + for (auto &ctrl : values_) {\n> > + Info &info = ctrl.second[queueCount_];\n> > + info.value = values_[ctrl.first][queueCount_ - 1].value;\n> > + info.updated = false;\n> > + }\n> > +\n> > + /* Update with new controls. */\n> > + const ControlIdMap &idmap = device_->controls().idmap();\n> > + for (const auto &control : controls) {\n> > + const auto &it = idmap.find(control.first);\n> > + if (it == idmap.end()) {\n> > + LOG(DelayedControls, Warning)\n> > + << \"Unknown control \" << control.first;\n> > + return false;\n> > + }\n> > +\n> > + const ControlId *id = it->second;\n> > +\n> > + if (delays_.find(id) == delays_.end())\n> > + return false;\n> > +\n> > + Info &info = values_[id][queueCount_];\n> > +\n> > + info.value = control.second;\n> > + info.updated = true;\n> > +\n> > + LOG(DelayedControls, Debug)\n> > + << \"Queuing \" << id->name()\n> > + << \" to \" << info.value.toString()\n> > + << \" at index \" << queueCount_;\n> > + }\n> > +\n> > + queueCount_++;\n> > +\n> > + return true;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Read back controls in effect at a sequence number\n> > + * \\param[in] sequence The sequence number to get controls for\n> > + *\n> > + * Read back what controls where in effect at a specific sequence number.\n> > The\n> > + * history is a ring buffer of 16 entries where new and old values\n> > coexist. It's\n> > + * the callers responsibility to not read too old sequence numbers that\n> > have been\n> > + * pushed out of the history.\n> > + *\n> > + * Historic values are evicted by pushing new values onto the queue using\n> > + * push(). The max history from the current sequence number that yields\n> > valid\n> > + * values are thus 16 minus number of controls pushed.\n> > + *\n> > + * \\return The controls at \\a sequence number\n> > + */\n> > +ControlList DelayedControls::get(uint32_t sequence)\n> > +{\n> > + uint32_t adjustedSeq = sequence - firstSequence_ + 1;\n> > + unsigned int index = std::max(0, adjustedSeq - maxDelay_);\n> > +\n> > + ControlList out(device_->controls());\n> > + for (const auto &ctrl : values_) {\n> > + const ControlId *id = ctrl.first;\n> > + const Info &info = ctrl.second[index];\n> > +\n> > + out.set(id->id(), info.value);\n> > +\n> > + LOG(DelayedControls, Debug)\n> > + << \"Reading \" << id->name()\n> > + << \" to \" << info.value.toString()\n> > + << \" at index \" << index;\n> > + }\n> > +\n> > + return out;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Inform DelayedControls of the start of a new frame\n> > + * \\param[in] sequence Sequence number of the frame that started\n> > + *\n> > + * Inform the state machine that a new frame has started and of its\n> > sequence\n> > + * number. Any user of these helpers is responsible to inform the helper\n> > about\n> > + * the start of any frame.This can be connected with ease to the start of\n> > a\n> > + * exposure (SOE) V4L2 event.\n> > + */\n> > +void DelayedControls::applyControls(uint32_t sequence)\n> > +{\n> > + LOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> > +\n> > + if (!running_) {\n> > + firstSequence_ = sequence;\n> > + running_ = true;\n> > + }\n> > +\n> > + /*\n> > + * Create control list peaking ahead in the value queue to ensure\n> > + * values are set in time to satisfy the sensor delay.\n> > + */\n> > + ControlList out(device_->controls());\n> > + for (const auto &ctrl : values_) {\n> > + const ControlId *id = ctrl.first;\n> > + unsigned int delayDiff = maxDelay_ - delays_[id];\n> > + unsigned int index = std::max(0, writeCount_ -\n> > delayDiff);\n> > + const Info &info = ctrl.second[index];\n> > +\n> > + if (info.updated) {\n> > + out.set(id->id(), info.value);\n> > + LOG(DelayedControls, Debug)\n> > + << \"Setting \" << id->name()\n> > + << \" to \" << info.value.toString()\n> > + << \" at index \" << index;\n> > + }\n> > + }\n> > +\n> > + writeCount_++;\n> > +\n> > + while (writeCount_ >= queueCount_) {\n> > + LOG(DelayedControls, Debug)\n> > + << \"Queue is empty, auto queue no-op.\";\n> > + queue({});\n> > + }\n> > +\n> > + device_->setControls(&out);\n> >\n> \n> As mentioned previously, this will need to separate out controls that must\n> be written immediately (e.g. VBLANK) instead of batched together, but that\n> can be added at a later stage.\n\nI agree this is just the first step and I'm sure we need to evolve and \nrework parts of DelayedControls as we gain more capabilities.\n\n> \n> Regards,\n> Naush\n> \n> \n> \n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -12,6 +12,7 @@ libcamera_sources = files([\n> > 'controls.cpp',\n> > 'control_serializer.cpp',\n> > 'control_validator.cpp',\n> > + 'delayed_controls.cpp',\n> > 'device_enumerator.cpp',\n> > 'device_enumerator_sysfs.cpp',\n> > 'event_dispatcher.cpp',\n> > --\n> > 2.29.2\n> >\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 153E7C3256\n\tfor ;\n\tFri, 8 Jan 2021 15:21:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D91A67F0C;\n\tFri, 8 Jan 2021 16:21:21 +0100 (CET)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 82CB1631AC\n\tfor ;\n\tFri, 8 Jan 2021 16:21:20 +0100 (CET)","by mail-lf1-x12c.google.com with SMTP id b26so23727960lff.9\n\tfor ;\n\tFri, 08 Jan 2021 07:21:20 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tt9sm2031117lff.45.2021.01.08.07.21.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 08 Jan 2021 07:21:18 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"LdmzqXvh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=Ge1TEnjKcFHxyha7Wr8MgF8j3MJYFhPzpfohomrxbO0=;\n\tb=LdmzqXvh6nIFlL+XYpxMyZjAlYDvS6jw0ptiWXbEutXlXGGVh4jJBip7qQolsGVhW6\n\t4WRw0FYRhL6GHg33sj4r7UxEy0MSOXNwbBu1SZ0Cu+bMenWP99OhwLZsh6nXW+A9SRBD\n\tEao4tsm39il3AudaVvl6yQOidBdJLsa8HGR61eCK/g7vAKw9KGYj4iiiaxSYkQNtBe+r\n\tG9E+1/jXvEjmp41LJUV+nkc2Jib/mfV8SjcG4BOxiS1Zdl3dARhTtgeszqlrT1UghKTc\n\t5o2MHmFiOEI3jujTDVvmkmtMnKQ77k0TNWKslRQXpIpp/tR8vcr/AjLqah/79F2ohyfs\n\tfVjg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=Ge1TEnjKcFHxyha7Wr8MgF8j3MJYFhPzpfohomrxbO0=;\n\tb=h1V2CauYnMLUF6jEwZZfPFsGrFsrs3lam0BQRHbsmkPIia4hqsUuXdeh0EyBp/n+h4\n\tVq0x0kBOwUFK4hJvuJbL/x6F0yXXckYdQiLNTYaZ65NQsWUVgkPwkMUBoVaLaCcf+Gr4\n\t6arRKQKeEGP5FxQk9WuD8dNEYyiKcM06118K2S5kSJZUUWmgT+0N18Bur6YfgcNsYHfm\n\t7+ty5qzOUR632RO46GXnsaA+EoqLWBtK924aHT8A+37+piF7oIm02DwxUetJmzkqLTnV\n\tOuogxYimwc4VVSWIWR+/Xotcu29G6ER8nFByqTyTTH6VsDP3h7FppiYGAkP9h0k8uAxh\n\toA1w==","X-Gm-Message-State":"AOAM533sB1PKJ3C2aahD9K+gbBauPtU/k08M+meejeivpYcyfvyuR+Dj\n\tmUk204zSJVKFcqBAlUgZBNx0KA==","X-Google-Smtp-Source":"ABdhPJwmHW8FwCIIn3S2qlqJpns1bnDIPspS639/6AmMsl5HHzsxF/MH4qIEgW/dF8f+M5WzuNJYMA==","X-Received":"by 2002:a2e:99cd:: with SMTP id\n\tl13mr1816945ljj.318.1610119279546; \n\tFri, 08 Jan 2021 07:21:19 -0800 (PST)","Date":"Fri, 8 Jan 2021 16:21:17 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Naushir Patuck ","Message-ID":"","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-2-niklas.soderlund@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14479,"web_url":"https://patchwork.libcamera.org/comment/14479/","msgid":"","date":"2021-01-08T15:31:33","subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Niklas,\n\n\n\nOn Fri, 8 Jan 2021 at 15:21, Niklas Söderlund \nwrote:\n\n> Hi Naushir,\n>\n> Thanks for your comments.\n>\n> On 2020-12-15 13:53:12 +0000, Naushir Patuck wrote:\n> > Hi Niklas,\n> >\n> > Thank you for your patch.\n> >\n> > On Tue, 15 Dec 2020 at 00:48, Niklas Söderlund <\n> > niklas.soderlund@ragnatech.se> wrote:\n> >\n> > > Some sensor controls take effect with a delay as the sensor needs time\n> > > to adjust, for example exposure. Add an optional helper DelayedControls\n> > > to help pipelines deal with such controls.\n> > >\n> > > The idea is to provide a queue of controls towards the V4L2 device and\n> > > apply individual controls with the specified delay with the aim to get\n> > > predictable and retrievable control values for any given frame. To do\n> > > this the queue of controls needs to be at least as deep as the control\n> > > with the largest delay.\n> > >\n> > > The DelayedControls needs to be informed of every start of exposure.\n> > > This can be emulated but the helper is designed to be used with this\n> > > event being provide by the kernel thru V4L2 events.\n> > >\n> > > This helper is based on StaggeredCtrl from the Raspberry Pi pipeline\n> > > handler but expands on its API. This helpers aims to replace the\n> > > Raspberry Pi implementations and mimics it behavior perfectly.\n> > >\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > * Changes since v2\n> > > - Drop optional argument to reset().\n> > > - Update commit message.\n> > > - Remove usage of Mutex.\n> > > - Rename frameStart() to applyControls)_.\n> > > - Rename ControlInfo to Into.\n> > > - Rename ControlArray to ControlRingBuffer.\n> > > - Drop ControlsDelays and ControlsValues.\n> > > - Sort headers.\n> > > - Rename iterators.\n> > > - Simplify queueCount_ handeling in reset().\n> > > - Add more warnings.\n> > > - Update documentation.\n> > >\n> > > * Changes since v2\n> > > - Improve error logic in queue() as suggested by Jean-Michel Hautbois.\n> > > - s/fistSequence_/firstSequence_/\n> > >\n> > > * Changes since v1\n> > > - Correct copyright to reflect work is derived from Raspberry Pi\n> > > pipeline handler. This was always the intention and was wrong in v1.\n> > > - Rewrite large parts of the documentation.\n> > > - Join two loops to one in DelayedControls::DelayedControls()\n> > > ---\n> > > include/libcamera/internal/delayed_controls.h | 82 ++++++\n> > > src/libcamera/delayed_controls.cpp | 252 ++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 3 files changed, 335 insertions(+)\n> > > create mode 100644 include/libcamera/internal/delayed_controls.h\n> > > create mode 100644 src/libcamera/delayed_controls.cpp\n> > >\n> > > diff --git a/include/libcamera/internal/delayed_controls.h\n> > > b/include/libcamera/internal/delayed_controls.h\n> > > new file mode 100644\n> > > index 0000000000000000..1292b484ec9f53e9\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/delayed_controls.h\n> > > @@ -0,0 +1,82 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > > + *\n> > > + * delayed_controls.h - Helper to deal with controls that are applied\n> > > with a delay\n> > > + */\n> > > +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > > +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class V4L2Device;\n> > > +\n> > > +class DelayedControls\n> > > +{\n> > > +public:\n> > > + DelayedControls(V4L2Device *device,\n> > > + const std::unordered_map int>\n> > > &delays);\n> > > +\n> > > + void reset();\n> > > +\n> > > + bool push(const ControlList &controls);\n> > > + ControlList get(uint32_t sequence);\n> > > +\n> > > + void applyControls(uint32_t sequence);\n> > > +\n> > > +private:\n> > > + class Info\n> > > + {\n> > > + public:\n> > > + Info()\n> > > + : updated(false)\n> > > + {\n> > > + }\n> > > +\n> > > + Info(const ControlValue &v)\n> > > + : value(v), updated(true)\n> > > + {\n> > > + }\n> > > +\n> > > + ControlValue value;\n> > > + bool updated;\n> > > + };\n> > > +\n> > > + /* \\todo: Make the listSize configurable at instance creation\n> > > time. */\n> > > + static constexpr int listSize = 16;\n> > > + class ControlRingBuffer : public std::array\n> > > + {\n> > > + public:\n> > > + Info &operator[](unsigned int index)\n> > > + {\n> > > + return std::array > > listSize>::operator[](index % listSize);\n> > > + }\n> > > +\n> > > + const Info &operator[](unsigned int index) const\n> > > + {\n> > > + return std::array > > listSize>::operator[](index % listSize);\n> > > + }\n> > > + };\n> > > +\n> > > + bool queue(const ControlList &controls);\n> > > +\n> > > + V4L2Device *device_;\n> > > + std::unordered_map delays_;\n> > > + unsigned int maxDelay_;\n> > > +\n> > > + bool running_;\n> > > + uint32_t firstSequence_;\n> > > +\n> > > + uint32_t queueCount_;\n> > > + uint32_t writeCount_;\n> > > + std::unordered_map\n> values_;\n> > > +};\n> > >\n> >\n> > StaggeredCtrl has a mutex to protect access to state. I wasn't entirely\n> > sure that was needed. Presumably the SoF event calling applyControls()\n> and\n> > all other public method access occur in the same thread context, so no\n> > protection is needed? Do you foresee this ever changing?\n>\n> As Laurent already replied, the mutex is not needed in out current\n> design.\n>\n> >\n> > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */\n> > > diff --git a/src/libcamera/delayed_controls.cpp\n> > > b/src/libcamera/delayed_controls.cpp\n> > > new file mode 100644\n> > > index 0000000000000000..db2e51f8c93c4755\n> > > --- /dev/null\n> > > +++ b/src/libcamera/delayed_controls.cpp\n> > > @@ -0,0 +1,252 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > > + *\n> > > + * delayed_controls.h - Helper to deal with controls that are applied\n> > > with a delay\n> > > + */\n> > > +\n> > > +#include \"libcamera/internal/delayed_controls.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/log.h\"\n> > > +#include \"libcamera/internal/v4l2_device.h\"\n> > > +\n> > > +/**\n> > > + * \\file delayed_controls.h\n> > > + * \\brief Helper to deal with controls that are applied with a delay\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(DelayedControls)\n> > > +\n> > > +/**\n> > > + * \\class DelayedControls\n> > > + * \\brief Helper to deal with controls that take effect with a delay\n> > > + *\n> > > + * Some sensor controls take effect with a delay as the sensor needs\n> time\n> > > to\n> > > + * adjust, for example exposure and focus. This is a helper class to\n> deal\n> > > with\n> > > + * such controls and the intended users are pipeline handlers.\n> > > + *\n> > > + * The idea is to extend the concept of the buffer depth of a\n> pipeline the\n> > > + * application needs to maintain to also cover controls. Just as with\n> > > buffer\n> > > + * depth if the application keeps the number of requests queued above\n> the\n> > > + * control depth the controls are guaranteed to take effect for the\n> > > correct\n> > > + * request. The control depth is determined by the control with the\n> > > greatest\n> > > + * delay.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct a DelayedControls instance\n> > > + * \\param[in] device The V4L2 device the controls have to be applied\n> to\n> > > + * \\param[in] delays Map of the numerical V4L2 control ids to their\n> > > associated\n> > > + * delays (in frames)\n> > > + *\n> > > + * Only controls specified in \\a delays are handled. If it's desired\n> to\n> > > mix\n> > > + * delayed controls and controls that take effect immediately the\n> > > immediate\n> > > + * controls must be listed in the \\a delays map with a delay value of\n> 0.\n> > > + */\n> > > +DelayedControls::DelayedControls(V4L2Device *device,\n> > > + const std::unordered_map > > unsigned int> &delays)\n> > > + : device_(device), maxDelay_(0)\n> > > +{\n> > > + const ControlInfoMap &controls = device_->controls();\n> > > +\n> > > + /*\n> > > + * Create a map of control ids to delays for controls exposed\n> by\n> > > the\n> > > + * device.\n> > > + */\n> > > + for (auto const &delay : delays) {\n> > > + auto it = controls.find(delay.first);\n> > > + if (it == controls.end()) {\n> > > + LOG(DelayedControls, Error)\n> > > + << \"Delay request for control id \"\n> > > + << utils::hex(delay.first)\n> > > + << \" but control is not exposed by\n> device \"\n> > > + << device_->deviceNode();\n> > > + continue;\n> > > + }\n> > > +\n> > > + const ControlId *id = it->first;\n> > > +\n> > > + delays_[id] = delay.second;\n> > > +\n> > > + LOG(DelayedControls, Debug)\n> > > + << \"Set a delay of \" << delays_[id]\n> > > + << \" for \" << id->name();\n> > > +\n> > > + maxDelay_ = std::max(maxDelay_, delays_[id]);\n> > > + }\n> > > +\n> > > + reset();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Reset state machine\n> > > + *\n> > > + * Resets the state machine to a starting position based on control\n> values\n> > > + * retrieved from the device.\n> > > + */\n> > > +void DelayedControls::reset()\n> > > +{\n> > > + running_ = false;\n> > > + firstSequence_ = 0;\n> > > + queueCount_ = 1;\n> > > + writeCount_ = 0;\n> > > +\n> > > + /* Retrieve control as reported by the device. */\n> > > + std::vector ids;\n> > > + for (auto const &delay : delays_)\n> > > + ids.push_back(delay.first->id());\n> > > +\n> > > + ControlList controls = device_->getControls(ids);\n> > > +\n> > > + /* Seed the control queue with the controls reported by the\n> > > device. */\n> > > + values_.clear();\n> > > + for (const auto &ctrl : controls) {\n> > > + const ControlId *id =\n> > > device_->controls().idmap().at(ctrl.first);\n> > > + values_[id][0] = Info(ctrl.second);\n> > > + }\n> > >\n> >\n> > I think this may have been mentioned in one of the earlier rounds - this\n> > may not be correct as all the controls available to the device may not\n> be\n> > registered with DelayedControls. In those cases, the values_ map lookup\n> > would fail. You would need to validate that id is an available key in\n> the\n> > map.\n>\n> Is this not already done? The 'controls' that are iterated in this loop\n> is constructed from the IDs in delays_ which only contains the controls\n> with an associated delay.\n>\n\nYes, I see that now. Sorry I missed that in my earlier read of the patch.\nWith the minor fixup to merge push() and queue(),\n\nReviewed-by: Naushir Patuck \n\n\n>\n> >\n> >\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Push a set of controls on the queue\n> > > + * \\param[in] controls List of controls to add to the device queue\n> > > + *\n> > > + * Push a set of controls to the control queue. This increases the\n> > > control queue\n> > > + * depth by one.\n> > > + *\n> > > + * \\returns true if \\a controls are accepted, or false otherwise\n> > > + */\n> > > +bool DelayedControls::push(const ControlList &controls)\n> > > +{\n> > > + return queue(controls);\n> > > +}\n> > >\n> >\n> > Apologies, I may have missed something obvious, but why do we have the\n> > push() and queue() methods when one calls the other?\n>\n> Good call! This is a leftover from when there was a mutex that needed to\n> be locked when called from the public API but not when the functionality\n> was used internally where the mutex was already locked. Will fold these\n> two together.\n>\n> >\n> >\n> > > +\n> > > +bool DelayedControls::queue(const ControlList &controls)\n> > > +{\n> > > + /* Copy state from previous frame. */\n> > > + for (auto &ctrl : values_) {\n> > > + Info &info = ctrl.second[queueCount_];\n> > > + info.value = values_[ctrl.first][queueCount_ -\n> 1].value;\n> > > + info.updated = false;\n> > > + }\n> > > +\n> > > + /* Update with new controls. */\n> > > + const ControlIdMap &idmap = device_->controls().idmap();\n> > > + for (const auto &control : controls) {\n> > > + const auto &it = idmap.find(control.first);\n> > > + if (it == idmap.end()) {\n> > > + LOG(DelayedControls, Warning)\n> > > + << \"Unknown control \" << control.first;\n> > > + return false;\n> > > + }\n> > > +\n> > > + const ControlId *id = it->second;\n> > > +\n> > > + if (delays_.find(id) == delays_.end())\n> > > + return false;\n> > > +\n> > > + Info &info = values_[id][queueCount_];\n> > > +\n> > > + info.value = control.second;\n> > > + info.updated = true;\n> > > +\n> > > + LOG(DelayedControls, Debug)\n> > > + << \"Queuing \" << id->name()\n> > > + << \" to \" << info.value.toString()\n> > > + << \" at index \" << queueCount_;\n> > > + }\n> > > +\n> > > + queueCount_++;\n> > > +\n> > > + return true;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Read back controls in effect at a sequence number\n> > > + * \\param[in] sequence The sequence number to get controls for\n> > > + *\n> > > + * Read back what controls where in effect at a specific sequence\n> number.\n> > > The\n> > > + * history is a ring buffer of 16 entries where new and old values\n> > > coexist. It's\n> > > + * the callers responsibility to not read too old sequence numbers\n> that\n> > > have been\n> > > + * pushed out of the history.\n> > > + *\n> > > + * Historic values are evicted by pushing new values onto the queue\n> using\n> > > + * push(). The max history from the current sequence number that\n> yields\n> > > valid\n> > > + * values are thus 16 minus number of controls pushed.\n> > > + *\n> > > + * \\return The controls at \\a sequence number\n> > > + */\n> > > +ControlList DelayedControls::get(uint32_t sequence)\n> > > +{\n> > > + uint32_t adjustedSeq = sequence - firstSequence_ + 1;\n> > > + unsigned int index = std::max(0, adjustedSeq - maxDelay_);\n> > > +\n> > > + ControlList out(device_->controls());\n> > > + for (const auto &ctrl : values_) {\n> > > + const ControlId *id = ctrl.first;\n> > > + const Info &info = ctrl.second[index];\n> > > +\n> > > + out.set(id->id(), info.value);\n> > > +\n> > > + LOG(DelayedControls, Debug)\n> > > + << \"Reading \" << id->name()\n> > > + << \" to \" << info.value.toString()\n> > > + << \" at index \" << index;\n> > > + }\n> > > +\n> > > + return out;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Inform DelayedControls of the start of a new frame\n> > > + * \\param[in] sequence Sequence number of the frame that started\n> > > + *\n> > > + * Inform the state machine that a new frame has started and of its\n> > > sequence\n> > > + * number. Any user of these helpers is responsible to inform the\n> helper\n> > > about\n> > > + * the start of any frame.This can be connected with ease to the\n> start of\n> > > a\n> > > + * exposure (SOE) V4L2 event.\n> > > + */\n> > > +void DelayedControls::applyControls(uint32_t sequence)\n> > > +{\n> > > + LOG(DelayedControls, Debug) << \"frame \" << sequence << \"\n> started\";\n> > > +\n> > > + if (!running_) {\n> > > + firstSequence_ = sequence;\n> > > + running_ = true;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Create control list peaking ahead in the value queue to\n> ensure\n> > > + * values are set in time to satisfy the sensor delay.\n> > > + */\n> > > + ControlList out(device_->controls());\n> > > + for (const auto &ctrl : values_) {\n> > > + const ControlId *id = ctrl.first;\n> > > + unsigned int delayDiff = maxDelay_ - delays_[id];\n> > > + unsigned int index = std::max(0, writeCount_ -\n> > > delayDiff);\n> > > + const Info &info = ctrl.second[index];\n> > > +\n> > > + if (info.updated) {\n> > > + out.set(id->id(), info.value);\n> > > + LOG(DelayedControls, Debug)\n> > > + << \"Setting \" << id->name()\n> > > + << \" to \" << info.value.toString()\n> > > + << \" at index \" << index;\n> > > + }\n> > > + }\n> > > +\n> > > + writeCount_++;\n> > > +\n> > > + while (writeCount_ >= queueCount_) {\n> > > + LOG(DelayedControls, Debug)\n> > > + << \"Queue is empty, auto queue no-op.\";\n> > > + queue({});\n> > > + }\n> > > +\n> > > + device_->setControls(&out);\n> > >\n> >\n> > As mentioned previously, this will need to separate out controls that\n> must\n> > be written immediately (e.g. VBLANK) instead of batched together, but\n> that\n> > can be added at a later stage.\n>\n> I agree this is just the first step and I'm sure we need to evolve and\n> rework parts of DelayedControls as we gain more capabilities.\n>\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -12,6 +12,7 @@ libcamera_sources = files([\n> > > 'controls.cpp',\n> > > 'control_serializer.cpp',\n> > > 'control_validator.cpp',\n> > > + 'delayed_controls.cpp',\n> > > 'device_enumerator.cpp',\n> > > 'device_enumerator_sysfs.cpp',\n> > > 'event_dispatcher.cpp',\n> > > --\n> > > 2.29.2\n> > >\n> > >\n>\n> --\n> Regards,\n> Niklas Söderlund\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BC2CCC0F1A\n\tfor ;\n\tFri, 8 Jan 2021 15:31:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 48DA467F70;\n\tFri, 8 Jan 2021 16:31:53 +0100 (CET)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9ACEC635A0\n\tfor ;\n\tFri, 8 Jan 2021 16:31:50 +0100 (CET)","by mail-lf1-x135.google.com with SMTP id 23so23760536lfg.10\n\tfor ;\n\tFri, 08 Jan 2021 07:31:50 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"FMCDAvKo\"; 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=l4DSvj5+BXUcn4Q5/y+F558QqtcyjpPx1h+ndoF0kog=;\n\tb=FMCDAvKog1q1UV00klVx4ZlWeYYZVWabIWv9budeaE3PUYfNpCYR+auMgTLU0OQxtL\n\tk7ouuoyEF87ZruNKVph2tEX1FVJBPmoCBY2rIcWXr3OaFLBu/JC7qlKmXAzZoHcyUmmY\n\t2xNFswC/a8Hg9VPSKFGlJt7Hgx7RsQD9JsUcvyQ1sH3EYmhdCR2Tg0lEeD0qD8Qa5G18\n\tYktFAUX8Utcj8riN6fdN6V/ZVtSepMIIw+S+9NHqkgqe1SsIq991ld2lc7YZdltkZ5Zr\n\tGyoADAHCDVkjS+i9lnyTUO7STwCCueSxZBKsfO1//iqQdXmb2x8eXrzURsUmK2zNOLlu\n\to30Q==","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=l4DSvj5+BXUcn4Q5/y+F558QqtcyjpPx1h+ndoF0kog=;\n\tb=N674Wi3HrhNRkCSMMc+06yYoveeZDa/BZiR72Vezwsn5jJXQmrNGIl2dHIRUGzZW9w\n\taKxpAbN24Xe4V+x36r8eT2PcEV6qN43oiOxyLHfiUeOfsFCwS/0lQvRCV21bmwP72536\n\tz8MpDePpj63d85mORaPmKmY7nozzCGD7AVl+1sWrESEKCyVRgc9WIc7/4odX9nYa15sL\n\tti8/S0mlNW2j8mflNSq1XupfE+8kl9hsTvw1+7aBsFlEp0G1Jf64KIdPJQ/MPfDPCFu+\n\t/hT3ImSuqdj0DDzi0ARBPmxSkrqkvW/CaoftPiKDWLX5465IpFAYCXblCuWqEVuGBfkU\n\tZdRg==","X-Gm-Message-State":"AOAM530cGOdSP6YK7LvDdfYFwLMRfCkOuLvcmKv1z5MsKlpsYEL5HZXS\n\t2qB88En6XANPUIgHvKSFy0yhsuugbR51M0xQgYCKFNudqR00/Q==","X-Google-Smtp-Source":"ABdhPJxSHyA8JgnUJI42wOHbqQfvmyoTI93pIO9jHgAMNusHgTVi3YzwFC19dztbDNXmhVjWeOCQf1Zz2dn5phzoUdM=","X-Received":"by 2002:a05:651c:503:: with SMTP id\n\to3mr1657038ljp.253.1610119909944; \n\tFri, 08 Jan 2021 07:31:49 -0800 (PST)","MIME-Version":"1.0","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-2-niklas.soderlund@ragnatech.se>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 8 Jan 2021 15:31:33 +0000","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============6671867945548028086==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14495,"web_url":"https://patchwork.libcamera.org/comment/14495/","msgid":"","date":"2021-01-10T16:07:16","subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nIn the subject line, s/applies/apply/\n\nOn Tue, Dec 15, 2020 at 01:48:04AM +0100, Niklas Söderlund wrote:\n> Some sensor controls take effect with a delay as the sensor needs time\n> to adjust, for example exposure. Add an optional helper DelayedControls\n> to help pipelines deal with such controls.\n> \n> The idea is to provide a queue of controls towards the V4L2 device and\n> apply individual controls with the specified delay with the aim to get\n> predictable and retrievable control values for any given frame. To do\n> this the queue of controls needs to be at least as deep as the control\n> with the largest delay.\n> \n> The DelayedControls needs to be informed of every start of exposure.\n> This can be emulated but the helper is designed to be used with this\n> event being provide by the kernel thru V4L2 events.\n\ns/thru/through/\n\n> This helper is based on StaggeredCtrl from the Raspberry Pi pipeline\n> handler but expands on its API. This helpers aims to replace the\n> Raspberry Pi implementations and mimics it behavior perfectly.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> * Changes since v2\n> - Drop optional argument to reset().\n> - Update commit message.\n> - Remove usage of Mutex.\n> - Rename frameStart() to applyControls)_.\n> - Rename ControlInfo to Into.\n> - Rename ControlArray to ControlRingBuffer.\n> - Drop ControlsDelays and ControlsValues.\n> - Sort headers.\n> - Rename iterators.\n> - Simplify queueCount_ handeling in reset().\n> - Add more warnings.\n> - Update documentation.\n> \n> * Changes since v2\n> - Improve error logic in queue() as suggested by Jean-Michel Hautbois.\n> - s/fistSequence_/firstSequence_/\n> \n> * Changes since v1\n> - Correct copyright to reflect work is derived from Raspberry Pi\n> pipeline handler. This was always the intention and was wrong in v1.\n> - Rewrite large parts of the documentation.\n> - Join two loops to one in DelayedControls::DelayedControls()\n> ---\n> include/libcamera/internal/delayed_controls.h | 82 ++++++\n> src/libcamera/delayed_controls.cpp | 252 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 335 insertions(+)\n> create mode 100644 include/libcamera/internal/delayed_controls.h\n> create mode 100644 src/libcamera/delayed_controls.cpp\n> \n> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> new file mode 100644\n> index 0000000000000000..1292b484ec9f53e9\n> --- /dev/null\n> +++ b/include/libcamera/internal/delayed_controls.h\n> @@ -0,0 +1,82 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> + */\n> +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class V4L2Device;\n> +\n> +class DelayedControls\n> +{\n> +public:\n> +\tDelayedControls(V4L2Device *device,\n> +\t\t\tconst std::unordered_map &delays);\n\nI've sent the following comment on v3:\n\n> I could imagine this being useful for non-V4L2 devices too. We could\n> instead pass a ControlInfoMap to the constructor, and move the\n> setControls() call to the user of this class. There's no need to do so\n> now (but of course if you think it's a good idea, I'm not asking to\n> delay such rework :-)), but could you capture this in a \\todo comment\n> (if you agree with the idea) ?\n\nI see neither a rework of the API, nor a \\todo comment. Is that an\noversight, or did you disagree with the idea ? In the latter case, a\nreply to the review would be nice.\n\n> +\n> +\tvoid reset();\n> +\n> +\tbool push(const ControlList &controls);\n> +\tControlList get(uint32_t sequence);\n> +\n> +\tvoid applyControls(uint32_t sequence);\n> +\n> +private:\n> +\tclass Info\n> +\t{\n> +\tpublic:\n> +\t\tInfo()\n> +\t\t\t: updated(false)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tInfo(const ControlValue &v)\n> +\t\t\t: value(v), updated(true)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tControlValue value;\n> +\t\tbool updated;\n> +\t};\n\nSame here, v3 had the following review comment:\n\n> You could also make this class inherit from ControlValue, to avoid the\n> explicit .value below.\n\n> +\n> +\t/* \\todo: Make the listSize configurable at instance creation time. */\n> +\tstatic constexpr int listSize = 16;\n> +\tclass ControlRingBuffer : public std::array\n> +\t{\n> +\tpublic:\n> +\t\tInfo &operator[](unsigned int index)\n> +\t\t{\n> +\t\t\treturn std::array::operator[](index % listSize);\n> +\t\t}\n> +\n> +\t\tconst Info &operator[](unsigned int index) const\n> +\t\t{\n> +\t\t\treturn std::array::operator[](index % listSize);\n> +\t\t}\n> +\t};\n\nThere were further comments in the review of v3 that have also not been\naddressed.\n\n> +\n> +\tbool queue(const ControlList &controls);\n> +\n> +\tV4L2Device *device_;\n> +\tstd::unordered_map delays_;\n> +\tunsigned int maxDelay_;\n> +\n> +\tbool running_;\n> +\tuint32_t firstSequence_;\n> +\n> +\tuint32_t queueCount_;\n> +\tuint32_t writeCount_;\n> +\tstd::unordered_map values_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */\n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> new file mode 100644\n> index 0000000000000000..db2e51f8c93c4755\n> --- /dev/null\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -0,0 +1,252 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> + */\n> +\n> +#include \"libcamera/internal/delayed_controls.h\"\n> +\n> +#include \n> +\n> +#include \"libcamera/internal/log.h\"\n> +#include \"libcamera/internal/v4l2_device.h\"\n> +\n> +/**\n> + * \\file delayed_controls.h\n> + * \\brief Helper to deal with controls that are applied with a delay\n\ns/are applied/take effect/ (matching the documentation of the class\nbelow)\n\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(DelayedControls)\n> +\n> +/**\n> + * \\class DelayedControls\n> + * \\brief Helper to deal with controls that take effect with a delay\n> + *\n> + * Some sensor controls take effect with a delay as the sensor needs time to\n> + * adjust, for example exposure and focus. This is a helper class to deal with\n\nFocus is a different beast. Let's s/focus/analog gain/, that's a better\nexample.\n\n> + * such controls and the intended users are pipeline handlers.\n> + *\n> + * The idea is to extend the concept of the buffer depth of a pipeline the\n> + * application needs to maintain to also cover controls. Just as with buffer\n> + * depth if the application keeps the number of requests queued above the\n> + * control depth the controls are guaranteed to take effect for the correct\n> + * request. The control depth is determined by the control with the greatest\n> + * delay.\n> + */\n> +\n> +/**\n> + * \\brief Construct a DelayedControls instance\n> + * \\param[in] device The V4L2 device the controls have to be applied to\n> + * \\param[in] delays Map of the numerical V4L2 control ids to their associated\n> + * delays (in frames)\n> + *\n> + * Only controls specified in \\a delays are handled. If it's desired to mix\n> + * delayed controls and controls that take effect immediately the immediate\n> + * controls must be listed in the \\a delays map with a delay value of 0.\n> + */\n> +DelayedControls::DelayedControls(V4L2Device *device,\n> +\t\t\t\t const std::unordered_map &delays)\n> +\t: device_(device), maxDelay_(0)\n> +{\n> +\tconst ControlInfoMap &controls = device_->controls();\n> +\n> +\t/*\n> +\t * Create a map of control ids to delays for controls exposed by the\n> +\t * device.\n> +\t */\n> +\tfor (auto const &delay : delays) {\n> +\t\tauto it = controls.find(delay.first);\n> +\t\tif (it == controls.end()) {\n> +\t\t\tLOG(DelayedControls, Error)\n> +\t\t\t\t<< \"Delay request for control id \"\n> +\t\t\t\t<< utils::hex(delay.first)\n> +\t\t\t\t<< \" but control is not exposed by device \"\n> +\t\t\t\t<< device_->deviceNode();\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tconst ControlId *id = it->first;\n> +\n> +\t\tdelays_[id] = delay.second;\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Set a delay of \" << delays_[id]\n> +\t\t\t<< \" for \" << id->name();\n> +\n> +\t\tmaxDelay_ = std::max(maxDelay_, delays_[id]);\n> +\t}\n> +\n> +\treset();\n> +}\n> +\n> +/**\n> + * \\brief Reset state machine\n> + *\n> + * Resets the state machine to a starting position based on control values\n> + * retrieved from the device.\n\nThis is a bit terse, and so is the \\class documentation. Reading the\ndocumentation only, not the code, it would be quite difficult to use\nthis class :-S\n\n> + */\n> +void DelayedControls::reset()\n> +{\n> +\trunning_ = false;\n> +\tfirstSequence_ = 0;\n> +\tqueueCount_ = 1;\n> +\twriteCount_ = 0;\n> +\n> +\t/* Retrieve control as reported by the device. */\n> +\tstd::vector ids;\n> +\tfor (auto const &delay : delays_)\n> +\t\tids.push_back(delay.first->id());\n> +\n> +\tControlList controls = device_->getControls(ids);\n> +\n> +\t/* Seed the control queue with the controls reported by the device. */\n> +\tvalues_.clear();\n> +\tfor (const auto &ctrl : controls) {\n> +\t\tconst ControlId *id = device_->controls().idmap().at(ctrl.first);\n> +\t\tvalues_[id][0] = Info(ctrl.second);\n> +\t}\n> +}\n> +\n> +/**\n> + * \\brief Push a set of controls on the queue\n> + * \\param[in] controls List of controls to add to the device queue\n> + *\n> + * Push a set of controls to the control queue. This increases the control queue\n> + * depth by one.\n> + *\n> + * \\returns true if \\a controls are accepted, or false otherwise\n> + */\n> +bool DelayedControls::push(const ControlList &controls)\n> +{\n> +\treturn queue(controls);\n> +}\n> +\n> +bool DelayedControls::queue(const ControlList &controls)\n> +{\n> +\t/* Copy state from previous frame. */\n> +\tfor (auto &ctrl : values_) {\n> +\t\tInfo &info = ctrl.second[queueCount_];\n> +\t\tinfo.value = values_[ctrl.first][queueCount_ - 1].value;\n> +\t\tinfo.updated = false;\n> +\t}\n> +\n> +\t/* Update with new controls. */\n> +\tconst ControlIdMap &idmap = device_->controls().idmap();\n> +\tfor (const auto &control : controls) {\n> +\t\tconst auto &it = idmap.find(control.first);\n> +\t\tif (it == idmap.end()) {\n> +\t\t\tLOG(DelayedControls, Warning)\n> +\t\t\t\t<< \"Unknown control \" << control.first;\n> +\t\t\treturn false;\n> +\t\t}\n> +\n> +\t\tconst ControlId *id = it->second;\n> +\n> +\t\tif (delays_.find(id) == delays_.end())\n> +\t\t\treturn false;\n> +\n> +\t\tInfo &info = values_[id][queueCount_];\n> +\n> +\t\tinfo.value = control.second;\n> +\t\tinfo.updated = true;\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Queuing \" << id->name()\n> +\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t<< \" at index \" << queueCount_;\n> +\t}\n> +\n> +\tqueueCount_++;\n> +\n> +\treturn true;\n> +}\n> +\n> +/**\n> + * \\brief Read back controls in effect at a sequence number\n> + * \\param[in] sequence The sequence number to get controls for\n> + *\n> + * Read back what controls where in effect at a specific sequence number. The\n> + * history is a ring buffer of 16 entries where new and old values coexist. It's\n> + * the callers responsibility to not read too old sequence numbers that have been\n> + * pushed out of the history.\n> + *\n> + * Historic values are evicted by pushing new values onto the queue using\n> + * push(). The max history from the current sequence number that yields valid\n> + * values are thus 16 minus number of controls pushed.\n> + *\n> + * \\return The controls at \\a sequence number\n> + */\n> +ControlList DelayedControls::get(uint32_t sequence)\n> +{\n> +\tuint32_t adjustedSeq = sequence - firstSequence_ + 1;\n> +\tunsigned int index = std::max(0, adjustedSeq - maxDelay_);\n> +\n> +\tControlList out(device_->controls());\n> +\tfor (const auto &ctrl : values_) {\n> +\t\tconst ControlId *id = ctrl.first;\n> +\t\tconst Info &info = ctrl.second[index];\n> +\n> +\t\tout.set(id->id(), info.value);\n> +\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Reading \" << id->name()\n> +\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t<< \" at index \" << index;\n> +\t}\n> +\n> +\treturn out;\n> +}\n> +\n> +/**\n> + * \\brief Inform DelayedControls of the start of a new frame\n> + * \\param[in] sequence Sequence number of the frame that started\n> + *\n> + * Inform the state machine that a new frame has started and of its sequence\n> + * number. Any user of these helpers is responsible to inform the helper about\n> + * the start of any frame.This can be connected with ease to the start of a\n> + * exposure (SOE) V4L2 event.\n> + */\n> +void DelayedControls::applyControls(uint32_t sequence)\n> +{\n> +\tLOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> +\n> +\tif (!running_) {\n> +\t\tfirstSequence_ = sequence;\n> +\t\trunning_ = true;\n> +\t}\n> +\n> +\t/*\n> +\t * Create control list peaking ahead in the value queue to ensure\n> +\t * values are set in time to satisfy the sensor delay.\n> +\t */\n> +\tControlList out(device_->controls());\n> +\tfor (const auto &ctrl : values_) {\n> +\t\tconst ControlId *id = ctrl.first;\n> +\t\tunsigned int delayDiff = maxDelay_ - delays_[id];\n> +\t\tunsigned int index = std::max(0, writeCount_ - delayDiff);\n\nA reply to the v3 review would also be appreciated for the issue pointed\nour regarding this code. I still can't see the implementation being\ncorrect :-S\n\n> +\t\tconst Info &info = ctrl.second[index];\n> +\n> +\t\tif (info.updated) {\n> +\t\t\tout.set(id->id(), info.value);\n> +\t\t\tLOG(DelayedControls, Debug)\n> +\t\t\t\t<< \"Setting \" << id->name()\n> +\t\t\t\t<< \" to \" << info.value.toString()\n> +\t\t\t\t<< \" at index \" << index;\n> +\t\t}\n> +\t}\n> +\n> +\twriteCount_++;\n> +\n> +\twhile (writeCount_ >= queueCount_) {\n> +\t\tLOG(DelayedControls, Debug)\n> +\t\t\t<< \"Queue is empty, auto queue no-op.\";\n> +\t\tqueue({});\n> +\t}\n> +\n> +\tdevice_->setControls(&out);\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -12,6 +12,7 @@ libcamera_sources = files([\n> 'controls.cpp',\n> 'control_serializer.cpp',\n> 'control_validator.cpp',\n> + 'delayed_controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> 'event_dispatcher.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 53B2DBD80C\n\tfor ;\n\tSun, 10 Jan 2021 16:07:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BD17868069;\n\tSun, 10 Jan 2021 17:07:32 +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 920FD60523\n\tfor ;\n\tSun, 10 Jan 2021 17:07:31 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 030F8DA;\n\tSun, 10 Jan 2021 17:07:30 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"o74TO+d3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610294851;\n\tbh=dVnXRRUmJd+JoOpwGe1jRuoZM50SfdEzHX3wOye1SOk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=o74TO+d3b7WEFrKwM+8xdtjN6KFmfgoU6Vkj6anYBEFqSjojcZ3oD22eaUXoKBVhE\n\thgLSrahnphpS5SRkSQ6d74SRvECgN+ik2Zoln6ZyaUMQ+fDLdpklu56zn289l+hep1\n\th/Y7O0jTjTZ+73WN9GkUFP5dQgzBqmP684xsnUS4=","Date":"Sun, 10 Jan 2021 18:07:16 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201215004811.602429-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14559,"web_url":"https://patchwork.libcamera.org/comment/14559/","msgid":"","date":"2021-01-16T09:56:50","subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nFirst let me apologise for for the misunderstanding on my side regarding \nthe comments from you in v3 that are not addressed in this version. \nAfter reading your comments in addition to offline discussions my \nunderstanding of the way forward was to,\n\n- Remove my attempt in v1 and v2 to integrate DelayedControls into \n CameraSensor and leave the work for pipeline handlers. The goal was to \n not prematurely build a CameraSensor integration which did not suite \n all users. This I agreed with and it is done.\n\n- Likewise my understanding was that we aimed to not make any large \n redesigns of the DelayedControls logic as it currently performs in \n lockstep with StaggeredCtrls which it is replacing. Once we had \n integrated it with RkISP1 replacing the awfully Timeline solution (I'm \n allowed to cal it names as I designed it :-P) and the IPU3 to allow a \n IPA base to take shape would we start to fix it up in a way that would \n solve problems for all pipelines.\n\nMy view was that this would be done on top as I think the integration in \nCameraSensor and improving on the DelayedControls is a bit related to \neach other and that is why I \"ignored\" your comments when sending v4. \nJudging from your reaction I have misread the situation and for that I'm \nsorry. I never meant to ignore your comments, I only did so believing we \nboth shared the same understanding.\n\norz\n\nI will reply to your comments below and I will copy-paste in your \ncomments from v3 that you have not already yourself.\n\nOn 2021-01-10 18:07:16 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> In the subject line, s/applies/apply/\n> \n> On Tue, Dec 15, 2020 at 01:48:04AM +0100, Niklas Söderlund wrote:\n> > Some sensor controls take effect with a delay as the sensor needs time\n> > to adjust, for example exposure. Add an optional helper DelayedControls\n> > to help pipelines deal with such controls.\n> > \n> > The idea is to provide a queue of controls towards the V4L2 device and\n> > apply individual controls with the specified delay with the aim to get\n> > predictable and retrievable control values for any given frame. To do\n> > this the queue of controls needs to be at least as deep as the control\n> > with the largest delay.\n> > \n> > The DelayedControls needs to be informed of every start of exposure.\n> > This can be emulated but the helper is designed to be used with this\n> > event being provide by the kernel thru V4L2 events.\n> \n> s/thru/through/\n> \n> > This helper is based on StaggeredCtrl from the Raspberry Pi pipeline\n> > handler but expands on its API. This helpers aims to replace the\n> > Raspberry Pi implementations and mimics it behavior perfectly.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > * Changes since v2\n> > - Drop optional argument to reset().\n> > - Update commit message.\n> > - Remove usage of Mutex.\n> > - Rename frameStart() to applyControls)_.\n> > - Rename ControlInfo to Into.\n> > - Rename ControlArray to ControlRingBuffer.\n> > - Drop ControlsDelays and ControlsValues.\n> > - Sort headers.\n> > - Rename iterators.\n> > - Simplify queueCount_ handeling in reset().\n> > - Add more warnings.\n> > - Update documentation.\n> > \n> > * Changes since v2\n> > - Improve error logic in queue() as suggested by Jean-Michel Hautbois.\n> > - s/fistSequence_/firstSequence_/\n> > \n> > * Changes since v1\n> > - Correct copyright to reflect work is derived from Raspberry Pi\n> > pipeline handler. This was always the intention and was wrong in v1.\n> > - Rewrite large parts of the documentation.\n> > - Join two loops to one in DelayedControls::DelayedControls()\n> > ---\n> > include/libcamera/internal/delayed_controls.h | 82 ++++++\n> > src/libcamera/delayed_controls.cpp | 252 ++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 3 files changed, 335 insertions(+)\n> > create mode 100644 include/libcamera/internal/delayed_controls.h\n> > create mode 100644 src/libcamera/delayed_controls.cpp\n> > \n> > diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> > new file mode 100644\n> > index 0000000000000000..1292b484ec9f53e9\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/delayed_controls.h\n> > @@ -0,0 +1,82 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> > + */\n> > +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class V4L2Device;\n> > +\n> > +class DelayedControls\n> > +{\n> > +public:\n> > +\tDelayedControls(V4L2Device *device,\n> > +\t\t\tconst std::unordered_map &delays);\n> \n> I've sent the following comment on v3:\n> \n> > I could imagine this being useful for non-V4L2 devices too. We could\n> > instead pass a ControlInfoMap to the constructor, and move the\n> > setControls() call to the user of this class. There's no need to do so\n> > now (but of course if you think it's a good idea, I'm not asking to\n> > delay such rework :-)), but could you capture this in a \\todo comment\n> > (if you agree with the idea) ?\n> \n> I see neither a rework of the API, nor a \\todo comment. Is that an\n> oversight, or did you disagree with the idea ? In the latter case, a\n> reply to the review would be nice.\n\nUnluckily for my situation, this is a comment I do not agree with :-)\n\nMaybe down the road once we know the design of how we want to integrate \nthis with CameraSensor and what type of other non-V4L2 devices this \ncould be useful for I might be see it differently. Foe now I think we \nshould keep it as is as, at least until we know how to integrate it with \nCameraSensor which I think should be the next stop done on top of this \n(together with integrating the sensor database).\n\n> \n> > +\n> > +\tvoid reset();\n> > +\n> > +\tbool push(const ControlList &controls);\n> > +\tControlList get(uint32_t sequence);\n> > +\n> > +\tvoid applyControls(uint32_t sequence);\n> > +\n> > +private:\n> > +\tclass Info\n> > +\t{\n> > +\tpublic:\n> > +\t\tInfo()\n> > +\t\t\t: updated(false)\n> > +\t\t{\n> > +\t\t}\n> > +\n> > +\t\tInfo(const ControlValue &v)\n> > +\t\t\t: value(v), updated(true)\n> > +\t\t{\n> > +\t\t}\n> > +\n> > +\t\tControlValue value;\n> > +\t\tbool updated;\n> > +\t};\n> \n> Same here, v3 had the following review comment:\n> \n> > You could also make this class inherit from ControlValue, to avoid the\n> > explicit .value below.\n\nThis one I agree with but \"ignored\" as to keep the deign as close to \nStaggerdCtrls as possible, I will do so for v5.\n\n> \n> > +\n> > +\t/* \\todo: Make the listSize configurable at instance creation time. */\n> > +\tstatic constexpr int listSize = 16;\n> > +\tclass ControlRingBuffer : public std::array\n> > +\t{\n> > +\tpublic:\n> > +\t\tInfo &operator[](unsigned int index)\n> > +\t\t{\n> > +\t\t\treturn std::array::operator[](index % listSize);\n> > +\t\t}\n> > +\n> > +\t\tconst Info &operator[](unsigned int index) const\n> > +\t\t{\n> > +\t\t\treturn std::array::operator[](index % listSize);\n> > +\t\t}\n> > +\t};\n> \n> There were further comments in the review of v3 that have also not been\n> addressed.\n\nOnce more sorry for the misunderstanding, the comments where,\n\n> As these two types are only used once, below in the class declaration,\n> how about dropping the aliases ? Aliases are useful to avoid typing\n> types out explicitly in every location, but they have the drawback of\n> possibly obfuscating the code. I would then rename the above ControlInfo\n> to Value, as it stores a value.\n\nThis is mostly done in v4, ControlInfo was renamed INfo and not Value.\n\n> \n> I wonder if we should bundle the delay and array in a single class, to\n> have a single map (now or on top).\n\nI can see pros and cons with both solutions. I don't feel strongly about \neither one, unless we find a good reason to rework this now I would \nrather do it on-top later once we have a reason to do so.\n\n> \n> I also wonder if we shouldn't use the integer control ID as the key,\n> this would simplify DelayedControls::queue() that wouldn't have to look\n> up the ControlId pointer from the idmap.\n\nI'm a bit debated here. Once more I can see pros and cons with both. How \nabout I add a todo for now so we don't forget to reevaluate the design \nas we integrat it closer in CameraSensor?\n\n> \n> > +\n> > +\tbool queue(const ControlList &controls);\n> > +\n> > +\tV4L2Device *device_;\n> > +\tstd::unordered_map delays_;\n> > +\tunsigned int maxDelay_;\n> > +\n> > +\tbool running_;\n> > +\tuint32_t firstSequence_;\n> > +\n> > +\tuint32_t queueCount_;\n> > +\tuint32_t writeCount_;\n> > +\tstd::unordered_map values_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */\n> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > new file mode 100644\n> > index 0000000000000000..db2e51f8c93c4755\n> > --- /dev/null\n> > +++ b/src/libcamera/delayed_controls.cpp\n> > @@ -0,0 +1,252 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * delayed_controls.h - Helper to deal with controls that are applied with a delay\n> > + */\n> > +\n> > +#include \"libcamera/internal/delayed_controls.h\"\n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/log.h\"\n> > +#include \"libcamera/internal/v4l2_device.h\"\n> > +\n> > +/**\n> > + * \\file delayed_controls.h\n> > + * \\brief Helper to deal with controls that are applied with a delay\n> \n> s/are applied/take effect/ (matching the documentation of the class\n> below)\n> \n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(DelayedControls)\n> > +\n> > +/**\n> > + * \\class DelayedControls\n> > + * \\brief Helper to deal with controls that take effect with a delay\n> > + *\n> > + * Some sensor controls take effect with a delay as the sensor needs time to\n> > + * adjust, for example exposure and focus. This is a helper class to deal with\n> \n> Focus is a different beast. Let's s/focus/analog gain/, that's a better\n> example.\n> \n> > + * such controls and the intended users are pipeline handlers.\n> > + *\n> > + * The idea is to extend the concept of the buffer depth of a pipeline the\n> > + * application needs to maintain to also cover controls. Just as with buffer\n> > + * depth if the application keeps the number of requests queued above the\n> > + * control depth the controls are guaranteed to take effect for the correct\n> > + * request. The control depth is determined by the control with the greatest\n> > + * delay.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a DelayedControls instance\n> > + * \\param[in] device The V4L2 device the controls have to be applied to\n> > + * \\param[in] delays Map of the numerical V4L2 control ids to their associated\n> > + * delays (in frames)\n> > + *\n> > + * Only controls specified in \\a delays are handled. If it's desired to mix\n> > + * delayed controls and controls that take effect immediately the immediate\n> > + * controls must be listed in the \\a delays map with a delay value of 0.\n> > + */\n> > +DelayedControls::DelayedControls(V4L2Device *device,\n> > +\t\t\t\t const std::unordered_map &delays)\n> > +\t: device_(device), maxDelay_(0)\n> > +{\n> > +\tconst ControlInfoMap &controls = device_->controls();\n> > +\n> > +\t/*\n> > +\t * Create a map of control ids to delays for controls exposed by the\n> > +\t * device.\n> > +\t */\n> > +\tfor (auto const &delay : delays) {\n> > +\t\tauto it = controls.find(delay.first);\n> > +\t\tif (it == controls.end()) {\n> > +\t\t\tLOG(DelayedControls, Error)\n> > +\t\t\t\t<< \"Delay request for control id \"\n> > +\t\t\t\t<< utils::hex(delay.first)\n> > +\t\t\t\t<< \" but control is not exposed by device \"\n> > +\t\t\t\t<< device_->deviceNode();\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tconst ControlId *id = it->first;\n> > +\n> > +\t\tdelays_[id] = delay.second;\n> > +\n> > +\t\tLOG(DelayedControls, Debug)\n> > +\t\t\t<< \"Set a delay of \" << delays_[id]\n> > +\t\t\t<< \" for \" << id->name();\n> > +\n> > +\t\tmaxDelay_ = std::max(maxDelay_, delays_[id]);\n> > +\t}\n> > +\n> > +\treset();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Reset state machine\n> > + *\n> > + * Resets the state machine to a starting position based on control values\n> > + * retrieved from the device.\n> \n> This is a bit terse, and so is the \\class documentation. Reading the\n> documentation only, not the code, it would be quite difficult to use\n> this class :-S\n\nAgreed, it's also a bit by design as much of this design is in flux \nright now. Once we agree on the overall design I will try to expand the \ndocumentation a bit more, we also had this comment in v3,\n\n> Once we agree on the review comments (and a new version is posted to\n> address them if needed), I can help with the documentation if that would\n> be useful.\n\n;-P\n\n> \n> > + */\n> > +void DelayedControls::reset()\n> > +{\n> > +\trunning_ = false;\n> > +\tfirstSequence_ = 0;\n> > +\tqueueCount_ = 1;\n> > +\twriteCount_ = 0;\n> > +\n> > +\t/* Retrieve control as reported by the device. */\n> > +\tstd::vector ids;\n> > +\tfor (auto const &delay : delays_)\n> > +\t\tids.push_back(delay.first->id());\n> > +\n> > +\tControlList controls = device_->getControls(ids);\n> > +\n> > +\t/* Seed the control queue with the controls reported by the device. */\n> > +\tvalues_.clear();\n> > +\tfor (const auto &ctrl : controls) {\n> > +\t\tconst ControlId *id = device_->controls().idmap().at(ctrl.first);\n> > +\t\tvalues_[id][0] = Info(ctrl.second);\n> > +\t}\n> > +}\n> > +\n> > +/**\n> > + * \\brief Push a set of controls on the queue\n> > + * \\param[in] controls List of controls to add to the device queue\n> > + *\n> > + * Push a set of controls to the control queue. This increases the control queue\n> > + * depth by one.\n> > + *\n> > + * \\returns true if \\a controls are accepted, or false otherwise\n> > + */\n> > +bool DelayedControls::push(const ControlList &controls)\n> > +{\n> > +\treturn queue(controls);\n> > +}\n> > +\n> > +bool DelayedControls::queue(const ControlList &controls)\n> > +{\n> > +\t/* Copy state from previous frame. */\n> > +\tfor (auto &ctrl : values_) {\n> > +\t\tInfo &info = ctrl.second[queueCount_];\n> > +\t\tinfo.value = values_[ctrl.first][queueCount_ - 1].value;\n> > +\t\tinfo.updated = false;\n> > +\t}\n> > +\n> > +\t/* Update with new controls. */\n> > +\tconst ControlIdMap &idmap = device_->controls().idmap();\n> > +\tfor (const auto &control : controls) {\n> > +\t\tconst auto &it = idmap.find(control.first);\n> > +\t\tif (it == idmap.end()) {\n> > +\t\t\tLOG(DelayedControls, Warning)\n> > +\t\t\t\t<< \"Unknown control \" << control.first;\n> > +\t\t\treturn false;\n> > +\t\t}\n> > +\n> > +\t\tconst ControlId *id = it->second;\n> > +\n> > +\t\tif (delays_.find(id) == delays_.end())\n> > +\t\t\treturn false;\n> > +\n> > +\t\tInfo &info = values_[id][queueCount_];\n> > +\n> > +\t\tinfo.value = control.second;\n> > +\t\tinfo.updated = true;\n> > +\n> > +\t\tLOG(DelayedControls, Debug)\n> > +\t\t\t<< \"Queuing \" << id->name()\n> > +\t\t\t<< \" to \" << info.value.toString()\n> > +\t\t\t<< \" at index \" << queueCount_;\n> > +\t}\n> > +\n> > +\tqueueCount_++;\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Read back controls in effect at a sequence number\n> > + * \\param[in] sequence The sequence number to get controls for\n> > + *\n> > + * Read back what controls where in effect at a specific sequence number. The\n> > + * history is a ring buffer of 16 entries where new and old values coexist. It's\n> > + * the callers responsibility to not read too old sequence numbers that have been\n> > + * pushed out of the history.\n> > + *\n> > + * Historic values are evicted by pushing new values onto the queue using\n> > + * push(). The max history from the current sequence number that yields valid\n> > + * values are thus 16 minus number of controls pushed.\n> > + *\n> > + * \\return The controls at \\a sequence number\n> > + */\n> > +ControlList DelayedControls::get(uint32_t sequence)\n> > +{\n> > +\tuint32_t adjustedSeq = sequence - firstSequence_ + 1;\n> > +\tunsigned int index = std::max(0, adjustedSeq - maxDelay_);\n> > +\n> > +\tControlList out(device_->controls());\n> > +\tfor (const auto &ctrl : values_) {\n> > +\t\tconst ControlId *id = ctrl.first;\n> > +\t\tconst Info &info = ctrl.second[index];\n> > +\n> > +\t\tout.set(id->id(), info.value);\n> > +\n> > +\t\tLOG(DelayedControls, Debug)\n> > +\t\t\t<< \"Reading \" << id->name()\n> > +\t\t\t<< \" to \" << info.value.toString()\n> > +\t\t\t<< \" at index \" << index;\n> > +\t}\n> > +\n> > +\treturn out;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Inform DelayedControls of the start of a new frame\n> > + * \\param[in] sequence Sequence number of the frame that started\n> > + *\n> > + * Inform the state machine that a new frame has started and of its sequence\n> > + * number. Any user of these helpers is responsible to inform the helper about\n> > + * the start of any frame.This can be connected with ease to the start of a\n> > + * exposure (SOE) V4L2 event.\n> > + */\n> > +void DelayedControls::applyControls(uint32_t sequence)\n> > +{\n> > +\tLOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> > +\n> > +\tif (!running_) {\n> > +\t\tfirstSequence_ = sequence;\n> > +\t\trunning_ = true;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Create control list peaking ahead in the value queue to ensure\n> > +\t * values are set in time to satisfy the sensor delay.\n> > +\t */\n> > +\tControlList out(device_->controls());\n> > +\tfor (const auto &ctrl : values_) {\n> > +\t\tconst ControlId *id = ctrl.first;\n> > +\t\tunsigned int delayDiff = maxDelay_ - delays_[id];\n> > +\t\tunsigned int index = std::max(0, writeCount_ - delayDiff);\n> \n> A reply to the v3 review would also be appreciated for the issue pointed\n> our regarding this code. I still can't see the implementation being\n> correct :-S\n\nComment from v3:\n\n> Let's assume a sensor that only a single control, with a delay of 1\n> frame. It has been pre-programmed before stream start with an initial\n> value X with reset(). Then, two requests are queued before the first\n> SOF, with values A and B. push() is called twice, with A and B.\n> queueCount is thus 3 (initial value set to 1 in reset(), and increased\n> twice by push), and the queue contains X, A and B in positions 0, 1 and\n> 2.\n> \n> The first frame start arrives, frameStart(0) is called. writeCount_ is\n> 0, maxDelay_ is 1, delays_[id] is 1. index will thus be 0, which causes\n> X to be written to the sensor.\n> \n> Isn't that an incorrect behaviour ? Shouldn't B be written instead,\n> given that the value will take effect in frame 1, which will be captured\n> in request B ?\n\nIt may be incorrect behavior in the end. It do match how StaggerdCtrls \nwork and that was my goal to make the transition as non-controversial as \npossible. I think we need to revisit this at some point when we really \nknow what we want here. I am of course open to doing that work now but I \nfear the change in behavior may impact the RPi pipeline and delay this \nseries even longer. I also don't know exactly how we want this to behave \nin the end so I would need input on that.\n\n> \n> > +\t\tconst Info &info = ctrl.second[index];\n> > +\n> > +\t\tif (info.updated) {\n> > +\t\t\tout.set(id->id(), info.value);\n> > +\t\t\tLOG(DelayedControls, Debug)\n> > +\t\t\t\t<< \"Setting \" << id->name()\n> > +\t\t\t\t<< \" to \" << info.value.toString()\n> > +\t\t\t\t<< \" at index \" << index;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\twriteCount_++;\n> > +\n> > +\twhile (writeCount_ >= queueCount_) {\n> > +\t\tLOG(DelayedControls, Debug)\n> > +\t\t\t<< \"Queue is empty, auto queue no-op.\";\n> > +\t\tqueue({});\n> > +\t}\n> > +\n> > +\tdevice_->setControls(&out);\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -12,6 +12,7 @@ libcamera_sources = files([\n> > 'controls.cpp',\n> > 'control_serializer.cpp',\n> > 'control_validator.cpp',\n> > + 'delayed_controls.cpp',\n> > 'device_enumerator.cpp',\n> > 'device_enumerator_sysfs.cpp',\n> > 'event_dispatcher.cpp',\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3583FC0F1C\n\tfor ;\n\tSat, 16 Jan 2021 09:56:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72B27680FA;\n\tSat, 16 Jan 2021 10:56:55 +0100 (CET)","from mail-lj1-x232.google.com (mail-lj1-x232.google.com\n\t[IPv6:2a00:1450:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5AE1660108\n\tfor ;\n\tSat, 16 Jan 2021 10:56:53 +0100 (CET)","by mail-lj1-x232.google.com with SMTP id p13so13069840ljg.2\n\tfor ;\n\tSat, 16 Jan 2021 01:56:53 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tf23sm1214630lfh.196.2021.01.16.01.56.51\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 16 Jan 2021 01:56:51 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"S9A5XLwW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=TZ0/vnq0ix3T3oR1dCIZpiPHRMfF65ucmAY3roRMWx8=;\n\tb=S9A5XLwWsYxrZ1AVkgcrOu3n7JNuihQ1hevuMVL71TR6kP6FPRKjkABcp1K5cMDElG\n\tNQRj0sKjbSqo5SEqvyav3wOm2qHGgU8Fww/1WF+9RF0FdUWV+mBxZkRPWhxESi6wn17F\n\t8+w+7ZFX2uJO2qVjt/5bhoIN1XFju6i0W2tvxuQwDO10xxBxsR4p6QCNl4a61rAwEb5h\n\tAd9RCr1GlJpZLKDOwdZkzGgmJgL2yxkhGRyzRQybT90dCWPTkyf7YAh3e3qfZUUjeD1f\n\tdONn9/wPOQsmYvuZ54dzK/W2p8ZyLvao9UMDF6SftIneW6KRzyNp1kgsK4vjy2awSRpU\n\t4idw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=TZ0/vnq0ix3T3oR1dCIZpiPHRMfF65ucmAY3roRMWx8=;\n\tb=px+Z41d6ygJdWhxlFjrYSPAcE9ggss3PdC9ZwEr/gKiwv7T1/Q/iQAkIl0TvCToShW\n\thVFCG2yN09ugm0mDEkfXRO03nu2xEWt+l33RDBnINM5NNN2mD0jIhNTKmY51YN6upw12\n\tvkKR+2m4FEck+0id3vnXFGGNq9HNe6/YzUgbsjyi944CwcO0/LCG2ozk/WH4QyKgLebF\n\tUaIbYHgcxVafEAfkCEFcUSCZFoWOJQ9vvJaqMGao5Ps7v3fpEXATVazo50qVCW2baTjS\n\t4o97EtvqErIobUvMQGELE84w8WsQNhmoPo+w2xLwQKZJmlOGCQkxXKbzZHecs5bwnQl6\n\tlF2A==","X-Gm-Message-State":"AOAM533E0DSct6gwy+M5mSE1wwjfOPfsmfFlMcgXwNM89o4X+5g1IuhQ\n\te+A2+qAA9JIiimH/PUoqEgBfcA==","X-Google-Smtp-Source":"ABdhPJyccx5zNEpR4aN2fuPHow7m3Yp/ZWtMV4iHclUyJCNTRPQAWzIiGtxzwGPod7pbUGSM5UxYaw==","X-Received":"by 2002:a05:651c:30d:: with SMTP id\n\ta13mr7218423ljp.238.1610791012354; \n\tSat, 16 Jan 2021 01:56:52 -0800 (PST)","Date":"Sat, 16 Jan 2021 10:56:50 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-2-niklas.soderlund@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14561,"web_url":"https://patchwork.libcamera.org/comment/14561/","msgid":"","date":"2021-01-16T10:23:21","subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"HI Niklas and Laurent,\n\nOn Sat, 16 Jan 2021 at 09:56, Niklas Söderlund <\nniklas.soderlund@ragnatech.se> wrote:\n\n> Hi Laurent,\n>\n> Thanks for your feedback.\n>\n> First let me apologise for for the misunderstanding on my side regarding\n> the comments from you in v3 that are not addressed in this version.\n> After reading your comments in addition to offline discussions my\n> understanding of the way forward was to,\n>\n> - Remove my attempt in v1 and v2 to integrate DelayedControls into\n> CameraSensor and leave the work for pipeline handlers. The goal was to\n> not prematurely build a CameraSensor integration which did not suite\n> all users. This I agreed with and it is done.\n>\n> - Likewise my understanding was that we aimed to not make any large\n> redesigns of the DelayedControls logic as it currently performs in\n> lockstep with StaggeredCtrls which it is replacing. Once we had\n> integrated it with RkISP1 replacing the awfully Timeline solution (I'm\n> allowed to cal it names as I designed it :-P) and the IPU3 to allow a\n> IPA base to take shape would we start to fix it up in a way that would\n> solve problems for all pipelines.\n>\n> My view was that this would be done on top as I think the integration in\n> CameraSensor and improving on the DelayedControls is a bit related to\n> each other and that is why I \"ignored\" your comments when sending v4.\n> Judging from your reaction I have misread the situation and for that I'm\n> sorry. I never meant to ignore your comments, I only did so believing we\n> both shared the same understanding.\n>\n> orz\n>\n> I will reply to your comments below and I will copy-paste in your\n> comments from v3 that you have not already yourself.\n>\n> On 2021-01-10 18:07:16 +0200, Laurent Pinchart wrote:\n> > Hi Niklas,\n> >\n> > Thank you for the patch.\n> >\n> > In the subject line, s/applies/apply/\n> >\n> > On Tue, Dec 15, 2020 at 01:48:04AM +0100, Niklas Söderlund wrote:\n> > > Some sensor controls take effect with a delay as the sensor needs time\n> > > to adjust, for example exposure. Add an optional helper DelayedControls\n> > > to help pipelines deal with such controls.\n> > >\n> > > The idea is to provide a queue of controls towards the V4L2 device and\n> > > apply individual controls with the specified delay with the aim to get\n> > > predictable and retrievable control values for any given frame. To do\n> > > this the queue of controls needs to be at least as deep as the control\n> > > with the largest delay.\n> > >\n> > > The DelayedControls needs to be informed of every start of exposure.\n> > > This can be emulated but the helper is designed to be used with this\n> > > event being provide by the kernel thru V4L2 events.\n> >\n> > s/thru/through/\n> >\n> > > This helper is based on StaggeredCtrl from the Raspberry Pi pipeline\n> > > handler but expands on its API. This helpers aims to replace the\n> > > Raspberry Pi implementations and mimics it behavior perfectly.\n> > >\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > * Changes since v2\n> > > - Drop optional argument to reset().\n> > > - Update commit message.\n> > > - Remove usage of Mutex.\n> > > - Rename frameStart() to applyControls)_.\n> > > - Rename ControlInfo to Into.\n> > > - Rename ControlArray to ControlRingBuffer.\n> > > - Drop ControlsDelays and ControlsValues.\n> > > - Sort headers.\n> > > - Rename iterators.\n> > > - Simplify queueCount_ handeling in reset().\n> > > - Add more warnings.\n> > > - Update documentation.\n> > >\n> > > * Changes since v2\n> > > - Improve error logic in queue() as suggested by Jean-Michel Hautbois.\n> > > - s/fistSequence_/firstSequence_/\n> > >\n> > > * Changes since v1\n> > > - Correct copyright to reflect work is derived from Raspberry Pi\n> > > pipeline handler. This was always the intention and was wrong in v1.\n> > > - Rewrite large parts of the documentation.\n> > > - Join two loops to one in DelayedControls::DelayedControls()\n> > > ---\n> > > include/libcamera/internal/delayed_controls.h | 82 ++++++\n> > > src/libcamera/delayed_controls.cpp | 252 ++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 3 files changed, 335 insertions(+)\n> > > create mode 100644 include/libcamera/internal/delayed_controls.h\n> > > create mode 100644 src/libcamera/delayed_controls.cpp\n> > >\n> > > diff --git a/include/libcamera/internal/delayed_controls.h\n> b/include/libcamera/internal/delayed_controls.h\n> > > new file mode 100644\n> > > index 0000000000000000..1292b484ec9f53e9\n> > > --- /dev/null\n> > > +++ b/include/libcamera/internal/delayed_controls.h\n> > > @@ -0,0 +1,82 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > > + *\n> > > + * delayed_controls.h - Helper to deal with controls that are applied\n> with a delay\n> > > + */\n> > > +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > > +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class V4L2Device;\n> > > +\n> > > +class DelayedControls\n> > > +{\n> > > +public:\n> > > + DelayedControls(V4L2Device *device,\n> > > + const std::unordered_map\n> &delays);\n> >\n> > I've sent the following comment on v3:\n> >\n> > > I could imagine this being useful for non-V4L2 devices too. We could\n> > > instead pass a ControlInfoMap to the constructor, and move the\n> > > setControls() call to the user of this class. There's no need to do so\n> > > now (but of course if you think it's a good idea, I'm not asking to\n> > > delay such rework :-)), but could you capture this in a \\todo comment\n> > > (if you agree with the idea) ?\n> >\n> > I see neither a rework of the API, nor a \\todo comment. Is that an\n> > oversight, or did you disagree with the idea ? In the latter case, a\n> > reply to the review would be nice.\n>\n> Unluckily for my situation, this is a comment I do not agree with :-)\n>\n> Maybe down the road once we know the design of how we want to integrate\n> this with CameraSensor and what type of other non-V4L2 devices this\n> could be useful for I might be see it differently. Foe now I think we\n> should keep it as is as, at least until we know how to integrate it with\n> CameraSensor which I think should be the next stop done on top of this\n> (together with integrating the sensor database).\n>\n> >\n> > > +\n> > > + void reset();\n> > > +\n> > > + bool push(const ControlList &controls);\n> > > + ControlList get(uint32_t sequence);\n> > > +\n> > > + void applyControls(uint32_t sequence);\n> > > +\n> > > +private:\n> > > + class Info\n> > > + {\n> > > + public:\n> > > + Info()\n> > > + : updated(false)\n> > > + {\n> > > + }\n> > > +\n> > > + Info(const ControlValue &v)\n> > > + : value(v), updated(true)\n> > > + {\n> > > + }\n> > > +\n> > > + ControlValue value;\n> > > + bool updated;\n> > > + };\n> >\n> > Same here, v3 had the following review comment:\n> >\n> > > You could also make this class inherit from ControlValue, to avoid the\n> > > explicit .value below.\n>\n> This one I agree with but \"ignored\" as to keep the deign as close to\n> StaggerdCtrls as possible, I will do so for v5.\n>\n> >\n> > > +\n> > > + /* \\todo: Make the listSize configurable at instance creation\n> time. */\n> > > + static constexpr int listSize = 16;\n> > > + class ControlRingBuffer : public std::array\n> > > + {\n> > > + public:\n> > > + Info &operator[](unsigned int index)\n> > > + {\n> > > + return std::array listSize>::operator[](index % listSize);\n> > > + }\n> > > +\n> > > + const Info &operator[](unsigned int index) const\n> > > + {\n> > > + return std::array listSize>::operator[](index % listSize);\n> > > + }\n> > > + };\n> >\n> > There were further comments in the review of v3 that have also not been\n> > addressed.\n>\n> Once more sorry for the misunderstanding, the comments where,\n>\n> > As these two types are only used once, below in the class declaration,\n> > how about dropping the aliases ? Aliases are useful to avoid typing\n> > types out explicitly in every location, but they have the drawback of\n> > possibly obfuscating the code. I would then rename the above ControlInfo\n> > to Value, as it stores a value.\n>\n> This is mostly done in v4, ControlInfo was renamed INfo and not Value.\n>\n> >\n> > I wonder if we should bundle the delay and array in a single class, to\n> > have a single map (now or on top).\n>\n> I can see pros and cons with both solutions. I don't feel strongly about\n> either one, unless we find a good reason to rework this now I would\n> rather do it on-top later once we have a reason to do so.\n>\n> >\n> > I also wonder if we shouldn't use the integer control ID as the key,\n> > this would simplify DelayedControls::queue() that wouldn't have to look\n> > up the ControlId pointer from the idmap.\n>\n> I'm a bit debated here. Once more I can see pros and cons with both. How\n> about I add a todo for now so we don't forget to reevaluate the design\n> as we integrat it closer in CameraSensor?\n>\n> >\n> > > +\n> > > + bool queue(const ControlList &controls);\n> > > +\n> > > + V4L2Device *device_;\n> > > + std::unordered_map delays_;\n> > > + unsigned int maxDelay_;\n> > > +\n> > > + bool running_;\n> > > + uint32_t firstSequence_;\n> > > +\n> > > + uint32_t queueCount_;\n> > > + uint32_t writeCount_;\n> > > + std::unordered_map values_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */\n> > > diff --git a/src/libcamera/delayed_controls.cpp\n> b/src/libcamera/delayed_controls.cpp\n> > > new file mode 100644\n> > > index 0000000000000000..db2e51f8c93c4755\n> > > --- /dev/null\n> > > +++ b/src/libcamera/delayed_controls.cpp\n> > > @@ -0,0 +1,252 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > > + *\n> > > + * delayed_controls.h - Helper to deal with controls that are applied\n> with a delay\n> > > + */\n> > > +\n> > > +#include \"libcamera/internal/delayed_controls.h\"\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/log.h\"\n> > > +#include \"libcamera/internal/v4l2_device.h\"\n> > > +\n> > > +/**\n> > > + * \\file delayed_controls.h\n> > > + * \\brief Helper to deal with controls that are applied with a delay\n> >\n> > s/are applied/take effect/ (matching the documentation of the class\n> > below)\n> >\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(DelayedControls)\n> > > +\n> > > +/**\n> > > + * \\class DelayedControls\n> > > + * \\brief Helper to deal with controls that take effect with a delay\n> > > + *\n> > > + * Some sensor controls take effect with a delay as the sensor needs\n> time to\n> > > + * adjust, for example exposure and focus. This is a helper class to\n> deal with\n> >\n> > Focus is a different beast. Let's s/focus/analog gain/, that's a better\n> > example.\n> >\n> > > + * such controls and the intended users are pipeline handlers.\n> > > + *\n> > > + * The idea is to extend the concept of the buffer depth of a\n> pipeline the\n> > > + * application needs to maintain to also cover controls. Just as with\n> buffer\n> > > + * depth if the application keeps the number of requests queued above\n> the\n> > > + * control depth the controls are guaranteed to take effect for the\n> correct\n> > > + * request. The control depth is determined by the control with the\n> greatest\n> > > + * delay.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct a DelayedControls instance\n> > > + * \\param[in] device The V4L2 device the controls have to be applied\n> to\n> > > + * \\param[in] delays Map of the numerical V4L2 control ids to their\n> associated\n> > > + * delays (in frames)\n> > > + *\n> > > + * Only controls specified in \\a delays are handled. If it's desired\n> to mix\n> > > + * delayed controls and controls that take effect immediately the\n> immediate\n> > > + * controls must be listed in the \\a delays map with a delay value of\n> 0.\n> > > + */\n> > > +DelayedControls::DelayedControls(V4L2Device *device,\n> > > + const std::unordered_map unsigned int> &delays)\n> > > + : device_(device), maxDelay_(0)\n> > > +{\n> > > + const ControlInfoMap &controls = device_->controls();\n> > > +\n> > > + /*\n> > > + * Create a map of control ids to delays for controls exposed by\n> the\n> > > + * device.\n> > > + */\n> > > + for (auto const &delay : delays) {\n> > > + auto it = controls.find(delay.first);\n> > > + if (it == controls.end()) {\n> > > + LOG(DelayedControls, Error)\n> > > + << \"Delay request for control id \"\n> > > + << utils::hex(delay.first)\n> > > + << \" but control is not exposed by device \"\n> > > + << device_->deviceNode();\n> > > + continue;\n> > > + }\n> > > +\n> > > + const ControlId *id = it->first;\n> > > +\n> > > + delays_[id] = delay.second;\n> > > +\n> > > + LOG(DelayedControls, Debug)\n> > > + << \"Set a delay of \" << delays_[id]\n> > > + << \" for \" << id->name();\n> > > +\n> > > + maxDelay_ = std::max(maxDelay_, delays_[id]);\n> > > + }\n> > > +\n> > > + reset();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Reset state machine\n> > > + *\n> > > + * Resets the state machine to a starting position based on control\n> values\n> > > + * retrieved from the device.\n> >\n> > This is a bit terse, and so is the \\class documentation. Reading the\n> > documentation only, not the code, it would be quite difficult to use\n> > this class :-S\n>\n> Agreed, it's also a bit by design as much of this design is in flux\n> right now. Once we agree on the overall design I will try to expand the\n> documentation a bit more, we also had this comment in v3,\n>\n> > Once we agree on the review comments (and a new version is posted to\n> > address them if needed), I can help with the documentation if that would\n> > be useful.\n>\n> ;-P\n>\n> >\n> > > + */\n> > > +void DelayedControls::reset()\n> > > +{\n> > > + running_ = false;\n> > > + firstSequence_ = 0;\n> > > + queueCount_ = 1;\n> > > + writeCount_ = 0;\n> > > +\n> > > + /* Retrieve control as reported by the device. */\n> > > + std::vector ids;\n> > > + for (auto const &delay : delays_)\n> > > + ids.push_back(delay.first->id());\n> > > +\n> > > + ControlList controls = device_->getControls(ids);\n> > > +\n> > > + /* Seed the control queue with the controls reported by the\n> device. */\n> > > + values_.clear();\n> > > + for (const auto &ctrl : controls) {\n> > > + const ControlId *id =\n> device_->controls().idmap().at(ctrl.first);\n> > > + values_[id][0] = Info(ctrl.second);\n> > > + }\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Push a set of controls on the queue\n> > > + * \\param[in] controls List of controls to add to the device queue\n> > > + *\n> > > + * Push a set of controls to the control queue. This increases the\n> control queue\n> > > + * depth by one.\n> > > + *\n> > > + * \\returns true if \\a controls are accepted, or false otherwise\n> > > + */\n> > > +bool DelayedControls::push(const ControlList &controls)\n> > > +{\n> > > + return queue(controls);\n> > > +}\n> > > +\n> > > +bool DelayedControls::queue(const ControlList &controls)\n> > > +{\n> > > + /* Copy state from previous frame. */\n> > > + for (auto &ctrl : values_) {\n> > > + Info &info = ctrl.second[queueCount_];\n> > > + info.value = values_[ctrl.first][queueCount_ - 1].value;\n> > > + info.updated = false;\n> > > + }\n> > > +\n> > > + /* Update with new controls. */\n> > > + const ControlIdMap &idmap = device_->controls().idmap();\n> > > + for (const auto &control : controls) {\n> > > + const auto &it = idmap.find(control.first);\n> > > + if (it == idmap.end()) {\n> > > + LOG(DelayedControls, Warning)\n> > > + << \"Unknown control \" << control.first;\n> > > + return false;\n> > > + }\n> > > +\n> > > + const ControlId *id = it->second;\n> > > +\n> > > + if (delays_.find(id) == delays_.end())\n> > > + return false;\n> > > +\n> > > + Info &info = values_[id][queueCount_];\n> > > +\n> > > + info.value = control.second;\n> > > + info.updated = true;\n> > > +\n> > > + LOG(DelayedControls, Debug)\n> > > + << \"Queuing \" << id->name()\n> > > + << \" to \" << info.value.toString()\n> > > + << \" at index \" << queueCount_;\n> > > + }\n> > > +\n> > > + queueCount_++;\n> > > +\n> > > + return true;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Read back controls in effect at a sequence number\n> > > + * \\param[in] sequence The sequence number to get controls for\n> > > + *\n> > > + * Read back what controls where in effect at a specific sequence\n> number. The\n> > > + * history is a ring buffer of 16 entries where new and old values\n> coexist. It's\n> > > + * the callers responsibility to not read too old sequence numbers\n> that have been\n> > > + * pushed out of the history.\n> > > + *\n> > > + * Historic values are evicted by pushing new values onto the queue\n> using\n> > > + * push(). The max history from the current sequence number that\n> yields valid\n> > > + * values are thus 16 minus number of controls pushed.\n> > > + *\n> > > + * \\return The controls at \\a sequence number\n> > > + */\n> > > +ControlList DelayedControls::get(uint32_t sequence)\n> > > +{\n> > > + uint32_t adjustedSeq = sequence - firstSequence_ + 1;\n> > > + unsigned int index = std::max(0, adjustedSeq - maxDelay_);\n> > > +\n> > > + ControlList out(device_->controls());\n> > > + for (const auto &ctrl : values_) {\n> > > + const ControlId *id = ctrl.first;\n> > > + const Info &info = ctrl.second[index];\n> > > +\n> > > + out.set(id->id(), info.value);\n> > > +\n> > > + LOG(DelayedControls, Debug)\n> > > + << \"Reading \" << id->name()\n> > > + << \" to \" << info.value.toString()\n> > > + << \" at index \" << index;\n> > > + }\n> > > +\n> > > + return out;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Inform DelayedControls of the start of a new frame\n> > > + * \\param[in] sequence Sequence number of the frame that started\n> > > + *\n> > > + * Inform the state machine that a new frame has started and of its\n> sequence\n> > > + * number. Any user of these helpers is responsible to inform the\n> helper about\n> > > + * the start of any frame.This can be connected with ease to the\n> start of a\n> > > + * exposure (SOE) V4L2 event.\n> > > + */\n> > > +void DelayedControls::applyControls(uint32_t sequence)\n> > > +{\n> > > + LOG(DelayedControls, Debug) << \"frame \" << sequence << \" started\";\n> > > +\n> > > + if (!running_) {\n> > > + firstSequence_ = sequence;\n> > > + running_ = true;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Create control list peaking ahead in the value queue to ensure\n> > > + * values are set in time to satisfy the sensor delay.\n> > > + */\n> > > + ControlList out(device_->controls());\n> > > + for (const auto &ctrl : values_) {\n> > > + const ControlId *id = ctrl.first;\n> > > + unsigned int delayDiff = maxDelay_ - delays_[id];\n> > > + unsigned int index = std::max(0, writeCount_ -\n> delayDiff);\n> >\n> > A reply to the v3 review would also be appreciated for the issue pointed\n> > our regarding this code. I still can't see the implementation being\n> > correct :-S\n>\n> Comment from v3:\n>\n> > Let's assume a sensor that only a single control, with a delay of 1\n> > frame. It has been pre-programmed before stream start with an initial\n> > value X with reset(). Then, two requests are queued before the first\n> > SOF, with values A and B. push() is called twice, with A and B.\n> > queueCount is thus 3 (initial value set to 1 in reset(), and increased\n> > twice by push), and the queue contains X, A and B in positions 0, 1 and\n> > 2.\n> >\n> > The first frame start arrives, frameStart(0) is called. writeCount_ is\n> > 0, maxDelay_ is 1, delays_[id] is 1. index will thus be 0, which causes\n> > X to be written to the sensor.\n> >\n> > Isn't that an incorrect behaviour ? Shouldn't B be written instead,\n> > given that the value will take effect in frame 1, which will be captured\n> > in request B ?\n>\n> It may be incorrect behavior in the end. It do match how StaggerdCtrls\n> work and that was my goal to make the transition as non-controversial as\n> possible. I think we need to revisit this at some point when we really\n> know what we want here. I am of course open to doing that work now but I\n> fear the change in behavior may impact the RPi pipeline and delay this\n> series even longer. I also don't know exactly how we want this to behave\n> in the end so I would need input on that.\n>\n\nWith regards to the above usage described by Laurent - I should clarify\nthat staggered ctrl essentially has a storage depth of 1. By that I mean,\nif you attempt to queue controls via multiple calls to set(), only the last\ncall to set() is taken into account. This is the same if the pipeline is\nstreaming or not. Only once we write the last provided controls to the\nsensor through write() can any new controls be set() to be applied in\nfuture frames. To go into a bit more detail, the setCount index into the\ncontrol array is only incremented in write(), and not on each call to set().\n\nOne of the key differences with DelayedCtrl is you can queue up controls\nwith multiple calls to push(). This is something that the Raspberry Pi IPA\nnever ever does, so I did not add support for it in staggered ctrl. So\nany fixes to the queuing behavior should not affect the Raspberry Pi\nstack.... I think :-)\n\nRegards,\nNaush\n\n\n\n>\n> >\n> > > + const Info &info = ctrl.second[index];\n> > > +\n> > > + if (info.updated) {\n> > > + out.set(id->id(), info.value);\n> > > + LOG(DelayedControls, Debug)\n> > > + << \"Setting \" << id->name()\n> > > + << \" to \" << info.value.toString()\n> > > + << \" at index \" << index;\n> > > + }\n> > > + }\n> > > +\n> > > + writeCount_++;\n> > > +\n> > > + while (writeCount_ >= queueCount_) {\n> > > + LOG(DelayedControls, Debug)\n> > > + << \"Queue is empty, auto queue no-op.\";\n> > > + queue({});\n> > > + }\n> > > +\n> > > + device_->setControls(&out);\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -12,6 +12,7 @@ libcamera_sources = files([\n> > > 'controls.cpp',\n> > > 'control_serializer.cpp',\n> > > 'control_validator.cpp',\n> > > + 'delayed_controls.cpp',\n> > > 'device_enumerator.cpp',\n> > > 'device_enumerator_sysfs.cpp',\n> > > 'event_dispatcher.cpp',\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n>\n> --\n> Regards,\n> Niklas Söderlund\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3B417C0F1C\n\tfor ;\n\tSat, 16 Jan 2021 10:23:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A56EC68107;\n\tSat, 16 Jan 2021 11:23:40 +0100 (CET)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A731E680D0\n\tfor ;\n\tSat, 16 Jan 2021 11:23:38 +0100 (CET)","by mail-lf1-x12d.google.com with SMTP id x20so16906479lfe.12\n\tfor ;\n\tSat, 16 Jan 2021 02:23:38 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"XgJebCiC\"; 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=6U+7/rZ2mM4q3tnxZxYzyiAWC1OcU8mi6ayTubBSKrM=;\n\tb=XgJebCiC/Gx7jGHhisjwDrWT0juSGFJSs5MTRAzwhRzgB/cVpcU+y93EFTUx6kW2Qi\n\tGXnbzLJHAJNbYSn73045coJUGeO1q65XyCyWz7NVC13XAStUn/ays0gSo1VzsW9fFoW1\n\ths9jpqZFy4vWqISjK3iSQ9SrttApCuax7VGHrq4CFQTFHwaHULKARN3n9bl7ZlZm2kfq\n\t7fk9AVRHMRY2D6yL+SWe9J/2jQMubDAxkHQ7lwBXqos5rLUj/bhxdIQamTRzT6aqq+Ci\n\tvWVvK2PgQ4CCuL6sbhmWFwEM9h3uAln/cBFPq8kBTVdexQZOYE2dYBi17kVw43TFNhHo\n\txXgw==","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=6U+7/rZ2mM4q3tnxZxYzyiAWC1OcU8mi6ayTubBSKrM=;\n\tb=mRPvF5OltQ4QKsHVlGmKzyiIqeODpAgVA97BawZM2kdgp3RvtnHnzGvBhNfygL8Ujw\n\tFhKXnfG8g1BpNvy9SE/gaaP63taEJlNOcSF9On5gsaZMBr71F7njpF1sUhGs3PpgdgPT\n\t9Qpt/tf7LqNtRUdeJ1LeuLdLksq97aUR6uY0hmiciRPsM2gH9FAHOgjSWbpvpyIstr+1\n\taScReIAyMyfizDJqM7ouVvOUFF2/+gZZhAyLUdIcEerkxHM/4EE6+WTjgcNwtfpG9cBG\n\ta2LVNhP8EAcviY4TyHUdRaqEwg7L+eeNdGRBdp+hu5//Jz/t9/7bVVqMgKt6sIBhC2o8\n\tZ3mA==","X-Gm-Message-State":"AOAM530cstZk/JX509Lbju13SWecYLc98Pu2hqTbgH8xNinELnFe9vah\n\trWIvXJUW0Mo7cPf+7WAeWzp6752SMaVVk1Rdgdgg9IhsZ62oow==","X-Google-Smtp-Source":"ABdhPJwjU1QavQ4jlb1GuBKl0GU2lzd8KJGEh8COVjS4SwJB3G95KD3oa7sx/dxJLpori1QoxfE0Krr+91YYqYXzYn0=","X-Received":"by 2002:a19:e8e:: with SMTP id 136mr7717943lfo.272.1610792617949;\n\tSat, 16 Jan 2021 02:23:37 -0800 (PST)","MIME-Version":"1.0","References":"<20201215004811.602429-1-niklas.soderlund@ragnatech.se>\n\t<20201215004811.602429-2-niklas.soderlund@ragnatech.se>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Sat, 16 Jan 2021 10:23:21 +0000","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls:\n\tAdd helper for controls that applies with a delay","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============3820907330916396158==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]