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