[{"id":15177,"web_url":"https://patchwork.libcamera.org/comment/15177/","msgid":"","date":"2021-02-16T12:19:39","subject":"Re: [libcamera-devel] [PATCH v2 0/5] DelayedControls updates and\n\tfixes","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nDon't feel terribly well qualified to review all this, however, as\nI've been using it, for the whole set:\n\nTested-by: David Plowman \n\nThanks!\nDavid\n\nOn Tue, 16 Feb 2021 at 08:53, Naushir Patuck wrote:\n>\n> Hi,\n>\n> Here is a revised version of the DelayedControls patch. The only difference between v2 and v1 is in patch 4/5 where the fix was incomplete. To reiterate, here is a summary of the patches:\n>\n> Patch 1/5\n> This adds the notion of priority write to fixup the known issue of settings VBLANK and\n> EXPOSURE in a single batch. It is simply a port of the fix applied to StaggeredCtrl\n> that had been reviewed and subsequently deprecated, so hopefully nothing too controversial.\n>\n> Patch 2/5\n> This is simply a python script that I am using to debug the small problems (more details\n> below) that we have encountered. It parses the DelayedControls logs and nicely tabulates\n> the results to show what controls and values have been set/queued/get on each frame.\n> This has helped me tremendously in identifying problems and fixing them. However, this\n> may not be useful to others, so I am happy to not have this merged if folks do not think\n> it is the right place to put it.\n>\n> Patch 3/5\n> Fixes a spurious write to the device on startup. The following is an extract from using\n> the script to parse the logs:\n>\n> Frame Action Gain Exposure Vblank\n> 0 Write 0 [0] 52 [0] 531 [0] <<<<<\n> 0 Get 0 [0] 52 [0] 531 [0]\n> 0 Queue --- [-] --- [-] --- [-]\n> 1 Write 0 [0] --- [-] --- [-]\n> 1 Get 0 [0] 52 [0] 531 [0]\n> 1 Queue --- [-] --- [-] --- [-]\n> 2 Write --- [-] --- [-] --- [-]\n> 2 Get 0 [1] 52 [1] 531 [1]\n> 2 Queue 192 [4] 1664 [4] 531 [4]\n>\n> You can see above, on frame 0 we are writing controls to the sensor, but this is unneeded.\n> This spurious write should really not happen as there is no controls queued by the\n> pipeline_handler at this point. This problem is mostly inconsequential.\n>\n> Patch 4/5\n> This fixes an issue where controls queued by the pipeline handler are delayed by and\n> additional frame when writing. You can see better in the parsed log:\n>\n> Frame Action Gain Exposure Vblank\n> 2 Write --- [-] --- [-] --- [-]\n> 2 Get 0 [1] 52 [1] 531 [1]\n> 2 Queue 192 [4] 1664 [4] 531 [4] <<<<<\n> 3 Write --- [-] --- [-] --- [-] <<<<<\n> 3 Get 0 [2] 52 [2] 531 [2]\n> 3 Queue 192 [5] 1664 [5] 531 [5]\n> 4 Write --- [-] 1664 [4] 531 [4] <<<<<\n> 4 Get 0 [3] 52 [3] 531 [3]\n> 4 Queue 192 [6] 1664 [6] 531 [6]\n>\n> On frame 2, we queue controls from the pipeline handler. Exposure and Vblank must be\n> written one frame before gain, so you would expect them to be written on frame 3 as\n> nothing else is in the queue. However, they only get written on frame 4, one frame\n> later than expected.\n>\n> This is because of how DelayedControls handles \"no-op\" queue items, i.e. frames where\n> we do not provide the helper with controls to use. It was adding one more no-op than\n> needed, and causing an extra frame delay when setting the control on the device.\n>\n> As a consequence of this change, we must also ensure that controls that have been sent\n> to the device must have the update flag cleared or there is a change it may be re-used\n> when going around the ring buffer.\n>\n> Patch 5/5\n> We had an off-by-one error when reading back values from the queues. See the parsed\n> logs below:\n>\n> Frame Action Gain Exposure Vblank\n> 7 Write 192 [6] 1664 [7] 531 [7] <<<<<\n> 7 Get 192 [6] 1664 [6] 531 [6]\n> 7 Queue 210 [9] 3174 [9] 1946 [9]\n> 8 Write 192 [7] 3526 [8] 2298 [8]\n> 8 Get 192 [7] 1664 [7] 531 [7] <<<<<\n> 8 Queue 210 [10] 3174 [10] 1946 [10]\n> 9 Write 213 [8] 3174 [9] 1946 [9]\n> 9 Get 213 [8] 3526 [8] 2298 [8] <<<<<\n> 9 Queue 213 [12] 3526 [12] 2298 [12]\n>\n> On frame 7, we write exposure and vblank with values 1664 and 531 respectively. These\n> values take 2 frames to consume, so should be retuned to the pipeline handler by the\n> DelayedControls::get() on frame 9. However, it returns on frame 8 instead.\n>\n> This only causes slight (but visible) oscillations in brightness in the AGC loop as\n> the values used by the sensor are not in lock-step to what is reported by DelayedControls::get().\n>\n> Naushir Patuck (5):\n> libcamera: delayed_controls: Add notion of priority write\n> utils: raspberrypi: Add a DelayedControls log parser\n> libcamera: delayed_controls: Remove unneeded write when starting up\n> libcamera: delayed_controls: Remove spurious no-op queued controls\n> libcamera: delayed_controls: Fix off-by-one error in get()\n>\n> include/libcamera/internal/delayed_controls.h | 13 ++-\n> src/libcamera/delayed_controls.cpp | 79 ++++++++++-----\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> utils/raspberrypi/delayedctrls_parse.py | 98 +++++++++++++++++++\n> 7 files changed, 183 insertions(+), 56 deletions(-)\n> create mode 100644 utils/raspberrypi/delayedctrls_parse.py\n>\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 97ADBBD160\n\tfor ;\n\tTue, 16 Feb 2021 12:19:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16007637DD;\n\tTue, 16 Feb 2021 13:19:54 +0100 (CET)","from mail-oi1-x22a.google.com (mail-oi1-x22a.google.com\n\t[IPv6:2607:f8b0:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B568160D37\n\tfor ;\n\tTue, 16 Feb 2021 13:19:52 +0100 (CET)","by mail-oi1-x22a.google.com with SMTP id r75so10969403oie.11\n\tfor ;\n\tTue, 16 Feb 2021 04:19:52 -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=\"bSqKQ9Cp\"; 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=Wza0PNFhAgt9vBm/VrH6/YIzAZO5AxMObmUZOL2aqig=;\n\tb=bSqKQ9CpKNMWNwvCs4m5wnxl4GYbrtljZZLDV4QajqTBDFjx0hDOv6HbpOK/4oxI4r\n\tVoQzBTq40XEAiae2KFYO/mmXJfJq3PdH75gwhPs93VSSMbQolN3pub8AvNbV8fwgx3q3\n\trAbV8ccAqoIfk04puEFzoBHtL5y4wCRkuacZE96poAoeIAfMxdWwfta6G0q9K60KVV+p\n\tw8plSxsGMiRU8M1Jz4D0S/vSwtUIivWN3k6ccCyrDpJ1G3SxS8aSxfL5Hme1TQktEFxb\n\tIhvkN/RfeGcCGuAnPyxxSyazT/iEn52fvpvF/fD+F79Neh17aJeN2IsKe1tCBUZeedeY\n\tUA0A==","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=Wza0PNFhAgt9vBm/VrH6/YIzAZO5AxMObmUZOL2aqig=;\n\tb=pFppzTauQlueSiDgeH5qWTTfBA7KWE8I9TNvtPsX5u3LicUPQia200uLNWosLVF1Fw\n\txNQ8ySpxH1K+Ceoi0JU/KYeOQ4NLt7TiIn/uHJkLDP010+mFt4X9zxuDLMwlklltNwZG\n\t6zF2JphGDbGTge63YGlO28c1tLzYt0Sp68GoG7v/qoqDjfEaY1JzE5mEgGXCJ2R5iDZv\n\toZ5RonYnUUgNxr1JHhraKjT6dA75/DOXlfT1Bf1psrDskZB2w9v+mZjEPrmHS6+vRIpp\n\t8hZFehMTJ6xal1GxecUkq8HtCoaYiMH4ZGiiAYZaNEamxit9lZAk5x8w/HD+vB7UqJZU\n\tNrww==","X-Gm-Message-State":"AOAM531wTdoU5XzWxuRmOuEJTKza55yz9/ncw/rnhMrSvEUvdR7phvkN\n\tXF/yV98ctoVVQ06x4vBkDsn956TFl8VxdQMLV0v6Rg==","X-Google-Smtp-Source":"ABdhPJxoyn7rTDtWducric5jTQVNajq8E6ypeFu2MFNmGfEFkLywKFNVc5n6NvSM69vhfxcYjOdzG35r1sojrCv114c=","X-Received":"by 2002:a05:6808:608:: with SMTP id\n\ty8mr2398920oih.55.1613477991324; \n\tTue, 16 Feb 2021 04:19:51 -0800 (PST)","MIME-Version":"1.0","References":"<20210216085342.1012717-1-naush@raspberrypi.com>","In-Reply-To":"<20210216085342.1012717-1-naush@raspberrypi.com>","From":"David Plowman ","Date":"Tue, 16 Feb 2021 12:19:39 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH v2 0/5] DelayedControls updates and\n\tfixes","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15313,"web_url":"https://patchwork.libcamera.org/comment/15313/","msgid":"","date":"2021-02-25T10:06:21","subject":"Re: [libcamera-devel] [PATCH v2 0/5] DelayedControls updates and\n\tfixes","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\nGentle ping, would appreciate any feedback for these changes.\n\nRegards,\nNaush\n\nOn Tue, 16 Feb 2021 at 08:53, Naushir Patuck wrote:\n\n> Hi,\n>\n> Here is a revised version of the DelayedControls patch. The only\n> difference between v2 and v1 is in patch 4/5 where the fix was incomplete.\n> To reiterate, here is a summary of the patches:\n>\n> Patch 1/5\n> This adds the notion of priority write to fixup the known issue of\n> settings VBLANK and\n> EXPOSURE in a single batch. It is simply a port of the fix applied to\n> StaggeredCtrl\n> that had been reviewed and subsequently deprecated, so hopefully nothing\n> too controversial.\n>\n> Patch 2/5\n> This is simply a python script that I am using to debug the small problems\n> (more details\n> below) that we have encountered. It parses the DelayedControls logs and\n> nicely tabulates\n> the results to show what controls and values have been set/queued/get on\n> each frame.\n> This has helped me tremendously in identifying problems and fixing them.\n> However, this\n> may not be useful to others, so I am happy to not have this merged if\n> folks do not think\n> it is the right place to put it.\n>\n> Patch 3/5\n> Fixes a spurious write to the device on startup. The following is an\n> extract from using\n> the script to parse the logs:\n>\n> Frame Action Gain Exposure Vblank\n> 0 Write 0 [0] 52 [0] 531 [0] <<<<<\n> 0 Get 0 [0] 52 [0] 531 [0]\n> 0 Queue --- [-] --- [-] --- [-]\n> 1 Write 0 [0] --- [-] --- [-]\n> 1 Get 0 [0] 52 [0] 531 [0]\n> 1 Queue --- [-] --- [-] --- [-]\n> 2 Write --- [-] --- [-] --- [-]\n> 2 Get 0 [1] 52 [1] 531 [1]\n> 2 Queue 192 [4] 1664 [4] 531 [4]\n>\n> You can see above, on frame 0 we are writing controls to the sensor, but\n> this is unneeded.\n> This spurious write should really not happen as there is no controls\n> queued by the\n> pipeline_handler at this point. This problem is mostly inconsequential.\n>\n> Patch 4/5\n> This fixes an issue where controls queued by the pipeline handler are\n> delayed by and\n> additional frame when writing. You can see better in the parsed log:\n>\n> Frame Action Gain Exposure Vblank\n> 2 Write --- [-] --- [-] --- [-]\n> 2 Get 0 [1] 52 [1] 531 [1]\n> 2 Queue 192 [4] 1664 [4] 531 [4] <<<<<\n> 3 Write --- [-] --- [-] --- [-] <<<<<\n> 3 Get 0 [2] 52 [2] 531 [2]\n> 3 Queue 192 [5] 1664 [5] 531 [5]\n> 4 Write --- [-] 1664 [4] 531 [4] <<<<<\n> 4 Get 0 [3] 52 [3] 531 [3]\n> 4 Queue 192 [6] 1664 [6] 531 [6]\n>\n> On frame 2, we queue controls from the pipeline handler. Exposure and\n> Vblank must be\n> written one frame before gain, so you would expect them to be written on\n> frame 3 as\n> nothing else is in the queue. However, they only get written on frame 4,\n> one frame\n> later than expected.\n>\n> This is because of how DelayedControls handles \"no-op\" queue items, i.e.\n> frames where\n> we do not provide the helper with controls to use. It was adding one more\n> no-op than\n> needed, and causing an extra frame delay when setting the control on the\n> device.\n>\n> As a consequence of this change, we must also ensure that controls that\n> have been sent\n> to the device must have the update flag cleared or there is a change it\n> may be re-used\n> when going around the ring buffer.\n>\n> Patch 5/5\n> We had an off-by-one error when reading back values from the queues. See\n> the parsed\n> logs below:\n>\n> Frame Action Gain Exposure Vblank\n> 7 Write 192 [6] 1664 [7] 531 [7] <<<<<\n> 7 Get 192 [6] 1664 [6] 531 [6]\n> 7 Queue 210 [9] 3174 [9] 1946 [9]\n> 8 Write 192 [7] 3526 [8] 2298 [8]\n> 8 Get 192 [7] 1664 [7] 531 [7] <<<<<\n> 8 Queue 210 [10] 3174 [10] 1946 [10]\n> 9 Write 213 [8] 3174 [9] 1946 [9]\n> 9 Get 213 [8] 3526 [8] 2298 [8] <<<<<\n> 9 Queue 213 [12] 3526 [12] 2298 [12]\n>\n> On frame 7, we write exposure and vblank with values 1664 and 531\n> respectively. These\n> values take 2 frames to consume, so should be retuned to the pipeline\n> handler by the\n> DelayedControls::get() on frame 9. However, it returns on frame 8 instead.\n>\n> This only causes slight (but visible) oscillations in brightness in the\n> AGC loop as\n> the values used by the sensor are not in lock-step to what is reported by\n> DelayedControls::get().\n>\n> Naushir Patuck (5):\n> libcamera: delayed_controls: Add notion of priority write\n> utils: raspberrypi: Add a DelayedControls log parser\n> libcamera: delayed_controls: Remove unneeded write when starting up\n> libcamera: delayed_controls: Remove spurious no-op queued controls\n> libcamera: delayed_controls: Fix off-by-one error in get()\n>\n> include/libcamera/internal/delayed_controls.h | 13 ++-\n> src/libcamera/delayed_controls.cpp | 79 ++++++++++-----\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> utils/raspberrypi/delayedctrls_parse.py | 98 +++++++++++++++++++\n> 7 files changed, 183 insertions(+), 56 deletions(-)\n> create mode 100644 utils/raspberrypi/delayedctrls_parse.py\n>\n> --\n> 2.25.1\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 8431FBD808\n\tfor ;\n\tThu, 25 Feb 2021 10:06:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D292668A43;\n\tThu, 25 Feb 2021 11:06:40 +0100 (CET)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 298FF60106\n\tfor ;\n\tThu, 25 Feb 2021 11:06:39 +0100 (CET)","by mail-lj1-x234.google.com with SMTP id y7so5772563lji.7\n\tfor ;\n\tThu, 25 Feb 2021 02:06:39 -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=\"Bpsi3/1x\"; 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\tbh=F1aHmcEna1Vuto/9KDxLWvFlRAt1NZ+8ctgKg5YLAAg=;\n\tb=Bpsi3/1xi0h7nO/z9uDA6ZQjtUhyu8ZLR2QV/7MMu/UXkZvZhgaolxnM+TlE/mUIu+\n\tgEXVqRHxZbG9cyteX0Ldeaw+1xSGK8qkZg5Yzfs9sUQaNv0SssJFgLpH1K+eQjBlRSUw\n\tZm2tlW2FrDqi0A6MIei7DwRSya5TT6nUYbLNky2+3vKxNcXfLtupVrsz09IiKFXEQE1k\n\td55ttV1jy+rlo7auKWIdn6eU8xYRffxyonqbkybwvlPIpRJTbksiRm8IDYxHxcVHL+Lu\n\tB+d9uPig3+sQo6U4NKCaShk97eRIICsL7GxzfgGsXZGyMHciiuug/y4zG1/ALjmnHgfn\n\tw3Ww==","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;\n\tbh=F1aHmcEna1Vuto/9KDxLWvFlRAt1NZ+8ctgKg5YLAAg=;\n\tb=bhfuiN22OsGgvH44RkbHKaP10B+rclW7rCyl2zXcF3WGVnvz8xzAppwD+TStJpNJw0\n\tsSTMfa8iik3jzbbgBMlJVBLVKVY8Oiv0hU/VZKBPJrxlpMb/XH3y7gLac8wSHcrptcXm\n\tElqsi41rDgKT+YiIKTbzjl0/+wWasJA5fubDqgo87hs+IOhPRjzpu1wo/4XbAGaiBLkI\n\tAnnN25gGv3lHij8PhsVUehlgHwuydhaAGmBOhX5MWHAz6IZvTl5P8FdqJYn9Xn0dsxyw\n\tvI2A+RNTpD6QCLC/9HyUGJFVTM1lPCBpfUdPfjLMtmJASYTGZkNwTU/TszGiGqZb9s0d\n\ty6Rw==","X-Gm-Message-State":"AOAM531y/Zpv0klB635EkBSl2MtJl9ODWTa6HYWzp4uTQFJEfVdY4gQp\n\tzshvPyjzNxDKkvdEB9b+j5zsVOhWYclF8FHqUlAVbSr/UF7e0Q==","X-Google-Smtp-Source":"ABdhPJw3ZkxkBBT9y8cPvzUuzYdqxzLHi3O6rs6jiusboRZ2LpiyI2PrUl1FgyZxqlu/gHGyxj+vP375T3+MX6yYhn4=","X-Received":"by 2002:a05:651c:2108:: with SMTP id\n\ta8mr1172791ljq.329.1614247597868; \n\tThu, 25 Feb 2021 02:06:37 -0800 (PST)","MIME-Version":"1.0","References":"<20210216085342.1012717-1-naush@raspberrypi.com>","In-Reply-To":"<20210216085342.1012717-1-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Thu, 25 Feb 2021 10:06:21 +0000","Message-ID":"","To":"libcamera devel ","Subject":"Re: [libcamera-devel] [PATCH v2 0/5] DelayedControls updates and\n\tfixes","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","Content-Type":"multipart/mixed;\n\tboundary=\"===============3430187277298921187==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]