[{"id":16464,"web_url":"https://patchwork.libcamera.org/comment/16464/","msgid":"","date":"2021-04-21T18:37:18","subject":"Re: [libcamera-devel] [PATCH v3 05/16] libcamera: ipu3: Report\n\tsensor timestamp","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2021-04-21 18:03:08 +0200, Jacopo Mondi wrote:\n> Report the sensor's timestamp in the Request metadata by using the\n> CIO2 buffer timestamp as an initial approximation.\n> \n> The buffer's timestamp is recorded at DMA-transfer time, and it does not\n> theoretically matches the 'start of exposure' definition, but when used\n> to compare two consecutive frames it gives an acceptable estimation of\n> the sensor frame period duration.\n> \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Hirokazu Honda \n> Reviewed-by: Laurent Pinchart \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 51446fcf5bc1..28e849a43a3e 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1255,6 +1255,15 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> \n> \tRequest *request = info->request;\n> \n> +\t/*\n> +\t * Record the sensor's timestamp in the request metadata.\n> +\t *\n> +\t * \\todo The sensor timestamp should be better estimated by connecting\n> +\t * to the V4L2Device::frameStart signal.\n> +\t */\n\nI'm OK with this patch as is, but just in case you missed it. The signal \nis already wired to CIO2Device::frameStart() is and used to feed the \nDelayedControls state machine. So all the building blocks for this todo \nI think is already in place,\n\n void PipelineHandlerIPU3::stamp(uint32_t sequence)\n {\n IPU3Frames::Info *info = frameInfos_.find(buffer);\n\n ...\n\n info->request->metadata().set(controls::SensorTimestamp, ...);\n }\n\n int PipelineHandlerIPU3::registerCameras()\n {\n ....\n\n data->cio2_.frameStart().connect(this, &PipelineHandlerIPU3::stamp);\n\n ....\n }\n\nThat being said I think we might get better values using the buffer \ntimestamp as done in this patch, if nothing else less jitter. Only draw \nback I can think of is that we can't grantee the timestamp coming from \nthe kernel relates to BOOTTIME.\n\n> +\trequest->metadata().set(controls::SensorTimestamp,\n> +\t\t\t\tbuffer->metadata().timestamp);\n> +\n> \t/* If the buffer is cancelled force a complete of the whole request. */\n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> \t\tfor (auto it : request->buffers())\n> -- \n> 2.31.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 085EBBDB17\n\tfor ;\n\tWed, 21 Apr 2021 18:37:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8639768846;\n\tWed, 21 Apr 2021 20:37:22 +0200 (CEST)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67EFA68840\n\tfor ;\n\tWed, 21 Apr 2021 20:37:20 +0200 (CEST)","by mail-lf1-x12e.google.com with SMTP id x19so37892806lfa.2\n\tfor ;\n\tWed, 21 Apr 2021 11:37:20 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id z9sm36342lfd.2.2021.04.21.11.37.19\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 21 Apr 2021 11:37:19 -0700 (PDT)"],"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=\"PkUSfB5j\"; 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=JDIdAYCZYPDdLqfK4C04/xlMbIUF8eTGiwvF6LPGVPI=;\n\tb=PkUSfB5jDgMVOKKuvPjwaymn5P/RbIn48HQbVBiWsw79+UpmyZqAEcoJmbDp5RIRUS\n\to9aZZSaNFL4X2G42qfnNqse6dR8+kaPJ9B/oZqrlEg/cj2WaKiEE/8nB8BuJUtDp5/uK\n\tHosoZBvKLXSFN2/vKmetHfIN4MpbCp0PeigwwAzr70aJvEJlB/pxTX6IctthP6TVPuE/\n\tgkO9ZoDRKebKZLh9pPpQrcWYzHLSkjEP2vrn/4Q1NcN0sxUdBlOg1Xe3c/IWKxo1Huv1\n\tfNAjAgpmtEPVQJqMpH19o0Hv/yjemMqy3ChtvUZntyl9oN4ywsQ4z7YDmCxfplXEqC8q\n\tovRg==","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=JDIdAYCZYPDdLqfK4C04/xlMbIUF8eTGiwvF6LPGVPI=;\n\tb=J3q2oP16oWnlvKuMtobZAPvVwvEwpN8cX2e41WwlrveK73Vyh/YBphBy5D36faRTPo\n\tun/RyrggHCYjQB4ZpZmloeuuvWWczpQybQF94vtds2cUs1RPOd5RKpoTJlCq54AjBnjc\n\tLFRUfTN8B93MgOrutTvnDWWFBEEH0H90CtTN6fjj/hJjeWEws/XNFe0ikRZ9RydfAGec\n\txYMTGFXPT8JeA4EHRxjTCYbv6SfVy3MvihlV8Qad8YCghdq/+Apv9q1OoPwpYYAl3gPg\n\tN2e9k181AQ2jCgFoqC/8RFMPoZhXo4j0XUH+D1n21IhGWLviwhpBupPWnVi7BIWi1NpR\n\tf4eg==","X-Gm-Message-State":"AOAM533OnjH5Eb1kzg/09z9OK2XMCOgO5k9BoShqVrAIf0YNSZWTGWIY\n\tEbxyzDASHWtjSSeIQJaP04jYKQ==","X-Google-Smtp-Source":"ABdhPJx5GvSIeut6i7czI8IOFJtr3Tg35J/cN9NITDWu6MWAaZfnhdgrl6ZD8HlN9erRI4azO5xv9g==","X-Received":"by 2002:ac2:5f51:: with SMTP id\n\t17mr11602459lfz.431.1619030239801; \n\tWed, 21 Apr 2021 11:37:19 -0700 (PDT)","Date":"Wed, 21 Apr 2021 20:37:18 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210421160319.42251-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 05/16] libcamera: ipu3: Report\n\tsensor timestamp","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":16490,"web_url":"https://patchwork.libcamera.org/comment/16490/","msgid":"<20210422062503.yqkpwqt6if3en3cs@uno.localdomain>","date":"2021-04-22T06:25:03","subject":"Re: [libcamera-devel] [PATCH v3 05/16] libcamera: ipu3: Report\n\tsensor timestamp","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Apr 21, 2021 at 08:37:18PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2021-04-21 18:03:08 +0200, Jacopo Mondi wrote:\n> > Report the sensor's timestamp in the Request metadata by using the\n> > CIO2 buffer timestamp as an initial approximation.\n> >\n> > The buffer's timestamp is recorded at DMA-transfer time, and it does not\n> > theoretically matches the 'start of exposure' definition, but when used\n> > to compare two consecutive frames it gives an acceptable estimation of\n> > the sensor frame period duration.\n> >\n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Hirokazu Honda \n> > Reviewed-by: Laurent Pinchart \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++\n> > 1 file changed, 9 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 51446fcf5bc1..28e849a43a3e 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1255,6 +1255,15 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> >\n> > \tRequest *request = info->request;\n> >\n> > +\t/*\n> > +\t * Record the sensor's timestamp in the request metadata.\n> > +\t *\n> > +\t * \\todo The sensor timestamp should be better estimated by connecting\n> > +\t * to the V4L2Device::frameStart signal.\n> > +\t */\n>\n> I'm OK with this patch as is, but just in case you missed it. The signal\n> is already wired to CIO2Device::frameStart() is and used to feed the\n> DelayedControls state machine. So all the building blocks for this todo\n> I think is already in place,\n>\n> void PipelineHandlerIPU3::stamp(uint32_t sequence)\n> {\n> IPU3Frames::Info *info = frameInfos_.find(buffer);\n>\n> ...\n>\n> info->request->metadata().set(controls::SensorTimestamp, ...);\n> }\n>\n> int PipelineHandlerIPU3::registerCameras()\n> {\n> ....\n>\n> data->cio2_.frameStart().connect(this, &PipelineHandlerIPU3::stamp);\n>\n> ....\n> }\n\nI've seen that, but plumbing in the infrastructure for timestamping in\nthe library was considered not necessary at this time\n\n>\n> That being said I think we might get better values using the buffer\n> timestamp as done in this patch, if nothing else less jitter. Only draw\n> back I can think of is that we can't grantee the timestamp coming from\n> the kernel relates to BOOTTIME.\n>\n\nI keep hitting walls all over the places....\nTimestamping is performed in the cio2 receiver driver with:\n\n ns = ktime_get_ns()\n\nktime_get() documentation reads as:\n\n /*\n * ktime_get() family: read the current time in a multitude of ways,\n *\n * The default time reference is CLOCK_MONOTONIC, starting at\n * boot time but not counting the time spent in suspend.\n * For other references, use the functions with \"real\", \"clocktai\",\n * \"boottime\" and \"raw\" suffixes.\n *\n * To get the time in a different format, use the ones wit\n * \"ns\", \"ts64\" and \"seconds\" suffix.\n *\n * See Documentation/core-api/timekeeping.rst for more details.\n */\n\nI recall I repeated multiple times the reference clock was BOOTTIME,\nnot sure because I overlooked it or because at the time (last week...)\nI had no real understanding of the differences between MONOTONIC and\nBOOTTIME...\n\nShould we change the definition of the control again to support\nmultiple time sources, and add one property to report which reference\nis used ?\n\n> > +\trequest->metadata().set(controls::SensorTimestamp,\n> > +\t\t\t\tbuffer->metadata().timestamp);\n> > +\n> > \t/* If the buffer is cancelled force a complete of the whole request. */\n> > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > \t\tfor (auto it : request->buffers())\n> > --\n> > 2.31.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 44A78BDB15\n\tfor ;\n\tThu, 22 Apr 2021 06:24:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5955368852;\n\tThu, 22 Apr 2021 08:24:24 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1488A6884B\n\tfor ;\n\tThu, 22 Apr 2021 08:24:23 +0200 (CEST)","from uno.localdomain (host-82-57-193-192.retail.telecomitalia.it\n\t[82.57.193.192]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 4C0C5E0004;\n\tThu, 22 Apr 2021 06:24:21 +0000 (UTC)"],"X-Originating-IP":"82.57.193.192","Date":"Thu, 22 Apr 2021 08:25:03 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210422062503.yqkpwqt6if3en3cs@uno.localdomain>","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-6-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 05/16] libcamera: ipu3: Report\n\tsensor timestamp","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":16492,"web_url":"https://patchwork.libcamera.org/comment/16492/","msgid":"","date":"2021-04-22T06:55:01","subject":"Re: [libcamera-devel] [PATCH v3 05/16] libcamera: ipu3: Report\n\tsensor timestamp","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2021-04-22 08:25:03 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Apr 21, 2021 at 08:37:18PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > On 2021-04-21 18:03:08 +0200, Jacopo Mondi wrote:\n> > > Report the sensor's timestamp in the Request metadata by using the\n> > > CIO2 buffer timestamp as an initial approximation.\n> > >\n> > > The buffer's timestamp is recorded at DMA-transfer time, and it does not\n> > > theoretically matches the 'start of exposure' definition, but when used\n> > > to compare two consecutive frames it gives an acceptable estimation of\n> > > the sensor frame period duration.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > > Reviewed-by: Hirokazu Honda \n> > > Reviewed-by: Laurent Pinchart \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++\n> > > 1 file changed, 9 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 51446fcf5bc1..28e849a43a3e 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -1255,6 +1255,15 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > >\n> > > \tRequest *request = info->request;\n> > >\n> > > +\t/*\n> > > +\t * Record the sensor's timestamp in the request metadata.\n> > > +\t *\n> > > +\t * \\todo The sensor timestamp should be better estimated by connecting\n> > > +\t * to the V4L2Device::frameStart signal.\n> > > +\t */\n> >\n> > I'm OK with this patch as is, but just in case you missed it. The signal\n> > is already wired to CIO2Device::frameStart() is and used to feed the\n> > DelayedControls state machine. So all the building blocks for this todo\n> > I think is already in place,\n> >\n> > void PipelineHandlerIPU3::stamp(uint32_t sequence)\n> > {\n> > IPU3Frames::Info *info = frameInfos_.find(buffer);\n> >\n> > ...\n> >\n> > info->request->metadata().set(controls::SensorTimestamp, ...);\n> > }\n> >\n> > int PipelineHandlerIPU3::registerCameras()\n> > {\n> > ....\n> >\n> > data->cio2_.frameStart().connect(this, &PipelineHandlerIPU3::stamp);\n> >\n> > ....\n> > }\n> \n> I've seen that, but plumbing in the infrastructure for timestamping in\n> the library was considered not necessary at this time\n> \n> >\n> > That being said I think we might get better values using the buffer\n> > timestamp as done in this patch, if nothing else less jitter. Only draw\n> > back I can think of is that we can't grantee the timestamp coming from\n> > the kernel relates to BOOTTIME.\n> >\n> \n> I keep hitting walls all over the places....\n> Timestamping is performed in the cio2 receiver driver with:\n> \n> ns = ktime_get_ns()\n> \n> ktime_get() documentation reads as:\n> \n> /*\n> * ktime_get() family: read the current time in a multitude of ways,\n> *\n> * The default time reference is CLOCK_MONOTONIC, starting at\n> * boot time but not counting the time spent in suspend.\n> * For other references, use the functions with \"real\", \"clocktai\",\n> * \"boottime\" and \"raw\" suffixes.\n> *\n> * To get the time in a different format, use the ones wit\n> * \"ns\", \"ts64\" and \"seconds\" suffix.\n> *\n> * See Documentation/core-api/timekeeping.rst for more details.\n> */\n> \n> I recall I repeated multiple times the reference clock was BOOTTIME,\n> not sure because I overlooked it or because at the time (last week...)\n> I had no real understanding of the differences between MONOTONIC and\n> BOOTTIME...\n> \n> Should we change the definition of the control again to support\n> multiple time sources, and add one property to report which reference\n> is used ?\n\nI'm a bit slow and I still don't understand the need for users to know \nwhat the reference clock is used :-) Is it not enough that it have a ts \nit can diff between two exposers and get a usable delta in a known time \nunit? And of course that it's not subjected to the \"fun\" of NTP clock \nadjusting mechanisms.\n\nI don't want to distract for the real work in this patch and as I said \nI'm fine with the patch as is, only wanted to point out that the todo \nwas ready to be acted on. And that IMHO I think we would gain more from \nthe solution without the todo addressed, or of course change the SOE \nsignal API to include a ts from the kernel set at the IRQ. My fear being \nthat if we produce the ts in user-space it will be subjected to jitter \nthat will be worse then using the buffer ts as done here.\n\n> \n> > > +\trequest->metadata().set(controls::SensorTimestamp,\n> > > +\t\t\t\tbuffer->metadata().timestamp);\n> > > +\n> > > \t/* If the buffer is cancelled force a complete of the whole request. */\n> > > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > \t\tfor (auto it : request->buffers())\n> > > --\n> > > 2.31.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","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 AA83CBDB15\n\tfor ;\n\tThu, 22 Apr 2021 06:55:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 12FCC68850;\n\tThu, 22 Apr 2021 08:55:05 +0200 (CEST)","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 7288368847\n\tfor ;\n\tThu, 22 Apr 2021 08:55:03 +0200 (CEST)","by mail-lf1-x12d.google.com with SMTP id j4so30999827lfp.0\n\tfor ;\n\tWed, 21 Apr 2021 23:55:03 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tp11sm173648lfs.306.2021.04.21.23.55.02\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 21 Apr 2021 23:55:02 -0700 (PDT)"],"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=\"vd/SzOik\"; 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=OrH2Ezf7RMyN9yefzXJvS2y8/hl7O/Fc5g7/4aKMuPg=;\n\tb=vd/SzOikbXGTjhMGMiNrqywhllqat4zFHBQJyBZiha6JGWvQ507qrZniejAdGlZzip\n\twVSIv1UWS6BC3cWuz5MWWbngq3VZIPPbNNA++JTknrWRGfKBGD2PkX+2IO2F1JOrfgY6\n\tsVWVx7ikhsXWj9DTbaPpauPt/wav6gAMdVpqFjWkjePxdfegJO0yQTH30m3ip5xDa6bj\n\t//VMeTuQOjYBqOhrF7TncVGHC3UZRWQh6sOzqd4ltdMe4PUhAEU5ug/gNuIWp9p/oy/Z\n\t1vIJp4vwDaA2NeXjvf5r6QWJDJcDaWh1zhdNStu+opta5bCjtcXAlsx5X3N0y2VaIkKn\n\tCMkQ==","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=OrH2Ezf7RMyN9yefzXJvS2y8/hl7O/Fc5g7/4aKMuPg=;\n\tb=qeLg9g+2Od3aMwCZKxOhTRtmA1G06P7p48TGV/68D18fzpJBF9hEOgbIqv3cAS2OVh\n\t/6DL5aqOwrVci2o5Dk3IdiXT/UqjdfIOJbBiGpQbI5WNX9qafjnyb2Oo1DrePKAKYEaF\n\tUPOaURQrhIMJxFA2vQcyAKHeAbaxCxxxMMfIU7VERHmSDUkhe7weGnVZXSaVSzthX8a/\n\trF7C5+Wu6VfVmEb3wItu+FB6GrDh0qXEMt6jYiT4Ay/70l22xkRIvQFGju5b5kBrjSyD\n\tl3NxennqEjg4vUnpm7JZHwXVQeObuKjzby1+ciBF0DrF/+QypYG3jnS8+xWkJRoSda1W\n\tDwjg==","X-Gm-Message-State":"AOAM530vT/O1c9mZDUq77SJXgwS41MXPcdU60dv6gEM8EQDC0CTev7JU\n\txuf78iL8ESaZ0DYqgw5ftR3sS8jBU3726rg4","X-Google-Smtp-Source":"ABdhPJzbQNu+POsox2HQvFz+XJy3DmfowtKAWf7K79tKU3h6Rvv6pcewDWPCiOXlBqRNtqj/I3MP2w==","X-Received":"by 2002:a05:6512:2088:: with SMTP id\n\tt8mr1389142lfr.55.1619074502809; \n\tWed, 21 Apr 2021 23:55:02 -0700 (PDT)","Date":"Thu, 22 Apr 2021 08:55:01 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-6-jacopo@jmondi.org>\n\t\n\t<20210422062503.yqkpwqt6if3en3cs@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210422062503.yqkpwqt6if3en3cs@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 05/16] libcamera: ipu3: Report\n\tsensor timestamp","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":16558,"web_url":"https://patchwork.libcamera.org/comment/16558/","msgid":"","date":"2021-04-26T04:30:05","subject":"Re: [libcamera-devel] [PATCH v3 05/16] libcamera: ipu3: Report\n\tsensor timestamp","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas and Jacopo,\n\nOn Thu, Apr 22, 2021 at 08:55:01AM +0200, Niklas Söderlund wrote:\n> On 2021-04-22 08:25:03 +0200, Jacopo Mondi wrote:\n> > On Wed, Apr 21, 2021 at 08:37:18PM +0200, Niklas Söderlund wrote:\n> > > On 2021-04-21 18:03:08 +0200, Jacopo Mondi wrote:\n> > > > Report the sensor's timestamp in the Request metadata by using the\n> > > > CIO2 buffer timestamp as an initial approximation.\n> > > >\n> > > > The buffer's timestamp is recorded at DMA-transfer time, and it does not\n> > > > theoretically matches the 'start of exposure' definition, but when used\n> > > > to compare two consecutive frames it gives an acceptable estimation of\n> > > > the sensor frame period duration.\n> > > >\n> > > > Reviewed-by: Kieran Bingham \n> > > > Reviewed-by: Hirokazu Honda \n> > > > Reviewed-by: Laurent Pinchart \n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++\n> > > > 1 file changed, 9 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 51446fcf5bc1..28e849a43a3e 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -1255,6 +1255,15 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > >\n> > > > \tRequest *request = info->request;\n> > > >\n> > > > +\t/*\n> > > > +\t * Record the sensor's timestamp in the request metadata.\n> > > > +\t *\n> > > > +\t * \\todo The sensor timestamp should be better estimated by connecting\n> > > > +\t * to the V4L2Device::frameStart signal.\n> > > > +\t */\n> > >\n> > > I'm OK with this patch as is, but just in case you missed it. The signal\n> > > is already wired to CIO2Device::frameStart() is and used to feed the\n> > > DelayedControls state machine. So all the building blocks for this todo\n> > > I think is already in place,\n> > >\n> > > void PipelineHandlerIPU3::stamp(uint32_t sequence)\n> > > {\n> > > IPU3Frames::Info *info = frameInfos_.find(buffer);\n> > >\n> > > ...\n> > >\n> > > info->request->metadata().set(controls::SensorTimestamp, ...);\n> > > }\n> > >\n> > > int PipelineHandlerIPU3::registerCameras()\n> > > {\n> > > ....\n> > >\n> > > data->cio2_.frameStart().connect(this, &PipelineHandlerIPU3::stamp);\n> > >\n> > > ....\n> > > }\n> > \n> > I've seen that, but plumbing in the infrastructure for timestamping in\n> > the library was considered not necessary at this time\n> > \n> > > That being said I think we might get better values using the buffer\n> > > timestamp as done in this patch, if nothing else less jitter. Only draw\n> > > back I can think of is that we can't grantee the timestamp coming from\n> > > the kernel relates to BOOTTIME.\n> > \n> > I keep hitting walls all over the places....\n> > Timestamping is performed in the cio2 receiver driver with:\n> > \n> > ns = ktime_get_ns()\n> > \n> > ktime_get() documentation reads as:\n> > \n> > /*\n> > * ktime_get() family: read the current time in a multitude of ways,\n> > *\n> > * The default time reference is CLOCK_MONOTONIC, starting at\n> > * boot time but not counting the time spent in suspend.\n> > * For other references, use the functions with \"real\", \"clocktai\",\n> > * \"boottime\" and \"raw\" suffixes.\n> > *\n> > * To get the time in a different format, use the ones wit\n> > * \"ns\", \"ts64\" and \"seconds\" suffix.\n> > *\n> > * See Documentation/core-api/timekeeping.rst for more details.\n> > */\n> > \n> > I recall I repeated multiple times the reference clock was BOOTTIME,\n> > not sure because I overlooked it or because at the time (last week...)\n> > I had no real understanding of the differences between MONOTONIC and\n> > BOOTTIME...\n> > \n> > Should we change the definition of the control again to support\n> > multiple time sources, and add one property to report which reference\n> > is used ?\n\nI'd rather standardize on BOOTTIME and fix it on the kernel side. The\nexisting implementation won't match the documentation, and that's a\nsystem bug :-)\n\n> I'm a bit slow and I still don't understand the need for users to know \n> what the reference clock is used :-) Is it not enough that it have a ts \n> it can diff between two exposers and get a usable delta in a known time \n> unit? And of course that it's not subjected to the \"fun\" of NTP clock \n> adjusting mechanisms.\n\nIf you only consider one camera, sure, but if you consider\nsynchronization with audio, as well as with other sensors\n(accelerometers for instance), knowing the exact clock source is\nimportant.\n\n> I don't want to distract for the real work in this patch and as I said \n> I'm fine with the patch as is, only wanted to point out that the todo \n> was ready to be acted on. And that IMHO I think we would gain more from \n> the solution without the todo addressed, or of course change the SOE \n> signal API to include a ts from the kernel set at the IRQ. My fear being \n> that if we produce the ts in user-space it will be subjected to jitter \n> that will be worse then using the buffer ts as done here.\n> \n> > > > +\trequest->metadata().set(controls::SensorTimestamp,\n> > > > +\t\t\t\tbuffer->metadata().timestamp);\n> > > > +\n> > > > \t/* If the buffer is cancelled force a complete of the whole request. */\n> > > > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > \t\tfor (auto it : request->buffers())","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 0047CBDC91\n\tfor ;\n\tMon, 26 Apr 2021 04:30:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 413AE688A1;\n\tMon, 26 Apr 2021 06:30:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F702605BD\n\tfor ;\n\tMon, 26 Apr 2021 06:30:12 +0200 (CEST)","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 D68AE4FB;\n\tMon, 26 Apr 2021 06:30:11 +0200 (CEST)"],"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=\"GWkgdAaw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619411412;\n\tbh=X7XtoN13BEKn9D18+qTeMgTPcQf67bbuUjPpeCyERHM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GWkgdAawr0J3YF+xJGESrBVwl0QyYENjb7yyto6X23x8kGpDkqlrAb8z47bZ/5282\n\t21rVEb3w/HrBVTjM+oIWJxInMLE4n5VKbVexApnsrmQUXvGKGZyfPN/1TlC6ytPnc6\n\t19l7CGb0TEbxUERaxFRlMBD8H7TiiJXQcDoRyQ2E=","Date":"Mon, 26 Apr 2021 07:30:05 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210421160319.42251-1-jacopo@jmondi.org>\n\t<20210421160319.42251-6-jacopo@jmondi.org>\n\t\n\t<20210422062503.yqkpwqt6if3en3cs@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 05/16] libcamera: ipu3: Report\n\tsensor timestamp","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\" "}}]