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

[Naushir Patuck]

Hi Kieran,

On Mon, 25 Sept 2023 at 15:52, Kieran Bingham
<kieran.bingham at 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 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

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 &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
> >