From patchwork Mon Jun 14 09:53:40 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Naushir Patuck X-Patchwork-Id: 12584 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 2ADFCC3218 for ; Mon, 14 Jun 2021 09:53:57 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id C8A1A68933; Mon, 14 Jun 2021 11:53:56 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="Dhk43McI"; dkim-atps=neutral Received: from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com [IPv6:2a00:1450:4864:20::32b]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 3144768937 for ; Mon, 14 Jun 2021 11:53:50 +0200 (CEST) Received: by mail-wm1-x32b.google.com with SMTP id b205so7061152wmb.3 for ; Mon, 14 Jun 2021 02:53:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=zdn6EoWhiSuHC85PVxRu8OiYwIw3pJWhCuzsm/sYzZ8=; b=Dhk43McIElE50QB17wAX7E0cU12M8wLYzHmyUwNetylOSCA7dBS/yVf59byYAOxysq m0Lr26fV+PELrhQtoOvpbyUfwccvCmNkVxNEpE8ZsTnk+TLLNDEuGXsXVlGh0M/Mqt+a qJkE+YgYsDit79O4TB/VSFtgXYdJDuKskRRSJ1BJ+oviECsuFU6FuEKAMAw5bF/+J8Nd N6PldwmjFpl35BIeBJhNQpksVwnZah9nqmFx+nOxM1mYzKJVxWfxZhbxWIRPUWKwu9LM wS6r7NPnZ4kBD4py+P7ru/bcp0FAsZ+bQqsqX6NT9byivS/yq20M8j/Z6rhquSOjdWiM aTGA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=zdn6EoWhiSuHC85PVxRu8OiYwIw3pJWhCuzsm/sYzZ8=; b=j8vt9jwoy5ZFBSAXayT/AJpw3Bq2yYimvVNpeQkFCBkOmu+azEYxudC/lOpu2Nu+30 NCESp4cF2b/hTffh+Q1vfVgmSKNL8dmXZmgO39gwZlAt8s+1hSBGBp8Wz+XWAH0iDYvO Pb7NOw5RDDBIKWOTbOcY820DQ7JQvFPE+8hGlyQ86FdzCNaSlk8YFgnU0deeO9qofx2b 6uniH05N8DpTTuiaIgcLTC8QgMFnat+qwZMB/NsVCP5o7sYt99VtwEhxlQzOQEjA+SkL 77YjcEzp5AdXdiYvlXtkKKk3sXS5ZoG3b75in+EUJN5WryC8cPTw60WPcAsWsWVi0/cA Ujuw== X-Gm-Message-State: AOAM5320PWMc8OUy5rxmZTeUKR3WOG2/jGQi6NB9iI/Pm64soZJ1Bebh I94PLcyuOXg56Zq8TFh1uu8PxTho7h4Wlw== X-Google-Smtp-Source: ABdhPJz8abpU5A0xGM0NyYNg0CYbXLsMPeehknLPv/+0iHFJWL03hC1njrzb8qfILu0nyElGVX2CAQ== X-Received: by 2002:a1c:7210:: with SMTP id n16mr30886583wmc.75.1623664429324; Mon, 14 Jun 2021 02:53:49 -0700 (PDT) Received: from naush-laptop.pitowers.org ([2a00:1098:3142:14:fd93:d554:2dff:83ca]) by smtp.gmail.com with ESMTPSA id c2sm19834891wmf.24.2021.06.14.02.53.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 14 Jun 2021 02:53:48 -0700 (PDT) From: Naushir Patuck To: libcamera-devel@lists.libcamera.org Date: Mon, 14 Jun 2021 10:53:40 +0100 Message-Id: <20210614095340.3051816-7-naush@raspberrypi.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210614095340.3051816-1-naush@raspberrypi.com> References: <20210614095340.3051816-1-naush@raspberrypi.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 6/6] ipa: raspberrypi: Generalise the SMIA metadata parser X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Instead of having each CamHelper subclass the MdParserSmia, change the implementation of MdParserSmia to be more generic. The MdParserSmia now gets given a list of registers to search for and helper functions are used to compute exposure lines and gain codes from these registers. Update the imx219 and imx477 CamHelpers by using this new mechanism. As a drive-by change, fixup a possible buffer overrun in the parsing code. Signed-off-by: Naushir Patuck --- src/ipa/raspberrypi/cam_helper_imx219.cpp | 117 ++++---------------- src/ipa/raspberrypi/cam_helper_imx477.cpp | 125 ++++------------------ src/ipa/raspberrypi/md_parser.hpp | 42 ++++++-- src/ipa/raspberrypi/md_parser_smia.cpp | 89 ++++++++++++--- 4 files changed, 153 insertions(+), 220 deletions(-) diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp index 36dbe8cd941a..72c1042ad6be 100644 --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp @@ -23,21 +23,13 @@ using namespace RPiController; -/* Metadata parser implementation specific to Sony IMX219 sensors. */ - -class MdParserImx219 : public MdParserSmia -{ -public: - MdParserImx219(); - Status Parse(libcamera::Span buffer) override; - Status GetExposureLines(unsigned int &lines) override; - Status GetGainCode(unsigned int &gain_code) override; -private: - /* Offset of the register's value in the metadata block. */ - int reg_offsets_[3]; - /* Value of the register, once read from the metadata block. */ - int reg_values_[3]; -}; +/* + * We care about one gain register and a pair of exposure registers. Their I2C + * addresses from the Sony IMX219 datasheet: + */ +constexpr uint32_t gainReg = 0x157; +constexpr uint32_t expHiReg = 0x15A; +constexpr uint32_t expLoReg = 0x15B; class CamHelperImx219 : public CamHelper { @@ -55,11 +47,23 @@ private: */ static constexpr int frameIntegrationDiff = 4; - MdParserImx219 imx219_parser; + MdParserSmia imx219_parser; + + static uint32_t ParseExposureLines(const MdParserSmia::RegMap &map) + { + return map.at(expHiReg).value * 256 + map.at(expLoReg).value; + } + + static uint32_t ParseGainCode(const MdParserSmia::RegMap &map) + { + return map.at(gainReg).value; + } }; CamHelperImx219::CamHelperImx219() - : CamHelper(frameIntegrationDiff) + : CamHelper(frameIntegrationDiff), + imx219_parser({ expHiReg, expLoReg, gainReg }, + ParseGainCode, ParseExposureLines) { #if ENABLE_EMBEDDED_DATA parser_ = &imx219_parser; @@ -97,82 +101,3 @@ static CamHelper *Create() } static RegisterCamHelper reg("imx219", &Create); - -/* - * We care about one gain register and a pair of exposure registers. Their I2C - * addresses from the Sony IMX219 datasheet: - */ -#define GAIN_REG 0x157 -#define EXPHI_REG 0x15A -#define EXPLO_REG 0x15B - -/* - * Index of each into the reg_offsets and reg_values arrays. Must be in - * register address order. - */ -#define GAIN_INDEX 0 -#define EXPHI_INDEX 1 -#define EXPLO_INDEX 2 - -MdParserImx219::MdParserImx219() -{ - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1; -} - -MdParser::Status MdParserImx219::Parse(libcamera::Span buffer) -{ - bool try_again = false; - - if (reset_) { - /* - * Search again through the metadata for the gain and exposure - * registers. - */ - assert(bits_per_pixel_); - /* Need to be ordered */ - uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG }; - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1; - int ret = static_cast(findRegs(buffer, - regs, reg_offsets_, 3)); - /* - * > 0 means "worked partially but parse again next time", - * < 0 means "hard error". - */ - if (ret > 0) - try_again = true; - else if (ret < 0) - return ERROR; - } - - for (int i = 0; i < 3; i++) { - if (reg_offsets_[i] == -1) - continue; - - reg_values_[i] = buffer[reg_offsets_[i]]; - } - - /* Re-parse next time if we were unhappy in some way. */ - reset_ = try_again; - - return OK; -} - -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines) -{ - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1) - return NOTFOUND; - - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX]; - - return OK; -} - -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code) -{ - if (reg_offsets_[GAIN_INDEX] == -1) - return NOTFOUND; - - gain_code = reg_values_[GAIN_INDEX]; - - return OK; -} diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp index 038a8583d311..7a1100c25afc 100644 --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp @@ -15,21 +15,14 @@ using namespace RPiController; -/* Metadata parser implementation specific to Sony IMX477 sensors. */ - -class MdParserImx477 : public MdParserSmia -{ -public: - MdParserImx477(); - Status Parse(libcamera::Span buffer) override; - Status GetExposureLines(unsigned int &lines) override; - Status GetGainCode(unsigned int &gain_code) override; -private: - /* Offset of the register's value in the metadata block. */ - int reg_offsets_[4]; - /* Value of the register, once read from the metadata block. */ - int reg_values_[4]; -}; +/* + * We care about two gain registers and a pair of exposure registers. Their + * I2C addresses from the Sony IMX477 datasheet: + */ +constexpr uint32_t expHiReg = 0x0202; +constexpr uint32_t expLoReg = 0x0203; +constexpr uint32_t gainHiReg = 0x0204; +constexpr uint32_t gainLoReg = 0x0205; class CamHelperImx477 : public CamHelper { @@ -48,11 +41,23 @@ private: */ static constexpr int frameIntegrationDiff = 22; - MdParserImx477 imx477_parser; + MdParserSmia imx477_parser; + + static uint32_t ParseExposureLines(const MdParserSmia::RegMap &map) + { + return map.at(expHiReg).value * 256 + map.at(expLoReg).value; + } + + static uint32_t ParseGainCode(const MdParserSmia::RegMap &map) + { + return map.at(gainHiReg).value * 256 + map.at(gainLoReg).value; + } }; CamHelperImx477::CamHelperImx477() - : CamHelper(frameIntegrationDiff) + : CamHelper(frameIntegrationDiff), + imx477_parser({ expHiReg, expLoReg, gainHiReg, gainLoReg }, + ParseGainCode, ParseExposureLines) { parser_ = &imx477_parser; } @@ -86,89 +91,3 @@ static CamHelper *Create() } static RegisterCamHelper reg("imx477", &Create); - -/* - * We care about two gain registers and a pair of exposure registers. Their - * I2C addresses from the Sony IMX477 datasheet: - */ -#define EXPHI_REG 0x0202 -#define EXPLO_REG 0x0203 -#define GAINHI_REG 0x0204 -#define GAINLO_REG 0x0205 - -/* - * Index of each into the reg_offsets and reg_values arrays. Must be in register - * address order. - */ -#define EXPHI_INDEX 0 -#define EXPLO_INDEX 1 -#define GAINHI_INDEX 2 -#define GAINLO_INDEX 3 - -MdParserImx477::MdParserImx477() -{ - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1; -} - -MdParser::Status MdParserImx477::Parse(libcamera::Span buffer) -{ - bool try_again = false; - - if (reset_) { - /* - * Search again through the metadata for the gain and exposure - * registers. - */ - assert(bits_per_pixel_); - /* Need to be ordered */ - uint32_t regs[4] = { - EXPHI_REG, - EXPLO_REG, - GAINHI_REG, - GAINLO_REG - }; - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1; - int ret = static_cast(findRegs(buffer, - regs, reg_offsets_, 4)); - /* - * > 0 means "worked partially but parse again next time", - * < 0 means "hard error". - */ - if (ret > 0) - try_again = true; - else if (ret < 0) - return ERROR; - } - - for (int i = 0; i < 4; i++) { - if (reg_offsets_[i] == -1) - continue; - - reg_values_[i] = buffer[reg_offsets_[i]]; - } - - /* Re-parse next time if we were unhappy in some way. */ - reset_ = try_again; - - return OK; -} - -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines) -{ - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1) - return NOTFOUND; - - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX]; - - return OK; -} - -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code) -{ - if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX] == -1) - return NOTFOUND; - - gain_code = reg_values_[GAINHI_INDEX] * 256 + reg_values_[GAINLO_INDEX]; - - return OK; -} diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp index 25ba0e7c9400..6bbcdec0830b 100644 --- a/src/ipa/raspberrypi/md_parser.hpp +++ b/src/ipa/raspberrypi/md_parser.hpp @@ -6,6 +6,10 @@ */ #pragma once +#include +#include +#include + #include /* Camera metadata parser class. Usage as shown below. @@ -16,7 +20,8 @@ * application code doesn't have to worry which to kind to instantiate. But for * the sake of example let's suppose we're parsing imx219 metadata. * - * MdParser *parser = new MdParserImx219(); // for example + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg }, + ParseGainCode, ParseExposureLines)); * parser->SetBitsPerPixel(bpp); * parser->SetLineLengthBytes(pitch); * parser->SetNumLines(2); @@ -113,14 +118,32 @@ protected: * md_parser_imx219.cpp for an example). */ -class MdParserSmia : public MdParser +class MdParserSmia final : public MdParser { public: - MdParserSmia() : MdParser() - { - } + struct Register { + Register() + : offset(0), value(0), found(false) + { + } + + uint32_t offset; + uint32_t value; + bool found; + }; -protected: + /* Maps register address to offset in the buffer. */ + using RegMap = std::map; + using GetFn = std::function; + + MdParserSmia(const std::vector ®s, GetFn gain_fn, + GetFn exposureFn); + + MdParser::Status Parse(libcamera::Span buffer) override; + Status GetExposureLines(unsigned int &lines) override; + Status GetGainCode(unsigned int &gain_code) override; + +private: /* * Note that error codes > 0 are regarded as non-fatal; codes < 0 * indicate a bad data buffer. Status codes are: @@ -138,8 +161,11 @@ protected: BAD_PADDING = -5 }; - ParseStatus findRegs(libcamera::Span buffer, uint32_t regs[], - int offsets[], unsigned int num_regs); + ParseStatus findRegs(libcamera::Span buffer); + + RegMap map_; + GetFn gain_fn_; + GetFn exposure_fn_; }; } // namespace RPi diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp index 65ffbe00c76e..f4748dd535d0 100644 --- a/src/ipa/raspberrypi/md_parser_smia.cpp +++ b/src/ipa/raspberrypi/md_parser_smia.cpp @@ -8,9 +8,11 @@ #include #include +#include "libcamera/internal/log.h" #include "md_parser.hpp" using namespace RPiController; +using namespace libcamera; /* * This function goes through the embedded data to find the offsets (not @@ -28,18 +30,79 @@ constexpr unsigned int REG_LOW_BITS = 0xa5; constexpr unsigned int REG_VALUE = 0x5a; constexpr unsigned int REG_SKIP = 0x55; -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer, - uint32_t regs[], int offsets[], - unsigned int num_regs) +MdParserSmia::MdParserSmia(const std::vector ®isters, + GetFn gain_fn, GetFn exposure_fn) + : gain_fn_(gain_fn), exposure_fn_(exposure_fn) { - assert(num_regs > 0); + for (auto r : registers) + map_[r] = {}; +} + +MdParser::Status MdParserSmia::Parse(libcamera::Span buffer) +{ + if (reset_) { + /* + * Search again through the metadata for the gain and exposure + * registers. + */ + ASSERT(bits_per_pixel_); + + ParseStatus ret = findRegs(buffer); + /* + * > 0 means "worked partially but parse again next time", + * < 0 means "hard error". + * + * In either case, we retry parsing on the next frame. + */ + if (ret != PARSE_OK) + return ERROR; + + reset_ = false; + } + + /* Populate the register values requested. */ + for (auto &kv : map_) { + Register ® = kv.second; + + if (!reg.found) { + reset_ = true; + return NOTFOUND; + } + + reg.value = buffer[reg.offset]; + } + + return OK; +} + +MdParser::Status MdParserSmia::GetExposureLines(unsigned int &lines) +{ + if (reset_) + return NOTFOUND; + + lines = exposure_fn_(map_); + return OK; +} + +MdParser::Status MdParserSmia::GetGainCode(unsigned int &gain_code) +{ + if (reset_) + return NOTFOUND; + + gain_code = gain_fn_(map_); + return OK; +} + +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer) +{ + ASSERT(map_.size()); if (buffer[0] != LINE_START) return NO_LINE_START; unsigned int current_offset = 1; /* after the LINE_START */ unsigned int current_line_start = 0, current_line = 0; - unsigned int reg_num = 0, first_reg = 0; + unsigned int reg_num = 0, regs_done = 0; while (1) { int tag = buffer[current_offset++]; @@ -73,8 +136,8 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span return NO_LINE_START; } else { /* allow a zero line length to mean "hunt for the next line" */ - while (buffer[current_offset] != LINE_START && - current_offset < buffer.size()) + while (current_offset < buffer.size() && + buffer[current_offset] != LINE_START) current_offset++; if (current_offset == buffer.size()) @@ -91,13 +154,13 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span else if (tag == REG_SKIP) reg_num++; else if (tag == REG_VALUE) { - while (reg_num >= - /* assumes registers are in order... */ - regs[first_reg]) { - if (reg_num == regs[first_reg]) - offsets[first_reg] = current_offset - 1; + auto reg = map_.find(reg_num); + + if (reg != map_.end()) { + map_[reg_num].offset = current_offset - 1; + map_[reg_num].found = true; - if (++first_reg == num_regs) + if (++regs_done == map_.size()) return PARSE_OK; } reg_num++;