[{"id":16354,"web_url":"https://patchwork.libcamera.org/comment/16354/","msgid":"","date":"2021-04-19T13:45:29","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: Report\n\tsensor timestamp","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nThank you for your work.\n\nOn Mon, 19 Apr 2021 at 14:14, Jacopo Mondi wrote:\n\n> Report the sensor's timestamp in the Request metadata by using the\n> Unicam::Image 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> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index d1902bfc3393..7dc92acf3c4f 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1415,6 +1415,18 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer\n> *buffer)\n> << \", timestamp: \" << buffer->metadata().timestamp;\n>\n> if (stream == &unicam_[Unicam::Image]) {\n> + /*\n> + * Record the sensor timestamp in the Request.\n> + *\n> + * \\todo The sensor timestamp should be better estimated by\n> + * sampling the V4L2Device::frameStart signal.\n> + */\n>\n\nJust a minor correction to this comment - the VB2 buffer timestamp that\nis provided by the device driver is actually sampled at the CSI2 frame start\nevent, so is pretty much as close to the start of exposure as we can get\nwithout dead reckoning. This sample is also before the DMA to memory\nand V4L2Device::frameStart signal.\n\nOther than that,\nReviewed-by: Naushir Patuck \n\n\n\n> + Request *request = requestQueue_.front();\n> + ASSERT(request);\n> +\n> + request->metadata().set(controls::SensorTimestamp,\n> + buffer->metadata().timestamp);\n> +\n> /*\n> * Lookup the sensor controls used for this frame sequence\n> from\n> * DelayedControl and queue them along with the frame\n> buffer.\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>","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 86143BD814\n\tfor ;\n\tMon, 19 Apr 2021 13:46:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 094A568839;\n\tMon, 19 Apr 2021 15:46:13 +0200 (CEST)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A37BD602C4\n\tfor ;\n\tMon, 19 Apr 2021 15:46:11 +0200 (CEST)","by mail-lj1-x233.google.com with SMTP id r22so28736337ljc.5\n\tfor ;\n\tMon, 19 Apr 2021 06:46:11 -0700 (PDT)"],"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=\"OfVam/2R\"; 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=XJ/z6ccIHr9f9vxQvEd9uZrJ7qYDSLVX8fplmF8w290=;\n\tb=OfVam/2RZbl7u/R55NHimZlR/pa+XJ96pxEBHa+GiNC2Dw9a/bd3cRdfepc+n0tfGY\n\t2oxetgdgs/tljsTNM+QTx2p3BThGG3r9vTlX2Hj2+XfB7vsmWo6YQH5rVzhpfJ5gUT3f\n\tHTm0m3+RZg+c0gMUWTNSq+th05LTeGpY5mUxWd2HjmY5VC+DqFhGPQfou2yniSKaRlfA\n\tLi9xFH31HWu9BVwMzj7cH68y9o3tAsPEJZrSNunLrJFC2lCITrwTCFS9mpn/GFT5KtNl\n\t/PKhpWs+jp+H8pR4MaX5Z5wYCEfRI7syHGg4d8v4LwJTuOL5DzMv/la6Eq+W8oysZnBu\n\t+sRg==","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=XJ/z6ccIHr9f9vxQvEd9uZrJ7qYDSLVX8fplmF8w290=;\n\tb=cyT7uUy/MYTB6zpPKoo8Ch7s7GDABLjrs/AjbBwYNUCW79NP4NgMllDlS67nRkEz68\n\tN/WCEIuAKS1M4yssSOOYOzx44oIBXx1io81cRth+ipVqEArQainf2oj8AvG2XBXb1rfd\n\tIUyKKXWYBImYBjrtouXJq6cxRPB9auWrIcd960o66WWiitF/1Y7cDiAuMPdcPFLe/Lc5\n\tvZX859phUc2l9yahSt7+oEdvb1F5fYXjwIwrKvRrSASlBPQG5/NWz56DRtxCVpQDSIpL\n\tBgusga7JkSSNujcAZcjLjtz+NilkWxA+QlVXzNeL31OuNu8QyXrtryhGH10x+3rP+dhj\n\tH69w==","X-Gm-Message-State":"AOAM532pWyGFVaM8rb1yrHWyaKPqJ/jPgvRimVIV0sHfmCOhW5ob+ekT\n\ticm2j0C0JdMazceKqfKjh9R77CuHjnChlS246wWDzc43ULW6ig==","X-Google-Smtp-Source":"ABdhPJxKTlvQMKw8im8MivYBE9bTFSywMfJiF2/LZ6I/tDjyI2dyb/bf9VZkQVPxNdYk0o5IOLFfO1bVvWdyxNvaSl0=","X-Received":"by 2002:a2e:6818:: with SMTP id\n\tc24mr11089795lja.48.1618839971062; \n\tMon, 19 Apr 2021 06:46:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20210419131433.22920-1-jacopo@jmondi.org>\n\t<20210419131433.22920-11-jacopo@jmondi.org>","In-Reply-To":"<20210419131433.22920-11-jacopo@jmondi.org>","From":"Naushir Patuck ","Date":"Mon, 19 Apr 2021 14:45:29 +0100","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============1125748889334439902==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16361,"web_url":"https://patchwork.libcamera.org/comment/16361/","msgid":"<20210419135539.k34bklgdagstm6m6@uno.localdomain>","date":"2021-04-19T13:55:39","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: Report\n\tsensor timestamp","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n thanks for review!\n\nOn Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> Thank you for your work.\n>\n> On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi wrote:\n>\n> > Report the sensor's timestamp in the Request metadata by using the\n> > Unicam::Image 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> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++\n> > 1 file changed, 12 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index d1902bfc3393..7dc92acf3c4f 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1415,6 +1415,18 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer\n> > *buffer)\n> > << \", timestamp: \" << buffer->metadata().timestamp;\n> >\n> > if (stream == &unicam_[Unicam::Image]) {\n> > + /*\n> > + * Record the sensor timestamp in the Request.\n> > + *\n> > + * \\todo The sensor timestamp should be better estimated by\n> > + * sampling the V4L2Device::frameStart signal.\n> > + */\n> >\n>\n> Just a minor correction to this comment - the VB2 buffer timestamp that\n> is provided by the device driver is actually sampled at the CSI2 frame start\n> event, so is pretty much as close to the start of exposure as we can get\n> without dead reckoning. This sample is also before the DMA to memory\n> and V4L2Device::frameStart signal.\n\nAh you see! That's great, I wrongly assumed the raw buffer timestamp\nwas sampled at DMA transfer time end, as it happens on IPU3. It's\ngreat if I could drop that todo item!\n\n>\n> Other than that,\n> Reviewed-by: Naushir Patuck \n>\n>\n>\n> > + Request *request = requestQueue_.front();\n> > + ASSERT(request);\n> > +\n> > + request->metadata().set(controls::SensorTimestamp,\n> > + buffer->metadata().timestamp);\n> > +\n> > /*\n> > * Lookup the sensor controls used for this frame sequence\n> > from\n> > * DelayedControl and queue them along with the frame\n> > buffer.\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> >","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 0201BBD812\n\tfor ;\n\tMon, 19 Apr 2021 13:55:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C3A956883A;\n\tMon, 19 Apr 2021 15:55:00 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0702868824\n\tfor ;\n\tMon, 19 Apr 2021 15:55:00 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 9CA6B100013;\n\tMon, 19 Apr 2021 13:54:59 +0000 (UTC)"],"Date":"Mon, 19 Apr 2021 15:55:39 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210419135539.k34bklgdagstm6m6@uno.localdomain>","References":"<20210419131433.22920-1-jacopo@jmondi.org>\n\t<20210419131433.22920-11-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16411,"web_url":"https://patchwork.libcamera.org/comment/16411/","msgid":"","date":"2021-04-20T21:58:21","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: 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 Jacopo,\n\nThank you for the patch.\n\nOn Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote:\n> On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote:\n> > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi wrote:\n> > > Report the sensor's timestamp in the Request metadata by using the\n> > > Unicam::Image 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> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++\n> > > 1 file changed, 12 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index d1902bfc3393..7dc92acf3c4f 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1415,6 +1415,18 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > > << \", timestamp: \" << buffer->metadata().timestamp;\n> > >\n> > > if (stream == &unicam_[Unicam::Image]) {\n> > > + /*\n> > > + * Record the sensor timestamp in the Request.\n> > > + *\n> > > + * \\todo The sensor timestamp should be better estimated by\n> > > + * sampling the V4L2Device::frameStart signal.\n> > > + */\n> >\n> > Just a minor correction to this comment - the VB2 buffer timestamp that\n> > is provided by the device driver is actually sampled at the CSI2 frame start\n> > event, so is pretty much as close to the start of exposure as we can get\n> > without dead reckoning. This sample is also before the DMA to memory\n> > and V4L2Device::frameStart signal.\n\nI wish more devices and drivers would do so :-)\n\n> Ah you see! That's great, I wrongly assumed the raw buffer timestamp\n> was sampled at DMA transfer time end, as it happens on IPU3. It's\n> great if I could drop that todo item!\n>\n> > Other than that,\n> > Reviewed-by: Naushir Patuck \n> >\n> > > + Request *request = requestQueue_.front();\n\nDon't we queue multiple buffers to Unicam ? Can't a buffer complete here\nwhile the ISP is busy processing the previous frame, wouldn't\nrequestQueue_.front() then correspond to the frame processed by the ISP\n? Maybe you could use buffer->request() instead (assuming the RPi\npipeline handler calls setRequest()).\n\n> > > + ASSERT(request);\n> > > +\n> > > + request->metadata().set(controls::SensorTimestamp,\n> > > + buffer->metadata().timestamp);\n> > > +\n> > > /*\n> > > * Lookup the sensor controls used for this frame sequence from\n> > > * DelayedControl and queue them along with the frame buffer.","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 053C4BDB15\n\tfor ;\n\tTue, 20 Apr 2021 21:58:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8172168843;\n\tTue, 20 Apr 2021 23:58:27 +0200 (CEST)","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 42F1860516\n\tfor ;\n\tTue, 20 Apr 2021 23:58:26 +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 B430C411;\n\tTue, 20 Apr 2021 23:58:25 +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=\"GYGeb4P0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618955905;\n\tbh=0CLHveyODCgyBGoNOlY4r0vczPCEDeH0JS2bJ7e074U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GYGeb4P0HUbN1dZLcEzAflRH0YViGRGoeuUwk4CeOeSVzR1h2IPxTUvmpuZJ/9Rgl\n\tUCzblyD6t9cGi/0yzY5fHsP4U8bouzfx3oy9xkNv/ghbe73ZYwuaRMXeTHLTX+xazg\n\tSpygp3VGXvvZ0pQON1q2Y8OQBQoSb7O7GdNOWYpQ=","Date":"Wed, 21 Apr 2021 00:58:21 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210419131433.22920-1-jacopo@jmondi.org>\n\t<20210419131433.22920-11-jacopo@jmondi.org>\n\t\n\t<20210419135539.k34bklgdagstm6m6@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210419135539.k34bklgdagstm6m6@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16429,"web_url":"https://patchwork.libcamera.org/comment/16429/","msgid":"<20210421075017.a6kg2g2uuba3gmfn@uno.localdomain>","date":"2021-04-21T07:50:17","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: Report\n\tsensor timestamp","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n + Naush\n\nOn Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote:\n> > On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote:\n> > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi wrote:\n> > > > Report the sensor's timestamp in the Request metadata by using the\n> > > > Unicam::Image 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> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++\n> > > > 1 file changed, 12 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index d1902bfc3393..7dc92acf3c4f 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -1415,6 +1415,18 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > > > << \", timestamp: \" << buffer->metadata().timestamp;\n> > > >\n> > > > if (stream == &unicam_[Unicam::Image]) {\n> > > > + /*\n> > > > + * Record the sensor timestamp in the Request.\n> > > > + *\n> > > > + * \\todo The sensor timestamp should be better estimated by\n> > > > + * sampling the V4L2Device::frameStart signal.\n> > > > + */\n> > >\n> > > Just a minor correction to this comment - the VB2 buffer timestamp that\n> > > is provided by the device driver is actually sampled at the CSI2 frame start\n> > > event, so is pretty much as close to the start of exposure as we can get\n> > > without dead reckoning. This sample is also before the DMA to memory\n> > > and V4L2Device::frameStart signal.\n>\n> I wish more devices and drivers would do so :-)\n>\n> > Ah you see! That's great, I wrongly assumed the raw buffer timestamp\n> > was sampled at DMA transfer time end, as it happens on IPU3. It's\n> > great if I could drop that todo item!\n> >\n> > > Other than that,\n> > > Reviewed-by: Naushir Patuck \n> > >\n> > > > + Request *request = requestQueue_.front();\n>\n> Don't we queue multiple buffers to Unicam ? Can't a buffer complete here\n> while the ISP is busy processing the previous frame, wouldn't\n> requestQueue_.front() then correspond to the frame processed by the ISP\n> ? Maybe you could use buffer->request() instead (assuming the RPi\n> pipeline handler calls setRequest()).\n>\n\nThis has required me quite some mumbling, as I was a bit confused too\non how better associate a buffer with a request in this pipeline.\n\nSo, RPi does not use anything similar to FrameInfo, which I think we\nshould bring to the core framework level soon, and have ph opt-in.\nIt does not call setRequest() on buffers, and I failed to identify\nwhere to do so, given the ph design. The rest of the ph code when\nit has to retrieve the Request * goes and assume it's the first one\n(ie the oldest one) it's the one to chose. I have been wondering too\nif that's correct. Maybe I got lost into something trivial, Naush do\nyou have opinions to share ?\n\nThanks\n j\n\n> > > > + ASSERT(request);\n> > > > +\n> > > > + request->metadata().set(controls::SensorTimestamp,\n> > > > + buffer->metadata().timestamp);\n> > > > +\n> > > > /*\n> > > > * Lookup the sensor controls used for this frame sequence from\n> > > > * DelayedControl and queue them along with the frame buffer.\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 468C7BDB15\n\tfor ;\n\tWed, 21 Apr 2021 07:49:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B14D668806;\n\tWed, 21 Apr 2021 09:49:38 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 61424602CA\n\tfor ;\n\tWed, 21 Apr 2021 09:49:37 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 9E28B240008;\n\tWed, 21 Apr 2021 07:49:36 +0000 (UTC)"],"Date":"Wed, 21 Apr 2021 09:50:17 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ,\n\tNaushir Patuck ","Message-ID":"<20210421075017.a6kg2g2uuba3gmfn@uno.localdomain>","References":"<20210419131433.22920-1-jacopo@jmondi.org>\n\t<20210419131433.22920-11-jacopo@jmondi.org>\n\t\n\t<20210419135539.k34bklgdagstm6m6@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16439,"web_url":"https://patchwork.libcamera.org/comment/16439/","msgid":"","date":"2021-04-21T09:14:57","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: Report\n\tsensor timestamp","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo and Laurent,\n\nOn Wed, 21 Apr 2021 at 08:49, Jacopo Mondi wrote:\n\n> Hi Laurent,\n> + Naush\n>\n> On Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote:\n> > > On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote:\n> > > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi \n> wrote:\n> > > > > Report the sensor's timestamp in the Request metadata by using the\n> > > > > Unicam::Image buffer timestamp as an initial approximation.\n> > > > >\n> > > > > The buffer's timestamp is recorded at DMA-transfer time, and it\n> does not\n> > > > > theoretically matches the 'start of exposure' definition, but when\n> used\n> > > > > to compare two consecutive frames it gives an acceptable\n> estimation of\n> > > > > the sensor frame period duration.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12\n> ++++++++++++\n> > > > > 1 file changed, 12 insertions(+)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index d1902bfc3393..7dc92acf3c4f 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -1415,6 +1415,18 @@ void\n> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > > > > << \", timestamp: \" <<\n> buffer->metadata().timestamp;\n> > > > >\n> > > > > if (stream == &unicam_[Unicam::Image]) {\n> > > > > + /*\n> > > > > + * Record the sensor timestamp in the Request.\n> > > > > + *\n> > > > > + * \\todo The sensor timestamp should be better\n> estimated by\n> > > > > + * sampling the V4L2Device::frameStart signal.\n> > > > > + */\n> > > >\n> > > > Just a minor correction to this comment - the VB2 buffer timestamp\n> that\n> > > > is provided by the device driver is actually sampled at the CSI2\n> frame start\n> > > > event, so is pretty much as close to the start of exposure as we can\n> get\n> > > > without dead reckoning. This sample is also before the DMA to memory\n> > > > and V4L2Device::frameStart signal.\n> >\n> > I wish more devices and drivers would do so :-)\n> >\n> > > Ah you see! That's great, I wrongly assumed the raw buffer timestamp\n> > > was sampled at DMA transfer time end, as it happens on IPU3. It's\n> > > great if I could drop that todo item!\n> > >\n> > > > Other than that,\n> > > > Reviewed-by: Naushir Patuck \n> > > >\n> > > > > + Request *request = requestQueue_.front();\n> >\n> > Don't we queue multiple buffers to Unicam ? Can't a buffer complete here\n> > while the ISP is busy processing the previous frame, wouldn't\n> > requestQueue_.front() then correspond to the frame processed by the ISP\n> > ? Maybe you could use buffer->request() instead (assuming the RPi\n> > pipeline handler calls setRequest()).\n> >\n>\n\nQuite right, because of the queueing, there is a possibility that the top of\nthe request queue may lag behind and will could return out a wrong\ntimestamp with the above.\n\n\n>\n> This has required me quite some mumbling, as I was a bit confused too\n> on how better associate a buffer with a request in this pipeline.\n>\n> So, RPi does not use anything similar to FrameInfo, which I think we\n> should bring to the core framework level soon, and have ph opt-in.\n> It does not call setRequest() on buffers, and I failed to identify\n> where to do so, given the ph design. The rest of the ph code when\n> it has to retrieve the Request * goes and assume it's the first one\n> (ie the oldest one) it's the one to chose. I have been wondering too\n> if that's correct. Maybe I got lost into something trivial, Naush do\n> you have opinions to share ?\n>\n\nWe don't use FrameInfo like the other pipeline handlers, but we do have\na similar mechanism to store frame controls with a buffer.\n\nCoincidentally, I do have a patch that stores the timestamp for my IPA\nrate-control series [1]. This almost does what we need, the only thing\nmissing is some code to transfer the timestamp from this ControlList\nto the request metadata.\n\nTo get this change, we can do one of the following:\n\n1) I can backport part of the change in [1] that we care about here, and\nadd code to transfer to the request metadata. I can either provide a patch\nto add to this series or provide it on top of this series.\n\n2) I can make this change in my patch series fairly easily as well. Given\nthis\nseems to be related to Android, I doubt anybody would miss this feature\nfrom the Raspberry Pi platform until that series is merged.\n\nLet me know what you think.\n\nRegards,\nNaush\n\n\n[1] https://patchwork.libcamera.org/patch/12006/\n\n\n\n>\n> Thanks\n> j\n>\n> > > > > + ASSERT(request);\n> > > > > +\n> > > > > + request->metadata().set(controls::SensorTimestamp,\n> > > > > +\n> buffer->metadata().timestamp);\n> > > > > +\n> > > > > /*\n> > > > > * Lookup the sensor controls used for this frame\n> sequence from\n> > > > > * DelayedControl and queue them along with the\n> frame buffer.\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 07C6ABDB16\n\tfor ;\n\tWed, 21 Apr 2021 09:15:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C244168843;\n\tWed, 21 Apr 2021 11:15:15 +0200 (CEST)","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3CA4602D1\n\tfor ;\n\tWed, 21 Apr 2021 11:15:13 +0200 (CEST)","by mail-lf1-x136.google.com with SMTP id g8so65865500lfv.12\n\tfor ;\n\tWed, 21 Apr 2021 02:15:13 -0700 (PDT)"],"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=\"NFzYNh7y\"; 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=6ySOgzTxRbR+bkiAiN0Lj75rLeKOF9BGIvIwiMeZUqY=;\n\tb=NFzYNh7ylTEw4o9IOHQUJ0BaaJqmd1gslgmIHI3Alq2n9oiqssLE+X/tj2XCSrXBYC\n\t+ngtLKL1vW4sGhrABI9r74boGgDAgSx4skK9xIs+44D3j4whY5u1wTcnrJ4QgAPQqYPP\n\tgXJRQ/GNc0dwhDd76uUxXBLZUD0LFhefjJ8+ZCM+bPu8/i1LJIpB83KsWFlKk9z/Bikd\n\tyLNPaGU16AncbAVOTFAr346BVvUU+//J3I/CrIJgCnRakACTixsnS5BPhrZJdd88cd0v\n\tBold2rtnbKN+LQhE9AFnLJYd5FMgLupkUwuZbOakoMRxyH9puKKtaBYUKVwLLW/bgVbX\n\tBBSA==","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=6ySOgzTxRbR+bkiAiN0Lj75rLeKOF9BGIvIwiMeZUqY=;\n\tb=AjSu1YYEw+M8t3ddWP4IeaRalC5182QYIXzHOREUo/1GbxWxyBEbS1cdENjGvFhP2U\n\tF9/V7fkxzsl8Fvg5qQBZb3VX1OBX+vfnnZg2qH9RDJ+EQ/kNVVuAi4nDLf1HE83WGw9x\n\tRo6n66zDb8S2B8r9ooKOfwwUAy4WDqa/pz3lG6mSfuzp/GQBnlR6aw5v694xAUlrUVPX\n\tQC8zLkOw+EdI/FYj7cnGnvqoKuabmm2R11AAYlwqtB4ply/HgjMBHpN0838sxVJlaf7u\n\tRMVKFnxSPrNTB6U/gSBMfPtHzfaBhph4ib0R8lI4UqzlVlF++lilRX5eO+ajpSAfoaCd\n\tE9GA==","X-Gm-Message-State":"AOAM5311Zgj9w+1bm8TjETZp3lKKRA1qTYV4gPHtVw/bEzBcRSkb6uXU\n\tfw8MC1myapSr05KIZcenWSWD8zHvkzRgQgtCIap9Lq31ir2w4w==","X-Google-Smtp-Source":"ABdhPJzvlyGNXtpnNGRpuvFpa5AiwDfe+WEA4E8HlmpbgBJDZdtoMrJGJvG9cBEtpx/PbH8QaBYWqAnE7AV659gGdMg=","X-Received":"by 2002:a05:6512:922:: with SMTP id\n\tf2mr17895438lft.171.1618996513441; \n\tWed, 21 Apr 2021 02:15:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20210419131433.22920-1-jacopo@jmondi.org>\n\t<20210419131433.22920-11-jacopo@jmondi.org>\n\t\n\t<20210419135539.k34bklgdagstm6m6@uno.localdomain>\n\t\n\t<20210421075017.a6kg2g2uuba3gmfn@uno.localdomain>","In-Reply-To":"<20210421075017.a6kg2g2uuba3gmfn@uno.localdomain>","From":"Naushir Patuck ","Date":"Wed, 21 Apr 2021 10:14:57 +0100","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============2051989172480340803==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16446,"web_url":"https://patchwork.libcamera.org/comment/16446/","msgid":"","date":"2021-04-21T09:44:02","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: Report\n\tsensor timestamp","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Apr 21, 2021 at 10:14:57AM +0100, Naushir Patuck wrote:\n> On Wed, 21 Apr 2021 at 08:49, Jacopo Mondi wrote:\n> > On Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote:\n> > > On Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote:\n> > > > On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote:\n> > > > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi wrote:\n> > > > > > Report the sensor's timestamp in the Request metadata by using the\n> > > > > > Unicam::Image 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> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++\n> > > > > > 1 file changed, 12 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > index d1902bfc3393..7dc92acf3c4f 100644\n> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -1415,6 +1415,18 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > > > > > << \", timestamp: \" << buffer->metadata().timestamp;\n> > > > > >\n> > > > > > if (stream == &unicam_[Unicam::Image]) {\n> > > > > > + /*\n> > > > > > + * Record the sensor timestamp in the Request.\n> > > > > > + *\n> > > > > > + * \\todo The sensor timestamp should be better estimated by\n> > > > > > + * sampling the V4L2Device::frameStart signal.\n> > > > > > + */\n> > > > >\n> > > > > Just a minor correction to this comment - the VB2 buffer timestamp that\n> > > > > is provided by the device driver is actually sampled at the CSI2 frame start\n> > > > > event, so is pretty much as close to the start of exposure as we can get\n> > > > > without dead reckoning. This sample is also before the DMA to memory\n> > > > > and V4L2Device::frameStart signal.\n> > >\n> > > I wish more devices and drivers would do so :-)\n> > >\n> > > > Ah you see! That's great, I wrongly assumed the raw buffer timestamp\n> > > > was sampled at DMA transfer time end, as it happens on IPU3. It's\n> > > > great if I could drop that todo item!\n> > > >\n> > > > > Other than that,\n> > > > > Reviewed-by: Naushir Patuck \n> > > > >\n> > > > > > + Request *request = requestQueue_.front();\n> > >\n> > > Don't we queue multiple buffers to Unicam ? Can't a buffer complete here\n> > > while the ISP is busy processing the previous frame, wouldn't\n> > > requestQueue_.front() then correspond to the frame processed by the ISP\n> > > ? Maybe you could use buffer->request() instead (assuming the RPi\n> > > pipeline handler calls setRequest()).\n> \n> Quite right, because of the queueing, there is a possibility that the top of\n> the request queue may lag behind and will could return out a wrong\n> timestamp with the above.\n> \n> > This has required me quite some mumbling, as I was a bit confused too\n> > on how better associate a buffer with a request in this pipeline.\n> >\n> > So, RPi does not use anything similar to FrameInfo, which I think we\n> > should bring to the core framework level soon, and have ph opt-in.\n> > It does not call setRequest() on buffers, and I failed to identify\n> > where to do so, given the ph design. The rest of the ph code when\n> > it has to retrieve the Request * goes and assume it's the first one\n> > (ie the oldest one) it's the one to chose. I have been wondering too\n> > if that's correct. Maybe I got lost into something trivial, Naush do\n> > you have opinions to share ?\n> \n> We don't use FrameInfo like the other pipeline handlers, but we do have\n> a similar mechanism to store frame controls with a buffer.\n> \n> Coincidentally, I do have a patch that stores the timestamp for my IPA\n> rate-control series [1]. This almost does what we need, the only thing\n> missing is some code to transfer the timestamp from this ControlList\n> to the request metadata.\n> \n> To get this change, we can do one of the following:\n> \n> 1) I can backport part of the change in [1] that we care about here, and\n> add code to transfer to the request metadata. I can either provide a patch\n> to add to this series or provide it on top of this series.\n> \n> 2) I can make this change in my patch series fairly easily as well. Given this\n> seems to be related to Android, I doubt anybody would miss this feature\n> from the Raspberry Pi platform until that series is merged.\n\nThe timestamp metadata isn't limited to Android support, but nobody else\nuses it now, so the risk of breakage is fairly limited. Jacopo's series\nis nearly ready to get merged so I would avoid delaying it if possible.\nJacopo, how about keeping this patch as-is with a \\todo comment to tell\nthat the front of the queue may not be the right buffer, and fixing it\non top once both this series and Naush's series get merged ? Naush,\nwould you be able to then submit a patch on top of the combination of\nboth ? I'll review v5 of the rate limitation now.\n\n> Let me know what you think.\n> \n> [1] https://patchwork.libcamera.org/patch/12006/\n> \n> > > > > > + ASSERT(request);\n> > > > > > +\n> > > > > > + request->metadata().set(controls::SensorTimestamp,\n> > > > > > + buffer->metadata().timestamp);\n> > > > > > +\n> > > > > > /*\n> > > > > > * Lookup the sensor controls used for this frame sequence from\n> > > > > > * DelayedControl and queue them along with the frame buffer.","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 D4F1FBDB15\n\tfor ;\n\tWed, 21 Apr 2021 09:44:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E17268844;\n\tWed, 21 Apr 2021 11:44:08 +0200 (CEST)","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 B926768806\n\tfor ;\n\tWed, 21 Apr 2021 11:44:06 +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 3AAA53EE;\n\tWed, 21 Apr 2021 11:44:06 +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=\"k87vRq/9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618998246;\n\tbh=M+QnkcJ0FolmIEUfCUBgeXWIyMjDGF+rfdxcLpY7BEE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=k87vRq/9vF6pQWPrO20o3IcvUcEZMO9X2mp1uL0G+wnReKrWWcKQDb++qNmNkI/ql\n\t6ug/oFx9uTbsYPldP8Gct5VB3y6VJrRuMu7hb5bRu0zZecdlnToIVlqBaoZ8qQcOnC\n\tMGolJg64lFxIgjrKpQUm6XxefHLAXG7k5oQ9S06U=","Date":"Wed, 21 Apr 2021 12:44:02 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210419131433.22920-1-jacopo@jmondi.org>\n\t<20210419131433.22920-11-jacopo@jmondi.org>\n\t\n\t<20210419135539.k34bklgdagstm6m6@uno.localdomain>\n\t\n\t<20210421075017.a6kg2g2uuba3gmfn@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16448,"web_url":"https://patchwork.libcamera.org/comment/16448/","msgid":"","date":"2021-04-21T10:19:13","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: Report\n\tsensor timestamp","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Wed, 21 Apr 2021 at 10:44, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hello,\n>\n> On Wed, Apr 21, 2021 at 10:14:57AM +0100, Naushir Patuck wrote:\n> > On Wed, 21 Apr 2021 at 08:49, Jacopo Mondi wrote:\n> > > On Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote:\n> > > > On Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote:\n> > > > > On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote:\n> > > > > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi wrote:\n> > > > > > > Report the sensor's timestamp in the Request metadata by using\n> the\n> > > > > > > Unicam::Image buffer timestamp as an initial approximation.\n> > > > > > >\n> > > > > > > The buffer's timestamp is recorded at DMA-transfer time, and\n> it does not\n> > > > > > > theoretically matches the 'start of exposure' definition, but\n> when used\n> > > > > > > to compare two consecutive frames it gives an acceptable\n> estimation of\n> > > > > > > the sensor frame period duration.\n> > > > > > >\n> > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > ---\n> > > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12\n> ++++++++++++\n> > > > > > > 1 file changed, 12 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > index d1902bfc3393..7dc92acf3c4f 100644\n> > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > @@ -1415,6 +1415,18 @@ void\n> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > > > > > > << \", timestamp: \" <<\n> buffer->metadata().timestamp;\n> > > > > > >\n> > > > > > > if (stream == &unicam_[Unicam::Image]) {\n> > > > > > > + /*\n> > > > > > > + * Record the sensor timestamp in the Request.\n> > > > > > > + *\n> > > > > > > + * \\todo The sensor timestamp should be better\n> estimated by\n> > > > > > > + * sampling the V4L2Device::frameStart signal.\n> > > > > > > + */\n> > > > > >\n> > > > > > Just a minor correction to this comment - the VB2 buffer\n> timestamp that\n> > > > > > is provided by the device driver is actually sampled at the CSI2\n> frame start\n> > > > > > event, so is pretty much as close to the start of exposure as we\n> can get\n> > > > > > without dead reckoning. This sample is also before the DMA to\n> memory\n> > > > > > and V4L2Device::frameStart signal.\n> > > >\n> > > > I wish more devices and drivers would do so :-)\n> > > >\n> > > > > Ah you see! That's great, I wrongly assumed the raw buffer\n> timestamp\n> > > > > was sampled at DMA transfer time end, as it happens on IPU3. It's\n> > > > > great if I could drop that todo item!\n> > > > >\n> > > > > > Other than that,\n> > > > > > Reviewed-by: Naushir Patuck \n> > > > > >\n> > > > > > > + Request *request = requestQueue_.front();\n> > > >\n> > > > Don't we queue multiple buffers to Unicam ? Can't a buffer complete\n> here\n> > > > while the ISP is busy processing the previous frame, wouldn't\n> > > > requestQueue_.front() then correspond to the frame processed by the\n> ISP\n> > > > ? Maybe you could use buffer->request() instead (assuming the RPi\n> > > > pipeline handler calls setRequest()).\n> >\n> > Quite right, because of the queueing, there is a possibility that the\n> top of\n> > the request queue may lag behind and will could return out a wrong\n> > timestamp with the above.\n> >\n> > > This has required me quite some mumbling, as I was a bit confused too\n> > > on how better associate a buffer with a request in this pipeline.\n> > >\n> > > So, RPi does not use anything similar to FrameInfo, which I think we\n> > > should bring to the core framework level soon, and have ph opt-in.\n> > > It does not call setRequest() on buffers, and I failed to identify\n> > > where to do so, given the ph design. The rest of the ph code when\n> > > it has to retrieve the Request * goes and assume it's the first one\n> > > (ie the oldest one) it's the one to chose. I have been wondering too\n> > > if that's correct. Maybe I got lost into something trivial, Naush do\n> > > you have opinions to share ?\n> >\n> > We don't use FrameInfo like the other pipeline handlers, but we do have\n> > a similar mechanism to store frame controls with a buffer.\n> >\n> > Coincidentally, I do have a patch that stores the timestamp for my IPA\n> > rate-control series [1]. This almost does what we need, the only thing\n> > missing is some code to transfer the timestamp from this ControlList\n> > to the request metadata.\n> >\n> > To get this change, we can do one of the following:\n> >\n> > 1) I can backport part of the change in [1] that we care about here, and\n> > add code to transfer to the request metadata. I can either provide a\n> patch\n> > to add to this series or provide it on top of this series.\n> >\n> > 2) I can make this change in my patch series fairly easily as well.\n> Given this\n> > seems to be related to Android, I doubt anybody would miss this feature\n> > from the Raspberry Pi platform until that series is merged.\n>\n> The timestamp metadata isn't limited to Android support, but nobody else\n> uses it now, so the risk of breakage is fairly limited. Jacopo's series\n> is nearly ready to get merged so I would avoid delaying it if possible.\n> Jacopo, how about keeping this patch as-is with a \\todo comment to tell\n> that the front of the queue may not be the right buffer, and fixing it\n> on top once both this series and Naush's series get merged ? Naush,\n> would you be able to then submit a patch on top of the combination of\n> both ? I'll review v5 of the rate limitation now.\n>\n\nSounds good, we fix this up once Jacopo's series and my rate-control have\nbeen merged. Note that my series is rebased on top of David's work at:\nhttps://patchwork.libcamera.org/patch/11730/)\n\nso I will also have to wait for that to be submitted.\n\nRegards,\nNaush\n\n\n\n>\n> > Let me know what you think.\n> >\n> > [1] https://patchwork.libcamera.org/patch/12006/\n> >\n> > > > > > > + ASSERT(request);\n> > > > > > > +\n> > > > > > > +\n> request->metadata().set(controls::SensorTimestamp,\n> > > > > > > +\n> buffer->metadata().timestamp);\n> > > > > > > +\n> > > > > > > /*\n> > > > > > > * Lookup the sensor controls used for this\n> frame sequence from\n> > > > > > > * DelayedControl and queue them along with\n> the frame buffer.\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 8203DBDB16\n\tfor ;\n\tWed, 21 Apr 2021 10:19:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1051B68841;\n\tWed, 21 Apr 2021 12:19:34 +0200 (CEST)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D4E0168840\n\tfor ;\n\tWed, 21 Apr 2021 12:19:31 +0200 (CEST)","by mail-lj1-x22d.google.com with SMTP id l22so39716694ljc.9\n\tfor ;\n\tWed, 21 Apr 2021 03:19:31 -0700 (PDT)"],"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=\"jv2pF9x0\"; 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=tcEMAgt8t9hG6fwC2XKHS9xUwt1wIS4l+GBpizTSNRI=;\n\tb=jv2pF9x0YOdfFj8yHWhB+KayzGnloVGjCuTIJj+mpZ02Hxl47PDA/NKIUMnYiUTIuw\n\txwDnpG5Xp1aaoxkcHOCFL1bXtRaOrzM4VlDySCtIC8pJXq3xpd8nGQpJDmq4J5FJb6SC\n\tBY105SORYnza0oZ5uj1qbJzz3lkUo6WDuVb+qVo1fG7DBWSVzKIX9CYV6vAbBSuRdfnV\n\tWkbEOadl60rfEaky0voe0nHqazALl9NhRtOpxJzBe5LjvOhPkBI41MH1n9GtBReIIGqy\n\tvzi6wbsgvs3Y1ThYAGobQCt0+dwfL7zCwOFRK7Qyl/q4d+komMuZkKFbnXTW4rrKRkF0\n\tGEjA==","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=tcEMAgt8t9hG6fwC2XKHS9xUwt1wIS4l+GBpizTSNRI=;\n\tb=oXcP9SWC5zB/BQTvSbIxQbURYSknDwKuU+5eG2jfZMBcQXQ58eZepc6LGT89TTX0xE\n\tMAjPIeT2AAWGaRKIoflMVZ0yyBxYXrTILDfqejBts1hz831SIN6Qm+1QMznJ1zc2CqRr\n\teo70AhWHJGo706buPUtu4UZwMz6mi6BVkofJ4sZyCkbcMjMeTuMm+QrJ1AT6zauXdivt\n\tifulBSppaVYF+7aIcedUUmg7fPvDeGHwBtd58cZrNJ8/oPgFodHeESlXzkowhWsxqNhU\n\tWmt4Q5uBMS93Lm9D8Bx8nXoEdx0f+bEU0Gi7pDz/HcXHQlFhnXFABvRf8i4s0WSitn4n\n\tgJgg==","X-Gm-Message-State":"AOAM530gwQHV6gQbKbTODMMEwoaz0K6WApFyVRs9TTzlFRVzDzwpC9eh\n\t+PKGahLxghIrN8n7Ox8GXc6gvaj4ALumforXoR2hxuzGGl3DmQ==","X-Google-Smtp-Source":"ABdhPJzg5aI3OAUSzpV9LbOEaLJpDKiMeACJ1qV3W4sMPuMLa7kzZBmBUMK1A+UQjHkjBTq7YFbzMEDwb4Q1uVtKfOE=","X-Received":"by 2002:a2e:8195:: with SMTP id\n\te21mr3807988ljg.253.1619000371257; \n\tWed, 21 Apr 2021 03:19:31 -0700 (PDT)","MIME-Version":"1.0","References":"<20210419131433.22920-1-jacopo@jmondi.org>\n\t<20210419131433.22920-11-jacopo@jmondi.org>\n\t\n\t<20210419135539.k34bklgdagstm6m6@uno.localdomain>\n\t\n\t<20210421075017.a6kg2g2uuba3gmfn@uno.localdomain>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 21 Apr 2021 11:19:13 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: 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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============7215124949353975653==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16459,"web_url":"https://patchwork.libcamera.org/comment/16459/","msgid":"<20210421144035.fv67dfovmwqdp3j5@uno.localdomain>","date":"2021-04-21T14:40:35","subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: Report\n\tsensor timestamp","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello\n\nOn Wed, Apr 21, 2021 at 11:19:13AM +0100, Naushir Patuck wrote:\n\n[snip]\n\n> > > > > > >\n> > > > > > > > + Request *request = requestQueue_.front();\n> > > > >\n> > > > > Don't we queue multiple buffers to Unicam ? Can't a buffer complete\n> > here\n> > > > > while the ISP is busy processing the previous frame, wouldn't\n> > > > > requestQueue_.front() then correspond to the frame processed by the\n> > ISP\n> > > > > ? Maybe you could use buffer->request() instead (assuming the RPi\n> > > > > pipeline handler calls setRequest()).\n> > >\n> > > Quite right, because of the queueing, there is a possibility that the\n> > top of\n> > > the request queue may lag behind and will could return out a wrong\n> > > timestamp with the above.\n> > >\n> > > > This has required me quite some mumbling, as I was a bit confused too\n> > > > on how better associate a buffer with a request in this pipeline.\n> > > >\n> > > > So, RPi does not use anything similar to FrameInfo, which I think we\n> > > > should bring to the core framework level soon, and have ph opt-in.\n> > > > It does not call setRequest() on buffers, and I failed to identify\n> > > > where to do so, given the ph design. The rest of the ph code when\n> > > > it has to retrieve the Request * goes and assume it's the first one\n> > > > (ie the oldest one) it's the one to chose. I have been wondering too\n> > > > if that's correct. Maybe I got lost into something trivial, Naush do\n> > > > you have opinions to share ?\n> > >\n> > > We don't use FrameInfo like the other pipeline handlers, but we do have\n> > > a similar mechanism to store frame controls with a buffer.\n> > >\n> > > Coincidentally, I do have a patch that stores the timestamp for my IPA\n> > > rate-control series [1]. This almost does what we need, the only thing\n> > > missing is some code to transfer the timestamp from this ControlList\n> > > to the request metadata.\n> > >\n> > > To get this change, we can do one of the following:\n> > >\n> > > 1) I can backport part of the change in [1] that we care about here, and\n> > > add code to transfer to the request metadata. I can either provide a\n> > patch\n> > > to add to this series or provide it on top of this series.\n> > >\n> > > 2) I can make this change in my patch series fairly easily as well.\n> > Given this\n> > > seems to be related to Android, I doubt anybody would miss this feature\n> > > from the Raspberry Pi platform until that series is merged.\n> >\n> > The timestamp metadata isn't limited to Android support, but nobody else\n> > uses it now, so the risk of breakage is fairly limited. Jacopo's series\n> > is nearly ready to get merged so I would avoid delaying it if possible.\n> > Jacopo, how about keeping this patch as-is with a \\todo comment to tell\n> > that the front of the queue may not be the right buffer, and fixing it\n> > on top once both this series and Naush's series get merged ? Naush,\n> > would you be able to then submit a patch on top of the combination of\n> > both ? I'll review v5 of the rate limitation now.\n> >\n>\n> Sounds good, we fix this up once Jacopo's series and my rate-control have\n> been merged. Note that my series is rebased on top of David's work at:\n> https://patchwork.libcamera.org/patch/11730/)\n>\n> so I will also have to wait for that to be submitted.\n\nGreat, I'll record that with a \\todo then\n\nThanks\n j\n\n>\n> Regards,\n> Naush\n>\n>\n>\n> >\n> > > Let me know what you think.\n> > >\n> > > [1] https://patchwork.libcamera.org/patch/12006/\n> > >\n> > > > > > > > + ASSERT(request);\n> > > > > > > > +\n> > > > > > > > +\n> > request->metadata().set(controls::SensorTimestamp,\n> > > > > > > > +\n> > buffer->metadata().timestamp);\n> > > > > > > > +\n> > > > > > > > /*\n> > > > > > > > * Lookup the sensor controls used for this\n> > frame sequence from\n> > > > > > > > * DelayedControl and queue them along with\n> > the frame buffer.\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 B20F0BDB15\n\tfor ;\n\tWed, 21 Apr 2021 14:39:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 135D36884B;\n\tWed, 21 Apr 2021 16:39:57 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B576168835\n\tfor ;\n\tWed, 21 Apr 2021 16:39:55 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 0333620000A;\n\tWed, 21 Apr 2021 14:39:54 +0000 (UTC)"],"Date":"Wed, 21 Apr 2021 16:40:35 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210421144035.fv67dfovmwqdp3j5@uno.localdomain>","References":"<20210419131433.22920-1-jacopo@jmondi.org>\n\t<20210419131433.22920-11-jacopo@jmondi.org>\n\t\n\t<20210419135539.k34bklgdagstm6m6@uno.localdomain>\n\t\n\t<20210421075017.a6kg2g2uuba3gmfn@uno.localdomain>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 10/13] libcamera: raspberry: 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]