[{"id":17914,"web_url":"https://patchwork.libcamera.org/comment/17914/","msgid":"","date":"2021-06-29T13:53:50","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Tue, 29 Jun 2021 at 11:45, Naushir Patuck wrote:\n\n> Instead of having each CamHelper subclass the MdParserSmia, change the\n> implementation of MdParserSmia to be more generic. The MdParserSmia now\n> gets\n> given a list of registers to search for and helper functions are used to\n> compute\n> exposure lines and gain codes from these registers.\n>\n> Update the imx219 and imx477 CamHelpers by using this new mechanism.\n>\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 33 +++---\n> src/ipa/raspberrypi/cam_helper.hpp | 2 +\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------\n> src/ipa/raspberrypi/md_parser.hpp | 41 +++++---\n> src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--\n> 6 files changed, 144 insertions(+), 234 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> index 90498c37af98..e6d2258c66d7 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -159,32 +159,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch()\n> const\n> void CamHelper::parseEmbeddedData(Span buffer,\n> Metadata &metadata)\n> {\n> + MdParser::RegisterMap registers;\n> + Metadata parsedMetadata;\n> +\n> if (buffer.empty())\n> return;\n>\n> - uint32_t exposureLines, gainCode;\n> -\n> - if (parser_->Parse(buffer) != MdParser::Status::OK ||\n> - parser_->GetExposureLines(exposureLines) !=\n> MdParser::Status::OK ||\n> - parser_->GetGainCode(gainCode) != MdParser::Status::OK) {\n> + if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {\n> LOG(IPARPI, Error) << \"Embedded data buffer parsing\n> failed\";\n> return;\n> }\n>\n> + PopulateMetadata(registers, parsedMetadata);\n> + metadata.Merge(parsedMetadata);\n> +\n> /*\n> - * Overwrite the exposure/gain values in the DeviceStatus, as\n> - * we know better. Fetch it first in case any other fields were\n> - * set meaningfully.\n> + * Overwrite the exposure/gain values in the existing DeviceStatus\n> with\n> + * values from the parsed embedded buffer. Fetch it first in case\n> any\n> + * other fields were set meaningfully.\n> */\n> - DeviceStatus deviceStatus;\n> -\n> - if (metadata.Get(\"device.status\", deviceStatus) != 0) {\n> + DeviceStatus deviceStatus, parsedDeviceStatus;\n> + if (metadata.Get(\"device.status\", deviceStatus) ||\n> + parsedMetadata.Get(\"device.status\", parsedDeviceStatus)) {\n> LOG(IPARPI, Error) << \"DeviceStatus not found\";\n> return;\n> }\n>\n> - deviceStatus.shutter_speed = Exposure(exposureLines);\n> - deviceStatus.analogue_gain = Gain(gainCode);\n> + deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> + deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n>\n> LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> << deviceStatus.shutter_speed\n> @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span\n> buffer,\n> metadata.Set(\"device.status\", deviceStatus);\n> }\n>\n> +void CamHelper::PopulateMetadata([[maybe_unused]] const\n> MdParser::RegisterMap ®isters,\n> + [[maybe_unused]] Metadata &metadata) const\n> +{\n> +}\n> +\n> RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> CamHelperCreateFunc create_func)\n> {\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> index fc3139e22be0..200cc83f3872 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -92,6 +92,8 @@ public:\n> protected:\n> void parseEmbeddedData(libcamera::Span buffer,\n> Metadata &metadata);\n> + virtual void PopulateMetadata(const MdParser::RegisterMap\n> ®isters,\n> + Metadata &metadata) const;\n>\n> std::unique_ptr parser_;\n> CameraMode mode_;\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index c85044a5fa6d..4e4994188ff3 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -23,21 +23,14 @@\n>\n> using namespace RPiController;\n>\n> -/* Metadata parser implementation specific to Sony IMX219 sensors. */\n> -\n> -class MdParserImx219 : public MdParserSmia\n> -{\n> -public:\n> - MdParserImx219();\n> - Status Parse(libcamera::Span buffer) override;\n> - Status GetExposureLines(unsigned int &lines) override;\n> - Status GetGainCode(unsigned int &gain_code) override;\n> -private:\n> - /* Offset of the register's value in the metadata block. */\n> - int reg_offsets_[3];\n> - /* Value of the register, once read from the metadata block. */\n> - int reg_values_[3];\n> -};\n> +/*\n> + * We care about one gain register and a pair of exposure registers.\n> Their I2C\n> + * addresses from the Sony IMX219 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 = { expHiReg,\n> expLoReg, gainReg };\n>\n> class CamHelperImx219 : public CamHelper\n> {\n> @@ -54,11 +47,14 @@ private:\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> CamHelperImx219::CamHelperImx219()\n> #if ENABLE_EMBEDDED_DATA\n> - : CamHelper(std::make_unique(),\n> frameIntegrationDiff)\n> + : CamHelper(std::make_unique(registerList),\n> frameIntegrationDiff)\n> #else\n> : CamHelper({}, frameIntegrationDiff)\n> #endif\n> @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const\n> return ENABLE_EMBEDDED_DATA;\n> }\n>\n> -static CamHelper *Create()\n> +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap\n> ®isters,\n> + Metadata &metadata) const\n> {\n> - return new CamHelperImx219();\n> -}\n> + DeviceStatus deviceStatus;\n>\n> -static RegisterCamHelper reg(\"imx219\", &Create);\n> + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) *\n> 256 + registers.at(expLoReg));\n> + deviceStatus.analogue_gain = Gain(registers.at(gainReg));\n>\n> -/*\n> - * We care about one gain register and a pair of exposure registers.\n> Their I2C\n> - * addresses from the Sony IMX219 datasheet:\n> - */\n> -#define GAIN_REG 0x157\n> -#define EXPHI_REG 0x15A\n> -#define EXPLO_REG 0x15B\n> -\n> -/*\n> - * Index of each into the reg_offsets and reg_values arrays. Must be in\n> - * register address order.\n> - */\n> -#define GAIN_INDEX 0\n> -#define EXPHI_INDEX 1\n> -#define EXPLO_INDEX 2\n> -\n> -MdParserImx219::MdParserImx219()\n> -{\n> - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> + metadata.Set(\"device.status\", deviceStatus);\n> }\n>\n> -MdParser::Status MdParserImx219::Parse(libcamera::Span\n> buffer)\n> -{\n> - bool try_again = false;\n> -\n> - if (reset_) {\n> - /*\n> - * Search again through the metadata for the gain and\n> exposure\n> - * registers.\n> - */\n> - assert(bits_per_pixel_);\n> - /* Need to be ordered */\n> - uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };\n> - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> - int ret = static_cast(findRegs(buffer,\n> - regs, reg_offsets_,\n> 3));\n> - /*\n> - * > 0 means \"worked partially but parse again next time\",\n> - * < 0 means \"hard error\".\n> - */\n> - if (ret > 0)\n> - try_again = true;\n> - else if (ret < 0)\n> - return ERROR;\n> - }\n> -\n> - for (int i = 0; i < 3; i++) {\n> - if (reg_offsets_[i] == -1)\n> - continue;\n> -\n> - reg_values_[i] = buffer[reg_offsets_[i]];\n> - }\n> -\n> - /* Re-parse next time if we were unhappy in some way. */\n> - reset_ = try_again;\n> -\n> - return OK;\n> -}\n> -\n> -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)\n> +static CamHelper *Create()\n> {\n> - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX]\n> == -1)\n> - return NOTFOUND;\n> -\n> - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> -\n> - return OK;\n> + return new CamHelperImx219();\n> }\n>\n> -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)\n> -{\n> - if (reg_offsets_[GAIN_INDEX] == -1)\n> - return NOTFOUND;\n> -\n> - gain_code = reg_values_[GAIN_INDEX];\n> -\n> - return OK;\n> -}\n> +static RegisterCamHelper reg(\"imx219\", &Create);\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index d72a9be0438e..efd1a5893db8 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -15,21 +15,15 @@\n>\n> using namespace RPiController;\n>\n> -/* Metadata parser implementation specific to Sony IMX477 sensors. */\n> -\n> -class MdParserImx477 : public MdParserSmia\n> -{\n> -public:\n> - MdParserImx477();\n> - Status Parse(libcamera::Span buffer) override;\n> - Status GetExposureLines(unsigned int &lines) override;\n> - Status GetGainCode(unsigned int &gain_code) override;\n> -private:\n> - /* Offset of the register's value in the metadata block. */\n> - int reg_offsets_[4];\n> - /* Value of the register, once read from the metadata block. */\n> - int reg_values_[4];\n> -};\n> +/*\n> + * We care about two gain registers and a pair of exposure registers.\n> Their\n> + * I2C addresses from the Sony IMX477 datasheet:\n> + */\n> +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>\n> class CamHelperImx477 : public CamHelper\n> {\n> @@ -47,10 +41,13 @@ private:\n> * in units of lines.\n> */\n> static constexpr int frameIntegrationDiff = 22;\n> +\n> + void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> + Metadata &metadata) const override;\n> };\n>\n> CamHelperImx477::CamHelperImx477()\n> - : CamHelper(std::make_unique(),\n> frameIntegrationDiff)\n> + : CamHelper(std::make_unique(registerList),\n> frameIntegrationDiff)\n> {\n> }\n>\n> @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const\n> return true;\n> }\n>\n> -static CamHelper *Create()\n> +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap\n> ®isters,\n> + Metadata &metadata) const\n> {\n> - return new CamHelperImx477();\n> -}\n> + DeviceStatus deviceStatus;\n>\n> -static RegisterCamHelper reg(\"imx477\", &Create);\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>\n> -/*\n> - * We care about two gain registers and a pair of exposure registers.\n> Their\n> - * I2C addresses from the Sony IMX477 datasheet:\n> - */\n> -#define EXPHI_REG 0x0202\n> -#define EXPLO_REG 0x0203\n> -#define GAINHI_REG 0x0204\n> -#define GAINLO_REG 0x0205\n> -\n> -/*\n> - * Index of each into the reg_offsets and reg_values arrays. Must be in\n> register\n> - * address order.\n> - */\n> -#define EXPHI_INDEX 0\n> -#define EXPLO_INDEX 1\n> -#define GAINHI_INDEX 2\n> -#define GAINLO_INDEX 3\n> -\n> -MdParserImx477::MdParserImx477()\n> -{\n> - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =\n> reg_offsets_[3] = -1;\n> + metadata.Set(\"device.status\", deviceStatus);\n> }\n>\n> -MdParser::Status MdParserImx477::Parse(libcamera::Span\n> buffer)\n> -{\n> - bool try_again = false;\n> -\n> - if (reset_) {\n> - /*\n> - * Search again through the metadata for the gain and\n> exposure\n> - * registers.\n> - */\n> - assert(bits_per_pixel_);\n> - /* Need to be ordered */\n> - uint32_t regs[4] = {\n> - EXPHI_REG,\n> - EXPLO_REG,\n> - GAINHI_REG,\n> - GAINLO_REG\n> - };\n> - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =\n> reg_offsets_[3] = -1;\n> - int ret = static_cast(findRegs(buffer,\n> - regs, reg_offsets_,\n> 4));\n> - /*\n> - * > 0 means \"worked partially but parse again next time\",\n> - * < 0 means \"hard error\".\n> - */\n> - if (ret > 0)\n> - try_again = true;\n> - else if (ret < 0)\n> - return ERROR;\n> - }\n> -\n> - for (int i = 0; i < 4; i++) {\n> - if (reg_offsets_[i] == -1)\n> - continue;\n> -\n> - reg_values_[i] = buffer[reg_offsets_[i]];\n> - }\n> -\n> - /* Re-parse next time if we were unhappy in some way. */\n> - reset_ = try_again;\n> -\n> - return OK;\n> -}\n> -\n> -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)\n> +static CamHelper *Create()\n> {\n> - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX]\n> == -1)\n> - return NOTFOUND;\n> -\n> - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> -\n> - return OK;\n> + return new CamHelperImx477();\n> }\n>\n> -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)\n> -{\n> - if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX]\n> == -1)\n> - return NOTFOUND;\n> -\n> - gain_code = reg_values_[GAINHI_INDEX] * 256 +\n> reg_values_[GAINLO_INDEX];\n> -\n> - return OK;\n> -}\n> +static RegisterCamHelper reg(\"imx477\", &Create);\n> diff --git a/src/ipa/raspberrypi/md_parser.hpp\n> b/src/ipa/raspberrypi/md_parser.hpp\n> index 8497216f8db7..e3e2738537a8 100644\n> --- a/src/ipa/raspberrypi/md_parser.hpp\n> +++ b/src/ipa/raspberrypi/md_parser.hpp\n> @@ -6,6 +6,9 @@\n> */\n> #pragma once\n>\n> +#include \n> +#include \n> +#include \n> #include \n>\n> #include \n> @@ -19,7 +22,7 @@\n> * application code doesn't have to worry which kind to instantiate. But\n> for\n> * the sake of example let's suppose we're parsing imx219 metadata.\n> *\n> - * MdParser *parser = new MdParserImx219(); // for example\n> + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });\n> * parser->SetBitsPerPixel(bpp);\n> * parser->SetLineLengthBytes(pitch);\n> * parser->SetNumLines(2);\n> @@ -32,13 +35,11 @@\n> *\n> * Then on every frame:\n> *\n> - * if (parser->Parse(buffer) != MdParser::OK)\n> + * RegisterMap registers;\n> + * if (parser->Parse(buffer, registers) != MdParser::OK)\n> * much badness;\n> - * unsigned int exposure_lines, gain_code\n> - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)\n> - * exposure was not found;\n> - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> - * gain code was not found;\n> + * Metadata metadata;\n> + * CamHelper::PopulateMetadata(registers, metadata);\n> *\n> * (Note that the CamHelper class converts to/from exposure lines and\n> time,\n> * and gain_code / actual gain.)\n> @@ -59,6 +60,8 @@ namespace RPiController {\n> class MdParser\n> {\n> public:\n> + using RegisterMap = std::map;\n> +\n> /*\n> * Parser status codes:\n> * OK - success\n> @@ -98,9 +101,8 @@ public:\n> line_length_bytes_ = num_bytes;\n> }\n>\n> - virtual Status Parse(libcamera::Span buffer) = 0;\n> - virtual Status GetExposureLines(unsigned int &lines) = 0;\n> - virtual Status GetGainCode(unsigned int &gain_code) = 0;\n> + virtual Status Parse(libcamera::Span buffer,\n> + RegisterMap ®isters) = 0;\n>\n> protected:\n> bool reset_;\n> @@ -116,14 +118,18 @@ protected:\n> * md_parser_imx219.cpp for an example).\n> */\n>\n> -class MdParserSmia : public MdParser\n> +class MdParserSmia final : public MdParser\n> {\n> public:\n> - MdParserSmia() : MdParser()\n> - {\n> - }\n> + MdParserSmia(std::initializer_list registerList);\n> +\n> + MdParser::Status Parse(libcamera::Span buffer,\n> + RegisterMap ®isters) override;\n> +\n> +private:\n> + /* Maps register address to offset in the buffer. */\n> + using OffsetMap = std::map>;\n>\n> -protected:\n> /*\n> * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> * indicate a bad data buffer. Status codes are:\n> @@ -141,8 +147,9 @@ protected:\n> BAD_PADDING = -5\n> };\n>\n> - ParseStatus findRegs(libcamera::Span buffer,\n> uint32_t regs[],\n> - int offsets[], unsigned int num_regs);\n> + ParseStatus findRegs(libcamera::Span buffer);\n> +\n> + OffsetMap offsets_;\n> };\n>\n> } // namespace RPi\n> diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp\n> b/src/ipa/raspberrypi/md_parser_smia.cpp\n> index 0a14875575a2..7f72fc03948f 100644\n> --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> @@ -6,9 +6,11 @@\n> */\n> #include \n>\n\nOops, this header is not needed any more!\n\n\n\n>\n> +#include \"libcamera/base/log.h\"\n> #include \"md_parser.hpp\"\n>\n> using namespace RPiController;\n> +using namespace libcamera;\n>\n> /*\n> * This function goes through the embedded data to find the offsets (not\n> @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;\n> constexpr unsigned int REG_VALUE = 0x5a;\n> constexpr unsigned int REG_SKIP = 0x55;\n>\n> -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span uint8_t> buffer,\n> - uint32_t regs[], int\n> offsets[],\n> - unsigned int num_regs)\n> +MdParserSmia::MdParserSmia(std::initializer_list registerList)\n> {\n> - assert(num_regs > 0);\n> + for (auto r : registerList)\n> + offsets_[r] = {};\n> +}\n> +\n> +MdParser::Status MdParserSmia::Parse(libcamera::Span\n> buffer,\n> + RegisterMap ®isters)\n> +{\n> + if (reset_) {\n> + /*\n> + * Search again through the metadata for all the registers\n> + * requested.\n> + */\n> + ASSERT(bits_per_pixel_);\n> +\n> + for (const auto &[reg, offset] : offsets_)\n> + offsets_[reg] = {};\n> +\n> + ParseStatus ret = findRegs(buffer);\n> + /*\n> + * > 0 means \"worked partially but parse again next time\",\n> + * < 0 means \"hard error\".\n> + *\n> + * In either case, we retry parsing on the next frame.\n> + */\n> + if (ret != PARSE_OK)\n> + return ERROR;\n> +\n> + reset_ = false;\n> + }\n> +\n> + /* Populate the register values requested. */\n> + registers.clear();\n> + for (const auto &[reg, offset] : offsets_) {\n> + if (!offset) {\n> + reset_ = true;\n> + return NOTFOUND;\n> + }\n> + registers[reg] = buffer[offset.value()];\n> + }\n> +\n> + return OK;\n> +}\n> +\n> +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span uint8_t> buffer)\n> +{\n> + ASSERT(offsets_.size());\n>\n> if (buffer[0] != LINE_START)\n> return NO_LINE_START;\n>\n> unsigned int current_offset = 1; /* after the LINE_START */\n> unsigned int current_line_start = 0, current_line = 0;\n> - unsigned int reg_num = 0, first_reg = 0;\n> + unsigned int reg_num = 0, regs_done = 0;\n>\n> while (1) {\n> int tag = buffer[current_offset++];\n> @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span\n> else if (tag == REG_SKIP)\n> reg_num++;\n> else if (tag == REG_VALUE) {\n> - while (reg_num >=\n> - /* assumes registers are in\n> order... */\n> - regs[first_reg]) {\n> - if (reg_num == regs[first_reg])\n> - offsets[first_reg] =\n> current_offset - 1;\n> + auto reg = offsets_.find(reg_num);\n> +\n> + if (reg != offsets_.end()) {\n> + offsets_[reg_num] = current_offset\n> - 1;\n>\n> - if (++first_reg == num_regs)\n> + if (++regs_done == offsets_.size())\n> return PARSE_OK;\n> }\n> reg_num++;\n> --\n> 2.25.1\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 E3366C321F\n\tfor ;\n\tTue, 29 Jun 2021 13:54:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3042A684EC;\n\tTue, 29 Jun 2021 15:54:09 +0200 (CEST)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7C91684CB\n\tfor ;\n\tTue, 29 Jun 2021 15:54:07 +0200 (CEST)","by mail-lj1-x22d.google.com with SMTP id d25so31190999lji.7\n\tfor ;\n\tTue, 29 Jun 2021 06:54:07 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"feuUtFC/\"; 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=LdZCnHsQn1ZPrVjcI/9WOrfp3TsPYBnLyUXfJMsRMeQ=;\n\tb=feuUtFC/q5+NoMPbBvmBidezUJyOjFS+Aji9unZ68xMO9gAzHQKAjf5ATdC5RSpe1R\n\tTynoSnDQshwz5C5nsIa8MwWIg3v/joOajKkf4vSr6cl4edLssLYn9seh6CZOCCNAzThh\n\t29qf0XZVp+5QH1Ll+C+LyqNFrMOPOohJ4mEsPy10FhcEkWZoIf3nJ94kUi7tqlf8L2yu\n\tnUoeD94OQaqNITDD2aPnz0Qch9/SXUFa359+pSKGAAiuW+OA+KEww05Cw55Won08Tzbj\n\tadnXwx22Dwlk9s/YfTfpNnIb4OCSBhnNbhA1sbKjpSlRp8NDQWVnh4Mb1A/q9QUhvnSO\n\tM8jg==","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=LdZCnHsQn1ZPrVjcI/9WOrfp3TsPYBnLyUXfJMsRMeQ=;\n\tb=G6WuR08+38TX7PSDY6nRevJrXMKF4xVVWFvbY5jBxjfYd0CRnKRBUfwgK3nL8m+bSN\n\tJ9OSSqi+FAvYMAEoTuFO9wPy/4vsjlpEmahkrGCIbpbpn40WdSXHw7ckFpQx7C0cy2Ee\n\tP9QKkLc2XKEKTyt7xhY72QzsCrJSzfBsAWMd8kDi+aQ5JkLWtOujxBtPrY41yCHUi5c4\n\tLZO22HCDt6mWdHP6LJP1RJyJMaSScfA4brC7zZuHkHlW21oDUOIboCHRHNI3picF5MCz\n\tQyP4JfV4ZDjOkKfTc94SAn4i5d3Q8ks4y5x1v8UjrSaLEG4i3sVCcZ1f600KmR4lX2yq\n\tis1g==","X-Gm-Message-State":"AOAM5312LTOxbhsSL4gMc7U39i5ySPQtmfzbZLiffmw2ybSacKJF8fCK\n\tTPb7G+icxzgXy0bp7Jcjyq25NTSfp+XLmfK4CmF5jPWrBiE=","X-Google-Smtp-Source":"ABdhPJzURCywkqCfvnxMqzPYF+sV338VsKrfEqvMpX9zNiX8YH1If/JqJM0+E5Njo19JVAN+ZTc6u+OUFvO1CmfVUF0=","X-Received":"by 2002:a2e:a706:: with SMTP id s6mr4108355lje.169.1624974846570;\n\tTue, 29 Jun 2021 06:54:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20210629104500.51672-1-naush@raspberrypi.com>\n\t<20210629104500.51672-3-naush@raspberrypi.com>","In-Reply-To":"<20210629104500.51672-3-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Tue, 29 Jun 2021 14:53:50 +0100","Message-ID":"","To":"libcamera devel ","Content-Type":"multipart/alternative; boundary=\"0000000000007fc63005c5e7f017\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","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":17918,"web_url":"https://patchwork.libcamera.org/comment/17918/","msgid":"","date":"2021-06-29T14:44:12","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Jun 29, 2021 at 02:53:50PM +0100, Naushir Patuck wrote:\n> On Tue, 29 Jun 2021 at 11:45, Naushir Patuck wrote:\n> \n> > Instead of having each CamHelper subclass the MdParserSmia, change the\n> > implementation of MdParserSmia to be more generic. The MdParserSmia now gets\n> > given a list of registers to search for and helper functions are used to compute\n> > exposure lines and gain codes from these registers.\n> >\n> > Update the imx219 and imx477 CamHelpers by using this new mechanism.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 33 +++---\n> > src/ipa/raspberrypi/cam_helper.hpp | 2 +\n> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------\n> > src/ipa/raspberrypi/md_parser.hpp | 41 +++++---\n> > src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--\n> > 6 files changed, 144 insertions(+), 234 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> > index 90498c37af98..e6d2258c66d7 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -159,32 +159,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n> > void CamHelper::parseEmbeddedData(Span buffer,\n> > Metadata &metadata)\n> > {\n> > + MdParser::RegisterMap registers;\n> > + Metadata parsedMetadata;\n> > +\n> > if (buffer.empty())\n> > return;\n> >\n> > - uint32_t exposureLines, gainCode;\n> > -\n> > - if (parser_->Parse(buffer) != MdParser::Status::OK ||\n> > - parser_->GetExposureLines(exposureLines) != MdParser::Status::OK ||\n> > - parser_->GetGainCode(gainCode) != MdParser::Status::OK) {\n> > + if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {\n> > LOG(IPARPI, Error) << \"Embedded data buffer parsing failed\";\n> > return;\n> > }\n> >\n> > + PopulateMetadata(registers, parsedMetadata);\n> > + metadata.Merge(parsedMetadata);\n> > +\n> > /*\n> > - * Overwrite the exposure/gain values in the DeviceStatus, as\n> > - * we know better. Fetch it first in case any other fields were\n> > - * set meaningfully.\n> > + * Overwrite the exposure/gain values in the existing DeviceStatus with\n> > + * values from the parsed embedded buffer. Fetch it first in case any\n> > + * other fields were set meaningfully.\n> > */\n> > - DeviceStatus deviceStatus;\n> > -\n> > - if (metadata.Get(\"device.status\", deviceStatus) != 0) {\n> > + DeviceStatus deviceStatus, parsedDeviceStatus;\n> > + if (metadata.Get(\"device.status\", deviceStatus) ||\n> > + parsedMetadata.Get(\"device.status\", parsedDeviceStatus)) {\n> > LOG(IPARPI, Error) << \"DeviceStatus not found\";\n> > return;\n> > }\n> >\n> > - deviceStatus.shutter_speed = Exposure(exposureLines);\n> > - deviceStatus.analogue_gain = Gain(gainCode);\n> > + deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> > + deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> >\n> > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > << deviceStatus.shutter_speed\n> > @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span buffer,\n> > metadata.Set(\"device.status\", deviceStatus);\n> > }\n> >\n> > +void CamHelper::PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap ®isters,\n> > + [[maybe_unused]] Metadata &metadata) const\n> > +{\n> > +}\n> > +\n> > RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> > CamHelperCreateFunc create_func)\n> > {\n> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> > index fc3139e22be0..200cc83f3872 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -92,6 +92,8 @@ public:\n> > protected:\n> > void parseEmbeddedData(libcamera::Span buffer,\n> > Metadata &metadata);\n> > + virtual void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > + Metadata &metadata) const;\n> >\n> > std::unique_ptr parser_;\n> > CameraMode mode_;\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > index c85044a5fa6d..4e4994188ff3 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > @@ -23,21 +23,14 @@\n> >\n> > using namespace RPiController;\n> >\n> > -/* Metadata parser implementation specific to Sony IMX219 sensors. */\n> > -\n> > -class MdParserImx219 : public MdParserSmia\n> > -{\n> > -public:\n> > - MdParserImx219();\n> > - Status Parse(libcamera::Span buffer) override;\n> > - Status GetExposureLines(unsigned int &lines) override;\n> > - Status GetGainCode(unsigned int &gain_code) override;\n> > -private:\n> > - /* Offset of the register's value in the metadata block. */\n> > - int reg_offsets_[3];\n> > - /* Value of the register, once read from the metadata block. */\n> > - int reg_values_[3];\n> > -};\n> > +/*\n> > + * We care about one gain register and a pair of exposure registers. Their I2C\n> > + * addresses from the Sony IMX219 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 = { expHiReg, expLoReg, gainReg };\n> >\n> > class CamHelperImx219 : public CamHelper\n> > {\n> > @@ -54,11 +47,14 @@ private:\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> > CamHelperImx219::CamHelperImx219()\n> > #if ENABLE_EMBEDDED_DATA\n> > - : CamHelper(std::make_unique(), frameIntegrationDiff)\n> > + : CamHelper(std::make_unique(registerList), frameIntegrationDiff)\n> > #else\n> > : CamHelper({}, frameIntegrationDiff)\n> > #endif\n> > @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const\n> > return ENABLE_EMBEDDED_DATA;\n> > }\n> >\n> > -static CamHelper *Create()\n> > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > + Metadata &metadata) const\n> > {\n> > - return new CamHelperImx219();\n> > -}\n> > + DeviceStatus deviceStatus;\n> >\n> > -static RegisterCamHelper reg(\"imx219\", &Create);\n> > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> > + deviceStatus.analogue_gain = Gain(registers.at(gainReg));\n> >\n> > -/*\n> > - * We care about one gain register and a pair of exposure registers. Their I2C\n> > - * addresses from the Sony IMX219 datasheet:\n> > - */\n> > -#define GAIN_REG 0x157\n> > -#define EXPHI_REG 0x15A\n> > -#define EXPLO_REG 0x15B\n> > -\n> > -/*\n> > - * Index of each into the reg_offsets and reg_values arrays. Must be in\n> > - * register address order.\n> > - */\n> > -#define GAIN_INDEX 0\n> > -#define EXPHI_INDEX 1\n> > -#define EXPLO_INDEX 2\n> > -\n> > -MdParserImx219::MdParserImx219()\n> > -{\n> > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > + metadata.Set(\"device.status\", deviceStatus);\n> > }\n> >\n> > -MdParser::Status MdParserImx219::Parse(libcamera::Span buffer)\n> > -{\n> > - bool try_again = false;\n> > -\n> > - if (reset_) {\n> > - /*\n> > - * Search again through the metadata for the gain and exposure\n> > - * registers.\n> > - */\n> > - assert(bits_per_pixel_);\n> > - /* Need to be ordered */\n> > - uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };\n> > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > - int ret = static_cast(findRegs(buffer,\n> > - regs, reg_offsets_, 3));\n> > - /*\n> > - * > 0 means \"worked partially but parse again next time\",\n> > - * < 0 means \"hard error\".\n> > - */\n> > - if (ret > 0)\n> > - try_again = true;\n> > - else if (ret < 0)\n> > - return ERROR;\n> > - }\n> > -\n> > - for (int i = 0; i < 3; i++) {\n> > - if (reg_offsets_[i] == -1)\n> > - continue;\n> > -\n> > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > - }\n> > -\n> > - /* Re-parse next time if we were unhappy in some way. */\n> > - reset_ = try_again;\n> > -\n> > - return OK;\n> > -}\n> > -\n> > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)\n> > +static CamHelper *Create()\n> > {\n> > - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> > - return NOTFOUND;\n> > -\n> > - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> > -\n> > - return OK;\n> > + return new CamHelperImx219();\n> > }\n> >\n> > -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)\n> > -{\n> > - if (reg_offsets_[GAIN_INDEX] == -1)\n> > - return NOTFOUND;\n> > -\n> > - gain_code = reg_values_[GAIN_INDEX];\n> > -\n> > - return OK;\n> > -}\n> > +static RegisterCamHelper reg(\"imx219\", &Create);\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index d72a9be0438e..efd1a5893db8 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -15,21 +15,15 @@\n> >\n> > using namespace RPiController;\n> >\n> > -/* Metadata parser implementation specific to Sony IMX477 sensors. */\n> > -\n> > -class MdParserImx477 : public MdParserSmia\n> > -{\n> > -public:\n> > - MdParserImx477();\n> > - Status Parse(libcamera::Span buffer) override;\n> > - Status GetExposureLines(unsigned int &lines) override;\n> > - Status GetGainCode(unsigned int &gain_code) override;\n> > -private:\n> > - /* Offset of the register's value in the metadata block. */\n> > - int reg_offsets_[4];\n> > - /* Value of the register, once read from the metadata block. */\n> > - int reg_values_[4];\n> > -};\n> > +/*\n> > + * We care about two gain registers and a pair of exposure registers. Their\n> > + * I2C addresses from the Sony IMX477 datasheet:\n> > + */\n> > +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> >\n> > class CamHelperImx477 : public CamHelper\n> > {\n> > @@ -47,10 +41,13 @@ private:\n> > * in units of lines.\n> > */\n> > static constexpr int frameIntegrationDiff = 22;\n> > +\n> > + void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > + Metadata &metadata) const override;\n> > };\n> >\n> > CamHelperImx477::CamHelperImx477()\n> > - : CamHelper(std::make_unique(), frameIntegrationDiff)\n> > + : CamHelper(std::make_unique(registerList), frameIntegrationDiff)\n> > {\n> > }\n> >\n> > @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const\n> > return true;\n> > }\n> >\n> > -static CamHelper *Create()\n> > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > + Metadata &metadata) const\n> > {\n> > - return new CamHelperImx477();\n> > -}\n> > + DeviceStatus deviceStatus;\n> >\n> > -static RegisterCamHelper reg(\"imx477\", &Create);\n> > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> > + deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));\n> >\n> > -/*\n> > - * We care about two gain registers and a pair of exposure registers. Their\n> > - * I2C addresses from the Sony IMX477 datasheet:\n> > - */\n> > -#define EXPHI_REG 0x0202\n> > -#define EXPLO_REG 0x0203\n> > -#define GAINHI_REG 0x0204\n> > -#define GAINLO_REG 0x0205\n> > -\n> > -/*\n> > - * Index of each into the reg_offsets and reg_values arrays. Must be in register\n> > - * address order.\n> > - */\n> > -#define EXPHI_INDEX 0\n> > -#define EXPLO_INDEX 1\n> > -#define GAINHI_INDEX 2\n> > -#define GAINLO_INDEX 3\n> > -\n> > -MdParserImx477::MdParserImx477()\n> > -{\n> > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> > + metadata.Set(\"device.status\", deviceStatus);\n> > }\n> >\n> > -MdParser::Status MdParserImx477::Parse(libcamera::Span buffer)\n> > -{\n> > - bool try_again = false;\n> > -\n> > - if (reset_) {\n> > - /*\n> > - * Search again through the metadata for the gain and exposure\n> > - * registers.\n> > - */\n> > - assert(bits_per_pixel_);\n> > - /* Need to be ordered */\n> > - uint32_t regs[4] = {\n> > - EXPHI_REG,\n> > - EXPLO_REG,\n> > - GAINHI_REG,\n> > - GAINLO_REG\n> > - };\n> > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> > - int ret = static_cast(findRegs(buffer,\n> > - regs, reg_offsets_, 4));\n> > - /*\n> > - * > 0 means \"worked partially but parse again next time\",\n> > - * < 0 means \"hard error\".\n> > - */\n> > - if (ret > 0)\n> > - try_again = true;\n> > - else if (ret < 0)\n> > - return ERROR;\n> > - }\n> > -\n> > - for (int i = 0; i < 4; i++) {\n> > - if (reg_offsets_[i] == -1)\n> > - continue;\n> > -\n> > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > - }\n> > -\n> > - /* Re-parse next time if we were unhappy in some way. */\n> > - reset_ = try_again;\n> > -\n> > - return OK;\n> > -}\n> > -\n> > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)\n> > +static CamHelper *Create()\n> > {\n> > - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> > - return NOTFOUND;\n> > -\n> > - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> > -\n> > - return OK;\n> > + return new CamHelperImx477();\n> > }\n> >\n> > -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)\n> > -{\n> > - if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX] == -1)\n> > - return NOTFOUND;\n> > -\n> > - gain_code = reg_values_[GAINHI_INDEX] * 256 +\n> > reg_values_[GAINLO_INDEX];\n> > -\n> > - return OK;\n> > -}\n> > +static RegisterCamHelper reg(\"imx477\", &Create);\n> > diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp\n> > index 8497216f8db7..e3e2738537a8 100644\n> > --- a/src/ipa/raspberrypi/md_parser.hpp\n> > +++ b/src/ipa/raspberrypi/md_parser.hpp\n> > @@ -6,6 +6,9 @@\n> > */\n> > #pragma once\n> >\n> > +#include \n> > +#include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -19,7 +22,7 @@\n> > * application code doesn't have to worry which kind to instantiate. But for\n> > * the sake of example let's suppose we're parsing imx219 metadata.\n> > *\n> > - * MdParser *parser = new MdParserImx219(); // for example\n> > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });\n> > * parser->SetBitsPerPixel(bpp);\n> > * parser->SetLineLengthBytes(pitch);\n> > * parser->SetNumLines(2);\n> > @@ -32,13 +35,11 @@\n> > *\n> > * Then on every frame:\n> > *\n> > - * if (parser->Parse(buffer) != MdParser::OK)\n> > + * RegisterMap registers;\n> > + * if (parser->Parse(buffer, registers) != MdParser::OK)\n> > * much badness;\n> > - * unsigned int exposure_lines, gain_code\n> > - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)\n> > - * exposure was not found;\n> > - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> > - * gain code was not found;\n> > + * Metadata metadata;\n> > + * CamHelper::PopulateMetadata(registers, metadata);\n> > *\n> > * (Note that the CamHelper class converts to/from exposure lines and time,\n> > * and gain_code / actual gain.)\n> > @@ -59,6 +60,8 @@ namespace RPiController {\n> > class MdParser\n> > {\n> > public:\n> > + using RegisterMap = std::map;\n> > +\n> > /*\n> > * Parser status codes:\n> > * OK - success\n> > @@ -98,9 +101,8 @@ public:\n> > line_length_bytes_ = num_bytes;\n> > }\n> >\n> > - virtual Status Parse(libcamera::Span buffer) = 0;\n> > - virtual Status GetExposureLines(unsigned int &lines) = 0;\n> > - virtual Status GetGainCode(unsigned int &gain_code) = 0;\n> > + virtual Status Parse(libcamera::Span buffer,\n> > + RegisterMap ®isters) = 0;\n> >\n> > protected:\n> > bool reset_;\n> > @@ -116,14 +118,18 @@ protected:\n> > * md_parser_imx219.cpp for an example).\n> > */\n> >\n> > -class MdParserSmia : public MdParser\n> > +class MdParserSmia final : public MdParser\n> > {\n> > public:\n> > - MdParserSmia() : MdParser()\n> > - {\n> > - }\n> > + MdParserSmia(std::initializer_list registerList);\n> > +\n> > + MdParser::Status Parse(libcamera::Span buffer,\n> > + RegisterMap ®isters) override;\n> > +\n> > +private:\n> > + /* Maps register address to offset in the buffer. */\n> > + using OffsetMap = std::map>;\n> >\n> > -protected:\n> > /*\n> > * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> > * indicate a bad data buffer. Status codes are:\n> > @@ -141,8 +147,9 @@ protected:\n> > BAD_PADDING = -5\n> > };\n> >\n> > - ParseStatus findRegs(libcamera::Span buffer, uint32_t regs[],\n> > - int offsets[], unsigned int num_regs);\n> > + ParseStatus findRegs(libcamera::Span buffer);\n> > +\n> > + OffsetMap offsets_;\n> > };\n> >\n> > } // namespace RPi\n> > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > index 0a14875575a2..7f72fc03948f 100644\n> > --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > @@ -6,9 +6,11 @@\n> > */\n> > #include \n> >\n> \n> Oops, this header is not needed any more!\n> \n> > +#include \"libcamera/base/log.h\"\n\nAnd this should be . I'll fix when applying.\n\n> > #include \"md_parser.hpp\"\n> >\n> > using namespace RPiController;\n> > +using namespace libcamera;\n> >\n> > /*\n> > * This function goes through the embedded data to find the offsets (not\n> > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;\n> > constexpr unsigned int REG_VALUE = 0x5a;\n> > constexpr unsigned int REG_SKIP = 0x55;\n> >\n> > -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer,\n> > - uint32_t regs[], int offsets[],\n> > - unsigned int num_regs)\n> > +MdParserSmia::MdParserSmia(std::initializer_list registerList)\n> > {\n> > - assert(num_regs > 0);\n> > + for (auto r : registerList)\n> > + offsets_[r] = {};\n> > +}\n> > +\n> > +MdParser::Status MdParserSmia::Parse(libcamera::Span buffer,\n> > + RegisterMap ®isters)\n> > +{\n> > + if (reset_) {\n> > + /*\n> > + * Search again through the metadata for all the registers\n> > + * requested.\n> > + */\n> > + ASSERT(bits_per_pixel_);\n> > +\n> > + for (const auto &[reg, offset] : offsets_)\n> > + offsets_[reg] = {};\n> > +\n> > + ParseStatus ret = findRegs(buffer);\n> > + /*\n> > + * > 0 means \"worked partially but parse again next time\",\n> > + * < 0 means \"hard error\".\n> > + *\n> > + * In either case, we retry parsing on the next frame.\n> > + */\n> > + if (ret != PARSE_OK)\n> > + return ERROR;\n> > +\n> > + reset_ = false;\n> > + }\n> > +\n> > + /* Populate the register values requested. */\n> > + registers.clear();\n> > + for (const auto &[reg, offset] : offsets_) {\n> > + if (!offset) {\n> > + reset_ = true;\n> > + return NOTFOUND;\n> > + }\n> > + registers[reg] = buffer[offset.value()];\n> > + }\n> > +\n> > + return OK;\n> > +}\n> > +\n> > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer)\n> > +{\n> > + ASSERT(offsets_.size());\n> >\n> > if (buffer[0] != LINE_START)\n> > return NO_LINE_START;\n> >\n> > unsigned int current_offset = 1; /* after the LINE_START */\n> > unsigned int current_line_start = 0, current_line = 0;\n> > - unsigned int reg_num = 0, first_reg = 0;\n> > + unsigned int reg_num = 0, regs_done = 0;\n> >\n> > while (1) {\n> > int tag = buffer[current_offset++];\n> > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> > else if (tag == REG_SKIP)\n> > reg_num++;\n> > else if (tag == REG_VALUE) {\n> > - while (reg_num >=\n> > - /* assumes registers are in order... */\n> > - regs[first_reg]) {\n> > - if (reg_num == regs[first_reg])\n> > - offsets[first_reg] = current_offset - 1;\n> > + auto reg = offsets_.find(reg_num);\n> > +\n> > + if (reg != offsets_.end()) {\n> > + offsets_[reg_num] = current_offset - 1;\n> >\n> > - if (++first_reg == num_regs)\n> > + if (++regs_done == offsets_.size())\n> > return PARSE_OK;\n> > }\n> > reg_num++;","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 D0739C321F\n\tfor ;\n\tTue, 29 Jun 2021 14:44:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D661684EC;\n\tTue, 29 Jun 2021 16:44:17 +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 C38E3684CB\n\tfor ;\n\tTue, 29 Jun 2021 16:44:15 +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 2B71E4AD;\n\tTue, 29 Jun 2021 16:44:15 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"v9YWNUD3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624977855;\n\tbh=PsBwGKhv3YZwmeWwuUEbnGK/YW2MhZkEn5hKjdbSK1w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=v9YWNUD3j7rNbRaDPird17Nif+K4iQUw4guhYOWcdEkzS0YqdfE+iPCwHsbHcRFoh\n\tv5HoWpGOXRn40x8qEGrVPb2Kq5cZblYcqiqwlwRY6JN79Hxb+e9jmDnK24Up5gtwjc\n\tKaGJYkxDMuC45JNt8b5Tsla6t77W926r5MwlCzj8=","Date":"Tue, 29 Jun 2021 17:44:12 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210629104500.51672-1-naush@raspberrypi.com>\n\t<20210629104500.51672-3-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","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":17919,"web_url":"https://patchwork.libcamera.org/comment/17919/","msgid":"","date":"2021-06-29T14:50:04","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Tue, 29 Jun 2021 at 15:44, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Tue, Jun 29, 2021 at 02:53:50PM +0100, Naushir Patuck wrote:\n> > On Tue, 29 Jun 2021 at 11:45, Naushir Patuck \n> wrote:\n> >\n> > > Instead of having each CamHelper subclass the MdParserSmia, change the\n> > > implementation of MdParserSmia to be more generic. The MdParserSmia\n> now gets\n> > > given a list of registers to search for and helper functions are used\n> to compute\n> > > exposure lines and gain codes from these registers.\n> > >\n> > > Update the imx219 and imx477 CamHelpers by using this new mechanism.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: Laurent Pinchart \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper.cpp | 33 +++---\n> > > src/ipa/raspberrypi/cam_helper.hpp | 2 +\n> > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------\n> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------\n> > > src/ipa/raspberrypi/md_parser.hpp | 41 +++++---\n> > > src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--\n> > > 6 files changed, 144 insertions(+), 234 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index 90498c37af98..e6d2258c66d7 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -159,32 +159,34 @@ unsigned int\n> CamHelper::MistrustFramesModeSwitch() const\n> > > void CamHelper::parseEmbeddedData(Span buffer,\n> > > Metadata &metadata)\n> > > {\n> > > + MdParser::RegisterMap registers;\n> > > + Metadata parsedMetadata;\n> > > +\n> > > if (buffer.empty())\n> > > return;\n> > >\n> > > - uint32_t exposureLines, gainCode;\n> > > -\n> > > - if (parser_->Parse(buffer) != MdParser::Status::OK ||\n> > > - parser_->GetExposureLines(exposureLines) !=\n> MdParser::Status::OK ||\n> > > - parser_->GetGainCode(gainCode) != MdParser::Status::OK) {\n> > > + if (parser_->Parse(buffer, registers) != MdParser::Status::OK)\n> {\n> > > LOG(IPARPI, Error) << \"Embedded data buffer parsing\n> failed\";\n> > > return;\n> > > }\n> > >\n> > > + PopulateMetadata(registers, parsedMetadata);\n> > > + metadata.Merge(parsedMetadata);\n> > > +\n> > > /*\n> > > - * Overwrite the exposure/gain values in the DeviceStatus, as\n> > > - * we know better. Fetch it first in case any other fields were\n> > > - * set meaningfully.\n> > > + * Overwrite the exposure/gain values in the existing\n> DeviceStatus with\n> > > + * values from the parsed embedded buffer. Fetch it first in\n> case any\n> > > + * other fields were set meaningfully.\n> > > */\n> > > - DeviceStatus deviceStatus;\n> > > -\n> > > - if (metadata.Get(\"device.status\", deviceStatus) != 0) {\n> > > + DeviceStatus deviceStatus, parsedDeviceStatus;\n> > > + if (metadata.Get(\"device.status\", deviceStatus) ||\n> > > + parsedMetadata.Get(\"device.status\", parsedDeviceStatus)) {\n> > > LOG(IPARPI, Error) << \"DeviceStatus not found\";\n> > > return;\n> > > }\n> > >\n> > > - deviceStatus.shutter_speed = Exposure(exposureLines);\n> > > - deviceStatus.analogue_gain = Gain(gainCode);\n> > > + deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> > > + deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> > >\n> > > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > > << deviceStatus.shutter_speed\n> > > @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span uint8_t> buffer,\n> > > metadata.Set(\"device.status\", deviceStatus);\n> > > }\n> > >\n> > > +void CamHelper::PopulateMetadata([[maybe_unused]] const\n> MdParser::RegisterMap ®isters,\n> > > + [[maybe_unused]] Metadata &metadata)\n> const\n> > > +{\n> > > +}\n> > > +\n> > > RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> > > CamHelperCreateFunc create_func)\n> > > {\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> > > index fc3139e22be0..200cc83f3872 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > @@ -92,6 +92,8 @@ public:\n> > > protected:\n> > > void parseEmbeddedData(libcamera::Span buffer,\n> > > Metadata &metadata);\n> > > + virtual void PopulateMetadata(const MdParser::RegisterMap\n> ®isters,\n> > > + Metadata &metadata) const;\n> > >\n> > > std::unique_ptr parser_;\n> > > CameraMode mode_;\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > index c85044a5fa6d..4e4994188ff3 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > @@ -23,21 +23,14 @@\n> > >\n> > > using namespace RPiController;\n> > >\n> > > -/* Metadata parser implementation specific to Sony IMX219 sensors. */\n> > > -\n> > > -class MdParserImx219 : public MdParserSmia\n> > > -{\n> > > -public:\n> > > - MdParserImx219();\n> > > - Status Parse(libcamera::Span buffer) override;\n> > > - Status GetExposureLines(unsigned int &lines) override;\n> > > - Status GetGainCode(unsigned int &gain_code) override;\n> > > -private:\n> > > - /* Offset of the register's value in the metadata block. */\n> > > - int reg_offsets_[3];\n> > > - /* Value of the register, once read from the metadata block. */\n> > > - int reg_values_[3];\n> > > -};\n> > > +/*\n> > > + * We care about one gain register and a pair of exposure registers.\n> Their I2C\n> > > + * addresses from the Sony IMX219 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 = { expHiReg,\n> expLoReg, gainReg };\n> > >\n> > > class CamHelperImx219 : public CamHelper\n> > > {\n> > > @@ -54,11 +47,14 @@ private:\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> > > CamHelperImx219::CamHelperImx219()\n> > > #if ENABLE_EMBEDDED_DATA\n> > > - : CamHelper(std::make_unique(),\n> frameIntegrationDiff)\n> > > + : CamHelper(std::make_unique(registerList),\n> frameIntegrationDiff)\n> > > #else\n> > > : CamHelper({}, frameIntegrationDiff)\n> > > #endif\n> > > @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent()\n> const\n> > > return ENABLE_EMBEDDED_DATA;\n> > > }\n> > >\n> > > -static CamHelper *Create()\n> > > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap\n> ®isters,\n> > > + Metadata &metadata) const\n> > > {\n> > > - return new CamHelperImx219();\n> > > -}\n> > > + DeviceStatus deviceStatus;\n> > >\n> > > -static RegisterCamHelper reg(\"imx219\", &Create);\n> > > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg)\n> * 256 + registers.at(expLoReg));\n> > > + deviceStatus.analogue_gain = Gain(registers.at(gainReg));\n> > >\n> > > -/*\n> > > - * We care about one gain register and a pair of exposure registers.\n> Their I2C\n> > > - * addresses from the Sony IMX219 datasheet:\n> > > - */\n> > > -#define GAIN_REG 0x157\n> > > -#define EXPHI_REG 0x15A\n> > > -#define EXPLO_REG 0x15B\n> > > -\n> > > -/*\n> > > - * Index of each into the reg_offsets and reg_values arrays. Must be\n> in\n> > > - * register address order.\n> > > - */\n> > > -#define GAIN_INDEX 0\n> > > -#define EXPHI_INDEX 1\n> > > -#define EXPLO_INDEX 2\n> > > -\n> > > -MdParserImx219::MdParserImx219()\n> > > -{\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > > + metadata.Set(\"device.status\", deviceStatus);\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx219::Parse(libcamera::Span\n> buffer)\n> > > -{\n> > > - bool try_again = false;\n> > > -\n> > > - if (reset_) {\n> > > - /*\n> > > - * Search again through the metadata for the gain and\n> exposure\n> > > - * registers.\n> > > - */\n> > > - assert(bits_per_pixel_);\n> > > - /* Need to be ordered */\n> > > - uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =\n> -1;\n> > > - int ret = static_cast(findRegs(buffer,\n> > > - regs,\n> reg_offsets_, 3));\n> > > - /*\n> > > - * > 0 means \"worked partially but parse again next\n> time\",\n> > > - * < 0 means \"hard error\".\n> > > - */\n> > > - if (ret > 0)\n> > > - try_again = true;\n> > > - else if (ret < 0)\n> > > - return ERROR;\n> > > - }\n> > > -\n> > > - for (int i = 0; i < 3; i++) {\n> > > - if (reg_offsets_[i] == -1)\n> > > - continue;\n> > > -\n> > > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > > - }\n> > > -\n> > > - /* Re-parse next time if we were unhappy in some way. */\n> > > - reset_ = try_again;\n> > > -\n> > > - return OK;\n> > > -}\n> > > -\n> > > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)\n> > > +static CamHelper *Create()\n> > > {\n> > > - if (reg_offsets_[EXPHI_INDEX] == -1 ||\n> reg_offsets_[EXPLO_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - lines = reg_values_[EXPHI_INDEX] * 256 +\n> reg_values_[EXPLO_INDEX];\n> > > -\n> > > - return OK;\n> > > + return new CamHelperImx219();\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)\n> > > -{\n> > > - if (reg_offsets_[GAIN_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - gain_code = reg_values_[GAIN_INDEX];\n> > > -\n> > > - return OK;\n> > > -}\n> > > +static RegisterCamHelper reg(\"imx219\", &Create);\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > index d72a9be0438e..efd1a5893db8 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > @@ -15,21 +15,15 @@\n> > >\n> > > using namespace RPiController;\n> > >\n> > > -/* Metadata parser implementation specific to Sony IMX477 sensors. */\n> > > -\n> > > -class MdParserImx477 : public MdParserSmia\n> > > -{\n> > > -public:\n> > > - MdParserImx477();\n> > > - Status Parse(libcamera::Span buffer) override;\n> > > - Status GetExposureLines(unsigned int &lines) override;\n> > > - Status GetGainCode(unsigned int &gain_code) override;\n> > > -private:\n> > > - /* Offset of the register's value in the metadata block. */\n> > > - int reg_offsets_[4];\n> > > - /* Value of the register, once read from the metadata block. */\n> > > - int reg_values_[4];\n> > > -};\n> > > +/*\n> > > + * We care about two gain registers and a pair of exposure registers.\n> Their\n> > > + * I2C addresses from the Sony IMX477 datasheet:\n> > > + */\n> > > +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> > >\n> > > class CamHelperImx477 : public CamHelper\n> > > {\n> > > @@ -47,10 +41,13 @@ private:\n> > > * in units of lines.\n> > > */\n> > > static constexpr int frameIntegrationDiff = 22;\n> > > +\n> > > + void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > > + Metadata &metadata) const override;\n> > > };\n> > >\n> > > CamHelperImx477::CamHelperImx477()\n> > > - : CamHelper(std::make_unique(),\n> frameIntegrationDiff)\n> > > + : CamHelper(std::make_unique(registerList),\n> frameIntegrationDiff)\n> > > {\n> > > }\n> > >\n> > > @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent()\n> const\n> > > return true;\n> > > }\n> > >\n> > > -static CamHelper *Create()\n> > > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap\n> ®isters,\n> > > + Metadata &metadata) const\n> > > {\n> > > - return new CamHelperImx477();\n> > > -}\n> > > + DeviceStatus deviceStatus;\n> > >\n> > > -static RegisterCamHelper reg(\"imx477\", &Create);\n> > > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg)\n> * 256 + registers.at(expLoReg));\n> > > + deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) *\n> 256 + registers.at(gainLoReg));\n> > >\n> > > -/*\n> > > - * We care about two gain registers and a pair of exposure registers.\n> Their\n> > > - * I2C addresses from the Sony IMX477 datasheet:\n> > > - */\n> > > -#define EXPHI_REG 0x0202\n> > > -#define EXPLO_REG 0x0203\n> > > -#define GAINHI_REG 0x0204\n> > > -#define GAINLO_REG 0x0205\n> > > -\n> > > -/*\n> > > - * Index of each into the reg_offsets and reg_values arrays. Must be\n> in register\n> > > - * address order.\n> > > - */\n> > > -#define EXPHI_INDEX 0\n> > > -#define EXPLO_INDEX 1\n> > > -#define GAINHI_INDEX 2\n> > > -#define GAINLO_INDEX 3\n> > > -\n> > > -MdParserImx477::MdParserImx477()\n> > > -{\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =\n> reg_offsets_[3] = -1;\n> > > + metadata.Set(\"device.status\", deviceStatus);\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx477::Parse(libcamera::Span\n> buffer)\n> > > -{\n> > > - bool try_again = false;\n> > > -\n> > > - if (reset_) {\n> > > - /*\n> > > - * Search again through the metadata for the gain and\n> exposure\n> > > - * registers.\n> > > - */\n> > > - assert(bits_per_pixel_);\n> > > - /* Need to be ordered */\n> > > - uint32_t regs[4] = {\n> > > - EXPHI_REG,\n> > > - EXPLO_REG,\n> > > - GAINHI_REG,\n> > > - GAINLO_REG\n> > > - };\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =\n> reg_offsets_[3] = -1;\n> > > - int ret = static_cast(findRegs(buffer,\n> > > - regs,\n> reg_offsets_, 4));\n> > > - /*\n> > > - * > 0 means \"worked partially but parse again next\n> time\",\n> > > - * < 0 means \"hard error\".\n> > > - */\n> > > - if (ret > 0)\n> > > - try_again = true;\n> > > - else if (ret < 0)\n> > > - return ERROR;\n> > > - }\n> > > -\n> > > - for (int i = 0; i < 4; i++) {\n> > > - if (reg_offsets_[i] == -1)\n> > > - continue;\n> > > -\n> > > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > > - }\n> > > -\n> > > - /* Re-parse next time if we were unhappy in some way. */\n> > > - reset_ = try_again;\n> > > -\n> > > - return OK;\n> > > -}\n> > > -\n> > > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)\n> > > +static CamHelper *Create()\n> > > {\n> > > - if (reg_offsets_[EXPHI_INDEX] == -1 ||\n> reg_offsets_[EXPLO_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - lines = reg_values_[EXPHI_INDEX] * 256 +\n> reg_values_[EXPLO_INDEX];\n> > > -\n> > > - return OK;\n> > > + return new CamHelperImx477();\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)\n> > > -{\n> > > - if (reg_offsets_[GAINHI_INDEX] == -1 ||\n> reg_offsets_[GAINLO_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - gain_code = reg_values_[GAINHI_INDEX] * 256 +\n> > > reg_values_[GAINLO_INDEX];\n> > > -\n> > > - return OK;\n> > > -}\n> > > +static RegisterCamHelper reg(\"imx477\", &Create);\n> > > diff --git a/src/ipa/raspberrypi/md_parser.hpp\n> b/src/ipa/raspberrypi/md_parser.hpp\n> > > index 8497216f8db7..e3e2738537a8 100644\n> > > --- a/src/ipa/raspberrypi/md_parser.hpp\n> > > +++ b/src/ipa/raspberrypi/md_parser.hpp\n> > > @@ -6,6 +6,9 @@\n> > > */\n> > > #pragma once\n> > >\n> > > +#include \n> > > +#include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -19,7 +22,7 @@\n> > > * application code doesn't have to worry which kind to instantiate.\n> But for\n> > > * the sake of example let's suppose we're parsing imx219 metadata.\n> > > *\n> > > - * MdParser *parser = new MdParserImx219(); // for example\n> > > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg\n> });\n> > > * parser->SetBitsPerPixel(bpp);\n> > > * parser->SetLineLengthBytes(pitch);\n> > > * parser->SetNumLines(2);\n> > > @@ -32,13 +35,11 @@\n> > > *\n> > > * Then on every frame:\n> > > *\n> > > - * if (parser->Parse(buffer) != MdParser::OK)\n> > > + * RegisterMap registers;\n> > > + * if (parser->Parse(buffer, registers) != MdParser::OK)\n> > > * much badness;\n> > > - * unsigned int exposure_lines, gain_code\n> > > - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)\n> > > - * exposure was not found;\n> > > - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> > > - * gain code was not found;\n> > > + * Metadata metadata;\n> > > + * CamHelper::PopulateMetadata(registers, metadata);\n> > > *\n> > > * (Note that the CamHelper class converts to/from exposure lines and\n> time,\n> > > * and gain_code / actual gain.)\n> > > @@ -59,6 +60,8 @@ namespace RPiController {\n> > > class MdParser\n> > > {\n> > > public:\n> > > + using RegisterMap = std::map;\n> > > +\n> > > /*\n> > > * Parser status codes:\n> > > * OK - success\n> > > @@ -98,9 +101,8 @@ public:\n> > > line_length_bytes_ = num_bytes;\n> > > }\n> > >\n> > > - virtual Status Parse(libcamera::Span buffer) =\n> 0;\n> > > - virtual Status GetExposureLines(unsigned int &lines) = 0;\n> > > - virtual Status GetGainCode(unsigned int &gain_code) = 0;\n> > > + virtual Status Parse(libcamera::Span buffer,\n> > > + RegisterMap ®isters) = 0;\n> > >\n> > > protected:\n> > > bool reset_;\n> > > @@ -116,14 +118,18 @@ protected:\n> > > * md_parser_imx219.cpp for an example).\n> > > */\n> > >\n> > > -class MdParserSmia : public MdParser\n> > > +class MdParserSmia final : public MdParser\n> > > {\n> > > public:\n> > > - MdParserSmia() : MdParser()\n> > > - {\n> > > - }\n> > > + MdParserSmia(std::initializer_list registerList);\n> > > +\n> > > + MdParser::Status Parse(libcamera::Span buffer,\n> > > + RegisterMap ®isters) override;\n> > > +\n> > > +private:\n> > > + /* Maps register address to offset in the buffer. */\n> > > + using OffsetMap = std::map>;\n> > >\n> > > -protected:\n> > > /*\n> > > * Note that error codes > 0 are regarded as non-fatal; codes\n> < 0\n> > > * indicate a bad data buffer. Status codes are:\n> > > @@ -141,8 +147,9 @@ protected:\n> > > BAD_PADDING = -5\n> > > };\n> > >\n> > > - ParseStatus findRegs(libcamera::Span buffer,\n> uint32_t regs[],\n> > > - int offsets[], unsigned int num_regs);\n> > > + ParseStatus findRegs(libcamera::Span buffer);\n> > > +\n> > > + OffsetMap offsets_;\n> > > };\n> > >\n> > > } // namespace RPi\n> > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp\n> b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > index 0a14875575a2..7f72fc03948f 100644\n> > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > @@ -6,9 +6,11 @@\n> > > */\n> > > #include \n> > >\n> >\n> > Oops, this header is not needed any more!\n> >\n> > > +#include \"libcamera/base/log.h\"\n>\n> And this should be . I'll fix when applying.\n>\n\nThanks Laurent!\n\n\n>\n> > > #include \"md_parser.hpp\"\n> > >\n> > > using namespace RPiController;\n> > > +using namespace libcamera;\n> > >\n> > > /*\n> > > * This function goes through the embedded data to find the offsets\n> (not\n> > > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;\n> > > constexpr unsigned int REG_VALUE = 0x5a;\n> > > constexpr unsigned int REG_SKIP = 0x55;\n> > >\n> > > -MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span buffer,\n> > > - uint32_t regs[], int\n> offsets[],\n> > > - unsigned int num_regs)\n> > > +MdParserSmia::MdParserSmia(std::initializer_list\n> registerList)\n> > > {\n> > > - assert(num_regs > 0);\n> > > + for (auto r : registerList)\n> > > + offsets_[r] = {};\n> > > +}\n> > > +\n> > > +MdParser::Status MdParserSmia::Parse(libcamera::Span\n> buffer,\n> > > + RegisterMap ®isters)\n> > > +{\n> > > + if (reset_) {\n> > > + /*\n> > > + * Search again through the metadata for all the\n> registers\n> > > + * requested.\n> > > + */\n> > > + ASSERT(bits_per_pixel_);\n> > > +\n> > > + for (const auto &[reg, offset] : offsets_)\n> > > + offsets_[reg] = {};\n> > > +\n> > > + ParseStatus ret = findRegs(buffer);\n> > > + /*\n> > > + * > 0 means \"worked partially but parse again next\n> time\",\n> > > + * < 0 means \"hard error\".\n> > > + *\n> > > + * In either case, we retry parsing on the next frame.\n> > > + */\n> > > + if (ret != PARSE_OK)\n> > > + return ERROR;\n> > > +\n> > > + reset_ = false;\n> > > + }\n> > > +\n> > > + /* Populate the register values requested. */\n> > > + registers.clear();\n> > > + for (const auto &[reg, offset] : offsets_) {\n> > > + if (!offset) {\n> > > + reset_ = true;\n> > > + return NOTFOUND;\n> > > + }\n> > > + registers[reg] = buffer[offset.value()];\n> > > + }\n> > > +\n> > > + return OK;\n> > > +}\n> > > +\n> > > +MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span buffer)\n> > > +{\n> > > + ASSERT(offsets_.size());\n> > >\n> > > if (buffer[0] != LINE_START)\n> > > return NO_LINE_START;\n> > >\n> > > unsigned int current_offset = 1; /* after the LINE_START */\n> > > unsigned int current_line_start = 0, current_line = 0;\n> > > - unsigned int reg_num = 0, first_reg = 0;\n> > > + unsigned int reg_num = 0, regs_done = 0;\n> > >\n> > > while (1) {\n> > > int tag = buffer[current_offset++];\n> > > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span\n> > > else if (tag == REG_SKIP)\n> > > reg_num++;\n> > > else if (tag == REG_VALUE) {\n> > > - while (reg_num >=\n> > > - /* assumes registers are in\n> order... */\n> > > - regs[first_reg]) {\n> > > - if (reg_num == regs[first_reg])\n> > > - offsets[first_reg] =\n> current_offset - 1;\n> > > + auto reg = offsets_.find(reg_num);\n> > > +\n> > > + if (reg != offsets_.end()) {\n> > > + offsets_[reg_num] =\n> current_offset - 1;\n> > >\n> > > - if (++first_reg == num_regs)\n> > > + if (++regs_done ==\n> offsets_.size())\n> > > return PARSE_OK;\n> > > }\n> > > reg_num++;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EDFC9C3220\n\tfor ;\n\tTue, 29 Jun 2021 14:50:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 297F0684EC;\n\tTue, 29 Jun 2021 16:50:24 +0200 (CEST)","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 784AD684CB\n\tfor ;\n\tTue, 29 Jun 2021 16:50:21 +0200 (CEST)","by mail-lf1-x136.google.com with SMTP id w19so11379861lfk.5\n\tfor ;\n\tTue, 29 Jun 2021 07:50:21 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"WnlAVJrO\"; 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=7RTHT8XjIiS8YRwh9ozz5nUJzGJGRyqfcTJtFnjQldM=;\n\tb=WnlAVJrOzmgrB/CBtqPGYH5zoI11w3E9mf2WK0a5HgU5t2dyAzIXnWNiQX6fp1Kqm3\n\tp7EwXfkS2IMTPYs5/kA2GH0tWoccYKoMSUMtH61PdmYBp7TGkDVDSnbiq0VfI0Z7yyzM\n\tg/MGJCuzeE6mhamsKjor2T5Gws99rcvk1m2PLzbOi0kcnNxb6QB24pxuYELJ6RulM7Ru\n\tgm7O+CcWR+6kWCN0cZzrKI36hTjKbaRfOZubi9yrdLmN0Oiz02t7GWHt8tne5AuVbP8W\n\t8Xf6CPaq5R0UmOqiLXO4qjw1rtTZJst7aISPj5ZoWlkxcM1LpUY17J3HyruCIC8wa9T/\n\tpwTw==","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=7RTHT8XjIiS8YRwh9ozz5nUJzGJGRyqfcTJtFnjQldM=;\n\tb=rzxbTZYz/9vtcExlTA4ywM+buNtfrY0wf2d5U2JEjJ67EH122kcuOiTwG11O/1K3Cx\n\tSmpfoaULx8PacHiAtUSHPdE2LVCnH9E/wQAOFZ2PXvAJsV5QwKOqQ6X19lZLOf1UHRDC\n\tHE4BbkY3nxpaQm8rgHXz6ZPKFGrsKqC66jK/vOYsi7YqfXj6ZDxDT2OyTDF4ed3ReDdB\n\tSy/EskOabZi5347WKiRR0uAM3K9D0La1NQ8fls/hRONxJsOGkaO0njfFdDCnrAriBIGL\n\tfcmcf2p+i2EwON0DUHjaert/3AbwpBchITKsmNYdkPML02bLmvqLvFPMlffLtDE/3KHr\n\taM0w==","X-Gm-Message-State":"AOAM5303+LDYe0/HHl4P3r7sbtveanf7Ycvq71Gr4BLqopJNJqIF5GKe\n\t+Z7R5fIwFvdQVP/PaHIADUxlObCzroYjeCEH4vxKGw==","X-Google-Smtp-Source":"ABdhPJyEiHVT6xmATzVGCDCjcH3C4wXJ0h/tQbebjAYmFPZE8Y1jJwfKZvDx/ETBIWBPn3oFI2teSPFdyUcdyzZ8+3Y=","X-Received":"by 2002:ac2:4342:: with SMTP id o2mr7000280lfl.375.1624978220705;\n\tTue, 29 Jun 2021 07:50:20 -0700 (PDT)","MIME-Version":"1.0","References":"<20210629104500.51672-1-naush@raspberrypi.com>\n\t<20210629104500.51672-3-naush@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 29 Jun 2021 15:50:04 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000009cfc6f05c5e8b9d0\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","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":17922,"web_url":"https://patchwork.libcamera.org/comment/17922/","msgid":"","date":"2021-06-29T15:47:00","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello again,\n\nOn Tue, Jun 29, 2021 at 05:44:14PM +0300, Laurent Pinchart wrote:\n> On Tue, Jun 29, 2021 at 02:53:50PM +0100, Naushir Patuck wrote:\n> > On Tue, 29 Jun 2021 at 11:45, Naushir Patuck wrote:\n> > \n> > > Instead of having each CamHelper subclass the MdParserSmia, change the\n> > > implementation of MdParserSmia to be more generic. The MdParserSmia now gets\n> > > given a list of registers to search for and helper functions are used to compute\n> > > exposure lines and gain codes from these registers.\n> > >\n> > > Update the imx219 and imx477 CamHelpers by using this new mechanism.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: Laurent Pinchart \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper.cpp | 33 +++---\n> > > src/ipa/raspberrypi/cam_helper.hpp | 2 +\n> > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------\n> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------\n> > > src/ipa/raspberrypi/md_parser.hpp | 41 +++++---\n> > > src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--\n> > > 6 files changed, 144 insertions(+), 234 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index 90498c37af98..e6d2258c66d7 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -159,32 +159,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n> > > void CamHelper::parseEmbeddedData(Span buffer,\n> > > Metadata &metadata)\n> > > {\n> > > + MdParser::RegisterMap registers;\n> > > + Metadata parsedMetadata;\n> > > +\n> > > if (buffer.empty())\n> > > return;\n> > >\n> > > - uint32_t exposureLines, gainCode;\n> > > -\n> > > - if (parser_->Parse(buffer) != MdParser::Status::OK ||\n> > > - parser_->GetExposureLines(exposureLines) != MdParser::Status::OK ||\n> > > - parser_->GetGainCode(gainCode) != MdParser::Status::OK) {\n> > > + if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {\n> > > LOG(IPARPI, Error) << \"Embedded data buffer parsing failed\";\n> > > return;\n> > > }\n> > >\n> > > + PopulateMetadata(registers, parsedMetadata);\n> > > + metadata.Merge(parsedMetadata);\n> > > +\n> > > /*\n> > > - * Overwrite the exposure/gain values in the DeviceStatus, as\n> > > - * we know better. Fetch it first in case any other fields were\n> > > - * set meaningfully.\n> > > + * Overwrite the exposure/gain values in the existing DeviceStatus with\n> > > + * values from the parsed embedded buffer. Fetch it first in case any\n> > > + * other fields were set meaningfully.\n> > > */\n> > > - DeviceStatus deviceStatus;\n> > > -\n> > > - if (metadata.Get(\"device.status\", deviceStatus) != 0) {\n> > > + DeviceStatus deviceStatus, parsedDeviceStatus;\n> > > + if (metadata.Get(\"device.status\", deviceStatus) ||\n> > > + parsedMetadata.Get(\"device.status\", parsedDeviceStatus)) {\n> > > LOG(IPARPI, Error) << \"DeviceStatus not found\";\n> > > return;\n> > > }\n> > >\n> > > - deviceStatus.shutter_speed = Exposure(exposureLines);\n> > > - deviceStatus.analogue_gain = Gain(gainCode);\n> > > + deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> > > + deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> > >\n> > > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > > << deviceStatus.shutter_speed\n> > > @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span buffer,\n> > > metadata.Set(\"device.status\", deviceStatus);\n> > > }\n> > >\n> > > +void CamHelper::PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap ®isters,\n> > > + [[maybe_unused]] Metadata &metadata) const\n> > > +{\n> > > +}\n> > > +\n> > > RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> > > CamHelperCreateFunc create_func)\n> > > {\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> > > index fc3139e22be0..200cc83f3872 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > @@ -92,6 +92,8 @@ public:\n> > > protected:\n> > > void parseEmbeddedData(libcamera::Span buffer,\n> > > Metadata &metadata);\n> > > + virtual void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > > + Metadata &metadata) const;\n> > >\n> > > std::unique_ptr parser_;\n> > > CameraMode mode_;\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > index c85044a5fa6d..4e4994188ff3 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > @@ -23,21 +23,14 @@\n> > >\n> > > using namespace RPiController;\n> > >\n> > > -/* Metadata parser implementation specific to Sony IMX219 sensors. */\n> > > -\n> > > -class MdParserImx219 : public MdParserSmia\n> > > -{\n> > > -public:\n> > > - MdParserImx219();\n> > > - Status Parse(libcamera::Span buffer) override;\n> > > - Status GetExposureLines(unsigned int &lines) override;\n> > > - Status GetGainCode(unsigned int &gain_code) override;\n> > > -private:\n> > > - /* Offset of the register's value in the metadata block. */\n> > > - int reg_offsets_[3];\n> > > - /* Value of the register, once read from the metadata block. */\n> > > - int reg_values_[3];\n> > > -};\n> > > +/*\n> > > + * We care about one gain register and a pair of exposure registers. Their I2C\n> > > + * addresses from the Sony IMX219 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 = { expHiReg, expLoReg, gainReg };\n\nThis needs a [[maybe_unused]] in case ENABLE_EMBEDDED_DATA isn't\ndefined.\n\n> > >\n> > > class CamHelperImx219 : public CamHelper\n> > > {\n> > > @@ -54,11 +47,14 @@ private:\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> > > CamHelperImx219::CamHelperImx219()\n> > > #if ENABLE_EMBEDDED_DATA\n> > > - : CamHelper(std::make_unique(), frameIntegrationDiff)\n> > > + : CamHelper(std::make_unique(registerList), frameIntegrationDiff)\n> > > #else\n> > > : CamHelper({}, frameIntegrationDiff)\n> > > #endif\n> > > @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const\n> > > return ENABLE_EMBEDDED_DATA;\n> > > }\n> > >\n> > > -static CamHelper *Create()\n> > > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > > + Metadata &metadata) const\n> > > {\n> > > - return new CamHelperImx219();\n> > > -}\n> > > + DeviceStatus deviceStatus;\n> > >\n> > > -static RegisterCamHelper reg(\"imx219\", &Create);\n> > > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> > > + deviceStatus.analogue_gain = Gain(registers.at(gainReg));\n> > >\n> > > -/*\n> > > - * We care about one gain register and a pair of exposure registers. Their I2C\n> > > - * addresses from the Sony IMX219 datasheet:\n> > > - */\n> > > -#define GAIN_REG 0x157\n> > > -#define EXPHI_REG 0x15A\n> > > -#define EXPLO_REG 0x15B\n> > > -\n> > > -/*\n> > > - * Index of each into the reg_offsets and reg_values arrays. Must be in\n> > > - * register address order.\n> > > - */\n> > > -#define GAIN_INDEX 0\n> > > -#define EXPHI_INDEX 1\n> > > -#define EXPLO_INDEX 2\n> > > -\n> > > -MdParserImx219::MdParserImx219()\n> > > -{\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > > + metadata.Set(\"device.status\", deviceStatus);\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx219::Parse(libcamera::Span buffer)\n> > > -{\n> > > - bool try_again = false;\n> > > -\n> > > - if (reset_) {\n> > > - /*\n> > > - * Search again through the metadata for the gain and exposure\n> > > - * registers.\n> > > - */\n> > > - assert(bits_per_pixel_);\n> > > - /* Need to be ordered */\n> > > - uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > > - int ret = static_cast(findRegs(buffer,\n> > > - regs, reg_offsets_, 3));\n> > > - /*\n> > > - * > 0 means \"worked partially but parse again next time\",\n> > > - * < 0 means \"hard error\".\n> > > - */\n> > > - if (ret > 0)\n> > > - try_again = true;\n> > > - else if (ret < 0)\n> > > - return ERROR;\n> > > - }\n> > > -\n> > > - for (int i = 0; i < 3; i++) {\n> > > - if (reg_offsets_[i] == -1)\n> > > - continue;\n> > > -\n> > > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > > - }\n> > > -\n> > > - /* Re-parse next time if we were unhappy in some way. */\n> > > - reset_ = try_again;\n> > > -\n> > > - return OK;\n> > > -}\n> > > -\n> > > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)\n> > > +static CamHelper *Create()\n> > > {\n> > > - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> > > -\n> > > - return OK;\n> > > + return new CamHelperImx219();\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)\n> > > -{\n> > > - if (reg_offsets_[GAIN_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - gain_code = reg_values_[GAIN_INDEX];\n> > > -\n> > > - return OK;\n> > > -}\n> > > +static RegisterCamHelper reg(\"imx219\", &Create);\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > index d72a9be0438e..efd1a5893db8 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > @@ -15,21 +15,15 @@\n> > >\n> > > using namespace RPiController;\n> > >\n> > > -/* Metadata parser implementation specific to Sony IMX477 sensors. */\n> > > -\n> > > -class MdParserImx477 : public MdParserSmia\n> > > -{\n> > > -public:\n> > > - MdParserImx477();\n> > > - Status Parse(libcamera::Span buffer) override;\n> > > - Status GetExposureLines(unsigned int &lines) override;\n> > > - Status GetGainCode(unsigned int &gain_code) override;\n> > > -private:\n> > > - /* Offset of the register's value in the metadata block. */\n> > > - int reg_offsets_[4];\n> > > - /* Value of the register, once read from the metadata block. */\n> > > - int reg_values_[4];\n> > > -};\n> > > +/*\n> > > + * We care about two gain registers and a pair of exposure registers. Their\n> > > + * I2C addresses from the Sony IMX477 datasheet:\n> > > + */\n> > > +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\nSame here.\n\n> > >\n> > > class CamHelperImx477 : public CamHelper\n> > > {\n> > > @@ -47,10 +41,13 @@ private:\n> > > * in units of lines.\n> > > */\n> > > static constexpr int frameIntegrationDiff = 22;\n> > > +\n> > > + void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > > + Metadata &metadata) const override;\n> > > };\n> > >\n> > > CamHelperImx477::CamHelperImx477()\n> > > - : CamHelper(std::make_unique(), frameIntegrationDiff)\n> > > + : CamHelper(std::make_unique(registerList), frameIntegrationDiff)\n> > > {\n> > > }\n> > >\n> > > @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const\n> > > return true;\n> > > }\n> > >\n> > > -static CamHelper *Create()\n> > > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > > + Metadata &metadata) const\n> > > {\n> > > - return new CamHelperImx477();\n> > > -}\n> > > + DeviceStatus deviceStatus;\n> > >\n> > > -static RegisterCamHelper reg(\"imx477\", &Create);\n> > > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> > > + deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));\n> > >\n> > > -/*\n> > > - * We care about two gain registers and a pair of exposure registers. Their\n> > > - * I2C addresses from the Sony IMX477 datasheet:\n> > > - */\n> > > -#define EXPHI_REG 0x0202\n> > > -#define EXPLO_REG 0x0203\n> > > -#define GAINHI_REG 0x0204\n> > > -#define GAINLO_REG 0x0205\n> > > -\n> > > -/*\n> > > - * Index of each into the reg_offsets and reg_values arrays. Must be in register\n> > > - * address order.\n> > > - */\n> > > -#define EXPHI_INDEX 0\n> > > -#define EXPLO_INDEX 1\n> > > -#define GAINHI_INDEX 2\n> > > -#define GAINLO_INDEX 3\n> > > -\n> > > -MdParserImx477::MdParserImx477()\n> > > -{\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> > > + metadata.Set(\"device.status\", deviceStatus);\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx477::Parse(libcamera::Span buffer)\n> > > -{\n> > > - bool try_again = false;\n> > > -\n> > > - if (reset_) {\n> > > - /*\n> > > - * Search again through the metadata for the gain and exposure\n> > > - * registers.\n> > > - */\n> > > - assert(bits_per_pixel_);\n> > > - /* Need to be ordered */\n> > > - uint32_t regs[4] = {\n> > > - EXPHI_REG,\n> > > - EXPLO_REG,\n> > > - GAINHI_REG,\n> > > - GAINLO_REG\n> > > - };\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> > > - int ret = static_cast(findRegs(buffer,\n> > > - regs, reg_offsets_, 4));\n> > > - /*\n> > > - * > 0 means \"worked partially but parse again next time\",\n> > > - * < 0 means \"hard error\".\n> > > - */\n> > > - if (ret > 0)\n> > > - try_again = true;\n> > > - else if (ret < 0)\n> > > - return ERROR;\n> > > - }\n> > > -\n> > > - for (int i = 0; i < 4; i++) {\n> > > - if (reg_offsets_[i] == -1)\n> > > - continue;\n> > > -\n> > > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > > - }\n> > > -\n> > > - /* Re-parse next time if we were unhappy in some way. */\n> > > - reset_ = try_again;\n> > > -\n> > > - return OK;\n> > > -}\n> > > -\n> > > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)\n> > > +static CamHelper *Create()\n> > > {\n> > > - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> > > -\n> > > - return OK;\n> > > + return new CamHelperImx477();\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)\n> > > -{\n> > > - if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - gain_code = reg_values_[GAINHI_INDEX] * 256 +\n> > > reg_values_[GAINLO_INDEX];\n> > > -\n> > > - return OK;\n> > > -}\n> > > +static RegisterCamHelper reg(\"imx477\", &Create);\n> > > diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp\n> > > index 8497216f8db7..e3e2738537a8 100644\n> > > --- a/src/ipa/raspberrypi/md_parser.hpp\n> > > +++ b/src/ipa/raspberrypi/md_parser.hpp\n> > > @@ -6,6 +6,9 @@\n> > > */\n> > > #pragma once\n> > >\n> > > +#include \n> > > +#include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -19,7 +22,7 @@\n> > > * application code doesn't have to worry which kind to instantiate. But for\n> > > * the sake of example let's suppose we're parsing imx219 metadata.\n> > > *\n> > > - * MdParser *parser = new MdParserImx219(); // for example\n> > > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });\n> > > * parser->SetBitsPerPixel(bpp);\n> > > * parser->SetLineLengthBytes(pitch);\n> > > * parser->SetNumLines(2);\n> > > @@ -32,13 +35,11 @@\n> > > *\n> > > * Then on every frame:\n> > > *\n> > > - * if (parser->Parse(buffer) != MdParser::OK)\n> > > + * RegisterMap registers;\n> > > + * if (parser->Parse(buffer, registers) != MdParser::OK)\n> > > * much badness;\n> > > - * unsigned int exposure_lines, gain_code\n> > > - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)\n> > > - * exposure was not found;\n> > > - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> > > - * gain code was not found;\n> > > + * Metadata metadata;\n> > > + * CamHelper::PopulateMetadata(registers, metadata);\n> > > *\n> > > * (Note that the CamHelper class converts to/from exposure lines and time,\n> > > * and gain_code / actual gain.)\n> > > @@ -59,6 +60,8 @@ namespace RPiController {\n> > > class MdParser\n> > > {\n> > > public:\n> > > + using RegisterMap = std::map;\n> > > +\n> > > /*\n> > > * Parser status codes:\n> > > * OK - success\n> > > @@ -98,9 +101,8 @@ public:\n> > > line_length_bytes_ = num_bytes;\n> > > }\n> > >\n> > > - virtual Status Parse(libcamera::Span buffer) = 0;\n> > > - virtual Status GetExposureLines(unsigned int &lines) = 0;\n> > > - virtual Status GetGainCode(unsigned int &gain_code) = 0;\n> > > + virtual Status Parse(libcamera::Span buffer,\n> > > + RegisterMap ®isters) = 0;\n> > >\n> > > protected:\n> > > bool reset_;\n> > > @@ -116,14 +118,18 @@ protected:\n> > > * md_parser_imx219.cpp for an example).\n> > > */\n> > >\n> > > -class MdParserSmia : public MdParser\n> > > +class MdParserSmia final : public MdParser\n> > > {\n> > > public:\n> > > - MdParserSmia() : MdParser()\n> > > - {\n> > > - }\n> > > + MdParserSmia(std::initializer_list registerList);\n> > > +\n> > > + MdParser::Status Parse(libcamera::Span buffer,\n> > > + RegisterMap ®isters) override;\n> > > +\n> > > +private:\n> > > + /* Maps register address to offset in the buffer. */\n> > > + using OffsetMap = std::map>;\n> > >\n> > > -protected:\n> > > /*\n> > > * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> > > * indicate a bad data buffer. Status codes are:\n> > > @@ -141,8 +147,9 @@ protected:\n> > > BAD_PADDING = -5\n> > > };\n> > >\n> > > - ParseStatus findRegs(libcamera::Span buffer, uint32_t regs[],\n> > > - int offsets[], unsigned int num_regs);\n> > > + ParseStatus findRegs(libcamera::Span buffer);\n> > > +\n> > > + OffsetMap offsets_;\n> > > };\n> > >\n> > > } // namespace RPi\n> > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > index 0a14875575a2..7f72fc03948f 100644\n> > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > @@ -6,9 +6,11 @@\n> > > */\n> > > #include \n> > >\n> > \n> > Oops, this header is not needed any more!\n> > \n> > > +#include \"libcamera/base/log.h\"\n> \n> And this should be . I'll fix when applying.\n> \n> > > #include \"md_parser.hpp\"\n> > >\n> > > using namespace RPiController;\n> > > +using namespace libcamera;\n> > >\n> > > /*\n> > > * This function goes through the embedded data to find the offsets (not\n> > > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;\n> > > constexpr unsigned int REG_VALUE = 0x5a;\n> > > constexpr unsigned int REG_SKIP = 0x55;\n> > >\n> > > -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer,\n> > > - uint32_t regs[], int offsets[],\n> > > - unsigned int num_regs)\n> > > +MdParserSmia::MdParserSmia(std::initializer_list registerList)\n> > > {\n> > > - assert(num_regs > 0);\n> > > + for (auto r : registerList)\n> > > + offsets_[r] = {};\n> > > +}\n> > > +\n> > > +MdParser::Status MdParserSmia::Parse(libcamera::Span buffer,\n> > > + RegisterMap ®isters)\n> > > +{\n> > > + if (reset_) {\n> > > + /*\n> > > + * Search again through the metadata for all the registers\n> > > + * requested.\n> > > + */\n> > > + ASSERT(bits_per_pixel_);\n> > > +\n> > > + for (const auto &[reg, offset] : offsets_)\n> > > + offsets_[reg] = {};\n\nI'm afraid this causes compilation failures with gcc 7 and gcc 8, due to\noffset being unused. We can't throw a [[maybe_unused]] here, that's not\nsupported, so we need to revert to the syntax from v2.\n\n\t\tfor (const auto &kv : offsets_)\n\t\t\toffsets_[kv.first] = {};\n\nI'll post a v3.1, Naush, could you give it a try ?\n\n> > > +\n> > > + ParseStatus ret = findRegs(buffer);\n> > > + /*\n> > > + * > 0 means \"worked partially but parse again next time\",\n> > > + * < 0 means \"hard error\".\n> > > + *\n> > > + * In either case, we retry parsing on the next frame.\n> > > + */\n> > > + if (ret != PARSE_OK)\n> > > + return ERROR;\n> > > +\n> > > + reset_ = false;\n> > > + }\n> > > +\n> > > + /* Populate the register values requested. */\n> > > + registers.clear();\n> > > + for (const auto &[reg, offset] : offsets_) {\n> > > + if (!offset) {\n> > > + reset_ = true;\n> > > + return NOTFOUND;\n> > > + }\n> > > + registers[reg] = buffer[offset.value()];\n> > > + }\n> > > +\n> > > + return OK;\n> > > +}\n> > > +\n> > > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer)\n> > > +{\n> > > + ASSERT(offsets_.size());\n> > >\n> > > if (buffer[0] != LINE_START)\n> > > return NO_LINE_START;\n> > >\n> > > unsigned int current_offset = 1; /* after the LINE_START */\n> > > unsigned int current_line_start = 0, current_line = 0;\n> > > - unsigned int reg_num = 0, first_reg = 0;\n> > > + unsigned int reg_num = 0, regs_done = 0;\n> > >\n> > > while (1) {\n> > > int tag = buffer[current_offset++];\n> > > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> > > else if (tag == REG_SKIP)\n> > > reg_num++;\n> > > else if (tag == REG_VALUE) {\n> > > - while (reg_num >=\n> > > - /* assumes registers are in order... */\n> > > - regs[first_reg]) {\n> > > - if (reg_num == regs[first_reg])\n> > > - offsets[first_reg] = current_offset - 1;\n> > > + auto reg = offsets_.find(reg_num);\n> > > +\n> > > + if (reg != offsets_.end()) {\n> > > + offsets_[reg_num] = current_offset - 1;\n> > >\n> > > - if (++first_reg == num_regs)\n> > > + if (++regs_done == offsets_.size())\n> > > return PARSE_OK;\n> > > }\n> > > reg_num++;","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 D2C3DC3220\n\tfor ;\n\tTue, 29 Jun 2021 15:47:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FC51684EC;\n\tTue, 29 Jun 2021 17:47:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1BA4684CB\n\tfor ;\n\tTue, 29 Jun 2021 17:47:04 +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 EF8B54AD;\n\tTue, 29 Jun 2021 17:47:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tQIrek1B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624981624;\n\tbh=juWgdjdk2lalMLR6lgsZ4N3Yj3jPz9y7e3YEwEpAczs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tQIrek1B62b+e0SJkewJhx76pLQ1cemChEpwrSCu9zLkHDY+fzcpLjfxhSC2u5CUQ\n\tX9vTag82Y+PMv64WjCmY/KUUGs2D12ByZMVU9Wxv7ivqpKdbw7FUossy17tlVxTSXF\n\t23e99M/VcFrP9LBAMZ/R9LRcRWHuEcHuSfX1uzRw=","Date":"Tue, 29 Jun 2021 18:47:00 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210629104500.51672-1-naush@raspberrypi.com>\n\t<20210629104500.51672-3-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","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":17923,"web_url":"https://patchwork.libcamera.org/comment/17923/","msgid":"","date":"2021-06-29T18:20:13","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 29 Jun 2021 at 16:47, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hello again,\n>\n> On Tue, Jun 29, 2021 at 05:44:14PM +0300, Laurent Pinchart wrote:\n> > On Tue, Jun 29, 2021 at 02:53:50PM +0100, Naushir Patuck wrote:\n> > > On Tue, 29 Jun 2021 at 11:45, Naushir Patuck \n> wrote:\n> > >\n> > > > Instead of having each CamHelper subclass the MdParserSmia, change\n> the\n> > > > implementation of MdParserSmia to be more generic. The MdParserSmia\n> now gets\n> > > > given a list of registers to search for and helper functions are\n> used to compute\n> > > > exposure lines and gain codes from these registers.\n> > > >\n> > > > Update the imx219 and imx477 CamHelpers by using this new mechanism.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: Laurent Pinchart \n> > > > ---\n> > > > src/ipa/raspberrypi/cam_helper.cpp | 33 +++---\n> > > > src/ipa/raspberrypi/cam_helper.hpp | 2 +\n> > > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------\n> > > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 122\n> ++++------------------\n> > > > src/ipa/raspberrypi/md_parser.hpp | 41 +++++---\n> > > > src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--\n> > > > 6 files changed, 144 insertions(+), 234 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > index 90498c37af98..e6d2258c66d7 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > @@ -159,32 +159,34 @@ unsigned int\n> CamHelper::MistrustFramesModeSwitch() const\n> > > > void CamHelper::parseEmbeddedData(Span buffer,\n> > > > Metadata &metadata)\n> > > > {\n> > > > + MdParser::RegisterMap registers;\n> > > > + Metadata parsedMetadata;\n> > > > +\n> > > > if (buffer.empty())\n> > > > return;\n> > > >\n> > > > - uint32_t exposureLines, gainCode;\n> > > > -\n> > > > - if (parser_->Parse(buffer) != MdParser::Status::OK ||\n> > > > - parser_->GetExposureLines(exposureLines) !=\n> MdParser::Status::OK ||\n> > > > - parser_->GetGainCode(gainCode) != MdParser::Status::OK) {\n> > > > + if (parser_->Parse(buffer, registers) !=\n> MdParser::Status::OK) {\n> > > > LOG(IPARPI, Error) << \"Embedded data buffer parsing\n> failed\";\n> > > > return;\n> > > > }\n> > > >\n> > > > + PopulateMetadata(registers, parsedMetadata);\n> > > > + metadata.Merge(parsedMetadata);\n> > > > +\n> > > > /*\n> > > > - * Overwrite the exposure/gain values in the DeviceStatus, as\n> > > > - * we know better. Fetch it first in case any other fields\n> were\n> > > > - * set meaningfully.\n> > > > + * Overwrite the exposure/gain values in the existing\n> DeviceStatus with\n> > > > + * values from the parsed embedded buffer. Fetch it first in\n> case any\n> > > > + * other fields were set meaningfully.\n> > > > */\n> > > > - DeviceStatus deviceStatus;\n> > > > -\n> > > > - if (metadata.Get(\"device.status\", deviceStatus) != 0) {\n> > > > + DeviceStatus deviceStatus, parsedDeviceStatus;\n> > > > + if (metadata.Get(\"device.status\", deviceStatus) ||\n> > > > + parsedMetadata.Get(\"device.status\", parsedDeviceStatus))\n> {\n> > > > LOG(IPARPI, Error) << \"DeviceStatus not found\";\n> > > > return;\n> > > > }\n> > > >\n> > > > - deviceStatus.shutter_speed = Exposure(exposureLines);\n> > > > - deviceStatus.analogue_gain = Gain(gainCode);\n> > > > + deviceStatus.shutter_speed =\n> parsedDeviceStatus.shutter_speed;\n> > > > + deviceStatus.analogue_gain =\n> parsedDeviceStatus.analogue_gain;\n> > > >\n> > > > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > > > << deviceStatus.shutter_speed\n> > > > @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span uint8_t> buffer,\n> > > > metadata.Set(\"device.status\", deviceStatus);\n> > > > }\n> > > >\n> > > > +void CamHelper::PopulateMetadata([[maybe_unused]] const\n> MdParser::RegisterMap ®isters,\n> > > > + [[maybe_unused]] Metadata\n> &metadata) const\n> > > > +{\n> > > > +}\n> > > > +\n> > > > RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> > > > CamHelperCreateFunc create_func)\n> > > > {\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> > > > index fc3139e22be0..200cc83f3872 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > > @@ -92,6 +92,8 @@ public:\n> > > > protected:\n> > > > void parseEmbeddedData(libcamera::Span buffer,\n> > > > Metadata &metadata);\n> > > > + virtual void PopulateMetadata(const MdParser::RegisterMap\n> ®isters,\n> > > > + Metadata &metadata) const;\n> > > >\n> > > > std::unique_ptr parser_;\n> > > > CameraMode mode_;\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > index c85044a5fa6d..4e4994188ff3 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > @@ -23,21 +23,14 @@\n> > > >\n> > > > using namespace RPiController;\n> > > >\n> > > > -/* Metadata parser implementation specific to Sony IMX219 sensors.\n> */\n> > > > -\n> > > > -class MdParserImx219 : public MdParserSmia\n> > > > -{\n> > > > -public:\n> > > > - MdParserImx219();\n> > > > - Status Parse(libcamera::Span buffer) override;\n> > > > - Status GetExposureLines(unsigned int &lines) override;\n> > > > - Status GetGainCode(unsigned int &gain_code) override;\n> > > > -private:\n> > > > - /* Offset of the register's value in the metadata block. */\n> > > > - int reg_offsets_[3];\n> > > > - /* Value of the register, once read from the metadata block.\n> */\n> > > > - int reg_values_[3];\n> > > > -};\n> > > > +/*\n> > > > + * We care about one gain register and a pair of exposure\n> registers. Their I2C\n> > > > + * addresses from the Sony IMX219 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 = {\n> expHiReg, expLoReg, gainReg };\n>\n> This needs a [[maybe_unused]] in case ENABLE_EMBEDDED_DATA isn't\n> defined.\n>\n> > > >\n> > > > class CamHelperImx219 : public CamHelper\n> > > > {\n> > > > @@ -54,11 +47,14 @@ private:\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> > > > CamHelperImx219::CamHelperImx219()\n> > > > #if ENABLE_EMBEDDED_DATA\n> > > > - : CamHelper(std::make_unique(),\n> frameIntegrationDiff)\n> > > > + : CamHelper(std::make_unique(registerList),\n> frameIntegrationDiff)\n> > > > #else\n> > > > : CamHelper({}, frameIntegrationDiff)\n> > > > #endif\n> > > > @@ -90,88 +86,20 @@ bool\n> CamHelperImx219::SensorEmbeddedDataPresent() const\n> > > > return ENABLE_EMBEDDED_DATA;\n> > > > }\n> > > >\n> > > > -static CamHelper *Create()\n> > > > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap\n> ®isters,\n> > > > + Metadata &metadata) const\n> > > > {\n> > > > - return new CamHelperImx219();\n> > > > -}\n> > > > + DeviceStatus deviceStatus;\n> > > >\n> > > > -static RegisterCamHelper reg(\"imx219\", &Create);\n> > > > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg)\n> * 256 + registers.at(expLoReg));\n> > > > + deviceStatus.analogue_gain = Gain(registers.at(gainReg));\n> > > >\n> > > > -/*\n> > > > - * We care about one gain register and a pair of exposure\n> registers. Their I2C\n> > > > - * addresses from the Sony IMX219 datasheet:\n> > > > - */\n> > > > -#define GAIN_REG 0x157\n> > > > -#define EXPHI_REG 0x15A\n> > > > -#define EXPLO_REG 0x15B\n> > > > -\n> > > > -/*\n> > > > - * Index of each into the reg_offsets and reg_values arrays. Must\n> be in\n> > > > - * register address order.\n> > > > - */\n> > > > -#define GAIN_INDEX 0\n> > > > -#define EXPHI_INDEX 1\n> > > > -#define EXPLO_INDEX 2\n> > > > -\n> > > > -MdParserImx219::MdParserImx219()\n> > > > -{\n> > > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > > > + metadata.Set(\"device.status\", deviceStatus);\n> > > > }\n> > > >\n> > > > -MdParser::Status MdParserImx219::Parse(libcamera::Span uint8_t> buffer)\n> > > > -{\n> > > > - bool try_again = false;\n> > > > -\n> > > > - if (reset_) {\n> > > > - /*\n> > > > - * Search again through the metadata for the gain\n> and exposure\n> > > > - * registers.\n> > > > - */\n> > > > - assert(bits_per_pixel_);\n> > > > - /* Need to be ordered */\n> > > > - uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG\n> };\n> > > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2]\n> = -1;\n> > > > - int ret = static_cast(findRegs(buffer,\n> > > > - regs,\n> reg_offsets_, 3));\n> > > > - /*\n> > > > - * > 0 means \"worked partially but parse again next\n> time\",\n> > > > - * < 0 means \"hard error\".\n> > > > - */\n> > > > - if (ret > 0)\n> > > > - try_again = true;\n> > > > - else if (ret < 0)\n> > > > - return ERROR;\n> > > > - }\n> > > > -\n> > > > - for (int i = 0; i < 3; i++) {\n> > > > - if (reg_offsets_[i] == -1)\n> > > > - continue;\n> > > > -\n> > > > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > > > - }\n> > > > -\n> > > > - /* Re-parse next time if we were unhappy in some way. */\n> > > > - reset_ = try_again;\n> > > > -\n> > > > - return OK;\n> > > > -}\n> > > > -\n> > > > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int\n> &lines)\n> > > > +static CamHelper *Create()\n> > > > {\n> > > > - if (reg_offsets_[EXPHI_INDEX] == -1 ||\n> reg_offsets_[EXPLO_INDEX] == -1)\n> > > > - return NOTFOUND;\n> > > > -\n> > > > - lines = reg_values_[EXPHI_INDEX] * 256 +\n> reg_values_[EXPLO_INDEX];\n> > > > -\n> > > > - return OK;\n> > > > + return new CamHelperImx219();\n> > > > }\n> > > >\n> > > > -MdParser::Status MdParserImx219::GetGainCode(unsigned int\n> &gain_code)\n> > > > -{\n> > > > - if (reg_offsets_[GAIN_INDEX] == -1)\n> > > > - return NOTFOUND;\n> > > > -\n> > > > - gain_code = reg_values_[GAIN_INDEX];\n> > > > -\n> > > > - return OK;\n> > > > -}\n> > > > +static RegisterCamHelper reg(\"imx219\", &Create);\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > > index d72a9be0438e..efd1a5893db8 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > > @@ -15,21 +15,15 @@\n> > > >\n> > > > using namespace RPiController;\n> > > >\n> > > > -/* Metadata parser implementation specific to Sony IMX477 sensors.\n> */\n> > > > -\n> > > > -class MdParserImx477 : public MdParserSmia\n> > > > -{\n> > > > -public:\n> > > > - MdParserImx477();\n> > > > - Status Parse(libcamera::Span buffer) override;\n> > > > - Status GetExposureLines(unsigned int &lines) override;\n> > > > - Status GetGainCode(unsigned int &gain_code) override;\n> > > > -private:\n> > > > - /* Offset of the register's value in the metadata block. */\n> > > > - int reg_offsets_[4];\n> > > > - /* Value of the register, once read from the metadata block.\n> */\n> > > > - int reg_values_[4];\n> > > > -};\n> > > > +/*\n> > > > + * We care about two gain registers and a pair of exposure\n> registers. Their\n> > > > + * I2C addresses from the Sony IMX477 datasheet:\n> > > > + */\n> > > > +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>\n> Same here.\n>\n> > > >\n> > > > class CamHelperImx477 : public CamHelper\n> > > > {\n> > > > @@ -47,10 +41,13 @@ private:\n> > > > * in units of lines.\n> > > > */\n> > > > static constexpr int frameIntegrationDiff = 22;\n> > > > +\n> > > > + void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > > > + Metadata &metadata) const override;\n> > > > };\n> > > >\n> > > > CamHelperImx477::CamHelperImx477()\n> > > > - : CamHelper(std::make_unique(),\n> frameIntegrationDiff)\n> > > > + : CamHelper(std::make_unique(registerList),\n> frameIntegrationDiff)\n> > > > {\n> > > > }\n> > > >\n> > > > @@ -77,95 +74,20 @@ bool\n> CamHelperImx477::SensorEmbeddedDataPresent() const\n> > > > return true;\n> > > > }\n> > > >\n> > > > -static CamHelper *Create()\n> > > > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap\n> ®isters,\n> > > > + Metadata &metadata) const\n> > > > {\n> > > > - return new CamHelperImx477();\n> > > > -}\n> > > > + DeviceStatus deviceStatus;\n> > > >\n> > > > -static RegisterCamHelper reg(\"imx477\", &Create);\n> > > > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg)\n> * 256 + registers.at(expLoReg));\n> > > > + deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) *\n> 256 + registers.at(gainLoReg));\n> > > >\n> > > > -/*\n> > > > - * We care about two gain registers and a pair of exposure\n> registers. Their\n> > > > - * I2C addresses from the Sony IMX477 datasheet:\n> > > > - */\n> > > > -#define EXPHI_REG 0x0202\n> > > > -#define EXPLO_REG 0x0203\n> > > > -#define GAINHI_REG 0x0204\n> > > > -#define GAINLO_REG 0x0205\n> > > > -\n> > > > -/*\n> > > > - * Index of each into the reg_offsets and reg_values arrays. Must\n> be in register\n> > > > - * address order.\n> > > > - */\n> > > > -#define EXPHI_INDEX 0\n> > > > -#define EXPLO_INDEX 1\n> > > > -#define GAINHI_INDEX 2\n> > > > -#define GAINLO_INDEX 3\n> > > > -\n> > > > -MdParserImx477::MdParserImx477()\n> > > > -{\n> > > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =\n> reg_offsets_[3] = -1;\n> > > > + metadata.Set(\"device.status\", deviceStatus);\n> > > > }\n> > > >\n> > > > -MdParser::Status MdParserImx477::Parse(libcamera::Span uint8_t> buffer)\n> > > > -{\n> > > > - bool try_again = false;\n> > > > -\n> > > > - if (reset_) {\n> > > > - /*\n> > > > - * Search again through the metadata for the gain\n> and exposure\n> > > > - * registers.\n> > > > - */\n> > > > - assert(bits_per_pixel_);\n> > > > - /* Need to be ordered */\n> > > > - uint32_t regs[4] = {\n> > > > - EXPHI_REG,\n> > > > - EXPLO_REG,\n> > > > - GAINHI_REG,\n> > > > - GAINLO_REG\n> > > > - };\n> > > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2]\n> = reg_offsets_[3] = -1;\n> > > > - int ret = static_cast(findRegs(buffer,\n> > > > - regs,\n> reg_offsets_, 4));\n> > > > - /*\n> > > > - * > 0 means \"worked partially but parse again next\n> time\",\n> > > > - * < 0 means \"hard error\".\n> > > > - */\n> > > > - if (ret > 0)\n> > > > - try_again = true;\n> > > > - else if (ret < 0)\n> > > > - return ERROR;\n> > > > - }\n> > > > -\n> > > > - for (int i = 0; i < 4; i++) {\n> > > > - if (reg_offsets_[i] == -1)\n> > > > - continue;\n> > > > -\n> > > > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > > > - }\n> > > > -\n> > > > - /* Re-parse next time if we were unhappy in some way. */\n> > > > - reset_ = try_again;\n> > > > -\n> > > > - return OK;\n> > > > -}\n> > > > -\n> > > > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int\n> &lines)\n> > > > +static CamHelper *Create()\n> > > > {\n> > > > - if (reg_offsets_[EXPHI_INDEX] == -1 ||\n> reg_offsets_[EXPLO_INDEX] == -1)\n> > > > - return NOTFOUND;\n> > > > -\n> > > > - lines = reg_values_[EXPHI_INDEX] * 256 +\n> reg_values_[EXPLO_INDEX];\n> > > > -\n> > > > - return OK;\n> > > > + return new CamHelperImx477();\n> > > > }\n> > > >\n> > > > -MdParser::Status MdParserImx477::GetGainCode(unsigned int\n> &gain_code)\n> > > > -{\n> > > > - if (reg_offsets_[GAINHI_INDEX] == -1 ||\n> reg_offsets_[GAINLO_INDEX] == -1)\n> > > > - return NOTFOUND;\n> > > > -\n> > > > - gain_code = reg_values_[GAINHI_INDEX] * 256 +\n> > > > reg_values_[GAINLO_INDEX];\n> > > > -\n> > > > - return OK;\n> > > > -}\n> > > > +static RegisterCamHelper reg(\"imx477\", &Create);\n> > > > diff --git a/src/ipa/raspberrypi/md_parser.hpp\n> b/src/ipa/raspberrypi/md_parser.hpp\n> > > > index 8497216f8db7..e3e2738537a8 100644\n> > > > --- a/src/ipa/raspberrypi/md_parser.hpp\n> > > > +++ b/src/ipa/raspberrypi/md_parser.hpp\n> > > > @@ -6,6 +6,9 @@\n> > > > */\n> > > > #pragma once\n> > > >\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \n> > > > @@ -19,7 +22,7 @@\n> > > > * application code doesn't have to worry which kind to\n> instantiate. But for\n> > > > * the sake of example let's suppose we're parsing imx219 metadata.\n> > > > *\n> > > > - * MdParser *parser = new MdParserImx219(); // for example\n> > > > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg,\n> gainReg });\n> > > > * parser->SetBitsPerPixel(bpp);\n> > > > * parser->SetLineLengthBytes(pitch);\n> > > > * parser->SetNumLines(2);\n> > > > @@ -32,13 +35,11 @@\n> > > > *\n> > > > * Then on every frame:\n> > > > *\n> > > > - * if (parser->Parse(buffer) != MdParser::OK)\n> > > > + * RegisterMap registers;\n> > > > + * if (parser->Parse(buffer, registers) != MdParser::OK)\n> > > > * much badness;\n> > > > - * unsigned int exposure_lines, gain_code\n> > > > - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)\n> > > > - * exposure was not found;\n> > > > - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> > > > - * gain code was not found;\n> > > > + * Metadata metadata;\n> > > > + * CamHelper::PopulateMetadata(registers, metadata);\n> > > > *\n> > > > * (Note that the CamHelper class converts to/from exposure lines\n> and time,\n> > > > * and gain_code / actual gain.)\n> > > > @@ -59,6 +60,8 @@ namespace RPiController {\n> > > > class MdParser\n> > > > {\n> > > > public:\n> > > > + using RegisterMap = std::map;\n> > > > +\n> > > > /*\n> > > > * Parser status codes:\n> > > > * OK - success\n> > > > @@ -98,9 +101,8 @@ public:\n> > > > line_length_bytes_ = num_bytes;\n> > > > }\n> > > >\n> > > > - virtual Status Parse(libcamera::Span buffer)\n> = 0;\n> > > > - virtual Status GetExposureLines(unsigned int &lines) = 0;\n> > > > - virtual Status GetGainCode(unsigned int &gain_code) = 0;\n> > > > + virtual Status Parse(libcamera::Span buffer,\n> > > > + RegisterMap ®isters) = 0;\n> > > >\n> > > > protected:\n> > > > bool reset_;\n> > > > @@ -116,14 +118,18 @@ protected:\n> > > > * md_parser_imx219.cpp for an example).\n> > > > */\n> > > >\n> > > > -class MdParserSmia : public MdParser\n> > > > +class MdParserSmia final : public MdParser\n> > > > {\n> > > > public:\n> > > > - MdParserSmia() : MdParser()\n> > > > - {\n> > > > - }\n> > > > + MdParserSmia(std::initializer_list registerList);\n> > > > +\n> > > > + MdParser::Status Parse(libcamera::Span buffer,\n> > > > + RegisterMap ®isters) override;\n> > > > +\n> > > > +private:\n> > > > + /* Maps register address to offset in the buffer. */\n> > > > + using OffsetMap = std::map std::optional>;\n> > > >\n> > > > -protected:\n> > > > /*\n> > > > * Note that error codes > 0 are regarded as non-fatal;\n> codes < 0\n> > > > * indicate a bad data buffer. Status codes are:\n> > > > @@ -141,8 +147,9 @@ protected:\n> > > > BAD_PADDING = -5\n> > > > };\n> > > >\n> > > > - ParseStatus findRegs(libcamera::Span buffer,\n> uint32_t regs[],\n> > > > - int offsets[], unsigned int num_regs);\n> > > > + ParseStatus findRegs(libcamera::Span buffer);\n> > > > +\n> > > > + OffsetMap offsets_;\n> > > > };\n> > > >\n> > > > } // namespace RPi\n> > > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp\n> b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > > index 0a14875575a2..7f72fc03948f 100644\n> > > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > > @@ -6,9 +6,11 @@\n> > > > */\n> > > > #include \n> > > >\n> > >\n> > > Oops, this header is not needed any more!\n> > >\n> > > > +#include \"libcamera/base/log.h\"\n> >\n> > And this should be . I'll fix when applying.\n> >\n> > > > #include \"md_parser.hpp\"\n> > > >\n> > > > using namespace RPiController;\n> > > > +using namespace libcamera;\n> > > >\n> > > > /*\n> > > > * This function goes through the embedded data to find the offsets\n> (not\n> > > > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;\n> > > > constexpr unsigned int REG_VALUE = 0x5a;\n> > > > constexpr unsigned int REG_SKIP = 0x55;\n> > > >\n> > > > -MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span buffer,\n> > > > - uint32_t regs[],\n> int offsets[],\n> > > > - unsigned int\n> num_regs)\n> > > > +MdParserSmia::MdParserSmia(std::initializer_list\n> registerList)\n> > > > {\n> > > > - assert(num_regs > 0);\n> > > > + for (auto r : registerList)\n> > > > + offsets_[r] = {};\n> > > > +}\n> > > > +\n> > > > +MdParser::Status MdParserSmia::Parse(libcamera::Span\n> buffer,\n> > > > + RegisterMap ®isters)\n> > > > +{\n> > > > + if (reset_) {\n> > > > + /*\n> > > > + * Search again through the metadata for all the\n> registers\n> > > > + * requested.\n> > > > + */\n> > > > + ASSERT(bits_per_pixel_);\n> > > > +\n> > > > + for (const auto &[reg, offset] : offsets_)\n> > > > + offsets_[reg] = {};\n>\n> I'm afraid this causes compilation failures with gcc 7 and gcc 8, due to\n> offset being unused. We can't throw a [[maybe_unused]] here, that's not\n> supported, so we need to revert to the syntax from v2.\n>\n> for (const auto &kv : offsets_)\n> offsets_[kv.first] = {};\n>\n> I'll post a v3.1, Naush, could you give it a try ?\n>\n\nThank you for the fixups! I'm a bit curious why my compiler did not throw\nout warnings\nfor these though...?\n\nI'll test and report back first thing tomorrow.\n\nRegards,\nNaush\n\n\n> > > > +\n> > > > + ParseStatus ret = findRegs(buffer);\n> > > > + /*\n> > > > + * > 0 means \"worked partially but parse again next\n> time\",\n> > > > + * < 0 means \"hard error\".\n> > > > + *\n> > > > + * In either case, we retry parsing on the next\n> frame.\n> > > > + */\n> > > > + if (ret != PARSE_OK)\n> > > > + return ERROR;\n> > > > +\n> > > > + reset_ = false;\n> > > > + }\n> > > > +\n> > > > + /* Populate the register values requested. */\n> > > > + registers.clear();\n> > > > + for (const auto &[reg, offset] : offsets_) {\n> > > > + if (!offset) {\n> > > > + reset_ = true;\n> > > > + return NOTFOUND;\n> > > > + }\n> > > > + registers[reg] = buffer[offset.value()];\n> > > > + }\n> > > > +\n> > > > + return OK;\n> > > > +}\n> > > > +\n> > > > +MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span buffer)\n> > > > +{\n> > > > + ASSERT(offsets_.size());\n> > > >\n> > > > if (buffer[0] != LINE_START)\n> > > > return NO_LINE_START;\n> > > >\n> > > > unsigned int current_offset = 1; /* after the LINE_START */\n> > > > unsigned int current_line_start = 0, current_line = 0;\n> > > > - unsigned int reg_num = 0, first_reg = 0;\n> > > > + unsigned int reg_num = 0, regs_done = 0;\n> > > >\n> > > > while (1) {\n> > > > int tag = buffer[current_offset++];\n> > > > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span\n> > > > else if (tag == REG_SKIP)\n> > > > reg_num++;\n> > > > else if (tag == REG_VALUE) {\n> > > > - while (reg_num >=\n> > > > - /* assumes registers are in\n> order... */\n> > > > - regs[first_reg]) {\n> > > > - if (reg_num ==\n> regs[first_reg])\n> > > > - offsets[first_reg] =\n> current_offset - 1;\n> > > > + auto reg = offsets_.find(reg_num);\n> > > > +\n> > > > + if (reg != offsets_.end()) {\n> > > > + offsets_[reg_num] =\n> current_offset - 1;\n> > > >\n> > > > - if (++first_reg == num_regs)\n> > > > + if (++regs_done ==\n> offsets_.size())\n> > > > return PARSE_OK;\n> > > > }\n> > > > reg_num++;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EC265C3220\n\tfor ;\n\tTue, 29 Jun 2021 18:20:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B264684EC;\n\tTue, 29 Jun 2021 20:20:33 +0200 (CEST)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B9F7684CB\n\tfor ;\n\tTue, 29 Jun 2021 20:20:31 +0200 (CEST)","by mail-lf1-x12e.google.com with SMTP id a15so33237073lfr.6\n\tfor ;\n\tTue, 29 Jun 2021 11:20:31 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"mPiDephF\"; 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=Vv9vDsPL9CcktYJNd53l5OBMOa6ay/j+gxLR3cMknK4=;\n\tb=mPiDephFDp4fOQ7kYEOLqQYpeGs9mDJVgBPghQ41uG1tWO7KOpVGj13+R8mvjcQwhs\n\thZk/W6pJ0zqY60unCDge4XhteWvGHtz8a3CQTlbVGRfAKv1IeMROOmzr2lbx8jN5mlqi\n\tSoKZt7GTTj5B0reeMhVGBXw76hCnT6KnjaItG2dyeSnagIAvFSmlvTI0g6pShEedCtws\n\tcaTAvxG1gvkrmSf0VwFIW/j/D5sI7oxwWJzvjV+v5bG0TaTMDhj74XBQ21LrScJ2pJvK\n\t0vkoLWVcHsU7DbifQOaHB0r8Qw3uc9J1PT1qanpmCva0njn10sGYjLl2MKsOcmipy8yP\n\txVDQ==","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=Vv9vDsPL9CcktYJNd53l5OBMOa6ay/j+gxLR3cMknK4=;\n\tb=fsLrTXbTx27XGBw70UIo/HwZFAk/EVeC7rOpbq619TJHXKJrUd+tHD4tBDMpL1/nvh\n\tFBg3pZMHS8jSZrrwPOT0WgJxQw6lyfBp4E5sx1uDBgeMldgvR9d8RkEJqo1Fg38olQP4\n\tqjXf/aDEWBl9g9UGPJJ0J5DyHJY8rBjsJ71RUrtrBi0vdA5WPfSSWtD7qqawTzISe24U\n\t5oDR04Z5Fizk3c0q93m7s7RTihXOi1S3xpy+Q8Iz40kuGYHpgTVcZ6qRwpHWZn2iVf/L\n\tUliMNoQTdoBP0PrAHffY99ZD0BV5lE3qQ4UBBblfpVI6QyabJX00ma+qrff0QTMCB0S+\n\t52MQ==","X-Gm-Message-State":"AOAM532bGIfQXFzRZa0XqLXILYRSOzWoZ3dN0vl8BAr2N9R6O2GXcpNp\n\tmNSdwAoXq58F+8EGLkjnou6XaYPrjWZLGwrQ+uHbQw==","X-Google-Smtp-Source":"ABdhPJyHtUu8QEMpuV2BuD17ur300AHV+ifaFYKognrgkslvmV4NIMdOozloKXw4n/QtXSLlvkPQ12Xx7XHSy+qDUP8=","X-Received":"by 2002:ac2:4db6:: with SMTP id\n\th22mr23087370lfe.171.1624990830450; \n\tTue, 29 Jun 2021 11:20:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20210629104500.51672-1-naush@raspberrypi.com>\n\t<20210629104500.51672-3-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 29 Jun 2021 19:20:13 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000036729705c5eba9ca\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","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":17924,"web_url":"https://patchwork.libcamera.org/comment/17924/","msgid":"","date":"2021-06-29T19:29:45","subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Jun 29, 2021 at 07:20:13PM +0100, Naushir Patuck wrote:\n> On Tue, 29 Jun 2021 at 16:47, Laurent Pinchart wrote:\n> > On Tue, Jun 29, 2021 at 05:44:14PM +0300, Laurent Pinchart wrote:\n> > > On Tue, Jun 29, 2021 at 02:53:50PM +0100, Naushir Patuck wrote:\n> > > > On Tue, 29 Jun 2021 at 11:45, Naushir Patuck wrote:\n> > > >\n> > > > > Instead of having each CamHelper subclass the MdParserSmia, change the\n> > > > > implementation of MdParserSmia to be more generic. The MdParserSmia now gets\n> > > > > given a list of registers to search for and helper functions are used to compute\n> > > > > exposure lines and gain codes from these registers.\n> > > > >\n> > > > > Update the imx219 and imx477 CamHelpers by using this new mechanism.\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > Reviewed-by: Laurent Pinchart \n> > > > > ---\n> > > > > src/ipa/raspberrypi/cam_helper.cpp | 33 +++---\n> > > > > src/ipa/raspberrypi/cam_helper.hpp | 2 +\n> > > > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------\n> > > > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------\n> > > > > src/ipa/raspberrypi/md_parser.hpp | 41 +++++---\n> > > > > src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--\n> > > > > 6 files changed, 144 insertions(+), 234 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > > index 90498c37af98..e6d2258c66d7 100644\n> > > > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > > @@ -159,32 +159,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n> > > > > void CamHelper::parseEmbeddedData(Span buffer,\n> > > > > Metadata &metadata)\n> > > > > {\n> > > > > + MdParser::RegisterMap registers;\n> > > > > + Metadata parsedMetadata;\n> > > > > +\n> > > > > if (buffer.empty())\n> > > > > return;\n> > > > >\n> > > > > - uint32_t exposureLines, gainCode;\n> > > > > -\n> > > > > - if (parser_->Parse(buffer) != MdParser::Status::OK ||\n> > > > > - parser_->GetExposureLines(exposureLines) != MdParser::Status::OK ||\n> > > > > - parser_->GetGainCode(gainCode) != MdParser::Status::OK) {\n> > > > > + if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {\n> > > > > LOG(IPARPI, Error) << \"Embedded data buffer parsing failed\";\n> > > > > return;\n> > > > > }\n> > > > >\n> > > > > + PopulateMetadata(registers, parsedMetadata);\n> > > > > + metadata.Merge(parsedMetadata);\n> > > > > +\n> > > > > /*\n> > > > > - * Overwrite the exposure/gain values in the DeviceStatus, as\n> > > > > - * we know better. Fetch it first in case any other fields were\n> > > > > - * set meaningfully.\n> > > > > + * Overwrite the exposure/gain values in the existing DeviceStatus with\n> > > > > + * values from the parsed embedded buffer. Fetch it first in case any\n> > > > > + * other fields were set meaningfully.\n> > > > > */\n> > > > > - DeviceStatus deviceStatus;\n> > > > > -\n> > > > > - if (metadata.Get(\"device.status\", deviceStatus) != 0) {\n> > > > > + DeviceStatus deviceStatus, parsedDeviceStatus;\n> > > > > + if (metadata.Get(\"device.status\", deviceStatus) ||\n> > > > > + parsedMetadata.Get(\"device.status\", parsedDeviceStatus)) {\n> > > > > LOG(IPARPI, Error) << \"DeviceStatus not found\";\n> > > > > return;\n> > > > > }\n> > > > >\n> > > > > - deviceStatus.shutter_speed = Exposure(exposureLines);\n> > > > > - deviceStatus.analogue_gain = Gain(gainCode);\n> > > > > + deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> > > > > + deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> > > > >\n> > > > > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > > > > << deviceStatus.shutter_speed\n> > > > > @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span buffer,\n> > > > > metadata.Set(\"device.status\", deviceStatus);\n> > > > > }\n> > > > >\n> > > > > +void CamHelper::PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap ®isters,\n> > > > > + [[maybe_unused]] Metadata &metadata) const\n> > > > > +{\n> > > > > +}\n> > > > > +\n> > > > > RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> > > > > CamHelperCreateFunc create_func)\n> > > > > {\n> > > > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> > > > > index fc3139e22be0..200cc83f3872 100644\n> > > > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > > > @@ -92,6 +92,8 @@ public:\n> > > > > protected:\n> > > > > void parseEmbeddedData(libcamera::Span buffer,\n> > > > > Metadata &metadata);\n> > > > > + virtual void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > > > > + Metadata &metadata) const;\n> > > > >\n> > > > > std::unique_ptr parser_;\n> > > > > CameraMode mode_;\n> > > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > > index c85044a5fa6d..4e4994188ff3 100644\n> > > > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > > @@ -23,21 +23,14 @@\n> > > > >\n> > > > > using namespace RPiController;\n> > > > >\n> > > > > -/* Metadata parser implementation specific to Sony IMX219 sensors. */\n> > > > > -\n> > > > > -class MdParserImx219 : public MdParserSmia\n> > > > > -{\n> > > > > -public:\n> > > > > - MdParserImx219();\n> > > > > - Status Parse(libcamera::Span buffer) override;\n> > > > > - Status GetExposureLines(unsigned int &lines) override;\n> > > > > - Status GetGainCode(unsigned int &gain_code) override;\n> > > > > -private:\n> > > > > - /* Offset of the register's value in the metadata block. */\n> > > > > - int reg_offsets_[3];\n> > > > > - /* Value of the register, once read from the metadata block. */\n> > > > > - int reg_values_[3];\n> > > > > -};\n> > > > > +/*\n> > > > > + * We care about one gain register and a pair of exposure registers. Their I2C\n> > > > > + * addresses from the Sony IMX219 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 = { expHiReg, expLoReg, gainReg };\n> >\n> > This needs a [[maybe_unused]] in case ENABLE_EMBEDDED_DATA isn't defined.\n> >\n> > > > >\n> > > > > class CamHelperImx219 : public CamHelper\n> > > > > {\n> > > > > @@ -54,11 +47,14 @@ private:\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> > > > > CamHelperImx219::CamHelperImx219()\n> > > > > #if ENABLE_EMBEDDED_DATA\n> > > > > - : CamHelper(std::make_unique(), frameIntegrationDiff)\n> > > > > + : CamHelper(std::make_unique(registerList), frameIntegrationDiff)\n> > > > > #else\n> > > > > : CamHelper({}, frameIntegrationDiff)\n> > > > > #endif\n> > > > > @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const\n> > > > > return ENABLE_EMBEDDED_DATA;\n> > > > > }\n> > > > >\n> > > > > -static CamHelper *Create()\n> > > > > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > > > > + Metadata &metadata) const\n> > > > > {\n> > > > > - return new CamHelperImx219();\n> > > > > -}\n> > > > > + DeviceStatus deviceStatus;\n> > > > >\n> > > > > -static RegisterCamHelper reg(\"imx219\", &Create);\n> > > > > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> > > > > + deviceStatus.analogue_gain = Gain(registers.at(gainReg));\n> > > > >\n> > > > > -/*\n> > > > > - * We care about one gain register and a pair of exposure registers. Their I2C\n> > > > > - * addresses from the Sony IMX219 datasheet:\n> > > > > - */\n> > > > > -#define GAIN_REG 0x157\n> > > > > -#define EXPHI_REG 0x15A\n> > > > > -#define EXPLO_REG 0x15B\n> > > > > -\n> > > > > -/*\n> > > > > - * Index of each into the reg_offsets and reg_values arrays. Must be in\n> > > > > - * register address order.\n> > > > > - */\n> > > > > -#define GAIN_INDEX 0\n> > > > > -#define EXPHI_INDEX 1\n> > > > > -#define EXPLO_INDEX 2\n> > > > > -\n> > > > > -MdParserImx219::MdParserImx219()\n> > > > > -{\n> > > > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > > > > + metadata.Set(\"device.status\", deviceStatus);\n> > > > > }\n> > > > >\n> > > > > -MdParser::Status MdParserImx219::Parse(libcamera::Span buffer)\n> > > > > -{\n> > > > > - bool try_again = false;\n> > > > > -\n> > > > > - if (reset_) {\n> > > > > - /*\n> > > > > - * Search again through the metadata for the gain and exposure\n> > > > > - * registers.\n> > > > > - */\n> > > > > - assert(bits_per_pixel_);\n> > > > > - /* Need to be ordered */\n> > > > > - uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };\n> > > > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > > > > - int ret = static_cast(findRegs(buffer,\n> > > > > - regs, reg_offsets_, 3));\n> > > > > - /*\n> > > > > - * > 0 means \"worked partially but parse again next time\",\n> > > > > - * < 0 means \"hard error\".\n> > > > > - */\n> > > > > - if (ret > 0)\n> > > > > - try_again = true;\n> > > > > - else if (ret < 0)\n> > > > > - return ERROR;\n> > > > > - }\n> > > > > -\n> > > > > - for (int i = 0; i < 3; i++) {\n> > > > > - if (reg_offsets_[i] == -1)\n> > > > > - continue;\n> > > > > -\n> > > > > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > > > > - }\n> > > > > -\n> > > > > - /* Re-parse next time if we were unhappy in some way. */\n> > > > > - reset_ = try_again;\n> > > > > -\n> > > > > - return OK;\n> > > > > -}\n> > > > > -\n> > > > > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)\n> > > > > +static CamHelper *Create()\n> > > > > {\n> > > > > - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> > > > > - return NOTFOUND;\n> > > > > -\n> > > > > - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> > > > > -\n> > > > > - return OK;\n> > > > > + return new CamHelperImx219();\n> > > > > }\n> > > > >\n> > > > > -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)\n> > > > > -{\n> > > > > - if (reg_offsets_[GAIN_INDEX] == -1)\n> > > > > - return NOTFOUND;\n> > > > > -\n> > > > > - gain_code = reg_values_[GAIN_INDEX];\n> > > > > -\n> > > > > - return OK;\n> > > > > -}\n> > > > > +static RegisterCamHelper reg(\"imx219\", &Create);\n> > > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > > > index d72a9be0438e..efd1a5893db8 100644\n> > > > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > > > @@ -15,21 +15,15 @@\n> > > > >\n> > > > > using namespace RPiController;\n> > > > >\n> > > > > -/* Metadata parser implementation specific to Sony IMX477 sensors. */\n> > > > > -\n> > > > > -class MdParserImx477 : public MdParserSmia\n> > > > > -{\n> > > > > -public:\n> > > > > - MdParserImx477();\n> > > > > - Status Parse(libcamera::Span buffer) override;\n> > > > > - Status GetExposureLines(unsigned int &lines) override;\n> > > > > - Status GetGainCode(unsigned int &gain_code) override;\n> > > > > -private:\n> > > > > - /* Offset of the register's value in the metadata block. */\n> > > > > - int reg_offsets_[4];\n> > > > > - /* Value of the register, once read from the metadata block. */\n> > > > > - int reg_values_[4];\n> > > > > -};\n> > > > > +/*\n> > > > > + * We care about two gain registers and a pair of exposure registers. Their\n> > > > > + * I2C addresses from the Sony IMX477 datasheet:\n> > > > > + */\n> > > > > +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> >\n> > Same here.\n> >\n> > > > >\n> > > > > class CamHelperImx477 : public CamHelper\n> > > > > {\n> > > > > @@ -47,10 +41,13 @@ private:\n> > > > > * in units of lines.\n> > > > > */\n> > > > > static constexpr int frameIntegrationDiff = 22;\n> > > > > +\n> > > > > + void PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > > > > + Metadata &metadata) const override;\n> > > > > };\n> > > > >\n> > > > > CamHelperImx477::CamHelperImx477()\n> > > > > - : CamHelper(std::make_unique(), frameIntegrationDiff)\n> > > > > + : CamHelper(std::make_unique(registerList), frameIntegrationDiff)\n> > > > > {\n> > > > > }\n> > > > >\n> > > > > @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const\n> > > > > return true;\n> > > > > }\n> > > > >\n> > > > > -static CamHelper *Create()\n> > > > > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap ®isters,\n> > > > > + Metadata &metadata) const\n> > > > > {\n> > > > > - return new CamHelperImx477();\n> > > > > -}\n> > > > > + DeviceStatus deviceStatus;\n> > > > >\n> > > > > -static RegisterCamHelper reg(\"imx477\", &Create);\n> > > > > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));\n> > > > > + deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));\n> > > > >\n> > > > > -/*\n> > > > > - * We care about two gain registers and a pair of exposure registers. Their\n> > > > > - * I2C addresses from the Sony IMX477 datasheet:\n> > > > > - */\n> > > > > -#define EXPHI_REG 0x0202\n> > > > > -#define EXPLO_REG 0x0203\n> > > > > -#define GAINHI_REG 0x0204\n> > > > > -#define GAINLO_REG 0x0205\n> > > > > -\n> > > > > -/*\n> > > > > - * Index of each into the reg_offsets and reg_values arrays. Must be in register\n> > > > > - * address order.\n> > > > > - */\n> > > > > -#define EXPHI_INDEX 0\n> > > > > -#define EXPLO_INDEX 1\n> > > > > -#define GAINHI_INDEX 2\n> > > > > -#define GAINLO_INDEX 3\n> > > > > -\n> > > > > -MdParserImx477::MdParserImx477()\n> > > > > -{\n> > > > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> > > > > + metadata.Set(\"device.status\", deviceStatus);\n> > > > > }\n> > > > >\n> > > > > -MdParser::Status MdParserImx477::Parse(libcamera::Span buffer)\n> > > > > -{\n> > > > > - bool try_again = false;\n> > > > > -\n> > > > > - if (reset_) {\n> > > > > - /*\n> > > > > - * Search again through the metadata for the gain and exposure\n> > > > > - * registers.\n> > > > > - */\n> > > > > - assert(bits_per_pixel_);\n> > > > > - /* Need to be ordered */\n> > > > > - uint32_t regs[4] = {\n> > > > > - EXPHI_REG,\n> > > > > - EXPLO_REG,\n> > > > > - GAINHI_REG,\n> > > > > - GAINLO_REG\n> > > > > - };\n> > > > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> > > > > - int ret = static_cast(findRegs(buffer,\n> > > > > - regs, reg_offsets_, 4));\n> > > > > - /*\n> > > > > - * > 0 means \"worked partially but parse again next time\",\n> > > > > - * < 0 means \"hard error\".\n> > > > > - */\n> > > > > - if (ret > 0)\n> > > > > - try_again = true;\n> > > > > - else if (ret < 0)\n> > > > > - return ERROR;\n> > > > > - }\n> > > > > -\n> > > > > - for (int i = 0; i < 4; i++) {\n> > > > > - if (reg_offsets_[i] == -1)\n> > > > > - continue;\n> > > > > -\n> > > > > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > > > > - }\n> > > > > -\n> > > > > - /* Re-parse next time if we were unhappy in some way. */\n> > > > > - reset_ = try_again;\n> > > > > -\n> > > > > - return OK;\n> > > > > -}\n> > > > > -\n> > > > > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)\n> > > > > +static CamHelper *Create()\n> > > > > {\n> > > > > - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> > > > > - return NOTFOUND;\n> > > > > -\n> > > > > - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> > > > > -\n> > > > > - return OK;\n> > > > > + return new CamHelperImx477();\n> > > > > }\n> > > > >\n> > > > > -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)\n> > > > > -{\n> > > > > - if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX] == -1)\n> > > > > - return NOTFOUND;\n> > > > > -\n> > > > > - gain_code = reg_values_[GAINHI_INDEX] * 256 +\n> > > > > reg_values_[GAINLO_INDEX];\n> > > > > -\n> > > > > - return OK;\n> > > > > -}\n> > > > > +static RegisterCamHelper reg(\"imx477\", &Create);\n> > > > > diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp\n> > > > > index 8497216f8db7..e3e2738537a8 100644\n> > > > > --- a/src/ipa/raspberrypi/md_parser.hpp\n> > > > > +++ b/src/ipa/raspberrypi/md_parser.hpp\n> > > > > @@ -6,6 +6,9 @@\n> > > > > */\n> > > > > #pragma once\n> > > > >\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > > @@ -19,7 +22,7 @@\n> > > > > * application code doesn't have to worry which kind to instantiate. But for\n> > > > > * the sake of example let's suppose we're parsing imx219 metadata.\n> > > > > *\n> > > > > - * MdParser *parser = new MdParserImx219(); // for example\n> > > > > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });\n> > > > > * parser->SetBitsPerPixel(bpp);\n> > > > > * parser->SetLineLengthBytes(pitch);\n> > > > > * parser->SetNumLines(2);\n> > > > > @@ -32,13 +35,11 @@\n> > > > > *\n> > > > > * Then on every frame:\n> > > > > *\n> > > > > - * if (parser->Parse(buffer) != MdParser::OK)\n> > > > > + * RegisterMap registers;\n> > > > > + * if (parser->Parse(buffer, registers) != MdParser::OK)\n> > > > > * much badness;\n> > > > > - * unsigned int exposure_lines, gain_code\n> > > > > - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)\n> > > > > - * exposure was not found;\n> > > > > - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> > > > > - * gain code was not found;\n> > > > > + * Metadata metadata;\n> > > > > + * CamHelper::PopulateMetadata(registers, metadata);\n> > > > > *\n> > > > > * (Note that the CamHelper class converts to/from exposure lines and time,\n> > > > > * and gain_code / actual gain.)\n> > > > > @@ -59,6 +60,8 @@ namespace RPiController {\n> > > > > class MdParser\n> > > > > {\n> > > > > public:\n> > > > > + using RegisterMap = std::map;\n> > > > > +\n> > > > > /*\n> > > > > * Parser status codes:\n> > > > > * OK - success\n> > > > > @@ -98,9 +101,8 @@ public:\n> > > > > line_length_bytes_ = num_bytes;\n> > > > > }\n> > > > >\n> > > > > - virtual Status Parse(libcamera::Span buffer) = 0;\n> > > > > - virtual Status GetExposureLines(unsigned int &lines) = 0;\n> > > > > - virtual Status GetGainCode(unsigned int &gain_code) = 0;\n> > > > > + virtual Status Parse(libcamera::Span buffer,\n> > > > > + RegisterMap ®isters) = 0;\n> > > > >\n> > > > > protected:\n> > > > > bool reset_;\n> > > > > @@ -116,14 +118,18 @@ protected:\n> > > > > * md_parser_imx219.cpp for an example).\n> > > > > */\n> > > > >\n> > > > > -class MdParserSmia : public MdParser\n> > > > > +class MdParserSmia final : public MdParser\n> > > > > {\n> > > > > public:\n> > > > > - MdParserSmia() : MdParser()\n> > > > > - {\n> > > > > - }\n> > > > > + MdParserSmia(std::initializer_list registerList);\n> > > > > +\n> > > > > + MdParser::Status Parse(libcamera::Span buffer,\n> > > > > + RegisterMap ®isters) override;\n> > > > > +\n> > > > > +private:\n> > > > > + /* Maps register address to offset in the buffer. */\n> > > > > + using OffsetMap = std::map>;\n> > > > >\n> > > > > -protected:\n> > > > > /*\n> > > > > * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> > > > > * indicate a bad data buffer. Status codes are:\n> > > > > @@ -141,8 +147,9 @@ protected:\n> > > > > BAD_PADDING = -5\n> > > > > };\n> > > > >\n> > > > > - ParseStatus findRegs(libcamera::Span buffer, uint32_t regs[],\n> > > > > - int offsets[], unsigned int num_regs);\n> > > > > + ParseStatus findRegs(libcamera::Span buffer);\n> > > > > +\n> > > > > + OffsetMap offsets_;\n> > > > > };\n> > > > >\n> > > > > } // namespace RPi\n> > > > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > > > index 0a14875575a2..7f72fc03948f 100644\n> > > > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > > > @@ -6,9 +6,11 @@\n> > > > > */\n> > > > > #include \n> > > > >\n> > > >\n> > > > Oops, this header is not needed any more!\n> > > >\n> > > > > +#include \"libcamera/base/log.h\"\n> > >\n> > > And this should be . I'll fix when applying.\n> > >\n> > > > > #include \"md_parser.hpp\"\n> > > > >\n> > > > > using namespace RPiController;\n> > > > > +using namespace libcamera;\n> > > > >\n> > > > > /*\n> > > > > * This function goes through the embedded data to find the offsets (not\n> > > > > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;\n> > > > > constexpr unsigned int REG_VALUE = 0x5a;\n> > > > > constexpr unsigned int REG_SKIP = 0x55;\n> > > > >\n> > > > > -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer,\n> > > > > - uint32_t regs[], int offsets[],\n> > > > > - unsigned int num_regs)\n> > > > > +MdParserSmia::MdParserSmia(std::initializer_list registerList)\n> > > > > {\n> > > > > - assert(num_regs > 0);\n> > > > > + for (auto r : registerList)\n> > > > > + offsets_[r] = {};\n> > > > > +}\n> > > > > +\n> > > > > +MdParser::Status MdParserSmia::Parse(libcamera::Span buffer,\n> > > > > + RegisterMap ®isters)\n> > > > > +{\n> > > > > + if (reset_) {\n> > > > > + /*\n> > > > > + * Search again through the metadata for all the registers\n> > > > > + * requested.\n> > > > > + */\n> > > > > + ASSERT(bits_per_pixel_);\n> > > > > +\n> > > > > + for (const auto &[reg, offset] : offsets_)\n> > > > > + offsets_[reg] = {};\n> >\n> > I'm afraid this causes compilation failures with gcc 7 and gcc 8, due to\n> > offset being unused. We can't throw a [[maybe_unused]] here, that's not\n> > supported, so we need to revert to the syntax from v2.\n> >\n> > for (const auto &kv : offsets_)\n> > offsets_[kv.first] = {};\n> >\n> > I'll post a v3.1, Naush, could you give it a try ?\n> \n> Thank you for the fixups! I'm a bit curious why my compiler did not throw out warnings\n> for these though...?\n\nIt got fixed in gcc 9, maybe that's why ?\n\n> I'll test and report back first thing tomorrow.\n\nThank you.\n\n> > > > > +\n> > > > > + ParseStatus ret = findRegs(buffer);\n> > > > > + /*\n> > > > > + * > 0 means \"worked partially but parse again next time\",\n> > > > > + * < 0 means \"hard error\".\n> > > > > + *\n> > > > > + * In either case, we retry parsing on the next frame.\n> > > > > + */\n> > > > > + if (ret != PARSE_OK)\n> > > > > + return ERROR;\n> > > > > +\n> > > > > + reset_ = false;\n> > > > > + }\n> > > > > +\n> > > > > + /* Populate the register values requested. */\n> > > > > + registers.clear();\n> > > > > + for (const auto &[reg, offset] : offsets_) {\n> > > > > + if (!offset) {\n> > > > > + reset_ = true;\n> > > > > + return NOTFOUND;\n> > > > > + }\n> > > > > + registers[reg] = buffer[offset.value()];\n> > > > > + }\n> > > > > +\n> > > > > + return OK;\n> > > > > +}\n> > > > > +\n> > > > > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer)\n> > > > > +{\n> > > > > + ASSERT(offsets_.size());\n> > > > >\n> > > > > if (buffer[0] != LINE_START)\n> > > > > return NO_LINE_START;\n> > > > >\n> > > > > unsigned int current_offset = 1; /* after the LINE_START */\n> > > > > unsigned int current_line_start = 0, current_line = 0;\n> > > > > - unsigned int reg_num = 0, first_reg = 0;\n> > > > > + unsigned int reg_num = 0, regs_done = 0;\n> > > > >\n> > > > > while (1) {\n> > > > > int tag = buffer[current_offset++];\n> > > > > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> > > > > else if (tag == REG_SKIP)\n> > > > > reg_num++;\n> > > > > else if (tag == REG_VALUE) {\n> > > > > - while (reg_num >=\n> > > > > - /* assumes registers are in order... */\n> > > > > - regs[first_reg]) {\n> > > > > - if (reg_num == regs[first_reg])\n> > > > > - offsets[first_reg] = current_offset - 1;\n> > > > > + auto reg = offsets_.find(reg_num);\n> > > > > +\n> > > > > + if (reg != offsets_.end()) {\n> > > > > + offsets_[reg_num] = current_offset - 1;\n> > > > >\n> > > > > - if (++first_reg == num_regs)\n> > > > > + if (++regs_done == offsets_.size())\n> > > > > return PARSE_OK;\n> > > > > }\n> > > > > reg_num++;","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 1E462C3220\n\tfor ;\n\tTue, 29 Jun 2021 19:30:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72C4F684EC;\n\tTue, 29 Jun 2021 21:30:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94283684CB\n\tfor ;\n\tTue, 29 Jun 2021 21:30:23 +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 005904AD;\n\tTue, 29 Jun 2021 21:30:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vRUc8jWG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624995023;\n\tbh=S97zF3+JKAEHrCLJVGV2ncn5EaAhlpPGGz4gy8R1+9w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vRUc8jWGgnab7tCmnKhCCG5YnT7ta6HqhW6Gl/TQ/91AwYfDFHIpk7Oh7nEcLLyR+\n\tTvPHdj6y44IAxXGipb8Lqui72OE532rWBEsiEsSS5+uUDHd+OhPKqunWRaL1ZZYOIw\n\tKQZjcPUUJ3Ea5YuDUBItVwXhGVr+rH6J108Qwl5c=","Date":"Tue, 29 Jun 2021 22:29:45 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210629104500.51672-1-naush@raspberrypi.com>\n\t<20210629104500.51672-3-naush@raspberrypi.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","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\" "}}]