[{"id":21826,"web_url":"https://patchwork.libcamera.org/comment/21826/","msgid":"","date":"2021-12-20T07:41:37","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: raspberrypi: Add camera\n\thelper for Sony IMX296 sensor","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your work.\n\nOn Sun, 19 Dec 2021 at 23:27, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> From: Naushir Patuck \n>\n> The Sony IMX296 sensor has an exponential gain model, and adds a fixed\n> 14.26µs offset to the exposure time expressed in line units.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/raspberrypi/cam_helper_imx296.cpp | 126 ++++++++++++++++++++++\n> src/ipa/raspberrypi/meson.build | 1 +\n> 2 files changed, 127 insertions(+)\n> create mode 100644 src/ipa/raspberrypi/cam_helper_imx296.cpp\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> new file mode 100644\n> index 000000000000..9c34582861f1\n> --- /dev/null\n> +++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> @@ -0,0 +1,126 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> + *\n> + * cam_helper_imx296.cpp - camera helper for imx296 sensor\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +/*\n> + * We have observed the imx296 embedded data stream randomly return junk\n> + * reister values. Do not rely on embedded data until this has been\n> resolved.\n> + */\n> +#define ENABLE_EMBEDDED_DATA 0\n>\n\nI'm afraid this may not be completely true :-)\n\nThis file was created by doing a search/replace from the imx219 cam helper.\nPerhaps it might be better to completely remove embedded data support\nuntil we have a chance to properly evaluate this functionality on the\nimx296?\n\nEither way,\n\nSigned-off-by: Naushir Patuck @raspberrypi.com>\nReviewed-by: Naushir Patuck @raspberrypi.com>\n\n\n\n> +\n> +#include \"cam_helper.hpp\"\n> +#if ENABLE_EMBEDDED_DATA\n> +#include \"md_parser.hpp\"\n> +#endif\n> +\n> +//#include \"imx296_gain_table.hpp\"\n> +\n> +using namespace RPiController;\n> +using libcamera::utils::Duration;\n> +using namespace std::literals::chrono_literals;\n> +\n> +/*\n> + * We care about one gain register and a pair of exposure registers.\n> Their I2C\n> + * addresses from the Sony IMX296 datasheet:\n> + */\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> +\n> +class CamHelperImx296 : public CamHelper\n> +{\n> +public:\n> + CamHelperImx296();\n> + uint32_t GainCode(double gain) const override;\n> + double Gain(uint32_t gain_code) const override;\n> + uint32_t ExposureLines(Duration exposure) const override;\n> + Duration Exposure(uint32_t exposure_lines) const override;\n> + unsigned int MistrustFramesModeSwitch() const override;\n> + bool SensorEmbeddedDataPresent() const override;\n> +\n> +private:\n> + static constexpr uint32_t maxGainCode = 239;\n> + static constexpr Duration timePerLine = 550.0 / 37.125e6 * 1.0s;\n> +\n> + /*\n> + * Smallest difference between the frame length and integration\n> time,\n> + * in units of lines.\n> + */\n> + static constexpr int frameIntegrationDiff = 4;\n> +\n> + void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> + Metadata &metadata) const override;\n> +};\n> +\n> +CamHelperImx296::CamHelperImx296()\n> +#if ENABLE_EMBEDDED_DATA\n> + : CamHelper(std::make_unique(registerList),\n> frameIntegrationDiff)\n> +#else\n> + : CamHelper(nullptr, frameIntegrationDiff)\n> +#endif\n> +{\n> +}\n> +\n> +uint32_t CamHelperImx296::GainCode(double gain) const\n> +{\n> + uint32_t code = 20 * std::log10(gain) * 10;\n> + return std::min(code, maxGainCode);\n> +}\n> +\n> +double CamHelperImx296::Gain(uint32_t gain_code) const\n> +{\n> + return std::pow(10.0, gain_code / 200.0);\n> +}\n> +\n> +uint32_t CamHelperImx296::ExposureLines(Duration exposure) const\n> +{\n> + return (exposure - 14.26us) / timePerLine;\n> +}\n> +\n> +Duration CamHelperImx296::Exposure(uint32_t exposure_lines) const\n> +{\n> + return exposure_lines * timePerLine + 14.26us;\n> +}\n> +\n> +unsigned int CamHelperImx296::MistrustFramesModeSwitch() const\n> +{\n> + /*\n> + * For reasons unknown, we do occasionally get a bogus metadata\n> frame\n> + * at a mode switch (though not at start-up). Possibly warrants\n> some\n> + * investigation, though not a big deal.\n> + */\n> + return 1;\n>\n\nThis may also be something to evaluate (assuming metadata gets enabled).\n\n\n> +}\n> +\n> +bool CamHelperImx296::SensorEmbeddedDataPresent() const\n> +{\n> + return ENABLE_EMBEDDED_DATA;\n> +}\n> +\n> +void CamHelperImx296::PopulateMetadata(const MdParser::RegisterMap\n> ®isters,\n> + Metadata &metadata) const\n> +{\n> + DeviceStatus deviceStatus;\n> +\n> + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) *\n> 256 + registers.at(expLoReg));\n> + deviceStatus.analogue_gain = Gain(registers.at(gainReg));\n> +\n> + metadata.Set(\"device.status\", deviceStatus);\n> +}\n> +\n> +static CamHelper *Create()\n> +{\n> + return new CamHelperImx296();\n> +}\n> +\n> +static RegisterCamHelper reg(\"imx296\", &Create);\n> diff --git a/src/ipa/raspberrypi/meson.build\n> b/src/ipa/raspberrypi/meson.build\n> index 176055f42248..32897e07dad9 100644\n> --- a/src/ipa/raspberrypi/meson.build\n> +++ b/src/ipa/raspberrypi/meson.build\n> @@ -21,6 +21,7 @@ rpi_ipa_sources = files([\n> 'cam_helper_ov5647.cpp',\n> 'cam_helper_imx219.cpp',\n> 'cam_helper_imx290.cpp',\n> + 'cam_helper_imx296.cpp',\n> 'cam_helper_imx477.cpp',\n> 'cam_helper_imx519.cpp',\n> 'cam_helper_ov9281.cpp',\n> --\n> Regards,\n>\n> Laurent Pinchart\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 ADA28BE080\n\tfor ;\n\tMon, 20 Dec 2021 07:41:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C84FD60894;\n\tMon, 20 Dec 2021 08:41:56 +0100 (CET)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 91FBA60115\n\tfor ;\n\tMon, 20 Dec 2021 08:41:55 +0100 (CET)","by mail-lf1-x12d.google.com with SMTP id o12so421109lfk.1\n\tfor ;\n\tSun, 19 Dec 2021 23:41:55 -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=\"j5r+S5GQ\"; 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=ybt3oF8y+PxWcNAui5HpTTBdRAk2U2RktUQf7IKLBWE=;\n\tb=j5r+S5GQbiMl/e+1NKMjqLVAFF8MQDFa2xy59tCO+M2TFZK9FnNxG1Tc0JtfeqZm/b\n\tgwptc1gbPKVLciHyrpEOcfAZdOwjeP6scHWDOZ6U7275O/KteJe4j9QMC8PhpzWMOYQs\n\tzLDvrNgsjAyZTBtqzrILuHJP+uzFLzDE3Pfp7oOVVs99Ul4yOybL7RxENbTROYBaShDq\n\t0VBnHDdKW/C8kZHIZljyMEmu5eh7TVr0f5ieICuf1Ahc8E7Dc/XzXp721b39Vgue+E2I\n\tZ1ufaI4wb66VGHbjaSSkzLEmBmwzKHXBtXcDBbVKmy/7vHrGE41zF/8mnqAfnKak4TwX\n\tLjmw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=ybt3oF8y+PxWcNAui5HpTTBdRAk2U2RktUQf7IKLBWE=;\n\tb=vtzf0/tB47Q0/yD0UEml1TOIy/RBvhiKEKBnKBPuxGHhRifFUboYLvD0Nzz4IBuAlA\n\tcm/u0xsksagbk1eNkHjpl0OEyqsw5peqaimIG//AjOZzLMSp0TMiQJTy367TS2WMU+Zh\n\tqQZ+ZN3nXvi8KBtsEgQqJ6EXOBLEif9RoLwxcpamC5DsjWfi8aDiDmr6eAfNFWYtRW5C\n\tbZDpmCIm4dbIyhDhBhY1SyMPJq9QaWlIkRHZMJC4cA9279MfkwMRfXrO/iTrF3tOtKt8\n\t2o6YjUOlTIsD0v3FzL2MBzL3UpfzfYVzolPsgr+7J+80oLHoe8JR0LRGmk9CrowVbbCe\n\tOl4g==","X-Gm-Message-State":"AOAM530j8s2dn/s1Zk5fflgDW45rcutZq8zARy0/CwVeCUth6GX33xzN\n\tOy0Yb9bjL8iLSYHNOwmX9Dx6Qpn1NQoa8WpYT9a0XgAy8zZAfQ==","X-Google-Smtp-Source":"ABdhPJwGmeHmVfQpTYQJbD29oLUMq9ncslo3gfxeKczzliqI5UoiSoogE2X8hHOwuqCRZJ1ZjF6CqDSOolzYdIPMuaw=","X-Received":"by 2002:a05:6512:2204:: with SMTP id\n\th4mr5739564lfu.315.1639986114891; \n\tSun, 19 Dec 2021 23:41:54 -0800 (PST)","MIME-Version":"1.0","References":"<20211219232714.11427-1-laurent.pinchart@ideasonboard.com>\n\t<20211219232714.11427-4-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20211219232714.11427-4-laurent.pinchart@ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 20 Dec 2021 07:41:37 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000d0a34205d38f05b5\"","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: raspberrypi: Add camera\n\thelper for Sony IMX296 sensor","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":21863,"web_url":"https://patchwork.libcamera.org/comment/21863/","msgid":"","date":"2021-12-21T23:32:58","subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: raspberrypi: Add camera\n\thelper for Sony IMX296 sensor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Mon, Dec 20, 2021 at 07:41:37AM +0000, Naushir Patuck wrote:\n> On Sun, 19 Dec 2021 at 23:27, Laurent Pinchart wrote:\n> \n> > From: Naushir Patuck \n> >\n> > The Sony IMX296 sensor has an exponential gain model, and adds a fixed\n> > 14.26µs offset to the exposure time expressed in line units.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/raspberrypi/cam_helper_imx296.cpp | 126 ++++++++++++++++++++++\n> > src/ipa/raspberrypi/meson.build | 1 +\n> > 2 files changed, 127 insertions(+)\n> > create mode 100644 src/ipa/raspberrypi/cam_helper_imx296.cpp\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> > b/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> > new file mode 100644\n> > index 000000000000..9c34582861f1\n> > --- /dev/null\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx296.cpp\n> > @@ -0,0 +1,126 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> > + *\n> > + * cam_helper_imx296.cpp - camera helper for imx296 sensor\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +/*\n> > + * We have observed the imx296 embedded data stream randomly return junk\n> > + * reister values. Do not rely on embedded data until this has been resolved.\n> > + */\n> > +#define ENABLE_EMBEDDED_DATA 0\n> \n> I'm afraid this may not be completely true :-)\n> \n> This file was created by doing a search/replace from the imx219 cam helper.\n> Perhaps it might be better to completely remove embedded data support\n> until we have a chance to properly evaluate this functionality on the\n> imx296?\n\nSounds good to me. I'll drop it.\n\n> Either way,\n> \n> Signed-off-by: Naushir Patuck @raspberrypi.com>\n> Reviewed-by: Naushir Patuck @raspberrypi.com>\n> \n> > +\n> > +#include \"cam_helper.hpp\"\n> > +#if ENABLE_EMBEDDED_DATA\n> > +#include \"md_parser.hpp\"\n> > +#endif\n> > +\n> > +//#include \"imx296_gain_table.hpp\"\n> > +\n> > +using namespace RPiController;\n> > +using libcamera::utils::Duration;\n> > +using namespace std::literals::chrono_literals;\n> > +\n> > +/*\n> > + * We care about one gain register and a pair of exposure registers. Their I2C\n> > + * addresses from the Sony IMX296 datasheet:\n> > + */\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> > +\n> > +class CamHelperImx296 : public CamHelper\n> > +{\n> > +public:\n> > + CamHelperImx296();\n> > + uint32_t GainCode(double gain) const override;\n> > + double Gain(uint32_t gain_code) const override;\n> > + uint32_t ExposureLines(Duration exposure) const override;\n> > + Duration Exposure(uint32_t exposure_lines) const override;\n> > + unsigned int MistrustFramesModeSwitch() const override;\n> > + bool SensorEmbeddedDataPresent() const override;\n> > +\n> > +private:\n> > + static constexpr uint32_t maxGainCode = 239;\n> > + static constexpr Duration timePerLine = 550.0 / 37.125e6 * 1.0s;\n> > +\n> > + /*\n> > + * Smallest difference between the frame length and integration time,\n> > + * in units of lines.\n> > + */\n> > + static constexpr int frameIntegrationDiff = 4;\n> > +\n> > + void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > + Metadata &metadata) const override;\n> > +};\n> > +\n> > +CamHelperImx296::CamHelperImx296()\n> > +#if ENABLE_EMBEDDED_DATA\n> > + : CamHelper(std::make_unique(registerList),\n> > frameIntegrationDiff)\n> > +#else\n> > + : CamHelper(nullptr, frameIntegrationDiff)\n> > +#endif\n> > +{\n> > +}\n> > +\n> > +uint32_t CamHelperImx296::GainCode(double gain) const\n> > +{\n> > + uint32_t code = 20 * std::log10(gain) * 10;\n> > + return std::min(code, maxGainCode);\n> > +}\n> > +\n> > +double CamHelperImx296::Gain(uint32_t gain_code) const\n> > +{\n> > + return std::pow(10.0, gain_code / 200.0);\n> > +}\n> > +\n> > +uint32_t CamHelperImx296::ExposureLines(Duration exposure) const\n> > +{\n> > + return (exposure - 14.26us) / timePerLine;\n> > +}\n> > +\n> > +Duration CamHelperImx296::Exposure(uint32_t exposure_lines) const\n> > +{\n> > + return exposure_lines * timePerLine + 14.26us;\n> > +}\n> > +\n> > +unsigned int CamHelperImx296::MistrustFramesModeSwitch() const\n> > +{\n> > + /*\n> > + * For reasons unknown, we do occasionally get a bogus metadata frame\n> > + * at a mode switch (though not at start-up). Possibly warrants some\n> > + * investigation, though not a big deal.\n> > + */\n> > + return 1;\n> \n> This may also be something to evaluate (assuming metadata gets enabled).\n\nI'll drop it too.\n\n> > +}\n> > +\n> > +bool CamHelperImx296::SensorEmbeddedDataPresent() const\n> > +{\n> > + return ENABLE_EMBEDDED_DATA;\n> > +}\n> > +\n> > +void CamHelperImx296::PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > + Metadata &metadata) const\n> > +{\n> > + DeviceStatus deviceStatus;\n> > +\n> > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> > + deviceStatus.analogue_gain = Gain(registers.at(gainReg));\n> > +\n> > + metadata.Set(\"device.status\", deviceStatus);\n> > +}\n> > +\n> > +static CamHelper *Create()\n> > +{\n> > + return new CamHelperImx296();\n> > +}\n> > +\n> > +static RegisterCamHelper reg(\"imx296\", &Create);\n> > diff --git a/src/ipa/raspberrypi/meson.build\n> > b/src/ipa/raspberrypi/meson.build\n> > index 176055f42248..32897e07dad9 100644\n> > --- a/src/ipa/raspberrypi/meson.build\n> > +++ b/src/ipa/raspberrypi/meson.build\n> > @@ -21,6 +21,7 @@ rpi_ipa_sources = files([\n> > 'cam_helper_ov5647.cpp',\n> > 'cam_helper_imx219.cpp',\n> > 'cam_helper_imx290.cpp',\n> > + 'cam_helper_imx296.cpp',\n> > 'cam_helper_imx477.cpp',\n> > 'cam_helper_imx519.cpp',\n> > 'cam_helper_ov9281.cpp',","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 9E90BBE080\n\tfor ;\n\tTue, 21 Dec 2021 23:33:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C2CB7608F9;\n\tWed, 22 Dec 2021 00:33:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C75160223\n\tfor ;\n\tWed, 22 Dec 2021 00:33:02 +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 A6A6A894;\n\tWed, 22 Dec 2021 00:33:01 +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=\"IEO/eyxi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1640129581;\n\tbh=a9cnyaIa5qdiGrx3+2FkZHUgxZOqxA/RbXEyhJH07mg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IEO/eyxijahoYbjX4gbAx8doL5AVFpUKqF4WN3/DTvwXCjlN+hSJEE2uOkLv4lfFM\n\ty7gi8iBg3WyZkzawT3ZQbUeufjGRCc3LhcmCGqqs/7PgflHsHQ1V/sia7DIuxLl/pW\n\t46gu3mJG9mMt8fOtzB8/lm7aPDFGC4Rm3HSUHfrs=","Date":"Wed, 22 Dec 2021 01:32:58 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20211219232714.11427-1-laurent.pinchart@ideasonboard.com>\n\t<20211219232714.11427-4-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/4] ipa: raspberrypi: Add camera\n\thelper for Sony IMX296 sensor","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\" "}}]