[{"id":18049,"web_url":"https://patchwork.libcamera.org/comment/18049/","msgid":"<538d7cf2-1d1a-ec00-3386-0bec398652ac@ideasonboard.com>","date":"2021-07-09T13:17:11","subject":"Re: [libcamera-devel] [PATCH v5 4/8] ipa: raspberrypi: Add\n\tframe_length to DeviceStatus","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 09/07/2021 10:56, 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> ---\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\nis the maybe_unused still applicable?\nIf it's not used - what is the declaration for ?\n\n\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..916471ab258b 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\nIt's always better to be doing this in one place isn't it ;-)\n\n\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\nMixing C / C++ comment style here ?\n\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>","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 CECA6C3224\n\tfor ;\n\tFri, 9 Jul 2021 13:17:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 313D46851B;\n\tFri, 9 Jul 2021 15:17:15 +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 C1FFA605AC\n\tfor ;\n\tFri, 9 Jul 2021 15:17:13 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 55061E7;\n\tFri, 9 Jul 2021 15:17:13 +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=\"h8JQlD2Z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625836633;\n\tbh=pf+4om5ZaHNImUydKN2e02KyroY62gyr5B1jz6pN47Q=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=h8JQlD2Z6QHFhzdbSJmgjzvnpEgMqo+5Hgt6Yp4OwhavjBLldQPzEPF1jgIrVCao7\n\t0eGu7RPvG6MYdQLYGOeuzKqk/8B1hdXeP4tCCq2op5iybg/sbTswc0uN1zEZDDx4sk\n\t0loortojzI90QN3Qf0fj0jn3tqaYD40CERGZMaLE=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210709095638.2801713-1-naush@raspberrypi.com>\n\t<20210709095638.2801713-4-naush@raspberrypi.com>","From":"Kieran Bingham ","Message-ID":"<538d7cf2-1d1a-ec00-3386-0bec398652ac@ideasonboard.com>","Date":"Fri, 9 Jul 2021 14:17:11 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210709095638.2801713-4-naush@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v5 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18050,"web_url":"https://patchwork.libcamera.org/comment/18050/","msgid":"","date":"2021-07-09T13:27:36","subject":"Re: [libcamera-devel] [PATCH v5 4/8] ipa: raspberrypi: Add\n\tframe_length to DeviceStatus","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for your review feedback.\n\nOn Fri, 9 Jul 2021 at 14:17, Kieran Bingham \nwrote:\n\n> Hi Naush,\n>\n> On 09/07/2021 10:56, 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\n> from the\n> > embedded data buffer.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \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\n> 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 uint8_t> buffer,\n> >\n> > deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> > deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> > + deviceStatus.frame_length = parsedDeviceStatus.frame_length;\n> >\n> > LOG(IPARPI, Debug) << \"Metadata updated - \" << deviceStatus;\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> 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]]\n> = { expHiReg, expLoReg, gainReg };\n> > +constexpr uint32_t frameLengthHiReg = 0x160;\n> > +constexpr uint32_t frameLengthLoReg = 0x161;\n> > +constexpr std::initializer_list registerList [[maybe_unused]]\n>\n> is the maybe_unused still applicable?\n> If it's not used - what is the declaration for ?\n>\n\nYes it is still needed. There is a #define EMABLE_EMBEDDED_DATA switch\nat the top which disables the embedded data parsing path, which is\ncurrently set.\nWe have seen some problems with the imx219 embedded data values sometimes\nhaving junk values in them. This needs investigating/fixing before we can\nturn it\nback on again.\n\n\n>\n>\n> > + = { expHiReg, expLoReg, gainReg, frameLengthHiReg,\n> frameLengthLoReg };\n> >\n> > class CamHelperImx219 : public CamHelper\n> > {\n> > @@ -93,6 +96,7 @@ void CamHelperImx219::PopulateMetadata(const\n> MdParser::RegisterMap ®isters,\n> >\n> > deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) *\n> 256 + registers.at(expLoReg));\n> > deviceStatus.analogue_gain = Gain(registers.at(gainReg));\n> > + deviceStatus.frame_length = registers.at(frameLengthHiReg) * 256\n> + registers.at(frameLengthLoReg);\n> >\n> > metadata.Set(\"device.status\", deviceStatus);\n> > }\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> 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,\n> expLoReg, gainHiReg, gainLoReg };\n> > +constexpr uint32_t frameLengthHiReg = 0x0340;\n> > +constexpr uint32_t frameLengthLoReg = 0x0341;\n> > +constexpr std::initializer_list registerList =\n> > + { expHiReg, expLoReg, gainHiReg, gainLoReg, frameLengthHiReg,\n> frameLengthLoReg };\n> >\n> > class CamHelperImx477 : public CamHelper\n> > {\n> > @@ -81,6 +84,7 @@ void CamHelperImx477::PopulateMetadata(const\n> MdParser::RegisterMap ®isters,\n> >\n> > deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) *\n> 256 + registers.at(expLoReg));\n> > deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 +\n> registers.at(gainLoReg));\n> > + deviceStatus.frame_length = registers.at(frameLengthHiReg) * 256\n> + registers.at(frameLengthLoReg);\n> >\n> > metadata.Set(\"device.status\", deviceStatus);\n> > }\n> > diff --git a/src/ipa/raspberrypi/controller/device_status.h\n> b/src/ipa/raspberrypi/controller/device_status.h\n> > index e2511d19b96d..916471ab258b 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> > DeviceStatus()\n> > - : shutter_speed(std::chrono::seconds(0)),\n> analogue_gain(0.0),\n> > + : shutter_speed(std::chrono::seconds(0)), frame_length(0),\n> analogue_gain(0.0),\n> > lens_position(0.0), aperture(0.0), flash_intensity(0.0)\n> > {\n> > }\n> > @@ -28,6 +28,7 @@ struct DeviceStatus {\n> >\n> > out << \"Exposure: \" << d.shutter_speed\n> > << \" Gain: \" << d.analogue_gain\n> > + << \" Frame Length: \" << d.frame_length\n>\n> It's always better to be doing this in one place isn't it ;-)\n>\n>\n> > << \" Aperture: \" << d.aperture\n> > << \" Lens: \" << d.lens_position\n> > << \" Flash: \" << d.flash_intensity;\n> > @@ -37,6 +38,8 @@ struct DeviceStatus {\n> >\n> > /* time shutter is open */\n> > libcamera::utils::Duration shutter_speed;\n> > + // frame length given in number of lines\n>\n> Mixing C / C++ comment style here ?\n>\n\nArgh. David also pointed this out, and I forgot to add it in this revision.\nWill fix it in the next review :-)\n\nRegards,\nNaush\n\n\n>\n> > + uint32_t frame_length;\n> > double analogue_gain;\n> > /* 1.0/distance-in-metres, or 0 if unknown */\n> > double lens_position;\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> 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\n> &sensorControls)\n> >\n> > int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> > int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> > + int32_t vblank =\n> sensorControls.get(V4L2_CID_VBLANK).get();\n> >\n> > deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> > deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> > + deviceStatus.frame_length = mode_.height + vblank;\n> >\n> > LOG(IPARPI, Debug) << \"Metadata - \" << deviceStatus;\n> >\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 141A4BD794\n\tfor ;\n\tFri, 9 Jul 2021 13:27:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 794A8684E7;\n\tFri, 9 Jul 2021 15:27:54 +0200 (CEST)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C123B605AC\n\tfor ;\n\tFri, 9 Jul 2021 15:27:52 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id v14so23296326lfb.4\n\tfor ;\n\tFri, 09 Jul 2021 06:27:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"G2IDrAZQ\"; 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=Afi2afi6yssTkomREECGgK9pHKEhUaTkqI28KlTDa3s=;\n\tb=G2IDrAZQ+dNjRPLmHhVTkAqF9/4t8a34SuAHCP8zgMjzgJMMZbEyMnddtlq86w2jRM\n\tVYxzUwM6+5j0sbPMsBDur670GT0x+DXcEYEhz9u48/R4bx8SpdxO98jj097ubRQLfjYJ\n\tJn+jN4JcUmz5QhhTH+wr4ZMIFVsZQuhra6h4so5zHp4rKgM214vdFJ1O659F9ARDP6Ws\n\tzElaHWvZ4PfI+a6znBrpXPNB7jugKxUzQSnBQiRdQ4cnmiOS1TIy0tR1idx4jLNqHTEs\n\tgez5q8u7L1nG5dXx2cdllcATNds+M/tHAKob7Iq6tTTXWRhGE6XLePRHrZNrXEWMo06o\n\taiag==","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=Afi2afi6yssTkomREECGgK9pHKEhUaTkqI28KlTDa3s=;\n\tb=O/vj7j/Ue9Y2CrLl7J2ZzqCan+xte1uXtRAWqpbeghPYjtFbvJtqh3Z9uIjFXkjWRZ\n\tecPLZyLBfPQFmcF4dKvw2X0ySEzbnY4d/yO04PaHJjlTkFsSRRR5FGuoDbC2KFV7odwd\n\tiaevISeiPNAsGWQCzYGsaZDjmIp1f1Qf5LCKJ28vI6X7tAbwIRIq07f8/xqdbHDO5G7/\n\tSuEAdyyjU8BC1TpBc6lI+FcgszG0I5XlNVrMg4xINSydGVXO+jipoCXsshyfcZsyxn68\n\tJ2wb/Q2iEnr53peJnljR50ByvOZ8/cM4mGUiG95Hd//p5zNJVyCAcLRYrc3K/hNK1Y+j\n\tufMQ==","X-Gm-Message-State":"AOAM530unf4oWvAnG1psWVa8sZbuPKeQs1lFIjBxCxJ2Ir/+MmMJtjar\n\tO///I/Jg1xFa/CmxjjdSpiw3AmSUIgKKibMrX3ivHA==","X-Google-Smtp-Source":"ABdhPJxT3fD2b+B1lmCTl3eHYhn1ezvXAIxnTLZ9RlbwdO5ogxz3PjW1GMPk+nJdAnPC0UOli7lAAcKs8VRLUQRrz6M=","X-Received":"by 2002:a05:6512:3a1:: with SMTP id\n\tv1mr7163934lfp.171.1625837272066; \n\tFri, 09 Jul 2021 06:27:52 -0700 (PDT)","MIME-Version":"1.0","References":"<20210709095638.2801713-1-naush@raspberrypi.com>\n\t<20210709095638.2801713-4-naush@raspberrypi.com>\n\t<538d7cf2-1d1a-ec00-3386-0bec398652ac@ideasonboard.com>","In-Reply-To":"<538d7cf2-1d1a-ec00-3386-0bec398652ac@ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 9 Jul 2021 14:27:36 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"000000000000107fe205c6b0bde6\"","Subject":"Re: [libcamera-devel] [PATCH v5 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18052,"web_url":"https://patchwork.libcamera.org/comment/18052/","msgid":"<1091cc3e-4f05-b218-2b42-adbe7632c3bd@ideasonboard.com>","date":"2021-07-09T13:31:20","subject":"Re: [libcamera-devel] [PATCH v5 4/8] ipa: raspberrypi: Add\n\tframe_length to DeviceStatus","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 09/07/2021 14:27, Naushir Patuck wrote:\n> Hi Kieran,\n> \n> Thank you for your review feedback.\n> \n> On Fri, 9 Jul 2021 at 14:17, Kieran Bingham\n> > wrote:\n> \n> Hi Naush,\n> \n> On 09/07/2021 10:56, Naushir Patuck wrote:\n> > Update the IPA to fill frame length into the DeviceStatus struct\n> from the\n> > VBLANK Control value passed from DelayedControls.\n> >\n> > Update imx477 and imx219 CamHelper classes to extract the frame\n> length from the\n> > embedded data buffer.\n> >\n> > Signed-off-by: Naushir Patuck >\n> > Reviewed-by: David Plowman >\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\n> 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 uint8_t> buffer,\n> > \n> >       deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> >       deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> > +     deviceStatus.frame_length = parsedDeviceStatus.frame_length;\n> > \n> >       LOG(IPARPI, Debug) << \"Metadata updated - \" << deviceStatus;\n> > \n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> 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\n> [[maybe_unused]] = { expHiReg, expLoReg, gainReg };\n> > +constexpr uint32_t frameLengthHiReg = 0x160;\n> > +constexpr uint32_t frameLengthLoReg = 0x161;\n> > +constexpr std::initializer_list registerList\n> [[maybe_unused]]\n> \n> is the maybe_unused still applicable?\n> If it's not used - what is the declaration for ?\n> \n> \n> Yes it is still needed.  There is a #define EMABLE_EMBEDDED_DATA switch\n> at the top which disables the embedded data parsing path, which is\n> currently set.\n> We have seen some problems with the imx219 embedded data values sometimes\n> having junk values in them.  This needs investigating/fixing before we\n> can turn it\n> back on again.\n>  \n> \n> \n> \n> > +     = { expHiReg, expLoReg, gainReg, frameLengthHiReg,\n> frameLengthLoReg };\n> > \n> >  class CamHelperImx219 : public CamHelper\n> >  {\n> > @@ -93,6 +96,7 @@ void CamHelperImx219::PopulateMetadata(const\n> MdParser::RegisterMap ®isters,\n> > \n> >       deviceStatus.shutter_speed = Exposure(registers.at\n> (expHiReg) * 256 + registers.at\n> (expLoReg));\n> >       deviceStatus.analogue_gain = Gain(registers.at\n> (gainReg));\n> > +     deviceStatus.frame_length = registers.at\n> (frameLengthHiReg) * 256 + registers.at\n> (frameLengthLoReg);\n> > \n> >       metadata.Set(\"device.status\", deviceStatus);\n> >  }\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> 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 = {\n> expHiReg, expLoReg, gainHiReg, gainLoReg };\n> > +constexpr uint32_t frameLengthHiReg = 0x0340;\n> > +constexpr uint32_t frameLengthLoReg = 0x0341;\n> > +constexpr std::initializer_list registerList =\n> > +     { expHiReg, expLoReg, gainHiReg, gainLoReg,\n> frameLengthHiReg, frameLengthLoReg  };\n> > \n> >  class CamHelperImx477 : public CamHelper\n> >  {\n> > @@ -81,6 +84,7 @@ void CamHelperImx477::PopulateMetadata(const\n> MdParser::RegisterMap ®isters,\n> > \n> >       deviceStatus.shutter_speed = Exposure(registers.at\n> (expHiReg) * 256 + registers.at\n> (expLoReg));\n> >       deviceStatus.analogue_gain = Gain(registers.at\n> (gainHiReg) * 256 + registers.at\n> (gainLoReg));\n> > +     deviceStatus.frame_length = registers.at\n> (frameLengthHiReg) * 256 + registers.at\n> (frameLengthLoReg);\n> > \n> >       metadata.Set(\"device.status\", deviceStatus);\n> >  }\n> > diff --git a/src/ipa/raspberrypi/controller/device_status.h\n> b/src/ipa/raspberrypi/controller/device_status.h\n> > index e2511d19b96d..916471ab258b 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> >       DeviceStatus()\n> > -             : shutter_speed(std::chrono::seconds(0)),\n> analogue_gain(0.0),\n> > +             : shutter_speed(std::chrono::seconds(0)),\n> frame_length(0), analogue_gain(0.0),\n> >                 lens_position(0.0), aperture(0.0),\n> flash_intensity(0.0)\n> >       {\n> >       }\n> > @@ -28,6 +28,7 @@ struct DeviceStatus {\n> > \n> >               out << \"Exposure: \" << d.shutter_speed\n> >                   << \" Gain: \" << d.analogue_gain\n> > +                 << \" Frame Length: \" << d.frame_length\n> \n> It's always better to be doing this in one place isn't it ;-)\n> \n> \n> >                   << \" Aperture: \" << d.aperture\n> >                   << \" Lens: \" << d.lens_position\n> >                   << \" Flash: \" << d.flash_intensity;\n> > @@ -37,6 +38,8 @@ struct DeviceStatus {\n> > \n> >       /* time shutter is open */\n> >       libcamera::utils::Duration shutter_speed;\n> > +     // frame length given in number of lines\n> \n> Mixing C / C++ comment style here ?\n> \n> \n> Argh.  David also pointed this out, and I forgot to add it in this revision.\n> Will fix it in the next review :-)\n> \n\nAdd\n\nReviewed-by: Kieran Bingham when it's\ndone too ;-)\n\n\n> Regards,\n> Naush\n>  \n> \n> \n> > +     uint32_t frame_length;\n> >       double analogue_gain;\n> >       /* 1.0/distance-in-metres, or 0 if unknown */\n> >       double lens_position;\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> 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\n> ControlList &sensorControls)\n> > \n> >       int32_t exposureLines =\n> sensorControls.get(V4L2_CID_EXPOSURE).get();\n> >       int32_t gainCode =\n> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> > +     int32_t vblank =\n> sensorControls.get(V4L2_CID_VBLANK).get();\n> > \n> >       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);\n> >       deviceStatus.analogue_gain = helper_->Gain(gainCode);\n> > +     deviceStatus.frame_length = mode_.height + vblank;\n> > \n> >       LOG(IPARPI, Debug) << \"Metadata - \" << deviceStatus;\n> > \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 6F8B6C3224\n\tfor ;\n\tFri, 9 Jul 2021 13:31:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2AE056851B;\n\tFri, 9 Jul 2021 15:31:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F900605AC\n\tfor ;\n\tFri, 9 Jul 2021 15:31:23 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4BF3FE7;\n\tFri, 9 Jul 2021 15:31:23 +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=\"i+vZFlye\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625837483;\n\tbh=n3Xn9hBYB3vif7zfIS9r937mO7tEpGxtxz2xCLTpHfE=;\n\th=From:Subject:To:Cc:References:Date:In-Reply-To:From;\n\tb=i+vZFlyeHVIU0DMsWzyOa5yYEy3hdb3Z1VXZ+5W4356Z/iR+/1nKyVbAeu11PAHv0\n\t1tjbGjpUvkpeYgOsb4GHHs82VrTbfGpg1d6YpC6C1FZxCYuplorJ22QEk3y+QIkzJi\n\t02kpuelMpCvl7mb7xyeK89DaWqnWmoNvdNMMSUgc=","From":"Kieran Bingham ","To":"Naushir Patuck ","References":"<20210709095638.2801713-1-naush@raspberrypi.com>\n\t<20210709095638.2801713-4-naush@raspberrypi.com>\n\t<538d7cf2-1d1a-ec00-3386-0bec398652ac@ideasonboard.com>\n\t","Message-ID":"<1091cc3e-4f05-b218-2b42-adbe7632c3bd@ideasonboard.com>","Date":"Fri, 9 Jul 2021 14:31:20 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v5 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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]