Patch Detail
Show a patch.
GET /api/1.1/patches/11254/?format=api
{ "id": 11254, "url": "https://patchwork.libcamera.org/api/1.1/patches/11254/?format=api", "web_url": "https://patchwork.libcamera.org/patch/11254/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20210212113312.239076-2-naush@raspberrypi.com>", "date": "2021-02-12T11:33:08", "name": "[libcamera-devel,1/5] libcamera: delayed_controls: Add notion of priority write", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "6ce8f4bff69c2918153c4d2b20c470a94e114dc3", "submitter": { "id": 34, "url": "https://patchwork.libcamera.org/api/1.1/people/34/?format=api", "name": "Naushir Patuck", "email": "naush@raspberrypi.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/11254/mbox/", "series": [ { "id": 1683, "url": "https://patchwork.libcamera.org/api/1.1/series/1683/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=1683", "date": "2021-02-12T11:33:07", "name": "DelayedControls updates and fixes", "version": 1, "mbox": "https://patchwork.libcamera.org/series/1683/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/11254/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/11254/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "X-Original-To": "parsemail@patchwork.libcamera.org", "Delivered-To": "parsemail@patchwork.libcamera.org", "Received": [ "from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 37528BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 12 Feb 2021 11:37:35 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 072FE6377E;\n\tFri, 12 Feb 2021 12:37:35 +0100 (CET)", "from mail-wm1-x32e.google.com (mail-wm1-x32e.google.com\n\t[IPv6:2a00:1450:4864:20::32e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF6A863772\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 12 Feb 2021 12:37:32 +0100 (CET)", "by mail-wm1-x32e.google.com with SMTP id l17so612811wmq.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 12 Feb 2021 03:37:32 -0800 (PST)", "from naush-laptop.patuck.local ([88.97.76.4])\n\tby smtp.gmail.com with ESMTPSA id\n\tv5sm10005020wro.71.2021.02.12.03.37.31\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 12 Feb 2021 03:37:31 -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=\"rH6rByyi\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=from:to:cc:subject:date:message-id:in-reply-to:references\n\t:mime-version:content-transfer-encoding;\n\tbh=z4ZreEGEk3BzO5Jp44WZDMtRVNGW6hoiw0T4y9VtVzw=;\n\tb=rH6rByyi45MkgnjdnhmIx5RUi5oNl/66tpytzh4lugFr8xoFb6YO6PNCO7uZ4moES3\n\tgcgduGu6shgffd5STIiRakmzA7RyGrh4QT0Kzhd7lmXQOHzEP7F66uznKTPil/KREXtM\n\tQweRHuP0T6X/NOSMd4QKPXU4GVrvRpnkePkeZiUD1qVtIh++vb948xcAGpBTIRgrmz6x\n\tugHDJtJZHbNOYjGHYmrGDMDcRhtDkorddqRmE5Q67l6epSgpNbimFBbgGlaHTH4IFxj8\n\t2IAqmRw7U2sJvrVnVZmNQmvZ51UqoShxeYlHaCtwdV6GvaxE6GLLbYF2imxUQ4MJOe5w\n\tsKMg==", "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to\n\t:references:mime-version:content-transfer-encoding;\n\tbh=z4ZreEGEk3BzO5Jp44WZDMtRVNGW6hoiw0T4y9VtVzw=;\n\tb=fYbORLowGwIJ7gcwLTgFD9+TuC4hhQzQCwxkHcaW0b9ZimRfPdfh6QtS2V+tth/39p\n\tSKUoSoWWFO2i5QHDDvVV/nQYUgSaKEoD3885p2Voot1zXkIjxrIk/4QIXCGVIWOwzbMq\n\tZDLMWJ4K1T/EzWUJYcxN4cjghoty187/xLpvSFjOsvric3B8XGH2Y9wVm0/Lvvb02VZZ\n\tnW4Ga11QsWOY1AXurqn/LtHVNV93sn4wgSLZRID8bTPtrLtTlP4T80W+t2yuoXBs+6ZG\n\t8Dy0pg6HI0H1h7wCKTQn9KrT9suuRXhGOB19+qM2aMr6Wiq4E5KDzd6yH6mWDHwKGIAI\n\tbkKQ==", "X-Gm-Message-State": "AOAM532reoK9ZoQnDMrYiextMA5+pZl4no89LSP2LIveum/xMqy6DOTn\n\t/XGq/U/cQqxqEcYlzOPy8GWYhsjoZ7RUQM3p", "X-Google-Smtp-Source": "ABdhPJxbNswklWwKAKoezp8ulfRN7lhNzFhcnWAXYF5SyjFBP0OtSN07AK0PPHyxy6GL0jjJdB8HjQ==", "X-Received": "by 2002:a1c:107:: with SMTP id 7mr2285689wmb.28.1613129852031;\n\tFri, 12 Feb 2021 03:37:32 -0800 (PST)", "From": "Naushir Patuck <naush@raspberrypi.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Fri, 12 Feb 2021 11:33:08 +0000", "Message-Id": "<20210212113312.239076-2-naush@raspberrypi.com>", "X-Mailer": "git-send-email 2.25.1", "In-Reply-To": "<20210212113312.239076-1-naush@raspberrypi.com>", "References": "<20210212113312.239076-1-naush@raspberrypi.com>", "MIME-Version": "1.0", "Subject": "[libcamera-devel] [PATCH 1/5] libcamera: delayed_controls: Add\n\tnotion of priority write", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "7bit", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "If an exposure time change adjusts the vblanking limits, and we write\nboth VBLANK and EXPOSURE controls as a set, the latter may fail if the\nvalue is outside of the limits calculated by the old VBLANK value. This\nis a limitation in V4L2 and cannot be fixed by writing VBLANK before\nEXPOSURE.\n\nThe workaround here is to have the DelayedControls object mark the\nVBLANK control as \"priority write\", which then write VBLANK separately\nfrom (and ahead of) any other controls. This way, the sensor driver will\nupdate the EXPOSURE control with new limits before the new values is\npresented, and will thus be seen as valid.\n\nTo support this, a new struct DelayedControls::ControlParams is used in\nthe constructor to provide the control delay value as well as the\npriority write flag.\n\nSigned-off-by: Naushir Patuck <naush@raspberrypi.com>\n---\n include/libcamera/internal/delayed_controls.h | 9 +++-\n src/libcamera/delayed_controls.cpp | 54 +++++++++++++------\n src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +--\n .../pipeline/raspberrypi/raspberrypi.cpp | 13 +++--\n src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +--\n test/delayed_contols.cpp | 20 +++----\n 6 files changed, 68 insertions(+), 44 deletions(-)", "diff": "diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h\nindex dc447a882514..564d9f2e2440 100644\n--- a/include/libcamera/internal/delayed_controls.h\n+++ b/include/libcamera/internal/delayed_controls.h\n@@ -19,8 +19,13 @@ class V4L2Device;\n class DelayedControls\n {\n public:\n+\tstruct ControlParams {\n+\t\tunsigned int delay;\n+\t\tbool priorityWrite;\n+\t};\n+\n \tDelayedControls(V4L2Device *device,\n-\t\t\tconst std::unordered_map<uint32_t, unsigned int> &delays);\n+\t\t\tconst std::unordered_map<uint32_t, ControlParams> &controlParams);\n \n \tvoid reset();\n \n@@ -64,7 +69,7 @@ private:\n \n \tV4L2Device *device_;\n \t/* \\todo Evaluate if we should index on ControlId * or unsigned int */\n-\tstd::unordered_map<const ControlId *, unsigned int> delays_;\n+\tstd::unordered_map<const ControlId *, ControlParams> controlParams_;\n \tunsigned int maxDelay_;\n \n \tbool running_;\ndiff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\nindex ab1d40057c5f..5317f820b6bd 100644\n--- a/src/libcamera/delayed_controls.cpp\n+++ b/src/libcamera/delayed_controls.cpp\n@@ -40,15 +40,19 @@ LOG_DEFINE_CATEGORY(DelayedControls)\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+ * \\param[in] controlParams Map of the numerical V4L2 control ids to their\n+ * associated control parameters.\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+ * The control parameters comprise of delays (in frames) and a priority write\n+ * flag. If this flag is set, the relevant control is written separately from,\n+ * ahead of the reset of the batched controls.\n+ *\n+ * Only controls specified in \\a controlParams are handled. If it's desired to\n+ * mix delayed controls and controls that take effect immediately the immediate\n+ * controls must be listed in the \\a controlParams map with a delay value of 0.\n */\n DelayedControls::DelayedControls(V4L2Device *device,\n-\t\t\t\t const std::unordered_map<uint32_t, unsigned int> &delays)\n+\t\t\t\t const std::unordered_map<uint32_t, ControlParams> &controlParams)\n \t: device_(device), maxDelay_(0)\n {\n \tconst ControlInfoMap &controls = device_->controls();\n@@ -57,12 +61,12 @@ DelayedControls::DelayedControls(V4L2Device *device,\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+\tfor (auto const ¶m : controlParams) {\n+\t\tauto it = controls.find(param.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<< utils::hex(param.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@@ -70,13 +74,14 @@ DelayedControls::DelayedControls(V4L2Device *device,\n \n \t\tconst ControlId *id = it->first;\n \n-\t\tdelays_[id] = delay.second;\n+\t\tcontrolParams_[id] = param.second;\n \n \t\tLOG(DelayedControls, Debug)\n-\t\t\t<< \"Set a delay of \" << delays_[id]\n+\t\t\t<< \"Set a delay of \" << controlParams_[id].delay\n+\t\t\t<< \" and priority write flag \" << controlParams_[id].priorityWrite\n \t\t\t<< \" for \" << id->name();\n \n-\t\tmaxDelay_ = std::max(maxDelay_, delays_[id]);\n+\t\tmaxDelay_ = std::max(maxDelay_, controlParams_[id].delay);\n \t}\n \n \treset();\n@@ -97,8 +102,8 @@ void DelayedControls::reset()\n \n \t/* Retrieve control as reported by the device. */\n \tstd::vector<uint32_t> ids;\n-\tfor (auto const &delay : delays_)\n-\t\tids.push_back(delay.first->id());\n+\tfor (auto const ¶m : controlParams_)\n+\t\tids.push_back(param.first->id());\n \n \tControlList controls = device_->getControls(ids);\n \n@@ -140,7 +145,7 @@ bool DelayedControls::push(const ControlList &controls)\n \n \t\tconst ControlId *id = it->second;\n \n-\t\tif (delays_.find(id) == delays_.end())\n+\t\tif (controlParams_.find(id) == controlParams_.end())\n \t\t\treturn false;\n \n \t\tInfo &info = values_[id][queueCount_];\n@@ -220,12 +225,27 @@ void DelayedControls::applyControls(uint32_t sequence)\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 delayDiff = maxDelay_ - controlParams_[id].delay;\n \t\tunsigned int index = std::max<int>(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);\n+\t\t\tif (controlParams_[id].priorityWrite) {\n+\t\t\t\t/*\n+\t\t\t\t * This control must be written now, it could\n+\t\t\t\t * affect validity of the other controls.\n+\t\t\t\t */\n+\t\t\t\tControlList priority(device_->controls());\n+\t\t\t\tpriority.set(id->id(), info);\n+\t\t\t\tdevice_->setControls(&priority);\n+\t\t\t} else {\n+\t\t\t\t/*\n+\t\t\t\t * Batch up the list of controls and write them\n+\t\t\t\t * at the end of the function.\n+\t\t\t\t */\n+\t\t\t\tout.set(id->id(), info);\n+\t\t\t}\n+\n \t\t\tLOG(DelayedControls, Debug)\n \t\t\t\t<< \"Setting \" << id->name()\n \t\t\t\t<< \" to \" << info.toString()\ndiff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\nindex 61f7bf43ea08..eda07d915f0c 100644\n--- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n@@ -988,14 +988,14 @@ int PipelineHandlerIPU3::registerCameras()\n \t\t * a sensor database. For now use generic values taken from\n \t\t * the Raspberry Pi and listed as 'generic values'.\n \t\t */\n-\t\tstd::unordered_map<uint32_t, unsigned int> delays = {\n-\t\t\t{ V4L2_CID_ANALOGUE_GAIN, 1 },\n-\t\t\t{ V4L2_CID_EXPOSURE, 2 },\n+\t\tstd::unordered_map<uint32_t, DelayedControls::ControlParams> params = {\n+\t\t\t{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n+\t\t\t{ V4L2_CID_EXPOSURE, { 2, false } },\n \t\t};\n \n \t\tdata->delayedCtrls_ =\n \t\t\tstd::make_unique<DelayedControls>(cio2->sensor()->device(),\n-\t\t\t\t\t\t\t delays);\n+\t\t\t\t\t\t\t params);\n \t\tdata->cio2_.frameStart().connect(data->delayedCtrls_.get(),\n \t\t\t\t\t\t &DelayedControls::applyControls);\n \ndiff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\nindex b0bb12ab0f37..bdb439021c46 100644\n--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n@@ -1273,16 +1273,15 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n \tif (result.operation & RPi::IPA_RESULT_SENSOR_PARAMS) {\n \t\t/*\n \t\t * Setup our delayed control writer with the sensor default\n-\t\t * gain and exposure delays.\n+\t\t * gain and exposure delays. Mark VBLANK for priority write.\n \t\t */\n-\t\tstd::unordered_map<uint32_t, unsigned int> delays = {\n-\t\t\t{ V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },\n-\t\t\t{ V4L2_CID_EXPOSURE, result.data[resultIdx++] },\n-\t\t\t{ V4L2_CID_VBLANK, result.data[resultIdx++] }\n+\t\tstd::unordered_map<uint32_t, DelayedControls::ControlParams> params = {\n+\t\t\t{ V4L2_CID_ANALOGUE_GAIN, { result.data[resultIdx++], false } },\n+\t\t\t{ V4L2_CID_EXPOSURE, { result.data[resultIdx++], false } },\n+\t\t\t{ V4L2_CID_VBLANK, { result.data[resultIdx++], true } }\n \t\t};\n \n-\t\tdelayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);\n-\n+\t\tdelayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), params);\n \t\tsensorMetadata_ = result.data[resultIdx++];\n \t}\n \ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\nindex e85979a719b7..ea9b1e2b9fcb 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n@@ -942,14 +942,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n \t * a sensor database. For now use generic values taken from\n \t * the Raspberry Pi and listed as generic values.\n \t */\n-\tstd::unordered_map<uint32_t, unsigned int> delays = {\n-\t\t{ V4L2_CID_ANALOGUE_GAIN, 1 },\n-\t\t{ V4L2_CID_EXPOSURE, 2 },\n+\tstd::unordered_map<uint32_t, DelayedControls::ControlParams> params = {\n+\t\t{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },\n+\t\t{ V4L2_CID_EXPOSURE, { 2, false } },\n \t};\n \n \tdata->delayedCtrls_ =\n \t\tstd::make_unique<DelayedControls>(data->sensor_->device(),\n-\t\t\t\t\t\t delays);\n+\t\t\t\t\t\t params);\n \tisp_->frameStart.connect(data->delayedCtrls_.get(),\n \t\t\t\t &DelayedControls::applyControls);\n \ndiff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp\nindex 50169b12e566..3855eb18ecd4 100644\n--- a/test/delayed_contols.cpp\n+++ b/test/delayed_contols.cpp\n@@ -72,8 +72,8 @@ protected:\n \n \tint singleControlNoDelay()\n \t{\n-\t\tstd::unordered_map<uint32_t, unsigned int> delays = {\n-\t\t\t{ V4L2_CID_BRIGHTNESS, 0 },\n+\t\tstd::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {\n+\t\t\t{ V4L2_CID_BRIGHTNESS, { 0, false } },\n \t\t};\n \t\tstd::unique_ptr<DelayedControls> delayed =\n \t\t\tstd::make_unique<DelayedControls>(dev_.get(), delays);\n@@ -109,8 +109,8 @@ protected:\n \n \tint singleControlWithDelay()\n \t{\n-\t\tstd::unordered_map<uint32_t, unsigned int> delays = {\n-\t\t\t{ V4L2_CID_BRIGHTNESS, 1 },\n+\t\tstd::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {\n+\t\t\t{ V4L2_CID_BRIGHTNESS, { 1, false } },\n \t\t};\n \t\tstd::unique_ptr<DelayedControls> delayed =\n \t\t\tstd::make_unique<DelayedControls>(dev_.get(), delays);\n@@ -150,9 +150,9 @@ protected:\n \n \tint dualControlsWithDelay(uint32_t startOffset)\n \t{\n-\t\tstd::unordered_map<uint32_t, unsigned int> delays = {\n-\t\t\t{ V4L2_CID_BRIGHTNESS, 1 },\n-\t\t\t{ V4L2_CID_CONTRAST, 2 },\n+\t\tstd::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {\n+\t\t\t{ V4L2_CID_BRIGHTNESS, { 1, false } },\n+\t\t\t{ V4L2_CID_CONTRAST, { 2, false } },\n \t\t};\n \t\tstd::unique_ptr<DelayedControls> delayed =\n \t\t\tstd::make_unique<DelayedControls>(dev_.get(), delays);\n@@ -197,9 +197,9 @@ protected:\n \n \tint dualControlsMultiQueue()\n \t{\n-\t\tstd::unordered_map<uint32_t, unsigned int> delays = {\n-\t\t\t{ V4L2_CID_BRIGHTNESS, 1 },\n-\t\t\t{ V4L2_CID_CONTRAST, 2 },\n+\t\tstd::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {\n+\t\t\t{ V4L2_CID_BRIGHTNESS, { 1, false } },\n+\t\t\t{ V4L2_CID_CONTRAST, { 2, false } }\n \t\t};\n \t\tstd::unique_ptr<DelayedControls> delayed =\n \t\t\tstd::make_unique<DelayedControls>(dev_.get(), delays);\n", "prefixes": [ "libcamera-devel", "1/5" ] }