[{"id":15350,"web_url":"https://patchwork.libcamera.org/comment/15350/","msgid":"<20210301092457.GC3084@pyrite.rasen.tech>","date":"2021-03-01T09:24:57","subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: delayed_controls:\n\tRemove unneeded write when starting up","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Feb 16, 2021 at 08:53:40AM +0000, Naushir Patuck wrote:\n> On DelayedControls::reset(), the values retrieved from the sensor device\n> were added to the queues with the updated flag set to true. This would\n> cause the helper to write out the value to the device again on the first\n> DelayedControls::applyControls() call. This is unnecessary, as the\n> controls written are identical to what is stored in the device driver.\n> \n> Fix this by explicitly setting the update flag to false in\n> DelayedControls::reset() when adding the controls to the queue.\n> \n> Additionally, use the Info() constructor when adding items to the queue\n> for consistency.\n> \n> Signed-off-by: Naushir Patuck \n> Fixes: 3d4b7b005911 (\"libcamera: delayed_controls: Add helper for controls that apply with a delay\")\n> ---\n> include/libcamera/internal/delayed_controls.h | 4 ++--\n> src/libcamera/delayed_controls.cpp | 14 ++++++++------\n> 2 files changed, 10 insertions(+), 8 deletions(-)\n> \n> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\n> index 564d9f2e2440..2a6a912bde10 100644\n> --- a/include/libcamera/internal/delayed_controls.h\n> +++ b/include/libcamera/internal/delayed_controls.h\n> @@ -43,8 +43,8 @@ private:\n> \t\t{\n> \t\t}\n> \n> -\t\tInfo(const ControlValue &v)\n> -\t\t\t: ControlValue(v), updated(true)\n> +\t\tInfo(const ControlValue &v, bool updated_ = true)\n> +\t\t\t: ControlValue(v), updated(updated_)\n> \t\t{\n> \t\t}\n> \n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> index 3ed1dfebd035..21dc3e164fe9 100644\n> --- a/src/libcamera/delayed_controls.cpp\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -111,7 +111,11 @@ void DelayedControls::reset()\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\t/*\n> +\t\t * Do not mark this control value as updated, it does not need\n> +\t\t * to be written to to device on startup.\n> +\t\t */\n> +\t\tvalues_[id][0] = Info(ctrl.second, false);\n> \t}\n> }\n> \n> @@ -126,11 +130,10 @@ void DelayedControls::reset()\n> */\n> bool DelayedControls::push(const ControlList &controls)\n> {\n> -\t/* Copy state from previous frame. */\n> +\t/* Copy state from previous frame and clear the updated flag */\n> \tfor (auto &ctrl : values_) {\n> \t\tInfo &info = ctrl.second[queueCount_];\n> -\t\tinfo = values_[ctrl.first][queueCount_ - 1];\n> -\t\tinfo.updated = false;\n> +\t\tinfo = Info(values_[ctrl.first][queueCount_ - 1], false);\n\nIsn't values_[ctrl.first][queueCount_ - 1] an Info instance? I didn't\nthink implicit copy constructor worked with the extra argument.\n\nThe rest looks good to me.\n\n\nPaul\n\n> \t}\n> \n> \t/* Update with new controls. */\n> @@ -150,8 +153,7 @@ bool DelayedControls::push(const ControlList &controls)\n> \n> \t\tInfo &info = values_[id][queueCount_];\n> \n> -\t\tinfo = control.second;\n> -\t\tinfo.updated = true;\n> +\t\tinfo = Info(control.second);\n> \n> \t\tLOG(DelayedControls, Debug)\n> \t\t\t<< \"Queuing \" << id->name()\n> -- \n> 2.25.1\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 34D72BD808\n\tfor ;\n\tMon, 1 Mar 2021 09:25:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F315D68A7E;\n\tMon, 1 Mar 2021 10:25:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 90BEB60521\n\tfor ;\n\tMon, 1 Mar 2021 10:25:05 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 05BDF332;\n\tMon, 1 Mar 2021 10:25:03 +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=\"avxHOj3S\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614590705;\n\tbh=xSd2GZeVB4iALxe63DNMe2MJHdQizZITihMSf1DIzKA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=avxHOj3S6hBMyCLJZQNRo/xuIx2ohr+S0N9Pg5tBC2hBCRqEghWxklKMViQMVQYap\n\tnX0fS9tJqTTmENKIvC9W7LBJnubWA48D0vXD2d8A8LPpnsZmSmA+1PkIvb5gd94Q5f\n\twSw0eO0GpuN0/kSFR69ICBzG77/MTnkbD1iJbr1Q=","Date":"Mon, 1 Mar 2021 18:24:57 +0900","From":"paul.elder@ideasonboard.com","To":"Naushir Patuck ","Message-ID":"<20210301092457.GC3084@pyrite.rasen.tech>","References":"<20210216085342.1012717-1-naush@raspberrypi.com>\n\t<20210216085342.1012717-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210216085342.1012717-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: delayed_controls:\n\tRemove unneeded write when starting up","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15354,"web_url":"https://patchwork.libcamera.org/comment/15354/","msgid":"","date":"2021-03-01T09:45:42","subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: delayed_controls:\n\tRemove unneeded write when starting up","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Paul,\n\nThank you for your review feedback.\n\nOn Mon, 1 Mar 2021 at 09:25, wrote:\n\n> Hi Naush,\n>\n> On Tue, Feb 16, 2021 at 08:53:40AM +0000, Naushir Patuck wrote:\n> > On DelayedControls::reset(), the values retrieved from the sensor device\n> > were added to the queues with the updated flag set to true. This would\n> > cause the helper to write out the value to the device again on the first\n> > DelayedControls::applyControls() call. This is unnecessary, as the\n> > controls written are identical to what is stored in the device driver.\n> >\n> > Fix this by explicitly setting the update flag to false in\n> > DelayedControls::reset() when adding the controls to the queue.\n> >\n> > Additionally, use the Info() constructor when adding items to the queue\n> > for consistency.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Fixes: 3d4b7b005911 (\"libcamera: delayed_controls: Add helper for\n> controls that apply with a delay\")\n> > ---\n> > include/libcamera/internal/delayed_controls.h | 4 ++--\n> > src/libcamera/delayed_controls.cpp | 14 ++++++++------\n> > 2 files changed, 10 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/delayed_controls.h\n> b/include/libcamera/internal/delayed_controls.h\n> > index 564d9f2e2440..2a6a912bde10 100644\n> > --- a/include/libcamera/internal/delayed_controls.h\n> > +++ b/include/libcamera/internal/delayed_controls.h\n> > @@ -43,8 +43,8 @@ private:\n> > {\n> > }\n> >\n> > - Info(const ControlValue &v)\n> > - : ControlValue(v), updated(true)\n> > + Info(const ControlValue &v, bool updated_ = true)\n> > + : ControlValue(v), updated(updated_)\n> > {\n> > }\n> >\n> > diff --git a/src/libcamera/delayed_controls.cpp\n> b/src/libcamera/delayed_controls.cpp\n> > index 3ed1dfebd035..21dc3e164fe9 100644\n> > --- a/src/libcamera/delayed_controls.cpp\n> > +++ b/src/libcamera/delayed_controls.cpp\n> > @@ -111,7 +111,11 @@ void DelayedControls::reset()\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> > + * Do not mark this control value as updated, it does not\n> need\n> > + * to be written to to device on startup.\n> > + */\n> > + values_[id][0] = Info(ctrl.second, false);\n> > }\n> > }\n> >\n> > @@ -126,11 +130,10 @@ void DelayedControls::reset()\n> > */\n> > bool DelayedControls::push(const ControlList &controls)\n> > {\n> > - /* Copy state from previous frame. */\n> > + /* Copy state from previous frame and clear the updated flag */\n> > for (auto &ctrl : values_) {\n> > Info &info = ctrl.second[queueCount_];\n> > - info = values_[ctrl.first][queueCount_ - 1];\n> > - info.updated = false;\n> > + info = Info(values_[ctrl.first][queueCount_ - 1], false);\n>\n> Isn't values_[ctrl.first][queueCount_ - 1] an Info instance? I didn't\n> think implicit copy constructor worked with the extra argument.\n>\n\nIt works as expected, but does seem confusing. I will update the code\nabove to be more explicit.\n\nThanks,\nNaush\n\n\n\n>\n> The rest looks good to me.\n>\n>\n> Paul\n>\n> > }\n> >\n> > /* Update with new controls. */\n> > @@ -150,8 +153,7 @@ bool DelayedControls::push(const ControlList\n> &controls)\n> >\n> > Info &info = values_[id][queueCount_];\n> >\n> > - info = control.second;\n> > - info.updated = true;\n> > + info = Info(control.second);\n> >\n> > LOG(DelayedControls, Debug)\n> > << \"Queuing \" << id->name()\n> > --\n> > 2.25.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 F079FBD1F1\n\tfor ;\n\tMon, 1 Mar 2021 09:46:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A9D5689DD;\n\tMon, 1 Mar 2021 10:46:01 +0100 (CET)","from mail-lf1-x134.google.com (mail-lf1-x134.google.com\n\t[IPv6:2a00:1450:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8002A60521\n\tfor ;\n\tMon, 1 Mar 2021 10:45:59 +0100 (CET)","by mail-lf1-x134.google.com with SMTP id d3so24382903lfg.10\n\tfor ;\n\tMon, 01 Mar 2021 01:45:59 -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=\"A7xzciKy\"; 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=bjD9ArfuPnkWk1mHiVaHoDKLo7G+GTFrAxu6ZI/pfAo=;\n\tb=A7xzciKyL+vfCO0+QbBYM/FX4vZX5lr06J5Tx1EYginZ90+7X/Fv1izHpKwHKcv75k\n\tZXMkws17gjFS17bAc3N9KetrUJRlPZjnfGeI/Dk/p5vYQ2TVIN2TWJgUET+Lbfo3WJzW\n\tVI+qyFVZcAd7mgls267xk1FXN2FYOTIO3MrxGvOXp/a6XqAluwhaz3/eqy3s3MyGqvgc\n\tTn10Gjm7HqBtiUGgFzOss7gKzWX+jgjXKhRvOfIau1kT5E+W+VXjj7IhsOn2MfbA8yoC\n\t4Y/aOnjl/pvnYpBI6UfcK6Cs6WWVQRedoeI5Pz/vvpMmFqlQdXFV9tMhad4nII4Eufc0\n\t2zsQ==","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=bjD9ArfuPnkWk1mHiVaHoDKLo7G+GTFrAxu6ZI/pfAo=;\n\tb=VzOHB4KvucbCDhQG/CZjAAFvVmWb7Kun3Nmx3oRgTIq8piHeVXyMgF1dU/9cbLfo+j\n\ta6/rzcIJCTlZm4KtTNJYHAs1V3P3CS9Z8tCHXphYlMbbCTXLkOOguPU3LQ+KHT5fClI/\n\t9iyPSvq7mEiShO+QFQfSRp5Q+082uql4iWVfan+/XS+SDDyTD7y5WhDsnyWsLjX6J11V\n\tK0XF1Zt/G3E93V4sYk4+VPzzfjlMomSE1fxHNLRhFTnvYWDBU9f7Ve9/IM8rCMf1lN4r\n\t9/szcsGbjXwJa4w78mYJ2tSWDsUu49/zjVei4SfXb0YLD+lNfxXxuwdeRyF05pz+EUTQ\n\tQaqA==","X-Gm-Message-State":"AOAM533V+EP40+DsHqc2qVG7ILqaZ3otJTxKHxHgzDr9veOo47tgLpfa\n\t4AanAol5c52h+hPtoFQAxwwwAu7iaHx0/rfsg6fZH70wLo2VrA==","X-Google-Smtp-Source":"ABdhPJzSF4VnfDaRtYc01PaenD8CsVSzBXMUNvYB1tMBmtOHyPUKWLdu50jZrBFe29EU76QLrNMe5DQehpwYAPi7xpU=","X-Received":"by 2002:a05:6512:3226:: with SMTP id\n\tf6mr8635341lfe.171.1614591958961; \n\tMon, 01 Mar 2021 01:45:58 -0800 (PST)","MIME-Version":"1.0","References":"<20210216085342.1012717-1-naush@raspberrypi.com>\n\t<20210216085342.1012717-4-naush@raspberrypi.com>\n\t<20210301092457.GC3084@pyrite.rasen.tech>","In-Reply-To":"<20210301092457.GC3084@pyrite.rasen.tech>","From":"Naushir Patuck ","Date":"Mon, 1 Mar 2021 09:45:42 +0000","Message-ID":"","To":"Paul Elder ","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] libcamera: delayed_controls:\n\tRemove unneeded write when starting up","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=\"===============9016479153355871319==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]