[{"id":17547,"web_url":"https://patchwork.libcamera.org/comment/17547/","msgid":"","date":"2021-06-15T03:09:38","subject":"Re: [libcamera-devel] [PATCH 6/6] ipa: raspberrypi: Generalise the\n\tSMIA 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\nThank you for the patch.\n\nOn Mon, Jun 14, 2021 at 10:53:40AM +0100, Naushir Patuck wrote:\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> As a drive-by change, fixup a possible buffer overrun in the parsing code.\n\nCould that be split to a separate patch ?\n\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 117 ++++----------------\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 125 ++++------------------\n> src/ipa/raspberrypi/md_parser.hpp | 42 ++++++--\n> src/ipa/raspberrypi/md_parser_smia.cpp | 89 ++++++++++++---\n> 4 files changed, 153 insertions(+), 220 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index 36dbe8cd941a..72c1042ad6be 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -23,21 +23,13 @@\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> -\tMdParserImx219();\n> -\tStatus Parse(libcamera::Span buffer) override;\n> -\tStatus GetExposureLines(unsigned int &lines) override;\n> -\tStatus GetGainCode(unsigned int &gain_code) override;\n> -private:\n> -\t/* Offset of the register's value in the metadata block. */\n> -\tint reg_offsets_[3];\n> -\t/* Value of the register, once read from the metadata block. */\n> -\tint 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\nLowercase for hex constants please.\n\n> +constexpr uint32_t expLoReg = 0x15B;\n> \n> class CamHelperImx219 : public CamHelper\n> {\n> @@ -55,11 +47,23 @@ private:\n> \t */\n> \tstatic constexpr int frameIntegrationDiff = 4;\n> \n> -\tMdParserImx219 imx219_parser;\n> +\tMdParserSmia imx219_parser;\n> +\n> +\tstatic uint32_t ParseExposureLines(const MdParserSmia::RegMap &map)\n> +\t{\n> +\t\treturn map.at(expHiReg).value * 256 + map.at(expLoReg).value;\n> +\t}\n> +\n> +\tstatic uint32_t ParseGainCode(const MdParserSmia::RegMap &map)\n> +\t{\n> +\t\treturn map.at(gainReg).value;\n> +\t}\n> };\n> \n> CamHelperImx219::CamHelperImx219()\n> -\t: CamHelper(frameIntegrationDiff)\n> +\t: CamHelper(frameIntegrationDiff),\n> +\t imx219_parser({ expHiReg, expLoReg, gainReg },\n> +\t\t\tParseGainCode, ParseExposureLines)\n\nI'm not thrilled by this to be honest, as we'll have to add more\nfunctions when we'll want to parse more than just the exposure time and\ngain. Could we use a mechanism that would scale better ? Decoupling the\ndata parser from the data interpreter would be one way to achieve this.\nYou could pass the list of registers you're interested in to the\nMdParserSmia constructor. It would the parse the embedded data in\nfindRegs() (which should be renamed) and return a map of register\naddress to register value. The CamHelper would then be responsible for\nextracting the register values and converting them.\n\n> {\n> #if ENABLE_EMBEDDED_DATA\n> \tparser_ = &imx219_parser;\n> @@ -97,82 +101,3 @@ static CamHelper *Create()\n> }\n> \n> static RegisterCamHelper reg(\"imx219\", &Create);\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> -\treg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> -}\n> -\n> -MdParser::Status MdParserImx219::Parse(libcamera::Span buffer)\n> -{\n> -\tbool try_again = false;\n> -\n> -\tif (reset_) {\n> -\t\t/*\n> -\t\t * Search again through the metadata for the gain and exposure\n> -\t\t * registers.\n> -\t\t */\n> -\t\tassert(bits_per_pixel_);\n> -\t\t/* Need to be ordered */\n> -\t\tuint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };\n> -\t\treg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> -\t\tint ret = static_cast(findRegs(buffer,\n> -\t\t\t\t\t\t regs, reg_offsets_, 3));\n> -\t\t/*\n> -\t\t * > 0 means \"worked partially but parse again next time\",\n> -\t\t * < 0 means \"hard error\".\n> -\t\t */\n> -\t\tif (ret > 0)\n> -\t\t\ttry_again = true;\n> -\t\telse if (ret < 0)\n> -\t\t\treturn ERROR;\n> -\t}\n> -\n> -\tfor (int i = 0; i < 3; i++) {\n> -\t\tif (reg_offsets_[i] == -1)\n> -\t\t\tcontinue;\n> -\n> -\t\treg_values_[i] = buffer[reg_offsets_[i]];\n> -\t}\n> -\n> -\t/* Re-parse next time if we were unhappy in some way. */\n> -\treset_ = try_again;\n> -\n> -\treturn OK;\n> -}\n> -\n> -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)\n> -{\n> -\tif (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> -\t\treturn NOTFOUND;\n> -\n> -\tlines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> -\n> -\treturn OK;\n> -}\n> -\n> -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)\n> -{\n> -\tif (reg_offsets_[GAIN_INDEX] == -1)\n> -\t\treturn NOTFOUND;\n> -\n> -\tgain_code = reg_values_[GAIN_INDEX];\n> -\n> -\treturn OK;\n> -}\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 038a8583d311..7a1100c25afc 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -15,21 +15,14 @@\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> -\tMdParserImx477();\n> -\tStatus Parse(libcamera::Span buffer) override;\n> -\tStatus GetExposureLines(unsigned int &lines) override;\n> -\tStatus GetGainCode(unsigned int &gain_code) override;\n> -private:\n> -\t/* Offset of the register's value in the metadata block. */\n> -\tint reg_offsets_[4];\n> -\t/* Value of the register, once read from the metadata block. */\n> -\tint 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> \n> class CamHelperImx477 : public CamHelper\n> {\n> @@ -48,11 +41,23 @@ private:\n> \t */\n> \tstatic constexpr int frameIntegrationDiff = 22;\n> \n> -\tMdParserImx477 imx477_parser;\n> +\tMdParserSmia imx477_parser;\n> +\n> +\tstatic uint32_t ParseExposureLines(const MdParserSmia::RegMap &map)\n> +\t{\n> +\t\treturn map.at(expHiReg).value * 256 + map.at(expLoReg).value;\n> +\t}\n> +\n> +\tstatic uint32_t ParseGainCode(const MdParserSmia::RegMap &map)\n> +\t{\n> +\t\treturn map.at(gainHiReg).value * 256 + map.at(gainLoReg).value;\n> +\t}\n> };\n> \n> CamHelperImx477::CamHelperImx477()\n> -\t: CamHelper(frameIntegrationDiff)\n> +\t: CamHelper(frameIntegrationDiff),\n> +\t imx477_parser({ expHiReg, expLoReg, gainHiReg, gainLoReg },\n> +\t\t\tParseGainCode, ParseExposureLines)\n> {\n> \tparser_ = &imx477_parser;\n> }\n> @@ -86,89 +91,3 @@ static CamHelper *Create()\n> }\n> \n> static RegisterCamHelper reg(\"imx477\", &Create);\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> -\treg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> -}\n> -\n> -MdParser::Status MdParserImx477::Parse(libcamera::Span buffer)\n> -{\n> -\tbool try_again = false;\n> -\n> -\tif (reset_) {\n> -\t\t/*\n> -\t\t * Search again through the metadata for the gain and exposure\n> -\t\t * registers.\n> -\t\t */\n> -\t\tassert(bits_per_pixel_);\n> -\t\t/* Need to be ordered */\n> -\t\tuint32_t regs[4] = {\n> -\t\t\tEXPHI_REG,\n> -\t\t\tEXPLO_REG,\n> -\t\t\tGAINHI_REG,\n> -\t\t\tGAINLO_REG\n> -\t\t};\n> -\t\treg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> -\t\tint ret = static_cast(findRegs(buffer,\n> -\t\t\t\t\t\t regs, reg_offsets_, 4));\n> -\t\t/*\n> -\t\t * > 0 means \"worked partially but parse again next time\",\n> -\t\t * < 0 means \"hard error\".\n> -\t\t */\n> -\t\tif (ret > 0)\n> -\t\t\ttry_again = true;\n> -\t\telse if (ret < 0)\n> -\t\t\treturn ERROR;\n> -\t}\n> -\n> -\tfor (int i = 0; i < 4; i++) {\n> -\t\tif (reg_offsets_[i] == -1)\n> -\t\t\tcontinue;\n> -\n> -\t\treg_values_[i] = buffer[reg_offsets_[i]];\n> -\t}\n> -\n> -\t/* Re-parse next time if we were unhappy in some way. */\n> -\treset_ = try_again;\n> -\n> -\treturn OK;\n> -}\n> -\n> -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)\n> -{\n> -\tif (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> -\t\treturn NOTFOUND;\n> -\n> -\tlines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> -\n> -\treturn OK;\n> -}\n> -\n> -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)\n> -{\n> -\tif (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX] == -1)\n> -\t\treturn NOTFOUND;\n> -\n> -\tgain_code = reg_values_[GAINHI_INDEX] * 256 + reg_values_[GAINLO_INDEX];\n> -\n> -\treturn OK;\n> -}\n> diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp\n> index 25ba0e7c9400..6bbcdec0830b 100644\n> --- a/src/ipa/raspberrypi/md_parser.hpp\n> +++ b/src/ipa/raspberrypi/md_parser.hpp\n> @@ -6,6 +6,10 @@\n> */\n> #pragma once\n> \n> +#include \n> +#include \n> +#include \n> +\n> #include \n> \n> /* Camera metadata parser class. Usage as shown below.\n> @@ -16,7 +20,8 @@\n> * application code doesn't have to worry which to 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> +\t\t\t\t ParseGainCode, ParseExposureLines));\n> * parser->SetBitsPerPixel(bpp);\n> * parser->SetLineLengthBytes(pitch);\n> * parser->SetNumLines(2);\n> @@ -113,14 +118,32 @@ 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> -\tMdParserSmia() : MdParser()\n> -\t{\n> -\t}\n> +\tstruct Register {\n> +\t\tRegister()\n> +\t\t\t: offset(0), value(0), found(false)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tuint32_t offset;\n> +\t\tuint32_t value;\n> +\t\tbool found;\n> +\t};\n> \n> -protected:\n> +\t/* Maps register address to offset in the buffer. */\n> +\tusing RegMap = std::map;\n> +\tusing GetFn = std::function;\n> +\n> +\tMdParserSmia(const std::vector ®s, GetFn gain_fn,\n> +\t\t GetFn exposureFn);\n> +\n> +\tMdParser::Status Parse(libcamera::Span buffer) override;\n> +\tStatus GetExposureLines(unsigned int &lines) override;\n> +\tStatus GetGainCode(unsigned int &gain_code) override;\n> +\n> +private:\n> \t/*\n> \t * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> \t * indicate a bad data buffer. Status codes are:\n> @@ -138,8 +161,11 @@ protected:\n> \t\tBAD_PADDING = -5\n> \t};\n> \n> -\tParseStatus findRegs(libcamera::Span buffer, uint32_t regs[],\n> -\t\t\t int offsets[], unsigned int num_regs);\n> +\tParseStatus findRegs(libcamera::Span buffer);\n> +\n> +\tRegMap map_;\n> +\tGetFn gain_fn_;\n> +\tGetFn exposure_fn_;\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 65ffbe00c76e..f4748dd535d0 100644\n> --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> @@ -8,9 +8,11 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/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> @@ -28,18 +30,79 @@ 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> -\t\t\t\t\t\t uint32_t regs[], int offsets[],\n> -\t\t\t\t\t\t unsigned int num_regs)\n> +MdParserSmia::MdParserSmia(const std::vector ®isters,\n> +\t\t\t GetFn gain_fn, GetFn exposure_fn)\n> +\t: gain_fn_(gain_fn), exposure_fn_(exposure_fn)\n> {\n> -\tassert(num_regs > 0);\n> +\tfor (auto r : registers)\n> +\t\tmap_[r] = {};\n> +}\n> +\n> +MdParser::Status MdParserSmia::Parse(libcamera::Span buffer)\n> +{\n> +\tif (reset_) {\n> +\t\t/*\n> +\t\t * Search again through the metadata for the gain and exposure\n> +\t\t * registers.\n> +\t\t */\n> +\t\tASSERT(bits_per_pixel_);\n> +\n> +\t\tParseStatus ret = findRegs(buffer);\n> +\t\t/*\n> +\t\t * > 0 means \"worked partially but parse again next time\",\n> +\t\t * < 0 means \"hard error\".\n> +\t\t *\n> +\t\t * In either case, we retry parsing on the next frame.\n> +\t\t */\n> +\t\tif (ret != PARSE_OK)\n> +\t\t\treturn ERROR;\n> +\n> +\t\treset_ = false;\n> +\t}\n> +\n> +\t/* Populate the register values requested. */\n> +\tfor (auto &kv : map_) {\n> +\t\tRegister ® = kv.second;\n> +\n> +\t\tif (!reg.found) {\n> +\t\t\treset_ = true;\n> +\t\t\treturn NOTFOUND;\n> +\t\t}\n> +\n> +\t\treg.value = buffer[reg.offset];\n> +\t}\n> +\n> +\treturn OK;\n> +}\n> +\n> +MdParser::Status MdParserSmia::GetExposureLines(unsigned int &lines)\n> +{\n> +\tif (reset_)\n> +\t\treturn NOTFOUND;\n> +\n> +\tlines = exposure_fn_(map_);\n> +\treturn OK;\n> +}\n> +\n> +MdParser::Status MdParserSmia::GetGainCode(unsigned int &gain_code)\n> +{\n> +\tif (reset_)\n> +\t\treturn NOTFOUND;\n> +\n> +\tgain_code = gain_fn_(map_);\n> +\treturn OK;\n> +}\n> +\n> +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer)\n> +{\n> +\tASSERT(map_.size());\n> \n> \tif (buffer[0] != LINE_START)\n> \t\treturn NO_LINE_START;\n> \n> \tunsigned int current_offset = 1; /* after the LINE_START */\n> \tunsigned int current_line_start = 0, current_line = 0;\n> -\tunsigned int reg_num = 0, first_reg = 0;\n> +\tunsigned int reg_num = 0, regs_done = 0;\n> \n> \twhile (1) {\n> \t\tint tag = buffer[current_offset++];\n> @@ -73,8 +136,8 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> \t\t\t\t\treturn NO_LINE_START;\n> \t\t\t} else {\n> \t\t\t\t/* allow a zero line length to mean \"hunt for the next line\" */\n> -\t\t\t\twhile (buffer[current_offset] != LINE_START &&\n> -\t\t\t\t current_offset < buffer.size())\n> +\t\t\t\twhile (current_offset < buffer.size() &&\n> +\t\t\t\t buffer[current_offset] != LINE_START)\n> \t\t\t\t\tcurrent_offset++;\n> \n> \t\t\t\tif (current_offset == buffer.size())\n> @@ -91,13 +154,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> \t\t\telse if (tag == REG_SKIP)\n> \t\t\t\treg_num++;\n> \t\t\telse if (tag == REG_VALUE) {\n> -\t\t\t\twhile (reg_num >=\n> -\t\t\t\t /* assumes registers are in order... */\n> -\t\t\t\t regs[first_reg]) {\n> -\t\t\t\t\tif (reg_num == regs[first_reg])\n> -\t\t\t\t\t\toffsets[first_reg] = current_offset - 1;\n> +\t\t\t\tauto reg = map_.find(reg_num);\n> +\n> +\t\t\t\tif (reg != map_.end()) {\n> +\t\t\t\t\tmap_[reg_num].offset = current_offset - 1;\n> +\t\t\t\t\tmap_[reg_num].found = true;\n> \n> -\t\t\t\t\tif (++first_reg == num_regs)\n> +\t\t\t\t\tif (++regs_done == map_.size())\n> \t\t\t\t\t\treturn PARSE_OK;\n> \t\t\t\t}\n> \t\t\t\treg_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 EF974BD78E\n\tfor ;\n\tTue, 15 Jun 2021 03:10:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 669CA68944;\n\tTue, 15 Jun 2021 05:10:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4FBF6050D\n\tfor ;\n\tTue, 15 Jun 2021 05:09:58 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 574D73E5;\n\tTue, 15 Jun 2021 05:09:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lvacb6nD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623726598;\n\tbh=qBhyDo3L55cUU1EBENR8BZibBRdogVAdWoIV/qywpeY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lvacb6nDKkTgwBOjHhaqElTyCDMF5XwfQ1ikXcxMlS9lUXcRm0BLE2QV1sKUxndMm\n\tGpNIddXinTel9lLyPSgGiSwWVMvM+3cjm9OnaiDzuShZ2yuoSQsWwqFDlWaC3urfuU\n\tb1OS/BDucIMNEYrqWoZcKFTP7znluAKfEkzpwYrQ=","Date":"Tue, 15 Jun 2021 06:09:38 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210614095340.3051816-1-naush@raspberrypi.com>\n\t<20210614095340.3051816-7-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210614095340.3051816-7-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 6/6] ipa: raspberrypi: Generalise the\n\tSMIA 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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17561,"web_url":"https://patchwork.libcamera.org/comment/17561/","msgid":"","date":"2021-06-15T09:46:44","subject":"Re: [libcamera-devel] [PATCH 6/6] ipa: raspberrypi: Generalise the\n\tSMIA metadata parser","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Tue, 15 Jun 2021 at 04:09, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Jun 14, 2021 at 10:53:40AM +0100, Naushir Patuck wrote:\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> > As a drive-by change, fixup a possible buffer overrun in the parsing\n> code.\n>\n> Could that be split to a separate patch ?\n\n\nSure, will do.\n\n\n>\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 117 ++++----------------\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 125 ++++------------------\n> > src/ipa/raspberrypi/md_parser.hpp | 42 ++++++--\n> > src/ipa/raspberrypi/md_parser_smia.cpp | 89 ++++++++++++---\n> > 4 files changed, 153 insertions(+), 220 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > index 36dbe8cd941a..72c1042ad6be 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > @@ -23,21 +23,13 @@\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>\n> Lowercase for hex constants please.\n>\n\nAck.\n\n\n>\n> > +constexpr uint32_t expLoReg = 0x15B;\n> >\n> > class CamHelperImx219 : public CamHelper\n> > {\n> > @@ -55,11 +47,23 @@ private:\n> > */\n> > static constexpr int frameIntegrationDiff = 4;\n> >\n> > - MdParserImx219 imx219_parser;\n> > + MdParserSmia imx219_parser;\n> > +\n> > + static uint32_t ParseExposureLines(const MdParserSmia::RegMap &map)\n> > + {\n> > + return map.at(expHiReg).value * 256 + map.at\n> (expLoReg).value;\n> > + }\n> > +\n> > + static uint32_t ParseGainCode(const MdParserSmia::RegMap &map)\n> > + {\n> > + return map.at(gainReg).value;\n> > + }\n> > };\n> >\n> > CamHelperImx219::CamHelperImx219()\n> > - : CamHelper(frameIntegrationDiff)\n> > + : CamHelper(frameIntegrationDiff),\n> > + imx219_parser({ expHiReg, expLoReg, gainReg },\n> > + ParseGainCode, ParseExposureLines)\n>\n> I'm not thrilled by this to be honest, as we'll have to add more\n> functions when we'll want to parse more than just the exposure time and\n> gain. Could we use a mechanism that would scale better ? Decoupling the\n> data parser from the data interpreter would be one way to achieve this.\n> You could pass the list of registers you're interested in to the\n> MdParserSmia constructor. It would the parse the embedded data in\n> findRegs() (which should be renamed) and return a map of register\n> address to register value. The CamHelper would then be responsible for\n> extracting the register values and converting them.\n>\n\nYes, I do agree that this is somewhat restrictive. Here's a way to decouple\nthe parsing and supplying values:\n\n1) MdParser::Parse and MdParserSmia::Parse now return the register to offset\nmap, instead of keeping this internal:\n\nStatus Parse(libcamera::Span buffer, RegMap &map);\n\n2) CamHelper::parseEmbeddedData calls MdParser::Parse() as it currently\ndoes,\nand this will now call a new virtual method CamHelper::fillMetadata() that\nwill\nfill our metadata buffer with whatever embedded data bits we think useful.\nThis way, all the sensor specific dealing is hidden behind the sensor\nderived\nCamHelper class.\n\nI did not get your point about renaming findRegs(), could you elaborate on\nthat\na bit?\n\nI'll prepare this change and we can discuss further once I have some code\nwritten.\n\nRegards,\nNaush\n\n\n> > {\n> > #if ENABLE_EMBEDDED_DATA\n> > parser_ = &imx219_parser;\n> > @@ -97,82 +101,3 @@ static CamHelper *Create()\n> > }\n> >\n> > static RegisterCamHelper reg(\"imx219\", &Create);\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> > -}\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> > -{\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> > -}\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> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index 038a8583d311..7a1100c25afc 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -15,21 +15,14 @@\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> >\n> > class CamHelperImx477 : public CamHelper\n> > {\n> > @@ -48,11 +41,23 @@ private:\n> > */\n> > static constexpr int frameIntegrationDiff = 22;\n> >\n> > - MdParserImx477 imx477_parser;\n> > + MdParserSmia imx477_parser;\n> > +\n> > + static uint32_t ParseExposureLines(const MdParserSmia::RegMap &map)\n> > + {\n> > + return map.at(expHiReg).value * 256 + map.at\n> (expLoReg).value;\n> > + }\n> > +\n> > + static uint32_t ParseGainCode(const MdParserSmia::RegMap &map)\n> > + {\n> > + return map.at(gainHiReg).value * 256 + map.at\n> (gainLoReg).value;\n> > + }\n> > };\n> >\n> > CamHelperImx477::CamHelperImx477()\n> > - : CamHelper(frameIntegrationDiff)\n> > + : CamHelper(frameIntegrationDiff),\n> > + imx477_parser({ expHiReg, expLoReg, gainHiReg, gainLoReg },\n> > + ParseGainCode, ParseExposureLines)\n> > {\n> > parser_ = &imx477_parser;\n> > }\n> > @@ -86,89 +91,3 @@ static CamHelper *Create()\n> > }\n> >\n> > static RegisterCamHelper reg(\"imx477\", &Create);\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> > -}\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> > -{\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> > -}\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> > diff --git a/src/ipa/raspberrypi/md_parser.hpp\n> b/src/ipa/raspberrypi/md_parser.hpp\n> > index 25ba0e7c9400..6bbcdec0830b 100644\n> > --- a/src/ipa/raspberrypi/md_parser.hpp\n> > +++ b/src/ipa/raspberrypi/md_parser.hpp\n> > @@ -6,6 +6,10 @@\n> > */\n> > #pragma once\n> >\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > #include \n> >\n> > /* Camera metadata parser class. Usage as shown below.\n> > @@ -16,7 +20,8 @@\n> > * application code doesn't have to worry which to 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> > + ParseGainCode, ParseExposureLines));\n> > * parser->SetBitsPerPixel(bpp);\n> > * parser->SetLineLengthBytes(pitch);\n> > * parser->SetNumLines(2);\n> > @@ -113,14 +118,32 @@ 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> > + struct Register {\n> > + Register()\n> > + : offset(0), value(0), found(false)\n> > + {\n> > + }\n> > +\n> > + uint32_t offset;\n> > + uint32_t value;\n> > + bool found;\n> > + };\n> >\n> > -protected:\n> > + /* Maps register address to offset in the buffer. */\n> > + using RegMap = std::map;\n> > + using GetFn = std::function;\n> > +\n> > + MdParserSmia(const std::vector ®s, GetFn gain_fn,\n> > + GetFn exposureFn);\n> > +\n> > + MdParser::Status Parse(libcamera::Span buffer)\n> override;\n> > + Status GetExposureLines(unsigned int &lines) override;\n> > + Status GetGainCode(unsigned int &gain_code) override;\n> > +\n> > +private:\n> > /*\n> > * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> > * indicate a bad data buffer. Status codes are:\n> > @@ -138,8 +161,11 @@ 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> > + RegMap map_;\n> > + GetFn gain_fn_;\n> > + GetFn exposure_fn_;\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 65ffbe00c76e..f4748dd535d0 100644\n> > --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > @@ -8,9 +8,11 @@\n> > #include \n> > #include \n> >\n> > +#include \"libcamera/internal/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> > @@ -28,18 +30,79 @@ 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(const std::vector ®isters,\n> > + GetFn gain_fn, GetFn exposure_fn)\n> > + : gain_fn_(gain_fn), exposure_fn_(exposure_fn)\n> > {\n> > - assert(num_regs > 0);\n> > + for (auto r : registers)\n> > + map_[r] = {};\n> > +}\n> > +\n> > +MdParser::Status MdParserSmia::Parse(libcamera::Span\n> buffer)\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> > +\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> > + for (auto &kv : map_) {\n> > + Register ® = kv.second;\n> > +\n> > + if (!reg.found) {\n> > + reset_ = true;\n> > + return NOTFOUND;\n> > + }\n> > +\n> > + reg.value = buffer[reg.offset];\n> > + }\n> > +\n> > + return OK;\n> > +}\n> > +\n> > +MdParser::Status MdParserSmia::GetExposureLines(unsigned int &lines)\n> > +{\n> > + if (reset_)\n> > + return NOTFOUND;\n> > +\n> > + lines = exposure_fn_(map_);\n> > + return OK;\n> > +}\n> > +\n> > +MdParser::Status MdParserSmia::GetGainCode(unsigned int &gain_code)\n> > +{\n> > + if (reset_)\n> > + return NOTFOUND;\n> > +\n> > + gain_code = gain_fn_(map_);\n> > + return OK;\n> > +}\n> > +\n> > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span uint8_t> buffer)\n> > +{\n> > + ASSERT(map_.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> > @@ -73,8 +136,8 @@ MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span\n> > return NO_LINE_START;\n> > } else {\n> > /* allow a zero line length to mean \"hunt\n> for the next line\" */\n> > - while (buffer[current_offset] !=\n> LINE_START &&\n> > - current_offset < buffer.size())\n> > + while (current_offset < buffer.size() &&\n> > + buffer[current_offset] !=\n> LINE_START)\n> > current_offset++;\n> >\n> > if (current_offset == buffer.size())\n> > @@ -91,13 +154,13 @@ 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 = map_.find(reg_num);\n> > +\n> > + if (reg != map_.end()) {\n> > + map_[reg_num].offset =\n> current_offset - 1;\n> > + map_[reg_num].found = true;\n> >\n> > - if (++first_reg == num_regs)\n> > + if (++regs_done == map_.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 D2019BD78E\n\tfor ;\n\tTue, 15 Jun 2021 09:47:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1818468946;\n\tTue, 15 Jun 2021 11:47:03 +0200 (CEST)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4130360297\n\tfor ;\n\tTue, 15 Jun 2021 11:47:01 +0200 (CEST)","by mail-lf1-x12c.google.com with SMTP id p17so25943567lfc.6\n\tfor ;\n\tTue, 15 Jun 2021 02:47:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"hM0SAjTm\"; 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=BXiVPaNAAehknUDjoOCpSl4PtI9IhHhqozldUGSffuM=;\n\tb=hM0SAjTmhBFHwNOt2SrqpZjbojaHKAumubyiy88EdYhZpaYdT51rjZR6qDywCJpgHW\n\tTmcITu5BKmo72l2B7Shhzo0R1nx57fiMj4bB1BlEbpuO+uHyxMm3Av17C2ROfaSppcTG\n\tYXBbiRP70k74YLtzeAlO1XOhZerRTKMOZ7SklIlIUMldol7vg7ROtj6b6XrwQPF265ic\n\torAZ8ZvLJo6+XGAqi8kjNXZmywWzhltr2vwxmXU5UbqUaZD6Yr7AXLvkIiWh7yxI73XO\n\tBhtsMznapO3C7q3gOfgJGU3ZFoQQBSXMvKoe8O9xLLJ6LyZdgE9s/kmVeYuV9+EYjIB/\n\tx0mw==","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=BXiVPaNAAehknUDjoOCpSl4PtI9IhHhqozldUGSffuM=;\n\tb=jdCkpfeI+4PlKyxd7nKNOADv7efm35awWLrfDBsL7/f81BsdadJHBhHnXmQOHFfzJb\n\togIyXojelk6xG/GcThqbgMS8ye84N5SM8HktW1ZqalHtDcKUaAx9n112TLqRK0Jz0A0R\n\t7YUZuOwAwtZ/EQYescZAEjBDt4ummvIggYgvyXXsZrj1QXaFMLdHO4mVeihnHQBpag0V\n\tHYLQQvvuAiHM1in2Grj6H8eJ6OkxnUuqhyNEg9wxAacZxlRLMbFOgWiE1QA5zgqUNY3F\n\tQAMfgE8XIoBAS+P6roAo+1ugWmCgbz6WDHV7A16Wo2wPjq3nGL5kjIJVk+h0y15GbnS0\n\tzihg==","X-Gm-Message-State":"AOAM5303E5OZddJAcjWhMDnYlhqFamo8J/toT9lCYbIyCOLpsEm1nS28\n\toE2q3PnXrE9/r6nL0sXJs6ZpWYy+1qLx/G5FLoFobY973h03vQ==","X-Google-Smtp-Source":"ABdhPJzdJoD6VT+3DnJuyAc6AlmDU8zzm8rWOy/p2VTYAh1jqKTjcyX81Lw4fmcmtrt8ZPVqYSTd7r7ICMcYQJX+0PU=","X-Received":"by 2002:a05:6512:2096:: with SMTP id\n\tt22mr15321977lfr.272.1623750420570; \n\tTue, 15 Jun 2021 02:47:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20210614095340.3051816-1-naush@raspberrypi.com>\n\t<20210614095340.3051816-7-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 15 Jun 2021 10:46:44 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000005b37605c4cadb0d\"","Subject":"Re: [libcamera-devel] [PATCH 6/6] ipa: raspberrypi: Generalise the\n\tSMIA 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":17562,"web_url":"https://patchwork.libcamera.org/comment/17562/","msgid":"","date":"2021-06-15T09:53:45","subject":"Re: [libcamera-devel] [PATCH 6/6] ipa: raspberrypi: Generalise the\n\tSMIA 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 15, 2021 at 10:46:44AM +0100, Naushir Patuck wrote:\n> On Tue, 15 Jun 2021 at 04:09, Laurent Pinchart wrote:\n> > On Mon, Jun 14, 2021 at 10:53:40AM +0100, Naushir Patuck wrote:\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> > > As a drive-by change, fixup a possible buffer overrun in the parsing code.\n> >\n> > Could that be split to a separate patch ?\n> \n> Sure, will do.\n> \n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 117 ++++----------------\n> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 125 ++++------------------\n> > > src/ipa/raspberrypi/md_parser.hpp | 42 ++++++--\n> > > src/ipa/raspberrypi/md_parser_smia.cpp | 89 ++++++++++++---\n> > > 4 files changed, 153 insertions(+), 220 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > index 36dbe8cd941a..72c1042ad6be 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > @@ -23,21 +23,13 @@\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> >\n> > Lowercase for hex constants please.\n> \n> Ack.\n> \n> > > +constexpr uint32_t expLoReg = 0x15B;\n> > >\n> > > class CamHelperImx219 : public CamHelper\n> > > {\n> > > @@ -55,11 +47,23 @@ private:\n> > > */\n> > > static constexpr int frameIntegrationDiff = 4;\n> > >\n> > > - MdParserImx219 imx219_parser;\n> > > + MdParserSmia imx219_parser;\n> > > +\n> > > + static uint32_t ParseExposureLines(const MdParserSmia::RegMap &map)\n> > > + {\n> > > + return map.at(expHiReg).value * 256 + map.at(expLoReg).value;\n> > > + }\n> > > +\n> > > + static uint32_t ParseGainCode(const MdParserSmia::RegMap &map)\n> > > + {\n> > > + return map.at(gainReg).value;\n> > > + }\n> > > };\n> > >\n> > > CamHelperImx219::CamHelperImx219()\n> > > - : CamHelper(frameIntegrationDiff)\n> > > + : CamHelper(frameIntegrationDiff),\n> > > + imx219_parser({ expHiReg, expLoReg, gainReg },\n> > > + ParseGainCode, ParseExposureLines)\n> >\n> > I'm not thrilled by this to be honest, as we'll have to add more\n> > functions when we'll want to parse more than just the exposure time and\n> > gain. Could we use a mechanism that would scale better ? Decoupling the\n> > data parser from the data interpreter would be one way to achieve this.\n> > You could pass the list of registers you're interested in to the\n> > MdParserSmia constructor. It would the parse the embedded data in\n> > findRegs() (which should be renamed) and return a map of register\n> > address to register value. The CamHelper would then be responsible for\n> > extracting the register values and converting them.\n> \n> Yes, I do agree that this is somewhat restrictive. Here's a way to decouple\n> the parsing and supplying values:\n> \n> 1) MdParser::Parse and MdParserSmia::Parse now return the register to offset\n> map, instead of keeping this internal:\n> \n> Status Parse(libcamera::Span buffer, RegMap &map);\n> \n> 2) CamHelper::parseEmbeddedData calls MdParser::Parse() as it currently does,\n> and this will now call a new virtual method CamHelper::fillMetadata() that will\n> fill our metadata buffer with whatever embedded data bits we think useful.\n> This way, all the sensor specific dealing is hidden behind the sensor derived\n> CamHelper class.\n> \n> I did not get your point about renaming findRegs(), could you elaborate on that\n> a bit?\n\nGiven that storing an offset won't take less space than storing a\nregister value, I was thinking of replacing the register address to\noffset map by a register address to value map. findRegs() wouldn't be a\ngreat name anymore.\n\n> I'll prepare this change and we can discuss further once I have some code\n> written.\n> \n> > > {\n> > > #if ENABLE_EMBEDDED_DATA\n> > > parser_ = &imx219_parser;\n> > > @@ -97,82 +101,3 @@ static CamHelper *Create()\n> > > }\n> > >\n> > > static RegisterCamHelper reg(\"imx219\", &Create);\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> > > -}\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> > > -{\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> > > -}\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> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > index 038a8583d311..7a1100c25afc 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > @@ -15,21 +15,14 @@\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> > >\n> > > class CamHelperImx477 : public CamHelper\n> > > {\n> > > @@ -48,11 +41,23 @@ private:\n> > > */\n> > > static constexpr int frameIntegrationDiff = 22;\n> > >\n> > > - MdParserImx477 imx477_parser;\n> > > + MdParserSmia imx477_parser;\n> > > +\n> > > + static uint32_t ParseExposureLines(const MdParserSmia::RegMap &map)\n> > > + {\n> > > + return map.at(expHiReg).value * 256 + map.at (expLoReg).value;\n> > > + }\n> > > +\n> > > + static uint32_t ParseGainCode(const MdParserSmia::RegMap &map)\n> > > + {\n> > > + return map.at(gainHiReg).value * 256 + map.at (gainLoReg).value;\n> > > + }\n> > > };\n> > >\n> > > CamHelperImx477::CamHelperImx477()\n> > > - : CamHelper(frameIntegrationDiff)\n> > > + : CamHelper(frameIntegrationDiff),\n> > > + imx477_parser({ expHiReg, expLoReg, gainHiReg, gainLoReg },\n> > > + ParseGainCode, ParseExposureLines)\n> > > {\n> > > parser_ = &imx477_parser;\n> > > }\n> > > @@ -86,89 +91,3 @@ static CamHelper *Create()\n> > > }\n> > >\n> > > static RegisterCamHelper reg(\"imx477\", &Create);\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> > > -}\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> > > -{\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> > > -}\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 + reg_values_[GAINLO_INDEX];\n> > > -\n> > > - return OK;\n> > > -}\n> > > diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp\n> > > index 25ba0e7c9400..6bbcdec0830b 100644\n> > > --- a/src/ipa/raspberrypi/md_parser.hpp\n> > > +++ b/src/ipa/raspberrypi/md_parser.hpp\n> > > @@ -6,6 +6,10 @@\n> > > */\n> > > #pragma once\n> > >\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > #include \n> > >\n> > > /* Camera metadata parser class. Usage as shown below.\n> > > @@ -16,7 +20,8 @@\n> > > * application code doesn't have to worry which to 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> > > + ParseGainCode, ParseExposureLines));\n> > > * parser->SetBitsPerPixel(bpp);\n> > > * parser->SetLineLengthBytes(pitch);\n> > > * parser->SetNumLines(2);\n> > > @@ -113,14 +118,32 @@ 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> > > + struct Register {\n> > > + Register()\n> > > + : offset(0), value(0), found(false)\n> > > + {\n> > > + }\n> > > +\n> > > + uint32_t offset;\n> > > + uint32_t value;\n> > > + bool found;\n> > > + };\n> > >\n> > > -protected:\n> > > + /* Maps register address to offset in the buffer. */\n> > > + using RegMap = std::map;\n> > > + using GetFn = std::function;\n> > > +\n> > > + MdParserSmia(const std::vector ®s, GetFn gain_fn,\n> > > + GetFn exposureFn);\n> > > +\n> > > + MdParser::Status Parse(libcamera::Span buffer) override;\n> > > + Status GetExposureLines(unsigned int &lines) override;\n> > > + Status GetGainCode(unsigned int &gain_code) override;\n> > > +\n> > > +private:\n> > > /*\n> > > * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> > > * indicate a bad data buffer. Status codes are:\n> > > @@ -138,8 +161,11 @@ 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> > > + RegMap map_;\n> > > + GetFn gain_fn_;\n> > > + GetFn exposure_fn_;\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 65ffbe00c76e..f4748dd535d0 100644\n> > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > @@ -8,9 +8,11 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \"libcamera/internal/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> > > @@ -28,18 +30,79 @@ 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(const std::vector ®isters,\n> > > + GetFn gain_fn, GetFn exposure_fn)\n> > > + : gain_fn_(gain_fn), exposure_fn_(exposure_fn)\n> > > {\n> > > - assert(num_regs > 0);\n> > > + for (auto r : registers)\n> > > + map_[r] = {};\n> > > +}\n> > > +\n> > > +MdParser::Status MdParserSmia::Parse(libcamera::Span buffer)\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> > > +\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> > > + for (auto &kv : map_) {\n> > > + Register ® = kv.second;\n> > > +\n> > > + if (!reg.found) {\n> > > + reset_ = true;\n> > > + return NOTFOUND;\n> > > + }\n> > > +\n> > > + reg.value = buffer[reg.offset];\n> > > + }\n> > > +\n> > > + return OK;\n> > > +}\n> > > +\n> > > +MdParser::Status MdParserSmia::GetExposureLines(unsigned int &lines)\n> > > +{\n> > > + if (reset_)\n> > > + return NOTFOUND;\n> > > +\n> > > + lines = exposure_fn_(map_);\n> > > + return OK;\n> > > +}\n> > > +\n> > > +MdParser::Status MdParserSmia::GetGainCode(unsigned int &gain_code)\n> > > +{\n> > > + if (reset_)\n> > > + return NOTFOUND;\n> > > +\n> > > + gain_code = gain_fn_(map_);\n> > > + return OK;\n> > > +}\n> > > +\n> > > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer)\n> > > +{\n> > > + ASSERT(map_.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> > > @@ -73,8 +136,8 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> > > return NO_LINE_START;\n> > > } else {\n> > > /* allow a zero line length to mean \"hunt for the next line\" */\n> > > - while (buffer[current_offset] != LINE_START &&\n> > > - current_offset < buffer.size())\n> > > + while (current_offset < buffer.size() &&\n> > > + buffer[current_offset] != LINE_START)\n> > > current_offset++;\n> > >\n> > > if (current_offset == buffer.size())\n> > > @@ -91,13 +154,13 @@ 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 = map_.find(reg_num);\n> > > +\n> > > + if (reg != map_.end()) {\n> > > + map_[reg_num].offset = current_offset - 1;\n> > > + map_[reg_num].found = true;\n> > >\n> > > - if (++first_reg == num_regs)\n> > > + if (++regs_done == map_.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 1892FC3218\n\tfor ;\n\tTue, 15 Jun 2021 09:54:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AEAE68943;\n\tTue, 15 Jun 2021 11:54:07 +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 0E76860297\n\tfor ;\n\tTue, 15 Jun 2021 11:54:06 +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 6FD50436;\n\tTue, 15 Jun 2021 11:54:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"W8Cy5Md/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623750845;\n\tbh=nEiiTOmxGz7JW7EwEKz+7Fbaku+CzdU2gsX9Zxhncuo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=W8Cy5Md/qXQPXZ5jyOoGLmd5//JcV8986rbjK0UHwuj03ut6ZI+MYkAMbgZKPM7ZB\n\tzKJy8azoFuBufqhvz5Fp+24lttOm5ugYTQvv66HL+7LGPQ9MhjrOQCxjJVjhbSBmYy\n\tMMrfZOTOiTjM4SuOTLdgr7aswb0LMavTpWmT0OGE=","Date":"Tue, 15 Jun 2021 12:53:45 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210614095340.3051816-1-naush@raspberrypi.com>\n\t<20210614095340.3051816-7-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 6/6] ipa: raspberrypi: Generalise the\n\tSMIA 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":17563,"web_url":"https://patchwork.libcamera.org/comment/17563/","msgid":"","date":"2021-06-15T10:04:14","subject":"Re: [libcamera-devel] [PATCH 6/6] ipa: raspberrypi: Generalise the\n\tSMIA 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, 15 Jun 2021 at 10:54, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Tue, Jun 15, 2021 at 10:46:44AM +0100, Naushir Patuck wrote:\n> > On Tue, 15 Jun 2021 at 04:09, Laurent Pinchart wrote:\n> > > On Mon, Jun 14, 2021 at 10:53:40AM +0100, Naushir Patuck wrote:\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> > > > As a drive-by change, fixup a possible buffer overrun in the parsing\n> code.\n> > >\n> > > Could that be split to a separate patch ?\n> >\n> > Sure, will do.\n> >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 117 ++++----------------\n> > > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 125\n> ++++------------------\n> > > > src/ipa/raspberrypi/md_parser.hpp | 42 ++++++--\n> > > > src/ipa/raspberrypi/md_parser_smia.cpp | 89 ++++++++++++---\n> > > > 4 files changed, 153 insertions(+), 220 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > index 36dbe8cd941a..72c1042ad6be 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > > @@ -23,21 +23,13 @@\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> > > > - 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> > >\n> > > Lowercase for hex constants please.\n> >\n> > Ack.\n> >\n> > > > +constexpr uint32_t expLoReg = 0x15B;\n> > > >\n> > > > class CamHelperImx219 : public CamHelper\n> > > > {\n> > > > @@ -55,11 +47,23 @@ private:\n> > > > */\n> > > > static constexpr int frameIntegrationDiff = 4;\n> > > >\n> > > > - MdParserImx219 imx219_parser;\n> > > > + MdParserSmia imx219_parser;\n> > > > +\n> > > > + static uint32_t ParseExposureLines(const MdParserSmia::RegMap\n> &map)\n> > > > + {\n> > > > + return map.at(expHiReg).value * 256 + map.at\n> (expLoReg).value;\n> > > > + }\n> > > > +\n> > > > + static uint32_t ParseGainCode(const MdParserSmia::RegMap &map)\n> > > > + {\n> > > > + return map.at(gainReg).value;\n> > > > + }\n> > > > };\n> > > >\n> > > > CamHelperImx219::CamHelperImx219()\n> > > > - : CamHelper(frameIntegrationDiff)\n> > > > + : CamHelper(frameIntegrationDiff),\n> > > > + imx219_parser({ expHiReg, expLoReg, gainReg },\n> > > > + ParseGainCode, ParseExposureLines)\n> > >\n> > > I'm not thrilled by this to be honest, as we'll have to add more\n> > > functions when we'll want to parse more than just the exposure time and\n> > > gain. Could we use a mechanism that would scale better ? Decoupling the\n> > > data parser from the data interpreter would be one way to achieve this.\n> > > You could pass the list of registers you're interested in to the\n> > > MdParserSmia constructor. It would the parse the embedded data in\n> > > findRegs() (which should be renamed) and return a map of register\n> > > address to register value. The CamHelper would then be responsible for\n> > > extracting the register values and converting them.\n> >\n> > Yes, I do agree that this is somewhat restrictive. Here's a way to\n> decouple\n> > the parsing and supplying values:\n> >\n> > 1) MdParser::Parse and MdParserSmia::Parse now return the register to\n> offset\n> > map, instead of keeping this internal:\n> >\n> > Status Parse(libcamera::Span buffer, RegMap &map);\n> >\n> > 2) CamHelper::parseEmbeddedData calls MdParser::Parse() as it currently\n> does,\n> > and this will now call a new virtual method CamHelper::fillMetadata()\n> that will\n> > fill our metadata buffer with whatever embedded data bits we think\n> useful.\n> > This way, all the sensor specific dealing is hidden behind the sensor\n> derived\n> > CamHelper class.\n> >\n> > I did not get your point about renaming findRegs(), could you elaborate\n> on that\n> > a bit?\n>\n> Given that storing an offset won't take less space than storing a\n> register value, I was thinking of replacing the register address to\n> offset map by a register address to value map. findRegs() wouldn't be a\n> great name anymore.\n>\n\nStill not sure I get it :-)\n\nThe MdParserSmia will have to keep a register -> buffer offset map as a\nprivate\ntype. This is populated by findRegs(), and only called once (or of reset_\nis true).\n\nThe map returned from MdParser::Parse() will indeed be register -> value\nmap,\nwhich in-turn will be populated by the private register -> offset map on\nevery frame.\nSo findRegs() is still a descriptive name, or perhaps call it\nfindRegisterOffsets()?\n\nOr have I missed your intention?\n\nRegards,\nNaush\n\n\n\n>\n> > I'll prepare this change and we can discuss further once I have some code\n> > written.\n> >\n> > > > {\n> > > > #if ENABLE_EMBEDDED_DATA\n> > > > parser_ = &imx219_parser;\n> > > > @@ -97,82 +101,3 @@ static CamHelper *Create()\n> > > > }\n> > > >\n> > > > static RegisterCamHelper reg(\"imx219\", &Create);\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> > > > -}\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 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\n> &lines)\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> > > > -}\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> > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > > index 038a8583d311..7a1100c25afc 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > > @@ -15,21 +15,14 @@\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> > > > - 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> > > >\n> > > > class CamHelperImx477 : public CamHelper\n> > > > {\n> > > > @@ -48,11 +41,23 @@ private:\n> > > > */\n> > > > static constexpr int frameIntegrationDiff = 22;\n> > > >\n> > > > - MdParserImx477 imx477_parser;\n> > > > + MdParserSmia imx477_parser;\n> > > > +\n> > > > + static uint32_t ParseExposureLines(const MdParserSmia::RegMap\n> &map)\n> > > > + {\n> > > > + return map.at(expHiReg).value * 256 + map.at\n> (expLoReg).value;\n> > > > + }\n> > > > +\n> > > > + static uint32_t ParseGainCode(const MdParserSmia::RegMap &map)\n> > > > + {\n> > > > + return map.at(gainHiReg).value * 256 + map.at\n> (gainLoReg).value;\n> > > > + }\n> > > > };\n> > > >\n> > > > CamHelperImx477::CamHelperImx477()\n> > > > - : CamHelper(frameIntegrationDiff)\n> > > > + : CamHelper(frameIntegrationDiff),\n> > > > + imx477_parser({ expHiReg, expLoReg, gainHiReg, gainLoReg },\n> > > > + ParseGainCode, ParseExposureLines)\n> > > > {\n> > > > parser_ = &imx477_parser;\n> > > > }\n> > > > @@ -86,89 +91,3 @@ static CamHelper *Create()\n> > > > }\n> > > >\n> > > > static RegisterCamHelper reg(\"imx477\", &Create);\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> > > > -}\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 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\n> &lines)\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> > > > -}\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> > > > diff --git a/src/ipa/raspberrypi/md_parser.hpp\n> b/src/ipa/raspberrypi/md_parser.hpp\n> > > > index 25ba0e7c9400..6bbcdec0830b 100644\n> > > > --- a/src/ipa/raspberrypi/md_parser.hpp\n> > > > +++ b/src/ipa/raspberrypi/md_parser.hpp\n> > > > @@ -6,6 +6,10 @@\n> > > > */\n> > > > #pragma once\n> > > >\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > #include \n> > > >\n> > > > /* Camera metadata parser class. Usage as shown below.\n> > > > @@ -16,7 +20,8 @@\n> > > > * application code doesn't have to worry which to 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> > > > + ParseGainCode,\n> ParseExposureLines));\n> > > > * parser->SetBitsPerPixel(bpp);\n> > > > * parser->SetLineLengthBytes(pitch);\n> > > > * parser->SetNumLines(2);\n> > > > @@ -113,14 +118,32 @@ 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> > > > + struct Register {\n> > > > + Register()\n> > > > + : offset(0), value(0), found(false)\n> > > > + {\n> > > > + }\n> > > > +\n> > > > + uint32_t offset;\n> > > > + uint32_t value;\n> > > > + bool found;\n> > > > + };\n> > > >\n> > > > -protected:\n> > > > + /* Maps register address to offset in the buffer. */\n> > > > + using RegMap = std::map;\n> > > > + using GetFn = std::function;\n> > > > +\n> > > > + MdParserSmia(const std::vector ®s, GetFn gain_fn,\n> > > > + GetFn exposureFn);\n> > > > +\n> > > > + MdParser::Status Parse(libcamera::Span buffer)\n> override;\n> > > > + Status GetExposureLines(unsigned int &lines) override;\n> > > > + Status GetGainCode(unsigned int &gain_code) override;\n> > > > +\n> > > > +private:\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> > > > @@ -138,8 +161,11 @@ 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> > > > + RegMap map_;\n> > > > + GetFn gain_fn_;\n> > > > + GetFn exposure_fn_;\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 65ffbe00c76e..f4748dd535d0 100644\n> > > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > > @@ -8,9 +8,11 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \"libcamera/internal/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\n> (not\n> > > > @@ -28,18 +30,79 @@ 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(const std::vector ®isters,\n> > > > + GetFn gain_fn, GetFn exposure_fn)\n> > > > + : gain_fn_(gain_fn), exposure_fn_(exposure_fn)\n> > > > {\n> > > > - assert(num_regs > 0);\n> > > > + for (auto r : registers)\n> > > > + map_[r] = {};\n> > > > +}\n> > > > +\n> > > > +MdParser::Status MdParserSmia::Parse(libcamera::Span\n> buffer)\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> > > > +\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> > > > + for (auto &kv : map_) {\n> > > > + Register ® = kv.second;\n> > > > +\n> > > > + if (!reg.found) {\n> > > > + reset_ = true;\n> > > > + return NOTFOUND;\n> > > > + }\n> > > > +\n> > > > + reg.value = buffer[reg.offset];\n> > > > + }\n> > > > +\n> > > > + return OK;\n> > > > +}\n> > > > +\n> > > > +MdParser::Status MdParserSmia::GetExposureLines(unsigned int &lines)\n> > > > +{\n> > > > + if (reset_)\n> > > > + return NOTFOUND;\n> > > > +\n> > > > + lines = exposure_fn_(map_);\n> > > > + return OK;\n> > > > +}\n> > > > +\n> > > > +MdParser::Status MdParserSmia::GetGainCode(unsigned int &gain_code)\n> > > > +{\n> > > > + if (reset_)\n> > > > + return NOTFOUND;\n> > > > +\n> > > > + gain_code = gain_fn_(map_);\n> > > > + return OK;\n> > > > +}\n> > > > +\n> > > > +MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span buffer)\n> > > > +{\n> > > > + ASSERT(map_.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> > > > @@ -73,8 +136,8 @@ MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span\n> > > > return NO_LINE_START;\n> > > > } else {\n> > > > /* allow a zero line length to mean\n> \"hunt for the next line\" */\n> > > > - while (buffer[current_offset] !=\n> LINE_START &&\n> > > > - current_offset < buffer.size())\n> > > > + while (current_offset < buffer.size()\n> &&\n> > > > + buffer[current_offset] !=\n> LINE_START)\n> > > > current_offset++;\n> > > >\n> > > > if (current_offset == buffer.size())\n> > > > @@ -91,13 +154,13 @@ 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 = map_.find(reg_num);\n> > > > +\n> > > > + if (reg != map_.end()) {\n> > > > + map_[reg_num].offset =\n> current_offset - 1;\n> > > > + map_[reg_num].found = true;\n> > > >\n> > > > - if (++first_reg == num_regs)\n> > > > + if (++regs_done == map_.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 D801BBD78E\n\tfor ;\n\tTue, 15 Jun 2021 10:04:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22A5F68942;\n\tTue, 15 Jun 2021 12:04:33 +0200 (CEST)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 405FD60297\n\tfor ;\n\tTue, 15 Jun 2021 12:04:31 +0200 (CEST)","by mail-lf1-x12c.google.com with SMTP id a1so25990000lfr.12\n\tfor ;\n\tTue, 15 Jun 2021 03:04:31 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"dJHpA8Mv\"; 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=yg29mRqOSrWS1uLQLJeiotnl+onwFh/0mzVnHirR8E8=;\n\tb=dJHpA8MviXdstmrMc46UlfKu1P6urPu7a3zirpwReKNWm7fEYpcWXcBaopj8b0GUTI\n\tPX8F9QaMiWoFYCwub4ubv1M2coN3YBac2F8//XMA/mYuyzbqcL2LmqTG2ca14oaFyqmg\n\t30mVkkQIectSr6KNpo8VpkUTP5Tk+0qNoqcVQw6dRLc+PA7yLvt/HoMQgDSnNK//yWED\n\tRVwFXucQ68jNLkKRkfRLOIMHYBRSYRXmpXmFsqGxbMFhVAM1WFIgwFpl8QUM7h4hVDqD\n\tgZ5GRU3uLzr/FhvNvpcv9vXIjqzoAcDyW5cqfQrW4uWMKxCtUrKBwkRhpQQPT8a/9bMS\n\tfyNQ==","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=yg29mRqOSrWS1uLQLJeiotnl+onwFh/0mzVnHirR8E8=;\n\tb=EzFCZypWqSHw9WfHJLOsl6807wsQK8lOoOcxxFxMNJOd7Bmm3XrtibS7GA42fKReew\n\tf7FcNWIOnMBSWgzyn8iWSL1Zqs8XmRk6cX8zvK0y5613bQGBaVpVcGwYbaMYqKUF7KFo\n\th+2Gh4DjT0ARpqeswXIK3SChb2AYgTmXEYbXqH2mR5LJOxSwFoFR/TTET0th+O+2OW01\n\tjcxHru88F2a/RE7E/VJ0Vf9/6g7+dwxrFK4Dpbyw4ujZl35oxuf1oIcZmtnj/AE+fp1m\n\tkNhaYsmBQqRbb0YOOQ9pCYPdqs3infzCFAGIEcjdp6LtrovYGcGyoUVWgbE3VQXadtfT\n\tHkeg==","X-Gm-Message-State":"AOAM530U7v2vsEh2cHyYdqqrmH+4TBhMbqHBzP6JztT2SGFOPIaoAOrb\n\thYtg7JfcgqlygA1Y8HCamcJO63KPSuSsdvugf16cOQ==","X-Google-Smtp-Source":"ABdhPJwguPInl+H2Xipxjzyjqr+HpAmFmgdcC3gXWIrUUJBkpC9QlkbMqWXQaCvYnY3tiHwkW7a5wcIp4wDqeDNx7gI=","X-Received":"by 2002:ac2:5193:: with SMTP id\n\tu19mr14877177lfi.210.1623751470551; \n\tTue, 15 Jun 2021 03:04:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20210614095340.3051816-1-naush@raspberrypi.com>\n\t<20210614095340.3051816-7-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 15 Jun 2021 11:04:14 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000009b283005c4cb1958\"","Subject":"Re: [libcamera-devel] [PATCH 6/6] ipa: raspberrypi: Generalise the\n\tSMIA 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\" "}}]