[libcamera-devel] [PATCH] libcamera: ipa: Add IMX335 support
Naushir Patuck
naush at raspberrypi.com
Tue Nov 7 09:53:53 CET 2023
Hi Kieran,
On Mon, 6 Nov 2023 at 22:18, Kieran Bingham
<kieran.bingham at 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 at lists.libcamera.org> wrote:
> > >
> > > Provide support for the Sony IMX335 in both libipa and RaspberryPi IPA
> > > modules.
> >
> > Without an imx335.json camera tuning file present, this will not work
> > on the RPi platform. Was that file meant to be added in this patch?
>
> So far I've used only an unmodified 'uncalibrated.json' for both RPi4
> and Pi5.
>
> I've now successfully tested the module and updated kernel driver with
> both 2 and 4 lane operation (2 lane on RPi4, 4 lane on Pi5).
>
> I haven't yet gone through a full 'tuning' process on Raspberry Pi
> though as it's not my target platform for this camera module. But it's
> /very/ helpful that I can test on Raspberry Pi.
>
> It gives good results even using uncalibrated.json on both platforms
> too! (Which I believe is a testament to Raspberry Pi's IPA development!)
>
>
> I would always like to think that 'every camera' which can be connected
> should be supported on all platforms. But I don't know if that means to
> get a camera into libcamera it should be tuned for each platform
> explicitly?
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 at ideasonboard.com>
> > > ---
> > > src/ipa/libipa/camera_sensor_helper.cpp | 23 ++++++
> > > src/ipa/rpi/cam_helper/cam_helper_imx335.cpp | 74 ++++++++++++++++++++
> > > src/ipa/rpi/cam_helper/meson.build | 1 +
> > > src/libcamera/camera_sensor_properties.cpp | 4 ++
> > > 4 files changed, 102 insertions(+)
> > > create mode 100644 src/ipa/rpi/cam_helper/cam_helper_imx335.cpp
> > >
> > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > index f0ecc3830115..ddab5af6eac2 100644
> > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > @@ -444,6 +444,29 @@ class CameraSensorHelperImx327 : public CameraSensorHelperImx290
> > > };
> > > REGISTER_CAMERA_SENSOR_HELPER("imx327", CameraSensorHelperImx327)
> > >
> > > +class CameraSensorHelperImx335 : public CameraSensorHelper
> > > +{
> > > +public:
> > > + uint32_t gainCode(double gain) const override;
> > > + double gain(uint32_t gainCode) const override;
> > > +private:
> > > + static constexpr uint32_t maxGainCode_ = 240;
> > > +};
> > > +
> > > +uint32_t CameraSensorHelperImx335::gainCode(double gain) const
> > > +{
> > > + uint32_t code = 10 * std::log10(gain) * 10 / 3;
> > > +
> > > + return std::min(code, maxGainCode_);
> > > +}
> > > +
> > > +double CameraSensorHelperImx335::gain(uint32_t gainCode) const
> > > +{
> > > + return std::pow(10.0, gainCode / (10 * 10 / 3));
> > > +}
> > > +
> > > +REGISTER_CAMERA_SENSOR_HELPER("imx335", CameraSensorHelperImx335)
> > > +
> > > class CameraSensorHelperImx477 : public CameraSensorHelper
> > > {
> > > public:
> > > diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx335.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx335.cpp
> > > new file mode 100644
> > > index 000000000000..659c69d6b6c7
> > > --- /dev/null
> > > +++ b/src/ipa/rpi/cam_helper/cam_helper_imx335.cpp
> > > @@ -0,0 +1,74 @@
> > > +/* SPDX-License-Identifier: BSD-2-Clause */
> > > +/*
> > > + * Copyright (C) 2023, Ideas on Board Oy.
> > > + *
> > > + * cam_helper_imx335.cpp - camera information for the Sony IMX335 sensor
> > > + */
> > > +
> > > +#include <assert.h>
> > > +
> > > +#include "cam_helper.h"
> > > +#include "math.h"
> > > +
> > > +using namespace RPiController;
> > > +
> > > +class CamHelperImx335 : public CamHelper
> > > +{
> > > +public:
> > > + CamHelperImx335();
> > > + uint32_t gainCode(double gain) const override;
> > > + double gain(uint32_t gainCode) const override;
> > > + void getDelays(int &exposureDelay, int &gainDelay,
> > > + int &vblankDelay, int &hblankDelay) const override;
> > > + unsigned int hideFramesModeSwitch() const override;
> > > +
> > > +private:
> > > + /*
> > > + * Smallest difference between the frame length and integration time,
> > > + * in units of lines.
> > > + */
> > > + static constexpr int frameIntegrationDiff = 4;
> > > + static constexpr uint32_t maxGainCode = 240;
> > > +};
> > > +
> > > +/*
> > > + * IMX335 Metadata isn't yet supported.
> > > + */
> > > +
> > > +CamHelperImx335::CamHelperImx335()
> > > + : CamHelper({}, frameIntegrationDiff)
> > > +{
> > > +}
> > > +
> > > +uint32_t CamHelperImx335::gainCode(double gain) const
> > > +{
> > > + uint32_t code = 10 * std::log10(gain) * 10 / 3;
> > > + return std::min(code, maxGainCode);
> > > +}
> > > +
> > > +double CamHelperImx335::gain(uint32_t gainCode) const
> > > +{
> > > + return std::pow(10.0, gainCode / (10 * 10 / 3));
> > > +}
> > > +
> > > +void CamHelperImx335::getDelays(int &exposureDelay, int &gainDelay,
> > > + int &vblankDelay, int &hblankDelay) const
> > > +{
> > > + exposureDelay = 2;
> > > + gainDelay = 2;
> > > + vblankDelay = 2;
> > > + hblankDelay = 2;
> > > +}
> > > +
> > > +unsigned int CamHelperImx335::hideFramesModeSwitch() const
> > > +{
> > > + /* One bad frame can be expected after a mode switch. */
> > > + return 1;
> > > +}
> > > +
> > > +static CamHelper *create()
> > > +{
> > > + return new CamHelperImx335();
> > > +}
> > > +
> > > +static RegisterCamHelper reg("imx335", &create);
> > > diff --git a/src/ipa/rpi/cam_helper/meson.build b/src/ipa/rpi/cam_helper/meson.build
> > > index bdf2db8eb742..17c25cb0e4a6 100644
> > > --- a/src/ipa/rpi/cam_helper/meson.build
> > > +++ b/src/ipa/rpi/cam_helper/meson.build
> > > @@ -6,6 +6,7 @@ rpi_ipa_cam_helper_sources = files([
> > > 'cam_helper_imx219.cpp',
> > > 'cam_helper_imx290.cpp',
> > > 'cam_helper_imx296.cpp',
> > > + 'cam_helper_imx335.cpp',
> > > 'cam_helper_imx477.cpp',
> > > 'cam_helper_imx519.cpp',
> > > 'cam_helper_imx708.cpp',
> > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> > > index 27d6799a2686..dc76051fa349 100644
> > > --- a/src/libcamera/camera_sensor_properties.cpp
> > > +++ b/src/libcamera/camera_sensor_properties.cpp
> > > @@ -111,6 +111,10 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > .unitCellSize = { 2900, 2900 },
> > > .testPatternModes = {},
> > > } },
> > > + { "imx335", {
> > > + .unitCellSize = { 2000, 2000 },
> > > + .testPatternModes = {},
> > > + } },
> > > { "imx477", {
> > > .unitCellSize = { 1550, 1550 },
> > > .testPatternModes = {},
> > > --
> > > 2.34.1
> > >
More information about the libcamera-devel
mailing list