[libcamera-devel] [PATCH] libcamera: ipa: Add IMX335 support

[Kieran Bingham]

Hi Naush,

Quoting Naushir Patuck (2023-11-03 09:30:29)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, 2 Nov 2023 at 18:09, Kieran Bingham via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > Provide support for the Sony IMX335 in both libipa and RaspberryPi IPA
> > modules.
> 
> Without an imx335.json camera tuning file present, this will not work
> on the RPi platform.  Was that file meant to be added in this patch?

So far I've used only an unmodified 'uncalibrated.json' for both RPi4
and Pi5.

I've now successfully tested the module and updated kernel driver with
both 2 and 4 lane operation (2 lane on RPi4, 4 lane on Pi5).

I haven't yet gone through a full 'tuning' process on Raspberry Pi
though as it's not my target platform for this camera module. But it's
/very/ helpful that I can test on Raspberry Pi.

It gives good results even using uncalibrated.json on both platforms
too! (Which I believe is a testament to Raspberry Pi's IPA development!)


I would always like to think that 'every camera' which can be connected
should be supported on all platforms. But I don't know if that means to
get a camera into libcamera it should be tuned for each platform
explicitly?


I'm weary of providing a Raspberry Pi 'tuning' file that implies I have
performed any full tuning on the module. What are your thoughts about
defaulting to use the 'uncalibrated.json' when no tuning file is
identified (and printing a warning to report this?)

Otherwise I would be submitting essentially
  'cp uncalibrated.json imx335.json'

And that would perhaps give potential future users undue belief in the
tuning file. I'm going to go through the tuning process on a Pi now -
but even with that - this would only be a very 'home lab' basic effort.

--
Regards

Kieran


> 
> Regards,
> Naush
> 
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  src/ipa/libipa/camera_sensor_helper.cpp      | 23 ++++++
> >  src/ipa/rpi/cam_helper/cam_helper_imx335.cpp | 74 ++++++++++++++++++++
> >  src/ipa/rpi/cam_helper/meson.build           |  1 +
> >  src/libcamera/camera_sensor_properties.cpp   |  4 ++
> >  4 files changed, 102 insertions(+)
> >  create mode 100644 src/ipa/rpi/cam_helper/cam_helper_imx335.cpp
> >
> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > index f0ecc3830115..ddab5af6eac2 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > @@ -444,6 +444,29 @@ class CameraSensorHelperImx327 : public CameraSensorHelperImx290
> >  };
> >  REGISTER_CAMERA_SENSOR_HELPER("imx327", CameraSensorHelperImx327)
> >
> > +class CameraSensorHelperImx335 : public CameraSensorHelper
> > +{
> > +public:
> > +       uint32_t gainCode(double gain) const override;
> > +       double gain(uint32_t gainCode) const override;
> > +private:
> > +       static constexpr uint32_t maxGainCode_ = 240;
> > +};
> > +
> > +uint32_t CameraSensorHelperImx335::gainCode(double gain) const
> > +{
> > +       uint32_t code = 10 * std::log10(gain) * 10 / 3;
> > +
> > +       return std::min(code, maxGainCode_);
> > +}
> > +
> > +double CameraSensorHelperImx335::gain(uint32_t gainCode) const
> > +{
> > +       return std::pow(10.0, gainCode / (10 * 10 / 3));
> > +}
> > +
> > +REGISTER_CAMERA_SENSOR_HELPER("imx335", CameraSensorHelperImx335)
> > +
> >  class CameraSensorHelperImx477 : public CameraSensorHelper
> >  {
> >  public:
> > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx335.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx335.cpp
> > new file mode 100644
> > index 000000000000..659c69d6b6c7
> > --- /dev/null
> > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx335.cpp
> > @@ -0,0 +1,74 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2023, Ideas on Board Oy.
> > + *
> > + * cam_helper_imx335.cpp - camera information for the Sony IMX335 sensor
> > + */
> > +
> > +#include <assert.h>
> > +
> > +#include "cam_helper.h"
> > +#include "math.h"
> > +
> > +using namespace RPiController;
> > +
> > +class CamHelperImx335 : public CamHelper
> > +{
> > +public:
> > +       CamHelperImx335();
> > +       uint32_t gainCode(double gain) const override;
> > +       double gain(uint32_t gainCode) const override;
> > +       void getDelays(int &exposureDelay, int &gainDelay,
> > +                      int &vblankDelay, int &hblankDelay) const override;
> > +       unsigned int hideFramesModeSwitch() const override;
> > +
> > +private:
> > +       /*
> > +        * Smallest difference between the frame length and integration time,
> > +        * in units of lines.
> > +        */
> > +       static constexpr int frameIntegrationDiff = 4;
> > +       static constexpr uint32_t maxGainCode = 240;
> > +};
> > +
> > +/*
> > + * IMX335 Metadata isn't yet supported.
> > + */
> > +
> > +CamHelperImx335::CamHelperImx335()
> > +       : CamHelper({}, frameIntegrationDiff)
> > +{
> > +}
> > +
> > +uint32_t CamHelperImx335::gainCode(double gain) const
> > +{
> > +       uint32_t code = 10 * std::log10(gain) * 10 / 3;
> > +       return std::min(code, maxGainCode);
> > +}
> > +
> > +double CamHelperImx335::gain(uint32_t gainCode) const
> > +{
> > +       return std::pow(10.0, gainCode / (10 * 10 / 3));
> > +}
> > +
> > +void CamHelperImx335::getDelays(int &exposureDelay, int &gainDelay,
> > +                               int &vblankDelay, int &hblankDelay) const
> > +{
> > +       exposureDelay = 2;
> > +       gainDelay = 2;
> > +       vblankDelay = 2;
> > +       hblankDelay = 2;
> > +}
> > +
> > +unsigned int CamHelperImx335::hideFramesModeSwitch() const
> > +{
> > +       /* One bad frame can be expected after a mode switch. */
> > +       return 1;
> > +}
> > +
> > +static CamHelper *create()
> > +{
> > +       return new CamHelperImx335();
> > +}
> > +
> > +static RegisterCamHelper reg("imx335", &create);
> > diff --git a/src/ipa/rpi/cam_helper/meson.build b/src/ipa/rpi/cam_helper/meson.build
> > index bdf2db8eb742..17c25cb0e4a6 100644
> > --- a/src/ipa/rpi/cam_helper/meson.build
> > +++ b/src/ipa/rpi/cam_helper/meson.build
> > @@ -6,6 +6,7 @@ rpi_ipa_cam_helper_sources = files([
> >      'cam_helper_imx219.cpp',
> >      'cam_helper_imx290.cpp',
> >      'cam_helper_imx296.cpp',
> > +    'cam_helper_imx335.cpp',
> >      'cam_helper_imx477.cpp',
> >      'cam_helper_imx519.cpp',
> >      'cam_helper_imx708.cpp',
> > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> > index 27d6799a2686..dc76051fa349 100644
> > --- a/src/libcamera/camera_sensor_properties.cpp
> > +++ b/src/libcamera/camera_sensor_properties.cpp
> > @@ -111,6 +111,10 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> >                         .unitCellSize = { 2900, 2900 },
> >                         .testPatternModes = {},
> >                 } },
> > +               { "imx335", {
> > +                       .unitCellSize = { 2000, 2000 },
> > +                       .testPatternModes = {},
> > +               } },
> >                 { "imx477", {
> >                         .unitCellSize = { 1550, 1550 },
> >                         .testPatternModes = {},
> > --
> > 2.34.1
> >