[{"id":18067,"web_url":"https://patchwork.libcamera.org/comment/18067/","msgid":"<20210709161534.emoc35b6skeyrn7a@uno.localdomain>","date":"2021-07-09T16:15:34","subject":"Re: [libcamera-devel] [PATCH v6 4/8] ipa: raspberrypi: Add\n\tframe_length to DeviceStatus","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Fri, Jul 09, 2021 at 03:58:21PM +0100, Naushir Patuck wrote:\n> Update the IPA to fill frame length into the DeviceStatus struct from the\n> VBLANK Control value passed from DelayedControls.\n>\n> Update imx477 and imx219 CamHelper classes to extract the frame length from the\n> embedded data buffer.\n>\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Reviewed-by: Kieran Bingham \n\nSeems good!\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 1 +\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +++++-\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 6 +++++-\n> src/ipa/raspberrypi/controller/device_status.h | 5 ++++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 2 ++\n> 5 files changed, 17 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 1ec3f03e1aa3..3c6afce7572b 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -187,6 +187,7 @@ void CamHelper::parseEmbeddedData(Span buffer,\n>\n> \tdeviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> \tdeviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> +\tdeviceStatus.frame_length = parsedDeviceStatus.frame_length;\n>\n> \tLOG(IPARPI, Debug) << \"Metadata updated - \" << deviceStatus;\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index 4d68e01fce71..a3caab714602 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -30,7 +30,10 @@ using namespace RPiController;\n> constexpr uint32_t gainReg = 0x157;\n> constexpr uint32_t expHiReg = 0x15a;\n> constexpr uint32_t expLoReg = 0x15b;\n> -constexpr std::initializer_list registerList [[maybe_unused]] = { expHiReg, expLoReg, gainReg };\n> +constexpr uint32_t frameLengthHiReg = 0x160;\n> +constexpr uint32_t frameLengthLoReg = 0x161;\n> +constexpr std::initializer_list registerList [[maybe_unused]]\n> +\t= { expHiReg, expLoReg, gainReg, frameLengthHiReg, frameLengthLoReg };\n>\n> class CamHelperImx219 : public CamHelper\n> {\n> @@ -93,6 +96,7 @@ void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap ®isters,\n>\n> \tdeviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> \tdeviceStatus.analogue_gain = Gain(registers.at(gainReg));\n> +\tdeviceStatus.frame_length = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg);\n>\n> \tmetadata.Set(\"device.status\", deviceStatus);\n> }\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index efd1a5893db8..91d05d9226ff 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -23,7 +23,10 @@ constexpr uint32_t expHiReg = 0x0202;\n> constexpr uint32_t expLoReg = 0x0203;\n> constexpr uint32_t gainHiReg = 0x0204;\n> constexpr uint32_t gainLoReg = 0x0205;\n> -constexpr std::initializer_list registerList = { expHiReg, expLoReg, gainHiReg, gainLoReg };\n> +constexpr uint32_t frameLengthHiReg = 0x0340;\n> +constexpr uint32_t frameLengthLoReg = 0x0341;\n> +constexpr std::initializer_list registerList =\n> +\t{ expHiReg, expLoReg, gainHiReg, gainLoReg, frameLengthHiReg, frameLengthLoReg };\n>\n> class CamHelperImx477 : public CamHelper\n> {\n> @@ -81,6 +84,7 @@ void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap ®isters,\n>\n> \tdeviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> \tdeviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));\n> +\tdeviceStatus.frame_length = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg);\n>\n> \tmetadata.Set(\"device.status\", deviceStatus);\n> }\n> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h\n> index e2511d19b96d..7bb8852375bd 100644\n> --- a/src/ipa/raspberrypi/controller/device_status.h\n> +++ b/src/ipa/raspberrypi/controller/device_status.h\n> @@ -17,7 +17,7 @@\n>\n> struct DeviceStatus {\n> \tDeviceStatus()\n> -\t\t: shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0),\n> +\t\t: shutter_speed(std::chrono::seconds(0)), frame_length(0), analogue_gain(0.0),\n> \t\t lens_position(0.0), aperture(0.0), flash_intensity(0.0)\n> \t{\n> \t}\n> @@ -28,6 +28,7 @@ struct DeviceStatus {\n>\n> \t\tout << \"Exposure: \" << d.shutter_speed\n> \t\t << \" Gain: \" << d.analogue_gain\n> +\t\t << \" Frame Length: \" << d.frame_length\n> \t\t << \" Aperture: \" << d.aperture\n> \t\t << \" Lens: \" << d.lens_position\n> \t\t << \" Flash: \" << d.flash_intensity;\n> @@ -37,6 +38,8 @@ struct DeviceStatus {\n>\n> \t/* time shutter is open */\n> \tlibcamera::utils::Duration shutter_speed;\n> +\t/* frame length given in number of lines */\n> +\tuint32_t frame_length;\n> \tdouble analogue_gain;\n> \t/* 1.0/distance-in-metres, or 0 if unknown */\n> \tdouble lens_position;\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f51c970befb5..db103a885b7a 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -1015,9 +1015,11 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n>\n> \tint32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> \tint32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> +\tint32_t vblank = sensorControls.get(V4L2_CID_VBLANK).get();\n>\n> \tdeviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> \tdeviceStatus.analogue_gain = helper_->Gain(gainCode);\n> +\tdeviceStatus.frame_length = mode_.height + vblank;\n>\n> \tLOG(IPARPI, Debug) << \"Metadata - \" << deviceStatus;\n>\n> --\n> 2.25.1\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 B99AABD794\n\tfor ;\n\tFri, 9 Jul 2021 16:14:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BDD66851B;\n\tFri, 9 Jul 2021 18:14:47 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8E0E9605AC\n\tfor ;\n\tFri, 9 Jul 2021 18:14:46 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 1AA0D1C0007;\n\tFri, 9 Jul 2021 16:14:45 +0000 (UTC)"],"Date":"Fri, 9 Jul 2021 18:15:34 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210709161534.emoc35b6skeyrn7a@uno.localdomain>","References":"<20210709145825.2943443-1-naush@raspberrypi.com>\n\t<20210709145825.2943443-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210709145825.2943443-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v6 4/8] ipa: raspberrypi: Add\n\tframe_length to DeviceStatus","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18079,"web_url":"https://patchwork.libcamera.org/comment/18079/","msgid":"","date":"2021-07-11T18:58:12","subject":"Re: [libcamera-devel] [PATCH v6 4/8] ipa: raspberrypi: Add\n\tframe_length to DeviceStatus","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Fri, Jul 09, 2021 at 03:58:21PM +0100, Naushir Patuck wrote:\n> Update the IPA to fill frame length into the DeviceStatus struct from the\n> VBLANK Control value passed from DelayedControls.\n\nThat's only for the case where no embedded data is present, right ? The\nfollowing commit message would seem clearer to me:\n\nStore the frame length into the DeviceStatus struct. The value is\nextracted from embedded data when available, or calculated from the\nVBLANK value passed from DelayedControls otherwise.\n\n> Update imx477 and imx219 CamHelper classes to extract the frame length from the\n> embedded data buffer.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Reviewed-by: Kieran Bingham \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 1 +\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +++++-\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 6 +++++-\n> src/ipa/raspberrypi/controller/device_status.h | 5 ++++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 2 ++\n> 5 files changed, 17 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 1ec3f03e1aa3..3c6afce7572b 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -187,6 +187,7 @@ void CamHelper::parseEmbeddedData(Span buffer,\n> \n> \tdeviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> \tdeviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> +\tdeviceStatus.frame_length = parsedDeviceStatus.frame_length;\n> \n> \tLOG(IPARPI, Debug) << \"Metadata updated - \" << deviceStatus;\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index 4d68e01fce71..a3caab714602 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -30,7 +30,10 @@ using namespace RPiController;\n> constexpr uint32_t gainReg = 0x157;\n> constexpr uint32_t expHiReg = 0x15a;\n> constexpr uint32_t expLoReg = 0x15b;\n> -constexpr std::initializer_list registerList [[maybe_unused]] = { expHiReg, expLoReg, gainReg };\n> +constexpr uint32_t frameLengthHiReg = 0x160;\n> +constexpr uint32_t frameLengthLoReg = 0x161;\n> +constexpr std::initializer_list registerList [[maybe_unused]]\n> +\t= { expHiReg, expLoReg, gainReg, frameLengthHiReg, frameLengthLoReg };\n> \n> class CamHelperImx219 : public CamHelper\n> {\n> @@ -93,6 +96,7 @@ void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap ®isters,\n> \n> \tdeviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> \tdeviceStatus.analogue_gain = Gain(registers.at(gainReg));\n> +\tdeviceStatus.frame_length = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg);\n> \n> \tmetadata.Set(\"device.status\", deviceStatus);\n> }\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index efd1a5893db8..91d05d9226ff 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -23,7 +23,10 @@ constexpr uint32_t expHiReg = 0x0202;\n> constexpr uint32_t expLoReg = 0x0203;\n> constexpr uint32_t gainHiReg = 0x0204;\n> constexpr uint32_t gainLoReg = 0x0205;\n> -constexpr std::initializer_list registerList = { expHiReg, expLoReg, gainHiReg, gainLoReg };\n> +constexpr uint32_t frameLengthHiReg = 0x0340;\n> +constexpr uint32_t frameLengthLoReg = 0x0341;\n> +constexpr std::initializer_list registerList =\n> +\t{ expHiReg, expLoReg, gainHiReg, gainLoReg, frameLengthHiReg, frameLengthLoReg };\n> \n> class CamHelperImx477 : public CamHelper\n> {\n> @@ -81,6 +84,7 @@ void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap ®isters,\n> \n> \tdeviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> \tdeviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));\n> +\tdeviceStatus.frame_length = registers.at(frameLengthHiReg) * 256 + registers.at(frameLengthLoReg);\n> \n> \tmetadata.Set(\"device.status\", deviceStatus);\n> }\n> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h\n> index e2511d19b96d..7bb8852375bd 100644\n> --- a/src/ipa/raspberrypi/controller/device_status.h\n> +++ b/src/ipa/raspberrypi/controller/device_status.h\n> @@ -17,7 +17,7 @@\n> \n> struct DeviceStatus {\n> \tDeviceStatus()\n> -\t\t: shutter_speed(std::chrono::seconds(0)), analogue_gain(0.0),\n> +\t\t: shutter_speed(std::chrono::seconds(0)), frame_length(0), analogue_gain(0.0),\n\nA line wrap may be nice.\n\nReviewed-by: Laurent Pinchart \n\n> \t\t lens_position(0.0), aperture(0.0), flash_intensity(0.0)\n> \t{\n> \t}\n> @@ -28,6 +28,7 @@ struct DeviceStatus {\n> \n> \t\tout << \"Exposure: \" << d.shutter_speed\n> \t\t << \" Gain: \" << d.analogue_gain\n> +\t\t << \" Frame Length: \" << d.frame_length\n> \t\t << \" Aperture: \" << d.aperture\n> \t\t << \" Lens: \" << d.lens_position\n> \t\t << \" Flash: \" << d.flash_intensity;\n> @@ -37,6 +38,8 @@ struct DeviceStatus {\n> \n> \t/* time shutter is open */\n> \tlibcamera::utils::Duration shutter_speed;\n> +\t/* frame length given in number of lines */\n> +\tuint32_t frame_length;\n> \tdouble analogue_gain;\n> \t/* 1.0/distance-in-metres, or 0 if unknown */\n> \tdouble lens_position;\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index f51c970befb5..db103a885b7a 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -1015,9 +1015,11 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)\n> \n> \tint32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get();\n> \tint32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> +\tint32_t vblank = sensorControls.get(V4L2_CID_VBLANK).get();\n> \n> \tdeviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> \tdeviceStatus.analogue_gain = helper_->Gain(gainCode);\n> +\tdeviceStatus.frame_length = mode_.height + vblank;\n> \n> \tLOG(IPARPI, Debug) << \"Metadata - \" << deviceStatus;\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 BE3E4C3224\n\tfor ;\n\tSun, 11 Jul 2021 18:59:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 48B976851D;\n\tSun, 11 Jul 2021 20:59:00 +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 EAEEB68519\n\tfor ;\n\tSun, 11 Jul 2021 20:58:58 +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 616A8CC;\n\tSun, 11 Jul 2021 20:58:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IFWMFFiD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626029938;\n\tbh=NsLnK3zB8HrD1MTiVSzJex1+/JIrHe2sWGs1tl0SB8U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IFWMFFiDomMv6DF9hMYywbUxXaadF53VBCV3rlCXlu2MDDp65nBpWtzEpMGBAzcL1\n\tYcPO/vCvZgCCie6Re1egRIHFq/86Ylbfo2h1E2IIKTgm3XnqHzl2LbsudKQ/NEaTIC\n\t2vI5h14ATaHUb0H2LTaB6rrzCQ024vQU9jqJue4M=","Date":"Sun, 11 Jul 2021 21:58:12 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210709145825.2943443-1-naush@raspberrypi.com>\n\t<20210709145825.2943443-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210709145825.2943443-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v6 4/8] ipa: raspberrypi: Add\n\tframe_length to DeviceStatus","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]