| Message ID | 20230626143932.18888-1-naush@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2023-06-26 15:39:32) > Enable embedded data parsing for the imx296 sensor. > > However, the imx296 has a quirk where the embedded data is ahead by a > single frame, i.e. embedded data in frame N corresponds to the image > data in frame N+1. So make a copy of the embedded data buffer stored in > the camera helper state, and use that copy in the next frame. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Anything blocking this patch? It looks like there was small discussion on v1 and that was handled with an update noted below. This seems reasonable to me here... so > --- > Changes since v1: > - Replace the use of std::unique_ptr with std::vector<uint8_t> > --- > src/ipa/rpi/cam_helper/cam_helper_imx296.cpp | 60 +++++++++++++++++++- > 1 file changed, 59 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > index ecb845e76e12..e84243704e5a 100644 > --- a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > @@ -7,7 +7,9 @@ > > #include <algorithm> > #include <cmath> > +#include <string.h> > #include <stddef.h> > +#include <vector> > > #include "cam_helper.h" > > @@ -15,6 +17,17 @@ using namespace RPiController; > using libcamera::utils::Duration; > using namespace std::literals::chrono_literals; > > +constexpr uint32_t gainReg = 0x41ba; > +constexpr uint32_t shsLoReg = 0x41b4; > +constexpr uint32_t shsMidReg = 0x41b5; > +constexpr uint32_t shsHiReg = 0x41b6; > +constexpr uint32_t vmaxLoReg = 0x41cc; > +constexpr uint32_t vmaxMidReg = 0x41cd; > +constexpr uint32_t vmaxHiReg = 0x41ce; > +constexpr uint32_t tempReg = 0x41da; > +constexpr std::initializer_list<uint32_t> registerList = > + { gainReg, shsLoReg, shsMidReg, shsHiReg, vmaxLoReg, vmaxMidReg, vmaxHiReg, tempReg }; > + > class CamHelperImx296 : public CamHelper > { > public: > @@ -23,8 +36,12 @@ public: > double gain(uint32_t gainCode) const override; > uint32_t exposureLines(const Duration exposure, const Duration lineLength) const override; > Duration exposure(uint32_t exposureLines, const Duration lineLength) const override; > + void prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) override; > void getDelays(int &exposureDelay, int &gainDelay, > int &vblankDelay, int &hblankDelay) const override; > + bool sensorEmbeddedDataPresent() const override; > + void populateMetadata(const MdParser::RegisterMap ®isters, > + Metadata &metadata) const override; > > private: > static constexpr uint32_t minExposureLines = 1; > @@ -36,10 +53,12 @@ private: > * in units of lines. > */ > static constexpr int frameIntegrationDiff = 4; > + > + std::vector<uint8_t> lastEmbeddedBuffer_; > }; > > CamHelperImx296::CamHelperImx296() > - : CamHelper(nullptr, frameIntegrationDiff) > + : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff) > { > } > > @@ -66,6 +85,23 @@ Duration CamHelperImx296::exposure(uint32_t exposureLines, > return std::max<uint32_t>(minExposureLines, exposureLines) * timePerLine + 14.26us; > } > > +void CamHelperImx296::prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) > +{ > + /* > + * The imx296 embedded data is ahead by a single frame, i.e. embedded > + * data in frame N corresponds to the image data in frame N+1. So make > + * a copy of the embedded data buffer and use it as normal for the next > + * frame. > + */ > + CamHelper::prepare({ lastEmbeddedBuffer_.data(), lastEmbeddedBuffer_.size() }, > + metadata); > + > + if (lastEmbeddedBuffer_.size() != buffer.size()) > + lastEmbeddedBuffer_.resize(buffer.size()); > + > + memcpy(lastEmbeddedBuffer_.data(), buffer.data(), buffer.size()); > +} > + > void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay, > int &vblankDelay, int &hblankDelay) const > { > @@ -75,6 +111,28 @@ void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay, > hblankDelay = 2; > } > > +bool CamHelperImx296::sensorEmbeddedDataPresent() const > +{ > + return true; > +} > + > +void CamHelperImx296::populateMetadata(const MdParser::RegisterMap ®isters, > + Metadata &metadata) const > +{ > + uint32_t shs = registers.at(shsLoReg) + (registers.at(shsMidReg) << 8) + > + (registers.at(shsHiReg) << 16); > + uint32_t vmax = registers.at(vmaxLoReg) + (registers.at(vmaxMidReg) << 8) + > + (registers.at(vmaxHiReg) << 16); Maybe in the future we could add some register access helpers to make this easier? But I don't see that blocking here. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > + DeviceStatus deviceStatus; > + deviceStatus.lineLength = timePerLine; > + deviceStatus.frameLength = vmax; > + deviceStatus.shutterSpeed = exposure(vmax - shs, timePerLine); > + deviceStatus.analogueGain = gain(registers.at(gainReg)); > + > + metadata.set("device.status", deviceStatus); > +} > + > static CamHelper *create() > { > return new CamHelperImx296(); > -- > 2.34.1 >
Hi Kieran, On Mon, 25 Sept 2023 at 15:52, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Naushir Patuck via libcamera-devel (2023-06-26 15:39:32) > > Enable embedded data parsing for the imx296 sensor. > > > > However, the imx296 has a quirk where the embedded data is ahead by a > > single frame, i.e. embedded data in frame N corresponds to the image > > data in frame N+1. So make a copy of the embedded data buffer stored in > > the camera helper state, and use that copy in the next frame. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Anything blocking this patch? > > It looks like there was small discussion on v1 and that was handled with > an update noted below. This seems reasonable to me here... so The only reason to hold back is that the imx296 kernel driver does not currently enable/advertise embedded data. But the pipeline handler code should handle this so.... I am ok to merge this now. > > > > --- > > Changes since v1: > > - Replace the use of std::unique_ptr with std::vector<uint8_t> > > --- > > src/ipa/rpi/cam_helper/cam_helper_imx296.cpp | 60 +++++++++++++++++++- > > 1 file changed, 59 insertions(+), 1 deletion(-) > > > > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > > index ecb845e76e12..e84243704e5a 100644 > > --- a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > > @@ -7,7 +7,9 @@ > > > > #include <algorithm> > > #include <cmath> > > +#include <string.h> > > #include <stddef.h> > > +#include <vector> > > > > #include "cam_helper.h" > > > > @@ -15,6 +17,17 @@ using namespace RPiController; > > using libcamera::utils::Duration; > > using namespace std::literals::chrono_literals; > > > > +constexpr uint32_t gainReg = 0x41ba; > > +constexpr uint32_t shsLoReg = 0x41b4; > > +constexpr uint32_t shsMidReg = 0x41b5; > > +constexpr uint32_t shsHiReg = 0x41b6; > > +constexpr uint32_t vmaxLoReg = 0x41cc; > > +constexpr uint32_t vmaxMidReg = 0x41cd; > > +constexpr uint32_t vmaxHiReg = 0x41ce; > > +constexpr uint32_t tempReg = 0x41da; > > +constexpr std::initializer_list<uint32_t> registerList = > > + { gainReg, shsLoReg, shsMidReg, shsHiReg, vmaxLoReg, vmaxMidReg, vmaxHiReg, tempReg }; > > + > > class CamHelperImx296 : public CamHelper > > { > > public: > > @@ -23,8 +36,12 @@ public: > > double gain(uint32_t gainCode) const override; > > uint32_t exposureLines(const Duration exposure, const Duration lineLength) const override; > > Duration exposure(uint32_t exposureLines, const Duration lineLength) const override; > > + void prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) override; > > void getDelays(int &exposureDelay, int &gainDelay, > > int &vblankDelay, int &hblankDelay) const override; > > + bool sensorEmbeddedDataPresent() const override; > > + void populateMetadata(const MdParser::RegisterMap ®isters, > > + Metadata &metadata) const override; > > > > private: > > static constexpr uint32_t minExposureLines = 1; > > @@ -36,10 +53,12 @@ private: > > * in units of lines. > > */ > > static constexpr int frameIntegrationDiff = 4; > > + > > + std::vector<uint8_t> lastEmbeddedBuffer_; > > }; > > > > CamHelperImx296::CamHelperImx296() > > - : CamHelper(nullptr, frameIntegrationDiff) > > + : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff) > > { > > } > > > > @@ -66,6 +85,23 @@ Duration CamHelperImx296::exposure(uint32_t exposureLines, > > return std::max<uint32_t>(minExposureLines, exposureLines) * timePerLine + 14.26us; > > } > > > > +void CamHelperImx296::prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) > > +{ > > + /* > > + * The imx296 embedded data is ahead by a single frame, i.e. embedded > > + * data in frame N corresponds to the image data in frame N+1. So make > > + * a copy of the embedded data buffer and use it as normal for the next > > + * frame. > > + */ > > + CamHelper::prepare({ lastEmbeddedBuffer_.data(), lastEmbeddedBuffer_.size() }, > > + metadata); > > + > > + if (lastEmbeddedBuffer_.size() != buffer.size()) > > + lastEmbeddedBuffer_.resize(buffer.size()); > > + > > + memcpy(lastEmbeddedBuffer_.data(), buffer.data(), buffer.size()); > > +} > > + > > void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay, > > int &vblankDelay, int &hblankDelay) const > > { > > @@ -75,6 +111,28 @@ void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay, > > hblankDelay = 2; > > } > > > > +bool CamHelperImx296::sensorEmbeddedDataPresent() const > > +{ > > + return true; > > +} > > + > > +void CamHelperImx296::populateMetadata(const MdParser::RegisterMap ®isters, > > + Metadata &metadata) const > > +{ > > + uint32_t shs = registers.at(shsLoReg) + (registers.at(shsMidReg) << 8) + > > + (registers.at(shsHiReg) << 16); > > + uint32_t vmax = registers.at(vmaxLoReg) + (registers.at(vmaxMidReg) << 8) + > > + (registers.at(vmaxHiReg) << 16); > > Maybe in the future we could add some register access helpers to make > this easier? But I don't see that blocking here. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + > > + DeviceStatus deviceStatus; > > + deviceStatus.lineLength = timePerLine; > > + deviceStatus.frameLength = vmax; > > + deviceStatus.shutterSpeed = exposure(vmax - shs, timePerLine); > > + deviceStatus.analogueGain = gain(registers.at(gainReg)); > > + > > + metadata.set("device.status", deviceStatus); > > +} > > + > > static CamHelper *create() > > { > > return new CamHelperImx296(); > > -- > > 2.34.1 > >
Quoting Naushir Patuck (2023-09-25 16:06:07) > Hi Kieran, > > On Mon, 25 Sept 2023 at 15:52, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting Naushir Patuck via libcamera-devel (2023-06-26 15:39:32) > > > Enable embedded data parsing for the imx296 sensor. > > > > > > However, the imx296 has a quirk where the embedded data is ahead by a > > > single frame, i.e. embedded data in frame N corresponds to the image > > > data in frame N+1. So make a copy of the embedded data buffer stored in > > > the camera helper state, and use that copy in the next frame. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Anything blocking this patch? > > > > It looks like there was small discussion on v1 and that was handled with > > an update noted below. This seems reasonable to me here... so > > The only reason to hold back is that the imx296 kernel driver does not > currently enable/advertise embedded data. But the pipeline handler > code should handle this so.... I am ok to merge this now. Well I'll leave that to an additional tag appearing! -- Kieran > > > > > > > > --- > > > Changes since v1: > > > - Replace the use of std::unique_ptr with std::vector<uint8_t> > > > --- > > > src/ipa/rpi/cam_helper/cam_helper_imx296.cpp | 60 +++++++++++++++++++- > > > 1 file changed, 59 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > > > index ecb845e76e12..e84243704e5a 100644 > > > --- a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > > > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > > > @@ -7,7 +7,9 @@ > > > > > > #include <algorithm> > > > #include <cmath> > > > +#include <string.h> > > > #include <stddef.h> > > > +#include <vector> > > > > > > #include "cam_helper.h" > > > > > > @@ -15,6 +17,17 @@ using namespace RPiController; > > > using libcamera::utils::Duration; > > > using namespace std::literals::chrono_literals; > > > > > > +constexpr uint32_t gainReg = 0x41ba; > > > +constexpr uint32_t shsLoReg = 0x41b4; > > > +constexpr uint32_t shsMidReg = 0x41b5; > > > +constexpr uint32_t shsHiReg = 0x41b6; > > > +constexpr uint32_t vmaxLoReg = 0x41cc; > > > +constexpr uint32_t vmaxMidReg = 0x41cd; > > > +constexpr uint32_t vmaxHiReg = 0x41ce; > > > +constexpr uint32_t tempReg = 0x41da; > > > +constexpr std::initializer_list<uint32_t> registerList = > > > + { gainReg, shsLoReg, shsMidReg, shsHiReg, vmaxLoReg, vmaxMidReg, vmaxHiReg, tempReg }; > > > + > > > class CamHelperImx296 : public CamHelper > > > { > > > public: > > > @@ -23,8 +36,12 @@ public: > > > double gain(uint32_t gainCode) const override; > > > uint32_t exposureLines(const Duration exposure, const Duration lineLength) const override; > > > Duration exposure(uint32_t exposureLines, const Duration lineLength) const override; > > > + void prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) override; > > > void getDelays(int &exposureDelay, int &gainDelay, > > > int &vblankDelay, int &hblankDelay) const override; > > > + bool sensorEmbeddedDataPresent() const override; > > > + void populateMetadata(const MdParser::RegisterMap ®isters, > > > + Metadata &metadata) const override; > > > > > > private: > > > static constexpr uint32_t minExposureLines = 1; > > > @@ -36,10 +53,12 @@ private: > > > * in units of lines. > > > */ > > > static constexpr int frameIntegrationDiff = 4; > > > + > > > + std::vector<uint8_t> lastEmbeddedBuffer_; > > > }; > > > > > > CamHelperImx296::CamHelperImx296() > > > - : CamHelper(nullptr, frameIntegrationDiff) > > > + : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff) > > > { > > > } > > > > > > @@ -66,6 +85,23 @@ Duration CamHelperImx296::exposure(uint32_t exposureLines, > > > return std::max<uint32_t>(minExposureLines, exposureLines) * timePerLine + 14.26us; > > > } > > > > > > +void CamHelperImx296::prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) > > > +{ > > > + /* > > > + * The imx296 embedded data is ahead by a single frame, i.e. embedded > > > + * data in frame N corresponds to the image data in frame N+1. So make > > > + * a copy of the embedded data buffer and use it as normal for the next > > > + * frame. > > > + */ > > > + CamHelper::prepare({ lastEmbeddedBuffer_.data(), lastEmbeddedBuffer_.size() }, > > > + metadata); > > > + > > > + if (lastEmbeddedBuffer_.size() != buffer.size()) > > > + lastEmbeddedBuffer_.resize(buffer.size()); > > > + > > > + memcpy(lastEmbeddedBuffer_.data(), buffer.data(), buffer.size()); > > > +} > > > + > > > void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay, > > > int &vblankDelay, int &hblankDelay) const > > > { > > > @@ -75,6 +111,28 @@ void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay, > > > hblankDelay = 2; > > > } > > > > > > +bool CamHelperImx296::sensorEmbeddedDataPresent() const > > > +{ > > > + return true; > > > +} > > > + > > > +void CamHelperImx296::populateMetadata(const MdParser::RegisterMap ®isters, > > > + Metadata &metadata) const > > > +{ > > > + uint32_t shs = registers.at(shsLoReg) + (registers.at(shsMidReg) << 8) + > > > + (registers.at(shsHiReg) << 16); > > > + uint32_t vmax = registers.at(vmaxLoReg) + (registers.at(vmaxMidReg) << 8) + > > > + (registers.at(vmaxHiReg) << 16); > > > > Maybe in the future we could add some register access helpers to make > > this easier? But I don't see that blocking here. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > + > > > + DeviceStatus deviceStatus; > > > + deviceStatus.lineLength = timePerLine; > > > + deviceStatus.frameLength = vmax; > > > + deviceStatus.shutterSpeed = exposure(vmax - shs, timePerLine); > > > + deviceStatus.analogueGain = gain(registers.at(gainReg)); > > > + > > > + metadata.set("device.status", deviceStatus); > > > +} > > > + > > > static CamHelper *create() > > > { > > > return new CamHelperImx296(); > > > -- > > > 2.34.1 > > >
diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp index ecb845e76e12..e84243704e5a 100644 --- a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp +++ b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp @@ -7,7 +7,9 @@ #include <algorithm> #include <cmath> +#include <string.h> #include <stddef.h> +#include <vector> #include "cam_helper.h" @@ -15,6 +17,17 @@ using namespace RPiController; using libcamera::utils::Duration; using namespace std::literals::chrono_literals; +constexpr uint32_t gainReg = 0x41ba; +constexpr uint32_t shsLoReg = 0x41b4; +constexpr uint32_t shsMidReg = 0x41b5; +constexpr uint32_t shsHiReg = 0x41b6; +constexpr uint32_t vmaxLoReg = 0x41cc; +constexpr uint32_t vmaxMidReg = 0x41cd; +constexpr uint32_t vmaxHiReg = 0x41ce; +constexpr uint32_t tempReg = 0x41da; +constexpr std::initializer_list<uint32_t> registerList = + { gainReg, shsLoReg, shsMidReg, shsHiReg, vmaxLoReg, vmaxMidReg, vmaxHiReg, tempReg }; + class CamHelperImx296 : public CamHelper { public: @@ -23,8 +36,12 @@ public: double gain(uint32_t gainCode) const override; uint32_t exposureLines(const Duration exposure, const Duration lineLength) const override; Duration exposure(uint32_t exposureLines, const Duration lineLength) const override; + void prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) override; void getDelays(int &exposureDelay, int &gainDelay, int &vblankDelay, int &hblankDelay) const override; + bool sensorEmbeddedDataPresent() const override; + void populateMetadata(const MdParser::RegisterMap ®isters, + Metadata &metadata) const override; private: static constexpr uint32_t minExposureLines = 1; @@ -36,10 +53,12 @@ private: * in units of lines. */ static constexpr int frameIntegrationDiff = 4; + + std::vector<uint8_t> lastEmbeddedBuffer_; }; CamHelperImx296::CamHelperImx296() - : CamHelper(nullptr, frameIntegrationDiff) + : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff) { } @@ -66,6 +85,23 @@ Duration CamHelperImx296::exposure(uint32_t exposureLines, return std::max<uint32_t>(minExposureLines, exposureLines) * timePerLine + 14.26us; } +void CamHelperImx296::prepare(libcamera::Span<const uint8_t> buffer, Metadata &metadata) +{ + /* + * The imx296 embedded data is ahead by a single frame, i.e. embedded + * data in frame N corresponds to the image data in frame N+1. So make + * a copy of the embedded data buffer and use it as normal for the next + * frame. + */ + CamHelper::prepare({ lastEmbeddedBuffer_.data(), lastEmbeddedBuffer_.size() }, + metadata); + + if (lastEmbeddedBuffer_.size() != buffer.size()) + lastEmbeddedBuffer_.resize(buffer.size()); + + memcpy(lastEmbeddedBuffer_.data(), buffer.data(), buffer.size()); +} + void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay, int &vblankDelay, int &hblankDelay) const { @@ -75,6 +111,28 @@ void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay, hblankDelay = 2; } +bool CamHelperImx296::sensorEmbeddedDataPresent() const +{ + return true; +} + +void CamHelperImx296::populateMetadata(const MdParser::RegisterMap ®isters, + Metadata &metadata) const +{ + uint32_t shs = registers.at(shsLoReg) + (registers.at(shsMidReg) << 8) + + (registers.at(shsHiReg) << 16); + uint32_t vmax = registers.at(vmaxLoReg) + (registers.at(vmaxMidReg) << 8) + + (registers.at(vmaxHiReg) << 16); + + DeviceStatus deviceStatus; + deviceStatus.lineLength = timePerLine; + deviceStatus.frameLength = vmax; + deviceStatus.shutterSpeed = exposure(vmax - shs, timePerLine); + deviceStatus.analogueGain = gain(registers.at(gainReg)); + + metadata.set("device.status", deviceStatus); +} + static CamHelper *create() { return new CamHelperImx296();
Enable embedded data parsing for the imx296 sensor. However, the imx296 has a quirk where the embedded data is ahead by a single frame, i.e. embedded data in frame N corresponds to the image data in frame N+1. So make a copy of the embedded data buffer stored in the camera helper state, and use that copy in the next frame. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- Changes since v1: - Replace the use of std::unique_ptr with std::vector<uint8_t> --- src/ipa/rpi/cam_helper/cam_helper_imx296.cpp | 60 +++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-)