[libcamera-devel] [PATCH v2] ipa: rpi: imx296: Enable embedded data

[Kieran Bingham]

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 at 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 &registers,
> +                             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 &registers,
> +                                      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 at 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
>