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

[Kieran Bingham]

Quoting Naushir Patuck (2023-09-25 16:06:07)
> 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.

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