[libcamera-devel] libcamera: ipa: Add IMX335 support
diff mbox series

Message ID 20231102180916.3575006-1-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • [libcamera-devel] libcamera: ipa: Add IMX335 support
Related show

Commit Message

Kieran Bingham Nov. 2, 2023, 6:09 p.m. UTC 
Provide support for the Sony IMX335 in both libipa and RaspberryPi IPA
modules.

Signed-off-by: Kieran Bingham <kieran.bingham@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

Comments

Laurent Pinchart Nov. 2, 2023, 6:41 p.m. UTC | #1 
Hi Kieran,

Thank you for the patch.

On Thu, Nov 02, 2023 at 06:09:16PM +0000, Kieran Bingham via libcamera-devel wrote:
> Provide support for the Sony IMX335 in both libipa and RaspberryPi IPA
> modules.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@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;

That looks like an exponential model, there's a helper for that.

> +
> +	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;

Have you validated all these delays ?

> +}
> +
> +unsigned int CamHelperImx335::hideFramesModeSwitch() const
> +{
> +	/* One bad frame can be expected after a mode switch. */
> +	return 1;

Same here.

> +}
> +
> +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 = {},
Naushir Patuck Nov. 3, 2023, 9:30 a.m. UTC | #2 
Hi Kieran,

Thank you for the patch.

On Thu, 2 Nov 2023 at 18:09, Kieran Bingham via libcamera-devel
<libcamera-devel@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?

Regards,
Naush

>
> Signed-off-by: Kieran Bingham <kieran.bingham@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
>
Kieran Bingham Nov. 6, 2023, 10:17 p.m. UTC | #3 
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@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@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
> >
Laurent Pinchart Nov. 7, 2023, 8:50 a.m. UTC | #4 
On Mon, Nov 06, 2023 at 10:17:58PM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Naushir Patuck (2023-11-03 09:30:29)
> > On Thu, 2 Nov 2023 at 18:09, Kieran Bingham via libcamera-devel 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 don't think that would be a reasonable expectation, it just wouldn't
scale.

There's a difference between teaching libcamera about intrinsic
properties of a camera sensor, which by definition are not dependent on
the platform it is connected to, and providing tuning data that covers
the combination of a camera module and an ISP. The former should simply
be a matter of updating a shared implementation, without a need to test
on all platforms, while the latter requires per-platform work.

One issue we have today is that there are, for historical reasons, two
sets of helpers for camera sensors, one in libipa, and one in the
Raspberry Pi IPA. It seems to be time to add missing information to the
former and drop the latter. I'm tempted to give a strong incentive in
that direction by refusing new additions to the Raspberry Pi-specific
camera helpers.

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

Defaulting to uncalibrated.json is what we do on some other platforms,
and I think it's better than pretending we have tuned a particular
camera module for a Pi 4 or Pi 5 board.

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

That's of course useful too :-) It just shouldn't be a hard requirement.

> > > Signed-off-by: Kieran Bingham <kieran.bingham@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 = {},
Naushir Patuck Nov. 7, 2023, 8:53 a.m. UTC | #5 
Hi Kieran,

On Mon, 6 Nov 2023 at 22:18, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> 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@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?

T

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

I would prefer not to do this.  The uncalibrated.json file is intended
to be used for bringup and get libcamera running enough to capture DNG
files for tuning purposes.  IMO, I would be hesitant to add partial
(untuned in this case) support for sensors as inevitably someone is
going to run this sensor, see bad IQ and complain to Raspberry Pi
about it.

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

Our tuning process is meant to be simple enough that you should get a
reasonable image without all the equipment.  So you ought to get a
reasonable output from the tuning tool, then we'd be able to merge the
tuning file for full support of the sensor, even if the tuning is not
"optimal" in some respects.  Again, this is my opinion, David please
chime in if you think otherwise.

Regards,
Naush

