[{"id":15533,"web_url":"https://patchwork.libcamera.org/comment/15533/","msgid":"","date":"2021-03-08T19:18:33","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Make CamHelpers\n\treturn the frame delay for vblanking","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Thu, Mar 04, 2021 at 03:31:19PM +0000, David Plowman wrote:\n> For some sensors (e.g. imx477) we need to update the vblanking on the\n> frame before the exposure.\n\nThat is really peculiar, and quite annoying. I'm OK with this change as\na temporary fix, do you think we could get some support from Sony to\nfigure out what's happening ?\n\n> For this reason the GetDelays method must\n> also return the number of frame delays for the vblanking control.\n> \n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 4 +++-\n> src/ipa/raspberrypi/cam_helper.hpp | 9 ++++++---\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 7 +++++--\n> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 7 +++++--\n> src/ipa/raspberrypi/raspberrypi.cpp | 6 +++---\n> 5 files changed, 22 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 2837fcce..0ae0baa0 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -95,7 +95,8 @@ void CamHelper::SetCameraMode(const CameraMode &mode)\n> \tinitialized_ = true;\n> }\n> \n> -void CamHelper::GetDelays(int &exposure_delay, int &gain_delay) const\n> +void CamHelper::GetDelays(int &exposure_delay, int &gain_delay,\n> +\t\t\t int &vblank_delay) const\n> {\n> \t/*\n> \t * These values are correct for many sensors. Other sensors will\n> @@ -103,6 +104,7 @@ void CamHelper::GetDelays(int &exposure_delay, int &gain_delay) const\n> \t */\n> \texposure_delay = 2;\n> \tgain_delay = 1;\n> +\tvblank_delay = 2;\n> }\n> \n> bool CamHelper::SensorEmbeddedDataPresent() const\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index 14d70112..8c5659ed 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -28,8 +28,10 @@ namespace RPiController {\n> // exposure time, and to convert between the sensor's gain codes and actual\n> // gains.\n> //\n> -// A method to return the number of frames of delay between updating exposure\n> -// and analogue gain and the changes taking effect. For many sensors these\n> +// A method to return the number of frames of delay between updating exposure,\n> +// analogue gain and vblanking, and for the changes to take effect. For many\n> +// sensors these take the values 2, 1 and 2 respectively, but sensors that are\n> +// different will need to over-ride the default method provided.\n> // take the values 2 and 1 respectively, but sensors that are different will\n> // need to over-ride the default method provided.\n\nHave you forgotten to remove the last two lines ?\n\n> //\n> @@ -72,7 +74,8 @@ public:\n> \t\t\t\t double maxFrameDuration) const;\n> \tvirtual uint32_t GainCode(double gain) const = 0;\n> \tvirtual double Gain(uint32_t gain_code) const = 0;\n> -\tvirtual void GetDelays(int &exposure_delay, int &gain_delay) const;\n> +\tvirtual void GetDelays(int &exposure_delay, int &gain_delay,\n> +\t\t\t int &vblank_delay) const;\n> \tvirtual bool SensorEmbeddedDataPresent() const;\n> \tvirtual unsigned int HideFramesStartup() const;\n> \tvirtual unsigned int HideFramesModeSwitch() const;\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 419f8e77..73a5ca7d 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -37,7 +37,8 @@ public:\n> \tCamHelperImx477();\n> \tuint32_t GainCode(double gain) const override;\n> \tdouble Gain(uint32_t gain_code) const override;\n> -\tvoid GetDelays(int &exposure_delay, int &gain_delay) const override;\n> +\tvoid GetDelays(int &exposure_delay, int &gain_delay,\n> +\t\t int &vblank_delay) const override;\n> \tbool SensorEmbeddedDataPresent() const override;\n> \n> private:\n> @@ -63,10 +64,12 @@ double CamHelperImx477::Gain(uint32_t gain_code) const\n> \treturn 1024.0 / (1024 - gain_code);\n> }\n> \n> -void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay) const\n> +void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,\n> +\t\t\t\tint &vblank_delay) const\n> {\n> \texposure_delay = 2;\n> \tgain_delay = 2;\n> +\tvblank_delay = 3;\n> }\n> \n> bool CamHelperImx477::SensorEmbeddedDataPresent() const\n> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> index 75486e90..12be6bf9 100644\n> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> @@ -17,7 +17,8 @@ public:\n> \tCamHelperOv5647();\n> \tuint32_t GainCode(double gain) const override;\n> \tdouble Gain(uint32_t gain_code) const override;\n> -\tvoid GetDelays(int &exposure_delay, int &gain_delay) const override;\n> +\tvoid GetDelays(int &exposure_delay, int &gain_delay,\n> +\t\t int &vblank_delay) const override;\n> \tunsigned int HideFramesStartup() const override;\n> \tunsigned int HideFramesModeSwitch() const override;\n> \tunsigned int MistrustFramesStartup() const override;\n> @@ -51,7 +52,8 @@ double CamHelperOv5647::Gain(uint32_t gain_code) const\n> \treturn static_cast(gain_code) / 16.0;\n> }\n> \n> -void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const\n> +void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay,\n> +\t\t\t\tint &vblank_delay) const\n> {\n> \t/*\n> \t * We run this sensor in a mode where the gain delay is bumped up to\n> @@ -59,6 +61,7 @@ void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const\n> \t */\n> \texposure_delay = 2;\n> \tgain_delay = 2;\n> +\tvblank_delay = 2;\n> }\n> \n> unsigned int CamHelperOv5647::HideFramesStartup() const\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 6348d071..741bff4c 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -342,14 +342,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\t * Pass out the sensor config to the pipeline handler in order\n> \t\t * to setup the staggered writer class.\n> \t\t */\n> -\t\tint gainDelay, exposureDelay, sensorMetadata;\n> -\t\thelper_->GetDelays(exposureDelay, gainDelay);\n> +\t\tint gainDelay, exposureDelay, vblankDelay, sensorMetadata;\n> +\t\thelper_->GetDelays(exposureDelay, gainDelay, vblankDelay);\n> \t\tsensorMetadata = helper_->SensorEmbeddedDataPresent();\n> \n> \t\tresult->params |= ipa::RPi::ConfigSensorParams;\n> \t\tresult->sensorConfig.gainDelay = gainDelay;\n> \t\tresult->sensorConfig.exposureDelay = exposureDelay;\n> -\t\tresult->sensorConfig.vblank = exposureDelay;\n> +\t\tresult->sensorConfig.vblank = vblankDelay;\n\nUnrelated to this patch, should the vblank field be renamed to\nvblankDelay ?\n\nAt some point it could be useful to make this interface a bit more\ngeneric, but that can wait.\n\nWith the above issue fixed,\n\nReviewed-by: Laurent Pinchart \n\n> \t\tresult->sensorConfig.sensorMetadata = sensorMetadata;\n> \t}\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 247D7BD1F1\n\tfor ;\n\tMon, 8 Mar 2021 19:19:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C2BD68AA2;\n\tMon, 8 Mar 2021 20:19:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A45B860520\n\tfor ;\n\tMon, 8 Mar 2021 20:19:04 +0100 (CET)","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 2A5E2E7B;\n\tMon, 8 Mar 2021 20:19:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qsoLrsH7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615231144;\n\tbh=64kSvEWcGk+G7cECy0e9tRtDZoQnGlXwJ9i2kLu4x0A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qsoLrsH7PWi9C30hPRfMzvfoFn5UwL4ZwGF7K+9VcCxoj+zn6fJZa6otazaP2/Uer\n\tqiyGRMp8VeWoyG/GHrs30LyWk16HIF2foqjm9VH0ODjjWBaltwT9CcV+hnZncr8G8D\n\tCa493YiJHx/q8lM/wDhY91TbDB6gTh0Ztun+RVVY=","Date":"Mon, 8 Mar 2021 21:18:33 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210304153120.1904-1-david.plowman@raspberrypi.com>\n\t<20210304153120.1904-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210304153120.1904-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Make CamHelpers\n\treturn the frame delay for vblanking","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15535,"web_url":"https://patchwork.libcamera.org/comment/15535/","msgid":"","date":"2021-03-08T22:01:37","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Make CamHelpers\n\treturn the frame delay for vblanking","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the review.\n\nOn Mon, 8 Mar 2021 at 19:19, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Thu, Mar 04, 2021 at 03:31:19PM +0000, David Plowman wrote:\n> > For some sensors (e.g. imx477) we need to update the vblanking on the\n> > frame before the exposure.\n>\n> That is really peculiar, and quite annoying. I'm OK with this change as\n> a temporary fix, do you think we could get some support from Sony to\n> figure out what's happening ?\n\nYes, we shall come back and take another look at this. We've been\nround the loop of putting in extra debug, adding careful manipulation\nof the group hold register, but without getting it to work. But once\nwe've got the DelayedControls fixed so that all our sensors actually\nwork as they're meant to, then I'm sure we'll return to this and\nmanage to figure it out.\n\n>\n> > For this reason the GetDelays method must\n> > also return the number of frame delays for the vblanking control.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 4 +++-\n> > src/ipa/raspberrypi/cam_helper.hpp | 9 ++++++---\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 7 +++++--\n> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 7 +++++--\n> > src/ipa/raspberrypi/raspberrypi.cpp | 6 +++---\n> > 5 files changed, 22 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> > index 2837fcce..0ae0baa0 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -95,7 +95,8 @@ void CamHelper::SetCameraMode(const CameraMode &mode)\n> > initialized_ = true;\n> > }\n> >\n> > -void CamHelper::GetDelays(int &exposure_delay, int &gain_delay) const\n> > +void CamHelper::GetDelays(int &exposure_delay, int &gain_delay,\n> > + int &vblank_delay) const\n> > {\n> > /*\n> > * These values are correct for many sensors. Other sensors will\n> > @@ -103,6 +104,7 @@ void CamHelper::GetDelays(int &exposure_delay, int &gain_delay) const\n> > */\n> > exposure_delay = 2;\n> > gain_delay = 1;\n> > + vblank_delay = 2;\n> > }\n> >\n> > bool CamHelper::SensorEmbeddedDataPresent() const\n> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> > index 14d70112..8c5659ed 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -28,8 +28,10 @@ namespace RPiController {\n> > // exposure time, and to convert between the sensor's gain codes and actual\n> > // gains.\n> > //\n> > -// A method to return the number of frames of delay between updating exposure\n> > -// and analogue gain and the changes taking effect. For many sensors these\n> > +// A method to return the number of frames of delay between updating exposure,\n> > +// analogue gain and vblanking, and for the changes to take effect. For many\n> > +// sensors these take the values 2, 1 and 2 respectively, but sensors that are\n> > +// different will need to over-ride the default method provided.\n> > // take the values 2 and 1 respectively, but sensors that are different will\n> > // need to over-ride the default method provided.\n>\n> Have you forgotten to remove the last two lines ?\n\nAh yes, must have missed those. Will submit a revised version!\n\n>\n> > //\n> > @@ -72,7 +74,8 @@ public:\n> > double maxFrameDuration) const;\n> > virtual uint32_t GainCode(double gain) const = 0;\n> > virtual double Gain(uint32_t gain_code) const = 0;\n> > - virtual void GetDelays(int &exposure_delay, int &gain_delay) const;\n> > + virtual void GetDelays(int &exposure_delay, int &gain_delay,\n> > + int &vblank_delay) const;\n> > virtual bool SensorEmbeddedDataPresent() const;\n> > virtual unsigned int HideFramesStartup() const;\n> > virtual unsigned int HideFramesModeSwitch() const;\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index 419f8e77..73a5ca7d 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -37,7 +37,8 @@ public:\n> > CamHelperImx477();\n> > uint32_t GainCode(double gain) const override;\n> > double Gain(uint32_t gain_code) const override;\n> > - void GetDelays(int &exposure_delay, int &gain_delay) const override;\n> > + void GetDelays(int &exposure_delay, int &gain_delay,\n> > + int &vblank_delay) const override;\n> > bool SensorEmbeddedDataPresent() const override;\n> >\n> > private:\n> > @@ -63,10 +64,12 @@ double CamHelperImx477::Gain(uint32_t gain_code) const\n> > return 1024.0 / (1024 - gain_code);\n> > }\n> >\n> > -void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay) const\n> > +void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,\n> > + int &vblank_delay) const\n> > {\n> > exposure_delay = 2;\n> > gain_delay = 2;\n> > + vblank_delay = 3;\n> > }\n> >\n> > bool CamHelperImx477::SensorEmbeddedDataPresent() const\n> > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > index 75486e90..12be6bf9 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > @@ -17,7 +17,8 @@ public:\n> > CamHelperOv5647();\n> > uint32_t GainCode(double gain) const override;\n> > double Gain(uint32_t gain_code) const override;\n> > - void GetDelays(int &exposure_delay, int &gain_delay) const override;\n> > + void GetDelays(int &exposure_delay, int &gain_delay,\n> > + int &vblank_delay) const override;\n> > unsigned int HideFramesStartup() const override;\n> > unsigned int HideFramesModeSwitch() const override;\n> > unsigned int MistrustFramesStartup() const override;\n> > @@ -51,7 +52,8 @@ double CamHelperOv5647::Gain(uint32_t gain_code) const\n> > return static_cast(gain_code) / 16.0;\n> > }\n> >\n> > -void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const\n> > +void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay,\n> > + int &vblank_delay) const\n> > {\n> > /*\n> > * We run this sensor in a mode where the gain delay is bumped up to\n> > @@ -59,6 +61,7 @@ void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const\n> > */\n> > exposure_delay = 2;\n> > gain_delay = 2;\n> > + vblank_delay = 2;\n> > }\n> >\n> > unsigned int CamHelperOv5647::HideFramesStartup() const\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 6348d071..741bff4c 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -342,14 +342,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > * Pass out the sensor config to the pipeline handler in order\n> > * to setup the staggered writer class.\n> > */\n> > - int gainDelay, exposureDelay, sensorMetadata;\n> > - helper_->GetDelays(exposureDelay, gainDelay);\n> > + int gainDelay, exposureDelay, vblankDelay, sensorMetadata;\n> > + helper_->GetDelays(exposureDelay, gainDelay, vblankDelay);\n> > sensorMetadata = helper_->SensorEmbeddedDataPresent();\n> >\n> > result->params |= ipa::RPi::ConfigSensorParams;\n> > result->sensorConfig.gainDelay = gainDelay;\n> > result->sensorConfig.exposureDelay = exposureDelay;\n> > - result->sensorConfig.vblank = exposureDelay;\n> > + result->sensorConfig.vblank = vblankDelay;\n>\n> Unrelated to this patch, should the vblank field be renamed to\n> vblankDelay ?\n\nIndeed, that looks like vblankDelay would be a better name. Looks like\n\"raspberrypi.mojom\" is the offender there - shall I add an extra\ncommit to fix that?\n\nLet me make a v2 of this set with that extra patch and I'll re-send.\n\nThanks again!\nDavid\n\n>\n> At some point it could be useful to make this interface a bit more\n> generic, but that can wait.\n>\n> With the above issue fixed,\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > result->sensorConfig.sensorMetadata = sensorMetadata;\n> > }\n> >\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 4D8CCBD1F1\n\tfor ;\n\tMon, 8 Mar 2021 22:01:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C1D968AA4;\n\tMon, 8 Mar 2021 23:01:51 +0100 (CET)","from mail-oi1-x22f.google.com (mail-oi1-x22f.google.com\n\t[IPv6:2607:f8b0:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97797602ED\n\tfor ;\n\tMon, 8 Mar 2021 23:01:50 +0100 (CET)","by mail-oi1-x22f.google.com with SMTP id x135so8177413oia.9\n\tfor ;\n\tMon, 08 Mar 2021 14:01:50 -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=\"tbF444hB\"; 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=YKEjC+Kt0vLyPP4eO3R3jMgT958tvCHfPZo8iDHa+V8=;\n\tb=tbF444hBBIiO3i0bSGk8PeG8vbegSIIu7i+HsL4s4VsDYoGPB9fu7hJbWXCvXtdB2U\n\tLVY8ctDhA57PmgkFytAVGufeSB9FBYt0lnok7Uj/C+6xUno+GgvM4HkCRqHR6QAOC0Jm\n\tdCYoFjIKB1bVnAPLlCLI+30rNDSv0EmRJUAQAwuwF6Lz6Hpgp1TyPYtjNW+ZMqk9EFo8\n\tgxQDUzCDAPRyeEE2Trzkk/liMH+09VQzxot/U6CxZEM90QtQx9XXOOPpYmXU+BS5Xq5M\n\tgF12bvcmeFmjI5OYNWZzaEPFxf9t9Vdj6Pe7P1raH2sPNmrZ8CHAGVDzclSq1shdoQq5\n\trE9g==","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=YKEjC+Kt0vLyPP4eO3R3jMgT958tvCHfPZo8iDHa+V8=;\n\tb=GZrq0MzZbczwdq9bgsmfgVBiPYHoXH5bGkMECCKc1tbjKA3lnCHRs2szfhAVUGbuDy\n\t4QZ6bYI6zF66ngRkgDyGingYV+85F0m/2bB2q8CvByTdYf3yk0afqEUvPvyWhQbrm22H\n\tFmKbaYNtWq7w7rNBtg2kjO9ysomBdN8KCjGlOt1fFA3F0oezcOyopLY/l6DDk06P6/Mw\n\txOl7qrRCN828GwaKETyCawsxLMxCh24pPVxtK4UZXV2m9KGwgtGTC/ScoIHUKLEglDsf\n\tD1SiOEPIwOlJJQEuvh1YCyK/Y1DsuQEd535Gq9wTVwpvbjDDfX8dP2PrKp6A+eTLk+WE\n\tJDUg==","X-Gm-Message-State":"AOAM531OWL0da5ttrd7dwPrTSuR+iPeDTzAY/9RAfQVMvs/HaxnO9SQH\n\tzZxLVURx8mVfkSbHZv5P3hVMy60wSnq7PdZWktutJw==","X-Google-Smtp-Source":"ABdhPJy9xvDSToM1qC5Y0HLSJEL7He0CqRRM1QeUnVIpfvi0Gf5ubp/Af5moE/nnSZFrNPWzOlLFbMbKX6r4XAnRgyw=","X-Received":"by 2002:aca:f389:: with SMTP id r131mr745589oih.22.1615240909123;\n\tMon, 08 Mar 2021 14:01:49 -0800 (PST)","MIME-Version":"1.0","References":"<20210304153120.1904-1-david.plowman@raspberrypi.com>\n\t<20210304153120.1904-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Mon, 8 Mar 2021 22:01:37 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Make CamHelpers\n\treturn the frame delay for vblanking","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":15536,"web_url":"https://patchwork.libcamera.org/comment/15536/","msgid":"","date":"2021-03-08T22:04:27","subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Make CamHelpers\n\treturn the frame delay for vblanking","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Mon, Mar 08, 2021 at 10:01:37PM +0000, David Plowman wrote:\n> On Mon, 8 Mar 2021 at 19:19, Laurent Pinchart wrote:\n> > On Thu, Mar 04, 2021 at 03:31:19PM +0000, David Plowman wrote:\n> > > For some sensors (e.g. imx477) we need to update the vblanking on the\n> > > frame before the exposure.\n> >\n> > That is really peculiar, and quite annoying. I'm OK with this change as\n> > a temporary fix, do you think we could get some support from Sony to\n> > figure out what's happening ?\n> \n> Yes, we shall come back and take another look at this. We've been\n> round the loop of putting in extra debug, adding careful manipulation\n> of the group hold register, but without getting it to work. But once\n> we've got the DelayedControls fixed so that all our sensors actually\n> work as they're meant to, then I'm sure we'll return to this and\n> manage to figure it out.\n> \n> > > For this reason the GetDelays method must\n> > > also return the number of frame delays for the vblanking control.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper.cpp | 4 +++-\n> > > src/ipa/raspberrypi/cam_helper.hpp | 9 ++++++---\n> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 7 +++++--\n> > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 7 +++++--\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 6 +++---\n> > > 5 files changed, 22 insertions(+), 11 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index 2837fcce..0ae0baa0 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -95,7 +95,8 @@ void CamHelper::SetCameraMode(const CameraMode &mode)\n> > > initialized_ = true;\n> > > }\n> > >\n> > > -void CamHelper::GetDelays(int &exposure_delay, int &gain_delay) const\n> > > +void CamHelper::GetDelays(int &exposure_delay, int &gain_delay,\n> > > + int &vblank_delay) const\n> > > {\n> > > /*\n> > > * These values are correct for many sensors. Other sensors will\n> > > @@ -103,6 +104,7 @@ void CamHelper::GetDelays(int &exposure_delay, int &gain_delay) const\n> > > */\n> > > exposure_delay = 2;\n> > > gain_delay = 1;\n> > > + vblank_delay = 2;\n> > > }\n> > >\n> > > bool CamHelper::SensorEmbeddedDataPresent() const\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> > > index 14d70112..8c5659ed 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > @@ -28,8 +28,10 @@ namespace RPiController {\n> > > // exposure time, and to convert between the sensor's gain codes and actual\n> > > // gains.\n> > > //\n> > > -// A method to return the number of frames of delay between updating exposure\n> > > -// and analogue gain and the changes taking effect. For many sensors these\n> > > +// A method to return the number of frames of delay between updating exposure,\n> > > +// analogue gain and vblanking, and for the changes to take effect. For many\n> > > +// sensors these take the values 2, 1 and 2 respectively, but sensors that are\n> > > +// different will need to over-ride the default method provided.\n> > > // take the values 2 and 1 respectively, but sensors that are different will\n> > > // need to over-ride the default method provided.\n> >\n> > Have you forgotten to remove the last two lines ?\n> \n> Ah yes, must have missed those. Will submit a revised version!\n> \n> > > //\n> > > @@ -72,7 +74,8 @@ public:\n> > > double maxFrameDuration) const;\n> > > virtual uint32_t GainCode(double gain) const = 0;\n> > > virtual double Gain(uint32_t gain_code) const = 0;\n> > > - virtual void GetDelays(int &exposure_delay, int &gain_delay) const;\n> > > + virtual void GetDelays(int &exposure_delay, int &gain_delay,\n> > > + int &vblank_delay) const;\n> > > virtual bool SensorEmbeddedDataPresent() const;\n> > > virtual unsigned int HideFramesStartup() const;\n> > > virtual unsigned int HideFramesModeSwitch() const;\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > index 419f8e77..73a5ca7d 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > @@ -37,7 +37,8 @@ public:\n> > > CamHelperImx477();\n> > > uint32_t GainCode(double gain) const override;\n> > > double Gain(uint32_t gain_code) const override;\n> > > - void GetDelays(int &exposure_delay, int &gain_delay) const override;\n> > > + void GetDelays(int &exposure_delay, int &gain_delay,\n> > > + int &vblank_delay) const override;\n> > > bool SensorEmbeddedDataPresent() const override;\n> > >\n> > > private:\n> > > @@ -63,10 +64,12 @@ double CamHelperImx477::Gain(uint32_t gain_code) const\n> > > return 1024.0 / (1024 - gain_code);\n> > > }\n> > >\n> > > -void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay) const\n> > > +void CamHelperImx477::GetDelays(int &exposure_delay, int &gain_delay,\n> > > + int &vblank_delay) const\n> > > {\n> > > exposure_delay = 2;\n> > > gain_delay = 2;\n> > > + vblank_delay = 3;\n> > > }\n> > >\n> > > bool CamHelperImx477::SensorEmbeddedDataPresent() const\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > index 75486e90..12be6bf9 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > @@ -17,7 +17,8 @@ public:\n> > > CamHelperOv5647();\n> > > uint32_t GainCode(double gain) const override;\n> > > double Gain(uint32_t gain_code) const override;\n> > > - void GetDelays(int &exposure_delay, int &gain_delay) const override;\n> > > + void GetDelays(int &exposure_delay, int &gain_delay,\n> > > + int &vblank_delay) const override;\n> > > unsigned int HideFramesStartup() const override;\n> > > unsigned int HideFramesModeSwitch() const override;\n> > > unsigned int MistrustFramesStartup() const override;\n> > > @@ -51,7 +52,8 @@ double CamHelperOv5647::Gain(uint32_t gain_code) const\n> > > return static_cast(gain_code) / 16.0;\n> > > }\n> > >\n> > > -void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const\n> > > +void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay,\n> > > + int &vblank_delay) const\n> > > {\n> > > /*\n> > > * We run this sensor in a mode where the gain delay is bumped up to\n> > > @@ -59,6 +61,7 @@ void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const\n> > > */\n> > > exposure_delay = 2;\n> > > gain_delay = 2;\n> > > + vblank_delay = 2;\n> > > }\n> > >\n> > > unsigned int CamHelperOv5647::HideFramesStartup() const\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 6348d071..741bff4c 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -342,14 +342,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > * Pass out the sensor config to the pipeline handler in order\n> > > * to setup the staggered writer class.\n> > > */\n> > > - int gainDelay, exposureDelay, sensorMetadata;\n> > > - helper_->GetDelays(exposureDelay, gainDelay);\n> > > + int gainDelay, exposureDelay, vblankDelay, sensorMetadata;\n> > > + helper_->GetDelays(exposureDelay, gainDelay, vblankDelay);\n> > > sensorMetadata = helper_->SensorEmbeddedDataPresent();\n> > >\n> > > result->params |= ipa::RPi::ConfigSensorParams;\n> > > result->sensorConfig.gainDelay = gainDelay;\n> > > result->sensorConfig.exposureDelay = exposureDelay;\n> > > - result->sensorConfig.vblank = exposureDelay;\n> > > + result->sensorConfig.vblank = vblankDelay;\n> >\n> > Unrelated to this patch, should the vblank field be renamed to\n> > vblankDelay ?\n> \n> Indeed, that looks like vblankDelay would be a better name. Looks like\n> \"raspberrypi.mojom\" is the offender there - shall I add an extra\n> commit to fix that?\n\nThat would be nice. Thank :-)\n\n> Let me make a v2 of this set with that extra patch and I'll re-send.\n> \n> > At some point it could be useful to make this interface a bit more\n> > generic, but that can wait.\n> >\n> > With the above issue fixed,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > result->sensorConfig.sensorMetadata = sensorMetadata;\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 0F4E9BD80C\n\tfor ;\n\tMon, 8 Mar 2021 22:05:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5918568A9F;\n\tMon, 8 Mar 2021 23:05:00 +0100 (CET)","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 89FCE602ED\n\tfor ;\n\tMon, 8 Mar 2021 23:04:58 +0100 (CET)","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 05413E7B;\n\tMon, 8 Mar 2021 23:04:57 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jR8HUASL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615241098;\n\tbh=2+6s4+yJa43pp7VLDD46AaWNpV/roWuQ6fl8gkKYGu0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jR8HUASL2NnrR8jlzqVCQ7gx6xQYmewlDh+SvEzbIbOkOSVDCj7bHhO0ZC+HRQxhm\n\tIENliYk4vFR8Ebk1AoFxeRo1jWA16YwoTWOd8A7ltuIg1zAKaglNfq2maKwBuNIvLw\n\tFTfewi7akH7IIHiMH/w7V3oQibf5EnpAcpACaPzw=","Date":"Tue, 9 Mar 2021 00:04:27 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210304153120.1904-1-david.plowman@raspberrypi.com>\n\t<20210304153120.1904-2-david.plowman@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/2] ipa: raspberrypi: Make CamHelpers\n\treturn the frame delay for vblanking","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\" "}}]