| Message ID | 20231102180916.3575006-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 = {},
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 >
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 > >
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 = {},
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 > > >
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 = {},
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
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 = {},
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