| Message ID | 20230621123712.7787-1-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Wed, Jun 21, 2023 at 01:37:12PM +0100, Naushir Patuck via libcamera-devel wrote: > Enable embedded data parising for the imx296 sensor. s/parising/parsing/ > 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. We wouldn't make a copy if this was image data, but would instead keep the buffer until the next frame. Would it make sense to do that even if the amount of data is smaller ? > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/rpi/cam_helper/cam_helper_imx296.cpp | 61 +++++++++++++++++++- > 1 file changed, 60 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..8f06981b500e 100644 > --- a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > @@ -7,6 +7,7 @@ > > #include <algorithm> > #include <cmath> > +#include <string.h> > #include <stddef.h> > > #include "cam_helper.h" > @@ -15,6 +16,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 +35,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 +52,13 @@ private: > * in units of lines. > */ > static constexpr int frameIntegrationDiff = 4; > + > + std::unique_ptr<uint8_t[]> lastEmbeddedBuffer_; > + libcamera::Span<uint8_t> embeddedBufferSpan_; > }; > > CamHelperImx296::CamHelperImx296() > - : CamHelper(nullptr, frameIntegrationDiff) > + : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff) > { > } > > @@ -66,6 +85,24 @@ 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(embeddedBufferSpan_, metadata); > + > + if (embeddedBufferSpan_.size() != buffer.size()) { > + lastEmbeddedBuffer_ = std::make_unique<uint8_t[]>(buffer.size()); > + embeddedBufferSpan_ = libcamera::Span(lastEmbeddedBuffer_.get(), buffer.size()); > + } > + > + memcpy(embeddedBufferSpan_.data(), buffer.data(), embeddedBufferSpan_.size()); If you made lastEmbeddedBuffer_ a std::vector<uint8_t> you could simplify this code and drop embeddedBufferSpan_. > +} > + > void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay, > int &vblankDelay, int &hblankDelay) const > { > @@ -75,6 +112,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();
Hi Laurent, Thank you for the feedback. On Sat, 24 Jun 2023 at 01:09, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > Thank you for the patch. > > On Wed, Jun 21, 2023 at 01:37:12PM +0100, Naushir Patuck via libcamera-devel wrote: > > Enable embedded data parising for the imx296 sensor. > > s/parising/parsing/ > > > 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. > > We wouldn't make a copy if this was image data, but would instead keep > the buffer until the next frame. Would it make sense to do that even if > the amount of data is smaller ? I did consider this approach but abandoned it for simplicity. The IPA (ipa_base.cpp) returns the buffer to the pipeline handler to recycle. If I wanted to defer this recycling, the IPA or pipeline handler needs to know about this specific sensor behaviour and do what needs to be done. Which means more API changes or adding sensor specific understanding in the IPA base - and that's something we want to avoid entirely. Given the size of the buffer is relatively small, a memcpy to simplify the change seems reasonable to me. > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/rpi/cam_helper/cam_helper_imx296.cpp | 61 +++++++++++++++++++- > > 1 file changed, 60 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..8f06981b500e 100644 > > --- a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp > > @@ -7,6 +7,7 @@ > > > > #include <algorithm> > > #include <cmath> > > +#include <string.h> > > #include <stddef.h> > > > > #include "cam_helper.h" > > @@ -15,6 +16,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 +35,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 +52,13 @@ private: > > * in units of lines. > > */ > > static constexpr int frameIntegrationDiff = 4; > > + > > + std::unique_ptr<uint8_t[]> lastEmbeddedBuffer_; > > + libcamera::Span<uint8_t> embeddedBufferSpan_; > > }; > > > > CamHelperImx296::CamHelperImx296() > > - : CamHelper(nullptr, frameIntegrationDiff) > > + : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff) > > { > > } > > > > @@ -66,6 +85,24 @@ 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(embeddedBufferSpan_, metadata); > > + > > + if (embeddedBufferSpan_.size() != buffer.size()) { > > + lastEmbeddedBuffer_ = std::make_unique<uint8_t[]>(buffer.size()); > > + embeddedBufferSpan_ = libcamera::Span(lastEmbeddedBuffer_.get(), buffer.size()); > > + } > > + > > + memcpy(embeddedBufferSpan_.data(), buffer.data(), embeddedBufferSpan_.size()); > > If you made lastEmbeddedBuffer_ a std::vector<uint8_t> you could > simplify this code and drop embeddedBufferSpan_. Yes, I can make that change. Will still have to construct a Span to pass into CamHelper::prepare above, but that can be done in-place. Regards, Naush > > > +} > > + > > void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay, > > int &vblankDelay, int &hblankDelay) const > > { > > @@ -75,6 +112,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(); > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp index ecb845e76e12..8f06981b500e 100644 --- a/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp +++ b/src/ipa/rpi/cam_helper/cam_helper_imx296.cpp @@ -7,6 +7,7 @@ #include <algorithm> #include <cmath> +#include <string.h> #include <stddef.h> #include "cam_helper.h" @@ -15,6 +16,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 +35,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 +52,13 @@ private: * in units of lines. */ static constexpr int frameIntegrationDiff = 4; + + std::unique_ptr<uint8_t[]> lastEmbeddedBuffer_; + libcamera::Span<uint8_t> embeddedBufferSpan_; }; CamHelperImx296::CamHelperImx296() - : CamHelper(nullptr, frameIntegrationDiff) + : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff) { } @@ -66,6 +85,24 @@ 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(embeddedBufferSpan_, metadata); + + if (embeddedBufferSpan_.size() != buffer.size()) { + lastEmbeddedBuffer_ = std::make_unique<uint8_t[]>(buffer.size()); + embeddedBufferSpan_ = libcamera::Span(lastEmbeddedBuffer_.get(), buffer.size()); + } + + memcpy(embeddedBufferSpan_.data(), buffer.data(), embeddedBufferSpan_.size()); +} + void CamHelperImx296::getDelays(int &exposureDelay, int &gainDelay, int &vblankDelay, int &hblankDelay) const { @@ -75,6 +112,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 parising 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> --- src/ipa/rpi/cam_helper/cam_helper_imx296.cpp | 61 +++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-)