[libcamera-devel] [PATCH v3 2/2] ipa: raspberrypi: Generalise the SMIA metadata parser
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 29 21:29:45 CEST 2021
Hi Naush,
On Tue, Jun 29, 2021 at 07:20:13PM +0100, Naushir Patuck wrote:
> On Tue, 29 Jun 2021 at 16:47, Laurent Pinchart wrote:
> > On Tue, Jun 29, 2021 at 05:44:14PM +0300, Laurent Pinchart wrote:
> > > On Tue, Jun 29, 2021 at 02:53:50PM +0100, Naushir Patuck wrote:
> > > > On Tue, 29 Jun 2021 at 11:45, Naushir Patuck wrote:
> > > >
> > > > > 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.
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > ---
> > > > > src/ipa/raspberrypi/cam_helper.cpp | 33 +++---
> > > > > src/ipa/raspberrypi/cam_helper.hpp | 2 +
> > > > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------
> > > > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------
> > > > > src/ipa/raspberrypi/md_parser.hpp | 41 +++++---
> > > > > src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--
> > > > > 6 files changed, 144 insertions(+), 234 deletions(-)
> > > > >
> > > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> > > > > index 90498c37af98..e6d2258c66d7 100644
> > > > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > > > @@ -159,32 +159,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const
> > > > > void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> > > > > Metadata &metadata)
> > > > > {
> > > > > + MdParser::RegisterMap registers;
> > > > > + Metadata parsedMetadata;
> > > > > +
> > > > > if (buffer.empty())
> > > > > return;
> > > > >
> > > > > - uint32_t exposureLines, gainCode;
> > > > > -
> > > > > - if (parser_->Parse(buffer) != MdParser::Status::OK ||
> > > > > - parser_->GetExposureLines(exposureLines) != MdParser::Status::OK ||
> > > > > - parser_->GetGainCode(gainCode) != MdParser::Status::OK) {
> > > > > + if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {
> > > > > LOG(IPARPI, Error) << "Embedded data buffer parsing failed";
> > > > > return;
> > > > > }
> > > > >
> > > > > + PopulateMetadata(registers, parsedMetadata);
> > > > > + metadata.Merge(parsedMetadata);
> > > > > +
> > > > > /*
> > > > > - * Overwrite the exposure/gain values in the DeviceStatus, as
> > > > > - * we know better. Fetch it first in case any other fields were
> > > > > - * set meaningfully.
> > > > > + * Overwrite the exposure/gain values in the existing DeviceStatus with
> > > > > + * values from the parsed embedded buffer. Fetch it first in case any
> > > > > + * other fields were set meaningfully.
> > > > > */
> > > > > - DeviceStatus deviceStatus;
> > > > > -
> > > > > - if (metadata.Get("device.status", deviceStatus) != 0) {
> > > > > + DeviceStatus deviceStatus, parsedDeviceStatus;
> > > > > + if (metadata.Get("device.status", deviceStatus) ||
> > > > > + parsedMetadata.Get("device.status", parsedDeviceStatus)) {
> > > > > LOG(IPARPI, Error) << "DeviceStatus not found";
> > > > > return;
> > > > > }
> > > > >
> > > > > - deviceStatus.shutter_speed = Exposure(exposureLines);
> > > > > - deviceStatus.analogue_gain = Gain(gainCode);
> > > > > + deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
> > > > > + deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
> > > > >
> > > > > LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > > > > << deviceStatus.shutter_speed
> > > > > @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> > > > > metadata.Set("device.status", deviceStatus);
> > > > > }
> > > > >
> > > > > +void CamHelper::PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap ®isters,
> > > > > + [[maybe_unused]] Metadata &metadata) const
> > > > > +{
> > > > > +}
> > > > > +
> > > > > RegisterCamHelper::RegisterCamHelper(char const *cam_name,
> > > > > CamHelperCreateFunc create_func)
> > > > > {
> > > > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> > > > > index fc3139e22be0..200cc83f3872 100644
> > > > > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > > > > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > > > > @@ -92,6 +92,8 @@ public:
> > > > > protected:
> > > > > void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
> > > > > Metadata &metadata);
> > > > > + virtual void PopulateMetadata(const MdParser::RegisterMap ®isters,
> > > > > + Metadata &metadata) const;
> > > > >
> > > > > std::unique_ptr<MdParser> parser_;
> > > > > CameraMode mode_;
> > > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > > index c85044a5fa6d..4e4994188ff3 100644
> > > > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > > @@ -23,21 +23,14 @@
> > > > >
> > > > > using namespace RPiController;
> > > > >
> > > > > -/* Metadata parser implementation specific to Sony IMX219 sensors. */
> > > > > -
> > > > > -class MdParserImx219 : public MdParserSmia
> > > > > -{
> > > > > -public:
> > > > > - MdParserImx219();
> > > > > - Status Parse(libcamera::Span<const uint8_t> 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;
> > > > > +constexpr std::initializer_list<uint32_t> registerList = { expHiReg, expLoReg, gainReg };
> >
> > This needs a [[maybe_unused]] in case ENABLE_EMBEDDED_DATA isn't defined.
> >
> > > > >
> > > > > class CamHelperImx219 : public CamHelper
> > > > > {
> > > > > @@ -54,11 +47,14 @@ private:
> > > > > * in units of lines.
> > > > > */
> > > > > static constexpr int frameIntegrationDiff = 4;
> > > > > +
> > > > > + void PopulateMetadata(const MdParser::RegisterMap ®isters,
> > > > > + Metadata &metadata) const override;
> > > > > };
> > > > >
> > > > > CamHelperImx219::CamHelperImx219()
> > > > > #if ENABLE_EMBEDDED_DATA
> > > > > - : CamHelper(std::make_unique<MdParserImx219>(), frameIntegrationDiff)
> > > > > + : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff)
> > > > > #else
> > > > > : CamHelper({}, frameIntegrationDiff)
> > > > > #endif
> > > > > @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const
> > > > > return ENABLE_EMBEDDED_DATA;
> > > > > }
> > > > >
> > > > > -static CamHelper *Create()
> > > > > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap ®isters,
> > > > > + Metadata &metadata) const
> > > > > {
> > > > > - return new CamHelperImx219();
> > > > > -}
> > > > > + DeviceStatus deviceStatus;
> > > > >
> > > > > -static RegisterCamHelper reg("imx219", &Create);
> > > > > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));
> > > > > + deviceStatus.analogue_gain = Gain(registers.at(gainReg));
> > > > >
> > > > > -/*
> > > > > - * 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;
> > > > > + metadata.Set("device.status", deviceStatus);
> > > > > }
> > > > >
> > > > > -MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t> 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<int>(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)
> > > > > +static CamHelper *Create()
> > > > > {
> > > > > - 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;
> > > > > + return new CamHelperImx219();
> > > > > }
> > > > >
> > > > > -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)
> > > > > -{
> > > > > - if (reg_offsets_[GAIN_INDEX] == -1)
> > > > > - return NOTFOUND;
> > > > > -
> > > > > - gain_code = reg_values_[GAIN_INDEX];
> > > > > -
> > > > > - return OK;
> > > > > -}
> > > > > +static RegisterCamHelper reg("imx219", &Create);
> > > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > > index d72a9be0438e..efd1a5893db8 100644
> > > > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > > @@ -15,21 +15,15 @@
> > > > >
> > > > > using namespace RPiController;
> > > > >
> > > > > -/* Metadata parser implementation specific to Sony IMX477 sensors. */
> > > > > -
> > > > > -class MdParserImx477 : public MdParserSmia
> > > > > -{
> > > > > -public:
> > > > > - MdParserImx477();
> > > > > - Status Parse(libcamera::Span<const uint8_t> 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;
> > > > > +constexpr std::initializer_list<uint32_t> registerList = { expHiReg, expLoReg, gainHiReg, gainLoReg };
> >
> > Same here.
> >
> > > > >
> > > > > class CamHelperImx477 : public CamHelper
> > > > > {
> > > > > @@ -47,10 +41,13 @@ private:
> > > > > * in units of lines.
> > > > > */
> > > > > static constexpr int frameIntegrationDiff = 22;
> > > > > +
> > > > > + void PopulateMetadata(const MdParser::RegisterMap ®isters,
> > > > > + Metadata &metadata) const override;
> > > > > };
> > > > >
> > > > > CamHelperImx477::CamHelperImx477()
> > > > > - : CamHelper(std::make_unique<MdParserImx477>(), frameIntegrationDiff)
> > > > > + : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff)
> > > > > {
> > > > > }
> > > > >
> > > > > @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const
> > > > > return true;
> > > > > }
> > > > >
> > > > > -static CamHelper *Create()
> > > > > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap ®isters,
> > > > > + Metadata &metadata) const
> > > > > {
> > > > > - return new CamHelperImx477();
> > > > > -}
> > > > > + DeviceStatus deviceStatus;
> > > > >
> > > > > -static RegisterCamHelper reg("imx477", &Create);
> > > > > + deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));
> > > > > + deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));
> > > > >
> > > > > -/*
> > > > > - * 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;
> > > > > + metadata.Set("device.status", deviceStatus);
> > > > > }
> > > > >
> > > > > -MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t> 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<int>(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)
> > > > > +static CamHelper *Create()
> > > > > {
> > > > > - 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;
> > > > > + return new CamHelperImx477();
> > > > > }
> > > > >
> > > > > -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;
> > > > > -}
> > > > > +static RegisterCamHelper reg("imx477", &Create);
> > > > > diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp
> > > > > index 8497216f8db7..e3e2738537a8 100644
> > > > > --- a/src/ipa/raspberrypi/md_parser.hpp
> > > > > +++ b/src/ipa/raspberrypi/md_parser.hpp
> > > > > @@ -6,6 +6,9 @@
> > > > > */
> > > > > #pragma once
> > > > >
> > > > > +#include <initializer_list>
> > > > > +#include <map>
> > > > > +#include <optional>
> > > > > #include <stdint.h>
> > > > >
> > > > > #include <libcamera/base/span.h>
> > > > > @@ -19,7 +22,7 @@
> > > > > * application code doesn't have to worry which 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 });
> > > > > * parser->SetBitsPerPixel(bpp);
> > > > > * parser->SetLineLengthBytes(pitch);
> > > > > * parser->SetNumLines(2);
> > > > > @@ -32,13 +35,11 @@
> > > > > *
> > > > > * Then on every frame:
> > > > > *
> > > > > - * if (parser->Parse(buffer) != MdParser::OK)
> > > > > + * RegisterMap registers;
> > > > > + * if (parser->Parse(buffer, registers) != MdParser::OK)
> > > > > * much badness;
> > > > > - * unsigned int exposure_lines, gain_code
> > > > > - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)
> > > > > - * exposure was not found;
> > > > > - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)
> > > > > - * gain code was not found;
> > > > > + * Metadata metadata;
> > > > > + * CamHelper::PopulateMetadata(registers, metadata);
> > > > > *
> > > > > * (Note that the CamHelper class converts to/from exposure lines and time,
> > > > > * and gain_code / actual gain.)
> > > > > @@ -59,6 +60,8 @@ namespace RPiController {
> > > > > class MdParser
> > > > > {
> > > > > public:
> > > > > + using RegisterMap = std::map<uint32_t, uint32_t>;
> > > > > +
> > > > > /*
> > > > > * Parser status codes:
> > > > > * OK - success
> > > > > @@ -98,9 +101,8 @@ public:
> > > > > line_length_bytes_ = num_bytes;
> > > > > }
> > > > >
> > > > > - virtual Status Parse(libcamera::Span<const uint8_t> buffer) = 0;
> > > > > - virtual Status GetExposureLines(unsigned int &lines) = 0;
> > > > > - virtual Status GetGainCode(unsigned int &gain_code) = 0;
> > > > > + virtual Status Parse(libcamera::Span<const uint8_t> buffer,
> > > > > + RegisterMap ®isters) = 0;
> > > > >
> > > > > protected:
> > > > > bool reset_;
> > > > > @@ -116,14 +118,18 @@ protected:
> > > > > * md_parser_imx219.cpp for an example).
> > > > > */
> > > > >
> > > > > -class MdParserSmia : public MdParser
> > > > > +class MdParserSmia final : public MdParser
> > > > > {
> > > > > public:
> > > > > - MdParserSmia() : MdParser()
> > > > > - {
> > > > > - }
> > > > > + MdParserSmia(std::initializer_list<uint32_t> registerList);
> > > > > +
> > > > > + MdParser::Status Parse(libcamera::Span<const uint8_t> buffer,
> > > > > + RegisterMap ®isters) override;
> > > > > +
> > > > > +private:
> > > > > + /* Maps register address to offset in the buffer. */
> > > > > + using OffsetMap = std::map<uint32_t, std::optional<uint32_t>>;
> > > > >
> > > > > -protected:
> > > > > /*
> > > > > * Note that error codes > 0 are regarded as non-fatal; codes < 0
> > > > > * indicate a bad data buffer. Status codes are:
> > > > > @@ -141,8 +147,9 @@ protected:
> > > > > BAD_PADDING = -5
> > > > > };
> > > > >
> > > > > - ParseStatus findRegs(libcamera::Span<const uint8_t> buffer, uint32_t regs[],
> > > > > - int offsets[], unsigned int num_regs);
> > > > > + ParseStatus findRegs(libcamera::Span<const uint8_t> buffer);
> > > > > +
> > > > > + OffsetMap offsets_;
> > > > > };
> > > > >
> > > > > } // namespace RPi
> > > > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp
> > > > > index 0a14875575a2..7f72fc03948f 100644
> > > > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp
> > > > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp
> > > > > @@ -6,9 +6,11 @@
> > > > > */
> > > > > #include <assert.h>
> > > > >
> > > >
> > > > Oops, this header is not needed any more!
> > > >
> > > > > +#include "libcamera/base/log.h"
> > >
> > > And this should be <libcamera/base/log.h>. I'll fix when applying.
> > >
> > > > > #include "md_parser.hpp"
> > > > >
> > > > > using namespace RPiController;
> > > > > +using namespace libcamera;
> > > > >
> > > > > /*
> > > > > * This function goes through the embedded data to find the offsets (not
> > > > > @@ -26,18 +28,61 @@ 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<const uint8_t> buffer,
> > > > > - uint32_t regs[], int offsets[],
> > > > > - unsigned int num_regs)
> > > > > +MdParserSmia::MdParserSmia(std::initializer_list<uint32_t> registerList)
> > > > > {
> > > > > - assert(num_regs > 0);
> > > > > + for (auto r : registerList)
> > > > > + offsets_[r] = {};
> > > > > +}
> > > > > +
> > > > > +MdParser::Status MdParserSmia::Parse(libcamera::Span<const uint8_t> buffer,
> > > > > + RegisterMap ®isters)
> > > > > +{
> > > > > + if (reset_) {
> > > > > + /*
> > > > > + * Search again through the metadata for all the registers
> > > > > + * requested.
> > > > > + */
> > > > > + ASSERT(bits_per_pixel_);
> > > > > +
> > > > > + for (const auto &[reg, offset] : offsets_)
> > > > > + offsets_[reg] = {};
> >
> > I'm afraid this causes compilation failures with gcc 7 and gcc 8, due to
> > offset being unused. We can't throw a [[maybe_unused]] here, that's not
> > supported, so we need to revert to the syntax from v2.
> >
> > for (const auto &kv : offsets_)
> > offsets_[kv.first] = {};
> >
> > I'll post a v3.1, Naush, could you give it a try ?
>
> Thank you for the fixups! I'm a bit curious why my compiler did not throw out warnings
> for these though...?
It got fixed in gcc 9, maybe that's why ?
> I'll test and report back first thing tomorrow.
Thank you.
> > > > > +
> > > > > + 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. */
> > > > > + registers.clear();
> > > > > + for (const auto &[reg, offset] : offsets_) {
> > > > > + if (!offset) {
> > > > > + reset_ = true;
> > > > > + return NOTFOUND;
> > > > > + }
> > > > > + registers[reg] = buffer[offset.value()];
> > > > > + }
> > > > > +
> > > > > + return OK;
> > > > > +}
> > > > > +
> > > > > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer)
> > > > > +{
> > > > > + ASSERT(offsets_.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++];
> > > > > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t>
> > > > > 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 = offsets_.find(reg_num);
> > > > > +
> > > > > + if (reg != offsets_.end()) {
> > > > > + offsets_[reg_num] = current_offset - 1;
> > > > >
> > > > > - if (++first_reg == num_regs)
> > > > > + if (++regs_done == offsets_.size())
> > > > > return PARSE_OK;
> > > > > }
> > > > > reg_num++;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list