>
> --
> Regards
>
> Kieran
>
>
> >
> > Regards,
> > Naush
> >
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham@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
> > >
Laurent Pinchart Nov. 9, 2023, 10:30 a.m. UTC | #6 
On Tue, Nov 07, 2023 at 10:50:24AM +0200, Laurent Pinchart via libcamera-devel wrote:
> On Mon, Nov 06, 2023 at 10:17:58PM +0000, Kieran Bingham via libcamera-devel wrote:
> > Quoting Naushir Patuck (2023-11-03 09:30:29)
> > > On Thu, 2 Nov 2023 at 18:09, Kieran Bingham via libcamera-devel 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 don't think that would be a reasonable expectation, it just wouldn't
> scale.
> 
> There's a difference between teaching libcamera about intrinsic
> properties of a camera sensor, which by definition are not dependent on
> the platform it is connected to, and providing tuning data that covers
> the combination of a camera module and an ISP. The former should simply
> be a matter of updating a shared implementation, without a need to test
> on all platforms, while the latter requires per-platform work.
> 
> One issue we have today is that there are, for historical reasons, two
> sets of helpers for camera sensors, one in libipa, and one in the
> Raspberry Pi IPA. It seems to be time to add missing information to the
> former and drop the latter. I'm tempted to give a strong incentive in
> that direction by refusing new additions to the Raspberry Pi-specific
> camera helpers.

Re-reading this, I realize the wording is more aggressive than I
intended it to be. To clarify my point, I don't call for refusing new
additions right now, blocking this patch. I do believe that it's time to
merge the two sets of helpers in one, as if we have an agreement on
that, and can agree on a concrete plan, then I'll be happy.

> > 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.
> 
> Defaulting to uncalibrated.json is what we do on some other platforms,
> and I think it's better than pretending we have tuned a particular
> camera module for a Pi 4 or Pi 5 board.
> 
> > 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.
> 
> That's of course useful too :-) It just shouldn't be a hard requirement.
> 
> > > > Signed-off-by: Kieran Bingham <kieran.bingham@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 = {},
Naushir Patuck Nov. 9, 2023, 11:13 a.m. UTC | #7 
On Thu, 9 Nov 2023 at 10:30, Laurent Pinchart via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> On Tue, Nov 07, 2023 at 10:50:24AM +0200, Laurent Pinchart via libcamera-devel wrote:
> > On Mon, Nov 06, 2023 at 10:17:58PM +0000, Kieran Bingham via libcamera-devel wrote:
> > > Quoting Naushir Patuck (2023-11-03 09:30:29)
> > > > On Thu, 2 Nov 2023 at 18:09, Kieran Bingham via libcamera-devel 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 don't think that would be a reasonable expectation, it just wouldn't
> > scale.
> >
> > There's a difference between teaching libcamera about intrinsic
> > properties of a camera sensor, which by definition are not dependent on
> > the platform it is connected to, and providing tuning data that covers
> > the combination of a camera module and an ISP. The former should simply
> > be a matter of updating a shared implementation, without a need to test
> > on all platforms, while the latter requires per-platform work.
> >
> > One issue we have today is that there are, for historical reasons, two
> > sets of helpers for camera sensors, one in libipa, and one in the
> > Raspberry Pi IPA. It seems to be time to add missing information to the
> > former and drop the latter. I'm tempted to give a strong incentive in
> > that direction by refusing new additions to the Raspberry Pi-specific
> > camera helpers.
>
> Re-reading this, I realize the wording is more aggressive than I
> intended it to be. To clarify my point, I don't call for refusing new
> additions right now, blocking this patch. I do believe that it's time to
> merge the two sets of helpers in one, as if we have an agreement on
> that, and can agree on a concrete plan, then I'll be happy.

Thank you for clarifying this!

Agree, it would be nice to consolidate these 2 helpers - hopefully we
can find a reasonably painless path forward to doing this.  They key
trouble I see is trying to keep our existing structures in-place since
we use them through all our IPA and algorithms source code.

Regards,
Naush



>
> > > 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.
> >
> > Defaulting to uncalibrated.json is what we do on some other platforms,
> > and I think it's better than pretending we have tuned a particular
> > camera module for a Pi 4 or Pi 5 board.
> >
> > > 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.
> >
> > That's of course useful too :-) It just shouldn't be a hard requirement.
> >
> > > > > Signed-off-by: Kieran Bingham <kieran.bingham@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 = {},
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

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 = {},